Mesh hub: never persist its transient node; never NACK a message addressed to it#475
Mesh hub: never persist its transient node; never NACK a message addressed to it#475rbuergi wants to merge 3 commits into
Conversation
…essed to it
The root mesh hub's address (Address.Type == "mesh") is transient per-process
infrastructure (a fresh guid every process via CreateMeshAddress), NOT an
addressable node. Two leaks, each a known catastrophe class:
B — never persist (StoragePostCommitFlush.Flush): when the flushing hub is the
mesh hub, log an error and skip the write (the ack still resolves true, so
read-after-write is intact). Persisting mesh/{id} just orphaned a DB row on every
restart ("puke standing") — the stray mesh/{id} rows another agent found in the DB.
Same as portals: routable, never persisted.
Receive-guard (MessageHub.FinishDelivery): a message addressed to the mesh hub
that reaches no handler is LOGGED as an error (to expose the abusing sender) and
returns delivery.Processed() — NOT a DeliveryFailure/NACK back to the sender.
Echoing an error back wedges the sender (it treats it as a fault, retries → storm
→ the 60s-timeout mesh-wide-outage class). NOTE the non-obvious part: delivery.Ignored()
is NOT enough — MessageService turns an on-target Ignored into a DeliveryFailure{Ignored}
sent back to the sender (see UnhandledMessageReportsFailureTest). Only Processed()
("handled, no failure") fully suppresses the echo; a requesting sender simply times out.
Dedicated test MeshHubNotAnEndpointTest: an unhandled IRequest<T> to the mesh hub
must yield a TimeoutException (Ignored/no-NACK), never a DeliveryFailureException —
the exact inverse of UnhandledMessageReportsFailureTest.
Deferred (separate): registering the mesh hub as a routing participant (so routing
resolves mesh/{id} instead of CreateHub-ing it) — the naive WithInitialization →
RegisterStream deadlocks the mesh at startup because IRoutingService is mesh-scoped
(circular: mesh hub construction → resolve IRoutingService → needs the mesh hub).
Needs a post-construction hook or routing-side self-recognition of the mesh address.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the root mesh hub (Address.Type == "mesh") as transient routing infrastructure (not an addressable/persisted node) by preventing two failure modes: (1) persisting mesh/{id} into storage and (2) NACK/DeliveryFailure echoing back to senders for unhandled messages targeting the mesh hub (which can trigger retry storms and wedge the system).
Changes:
- Add a receive-guard in
MessageHub.FinishDeliveryso unhandled deliveries targeting the mesh hub are logged and treated asProcessed()(no echoed failure). - Prevent
StoragePostCommitFlushfrom persisting the mesh hub’s transient node (log + skip write, still acktrue). - Add an integration-style test pinning the invariant: an unhandled request addressed to the mesh hub must time out (no
DeliveryFailureException).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/MeshHubNotAnEndpointTest.cs | New test asserting that requests sent directly to the mesh hub are ignored (no NACK), resulting in a timeout. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Guard in FinishDelivery to suppress failure echoing for unhandled messages targeting the mesh hub, while logging an error for visibility. |
| src/MeshWeaver.Hosting/Persistence/StoragePostCommitFlush.cs | Skip persistence for mesh-hub node commits to avoid orphan DB rows for transient mesh/{id} addresses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 🚨 The root mesh hub is routing INFRASTRUCTURE, not a message endpoint — nothing | ||
| // should ever address it directly (its `mesh/{id}` is a transient per-process id, not | ||
| // an addressable node). If something did and no handler matched, LOG loudly so the | ||
| // abusing sender is visible, but return Ignored: answering a DeliveryFailure/NACK back | ||
| // to the sender is a WEDGE (the sender treats it as a fault → retries → storm — the |
Test Results (shard 0)889 tests ±0 879 ✅ - 1 4m 40s ⏱️ +19s For more details on these failures, see this check. Results for commit 129d3f2. ± Comparison against base commit c10686c. ♻️ This comment has been updated with latest results. |
Test Results (shard 2)1 255 tests ±0 1 151 ✅ - 1 4m 45s ⏱️ -3s For more details on these failures, see this check. Results for commit 129d3f2. ± Comparison against base commit c10686c. ♻️ This comment has been updated with latest results. |
Test Results (shard 3) 12 files 12 suites 3m 59s ⏱️ For more details on these failures, see this check. Results for commit 129d3f2. ♻️ This comment has been updated with latest results. |
Test Results (shard 1)472 tests - 341 285 ✅ - 341 1m 5s ⏱️ - 3m 40s For more details on these failures, see this check. Results for commit 129d3f2. ± Comparison against base commit c10686c. This pull request removes 341 tests.♻️ This comment has been updated with latest results. |
Test Results (shard 5)1 253 tests ±0 1 239 ✅ - 13 7m 32s ⏱️ + 2m 28s For more details on these failures, see this check. Results for commit 129d3f2. ± Comparison against base commit c10686c. ♻️ This comment has been updated with latest results. |
Test Results 60 files - 2 60 suites - 2 27m 42s ⏱️ -29s For more details on these failures, see this check. Results for commit 129d3f2. ± Comparison against base commit c10686c. This pull request removes 341 tests.♻️ This comment has been updated with latest results. |
…nly never-persist CI on #475 failed 5/6 shards — 16 message-flow / data-change / activity tests (Acme, AI, Content, GitSync, Orleans, Persistence, Threading) timed out at 60s. Root cause: the FinishDelivery receive-guard was built on a wrong premise. The root mesh hub is NOT a pure non-endpoint — legitimate framework flows POST requests to it and AWAIT the response (even an unhandled-NACK unblocks the caller). Returning Processed() gave those callers NO response, so they hung until timeout. Revert the FinishDelivery change entirely (MessageHub back to baseline) and drop MeshHubNotAnEndpointTest. Keep the safe, independent, valuable part: StoragePostCommitFlush still refuses to persist the mesh hub's own transient mesh/{id} node (the actual DB-puke bug). A correct wedge-safe receive-guard needs a much narrower predicate and its own analysis — deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Closing: all three approaches in this PR were defective and reverted — (A) registering the mesh hub in its own WithInitialization deadlocks (IRoutingService is mesh-scoped → circular); (B) the never-persist guard checked the injected hub, which is the SINGLETON mesh hub for every node's flush, so it skipped ALL persistence (16 data-change/comment/version/activity tests red); (C) the receive-guard returned Processed() for unhandled mesh-hub messages, but legitimate flows POST-and-await the mesh hub, so they hung → timeout. The stray mesh/{id} DB rows need the actual write path confirmed first, then a fix keyed on the NODE identity, verified locally against the persistence tests. Documented for a proper redo. |
The root mesh hub's address (
Address.Type == \"mesh\") is transient per-process infrastructure — a fresh guid every process (CreateMeshAddress), not an addressable node. It leaked two ways, each a known catastrophe class. This PR fixes both; a third (routing registration) is deferred because the naive version deadlocks — see below.B — never persist the transient node (
StoragePostCommitFlush.Flush)When the flushing hub is the mesh hub, log an error and skip the write (the ack still resolves
true, so read-after-write is intact). Persistingmesh/{id}orphaned a DB row on every restart — the straymesh/{id}rows found in the database. Same behaviour as portals: routable, never persisted.Receive-guard — never NACK back (
MessageHub.FinishDelivery)A message addressed to the mesh hub that reaches no handler is logged as an error (to expose the abusing sender) and returns
delivery.Processed()— never aDeliveryFailureback to the sender. Echoing an error back wedges the sender (it treats it as a fault → retries → storm → the 60s-timeout mesh-wide-outage class).Non-obvious trap the test caught:
delivery.Ignored()is not enough —MessageServiceturns an on-targetIgnoredinto aDeliveryFailure{Ignored}sent back to the sender (seeUnhandledMessageReportsFailureTest). OnlyProcessed()fully suppresses the echo; a requesting sender simply times out.Test
MeshHubNotAnEndpointTest— an unhandledIRequest<T>addressed to the mesh hub must yield aTimeoutException(Ignored / no-NACK), never aDeliveryFailureException. The exact inverse ofUnhandledMessageReportsFailureTest. Red → green across the fix.Validation
Messaging.Hub.Test89/89; aMonolithMeshTestBasetest green (mesh starts fine).-warnaserrorclean:Messaging.Hub,Hosting.Deferred (separate): register the mesh hub as a routing participant
So routing resolves
mesh/{id}instead ofCreateHub-ing it. The naiveWithInitialization → RegisterStream(hub)deadlocks the mesh at startup (deterministic SIGABRT at the 60s timeout):IRoutingServiceis mesh-scoped, so resolving it during the mesh hub's own construction is circular. The correct fix is a post-construction hook or routing-side self-recognition of the mesh address — a separate, carefully-tested change, not rushed in here.🤖 Generated with Claude Code