Fix the node-20 CI hang: GC-vulnerable AbortSignal composition in the shared HTTP client - #19
Merged
Merged
Conversation
…TP client The shared model-API HTTP client composed its total timeout as AbortSignal.any([AbortSignal.timeout(ms), external]) built inline in the fetch call. On Node 20 the composite holds only weak references to its source signals, so an otherwise-unreferenced timeout signal can be garbage collected before its timer fires -- and a request against an endpoint that never answers then hangs forever instead of timing out. This is the root cause of the recurring node-20 CI failure where "a timeout aborts the request deterministically" (contract: ~1.5 s) burned the full 30-second Vitest budget: the abort simply never happened. The flake predates v1.3 (identical failures on the #14 and #16 merge commits) and is GC-timing dependent, which is why it appeared intermittently and almost always on the node 20 runners. Every consumer of safeHttpRequest was exposed: Ollama, OpenAI-compatible endpoints, the managed local model, and registry downloads. The client now creates one explicit AbortController with a real timer per request -- no GC dependence on any Node version -- wired to the external signal and released in a finally, which guarantees the timer and listener never outlive the request. This also fixes the 'abort' listener any() leaked on long-lived external signals (one per request, never unsubscribed), and makes the timeout genuinely TOTAL across redirect hops and body streaming, as the contract always documented. New regression tests pin the replacement mechanism at the client level: never-responding and headers-then-stall endpoints time out deterministically, external aborts classify as cancelled, sequential requests leave zero leaked listeners, and createBoundedAbort's fire/release/propagation semantics are covered directly. The plugin dist bundles are regenerated. Full suite: 1788 tests pass; lint, typecheck, snapshots, smoke-relevant bundle checks all verify.
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.
Summary
Fixes the recurring
mainCI failure wheretests/runners/ollama.test.ts > "a timeout aborts the request deterministically"— a test whose own contract is ~1.5 seconds — intermittently burned the full 30-second Vitest budget, almost always on the node 20 runners (it failed the merge commits of #14, #16, and #18 while every PR run was green). #17 made the fixture's teardown bounded and named, so today's recurrence as an anonymous 30-second timeout proved the hang was on the client side: the abort never fired.Root cause:
safeHttpRequestcomposed its timeout asAbortSignal.any([AbortSignal.timeout(ms), external])built inline in thefetchcall. On Node 20 the composite holds only weak references to its source signals, so an otherwise-unreferenced timeout signal can be garbage-collected before its timer fires — and a request against an endpoint that never answers then hangs forever. GC timing explains the intermittence and the node-20-only pattern (Node 22 carries the fix). Every consumer of the shared client was exposed: Ollama, OpenAI-compatible endpoints, the managed local model used by the v1.2 job driver, and registry downloads.The client now creates one explicit
AbortControllerwith a real timer per request — no GC dependence on any Node version — wired to the external signal and released in afinally, so neither the timer nor the listener can outlive the request. Two adjacent defects fall out of the same change:any()leaked one'abort'listener on a long-lived external signal per request (never unsubscribed), now detached on release; and the timeout is now genuinely total across redirect hops and body streaming, which is what the client's contract always documented (previously each redirect hop silently reset the clock).Reviewer notes. The GC race itself cannot be forced from a test, so the new
tests/runners/http-timeout.test.tspins the replacement mechanism: never-responding and headers-then-stall endpoints time out deterministically far under budget, external aborts classify ascancelled(nevertimeout), fifteen sequential requests sharing one external signal leave zero leaked listeners (getEventListeners), andcreateBoundedAbort's fire/release/propagation semantics are covered directly.createBoundedAbortis exported for tests, following thecheckRedirectTargetprecedent. The plugindist/bundles are regenerated because they embed the runners code. Failure classification is unchanged: the existing handlers already treatAbortErroras timeout when no external abort happened.Checklist
pnpm lint,pnpm typecheck, andpnpm testpass locally (133 files / 1788 tests, including the previously flaky suite)pnpm check:public-contractspasses — no stable contract changed1.3.0 (unreleased)→ Fixed)Security-relevant change: none of the client's safety invariants moved — redirect rejection/bounding, cross-origin header dropping, response-size streaming limits, and credential-free URLs are untouched; the change only makes the documented timeout guarantee actually hold on Node 20.