Repository navigation
fix(teardown): a hub disposing with its OWN deferred delivery strands nobody — Debug, not Error (#4178) - #4191
Conversation
… nobody — Debug, not Error (#4178) `MessageService.Dispose` logged event 7301 at Error for every deferred delivery it discarded, with the sentence "the sender is answered ShuttingDown". For a SELF-ADDRESSED delivery that sentence is false: `NackThroughParent` declines it one method down with `NACK_DECLINED reason=sender-is-self`, because the pending-response registry that would resolve it is this hub's own and is cancelled in the same disposal. The reader is told by `CancelCallbacks` instead ("Hub … was disposed before the response arrived") — a better report, to the same reader. So the Error alleged stranded work, named an answer that was never sent, and pointed the reader at a producer that had done nothing wrong. Measured (#4178): 76 Error lines per plugin-gate shard, every one a `$model-probe/{guid}` hub discarding its own `GetDataRequest` parked behind [DataContextInit,MeshNodeInit] — the lifetime `TransientNodeProbe` declares by design ("created, read once, disposed"), and which `MessageHub.HandleInitialize` already classifies as a recognized shutdown rather than a failure (#1122-#1125). The rule is the FACT, not the probe: when the sender IS this hub, nobody outside is waiting, so the discard is teardown-internal. The line still fires and still names the message, id, gates and run level — at Debug, with a sentence that no longer claims an answer went out. A delivery from any OTHER sender still strands a real waiter on a transient NACK and stays an Error, unchanged. Both halves pinned, each watched failing: - widen the predicate to always-true -> the external-sender test reds for LOSING the Error - narrow it to always-false -> the self-addressed test reds for STOPPING to report The second control is the one that matters: a reclassification that quietly stopped reporting would be a silenced fault, not a classification. Docs: TeardownLayers.md gains the split; What's New entry (Fix). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The diagnostic wording is too specific, and the regression test does not cover pending-response teardown or verify the corrected message.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adjusts teardown logging for self-addressed deferred messages, keeping externally originated deliveries at Error level.
Changes:
- Reclassifies self-addressed discards as Debug.
- Adds regression coverage for log levels and messages.
- Updates teardown documentation and release notes.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs |
Adds self-addressed teardown logging coverage. |
src/MeshWeaver.Messaging.Hub/MessageService.cs |
Updates deferred-discard classification and diagnostic text. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-13-a-hub-disposing-with-its-own-question-unanswered-is-not-an-error.md |
Adds the release note. |
src/MeshWeaver.Documentation/Data/Architecture/TeardownLayers.md |
Documents the revised teardown behavior. |
Review details
Suppressed comments (1)
test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs:165
- This does not reproduce the production self-addressed question:
Postis fire-and-forget, so no response subject is registered andNackThroughParentexits withNACK_DECLINED reason=not-awaited, neversender-is-self;CancelCallbacksalso has nothing to cancel. UseObservewith an error subscriber to register the callback before posting, otherwise this test can pass without exercising the pending-response teardown path described by the PR.
selfTalker!.Post(new GatedRequest(), o => o.WithTarget(SelfTalkerAddress));
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| + "disposal. Teardown-normal (a transient node probe is created, read once and " | ||
| + "disposed by design); not a discard of anybody else's work.", |
| log.At(LogLevel.Debug).Should().Contain(m => m.Contains("self-gate-never-opens"), | ||
| "the discard must STILL be reported and still name the gate that held it — a " | ||
| + "reclassification that stops reporting would be a silenced fault, not a classification"); |
Test Results (shard 1)512 tests 512 ✅ 39s ⏱️ Results for commit 3c42160. ♻️ This comment has been updated with latest results. |
Test Results (shard 0)262 tests 262 ✅ 2m 8s ⏱️ Results for commit 3c42160. |
Test Results (shard 3)1 263 tests 1 263 ✅ 1m 42s ⏱️ Results for commit 3c42160. |
Test Results (shard 5) 4 files 4 suites 3m 56s ⏱️ Results for commit 3c42160. |
Test Results (shard 4)1 576 tests 1 576 ✅ 4m 39s ⏱️ Results for commit 3c42160. |
Test Results (shard 2)1 530 tests 1 530 ✅ 7m 45s ⏱️ Results for commit 3c42160. |
Test Results 14 files 14 suites 20m 51s ⏱️ Results for commit 3c42160. ♻️ This comment has been updated with latest results. |
The defect
MessageService.Disposelogs event 7301 at Error for every deferred delivery it discards, saying "the message is NOT processed; the sender is answeredShuttingDown".For a self-addressed delivery that sentence is false. One method down,
NackThroughParentdeclines exactly this delivery withNACK_DECLINED reason=sender-is-self— there is nowhere to post the answer, because the pending-response registry that would resolve it belongs to this hub and is cancelled in the sameDispose. The reader is told by a different and better route (CancelCallbackserrors those subjects with "Hub … was disposed before the response arrived"). So the Error alleged stranded work, named an answer that was never sent, and told whoever read it to go find a producer that had done nothing wrong.The measurement
76 Error lines per plugin-gate shard (Plugins run 34731952463,
Gate shard 1/1, same line on every shard measured 2026-09-13), every one a$model-probe/{guid}hub discarding its ownGetDataRequestparked behind[DataContextInit,MeshNodeInit]:Sender == target == the hub. A transient node probe is created, read once and disposed —
TransientNodeProbestates that outright, andMessageHub.HandleInitializealready classifies the same probe's dispose-during-init as a recognized shutdown rather than a failure (#1122–#1125). Its own-node read collapsing onto its synthetic address is the shapeTransientProbeAddressesexists for; two other read seams (GetMeshNodeOutcome,MeshNodeStreamCache.GetStreamRaw) already answer it directly. This is the third, reached as a raw deferral.The change
One predicate at the discard site. When
delivery.Senderis this hub's own address, the line is logged at Debug with a sentence that no longer claims an answer went out; it still names the message type, id, the gates it sat behind and the run level. A delivery from any other sender strands a real waiter on a transient NACK and stays an Error, unchanged.The rule follows the fact, not the probe — it is keyed on "is anyone outside waiting", the same thing
NackThroughParentalready decides, not on a probe-address special case.Why this is not a silenced fault
Nothing is suppressed and no bound was widened: the delivery is answered exactly as before (it was never answered for this shape), the site still reports, and the external-sender Error — the one with a victim who can act — is untouched. What changed is a verdict that was measurably false for one shape.
The alternative considered and not taken: guard the third read seam so a probe's own-address
GetDataRequestis answered immediately instead of parked. That is a larger change to what probes read, with no way to know which probes depend on the parked read completing — and it would still leave the Error wrong for every other self-addressed discard.Negative controls — both watched failing
The first control is the one that matters: it proves the new test cannot pass by the line vanishing. The log capture was widened to record the level, so "Error is gone" and "the line is gone" are two different readings.
Verification
dotnet build -c Release -warnaserror(whole solution): 0 Warning(s), 0 Error(s)MeshWeaver.Messaging.Hub.Test: 436 passed, 0 failedMeshWeaver.Documentation.Test: 447 passed, 0 failed (includesDocumentationLinkIntegrityTestfor the two doc edits)TestTimeoutLiteralRatchetGuardreported the new test's literalTimeout = 30_000and was obeyed by converting the site, not raising the baseline — the test's waits already carryTestTimeouts.Convergence.Docs
TeardownLayers.md— "What a hub still discards, and why that is an Error too" gains the self-addressed split, with thesender-is-selfdecline as the reason.Category: Fix).Cross-repo
No public type or member removed, no interface member added, no i18n catalog key touched —
Pairs-with:,Implementers:andMirror-sync:do not apply.Fixes #4178
🤖 Generated with Claude Code