Repository navigation
refactor(faults): one FaultClassifier for cancellation, teardown and transient faults - #7117
Merged
Merged
Conversation
…transient faults
About twelve cancellation/teardown predicates and eight transient-fault
predicates answered slices of one question ("why did this end in an
exception?") and disagreed. FaultClassifier.Classify(exception, context) in
MeshWeaver.Messaging.Contract is now the only implementation:
CooperativeCancel / Timeout / ScopeTeardown / HubTeardown / Transient(Layer)
/ Fault, walked over the exception GRAPH (ExceptionChain-style, bounded,
cycle-safe). Every old predicate is a forwarder; public names used by
Plugins keep their signatures.
Each disagreement is decided and pinned by a test that fails on the copy that
was wrong (11 facts, all red on origin/main). Doc page:
Doc/Architecture/FaultClassification.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Wrapped and shared exception graphs can produce incorrect cancellation and recovery decisions, while mixed aggregates can bypass the provider-stall retry exclusion.
5 open issues
What changed in this PR
This PR centralizes fault classification in the messaging contract so cancellation, teardown and retry decisions share one implementation across MeshWeaver’s framework layers.
Changes:
- Adds
FaultClassifier, classification records, open vocabularies and an interface for higher-layer exceptions. - Replaces duplicated predicates with forwarders and unifies the two Orleans routing predicates.
- Adds regression tests and documents the intended behavior changes.
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/FaultClassifierTest.cs | Adds classifier truth-table and graph tests. |
| test/MeshWeaver.Messaging.Hub.Test/FailStartupTeardownClassificationTest.cs | Tests disposal in a later aggregate branch. |
| test/MeshWeaver.Layout.Test/FaultClassifierForwardersTest.cs | Tests cancellation, view retry and storage verdicts. |
| test/MeshWeaver.Hosting.Test/FaultClassifierHostingForwardersTest.cs | Tests storage-walk cancellation and owner failures. |
| test/MeshWeaver.Hosting.Orleans.TestBase/OrleansShutdownRaceSuppressor.cs | Delegates disposed-container detection. |
| test/MeshWeaver.Hosting.Orleans.Test/FaultClassifierOrleansForwardersTest.cs | Tests routing and activation teardown verdicts. |
| test/MeshWeaver.Fixture/TestTeardownExceptionSuppressor.cs | Uses shared disposed-container detection. |
| test/MeshWeaver.Fixture/TeardownStragglerCapturer.cs | Uses shared Autofac teardown detection. |
| src/MeshWeaver.Messaging.Hub/ScopeTeardown.cs | Classifies teardown with the caller’s scope probe. |
| src/MeshWeaver.Messaging.Hub/MessageHub.cs | Delegates startup disposal classification. |
| src/MeshWeaver.Messaging.Contract/InfrastructureFault.cs | Reads typed transient branch verdicts. |
| src/MeshWeaver.Messaging.Contract/HubDisposingException.cs | Delegates disposed-container detection. |
| src/MeshWeaver.Messaging.Contract/FaultClassifier.cs | Introduces shared classification and graph traversal. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Uses contextual teardown classification. |
| src/MeshWeaver.Mesh.Contract/CancellationClassifier.cs | Forwards cooperative-cancellation detection. |
| src/MeshWeaver.Mesh.Contract/ActivityControlPlaneExtensions.cs | Delegates address-specific hub disposal detection. |
| src/MeshWeaver.Layout/AreaErrorClassifier.cs | Uses shared retry and storage classifications. |
| src/MeshWeaver.Hosting/StandingWatchRecovery.cs | Uses typed transient verdicts for recovery. |
| src/MeshWeaver.Hosting/Persistence/Query/StorageAdapterMeshQueryProvider.cs | Distinguishes timeout cancellation from teardown. |
| src/MeshWeaver.Hosting/MeshNodeStreamCache.cs | Replaces owner-failure predicates and marker copies. |
| src/MeshWeaver.Hosting.Orleans/RoutingGrain.cs | Forwards to the routing service’s predicate. |
| src/MeshWeaver.Hosting.Orleans/MessageHubGrain.cs | Delegates activation teardown classification. |
| src/MeshWeaver.Documentation/Data/Architecture/FaultClassification.md | Documents classifications, forwarders and decisions. |
| src/MeshWeaver.Data/Serialization/SynchronizationStream.cs | Uses shared disposal and transient classifications. |
| src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs | Makes owner-refusal exceptions self-classifying. |
| src/MeshWeaver.Data/DataExtensions.cs | Delegates owner teardown detection. |
| src/MeshWeaver.Data.Contract/StorageFaults.cs | Uses typed store verdicts for connection failures. |
| src/MeshWeaver.Data.Contract/QueryProviderStalledException.cs | Declares the provider-stall classification. |
| src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs | Shares a graph-aware routing-failure predicate. |
Review effort: Balanced
Contributor
Test Results 26 files 26 suites 1h 10m 52s ⏱️ Results for commit 2ec1ab6. ♻️ This comment has been updated with latest results. |
…ds to the typed cause it wraps Answers the five review findings on #7117: - a node shared by two aggregate branches contributed to the first branch only, so IsCancellation depended on branch order; the walk now memoizes per (node, head-ness, pending text verdict) and a shared node counts in both; - an aggregate under a wrapping fault reset its branches to outermost, so a fault around a bridged cancellation read as a cancellation; - a non-transient DbException was a store outage when ANY frame below it was a socket fault; now EVERY branch below must reach one; - a message match on a wrapper hid the typed transient cause it wrapped; a text verdict now only stands in until a typed frame below decides; - a view retried a provider stall beside a socket or timeout sibling; the stall is now refused in any branch. Also resolves the merge with main's #7066 owner-classified store fault: a MeshNodeStreamException states its own verdict (Transient/OwnerStore), which the standing watch accepts. Lists Fault Classification in the topic map and drops a literal 30 s Fact timeout from a synchronous test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ph, a nested connect SQLSTATE, or its own text - EveryBranchReaches memoizes each node's answer by reference before charging the budget again, so a shared aggregate graph (9 distinct nodes, 511 paths) is judged, not cut off. - The driver probe also accepts a nested driver frame carrying a connection-class SQLSTATE, so a transient no-SQLSTATE DbException over 57P03 is Store, not contention. - An unflagged DbException that decides nothing by type reaches the message markers (ByMessage), as the stream cache's predicate did before the refactor, so an 'activation failed' wrapper around 'connection refused' is not negative-cached. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r fills an aggregate sibling; a timeout over a stall is the stall Answers the three review findings on FaultClassifier: - the walk's memo keyed the exception by value, so two value-equal driver exceptions (57P03 and 42P01) shared one verdict; it is now keyed by reference; - an outer text match was copied into every aggregate child, so a defect beside a socket fault became a second transient branch; it now stands in only when no child decides; - a typed timeout wrapping a stalled provider hid the stall from IsStorageUnavailable and let IsTransientHubFailure retry it; the stall below now decides. Each has a regression in FaultClassifierTest that fails without the fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved classification and diagnostic regressions, together with unverified downstream effects of the cross-layer behavior changes, require fixes and human review.
4 open issues
3 resolved since last review
Review effort: Balanced
…driver over a defect is not contention; suppressors keep the every-branch rule - FaultClassifier: a hub teardown recognised only by its 'is shutting down' banner now reports ByMessage (both the own-hub HubTeardown verdict and the other-hub Transient/Transport leaf), so EveryBranchTypedTransientIn never accepts text-only evidence; typed teardowns are unchanged. - A provider-flagged DbException over a nested aggregate is StoreContention only when every branch below is a transient cause; a socket reset beside a JsonException is a Fault again, so StandingWatchRecovery does not re-open on it (the removed walk's behaviour). - EveryBranchIsDisposedContainer / IsDisposedContainerFrame: the teardown suppressors keep the every-branch rule through wrappers, and the first-chance capturer admits the frame only. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…sted flagged driver, are judged by what they wrap Two regressions against main's walks, both pinned: - A typed TimeoutException over a SocketException, or over a driver fault with a connection-class SQLSTATE, decided Timeout and stopped, so InfrastructureFault.IsTransient and StandingWatchRecovery.IsTransient answered false for a recoverable outage. The timeout now stands in only as a fallback when typed store or network evidence lies beneath it, as it already did over a stalled provider. A cause-free timeout is unchanged. - A flagged driver nested in a flagged driver over Aggregate(socket, JsonException) was accepted by its flag, so the outer became StoreContention. A nested flagged driver now counts only when it encloses no aggregate; otherwise its branches are walked. Negative control: with the classifier restored, the three new facts fail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t; driver probes read past a caused timeout/IO frame
- WrapsTypedStoreOrNetworkCause accepts a transient OwnerStore fault, so
Timeout(MeshNodeStreamException{Transient}) is that outage and StandingWatchRecovery re-opens
on it, as the removed walk did.
- IsTransportFrame: inside a driver exception a timeout or IO frame is the evidence only when it
has no cause; with one, EveryBranchReaches walks into it. A driver (flagged or not) over
Timeout(Aggregate(socket, JsonException)) is a fault again, while a cause-free driver timeout
and IO(socket) stay a transient store fault. Used by both driver probes.
- Two regressions in FaultClassifierHostingForwardersTest, both red before the fix (found True /
found False) and green after; FaultClassifierTest 43/43 and every classifier forwarder suite in
Hosting, Messaging.Hub and Layout green.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…te it encloses (#7117 review) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
added a commit
that referenced
this pull request
Oct 25, 2026
…lock-order deadlock behind the case-rename stall) (#7149) * fix(query): the query merge never calls its consumer under its gate (lock-order deadlock behind #7117) CaseOnlyRenameKeepsATargetEditedBeforeItsCleanupTest stalled on main (run 40129840536) and on #7117 (run 40136105238): leg 1's staging creates never returned from stage `write`. Reproduced locally (DOTNET_PROCESSOR_COUNT=2, cold JIT); the stacks were a lock-order deadlock: - thread B delivered a provider's Initial into MeshQuery's merge and, holding the merge's gate, called the observer; a create's CreatableTypesCreationValidator ran on synchronously into the storage write, whose change delivery blocked on the storage feed's Observable.Merge gate; - thread A held that feed gate delivering a change into a live query over the same store and blocked on the merge's gate. GateFreeEmitter: the merge enqueues frames under its gate and drains them after leaving it; one thread drains, any other returns at once. Applied to the multi-provider merge, the single-provider path and CombineAnswers. AQueryMergeNeverHoldsItsGateWhileItsConsumerRunsTest pins it deterministically (red without the fix: the other thread's delivery stays blocked for the 5 s bound). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(query): the snapshot fan-out combines its providers under its own gate, not CombineLatest (review of #7149) CombineLatest calls its subscriber under its own serializing lock, so the snapshot Query and Autocomplete still ran their consumer under a lock a second provider's delivery needed, even with the merge gate released: the same lock-order deadlock. CombineAnswers now keeps each provider's latest answer under its gate and emits through GateFreeEmitter. ASnapshotFanOutNeverHoldsALockWhileItsConsumerRunsTest pins both surfaces; with CombineLatest both cases are red (the delivery stays blocked 5 s). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(query): a leg factory that throws no longer leaks the providers already subscribed (review of #7149) The snapshot fan-out built each provider's leg inside its subscription setup. A factory that threw left the setup before the composite holding the earlier legs' subscriptions was returned, so those stayed subscribed with nothing to dispose them. Each leg is now deferred: a factory fault reaches the leg's error handler, fails the fan-out, and the composite is returned and disposed. Regression test fails without the fix (early provider still subscribed after the fault), on both the query and autocomplete surfaces. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
added a commit
that referenced
this pull request
Oct 25, 2026
The disposed-container match now lives once, in FaultClassifier (#7117), keyed on MeshServiceProvider's object name and phrase; HubDisposingException forwards to it. Main's new FaultClassifier tests are ported from the Autofac exception shape to the container's own. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
What
About twelve cancellation/teardown predicates and eight transient-fault predicates each answered part of one question: "why did this end in an exception?" They disagreed with each other.
FaultClassifier.Classify(exception, TeardownContext?)inMeshWeaver.Messaging.Contractis now the only implementation. It returns aFaultClassification:Kind∈ {CooperativeCancel,Timeout,ScopeTeardown,HubTeardown,Transient,Fault}, plusLayer∈ {Network,Store,StoreContention,StoreStall,Transport},IsCancellation,ByMessageand the per-branch verdicts. Both vocabularies are open string constants. It always walks the exception graph, bounded, deduplicated and cycle-safe. Every copy is now a call to it:CancellationClassifier, the storage walk'sIsTeardownCancellation,MeshExtensions.IsTeardownOfThisHub,MessageHub.IsTeardownDisposal,SynchronizationStream.IsObjectDisposed/IsTransientHubTimeout,DataExtensions.IsOwnerTeardown,ScopeTeardown.IsScopeTeardown,HubDisposingException.IsDisposedContainer,ActivityControlPlaneExtensions.IsOwnHubDisposing,MessageHubGrain.IsActivationTeardownRace, and the three test-fixture filters. The Autofac "LifetimeScope" text now exists once.StorageFaults(the SQLSTATE set moved into the classifier),MeshNodeStreamCache.IsTransientOwnerFailure(the 13 store markers moved into the classifier),AreaErrorClassifier.IsTransientHubFailure/IsStorageUnavailable,StandingWatchRecovery.IsTransient,InfrastructureFault.IsTransient.RoutingGrain.IsTransientFailurenow forwards toOrleansRoutingService.IsTransientFailure, which is the one Orleans definition.QueryProviderStalledExceptionandSyncStreamOwnerRefusedExceptionstate their own verdict through the newIClassifiedFault. Both of its members have default implementations, so implementing it obliges nothing.Public names that Plugins uses keep their signatures:
CancellationClassifier.IsCooperativeCancellation,AreaErrorClassifier.IsTransientHubFailure, andStandingWatchRecovery.ReopenOnTransientFault/IsTransient. No public type or member is removed. Twointernalhelpers ofMeshNodeStreamCachethat are now dead are deleted.Doc page:
Doc/Architecture/FaultClassification(new).🚨 Behaviour changes (each is a SEMANTIC change; the pair gate cannot see them)
Each change is pinned by a test that fails on the copy that was wrong. All 11 facts below were run against origin/main's product code and were red there, as the negative control. They are green on this branch.
TimeoutExceptionended the whole walk → it is a per-path read fault: dropped and recorded, and the walk goes onFaultClassifierHostingForwardersTest.ATimeoutCancellation_IsNotTeardown_…CancellationClassifier.IsCooperativeCancellationFaultClassifierForwardersTest.ACancellationCarryingATimeoutAtAggregateIndexOne_IsNotCooperativeMessageHub.FailStartup,SynchronizationStream.OnErrorFailStartupTeardownClassificationTest.FailStartup_WithDisposalAtAggregateIndexOne_…RoutingGrain.IsTransientFailure(and soIsResendableDeliveryFailure)SocketException/HttpRequestException, which the routing service had → included. Also walks the graph, notInnerExceptionFaultClassifierOrleansForwardersTest.ASocketFault_…,ATimeoutAtAggregateIndexOne_IsTransientAreaErrorClassifier.IsTransientHubFailureFaultverdict except a stalled provider. NEW: the routed Orleans reject text ("Forwarding failed / invalid activation / Rejecting now"), the Orleans rejection type, store faults (typed and text markers),SocketException, an Autofac scope teardown,HubDisposedBeforeResponseException. Cooperative cancellation stays true, deliberately (see the doc page; it is not a contradiction ofCancellationClassifier). Mixed aggregates with a defect: worst branch winsFaultClassifierForwardersTest.TheRoutedOrleansReject_IsTransientForAView,ACancellation_IsRetryableForAView,AProviderStall_IsNever…MeshNodeStreamCache.IsTransientOwnerFailureSocketException, typed store faults, scope teardown,HubDisposedBeforeResponseException. A mixed aggregate whose branch 0 was transient and which also carries a defect is now not transientFaultClassifierHostingForwardersTest.ATypedSocketFault_IsATransientOwnerFailureStorageFaults.IsTransientConnectFault,AreaErrorClassifier.IsStorageUnavailableFaultClassifierForwardersTest.AnAggregateCarryingADefect_IsNotAStoreOutageAreaErrorClassifier.IsStorageUnavailableFaultClassifierForwardersTest.AWrappedProviderStall_IsStorageUnavailableInfrastructureFault.IsTransient(hub-init retire vs FAILED latch),StandingWatchRecovery.IsTransientDbException.IsTransient→ a connection-class SQLSTATE, or a driver exception wrapping timeout/socket/IO, is transient even when the provider did not set the flagFaultClassifierTest.AConnectClassSqlState_IsTransientInfrastructure_EvenUnflaggedMessageHubGrain.IsActivationTeardownRace(log level Error → Debug)ShuttingDowndelivery failure, "is shutting down" banner,HubDisposedBeforeResponseExceptionFaultClassifierOrleansForwardersTest.AShuttingDownDelivery_IsAnActivationTeardownRaceObjectDisposedException, and the filters now walk the graphFaultClassifierTest.TheAutofacText_OnANonDisposedException_IsNotADisposedContainerDataExtensions.IsOwnerTeardownHubDisposedBeforeResponseException(already an ODE)FaultClassifierTest)SynchronizationStream.OnErrorlog levelUnchanged on purpose:
InfrastructureFaultstill excludesHttpRequestException; a bareTimeoutExceptionis still not transient infrastructure; a stall is still never retried by a view or by the connect retry;MessageHub.FailStartupandSynchronizationStreamstill treat anyObjectDisposedExceptionas their own teardown (now stated asTeardownContext.DisposedIsOwnTeardown).The classifier also exposed a real defect while being tested: reading an
AggregateException.Messageis exponential on nested aggregates, because the message is composed from every branch's message.APathologicallyDeepGraph_Terminatesoverflowed a string until the classifier stopped reading aggregate text.Verification
dotnet build -c Release -warnaserror, run per project:MeshWeaver.Messaging.Contract,MeshWeaver.Documentation, and the test projectsMeshWeaver.Messaging.Hub.Test,MeshWeaver.Layout.Test,MeshWeaver.Hosting.Test,MeshWeaver.Hosting.Orleans.Test, which pull in every touchedsrc/project. All report0 Warning(s) 0 Error(s). The last two were re-run after merging origin/main.InfrastructureFaultTest,ExceptionChainTest,FailStartupTeardownClassificationTest,DisposedContainerIsATeardownFactTest,HubDisposedBeforeResponseIsClassifiedTest,DeletedAddressNackClassificationTest,DisposalRaceNackTest,TimeoutMessageNamesTheCallersOwnStateTest,DeferredDeliveryNackedOnDisposeTest,AreaErrorClassifierTest,StorageUnavailableRenderTest,DeliveryTimeoutIsTransientTest,AreaStreamRetryTest,AreaStreamReopenTest,ReadBudgetTest,MeshQueryTransientRetryTest,RecycledHubRefusalTest,OwnerAnswerRecognitionGuard,LiveRecordCensusReopensAfterTransientFaultTest,ChangeFeedResetReleasesUpstreamTest,RoutingGrainFailureClassificationTest,OrleansDirectoryInstabilityClassificationTest,TimedOutDeliveryIsNotResentTest,StreamPostTimeoutAttributionTest.Dependent suites (MeshWeaver.Plugins)verdict for this PR's run is the evidence; the selected suites are to be confirmed from that run.IsCooperativeCancellation(36 call sites),IsTransientHubFailure(24) andReopenOnTransientFault(44) are the surfaces it reaches.Pairs-with: none — no public type or member leaves
src/; only two deadinternalhelpers ofMeshNodeStreamCacheare removed.Implementers: IClassifiedFault.ClassifiedKind, IClassifiedFault.ClassifiedLayer — a NEW interface, and both members carry default implementations, so no implementer anywhere is obliged.
Mirror-sync: none — no i18n catalog key is added or changed.
No addresses need a recycle beyond the normal roll: this is compiled framework code, with no NodeType source or node content.
🤖 Generated with Claude Code