Skip to content

TestKit.Xunit: implement the async dispose chain so a derived DisposeAsync no longer leaks the ActorSystem; de-flake StreamRefsSpec - #8545

Merged
Aaronontheweb merged 5 commits into
devfrom
fix/testkit-xunit-async-dispose-chain
Sep 11, 2026
Merged

Aaronontheweb merged 5 commits into
devfrom
fix/testkit-xunit-async-dispose-chain

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

Stacked on #8558.

What changes

  • Akka.TestKit.Xunit.TestKit implements the async lifecycle. It gains virtual InitializeAsync() and virtual DisposeAsync(). Both public entry points run the same chain, Dispose(bool) with AfterAll and any override inside it, and differ only in how they terminate the ActorSystem: Dispose() blocks on Shutdown(), DisposeAsync() awaits ShutdownAsync(). A derived test class that overrides DisposeAsync and chains to the base no longer skips teardown. This is stalled PR fix: implement IAsyncLifetime on Akka.TestKit.Xunit (v3) to stop ActorSystem leaks (#8191) #8217 ported to dev, minus its TestKitBase hunk, which landed in Add TestKitBase.ShutdownAsync and adopt it in StressSpec churn teardown #8499. Its regression test, Bugfix8191Spec, comes with it. Extend-only.
  • Four derived classes that declared their own DisposeAsync or InitializeAsync now use override and chain to the base. BREAKING_CHANGES_V1.6.md records the compile-warning risk for user code in the same shape.
  • StreamRefsSpec overrides DisposeAsync to shut the remote system down asynchronously and then chain to the base, binds its remote system to port 0 instead of a probed port, and its remoting fact is now async, gated on WatchTermination so the test waits for the stream to finish rather than for a clock, with a dilated 15 s budget on all four remote waits.

Why

Build 131174 failed SinkRef_must_receive_elements_via_remoting after a flat 3 s wait, on a PR that changed one package version. The stream-ref handshake is correct and matches Pekko; no element can leave before the partner is watched. The test was fragile for two reasons the analysis established:

  • Issue Akka.TestKit.Xunit (v3): base.DisposeAsync() unreachable from derived IAsyncLifetime — silently leaks ActorSystems #8191. xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to Dispose() when a class has both. StreamRefsSpec had a no-op DisposeAsync, so its teardown never ran and every fact leaked two remoting-enabled ActorSystems, each keeping DotNetty threads and a 100 Hz scheduler on the shared thread pool. Twenty-eight leaked systems by the end of the class.
  • The test blocked its calling thread with .GetAwaiter().GetResult() while the handshake needed about ten thread-pool dispatches, on a 2-vCPU agent whose pool floor is two. That is the shape every other thread-pool flake this week shared.

No thread pool setting is changed. Two product items from the analysis are not in this PR and are worth their own issues: SinkRefImpl lacks Pekko's grace period on early partner termination, and ActorMaterializerImpl.ActorOf blocks on a task result. A separate small PR fixes a TestKit config key that never took effect.

How it was checked

Akka.TestKit.Xunit and Akka.Streams.Tests build with warnings as errors, as do the four other touched test projects. Akka.API.Tests: 18 passed, no approval change, since the v3 TestKit has no approval file and TestKitBase is untouched. StreamRefsSpec run five times: 16 passed, 1 skipped, every time. Bugfix8191Spec, all of Akka.TestKit.Xunit.Tests (33), and Akka.TestKit.Tests (220) pass.

Second commit: the touched files move to the async TestKit API

Per the maintainer's rule that a touched test file migrates in the same PR: the fourteen remaining facts in StreamRefsSpec are async Task and await ExpectMsgAsync, ExpectNoMsgAsync, and the stream-completion tasks (three .Wait(8s) calls become awaits with the same timeout). One fact each in the DI BugFixSpec and Bugfix8144Spec migrate the same way, since this PR already touched them for the override. No assertion or skip marker changes. A grep for the synchronous forms over StreamRefsSpec returns nothing. StreamRefsSpec run three times after the migration: 16 passed, 1 skipped, each time.

Third commit: what the adversarial review found

  • Seven blocking stream-probe calls were still inside async facts, each a blocking wait on its async twin: EnsureSubscription, ExpectNext, ExpectNextN, ExpectError. Three more turned up on a full scan, ExpectCancellation twice and ExpectRequest once. All ten now await the async form.
  • Two thread sleeps become dilated task delays. There is no event to wait on there: the remote side's 500 ms subscription timeout fires with nothing materialized locally, so the wait is a real clock wait, now dilated instead of flat.
  • The spec's log level goes back to INFO. Debug logging on a remoting plus streams spec loads the thread pool this PR blames.
  • The DI BugFixSpec still shut its DI-managed system down with a blocking call in AfterAll; it now overrides DisposeAsync, shuts that system down asynchronously, and chains to the base, the same shape StreamRefsSpec uses.

StreamRefsSpec run three times: 16 passed, 1 skipped, each time. BugFixSpec passes. Both projects build with warnings as errors. The grep for blocking probe and TestKit calls on both files returns only comments.

Fourth commit: the lease spec joins the chain

ClusterShardingLeaseSpec on #8558 declares its own InitializeAsync and DisposeAsync through IAsyncLifetime, and this PR makes both virtual on the TestKit, so whichever landed second would have failed the build with two hidden-member errors. This PR is now stacked on #8558, and its fourth commit converts that spec the way the other derived classes were converted: InitializeAsync becomes an override that chains to the base, the dispose bridge goes away because the base now runs the synchronous dispose and the async shutdown itself, and the interface declaration is dropped. The sharding test project builds with warnings as errors, which is the build that would have broken, and the lease spec passes 15 of 15 twice.

Fifth commit: one flag, one chain

The maintainer asked why the TestKit carried three flags. Two predated this PR, and one of those was already redundant since it was never cleared; the third was a mode switch this PR added so DisposeAsync could run the virtual Dispose(bool) while suppressing the blocking Shutdown() inside it. The chain is now the smallest correct shape: Dispose(bool) runs AfterAll() and nothing else, so it is the one place overriders hook; a single _disposed flag is checked and set at the top of each public entry point; Dispose() runs the chain then Shutdown() in a finally, DisposeAsync() runs it then await ShutdownAsync() in a finally; a second call by either route, including Dispose() after DisposeAsync(), is a no-op.

What changes for an overrider: base.Dispose(disposing) no longer terminates the system; the public entry point does, after the whole chain. Every Dispose(bool) override in this repo is on a stream or stage type, not on this TestKit, and the derived DisposeAsync overrides in StreamRefsSpec and the DI BugFixSpec shut their own second system down and then chain to base, which is unaffected. A new fact in Bugfix8191Spec proves the chain runs once when Dispose() follows DisposeAsync().

Checked: Akka.TestKit.Xunit builds with warnings as errors; Akka.TestKit.Xunit.Tests 34 passed; Akka.TestKit.Tests 331 passed, 1 skipped; StreamRefsSpec 16 passed, 1 skipped; Akka.API.Tests 18 passed, no approval diff.

Aaronontheweb added a commit that referenced this pull request Sep 9, 2026
… fact; name it distinctly

Each fact now owns its peer ActorSystem and shuts it down with ShutdownAsync in a finally block, so teardown no longer pins a thread pool thread in the synchronous AfterAll and no DisposeAsync is declared on the class, which would collide with the async dispose chain the TestKit gains in #8545. The peer system gets a distinct name; it self-joins its own cluster and Sys never joins it, so nothing depends on the two names matching.
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from 4a56b3c to b18522d Compare September 9, 2026 18:18
Aaronontheweb added a commit that referenced this pull request Sep 9, 2026
… fact; name it distinctly

Each fact now owns its peer ActorSystem and shuts it down with ShutdownAsync in a finally block, so teardown no longer pins a thread pool thread in the synchronous AfterAll and no DisposeAsync is declared on the class, which would collide with the async dispose chain the TestKit gains in #8545. The peer system gets a distinct name; it self-joins its own cluster and Sys never joins it, so nothing depends on the two names matching.
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from b18522d to 4698131 Compare September 10, 2026 04:59
@Aaronontheweb
Aaronontheweb changed the base branch from dev to fix/sharding-lease-spec-async-join September 10, 2026 04:59
@Aaronontheweb
Aaronontheweb added this pull request to stack #8578 September 10, 2026 12:59
Aaronontheweb added a commit that referenced this pull request Sep 10, 2026
…fresh probe per attempt under a sharding-derived budget; per-system settings

The first message through a fresh region has to survive coordinator
allocation and shard start, all ddata majority writes/reads that degrade
to "all nodes" on this test's 2-node cluster. If the shard's
remember-entities write stalls past its deadline, the Shard restarts and
its buffered messages die with it - no dead letter, nothing left to
re-deliver. Sharding is at-most-once, so a bare ExpectMsgAsync on the
flat akka.test.single-expect-default can time out waiting for a message
that no longer exists; waiting longer never helps.

FirstMessageThrough re-sends each of the three cold sends through
AwaitAssertAsync with a fresh TestProbe per attempt (so a late reply to
an abandoned attempt can't satisfy a later one, or leak into the warm
phase below), budgeted at ShardStartTimeout + UpdatingStateTimeout (the
cold path plus one full Shard restart cycle) with each attempt capped at
RetryInterval so the loop actually iterates. The counter snapshots are
taken after the loops, so BeGreaterOrEqualTo still holds under retries.

The warm-phase sends stay single sends (entities are live; a shard
restart here would mean no remember-entities write ever happened, so
retrying would hide a real bug) bounded by UpdatingStateTimeout instead
of the flat probe default, plus a LastSender check against the original
entity ref so a restart between phases can't hide behind the
assertions.

Also: StartShard was building both regions from Sys's settings instead
of the settings of the system whose region is being started (harmless
today since sysB is created from Sys.Settings.Config, but wrong); and
AfterAll shut Sys down twice (once directly, once via TestKit.Dispose's
finally) since _sysA is Sys - removed the redundant call and left a
TODO(#8545) for migrating both shutdowns to the async TestKit dispose
chain once that lands.
Aaronontheweb added a commit that referenced this pull request Sep 10, 2026
…fresh probe per attempt under a sharding-derived budget; per-system settings

The first message through a fresh region has to survive coordinator
allocation and shard start, all ddata majority writes/reads that degrade
to "all nodes" on this test's 2-node cluster. If the shard's
remember-entities write stalls past its deadline, the Shard restarts and
its buffered messages die with it - no dead letter, nothing left to
re-deliver. Sharding is at-most-once, so a bare ExpectMsgAsync on the
flat akka.test.single-expect-default can time out waiting for a message
that no longer exists; waiting longer never helps.

FirstMessageThrough re-sends each of the three cold sends through
AwaitAssertAsync with a fresh TestProbe per attempt (so a late reply to
an abandoned attempt can't satisfy a later one, or leak into the warm
phase below), budgeted at ShardStartTimeout + UpdatingStateTimeout (the
cold path plus one full Shard restart cycle) with each attempt capped at
RetryInterval so the loop actually iterates. The counter snapshots are
taken after the loops, so BeGreaterOrEqualTo still holds under retries.

The warm-phase sends stay single sends (entities are live; a shard
restart here would mean no remember-entities write ever happened, so
retrying would hide a real bug) bounded by UpdatingStateTimeout instead
of the flat probe default, plus a LastSender check against the original
entity ref so a restart between phases can't hide behind the
assertions.

Also: StartShard was building both regions from Sys's settings instead
of the settings of the system whose region is being started (harmless
today since sysB is created from Sys.Settings.Config, but wrong); and
AfterAll shut Sys down twice (once directly, once via TestKit.Dispose's
finally) since _sysA is Sys - removed the redundant call and left a
TODO(#8545) for migrating both shutdowns to the async TestKit dispose
chain once that lands.
Aaronontheweb added a commit that referenced this pull request Sep 10, 2026
…fresh probe per attempt under a sharding-derived budget (#8573)

* De-flake ShardingBufferAdapterSpec: re-send the cold messages with a fresh probe per attempt under a sharding-derived budget; per-system settings

The first message through a fresh region has to survive coordinator
allocation and shard start, all ddata majority writes/reads that degrade
to "all nodes" on this test's 2-node cluster. If the shard's
remember-entities write stalls past its deadline, the Shard restarts and
its buffered messages die with it - no dead letter, nothing left to
re-deliver. Sharding is at-most-once, so a bare ExpectMsgAsync on the
flat akka.test.single-expect-default can time out waiting for a message
that no longer exists; waiting longer never helps.

FirstMessageThrough re-sends each of the three cold sends through
AwaitAssertAsync with a fresh TestProbe per attempt (so a late reply to
an abandoned attempt can't satisfy a later one, or leak into the warm
phase below), budgeted at ShardStartTimeout + UpdatingStateTimeout (the
cold path plus one full Shard restart cycle) with each attempt capped at
RetryInterval so the loop actually iterates. The counter snapshots are
taken after the loops, so BeGreaterOrEqualTo still holds under retries.

The warm-phase sends stay single sends (entities are live; a shard
restart here would mean no remember-entities write ever happened, so
retrying would hide a real bug) bounded by UpdatingStateTimeout instead
of the flat probe default, plus a LastSender check against the original
entity ref so a restart between phases can't hide behind the
assertions.

Also: StartShard was building both regions from Sys's settings instead
of the settings of the system whose region is being started (harmless
today since sysB is created from Sys.Settings.Config, but wrong); and
AfterAll shut Sys down twice (once directly, once via TestKit.Dispose's
finally) since _sysA is Sys - removed the redundant call and left a
TODO(#8545) for migrating both shutdowns to the async TestKit dispose
chain once that lands.

* ShardingBufferAdapterSpec: the identity check's comment names the right equality
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from 4698131 to d682a57 Compare September 10, 2026 15:27
Base automatically changed from fix/sharding-lease-spec-async-join to dev September 10, 2026 15:46
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from d682a57 to 26d8a57 Compare September 10, 2026 15:46

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Aaronontheweb added a commit that referenced this pull request Sep 10, 2026
… fact; name it distinctly

Each fact now owns its peer ActorSystem and shuts it down with ShutdownAsync in a finally block, so teardown no longer pins a thread pool thread in the synchronous AfterAll and no DisposeAsync is declared on the class, which would collide with the async dispose chain the TestKit gains in #8545. The peer system gets a distinct name; it self-joins its own cluster and Sys never joins it, so nothing depends on the two names matching.
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from 36bd569 to 84a5a6d Compare September 11, 2026 14:25
@Aaronontheweb

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec

Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to
IDisposable.Dispose() whenever a type implements both, so a derived spec's own
no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous
dispose chain and silently leaked a remoting-enabled ActorSystem per test.
Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual
InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then
shuts the system down with the non-blocking ShutdownAsync(). Ports the
TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its
TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via
81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current
AwaitAssertAsync retry body. Five other TestKit-derived specs that declared
their own DisposeAsync/InitializeAsync are updated to override and chain to
base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both
base classes), and EventFilterTestBase.

StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top
of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of
asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout,
and gives every cross-boundary wait a dilated 15s budget. Remote system port
is now 0 and loglevel is DEBUG for stage-level tracing.
…Kit API

Convert ExpectMsg, ExpectNoMsg, and .Wait()-on-stream-completion calls to ExpectMsgAsync, ExpectNoMsgAsync, and awaited AwaitWithTimeout across all facts in the three touched files.
… so the async path needs no mode switch

Dispose(bool) now only runs AfterAll() (and any derived teardown); it no
longer terminates the ActorSystem and no longer guards re-entrancy. The
guard collapses to a single _disposed flag checked once, in each public
entry point: Dispose() runs the chain then calls the blocking Shutdown(),
DisposeAsync() runs the chain then awaits ShutdownAsync(). This drops the
now-redundant _disposing/_disposingAsync flags and the nested try/finally
that used to suppress Shutdown() inside the async path.

Verified: no Dispose(bool) override in the tree depends on the ActorSystem
being torn down when base.Dispose(disposing) returns (the only overrides in
the repo are on System.IO.Stream/GraphStageLogic, unrelated to this
TestKit); StreamRefsSpec, BugFixSpec's DisposeAsync overrides still tear
down their own systems before chaining to base.DisposeAsync(), and
ClusterShardingLeaseSpec no longer declares one at all. Added a
Bugfix8191Spec case covering Dispose() called after DisposeAsync().
@Aaronontheweb
Aaronontheweb force-pushed the fix/testkit-xunit-async-dispose-chain branch from 84a5a6d to c2b01e7 Compare September 11, 2026 17:34
This was referenced Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant