fix(windows): a Codex login race and cross-process settings writes - #1248
Merged
Conversation
The refresh reread auth.json before writing the rotated tokens but only checked it was still ChatGPT mode, not that it was still the same login. A `codex login` account switch, or a second CodeBurn process refreshing the same credential, landing while our refresh was in flight let the late response splice one account's tokens into another's document: a new account id over an old refresh token, or an older refresh token over a newer one. Either signs the user out or binds them to the wrong account. The login the grant belongs to is now captured before the grant and compared against the reread document on account id, refresh token and id-token subject. A demonstrated mismatch discards the write and says so. An unreadable or key-sparse file is not a mismatch and still writes, so a rotated token is never dropped. Both the CLI and the Electron copy.
The desktop app and the tray app both read, merge and rename the same preference files. The atomic rename stopped a torn file but not a lost update: both read state A, each changed a different field, and the last rename erased the other's change, so dock placement, enabled state, provider choice or scale silently reverted. One cross-process lock now covers the whole read-modify-write cycle, designed once and implemented in both Rust and Node against the same protocol: a sibling .lock file taken by exclusive create, carrying the holder's pid and timestamp, taken over only when abandoned by a dead pid or a stale mtime, and failing loudly rather than writing behind a live holder. It follows the CLI's existing cache-refresh lock. All three sites use it, including the dock's enabled, provider and placement writes. A test drives real Node and Rust processes at the same file and fails without the lock.
The new lock test starts a real Node process alongside the Rust one so both write the same settings file at once, which is the only way to prove a cross-process lock. The Node half is TypeScript run through tsx, and tsx is a repo-root devDependency, while this job only runs `npm ci` inside windows/. The worker exited 1 with "Cannot find package 'tsx'" and the test failed on both legs. The test's own skip path covers a node binary that will not spawn, not a worker that starts and dies, and widening it would let the guard disappear in silence, which is worse than a red job. Install the dependency instead.
writeDockEnabled still did its own read, mutate and atomic write of windows-dock.json, outside the lock the rest of the module takes. It is the Node side of the dock's enabled switch, reached from the desktop app's Sidebar toggle, so a toggle there could still land between the tray app's read and its rename and erase whatever it had just stored, which in practice is the rail's placement. It now merges through patchTrayFile, which takes the lock for the whole cycle; enabled is already an accepted dock key, so nothing is dropped on the way. Stale-lock recovery could hand out two owners at once. Both sides did "if abandoned, unlink and retry", and nothing stood between the check and the unlink: contender A could already have removed the stale lock and created a fresh one of its own, and B's unlink then deleted A's fresh lock, leaving both of them holding the file. Windows' share mode hides this most of the time, because a lock a live holder still has open will not unlink at all, but the same protocol runs on Linux and macOS, where the unlink always succeeds. Removing an abandoned lock is now itself arbitrated, by the same exclusive create the lock uses, on a sibling <lock>.takeover file. Only its winner may unlink, and only after re-checking staleness under that right, so a rival that reclaimed first keeps the lock it now holds. A takeover file is dropped on every path out, never held across the wait, and one left behind by a reclaimer that died is freed by the same staleness rule as the lock itself. The cross-process test asserted only each side's final value, so it noticed a lost update only when the last writes happened to collide: on a deliberately broken lock it passed about one run in three, which is weak evidence for the property it exists to prove. Each side now writes a counter that only rises, and a reader thread watches the file for the whole run. A separate reader is the point: neither writer can see its own counter rolled back, because in the unlocked cycle each carries the other side's value forward itself, so what it reads back is never older than what it last read. With the lock both counters only ever rise in the file; without it one falls. It now fails on every unlocked run. Also: a placement that cannot be saved is logged instead of discarded silently, since losing the lock under contention is a new way for the rail to revert at the next launch; and the Codex sameLogin comment now states outright that a replacement document carrying none of the three identifying fields reads as the same login and is written over, which is the deliberate side to err on.
The Windows CI leg failed on the cross-process test with EPERM from renameSync. It is not a test artifact. Windows refuses a rename over a file another process holds open rather than waiting, with ERROR_ACCESS_DENIED or ERROR_SHARING_VIOLATION, and reads of these two files are deliberately lock-free on both sides. So any tray or app read that overlaps a writer's replace fails that write, and since this branch made writers report a failure instead of swallowing it, the outcome is exactly the lost update the lock exists to prevent, reached by another route. The test's reader thread only made it visible by reading the way both apps really do, continuously and without the lock. The replace is now retried on that error class, ten attempts about 20ms apart, on both sides: replace_file in settings.rs, used by settings patch and by the dock's write_state_at, and renameOnto behind writeFileAtomic in tray-settings.ts. The lock is already held at that point, so the wait can only ever be on a reader, never on another writer, and a read of one of these files is microseconds of open file. Once the budget is out the original error is returned unchanged and the caller cleans up as before. On POSIX, where a rename over an open file cannot fail this way, it compiles out in Rust and short-circuits on process.platform in Node. Reads stay lock-free, which is the right trade and the reason the files can be read at all without coordination; both read paths now say what it costs and where that cost is absorbed. The reader thread stays as it is, and is now the regression test for the retry as well: it is what puts a replace under a continuous reader. The retry itself is covered on the Node side by faking the platform and the rename, since a rename that fails this way cannot be produced on a Mac: a reader waited out and the write landing, the budget running out with the old file still whole, and an error a reader cannot cause not being waited out at all. The Rust half is compiled out off Windows, so CI is its only proof.
…passes on Windows The rename retry landed, but its test kept the watcher reading in a tight loop, and on real Windows that lost 8 runs in 12: a reader holding the file open every instant is a race no bounded retry can win, because an open handle blocks a rename until it closes. That is not a reader either app has. The watcher now polls at 1ms, which still samples the file thousands of times and still catches a lost update, while leaving the writers room to rename. The locked path passes 15 of 15 on Windows, and the unlocked path still reports a rolled-back write. Also restores the lineage case the review asked for: a refresh where the account id and refresh token are unchanged and only the id_token subject moves, so the subject comparison is what discards the write. Both copies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two concurrency fixes from the Windows review, split from the Capacity Dock work so they can be read on their own.
P1 — a Codex refresh can be written over a login that changed under it
A quota read refreshes the Codex token and writes the rotated one back. If the user signs in as someone else, or signs out and in again, while that request is in flight, the response belongs to the old login and overwrites the new credentials.
The refresh now checks lineage before it writes: the account id, the refresh token it started from, and the id-token subject. If any of them moved, the response is discarded rather than persisted. Fixed in both copies of the reader (
src/quota/codex.tsand the desktop app's).P2 — settings writes race across processes
The tray app (Rust) and the desktop app (Node) both write the same preference files. Read-modify-write from two processes loses updates, and a torn read parses as
{}, which silently collapses every dock setting to its default.One cross-process lock now covers all three write sites on both sides, including the dock's enabled, provider and placement writes.
Verification
The cross-process test was proven to fail without the lock, not just to pass with it.
Not covered here, and why
Two acceptance passes need hardware this branch was written on and could not honestly cover: mixed-DPI and multi-monitor behaviour, and a current Store build (that machine has a single 1600x900 display at 96 DPI and a stale 0.9.21 Store package). The NSIS uninstall-and-upgrade pass is possible there and is being run separately.