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 1 commit
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
195 changes: 49 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,11 @@ 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.
//
// NOTE: find_subtrees reads from the committed transaction state
// (without the pending storage_batch), so any subtrees *inserted*
Expand All @@ -6774,25 +6777,16 @@ 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,
"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 +6843,24 @@ 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,
"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 +7522,16 @@ 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,
"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 +7584,20 @@ 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,
"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
85 changes: 83 additions & 2 deletions grovedb/src/operations/auxiliary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ use grovedb_costs::{
cost_return_on_error, storage_cost::key_value_cost::KeyValueStorageCost, CostResult, CostsExt,
OperationCost,
};
use grovedb_element::indexed::IndexAxis;
use grovedb_path::SubtreePath;
use grovedb_storage::{Storage, StorageContext};
use grovedb_storage::{rocksdb_storage::RocksDbStorage, Storage, StorageBatch, StorageContext};
use grovedb_version::version::GroveVersion;

use crate::{
element::elements_iterator::ElementIteratorExtensions, util::TxRef, Element, Error, GroveDb,
TransactionArg,
Transaction, TransactionArg,
};

impl GroveDb {
Expand Down Expand Up @@ -193,4 +194,84 @@ impl GroveDb {
}
Ok(result).wrap_with_cost(cost)
}

/// Recursively clear ALL storage owned by the subtree at `path`: the
/// primary namespace of every nested subtree discovered by
/// [`Self::find_subtrees`], plus — for EVERY discovered subtree — the
/// per-axis indexed-tree secondary namespaces at
/// `Blake3(subtree_prefix ‖ axis_tag)` (S2-B derivation).
///
/// This is the single recursive ownership-cleanup routine (issue #888)
/// shared by the direct delete (`delete_internal_on_transaction`
/// v0/v1), the batch `DeleteTree` post-apply pass (full and partial),
/// the batch cidx safe-subset overwrite pass, and the dedicated
/// indexed-tree child overwrite. `find_subtrees` only walks
/// path-derived prefixes, so a nested indexed-tree primary's secondary
/// namespaces are invisible to it; without the per-descendant sweep
/// they would be orphaned, and a later re-creation of the same
/// deterministic path (prefixes are path-derived) would resurrect the
/// stale secondary rows, breaking primary-secondary agreement.
///
/// All three axis tags are swept unconditionally for every discovered
/// subtree rather than decoding each subtree's element to check its
/// tree type: clearing an empty namespace is a no-op, so the
/// redundancy is intentional defense-in-depth (it also removes a class
/// of missed-decoding bugs).
///
/// `context` names the calling operation in error messages.
pub(crate) fn clear_subtree_storage_recursively<'db, B: AsRef<[u8]>>(
&'db self,
path: &SubtreePath<B>,
transaction: &'db Transaction,
batch: &'db StorageBatch,
context: &str,
grove_version: &GroveVersion,
) -> CostResult<(), Error> {
let mut cost = OperationCost::default();

let subtrees_paths = cost_return_on_error!(
&mut cost,
self.find_subtrees(path, Some(transaction), 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.clone(), Some(batch), transaction)
.unwrap_add_cost(&mut cost);
cost_return_on_error!(
&mut cost,
storage.clear().map_err(|e| {
Error::CorruptedData(format!(
"unable to clean up subtree storage in {context}: {e}",
))
})
);

let primary_prefix = RocksDbStorage::build_prefix(p).unwrap_add_cost(&mut cost);
for axis in [IndexAxis::Count, IndexAxis::Sum, IndexAxis::Avg] {
let secondary_prefix =
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(batch),
transaction,
)
.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 {axis:?}) in \
{context}: {e}",
))
})
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Ok(()).wrap_with_cost(cost)
}
}
Loading
Loading