Repository navigation
test(core): the MDC precondition test establishes its state and accepts either spelling of empty - #418
Conversation
…n earlier class leaves behind
MdcContextPropagationTest.anEmptyCallerContextIsHandledAndNothingLeaks opened by asserting
MDC.getCopyOfContextMap() is null, and went red with "expected: null but was : {}" -
locally on the #262 branch, twice on consecutive heads of #416 with no Java
in its diff, and then on master's own build for the #415 merge.
The write-up's hypothesis was that logback's MDC.clear() leaves an empty map. A probe
against the build's own jars refuted it: clear() nulls the thread-local, remove(key)
does not. The arming action is therefore a put-then-remove with no clear after it, on
the thread that later runs this test. The only MDC.remove in product code is in the
control loop's mailbox processing, and ProducerManagerTest and TransactionalBulkCommitTest
drive controlLoop() on the test thread without clearing afterwards. With the ci profile's
reused forks and no intra-JVM parallelism, whether one of them precedes this class on the
same thread is decided per run - a per-run coin, which is the shape the ledger recorded.
Control experiment, one term changed: with the two classes in one fork,
ProducerManagerTest first fails the precondition and MDC-test first passes; the failing
order passes against this fix.
The fix is test-side and deliberately so. PC treats null and {} as the same thing - no
diagnostic context - so the precondition was asserting a logging-binding detail and
calling it product behaviour. It now accepts null or an empty map. The controller-thread
leftover is harmless where it actually lives, and clearing the MDC in the two arming
tests would only hold until the next test drives the control loop on the test thread.
The inflight note is migrated to docs/solutions/test-flakiness/ and deleted; the ledger
row in test-untracked-ci-flakes.md now records four sightings and the fix.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…ts either spelling of empty
Two sessions diagnosed MdcContextPropagationTest.anEmptyCallerContextIsHandledAndNothingLeaks
on the same day without seeing each other, reached the same mechanism by controlled experiment,
and fixed the test in two different places. This commit consolidates them on the one branch
that was cut for the fix, so master gets one change and one write-up instead of a conflict.
The mechanism, agreed by both: on logback 1.6.1 MDC.clear() nulls the thread-local but
MDC.remove(lastKey) leaves an empty map, and the control loop's mailbox processing does a
put-then-remove of the work descriptor. ProducerManagerTest and TransactionalBulkCommitTest
drive that loop on the JUnit runner thread and never clear it, so whichever of them lands in
the same fork before this class leaves {} where the precondition asserted null. With the ci
profile's reused forks and no intra-JVM parallelism, that is decided per run by fork placement.
Sightings: locally on 2026-09-01 (#262), twice on #416, then master's own build
for the #415 merge on 2026-09-02, which turned master red.
What the parent commit did: accept null or an empty map, because PC treats them as the same
thing and the assertion was asserting the logging binding. What c4cbb70 did, on the
#201 and #203 branches: make clearCallersContext @beforeeach as well as
@AfterEach, because a test may only assert a precondition it has established itself, and
correct MdcPropagation.capture()'s comment, which claimed it never returns an empty map.
This commit applies the second on top of the first. They are complementary, not competing:
the @beforeeach makes the precondition deterministic on logback, and accepting {} keeps it
true on a binding that returns an empty map for an empty context, the hazard the original MDC
note already named. Both write-ups are merged into the one under docs/solutions/test-flakiness/,
with each session's ruled-out list and control arm kept, and a new lesson: run the prior-art
search when you start the fix, not only the diagnosis - it is how the third session found both
branches in one command.
Control arm against this tree, one fork, parallelism off, ProducerManagerTest first by
reverse-alphabetical run order: 9 of 9 and 5 of 5 pass. Predicted before the run.
When #201 and #203 next merge master they will conflict on this test file and on
the inflight note both sides delete; resolve to this side and drop their copy of the write-up,
mdc-precondition-asserted-a-thread-state-it-never-established-2026-09-02.md.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…xt-precondition-accepts-empty-map # Conflicts: # docs/inflight/test-untracked-ci-flakes.md # parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/MdcContextPropagationTest.java
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ 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)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
@claude review this |
|
Claude finished @astubbs's task in 2m 51s —— View job Review:
|
|
[superseded - a quarantined test changed outcome] 🧪🔒 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 Updated for Superseded by a newer quarantine lane report. |
🟢 Throughput — OKThis branch measured about 2% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: 8df9cc7, 88f5774, ebd8bcb, 626fc32, 9ff715d, 856dc54, b917ae6, 54e8870, 3ae5dc3, beb01e1 Since the previous push: ratio 0.987 -> 1.022, share 1.659 -> 1.602, rate 71425 -> 64283 (-10.0%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
============================================
+ Coverage 81.19% 81.55% +0.36%
- Complexity 1411 1419 +8
============================================
Files 92 92
Lines 4988 4988
Branches 486 486
============================================
+ Hits 4050 4068 +18
+ Misses 741 727 -14
+ Partials 197 193 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…retired The MDC write-up cited branch-mdc-context-propagation.md by bare filename for the empty-map-binding hazard. Master retired that note in e616c3e and #203 migrated its content into MdcPropagation's class javadoc, so the citation now points there, by a quoted sentence the gate can grep. A single-segment filename is invisible to bin/check-file-refs.sh, which is why the automated review caught it and the gate did not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
🧪🔒 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 Since the previous push: Updated for |
#418 and #203. The first FIXES the MdcContextPropagationTest flake this branch had been recording sightings of, and retires the note they went into - so most of this merge is accepting that master's answer beats what was staged here. THE MDC NOTE IS DELETED, DELIBERATELY, AND NOTHING IS MIGRATED OUT OF IT modify/delete: master retired the note into docs/solutions/test-flakiness/mdc-null-precondition-armed-by-an-earlier-class-in-the-same-fork-2026-09-02.md while this branch was adding a fifth sighting to it. The deletion is accepted whole, and per the four outcomes in docs/inflight/AGENTS.md the question was whether anything in it was still true and unowned. Nothing is: - #418's diagnosis REFUTES the note's hypothesis. The note guessed MDC.clear() leaves an empty map; the real mechanism is MDC.remove(lastKey), with ProducerManagerTest and TransactionalBulkCommitTest leaving {} on the shared runner thread and fork placement deciding the order. It probed that against the build's jars rather than reasoning about it. - The one thing this branch contributed beyond sightings was a control - the class alone passed 5/5, so the polluter is not a sibling method - and that is the conclusion #418 reached by naming the two polluting classes. Superseded, not lost. - The fifth sighting adds nothing to a fixed flake, and master's summary line already carries the one that mattered, master's own build. The ledger's MDC row goes with it: master's "fixed and out" line above the table now owns that test. processInKeyOrder: master's row and section carry a control arm this branch could not run - 1 of 8 failures on a detached worktree at UNMODIFIED master - which is what makes it master-state rather than a branch's problem, plus the same awaitForOneLoopCycle lead this branch reached independently. Master's prose is taken whole and this branch's three load arms are folded in beneath it as corroboration, because they measure the load dependence master's control does not. Verified after resolving: the #197 default flip survived the ParallelConsumerOptions auto-merge (still IGNORE), unit lane green at 709 tests across the four library modules, bin/check-all.sh 15/15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
1 participant
Description
MdcContextPropagationTest.anEmptyCallerContextIsHandledAndNothingLeaksturned master red on 2026-09-02 (the push build for #415, run 33640912294) with the shape the inflight notetest-mdc-empty-context-precondition-is-order-dependent.mdhad recorded from one local sighting the day before:That is the test's own precondition, before Parallel Consumer is constructed. Four sightings now: locally on #262, twice on consecutive heads of #416 (no Java in its diff), then master.
Mechanism, established twice by controlled experiment. On logback 1.6.1,
MDC.clear()nulls the thread-local butMDC.remove(lastKey)leaves an empty map. The control loop's mailbox processing does a put-then-remove of the work descriptor, andProducerManagerTestandTransactionalBulkCommitTestdrive that loop on the JUnit runner thread without clearing it. The ci profile reuses forks with intra-JVM parallelism off, so whether one of those classes precedes this one in the same fork is decided per run by fork placement. Class order alone reproduces it; the inflight note'sclear()hypothesis was refuted by a probe against the build's own jars. The very next master build (run 33643064815, the #416 merge, no Java in its diff) went green with the test unchanged - consistent with fork placement deciding the outcome, and not evidence on its own, which is why the class-order experiment is the evidence.Why this PR has two fixes in one test. Two sessions diagnosed this on the same day without seeing each other's work and fixed it in two different places:
nullor an empty map, because PC treats them as the same thing and the assertion was asserting the logging binding;clearCallersContext@BeforeEachas well as@AfterEach, because a test may only assert a precondition it has established, and corrects a wrong comment inMdcPropagation.capture().They are complementary: the
@BeforeEachmakes the precondition deterministic on logback, and accepting{}keeps it true on a binding that returns an empty map for an empty context, the hazard the original MDC note already named. This PR applies both, and merges the two write-ups into one underdocs/solutions/test-flakiness/, with each session's ruled-out list and control arm kept.Control arm on this tree (one fork, parallelism off,
ProducerManagerTestfirst by reverse-alphabetical run order, predicted before the run): both classes pass.Deliberately not done: touching the control loop (the leftover
{}is harmless on the controller thread it owns), or making the two polluting tests clean up (every future test that borrows the runner thread would need the same courtesy). Both sessions rejected both, independently.Reconciled with master after #203 merged carrying c4cbb70. The merge conflicted on the
@BeforeEachjavadoc and the flake ledger, both resolved to this side; master's copy of the second write-up,mdc-precondition-asserted-a-thread-state-it-never-established-2026-09-02.md, is deleted here with its two paragraphs this one lacked folded in, and the ledger's fixed-line re-pointed. #201 still carries the same commit: when it next merges master it will conflict on the same two files, and resolving to master's side is correct once this lands.Checklist
docs/solutions/test-flakiness/, and the flake ledger row records the four sightings and the fixdocs/features/- N/A - test-only change, no user-visible behaviourdocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - the note this PR retires was the working note, and its content now lives in the solution write-up; nothing remains thatghcannot showce-simplifyandce-code-reviewlocally - N/A - two comment blocks and one annotation; the cheaper@claude review thisroute on the PR is the intended review, to be requested by the author