Repository navigation
Seven sites, one defect: Rx timers whose subscription reaches no owner's disposal chain #995
Description
Summary
Seven sites, one defect: an Rx timer whose subscription is not owned by anything that dies with the thing its closure captured. An Observable.Timer / Observable.Interval entry lives on the process-wide TimerQueue, which is a strong GC root — so while it is pending it holds its tick closure and everything that closure transitively captured. When that tail is a MessageHub, the hub cannot be collected.
Each occurrence has been found by MeshHubDisposalLeakTest firing at random on an unrelated PR, months apart, and fixed in isolation. This issue is about the pattern, not another instance.
The ledger
Two sub-shapes.
(1) The subscription is discarded outright — nothing can cancel the pending timer, so teardown does not release it:
| Site | Fixed in |
|---|---|
ActivityControlPlaneExtensions.SubscribeWithReEstablish |
#991 |
ThreadSubmission.InstallSubmissionWatcher |
#991 |
ThreadExecution.InstallExecRoundWatcher |
#991 |
ThreadExecution.InitializeThreadLifecycle |
#991 |
(2) The subscription is held, but strongly pins an owner that may never be disposed — fine for an owner that always tears down, fatal for an abandoned one (a hub created at RunLevel=1 that never reaches teardown):
| Site | Shape | Fix at the time |
|---|---|---|
MessageHub disposal watchdog |
uncancelled Task.Delay(25s) rooted the whole hub graph after every dispose |
Observable.Timer + TakeUntil(disposalCompleted) |
MessageHub.InstallStaleCallbackScanner |
Observable.Interval closure captured this |
WeakReference<MessageHub> + self-dispose on dead target |
JsonSynchronizationStream heartbeat |
Observable.Interval closure captured hub |
WeakReference<IMessageHub> + self-dispose on dead target |
Each of those three carries a comment naming the leak it caused, which is good — but a comment on the fixed site cannot prevent the next one.
Why the current detection is not enough
MeshHubDisposalLeakTest is what caught all of these, and it is a discovery probe, not a regression test (now documented on the test itself, in #991):
- It samples. A root live only for a bounded window — A pending Rx timer in ActivityControlPlaneExtensions pins the AccessAssignment hub past Mesh.Dispose() #991's was ~1 s — is caught only if the probe's forced GC lands inside that window. Otherwise: green, defect fully present.
- It cannot attribute. It reports the first
MessageHubreachable from any non-stack root, so it names whatever chain it walked, not the one you are chasing. - It is inconclusive off Linux. ClrMD snapshot-attach throws on macOS, so a survivor SKIPs (MeshHubDisposalLeakTest passes unconditionally on macOS (ClrMD unavailable → analysis never runs) #674) — a developer cannot reproduce locally on a Mac.
Net effect: the defect is found late, at random, on someone else's PR. #991 red-flagged PR #984, whose diff was five files with no C# in it at all.
What to decide
Audit every Observable.Timer / Observable.Interval in src/ and classify each: does its subscription reach an owner's disposal chain, and does its closure strongly capture something that might outlive that owner? (grep -rn "Observable\.Timer\|Observable\.Interval" --include="*.cs" src/ is ~35 hits, most of which are fine — composed inside a stream, or already assigned to a SerialDisposable / field the owner disposes.)
Then decide whether this is worth enforcing rather than remembering. Options, roughly in cost order:
- Nothing — accept that the probe finds one every few months, now that its limits are documented.
- A convention plus a review habit: a timer subscription is either assigned into an owner-disposed disposable, or composed into a stream that is; a periodic timer that outlives its subject holds it weakly. Write it down next to the existing guidance in
AsynchronousCalls.md/ControlledIoPooling.md. - A test that enumerates the sites — cheap and deterministic, but only as good as its list.
- An analyzer (Roslyn) for "
Observable.Timer(...)/Interval(...)whose.Subscribe(...)result is discarded". Catches sub-shape (1) mechanically. Sub-shape (2) is a capture-lifetime question and is probably not statically decidable here.
My read: (4) covers exactly the shape that caused #991 and is mechanical, while (2) is what actually covers both. Worth a maintainer's call rather than an agent's — the cost is small either way, but so is the incident rate, and the audit in step one should probably come first to size it.
Provenance
Split out of #991 at review, where fixing the fourth instance made the pattern visible.
Activity
rbuergi commentedon Aug 9, 2026
Audit result: the class is real but small — recommendation is (2) write the convention down, not an analyzer
Audited
src/(+test/for production-type impact) at4423f272d, with #996's four fixes present and verified in place.Counts
src/The number that decides the question: a sub-shape-1 analyzer would flag 1 of the 46 sites and 1 of the 6 findings. The other live defect is held in a field and reads as correct to any static rule. All 4 ambiguous sites are held too. Sub-shape 2 — which is 5 of the 9 sites already fixed, including all three in the issue's own ledger — is not statically decidable, so only a written convention covers the class.
There is a second reason the analyzer must stay narrow: an un-restricted "discarded
Subscriberesult" rule fires on 388 sites insrc/, of which ~386 are the cold-observablestream.Update(...).Subscribe(...)pattern that AGENTS.md mandates. Scoped down to timers, it finds one 100 ms bug. That is not an analyzer's worth of value.Method — and what is measured vs. judged
Three passes, deliberately not grep-only:
.csinsrc/andtest/: eachSubscribe/Connectinvocation classified by whether its value is consumed, plus the text of its receiver chain. 697 invocations insrc/, 388 with the result discarded. This is what makes a discardedIDisposablevisible when it is the tail of a fluent chain, which a text search misses.Observable.Timer|Intervalhits, 4System.Threading.Timersites, and 30Task.Delaysites — capture tail and owner-disposal path for each.Verified by construction (measured):
Task.WhenAny(winner, Task.Delay(long)).ContinueWith(…)— winner completes after hookupTask.WhenAny(completedTask, Task.Delay(long)).ContinueWith(…)— fast pathObservable.Timer(long).Subscribe(…)— result discardedObservable.Timer(long).Subscribe(…)— held +Dispose()Observable.Interval(long).Subscribe(…)— held, not disposed, strong captureTask.Delay(long).ContinueWith(…)— no tokenTask.Delay(long, token).ContinueWith(…)— token cancelledTask.Delay(long, token).ContinueWith(…)— token not cancellednew Timer(cb, …)one-shot —Dispose()before duenew Timer(cb, …)one-shot — not disposed, strong capturenew Timer(static cb, cell)— not disposed, cell severedObservable.Interval+WeakReferencecaptureJudged by reading (not measured): which owners are guaranteed to reach disposal. That is the residual uncertainty behind every "ambiguous" row below, and it is exactly the part no tool can settle.
The
Task.WhenAnyrow changed the outcome of this audit.DataContext.OpenInitializationGatearms an uncancelledTask.Delay(120 s)on every hub with a DataContext — by inspection the biggest find in the tree, and the same shape as the originalMessageHubwatchdog. It is not a leak: .NET'sWhenAnyremoves its continuation from the losing task, so the pending delay no longer reaches the closure. Had I stopped at reading, I would have reported a 120 s hub-wide leak that does not exist. Two other sites (McpMeshPlugin:871,MessageHubGrain:598) are cleared by the same measurement.The two live defects
Both are real roots by the measured table, both are sub-second, and neither is on the hub-lifecycle path — I am flagging them, not fixing them.
src/MeshWeaver.Kernel.Hub/ActivityLogLogger.cs:116Observable.Timer(…).Subscribe(…)discardedActivityLogLogger→hub(captured primary-ctor param, used athub.Post, line 211)ThrottleMs)src/MeshWeaver.Blazor/Components/MeshNodePickerView.razor.cs:232_debounceSub, but never disposed at teardownIMeshService→ mesh servicesDebounceMs)_debounceSubis disposed only by the next keystroke (line 231). The component never overridesDisposeAsyncand_debounceSubis not inBlazorView.Disposables, so the last pending debounce survives component disposal. An analyzer would pass this file clean — the field assignment looks like ownership.The four ambiguous sites (sub-shape 2)
Held, strongly capturing, and safe if the owner is disposed — which it is on every path I can trace. Listed because "owner is always disposed" is the assumption that failed in all three ledger cases.
src/MeshWeaver.Hosting/MeshNodeStreamCache.cs:520Observable.Intervalrecurring, fieldidleSweep, strongthiscacheHub.RegisterForDisposal(_ => Dispose())src/MeshWeaver.Graph/EventSubscriptionRunner.cs:362Observable.Timer(delay), held intimerSubs, strongthis→hubDispose()disposes every slotdelay = FireAt − nowis unbounded — a subscription scheduled days out pins the runner for days if the runner is ever abandoned. Longest window in the tree by orders of magnitude.src/MeshWeaver.Hosting.Snowflake/SnowflakeChangeFeedPoller.cs:85Observable.Intervalrecurring, field, strongthisDispose()→Stop()WeakReferencefixes.src/MeshWeaver.Messaging.Hub/MessageService.cs:876Task.Delay(30 s, cts.Token).ContinueWith(…), capturesthis→ hubProcessDeferredMessage) and onDispose()MessageServiceabandoned at RunLevel=1 with a deferred delivery — precisely the abandoned-hub state that produces deferrals — roots itself for 30 s.Two already-fixed sites missing from the ledger
The issue counts seven. There are nine; these two carry the same defect comment and the same ClrMD chain, so the pattern was recognised more often than the ledger records:
src/MeshWeaver.Kernel.Hub/KernelContainer.cs:178— idle-disconnectTimer, fixed withWeakReference+ astaticcallback +RegisterForDisposal. Its comment names the exact chain:StrongHandle → Timer → KernelContainer.<>c__DisplayClass → hub [RunLevel=1].src/MeshWeaver.Graph/MeshNodeTypeSource.cs:534— debounceTimer, fixed with a severable cell +staticcallback, with measurements in the comment (9 pinned hubs withthiscaptured, 0 through the cell). My probe reproduces both halves (rows E2/E3).That the same three idioms —
SerialDisposable,WeakReference+ self-dispose, severable cell — were each independently rediscovered is the actual argument for writing them down.Feasibility of (3), a test that enumerates sites
Feasible and cheap to write — my scanner is ~130 lines of
Microsoft.CodeAnalysis.CSharp, syntax-only, noMSBuildWorkspace, runs over the source tree in about a second. But:test/tree has two deliberate discarded-timer constructs that are the subject under test —ActivityOutlivesDisposeTest,IoPoolTest), and a stale allowlist is how an enumerating test rots into noise;For one 100 ms site that is a bad trade. If mechanical coverage is wanted later, keep the scanner as a one-off script re-run at audit time, not a committed test.
Recommendation, with the trade-off stated
Option 2 — write the convention down, next to the existing guidance in
AsynchronousCalls.md, and make the measured truth table part of it. Concretely: a timer subscription is either assigned into something the owner disposes, or composed into a stream that is; a timer that can outlive its subject holds that subject weakly or through a severable cell; aTask.Delayused as a gate is either insideTask.WhenAnyor carries a token that is always cancelled. Those four clauses cover all nine fixed sites and both live defects.What you give up by not building the analyzer: the mechanical guarantee on sub-shape 1. Today that guarantee is worth one site, and the sampling probe already found four of them. What you give up by building it: nothing much, but it buys 1-in-6 coverage while implying the class is handled — which is the worse failure mode, because the shape that actually recurs is the one it cannot see.
I'd also suggest the convention page carry the truth table verbatim. It is short, it was cheap to measure, and it settles the questions that get argued from memory — including that
Task.WhenAny+Task.Delayis safe, which is counter-intuitive and which I would have got wrong by reading.Scope note
This is an audit only. No analyzer added, no behaviour changed, no PR opened. The two live defects are flagged, not fixed — both are sub-second and off the hub-lifecycle path, so neither is actively harmful; route them separately if you want them closed.
Do not leave a live timer in a renderer-less probe (Copilot review)