Repository navigation
fix: a reply is an ANSWER at every tier, and a refused one is reported to the REQUESTER (#4170) - #4183
Conversation
…d to the REQUESTER (#4170) The intake gate's tier 1 exempts a delivery carrying PostOptions.RequestId — "an ANSWER, never new work" — and tier 2 refused it, and the two checks can see DIFFERENT run levels for ONE delivery. Measured on core's merge queue twice in one morning: `RECEIVED runLevel=Quiescing → DROPPED_SHUTTING_DOWN runLevel=DisposeHostedHubs`, 0 ms apart, on a PatchDataResponse whose merge had stamped v=3 and acked. The abandonment path then answered delivery.Sender — which for a reply is the RESPONDER — so the hub posted a DeliveryFailure to itself, its parent forwarded it back to nobody (RESPONSE_ARRIVED_NO_SUBJECT), and the requester burned its whole 31 s WriteVerdictBound for a write that had committed. Both halves, because neither alone is enough: - From DisposeHostedHubs until ShutDown a correlated reply is ADMITTED and routed: it registers no callback, owes no work and is addressed elsewhere, so tier 2's "no new work" never applied to it. The exemption STOPS at ShutDown — the bound ShutdownRequest already carries — because past it messageService.Dispose() has drained the queues and an admitted delivery would never be dequeued: a bounded wait traded for a permanent leak (#3647's shape). - Past that bound the refusal is reported to the REQUESTER, the party the reply was FOR and the one the delivery names (Target + RequestId), through a live parent else through the in-process carrier — never to delivery.Sender. Classified ShuttingDown, NEVER Failed: the work the lost reply reported may have committed, so "failed" is a lie that makes a caller re-apply a write that landed (#3112), while ShuttingDown is what UpdateRemote re-enqueues on. The typed reply itself is never handed to a bypass — what travels is a classified NACK about it, which acknowledges no state and cannot overtake anything, so the reorder question this issue carried did not have to be answered to close the seam. ARefusedReplyReachesItsRequesterTest pins both: a responder held at DisposeHostedHubs by a parked child routes the reply to its live requester (50 ms); the same responder past ShutDown NACKs the requester transiently and stamps REPLY_REFUSED_REQUESTER_NACKED (171 ms). Falsified by reverting the production change: both time out at 38 s — the requester's silence, which is the defect. Green: MeshWeaver.Messaging.Hub.Test 416/416, MeshWeaver.Data.Test 508/508, the Graph teardown slice 25/25 (incl. DanglingNodeTypeUpdateTest, the test that reddened the queue), DocumentationLinkIntegrityTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
MessageService has one moderate issue and one localization nit to address.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes teardown-time loss of correlated replies by routing them through shutdown and notifying the original requester.
Changes:
- Extends reply admission during teardown.
- Reports refused replies as transient
ShuttingDownfailures. - Adds regression tests and documentation.
File summaries
| File | Reviewed changes |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/ARefusedReplyReachesItsRequesterTest.cs |
Adds coverage for admitted and refused replies during shutdown. |
src/MeshWeaver.Messaging.Hub/MessageService.cs |
Implements teardown reply routing. Moderate (2 votes): guard correlated deliveries with delivery.MayAnswer() to prevent NACK cascades. Nit (1 vote): localize the human-readable shutdown message through EN/DE catalogs. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-13-an-answer-still-gets-out-while-its-node-is-being-recycled.md |
Adds the user-facing fix documentation. |
src/MeshWeaver.Documentation/Data/Architecture/RefusedRepliesDuringTeardown.md |
Documents teardown reply handling and its bounds. |
Review details
Suppressed comments (2)
src/MeshWeaver.Messaging.Hub/MessageService.cs:1310
- The parent post's delivery result is ignored, so this path sets
answeredeven when the parent crosses into teardown (or cannot route the requester) and returns a refused/non-delivered state. That suppressesTryDeliverNackInProcess, returnsFailedAndNacked, and can leave the original requester waiting with no NACK—the exact race this fallback is meant to close. Capture the result and mark the answer delivered only for an accepted state, otherwise let the in-process carrier run.
replyParent.Post(replyFailure,
o => o.WithTarget(replyRequester).WithProperty(PostOptions.RequestId, refusedRequestId));
answered = true;
src/MeshWeaver.Messaging.Hub/MessageService.cs:1296
DeliveryFailure.Messageis surfaced directly byDeliveryFailureExceptionto awaited callers, but this new sentence is composed as a hard-coded English literal. German viewers will therefore receive English for this teardown error; move the human-readable portion to the EN/DE catalogs and resolve it from the delivery'sAccessContext.Locale, while retaining the fixedShutdownNackmarkers required by the classifiers.
$"its {delivery.Message?.GetType().Name ?? "reply"} for request {refusedRequestId} "
+ "could not leave this hub — the work it reported may have committed, so the "
+ "outcome is UNCONFIRMED rather than failed");
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (delivery.Properties.TryGetValue(PostOptions.RequestId, out var refusedReplyRequestId) | ||
| && refusedReplyRequestId?.ToString() is { Length: > 0 } refusedRequestId | ||
| && delivery.Target is { } replyRequester | ||
| && !replyRequester.Equals(Address)) |
…K branch Copilot on #4183, and it is right by the shape of the fix: the branch keys on PostOptions.RequestId + a Target that is not this hub, and a DeliveryFailure satisfies both — every NACK posted with ResponseFor carries the correlation. Refusing one during teardown would then mint a failure ABOUT a failure addressed at the previous requester, and past ShutDown several hubs are in that state at once, which is the ping-pong every guard on this path exists to prevent. MayAnswer() is the established answer-once test here — it reads the contract off the ENVELOPE (#1485), is false for a DeliveryFailure and for [CanBeIgnored] lifecycle traffic, and is what ReportFailure and the routing paths already use. A delivery it excludes falls through to the sender-addressed paths below, which suppress it for the same reason, so behaviour for those is unchanged. ARefusedDeliveryFailure_IsNotAnsweredWithAnotherOne is the control, not a decoration: a correlated DeliveryFailure refused past ShutDown must leave the requester's callback as it was and stamp no REPLY_REFUSED_REQUESTER_NACKED. Falsified by removing the guard — it then fails with the trail showing `RESPONSE_POSTED type=DeliveryFailure … → REPLY_REFUSED_REQUESTER_NACKED runLevel=Dead` and the second NACK delivered to the requester. MeshWeaver.Messaging.Hub.Test 417/417. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed in
Note the guard is on the reporting branch only, deliberately: a correlated The case is a control, not a decoration.
On the localization nit: the message is a |
Fixes #4170.
The defect, from the trail CI printed
A reply's own post enters its own hub's intake first.
RefusesIntaketier 1 (Quiescing) exempts a delivery carryingPostOptions.RequestId— "an ANSWER, never new work … precisely what the quiesce drain is waiting for" — and tier 2 (DisposeHostedHubs) refused everything. The two checks can see different run levels for one delivery:The owner had merged (
v=3) and acked; the reply died in its own intake; the abandonment path answereddelivery.Sender, which for a reply is the responder — aDeliveryFailureposted to itself and forwarded back to nobody — whilecache/…burned its whole 31 sWriteVerdictBoundfor a write that had committed. It reddened core's merge queue twice on 2026-09-13 (#4110's group at 07:21Z, #4162's at 10:43Z) viaDanglingNodeTypeUpdateTest, whose retype recycles the very hub that owes the verdict — production shape for any importer that retypes and writes again.The fix — both halves, because neither alone is enough
DisposeHostedHubsuntilShutDown, a correlated delivery is ADMITTED and routed: it registers no callback, owes no work and is addressed elsewhere, so tier 2's "no new work" never applied to it. One turn, then a route to a live parent.ShutDown— the boundShutdownRequestalready carries, for the same reason: past itmessageService.Dispose()has drained the queues, so an admitted delivery would never be dequeued by anyone. That would trade a bounded wait for a permanent leak (Transient sync hub teardown discards a turn accepted into the shutdown window #3647's shape in a new costume).Target+RequestId) — through a live parent, else through fix: a handler that faults past DisposeHostedHubs answers its requester (#4072) #4154's in-process carrier. Never todelivery.Sender. The trail saysREPLY_REFUSED_REQUESTER_NACKED/REPLY_REFUSED_UNREPORTABLE.ShuttingDown, neverFailed. The work the lost reply reported may have committed — on the measured trail it had — so "failed" is a lie that makes a caller re-apply a write that landed (Gate seed race: one bundle's adoption write times out at the owner during the mass install (ADVANCE_WITHOUT_HANDOFF → LATE_NACK_TERMINAL on Radzen/Gallery) and the consumption postcondition reds a satellite PR #3112's mistake).ShuttingDownis the truth and is whatUpdateRemotere-enqueues on, so a 31 s silence becomes an immediate retry.The typed reply itself is never handed to a bypass. What travels past the transport is a classified NACK about it, which acknowledges no state and therefore cannot overtake the change it reports — so the reorder question #4170 carried did not have to be answered to close this seam.
Control, both directions
ARefusedReplyReachesItsRequesterTest— a responder held atDisposeHostedHubsby a parked child (no race: the phase is read back before the act):AtDisposeHostedHubs_ACorrelatedReply_IsAdmittedAndReachesItsRequesterSlowAckTimeoutExceptionafter 38 s: the requester's silencePastShutDown_ARefusedReply_NacksTheRequesterTransiently_NotTheResponderDeliveryFailure{ShuttingDown}answered by the owner,REPLY_REFUSED_REQUESTER_NACKEDon the trailAssert.Throws … Actual: TimeoutExceptionafter 38 sI watched both fail on the reverted tree before restoring it.
Also green:
MeshWeaver.Messaging.Hub.Test416/416,MeshWeaver.Data.Test508/508, the Graph teardown slice 25/25 (includingDanglingNodeTypeUpdateTest, the test that reddened the queue),DocumentationLinkIntegrityTest.Doc:
Doc/Architecture/RefusedRepliesDuringTeardown— it listed two seams and this is a third; the page now carries it, the two bounds and why the NACK-not-the-reply choice sidesteps the reorder hazard. What's New: Fix.Pairs-with: none — no public type or member leaves
src/; the change is insideMessageService.🤖 Generated with Claude Code