fix: message-pump observability fixes (#4086, #4089, #4090)

[iancooper]

Summary

Rolls up three independent message-pump observability fixes, each developed diagnosis-first via the /bugfix workflow (triage → confirm → test-first → fix → verify). All three touch the consumer pump tracing/metrics path and each ships with regression tests.

Issue	Fix	Area
#4086	Exclude the pump begin span from the client-operation duration metric	BrighterMetricsFromTracesProcessor
#4089	Serialize the message header once per message, not twice	Message / BrighterTracer
#4090	End the pump and process spans on exception paths	Reactor / Proactor / BrighterTracer

#4086 — Pump begin span skews messaging.client.operation.duration

Symptom: every time a consumer pump (Reactor.Run / Proactor.EventLoop) shuts down, an outlier equal to the pump's entire wall-clock lifetime (minutes → days) is recorded into the messaging.client.operation.duration histogram, making p50/p95/p99 unusable for SLOs.

Confirmed root cause: BrighterMetricsFromTracesProcessor.OnEnd recorded the long-lived pump begin span into the client-operation-duration histogram via the default arm of its inner switch (operation). The span opens at pump start and closes at shutdown, so its Activity.Duration is the whole pump lifetime.

Fix: added case "begin": break; immediately before the default arm — short-circuits only the pump-lifetime span; every other operation still records. (Confirm proved the issue's preferred "drop the default arm" would have regressed create/deposit/send/clear/archive/scheduler/settle.)

#4089 — Message header serialized twice per message

Symptom: for every serviceable message, JsonSerializer.Serialize(message.Header, …) runs twice — once enriching the receive span, once creating the process span — a per-message hot-path CPU/allocation regression introduced when #4085 split the consumer span into receive + process.

Confirmed root cause: the receive span and process span each independently full-serialize the header (reflection over Bag + Baggage) under the default InstrumentationOptions.Messaging flag. 2× per serviceable message, 1× for MT_UNACCEPTABLE. No correctness defect — purely performance.

Fix: added an internal lazy-cached Message.HeaderJson (snapshot on first access) and pointed both pump-path sites (EnrichReceiveSpan, CreateSpan(Process)) at it. Header now serializes once per message; both spans share the same string. No public API change; MessageBody/RequestBody left untouched (plain string, no reflection).

#4090 — Pump and process spans leak on exception paths

Symptom: two Activity spans can be started but never ended on exception paths — leaking activities, polluting Activity.Current, and skewing/dropping span durations.

Confirmed root cause:

• pumpSpan — EndSpan(pumpSpan) sat after the receive loop with no try/finally, so a throw out of the loop (e.g. the deterministic message-is-null path) skipped it and leaked the begin activity. Deterministic; symmetric in Reactor and Proactor.
• processSpan — CreateSpan's post-start enrichment (TraceStateString, baggage, Activity.Current) ran outside the caller's try, so a throw there orphaned an already-started activity. (Confirm corrected the issue here: mirroring the receive-span pattern in the caller would have been a no-op — the fix had to live inside CreateSpan.)

Fix: wrapped the receive loop in try/finally { EndSpan(pumpSpan) } in both pumps (exception still propagates, preserving shutdown semantics); and inside CreateSpan (Process overload) a post-start throw now ends the activity (status Error) and rethrows instead of leaking it.

Testing

• Per-fix regression tests added and green on net9.0 + net10.0.
• Full Paramore.Brighter.Core.Tests suite: 843 passed, 0 failed, 7 pre-existing skips (net10.0).

Also included

• fix(bugfix): remove illegal command substitution from gate preflights — a small fix to the /bugfix skill's preflight scripts, unrelated to runtime behaviour.

Fixes #4086
Fixes #4089
Fixes #4090

🤖 Generated with Claude Code

[claude]

Review: message-pump observability fixes (#4086, #4089, #4090)

Thanks for the well-organized PR. The diagnosis-first /bugfix write-ups make this easy to follow, the three concerns are cleanly separated, and each ships a regression test. The changes are appropriately minimal and respect the project's "change only what was asked" guidance. Below is what I found.

Strengths

• Pump-lifetime span (MessagePumpSpanOperation.Begin) recorded into messaging.client.operation.duration histogram #4086 — case "begin": break; is the right surgical fix. The bugfix.md reasoning for rejecting the issue's preferred "drop the default arm" (it would have silently stopped recording create/deposit/send/clear/archive/scheduler/settle) is exactly the kind of adversarial check CLAUDE.md asks for, and the over-suppression guard test (When_ending_a_send_span_should_still_record_a_client_operation) locks that in.
• Pump observability: pumpSpan and processSpan can leak on exception paths #4090 — Correctly identified that mirroring the receive-span try/finally in the caller would be a no-op for processSpan, and that the fix must live inside CreateSpan itself. Relying on EndSpan's null-guard (if (span is null) return;) to keep the finally wraps safe is a nice touch. Reactor/Proactor parity is maintained.
• Tests follow repo conventions (When_… naming, GWT structure, licence headers), are RED-verified, and the Assert.Same reference-equality check in When_A_Message_Is_Dispatched_The_Header_Is_Serialized_Once is a clever and reliable way to prove single serialization (JSON output is never interned).

Things to consider

1. #4089 — process span silently loses the correlationId baggage entry (behavioral change, untested).
The HeaderJson snapshot is taken in EnrichReceiveSpan (line 303) before correlationId is added to Header.Baggage (line 317). Previously the process span re-serialized in CreateSpan after that mutation, so its MessageHeaders tag included the correlationId baggage entry; now both spans share the pre-mutation snapshot, so the process span no longer carries it. You documented this, and ConversationId still carries the value as a separate tag — but the new test uses a message with an empty CorrelationId, so the Assert.Equal(expectedHeaderJson, processHeaderJson) assertion never exercises the difference. Consider adding a case with a non-empty CorrelationId to pin the intended behavior (or confirm it's acceptable that the process-span header diverges from the live header).

2. #4089 — _headerJson ??= is not thread-safe.
Fine for the single-threaded pump path that drives it today, but HeaderJson lives on the general-purpose Message type. If a Message instance were ever serialized from two threads concurrently, the field could be assigned twice (harmless — equal values — just a lost-cache double-serialize). A one-line note in the <remarks> would save a future reader the analysis.

3. #4089 — coupling Message to an observability concern.
Memoizing on Message makes the model aware of tracing, and the snapshot can go stale against a mutable MessageHeader (Baggage, HandledCount). The internal + documented-snapshot approach is a reasonable, low-risk tradeoff, but threading the pre-serialized string through the pump call chain (rather than caching on the entity) would keep Message observability-agnostic. Acknowledged design call — flagging for awareness, not blocking.

4. Repo hygiene — /bugfix workflow scratch state is committed.
The bugfix.md diagnosis docs are genuinely valuable to keep. But bugfixes/.current-bug (pointing at 0002…), the empty .confirm-approved markers, and .issue-number files look like the bugfix workflow's local state leaking into the repo. Consider .gitignore-ing those marker/state files (keeping only bugfix.md), or dropping them from the PR.

5. Follow-up (out of scope, pre-existing) — a poison message can still tear down the pump.
With #4090, when CreateSpan(Process) enrichment throws (the test's correlationId: "bad=value" → Baggage validation ArgumentException), the activity is now correctly ended instead of leaked — good. But the exception still propagates out of the loop, and with the new pumpSpan try/finally it now ends the pump cleanly. So a single message with a malformed CorrelationId from a producer can crash the consumer. That fatal behavior is pre-existing (not introduced here), but since observability should never take down the pump, a follow-up issue to make enrichment failures non-fatal (catch + continue) seems worthwhile.

Nits

• NullReturningChannel.Receive returning null! is fine for a forced-failure test double.

Test coverage

Good overall — each fix has a dedicated regression test plus, for #4086, an over-suppression guard. The one gap is the #4089 correlationId divergence noted in (1).

Nice work. The functional changes look correct; my only substantive ask is to decide on the #4089 correlationId behavior (and ideally test it), and to reconsider committing the /bugfix scratch-state files.

_{🤖 Automated review}

[iancooper]

Thanks for the thorough review. Points 1 and 3 made me reconsider whether caching the serialized header on Message was the right approach at all, so I reworked #4089 rather than patching the cache.

The reframing: CorrelationId is already a top-level serialized property of MessageHeader, so the serialized header always carries it. The only thing the process span lost (vs. pre-#4089) was a redundant duplicate of that value nested inside the serialized Baggage sub-object — the same value that's also on the dedicated ConversationId tag of both spans. That turns point 1 from "lost value" into "redundant duplicate," which made a cleaner fix possible.

What changed:

1. Removed Message.HeaderJson entirely — Message is observability-agnostic again (point 3: no coupling, no stale snapshot, no thread-safety question).

2. Serialize the as-received header once, share it. EnrichReceiveSpan now serializes the header once and returns the string; the pump threads it into CreateSpan(Process), which reuses it (falling back to serializing only when there's no receive span). Both spans now carry the identical header tag — the existing Assert.Same invariant is now intentional rather than an accident of caching (point 1).

3. Separated baggage propagation from serialization. The correlationId → Baggage + SetBaggage step is propagation setup, not "the headers," so it moved out of both span methods into a single PropagateConsumerContext(message) call made once per message by the pump. As a side effect, a malformed correlationId now surfaces there and is caught non-fatally by the pump — so a poison message no longer tears the consumer down (eases point 5).

Tests:

• Added a non-empty-correlationId case asserting both spans share one header and both carry the id (the gap flagged in point 1).
• Reworked the now-moot When_Create_Span_Throws_After_Starting… test (its baggage-throw-inside-CreateSpan scenario no longer exists by construction) into one asserting the decoupled contract; the validation now surfaces from PropagateConsumerContext.
• Added a pump-level test proving a malformed correlationId doesn't stop the pump.

Full Paramore.Brighter.Core.Tests: 845 passed, 0 failed, 7 pre-existing skips on net9.0 + net10.0.

On point 4 (committed /bugfix scratch state) — separate concern, happy to .gitignore the marker files in a follow-up.

_{🤖 Generated with Claude Code}

[claude]

Review: message-pump observability fixes (4086, 4089, 4090)

Reviewed the full diff against CLAUDE.md conventions. This is a strong, well-disciplined PR — three independently-scoped fixes, each diagnosis-first with regression tests, applied symmetrically across Reactor and Proactor, and no public API breaks. The evolution in the last two commits (dropping the Message.HeaderJson cache in favour of threading the serialized header through the pump, and extracting PropagateConsumerContext) is a genuine improvement: it keeps Message observability-agnostic and removes a per-message double baggage-add. Nicely done.

Below are findings — mostly minor; nothing blocking.

4086 — exclude begin span from client-operation duration (OK)

• case "begin": break; is the correct minimal fix. Verified "begin" maps only from MessagePumpSpanOperation.Begin (BrighterSpanExtensions.cs:69); CommandProcessorSpanOperation never produces it, so create/deposit/send/clear/archive/scheduler are untouched. The over-suppression guard test (When_ending_a_send_span_should_still_record_a_client_operation) locks that in — good adversarial coverage.
• Nit: the switch keys on magic strings ("publish"/"receive"/"process"/"begin"). This matches the pre-existing style so it is consistent, but a shared constant (or keying off the enum) would be more refactor-safe. Not worth changing here.

4089 — serialize the header once (OK)

• The threaded-serializedHeader design is clean and the Assert.Same test (..._Both_Spans_Share_One_Header) is a precise way to prove single serialization. The correlationId-preservation assertions (top-level field plus ConversationId tag) directly answer the earlier review concern about lost data.
• The serializedHeader ?? JsonSerializer.Serialize(...) fallback adds an optional positional param to the public IAmABrighterTracer.CreateSpan. It is documented and defaulted (non-breaking), but it does trade Message-to-observability coupling for a small leak of a pump-internal optimization into the public tracer contract. Acceptable given the goal; flagging as a conscious trade-off.

4090 — end spans on exception paths (OK, one note)

• The try/finally { EndSpan(pumpSpan) } wrap in both pumps is correct and preserves shutdown semantics (propagates, does not swallow). Parity tests for both pumps are good.
• The CreateSpan post-start try/catch is now largely defensive-only. The 4090 fix added it to catch a throw from baggage propagation after StartConsumerActivity. But the later refactor moved baggage out of CreateSpan into PropagateConsumerContext. The only remaining post-start statements are activity.TraceStateString = traceState and Activity.Current = activity, neither of which realistically throws. So the guard no longer protects against a live failure mode, and the renamed test (..._Baggage_Propagation_Is_Decoupled) no longer exercises a genuine post-start throw. Keeping it as defense-in-depth is fine, but consider a one-line comment noting it is purely defensive (the real malformed-correlationId path is now handled in the pump), so a future reader does not assume it is load-bearing.

Cross-cutting

1. Possible (caught) NRE via PropagateConsumerContext on a null message. In the pump, both EnrichReceiveSpan(receiveSpan, message, ...) and PropagateConsumerContext(message) run before the if (message is null) check. When Tracer is non-null but receiveSpan is null (ActivitySource has no listeners) and Channel.Receive returns null, EnrichReceiveSpan short-circuits on its span is null guard and returns harmlessly — but PropagateConsumerContext(message) then dereferences message.Header.CorrelationId, giving an NRE. It is caught by the generic catch (Exception) and the pump still throws the intended NoMessageReceivedDescription, so the outcome is unchanged, but it logs a misleading ExceptionReceivingMessages NRE. EnrichReceiveSpan already had a (different) version of this exposure pre-PR; PropagateConsumerContext adds a new site. Cheap fix: a message is null guard at the top of PropagateConsumerContext (and/or EnrichReceiveSpan), or call both only after the null check. Low severity.

2. The bugfixes/ workflow artifacts are committed. bugfixes/.current-bug, */.confirm-approved, */.issue-number are ephemeral workflow-state markers (.current-bug even points at 0002, stale vs. the final state). The bugfix.md diagnosis docs may be worth keeping as records, but the marker files look like local scaffolding. Consider gitignoring the markers (and deciding whether the bugfix.md files belong in-repo vs. the PR description). Housekeeping only.

3. Pre-existing nullable note: passing Message? message into the non-nullable Message params of EnrichReceiveSpan/PropagateConsumerContext can raise CS8604; not introduced by intent but adjacent to the new call site.

Tests

Coverage is excellent for a fix PR: metric inclusion/exclusion, header single-serialization with Assert.Same, correlationId preservation, malformed-correlationId-is-non-fatal, and Reactor/Proactor parity for the pump-span leak. The NullReturningChannel(Async) doubles are a clean way to force the leak path.

Overall: approve with the minor notes above (the PropagateConsumerContext null guard and the now-defensive CreateSpan try/catch comment being the two most worth addressing).

Reviewed by Claude Opus 4.8 (1M context)

[claude]

Review — PR #4207: message-pump observability fixes

Thanks for the diagnosis-first workups — the three fixes are each small, well-targeted, and ship with focused regression tests. The core changes look correct. I do want to flag that the diff contains a fourth, undocumented change and that the PR description doesn't match the implementation in a couple of places.

✅ The three documented fixes look good

• Pump-lifetime span (MessagePumpSpanOperation.Begin) recorded into messaging.client.operation.duration histogram #4086 (begin span skews client.operation.duration) — case "begin": break; ahead of the default arm is the minimal correct change; the default still records create/deposit/send/clear/archive/scheduler/settle. When_ending_a_message_pump_begin_span_should_not_record_a_client_operation (plus the send-still-records companion) pins both sides. 👍
• Perf: message header serialized twice per message in pump observability hot path #4089 (double header serialization) — implemented cleanly by having EnrichReceiveSpan return the serialized header and threading it into CreateSpan(Process) via the new serializedHeader parameter, falling back to serializing only when there was no receive span. Assert.Same(...) in the regression tests is a nice way to prove a single allocation is shared.
• Pump observability: pumpSpan and processSpan can leak on exception paths #4090 (span leak on exception paths) — try/finally { EndSpan(pumpSpan) } around the loop in both pumps, plus the post-start try/catch in CreateSpan(Process) that ends the activity and rethrows, is symmetric and correct. This is strictly better than master, where a throw out of CreateSpan both crashed the pump and leaked the activity.

⚠️ Findings

1. (Scope / API) An undocumented fourth change is bundled in.
The PR introduces a new public interface member IAmABrighterTracer.PropagateConsumerContext(Message) and changes EnrichReceiveSpan's return type from void to string?, moving baggage propagation out of EnrichReceiveSpan/CreateSpan(Process) into a new pump step. None of this appears in the "three fixes" summary. Two concerns:

• Adding a member to a public interface is a source/binary breaking change for any external IAmABrighterTracer implementer (in-repo, only BrighterTracer and two test doubles implement it, so the repo builds — but downstream implementers will break).
• Per the repo's CLAUDE.md Change Scope guidance ("Do NOT change defaults or make changes beyond what was explicitly requested"), this refactor should at least be called out in the description, and ideally justified as part of Pump observability: pumpSpan and processSpan can leak on exception paths #4090 (it removes the Baggage.SetBaggage call that was the most likely thrower during CreateSpan enrichment) or split into its own PR.

2. (Docs) The PR description doesn't match the implementation for #4089.
The description says the fix added "an internal lazy-cached Message.HeaderJson (snapshot on first access)". The actual implementation does not touch Message.cs at all — it threads serializedHeader through the tracer instead. The implemented approach is arguably better (no mutable cached state on Message, no staleness risk if a header is mutated), but the mismatch will mislead anyone reading the PR later. Please update the description.

3. (Minor / correctness) Nullable annotation mismatch on PropagateConsumerContext.
The signature is void PropagateConsumerContext(Message message) (non-nullable), but the body guards if (message is null) return; and When_Propagating_Consumer_Context_For_A_Null_Message passes null!. Since the pump calls it before its own null-message check, the guard is genuinely needed — so the parameter should be Message? to make the contract honest; otherwise the null! in the test and the runtime guard contradict the declared type.

4. (Minor) Confirm the in-pump malformed-correlation-id path is intended degraded behavior.
PropagateConsumerContext now runs in the receive try, before the null/MT_NONE checks, for every received message. A correlation id that's invalid as baggage (e.g. containing =) makes Baggage.SetBaggage throw ArgumentException, which is swallowed by the generic catch (Exception ex) receive handler, logged as a receive failure, after which the message still proceeds to processing without baggage. The decoupled unit test confirms CreateSpan no longer throws, and the null test is covered, but there's no end-to-end pump test for the malformed-id path. (This is still better than master, where the second Baggage.Add in CreateSpan would have crashed the pump.) Worth a one-line comment or test asserting the intended outcome.

5. (Minor / housekeeping) Workflow scratch artifacts are being committed.
The diff adds bugfixes/.current-bug, bugfixes/000*/.confirm-approved, .issue-number, etc. .current-bug even points at 0002-double-header-serialization rather than the last bug (0003), confirming these are transient workflow state rather than durable docs. Consider .gitignore-ing the dotfiles (and deciding whether the bugfix.md write-ups belong in the repo) so per-run state doesn't land on master.

Nits

• case "begin": matches a magic string, consistent with the existing "publish"/"receive"/"process" arms — fine as-is, though a shared constant for all of them would be more robust.

Tests

Coverage for the three fixes is solid and the test names follow the When_... convention. The main gap is an end-to-end pump assertion for the swallowed-ArgumentException baggage path (finding #4). I couldn't run the suite here (build requires approval in this environment), so I'm trusting the stated 843 passed / 0 failed on net10.0.

Overall: the runtime fixes are correct and minimal. The main asks are documentation accuracy (#1, #2) and a decision on the bundled interface change and committed scratch files.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4207: message-pump observability fixes

Thanks for the careful, diagnosis-first work here. I reviewed all three fixes against the source and the new tests. Overall this is high quality: each fix is narrowly scoped to its root cause, Reactor/Proactor stay symmetric, and the regression test coverage is genuinely good (per-fix tests plus edge cases — null message, malformed correlation id, "send still records", "both spans share one header"). The metrics case "begin": break; correctly matches the MessagePumpSpanOperation.Begin.ToSpanName() == "begin" tag value, and the try/finally { EndSpan(pumpSpan) } wrapping preserves shutdown exception-propagation semantics. 👍

A handful of things worth addressing before merge — none are blockers for the runtime behaviour, but a couple are worth a quick fix.

1. The PR adds a code-style rule it then violates ⚠️

This PR adds to .agent_instructions/code_style.md:

Do not put links or references to issues or PRs (e.g. (issue #4089), #4207) in code or XML doc comments.

…but src/Paramore.Brighter/Observability/BrighterMetricsFromTracesProcessor.cs:78 ends the new comment with (#4086). That's the one source-comment issue-ref in the diff. Recommend dropping (#4086) to satisfy the rule you're introducing in the same PR.

2. PR description ↔ implementation mismatch for #4089

The description says the fix "added an internal lazy-cached Message.HeaderJson (snapshot on first access)". The actual implementation is different (and arguably cleaner): Message.cs only has a whitespace change; instead the serialized header is threaded through EnrichReceiveSpan's new string? return value into CreateSpan(Process, …)'s new optional serializedHeader parameter. Worth updating the description so future git-archaeology matches the code.

3. Public interface contract change — call out for back-compat 📌

IAmABrighterTracer changes:

• EnrichReceiveSpan return type void → string?
• new method PropagateConsumerContext(Message)

Adding a member to a public interface is a breaking change for any external implementer of IAmABrighterTracer (the in-tree test doubles were updated correctly). For a framework type this should at least be noted in the changelog/release notes; consider whether a default interface implementation for PropagateConsumerContext would soften the break.

4. Baggage propagation is now unconditional — confirm this is intended

Previously baggage propagation was gated: in the old EnrichReceiveSpan it ran only when span != null, and the CreateSpan(Process) copy ran only when an activity existed. The new PropagateConsumerContext is called once per message with no span / sampling / InstrumentationOptions guard, so baggage now propagates even when there is no receive span (sampled out, or instrumentation disabled). This is plausibly a correctness improvement, but it's a behaviour change slightly beyond a pure perf/leak fix — flagging it against the repo's "Change Scope" guidance so it's a conscious decision, ideally captured in the ADR.

5. Malformed correlation id now produces a per-message receive-error log

Because PropagateConsumerContext throws ArgumentException on a W3C-invalid correlation id (e.g. "bad=value") and it sits inside the receive try, each such message now hits the generic catch (Exception) → Log.ExceptionReceivingMessages and marks the receive span Error before still dispatching. When_A_Message_Has_A_Malformed_Correlation_Id_The_Pump_Continues correctly proves the pump survives (good — this is better than the previous leak), but worth being aware the change trades a leak for log noise on poisoned messages. Acceptable; just noting.

6. Should the bugfixes/ scratch artifacts be committed?

The diff commits bugfixes/.current-bug, bugfixes/*/.confirm-approved, .issue-number, and bugfix.md files, and there's no .gitignore entry for them. bugfixes/.current-bug in particular (0003-pump-span-leak-exception-paths) is ephemeral session state — it points at "the active bug" and will be stale the moment the next bug starts. Recommend either gitignoring the /bugfix workflow's transient state (.current-bug, .confirm-approved, .issue-number) or deciding deliberately to keep bugfix.md only as an audit trail.

Minor nits

• Test comments hard-code line numbers (Proactor.cs:190, Reactor.cs:149) which will rot; prefer describing the branch ("the message-is-null path").
• case "begin" uses a magic string like its siblings ("publish"/"receive"/"process") rather than MessagePumpSpanOperation.Begin.ToSpanName(). Consistent with existing style and guarded by a test, so fine — just note the silent coupling: if ToSpanName ever changes, the switch falls through to default and the Pump-lifetime span (MessagePumpSpanOperation.Begin) recorded into messaging.client.operation.duration histogram #4086 bug silently returns.
• The retained try/catch inside CreateSpan (Process) is now largely defensive — with baggage moved out, TraceStateString/Activity.Current assignment is unlikely to throw. No harm; just no longer the primary mechanism of the Pump observability: pumpSpan and processSpan can leak on exception paths #4090 fix (moving baggage out is).

Things I could not fully verify

I reviewed statically and did not run the suite locally; I'm trusting the stated 843 passed / 0 failed / 7 skipped on net10.0.

Nice work overall — the fixes are correct and the tests are the strong point. Addressing #1 (the self-inconsistent comment) and #2 (description) is quick; #3/#4/#6 are judgment calls worth a sentence in the ADR/changelog.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4207: message-pump observability fixes (4086, 4089, 4090)

Reviewed the runtime changes in BrighterMetricsFromTracesProcessor, BrighterTracer, IAmABrighterTracer, Reactor, Proactor, and Message, plus the new regression tests. Overall this is a high-quality, well-scoped set of fixes: each is diagnosis-first, minimal, and ships a regression test. The three core fixes are correct:

• 4086 — case "begin": break; ahead of the default arm is the minimal, correct fix. MessagePumpSpanOperation.Begin.ToSpanName() does map to "begin", and create/deposit/send/clear/archive/scheduler still fall through default. The added test pair (begin excluded / send still recorded) pins both sides.
• 4090 — wrapping the receive loop in try/finally { EndSpan(pumpSpan) } is correct and applied symmetrically in Reactor.Run and Proactor.EventLoop; the exception still propagates so shutdown semantics are preserved. Ending the activity inside CreateSpan when post-start enrichment throws is the right place for that fix.
• 4089 / decoupling — returning the serialized header from EnrichReceiveSpan and threading it into CreateSpan(Process) removes the double serialization while keeping Message observability-agnostic (the earlier Message.HeaderJson cache is gone). Moving baggage propagation into a single PropagateConsumerContext call is a clean separation.

Good test coverage: header-serialized-once, both-spans-share-one-header, malformed-correlation-id-pump-continues, and the null-message guard.

Issues / things to consider

1. Public breaking change on IAmABrighterTracer (most important). It is a public interface, so:

• Adding void PropagateConsumerContext(Message message) is a source- and binary-breaking change — any third-party / downstream implementation will no longer compile.
• Changing EnrichReceiveSpan from void to string? is also a breaking signature change for implementers.

These are reasonable designs, but they warrant a version bump per semver and a release note. If preserving the interface matters, consider a default interface method for PropagateConsumerContext, or document the break explicitly.

2. Null-message + tracing-enabled may still log the misleading NRE the last commit set out to remove. The guard added to PropagateConsumerContext fixes the no-listener case. But in the pump, EnrichReceiveSpan(receiveSpan, message, …) runs before PropagateConsumerContext, and when a listener is present receiveSpan is non-null, so EnrichReceiveSpan dereferences message.Header and throws an NRE first — caught and logged as ExceptionReceivingMessages ahead of the dedicated null-message path. So with tracing actually enabled, the misleading log can persist. The new When_Propagating_Consumer_Context_For_A_Null_Message test exercises the method directly, not the pump flow with a live listener, so it would not catch this. Pre-existing (not a regression here), but if the goal was to remove the misleading NRE, a null guard at the top of EnrichReceiveSpan (mirroring PropagateConsumerContext) plus a pump-level test with tracing on would close the gap.

3. Comment self-violates the guideline this PR introduces. The same PR adds a code_style.md rule "no issue/PR links in comments," yet the new begin comment in BrighterMetricsFromTracesProcessor.OnEnd ends with a (4086) issue reference. Recommend dropping it to match the rule you just landed.

4. Doc drift in EnrichReceiveSpan XML comment. The interface XML doc was correctly updated to "propagates the producer's tracestate onto the receive span," but the implementation summary in BrighterTracer.cs still reads "and propagates the producer's tracestate and baggage onto the consumer side." Baggage moved to PropagateConsumerContext, so this line is now inaccurate.

Behavioural note (looks intentional, flagging for awareness)
Because PropagateConsumerContext adds correlationId into message.Header.Baggage after EnrichReceiveSpan serializes the header, and the process span now reuses that same string, the process span's messaging.headers tag no longer carries the redundant correlationId-in-baggage copy the old code happened to include (old CreateSpan(Process) re-serialized after the baggage mutation). CorrelationId is still present as the top-level header field and the ConversationId tag, so this is fine — and it is called out in the commit message — but anyone parsing messaging.headers[...].Baggage.correlationId downstream would see the difference.

Minor

• The large reformatting in Reactor/Proactor (re-indent to wrap in try) makes the diff heavy; the per-message catch/continue semantics are preserved. I traced both pumps and the logic looks behaviour-preserving.

Nice work overall — the fixes are correct and well-tested. The main actionable items are (1) flagging/handling the IAmABrighterTracer break for semver, and the two small comment/doc cleanups (3, 4).

_{🤖 Automated review by Claude Code.}

[claude]

Review — message-pump observability fixes (#4086, #4089, #4090)

Solid, well-scoped set of fixes. Each comes with a regression test, the diagnosis-first approach shows in the code comments, and the big line-counts in Reactor/Proactor are almost entirely re-indentation from wrapping the receive loop in try { … } finally { EndSpan(pumpSpan) } — not new logic. Overall this looks correct and ready, with a few things worth addressing before merge.

🔴 PR description no longer matches the code (#4089)

The description says the fix adds "an internal lazy-cached Message.HeaderJson" and "No public API change." The actual implementation is different:

• EnrichReceiveSpan return type changed from void → string? and the serialized header is threaded through to CreateSpan via a new serializedHeader parameter.
• A brand-new method PropagateConsumerContext(Message) was added.
• Message.cs only contains a whitespace change — there is no HeaderJson member.

Please update the description so reviewers and the changelog reflect what actually shipped. The threaded-parameter approach is fine (the Assert.Same test nicely proves single-serialization), but the stated "no public API change" is incorrect — see below.

🟠 This is a breaking change to the public IAmABrighterTracer interface

All three signature changes land on the public IAmABrighterTracer:

• EnrichReceiveSpan return type void → string?
• new optional serializedHeader param on CreateSpan
• new member PropagateConsumerContext

Adding a member to a public interface is source- and binary-breaking for anyone implementing it (custom tracers, test doubles). For a widely-consumed library this warrants either a default interface implementation for the new method, or an explicit callout in the release notes / a semver decision. At minimum it shouldn't be described as "no public API change."

🟠 Behavioural shift in baggage propagation gating (worth a test)

Previously baggage propagation (Baggage.Add("correlationId", …) + Baggage.SetBaggage) ran unconditionally inside CreateSpan(Process) whenever the process activity existed (and again inside EnrichReceiveSpan). Now it lives in PropagateConsumerContext, called from the pump only when receiveSpan is not null.

In the edge case where the receive span is sampled out but the process span is not (possible under a custom sampler that treats Consumer vs Internal span kinds differently), baggage is now skipped where it previously propagated. The inline comment claims this "mirrors the historic span-scoped propagation," which is true for the common case, but the receive-null/process-non-null case is a genuine semantic change. A targeted test for that combination would lock the intended behaviour down.

🟢 #4086 — clean

case "begin": break; short-circuits only the pump-lifetime span, leaves every other operation recording, and the comment explains why. String-literal cases are consistent with the surrounding "publish"/"receive"/"process" arms. Good.

🟢 #4090 — correct and symmetric

The try/finally { EndSpan(pumpSpan) } preserves exception propagation (shutdown semantics intact) and is mirrored in both Reactor and Proactor. The CreateSpan post-start try/catch that ends + rethrows on enrichment failure correctly prevents the orphaned-activity leak. The catch (Exception) is broad but the guarded statements (TraceStateString, Activity.Current) rarely throw, so it's acceptable.

Minor

• Workflow artifacts committed to the repo: bugfixes/000{1,2,3}-*/ (incl. .confirm-approved, .issue-number, .current-bug) are /bugfix scratch state. Unless intentional, these add repo clutter and probably belong in .gitignore rather than in the tree.
• EnrichReceiveSpan vs null message: PropagateConsumerContext guards if (message is null) return;, but EnrichReceiveSpan — called on the line above with the same possibly-null message before the message is null check — does not, and would NRE on message.Header if a span exists and the broker returned null. This is pre-existing (same ordering before this PR), so not a regression, but guarding in one method and not the other reads as slightly inconsistent.

Tests

Coverage is good: per-fix regression tests, both pumps covered, null-message and malformed-correlation-id paths exercised, and Assert.Same is a strong assertion for the single-serialization guarantee. Consider adding the sampled-out-receive / sampled-in-process baggage case noted above.

🤖 Generated with Claude Code

[codescene-delta-analysis]

Gates Failed
Enforce advisory code health rules (3 files with Complex Method)

Our agent can fix these. Install it.

Gates Passed
3 Quality Gates Passed

Reason for failure

Enforce advisory code health rules	Violations	Code Health Impact
BrighterMetricsFromTracesProcessor.cs	1 advisory rule	9.66 → 9.64	Suppress
Proactor.cs	1 advisory rule	7.79 → 7.77	Suppress
Reactor.cs	1 advisory rule	7.79 → 7.77	Suppress

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 — message-pump observability fixes (#4086, #4089, #4090)

Thanks for the diagnosis-first workup on all three — the /bugfix trail (triage → confirm → test-first → fix → verify) makes each change easy to follow, and every fix ships regression tests. Overall this looks solid; the #4086 fix in particular is clean and correctly guards against over-suppression with the send-still-records test. A few things worth addressing before merge.

🟠 The case "begin" comment violates the code-style rule this PR adds

.agent_instructions/code_style.md gains (in this PR):

Do not put links or references to issues or PRs (e.g. (issue #4089), #4207) in code or XML doc comments… Keep comments concise. Prefer one tight line over a paragraph.

But BrighterMetricsFromTracesProcessor.cs:74-78 then adds a six-line paragraph ending in (#4086):

case "begin":
    // The message-pump "begin" span lives for the entire pump lifetime
    // (opened at pump start, closed at shutdown), so recording its duration
    // as a client operation pollutes the messaging.client.operation.duration
    // histogram with a multi-hour outlier on every shutdown. It is still
    // emitted as a trace span; it just must not feed the client metric (#4086).
    break;

Please trim to a single line and drop the (#4086) so the code matches the rule introduced alongside it, e.g. // begin spans live for the whole pump lifetime — recording their duration would swamp the client-operation histogram.

🟠 Public API breaking changes on IAmABrighterTracer

The #4089/#4090 fixes reshape the public IAmABrighterTracer interface:

• EnrichReceiveSpan return type changes void → string?
• CreateSpan(...) gains a string? serializedHeader = null parameter
• a new method PropagateConsumerContext(Message) is added

Adding a member (and changing a signature) on a public interface is a source-breaking change for any external implementer of IAmABrighterTracer (custom tracers / test doubles in downstream code). For fixes framed as minimal internal observability corrections, this is a larger surface change than advertised. Worth confirming it's acceptable under Brighter's compatibility policy and calling it out in the release notes. If a break is undesirable, an alternative is threading the serialized header without altering the interface (e.g. the Message-level cache the bugfix write-up originally described).

🟠 #4090 processSpan fix diverges from its own write-up and guards near-unthrowable code

The committed bugfixes/0003-.../bugfix.md says the processSpan fix wraps the post-start work — TraceStateString, baggage add/set, Activity.Current — in a try/finally inside CreateSpan, and cites a regression test When_Create_Span_Throws_After_Starting_Close_The_Span.cs. The shipped code does something different:

• baggage propagation was moved out of CreateSpan into the new PropagateConsumerContext, so CreateSpan's new try/catch (BrighterTracer.cs:214-228) now wraps only activity.TraceStateString = traceState and Activity.Current = activity — setters that don't realistically throw. The catch arm is close to dead defensive code.
• the genuinely-throwing operation (malformed-correlationId Baggage.Add) now lives in PropagateConsumerContext, which is not itself wrapped; it relies on the pump's receive try/finally for cleanup. Fine for leak-safety, but the documented root-cause throw is no longer covered by the CreateSpan guard.
• the cited When_Create_Span_Throws_After_Starting... test isn't in the PR (replaced by When_A_Message_Has_A_Malformed_Correlation_Id_The_Pump_Continues / When_Creating_A_Process_Span_Baggage_Propagation_Is_Decoupled).

Net behavior is leak-safe (verified: pumpSpan wrapped in try/finally in both Reactor.Run and Proactor.EventLoop; receiveSpan ended in its finally), so this isn't a blocker — but please reconcile the committed bugfix.md with what actually shipped, and consider whether the now-inert CreateSpan try/catch is still worth keeping.

🟡 #4089 changes process-span header content (behavioral)

Because EnrichReceiveSpan now snapshots the serialized header before PropagateConsumerContext lifts correlationId into Header.Baggage, and the process span reuses that snapshot, the process span's MessageHeaders tag no longer contains the correlationId baggage entry it previously carried. The 0002 write-up acknowledges this, and no test asserts process-span header content, so it's low-risk — but it's an observable change in emitted span data and belongs in the release notes.

🟡 Baggage propagation is now gated on trace sampling

PropagateConsumerContext is only called if (receiveSpan is not null) (Reactor.cs:126-127, Proactor.cs:167-168), i.e. only when tracing is sampled/enabled. So OpenTelemetry baggage — a context-propagation concern that arguably shouldn't depend on trace sampling — stops flowing to handlers when tracing is off. The doc comment notes this "mirrors the historic span-scoped propagation," which is accurate (the old CreateSpan path was also gated on an activity existing), so it's not a regression. Flagging it as a deliberate design point worth a decision rather than an accident.

🟡 PropagateConsumerContext(Message message) nullability mismatch

The parameter is declared non-nullable Message, but the body guards if (message is null) return; and the pumps can pass a null message (broker returned null while a receive span exists). Declare it Message? so the annotation matches the contract it documents.

🟡 Committed workflow scratch state

bugfixes/.current-bug, the per-bug .confirm-approved / .issue-number markers, and the bugfix.md files are committed into the repo. The marker files look like ephemeral /bugfix workflow state (.current-bug points at 0003-...) rather than durable artifacts — consider .gitignore-ing those. And as noted above, the bugfix.md files describe an implementation (Message.HeaderJson lazy property) that isn't what shipped (serializedHeader threading), so if kept they should be updated to match.

Nits / pre-existing

• EnrichReceiveSpan still serializes the header for MT_NONE (empty-queue poll) and MT_QUIT, which are never serviceable — pre-existing, low impact, not introduced here.

Test coverage

Good — Reactor + Proactor parity tests for the pump-span leak, the header-serialized-once test, the over-suppression guard for #4086, and the malformed-correlation-id / baggage-decoupling cases. Nicely covered.

---

🤖 Automated review. I read the diff and the changed source in BrighterMetricsFromTracesProcessor.cs, BrighterTracer.cs, Reactor.cs, Proactor.cs, and IAmABrighterTracer.cs; verify locally before acting on anything above.