Keep the later usage-index snapshot when two Charts scans commit - #359
Merged
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 |
Automated reviewNothing new in this pass; 1 finding(s) from the previous pass still open below. Still open from earlier passes:
Resolved since the previous pass: 1. For coding agents: fix BLOCK and FIX IF QUICK findings now; everything else is tracked or informational; never exceed one CodeRev fix round per PR. Advisory. Findings generated by |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ceiling | 98b2100 | Commit Preview URL Branch Preview URL |
Aug 22 2026, 05:23 PM |
SBS-948: encoding under the write lock did not order the file replace, so the slower card could put an older snapshot back on disk.
The SBS-948 race test treated ENCODED as proof the first writer was already sleeping, so it could skip the pause window, and the process-wide flags could stall other tests. Pause state is now per-store, handshake on pausing, and reset on Drop. Co-authored-by: Cursor <cursoragent@cursor.com>
btsouth
force-pushed
the
fix/sbs-948-index-commit-order
branch
from
August 22, 2026 17:22
e114ec5 to
98b2100
Compare
btsouth
enabled auto-merge (squash)
August 22, 2026 17:22
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
## Why `main` is red. `Rust / shared` on [`8f4868e5`](8f4868e5) failed: ``` ---- secure_file::tests::flock_unsupported_serializes_through_exclusive_create stdout ---- called `Result::unwrap()` on an `Err` value: Custom { kind: PermissionDenied, error: "flock is unsupported (unsupported) and exclusive-create failed: could not create the exclusive-create lock file: Access is denied. (os error 5)" } ``` It is a **flake, not a regression from that commit** — #359 touched `usage_index.rs` only, and main was green on `380db89c` immediately before. But the underlying bug is real and not test-only. Windows reports a file that has been unlinked while a handle is still closing as `ERROR_ACCESS_DENIED`, not `ERROR_FILE_EXISTS`. A waiter calling `create_new` while the holder is releasing therefore sees a name that is neither takeable nor already-taken, and `try_exclusive_create` mapped that to a hard `Failed`. In production that means **a release can hand the next writer an error instead of the lock**. ## What - `try_exclusive_create` retries a `PermissionDenied` create for up to one second — orders of magnitude longer than delete-pending takes to clear, and short enough that a genuine permission problem still reports its own error instead of waiting out the 10s acquire timeout. - The retry is gated on `const RETRIES_DELETE_PENDING: bool = cfg!(windows)` rather than `#[cfg(windows)]`, so the branch is compiled and lint-checked on Linux too. I can't build the MSVC target here, so this keeps the Windows-only path from going unchecked. ## Verified Linux: 26 `secure_file` tests pass, clippy `-D warnings` clean. The new `releasing_the_sibling_hands_the_lock_to_the_next_writer` runs the handoff 25 times per invocation; **15/15 invocations clean**. The Windows path itself can only be confirmed by CI on this PR. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches the cross-process lock that serializes settings and credential writes. The retry is short and Windows-only, but a misclassified permission error could delay a real failure by up to a second. > > **Overview** > On Windows, releasing the flock-fallback sibling lock can fail the next writer instead of handing the lock over. `create_new` racing an unlink that still has a handle closing returns `ERROR_ACCESS_DENIED`, which was treated as a hard failure. > > `try_exclusive_create` now retries `PermissionDenied` for up to one second, gated by `cfg!(windows)` so the path still compiles on Unix. A 25-round handoff test covers the race, and the changelog notes that a waiter in that window gets the lock rather than os error 5. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit c3323d3. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> > [!NOTE] > ### Retry `try_exclusive_create` on Windows when sibling is delete-pending > On Windows, deleting the exclusive-create sibling can leave a brief window where the next writer gets `PermissionDenied`. The lock helper now retries for up to 1 second (`DELETE_PENDING_GRACE`) before returning an error, using a small sleep between attempts. > - Extracts the single-attempt path into `try_exclusive_create_once` and wraps it in a retry loop gated by `RETRIES_DELETE_PENDING` (`cfg!(windows)`). > - Adds a regression test (`releasing_the_sibling_hands_the_lock_to_the_next_writer`) that runs 25 rounds of lock handoff to cover the delete-pending race. > - Risk: on Windows, lock acquisition can now take up to 1s longer in the delete-pending case before failing; non-Windows behavior is unchanged. > > <!-- Macroscope's review summary starts here --> > > <sup><a href="https://app.macroscope.com">Macroscope</a> summarized c3323d3.</sup> > <!-- Macroscope's review summary ends here --> > <!-- Macroscope's pull request summary ends here -->
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
persist() loaded it before taking the lock and reused that value for the decision afterwards. An apply() that landed in the window between the two set the flag on inserts this persist then skipped, and nothing wrote them. The authoritative read now happens under the lock; the pre-lock load stays as a fast path, which is safe because the scan that set the flag runs its own persist. The other half of the report - a stale snapshot being written over a later one, then clearing the flag - is closed by #359, which holds the write guard through atomic_write. Encode, write, and the clear are now all under the same lock, so no apply can interleave.
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.
Summary
IndexStore::commitencoded under the write lock, then dropped it beforeatomic_write. Two Charts cards (API value and heatmap) scan at the same time; the slower writer could put its older snapshot over the later one.Closes nothing on GitHub. Linear SBS-948.
Test plan
a_later_commit_is_not_overwritten_by_an_earlier_snapshot— two threads commit different files; after the first encoder is paused, the second still has to be on disk when both finish (the snapshot a restart would load).drop(guard)after encode. Test failed withsecond scan's file must not have been overwritten. Production lock hold restored; test passes.Quality gate (
.github/workflows/ci.ymlrust-shared)Fail-without-fix (
drop(guard)after encode, test only):Frontend and rust-desktop were not run: this diff is shared rust only.
Pattern sweep
Other
atomic_writesites (widget snapshot, models.dev cache, jsonl cache, credentials) do not encode a lock-protected in-memory snapshot and then release before persist.What this makes more likely
A slow or hung disk write now blocks the other card from taking the write lock and also blocks new
read()s. Encode was already under the write lock; this extends that hold acrosscreate_dir_all+atomic_write. The ticket allowed that, or a separate persist mutex. This PR takes the simpler of the two.Leftovers
commitintoapply+persist.persiststill encodes then releases before write. After that lands, persist needs the same hold-across-write. Not this ticket; merge conflict onusage_index.rsis expected.target/deleted after tests.Note
Fix race in
IndexStore.commitwhere concurrent chart scans could overwrite the usage index with an older snapshotWhen two chart scans committed concurrently, the write lock was released before
atomic_write, allowing an earlier snapshot to overwrite a later one on disk. The fix holds the write lock through the file write in usage_index.rs, ensuring the latest snapshot always wins. A new concurrency test reproduces the race using a controlled post-encode pause.Macroscope summarized 98b2100.
Note
Medium Risk
Touches concurrent persist of the Charts usage index: a hung disk write now blocks other commits and new reads. Persistence and restart behavior are affected, but the change is a small lock-scope fix with a dedicated race test.
Overview
Stops concurrent Charts scans from clobbering the on-disk usage index.
IndexStore::commitused to encode under the write lock, then drop it beforeatomic_write, so the slower of the API-value and heatmap scans could write an older snapshot and force a cold re-parse after restart.The write guard now stays held through the file replace, so disk always matches the latest encode. In-memory totals were already correct. A concurrency test pauses after encode to prove a later commit is not overwritten.
Reviewed by Cursor Bugbot for commit 98b2100. Bugbot is set up for automated code reviews on this repo. Configure here.