Skip to content

fix: bound the router's own grain call, the fourth transport leg (#2885) - #3032

Merged
rbuergi merged 3 commits into
mainfrom
fix/2885-2896-write-path
Sep 2, 2026
Merged

rbuergi merged 3 commits into
mainfrom
fix/2885-2896-write-path

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What this is

A re-measurement of #2885 and #2896 against the three fixes that landed on main last night (#3018, #3009, #3020), and the one fix that measurement justified.

Neither issue is closed here. Both measurements are posted on the issues; the code change addresses a real gap #2885 exposed, and the docs conserve both findings.


#2885 — the mechanism SURVIVES, and #3018 could not have caught it

#3018 refuses an oversized delivery on three sites — RoutingGrain's two forward grain legs and its memory-stream publish. #2885 died one hop earlier, on the leg that reaches RoutingGrain at all:

OrleansRoutingService.DispatchObservable
  -> grain.RouteMessage(delivery)        <-- Orleans serialises the ARGUMENT here

Orleans' JSON codec is registered AddJsonSerializer(_ => true, _ => true, …), so it claims the IMessageDelivery envelope. All three guarded sites are strictly downstream of this hop, so for the payload that killed the pod, none of them ever ran. OrleansRoutingService contains no reference to MessageSizeGuard on main.

What overturned the prior conclusion

The page Doc/Architecture/OversizedDeliveryRefusal concluded "no guard placed at the router can fix this", reasoning that packaging itself had exhausted memory. The frame order refutes that:

MessageDeliveryConverter.Write                    <- serialises the ENVELOPE
  -> ObjectDefaultConverter.OnTryWrite
  -> JsonPropertyInfo.GetMemberAndWriteJson       <- one property: the payload
  -> Utf8JsonWriter.TranscodeAndWriteRawValue     <- RawJsonConverter.WriteRawValue(string)
  -> SharedArrayPool.Rent -> OutOfMemoryException

MessageDelivery.Package(...) serialises only the payload (JsonSerializer.Serialize(message, …) → new RawJson(serialized)); it never invokes MessageDeliveryConverter, which converts the envelope. So this stack proves the payload was already a materialised RawJson string in memory when the allocation failed — packaging had finished, and what blew up was re-serialising the packaged envelope for transport.

That matters because MessageSizeGuard.IsOversized needs nothing but RawJson.Content.Length: the fast path is 3 × Length < limit, O(1) and allocation-free. It can measure a payload the very next statement cannot transcode. The bound was never the problem — its placement was.

The change

  • OrleansRoutingService.DeliverMessage refuses at this leg before the dispatch, against the live IOptions<ClientMessagingOptions>.MaxMessageBodySize (this assembly is the client connection; the client's own limit governs a client→silo call), falling back to Orleans' own default. Terminal ErrorType.Rejected — the size is a property of the message, not of the attempt. No bound is raised and nothing catches the OutOfMemoryException.
  • SendDeliveryFailure strips an undeliverable echo via MessageSizeGuard.WithoutOversizedPayload — the protection RoutingGrain.PostFailure has had since MemoryPooledCache rejects oversized messages on Orleans in-memory queue, causing stream delivery failures #1890 and this site did not. On this leg the omission is worse than a lost report: echoing the payload re-runs the identical 3× transcode, so the refusal becomes a second allocation failure.
  • MessageSizeGuard moves to MeshWeaver.Messaging.Hub so both routers share ONE guard. Hosting.Orleans references Connection.Orleans, so it could not stay put without being copy-pasted into the other — the shape that let Flake: OrleansMeshTests.HubWorksAfterDisposal — shard-0 Orleans suite is red across unrelated PRs #2346 outlive two fixes.

Why the guard is public, not internal + InternalsVisibleTo

Because I tried the InternalsVisibleTo route and it broke the build in a way worth recording. An assembly-wide InternalsVisibleTo on MeshWeaver.Messaging.Hub exposes the internal IMessageHub.Observe(object, Func<PostOptions, PostOptions>) as an instance method, which then outranks the public generic Observe<TResponse> extension in overload resolution — so response.Message silently becomes object. Granting it to MeshWeaver.Hosting.Orleans.Test reddened 20+ untouched files with CS1061 'object' does not contain a definition for 'Success'. MessageStormBreaker already documents exactly this hazard and resolves it the same way.

Test that would have failed before

OversizedRouterDispatchRefusedTest drives the real OrleansRoutingService with a null IGrainFactory as the placement probe — the same technique OrleansRoutingShutdownClassificationTest uses — so "refused before the grain call" and "reached the grain call" are both directly assertable rather than inferred from silence. Against origin/main the oversized delivery reaches placement and throws. The fourth test is the control: a delivery that fits still reaches placement, so the guard cannot be indistinguishable from "refuses everything".

🚨 What this does NOT claim — why #2885 stays open

The incident's stack carries no payload size. The transcode peaks at ~3× the payload while the bound is the frame limit, so a body comfortably under the limit can still exhaust a pressured pod. It cannot be shown that this bound would have refused that delivery — only that the leg is now bounded and attributable, where before it was neither. The residual is producer-side (an import that batches instead of building one delivery whole), which is where the issue's own last measurement scoped it.


#2896 — the issue's premise is falsified by its own log line (no code change)

The issue says the owning hub "produced no verdict for the patch" and points at [UpdateRemote] VERDICT_TIMEOUT. The logged exception says otherwise:

MeshWeaver.Mesh.MeshNodeStreamException: MeshNode Unknown at '…/StructureTheMessBook':
  TimeoutException: The operation has timed out.

MeshNodeStreamException renders $"MeshNode {error.Code} at '{error.Path}': {error.Message}", so this carries Code = Unknown and Message = "TimeoutException: The operation has timed out.".

  • The writer's actual no-verdict path stamps OwnerUnreachable and writes "The owner of '…' returned no verdict for this update within Ns… Request trail: …". Neither the code nor that sentence appears.
  • The only site in src/ that mints a MeshNodeError shaped {TypeName}: {message} is the owner's ClassifyPatchException default arm (_ => (MeshNodeErrorCode.Unknown, ex.GetType().Name)).

So the owner answered — with a NACK carrying its own internal TimeoutException from one of its two ack-chain bounds (the 20 s commit-echo watcher or the 10 s IPostCommitFlush durable write). That means:

PR Failure mode it closes Accounts for #2896?
#3020 base read completes empty ⇒ no terminal at all, no patch posted No — mutually exclusive. An owner-minted error requires the patch to have been posted.
#3009 mid-recycle owner ⇒ the base read never arrives No — that path fails as a bare TimeoutException("Update aborted: no initial state arrived…"), never a MeshNodeStreamException.
#3018 oversized grain-routed deliveries No — a text-area save is small.

Conserved as Doc/Architecture/ReadingAWriteVerdict: which verdict shape means which hub, how the elapsed time names which owner bound fired, and the fact that in the flush case the write is already committed when the NACK is sent — so the caller is told a landed write failed.

#2896 stays open: the recorded occurrence is real and unexplained at the level of why the owner's commit echo or storage flush stalled, and that is not derivable from the code. The decisive next sample is named on the issue.


Verification

Check Result
dotnet build MeshWeaver.slnx -c Release -warnaserror 61/61 projects, 0 Warning(s), 0 Error(s)
test/MeshWeaver.Hosting.Orleans.Test (full) 226 passed, 0 failed
test/MeshWeaver.Messaging.Hub.Test (full) 341 passed, 0 failed
test/MeshWeaver.Documentation.Test (full, incl. DocumentationLinkIntegrityTest) 172 passed, 0 failed
scripts/check-type-forwards.py --base origin/main OK — no unguarded public-type move
scripts/check-record-signatures.py --base origin/main OK — no binary-breaking changes

Refs #2885, refs #2896

🤖 Generated with Claude Code

https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa

)

#3018 refuses an oversized delivery on the two forward grain legs and the
memory-stream leg, all inside RoutingGrain. #2885 died one hop EARLIER, on the
leg that reaches RoutingGrain at all: OrleansRoutingService.DispatchObservable
calls IRoutingGrain.RouteMessage(delivery), and Orleans serialises that argument
with the mesh's own System.Text.Json options. All three guarded sites are
strictly downstream, so none of them ever executed.

Reading the incident's frame order is what overturned the prior conclusion. The
page said "no guard placed at the router can fix this" on the reasoning that
packaging itself had exhausted memory. But MessageDelivery.Package serialises
only the PAYLOAD (JsonSerializer.Serialize(message) -> new RawJson(...)); it
never invokes MessageDeliveryConverter, which converts the ENVELOPE. The stack

    MessageDeliveryConverter.Write
      -> ObjectDefaultConverter.OnTryWrite
      -> JsonPropertyInfo.GetMemberAndWriteJson
      -> Utf8JsonWriter.TranscodeAndWriteRawValue
      -> SharedArrayPool.Rent -> OutOfMemoryException

therefore proves the payload was ALREADY a materialised RawJson string when the
failing allocation was attempted: packaging had finished, and what blew up was
re-serialising the packaged envelope for transport. MessageSizeGuard.IsOversized
needs nothing but RawJson.Content.Length — the fast path is 3 * Length < limit,
O(1) and allocation-free — so it can measure a payload the very next statement
cannot transcode. The bound was never the problem; its PLACEMENT was.

- OrleansRoutingService.DeliverMessage now refuses at this leg before the
  dispatch, against the live IOptions<ClientMessagingOptions>.MaxMessageBodySize
  (this assembly is the client connection, and the client's own limit governs a
  client->silo call), falling back to Orleans' own default. The refusal is
  terminal (ErrorType.Rejected): the size is a property of the message, not of
  the attempt. No bound is raised and nothing catches the OutOfMemoryException.

- SendDeliveryFailure now strips an undeliverable echo via
  MessageSizeGuard.WithoutOversizedPayload — the protection RoutingGrain.PostFailure
  has had since #1890 and this site did not. On this leg the omission is worse
  than a lost report: echoing the payload re-runs the identical 3x transcode, so
  the refusal becomes a SECOND allocation failure.

- MessageSizeGuard moves to MeshWeaver.Messaging.Hub so both routers share ONE
  guard. Hosting.Orleans references Connection.Orleans, so it could not stay put
  without being copy-pasted into the other — the shape that let #2346 outlive two
  fixes. It is public rather than internal + InternalsVisibleTo for the reason
  MessageStormBreaker already documents: an assembly-wide InternalsVisibleTo on
  Messaging.Hub exposes the internal IMessageHub.Observe(object, ...) as an
  instance method, which outranks the public generic Observe<TResponse> extension
  and silently turns every response.Message in the granted assembly into object.
  Measured: granting it to MeshWeaver.Hosting.Orleans.Test reddened 20+ untouched
  files with CS1061.

OversizedRouterDispatchRefusedTest drives the real OrleansRoutingService with a
null IGrainFactory as the placement probe, so "refused before the grain call" and
"reached the grain call" are both directly assertable rather than inferred from
silence. Against origin/main the oversized delivery reaches placement and throws.
A delivery that fits still reaches placement — the control that keeps the guard
from being indistinguishable from "refuses everything".

Deliberately NOT closing #2885. The incident's stack carries no payload size, so
it cannot be shown this bound would have refused THAT delivery: the transcode
peaks at ~3x the payload while the bound is the frame limit, so a body under the
limit can still exhaust a pressured pod. The residual is producer-side — an
import that batches instead of building one delivery whole — which is where the
issue's own last measurement scoped it.

Docs: Doc/Architecture/OversizedDeliveryRefusal gains the fourth site and has its
"where the bound cannot reach" section corrected; Doc/Architecture/ReadingAWriteVerdict
is new and conserves the #2896 measurement (which verdict shape means which hub).

Refs #2885, refs #2896

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There’s a likely configuration/behavior mismatch in how the router reads the Orleans max-body-size limit on silo hosts, and the new test introduces TaskCompletionSource/WaitAsync gating that should be expressed as an observable signal instead.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the Orleans routing path against oversized deliveries by adding a producer-side size refusal before the router’s own IRoutingGrain.RouteMessage(delivery) call (the “fourth transport leg” implicated in #2885), and ensures the resulting DeliveryFailure does not echo an undeliverable payload. It also consolidates the size-guard utility into MeshWeaver.Messaging.Hub so both routing layers share a single implementation, and conserves the investigation findings in docs.

Changes:

  • Add an early oversized-payload refusal in OrleansRoutingService.DeliverMessage (before the grain call) and strip oversized payloads from delivery-failure echoes.
  • Move/retarget MessageSizeGuard into MeshWeaver.Messaging.Serialization as a shared public utility and update call sites.
  • Add/adjust regression tests and documentation (new “What’s New” entry and new architecture page on interpreting write verdicts).
File summaries
File Description
test/MeshWeaver.Hosting.Orleans.Test/OversizedStreamMessageRefusedTest.cs Imports updated to reference the new MeshWeaver.Messaging.Serialization namespace.
test/MeshWeaver.Hosting.Orleans.Test/OversizedRouterDispatchRefusedTest.cs New regression tests for refusing oversized deliveries before the router’s grain call and for stripping echoed payloads.
test/MeshWeaver.Hosting.Orleans.Test/OversizedGrainDeliveryRefusedTest.cs Imports updated to reference the new MeshWeaver.Messaging.Serialization namespace.
src/MeshWeaver.Messaging.Hub/Serialization/MessageSizeGuard.cs Moves the guard to MeshWeaver.Messaging.Serialization, makes it public, and adds router-leg refusal wording.
src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs Updates references to the moved MessageSizeGuard namespace.
src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs Adds the pre-grain-call size refusal and strips oversized payloads in SendDeliveryFailure.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-large-imports-no-longer-take-a-portal-down.md Adds a user-facing “What’s New” entry describing the hardening.
src/MeshWeaver.Documentation/Data/Architecture/ReadingAWriteVerdict.md Adds an architecture note for interpreting cross-hub write verdict shapes (#2896 investigation).
src/MeshWeaver.Documentation/Data/Architecture/OversizedDeliveryRefusal.md Updates the architecture page to include the newly guarded router leg and corrected frame-order reasoning.
src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md Adds a cross-reference to the new “Reading a Write Verdict” page.
Review details

Suppressed comments (2)

test/MeshWeaver.Hosting.Orleans.Test/OversizedRouterDispatchRefusedTest.cs:192

  • Same as above: prefer awaiting the observable signal with Rx Timeout rather than Task.WaitAsync.
        var failure = await nack.Task.WaitAsync(TimeSpan.FromSeconds(10));

test/MeshWeaver.Hosting.Orleans.Test/OversizedRouterDispatchRefusedTest.cs:160

  • After switching the NACK signal away from TaskCompletionSource, avoid Task.WaitAsync here as well; await the observable directly with a bounded Rx Timeout so the test doesn't introduce a Task-based scheduler hop.
        var failure = await nack.Task.WaitAsync(TimeSpan.FromSeconds(10));
  • Files reviewed: 10/10 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    3 files  ±0      3 suites  ±0   1m 14s ⏱️ -5s
1 226 tests ±0  1 034 ✅ ±0  192 💤 ±0  0 ❌ ±0 
1 227 runs  ±0  1 035 ✅ ±0  192 💤 ±0  0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

231 tests  +4   231 ✅ +4   2m 37s ⏱️ -4s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

599 tests  ±0   599 ✅ ±0   49s ⏱️ +11s
  2 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

420 tests  ±0   420 ✅ ±0   1m 35s ⏱️ -7s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 744 tests  ±0   1 744 ✅ ±0   1m 54s ⏱️ +6s
    3 suites ±0       0 💤 ±0 
    3 files   ±0       0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

535 tests  ±0   533 ✅ ±0   1m 31s ⏱️ +3s
  4 suites ±0     2 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   14 files  ±0     14 suites  ±0   9m 43s ⏱️ +4s
4 755 tests +4  4 561 ✅ +4  194 💤 ±0  0 ❌ ±0 
4 756 runs  +4  4 562 ✅ +4  194 💤 ±0  0 ❌ ±0 

Results for commit a40ed93. ± Comparison against base commit f4861d8.

♻️ This comment has been updated with latest results.

Roland Bürgi and others added 2 commits September 2, 2026 03:04
Both from the automatic review on #3032, and both real.

OrleansRoutingService is registered on SILO hosts as well as clients
(OrleansServerRegistryExtensions), and the two bounds are configured
separately: a silo's outbound grain call is bounded by SiloMessagingOptions, a
client's by ClientMessagingOptions. Reading only the client option would
UNDER-refuse where the silo bound is smaller — the oversized frame still tears
the connection down, which is the exact failure this guard exists to prevent —
and OVER-refuse where it is larger. Now keyed on CanHostGrains, which is
Orleans' own ILocalSiloDetails answer to "is this a silo", already resolved two
lines above. Each option stays optional with a cross-fallback, so a bare mesh in
a unit test still lands on Orleans' default rather than on "unbounded".

The test used a TaskCompletionSource as a cross-thread gate. That is a
hand-woven async gate whose continuation can resume INLINE on the hub thread
that completed it, inside the action block — forbidden by AGENTS.md in test/ as
well as src/. Replaced with an AsyncSubject<DeliveryFailure> the handler
completes, awaited through the repo's reactive assertion, so the wait is bounded
by the assertion's own timeout and its failure names what did not arrive.

Full MeshWeaver.Hosting.Orleans.Test: 226/226. Release -warnaserror clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
# Conflicts:
#	src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs
@rbuergi

rbuergi commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Merged origin/main in to clear DIRTY. Recording the resolution, because this one was a code conflict where a careless take-one-side would have silently reverted a fix that landed an hour ago.

Both sides added a different statement to the same constructor region — neither replaces the other:

side added
this branch the GrainBodyLimitBytes resolution (silo-vs-client MaxMessageBodySize)
origin/main (#3034) membershipFeed = serviceProvider.GetService<IClusterMembershipFeed>() — the feed that turns the pod-hub claim from a one-shot assertion into a re-asserted one

Taking --ours would have dropped #3034's membership feed and quietly re-broken #2938/#2915, which had just been closed. Kept both, main's first.

Verified afterwards rather than assumed:

$ grep -c IClusterMembershipFeed OrleansRoutingService.cs     → 2   (#3034 intact)
$ grep -c SiloMessagingOptions   OrleansRoutingService.cs     → 3   (this branch intact)
$ git diff origin/main --stat                                 → only this branch's own additions

Re-verified on the merged tree: dotnet build -c Release -warnaserror clean, and the full MeshWeaver.Hosting.Orleans.Test project — not a filter — 231 passed, 0 failed (up from 226 pre-merge, i.e. #3034's new pins are running rather than silently absent).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants