Repository navigation
Absorb a re-raised interrupt still in flight when a --load run ends (#2973) - #2978
Merged
Merged
Conversation
…2973) when-exit (via atomically) re-raises the first SIGINT/SIGTERM with process.kill(process.pid, ...). A self-sent signal is handed to whichever thread the kernel picks, and on a loaded machine that delivery can lag the in-flight calls settling. withLoadInterrupt then removed its last listener, restoring the default disposition, and the pending copy killed the CLI by the signal after the partial summary was printed: exit code null, not 130. When the run ends without the re-raise having been seen, leave a one-shot listener that swallows it, added before the run's listeners come off so the signal never has zero listeners. Its removal is driven by the copy arriving, not by a timer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues for the CLI shutdown-race fix.
No open issues 🎉
What changed in this PR
Fixes a CLI --load shutdown race where a delayed re-raised interrupt could terminate the process by signal instead of preserving exit code 130 after printing the partial summary.
Changes:
- Tracks whether the re-raised signal arrived and installs a one-shot absorber before removing run listeners when it has not.
- Adds regression coverage for late signal delivery and updates test helpers to invoke raw listeners so one-shot cleanup behaves correctly.
| File | Description |
|---|---|
clients/cli/src/handlers/load-interrupt.ts |
Absorbs a pending re-raised interrupt after the run ends and documents listener lifetime. |
clients/cli/__tests__/load-summary.test.ts |
Tests late re-raise handling and updates simulated signal delivery and cleanup. |
Review effort: Balanced
Member
Author
|
Copilot review loop closed: round 1 (review 5871309902) was clean — no inline comments, nothing in the headline, no suppressed comments. Exit: clean round. |
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.
Closes #2973
Root cause: a production bug, not a test race
The test was right. The CLI could lose its exit status.
when-exit(pulled in byatomically) hooks SIGINT/SIGTERM withprocess.once, runs its cleanups, and then re-raises the signal withprocess.kill(process.pid, …).withLoadInterruptalready ignores that copy when it arrives while the run is pending (#2834). It did not cover the case where the copy has not arrived by the time the run ends.A signal a process sends to itself is handled by whichever thread the kernel picks. On a loaded machine that thread can wait several milliseconds to be scheduled, which is longer than the in-flight 50ms calls take to settle. The run's
finallythen removed the last SIGINT/SIGTERM listener, so Node restored the default disposition. The pending copy then landed and killed the process by the signal: the partial summary had already been printed, but the process exited with codenullinstead of130. A real user hitting Ctrl+C or a CI job sending SIGTERM on a busy host gets the same wrong status.I confirmed this by instrumenting the built CLI under load. Each failing child logged the first signal, then
finally2–3ms later, and never logged the re-raise. Each passing child logged the re-raise about 0.3–1ms after the first signal, beforefinally. The summary was on stdout in every failure.Fix (
clients/cli/src/handlers/load-interrupt.ts)echoed).process.once) no-op listener for the first signal. It is added before the run's listeners are removed, so the signal never has zero listeners. A synchronous gap would not be safe, because the copy is delivered on another thread.The one cost is a caller with no re-raising dependency that invokes the listener directly, such as the in-process tests. It is left with one no-op
oncelistener that swallows one later signal and then removes itself. The header comment documents this.Tests (
clients/cli/__tests__/load-summary.test.ts)signalLoadRunnow calls the raw listener.process.listeners()returns the function aoncewrapper wraps, and calling that directly would leave the wrapper registered.load-interrupt-e2e.test.tsis unchanged; it is the test that catches this.Reproduction, before and after
I ran
load-interrupt-e2e.test.tsin 4 parallel lanes of 15 runs each, alongside 30 busy-loop CPU burners on an 18-core machine. Load average was about 30–40 with other agent gates also running.origin/v2/mainexpected null to be 130An earlier 32-run pass on the original code under the same load failed 2 times (both SIGINT; this is where the instrumentation above came from). Both signals share the race. The issue happened to observe the SIGTERM case.
npm run local:gateis green.🤖 Generated with Claude Code