fix(storage): don't tombstone Frozen descendants on subtree delete

[rtb-12]

The gap (#286 cascade)

The recursive subtree-tombstoning added for #286, Index::tombstone_descendants_of (crates/storage/src/index.rs), walks every descendant of a deleted node and tombstones each whose deleted_at >= updated_at. It never checked StorageType, so deleting a non-frozen parent that contains a Frozen descendant (e.g. a FrozenStorage field) tombstoned the frozen leaf too.

That violates the invariant that Frozen data is immutable and never deleted — the rule the RemoveMode::Delete guard in Interface::remove_child_from (and the StorageType::Frozen arm in apply_action) already enforces at the direct-delete boundary. The subtree cascade was a hole in that guard: a frozen entity reachable only as a descendant of a deleted non-frozen parent got tombstoned anyway.

Policy: reject the local delete (don't silently orphan)

A delete reaches the subtree walk on two paths with opposite freedoms:

Path	Caller	Can it error?
Local — user initiates a delete	Interface::remove_child_from → emits a DeleteRef	✅ Yes — reject before mutating, nothing broadcast
Replay — a DeleteRef arrives from a peer	apply_delete_ref_action	❌ No — the emitter already committed; erroring diverges the replica (split-brain)

Local path (the fix): remove_child_from_inner now runs a read-only Index::find_frozen_descendant scan over the target subtree before mutating any state. If any Frozen entity exists at any depth, it returns ActionNotAllowed naming the frozen id and asking the operator to relocate it out of the subtree first. The delete is refused atomically — no tombstones, no DeleteRef broadcast. This extends the existing direct-child Frozen guard (and its identical split-brain rationale) to the whole subtree, so in normal operation no detached orphan is ever created because the delete never happens.

Replay path (defense-in-depth): tombstone_descendants_of keeps skipping a Frozen node and its whole subtree. A compliant peer never emits such a delete now, but a replica must stay convergent if it receives one from an old/buggy/malicious peer — so it preserves the frozen data (leaving it detached) and converges rather than erroring.

Tests

• index::tests::subtree_tombstoning::find_frozen_descendant_locates_deep_frozen_and_ignores_root — the scan finds a frozen entity nested below the delete root, returns None for a frozen-free subtree, and excludes the root itself.
• interface::tests::frozen_storage_verification::remove_child_from_rejects_subtree_with_frozen_descendant — a local delete of a non-frozen subtree with a buried frozen descendant is rejected with ActionNotAllowed, nothing is tombstoned, and no DeleteRef is broadcast.
• The existing subtree_delete_skips_frozen_descendant_and_its_subtree / ..._whole_frozen_subtree_and_frozen_leaf tests continue to validate the replay-side skip in tombstone_descendants_of.

Full calimero-storage suite green (672 passing); clippy + fmt clean.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 32% | Review time: 48.9s

💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-de8c7b63}

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 AGENTS.md: AGENTS.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
• 🟡 CONTRIBUTING.md: CONTRIBUTING.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
• 🟡 architecture/migrations.html: Files matching crates/storage/** were changed but architecture/migrations.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/storage-schema.html: Files matching crates/storage/** were changed but architecture/storage-schema.html was not updated (per source_to_docs_mapping).
• 🟡 docs/: Static HTML docs in docs/ may need updating — architecture-impacting changes detected. On merge, update-docs will scan this directory and open a PR if any pages need to change.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 85% | Review time: 97.2s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-2d0784db}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 30% | Review time: 117.6s

💡 2 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-407e75fe}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 85% | Review time: 186.4s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-44ec6ccc}

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 31% | Review time: 42.0s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-5cc2cef8}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 31% | Review time: 109.9s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b363e8c3}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 32% | Review time: 109.7s

💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-3b5e3e3f}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 56% | Review time: 255.6s

💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b502e3d5}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 49% | Review time: 122.6s

🟡 2 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-de802e67}

[meroreviewer]

🟡 subtree_delete_skips_frozen_descendant_and_its_subtree calls Index::remove_child_from directly, bypassing the Interface-level Frozen guard

The test at line 870 calls <Index<S>>::remove_child_from(root, a, time_now()) directly. This bypasses Interface::remove_child_from_inner's find_frozen_descendant pre-check, so the test is actually exercising the REPLAY path (tombstone_descendants_of skip), not the local-delete rejection path. The test comment says 'The skip here is the replay-side fallback' which is correct, but the test name subtree_delete_skips_frozen_descendant_and_its_subtree could mislead a reader into thinking the local guard is being tested. More critically: calling Index::remove_child_from directly with a subtree containing a Frozen descendant succeeds (the frozen node is skipped by tombstone_descendants_of), which is the intended replay behavior. This is correct but the test is validating that the Index-level call does NOT reject — which is the opposite of what the Interface-level guard does. This is intentional per the PR description but could cause confusion.

Suggested fix:

Rename the test to `replay_subtree_delete_skips_frozen_descendant_and_its_subtree` to make clear it tests the replay path, not the local-delete rejection path.

[meroreviewer]

🟡 subtree_delete_skips_frozen_descendant_and_its_subtree calls Index::remove_child_from directly, bypassing the Interface guard

The test at line 862 calls <Index<S>>::remove_child_from(root, a, time_now()) directly. According to the PR description, the local delete guard (find_frozen_descendant rejection) lives in Interface::remove_child_from_inner, NOT in Index::remove_child_from. So this test exercises the replay-side skip in tombstone_descendants_of (correct, as the doc comment says), but the test comment says 'deleting the parent must SKIP the frozen node' — which is true at the Index level for the replay path. However, a reader might expect this test to also validate that the Interface-level guard fires. The test is correctly scoped but the setup calls <Index<S>>::remove_child_from which will NOT trigger the ActionNotAllowed rejection — it will proceed to tombstone_descendants_of and exercise the skip. This is intentional per the doc comment, but the test name subtree_delete_skips_frozen_descendant_and_its_subtree could mislead: it sounds like the delete 'skips' the frozen node (replay behavior), but a reader might think it tests the rejection. The companion test in interface.rs covers the rejection. No bug, but the test comment at line 862 says 'deleting the parent must SKIP' which is ambiguous — it should say 'the tombstone walk skips' to distinguish from 'the delete is rejected'.

Suggested fix:

Clarify the assertion message or the test name to explicitly say this tests the `tombstone_descendants_of` replay-side skip, not the `remove_child_from_inner` pre-check rejection. E.g. rename to `tombstone_walk_skips_frozen_descendant_and_its_subtree`.

[meroreviewer]

💡 Test uses Index::add_child_to directly to inject a Frozen entity, bypassing Interface invariants

The test remove_child_from_rejects_subtree_with_frozen_descendant injects a Frozen entity by calling <Index<MainStorage>>::add_child_to(para.id(), ChildInfo::new(frozen_id, ...)) directly, without writing a Key::Entry for frozen_id. This means the frozen entity exists in the index but has no backing storage entry. While this is sufficient to trigger find_frozen_descendant (which only reads the index), it creates an inconsistent state that wouldn't occur in production. The test is valid for its purpose, but a comment explaining why direct index injection is used (to avoid the Interface-level Frozen-add path, which may have its own guards) would help future readers understand the setup is intentional.

Suggested fix:

Add a comment: `// Inject directly into the index (no Key::Entry) to simulate a Frozen descendant without going through Interface::add_child_to, which has its own guards. find_frozen_descendant only reads the index, so no backing entry is needed for this test.`

[github-actions]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 462.8s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-74252f26}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 341.2s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-fbd0568a}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 217.9s

---

✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-6019b351}