Skip to content

GH-3950: quiesce a Rabbit channel that the broker has already closed - #3964

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-3950-quiesce-rejected-settle
Aug 16, 2026
Merged

GH-3950: quiesce a Rabbit channel that the broker has already closed#3964
jeremydmiller merged 1 commit into
mainfrom
gh-3950-quiesce-rejected-settle

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Addresses suggestion 3 of #3950. (Suggestion 2 shipped as #3961; suggestion 1 is the upstream SessionManager.Lookup defect and is not ours.)

When the broker rejects a settle with PRECONDITION_FAILED - unknown delivery tag, it has already closed the channel that tag arrived on. Wolverine logged the rejection and carried on, leaving deliveries streaming into a channel being torn down — which is precisely the condition that makes RabbitMQ.Client race itself: an inbound frame arrives for a channel number just removed from the session map, SessionManager.Lookup does an indexer read and throws KeyNotFoundException, and the client escalates that into a library-initiated close of the whole connection (code=541), taking down every listener and sender on it.

A rejected settle now cancels that channel's consumer, tears it down and rebuilds, through the existing ReconnectedAsync() path. Latched per channel generation so a burst of rejected settles on one dead channel triggers exactly one rebuild, and dispatched off the settle path because ReconnectedAsync takes the reconnect lock and does real broker work. Wired into all three sites that recognise the rejection.

What this does not do

It does not prevent the connection death. The KeyNotFoundException is raised on the client's own MainLoop after our ack has already gone out, and the issue's own measurements show a single poisoned tag killed the connection on 4 of 5 runs — far too early for anything here to intervene.

What it does is narrow the window for further frames on a channel already known to be dead, and get the listener back onto a healthy one sooner. I'd rather state that plainly than let the change read as a fix for the 541s. The real fix is upstream: SessionManager.Lookup should TryGetValue and drop frames for a dead channel rather than throwing.

About the test

The new test guards the hazard this change introduces, not the bug it mitigates.

#3391 is the precedent: a rebuild that only swaps the channel leaves a listener sitting on an open channel with zero consumers while still reporting Connected — silently dead. The poisoned message is redelivered by the broker either way, so no existing assertion would catch that; only a fresh batch published after the rebuild distinguishes "recovered" from "quietly stopped listening."

It also needed a poll-based wait helper: the existing WaitForAll completes a single TaskCompletionSource, so a second call in the same test returns immediately on the already-completed task and asserts against nothing. The first version of this test "passed" in 985ms that way.

Verification

  • stale_delivery_tag_settling 2/2, including the existing test
  • Full Wolverine.RabbitMQ.Tests suite results to follow in a comment

🤖 Generated with Claude Code

https://claude.ai/code/session_01JG8Un6iNeyXECKJk3jo5uC

When the broker rejects a settle with PRECONDITION_FAILED - unknown delivery tag, it
has ALREADY closed the channel that tag arrived on. Wolverine logged the rejection and
carried on, leaving deliveries streaming into a channel being torn down -- which is the
condition that makes RabbitMQ.Client race itself: an inbound frame arrives for a channel
number just removed from the session map, SessionManager.Lookup does an indexer read and
throws KeyNotFoundException, and the client escalates that into a library-initiated
close of the WHOLE connection (code=541), taking down every listener and sender on it.

A rejected settle now cancels that channel's consumer, tears it down and rebuilds,
through the existing ReconnectedAsync() path. Latched per channel generation so a burst
of rejected settles on one dead channel triggers exactly one rebuild, and dispatched off
the settle path because ReconnectedAsync takes the reconnect lock and does broker work.
Wired into all three places that recognise the rejection.

WHAT THIS DOES NOT DO: it does not prevent the connection death. The KeyNotFoundException
is raised on the client's own MainLoop after our ack has already gone out, and the issue's
own measurements show a SINGLE poisoned tag killed the connection on 4 of 5 runs -- far
too early for anything here to intervene. This narrows the window for further frames on a
channel already known to be dead and gets the listener back onto a healthy one sooner. The
real fix is upstream: SessionManager.Lookup should TryGetValue and drop frames for a dead
channel rather than throwing.

The new test guards the hazard this change INTRODUCES rather than the bug it mitigates.
#3391 is the precedent: a rebuild that only swaps the channel leaves a listener on an open
channel with ZERO consumers while still reporting Connected -- silently dead. The poisoned
message is redelivered by the broker either way, so only a fresh batch published AFTER the
rebuild distinguishes "recovered" from "quietly stopped listening".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JG8Un6iNeyXECKJk3jo5uC
@jeremydmiller

Copy link
Copy Markdown
Member Author

Full Wolverine.RabbitMQ.Tests suite on this branch: 503 passed / 504, with one failure in ConventionalRouting.end_to_end_with_conventional_routing.send_from_one_node_to_another_all_with_conventional_routing.

That test passes 3/3 when its class is run in isolation, so it looks like suite-level interference rather than a defect in this change — but that suite has a documented history of chronic first-round failures, and I'd rather not call it a flake from one data point. Running the same full suite against main with this change reverted to get an apples-to-apples baseline; will post the comparison here.

@jeremydmiller

Copy link
Copy Markdown
Member Author

Baseline comparison done — the failure is pre-existing on main, not caused by this change.

Same machine, same broker, same full-suite invocation:

total failed failing test
this branch 504 1 end_to_end_with_conventional_routing.send_from_one_node_to_another_all_with_conventional_routing
main, this change reverted 503 1 same test

(The totals differ by exactly the one test this PR adds, which confirms the two runs are distinct.)

So the full-suite result for this branch is 503 passed with one pre-existing failure, and stale_delivery_tag_settling is 2/2.

Worth filing separately: the handoff notes recorded this suite at 503/503 as recently as yesterday, so this test has started failing on main since — the candidates are the three RabbitMQ changes merged since (#3796, #3960, #3961). Recording that as a lead rather than a diagnosis; it is out of scope here.

@jeremydmiller
jeremydmiller merged commit 7f2595f into main Aug 16, 2026
37 checks passed
esond pushed a commit to esond/wolverine that referenced this pull request Aug 18, 2026
Covers JasperFxGH-3956 (store agnostic document contracts), JasperFxGH-3954 (durability agents no
longer assigned to incapable nodes), JasperFxGH-3953 (persistence provider owns the chain's
transaction), JasperFxGH-3955 (idle reaper latching durable endpoints), and the three RabbitMQ
changes: the community DrainWaitForPrefetch opt-in (JasperFx#3796), its non-terminal-stop
follow-up (JasperFx#3960), and the JasperFxGH-3950 pair (JasperFx#3961 coverage, JasperFx#3964 channel quiesce).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JG8Un6iNeyXECKJk3jo5uC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant