Skip to content

fix(delete): preserve parent type for non-empty subtree deletes - #732

Open
QuantumExplorer wants to merge 9 commits into
developfrom
codex/fix-686-delete-parent-tree-type
Open

fix(delete): preserve parent type for non-empty subtree deletes#732
QuantumExplorer wants to merge 9 commits into
developfrom
codex/fix-686-delete-parent-tree-type

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • reuse the already-open parent Merk when deleting a non-empty child tree
  • keep delete propagation using the parent tree type instead of reopening with the child tree type
  • add CountTree and CountSumTree regressions that delete populated child trees and verify aggregates

Fixes #686.

Verification

  • cargo test -p grovedb non_empty_tree_delete_under --lib
  • cargo test -p grovedb test_recurring_deletion_through_subtrees --lib

Summary by CodeRabbit

  • New Features

    • Delete operation support enabled for GroveDB v3 protocol.
  • Improvements

    • Enhanced error handling and messages for delete operations.
  • Tests

    • Added comprehensive test coverage for delete operations across protocol versions, including backward compatibility validation.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dd897149-e228-4f44-8b24-1338d8ba4769

📥 Commits

Reviewing files that changed from the base of the PR and between a2129a9 and 38944e8.

📒 Files selected for processing (2)
  • grovedb-version/src/version/v3.rs
  • grovedb/src/operations/delete/mod.rs

📝 Walkthrough

Walkthrough

This PR enables delete_internal_on_transaction for GroveDB v3 and fixes a critical bug where non-empty tree deletion under aggregate parents (CountTree, sum, count-sum) could reopen the parent using the child's tree type instead of the parent's, corrupting counter/sum metadata. The implementation introduces version-dependent logic with a legacy v0 path and modern v1+ path, ensuring consistent use of parent tree type throughout.

Changes

Delete operation v1 versioning and aggregation fix

Layer / File(s) Summary
Version flag configuration
grovedb-version/src/version/v3.rs
GROVE_V3 protocol version enables delete_internal_on_transaction operation by updating its version flag from 0 to 1.
Version check infrastructure and error handling
grovedb/src/operations/delete/mod.rs
Imports check_grovedb_v0_or_v1_with_cost function, adds cannot_open_subtree_with_root_key_error helper to convert subtree opening failures into Error::CorruptedData, and switches delete_internal_on_transaction version gate macro from v0-only to v0-or-v1 checking.
Version-dependent deletion with parent tree type
grovedb/src/operations/delete/mod.rs
Introduces parent_tree_type variable saved from parent subtree. Reworks non-empty tree deletion into version-dependent branches: v0 legacy path reopens parent storage using subtree root key (with error mapping) then performs sectioned deletion; v1+ path deletes directly on already-open transactional merk. Both paths consistently use parent_tree_type when calling Element::delete_with_sectioned_removal_bytes.
Error mapping and aggregate parent deletion tests
grovedb/src/operations/delete/mod.rs
Imports GROVE_V2 for legacy tests. Adds test_open_subtree_root_key_error_mapping to verify error conversion. Adds comprehensive tests deleting non-empty children under count/count-sum aggregate parents, verifying parent counter/sum metadata decreases correctly, verify_grovedb returns no issues, and a GROVE_V2 compatibility test ensures legacy v0 deletion path behavior is preserved.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops through versions true,
Where v0 paths meet v1 anew.
Parent trees stay pure and bright,
Aggregate counts held ever tight.
Delete flows smooth, both old and new! 🌳

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing delete operations to preserve parent type when handling non-empty subtree deletes.
Linked Issues check ✅ Passed The PR addresses issue #686 by implementing version-dependent delete behavior: v0 uses legacy reopening (child type) for backward compatibility, v1 reuses open parent Merk (parent type), with comprehensive test coverage for CountTree/CountSumTree aggregate propagation.
Out of Scope Changes check ✅ Passed All changes are in-scope: version flag update for v3 support, delete operation logic refactoring with version dispatching, helper function for error mapping, and targeted test additions for aggregate validation.
Docstring Coverage ✅ Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-686-delete-parent-tree-type

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.47%. Comparing base (60f2968) to head (38944e8).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #732      +/-   ##
===========================================
+ Coverage    91.42%   91.47%   +0.04%     
===========================================
  Files          236      236              
  Lines        67053    67321     +268     
===========================================
+ Hits         61305    61580     +275     
+ Misses        5748     5741       -7     
Components Coverage Δ
grovedb-core 89.06% <100.00%> (+0.11%) ⬆️
merk 92.25% <ø> (-0.02%) ⬇️
storage 86.36% <ø> (ø)
commitment-tree 96.43% <ø> (ø)
mmr 96.79% <ø> (+0.03%) ⬆️
bulk-append-tree 89.39% <ø> (+0.13%) ⬆️
element 97.38% <ø> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@QuantumExplorer QuantumExplorer changed the title Reuse parent Merk when deleting non-empty subtrees fix(delete): preserve parent type for non-empty subtree deletes May 21, 2026
@QuantumExplorer
QuantumExplorer force-pushed the codex/fix-686-delete-parent-tree-type branch from 5fceb53 to ec1442c Compare May 21, 2026 03:56
QuantumExplorer and others added 5 commits May 21, 2026 18:46
…tion

Replace the inline `if version == 0` branch with the established
mod.rs-dispatcher + v0.rs/v1.rs pattern used elsewhere (e.g. merk
add_average_case_merk_propagate). v0 preserves grove v1/v2 behavior
(reopen parent with child tree type); v1 (grove v3+) reuses the open
parent merk so aggregate propagation uses the parent tree type.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cost_return_on_error_into, ElementCostExtensions and
ElementDeleteFromStorageExtensions are no longer used in delete/mod.rs
(moved into v0/v1); StorageContext is unused in v0/v1 since clear() is
inherent. Required for CI's clippy -D warnings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The legacy reopen path now lives in v0.rs; route its map_err through the
shared helper added in cbdded1 so the helper is exercised by production
code (not just its unit test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@QuantumExplorer QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

QuantumExplorer added a commit that referenced this pull request Jul 30, 2026
* feat(version): add GROVE_V4, behaviourally identical to V3

GROVE_V3 is live, so a fix that changes an accepted/rejected outcome, a
committed root hash, or a tracked cost cannot be applied unconditionally —
nodes carrying it would diverge from nodes that do not. There is currently
nowhere for such a fix to land, which has left several of them stuck:

  - #776: overwriting an indexed tree with a bare Reference skips the
    per-axis secondary cleanup. Closing it costs an extra stored-element read
    on EVERY reference overwrite (+1 seek, +79 storage_loaded_bytes, measured
    by the refresh-reference cost tests), and references over plain trees are
    shipped functionality.
  - Batch DeleteTree treats the caller-declared tree type as authoritative
    when selecting cleanup namespaces. Reading the stored element instead
    fixes both an indexed type-confusion and a live CommitmentTree
    wrong-emptiness-path bug, but adds a read to a released path.
  - Per project notes, five audit-fix PRs (#726, #730, #732, #734, #739)
    are gated on v3 and need re-gating before they can merge.

This adds the version and nothing else. Every method-version slot is copied
from V3 unchanged, so activating protocol version 4 today is a no-op; each
gate is a deliberate, separately-reviewable slot bump.

Verified rather than assumed: registering V4 changes what
`GroveVersion::latest()` resolves to, and the whole test suite defaults to
latest. The full workspace suite passes with V4 as latest (2459 grovedb + 705
merk + the rest), and the only two failures were the version registry's own
self-describing tests — `grove_version_latest_returns_v3` and
`grove_versions_count` — which are updated here. That is the evidence the
change is inert.

Adds `grove_v4_is_behaviourally_identical_to_v3_until_a_gate_is_added`, which
compares every slot and fails the moment one is bumped. That failure is the
intended prompt to document the gate rather than let V4 accrete behaviour
silently.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(version): drop the V3/V4 slot-identity assertion

It pinned V4's initial state as inert, which was worth verifying once but
becomes churn the moment a gate is added — and the DeleteTree read and #776
are both queued to gate on V4 next, so it would fail immediately and be
deleted anyway.

The evidence it provided is preserved where it belongs: the PR description
records that the full workspace suite passed with V4 as latest and that only
the registry's own self-describing tests changed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[audit][medium] Non-empty tree delete can reopen parent with child tree type

1 participant