Repository navigation
fix(desktop): retry a settings write that loses the rename to a Windows lock - #54530
waterundman wants to merge 2 commits into
Conversation
…ws 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 anomalyco#51470
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I couldn't verify the fix: the lock it handles only happens on Windows, and the two bun:ffi regression tests are skipped on Linux. bun test src/main passes in packages/desktop, but bun run typecheck fails on the new helper (see the inline comment), so that needs fixing before this can land.
The rest looks sound. The parent folder is still created before writing. The retry waits with Atomics.wait instead of spinning. Errors that waiting can't fix still throw. The .tmp file is removed on every path. If the write-through fallback is interrupted, store.ts and readWindowState already move an unreadable file aside, so a truncated file won't block startup.
Public API: unchanged. File names and formats stay the same, and nothing outside packages/desktop/src/main is touched.
Related: #45154
There was a problem hiding this comment.
This breaks bun run typecheck in packages/desktop: atomic-write.ts(48,32): Type 'unknown' is not assignable to type 'Error | undefined'. isLockedError returns a plain boolean, so cause is still unknown when retryWhileLocked returns it. Making this a type guard (e.g. cause is NodeJS.ErrnoException) should fix it.
There was a problem hiding this comment.
Good catch - reproduced standalone before touching anything: tsc 5.9.3 --strict on this file flags the same error at the same position (48,32), so the local sandbox was blind to it (no tsconfig there).
Fixed in 2d9b8f9 by making isLockedError a cause is NodeJS.ErrnoException guard: the catch variable is narrowed past the early throw, so the loop can return it as an Error. Tests (13 pass), oxlint with the repo's anti-slop plugin, and prettier are unchanged.
`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.
This branch has not been deployed
1 participant
Issue for this PR
Closes #51470
Type of change
What does this PR do?
Two main-process writers replace a JSON file with
writeFileSync(file.tmp)+renameSync(file.tmp, file). On Windows that rename fails withEPERMwhenever any other handle has the target open, so an antivirus scanner or indexer readingAppData\Roamingis enough to lose the write, and V2 does not retry it.Both now call one
writeFileAtomichelper that retries the rename four times (15/30/45 ms) while the error is EPERM/EACCES/EBUSY, and if the target is still locked writes through the open handle instead of dropping the change. That second path works whenever the holder grantedFILE_SHARE_WRITE, which is exactly the scanner pattern. Either way the.tmpfile is removed, so a failed write leaves nothing behind.Contents, file format and the synchronous API are unchanged.
store.tsstays a file on purpose: it is read before the storage layer exists, so it cannot be SQLite.There is no atomic-write helper in the tree to reuse —
renameSyncappears in four files, two of them a vite config and a TUI test — andpackages/desktophas no fs dependency beyondnode:fs.How did you verify your code works?
cd packages/desktop && bun test src/main— 13 pass, 5 of them in the newatomic-write.test.ts.The regression test holds the target open with
FILE_SHARE_READ|FILE_SHARE_WRITEthroughbun:ffi, becausenode:fscannot produce this failure: it grants every share bit, so twonode:fshandles never contend and the rename always wins. Run against the pre-PR body of the helper, that test fails with the error from the issue and leaves the.tmpfile behind; I ran it both ways to confirm. Those two lock tests areskipIf-guarded, so a Linux or macOS run reports them as skipped and the other three still run.I also measured the lock directly on Windows 11 with
CreateFileWand all eightdwShareModevalues:dwShareModeTwo things follow: any open handle breaks the rename, and the fallback only helps holders that grant
FILE_SHARE_WRITE. WithdwShareMode: 0the helper gives up after ~90 ms, keeps the old contents and cleans up the.tmp.oxlintreports no new warnings andprettier --checkpasses on the four files.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #45154