Skip to content

Fix the node-20 CI hang: GC-vulnerable AbortSignal composition in the shared HTTP client - #19

Merged
HelloThisWorld merged 1 commit into
mainfrom
fix/http-abort-gc-hang
Aug 20, 2026
Merged

Fix the node-20 CI hang: GC-vulnerable AbortSignal composition in the shared HTTP client#19
HelloThisWorld merged 1 commit into
mainfrom
fix/http-abort-gc-hang

Conversation

@HelloThisWorld

Copy link
Copy Markdown
Owner

Summary

Fixes the recurring main CI failure where tests/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: safeHttpRequest composed its 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. 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 AbortController with a real timer per request — no GC dependence on any Node version — wired to the external signal and released in a finally, 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.ts pins the replacement mechanism: never-responding and headers-then-stall endpoints time out deterministically far under budget, external aborts classify as cancelled (never timeout), fifteen sequential requests sharing one external signal leave zero leaked listeners (getEventListeners), and createBoundedAbort's fire/release/propagation semantics are covered directly. createBoundedAbort is exported for tests, following the checkRedirectTarget precedent. The plugin dist/ bundles are regenerated because they embed the runners code. Failure classification is unchanged: the existing handlers already treat AbortError as timeout when no external abort happened.

Checklist

  • pnpm lint, pnpm typecheck, and pnpm test pass locally (133 files / 1788 tests, including the previously flaky suite)
  • pnpm check:public-contracts passes — no stable contract changed
  • Documentation updated for any user-visible behavior change (CHANGELOG covers the behavior change; no separate doc describes the internal client)
  • CHANGELOG entry added (1.3.0 (unreleased) → Fixed)
  • Everything is in English (code, comments, docs, commit messages)
  • No employer or client proprietary content — examples and fixtures are synthetic
  • No credentials, tokens, or secret values anywhere in the diff, fixtures, or recorded test output

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.

…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.
@HelloThisWorld
HelloThisWorld merged commit 18a137d into main Aug 20, 2026
7 checks passed
@HelloThisWorld
HelloThisWorld deleted the fix/http-abort-gc-hang branch August 20, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant