docs(fetch_winhttp): add design document for WinHTTP custom transport

[sandersaares]

[Copilot speaking]

Adds a placeholder fetch_winhttp crate 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_winhttp is a Windows-only custom fetch transport that drives the OS WinHTTP API in asynchronous mode as a Tokio-free alternative to fetch_hyper. OS calls go through a bindings facade (real windows-crate impl plus a mock for unit tests), mirroring the pattern used in oxidizer_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 RequestContext reclaimed on the final HANDLE_CLOSING), error handling, HTTP protocol negotiation, TLS (WinHTTP/Schannel-managed, with an insecure mode), compression, and request/response body streaming modelled as bytesbuf_io reads/writes. It also includes a test plan (mock-bindings unit tests plus localhost integration tests) and a chapter of feedback on the fetch API 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.

[github-actions]

✅ Version increments look sufficient

cargo semver-checks compared the 1 crate(s) this PR publishes against their previous version-bump commit in git history. Every version increment is sufficient for the detected API changes.

Crate	Baseline	Baseline commit	This PR	Minimum required	Status
fetch_winhttp	new crate	—	0.1.0	0.1.0	✅ ok

This check is informational and does not block the merge.

View the check run

[Copilot]

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_winhttp crate with a placeholder lib.rs and a detailed docs/DESIGN.md describing 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.

[martintmk]

nit: 1400 lines of design, can we shorten it?

[sandersaares]

Do you have specific suggestions for which sections are unnecessary or overly verbose? Hard to take action based on line count alone.

[ralfbiedert]

Since I had the same issue, I used an agent to help me outline my thoughts here. Please take some time go through them. If you think anything is unnecessary or verbose, I'd love to hear specific suggestions how to improve it!

fetch_winhttp_design_analysis.md

[martintmk]

I read Ralf's analysis and more or less agree with that assessment pages. I would love to see a shortened section, max 3-4 pages with core tenets and requirements.

Many parts of this document read more as an implementation plan for agents.

[martintmk]

What would really help is to see actual APIs and how these are used in fetch.

// do we need some arguments for win http?
HttpClient::builder_winhttp(...)
   .winhttp(...) // is there win-http specific configuration
   .tls(TlsOptions::default()) // !!! THESE ARE IGNORED, we need to address this in fetch
   .build()

Maybe even highlight fetch knobs that are not applied.

[sandersaares]

Factored document into 3 separate documents to simplify reviewing from different viewpoints. Added API example for usage in fetch. The API shape is not something I am particularly happy with but I think this needs fetch-level design work to make working with transport-specific concerns more ergonomic, not something specific to winhttp integration.

@ralfbiedert I found it quite hard to extract actionable feedback from the attached analysis. There is a lot of just description in there and a hilarious amount of "the key problem with this plan is that it is a plan so we don't really know for sure yet", which obscures the more valuable points. I was able to extract some specific items from there but it feels like it was missing a "robot, condense this analysis to actionable suggestions" turn at the end.

[martintmk]

Thanks Sander, the shortened design is now much more digestible. Will go through it.

[martintmk]

it's better to expose these as extensions so you get a nice HttpClient::builder_win_http experience

[sandersaares]

Integrated into doc.

[martintmk]

TlsOptions are agnostic to hyper, the plumbing that consumes TlsOptions and adapts them to hypers lives in fetch_hyper

[sandersaares]

Clarified in text that "hyper" here refers to "fetch_hyper" (the fetch feedback is that TLS is a property of transports like fetch_hyper not of the pipeline composer fetch).

[martintmk]

we might leave this configurable, for clients with low-traffic, there is no need to create per-core instances

[sandersaares]

Might be worth noting down as a low-traffic client optimization pass (possibly deferred), though needs measurements/benchmarks to evaluate impact. Which of our current known customer scenarios would you classify as the most representative of this situation and how would you characterize their usage of fetch (i.e. what aspects would be important to replicate in a benchmark)?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (53018e0) to head (51169ff).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #595   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         442      443    +1     
  Lines       42616    42617    +1     
=======================================
+ Hits        42616    42617    +1     

Flag	Coverage Δ
linux	100.0% <100.0%> (?)
linux-arm	100.0% <100.0%> (?)
windows	100.0% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ralfbiedert]

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.

[martintmk]

The convenience for common workloads is intentional. We don't want to go through extra loops for 90% of users that just want to use tokio with whatever default transport we choose.

Also, let's think about the current setup more as a stopgap while we work on fine-tuning and stabilizing other parts of our stack (runtime, security). Once that's in, we will also have built-in builder_oxidizer + new_oxidizer APIs.

[martintmk]

Current bundling to fetch crate might be unfortunate because indeed the fetch_tls might not be useable in current form with winhttp.

However, my thinking is that fetch_tls might server as a set of descriptors that can be interpreted by different TLS technologies.

One thing we need to address is how the different capabilities should be represented. (for example, certificate validation can inspected by callbacks for rustls but not for native-tls.

[martintmk]

Suggested change

	`HttpClient::build(transport().tls(cfg))` shape would express this cleanly.
	`HttpClient::builder_tokio(deps, tls)`

this is also possible alternative, do not tie tls options to builder but rather explicitly require them on construction

[martintmk]

If we ignore all technical details of timeouts across different timeouts, one timeout that the end consumer of fetch cares about is "connect timeout".

While I believe configuration knobs are useful, on the level of common API, connect timeout is something we should keep and let transports interpret it.

Last resort for transport is to leverage drop safety, if the future is cancelled, transport should handle it properly.

[martintmk]

Maybe HttpClient::builder() could return HttpClientBuilder<NeedsTransport>. APIs for HttpClientBuilder<NeedsTransport> could expose configuration for pipeline, i.e. things that are not transport agnostic.

[martintmk]

TLDR: many good points but one major point I want to highlight. fetch is batteries includes so we DO play favorites. We will pick our pick for default transport for each supporter runtime and support for custom transports should not impact the ergonomics for our default choices.

This can be done ergonomically where HttpClient::builder(...) could return HttpClientBuilder<NeedsTransport> while HttpClient::builder_tokio(...) returns something like HttpClientBuilder<DefaultTransport> where we plug-in our recommended transport.

[martintmk]

also the generic parameter should be defaulted, i.e. HttpClientBuilder<T = DefaultTransport>

(we can live with generics on HttpClientBuilder, generics on HttpClient are probably a no-go)

[martintmk]

I also think that is ok for us to expose lowest common denominator for our default transport and then also enable deeper customization through specific transport knobs:

HttpClient::builder(...).hyper(hyper_transport)

[martintmk]

For use of ..Default::default() we cannot apply #[non_exhaustive]. Are we sure we won't be adding new fields later?

[sandersaares]

[Copilot speaking] Agreed - we should expect to add fields. Reworked design.md §1.1 so WinHttpDeps, WinHttpOptions and WinHttpTlsConfig are all #[non_exhaustive] and built through builders (WinHttpDeps::builder().tls(...).options(...).sink(...).build()) instead of a struct literal with ..Default::default(), so new knobs can land compatibly. Fixed in 2883b8d.

[martintmk]

from the perspective of transport, it needs to ensure drop safety, i.e. when fetch cancels the future, transport needs to honor it.

Also, fetch attaches timeout information to each request that can be inspected and may be even applied to transport?

See:
https://docs.rs/http_extensions/latest/http_extensions/timeout/index.html

[martintmk]

saw this mentioned bellow, drop safety still applies

[sandersaares]

[Copilot speaking] Both points addressed in 2883b8d.

Drop safety: honored. Dropping the request future closes the WinHTTP handle, which cancels the in-flight operation and delivers the terminal HANDLE_CLOSING that reclaims the request context (design.md §7 cancellation model; implementation.md §4). I added an explicit note in design.md §6.1 that cancellation is the transport's backstop for any timeout it does not enforce natively.

Per-request timeout inspection: adopted where the semantics actually match, which turned out to be a split:

• BodyTimeout (idle gap between body chunks, reset on progress) matches WINHTTP_OPTION_RECEIVE_TIMEOUT (per-receive-operation idle timer, reset each read), so the transport now reads it from the request extensions and programs that native timer per-request.
• ResponseTimeout is a total over connect + send + receive-headers. WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT covers only the post-send wait for the first response byte (it excludes connect and send), so it is not a faithful remap. ResponseTimeout therefore stays enforced above the transport (as fetch/http_extensions already do) and the native receive-response timer is set only as a looser liveness backstop.

Mapping detail is in implementation.md §10.4.

[martintmk]

SendTimeout is really just the ResposeTimout in disguise. We send request and wait for the response without touching the response body. After that the BodyTimeout applies.

[sandersaares]

[Copilot speaking] Agreed - fixed in 51169ff. Removed the standalone "send timeout" framing: §6.1 now states send is not a distinct concept (send + await-headers is exactly the span ResponseTimeout governs, then BodyTimeout takes over), leaving only a Resolve transport-specific knob. Updated the implementation.md §10.4 timeout table to match (WinHTTP send field left as a loose backstop, not driven by a dedicated fetch knob).

[martintmk]

Thanks for fine-tuning the design for humans, the split into design + implementation might be something we should leverage more to avoid overly technical design docs.

I think this is good to go, minor comments. The fetch stabilization is a different topic we need address. Custom winhttp transport implementation will give us a lot of useful insights.

[martintmk]

I think this is good to go, fetch API design is different discussion

[martin-kolinek]

🤖: connect_timeout is configured here via WinHttpOptions, but §6.1/§6.2 and implementation.md §10.4 say the transport reads TransportOptions.connect_timeout (implementation.md's timeout test table also alternates between the two). Please pick one source of truth or specify precedence — otherwise this public example and the implementation contract can yield different connect-timeout values.

[martin-kolinek]

🤖: Silently ignoring fetch's TlsOptions is the one dropped option with security consequences. CustomContext.tls is always populated, so a caller who set non-default TLS material — a private CA root, a client identity (mTLS), or cert-pinning — silently gets a default system-trust connection with no signal, even though the same code 'works' on fetch_hyper. Please make the design explicit that a non-default TlsOptions triggers the §11 build-time warning, and consider a hard error (not just a warn) when security-affecting TLS material is present, since this is a silent security weakening rather than a benign fidelity gap.

[martin-kolinek]

🤖: Ignoring PoolIndex collapses fetch's configured multiple_pools into a single WinHTTP connection pool: fetch invokes the custom transport factory once per pool slot specifically so routing to a PoolIndex selects an independent transport pool, but every handler here shares the one OS session — so nominally separate pools all use the same OS connection pool. That contradicts the fetch contract and this design's §11 warning policy. Either key WinHTTP sessions by PoolIndex (one session per pool slot, shared across cores within a slot), or add multiple_pools to the explicitly-unhonored options and emit the §11 warning instead of silently collapsing it.

[martin-kolinek]

🤖: Opening and capturing the session in builder_winhttp breaks the per-built-client isolation guarantee (design.md §2) when the builder is cloned: HttpClientBuilder is Clone, so two clones can be built into independent clients while retaining this same captured Arc<WinHttpSession> — and therefore the same WinHTTP pool. Since that isolation is a stated TLS-policy security boundary (a strict client must not share a pooled connection with an accept_invalid_certs one), please make the session build-scoped (e.g. create per-client shared state at build, lazily initialized from the finalized CustomContext) rather than builder-scoped, and add an integration test asserting that two independently built clients targeting the same authority establish separate connections.

[martin-kolinek]

🤖: The integration plan only exercises accept_invalid_certs, but the design exposes accept_invalid_certs and accept_invalid_hostnames as independent security relaxations. Please add a localhost TLS scenario with both an untrusted cert and a hostname mismatch, covering all four flag combinations: either flag alone must leave the other validation active, and only both together should succeed. That catches accidentally coupling the two Schannel ignore flags.