Repository navigation
feat(core) #197: carry the caller's MDC into the threads that run your function - #205
Merged
Merged
Conversation
…r function A caller who had established diagnostic context - a trace_id, a request_id, a tenant - lost all of it the moment a record crossed into the worker pool, and again crossing into vert.x, Reactor or Mutiny. The logs their user function wrote could not be correlated back to the originating request. PC set only its own two keys (pcId, offset) and called neither MDC.getCopyOfContextMap() nor MDC.setContextMap() anywhere in the tree. The context is snapshotted on the thread that calls poll*() - the only moment it is reachable, since none of PC's threads exist yet and the SLF4J MDC is not inheritable - and re-established around the user function on the thread that actually runs it, then torn down again. The teardown is the half that matters. Worker threads are pooled: one that keeps the previous task's trace_id produces logs that are actively misleading, which is worse than no context at all. That leak already existed on master for the user function's own MDC.put calls - five records on one pooled thread carry four poisoned keys - and is fixed by the same scope. Precedence: PC's own pcId/offset are applied AFTER the caller's map at every call site, so a caller key of the same name cannot shadow them. PC's own log lines are read by those keys. On by default, with ParallelConsumerOptions.propagateMdc as the kill switch. Not propagating fails silently for everyone; propagating fails visibly, as one extra key in a log line, and can be switched off. Setting it false restores the old behaviour exactly, leak included, so it is a true revert rather than half of one. PC logs the captured keys - never the values - at INFO, so that a request-scoped value accidentally pinned for the life of the consumer is discoverable rather than silently wrong. Cost on the hot path is nil for anyone who never touches the MDC: getCopyOfContextMap() returns null, allocating nothing, for an empty context, and pcId is only ever set when setMyId() has been used. Deliberately not covered: Reactor's and Mutiny's own operator chains. We carry the context into the user's function and into PC's terminal signal handling; following it through the Publisher/Uni they return needs Reactor's own io.micrometer:context-propagation and is the user's call, not ours. Upstream-Issue: confluentinc#907 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Claude is reviewing this PR
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
|
…ntry Lifecycle transition per AGENTS.md - the manifest is the source of truth for the fork side, and nothing automated checks it, so an entry saying prs: [] with an open PR rots silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 9m 8s —— View job Claude's review of #205Tasks
SummaryNo issues found. This is a careful, well-scoped change and the PR body's claims check out under independent verification:
One minor accuracy nit (non-blocking)The PR checklist says "13 new tests across 4 modules." Actual count is 17: |
|
… tests The file-similarity report flagged the three new engine tests as newly introduced duplication (Mutiny<->Reactor 69%, Mutiny<->vertx 55%, Reactor<->vertx 53%). The identical part - the caller key/value, the two observation queues, the MDC teardown, and the "context was visible" assertion - now lives once in MdcBoundaryProbe, in the core test-jar, the same route TestConventionRules and WireMockUtils already take. Used by composition, not a shared base class: each engine test already extends its own module's unit-test base, so inheritance is not available. The per-engine assertions deliberately stay in the tests, because they are what stops a test quietly degrading into a re-run of the core worker-pool case, and they genuinely differ: Reactor asserts its scheduler thread positively (boundedElastic), while vertx and Mutiny can only assert "not a pc- thread" since their executor is caller-supplied. Nothing was weakened - the probe's failure messages now also report the threads and values seen, which the previous assertions did not. Re-verified all three can still go red: temporarily overriding initAsyncConsumer with propagateMdc(false) fails each on its own "context must be visible on the <engine>" assertion with `values seen: [null, ...]`. Also: - the escape-hatch test's javadoc read as though propagation were off by default; say plainly that it is on - record the measured allocation cost in the inflight note: 0 bytes per batch when the caller never touches the MDC (measured with ThreadMXBean.getThreadAllocatedBytes, not assumed), ~424 bytes when they do, with the caveat that the zero is a logback MDCAdapter property - note that the startup INFO line is structurally once-per-instance, since supervisorLoop throws if poll*() is called twice Refs #197, #195, upstream confluentinc#907. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Owner
Author
|
Responding to the review in #205 (comment) and to the two automated duplication reports. Fixed in 544347b. 1. "13 new tests" was wrong — you were right, it is 17. FixedCounted independently rather than taking the number on faith, and the reviewer's count is exactly right:
Confirmed by surefire, not just by grep — core reports 2. Startup INFO line — checked proactively, it is once per instanceWorth stating explicitly since #203 is specifically about log noise, and an INFO line on a hot path would be a genuine regression. It is not on one, and structurally cannot be: if (state != State.UNUSED) {
throw new IllegalStateException(msg("Invalid state - you cannot call the poll* or pollAndProduce* methods more than once ..."));
}so it runs once per consumer, at startup, before any PC thread exists. Not per poll, not per record. Now recorded in the inflight note so it stays that way. 3. Hot-path cost — measured rather than reasoned aboutThe PR claimed "nil for users who never touch the MDC" on the strength of an argument. Measured it instead, with
So the claim holds exactly: zero bytes, not merely "small". One honest caveat I have added to the PR body and the inflight note: that zero is a logback 4. File-similarity report — refactoredThe similarity check flagged the three new engine tests as newly introduced duplication (Mutiny↔Reactor 69.0%, Mutiny↔vertx 55.2%, Reactor↔vertx 53.1%). That is duplication this PR introduced, so it is in scope and is now removed rather than argued away. The identical part — caller key/value, the two observation queues, the MDC teardown, and the "context was visible" assertion — now lives once in What deliberately did not move: the per-engine assertions. Those are precisely what stops a test quietly degrading into a re-run of the core worker-pool case, and they genuinely differ — Reactor asserts its scheduler thread positively ( Nothing was weakened; the assertions got slightly stronger. The probe's failure messages now report the threads and the values actually seen, which the previous hand-rolled assertions did not. And all three were re-verified to still go red after the extraction — temporarily overriding A refactor that silently defused the tests would have been worse than the duplication. 5. Duplicate-code report, jscpd's "+1 new clone" — pushing back, no changejscpd flagged an 8-line clone at Worth noting the cross-check engine disagrees in the right direction: PMD CPD reports −1 clone and −28 duplicated lines for this PR, and overall duplication falls on both engines (CPD 2.51% → 2.43%, jscpd 3.26% → 3.20%). Both checks pass. 6. One thing for the maintainer, not for me —
|
|
Claude finished @astubbs's task in 8m 37s —— View job Claude's review of #205 (re-review after commit 544347b)Tasks
SummaryNo issues found. This is the follow-up commit (
Did not re-run the destructive "flip No other findings. The response to the first round (test-count fix, probe extraction, measured-cost table, once-per-instance clarification) addresses everything raised, and nothing new turned up on a fresh pass. |
Owner
Author
|
Closing the loop on 544347b — all 25 checks green, and the re-review came back clean ("No issues found", having independently re-run all 17 tests and hand-traced the MDC precedence order). The probe extraction worked, measurablyThe similarity tool re-ran against the new commit. Before → after, on the three files it had flagged as newly introduced duplication:
One new entry appears in its place — PMD CPD remains at −1 clone / −28 duplicated lines vs base. Both engines pass. Why the
|
Merged
4 tasks done
…opagation # Conflicts: # src/docs/development/upstream-map.yaml
…ew schema Master's mirror landed while this branch was open, and it changes two things under this PR's feet. Issue references must now name their repo below #1000 (#197 / confluentinc#907), enforced on added lines by .github/scripts/issue-ref-gate.js. Three of this branch's added lines were bare - two in the inflight note, one in MdcContextPropagationTest's javadoc - so the PR Checklist gate would have failed on merge. Verified by running the gate's own suspectRefs() over this PR's diff: three hits before, zero after. Also dropped "upstream confluentinc#907" for "confluentinc#907". The bare form still passes the gate, but AGENTS.md now asks new writing to name the owner rather than the role, since this fork is itself upstream to anyone who forks it. upstream-map.yaml was restructured by the mirror commit: it tracks upstream PRs only, entries moved to block style, and fork work now cross-links its mirror via fork_issue. Refitted this entry to match and renamed it issue-907-mdc-propagation -> mdc-context-propagation, since the "issue-" prefix now reads as one of the tracking-only entries that commit deleted. Safe to rename: nothing outside this branch referenced it. fork_issue: 195 is the mirror of confluentinc#907. Kept the entry rather than deleting it. AGENTS.md is in slight tension here - the manifest "tracks upstream PRs only" (this maps to an issue, not a PR), but it also says to record status: pr-open for your own fork work at each lifecycle transition. fork_issue: 195 satisfies both: the upstream issue's diagnosis and closing live on the mirror, the fork work's lifecycle lives here. upstream-map.py validate: OK, 15 entries. Refs #197, confluentinc#907. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
… two Master's 16ac63b fixed MultiInstanceMetricsTest for awaiting a counter that LEADS the value being asserted, and generalised the rule: await the value you are about to assert, never a proxy that leads it. MdcBoundaryProbe, added earlier in this PR, had the same shape. observeCurrentThread() appended to threadsUsed and then to contextSeen, and observations() returned contextSeen. So the engine tests awaited contextSeen reaching 3 and then asserted over threadsUsed - a collection that is always one step ahead. A fourth invocation arriving concurrently could have contributed its thread name but not yet its context value, and assertObservedOnlyOn would have inspected four entries having awaited three. It could not actually lose today, because each engine test primes exactly three records, so no fourth invocation exists. That is a property of the fixture, not of the probe - exactly the "all four sites had the pattern, only one had lost the race so far" situation 16ac63b describes. Now one Observation (thread + value seen) is added atomically to one collection, so awaiting observations() bounds everything the assertions read. This is the shape MdcContextPropagationTest already used for its own @value Observation; the probe should not have diverged from it when the engine bookkeeping was extracted. Nothing loosened - the assertions are unchanged in strength and still name the threads and values seen on failure. Re-verified both Reactor and Mutiny still go RED under propagateMdc(false) after the change, with `values seen: [null, null, null]`; all 17 tests green. Refs #197. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 5m 49s —— View job Claude's review of #205 (re-review after commit
|
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
astubbs
marked this pull request as draft
August 19, 2026 03:11
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…-up is not a new mode Two corrections from the owner. The missing instance identifier needs no new mechanism and is not what #205 provides: MDC_INSTANCE_ID, the control thread's addInstanceMDC, the poller's own put and the logback pattern all exist already. It renders blank because myId defaults to empty - built as a manual aid for testing several instances and never defaulted. Defaulting it to the UUID PCMetrics already generates would make a log line and a metric name the same instance for free. #205 carries the caller's context into the worker threads, the opposite direction. And catch-up does not license saturating the downstream, which collapses it from a fourth mode into the standing objective with the latency ceiling suspended - the same equilibrium search, one suspension, no new controller. The distinction that survives: the knee and the throughput plateau are different points, normal running probably wants the knee and catch-up the plateau, and the plateau self-limits because a saturated dependency stops paying for more admission.
The branch was already renamed to bz.stub.* by the grand sweep, so this merge takes the order AGENTS.md and bin/rename-packages.sh require - renamed branch meeting renamed master - and git had a rename to pair against on each side. 54 conflicts, split by whether this PR contributes content to the file. Five do. Four are the MdcPropagation import landing in a block master also edited, and keep both sides. The fifth is a real semantic merge in AbstractParallelEoSStreamProcessor#submitWorkToPoolInner: master wrapped the pool submit in a RejectedExecutionException catch that drops the batch when the pool is shut down and rethrows when a live pool rejects, while this branch wrapped the submitted lambda in an MDC scope. They compose - the capture stays on the controller thread ahead of the try, the try/catch is master's verbatim, and the MDC scope stays inside the lambda - so the rejection contract and the context propagation both survive. The other 49 are files this branch only renamed, or tooling and test logging config it took from an older master. Each takes master's version, or accepts master's deletion. The set was derived by diffing the branch's content commits against the merge base and mapping through the rename commit's own rename map, not by eye. README.adoc and docs/todo-index.md are regenerated rather than merged. bin/check-copyright-headers.sh reports 0 violations across the result.
check-inflight-tags.sh and check-branch-self-reference.sh both arrived from master in the merge, and both fail on work this branch owns rather than on anything master left behind. The MDC note gains `inflight-type: feature` and `inflight-impact: blind-spot` - the consequence it addresses is that a caller's trace_id was invisible in the logs their own function wrote, which is a missing instrument rather than a lying one. It also gains `post-merge: exempt-file`, the marker the gate prescribes for a note that IS the note for the PR it names: it discusses its own branch throughout by design, and it is deleted when that PR lands. The rename sweep's 38-branch worklist listed this branch as sweep work still to schedule, which stops being true the moment this merges - the branch was brought across by hand, ahead of the sweep. Its row is removed and the reason recorded below the table, inside a checked block, so a later agent reading the worklist does not go looking for a job that is already done. Recorded rather than silently deleted, because a row that just vanishes reads as an oversight.
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
5 tasks done
…s own lines
The master merge brought `-Xlint:all` and SpotBugs-over-test-code with it, and
both then fired on lines this branch wrote. `bin/check-pr-analysis-surfaces.sh`
separates those from the inherited ones and exited non-zero on exactly three.
AGENTS.md gives two ways out - fix it, or say in the PR why not - and all three
turned out to be fix.
`MdcBoundaryProbe.observations()` returned `java.util.Collection`
(IMC_IMMATURE_CLASS_COLLECTION_RETURN). The field is a `ConcurrentLinkedQueue`,
so the declared type was strictly less informative than the thing behind it. It
now returns `Queue`, which is both what it is and what callers want to know: the
javadoc says a test awaits this to bound what the assertions read, and `Queue`
says the collection is being appended to concurrently in a way `Collection` does
not. Every caller reaches it through Truth's iterable subject, so nothing else
moves.
The other two were PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS on `MDC.get(String)` -
the detector seeing the same key read twice with the same constant argument and
taking the second read for a cacheable repeat of the first. It is not: the scope
under test is precisely what changes the thread's context between the two reads,
so caching would delete the assertion rather than tidy it.
That makes the rule wrong HERE while remaining right in general, and the rule
registry lists PRMC among the ones kept on. So the fix is not a suppression: all
MDC reads in the class now go through one `assertMdc(key, expected)` helper, and
each test method calls it with a different expectation instead of calling
`MDC.get` twice. The detector is satisfied because the repeat is gone, not
because it was told to look away.
It pays for itself independently of the analyser. `assertThat(MDC.get(k))` fails
with "expected abc, but was null" and never says which key it meant; the helper
uses `assertWithMessage("MDC[%s]", key)`, so the failure names it.
Verified rather than assumed, in both directions. SpotBugs run locally over the
core module before and after: the three findings on these files are present
before and absent after. And the restructure was given a negative control -
breaking one expectation on purpose fails the test, at the right call site, with
the key in the message - because a test edit that cannot be shown to still fail
is a weakened test.
Not fixed, and deliberately: the `[try] auto-closeable resource scope is never
referenced` warnings on the try-with-resources blocks in `MdcPropagationTest`.
They are real javac output, but they reach the build log only - they are not in
the check-run annotations, so the analysis-surface gate does not report them and
the fix-or-explain rule does not bite. The variable is unused because the scope
is the subject of the test rather than something it talks to, which is the shape
`-Xlint:try` exists to flag and the one case where it is not telling you
anything.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…to a stale branch (#372) A remote-tracking ref is a cache, and a stale one does not error - it answers. `origin/master` being fresh says nothing about `origin/<your-branch>`, which in a repo where several agents run at once moves without you: another session, another machine, or a sweep that touches every open branch in one pass. MEASURED, ON 2026-08-26. A session picked up #205, fetched `origin/master`, and merged it. It never fetched the branch's own ref, two weeks stale since the package-rename sweep had pushed to it - so it re-did the rename from scratch, resolved the resulting conflicts on top of the duplicate, and found out at `git push`, which rejected the non-fast-forward. The recovery was to discard all of it and rebuild on the published tip. Every command in between reported success, which is the whole problem: nothing was looking. `.claude/hooks/check-branch-behind-its-own-remote.sh` has two arms against that one failure. At `SessionStart` it runs `git fetch --all --prune`, throttled on a stamp file, so every ref the session goes on to read is real. On `git merge` or `git rebase` it refuses while `origin/<branch>` holds commits the checkout does not, and names them. It is the mirror image of `remind-master-drift-on-push.sh`: that one watches the base moving under you, this one watches your own branch. WHY IT DENIES WHERE THE PUSH HOOKS ONLY REPORT. `docs/agent-harness.md` already draws the line - a situation with no wrong answer gets `additionalContext`, a violation gets a `deny`. Merging onto a ref known to be behind its published tip is not a judgement call: the result cannot be pushed without discarding somebody else's commits, so there is no outcome the agent wanted. WHAT IT MUST NEVER DENY, because a guard that blocks its own remedy gets switched off. Merging or rebasing onto the branch's own tip - `origin/<branch>`, `@{upstream}`, `FETCH_HEAD` - is the reconciliation the deny message asks for. And `--abort`, `--continue`, `--skip`, `--quit` are the way out of a conflicted tree; trapping an agent between a rebase it cannot finish and one it cannot abandon is worse than any staleness. Both were denied by the first version, and the first of them meant that on `master`, where the branch's own counterpart IS `origin/master`, the commonest command in this repo was refused every time master advanced. `BRANCH_FRESHNESS_OVERRIDE=1` is the escape hatch, honoured both as an exported variable and as a TOKEN of the parsed command - never as a raw-payload substring, which any prose naming the variable could satisfy, including the agent-written `description` field, on a guard whose own message teaches that exact string. `git push` is deliberately not an arm: git refuses that case itself, legibly, and that refusal is what ended the incident. Three helpers move into `lib/hook-common.sh` rather than being copied. The tokeniser now emits each git invocation's ARGUMENTS as well as its subcommand, because the exemptions above need the flags and refs it was discarding and re-tokenising in the caller is what that file exists to prevent; `hook_git_runs_any` answers "merge or rebase" in one interpreter spawn instead of two; and `hook_throttle_expired` owns the now/mtime/shape-guard/compare triad this was becoming the third copy of. FOUR REVIEW PASSES, AND THEY EARNED THEIR KEEP. Between them they found that the guard denied its own remedy, trapped a conflicted rebase, took a substring for its override, went silently dead on a broken interpreter, keyed its throttle stamp so that every clone on the machine shared one - so one clone's session start suppressed another's fetch, leaving it reading exactly the stale refs this exists to refresh - cost several times its siblings on a path that runs on every Bash call, and let a redirect target spoof an exemption because the argument walk stopped at command separators while shlex also emits `>` as a token. All are fixed here; `-s ours` defeating the remedy exemption is recorded in the header's imprecision list instead, since an agent does not reach for it unprompted. The self-test drives both arms, including the `SessionStart` half that produces no output and is therefore the part most able to stop working invisibly: it asserts the tracking ref actually moves, and that a second start inside the throttle does not fetch. Every ALLOW case is paired with a DENY proving the same path can still fire, because an allow-only suite is passed perfectly by a hook that does nothing - which is precisely the shape this one would fail into, since every guard in it exits silent when it cannot answer. The regression cases were verified as true negative controls: reverting a fix alone, fixtures left in place, turns them red. One fix caught another. The interpreter guard was first written as `command -v python3`, and the new self-test case defeated it immediately - that is an existence test, and an interpreter present on PATH but exiting non-zero passes it while the guard goes quietly dead. It now runs `python3 -c ''`, the same probe-do-not-assume reasoning `hook-common.sh` already applies to `stat`. The same existence-not-works form remains in the other hooks and in `bin/check-pr-analysis-surfaces.sh`, with the identical class already written up for `node` elsewhere in `bin/`; that sweep is deliberately left as a separate decision rather than smuggled in here. `AGENTS.md` gains the rule in one line plus its enforcer's name, because Codex and anything else that reads it get no hooks at all. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, and it is a real one: master and this branch both changed how the Reactor subscription terminates, for unrelated reasons that have to compose. Master's `fix(reactor) #342` replaced the two-argument `Flux.subscribe(onNext, onError)` with the three-argument form. The two-argument overload has no completion callback, so `onComplete` - the method that retires the record - was wired as the ON NEXT consumer: a Publisher emitting several elements retired its record once per element, and one completing empty (`Mono.empty()`, or a user function returning null) retired it never, leaking the record's in-flight accounting and, since the dispatch ceiling landed, its dispatch permit for the life of the process. This branch wrapped both of those callbacks in an MDC scope, because they run on the Reactor scheduler rather than the worker thread that captured the caller's context. The resolution keeps both: master's three-argument shape, with each TERMINAL callback body inside the scope. Dropping either side is clearly wrong - master's half prevents a wedged engine, this branch's half is the feature - so this is a composition rather than a choice between them. One judgement call inside it: the element consumer is left unwrapped. It is neither the user's function nor terminal handling, which is the boundary this propagation covers and says so on the wrapper above it, and the `doOnNext` trace in the same chain is unwrapped for the same reason. Recorded in the code next to the decision rather than only here. Each side keeps its own pin, and both are green after the merge: `ReactorPCTest#anEmptyPublisherStillRetiresTheRecord`, which master's commit message records as red against the two-argument subscribe, and `ReactorMdcPropagationTest`. A merge that satisfied only one of them would not have shown up as a conflict.
…efore its log expires `ChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedger` was killed fail-fast by the probe on this branch's CI, with the same `ZOMBIE_MEMBER/REBALANCE_BLOCKED` autopsy as the fourth, fifth and eighth entries in the family ledger - a group dwelling the full 15s bound in PreparingRebalance with a member not answering the protocol, and the whole assignment frozen for well over two minutes rather than a single shard. Recorded now because the ledger's own closing argument applies exactly: the sighting is easy to lose. The red belongs to a head that will not be the tip for long, the log expires, and the seed goes with it - which is how the entry above this one describes #345 losing its capture entirely. THE REASON THIS ONE IS NOT JUST ANOTHER TALLY MARK. Every previous capture of this arm predates the dispatch ceiling, which landed on master hours before this run. The commit immediately after the ceiling says in its own body that a leaked dispatch permit now takes the engine to a full stop rather than a stuck record, and a member that has stopped answering the rebalance protocol is what that would look like from outside. So the old intermittency and a new cause are both consistent with this autopsy, and the probe output cannot separate them. The entry says what would settle it, since nothing here does: replay the recorded seed at the ceiling commit and at its parent. Same seed, different position, which is the control this file keeps asking for and rarely gets. No test is quarantined and no assertion is touched. This commit adds a record and nothing else - the failing test is left failing, because a flake that fails the build is the deliberate design and the lever for changing that is quarantine with evidence, not a retry.
…what this file already settled The seventeenth sighting was written from the autopsy and a grep for the signature, without reading the entries it was joining. It therefore offered the dispatch ceiling as a co-equal explanation - "equally consistent, and the two are not distinguishable" - against a finding this file has built over several entries and which is far better evidenced than one capture. What it walked past: the eleventh sighting concluded the reds are seed-dependent rather than branch-dependent; the fourteenth caught three of them on a branch whose whole diff was markdown, a comment and one `iterations` value, with no `src/main` file anywhere, and pointed at the box or at master; the runner-contention hypothesis was ruled out there on timing rather than left unreplayed; and the pair of runs one commit apart at the end of the file states outright that the trigger is the schedule, not the tree. A capture on a branch that had just merged master is what all of that predicts. The ceiling is still worth naming, because a variable genuinely changed underneath and every previous capture of this arm predates it. But it is a question to rule out the way contention was ruled out - explicitly, with evidence - not a rival diagnosis. The entry now says so, and says the control is the same seed replay every entry here asks for, in the same-seed-different-position form that separates the two. Also corrected: the entry implied the red mattered to the PR. It does not. `Chaos Pain Suite` is not in master's required-checks ruleset, which the fourteenth sighting already records, so the capture gates nothing. It is written down because the log expires and the seed is the asset. The lesson is the one this repo already has a rule for and I did not follow: prior art is not a grep for the symptom. Grepping the signature found the matching entries; only reading them found the conclusions those entries had already reached.
One conflict, in `docs/inflight/bug-857-family.md`, and it is an add/add: master and this branch each appended a new entry immediately above `## Delete when`. Both are kept - they are independent captures of different arms. Order matters more than it looks. The `## 2026-08-26, Nth capture` sections are one continuous narrative about the BLOCKED-on-monitor discriminator, and this branch's sighting - a `ZOMBIE_MEMBER` capture - had landed between the third and fourth of them, splitting the series. Master's fourth capture now follows the third, and the sighting sits after it, which is also the correct recording order. One line of the sighting is corrected by what arrived. It cited the fourteenth sighting's "points at the box or at master" as a live explanation; master's fourth capture closes the ambient half of that outright - `Chaos Pain Suite` now runs one job per VM, so co-residency between two chaos runs is structurally impossible. The sighting now says so and cites the capture above it rather than resting on a superseded reading. Also inherited and checked rather than assumed: master's `fix(core) #322` rejects a `PCModule` getter that memoises a collaborator while taking that collaborator's owner as a parameter, and says it swept the class for the same shape. `mdcPropagation()`, added by this branch after that sweep, takes no owner parameter - it memoises off `options()`, the same shape as `pcMetrics()` - so it is outside the defect class rather than an instance of it the sweep could not have seen. Nothing else conflicted. Compile clean, and the tests that pin both sides of the earlier Reactor composition still pass: `ReactorPCTest` including `anEmptyPublisherStillRetiresTheRecord`, and the MDC suites across core, vert.x, Reactor and Mutiny.
…dings land where nothing can read them Capture only - no implementation, deliberately. #371 fixed the PIT lane and #344 was the first PR whose core changes it could actually score: 34 mutants, 30 killed, 88% test strength, four survivors. Recovering which four cost a ~25-minute local re-run and a hand-written mutations.xml parser, because the survivor table is written only to $GITHUB_STEP_SUMMARY and the REST API does not expose it. Two notes: ci-mutation-survivor-list-is-unreadable.md - where the table is lost (the script builds it complete, then emits it through summary() and nowhere else), and three fixes cheapest-first: echo to stdout, upload mutations.xml as an artifact, emit annotations on the diff. Recommends the first two together and deliberately holds back the third, because annotations change what every reviewer sees on a lane that is non-gating on purpose. bin/check-pr-analysis- surfaces.sh already lists this table under "surfaces this script cannot read", so the gap was documented before the lane produced anything worth reading. test-partitionstate-mutation-survivors.md - the findings themselves, with the command to re-derive them rather than a tally to trust. The survivor on #344's own new code is the conditional boundary in getIncompleteOffsetsBelow, and the note argues it is an EQUIVALENT mutant rather than a missing test: the bound is a sample of offsetHighestSucceeded and a succeeded offset is by construction absent from incompleteOffsets, so `<` and `<=` cannot differ while that invariant holds. The useful response is to pin the invariant if anything, not the boundary - writing a test to kill an equivalent mutant is how a suite acquires assertions describing the implementation instead of the contract. The wider sweep's real gaps are elsewhere and predate #344: metrics teardown is entirely unasserted, the back-pressure ratio arithmetic is unpinned, and several state predicates survive being replaced with a constant. That is a coherent piece of work and a better use of the lane than chasing individual survivors. Why this matters beyond convenience: a survivor is not self-evidently a test gap, and telling a real gap from an equivalent mutant is the whole judgement the lane exists to support. A reader who cannot retrieve the list cannot make it, so the lane degrades to a percentage that says nothing about what the missing percentage is.
astubbs
marked this pull request as ready for review
August 26, 2026 23:54
Master carried three things addressed at exactly this merge, and two of them overturn edits made on this branch. Taking the inherited decisions, and recording what is being overridden so the next reader does not take it for an oversight. THE SWEEP ROSTER IS A RECORD, NOT A WORKLIST. This branch had deleted its own row from the package-rename roster and written a paragraph explaining the blank, because the self-reference gate flagged the row. Master's `docs #120` settles it the other way: the roster is the set of open PRs at the time the plan was written, so a row whose PR has since merged records what the sweep was scoped to cover and needs no edit - and it wraps the whole table in the checked markers, which satisfies the gate for every row at once. Master's version is taken whole; the deletion and its paragraph are gone. THE SIGHTING RENUMBERS, THE EXISTING ONES DO NOT. Master added three `CLASS2_STALL` sightings while this was open, taking the ordinals through nineteen, and says why they cannot move: their numbers are cited from two other notes and from inside the ledger. So this branch's `ZOMBIE_MEMBER` capture becomes the twentieth. Its cross-reference to "the fourth capture immediately above" is loosened, since a fifth capture has since landed between them, and its claim that `Chaos Pain Suite` gates nothing is corrected: master's fifth capture records it as required and gating, which is the opposite of what the fourteenth sighting said, and master's reading is the current one. Master's standing advice in the same commit is the reason none of this is load-bearing - cite the seed, never the ordinal. THE MANIFEST ENTRY IS SET BEFORE THE MERGE, NOT AFTER. `tooling(upstream-map) #204` introduces the rule and a guard that insists: write `merged` in the branch and push it, because branch content is visible to nobody until it lands and is correct the moment it does, whereas the other order leaves the manifest lying until somebody remembers. This entry moves to `merged` accordingly. Also settled, and recorded where it will be re-read rather than only here: the `propagateMdc` default. It was the one open decision this work carried, flagged as the maintainer's to overrule; Antony confirmed default-on at merge prep. The note now says it is decided rather than proposed, and says the residual pinning risk was what was put to him and taken - so the risk alone is not grounds to reopen it. No code conflicted. Compile clean, and the MDC suites across core, vert.x, Reactor and Mutiny still pass alongside `ReactorPCTest`, which pins the other half of the earlier Reactor composition.
…o feat/mdc-context-propagation
… as corroboration not a control `ChaosRevokeUnderWorkDrainIT` went red on this branch's CI with the BLOCKED-on-monitor discriminator: the poll thread parked acquiring the `commitCommand` monitor, held by that instance's own control thread, reached through `commitOffsetsThatAreReady` from `onPartitionsRevoked`. Same monitor, same holder, same method pair as the five captures above it. FILED AS WEAKER EVIDENCE ON PURPOSE. The frames read `:1639` / `:573` rather than master's `:1585` / `:548`, and this time the shift is the branch's own - this work edits `AbstractParallelEoSStreamProcessor` to wrap the pool submit in an MDC scope, which moves everything below it. Resolved against this branch's copy the pair is identical, matched by method and monitor the way the fifth capture instructs rather than by line number. But a branch that edits the file cannot rule itself out the way #346 and #374 could, so this adds another arm-and-holder match and no independent evidence about cause. The fourth capture already draws that distinction for #267 and this follows it. IT IS NOT THE FLAKE, AND THE ENTRY SAYS SO. The same branch produced a `ZOMBIE_MEMBER` capture on an earlier head, recorded below as a sighting with its own standing seed-dependent explanation. Different probe, different failure mode. Reading any red `Chaos Pain Suite` on this branch as "the flake" would merge the two, and only the monitor-and-holder match separates them - the discriminator is the live defect. Seed `818084281700661522`, recorded with its replay command. RETRIEVAL DECIDED WHAT THIS ENTRY COULD SAY, so it is recorded too. The job log is large, and `gh-run-view-log-truncation.md` documents three occasions where a truncated fetch produced a published-and-wrong diagnosis - one of them to a reader who had that document open. The autopsy here came from the uploaded `chaos-suite-reports-*` artifact, route 1 in that file, which survives server-side stream truncation because the autopsy is `system-out` inside the failsafe XML. Seed and both frames came out intact. Also repaired: the `ZOMBIE_MEMBER` sighting below lost its opening `post-merge: checked-begin` when this file was rebuilt from master's copy during an earlier merge, leaving an unmatched `checked-end` and three self-references the gate could no longer see as attested. Begin and end markers now balance.
…idence about A capture that names a defect but not its fix makes the next reader re-derive the link, and this file has enough entries that re-deriving it is not free. #29 is the open draft that replaces the `synchronized (commitCommand)` acquisition in the captured frames with `ReentrantLock.tryLock()`, so it targets exactly the pair the capture records. Stated as what it makes possible rather than as a cross-reference: the seed joins the set this file has been accumulating for the verification it declared before the fact - replay with the tryLock applied and read whether the poll thread is still BLOCKED on that monitor. Landing the fix then tests the prediction instead of merely following it.
Master had already recorded this branch's own chaos capture, so the entry written here is deleted rather than kept alongside it. THE DUPLICATE. The sixth BLOCKED-on-monitor capture was written up on this branch from the failsafe artifact; master's `## 2026-08-27` section records the same run, the same seed `818084281700661522`, the same frames, and reaches the same reading - corroboration rather than a clean control, because this branch edits `AbstractParallelEoSStreamProcessor` and so cannot rule itself out. Master's version is also the better one: it sits inside a pairing analysis that reads two PRs' reds ten minutes apart, on per-PR VMs where co-residency is impossible, with master green between them - which is a claim neither capture supports alone. So this keeps master's and drops the local one whole. AGENTS.md's rule is that a fact stated twice drifts apart, and the owner is whichever copy the rest of the file is written around. Nothing is lost: the seed, the frames, the match-by-method-and-monitor instruction and the not-a-control caveat are all in master's paragraph already. The `ZOMBIE_MEMBER` sighting on this branch is NOT a duplicate and stays - a different run, a different seed, a different arm from the discriminator, which is the distinction that whole entry exists to draw. THE GATE CAUGHT A REFERENCE MASTER COULD NOT SEE. Master's paragraph cited this work by BRANCH NAME, and a branch name stops resolving the moment the branch is deleted. `check-branch-self-reference.sh` only ever inspects the branch it is run on, so it never fired for whoever wrote that line and fired immediately here. The reference now names the PR, which is permanent, and the paragraph is attested. That is the gate working as designed rather than an inconvenience: the reference would have rotted on merge, silently, in a file people navigate by citation.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…t as open Both release notes still described MDC context as un-captured at submit time - "no `copyOfContextMap` anywhere". #205 merged on 2026-08-27 (commit 15a23de) and added `internal/MdcPropagation.java`, which captures the caller's map on the submitting thread and restores the worker's own on the way out. So the claim was false on both pages, and this branch made it worse: 5694c55 rewrote the blockers bullet, re-asserted "two are still open", and attested the paragraph `post-merge: checked` - the attestation covered the tense, which was the thing being checked, and carried the stale fact through with it. That is the failure docs/inflight/AGENTS.md's "track only what is currently OPEN" exists to stop, arriving in the one file a reader consults to decide whether 0.6.0.0 can be cut. A release operator reading it would have gone looking for an MDC fix that shipped four days earlier. - `release-0600-blockers.md`: the #197 triage bullet now names the one item still open (#155, fix on #201) and the landed ones by the PR that closed each - #217, #205, #199. The live tally is gone: "two are still open" is a count, which that doc forbids precisely because it rots without anything going red, and this is the second time this bullet's count has been wrong. - `release-0.6.0.0.md`: the MDC entry is deleted rather than rewritten as fixed - the directory's rule is that making a stale entry accurate is the wrong move. Nothing is lost: the blockers file now names #205, and #195, the mirror that raised it, already has its own line in the release sweep. Also names `CHANGELOG.adoc` where docs/releasing.md said "every section of this file". The bullet sits inside docs/releasing.md, so "this file" read as the wrong one; the renderer self-test walks the changelog, not this doc. Verified: bin/check-all.sh 15/15, and check-branch-self-reference still passes over the rewritten paragraph.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
docs/inflight/branch-mdc-context-propagation.md carries `post-merge: exempt-file - ... it is deleted when that PR lands`. Its PR, #205, merged on 2026-08-27 and did not delete it, so the marker has been sitting on master reading as live for five days - exactly what docs/inflight/AGENTS.md predicts: Never leave a "delete this when #NN merges" marker on master. The merge is exactly when nobody is looking here, so the marker outlives the work and the next reader inherits a stale note that reads as live. WHY THIS RIDES ON #203 rather than becoming its own PR: this PR just retired its own instance of the same defect (f029828, pr-log-verbosity.md), and the same-defect sweep the merge checklist requires - "before merging a fix, look for other instances of the same defect" - is what turned this one up. Landing the sweep's finding beside the fix is the sweep working, not scope creep. Antony asked for it here. The note's own subject has already shipped, so "what landed" is now just a description of code. Took the directory's four outcomes in order; every symbol it names was re-grepped and still resolves (MdcPropagation, PCModule.mdcPropagation, ParallelConsumerOptions.propagateMdc, MdcContextPropagationTest, MdcBoundaryProbe, captureCallersDiagnosticContext, supervisorLoop), under the bz.stub package. MIGRATED, because it outlives the note and nothing owned it: - The default-on DECISION, with its reasoning, onto ParallelConsumerOptions' propagateMdc javadoc. The option said "On by default"; it did not say why, nor that the choice is settled. Carried forward as settled - not propagating fails silently for everyone, propagating fails visibly and has a kill switch, and the pinning risk was put to Antony at merge prep with that mitigation and taken. Flipping it takes evidence of the pinning biting, not a re-reading of the trade. - The KNOWN GAP on the reactive engines, same javadoc, because it is the only user-facing item in the note and the option is where a user reads about propagation. `grep -rni "context-propagation|micrometer"` found it stated nowhere outside the note: Reactor/Mutiny propagation covers the user function's invocation and PC's terminal signal handling, not the operators of a returned Publisher/Uni on further schedulers, which needs io.micrometer:context-propagation and is the user's call. - The ONCE-PER-INSTANCE guarantee for the startup INFO line, onto captureCallersDiagnosticContext's javadoc. That javadoc already said where the method is called from; it did not say the once-per-instance property is STRUCTURAL - supervisorLoop is the only caller and throws IllegalStateException on a second poll* - nor that moving the line to a per-batch path would be a regression, which is the part a future change needs to meet. - The MEASUREMENT behind the zero-allocation claim, onto MdcPropagation's class javadoc, next to the claim it supports rather than into docs/solutions/. That directory is for solved problems; this is evidence for a design claim, and the claim is what a reader is standing in front of when they want it. The binding-specific caveat came with it - the zero is logback's MDCAdapter returning null for an empty context, and a binding returning an empty map would allocate one small map per batch, so re-measure before repeating the claim elsewhere. The pinned logback version was dropped in the move: the caveat is about the binding family, and a version in javadoc goes stale silently. - The RED-VERIFICATION for the three engine tests, onto MdcBoundaryProbe's javadoc - the one place all three already @see. Each was run with its module's initAsyncConsumer(..) overridden to propagateMdc(false) and fails on assertCallersContextWasVisible. The probe now says to redo that override if the propagation or isolation logic changes, and points at the core test's javadoc for the worker-pool equivalent. CONFIRMED ALREADY OWNED, so it dies with the note rather than being restated: - PC's own keys winning a collision - MdcPropagation's "Precedence" paragraph states it and why (PC's log lines are read by those keys), and the option javadoc names pcId and offset. - Capture at poll* time rather than per record, AND the reason - the captureCallersDiagnosticContext javadoc gives it verbatim: that is the only moment the caller's context is reachable, since PC's threads do not inherit it and the SLF4J MDC is not inheritable. - The pinning consequence - the option javadoc has it, so the note's own claim that it is "documented on the option" checks out. Keys-only-at-INFO, and that it is logged so an accidental pin is discoverable, is the comment inside captureCallersDiagnosticContext. - propagateMdc=false being an exact revert including the pre-existing pooled-thread leak - the option javadoc's last paragraph. - Which boundaries are covered, and that the controller and broker poller adopt() outright - the option javadoc and MdcPropagation#adopt's javadoc respectively. - The core leak-on-reuse test being known to go red, with the flip instruction - MdcContextPropagationTest's class javadoc, verbatim. - Composition-not-inheritance for the engine tests, and why the per-engine assertions stay - MdcBoundaryProbe's class javadoc, verbatim. - The fork/upstream mapping (#195 mirroring confluentinc#907, raised on #197) - the mdc-context-propagation entry in upstream-map.yaml, plus MdcContextPropagationTest's javadoc. Nothing transient survives: "branch feat/mdc-context-propagation exists" is `git branch`'s answer, not a file's. ALSO CORRECTED, found by the same read: the upstream-map.yaml entry still said the default was "NOT yet signed off by the maintainer; see the PR body" - untrue since merge prep, and it pointed at a merged PR's body for a risk that now lives on the option's javadoc. The manifest owns the fork side of this mapping, so a wrong claim there passes every check and rots. Fixed in place, carrying the decision forward as settled. No CHANGELOG entry, by policy. Nothing in the tree cited the note: `grep -rn branch-mdc-context-propagation` over the worktree is empty. Verified locally: bin/check-all.sh 15 ran / 15 passed / 0 failed (including check-file-refs, check-inflight-tags, check-issue-refs, check-copyright-headers, check-upstream-map); bin/todo-index.sh --check up to date; javadoc:javadoc on core clean for every link added; core unit suite unchanged at 546 run / 0 failures / 0 errors / 8 skipped. No main or test code behaviour changed - javadoc and one manifest sentence only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
4 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…lure log lines (#203) Two log lines interpolated a whole polled record batch, so each grew with max.poll.records and with the assigned partition count until log tooling truncated away the part the line existed to report. Closes #169 (confluentinc#631) and #170 (confluentinc#640). - RemovedPartitionState.maybeRegisterNewPollBatchAsWork logged the entire RecordsAndEpoch at WARN. It now logs the topic-partition, record count, offset range and epoch at poll - what an operator needs to tie the drop to a rebalance. - The user-function failure line logged the whole PollContextInternal, which renders every record with its keys and values. It now logs a summary of the same shape. Nothing is lost: the unabridged object moved one level down to DEBUG, so it has to be asked for rather than shipped on every occurrence. HOW. The formatting lives in one place, RecordBatchSummary, rather than two ad-hoc format strings. It names at most five partitions before collapsing the rest to a count, so the line has a fixed ceiling regardless of batch size or assignment size - which a plain "log the count too" would not give on a consumer holding thousands of partitions. The bound is O(1) in the batch but linear in the topic name; a test replays the batch under Kafka's 249-character maximum topic name, which the first version of the bound assertion would have failed. Both new assertions read the emitted line, not the format string, and each was proven to discriminate by reverting its production line. The DEBUG dump sits INSIDE ThrowableUtils.logWithoutEscaping, below the error line, and that placement is load-bearing: rendering PollContextInternal calls toString() on user keys and values on the failure path, which is the hazard logWithoutEscaping exists to contain, and moved above the error line a throwing toString() would suppress the report itself. SLF4J's safeObjectAppend is an independent second layer; both are written down beside the call. WHAT REVIEW CHANGED. The summary was first built unconditionally inside the lambda and handed to log.debug on the PCRetriableException arm - PC's designed retry signal, driven at full processing rate by a downstream outage - and summariseForLog walks the whole batch. Now built per arm behind isDebugEnabled(). RecordBatchSummary no longer boxes every offset into a List to take min and max: one primitive pass, 1M records 40 ms to 14 ms, all 26 probe inputs rendering byte-identically. A dead trailing exception argument that MessageFormatter silently trimmed is gone. The PCRetriableException arm went from log.debug to log.trace: retries are the normal path for a retrying consumer, and a DEBUG line per retry at rate was the original complaint in a slower form. TEST INFRASTRUCTURE. LogCapture is the shared helper for asserting on a class's emitted log lines, restoring the level afterwards; its class javadoc owns the two obligations that make raising a JVM-shared logger safe under thread parallelism. UserFunctionFailureLoggingTest is @isolated for exactly that reason - its first run flooded the timing-sensitive shutdown tests with DEBUG. Master's three headroom tests in AmbientProbeExtensionTest, written against raw logback while this branch was in flight, are ported onto the helper: the merge had taken this side's imports and master's bodies and compiled to nothing with no conflict reported. MdcContextPropagationTest.anEmptyCallerContextIsHandledAndNothingLeaks failed 2 of 2 full runs on this tree and on #201's CI while passing elsewhere. Not a product flake: it asserted, as a precondition, that the JUnit runner thread arrives with a null MDC, on a thread every class in the fork shares. Bisected to ProducerManagerTest and TransactionalBulkCommitTest, which drive processWorkCompleteMailBox on that thread; logback's remove of the last key leaves {} where clear() gives null, both measured. The test now establishes its precondition with a @beforeeach clear. Write-up in docs/solutions/test-flakiness/, the inflight note that tracked the sighting retired, the flake ledger updated. Also retired: the MDC context-propagation note #205 left live on master with a delete-on-merge marker, its content migrated to the javadoc of the code it described. The defect class is wider than these two lines. Eight sites render a collection or a state object into a log line, including PartitionStateManager rendering PartitionState at WARN where Lombok's @tostring prints every tracked record; they are recorded with greppable anchors in docs/inflight/bug-unbounded-log-lines.md for their own change. Upstream-Issue: confluentinc#631 Upstream-Issue: confluentinc#640 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…landed on both sides #201 squash-merged while this branch carried the same two release notes with a fresher MDC state. Both hunks were the same fact from two directions: master had dropped the load-factor bullet as "now #201" but still carried the stale "MDC is not captured" claim this branch had already corrected (#205 landed it). Resolved to the state that is true now that both have merged - the triage list is recorded as landed in the blockers note, and the release note's "bugs found while triaging" section, which had nothing open left in it, is removed along with its heading, which nothing cites. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 3, 2026
#329) The Reactor engine now promises, in its javadoc, that a record succeeds when its publisher completes and fails when it errors - the items emitted in between are the user's, and the engine neither counts nor inspects them. This lands the test suite that pins that promise, and the solutions write-up that records the defect class it guards against. The defect: a user function returning a publisher that emits nothing - Mono.empty(), a Mono<Void> from Mono.fromRunnable(..), a Flux whose filter removed everything - left its record in flight forever. In-flight filled to maxConcurrency, back-pressure halted selection, and the consumer stopped with no exception, no failed record and nothing in the log. ReactorProcessor#react subscribed with Flux#subscribe(Consumer, Consumer), which takes onNext and onError, so completion was only ever reached by an emitted item - and a publisher emitting several items ran the success path once per item, corrupting the in-flight counter the engine throttles on. This branch found and fixed that independently; master then landed the same three-argument subscribe via #342, wrapped in the MDC propagation from #205, with a single regression test for the empty Mono. What lands here on top of that: - ReactorEmptyPublisherTest: empty Mono, Mono<Void> from a runnable, empty Flux, a single-item control, the multi-item case, and an erroring publisher as a second control. Every case asserts two numbers, because the committed offset alone cannot tell a stall from a slow run: the user-function invocation count pins at exactly maxConcurrency when completed records are not being released. The suite runs with maxConcurrency deliberately below the record count; at the module's usual cap the defect looks like nothing worse than a missing commit. The multi-item case asserts in-flight returns to zero, which is what catches over-completion - a commit cannot see it. Both controls were green before the fix, so a change that broke completion outright could not pass as success. - ReactorUnitTestBase gains overridable maxConcurrency and commit mode so the suite reuses the harness instead of forking it. - The react(..) javadoc states the completion contract. - docs/solutions records the defect class - a reactive subscribe whose terminal action is wired to the wrong signal - the invocation-count tell, and the sweep. Mutiny was the counterexample that made this a Reactor defect rather than an ExternalEngine one: MutinyProcessor#onRecord already subscribed with an explicit completion callback, and the two engines are offered as interchangeable, so they must agree on whether "produced no value" means "succeeded". The defect-class sweep over every .subscribe( in the tree found no other instance: Vert.x uses Future#onSuccess, which fires once even on a null result, and the remaining hits are topic subscriptions with no completion signal to mis-wire. Not settled, and deliberately kept out of the solutions doc: the path by which pre-fix duplicate completions were only partly absorbed - every duplicate reaches onComplete, yet in-flight settled at one rather than being driven negative. The corrupt end state is asserted against; the absorption path is not. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…hecklist #476 vetted every open note and touched five this branch also edits. Three conflicted and each is resolved on the side that reflects the later decision: - release-0600-blockers.md: deleted here, modified there. Kept deleted; the two corrections the sweep made to it are folded into the burn-down's tag-day checks. One of them is only half right and is recorded as such: release.yml does build a notes file from the changelog (#72), but its exact heading match fails on the "(unreleased)" suffix and falls back to generated notes, which is what #199 fixes. The sweep's "the MDC gap is real" is refuted on the tree - MdcPropagation captures and restores the caller's context since #205 - so the burn-down keeps that correction. - release-experimental-module-records.md: the 2026-09-07 deferral wins over the sweep's vetted marker, per the per-note contract (a deferred note carries the state, not the marker). - release-when-is-v6-good-enough.md: the sweep's dated "the date passed" paragraph and its stale vetted marker ("the question is still unanswered") are dropped; the rewritten note records the decisions and names the failure mode in its own words. Also carried from the sweep: the citation to the poisoned-transaction wedge note is repointed to the sibling #476 merged it into; the burn-down now points at the sweep's own "what gates v6" list in process-candidate-ranking.md and records where the two disagree (the sweep reads the poisoned pair as not gating, and the batchSize validate() bound as the cheapest real fix, which could ride in tier 1); and the #44 exception is narrowed - #466 bounded the revoke wait, so what #408 still owns is declining rather than waiting. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
56 commits of master, none of which touched this PR's mechanism: master changed neither `.github/workflows/release.yml`, `bin/release-notes.py`, `bin/test-release-notes.sh` nor `.github/workflows/mutation-full-sweep.yml` since the branch's last master merge, so the renderer and the workflow step that calls it merge unchanged. Master's `release.yml` still carries the inline `awk`/`sed` converter this PR replaces. Master also changed six of the files this PR touches. Four auto-merged: `docs/releasing.md` and `bin/AGENTS.md` (#457's job batching, #442's integration sharding), `.gitignore` (#378, #440), and `docs/inflight/release-0.6.0.0.md` (#448, #476). Two conflicted. `docs/inflight/release-0600-blockers.md` - the #197 triage bullet, inside the `post-merge: checked` markers. This branch (2026-09-01, 2026-09-03) rewrote it in post-merge terms as "all four landed"; master's grooming sweep (#476, 2026-09-08, correcting on 2026-09-07) rewrote the same bullet to say the empty-release-body item **was already false when written** - #72 gave the workflow a `--notes-file` built from the `CHANGELOG.adoc` section on 2026-07-29, with `--generate-notes` only as a fallback. Master's correction is right and is the later decision, so it is kept; master's claim that MDC is still open is not - #205 merged 2026-08-27, which is why this branch corrected it, and that correction is kept. Resolved as one bullet carrying both later facts, and it now states what #199 actually does (replace the inline converter, and fail the release on a missing or unrenderable section) rather than the retired claim that it restores a body that was never absent. `docs/refactoring.md` - two unrelated new sections appended at the same point. Both kept: this branch's "`bin/release-notes.py` is Python in a Node-default `bin/`" and master's "JUnit tag resolution is implemented twice". No content from either side dropped. Neither `--ours` nor `--theirs` was used. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XS64Xttx4vF5datYh7fmk
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Raised as a non-blocking finding on #197 by a user in the confluentinc#907 thread (mirrored on this fork as #195): the
caller's SLF4J MDC is not propagated into the worker pool.
Confirmed on master before writing anything:
pcId(MDC_INSTANCE_ID, put inaddInstanceMDC()and inBrokerPollSystem) andoffset(MDC_WORK_CONTAINER_DESCRIPTOR).MDC.getCopyOfContextMap()andMDC.setContextMap()appear nowhere in the repository.So a caller who has established diagnostic context - a
trace_id, arequest_id, a tenant - loses all of it themoment a record crosses into the worker pool, and again crossing into vert.x, Reactor or Mutiny. The logs their
function writes cannot be correlated back to the originating request.
Where the capture and the restore live
New
MdcPropagation(coreinternal):capture()on the thread that has the context, andenter(..)returningan
AutoCloseablescope on the thread that runs the work.submitWorkToPoolInner, on the controller thread at submit timeaddVertxHooksand the web-request callback, on the worker threadFuturereact(..)'s wrapper, on the worker threadMono.fromCallableand both terminal signalsonRecord(..)'s wrapper, on the worker threadUni.deferredand both terminal signalsThe caller's own snapshot is taken in
supervisorLoop- i.e. on the thread that callspoll*(). That is the onlymoment it is reachable: none of PC's threads exist yet and the SLF4J MDC is not inheritable. The controller and
broker-poller threads then
adopt(..)it outright (they serve one PC instance for life, so there is no later taskto leak into), which is also what makes the submit-time capture pick up the caller's context.
Judgement calls
Precedence - PC's keys win.
pcIdandoffsetare applied after the caller's map at every call site, so acaller key of the same name cannot shadow them. PC's own log lines are read by those keys; a caller key shadowing
offsetwould silently corrupt PC's own diagnostics. Covered by a test that deliberately collides onpcId.On by default, with
ParallelConsumerOptions.propagateMdcas the kill switch, wired throughPCModule.mdcPropagation(). Not propagating fails silently, for everyone, and only ever gets fixed by users whoread a changelog; propagating fails visibly, as one extra key in a log line, and can be switched off. The
asymmetry decides it. Setting it
falserestores the old behaviour exactly - leak included - so it is a truerevert rather than half of one.
The one genuine risk of a default-on capture is that it is taken once, at
poll*(): a request-scoped value in theMDC at that moment gets pinned to the consumer for its whole life, which is the same "actively misleading logs"
failure this PR exists to prevent. Mitigated rather than ignored - PC logs the captured keys (never the values,
which are the user's data) at INFO on startup, so the mistake is discoverable. It is also called out on the option's
javadoc.
That INFO line is once per consumer instance, not per poll and not per record, and structurally so:
captureCallersDiagnosticContext()is called only fromsupervisorLoop, which throwsIllegalStateExceptionifpoll*()is called more than once. There is no hot path it can reach.The leak is fixed too, not just the propagation. On master, whatever the user function puts into the MDC stays
on the pooled thread for the next, unrelated, record. The restore scope fixes that as a side effect, and it is the
half of this change that matters most.
Null-safety.
getCopyOfContextMap()returnsnullfor an empty context; that is the normal path and ishandled throughout, with a test asserting no NPE and correct behaviour when the caller sets nothing at all.
Hot-path cost: measured, not assumed.
getCopyOfContextMap()returnsnullwithout allocating when thecontext is empty,
enter(null)returns a shared singleton scope, andpcIdis only ever set whensetMyId()hasbeen used. Rather than leave that as reasoning, it was measured with
ThreadMXBean.getThreadAllocatedBytesover amillion
capture()/enter()/close()cycles on logback 1.6.1:capture()on the controller threadenter()+close()on a clean worker threadSo the "nil for users who never touch the MDC" claim holds exactly - zero bytes, not merely "small". For a user who
has set MDC it is ~424 bytes per batch, against the
groupingBycollector, twoArrayLists, aPollContextInternaland aFutureTaskthatrunUserFunctionalready allocates per batch.One caveat now recorded in the inflight note: the zero is a property of logback's
MDCAdapter, which returnsnullfor an empty context. Some other SLF4J bindings return an empty map instead, which would makecapture()allocate one small map per batch. Behaviour is correct either way; only the zero-allocation figure is
binding-specific.
Modules covered, and what is not
Covered: core (worker pool), vertx (event loop), reactor (scheduler), and mutiny. Mutiny was not in
the brief, but it is a genuine third boundary of exactly the same shape as Reactor's -
runSubscriptionOn(executor)means the user's function does not run on the worker thread either - so leaving it out would have shipped a
documented inconsistency for ~5 lines of saving. It has its own test like the others.
Deliberately not covered: the operator chains of the
Publisher/Unithe user returns. We carry the contextinto the invocation of their function and into PC's terminal signal handling; following it further, onto whatever
schedulers their own chain hops to, needs Reactor's
io.micrometer:context-propagationand is the user's call, notours. Recorded in the inflight note so the next person does not treat it as an oversight.
Tests
17 tests:
MdcPropagationTest(9, the primitive itself),MdcContextPropagationTest(5, end-to-end throughthe real worker pool), and 1 each in vertx / reactor / mutiny. They assert the caller's key is visible inside the
user function, that PC's own keys survive and win a collision, that the context does not leak to a later task on the
same pooled thread, and that an empty/null context does not NPE.
The three engine tests share
MdcBoundaryProbe(core test-jar, the same routeTestConventionRulesandWireMockUtilsalready take) by composition - each already extends its own module's unit-test base, so a commonsuperclass is not available. The probe holds only what is genuinely identical: the caller key/value, the observation
bookkeeping, the MDC teardown, and the "context was visible" assertion. The per-engine assertions stay in the tests,
because they are what stops a test quietly degrading into a re-run of the core worker-pool case, and they really do
differ - Reactor asserts its scheduler thread positively (
boundedElastic), while vertx and Mutiny can onlyassert "not a
pc-thread" since their executor is caller-supplied.Every one of them is verified to be able to fail. A green test that cannot go red proves nothing, so:
expected to be empty but was: [poison_from_offset_2, poison_from_offset_3, poison_from_offset_0, poison_from_offset_1]threadsUsedhas size 1, so it cannot pass by accidentally getting a fresh thread perrecord.
maxConcurrency(1)makes the reuse deterministic rather than lucky.propagateMdc(false)(temporarily overridinginitAsyncConsumer(..)) and each failed on its own "context must be visible on the <engine>" assertion,reporting
values seen: [null, ...]. Re-verified again after the shared-probe refactor, so the extraction did notquietly defuse them. They also assert which thread observed - Reactor positively (
boundedElastic), vertx andMutiny as "not a
pc-thread" - so they cannot silently stop covering the second boundary.No existing assertion was weakened. The one failure I hit while developing (
ReactorMdcPropagationTestexpecting 4records) was my own arithmetic -
ReactorPCTestprimes a 4th record in its own@BeforeEach,ReactorUnitTestBaseprimes none - diagnosed from the surefire report (exactly 3 processed, 3 primed) rather than papered over with a
longer timeout.
Full unit suite (
bin/ci-unit-test.sh) green across all 11 modules;bin/check-copyright-headers.shclean;upstream-map.py validateOK.Refs #197, #195, confluentinc#907.
Merged up from master
Master moved a long way while this was open, and it was merged in more than once. Only the parts that changed this PR's code or its reasoning are recorded here.
The upstream-map restructure. #114 made the manifest track upstream PRs only, because every upstream issue now has a fork mirror, and deleted ten tracking-only entries — one immediately beside the entry this PR adds. Master's deletions were taken wholesale and this PR's entry refitted to the new shape: block style,
fork_issue: 195cross-linking the mirror of confluentinc#907, and renamedissue-907-mdc-propagation→mdc-context-propagation, since anissue-prefix now reads as one of the entries that commit removed.That reading is still open to challenge: the manifest says it tracks upstream PRs, and this maps to an issue.
fork_issue: 195satisfies both readings — the upstream issue's diagnosis and closing live on the mirror, this PR's lifecycle lives in the manifest. Happy to drop the entry instead.The package rename. This branch predated
io.confluent.*→bz.stub.*and was brought across withbin/rename-packages.shfollowing its ownBRINGING AN OPEN BRANCH ACROSSprocedure — tooling taken from master, legacy-token residue cleared, rename applied, and only then master merged. The order is the whole point: renamed-branch meeting renamed-master gives git a rename to pair on each side, and theTestConventionsArchTest.javafiles duly surfaced as loud rename/rename conflicts rather than the silent cross-module misapplication the un-renamed order produces.A real semantic conflict in
ReactorProcessor. Master'sfix(reactor) astubbs#342replaced the two-argumentFlux.subscribe(onNext, onError)with the three-argument form: the two-argument overload has no completion callback, soonComplete— which retires the record — was wired as the onNext consumer. A Publisher emitting several elements retired its record once per element; one completing empty retired it never, leaking the record's in-flight accounting and, since the dispatch ceiling landed, its dispatch permit for the life of the process.This PR wraps those same callbacks in an MDC scope. The two compose rather than compete, and both survive: master's three-argument shape, with each terminal callback body inside the scope. The element consumer is deliberately left unwrapped — it is neither the user's function nor terminal handling, which is the boundary this propagation claims, and
doOnNextin the same chain is unwrapped for the same reason. Each side keeps its own pin and both pass:ReactorPCTest#anEmptyPublisherStillRetiresTheRecord, which master's commit records as red against the two-argument form, andReactorMdcPropagationTest.Checked, not assumed: master's
fix(core) astubbs#322rejectsPCModulegetters that memoise a collaborator while taking that collaborator's owner as a parameter, and swept the class for the shape.mdcPropagation()was added after that sweep, so it could not have been covered — but it takes no owner parameter and memoises offoptions(), the same shape aspcMetrics(), so it is outside the defect class rather than an unswept instance of it.Static analysis on this PR's own lines
The merge brought
-Xlint:alland SpotBugs-over-test-code with it, and both then fired on lines this branch wrote.bin/check-pr-analysis-surfaces.shseparates those from inherited findings and reported three; it now reports none.MdcBoundaryProbe.observations()returnedjava.util.Collection(IMC_IMMATURE_CLASS_COLLECTION_RETURN). The field is aConcurrentLinkedQueue, so the declared type said strictly less than the thing behind it; it returnsQueuenow, which is both accurate and what a caller awaiting it wants to know.PRMC_POSSIBLY_REDUNDANT_METHOD_CALLSonMDC.get(String)— the detector seeing a key read twice with the same constant argument and taking the second read for a cacheable repeat of the first. It is not: the scope under test is what changes the thread's context between the reads, so caching would delete the assertion. The rule registry keeps PRMC on, so this is not suppressed: all MDC reads now go through oneassertMdc(key, expected)helper, and each test calls it with a different expectation. The detector is satisfied because the repeat is gone, not because it was told to look away. It pays for itself independently — the helper names the key in the failure message, whichassertThat(MDC.get(k))never did.Verified in both directions rather than assumed: SpotBugs run locally over the core module before and after, and the test restructure given a negative control — breaking one expectation on purpose fails, at the right call site, with the key in the message.
Left unfixed, deliberately: the
[try] auto-closeable resource scope is never referencedwarnings on the try-with-resources blocks. They are real javac output but reach the build log only — not the check-run annotations — so the analysis-surface gate does not report them. The variable is unused because the scope is the subject of the test.A chaos flake captured on this branch's CI
ChaosChurnStormITtripped theZOMBIE_MEMBER/REBALANCE_BLOCKEDprobe on one run. It is recorded as the seventeenth sighting indocs/inflight/bug-857-family.md, with its seed and replay command, because that ledger's own argument is that these captures are easy to lose — the log expires and the seed goes with it.It is not this branch's defect, and the ledger already establishes why: the reds are seed-dependent rather than branch-dependent, one earlier capture came from a branch whose entire diff was markdown and a comment, and runner contention was ruled out on timing. The entry adds one new question — every prior capture of this arm predates the dispatch ceiling, which changes what a leaked permit costs — and names the control that would settle it.
Chaos Pain Suiteis not a required check, so it gated nothing.Riding along: a chaos capture, and a citation repair
One
Chaos Pain Suitered on this branch's CI is recorded indocs/inflight/bug-857-family.md— aZOMBIE_MEMBERcapture, seed7543483068749855826, which that ledger already explains as seed-dependent rather than branch-dependent. It is not caused by this PR and it gates nothing.A second red on this branch was the BLOCKED-on-monitor discriminator — the poll thread parked acquiring the
commitCommandmonitor held by its own control thread, which is the live defect that ledger's capture series is hunting, and whose fix is #29. It is deliberately not in this diff: it was written up here first, then master recorded the same run independently and better, inside a pairing analysis reading two PRs' reds ten minutes apart. The local copy was deleted rather than kept alongside master's, since a fact stated twice drifts apart.What this PR does still carry from that is a repair to master's paragraph: it cited this work by branch name, which stops resolving the moment this branch is deleted.
check-branch-self-reference.shonly inspects the branch it runs on, so it could never have fired for whoever wrote that line, and fired immediately here. It now names the PR, which is permanent.Worth keeping straight, because conflating them loses the distinction the ledger exists to draw: the
ZOMBIE_MEMBERarm has a standing seed-dependent explanation; the discriminator does not. Only the monitor-and-holder match separates them, and reading either as "the chaos flake" merges them.Riding along: the mutation-lane findings
docs/mutation-lane-findingsis folded in here rather than opened as its own PR — twodocs/inflight/notes, no code, cut from master and carrying no dependency on this work.They record what the PR-scoped mutation lane found once #371 fixed it, and a gap in the lane itself: the survivor table is written only to
$GITHUB_STEP_SUMMARY, which the REST API does not expose, so recovering which mutants survived cost a local re-run and a hand-writtenmutations.xmlparser.bin/check-pr-analysis-surfaces.shalready lists that table under surfaces it cannot read, so the gap was documented before the lane produced anything worth reading.The second note argues the survivor on #344's own new code is an equivalent mutant rather than a missing test — the bound is a sample of
offsetHighestSucceeded, and a succeeded offset is by construction absent fromincompleteOffsets, so<and<=cannot differ while that invariant holds. Writing a test to kill it would describe the implementation rather than the contract. It names the wider gaps that are real and predate that PR.This is unrelated to MDC propagation and is here only because it needed a ride. Same for the chaos sighting above. Both are capture-only.
Defect-class sweep
The boundary this PR fixes is "work crosses onto a thread the caller's context never reached". Every
ExternalEnginesubclass was checked for it:VertxParallelEoSStreamProcessor,ReactorProcessorandMutinyProcessorare the complete set, and all three are covered here alongside the core worker pool. No fifth boundary was found uncovered.Checklist
MdcPropagationclass javadoc,docs/inflight/branch-mdc-context-propagation.md, and anupstream-map.yamlentry refitted to the post-mirror schema.CHANGELOG.adocdeliberately untouched, per AGENTS.md (generated at release time, not a per-PR chore)docs/features/: this changes what PC's own logging carries, configured by one existing-style option; there is no feature surface to describe beyond the option javadocce-code-reviewvia@claude review thison the PR (three rounds, the last returning no issues) —N/Afor a localce-simplifyrun on the feature itself; the simplify pass this session was spent on ci(hooks): fetch every remote at session start, and refuse a merge onto a stale branch #372Not closing an issue
Neither reference is a closing one, and that is deliberate. #197 is the 0.6.0.0 release umbrella and this is one item within it; #195 is the mirror of confluentinc#907, whose subject is whether the project is maintained — this PR fixes a finding raised inside that thread, not the thread. Closing either would take unrelated work with it.
Refs #197, #195, confluentinc#907.