fix(routing): the ordering FIFO's channel is (destination, stream), not the destination by rbuergi · Pull Request #5151 · Systemorph/MeshWeaver

rbuergi · 2026-09-21T19:57:39Z

Addresses the head-of-line shape of #5009 (sev:H, live on both production portals). Deliberately does NOT touch #5018's shape — see "What this does not fix".

What the defect actually is

Not a stuck leg, and not a bound that is too small. The router's ordering FIFO was keyed on the wrong thing.

OrderedRouteDispatcher serialises route legs per destination address so the delta protocol's receive-side monotonicity guard stays sound. But that guard operates per mirror — per synchronization stream — and a stream-routed address is a multiplexer: cache/{meshId} fronts one sync/{streamId} sub-hub per node the process mirrors, and DataExtensions.RouteStreamMessage receives every frame at the hub's own address and forwards it internally by StreamId. So the router never sees a per-stream address, and keying on the address alone put a whole process's data-sync traffic on one channel with one in-flight IPodHubGrain.Deliver grain call.

Where that destination is on another silo, the channel's service rate is one network round trip per frame (plus Orleans' JsonCodec copy of the full delivery, both ways). A busy portal's change fan-out exceeds it.

The evidence, and the asymmetry that names the mechanism

memex-cloud 2026-09-20 19:07–20:34Z, 17 crossings, both pods:

Pod	In flight	Destinations queued	Deepest	Target that crossed
`…-s246c`	64	1	62	`cache/12xX8OXQIEmdwvf_ZZ8LjA`
`…-ndnxt`	64	0	0	a different address every time

cache/{meshId} is keyed by the mesh hub's id, which is per process, so each pod has its own cache address and its own PodHubGrain activation on its own silo. A pod's channel to its own cache hub is a local call and drains as fast as it fills → deepest 0. Its channel to the peer pod's cache hub is a network round trip → 62 legs stacked. One pod reporting head-of-line blocking while the other reports load was never two faults; it is one fault seen from the two ends of one cross-silo channel. (The same 12xX8… address appears from gx6z6 on 09-19 and from s246c on 09-20 — both the non-ndnxt pod of the day.)

Episode stamps rose throughout on both activations, so every episode drained: no slot leaked and no leg was stuck. The 62 legs were independent streams waiting on each other because the channel key could not tell them apart. The log line's rule (deepest ≥ 1 means legs are blocked behind a leg) was applied correctly and still yielded the wrong conclusion, because it is silent about why the legs share a queue.

Why #1172 did not hold

It is the third of the three options: a different path sharing a fingerprint. #1172 was never closed by a fix to this queueing structure, and it predates the instrument that separates the two shapes.

Closed 2026-08-10 on An in-place update no longer costs the router a storage query per message #1179 — an amplifier (an in-place Updated no longer costs the router a storage query per routed message).
Reopened 2026-08-12 when the signature returned in prod and in a 2-vCPU CI container. Live dotnet-stack / EventPipe profiling on the pod then reclassified it: "nothing is stuck. The silo is CPU-starved" — unbounded synchronous Rx recursion on hub turns, driven by _Activity/import-* patch volume. A separate comment corrected the ticket's premises outright: 64 is RoutingGrain.SaturationThreshold, a MeshWeaver constant, not Orleans' NonReentrancyQueueSize, and "nothing throttles, queues or refuses at 64".
Closed again 2026-08-13 — with the log line rewritten to say what it actually saw, which is the change that first added deepest per-destination queue to the line.

So #1172's accepted diagnosis is deepest 0 — #5018's shape. Every fix on that ticket lowered the arrival rate or the service time of a lane that was still one lane wide (storage query per message #1179, O(node-size) patch construction #1341, the cancelled-drain slot leak #1358, seven copies of every timed-out delivery). A single-server queue whose arrival rate exceeds its service rate recurs at any offered load above that rate: reducing the load moves the threshold, it does not remove it. The head-of-line shape only became visible in the 2026-08-13 rewrite, and both #5009 and #5018 inherited the same fingerprint e4a97855ab595beb — which is exactly why "#1172 is primary" kept being cited. #4903 was closed as a duplicate consolidated onto #4904; the shape itself was never worked.

Independently corroborated: Doc/Architecture/ATimedOutDeliveryIsStillHeldByTheCallee already states this diagnosis in prose — FIFO key is the address, invariant is per stream, "that over-broadness is what lets one slow destination stack ~62 legs" — and marks it "Not addressed here." This PR is that.

The fix

The ordering domain is the stream, so the channel key is the stream. The router cannot read a stream id out of the payload — on the turn the payload is RawJson, and parsing it there is the one thing the turn must never do — but it does not have to: the identity is already on the envelope. MessageDelivery.Package stamps IDiagnosticKeyed.DiagnosticKey (for every StreamMessage, the stream id) onto the delivery's properties immediately before erasing the payload type, and every mesh delivery reaches the router through Package.

var orderingKey = DeliveryIdentity.Read(delivery);          // one dictionary lookup of a string
orderedDispatcher.Enqueue(addressPath, orderingKey, leg, onLegCompleted);

Same stream ⇒ same channel. Arrival order preserved exactly, one in-flight leg, as before.
Different streams ⇒ different channels. Their round trips overlap.
No identity ⇒ the destination-wide channel, byte-for-byte the previous behaviour — never a guess.
The key is a (string, string?) pair, never a concatenation: mesh addresses contain both / and ~, so any single delimiter would make ("cache/a", "b/c") and ("cache/a/b", "c") one channel.

This raises no bound. The routing IIoPool cap (256), SaturationThreshold (64, still a log gate and nothing else) and PodHubGrain's non-reentrancy are untouched. Only legs with no ordering relationship stop waiting on each other — the pipelining a correct channel key allows and an over-coarse one forbade. PodHubGrain.Deliver is a synchronous hand-off into the target hub's own queue, so what overlaps is the network, not any work. No timeout, retry, pool size or watchdog is added or changed.

DeliveryIdentity.Read is shared with MessageStormBreaker, which keys its rate buckets on the same identity. Both layers ask which messages are about one thing?, and both fail the same way when they cannot answer: the breaker folds a legitimate fan-out into one bucket and drops it (#1200), the router folded independent streams into one channel and queued them. One answer, one place.

The report now reads

ordered channels queued 64 over 1 stream destination(s), deepest per-channel queue 0

A non-zero Deepest is now a much stronger statement: frames of the same stream are stacking up. The former reading reports as many channels over few destinations, which is load. Both counts are printed because their ratio is the remaining discriminator. Everything else on the line is unchanged, including the ThreadPool-wait caveat and the episode stamp. Doc/Architecture/LogWatchTriage's consolidation rule is updated for the renamed field, with the note that a line from an older image spells it the old way.

Controls — measured on both sides

Negative control run by reverting the channel key to destination-only (new Channel(destination, null)), rebuilding and re-running the same 10 tests:

	destination-only key	channel key
`IndependentStreamsOnOneDestination_NeverWaitOnEachOther`	FAIL (10 s timeout — the second stream's leg is never subscribed)	pass
`OneMultiplexerDestination_ManyStreams_IsBreadthNotHeadOfLine`	FAIL (1 channel / deepest 63 instead of 64 / 0)	pass
`FramesOfOneStream_OnAMultiplexer_StillSubscribeInOrderOneAtATime`	pass	pass
`ChannelKeyIsAPair_NotAConcatenation`	pass	pass
`SameInFlightCount_MeansBothBusyAndBlocked_…`	pass	pass
`OrderedRouteDispatcherTest` × 4 (order, independence, release, unordered negative control)	pass	pass
`OrderedRouteDispatcherDrainRecursionTest`	pass	pass
total	8 / 10	10 / 10

The order and drain invariants hold on both sides — the narrowing could have broken them, and the tests say it did not. DeliveryIdentitySurvivesPackagingTest (4 cases, MeshWeaver.Messaging.Hub.Test) pins the seam the whole fix rests on, because both readers degrade silently to their pre-identity behaviour when the stamp is absent: identity readable after Package erased the type, after a JSON hop (value arrives as JsonElement), and null for a payload that exposes none.

MessageStormBreakerTest (unchanged behaviour, refactored resolver) and DocumentationLinkIntegrityTest also pass. dotnet build -c Release -warnaserror clean (0 Error(s), 0 Warning(s)) on MeshWeaver.Messaging.Contract, MeshWeaver.Hosting.Orleans, MeshWeaver.Messaging.Hub.Test, MeshWeaver.Hosting.Orleans.Test, MeshWeaver.Documentation.Test.

What this does NOT fix

🚨 #5018's shape is untouched and must stay a separate defect. deepest 0 with many channels is dispatch volume / ThreadPool starvation, where a slot is charged for an unbounded wait before the leg's own timeouts start and load is indistinguishable from a leak. Nothing here narrows it. It does change #5018's fingerprint, because both shapes share one log site and that site's wording moved — unavoidable, and noted on the ticket. The incident-filer's episode-stamp fingerprint splitting (also on #5018) is a MeshWeaver.Plugins concern and is not touched.

Production verification — this cannot be closed on CI

The defect is only observable under real cross-silo fan-out on a multi-pod portal. What would verify it, in order:

The image is actually running — read the running tag per instance on the Fleet Console (/Hosting/Console), not the CD tick.
🚨 RoutingGrain is a grain activation and binds nothing from a node, but the pods must be REPLACED for the new assembly to serve — a roll ends the activations on the pods it replaces. No address needs an explicit recycle for this change: the router is not a per-node hub and holds no NodeType binding.
The positive signal on memex-cloud with ≥ 2 pods and normal load: [ROUTE] Routing back-pressure lines, if they appear at all, read deepest per-channel queue 0 with ordered channels queued ≈ the in-flight count over a small number of destinations. A line still reading the OLD field names is a pre-fix replica, not a result.
The falsifier: any line with deepest per-channel queue ≥ ~10 on a cache/… destination means frames of ONE stream are genuinely stacking up — a different, real defect this fix does not cover, and the ticket should be re-opened naming it.
The counter-check that it did not break ordering, which is the risk this change carries: zero increase in "awaiting first data" / "Building layout…" stuck-view reports, and no StreamVersionMonotonicityTest-shaped regression in the suites. A reordering regression is silent by construction (nothing is logged above Debug on either side), so this one has to be watched rather than queried.

Handing that over — I have not run any instance action, and I read nothing but the tickets on the live portals.

Two deltas the narrowing carries — named, not left to be found

Both documented in OrderedRouteChannels and in the dispatcher's own remarks.

Control traffic with no identity is no longer ordered against data frames. A DisposeRequest, a heartbeat, a correlated response stays on the destination-wide channel, so it can now overtake or be overtaken by a data frame for a stream on that destination. Nothing relies on the pairing: the entire data-sync protocol is keyed — DataChangedEvent, SubscribeRequest, UnsubscribeRequest, StreamErrorEvent, StreamEndedEvent and every IUserAction carry the stream id (verified in Messages.cs) — responses correlate by request id, and a frame racing its destination's teardown was already a race the router never decided. The one case where a drop at teardown matters is a user action, and that is protected by its UserActionAccepted receipt and the sub-hub registration grace, never by router ordering.
The dispatcher's entry count is bounded by in-flight legs, not in-flight destinations. An entry lives only while a channel holds a leg, and every held leg already holds a route slot — so the bound is the quantity the saturation report prints, at tens of bytes per entry against a whole delivery each. The old key kept the entry count smaller by making the queue behind one entry the unbounded quantity. That was 62.

Review round

Both automatic-review findings were answered on their threads and both are addressed.

The nested <para>/</para></para>: the tags were balanced and MeshWeaver.Hosting.Orleans built clean under -warnaserror (0 Warning(s), 0 Error(s), measured twice at the reviewed commit), so the reported CS1570 was not real — recorded on the thread because this repo now ratchets in-mesh C# on CS1570 by name and a false positive there misdirects the next reader. Flattened to three siblings anyway: it read as unbalanced to a reviewer, which is a real cost.
The mutable HashSet in QueueSnapshot: taken. It was lock-local and never escaped, so it did not carry the risk the collections policy is written against — but the policy is stated without carve-outs on purpose, and queues.Keys.Select(c => c.Destination).Distinct().Count() leaves no collection shape of ours at all. The call runs once per saturation episode (the report latches), so there is nothing to optimise.

Also corrected PodHubDeliveryRollPlan, whose load-bearing "the FIFO cannot be removed" statement described it as per-destination.

CI round: one red, and it was mine

Run tests (shard 4) failed on TestTimeoutLiteralRatchetGuard — "Hand-written test timeouts rose to 387 (baseline 386)". Checked the job's final lines first for the Failed to FinalizeArtifact … (403) Forbidden signature that is reddening jobs elsewhere in the fleet right now — its cause is unknown (the org's Actions cap was checked directly and cannot have fired, so it is not a budget refusal), and it is GitHub-declared non-retryable; this was not that — a real, attributable failure. The three new cases each copied this file's existing [Fact(Timeout = 30_000)].

The guard says not to raise the baseline, and it is right for a sharper reason than style: 30 s is the framework's own write bound (LateResponseWatchBound 30 s + VerdictBoundGrace 1 s), so a test bounded there always gives up one second before the framework can name OwnerUnreachable, and the failure then reads as an anonymous timeout.

Converted all seven sites in the file, not the three that were new:

every positive wait → TestTimeouts.Quick. This dispatcher is in-memory with no mesh and no cluster, which is precisely the "local settle" that bound names, so it now scales with the runner instead of guessing at it.
the outer bound → a literal 120_000, which dominates that wait (Quick × OuterMargin at the CI factor). An attribute argument cannot be a property, so the file states once, where a reader meets it, why the number is a literal and why it is not 30 s.
RoutingBackpressureShapeTest's SpinWait budget → TestTimeouts.Quick too. The regex never caught its TimeSpan.FromSeconds(10), which does not make a guessed 10 s any better on a loaded runner.
the two negative windows stay short literals on purpose: a bounded absence spends its whole window every run, and what it is evidence for is established by the positive half that follows it.

Baseline 386 → 380 — six off the count, and measured, not computed: baseline temporarily 0, count read back out of the failure, the procedure the guard's own history describes.

Local verification

dotnet build -c Release -warnaserror clean (0 Error(s), 0 Warning(s)) on every touched project and on MeshWeaver.Layout.Test as a dependent. Full suites, Release:

Project	Result
`MeshWeaver.Hosting.Orleans.Test`	306 / 306
`MeshWeaver.Messaging.Hub.Test`	493 / 493
`MeshWeaver.Data.Test`	533 / 533
`MeshWeaver.Layout.Test`	519 / 519
`MeshWeaver.Hosting.Test`	861 / 862 (1 skipped)
`MeshWeaver.Documentation.Test`	604 / 604

The Orleans project is the one that matters most — it is the only suite that exercises this code through a real cluster — and its 51 Sync/Stream/Subscribe cases are the ones that would catch a reordering regression.

Pairs-with: none — no public type or member is removed. The only public-surface change is an ADDED type (DeliveryIdentity); OrderedRouteDispatcher and every member whose name changed (ActiveDestinations → ActiveChannels, QueueSnapshot's tuple) are internal.

Refs #5009, #5018, #1172, #4903.

🤖 Generated with Claude Code

…ot the destination A stream-routed address is a MULTIPLEXER: `cache/{meshId}` fronts one `sync/{streamId}` sub-hub per node the process mirrors, and `DataExtensions.RouteStreamMessage` receives every frame at the hub's OWN address and forwards it internally by StreamId. So keying `OrderedRouteDispatcher`'s FIFO on the destination address put a whole process's data-sync traffic on ONE channel with ONE in-flight `IPodHubGrain.Deliver` grain call. Where the destination is on another silo that channel's service rate is one network round trip per frame — far below a busy portal's change fan-out. memex-cloud measured the consequence for ~1.5 h on both pods (#5009): 62 of 64 in-flight legs queued behind one `cache/…` address on one pod, while the peer pod — whose own cache channel is LOCAL and therefore fast — reported deepest 0 and read as load. Every episode drained, so nothing leaked and no leg was stuck; the 62 legs were independent streams waiting on each other because the channel key could not tell them apart. The invariant the FIFO protects is the delta protocol's receive-side monotonicity guard, which operates per MIRROR, i.e. per stream. The channel key now is too. The identity is `IDiagnosticKeyed.DiagnosticKey` — the stream id for every `StreamMessage` — which `MessageDelivery.Package` already stamps on the envelope before erasing the payload type, so `RoutingGrain` reads it with one dictionary lookup of a string on the turn: no cast, nothing parsed. Same stream ⇒ same channel ⇒ arrival order preserved exactly; different streams ⇒ different channels; no identity ⇒ the destination-wide channel, unchanged. Raises no bound: the routing IIoPool's cap, `SaturationThreshold` and `PodHubGrain`'s non-reentrancy are all untouched. Only legs with no ordering relationship stop waiting on each other. `DeliveryIdentity.Read` is shared with `MessageStormBreaker`, which keys its rate buckets on the same identity — "which messages are about one thing" must be one answer, or an ordered channel and a rate bucket drift apart. The saturation report now prints `ordered channels queued N over M stream destination(s), deepest per-channel queue D`; a non-zero D is a far stronger statement than before. The load shape at the same log site (deepest 0, a slot charged for an unbounded ThreadPool wait) is a separate defect and is NOT narrowed here. Controls measured on both sides: with the destination-only key the two blocking tests fail (10 s timeout — the second stream's leg is never subscribed) while all 8 order/load/drain controls pass; with the channel key all 10 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…rries Neither is a problem, and both are reviewer-findable, so they are stated rather than left to be discovered. 1. Control traffic with no identity (a DisposeRequest, a heartbeat, a correlated response) stays on the destination-wide channel and is therefore no longer ordered against that destination's data frames. Nothing relies on that pairing: the whole data-sync protocol IS keyed — DataChangedEvent, SubscribeRequest, UnsubscribeRequest, StreamErrorEvent, StreamEndedEvent and every IUserAction carry the stream id — responses correlate by request id, and a frame racing its destination's teardown was already a race the router never decided. The one case where a drop at teardown matters is a user ACTION, and that is protected by its UserActionAccepted receipt and the registration grace, never by router ordering. 2. The dispatcher's entry count is now bounded by in-flight LEGS rather than by in-flight DESTINATIONS. An entry lives only while a channel holds a leg and every held leg already holds a route slot, so the bound is the quantity the saturation report prints — tens of bytes per entry against a whole delivery each. The previous key kept the entry count smaller by making the queue behind one entry the unbounded quantity, which is what 62 was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Correct identity wording and address the dispatcher documentation and immutable-collection findings.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Remove duplicate XML documentation closing tag · New
Use immutable collection for snapshot path · New

What changed in this PR

Updates routing FIFO channels from destination-only to (destination, identity), allowing independent streams to proceed concurrently while preserving ordering.

Changes:

Adds shared delivery-identity resolution.
Updates routing, diagnostics, and storm detection.
Adds regression tests and architecture documentation.

File	Reviewed change
`test/MeshWeaver.Messaging.Hub.Test/DeliveryIdentitySurvivesPackagingTest.cs`	Tests identity preservation across packaging.
`test/MeshWeaver.Hosting.Orleans.Test/RoutingBackpressureShapeTest.cs`	Tests routing back-pressure behavior.
`test/MeshWeaver.Hosting.Orleans.Test/OrderedRouteDispatcherTest.cs`	Tests FIFO ordering and stream independence.
`test/MeshWeaver.Hosting.Orleans.Test/OrderedRouteDispatcherDrainRecursionTest.cs`	Tests dispatcher drain behavior.
`src/MeshWeaver.Messaging.Hub/MessageStormBreaker.cs`	Reuses shared identity resolution.
`src/MeshWeaver.Messaging.Contract/DeliveryIdentity.cs`	Provides delivery identity resolution.
`src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs`	Applies identity-based routing and diagnostics.
`src/MeshWeaver.Hosting.Orleans/OrderedRouteDispatcher.cs`	Implements pair-keyed FIFO channels.
`src/MeshWeaver.Documentation/Data/Architecture/OrderedRouteChannels.md`	Documents routing channel behavior.
`src/MeshWeaver.Documentation/Data/Architecture/LogWatchTriage.md`	Updates triage guidance.
`src/MeshWeaver.Documentation/Data/Architecture/ATimedOutDeliveryIsStillHeldByTheCallee.md`	Updates the diagnosis reference.
`src/MeshWeaver.Documentation/Data/Architecture.md`	Adds the architecture index entry.

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

rbuergi · 2026-09-21T20:04:09Z

-            foreach (var queue in queues.Values)
+            // Allocated only here, and this runs once per saturation episode (the report latches) or
+            // from a test — never on the dispatch path.
+            var destinations = new HashSet<string>(StringComparer.Ordinal);


Agreed, taking the second of your two suggestions — queues.Keys.Select(k => k.Destination).Distinct().Count(), so the diagnostic introduces no collection shape of its own at all.

The HashSet was a lock-local accumulator that never escaped the method, so it did not carry the risk the collections policy is written against (state that survives a disposal or bleeds across users). But the policy is stated without that carve-out on purpose: the exceptions are the part people copy, and the argument "this one is local so it is fine" is exactly how the next one gets an instance field. An ImmutableHashSet builder would satisfy the letter of it, but this call runs once per saturation episode — the report latches — so there is no reason to hand-build anything when LINQ's own dedupe reads better and leaves nothing for a future reader to reason about.

Pushing with the <para> fix from the other thread.

Both from the automatic review on #5151; answered on their threads. The nested <para> was balanced and the project did build clean under -warnaserror (measured: 0 Warning(s), 0 Error(s)) — so the reported CS1570 was not real — but a <para> inside a <para> closed by a "</para></para>" run reads as unbalanced to a human, which this review demonstrated. Three siblings now. QueueSnapshot's distinct-destination count no longer allocates a mutable HashSet in src/. The set was lock-local and never escaped, so it did not carry the risk the collections policy is written against, but the policy is stated without carve-outs on purpose. LINQ's own dedupe (ordinal by default, which is what an address path needs) leaves no collection shape of ours to reason about, and this call runs once per saturation episode — the report latches — so there is nothing to optimise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… no longer the key PodHubDeliveryRollPlan carries the load-bearing statement that OrderedRouteDispatcher cannot simply be removed — a call into a [Reentrant] grain does not restore ordering. That is unchanged and still true. But it described the FIFO as per-destination, which after this change it is not, so the sentence now says what stays (the FIFO, and claiming the order on the turn) and points at the page that explains what narrowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-21T20:15:31Z