From 725716637dfbce421bc42bca0814583bdeaae605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=A1=95=E6=A0=91=E5=9C=B0=E8=A1=97?= <120729380+waterundman@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:45:31 +0800 Subject: [PATCH 1/2] fix(desktop): retry a settings write that loses the rename to a Windows lock `store.ts` and `window-state.ts` replace a JSON file by writing a temporary file and renaming it over the target. Windows refuses that rename while any other handle holds the target open, which antivirus scanners and search indexers do while they read AppData\Roaming, and V2 does not retry the rename at all. Both call sites now share a writeFileAtomic helper: retry the rename while the failure is a sharing violation, fall back to writing through the open handle if the target stays locked, and remove the temporary file either way. Closes #51470 --- .../src/main/storage/atomic-write.test.ts | 105 ++++++++++++++++++ .../desktop/src/main/storage/atomic-write.ts | 90 +++++++++++++++ packages/desktop/src/main/storage/store.ts | 9 +- .../desktop/src/main/windows/window-state.ts | 9 +- 4 files changed, 202 insertions(+), 11 deletions(-) create mode 100644 packages/desktop/src/main/storage/atomic-write.test.ts create mode 100644 packages/desktop/src/main/storage/atomic-write.ts diff --git a/packages/desktop/src/main/storage/atomic-write.test.ts b/packages/desktop/src/main/storage/atomic-write.test.ts new file mode 100644 index 000000000000..3032654814d6 --- /dev/null +++ b/packages/desktop/src/main/storage/atomic-write.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, test } from "bun:test" +import { dlopen, FFIType, ptr } from "bun:ffi" +import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import path from "node:path" +import { writeFileAtomic } from "./atomic-write" + +const dir = () => mkdtempSync(path.join(tmpdir(), "opencode-atomic-write-")) + +const writes = [ + { name: "a new file", existing: undefined }, + { name: "an existing file", existing: '{"windowIds":["a"]}' }, +] + +describe("writeFileAtomic", () => { + for (const item of writes) { + test(`writes ${item.name}, creating its directory, without leaving a temporary behind`, () => { + const file = path.join(dir(), "nested", "opencode.settings") + + if (item.existing !== undefined) { + mkdirSync(path.dirname(file), { recursive: true }) + writeFileSync(file, item.existing) + } + + writeFileAtomic(file, '{"pinchZoomEnabled":true}') + + expect(readFileSync(file, "utf8")).toBe('{"pinchZoomEnabled":true}') + expect(existsSync(`${file}.tmp`)).toBe(false) + }) + } + + // A write that cannot land must still clean up after itself, so a reader never mistakes a + // staging file for state. + test("removes the temporary file when the target cannot be replaced", () => { + const target = path.join(dir(), "blocked") + mkdirSync(target) + + expect(() => writeFileAtomic(target, "{}")).toThrow() + expect(existsSync(`${target}.tmp`)).toBe(false) + }) +}) + +// The failure this module fixes only exists on Windows, and node:fs cannot produce it: every handle +// it opens grants FILE_SHARE_READ|WRITE|DELETE, so two node:fs handles never contend and a rename +// always wins. Asking for a specific dwShareMode needs Win32, and bun:ffi is the only way to reach +// it without a native build or a test-only seam in the module under test. +function holdOpen(file: string, mode: "read+write" | "exclusive") { + const GENERIC_READ = 0x80000000 + const FILE_SHARE_READ = 0x00000001 + const FILE_SHARE_WRITE = 0x00000002 + const OPEN_EXISTING = 3 + const share = mode === "read+write" ? FILE_SHARE_READ | FILE_SHARE_WRITE : 0 + + const kernel32 = dlopen("kernel32.dll", { + CreateFileW: { + args: [FFIType.ptr, FFIType.u32, FFIType.u32, FFIType.ptr, FFIType.u32, FFIType.u32, FFIType.ptr], + returns: FFIType.ptr, + }, + CloseHandle: { args: [FFIType.ptr], returns: FFIType.i32 }, + }) + + const wide = Buffer.from(`${file}\0`, "utf16le") + const handle = kernel32.symbols.CreateFileW(ptr(wide), GENERIC_READ, share, null, OPEN_EXISTING, 0, null) + + if (handle === null) throw new Error(`could not open a handle on ${file}`) + + return { handle, release: () => kernel32.symbols.CloseHandle(handle) } +} + +const windowsTest = test.skipIf(process.platform !== "win32") + +// An antivirus scanner or search indexer reading the file under AppData\Roaming is what produced +// the reported EPERM: it grants FILE_SHARE_WRITE, which permits writing through the open handle but +// not replacing the file. +windowsTest("writes while another handle holds the target open with FILE_SHARE_READ|FILE_SHARE_WRITE", () => { + const file = path.join(dir(), "opencode.settings") + writeFileSync(file, '{"ready":false}') + const holder = holdOpen(file, "read+write") + + try { + writeFileAtomic(file, '{"ready":true}') + } finally { + holder.release() + } + + expect(readFileSync(file, "utf8")).toBe('{"ready":true}') + expect(existsSync(`${file}.tmp`)).toBe(false) +}) + +// A holder that shares nothing blocks the replacement and the write-through alike, so the write has +// to fail loudly. What it must not do is leave the staging file behind or damage the old contents. +windowsTest("reports the lock and keeps the old contents when nothing can be written", () => { + const file = path.join(dir(), "opencode.settings") + writeFileSync(file, '{"ready":false}') + const holder = holdOpen(file, "exclusive") + + try { + expect(() => writeFileAtomic(file, '{"ready":true}')).toThrow() + } finally { + holder.release() + } + + expect(readFileSync(file, "utf8")).toBe('{"ready":false}') + expect(existsSync(`${file}.tmp`)).toBe(false) +}) diff --git a/packages/desktop/src/main/storage/atomic-write.ts b/packages/desktop/src/main/storage/atomic-write.ts new file mode 100644 index 000000000000..8d6001bd22e8 --- /dev/null +++ b/packages/desktop/src/main/storage/atomic-write.ts @@ -0,0 +1,90 @@ +import { mkdirSync, renameSync, rmSync, writeFileSync } from "node:fs" +import path from "node:path" + +// Replacing a file is not as safe as it looks on Windows. `rename` over an existing file fails with +// EPERM/EBUSY while *any* other handle holds that file open, even one that granted every share bit +// in `dwShareMode`. Antivirus, search indexers and backup agents open files under AppData\Roaming +// routinely while they scan, so the failure is transient, real, and reported against this code +// path: https://github.com/anomalyco/opencode/issues/51470 +// +// The lock clears on its own, so retry before concluding the write cannot land. +const attempts = 4 + +const retryDelayMs = 15 + +// A sharing violation is the one failure a later attempt can survive; nothing else clears by +// waiting. The three codes are what Node reports for it, depending on how the holder opened the +// file: EPERM for a denied replacement, EBUSY for a denied open. +function isLockedError(cause: unknown) { + if (!(cause instanceof Error) || !("code" in cause)) return false + + return cause.code === "EPERM" || cause.code === "EACCES" || cause.code === "EBUSY" +} + +// The settings store is read before the storage layer exists and window state is written as the +// window closes, so both callers are synchronous by design. Atomics.wait blocks without spinning, +// and the buffer is per call, so nothing is shared between calls. +function waitForLockToClear(ms: number) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) +} + +/** + * Runs an operation, retrying while the failure keeps looking like a transient Windows lock. + * Returns the lock error once the retries run out, and throws anything that waiting cannot fix. + */ +function retryWhileLocked(operation: () => void): Error | undefined { + let attempt = 0 + + while (true) { + try { + operation() + + return undefined + } catch (cause) { + if (!isLockedError(cause)) throw cause + + attempt += 1 + + if (attempt >= attempts) return cause + + waitForLockToClear(retryDelayMs * attempt) + } + } +} + +/** + * Replaces a file without ever exposing a half-written one. The contents are written beside the + * target and renamed over it, retrying the rename while another process holds the target open. + */ +export function writeFileAtomic(file: string, contents: string) { + const tmp = `${file}.tmp` + + mkdirSync(path.dirname(file), { recursive: true }) + writeFileSync(tmp, contents) + + let locked: Error | undefined + + try { + locked = retryWhileLocked(() => renameSync(tmp, file)) + } catch (cause) { + rmSync(tmp, { force: true }) + + throw cause + } + + if (locked === undefined) return + + // The target stayed locked past the retries. A replacement needs the file closed, but writing + // through the open handle only needs FILE_SHARE_WRITE, which the scanners that cause this grant. + // Keeping the update beats dropping it silently, and a write interrupted by a crash is set aside + // by the reader instead of blocking startup. + try { + writeFileSync(file, contents) + } catch { + rmSync(tmp, { force: true }) + + throw locked + } + + rmSync(tmp, { force: true }) +} diff --git a/packages/desktop/src/main/storage/store.ts b/packages/desktop/src/main/storage/store.ts index 85e80e8c502b..255f01801e73 100644 --- a/packages/desktop/src/main/storage/store.ts +++ b/packages/desktop/src/main/storage/store.ts @@ -1,7 +1,8 @@ -import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs" +import { existsSync, readFileSync, renameSync } from "node:fs" import path from "node:path" import electron from "electron" +import { writeFileAtomic } from "./atomic-write" import { SETTINGS_STORE } from "./keys" const cache = new Map() @@ -32,11 +33,7 @@ export function getStore(name = SETTINGS_STORE) { export function createSettingsStore(file: string) { let data = read(file) - const write = () => { - mkdirSync(path.dirname(file), { recursive: true }) - writeFileSync(`${file}.tmp`, JSON.stringify(data, null, "\t")) - renameSync(`${file}.tmp`, file) - } + const write = () => writeFileAtomic(file, JSON.stringify(data, null, "\t")) return { path: file, diff --git a/packages/desktop/src/main/windows/window-state.ts b/packages/desktop/src/main/windows/window-state.ts index 63bc3d6a7f9b..87333fbe69f9 100644 --- a/packages/desktop/src/main/windows/window-state.ts +++ b/packages/desktop/src/main/windows/window-state.ts @@ -1,7 +1,8 @@ -import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs" -import path from "node:path" +import { existsSync, readFileSync } from "node:fs" import type { BrowserWindow, Rectangle } from "electron" +import { writeFileAtomic } from "../storage/atomic-write" + // Remembers a window's bounds and maximized / fullscreen flags across launches in the same JSON // file electron-window-state wrote, so existing profiles restore where they were. @@ -100,9 +101,7 @@ export function manageWindowState(win: BrowserWindow, file: string, initial: Win win.off("move", changed) win.off("close", update) win.off("closed", closed) - mkdirSync(path.dirname(file), { recursive: true }) - writeFileSync(`${file}.tmp`, JSON.stringify(state)) - renameSync(`${file}.tmp`, file) + writeFileAtomic(file, JSON.stringify(state)) } win.on("resize", changed) From 2d9b8f9d497a4a97152d714fa5a2edafe05cefca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=A1=95=E6=A0=91=E5=9C=B0=E8=A1=97?= <120729380+waterundman@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:27:03 +0800 Subject: [PATCH 2/2] fix(desktop): narrow the lock check into a type guard `atomic-write.ts(48,32): Type 'unknown' is not assignable to type 'Error | undefined'` under `bun run typecheck` in packages/desktop: the retry loop returns the caught error after the retries run out, but a plain boolean helper does not narrow the catch variable. Making isLockedError a `cause is NodeJS.ErrnoException` guard narrows the caught value past the early throw. Tests, lint and formatting are unchanged. --- packages/desktop/src/main/storage/atomic-write.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/desktop/src/main/storage/atomic-write.ts b/packages/desktop/src/main/storage/atomic-write.ts index 8d6001bd22e8..776c8434c4d0 100644 --- a/packages/desktop/src/main/storage/atomic-write.ts +++ b/packages/desktop/src/main/storage/atomic-write.ts @@ -15,7 +15,7 @@ const retryDelayMs = 15 // A sharing violation is the one failure a later attempt can survive; nothing else clears by // waiting. The three codes are what Node reports for it, depending on how the holder opened the // file: EPERM for a denied replacement, EBUSY for a denied open. -function isLockedError(cause: unknown) { +function isLockedError(cause: unknown): cause is NodeJS.ErrnoException { if (!(cause instanceof Error) || !("code" in cause)) return false return cause.code === "EPERM" || cause.code === "EACCES" || cause.code === "EBUSY"