diff --git a/crates/storage/src/index.rs b/crates/storage/src/index.rs index 2330bec862..306d2cda67 100644 --- a/crates/storage/src/index.rs +++ b/crates/storage/src/index.rs @@ -1003,6 +1003,14 @@ impl Index { /// single-level concurrent add/update-vs-delete semantics applied /// transitively. /// + /// `Frozen` descendants are the one exception: they are immutable and never + /// deleted, so the walk skips a `Frozen` node and its whole subtree rather + /// than tombstoning it. A local delete never reaches this state — it is + /// rejected up front by `remove_child_from`'s `find_frozen_descendant` scan. + /// The skip here is the replay-side fallback: on `apply_delete_ref_action` + /// this replica can't reject a peer's `DeleteRef` without diverging, so it + /// preserves the frozen data (leaving it a detached orphan) and converges. + /// /// Internal subtree parent-lists and hashes are intentionally NOT /// recomputed: the whole subtree is detached at the root, so none of it /// feeds a live hash. Only the root's removal from its parent (done by the @@ -1029,6 +1037,21 @@ impl Index { let Some(index) = Self::get_index(id)? else { continue; }; + // Frozen data is immutable and never deleted: skip a Frozen node AND + // its subtree (don't recurse) so deleting a non-frozen parent leaves + // the frozen descendant as a surviving orphan. Mirrors the + // `RemoveMode::Delete` Frozen guard in `Interface`. Scoped to + // descendants (`id != root_id`): both callers (`remove_child_from`, + // `apply_delete_ref_action`) reject deleting Frozen data upstream, so + // the root is never Frozen here; the caller tombstones it regardless. + if id != root_id + && matches!( + index.metadata.storage_type, + crate::entities::StorageType::Frozen + ) + { + continue; + } if let Some(children) = &index.children { for child in children { stack.push(child.id()); @@ -1051,6 +1074,41 @@ impl Index { Ok(()) } + /// Returns the id of the first `Frozen` entity in `root_id`'s subtree + /// (excluding `root_id` itself), or `None` if the subtree holds no frozen + /// data. + /// + /// Read-only pre-check for the local delete guard in + /// [`remove_child_from`](crate::interface::Interface::remove_child_from): a + /// genuine subtree delete must refuse to strand `Frozen` data, so the + /// caller rejects the delete and asks the operator to relocate the frozen + /// entity out of the subtree first. Kept separate from + /// [`tombstone_descendants_of`](Self::tombstone_descendants_of) because the + /// check must complete and reject BEFORE any state is mutated. + pub(crate) fn find_frozen_descendant(root_id: Id) -> Result, StorageError> { + let _mutation_guard = index_mutation_guard(); + let mut stack = vec![root_id]; + while let Some(id) = stack.pop() { + let Some(index) = Self::get_index(id)? else { + continue; + }; + if id != root_id + && matches!( + index.metadata.storage_type, + crate::entities::StorageType::Frozen + ) + { + return Ok(Some(id)); + } + if let Some(children) = &index.children { + for child in children { + stack.push(child.id()); + } + } + } + Ok(None) + } + /// Removes a child reference from a parent without creating a tombstone. /// /// Used when reassigning collection IDs - we need to remove the old child diff --git a/crates/storage/src/interface.rs b/crates/storage/src/interface.rs index a008a3d733..a396e9016a 100644 --- a/crates/storage/src/interface.rs +++ b/crates/storage/src/interface.rs @@ -2500,6 +2500,21 @@ impl Interface { )); } + // A genuine subtree delete must not strand Frozen data buried deeper in + // the tree either. Scan descendants BEFORE mutating any state; if any + // Frozen entity exists, reject so the operator relocates it out of the + // subtree first. Same split-brain avoidance as the direct-child guard + // above: we never tombstone the subtree or broadcast a `DeleteRef` that + // would leave the frozen data detached on every peer. + if mode == RemoveMode::Delete { + if let Some(frozen_id) = >::find_frozen_descendant(child_id)? { + return Err(StorageError::ActionNotAllowed(format!( + "cannot delete subtree {child_id}: it contains Frozen data at {frozen_id}; \ + relocate the frozen entity out of the subtree before deleting" + ))); + } + } + // If this is a local user action, set the nonce if let StorageType::User { owner, .. } = metadata.storage_type { if *owner == crate::env::executor_id() { diff --git a/crates/storage/src/tests/index.rs b/crates/storage/src/tests/index.rs index c3b3010b5a..992a5df982 100644 --- a/crates/storage/src/tests/index.rs +++ b/crates/storage/src/tests/index.rs @@ -793,6 +793,171 @@ mod subtree_tombstoning { } assert!(!>::is_deleted(root).unwrap()); } + + /// A non-frozen parent may contain a `Frozen` descendant (e.g. a + /// `FrozenStorage` field). Frozen data is immutable and never deleted, so + /// deleting the parent must SKIP the frozen node AND its subtree (no + /// recursion) while still tombstoning the non-frozen rows. Regression for + /// the #286 cascade, which tombstoned frozen leaves too. + /// + /// This exercises the `tombstone_descendants_of` skip layer directly + /// (`Index`-level). The `Interface`-level behavior — a local delete of such + /// a subtree is REJECTED before any mutation — is covered separately by + /// `remove_child_from_rejects_subtree_with_frozen_descendant` in + /// `tests/interface.rs`. The skip here is the replay-side fallback. + #[test] + fn subtree_delete_skips_frozen_descendant_and_its_subtree() { + use crate::entities::StorageType; + type S = MockedStorage<2104>; + + let frozen_md = Metadata { + storage_type: StorageType::Frozen, + ..Metadata::default() + }; + + let root = Id::random(); + let a = Id::random(); // non-frozen parent being deleted + let b = Id::random(); // non-frozen descendant -> tombstoned + let f = Id::random(); // Frozen descendant -> survives + let fc = Id::random(); // element under the frozen node -> survives (no recursion) + + >::add_root(ChildInfo::new(root, [1; 32], Metadata::default())).unwrap(); + >::add_child_to(root, ChildInfo::new(a, [2; 32], Metadata::default())).unwrap(); + >::add_child_to(a, ChildInfo::new(b, [3; 32], Metadata::default())).unwrap(); + >::add_child_to(a, ChildInfo::new(f, [4; 32], frozen_md.clone())).unwrap(); + >::add_child_to(f, ChildInfo::new(fc, [5; 32], frozen_md)).unwrap(); + + >::remove_child_from(root, a, time_now()).unwrap(); + + assert!( + >::is_deleted(a).unwrap(), + "deleted parent tombstoned" + ); + assert!( + >::is_deleted(b).unwrap(), + "non-frozen descendant tombstoned" + ); + assert!( + !>::is_deleted(f).unwrap(), + "Frozen descendant must survive the subtree delete" + ); + assert!( + !>::is_deleted(fc).unwrap(), + "walk must not recurse into a Frozen node: its subtree survives too" + ); + assert!( + >::get_children_of(root) + .unwrap() + .iter() + .all(|c| c.id() != a), + "deleted node must be removed from parent's children list" + ); + // The skipped frozen node must NOT be advertised as deleted on the sync + // wire: it lives on, so listing it in its parent's `deleted_children` + // would make a syncing peer apply a spurious delete-wins against it. + assert!( + !>::get_index(a) + .unwrap() + .unwrap() + .deleted_children() + .contains(&f), + "surviving frozen node must not appear in parent's deleted_children advert" + ); + } + + /// The skip is by-subtree, not by-node-type: a Frozen node is skipped + /// together with EVERYTHING under it, including a *non-frozen* child. Also + /// covers a Frozen leaf (no children of its own). Complements + /// `subtree_delete_skips_frozen_descendant_and_its_subtree`. + #[test] + fn subtree_delete_skips_whole_frozen_subtree_and_frozen_leaf() { + use crate::entities::StorageType; + type S = MockedStorage<2105>; + + let frozen_md = Metadata { + storage_type: StorageType::Frozen, + ..Metadata::default() + }; + + let root = Id::random(); + let a = Id::random(); // non-frozen parent being deleted + let f = Id::random(); // Frozen node with a non-frozen child below it + let nf_under_f = Id::random(); // NON-frozen, under Frozen -> still survives (no recursion) + let fleaf = Id::random(); // Frozen leaf (no children) -> survives + + >::add_root(ChildInfo::new(root, [1; 32], Metadata::default())).unwrap(); + >::add_child_to(root, ChildInfo::new(a, [2; 32], Metadata::default())).unwrap(); + >::add_child_to(a, ChildInfo::new(f, [3; 32], frozen_md.clone())).unwrap(); + >::add_child_to(f, ChildInfo::new(nf_under_f, [4; 32], Metadata::default())) + .unwrap(); + >::add_child_to(a, ChildInfo::new(fleaf, [5; 32], frozen_md)).unwrap(); + + >::remove_child_from(root, a, time_now()).unwrap(); + + assert!( + >::is_deleted(a).unwrap(), + "deleted parent tombstoned" + ); + assert!(!>::is_deleted(f).unwrap(), "Frozen node survives"); + assert!( + !>::is_deleted(nf_under_f).unwrap(), + "non-frozen child under a Frozen node survives: the walk skips the \ + whole frozen subtree, not just frozen nodes" + ); + assert!( + !>::is_deleted(fleaf).unwrap(), + "Frozen leaf (no children) survives" + ); + } + + /// `find_frozen_descendant` powers the local delete guard: it locates a + /// Frozen entity buried anywhere below the delete root (excluding the root + /// itself) and returns `None` for a frozen-free subtree. + #[test] + fn find_frozen_descendant_locates_deep_frozen_and_ignores_root() { + use crate::entities::StorageType; + type S = MockedStorage<2106>; + + let frozen_md = Metadata { + storage_type: StorageType::Frozen, + ..Metadata::default() + }; + + let root = Id::random(); + let a = Id::random(); // delete root (non-frozen) + let b = Id::random(); // non-frozen descendant + let f = Id::random(); // Frozen descendant, two levels down + + >::add_root(ChildInfo::new(root, [1; 32], Metadata::default())).unwrap(); + >::add_child_to(root, ChildInfo::new(a, [2; 32], Metadata::default())).unwrap(); + >::add_child_to(a, ChildInfo::new(b, [3; 32], Metadata::default())).unwrap(); + >::add_child_to(b, ChildInfo::new(f, [4; 32], frozen_md.clone())).unwrap(); + + assert_eq!( + >::find_frozen_descendant(a).unwrap(), + Some(f), + "must find the frozen entity nested below the delete root" + ); + + // A frozen-free subtree returns None. + let c = Id::random(); + >::add_child_to(root, ChildInfo::new(c, [5; 32], Metadata::default())).unwrap(); + assert_eq!( + >::find_frozen_descendant(c).unwrap(), + None, + "a subtree with no frozen data must not be flagged" + ); + + // The root's own StorageType is ignored — only descendants count. + let froot = Id::random(); + >::add_child_to(root, ChildInfo::new(froot, [6; 32], frozen_md)).unwrap(); + assert_eq!( + >::find_frozen_descendant(froot).unwrap(), + None, + "the scan excludes the root itself: a Frozen root with no frozen \ + descendants returns None" + ); + } } #[cfg(test)] diff --git a/crates/storage/src/tests/interface.rs b/crates/storage/src/tests/interface.rs index f6bbeb2ceb..ce713fc464 100644 --- a/crates/storage/src/tests/interface.rs +++ b/crates/storage/src/tests/interface.rs @@ -2018,6 +2018,76 @@ mod frozen_storage_verification { } } + /// Deleting a non-frozen subtree that has a Frozen entity buried below it + /// must also be rejected — the operator has to relocate the frozen data + /// out first. The direct child is NOT frozen here, so this exercises the + /// descendant scan (`find_frozen_descendant`), not the direct-child guard. + #[test] + fn remove_child_from_rejects_subtree_with_frozen_descendant() { + use crate::delta::{commit_causal_delta, reset_delta_context}; + use crate::entities::{ChildInfo, Metadata}; + use crate::index::Index; + use crate::store::MainStorage; + + env::reset_for_testing(); + + // Non-frozen parent under root, with a non-frozen paragraph beneath it. + let mut page = Page::new_from_element("Parent", Element::root()); + assert!(MainInterface::save(&mut page).unwrap()); + let mut para = Paragraph::new_from_element("Middle", Element::new(None)); + assert!(MainInterface::add_child_to(page.id(), &mut para).unwrap()); + + // Attach a Frozen entity two levels down, under the paragraph. + let frozen_id = Id::new([9; 32]); + >::add_child_to( + para.id(), + ChildInfo::new( + frozen_id, + [7; 32], + Metadata { + storage_type: StorageType::Frozen, + ..Metadata::default() + }, + ), + ) + .unwrap(); + + reset_delta_context(); + + // Deleting the paragraph must be refused: its subtree holds frozen data. + match MainInterface::remove_child_from(page.id(), para.id()) { + Err(StorageError::ActionNotAllowed(msg)) => { + assert!( + msg.contains("Frozen") && msg.contains("subtree"), + "error should name the frozen subtree: {msg}" + ); + } + other => panic!("Expected ActionNotAllowed error, got {other:?}"), + } + + // Nothing tombstoned: the paragraph and the frozen node both survive. + assert!( + !>::is_deleted(para.id()).unwrap(), + "paragraph must not be tombstoned by a rejected delete" + ); + assert!( + !>::is_deleted(frozen_id).unwrap(), + "frozen descendant must survive" + ); + + // And no `DeleteRef` was broadcast — the guard fires before mutation. + if let Some(delta) = commit_causal_delta(&[0; 32]).unwrap() { + assert!( + !delta + .actions + .iter() + .any(|a| matches!(a, Action::DeleteRef { id, .. } if *id == para.id())), + "rejected delete must not emit a DeleteRef, got: {:?}", + delta.actions + ); + } + } + #[test] fn frozen_blob_too_small_fails() { env::reset_for_testing();