Stop dereferencing a null channel when a RabbitMQ listener is disposed mid-startup (GH-3842) - #3843
Merged
Merged
Conversation
…rtup (GH-3842) RabbitMqChannelAgent.EnsureInitiated() is best-effort and can return without a channel two different ways: it bails early when the agent is disposed, and it logs-and-swallows a failure to open one. RabbitMqListener.CreateAsync() called `Queue.DeclareAsync(Channel!, Logger)` straight afterwards, so both outcomes surfaced as a bare NullReferenceException thrown from RabbitMqQueue.DeclareAsync -- six frames from the actual cause, at a line where nothing is wrong. The null-forgiving operator is what kept the compiler from saying so. Reproduced as the intermittent failure of Bug_189_fails_if_there_are_many_messages_in_queue_ on_startup: it fails on attempt 1 and passes on retry under the CIRabbitMQ target's worker processes, while passing 3/3 in isolation, because the race needs concurrent host lifecycle. CreateAsync now captures the channel once and checks it, which also closes a smaller hole -- reading the property five times in a row races a concurrent rebuild replacing it mid-method. Disposal during startup returns quietly, since a host that stops while its listeners are still coming up hits that routinely and there is nothing left to build against. A live agent with no channel throws InvalidOperationException naming the endpoint and queue, because that one is a real failure that EnsureInitiated only logged. The settle path in this same file already learned this lesson -- CanSettle() captures Channel into a local and null-checks it, and its comment cites `Channel!` throwing a NullReferenceException mid-reconnect. The create path never got the same treatment. Tests, red on the unfixed listener with the reported NullReferenceException, green after: - disposed_during_startup_abandons_creation_quietly - a_live_agent_that_cannot_open_a_channel_throws_something_diagnosable, which disposes the transport's ListeningConnection so startNewChannel() genuinely fails. Nulling Channel by hand is not enough -- EnsureInitiated() would just open a fresh one and the branch under test would never run. Full CIRabbitMQ target: 492 passed, Bug_189 clean (the one remaining flake, multi_tenancy_through_virtual_hosts, is pre-existing tracked debt). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHAuhdWS3XeAk16swV9G8m
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 #3842.
The defect
RabbitMqChannelAgent.EnsureInitiated()is best-effort and can return without a channel two different ways — it bails early when the agent is disposed, and it logs-and-swallows a failure to open one:RabbitMqListener.CreateAsync()calledQueue.DeclareAsync(Channel!, Logger)immediately afterwards. The!stopped the compiler from objecting, andDeclareAsynctakes a non-nullableIChannel, so the null travelled six frames before anything dereferenced it:RabbitMqQueue.cs:341isawait channel.QueueDeclareAsync(...)— the first use of the parameter. Nothing there is wrong; it is just where the null finally lands.In the field this is any host that disposes a listener while it is still starting: a fast restart, a failed startup, a cancelled host. You get an unexplained NRE in queue declaration instead of "this listener was disposed during startup."
How it was found
Chasing the intermittent failure of
Bug_189_fails_if_there_are_many_messages_in_queue_on_startup. It passes 3/3 in isolation and fails on attempt 1 under theCIRabbitMQtarget's worker processes — the race needs concurrent host lifecycle, which is exactly why single-test reruns kept looking green.The change
CreateAsynccaptures the channel once and checks it. That also closes a smaller hole: reading the property five times in a row races a concurrent rebuild replacing it mid-method.InvalidOperationExceptionnaming the endpoint and queue, because that one is a real failure thatEnsureInitiatedmerely logged.EnsureInitiatednow documents that it does not guarantee a channel, so the next caller does not have to rediscover this.Worth noting the settle path in this same file already learned this:
CanSettle()capturesChannelinto a local and null-checks it, and its doc comment citesChannel!throwing aNullReferenceExceptionmid-reconnect. The create path never got the same treatment.Verification
Both tests were run against the unfixed listener first and fail with the reported
NullReferenceException; they pass after:disposed_during_startup_abandons_creation_quietlya_live_agent_that_cannot_open_a_channel_throws_something_diagnosableThe second test disposes the transport's
ListeningConnectionsostartNewChannel()genuinely fails. NullingChannelby hand is not enough —CreateAsynccallsEnsureInitiated()first, which would simply open a fresh channel, and the branch under test would never run while the test still passed.For the red baseline I reverted only
RabbitMqListener.cs, keeping the newIsDisposedaccessor on the agent — that is test-visible plumbing rather than the fix, and reverting it too would have broken compilation and proved nothing.Full
CIRabbitMQtarget after the fix: 492 passed,Bug_189clean. The one remaining flake,multi_tenancy_through_virtual_hosts, is pre-existing tracked debt.One honest caveat:
Bug_189is a race, so a clean run is encouraging rather than conclusive. The mechanism is confirmed and the tests pin it directly; whether it was the only mechanism behind that flake is a separate question — #3842 also records a second failure mode seen on CI (OperationInterruptedException/ channel-1KeyNotFoundinside the client's receive loop) that this PR does not claim to address.