Repository navigation
Two more pending timers nobody owned — one discarded, one only the next keystroke could cancel - #1007
Merged
Merged
Conversation
Both sites arm an Rx timer and then have no way to cancel it. Observable.Timer parks on the process-wide TimerQueue — a strong GC root — so for as long as the timer is pending it holds the tick closure and everything the closure captured, past the owner's disposal. ActivityLogLogger discarded the tail-flush Subscribe result outright (the same shape as the four watcher timers fixed in #996, at 100 ms instead of 1 s). The pending flush held the logger, and the logger holds the hub it posts to. MeshNodePickerView looked correct — the subscription IS assigned to a field — but the only thing that ever disposed that field was the NEXT keystroke. Type, then navigate away, and the last 200 ms timer outlives the component and roots it through the injected IMeshService. Ownership, not capture, was missing, which is why a static analyzer reads the file clean. Both now hold the pending timer in a SerialDisposable that the owner's disposal chain disposes: the hub's composite for the logger, BlazorView.Disposables for the picker. Assignment keeps the existing 'a new one cancels the old one' semantics. Refs #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither test is a GC sampling probe. Both assert the ownership property directly and timing-free — the handle holding the pending timer is disposed by the owner's teardown — so they hold whether or not the timer has already fired, which is exactly what MeshHubDisposalLeakTest cannot do. The picker test also asserts that the keystroke path arms INTO the registered handle. Without it, the registration could guard a slot nothing writes to and the test would still pass. Refs #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe posts a real DataChangeRequest to its own address, as KernelExecutor does; without a handler and type registrations every run left a DeliveryFailure warning plus six auto-short-name warnings. Accept the message the way the activity hub does in production. Refs #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refs #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two retention defects where Observable.Timer(...).Subscribe(...) was not owned by an appropriate disposal chain, allowing pending timers (rooted by the process-wide TimerQueue) to keep hubs/components alive past teardown.
Changes:
- Fix
ActivityLogLoggertail-flush scheduling by storing the timer subscription in aSerialDisposableregistered viahub.RegisterForDisposal(...). - Fix
MeshNodePickerViewsearch debounce ownership by using a component-lifetimeSerialDisposableregistered once inBlazorView.Disposables(OnInitialized), soDisposeAsynccancels the last pending debounce. - Add two targeted ownership tests to assert teardown cancels pending timers for both sites.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Blazor.Test/MeshNodePickerDebounceOwnershipTest.cs | Adds a test asserting the picker’s debounce timer is owned by the component teardown path. |
| test/MeshWeaver.AI.Test/ActivityLogTailFlushOwnershipTest.cs | Adds a test asserting the activity-log tail-flush timer is cancelled when the owning hub is disposed. |
| src/MeshWeaver.Kernel.Hub/ActivityLogLogger.cs | Owns the tail-flush Observable.Timer via a SerialDisposable registered in the hub disposal chain. |
| src/MeshWeaver.Blazor/Components/MeshNodePickerView.razor.cs | Owns the debounce Observable.Timer via a component-lifetime SerialDisposable registered in Disposables during OnInitialized. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The keystroke arms a real 200 ms Observable.Timer, and this component has no render handle — a tick would reach InvokeAsync/StateHasChanged and throw on the timer thread. The test finished in microseconds, but 'finishes before the timer' is a timing assumption, and this test exists precisely to stop relying on those. Swap the armed timer for a stand-in once it has served its purpose (proving the keystroke writes into the component-owned handle). Teardown then cancels the stand-in, which asks the same question with no live timer anywhere. Refs #995 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results (shard 4)1 271 tests +76 1 266 ✅ +81 7m 17s ⏱️ -31s Results for commit 0776bce. ± Comparison against base commit 3e18426. This pull request removes 536 and adds 612 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Contributor
Test Results (shard 3) 11 files ± 0 11 suites ±0 5m 45s ⏱️ + 1m 7s Results for commit 0776bce. ± Comparison against base commit 3e18426. This pull request removes 598 and adds 536 tests. Note that renamed tests count towards both. |
Contributor
Contributor
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 the two live defects the #995 audit found — and only those. The 40 verified-safe sites and the 4 ambiguous ones are untouched; the audit measured them, and widening this diff would only add regression surface.
Both are the class #996 fixed: an Rx timer whose
IDisposablenever reaches an owner's disposal chain.Observable.Timerparks its entry on the process-wideTimerQueue, a strong GC root, so while the timer is pending it holds the tick closure and everything the closure captured — past the owner's disposal.The two sites
ActivityLogLogger.cs:116— a discardedSubscribe. Structurally identical to the four in #996, at a tenth of the window. A burst of script log calls is coalesced by arming a 100 ms tail-flush timer; the subscription it returned was dropped on the floor, so nothing could cancel a flush still pending when the kernel executor hub tore down. The pending flush held the logger, and the logger holds theIMessageHubit posts to.Fix: the timer goes into a
SerialDisposableregistered on that hub (RegisterForDisposal). Teardown disposes the hub's composite, which cancels the timer. Assignment keeps the existing "arm one at a time" semantics —_publishScheduledalready guarantees at most one is pending, so a live flush can never be clobbered — and a schedule handed over after disposal is disposed on assignment, so there is no dispose race either.MeshNodePickerView.razor.cs:232— capture without ownership. This one reads correct: the subscription IS assigned to a field, and a discarded-Subscribeanalyzer passes the file clean. But the only code that ever disposed that field was the next keystroke. The component never overrodeDisposeAsyncand the field was not inBlazorView.Disposables, so a user who typed into the picker and navigated away left the last 200 ms timer on theTimerQueue, rooting the component → its injectedIMeshService→ the mesh services.Fix: same
SerialDisposable, registered once inOnInitializedinto the framework's existingBlazorView.Disposables(released byDisposeAsync) rather than a bespoke override. Registered there and not inBindData, which re-runs on every binding-relevant parameter change — that would both duplicate the entry and cancel a debounce the user is still mid-typing.Does this eliminate the window or narrow it?
Eliminated in both cases, not narrowed — the fix is ownership, not a shorter timer.
disposables.Dispose(). There is no interval in which a pending timer outlives the hub, and no race with a concurrent arm (SerialDisposabledisposes an incoming value when already disposed).DisposeAsyncreleasesDisposables.OnInitializedis guaranteed by Blazor to run before any event handler can fire, so there is no keystroke that can arm a timer outside the registration.What remains bounded rather than zero is normal operation: while a hub or component is alive, a pending timer legitimately holds it for its 100/200 ms. That is the feature.
Verification
Two targeted tests, one per site. Deliberately not
MeshHubDisposalLeakTest— as its own remarks now say, it samples (a bounded-window root is caught only if the forced GC lands inside the window), it cannot attribute, and it SKIPs on macOS (#674). Both new tests assert the ownership property directly and timing-free: the handle holding the pending timer is disposed by the owner's teardown, which holds whether or not the timer has already fired. Neither can flake, and neither can pass by accident.Negative control run, per site — revert only the ownership line, signature unchanged:
hub.RegisterForDisposal(pending)inActivityLogLogger.RegisterPendingFlushDisposingTheHub_CancelsThePendingTailFlushFAILS: "Expected value to be true because tearing down the hub must cancel a tail flush that is still pending … but found False"Disposables.Add(_debounceSub)inMeshNodePickerView.OnInitializedDisposeAsync_CancelsTheLastPendingDebounceFAILS: "Expected collection to contain a single item because OnInitialized must register the debounce handle in BlazorView.Disposables … but found 0"Both restored, both green. The picker test also asserts that the keystroke path arms into the registered handle — without that, the registration could guard a slot nothing ever writes to and the test would still pass.
One thing I measured rather than reasoned about, since it decides whether
RegisterForDisposaltrades one leak for another: a firedObservable.Timer(…).Subscribe(…)handle, still held, does not retain its callback closure (Rx's sink detaches its observer on completion). Verified with aWeakReferenceprobe alongside three controls, including "pending + discarded ⇒ ROOTED", which confirms the probe can see the defect shape. So the hub retains only theSerialDisposableitself (~32 B per logger); a settled logger and its message list stay collectable while the hub lives on.Also green:
ActivityTerminalSettleTest,ActivityLogStreamTest,MultiLanguageCodeExecutionTest(20 tests) — the kernel/activity-log paths through the changed logger. Full solution builds clean under CI's flags (-c Release -warnaserror -p:CIRun=true), 0 warnings.Notes
Refs #995, deliberately notFixes— Seven sites, one defect: Rx timers whose subscription reaches no owner's disposal chain #995 tracks the defect class, and the maintainer's decision on option (2), writing the convention down next toAsynchronousCalls.md, is still open. This PR closes the two live sites, not the issue.Refs #995