fix(delete): a vanished descendant no longer refuses the whole recursive delete by rbuergi · Pull Request #4696 · Systemorph/MeshWeaver

rbuergi · 2026-09-18T07:22:19Z

The defect

A recursive delete plans its subtree by enumerating storage once (stage 3), and both later
stages address that one snapshot: the bulk-atomic pre-flight (3b) and the bottom-up commit (4). A
concurrent delete that removed one of the planned leaves at any point afterwards left the operation
holding a path that really was gone — and the whole subtree delete was refused over a node already
in exactly the state the caller asked for. That is #4668's decision one level down: an absent node
satisfies the delete's postcondition for itself.

Why the obvious fix does not apply

The issue measured it, and this PR re-measured it before changing anything. Relaxing the
pre-flight's NodeNotFound verdict is dead code for the dominant shape: with no row at the
address and no activated hub to short-circuit on, the post never ROUTES, so no verdict is produced
at all — the leg's fall-through reports

Cannot delete 'TestData/vanishing-descendant/gone-by-preflight':
No node found at 'TestData/vanishing-descendant/gone-by-preflight'.
Closest ancestor is 'TestData/vanishing-descendant' (remainder='gone-by-preflight').
This usually means the node is missing, has no NodeType, or has an invalid NodeType.

That sentence names three different situations, and the last two are precisely what the
bulk-atomic pre-flight exists to refuse before any storage side effect fires (#1198 / #1446 — a
partially destroyed subtree). The verdict half is no better: it says what the handler could read,
not that the row is gone.

The fix

MeshExtensions.ConfirmDescendantGone asks the store of record — IStorageAdapter.Exists — and
emits confirmed gone, never exists, so failing closed is structural: a probe that errors or
does not answer inside its own bound emits false and the original refusal stands, reason and
message untouched. It is a read on an error path only (a leg that has already failed, never once
per planned descendant), bounded one rung inside the leg it runs in — it runs in that leg's .Catch,
past the point the leg's own bound covers — derived with MeshOperationOptions.Nest like every other
rung on this path.

Both stages take the reading, because both address the same snapshot. Fixing only the pre-flight
moves the failure one stage later, and that was measured too: [DeleteNode] not-found … partial-deleted=0. The commit leg then emits nothing, so the vanished path is not recorded as
removed and the operation still says truthfully what it took away rather than claiming somebody
else's removal.

🚨 This cannot report success over a live node, and the reason is structural: "drained" is decided by
DeleteSubtreeUntilDrained's own storage re-enumeration, independent of every leg's verdict. A
path that is still there comes back as a survivor and is deleted in a follow-up pass; a subtree that
never drains still fails loudly at MaxDeleteDrainPasses.

The test, and the control

RecursiveDeleteVanishedDescendantTest — an IStorageAdapter decorator over a real
InMemoryStorageAdapter (nothing mocked; every call reaches the store) that removes one enumerated
descendant after the snapshot is taken and before it is answered, through the inner adapter's own
Delete so it publishes on the change feed exactly as another writer's delete would.

The second test is the discriminator, not a restatement: same leaf, same unroutable address, same
refusal wording — and a store of record that says the node is there (the stand-in for a NodeType
that will not load). The delete must still be refused, with the original message, and nothing may be
removed. Without it the first test would pass equally well on a pre-flight that had simply been
taught to ignore routing failures.

Measured both ways

dotnet build … -c Release -warnaserror, one project per invocation, 0 Error(s) confirmed before
every run, and the built assembly checked to carry (or not carry) the new symbols.

	`RecursiveDeleteVanishedDescendantTest`
with the fix	2 total, 0 failed, 2 succeeded — 1.1 s
fix reverted (`git apply -R`, rebuilt, symbols absent from the test bin)	2 total, 1 failed, 1 succeeded — the subject fails on the routing refusal; the control still passes
pre-flight half only	subject fails one stage later: `[DeleteNode] not-found … partial-deleted=0`

No regression: MeshWeaver.Graph.Test ~Delete 23/23 · MeshWeaver.Hosting.Test ~Delete 48/48 ·
MeshWeaver.Documentation.Test guards (link integrity, What's New integrity, front matter, unkeyed
activity log, hub-reachable async, hand-woven gates, test-timeout literals, subscribe error arms,
ToTask bridges) 37/37.

Conserved

Doc/Architecture/IdempotentDelete — the "🚧 measured and NOT fixed here" section is replaced by
the account above (why the obvious fix fails, what the discriminator is, why both stages).
Doc/Architecture/RecursiveDeleteDrain §4 — the pre-flight section gains the same rule.
What's New entry (Category: Fix, Order: -20260918).

Fixes #4680

🤖 Generated with Claude Code

…ive delete A recursive delete plans its subtree by enumerating storage ONCE (stage 3), and both later stages address that one snapshot: the bulk-atomic pre-flight (3b) and the bottom-up commit (4). A concurrent delete that removed one of the planned leaves at any point afterwards left the operation holding a path that really was gone, and the WHOLE subtree delete was refused over a node already in exactly the state the caller asked for -- #4668's decision one level down. Relaxing the pre-flight's NodeNotFound verdict is dead code for the dominant shape, which is why #4680 was filed rather than bundled: with no row at the address and no activated hub to short-circuit on, the post does not ROUTE, so no verdict is produced at all and the leg's fall-through reports "No node found at 'X'. Closest ancestor is ...". That sentence names three different situations, and the last two ("has no NodeType", "has an invalid NodeType") are precisely what the bulk-atomic pre-flight exists to refuse before any storage side effect fires (#1198/#1446). A verdict is no better: it says what the handler could read, not that the row is gone. So the discriminator is the store of record. ConfirmDescendantGone asks IStorageAdapter.Exists and emits "confirmed gone" -- never "exists" -- so failing closed is structural: a probe that errors or does not answer inside its own bound emits false and the original refusal stands, reason and message untouched. It is a read on an ERROR PATH ONLY, bounded one rung inside the leg it runs in (it runs in that leg's .Catch, past the point the leg's bound covers), derived with MeshOperationOptions.Nest like every other rung here. Both stages take it, because both address the same snapshot -- fixing only the pre-flight moved the failure one stage later, measured as "[DeleteNode] not-found ... partial-deleted=0". The commit leg then emits NOTHING, so the vanished path is not recorded as removed and the operation still says truthfully what IT took away. This cannot report success over a live node: "drained" is decided by DeleteSubtreeUntilDrained's own storage re-enumeration, independent of every leg's verdict, so a surviving path comes back as a survivor and a subtree that never drains still fails at MaxDeleteDrainPasses. RecursiveDeleteVanishedDescendantTest is the pair. An IStorageAdapter decorator over a real InMemoryStorageAdapter removes one enumerated descendant after the snapshot is taken and before it is answered. The second test is the discriminator: same leaf, same unroutable address, same refusal wording, and a store of record that says the node IS there -- the delete must still be refused with nothing removed. Measured, Release -warnaserror, one project per invocation: with the fix RecursiveDeleteVanishedDescendantTest 2/2 passed, 1.1 s fix reverted 1 failed (the subject), 1 succeeded (the control) regression Graph.Test ~Delete 23/23 · Hosting.Test ~Delete 48/48 · Documentation.Test guards 37/37 Fixes #4680 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Address the critical test setup issue and three moderate implementation issues before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR makes recursive deletes tolerate descendants removed concurrently while preserving refusal for nodes still present.

Changes:

Adds storage-backed absence handling during delete pre-flight and commit.
Adds regression and negative-control tests.
Updates architecture documentation and What's New.

File summaries

File	Review
`test/MeshWeaver.Graph.Test/RecursiveDeleteVanishedDescendantTest.cs`	Critical: Fix adapter initialization before registration.
`src/MeshWeaver.Mesh.Contract/MeshExtensions.cs`	Moderate: Use writable-only existence checks, fail closed on empty probe results, and handle `AlreadyAbsent` commit responses without recording deletion.
`src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-18-recursive-delete-tolerates-vanished-descendant.md`	No findings.
`src/MeshWeaver.Documentation/Data/Architecture/RecursiveDeleteDrain.md`	No findings.
`src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md`	No findings.

Review details

Suppressed comments (2)

src/MeshWeaver.Mesh.Contract/MeshExtensions.cs:4562

Use the writable-only existence predicate here. ListDescendantPaths (the plan) and the drain verifier both operate on writable providers, but PersistenceService.Exists ORs read-only providers too (PersistenceService.cs:454-474). If a concurrent delete removes a writable override while an embedded/static row remains at the same path, this probe reports the descendant as present and aborts even though the planned/deletable row is gone. Call ExistsInWritableStorage so this check matches the delete's snapshot and drain semantics.

        => storage.Exists(path)

src/MeshWeaver.Mesh.Contract/MeshExtensions.cs:4565

This does not fail closed when the probe completes without emitting: Take(1) followed by Select leaves an empty source empty, and Timeout does not fire after a normal completion. The outer SelectMany can therefore treat an unanswered existence check as no blocker and proceed. Add a DefaultIfEmpty(false) to the confirmed-gone result before the timeout so an empty probe remains a refusal.

        => storage.Exists(path)
            .Take(1)
            .Select(exists => !exists)
            .Timeout(probeBudget)

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

rbuergi · 2026-09-18T08:04:23Z

+    private readonly VanishingDescendantStorageAdapter storage = new(new InMemoryStorageAdapter());
+
+    /// <inheritdoc />
+    protected override MeshBuilder ConfigureMesh(MeshBuilder builder)
+        => base.ConfigureMesh(builder.ConfigureServices(services =>
+        {
+            // Registered BEFORE the bootstrap's TryAddSingleton, so this IS the store of record and
+            // production's version-writing / monotonic-guard decorators wrap it unchanged.
+            services.AddSingleton<IStorageAdapter>(storage);
+            return services;
+        }));


This one I believe is incorrect, and the language semantics are the reason — but it is a good thing to have checked, because if it held the test would be VACUOUS.

In C#, a derived class's field initializers run before the base constructor is invoked — the reverse of Java, where field initializers run after super() and the virtual-call-from-constructor bug is genuinely live. So the order here is:

storage = new VanishingDescendantStorageAdapter(new InMemoryStorageAdapter()) (line 183, derived field initializer)

MonolithMeshTestBase(output) — whose body reaches ConfigureMesh(...) at MonolithMeshTestBase.cs:216

the derived constructor body

storage is therefore non-null when ConfigureMesh registers it.

The empirical corroboration is the negative control, which is why it was run: with the fix patched out and the project rebuilt (new log symbols verified absent from the assembly), the subject test fails with Cannot delete …: No node found at … — the vanished-descendant refusal. That failure is only reachable if the decorator was installed and actually removed the leaf mid-operation; a null or absent registration would fail differently, or not at all. storage.Vanished.Should().BeTrue(...) in the subject test asserts the same thing directly.

Leaving the registration as it is.

rbuergi · 2026-09-18T08:04:21Z

+                    // 🚨 THE LEAF VANISHED WHILE THE CASCADE WAS IN FLIGHT — #4680, the commit half
+                    // of the same fact the pre-flight now settles. The plan is a snapshot, so a
+                    // concurrent delete can remove one of its leaves at ANY point after it is taken;
+                    // the pre-flight's window is merely the first one. With no row at the address
+                    // and no activated hub to short-circuit on, this post does not ROUTE, and the


Right, and it is the same false claim reached by the other door — fixed in e76ab8c8d4.

The .Catch only sees the leaf whose hub is GONE, so the post does not route. With the hub still ACTIVATED the post routes normally, HandleDeleteNodeRequest answers NothingToDelete() (Success=true, AlreadyAbsent=true, #4668), and that lands in the success branch — returning the path so .Do(RecordDeleted) counts a removal this cascade did not perform. That is exactly the truthfulness this change exists to keep, and it is the COMMON production shape: delete a node, then delete its parent recursively.

The success branch now emits nothing when resp.AlreadyAbsent, identically to the confirmed-gone arm.

🚨 Being explicit about what this is NOT: there is no discriminating test for this arm. IMeshService.DeleteNode returns IObservable<bool>, so the deleted-path list this affects is not observable through the public API — a test that could actually fail needs AffectedPaths/DeletedPaths surfaced to the caller, or the activity log captured. I would rather leave that stated than imply coverage that does not exist. The existing pair still passes 2/2 and still discriminates the arm it was written for.

github-actions · 2026-09-18T07:28:06Z