test(gap): de-flake test_gap_node_fs — its temp dir was not unique#6410
Conversation
The test built its scratch directory by hand:
const tmpDir = '/tmp/perry_fs_test_' + Date.now();
fs.mkdirSync(tmpDir, { recursive: true });
Date.now() has millisecond resolution, so two executions that start in the same
millisecond pick the SAME directory — and the test ends by tearing its directory
down with a recursive rmSync. One run then touches a file another run is
deleting. On Linux CI that surfaced as
FAIL test_gap_node_fs (output mismatch)
Node.js: true
Perry: Error: EACCES: Permission denied (os error 13),
stat '/tmp/perry_fs_test_1784051019891/test.txt'
which is not a parity failure at all. It red-lit unrelated PRs at random — PerryTS#6409
(an HIR capture-box change, nothing to do with fs) failed one run and passed the
next on the SAME commit, and PerryTS#6403 failed once then went green untouched.
mkdtempSync is the primitive for this: the kernel picks the suffix and creates
the directory atomically, so no two runs can collide.
Proof, 8 concurrent runs of the compiled binary:
before — 2 of 8 fail (ENOENT: another run's rmSync removed the shared dir)
after — 0 of 8 fail
Output is byte-for-byte unchanged (30 lines); no assertion touched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe filesystem parity test now creates its temporary directory atomically under the system temporary directory using ChangesFilesystem Test Setup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Follow-up: the same hazard is latent in three more gap tests. They use a fixed
( They haven't bitten yet, so I've left them out of this PR rather than expand its scope. Happy to convert them to One honest caveat on the mechanism: what I proved is that the old path collides under concurrency (2 of 8 simultaneous runs fail, 0 of 8 after). I did not establish exactly where CI's concurrency comes from — whether the harness overlaps the node and perry executions, or something else on the runner. Either way the root cause is the same (a non-unique path plus a recursive teardown), and |
1 participant
test_gap_node_fsis flaky, and liketest_gap_fs_fd_2749(#6400) it has been red-lighting unrelated PRs at random. This is the second one from the same family.It isn't a parity failure
The CI diff looks alarming but says nothing about parity:
That's a filesystem permission error, not a wrong answer.
The temp directory is not unique
Date.now()has millisecond resolution, so two executions starting in the same millisecond pick the same directory — and the test ends by tearing its directory down with a recursivermSync. One run then stats a file another run is deleting.Evidence it's a race, not a regression
fs— failed one run and passed the next on the same commit.Running 8 copies of the compiled binary concurrently:
Date.now()path)ENOENT, another run'srmSyncremoved the shared dirmkdtempSync)The fix
mkdtempSyncis the primitive for exactly this: the kernel picks the suffix and creates the directory atomically, so no two runs can collide.Output is byte-for-byte unchanged (all 30 lines); no assertion is touched — only the directory's construction.
Summary by CodeRabbit