Skip to content

fix(mesh): a subscriber's own release is not announced back; a same-silo grain call shares the immutable delivery envelope - #5635

Merged
meshweaver-cloud[bot] merged 2 commits into
mainfrom
fix/stream-teardown-family
Sep 24, 2026
Merged

meshweaver-cloud[bot] merged 2 commits into
mainfrom
fix/stream-teardown-family

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What was wrong

1. The owner announced a subscriber's own release back to it (#5532)

JsonSynchronizationStream posts StreamEndedEvent from the owner-side stream's disposal (#2191). That event is meant for an end the subscriber did not ask for. The most common disposal is the subscriber's own UnsubscribeRequest, and that one was announced too. The subscriber's sync/ hub had already disposed itself before the request left, so each announcement crossed the subscriber's mesh hub and was dropped there ([CanBeIgnored]).

One wasted message per release was invisible. When a subscriber releases N mirrors at once, for example a cache hub going down, the owners send N StreamEndedEvents back through one router turn loop. That is the shape of #5532: 454 aggregate-watermark trips in 36 s on mesh/YxuIePH430S3wXXs-HRa8A, shedding StreamEndedEvent. There was a second problem too. A stream id is reused across a re-subscribe, so a late echo could reach the next incarnation and read as an owner-side end.

Fix: the owner's sync/ hub records that an UnsubscribeRequest ended it (SynchronizationStream.MarkSubscriberEnded, set before hub.Dispose()). The announcement skips that end. Owner-side ends are still announced: an idle release, or EvictClientSubscriptions.

2. Every same-silo grain call copied the delivery through JSON (#4824)

ConfigureMeshWeaver registered JsonCodec as Orleans' copier for every type (AddJsonSerializer(_ => true, _ => true, …), where the second predicate is isCopyable). So every local RouteMessage / DeliverMessage / Deliver serialised its delivery and parsed it back, for the arguments and again for the result. That copy is where the production OOM stacks sit: ObjectPolymorphicConverter.Write under the Orleans deep-copy (#4824), and PooledResponseCopier → JsonCodec.DeepCopy (#3045).

The copy isolates nothing:

  • MessageDelivery<T> is a record with only init members, an ImmutableDictionary of properties and an ImmutableList routing path.
  • The payload that reaches a grain call is RawJson, because the router is only reached through delivery.Package(…).
  • A non-Orleans mesh already passes the same instances between hubs.

Fix: MessageDeliveryCopier (an IGeneralizedCopier) claims exactly the packaged shape, MessageDelivery<RawJson>, and returns the input. JsonCodec's isCopyable excludes that type. An un-packaged delivery, whose typed payload may be mutable, keeps the isolating copy. Cross-silo calls still serialise through JsonCodec; that path is unchanged.

This answers the #4824 question "is DeepCopy of large payloads avoidable?" for the local hop: yes, and it is now gone. The $type-first question for the wire path stays open. The doc states what I did and did not establish there.

Evidence

  • SubscriberReleaseIsNotAnnouncedBackTest, counting on the owner's post pipeline:
    • A subscriber release produces 0 announcements. Negative control (fix disabled with false &&): fails with "expected 0 … but found 1".
    • The control case, an owner-side end, still produces exactly 1.
  • LocalGrainCallSharesTheEnvelopeTest, through the silo's own DeepCopier:
    • A delivery comes back as the same instance. Negative control (old registration restored): fails with "expected true … but found False".
    • Controls: an un-packaged delivery and a mutable JsonObject are still copied.
  • Full suites, local:
    • MeshWeaver.Data.Test: 547/547
    • MeshWeaver.Layout.Test: 521/521
    • MeshWeaver.Graph.Test: 1979/1979
    • MeshWeaver.Hosting.Orleans.Test: 336/336
  • dotnet build -c Release -warnaserror gives 0 Warning(s) / 0 Error(s) for:
    • MeshWeaver.Data
    • MeshWeaver.Connection.Orleans
    • MeshWeaver.Documentation
    • MeshWeaver.Data.Test
    • MeshWeaver.Hosting.Orleans.Test

Not established

Docs: LiveMirrorsAndTheChangeFeed (step 5 note) and OversizedDeliveryRefusal (new section "A same-silo call does not copy the envelope at all").

Deploy / recycle

Framework code. It takes effect with the image roll. No address needs a manual recycle.

Refs #5532
Refs #4824

Pairs-with: none — nothing public removed (internal members and one internal type added)

🤖 Generated with Claude Code

…ilo grain call shares the immutable delivery envelope

- The owner's StreamEndedEvent no longer answers an end the subscriber asked for
  (UnsubscribeRequest). Its sync hub is already gone, so the announcement only crossed the
  subscriber's mesh hub to be dropped; a bulk release (a cache hub releasing thousands of
  mirrors) put thousands on one router turn loop and tripped the aggregate watermark (#5532).
  Owner-side ends (idle release, EvictClientSubscriptions) are still announced.
- Orleans deep-copied every same-silo grain call's IMessageDelivery through JsonCodec
  (serialise + parse, arguments and result). The envelope is immutable and its crossing payload
  is RawJson, so MessageDeliveryCopier hands the same instance across; JsonCodec no longer
  claims copying for delivery types. Cross-silo serialisation is unchanged (#4824).

Refs #5532
Refs #4824

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 24, 2026 02:11

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.

Copilot review overview

🟡 Changes recommended

Restrict the copier scope and correct the eviction disposal path and related documentation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Low severity

Open (2)
What changed in this PR

This PR suppresses redundant stream-end notifications and avoids same-silo JSON deep copies of delivery envelopes.

Changes:

  • Tracks subscriber-requested stream termination.
  • Adds and registers a delivery-envelope copier.
  • Adds regression tests and documentation.
File Summary
test/​MeshWeaver.Hosting.Orleans.Test/​LocalGrainCallSharesTheEnvelopeTest.cs Tests same-silo envelope sharing.
test/​MeshWeaver.Data.Test/​SubscriberReleaseIsNotAnnouncedBackTest.cs Tests release announcement behavior.
src/​MeshWeaver.Documentation/​Data/​Architecture/​OversizedDeliveryRefusal.md Documents local-call copying behavior.
src/​MeshWeaver.Documentation/​Data/​Architecture/​LiveMirrorsAndTheChangeFeed.md Documents stream-release behavior; causal wording requires correction.
src/​MeshWeaver.Data/​Serialization/​SynchronizationStream.cs Tracks subscriber-requested termination; eviction disposal remains unresolved.
src/​MeshWeaver.Data/​Serialization/​JsonSynchronizationStream.cs Suppresses redundant end announcements.
src/​MeshWeaver.Connection.Orleans/​OrleansConnectionExtensions.cs Registers the custom copier and excludes envelopes from JSON copying.
src/​MeshWeaver.Connection.Orleans/​MessageDeliveryCopier.cs Adds envelope sharing, but its broad scope can violate deep-copy isolation.

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

/// <summary>Whether <paramref name="type"/> is a delivery envelope this copier owns — also the
/// predicate that takes those types away from <c>JsonCodec</c>'s copying, so the two can never
/// both claim one type.</summary>
internal static bool Covers(Type type) => typeof(IMessageDelivery).IsAssignableFrom(type);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and fixed in eb1bdb3. Covers is now exactly type == typeof(MessageDelivery<RawJson>), the packaged shape the immutability argument covers. A delivery still carrying a typed CLR message keeps JsonCodec's isolating copy. The router only hands packaged deliveries to grains, so the hot path and every large payload still take the reference. New control An_unpackaged_delivery_is_still_copied asserts the narrowing through the silo's own DeepCopier. Orleans suite: 336/336.

Comment on lines +336 to +340
/// <summary>
/// 🚨 Set when this server-side stream ends because its SUBSCRIBER is gone — it asked for the
/// end itself (an <see cref="UnsubscribeRequest"/> reached this stream's sync hub), or the
/// router proved its address unserved (<c>Workspace.EvictClientSubscriptions</c>).
///

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct: nothing on the eviction path sets it, and that is intended. The summary now names its one setter, the UnsubscribeRequest handler, and says explicitly that owner-side ends (idle release, EvictClientSubscriptions) are still announced. Fixed in eb1bdb3.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

  1 files    1 suites   2m 57s ⏱️
332 tests 332 ✅ 0 💤 0 ❌
336 runs  336 ✅ 0 💤 0 ❌

Results for commit eb1bdb3.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

547 tests   547 ✅  1m 8s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit eb1bdb3.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

429 tests   429 ✅  59s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit eb1bdb3.

♻️ This comment has been updated with latest results.

…y<RawJson>; SubscriberEnded doc names its one setter

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

    2 files      2 suites   6m 12s ⏱️
2 604 tests 2 604 ✅ 0 💤 0 ❌
2 605 runs  2 605 ✅ 0 💤 0 ❌

Results for commit eb1bdb3.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

1 602 tests   1 602 ✅  8m 59s ⏱️
    2 suites      0 💤
    2 files        0 ❌

Results for commit eb1bdb3.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    5 files      5 suites   15m 1s ⏱️
3 413 tests 3 413 ✅ 0 💤 0 ❌
3 417 runs  3 417 ✅ 0 💤 0 ❌

Results for commit eb1bdb3.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

   13 files     13 suites   35m 18s ⏱️
8 927 tests 8 927 ✅ 0 💤 0 ❌
8 936 runs  8 936 ✅ 0 💤 0 ❌

Results for commit eb1bdb3.

@meshweaver-cloud
meshweaver-cloud Bot added this pull request to the merge queue Sep 24, 2026
Merged via the queue into main with commit bf5ac85 Sep 24, 2026
45 checks passed
@rbuergi
rbuergi deleted the fix/stream-teardown-family branch October 10, 2026 13:11
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