Three idioms, nine sites, rediscovered by chance — the convention was never written down by rbuergi · Pull Request #1014 · Systemorph/MeshWeaver

rbuergi · 2026-08-09T13:17:42Z

Implements the audit's recommendation on #995: option (2), write the convention down. No analyzer, no enumerating test, no behaviour change — one new architecture page and four cross-links.

Why not the analyzer

The audit produced the number that settles it. A sub-shape-1 Roslyn rule — "Observable.Timer/Interval whose .Subscribe(...) result is discarded" — would have flagged 1 of the 46 timer/gate sites in src/ and 1 of the 6 findings. The second live defect (MeshNodePickerView) was held in a field and reads as correct to any static rule, as do all four ambiguous sites, as does the majority of the prior art. Widening it is worse: an unrestricted discarded-Subscribe rule fires on 388 of the 697 Subscribe/Connect invocations in src/, ~386 of which are the cold-observable stream.Update(...).Subscribe(...) pattern AGENTS.md mandates.

The page says all of that, with the numbers, because someone will propose the analyzer again.

New page, not a section — and why

Doc/Architecture/SubscriptionOwnership, a sibling of AsynchronousCalls rather than a section inside it. Two reasons: AsynchronousCalls.md is already 1456 lines and its scope is composing async work without await, whereas this is a lifetime/disposal rule; and it needs to be linkable from HubDisposalModel and DebuggingDisposalAndLeaks, which a subsection of a 1456-line page serves badly. AsynchronousCalls gets a short new subsection under "Subscribe is mandatory" — the rule that makes this class easy to get wrong — that states the pending-vs-completed distinction and hands off.

What it covers

The rule, applicable without the backstory: a timer/interval/delayed subscription must reach its owner's disposal chain, on every path. "Held in a field" is not ownership.
Both sub-shapes with real before/after, drawn from the actual fixes — ActivityLogLogger's discarded Subscribe → SerialDisposable + hub.RegisterForDisposal (Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel #1007), and MeshNodePickerView's field-that-only-the-next-keystroke-disposed → Disposables.Add(...) registered once in OnInitialized (Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel #1007), the sibling of MauiViewPack's Disposables.Add(_searchSub). Plus the two idioms for owners whose disposal is not guaranteed: WeakReference + self-dispose (InstallStaleCallbackScanner, the sync-stream heartbeat, KernelContainer) and the severable cell (MeshNodeTypeSource, with its measured 9-pinned-hubs-vs-0).
The measured truth table, verbatim — the most valuable part, because it stops people reasoning from memory. The three counter-intuitive rows are called out: Task.WhenAny removes its continuation from the losing task so it does not root (this cleared DataContext.OpenInitializationGate, which by inspection reads as a 120 s hub-wide leak and is not one); a fired Observable.Timer(...).Subscribe(...) handle, still held, does not retain its callback closure; a bare Task.Delay(...).ContinueWith(...) does root. That last fact is also why discarding stream.Update's handle is safe — which is the same fact that makes the analyzer's false-positive rate what it is, so the page ties the two together rather than leaving them as unrelated trivia.
Why a green MeshHubDisposalLeakTest is not evidence — it samples, it cannot attribute, it SKIPs on macOS (MeshHubDisposalLeakTest passes unconditionally on macOS (ClrMD unavailable → analysis never runs) #674) — and what the right pin is instead: a targeted, timing-free ownership test plus a negative control, with Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel #1007's two negative-control results as the worked example. Also: assert both halves (teardown disposes the handle and the working path arms into it), or the registration can guard a slot nothing writes to and the test still passes.
How to re-audit: three passes, only the first mechanical. The ~130-line Roslyn scanner stays a one-off script re-run at audit time, out of the tree — the reasoning (allowlist rot, coupling a unit test to repo layout) is recorded so the next person does not re-litigate it.

Written as running prose with real code, not a bullet list of platitudes — a developer who has never seen #995 should be able to apply it, and the four-clause summary at the end is what fits in a review comment.

Cross-links

Added from the four places a reader would actually start: AsynchronousCalls ("Subscribe is mandatory"), DebuggingDisposalAndLeaks (the ClrMD probe section — which now also carries the "a pass here pins nothing" warning inline, matching the remarks already on the test), HubDisposalModel ("adding disposal work"), and the Architecture topic map under Reactive core.

Verification

dotnet build src/MeshWeaver.Documentation -c Release -warnaserror — 0 warnings, 0 errors.
DocumentationLinkIntegrityTest — passed (every link is absolute /Doc/…, per AuthoringDocumentation).
No H1 in the new page: the frontmatter Name is the page title, so the body opens on the lead paragraph.

Notes

No What's New entry. The feed is for changes a portal user notices; this is a contributor-facing engineering convention with no behaviour change. Same call ReleaseToProductionPipeline.md made, and the same reason Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel #1007 skipped one for the fixes themselves.
Fixes #995 — Seven sites, one defect: Rx timers whose subscription reaches no owner's disposal chain #995 asked for a decision between four options and the audit recommended (2). The two live defects it turned up were closed by Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel #1007; writing the convention closes the class item, which is what the issue is about.
One thing worth flagging separately rather than fixing here: the audit's four ambiguous sites (MeshNodeStreamCache idle sweep, EventSubscriptionRunner's unbounded FireAt − now delay, SnowflakeChangeFeedPoller, MessageService's 30 s deferred-delivery delay) remain as the audit left them — safe if the owner is disposed, which is the assumption that failed in all three ledger cases. The page now states the rule they'd be measured against; whether to harden them is a separate call.

Fixes #995

…overing Nine sites in src/ independently arrived at the same three fixes for the same defect — an Rx timer whose subscription reaches no owner's disposal chain — months apart, each found by chance when MeshHubDisposalLeakTest sampled an unrelated PR. The #995 audit swept the tree and settled the enforcement question with numbers: a sub-shape-1 analyzer would flag 1 of 46 sites and 1 of 6 findings, and an unrestricted "discarded Subscribe" rule fires on 388 sites in src/, ~386 of which are the cold-observable stream.Update(...).Subscribe(...) pattern AGENTS.md mandates. So: no analyzer, no enumerating test — write the convention down. New page Doc/Architecture/SubscriptionOwnership, a sibling of AsynchronousCalls rather than a section inside it: that page is 1456 lines about composing async work without await, and this is a lifetime/disposal rule that HubDisposalModel and DebuggingDisposalAndLeaks also need to point at. It carries both sub-shapes with the real before/after from #996 and #1007 (SerialDisposable + RegisterForDisposal on the hub; Disposables.Add registered once in OnInitialized for Blazor), the WeakReference and severable-cell idioms for owners that may never be disposed, and the measured GC-root truth table — including the counter-intuitive rows: Task.WhenAny removes its continuation from the losing task so it does NOT root (this cleared DataContext's 120 s OpenInitializationGate, which by inspection reads as a hub-wide leak), a FIRED Rx timer handle does not retain its callback closure, and a bare Task.Delay(...).ContinueWith(...) does root. That same detach-on-completion fact is why discarding stream.Update's handle is safe, which is why the analyzer's false-positive rate is what it is. Also records why a green MeshHubDisposalLeakTest is not evidence (it samples, cannot attribute, SKIPs on macOS per #674) and that the right pin is a timing-free ownership test plus a negative control; and why the audit's ~130-line Roslyn scanner stays a one-off script rather than a committed test with an allowlist that would rot. Cross-linked from AsynchronousCalls' "Subscribe is mandatory" section (the rule that makes this class easy to get wrong), the DebuggingDisposalAndLeaks ClrMD probe, HubDisposalModel's "adding disposal work", and the Architecture topic map. Fixes #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…18285d07eae

Copilot

Pull request overview

Adds a new architecture documentation page that formalizes the repository’s convention for subscription/timer ownership and disposal, and cross-links it from the most relevant existing guidance so the rule is discoverable during implementation and debugging.

Changes:

Introduces Subscription Ownership architecture page documenting timer/interval/delay rooting behavior, the two leak shapes, and the recommended ownership idioms.
Adds focused cross-links and short guidance callouts in existing architecture pages (AsynchronousCalls, HubDisposalModel, DebuggingDisposalAndLeaks).
Updates the Architecture topic map to include the new page under “Reactive core”.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/MeshWeaver.Documentation/Data/Architecture/SubscriptionOwnership.md	New architecture page defining the timer/subscription ownership convention and rationale.
src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md	Adds an explicit disposal-chain reminder and link for timer/debounce subscriptions.
src/MeshWeaver.Documentation/Data/Architecture/DebuggingDisposalAndLeaks.md	Clarifies probe limitations (“pass pins nothing”) and links to the ownership convention.
src/MeshWeaver.Documentation/Data/Architecture/AsynchronousCalls.md	Adds a targeted subsection distinguishing completed subscriptions from pending timers and links to the new page.
src/MeshWeaver.Documentation/Data/Architecture.md	Adds Subscription Ownership to the “Reactive core” topic map for discoverability.

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

github-actions · 2026-08-09T13:32:23Z