diff --git a/.github/workflows/windows-menubar-ci.yml b/.github/workflows/windows-menubar-ci.yml index 9a2e0298..3606de40 100644 --- a/.github/workflows/windows-menubar-ci.yml +++ b/.github/workflows/windows-menubar-ci.yml @@ -63,6 +63,15 @@ jobs: working-directory: windows run: npm ci + # The Rust suite's cross-process lock test spawns a real Node process beside the + # Rust one, so both hammer the same settings file (dock.rs, + # `concurrent_node_and_rust_patches_to_the_dock_file_both_survive`). The worker is + # TypeScript run through tsx, which lives in the repo root's devDependencies and is + # not installed by `npm ci` inside windows/. Without it the worker exits 1 and the + # test fails, which is what it did rather than skipping. + - name: Install the repo-root tools the Rust tests spawn + run: npm ci --no-audit --no-fund + - name: Typecheck frontend working-directory: windows run: npx tsc --noEmit diff --git a/app/electron/menubar.ts b/app/electron/menubar.ts index 42d8705d..96719cae 100644 --- a/app/electron/menubar.ts +++ b/app/electron/menubar.ts @@ -312,16 +312,13 @@ export function readDockEnabled(home = homedir()): boolean | undefined { } /** Read, change one key, write back. The tray app owns every other key in this file (the - * rail's placement, size and provider set), so the whole object is preserved. */ + * rail's placement, size and provider set), so the whole object is preserved. The cycle goes + * through {@link patchTrayFile} rather than being done here, because the tray app writes this + * same file: a read/modify/write outside the cross-process lock erases whatever the tray app + * stored between this side's read and its rename, which is the rail's placement more often + * than not. */ export function writeDockEnabled(enabled: boolean, home = homedir()): void { - const path = dockPrefsPath(home) - let prefs: Record = {} - try { - const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) - if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) prefs = parsed as Record - } catch { /* a missing or unreadable file simply starts from nothing */ } - prefs.enabled = enabled - writeFileAtomic(path, `${JSON.stringify(prefs, null, 2)}\n`) + patchTrayFile('dock', { enabled }, home) } // Talking to the tray app ---------------------------------------------------------------------- diff --git a/app/electron/quota/codex-refresh.test.ts b/app/electron/quota/codex-refresh.test.ts index 5e8a7615..565f102a 100644 --- a/app/electron/quota/codex-refresh.test.ts +++ b/app/electron/quota/codex-refresh.test.ts @@ -222,6 +222,131 @@ describe('Codex quota credential rotation', () => { expect((await readAuth()).tokens.refresh_token).toBe(NEW_REFRESH) }) + it('discards a rotation when the login on disk changes while the grant is in flight', async () => { + await writeAuth(authDoc()) + const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // The user ran `codex login` and switched accounts while our grant was open. + await reachedGrant + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { + access_token: 'access-other', + refresh_token: 'refresh-other-account', + id_token: 'id-other', + account_id: 'account-2', + }, + last_refresh: new Date(NOW).toISOString(), + }) + releaseGrant() + await pending + + // The new account's document is left exactly as the CLI wrote it: our grant + // spent account-1's refresh token, so writing it back here would splice + // account-1's token onto account-2's identity. + const saved = await readAuth() + expect(saved.tokens.account_id).toBe('account-2') + expect(saved.tokens.refresh_token).toBe('refresh-other-account') + expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH) + expect(errors).toHaveBeenCalledTimes(1) + }) + + // The id_token subject is the last of the three lineage fields and the only one the earlier + // account-switch case does not reach, since that one differs on account_id first. Here the + // account_id and refresh_token are identical on both documents and only the JWT subject + // moves, so a discard proves the subject check itself fires. + it('discards the rotation when only the id_token subject has changed', async () => { + const jwtWithSubject = (sub: string): string => + `h.${Buffer.from(JSON.stringify({ sub })).toString('base64url')}.s` + const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined) + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { access_token: 'access-old', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-a'), account_id: 'account-1' }, + last_refresh: new Date(NOW - 30 * 86_400_000).toISOString(), + }) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // Same account_id and refresh_token, a different person behind the same slot. + await reachedGrant + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { access_token: 'access-b', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-b'), account_id: 'account-1' }, + last_refresh: new Date(NOW).toISOString(), + }) + releaseGrant() + await pending + + const saved = await readAuth() + expect(saved.tokens.id_token).toBe(jwtWithSubject('user-b')) + expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH) + expect(errors).toHaveBeenCalledTimes(1) + }) + + it('still writes the rotation when the login on disk is unchanged', async () => { + await writeAuth(authDoc()) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // Same account, same refresh token: an unrelated rewrite is not a lineage change. + await reachedGrant + await writeAuth({ ...authDoc(), extra_field: 'harmless' }) + releaseGrant() + await pending + + expect((await readAuth()).tokens.refresh_token).toBe(NEW_REFRESH) + }) + + it('keeps the rotated refresh token when the reread file merely omits lineage keys', async () => { + await writeAuth(authDoc()) + + await fetchCodexQuota({ + authPath, + now: () => NOW, + // A cleanly parsed ChatGPT document that simply lacks account_id and + // refresh_token is not proof of a different login. + readFileSync: () => JSON.stringify({ auth_mode: 'chatgpt', tokens: { access_token: 'access-old' } }), + fetch: routes({ token: rotatedGrant, usage: usagePayload }), + }) + + expect((await readAuth()).tokens.refresh_token).toBe(NEW_REFRESH) + }) + // POSIX mode bits do not exist on Windows, where the ACL carries the privacy. it.skipIf(process.platform === 'win32')('rewrites the credential with the permissions it already had', async () => { for (const mode of [0o600, 0o400]) { diff --git a/app/electron/quota/codex.ts b/app/electron/quota/codex.ts index 8fb50f09..13660ae3 100644 --- a/app/electron/quota/codex.ts +++ b/app/electron/quota/codex.ts @@ -245,12 +245,47 @@ function str(value: unknown): string | null { return typeof value === 'string' && value ? value : null } +/** The `sub` claim of a JWT id_token, or null when it is absent or does not + * decode. Defensive at every step: a malformed id_token is evidence of nothing, + * so it must never itself read as a login mismatch. */ +function idTokenSubject(token: unknown): string | null { + if (typeof token !== 'string') return null + const payload = token.split('.')[1] + if (!payload) return null + try { + const claims = JSON.parse(Buffer.from(payload, 'base64url').toString('utf8')) as { sub?: unknown } + return typeof claims.sub === 'string' && claims.sub ? claims.sub : null + } catch { + return null + } +} + +/** Whether two documents belong to the same Codex login. A field counts as a + * mismatch only when it is a non-empty string on both sides and the two values + * differ: a document that merely omits account_id, refresh_token or id_token is + * not proof of a different login, and treating it as one would drop a rotated + * refresh token and sign the user out. Taken to its limit that means a document + * carrying NONE of the three reads as the same login and is written over. That + * is the deliberate side to err on: the alternative discards a live credential + * every time the file on disk cannot identify itself. */ +function sameLogin(held: AuthDoc, latest: AuthDoc): boolean { + const differs = (a: unknown, b: unknown): boolean => + typeof a === 'string' && a !== '' && typeof b === 'string' && b !== '' && a !== b + if (differs(held.tokens?.account_id, latest.tokens?.account_id)) return false + if (differs(held.tokens?.refresh_token, latest.tokens?.refresh_token)) return false + return !differs(idTokenSubject(held.tokens?.id_token), idTokenSubject(latest.tokens?.id_token)) +} + /** The document the rotated tokens are merged into: whatever the Codex CLI has * on disk right now, so a login it wrote since our own read is not clobbered. - * `null` means the login this rotation belongs to is gone (file removed, or no - * longer ChatGPT mode) and writing would resurrect it. An unreadable or - * unparseable file is NOT one of those cases: it falls back to the document we - * already hold, because dropping a rotated refresh token signs the user out. */ + * `null` means writing would be wrong: the login this rotation belongs to is + * gone (file removed, or no longer ChatGPT mode), or the file now holds a + * different login (the user ran `codex login` and switched accounts, or another + * process rotated the credential while our grant was in flight) and merging our + * tokens in would splice one account's account_id onto another's refresh token. + * An unreadable or unparseable file is NOT one of those cases: it falls back to + * the document we already hold, because a lineage check may only fire on a file + * that reads cleanly, and dropping a rotated refresh token signs the user out. */ function mergeBase(auth: AuthDoc, deps: CodexDeps): AuthDoc | null { let raw: string | null try { @@ -267,7 +302,11 @@ function mergeBase(auth: AuthDoc, deps: CodexDeps): AuthDoc | null { } if (!latest || typeof latest !== 'object') return auth const doc = latest as AuthDoc - return doc.auth_mode === 'chatgpt' ? doc : null + if (doc.auth_mode !== 'chatgpt') return null + // Serializing refreshes across processes with a file lock would also stop the + // wasted double grant, but the lineage check is what closes the corrupt-write + // hole on its own, so the lock is left out as more than the minimum here. + return sameLogin(auth, doc) ? doc : null } type RotatedTokens = { access: string | null; refresh: string | null; id: string | null } @@ -283,7 +322,7 @@ type RotatedTokens = { access: string | null; refresh: string | null; id: string function persistRotated(auth: AuthDoc, rotated: RotatedTokens, deps: CodexDeps): AuthDoc | null { const base = mergeBase(auth, deps) if (!base) { - console.error(`Codex refreshed its login but the credential at ${deps.authPath} no longer holds that ChatGPT login, so the new token was discarded. Run \`codex login\` if Codex signs you out.`) + console.error(`Codex refreshed its login but the credential at ${deps.authPath} no longer holds the ChatGPT login this rotation belongs to, so the new token was discarded. Run \`codex login\` if Codex signs you out.`) return null } const merged: AuthDoc = { diff --git a/app/electron/tray-settings-atomic.test.ts b/app/electron/tray-settings-atomic.test.ts index a31853a9..bb0ce5be 100644 --- a/app/electron/tray-settings-atomic.test.ts +++ b/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) => { + 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') }) diff --git a/app/electron/tray-settings-lock.test.ts b/app/electron/tray-settings-lock.test.ts new file mode 100644 index 00000000..9cc21afc --- /dev/null +++ b/app/electron/tray-settings-lock.test.ts @@ -0,0 +1,112 @@ +// @vitest-environment node +// The cross-process lock that serializes the read/modify/write of the tray app's files. These +// are the single-process unit tests (taken and released, a stale lock taken over, a dead +// holder's lock taken over, a live lock waited for, an abandoned lock left alone while someone +// else holds the right to reclaim it, and the merge preserving the other side's keys); the real +// Node-against-Rust contention test lives in windows/src-tauri/src/dock.rs. +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, utimesSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { acquireTrayFileLock, patchTrayFile, releaseTrayFileLock } from './tray-settings' + +let home: string +const dockTarget = () => join(home, '.config', 'codeburn', 'windows-dock.json') +const lockPath = () => join(home, '.config', 'codeburn', '.windows-dock.lock') + +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'tray-lock-')) + mkdirSync(join(home, '.config', 'codeburn'), { recursive: true }) +}) +afterEach(() => { rmSync(home, { recursive: true, force: true }) }) + +describe('the cross-process preference lock', () => { + it('takes the lock and releases it again', () => { + const fd = acquireTrayFileLock(dockTarget()) + expect(existsSync(lockPath())).toBe(true) + expect(JSON.parse(readFileSync(lockPath(), 'utf8')).pid).toBe(process.pid) + + releaseTrayFileLock(dockTarget(), fd) + expect(existsSync(lockPath())).toBe(false) + }) + + it('takes over a lock whose mtime is past the stale window', () => { + // A live pid (our own), so only the age gate can free it. Backdate the mtime rather than + // sleeping for the whole window. + writeFileSync(lockPath(), JSON.stringify({ pid: process.pid, at: 1 })) + const old = new Date(Date.now() - 60_000) + utimesSync(lockPath(), old, old) + + const fd = acquireTrayFileLock(dockTarget(), { staleMs: 30_000, waitMs: 500 }) + expect(JSON.parse(readFileSync(lockPath(), 'utf8')).pid).toBe(process.pid) + releaseTrayFileLock(dockTarget(), fd) + }) + + it('takes over a dead holder without waiting out the budget', () => { + // A pid that is not us and is almost certainly gone, with a fresh mtime: only the pid probe, + // not the age gate, can free this one. + writeFileSync(lockPath(), JSON.stringify({ pid: 2_147_483_646, at: Date.now() })) + const started = Date.now() + const fd = acquireTrayFileLock(dockTarget(), { staleMs: 60_000, waitMs: 1_000, pollMs: 20 }) + expect(Date.now() - started).toBeLessThan(400) + releaseTrayFileLock(dockTarget(), fd) + }) + + it('waits for a live lock and then fails loudly rather than writing behind it', () => { + const held = acquireTrayFileLock(dockTarget()) + const started = Date.now() + // Our own pid reads as alive and the mtime is fresh, so the lock is never abandoned. + expect(() => acquireTrayFileLock(dockTarget(), { staleMs: 60_000, waitMs: 150, pollMs: 20 })) + .toThrow(/could not lock/) + expect(Date.now() - started).toBeGreaterThanOrEqual(130) + releaseTrayFileLock(dockTarget(), held) + }) + + it('leaves an abandoned lock alone while another process holds the takeover right', () => { + // Abandoned by the age gate, so without the takeover right any contender would remove it. + writeFileSync(lockPath(), JSON.stringify({ pid: process.pid, at: 1 })) + const old = new Date(Date.now() - 60_000) + utimesSync(lockPath(), old, old) + // A reclaim already in progress: a fresh mtime and a live pid, so the takeover file is not + // itself abandoned. The contender that finds it must not touch the lock, because the + // reclaimer may already have replaced it with a fresh lock of its own, and removing that + // would leave the two of them holding the file at once. + const takeover = `${lockPath()}.takeover` + writeFileSync(takeover, JSON.stringify({ pid: process.pid, at: Date.now() })) + + expect(() => acquireTrayFileLock(dockTarget(), { staleMs: 30_000, waitMs: 100, pollMs: 20 })) + .toThrow(/could not lock/) + expect(existsSync(lockPath())).toBe(true) + + // With nobody reclaiming, the same abandoned lock is taken over as it always was, and the + // takeover file is not left behind. + rmSync(takeover) + const fd = acquireTrayFileLock(dockTarget(), { staleMs: 30_000, waitMs: 500, pollMs: 20 }) + expect(JSON.parse(readFileSync(lockPath(), 'utf8')).pid).toBe(process.pid) + expect(existsSync(takeover)).toBe(false) + releaseTrayFileLock(dockTarget(), fd) + }) + + it('leaves a successor lock alone on release', () => { + const fd = acquireTrayFileLock(dockTarget()) + // Simulate a successor that took over after us (only possible past the stale window). + writeFileSync(lockPath(), JSON.stringify({ pid: process.pid + 1, at: Date.now() })) + releaseTrayFileLock(dockTarget(), fd) + expect(existsSync(lockPath())).toBe(true) + expect(JSON.parse(readFileSync(lockPath(), 'utf8')).pid).toBe(process.pid + 1) + }) + + it('still merges keys the other side owns, and cleans up its lock', () => { + writeFileSync(dockTarget(), JSON.stringify({ enabled: true, placement: { docked: 'right' } })) + + expect(patchTrayFile('dock', { scale: 1.1 }, home)).toEqual({ + enabled: true, + placement: { docked: 'right' }, + scale: 1.1, + }) + // No lock file is left behind once the patch returns. + expect(existsSync(lockPath())).toBe(false) + expect(() => statSync(lockPath())).toThrow() + }) +}) diff --git a/app/electron/tray-settings.ts b/app/electron/tray-settings.ts index e3395089..60a430dc 100644 --- a/app/electron/tray-settings.ts +++ b/app/electron/tray-settings.ts @@ -15,9 +15,11 @@ // After a write the tray app is nudged with `--reload-settings`, which makes it re-read both // files and broadcast to its own windows. That is done by the caller, which owns the process. -import { mkdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from 'node:fs' +import { + closeSync, mkdirSync, openSync, readFileSync, renameSync, statSync, unlinkSync, writeFileSync, writeSync, +} from 'node:fs' import { homedir } from 'node:os' -import { dirname, join } from 'node:path' +import { basename, dirname, join } from 'node:path' export type TraySettingsFile = 'app' | 'dock' @@ -44,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 { try { const parsed: unknown = JSON.parse(readFileSync(filePath(file, home), 'utf8').replace(/^/, '')) @@ -61,15 +101,178 @@ export function readTrayFile(file: TraySettingsFile, home = homedir()): Record.lock` (windows-dock.json -> +// `.windows-dock.lock`). +// Body: one line of JSON, `{"pid":,"at":}`. +// Acquire: exclusive create ('wx'). On success keep the handle open for the whole cycle. +// On collision take the lock over only if abandoned, else poll until a short wait +// budget runs out and then throw rather than write behind the holder. +// Abandoned: its mtime is older than the stale window (a crashed holder never refreshes it), +// OR its recorded pid is not ours and no longer alive. A live holder is neither. +// Takeover: removing an abandoned lock is itself arbitrated, by an exclusive create of +// `.takeover`. Only its winner may unlink the lock, and only after +// re-checking that the lock is still abandoned. See `reclaimAbandoned`. +// Release: close the handle and unlink, unless it now carries a different pid. + +const LOCK_POLL_MS = 50 +const LOCK_WAIT_MS = 2_000 +const LOCK_STALE_MS = 30_000 + +export type TrayLockOptions = { waitMs?: number; pollMs?: number; staleMs?: number } + +function lockPathFor(target: string): string { + const stem = basename(target).replace(/\.json$/, '') + return join(dirname(target), `.${stem}.lock`) +} + +// A synchronous sleep, so the whole lock stays synchronous and callers of patchTrayFile do not +// have to become async. Atomics.wait blocks this thread without spinning the CPU. +const lockSleepBuffer = new Int32Array(new SharedArrayBuffer(4)) +function sleepSync(ms: number): void { + Atomics.wait(lockSleepBuffer, 0, 0, ms) +} + +// Signal-0 style liveness. A false "alive" (a reused pid) only delays recovery to the age gate; +// a false "dead" is the dangerous direction, so our own pid and anything unprobeable read alive. +function holderAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false + if (pid === process.pid) return true + try { process.kill(pid, 0); return true } + catch (err) { return (err as NodeJS.ErrnoException).code === 'EPERM' } +} + +function lockAbandoned(lockPath: string, staleMs: number): boolean { + let mtimeMs: number + try { mtimeMs = statSync(lockPath).mtimeMs } + catch { return false } // vanished between the failed create and this check; just retry create + if (Date.now() - mtimeMs > staleMs) return true + let pid: number | undefined + try { + const parsed = JSON.parse(readFileSync(lockPath, 'utf8')) as { pid?: unknown } + if (typeof parsed?.pid === 'number') pid = parsed.pid + } catch { /* empty or unparseable body: only the age gate can free it */ } + return pid !== undefined && pid !== process.pid && !holderAlive(pid) +} + +/** + * Remove an abandoned lock, but only as the one process entitled to. Two contenders that both + * find the same stale lock would otherwise both unlink it: the first has already replaced it + * with a lock of its own, and the second's unlink deletes that fresh one, so both walk away + * holding the file. Windows usually hides this, because a lock a live holder still has open + * will not unlink at all, but this protocol is also run on Linux and macOS, where the unlink + * always succeeds. + * + * So the removal is arbitrated by the same primitive the lock itself uses: an exclusive create + * of `.takeover`. Its winner alone may unlink, and only after re-checking staleness under + * that right, because a rival may have reclaimed already and now hold a lock that is not + * abandoned and not ours to remove. The takeover file is dropped on every path out, and never + * held across the wait; one left behind by a reclaimer that died is freed by the same staleness + * rule as the lock. + * + * Returns whether the caller should retry the exclusive create at once. + */ +function reclaimAbandoned(lockPath: string, staleMs: number): boolean { + const takeoverPath = `${lockPath}.takeover` + let fd: number + try { + fd = openSync(takeoverPath, 'wx', 0o600) + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err + // Somebody else is reclaiming, or a dead reclaimer left this behind. The lock itself is + // never touched from here; the most this does is free the takeover file for a later try. + if (lockAbandoned(takeoverPath, staleMs)) { + try { unlinkSync(takeoverPath) } catch { /* already gone */ } + } + return false + } + try { + try { writeSync(fd, JSON.stringify({ pid: process.pid, at: Date.now() })) } catch { /* advisory */ } + if (!lockAbandoned(lockPath, staleMs)) return false + try { unlinkSync(lockPath) } catch { return false } + return true + } finally { + try { closeSync(fd) } catch { /* already closed */ } + try { unlinkSync(takeoverPath) } catch { /* already gone */ } + } +} + +/** + * Take the cross-process lock for `target`, returning the open descriptor to release. Throws if + * the lock stays held past the wait budget rather than writing behind the holder, so a contended + * write is reported to the caller instead of being silently lost. + */ +export function acquireTrayFileLock(target: string, options: TrayLockOptions = {}): number { + const waitMs = options.waitMs ?? LOCK_WAIT_MS + const pollMs = options.pollMs ?? LOCK_POLL_MS + const staleMs = options.staleMs ?? LOCK_STALE_MS + mkdirSync(dirname(target), { recursive: true }) + const lockPath = lockPathFor(target) + const body = JSON.stringify({ pid: process.pid, at: Date.now() }) + const deadline = Date.now() + waitMs + for (;;) { + let fd: number + try { + fd = openSync(lockPath, 'wx', 0o600) + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err + // A successful reclaim means the holder was genuinely abandoned and this process won the + // right to remove its lock; retry the create at once. Anything else (a live holder, a + // contender already reclaiming, an unlink that failed) falls through to the wait. + if (lockAbandoned(lockPath, staleMs) && reclaimAbandoned(lockPath, staleMs)) continue + if (Date.now() >= deadline) { + throw new Error( + `could not lock ${basename(lockPath)} within ${waitMs}ms; another process is writing ` + + `${basename(target)}, so nothing was changed`, + ) + } + sleepSync(pollMs) + continue + } + // The body is advisory; even if this write fails we still hold the lock the create just won. + try { writeSync(fd, body) } catch { /* body is advisory */ } + return fd + } +} + +/** Close the descriptor and remove the lock, unless it now carries a successor's pid. */ +export function releaseTrayFileLock(target: string, fd: number): void { + const lockPath = lockPathFor(target) + try { closeSync(fd) } catch { /* already closed */ } + try { + const parsed = JSON.parse(readFileSync(lockPath, 'utf8')) as { pid?: unknown } + // A different pid means we were taken over (only possible past the stale window); leave it. + if (typeof parsed?.pid === 'number' && parsed.pid !== process.pid) return + } catch { /* empty or unparseable: it is the one we just made, so remove it */ } + try { unlinkSync(lockPath) } catch { /* already gone */ } +} + /** Merge the given keys into the file, leaving every other key the tray app owns alone. */ export function patchTrayFile( file: TraySettingsFile, patch: Record, home = homedir(), ): Record { - const merged = { ...readTrayFile(file, home), ...patch } - writeFileAtomic(filePath(file, home), `${JSON.stringify(merged, null, 2)}\n`) - return merged + const target = filePath(file, home) + // The whole read/modify/write is serialized against the tray app. See the protocol above. + const fd = acquireTrayFileLock(target) + try { + const merged = { ...readTrayFile(file, home), ...patch } + writeFileAtomic(target, `${JSON.stringify(merged, null, 2)}\n`) + return merged + } finally { + releaseTrayFileLock(target, fd) + } } // What each setting may be ----------------------------------------------------------------- diff --git a/app/scripts/tray-settings-xproc-worker.ts b/app/scripts/tray-settings-xproc-worker.ts new file mode 100644 index 00000000..2b553aa4 --- /dev/null +++ b/app/scripts/tray-settings-xproc-worker.ts @@ -0,0 +1,56 @@ +// One party in the cross-process lock test. The Rust side (windows/src-tauri/src/dock.rs, test +// `concurrent_node_and_rust_patches_to_the_dock_file_both_survive`) spawns this with a shared +// home directory, so a real Node process and a real Rust process hammer windows-dock.json at +// once. Each patches its OWN key with a counter that only ever rises; the Rust side watches the +// file from a reader thread, and a counter seen to fall there is a write the other side erased. +// +// It lives in scripts/ rather than electron/ so it is neither bundled into the app nor picked up +// as a vitest test, only run on demand by the Rust test. +// +// argv: +// CB_TRAY_XPROC_NOLOCK=1 replays the same read/modify/write WITHOUT the lock, which is the bug +// the test catches; the Rust side reads the same variable so one env toggles both parties. +import { existsSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' + +import { patchTrayFile, readTrayFile, writeFileAtomic } from '../electron/tray-settings' + +const [home, key, itersRaw, barriers] = process.argv.slice(2) +if (!home || !key || !itersRaw || !barriers) { + console.error('usage: tray-settings-xproc-worker ') + process.exit(2) +} +const iters = Number(itersRaw) +const nolock = process.env['CB_TRAY_XPROC_NOLOCK'] === '1' +const dockPath = join(home, '.config', 'codeburn', 'windows-dock.json') + +const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)) +function sleepSync(ms: number): void { + Atomics.wait(sleepBuffer, 0, 0, ms) +} + +function waitFor(path: string): void { + const deadline = Date.now() + 60_000 + while (!existsSync(path)) { + if (Date.now() >= deadline) throw new Error(`timed out waiting for ${path}`) + sleepSync(10) + } +} + +// Started, but do not race ahead: wait for the go barrier so both loops overlap. +writeFileSync(join(barriers, 'node.ready'), '') +waitFor(join(barriers, 'go')) + +for (let i = 0; i < iters; i++) { + if (nolock) { + const current = readTrayFile('dock', home) + // Widen the read/modify/write window so the lost update is easy to provoke. + sleepSync(1) + writeFileAtomic(dockPath, `${JSON.stringify({ ...current, [key]: i }, null, 2)}\n`) + } else { + patchTrayFile('dock', { [key]: i }, home) + } + if (i % 7 === 0) sleepSync(1) +} + +writeFileSync(join(barriers, 'node.done'), '') diff --git a/src/quota/codex.ts b/src/quota/codex.ts index ac929226..a0dddcdc 100644 --- a/src/quota/codex.ts +++ b/src/quota/codex.ts @@ -245,12 +245,47 @@ function str(value: unknown): string | null { return typeof value === 'string' && value ? value : null } +/** The `sub` claim of a JWT id_token, or null when it is absent or does not + * decode. Defensive at every step: a malformed id_token is evidence of nothing, + * so it must never itself read as a login mismatch. */ +function idTokenSubject(token: unknown): string | null { + if (typeof token !== 'string') return null + const payload = token.split('.')[1] + if (!payload) return null + try { + const claims = JSON.parse(Buffer.from(payload, 'base64url').toString('utf8')) as { sub?: unknown } + return typeof claims.sub === 'string' && claims.sub ? claims.sub : null + } catch { + return null + } +} + +/** Whether two documents belong to the same Codex login. A field counts as a + * mismatch only when it is a non-empty string on both sides and the two values + * differ: a document that merely omits account_id, refresh_token or id_token is + * not proof of a different login, and treating it as one would drop a rotated + * refresh token and sign the user out. Taken to its limit that means a document + * carrying NONE of the three reads as the same login and is written over. That + * is the deliberate side to err on: the alternative discards a live credential + * every time the file on disk cannot identify itself. */ +function sameLogin(held: AuthDoc, latest: AuthDoc): boolean { + const differs = (a: unknown, b: unknown): boolean => + typeof a === 'string' && a !== '' && typeof b === 'string' && b !== '' && a !== b + if (differs(held.tokens?.account_id, latest.tokens?.account_id)) return false + if (differs(held.tokens?.refresh_token, latest.tokens?.refresh_token)) return false + return !differs(idTokenSubject(held.tokens?.id_token), idTokenSubject(latest.tokens?.id_token)) +} + /** The document the rotated tokens are merged into: whatever the Codex CLI has * on disk right now, so a login it wrote since our own read is not clobbered. - * `null` means the login this rotation belongs to is gone (file removed, or no - * longer ChatGPT mode) and writing would resurrect it. An unreadable or - * unparseable file is NOT one of those cases: it falls back to the document we - * already hold, because dropping a rotated refresh token signs the user out. */ + * `null` means writing would be wrong: the login this rotation belongs to is + * gone (file removed, or no longer ChatGPT mode), or the file now holds a + * different login (the user ran `codex login` and switched accounts, or another + * process rotated the credential while our grant was in flight) and merging our + * tokens in would splice one account's account_id onto another's refresh token. + * An unreadable or unparseable file is NOT one of those cases: it falls back to + * the document we already hold, because a lineage check may only fire on a file + * that reads cleanly, and dropping a rotated refresh token signs the user out. */ function mergeBase(auth: AuthDoc, deps: CodexDeps): AuthDoc | null { let raw: string | null try { @@ -267,7 +302,11 @@ function mergeBase(auth: AuthDoc, deps: CodexDeps): AuthDoc | null { } if (!latest || typeof latest !== 'object') return auth const doc = latest as AuthDoc - return doc.auth_mode === 'chatgpt' ? doc : null + if (doc.auth_mode !== 'chatgpt') return null + // Serializing refreshes across processes with a file lock would also stop the + // wasted double grant, but the lineage check is what closes the corrupt-write + // hole on its own, so the lock is left out as more than the minimum here. + return sameLogin(auth, doc) ? doc : null } type RotatedTokens = { access: string | null; refresh: string | null; id: string | null } @@ -283,7 +322,7 @@ type RotatedTokens = { access: string | null; refresh: string | null; id: string function persistRotated(auth: AuthDoc, rotated: RotatedTokens, deps: CodexDeps): AuthDoc | null { const base = mergeBase(auth, deps) if (!base) { - console.error(`Codex refreshed its login but the credential at ${deps.authPath} no longer holds that ChatGPT login, so the new token was discarded. Run \`codex login\` if Codex signs you out.`) + console.error(`Codex refreshed its login but the credential at ${deps.authPath} no longer holds the ChatGPT login this rotation belongs to, so the new token was discarded. Run \`codex login\` if Codex signs you out.`) return null } const merged: AuthDoc = { diff --git a/tests/quota-codex-refresh.test.ts b/tests/quota-codex-refresh.test.ts index 8e2a509c..730b597d 100644 --- a/tests/quota-codex-refresh.test.ts +++ b/tests/quota-codex-refresh.test.ts @@ -336,6 +336,131 @@ describe('Codex quota credential rotation', () => { expect(errors).toHaveBeenCalledTimes(1) }) + it('discards a rotation when the login on disk changes while the grant is in flight', async () => { + await writeAuth(authDoc()) + const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // The user ran `codex login` and switched accounts while our grant was open. + await reachedGrant + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { + access_token: 'access-other', + refresh_token: 'refresh-other-account', + id_token: 'id-other', + account_id: 'account-2', + }, + last_refresh: new Date(NOW).toISOString(), + }) + releaseGrant() + await pending + + // The new account's document is left exactly as the CLI wrote it: our grant + // spent account-1's refresh token, so writing it back here would splice + // account-1's token onto account-2's identity. + const saved = await readAuth() + expect(saved.tokens.account_id).toBe('account-2') + expect(saved.tokens.refresh_token).toBe('refresh-other-account') + expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH) + expect(errors).toHaveBeenCalledTimes(1) + }) + + // The id_token subject is the last of the three lineage fields and the only one the earlier + // account-switch case does not reach, since that one differs on account_id first. Here the + // account_id and refresh_token are identical on both documents and only the JWT subject + // moves, so a discard proves the subject check itself fires. + it('discards the rotation when only the id_token subject has changed', async () => { + const jwtWithSubject = (sub: string): string => + `h.${Buffer.from(JSON.stringify({ sub })).toString('base64url')}.s` + const errors = vi.spyOn(console, 'error').mockImplementation(() => undefined) + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { access_token: 'access-old', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-a'), account_id: 'account-1' }, + last_refresh: new Date(NOW - 30 * 86_400_000).toISOString(), + }) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // Same account_id and refresh_token, a different person behind the same slot. + await reachedGrant + await writeAuth({ + auth_mode: 'chatgpt', + tokens: { access_token: 'access-b', refresh_token: OLD_REFRESH, id_token: jwtWithSubject('user-b'), account_id: 'account-1' }, + last_refresh: new Date(NOW).toISOString(), + }) + releaseGrant() + await pending + + const saved = await readAuth() + expect(saved.tokens.id_token).toBe(jwtWithSubject('user-b')) + expect(saved.tokens.refresh_token).not.toBe(NEW_REFRESH) + expect(errors).toHaveBeenCalledTimes(1) + }) + + it('still writes the rotation when the login on disk is unchanged', async () => { + await writeAuth(authDoc()) + let grantStarted = (): void => {} + const reachedGrant = new Promise(resolve => { grantStarted = resolve }) + let releaseGrant = (): void => {} + const heldGrant = new Promise(resolve => { releaseGrant = resolve }) + + const pending = fetchCodexQuota({ + authPath, + now: () => NOW, + fetch: routes({ + token: async () => { grantStarted(); await heldGrant; return rotatedGrant() }, + usage: usagePayload, + }), + }) + + // Same account, same refresh token: an unrelated rewrite is not a lineage change. + await reachedGrant + await writeAuth({ ...authDoc(), extra_field: 'harmless' }) + releaseGrant() + await pending + + expect((await readAuth()).tokens.refresh_token).toBe(NEW_REFRESH) + }) + + it('keeps the rotated refresh token when the reread file merely omits lineage keys', async () => { + await writeAuth(authDoc()) + + await fetchCodexQuota({ + authPath, + now: () => NOW, + // A cleanly parsed ChatGPT document that simply lacks account_id and + // refresh_token is not proof of a different login. + readFileSync: () => JSON.stringify({ auth_mode: 'chatgpt', tokens: { access_token: 'access-old' } }), + fetch: routes({ token: rotatedGrant, usage: usagePayload }), + }) + + expect((await readAuth()).tokens.refresh_token).toBe(NEW_REFRESH) + }) + // POSIX mode bits do not exist on Windows, where the ACL carries the privacy. it.skipIf(process.platform === 'win32')('rewrites the credential with the permissions it already had', async () => { for (const mode of [0o600, 0o400]) { diff --git a/windows/src-tauri/src/dock.rs b/windows/src-tauri/src/dock.rs index 5b29b5bb..36855596 100644 --- a/windows/src-tauri/src/dock.rs +++ b/windows/src-tauri/src/dock.rs @@ -12,7 +12,7 @@ //! into the frames it is handed. use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::Mutex; @@ -519,7 +519,15 @@ fn state_path() -> PathBuf { } fn read_state() -> serde_json::Map { - fs::read(state_path()) + 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 { + fs::read(path) .ok() .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) .and_then(|value| value.as_object().cloned()) @@ -534,8 +542,7 @@ static WRITE_SEQUENCE: AtomicU64 = AtomicU64::new(0); /// The desktop app reads this file as well, and a plain write is not one step: a reader that /// arrives partway through it gets truncated JSON, falls back to an empty state, and the /// placement and the dock's own switch are gone. Writing beside it and renaming over is. -fn write_state(state: &serde_json::Map) -> Result<()> { - let path = state_path(); +fn write_state_at(path: &Path, state: &serde_json::Map) -> Result<()> { if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; } @@ -546,7 +553,9 @@ fn write_state(state: &serde_json::Map) -> Result<()> 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()); @@ -554,6 +563,30 @@ fn write_state(state: &serde_json::Map) -> Result<()> Ok(()) } +/// The one write path for this file: take the cross-process lock, read the current state, +/// apply `mutate`, and rename the whole thing back, all before the lock is released. The +/// desktop app owns this file too and runs the same protocol from `tray-settings.ts`, so +/// without the lock two writers that touch different keys lose each other's change even though +/// the rename keeps the file whole. The rename is still what keeps a reader from seeing a torn +/// file; the lock is what keeps a second writer from erasing this one. See +/// `crate::settings::prefs_lock` for the protocol. +fn patch_state( + mutate: impl FnOnce(&mut serde_json::Map), +) -> Result> { + patch_state_at(&state_path(), mutate) +} + +fn patch_state_at( + path: &Path, + mutate: impl FnOnce(&mut serde_json::Map), +) -> Result> { + let _lock = crate::settings::prefs_lock::acquire(path)?; + let mut state = read_state_at(path); + mutate(&mut state); + write_state_at(path, &state)?; + Ok(state) +} + /// What the stored state reads as from outside. The file is shared with the desktop app, which /// writes it on every platform, so a switch turned on there is normalized away here rather than /// reported as a dock that is running when none can be. @@ -576,15 +609,15 @@ pub fn read_prefs() -> serde_json::Map { pub fn patch_prefs( values: serde_json::Map, ) -> Result> { - let mut state = read_state(); - for (key, value) in values { - if value.is_null() { - state.remove(&key); - } else { - state.insert(key, value); + let state = patch_state(|state| { + for (key, value) in values { + if value.is_null() { + state.remove(&key); + } else { + state.insert(key, value); + } } - } - write_state(&state)?; + })?; Ok(normalized(state)) } @@ -608,17 +641,20 @@ pub fn telemetry_props() -> serde_json::Value { } pub fn set_enabled(enabled: bool) -> Result<()> { - let mut state = read_state(); - state.insert("enabled".into(), serde_json::Value::Bool(enabled)); - write_state(&state) + patch_state(|state| { + state.insert("enabled".into(), serde_json::Value::Bool(enabled)); + })?; + Ok(()) } /// The provider the resting rail shows. A click on another row makes it the preferred one; /// the page reads the current one out of the preferences with everything else. pub fn set_preferred_provider(id: &str) -> Result<()> { - let mut state = read_state(); - state.insert("preferred".into(), serde_json::Value::String(id.to_owned())); - write_state(&state) + let id = id.to_owned(); + patch_state(|state| { + state.insert("preferred".into(), serde_json::Value::String(id)); + })?; + Ok(()) } fn load_placement() -> Placement { @@ -629,9 +665,11 @@ fn load_placement() -> Placement { } fn save_placement(placement: &Placement) -> Result<()> { - let mut state = read_state(); - state.insert("placement".into(), serde_json::to_value(placement)?); - write_state(&state) + let value = serde_json::to_value(placement)?; + patch_state(|state| { + state.insert("placement".into(), value); + })?; + Ok(()) } // Live state -------------------------------------------------------------------------------- @@ -888,7 +926,13 @@ pub fn apply_layout(window: &tauri::WebviewWindow, request: LayoutRequest) -> Op /// Re-homes the rail after a drop or a menu choice and tells the page where it came from so /// it can glide into place. fn settle(app: &AppHandle, window: &tauri::WebviewWindow, placement: Placement, from_rail: Rect) { - let _ = save_placement(&placement); + if let Err(err) = save_placement(&placement) { + // The rail still moves: the placement below is what this run lays out from. What is + // lost is the next launch, which reads the file and puts the rail back where it was. + // Losing the cross-process lock to the desktop app is a new way for this to happen, and + // it happens silently, so it is worth a line in the log. + crate::log_line!("codeburn dock: placement not saved, the rail will revert at next launch: {err:#}"); + } { let mut state = lock(); state.placement = Some(placement); @@ -1841,7 +1885,7 @@ mod tests { #[test] fn a_state_write_swaps_a_whole_file_in_and_leaves_no_scratch_behind() { let before = read_state(); - let Ok(()) = write_state(&before) else { return }; + let Ok(()) = write_state_at(&state_path(), &before) else { return }; assert_eq!(read_state(), before); // The scratch name carries this process's id, so only this test's own leavings count @@ -1938,4 +1982,209 @@ mod tests { assert_eq!(edge, Edge::Right); assert!((progress - 0.5).abs() < 0.01); } + + /// The merge keeps a key the caller never touched, which is what lets each side write only + /// its own keys into a file the other side co-owns. Same shape as settings.rs's own + /// null-patch test, kept off the real state file so it never touches the developer's home. + #[test] + fn patching_one_key_leaves_the_other_sides_keys_alone() { + let dir = std::env::temp_dir().join(format!("cb-dock-merge-{}", std::process::id())); + fs::create_dir_all(&dir).unwrap(); + let file = dir.join("windows-dock.json"); + // Keys this side does not own: the placement (written by dragging) and the tray app's + // own enabled switch. + fs::write(&file, "{\"placement\":{\"docked\":\"right\"},\"enabled\":true}").unwrap(); + + let merged = patch_state_at(&file, |state| { + state.insert("scale".into(), serde_json::json!(1.1)); + }) + .unwrap(); + + assert_eq!(merged.get("scale"), Some(&serde_json::json!(1.1))); + assert_eq!(merged.get("enabled"), Some(&serde_json::Value::Bool(true))); + assert_eq!( + merged.get("placement"), + Some(&serde_json::json!({ "docked": "right" })) + ); + // And a null value removes just that key, leaving the rest. + let after = patch_state_at(&file, |state| { + state.remove("scale"); + }) + .unwrap(); + assert!(after.get("scale").is_none()); + assert!(after.get("placement").is_some()); + let _ = fs::remove_dir_all(&dir); + } + + /// The headline regression: a real Node process and this real Rust process patch DIFFERENT + /// keys of windows-dock.json at the same time, and both changes must survive. Without the + /// shared lock the two read/modify/write cycles interleave and the later rename erases the + /// other side's key; run it with CB_TRAY_XPROC_NOLOCK=1 to see exactly that failure. It is + /// two processes, not two threads, which is the only way to exercise the cross-process lock. + /// + /// What is asserted is monotonicity, not finality. Each side's key holds a counter that only + /// ever rises, and a reader thread watches the file throughout: a lost update resurrects a + /// stale value, so a counter falls. Comparing only the two FINAL values, as this test first + /// did, catches a lost update only when the last writes happen to collide, which let it pass + /// on a deliberately broken lock about one run in three: weak evidence for the one property + /// it exists to prove. + #[test] + fn concurrent_node_and_rust_patches_to_the_dock_file_both_survive() { + use std::io::Read; + use std::process::{Command, Stdio}; + use std::sync::Arc; + use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + + let repo = Path::new(env!("CARGO_MANIFEST_DIR")).join("..").join(".."); + let worker = repo.join("app").join("scripts").join("tray-settings-xproc-worker.ts"); + if !worker.exists() { + eprintln!("skipping cross-process test: worker not found at {}", worker.display()); + return; + } + + let nanos = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos(); + let root = std::env::temp_dir().join(format!("cb-xproc-{}-{nanos}", std::process::id())); + let config_dir = root.join(".config").join("codeburn"); + let barriers = root.join("barriers"); + fs::create_dir_all(&config_dir).unwrap(); + fs::create_dir_all(&barriers).unwrap(); + let dock_file = config_dir.join("windows-dock.json"); + // A key neither side writes, to prove the merge preserves it through all the churn. + fs::write(&dock_file, "{\"other\":\"keep\"}").unwrap(); + + let iters: u32 = 100; + let nolock = std::env::var("CB_TRAY_XPROC_NOLOCK").ok().as_deref() == Some("1"); + + let mut child = match Command::new("node") + .arg("--import") + .arg("tsx") + .arg(&worker) + .arg(&root) + .arg("fromNode") + .arg(iters.to_string()) + .arg(&barriers) + .current_dir(&repo) + .stdout(Stdio::null()) + .stderr(Stdio::piped()) + .spawn() + { + Ok(child) => child, + Err(err) => { + eprintln!("skipping cross-process test: could not spawn node: {err}"); + let _ = fs::remove_dir_all(&root); + return; + } + }; + + let read_stderr = |child: &mut std::process::Child| -> String { + let mut text = String::new(); + if let Some(mut pipe) = child.stderr.take() { + let _ = pipe.read_to_string(&mut text); + } + text + }; + + // Wait until the worker has finished starting (tsx can take several seconds on this VM) + // before releasing either loop, so the two really do write at the same time. + let ready = barriers.join("node.ready"); + let start = Instant::now(); + while !ready.exists() { + if let Ok(Some(status)) = child.try_wait() { + panic!("node worker exited early ({status}); stderr:\n{}", read_stderr(&mut child)); + } + if start.elapsed() > Duration::from_secs(60) { + let _ = child.kill(); + panic!("node worker never signalled ready; stderr:\n{}", read_stderr(&mut child)); + } + std::thread::sleep(Duration::from_millis(50)); + } + fs::write(barriers.join("go"), b"").unwrap(); + + // The watcher: a third party that only ever reads. It has to be a separate reader, + // because neither writer can see its own counter rolled back. In the unlocked cycle each + // one carries the other side's value forward itself, so what it reads back is never + // older than what it last read, whatever the file did in between. A pure observer sees + // 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 lock-free, the way both apps really do, and polls at 1ms rather than spinning: + // a settings reader reads on reload and on a schedule, not every instant. That still + // samples the file thousands of times across the run, so a counter rolled back by a lost + // write is caught, and it still puts `crate::settings::replace_file`'s retry under real + // load, a replace colliding with an open read handle many times over. A reader that held + // the handle every instant is not one either app has: on Windows, where an open handle + // blocks a rename, it would be an unwinnable race no bounded retry could pass, and on real + // Windows CI that is exactly what it did. The rolled-back value persists in the file + // regardless, so the timing-independent final-state assertions below are the real + // backstop, and the unlocked path still fails, so detection is intact. + let stop = Arc::new(AtomicBool::new(false)); + let watcher = { + let (dock_file, stop) = (dock_file.clone(), stop.clone()); + std::thread::spawn(move || { + let (mut high_node, mut high_rust) = (0u64, 0u64); + let mut regression: Option = None; + while !stop.load(Ordering::Relaxed) { + std::thread::sleep(Duration::from_millis(1)); + let state = read_state_at(&dock_file); + for (key, high) in [("fromNode", &mut high_node), ("fromRust", &mut high_rust)] { + let Some(value) = state.get(key).and_then(serde_json::Value::as_u64) else { + continue; + }; + if value < *high { + regression.get_or_insert(format!( + "{key} went backwards ({} -> {value}): a write was lost", + *high + )); + } + *high = (*high).max(value); + } + } + regression + }) + }; + + for i in 0..iters { + let value = serde_json::Value::from(i); + if nolock { + // The demonstration path: the same cycle without the lock, i.e. the bug. + let mut state = read_state_at(&dock_file); + std::thread::sleep(Duration::from_millis(1)); + state.insert("fromRust".into(), value); + write_state_at(&dock_file, &state).unwrap(); + } else { + patch_state_at(&dock_file, |state| { + state.insert("fromRust".into(), value); + }) + .unwrap(); + } + if i % 7 == 0 { + std::thread::sleep(Duration::from_millis(1)); + } + } + + let status = child.wait().unwrap(); + let stderr = read_stderr(&mut child); + stop.store(true, Ordering::Relaxed); + let regression = watcher.join().unwrap(); + let final_state = read_state_at(&dock_file); + let _ = fs::remove_dir_all(&root); + assert!(status.success(), "node worker failed: {stderr}"); + assert!(regression.is_none(), "{}", regression.unwrap_or_default()); + assert_eq!( + final_state.get("other").and_then(|v| v.as_str()), + Some("keep"), + "the key neither side wrote survived" + ); + assert_eq!( + final_state.get("fromNode").and_then(serde_json::Value::as_u64), + Some(u64::from(iters - 1)), + "the desktop app's final write survived the tray app's writes" + ); + assert_eq!( + final_state.get("fromRust").and_then(serde_json::Value::as_u64), + Some(u64::from(iters - 1)), + "the tray app's final write survived the desktop app's writes" + ); + } } diff --git a/windows/src-tauri/src/settings.rs b/windows/src-tauri/src/settings.rs index 7d85295c..e19e076a 100644 --- a/windows/src-tauri/src/settings.rs +++ b/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}; @@ -41,6 +41,413 @@ const MIN_HEIGHT: f64 = 520.0; /// mount, because an event emitted while the webview is still loading has nobody to hear it. static PENDING_SECTION: Mutex> = Mutex::new(None); +// Cross-process preference lock --------------------------------------------------------- +// +// windows-settings.json and windows-dock.json are each owned by two processes at once: this +// tray app and the Electron desktop app. Both do a read / modify / write to merge their own +// keys into whatever the other last stored. The atomic rename each already does stops a torn +// file, but not a lost update: two writers read the same state, change different keys, and the +// later rename erases the earlier writer's key. The user watches a dock placement, an enabled +// switch, a provider choice or a scale silently revert. This lock serializes the whole cycle +// so that never happens. It is the one place the protocol is written down; `dock.rs` and +// `app/electron/tray-settings.ts` point here rather than restating it. +// +// PROTOCOL (honoured identically by app/electron/tray-settings.ts): +// +// Lock file: sibling of the target named `..lock` (windows-dock.json -> +// `.windows-dock.lock`), the same `.config.lock` shape config.rs uses. +// Body: one line of JSON, `{"pid":,"at":}`. +// Acquire: exclusive create (create_new / 'wx'). On success the holder keeps the handle +// open for the whole cycle and returns a guard. On collision the existing lock +// is taken over only if abandoned; otherwise the contender polls until a short +// wait budget runs out and then FAILS loudly rather than writing behind the +// holder, so a contended write is reported to the caller, never silently lost. +// Abandoned: its mtime is older than the stale window (a crashed holder never refreshes +// it), OR its recorded pid is not our own and no longer alive. The pid probe is +// the fast path for a crash: a dead holder is recovered on the first poll, well +// inside the wait budget. The age gate is the backstop for a pid we cannot probe +// or a reused one. A live holder is neither, which is the safety argument: a +// fresh mtime plus a live pid are never taken, and the read/modify/write it +// guards finishes in milliseconds, orders of magnitude inside the stale window. +// Takeover: removing an abandoned lock is itself arbitrated, by an exclusive create of +// `.takeover`. Only its winner may unlink the lock, and only after +// re-checking that the lock is still abandoned, so two contenders racing the +// same stale lock cannot both end up holding it. See `reclaim`. +// Release: close the handle and unlink the lock, unless it now carries a different pid +// (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). 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 +// bounded wait that never blocks indefinitely. +pub(crate) mod prefs_lock { + use std::fs::{self, OpenOptions}; + use std::io::Write; + use std::path::{Path, PathBuf}; + use std::thread::sleep; + use std::time::{Duration, SystemTime}; + + use anyhow::{anyhow, Result}; + + const POLL: Duration = Duration::from_millis(50); + const WAIT_BUDGET: Duration = Duration::from_millis(2_000); + const STALE: Duration = Duration::from_secs(30); + + /// Held for the whole read/modify/write cycle. Dropping it closes the handle and removes + /// the lock file, so a normal return, an early `?`, or a panic all release it. + pub struct Guard { + path: PathBuf, + file: Option, + } + + impl Drop for Guard { + fn drop(&mut self) { + // Close our handle first: Windows will not unlink a file that is still open. + self.file.take(); + // A lock that now carries someone else's pid has been taken over from us (only + // ever possible if we outlived the stale window); leave the successor's alone. + match holder_pid(&self.path) { + Some(pid) if pid != std::process::id() => {} + _ => { + let _ = fs::remove_file(&self.path); + } + } + } + } + + fn lock_path(target: &Path) -> PathBuf { + let stem = target.file_stem().and_then(|s| s.to_str()).unwrap_or("prefs"); + target.with_file_name(format!(".{stem}.lock")) + } + + fn body() -> String { + let at = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .map(|d| d.as_millis()) + .unwrap_or(0); + format!("{{\"pid\":{},\"at\":{}}}", std::process::id(), at) + } + + fn holder_pid(path: &Path) -> Option { + let text = fs::read_to_string(path).ok()?; + serde_json::from_str::(&text) + .ok()? + .get("pid")? + .as_u64() + .map(|pid| pid as u32) + } + + fn abandoned(path: &Path, stale: Duration) -> bool { + match fs::metadata(path).and_then(|m| m.modified()) { + Ok(mtime) => { + if SystemTime::now().duration_since(mtime).map(|age| age > stale).unwrap_or(false) { + return true; + } + } + // The file vanished between the failed create and this check; not abandoned, the + // caller simply retries the create. + Err(_) => return false, + } + match holder_pid(path) { + Some(pid) if pid != std::process::id() => !pid_alive(pid), + // Our own pid, or a body too young to be stale that we cannot parse a pid from. + _ => false, + } + } + + /// Signal-0 style liveness. A false "alive" (a reused pid) only delays recovery to the age + /// gate; a false "dead" would be the dangerous direction and is what the conservative + /// branches below avoid. + #[cfg(not(windows))] + fn pid_alive(pid: u32) -> bool { + // Declared directly rather than pulling in a crate, the same way config.rs externs + // flock. `kill(pid, 0)` probes existence without delivering a signal. + extern "C" { + fn kill(pid: i32, sig: i32) -> i32; + } + let ret = unsafe { kill(pid as i32, 0) }; + // 0 => alive; EPERM (1) => alive under another user; ESRCH => gone. + ret == 0 || std::io::Error::last_os_error().raw_os_error() == Some(1) + } + + #[cfg(windows)] + fn pid_alive(pid: u32) -> bool { + // Declared directly so this needs no extra windows-sys feature. kernel32 is always + // linked, so the symbols resolve without a `#[link]` attribute. + extern "system" { + fn OpenProcess(access: u32, inherit: i32, pid: u32) -> *mut core::ffi::c_void; + fn CloseHandle(handle: *mut core::ffi::c_void) -> i32; + fn GetLastError() -> u32; + } + const PROCESS_QUERY_LIMITED_INFORMATION: u32 = 0x1000; + const ERROR_INVALID_PARAMETER: u32 = 87; + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if !handle.is_null() { + CloseHandle(handle); + return true; + } + // Access-denied and the like mean the process exists under a token we cannot open; + // only a clearly invalid pid is proof it is gone. + GetLastError() != ERROR_INVALID_PARAMETER + } + } + + fn takeover_path(lock: &Path) -> PathBuf { + let mut name = lock.as_os_str().to_owned(); + name.push(".takeover"); + PathBuf::from(name) + } + + /// Remove an abandoned lock, but only as the one process entitled to. Two contenders that + /// both find the same stale lock would otherwise both unlink it: the first has already + /// replaced it with a lock of its own, and the second's unlink deletes that fresh one, so + /// both walk away holding the file. Windows usually hides this, because a lock a live + /// holder still has open will not unlink at all, but this crate also builds for Linux and + /// macOS, where the unlink always succeeds. + /// + /// So the removal is arbitrated by the same primitive the lock itself uses: an exclusive + /// create of `.takeover`. Its winner alone may unlink, and only after re-checking + /// staleness under that right, because a rival may have reclaimed already and now hold a + /// lock that is not abandoned and not ours to remove. The takeover file is dropped on every + /// path out, and never held across the wait; one left behind by a reclaimer that died is + /// freed by the same staleness rule as the lock. + /// + /// Returns whether the caller should retry the exclusive create at once. + fn reclaim(lock: &Path, stale: Duration) -> bool { + let takeover = takeover_path(lock); + match OpenOptions::new().write(true).create_new(true).open(&takeover) { + Ok(mut file) => { + let _ = file.write_all(body().as_bytes()); + let _ = file.flush(); + drop(file); + let reclaimed = abandoned(lock, stale) && fs::remove_file(lock).is_ok(); + let _ = fs::remove_file(&takeover); + reclaimed + } + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { + // Somebody else is reclaiming, or a dead reclaimer left this behind. The lock + // itself is never touched from here; the most this does is free the takeover + // file for a later try. + if abandoned(&takeover, stale) { + let _ = fs::remove_file(&takeover); + } + false + } + Err(_) => false, + } + } + + pub fn acquire(target: &Path) -> Result { + acquire_with(target, WAIT_BUDGET, POLL, STALE) + } + + /// The timing is a parameter only so the tests can prove the wait and the takeover without + /// sleeping for whole seconds; every caller in the app uses [`acquire`]. + pub fn acquire_with( + target: &Path, + wait: Duration, + poll: Duration, + stale: Duration, + ) -> Result { + if let Some(parent) = target.parent() { + fs::create_dir_all(parent)?; + } + let path = lock_path(target); + let deadline = SystemTime::now() + wait; + loop { + match OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(mut file) => { + // The body is advisory; even if this write fails we still hold the lock the + // exclusive create just won. + let _ = file.write_all(body().as_bytes()); + let _ = file.flush(); + return Ok(Guard { path, file: Some(file) }); + } + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { + // A successful reclaim means the holder was genuinely abandoned and this + // process won the right to remove its lock; retry the create at once. + // Anything else (a live holder, a contender already reclaiming, an unlink + // that failed) falls through to the wait. + if abandoned(&path, stale) && reclaim(&path, stale) { + continue; + } + if SystemTime::now() >= deadline { + return Err(anyhow!( + "could not lock {} within {:?}; another process is writing these \ + preferences, so nothing was changed", + path.display(), + wait + )); + } + sleep(poll); + } + Err(err) => return Err(err.into()), + } + } + } + + #[cfg(test)] + mod tests { + use super::*; + + /// Two tests starting in the same microsecond would otherwise share a directory, and + /// whichever finished first would delete the other's out from under it: SystemTime is + /// only microsecond-resolution on macOS, so the clock alone does not separate them. + static TEMP_SEQUENCE: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + + fn temp_target() -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "codeburn-prefs-lock-{}-{}-{}", + std::process::id(), + SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_nanos(), + TEMP_SEQUENCE.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + )); + fs::create_dir_all(&dir).unwrap(); + dir.join("windows-dock.json") + } + + #[test] + fn a_lock_is_taken_then_released() { + let target = temp_target(); + let lock = lock_path(&target); + { + let _guard = acquire(&target).unwrap(); + assert!(lock.exists(), "the lock file exists while it is held"); + assert_eq!(holder_pid(&lock), Some(std::process::id())); + } + assert!(!lock.exists(), "the lock file is gone once the guard drops"); + let _ = fs::remove_dir_all(target.parent().unwrap()); + } + + #[test] + fn a_stale_lock_is_taken_over() { + let target = temp_target(); + let lock = lock_path(&target); + // A live pid (our own), so only the age gate can free it. A tiny stale window plus a + // sleep past it makes the lock look abandoned without touching its mtime by hand. + fs::write(&lock, format!("{{\"pid\":{},\"at\":1}}", std::process::id())).unwrap(); + let stale = Duration::from_millis(40); + sleep(Duration::from_millis(80)); + let guard = acquire_with(&target, Duration::from_millis(500), Duration::from_millis(20), stale); + assert!(guard.is_ok(), "an abandoned lock older than the stale window is taken over"); + assert_eq!(holder_pid(&lock), Some(std::process::id())); + drop(guard); + let _ = fs::remove_dir_all(target.parent().unwrap()); + } + + #[test] + fn a_dead_holders_lock_is_taken_over_without_waiting_for_the_age_gate() { + let target = temp_target(); + let lock = lock_path(&target); + // A pid far above anything the OS hands out, so it is not a live process: on Windows + // OpenProcess reports it invalid, on Unix kill(0) reports ESRCH. With a fresh mtime + // only the pid probe, not the age gate, can free it inside the wait budget. + fs::write(&lock, "{\"pid\":2147483646,\"at\":1}").unwrap(); + let started = SystemTime::now(); + let guard = acquire_with( + &target, + Duration::from_millis(500), + Duration::from_millis(20), + Duration::from_secs(60), + ); + assert!(guard.is_ok(), "a dead holder's lock is recovered by the pid probe"); + assert!( + started.elapsed().unwrap() < Duration::from_millis(400), + "recovery did not wait out the whole budget" + ); + drop(guard); + let _ = fs::remove_dir_all(target.parent().unwrap()); + } + + /// The takeover right, from the losing side: a lock that IS abandoned still may not be + /// removed while another process holds the right to reclaim it, because that process may + /// already have replaced it with a lock of its own. + #[test] + fn a_stale_lock_is_left_alone_while_someone_else_holds_the_takeover_right() { + let target = temp_target(); + let lock = lock_path(&target); + fs::write(&lock, format!("{{\"pid\":{},\"at\":1}}", std::process::id())).unwrap(); + let stale = Duration::from_millis(200); + sleep(Duration::from_millis(220)); + // A takeover file with our own live pid and a fresh mtime: a reclaim in progress. + // The wait below is kept well inside the stale window, so it is the takeover right + // and not the takeover file's own age that decides this. + fs::write(takeover_path(&lock), format!("{{\"pid\":{},\"at\":1}}", std::process::id())) + .unwrap(); + + let blocked = acquire_with(&target, Duration::from_millis(60), Duration::from_millis(10), stale); + assert!(blocked.is_err(), "the stale lock is not stolen out from under the reclaimer"); + assert!(lock.exists(), "and it is still there for the reclaimer to replace"); + + // Once the reclaimer is gone the same lock is taken over as it always was. + fs::remove_file(takeover_path(&lock)).unwrap(); + let guard = acquire_with(&target, Duration::from_millis(500), Duration::from_millis(10), stale); + assert!(guard.is_ok(), "with nobody reclaiming, the abandoned lock is taken over"); + drop(guard); + let _ = fs::remove_dir_all(target.parent().unwrap()); + } + + #[test] + fn a_live_lock_is_waited_for_then_fails_loudly() { + let target = temp_target(); + // Our own pid reads as alive and a fresh mtime is inside a long window, so the lock + // is never abandoned and the contender must give up rather than write behind it. + let _held = acquire(&target).unwrap(); + let started = SystemTime::now(); + let err = acquire_with( + &target, + Duration::from_millis(200), + Duration::from_millis(20), + Duration::from_secs(60), + ); + assert!(err.is_err(), "a live lock is not stolen; the contender fails"); + assert!(started.elapsed().unwrap() >= Duration::from_millis(180), "it waited the budget"); + drop(_held); + let _ = fs::remove_dir_all(target.parent().unwrap()); + } + } +} + +/// 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 { @@ -62,6 +469,10 @@ pub fn read() -> Map { /// key, so the page can reset a preference to its default without knowing what that default /// is. pub fn patch(values: Map) -> Result> { + let path = settings_path(); + // Serialize the whole read/modify/write against the desktop app, which owns this file too. + // The guard is held until the rename lands. See `prefs_lock` for the shared protocol. + let _lock = prefs_lock::acquire(&path)?; let mut stored = read(); for (key, value) in values { if value.is_null() { @@ -70,13 +481,12 @@ pub fn patch(values: Map) -> Result> { stored.insert(key, value); } } - let path = settings_path(); if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; } 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) }