Skip to content

feat: Allow waiting for prefetched messages on drain - #3796

Merged
jeremydmiller merged 5 commits into
JasperFx:mainfrom
simplisafe-oss:rabbit-drain-wait-for-prefetch
Aug 16, 2026
Merged

feat: Allow waiting for prefetched messages on drain#3796
jeremydmiller merged 5 commits into
JasperFx:mainfrom
simplisafe-oss:rabbit-drain-wait-for-prefetch

Conversation

@benjamin-alexander-simplisafe

@benjamin-alexander-simplisafe benjamin-alexander-simplisafe commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Adds an option to wait for prefetched messages in rabbitmq to be passed to the receiver before continuing with the shutdown process. Disabled by default to maintain parity in existing code, especially with the default 100 message prefetch (where this could be harmful).

There is a caviat where this won't necessarily handle parallel dispatch cleanly, as the cancel is handled in parallel with real messages, which has been documented.

Copilot AI review requested due to automatic review settings August 3, 2026 15:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an opt-in RabbitMQ listener behavior to wait for prefetched messages already sitting in the RabbitMQ client’s internal dispatcher buffer to be delivered to the consumer during shutdown (“drain”), reducing “silent” redeliveries when a channel is closed while messages are prefetched but not yet handed to the receiver.

Changes:

  • Introduces a per-listener configuration toggle (DrainWaitForPrefetch) and fluent config API to enable it.
  • Tracks client-dispatched basic.cancel-ok in the RabbitMQ consumer to support “wait for dispatcher to drain” semantics.
  • Updates listener shutdown to conditionally use BasicCancelAsync(..., noWait: false) and await the cancel-ok dispatch signal.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqListenerConfiguration.cs Adds fluent configuration method to enable waiting for prefetched-message drain during shutdown.
src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/WorkerQueueMessageConsumer.cs Adds a completion signal that fires when basic.cancel-ok is dispatched by the client.
src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqQueue.cs Adds queue-level boolean option (DrainWaitForPrefetch) with documentation.
src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqListener.cs Implements conditional no-wait cancel + optional wait-for-cancel-ok during stop.
Suppressed comments (1)

src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqListenerConfiguration.cs:121

  • The XML doc comment for ConsumerDispatchConcurrency is malformed (missing the opening tag), which can break generated docs and may trigger XML doc warnings/errors.
    ///     Override the RabbitMQ client's consumer dispatch concurrency for just this endpoint's
    ///     listening channels. This governs how many deliveries the client hands to the consumer
    ///     at once: at the default of 1, an <c>Inline</c> listener runs strictly one message at a
    ///     time through the handler no matter how MaxDegreeOfParallelism is set. Raising it is the
    ///     per-endpoint alternative to the transport-wide
    ///     <c>ConfigureChannelCreation(o =&gt; o.ConsumerDispatchConcurrency = n)</c>. See GH-3492.
    /// </summary>

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqListener.cs Outdated
Comment thread src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqListener.cs Outdated
@benjamin-alexander-simplisafe
benjamin-alexander-simplisafe marked this pull request as draft August 3, 2026 15:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqListenerConfiguration.cs:119

  • The XML doc comment for ConsumerDispatchConcurrency is missing its opening tag, which makes the documentation malformed (the block ends with but never starts). Add the missing opening tag so tooling generates correct docs.
    ///     Override the RabbitMQ client's consumer dispatch concurrency for just this endpoint's
    ///     listening channels. This governs how many deliveries the client hands to the consumer
    ///     at once: at the default of 1, an <c>Inline</c> listener runs strictly one message at a
    ///     time through the handler no matter how MaxDegreeOfParallelism is set. Raising it is the
    ///     per-endpoint alternative to the transport-wide

src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqListener.cs:178

  • The new DrainWaitForPrefetch shutdown behavior is subtle (timing-sensitive and dependent on RabbitMQ dispatch semantics), but there is no integration test coverage for it. Adding a focused RabbitMQ test around StopAndDrainAsync with prefetched messages would help prevent regressions.
            using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
            try
            {
                foreach (var consumerTag in consumer.ConsumerTags)
                {
                    await channel.BasicCancelAsync(consumerTag, false, cts.Token);
                }

                await consumer.CancelOkReceived.WaitAsync(cts.Token);

Comment thread src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/RabbitMqListener.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Transports/RabbitMQ/Wolverine.RabbitMQ.Tests/drain_wait_for_prefetch.cs:126

  • This test waits exactly 30 seconds for StopAndDrainAsync, but the implementation bounds the new drain wait by DurabilitySettings.DrainTimeout (default 30s) and StopAndDrain can legitimately take that long (or longer with additional drain steps). Using an equal timeout makes this test more likely to be flaky under load/slow CI.
        await stopTask.WaitAsync(30.Seconds(), TestContext.Current.CancellationToken);

src/Transports/RabbitMQ/Wolverine.RabbitMQ/Internal/WorkerQueueMessageConsumer.cs:84

  • XML doc comment has a grammatical error (“so once all are in the prefetch backlog has drained”) and is hard to parse. This is public-facing documentation for a shutdown guarantee, so it should read cleanly.
    /// Wait until the broker has replied cancel-ok for <paramref name="count"/> cancelled consumer
    /// tags. Each cancel-ok is dispatched by the client only after every prefetched delivery ahead of
    /// it in the FIFO dispatcher queue has been processed, so once all are in the prefetch backlog has
    /// drained (to the batching channel, in durable micro-batching mode).

@jeremydmiller

Copy link
Copy Markdown
Member

@benjamin-alexander-simplisafe Are you ready for me to review and maybe get this in today? There's yet another release brewing up this morning

@jeremydmiller
jeremydmiller merged commit 8a91eda into JasperFx:main Aug 16, 2026
34 checks passed
jeremydmiller added a commit that referenced this pull request Aug 16, 2026
…op (#3960)

* GH-3796 follow-up: make the prefetch drain safe for a non-terminal stop

DrainWaitForPrefetch (#3796) completes the consumer's BatchingChannel inside
RabbitMqListener.StopAsync. StopAsync is not always a terminal shutdown:
RequeueContinuation stops the listener inline from the handler pipeline and the
background PauseAsync then stops the SAME consumer again before disposing it, so
a durable listener genuinely sees two StopAsync calls against one channel.

Measured what a JasperFx BatchingChannel actually does after Complete(), rather
than assuming: TriggerBatch, Complete, WaitForCompletionAsync and PostAsync all
quietly do nothing. Nothing throws. That makes the second stop harmless but not
free, and it makes the window between Complete() and Dispose's latch the real
problem -- a delivery landing there is posted into a completed channel and
SILENTLY DISCARDED. It never reaches the receiver and is never acked, so the
broker redelivers it on channel close: exactly the redelivery DrainWaitForPrefetch
exists to prevent, arrived at through a silent path.

DrainBatchedDeliveriesAsync now latches BEFORE completing, so such a delivery is
rejected-with-requeue through the existing guard in HandleBasicDeliverAsync
instead of vanishing. Safe because the drain only runs after cancel-ok, and the
broker sends no further deliveries on a cancelled consumer. The drain is also
idempotent now, so the second stop does not re-run the cancel-ok wait against an
already drained channel, and Dispose skips a TriggerBatch that would do nothing.

SqsListener sidesteps this ordering entirely -- StopAsync only calls
TriggerBatch, and Complete lives in the dispose-time flush, with a comment
naming the paused-listener case. The Rabbit drain has to complete earlier than
dispose to beat LatchReceiver, so the state is tracked instead.

The new test pins the double-stop sequence end to end. It is a regression guard,
not a red-first repro -- verified passing both with and without the fix, because
the channel tolerates the duplicate calls. What it protects is that everything
still drains when StopAsync runs twice, which a naive "guard the second
Complete" fix would break.

Also fixes an unrelated chronic flake in the same suite, folded in because it
failed two consecutive full-suite runs and would otherwise redden this PR's own
CI: multi_tenancy_through_virtual_hosts.send_message_to_a_specific_tenant waited
only for the REQUEST to reach node two, then asserted on the RESPONSE arriving
back at main. The trace showed the response Sent in the same millisecond the
completion condition was satisfied. It now waits for the response too.

Local: Wolverine.RabbitMQ.Tests 502/502 -- the first fully clean run of this
suite in this session, where the two runs before the flake fix were 501/502 and
498/499 with that same test failing. wolverine.slnx -c Release -f net9.0 clean.

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

* Reconcile the GH-3763 standing note with the multi-tenancy race fix

The class note in multi_tenancy_through_virtual_hosts records a prior inference
from CI run 30856898284 that "the request never produced its response". The local
full-suite trace behind the previous commit shows the opposite: node two DID send
MultiTenantResponse, in the same millisecond the tracked session's completion
condition was satisfied. Leaving the two accounts side by side without comment
would have the file arguing with itself.

Also walks back the "chronic flake fixed" framing. One contributing race is
identified and fixed; the fixed-queue-name interference the note has been tracking
since GH-3763 is untouched, and two failures plus one clean run is evidence rather
than proof. If that interference bites now, the symptom changes from "no messages
of type MultiTenantResponse" to a 15s timeout, which the note now says.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

3 participants