fix: batch DeleteTree now enforces emptiness check and cleans up subtree storage (H1, H2) - #599
Conversation
…eteTree Batch DeleteTree had two issues: H1: When DeleteTree ran in batch mode for standard Merk trees, only the parent key was removed. Child subtree storage and all nested data remained as orphans, causing corruption when re-inserting at the same path. Now after apply_body, we call find_subtrees to recursively discover all nested subtrees and clear their storage, matching the non-batch delete behavior. H2: The batch path unconditionally deleted tree elements without checking is_empty_tree(). The allow_deleting_non_empty_trees option in BatchApplyOptions was never consulted. Now before apply_body, we check subtree emptiness (using is_empty_tree_except for Merk trees and non_merk_entry_count for non-Merk trees) and respect the batch options. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #599 +/- ##
===========================================
- Coverage 90.67% 90.62% -0.05%
===========================================
Files 182 182
Lines 49913 50534 +621
===========================================
+ Hits 45258 45797 +539
- Misses 4655 4737 +82
🚀 New features to boost your workflow:
|
…d InsertIfNotExists Add 19 tests covering previously uncovered code paths in the batch processing changes: - Batch DeleteTree skip mode (allow=false, error=false): verify non-empty tree deletion is silently skipped while the tree remains intact - Batch DeleteTree with simultaneous child deletion: test that is_empty_tree_except correctly accounts for children being deleted in the same batch - Batch DeleteTree with partial child deletion: verify tree is still considered non-empty when only some children are deleted - Partial batch (apply_partial_batch) emptiness checks: error mode, skip mode, and allow mode with recursive Merk cleanup - Partial batch with simultaneous child deletion - Transactional batch emptiness check - Deeply nested subtree cleanup (3+ levels) - Skip mode mixed batch (skipped DeleteTree + successful DeleteTree + insert) - InsertIfNotExists: success for new key, error when key exists - InsertIfNotExists or skip: silently skip existing key - InsertWithKnownToNotAlreadyExist: success for new key - InsertIfNotExists through apply_batch_with_element_flags_update path - Debug format coverage for new InsertIfNotExists/InsertWithKnownToNotAlreadyExist variants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
left a comment
There was a problem hiding this comment.
Approved, this would cause Platform to no longer work, but PR dashpay/platform#3210 will solve that on Platform side.
… 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>
… 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>
* fix: move DeleteTree emptiness check from BatchApplyOptions to per-op 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> * fix: address CodeRabbit review findings - 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> * test: add coverage tests for SubelementsDeletionBehavior variants 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> * test: add coverage tests for SubelementsDeletionBehavior variants 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> * test: add complete-children-delete tests with Error and DontCheck modes 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> * test: add complete-children-delete test with Skip mode 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> * test: add partial-children-delete tests with DontCheck and Skip modes 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> * fix: address CodeRabbit review comments on PR #634 - 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> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Two related fixes for batch DeleteTree operations:
H2 — Emptiness check: Before
apply_bodyruns, eachDeleteTreeop is now checked for subtree emptiness whenallow_deleting_non_empty_treesis false. Standard Merk trees useis_empty_tree_except(batch_deleted_keys)to correctly account for sibling deletes in the same batch. Non-Merk trees useelement.non_merk_entry_count(). When a non-empty tree is found anddeleting_non_empty_trees_returns_erroris true, returnsError::DeletingNonEmptyTree; otherwise silently skips the op.H1 — Storage cleanup: After
apply_bodycompletes, for each standard Merk tree deletion,find_subtreesrecursively discovers all nested subtrees andstorage.clear()is applied to each. This matches the non-batch delete behavior indelete_internal_on_transaction. Previously, only the parent key was removed, leaving orphaned storage data that could corrupt a later re-insert at the same path.Both fixes apply to the full batch and partial batch paths.
Test plan
test_batch_delete_tree_non_empty_should_fail_when_not_allowed— H2: error on non-empty tree deletetest_batch_delete_tree_non_empty_succeeds_when_allowed— H2: success when allowedtest_batch_delete_empty_tree_succeeds— H2: empty tree always workstest_batch_delete_tree_cleans_up_subtree_storage— H1: no orphaned storage after deletetest_batch_delete_tree_then_reinsert_produces_clean_tree— H1: clean re-insertion after deleteallow_deleting_non_empty_trees: true(they intentionally delete non-empty trees)🤖 Generated with Claude Code