fix: per-op DeleteTree emptiness check via IsSubtreeNonEmpty enum - #634
Merged
QuantumExplorer merged 8 commits intoMar 9, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughGroveOp::DeleteTree now carries a second parameter: SubelementsDeletionBehavior. The new public enum (DontCheck, Error, DeleteChildren, Skip) is exposed and propagated through batch APIs, per-op logic, delete operation exports, pattern matches, default behaviors, and tests; BatchApplyOptions fields were removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
grovedb/src/batch/single_sum_item_deletion_cost_tests.rs (1)
47-52:⚠️ Potential issue | 🟠 MajorUse
TreeType::SumTreefor these sum-tree deletions.Both tests create a sum tree, then build the batch delete with
TreeType::NormalTree. That means the batch path is not exercising the same tree type as the non-batch delete, so the “costs match” assertion can miss sum-tree-specific regressions.Proposed fix
let ops = vec![QualifiedGroveDbOp::delete_tree_op( vec![], b"key1".to_vec(), - TreeType::NormalTree, + TreeType::SumTree, IsSubtreeNonEmpty::Error, )];let ops = vec![QualifiedGroveDbOp::delete_tree_op( vec![], b"key1".to_vec(), - TreeType::NormalTree, + TreeType::SumTree, IsSubtreeNonEmpty::Error, )];Also applies to: 155-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/single_sum_item_deletion_cost_tests.rs` around lines 47 - 52, The batch delete ops in the tests use QualifiedGroveDbOp::delete_tree_op with TreeType::NormalTree though the trees under test are sum trees; update those calls to use TreeType::SumTree so the batch deletion path matches the non-batch sum-tree deletion (replace TreeType::NormalTree with TreeType::SumTree in the delete_tree_op invocations around the mentioned test blocks, including the occurrences at lines ~47-52 and ~155-160).grovedb/src/operations/delete/mod.rs (1)
683-688:⚠️ Potential issue | 🟠 MajorUse
DontCheckfor already-validated delete-tree ops.
delete_operation_for_delete_internal()has already computedis_emptybefore it reaches this branch. EmittingIsSubtreeNonEmpty::Errormakes batch execution reopen the child subtree and run the emptiness check again, which brings back the extra Merk open / fee inflation this PR is trying to remove. Since this branch only returns an op when the tree is already known empty,DontCheckpreserves behavior without the duplicate check.Suggested fix
} else if is_empty { Ok(Some(QualifiedGroveDbOp::delete_tree_op( path.to_vec(), key.to_vec(), tree_type, - IsSubtreeNonEmpty::Error, + IsSubtreeNonEmpty::DontCheck, ))) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/delete/mod.rs` around lines 683 - 688, The delete-tree op being emitted uses IsSubtreeNonEmpty::Error which forces a redundant subtree emptiness re-check; update the QualifiedGroveDbOp::delete_tree_op call in delete_operation_for_delete_internal (the branch returning Ok(Some(QualifiedGroveDbOp::delete_tree_op(...)))) to pass IsSubtreeNonEmpty::DontCheck instead so the already-validated empty-tree path skips the duplicate check and avoids reopening the child subtree.grovedb/src/batch/estimated_costs/worst_case_costs.rs (1)
146-152:⚠️ Potential issue | 🟠 MajorModel
IsSubtreeNonEmptyin worst-case pricing.This now prices
DeleteTree(DontCheck | DeleteChildren)exactly like an empty-tree delete, but batch execution recursively clears child storage for non-empty subtrees in those modes. That underestimates the worst-case fee for the new per-op policies. Please branch onIsSubtreeNonEmptyhere and either add the cleanup cost or reject unsupported policies until the estimator can model them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/estimated_costs/worst_case_costs.rs` around lines 146 - 152, The DeleteTree branch in the worst-case estimator currently calls GroveDb::worst_case_merk_delete_tree without considering IsSubtreeNonEmpty; update the GroveOp::DeleteTree handling to inspect the IsSubtreeNonEmpty policy value and if the subtree may be non-empty either add the additional recursive cleanup cost to the worst-case estimate (modeling child deletion/storage clears using worst_case_layer_element_estimates, propagate and grove_version) or explicitly reject/return an error for unsupported policies until the estimator models them; ensure you reference the IsSubtreeNonEmpty variant when branching and keep the existing call to worst_case_merk_delete_tree only for the empty-subtree case.grovedb/src/batch/estimated_costs/average_case_costs.rs (1)
154-160:⚠️ Potential issue | 🟠 MajorDelete-tree average-case pricing still ignores
IsSubtreeNonEmpty.Line 154 throws away the new policy and sends every
DeleteTreethrough the sameaverage_case_merk_delete_tree()path, but the real batch logic now branches onIsSubtreeNonEmptybefore deleting. That means a non-emptySkipcan be billed like a full delete, whileError/DeleteChildrenmiss the extra emptiness-check I/O. This leaves estimated fees out of sync with the behavior this PR is introducing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/estimated_costs/average_case_costs.rs` around lines 154 - 160, The DeleteTree branch ignores the new IsSubtreeNonEmpty policy and always calls GroveDb::average_case_merk_delete_tree; update the GroveOp::DeleteTree handling to branch on the IsSubtreeNonEmpty enum (the same way the real batch logic does) and route to the correct cost path: for the Skip case use the cheaper skip-cost estimator, while for Error/DeleteChildren include the extra emptiness-check I/O by invoking the estimator that accounts for the subtree-nonempty check before delete (i.e., add a branch that mirrors IsSubtreeNonEmpty and calls the appropriate average-case helper instead of always calling average_case_merk_delete_tree), preserving existing args like key, layer_element_estimates, propagate, and grove_version.
🧹 Nitpick comments (3)
grovedb/src/tests/misc_coverage_tests.rs (1)
1485-1490: Consider covering a non-Errordelete policy here.These updates make the tests compile against the new API, but they still only exercise
IsSubtreeNonEmpty::Error. Since this PR’s behavioral change is policy-dependent, adding at least oneDontCheck,Skip, orDeleteChildrencase here would make this coverage file guard the new branch-specific cost behavior.Also applies to: 1523-1528, 2358-2363, 2396-2401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/misc_coverage_tests.rs` around lines 1485 - 1490, Add test cases that exercise non-Error delete policies by duplicating the existing QualifiedGroveDbOp::delete_tree_op usages and switching IsSubtreeNonEmpty::Error to another variant (e.g., IsSubtreeNonEmpty::DontCheck or IsSubtreeNonEmpty::Skip or IsSubtreeNonEmpty::DeleteChildren) so the new policy-dependent branch is exercised; update the ops vectors at the three other spots referenced (the similar blocks around the other occurrences) to include at least one op with a non-Error policy and run the same assertions for those cases to cover the branch-specific cost/behavior.grovedb/src/tests/batch_delete_tree_tests.rs (1)
225-231: Please add aDeleteChildrenvariant of this cleanup test.This file now covers
Error,DontCheck, andSkip, but not the newDeleteChildrenpolicy. A parallel case here would be useful becauseDeleteChildrenhas different control flow fromDontCheck: it still checks non-empty state before doing the recursive cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/batch_delete_tree_tests.rs` around lines 225 - 231, Add a parallel test case that uses IsSubtreeNonEmpty::DeleteChildren for the QualifiedGroveDbOp::delete_tree_op call so the suite covers the new policy; specifically, create an ops vector like the existing one but with IsSubtreeNonEmpty::DeleteChildren, execute the same batch delete flow (calling the same test helper or invocation used for DontCheck/Skip), and add assertions that verify the recursive children were checked and removed (i.e., parent and descendant trees no longer exist) to reflect the different control flow of DeleteChildren versus DontCheck.grovedb/src/tests/batch_coverage_tests.rs (1)
153-204: Add regression coverage forSkipandDeleteChildren.These updated tests now cover
ErrorandDontCheck, but the two new behavioral branches areSkipandDeleteChildrenon non-empty subtrees. Without at least one explicit assertion for each, both the batch and sequential delete-tree paths can regress without the suite noticing.Also applies to: 457-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/batch_coverage_tests.rs` around lines 153 - 204, Add assertions and test cases to cover the remaining IsSubtreeNonEmpty branches Skip and DeleteChildren for non-empty trees: update the existing tests around test_batch_delete_non_empty_tree_with_allow_option and the sequential delete-tree tests to include operations created with QualifiedGroveDbOp::delete_tree_op using IsSubtreeNonEmpty::Skip and IsSubtreeNonEmpty::DeleteChildren, then assert expected behavior (Skip should leave the non-empty tree intact and DeleteChildren should remove the tree and its children); reference the same tree setup (e.g., "tree_with_items" and its child) and use db.get or db.get_path checks to verify presence/absence after applying db.apply_batch or the sequential delete methods so both batch and sequential paths exercise these branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/book/src/batch-operations.md`:
- Line 16: Add a short enum/migration subsection to the page that documents the
new IsSubtreeNonEmpty parameter introduced on DeleteTree (parameterized by
TreeType), listing and describing each variant (DontCheck, Error,
DeleteChildren, Skip), and explicitly show how each maps from the previous
BatchApplyOptions flags; for each variant include a one-line semantic
description and a brief migration note stating when to choose it (e.g.,
DontCheck = legacy-ignore, Error = fail on non-empty, DeleteChildren =
cascade-delete children, Skip = no-op when non-empty) so callers know which
variant replaces which BatchApplyOptions behavior and what runtime effect to
expect.
In `@docs/book/translations/es/src/batch-operations.md`:
- Line 16: The inline comment for the operation DeleteTree is outdated: update
the comment on the line containing DeleteTree(TreeType, IsSubtreeNonEmpty) to
mention both parameters and the per-operation policy by describing that
DeleteTree is parameterized by TreeType and an IsSubtreeNonEmpty flag (or
per-operation policy) which governs whether subtree deletion is allowed; ensure
the comment succinctly documents both parameters and the new per-operation
policy so readers understand the meaning of TreeType and IsSubtreeNonEmpty.
In `@docs/book/translations/it/src/batch-operations.md`:
- Line 16: The inline note is outdated: update the comment for DeleteTree to
reflect both parameters by changing the note to indicate DeleteTree(TreeType,
IsSubtreeNonEmpty) is parameterized by the tree type and a boolean indicating
whether the subtree is non-empty; reference the DeleteTree symbol and its
parameters TreeType and IsSubtreeNonEmpty so the API surface in the sample
matches the signature.
In `@docs/book/translations/ja/src/batch-operations.md`:
- Line 16: Update the stale inline comment for DeleteTree(TreeType,
IsSubtreeNonEmpty) to describe both parameters: explain that the first parameter
identifies the TreeType and the second parameter IsSubtreeNonEmpty specifies the
per-operation emptiness policy (whether the delete applies only to non-empty
subtrees or can remove empty subtrees), so readers understand how the operation
is parameterized by tree type and the per-op emptiness policy.
In `@docs/book/translations/ru/src/batch-operations.md`:
- Line 16: The inline comment for the DeleteTree entry is outdated: update the
trailing comment on the DeleteTree line to mention both parameters (TreeType and
IsSubtreeNonEmpty) and briefly describe the emptiness-policy parameter (e.g.,
whether non-empty subtrees are allowed or will error/delete), so the sample
shows both symbols DeleteTree, TreeType and IsSubtreeNonEmpty and clarifies the
new policy.
In `@grovedb/src/batch/mod.rs`:
- Around line 3534-3559: The code opens a layered Merk for the subtree with a
None root key which can mis-report non-empty subtrees; change the
Merk::open_layered_with_root_key call (used when constructing child_merk from
child_storage/subtree_path) to pass the subtree’s actual stored root key instead
of None (retrieve the root key from child_storage or the subtree metadata
available in your DB API) so that is_empty_tree_except()/IsSubtreeNonEmpty logic
uses the real root for the emptiness check; ensure the same fix is applied to
the other occurrence (around lines 3886-3911) where open_layered_with_root_key
is called with None.
- Around line 3518-3532: The code currently builds batch_deleted_keys from ops
before knowing which DeleteTree child deletes are actually executed, so
IsSubtreeNonEmpty::Skip decisions may be incorrectly treated as deletions and
cause ancestor DeleteTree to remove whole subtrees; fix by resolving skip/error
decisions for child DeleteTree ops first (i.e., determine the effective delete
set or filter out ops where IsSubtreeNonEmpty::Skip applies) and then build
batch_deleted_keys from that effective set (or compute the deleted-key set
bottom-up from effective deletions) wherever batch_deleted_keys and
batch_deleted_keys_refs are computed (referencing batch_deleted_keys,
batch_deleted_keys_refs, ops, and IsSubtreeNonEmpty::Skip and
DeleteTree(Error|Skip|DeleteChildren)).
---
Outside diff comments:
In `@grovedb/src/batch/estimated_costs/average_case_costs.rs`:
- Around line 154-160: The DeleteTree branch ignores the new IsSubtreeNonEmpty
policy and always calls GroveDb::average_case_merk_delete_tree; update the
GroveOp::DeleteTree handling to branch on the IsSubtreeNonEmpty enum (the same
way the real batch logic does) and route to the correct cost path: for the Skip
case use the cheaper skip-cost estimator, while for Error/DeleteChildren include
the extra emptiness-check I/O by invoking the estimator that accounts for the
subtree-nonempty check before delete (i.e., add a branch that mirrors
IsSubtreeNonEmpty and calls the appropriate average-case helper instead of
always calling average_case_merk_delete_tree), preserving existing args like
key, layer_element_estimates, propagate, and grove_version.
In `@grovedb/src/batch/estimated_costs/worst_case_costs.rs`:
- Around line 146-152: The DeleteTree branch in the worst-case estimator
currently calls GroveDb::worst_case_merk_delete_tree without considering
IsSubtreeNonEmpty; update the GroveOp::DeleteTree handling to inspect the
IsSubtreeNonEmpty policy value and if the subtree may be non-empty either add
the additional recursive cleanup cost to the worst-case estimate (modeling child
deletion/storage clears using worst_case_layer_element_estimates, propagate and
grove_version) or explicitly reject/return an error for unsupported policies
until the estimator models them; ensure you reference the IsSubtreeNonEmpty
variant when branching and keep the existing call to worst_case_merk_delete_tree
only for the empty-subtree case.
In `@grovedb/src/batch/single_sum_item_deletion_cost_tests.rs`:
- Around line 47-52: The batch delete ops in the tests use
QualifiedGroveDbOp::delete_tree_op with TreeType::NormalTree though the trees
under test are sum trees; update those calls to use TreeType::SumTree so the
batch deletion path matches the non-batch sum-tree deletion (replace
TreeType::NormalTree with TreeType::SumTree in the delete_tree_op invocations
around the mentioned test blocks, including the occurrences at lines ~47-52 and
~155-160).
In `@grovedb/src/operations/delete/mod.rs`:
- Around line 683-688: The delete-tree op being emitted uses
IsSubtreeNonEmpty::Error which forces a redundant subtree emptiness re-check;
update the QualifiedGroveDbOp::delete_tree_op call in
delete_operation_for_delete_internal (the branch returning
Ok(Some(QualifiedGroveDbOp::delete_tree_op(...)))) to pass
IsSubtreeNonEmpty::DontCheck instead so the already-validated empty-tree path
skips the duplicate check and avoids reopening the child subtree.
---
Nitpick comments:
In `@grovedb/src/tests/batch_coverage_tests.rs`:
- Around line 153-204: Add assertions and test cases to cover the remaining
IsSubtreeNonEmpty branches Skip and DeleteChildren for non-empty trees: update
the existing tests around test_batch_delete_non_empty_tree_with_allow_option and
the sequential delete-tree tests to include operations created with
QualifiedGroveDbOp::delete_tree_op using IsSubtreeNonEmpty::Skip and
IsSubtreeNonEmpty::DeleteChildren, then assert expected behavior (Skip should
leave the non-empty tree intact and DeleteChildren should remove the tree and
its children); reference the same tree setup (e.g., "tree_with_items" and its
child) and use db.get or db.get_path checks to verify presence/absence after
applying db.apply_batch or the sequential delete methods so both batch and
sequential paths exercise these branches.
In `@grovedb/src/tests/batch_delete_tree_tests.rs`:
- Around line 225-231: Add a parallel test case that uses
IsSubtreeNonEmpty::DeleteChildren for the QualifiedGroveDbOp::delete_tree_op
call so the suite covers the new policy; specifically, create an ops vector like
the existing one but with IsSubtreeNonEmpty::DeleteChildren, execute the same
batch delete flow (calling the same test helper or invocation used for
DontCheck/Skip), and add assertions that verify the recursive children were
checked and removed (i.e., parent and descendant trees no longer exist) to
reflect the different control flow of DeleteChildren versus DontCheck.
In `@grovedb/src/tests/misc_coverage_tests.rs`:
- Around line 1485-1490: Add test cases that exercise non-Error delete policies
by duplicating the existing QualifiedGroveDbOp::delete_tree_op usages and
switching IsSubtreeNonEmpty::Error to another variant (e.g.,
IsSubtreeNonEmpty::DontCheck or IsSubtreeNonEmpty::Skip or
IsSubtreeNonEmpty::DeleteChildren) so the new policy-dependent branch is
exercised; update the ops vectors at the three other spots referenced (the
similar blocks around the other occurrences) to include at least one op with a
non-Error policy and run the same assertions for those cases to cover the
branch-specific cost/behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6ac33e5-fffd-4d01-a0e6-06edf4381f0e
📒 Files selected for processing (29)
docs/book/src/batch-operations.mddocs/book/translations/ar/src/batch-operations.mddocs/book/translations/cs/src/batch-operations.mddocs/book/translations/de/src/batch-operations.mddocs/book/translations/es/src/batch-operations.mddocs/book/translations/fr/src/batch-operations.mddocs/book/translations/id/src/batch-operations.mddocs/book/translations/it/src/batch-operations.mddocs/book/translations/ja/src/batch-operations.mddocs/book/translations/ko/src/batch-operations.mddocs/book/translations/pl/src/batch-operations.mddocs/book/translations/pt/src/batch-operations.mddocs/book/translations/ru/src/batch-operations.mddocs/book/translations/th/src/batch-operations.mddocs/book/translations/tr/src/batch-operations.mddocs/book/translations/vi/src/batch-operations.mddocs/book/translations/zh/src/batch-operations.mdgrovedb/src/batch/batch_structure.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/batch/estimated_costs/worst_case_costs.rsgrovedb/src/batch/mod.rsgrovedb/src/batch/options.rsgrovedb/src/batch/single_deletion_cost_tests.rsgrovedb/src/batch/single_sum_item_deletion_cost_tests.rsgrovedb/src/operations/delete/mod.rsgrovedb/src/tests/batch_coverage_tests.rsgrovedb/src/tests/batch_delete_tree_tests.rsgrovedb/src/tests/batch_unit_tests.rsgrovedb/src/tests/misc_coverage_tests.rs
… enum Replace the batch-level `allow_deleting_non_empty_trees` and `deleting_non_empty_trees_returns_error` flags on `BatchApplyOptions` with a per-operation `IsSubtreeNonEmpty` enum on `DeleteTree`. The new enum has four variants: - `DontCheck`: skip emptiness check (old allow=true) - `Error`: error if non-empty (old default) - `DeleteChildren`: check and recursively delete if non-empty (new) - `Skip`: silently skip if non-empty (old skip mode) This fixes two categories of platform test failures caused by PR #599: 1. Fee assertion mismatches — the old batch-level check opened every child Merk even for empty trees. Now callers that know a tree is empty can pass `DontCheck` to avoid the extra reads. 2. DeletingNonEmptyTree errors — code that relied on the batch-level flag being set can now pass `DontCheck` on the specific op instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/H1-delete-tree-per-op-emptiness-check
branch
from
March 9, 2026 16:44
3b21c63 to
d4bbdad
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #634 +/- ##
===========================================
+ Coverage 90.71% 90.80% +0.08%
===========================================
Files 182 182
Lines 51813 51821 +8
===========================================
+ Hits 47004 47057 +53
+ Misses 4809 4764 -45
🚀 New features to boost your workflow:
|
- Rename IsSubtreeNonEmpty to SubelementsDeletionBehavior - Fix DontCheck doc comment: children are NOT removed, they are left as orphaned data; callers use this when they already ensured emptiness - Use DontCheck in delete_operation_for_delete_internal since emptiness was already validated, avoiding a redundant re-check in batch mode - Exclude DeleteTree(Skip) ops from batch_deleted_keys exception set: Skip ops might not execute, so counting them as deletions could make a parent tree incorrectly appear empty - Update docs with SubelementsDeletionBehavior enum description Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
commented
Mar 9, 2026
QuantumExplorer
commented
Mar 9, 2026
QuantumExplorer
commented
Mar 9, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
grovedb/src/batch/estimated_costs/worst_case_costs.rs (1)
146-152:⚠️ Potential issue | 🟠 MajorWorst-case
DeleteTreepricing is still behavior-blind.
DeleteChildrenis the dominant worst-case path here, but this match throws awaySubelementsDeletionBehaviorand gives everyDeleteTreethe same estimate. That means recursive subtree deletion can be charged like a simple checked delete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/estimated_costs/worst_case_costs.rs` around lines 146 - 152, The DeleteTree arm discards the SubelementsDeletionBehavior making all DeleteTree ops charged the same; update the pattern from GroveOp::DeleteTree(tree_type, _) to capture the behavior (e.g., GroveOp::DeleteTree(tree_type, subelements_behavior)) and branch on SubelementsDeletionBehavior (or pass it into the cost helper) so DeleteChildren uses the recursive worst-case path while other behaviors use the cheaper estimate—adjust the call site (GroveDb::worst_case_merk_delete_tree or a new helper) to accept and use the SubelementsDeletionBehavior accordingly.grovedb/src/operations/delete/mod.rs (1)
639-648:⚠️ Potential issue | 🟠 MajorDon’t treat
DeleteTree(Skip)as a guaranteed child deletion.This blanket
DeleteTree(..)match foldsSkipintobatch_deleted_keys, so a non-empty child that will be skipped can make the parent look empty here. That can synthesize a parent delete which later fails withDeletingNonEmptyTree.Suggested direction
- .filter_map(|op| match op.op { - GroveOp::Delete | GroveOp::DeleteTree(..) => { + .filter_map(|op| match op.op { + GroveOp::Delete => { + if op.path.eq_path_vec(&subtree_merk_path_vec) { + Some(op.key.as_ref()?.as_slice()) + } else { + None + } + } + GroveOp::DeleteTree(_, SubelementsDeletionBehavior::Skip) => None, + GroveOp::DeleteTree(..) => { if op.path.eq_path_vec(&subtree_merk_path_vec) { Some(op.key.as_ref()?.as_slice()) } else { None } } _ => None, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/delete/mod.rs` around lines 639 - 648, The closure inside the filter_map currently treats all GroveOp::DeleteTree(..) as deletions, which folds DeleteTree(Skip) into batch_deleted_keys and can cause spurious parent deletes; update the match to only include DeleteTree variants that represent a real deletion and explicitly ignore the Skip variant (e.g. match GroveOp::DeleteTree(Skip) => None and GroveOp::DeleteTree(actual) => Some(...)). Keep the existing checks for op.path.eq_path_vec(&subtree_merk_path_vec) and returning op.key.as_ref()?.as_slice() for real deletes so only true child deletions are added.grovedb/src/batch/estimated_costs/average_case_costs.rs (1)
154-160:⚠️ Potential issue | 🟠 MajorAverage-case
DeleteTreepricing still drops the new behavior.This arm discards
SubelementsDeletionBehaviorand routes everyDeleteTreethrough the same estimator.DeleteChildren,Skip,Error, andDontCheckno longer do the same work, so this will misprice at least some of the new variants and undermines the fee-calculation goal of the change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/estimated_costs/average_case_costs.rs` around lines 154 - 160, The DeleteTree match arm currently ignores the Sub-elements deletion behavior and always calls GroveDb::average_case_merk_delete_tree; update the GroveOp::DeleteTree branch to inspect the second tuple element (the SubelementsDeletionBehavior value) and dispatch to the appropriate estimator for each variant (e.g., call average_case_merk_delete_tree for the legacy/DeleteChildren case, a different estimator for Skip/DontCheck/Error variants or add new functions like average_case_delete_tree_skip / average_case_delete_tree_error), passing through key, tree_type, layer_element_estimates, propagate and grove_version as needed so each Sub-elements deletion behavior is priced correctly.
♻️ Duplicate comments (2)
grovedb/src/batch/mod.rs (2)
3519-3533:⚠️ Potential issue | 🔴 CriticalResolve
Skipdeletes before derivingbatch_deleted_keys.
batch_deleted_keysis still built from the originalopslist here, butSubelementsDeletionBehavior::Skipdeletes are only filtered out later. A parentDeleteTree(Error|Skip|DeleteChildren)can therefore count a descendant skip-delete as if it will remove the child key, conclude the parent becomes empty, and delete the subtree even though that child delete is later discarded. Build the deleted-key set from the effective ops after skip resolution, or resolve these emptiness checks bottom-up.Also applies to: 3598-3615, 3871-3885, 3949-3966
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/mod.rs` around lines 3519 - 3533, The code builds batch_deleted_keys directly from ops which still contains DeleteTree entries that may later be resolved to Skip; change the logic to compute batch_deleted_keys (and batch_deleted_keys_refs) from the effective ops after subelement skip-resolution (e.g., use the resolved_ops/result of resolve_subelement_deletes or perform the skip-resolution pass first) so Skip deletes are excluded when computing parent-emptiness; update the same pattern found around the other occurrences (the blocks producing batch_deleted_keys/batch_deleted_keys_refs at the ranges mentioned) to use the post-resolution ops or perform a bottom-up emptiness check so parents don't incorrectly count skipped descendant deletes (refer to variables/function names ops, batch_deleted_keys, batch_deleted_keys_refs, child_path, and the GroveOp::Delete/ GroveOp::DeleteTree matching).
3546-3559:⚠️ Potential issue | 🔴 CriticalOpen the subtree with its real root key for emptiness checks.
Passing
NonetoMerk::open_layered_with_root_keyon an existing subtree can makeis_empty_tree_except()inspect the wrong state and misclassify a populated subtree as empty. That letsError/Skiptake the wrong branch and can delete data that should have been preserved. Reuseopen_batch_transactional_merk_at_path(...)here or otherwise fetch the stored root key before opening.Also applies to: 3898-3911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/mod.rs` around lines 3546 - 3559, The subtree is being opened with None as the root key which can make is_empty_tree_except() inspect the wrong tree state; instead obtain the subtree's actual root key and pass it into Merk::open_layered_with_root_key (or reuse open_batch_transactional_merk_at_path(...) to open the child Merk) when creating child_merk; ensure you still pass child_storage, *tree_type, Some(&Element::value_defined_cost_for_serialized_value), and grove_version, and propagate errors via the existing cost_return_on_error! wrapper so emptiness checks and subsequent Error/Skip branches operate on the correct subtree.
🧹 Nitpick comments (1)
grovedb/src/tests/batch_delete_tree_tests.rs (1)
1-8: Consider adding test coverage forSubelementsDeletionBehavior::DeleteChildren.The test file covers the
Error,Skip, andDontCheckvariants, butDeleteChildrenis not tested. According to the enum documentation,DeleteChildrenshould "check emptiness and recursively delete all children before deleting the tree itself" — behavior distinct fromDontCheck(which deletes unconditionally without checking). Adding a test case to verify the recursive child deletion behavior would complete the coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/tests/batch_delete_tree_tests.rs` around lines 1 - 8, Add a test in grovedb/src/tests/batch_delete_tree_tests.rs that covers SubelementsDeletionBehavior::DeleteChildren: create a parent tree with nested non-empty child subtrees and entries, call the function under test (batch_delete_tree / GroveDb::batch_delete_tree) with behavior = SubelementsDeletionBehavior::DeleteChildren, then assert that all child entries and subtrees were recursively deleted and the parent key is removed; ensure the test distinguishes this from DontCheck by verifying that recursive deletion happens only when DeleteChildren is set and that non-empty children are not left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/book/translations/es/src/batch-operations.md`:
- Line 16: The docs line for the DeleteTree operation is out of sync: replace
the old parameter name IsSubtreeNonEmpty with the current
SubelementsDeletionBehavior and ensure the signature lists the correct parameter
ordering/types (e.g., DeleteTree(TreeType, SubelementsDeletionBehavior)) so the
documentation matches the API; update any surrounding text that references
IsSubtreeNonEmpty to use SubelementsDeletionBehavior as well.
In `@docs/book/translations/it/src/batch-operations.md`:
- Line 16: The documentation line uses the obsolete parameter name
IsSubtreeNonEmpty for DeleteTree; update the sample to use the current
second-parameter type SubelementsDeletionBehavior (or an appropriate enum value
from SubelementsDeletionBehavior) so the signature reads DeleteTree(TreeType,
SubelementsDeletionBehavior) (or DeleteTree(TreeType, <appropriate
SubelementsDeletionBehavior value>), matching the actual API.
In `@docs/book/translations/ko/src/batch-operations.md`:
- Line 16: The documentation snippet incorrectly references a non-existent
symbol IsSubtreeNonEmpty for the DeleteTree variant; update the line using the
actual API variant DeleteTree(TreeType, SubelementsDeletionBehavior) by
replacing IsSubtreeNonEmpty with SubelementsDeletionBehavior so the docs match
the codebase's DeleteTree(TreeType, SubelementsDeletionBehavior) definition.
In `@docs/book/translations/pt/src/batch-operations.md`:
- Line 16: The snippet uses a non-existent parameter name IsSubtreeNonEmpty for
the DeleteTree variant; update the declaration to match the current public enum
signature DeleteTree(TreeType, SubelementsDeletionBehavior) by replacing
IsSubtreeNonEmpty with SubelementsDeletionBehavior in the line containing
DeleteTree(TreeType, ...), and adjust any accompanying comment text to reflect
the new parameter name (e.g., explain it controls subelements deletion
behavior).
In `@docs/book/translations/ru/src/batch-operations.md`:
- Line 16: The sample line uses a stale parameter name IsSubtreeNonEmpty for
GroveOp::DeleteTree; update the declaration to match the public API by replacing
that parameter with SubelementsDeletionBehavior (or the correct enum/type used
by GroveOp::DeleteTree) so the sample signature reads DeleteTree(TreeType,
SubelementsDeletionBehavior) and aligns with the actual GroveOp::DeleteTree
definition.
---
Outside diff comments:
In `@grovedb/src/batch/estimated_costs/average_case_costs.rs`:
- Around line 154-160: The DeleteTree match arm currently ignores the
Sub-elements deletion behavior and always calls
GroveDb::average_case_merk_delete_tree; update the GroveOp::DeleteTree branch to
inspect the second tuple element (the SubelementsDeletionBehavior value) and
dispatch to the appropriate estimator for each variant (e.g., call
average_case_merk_delete_tree for the legacy/DeleteChildren case, a different
estimator for Skip/DontCheck/Error variants or add new functions like
average_case_delete_tree_skip / average_case_delete_tree_error), passing through
key, tree_type, layer_element_estimates, propagate and grove_version as needed
so each Sub-elements deletion behavior is priced correctly.
In `@grovedb/src/batch/estimated_costs/worst_case_costs.rs`:
- Around line 146-152: The DeleteTree arm discards the
SubelementsDeletionBehavior making all DeleteTree ops charged the same; update
the pattern from GroveOp::DeleteTree(tree_type, _) to capture the behavior
(e.g., GroveOp::DeleteTree(tree_type, subelements_behavior)) and branch on
SubelementsDeletionBehavior (or pass it into the cost helper) so DeleteChildren
uses the recursive worst-case path while other behaviors use the cheaper
estimate—adjust the call site (GroveDb::worst_case_merk_delete_tree or a new
helper) to accept and use the SubelementsDeletionBehavior accordingly.
In `@grovedb/src/operations/delete/mod.rs`:
- Around line 639-648: The closure inside the filter_map currently treats all
GroveOp::DeleteTree(..) as deletions, which folds DeleteTree(Skip) into
batch_deleted_keys and can cause spurious parent deletes; update the match to
only include DeleteTree variants that represent a real deletion and explicitly
ignore the Skip variant (e.g. match GroveOp::DeleteTree(Skip) => None and
GroveOp::DeleteTree(actual) => Some(...)). Keep the existing checks for
op.path.eq_path_vec(&subtree_merk_path_vec) and returning
op.key.as_ref()?.as_slice() for real deletes so only true child deletions are
added.
---
Duplicate comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 3519-3533: The code builds batch_deleted_keys directly from ops
which still contains DeleteTree entries that may later be resolved to Skip;
change the logic to compute batch_deleted_keys (and batch_deleted_keys_refs)
from the effective ops after subelement skip-resolution (e.g., use the
resolved_ops/result of resolve_subelement_deletes or perform the skip-resolution
pass first) so Skip deletes are excluded when computing parent-emptiness; update
the same pattern found around the other occurrences (the blocks producing
batch_deleted_keys/batch_deleted_keys_refs at the ranges mentioned) to use the
post-resolution ops or perform a bottom-up emptiness check so parents don't
incorrectly count skipped descendant deletes (refer to variables/function names
ops, batch_deleted_keys, batch_deleted_keys_refs, child_path, and the
GroveOp::Delete/ GroveOp::DeleteTree matching).
- Around line 3546-3559: The subtree is being opened with None as the root key
which can make is_empty_tree_except() inspect the wrong tree state; instead
obtain the subtree's actual root key and pass it into
Merk::open_layered_with_root_key (or reuse
open_batch_transactional_merk_at_path(...) to open the child Merk) when creating
child_merk; ensure you still pass child_storage, *tree_type,
Some(&Element::value_defined_cost_for_serialized_value), and grove_version, and
propagate errors via the existing cost_return_on_error! wrapper so emptiness
checks and subsequent Error/Skip branches operate on the correct subtree.
---
Nitpick comments:
In `@grovedb/src/tests/batch_delete_tree_tests.rs`:
- Around line 1-8: Add a test in grovedb/src/tests/batch_delete_tree_tests.rs
that covers SubelementsDeletionBehavior::DeleteChildren: create a parent tree
with nested non-empty child subtrees and entries, call the function under test
(batch_delete_tree / GroveDb::batch_delete_tree) with behavior =
SubelementsDeletionBehavior::DeleteChildren, then assert that all child entries
and subtrees were recursively deleted and the parent key is removed; ensure the
test distinguishes this from DontCheck by verifying that recursive deletion
happens only when DeleteChildren is set and that non-empty children are not left
behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7b5fe2f-6682-4cc5-bcf0-65ad6bb3cfa0
📒 Files selected for processing (29)
docs/book/src/batch-operations.mddocs/book/translations/ar/src/batch-operations.mddocs/book/translations/cs/src/batch-operations.mddocs/book/translations/de/src/batch-operations.mddocs/book/translations/es/src/batch-operations.mddocs/book/translations/fr/src/batch-operations.mddocs/book/translations/id/src/batch-operations.mddocs/book/translations/it/src/batch-operations.mddocs/book/translations/ja/src/batch-operations.mddocs/book/translations/ko/src/batch-operations.mddocs/book/translations/pl/src/batch-operations.mddocs/book/translations/pt/src/batch-operations.mddocs/book/translations/ru/src/batch-operations.mddocs/book/translations/th/src/batch-operations.mddocs/book/translations/tr/src/batch-operations.mddocs/book/translations/vi/src/batch-operations.mddocs/book/translations/zh/src/batch-operations.mdgrovedb/src/batch/batch_structure.rsgrovedb/src/batch/estimated_costs/average_case_costs.rsgrovedb/src/batch/estimated_costs/worst_case_costs.rsgrovedb/src/batch/mod.rsgrovedb/src/batch/options.rsgrovedb/src/batch/single_deletion_cost_tests.rsgrovedb/src/batch/single_sum_item_deletion_cost_tests.rsgrovedb/src/operations/delete/mod.rsgrovedb/src/tests/batch_coverage_tests.rsgrovedb/src/tests/batch_delete_tree_tests.rsgrovedb/src/tests/batch_unit_tests.rsgrovedb/src/tests/misc_coverage_tests.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/book/translations/cs/src/batch-operations.md
- docs/book/src/batch-operations.md
- grovedb/src/batch/single_deletion_cost_tests.rs
- docs/book/translations/th/src/batch-operations.md
- docs/book/translations/ja/src/batch-operations.md
- grovedb/src/batch/single_sum_item_deletion_cost_tests.rs
- grovedb/src/tests/batch_coverage_tests.rs
- docs/book/translations/de/src/batch-operations.md
- docs/book/translations/vi/src/batch-operations.md
Adds 16 new tests covering previously uncovered code paths: - DeleteChildren variant for batch, partial batch, and empty tree - apply_operations_without_batching fallback for all 4 enum variants - Non-Merk tree emptiness checks (CommitmentTree with Error, Skip, DeleteChildren) - as_delete_options() in options.rs via Delete op with explicit options - Debug format including SubelementsDeletionBehavior These tests target the 66 uncovered lines in the patch (69.58% -> ~85%+). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds 18 new tests covering previously uncovered code paths: - DeleteChildren variant for batch, partial batch, and empty tree - apply_operations_without_batching fallback for all 4 enum variants - Non-Merk tree emptiness checks (CommitmentTree with Error, Skip, DeleteChildren) - as_delete_options() in options.rs via Delete op with explicit options - Debug format including SubelementsDeletionBehavior - Architectural constraint: partial child delete + DeleteTree rejected - DeleteChildren standalone with nested subtrees verifies cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests the pattern of deleting ALL children explicitly + DeleteTree parent: - Error mode: is_empty_tree_except accounts for all child deletes, succeeds - DontCheck mode: skips emptiness check entirely, succeeds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Skip mode runs the emptiness check; since all children are in the batch delete set, is_empty_tree_except reports empty and the tree is deleted normally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the behavior when deleting SOME children + DeleteTree parent: - DontCheck: apply_body rejects (child ops produce root key for deleted tree) - Skip: DeleteTree filtered out, child Delete still runs, parent survives Together with the existing Error-mode test, all 4 variants are now covered for both complete and partial child deletion scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
grovedb/src/operations/delete/mod.rs (1)
640-648:⚠️ Potential issue | 🟠 Major
DeleteTreeops should be excluded frombatch_deleted_keysto match the implementation inbatch/mod.rs.Line 640 currently includes all
DeleteTreeoperations via the patternGroveOp::DeleteTree(..), but the proper handling (atgrovedb/src/batch/mod.rslines 3524–3550 and 3891–3914) explicitly excludes DeleteTree ops with an explanatory comment: "Exclude DeleteTree ops with Skip policy — those might not execute if their target is non-empty, so we cannot assume they will delete their key." This inconsistency could cause theis_empty_tree_exceptcheck to incorrectly account for conditional deletions that may never execute.The fix is to match only
GroveOp::Delete, notGroveOp::DeleteTree:🐛 Proposed fix
let batch_deleted_keys = current_batch_operations .iter() .filter_map(|op| match op.op { - GroveOp::Delete | GroveOp::DeleteTree(..) => { + GroveOp::Delete => { if op.path.eq_path_vec(&subtree_merk_path_vec) { Some(op.key.as_ref()?.as_slice()) } else { None } } _ => None, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/delete/mod.rs` around lines 640 - 648, The batch_deleted_keys collection is incorrectly including GroveOp::DeleteTree ops; update the match in the closure so it only matches GroveOp::Delete (remove GroveOp::DeleteTree(..)) to ensure DeleteTree ops are excluded as in batch/mod.rs, so that is_empty_tree_except doesn't count conditional DeleteTree operations that may not execute.grovedb/src/batch/mod.rs (1)
3608-3612:⚠️ Potential issue | 🔴 Critical
DontCheckis still routed through recursive subtree cleanup.These paths are queued for the same cleanup pass as
DeleteChildren, so afterapply_bodya non-emptyDontCheckdelete removes nested Merk/non-Merk storage instead of leaving it orphaned. That breaks the documented contract forSubelementsDeletionBehavior::DontCheckand makes it destructive in cases where callers explicitly opted out of child cleanup.Also applies to: 3971-3975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/mod.rs` around lines 3608 - 3612, The code currently adds all child_path entries into non_merk_delete_paths or merk_delete_paths regardless of SubelementsDeletionBehavior, causing DontCheck to be treated like DeleteChildren; update the logic in the apply_body (the block that inspects tree_type.uses_non_merk_data_storage()) so it only queues child_path for recursive deletion when the deletion behavior is DeleteChildren (and preserve existing behavior for ErrorIfSubelementsExist if present), and explicitly skip adding paths when the behaviour is DontCheck so nested Merk/non-Merk storage remains orphaned; ensure you reference and check the SubelementsDeletionBehavior value used by this code path and modify the branching that populates non_merk_delete_paths and merk_delete_paths accordingly.
♻️ Duplicate comments (1)
grovedb/src/batch/mod.rs (1)
3524-3549:⚠️ Potential issue | 🟠 MajorResolve
Skipdeletes bottom-up before ancestor emptiness checks.Blanket-excluding every
DeleteTree(_, SubelementsDeletionBehavior::Skip)frombatch_deleted_keysis too conservative. A childSkipdelete against an already-empty subtree will execute, but ancestors still treat that key as surviving, so parentError/Skipdecisions can fail even though the effective delete set would leave the parent empty.Also applies to: 3889-3915
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/batch/mod.rs` around lines 3524 - 3549, The current filter for batch_deleted_keys blindly ignores GroveOp::DeleteTree(_, SubelementsDeletionBehavior::Skip), which is too conservative because a Skip DeleteTree should be treated as deleting its key if its target subtree is actually empty once other ops in the batch are applied; update the logic that builds batch_deleted_keys (the ops.iter().filter_map using other_op, GroveOp::Delete, GroveOp::DeleteTree and child_path) to first resolve deletes bottom-up: simulate/apply deletes on descendant paths in order to determine whether a Skip DeleteTree will actually execute, and include its key in batch_deleted_keys only when that simulation shows the subtree is empty; apply the same fix to the analogous block that handles the same logic elsewhere (the other DeleteTree/Skip handling block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/book/translations/de/src/batch-operations.md`:
- Line 16: The German translation is missing the SubelementsDeletionBehavior
enum docs referenced by the DeleteTree(TreeType, SubelementsDeletionBehavior)
signature; add a translated enum documentation block for
SubelementsDeletionBehavior (listing and describing the four variants:
DontCheck, Error, DeleteChildren, Skip) and insert it in the document structure
after the existing section around line 30 so the translation matches the English
source's structure and explains the new parameter.
In `@docs/book/translations/th/src/batch-operations.md`:
- Line 16: The Thai translation updated the DeleteTree signature but omitted the
SubelementsDeletionBehavior enum documentation; add the missing enum definition
block named SubelementsDeletionBehavior (same content as the English source)
back into docs/book/translations/th/src/batch-operations.md near the DeleteTree
entry so the enum variants and explanations for subelement deletion behavior are
present and match the original.
In `@docs/book/translations/zh/src/batch-operations.md`:
- Line 16: The Chinese translation updated the DeleteTree signature but omitted
the SubelementsDeletionBehavior enum documentation; add the missing
SubelementsDeletionBehavior enum documentation block to this translation
(matching the English source) near the DeleteTree(TreeType,
SubelementsDeletionBehavior) line so the enum members and their descriptions are
present for Chinese readers and consistent with the original.
In `@grovedb/src/batch/mod.rs`:
- Around line 3847-3849: The preflight scan only inspects the initial ops
iterator (for op in ops.iter()) and thus misses DeleteTree ops produced later by
add_on_operations, letting them bypass preflight and continue_partial_apply_body
without resolving Error/Skip or enqueuing subtrees for cleanup; update the flow
so that after calling add_on_operations you rescan or validate returned ops
(specifically handle GroveOp::DeleteTree from add_on_operations) and ensure they
go through the same preflight checks and are added to the cleanup queues before
calling continue_partial_apply_body, so all DeleteTree instances are treated
uniformly regardless of origin.
- Around line 3077-3100: The current handling in
apply_operations_without_batching (GroveOp::DeleteTree branch) collapses
SubelementsDeletionBehavior variants because DeleteOptions only has legacy
booleans; to fix, explicitly detect SubelementsDeletionBehavior::DeleteChildren
in that match and early-return an appropriate Error (e.g., InvalidBatchOperation
or a new specific error) from apply_operations_without_batching so callers
cannot request DeleteChildren until the lower-level single-delete API (or
DeleteOptions) is extended to accept the enum; alternatively, if you prefer to
preserve behavior, extend DeleteOptions to carry SubelementsDeletionBehavior and
propagate that enum through the single-delete call paths instead of mapping to
the two booleans.
---
Outside diff comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 3608-3612: The code currently adds all child_path entries into
non_merk_delete_paths or merk_delete_paths regardless of
SubelementsDeletionBehavior, causing DontCheck to be treated like
DeleteChildren; update the logic in the apply_body (the block that inspects
tree_type.uses_non_merk_data_storage()) so it only queues child_path for
recursive deletion when the deletion behavior is DeleteChildren (and preserve
existing behavior for ErrorIfSubelementsExist if present), and explicitly skip
adding paths when the behaviour is DontCheck so nested Merk/non-Merk storage
remains orphaned; ensure you reference and check the SubelementsDeletionBehavior
value used by this code path and modify the branching that populates
non_merk_delete_paths and merk_delete_paths accordingly.
In `@grovedb/src/operations/delete/mod.rs`:
- Around line 640-648: The batch_deleted_keys collection is incorrectly
including GroveOp::DeleteTree ops; update the match in the closure so it only
matches GroveOp::Delete (remove GroveOp::DeleteTree(..)) to ensure DeleteTree
ops are excluded as in batch/mod.rs, so that is_empty_tree_except doesn't count
conditional DeleteTree operations that may not execute.
---
Duplicate comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 3524-3549: The current filter for batch_deleted_keys blindly
ignores GroveOp::DeleteTree(_, SubelementsDeletionBehavior::Skip), which is too
conservative because a Skip DeleteTree should be treated as deleting its key if
its target subtree is actually empty once other ops in the batch are applied;
update the logic that builds batch_deleted_keys (the ops.iter().filter_map using
other_op, GroveOp::Delete, GroveOp::DeleteTree and child_path) to first resolve
deletes bottom-up: simulate/apply deletes on descendant paths in order to
determine whether a Skip DeleteTree will actually execute, and include its key
in batch_deleted_keys only when that simulation shows the subtree is empty;
apply the same fix to the analogous block that handles the same logic elsewhere
(the other DeleteTree/Skip handling block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 082d03a6-5962-4ee3-9e89-e04e70757035
📒 Files selected for processing (20)
docs/book/src/batch-operations.mddocs/book/translations/ar/src/batch-operations.mddocs/book/translations/cs/src/batch-operations.mddocs/book/translations/de/src/batch-operations.mddocs/book/translations/es/src/batch-operations.mddocs/book/translations/fr/src/batch-operations.mddocs/book/translations/id/src/batch-operations.mddocs/book/translations/it/src/batch-operations.mddocs/book/translations/ja/src/batch-operations.mddocs/book/translations/ko/src/batch-operations.mddocs/book/translations/pl/src/batch-operations.mddocs/book/translations/pt/src/batch-operations.mddocs/book/translations/ru/src/batch-operations.mddocs/book/translations/th/src/batch-operations.mddocs/book/translations/tr/src/batch-operations.mddocs/book/translations/vi/src/batch-operations.mddocs/book/translations/zh/src/batch-operations.mdgrovedb/src/batch/mod.rsgrovedb/src/operations/delete/mod.rsgrovedb/src/tests/batch_delete_tree_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/book/translations/ru/src/batch-operations.md
- docs/book/translations/es/src/batch-operations.md
- docs/book/translations/fr/src/batch-operations.md
- docs/book/translations/ar/src/batch-operations.md
- docs/book/translations/ko/src/batch-operations.md
| RefreshReference { reference_path_type, max_reference_hop, flags, trust_refresh_reference }, | ||
| Delete, | ||
| DeleteTree(TreeType), // Parametrisiert nach Baumtyp | ||
| DeleteTree(TreeType, SubelementsDeletionBehavior), // Per-op deletion policy |
Contributor
There was a problem hiding this comment.
Translation is missing the SubelementsDeletionBehavior enum documentation.
The English source documentation (docs/book/src/batch-operations.md) includes a dedicated section documenting the SubelementsDeletionBehavior enum with all four variants (DontCheck, Error, DeleteChildren, Skip). This German translation updates the DeleteTree signature but omits the corresponding enum definition block, leaving readers without an explanation of the new parameter.
Consider adding the translated enum documentation after line 30 to match the English source structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/book/translations/de/src/batch-operations.md` at line 16, The German
translation is missing the SubelementsDeletionBehavior enum docs referenced by
the DeleteTree(TreeType, SubelementsDeletionBehavior) signature; add a
translated enum documentation block for SubelementsDeletionBehavior (listing and
describing the four variants: DontCheck, Error, DeleteChildren, Skip) and insert
it in the document structure after the existing section around line 30 so the
translation matches the English source's structure and explains the new
parameter.
| RefreshReference { reference_path_type, max_reference_hop, flags, trust_refresh_reference }, | ||
| Delete, | ||
| DeleteTree(TreeType), // Parameterized by tree type | ||
| DeleteTree(TreeType, SubelementsDeletionBehavior), // Per-op deletion policy |
Contributor
There was a problem hiding this comment.
Translation is missing the SubelementsDeletionBehavior enum documentation.
Same as other translations, this Chinese translation updates the DeleteTree signature but omits the SubelementsDeletionBehavior enum definition block from the English source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/book/translations/zh/src/batch-operations.md` at line 16, The Chinese
translation updated the DeleteTree signature but omitted the
SubelementsDeletionBehavior enum documentation; add the missing
SubelementsDeletionBehavior enum documentation block to this translation
(matching the English source) near the DeleteTree(TreeType,
SubelementsDeletionBehavior) line so the enum members and their descriptions are
present for Chinese readers and consistent with the original.
Comment on lines
+3077
to
+3100
| GroveOp::DeleteTree(_, subelements_deletion_behavior) => { | ||
| let path_slices: Vec<&[u8]> = | ||
| op.path.iterator().map(|p| p.as_slice()).collect(); | ||
| let key = cost_return_on_error_no_add!( | ||
| cost, | ||
| op.key | ||
| .as_ref() | ||
| .ok_or(Error::InvalidBatchOperation("delete op is missing a key")) | ||
| ); | ||
| let delete_options = DeleteOptions { | ||
| allow_deleting_non_empty_trees: matches!( | ||
| subelements_deletion_behavior, | ||
| SubelementsDeletionBehavior::DontCheck | ||
| | SubelementsDeletionBehavior::DeleteChildren | ||
| ), | ||
| deleting_non_empty_trees_returns_error: matches!( | ||
| subelements_deletion_behavior, | ||
| SubelementsDeletionBehavior::Error | ||
| ), | ||
| base_root_storage_is_free: options | ||
| .as_ref() | ||
| .is_none_or(|o| o.base_root_storage_is_free), | ||
| validate_tree_at_path_exists: false, | ||
| }; |
Contributor
There was a problem hiding this comment.
apply_operations_without_batching() collapses the new DeleteTree semantics.
DeleteOptions in grovedb/src/operations/delete/mod.rs:83-92 still only carries the two legacy booleans, so SubelementsDeletionBehavior::DeleteChildren and SubelementsDeletionBehavior::DontCheck are serialized to the same request here. That means this public path cannot preserve both behaviors; either thread the enum through the single-delete API or reject DeleteChildren in this method until the lower layer can distinguish it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@grovedb/src/batch/mod.rs` around lines 3077 - 3100, The current handling in
apply_operations_without_batching (GroveOp::DeleteTree branch) collapses
SubelementsDeletionBehavior variants because DeleteOptions only has legacy
booleans; to fix, explicitly detect SubelementsDeletionBehavior::DeleteChildren
in that match and early-return an appropriate Error (e.g., InvalidBatchOperation
or a new specific error) from apply_operations_without_batching so callers
cannot request DeleteChildren until the lower-level single-delete API (or
DeleteOptions) is extended to accept the enum; alternatively, if you prefer to
preserve behavior, extend DeleteOptions to carry SubelementsDeletionBehavior and
propagate that enum through the single-delete call paths instead of mapping to
the two booleans.
Comment on lines
3847
to
3849
| for op in ops.iter() { | ||
| if let GroveOp::DeleteTree(tree_type) = &op.op | ||
| if let GroveOp::DeleteTree(tree_type, subelements_deletion_behavior) = &op.op | ||
| && let Some(key) = op.key.as_ref() |
Contributor
There was a problem hiding this comment.
Partial-batch add-on DeleteTree ops bypass the new preflight entirely.
This scan only walks the initial ops. Any DeleteTree returned later from add_on_operations goes straight into continue_partial_apply_body, so Error/Skip are never resolved there and the subtree never gets added to the cleanup queues. In that path, callback-generated deletes can degrade into unconditional deletes or leak subtree storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@grovedb/src/batch/mod.rs` around lines 3847 - 3849, The preflight scan only
inspects the initial ops iterator (for op in ops.iter()) and thus misses
DeleteTree ops produced later by add_on_operations, letting them bypass
preflight and continue_partial_apply_body without resolving Error/Skip or
enqueuing subtrees for cleanup; update the flow so that after calling
add_on_operations you rescan or validate returned ops (specifically handle
GroveOp::DeleteTree from add_on_operations) and ensure they go through the same
preflight checks and are added to the cleanup queues before calling
continue_partial_apply_body, so all DeleteTree instances are treated uniformly
regardless of origin.
- Add SubelementsDeletionBehavior enum docs to all 16 translation files - Add explanatory comment for DeleteChildren/DontCheck semantic collapse in apply_operations_without_batching - Document add-on ops limitation bypassing preflight checks - Replace open_layered_with_root_key(..., None, ...) with open_batch_transactional_merk_at_path for correct root key in emptiness checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
4 tasks
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
1 participant
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.
Summary
allow_deleting_non_empty_treesanddeleting_non_empty_trees_returns_errorflags onBatchApplyOptionswith a per-operationIsSubtreeNonEmptyenum onGroveOp::DeleteTreeDontCheck,Error,DeleteChildren,Skipdelete_tree_op()anddelete_estimated_tree_op()now take a 4th parameter specifying the emptiness policydelete_tree_opmust add the new parameterMotivation
PR #599 (batch DeleteTree emptiness enforcement) caused two categories of test failures in Platform:
Fee assertion mismatches — the batch-level emptiness check opens the child Merk for every
DeleteTreeop, even when the tree IS empty and the check passes. This adds seek/storage costs that change processing fees.DeletingNonEmptyTree errors — code that relied on
allow_deleting_non_empty_trees: trueat the batch level now needs a way to express this per-op, since different ops in the same batch may need different policies.Moving the policy to each
DeleteTreeop solves both: callers that know a tree is empty passDontCheck(zero extra cost), and callers that need to force-delete passDontCheckon just that op rather than the entire batch.Migration guide
BatchApplyOptionsflagsIsSubtreeNonEmptyvariantallow_deleting_non_empty_trees: trueIsSubtreeNonEmpty::DontCheckallow_deleting_non_empty_trees: false, deleting_non_empty_trees_returns_error: true(default)IsSubtreeNonEmpty::Errorallow_deleting_non_empty_trees: false, deleting_non_empty_trees_returns_error: falseIsSubtreeNonEmpty::SkipIsSubtreeNonEmpty::DeleteChildrenTest plan
cargo clippy -- -D warningscleancargo build --features full,estimated_costsclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Breaking Changes