fix(#4196): forward dead-letter reason and description to Azure Service Bus native fields

[DevJonny]

Fixes #4196.

When a handler rejects a message consumed from Azure Service Bus, AzureServiceBusConsumer.RejectAsync computed the rejection reason and description, logged them, then settled via the no-reason DeadLetterAsync overload — leaving the broker's native DeadLetterReason / DeadLetterErrorDescription fields blank. Operators triaging the DLQ saw empty columns and had to pivot to logs and correlate by sequence number.

git blame shows the reason/description were introduced in #3918 (Universal DLQ) and wired only into the log line; the no-reason settle predates it and wasn't updated. As confirmed on #4196, this is an oversight, and forwarding to the native overload is the intended fix (consistent with ADR 0045's "defer to native").

Change

• New DeadLetterAsync(lockToken, reason, description) overload on the public IServiceBusReceiverWrapper (per maintainer steer on ASB: rejected messages dead-lettered with blank reason/description #4196 — interface addition accepted; DIM not viable under netstandard2.0).

• Implemented via the existing message shiv against the SDK's native DeadLetterMessageAsync, truncating to ASB's 4096-char limit.

• RejectAsync forwards the values it already computes; sync Reject inherits via BrighterAsyncContext.Run.

  Testing

  • Developer tests (no mocks, fake wrapper) for both Proactor (async) and Reactor (sync) consumers.
  • Builds clean across netstandard2.0;net8.0;net9.0;net10.0; full ASB unit suite green (live-infra tests excluded).
  • No ADR (bug fix, honours existing ADR 0045); release note added.

[claude]

Code Review — PR #4201: forward dead-letter reason/description to ASB native fields

Thanks for this — it's a tight, well-scoped fix. The diagnosis (reason/description computed in RejectAsync but dropped on the no-reason settle) is correct, the change is minimal, and routing both Proactor and Reactor through the same async path is the right call. A few observations, none blocking.

Correctness ✅

• The wrapper maps (shiv, reason, description) onto Azure.Messaging.ServiceBus 7.20.1's DeadLetterMessageAsync(message, deadLetterReason, deadLetterErrorDescription) in the correct order.
• RejectAsync already coalesces description to "unknown" (never null on this path), and the wrapper still defends the nullable case — good belt-and-braces.
• The single-arg IServiceBusReceiverWrapper.DeadLetterAsync(string lockToken) is now unused in production (only the 3-arg overload is called). Reasonable to keep for compatibility, but worth a deliberate decision: since you're already extending the interface, consider whether the old one should be [Obsolete] or dropped, rather than left as quiet dead code.

Test coverage 🔶

Per the repo's strict TDD stance, two introduced behaviors are unverified:

1. Truncation — Truncate() and the 4096 boundary are entirely untested. A test with a >4096-char description asserting the forwarded value is clamped (and exactly-4096 is untouched) would lock in the limit and document why it exists.
2. Null/default reason path — the reason is null → "DeliveryError"/"unknown" branch in RejectAsync and the wrapper's description is null ? null branch aren't exercised. The new tests only cover the populated-reason case.

The two happy-path tests are clear and mirror each other well across Proactor/Reactor.

Style nit

• MAX_DEAD_LETTER_FIELD_LENGTH uses SCREAMING_CASE, but the project convention for constants is PascalCase (see ASBConstants: LockTokenHeaderBagKey, etc.). Suggest MaxDeadLetterFieldLength.

Minor edge case

• Substring(0, 4096) can split a surrogate pair at the boundary, yielding a lone surrogate. Extremely unlikely to matter for diagnostic text and ASB accepts it — just noting, not worth special-casing.

Docs/notes ✅

Release note is clear and accurately scoped; XML docs on both the interface and impl explain the 4096 cap.

Overall: a solid bug fix that does exactly what #4196 asked. Main suggestion is adding a truncation test (and ideally a null-reason test) to satisfy the repo's TDD guardrails before merge.

[Copilot]

Pull request overview

This PR fixes an Azure Service Bus DLQ observability gap by forwarding the already-computed rejection reason and description from AzureServiceBusConsumer.RejectAsync into Azure Service Bus’s native DeadLetterReason / DeadLetterErrorDescription fields (instead of only logging them and calling the no-reason dead-letter overload).

Changes:

• Add a new DeadLetterAsync(lockToken, reason, description) overload to IServiceBusReceiverWrapper and implement it in ServiceBusReceiverWrapper using the native SDK dead-letter overload with 4096-char truncation.
• Update AzureServiceBusConsumer.RejectAsync to call the new overload, preserving operator-visible DLQ fields.
• Add reactor (sync) and proactor (async) tests verifying that reason/description are forwarded.

Reviewed changes

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

Show a summary per file

File	Description
tests/Paramore.Brighter.AzureServiceBus.Tests/MessagingGateway/Reactor/AzureServiceBusConsumerTests.cs	Adds sync-path test asserting dead-letter reason/description are forwarded.
tests/Paramore.Brighter.AzureServiceBus.Tests/MessagingGateway/Proactor/AzureServiceBusConsumerTestsAsync.cs	Adds async-path test asserting dead-letter reason/description are forwarded.
tests/Paramore.Brighter.AzureServiceBus.Tests/Fakes/FakeServiceBusReceiverWrapper.cs	Extends fake receiver wrapper to capture dead-letter reason/description for assertions.
src/Paramore.Brighter.MessagingGateway.AzureServiceBus/AzureServiceBusWrappers/ServiceBusReceiverWrapper.cs	Implements new dead-letter overload and truncates fields to ASB’s 4096-char limit.
src/Paramore.Brighter.MessagingGateway.AzureServiceBus/AzureServiceBusWrappers/IServiceBusReceiverWrapper.cs	Adds the new public dead-letter overload to propagate native DLQ fields.
src/Paramore.Brighter.MessagingGateway.AzureServiceBus/AzureServiceBusConsumer.cs	Switches RejectAsync to call the reason/description dead-letter overload.
release_notes.md	Documents the fix and the new interface overload.

[iancooper]

@DevJonny Can you peer at this feedback for me:

"Test coverage 🔶

Per the repo's strict TDD stance, two introduced behaviors are unverified:

Truncation — Truncate() and the 4096 boundary are entirely untested. A test with a >4096-char description asserting the forwarded value is clamped (and exactly-4096 is untouched) would lock in the limit and document why it exists.
Null/default reason path — the reason is null → "DeliveryError"/"unknown" branch in RejectAsync and the wrapper's description is null ? null branch aren't exercised. The new tests only cover the populated-reason case.

The two happy-path tests are clear and mirror each other well across Proactor/Reactor."

[Jonny-Freemarket]

Thanks for the nudge @iancooper — facepalm, I'd already forgotten about these. 🤦

Done in the latest commit:

• Null/default reason path — added Proactor + Reactor tests asserting that rejecting with no reason forwards the default DeliveryError/unknown to the DLQ. Good catch, that gap was real.
• Constant naming — MAX_DEAD_LETTER_FIELD_LENGTH → MaxDeadLetterFieldLength.

On truncation — leaving it untested deliberately. Truncate is a private guard in ServiceBusReceiverWrapper and per our "test public surface, not internals" line the only ways to reach it are reaching into internals or a live-broker integration test, neither of which is worth it for a defensive clamp on diagnostic text. Happy to revisit if you'd rather see it covered.

Also leaving the single-arg DeadLetterAsync(string) overload in place rather than [Obsolete]-ing it for now — didn't want to touch the public surface beyond what this fix needs.

[codescene-delta-analysis]

Our agent can fix these. Install it.

Gates Passed
4 Quality Gates Passed

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Code Review — fix(#4196): forward dead-letter reason/description to ASB native fields

Nicely scoped bug fix. The change does exactly what the issue asks: RejectAsync already computed reasonString/description for the log line, and this forwards those same values to the native DeadLetterMessageAsync overload so the DLQ columns are no longer blank. No defaults changed, no scope creep — consistent with the repo's change-scope guidance and ADR 0045's "defer to native". 👍

Strengths

• Right layering. Truncation lives in ServiceBusReceiverWrapper (the SDK boundary), not smeared into the consumer. The consumer just passes the values it already has.
• Both paths covered. Tests assert behaviour for the Proactor (RejectAsync) and Reactor (Reject via BrighterAsyncContext.Run), and for both the explicit-reason and no-reason/default (DeliveryError/unknown) cases.
• Defensive nullability. The wrapper handles description is null, even though RejectAsync currently never passes null (?? "unknown") — sensible for the broader interface contract.
• Clear XML docs and a release note.

Suggestions (non-blocking)

1. Truncation is not directly unit-tested. All tests use FakeServiceBusReceiverWrapper, which stores the values verbatim and never exercises Truncate. The 4096-char clamp — the one piece of real logic added to the wrapper — has no coverage. A small unit test on Truncate (or asserting a >4096-char reason is clamped) would lock in the behaviour the comment promises.
2. Substring can split a surrogate pair at exactly the 4096 boundary, yielding a lone surrogate. ASB will still accept it, so this is cosmetic, but worth a mention — not worth complicating the code over unless you care about it.
3. The original DeadLetterAsync(string lockToken) overload is now unused in production code (only the new 3-arg overload is called). It remains on the public interface, implementation, and fake. Keeping it for backward compatibility is the right call for a public interface — just flagging it's now dead internally in case a future cleanup wants to [Obsolete] it.

Verification

• Confirmed DeadLetterAsync(lockToken, reason, description) is the sole production caller (AzureServiceBusConsumer.cs:314); NackAsync was not touched, correctly.
• Interface addition over a default-interface-method is justified — DIM isn't viable under netstandard2.0, matching the maintainer steer on ASB: rejected messages dead-lettered with blank reason/description #4196.

Overall: correct, minimal, well-tested at the consumer level. The only real gap is direct coverage of the truncation clamp. LGTM pending your call on that.