Repository navigation
fix(#1486): the NACK takes the local route the forward message took - #1612
Merged
Merged
Conversation
RoutingGrain.PostFailure returned the DeliveryFailure by publishing to the sender's Orleans stream, unconditionally. Every OTHER same-process delivery to a co-hosted hub short-circuits on OrleansRoutingService's local route table and never touches a stream — so the failure leg was the WEAKEST inbound path in the system: a hub whose stream subscription was never attached, or was attached and then lost (SubscribeWhenStreamingReadyAsync can give up while the local route stays live), is reachable for forward traffic and unreachable for NACKs. And it fails silently by construction: a publish to a memory stream with NO live subscriber SUCCEEDS. Nothing faults, the ContinueWith never sees IsFaulted, and the NACK is simply gone — so the requester waits forever for an answer the router believes it sent. That is the [STALE-CALLBACK] shape. PostFailure now asks the routing service for the sender's LOCAL route first and delivers through it when this silo hosts the sender — the same step-1 short-circuit the forward leg takes, so the answer travels the way the question did. Only a sender on another silo still needs the stream, and that path's trace line is renamed FAILURE_DELIVER_OK_UNCONFIRMED because "the publish did not fault" is all it can honestly claim. 🚨 Not closed by adding a bound. The issue is explicit that a timeout here would convert a silent hang into a logged one without ever delivering the NACK, so the fix is the routing decision, not a deadline. This also settles the issue's third question — SubscribeWhenStreamingReadyAsync giving up while the local route stays live is "two views of reachability that can disagree, with nothing reconciling them". For a co-hosted hub they no longer disagree, because the NACK now consults the same view the forward path does. Its SECOND question is deliberately left open: for a genuinely remote sender, a publish with no subscriber still cannot distinguish delivered from discarded. That needs a stream-level answer this change does not have, and it is now the only remaining case rather than the majority one. #1563 is the caller's side of the same defect — a ContentRoute GetDataRequest to an unreachable hub waiting out its full 60 s instead of taking the prompt NotFound. Its sender is the portal's own hub, i.e. co-hosted with the router, so its NACK went down exactly this path. Commented there rather than claimed closed here: one occurrence, no repro, so the honest statement is the mechanism, not a guarantee. Verified: 3 new tests pin the routing decision (a co-hosted sender resolves and DELIVERS; a sender this silo does not host has no local route, so the stream stays its transport; a deregistered hub stops resolving, or the fix would trade a silent stream drop for a silent local one). MeshWeaver.Hosting.Orleans.Test 163/163. -c Release -warnaserror clean for MeshWeaver.Connection.Orleans, MeshWeaver.Hosting.Orleans and the test project. Fixes #1486 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmBFaHTqhy7h742wkAaJKj
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a routing asymmetry in the Orleans router where RoutingGrain.PostFailure (NACK/DeliveryFailure) always published to an Orleans stream, even when the sender hub is co-hosted and forward traffic uses the local-route short-circuit. The change makes failure responses follow the same local route as the original forward message when possible, avoiding silent NACK loss when stream subscriptions are missing.
Changes:
- Route failure deliveries via
OrleansRoutingServicelocal route table first (co-hosted sender case), falling back to stream publish only when no local route exists. - Expose
OrleansRoutingService.TryGetLocalRoute(...)so the failure leg can reuse the forward-leg’s local-route decision. - Add focused tests pinning the local-route vs stream decision, plus a WhatsNew entry describing the user-visible improvement.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs |
Uses a local-route lookup to deliver DeliveryFailure to co-hosted senders, avoiding stream-only NACK delivery. |
src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs |
Adds TryGetLocalRoute accessor to expose the existing local-route table to the failure leg. |
test/MeshWeaver.Hosting.Orleans.Test/RoutingFailureTakesTheLocalRouteTest.cs |
New tests intended to verify local-route resolution and non-resolution behavior for senders. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-14-faster-failure-answers.md |
Documents the faster/more reliable failure-answer behavior as a user-facing fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Copilot is right: the counter was bumped in the delegate body, so it read 1 for a route whose returned observable was never subscribed — the assertion "resolving the route must actually deliver" would have passed without the NACK ever being handed over. Deferring the increment makes it mean what it says, and matches how PostFailure actually calls it: Invoke(...).Subscribe(...). Added the negative half in the same test: after Invoke and before Subscribe the count must still be 0, which is what proves the counter now tracks the subscription rather than the lookup. 3/3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmBFaHTqhy7h742wkAaJKj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RoutingGrain.PostFailurereturned theDeliveryFailureby publishing to the sender's Orleans stream — unconditionally.Every other same-process delivery to a co-hosted hub short-circuits on
OrleansRoutingService's local route table and never touches a stream. So the failure leg was the weakest inbound path in the system: a hub whose stream subscription was never attached — or was attached and then lost, whichSubscribeWhenStreamingReadyAsynccan do while the local route stays live — is reachable for forward traffic and unreachable for NACKs.And it fails silently by construction: a publish to a memory stream with no live subscriber succeeds. Nothing faults, the
ContinueWithnever seesIsFaulted, and the NACK is simply gone — so the requester waits forever for an answer the router believes it sent. That is the[STALE-CALLBACK]shape.The fix is the routing decision, not a deadline
PostFailurenow asks the routing service for the sender's local route first and delivers through it when this silo hosts the sender — the same step-1 short-circuit the forward leg takes. The answer travels the way the question did.Only a sender on another silo still needs the stream, and that path's trace line is renamed
FAILURE_DELIVER_OK_UNCONFIRMED, because "the publish did not fault" is all it can honestly claim.🚨 The issue is explicit that a timeout here "would only convert a silent hang into a logged one; it would not deliver the NACK" — so no bound was added.
What this settles, and what it does not
Question 3 — settled. "
SubscribeWhenStreamingReadyAsyncgiving up while the local route stays live is its own inconsistency: two views of reachability that can disagree, with nothing reconciling them." For a co-hosted hub they no longer disagree, because the NACK now consults the same view the forward path does.Question 2 — deliberately open. For a genuinely remote sender, a publish with no subscriber still cannot distinguish delivered from discarded. That needs a stream-level answer this change does not have — it is now the remaining case rather than the majority one.
Relationship to #1563
#1563 is the caller's side of the same defect: a
ContentRouteGetDataRequestto an unreachable hub waiting out its full 60 s instead of taking the prompt NotFound. Its sender is the portal's own hub — co-hosted with the router — so its NACK went down exactly this path.I've commented there rather than claiming it closed here: one occurrence, no repro, so the honest statement is the mechanism, not a guarantee.
Verification
Three tests pin the routing decision, including the two ways a fix here could be wrong:
MeshWeaver.Hosting.Orleans.Test163/163.-c Release -warnaserrorclean forMeshWeaver.Connection.Orleans,MeshWeaver.Hosting.Orleansand the test project.Fixes #1486
🤖 Generated with Claude Code
https://claude.ai/code/session_01KmBFaHTqhy7h742wkAaJKj