Repository navigation
fix(#6351): say a recursive delete that failed after removing nodes is partial and retriable - #6407
Conversation
…d retriable (refs #6351)
Test Results 20 files 20 suites 52m 35s ⏱️ Results for commit 738bd76. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
The activity warning lacks localization and documentation, while several reporting branches remain untested.
3 open findings
What changed in this PR
Improves recursive-delete failure reporting so partial deletions are identified as retriable.
Changes:
- Adds partial-deletion and retry guidance to timeout/unavailable failures.
- Adds integration tests covering failure and successful retry.
| File | Description |
|---|---|
src/MeshWeaver.Mesh.Contract/MeshExtensions.cs |
Adds partial-delete reporting and warning logs. |
test/MeshWeaver.Graph.Test/PartialDeleteIsReportedAsRetriableTest.cs |
Tests partial failure reporting and retry completion. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ? tornSubtreeNote | ||
| : string.Empty; | ||
| if ((isTimeout && tornSubtreeNote.Length > 0) || tornLeafNote.Length > 0) | ||
| failMsgs = failMsgs.Add(new LogMessage(tornSubtreeNote.Trim(), LogLevel.Warning)); |
There was a problem hiding this comment.
Fixed in 738bd76: the Warning now carries .WithKey("activity.delete.partialRetriable", ("path", path), ("count", partial.Count)), with the key in both strings.en.json and strings.de.json (English identical to the stored fallback; LocalizationTest 66/66). The client-mirror handoff is in the PR body as a Mirror-sync: line.
| 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"); |
There was a problem hiding this comment.
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.
| // 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." |
There was a problem hiding this comment.
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.
… (xUnit1069) Both tests carry a Fact Timeout but never referenced TestContext.Current.CancellationToken, which xUnit1069 refuses under -warnaserror and which reddened 'Build solution (once)'. Pass the token into the Emit waits, as the sibling delete-drain tests do, so a timed-out test also terminates its wait. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… contract The partial-delete Warning lands in the user-visible activity log, so it now carries activity.delete.partialRetriable (en + de) like its cancelled siblings. RecursiveDeleteDrain gains the section describing when a failure after removals says the subtree is partial and that a retry is meaningful, and that only availability failures receive that advice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What
HandleDeleteNodeRequest(src/MeshWeaver.Mesh.Contract/MeshExtensions.cs) now says so when a recursive delete fails AFTER it removed nodes: the failure text gets one more sentence - "N node(s) under '' 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." - and a matching warning is added to the operation's log messages. It is added for a commit-stage timeout and for a cascade leaf refused asUnavailable(the two shapes of the #6351 failure). A leaf refused for another reason (permission, validation) does NOT get it - a retry would not help there.Why
Refs #6351. Production:
[DeleteNode] unexpected path=Marketing partial-deleted=1383- the delete had removed 1,383 paths when one leaf's no-progress commit watchdog fired on the cap-1pg:Postgreswrite pool, and the caller read only that leaf's sentence, with nothing about the 1,383 nodes already gone or that asking again is meaningful. The queueing itself was caused by the cascade flooding the cap-1 lane with its OWN leaf writes; that root cause is already on main (#6357: bounded cascade fan-out,MeshOperationOptions.CascadeFanOutConcurrency). This change closes what #6357 left open: reporting a partially completed subtree delete as partial and retriable. No cap, budget, retry or timeout is changed - the cap is not raised and the watchdog is not silenced.How tested
test/MeshWeaver.Graph.Test/PartialDeleteIsReportedAsRetriableTest.csruns the existing serialised-write-lane harness (160 leaves, one delete at a time, fan-out bound 8) with the lane going silent after 40 removals - "held, whoever holds it":AFailureAfterRemovingNodes_SaysTheSubtreeIsPartial_AndThatARetryIsMeaningful: the delete fails with the leaf's own "made no progress" text (cause not rewritten), is not anUnauthorizedAccessException, and now says "partially deleted" and "retrying"; the store really holds some but not all leaves.ARetryOnceTheLaneServesAgain_RemovesWhatTheFailedDeleteLeftBehind: after the lane serves again a second delete removes every leaf and the root.Before this change the first test fails on its "partially deleted" / "retrying" assertions. The existing
WideDelete*,LongDeleteKeepsItsCallerTestandStalledDeleteStillTimesItsCallerOutTestinDeleteDrainCompletionTest.csare untouched. Run locally (Release, -warnaserror): both new tests pass;LocalizationTest66/66.Follow-ups on review
xUnit1069(theBuild solution (once)red): both tests now passTestContext.Current.CancellationTokeninto theirEmitwaits, as the sibling delete-drain tests do.activity.delete.partialRetriableinstrings.en.jsonandstrings.de.json.Doc/Architecture/RecursiveDeleteDraingains "A failure after removals says the subtree is partial, and that a retry is meaningful".Mirror-sync:
npm run sync:i18n -- --ref <merged core sha>in MeshWeaver.Pluginsclients/reactonce this merges; it adds the one keyactivity.delete.partialRetriable.Refs #6351
Bug-Thread: Hosting/Triage/_Thread/bug-systemorph-meshweaver-6351
Opened by Dispatch's control plane on behalf of
Essentials/Agent/bug-triage(auto · Provider/OpenRouterEU/anthropic/claude-sonnet-5.5), from the bug's own branchbugfix/systemorph-meshweaver-6351. It is never merged by Dispatch: the merge is the governed activitydev.merge, signed by a person once the checks and the internal review are green.