fix: move inbox attribute addition outside static pipeline cache

[holytshirt]

Summary

• Moves AddGlobalInboxAttributes / AddGlobalInboxAttributesAsync calls outside the static s_preAttributesMemento cache block in both BuildPipeline and BuildAsyncPipeline
• The cache now only stores the handler's declared pipeline attributes — inbox attributes are applied fresh each time based on the current _inboxConfiguration
• Previously, inbox attributes were cached alongside handler attributes, causing cross-contamination between pipeline builders with different inbox configurations:
  • Tests without inbox config would pick up cached inbox attributes from earlier tests → ArgumentOutOfRangeException when the handler factory couldn't create UseInboxHandlerAsync<T>
  • Tests with inbox config hitting a cache populated without inbox → inbox handler never added → inbox assertions failed
  • On cache miss with inbox config, the async version had a duplicate call that added the inbox handler twice

Test plan

• CI core tests pass (557 total, 547 passed, 0 failed)
• AsyncCommandProcessorPublishObservabilityTests — previously failing, now pass
• CommandProcessorBuildDefaultInboxPublishAsyncTests — previously failing, now passes
• No regressions in other pipeline builder tests

🤖 Generated with Claude Code

[claude]

Code Review

The PR has evolved well from the initial version. Here is a fresh assessment of the current state.

What is now correct

Cache isolation fix - AddGlobalInboxAttributes(Async) is applied after the cache block, so s_preAttributesMemento stores only the handler declared attributes. Per-instance inbox configuration is applied dynamically on every call.

Async duplicate-call removed - The original BuildAsyncPipeline added the inbox attribute twice on cache miss. Now both sync and async paths are symmetric.

PushOntoAttributeList no longer mutates cached objects - Replacing handler.Step++ with minStep - 1 is a significant correctness improvement. The old code incremented Step on RequestHandlerAttribute instances held inside the static cache, so step values grew unboundedly across the lifetime of a service. The new approach computes the inbox step relative to the original unmodified cached values on each call.

Both reversal directions tested - All four test scenarios are now present: with-inbox to without-inbox, without-inbox to with-inbox, and both async equivalents. This was the main gap in earlier iterations.

Redundant null check removed - The dead null guard after the early return is gone from both AddGlobalInboxAttributes and AddGlobalInboxAttributesAsync.

---

Remaining observations

FindHandlerMethod() called twice per add - In AddGlobalInboxAttributes(Async), FindHandlerMethod() is called once for HasNoInboxAttributesInPipeline() and once for HasExistingUseInboxAttributesInPipeline(). Since this now runs on every pipeline build (not just cache misses), the double reflection hit is more frequent than before. Caching the MethodInfo in a local variable would halve the reflection cost. Low priority follow-up.

s_postAttributesMemento is never populated (pre-existing) - In both BuildPipeline and BuildAsyncPipeline, when post-attributes are not in the cache they are computed but never added via TryAdd. Post-attributes are therefore re-reflected on every call. This predates the PR but worth a follow-up issue.

Step underflow is benign but undocumented - When existing attributes include step 0, minStep - 1 equals -1. Ordering remains correct (inbox sorts last in descending order, meaning outermost in the pipeline). The existing comment explains the invariant but noting that steps can be negative would help maintainers not be surprised by diagnostic output.

---

Summary

Area	Status
Cache contamination fix	Correct
Async duplicate-call fix	Correct
Mutation of cached attributes	Fixed
Both cache-reversal directions tested	Now covered
Redundant null check	Removed
Double FindHandlerMethod() reflection per call	Minor follow-up
s_postAttributesMemento never populated	Pre-existing follow-up

The fix is correct and complete. Ready to merge.

[iancooper]

TBH I never really liked the global inbox attribute but we came under such pressure from the CTO at Huddle to add it. The problem it creates is why not global outbox, resilience pipeline etc, which gets into fluent over attribute syntax questions

[holytshirt]

@iancooper are we okay with the order being negative, I assume it's not a problem, from reviewing the other code I think it's fine.

[iancooper]

I guess

[iancooper]

Let us know when you are done

[claude]

Code Review

Summary

This PR fixes a genuine cache-contamination bug in PipelineBuilder<TRequest>. Because AddGlobalInboxAttributes was called inside the TryGetValue miss block, inbox attributes were baked into the static cache. Any subsequent builder with a different (or absent) _inboxConfiguration would reuse contaminated cached attributes, causing either spurious ArgumentOutOfRangeException failures or missing inbox handlers. Moving the calls outside the cache block is the correct fix.

---

What's Good

• Root cause is correctly identified and fixed. The cache now stores only the handler's declared pipeline attributes; inbox attributes are applied fresh on every build from the current instance's _inboxConfiguration.
• PushOntoAttributeList is significantly improved. The old implementation called handler.Step++ on every cached attribute — mutating objects that live in the static cache across all future pipeline builds. The new minStep - 1 approach avoids mutating any cached object.
• Dead code removed. The redundant if (_inboxConfiguration is null) throw (unreachable because the preceding == null guard already returns) is gone.
• Tests cover the exact failure scenarios described in the PR summary (with-then-without, without-then-with, both sync and async).

---

Issues and Suggestions

1. Test isolation — parallel execution risk (medium)

Each test starts with PipelineBuilder<MyCommand>.ClearPipelineCache() but does not clear it in teardown. s_preAttributesMemento is a static field shared across all PipelineBuilder<MyCommand> instances in the process. xUnit runs test classes in parallel by default. If another test class touching MyCommand handlers runs concurrently, either the new tests or the other tests may see unexpected cached state.

Suggestion: Add a Dispose / IAsyncLifetime teardown that calls ClearPipelineCache(), or use [Collection("PipelineBuilderTests")] on any tests that rely on or modify the static cache to enforce serial execution.

2. Step value passed to inbox attribute constructors is immediately discarded (minor)

var useInboxAttribute = new UseInboxAttribute(
    step: 0,   // ← this value is overwritten unconditionally in PushOntoAttributeList
    ...

PushOntoAttributeList always overwrites requestHandlerAttribute.Step with minStep - 1. The initial value 0 is never used. While the existing comment in PushOntoAttributeList warns callers not to cache/reuse the attribute, it doesn't signal that the initial step argument is meaningless.

Suggestion (optional): Either update the comment here to say the initial step is overwritten, or change the UseInboxAttribute / UseInboxAsyncAttribute construction to not pretend to supply a meaningful step (e.g., use a sentinel like -1 with a note that it will be set by the caller).

3. PushOntoAttributeList may produce negative step values (low risk, worth noting)

When existing attributes occupy steps 0, 1, 2 …, the inbox attribute gets minStep - 1 = -1. This is fine as long as nothing in the framework assumes non-negative steps. However, if a handler declares attributes with step int.MinValue, this would underflow. This is an extremely unlikely edge case, but worth having on record.

4. Leading whitespace on line 364 (nit)

             PushOntoAttributeList(ref preAttributes, useInboxAttribute);

There is an extra leading space before PushOntoAttributeList. Same on line 384 in AddGlobalInboxAttributesAsync. Minor but inconsistent with surrounding code.

5. preAttributes in the cache is a deferred LINQ chain (pre-existing, informational)

The value stored in s_preAttributesMemento is IOrderedEnumerable<RequestHandlerAttribute> produced by ...GetOtherHandlersInPipeline().Where(...).OrderByDescending(...) — a deferred LINQ expression. On every cache hit, this chain re-executes the reflection. This isn't introduced by this PR, but it's worth noting for a future optimisation: materialising to List<RequestHandlerAttribute> before storing would avoid repeated reflection calls.

6. Post-attributes cache is never populated for sync pipelines (pre-existing, informational)

BuildPipeline reads s_postAttributesMemento but never writes to it on a miss, meaning post-attributes are re-computed from reflection on every build. Again, pre-existing and out of scope for this PR — just flagging it.

---

Verdict

The fix is correct and the approach is sound. The PushOntoAttributeList refactor is a nice side-benefit — it removes the latent bug of mutating cached attribute objects. The test coverage directly exercises the failure modes described in the PR. The issues above are minor; only the parallel test isolation point (item 1) warrants attention before merge.

🤖 Reviewed with Claude Code

[claude]

Code Review

Overview

This PR fixes a genuine cache-contamination bug in PipelineBuilder: inbox attributes were being stored inside the static s_preAttributesMemento cache block, causing cross-contamination when two PipelineBuilder instances sharing the same handler type had different inbox configurations. The fix (moving AddGlobalInboxAttributes / AddGlobalInboxAttributesAsync calls outside the cache block) is correct and well-targeted.

---

What Works Well

• Root cause correctly identified and fixed. The static cache should only store handler-declared (compile-time) pipeline attributes. Inbox configuration is instance-level and must be applied fresh every build. Moving the calls outside the if (!TryGetValue) block achieves exactly this.
• PushOntoAttributeList refactor avoids attribute mutation. The old implementation incremented handler.Step++ on each existing attribute, which mutated the objects returned by reflection. Since those instances may be cached by the .NET runtime via GetCustomAttributes, that mutation could have had surprising side-effects. The new approach (computing minStep and assigning requestHandlerAttribute.Step = minStep - 1) is cleaner.
• Duplicate null check removed. The back-to-back _inboxConfiguration == null / _inboxConfiguration is null guard was dead code — the second branch could never be reached. Good cleanup.
• Test coverage is excellent. Four tests cover all permutations (sync/async × inbox-first/no-inbox-first) and each verifies the pipeline trace to confirm the inbox handler is present or absent. ClearPipelineCache() at the start of each test properly isolates the shared static state.

---

Issues and Suggestions

1. step: 0 initial value is now misleading (minor)

var useInboxAttribute = new UseInboxAttribute(
    step: 0,    // <-- immediately overwritten by PushOntoAttributeList
    ...

PushOntoAttributeList unconditionally overwrites Step with minStep - 1, so the 0 passed here is never visible. Consider passing an obviously-invalid sentinel like -1 or a named constant, or add a brief inline comment, so the reader isn't misled into thinking 0 is meaningful.

2. Integer underflow edge case in PushOntoAttributeList

requestHandlerAttribute.Step = minStep == int.MaxValue ? 0 : minStep - 1;

If minStep is int.MinValue, this overflows to int.MaxValue. In practice this would require a handler with int.MinValue step values, which is contrived — but a checked subtraction or a guard would make the invariant explicit.

3. s_postAttributesMemento is never written (pre-existing dead cache)

// BuildPipeline (and BuildAsyncPipeline) — post-attributes block
if (!s_postAttributesMemento.TryGetValue(..., out ... postAttributes))
{
    postAttributes = ... // computed
    // ← s_postAttributesMemento.TryAdd is never called here
}

The read path checks the cache but the write path (TryAdd) is absent for post-attributes in both sync and async variants — only s_preAttributesMemento.TryAdd is called. The post-attributes cache is effectively never populated, so it's always a miss and the computation runs on every pipeline build. This is a pre-existing bug not introduced by this PR, but since you're already touching this file it would be worth fixing or filing a follow-up issue.

4. FindHandlerMethod() called twice per AddGlobalInboxAttributes[Async] invocation (minor perf)

|| implicitHandler.FindHandlerMethod().HasNoInboxAttributesInPipeline()
|| implicitHandler.FindHandlerMethod().HasExistingUseInboxAttributesInPipeline()

If FindHandlerMethod() involves reflection without an internal cache, this doubles the reflection cost on every pipeline build. Consider caching the result in a local variable.

---

Summary

The core fix is correct and the test suite for the new behaviour is thorough. The main thing worth addressing before merge is clarifying the misleading step: 0 initializer. The dead s_postAttributesMemento write is a separate pre-existing bug — worth a follow-up issue if not fixed here. The rest are minor.

🤖 Generated with Claude Code

[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]

Code Review — PR #4061: Fix inbox attribute addition outside static pipeline cache

Overall assessment: Approve with minor notes. The fix is correct, well-targeted, and backed by solid regression tests. A few observations below.

---

What the PR does

The PipelineBuilder uses a static ConcurrentDictionary (s_preAttributesMemento) to cache handler pipeline attributes per handler type. Previously, AddGlobalInboxAttributes / AddGlobalInboxAttributesAsync were called inside the cache-population block — so inbox configuration was baked into the cached entry. This caused cross-contamination: a second PipelineBuilder instance sharing the same static cache but with a different (or no) _inboxConfiguration would get the wrong pipeline.

The fix correctly moves the inbox attribute injection to after the cache read/write, so the cache only stores handler-declared attributes, and inbox is applied fresh on every pipeline build from the instance's _inboxConfiguration.

---

Code quality

Positives:

• The fix is minimal and surgically correct. Exactly the right change for the described bug.
• PushOntoAttributeList is a meaningful improvement — the old code mutated handler.Step++ on attributes that are references to cached objects. This was a latent corruption bug that would have manifested if PushOntoAttributeList were ever called more than once per cache entry. The new approach (assign a step below the current minimum without touching existing objects) is strictly safer.
• Removed the redundant dead code: if (_inboxConfiguration is null) throw ... was unreachable after the _inboxConfiguration == null guard two lines above. Good cleanup.
• PipelineBuilder<MyCommand>.ClearPipelineCache() → PipelineBuilder<MyEvent>.ClearPipelineCache() in the publish tests is a correct fix — those tests publish MyEvent, not MyCommand, so the wrong type was being cleared.
• ConcurrentDictionary for _receivedMessages in the publish tests is a good defensive improvement for async multi-subscriber scenarios.

---

Observations and suggestions

1. step: 0 in inbox attribute constructors is a misleading no-op

In both AddGlobalInboxAttributes and AddGlobalInboxAttributesAsync, the attribute is created with step: 0:

var useInboxAttribute = new UseInboxAttribute(step: 0, ...);

But PushOntoAttributeList immediately overwrites requestHandlerAttribute.Step with minStep - 1. The initial step: 0 is never used. A follow-up could either document this explicitly at the call site (step: 0 /* overridden by PushOntoAttributeList */) or adjust the comment in PushOntoAttributeList to reference this caller contract more directly.

2. Performance regression on hot paths (minor, worth noting)

AddGlobalInboxAttributes is now called on every BuildPipeline invocation — including cache hits. The old code only called it on cache miss. Each call goes through FindHandlerMethod() up to three times (for HasNoInboxAttributesInPipeline(), HasExistingUseInboxAttributesInPipeline(), and the context delegate call). If FindHandlerMethod() uses reflection, this could add measurable overhead on high-throughput message pipelines that reuse the same handler type.

This is a correct trade-off for correctness, but it's worth benchmarking in scenarios with high BuildPipeline call rates (e.g., subscribers that see thousands of messages/sec).

3. Test isolation via static cache

The new test class correctly calls PipelineBuilder<MyCommand>.ClearPipelineCache() at the start of each [Fact]. However, the static cache is shared across all tests in the process. If the test runner executes multiple test classes in parallel and those classes also touch MyCommand pipelines, there is potential for flaky interference. This is a pre-existing concern with the static cache design, not introduced by this PR, but the new tests make it more visible. A [Collection("PipelineCacheTests")] attribute across all pipeline-touching test classes would make this robust.

4. Post-cache preAttributes mutation is safe — but worth a comment

After a cache hit, preAttributes is a reference to the cached IOrderedEnumerable. PushOntoAttributeList creates a new list from it (via preAttributes.Each(...) + new List<>) and then reassigns the local ref preAttributes — it does not mutate the cached object. This is correct but non-obvious. A one-line comment on the TryGetValue + AddGlobalInbox block clarifying "inbox is applied to a local copy; the cached entry is not modified" would help future readers.

---

Test coverage

The 4 new tests in When_Building_A_Pipeline_Inbox_Cache_Does_Not_Leak_Across_Configurations cover all four combinations of ordering (with-inbox-first, without-inbox-first) × sync/async. This directly reproduces the bug scenarios described in the PR and is a good regression suite.

---

Summary

The fix is correct and complete. The PushOntoAttributeList improvement (avoiding step mutation of cached attributes) is a valuable bonus fix. The performance consideration in point 2 is worth a note in a follow-up issue if Brighter targets high-throughput scenarios. Everything else is minor polish.

🤖 Generated with Claude Code