Repository navigation
Refuse to build a dead SynchronizationStream; classify the refusal as transient (page dies on recycle) - #680
Merged
Merged
Conversation
… transient
A page subscribing while its hub recycles died with a terminal DeliveryFailure:
NullReferenceException at LayoutAreaHost..ctor. Reproduced 6 times in 26 rounds
of OverlaySelfHealInstanceRecycleTest (the sweep saw 4/8).
The window is STRUCTURAL, not a millisecond fluke, and it sits between two gates
that disagree:
- MessageHub.Dispose() freezes hosted-hub creation on its FIRST statement
(HostedHubsCollection.CloseCreation) — before the ShutdownRequest that moves
RunLevel off Started is even posted — and the freeze CASCADES to the whole
subtree, so a child is frozen while its own RunLevel still reads Started.
- MessageService.ScheduleNotify's intake gate only rejects from
DisposeHostedHubs on.
The entire Quiescing drain sits in that gap, so a disposing hub accepts requests
it cannot serve. Serving a layout area means constructing a
SynchronizationStream, which must own a hosted sub-hub. The ctor answered the
refusal by fabricating a corpse — isDisposed, completed store, Hub = null! —
documenting that consumers must use TryGetActiveHub. No consumer can honour
that: grep -rn TryGetActiveHub src matches only that file, ~96 sites dereference
the interface's NON-NULLABLE Hub, and ISynchronizationStream exposes no liveness
member to check. LayoutAreaHost..ctor:136 NRE'd, and it reached the subscriber
as a terminal DeliveryFailure (ErrorType.Unknown), which the client's sync
stream OnErrors — killing the keep-alive and resubscribe latch. Page dead.
Fix: the constructor REFUSES instead of fabricating an invariant-violating
object, throwing a typed transient HubDisposingException, and MessageService
classifies any handler exception that is or wraps one as ErrorType.ShuttingDown
— the same "ask again" answer #672 gave one layer down. One decision point,
covering every stream-creating path.
Rejected alternatives, for the record:
- Guard every dereference: untenable at ~96 sites, and node-native plugins
register their own reducers compiled live on the mesh, so the site set is
unbounded.
- Early-reject SubscribeRequest: racy and incomplete — it cannot cover the
ancestor-cascade freeze (RunLevel still Started), misses in-process stream
creation (Blazor circuit, WorkspaceStreams, MeshNodeStreamCache), and
duplicates the decision in two places.
Two existing facts made refusal the safe shape rather than a risky change: the
sibling path JsonSynchronizationStream.CreateExternalClient ALREADY threw here,
and Blazor already wraps BindStream in catch (ObjectDisposedException) with the
comment "old circuit's hub may already be disposing". So HubDisposingException
derives from ObjectDisposedException — existing teardown handling keeps working
and only the classification is new. The original "don't throw" rationale was an
IDE first-chance-break annoyance at a call site that already had a Catch
downstream.
The failure is logged at Debug, not Error — a recycle race is routine, and
paging operators for normal teardown is noise — and logs message TYPE + ID
rather than LogText(delivery), which was the core #608 hot-path serialization
storm.
The deterministic test holds the window open by parking one un-answered response
callback so Quiescing cannot drain, then WAITS until RunLevel has demonstrably
reached Quiescing before subscribing: verified state, no sleeps, no racing.
Reverting only the ctor makes it fail with exactly the production signature
(errorType=Unknown, TargetInvocationException -> NRE).
The test's own comment claimed "the wait exhausted all attempts". That is wrong
and is corrected here: the loop catches only TimeoutException, so the
DeliveryFailureException escaped on the FIRST probe — six 15 s windows cannot
fit inside a 2.1-2.6 s failure.
Before/after, DOTNET_PROCESSOR_COUNT=2, one test process at a time:
baseline 26 rounds -> 6 NREs at LayoutAreaHost..ctor
with fix 26 rounds -> 0
What this does NOT establish: that CI's rate for this test goes to zero. A
SECOND, independent failure mode is untouched — 2/18 baseline vs 2/26 with the
fix (~10% either way): step 3 times out because the NodeType never reaches Ok
with a usable build within 90 s (the parked-Error -> auto-un-park -> recompile
path). Different signature, no NRE, and it will keep this test red at roughly
that rate until investigated separately.
Verified: Messaging.Hub 107/107, Data 287/287, Layout 338/338, Graph 636/636,
Hosting.Monolith 402/402 (2 env-gated skips); full solution
Release/-warnaserror clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the recycle/disposal window behavior by preventing construction of a “dead” SynchronizationStream (with Hub = null) and ensuring that hub-disposal races are reported as a transient DeliveryFailure (ErrorType.ShuttingDown) so callers can retry after reactivation instead of faulting permanently.
Changes:
- Introduces
HubDisposingException(transient,ObjectDisposedException-shaped) and teachesMessageServiceto classify handler failures that are/contain it asErrorType.ShuttingDown. - Updates
SynchronizationStream/JsonSynchronizationStreamto refuse stream creation during hub disposal (throw) rather than fabricating a hub-less stream. - Adds/updates tests and documentation to pin the recycle-window subscribe repro and the failure-classification contract.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/HubDisposalFailureClassificationTest.cs | New test that pins transient classification for direct and reflection-wrapped hub-disposal exceptions. |
| test/MeshWeaver.Layout.Test/SubscribeDuringRecycleTest.cs | New end-to-end deterministic repro asserting recycle-window subscribe yields transient NACK, not NRE/terminal failure. |
| test/MeshWeaver.Hosting.Monolith.Test/OverlaySelfHealInstanceRecycleTest.cs | Clarifies the retry loop semantics and why terminal DeliveryFailureException must fail loudly. |
| test/MeshWeaver.Data.Test/DeadStreamSafetyTest.cs | Updates tests to match the new “refuse on disposing host” contract while keeping disposed-stream no-op coverage. |
| src/MeshWeaver.Messaging.Hub/MessageService.cs | Classifies HubDisposingException (including wrapped) as transient ErrorType.ShuttingDown and logs at Debug. |
| src/MeshWeaver.Messaging.Contract/HubDisposingException.cs | Adds the typed transient exception plus an inner-exception classifier helper. |
| src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md | Documents the “creation window” and the refuse/classify contract with pointers to the new tests. |
| src/MeshWeaver.Data/Serialization/SynchronizationStream.cs | Refuses to construct a stream when hosted-hub creation is frozen; removes “dead stream with null Hub” behavior. |
| src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs | Switches external-client creation failure from generic ObjectDisposedException to HubDisposingException. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+63
to
+76
| private static bool IsHubDisposal(Exception? exception, int depth) | ||
| { | ||
| if (depth > 16) | ||
| return false; | ||
| for (var e = exception; e is not null; e = e.InnerException) | ||
| { | ||
| if (e is HubDisposingException) | ||
| return true; | ||
| if (e is AggregateException agg | ||
| && agg.InnerExceptions.Any(inner => IsHubDisposal(inner, depth + 1))) | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
Comment on lines
+131
to
+132
| for (var i = 0; i < 200 && hub.RunLevel < MessageHubRunLevel.Quiescing; i++) | ||
| await Task.Delay(10); |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results (shard 5)1 139 tests ±0 1 138 ✅ ±0 6m 21s ⏱️ -4s Results for commit 54a49d6. ± Comparison against base commit 95f6ecc. This pull request removes 5 and adds 5 tests. Note that renamed tests count towards both. |
Contributor
Test Results 61 files ±0 61 suites ±0 33m 58s ⏱️ + 1m 23s Results for commit 54a49d6. ± Comparison against base commit 95f6ecc. This pull request removes 5 and adds 9 tests. Note that renamed tests count towards both. |
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.
A page subscribing while its hub recycles died with a terminal
DeliveryFailure—NullReferenceException at LayoutAreaHost..ctor. Reproduced 6 times in 26 rounds locally (a sweep saw 4/8). This is the same bug class as #672, one layer up: #672 taught the messaging layer to answer a recycle race with a retryable nack; the data/layout layer still hard-faulted.The window is structural, not a fluke
Two gates disagree:
MessageHub.Dispose()freezes hosted-hub creation on its first statement (HostedHubsCollection.CloseCreation) — before theShutdownRequestthat movesRunLeveloffStartedis even posted — and the freeze cascades to the whole subtree, so a child is frozen while its ownRunLevelstill readsStarted.MessageService.ScheduleNotify's intake gate only rejects fromDisposeHostedHubson.The entire
Quiescingdrain sits in that gap, so a disposing hub accepts requests it structurally cannot serve.Serving a layout area means constructing a
SynchronizationStream, which must own a hosted sub-hub. The constructor answered the refusal by fabricating a corpse —isDisposed, completed store,Hub = null!— documenting that consumers must useTryGetActiveHub.No consumer can honour that contract:
grep -rn TryGetActiveHub srcmatches only that file, ~96 sites dereference the interface's non-nullableHub, andISynchronizationStreamexposes no liveness member to check.LayoutAreaHost..ctor:136NRE'd, and it reached the subscriber as a terminalDeliveryFailure(ErrorType.Unknown) — which the client's sync streamOnErrors, killing the keep-alive and resubscribe latch. Page dead.Chosen fix: make the corpse unconstructible, then classify the refusal
The constructor refuses rather than fabricating an invariant-violating object, throwing a typed transient
HubDisposingException;MessageServiceclassifies any handler exception that is or wraps one asErrorType.ShuttingDown— the same "ask again" answer #672 gave one layer down. One decision point, covering every stream-creating path.Alternatives rejected, for the record:
SubscribeRequest— racy and incomplete: can't cover the ancestor-cascade freeze (RunLevel stillStarted), misses in-process stream creation (Blazor circuit,WorkspaceStreams,MeshNodeStreamCache), and duplicates the decision in two places.Two existing facts made refusal the safe shape rather than a risky change: the sibling path
JsonSynchronizationStream.CreateExternalClientalready threw here, and Blazor already wrapsBindStreamincatch (ObjectDisposedException)with the comment "old circuit's hub may already be disposing". SoHubDisposingException : ObjectDisposedException— existing teardown handling keeps working, and only the classification is new. The original "don't throw" rationale was an IDE first-chance-break annoyance at a call site that already had aCatchdownstream.Logged at Debug, not Error — a recycle race is routine and paging operators for normal teardown is noise — and logging message type + id, never
LogText(delivery), which was the core #608 hot-path serialization storm.Evidence
The new
SubscribeDuringRecycleTestholds the window open by parking one un-answered response callback soQuiescingcannot drain, then waits untilRunLevelhas demonstrably reachedQuiescingbefore subscribing — verified state, no sleeps, no racing. Reverting only the constructor makes it fail with exactly the production signature (errorType=Unknown,TargetInvocationException→ NRE).LayoutAreaHost..ctorAlso corrected: the test's own comment claimed "the wait exhausted all attempts". That is wrong — the loop catches only
TimeoutException, so theDeliveryFailureExceptionescaped on the first probe; six 15 s windows cannot fit in a 2.1–2.6 s failure.Messaging.Hub107/107 ·Data287/287 ·Layout338/338 ·Graph636/636 ·Hosting.Monolith402/402 (2 env-gated skips) · full solution-c Release -p:CIRun=true -warnaserrorclean.What this does not establish: that CI's rate for
OverlaySelfHealInstanceRecycleTestgoes to zero — see below.Found but not fixed
Okwith a usable build within 90 s (the parked-Error→ auto-un-park → recompile path). Different signature, no NRE, never reaches the probe loop. It will keep this test red at roughly that rate until investigated separately.Workspace.GetExternalClientSynchronizationStream'sLazy<…>(ExecutionAndPublication)caches the exception whenCreateExternalClientthrows —lazy.Valuethrows before the loop's removal code runs, so the poisoned entry never clears. Harmless today (that hub's workspace is dying anyway), pre-existing.HandleSubscribeRequestdispatchesSubscribeToClientinsideRunReadValidators(...).Subscribe(onNext). Every validator completes synchronously today, so a throw propagates into the handler and is caught — but if one ever completes off the hub turn, the exception escapes unobserved on a foreign thread. Pre-existing; the NRE had the same exposure.Documentation: new "The creation window" section in
HubDisposalModel.md(plus a stale test-name reference fixed).No What's New entry — the user-visible effect is a page that no longer dies during a recycle; there is no new behaviour to describe.
🤖 Generated with Claude Code