Skip to content

fix(desktop): retry store writes on Windows EPERM - #51474

Closed
waterundman wants to merge 1 commit into
anomalyco:devfrom
waterundman:store-retry
Closed

waterundman wants to merge 1 commit into
anomalyco:devfrom
waterundman:store-retry

Conversation

@waterundman

Copy link
Copy Markdown

Issue for this PR

Closes #51470

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

store-set, store-delete and store-clear called electron-store directly, so when the final rename hit a transient Windows sharing violation (EPERM/EBUSY/EACCES) the error was thrown straight back into the renderer and the write was silently lost — the store kept its old contents and the change disappeared on the next reload. Log line in #51470.

The three handlers now go through mutateStore (store-write.ts):

  • It retries the mutation up to 3 times with a short backoff. conf/atomically already retry the rename itself for 1s, but that is a hot sync loop that blocks the main process; the extra attempts are awaited with a timer in between, so the event loop gets a chance to run between them.
  • If the write still fails, it applies the same mutation to the data read from disk and writes the file in place with writeFileSync. That is not atomic, but a tiny JSON file that survives a crash mid-write is the better trade against dropping the user's change and keeping the stale file. If even that fails, it logs and resolves instead of rejecting into the renderer.
  • Mutations of the same store file are serialized per name, so two concurrent IPC writes cannot race each other's rename, and the fire-and-forget removeStoreFileIfEmpty after delete/clear now runs through the same queue instead of reading/deleting the file while a write is renaming onto it.

Errors that are not transient still propagate to the renderer exactly as before.

How did you verify your code works?

  • bun test in packages/desktop: added store-write.test.ts with 5 tests (retry until it lands, in-place fallback, non-transient errors rejected, per-file serialization, unrelated stores not blocked) — all pass; the two pre-existing failures in that package (draft-store, wsl/servers) fail identically on a clean checkout here because of this machine's environment (node:sqlite missing, Electron binary not installed).
  • bun run typecheck in packages/desktop passes.
  • bun run lint: no findings on the changed files.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@waterundman

Copy link
Copy Markdown
Author

One thing worth flagging for reviewers: store-delete/store-clear used to kick off removeStoreFileIfEmpty(name) fire-and-forget, so that readFile/rm could overlap a later store-set renaming onto the same file — the same class of race as #45154. In this PR the cleanup runs through the same per-file queue as the mutations, which fixes the write-path half of it. The startup cleanupStoreFiles() scan still runs independently, so I don't think this closes #45154 on its own.

@waterundman

Copy link
Copy Markdown
Author

Two things that might be keeping this out of the queue, plus one scope question.

  1. No CI has run. There are 0 workflow runs for fe1908b4 — as a first-time contributor to this repo, test.yml likely needs an approval click from a maintainer before it will start. If that's the gate, the PR isn't waiting on anything from me.
  2. The standards checks passed (no needs:title / needs:issue / needs:compliance), and the branch is mergeable against dev; mergeable_state is blocked on required review only.

Scope question: the mutation-path half of this race is the same class as #45154 (desktop scoped-store cleanup can race later writes), which is assigned to you — and #47497 covered the CLI half before it closed. Would you rather have this as a standalone fix for the store mutation path, or folded into #45154's cleanup work? Happy either way; I just don't want to land a second fix for the same race.

For the record on timing: I know older PRs here are auto-closed at one month without 2 positive reactions. Not fishing for reactions — just noting that if the shape is wrong, closing it is cheaper for everyone than letting it time out.

@thdxr

thdxr commented Oct 10, 2026

Copy link
Copy Markdown
Member

This branch has been upgraded to V2. We're no longer taking PRs for V1. If you think this is still relevant, please port it over and open it for V2.

— from 𝕺𝖕𝖊𝖓𝕮𝖔𝖉𝖊

@thdxr thdxr closed this Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: desktop settings and window-state writes can fail with EPERM on rename, silently dropping persisted state

2 participants