Resilience Pipeline Can Be Reused

[iancooper]

This PR closes #4042

Currently, Brighter's resilience pipeline resolution has two bugs that prevent pipeline reuse:

1. Async handler ignores UseTypePipeline flag: ResilienceExceptionPolicyHandlerAsync<TRequest>.InitializeFromAttributeParams() always calls GetPipeline<TRequest>(key) (the generic/typed lookup) regardless of the UseTypePipeline attribute setting. It never reads initializerList[1] and has no non-generic _pipeline field. This means async handlers always require a per-command-type generic pipeline registration.

2. No fallback from generic to non-generic lookup: When UseTypePipeline = true is set on the sync handler (ResilienceExceptionPolicyHandler), it calls GetPipeline<TRequest>(key) which throws KeyNotFoundException if only a non-generic pipeline was registered with that key. There is no fallback to check for a non-generic pipeline.

The reporter's scenario: they register a non-generic ResiliencePipeline (e.g., a timeout pipeline named "MyNonGenericTimeoutPipeline") and reference it from a handler via [UseResiliencePipeline("MyNonGenericTimeoutPipeline", 1)]. The async handler always uses the generic lookup path, causing KeyNotFoundException.

Solution

Fix the resilience pipeline handlers so that:

1. Both sync and async handlers respect the UseTypePipeline flag
2. When UseTypePipeline = false (the default), use GetPipeline(key) to get a non-generic pipeline that can be shared across command types
3. When UseTypePipeline = true, use GetPipeline<TRequest>(key) to get a type-specific pipeline
4. Optionally: when UseTypePipeline = true and the generic lookup fails, fall back to the non-generic lookup (graceful degradation)

[claude]

PR Review: Resilience Pipeline Can Be Reused - This is a well-targeted bug fix. The root cause analysis is accurate and the solution correctly mirrors the sync handler pattern. See detailed notes below.

[claude]

Core Fix - ResilienceExceptionPolicyHandlerAsync.cs: The change is correct and minimal. The async handler now properly reads initializerList[1] to determine the pipeline type and branches accordingly, matching the sync handler logic line-for-line. Field naming (_pipeline for non-generic, _typePipeline for typed) is consistent with the sync handler. The _initialized guard is preserved. The removed using directives (System.Collections.Generic, Paramore.Brighter.Extensions) were no longer needed.

[claude]

Detailed Review Notes (continued)

ConfigureAwait inconsistency (pre-existing, not introduced here):

In the HandleAsync cancellationToken branch (no ResilienceContext), the inner lambda is missing ConfigureAwait on the inner await:

await _pipeline.ExecuteAsync(async ct => await base.HandleAsync(command, ct), cancellationToken)
    .ConfigureAwait(ContinueOnCapturedContext);

The ResilienceContext branch does include ConfigureAwait on the inner await. This inconsistency exists in the pre-existing _typePipeline path and is carried over here. Not a regression from this PR, but worth a follow-up tidy.

---

Tests

Three new tests verify the happy path (command received, no exception, zero retries). Good for confirming pipeline resolution.

Gap - no failure/retry test for the async non-generic path:

The existing suite has MyFailsWithDivideByZeroWithResiliencePipelineHandler for the sync path (handler throws, retry policy fires). No async equivalent is added here. A test that sends a command through an async non-generic pipeline where the handler throws, and verifies the retry counter increments, would give stronger confidence the pipeline is actually executing - not just resolving without error.

Test double placement:

When_Sending_Different_Async_Commands_That_Share_A_ResiliencePipeline.cs defines MyOtherCommand, MyCommandHandlerWithSharedPipelineAsync, and MyOtherCommandHandlerWithSharedPipelineAsync inline at the bottom of the test file. The project convention places these in TestDoubles/. Minor deviation from convention.

---

Summary

Area	Status
Bug fix correctness	Correct, minimal, mirrors sync handler exactly
Breaking changes	None
Happy-path test coverage	Three new tests: non-generic, typed, and shared paths
Failure/retry test for async non-generic	Missing - gap vs sync handler tests
Test double placement	Inline in test file (minor convention deviation)
Documentation	ADR and spec are thorough
Pre-existing ConfigureAwait inconsistency	Carried over, not introduced here

The fix is correct and well-documented. Main follow-up: a retry-on-failure test for the async non-generic path to close the coverage gap relative to the sync handler test suite.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

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]

CLAUDE.md

This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

How to Use This File

This file contains instructions for Claude Code to help it understand the context and requirements of the project. It is not intended to be modified by contributors. Human contributors should follow the guidelines in the CONTRIBUTING.md file. These guidelines derive from that document.

⛔ TDD Workflow (MANDATORY - NOT OPTIONAL)

When working on implementation tasks in specs/*/tasks.md:

• ALWAYS use /test-first <behavior> for TEST tasks
• NEVER write tests manually and proceed to implementation
• STOP and ASK FOR APPROVAL after writing each test
• The user will review the test in their IDE before you implement
• Each TEST task in tasks.md specifies the exact /test-first command to use
• The skill enforces the approval gate automatically - you cannot bypass it

Why this is mandatory:

1. Tests correctly specify desired behavior before implementation
2. Scope control - only code required by tests is written
3. No speculative code
4. User reviews test in IDE, not in CLI output

If a task says /test-first when ... - YOU MUST USE THAT COMMAND. Do not write the test file manually.

Claude Code Skills (Recommended)

Claude Code skills automate common workflows and enforce mandatory engineering practices. Use these skills proactively rather than manually following documented procedures:

• Skills Overview - Quick reference for all available skills
• Detailed Skills Documentation - Complete documentation for all skills

Core Development Skills

• /test-first <behavior> - TDD workflow with mandatory approval before implementation (docs)
• /tidy-first <change> - Separate structural (refactoring) from behavioral (feature) changes (docs)
• /adr <title> - Create Architecture Decision Records (docs)

Specification Workflow Skills

• /spec:requirements, /spec:design, /spec:tasks, /spec:implement, /spec:status - Complete specification-driven development workflow (docs)

When to use skills:

• Use /test-first when adding new behavior or fixing bugs
• Use /tidy-first when code needs refactoring before/during feature work
• Use /adr when documenting architectural decisions
• Use /spec:* commands for full feature development from requirements to implementation

Detailed Instructions

For comprehensive guidance on working with this codebase, Claude should read the following files as needed:

• Build and Development Commands - Build scripts, test commands, and Docker setup
• Project Structure - Organization of the codebase and testing framework
• Code Style - C# conventions and architectural patterns
• Design Principles - Responsibility-Driven Design and architectural guidance
• Testing - TDD practices, test structure, and testing guidelines
• Generated Tests - Test generator templates, configuration, and regeneration workflow
• Documentation - XML documentation standards and licensing requirements
• Dependency Management - Package management with Directory.Packages.props

[claude]

PR Review: Resilience Pipeline Can Be Reused\n\nThis is a clean, well-scoped bug fix. The root cause is clearly identified and the solution correctly mirrors the sync handler.\n\nSummary\n\nThe core change in ResilienceExceptionPolicyHandlerAsync.cs is correct. The async handler now:\n1. Reads UseTypePipeline from initializerList[1]\n2. Branches to populate either _pipeline (non-generic) or _typePipeline (generic)\n3. Uses _pipeline != ResiliencePipeline.Empty as the runtime discriminator in HandleAsync\n\nThe result is a line-for-line alignment with the sync ResilienceExceptionPolicyHandler - exactly the right approach for a parity bug fix.\n\n---\n\nIssues\n\nMinor: Typo in test filename\n\nWhen_Sending_An_Async_Command_That_Repeatedely_Fails_Break_The_Circuit_With_ResiliencePipeline.cs - Repeatedely should be Repeatedly. Worth fixing for consistency.\n\nMinor: Missing static constructor initialization in two new test doubles\n\nMyCommandHandlerWithSharedPipelineAsync and MyOtherCommandHandlerWithSharedPipelineAsync declare ReceivedCommand as a static property but have no static constructor to set the default. The test resets them in its own constructor so this works, but the pattern used across all other test doubles in this directory uses a static constructor. This is inconsistent and slightly fragile if the handler is reused in a future test that does not reset the flag.\n\nMinor: Missing MIT license header on three new test doubles\n\nMyCommandHandlerWithSharedPipelineAsync.cs, MyOtherCommandHandlerWithSharedPipelineAsync.cs, and MyOtherCommand.cs omit the license block present on all other files in the same directory. MyFailsWithDivideByZeroWithResiliencePipelineHandlerAsync.cs correctly includes the header.\n\n---\n\nObservations (not issues)\n\ninitializerList[1] bounds safety - Both the new async handler and the existing sync handler access initializerList[1] without bounds checking. This is pre-existing and not introduced by this PR. The attribute always passes both parameters via InitializerParams() so it is not a practical risk.\n\n**_pipeline != ResiliencePipeline.Empty as discriminator** - This works correctly because the two fields are mutually exclusive: only one is assigned a real pipeline during initialization. The edge case where ResiliencePipeline.Empty itself is returned from the registry would cause incorrect dispatch, but that is an unrealistic configuration.\n\nUnused using removals - System.Collections.Generic and Paramore.Brighter.Extensions are correctly removed.\n\nTest coverage - The four new test classes cover all the required paths: non-generic pipeline success, typed pipeline success, shared pipeline across two command types, and circuit breaker trip. This matches the acceptance criteria in the spec.\n\n---\n\nVerdict\n\nThe implementation is correct, minimal, and well-tested. The three issues above are style/consistency nits rather than functional problems. The fix is safe to merge once those are addressed (or accepted as-is if the team prefers).

[iancooper]

@lillo42, want to give this the once over?