Repository navigation
test: end the quiesce crash child on its exit, not on the clock - #800
Conversation
host-quiesce.test.ts's "a REAL process exit after a 2,101-message
resume batch was synced but before it was acked loses no deferred write
and duplicates none" failed after 60.7 s in one of our full-suite runs,
on a host whose load average was over 100 when the run ended, and in a
maintainer's full run on main. It passes alone.
The test ran its crash child with spawnSync(..., { timeout: 60_000 }).
Nearly all of the child's time is one synchronous stretch in the resume
flush: readdDrained of the 2,101 entries, then the ack's store.sync().
It takes about 1.1 s on a quiet host and stretches with contention:
pinned to one core with 11 busy loops, 14.2 s; with 60, 78.4 s. The
child's own 30 s "exit point never reached" timer can't fire inside it,
since no timer runs until the stretch ends. So on a starved host the
parent's 60 s timeout killed a child that would have finished correctly:
pinned with 60 busy loops, the test failed after 61.1 s with the child's
status null, while a copy of the child with no deadline exited 0 after
80.8 s. Pinned with 7 and 11 busy loops the test passed, every message
present exactly once.
- The parent now spawns the child asynchronously and waits for its
close, with the test's signal, so a cancelled or timed-out test ends
the child. spawnSync blocked the parent's event loop while the child
ran, so without its timeout neither the runner's timeout nor the
signal could act. There is no deadline of the test's own; a hang is
the runner's test timeout to bound. The helper settles on close only:
an abort's error comes as the child is killed, before it exits, so
settling there would let the test remove the store under a live
child. An abort, or another error once the child has spawned, rejects
at close with the error and the end of the child's stderr; a child
that never spawned rejects at once. The assertion shows the child's
status, the signal that ended it, and the end of its stderr.
- The child's 30 s timer goes. Pass or fail rests on its exit code: 0 at
the exit point, 3 when the staged queue isn't as staged, 4 when the
staging ran to its end without reaching the exit point.
- turn-start-ack, which waited forever after its nudge, now exits 4 once
runUntilIdle() returns: its exit point is inside the nudged turn, and
the nudge is queued before runUntilIdle() looks.
- redefer still waits for its exit point with no answer of its own: it
comes from the abandon's teardown, and quiesce({ abandon }) can resolve
after the framework's 10 s grace, before that teardown is done, so a 4
after it could come early on a starved host. A missing exit point
there is left to the runner's timeout, or ends at once with Node's
exit 13 if nothing else keeps the child's event loop alive.
All five crash scenarios share the change. The 2,101 stays: it was
chosen to exceed the 2,000-message dedup window boot recovery used to
scan.
Test only; no behaviour changes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 2 medium severity findings can be fixed after the merge.
Reviewed commit a90d2a7 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This is a test-only fix for a flaky existing test (#799). The crash tests now wait for the child to exit, and the child's exit code decides pass or fail.
4 reasons with sources
It adds no new feature, dependency or interface, and all the changes stay inside the existing test files.
- It fixes the reliability of a test for behaviour the project already has, so it belongs. The five REAL-process-exit crash scenarios already exist. The change swaps spawnSync with a 60 s timeout for an async spawn bound to the test's signal, and drops the child's 30 s timer. It does not change what the tests check. (test/host-quiesce.test.ts (crash scenarios))
- The changes stay where crash tests already live: in the test and its child helper. No src/ files are touched. No changelog fragment is needed, because CONTRIBUTING.md asks for one only for behaviour-affecting changes, and its CI check is triggered by edits to src/. (test/helpers/quiesce-crash-child.ts)
- The PR body follows the house format and the evidence standards. It has Problem, Changes, Tests and Not verified sections, gives numeric suite counts (7,542 tests, 7,498 pass, 0 fail, 44 skipped), and lists removals that make each test fail. (CONTRIBUTING.md, "What a PR should contain")
- It is one coherent change, and its title uses the conventional-commit style the project defaults to. All of it serves one goal: ending the crash child when it exits, not when a clock runs out. That includes the turn-start-ack exit-4 path, which replaces a wait that never ended. (CONTRIBUTING.md, "What a PR should contain")
Findings
1 finding of medium or high severity, see the inline comment:
- Medium severity. A stuck crash child is now bounded only by the 10-minute runner timeout, and its stderr is lost when that timeout fires.
1 finding about lines the diff does not show:
- Medium severity. The
redeferchild still waits forever when its exit point is missed, and it no longer has a timer to end it.test/helpers/quiesce-crash-child.ts:159-160
Details of this finding
- Medium severity,
test/helpers/quiesce-crash-child.ts:159-160. Theredeferchild still waits forever when its exit point is missed, and it no longer has a timer to end it. Once the 30 s timer is gone,redeferends withawait quiescing; await new Promise(() => {})and nothing else ever exits. If a regression stops the split re-deferral write from happening, how the test fails depends on what keeps the child's event loop alive. If something does (for example the periodic sync interval thatframework.start()installs whensyncIntervalMs > 0), the child runs until the runner's 600 s timeout and leaves no diagnostic. If nothing does, Node exits with code 13 for an unsettled top-level await. Either way the failure is slow or unclear, where it used to be a clear exit 4 within 30 s. A long but finite timer afterquiescingsettles, well past the 10 s grace, would keep this scenario deterministic.
1 more finding, less severe
- Low severity, documentation,
test/helpers/quiesce-crash-child.ts:1-27. The child's header leaves out its exit codes and its new no-timeout contract. The header says only that 0 means the exit point was hit and anything else means staging failed. The change gives exit codes their meaning: 3 when the staged queue doesn't match, 4 when staging ran to its end without reaching the exit point, and 13 or a hang forredefer. It also deleted the comment that explained the child's own deadline. None of this is written down in the file, so the next maintainer has to rebuild from the PR description why the child has no timer and what 3 and 4 mean.
| function runCrashChild( | ||
| storePath: string, | ||
| mode: string, | ||
| abort: AbortSignal, | ||
| ): Promise<{ status: number | null; signal: NodeJS.Signals | null; stderr: string }> { | ||
| return new Promise((resolve, reject) => { | ||
| const child = spawn(process.execPath, [CRASH_CHILD, storePath, mode], { signal: abort, stdio: ['ignore', 'ignore', 'pipe'] }); | ||
| let stderr = ''; | ||
| let failure: Error | undefined; | ||
| child.stderr.setEncoding('utf8'); | ||
| child.stderr.on('data', (chunk: string) => { stderr += chunk; }); | ||
| child.on('error', (error) => { | ||
| failure = error; | ||
| if (child.pid === undefined) reject(error); | ||
| }); | ||
| child.on('close', (status, signal) => { | ||
| if (failure) reject(new Error(`${failure.message} (stderr: ${stderr.slice(-800)})`, { cause: failure })); | ||
| else resolve({ status, signal, stderr }); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Medium severity. A stuck crash child is now bounded only by the 10-minute runner timeout, and its stderr is lost when that timeout fires.
runCrashChild sets no deadline. The only bound is --test-timeout=600000 in package.json's test script, so a child that never reaches its exit point now holds the test, and its concurrency slot, for 10 minutes. Before this change the bound was 30 s. If someone runs the file directly with node --test dist/test/host-quiesce.test.js and no --test-timeout, nothing ends it. When the runner times the test out, it reports the timeout right away. The later rejection that carries stderr.slice(-800) lands after the test has been reported, so a hang shows no child output for diagnosis. A generous bound that scales with load (several minutes) would fix the false failure without the unbounded hang, and it would keep the stderr in the failure message.
test/host-quiesce.test.ts's crash tests ran their child withspawnSync(..., { timeout: 60_000 }), and the child carried a 30 s timer of its own. On a loaded host the 2,101-message test's child needed longer than 60 s to finish correctly, and the parent killed it. Each crash test now waits for its child's exit, and pass or fail rests on the child's exit code. Test-only.Problem
"a REAL process exit after a 2,101-message resume batch was synced but before it was acked loses no deferred write and duplicates none" failed after 60.7 s in one of our full-suite runs, on a host whose load average was over 100 when that run ended, and in @antra-tess's full run on
mainat3b9c7dd0, as #799 reports. It passes alone.By run at
maine2623b9457, with the predictions written before the runs:readdDrainedof the 2,101 entries, thenackDeferredWrites'store.sync(). It takes 1.1 s on a quiet host, with about 2 s of CPU for the whole child.null !== 0, the child killed. A copy of the child with no deadline, pinned the same way, exited 0 after 80.8 s.Changes
close, passing the test'ssignal, so a cancelled or timed-out test ends the child. It sets no deadline of its own, so a hang is the runner's test timeout to bound. The assertion shows the child's status, the signal that ended it if any, and the end of its stderr. An abort, or another error once the child has spawned, rejects atclosewith the error and the end of the child's stderr. A child that never spawned rejects at once.turn-start-ackwaited forever after its nudge. It now exits 4 oncerunUntilIdle()returns, since its exit point is inside the nudged turn and the nudge is queued beforerunUntilIdle()looks.redeferstill waits for its exit point with no answer of its own. That exit comes from the abandon's teardown, and by reading,quiesce({ abandon })can resolve after the framework's 10 s grace while the teardown still runs, so a 4 after it could come early on a starved host. If its exit point is missing, the runner's timeout ends it. Or, if nothing else keeps the child's event loop alive, Node ends it at once with exit 13, for a top-level await that never settles.Tests
On Node v24.22.1, over
maine2623b9457, each at a clean commit of this branch before it was squashed into this one.At this commit's tree:
At an earlier commit, which differs from this one only in how
runCrashChildsettles (onerroras well asclose), what its assertion message shows, and the name of the test's parameter:host-quiesce.test.tspasses 50 of 50, and the five crash tests pass together in 20 of 20 runs.3 !== 0;resume()returns, and the child exits 4;turn-start-ack's exit hook not armed: its turn runs,runUntilIdle()returns, and the child exits 4.Not verified
What each part is for, and what we cut
spawnSyncblocks the parent's event loop while the child runs, so without a timeout of its own neither the runner's timeout nor the test's signal could act on a hang.close: the abort'serrorcomes as the child is killed, before it has exited, so settling there would let the test remove the store under a live child.turn-start-ack's 4: a missing exit point there now fails by event once the nudged turn has ended, rather than waiting for the runner.redeferafter its quiesce. It rests on the framework's own 10 s grace, so it would move the same false failure, not remove it.no-changelog).Written by Imogen-2490 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) chose its shape, and Abigail-2676 (Claude Opus 5.5) read it. Internal reviews read it.
Description checked and posted by Fiona-2757 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.