Repository navigation
The thing that OPENED the lifetime scope closes it — not the hub inside it - #2438
Conversation
There was a problem hiding this comment.
Pull request overview
Ensures each MessageHub disposes the DI container it creates per hub instance, addressing leaks and shutdown instability by disposing the hub’s owned service provider during teardown and adding a regression test for the ownership contract.
Changes:
- Disposes the hub-owned
ServiceProviderduring theShutDownphase ofMessageHubteardown. - Adds a new test validating that disposing a hub disposes disposable singletons created within its DI container.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/HubDisposesItsServiceProviderTest.cs | Adds a regression test asserting hub disposal also disposes a disposable singleton resolved from the hub’s container. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Adds DisposeServiceProvider(...) and invokes it during the ShutDown phase before signalling DisposalCompleted. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 0)898 tests +2 898 ✅ +2 5m 29s ⏱️ -4s Results for commit 13546b1. ± Comparison against base commit 89f8862. This pull request removes 6 and adds 8 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 4) 10 files ±0 10 suites ±0 6m 5s ⏱️ -1s Results for commit 13546b1. ± Comparison against base commit 89f8862. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results 42 files ± 0 42 suites ±0 35m 19s ⏱️ +8s Results for commit 13546b1. ± Comparison against base commit 89f8862. This pull request removes 7 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
All 8 red checks are own-diff — the container is disposed one whole teardown phase too early3 shards actually failed (1, 2, 5); the other 5 red checks are their reporters plus the consolidator. 9 failing tests, none pre-existing. The diff is 2 files / +150 lines and introduces exactly one behavioural change — Reproduced deterministically on this branch, locally, in isolation: Root cause: placement
The PR disposes the scope inside the ShutDown phase, i.e. before What that costs, in four families
Why I stopped rather than pushing a fixThe leak this PR targets is real — hosted-hub child scopes do accumulate under a long-lived parent, and Autofac tracks every one until the parent dies. But making hub containers mortal is a system-wide invariant change: getting it green means changing the delete pipeline, stream construction, the test base and hosted-hub teardown ordering, and then auditing those 922 resolution sites — because a timing-dependent invariant that passes one CI run is not thereby safe. Chasing each That is a different change from "a hub never disposed its own DI container", so it needs your scope call. The two shapes I'd weigh:
|
Pull request was converted to draft
Converting to draft — the placement is wrong, and the tests are rightTwo independent tests caught this, and they say the same thing from different angles. Control on clean 1 — unconditional disposal broke The teardown orchestration resolves 2 — narrowed to hubs that own a child scope still breaks A disposing hub must refuse with the domain-meaningful The conclusionThe container must outlive the hub's own disposal, because the hub's disposal semantics are expressed through code that still resolves from it. What still standsThe leak is real: The right owner is
|
rbuergi
changed the title
|
CI found two real consumers that resolve from a hub's container after its disposal has completed. Both are regressions from this PR, not flakes, and I am putting it back to draft rather than re-running until it goes green. What failed
The test name is the contract: those callbacks must be graceful no-ops. They were, only because there had never been a closed scope for them to meet.
Not a straggler — a real operation failing. What this means for the changeThe ownership claim still holds: the thing that opened the scope should close it, nothing did, and the two costs (finalizers against a torn-down graph; one live scope per recycled hub) are real. What CI shows is that closing the scope is a behaviour change with a blast radius, because several paths legitimately use a hub's provider after So this needs the consumers fixed first, and each on its own merits:
What I deliberately did NOT doAdd a grace period before closing the scope. A timer would make both failures disappear and would leave a race that fires rarely and never reproduces — the shape this codebase keeps paying for. The leak is measured in hours of hub recycling; the fix should be ordering, not a delay. Local state for whoever picks this up: |
|
Correcting my previous comment — I attributed the Orleans failure to the wrong cause. The accurate reading, with stacks: 1. A process-poisoning FATAL in
|
|
Checked this PR against the #2346 fix I have in flight (#2482) — and it does not unblock you. Saying so explicitly, because "another Orleans flake on shard 3" is exactly the inference that keeps mis-attributing this cluster. Run 33040002238 failed three tests, none of them mine:
#2482 fixes one specific mechanism — a cross-hub write whose leaf is silently refused because the per-path queue released its slot on the optimistic emit instead of on the owner's verdict. Its signature is a wrong value at a terminal state ( Worth noting the two PostgreSql ones failed together — a pair in the same area is more often one real cause than two flakes, and neither appears in #2346. If the pair recurs it probably deserves its own issue rather than being folded into the cluster. |
Re-synced onto #2505, and the new diagnostics named it on first contact — this failure is OWN-DIFFI re-synced this branch after #2505 (the Orleans/test diagnostics fix) landed. The run now fails shard 2, a shard it had not failed before, and — unlike every previous attribution on this PR — the failure is legible without a matched local control: "Instances cannot be resolved and nested lifetimes cannot be created" is Autofac's
A delete now resolves DI from a hub whose container this PR has disposed. That is family 1 and 2 of the four the analysis listed, reaching a test that names the stage it died in. #2505 earning itself immediatelyAlso in that job: Under the previous reporting a host that died after streaming results was indistinguishable from a pass — the What this means for the PRThe earlier report on #2438 stopped short of merging and asked for a scope call, on the grounds that making hub containers mortal is a system-wide invariant change touching the delete pipeline, stream construction, the test base and hosted-hub teardown, with 922 resolution sites to audit. This failure is direct evidence for that position, not against it: the first shard to exercise the delete path under the new diagnostics found a disposed-scope resolution immediately. The two options remain as stated — sequence it (land the delete-pipeline and stream-construction fixes first, each correct on its own merits, then make containers mortal on top), or relocate the disposal to the Not merging, and not re-running. Recording the attribution because it is now measured rather than argued. |
A second front, and it strengthens the same conclusionShard 1 on the same run also failed: So the run now carries two independent Monolith tests on two different shards, plus a
Both are I am not claiming the two share one root by measurement — that would need the same discrimination I did for #2475. What I can say is that two tests, on two shards, in the paths this diff touches, failing on a branch whose only behavioural change is disposing hub containers, is not a coincidence worth explaining away. What is now different, and worth noting for its own sakeBoth shards also produced That matters beyond this PR: the previous "green" runs of branches touching hub disposal may have been masking host crashes. Anyone reading this PR's history should treat pre-#2505 green runs on disposal-adjacent work as weaker evidence than they appear. Still not merging, still not re-running. |
d9f2a91 to
a552094
Compare
…de it
A hub that owns its DI lifetime scope must not be the thing that closes it: closing the scope
disposes the hub that lives in it, so the hub would be tearing down the container it is still
running inside. HostedHubsCollection opened the scope, so it closes it — strictly after the hub
is terminally down, driven off DisposalCompleted.
That signal covers every terminal shape, which is the point:
- a clean shutdown signals it at the end of the ShutDown phase;
- a FAULTED disposal reaches it through .Catch(_ => Return(Unit)) — a teardown that failed is
when the handles are most likely still held, so it must free the scope too;
- a source completing WITHOUT emitting is settled by .DefaultIfEmpty rather than parked;
- and the watchdog's out-of-band ForceTeardownAfterWatchdog signals it as well, so a hub torn
down after a disposal deadlock closes its scope through the same single path.
REBUILT ONTO CURRENT MAIN. Core's history was rewritten on 2026-08-28, and this branch predates
it — so the two share no merge base and `git merge origin/main` produced 5996 files / 889k
insertions plus an add/add conflict, none of which is this change. The branch is therefore
replayed as the PR's actual 6-file diff on top of today's main rather than merged; the content is
unchanged (`gh pr diff` applied clean).
Verified with CI's flags: MeshWeaver.Messaging.Hub, MeshWeaver.Data and both touched test
projects build Release -warnaserror clean; HubDisposesItsServiceProviderTest 2/2 and
DeadStreamSafetyTest 6/6 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a552094 to
184a1fc
Compare
Rebuilt onto current
|
🚨 This is a real regression in the PR, not a flake — reproduced locally and attributed by a control runI chased this the way I'd chase someone else's red, and it does not come out in this PR's favour. The measurementSame machine, same command, same 749-test assembly,
The only variable is this diff. The failureCI (shard 2) and my local run failed different tests in the same family, with the identical error: Two distinct tests, two environments, one error class — and that error class is precisely what this PR introduces, since its entire subject is who closes hub lifetime scopes. It only reproduces with the FULL assemblyWorth recording so nobody repeats the cheap checks:
An isolated run discriminates nothing here: this is a cross-mesh scope race, so the interference between test classes IS the trigger. Mechanism — and one hypothesis I killedThe error names a parent scope, and closing a child scope never disposes a parent. So something resolves from a scope whose ANCESTOR already closed. I first suspected What fits the evidence: a recursive delete posts Two candidate fix shapes, neither obviously right
(1) is smaller and matches how the codebase already classifies disposal races; (2) addresses the ordering directly but is a bigger change to the delete path. Not merging this. The leak fix is right and the review thread about the watchdog is answered, but shipping it as-is breaks recursive delete. |
…node's scope is gone after it
Closing the regression this PR introduced, found by running the full assembly on both branches.
Making HostedHubsCollection actually CLOSE a hub's lifetime scope turned a leak into a live
failure in recursive delete:
Delete failed for 'TestData/rtspace/keep': Instances cannot be resolved and nested lifetimes
cannot be created from this LifetimeScope as it (or one of its parent scopes) has already
been disposed.
The stack names it exactly — not hub activation (CreateHub catches and returns null, so it cannot
produce this), but step 5:
at Autofac…AutofacServiceProvider.GetRequiredService(Type)
at MeshExtensions.RunPostDeletionHandlersObs(...) MeshExtensions.cs:3629
at MeshExtensions.<HandleDeleteNodeRequest>b__20(...) MeshExtensions.cs:2574
RunPostDeletionHandlersObs resolved INodePostDeletionHandler from hub.ServiceProvider AFTER the
subtree was deleted. That hub is the deleted root's own; its scope is now closed, so Autofac
throws at RESOLUTION — outside the per-handler .Catch, so it escapes as a failed delete even
though step 5's whole contract is that a handler failure cannot un-delete anything and stays a
Warning.
The handlers are now resolved while the scope is alive, before the cascade, and the list is
passed in. Deliberately NOT a guard around the resolution: these handlers are the side effects
that drop a partition's backing store when a partition-owning Space root is deleted, so skipping
them would trade a visible failure for a leaked database. A handler that itself resolves from the
dead scope still throws INSIDE Handle(), where the existing .Catch degrades it to the Warning the
contract promises.
Measured, same machine, same command, full 749-test assembly, DOTNET_PROCESSOR_COUNT=4:
main @ bf1ae3a 746 passed, 0 failed (9m31s)
this branch, before 745 passed, 1 failed (9m48s)
this branch, after 746 passed, 0 failed (9m43s)
An isolated run proves nothing here — RecursiveDeleteAuthoritativeTest alone passed 15/15 and the
7-test ~RecursiveDelete filter passed; only the full assembly reproduces it, because the trigger
is cross-mesh scope teardown between test classes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed at the root, and the before/after is a clean three-wayFound the actual throw site — my earlier mechanism in this thread was wrong and I want that on the record rather than quietly replaced. I said the cascade's hub activation resolved from a closing scope. It cannot: The stack from the local repro names it: It is step 5. The fix, and the option I rejectedHandlers are now resolved before the cascade, while the scope is alive, and the list is passed in. I deliberately did not guard the resolution, which is the one-line alternative. These handlers are the side effects that drop a partition's backing store when a partition-owning Space root is deleted — guarding would swap a visible failure for a silently leaked database. A handler that itself resolves from the dead scope still throws inside MeasurementSame machine, same command, full 749-test assembly,
The isolated runs stay worthless as evidence here and I am not citing them as such: Out of draft. |
A hub built under a parent gets its own Autofac
BeginLifetimeScopefromCreateServiceProvider, and nothing closed it. The call that would have sat inMessageHub's ShutDown phase, commented out.Two costs that look unrelated and are one bug:
IDisposablein the scope outlives the hub — Roslyn load contexts, Npgsql connections, native buffers — so its finalizer runs against a graph already torn down, the standard route to a SIGSEGV at process exit;Why the fix is not where the comment was
I put it back there first. It is wrong twice over:
The hub is a singleton IN the scope it would be destroying. Closing it from inside its own ShutDown phase pulls its logger — and the rest of that method's
finally— out from under it.A hub disposed ON ITS OWN never reaches a parent's teardown branch. That is the recycle, and the recycle is the case that actually leaks.
So the close lives on the parent side, in
HostedHubsCollection, subscribed to the child'sDisposalCompleted: strictly after the child is terminally down, and reached by both routes. Reactive throughout —Take(1),Catchso a faulted disposal still frees the scope (a failed teardown is when the handles are most likely still held),DefaultIfEmptyso a source that completes without emitting is settled from its known-terminal state rather than parked.Ownership is recorded, not inferred
MessageHubConfiguration.CreateServiceProvideris the only line that knows whether a scope was opened, so it says so. Every available proxy is wrong:parentAddress is not nullmisses a hub whose parent is a provider and not a hub, and "is it anAutofacServiceProvider" is equally true of the container the host owns and closes itself.One consequence, handled at the cause
SynchronizationStream's constructor resolved its logger before the gate that refuses on a disposing host. A host that has finished disposing no longer has a container, so that turned aHubDisposingExceptioninto Autofac's rawObjectDisposedException— same family, but naming the container instead of the hub, carrying no address for the caller to retry, and never reaching theShuttingDownclassification. Ordering is the fix; a catch around the resolve would only re-wrap the wrong fact.DeadStreamSafetyTestis what caught it.Evidence
HubDisposesItsServiceProviderTestpins both routes — the recycle and the parent teardown — because they run different code and a close wired onto one would leave the other leaking exactly as before.MeshWeaver.Messaging.Hub.Test: 281/281.🤖 Generated with Claude Code