From 87a48fd85c232233b7db51a84c3caf2572787252 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Sun, 9 Aug 2026 00:18:08 +0200 Subject: [PATCH] test(media): stop the media-links temp-dir cleanup failing its own race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- electron/media/mediaLinksRegistry.test.ts | 31 +++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/electron/media/mediaLinksRegistry.test.ts b/electron/media/mediaLinksRegistry.test.ts index bf50b11cf..0e50905db 100644 --- a/electron/media/mediaLinksRegistry.test.ts +++ b/electron/media/mediaLinksRegistry.test.ts @@ -13,6 +13,33 @@ async function makeTempDir(): Promise { return fs.mkdtemp(path.join(os.tmpdir(), "openscreen-media-links-")); } +/** + * `fs.rm` that is allowed to lose a race against a write landing in the directory. + * + * `force: true` covers ENOENT — the write lost and the path is already gone — but + * NOT ENOTEMPTY, which is what the write WINNING looks like: 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 that rejection escapes and fails whichever + * test or hook was running. + * + * This suite races a write against removal on purpose (see "survives the directory + * disappearing while the refresh is queued"), and every `afterEach` inherits the + * same exposure because a queued refresh can outlive the test that started it. + * Losing is explicitly fine — the contract under test is that no rejection escapes + * into the process, not that the removal succeeds. The retries are so the temp dir + * still usually gets cleaned up; the catch is so a loss is never a red test. + * + * Seen twice on CI, once on a `main` push (run 31279698618), and reproduced 14 + * times in 40 locally by racing a write against `fs.rm` over a large tree. + */ +async function rmBestEffort(dir: string): Promise { + try { + await fs.rm(dir, { recursive: true, force: true, maxRetries: 3 }); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOTEMPTY") throw err; + } +} + async function writeFileOfSize(filePath: string, sizeBytes: number, fill = "a"): Promise { // Not all-identical bytes at the seams so head/tail samples aren't trivially // equal to each other for small files — irrelevant for correctness, just @@ -28,7 +55,7 @@ describe("mediaLinksRegistry", () => { }); afterEach(async () => { - await fs.rm(tempDir, { recursive: true, force: true }); + await rmBestEffort(tempDir); }); describe("computeFingerprint", () => { @@ -288,7 +315,7 @@ describe("mediaLinksRegistry", () => { try { const rejections = await withoutUnhandledRejections(async () => { const lookup = findMediaLinksByFingerprint(tempDir, moved); - await fs.rm(tempDir, { recursive: true, force: true }); + await rmBestEffort(tempDir); await lookup; }); expect(rejections).toEqual([]);