diff --git a/src/claude/agents-inject.ts b/src/claude/agents-inject.ts index f40b8a97e4..07dd738def 100644 --- a/src/claude/agents-inject.ts +++ b/src/claude/agents-inject.ts @@ -11,9 +11,10 @@ * Ownership contract: this module only creates/overwrites/deletes files matching * `ocx-*.md` inside the agents dir. User-authored agents are never touched. */ -import { lstatSync, mkdirSync, readdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; +import { lstatSync, mkdirSync, readdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import type { OcxConfig } from "../types"; +import { renameAtomicFile } from "../lib/windows-atomic-replace"; import { claudeCodeAlias, claudeCodeNativeAlias } from "./alias"; import { AUTO_CONTEXT_OFF, shouldMarkOneMillion, stripOneMillionMarker, withOneMillionMarker } from "./context-windows"; import { claudeConfigDir } from "./gateway-cache"; @@ -235,7 +236,7 @@ export function syncClaudeAgentDefs(defs: readonly ClaudeAgentDef[], configDir = } catch { /* does not exist: ours to create */ } const tmp = `${target}.tmp-${process.pid}`; writeFileSync(tmp, renderAgentDef(def), { encoding: "utf8", mode: 0o644 }); - renameSync(tmp, target); + renameAtomicFile(tmp, target); written.push(def.file); } return written; diff --git a/src/codex/prompt-journal.ts b/src/codex/prompt-journal.ts index 4da0edbcc8..f566f2ede8 100644 --- a/src/codex/prompt-journal.ts +++ b/src/codex/prompt-journal.ts @@ -19,10 +19,11 @@ * concrete: crash after writing config.toml, user or Codex then edits it, * recovery sees a mismatch and overwrites their work with a stale image. */ -import { existsSync, mkdirSync, openSync, closeSync, fsyncSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, openSync, closeSync, fsyncSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { createHash, randomBytes } from "node:crypto"; import { forgetEphemeralSecretPath, hardenSecretPath, windowsSecretAclApplies } from "../lib/windows-secret-acl"; +import { renameAtomicFile } from "../lib/windows-atomic-replace"; const FILE_MODE = 0o600; const DIR_MODE = 0o700; @@ -79,7 +80,10 @@ export function durableWrite(path: string, content: string): void { fsyncSync(fd); closeSync(fd); fd = undefined; - renameSync(tmp, path); + // Windows can refuse the replace with EBUSY/EPERM/EACCES while a scanner + // still holds the target; the shared helper retries that briefly. Losing + // this publish breaks journal restore, so it should not fail on a blink. + renameAtomicFile(tmp, path); // The temp is renamed away: proven absent — release its ACL memos. forgetEphemeralSecretPath(tmp); fsyncDir(path); diff --git a/src/config.ts b/src/config.ts index d879195e88..c041f04cf5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,6 +1,6 @@ import { execFileSync } from "node:child_process"; import { createHash, randomUUID } from "node:crypto"; -import { chmodSync, constants as fsConstants, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, readFileSync, realpathSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, constants as fsConstants, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, readFileSync, realpathSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join, resolve } from "node:path"; import { Database } from "bun:sqlite"; @@ -93,34 +93,13 @@ import { isHostedToolUnsupportedForModel } from "./responses/hosted-tool-policy" let _atomicSeq = 0; -interface AtomicRenameIO { - platform: NodeJS.Platform; - rename: (source: string, destination: string) => void; - sleep: (milliseconds: number) => void; -} - -export function renameAtomicFile( - source: string, - destination: string, - io: AtomicRenameIO = { - platform: process.platform, - rename: renameSync, - sleep: Bun.sleepSync, - }, -): void { - for (let attempt = 0; ; attempt += 1) { - try { - io.rename(source, destination); - return; - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - const transientWindowsError = io.platform === "win32" - && (code === "EBUSY" || code === "EPERM" || code === "EACCES"); - if (!transientWindowsError || attempt >= 2) throw error; - io.sleep(25 * (attempt + 1)); - } - } -} +// The Windows-tolerant replace lives in lib/windows-atomic-replace: config-ownership +// is one of its callers and this module already imports config-ownership, so +// exporting it from here would close an import cycle. Re-exported because these +// names are part of this module's public surface and its callers. +export type { AtomicRenameIO } from "./lib/windows-atomic-replace"; +export { renameAtomicFile } from "./lib/windows-atomic-replace"; +import { renameAtomicFile, renameAtomicFileAsync } from "./lib/windows-atomic-replace"; /** * Write a file atomically (temp + rename) so concurrent writers — e.g. `ocx stop` and the @@ -284,21 +263,6 @@ export interface AtomicWriteAsyncTestSeam { afterTempWrite?: (tempPath: string) => void | Promise; } -async function renameAtomicFileAsync(source: string, destination: string): Promise { - for (let attempt = 0; ; attempt += 1) { - try { - renameSync(source, destination); - return; - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - const transientWindowsError = process.platform === "win32" - && (code === "EBUSY" || code === "EPERM" || code === "EACCES"); - if (!transientWindowsError || attempt >= 2) throw error; - await Bun.sleep(25 * (attempt + 1)); - } - } -} - /** * Async atomic write (#612): same temp+harden+rename and residual-temp policy as * atomicWriteFile, but Windows ACL harden yields the event loop. Timeout memo is keyed diff --git a/src/lab/automation/config-persistence.ts b/src/lab/automation/config-persistence.ts index e4dca2992f..20d70a8a3b 100644 --- a/src/lab/automation/config-persistence.ts +++ b/src/lab/automation/config-persistence.ts @@ -6,11 +6,11 @@ import { linkSync, openSync, readFileSync, - renameSync, unlinkSync, writeFileSync, } from "node:fs"; import { dirname, join } from "node:path"; +import { renameAtomicFile } from "../../lib/windows-atomic-replace"; import { ensureLabDirs, labAutomationPolicyPath, @@ -217,7 +217,7 @@ function writeConfigUnlocked( if (configCommitFaultForTests === "before_publish") { throw new LabAutomationError("synthetic automation config commit failure", "invalid_state"); } - renameSync(tmp, path); + renameAtomicFile(tmp, path); return normalized; } finally { if (fd !== null) closeSync(fd); diff --git a/src/lab/automation/persistence.ts b/src/lab/automation/persistence.ts index ef9ddb399b..b035301ff6 100644 --- a/src/lab/automation/persistence.ts +++ b/src/lab/automation/persistence.ts @@ -6,11 +6,11 @@ import { linkSync, openSync, readFileSync, - renameSync, unlinkSync, writeFileSync, } from "node:fs"; import { dirname, join } from "node:path"; +import { renameAtomicFile } from "../../lib/windows-atomic-replace"; import { ensureLabDirs, labAutomationPolicyPath, @@ -125,7 +125,7 @@ function atomicWriteJson(path: string, payload: unknown): void { const tmp = join(dir, `.${basename(path)}.${process.pid}.${Date.now()}.tmp`); const text = JSON.stringify(payload); writeFileSync(tmp, text, { encoding: "utf8", mode: 0o600 }); - renameSync(tmp, path); + renameAtomicFile(tmp, path); } function basename(path: string): string { diff --git a/src/lab/ledger/purge.ts b/src/lab/ledger/purge.ts index 4ec55709ab..ba2dd051fc 100644 --- a/src/lab/ledger/purge.ts +++ b/src/lab/ledger/purge.ts @@ -5,6 +5,7 @@ import { type TrustedArtifactDir, } from "../artifacts/secure-fs"; import { ArtifactFsError } from "../artifacts/secure-fs"; +import { renameAtomicFile } from "../../lib/windows-atomic-replace"; import { LAB_EVENT_SCHEMA_VERSION, LAB_PRODUCER, @@ -29,7 +30,6 @@ import { fsyncSync, openSync, readdirSync, - renameSync, rmSync, unlinkSync, writeSync, @@ -79,7 +79,7 @@ function atomicRewriteLedger(ledgerPath: string, events: LabEvent[]): void { } finally { closeSync(fd); } - renameSync(tmpPath, ledgerPath); + renameAtomicFile(tmpPath, ledgerPath); renamed = true; } catch (err) { if (!renamed) { diff --git a/src/lib/config-ownership.ts b/src/lib/config-ownership.ts index 27a8f823c3..2b62adacfb 100644 --- a/src/lib/config-ownership.ts +++ b/src/lib/config-ownership.ts @@ -5,7 +5,6 @@ import { readFileSync, readdirSync, realpathSync, - renameSync, rmdirSync, unlinkSync, writeFileSync, @@ -13,6 +12,7 @@ import { import { randomUUID } from "node:crypto"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; import type { GenerationContext } from "./state-store-sweeper"; +import { renameAtomicFile } from "./windows-atomic-replace"; export const CONFIG_OWNER_FILE = ".opencodex-owner.json"; export const CONFIG_UNINSTALL_MANIFEST = ".opencodex-uninstall.json"; @@ -232,7 +232,10 @@ function writeManifest(configDir: string, manifest: ConfigUninstallManifest): vo const temp = `${path}.${process.pid}.${randomUUID()}.tmp`; writeFileSync(temp, `${JSON.stringify(manifest, null, 2)}\n`, { encoding: "utf8", mode: 0o600 }); try { - renameSync(temp, path); + // Same Windows sharing-violation tolerance the config writer has: a + // scanner holding the manifest must not turn uninstall bookkeeping into a + // hard failure. + renameAtomicFile(temp, path); } catch (error) { try { unlinkSync(temp); } catch { /* best effort */ } throw error; diff --git a/src/lib/windows-atomic-replace.ts b/src/lib/windows-atomic-replace.ts new file mode 100644 index 0000000000..ab9b13a00a --- /dev/null +++ b/src/lib/windows-atomic-replace.ts @@ -0,0 +1,69 @@ +/** + * Windows-tolerant atomic replace. + * + * POSIX `rename()` replaces the destination entry unconditionally. Windows can + * refuse the same call with EBUSY, EPERM or EACCES while another process holds + * the target open — a real-time scanner that just indexed the file, a sync + * client, a backup agent. The hold is usually momentary, so a bounded retry + * turns an operational failure back into a successful publish. + * + * The envelope is deliberately small: two retries, 25ms then 50ms, about 75ms + * total. It is sized for a scanner blinking, not for a file someone actually + * has open. Widening it without evidence would trade a rare failure for a + * routine stall. + * + * This lives in its own module rather than in config.ts because + * config-ownership.ts is one of its callers and config.ts already imports + * config-ownership.ts — exporting it from there would close an import cycle. + */ + +import { renameSync } from "node:fs"; + +export interface AtomicRenameIO { + platform: NodeJS.Platform; + rename: (source: string, destination: string) => void; + sleep: (milliseconds: number) => void; +} + +const MAX_RETRIES = 2; + +/** Windows sharing violations only. Any other error is the caller's to see, immediately. */ +function isTransientWindowsReplaceError(platform: NodeJS.Platform, error: unknown): boolean { + if (platform !== "win32") return false; + const code = (error as NodeJS.ErrnoException).code; + return code === "EBUSY" || code === "EPERM" || code === "EACCES"; +} + +export function renameAtomicFile( + source: string, + destination: string, + io: AtomicRenameIO = { + platform: process.platform, + rename: renameSync, + sleep: Bun.sleepSync, + }, +): void { + for (let attempt = 0; ; attempt += 1) { + try { + io.rename(source, destination); + return; + } catch (error) { + if (!isTransientWindowsReplaceError(io.platform, error)) throw error; + if (attempt >= MAX_RETRIES) throw error; + io.sleep(25 * (attempt + 1)); + } + } +} + +export async function renameAtomicFileAsync(source: string, destination: string): Promise { + for (let attempt = 0; ; attempt += 1) { + try { + renameSync(source, destination); + return; + } catch (error) { + if (!isTransientWindowsReplaceError(process.platform, error)) throw error; + if (attempt >= MAX_RETRIES) throw error; + await Bun.sleep(25 * (attempt + 1)); + } + } +} diff --git a/src/storage/cleanup.ts b/src/storage/cleanup.ts index 82b1066554..dacfa98eb8 100644 --- a/src/storage/cleanup.ts +++ b/src/storage/cleanup.ts @@ -2435,7 +2435,7 @@ function writeRestorePending( throw new Error("test_fail_pending_rename"); } try { - renameSync(tmp, dest); + renameAtomicFile(tmp, dest); } catch (error) { try { unlinkSync(tmp); } catch { /* */ } throw error; diff --git a/src/tray/windows.ts b/src/tray/windows.ts index df30feff4b..cf62237491 100644 --- a/src/tray/windows.ts +++ b/src/tray/windows.ts @@ -1,6 +1,6 @@ import { execFile, execFileSync, spawn } from "node:child_process"; import { createHash } from "node:crypto"; -import { chmodSync, existsSync, mkdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { join, resolve } from "node:path"; import { expandUserPath, getConfigDir } from "../config"; @@ -8,6 +8,7 @@ import { durableBunRuntime } from "../lib/bun-runtime"; import type { BunRuntimeSource } from "../lib/bun-runtime"; import { forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; +import { renameAtomicFile } from "../lib/windows-atomic-replace"; const RUN_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion\\Run"; const RUN_PARENT_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion"; @@ -242,7 +243,7 @@ export function replaceWindowsTrayOwnedFile( const hardened = hardenSecretPath(target, { required: true, timeoutMemoKey: path }); if (!hardened.ok) throw new Error("Windows tray ACL hardening did not complete; refusing to persist executable state."); }, - rename: renameSync, + rename: renameAtomicFile, unlink: unlinkSync, }, ): void { diff --git a/tests/windows-atomic-replace.test.ts b/tests/windows-atomic-replace.test.ts new file mode 100644 index 0000000000..8e7e6c4712 --- /dev/null +++ b/tests/windows-atomic-replace.test.ts @@ -0,0 +1,82 @@ +/** + * The Windows-tolerant atomic replace. + * + * Windows can refuse rename with EBUSY/EPERM/EACCES while another process holds + * the target — a scanner that just indexed the file, a sync client, a backup + * agent. The hold is usually momentary, so a small bounded retry turns an + * operational failure back into a successful publish. + * + * These cases pin the envelope itself: which errors are transient, which + * platform retries at all, and that the retry count is bounded rather than + * hopeful. The envelope is deliberately small (two retries, ~75ms), so an + * accidental widening should fail here. + */ +import { describe, expect, test } from "bun:test"; + +import { renameAtomicFile, type AtomicRenameIO } from "../src/lib/windows-atomic-replace"; + +/** A rename that fails `failures` times with `code`, then succeeds. */ +function io(failures: number, code = "EBUSY", platform: NodeJS.Platform = "win32") { + const sleeps: number[] = []; + let attempts = 0; + const seam: AtomicRenameIO & { sleeps: number[]; attempts: () => number } = { + platform, + rename: () => { + attempts += 1; + if (attempts <= failures) { + const error = new Error(code) as NodeJS.ErrnoException; + error.code = code; + throw error; + } + }, + sleep: (ms: number) => { sleeps.push(ms); }, + sleeps, + attempts: () => attempts, + }; + return seam; +} + +describe("renameAtomicFile", () => { + test("a clean replace does not sleep", () => { + const seam = io(0); + renameAtomicFile("a", "b", seam); + expect(seam.attempts()).toBe(1); + expect(seam.sleeps).toEqual([]); + }); + + test("a transient sharing violation is retried and then succeeds", () => { + const seam = io(1); + renameAtomicFile("a", "b", seam); + expect(seam.attempts()).toBe(2); + expect(seam.sleeps).toEqual([25]); + }); + + test("the envelope is two retries with a rising backoff, then it gives up", () => { + const seam = io(99); + expect(() => renameAtomicFile("a", "b", seam)).toThrow("EBUSY"); + // Three attempts total: the original plus two retries. + expect(seam.attempts()).toBe(3); + expect(seam.sleeps).toEqual([25, 50]); + }); + + test.each(["EBUSY", "EPERM", "EACCES"])("%s is treated as transient on Windows", code => { + const seam = io(1, code); + renameAtomicFile("a", "b", seam); + expect(seam.attempts()).toBe(2); + }); + + test("any other error surfaces immediately", () => { + const seam = io(99, "ENOENT"); + expect(() => renameAtomicFile("a", "b", seam)).toThrow("ENOENT"); + expect(seam.attempts()).toBe(1); + expect(seam.sleeps).toEqual([]); + }); + + test("POSIX never retries, even on a code Windows would tolerate", () => { + // rename(2) replaces unconditionally; a sharing-violation retry there would + // only paper over a real error. + const seam = io(99, "EBUSY", "linux"); + expect(() => renameAtomicFile("a", "b", seam)).toThrow("EBUSY"); + expect(seam.attempts()).toBe(1); + }); +});