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
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,26 @@ latency an operator reads it as covering. The reading it changes is the pool's,
innocent. Pinned by `IoPoolQueueReadingCoversAcceptedWorkTest`, whose fourth case is `InvokeBlocking`
— green before and after, because it is the precedent the other three now follow.

### A failure after removals says the subtree is partial, and that a retry is meaningful (#6351)

A recursive delete that fails AFTER it removed nodes has left a torn subtree, and the leaf's own
sentence says nothing about that. Measured in production: `[DeleteNode] unexpected path=Marketing
partial-deleted=1383` — the operation had removed 1,383 paths when one leaf's commit watchdog
fired, and the caller read only the leaf's watchdog line.

So when the removals before the failure are non-zero and the failure is an **availability** one —
the stage's own timeout, or a leaf refused as `Unavailable` — the failure the caller reads keeps its
cause unchanged and appends one sentence: how many nodes were already removed, that the subtree is
partially deleted, and that the delete is idempotent, so retrying it is meaningful and removes what
is left. The same sentence is added to the response's `Log` as a Warning, keyed
`activity.delete.partialRetriable`, so the activity a user opens carries it in their language.

🚨 **Only availability failures get the retry advice.** A permission denial or a validator's refusal
is a decision, and asking again changes nothing — those arms keep their own sentence with no retry
note, and an availability failure never reaches the caller as a permission denial.
`PartialDeleteIsReportedAsRetriableTest` holds the lane after 40 of 160 removals and pins both the
sentence and the retry: once the lane serves again, a second delete removes everything left.

## 5. Record satellites leave in batches, not one leaf round-trip each

Every planned descendant used to cost two activations of its own per-node hub: one to answer the
Expand Down
19 changes: 18 additions & 1 deletion src/MeshWeaver.Mesh.Contract/MeshExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4553,13 +4553,30 @@ [new LogMessage(msg, LogLevel.Error)
("path", path), ("count", partial.Count))
: new LogMessage(ex.Message, LogLevel.Error);
var failMsgs = collectedMessages.ToImmutable().Add(failMsg);
// A delete that failed AFTER removing nodes has left a torn subtree. The caller is told so
// in so many words, with the one fact it needs next: the delete is idempotent, so asking
// again is meaningful and removes the rest (#6351).
var tornSubtreeNote = partial.Count > 0
? $" {partial.Count} node(s) under '{path}' were already removed before this failure, "
+ "so the subtree is partially deleted; the delete is idempotent - retrying it is "
+ "meaningful and removes what is left."
Comment on lines +4556 to +4562

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 738bd76: RecursiveDeleteDrain.md gains the section "A failure after removals says the subtree is partial, and that a retry is meaningful (#6351)". It covers the two availability arms that get the sentence, the keyed Warning in the response Log, and the rule that permission/validation refusals get no retry advice.

: string.Empty;
var tornLeafNote = leafReason == NodeDeletionRejectionReason.Unavailable
? tornSubtreeNote
: string.Empty;
if ((isTimeout && tornSubtreeNote.Length > 0) || tornLeafNote.Length > 0)
failMsgs = failMsgs.Add(
new LogMessage(tornSubtreeNote.Trim(), LogLevel.Warning)
.WithKey("activity.delete.partialRetriable",
("path", path), ("count", partial.Count)));
PostFailed(
isTimeout
// The stage detail rides along so the CALLER sees it too — the response
// is what a user, an agent or a test reads, and "exceeded 30s" alone is
// exactly as unreadable there as it was in the log.
? $"Delete of '{path}' exceeded {budget.TotalSeconds:0}s timeout "
+ $"in stage '{stage}': {ex.Message}"
+ tornSubtreeNote
: (isCancelled
? cancelledMessage
: (isNotFound
Expand All @@ -4570,7 +4587,7 @@ [new LogMessage(msg, LogLevel.Error)
// leaf's own classified refusal) — no "Unexpected error:"
// prefix, which would misdescribe a classified outcome.
? ex.Message
: $"Unexpected error: {ex.Message}"))),
: $"Unexpected error: {ex.Message}"))) + tornLeafNote,
isTimeout || isCancelled
// 🚨 A stage that ran out of time DECIDED nothing — it is an
// availability failure, and Unknown said neither that nor anything
Expand Down
1 change: 1 addition & 0 deletions src/MeshWeaver.Messaging.Hub/Localization/strings.de.json
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,7 @@
"activity.delete.blockedByDescendant": "'{path}' kann nicht gelöscht werden: {error}",
"activity.delete.cancelled": "Das Löschen von '{path}' wurde abgebrochen, bevor ein Node entfernt wurde.",
"activity.delete.cancelledPartial": "Das Löschen von '{path}' wurde abgebrochen, nachdem {count} Node(s) entfernt wurden — der Teilbaum ist unvollständig gelöscht.",
"activity.delete.partialRetriable": "Vor diesem Fehler wurden unter '{path}' bereits {count} Node(s) entfernt, der Teilbaum ist also unvollständig gelöscht; das Löschen ist idempotent - ein erneuter Versuch ist sinnvoll und entfernt den Rest.",
"activity.delete.cleanupFailed": "Das Aufräumen nach dem Löschen ({handler}) ist für '{path}' fehlgeschlagen: {error}",
"activity.node.created": "Node unter '{path}' erstellt",
"activity.node.updated": "Node unter '{path}' aktualisiert",
Expand Down
1 change: 1 addition & 0 deletions src/MeshWeaver.Messaging.Hub/Localization/strings.en.json
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,7 @@
"activity.delete.blockedByDescendant": "Cannot delete '{path}': {error}",
"activity.delete.cancelled": "Delete of '{path}' was cancelled before any node was removed.",
"activity.delete.cancelledPartial": "Delete of '{path}' was cancelled after removing {count} node(s) — the subtree is left partially deleted.",
"activity.delete.partialRetriable": "{count} node(s) under '{path}' were already removed before this failure, so the subtree is partially deleted; the delete is idempotent - retrying it is meaningful and removes what is left.",
"activity.delete.cleanupFailed": "Post-deletion cleanup ({handler}) failed for '{path}': {error}",
"activity.node.created": "Created node at '{path}'",
"activity.node.updated": "Updated node at '{path}'",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
using System.Reactive.Subjects;
using System.Threading.Tasks;
using MeshWeaver.Fixture;
using Xunit;

namespace MeshWeaver.Graph.Test;

/// <summary>
/// Issue #6351 - a recursive delete that fails AFTER it removed nodes has left a torn subtree, and its
/// failure must say so, and say that asking again is meaningful.
///
/// <para>Production shape: <c>[DeleteNode] unexpected path=Marketing partial-deleted=1383</c> - the operation
/// had removed 1,383 paths when one leaf's commit watchdog fired, and the failure the caller read was the
/// leaf's own sentence with nothing about the 1,383 nodes already gone. Here the store stops serving removals
/// after 40 of 160 (the write lane is held, whoever holds it), a leaf's no-progress watchdog fires, and the
/// caller is told the subtree is partially deleted and that the delete may be retried. A retry once the lane
/// serves again removes what is left.</para>
/// </summary>
public class PartialDeleteIsReportedAsRetriableTest(ITestOutputHelper output)
: WideDeleteOnASerialisedWriteLaneTestBase(output)
{
/// <inheritdoc />
protected override int FanOutConcurrency => 8;

[Fact(Timeout = 120000)]
public async Task AFailureAfterRemovingNodes_SaysTheSubtreeIsPartial_AndThatARetryIsMeaningful()
{
var rootPath = await SeedWideTree("partial-reported");
Storage.StallAfterDeletes = 40;

var failure = new AsyncSubject<Exception>();
using var deleting = NodeFactory.DeleteNode(rootPath).Subscribe(
_ => { },
ex =>
{
failure.OnNext(ex);
failure.OnCompleted();
});

var reported = await failure.Should().Within(60.Seconds()).Emit(
"a lane that stops serving removals must end the delete with a named failure",
cancellationToken: TestContext.Current.CancellationToken);

reported.Message.Should().Contain("made no progress",
"the failure is still the leaf's own watchdog - the cause is not rewritten");
(reported is UnauthorizedAccessException).Should().BeFalse(
$"an availability failure must not reach the caller as a permission denial, got {reported.GetType().Name}");
reported.Message.Should().Contain("partially deleted",
"nodes were removed before the failure, so the caller has to be told the subtree is torn");
reported.Message.Should().Contain("retrying",
"the delete is idempotent, so the caller is told that asking again is meaningful");
Comment on lines +44 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the scope: the two tests pin the production shape of #6351 (a cascade leaf refused as Unavailable after 40 of 160 removals, and the retry that finishes the job). They do not pin the stage-level isTimeout arm, the Warning entry in DeleteNodeResponse.Log, or the negative case that permission/validation refusals get no retry advice. Those branches are a one-line condition each (leafReason == NodeDeletionRejectionReason.Unavailable, isTimeout && partial.Count > 0), and the negative contract is now written down in RecursiveDeleteDrain.md. I am not widening this PR's test surface. Missing coverage is not a defect in the behaviour, and every push starts a new review round. Pinning the three branches is a reasonable follow-up.


var left = await Storage.Inner.ListDescendantPaths(rootPath).Should().Within(10.Seconds()).Emit();
left.Should().NotBeEmpty("the statement is only true if something really is left");
left.Count.Should().BeLessThan(Leaves, "the statement is only true if something really was removed");
}

[Fact(Timeout = 120000)]
public async Task ARetryOnceTheLaneServesAgain_RemovesWhatTheFailedDeleteLeftBehind()
{
var rootPath = await SeedWideTree("partial-retried");
Storage.StallAfterDeletes = 40;

var failure = new AsyncSubject<Exception>();
using var deleting = NodeFactory.DeleteNode(rootPath).Subscribe(
_ => { },
ex =>
{
failure.OnNext(ex);
failure.OnCompleted();
});
await failure.Should().Within(60.Seconds()).Emit(
"the first delete must fail while the lane is held",
cancellationToken: TestContext.Current.CancellationToken);

// The lane serves again.
Storage.StallAfterDeletes = null;

var deleted = await NodeFactory.DeleteNode(rootPath).Should().Within(90.Seconds()).Emit(
"a retry must remove what the failed delete left behind");
deleted.Should().BeTrue();

(await Storage.Inner.ListDescendantPaths(rootPath).Should().Within(10.Seconds()).Emit())
.Should().BeEmpty("every leaf must be gone after the retry");
(await Storage.Inner.Exists(rootPath).Should().Within(10.Seconds()).Emit())
.Should().BeFalse("the root must be gone after the retry");
}
}