A pending Rx timer kept disposed hubs alive — in four watchers, three of them on the live thread path by rbuergi · Pull Request #996 · Systemorph/MeshWeaver

rbuergi · 2026-08-09T08:56:37Z

Four watchers scheduled their self-healing re-establish as Observable.Timer(1s).Subscribe(...) and threw the returned IDisposable away. Nothing could cancel it, so tearing the watcher down left the timer pending — and an Rx timer lives on the process-wide TimerQueue, which is a strong GC root. While it is pending it holds the tick closure, the Establish delegate, the method closure, the caller's source factory, and everything that factory captured: a MessageHub and its entire graph.

Fixes #991.

The finding that matters: three of the four are on the live thread path

#991 was filed against ActivityControlPlaneExtensions, because that is the arm the ClrMD dump happened to catch. Auditing for the same shape turned up three more copies — hand-rolled versions of the same primitive in ThreadSubmission.InstallSubmissionWatcher and both watchers in ThreadExecution. Those are production code on the thread submission/execution path, not a test-only surface. So the honest framing is not "a leak test was flaky"; it is a leak in shipped code that a flaky test exposed.

Severity, quantified — because "leak" alone is too coarse. I checked what each watcher's lifetime actually is rather than assuming the worst:

All four are one per hub, installed at hub init. In particular _Exec is created eagerly once per thread hub (InstallExecutionHub, HostedHubCreation.Always), not per round, so this does not accumulate per submitted message or per agent round.
Each occurrence is bounded at roughly one timer interval (~1 s): when the tick lands, Establish early-returns on disposed and does not re-arm. Retention, not unbounded growth.
What it costs is teardown waves. Disposing a hub faults its own-node stream, so arming this timer is one of the last things many hubs do before dying. NodeType recompile churn, grain deactivation, thread-hub idle collection and portal shutdown all tear down many hubs inside the same second, so the retained graphs overlap and peak memory carries an extra generation of hub graphs.

A root that accumulated per message would be far worse than this; it does not, and I would rather say so than let the diff imply it.

What was actually wrong

SubscribeWithReEstablish is the sanctioned subscription primitive for any watcher whose death would leave a hub half-alive, so every per-NodeType hub installs three of them (compile, sources, release-request). Its teardown was:

disposed = true;
serial.Dispose();       // kills the live subscription…
                        // …but nothing owns the pending re-establish timer

The source matches the ClrMD chain on #991 exactly, including the display-class ordinals: DisplayClass2_1 is the reEstablish => Observable.Timer(...) lambda's scope, DisplayClass2_0 is SubscribeWithReEstablish's own closure (it holds source), and SubscribeWithReEstablish is the third closure-bearing method in the class, hence ordinal 2.

All four now hold the schedule in a SerialDisposable that teardown disposes. The scheduleReEstablish test seam changed from Action<Action> to Func<Action, IDisposable> so that a scheduled re-establish is expressible as cancellable at all — the old signature could not represent it.

This eliminates the window; it does not merely narrow it

Stating it plainly, since it is the load-bearing claim: after Dispose() returns, the TimerQueue entry is gone and the closure graph is immediately unreachable. There is no residual window — not a shorter one.

The scope of that claim is this root. The guard reds on any non-stack root reaching a hub, so if it reds again with a different chain, that is a different root and needs its own fix.

This is a class, not four one-off fixes — follow-up filed

Worth more to a reviewer than this diff: the same root-holding pattern has now been found three separate times, by the same probe, before this PR — each fixed in isolation, each with a comment in the code naming the leak it caused:

Where	Shape	Fix at the time
`MessageHub` disposal watchdog	uncancelled `Task.Delay(25s)` rooting the whole hub graph after every dispose	`Observable.Timer` + `TakeUntil(disposalCompleted)`
`MessageHub.InstallStaleCallbackScanner`	`Observable.Interval` closure capturing `this`	`WeakReference<MessageHub>` + self-dispose on dead target
`JsonSynchronizationStream` heartbeat	`Observable.Interval` closure capturing `hub`	`WeakReference<IMessageHub>` + self-dispose

Add this PR's four and it is seven sites of one defect: an Rx timer whose subscription is not owned by anything that dies with the thing its closure captured. Two sub-shapes — the subscription is discarded outright (this PR), or it is held but strongly pins an owner that may never be disposed (the WeakReference three). Each was found by a probe firing at random on an unrelated PR, months apart, which is an expensive way to find the eighth.

Filed as #995: audit every Observable.Timer / Observable.Interval in src/ for a subscription that reaches no owner's disposal chain, and decide whether this is worth enforcing rather than remembering. That issue lays out the options (do nothing / write the convention down / enumerate the sites in a test / a Roslyn analyzer for the discarded-subscription shape) but deliberately leaves the call to a maintainer — the incident rate is low, and the audit should size it first.

How this was verified

I did not take the ClrMD chain on faith. ActivityControlPlaneResubscribeTest gains two tests, and I ran the negative control both ways:

Dispose_CancelsAPendingReEstablishSchedule — the contract, no timing involved: disposing the watcher must dispose the handle the schedule returned.
Dispose_ReleasesTheSourceFactory_SoAPendingTimerCannotRootIt — the leak itself, with the real production 1 s timer and no seam: fault the watcher, dispose it, force a blocking gen-2 collect, assert what the source factory captured is gone.

With the two capture lines reverted (defect restored, signature unchanged) both fail with the expected messages; with the fix both pass. That is the pin.

`MeshHubDisposalLeakTest` pins nothing — now documented on the test

It should not be read as verification for this or any future leak fix, so this PR says so in its docstring rather than leaving a future reader to assume a green run means something it does not. It samples (a bounded-window root is caught only if the forced GC lands inside the window — the issue says this outright), it cannot attribute (it reports the first hub reachable from any non-stack root), and on macOS ClrMD snapshot-attach is unsupported so a survivor SKIPs as inconclusive (#674). I verified that SKIP is identical on unmodified origin/main in a throwaway worktree before attributing it to #674 rather than to this change. Its job is discovery — naming a root nobody knew about — which is exactly what it did here.

Why this was worth prioritising

The defect has been reddening PR #984 at random — a diff of five files containing no C# at all, which cannot create a GC root. An unrelated PR was blocked by this, and while it is live a genuine new leak would be hard to tell from the noise.

Testing

Solution builds clean under CI's flags (-c Release -p:CIRun=true -warnaserror), 0 warnings, including the new VerifyXunitRunnerConfigCopied guard from perf(ci): split CD bake job, mirror plugin image to GHCR, add per-project test parallelism #979.
MeshWeaver.Hosting.Test — 131/131.
MeshWeaver.AI.Test — 1090 passed, 5 skipped (pre-existing env-gated E2E).
MeshWeaver.Hosting.Monolith.Test — 498 passed, 4 skipped, 0 failed.

No What's New entry

Deliberate. The user-visible symptom a What's New entry would describe is a portal accumulating leaked hubs over a long session, and that is specifically what this is not: the retention is bounded per teardown and does not grow with usage. Nothing an operator or user would observe changes — this is hub teardown hygiene.

A watcher's re-establish was scheduled as Observable.Timer(1s).Subscribe(...) with the returned IDisposable thrown away. Rx parks that timer on the process-wide TimerQueue — a strong GC root — so while it is pending it holds the tick closure, the Establish delegate, this method's closure, and through it the caller's source factory and everything the factory captured. In production that tail is a per-NodeType MessageHub. Disposing the watcher set a disposed flag and killed the live subscription, but nothing could cancel the pending timer, so a hub whose stream faulted in the last second of its life stayed rooted past Mesh.Dispose(). Hold the schedule in a SerialDisposable and dispose it on teardown; the seam now returns the handle that cancels it. Same fix in the three hand-rolled copies in ThreadSubmission and ThreadExecution. Fixes #991 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…63c7bf46a1d

MeshHubDisposalLeakTest samples: a root that is live only for a bounded window is caught only if the forced GC lands inside it. It also cannot attribute — it reports the first hub reachable from any non-stack root — and it SKIPs as inconclusive off Linux. A future reader would otherwise take a green run as proof that a leak fix worked. Point at the pattern that does pin a root: a deterministic test next to the code that owns it, proven by reverting the fix and watching it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a hub-retention/memory-leak pattern where re-establish “self-heal” timers were scheduled via Observable.Timer(...).Subscribe(...) but the returned IDisposable was discarded, preventing teardown from cancelling the pending timer and allowing disposed hub graphs to stay rooted via the process-wide TimerQueue.

Changes:

Make re-establish scheduling cancellable by owning and disposing the timer subscription (SerialDisposable) across four watcher sites.
Update SubscribeWithReEstablish’s scheduling seam to return an IDisposable so teardown can cancel pending schedules.
Add deterministic tests that pin the leak root (cancellation + GC-root release) and document MeshHubDisposalLeakTest as a sampling probe rather than proof.

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.Test/ActivityControlPlaneResubscribeTest.cs	Updates the schedule seam usage and adds deterministic tests that prove pending re-establish schedules are cancelled and cannot root captured objects.
test/MeshWeaver.Hosting.Monolith.Test/MeshHubDisposalLeakTest.cs	Clarifies the probe’s limitations (sampling/attribution/platform support) to prevent misinterpretation of green runs.
src/MeshWeaver.Mesh.Contract/ActivityControlPlaneExtensions.cs	Fixes the core leak by owning/discarding pending re-establish schedules on teardown; updates the seam signature to return a cancellable `IDisposable`.
src/MeshWeaver.AI/ThreadSubmission.cs	Owns/disposes the pending re-establish timer to prevent disposed thread hubs being retained after faults during teardown.
src/MeshWeaver.AI/ThreadExecution.cs	Owns/disposes pending re-establish timers in execution watchers and ties them into hub disposal.

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

github-actions · 2026-08-09T09:12:23Z