fix(ai-hist): survive a truncated .sync-state.json instead of wedging sync - #44
Conversation
… sync When the disk filled up, save_sync_state's plain fs::write truncated .sync-state.json and then failed with ENOSPC, leaving a 0-byte file. load_sync_state fed that to serde_json and propagated the error, so every later run aborted with "EOF while parsing a value at line 1 column 0" -- before doing any work. The launchd sync agent failed this way every 60s for three days; recovery required knowing to delete the file by hand. Sync state is an optimization, not a source of truth, so losing it should cost a re-scan rather than stop sync entirely: - load_sync_state falls back to empty state with a warning on an empty, corrupt, or unreadable file. Re-scanning is safe because every insert path upserts (ON CONFLICT ... DO UPDATE), so no rows are duplicated. - save_sync_state writes a temp file and renames it into place, so an interrupted or out-of-space write leaves the previous state intact instead of a truncated one. Both paths carry context so a future failure names the file rather than surfacing a bare serde error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSync-state loading now treats unreadable, empty, and corrupt files as empty state with diagnostics. Saving uses unique temporary files and atomic renames. Tests cover recovery, round-tripping, cleanup, and concurrent writers. ChangesSync-state persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Writer as save_sync_state
participant TempFile as unique temporary file
participant StateFile as sync-state file
Writer->>TempFile: write serialized state
TempFile->>StateFile: atomically rename
StateFile-->>Writer: publish complete state
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let tmp_path = path.with_extension("json.tmp"); | ||
| fs::write(&tmp_path, serde_json::to_string_pretty(state)? + "\n") | ||
| .with_context(|| format!("writing sync state to {}", tmp_path.display()))?; | ||
| fs::rename(&tmp_path, path) | ||
| .with_context(|| format!("replacing sync state at {}", path.display()))?; |
There was a problem hiding this comment.
🟡 Two syncs running at once can make one of them fail outright
The saved progress file is staged through a single fixed side file that every run shares (path.with_extension("json.tmp") at crates/ai-hist/src/lib.rs:2918-2922), so when the scheduled run and a manual run overlap, one of them finds the side file already taken away and stops with an error.
Impact: Overlapping syncs can abort with a confusing "replacing sync state" failure, and the saved progress can end up as a mix of two runs' data.
Race between the shared temp path and rename
sync_basic (crates/ai-hist/src/lib.rs:1667-1700) is invoked both by the 60s launchd agent and by manual ai-hist sync. Both processes compute the same tmp_path (.sync-state.json.tmp). Sequence: A writes tmp, B writes tmp (truncating/interleaving A's bytes), A renames tmp→state, B's fs::rename then fails with ENOENT and, being propagated with ?, aborts the whole sync command — the exact class of failure this PR set out to remove. Even if both renames succeed, the renamed content may be a blend of the two writers.
Using a per-process unique temp name (e.g. including std::process::id() or a random suffix) makes each writer independent; the last rename simply wins.
| let tmp_path = path.with_extension("json.tmp"); | |
| fs::write(&tmp_path, serde_json::to_string_pretty(state)? + "\n") | |
| .with_context(|| format!("writing sync state to {}", tmp_path.display()))?; | |
| fs::rename(&tmp_path, path) | |
| .with_context(|| format!("replacing sync state at {}", path.display()))?; | |
| let tmp_path = path.with_extension(format!("json.tmp.{}", std::process::id())); | |
| fs::write(&tmp_path, serde_json::to_string_pretty(state)? + "\n") | |
| .with_context(|| format!("writing sync state to {}", tmp_path.display()))?; | |
| fs::rename(&tmp_path, path) | |
| .with_context(|| format!("replacing sync state at {}", path.display()))?; |
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 5c70f07902
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs::create_dir_all(parent)?; | ||
| } | ||
| fs::write(path, serde_json::to_string_pretty(state)? + "\n")?; | ||
| let tmp_path = path.with_extension("json.tmp"); |
There was a problem hiding this comment.
When two syncs overlap—for example, a minute-based cron run is still processing while the next run or a manual ai-hist sync starts—both writers use the same .sync-state.json.tmp. After both calls to fs::write succeed, the first rename removes that shared source path, so the second rename fails with No such file or directory and reports the whole sync as failed after its database work has already completed. Use a per-invocation temporary file or serialize state updates before renaming.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ai-hist/src/lib.rs`:
- Around line 2918-2922: The sync-state save logic around the temporary path
must avoid a shared filename during concurrent saves. Replace the deterministic
with_extension temporary path with a uniquely named file created exclusively in
the destination directory, ensure cleanup occurs when writing or renaming fails,
and add a test exercising concurrent saves without lost or missing state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88be77b7-376e-491b-872a-1b31f362afc0
📒 Files selected for processing (1)
crates/ai-hist/src/lib.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review feedback: the temp path was derived from the destination alone, so every writer shared `.sync-state.json.tmp`. sync_basic runs from the CLI, from watch_loop, and from the in-process napi binding, so saves overlap in practice. Two writers would open the same temp path, interleave their writes, and the slower rename would then fail with ENOENT on a path the faster writer already moved -- aborting a sync after its database work had already completed, which is the class of failure this change exists to prevent. Name the temp file per writer (pid + process-local counter) so each save is independent and the last rename simply wins, and remove the temp file when a save fails so a failure doesn't leave one behind. concurrent_saves_never_publish_a_torn_state_file drives 8 concurrent savers with different payload sizes and asserts the published file is one writer's state, intact. It fails reliably against the shared-path version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Problem
ai-hist synchad been failing on every run for three days with:~/.local/share/ai-hist/.sync-state.jsonwas 0 bytes.serde_json::from_str("")produces exactly that message, andload_sync_statepropagated it — abortingsync_basicbefore any work happened.How it got truncated
The disk filled up. From
/tmp/ai-hist-sync.err:save_sync_stateused a plainfs::write, which truncates first, then writes. It was interrupted byENOSPCmid-write, leaving an empty file — and the loader then treated that empty file as fatal on every subsequent run.This also silently killed the background
com.ai-hist.synclaunchd agent, which runs every 60s: it exited 1 each time for three days. Recovering required knowing to delete the file by hand.Fix
Sync state is an optimization, not a source of truth. Losing it should cost a re-scan, never stop sync.
load_sync_statefalls back to empty state with a warning on an empty, corrupt, or unreadable file. Re-scanning is safe: every insert path upserts (ON CONFLICT ... DO UPDATE), so no rows are duplicated.save_sync_statewrites to a temp file thenfs::renames it into place. An interrupted or out-of-space write now leaves the previous state intact rather than a truncated file..with_context()so a future failure names the file instead of surfacing a bare serde error.Testing
New test
load_sync_state_recovers_from_empty_or_corrupt_filecovers missing, empty, corrupt, and valid-round-trip states, and asserts no.tmpfile is left behind. It fails against the old code. Full workspace suite passes (58 tests).Verified end-to-end against a scratch DB with a deliberately emptied state file:
Note
This makes sync resilient to the symptom. The underlying trigger was a full disk, which is an operational issue, not addressed here.
🤖 Generated with Claude Code