fix(windows): wait out a reader when replacing a preference file

The Windows CI leg failed on the cross-process test with EPERM from
renameSync. It is not a test artifact. Windows refuses a rename over a
file another process holds open rather than waiting, with
ERROR_ACCESS_DENIED or ERROR_SHARING_VIOLATION, and reads of these two
files are deliberately lock-free on both sides. So any tray or app read
that overlaps a writer's replace fails that write, and since this branch
made writers report a failure instead of swallowing it, the outcome is
exactly the lost update the lock exists to prevent, reached by another
route. The test's reader thread only made it visible by reading the way
both apps really do, continuously and without the lock.

The replace is now retried on that error class, ten attempts about 20ms
apart, on both sides: replace_file in settings.rs, used by settings patch
and by the dock's write_state_at, and renameOnto behind writeFileAtomic
in tray-settings.ts. The lock is already held at that point, so the wait
can only ever be on a reader, never on another writer, and a read of one
of these files is microseconds of open file. Once the budget is out the
original error is returned unchanged and the caller cleans up as before.
On POSIX, where a rename over an open file cannot fail this way, it
compiles out in Rust and short-circuits on process.platform in Node.

Reads stay lock-free, which is the right trade and the reason the files
can be read at all without coordination; both read paths now say what it
costs and where that cost is absorbed. The reader thread stays as it is,
and is now the regression test for the retry as well: it is what puts a
replace under a continuous reader.

The retry itself is covered on the Node side by faking the platform and
the rename, since a rename that fails this way cannot be produced on a
Mac: a reader waited out and the write landing, the budget running out
with the old file still whole, and an error a reader cannot cause not
being waited out at all. The Rust half is compiled out off Windows, so
CI is its only proof.

app/electron/tray-settings-atomic.test.ts

@@ -96,6 +96,47 @@ describe('writing a file the tray app reads', () => {
     expect(readdirSync(dirname(dockPath()))).toEqual(['windows-dock.json'])
   })
 
+  // Windows refuses to rename over a file another process has open, so a lock-free read on
+  // either side can fail a write on the other. The retry is Windows-only and this machine is
+  // not Windows, so the platform is what is faked here; the real API behaviour is CI's to prove.
+  describe('on Windows, where a rename over an open file is refused', () => {
+    const real = process.platform
+    const failWith = (code: string) => Object.assign(new Error(code), { code })
+    beforeEach(() => { Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) })
+    afterEach(() => { Object.defineProperty(process, 'platform', { value: real, configurable: true }) })
+
+    it('waits out a reader and lands the write', () => {
+      let refusals = 2
+      renameMock.mockImplementation((...args: Parameters<typeof actualFs.renameSync>) => {
+        if (refusals-- > 0) throw failWith('EPERM')
+        return actualFs.renameSync(...args)
+      })
+
+      writeFileAtomic(dockPath(), 'landed')
+
+      expect(renameMock).toHaveBeenCalledTimes(3)
+      expect(readFileSync(dockPath(), 'utf8')).toBe('landed')
+    })
+
+    it('gives up with the same error once the budget is out, leaving nothing behind', () => {
+      writeFileSync(dockPath(), 'old')
+      renameMock.mockImplementation(() => { throw failWith('EACCES') })
+
+      expect(() => writeFileAtomic(dockPath(), 'new')).toThrow('EACCES')
+
+      expect(renameMock).toHaveBeenCalledTimes(10)
+      expect(readFileSync(dockPath(), 'utf8')).toBe('old')
+      expect(readdirSync(dirname(dockPath()))).toEqual(['windows-dock.json'])
+    })
+
+    it('does not wait out an error a reader cannot cause', () => {
+      renameMock.mockImplementation(() => { throw failWith('ENOSPC') })
+
+      expect(() => writeFileAtomic(dockPath(), 'new')).toThrow('ENOSPC')
+      expect(renameMock).toHaveBeenCalledTimes(1)
+    })
+  })
+
   it('reports a failed write rather than letting it read as a save', () => {
     writeMock.mockImplementation(() => { throw new Error('ENOSPC') })
 

app/electron/tray-settings.ts

@@ -46,13 +46,51 @@ export function writeFileAtomic(path: string, contents: string): void {
   const temp = `${path}.${process.pid}.${Date.now()}.tmp`
   try {
     writeFileSync(temp, contents)
-    renameSync(temp, path)
+    renameOnto(temp, path)
   } catch (err) {
     try { unlinkSync(temp) } catch { /* nothing to clean up */ }
     throw err
   }
 }
 
+/** How long a replace may spend waiting out a reader before the write is reported as failed:
+ *  ten tries about 20ms apart, so roughly 200ms in the worst case. A read of one of these files
+ *  is a few microseconds of open file, so anything near this budget is not a reader at all. */
+const RENAME_ATTEMPTS = 10
+const RENAME_RETRY_MS = 20
+
+/**
+ * The rename half of the write, retried briefly on a Windows sharing violation.
+ *
+ * Readers of these files take no lock, on this side or the tray app's, and on POSIX that costs
+ * nothing: a rename over a file somebody has open always succeeds, and the reader keeps reading
+ * the file it opened. Windows refuses instead, with EPERM or EACCES, whenever something else
+ * holds the target open without having agreed to its deletion, and a scanner that opened the
+ * file behind our back does it just as well as one of our own processes. That is transient by
+ * nature, so it is waited out rather than reported as a write that did not land, which would be
+ * the very lost update the lock around this exists to prevent. The lock is already held here, so
+ * the wait can only ever be on a reader, never on another writer.
+ *
+ * Once the budget is out the original error is thrown, unchanged, and the caller cleans up.
+ */
+function renameOnto(temp: string, path: string): void {
+  for (let attempt = 1; ; attempt++) {
+    try {
+      renameSync(temp, path)
+      return
+    } catch (err) {
+      const code = (err as NodeJS.ErrnoException).code
+      const sharingViolation = code === 'EPERM' || code === 'EACCES' || code === 'EBUSY'
+      if (process.platform !== 'win32' || !sharingViolation || attempt >= RENAME_ATTEMPTS) throw err
+      sleepSync(RENAME_RETRY_MS)
+    }
+  }
+}
+
+/** Lock-free by design: the writer renames a whole file into place, so this sees the old file
+ *  or the new one and never a torn one. What it costs is paid on the writer's side, where a read
+ *  overlapping a replace is a failure Windows reports rather than waits out; see
+ *  {@link writeFileAtomic}. */
 export function readTrayFile(file: TraySettingsFile, home = homedir()): Record<string, unknown> {
   try {
     const parsed: unknown = JSON.parse(readFileSync(filePath(file, home), 'utf8').replace(/^/, ''))

windows/src-tauri/src/dock.rs

@@ -522,6 +522,10 @@ fn read_state() -> serde_json::Map<String, serde_json::Value> {
     read_state_at(&state_path())
 }
 
+/// Lock-free by design: the writer renames a whole file into place, so this sees the old file or
+/// the new one and never a torn one. What it costs is paid on the writer's side, where a read
+/// overlapping a replace is a failure Windows reports rather than waits out; see
+/// `crate::settings::replace_file`.
 fn read_state_at(path: &Path) -> serde_json::Map<String, serde_json::Value> {
     fs::read(path)
         .ok()
@@ -549,7 +553,9 @@ fn write_state_at(path: &Path, state: &serde_json::Map<String, serde_json::Value
         WRITE_SEQUENCE.fetch_add(1, Ordering::Relaxed)
     ));
     fs::write(&temp, &bytes)?;
-    if let Err(err) = fs::rename(&temp, path) {
+    // Not a plain rename: on Windows a reader that has the target open can make the replace fail
+    // outright, and reads of this file take no lock. See `crate::settings::replace_file`.
+    if let Err(err) = crate::settings::replace_file(&temp, path) {
         // Nothing is left behind for the next launch to trip over.
         let _ = fs::remove_file(&temp);
         return Err(err.into());
@@ -2101,6 +2107,11 @@ mod tests {
         // the file as it really is, and the property under test is exactly what it watches for:
         // with the lock both counters only ever rise in the file, and a counter that falls is a
         // write that was erased, wherever in the run it happened.
+        //
+        // It reads the way both apps really do, lock-free and continuously, which on Windows is
+        // also what puts `crate::settings::replace_file`'s retry under load: this test is where
+        // a replace that gives up on a reader shows itself, as a writer reporting a failed
+        // write. Keep the loop tight for that reason as much as for the detection.
         let stop = Arc::new(AtomicBool::new(false));
         let watcher = {
             let (dock_file, stop) = (dock_file.clone(), stop.clone());

windows/src-tauri/src/settings.rs

@@ -22,7 +22,7 @@
 
 use std::collections::BTreeMap;
 use std::fs;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::Mutex;
 
 use anyhow::{anyhow, Context, Result};
@@ -77,7 +77,10 @@ static PENDING_SECTION: Mutex<Option<String>> = Mutex::new(None);
 //               (a successor's), which is left alone.
 //
 // Reads take no lock: every writer renames a whole file into place, so a reader sees the old
-// file or the new one, never a torn one (the same reason config::read is lock-free).
+// file or the new one, never a torn one (the same reason config::read is lock-free). That is
+// still the right trade, but it is not free on Windows: a rename over a file some other process
+// has open can fail outright there rather than wait, so a read on either side can make a write
+// on the other fail. `replace_file` is where that is absorbed.
 //
 // Lessons borrowed from src/cache-refresh-lock.ts: exclusive create as the arbiter, a pid +
 // timestamp record, a staleness window so a dead holder cannot wedge the file forever, and a
@@ -411,6 +414,40 @@ pub(crate) mod prefs_lock {
     }
 }
 
+/// Rename `temp` over `target`, retrying briefly on a Windows sharing violation.
+///
+/// The rename is what makes a write atomic to a reader, and readers of these files take no lock
+/// on either side. On POSIX that costs nothing: a rename over an open file always succeeds, and
+/// whoever had it open keeps reading the file they opened. Windows refuses instead, with
+/// ERROR_ACCESS_DENIED or ERROR_SHARING_VIOLATION, whenever something else holds the target open
+/// without having agreed to its deletion, and a scanner opening the file behind our back does it
+/// just as well as one of our own processes. That failure is transient by nature, so it is
+/// retried for a short bounded time rather than reported as a write that did not land, which is
+/// the very lost update the lock around this exists to prevent.
+///
+/// The lock is already held by the time this runs, so the wait can only ever be on a reader,
+/// never on another writer, and readers hold the file for microseconds. Once the budget is out
+/// the error is returned unchanged. On POSIX this is a plain rename.
+pub(crate) fn replace_file(temp: &Path, target: &Path) -> std::io::Result<()> {
+    #[cfg(windows)]
+    {
+        const RETRIES: u32 = 9;
+        const PAUSE: std::time::Duration = std::time::Duration::from_millis(20);
+        // ERROR_ACCESS_DENIED and ERROR_SHARING_VIOLATION, as raw OS codes: the io::ErrorKind
+        // they map to is not stable enough to match on.
+        const SHARING: [i32; 2] = [5, 32];
+        for _ in 0..RETRIES {
+            match fs::rename(temp, target) {
+                Err(err) if err.raw_os_error().is_some_and(|code| SHARING.contains(&code)) => {
+                    std::thread::sleep(PAUSE);
+                }
+                result => return result,
+            }
+        }
+    }
+    fs::rename(temp, target)
+}
+
 // Preferences ---------------------------------------------------------------------------
 
 fn settings_path() -> PathBuf {
@@ -449,7 +486,7 @@ pub fn patch(values: Map<String, Value>) -> Result<Map<String, Value>> {
     }
     let tmp = path.with_extension("tmp");
     fs::write(&tmp, serde_json::to_vec_pretty(&stored)?)?;
-    fs::rename(&tmp, &path)?;
+    replace_file(&tmp, &path)?;
     Ok(stored)
 }