Repository navigation
fix(#1485): carry answerability across the packaging boundary — the routers stop NACKing heartbeats - #1502
Conversation
… routers stop NACKing heartbeats
Every mesh delivery reaches a router as `delivery.Package(hub.JsonSerializerOptions)` —
`MeshBuilder` is the single call site of `IRoutingService.DeliverMessage` — and `Package`
replaces the payload with `RawJson`. The routing layer's answer-once guards were written as
CLR-type tests on `delivery.Message` (`is DeliveryFailure` / `HasAttribute<CanBeIgnored>`),
so on the routed path they inspected `RawJson` and could never match. The routers therefore
NACKed `[CanBeIgnored]` heartbeats to a departed owner — re-posted every heartbeat interval,
which IS the NotFound storm the guards' own comments describe — and could answer a
DeliveryFailure with a DeliveryFailure.
Seven dead sites, not two. The issue read the monolith guards as sitting BEFORE packaging and
therefore still working; they do not. Both routers sit behind the same single packaging call,
so the comment claiming they "both agree" was true only in that both were uniformly dead:
- RoutingGrain.PostFailure
- OrleansRoutingService.MayAnswer (both call sites)
- RoutingServiceBase.NackRouteFailure
- RoutingServiceBase.PostNotFound
- MonolithRoutingService.PostNotFound (also missing the [CanBeIgnored] clause outright)
- MessageService.NackThroughParent ([CanBeIgnored] half dead behind its RawJson clause)
- MessageService.ReportFailure (the routing TAIL — fixing only the routers would
have moved the same storm one level up)
The fix follows the precedent already inside `Package` for `IDiagnosticKeyed` (#1200): stamp
the FACT onto the envelope immediately before the type is erased, and have every guard read
the envelope via `delivery.MayAnswer()`. The fact, not the type name — the single question the
routing layer asks is "may I answer this", and a general-purpose type oracle on the envelope
would invite the payload-parsing decisions this codebase deliberately avoids. Only the
SUPPRESSED case is stamped, so ordinary traffic pays nothing and an unstamped delivery (one
that never went through `Package`, or arrived pre-serialised from an external client) degrades
to exactly the old CLR-type behaviour — never to "answer something you must not". The property
is presence-checked, never value-compared, because a delivery property crosses the wire as
JSON and returns as a JsonElement rather than the bool that was stamped.
Three tests, one per host plus the reporting tail, each demonstrated RED on the pre-fix tree
(all three suppressed+control deliveries were answered; expected one):
- OrleansRouterAnswerOnceAfterPackagingTest — real host lifetime, real hub at the sender
- MonolithRouterAnswerOnceAfterPackagingTest — real monolith mesh, real IRoutingService
- RoutingTailAnswerOnceAfterPackagingTest — the Orleans shutdown branch's return shape
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A participant proxy re-serializes the whole envelope (SignalRConnectionHub.DeliverMessage, the gRPC registry) and the routers on the far side apply the same answer-once contract to what comes back — so the stamp has to survive that hop, and it is the reason MayAnswer checks the property key's PRESENCE and never its value: an object-valued delivery property is stamped as a bool and arrives as a JsonElement. A value comparison would quietly stop matching after exactly one hop, which is the round-trip MessageStormBreaker.ResolvePayloadKey documents for the sibling DiagnosticKey stamp. Verified rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes #1485 by making the routing layer’s “answer-once” decision survive the MessageDelivery.Package(...) boundary (where payloads become RawJson). The change stamps “answer suppressed” onto the delivery envelope at packaging time and updates all relevant guard sites to consult the envelope via delivery.MayAnswer(), preventing heartbeat/NACK amplification storms during shutdown and missing-target scenarios.
Changes:
- Introduce
AnswerPolicyand stampAnswerSuppressedonto deliveries duringMessageDelivery.Package(...). - Replace CLR-type-based guards with
delivery.MayAnswer()across router + reporting tail paths. - Add Orleans/Monolith/tail regression tests and document the invariant (including a What’s New entry).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/RoutingTailAnswerOnceAfterPackagingTest.cs | Adds a regression test pinning the “routing tail” behavior on packaged deliveries that fail synchronously. |
| test/MeshWeaver.Hosting.Orleans.Test/OrleansRouterAnswerOnceAfterPackagingTest.cs | Adds Orleans router regression tests proving type erasure and enforcing answer-once behavior post-packaging. |
| test/MeshWeaver.Hosting.Monolith.Test/MonolithRouterAnswerOnceAfterPackagingTest.cs | Adds monolith router regression test ensuring the same answer-once behavior behind the packaging boundary. |
| src/MeshWeaver.Messaging.Hub/MessageService.cs | Updates failure reporting and parent-NACK behavior to use envelope-based MayAnswer() instead of payload CLR type. |
| src/MeshWeaver.Messaging.Hub/MessageDelivery.cs | Stamps answer-suppression metadata onto the envelope immediately before payload type erasure to RawJson. |
| src/MeshWeaver.Messaging.Contract/AnswerPolicy.cs | Centralizes the answer-once contract and provides MayAnswer(this IMessageDelivery) for all emit/guard sites. |
| src/MeshWeaver.Hosting/RoutingServiceBase.cs | Updates monolith routing guards (NackRouteFailure, PostNotFound) to use delivery.MayAnswer(). |
| src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs | Updates Orleans routing grain failure posting guard to use delivery.MayAnswer(). |
| src/MeshWeaver.Hosting.Monolith/MonolithRoutingService.cs | Fixes monolith NotFound sender-NACK decision to use envelope-based policy (and aligns with full contract). |
| src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs | Removes the local MayAnswer helper and uses delivery.MayAnswer() consistently in shutdown/dispatch failure paths. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-14-routing-answer-once.md | Adds a user-facing “Fix” entry describing the shutdown/node-deletion storm reduction. |
| src/MeshWeaver.Documentation/Data/Architecture/ActionBlockWedgePrevention.md | Documents the invariant: exemption must be read from the envelope, not payload CLR type, with references to the new tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 2)2 552 tests +10 2 551 ✅ +10 8m 25s ⏱️ +56s Results for commit 9d39fed. ± Comparison against base commit 4989ada. This pull request removes 154 and adds 164 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 1)1 464 tests +60 1 464 ✅ +60 6m 20s ⏱️ -24s Results for commit 9d39fed. ± Comparison against base commit 4989ada. This pull request removes 97 and adds 157 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…ionSource from the routing tests Standing constraint: 100% reactive, no polling, and no FirstAsync/Take(1) shenanigans — a one-shot take terminates the subscription and is "sample a moment" thinking spelled reactively. The three tests now push every NACK the router posts onto a ReplaySubject, and assert by SUBSCRIBING to a running fold of that stream through the repo's assertion idiom, `.Should().Within(...).Match(...)`, which settles on the first emission satisfying the predicate and fails with a real diagnostic on timeout. Gone: `DeliverMessage(...).FirstAsync().ToTask()` (now the sanctioned cold-observable `.Subscribe(...)`, which IS the dispatch), `TaskCompletionSource` + `WaitAsync`, and the mutable captured control-id. The assertion is a positive signal, not a wait: the ordinary control message is dispatched LAST down the same serial path, so the fold at the moment IT is answered already contains anything the two suppressed deliveries produced. `ContainSingle` on that fold is therefore a real assertion about a completed sequence — never "sleep, then look". Fail-before re-established for the rewritten bodies (the earlier evidence covered the old ones): disabling the stamp in Package alone turns all five assertions red — Orleans 2/3, monolith 1/1, routing tail 2/2, each reporting `found 3` where one is expected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up commit The three tests now push every NACK the router posts onto a The assertion stays a positive signal, not a wait: the ordinary control message is dispatched Fail-before re-established for the rewritten bodies (the earlier evidence covered the old ones): Re-verified locally on the new commit: Production code is unchanged by this commit and carries no async at all: |
… level its new volume can afford Two defects in the one log line the answer-once fix newly exercises. NAME: the payload at this point is RawJson by construction, so `delivery.Message.GetType().Name` printed the literal string "RawJson" for every suppressed delivery. GetMessageType reads the $type out of the JSON — the only way the line says what was actually suppressed. LEVEL: this is a cost/value change, not a debugging tweak. The branch used to be reachable only by TYPED control traffic (a packaged delivery could not match the CLR-type test), so it was rare and Warning was the right price. Reading the contract off the envelope makes it reachable once per SUPPRESSED delivery, and the moment that happens in bulk is a pod shutdown — prod 2026-08-10 measured 944 on one dying pod, i.e. ~1k Loki lines per shutdown, billed forever, emitted exactly when the process has least capacity. The value is near zero: suppression here is the DESIGNED outcome for a DeliveryFailure or a [CanBeIgnored] message, and MayAnswer() is false for nothing else, so the line reports normal behaviour rather than an anomaly. The storm it replaces — a posted NACK per message, each itself routed and logged — was the expensive thing; keeping a per-message Warning would bank a fraction of the same bill for no diagnostic gain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
⛔ Not merging yet — CI went red on an unrelated test and I am not satisfied it is unrelated
Why I did not just re-run it. Re-running would have been green (the next commit's full CI was What I established
The measurement that stopped me
That is suggestive but not conclusive — under the null hypothesis of an equal rate, all three Re-running both arms at 110 iterations to separate the hypotheses |
✅ Resolved — the branch was exonerated by the larger sample, and merged on a genuine greenRe-ran both arms at 110 iterations. The verdict inverts the first sample:
So
Merged on |
Closes #1485.
The defect
MeshBuilderis the only call site ofIRoutingService.DeliverMessage, and it passesdelivery.Package(routes.Hub.JsonSerializerOptions).Packagereplaces the payload withRawJson. Every answer-once guard in the routing layer was a CLR-type test ondelivery.Message—is DeliveryFailure/HasAttribute<CanBeIgnoredAttribute>()— so on therouted path it inspected
RawJsonand could never match.Consequence: the routers DID NACK
[CanBeIgnored]heartbeats to a departed owner — re-postedevery heartbeat interval, which is the NotFound storm the guards' own comments describe — and
COULD answer a
DeliveryFailurewith aDeliveryFailure.One thing in the issue was wrong
#1485 read the monolith
RoutingServiceBaseguards as sitting before packaging and thereforestill working, making the two routers "disagree in production". They do not. Both routers are
behind the same single packaging call, so the comment claiming they "both agree" was true only in
that both were uniformly dead.
MonolithRouterAnswerOnceAfterPackagingTestis that claim madeexecutable — it fails on the pre-fix tree exactly like the Orleans one.
The sweep found seven sites, not two
RoutingGrain.PostFailureOrleansRoutingService.MayAnswer(2 call sites)RoutingServiceBase.NackRouteFailureRoutingServiceBase.PostNotFoundMonolithRoutingService.PostNotFound[CanBeIgnored]clause outrightMessageService.NackThroughParent[CanBeIgnored]half dead behind its RawJson clauseMessageService.ReportFailureThat last one matters most:
ReportRoutingFailurereports whatever the mesh's route handler handsback, and that handler is
DeliverMessage(delivery.Package(...))— so fixing only the routerswould have moved the identical storm one level up, on the one path where a router returns
Failedsynchronously (the Orleans shutdown short-circuit, i.e. every pod shutdown).
The fix
Follows the precedent already inside
PackageforIDiagnosticKeyed(#1200): stamp the factonto the envelope immediately before the type is erased, and have every guard read the envelope via
delivery.MayAnswer()(newAnswerPolicy, inMeshWeaver.Messaging.Contract).The fact, not the type name — the one question the routing layer asks is "may I answer this", and
a general-purpose type oracle on the envelope would invite the payload-parsing decisions this
codebase deliberately avoids. Only the suppressed case is stamped, so ordinary traffic pays
nothing and an unstamped delivery (never packaged, or pre-serialised by an external client) degrades
to exactly the old CLR-type behaviour — never to "answer something you must not". The property is
presence-checked, never value-compared: a delivery property crosses the gRPC/SignalR wire as
JSON and returns as a
JsonElementrather than theboolthat was stamped, which is the trapMessageStormBreaker.ResolvePayloadKeydocuments for the sibling stamp.Fail-before, demonstrated
Each test was run on the pre-fix tree and is RED there — all three deliveries (heartbeat,
DeliveryFailure, ordinary control) were answered where exactly one should be:
OrleansRouterAnswerOnceAfterPackagingTest— realIHostApplicationLifetime, real hub at thesender's address;
found 3MonolithRouterAnswerOnceAfterPackagingTest— real monolith mesh, realIRoutingService;found 3RoutingTailAnswerOnceAfterPackagingTest— the Orleans shutdown branch's exact return shape(packaged, then
Failed);found 3(verified by reverting just that guard and re-running)Each also carries a positive control dispatched LAST through the same serial path, so "the
suppressed ones were processed" is proven rather than waited for.
Packaging_ErasesTheGuardsInputpins the mechanism itself.
Verification
dotnet build -c Release -warnaserror:MeshWeaver.Messaging.Hub.Test,MeshWeaver.Hosting.Orleans.Test,MeshWeaver.Hosting.Monolith.Test— 0 warnings, 0 errors(these transitively cover every touched src project and its dependents)
MeshWeaver.Messaging.Hub.Test— 242/242MeshWeaver.Hosting.Orleans.Testrouting suite — 19/19MeshWeaver.Hosting.Monolith.Testrouting/NACK subset — 32/32MeshWeaver.Documentation.Test— 21/21 (doc edit + What's New node)Not in this PR
#1486 is deliberately left open — see the analysis posted there. Short version: its natural fix
(route the NACK through the same path forward traffic takes, instead of publishing straight to the
Orleans stream) is only safe once this PR has landed, because without the answer-once guard it
would create a real NACK loop. That is very likely why the direct stream publish exists at all.
🤖 Generated with Claude Code