feat(asb)!: isolate retries with native queue scheduling

[gabisonia]

Description

Deferring a delivery from one Azure Service Bus topic subscription republishes it to every matching subscriber. Reusing the original MessageId on a retry queue with duplicate detection can also discard the retry while the original is acknowledged.

Direct subscriptions now abandon with the updated handled count. If a delay was requested, they warn and retry immediately, preserving startup compatibility without broadcasting. Opt-in ForwardTo provisions a dedicated queue and retains delayed retries; existing forwarding can also be managed externally with OnMissingChannel.Assume.

The implementation adds optional sync/async requeue-scheduler capabilities and an Azure Service Bus native implementation. Built-in queue producers schedule retries on their own broker by default, ahead of any global scheduler. Each attempt has a distinct, deterministic delivery ID and retains the first ID in x-original-message-id. The original is acknowledged only after scheduling succeeds. Ordinary Azure scheduled commands and requests keep their existing envelope behavior.

Compatibility: direct subscriptions cannot retain a delay until forwarding is configured. Abandon increments the broker delivery count; configure MaxDeliveryCount above Brighter's retry budget. Queue retry IDs change, and send/settlement remain nontransactional, so handlers must remain idempotent. Existing subscriptions are never automatically migrated. Custom direct-subscription receivers require the property-preserving abandon capability.

Related Issues

Fixes #4546. Fixes #4490. Related to the queue-targeted scheduler proposal in the issue discussion.

Design: docs/adr/0082-isolate-azure-service-bus-subscription-retries.md.
Deployment, rollback, and recovery: docs/guides/azure-service-bus-retries.md.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Validation

• 26 hardening cases pass on .NET 9 and .NET 10, covering duplicate detection, sessions, subscriber isolation, startup compatibility, cancellation, settlement failure, and broker ownership.
• Full .NET 10 ASB suite: 553 passed, 10 failed, 4 skipped. All ten failures exactly match the established baseline and reject TTL fixtures above the emulator's one-hour limit. No additional failures.
• Existing Azure scheduler tests: 13 passed. Generator tests: 312 passed. Generated retry conformance checks verify new delivery IDs and preserved original identity.
• Core, gateway, and scheduler release packages build for netstandard2.0, net8.0, net9.0, and net10.0. A separate .NET 8 application compiles against the exact local packages with zero warnings or errors.
• The regressions were observed failing before implementation; earlier settlement/identity mutations produced the intended failures and were reverted. git diff --check is clean.

Live Azure validation remains outstanding: least-privilege permissions, process/network recovery, representative load, and migration/rollback. The deployment guide defines these release acceptance checks. This PR is ready for review, but local emulator results do not establish production readiness. Public documentation publication and maintainer API/design approval remain pending.

Please merge with a merge commit rather than squash or rebase, as requested by the contribution guide.

[iancooper]

@gabisonia This is marked as draft at the moment?

[gabisonia]

@gabisonia This is marked as draft at the moment?

Yes, I created it as a draft PR to discuss the proposed approach before proceeding with the implementation. Could you please check the discussion in the linked issue first? #4546 (comment)

[iancooper]

It's worth us adding an issue to pick this up in ValidatePipelines as well

        if (sub.Configuration.ForwardTo is { } queueName)
        {
            if (sub.Configuration.UseServiceBusQueue || string.IsNullOrWhiteSpace(queueName)
                || Uri.TryCreate(queueName, UriKind.Absolute, out _))
            {
                throw new ConfigurationException(
                    "ForwardTo must name a queue in the same namespace and cannot be combined with UseServiceBusQueue.");
            }

[iancooper]

@claude Let's review this addition of a scheduler to allow us to requeue a message to the queue from which we were reading instead of to the topic (resulting in a broacast to all subscribers over a retry to a queue). We are looking for issues with the implementation of the requeue scheduler and the logic that tries this native requeue scheduler before falling back to any configured scheduler

[claude]

Claude finished @iancooper's task in 3m 18s —— View job

---

Review: native requeue scheduler and how it falls back

• Gather context (source changes, ADR 0082, Proactor/Reactor requeue path)
• Review IAmAMessageRequeueScheduler{Sync,Async} + AzureServiceBusRequeueScheduler
• Review consumer requeue path & fallback to configured scheduler
• Post findings

Note: the checkout is shallow and I couldn't run git fetch --unshallow, so git diff origin/master...HEAD had no merge base. I reviewed the files in the working tree and diffed against dba7b72^ instead. I didn't build or run any tests.

---

Summary

The main design works. With UseServiceBusQueue or ForwardTo, a retry is now scheduled straight onto the physical queue the consumer reads from. Each attempt gets its own deterministic MessageId, and the original is only completed after the broker accepts the schedule. That fixes the topic broadcast (#4546) and the duplicate-detection drop (#4490). Building the retry ID from (incoming MessageId, destination, updated HandledCount) is a good choice. If scheduling succeeds but the ack fails, the pump nacks, the message comes back with the same persisted handled count, and the second schedule gets the same ID, so duplicate detection drops it. A new delivery gets a new ID.

Most of my concerns are about the fallback logic. As wired today, the configured scheduler is effectively never used, and the precedence rule sits in a hidden cast.

---

1. The fallback to the configured Scheduler can't be reached with the built-in factory

AzureServiceBusConsumer.cs:379-400

if (!UseServiceBusQueue && ForwardTo is null) { ...abandon...; return true; }   // topic path exits here

var requeueScheduler = _messageProducer as IAmAMessageRequeueSchedulerAsync
    ?? Scheduler as IAmAMessageRequeueSchedulerAsync;

Going through AzureServiceBusConsumerFactory.Create (:79-125):

Path	Producer	Is the producer a requeue scheduler?	Is Scheduler consulted?
ForwardTo	AzureServiceBusQueueMessageProducer	yes	never
UseServiceBusQueue	AzureServiceBusQueueMessageProducer	yes	never
direct subscription	AzureServiceBusTopicMessageProducer	n/a	returns before the cast

As a result:

• IAmAChannelFactoryWithScheduler on AzureServiceBusChannelFactory and the Scheduler properties on the consumer factory and consumer add public API but change no behaviour on any built-in path. The schedulerMode cases 1 and 2 in When_scheduling_a_retry_should_target_its_queue_and_survive_duplicate_detection only show that the scheduler is ignored.
• No test exercises the fallback branch itself. It only runs if someone builds a consumer by hand with a custom producer.
• Ordinary schedulers (Quartz, Hangfire, InMemory) are skipped silently. Only an IAmAMessageRequeueSchedulerAsync counts, and nothing is logged. Someone who registers a global scheduler expecting it to own delayed retries won't be told.

Two options: (a) remove the Scheduler plumbing from this PR and add it when a real caller needs it, or (b) keep it, add a test that drives the fallback, and log at debug/warn when a configured scheduler is ignored. The CLAUDE.md rule on change scope leans towards (a).

2. The fallback contradicts its own comment about the namespace

AzureServiceBusConsumer.cs:391: "Native retries must stay on the consuming broker, regardless of a global scheduler's namespace." The next line then falls back to the global scheduler. The MessageScheduler.Azure AzureServiceBusRequeueScheduler sends with its own IServiceBusClientProvider (AzureServiceBusSchedulerFactory.cs:62). If that provider points at a different namespace, destination (a bare queue name) resolves against the wrong broker. The result is either MessagingEntityNotFound or, worse, a delivery to a queue with the same name in another namespace. ADR 0082 hands this to "custom components," but nothing checks it. If the fallback stays, either compare the scheduler's FullyQualifiedNamespace with the consumer's, or state in the XML docs that the scheduler must share the consumer's namespace.

3. The precedence rule depends on the producer secretly being a scheduler

AzureServiceBusQueueMessageProducer : …, IAmAMessageRequeueSchedulerAsync, IAmAMessageRequeueSchedulerSync. Since these interfaces extend IAmAMessageScheduler, the queue producer is now also an IAmAMessageScheduler. That mixes two roles in one object (see design_principles.md). The consumer then discovers the capability with an as cast on a field typed IAmAMessageProducer. The factory already has _clientProvider, so this would be clearer:

new AzureServiceBusQueueConsumer(sub, producer, admin, receiverProvider,
    requeueScheduler: new AzureServiceBusRequeueScheduler(_clientProvider))

The consumer would hold the requeue scheduler explicitly, the producer would go back to only producing, and the "local broker first" rule would show up in the constructor instead of a cast. This would also make finding 1 go away.

4. IAmAMessageRequeueSchedulerSync is new public core API with no caller

The consumer's sync Requeue runs RequeueAsync through BrighterAsyncContext, so only the async interface is ever checked (AzureServiceBusConsumer.cs:354, 392). The sync interface and its three implementations are unused. Either remove them, or have the sync path prefer the sync capability so the Reactor gains something from it.

5. Retry sends skip the producer's tracing and connection retry

The old path went through AzureServiceBusMessageProducer.SendWithDelayAsync, which had:

• BrighterTracer.WriteProducerEvent(...): retries no longer produce a producer span or event. That hurts observability exactly when things are failing.
• The Polly TopicConnectionRetryCount policy around _serviceBusSenderProvider.Get. The new scheduler calls _senderProvider.Get once with no retry.
• Wrapping errors in ChannelFailureException. The new path lets raw ServiceBusException through. The pump catches Exception, so nothing breaks, but the error type differs from every other producer failure.

The first is the one to fix. The other two are consistency points.

6. A new sender (AMQP link) is created and closed for every retry

AzureServiceBusRequeueScheduler.cs:96-105. The existing producer works the same way, so this isn't a regression. But retries spike during incidents, and opening and tearing down a link per failed message adds latency and load just when the broker is already under strain. A small cache of senders per destination, disposed with the scheduler, would help. This can be a follow-up.

7. The CloudEvents id changes on every retry

AzureServiceBusRequeueScheduler.cs:94 overwrites ce-id with the new broker ID. Brighter reads Message.Id from the broker MessageId (BrokeredMessageWrapper.Id), so the broker-side fix doesn't need this change. The CloudEvents spec says source + id identifies the event and a resent event may keep its id. Any downstream consumer that de-duplicates on ce-id will now treat each retry as a new event. I'd keep ce-id stable and change only the broker MessageId, unless this was a deliberate choice. The ADR says it "matches" but doesn't say why.

Smaller points

• Destination check (:74-76): on Linux, Uri.TryCreate("/foo", UriKind.Absolute, …) returns true because it's parsed as a file URI. That's harmless, since ASB names can't start with /, but the check doesn't do what it appears to. A subscription path such as topic/Subscriptions/a passes the check even though the interface docs forbid it. The broker will reject it, just later.
• TTL isn't carried over: ConvertToServiceBusMessage(message) is called without the original's TTL, so the retry gets the queue's default TTL. That's probably fine, but worth a sentence in the ADR.
• Sessions: SessionId is preserved, which is good. A delayed retry still goes to the back of its session, so later messages in that session are processed first. The behaviour hasn't changed, but the guide should mention it now that delayed session retries are a supported path.
• Repeated scheduling failure: after a failure the pump calls NackAsync, which uses plain AbandonAsync and doesn't persist the handled count. Brighter's count therefore stops moving and only the broker's MaxDeliveryCount limits the loop. That's acceptable, but it's another reason the guide's "set MaxDeliveryCount above Brighter's retry budget" advice matters.
• SHA256.Create() per call plus BitConverter.ToString(...).Replace("-", "") is fine for netstandard2.0. The ID is 79 characters, within ASB's 128-character limit.

What looks right

• The order is schedule then acknowledge. Cancellation goes to the schedule only. AcknowledgeAsync effectively ignores the token, so an ack can't be cancelled after a successful schedule.
• The input Message isn't modified, so the original lock token stays valid for the ack.
• x-original-message-id is set once and carried through, and the Proactor and Reactor already use it in their "dropping message" logs.
• Body bytes are copied from Body.Memory, so binary payloads survive.
• Lock token, sequence number and session ID are kept out of ApplicationProperties by ASBConstants.ReservedHeaders.

Recommendation

Decide what the Scheduler fallback is for (findings 1–3) before merging. My preference is to inject the requeue scheduler into the consumer explicitly and drop the unused Scheduler and sync plumbing for now. Ian's suggestion to also check ForwardTo / UseServiceBusQueue in ValidatePipelines would catch the namespace and configuration mistakes in finding 2 at startup.

[iancooper]

@gabisonia It may be worth thinking about this "native requeue" model a little deeper for other brokers. NServiceBus use a combination of TTL and a dead letter exchange to create a scheduled resend to a queue for example, which we could treat at a native scheduled requeue.

[iancooper]

@gabisonia This has grown in the doing, but its moving in a really positive direction. Some stuff to lok at from the Claude review:

1. The fallback to the configured Scheduler can't be reached with the built-in factory

See 3, but we should surface this option via the Publication i.e. you should choose if you want to use the native broker scheduler or a global one and that would push through to 3. It would also be easier to imply that there are steps, like ForwardTo, that come with that choice

2. The fallback contradicts its own comment about the namespace

If you solve 1, then you can allow the fallback to global to not follow this

3.The precedence rule depends on the producer secretly being a scheduler

This looks like a cleaner design, as it allows us to also more easily choose

4. 4. IAmAMessageRequeueSchedulerSync is new public core API with no caller

We should prefer to be "sync all the way down" or "async all the way down" so prefer to use sync options in Reactor

5. 5. Retry sends skip the producer's tracing and connection retry

Agreed, we should have consistency particularly around OTel

6. A new sender (AMQP link) is created and closed for every retry

This is worth fixing, but you can push to a new issue

7. The CloudEvents id changes on every retry

Agreed, worth remaining consistent

Worth reviewing the nits too

[gabisonia]

@claude Please re-review this PR after the latest updates. Revisit your previous findings and identify which are resolved or still outstanding. Pay particular attention to explicit native/global retry scheduler selection, sync/async compatibility, queue-path validation, retry failure and settlement behavior, duplicate detection, and trace propagation without modifying the original message. Check the tests for remaining gaps and report actionable findings with file and line references. Review only; do not modify code.

[claude]

Claude finished @gabisonia's task in 3m 47s —— View job

---

Re-review: native and global requeue scheduling

• Gather context (diff against origin/master, ADR 0082, guide, release notes)
• Check the earlier findings (1–7 plus the smaller points)
• Review native/global scheduler selection
• Review sync/async compatibility
• Review queue-path validation
• Review retry failure and settlement, and duplicate detection
• Review trace propagation and whether the original message is modified
• Look for test gaps
• Post findings

This is a read-only review. I didn't build the code or run any tests.

---

Summary

This is a real improvement. The producer no longer doubles as a scheduler. The requeue scheduler is now injected into the consumer, and the factory chooses it explicitly from Publication.RequeueScheduler. The factory also checks at channel creation that the scheduler supports the Reactor's sync path or the Proactor's async path. The CloudEvents id stays the same across retries. Telemetry and trace propagation run on a copy of the message, so the original is untouched. The retry ID is still derived deterministically from the incoming ID, the destination and the handled count.

Six of the seven earlier findings are resolved. The seventh, a new sender per retry, was deferred to a follow-up issue as agreed. What's left is mostly small and follow-up work. Two items are worth fixing before merge (A and B below).

---

Earlier findings: status

#	Finding	Status	Where
1	Fallback to the configured Scheduler couldn't be reached	✅ Resolved. RequeueSchedulerMode.Native/Global makes an explicit choice, and Global uses Scheduler. Global on a direct subscription, or with an incompatible or null scheduler, fails at channel creation.	AzureServiceBusConsumerFactory.cs:98-117
2	Fallback contradicted its own comment about the namespace	✅ Resolved by design. The mode's XML doc says Global uses "its broker connection". When_selecting_a_retry_scheduler_should_use_its_configured_broker shows the unselected broker isn't touched.	RequeueSchedulerMode.cs
3	Producer was secretly a scheduler	✅ Resolved. The consumer takes IAmAMessageScheduler? requeueScheduler, and the producer no longer implements the requeue interfaces.	AzureServiceBusConsumer.cs:64-76
4	Sync interface had no caller	✅ Resolved. The Reactor path calls IAmAMessageRequeueSchedulerSync.Requeue (AzureServiceBusConsumer.cs:361). The ASB native implementation still uses BrighterAsyncContext.Run (AzureServiceBusRequeueScheduler.cs:244-245), but that can't be avoided because the ASB SDK is async-only, and the rest of the gateway does the same.
5	Retries skipped tracing and connection retry	✅ Mostly resolved. There is now a producer event, a fallback to inject trace context, sender-creation retry, and ChannelFailureException wrapping. See C and D for what's left.	AzureServiceBusRequeueScheduler.cs:140-198
6	New AMQP link per retry	⏸️ Outstanding, deferred as agreed. Is there a tracking issue? I couldn't find one linked from the PR.	AzureServiceBusRequeueScheduler.cs:107-137
7	CloudEvents id changed per retry	✅ Resolved. ce-id is kept, falling back to the original ID, and the emulator test checks it.	AzureServiceBusRequeueScheduler.cs:103-105
nit	/foo parsed as a file URI; subscription paths accepted	✅ Resolved. AzureServiceBusQueuePath.IsValid rejects /Subscriptions/, $ segments and empty segments.	AzureServiceBusQueuePath.cs:31-35
nit	TTL not carried over	✅ Documented in the guide (azure-service-bus-retries.md:94-95).
nit	Delayed session retries can be reordered	✅ Documented in the guide (azure-service-bus-retries.md:91-92).
nit	Brighter's handled count stops moving after a scheduling failure	✅ Documented. The guide and the RequeueAsync remarks cover setting MaxDeliveryCount above Brighter's retry budget.
Ian	Validate ForwardTo/UseServiceBusQueue in ValidatePipelines	⏸️ Not in this PR. The checks run in CreateConsumer, so they happen at channel creation, not at pipeline validation. That still needs an issue.

---

New / remaining findings

A. Dead code: AzureServiceBusMessageProducer.DestinationOverride (AzureServiceBusMessageProducer.cs:77, 217)
Nothing in src/ or tests/ sets this internal property. It is left over from the earlier design, where the producer sent the retry. Per the change-scope rule in CLAUDE.md, it should be removed. Fix this →

B. Retry telemetry ignores the configured instrumentation options (AzureServiceBusRequeueScheduler.cs:164)
WriteProducerEvent(..., InstrumentationOptions.Messaging) is hard-coded. A user who sets InstrumentationOptions.None, or turns off header capture, still gets an event with the full serialised MessageHeaders, including the bag. The producer path respects _options (AzureServiceBusMessageProducer.cs:224). Two options:

• take InstrumentationOptions in the AzureServiceBusRequeueScheduler constructor and have AzureServiceBusConsumerFactory pass it through; or
• pass InstrumentationOptions.None for the tags and keep only the context injection.

Fix this →

C. The retry event records the failed delivery's ID, not the new one (AzureServiceBusRequeueScheduler.cs:88-101)
CopyForRetry keeps header.MessageId, and WriteRetryTelemetry(outgoing) runs before retry.MessageId is computed. So the event's messaging.message.id and cloudevents.event_id tags hold the incoming ID. That makes it hard to join the producer event to the consumer span of the redelivery. Compute the retry ID first and pass it to the event, or record it as an extra tag.

D. The sender-creation retry wraps an operation that doesn't touch the network (AzureServiceBusRequeueScheduler.cs:140-155)
ServiceBusClient.CreateSender is lazy. It doesn't open a link, and in practice it only throws ObjectDisposedException or argument exceptions, neither of which is transient. So the loop retries non-transient errors six times with a 100 ms pause. Meanwhile ScheduleMessageAsync, the call that actually touches the network, relies only on the client's ServiceBusRetryOptions. This matches the old producer's Polly policy, so it's consistent rather than wrong. But When_retry_sender_creation_fails_transiently_should_retry_before_acknowledging tests a failure mode the real SDK doesn't produce. Two options:

• drop the loop and document that broker retry comes from ServiceBusClientOptions.RetryOptions; or
• move the retry around ScheduleMessageAsync, which is safe because the ID is deterministic and duplicate detection makes a re-send idempotent.

E. Hand-built consumers find an incompatible scheduler only at runtime (AzureServiceBusQueueConsumer.cs:84-90, AzureServiceBusConsumer.cs:356-357, 405-406)
The public AzureServiceBusQueueConsumer constructor accepts any IAmAMessageScheduler, and the release notes tell people with manual constructor calls to update them. If someone passes an InMemoryScheduler, Requeue throws ConfigurationException from inside the pump. The pump catches it and nacks, the message is redelivered, and that repeats until MaxDeliveryCount. The factory check doesn't cover this path. Two options:

• in the constructor, reject a scheduler that implements neither requeue interface; or
• type the parameter more narrowly.

F. Publication.RequeueScheduler sits on the core Publication but only ASB reads it (Publication.cs:58)
Every transport's publication now exposes it, and setting Global on RMQ, SQS or Kafka does nothing without any warning. The XML doc says "for gateways that support queue-targeted requeue scheduling", which helps. The mode also applies to the whole consumer factory, not to each subscription. Two smaller points:

• Ian asked for this to go on the publication, so I'm noting it rather than asking for a change.
• It's worth a line in the guide that subscriptions needing different modes need separate AzureServiceBusChannelFactory instances, passed via Subscription.ChannelFactory.

G. Direct subscriptions log the warning on every retry (AzureServiceBusConsumer.cs:394-397, 419-423)
With RequeueDelay > 0 on a direct subscription, every failed message logs RetryDelayRequiresForwarding at Warning, and redelivery is now immediate (a hot retry loop up to MaxDeliveryCount). The loop is documented as a compatibility trade-off. The per-message warning will be noisy in an incident, though. Consider warning once per consumer, or once at CreateConsumer when sub.RequeueDelay > 0 and the subscription is direct.

H. Nit: unclear error when Global has no scheduler (AzureServiceBusConsumerFactory.cs:110-117)
If Scheduler is null, the error says "must implement IAmAMessageRequeueSchedulerAsync". "No scheduler is configured for RequeueSchedulerMode.Global" would be easier to act on.

---

Settlement, duplicate detection and the original message: verified

• Order. The retry is scheduled first, then the original is acknowledged (AzureServiceBusConsumer.cs:397-398). AcknowledgeAsync ignores its token, so cancellation can't stop the ack once scheduling has succeeded.
• Scheduling failure. A ChannelFailureException is thrown with no completion. The Proactor/Reactor then nack and wait DontAckDelay (Proactor.cs:553-563). The test asserts that the bag and ID are unchanged.
• Ack failure after scheduling succeeds. The redelivered original has the same (Id, destination, HandledCount), so it gets the same retry ID and queue duplicate detection drops it. When_acknowledgement_fails_should_reuse_the_scheduled_retry_identity checks this at unit level.
• Original untouched. CopyForRetry copies the header, the bag without local headers, and the baggage. The Baggage.Add fix lets propagation overwrite an existing key when the baggage is full. The emulator test checks that received.Id, TraceParent and Topic don't change.
• Trace. The Proactor's process span is still open during RequeueMessage (it ends at Proactor.cs:421). So Activity.Current is the consumer span and the retry's traceparent points to it, which the emulator test checks (activity.Id == redelivered.TraceParent).
• Forwarding validation. ValidateForwardingAsync handles the absolute sb:// URI that ASB returns, and rejects subscriptions that don't forward unless the mode is Assume.

---

Test gaps

1. Duplicate detection across a real redelivery isn't tested end to end. The identity test (When_retry_scheduling_fails_should_leave_the_original_unacknowledged.cs:80-116) calls Requeue twice on the same in-memory Message. It never checks that the handled count read back from ApplicationProperties after NackAsync produces the same ID. The emulator test …survive_duplicate_detection only shows the retry isn't dropped as a duplicate of the original (Azure Service Bus: on an entity with duplicate detection the requeued copy keeps its MessageId, so the broker drops it and the message is lost #4490). Suggested test: on a queue with duplicate detection, schedule, make the completion fail, nack, receive again, requeue again, and assert exactly one retry arrives.
2. No factory test for invalid UseServiceBusQueue routing keys. AzureServiceBusConsumerFactory.cs:99-100 rejects them, for example orders/Subscriptions/x or queue/$DeadLetterQueue, but the only tests cover ForwardTo and the scheduler's destination. Absolute URIs, leading or trailing /, and whitespace-only segments in IsValid aren't tested anywhere.
3. No test that instrumentation options are respected (finding B), and none that Activity.Current == null still delivers a retry carrying the original traceparent.
4. No test for Global with a null Scheduler on a sync channel. The reject test only covers async, or incompatible schedulers (When_global_retry_scheduling_has_no_compatible_scheduler_should_reject_configuration.cs). Please confirm both pump types are covered.
5. When_retry_sender_creation_fails_transiently… tests a failure the SDK doesn't produce (finding D). If the loop stays, the test should at least use an exception type CreateSender can actually throw.
6. AbandonForRetryAsync failures aren't tested (AzureServiceBusConsumer.cs:414-436). For example, MessageLockLost on a direct-subscription abandon, and the "receiver doesn't implement IAmAServiceBusRetryReceiver" branch.

---

Recommendation

The scheduler selection, sync/async wiring, settlement order and identity handling now look correct. Before merge, I'd fix A (dead code) and B (telemetry options), and add test gap 1. C–H can be done here or tracked as follow-ups alongside the issues for the sender cache (#6) and the ValidatePipelines check.

[iancooper]

@gabisonia This has come a long way. Thanks for sticking with it.

A-C definitely need fixing. Up to you if you tackle the others. Test 1 would be useful.

[gabisonia]

@claude Please re-review the latest commits and assess whether this PR is ready to merge and suitable for production use. Review only; do not modify code.

The latest changes address findings A–H from your previous review:

I also checked Ian's latest comment: #4559 (comment). His required fixes A–C and suggested Test 1 are included; D–H are addressed as well.

• Removed the unused producer destination override.
• Threaded instrumentation options through native and global retry schedulers, with the scheduled broker delivery ID and stable CloudEvents ID reported separately.
• Removed retries around lazy sender construction; network retries use the Azure SDK configuration.
• Added manual queue-consumer scheduler validation and a specific missing-global-scheduler error.
• Limited the direct-subscription delay warning to once per consumer.
• Documented factory-wide selection, separate factories for different subscription modes, and the current ASB-only scope.

Regression coverage includes actual broker redelivery after scheduling succeeds but completion fails, duplicate detection, invalid queue paths, instrumentation and trace propagation without an active Activity, sender construction failures, direct abandon failures, and receivers without property-preserving abandon support. Characterisation tests were checked with deliberate production-code mutations, all subsequently restored.

Please verify these fixes against the final code, revisit any remaining test gaps, and identify any production blockers involving message loss, unintended broadcasts, duplicate detection, settlement ordering, cancellation, telemetry privacy, or sync/async compatibility. Report actionable findings with severity and file/line references, and explicitly state whether you recommend merging.

Sender caching and broader ValidatePipelines diagnostics remain documented follow-up work, as previously agreed.

Validation before the remote master merge: 149 retry tests passed on net10.0; 48 focused cases and 13 Azure scheduler tests passed on net9.0. The full ASB net10.0 suite reported 628 passed, 10 failed, and 4 skipped. The ten failure names exactly match the pre-change baseline and concern the emulator's one-hour TTL limit. No live Azure validation was performed.

The remote master merge has now been incorporated locally without conflicts. The all-target Release build passed again with zero warnings and errors. The final retry-focused net10.0 run finished with 155 passed and one failed: ASBConsumerTests.When_requeueing_a_message_via_the_consumer, the same DefaultMessageTimeToLive emulator-limit failure present in the baseline. There were no new failures. Both sync/async broker-redelivery cases and all ten instrumentation cases passed in this final run.

[claude]

Claude finished @gabisonia's task in 2m 32s —— View job

---

Final re-review: native and global requeue schedulers

• Gather context (diff against origin/master, latest commits e4f2d4c, 405b669, 761135c, merge 4b46147)
• Check the fixes for A–H against the final code
• Look for production blockers: message loss, broadcast, duplicate detection, settlement, cancellation, telemetry privacy, sync/async
• Revisit the test gaps
• Post the findings and a merge recommendation

This review is read-only. I didn't build the code or run any tests, so I'm relying on your test results.

---

Verdict

I recommend merging once the low-severity documentation items below are fixed. I found no production blockers in the code. A–C are fixed, D–H are handled, and suggested test 1 now runs against a real broker redelivery for both pump types.

"Suitable for production" has the same limits as before. Nothing has been checked against live Azure: permissions, recovery, load, and migration and rollback are untested. The guide's acceptance checks should be run before rolling this out to a production namespace. That is a release gate, not a merge blocker.

---

Previous findings A–H: status

#	Status	Evidence
A. DestinationOverride dead code	✅ Fixed	AzureServiceBusMessageProducer.cs only adds a local destination variable now (a cosmetic leftover; it could revert to message.Header.Topic.Value).
B. Telemetry ignored instrumentation options	✅ Fixed	AzureServiceBusRequeueScheduler.cs:155-181 gates each tag group the same way BrighterTracer.WriteProducerEvent does. The setting is passed in through AzureServiceBusConsumerFactory.cs:74-79,117 (native) and AzureServiceBusSchedulerFactory.InstrumentationOptions (global).
C. Event recorded the old delivery ID	✅ Fixed	The retry ID is computed first (:97-103). MessageId gets retryId and CeMessageId gets the stable CloudEvents ID (:163, 176).
D. Retry around lazy CreateSender	✅ Fixed	The loop is gone (:114-129). The guide (:104-107) says network retries come from ServiceBusClientOptions.RetryOptions.
E. Hand-built consumer with incompatible scheduler	✅ Fixed	AzureServiceBusQueueConsumer constructor rejects schedulers implementing neither capability. A scheduler that implements only one is still found at runtime on the other pump type, which is acceptable because the factory checks this precisely.
F. Factory-wide mode	✅ Documented
G. Per-message warning	✅ Fixed	Interlocked.Exchange once per consumer (AzureServiceBusConsumer.cs:409-413).
H. Error when no scheduler is configured	✅ Fixed	AzureServiceBusConsumerFactory.cs:120-121.

---

Production-risk checklist

Area	Assessment
Unintended broadcast	✅ Fixed. Direct subscriptions abandon in place (AzureServiceBusConsumer.cs:415-437). The queue paths schedule onto ForwardTo or the queue routing key (:407). No retry is sent to the topic anymore.
Settlement order	✅ Schedule first, then acknowledge, on both the sync (:362-363) and async (:398-399) paths. If scheduling fails, ChannelFailureException is thrown and the original is not completed. The Proactor then calls NackAfterRequeueFailureAsync (Proactor.cs:557-563).
Message loss	✅ No loss path in the code. The original is completed only after the broker accepts the scheduled retry. The remaining risks are in topology; see L2.
Duplicate detection	✅ The ID is a hash of (incoming ID, destination, handled count). The new end-to-end test, When_completion_fails_should_deduplicate_retry_after_broker_redelivery, injects a completion failure, nacks, receives the redelivery with HandledCount == 0, requeues again, and asserts that only one retry arrives. That is exactly the test I asked for. See L1 for the caveat.
Cancellation	✅ The token is checked before abandon or scheduling. OperationCanceledException is rethrown unwrapped. AcknowledgeAsync ignores the token, so an accepted retry is always followed by an ack attempt.
Telemetry privacy	✅ Same gating as the producer path. Local-only headers are dropped (BagWithoutLocalHeaders), and lock token, sequence number and session are reserved. With None an empty-tag event is still recorded, which matches GetNewTagsCollection.
Trace propagation	✅ Done on a copy (CopyForRetry, :218-238). Each header is set in its own try (:205-216). With no Activity, the original traceparent is kept, and a test covers this.
Sync/async	✅ The Reactor calls IAmAMessageRequeueSchedulerSync, and the Proactor calls the async interface. The factory checks the capability matching the pump (AzureServiceBusConsumerFactory.cs:124-131). The native sync path uses BrighterAsyncContext.Run, which can't be avoided because the ASB SDK is async-only.
Queue-path validation	✅ AzureServiceBusQueuePath.IsValid is applied to ForwardTo, to the UseServiceBusQueue routing key, and inside the scheduler. A test shows that a queue named subscriptions/<guid> is correctly accepted.

---

Remaining findings

L1 (Low, docs): queues that Brighter creates don't have duplicate detection, so the ack-failure dedup protection depends on how the operator provisions the queue.
AdministrationClientWrapper.CreateQueueAsync(string, AzureServiceBusSubscriptionConfiguration) (AdministrationClientWrapper.cs:97-105) never sets RequiresDuplicateDetection, and AzureServiceBusSubscriptionConfiguration has no option for it. So with ForwardTo + OnMissingChannel.Create, an ack failure after a successful schedule produces two retries. The new test uses a queue it creates itself with RequiresDuplicateDetection = true (When_completion_fails…:51-56). The guide (:84-86, step 3 at :133) implies the protection but never says that Create doesn't enable it. Handlers already have to be idempotent, so this isn't a correctness bug, but the guide should state it plainly. Adding a configuration option would change scope and can be a follow-up.
Fix this →

L2 (Low, docs): a forwarding queue with a finite QueueIdleBeforeDelete can be deleted automatically while its subscription keeps forwarding.
AdministrationClientWrapper.cs:146-151 correctly pins the subscription's AutoDeleteOnIdle to MaxValue when ForwardTo is set. The destination queue still gets QueueIdleBeforeDelete (:104). If a user sets a finite value and the consumer and publishers are idle long enough, the queue is deleted, and the subscription then forwards to an entity that doesn't exist. Messages end up in the source's dead-letter queue, not lost silently, but this is an outage. It is also related to PurgeAsync, which deletes and recreates the queue (AzureServiceBusForwardingConsumer.cs:142-155). I'd add one sentence to the ForwardTo remarks or the guide: "do not set a finite QueueIdleBeforeDelete with ForwardTo". Rejecting it in CreateConsumer would change behaviour, so that needs a decision from the maintainers.

L3 (Low, docs): the release notes are out of date and leave out the main behaviour change.

• release_notes.md:11 still says retries "retry sender acquisition". D removed that loop.
• The section covers scheduler selection and changes to how consumers are constructed. It doesn't mention the change that affects users: direct topic subscriptions no longer honour RequeueDelay (they now abandon immediately and log a warning), ForwardTo is the opt-in for delayed retries, queue retry MessageIds change on every attempt, and MaxDeliveryCount must be set above Brighter's RequeueCount. The PR is marked ! (breaking), so these belong in the release notes, not only in the guide.

Fix this →

N1 (Nit): repeated schedules within one consumer. AzureServiceBusRequeueScheduler.cs:97 calls SHA256.Create() for every retry. That's fine. A static SHA256.HashData isn't available on netstandard2.0, so leave it.

---

Test coverage: earlier gaps

Gap	Status
1. Duplicate detection across a real redelivery	✅ When_completion_fails_should_deduplicate_retry_after_broker_redelivery (sync and async)
2. Invalid UseServiceBusQueue routing keys	✅ When_forwarding_to_a_subscription_path_should_reject_configuration plus the valid-path test above
3. Instrumentation options, and no Activity	✅ When_retrying_should_respect_instrumentation_options, When_retrying_without_an_activity_should_preserve_the_original_trace_context
4. Global with a null scheduler on both pumps	✅ When_global_retry_scheduling_has_no_compatible_scheduler_should_reject_configuration
5. Sender-creation test used an unrealistic failure	✅ Replaced with When_retry_sender_creation_fails_should_leave_the_original_unacknowledged
6. AbandonForRetryAsync failures	✅ When_abandoning_a_direct_retry_fails… and When_a_direct_retry_receiver_lacks_property_preserving_abandon…

Remaining gap (optional): no test covers a ForwardTo queue created with Create and then a completion failure. It would document L1's behaviour, which is two deliveries without duplicate detection.

---

Follow-ups already agreed (please make sure issues exist before merge)

• Cache senders per destination (earlier finding 6).
• Add ForwardTo / UseServiceBusQueue checks to ValidatePipelines (Ian's request).
• Optional: a configuration option for RequiresDuplicateDetection on Brighter-created queues (L1), and a guard on QueueIdleBeforeDelete with ForwardTo (L2).
• Native requeue for other brokers, for example RMQ TTL plus a dead-letter exchange (Ian's comment).

[iancooper]

Thanks, @gabisonia I am going to begin merging this one now