Skip to content

fix: a handler that faults past DisposeHostedHubs answers its requester (#4072) - #4154

Merged
meshweaver-cloud[bot] merged 4 commits into
mainfrom
fix/handler-fault-past-dispose-nacks
Sep 13, 2026
Merged

meshweaver-cloud[bot] merged 4 commits into
mainfrom
fix/handler-fault-past-dispose-nacks

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #4072.

The defect, reproduced

LeavingHubAdoptionSweepTest (core main, 14 s, passing) tears its mesh down with a SubscribeRequest in flight from cache/… to a per-node hub. With the fate ledger stamped at every carrier (this PR), the responder's trail reads:

HANDLER_FAULT TargetInvocationException→TargetInvocationException→HubDisposingException (+56ms)
  → NACK_DECLINED reason=parent-DisposeHostedHubs (+62ms)
  → FAILURE_REPORTED errorType=ShuttingDown runLevel=Dead (+63ms)
  → RESPONSE_POSTED type=DeliveryFailure target=cache/… (+63ms)
  → REPLY_REFUSED_SHUTTING_DOWN runLevel=Dead parent=mesh/…@DisposeHostedHubs (+63ms)

Before this PR the trail ended at HANDLER_FAULT: ReportFailure declined on its own run level (>= DisposeHostedHubs) before reaching the post seam — the loss the issue names — and NackThroughParent had already declined because the parent was past DisposeHostedHubs. The requester (cache/…, Quiescing, PendingCallbacks=1) re-armed its quiesce budget for the answer ([QUIESCE-WAIT] — "a shutting-down sibling answers before it goes") and got HubDisposedBeforeResponseException at 4 s. Mesh teardown: 4.0 s with a "still waiting" snapshot at 3 s → 2.0 s and no snapshot after this PR.

Why a handler runs on a hub past DisposeHostedHubs at all: AccessControlPipeline detaches next from the turn (decision outside the gate), so the handler runs concurrently with the hub's own shutdown phases — the hub read Dead 7 ms after the fault.

The fix (all in MeshWeaver.Messaging.Hub)

  1. ReportFailure has no run-level gate any more. The gate predates PostImplGeneric's forward-through-a-live-parent seam and could only lose answers. The seam decides, and stamps REPLY_REFUSED_SHUTTING_DOWN runLevel=… parent=… on the request's trail where it cannot carry.
  2. MessageHub.TryDeliverNackInProcess — the NACK's carrier of last resort: a DeliveryFailure carrying RequestId that no transport can take is handed straight to the in-process hub its requester lives under (host chain climbed to the root, descended, never created), when that hub still admits replies (< DisposeHostedHubs). NACKs only — nothing to reorder — and offered only where the drop happens: the post seam's refusal arm, the two routing drop arms after IUndeliverableReplySink declined, and (through ReportFailure) the intake gate's tier 2, which used to return without answering because "there is genuinely no route".
  3. IsDisposedContainer is a teardown fact in the handler fault chain: answered ShuttingDown in the owner's refusal vocabulary (ShutdownNack.IsAnsweredByOwner), not Unknown.
  4. The instrument: HANDLER_FAULT names the inner types; every carrier tried or declined is a stage (NACK_DECLINED reason=…, NACKED_THROUGH_PARENT, FAILURE_REPORTED, REPLY_FORWARDED_THROUGH_PARENT, NACK_DELIVERED_IN_PROCESS, REPLY_REFUSED_SHUTTING_DOWN); the verdict reads the last post-seam stage instead of RESPONSE_POSTED alone.

Test and falsification

HandlerFaultPastDisposeHostedHubsTest (4 cases, 0.5 s): a delivery-pipeline stage detaches the handler exactly as production does and releases it once the fence is read back — targeted teardown (parent live, genuine fault → Unknown through the parent) and whole-tree teardown (parent past DisposeHostedHubs, requester Quiescing on a 5-minute budget; HubDisposingException / disposed-scope → ShuttingDown, genuine → Unknown; NACK arrives in ~2 ms, the requester leaves Quiescing on the answer).

Reverting the production change (tests kept): 4/4 fail — every requester times out unanswered (TimeoutException after 36 s), and the whole-tree cases hold the mesh teardown open on the pending callback for 46 s. Restored: 4/4 green.

Also green with the fix: MeshWeaver.Messaging.Hub.Test 413/413; the teardown/NACK slice of MeshWeaver.Graph.Test (62/62 — NackReachesTheWaiterDuringTeardownTest, RefusedReplyReachesTheWaiterTest, UndeliverableReplyShapesTest, LateNackReenqueue*, UpsertAnswersWhenTheOwnerGoesAwayTest, …); LeavingHubAdoptionSweepTest 2/2; DocumentationLinkIntegrityTest.

Residue, named

A typed reply refused by a closed parent during a whole-tree teardown still drops (the documented RefusedRepliesDuringTeardown asymmetry): in the same repro nodeops answers type/StaleBundleType's compile-state upsert with CreateOrUpdateNodeResponse.Fail, which no carrier takes, so that sibling still pays one 2 s quiesce budget. That is the remaining 2.0 s.

Doc: Doc/Architecture/RefusedRepliesDuringTeardown (new section "The NACK's own carrier of last resort — #4072"). What's New: Fix.

Pairs-with: none — no public type or member leaves src/; the new members are internal/private.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BRLdhAiX5DjdZC2WUfPg9E

…er (#4072)

A handler fault on a hub already past DisposeHostedHubs was classified and then dropped:
ReportFailure declined on its own run level before reaching the post seam that already
forwards correlated replies through a live parent, and when the parent was past
DisposeHostedHubs too (every sibling's state during a whole-tree teardown) NackThroughParent
declined as well while the requester sat Quiescing on exactly that callback, re-arming its
budget for an answer that existed the whole time. Measured on LeavingHubAdoptionSweepTest
(14 s, PASSING): teardown 4.0 s with a pending-callback snapshot at 3 s; now 2.0 s and no
snapshot.

- ReportFailure has no run-level gate of its own any more; the post seam decides and stamps
  the request's trail with what it could not do (REPLY_REFUSED_SHUTTING_DOWN).
- MessageHub.TryDeliverNackInProcess: a DeliveryFailure no transport can carry is handed
  straight to the in-process hub its requester lives under, when that hub still admits
  replies. NACKs only (no state to reorder), offered only where the drop happens — the post
  seam's refusal arm, the two routing drop arms after IUndeliverableReplySink, and through
  ReportFailure the intake gate's tier 2.
- HubDisposingException.IsDisposedContainer is a teardown fact in the handler fault chain:
  answered ShuttingDown in the owner's refusal vocabulary, not Unknown.
- The fate ledger names the inner exception types on HANDLER_FAULT and stamps every carrier
  tried or declined (NACK_DECLINED reason=…, NACKED_THROUGH_PARENT, FAILURE_REPORTED,
  REPLY_FORWARDED_THROUGH_PARENT, NACK_DELIVERED_IN_PROCESS, REPLY_REFUSED_SHUTTING_DOWN),
  and the verdict reads the last post-seam stage instead of RESPONSE_POSTED alone.

Pinned by HandlerFaultPastDisposeHostedHubsTest (4 cases, 0.5 s); falsified by reverting the
production change: all four then time out with the requester unanswered and the whole-tree
cases hold the mesh teardown open on the pending callback.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRLdhAiX5DjdZC2WUfPg9E
Copilot AI lite review requested due to automatic review settings September 13, 2026 07:48
@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

261 tests   261 ✅  2m 6s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit a44948c.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

508 tests   508 ✅  44s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit a44948c.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

390 tests   390 ✅  43s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit a44948c.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

1 336 tests   1 334 ✅  3m 58s ⏱️
    3 suites      2 💤
    3 files        0 ❌

Results for commit a44948c.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

2 036 tests   2 036 ✅  5m 8s ⏱️
    4 suites      0 💤
    4 files        0 ❌

Results for commit a44948c.

♻️ This comment has been updated with latest results.

A reply is posted under a fresh delivery id nobody awaits, so every stage after RESPONSE_POSTED
was recorded nowhere and the verdict could only say "chase the response delivery" — the exact
sentence the 2026-09-13 merge-queue failure on DanglingNodeTypeUpdateTest (run 34744471576,
shard 4) left a reader with, and nothing to chase it by. The ledger now aliases the reply's id
to the request at RESPONSE_POSTED; the same Find(id)?.Add(...) every hub already performs then
writes the reply's stages into a sub-trail rendered as `↩ reply#1: RECEIVED@… → ROUTED … →
HANDLER_EXIT@<requester>`, and the verdict reads its last stage (no stage: the post seam refused
it or the target is outside the tree; ROUTED state=Failed: routing dropped it, stage names the
hub; RESPONSE_ARRIVED_NO_SUBJECT: it arrived where no callback was held; a last RECEIVED: it is
in that hub's queue). Bounded: three replies per request, a 512-entry alias ring.

QuiescingTimeoutNamesHandlerSideTest's misrouted-reply case now asserts the sharper verdict —
the sub-trail names the mesh hub the reply was handled on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRLdhAiX5DjdZC2WUfPg9E

Copilot AI left a comment

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.

🟡 Changes recommended

Delivery-state races and teardown classification issues may suppress fallback responses or misclassify faults.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes lost DeliveryFailure responses when handlers fault during hub teardown.

Changes:

  • Adds in-process NACK fallback routing.
  • Improves teardown fault classification and fate diagnostics.
  • Adds regression tests and documentation.
File summaries
File Summary
test/MeshWeaver.Messaging.Hub.Test/HandlerFaultPastDisposeHostedHubsTest.cs Adds teardown fault regression coverage.
src/MeshWeaver.Messaging.Hub/RequestFateLedger.cs Adds teardown carrier diagnostics.
src/MeshWeaver.Messaging.Hub/MessageService.cs Updates failure reporting and reply routing.
src/MeshWeaver.Messaging.Hub/MessageHub.cs Adds in-process NACK delivery.
src/MeshWeaver.Messaging.Hub/HierarchicalRouting.cs Integrates NACK fallback into routing drops.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-13-a-handler-that-fails-while-its-hub-shuts-down-still-answers.md Documents the fix in What's New.
src/MeshWeaver.Documentation/Data/Architecture/RefusedRepliesDuringTeardown.md Documents teardown reply behavior and remaining asymmetry.
Review details

Suppressed comments (5)

src/MeshWeaver.Messaging.Hub/MessageService.cs:2245

  • IsDisposedContainer classifies any matching Autofac ObjectDisposedException as hub teardown, even when a handler merely used an independently disposed nested scope while this hub's own scope is still live. That converts a genuine handler fault into a retryable ShuttingDown NACK. Use the existing probe-gated hub.IsTerminatedByScopeTeardown(e) here (while retaining the explicit HubDisposingException case) so only disposal of this hub's scope is treated as teardown.
                else if (HubDisposingException.IsHubDisposal(e) || HubDisposingException.IsDisposedContainer(e))

src/MeshWeaver.Messaging.Hub/MessageService.cs:998

  • This success test assumes Post propagates the result of its scheduling. The normal PostImplGeneric path calls ScheduleNotify but returns the original submitted delivery, so a late intake/routing rejection can still appear non-failed here. TryReportFailure then returns true and callers use FailedAndNacked, suppressing further reporting even though no DeliveryFailure reached the requester. Make the post path expose the actual rejection before using it as the answer-once decision.
                var posted = Post(new DeliveryFailure(delivery, message) { ErrorType = errorType },
                    new PostOptions(Address).ResponseFor(delivery));
                return posted is { State: not MessageDeliveryState.Failed };

src/MeshWeaver.Messaging.Hub/MessageService.cs:2615

  • replyParent.Post can return a Failed delivery without throwing, but this branch treats every return as an accepted forward and immediately returns. During the same teardown race, that suppresses the NACK in-process fallback for DeliveryFailure replies and records a misleading REPLY_FORWARDED_THROUGH_PARENT stage even though the parent rejected the post. Check the returned delivery state and only record/return on acceptance; otherwise fall through to the refusal/fallback paths.
                && ParentHub is { } replyParent
                && replyParent.RunLevel < MessageHubRunLevel.DisposeHostedHubs)
            {
                try

src/MeshWeaver.Messaging.Hub/MessageService.cs:890

  • This new helper uses a mutable List<string> in src/, contrary to the repository's immutable-collection rule; it only needs a bounded immutable accumulator, as the surrounding fate ledger already uses. Please use ImmutableList<string> here.
        var names = new List<string>(4);
        for (Exception? current = e; current is not null && names.Count < 4; current = current.InnerException)
            names.Add(current.GetType().Name);
        return string.Join("→", names);

src/MeshWeaver.Messaging.Hub/RequestFateLedger.cs:263

  • REPLY_REFUSED_SHUTTING_DOWN is emitted for every correlated message in PostImplGeneric, including a DeliveryFailure when TryDeliverNackInProcess returns false. This branch can therefore report a refused NACK as a “typed reply” and cite the typed-reply residue, making the new diagnostic verdict factually wrong and sending teardown investigation to the wrong path. Distinguish RESPONSE_POSTED type=DeliveryFailure before applying the typed-reply wording (or record a separate NACK-refusal stage).
                }
                // Sliding tail: the newest TailStages stages always survive, so the terminal
                // evidence the verdict reads can never be the part that gets dropped.
                tail = tail.Add(entry);
                if (tail.Count > TailStages)
  • Files reviewed: 9/9 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +3102 to +3103
requester.DeliverMessage(failure);
return true;
Comment on lines 818 to 822
parent.Post(
new DeliveryFailure(delivery) { ErrorType = errorType, Message = message },
o => o.ResponseFor(delivery));
fate?.Add($"NACKED_THROUGH_PARENT errorType={errorType} parent={parent.Address}", Address);
return true;
…ed-scope classification

Copilot review on #4154:
- NackThroughParent reported "answered" on a parent Post that returned Failed (the parent crossed
  the shutdown boundary after the check, or its post pipeline rejected) — now NACK_DECLINED
  reason=parent-post-refused, so the next carrier is tried.
- TryDeliverNackInProcess discarded DeliverMessage's verdict — a requester whose intake refused
  (Failed/Ignored) is no longer reported as delivered.
- The disposed-scope teardown fact is classified by ScopeTeardown.IsTerminatedByScopeTeardown
  (ObjectDisposedException AND this hub's own scope disposed), not by IsDisposedContainer's message
  match, so a handler that used some OTHER disposed scope keeps its genuine-fault verdict.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRLdhAiX5DjdZC2WUfPg9E
@rbuergi

rbuergi commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 7a2cccba1, all three:

  • NackThroughParent on a refused parent Post — now reads the returned delivery; a Failed (the parent crossed the shutdown boundary after the check, or its post pipeline rejected) stamps NACK_DECLINED reason=parent-post-refused and returns false, so ReportFailure and the in-process carrier are still tried.
  • TryDeliverNackInProcess discarding DeliverMessage's verdict — it now returns the intake's verdict (Failed/Ignored ⇒ not delivered), so a requester that crossed DisposeHostedHubs between the check and the call is not reported as answered.
  • IsDisposedContainer over-classifying — the arm now uses the probe-gated hub.IsTerminatedByScopeTeardown(e) (an ObjectDisposedException in the chain AND this hub's own scope disposed), the same classifier HandleInitialize and the permission fold use; a handler that reached into some other disposed scope keeps its genuine-fault verdict. The test's disposed-scope case now expects Unknown on a live scope for exactly that reason.

Plus, since the review: the ledger now writes a reply's own journey onto the request's trail (↩ reply#1: …), because this morning's merge-queue red (#4159) ended at RESPONSE_POSTED ⇒ chase the response delivery with nothing to chase it by.

A trail ending `RECEIVED → ENQUEUED → QUEUED depth=1` at the target and then nothing says the
target never dequeued the request, and nothing said why. Under each pending callback whose
target is a hub in this process, [STALE-CALLBACK], [QUIESCE-TIMEOUT] and the disposal snapshot
now print `target pump now: <address> RunLevel=… turn=<message> running <ms> buffer=…
deferred=… drainsInFlight=… awaitingScheduler=… draining=…` — the same snapshot a hub prints
for its own queue, taken for the hub the callback waits ON. Measured 2026-09-13 on the Plugins
gate (#2543/#4141): 25 stale callbacks, every SubscribeRequest among them parked exactly that
way at a NodeType hub with the pool at pendingWork=316. It is also #3593's M1/M2 discriminator,
readable from a live hub.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRLdhAiX5DjdZC2WUfPg9E
@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    6 files      6 suites   7m 42s ⏱️
2 892 tests 2 700 ✅ 192 💤 0 ❌
2 896 runs  2 704 ✅ 192 💤 0 ❌

Results for commit a44948c.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

   17 files     17 suites   20m 25s ⏱️
7 423 tests 7 229 ✅ 194 💤 0 ❌
7 427 runs  7 233 ✅ 194 💤 0 ❌

Results for commit a44948c.

@rbuergi
rbuergi marked this pull request as ready for review September 13, 2026 08:37
@meshweaver-cloud
meshweaver-cloud Bot added this pull request to the merge queue Sep 13, 2026
Merged via the queue into main with commit e3d8ec0 Sep 13, 2026
37 checks passed
rbuergi added a commit that referenced this pull request Sep 13, 2026
…ide of a coin toss

CI (queue build 34750238233, shard 1) failed the BuildupAction row with "No exception was
thrown". Its own output says why: TEST START …065 → "retiring this activation" …067 (+2 ms) →
FAILED …078. The activation faulted and retired BEFORE the request was posted, so the request
activated a fresh hub and was SERVED — which is exactly what step 3 of the same test asserts as
the contract. Both orderings are legal; the assertion was written for one of them, and which one
occurs was decided by how long GetClient()'s own hub construction took against an immediate
fault. Only BuildupAction faults immediately (a DataContext's init is slower), which is why only
that row failed.

NOT a #4165/#4151 interaction: the BuildupAction host has no AddData and therefore no
SynchronizationStream — its output block carries ZERO SYNC_STREAM lines where the passing
DataContext row carries five — so #4151's FaultStore/GetStream change cannot reach it.

The fence: the first init attempt parks until the test releases it, and the test releases only
once the request is provably in flight AT that activation. Step 3 already pins the other
ordering, so the test now states the whole contract instead of one side of it.

🚨 The first fence was wrong, and #4154's new report is what showed it:
  target pump now: host/1 RunLevel=Starting turn=InitializeHubRequest running 35004ms
                   buffer=1 deferred=0 … draining=True openGates=1
A BuildupAction runs ON the init turn, so the pump is busy and the request sits in the MAIN
queue, never reaching the deferral decision; a DataContext initialises OFF the turn, so the
request parks in the DEFERRED queue. InFlightAt accepts either queue, at that hub.

Verified on origin/main merged in (bd4314b, with #4151 and #4165): 4/4 green,
MeshWeaver.Data.Test 512/512, MeshWeaver.Messaging.Hub.Test 432/432.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants