Repository navigation
fix(runtime): bound proxied fetch transport teardown - #5900
Conversation
Preserve immediate socket cancellation while bounding dispatcher destruction to avoid wedging shared transport callers. Cover successful HTTPS CONNECT and stalled dispatcher teardown. Generated-by: Codex (gpt-6.1-sol)
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Summary
This PR bounds createProxiedFetchTransport().close() by racing dispatcher destruction against a 1 s grace timer (cleared in finally). close() stays idempotent and destroy errors are still swallowed. It also adds loopback HTTPS-over-CONNECT tests, one with normal destruction and one with a mocked, never-settling ProxyAgent.prototype.destroy.
Scope checked
- Merges cleanly against current
origin/main. No protocol or epoch changes. There were no prior reviews. - Built core/storage/mcp/runtime on Node 22.23.2 with undici 8.11.2 (runtime). All 56
packages/runtimenetwork tests pass, including 44/44 inscoped-fetch-transport. - When the race is reverted, the mocked-stall test fails as expected, but the non-stalled test still passes. The PR's tests therefore do not reproduce the real hang.
- Ad hoc repro: a loopback CONNECT proxy in front of an
http2.createSecureServertarget, so ALPN negotiates h2. This is what real providers negotiate.- On main,
close()never settles. - At this head,
close()takes about 1002 ms on every run. - Against an HTTP/1.1 target it takes about 3 ms.
- Calling
dispatcher.destroy()beforeconnections.abort()brings the h2 case to about 3 ms, and all 56 network tests still pass with that ordering.
- On main,
- Timer handling, abort propagation and error classification are unchanged. The timer is not
unref'd, so it can hold the event loop for up to 1 s. That is acceptable.
Findings
- P2: The bound does not fix the stall. It turns it into a fixed 1 s cost on every proxied h2
close(): onboarding verify, WebFetch, WebSearch, metadata refresh and model composition. Undici's destroy is also left pending. Changing the teardown order looks like the actual fix. Keep the bound as a safety net. - P3: The regression test mocks
destroyand uses an HTTP/1.1 target, so the real failing path (h2 over CONNECT) is not covered. - Nit: The code comment "Sockets are already cancelled" holds for CONNECT tunnels and SOCKS raw sockets. It does not hold for keep-alive sockets the direct dispatcher has already established, because the abort listener is disposed once the connection is made.
Not exercised
Node 24/26, real external proxies and providers, SOCKS5 teardown timing, and runtime-host caller suites.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| ?.destroy(new Error('Connection effect fetch transport closed')) | ||
| .catch(() => {}), | ||
| ]).then(() => undefined); | ||
| // Aborting a CONNECT tunnel can leave Undici's dispatcher destroy pending. |
There was a problem hiding this comment.
P2: The bound hides a stall that happens every time, not occasionally. I reproduced the original hang locally (Node 22.23.2, undici 8.11.2) with a loopback CONNECT proxy in front of an http2.createSecureServer target, so ALPN negotiates h2 like real providers do. On main, close() never settles. At this head it takes ~1002 ms on every call, because the timer is what resolves it and ProxyAgent.destroy() is still left pending. Against an HTTP/1.1 target it takes ~3 ms. So behind a proxy, every verify, WebFetch, WebSearch, metadata refresh and model-composition finally { await transport.close() } now pays a fixed 1 s.
The stall comes from aborting the upgraded tunnel (abortSocket in onRequestUpgrade) before undici starts tearing down the h2 client. If I start the destroys first and abort afterwards, the h2 case closes in ~3 ms, and all 56 packages/runtime network tests still pass:
const destroyed = Promise.all([directDispatcher.destroy(err).catch(() => {}), proxyDispatcher?.destroy(err).catch(() => {})]).then(() => undefined);
connections.abort(err); // after destroy() has startedI suggest fixing the ordering and keeping the 1 s race only as a safety net.
| const socketsClosed = Promise.all([...proxy.sockets].map(waitForSocketClose)); | ||
| if (stalledDestroy) { | ||
| // Reproduce the dispatcher completion failure independently of Undici/Node version. | ||
| t.mock.method(ProxyAgent.prototype, 'destroy', () => new Promise<void>(() => {})); |
There was a problem hiding this comment.
P3: This regression test mocks ProxyAgent.prototype.destroy and runs against an HTTP/1.1 (node:https) target. Without the fix, the non-stalled case passes, so neither test exercises the real failure. Behind a CONNECT proxy, an http2.createSecureServer({ key, cert }) target reproduces the hang deterministically on Node 22 / undici 8.11.2 with no mocks. Could you add that case and assert that close() settles well under TRANSPORT_CLOSE_GRACE_MS (for example, under 500 ms)? That way the test catches both the hang and the case where the grace timer is what resolves close().
Exercise negotiated HTTP/2 CONNECT teardown and retain the bounded destroy safety net. Generated-by: Codex (gpt-6.1-sol)
|
Moved dispatcher destruction ahead of connection abort in Root lint, formatting, and both Knip checks pass. Full build/typecheck did not complete in this checkout: the unchanged DeepSeek codec has open-responses type/export mismatches, and full typecheck also reports downstream declaration mismatches. No external proxy/provider or other platform validation is claimed. Automated follow-up by Codex (gpt-6.1-sol). |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental re-review at dcd2be3
What changed since 846ff36: close() in packages/runtime/src/network/scoped-fetch-transport.ts now calls destroy() on the direct and proxy dispatchers first and only then calls connections.abort(). The 1 s grace race stays as a safety net. The regression test is now parameterized over http/1.1 and h2 targets: the h2 target is http2.createSecureServer, and the test asserts the negotiated httpVersion. A normal close must settle within 750 ms, which is below the 1 s grace.
Prior findings:
- P2 (every HTTP/2 proxied close waited the full 1 s grace): fixed by the destroy-before-abort ordering.
- P3 (regression test only covered a mocked HTTP/1.1 target): fixed. The test now runs over real HTTP/2 through CONNECT.
- Nit (the "Sockets are already cancelled" comment): fixed. The new comment is accurate.
Scope checked:
- The incremental diff, plus merge cleanliness against
origin/main, which is clean. There are no protocol changes. - I rebuilt
@maka/runtimeand ran the network test suite: 58/58 pass across 3 runs (Node 22, undici 8.11.2). At this head the h2 close takes about 35 ms and the HTTP/1.1 close about 67 ms. The stalled-destroy variants settle at about 1.02 s because the grace timer fires. - Mutation check: putting
connections.abort()back beforedestroy()makes the h2 non-stalled test fail at about 780 ms with "completed CONNECT tunnel blocked transport.close()". The new test therefore guards the real root cause, not just the mock. - The tests that close during a pending CONNECT, TLS handshake, or immediate close still settle quickly with the new ordering.
Findings: none new.
Not exercised: the full workspace npm test, the desktop/UI packages, and real third-party proxies or providers. I only used loopback proxies and targets.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
|
Validation on a real-world proxied setup, covering items the review listed as Scope: the shared transport behind provider verify, WebFetch/WebSearch and
(Hang reproduced identically on re-run; settle times are from single runs and One follow-up this surfaced, not asking to expand this PR's scope: Provenance: this validation was run with an AI coding agent under my |
… sites (#6003) Two call sites build their own proxy dispatcher and dispose it themselves, so the abort-before-close teardown pattern that the shared transport fix reordered (#5900) survives in them: - testProxyConnection's finally aborts the controller and then awaits the dispatcher's graceful close. Behind a real proxy that close never settles, so the call stays pending long past its 8s probe timeout and wedges the serialized settings lane it occupies on the Desktop. Start the dispatcher teardown before the abort and race it against a 1s grace, mirroring the transport pattern. - proxiedFetch's error path awaits the same graceful close after aborting, so a stalled close delays the rejection the caller is owed. Apply the same ordering and bound there. The success path keeps its fire-and-forget close, which owns the streaming body lifecycle and is intentionally not bounded. Regression tests stall ProxyAgent.prototype.close (the same Undici/Node-version-independent technique the transport tests use) and assert both calls still settle; both tests fail on the previous ordering. Refs #5978 Generated-by: GLM-5.3-Flash (ZCode) fix(runtime): escalate proxied dispatcher teardown to destroy after the grace expires Root cause: the bounded dispatcher teardown in testProxyConnection and the proxiedFetch error path raced the graceful close against a 1s grace, but when the grace won, the dispatcher was simply abandoned with its close() still pending, leaking the agent (flagged as a P3 inline on #6003). Fix: track who wins the grace race and, when it expires, fire-and-forget destroy() with a distinguishing reason. The destroy error is swallowed so teardown can never replace the result or rejection the caller is already owed. The grace duration and its external semantics are unchanged; the success path of proxiedFetch keeps its unbounded close, which owns the streaming body lifecycle. Verified: new regressions in proxy-test.test.ts and proxied-fetch.test.ts stub ProxyAgent#close to never settle and assert destroy is called; both failed before the fix (callCount 0) and pass after. runtime network+bots suites 167/167, biome check clean, check:asf-headers clean. Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: the automated review of this exact head found no blocking (P0–P2) issues, and CI is green.
3 participants
Summary
Bound shared proxied-fetch transport teardown to a one-second grace period after immediate socket cancellation. A pending Undici dispatcher destroy promise can otherwise prevent completed CONNECT requests from returning to onboarding, model refresh, and WebFetch/WebSearch callers. Repeated close calls share the same completion promise; teardown errors remain best-effort.
Fixes #5898
Verification
deepseek-web-search-codec.ts/@ai-sdk/open-responsesAPI errors, also observed on the unchanged baseline; downstream typecheck additionally sees stale/missing compiled exports after the build stops. These failures are not reported as passes.AI use
Tool(s) and scope: Codex (gpt-6.1-sol) implemented and technically reviewed the change and ran the automated validation under contributor direction. The commit includes a Generated-by trailer.
Checklist
Does this PR entail a change in behavior?
HTTP/2 teardown follow-up
Dispatcher destruction now starts before connection abort; the one-second bound remains a safety net. Real HTTP/2-over-CONNECT and HTTP/1.1 regression coverage verifies protocol negotiation, close completion, and socket teardown.
Validation: 58 network tests, root lint/formatting, and desktop/UI Knip checks passed. The new h2 test fails with the old ordering. Full build/typecheck failed locally in unchanged DeepSeek codec/package interfaces and downstream declarations; those checks are not claimed as passing.