From fa903b2b2c3349775ff10c41bec97489dc71975d Mon Sep 17 00:00:00 2001 From: oldschoola Date: Fri, 19 Jun 2026 17:27:54 -0700 Subject: [PATCH] fix(test): migrate fs.rmSync to removeSyncWithRetries for EBUSY-safe cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Export removeSyncWithRetries from @oh-my-pi/pi-utils as a standalone function, then migrate the highest-impact fs.rmSync call sites: - test/helpers/temp-home-cleanup.ts: 2 fs.rmSync → removeSyncWithRetries (affects all tests using the cleanupTempHome helper) - test/core/apply-patch-regression.test.ts: 16 fs.rmSync → removeSyncWithRetries (the most fs.rmSync calls of any test file) removeSyncWithRetries retries on EBUSY/EPERM/ENOTEMPTY (40x25ms on Windows), matching TempDir.removeSync's retry logic. On Linux/macOS (CI) the retries are a no-op — fs.rmSync succeeds immediately. --- .../test/core/apply-patch-regression.test.ts | 33 ++++++++++--------- .../test/helpers/temp-home-cleanup.ts | 6 ++-- packages/utils/CHANGELOG.md | 4 +++ packages/utils/src/temp.ts | 2 +- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/packages/coding-agent/test/core/apply-patch-regression.test.ts b/packages/coding-agent/test/core/apply-patch-regression.test.ts index 7482eb60a..dfe1c78c7 100644 --- a/packages/coding-agent/test/core/apply-patch-regression.test.ts +++ b/packages/coding-agent/test/core/apply-patch-regression.test.ts @@ -11,6 +11,7 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import { applyPatch, findContextLine, seekSequence } from "@oh-my-pi/pi-coding-agent/edit"; +import { removeSyncWithRetries } from "@oh-my-pi/pi-utils"; describe("regression: indentation adjustment for line-based replacements (2B)", () => { let tempDir: string; @@ -21,7 +22,7 @@ describe("regression: indentation adjustment for line-based replacements (2B)", }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("line-based patch adjusts indentation when fuzzy matching at different indent level", async () => { @@ -102,7 +103,7 @@ describe("regression: ambiguity detection for context-less hunks (2C)", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("single-hunk simple diff rejects multiple occurrences", async () => { @@ -166,7 +167,7 @@ describe("regression: context search uses line hints (2D)", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("unified diff line numbers help locate correct position", async () => { @@ -251,7 +252,7 @@ describe("regression: insertion uses newStartLine fallback (2E)", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("pure addition with context uses context to find insertion point", async () => { @@ -418,7 +419,7 @@ describe("plan: partial line matching for @@ context", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ context matches when actual line contains it as substring", async () => { @@ -482,7 +483,7 @@ describe("plan: unified diff format line numbers", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ -10,6 +10,7 @@ is parsed as line numbers not literal text", async () => { @@ -556,7 +557,7 @@ describe("plan: Codex-style wrapped patches", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("strips *** Begin Patch / *** End Patch wrapper", async () => { @@ -697,7 +698,7 @@ describe("plan: strip + prefix from file creation", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("create file strips + prefix when all lines have it", async () => { @@ -755,7 +756,7 @@ describe("regression: *** End of File marker handling (2A/2G)", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("*** End of File marker is preserved in hunk parsing", async () => { @@ -814,7 +815,7 @@ describe("regression: model edit attempt - @@ line N syntax (session 2026-01-19) }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ line 125 is parsed as line hint, not literal context search", async () => { @@ -866,7 +867,7 @@ describe("regression: model edit attempt - nested @@ anchors (session 2026-01-19 }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ class X followed by @@ method on next line is parsed as nested anchors", async () => { @@ -959,7 +960,7 @@ describe("regression: model edit attempt - space-separated anchors (session 2026 }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ class PatchTool constructor is parsed as hierarchical anchors", async () => { @@ -1047,7 +1048,7 @@ describe("regression: model edit attempt - unique substring on long line (sessio }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ class ClassName matches long export line when unique", async () => { @@ -1127,7 +1128,7 @@ describe("regression: bench edit failures (2026-01-19)", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("@@ @@ is treated as empty context", async () => { @@ -1559,7 +1560,7 @@ describe("regression: trailing context lines don't delete file content", () => { }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("context lines cannot cause collateral deletion via fuzzy match", async () => { @@ -1806,7 +1807,7 @@ describe("regression: context-only hunks between @@ markers must not change inde }); afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); }); test("pure context hunk (no +/- lines) does not alter tab-indented file content", async () => { diff --git a/packages/coding-agent/test/helpers/temp-home-cleanup.ts b/packages/coding-agent/test/helpers/temp-home-cleanup.ts index 8d51583d1..3618d1264 100644 --- a/packages/coding-agent/test/helpers/temp-home-cleanup.ts +++ b/packages/coding-agent/test/helpers/temp-home-cleanup.ts @@ -1,4 +1,4 @@ -import * as fs from "node:fs"; +import { removeSyncWithRetries } from "@oh-my-pi/pi-utils"; export interface TempHomeState { tempDir: string; @@ -10,10 +10,10 @@ export function cleanupTempHome(getState: () => TempHomeState): () => void { return () => { const { tempDir, tempHomeDir, originalHome } = getState(); if (tempDir) { - fs.rmSync(tempDir, { recursive: true, force: true }); + removeSyncWithRetries(tempDir); } if (tempHomeDir) { - fs.rmSync(tempHomeDir, { recursive: true, force: true }); + removeSyncWithRetries(tempHomeDir); } if (originalHome === undefined) { delete process.env.HOME; diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index aca7129c0..0cda11748 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Exported `removeSyncWithRetries()` as a standalone function so tests that manage their own temp dirs can use the same retry-on-EBUSY cleanup logic as `TempDir.removeSync()`. + ## [16.1.3] - 2026-06-19 ### Changed diff --git a/packages/utils/src/temp.ts b/packages/utils/src/temp.ts index 6a8aa557a..1a061854c 100644 --- a/packages/utils/src/temp.ts +++ b/packages/utils/src/temp.ts @@ -95,7 +95,7 @@ async function removeWithRetries(target: string): Promise { } } -function removeSyncWithRetries(target: string): void { +export function removeSyncWithRetries(target: string): void { for (let attempt = 0; ; attempt++) { try { fs.rmSync(target, kRemoveOptions);