Repository navigation
Suppress the duplicate failure, never the only one - #1070
Conversation
Self-review of #1061. The FailureAlreadyReported marker was set unconditionally — including on the path where the typed NACK's Post THREW and the exception was swallowed and logged: try { Post(new DeliveryFailure(delivery) { ErrorType = ... }); } catch (Exception ex) { logger.LogError(...); } // swallowed return delivery.Failed(reason).WithProperty(FailureAlreadyReported, true); In that window the marker suppressed the unclassified follow-up while no typed failure had gone out, so the caller got NO failure response at all — a silent park, which is precisely the wedge this NACK path exists to prevent. #1061 traded a wrong classification for a possible hang; narrow, but the wrong direction. The marker is now set only when the Post actually returned, in both NACK branches (MessageService's RawJson path and MessageHub's unhandled-message path). Suppress the DUPLICATE, never the only answer. MeshWeaver.Messaging.Hub.Test 167/167. OrleansBrokenNodeTypeAccessTest 2/2, also under DOTNET_PROCESSOR_COUNT=4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AU2kBewK9hnv2WLQB6H8GH
There was a problem hiding this comment.
Pull request overview
Adjusts fallback-hub NACK handling in the messaging hub so the FailureAlreadyReported marker is only applied when the typed NACK is actually sent, preventing suppression of the only failure response and avoiding silent “parks” for callers awaiting a response.
Changes:
- In the RawJson/type-registry fallback NACK path, set
FailureAlreadyReportedonly when the typedDeliveryFailurepost succeeded. - In the fallback hub’s “unhandled message” NACK path, apply the same conditional marker behavior to avoid suppressing the only failure response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/MeshWeaver.Messaging.Hub/MessageService.cs | Conditional FailureAlreadyReported marking in the RawJson fallback NACK path. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Conditional FailureAlreadyReported marking in the fallback hub’s unhandled-message NACK path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ErrorType = nackPolicy.ErrorType, | ||
| NodeTypePath = nackPolicy.NodeTypePath, | ||
| Message = reason | ||
| }, new PostOptions(Address).ResponseFor(delivery)); | ||
| nackPosted = true; |
| ErrorType = nackPolicy.ErrorType, | ||
| NodeTypePath = nackPolicy.NodeTypePath, | ||
| Message = nackPolicy.Reason | ||
| }, o => o.ResponseFor(delivery)); | ||
| nackPosted = true; |
Test Results (shard 0)690 tests 690 ✅ 14m 14s ⏱️ Results for commit e5b1724. |
Test Results (shard 4)1 284 tests 1 279 ✅ 5m 54s ⏱️ Results for commit e5b1724. |
Test Results (shard 1)1 221 tests 1 221 ✅ 6m 23s ⏱️ Results for commit e5b1724. |
Test Results (shard 3) 11 files 11 suites 6m 8s ⏱️ Results for commit e5b1724. |
Test Results 65 files 65 suites 45m 26s ⏱️ Results for commit e5b1724. |
Self-review of #1061, which I merged an hour ago. It had a narrow hole in the wrong direction.
The hole
FailureAlreadyReportedwas set unconditionally — including on the path where the typed NACK'sPostthrew and the exception was swallowed and logged:In that window the marker suppressed the unclassified follow-up while no typed failure had gone out, so the caller got no failure response at all — a silent park. That is precisely the wedge this NACK path exists to prevent, so #1061 traded a wrong classification for a possible hang. Narrow, but the wrong direction.
The fix
The marker is set only when the
Postactually returned, in both NACK branches (MessageService's RawJson path andMessageHub's unhandled-message path).Suppress the duplicate, never the only answer.
Verified
MeshWeaver.Messaging.Hub.Test167/167 ·OrleansBrokenNodeTypeAccessTest2/2, including underDOTNET_PROCESSOR_COUNT=4(the CI-load reproduction).Not this PR
OrleansBrokenNodeTypeAccessTest.Subscribe_ToLayoutAreaOf_NonCompilingNodeTypeInstance_SurfacesError_NotWedgeis currently red on main. It is not caused by #1061 — it passes locally 2/2 even under the CPU constraint, and it is documented failing on PRs #1051 and #1048, both predating that work. It is the GUI-subscription half of the same wedge and still needs its own root-cause pass; it is what is keeping CD's gate shut.🤖 Generated with Claude Code
https://claude.ai/code/session_01AU2kBewK9hnv2WLQB6H8GH