fix(state): serialize concurrent updateState/saveState with file lock and atomic write#460
Conversation
… and atomic write The per-workspace job index (state.json) was persisted via an unguarded read-modify-write (loadState -> mutate -> saveState) with no lock, no atomic rename, and no compare-and-swap. When two companion invocations registered or updated jobs on the same workspace root concurrently, the last writer won and every other writer's job entry was silently dropped from the index. Worse, saveState treated 'job missing from the retained list' as 'job pruned' and deleted the losing jobs' .json payload and .log files — so the lost work wasn't just unindexed, its artifacts were destroyed. Fix: - Add withStateLock() using O_EXCL lock-file creation with PID-based stale-lock detection and a 10s timeout. - Wrap updateState() in the lock so the entire read-modify-write is atomic across processes. The locked loadState now sees the latest committed state instead of a stale snapshot. - Split saveState into saveStateLocked (internal, caller holds lock) and saveState (exported, acquires lock automatically). - Write state.json atomically (temp file + fs.renameSync) so concurrent readers never see a partially-written file. - Migrate cleanupSessionJobs() in session-lifecycle-hook.mjs from the stale loadState+saveState pattern to updateState, closing the same race class for the SessionEnd cleanup path. Closes openai#458
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a20b534478
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| const pid = parseInt(fs.readFileSync(lockFile, "utf8").trim(), 10); | ||
| if (pid && !isProcessAlive(pid)) { | ||
| fs.unlinkSync(lockFile); |
There was a problem hiding this comment.
When two companion processes recover the same stale state.lock, both can read the dead PID; after one process unlinks it and acquires a fresh lock, the other process can still run this unlinkSync based on the stale read and delete the fresh holder's lock. That lets both processes enter updateState/saveState concurrently, reintroducing the lost-update/artifact-deletion race this lock is meant to prevent. Re-read and compare the lock contents or use an owner token before removing a stale lock.
Useful? React with 👍 / 👎.
| // Lock exists — check if the owner is still alive. | ||
| try { | ||
| const pid = parseInt(fs.readFileSync(lockFile, "utf8").trim(), 10); | ||
| if (pid && !isProcessAlive(pid)) { |
There was a problem hiding this comment.
If the process crashes after creating state.lock but before writing the PID, or the lock file is truncated, parseInt returns NaN and this condition never removes the lock. In that state every later updateState/saveState call spins until the 10s timeout and fails until someone manually deletes the file, so stale-lock recovery does not cover a crash at one of the new lock-creation points. Treat unreadable, empty, or non-positive PID contents as stale.
Useful? React with 👍 / 👎.
Address two P2 edge cases flagged by the Codex review bot: 1. Recheck lock content before unlinking: two processes could read the same dead PID; after one unlinks and creates a fresh lock, the other might unlink the fresh holder's lock based on its stale read. Now re-reads the file and only unlinks if the content is unchanged. 2. Treat corrupt/empty lock files as stale: a process that crashes after creating the lock file but before writing its PID leaves an empty or non-numeric file. parseInt returns NaN, which was falsy under the old 'pid &&' guard, so the lock was never reclaimed. Now uses '!pid || !isProcessAlive(pid)' to cover NaN, 0, and dead PIDs alike. Added a test for corrupt (empty + non-numeric) lock file recovery.
1 participant
Summary
The per-workspace job index (
state.json) was persisted via an unguarded read-modify-write (loadState→ mutate →saveState) with no lock, no atomic rename, and no compare-and-swap. When two companion invocations registered or updated jobs on the same workspace root concurrently, the last writer won and every other writer's job entry was silently dropped from the index. Worse,saveStatetreated "job missing from the retained list" as "job pruned" and deleted the losing jobs'.jsonpayload and.logfiles — so the lost work wasn't just unindexed, its artifacts were destroyed.Changes
withStateLock):O_EXCLlock-file creation with PID-based stale-lock detection (reclaims locks from crashed processes) and a 10 s timeout. Serializes concurrentupdateState/saveStatecalls across processes.updateState: the entireloadState → mutate → saveStatecycle now runs inside the lock, so each caller sees the latest committed state instead of a stale snapshot.saveStatesplit:saveStateLocked(internal, caller holds lock) +saveState(exported, acquires lock automatically).state.jsonis written to a temp file thenfs.renameSync'd, so concurrent readers never see a partially-written file.session-lifecycle-hook.mjs: migratedcleanupSessionJobsfrom the staleloadState+saveStatepattern toupdateState, closing the same race class for the SessionEnd cleanup path.Reproduction
Verified the bug is reproducible on
mainbefore the fix: 5 concurrentupsertJobcalls (via real child processes) left only 1 job in the index — the other 4 were silently dropped and their.jsonfiles deleted.After the fix: all 5 jobs and their artifacts are preserved.
Testing
node --test tests/state.test.mjs tests/state-concurrency.test.mjs→ 8/8 pass (3 existing + 5 new):upsertJobfrom 5 child processes — all jobs and.jsonfiles preservedsaveStatewith stale snapshot — no temp files remain (atomic write)state.jsonis valid JSON, no temp files leftMAX_JOBS(50) with locking enabledExisting
state.test.mjstests (3) pass unchanged — no regression.Relationship to #439
#439 ships "safe, self-contained" concurrency hardening for the same file (
state.mjs): it preventssaveStatefrom deleting active (queued/running) jobs' files and adds atomic writes. Its description explicitly defers the state-transaction lock:This PR implements that deferred state-transaction lock. The two approaches are complementary: #439 prevents artifact deletion as a mitigation, while this PR prevents the stale-snapshot race at its root. If #439 merges first, this PR will need a rebase (both touch
saveState).Closes #458