Repository navigation
fix(hub): a hosted hub at ShutDown is retired and replaced, never handed out (#5136) - #5199
Conversation
…ded out (#5136) Root R2 of Doc/Architecture/DisposedScopeAndDyingHubs. GetHubWithOutcome answered a registry hit as Available unconditionally, while a hub at RunLevel >= ShutDown can serve nothing and leaves the registry only when its own removal registrant runs, several statements into that phase; a wedged teardown never got there, so one portal/nodeops hub was handed to four callers over 55 minutes on a serving pod. An Always lookup now retires such a hub (value-matched, so a concurrently-registered successor is untouched), mints a successor through the ordinary single-flight creation, and keeps the corpse in the collection's disposal join until its own DisposalCompleted - its scope is a child of the owner's. The bound is ShutDown, not IsDisposing: below it the hub is still draining accepted work and the intake's transient NACK is the documented answer; a successor there would overlap accepted writes. Retire-and-replace needs every removal a teardown makes to be value-matched. The third and last one lands here: PodHubGrain.Detach leaves the claim alone when the silo still holds a live local route for the address, because RegisterStream's disposal removes its route before it releases, so that route can only be a successor's. Two more key-only removals found in the sweep are gone: MonolithRoutingService.UnregisterStream(address) (armed on every route-created hub in the same registrant walk) and the podHubClaimSettled seam. Controls: AHubAtShutDownIsNotHandedOutTest parks the ShutDown turn inside a reactive dispose action and asserts the corpse is still registered - total 2 / failed 0 fixed, total 2 / failed 2 with the unconditional return restored. PodHubStaleReleaseTest on the real two-silo cluster - total 2 / failed 0 fixed, total 2 / failed 1 with the route check removed (the successor refused as RELEASED). Whole Messaging.Hub.Test 501/0, pod-hub suites 53/0, guards + link integrity 347/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retirement has unresolved disposal and lookup races, and test cleanup requires hardening.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
This PR fixes hosted-hub reuse during shutdown by retiring hubs, creating successors, and making cleanup successor-safe.
Changes:
- Added retire-and-replace lifecycle handling.
- Hardened route and claim cleanup.
- Added regression tests and updated documentation.
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/AHubAtShutDownIsNotHandedOutTest.cs |
Tests shutdown retirement and disposal joining. |
test/MeshWeaver.Hosting.Orleans.Test/PodHubStaleReleaseTest.cs |
Tests stale and valid pod releases. |
src/MeshWeaver.Messaging.Hub/HostedHubsCollection.cs |
Retires shutdown hubs and joins teardown. |
src/MeshWeaver.Hosting.Orleans/PodHubGrain.cs |
Prevents stale releases from affecting successors. |
src/MeshWeaver.Hosting.Monolith/MonolithRoutingService.cs |
Removes unsafe key-only cleanup. |
src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md |
Documents the shutdown creation window. |
src/MeshWeaver.Documentation/Data/Architecture/DisposedScopeAndDyingHubs.md |
Records the R2 fix and lifecycle details. |
src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs |
Makes claim cleanup value-matched. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!messageHubs.TryRemove(new KeyValuePair<Address, IMessageHub>(address, corpse))) | ||
| return; | ||
|
|
||
| retiring[corpse] = Unit.Default; |
There was a problem hiding this comment.
Real, and fixed in the next push — thank you. The gap was remove-then-add: a lookup that had already taken the corpse out of messageHubs but not yet put it into retiring left it in neither of the owner's two snapshots.
The correction is an ordering, not a lock. RetireCorpse now goes into retiring first, out of messageHubs second (TryAdd, then the value-matched TryRemove), and DisposeHubsReactive keeps snapshotting the registry first, retiring second. With those two orders opposed, the corpse is in at least one snapshot at every instant: a registry snapshot taken before the removal still holds it; one taken after was taken after the add that preceded that removal, so the retiring snapshot that follows holds it. Both dictionaries publish under their own locks, so the add is visible to any thread that already observed the removal. A hub caught in both snapshots is joined once, as retired (ImmutableHashSet by reference), and the late-construction leg now diffs against the registry snapshot rather than the live-legs subset so a hub retired between the snapshots is not re-disposed as "late". The retirement that races the IsDisposing check itself (check passes, owner flips, then retire) is covered by the same argument — the corpse was in the registry snapshot — and its would-be successor is refused by the creation Lazy's own IsDisposing re-check, exactly as before.
Both orders are documented at the two sites as load-bearing.
| if (!messageHubs.TryRemove(new KeyValuePair<Address, IMessageHub>(address, corpse))) | ||
| return; | ||
|
|
||
| retiring[corpse] = Unit.Default; |
There was a problem hiding this comment.
Considered, and deliberately not done — the window you describe is the ordinary recycle window, not a new one.
Today, without this change, every recycle produces exactly the sequence corpse → (removal registrant fires) → Absent → (next Always lookup) → successor, and the Absent stretch is unbounded: a recycled node nobody re-activates stays absent until something asks for it. A Never probe in that stretch answers Absent now, and is answered Absent on main today. Retirement moves the removal a few statements earlier — from the registrant walk to the retiring lookup — and the successor's construction that follows is the same single-flight Build that any Always creation runs. So the Never-miss answer during the successor's Build is the answer the platform has always given between a removal and the next activation; nothing here converts a transient refusal into a terminal one that was not already terminal in that window on the ordinary path.
An address-indexed retired lookup would make Never probes deliver into a hub at ShutDown for the duration of the successor's Build — intake refuses, a ShuttingDown NACK goes back through the parent — which is a different established answer for a window that is milliseconds long and already exists. Two answers for one state is more to reason about than one, so the lookup keeps the single semantics: Never sees the registry, and the registry holds one live hub per address.
The sync/… addresses RouteStreamMessage probes are per stream instance and never re-created under a predecessor's address, so the retire path is not reachable for them at all.
| var ct = TestContext.Current.CancellationToken; | ||
|
|
||
| // 1. The predecessor claims the address. | ||
| var predecessor = routes.RegisterStream(address, (delivery, _) => Observable.Return(delivery)); |
There was a problem hiding this comment.
Done in the next push — both registrations are using var now (line 64 and line 120), with the explicit early Dispose() kept; RegisterStream's disposable is Disposable.Create, so the second call is a no-op. The comment on the first one says why: the fixture is shared across the class, and a registration a failed assertion left behind would keep its local route and its pod-hub claim alive into the next test.
…stry RetireCorpse adds to retiring BEFORE the value-matched TryRemove from the registry, and DisposeHubsReactive snapshots the registry BEFORE retiring, so a retirement racing the owner's teardown lands in at least one snapshot whatever the interleaving; a hub in both is joined once, as retired, and the late-construction leg diffs against the registry snapshot. The two fixture registrations in PodHubStaleReleaseTest are using-scoped so a failed assertion cannot leave a route and a claim behind in the shared cluster. Messaging.Hub.Test 501/0 and PodHubStaleReleaseTest 2/0 on the rebuilt dlls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Root R2 of
Doc/Architecture/DisposedScopeAndDyingHubs— the one of the five with no fix and a fix that can be written today (status table: #3593 (comment)).Closes #5136.
The root
HostedHubsCollection.GetHubWithOutcomeanswered a registry hit asAvailable, unconditionally. A hosted hub leaves that registry only when its own removal registrant runs — several statements into the ShutDown phase, after theRunLevelflip,CancelCallbacksand the reactive dispose actions — while from the flip on it can serve nothing: intake refuses every delivery and a directObserve(...)faults synchronously withObjectDisposedException. A teardown that wedges anywhere in between (a registered cleanup blocking the turn — #4883's shape) never reaches the removal, so the window was unbounded. Measured: oneportal/nodeops-…hub handed to four webhook deliveries over 55 minutes on a pod serving everything else, each a 500.The fix — retire-and-replace, once every removal is value-matched
The page named this as the self-healing answer and gated it on three removals becoming value-matched so a predecessor's teardown cannot erase its successor. Two had landed (#5191 for the registry, #5159 for the local route); the third — the pod-hub cluster claim — had not.
GetHubWithOutcome: anAlwayslookup that finds a registered hub atRunLevel >= ShutDownretires it (value-matchedTryRemove(KeyValuePair), so a successor a concurrent lookup registered is untouched) and falls through to the ordinary single-flight creation. The corpse moves to a reference-keyedretiringset the collection still OWNS:Hubsenumerates it, the stall detector sees it, andDisposeHubsReactivejoins on it — its Autofac scope is a child of the owner's, and an owner that finished ahead of it would close that scope under a registrant walk still running (R1's straggler class, manufactured locally). It leaves on its ownDisposalCompleted. OneWarningline,[HOSTED-RETIRE], names the address and phase; the ordinary path never reaches it.ShutDown, notIsDisposing, deliberately. BelowShutDownthe hub is still draining accepted work and the intake's transientShuttingDownNACK is the documented answer (HubDisposalModel, the creation window); a successor minted there would put two activations on one address with accepted writes still in flight on the first. AtShutDownnothing accepted remains.Neverprobes — the router's — still find the corpse and are refused with that NACK, right for a message and wrong for a caller holding the reference. While the collection itself is disposing no successor can be minted, so the corpse stays the honest (attributable) answer — refusing withnullis still ruled out for the 25 null-forgiving call sites the thread measured.PodHubGrain.Detachleaves the claim alone when the silo still holds a live local route for the address.RegisterStream's disposal removes its route before it releases the claim, so a live route at that point can only be a successor's; the grain is not[Reentrant], so the read is ordered against the successor'sAttachand everyDeliver. Honouring a stale release would have stamped the terminalReleasedtombstone on a live hub — ten minutes of refusals with the one verdict the owner-side eviction acts on, and the pin cut from indefinite.MonolithRoutingService.UnregisterStream(address), armed on every route-created hub on top of the hub's own value-matched registration and running in the same registrant walk (would have taken the successor's address dark on the Monolith), and thepodHubClaimSettledtest seam.Deliberately left where it is: the wedge itself (#4883). This lets callers past a wedged hub and is the first line to name it by address; it does not free the turn.
Controls — both sides, measured
Run through the xUnit v3 host directly (
dotnet test --filterruns nothing on this SDK); on each side the hub dll's mtime was read after the source's.AHubAtShutDownIsNotHandedOutTest(Messaging.Hub.Test) — parks the ShutDown turn inside a reactive dispose action, asserts the corpse is still registered, then measures the lookup and the owner's joinQuicknegative window)PodHubStaleReleaseTest(Hosting.Orleans.Test, real two-silo cluster) — a stale release must leave a live successor served and pinned atMaxValue; a release with no live route must still tombstoneDetach: total 2, failed 1 — the successor's delivery threwPodHubNotHereException … was RELEASED by its owner; the honest-release case passed on both sides, as it mustWhole
MeshWeaver.Messaging.Hub.Testproject: total 501, failed 0 (215.8 s). Neighbouring pod-hub / cross-silo / silo-shutdown suites: total 53, failed 0. Guards (*Guardincl.HandWovenGateRatchetGuard,NoLiteralNulInSourceGuard,TestTimeoutLiteralRatchetGuard, the bridge guards) +DocumentationLinkIntegrityTest: total 347, failed 0, each named guardPassedon aMeshWeaver.Documentation.dllrebuilt after both page edits.Build verification
dotnet build -c Release -warnaserror, one project per invocation, each0 Warning(s) 0 Error(s):MeshWeaver.Messaging.Hub,Connection.Orleans,Hosting.Orleans,Hosting.Monolith,Hosting,Hosting.AspNetCore,Documentation, and theMessaging.Hub.Test,Hosting.Orleans.Test,Documentation.Testprojects.Docs
Doc/Architecture/DisposedScopeAndDyingHubs— R2 rewritten from "root identified, not fixed" to the fix, theShutDownbound and why, the three value-matched removals, and what stays on #4883; the settled "Addunreferenced" lead is recorded as settled.Doc/Architecture/HubDisposalModel— a third creation-window section for the hub atShutDownthat is still registered.Deploy
Framework code, no node content — nothing to recycle; a roll carries it. Pairs-with: none — no public type or member leaves
src/(UnregisterStreamwas private).🤖 Generated with Claude Code