Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
92 changes: 70 additions & 22 deletions src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,38 +104,86 @@ able to fail.

## The absence that is still a failure

`NodeDeletionRejectionReason.NodeNotFound` did not go away. It is now reserved for an absence
discovered **mid-operation**: a leaf that vanished under a cascade already in flight, surfacing as a
routing failure from the commit stage. That is a different fact — the subtree may be partially
removed, so it is not in the state the caller asked for, and the caller is told.
`NodeDeletionRejectionReason.NodeNotFound` did not go away.

`ValidateDeleteRequest` — the pre-flight *query*, "would this delete be allowed?" — also still
answers `NodeNotFound` for an absent node. It is answering a question about the node, not carrying
out a delete, and "there is no such node" is the correct answer to that question.
`ValidateDeleteRequest` — the pre-flight *query*, "would this delete be allowed?" — still answers
`NodeNotFound` for an absent node. It is answering a question about the node, not carrying out a
delete, and "there is no such node" is the correct answer to that question. What a recursive delete
then *does* with that answer is the subject of the next section.

## 🚧 The same defect one level down, measured and NOT fixed here
And a `NodeNotFound` still reaches a caller whenever the absence is not the plain one: a node the
operation cannot see and the store still holds. That is a different fact — the subtree is not in the
state the caller asked for — and the caller is told.

A **recursive** delete plans its subtree by enumerating storage once, then asks every planned
descendant, in a bulk-atomic pre-flight, whether it may be deleted. A leaf that a concurrent delete
removes in that window still aborts the whole operation.
## The same defect one level down — #4680

This was reproduced while fixing #4668 — a storage adapter that removes one planned descendant after
the plan is taken and before it is answered — and the repro **falsified the obvious fix**. The leg
never reaches a `ValidateDeleteResponse` at all: the post to the now-absent address fails to route,
and the refusal that comes out is
A **recursive** delete plans its subtree by enumerating storage ONCE (stage 3), asks every planned
descendant in a bulk-atomic pre-flight whether it may be deleted (stage 3b), and only then commits
bottom-up (stage 4). **Both stages address that one snapshot**, so a concurrent delete that removes
one of the planned leaves — at any moment after the enumeration is taken — left the operation
holding a path that really was gone. The whole subtree delete was then refused over a node that was
already in exactly the state the caller asked for: this page's own decision, one level down.

### Why the obvious fix does not apply

Relaxing the pre-flight's `NodeNotFound` verdict is **dead code for the dominant shape**. A repro —
a storage adapter that removes one planned descendant after the enumeration is taken and before it
is answered — showed the leg never reaches a `ValidateDeleteResponse` at all: with no row at the
address and no activated per-node hub to short-circuit on, the post does not ROUTE, and the leg's
fall-through reports

```text
Cannot delete 'X/gone': No node found at 'X/gone'. Closest ancestor is 'X' (remainder='gone').
This usually means the node is missing, has no NodeType, or has an invalid NodeType.
```

That message is ambiguous by its own wording. Telling "already gone" apart from "its type will not
load" needs a storage read the pre-flight does not take today, and that is a change to the
bulk-atomic refusal semantics built by #1198 and #1446 — not a relaxed verdict. It is tracked
separately as [#4680](https://github.com/Systemorph/MeshWeaver/issues/4680).

It is **not** on #4668's own path: `IMeshService.DeleteNode` leaves `IncludeSatellites` false, so a
`_Comment` satellite is never part of a recursive plan.
The verdict half is not dead in general — routing short-circuits on an address whose hub is still
activated, so such a descendant *is* delivered, reads null and answers `NodeNotFound` — but it is
one of two vocabularies for the same fact, and **both of them are ambiguous**. The routing sentence
names three different situations in one breath, 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* — the partially destroyed subtree of #1198 / #1446. A verdict, likewise, says what the
handler could read, not that the row is gone.

### The discriminator is the store of record

`MeshExtensions.ConfirmDescendantGone` asks `IStorageAdapter.Exists(path)` and emits **confirmed
gone**, never "exists":

- **confirmed gone** — the descendant blocks nothing. The pre-flight passes it, and the commit leg
completes having removed nothing.
- **still stored** — the original refusal stands, reason and message unchanged.
- **the store could not answer** (an error, or no answer inside its own bound) — also "not confirmed
gone", so the refusal stands. The return value is *confirmed gone* rather than *exists* exactly so
that failing closed is structural: "I could not tell" has no way to read as "it was gone".

It is a read on an **error path only** — taken for a leg that has already failed, never once per
planned descendant — and it is bounded one rung inside the leg it runs in (it runs in that leg's
`.Catch`, past the point the leg's own bound still covers), derived by `MeshOperationOptions.Nest`
like every other rung on this path.

### Both stages, because both address the same snapshot

Fixing the pre-flight alone moves the failure one stage later, and that was measured too: the
recursive delete then failed at the commit with `[DeleteNode] not-found … partial-deleted=0`. The
commit leg takes the same reading, and when the leaf is confirmed gone it **emits nothing** — so the
path is not recorded as removed, and the operation still says truthfully what *it* took away rather
than claiming somebody else's removal.

That cannot make a delete report success over a live node, and the reason is structural: "drained"
is decided by [the drain](/Doc/Architecture/RecursiveDeleteDrain)'s own storage RE-ENUMERATION,
which is 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`.

`RecursiveDeleteVanishedDescendantTest` carries the pair. The second test is the discriminator: the
same leaf, the same unroutable address, the same refusal wording — and a store of record that says
the node IS there. The delete must still be refused and nothing removed. Without it the first test
would pass equally well on a pre-flight that had simply been taught to ignore routing failures.

This is **not** on #4668's own path: `IMeshService.DeleteNode` leaves `IncludeSatellites` false, so a
`_Comment` satellite is never part of a recursive plan. It was found while fixing that one, and is
tracked as [#4680](https://github.com/Systemorph/MeshWeaver/issues/4680).

## See also

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,25 @@ Three properties, and each is the reason for a choice above:
The stage bound survives as a **backstop**: every leg terminates on its own, so reaching it means
the fan-out as a whole stopped progressing, and it says so rather than blaming a leaf.

### A leg that reports an ABSENCE is confirmed against storage first (#4680)

The pre-flight and the commit both address the **one** snapshot stage 3 enumerated, so a concurrent
delete that removes a planned leaf at any point afterwards leaves the operation holding a path that
really is gone. Such a leg refuses in one of two vocabularies — a `ValidateDeleteResponse` carrying
`NodeNotFound` while the leaf's hub is still activated, otherwise a routing failure whose sentence
names three different situations at once — and neither of them distinguishes "already deleted" from
"still stored, and its NodeType will not load". The second is exactly what this pre-flight exists to
refuse before a row is removed.

So an absence-shaped leg failure is confirmed against the store of record
(`MeshExtensions.ConfirmDescendantGone` → `IStorageAdapter.Exists`) before it is believed: confirmed
gone blocks nothing, still-stored keeps today's refusal verbatim, and a store that cannot answer is
"not confirmed gone" so the refusal stands. It is a read on an error path only, one rung inside the
leg it runs in — the `.Catch` is past the point the leg's own bound covers. The commit leg takes the
same reading and then emits nothing, so a leaf somebody else removed is never counted among the
paths this delete removed. Full account:
[Deleting What Is Already Gone](/Doc/Architecture/IdempotentDelete).

### What is NOT fixed: the commit's writes serialize process-wide

#1198's third item reads *"the `commit` stage's N-deletes-over-a-cap-1 `pg:{adapter}` pool"*. As
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
Name: Deleting a folder no longer fails because part of it was already deleted
Category: Fix
Description: Deleting something with contents refused the whole operation if a single item inside it had just been removed by someone else — telling you it could not delete a node that was already gone, and leaving everything else untouched. The delete now accounts for that item and carries on, and still refuses when an item is genuinely still there.
Icon: Delete
Order: -20260918
---

# Deleting a folder no longer fails because part of it was already deleted

Deleting something that has contents — a space, a folder, a document with attachments — is not one
removal but many. The platform first takes a list of everything inside, then checks every item, and
only then removes them. That list is a snapshot, and in a shared workspace the world does not hold
still while it is being acted on.

**If anybody removed one of the listed items in that moment, the whole delete was refused**, with a
message saying that a node could not be deleted because no node could be found at its path. Nothing
was removed. The most ordinary way to hit it was the most frustrating one: two people tidying the
same space, or a cleanup script running while someone worked — and the remedy was to try again and
hope the timing was kinder.

That is a contradiction of the same shape the platform already fixed for a single node. Asking for
something to be gone and being told it cannot be removed *because it is already gone* is not a
failure — it is the request having been granted by somebody else first.

**A recursive delete now accounts for an item that disappeared while it was working and carries
on.** Everything else in the subtree is removed, the operation succeeds, and the record of what it
removed names only what this delete actually took away — an item somebody else removed is not
claimed as its own work.

**What has not changed is the protection that makes a delete all-or-nothing.** The refusal that
used to fire here is worded ambiguously: the same sentence appears when an item is missing *and*
when it is still there but cannot be opened. Those are opposite situations, and waving the second
one through is exactly how a half-destroyed folder happens. So the platform now asks the store
itself which one it is, and only an item the store confirms is gone is passed over. An item that is
still stored still stops the delete, with the same message as before and nothing removed — and if
the store cannot answer at all, the delete is refused rather than guessed at.
Loading
Loading