composite.Store uses routers#3420
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3420 +/- ##
==========================================
+ Coverage 59.24% 59.43% +0.19%
==========================================
Files 2126 2116 -10
Lines 175715 174723 -992
==========================================
- Hits 104097 103849 -248
+ Misses 62538 61841 -697
+ Partials 9080 9033 -47
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview SC configuration model is simplified and expanded. Removes SC Integration/ops scripts pivot to “FlatKV from genesis” testing. CI FlatKV integration job now boots with Reviewed by Cursor Bugbot for commit c895b39. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // GetChildStoreByName returns the underlying child store by module name. | ||
| // This only applies to cosmos committer. | ||
| func (cs *CompositeCommitStore) GetChildStoreByName(name string) types.CommitKVStore { |
There was a problem hiding this comment.
GetChildStoreByName func won't return nil anymore, we should update store.go
tree := snap.GetChildStoreByName(k.Name())
if tree == nil {
return nil, fmt.Errorf("snapshot missing child store %q", k.Name())
}
There was a problem hiding this comment.
You make a good point, but I think the better behavior here would be to panic if we encounter an unsupported store. If a caller instantiates a bad store type and we allow the behavior, they will get an error during a later operation no matter what we do here. So it's best to raise the problem early.
This method now panics if the requested store name is not supported.
|
should we update
|
|
comments cleanup: we have many comments still using |
| // memiavl.ApplyUpgrades rejects duplicate names rather than skipping | ||
| // them, so we guard with a presence check. Read-only handles inherit | ||
| // the tree from the on-disk state. | ||
| if cs.memIAVL != nil && cs.config.WriteMode.IsMigrationMode() && |
There was a problem hiding this comment.
the readOnly mode in composite/store.go(200-217) assumes the migration tree already exists and it doesn't ApplyUpgrades the migration tree like the writable branch does. Between a write-mode switch into a migration mode and the first Commit(), the tree only lives in memiavl's working state. any concurrent read-only LoadVersion fails with store not found: migration from buildMemIAVLReader
There was a problem hiding this comment.
I was able to simplify this code significantly. We don't actually need to store migration metadata in memIAVL, we can just store all the data in flatKV. With this change, the bug you point out is no longer possible.
I've intentionally implemented this as a do nothing method. We never call it in production, and it's a method I plan to delete soon. The core reason why I hesitate to actually implement this method is because the flatKV implementation would be quite nontrivial. |
I did a pass and updated a bunch of comments. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c895b39. Configure here.
| _, err := cs.flatkvCommitter.LoadVersion(targetVersion, false) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load FlatKV version: %w", err) | ||
| return ro, nil |
There was a problem hiding this comment.
Read-only LoadVersion leaks backend handles on router failure
Medium Severity
In the read-only LoadVersion path, if ro.buildRouter() fails, both the freshly-opened memIAVLCommitter and flatKVStore handles are leaked. They were obtained from LoadVersion calls on the backends but the ro composite is never returned to the caller nor closed before returning the error. Repeated failures would accumulate open file descriptors until exhaustion.
Reviewed by Cursor Bugbot for commit c895b39. Configure here.
2 participants
Describe your changes and provide context
Integrates
migration.Routerwithcomposite.Storeimplementation.Testing performed to validate your change
Unit tests. More unit tests to follow in the next PR.