fix: eliminate unnecessary cleanup I/O costs for empty tree deletions - #639
Merged
Conversation
…skip emptiness check Previously DontCheck and DeleteChildren had nearly identical behavior: both added the deleted subtree to the post-apply cleanup list, but DontCheck skipped the emptiness preflight check while DeleteChildren ran it (then ignored the result). This meant every DeleteTree(NormalTree, DontCheck) op paid find_subtrees + storage.clear I/O costs even when the caller knew the tree was empty. Now: - DontCheckWithNoCleanup: skips both emptiness check AND post-apply storage cleanup. For callers who guarantee the subtree is already empty (e.g. batch_delete_up_tree_while_empty). - DeleteChildren: skips the emptiness check but still performs post-apply storage cleanup. For callers deleting trees that may have children needing recursive cleanup. This eliminates the unnecessary I/O costs (find_subtrees + clear) that were being charged as processing fees when Platform deleted known-empty index trees during document transfers and deletions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 (1)
📝 WalkthroughWalkthroughThis PR renames the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grovedb/src/tests/batch_delete_tree_tests.rs (1)
116-121:⚠️ Potential issue | 🟠 MajorThese tests now model
DontCheckWithNoCleanupon a known non-empty tree.That variant’s new contract is “caller guarantees tree is empty.” Using it here locks in unsupported usage instead of the intended non-empty deletion paths. If these tests are meant to cover sanctioned non-empty deletion, switch them to
DeleteChildren; otherwise rename the tests/comments to make clear they are exercising the unsafe no-cleanup bypass rather than the supported contract.Also applies to: 1652-1661
🤖 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 116 - 121, The tests call QualifiedGroveDbOp::delete_tree_op with SubelementsDeletionBehavior::DontCheckWithNoCleanup against a known non-empty tree, which violates that variant's "caller guarantees tree is empty" contract; either change the behavior to SubelementsDeletionBehavior::DeleteChildren to exercise the supported non-empty deletion path (update the ops vec at the sites using delete_tree_op), or if you intentionally want to test the unsafe bypass, rename the test and comments to explicitly state it exercises the unsafe DontCheckWithNoCleanup behavior; update all occurrences (e.g., the ops construction at the shown location and the similar block at lines ~1652-1661) accordingly.
🧹 Nitpick comments (1)
grovedb/src/batch/mod.rs (1)
5583-5590: Add one regression for theDontCheckWithNoCleanupbranch too.These updates keep the cleanup-required path covered, but the PR’s main behavior split is that
DontCheckWithNoCleanupmust avoid the cleanup work entirely. A companion regression in the cost tests or delete-tree tests would make the restored fee behavior much harder to regress.Also applies to: 5670-5676, 5747-5753
🤖 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 5583 - 5590, The PR adds regressions for the cleanup-required path but missed adding a test for the DontCheckWithNoCleanup path; add a companion regression that constructs a delete-tree op using QualifiedGroveDbOp::delete_tree_op with SubelementsDeletionBehavior::DontCheckWithNoCleanup (instead of DeleteChildren) and assert that no cleanup/fee-related side-effects occur (mirror the existing test structure used around the delete-tree/cost tests), so the restored-no-cleanup behavior is covered; place this new test alongside the other delete-tree regressions that use DeleteChildren to ensure both branches (DeleteChildren and DontCheckWithNoCleanup) are exercised.
🤖 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/pt/src/batch-operations.md`:
- Around line 47-49: The enum variant descriptions for
SubelementsDeletionBehavior are still in English in the localized page; update
the inline comments for DontCheckWithNoCleanup, Error, and DeleteChildren to
Portuguese equivalents, preserving the original meaning (skip emptiness check
and skip post-apply cleanup; return Error::DeletingNonEmptyTree if non-empty;
skip emptiness check but perform post-apply storage cleanup) so the localized
page matches the renamed enum documentation; apply the same translation change
to the other translated batch-operations.md files that were updated by this
rename.
In `@docs/book/translations/zh/src/batch-operations.md`:
- Around line 47-49: The inline comments for the enum variants
DontCheckWithNoCleanup, Error, and DeleteChildren are still English in the
Chinese translation page; update those three comments to Chinese to match the
surrounding localized prose (translate "Skip emptiness check AND post-apply
cleanup; caller guarantees tree is empty", "Return Error::DeletingNonEmptyTree
if non-empty", and "Skip emptiness check, but perform post-apply storage
cleanup" into concise Chinese and replace the existing English comments next to
DontCheckWithNoCleanup, Error, and DeleteChildren).
In `@grovedb/src/batch/mod.rs`:
- Around line 3088-3099: The non-batch path apply_operations_without_batching()
incorrectly treats SubelementsDeletionBehavior::DontCheckWithNoCleanup like
DeleteChildren (setting allow_deleting_non_empty_trees=true) which causes
recursive cleanup; change the mapping so DontCheckWithNoCleanup is treated like
Skip (i.e., do NOT set allow_deleting_non_empty_trees and ensure the delete call
uses error=false/returns Ok(false) for non-empty trees). Concretely, update the
DeleteOptions construction (the allow_deleting_non_empty_trees matches(...)
expression) to only include SubelementsDeletionBehavior::DeleteChildren, and
ensure any downstream logic that sets the “error on non-empty” behavior for
DeleteTree respects DontCheckWithNoCleanup as skip (allow=false + error=false).
In `@grovedb/src/operations/delete/mod.rs`:
- Around line 683-690: The branch currently returns
QualifiedGroveDbOp::delete_tree_op(...,
SubelementsDeletionBehavior::DontCheckWithNoCleanup) based on an emptiness
preflight that counts all nested DeleteTree ops as deletions; update the
emptiness check to mirror the batch-path logic by filtering out any nested
DeleteTree(_, Skip) ops before deciding the fast no-cleanup path so that
DontCheckWithNoCleanup is only returned when the preflight is exact; locate the
code that computes emptiness for the delete_tree_op (references:
QualifiedGroveDbOp::delete_tree_op,
SubelementsDeletionBehavior::DontCheckWithNoCleanup, and DeleteTree(_, Skip))
and exclude Skip children from that calculation prior to returning the
DontCheckWithNoCleanup variant.
---
Outside diff comments:
In `@grovedb/src/tests/batch_delete_tree_tests.rs`:
- Around line 116-121: The tests call QualifiedGroveDbOp::delete_tree_op with
SubelementsDeletionBehavior::DontCheckWithNoCleanup against a known non-empty
tree, which violates that variant's "caller guarantees tree is empty" contract;
either change the behavior to SubelementsDeletionBehavior::DeleteChildren to
exercise the supported non-empty deletion path (update the ops vec at the sites
using delete_tree_op), or if you intentionally want to test the unsafe bypass,
rename the test and comments to explicitly state it exercises the unsafe
DontCheckWithNoCleanup behavior; update all occurrences (e.g., the ops
construction at the shown location and the similar block at lines ~1652-1661)
accordingly.
---
Nitpick comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 5583-5590: The PR adds regressions for the cleanup-required path
but missed adding a test for the DontCheckWithNoCleanup path; add a companion
regression that constructs a delete-tree op using
QualifiedGroveDbOp::delete_tree_op with
SubelementsDeletionBehavior::DontCheckWithNoCleanup (instead of DeleteChildren)
and assert that no cleanup/fee-related side-effects occur (mirror the existing
test structure used around the delete-tree/cost tests), so the
restored-no-cleanup behavior is covered; place this new test alongside the other
delete-tree regressions that use DeleteChildren to ensure both branches
(DeleteChildren and DontCheckWithNoCleanup) are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b1e9420-b569-44df-8c99-5886bd442a87
📒 Files selected for processing (21)
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_coverage_tests.rsgrovedb/src/tests/batch_delete_tree_tests.rs
DeleteChildren now checks emptiness to decide whether cleanup is needed. Empty trees skip the post-apply find_subtrees + storage.clear phase since there's nothing to clean up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 48424b6.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #639 +/- ##
===========================================
- Coverage 90.83% 90.83% -0.01%
===========================================
Files 182 182
Lines 51898 51895 -3
===========================================
- Hits 47142 47138 -4
- Misses 4756 4757 +1
🚀 New features to boost your workflow:
|
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
Mar 10, 2026
Updates grovedb to include dashpay/grovedb#639 which renames DontCheck to DontCheckWithNoCleanup, eliminating unnecessary find_subtrees + storage.clear() I/O costs for known-empty tree deletions during document transfers and deletions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 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
SubelementsDeletionBehavior::DontChecktoDontCheckWithNoCleanup— now skips both the emptiness preflight check AND the post-apply storage cleanup phaseDeleteChildrento skip the emptiness check but still perform post-apply storage cleanupDeleteTreeop, even when Platform was deleting known-empty index trees viabatch_delete_up_tree_while_emptyBefore
DontCheckandDeleteChildrenhad nearly identical behavior — both added deleted subtrees to the cleanup list. The only difference wasDeleteChildrenran an emptiness check first (then ignored the result for non-empty trees). EveryDeleteTree(NormalTree, DontCheck)op paidfind_subtrees+storage.clear()I/O costs even for empty trees.After
DontCheckWithNoCleanupDontCheckWithNoCleanupDeleteChildrenDeleteChildrenErrorErrorSkipSkipFee impact
Platform uses
DontCheck(nowDontCheckWithNoCleanup) forbatch_delete_up_tree_while_empty, which prunes known-empty index trees during document transfers/deletions. These ops no longer incur find_subtrees + clear costs, restoring the previous fee levels.Test plan
DeleteChildrenDontCheckWithNoCleanup🤖 Generated with Claude Code