From 4e378d37d82555b88dfed0927a886930a0c54a92 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 1 May 2026 22:37:48 -0400 Subject: [PATCH] fix(#3008): kill cross-process race in install-minimal:307 mid-copy test (#3009) Old shape compared listTmpStageDirs() snapshots before/after the mid-copy throw. Under scripts/run-tests.cjs --test-concurrency=4, tests/install-minimal-all-runtimes.test.cjs runs in a parallel subprocess and also creates gsd-minimal-skills-* dirs in shared os.tmpdir(). The parallel process's create/remove activity between this test's two snapshots caused deterministic failure when timing aligned -- presented as 'flaky' but is a real race. CI failure data (PR #2993 run 25238555786): expected (before): ['gsd-minimal-skills-km1O1O'] actual (after): [] Both processes behaved correctly in isolation. The test was wrong: it observed a shared filesystem state across processes. Fix: stub fs.mkdtempSync inside this test to record THIS call's stage dir path. After the throw, assert fs.existsSync(stagedDir) === false. Direct observation of the function's own behavior; no global tmpdir scan; no parallel-process interference. Closes #3008 --- .changeset/calm-tigers-frolic.md | 5 +++ tests/install-minimal.test.cjs | 62 ++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 22 deletions(-) create mode 100644 .changeset/calm-tigers-frolic.md diff --git a/.changeset/calm-tigers-frolic.md b/.changeset/calm-tigers-frolic.md new file mode 100644 index 000000000..92b163e1a --- /dev/null +++ b/.changeset/calm-tigers-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3008 +--- +**`tests/install-minimal.test.cjs:307` no longer races on shared `os.tmpdir()` under parallel CI** — the previous shape compared `listTmpStageDirs()` snapshots before and after the throw. Under `scripts/run-tests.cjs --test-concurrency=4`, `tests/install-minimal-all-runtimes.test.cjs` runs in a parallel process and creates/removes `gsd-minimal-skills-*` dirs in the shared OS tmpdir between snapshots, so `deepStrictEqual` failed deterministically when the parallel process happened to have a live stage dir during the snapshot window. Fix: stub `fs.mkdtempSync` to record THIS call's stage dir, then assert that exact path no longer exists after the throw — no global filesystem snapshot, no race. (#3008) diff --git a/tests/install-minimal.test.cjs b/tests/install-minimal.test.cjs index b341cb9ca..df3709b45 100644 --- a/tests/install-minimal.test.cjs +++ b/tests/install-minimal.test.cjs @@ -305,33 +305,51 @@ describe('install-profiles: cleanupStagedSkills', () => { }); test('mid-copy failure removes the partial staged dir and re-throws', () => { + // The previous shape of this test compared listTmpStageDirs() snapshots + // before and after the throw. That assertion was unsound under + // `--test-concurrency=4` (scripts/run-tests.cjs:24): a parallel test + // process (notably install-minimal-all-runtimes.test.cjs, which also + // calls stageSkillsForMode) creates and removes `gsd-minimal-skills-*` + // dirs in the shared os.tmpdir() between our two snapshots, so + // deepStrictEqual failed deterministically when the parallel process + // happened to have a live stage dir during our snapshot window. + // + // Fix: observe THIS test's own stage dir directly. Stub fs.mkdtempSync + // to record the path stageSkillsForMode creates; on throw, assert that + // exact path no longer exists. No global tmpdir scan, no race with + // parallel processes. cleanupStagedSkills(); const src = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-stage-fail-')); fs.writeFileSync(path.join(src, 'plan-phase.md'), '# plan\n'); - try { - // Force a failure mid-loop by making fs.copyFileSync throw on the - // second allowlisted file. Capture the staged dir from the first - // successful call (we can't see it directly, so we count tmp dirs). - const before = listTmpStageDirs(); - const realCopy = fs.copyFileSync; - let copyCount = 0; - fs.copyFileSync = (s, d) => { - copyCount++; - if (copyCount === 2) throw new Error('synthetic disk full'); - return realCopy(s, d); - }; - // Need at least 2 allowlisted files in src for the second copy to fire. - fs.writeFileSync(path.join(src, 'execute-phase.md'), '# x\n'); - try { - assert.throws(() => stageSkillsForMode(src, 'minimal'), /synthetic disk full/); - } finally { - fs.copyFileSync = realCopy; + fs.writeFileSync(path.join(src, 'execute-phase.md'), '# x\n'); + const realCopy = fs.copyFileSync; + const realMkdtemp = fs.mkdtempSync; + let stagedDir = null; + fs.mkdtempSync = (prefix, ...rest) => { + const out = realMkdtemp(prefix, ...rest); + // Only track the stage dir created by stageSkillsForMode (its + // `gsd-minimal-skills-` prefix). Don't capture our own + // `gsd-stage-fail-` parent dir created above. + if (typeof prefix === 'string' && prefix.endsWith('gsd-minimal-skills-')) { + stagedDir = out; } - const after = listTmpStageDirs(); - // Partial dir must have been cleaned up by stageSkillsForMode itself - // before re-throwing — so the count is unchanged. - assert.deepStrictEqual(after, before, 'partial staged dir should be removed on throw'); + return out; + }; + let copyCount = 0; + fs.copyFileSync = (s, d) => { + copyCount++; + if (copyCount === 2) throw new Error('synthetic disk full'); + return realCopy(s, d); + }; + try { + assert.throws(() => stageSkillsForMode(src, 'minimal'), /synthetic disk full/); + assert.notStrictEqual(stagedDir, null, + 'stageSkillsForMode should have invoked mkdtempSync before the copy throw'); + assert.equal(fs.existsSync(stagedDir), false, + `partial staged dir should be removed on throw, but ${stagedDir} still exists`); } finally { + fs.copyFileSync = realCopy; + fs.mkdtempSync = realMkdtemp; fs.rmSync(src, { recursive: true, force: true }); cleanupStagedSkills(); }