fix: CircuitWatcher.Dispose() does not stop the ping-until-reconnected loop#3326
Merged
jeremydmiller merged 2 commits intoJul 6, 2026
Merged
Conversation
…d loop CircuitWatcher only stored the caller's CancellationToken and disposed the Task wrapper on Dispose() -- neither actually stops pingUntilConnectedAsync, which keeps running against the still-live caller token. SendingAgent's own DisposeAsync() never called circuit watcher disposal either, so a sender whose circuit had tripped kept pinging a dead destination forever after the owning host was disposed. Reproduced with a Kafka producer pointed at a permanently unreachable broker: disposing the WebApplicationFactory-hosted app never stopped the background reconnect/ping loop, keeping the test process alive indefinitely regardless of how quickly each connection attempt failed. - CircuitWatcher now links the caller's token into its own CancellationTokenSource and cancels it in Dispose(), so Dispose() can stop the loop on its own instead of only releasing the Task wrapper. - SendingAgent.DisposeAsync() now disposes the circuit watcher before tearing down the sender. Added regression tests exercising both the isolated CircuitWatcher and a BufferedSendingAgent end-to-end; both fail (ping count keeps climbing after Dispose()) without this fix and pass with it.
There was a problem hiding this comment.
Pull request overview
This PR fixes a shutdown/leak issue where CircuitWatcher’s background “ping until reconnected” loop could continue running after disposal, and ensures SendingAgent properly tears down its circuit watcher during shutdown to prevent indefinite background retries.
Changes:
- Link
CircuitWatcher’s cancellation to an internalCancellationTokenSourceand cancel it onDispose()to stop the ping loop. - Dispose the active circuit watcher in
SendingAgent.DisposeAsync()before disposing the underlying sender. - Add/adjust tests to assert circuit-watcher activity stops after disposal (both isolated and via
BufferedSendingAgent).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Wolverine/Transports/Sending/SendingAgent.cs | Disposes the circuit watcher during agent shutdown to stop background pinging. |
| src/Wolverine/Transports/Sending/CircuitWatcher.cs | Introduces a linked CTS and cancellation on dispose so the ping loop can be stopped. |
| src/Testing/CoreTests/Transports/Sending/SendingAgentDisposalTests.cs | Adds regression tests for stopping the ping loop on watcher/agent disposal. |
| src/Testing/CoreTests/Transports/Sending/CircuitWatcherTester.cs | Makes the stub circuit’s call counting/threading and retry interval configurable for new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
40
to
44
| private async Task pingUntilConnectedAsync() | ||
| { | ||
| using var timer=new PeriodicTimer(_senderCircuit.RetryInterval); | ||
| while (await timer.WaitForNextTickAsync(_cancellation)) | ||
| while (await timer.WaitForNextTickAsync(_cancellation.Token)) | ||
| { |
Comment on lines
+24
to
+28
| watcher.Dispose(); | ||
|
|
||
| var countAtDispose = circuit.CallCount; | ||
| await Task.Delay(200.Milliseconds()); | ||
|
|
Comment on lines
+56
to
+60
| await agent.DisposeAsync(); | ||
|
|
||
| var pingCountAtDispose = sender.PingCount; | ||
| await Task.Delay(200.Milliseconds()); | ||
|
|
- pingUntilConnectedAsync now wraps the whole loop (not just the inner ping) in a try/catch: cancelling and disposing the linked CancellationTokenSource in Dispose() can surface as OperationCanceledException from the timer wait, or ObjectDisposedException if a token registration races the dispose. Both are expected shutdown outcomes, not faults. - Both new tests settle for one retry interval after Dispose()/DisposeAsync() before taking the "count so far" baseline, so an already-in-flight ping can't tick over during the assertion window and cause a spurious failure.
Contributor
Author
|
Good catches, thanks — pushed a fix for both:
|
This was referenced Jul 9, 2026
Merged
This was referenced Jul 13, 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.
Summary
CircuitWatcheronly stores the caller'sCancellationTokenand itsDispose()just disposes theTaskwrapper -- neither actually stopspingUntilConnectedAsync, which keeps running against the still-live caller token.SendingAgent's ownDisposeAsync()never disposed the circuit watcher either, so once a sender's circuit trips, it keeps pinging a dead destination forever, even after the owning host/agent has been disposed.I hit this concretely with the Kafka transport: a test host pointed at a permanently unreachable broker (used deliberately to exercise startup config validation) never exited, because the producer's circuit-breaker ping loop kept retrying in the background indefinitely regardless of how fast/slow each connection attempt failed -- disposing the
WebApplicationFactorydid not stop it. The same gap applies to any transport that goes throughSendingAgent's circuit-breaker path, not just Kafka.Fix
CircuitWatchernow links the caller's token into its ownCancellationTokenSourceand cancels that source inDispose(), soDispose()can actually stop the loop instead of only releasing theTaskwrapper.SendingAgent.DisposeAsync()now disposes the circuit watcher (mirroring the existing cleanup already done inMarkSuccessAsync()) before tearing down the sender.Test plan
circuit_watcher_dispose_stops_the_ping_loop(isolatedCircuitWatcher+StubCircuit) anddisposing_a_sending_agent_stops_its_circuit_watcher(end-to-end viaBufferedSendingAgent) toCoreTests/Transports/Sending/.Dispose()/DisposeAsync()) and pass with it.dotnet build src/Wolverine/Wolverine.csprojanddotnet build src/Testing/CoreTests/CoreTests.csproj: clean.CoreTestssuite: 1856 passed, 4 pre-existing failures unrelated to this change (confirmed identical failures on unmodifiedmain:Bug_2896_mixed_lifetime_enumerable_dependencyand 3result_types_end_to_endtests, all "Cannot resolve scoped service ... from root provider" -- a DI scoping issue unrelated to sending/circuit-breaker code).