Repository navigation
chore: the per-compile _Activity hub retention is BOUNDED at 15 min, measured (#1324) - #1461
Merged
Merged
Conversation
…measured (#1324) #1324 asks whether the residual is a leak or a bounded retention. It is bounded, and the ceiling is KernelContainer's 15-minute idle disconnect. Measured unscaled at production timer values (15 min window, real 45 s heartbeat): the per-compile activity hubs sit flat at 15 through t+885s, drop to 5 at t+900s and reach 0 at t+915s — 13 hubs reclaimed in 15.25 min, mesh total 49 -> 32. So the ~6 hubs / 3-5 MB per recompile that NodeTypeRecompileAlcLeakTest counts is a TRANSIENT it is too short-lived to watch expire, not a leak. Why it is bounded: an Activity node hub carries an idle reaper of its own, monolith included — ActivityNodeType -> AddKernelSubHubHandlers() -> KernelContainer.DisposeOnTimeout, registered by KernelNodeType.AddKernel() which AddGraph() includes. The timer is one-shot and re-armed by every inbound message, which is exactly why the 45 s mirror heartbeat kept it at bay until #1435 released the mirror on the terminal write. The CI guard scales the window AND the heartbeat together, at production's 1:20 ratio. That pairing is load-bearing: shortening only the window would put the 45 s heartbeat outside it, so the timer would fire between two heartbeats and the test would pass while production reclaimed nothing. That is the same shape as NodeTypeAssemblyLeakTest — deleted earlier in this issue for asserting collection after a mesh disposal a portal never performs — and two "cannot fail" guards on one issue is a pattern worth naming. KernelHubOptions.IdleDisconnectTimeout defaults to the existing 15 minutes and changes no production behaviour; it exists so the property is assertable in a test budget. A test asserts that default stays 15 minutes, because #1324 prohibits shortening it and an option added for assertability is one refactor away from being dialled down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR establishes (and guards in CI) that per-compile _Activity/compile-<ts> hubs are retained transiently but reclaimed within a bounded idle window (15 minutes in production), by adding a scale-model retention test and making the kernel hub idle timeout explicitly configurable (default unchanged).
Changes:
- Introduces
KernelHubOptions.IdleDisconnectTimeout(default 15 minutes) so the retention property can be asserted within a test budget without altering production behavior. - Adds
CompileActivityHubRetentionTestto validate that per-compile activity hubs are reclaimed after one idle window (with heartbeat/window scaled together). - Updates the
NodeTypeRecompileAlcLeakTestexplanation and documents the measured ceiling inMeshNodeStreamCache.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeRecompileAlcLeakTest.cs | Updates commentary to reflect measured bounded retention for Activity hubs. |
| test/MeshWeaver.Hosting.Monolith.Test/CompileActivityHubRetentionTest.cs | Adds a scaled CI guard that measures reclamation of per-compile activity hubs after the idle window. |
| src/MeshWeaver.Kernel/KernelHubOptions.cs | Adds a DI-overridable (default 15 min) idle disconnect timeout for kernel-hosting hubs. |
| src/MeshWeaver.Kernel.Hub/KernelContainer.cs | Uses KernelHubOptions (if registered) instead of a hard-coded 15-minute timeout. |
| src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md | Documents the bounded retention mechanism, ceiling, and measurement results. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results (shard 2)2 601 tests +126 2 600 ✅ +126 8m 12s ⏱️ + 1m 14s Results for commit 69e658e. ± Comparison against base commit 9fece59. This pull request removes 260 and adds 386 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 1)1 402 tests - 124 1 402 ✅ - 124 6m 59s ⏱️ -28s Results for commit 69e658e. ± Comparison against base commit 9fece59. This pull request removes 384 and adds 260 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
… a build-time snapshot The review read this as `.Where` filtering the timeout fallback into an empty sequence and an opaque `InvalidOperationException`. That is not what happens — `.Where` is UPSTREAM of `.FirstAsync()`, and `.Timeout(due, other)` wraps the `FirstAsync()` observable, so `other` bypasses the filter entirely: the fallback emits, the await completes, and the intended `remaining.Should().BeEmpty(...)` diagnostic is what reports. There was a real defect underneath it, though, and it is why the line reads that way: `Observable.Return(remaining)` is evaluated EAGERLY when the chain is built, so the fallback carried the pre-poll population rather than the latest one. The emitted value is discarded — only the `remaining` variable, reassigned by the `Select` side effect, feeds the assertion — so behaviour was correct, but a fallback that silently disagrees with the variable it names is a trap for the next reader. Deferred, with the operator ordering recorded on the line. Verified: CompileActivityHubRetentionTest 2/2 Release, reclamation observed at 24 s against the 20 s scaled window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1324 — on evidence, not with a fix. The issue's own question was "is the per-compile
_Activityhub retention BOUNDED?" The answer is yes, at 15 minutes, and this PR is themeasurement that establishes it plus the guard that keeps it true.
The measurement
Two runs, deliberately kept distinct.
1. Unscaled, at production timer values — 15 min
DisconnectTimeout, real 45 s heartbeat.Three compiles of a
config => configNodeType, then wait and count:13 hubs reclaimed in 915 s = 15.25 min — one idle window plus the 15 s poll granularity. The
mesh's total hub count went 49 → 32. (The earlier 51 → 47 step near t+660s is the cache's own
10-minute idle sweep — a different reaper doing a different job.) Local one-off, 15m19s; far too
slow for CI, which is what run 2 is for.
2. A 1:20 scale model, committed as the CI guard — 20 s window, 1 s heartbeat.
13 hubs across 3 compile activities, all reclaimed at 20.3 s against the 20 s window. Exactly
one idle window, nothing re-arming the timer.
So the residual
NodeTypeRecompileAlcLeakTestcounts — ~6 hubs / 3–5 MB per recompile, essentiallyall of it the
_Activity/compile-<ts>node hub and itssync/sub-hubs — is a transient with a15-minute ceiling, not a leak. That test simply runs for seconds and cannot watch it expire.
Why it is bounded — the chain the issue's earlier framing missed
An Activity node hub is one of the few hub kinds that carries an idle reaper of its own, in the
monolith as well as on Orleans:
registered by
KernelNodeType.AddKernel(), whichAddGraph()includes. The timer is one-shot andre-armed by every inbound message — which is exactly why the 45 s heartbeat kept it at bay
forever, and why #1435's terminal-write mirror release is what let the clock finally run.
🚨 The part worth reading: I rejected the cheap version of this test
The obvious test is "shorten the window, assert the hub goes away". That test would have gone
green while production reclaimed nothing. Shorten only the window and the 45 s heartbeat falls
outside it: the timer fires between two heartbeats, the test passes, and production — where 45 s
sits far inside 15 min — still never reclaims. So the window and the heartbeat are compressed
as a pair, at production's 1:20 ratio, and a regression of #1435 (a finished activity whose
mirror is left warm) keeps the hub alive here exactly as it does in production.
That is worth stating plainly because it is the second "cannot fail" guard on this one issue.
The first was
NodeTypeAssemblyLeakTest, deleted earlier in #1324: it asserted a NodeType's loadcontext is collected after mesh disposal — something a portal never performs — so for the bug's
entire life it read as evidence that recompiles were clean while production grew 130–340 MB/min.
Two of them on one issue is a pattern, not a coincidence: an assertion whose precondition the
system never reaches is indistinguishable from a passing one.
The prohibition, made structural
#1324 forbids "fixing" this by shortening a timer, and #1435's whole lesson is that the clocks were
correct and something was resetting them.
KernelHubOptions.IdleDisconnectTimeoutthereforedefaults to the existing 15 minutes and changes no production behaviour — it exists so the
property is assertable inside a test budget.
An option added so a property is assertable is one refactor away from becoming an option someone
quietly dials down, so that is asserted too:
What is in the diff
KernelHubOptions(new,MeshWeaver.Kernel) —IdleDisconnectTimeout, default 15 min.KernelContainer'sstatic readonly DisconnectTimeoutbecomes an instance field read from it,falling back to the default when unregistered. No behaviour change.
CompileActivityHubRetentionTest(new) — the scaled guard plus the production-defaultassertion above.
NodeTypeRecompileAlcLeakTest— corrects the paragraph claiming "the monolith has no idlecollection for node hubs at all … a host-lifecycle question". The general statement is true; it
is wrong for an Activity hub, which is the one this issue is about. Now carries the measured
numbers and points at the new test.
MeshNodeStreamCache.md— a section documenting the ceiling, the chain, the measurementtable, and why the pairing in the scaled test is load-bearing.
Verification
dotnet build -c Release -warnaserror:MeshWeaver.Kernel,MeshWeaver.Kernel.Hub,MeshWeaver.Hosting.Monolith.Test— all0 Warning(s), 0 Error(s).CompileActivityHubRetentionTest2/2 ·NodeTypeRecompileAlcLeakTest2/2 · Release.DocumentationLinkIntegrityTest1/1.No What's New entry: no user-visible behaviour change — a test, a doc, and an option whose
default is the value it replaced.
🤖 Generated with Claude Code