chad/tansweat
1
0
Fork 0
Code Issues Pull requests 2 Projects Releases Packages Wiki Activity Actions

fix(test): wait for a whole pid in the ssh loopback kill tests #53

Open
glen wants to merge 1 commit from fix/ssh-loopback-pid-race into main
pull from: fix/ssh-loopback-pid-race
merge into: chad:main
Conversation 0 Commits 1 Files changed 1 +18 -8
glen commented 2026-11-06 22:31:42 -05:00
Collaborator
Copy link

Fixes the CI failure on #51. That failure wasn't caused by #51; it's a race in an existing test that could hit any PR.

The race

Four tests in sandboxes/ssh/loopback.test.ts spawn a remote shell that runs echo $! > pidFile. They poll until the file exists, then read it once. But > creates (truncates) the file a moment before echo writes the number. A read in that window gets "", which becomes Number("") = 0, and gone(0) is always true (there's no /proc/0/stat).

  • kill test: expect(gone(child)).toBe(false) fails, which is #51's CI log (expected true to be false).
  • the two abort tests: waitGone(0) is instantly true, so they passed without checking anything.

In a tight-poll probe, 358 of 400 reads landed in that window.

The fix

A readPid(file) helper that waits for a complete <positive pid>\n line (up to ~5s, then throws a clear error). All four tests use it. Test-only, one file, +18 −8.

Verification

  • Forced race, before and after: I made the shell create the empty file, wait 0.3s, then write the pid. The original code fails exactly like CI (expected true to be false); this branch passes.
  • Fresh sandbox, independent run: pnpm typecheck passed. The full pnpm test ran 3 times, each 1336 passed / 246 skipped, 0 failed. loopback.test.ts alone passed 20 of 20.
  • Merged into #51 locally: typecheck passed and 1345 tests passed, 0 failed.

After merging

Re-run #51's CI, or merge main into it, and it should go green.

Fixes the CI failure on #51. That failure wasn't caused by #51; it's a race in an existing test that could hit any PR. ## The race Four tests in `sandboxes/ssh/loopback.test.ts` spawn a remote shell that runs `echo $! > pidFile`. They poll until the file **exists**, then read it once. But `>` creates (truncates) the file a moment before `echo` writes the number. A read in that window gets `""`, which becomes `Number("") = 0`, and `gone(0)` is always true (there's no `/proc/0/stat`). - **kill test:** `expect(gone(child)).toBe(false)` fails, which is #51's CI log (`expected true to be false`). - **the two abort tests:** `waitGone(0)` is instantly true, so they passed **without checking anything**. In a tight-poll probe, 358 of 400 reads landed in that window. ## The fix A `readPid(file)` helper that waits for a complete `<positive pid>\n` line (up to ~5s, then throws a clear error). All four tests use it. Test-only, one file, +18 −8. ## Verification - **Forced race, before and after:** I made the shell create the empty file, wait 0.3s, then write the pid. The original code fails exactly like CI (`expected true to be false`); this branch passes. - **Fresh sandbox, independent run:** `pnpm typecheck` passed. The full `pnpm test` ran 3 times, each 1336 passed / 246 skipped, 0 failed. `loopback.test.ts` alone passed 20 of 20. - **Merged into #51 locally:** typecheck passed and 1345 tests passed, 0 failed. ## After merging Re-run #51's CI, or merge `main` into it, and it should go green.
fix(test): wait for a whole pid in the ssh loopback kill tests
All checks were successful
build / build-and-test (pull_request) Successful in 2m41s
Details
build / build-sandbox-image (pull_request) Successful in 1s
Details
build / deploy (pull_request) Has been skipped
Details
e3527416b2 
The remote shell's `echo $! > file` creates (truncates) the file a moment
before it writes the number. The tests polled for the file to EXIST, then
read it once: in that window they read "", Number("") = 0, and pid 0 is
always "gone". That failed CI on #51 (kill test: expected gone(0) false) and
made the two abort tests pass vacuously. readPid waits for a full
"<positive pid>\n" line instead.
All checks were successful
build / build-and-test (pull_request) Successful in 2m41s
Details
build / build-sandbox-image (pull_request) Successful in 1s
Details
build / deploy (pull_request) Has been skipped
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/ssh-loopback-pid-race:fix/ssh-loopback-pid-race
git switch fix/ssh-loopback-pid-race

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff fix/ssh-loopback-pid-race
git switch fix/ssh-loopback-pid-race
git rebase main
git switch main
git merge --ff-only fix/ssh-loopback-pid-race
git switch fix/ssh-loopback-pid-race
git rebase main
git switch main
git merge --no-ff fix/ssh-loopback-pid-race
git switch main
git merge --squash fix/ssh-loopback-pid-race
git switch main
git merge --ff-only fix/ssh-loopback-pid-race
git switch main
git merge fix/ssh-loopback-pid-race
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
chad/tansweat!53
No description provided.