Skip to content

Share the anonymous intermediate reduce instead of minting a hub per subscribe (#1324) - #1425

Merged
rbuergi merged 1 commit into
mainfrom
fix/1324-residual-retainers
Aug 13, 2026
Merged

rbuergi merged 1 commit into
mainfrom
fix/1324-residual-retainers

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Part of #1324 — closes retainer #2, the one #1415 named as "a NEW finding that replaces this issue's guess about compile-state". Does not close the issue: the per-compile activity node hubs remain, and what is now known about them is recorded in the test.

The retainer

MeshDataSource's own-node AddWorkspaceReferenceStream<MeshNode> factory builds

var collectionStream = primary?.Reduce<InstanceCollection>(new CollectionReference(nameof(MeshNode)));
return collectionStream?.Reduce((WorkspaceReference<MeshNode>)ownPathReference, configuration);

WorkspaceStreams.CreateReducedStream registers every child for disposal on its parent. Here the parent is primary, the data source's EntityStore stream, whose lifetime is the hub's — so each call permanently added a hosted sync/{id} sub-hub with its own Autofac scope, TypeRegistry and JsonSerializerOptions (~140 KB). The factory runs uncached on every call that passes a configuration, i.e. once per inbound SubscribeRequest, so an unsubscribe reaped the subscription's own stream and left the nameless intermediate behind forever.

That is exactly #1345's defect — an unmemoized Workspace.GetStream — one layer down at stream.Reduce.

The fix, and why it is opt-in

ISynchronizationStream.ReduceShared — a memoized reduce, keyed by reference, cached on the parent, with the same liveness re-check and faulted-Lazy eviction as Workspace.GetStream.

Opt-in rather than a change to Reduce, and that is load-bearing. The rule is ownership, not depth:

the stream is… use why
an intermediate nobody owns or disposes ReduceShared one per (parent, reference), dies with the parent
handed to a caller who will Dispose() it Reduce sharing would let one holder's teardown kill another's live stream
caller-specific (configuration with a client id / subscriber) Reduce not shareable by construction

Making every no-config Reduce shared was the obvious move and it is wrong: the Blazor LayoutAreaView explicitly Dispose()s the dialog and progress streams it reduces off its area stream (LayoutAreaView.razor.cs:148-149, :174-175), and its area stream already comes from the memoized Workspace.GetStream — so two views on the same layout area would share those reduces and the first to navigate away would kill the second's binding. Recording it here so nobody re-derives it.

The trap this hit, found by the suite and worth keeping

A cached child does not die with its parent. CreateReducedStream builds the child's sync/{id} sub-hub under stream.Host — the data source's hub — so it is the parent's sibling, not its descendant, and it stays alive after the parent is disposed. The first cut checked only the child's liveness (copying Workspace.GetStream), so once a parent stream was disposed the cache happily kept serving a mirror of a dead source: reads bound to a stream that would never emit and never complete, and the reader got neither an answer nor the NACK DataExtensions.HandleGetDataRequest's disposal arm owes it. It hung for its whole budget.

SilentReadNackTest caught it — deterministically, both arms, in isolation — because disposing the owner's data-source stream is exactly how that test manufactures an unanswerable read. ReduceShared now bypasses the cache entirely when the parent is disposed, falling through to a plain Reduce so the disposed-parent path behaves exactly as it always did. Documented in WorkspaceReferences.md next to the ownership table.

Measured — NodeTypeRecompileAlcLeakTest, Release, three recompiles of config => config

before after
hubs / recompile 12.7 6.1 (5.3 / 7.0 / 6.3 / 6.0 / 6.0 over five runs)
managed / recompile 7 MB 3–5 MB
_Activity/compile-state >> sync/* +13 0
the NodeType's own hub >> sync/* +3 0
cache/<mesh> >> sync/* +4 +3
_Activity/compile-<ts> >> sync/* +6 / +6 / +5 +3…5 apiece

compile-state — the largest single bucket after #1415 — goes to zero, which confirms #1415's attribution quantitatively: it really was intermediates, not subscriptions.

Cumulatively on this repro: 20 MB / 63 hubs → 3–5 MB / 6.2 hubs per recompile across #1345, #1415 and this.

Ratchet tightened: 15 → 9 hubs and 12 → 8 MB per recompile (measured max 7.0 hubs / 5 MB).

What still retains — recorded in the test, not guessed

The residual is now essentially all the per-compile _Activity/compile-<ts> node hubs. This is a node-lifecycle question, and the useful new fact is that two reclamation mechanisms already exist and both are time-based, so this test — which measures seconds — cannot see either:

  • the shared mesh-node cache's idle sweep (zero subscribers and 10 min untouched, MeshNodeStreamCacheOptions.ReadStreamIdleExpiration), and
  • KernelContainer.DisposeOnTimeout, which disposes an idle node hub after 15 min — but its timer is reset by every message, and a surviving mirror heart-beats the owner every 45 s (SyncStreamOptions.HeartbeatInterval), which also holds the Orleans grain up via TryDelayDeactivation.

So whether the residual is a permanent leak or a ~25-minute bounded retention turns on whether the activity's mirror is released once the compile ends — nothing in the compile path does that today (Complete(...) is the last touch). That needs a longer-horizon measurement, and the test says so, including do not "fix" it by shortening a timer.

Verification

  • dotnet build -c Release -warnaserror clean (0 errors) for MeshWeaver.Data, MeshWeaver.Graph, MeshWeaver.Hosting.Monolith.Test, MeshWeaver.Documentation.Test; full --no-incremental rebuild of the Monolith test graph also clean.
  • NodeTypeRecompileAlcLeakTest 2/2, five measurement runs.
  • MeshWeaver.Data.Test 344/344 · MeshWeaver.Documentation.Test 20/20 · MeshWeaver.Serialization.Test 14/14 · MeshWeaver.Messaging.Hub.Test 218/218.
  • MeshWeaver.Hosting.Monolith.Test full suite green (646/649, 3 skipped) — the same suite on unmodified origin/main was run as the control, which is what established the two SilentReadNackTest failures above as mine rather than pre-existing.

What's New entry included (Category: Fix).

🤖 Generated with Claude Code

… per subscribe (#1324)

`MeshDataSource`'s own-node `AddWorkspaceReferenceStream<MeshNode>` factory built
`primary.Reduce<InstanceCollection>(CollectionReference("MeshNode"))` and then reduced THAT
to the node. `WorkspaceStreams.CreateReducedStream` registers every child for disposal on its
PARENT — here the data source's primary EntityStore stream, whose lifetime is the hub's — so
each call permanently added a hosted `sync/{id}` sub-hub (its own Autofac scope, TypeRegistry
and JsonSerializerOptions, ~140 KB). The factory runs uncached on every call that passes a
`configuration`, i.e. once per inbound SubscribeRequest, so an unsubscribe reaped the
subscription's own stream and left the nameless intermediate behind forever.

Same defect as #1345 (an unmemoized `Workspace.GetStream`) one layer down at `stream.Reduce`.

The cure is `ISynchronizationStream.ReduceShared` — an opt-in memoized reduce, keyed by
reference, for an intermediate that no caller owns. Opt-in rather than a change to `Reduce`,
because a reduced stream a caller DOES own and disposes (the Blazor LayoutAreaView's dialog and
progress reduces) must stay private: sharing one would let one holder's teardown kill another
holder's live stream. Same split `Workspace.GetStream` already draws one layer up — memoize
only when there is no caller-specific configuration.

🚨 A cached child does NOT die with its parent, and the first cut got that wrong.
CreateReducedStream builds the child's `sync/{id}` hub under `stream.Host`, so it is the
parent's SIBLING, not its descendant, and survives the parent's disposal. Checking only the
child's liveness therefore kept serving a mirror of a dead source: reads bound to a stream that
never emits and never completes, so the reader got neither an answer nor the NACK
`HandleGetDataRequest`'s disposal arm owes it, and hung for its whole budget. SilentReadNackTest
caught it deterministically (both arms, in isolation) — disposing the owner's data-source stream
is exactly how it manufactures an unanswerable read. ReduceShared now bypasses the cache when
the parent is disposed, so that path behaves exactly as it always did.

Measured on NodeTypeRecompileAlcLeakTest, Release, three recompiles of `config => config`:

    hubs/recompile   12.7  ->  6.2   (5.3 / 7.0 / 6.3 / 6.0 / 6.0 over five runs)
    managed/recompile   7 MB -> 3-5 MB

    _Activity/compile-state  >> sync/*   +13  ->   0
    the NodeType's own hub   >> sync/*    +3  ->   0
    cache/<mesh>             >> sync/*    +4  ->  +3
    _Activity/compile-<ts>   >> sync/*  +6/+6/+5 -> +3..5 apiece

Cumulatively across #1345, #1415 and this: 20 MB / 63 hubs -> 3-5 MB / 6.2 hubs per recompile.
Ratchet tightened 15 -> 9 hubs and 12 -> 8 MB per recompile.

The residual is now essentially all the per-compile `_Activity/compile-<ts>` node hubs, which
is a node-lifecycle question rather than a stream one; the test comment records what is known
about it, including that both existing reclamation mechanisms are time-based (10 min cache idle
sweep, 15 min KernelContainer.DisposeOnTimeout) and therefore invisible to a seconds-long test.

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

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.

Pull request overview

This PR addresses a retained-hub/memory-growth issue in the workspace reduce chain by introducing an opt-in shared reduce for “ownerless” intermediate streams, then applying it to the MeshDataSource own-node path and tightening the leak-regression guardrails/tests. It fits into the data/streaming layer by reducing unnecessary sync/{id} hosted sub-hubs created per subscription.

Changes:

  • Add ISynchronizationStream.ReduceShared(...) and implement a memoized, parent-owned shared reduce cache in SynchronizationStream.
  • Switch MeshDataSource to use ReduceShared for the intermediate InstanceCollection stream to avoid minting a hub per inbound SubscribeRequest.
  • Update NodeTypeRecompileAlcLeakTest bounds/documentation and add supporting docs + a “What’s New” entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/MeshWeaver.Hosting.Monolith.Test/NodeTypeRecompileAlcLeakTest.cs Tightens hub/byte ceilings and updates commentary to reflect the new retainer attribution and fix.
src/MeshWeaver.Graph/MeshDataSource.cs Uses ReduceShared for the intermediate collection reduce in the own-node stream factory.
src/MeshWeaver.Data/Serialization/SynchronizationStream.cs Implements a per-parent shared reduce cache (ReduceShared) with liveness checks and faulted-entry eviction.
src/MeshWeaver.Data/ISynchronizationStream.cs Adds the ReduceShared API with guidance on safe/unsafe usage based on ownership.
src/MeshWeaver.Documentation/Data/Architecture/WorkspaceReferences.md Documents the “intermediate reduce must be shared” rule and the parent-liveness caching trap.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-rebuilding-in-portal-code-costs-half-of-what-it-did.md Adds a changelog entry describing the user-visible impact and measured improvements.

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

Comment on lines +126 to +127
if (isDisposed)
return Reduce(reference, x => x);
@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

723 tests  ±0   723 ✅ ±0   10m 30s ⏱️ +19s
 10 suites ±0     0 💤 ±0 
 10 files   ±0     0 ❌ ±0 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

   11 files  ±0     11 suites  ±0   5m 20s ⏱️ +4s
1 767 tests ±0  1 662 ✅ ±0  105 💤 ±0  0 ❌ ±0 
2 075 runs  +1  1 970 ✅ +1  105 💤 ±0  0 ❌ ±0 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 908 tests  ±0   1 716 ✅ ±0   6m 16s ⏱️ +4s
   11 suites ±0     192 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 975 tests  ±0   1 967 ✅ +1   8m 33s ⏱️ -16s
   11 suites ±0       8 💤 ±0 
   11 files   ±0       0 ❌  - 1 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

2 419 tests  ±0   2 418 ✅ ±0   6m 19s ⏱️ -14s
   12 suites ±0       1 💤 ±0 
   12 files   ±0       0 ❌ ±0 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 491 tests  ±0   1 491 ✅ ±0   7m 12s ⏱️ -22s
   11 suites ±0       0 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

    66 files  ±0      66 suites  ±0   44m 13s ⏱️ -25s
10 283 tests ±0   9 977 ✅ +1  306 💤 ±0  0 ❌  - 1 
10 591 runs  +1  10 285 ✅ +2  306 💤 ±0  0 ❌  - 1 

Results for commit 0f07872. ± Comparison against base commit 85bc832.

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