test(media): stop the media-links temp-dir cleanup failing its own race - #320
Conversation
`mediaLinksRegistry.test.ts` went red intermittently on CI with
ENOTEMPTY: directory not empty, rmdir '/tmp/openscreen-media-links-XXXXXX'
attributed to "survives the directory disappearing while the refresh is queued"
— the one test that deliberately removes the temp dir while a refresh write is
still queued. Seen twice, including on a `main` push (run 31279698618), so it is
not tied to any one branch.
`force: true` covers ENOENT, which is the write LOSING and the path already being
gone. It does not cover ENOTEMPTY, which is the write WINNING: it recreates an
entry between rm's recursive walk and its final rmdir. `fs.rm` does not retry
unless asked — `maxRetries` defaults to 0 — so the rejection escapes, and
`withoutUnhandledRejections` awaits its callback without a catch, so it fails the
test. The test's own comment already says either outcome is fine ("Whoever wins
the race is fine — what must not happen is a rejection escaping into the
process") and the assertion is on `rejections`; only the removal disagreed.
Both removal sites are exposed, not just the one in the test body: `afterEach`
inherits the same race, because a queued refresh can outlive the test that
started it. That is literally the shape the test comment describes. Both now go
through `rmBestEffort()`, which retries a few times so the directory still gets
cleaned, and tolerates ENOTEMPTY specifically so losing is never a red test. The
catch stays narrow — any other errno still surfaces.
Reproduced the mechanism outside the suite by racing a write against `fs.rm` over
a large tree: 14 failures in 40. Through `rmBestEffort` the same harness gives
0 escaping rejections and 0 leaked directories in 40, while an EACCES target
still rethrows. Suite: 142 files, 1698 passed, 0 failed.
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
1 participant
Summary
electron/media/mediaLinksRegistry.test.tsgoes red intermittently on CI:Seen twice, including on a
mainpush (run 31279698618, commitbd5e71ad) — so it is not tied to any one branch. It most recently took down an unrelated PR'sTestjob, which passed on a plain re-run.Why it happens
force: truecoversENOENT— the write lost the race and the path is already gone. It does not coverENOTEMPTY, which is what the write winning looks like: it recreates an entry betweenfs.rm's recursive walk and its finalrmdir. Andfs.rmdoes not retry unless asked —maxRetriesdefaults to0.The rejection then escapes.
withoutUnhandledRejectionsawaits its callback in atry/finallywith nocatch, so it fails the test.The test already declares either outcome acceptable, and asserts on
rejections, not on the removal:Only the removal disagreed.
Both sites, not one
afterEachinherits exactly the same exposure — a queued refresh can outlive the test that started it, which is literally the shape the test's own comment describes. Fixing only the in-test removal would leave the hook flaky for every other case in the suite. Both now go throughrmBestEffort(): retries a few times so the directory still gets cleaned, toleratesENOTEMPTYso a loss is never a red test, and rethrows every other errno so real failures still surface.Related issue
No existing issue — found while investigating a CI failure on #315.
Type of change
Release impact
Test-only; no shipped code changes.
Desktop impact
Screenshots / video
No visual change.
Testing
Reproduced the mechanism outside the suite, by racing a write against
fs.rmover a large tree:fs.rm(recursive, force)rmBestEffort()rmBestEffort()on an EACCES targetNote the flake does not reproduce by simply re-running the suite locally (0 failures in 15 runs of the file, before or after): the window only opens under CI-level load. The harness above is what makes it observable.
Suite —
npx vitest run→ 142 files, 1698 passed, 1 skipped, 0 failed.biome checkandnpx tsc --noEmitclean.🤖 Generated with Claude Code