Skip to content

fix(grovedb): sweep nested indexed secondaries in every recursive cleanup (#888) #934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
QuantumExplorer merged 2 commits into from
Sep 7, 2026
Merged

fix(grovedb): sweep nested indexed secondaries in every recursive cleanup (#888) #934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
106abba
fix(grovedb): sweep nested indexed secondaries in every recursive cle…
QuantumExplorer Sep 7, 2026
970b973
fix: preserve versioned batch cleanup costs
QuantumExplorer Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions grovedb-version/src/version/grovedb_versions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,18 @@ pub struct GroveDBApplyBatchVersions {
/// an accepted/rejected outcome — a mismatched delete that V1..V3
/// accept is refused on V4+ when an indexed tree is involved.
pub delete_tree_cleanup_type_source: FeatureVersion,
/// Whether full and partial batch `DeleteTree` cleanup sweeps every
/// discovered subtree's indexed secondary namespaces (issue #888).
///
/// - `0` (V1..V3): recursively clear primary namespaces only, retaining
/// the separate top-level indexed-secondary pass and its costs.
/// - `1` (V4+): also sweep all three secondary axes for every subtree.
///
/// Empty secondary sweeps still charge seeks, boundary reads, and
/// prefix hashes, including for ordinary trees. These costs must not
/// change on released versions. Direct deletion is unaffected: it
/// already swept secondaries recursively on every version.
pub delete_tree_recursive_secondary_cleanup: FeatureVersion,
/// Whether a batch overwrite (`InsertOrReplace` / `Replace` / `Patch`,
/// with tree-override protection off) classifies the element it
/// displaces to detect an indexed tree being overwritten.
Expand Down
1 change: 1 addition & 0 deletions grovedb-version/src/version/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub const GROVE_V1: GroveVersion = GroveVersion {
apply_partial_batch_with_element_flags_update: 0,
estimated_case_operations_for_batch: 0,
delete_tree_cleanup_type_source: 0,
delete_tree_recursive_secondary_cleanup: 0,
overwrite_indexed_cleanup_inspection: 0,
keyless_op_cost_dispatch: 0,
add_on_op_collision: 0,
Expand Down
1 change: 1 addition & 0 deletions grovedb-version/src/version/v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub const GROVE_V2: GroveVersion = GroveVersion {
apply_partial_batch_with_element_flags_update: 0,
estimated_case_operations_for_batch: 0,
delete_tree_cleanup_type_source: 0,
delete_tree_recursive_secondary_cleanup: 0,
overwrite_indexed_cleanup_inspection: 0,
keyless_op_cost_dispatch: 0,
add_on_op_collision: 0,
Expand Down
1 change: 1 addition & 0 deletions grovedb-version/src/version/v3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub const GROVE_V3: GroveVersion = GroveVersion {
apply_partial_batch_with_element_flags_update: 0,
estimated_case_operations_for_batch: 0,
delete_tree_cleanup_type_source: 0,
delete_tree_recursive_secondary_cleanup: 0,
overwrite_indexed_cleanup_inspection: 0,
keyless_op_cost_dispatch: 0,
add_on_op_collision: 0,
Expand Down
8 changes: 8 additions & 0 deletions grovedb-version/src/version/v4.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@
//! observer), so V4 charges exactly the V1..V3 cost — the gate exists
//! because it flips an accepted/rejected outcome, not because of cost.
//!
//! - `apply_batch.delete_tree_recursive_secondary_cleanup: 1` — full and
//! partial batch `DeleteTree` cleanup sweeps each descendant's indexed
//! secondary namespaces (issue #888). V1..V3 retain primary-only recursion
//! and the separate top-level secondary pass: even empty secondary sweeps
//! charge additional seeks, reads, and hashes for ordinary trees. Direct
//! deletion already swept descendants' secondaries and is unchanged.
//!
//! - `apply_batch.overwrite_indexed_cleanup_inspection: 1` — a batch
//! overwrite (with tree-override protection off, references included)
//! classifies the element it displaces to detect an indexed tree being
Expand Down Expand Up @@ -336,6 +343,7 @@ pub const GROVE_V4: GroveVersion = GroveVersion {
apply_partial_batch_with_element_flags_update: 0,
estimated_case_operations_for_batch: 0,
delete_tree_cleanup_type_source: 1,
delete_tree_recursive_secondary_cleanup: 1,
overwrite_indexed_cleanup_inspection: 1,
keyless_op_cost_dispatch: 1,
add_on_op_collision: 1,
Expand Down
209 changes: 63 additions & 146 deletions grovedb/src/batch/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6761,8 +6761,13 @@ impl GroveDb {
// Clean up storage for deleted standard Merk subtrees.
// The parent key has been removed from the parent Merk by apply_body,
// but the child subtree's storage (and any nested subtrees) remains.
// We use find_subtrees to recursively discover all nested subtrees
// and clear their storage, matching the non-batch delete behavior.
// The shared recursive cleanup discovers all nested subtrees via
// find_subtrees and clears each one's primary namespace AND its
// per-axis indexed-tree secondary namespaces (issue #888 — a nested
// indexed primary's secondaries live outside the path-prefix walk),
// matching the non-batch delete behavior on V4+. V1..V3 retain
// primary-only recursion because even empty secondary sweeps
// charge additional costs for ordinary trees.
//
// NOTE: find_subtrees reads from the committed transaction state
// (without the pending storage_batch), so any subtrees *inserted*
Expand All @@ -6774,25 +6779,21 @@ impl GroveDb {
// BatchApplyOptions::disable_operation_consistency_check.
for child_path in &merk_delete_paths {
let child_subtree_path: SubtreePath<Vec<u8>> = child_path.as_slice().into();
let subtrees_paths = cost_return_on_error!(
cost_return_on_error!(
&mut cost,
self.find_subtrees(&child_subtree_path, Some(tx.as_ref()), grove_version)
self.clear_subtree_storage_recursively(
&child_subtree_path,
tx.as_ref(),
&storage_batch,
grove_version
.grovedb_versions
.apply_batch
.delete_tree_recursive_secondary_cleanup
>= 1,
"batch delete",
grove_version,
)
);
for subtree_path in subtrees_paths {
let p: SubtreePath<_> = subtree_path.as_slice().into();
let mut storage = self
.db
.get_transactional_storage_context(p, Some(&storage_batch), tx.as_ref())
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up merk subtree storage in batch delete: {e}",
))
})
);
}
}

// Indexed-tree secondary cleanup. find_subtrees walks the
Expand Down Expand Up @@ -6849,66 +6850,25 @@ impl GroveDb {
// (parent_path + cidx_key).
for cidx_path in &cidx_overwrite_cleanup_paths {
let cidx_subtree_path: SubtreePath<Vec<u8>> = cidx_path.as_slice().into();
// Clear all primary subtree storage recursively via
// find_subtrees (same walk as DeleteTree cleanup above).
let subtrees_paths = cost_return_on_error!(
// Clear all primary subtree storage recursively (same walk as
// the DeleteTree cleanup above), sweeping every discovered
// subtree's per-axis secondary namespaces too — this covers
// both the replaced cidx's own secondaries (find_subtrees
// includes the root path itself) and any nested indexed
// primary inside it (issue #888). Sweeping all three axes is
// safe: clear on empty is a no-op, so this also works for
// PCIT-only overwrites (the sum / avg slots are empty).
cost_return_on_error!(
&mut cost,
self.find_subtrees(&cidx_subtree_path, Some(tx.as_ref()), grove_version)
self.clear_subtree_storage_recursively(
&cidx_subtree_path,
tx.as_ref(),
&storage_batch,
true,
"batch overwrite",
grove_version,
)
);
for subtree_path in subtrees_paths {
let p: SubtreePath<_> = subtree_path.as_slice().into();
let mut storage = self
.db
.get_transactional_storage_context(p, Some(&storage_batch), tx.as_ref())
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up cidx primary subtree storage in batch \
overwrite: {e}",
))
})
);
}
// Clear the per-axis secondary namespaces at
// Blake3(primary ‖ axis_tag). Sweep all three axes — clear
// on empty is a no-op, so this also works for PCIT-only
// overwrites (the sum / avg slots are empty).
let primary_prefix = grovedb_storage::rocksdb_storage::RocksDbStorage::build_prefix(
cidx_subtree_path.clone(),
)
.unwrap_add_cost(&mut cost);
for axis in [
grovedb_element::indexed::IndexAxis::Count,
grovedb_element::indexed::IndexAxis::Sum,
grovedb_element::indexed::IndexAxis::Avg,
] {
let secondary_prefix =
grovedb_storage::rocksdb_storage::RocksDbStorage::secondary_prefix_for(
&primary_prefix,
axis.tag(),
)
.unwrap_add_cost(&mut cost);
let mut secondary_storage = self
.db
.get_transactional_storage_context_by_subtree_prefix(
secondary_prefix,
Some(&storage_batch),
tx.as_ref(),
)
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
secondary_storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up indexed-tree secondary (axis {:?}) storage \
in batch overwrite: {e}",
axis
))
})
);
}
}

// TODO: compute batch costs
Expand Down Expand Up @@ -7570,25 +7530,21 @@ impl GroveDb {
// BatchApplyOptions::disable_operation_consistency_check.
for child_path in &merk_delete_paths {
let child_subtree_path: SubtreePath<Vec<u8>> = child_path.as_slice().into();
let subtrees_paths = cost_return_on_error!(
cost_return_on_error!(
&mut cost,
self.find_subtrees(&child_subtree_path, Some(tx.as_ref()), grove_version)
self.clear_subtree_storage_recursively(
&child_subtree_path,
tx.as_ref(),
&storage_batch,
grove_version
.grovedb_versions
.apply_batch
.delete_tree_recursive_secondary_cleanup
>= 1,
"batch delete",
grove_version,
)
);
for subtree_path in subtrees_paths {
let p: SubtreePath<_> = subtree_path.as_slice().into();
let mut storage = self
.db
.get_transactional_storage_context(p, Some(&storage_batch), tx.as_ref())
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up merk subtree storage in batch delete: {e}",
))
})
);
}
}

// Indexed-tree secondary cleanup (parallels the
Expand Down Expand Up @@ -7641,60 +7597,21 @@ impl GroveDb {
.collect();
for cidx_path in all_cidx_overwrite_paths {
let cidx_subtree_path: SubtreePath<Vec<u8>> = cidx_path.as_slice().into();
let subtrees_paths = cost_return_on_error!(
// Shared recursive cleanup: primary namespaces plus per-axis
// secondaries for every discovered subtree, covering the
// replaced cidx itself and any nested indexed primary inside
// it (issue #888).
cost_return_on_error!(
&mut cost,
self.find_subtrees(&cidx_subtree_path, Some(tx.as_ref()), grove_version)
self.clear_subtree_storage_recursively(
&cidx_subtree_path,
tx.as_ref(),
&storage_batch,
true,
"batch overwrite",
grove_version,
)
);
for subtree_path in subtrees_paths {
let p: SubtreePath<_> = subtree_path.as_slice().into();
let mut storage = self
.db
.get_transactional_storage_context(p, Some(&storage_batch), tx.as_ref())
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up cidx primary subtree storage in batch \
overwrite: {e}",
))
})
);
}
let primary_prefix = grovedb_storage::rocksdb_storage::RocksDbStorage::build_prefix(
cidx_subtree_path.clone(),
)
.unwrap_add_cost(&mut cost);
for axis in [
grovedb_element::indexed::IndexAxis::Count,
grovedb_element::indexed::IndexAxis::Sum,
grovedb_element::indexed::IndexAxis::Avg,
] {
let secondary_prefix =
grovedb_storage::rocksdb_storage::RocksDbStorage::secondary_prefix_for(
&primary_prefix,
axis.tag(),
)
.unwrap_add_cost(&mut cost);
let mut secondary_storage = self
.db
.get_transactional_storage_context_by_subtree_prefix(
secondary_prefix,
Some(&storage_batch),
tx.as_ref(),
)
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
secondary_storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up indexed-tree secondary (axis {:?}) storage \
in batch overwrite: {e}",
axis
))
})
);
}
}

// let's build the write batch
Expand Down
36 changes: 29 additions & 7 deletions grovedb/src/batch/single_insert_cost_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,15 +1620,17 @@ mod tests {
}

#[test]
fn test_batch_plain_overwrites_and_tree_delete_cost_parity_v3_v4() {
// A batch touching NO indexed trees must cost byte-for-byte the same
// under GROVE_V3 and GROVE_V4. Both V4 gates
fn test_batch_plain_overwrites_and_tree_delete_versioned_costs_v3_v4() {
// Classifying a batch touching NO indexed trees must cost the same
// under GROVE_V3 and GROVE_V4. Both classification gates
// (`overwrite_indexed_cleanup_inspection` and
// `delete_tree_cleanup_type_source`) derive the old element from
// data the apply already loads — the merk walk's own fetch of the
// node being rewritten or deleted, and the emptiness pre-scan's own
// read — instead of issuing a dedicated stored-element read, so
// their classification work is invisible to tracked cost.
// their classification work is invisible to tracked cost. V4's
// separate recursive-secondary-cleanup gate adds charged empty
// sweeps, which are checked independently below.
let run = |grove_version: &GroveVersion| {
let db = make_empty_grovedb();
let tx = db.start_transaction();
Expand Down Expand Up @@ -1679,12 +1681,32 @@ mod tests {

let v3 = run(&grovedb_version::version::v3::GROVE_V3);
let v4 = run(&grovedb_version::version::v4::GROVE_V4);
let mut v4_primary_cleanup = grovedb_version::version::v4::GROVE_V4.clone();
v4_primary_cleanup
.grovedb_versions
.apply_batch
.delete_tree_recursive_secondary_cleanup = 0;
let v4_primary_cleanup = run(&v4_primary_cleanup);
v3.value.as_ref().expect("v3 batch should apply");
v4.value.as_ref().expect("v4 batch should apply");
v4_primary_cleanup
.value
.as_ref()
.expect("v4 batch with legacy cleanup should apply");
assert_eq!(
v3.cost, v4.cost,
"a batch of plain overwrites plus a plain DeleteTree must \
produce an identical CostResult under V3 and V4"
v3.cost, v4_primary_cleanup.cost,
"V4 classification must not add costs to plain overwrites or deletion"
);
assert_eq!(
v4.cost,
v3.cost
+ OperationCost {
seek_count: 3,
storage_loaded_bytes: 864,
hash_node_calls: 4,
..Default::default()
},
"V4 cleanup adds only three empty secondary sweeps and their prefix hashes"
);
}

Expand Down
Loading
Loading