Uh oh!
There was an error while loading. Please reload this page.
Stop a losing rm -rf race from failing a passing test - #613
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the teardown() function in e2e/update_nix_flake.bats to prevent a cleanup race from turning a passing test red. Under bats -j (parallel), each test builds a throwaway git repo in $TMPDIR; on macOS, rm -rf "$WORK" intermittently loses a race for .git/objects and fails with ENOTEMPTY. Since BATS counts a failing teardown as a failing test, this produced false CI failures. The fix retries the removal briefly and then gives up quietly so cleanup can never decide the test outcome. It fits into the broader e2e test suite added/touched by #609.
Changes:
- Replace the single
rm -rf "$WORK"in teardown with a short retry loop (3 attempts, 0.1s apart) that returns success on first successful removal. - Fall back to a final best-effort
rm -rf ... || trueand explicitreturn 0so teardown always succeeds. - Add an explanatory comment documenting the race and rationale.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
update_nix_flake.bats builds a throwaway git repo per test, and under `bats -j` teardown's `rm -rf` intermittently fails with ENOTEMPTY on .git/objects. bats counts a failing teardown as a failing test, so a green assertion gets reported red — twice in six local bin/ci runs, landing on a different test name each time. ENOTEMPTY says the directory was not empty when rm reached it. It does not say what refilled it, and I have not established that, so the fix goes at the teardown rather than at a presumed cause: retry briefly, then give up quietly. Cleanup should not decide whether a test passed, and a leftover directory under TMPDIR is worth less than a signal people trust.
ded56b3 to
b028138CompareUh oh!
There was an error while loading. Please reload this page.
Not part of the SDK stack — a flake I kept hitting while running
bin/ciagainst it, in a file #609 last touched.
What is observed
e2e/update_nix_flake.batsbuilds a throwaway git repo per test. Underbats -j(whiche2e/run.shuses by default), teardown'srm -rf "$WORK"intermittently fails with ENOTEMPTY:
bats counts a failing teardown as a failing test, so the assertion passed and
the test was reported red. The test name it lands on varies between runs — I saw
it on two different tests, which is itself the tell.
It hit twice in six local
bin/ciruns, so it is frequent enough to costreal CI runs and, worse, to train people to re-run on red.
What is not claimed
ENOTEMPTY says only that the directory was not empty when
rmreached it duringparallel execution. It does not identify what put entries back, and I have not
established that. An earlier revision of this description asserted that something
"still holds
.git/objects" — that was a guess dressed as a finding, and it iswithdrawn. The evidence supports "cleanup lost a race under
-j", nothingnarrower.
That uncertainty is exactly why the fix is at the teardown rather than at a
presumed cause.
Fix
Retry briefly, then give up quietly. Cleanup should not decide whether a test
passed, and a leftover directory under
$TMPDIRis worth less than a signalpeople trust. This is a policy choice about teardown, and it holds regardless of
which actor wins the race.
Verification
-j 10) runs of that file: 0 failures.bin/ciexit 0.0 failures.