Repository navigation
A hub-owned watcher stops at the first instant of teardown, not in ShutDown (#3026) - #3072
Merged
meshweaver-cloud[bot] merged 2 commits intoSep 2, 2026
Merged
Conversation
…utDown (#3026) The sources / compile / release / adopted-stamp watchers of a per-NodeType hub were disposed by RegisterForDisposal in the hub's LAST phase, after Quiescing had waited its whole 2 s budget for the very @@-include GetDataRequests the sources watcher kept issuing from the tearing-down hub — the 22 caught `SourceIncludeUnavailableException ---> ObjectDisposedException` faults per FutuRe.Test shard, one per fixture teardown, each exactly one quiesce budget after DISPOSE_INVOKED. A re-establish evaluated the source factory on a bare timer tick, so a factory throwing on a disposed hub was an unhandled exception on a scheduler thread (exit 139). - IMessageHub.ShuttingDown: IsShuttingDown as a replaying source, raised at own Dispose() and at the ancestor's CloseCreation cascade. - ActivityControlPlaneExtensions.SubscribeHubWatcher(hub, …): stops at ShuttingDown (releasing in-flight cross-hub reads so Quiescing drains at once), gates every delivery on IsShuttingDown, is inert on a shutting-down hub, and evaluates the factory under Observable.Defer. All hub-owned watchers use it. - Docs (HubDisposalModel, ActivityControlPlane), What's New (Fix), three tests that fail before the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new Hosting test uses an invalid C# call pattern (named arg followed by positional arg) that will not compile, and the new teardown gating relies on a non-thread-safe disposed flag that can race across scheduler threads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses #3026 by ensuring hub-owned watchers stop at the first instant of teardown (including ancestor-triggered shutdown), preventing late cross-hub callbacks during disposal that were causing 2s quiesce timeouts and intermittent exit-139 host crashes.
Changes:
- Introduces
IMessageHub.ShuttingDown(a replaying one-shotIObservable<Unit>) and wires it intoMessageHubteardown entry points. - Adds
ActivityControlPlaneExtensions.SubscribeHubWatcher(...)to bind watcher lifetime tohub.ShuttingDown, gate deliveries onhub.IsShuttingDown, and evaluate watcher factories viaObservable.Defer. - Updates key hub-owned watchers to use
SubscribeHubWatcher, and adds tests + documentation/What’s New to pin the new contract.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/ShuttingDownSignalTest.cs | Adds a deterministic test pinning ShuttingDown semantics across a hosted subtree. |
| test/MeshWeaver.Hosting.Test/HubWatcherStopsAtTeardownStartTest.cs | Adds contract tests for watcher shutdown-at-teardown-start behavior and re-establish factory fault handling. |
| test/MeshWeaver.Graph.Test/SourcesWatcherStopsAtTeardownTest.cs | Adds an integration repro ensuring the sources watcher releases an in-flight @@ include read on teardown. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Implements the ShuttingDown source and raises it from both Dispose() and hosted-hub creation freeze cascades. |
| src/MeshWeaver.Messaging.Hub/IMessageHub.cs | Extends the hub contract with ShuttingDown and documents its intended watcher-lifetime usage. |
| src/MeshWeaver.Mesh.Contract/ActivityControlPlaneExtensions.cs | Adds SubscribeHubWatcher and updates existing watchers to use the hub-owned form. |
| src/MeshWeaver.Graph/Configuration/BuildNodeType.cs | Switches the build-claim arbiter watcher to SubscribeHubWatcher. |
| src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs | Switches NodeType compile/sources/release/adopted-stamp watchers to SubscribeHubWatcher. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-background-compile-watcher-stops-when-its-host-stops.md | Adds a user-facing “Fix” entry describing the watcher teardown behavior change. |
| src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md | Documents the new “first instant of teardown” signal and watcher rule; notes the quiesce-verdict blind spot. |
| src/MeshWeaver.Documentation/Data/Architecture/ActivityControlPlane.md | Updates guidance to reference SubscribeHubWatcher and the ShuttingDown lifetime contract. |
Review details
Suppressed comments (2)
test/MeshWeaver.Hosting.Test/HubWatcherStopsAtTeardownStartTest.cs:124
- This call mixes a named argument (
logger: null) followed by a positional argument ("test watcher"), which does not compile in C# (positional arguments cannot follow named arguments). Name the final argument as well.
using var watcher = ActivityControlPlaneExtensions.SubscribeHubWatcher<int>(
child, () => source, _ => delivered++, logger: null, "test watcher");
test/MeshWeaver.Hosting.Test/HubWatcherStopsAtTeardownStartTest.cs:164
- This call mixes a named argument (
logger: null) followed by a positional argument ("test watcher"), which does not compile in C# (positional arguments cannot follow named arguments). Name the final argument as well.
using var watcher = ActivityControlPlaneExtensions.SubscribeHubWatcher<int>(
hub, () => { factoryCalls++; return source; }, _ => { }, logger: null, "test watcher");
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+81
to
+82
| using var watcher = ActivityControlPlaneExtensions.SubscribeHubWatcher<int>( | ||
| hub, () => source, _ => delivered++, logger: null, "test watcher"); |
Comment on lines
+429
to
+436
| void Stop() | ||
| { | ||
| disposed = true; | ||
| // Drop the pending re-establish FIRST — that is the TimerQueue entry rooting | ||
| // this whole closure graph (#991); `serial` only holds the live subscription. | ||
| pendingReEstablish.Dispose(); | ||
| serial.Dispose(); | ||
| } |
Comment on lines
+1611
to
+1620
| try | ||
| { | ||
| shuttingDown.OnNext(Unit.Default); | ||
| shuttingDown.OnCompleted(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| TryLog(LogLevel.Warning, ex, | ||
| "[SHUTTING-DOWN] {Address}: a ShuttingDown subscriber threw — teardown proceeds", Address); | ||
| } |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…eout-literal ratchet stays at 386 TestTimeoutLiteralRatchetGuard.TheHandWrittenTimeoutCountOnlyEverGoesDown counted the two new `TimeSpan.FromSeconds(30)` bounds on the release polls (388 > baseline 386). The bound a parked worker is released under is the same convergence budget every other wait in these suites uses, so it is named, not guessed. Recount on the tree: 386. Both test projects rebuild clean under -c Release -warnaserror --no-incremental. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 2, 2026
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.
Fixes #3026.
What was wrong
A per-NodeType hub's watchers — sources /
IsDirty, compile, release-request, adopted-stamp — are installed throughSubscribeWithReEstablishand handed tohub.RegisterForDisposal. That disposes them in the hub's ShutDown phase, the LAST one: after Quiescing has waited up to its whole 2 s budget for pending callbacks and after the hosted subtree has been joined. And a hub is part of a shutdown even earlier than its ownDispose(): an ancestor'sDispose()freezes the subtree synchronously (CloseCreation, #613) and reaches the hub only in the ancestor's DisposeHostedHubs phase.Through that whole window the sources watcher kept working. Resolving the
@@-include closure issues cross-hubGetDataRequests from the hub that is tearing down — those are exactly the pending callbacks its Quiescing then waited 2 s for, andCancelCallbacksfinally errored them withObjectDisposedException: Hub FutuRe/LocalAnalysis was disposed before the response arrived (GetDataRequest, target FutuRe/GroupAnalysis/Source/ExternalDependencies). That is the[FAULT]the issue quotes, 2 052 ms afterDISPOSE_INVOKED, 22 times in one shard — one per fixture teardown, each exactly one quiesce budget late. A re-establish of the same watcher evaluates its source factory on a bareObservable.Timertick; a factory that throws synchronously there (a hub whose scope is gone) is an unhandled exception on a scheduler thread — the exit-139 host kill.What changed
IMessageHub.ShuttingDown—IsShuttingDownas a source: fires once at the FIRST instant the hub is part of a shutdown (ownDispose()or the ancestor cascade inCloseHostedHubCreation), then completes; replays to a late subscriber.ActivityControlPlaneExtensions.SubscribeHubWatcher(hub, …)— the hub-owned form ofSubscribeWithReEstablish(a distinct name, not an overload: MeshWeaver.Plugins carries a parameterlesscrefto the generic method, which a second overload would make ambiguous under its warnings-as-errors). It (1) stops atShuttingDown— live subscription and pending re-establish — so an in-flight cross-hub read is unsubscribed (its pending callback leaves the response registry) and Quiescing drains at once; (2) gates every delivery onhub.IsShuttingDown; (3) is inert on a hub already shutting down; (4) evaluates the source factory underObservable.Defer, so a synchronous factory throw on a re-establish is a stream fault the classifier owns, not a throw on the timer thread. All seven hub-owned watchers now use it (compile, release-request, sources, adopted-stamp, control-plane, submission, build-claim arbiter).HubDisposalModel.md(new section "The first instant of teardown:ShuttingDown", the rule for hub-owned watchers),ActivityControlPlane.md. What's New entry (Category: Fix).No
catchanywhere on the path, no re-run: the fix is the lifetime and the gate, as the issue directs.Deliberately left out — the fixture verdict's blind spot
The issue also asks that the fixture's
teardown cleanverdict not read clean while a hosted hub timed out. The cause is real: a hosted hub removes itself from its parent's registry in its own ShutDown phase, soAnyHubQuiescingTimedOut()walks an empty registry by the time the test base asks. I implemented the fix (the collection keeps each departing child's verdict) and ranMeshWeaver.Graph.Testwith it: it immediately reddened two unrelated classes on pre-existing hosted-hub timeouts —DanglingNodeTypeUpdateTest(acache/…hub with aSubscribeRequest@TestData/dnt…pending 2002 ms: routed, "no handler was ever entered" — a genuine leaked subscribe to an error-overlay node) andEventContinuationHandlerTest(acache/…hub whose callback drained exactly at the 2 s hand-off — the detector racing its own budget). Every other suite in the fleet would need the same sweep before that verdict can land, so it is split out rather than holding this crash fix hostage; the blind spot is documented inHubDisposalModel.mdunder the Quiescing phase.Tests (each fails before the fix)
ShuttingDownSignalTest(Messaging.Hub.Test): the signal fires synchronously insideDispose()on the whole subtree while the root's action block is deliberately parked (so no disposal phase can have run); a child'sIsDisposingis still false and itsRunLevelstillStarted; a late subscriber is told at once.HubWatcherStopsAtTeardownStartTest(Hosting.Test): own dispose and ancestor dispose stop the watcher before any phase runs (source.HasObserversfalse, nothing delivered); installing on a shutting-down hub is inert; a factory throw on a re-establish is classified, not thrown.SourcesWatcherStopsAtTeardownTest(Graph.Test): a real mesh, a NodeType whose one source has an@@include pointing at an instance of a NodeType whose hub never opens its init gate — so the watcher's include read is deterministically in flight — then the NodeType hub is disposed:QuiescingTimedOutis false and no "closure could not be established" warning is logged.Verification
dotnet build … -c Release -warnaserror:MeshWeaver.Messaging.Hub.Test,MeshWeaver.Hosting.Test,MeshWeaver.Graph.Test(closure covers Messaging.Hub, Mesh.Contract, Compiler.Pipeline, Graph, Hosting) — 0 warnings, 0 errors.DOTNET_PROCESSOR_COUNT=4,.trx): Messaging.Hub.Test 343/343, Hosting.Test 294/294, Graph.Test 1157/1159 — the 2 reds were the departed-verdict exposure described above and disappeared with its removal; the new integration test passed.MeshWeaver.FutuRe.Test(the observation site, in MeshWeaver.Plugins) was NOT run locally — stopped on the maintainer's quota directive before the Plugins build; CI on that repo is the verification.🤖 Generated with Claude Code
https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj