Dont Ack Action

[iancooper]

Brighter has a policy of load shedding in case of an error. That is, when a handler throws an error, our default approach is to acknowledge it, shed the load, and consume the next message. This prevents a poison pill message from blocking the pump thread.

Additions include a blocking retry via [UsePolicy] or [UseResiliencePipeline] that blocks until the policy "gives up" and we then ack again, and DeferMessageAction, which allows you to requeue with a delay (if supported).

However, sometimes, users want to keep retrying on an error, i.e., don't ack. Now this is possible, via a RetryForever policy, but it doesn't work so well for scenarios like a FeatureSwitch

• A FeatureSwitch, which does not consume any message whilst it is on, but just polls in case it is flipped.
• A blocking retry that terminates the application after a certain number of attempts

To support these are are adding a DontAckAction. This exception, when thrown, instructs the pump to pause and then attempt to resend the message.

Note that there are several risks here, such as locks timing out on a queue, so we may need to think about what happens when a nack is available on the transport, and that would be preferable as it makes it available to other consumers.

[claude]

Pull Request Review: DontAckAction Implementation

Overview

This PR introduces a DontAckAction exception that allows handlers to signal that a message should not be acknowledged, leaving it on the channel for re-delivery. The implementation is well-designed, thoroughly documented with ADR 0038, and follows established patterns in the codebase.

Strengths

Architectural Consistency

• Follows established patterns: The implementation mirrors the existing DeferMessageAction and RejectMessageAction patterns perfectly
• Well-documented: ADR 0038 provides comprehensive context, including use cases, alternatives considered, and architectural diagrams
• Responsibility-Driven Design: Clear separation of concerns with well-defined roles (Signal, Controller, Service Provider)

Code Quality

• Clean implementation: The exception class is simple and follows the standard exception pattern with three constructors
• Proper exception handling: Both Reactor and Proactor handle DontAckAction consistently in:
  • Direct catch blocks (lines 309-318 in Reactor, 342-351 in Proactor)
  • AggregateException unwrapping (lines 221-225 in Reactor, 259-263 in Proactor)
  • TranslateMessage/TranslateMessageAsync unwrapping (line 523 in Reactor, line 532 in Proactor)
• Good logging: Separate log entries for the DontAckAction itself and its inner exception if present
• XML documentation: All public APIs are properly documented

Test Coverage

• Comprehensive testing: 8 new test files covering:
  • Handler behavior (DontAckOnError handler sync/async)
  • Feature switch integration (sync/async)
  • Pump behavior (Reactor/Proactor)
  • AggregateException scenarios
• Tests verify key behaviors: Messages not acknowledged, unacceptable count incremented, pump continues running

Issues & Concerns

1. Inconsistent Delay Handling in Reactor ⚠️

Location: Reactor.cs:258 and Reactor.cs:316

// Line 258 (AggregateException path) - synchronous blocking
Thread.Sleep(DontAckDelay);

// Line 316 (direct catch path) - asynchronous
Task.Delay(DontAckDelay).GetAwaiter().GetResult();

Issue: The Reactor uses Thread.Sleep in the AggregateException path but Task.Delay(...).GetAwaiter().GetResult() in the direct catch path. This inconsistency is confusing.

Recommendation: Use Thread.Sleep consistently throughout Reactor since it's the synchronous pump. The async Task.Delay pattern is appropriate for Proactor but not needed here.

2. Missing Null Check for InnerException

Location: Reactor.cs:312-313, Proactor.cs:345-346

if (dontAckAction.InnerException != null)
    Log.DontAckActionInnerException(s_logger, dontAckAction.InnerException, ...);

Issue: While the null check is present before logging, the logged exception could theoretically be null if passed explicitly to the constructor.

Recommendation: This is actually fine as written - the null check prevents the logging call, and C#'s null-forgiving operator or runtime null handling would catch issues. No change needed, but worth noting in case logging frameworks require non-null.

3. Test Uses TimeSpan.Zero for DontAckDelay

Location: When_a_handler_throws_dont_ack_action_should_not_acknowledge.cs:49

DontAckDelay = TimeSpan.Zero

Issue: While this makes tests faster, it doesn't test the actual delay behavior and could mask tight-loop issues in production.

Recommendation: Add at least one test that verifies the delay is actually applied (perhaps using FakeTimeProvider or measuring elapsed time).

4. DontAckDelay Not Cancellable in Proactor

Location: Proactor.cs:349

await Task.Delay(DontAckDelay);

Issue: The Task.Delay call doesn't pass a CancellationToken, which means if the pump is being shut down, it will still wait for the full delay before checking the cancellation.

Recommendation: Pass a cancellation token to allow graceful shutdown:

await Task.Delay(DontAckDelay, cancellationToken);

However, this requires plumbing the cancellation token through the EventLoop, which may be out of scope for this PR.

5. Potential for Infinite Loops

Severity: Documentation/Design

The ADR correctly identifies this risk (line 335), but it bears emphasizing:

• If UnacceptableMessageLimit <= 0 and a message always triggers DontAckAction, the pump will retry indefinitely
• This is by design for feature switch use cases, but dangerous if misconfigured
• The 1-second default delay helps, but doesn't prevent the issue

Recommendation: Consider logging at WARN level every N iterations (e.g., every 100th retry) to help operators detect stuck consumers:

if (UnacceptableMessageCount % 100 == 0) {
    Log.DontAckActionRepeatedWarning(..., UnacceptableMessageCount);
}

This is not blocking for this PR but could be a future enhancement.

Performance Considerations

Positive

• Minimal overhead: Exception-based flow control is efficient for exceptional paths
• No allocations: The implementation doesn't allocate unnecessary objects
• Configurable delay: DontAckDelay allows tuning the trade-off between CPU usage and responsiveness

Consideration

• Reflection caching: The existing DispatchMethodCache and UnWrapPipelineFactoryCache mean DontAckAction doesn't add reflection overhead

Security Considerations

No security issues identified. The implementation:

• Doesn't expose sensitive data
• Doesn't introduce injection vulnerabilities
• Properly preserves exception context for debugging
• Follows the principle of least privilege

Suggestions for Future Work

1. Monitoring/Metrics: Consider adding metrics for:

   • Count of DontAckAction events per message/channel
   • Time messages spend unacknowledged
   • Detection of "stuck" messages

2. Transport-specific guidance: Document behavior for each supported transport (RabbitMQ, Kafka, SQS, etc.) since unacknowledged message handling varies

3. Nack support: As noted in the ADR, some transports support explicit nack with requeue, which could be more efficient than leaving messages unacknowledged

Verdict

APPROVE with minor suggestions

This is high-quality work that:

• ✅ Follows established patterns consistently
• ✅ Is thoroughly tested
• ✅ Is well-documented
• ✅ Solves real use cases (feature switches, blocking retries)
• ⚠️ Has one minor inconsistency (Thread.Sleep vs Task.Delay in Reactor)
• 💡 Could benefit from cancellation token support in Proactor (future enhancement)

The inconsistency in Reactor.cs:258 should be addressed, but it's not blocking. The other points are minor observations or future enhancements.

Great work on the comprehensive ADR and test coverage! 🎉

---

Files Reviewed:

• ADR 0038
• Core implementation: DontAckAction.cs, handlers, attributes
• Pump modifications: Reactor.cs, Proactor.cs, MessagePump.cs
• Feature switch integration
• Test coverage (8 test files)

Recommendation: Merge after addressing the Thread.Sleep inconsistency in Reactor.cs

[claude]

Pull Request Review: DontAck Action

Summary

This PR introduces a DontAckAction exception mechanism to allow handlers to signal that a message should remain unacknowledged on the channel. The implementation is well-designed, follows established patterns in the codebase, and includes comprehensive test coverage.

✅ Strengths

1. Excellent Architectural Consistency

• Follows the existing exception-as-signal pattern (DeferMessageAction, RejectMessageAction, InvalidMessageAction)
• Properly integrated into both Reactor (sync) and Proactor (async) message pumps
• Consistent handling in both standalone catch blocks and AggregateException processing

2. Comprehensive Documentation

• Well-written ADR (docs/adr/0038-dont-ack-action.md) explaining context, forces, and decisions
• Clear XML documentation on all public APIs
• Good inline comments explaining the behavior

3. Strong Test Coverage

• Unit tests for handler behavior (sync/async)
• Integration tests for message pump behavior (Reactor/Proactor)
• Feature switch integration tests
• Tests verify nack behavior, unacceptable message counting, and pump continuation

4. Safety Mechanisms

• DontAckDelay prevents tight-loop CPU burn
• Unacceptable message limit prevents infinite loops
• Inner exception logging for debugging
• Proper telemetry spans with error status

5. Broad Transport Support

• Nack implementation added to all messaging gateways:
  • AWS SQS (V4 and legacy)
  • Azure Service Bus
  • GCP Pub/Sub (Stream and Pull)
  • Kafka
  • MQTT
  • RabbitMQ (Sync and Async)
  • Redis
  • InMemoryMessageConsumer

🔍 Areas for Consideration

1. Potential Tight Loop in Some Transports (Medium Priority)

Location: Reactor.cs:310-320, Proactor.cs:343-352

When DontAckAction is thrown, the message is nacked and DontAckDelay is applied. However, for some transports (especially in-memory or those with very short visibility timeouts), the message might become available again before the delay completes, potentially causing a tight loop.

Observation: The delay is configurable (DontAckDelay) and the unacceptable message limit provides a circuit breaker, so this is mitigated. However, the PR description mentions "locks timing out on a queue" as a concern - this suggests the author is aware of transport-specific timing issues.

Recommendation: Consider documenting recommended DontAckDelay values for different transport types in the ADR or user documentation.

2. Nack Implementation Consistency Across Transports (Low-Medium Priority)

Locations: Various messaging gateway files

The Nack implementations vary across transports:

• RabbitMQ: Uses channel.BasicNack() - proper negative acknowledgment
• SQS: Uses ChangeMessageVisibility with timeout=0 - returns to queue immediately
• Kafka: No-op (doesn't commit offset) - relies on consumer poll timeout
• Azure Service Bus: Uses AbandonMessageAsync() - releases lock
• Redis: Sets visibility to past time - makes message immediately visible

Observation: This is appropriate - each transport has different ack/nack semantics. However, the behavior differences could surprise users.

Recommendation: Consider adding a table to the ADR or documentation showing how Nack behaves on each transport and any gotchas (e.g., Kafka offset not committing means consumer needs to restart to see message again).

3. Error Handling in DontAckOnErrorHandler (Low Priority)

Location: DontAckOnErrorHandler.cs:52-62, DontAckOnErrorHandlerAsync.cs:56-66

catch (Exception ex)
{
    throw new DontAckAction(ex.Message, ex);
}

This catches all exceptions, including DeferMessageAction, RejectMessageAction, and even ConfigurationException. This might not be the intended behavior - you probably want to let control-flow exceptions pass through.

Issue: If an inner handler throws DeferMessageAction, the DontAckOnErrorHandler will catch it and convert it to DontAckAction, changing the intended behavior.

Recommendation: Add exception type filtering:

catch (Exception ex) when (ex is not DeferMessageAction 
                           and not RejectMessageAction 
                           and not InvalidMessageAction 
                           and not DontAckAction 
                           and not ConfigurationException)
{
    throw new DontAckAction(ex.Message, ex);
}

4. Missing Nack in Channel Wrappers (Low Priority)

Location: Channel.cs, ChannelAsync.cs

The Channel and ChannelAsync classes expose Nack methods but don't appear to add any additional logic or validation. This is fine, but worth noting that the channel layer is just a pass-through.

Observation: This is consistent with how Acknowledge, Reject, and Requeue are handled. No action needed, just noting for completeness.

5. Reactor.cs Line 259 - Blocking Call (Low Priority)

Location: Reactor.cs:259

Thread.Sleep(DontAckDelay);

In the Reactor (sync pump), Thread.Sleep is used directly. While this is technically correct for the Reactor pattern, there's an inconsistency within the file:

• Line 123: Uses Task.Delay(...).GetAwaiter().GetResult()
• Line 259: Uses Thread.Sleep(...)

Recommendation: Use Task.Delay(...).GetAwaiter().GetResult() for consistency with other delay points in the same file (lines 123, 131, 155, 318).

6. Test Doubles Organization (Very Low Priority)

Location: Various test files

The test doubles (MyFailingDontAckHandler, SpyDontAckCommandProcessor) are well-implemented. Minor observation: SpyDontAckCommandProcessor has a static SendCount field which could cause issues if tests run in parallel.

Recommendation: Consider making the counter instance-based or using thread-safe increments if parallel test execution is supported.

🔐 Security Considerations

No security vulnerabilities identified. The implementation:

• ✅ Doesn't expose sensitive data in exceptions
• ✅ Properly increments unacceptable message count to prevent infinite loops
• ✅ Includes proper logging for observability
• ✅ Respects configured limits (UnacceptableMessageLimit)

⚡ Performance Considerations

• The delay mechanism (DontAckDelay) adds latency by design - this is intentional and necessary
• Nack operations are generally low-cost on most transports
• No additional memory allocations beyond the exception object
• The approach correctly avoids tight loops through the delay + limit mechanisms

📋 Test Coverage Assessment

Excellent coverage across:

• ✅ Unit tests for exception conversion (DontAckOnErrorHandler)
• ✅ Integration tests for message pump behavior (Reactor and Proactor)
• ✅ Feature switch integration
• ✅ Both sync and async paths
• ✅ Aggregate exception handling
• ✅ Nack verification through test infrastructure

Missing tests (minor):

• Transport-specific Nack behavior (currently relies on transport library tests)
• Edge cases like concurrent DontAckAction from multiple handlers in a pipeline

🎯 Recommendations

Must Fix (Before Merge)

1. Fix DontAckOnErrorHandler exception filtering to avoid catching control-flow exceptions (see Support async versions of Send and Publish #3 above)

Should Consider

2. Document transport-specific Nack behavior differences
3. Change Thread.Sleep to Task.Delay().GetAwaiter().GetResult() in Reactor.cs:259 for consistency
4. Document recommended DontAckDelay values for different scenarios

Nice to Have

5. Add transport-specific Nack behavior tests
6. Make test doubles thread-safe if parallel tests are used

📝 Additional Notes

Positive Implementation Details

• The ordering of exception catch blocks is correct and well-documented in the ADR
• The TranslateMessage exception unwrapping correctly handles DontAckAction (Reactor.cs:525, Proactor.cs:534)
• Logging is comprehensive with appropriate log levels
• The feature switch integration is elegant and maintains backward compatibility

ADR Quality

The ADR is well-written and follows the project's established format. It clearly explains:

• The problem and forces
• The decision rationale
• Implementation details with code examples
• The reasoning for adding Nack support (Amendment 2026-02-17)

Conclusion

This is a high-quality PR that adds a useful feature to the framework. The implementation follows established patterns, includes comprehensive tests, and is well-documented. The main concern is the exception filtering in DontAckOnErrorHandler which should be addressed before merge. Other recommendations are minor improvements that could be addressed in follow-up PRs if preferred.

Recommendation: Approve with minor changes requested for the exception filtering issue.

---

Review conducted using Claude Code following Brighter's design principles and architectural patterns.

[claude]

Pull Request Review: DontAckAction

Summary

This PR introduces a DontAckAction exception to enable message pumps to leave messages unacknowledged on the transport, addressing use cases like feature switches and controlled blocking retries. The implementation is comprehensive, well-documented, and follows established patterns in the codebase.

Code Quality Assessment: ✅ Excellent

Strengths

1. Exceptional Documentation

• ADR 0038 is exemplary - comprehensive context, clear decision rationale, architectural diagrams, and thorough consideration of alternatives
• XML documentation on all public APIs is complete and clear
• Tests are well-named and verify the correct behavior

2. Architectural Consistency

• Follows the established exception-as-signal pattern (DeferMessageAction, RejectMessageAction)
• Handler pipeline integration is identical to existing patterns (DontAckOnErrorHandler mirrors RejectMessageOnErrorHandler)
• Catch block ordering in Reactor.cs:310 and Proactor.cs is properly sequenced
• Amendment §6 (Transport Nack) addresses the original limitation thoughtfully

3. Comprehensive Implementation

• All 8 transport implementations updated (RabbitMQ, SQS, Azure Service Bus, Kafka, Redis, MQTT, GCP Pub/Sub, InMemory)
• Both sync (Reactor) and async (Proactor) message pumps covered
• AggregateException handling includes DontAckAction detection
• TargetInvocationException unwrapping updated

4. Safety Mechanisms

• DontAckDelay (1 second default) prevents tight-loop CPU burn
• Unacceptable message limit provides escape hatch
• Inner exception logging at Warning level for observability
• Tests verify limit-exceeded behavior

[claude]

Observations & Suggestions (Part 2)

1. Logging Consistency (Minor)

The inner exception is logged only in the direct catch block (Reactor.cs:313-314) but not in the AggregateException handler (lines 253-261). Consider capturing the inner exception in the aggregate handler and logging it there as well for consistency.

2. Transport Implementation - Error Handling

• AWS SQS Nack: Uses ChangeMessageVisibility(0) to make message immediately available. Consider documenting edge cases with delayed messages
• Redis Nack: No-op is correct (LPOP is destructive), but users should be aware Redis doesn't get immediate-redelivery benefit

3. FeatureSwitchAttribute Constructor

The attribute now has 5 parameters with dontAck. Good that it defaults to false for backward compatibility. The named-parameter pattern in examples is the right approach.

4. Potential Race Condition (Low Risk)

InMemoryMessageConsumer Nack removes from _lockedMessages and re-enqueues. In high-throughput scenarios with multiple consumers, there's a tiny window for double processing. This is acceptable for in-memory testing transports.

Security Considerations

No Issues Identified

• No new authentication/authorization concerns
• Exception handling doesn't leak sensitive data
• Transport-level operations use existing secure channels
• Logging is appropriately scoped (Warning level for inner exceptions)

Performance Considerations

Positive:

• DontAckDelay prevents CPU thrashing
• Nack is more efficient than waiting for visibility timeout on queue transports
• No additional allocations beyond the exception itself

Watch For:

• Misconfigured UnacceptableMessageLimit = -1 with perpetual failures could cause indefinite blocking
• Feature switches left off without dontAck=true will silently drop messages (by design)

[claude]

Testing Assessment & Recommendations (Part 3)

Test Coverage: Excellent

• Unit tests verify exception behavior
• Integration tests verify pump behavior
• Both sync and async paths covered
• Edge cases (aggregate exceptions) covered

Missing (Low Priority):

• No explicit test for DontAckDelay timing (though TimeSpan.Zero is used in tests)
• No test for UnacceptableMessageLimit = 1 scenario

Recommendations

Before Merge

1. ✅ Apply inner exception logging in AggregateException handlers (Reactor.cs:253-261, Proactor.cs:290-298)
2. ⚠️ Verify transport implementations have appropriate error handling (especially SQS ChangeMessageVisibility)

Post-Merge

3. 📖 Update user documentation with examples of:
   • Using DontAckOnErrorAttribute with retry policies
   • Configuring UnacceptableMessageLimit for different use cases
   • Transport-specific behavior of Nack (especially Redis/Kafka no-ops)
4. 📊 Consider adding metrics for DontAck occurrences in production (future work)

Conclusion

Recommendation: ✅ APPROVE with Minor Suggestions

This is a high-quality implementation that:

• Solves a real problem (feature switches, controlled blocking)
• Follows established architectural patterns
• Includes comprehensive documentation
• Has excellent test coverage
• Considers backward compatibility

The suggested improvements are minor and non-blocking. The code is production-ready.

Well done! The ADR in particular is a model for how to document architectural decisions.

---

Reviewed by Claude Code following Brighter's CLAUDE.md guidelines

[claude]

Pull Request Review: DontAckAction Feature

Summary

This PR introduces a DontAckAction exception mechanism that allows message handlers to signal the message pump to not acknowledge a message, causing it to remain on the channel for redelivery. This is a well-architected addition that follows Brighter's existing patterns for exception-based flow control.

---

✅ Strengths

1. Excellent Architectural Consistency

• Follows the established pattern of DeferMessageAction, RejectMessageAction, and InvalidMessageAction
• Properly integrated into both Reactor and Proactor pump implementations
• Consistent exception handling across sync/async paths
• Well-documented with comprehensive ADR (0038) that explains the reasoning and trade-offs

2. Comprehensive Implementation

• All transport implementations correctly implement Nack/NackAsync methods
• Proper distinction between queue-based transports (active nack) and stream-based transports (no-op)
• Correct handling of edge cases:
  • AggregateException unwrapping
  • TargetInvocationException unwrapping in TranslateMessage
  • Integration with unacceptable message count limits

3. Strong Test Coverage

• 22 test files covering various scenarios
• Tests for both sync (Reactor) and async (Proactor) paths
• Tests for DontAckOnError handlers
• Tests for FeatureSwitch integration
• Tests verify correct pump behavior (nack, unacceptable message count, status)

4. Good Developer Experience

• Clear XML documentation on all public APIs
• Two convenient attributes (DontAckOnErrorAttribute, FeatureSwitchAttribute.DontAck) for common scenarios
• Sensible defaults (1-second delay)
• Preserves inner exceptions for debugging

---

🔍 Code Quality Observations

Minor Issues

1. Thread.Sleep in Reactor (Blocking Call)

// In Reactor.cs
Thread.Sleep(DontAckDelay);

While this is acceptable for the synchronous Reactor, consider documenting that this blocks the pump thread. The async Proactor correctly uses await Task.Delay.

2. Exception Swallowing in DontAckOnErrorHandler

catch (Exception ex)
{
    throw new DontAckAction(ex.Message, ex);
}

This converts ALL exceptions to DontAckAction, which changes the semantics. Consider:

• What if the exception was a ConfigurationException that should stop the pump?
• Should certain exception types be allowed to propagate unchanged?

Recommendation: Document this behavior clearly in the handler's XML comments, or consider a whitelist/blacklist approach for exception types that should NOT be converted.

3. Missing Validation in FeatureSwitchHandler

_dontAck = initializerList.Length > 2 && initializerList[2] is true;

If initializerList[2] exists but is not a boolean, this silently defaults to false. Consider throwing an exception for invalid types to catch configuration errors early.

Documentation Gaps

4. Nack Implementation Behavior Not Fully Documented

The transport-specific Nack implementations have good inline comments, but the interface documentation could be clearer about:

• What happens if Nack is called multiple times on the same message?
• What happens if Nack fails (e.g., network error, invalid receipt handle)?
• For transports where Nack is a no-op, should users be warned about the visibility timeout delay?

Example from SQS implementation - good error handling, but errors are logged and rethrown:

catch (Exception ex)
{
    Log.ErrorNackingMessage(s_logger, ex, message.Id, receiptHandle, _queueName);
    throw;
}

Question: Should Nack failures stop the pump, or should they be swallowed with a warning?

5. ADR Could Mention Resource Implications

The ADR is excellent but could mention:

• Memory: Messages remain in transport memory/storage longer
• Lock timeouts: On some transports, locks may expire during the delay period
• Connection resources: Keeping connections open longer during delays
• Message ordering: Impact on ordered message processing

---

🔒 Security Considerations

No Critical Issues Found

However, consider these edge cases:

1. Potential for Accidental Infinite Loops

   • If UnacceptableMessageLimit is set to -1 and a handler always throws DontAckAction, the pump will run forever
   • Mitigation: This is by design (documented in requirements), but consider adding a metric or log warning after N consecutive DontAckActions

2. Resource Exhaustion

   • A malicious or buggy handler could cause all pump threads to block indefinitely
   • Mitigation: The UnacceptableMessageLimit provides a circuit breaker, but it's per-message, not per-pump

---

⚡ Performance Considerations

Positive Aspects

1. DontAckDelay prevents CPU burn - Good design
2. Nack implementations are efficient - Direct API calls, no polling
3. No additional allocations - Uses existing exception handling infrastructure

Potential Concerns

1. Repeated Message Processing Overhead

Each DontAckAction triggers:

• Handler execution
• Logging
• Telemetry/span updates
• Unacceptable count increment
• Transport Nack call
• Delay

For high-frequency DontAckActions, this overhead compounds. Consider:

• Exponential backoff for DontAckDelay after N consecutive failures?
• Metrics to track DontAckAction frequency per channel?

2. In-Memory Queue Growth

For InMemoryMessageConsumer, nacked messages go back to _bus.Enqueue(message):

public void Nack(Message message)
{
    _lockedMessages.TryRemove(message.Id, out _);
    _bus.Enqueue(message);
}

If DontAckActions are frequent, the in-memory queue could grow unbounded.
Recommendation: Document this behavior and recommend monitoring queue depth in production.

---

📋 Testing Assessment

Coverage is Strong

The tests cover:

• ✅ Basic DontAckAction handling in Reactor/Proactor
• ✅ AggregateException scenarios
• ✅ FeatureSwitch integration
• ✅ DontAckOnError handlers
• ✅ Unacceptable message limit behavior
• ✅ Nack calls to transport

Potential Test Gaps

1. No test for DontAckDelay timing - Consider adding a test that verifies the delay is actually applied
2. No test for Nack failure scenarios - What happens if the transport Nack call throws?
3. No test for rapid consecutive DontAckActions - Does the delay compound correctly?
4. No integration test with actual transports - The tests use InMemoryMessageConsumer; consider smoke tests with real RabbitMQ/SQS

---

🎯 Recommendations

High Priority

1. Document exception conversion behavior in DontAckOnErrorHandler XML comments (which exceptions should NOT be converted?)
2. Add validation for FeatureSwitchAttribute.InitializeFromAttributeParams to catch type mismatches

Medium Priority

3. Enhance Nack documentation on interfaces to clarify:

   • Behavior on repeated calls
   • Error handling expectations
   • No-op transport implications

4. Add metric/telemetry for consecutive DontAckAction count per message ID to help diagnose infinite loops

5. Consider exponential backoff for DontAckDelay to reduce overhead on persistent failures

Low Priority

6. Add timing test for DontAckDelay
7. Add Nack failure test to verify pump behavior when transport Nack throws
8. Update ADR with resource/performance implications

---

✨ Overall Assessment

This is a high-quality PR that demonstrates excellent software engineering practices:

• ✅ Solves a real user need (Feature Switch blocking, stream processing)
• ✅ Follows established architectural patterns
• ✅ Comprehensive documentation (ADR, requirements, XML docs)
• ✅ Strong test coverage
• ✅ Proper handling of both sync and async code paths
• ✅ Transport-specific implementations are correct and well-documented

The minor issues identified are primarily about defensive programming and documentation clarity rather than fundamental design flaws.

Recommendation: Approve with minor suggestions. The identified issues are non-blocking and could be addressed in follow-up PRs if needed.

---

📚 References

• ADR 0038: docs/adr/0038-dont-ack-action.md
• Requirements: specs/0020-DontAckAction/requirements.md
• Related patterns: DeferMessageAction, RejectMessageAction, InvalidMessageAction

[claude]

Code Review: DontAckAction (PR #4019)

This is a well-motivated feature that fills a genuine gap in Brighter's message-handling story — particularly for feature-switch polling and intentional retry-forever patterns. The ADR is thorough, the implementation follows existing conventions, and test coverage across both Reactor/Proactor paths is solid. A few issues worth addressing before merge:

---

🔴 Critical: DontAckOnErrorHandler swallows signal exceptions

DontAckOnErrorHandler<TRequest>.Handle() catches the base Exception type, which means it will intercept and re-wrap other Brighter signal exceptions — including DeferMessageAction, RejectMessageAction, InvalidMessageAction, and ConfigurationException:

// DontAck/Handlers/DontAckOnErrorHandler.cs
catch (Exception ex)
{
    throw new DontAckAction(ex.Message, ex);  // ← swallows DeferMessageAction, RejectMessageAction, etc.
}

Consequence: If a handler in the pipeline throws DeferMessageAction (to requeue) or RejectMessageAction (to DLQ), and [DontAckOnError] is outermost, the message will be nacked and re-delivered indefinitely rather than being deferred or rejected. This is a silent behavioral change that could cause message loops.

Suggested fix (same pattern should apply to DontAckOnErrorHandlerAsync):

catch (Exception ex) when (ex is not DontAckAction
                              and ex is not DeferMessageAction
                              and ex is not RejectMessageAction
                              and ex is not InvalidMessageAction
                              and ex is not ConfigurationException)
{
    throw new DontAckAction(ex.Message, ex);
}

---

🟡 Significant: Incrementing UnacceptableMessageCount on expected DontAck scenarios

Every DontAckAction — including those from an intentional feature switch (dontAck: true) — increments UnacceptableMessageCount. If UnacceptableMessageLimit is configured, a permanently-disabled feature switch will eventually stop the pump.

// Reactor.cs / Proactor.cs
catch (DontAckAction dontAckAction)
{
    Channel.Nack(message);
    IncrementUnacceptableMessageCount();  // ← will eventually stop the pump
    ...
}

The ADR mentions the risk but doesn't prescribe a mitigation. Users who combine [FeatureSwitch(dontAck: true)] with UnacceptableMessageLimit will see the pump stop. At minimum, the XML docs on DontAckOnErrorAttribute and FeatureSwitchAttribute should warn about this interaction. Alternatively, DontAckAction could carry a flag to indicate whether it should count against the limit.

---

🟡 Significant: Kafka (and other stream) Nack is a silent no-op

// KafkaMessageConsumer.cs
public void Nack(Message message)
{
    // no-op
}

A consumer using [DontAckOnError] with Kafka will believe the message is being nacked, but the message will actually be acknowledged on the next poll offset commit. This could be very surprising. At minimum a LogWarning should be emitted:

public void Nack(Message message)
{
    _logger.LogWarning("Nack is not supported for Kafka stream consumers. The message will be acknowledged on the next commit.");
}

Same applies to Redis, MQTT, GCP PubSub, MsSql, Postgres, and RocketMQ consumers.

---

🟡 Task.Delay(...).GetAwaiter().GetResult() in Reactor

Reactor is a synchronous pump, but the DontAck delay uses the async task machinery:

// Reactor.cs
Task.Delay(DontAckDelay).GetAwaiter().GetResult();

For a sync context, Thread.Sleep((int)DontAckDelay.TotalMilliseconds) is more idiomatic and avoids task allocation. Compare with the existing ChannelFailureDelay handling in the reactor which presumably uses the same pattern — worth making consistent.

---

🟡 Inconsistency in AggregateException path: inner exception not logged for DontAck

In the direct catch path, the inner exception is logged:

catch (DontAckAction dontAckAction)
{
    if (dontAckAction.InnerException != null)
        Log.DontAckActionInnerException(...);  // ← logged
    ...
}

But in the AggregateException path:

if (dontAck)
{
    Log.NotAcknowledgingMessage(...);
    await Channel.NackAsync(message);
    // ← inner exception from original DontAckAction not logged
    ...
}

The inner exception should also be logged in the aggregate path so operators can diagnose why the message wasn't acknowledged.

---

🟠 Minor: DontAckAction missing serialization support

Custom exception classes should follow the standard .NET serialization pattern for completeness, particularly if exceptions may cross AppDomain boundaries or be used in certain serialization contexts:

[Serializable]
public class DontAckAction : Exception
{
    public DontAckAction() {}
    public DontAckAction(string? reason) : base(reason) {}
    public DontAckAction(string? reason, Exception? innerException) : base(reason, innerException) {}

    [Obsolete("This constructor is for serialization only")]
    protected DontAckAction(SerializationInfo info, StreamingContext context) : base(info, context) {}
}

(Same applies to the existing DeferMessageAction, RejectMessageAction, etc. — so this may be a project-wide pattern to revisit, but worth noting here.)

---

🟠 Minor: DontAckDelay not validated

DontAckDelay can be set to TimeSpan.Zero or a negative value with no guard. A zero delay negates the CPU-burn protection the property is intended to provide. A simple check in the setter or in pump startup would help:

public TimeSpan DontAckDelay
{
    get => _dontAckDelay;
    set => _dontAckDelay = value > TimeSpan.Zero ? value : TimeSpan.FromSeconds(1);
}

---

📋 Test Coverage Gaps

The unit test coverage for the pump-level handling is good. A few gaps worth considering:

1. No test for UnacceptableMessageLimit interaction: A test asserting that a pump configured with UnacceptableMessageLimit = 3 stops after 3 DontAckAction occurrences would document and protect the behavior.

2. No integration test for transport Nack implementations: RMQ, SQS, and Azure Service Bus have non-trivial Nack implementations. A test verifying that the message becomes visible again (or is re-queued) would catch regressions.

3. AggregateException path inner exception logging: No test verifying that the inner exception is not lost when DontAckAction is caught from an aggregate.

---

✅ What's well done

• The feature follows the existing exception-as-signal pattern (DeferMessageAction, RejectMessageAction) consistently.
• The Nack method is correctly added to both sync/async channel and consumer interfaces, with real implementations for RMQ (BasicNack(requeue: true)), SQS (ChangeMessageVisibility(0)), and Azure Service Bus (AbandonAsync).
• FeatureSwitchAttribute.dontAck defaults to false, so it's a fully backward-compatible change.
• The ADR (0038) is comprehensive and honest about the risks.
• DontAckAction surfacing in TranslateMessage's TargetInvocationException unwrapping is correctly handled.
• The AggregateException priority ordering (stop > defer > dontAck > reject) is sensible.

Overall this is a solid feature — the critical issue with signal exception swallowing in DontAckOnErrorHandler is the one that needs attention before this ships.

[codescene-delta-analysis]

Code Health Improved (2 files improve in Code Health)

Gates Failed
Prevent hotspot decline (1 hotspot with Code Duplication)
Enforce critical code health rules (1 file with Low Cohesion)
Enforce advisory code health rules (4 files with Complex Method, Code Duplication)

Gates Passed
1 Quality Gates Passed

See analysis details in CodeScene

Reason for failure

Prevent hotspot decline	Violations	Code Health Impact
RmqMessageConsumer.cs	1 rule in this hotspot	no change	Suppress

Enforce critical code health rules	Violations	Code Health Impact
KafkaMessageConsumer.cs	1 critical rule	6.87 → 6.10	Suppress

Enforce advisory code health rules	Violations	Code Health Impact
AzureServiceBusConsumer.cs	2 advisory rules	9.21 → 8.43	Suppress
Proactor.cs	1 advisory rule	7.98 → 7.81	Suppress
Reactor.cs	1 advisory rule	7.98 → 7.81	Suppress
RmqMessageConsumer.cs	1 advisory rule	no change	Suppress

View Improvements

File	Code Health Impact	Categories Improved
InMemoryMessageConsumer.cs	8.96 → 9.54	Code Duplication
PostgresMessageConsumer.cs	6.86 → 7.29	Overall Code Complexity

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.

[codescene-delta-analysis]

❌ Getting worse: Complex Method
Run increases in cyclomatic complexity from 41 to 49, threshold = 9

_Suppress

[codescene-delta-analysis]

❌ Getting worse: Complex Method
EventLoop increases in cyclomatic complexity from 41 to 49, threshold = 9

_Suppress

[codescene-delta-analysis]

❌ New issue: Low Cohesion
This module has at least 3 different responsibilities amongst its 35 functions, threshold = 3

_Suppress

[claude]

PR Review: DontAckAction (#4019)

This is a well-thought-out feature that fills a real gap in Brighter's message pump behavior. The ADR is thorough, the exception-as-signal pattern is consistent with existing actions, and the transport-specific nack implementations are well-differentiated. Below are the findings, grouped by severity.

---

Issues to Address Before Merge

1. DontAckOnErrorHandler re-wraps existing DontAckAction exceptions

DontAck/Handlers/DontAckOnErrorHandler.cs and DontAckOnErrorHandlerAsync.cs

Both handlers catch all exceptions without a guard for action exceptions that should propagate unmodified:

catch (Exception ex)
{
    throw new DontAckAction(ex.Message, ex);  // Re-wraps DontAckAction in DontAckAction
}

If an inner handler throws DontAckAction directly (e.g., from FeatureSwitchHandler), the outer DontAckOnErrorHandler will catch it and re-wrap it: DontAckAction(reason, innerException: DontAckAction(...)). The pump will still do the right thing (not ack), but the logged inner exception chain will be misleading.

Compare RejectMessageOnErrorHandler — it has the same issue. However, this is a new handler and the pattern can be improved here. Suggest:

catch (Exception ex)
{
    if (ex is DontAckAction or DeferMessageAction or RejectMessageAction or InvalidMessageAction)
        throw;
    throw new DontAckAction(ex.Message, ex);
}

This prevents double-wrapping and ensures the inner exception in the log reflects the actual originating failure.

---

2. Missing CancellationToken in Proactor delay and nack

Proactor.cs (both the AggregateException branch and the catch block):

await Channel.NackAsync(message);            // Should pass cancellationToken
await Task.Delay(DontAckDelay);              // Should pass cancellationToken

The Proactor operates in an async loop with a cancellation token (ct). Both calls should propagate it so the pump can be cancelled promptly during shutdown rather than blocking for the full DontAckDelay. The existing Task.Delay calls elsewhere in the Proactor do pass the cancellation token — this is inconsistent.

---

3. MsSql and Redis Nack silently loses the message

MsSqlMessageConsumer.cs and RedisMessageConsumer.cs

The no-op nack implementations for MsSql and Redis mean that a DontAckAction thrown while consuming from these transports will silently drop the message — it has already been destructively removed from the source. The current XML doc for MsSql says "No implementation required because of atomic 'read-and-delete'" which is accurate but understates the consequence.

This should be called out prominently — either in the doc comment (e.g., "Warning: DontAckAction on this transport results in message loss as the message has already been consumed") or as a runtime warning log in the Nack method itself. Users relying on DontAckAction with MsSql or Redis transport will be surprised when messages disappear.

---

4. AggregateException: DontAckAction silently wins over RejectMessageAction

Reactor.cs and Proactor.cs (AggregateException handling):

When an AggregateException contains both a DontAckAction and a RejectMessageAction, DontAckAction takes precedence because it is checked first in the post-loop block:

if (dontAck != null)     { /* nack + continue */ }
if (reject)              { /* route to DLQ   */ }

Whether this is the right priority order is debatable (one could argue rejecting to DLQ is a stronger signal than not-acking), but at minimum the ADR should document this precedence. If the intent is that DontAckAction > RejectMessageAction > InvalidMessageAction, that decision should be explicit.

---

Minor Issues / Suggestions

5. ADR parameter order differs from implementation

docs/adr/0038-dont-ack-action.md (§4) shows:

public FeatureSwitchAttribute(..., bool dontAck = false, HandlerTiming timing = HandlerTiming.Before)

But the actual implementation (FeatureSwitchAttribute.cs and FeatureSwitchAsyncAttribute.cs) has:

public FeatureSwitchAttribute(..., HandlerTiming timing = HandlerTiming.Before, bool dontAck = false)

The code is what matters for correctness (callers using named params will be unaffected), but the ADR and code should agree. Update the ADR to match the implementation.

---

6. Postgres Nack comment is misleading

PostgresMessageConsumer.cs:

// No-op: visibility timeout will expire and message will become available for redelivery

The Postgres consumer uses FOR UPDATE SKIP LOCKED row-level locking rather than a visibility timeout. The message's lock is held by the database transaction; it won't be released until the transaction ends. A nack here behaves more like "wait for the transaction to roll back or time out" rather than an explicit release. The comment should reflect the actual mechanism.

---

7. Potential test flakiness in When_a_handler_throws_dont_ack_action_should_not_acknowledge_async.cs

The test relies on a time provider advance to trigger message requeue:

_timeProvider.Advance(TimeSpan.FromSeconds(2));
Assert.True(_commandProcessor.WaitForHandle(), "Re-delivery did not occur...");

There is a race: between advancing the time provider and the pump's internal timer checking for expired messages, the background task may not have re-delivered the message when WaitForHandle is called. A small synchronisation barrier (e.g., looping with a timeout, or using a ManualResetEventSlim around the requeue callback) would make this more robust in CI.

---

Positive Observations

• The exception-as-signal pattern is applied consistently and correctly.
• Transport-specific nack implementations are well-differentiated (active nack for queues, no-op for streams).
• The DontAckDelay property follows the same pattern as ChannelFailureDelay, EmptyChannelDelay, etc.
• The InMemoryMessageConsumer.Nack implementation (remove from _lockedMessages + re-enqueue) is correct for testing.
• The UnacceptableMessageLimit integration gives operators an escape hatch from stuck consumers — this is a crucial safety valve.
• The AbandonAsync addition to the Azure Service Bus wrapper and its interface is clean.
• Test coverage is solid: both Reactor and Proactor paths, both standalone and AggregateException cases, both FeatureSwitch and DontAckOnError paths are covered.

---

Review generated by Claude (claude-sonnet-4-6)