Skip to content

Fix three flakes in the sidecar's tool-reap test, including a never-exiting run, and bound CI's Test step - #970

Merged
nedtwigg merged 2 commits into
mainfrom
fix/ci-37124954399
Oct 3, 2026
Merged

nedtwigg merged 2 commits into
mainfrom
fix/ci-37124954399

Conversation

@dormouse-bot

@dormouse-bot dormouse-bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

On main CI run 37124954399 (attempt 1, for #969), Build & Test's Test step ran for 72 minutes until the run was cancelled. Every other package finished by 13:09. standalone/sidecar's node --test never exited, and the only missing results were the three tests in standalone/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 the Test step.

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 --test has 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: 20 on Test, which normally takes about 4 minutes. Any future never-exiting test then fails fast instead of holding main'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 reads HANDED=, so the signal could kill Node before any payload was written. The handler now goes in first.

ENOTEMPTY removing the temp home. A killed shell writes .bash_history into the bare home while close() is deleting it. The session's kill() now waits for pty-core's exit event, 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)
  • Hang: ran node --test tool-reap.test.js 200 times, 8 at a time, with a 40s cap. 12 runs never exited, though each had printed ok for all three tests. In a hung run, the file process held /dev/ptmx, and its child bash was alive (Ss+, in poll) with SIGHUP caught, not ignored. One more kill -HUP killed 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.
  • Production reaper: a standalone loop (Ctrl+C a Node Tool, kill at the prompt) leaked 3 of about 340 shells when killing immediately. Killing 100ms later, the reaper's STOP_SETTLE_MS in lib/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.
  • main's test file, run with --test-force-exit: 200 runs had 32 failures. There were 29 null-payload failures in the bash test and 3 ENOTEMPTY failures in the payload test. No run hung.
  • This PR's head: 200 runs at 8 in parallel, then 400 runs at 12 in parallel. 0 failures and 0 hangs.
  • I ran the whole sidecar suite with the flag. Every PTY test passed. The dor-control-server tests fail in this sandbox with EAFNOSUPPORT on Unix sockets, with or without the flag, so this PR's CI covers them.

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.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: be189ac
Status: ✅  Deploy successful!
Preview URL: https://191a75b6.mouseterm.pages.dev
Branch Preview URL: https://fix-ci-37124954399.mouseterm.pages.dev

View logs

…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.
@dormouse-bot dormouse-bot changed the title Let the sidecar tests exit when a killed bash survives its SIGHUP, and bound CI's Test step Fix three flakes in the sidecar's tool-reap test, including a never-exiting run, and bound CI's Test step Oct 3, 2026
@dormouse-bot

Copy link
Copy Markdown
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. Test started at 14:18. standalone/sidecar test printed nothing after 14:19:05, and the run was cancelled at 14:44. That suite reported 203 of its usual 206 passes, and the three missing are tool-reap.test.js's. Every other package finished by 14:21. When the runner cleaned up, one orphaned bash was still alive under the test-file process, which matches the lost-SIGHUP diagnosis above. This PR's head be189aca is green on every check that ran.

@nedtwigg
nedtwigg merged commit 54d5769 into main Oct 3, 2026
11 checks passed

This branch is waiting to be deployed

1 waiting deployment
hosted-preview — be189aca Waiting Oct 3, 2026 by nedtwigg via cleanup #949
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants