fix(persistence): write state.json and the search index atomically (#854) - #855
Merged
Merged
Conversation
…encent#854) saveState, saveStateForScope and buildIndex wrote in place: open + truncate + write. A crash, kill, ENOSPC or power loss mid-write leaves truncated JSON, and the readers answer null for what they cannot parse: - state.json: loadStateForScope falls back to StateSchema.parse({}) — lastPullRev and every per-checkout pushBaseRevs entry are gone without a word, so the next push compares against a stale base. That is the stale-base overwrite class the worktree fixes (Tencent#827) closed. - search-index.json: loadIndex returns null, so recall silently loses the whole corpus until the next rebuild, and the shrink guard loses its baseline. The repo already writes config.yaml (Tencent#831) and the votes file atomically for exactly this reason, and writeJsonAtomic exists: temp file + rename, preserving the target's permission bits. Move the three writers to it. No reader changes; a successful write behaves as before.
CI failed recall-rebuild-roots: the 'cannot be written' test chmod'd the index file 0o444, which fails the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling (the directory is writable) and renames over the target, and rename needs only the directory's permission. Same for the EISDIR test: renaming a file over a directory is EISDIR on POSIX but EPERM on Windows. Fail the fse.writeFile calls at the index path itself — the staged temp file included — so the injection works for both writers on every platform. The root-skip guard goes with the chmod it existed for.
…, not the file mode CI failed the 'cannot be recorded' case: it chmod'd state.json 0o444, which stops the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling and renames, and rename needs only the directory's permission, so the push completed normally and exited 0. (The e2e config retries once; the retry then failed teammatePublishes' git commit with 'nothing to commit', the run's second error.) The CLI runs as a subprocess, so the write call cannot be spied on the way state-atomic-save does it. Inject the failure from outside instead: a preload hook (NODE_OPTIONS --require) fails every fs.writeFile targeting the staged temp of this state file inside the CLI process — the same seam as the unit tests, on every platform.
jeff-r2026
self-requested a review
September 27, 2026 10:17
|
No findings. The PR description includes sufficient testing for the runtime change, including a representative real-CLI verification. The record names commit |
jeff-r2026
approved these changes
Sep 27, 2026
5 tasks
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
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.
Closes #854.
Problem
saveState,saveStateForScopeand thebuildIndexsave write in place (writeJson= open + truncate + write). A crash, kill, ENOSPC or power loss mid-write leaves truncated JSON, and the readers answer null for what they cannot parse — the loss is silent, which is what makes the state.json case dangerous:loadStateForScopefalls back toStateSchema.parse({}):lastPullRevand every per-checkoutpushBaseRevsentry are gone without a word, so the next push compares against a stale base. That is the stale-base overwrite class the worktree fixes (fix(push): stop offering a teammate's update back as a local edit (#823) #827, tracked in [bug] Follow-ups to the worktree fixes: import --from-mr never shares its learning, and push can still revert a teammate's update #823) closed.loadIndexreturns null: recall silently loses the whole corpus until the next rebuild, and the shrink guard loses its baseline.The repo already treats this exact failure mode as a bug: #831 made
config.yamlsaves atomic (Issue #823 item 14 — "a command reading it mid-save saw an empty file"), andsaveUserVotesdocuments it for votes ("A torn plain overwrite would leave truncated YAML … silently wiping every doc's counts, all pending deltas, and the whole upvote ledger").writeJsonAtomic(src/utils/fs.ts:161) already exists — temp file + rename, preserving the target's permission bits — these three writers just predate it.Fix
Move the three writers to
writeJsonAtomic:saveState/saveStateForScope(src/config.ts)buildIndex(src/utils/search-index.ts)No reader changes; a successful write behaves as before, including file modes (existing files keep theirs; new state files get 0600, the same default
writeJsonAtomicapplies to the local-agent config today).Tests
New
src/__tests__/state-atomic-save.test.ts, mirroring the #831 method: interceptfse.writeFileto truncate the target, run a concurrent reader, then complete or fail the write with ENOSPC.saveState/saveStateForScope: the concurrentloadStateForScopesees the old complete state (lastPullRevintact, neverparse({})), and a failed write leaves the previous state on disk.buildIndex: a concurrentloadIndexmid-rebuild sees the previous index, never null.All 5 tests fail on
main(the concurrent reader observes exactly the torn state:expected null to be 'abc1234',expected undefined to deeply equal ['a.md']) and pass on this branch.npx tsc --noEmitclean;npm run lint0 warnings under--deny-warnings.Real-CLI verification (
607de7c)npm run build, thennode dist/index.jsagainst a sandboxHOMEand a local team-repo fixture with alearnings/note — the writers behave end-to-end and leave no temp residue:The atomicity itself is what the unit tests above prove (a torn write cannot be reproduced in a passing CLI run by construction); this record shows the two writers work unchanged on the happy path.