Repository navigation
De-flake CircuitBreakerSpec + StressSpec (deterministic) - #8427
Merged
Merged
Conversation
…Async, not a wall-clock window Must_increment_failure_count_on_callTimeout_before_call_finishes polled CurrentFailureCount inside a ~1400ms hand-padded window after a 50ms call-timeout that fires on a scheduler timer. Under threadpool starvation on a loaded CI agent the timer callback slips past the window and the poll times out (observed failing on both the Windows and Linux unit lanes). Replace the hand-padded window with an async AwaitAssertAsync on CurrentFailureCount using the framework's default budget — event-based retry until the timed-out call records its failure. No wall-clock race window, no timeout widened.
…ReportResult ReportResult (called from every phase: join, remove, partition, exercise-join-remove, idle-gossip, join-one-by-one) resolved the ClusterResult aggregator via the one-shot ClusterResultAggregator() — a single Identify + blocking ExpectMsg on a shared long-lived probe, no retry. Under the churn this spec deliberately generates (partition/blackhole, join/remove loops) a lone lost or delayed ActorIdentity reply was fatal, surfacing as 'Timeout ... while waiting for ActorIdentity'. akkadotnet#8372 already fixed this exact pattern at the aggregator lifecycle sites (CreateResultAggregatorAsync / AwaitClusterResultAsync) by routing them through the retrying, fresh-probe-per-attempt ClusterResultAggregatorAsync(); ReportResult was simply missed. Route it through the same helper and drop the dead synchronous ReportResult<T>(Func<T>) overload (no call site binds to it). The convergence 'Assert.Equal' signature seen on the same spec is separate: AwaitMembersUpAsync already polls via AwaitAssertAsync under a node-count-scaled WithinAsync, so that residual is inherent stress variance rather than a harness defect.
Aaronontheweb
enabled auto-merge (squash)
July 24, 2026 18:21
The previous attempt (AwaitAssertAsync on CurrentFailureCount) still failed in CI with Actual: 0, because it dropped the original's 'wait until the detached task is Running' guard without removing the underlying dependency on thread-pool scheduling. WithSyncCircuitBreaker is 'WithCircuitBreaker(body, (b, ct) => Task.Run(b, ct)).GetAwaiter().GetResult()', so driving it from Task.Run needs two pool threads — the outer one blocks while the body needs a second. Under Windows-lane starvation the outer call may not start for seconds, leaving CurrentFailureCount at 0 for the whole assertion budget. Run the blocking call on a dedicated background thread instead, and wait until it has actually started. AtomicState.CallThrough awaits the body with .WaitAsync(_callTimeout), so the 50ms call-timeout throws and CallFails() records the failure whether or not the body task itself ever got a pool thread — the only prerequisite is entering the call, which a dedicated thread guarantees. The catch is required: the call throws TimeoutException by design, and an unhandled exception on a non-pool thread would kill the process. Verified: 6/6 with the machine at 2x core saturation; full CircuitBreaker suite 29/29.
…pty catch The dedicated-thread version passed the unit lanes but tripped the code-quality gate: 'error SW003: Empty catch block swallows exceptions without handling'. The catch existed only because an unhandled exception on a loose thread would terminate the host. Task.Factory.StartNew(..., TaskCreationOptions.LongRunning) keeps the property that matters — the blocking call runs on a dedicated thread rather than competing for pool threads, so it always starts promptly under pool pressure — while the expected TimeoutException is captured by the returned Task instead of being thrown on a loose thread. No catch needed, nothing swallowed. Also assert the captured failure is a TimeoutException, documenting the mechanism the test relies on: AtomicState.CallThrough awaits the body with .WaitAsync(callTimeout), so the failure is recorded once the 50ms budget elapses whether or not the body task was ever scheduled. Verified: full CircuitBreaker suite 29/29, three consecutive runs at 2x core saturation.
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
…wn (port of #8543) Hand-port of dev's #8543 substance, not a cherry-pick - #8543 is the last commit of a five-deep stack (#8372, #8427, #8429, #8499) and its auto-merged parts would not compile as-is on v1.5 (BuildConfig's arithmetic and the ClusterResultAggregatorAsync call sites are written against dev's 10-node/1-per-phase config, which v1.5 does not have). What changed, and why each piece is still correct on v1.5: * acceptable-heartbeat-pause raised from 3s to 20s, with the arithmetic comment explaining the phi-accrual crossing-threshold math (1s + 20s + 3*0.1s = 21.3s of detection). v1.5's failure-detector defaults (heartbeat-interval=1s, min-std-deviation=100ms) and split-brain-resolver stable-after (10s) already match dev's, so the same 3s-was-too-tight problem applies here: a churn round abruptly tearing down an ActorSystem can starve the surviving node's own heartbeat sender for longer than a 3s pause tolerates. * akka.test.single-expect-default = 10s and akka.test.timefactor = 3, ported from #8372 (the first commit of the same dev stack), which the earlier port of this commit had missed. Every Within/WithinAsync bound in this file is dilated by timefactor - including RemoveOneAsync's removal budget (TimeSpan.FromSeconds(25) + ConvergenceWithin(3s, NbrUsedRoles - 1), which #8543 never widened) - so raising acceptable-heartbeat-pause to 20s without also porting timefactor left that budget structurally unable to cover the ~34.3s abrupt-removal path this spec's own ChurnMemberRemovalWithin() arithmetic predicts at the node counts the 7-node CI phase sequence reaches. Without timefactor, RemoveOneAsync's ceiling at NbrUsedRoles=3 is a flat 31s; with it, 93s. Confirmed against three consecutive 7-node runs: the abrupt-removal phase measured 32.9-33.4s in each, and the AwaitAssert measured a 30.98s give-up in each, both matching the undilated 31s ceiling to within noise. All three runs pass with timefactor ported. * ChurnMemberRemovalWithin(), ported byte-for-byte from dev (it only calls Cluster.Settings.FailureDetectorConfig/HeartbeatInterval/GossipInterval, all present on v1.5). Computes how long an abruptly-terminated churn member takes to actually leave the ring: detection + stable-after + a leader-gossip margin = 34.3s at this spec's config. * ExerciseJoinRemoveAsync's loopDuration now includes ChurnMemberRemovalWithin() so each round's Within budget covers the full removal path, not just the new join. The abrupt-shutdown behavior itself (ShutdownAsync, no cluster Leave) was already in place from #8499 - this keeps exactly what the phase proves (abrupt loss), it only fixes the budget around it. * Async TestKit migration of the call sites #8543 touches: the two RunOn sends inside the churn Loop become RunOnAsync, and ClusterResultAggregator (a one-shot, non-retried Identify/ExpectMsg lookup) gains a ClusterResultAggregatorAsync sibling (fresh-probe-per-attempt, retried over 30s) ported from dev, since a lone lost reply under this phase's deliberate churn must not be fatal. Repointed CreateResultAggregatorAsync, AwaitClusterResultAsync, and the async ReportResult<T> overload - the three call chains ExerciseJoinRemoveAsync depends on - at the new async lookup. Every remaining call site in the file passes an async lambda with an explicit return statement, so all of them already bound to the async ReportResult<T>(Func<Task<T>>) overload; the sync ClusterResultAggregator() and the sync ReportResult<T>(Func<T>) overload it served had no callers left after that repointing and are removed here. (Not ported: dev's extra "result-aggregator-identified" barrier in CreateResultAggregatorAsync, a related but separate hardening against a PartitionSeveral race - open item below.) * RemoveOneAsync's watchee lookup, hand-ported from dev (#8372/#8429): a fresh CreateTestProbe() per attempt instead of the shared IdentifyProbe (the shared probe kept a timed-out attempt's late ActorIdentity queued, so the next attempt consumed that stale reply, and a reply resolved before the watchee existed carries a null Subject that the retry could never recover from); an explicit identity.Subject.Should().NotBeNull() guard before WatchAsync; WatchAsync bounded with WaitAsync(Dilated(3s)) instead of inheriting the outer Within's RemainingOrDefault, since a hung/slow watch Ask would otherwise burn the whole retry budget in a single attempt; and an explicit AwaitAssertAsync(10s, 1.25s) bound instead of an unbounded retry loop. This is the exact method that was failing in the runs above, so it is ported alongside the budget fix rather than left as a separate follow-up. * StressSpecConfig node-count env override already existed on v1.5 (MNTR_STRESSSPEC_NODECOUNT, default 13). What it lacked was BuildConfig's shrink arithmetic: below the reference count, phase sizes must shrink or Settings' constructor throws. v1.5's reference config is heavier than dev's - every joining phase defaults to 2 nodes here, not 1 - so reaching the same practical floor of 7 requires shrinking both the joining side (halve every 2 back to 1) and the leaving/shutdown side (drop the "-large" one-by-one phases, halve the simultaneous counts), the same technique dev's config uses on one more group of phases. Derived and documented in StressSpecConfigSpec.cs (ported alongside, adapted from dev's 10-node version to v1.5's 13-node/2-per-phase defaults): 7 is confirmed the practical floor (6 throws because the joining phases alone need 7 regardless of how far leaving/shutdown shrinks). Also fixed while building this: MultiNodeTestRunner.cs's Process.Kill(bool) call from the earlier #8515 hand-merge doesn't compile against netstandard2.0 - see the preceding commit. Verified: dotnet build src/core/Akka.Cluster.Tests.MultiNode -warnaserror clean; StressSpecConfigSpec 9/9; StressSpec run three times at MNTR_STRESSSPEC_NODECOUNT=7, all three passed (see the PR body for full timings). Open items for the maintainer: - dev's extra CreateResultAggregatorAsync barrier (guards a PartitionSeveral aggregator- identification race) was not ported; the retrying ClusterResultAggregatorAsync lookup narrows that race but does not close it the way the extra barrier does.
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
…wn (port of #8543) Hand-port of dev's #8543 substance, not a cherry-pick - #8543 is the last commit of a five-deep stack (#8372, #8427, #8429, #8499) and its auto-merged parts would not compile as-is on v1.5 (BuildConfig's arithmetic and the ClusterResultAggregatorAsync call sites are written against dev's 10-node/1-per-phase config, which v1.5 does not have). What changed, and why each piece is still correct on v1.5: * acceptable-heartbeat-pause raised from 3s to 20s, with the arithmetic comment explaining the phi-accrual crossing-threshold math (1s + 20s + 3*0.1s = 21.3s of detection). v1.5's failure-detector defaults (heartbeat-interval=1s, min-std-deviation=100ms) and split-brain-resolver stable-after (10s) already match dev's, so the same 3s-was-too-tight problem applies here: a churn round abruptly tearing down an ActorSystem can starve the surviving node's own heartbeat sender for longer than a 3s pause tolerates. * akka.test.single-expect-default = 10s and akka.test.timefactor = 3, ported from #8372 (the first commit of the same dev stack), which the earlier port of this commit had missed. Every Within/WithinAsync bound in this file is dilated by timefactor - including RemoveOneAsync's removal budget (TimeSpan.FromSeconds(25) + ConvergenceWithin(3s, NbrUsedRoles - 1), which #8543 never widened) - so raising acceptable-heartbeat-pause to 20s without also porting timefactor left that budget structurally unable to cover the ~34.3s abrupt-removal path this spec's own ChurnMemberRemovalWithin() arithmetic predicts at the node counts the 7-node CI phase sequence reaches. Without timefactor, RemoveOneAsync's ceiling at NbrUsedRoles=3 is a flat 31s; with it, 93s. Confirmed against three consecutive 7-node runs: the abrupt-removal phase measured 32.9-33.4s in each, and the AwaitAssert measured a 30.98s give-up in each, both matching the undilated 31s ceiling to within noise. All three runs pass with timefactor ported. * ChurnMemberRemovalWithin(), ported byte-for-byte from dev (it only calls Cluster.Settings.FailureDetectorConfig/HeartbeatInterval/GossipInterval, all present on v1.5). Computes how long an abruptly-terminated churn member takes to actually leave the ring: detection + stable-after + a leader-gossip margin = 34.3s at this spec's config. * ExerciseJoinRemoveAsync's loopDuration now includes ChurnMemberRemovalWithin() so each round's Within budget covers the full removal path, not just the new join. The abrupt-shutdown behavior itself (ShutdownAsync, no cluster Leave) was already in place from #8499 - this keeps exactly what the phase proves (abrupt loss), it only fixes the budget around it. * Async TestKit migration of the call sites #8543 touches: the two RunOn sends inside the churn Loop become RunOnAsync, and ClusterResultAggregator (a one-shot, non-retried Identify/ExpectMsg lookup) gains a ClusterResultAggregatorAsync sibling (fresh-probe-per-attempt, retried over 30s) ported from dev, since a lone lost reply under this phase's deliberate churn must not be fatal. Repointed CreateResultAggregatorAsync, AwaitClusterResultAsync, and the async ReportResult<T> overload - the three call chains ExerciseJoinRemoveAsync depends on - at the new async lookup. Every remaining call site in the file passes an async lambda with an explicit return statement, so all of them already bound to the async ReportResult<T>(Func<Task<T>>) overload; the sync ClusterResultAggregator() and the sync ReportResult<T>(Func<T>) overload it served had no callers left after that repointing and are removed here. (Not ported: dev's extra "result-aggregator-identified" barrier in CreateResultAggregatorAsync, a related but separate hardening against a PartitionSeveral race - open item below.) * RemoveOneAsync's watchee lookup, hand-ported from dev (#8372/#8429): a fresh CreateTestProbe() per attempt instead of the shared IdentifyProbe (the shared probe kept a timed-out attempt's late ActorIdentity queued, so the next attempt consumed that stale reply, and a reply resolved before the watchee existed carries a null Subject that the retry could never recover from); an explicit identity.Subject.Should().NotBeNull() guard before WatchAsync; WatchAsync bounded with WaitAsync(Dilated(3s)) instead of inheriting the outer Within's RemainingOrDefault, since a hung/slow watch Ask would otherwise burn the whole retry budget in a single attempt; and an explicit AwaitAssertAsync(10s, 1.25s) bound instead of an unbounded retry loop. This is the exact method that was failing in the runs above, so it is ported alongside the budget fix rather than left as a separate follow-up. * StressSpecConfig node-count env override already existed on v1.5 (MNTR_STRESSSPEC_NODECOUNT, default 13). What it lacked was BuildConfig's shrink arithmetic: below the reference count, phase sizes must shrink or Settings' constructor throws. v1.5's reference config is heavier than dev's - every joining phase defaults to 2 nodes here, not 1 - so reaching the same practical floor of 7 requires shrinking both the joining side (halve every 2 back to 1) and the leaving/shutdown side (drop the "-large" one-by-one phases, halve the simultaneous counts), the same technique dev's config uses on one more group of phases. Derived and documented in StressSpecConfigSpec.cs (ported alongside, adapted from dev's 10-node version to v1.5's 13-node/2-per-phase defaults): 7 is confirmed the practical floor (6 throws because the joining phases alone need 7 regardless of how far leaving/shutdown shrinks). Also fixed while building this: MultiNodeTestRunner.cs's Process.Kill(bool) call from the earlier #8515 hand-merge doesn't compile against netstandard2.0 - see the preceding commit. Verified: dotnet build src/core/Akka.Cluster.Tests.MultiNode -warnaserror clean; StressSpecConfigSpec 9/9; StressSpec run three times at MNTR_STRESSSPEC_NODECOUNT=7, all three passed (see the PR body for full timings). Open items for the maintainer: - dev's extra CreateResultAggregatorAsync barrier (guards a PartitionSeveral aggregator- identification race) was not ported; the retrying ClusterResultAggregatorAsync lookup narrows that race but does not close it the way the extra barrier does.
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
…wn (port of #8543) Hand-port of dev's #8543 substance, not a cherry-pick - #8543 is the last commit of a five-deep stack (#8372, #8427, #8429, #8499) and its auto-merged parts would not compile as-is on v1.5 (BuildConfig's arithmetic and the ClusterResultAggregatorAsync call sites are written against dev's 10-node/1-per-phase config, which v1.5 does not have). What changed, and why each piece is still correct on v1.5: * acceptable-heartbeat-pause raised from 3s to 20s, with the arithmetic comment explaining the phi-accrual crossing-threshold math (1s + 20s + 3*0.1s = 21.3s of detection). v1.5's failure-detector defaults (heartbeat-interval=1s, min-std-deviation=100ms) and split-brain-resolver stable-after (10s) already match dev's, so the same 3s-was-too-tight problem applies here: a churn round abruptly tearing down an ActorSystem can starve the surviving node's own heartbeat sender for longer than a 3s pause tolerates. * akka.test.single-expect-default = 10s and akka.test.timefactor = 3, ported from #8372 (the first commit of the same dev stack), which the earlier port of this commit had missed. Every Within/WithinAsync bound in this file is dilated by timefactor - including RemoveOneAsync's removal budget (TimeSpan.FromSeconds(25) + ConvergenceWithin(3s, NbrUsedRoles - 1), which #8543 never widened) - so raising acceptable-heartbeat-pause to 20s without also porting timefactor left that budget structurally unable to cover the ~34.3s abrupt-removal path this spec's own ChurnMemberRemovalWithin() arithmetic predicts at the node counts the 7-node CI phase sequence reaches. Without timefactor, RemoveOneAsync's ceiling at NbrUsedRoles=3 is a flat 31s; with it, 93s. Confirmed against three consecutive 7-node runs: the abrupt-removal phase measured 32.9-33.4s in each, and the AwaitAssert measured a 30.98s give-up in each, both matching the undilated 31s ceiling to within noise. All three runs pass with timefactor ported. * ChurnMemberRemovalWithin(), ported byte-for-byte from dev (it only calls Cluster.Settings.FailureDetectorConfig/HeartbeatInterval/GossipInterval, all present on v1.5). Computes how long an abruptly-terminated churn member takes to actually leave the ring: detection + stable-after + a leader-gossip margin = 34.3s at this spec's config. * ExerciseJoinRemoveAsync's loopDuration now includes ChurnMemberRemovalWithin() so each round's Within budget covers the full removal path, not just the new join. The abrupt-shutdown behavior itself (ShutdownAsync, no cluster Leave) was already in place from #8499 - this keeps exactly what the phase proves (abrupt loss), it only fixes the budget around it. * Async TestKit migration of the call sites #8543 touches: the two RunOn sends inside the churn Loop become RunOnAsync, and ClusterResultAggregator (a one-shot, non-retried Identify/ExpectMsg lookup) gains a ClusterResultAggregatorAsync sibling (fresh-probe-per-attempt, retried over 30s) ported from dev, since a lone lost reply under this phase's deliberate churn must not be fatal. Repointed CreateResultAggregatorAsync, AwaitClusterResultAsync, and the async ReportResult<T> overload - the three call chains ExerciseJoinRemoveAsync depends on - at the new async lookup. Every remaining call site in the file passes an async lambda with an explicit return statement, so all of them already bound to the async ReportResult<T>(Func<Task<T>>) overload; the sync ClusterResultAggregator() and the sync ReportResult<T>(Func<T>) overload it served had no callers left after that repointing and are removed here. (Not ported: dev's extra "result-aggregator-identified" barrier in CreateResultAggregatorAsync, a related but separate hardening against a PartitionSeveral race - open item below.) * RemoveOneAsync's watchee lookup, hand-ported from dev (#8372/#8429): a fresh CreateTestProbe() per attempt instead of the shared IdentifyProbe (the shared probe kept a timed-out attempt's late ActorIdentity queued, so the next attempt consumed that stale reply, and a reply resolved before the watchee existed carries a null Subject that the retry could never recover from); an explicit identity.Subject.Should().NotBeNull() guard before WatchAsync; WatchAsync bounded with WaitAsync(Dilated(3s)) instead of inheriting the outer Within's RemainingOrDefault, since a hung/slow watch Ask would otherwise burn the whole retry budget in a single attempt; and an explicit AwaitAssertAsync(10s, 1.25s) bound instead of an unbounded retry loop. This is the exact method that was failing in the runs above, so it is ported alongside the budget fix rather than left as a separate follow-up. * StressSpecConfig node-count env override already existed on v1.5 (MNTR_STRESSSPEC_NODECOUNT, default 13). What it lacked was BuildConfig's shrink arithmetic: below the reference count, phase sizes must shrink or Settings' constructor throws. v1.5's reference config is heavier than dev's - every joining phase defaults to 2 nodes here, not 1 - so reaching the same practical floor of 7 requires shrinking both the joining side (halve every 2 back to 1) and the leaving/shutdown side (drop the "-large" one-by-one phases, halve the simultaneous counts), the same technique dev's config uses on one more group of phases. Derived and documented in StressSpecConfigSpec.cs (ported alongside, adapted from dev's 10-node version to v1.5's 13-node/2-per-phase defaults): 7 is confirmed the practical floor (6 throws because the joining phases alone need 7 regardless of how far leaving/shutdown shrinks). Also fixed while building this: MultiNodeTestRunner.cs's Process.Kill(bool) call from the earlier #8515 hand-merge doesn't compile against netstandard2.0 - see the preceding commit. Verified: dotnet build src/core/Akka.Cluster.Tests.MultiNode -warnaserror clean; StressSpecConfigSpec 9/9; StressSpec run three times at MNTR_STRESSSPEC_NODECOUNT=7, all three passed (see the PR body for full timings). Open items for the maintainer: - dev's extra CreateResultAggregatorAsync barrier (guards a PartitionSeveral aggregator- identification race) was not ported; the retrying ClusterResultAggregatorAsync lookup narrows that race but does not close it the way the extra barrier does.
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
1 participant
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.
De-flakes two intermittently-failing tests surfaced by CI-history mining, batched together (test-only changes). Both are deterministic fixes — no timeouts widened.
CircuitBreakerSpec —
Must_increment_failure_count_on_callTimeout_before_call_finishes(unit, both OS lanes)Polled
CurrentFailureCountinside a ~1400ms hand-padded wall-clock window after a 50ms call-timeout that fires on a scheduler timer. Under threadpool starvation the timer callback slips past the window and the poll times out. Replaced with an asyncAwaitAssertAsynconCurrentFailureCountusing the framework's default budget — event-based retry until the timed-out call records its failure. No hand-tuned window.StressSpec —
Cluster_under_stress(MNTR)ReportResult(used by every phase) resolved theClusterResultaggregator via the one-shotClusterResultAggregator()— a singleIdentify+ blockingExpectMsgon a shared probe, no retry — so a lone lost/delayedActorIdentityreply under this spec's deliberate churn was fatal (Timeout ... while waiting for ActorIdentity). #8372 already fixed this pattern at the aggregator-lifecycle sites via the retrying, fresh-probe-per-attemptClusterResultAggregatorAsync();ReportResultwas simply missed. Route it through the same helper and drop the dead syncReportResult<T>(Func<T>)overload (no call site binds to it).(The convergence
Assert.Equalsignature occasionally seen on the same spec is separate —AwaitMembersUpAsyncalready polls viaAwaitAssertAsyncunder a node-count-scaledWithinAsync, so that residual is inherent stress variance, not a harness defect.)Test config/code only; no production changes.