mirror of
https://github.com/AgentSeal/codeburn.git
synced 2026-10-21 05:23:19 +00:00
Merge pull request #1248 from getagentseal/fix/windows-concurrency-review
fix(windows): a Codex login race and cross-process settings writes
This commit is contained in:
Resham Joshi
GitHub
commit
e90036b106
12 changed files with 1459 additions and 54 deletions
9
.github/workflows/windows-menubar-ci.yml
vendored
9
.github/workflows/windows-menubar-ci.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> = {}
|
||||
try {
|
||||
const parsed: unknown = JSON.parse(readFileSync(path, 'utf8'))
|
||||
if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) prefs = parsed as Record<string, unknown>
|
||||
} 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 ----------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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]) {
|
||||
|
|
|
|||
|
|
@ -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 = {
|
||||
|
|
|
|||
|
|
@ -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') })
|
||||
|
||||
|
|
|
|||
112
app/electron/tray-settings-lock.test.ts
Normal file
112
app/electron/tray-settings-lock.test.ts
Normal file
|
|
@ -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()
|
||||
})
|
||||
})
|
||||
|
|
@ -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<string, unknown> {
|
||||
try {
|
||||
const parsed: unknown = JSON.parse(readFileSync(filePath(file, home), 'utf8').replace(/^/, ''))
|
||||
|
|
@ -61,15 +101,178 @@ export function readTrayFile(file: TraySettingsFile, home = homedir()): Record<s
|
|||
}
|
||||
}
|
||||
|
||||
// Cross-process preference lock -------------------------------------------------------------
|
||||
//
|
||||
// windows-settings.json and windows-dock.json are each owned by two processes at once: this
|
||||
// desktop app and the Windows tray 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, so a dock placement, an enabled switch, a provider
|
||||
// choice or a scale silently reverts. This lock serializes the whole cycle so that never
|
||||
// happens. The tray app runs the same protocol from Rust; the single write-up lives in
|
||||
// windows/src-tauri/src/settings.rs (`prefs_lock`). The essentials, so both sides stay in step:
|
||||
//
|
||||
// Lock file: sibling of the target named `.<stem>.lock` (windows-dock.json ->
|
||||
// `.windows-dock.lock`).
|
||||
// Body: one line of JSON, `{"pid":<os pid>,"at":<unix ms>}`.
|
||||
// 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
|
||||
// `<lock>.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 `<lock>.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<string, unknown>,
|
||||
home = homedir(),
|
||||
): Record<string, unknown> {
|
||||
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 -----------------------------------------------------------------
|
||||
|
|
|
|||
56
app/scripts/tray-settings-xproc-worker.ts
Normal file
56
app/scripts/tray-settings-xproc-worker.ts
Normal file
|
|
@ -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: <home> <key> <iterations> <barriersDir>
|
||||
// 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 <home> <key> <iterations> <barriersDir>')
|
||||
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'), '')
|
||||
|
|
@ -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 = {
|
||||
|
|
|
|||
|
|
@ -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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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<void>(resolve => { grantStarted = resolve })
|
||||
let releaseGrant = (): void => {}
|
||||
const heldGrant = new Promise<void>(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]) {
|
||||
|
|
|
|||
|
|
@ -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<String, serde_json::Value> {
|
||||
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<String, serde_json::Value> {
|
||||
fs::read(path)
|
||||
.ok()
|
||||
.and_then(|bytes| serde_json::from_slice::<serde_json::Value>(&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<String, serde_json::Value>) -> Result<()> {
|
||||
let path = state_path();
|
||||
fn write_state_at(path: &Path, state: &serde_json::Map<String, serde_json::Value>) -> Result<()> {
|
||||
if let Some(parent) = path.parent() {
|
||||
fs::create_dir_all(parent)?;
|
||||
}
|
||||
|
|
@ -546,7 +553,9 @@ fn write_state(state: &serde_json::Map<String, serde_json::Value>) -> 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<String, serde_json::Value>) -> 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<String, serde_json::Value>),
|
||||
) -> Result<serde_json::Map<String, serde_json::Value>> {
|
||||
patch_state_at(&state_path(), mutate)
|
||||
}
|
||||
|
||||
fn patch_state_at(
|
||||
path: &Path,
|
||||
mutate: impl FnOnce(&mut serde_json::Map<String, serde_json::Value>),
|
||||
) -> Result<serde_json::Map<String, serde_json::Value>> {
|
||||
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<String, serde_json::Value> {
|
|||
pub fn patch_prefs(
|
||||
values: serde_json::Map<String, serde_json::Value>,
|
||||
) -> Result<serde_json::Map<String, serde_json::Value>> {
|
||||
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<String> = 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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Option<String>> = 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 `.<stem>.lock` (windows-dock.json ->
|
||||
// `.windows-dock.lock`), the same `.config.lock` shape config.rs uses.
|
||||
// Body: one line of JSON, `{"pid":<os pid>,"at":<unix ms>}`.
|
||||
// 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
|
||||
// `<lock>.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<fs::File>,
|
||||
}
|
||||
|
||||
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<u32> {
|
||||
let text = fs::read_to_string(path).ok()?;
|
||||
serde_json::from_str::<serde_json::Value>(&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 `<lock>.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<Guard> {
|
||||
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<Guard> {
|
||||
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<String, Value> {
|
|||
/// key, so the page can reset a preference to its default without knowing what that default
|
||||
/// is.
|
||||
pub fn patch(values: Map<String, Value>) -> Result<Map<String, Value>> {
|
||||
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<String, Value>) -> Result<Map<String, Value>> {
|
|||
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)
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue