Repository navigation
Recursive delete: authoritative plan, gap traversal, storage-verified drain, creation guard (#839) - #894
Merged
Conversation
… drain, creation guard Fixes #839 — DeleteNodeRequest{Recursive=true} reported success while leaving descendants alive. THREE mechanisms, each pinned by a test: 1. Stale plan source: CollectPathsForDelete enumerated the subtree via the eventually-consistent catalog query; rows the index had not caught up on silently survived. The plan now comes from authoritative storage via the new IStorageAdapter.ListDescendantPaths (native single-query overrides for PostgreSQL — one UNION ALL across mesh_nodes + every satellite table with LIKE-wildcard escaping so '_' in mesh paths cannot pull sibling subtrees into a deletion plan — and Snowflake; default level-walk for the rest; routed partition-storage mode gets a ListDescendantPathsRequest/Response pair). HandleMoveNodeRequest's source-subtree enumeration had the same defect and now uses the same surface. 2. Gap-blind traversal (previously unreported): HierarchicalPathDeletion only recursed into children that were themselves members of the path set, so a branch anchored under a node-less level ({nodeType}/Release/{version}, {path}/_Thread/{id}, {space}/Source/{file}) was counted in the plan but never visited — deleteOne never fired and the operation still reported success. Traversal now groups by next path segment and recurses through virtual levels, invoking deleteOne only for real set members. 3. Mid-flight creations (the compile watcher writing Release satellites seconds into the delete) were invisible to the point-in-time plan and NOT covered by the RecentlyDeletedRegistry resurrection tombstone. Two complementary invariants: (a) BeginSubtreeDeletion — a ref-counted scope opened BEFORE plan enumeration (Observable.Using; released on success, failure, timeout, or unsubscribe) enforced by the new outermost SubtreeDeletionGuardStorageAdapter, which refuses in-process writes at or under an active deletion root with OnError; (b) a storage-verified drain loop — after each bottom-up pass the subtree is re-enumerated from storage, survivors are tombstoned and deleted in a follow-up pass, success is reported ONLY when an enumeration returns empty (bound: 5 passes; exhaustion fails loudly with the surviving paths). The guard covers in-process writers; the drain covers cross-process ones. Also fixed while wiring the guard: RecentlyDeletedRegistry moved from a hub-level AddGraph registration to the MESH ROOT (MeshBuilder.Register). The hub-level registration created a SECOND instance, so the storage guard was checking a registry no delete handler ever opened a scope on and silently passed every write — caught by the new write-guard test. Hub SPs fall back to the root, so every existing consumer resolves the same shared instance. SimpleMeshNodeStorage's uncalled streaming walk is renamed WalkDescendantPaths to clear the CS0108 collision with the new interface member. Tests: RecursiveDeleteAuthoritativeTest (node-less-gap shapes verified empty against storage; deterministic mid-flight creation drained before success; guarded write refused and nothing survives), HierarchicalPathDeletionTests (virtual-level traversal), RecentlyDeletedRegistrySubtreeScopeTests (scope boundary/refcount/idempotence), ListDescendantPathsTests (PG satellite + gap enumeration, LIKE-escape containment, root exclusion). Verified: Hosting.Test 16/16, RecursiveDeleteAuthoritativeTest 3/3, DeleteNodeBehaviorTest 10/10, PG ListDescendantPathsTests 3/3, NodeOperations DeletionTests 7/7; Release -warnaserror clean on all touched projects. Closes #839 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-recursive-delete
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #839 where DeleteNodeRequest { Recursive = true } could report success while leaving descendant paths alive due to stale catalog-based planning, gap-blind traversal, and mid-flight creations during deletion. It moves recursive-delete planning and verification to authoritative storage enumeration, updates the traversal to handle node-less intermediate segments, and adds a storage-layer write guard plus a storage-verified drain loop to prevent/clean up mid-flight writes.
Changes:
- Add authoritative subtree enumeration via
IStorageAdapter.ListDescendantPaths(native overrides for PostgreSQL/Snowflake; routed + decorator forwarding; default level-walk fallback). - Fix recursive delete traversal to traverse virtual (node-less) intermediate levels and add a bounded storage-verified drain loop to ensure the subtree is actually empty before reporting success.
- Add an in-process creation guard during deletion (
RecentlyDeletedRegistry.BeginSubtreeDeletion+SubtreeDeletionGuardStorageAdapter) and comprehensive regression tests.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Test/RecentlyDeletedRegistrySubtreeScopeTests.cs | Unit tests for ref-counted active-subtree-deletion scopes. |
| test/MeshWeaver.Hosting.Test/HierarchicalPathDeletionTests.cs | Adds coverage for deletion through node-less intermediate path levels. |
| test/MeshWeaver.Hosting.PostgreSql.Test/ListDescendantPathsTests.cs | Validates PG native descendant enumeration, including LIKE escaping and root exclusion. |
| test/MeshWeaver.Hosting.Monolith.Test/RecursiveDeleteAuthoritativeTest.cs | End-to-end repro tests covering authoritative planning, drain loop, and write guard behavior. |
| src/MeshWeaver.Mesh.Contract/Services/RecentlyDeletedRegistry.cs | Adds active subtree deletion tracking (scoped + ref-counted) to support write guarding. |
| src/MeshWeaver.Mesh.Contract/Services/IStorageAdapter.cs | Introduces ListDescendantPaths with a default level-walk implementation. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Switches delete planning to storage enumeration; fixes traversal; adds bounded drain loop; updates move-source enumeration. |
| src/MeshWeaver.Mesh.Contract/MeshBuilder.cs | Registers RecentlyDeletedRegistry at mesh-root to ensure a single shared instance. |
| src/MeshWeaver.Mesh.Contract/HierarchicalPathDeletion.cs | Updates traversal to recurse through virtual (node-less) levels and only delete real set members. |
| src/MeshWeaver.Hosting/Persistence/VersionWritingStorageAdapter.cs | Forwards ListDescendantPaths to preserve native inner enumeration. |
| src/MeshWeaver.Hosting/Persistence/SubtreeDeletionGuardStorageAdapter.cs | New outermost decorator refusing writes under an actively-deleting subtree. |
| src/MeshWeaver.Hosting/Persistence/SimpleMeshNodeStorage.cs | Renames internal streaming walk helper to avoid interface-slot collision. |
| src/MeshWeaver.Hosting/Persistence/PersistenceService.cs | Aggregates descendant enumeration across writable providers for authoritative verification. |
| src/MeshWeaver.Hosting/Persistence/PersistenceExtensions.cs | Wires SubtreeDeletionGuardStorageAdapter as the outermost decorator. |
| src/MeshWeaver.Hosting/Persistence/PathFilteringStorageAdapter.cs | Forwards ListDescendantPaths to inner adapter. |
| src/MeshWeaver.Hosting/Persistence/PartitionStorage/RoutingProxyAdapter.cs | Routes descendant enumeration to owning partition hub via new messages and forwards errors. |
| src/MeshWeaver.Hosting/Persistence/PartitionStorage/PartitionStorageMessages.cs | Adds ListDescendantPathsRequest/Response. |
| src/MeshWeaver.Hosting/Persistence/PartitionStorage/PartitionStorageHubExtensions.cs | Adds handler to serve descendant enumeration from the partition hub. |
| src/MeshWeaver.Hosting/Persistence/MonotonicWriteGuardStorageAdapter.cs | Forwards ListDescendantPaths to preserve native inner enumeration. |
| src/MeshWeaver.Hosting/Persistence/InMemoryStorageAdapter.cs | Adds a native prefix-scan ListDescendantPaths override. |
| src/MeshWeaver.Hosting/Persistence/Http/PathRemappingStorageAdapter.cs | Forwards ListDescendantPaths through remapping. |
| src/MeshWeaver.Hosting.Snowflake/SnowflakeStorageAdapter.cs | Adds native UNION-based descendant enumeration with LIKE-escape containment. |
| src/MeshWeaver.Hosting.Snowflake/SnowflakePathRoutingAdapter.cs | Routes descendant enumeration to per-schema adapter. |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlStorageAdapter.cs | Adds native UNION-based descendant enumeration across primary + satellite tables with LIKE escaping. |
| src/MeshWeaver.Hosting.PostgreSql/PostgreSqlPathRoutingAdapter.cs | Routes descendant enumeration to per-schema adapter. |
| src/MeshWeaver.Graph/Configuration/GraphConfigurationExtensions.cs | Removes hub-level registry registration, relying on mesh-root singleton. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
55
to
+59
| /// Per-node delete delegate. Returns <c>IObservable<Unit></c> that | ||
| /// emits once + <c>OnCompleted</c> on success, or <c>OnError</c> on | ||
| /// failure. Called once per path in the set, only after all that path's | ||
| /// descendants have already completed. | ||
| /// descendants have already completed. Never called for virtual | ||
| /// (node-less) intermediate levels. |
Contributor
Contributor
Contributor
Contributor
Test Results (shard 2)2 007 tests - 15 2 006 ✅ - 15 7m 17s ⏱️ +8s Results for commit bda8d05. ± Comparison against base commit 0a01185. This pull request removes 62 and adds 47 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Contributor
…exempt correctly The authoritative plan now (correctly) enumerates _Access satellites into the recursive fan-out — nodes the old delete silently leaked (#839's defect). The per-leaf DeleteNodeRequest re-entered the handler with no cascade context, so SpaceAdminInvariantValidator refused the space's own admin grant ('cannot remove the last administrator') and every space teardown failed — while the pre-flight ValidateDeleteRequest, which already carries RootPath, had passed. DeleteNodeRequest gains CascadeRootPath (set only by the subtree fan-out); the handler's root validation uses it as DeleteCascadeRootPath, matching the pre-flight exemption. Guard-rail, not a security boundary — per-leaf Delete permission checks still apply. Verified locally on every class the PR run failed: GlobalAdminSpaceCrudTest 10/10, ThreadCreationTest 14/14, DeckLayoutAreaTest+RecursiveDeleteAuthoritativeTest 11/11, SpaceDeletionPartitionDropTests+UserPartitionDeletionCascadeTests 2/2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-recursive-delete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #839 —
DeleteNodeRequest { Recursive = true }reported success while leaving descendants alive (observed twice: 32 and 68 survivors on "successful" deletes, includingReleasesatellites written seconds INTO the delete). Three distinct mechanisms, each pinned by a test:CollectPathsForDeleteenumerated the subtree via the eventually-consistent catalog query — rows the index hadn't caught up on silently survived, and completion was never verified. The plan now derives from authoritative storage via newIStorageAdapter.ListDescendantPaths: native single-query overrides for PostgreSQL (oneUNION ALLacrossmesh_nodes+ every satellite table, with LIKE-wildcard escaping so_in mesh paths can never pull a sibling subtree into a deletion plan) and Snowflake; default level-walk for the rest; the routed partition-storage mode gets aListDescendantPathsRequest/Responsepair with errors forwarded.HandleMoveNodeRequest's source-subtree enumeration had the same defect and now uses the same surface.HierarchicalPathDeletionrecursed only into children that were themselves members of the path set — a branch under a node-less level ({nodeType}/Release/{version},{path}/_Thread/{id},{space}/Source/{file}) was counted in the plan but never visited. Traversal now groups by next segment and recurses through virtual levels, invoking the delete only for real set members.RecentlyDeletedRegistry.BeginSubtreeDeletion— a ref-counted scope opened before plan enumeration (Observable.Using, released on every exit path) enforced by the new outermostSubtreeDeletionGuardStorageAdapterthat refuses in-process writes under an active deletion root with OnError; plus a storage-verified drain loop — after each bottom-up pass the subtree is re-enumerated from storage, survivors tombstoned and deleted in a follow-up pass, success reported ONLY when an enumeration returns empty (bound 5 passes; exhaustion fails loudly naming the survivors). Guard covers in-process writers; drain covers cross-process.Found during verification: the write-guard test exposed that
RecentlyDeletedRegistryexisted as TWO instances — AddGraph registered it hub-level while the storage guard resolves it in the persistence container, so the guard checked a registry no delete ever opened a scope on and passed every write. It now registers once at the mesh root (MeshBuilder.Register); hub SPs fall back to the root, so all existing consumers share the instance.SimpleMeshNodeStorage's uncalled streaming walk is renamedWalkDescendantPathsto clear the CS0108 collision.Verification
RecursiveDeleteAuthoritativeTest3/3 — node-less-gap shapes verified empty against storage; deterministic mid-flight creation drained before success; guarded write refused with nothing surviving (fails without the registry-root fix).HierarchicalPathDeletionTests+RecentlyDeletedRegistrySubtreeScopeTests16/16; PGListDescendantPathsTests3/3 (satellite+gap enumeration, LIKE-escape containment, root exclusion);DeleteNodeBehaviorTest10/10 (existing contract); NodeOperationsDeletionTests7/7.dotnet build -c Release -warnaserrorclean on every touched project.Behavior note: an activation save fired during pre-validation is now refused and logged (
SAVE FAILED) instead of silently re-persisting a row about to be deleted — expect that new warning line in delete-heavy logs.No What's New entry: bug fix; the user-visible effect (recursive delete actually deletes) rides the fix.
🤖 Generated with Claude Code