Re-consume on a successful eager RabbitMQ channel restart + pin the ConnectionMonitor tracking invariant (GH-3391)#3419
Merged
Conversation
…nnectionMonitor tracking invariant (GH-3391) Two follow-ups from the #3370 review of RabbitMqChannelAgent's callback-exception eager restart. Behavioral fix -- a successful eager restart of a listener never re-consumed. HandleChannelExceptionAsync only ran teardownChannel() + startNewChannel(): a fresh channel, but no re-declare and no BasicConsumeAsync. Because teardownChannel() unsubscribes the shutdown handler before closing, the #3171/#3187 push-heal does not fire either, so a callback-exception-only failure left the listener on an OPEN CHANNEL WITH ZERO CONSUMERS while reporting State = Connected -- a silently dead listener. The eager restart is now a protected virtual restartAfterCallbackExceptionAsync(). The base implementation keeps the sender-shaped behavior (a fresh channel is all a sender needs). RabbitMqListener overrides it to defer to its existing ReconnectedAsync(), which already does the correct work -- cancel any surviving consumer, tear the channel down, re-declare, re-consume -- rather than bolting on a duplicate consume path. ReconnectedAsync() is now serialized behind a _reconnectLock so that its three triggers (connection recovery, unexpected channel-only shutdown, callback exception) cannot race two rebuilds onto the same channel and leave duplicate consumers behind; a burst of callback exceptions is exactly the reported shape. Test-only -- ConnectionMonitor.TrackedAgents (internal) plus Bug_3391_callback_exception_restart, which pins the invariant that an agent stays tracked across a callback-exception restart. Only tracked agents are rebuilt by connectionOnRecoverySucceededAsync, so before #3370 even a SUCCESSFUL restart left the agent untracked and one connection drop away from ghosting. Nothing pinned that. Both tests drive a genuine channel callback exception (an unroutable mandatory publish into a throwing BasicReturnAsync handler) against the live broker. The re-consume test fails on the pre-fix code exactly as described: 0 consumers on the queue after the restart. Fixes #3391 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 14, 2026
This was referenced Jul 18, 2026
Open
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.
Fixes #3391.
Two follow-ups from the review of #3370 (the RabbitMQ listener ghosting fix). One is a real behavioral bug; the other is test-only. They're separated below.
Part 1 (behavioral fix) — a successful eager restart of a listener never re-consumed
RabbitMqChannelAgent.HandleChannelExceptionAsynchandled a channel callback exception by runningteardownChannel()+startNewChannel(). That gives you a fresh channel and nothing else — no re-declare, noBasicConsumeAsync.The
#3171/#3187shutdown push-heal does not cover this, becauseteardownChannel()unsubscribes the shutdown handler before closing the channel. So a callback-exception-only failure (the channel itself survives) left the listener in exactly the state the issue describes:A silently dead listener on an apparently healthy process. Nothing pulls from the queue and nothing reports a problem.
Reproduced
The new
listener_still_consumes_after_a_successful_eager_restarttest fails on the pre-fix code precisely as predicted — after the eager restart the channel is open, the agent reportsConnected, and the broker reports 0 consumers on the queue:The fix
The eager restart is now a
protected virtual restartAfterCallbackExceptionAsync():RabbitMqChannelAgentkeeps the existing sender-shaped behavior as the base implementation — a fresh channel is all a sender needs, andRabbitMqSenderis the only other subclass. The failure path is unchanged: an exception still leaves the agent tracked andDisconnectedfor connection recovery to rebuild (that's Fix RabbitMQ listener ghosting after broker restart on channel callback exception #3370's fix).RabbitMqListeneroverrides it to defer to its existingReconnectedAsync(), which already does the correct work — cancel any surviving consumer, tear the channel down, re-declare, re-consume. No duplicate consume path was added;ReconnectedAsync()is the right authority and every other listener-rebuild trigger already routes through it.RabbitMqListener.ReconnectedAsync()is now serialized behind a_reconnectLock. It has three triggers (connection recovery, unexpected channel-only shutdown, and now callback exception), and callback exceptions arrive in bursts — that's the shape reported in Fix RabbitMQ listener ghosting after broker restart on channel callback exception #3370 (N × "Callback error in Rabbit Mq agent"). Without the gate, two concurrent rebuilds can race onto the same channel and leave duplicate consumers behind.Part 2 (test-only) — pinning the ConnectionMonitor tracking invariant
The invariant: an agent must remain tracked by
ConnectionMonitorafter a callback-exception-triggered restart, whether that restart succeeds or fails. Only tracked agents are iterated byconnectionOnRecoverySucceededAsync, so an agent that falls out of_agentsis never rebuilt on reconnect.Before #3370,
_monitor.Remove(this)had no compensating re-Track()anywhere — so even a successful eager restart left the agent permanently untracked and one connection drop away from ghosting. #3370 fixed it; nothing pinned it.internal ConnectionMonitor.TrackedAgentsaccessor over_agents(returns a snapshot under the existing lock).agent_stays_tracked_by_the_connection_monitor_across_a_callback_exception_restarttest.This part changes no runtime behavior.
How the tests drive it
Both tests provoke a genuine channel callback exception against the live broker rather than reaching in and invoking the handler: subscribe a throwing handler to
BasicReturnAsync, then publish a mandatory message to a routing key that resolves to nothing. The broker returns it, the handler throws, and RabbitMQ.Client raisesCallbackExceptionAsyncon the channel — while leaving the channel open, which is exactly the scenario the push-heal misses.Following the
Bug_3171_channel_only_shutdown_recoverypattern: internal-state assertions against a live broker, poll-based waits, unique queue names, no sharedfan_out/direct_exchangestate.Test results
Wolverine.RabbitMQ.Tests, full suite, live broker: 473 passed / 477, 13m16s. The 4 failures are pre-existing shared-broker flakes unrelated to this change —use_fan_out_exchange,multi_tenancy_through_virtual_hosts.send_message_to_a_specific_tenant, andBug_1594_ReplayDeadLetterQueue(both modes). All 4 pass in isolation with this change applied, and the vhost test also passes on unmodifiedmainonly when run alone — i.e. ordering/shared-state, not a regression.Full solution builds clean:
dotnet build wolverine.slnx -c Release→ 0 warnings, 0 errors.#3137 assessment — leaving the circuit-breaking tests quarantined
CircuitBreakingTestspasses 24/24 (3m59s) on this branch, but I am not un-quarantining it, for two reasons:BufferedReceiverdrain /Restartertiming (buffered_not_parallelized,buffered_and_parallel). This PR changes the channel callback-exception restart path — a different subsystem. There's no mechanism by which it would fix a pause/drain timing race.Un-quarantining here would be guesswork of exactly the kind #3137 warns against, so #3137 stays open and
circuit-breaking.ymlstays informational.🤖 Generated with Claude Code