docs(fetch_winhttp): add design document for WinHTTP custom transport - #595
Merged
Conversation
Design-only phase: introduces the Windows-only fetch_winhttp custom transport crate as a placeholder (no implementation yet) carrying the DESIGN.md architecture document for the WinHTTP-based fetch transport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Apply editorial restructuring from the final rubber-duck pass: - add a concise "Architecture at a glance" (section 1.3) so the mental model precedes the detail sections - deduplicate the threading (4), context-ownership (5.4/11.3), and connection-lifetime (7.5) explanations, cross-referencing the canonical section instead of re-deriving - convert 7.5's rejected alternatives and 14.2's per-factor test list into scannable tables, keeping prose only for the sequenced factors - refine 17 to distinguish network-phase timers (transport concern) from end-to-end pipeline deadlines Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Fixes flagged by a second rubber-duck editorial pass: - Remove client-cert benefit claim from S1 that implied v1 mTLS (contradicts S9.1) - State the full session-level callback notification mask once (S5.3) instead of only naming FLAG_HANDLES; reference it from the S3 table and test plan - Correct the S14.2 session-lifetime test to match S5.3: the body reader's Arc keeps the session wrapper alive in-flight, and native parent refcounting (not the Rust Arc) carries the OS session through the final HANDLE_CLOSING - Fix atomic cross-ref S5.1 -> S5.5 - Distinguish dropping the execute future vs the response body in the cancellation hazard and test plan - Clarify one-session-per-built-client wording (S1.3) - Point WinHttpOptions comment at S12 as well - Drop misleading '64-bit Windows' qualifier on DWORD_PTR - Clarify S16 boundary: observable HTTP behaviors are contractual; only the machinery is implementation detail - Deduplicate the 'one transport-scheduled delay' claim across S12 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
✅ Version increments look sufficient
|
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a new fetch_winhttp crate as a Windows-only fetch transport design artifact, allowing review of the proposed WinHTTP-based architecture before any implementation is added.
Changes:
- Adds the
fetch_winhttpcrate with a placeholderlib.rsand a detaileddocs/DESIGN.mddescribing the planned transport. - Wires the new crate into workspace metadata (workspace dependency list, root README and CHANGELOG, Cargo.lock).
- Updates workspace spellcheck dictionary for WinHTTP/design terminology used in the new documentation.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds fetch_winhttp to the top-level crate list. |
| crates/fetch_winhttp/src/lib.rs | Adds placeholder crate docs and rustdoc logo/favicon configuration. |
| crates/fetch_winhttp/README.md | Adds crate README describing scope/status and linking to the design doc. |
| crates/fetch_winhttp/logo.png | Adds crate logo asset via Git LFS pointer. |
| crates/fetch_winhttp/favicon.ico | Adds crate favicon asset via Git LFS pointer. |
| crates/fetch_winhttp/docs/DESIGN.md | Adds the WinHTTP transport design document and test plan. |
| crates/fetch_winhttp/CHANGELOG.md | Adds initial crate changelog stub. |
| crates/fetch_winhttp/Cargo.toml | Adds the new crate manifest and docs.rs metadata. |
| CHANGELOG.md | Adds fetch_winhttp to the workspace changelog index. |
| Cargo.toml | Adds fetch_winhttp to [workspace.dependencies]. |
| Cargo.lock | Adds the lockfile entry for the new crate. |
| .spelling | Adds WinHTTP- and design-related terms to the spellcheck allowlist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The design-only placeholder crate had no tests, so `cargo nextest run` failed with "no tests to run" (exit 4) in the pr-test CI job. Add a trivial placeholder test until the real transport and its test suite land. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
… in AGENTS - Reword the double-verb "is ... is" sentence in DESIGN.md S1.2. - AGENTS.md: state that auto-generated crate READMEs must not be hand-edited or flagged for formatting/wording/casing in review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
martintmk
reviewed
Jul 22, 2026
martintmk
reviewed
Jul 22, 2026
martintmk
reviewed
Jul 22, 2026
…ailable The `+ Send` bound on `bytesbuf_io`'s `ReadAsFuturesStream` in-flight read future is now merged, so state it as a provided guarantee rather than a merge-queue assumption (S11.2, S15). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
martintmk
reviewed
Jul 22, 2026
…ttp-design # Conflicts: # .spelling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 443 444 +1
Lines 43446 43447 +1
=======================================
+ Hits 43446 43447 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…action Both complaints were framed backwards. Restate them as fetch over-abstracting, not under-modeling: - TLS (S1.2, S17): different transports inherently support different TLS config models; that is a fact of life, not a fetch design choice. The shortcoming is fetch trying to configure TLS uniformly at the transport-agnostic level; it belongs on the transport. - Timeouts (S12.1, S17): fetch lacking fine-grained resolve/send timeouts is fine and expected - transports differ in which timers they support and what each includes/excludes. fetch should keep only pipeline-level deadlines and leave all network-phase timers to each transport, so exposing them via WinHttpOptions is the correct home, not a workaround. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…refs - Rewrite S5.3 to lead with the one ownership rule in bold, then three tight phases (before/after SetOption, validity), instead of a dense wall of prose. - Reference plurality's Box::into_raw/from_raw as available; drop the now-merged PR #585 links in S5.3 and S15. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Drop the pools' internal mechanics (EventPool's Arc-over-slab, MutexGuard-!Send reasoning, reference-counted backing details, thread-per-core aside) and keep only the contract-level facts: two transport-owned pools, what each rents, their Send/Sync properties, and why the plurality Pool needs a coarse uncontended Mutex. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…traction Reframe the S17 connection-management bullet to match the TLS/timeout framing and lead with max_connections as the concrete example: it presumes a fetch-managed pool, but different transports pool connections differently (WinHTTP owns its own pool with its own knobs), so there is no single model to configure uniformly at the fetch level - it belongs on the transport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
'Advisory' conflated two different cases. Replace with precise labels: - max_connections is 'Approximate' - the limit is applied but enforced per authority (WinHTTP) vs per pool (fetch), so a multi-host pool's effective cap differs. - connection_idle_timeout is 'Not honored' - WinHTTP exposes no idle-TTL knob, so the configured value has no effect and WinHTTP uses its own default. Also relabel the unhonored connection_lifetime rows and update the closing note. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…example Keep-alive is a property of how each transport's stack maintains its own connections (WinHTTP applies its own per-protocol keep-alive with its own floors), so include ConnectionKeepAlive alongside max_connections/connection_lifetime/ connection_idle_timeout as connection-management config that over-abstracts at the fetch level and belongs on the transport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…olicy The connection_lifetime-specific warning paragraph described a general behavior. Promote it to a shared policy subchapter (S17.1) under the fetch API feedback: for any configured option the transport cannot faithfully honor, warn at build time (tracing + telemetry) and proceed, rather than silently ignoring (hides a broken guarantee) or hard-erroring (breaks drop-in use). Condense S7.5 to reference S17.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…rk in S11.2 The sentence described something that is not done; it adds nothing and is not emphasizing anything. Start the paragraph with the substantive pull-based/ backpressure point instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…tive-timer backstop Rewrite S12.2 from a single dense paragraph into short, focused paragraphs and retitle it "The outer connect timeout" (it enforces a timeout, not a "delay"). Its core message is now up front: the native connect timer is per-attempt but fetch callers expect a total deadline, so the transport races the connect phase against one tick::Clock timer. Replace the unexplained "as backstop" note in the S12.1 table with a new S12.3 that states explicitly why the native receive timers are set: WinHTTP's own receive timers cannot be disabled, so we align them with the fetch value to keep the layers in agreement and to provide OS-level liveness if the caller's async executor stalls. Renumber the test subchapter to S12.4 and update cross-refs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Telemetry will go through the observed crate rather than tracing. Add an observed::Sink field to WinHttpDeps (it is Clone/ThreadAware, so it relocates per core like the rest of the config) and note the transport emits telemetry through it (detailed in v1.1). Replace the opentelemetry/tracing dependencies with observed and reword the S17.1 warning to emit an observed event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Resolve Cargo.toml [workspace.dependencies] conflict: origin/main merged the separate local-dependency block into the single alphabetical dependency list. Re-apply our only delta - the fetch_winhttp local dependency entry - in its alphabetical position after fetch_tls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
ralfbiedert
approved these changes
Jul 24, 2026
ralfbiedert
left a comment
ralfbiedert
left a comment
Collaborator
There was a problem hiding this comment.
Approved. The design is internally coherent, including FFI ownership, callback handoff, cancellation, and timeout reasoning. For implementation follow-up: keep the cross-crate design critique out of public rustdoc, verify docs.rs/fetch links, and hold the eventual unsafe FFI layer to the proposed mock/Miri validation.
Address PR review: a distinct "send timeout" is redundant - sending the request and awaiting response headers is the span ResponseTimeout already governs. Split the design.md §6.1 bullet into Send (not a distinct concept) and Resolve (the sole transport-specific knob), and update the implementation.md §10.4 timeout table and §7.2 test row accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
martintmk
approved these changes
Jul 27, 2026
martintmk
left a comment
martintmk
left a comment
Member
There was a problem hiding this comment.
I think this is good to go, fetch API design is different discussion
…lation, TLS/pool API design.md + implementation.md: - Connect timeout: single source of truth (TransportOptions.connect_timeout); drop the overloaded WinHttpOptions.connect_timeout from the example and state WinHttpOptions exposes no separate connect knob (D14). - Session isolation: open the OS session inside the per-core factory (build-scoped) instead of eagerly in builder_winhttp, so cloned builders no longer share a session/pool; document the per-core-session consequence and the missing per-built-client scope in fetch's custom API; add integration tests for cross-client pool isolation and for the four accept_invalid_certs x accept_invalid_hostnames combinations (D17, D18). - Note per-core instancing as a possible future knob, not in v1 scope (C). stabilization.md: aggregate reviewer input (collected for the stabilization phase, not agreed direction) - batteries-included default-transport ergonomics and NeedsTransport/DefaultTransport typestate (1), TLS-as-descriptors + silent-ignore hazard driving a TLS-config redesign (2, D15), unresolved pool ownership that may retire PoolIndex (3, D16), connect-timeout as a common knob (4). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
martin-kolinek
approved these changes
Jul 28, 2026
martin-kolinek
left a comment
martin-kolinek
left a comment
Collaborator
There was a problem hiding this comment.
The comments are related to the stabilization of fetch itself, not to the winhttp work - that looks good to me. I'd appreciate having the comments worked into the stabilization.md doc, but it's not a blocker.
Item 1: add reviewer input on whether the transport list must be open-ended (bundle fetch_winhttp directly) and a fetch_core/fetch crate split keeping fetch an enabler with defaults rather than a monolith. Item 7 (new): application-vs-library configuration responsibility - the three ownership models for indirect fetch use and why honoring the mixed model needs a transport abstraction richer than a Tower service. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
martin-kolinek
approved these changes
Jul 28, 2026
martin-kolinek
approved these changes
Jul 28, 2026
…s, future opportunities Reword two references left stale by the one-session -> per-core session change: the WinHttpOpen note (2.1) and the DISABLE_GLOBAL_POOLING pool scope (9.3) now describe per-core sessions/pools, matching 3.2. Add two 7.3 integration scenarios proving REDIRECT_POLICY_NEVER and DISABLE_COOKIES override WinHTTP defaults (302-to-sentinel never followed; Set-Cookie never replayed), not just that the options are set. Add chapter 14 Future opportunities durably recording the per-core vs single-session-per-client trade-off (and PoolIndex/instancing knobs) so they are revisited when the fetch API becomes more expressive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
Resolve Cargo.toml conflict: keep main's fetch_tls 0.2.6 bump (release #599) alongside the new fetch_winhttp 0.1.0 workspace dependency entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
sandersaares
enabled auto-merge (squash)
July 29, 2026 07:21
martintmk
approved these changes
Jul 29, 2026
Under the per-invocation session model, fetch invokes the transport factory once per pool slot (0..pool_count, client_builder.rs) and each invocation opens its own WinHTTP session; with DISABLE_GLOBAL_POOLING (per-session pooling) distinct PoolIndex slots already land in distinct sessions/pools. Ignoring the PoolIndex value therefore does not collapse multiple_pools onto one OS pool - the real v1 topology is one session/pool per (core x pool slot). Reword implementation.md 8, the chapter 14 future item, and fetch/docs/stabilization.md item 3 to describe the cores x pool slots resource profile instead of a single-pool collapse. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
…t) profile Follow-up to b733bc6. The 3.2 lifecycle summary, architecture-overview bullets, 9.3 pooling note, and the chapter 14 consolidation item still framed the topology as one session/pool per core. Reconcile them with the corrected 8: fetch invokes the factory once per core and once per multiple_pools slot (0..pool_count, one slot by default), so the profile is one session/pool per (core x pool slot), reducing to per-core in the default single-slot case. Add a 7.3 integration scenario that exercises the invariant the corrected design leans on: a single client built with multiple_pools(2) must not reuse a connection across slots (server-side connection counting), with an optional mock assertion that WinHttpOpen runs once per slot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8dbb7b85-e123-4586-8df2-a2a6a5b92194
psandana
reviewed
Jul 29, 2026
psandana
left a comment
psandana
left a comment
Contributor
There was a problem hiding this comment.
Design review notes from a walkthrough of this design PR (fetch_winhttp docs). See inline comments.
Member
Author
|
Going to merge this now - addressing some comments deferred to next phase (implementation). |
martin-kolinek
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Copilot speaking]
Adds a placeholder
fetch_winhttpcrate whose sole content is a design document:crates/fetch_winhttp/docs/DESIGN.md. No transport code yet - this PR is to review the design before implementation.fetch_winhttpis a Windows-only customfetchtransport that drives the OS WinHTTP API in asynchronous mode as a Tokio-free alternative tofetch_hyper. OS calls go through a bindings facade (realwindows-crate impl plus a mock for unit tests), mirroring the pattern used inoxidizer_io.The design covers the threading model (one shared OS session per built client, one transport instance per core, callbacks completing inline for processor affinity), the cancellation and FFI-ownership model (pooled
RequestContextreclaimed on the finalHANDLE_CLOSING), error handling, HTTP protocol negotiation, TLS (WinHTTP/Schannel-managed, with an insecure mode), compression, and request/response body streaming modelled asbytesbuf_ioreads/writes. It also includes a test plan (mock-bindings unit tests plus localhost integration tests) and a chapter of feedback on thefetchAPI where its Hyper/Tokio-shaped assumptions force workarounds.The document has been through several rounds of rubber-duck review; opening now for wider human review of the design.