test: scale broker/app-server spawn timeouts under CPU contention (#53) - #54
Conversation
Concurrent `npm test` runs made unrelated tests fail at almost exactly 5000ms: the broker-smoke IPC-ready wait and the withAppServer fallback watchdogs budget real process spawns, so under contention they lost the race against the machine rather than catching a logic defect. Add scaleTimeout() in tests/helpers.mjs and apply it to those budgets. The multiplier is a flat base rather than an os.loadavg() derivation: loadavg is a 1-minute average that still reads near-idle during the first seconds of a burst, which is exactly when these deadlines are computed — a load-derived multiplier measured 1x while the work needed 4x, and the suite still failed at a flat 5000ms. loadavg is kept only as a floor for the case where it has caught up. Handshake deadlines that are themselves under test (300/500ms) stay unscaled. Also clear the losing watchdog timer via a raceHang() helper. A bare setTimeout in a Promise.race keeps the event loop alive for the full budget even when the real promise wins; scaling the budget exposed this, inflating the file from 6.7s to 31.5s wall-clock. Clearing it brings the two files to 2.98s, faster than before this change. Verified on a 10-core box: 10 concurrent runs of the two files went 10/10 red before, 10/10 green after. Beyond ~10 (1x cores) the failures change character — they hit the production SPAWNED_INITIALIZE_TIMEOUT_MS (10s) in app-server.mjs, which is a genuine signal and deliberately left alone. Full suite 194/194 green; tsc build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185ypzB1bqu9CLxTVyWdrzx
|
@claude @codex review PR #54 at exact head 5211a84 (round 5038f345-2cfb-4ed0-bfdd-a262aabc6fe8). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@claude review PR #54 at exact head 5211a84 (round 4fd3773e-1c4a-4c11-bbcd-fa195fb33ea4). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production. |
|
@claude review PR #54 at exact head 5211a84 (round 4ef08a3b-4f6e-47f3-9204-a8c4fec6a04c). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production. |
1 participant
Fixes #53.
What was wrong
Concurrent
npm testruns made unrelated tests fail at almost exactly 5000 ms. Both offenders budget real process spawns, not logic:broker-smoke.test.mjs—onceMessage(child, "ready")waits for a real broker fork plus a real fake-codex app-server.broker-client-initialize-timeout.test.mjs— theHANGwatchdogs race awithAppServerfallback that spawns a direct app-server.Under contention these lose the race against the machine instead of catching a defect.
The fix
scaleTimeout()intests/helpers.mjs, applied to those budgets.The multiplier is a flat base, not an
os.loadavg()derivation. I tried loadavg first and it did not work: it is a 1-minute average, so it still reads near-idle during the first seconds of a burst — exactly when these deadlines are computed. Measured on a 10-core box, ten concurrent test processes produced a load-derived multiplier of 1 while the work needed ~4x, and the suite still failed at a flat 5000 ms. loadavg is kept only as a floor, for the case where it has genuinely caught up (a CI box already busy at start).Handshake deadlines that are themselves under test (300/500 ms) stay unscaled — scaling them would make the assertion meaningless.
Side fix: leaked watchdog timers
Scaling the budgets surfaced a pre-existing bug. A bare
setTimeoutinside aPromise.racekeeps the event loop alive for the full budget even when the real promise wins. At 5 s it was invisible; at 30 s it inflated the file from 6.7 s to 31.5 s wall-clock despite every test passing. AraceHang()helper now clears the losing timer — the two files run in 2.98 s, faster than before this change.Verification
On a 10-core box, 10 concurrent runs of the two files:
tsc -p tsconfig.app-server.json: clean.Known boundary
Beyond ~10 concurrent runs (1x cores) failures still occur, but they change character: they hit the production
SPAWNED_INITIALIZE_TIMEOUT_MS(10 s) inapp-server.mjs, reported ascodex app-server initialize timed out. That is a genuine signal about the system exceeding its own production budget at 1.6x oversubscription, not a test flake, so I deliberately left it alone — changing it would alter product behavior, which is out of scope for this issue. The scenario reported in #53 (three simultaneousnpm testruns) is covered with a wide margin.Test-only change; no version bump.
🤖 Generated with Claude Code
https://claude.ai/code/session_0185ypzB1bqu9CLxTVyWdrzx