From 23d9f7c89828fa0ad4c6a9a297f23527ed73f8d4 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 2 Jul 2026 19:11:25 +0000 Subject: [PATCH] fix(session): discarded temp on EPERM guard-reject branches FileSessionStorage.#replaceSessionFileAfterEpermSync now unlinks the staged temp file when commitGuard returns false in both fallback branches: the ENOENT-vanished-target path and the post-move-aside path (where the moved-aside backup is also restored). Honors the writeTextAtomic contract that a guard-rejected stage is discarded. Regressions cover all three guard-reject exits: the direct rename pre-check, the ENOENT branch inside the EPERM fallback, and the move-aside branch that also restores the backup. Each asserts no orphan .tmp remains in the session dir. Fixes #4338 --- .../src/session/session-storage.ts | 9 +- .../rewrite-rename-eperm.test.ts | 87 +++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/session/session-storage.ts b/packages/coding-agent/src/session/session-storage.ts index f7e2bcc60..485aa28f2 100644 --- a/packages/coding-agent/src/session/session-storage.ts +++ b/packages/coding-agent/src/session/session-storage.ts @@ -311,7 +311,10 @@ export class FileSessionStorage implements SessionStorage { this.renameSync(targetPath, backupPath); } catch (moveAsideError) { if (isEnoent(moveAsideError)) { - if (commitGuard && !commitGuard()) return; + if (commitGuard && !commitGuard()) { + this.#discardTemp(tempPath, targetPath); + return; + } this.renameSync(tempPath, targetPath); return; } @@ -320,7 +323,8 @@ export class FileSessionStorage implements SessionStorage { if (commitGuard && !commitGuard()) { // A concurrent synchronous rewrite published a fresh body between the // move-aside and this point. Restore the moved-aside file so we do - // not overwrite it with our staged (stale) body. + // not overwrite it with our staged (stale) body, and drop the temp + // so `writeTextAtomic`'s "discard on abandon" contract holds. try { this.renameSync(backupPath, targetPath); } catch (restoreErr) { @@ -330,6 +334,7 @@ export class FileSessionStorage implements SessionStorage { error: toError(restoreErr).message, }); } + this.#discardTemp(tempPath, targetPath); return; } try { diff --git a/packages/coding-agent/test/session-manager/rewrite-rename-eperm.test.ts b/packages/coding-agent/test/session-manager/rewrite-rename-eperm.test.ts index 5a580433f..e1c670616 100644 --- a/packages/coding-agent/test/session-manager/rewrite-rename-eperm.test.ts +++ b/packages/coding-agent/test/session-manager/rewrite-rename-eperm.test.ts @@ -136,6 +136,93 @@ describe("SessionManager rewrite EPERM rollback failure", () => { }); }); +describe("FileSessionStorage.writeTextAtomic commitGuard cleanup", () => { + let sessionDir: string; + + beforeEach(async () => { + sessionDir = await fsp.mkdtemp(path.join(os.tmpdir(), "omp-guard-cleanup-")); + }); + + afterEach(async () => { + await fsp.rm(sessionDir, { recursive: true, force: true }); + }); + + async function listTempFiles(): Promise { + const names = await fsp.readdir(sessionDir); + return names.filter(name => name.endsWith(".tmp")); + } + + it("discards the staged temp when commitGuard rejects on the direct rename path", async () => { + const storage = new FileSessionStorage(); + const target = path.join(sessionDir, "session.jsonl"); + await storage.writeTextAtomic(target, "existing\n", { commitGuard: () => false }); + expect(await listTempFiles()).toEqual([]); + expect(await Bun.file(target).exists()).toBe(false); + }); + + it("discards the staged temp when the EPERM move-aside fallback's commitGuard rejects", async () => { + let epermAttempted = false; + let guardCalls = 0; + class EpermThenGuardStorage extends FileSessionStorage { + override renameSync(source: string, targetPath: string): void { + if (source.includes(".tmp") && targetPath.endsWith(".jsonl") && !epermAttempted) { + epermAttempted = true; + throw new FsCodeError("EPERM", `EPERM: operation not permitted, rename '${source}' -> '${targetPath}'`); + } + super.renameSync(source, targetPath); + } + } + const storage = new EpermThenGuardStorage(); + const target = path.join(sessionDir, "session.jsonl"); + await fsp.writeFile(target, "seed\n"); + + await storage.writeTextAtomic(target, "next\n", { + commitGuard: () => { + guardCalls += 1; + // First call (before primary rename): pass so we hit EPERM. + // Second call (inside EPERM fallback, after move-aside): reject. + return guardCalls === 1; + }, + }); + + expect(epermAttempted).toBe(true); + expect(guardCalls).toBe(2); + expect(await listTempFiles()).toEqual([]); + // Backup was restored, so target still holds the seed content. + expect(await Bun.file(target).text()).toBe("seed\n"); + const backups = (await fsp.readdir(sessionDir)).filter(name => name.endsWith(".bak")); + expect(backups).toEqual([]); + }); + + it("discards the staged temp when the ENOENT move-aside branch's commitGuard rejects", async () => { + let epermAttempted = false; + let guardCalls = 0; + class EpermMissingTargetStorage extends FileSessionStorage { + override renameSync(source: string, targetPath: string): void { + if (source.includes(".tmp") && targetPath.endsWith(".jsonl") && !epermAttempted) { + epermAttempted = true; + throw new FsCodeError("EPERM", `EPERM: operation not permitted, rename '${source}' -> '${targetPath}'`); + } + super.renameSync(source, targetPath); + } + } + const storage = new EpermMissingTargetStorage(); + const target = path.join(sessionDir, "session.jsonl"); + // Target does not exist, so the move-aside step raises ENOENT. + await storage.writeTextAtomic(target, "next\n", { + commitGuard: () => { + guardCalls += 1; + return guardCalls === 1; + }, + }); + + expect(epermAttempted).toBe(true); + expect(guardCalls).toBe(2); + expect(await listTempFiles()).toEqual([]); + expect(await Bun.file(target).exists()).toBe(false); + }); +}); + describe("recoverOrphanedBackups", () => { it("promotes an orphaned .jsonl..bak back to the primary path when the primary is missing", async () => { const storage = new MemorySessionStorage();