Repository navigation
Make the composite store router an atomic pointer - #4470
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4470 +/- ##
==========================================
- Coverage 56.90% 56.85% -0.05%
==========================================
Files 2130 2130
Lines 167505 167515 +10
==========================================
- Hits 95315 95240 -75
- Misses 72185 72270 +85
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
SetWriteMode replaces the router while child-store views from GetChildStoreByName read it through a provider. Keep it in an atomic.Pointer and access it through loadRouter and storeRouter.
3f0eed8 to
5b08d3e
Compare
PR SummaryMedium Risk Overview The router field is now an Adds Reviewed by Cursor Bugbot for commit 1682d7d. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Moves CompositeCommitStore's migration router into an atomic.Pointer reached only through loadRouter/storeRouter, so child-store views stop racing with SetWriteMode. The router accesses are now consistently synchronized and nothing blocks; the open points are that the new test does not reliably overlap reads with the switch, and that views can still race on cs.flatKV. The tree at the merge ref was read in full, and no Go toolchain was available, so the race test was not run. Kept codex's test-synchronization finding, which checks out.
Non-blocking
1 finding on the changed lines, as inline comments.
Pre-existing
Already true on the base branch, not introduced here.
- suggestion — Views from GetChildStoreByName still read cs.flatKV without synchronization through their Iterator closure (cs.iterate, store.go ~1722). On the MemiavlOnly -> MigrateEVM edge, SetWriteMode assigns that field via materializeFlatKV/adoptFlatKV. That is the same concurrent-query scenario this PR addresses, so iteration during the kickoff block is still a data race, and the new test exercises only Get, so it would not catch it.
seidroid review · decision approve · session f47bf29309674702b9a3a0b73c556f88 · turn resp_claude_cf756772eda9d3ca8a3629f14b30d69b · item 81e309703d2551c989068341474d7445
Findings: 0 blocking | 1 non-blocking | 1 posted inline | 1 pre-existing
|
@seidroid review |
There was a problem hiding this comment.
Since the last review only the new test changed. It now waits for the reader's first successful Get before calling SetWriteMode, and a deferred, once-guarded stop always shuts the reader down before Close, which addresses my earlier thread (raised with codex). The store.go change is unchanged and correct, so nothing blocks; the pre-existing race on cs.flatKV during iteration still stands. Codex found nothing this round, which agrees with this reading. The tree at the merge ref was read in full.
Pre-existing
Already true on the base branch, not introduced here.
- suggestion — Views from GetChildStoreByName still read cs.flatKV without synchronization through their Iterator closure (cs.iterate, store.go ~1722). On the MemiavlOnly -> MigrateEVM edge, SetWriteMode assigns that field via materializeFlatKV/adoptFlatKV, so iteration during the kickoff block is still a data race. The new test exercises only Get, so it would not catch it.
seidroid review · decision approve · session f47bf29309674702b9a3a0b73c556f88 · turn resp_claude_fdf838def8fe9a3c7489f30c9b57f0fd · item 1821a50a3093553c9ca054db2026b13e
Findings: 0 blocking | 0 non-blocking | 0 posted inline | 1 pre-existing
|
Successfully created backport PR for |
Follow-up to #4470. Child-store views returned by `GetChildStoreByName`, and latest-height queries, read the composite store's `flatKV` backend and effective write mode without holding the root store's lock, while `SetWriteMode` can install both when the migration starts. As plain fields those accesses are unsynchronised. This stores both in `atomic.Pointer`s behind `loadFlatKV`/`storeFlatKV` and `loadWriteMode`/`storeWriteMode`, matching the router accessors from `TestComposite_Auto_ChildStoreIterationDuringWriteModeSwitch` iterates through a cached view and through fresh views across the `MemiavlOnly` to `MigrateEVM` switch; it reports data races under `-race` against the previous `store.go` and passes with this change. No change to the AppHash. (cherry picked from commit 36941db)
) Follow-up to sei-protocol#4470. Child-store views returned by `GetChildStoreByName`, and latest-height queries, read the composite store's `flatKV` backend and effective write mode without holding the root store's lock, while `SetWriteMode` can install both when the migration starts. As plain fields those accesses are unsynchronised. This stores both in `atomic.Pointer`s behind `loadFlatKV`/`storeFlatKV` and `loadWriteMode`/`storeWriteMode`, matching the router accessors from sei-protocol#4470, and has reader paths load each value once per call. `TestComposite_Auto_ChildStoreIterationDuringWriteModeSwitch` iterates through a cached view and through fresh views across the `MemiavlOnly` to `MigrateEVM` switch; it reports data races under `-race` against the previous `store.go` and passes with this change. No change to the AppHash.
CompositeCommitStorekeeps its migration router in a plain field.SetWriteModereplaces it when the write mode changes, while child-store views handed out byGetChildStoreByNameread it on every call through a provider, and those accesses weren't synchronized.The router now lives in an
atomic.Pointerand every access goes throughloadRouterandstoreRouter. A new test reads through a cached child-store view across aSetWriteModetransition under-race. Behaviour and the AppHash are unchanged.