Skip to content

GH-4012: share one settle budget across the stacked ack retry blocks - #4014

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-4012/ack-attempts-shared-budget
Aug 23, 2026
Merged

jeremydmiller merged 1 commit into
mainfrom
gh-4012/ack-attempts-shared-budget

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Addresses items 1 and 2 of #4012. Items 3-5 (per-transport terminal classification, broker-side redelivery-count bounding, JasperFx RetryBlock.ShouldRetry) stay open on that issue.

The multiplication bug

RetryBlock.MaximumAttempts bounds retries within one PostAsync. The durable completion path stacks two of them. Traced end to end rather than taken from the analysis doc:

DurableReceiver._completeBlock                              (RetryBlock, max 3)
  -> RabbitMqListener.CompleteAsync(Envelope)          :222
  -> RabbitMqChannelCallback.CompleteAsync             :63
  -> RabbitMqChannelCallback.Complete                        (RetryBlock, max 3)
  -> RabbitMqEnvelope.CompleteAsync                    :37
  -> RabbitMqListener.CompleteAsync(RabbitMqEnvelope)  :512  -> BasicAckAsync

Three by three is nine broker round trips for a single delivery, and neither block can see the other's count.

The fix

Envelope.AckAttempts rides the envelope so the layers share a budget — DurabilitySettings.MaximumAckAttempts, default 3.

Where the increment lives matters. It belongs to the innermost layer that actually issues the broker call, because that is the only layer that knows a round trip really happened. Outer layers only check. That has a deliberate consequence worth a reviewer's eye:

A transport that settles directly in Listener.CompleteAsync — no inner retry block — never increments, so the outer guard never trips and its behavior is byte-for-byte what it was. RabbitMQ gets the fix; every other transport is untouched until item 3 extends it per transport.

Exhausting the budget swallows rather than throws. An unsettled delivery is the recoverable outcome — the broker redelivers and the durable inbox deduplicates — and it is the same recovery the existing unknown-delivery-tag branch already relies on. Throwing would just feed the enclosing RetryBlock.

Honest scope: this cannot bound a redeliver → dedupe → re-ack loop. Every redelivery constructs a brand new envelope, so the counter starts over. Only a broker-side delivery count (redelivered/x-death, ApproximateReceiveCount, DeliveryCount) can do that — item 4, and it is called out in the xml-docs so the next reader doesn't over-trust the counter.

The pooling guard blind spot

envelope_pool_tests.Reset_zeroes_every_settable_property reflected with default binding — public only — which left every internal { get; set; } property on Envelope outside the guard. Widened to NonPublic, and the internal properties are now stamped in the arrange block so the assertions aren't vacuous.

That blind spot was not theoretical. It immediately caught BatchPendingSettled missing from Envelope.Reset() since CritterWatch#942.

Reachability, checked before claiming anything: Batch is only ever assigned in the new Envelope(IEnumerable<Envelope>) constructor, never on a pooled envelope, and SettleBatch early-returns when Batch == null. So a stale flag cannot bite today — this is a latent gap, not a live bug. Fixed anyway, because it is exactly the drift the guard exists to catch and the next batching change could make it reachable.

Tests

  • shared_ack_attempt_budget — a listener that spends the whole budget in one call (the Rabbit shape) is called once, not three times; a non-participating transport keeps its existing retry behavior; the budget is configurable.
  • envelope_ack_attempt_budget — the counter refuses past the maximum and does not keep incrementing after refusal.
  • The widened pooling guard covers AckAttempts and BatchPendingSettled.

Red-checks performed on both halves:

  • Setting maximumAckAttempts = int.MaxValue (guard disabled) fails 2 of 3 budget tests.
  • Removing the BatchPendingSettled = false; line produces Envelope.BatchPendingSettled not reset: expected False, got True.

Verification

  • dotnet build wolverine.slnx -c Release -f net9.0 — clean
  • Full CoreTests: 2487 passed, 0 failed, 2 skipped
  • Full Wolverine.RabbitMQ.Tests against docker-compose Rabbit: 506 passed, 0 failed (11m14s). Worth noting both of the failures I'd expected from prior runs — multi_tenancy_through_virtual_hosts and end_to_end.use_direct_exchange_with_binding_key — passed here too.

Not covered

No Rabbit integration test of the budget: forcing repeated real ack failures needs broker-side channel manipulation beyond what the existing harness offers. The arithmetic is pinned at the DurableReceiver seam instead, which is where the multiplication actually happened.

RetryBlock.MaximumAttempts bounds retries within a single PostAsync, but the
durable completion path stacks two of them:

  DurableReceiver._completeBlock
    -> RabbitMqListener.CompleteAsync(Envelope)
    -> RabbitMqChannelCallback.Complete
    -> RabbitMqEnvelope.CompleteAsync
    -> RabbitMqListener.CompleteAsync(RabbitMqEnvelope) -> BasicAckAsync

Three by three is nine broker round trips for one delivery, with neither block
able to see the other's count.

Envelope.AckAttempts rides the envelope so the layers share a budget
(DurabilitySettings.MaximumAckAttempts, default 3). The increment belongs to the
innermost layer that actually issues the broker call, since that is the only one
that knows a round trip occurred; outer layers only check. A transport that
settles directly in Listener.CompleteAsync never increments, so its guard never
trips and its behavior is unchanged -- extending it per transport is #4012 item 3.

Exhausting the budget swallows rather than throws. An unsettled delivery is the
recoverable outcome: the broker redelivers and the durable inbox deduplicates,
the same recovery the unknown-delivery-tag branch already relies on. This cannot
bound a redeliver -> dedupe -> re-ack loop, because every redelivery constructs a
new envelope; only a broker-side delivery count can, which is item 4.

Also widens the envelope pooling drift guard to NonPublic. The default binding is
public-only, which left every `internal { get; set; }` property outside the
guard, and that blind spot was real: BatchPendingSettled had been missing from
Envelope.Reset since CritterWatch#942. Not reachable today -- Batch is only ever
assigned in the `new Envelope(IEnumerable<Envelope>)` constructor, never on a
pooled envelope -- so it is a latent gap rather than a live bug, but it is exactly
the drift the guard exists to catch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HxbBpN6PmRB5CMSSTdGNr3
@jeremydmiller
jeremydmiller merged commit e235a1b into main Aug 23, 2026
39 checks passed
erdtsieck pushed a commit to erdtsieck/wolverine that referenced this pull request Aug 25, 2026
…with the broker's own count

Items 1-3 shipped in JasperFx#4014 and JasperFx#4106. This is the lever none of them could be: item 1's
Envelope.AckAttempts explicitly cannot bound this loop, because every redelivery arrives as
a brand new RabbitMqEnvelope / AzureServiceBusEnvelope with a fresh counter. Only the
broker remembers across that boundary.

The loop, from the original field report: a delivery that can never be settled (a
channel-scoped tag whose channel is gone), an inbox that deduplicates it on arrival, a
settle that fails again, and a broker that delivers it once more. Sustained indefinitely by
fresh arrivals rather than one immortal envelope.

Envelope.BrokerDeliveryCount (nullable) plus DurabilitySettings.MaximumBrokerRedeliveries,
defaulting to 0 = off so this is additive for every existing application. Enforcement sits
in DurableReceiver.handleDuplicateIncomingEnvelope, which is exactly where the loop turns:
past the limit the delivery goes to the dead letter queue instead of being settled again,
falling back to the ordinary settle if the dead letter move itself fails.

Populated from each transport's native signal:

- Azure Service Bus: ServiceBusReceivedMessage.DeliveryCount, always present.
- Amazon SQS: ApproximateReceiveCount, which needed MessageSystemAttributeNames added to
  the receive request -- SQS returns only the attributes a receive names, so the count was
  invisible before regardless of anyone wanting it.
- RabbitMQ: the x-death count, and ONLY that. The redelivered flag is a boolean -- "not the
  first delivery" and nothing more -- so x-death is the only real count RabbitMQ carries,
  present on dead-letter-and-retry topologies where a loop actually accumulates. Left null
  on plain nack-requeue rather than fabricating a 2, which would bound the wrong thing on
  the very first requeue.

SQS and RabbitMQ read the count AFTER the envelope mapper runs, so a custom user mapper
cannot clear Wolverine's own bookkeeping.

Seven tests. Four cover the predicate: the off-by-one (the limit itself is not past it),
default-off, and the null case for transports with no signal. Two drive the real duplicate
path through a stubbed inbox -- one asserting an over-delivered duplicate is dead lettered
and NOT re-acked, and a control asserting a duplicate within the limit still settles the old
way. Without the control the first would pass just as well if the bound fired on every
duplicate.

Verified: full solution builds Release/net9.0 clean; CoreTests 2644/0, the 2637 baseline
plus these seven.

Item 5 (JasperFx RetryBlock.ShouldRetry) remains, and is cross-repo.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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