Fix three flakes in the sidecar's tool-reap test, including a never-exiting run, and bound CI's Test step - #970
Merged
Merged
Conversation
tool-reap.test.js kills a PTY right as bash redraws its prompt after Ctrl+C; bash occasionally loses that SIGHUP and stays alive, and its PTY handle keeps the node --test file process open forever. Run the sidecar suite with --test-force-exit, and bound CI's Test step so any future never-exiting test file fails in minutes instead of hours.
Deploying mouseterm with
|
| Latest commit: |
be189ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://191a75b6.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-ci-37124954399.mouseterm.pages.dev |
…ites The test Tool printed HANDED= before installing its SIGINT handler, and the test sends Ctrl+C as soon as it reads HANDED=, so the signal could kill Node before any dehydrate payload (red on this PR's first CI run, zsh). The session's kill() and close() also removed the bare home while the killed shell was still writing its history (ENOTEMPTY). Install the handler first, and wait (bounded) for the shell's exit before respawning or removing.
Collaborator
Author
|
Rerunning main doesn't clear this, so main stays red until this PR merges. Attempt 2 of run 37124954399 hung the same way. |
2 tasks done
nedtwigg
requested a deployment
to
hosted-preview
October 3, 2026 14:53 — with
GitHub Actions
Waiting
This branch is waiting to be deployed
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.
On main CI run 37124954399 (attempt 1, for #969),
Build & Test'sTeststep ran for 72 minutes until the run was cancelled. Every other package finished by 13:09.standalone/sidecar'snode --testnever exited, and the only missing results were the three tests instandalone/sidecar/tool-reap.test.js. #969 changes nothing in the sidecar. That file has three intermittent faults, and this PR fixes all of them. It also puts a 20-minute bound on theTeststep.The hang (the cancelled main run). The test kills its PTY the moment bash redraws its prompt after Ctrl+C. Occasionally bash loses that SIGHUP and stays alive, sleeping in readline. The live PTY handle keeps the test-file process open even though all three tests have passed.
node --testhas no per-file bound and the step had none, so the job would have sat until GitHub's 6-hour default.standalone/sidecar/package.json:node --test --test-force-exit. Once all tests finish, the file process exits. That closes the PTY master, which takes the leaked bash with it..github/workflows/ci.yml:timeout-minutes: 20onTest, which normally takes about 4 minutes. Any future never-exiting test then fails fast instead of holdingmain's CI for hours.Null dehydrate payload (red on this PR's first CI run, zsh variant). The test Tool printed
HANDED=before installing its SIGINT handler. The test sends Ctrl+C as soon as it readsHANDED=, so the signal could kill Node before any payload was written. The handler now goes in first.ENOTEMPTYremoving the temp home. A killed shell writes.bash_historyinto the bare home whileclose()is deleting it. The session'skill()now waits for pty-core'sexitevent, bounded at 3s for the lost-SIGHUP case, before the next spawn or the removal.Reproduction and verification (local, Linux, bash 5.2.21, node-pty 1.2.0-beta.15; zsh is not installed here)
node --test tool-reap.test.js200 times, 8 at a time, with a 40s cap. 12 runs never exited, though each had printedokfor all three tests. In a hung run, the file process held/dev/ptmx, and its child bash was alive (Ss+, inpoll) with SIGHUP caught, not ignored. One morekill -HUPkilled it, and the test process exited at once. Every kill in the file was a leak site, and each was issued right after the post-Ctrl+C prompt.STOP_SETTLE_MSinlib/src/components/wall/tool-reaper.ts, leaked 0 of about 580. So the reap path does not visibly hit this, but the test's immediate kill does.--test-force-exit: 200 runs had 32 failures. There were 29 null-payload failures in the bash test and 3ENOTEMPTYfailures in the payload test. No run hung.dor-control-servertests fail in this sandbox withEAFNOSUPPORTon Unix sockets, with or without the flag, so this PR's CI covers them.