Uh oh!
There was an error while loading. Please reload this page.
fix(headless): keep provider proxy upstream on HTTP/1.1 - #1658
Merged
Conversation
Provider gateways negotiate HTTP/2 via ALPN, and undici's h2 client runs one request at a time per origin connection. All proxied cells in a harness run share one Node process, so their streaming completions serialized behind each other's full generation streams: every request waited for all earlier in-flight streams before its response headers arrived, and the wait was recorded as provider first-token latency (observed p50 ~13s, tails past 600s, versus ~1.5-4.5s on direct per-process connections). Route upstream fetches through an explicit undici Agent with allowH2 disabled, injectable per route so tests can trust a local CA. The regression test drives two concurrent held-open streams through the proxy against an ALPN-h2 upstream and asserts both arrive as HTTP/1.1 in parallel; with h2 re-enabled the second request deadlocks behind the first and the test fails.
responseHeadersMs starts at proxy receipt, so dispatcher connection-queue time was indistinguishable from upstream wait — the interval where the h2 serialization hid. upstreamStartMs stamps the moment undici begins writing the request to an upstream connection via a per-request composed dispatcher, making the split observable.
Delegate onRequestStart with this bound to the wrapper so undici handler state written across callbacks always lands on a single object instead of relying on undici's internal read/write ordering across the prototype split.
Deterministic injected-clock test: one upstream connection holds the first stream while the second request provably enters the pool queue, then exact equalities pin upstreamStartMs to when the request leaves the queue. A stamp taken before dispatch, at response headers, or dropped entirely all fail. Mutation-verified (pre-dispatch stamp fails with 0 !== 5000).
Uh oh!
There was an error while loading. Please reload this page.
Astro-Han added a commit
that referenced
this pull request
Jul 30, 2026
requestDrain() flips in-memory #state to 'draining' synchronously, but persisting that to the registration file is the async writeHostRegistration I/O in #closeResources, while #start() had already persisted 'recovering' before entering the factory. The fixed sleep(50) bet that I/O landed in time, which lost on loaded CI runners and read back stale 'recovering' (the #1658 main CI failure). Poll the registration file for 'draining' instead of sleeping a fixed gap. #closeResources writes 'draining' first, and the file stays 'draining' while the factory is suspended, because removeHostRegistration runs only after the blocked compositionStartup await later in #closeResources. A handshake connection does not work here: #closeResources calls server.close() right after the registration write, so on loaded CI the socket stops accepting before connectRuntimeHost finishes its resolveStorageRoot, mkdir, and registration read, returning 'unavailable' instead of 'draining' (the #1660 CI failure). Verified with a 300ms writeHostRegistration delay: the old sleep(50) form fails with the same 'recovering' seen on main, while the polling form passes.
Astro-Han added a commit
that referenced
this pull request
Jul 30, 2026
… on CI The "drain requested before factory completion begins drain before recovery exactly once" test failed the test job on main after #1658 landed (run 30544963454), even though #1658 only touches packages/headless. The flaky test was introduced in #1359. The flake came from asserting on the registration file mid-flight. requestDrain() flips in-memory #state to 'draining' synchronously, but persisting that to the registration file is the async writeHostRegistration I/O in #closeResources, while #start() had already persisted 'recovering' before entering the factory. The fixed sleep(50) bet that I/O landed in time, which lost on loaded CI runners and read back stale 'recovering'. That assertion tested a non-contract. No production caller reads HostRegistration.state; clients learn draining from the handshake (#admitHandshake returns kind:'draining' from #shutdownRequested), and connectResolvedRuntimeHost reads the registration only for rootId, hostEpoch, and the endpoint. The invariant the test name claims, "begins drain before recovery exactly once", is already proven by the final lifecycle array ['factory-return', 'begin-drain', 'recover', 'close'] with begin-drain counted once. The in-memory draining state is covered elsewhere (candidate.host.state === 'draining'). Drop the sleep, the readHostRegistration call, and the state assertion. The test still verifies, while the factory is suspended, that startup has not settled, that no lifecycle event has fired, and that the host still holds the interactive root owner lock, then checks the full lifecycle ordering after release. A handshake-based replacement was tried first but also raced: #closeResources calls server.close() right after the registration write, so on loaded CI the socket stops accepting before connectRuntimeHost connects, returning 'unavailable' instead of 'draining' (the first push of #1660). Refs #1658 (main CI failure, not a regression from that PR).
Astro-Han added a commit
that referenced
this pull request
Jul 30, 2026
… on CI (#1660) The "drain requested before factory completion begins drain before recovery exactly once" test failed the test job on main after #1658 landed (run 30544963454), even though #1658 only touches packages/headless. The flaky test was introduced in #1359. The flake came from asserting on the registration file mid-flight. requestDrain() flips in-memory #state to 'draining' synchronously, but persisting that to the registration file is the async writeHostRegistration I/O in #closeResources, while #start() had already persisted 'recovering' before entering the factory. The fixed sleep(50) bet that I/O landed in time, which lost on loaded CI runners and read back stale 'recovering'. That assertion tested a non-contract. No production caller reads HostRegistration.state; clients learn draining from the handshake (#admitHandshake returns kind:'draining' from #shutdownRequested), and connectResolvedRuntimeHost reads the registration only for rootId, hostEpoch, and the endpoint. The invariant the test name claims, "begins drain before recovery exactly once", is already proven by the final lifecycle array ['factory-return', 'begin-drain', 'recover', 'close'] with begin-drain counted once. The in-memory draining state is covered elsewhere (candidate.host.state === 'draining'). Drop the sleep, the readHostRegistration call, and the state assertion. The test still verifies, while the factory is suspended, that startup has not settled, that no lifecycle event has fired, and that the host still holds the interactive root owner lock, then checks the full lifecycle ordering after release. A handshake-based replacement was tried first but also raced: #closeResources calls server.close() right after the registration write, so on loaded CI the socket stops accepting before connectRuntimeHost connects, returning 'unavailable' instead of 'draining' (the first push of #1660). Refs #1658 (main CI failure, not a regression from that PR).
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 freeto 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.
Summary
All proxied harness cells (
opencode/kimi-code/codexarms, and any Maka arm configured through the proxy) share one Node process, and the proxy forwarded upstream requests with the globalfetch. Provider gateways (api.z.ai,api.kimi.com) negotiate HTTP/2 via ALPN, and once one h2 session existed, every concurrent cell's streaming completion serialized behind the full generation streams of the others (root cause below). The queueing happened between proxy receipt and upstream dispatch, so telemetry recorded it as provider first-token latency.Measured impact in existing runs (same key, same windows, direct-connect arms unaffected at ~1.5–4.5 s):
The fix routes upstream fetches through an explicit undici
AgentwithallowH2disabled (HTTP/1.1 pools dial parallel connections), injectable per route so tests can trust a local CA. Switching to the npmundici@8.8.0fetch also independently removes the gate; forcing h1 keeps the proxy safe regardless of which undici build serves the fetch.Existing A/B timing conclusions drawn from proxied arms need re-examination; report corrections are tracked separately.
Root cause
Node 26.5.0 bundles undici 8.7.0. Its
fetchwraps every non-empty request body into an async iterable (lib/web/fetch/index.js), and its h2 client refuses to multiplex stream/async-iterable bodies on a session with requests in flight (lib/dispatcher/client-h2.jsbusy():bodyLength !== 0 && (isStream || isAsyncIterable || isFormDataLike)→ busy), because such bodies cannot be retried after a mid-flight session error. The queued request stays in the h2 client's queue — the pool never dials a second connection. Net effect: every streaming POST to the same origin from one process runs strictly one-at-a-time. undici 8.8.0 removed this gate and multiplexes up tomaxConcurrentStreams.Differential confirmation (local ALPN-h2 upstream, zero network): staggered POSTs via built-in fetch serialize on one session; staggered GETs (empty body) multiplex on the same session; staggered POSTs via npm undici 8.8.0 with h2 enabled multiplex. Against the real provider: 4 staggered streaming requests + 1 tiny request from one process serialized strictly (tiny request waited 30.8 s); the same schedule from separate processes ran fully parallel (3.65 s).
Verification
npm run test --workspace @maka/headless— 1432 tests, 0 failures.httpVersion === '1.1'assertion is the timing-independent lock; the upstream additionally refuses to end either stream until both have arrived, so the undici ≤ 8.7 staggered-dispatch path deadlocks instead of passing by luck. Mutation check: flippingallowH2: truein the built dispatcher makes exactly this test fail.npm run format/npm run lintclean.Observability
responseHeadersMsstarts at proxy receipt, so dispatcher connection-queue time was indistinguishable from upstream wait — the interval where the serialization hid for two benchmark campaigns. The newupstreamStartMstelemetry field stamps the moment undici begins writing the request to an upstream connection (per-request composed dispatcher wrappingonRequestStart);responseHeadersMs − upstreamStartMsis now pure upstream wait. Live check through the fixed proxy:upstreamStartMs231–403 ms across 5 concurrent requests while headers took 1.9–4.7 s — under the old defect this field would have read 10–30 s and exposed the queue on day one.Review focus
proxy.close()intentionally does not close the module-level shared dispatcher; idle h1 sockets are unref'd and do not hold the process open.Post-review hardening
Two independent optimality reviews (Codex CLI and a separate Claude subagent, same criteria: first principles / Occam / test lock enumeration / refactor-to-optimal) both reported no P0/P1 and recommended merging. Accepted findings landed as the last two commits:
onRequestStartdelegation now bindsthisto the wrapper (.call(this, ...)), removing a latent dependency on which side of theObject.createprototype split undici handler callbacks write their state to.upstreamStartMsto dispatcher queue-exit semantics: one upstream connection holds the first stream while the second request provably enters the pool queue (compose-counter gate, no wall-clock waits), then exact equalities distinguish queue-exit (5000) from a pre-dispatch stamp (0), a response-headers stamp (6000), or a dropped stamp (undefined). Mutation-verified.Recorded as accepted residual risk (both reviews concur): the default-singleton fallback path (
?? defaultUpstreamDispatcher) has no direct test — the injected-dispatcher mutation lock plusAgent's ownallowH2: falsedefault keep the invariant — and the inline test PEM pair is a throwaway localhost self-signed keypair, not a secret.