fix(ai-hist): stop an interrupted sync from wedging into a permanent re-scan loop - #45
Conversation
sync_basic wrote .sync-state.json once, at the very end of the run. Any death before that point -- killed process, locked database, full disk -- discarded every source that had already finished, so the next run re-scanned the same files from scratch. One interrupted run turns into a loop that never persists anything: observed locally as ~148MB of Claude history being re-scanned every cycle, indefinitely, because the run consistently died after the claude source but before the final save. Save after each source that advances state, so a cursor becomes durable as soon as its source completes. An interrupted run now costs only the source it died in, not the whole run. Checkpoints are deliberately non-fatal. The rows are already committed, so a run that did real work should not be reported as failed because a bookkeeping write did not land; the next checkpoint retries, and save_sync_state leaves the prior state intact on failure, so the worst case is a re-scan rather than corruption. This does not explain why the process dies mid-run -- that is still unidentified -- but it stops a single mid-run death from erasing all progress and wedging sync into a permanent re-scan loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sync died mid-run with "database is locked" (SQLITE_BUSY). The database is shared by several long-lived writers -- the launchd sync agent, the MCP server, relay brokers -- and WAL admits one writer at a time. rusqlite already waits 5s by default, but a lock here can outlast that: the database is ~3GB with a large WAL, and recovery or checkpointing holds it well past five seconds. Sync is the longest-running writer, so it is the one most often waiting when that happens. Raise the busy timeout on the shared connection to 30s. sync_opencode_db already sets its own timeout for the same reason; this applies the same treatment to the main connection, which had none of its own. This is a mitigation for long lock holds, not a fix for their cause: a writer holding the lock beyond 30s will still fail. Paired with per-source checkpointing, a failure now costs one source rather than the whole run. 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. |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![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: 6f88066bb2
ℹ️ 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".
| /// leaves the previous state intact when a write fails, so the worst case is a | ||
| /// re-scan rather than corruption. | ||
| fn checkpoint_sync_state(path: &Path, state: &Map<String, Value>) { | ||
| if let Err(err) = save_sync_state(path, state) { |
There was a problem hiding this comment.
When two sync_basic runs overlap, each loads an independent copy of the entire state map, and this checkpoint atomically replaces the whole file without merging or serialization. A lagging run that has only completed Claude can therefore overwrite newer Codex/Cursor/etc. cursors published by a run that finished those sources; if the lagging run then fails, the next run rescans that work, recreating the loop this change is intended to prevent. Overlap is possible through the unlocked CLI, cron, and in-process NAPI entry points, so sync runs should be serialized or checkpoints should update only the source state they just completed.
Useful? React with 👍 / 👎.
| checkpoint_sync_state(&state_path, &state); | ||
| sync_claude_session_metadata(conn, &mut state, &home.join(".claude/projects"))?; | ||
| checkpoint_sync_state(&state_path, &state); |
There was a problem hiding this comment.
During a steady-state watch or the default 60-second service run, sources commonly report “up to date” without changing state, but these unconditional calls still serialize and rename the complete .sync-state.json seven times per cycle. Because the map contains per-file entries for rollouts, transcripts, and trajectories, large installations can incur seven full-file rewrites every minute instead of the previous single rewrite; checkpoint only when the just-completed source actually changed the state.
Useful? React with 👍 / 👎.
![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 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…e file Review feedback (raised independently by two reviewers). Checkpoints wrote this run's entire in-memory state map wholesale. Sync runs are not serialized -- the CLI, the background service, and the in-process napi entry point can all overlap -- and each holds its own copy of that map, so a slow run that had only finished claude could replace a faster run's newer codex and cursor positions with its own stale ones. The next run then rescanned work that was already done, which is the re-scan loop checkpointing exists to prevent. Merge per key against what is on disk instead. Sources this run did not touch are preserved, and because cursors are monotonic byte offsets, an offset that has not advanced past the stored one is stale and is dropped -- so a checkpoint can never rewind another run's progress. Also skip the write entirely when disk already reflects our state. In steady state every source reports "up to date" and checkpoints an unchanged map, so the previous code rewrote the full file once per source, seven times a minute, for no change. Two tests: a_slow_run_cannot_rewind_or_clobber_a_faster_runs_cursors (fails against the wholesale write, rewinding the cursor to 400) and an_unchanged_source_does_not_rewrite_the_state_file. This narrows the race rather than eliminating it -- the read-modify-write is still not atomic across processes -- but combined with monotonic cursors it converges safely. Full serialization is tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Follow-up to #44. That PR stopped a truncated
.sync-state.jsonfrom aborting sync. This one stops an interrupted sync from throwing away everything it had done.Problem
sync_basicwrote.sync-state.jsonexactly once, at the very end of the run. Any death before that point discarded every source that had already completed, so the next run re-scanned the same files from scratch — and died in the same place. One interrupted run becomes a loop that never persists anything.Observed on a live ~3GB database: ~148MB of Claude history re-scanned every cycle, indefinitely.
Changes
1. Checkpoint after each source, and within a source. State is saved as soon as a source completes, and JSONL ingestion now runs in 2000-line chunks that checkpoint their byte offset per commit. An interrupted run resumes mid-file rather than restarting. Chunking also takes the write lock once per chunk instead of once per row, and avoids holding it for minutes on a large backlog — which would starve the other writers.
Checkpoints are deliberately non-fatal: the rows are already committed, so a run that did real work shouldn't be reported as failed because a bookkeeping write didn't land.
2. Merge checkpoints rather than overwriting. Sync runs aren't serialized (CLI, background service, and in-process napi can overlap), so writing the whole in-memory map wholesale let a slow run replace a faster run's newer cursors. Checkpoints now merge per key, and monotonic cursors mean a non-advancing offset is dropped — a checkpoint can never rewind another run's progress. Also skips the write when disk is already current, instead of rewriting the full file once per source every minute.
3. Raise the busy timeout from rusqlite's 5s default to 30s. Scope note: this was written believing it addressed the outage that motivated this PR. It does not. That outage was caused by a process holding the write lock permanently (see below), which no timeout survives. This change only helps ordinary transient overlap between active writers, and is included on those narrower merits. Bounded retry with backoff would be the better mechanism — tracked in #49.
Scope — what this does not do
None of this explains why a writer holds the lock. The outage that prompted the work was an
agent-relayprocess SIGSTOP'd mid-transaction: a stopped process never runs again on its own, so it holds the single WAL write lock forever.sqlite3with a 10s busy timeout waited the full 10s and still failed.This PR makes sync survivable: progress accumulates across runs instead of resetting, so sync converges over several cycles even when individual runs die. It does not make it contention-free. Tracked follow-ups: #47 (single-writer architecture), #48 (per-source isolation), #49 (BUSY retry), #50 (why brokers get suspended).
Testing
jsonl_ingest_checkpoints_mid_source_and_resumes_from_there— checkpoints land inside the source, advance monotonically, and resuming from one ingests exactly the remaindera_slow_run_cannot_rewind_or_clobber_a_faster_runs_cursors— verified as a real regression test: fails against the wholesale write, rewinding the cursor to 400an_unchanged_source_does_not_rewrite_the_state_filecheckpoints_persist_between_sources_and_never_abort_a_run— including that an unwritable destination warns instead of unwindingopen_db_waits_longer_than_the_rusqlite_default_for_a_busy_writer— verified as a real regression test: failsleft: 5000, right: 30000without the changeFull workspace suite passes (64 tests). Validated end-to-end against the live database exhibiting the loop: the rollback-then-checkpoint path is what persisted state there.
🤖 Generated with Claude Code