fix(test): make the proxy-fallback and subprocess cases actually boundable - #1490
Merged
Merged
Conversation
…dable Two cases in continuous-test-suite-bugfixes have been failing on CI runners at a 240s bound while passing everywhere locally, blocking #1487. They are unrelated bugs with the same consequence: a case that cannot be bounded. The proxy-fallback case forced the idle-timeout path by patching `globalThis.setTimeout` to fire every timer at 0ms, then restoring it in a `finally`. Two problems. The patch is installed across an await inside a 280-case suite sharing one process, so for that window it rewrites the delay of EVERY timer created anywhere — including ones belonging to other cases' pending work. And if the awaited call never settles, the `finally` never runs and the patch leaks into the rest of the suite. That is not theoretical. A peer session trying to measure this very case wrote a `setTimeout`-based watchdog around it, and the watchdog was itself rewritten to 0ms by the patch it was measuring — reporting an instant false hang, twice, before the instrument was suspected rather than the code. Same failure at a 90s bound, same cause. `executeClaudeFallbackWithRetry` now takes an optional `idleTimeoutMs`, defaulting to FALLBACK_STREAM_IDLE_TIMEOUT_MS, and the case passes 1. No global is touched, so the class of problem is gone rather than this instance of it. The second case is a different mechanism with a sharper edge. It drives the CLI through `spawnSync` with `timeout: 10_000`, and spawnSync's timeout is not a guarantee: it sends `killSignal` — SIGTERM by default — and then keeps waiting. A child that ignores SIGTERM is never killed and spawnSync never returns. Verified directly: a child running `process.on("SIGTERM",()=>{})` with a live interval hangs spawnSync indefinitely, while the same call with `killSignal: "SIGKILL"` returns at the timeout with ETIMEDOUT. This is the one failure mode that defeats #1487's whole approach. spawnSync blocks the event loop, so a Promise.race per-case bound physically cannot fire while it is stuck — the bound reports only after spawnSync returns, and if it never returns, never. All seven timed subprocess calls in this suite now pass killSignal SIGKILL. Worth noting the suite already asserts that production code does this: the audioPlayer case checks `source.includes("killSignal")` so a hung decoder cannot block the CLI. The rule existed; the tests just were not following it. Confirmed the fallback case still catches a real defect by removing the abort from the idle-timeout path — it fails rather than passing quietly. Three consecutive full-suite runs: 280 passed, 0 failed, 58s each. The CI hang itself is not yet explained, and this does not claim to explain it. What it removes is the reason the two cases could not be bounded or measured honestly when it happens again.
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Eighteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a `runAllTests` they pass to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. That is not hypothetical: two of my own runs of continuous-test-suite-context died on a wall-clock limit this session with no per-case attribution, and this is why. `withCaseTimeout` moves to the shared harness rather than being copied per suite. #1484 added an identical local helper to the proxy suite while fixing the same bug there; the semantics here match it deliberately so the two cannot drift, and that suite is left alone to avoid conflicting with an open PR. 19 call sites across 18 suites. The one non-uniform case is autoresearch, which has two runners — the assertion in the conversion script caught that before anything was written, rather than silently converting one and leaving the other. Verified the bound actually fires, since a timeout helper that never triggers is worse than none: normal case returns its value untouched a hung case rejects at 301ms against a 300ms bound, naming the case the timer does not keep the process alive AND IT IMMEDIATELY CAUGHT A REAL HANG. `continuous-test-suite-bugfixes` now reports: ✗ proxy fallback: an idle stream is aborted without ambiguous replay exceeded 240000ms and was aborted — treat as a hang, not slowness That case previously hung silently inside a suite that reported 280 passed. The failure is pre-existing and intermittent; what changed is that it is now attributable instead of costing four minutes of wall-clock and no information. Folded in continuous-test-suite-proxy.ts, at its author's request now that #1484 has merged. Its local `withCaseTimeout` is deleted in favour of the shared one — two implementations that match today are two that can drift tomorrow, and there is now exactly one definition in the repo. The proxy suite keeps its own 180s bound rather than the shared 240s default, for the reason its author gave: every case there talks to a proxy this repo builds and spawns, not a live provider, so a hang is a defect and does not deserve the slack a remote endpoint gets. Checked the swallow hazard they flagged rather than assuming the shared wording is safe. `defineSuite` classifies a thrown error as SKIP when the message matches `isExpectedProviderError()`, and this message contains "aborted", which is the shape that can be swallowed. Ran the messages through it directly: "... exceeded 240000ms and was aborted — treat as a hang, not slowness" "... exceeded 180000ms and was aborted — treat as a hang, not slowness" "... exceeded 240000ms and was aborted" all three report as FAILURE, none are swallowed. A timeout that reported as a skip would be the exact false green this change exists to remove. Verified after folding: typecheck 4821 files 0 errors, eslint 0 errors, prettier clean, proxy suite 74 passed / 0 failed / 6 skipped, servers 41 passed / 0 failed, and exactly one `withCaseTimeout` definition repo-wide. Narrowed the globalThis.setTimeout patch in the proxy-fallback case, because bounding the cases exposed a cascade that the bound itself creates. `Promise.race` does not cancel the losing promise. When the harness bound fires on a hung case, that case KEEPS RUNNING — still inside its `try`, with its `finally` not yet reached. A case that has patched a global therefore leaks that patch to every case after it. In CI this showed as two failures rather than one: the proxy-fallback case timed out, and then ✗ CLI setup: --provider <p> --check takes the check-only path (no interactive prompt, no hang) which sits later in the same file, failed in its wake. One hang became two failures, and the second looked unrelated. The patch now matches on the delay and collapses only the fallback idle timeout, passing every other timer through untouched. That keeps the case testing exactly what it tested, makes the patch inert for other pending work in a 280-case shared process, and makes it harmless even in the window after an abandoned run. Proven both directions: an unrelated 400ms timer still waits 403ms, the targeted 120s timer collapses to 1ms. This does not explain why the case hangs in CI and passes everywhere else — that is still open, and it is not being papered over. What it removes is the mechanism by which one unexplained hang corrupts unrelated cases. The constant is mirrored locally with its provenance rather than exported from the product: if the product value ever changes, the collapse stops matching and the case fails loudly at the harness bound instead of passing for the wrong reason. bugfixes 280 passed / 0 failed after the change. Documented what this helper CANNOT bound, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free. Anything that blocks the loop is unprotected: `spawnSync`, `execSync`, a slow synchronous read. Measured rather than reasoned about — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. That matters here specifically. continuous-test-suite-bugfixes makes 20 `spawnSync` calls, 8 of them with a `timeout`, and `spawnSync`'s timeout is not a guarantee either: it sends `killSignal`, SIGTERM by default, then keeps waiting, so a child that ignores SIGTERM hangs it forever. Those cases were unboundable by construction and this change would have left them looking protected. Credit to cli-support-83, who found it from the second CI failure I sent them and is fixing the call sites in #1490. Also recorded on the helper: `Promise.race` does not cancel the losing promise, so an abandoned case keeps running with its `finally` unreached — which is the cascade that made one hang produce two CI failures, and the reason the proxy-fallback case's global patch is now scoped by delay.
Contributor
Review SummaryDecision: APPROVED ✅This PR addresses two critical issues blocking issue #1487 from progressing in CI: Changes Overview1.
|
Tara-ag
approved these changes
Aug 23, 2026
Contributor
Review SummaryDecision: APPROVED ✅ This PR fixes test infrastructure issues in Changes OverviewFile 1:
|
Tara-ag
approved these changes
Aug 23, 2026
Contributor
|
🎉 This PR is included in version 11.20.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
murdore
added a commit
that referenced
this pull request
Aug 23, 2026
`defineSuite` applies a 240s per-case timeout, but it lives in the `test(name, fn)` helper it hands back. Nineteen suites never use that helper: they keep their own `tests[]` array and loop `await test.fn()` inside a runner passed to `runSuite`. `runSuite(body)` only awaits the body — it adds no timeout of its own — so every case in those suites was UNBOUNDED. One hung case then hangs the whole run until CI kills the job, and the report says nothing about which case it was. Two of my own runs of continuous-test-suite-context died on a wall-clock limit with no per-case attribution; this is why. `withCaseTimeout` lives in the shared harness rather than being copied per suite. 20 call sites across 19 suites. The proxy suite keeps its own 180s bound because every case there talks to a proxy this repo spawns, not a live provider, so a hang is a defect rather than an environment problem. WHAT IT CANNOT BOUND is documented on the helper, because a bound that is believed and does not work is worse than no bound. It is `Promise.race`, so the timer only fires when the event loop is free: `spawnSync`, `execSync` and other synchronous blocking are unprotected. Measured — a 300ms bound around a 3s `spawnSync` returned at 3025ms without firing. The corollary is recorded too: `spawnSync`'s own `timeout` sends SIGTERM and keeps waiting, so a child that ignores it hangs forever; every call site needs `killSignal: "SIGKILL"`. cli-support-83 found that from a CI failure this change surfaced, and fixed all ten call sites in #1490. AND IT FAILS CLOSED. `Promise.race` does not cancel the loser, so an abandoned case keeps running and still holds whatever it mutated — a patched global, an open server, a temp dir. Every later case then runs in a process it does not own, and its PASS or FAIL means nothing. That was not theoretical: one hang produced a second, unrelated CI failure 176 lines further down the same file. So once a case has been abandoned the helper refuses to START another, and says why. Reporting the rest as skips would be exactly the false green this change exists to remove. `CaseTimeoutError` is exported so a runner can tell a bound from a real failure. Verified rather than asserted: a normal case passes through untouched, three in sequence a hung case rejects at 301ms against a 300ms bound, naming the case the rejection is a CaseTimeoutError, distinguishable from a test failure the next case is refused WITHOUT being started the timer does not keep the process alive a 400ms timer is unaffected while a targeted one collapses — no global perturbation typecheck 4821 files 0 errors, eslint 0 errors, prettier clean. bugfixes 280 passed, servers 41 passed, proxy 74 passed / 6 skipped. tts reports 17 passed / 1 failed (Fish Audio) here AND on release — pre-existing. Corrected the proxy suite's timeout rationale. The comment claimed every case there drives a locally spawned proxy and never a live provider. That is false: nine cases gate on `hasValidCredentials()` and, when it is true, reach Anthropic through the spawned proxy. Verified by counting the call sites rather than taking the review comment's word for it. 180s stays — it is comfortably above a live round-trip and the proxy under test is still one this repo builds — but the justification now says what is actually true, including that a breach is usually a defect and occasionally a slow upstream. A case that legitimately needs longer should carry its own bound rather than have this one raised for everyone.
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.
Why
Two cases in
continuous-test-suite-bugfixesfail on CI runners at a 240s bound while passing everywhere locally, blocking #1487. Different bugs, same consequence: a case that cannot be bounded.1. The proxy-fallback case patched a global to force its timeout
It set
globalThis.setTimeoutto fire every timer at 0ms, restoring it in afinally. Two problems:awaitinside a 280-case suite sharing one process, so for that window it rewrites the delay of every timer created anywhere — including ones belonging to other cases' pending work.finallynever runs and the patch leaks into the rest of the suite.Not theoretical. A peer session trying to measure this case wrote a
setTimeout-based watchdog around it — and the watchdog was rewritten to 0ms by the patch it was measuring, reporting an instant false hang. Twice, at two different bounds, before the instrument was suspected instead of the code.executeClaudeFallbackWithRetrynow takes an optionalidleTimeoutMs(defaulting toFALLBACK_STREAM_IDLE_TIMEOUT_MS); the case passes1. No global is touched, so the class is gone rather than the instance.2.
spawnSync's timeout is not a guarantee — and it defeats #1487 by constructionThe second case drives the CLI through
spawnSyncwithtimeout: 10_000. That timeout sendskillSignal— SIGTERM by default — and then keeps waiting. A child that ignores SIGTERM is never killed andspawnSyncnever returns.Verified directly:
This is the one failure mode that defeats #1487's whole approach.
spawnSyncblocks the event loop, so aPromise.raceper-case bound physically cannot fire while it's stuck — the bound reports only afterspawnSyncreturns, and if it never returns, never. All seven timed subprocess calls in this suite now passkillSignal: "SIGKILL".Worth noting the suite already asserts production code does this: the
audioPlayercase checkssource.includes("killSignal")so a hung decoder can't block the CLI. The rule existed; the tests weren't following it.Proof
The fallback case still catches a real defect — confirmed by removing the abort from the idle-timeout path:
What this does not claim
The CI hang itself is still unexplained. Two theories were refuted before this PR: the unpatched path would have waited the real 120s and passed slowly rather than exceeding 240s; and
isRetryableNetworkErrormatches oncode, which aTimeoutErrordoesn't carry, so there's no 2×120s retry path either.What this removes is the reason those two cases couldn't be bounded or measured honestly when it happens again. If the fallback case hangs after this, the suite bound will actually report it, and any watchdog pointed at it will actually work.
Root cause of the CI failure found and reported by a peer session; the
spawnSyncmechanism is mine.