Repository navigation
fix(batch): refuse an empty path on every reference hop instead of panicking - #1016
Conversation
…nicking apply_batch refused an empty reference path only for the op it was processing. Keys in a path run in byte order, so an earlier op could follow its chain into a later pending reference (or a trusted RefreshReference) whose path resolves to `[]` before that op's own check ran. follow_reference_get_value_hash then recursed with `[]` and reached process_reference, whose split_last().expect(..) panicked. follow_reference_get_value_hash now refuses an empty qualified path with InvalidBatchOperation. Every hop passes through it, so one check covers the pending-reference arms, the refresh branch of process_reference and process_reference_with_hop_count_greater_than_one. The check sits after the hop-limit check, so only runs that used to panic change. Not version-gated: a batch that took this path never committed at any grove version, so no stored state or replayed block depends on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No merge-blocking issue is identified. Merge after normal validation. Pre-merge checks |
|
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
🔍 Review in progress — actively reviewing now (commit 45872cd) · triage: low · Phase 2 only (no Phase 1 for this repository) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1016 +/- ##
========================================
Coverage 93.20% 93.20%
========================================
Files 336 336
Lines 106238 106244 +6
========================================
+ Hits 99018 99024 +6
Misses 7220 7220
🚀 New features to boost your workflow:
|
Issue
A crafted batch panics inside
apply_batchinstead of returning an error. This was ondevelopbefore #1015.apply_batchrefuses an empty reference path only for the reference op it is processing: "attempting to insert an empty reference" in the insert arm, and "attempting to refresh an empty reference" in the refresh arm. Ops run deepest level first, and keys within a path run in byte order. So an earlier op can follow its chain into a later pending reference before that op's own check runs.follow_reference_get_value_hashresolves the next hop from the pending element withpath_from_reference_qualified_path_type. That returns[]forAbsolutePathReference(vec![]),UpstreamRootHeightReference(0, vec![])andUpstreamFromElementHeightReference(path.len(), vec![]). The function recursed with that[]without checking it.ops_by_qualified_paths.get(&[])is alwaysNone, so the call went toprocess_reference(&[]). Therequalified_path.split_last().expect("path validated non-empty above")panicked.Reproducer, under
[TEST_LEAF]in one batch:A trusted
RefreshReferenceonbthat carries an empty path reaches the sameexpect, throughprocess_reference's in-batch branch. On unmodifieddevelopboth new tests panic atgrovedb/src/batch/mod.rs:2294withpath validated non-empty above.Fix
follow_reference_get_value_hashnow refuses an emptyqualified_pathwithInvalidBatchOperation("reference chain resolves to an empty path"). Every hop goes through this function: the two top-level apply arms call it, and so does every recursive hop (the pending-reference arms, the bidirectional and backward-references arms,process_reference's refresh branch, andprocess_reference_with_hop_count_greater_than_one). One check therefore covers every hop.process_reference_with_hop_count_greater_than_oneneeds no check of its own. It receiveskeyandreference_pathfromprocess_reference, which has already split a non-empty path. Its only recursion goes back throughfollow_reference_get_value_hash. The twosplit_last().expect(..)calls stay in place, and their messages now name the check they depend on.The check comes after the
recursions_allowed == 0→ReferenceLimitcheck. A chain that runs out of hops on an empty path still reportsReferenceLimit. Only runs that used to panic now behave differently.Why this is not version-gated
The repo rule is that GROVE_V3 is live, so behavior-changing fixes go behind GROVE_V4+. This fix changes behavior only for batches that used to panic:
qualified_paththat gets past the hop-limit check always reachedprocess_reference(&[])and panicked. No pending op has an empty qualified path, since an op always has a key, so the in-batch lookup could never match.A version gate would only keep a panic alive on older versions, so the fix applies at every grove version.
Tests
In
grovedb/src/tests/batch_unit_tests.rs(Group 16), both tests run over every version inGROVE_VERSIONSand over all three empty-resolving path types:test_batch_ref_hop_to_pending_empty_reference_errorscoversa → b, wherebis a pendingInsertOrReplaceorInsertWithKnownToNotAlreadyExistreference with an empty path. These go through two different match arms.test_batch_ref_hop_to_trusted_refresh_with_empty_path_errorscoversa → b, wherebis a trustedRefreshReferencethat carries an empty path.Each case checks for the new
InvalidBatchOperationerror, which confirms the case reached the empty hop, and checks that the root hash did not change.cargo test -p grovedb --lib batch: 749 passedcargo test -p grovedb: 3557 lib tests passed (8 ignored), plus every integration and doc-test binarycargo fmt --all -- --check: cleancargo clippy -p grovedb --all-targets -- -D warnings: no lints in the changed filesdocs/audit-non-issues.mdhas no entry covering this.🤖 Generated with Claude Code
PR Hygiene ·
45872cd/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit
InvalidBatchOperationwithout writing changes.