fix: version-gate commit_batch accumulated costs - #637
Conversation
The fix in 4a0731e changed commit_batch to return accumulated batch costs instead of discarding them. This affects cost calculations which need to be consistent per protocol version. - V1/V2: commit version 0 — discard batch costs (legacy behavior) - V3: commit version 1 — preserve accumulated batch costs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a new nested version struct Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Apply as Merk::apply_unchecked
participant Merk as Merk::commit
participant GV as GroveVersion
participant Cost as CostAccumulator
Caller->>Apply: invoke apply_unchecked(key_updates, aux, ...)
Activate Apply
Apply->>Merk: commit(key_updates, aux, options, old_specialized_cost, grove_version)
Activate Merk
Merk->>GV: read merk_versions.batch.commit
alt GV.batch.commit >= 1
Merk->>Cost: preserve and add accumulated batch costs
else GV.batch.commit < 1
Merk->>Cost: discard batch costs (legacy)
end
Merk-->>Apply: return CommitResult (with cost info)
Deactivate Merk
Apply-->>Caller: return result
Deactivate Apply
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #637 +/- ##
========================================
Coverage 90.84% 90.84%
========================================
Files 182 182
Lines 51772 51781 +9
========================================
+ Hits 47034 47043 +9
Misses 4738 4738
🚀 New features to boost your workflow:
|
Access path is now grove_version.merk_versions.batch.commit instead of grove_version.merk_versions.commit.commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
merk/src/merk/mod.rs (1)
381-388:⚠️ Potential issue | 🟠 MajorFix error wrapping at line 505 and confirm API break acceptability.
Line 505 violates the error-wrapping guideline:
.map_err(StorageError)should be.map_err(|e| Error::CorruptedData(format!("storage commit failed: {}", e)))to provide context.Additionally, this PR adds
grove_version: &GroveVersionas a required parameter to the publicMerk::commitmethod—a breaking change. If 4.x compatibility is a requirement, this needs an explicit migration path in the release notes and semver bump confirmation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merk/src/merk/mod.rs` around lines 381 - 388, The commit method's error handling incorrectly uses .map_err(StorageError) on the storage commit call; change it to map the error into a contextual Error variant such as Error::CorruptedData with a message like "storage commit failed: {e}" (i.e., .map_err(|e| Error::CorruptedData(format!("storage commit failed: {}", e)))) inside the pub fn commit(...) implementation to preserve context; also note that adding grove_version: &GroveVersion to the public Merk::commit signature is an API-breaking change—confirm a semver bump and add a migration note in the release notes documenting the new parameter (or provide a compatibility shim) before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@merk/src/merk/mod.rs`:
- Line 505: The call to self.storage.commit_batch(batch) currently maps all
failures to StorageError, losing call-site context; change the mapping to wrap
the error with a descriptive message (e.g., via .map_err(|e|
Error::CorruptedData(format!("commit_batch failed: {}", e)))) so the returned
error includes that it originated from commit_batch; update the binding
commit_result (or its use) to use this contextualized map_err instead of
StorageError.
---
Outside diff comments:
In `@merk/src/merk/mod.rs`:
- Around line 381-388: The commit method's error handling incorrectly uses
.map_err(StorageError) on the storage commit call; change it to map the error
into a contextual Error variant such as Error::CorruptedData with a message like
"storage commit failed: {e}" (i.e., .map_err(|e|
Error::CorruptedData(format!("storage commit failed: {}", e)))) inside the pub
fn commit(...) implementation to preserve context; also note that adding
grove_version: &GroveVersion to the public Merk::commit signature is an
API-breaking change—confirm a semver bump and add a migration note in the
release notes documenting the new parameter (or provide a compatibility shim)
before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2deee038-c69f-428f-a914-ccf6bc887960
📒 Files selected for processing (6)
grovedb-version/src/version/merk_versions.rsgrovedb-version/src/version/v1.rsgrovedb-version/src/version/v2.rsgrovedb-version/src/version/v3.rsmerk/src/merk/apply.rsmerk/src/merk/mod.rs
| .commit_batch(batch) | ||
| .map_err(StorageError) | ||
| .add_cost(cost) | ||
| let commit_result = self.storage.commit_batch(batch).map_err(StorageError); |
There was a problem hiding this comment.
Add call-site context before returning commit_batch failures.
This new path collapses the final batch flush failure into a bare StorageError, which makes it much harder to tell from logs that the error came from commit_batch rather than earlier tree work.
🛠️ Proposed fix
- let commit_result = self.storage.commit_batch(batch).map_err(StorageError);
+ let commit_result = self
+ .storage
+ .commit_batch(batch)
+ .map_err(|e| Error::CorruptedData(format!("commit_batch failed: {}", e)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let commit_result = self.storage.commit_batch(batch).map_err(StorageError); | |
| let commit_result = self | |
| .storage | |
| .commit_batch(batch) | |
| .map_err(|e| Error::CorruptedData(format!("commit_batch failed: {}", e))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@merk/src/merk/mod.rs` at line 505, The call to
self.storage.commit_batch(batch) currently maps all failures to StorageError,
losing call-site context; change the mapping to wrap the error with a
descriptive message (e.g., via .map_err(|e|
Error::CorruptedData(format!("commit_batch failed: {}", e)))) so the returned
error includes that it originated from commit_batch; update the binding
commit_result (or its use) to use this contextualized map_err instead of
StorageError.
1 participant
Summary
commit: 0): discard batch costs fromcommit_batch(legacy behavior matching pre-fix)commit: 1): preserve accumulated batch costs (seek counts, storage costs)MerkCommitVersionsstruct toMerkVersionsand passesgrove_versionthroughMerk::commitTest plan
cargo buildcleancargo test -p grovedb-merk— 344 passedcargo test -p grovedb --lib— 1403 passedcargo test -p grovedb-version— 47 passedcargo clippy -- -D warningsclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor