GH-3965: stop the ConventionalRouting tests leaking Rabbit consumers - #3967
Merged
Conversation
end_to_end_with_conventional_routing.send_from_one_node_to_another_all_with_conventional_routing
failed in a full Wolverine.RabbitMQ.Tests run and passed 3/3 in isolation. The tracked
session showed the message Sent and never Received by anyone -- something else consumed it.
Every conventionally routed ConventionallyRoutedMessage lands on ONE fixed queue: the type
carries [MessageIdentity("routed")], so the conventional queue is literally "routed", and
five classes in this namespace stand up hosts against it, several with a live listener.
All of them tore down as:
ValueTask IAsyncDisposable.DisposeAsync() => ValueTask.CompletedTask;
public void Dispose() => _host.Dispose();
IHost.Dispose() does NOT run IHostedService.StopAsync, so the Rabbit listeners were never
cancelled and their consumers stayed attached to "routed" on the broker. A later class's
message was then delivered to a leaked consumer belonging to an already-finished test, and
the real receiver never saw it.
ConventionalRoutingContext now implements IAsyncDisposable and exposes a protected
DisposeHostAsync(). Derived classes that implement IAsyncLifetime SHADOW the interface
implementation, so each one calls the helper explicitly rather than returning a completed
ValueTask -- that shadowing is the trap that made the base class fix alone insufficient.
Test infrastructure only; no shipped code path depends on Dispose() stopping a host.
Namespace run went 33/34 -> 34/34.
Not touched: discover_with_naming_prefix and Bug_3633_conventional_routing_respects_named_broker
leak the same way, but they use prefixed and named-broker queues that do not collide with
plain "routed", and they build their hosts in a constructor, so converting them is a larger
refactor than this fix needs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JG8Un6iNeyXECKJk3jo5uC
Member
Author
|
Full The progression across this session, all on the same machine and broker:
Namespace-level: That also retires the open question from #3964, where the same test failed on both the branch and the unmodified baseline — it was this leak, not the channel quiesce. |
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.
Closes #3965. Test infrastructure only — no shipped code path depends on
Dispose()stopping a host.The evidence
end_to_end_with_conventional_routing.send_from_one_node_to_another_all_with_conventional_routingfailed in a fullWolverine.RabbitMQ.Testsrun and passed 3/3 in isolation. The tracked session is what pinned it:Sent, and never Received by anyone. Not "received by the wrong host" — nothing in the session consumed it at all.
The cause
ConventionallyRoutedMessagecarries[MessageIdentity("routed")], so every conventionally routed instance lands on one fixed queue namedrouted. Five classes in this namespace stand up hosts against it, several with conventional discovery on, which gives them a live listener viaRoutedMessageHandler.Every one of them tore down like this:
IHost.Dispose()does not runIHostedService.StopAsync, so the Rabbit listeners were never cancelled and their consumers stayed attached toroutedon the broker after the class finished. A later class's message was delivered to one of those orphans, and the real receiver never saw it.The trap in the fix
Making
ConventionalRoutingContextimplementIAsyncDisposableis not sufficient on its own. Its derived classes implementIAsyncLifetime, whose explicitValueTask IAsyncDisposable.DisposeAsync()shadows the base implementation — so the base would simply never run. Each derived class therefore calls the newprotected DisposeHostAsync()explicitly.Deliberately not touched
discover_with_naming_prefixandBug_3633_conventional_routing_respects_named_brokerleak the same way, but they use prefixed and named-broker queue names that don't collide with plainrouted, so they don't contribute to this interference. They also build their hosts in a constructor, so converting them means restructuring toIAsyncLifetime— a larger change than this fix warrants. Worth a follow-up if the suite ever shows symptoms from them.Verification
Wolverine.RabbitMQ.Tests.ConventionalRoutingnamespace: 33/34 → 34/34Note on timing
The full suite was recorded at 503/503 on 2026-08-15, so something tipped the ordering or shutdown timing recently — #3796, #3960 and #3961 all touched
RabbitMqListenershutdown and are the candidates. The leak itself predates all of them, and is the defect regardless.🤖 Generated with Claude Code
https://claude.ai/code/session_01JG8Un6iNeyXECKJk3jo5uC