diff --git a/src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md b/src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md index f394a11540..2d432e53d0 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/IdempotentDelete.md @@ -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 diff --git a/src/MeshWeaver.Documentation/Data/Architecture/RecursiveDeleteDrain.md b/src/MeshWeaver.Documentation/Data/Architecture/RecursiveDeleteDrain.md index c7b40108e3..e6a6b3a6b4 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/RecursiveDeleteDrain.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/RecursiveDeleteDrain.md @@ -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 diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-18-recursive-delete-tolerates-vanished-descendant.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-18-recursive-delete-tolerates-vanished-descendant.md new file mode 100644 index 0000000000..b42a2d6cf3 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-18-recursive-delete-tolerates-vanished-descendant.md @@ -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. diff --git a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs index a2f9ce5404..917e71115c 100644 --- a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs +++ b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs @@ -3583,10 +3583,18 @@ [new LogMessage(msg, LogLevel.Error) // subtree's budget anonymously. Derived, never a second // constant: equal budgets are what this issue is named // after. + // 🚨 And a THIRD rung, for one read on an error path + // only: a leg that reported an absence is confirmed + // against the store of record before it is allowed to + // refuse the subtree (#4680). It runs inside the leg's + // own .Catch — past the leg bound — so it takes the + // rung below it, derived the same way. + var legBudget = opts.Nest(budget); + var absenceProbeBudget = opts.Nest(legBudget); var preValidate = capturedRequest.Recursive ? PreValidateDescendantsObs( issuingHub, path, collected.ToDelete, request.AccessContext, - budget, opts.Nest(budget), logger) + budget, legBudget, storage, absenceProbeBudget, logger) : Observable.Return<(string Path, string Error, NodeDeletionRejectionReason Reason)?>(null); return preValidate.SelectMany(failure => @@ -3701,7 +3709,7 @@ [new LogMessage(msg, LogLevel.Error) meshHub, issuingHub, storage, path, collected.ToDelete, capturedRequest, executionContext, recentlyDeleted, logger, collectedMessages, - deletedProgress, drainProgress) + deletedProgress, drainProgress, absenceProbeBudget) .Select(d => (IReadOnlyList?)d)) .TimeoutAtStage(budget, () => { @@ -4158,11 +4166,12 @@ private static IObservable> DeleteSubtreeUntilDrained( ILogger logger, ImmutableList.Builder collectedMessages, ImmutableList.Builder deletedProgress, - IObserver progress) + IObserver progress, + TimeSpan absenceProbeBudget) => RunDeletePass( meshHub, issuingHub, storage, rootPath, plannedPaths, baseRequest, callerAccessContext, recentlyDeleted, logger, collectedMessages, deletedProgress, progress, - pass: 1, deletedSoFar: ImmutableList.Empty); + absenceProbeBudget, pass: 1, deletedSoFar: ImmutableList.Empty); private static IObservable> RunDeletePass( IMessageHub meshHub, @@ -4177,6 +4186,7 @@ private static IObservable> RunDeletePass( ImmutableList.Builder collectedMessages, ImmutableList.Builder deletedProgress, IObserver progress, + TimeSpan absenceProbeBudget, int pass, ImmutableList deletedSoFar) { @@ -4194,7 +4204,8 @@ private static IObservable> RunDeletePass( return FanOutDeleteSubtree( meshHub, issuingHub, storage, rootPath, toDelete, baseRequest, callerAccessContext, - logger, collectedMessages, deletedProgress, progress, rootAlreadyDeleted: pass > 1) + logger, collectedMessages, deletedProgress, progress, absenceProbeBudget, + rootAlreadyDeleted: pass > 1) .Catch, Exception>(ex => { // Fold this pass's partial deletions into the accumulated total so @@ -4266,7 +4277,7 @@ private static IObservable> RunDeletePass( return RunDeletePass( meshHub, issuingHub, storage, rootPath, survivorSet, baseRequest, callerAccessContext, recentlyDeleted, logger, collectedMessages, - deletedProgress, progress, pass + 1, acc); + deletedProgress, progress, absenceProbeBudget, pass + 1, acc); }); }); } @@ -4313,6 +4324,7 @@ private static IObservable> FanOutDeleteSubtree( ImmutableList.Builder collectedMessages, ImmutableList.Builder deletedProgress, IObserver progress, + TimeSpan absenceProbeBudget, bool rootAlreadyDeleted = false) { // 🚨 Record each commit AS IT LANDS, not at the end. The caller bounds this whole fan-out @@ -4429,6 +4441,18 @@ void RecordDeleted(string deleted) { if (resp.Log?.Messages is { Count: > 0 } msgs) lock (collectedMessages) collectedMessages.AddRange(msgs); + // 🚨 THE SECOND COMMIT-SIDE SHAPE OF THE SAME RACE (review on #4696). + // The `.Catch` below only sees the leaf whose hub is GONE, so the post + // does not route. When the hub is still ACTIVATED the post routes fine + // and the handler answers NothingToDelete() — Success=true, + // AlreadyAbsent=true (#4668) — which lands HERE, in the success branch. + // Returning the path would have `.Do(RecordDeleted)` count a removal + // this cascade did not perform, which is exactly the truthfulness this + // fix exists to keep: the operation must say what IT removed. Somebody + // else removed this one, so emit nothing, identically to the + // confirmed-gone arm below. + if (resp.AlreadyAbsent) + return Observable.Empty(); return Observable.Return(path); } var failResp = delivery.Message as DeleteNodeResponse; @@ -4436,6 +4460,43 @@ void RecordDeleted(string deleted) return Observable.Throw(new InvalidOperationException( $"Delete failed for '{path}': {reason}")); }) + // 🚨 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 + // whole recursive delete failed — `[DeleteNode] not-found … partial-deleted=0` + // — because one node the caller wanted gone was gone. + // + // Confirmed against the store of record, never off the routing sentence (see + // ConfirmDescendantGone), and the leg then emits NOTHING: the path is not + // recorded as removed, so the operation still SAYS what it removed — this + // cascade removed nothing here, somebody else did. A leaf that is genuinely + // present and merely unreachable still fails the commit exactly as before. + // + // 🚨 This cannot make a delete report success over a live node. "Drained" is + // decided by DeleteSubtreeUntilDrained'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, and a subtree that never + // drains still fails loudly at MaxDeleteDrainPasses. + .Catch(ex => + { + if (ex is not DeliveryFailureException { Failure.ErrorType: ErrorType.NotFound }) + return Observable.Throw(ex); + return ConfirmDescendantGone(storage, path, absenceProbeBudget, logger) + .SelectMany(gone => + { + if (!gone) + return Observable.Throw(ex); + logger.LogInformation( + "[DeleteNode] cascade leaf already gone {Path} — removed by " + + "another writer while the recursive delete of '{Root}' was in " + + "flight, so its postcondition already holds and this cascade " + + "reports it as removed by nobody", + path, rootPath); + return Observable.Empty(); + }); + }) .Do(RecordDeleted); }); } @@ -4466,6 +4527,63 @@ void RecordDeleted(string deleted) /// private const int PreValidateFanOutConcurrency = 64; + /// + /// 🚨 IS THIS PLANNED DESCENDANT ACTUALLY GONE? — the one reading that tells "already deleted" + /// apart from "present, and its hub will not answer" (#4680). Emits true ONLY when the + /// store of record says there is no row at . + /// + /// Why the question exists. A recursive delete plans its subtree by enumerating + /// storage ONCE, and every later stage — the bulk-atomic pre-flight, then the bottom-up commit + /// — addresses that snapshot. A concurrent delete that removes one of the planned leaves in + /// between leaves the operation holding a path that really is gone, and both stages then + /// refused the WHOLE subtree over a node that is 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. + /// + /// 🚨 The refusal's own wording cannot decide it, which is why this takes a read. + /// The absence arrives in two vocabularies and both are ambiguous. While the leaf's per-node + /// hub is still activated, routing short-circuits on the hosted address, the request IS + /// delivered, and the handler answers — + /// a verdict about what the handler could read, not evidence that the row is gone. Otherwise + /// the post does not route at all and the caller sees "No node found at 'X'. Closest + /// ancestor is … This usually means the node is missing, has no NodeType, or has an invalid + /// NodeType" — three different facts in one sentence, and the last two are precisely what + /// the pre-flight exists to refuse BEFORE any storage side effect fires (#1198/#1446: a + /// partially destroyed subtree). Treating either as "already gone" would wave a present-but- + /// broken node straight into the commit. + /// + /// 🚨 It FAILS CLOSED, and the return type is what makes that structural. The + /// value is "confirmed gone", not "exists" — so a probe that errors, or does not answer within + /// , emits false and the caller's original refusal stands. + /// "I could not tell" can never be read as "it was gone". + /// + /// The read costs nothing on the healthy path: it is taken ONLY on a leg that has already + /// failed, never per planned descendant. + /// + /// The store of record — the same adapter the plan was enumerated from. + /// The planned descendant whose absence is being confirmed. + /// Bound on this one read. Derived with + /// MeshOperationOptions.Nest from the bound of the leg it runs inside, because it runs + /// in that leg's .Catch — past the point the leg's own bound still covers. + /// Where an unreadable store is reported. + private static IObservable ConfirmDescendantGone( + IStorageAdapter storage, + string path, + TimeSpan probeBudget, + ILogger logger) + => storage.Exists(path) + .Take(1) + .Select(exists => !exists) + .Timeout(probeBudget) + .Catch(ex => + { + logger.LogWarning(ex, + "[DeleteNode] existence probe failed {Path} — the store could not say whether " + + "the node is there, so it is NOT confirmed gone and the original refusal stands", + path); + return Observable.Return(false); + }); + /// /// Bulk-atomic pre-flight: post at every /// descendant address (root excluded — already validated by the caller) and @@ -4511,6 +4629,11 @@ void RecordDeleted(string deleted) /// One leg's bound. Must be strictly smaller than /// — derive it with MeshOperationOptions.Nest rather than /// configuring a second value, because equal budgets are not an ordering (#1198). + /// The store of record, consulted ONLY on a leg that reported an + /// absence — see AlreadyGoneBlocksNothing below (#4680). + /// Bound on that one existence read. One rung inside + /// , derived by MeshOperationOptions.Nest at the call site: + /// the read runs INSIDE the leg's .Catch, which the leg's own bound no longer covers. /// Where a per-leaf refusal is reported. private static IObservable<(string Path, string Error, NodeDeletionRejectionReason Reason)?> PreValidateDescendantsObs( IMessageHub issuingHub, @@ -4519,6 +4642,8 @@ void RecordDeleted(string deleted) AccessContext? callerAccessContext, TimeSpan timeout, TimeSpan legTimeout, + IStorageAdapter storage, + TimeSpan absenceProbeBudget, ILogger logger) { var descendants = allPaths @@ -4527,6 +4652,33 @@ void RecordDeleted(string deleted) if (descendants.Length == 0) return Observable.Return<(string, string, NodeDeletionRejectionReason)?>(null); + // A DESCENDANT THAT IS ALREADY GONE BLOCKS NOTHING (#4680) — see ConfirmDescendantGone for + // why the verdict has to come from storage and never from the refusal's own wording. This + // wrapper is the pre-flight's half: gone ⇒ no blocker, otherwise the refusal stands + // verbatim, reason and message untouched. + IObservable<(string, string, NodeDeletionRejectionReason)?> AlreadyGoneBlocksNothing( + string leaf, string error, NodeDeletionRejectionReason reason) + { + var asRefused = ((string, string, NodeDeletionRejectionReason)?)(leaf, error, reason); + return ConfirmDescendantGone(storage, leaf, absenceProbeBudget, logger) + .Select(gone => + { + if (!gone) + return asRefused; + // Information, at the same rung as the delete's own outcome lines: a plan that + // named a path nobody could delete because it had already been deleted is a + // fact an operator reading this delete must be able to see. One line per + // vanished leaf, on a path that is rare by construction. + logger.LogInformation( + "[DeleteNode] pre-flight descendant already gone {Path} — it vanished " + + "between the plan and the pre-flight, so the delete's postcondition " + + "already holds for it and it blocks the recursive delete of '{Root}' " + + "no more than a leaf this operation had removed itself", + leaf, rootPath); + return null; + }); + } + // 🚨 Who has NOT answered yet — filled when a leg is actually POSTED, never seeded up // front (issue #1198). The merge below is capped, so at any instant some legs have not // been subscribed at all; a map seeded with every descendant would let the stage backstop @@ -4564,30 +4716,25 @@ void RecordDeleted(string deleted) : o.WithTarget(new Address(p)).WithAccessContext(callerAccessContext)); }) .Take(1) - .Select(d => + .SelectMany(d => { var resp = d.Message as ValidateDeleteResponse; if (resp is null || resp.IsValid) - return ((string, string, NodeDeletionRejectionReason)?)null; - // 🚨 KNOWN GAP, MEASURED AND DELIBERATELY NOT FIXED HERE — see #4680. A descendant - // that vanished between the plan and this fan-out still refuses the WHOLE recursive - // delete, which is #4668's defect one level down. It is NOT fixed by relaxing this - // verdict: a repro (an adapter removing one planned leaf after the enumeration is - // taken and before it is answered) showed the leg does not reach a - // ValidateDeleteResponse at all — the post to the absent address fails to ROUTE, so - // it lands in the Catch below as `Cannot delete 'X': No node found at 'X' …`. That - // message is ambiguous by its own wording ("missing, has no NodeType, or has an - // invalid NodeType"), so telling "already gone" from "its type will not load" needs - // a storage read the pre-flight does not take today — a change to the bulk-atomic - // refusal semantics of #1198/#1446, not a one-line relaxation. #4668's own path - // does not come through here: IMeshService.DeleteNode leaves IncludeSatellites - // false, so a `_Comment` satellite is never part of a recursive plan. - // + return Observable.Return<(string, string, NodeDeletionRejectionReason)?>(null); + // 🚨 THE LEAF ITSELF SAYS IT IS NOT THERE (#4680). This half is reached only while + // the leaf's per-node hub is still activated — routing short-circuits on an + // already-hosted address, so the request IS delivered, the handler reads null and + // answers NodeNotFound. The other half of the same fact arrives as a routing + // failure in the Catch below; both are confirmed the same way, against storage, + // because a verdict is not evidence that the row is gone. + if (resp.Reason == NodeDeletionRejectionReason.NodeNotFound) + return AlreadyGoneBlocksNothing(p, resp.Errors[0], resp.Reason); // 🚨 The descendant's OWN reason, not a blanket ValidationFailed (#1198). When its // permission fold could not be established it says so, and that is the answer the // operator needs — re-labelling it as a validation verdict is what makes the next // occurrence unreadable all over again. - return (p, resp.Errors[0], resp.Reason); + return Observable.Return<(string, string, NodeDeletionRejectionReason)?>( + (p, resp.Errors[0], resp.Reason)); }) // 🚨 ONE BOUND PER LEG — the behavioural half of issue #1198. Before this the whole // fan-out shared ONE bound, so a single silent per-node hub consumed the entire @@ -4623,6 +4770,16 @@ void RecordDeleted(string deleted) return Observable.Return<(string, string, NodeDeletionRejectionReason)?>( (p, ex.Message, NodeDeletionRejectionReason.Unauthorized)); } + // 🚨 THE POST DID NOT ROUTE — the dominant shape of a vanished descendant (#4680), + // and the one a repro measured: with no row at the address and no activated hub to + // short-circuit on, routing refuses before any handler is reached and the leg sees + // `No node found at 'X'. Closest ancestor is …`. That sentence names THREE possible + // facts, so it decides nothing on its own; AlreadyGoneBlocksNothing asks the store + // which one it is, and keeps today's refusal — reason and message unchanged — for + // a node that IS there and merely will not load. + if (ex is DeliveryFailureException { Failure.ErrorType: ErrorType.NotFound }) + return AlreadyGoneBlocksNothing( + p, ex.Message, NodeDeletionRejectionReason.ValidationFailed); // 🚨 The leg's own bound lapsed: this leaf never spoke. That is an AVAILABILITY // failure, not a verdict — same vocabulary and the same reasoning as // NodeDeletionRejectionReason.Unavailable everywhere else on this path, and the diff --git a/test/MeshWeaver.Graph.Test/RecursiveDeleteVanishedDescendantTest.cs b/test/MeshWeaver.Graph.Test/RecursiveDeleteVanishedDescendantTest.cs new file mode 100644 index 0000000000..342bf67d72 --- /dev/null +++ b/test/MeshWeaver.Graph.Test/RecursiveDeleteVanishedDescendantTest.cs @@ -0,0 +1,300 @@ +using System.Collections.Generic; +using System.Reactive.Linq; +using System.Reactive.Subjects; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Hosting.Persistence; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Services; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Graph.Test; + +/// +/// A real over a real that makes +/// ONE planned descendant vanish in the window a recursive delete cannot see: the enumeration is +/// TAKEN from the backing store, the leaf is then removed from it, and only then is the (now stale) +/// snapshot answered. +/// +/// Nothing here is a mock of a core interface — the store of record IS an +/// InMemoryStorageAdapter and every call reaches it. What it reproduces is the one thing a +/// quiet single-writer in-memory suite never does: a CONCURRENT delete landing between a recursive +/// delete's plan and the legs that plan feeds. The removal goes through the inner adapter's own +/// Delete, so it publishes on the change feed exactly as another writer's delete would — +/// the path resolver drops it, and the address stops being routable. +/// +/// is what makes the pair of tests below a DISCRIMINATOR +/// rather than a restatement: it changes ONLY the answer the store of record gives about the +/// vanished path, leaving the routing failure and its wording byte-identical. If the fix read the +/// refusal's message instead of the store, both tests would come out the same way. +/// +internal sealed class VanishingDescendantStorageAdapter(InMemoryStorageAdapter inner) : IStorageAdapter +{ + /// Backing store of record — tests assert against this, never against a cache. + public InMemoryStorageAdapter Inner => inner; + + /// Only an enumeration of THIS root makes the leaf vanish. + public string? WatchedRoot { get; set; } + + /// + /// The descendant removed from the backing store the FIRST time is + /// enumerated — after the snapshot is taken, before it is answered. Later enumerations (the + /// drain's verification passes) see the truth, which is what a real concurrent delete leaves + /// behind. + /// + public string? VanishPath { get; set; } + + /// + /// Makes answer true for even though the + /// row is gone: the stand-in for a descendant that IS there and whose per-node hub cannot be + /// reached — a missing NodeType, one that will not load. Routing refuses it with exactly the + /// same sentence as a deleted node, so the store of record is the only thing that separates + /// them, and a delete must still be REFUSED in this case. + /// + public bool PretendStillPresent { get; set; } + + /// Whether the removal actually happened — a test that did not arm is not a test. + public bool Vanished => Volatile.Read(ref _vanished) == 1; + + private int _vanished; + + /// + public IObservable> ListDescendantPaths(string rootPath) + { + var listing = inner.ListDescendantPaths(rootPath); + if (WatchedRoot is not { Length: > 0 } watched + || !string.Equals(rootPath, watched, StringComparison.OrdinalIgnoreCase) + || VanishPath is not { Length: > 0 } vanish) + return listing; + + return listing.SelectMany(paths => + Interlocked.Exchange(ref _vanished, 1) != 0 + ? Observable.Return(paths) + // Straight through the inner adapter: a concurrent delete publishes on the change + // feed, so the resolver forgets the path and the address stops routing. Rx, never a + // Task — the removal IS part of answering this enumeration. + : inner.Delete(vanish).Select(_ => paths)); + } + + /// + public IObservable Exists(string path) + => PretendStillPresent + && VanishPath is { Length: > 0 } vanish + && string.Equals(path, vanish, StringComparison.OrdinalIgnoreCase) + ? Observable.Return(true) + : inner.Exists(path); + + /// + public IObservable Delete(string path) => inner.Delete(path); + + /// + public IObservable DeleteIfExists(string path) => inner.DeleteIfExists(path); + + /// + public IObservable Read(string path, JsonSerializerOptions options) + => inner.Read(path, options); + + /// + public IObservable Write(MeshNode node, JsonSerializerOptions options) + => inner.Write(node, options); + + /// + public IObservable WriteIfVersion( + MeshNode node, long expectedVersion, JsonSerializerOptions options) + => inner.WriteIfVersion(node, expectedVersion, options); + + /// + public IObservable ExistsInWritableStorage(string path) + => ((IStorageAdapter)inner).ExistsInWritableStorage(path); + + /// + public IObservable<(MeshNode? Node, int MatchedSegments)> FindBestPrefixMatch( + string fullPath, JsonSerializerOptions options) + => inner.FindBestPrefixMatch(fullPath, options); + + /// + public IObservable<(MeshNode? Node, int MatchedSegments)> ResolvePath( + string fullPath, JsonSerializerOptions options) + => inner.ResolvePath(fullPath, options); + + /// + public IObservable<(IEnumerable NodePaths, IEnumerable DirectoryPaths)> + ListChildPaths(string? parentPath) + => inner.ListChildPaths(parentPath); + + /// + public IObservable> ListPartitionSubPaths(string nodePath) + => ((IStorageAdapter)inner).ListPartitionSubPaths(nodePath); + + /// + public IObservable GetPartitionObjects( + string nodePath, string? subPath, JsonSerializerOptions options) + => inner.GetPartitionObjects(nodePath, subPath, options); + + /// + public IObservable SavePartitionObjects( + string nodePath, string? subPath, IReadOnlyCollection objects, + JsonSerializerOptions options) + => inner.SavePartitionObjects(nodePath, subPath, objects, options); + + /// + public IObservable DeletePartitionObjects( + string nodePath, string? subPath = null) + => inner.DeletePartitionObjects(nodePath, subPath); + + /// + public IObservable GetPartitionMaxTimestamp( + string nodePath, string? subPath = null) + => inner.GetPartitionMaxTimestamp(nodePath, subPath); + + /// + public IObservable Changes => inner.Changes; +} + +/// +/// Systemorph/MeshWeaver#4680 — a recursive delete must not be REFUSED by a descendant that is +/// already gone, and must still be refused by one that is merely unreachable. +/// +/// What happened. 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, and +/// only then commits bottom-up. A concurrent delete that removes one of those leaves in between +/// left the operation holding a path that really was gone — and the WHOLE subtree delete was +/// refused over a node that was 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 did not apply. 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 ever produced — +/// the leg's fall-through reports "No node found at 'X'. Closest ancestor is …". That +/// sentence names three different facts ("missing, has no NodeType, or has an invalid NodeType"), +/// and the last two are precisely what the bulk-atomic pre-flight exists to refuse before any +/// storage side effect fires. So the refusal is confirmed against the STORE OF RECORD instead, and +/// the second test here is that distinction: identical routing failure, opposite store answer, +/// opposite outcome. +/// +public class RecursiveDeleteVanishedDescendantTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private readonly VanishingDescendantStorageAdapter storage = new(new InMemoryStorageAdapter()); + + /// + 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(storage); + return services; + })); + + /// + /// THE SUBJECT. Before the fix this failed at the pre-flight with + /// Cannot delete 'TestData/…/gone-by-preflight': No node found at …; with the pre-flight + /// alone it failed one stage later, in the commit, as [DeleteNode] not-found … + /// partial-deleted=0. Both halves address the same snapshot, so both had to learn the same + /// fact. + /// + [Fact] + public async Task RecursiveDelete_WhenAPlannedDescendantVanished_Succeeds_AndRemovesTheRest() + { + var (rootPath, stays, gone) = await SeedSubtree("vanishing-descendant"); + storage.WatchedRoot = rootPath; + storage.VanishPath = gone; + + // Producer -> test signal: whichever arm fires completes its own AsyncSubject, so a failure + // is REPORTED rather than silently spending the positive assertion's whole window. + var removed = new AsyncSubject(); + var failure = new AsyncSubject(); + using var deleting = NodeFactory.DeleteNode(rootPath).Subscribe( + r => { removed.OnNext(r); removed.OnCompleted(); }, + ex => { failure.OnNext(ex); failure.OnCompleted(); }); + using var reported = failure.Subscribe(ex => Output.WriteLine("DELETE FAILED: " + ex)); + + var ok = await removed.Should().Within(TestTimeouts.Convergence).Emit( + "a descendant that is already gone satisfies the delete's postcondition for ITSELF, so " + + "it can no more refuse the subtree than a leaf this operation had removed itself", + cancellationToken: TestContext.Current.CancellationToken); + ok.Should().BeTrue("the root was there, so this call really removed something"); + + storage.Vanished.Should().BeTrue( + "the test only discriminates if the leaf really was removed mid-operation"); + (await storage.Inner.Exists(rootPath).Should().Within(TestTimeouts.Quick).Emit( + cancellationToken: TestContext.Current.CancellationToken)) + .Should().BeFalse("the root must be gone"); + (await storage.Inner.Exists(stays).Should().Within(TestTimeouts.Quick).Emit( + cancellationToken: TestContext.Current.CancellationToken)) + .Should().BeFalse( + "the sibling that really was there must be removed — the vanished leaf must not " + + "take the rest of the subtree down with it, in either direction"); + } + + /// + /// NEGATIVE CONTROL, and the reason this change is not "the pre-flight stopped refusing". The + /// leaf is unreachable in exactly the same way — same routing failure, same sentence — but the + /// store of record says the node IS there, which is what a missing or unloadable NodeType looks + /// like. The bulk-atomic pre-flight exists to refuse that BEFORE any storage side effect fires + /// (a partially destroyed subtree), so the delete must fail and nothing may be removed. + /// + /// Without this test, the first one would pass just as well on a pre-flight that had + /// simply been taught to ignore every routing failure. + /// + [Fact] + public async Task RecursiveDelete_WhenTheDescendantIsUnreachableButStillStored_IsRefused() + { + var (rootPath, stays, gone) = await SeedSubtree("unreachable-descendant"); + storage.WatchedRoot = rootPath; + storage.VanishPath = gone; + storage.PretendStillPresent = true; + + var removed = new AsyncSubject(); + var failure = new AsyncSubject(); + using var deleting = NodeFactory.DeleteNode(rootPath).Subscribe( + r => { removed.OnNext(r); removed.OnCompleted(); }, + ex => { failure.OnNext(ex); failure.OnCompleted(); }); + + var refused = await failure.Should().Within(TestTimeouts.Convergence).Emit( + "a descendant the store still holds is exactly what the bulk-atomic pre-flight exists " + + "to refuse — 'I cannot reach it' must never be read as 'it is already gone'", + cancellationToken: TestContext.Current.CancellationToken); + + Output.WriteLine(refused.Message); + refused.Message.Should().Contain(gone, + "the caller has to be told WHICH descendant blocked the delete"); + refused.Message.Should().Contain("No node found at", + "the original refusal survives verbatim — the fix changes which refusals are believed, " + + "never what a believed one says"); + + storage.Vanished.Should().BeTrue( + "the test only discriminates if the leaf really became unroutable"); + (await storage.Inner.Exists(rootPath).Should().Within(TestTimeouts.Quick).Emit( + cancellationToken: TestContext.Current.CancellationToken)) + .Should().BeTrue("a refused pre-flight removes NOTHING — the root included"); + (await storage.Inner.Exists(stays).Should().Within(TestTimeouts.Quick).Emit( + cancellationToken: TestContext.Current.CancellationToken)) + .Should().BeTrue("nor may a sibling be removed ahead of the refusal"); + } + + /// Root with two children; the second is the one that will vanish. + private async Task<(string Root, string Stays, string Gone)> SeedSubtree(string id) + { + var rootPath = $"{TestPartition}/{id}"; + await NodeFactory.CreateNode( + new MeshNode(id, TestPartition) { Name = id, NodeType = "Markdown" }) + .Should().Within(TestTimeouts.Convergence).Emit( + cancellationToken: TestContext.Current.CancellationToken); + await NodeFactory.CreateNode( + new MeshNode("stays", rootPath) { Name = "Stays", NodeType = "Markdown" }) + .Should().Within(TestTimeouts.Convergence).Emit( + cancellationToken: TestContext.Current.CancellationToken); + await NodeFactory.CreateNode( + new MeshNode("gone-by-preflight", rootPath) { Name = "Gone", NodeType = "Markdown" }) + .Should().Within(TestTimeouts.Convergence).Emit( + cancellationToken: TestContext.Current.CancellationToken); + return (rootPath, $"{rootPath}/stays", $"{rootPath}/gone-by-preflight"); + } +}