fix(app): stop the registry watcher clobbering a fresher in-memory write (SOU-329) - #624
Open
tsouth89 wants to merge 2 commits into
Open
fix(app): stop the registry watcher clobbering a fresher in-memory write (SOU-329)#624tsouth89 wants to merge 2 commits into
tsouth89 wants to merge 2 commits into
Conversation
…ite (SOU-329) watch_registry_for_app loads the registry file outside the RegistryState mutex and assigns the result under it, while write_registry holds the mutex only for its own update. That leaves a window: the watcher samples disk state A, a command writes B to both disk and cache, and the watcher then assigns its stale A over B and emits registry-changed with A. The UI shows the reverted state and the cache disagrees with the file until something else touches it. The disk read-modify-write is already serialized (SOU-23); it is the app's in-memory cache that had no generation guard. Adds a process-wide REGISTRY_GENERATION counter, bumped by every in-memory replacement while the RegistryState mutex is held. The watcher samples it before reading the file and, under the mutex, applies the load only if it is unchanged. A mismatch means the cache is now fresher, so the read is dropped and nothing is emitted; the winning write persisted to disk too, so its own mtime change brings the watcher back with the newer content on the next tick. Loading under the mutex would also have closed the race, but it holds the registry lock across file IO on every change, which is the pattern SOU-95 removed from the dispatch path. A module-level counter rather than more managed state because the registry is already a process-wide singleton and threading a second handle through ~70 command call sites is a lot of churn for one comparison. Mutation-verified: removing the guard fails a_racing_write_beats_a_stale_watcher_load and applying_advances_the_generation while an_uncontended_watcher_load_is_applied still passes, so the tests discriminate the fix rather than the code path. Signed-off-by: Tyler <258147599+tsouth89@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a race where the desktop app’s registry file watcher could overwrite a newer in-memory registry update with an older on-disk read, causing the UI to briefly revert and the cache to diverge from disk state.
Changes:
- Introduces a process-wide
REGISTRY_GENERATIONcounter to guard publishing watcher-loaded registry data against concurrent in-memory writes. - Updates registry write/refresh paths to bump the generation counter on in-memory cache replacement.
- Adds unit tests covering the race scenario and validating generation behavior, and documents the user-visible fix in the changelog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src-tauri/src/desktop.rs | Adds a generation guard to prevent the registry watcher from clobbering fresher in-memory state; adds targeted unit tests. |
| CHANGELOG.md | Documents the SOU-329 user-facing fix for the transient registry revert. |
Suppressed comments (1)
src-tauri/src/desktop.rs:2031
Ordering::SeqCstis stronger than needed for this generation counter. Since the registry data itself is protected by theRegistryStatemutex and the atomic is only used as a numeric generation marker for equality checks,Relaxedordering is sufficient and avoids unnecessary global synchronization.
REGISTRY_GENERATION.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2021
to
+2023
| /// Only ever read or written under the `RegistryState` mutex, which is what makes | ||
| /// the comparison meaningful; the atomic is for interior mutability, not for | ||
| /// lock-free access. |
| static REGISTRY_GENERATION: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); | ||
|
|
||
| fn registry_generation() -> u64 { | ||
| REGISTRY_GENERATION.load(std::sync::atomic::Ordering::SeqCst) |
📝 WalkthroughWalkthroughThe registry now uses a process-wide generation counter. Writes and reloads advance the counter. The disk watcher discards snapshots sampled before an in-memory change. Tests cover racing writes, external updates, and duplicate stale loads. ChangesRegistry consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegistryWatcher
participant DiskRegistry
participant RegistryCache
participant RegistryEvent
RegistryWatcher->>RegistryCache: sample generation
RegistryWatcher->>DiskRegistry: load registry snapshot
RegistryWatcher->>RegistryCache: guarded snapshot publication
RegistryCache-->>RegistryWatcher: applied or stale
RegistryWatcher->>RegistryEvent: emit update when applied
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@src-tauri/src/desktop.rs`:
- Around line 2056-2058: Update reload_into_state to snapshot the registry
generation before loading from disk, then verify it is unchanged immediately
before publishing fresh into the locked state. If the generation changed during
the load, reject or retry instead of replacing the newer cache; ensure
refresh_from_disk and nudge_gateway follow the same stale-load protection.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88358457-339e-445c-973a-42a4d42ff2a0
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src-tauri/src/desktop.rs
Comment on lines
+2056
to
+2058
| let mut guard = state.lock().unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| *guard = fresh.clone(); | ||
| bump_registry_generation(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard reload_into_state before it replaces the cache.
The disk load at Line 2055 completes before this mutex acquisition. A concurrent write_registry can persist and cache registry B, then advance the generation. This function can then replace B with its earlier registry A.
Sample the generation before the load. Reject or retry the load if the generation changed before publication. Otherwise, refresh_from_disk and nudge_gateway can still restore stale cached state.
🤖 Prompt for 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.
In `@src-tauri/src/desktop.rs` around lines 2056 - 2058, Update reload_into_state
to snapshot the registry generation before loading from disk, then verify it is
unchanged immediately before publishing fresh into the locked state. If the
generation changed during the load, reject or retry instead of replacing the
newer cache; ensure refresh_from_disk and nudge_gateway follow the same
stale-load protection.
From the review on #624. reload_into_state has the same shape as the watcher - load disk, then take the mutex and assign - so it had the identical race, reachable through refresh_from_disk and the team-sync paths rather than through the file watcher. Fixing only the watcher left the defect live behind a second door. Rather than a second copy of the guard, both callers now go through publish_if_unchanged. The first attempt at this DID duplicate the guard inline, and the test written for it passed with that guard mutated away, because the test exercised the shared helper while reload_into_state ran its own copy - coverage that proved nothing. One implementation removes the possibility. Mutation-verified now: removing the guard fails three of the four tests, and the uncontended case still passes. On a lost race the caller gets the cached registry, which is the newer of the two, so the return value is still the authoritative current state. Also corrects the generation counter's doc comment, which claimed reads were always under the mutex; the pre-load sample is deliberately outside it, since locking there would reintroduce the hold-across-IO this design avoids. SeqCst is kept over Relaxed, with the reasoning written down: a stale unlocked sample can only cause a false mismatch, which drops a load rather than clobbering one, but that is a subtlety a later edit could invalidate and the ordering costs nothing at one bump per write and one read per 1500 ms tick. Signed-off-by: Tyler <258147599+tsouth89@users.noreply.github.com>
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.
The race
watch_registry_for_apploads the registry file outside theRegistryStatemutex and assigns the result under it, whilewrite_registryholds the mutex only for its own update. So:registry-changedwith A.The UI shows the reverted state, and the cache disagrees with the file until something else touches it. The disk read-modify-write is already serialized (SOU-23); it is the app's in-memory cache that had no generation guard.
The fix
A process-wide
REGISTRY_GENERATIONcounter, bumped by every in-memory replacement while theRegistryStatemutex is held. The watcher samples it before reading the file and, under the mutex, applies the load only if it is unchanged.A mismatch means the cache is now fresher, so the read is dropped and nothing is emitted. Nothing is lost: the winning write persisted to disk too, so its own mtime change brings the watcher back with the newer content on the next tick.
last_jsonis deliberately left alone on a skip so the dropped content is not remembered as applied.Two design notes:
RegistryStatemutex, which is what makes the comparison meaningful; the atomic is for interior mutability, not lock-free access.Testing
3 new tests against the real mutex plus counter, serialized so the shared counter cannot make them interfere:
a_racing_write_beats_a_stale_watcher_load— the SOU-329 sequence in order; the newer write must survive.an_uncontended_watcher_load_is_applied— the ordinary case (gateway or team sync changed the file, nothing touched the cache) must still work, so the guard cannot be trivially satisfied by refusing everything.applying_advances_the_generation— publishing bumps the counter, so a second load sampled at the same moment cannot overwrite what was just published.Mutation-verified. With the guard removed, the first and third fail and the second still passes, so the tests discriminate the fix rather than the code path.
Full suites green: 692 lib + 219 gateway bin Rust tests.
Closes SOU-329.