Repository navigation
test(core): a Lincheck lane, calibrated by refinding four real races unaided - #347
Merged
Merged
Conversation
… at anything Lincheck is a scheduler-controlled concurrency tester: declare a class's operations, and it explores thread interleavings - bounded model checking with a controlled scheduler, plus a stress strategy - checking results against a sequential specification. It is the only tool class that can find torn reads NOBODY HAS NAMED YET; the racing-double seam tests this repo already has can only re-prove seams somebody found by hand. Test scope, core only, deliberately. Lincheck is Mozilla Public License 2.0, which is file-level copyleft and must not end up inside an Apache-2.0 artifact - as a test dependency it is never packaged and never in the published pom's runtime graph. It also drags in the Kotlin stdlib, kotlin-reflect, coroutines, ASM and ByteBuddy, and keeping that in one module keeps the graph in one place. THE FIRST RUN WAS A FALSE PASS, which is why LincheckToolchainProbeTest exists at all. It is a nine-line containsKey-then-get on a ConcurrentHashMap with the result dereferenced unconditionally - code that cannot survive two threads, and the same textbook probe SpotBugs at effort=Max reported nothing on. Lincheck reported no violations on it, in three seconds. The cause: wiremock-jre8 declares org.ow2.asm:asm 9.4 as a DIRECT dependency, so Maven's nearest-wins put 9.4 on the test classpath beside the 9.9.1 asm-commons Lincheck brings. Lincheck's class-file transformer then died inside EVERY transform with NoSuchMethodError: Type.getArgumentCount(String) - a method that only exists from ASM 9.6 - caught it per class, logged it, and carried on with the class uninstrumented. The model checker observed no shared memory and reported success. Pinning asm forward to 9.9.1 fixes it; the pin is the same shape as the byte-buddy one already carried for the same stale wiremock transitives. A control with a known answer is not ceremony here. Every "not found" this tool ever reports has to be told apart from "did not look", and nothing else can do it. LincheckSuperHashCodeProbeTest is the second control, and it settles a real limitation with an arm on each side. Lincheck 3.7's ConstantHashCodeTransformer rewrites every hashCode()I call site into Injections.hashCodeDeterministic(o), which dispatches VIRTUALLY, and does not distinguish INVOKESPECIAL. So super.hashCode() inside an overriding hashCode calls itself and recurses to StackOverflowError - which is exactly the shape Lombok's @EqualsAndHashCode(callSuper = true) generates, and exactly what ShardKey's KeyOrderedKey is. The two nested classes there are identical but for the super call: the one that delegates upwards crashes the model checker deterministically, the one that does not runs clean. ManagedStrategyGuarantee does not help - transformation happens before analysis sections are consulted. That probe is a tripwire, not a note: it asserts the defect is STILL PRESENT, so a future Lincheck that fixes it turns this test red and names the follow-up. The lane is opt-in and non-gating, modelled on the chaos suite: the `lincheck` tag sits in the pom's default excluded.groups and bin/lincheck-test.sh is the entry point, because four separate flags have to line up and each one fails silently on its own - the group filters (an include alone selects nothing), -Plincheck for the JDK 17 module opens the model checker needs, serial execution (Lincheck installs a JVM-wide agent, so two of its classes in one fork share it), and jacoco off (its probes are shared state, and they bury the trace lines that matter). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
Three harnesses over ShardManager, PartitionState and WorkManager, each declaring only the operations the two real threads perform - poll work in, complete it, fail it, sweep a revoked partition, collect commit data. None of them names a seam, injects a latch, overrides a method to widen a window, or orders anything. The question the whole exercise exists to answer is whether Lincheck finds bugs we already know are there WITHOUT being told where, because a tool that cannot refind known bugs has nothing to say about unknown ones. RESULT, per the dossier's four candidates: WorkManager.handleFutureResult (candidate 3, "the worst") FOUND, 2-5s ShardManager.removeWorkFromShardFor (candidate 2) FOUND, ~1.6s PartitionState.createOffsetAndMetadata (confluentinc#894) FOUND, ~4.7s OffsetMapCodecManager.encodeOffsetsCompressed HALF-FOUND and one nobody had named: PCMetrics keeps every registered meter in a plain ArrayList and adds to it from the commit path, which runs on either thread. Two concurrent commits throw ArrayIndexOutOfBoundsException out of ArrayList.add - the classic torn-grow signature, with silent Meter.Id loss as the ordinary outcome. Recorded in docs/inflight/, not fixed here. The encoder row is deliberately not a FOUND. Model checking did once place a switch exactly between the two reads the dossier names, which is the capability no seam test can have - it LOCATED the seam - but both reads returned the same value in that run, so what it demonstrated was the incompletes snapshot going stale, not the range top moving. It happened once in eight model-checking attempts across five configurations, and the leg the dossier actually names is not expressible in the harness as committed: it needs an offset above the current highest-succeeded to complete inside the window, and tracking one takes the stress arm from 3/3 to 1/3, because every extra value the offset generator can produce dilutes the chance a random scenario contains the pair that tears. Every finding above came from the STRESS strategy, and that decides how this can be adopted. No model-checking arm over the product classes survived, for two reasons that are not about the tool being wrong: - ShardKey.KeyOrderedKey is a Lombok @EqualsAndHashCode(callSuper = true) value type, and Lincheck 3.7 rewrites super.hashCode() into a virtual self-call (see the previous commit's probe). KEY ordering is the only mode the shard bug exists in, so there is no way around it. - The commit path is not deterministic under replay. Micrometer updates a timer, two summaries and a counter per encode; two PartitionState accessors build their result with parallelStream(), whose ForkJoinPool threads the model checker did not start and cannot schedule. Excluding the metrics stack cut a run from 287s to 29s and still found nothing, so both guarantee helpers were deleted rather than left as unexercised code. WHAT THE ORACLES ARE, since a harness is only as good as what it compares. PartitionState's commit() does not return the OffsetAndMetadata it produced; it returns what a consumer would RECONSTRUCT from it - the committed offset, plus the incompletes decoded from the payload against that same offset, which is the only number the broker hands back. Comparing base64 would flag any encoding difference; decoding first states the correctness property, so the violation reads "resumeFrom=1 replay=[1,2,3]" - a commit telling the broker to replay records that succeeded - rather than "these two blobs differ". WorkManager's oracle is an exception: acting on a reassigned state trips PartitionState's assert, and neither sequential order throws. TWO HARNESS-FIDELITY POINTS worth inheriting. Operations must not do first-time classloading - UniLists.of() resolves through ServiceLoader, and the model checker read that as a livelock. And a harness must model what production can reach: two overlapping rebalances tore the per-partition counter maps and Lincheck stops at the first violation, so the checkpoint-3 tear was never reached; ConsumerRebalanceListener callbacks come from inside one poll, so nonParallelGroup restores fidelity rather than hiding anything - that defect is recorded in the same inflight note. QuarantinedAnnotationContractTest pinned the entire excluded.groups literal, so adding any lane failed it with a message about quarantined tests gating - and the obvious repair, pasting the new literal in, leaves it pinning a list nobody reasoned about. It now asserts membership of the quarantine tag, which is the contract its own message describes, and still goes red if the tag is dropped. Every test here asserts that a bug EXISTS, so the four fix PRs in flight (#337, #344, #345, #346) turn this lane red on merge, by design - each javadoc names the PR that triggers its inversion. That inversion is the point: after it they stop being calibrations and become regression detectors over the whole operation set of these classes, not just the seams somebody thought of. Full calibration, cost table, and the recommendation: docs/plans/2026-08-25-001-test-lincheck-poc-plan.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
…ne was checked The Lincheck lane was excluded by the pom's default excluded.groups, which is the obvious place and the only one most people would look at. It was still heading for the GATING unit suite, silently, because bin/ci-unit-test.sh, bin/ci-integration-test.sh and bin/ci-build.sh deliberately do NOT inherit that default - a pom edit must not be able to change what gates, and their own headers say so. A tag the pom excludes and the wrappers do not simply runs. Nothing would have reported it. The lane would have appeared in the gating suite as extra passing tests, until the fixes for the four defects it asserts landed and turned the gate red on a PR that had nothing to do with it. So the three wrappers now exclude the tag, and QuarantinedAnnotationContractTest gains a check that every group the pom default excludes is excluded by all three - which is the general form, not a Lincheck special case. Verified by running bin/ci-unit-test.sh: green across every module, zero Lincheck classes selected. Two of that test's existing assertions had to change shape first. Both pinned the WHOLE excluded.groups literal, so any unrelated lane failed them with a message about quarantined tests gating - and the obvious repair, pasting the new literal in, leaves the test pinning a list nobody reasoned about. Membership is the contract each of them describes in its own failure message, and it still goes red the moment the quarantine tag is dropped, which is the failure they exist for. The mutation lane is the fifth place, and is excluded BY NAME rather than by tag. bin/ci-mutation-test.sh's own header records that whether pitest honours excludedGroups is unverified - it hinges on whether the property is interpolated in the raw Xpp3Dom pitest reads - and being wrong there is expensive rather than merely inaccurate: each Lincheck class runs a scheduler-controlled search taking seconds, and pitest re-runs its covering tests once per mutant. docs/testing.md gains the lane's section, alongside chaos and quarantine: what it is for, the four flags that must line up (each silent on its own), why it is stress-only over the product classes, and why the red control probe must never be deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
✅ 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 |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 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 |
Four `#1`s in the plan doc are verbatim Lincheck trace output - thread and object labels - which the gate reads as unqualified issue references. Wrapped in the gate's own in-file exempt markers, the mechanism its guidance prescribes for quoted source material. Caught late for a reason worth recording: this session's environment carried a NODE_OPTIONS preload pointing at a deleted temp file, so every node start died - and bin/check-issue-refs.sh exits 0 when its node engine dies mid-heredoc, so the standalone gate reported nothing while checking nothing. The PoC runs and the pre-PR check both "passed" that way. Same silent-false-green class as the BSD stat guard and the ASM-dead Lincheck transformer this branch itself documents; the standalone script's engine-death exit path should fail loudly, which is master-state and belongs on the hardening list rather than in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
…erge prep Ground truth for a fresh agent: what the branch is after the session that built it, the decisions that remain and whose they are, the cross-branch couplings that gh cannot show, and the traps already sprung so they are not re-learnt. Pointer- first per the handoff contract - the authoritative artifacts are the PR body, the dossier, and the commit bodies; this file only routes to them and carries what none of them can: the relationships between open branches. Wrapped in post-merge markers because a handoff is by nature a claim the merge falsifies - the file's own first instruction is its deletion in this PR at merge prep, which is what keeps it off master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
… contradicted the code A simplify pass over the Lincheck lane on #347. No behaviour changed: no bound moved, no operation removed, no assertion weakened. QuarantinedAnnotationContractTest parsed `<excluded.groups>` and `-Dexcluded.groups=` three times over, and the third copy had dropped the found-it assertion the first two carry. That is not cosmetic: `indexOf` returns -1 on a miss and the slice arithmetic around it still lands in bounds, so a gating wrapper that stopped passing the flag would have been mis-parsed silently rather than failing with the intended message. Now two helpers with one guard each, plus a GATING_SCRIPTS constant, and the guard fires from both call sites - verified by deleting the flag from bin/ci-build.sh and watching two tests go red on it. LincheckHarness.check() was a Runnable factory with exactly one caller, runExpectingViolation. Folding the options and the test class into that method's own signature removes the indirection and puts all five call sites on one line. The Runnable never carried meaning - it was the shape needed to hand a lambda to assertThrows. Four counts stated a number the code beside them contradicted, each wrong in the direction that hides work: - bin/lincheck-test.sh said THREE things have to line up, above a list of five. - The same header called the runtime "minutes"; the plan doc's own measurement is 26-29s for the whole lane, and the root pom's "minutes per class" was wrong the same way (0.2-5.0s per class, measured). - docs/testing.md said four flags, and credited the pom's excluded.groups with keeping the lane out of the gating suites. The wrappers hardcode their own lists and deliberately do not inherit it - which is the drift this PR added a check for, so the sentence contradicted the PR's own contribution. - The plan doc said adding a lane touches "four places" and then named a fifth in the same paragraph. Also dropped a `find` clause in the lane's zero-selected guard that could never match anything the clause beside it had not already matched. Verified locally on JDK 17.0.20-tem: bin/lincheck-test.sh selects all five Lincheck classes (8 tests, five surefire reports) and Lincheck returns five real violation verdicts plus the SuperHashCode probe's expected internal crash - so the lane instruments and explores rather than exiting 0 having selected nothing. bin/check-{shell-sigpipe,copyright-headers,issue-refs, file-refs,inflight-tags}.sh all pass. Not fixed here, and reported instead: WorkManagerLincheckTest's stress arm does not hit 100% at its committed bound on this hardware. Measured over eight single-class runs it missed once, and one run that did find the tear consumed 20.1s of a ~25s budget. The plan doc records 3/3 at this bound. Raising `iterations` is the honest lever and it changes a recorded measurement, so it is the owner's call rather than a simplify pass's.
…e claims pointed the wrong way Review findings from the pass over #347. Four fixes, each one a thing that was wrong rather than a thing that could be nicer; the judgement calls are reported, not applied. The guard is the one that mattered. `bin/lincheck-test.sh` counts `surefire-reports/TEST-*Lincheck*.xml` to prove the lane did not exit 0 having selected nothing - but it never cleans, so it counted reports from ANY previous run. Demonstrated rather than argued: with the five reports a normal run leaves on disk, `LINCHECK_TEST=NoSuchLincheckClass bin/lincheck-test.sh` counted 5 and would have passed. Deleting this run's matching reports before the run makes the count mean what the guard claims. Same command now prints "ZERO Lincheck classes ran" and exits 1, with the same five stale files present. A guard against false greens that cannot itself fail is the exact defect class this lane was built to catch, so it had no business shipping with one. Three statements pointed at something other than what they described: - `bin/ci-mutation-test.sh` said the lane is "excluded BY NAME above" - the `-DexcludedTestClasses` it means is below the comment, and a reader following the word "above" lands on the tag exclusions, which is the mechanism the sentence exists to say is NOT being used. - `docs/inflight/pr-347-handoff.md` listed "the PCMetrics arm" among the harnesses that invert when their fix PR lands. No such arm exists - the scenario that found that bug was retired as unreachable in production. Whoever went looking for it at merge prep would have found nothing and had to work out why. - The same file's heading carried a live `#347` self-reference outside its post-merge-checked block. `PR_NUMBER=347 bash bin/check-branch-self-reference.sh` fails on it; the gate runs in CI, so this was a red build waiting on the real PR branch, where the number resolves. The heading no longer names the PR at all, which is what makes it read correctly after the merge. Also asserted the closing `</excluded.groups>` tag in QuarantinedAnnotationContractTest's pom parser, the one guard the open tag already had - `indexOf` returns -1 on a miss and the slice arithmetic still lands in bounds, so a malformed pom would have been mis-parsed rather than reported. Verified: `bin/lincheck-test.sh` green, 5 classes, 8 tests, 35s; the zero-selection mutation above; QuarantinedAnnotationContractTest 12/12; and bin/check-{shell-sigpipe,copyright-headers,issue-refs, file-refs,inflight-tags,branch-self-reference}.sh all pass. Two reported P1s were run down and NOT fixed, because measurement said otherwise. Pitest does NOT break on core's new surefire `<argLine>@{argLine} ...`: a scoped `mutationCoverage` run logs "Replacing properties in argLine @{argLine}" from its own SurefireConfigConverter and completes the coverage phase on a live minion, so no `-DparseSurefireArgLine=false` is warranted. And PartitionStateLincheckTest's violation on this tree is the real offset tear (`commit(): resumeFrom=1 replay=[1, 2, 3]` racing `succeed(0)`), not the PCMetrics race - the discriminator is weak, but it is not currently passing for the wrong reason.
astubbs
commented
Aug 24, 2026
astubbs
left a comment
astubbs
left a comment
Owner
Author
There was a problem hiding this comment.
Looking good. My main question would be: how do we expand this as we move forward? Is there any other ground that is currently not covered by Linchik that we should add now that we know it is very valuable?
astubbs
marked this pull request as draft
August 24, 2026 22:54
astubbs
marked this pull request as ready for review
August 24, 2026 22:54
The lane's own false-green guard - counting surefire reports from any previous run, so a nonexistent test class 'passed' - was fixed on the branch. What is left needs the author. The headline is that WorkManagerLincheckTest misses its violation in 2 of 8 measured runs at the committed bound, against a plan doc recording 3/3. The pristine control is equally marginal, so it is not the review's edits. Raising iterations and re-measuring is the lever; a retry is not, and neither is loosening the assertion that IS the calibration. Also records the pitest argLine claim as disproven, so nobody re-raises it, and the handoff note's cross-branch obligation, so deleting that note at merge prep does not silently drop it.
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 2m 35s —— View job Review complete
SummaryThis is careful, well-evidenced work. I verified the load-bearing claims rather than taking them on faith:
Package placement (@astubbs's open question)Checked whether the Lincheck harnesses actually need
So: a dedicated sub-package (e.g. One confirmed, actionable finding
The connection is recorded - but only in Already tracked, not re-raising
On expanding coverage (@astubbs's question)Good candidates once the model-checking blockers clear or a stress arm becomes affordable:
Nothing here blocks merge on its own - the one actionable item is the dangling cross-reference above. |
… PCMetrics race Three review findings on #347, all in docs/inflight/, none touching the lane itself. The `inflight: tags` gate was red: `pr-347-review-findings.md` claimed "the simplify-and-review pass on this branch", a present-tense claim about a branch that stops existing at the merge. The intro is rewritten to read correctly afterwards and carries the `post-merge: checked` attestation the gate asks for. That is the whole of the CI failure. `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` read as an independent discovery and was not. #120 (the fork mirror of confluentinc#859) has reported the field for months, and #57 already carries the fix - `LinkedHashSet` with all nine mutation sites moved under a private `metersLock`, which closes the race as well as the leak. The note now says so, and says what is genuinely new: not the defect, the reproduced interleaving. Its "delete when" is repointed at #57 landing, with the correction that the `HashMap` stragglers in `WorkManager` are NOT in that PR's file list and still need their own pass. `pr-347-handoff.md` is deleted, which docs/inflight/AGENTS.md requires of a note whose own first line says to delete it in this PR - a "delete this when it merges" marker on master outlives the work and reads as live. Its one cross-branch obligation is executed rather than dropped: the Lincheck arm of the two-tool evaluation on #344's branch is recorded as done, with the plan doc as the evidence and the jcstress arm left open. Everything else it held was already stated where it gets looked up - the inversion contract and the red control in docs/testing.md, the five exclusion points in bin/lincheck-test.sh's header, Jabel and the model-checker blockers in the plan doc - so nothing was relocated twice.
…aise it to 1,000 `WorkManagerLincheckTest` asserts that Lincheck FINDS the checkpoint-3 tear, so a run that fails to find it is a flake - and this build has no retry, deliberately. The bound was committed at `iterations(200)` on the evidence of three green runs. Eight runs on one machine missed twice; eight on another missed none, which is a bound sitting on the edge rather than a difference between boxes. **Measured the per-iteration probability rather than the bound's outcome.** Three runs cannot separate a 10% miss rate from a 0% one, so the harness was deliberately starved to `iterations(25)`, where it hit 2 times in 8. That gives a per-iteration survival of 0.75^(1/25) = 0.9886, i.e. each iteration finds the tear with probability 1.14%, and that single number prices every bound: 200 misses 10% of the time, 400 misses 1%, 1,000 misses 0.001%. Eight two-second runs bought the whole curve. **Raised to 1,000, which costs nothing on the path that matters.** Lincheck stops at the first violation, so a run that finds the tear never reaches the extra iterations - measured 6.7-19.1s at 1,000 against 5.3-23.8s at 200, the same distribution, and eight whole-lane runs are 8/8 green with all five classes selected. The only run that gets longer is the one that was going to fail: either the flake this removes, or the designed inversion when #346 lands, which happens once and wants the certainty. One of those eight lane runs spent 32.2s in this arm before hitting - past what 200 iterations could have bought, so it is the miss, observed directly. The dated plan doc is corrected beside its original text rather than over it, per docs/citations.md, and `docs/testing.md` gains the general lever: under-budget a stress arm on purpose to price it, then raise `iterations` - never a retry, never a weaker assertion, both of which destroy the signal. docs/inflight/pr-347-review-findings.md drops the item, which is now closed rather than open.
…state package Review asked whether the lane should get its own package, since `state` is going to grow. Checked rather than assumed, and the answer is that two of the three real harnesses cannot move: `ShardManagerLincheckTest` drives `ShardManager.addWorkContainer` and `removeAnyShardEntriesReferencedFrom`, both package-private, and `PartitionStateLincheckTest` calls `PartitionState.createOffsetAndMetadata`, which is `protected`. That is not incidental. A harness models a seam, and the seams worth modelling are usually the ones the class does not expose - so moving the lane out means widening main-code visibility to suit a test, trading a real encapsulation boundary for a tidier package tree. Splitting only the two that could move is worse than either extreme, because the lane then has no single home for docs/testing.md to point at. Recorded in the doc rather than left in a review thread, so the question is not re-opened and answered differently next time.
…er is marginal Raising the WorkManager bound raises the obvious question about the other harnesses, and "their numbers look fine" is not an answer - three green runs is exactly the evidence that mis-sized the WorkManager bound in the first place. Probed both with the same deliberate under-budgeting: ShardManagerLincheckTest at 5 iterations instead of 50, PartitionStateLincheckTest at 30 instead of 300. Both hit 8 times in 8 at a TENTH of what they carry, which bounds the miss probability at the starved bound below 31% (95% confidence, zero failures in eight) and so below 1e-5 at the committed bounds. The variance says the same thing independently: 3.1-4.8s and 6.6-12.8s per run, against 4.7-32.2s for WorkManager across eight whole-lane runs. A search that finishes near the edge of its budget is what produces that spread, and only one harness had it. Neither bound is changed.
…ncheck next Review asked the question this PR had not answered: now that the tool is shown to work, what other ground should it cover? Answered as a ranked list in the note, and ranked by what each buys rather than by effort, because the lane's demonstrated value is finding seams nobody named - so widening what an existing harness explores beats adding a class built around a guess. Closing the encoder's range-top leg comes first, since HALF-FOUND is the only unclean verdict in the whole calibration. Then the unsynchronised counter maps, which the PoC already tripped over from a scenario aimed elsewhere and which the unopened sweep on `fix/concurrent-collection-sweep` will want a regression detector for. Then `ProcessingShard` and `RetryQueue`, the two state classes carrying scheduling state across the same two threads the existing harnesses already prove race. Model-checking arms cost nothing once `LincheckSuperHashCodeProbeTest` fires, which is why that tripwire is in the lane. Two negative answers matter as much: `@Validate` invariants are still the wrong next step for the reason section 4 of the plan doc gives - an invariant naming the retry queue is a hint - and the plain non-volatile `long` reads are ground Lincheck structurally cannot reach, because it reasons about interleavings and not about the memory model. That half stays jcstress's. Renamed from `pr-347-review-findings.md`. The note deliberately outlives the PR - its own text says to delete it when the items resolve, not when anything merges - so a filename naming an open PR is a filename that starts lying at the merge, which docs/inflight/AGENTS.md forbids. `test-` is the prefix the tag table gives for missing coverage and suite behaviour, and matches its existing `inflight-impact: test-debt`.
Owner
Author
Answered durably rather than only here — 1. Close the encoder's range-top leg first. It is the only unclean verdict in the whole calibration — HALF-FOUND, because the snapshot leg was exhibited and the two-reads-return-different-values leg was not, and is not expressible in the harness as committed. 2. The unsynchronised counter maps — the lane already tripped over them unprompted. 3. 4. Model-checking arms cost nothing once they become possible. Two negative answers, which matter as much:
The thing I would not do is widen the lane before it runs anywhere. Nothing invokes |
Owner
Author
|
@claude review this Head is Focus, in priority order:
Do not re-raise; all settled with evidence on this PR: the pitest |
|
Claude finished @astubbs's task in 5m 38s —— View job Review complete
1. The recalibration arithmetic — correct, but the confidence claimed outruns an 8-sample estimateThe arithmetic itself checks out exactly: The Bernoulli-independence framing is sound: each iteration draws a fresh random scenario and runs it across What's missing is an error bar. 6/8 is a tiny sample, and the exponential extrapolation to 1,000 iterations amplifies its uncertainty enormously. A Wilson 95% CI on the observed This is sharpened by contrast with the very next paragraph in the same plan doc: the None of this means 1,000 is unsafe — since raising 2. Raising
|
4 tasks done
…s, so measure 40 more Review was right, and the mistake was the same one this PR exists to catch, one level up. The recalibration justified `iterations(1_000)` with "1.14% per iteration, so one miss in a hundred thousand" - a bare point estimate from 8 starved runs, stated as though it were precise. Ran 24 more starved runs and the estimate moved by a factor of two: 2 hits in 8 became 14 in 32, so 1.14% per iteration became 2.33%. An 8-run figure was not enough to condemn the 200 bound, and it was not enough to bless its replacement either. That is exactly the reviewer's point, demonstrated rather than conceded. Fitted over all 48 single-class runs on this machine - 14 hits in 32 at `iterations(25)`, 8 of 8 at 200, 8 of 8 at 1,000 - by maximum likelihood: 2.33% per iteration, 95% profile-likelihood interval 1.38-3.72%. **The independent-trials model is now validated rather than assumed**, which was the other half of the review's question: the fit predicts 17.7 misses of 32 at 25 against 18 observed, and 0.07 of 8 at 200 against 0. The reviewer's objection - that many generated scenarios put both actors on `completeWork` and cannot tear at all - is true, and is already inside the measured marginal probability; agreement at three bounds spanning 40x is the evidence nothing else is going on. **The finding that outlives the bound: the hit rate is machine-dependent.** The 2-in-8 at 200 came from a different machine, and it is 3.4x slower to find the tear - 0.69% per iteration against 2.33%, with a likelihood-ratio test rejecting equality (LR 6.42 on 1 df, p = 0.011). So no single-machine calibration of a stress arm transfers, and every bound in this lane rests on one machine's runs. **The bound stays at 1,000, and the claim around it is now a range.** On this machine the miss rate there is below 1e-8%; on the slower machine's own estimate about 1 in 1,000. Only 8 runs exist from that machine, so the pessimistic end of its interval is roughly 1 in 14, and reaching 0.1% at that end would need about 2,700 iterations - recorded, not applied. Inflating a bound to cover the tail of an 8-sample estimate from a machine nobody re-measured is the same unfounded precision the review flagged; docs/inflight/test-lincheck-lane-open-items.md carries the open item and the ten minutes of measurement that would settle it. Also corrected: the note pointed at `bin/lincheck-test.sh`'s header for "the five exclusion points", but that header documents the five invocation FLAGS - a different list. The five gating-exclusion points live in the plan doc and are enforced by `QuarantinedAnnotationContractTest`. Costs are measured, not modelled: raising the bound is free on the hit path (mean 11.1s at 1,000 against 12.9s at 200, no increase), and the exhaust path runs 0.142s per iteration - from the 18 exhausted starved runs, cross-checked at 0.140s against a near-exhaust at 200.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…-reference gate this branch must now satisfy Master moved 55 commits under this branch since `0e21b903d`. What matters to a PR about quarantine evidence and a broker-level confluentinc#909 reproduction: - #351 (`test(core): the back-pressure test asserted an offset it had frozen`) diagnosed `OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessages...` as a test-design defect and removed its registry entry. **The registry is not empty** - `PCMetricsTest.metricsRegisterBinding` and `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` remain, the second owned by #57/#262 - so this PR's `Quarantined` rewording (evidence rather than a root cause) lands against two live entries, both of which already carry a diagnosis and so satisfy the stricter half of the rule either way. `bin/check-quarantine-registry.sh` confirms: 2 entries, consistent. - #347 (`test(core): a Lincheck lane`) edited `QuarantinedAnnotationContractTest`, the same file this PR rewords. Auto-merged cleanly; both edits are present, and the merged file's blank-attribute scan now reads "no quarantine without evidence". - **A self-reference gate arrived that this branch is not yet clean against**: `bin/check-branch-self-reference.sh` (`da91f3f61`) fails a `docs/inflight/` note that mentions THIS branch or THIS PR without an attestation that it reads correctly after the merge. It reports 17 findings here. The next commit settles them; they are not folded in, so the merge stays reviewable as a merge. - `docs/inflight/AGENTS.md` gained the issue-index carve-out (`docs/inflight/issue-index.md`) beside this PR's four-outcome retirement rule. Auto-merged, no overlap. - `docs/refactoring.md` gained entries from master's static-analysis work beside this PR's three. Auto-merged, no overlap. ONE CONFLICT, resolved as the union. `AGENTS.md`, the topic-doc routing table. Both sides edited the `docs/inflight/AGENTS.md` row's neighbourhood: this branch reworded that row (retiring a note is four outcomes, not just deletion) and master added a new row routing `parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.md` directly beneath it. Neither edit contradicts the other - one rewords a description, the other adds a destination - so both are kept, in master's ordering. Taking either side alone would have silently dropped a live route or reverted a rule this PR argues for. VERIFIED LOCALLY rather than deferred to CI: `bin/ci-unit-test.sh` on JDK 17 is BUILD SUCCESS over all eleven modules (core 411 tests, 0 failures, 8 skipped), and the two files this PR adds under `src/test-integration/` compile - `build-helper` adds that root at `generate-test-sources`, so `test` covers them. No test was loosened, no assertion weakened and no quarantine entry added to reconcile the two sides.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…cated it, so the lane mutated every module (#371) bin/ci-mutation-test.sh explained its -DexcludedTestClasses flag with a comment placed INSIDE the backslash continuation of its ./mvnw invocation. A backslash-newline splices the next line onto the command, so the `#` terminated it: Maven received everything up to -DtargetTests and nothing after, and each remaining line ran as a SEPARATE command. What the lane lost on every run: -pl parallel-consumer-core -am, so it was not PR-scoped at all and walked the whole reactor - ~24 minutes on core, then death in parallel-consumer-vertx with "No mutations found", a module no PR had touched, which is how this was noticed; -DexcludedTestClasses, the Lincheck harness exclusion the truncating comment was itself describing (#347 added it because those harnesses assert a violation IS found, so mutating against them is meaningless and slow); and -DjvmArgs, -DoutputFormats, both timeouts, -Dthreads and "$@". The orphaned argument then ran as a command - exit 127 - and its own `tee` overwrote the log the script parses for its verdict, while STATUS=${PIPESTATUS[0]} read that orphan pipeline rather than Maven's. The lane's report was computed from a clobbered log with a status from the wrong command. THE GUARD IS STRUCTURAL RATHER THAN A WARNING, ON REVIEW'S ARGUMENT. The first version hoisted the comment and left a "keep comments out of the invocation below" note; review pointed out that is a rule somebody has to remember, and that the argv arms only catch a truncation when the dropped flag happens to be one they assert. The arguments are now built as an array and passed as ./mvnw "${pit_args[@]}" "$@". There is no continuation to splice a `#` through, so reintroducing the defect requires converting back to a continuation first. Verified rather than assumed: a probe array carrying a comment mid-literal yields every element, exit 0, shellcheck clean. The Lincheck comment consequently returns to sitting immediately above the flag it explains - where it originally was, and what broke the lane. THE SELF-TEST COULD NOT HAVE CAUGHT IT, AND SAID SO IN ITS OWN HEADER. It opened "NO MAVEN RUNS HERE": every arm took the PIT_DRY_RUN_LOG branch, so nothing exercised the branch that invokes Maven and the suite stayed green through a broken invocation. Five argv arms now run the real branch against a stub mvnw that records its argv. Proven red against master's script - exactly those five fail while the pre-existing arms pass either way, which is the evidence they were never capable of catching this. A simplify pass then found a second-order bug in those new arms: the fixture wrote status="KILLED" while the subject only greps status='...', so the arms were silently landing on the exit-2 "scored NOTHING" verdict and passing anyway, because they assert argv and never the exit code. A fixture quietly disagreeing with the thing it tests - the same defect class this change exists to fix. Routed through the existing write_pit_report. NOT A NEW SHELLCHECK RULE, DELIBERATELY. ShellCheck names this exactly - SC2215, "This flag is used as a command name. Bad line break or missing [ .. ]?" - but it is a warning and bin/check-shell-lint.sh gates at error. Promoting the one code was rejected: the severity register has a one-knob rule because per-code lists grow, and this repo has already removed that shape from SpotBugs once. The incident is recorded instead as the argument for raising the floor tier-wide, which stays its own work. The array form's own SC2054 was earned away by quoting the comma-bearing element rather than suppressed, for the same reason. docs/inflight/static-shell-lint-severity-tiers.md carries that, plus two corrections the simplify pass found in it: its reproduce snippet had no shebang, so shellcheck --severity=error returned SC2148/exit 1 rather than the "silent, exit 0" printed beside it - the snippet disproved its own point - and raw counts were replaced by the commands that yield them. Defect-class sweep: shellcheck --include=SC2215 across bin/, .claude/hooks/ and .github/ matched only the site fixed here, and matches none now. Verified: bin/test-ci-mutation-test.sh 25 passed 0 failed; bin/check-all.sh 15 passed, 0 failed, 0 could not run; red control re-run after the array change. Note what this PR's own CI does NOT prove - it changes no Java, so the mutation lane correctly reports "nothing in scope" and exits before the invocation. That green tick is the scoping arm working, not evidence the fixed invocation runs; the evidence for that is behavioural and local.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…- poller death under KEY ordering (#345) Under KEY ordering, a record's success landing on the control thread while a rebalance revoked its partition could remove the now-empty shard between removeWorkFromShardFor's containsKey and its get. The sweep dereferenced null, and the NullPointerException flew out of the ConsumerRebalanceListener into consumer.poll on the broker-poll thread - the poller-death family. Narrow window, hot path, fatal consequence. THE FIX is the single-read getShard(key) Optional idiom every other access in the file already uses. One read, and both absent cases - removed before the sweep, and removed against the sweep's read - fall into the same guarded, trace-logged branch. Operating on the shard reference after it has left the map is benign: removing work from a detached shard mutates nothing shared, and removeShardIfEmpty re-reads through the same idiom. WHICH GUARD OWNS WHICH NULL, corrected here because this branch got it wrong twice and the next reader would use it to decide what is already covered. The containsKey test has been in this method since upstream's 2022 batching work (7e26075), two years before confluentinc#757 was filed. What confluentinc#757 - "NullPointerException on partitions revoked", closed - actually added is upstream PR 758's Objects.nonNull(removedWC) check one line lower, against retryQueue.remove(null), with ShardManagerTest.testAssignedQuickRevokeNPE for it; that test keeps a shard in the map throughout and never reaches the already-removed-shard branch. So this is the THIRD null on one revoke path - the same symptom family as confluentinc#757, a different null. CONTROLS, run rather than assumed. ShardManagerRevokeSweepNpeTest fires the real production shard removal between the two reads via a racing map seam: red on the unfixed tree with the exact predicted frame (NullPointerException at ProcessingShard.remove), green fixed, and red again when the main-code hunk alone is reverted with the test untouched - which also proves the rebuilt class reached the run. The control arm, the same production removal fired BEFORE the sweep instead of against its read, is green either way, so the failure belongs to the interleaving and not the fixture. Deleting the arm call fails the test's own seam-fired guard, so the seam cannot go silently dead. Core unit suite green. One deliberate test change, stated rather than hidden: the racing double now fires on the first read of the armed key rather than only on containsKey, because the fix deletes the call the original seam hung on. On fixed code the seam models the removal landing immediately after the single read, which the sweep must tolerate. The red side was re-verified identical after that generalisation. REJECTED: serialising the sweep against removeShardIfEmpty - a new lock on two hot paths to make a benign race impossible instead of harmless; and null-checking after the existing pair - keeps two reads and stays off the file's idiom. DEFECT-CLASS SWEEP, whole main tree. Class: two calls on a concurrently-mutated collection treated as one snapshot. Dismissed with reasons: PartitionStateManager.onPartitionsAssigned has the same shape but nothing ever removes from partitionStates - revoke PUTS a RemovedPartitionState - so its get cannot be null, an invariant nothing enforces; the counter HashMaps are check-then-put on the rebalance path, a duplicate registration at worst; OffsetMapCodecManager, PartitionState.polledOffsets and ProcessingShard.slowWork all operate on method-local collections; RetryQueue.contains and isRecordPreviouslyCompleted make one call with no paired get. Two live findings recorded rather than fixed here: ShardManager.removeShardIfEmpty is check-then-REMOVE across both threads and can drop a shard that just gained work, and ProcessingShard.addWorkContainer is get-then-put on the same shape, unreachable today only because registerWork is control-thread-only. ALSO IN THIS CHANGE, because the fix falsifies them the moment it lands. ShardManagerLincheckTest was the lane's calibration arm for this NPE and asserted that Lincheck FINDS it. The inversion #347 prescribed does NOT pass: on the fixed tree Lincheck still reports a violation with no NullPointerException in it - revokeSweep, then two concurrent addWork, which production cannot do because registration is control-thread-only. The harness now asserts the NPE is gone, which is the strongest claim the evidence supports, with the residual in docs/inflight/test-lincheck-lane-open-items.md. Six documents described MUI_CONTAINSKEY_BEFORE_GET naming this method in the present tense; the rule now reports zero there. And bin/lincheck-test.sh pinned its roster at 5 while 6 harnesses carry @tag("lincheck"), so the whole lane had been exiting 1 on master since RetryQueueLincheckTest landed - nothing invokes the lane, which is why nothing caught it. TWO SPOTBUGS FINDINGS ON THE NEW TEST DOUBLE, FIXED RATHER THAN SUPPRESSED. EQ_DOESNT_OVERRIDE_EQUALS and SE_NO_SERIALVERSIONID on RacingShardMap. Each rule had exactly one instance tree-wide and both were this change's, so a rule exclusion would have been targeted suppression dressed as policy, which spotbugs-exclude.xml's own header forbids. There is a genuine trilemma - declare neither and EQ_DOESNT_OVERRIDE_EQUALS fires, declare both and COM_PARENT_DELEGATED_CALL fires, declare equals alone and HE_EQUALS_NO_HASHCODE fires - so the file is not finding-free and this does not claim it is: one HE_EQUALS_NO_HASHCODE remains, naming a contract violation that provably is not present, since the inherited hashCode is AbstractMap's and consistent with the delegated equals. The zero-finding route is a class-scoped Match, deliberately not taken unilaterally at merge prep. A CHAOS RED ON THE WAY THROUGH, DIAGNOSED AND RECORDED RATHER THAN RE-RUN. ChaosRevokeUnderWorkCooperativeIT failed once on an intermediate head with a signature unrelated to this change: the ambient probe reported the poll thread BLOCKED on a monitor rather than waiting on a broker - the commitCommand AtomicBoolean held by the instance's own pc-control thread, reached through onPartitionsRevoked. That is the AB-BA revoke-path cycle whose write-up records its own verification status as Unproven and states that the fix addresses a cycle the test cannot reach while the block the test can reach is untouched. It is not this change's - the diff touches no locking and no AbstractParallelEoSStreamProcessor line - and the next head, a master merge with no main-code change, passed the same suite. Both outcomes one commit apart on one branch is the first direct intermittency datum the family ledger has, so it is filed as the third capture in docs/inflight/bug-857-family.md. The suite was never re-run to get it green and nothing was quarantined or loosened. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…-of-family findings their own notes The encoder fix in the preceding commit came out of a three-way audit of the commit path, the state managers, and shards/retry/metrics. This commit is that hunt's durable record, kept separate because it is not the fix's paperwork - most of it outlives the fix and some of it is about defects the fix does not touch. bug-torn-read-family.md states the family precisely (multiple reads of moving shared state within one logical operation, combined as though they were one consistent snapshot), carries the three surviving candidates with their control arms, and records the encoder seam's own residual - including that the reset window and the dirty-encode window are temporally disjoint by construction, which is the argument that makes this family's remaining exposure decidable without waiting on another PR. FOUR OUT-OF-FAMILY DEFECTS GET THEIR OWN NOTES, and that is the point of the split rather than tidiness. They were a paragraph inside a dossier whose own closing instruction is to delete it when the family's work lands - which would have taken four still-open defects with different owners and lifecycles with it - and a paragraph cannot be found by anyone listing the directory: async commit marked successful before broker ack; the unsynchronised cross-thread counter maps; the resetOffsetMapAndRemoveWork NPE on a partial-assignment path; and BrokerPollSystem's racy public pause API, which has zero main-code callers and so is latent rather than live. Each names its own consequence and points at an existing owner where one exists. The dossier's hard-coded dismissal tally is gone, replaced by the five dismissal shapes and a grep that re-derives the set. The audit's report artifacts are not durable and no command recomputed the number, so it would have become misleading as candidates were fixed - which is what docs/inflight/AGENTS.md's counts rule exists to prevent, and which fires hardest exactly when writing up a measurement just taken. The Lincheck/jcstress evaluation note is deleted rather than kept as a record of finished work: both arms executed and were adopted (#347, #348), so keeping it would have been the FIXED/DONE narrative that directory forbids, and without an inflight-state the session index kept presenting it as open. It never reached master, so there is no commit to point a history pointer at; both dated plans that cite it say so plainly and name the successor notes, which is what docs/citations.md prescribes when an item did not survive the move. Also recorded: the offset-decode test helper is copy-pasted seven times across two packages. The duplicate-code bot flagged one pair of them; the pair is not the finding. Extraction is a seven-file change that does not belong in a concurrency fix, so refactoring.md carries it with every site named, kept distinct from the racing-double unification the dossier tracks - that one is about the two Racing*State doubles, this is about the helper, and conflating them would send the next reader at the wrong mechanism. release-0.6.0.0.md gains the correctness campaign as announcement material: five concurrency defects sharing one root shape found in a week, two of them silent record loss present in every released 0.5.x line, each reproduced deterministically with control arms. #345 merged while this branch was being re-cut, so candidate 2 is marked FIXED rather than "fix before the release" - the dossier would otherwise have reached master already stale about a fix that is on it.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…-of-family findings their own notes The encoder fix in the preceding commit came out of a three-way audit of the commit path, the state managers, and shards/retry/metrics. This commit is that hunt's durable record, kept separate because it is not the fix's paperwork - most of it outlives the fix and some of it is about defects the fix does not touch. bug-torn-read-family.md states the family precisely (multiple reads of moving shared state within one logical operation, combined as though they were one consistent snapshot), carries the three surviving candidates with their control arms, and records the encoder seam's own residual - including that the reset window and the dirty-encode window are temporally disjoint by construction, which is the argument that makes this family's remaining exposure decidable without waiting on another PR. FOUR OUT-OF-FAMILY DEFECTS GET THEIR OWN NOTES, and that is the point of the split rather than tidiness. They were a paragraph inside a dossier whose own closing instruction is to delete it when the family's work lands - which would have taken four still-open defects with different owners and lifecycles with it - and a paragraph cannot be found by anyone listing the directory: async commit marked successful before broker ack; the unsynchronised cross-thread counter maps; the resetOffsetMapAndRemoveWork NPE on a partial-assignment path; and BrokerPollSystem's racy public pause API, which has zero main-code callers and so is latent rather than live. Each names its own consequence and points at an existing owner where one exists. The dossier's hard-coded dismissal tally is gone, replaced by the five dismissal shapes and a grep that re-derives the set. The audit's report artifacts are not durable and no command recomputed the number, so it would have become misleading as candidates were fixed - which is what docs/inflight/AGENTS.md's counts rule exists to prevent, and which fires hardest exactly when writing up a measurement just taken. The Lincheck/jcstress evaluation note is deleted rather than kept as a record of finished work: both arms executed and were adopted (#347, #348), so keeping it would have been the FIXED/DONE narrative that directory forbids, and without an inflight-state the session index kept presenting it as open. It never reached master, so there is no commit to point a history pointer at; both dated plans that cite it say so plainly and name the successor notes, which is what docs/citations.md prescribes when an item did not survive the move. Also recorded: the offset-decode test helper is copy-pasted seven times across two packages. The duplicate-code bot flagged one pair of them; the pair is not the finding. Extraction is a seven-file change that does not belong in a concurrency fix, so refactoring.md carries it with every site named, kept distinct from the racing-double unification the dossier tracks - that one is about the two Racing*State doubles, this is about the helper, and conflating them would send the next reader at the wrong mechanism. release-0.6.0.0.md gains the correctness campaign as announcement material: five concurrency defects sharing one root shape found in a week, two of them silent record loss present in every released 0.5.x line, each reproduced deterministically with control arms. #345 merged while this branch was being re-cut, so candidate 2 is marked FIXED rather than "fix before the release" - the dossier would otherwise have reached master already stale about a fix that is on it.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…its green is #347 committed this harness asserting the checkpoint-3 tear EXISTS, naming this fix as the one that inverts it. The previous head could not: #345's NullPointerException out of ShardManager.removeWorkFromShardFor was reached through the same revokeAndReassign operation, and `assertThat(report).contains("completeWork")` cannot tell one from the other. #345 has landed, so that blocker is gone and the lane was re-run rather than reasoned about, as docs/inflight/test-lincheck-lane-open-items.md requires. MEASURED, on this tree, at the committed iterations(1_000): - Seven valid runs, every one exhausting the whole bound with no violation, 120.3s-143.1s. A hit used to land in 9-13s, so none of these is a near miss. The designed-red arm therefore fails every run and cannot stay as it is. - CONTROL, the same sitting: a compiling mutant that re-resolves the partition state at handleFutureResult's two action sites - the checkpoint-3 tear put back, nothing else changed - hit ONCE IN SIX runs. The hit is exactly the calibrated signature: AssertionError at PartitionState.onSuccess, through PartitionStateManager.onSuccess and WorkManager.onSuccessResult out of handleFutureResult, completeWork racing revokeAndReassign, and no NullPointerException anywhere. So the harness still reaches its seam - it has not gone quiet. The honest reading of those two together, which the javadoc now carries: seven clean runs is WEAK evidence of absence when the positive control itself misses five runs in six. What pins this fix is WorkManagerStaleCheckDoubleLookupTest, which forces the interleaving deterministically; the Lincheck arm is a search running beside it, and a pass here must never be read as a proof. So: renamed to stressMustNotRediscoverTheCheckpointThreeTear and flipped to Lincheck's own linearizability check, the shape RetryQueueLincheckTest already uses. The bound is left exactly where it was measured - re-pricing needs the starve-and-count procedure the note prescribes, and neither raising nor lowering it on this handful of runs would be better founded than the estimate it replaces. THE COST, because it is now the lane's largest number. An inverted arm cannot stop at a first violation, so it always pays its whole bound: this arm is ~2 minutes every run and the whole lane went from 26-29s to 2m32s-2m37s (measured twice, green both times). bin/lincheck-test.sh's header and docs/testing.md said "well under a minute" and now say what it costs. THE OTHER HARNESSES, re-checked in the same sitting as that rule requires: - ShardManagerLincheckTest is unchanged by #335 landing - same counterexample the note records, revokeSweep(0) then addWork(0) against addWork(0). Nothing in the note's item 1 moves. - PartitionStateLincheckTest has now been run against a tree carrying #337's fix for the first time, and does NOT invert. It still reports a violation naming commit(), but the report is not stable between runs - mostly a value divergence between commit() and succeed(), once an ArrayIndexOutOfBoundsException out of two concurrent commit() calls printed with no frames. #344 remains its trigger; recorded as a sighting, not a finding. - RetryQueueLincheckTest and both toolchain controls green and unchanged - the model checker is still blocked on the Lombok callSuper defect, so the note's item 8 arms are still not free. docs/testing.md's "every harness asserts a bug EXISTS" bullet was already wrong before this change (RetryQueueLincheckTest never did, ShardManagerLincheckTest stopped when #345 landed); it now names the three shapes in the lane and why the third exists.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…, closing a stall and a wrong-commit route (#346) WorkManager.handleFutureResult's staleness checkpoint 3 validated one partitionStates.get(tp) lookup and acted on another. Nothing serialises the two against the broker-poll thread, so a revoke(+reassign) completing in the gap meant the check passed against the OLD state object while the actions ran against its replacement. Both harms were reproduced deterministically before any fix: - FAILURE PATH, a confluentinc#857-family permanent stall. The stale container was re-queued into the retry queue AFTER the revoke sweep had emptied its shard. removeStaleContainers reaches retry-queue entries only through shard contents, so nothing could remove the orphan again; once its retry delay elapsed, workIsWaitingToBeProcessed() read true forever with nothing assigned. - SUCCESS PATH, the only production route into a wrong commit. The completion landed on the freshly assigned state, tripping PartitionState.onSuccess's assert under -ea; without -ea it silently dirtied a bootstrap-phase state with a dead epoch's completion, which was the gate-opener making the bootstrap-reset commit tear reachable. THE FIX ACCEPTS THAT NO EPOCH CHECK HERE CAN BE ATOMIC WITH THE ACTIONS, and makes the actions safe by construction instead. The state is resolved ONCE and the staleness answer shares that reference with the success action, so a completion can only ever mutate the state object whose epoch matched the container; a concurrently revoked object is already unlinked from the live map, so the write is invisible to the commit path and the record is redelivered to the partition's next owner. At-least-once holds in every interleaving. The failure path additionally re-validates against the LIVE map immediately before the retry re-queue, because that action targets ShardManager structures the revoke sweep cleans and no captured PartitionState reference can protect; skipping the re-queue is the safe direction. PartitionStateManager.onSuccess/onFailure now take the caller-resolved state - WorkManager was their only production caller, and the single-arg overloads that did their own second lookup are reached from tests alone. HONEST RESIDUAL, FAILURE PATH ONLY. The re-queue re-validation is still check-then-act, shrunk from a whole-checkpoint gap a deterministic revoke reproducibly fit inside to a one-context-switch sliver. The class-level closure is the retryQueue sweep tracked in docs/inflight/bug-retry-queue-orphaned-by-inline-stale-removal.md. The success path has no residual. CONTROLS, run rather than assumed. WorkManagerStaleCheckDoubleLookupTest, five tests: both race arms red on the unfixed tree and green fixed, both serialized control arms green on both, so the failures belong to the interleaving and not the mutation. A mutant re-resolving the state at the action sites flips exactly the success arm and the bootstrap-gate probe red; deleting the re-queue re-validation flips exactly the failure arm red; deleting a test's arm call fails its raceHasFired guard, so the seam cannot go silently dead. THE LINCHECK INVERSION IS DISCHARGED, AND THE MEASUREMENT SAYS DO NOT TRUST IT ALONE. #347 committed WorkManagerLincheckTest asserting the tear EXISTS, to be inverted when this landed. On the fixed tree it missed all seven runs, each exhausting 1000 iterations - but a control carrying the defect re-introduced hit only once in six, producing exactly the checkpoint-3 signature and no NullPointerException now that #345 has landed. So the harness still reaches its seam and is simply a poor detector at this bound: seven clean runs is weak evidence of absence. The arm is inverted, the bound left where it was measured, and the javadoc and note both say the deterministic test is what pins this fix. The inverted arm cannot exit early, so the lane went from about 27s to about 2m35s - corrected in bin/lincheck-test.sh's header and docs/testing.md, which both claimed well under a minute. THE CONTRACT'S ONE-BUG-PER-HARNESS ASSUMPTION IS DEAD, and PartitionStateLincheckTest is now a VACUOUS GREEN. With #337 and #344 both landed it still passes contains("commit()") - on reports about neither tear it names, because the interleaving table names commit() whatever threw. Two shapes recur: an ArrayIndexOutOfBoundsException out of ArrayList.add, the plain-ArrayList defect #57 fixes, which the lane has now re-found from a second harness pointed elsewhere and with a frame this time; and PartitionState.onSuccess's assert reached from two parallel succeed operations, which production cannot perform because only the control thread completes work. Whether that second shape is artefact or defect is the same question already parked over ShardManagerLincheckTest's addWork, and it should be settled for both harnesses at once and AFTER #57 lands, since that PR removes one of the two shapes. Recorded rather than guessed at; the assertion is deliberately not re-pointed here. ALSO IN THIS CHANGE. The infer ratchet's gone-arm fired on WorkManager.onFailureResult; a three-tree control with pinned Infer v1.3.0 showed Pulse re-attributing an unchanged deref, with the identical shape in the neighbouring overload reported at neither head, so the line is deleted rather than a guard added. The same commit pins -encoding UTF-8 on the lane's hand-rolled javac, which had been inheriting the developer locale and made bin/infer-test.sh exit 2 under LANG=C. The duplicate-code thread is closed by extracting the shared fixture to ModelUtils.registerOneRecordAndTakeIt. WorkManager's pm and sm fields carry TODO(refactor) markers plus a docs/refactoring.md entry for the rename raised in review, not folded in here because both are @Getter(PUBLIC) and it would reach main, unit and integration sources far outside this seam. A CHAOS RED ON THE WAY THROUGH, DIAGNOSED AND RECORDED RATHER THAN RE-RUN. ChaosRevokeUnderWorkDrainIT failed once with the AB-BA revoke-path monitor block - the poll thread BLOCKED on the commitCommand AtomicBoolean held by its own pc-control thread, through onPartitionsRevoked - which is master-state and not this change's: the diff touches no locking and no AbstractParallelEoSStreamProcessor line, and the three earliest captures predate #335's merge with the identical monitor, holder and method pair. Filed as the fifth capture in docs/inflight/bug-857-family.md. The lag-stagnation observations in the same run were non-gating, so the CLASS2 demotion held. Nothing was quarantined, loosened, or re-run to hide it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
Learning capture from the Lincheck wiring incident on this PR: the roster guard EXPECTED_LINCHECK_CLASSES was correct when #347 pinned it at five, went stale at the very next roster addition (the RetryQueue fix added the sixth harness, no bump), stayed stale through this branch adding a seventh - and nothing noticed, because no CI workflow ever invoked the lane. Its first wired run caught two generations of drift at once. The rule: an execution path on every PR is part of a verification lane's definition of done; a guard is only as live as its most recent evaluation. Also: CONCEPTS.md gains the Lane entry (and the red-proof mismatched- pair paragraph returns to its own entry from under Starved run, where it had drifted), and the inflight lincheck note's now-resolved nothing-runs-the-lane section points at the wiring and the write-up. Every claim in the doc grounded against the tree and GitHub by an independent validation pass: guard value, workflow row, roster history commit by commit, PR states. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BHdUNhXgjvDwNJ3XYz5uM
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…quel ce-compound-refresh pass, scoped to a-check-that-reports-success-without-having-run.md: the doc's guidance stands and every cited artifact still exists, but its lincheck table row presented #347's roster-count fix as the resolution - and that count then sat stale through two harness additions because nothing executed the lane. The row and section 7 now carry the sequel (a-lane-nothing-runs-cannot-catch-its-own-guard-drifting.md, wired in by #392) so a reader arriving at the origin story finds the second incident and its execution-cadence rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BHdUNhXgjvDwNJ3XYz5uM
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…und dark (#404) Six Lincheck harnesses and bin/lincheck-test.sh landed with #347 and were never wired into CI: no workflow referenced them, and every CI script passed -Dexcluded.groups=...,lincheck, so the tag was excluded everywhere and included nowhere. A test that never runs is not a passing test, and nothing went red to say so for the lane's whole life. Running it found one arm dark. ShardManagerLincheckTest's stress arm is inverted - it asserts Lincheck FINDS a violation - and found nothing. Three hand-written controls said #336 was not the cause, and all three were wrong: each reverted one half of it onto today's tree, and the defect was in neither half alone. It was deciding the accounting from the pre-put read, which #336 removed wholesale by admitting to the population first and reading the outcome from the map. Settled by bisect with a FRESH worktree per commit, which turned out to matter: reusing one working copy reported 0 hits out of 10 at a commit that in fact fires 5 of 5, because Maven's incremental compilation left another commit's classes in place. A shared target/ hands you a clean, wrong bisect and nothing in the output says so. Re-run cleanly: fires 5/5 at #345, at confluentinc#905's hot-shard metric and at #373's claim compare-and-set; misses 5/5 at #336, the sole commit touching core's main sources in that interval. So the arm is flipped to stressFindsNoWayToBreakTheShardMap, asserting no violation, with #336 as what it regression-tests. The bound is unchanged at 50 x 5,000 because that is the budget the counterexample was FOUND at, which makes the zero worth more than one measured at a bound picked for cost: 0 in 250,000 invocations, and 0 in 2,500,000 at ten times it. #336's own commit message claims the lane was green "still finding the violation it is calibrated to find". Run on that tree it is RED, on this arm alone. The likely mechanism is in the same message - it is an adapted cherry-pick of fa4d1cf - and it is recorded in the javadoc rather than explained away. The matrix entry's gating argument is corrected too. It claimed the model checker explores interleavings deterministically, which is false of this lane: no model-checking arm over a product class runs at all, they are blocked on Lincheck rewriting a Lombok callSuper hashCode into a recursing self-call. What licenses gating is that the assert-no-violation arms need no hit to pass. The residual risk is named where it lives - PartitionStateLincheckTest and the toolchain probe still need a hit, and both fired on ubuntu-latest on every CI run of this PR. Method lesson in docs/solutions/best-practices/reverting-half-a-fix-is-not-a-control-2026-09-01.md.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…e Codex found A Codex review of c8193e5 returned five findings. All five hold; two are defects in this branch's own main code, one is a hole in the very gate this branch exists to build. **The guard did not do what it says.** `PollContextInternal#setProducingLock` refuses a second produce lock so a future call site cannot orphan the first. But both call sites in `ParallelEoSStreamProcessor#processAndProduceResults` read `context.setProducingLock(of(pm.beginProducing(context)))` - the second read hold is ALREADY TAKEN by the time the setter can refuse it, and the caller's only reference to it is the argument being passed in. Throwing kept the first hold and orphaned the second: read count 2, and the next commit's write-lock acquisition blocks forever. The guard caused the exact hang it advertises preventing while correctly reporting the misuse. It now releases the refused lock before throwing, which needed `ProducingLock#unlock()` widened from protected to public - its javadoc says why. Proved by control arm, not by reading: `PollContextInternalTest` is new, and removing only the `unlock()` line - one term, everything else identical - turns it red with `expected: 1 but was: 2`, the orphaned hold. Every other assertion in that test stays green through the control, which is why the hold COUNT is the one that carries it. **C4 was recorded PROVED on evidence that could not fail.** `crashAndReplay` waited until every result was visible, then separately waited for the source offset to reach its target. An implementation that published the records and committed the offset independently - with an arbitrarily wide window - satisfies both, because nothing ever looked at the pair. `TransactionalCrashReplayIT` now samples both on every poll and fails the moment the offset is seen to have landed without its records, which is the direction that matters: a consumer restarting from that offset skips work whose output nobody can see. Records-without-offset is the safe direction and stays unasserted, because the admin API's consumer-group view lags the transaction the records arrived in. The read ORDER is load-bearing and is the opposite of the intuitive one. Offset first, records second, so a violation means the offset had landed at t1 and the records were still missing at a strictly later t2 - and visibility is monotonic, so nothing in between can manufacture it. Read the other way round, records landing during the gap would report a correct implementation as violating. It is shared by every `crashAndReplay` caller rather than only C4's, because the pairing has to hold on every replay. **The coverage gate was blind to its own rot.** `aTerminallyFailedSendLeavesTheWholeTransactionInvisible` was annotated `@ProvesClaim(PRODUCE_MANY_ALL_OR_NONE)` - C7 only - while its own javadoc says "both C2 and C7 are recorded PROVED, and both rest on this". C2's other arms, the two in `TransactionalVisibilityIT`, never make a send FAIL. Deleting this method would therefore leave C2 reading PROVED on evidence that cannot reach the failure that once falsified it, and the gate could not see it because the annotation did not name C2. It names both now. **The same defect I had already fixed once, at the other end of the same change.** `cleanUpContext`'s new error log formatted the whole `PollContextInternal`, whose Lombok `toString` traverses the wrapped records and includes their keys and values. `setProducingLock`'s exception message deliberately avoids exactly that, four lines away in the file I wrote. Now logs `getOffsets()`. This is the "other instances of the same defect" sweep AGENTS.md asks for, and I did not run it - the reviewer did. **A generated doc contradicting the machine-checked register.** The README said both timeout guarantees are "attributed rather than re-proved ... neither is recorded as proved". The register has `PRODUCE_LOCK_TIMEOUT_RETRIES_RECORD` as PROVED with an observed control in `TransactionalEagerProcessingIT`. Corrected in `src/docs/README_TEMPLATE.adoc` and the generated `README.adoc`, which now reserve the attributed wording for the commit-lock claim alone. Also carried, at the request of the #404 follow-up and unrelated to the above: the lincheck matrix comment now says `LincheckToolchainProbeTest` may never be demoted to advisory. It is the lane's positive control, so an advisory probe makes every other zero in the lane uninterpretable - #347's unwired harnesses one layer along.
This was referenced Sep 5, 2026
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
This branch is docs-only - two markdown files differ from master - so neither red it drew can be its own. Both are recorded against the notes that own them, and reading each occurrence turned up a correction the note needed. INTEGRATION TESTS, on this PR's own head. The shape in ci-broker-container-exit-126-is-undiagnosable.md, exactly: one class fell slowly at the container-start timeout and every other broker class fell in milliseconds with NoClassDefFoundError on BrokerIntegrationTest. Codecov renders that as "20 Tests Failed"; it is one failure. What is new is that the cause was in the log all along. Testcontainers prints the failed container's own output at GenericContainer#tryStart, one line below the "Wait strategy failed" line the note's signature block quotes, and it reads "sh: /tmp/testcontainers_start.sh: Text file busy" - ETXTBSY, exec refused because the starter script was still open for writing. The container command waits for that script to EXIST and then executes it, so a file the daemon has created but not finished extracting is executable-shaped and not executable, and the shell reports the refusal as exit 126. A Testcontainers start race, widened by a busy runner; nothing in the product, the image or the Kafka configuration. Refetching #347's 2026-08-25 job, the run this note was written from, shows the identical two lines. So the note's premise under item 1 - "the container's stdout is nowhere in the job log" - was false of its own founding evidence. The instrument was fine; the triage stopped one line short. That correction is proposed in the note's vetting marker rather than applied, because the note's impact is misdirection and those are the owner's to close. CHAOS PAIN SUITE 4/4, on #495 - also docs-only, one README paragraph. ChaosChurnStormIT NO_PROGRESS at 96632/100000 for 30s against a 30s bound, seed 3717713223451201639. It goes in test-no-progress-window-may-not-transfer-to-w1.md as one appended row, with the part that makes it worth having: the fleet KEPT CONSUMING, reaching 99569 by the settle summary, so the outstanding count fell from 3368 to 431 - inside the TAIL_SLACK of 500. That is the "drains" branch of the deciding experiment the note states. It is the weak form and the row says so: no recovery diagnostic, so the counter compared is the ledger's rather than the probe's, and the conductor's churn ended 10s after the firing, so it is recovery-once-churn-stops. The bigger finding is that the deciding experiment had already been answered twice and this note never took delivery. test-857-churn-storm-async-stalls.md drained six for six on seed 9086872209853284830 with the diagnostic engaged, and its 2026-09-08 sighting drained seed 5650361238717170909 from 93487 to 101070/100000 at an outstanding count of 6513 - larger than every row in the table. That sighting says outright that this note owns the question; the pointer was written and nobody followed it. Proposed in the vetting marker for the same reason as above. RULED OUT, with a control arm rather than an argument. The six merges that landed on master today - #480, #487, #488, #491, #492 and #493 - are the obvious suspects for a chaos red, and #491 does touch ProgressProbe.java. Its diff does not touch the NO_PROGRESS path at all - it adds the UNCOMMITTED_COMPLETIONS detector, edits javadoc, and refactors the finding sink - and the same test PASSED on two heads that carry every one of those merges, four and six minutes either side of the failing run. A deterministic regression is excluded; a rate change is not, and one failure could not establish one. Nothing quarantined. The container fault has no test to quarantine and the exit-126 note says a re-run is the correct response there. The chaos firing has no rate that rule 1 would accept, and docs/quarantined-tests.md is empty - which is the state to preserve. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
The sighting claimed the plural on the strength of the pattern the note already records from #347. Checked it: only one earlier completed Integration Tests run exists on this branch, two days before, and it passed. The rest of that branch's runs were cancelled by the next push - which is the reading trap the flake register warns about, since a cancelled run is absent from every failure list and looks exactly like a pass. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
`bin/check-branch-self-reference.sh` flagged the sighting line for naming the PR that wrote it. The line is already written the way it will read after the merge - a merged PR number and a job id both outlive the branch, unlike a pre-squash SHA, which is why the note's existing #347 record cites heads by description rather than by hash - so this wraps it in the note's own `post-merge: checked` markers and drops the two phrasings that only made sense while the branch existed. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
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.
A Lincheck concurrency-testing lane, calibrated the only way that means anything: against a master that still contains every torn-read bug this month found by hand, asking whether Lincheck refinds them unaided - no seams named, no latches, no ordering.
The calibration verdict
WorkManager.handleFutureResultdouble lookup (#346)ShardManagercheck-then-get NPE (#345)PCMetrics.registeredMetersplainArrayListwritten from two threadsdocs/inflight/bug-pcmetrics-registered-meters-is-a-plain-arraylist.md, deletable when that PR merges.Whole lane:
bin/lincheck-test.sh, 26-29s including the build on one machine, 32-65s on another across 8 runs each; the spread is almost entirely theWorkManagerarm's search, which stops at the first violation and so varies with how quickly it lands. Excluded from every gating suite (verified against all five exclusion points - one of which was initially missed, which is its own commit).The design decisions a reviewer should check
assertThrows(LincheckAssertionError), because the fixes have not merged yet. When fix(core) astubbs#121: commit the offset the encoded payload was written against (confluentinc#894) #337/fix(core): the offset encoder read its range top after its snapshot, silently completing records (confluentinc#894 sibling) #344/fix(core): revoke sweep NPE when a shard is removed against its read - poller death under KEY ordering #345/fix(core): staleness checkpoint 3 checks and acts on one state lookup, closing a stall and a wrong-commit route #346 land, the lane goes red by design and the harnesses flip into regression detectors over the whole operation set. That inversion is the point of adopting it; it is also why the lane must stay non-gating.LincheckToolchainProbeTestis a deliberately-broken red control, and it earned its place immediately: the first run reported a clean pass against code that cannot survive two threads, becausewiremock-jre8pins ASM 9.4, Lincheck needs 9.6+, and its transformer failed per-class while reporting success. Every calibration verdict would have read "not found". A detector without a red control is a green lamp wired to nothing.@EqualsAndHashCode(callSuper = true)interaction (settled by a two-arm control experiment that doubles as an upstream-fix tripwire) and by replay non-determinism on the commit path (micrometer, andparallelStream()in twoPartitionStateaccessors - worth attention in its own right). Two tripwire tests keep the model checker's viability under watch rather than dropping it.Full findings, cost tables and knobs:
docs/plans/2026-08-25-001-test-lincheck-poc-plan.md.What review found: the lane's own false-green guard could not fail
bin/lincheck-test.shasserts that Lincheck classes actually ran - the guard against the "exits 0 having selected nothing" trap this lane exists to avoid. It counted surefire reports from any previous run, so with five stale reports present,LINCHECK_TEST=NoSuchLincheckClass bin/lincheck-test.shcounted 5 and passed. Demonstrated, then fixed by deleting matching reports first; the same command now printsZERO Lincheck classes ranand exits 1.Also fixed: the branch-self-reference gate was red in waiting on the handoff note's heading, and the exclusion-contract test parsed
excluded.groupsthree times over with the third copy missing the found-it assertion the other two carry -indexOfreturns -1 and the slice arithmetic still lands in bounds, so a wrapper that stopped passing the flag would have been mis-parsed silently. Four counts that contradicted the code beside them were corrected, including the "minutes per class" figure against a measured 26-29s for the whole lane.Proof the lane runs rather than being skipped: 8 tests, 5 Lincheck classes, 35s, exit 0, with five
Invalid execution resultsverdicts and the SuperHashCode probe's expected internal crash. A gating suite with the exclusions applied selects zero Lincheck classes (Tests run: 0, BUILD SUCCESS), and all five exclusion points were re-verified. Three mutations of the exclusion wiring each go red with the right message.Run down and rejected on evidence: the claim that core's
<argLine>@{argLine} ${lincheck.jvm.args}</argLine>feeds a literal@{argLine}to pitest's minion JVMs and silently breaks the mutation lane is false. A scopedmutationCoveragerun scored 35 mutants across 496 tests with zero minion errors; pitest's ownSurefireConfigConverterlogsReplacing properties in argLineand resolves it.Closed since: the WorkManager bound was a latent flake, and it is now priced rather than guessed
WorkManagerLincheckTestasserts that Lincheck finds the checkpoint-3 tear, so a run that fails to find it is a flake - and this build has no retry, deliberately. The bound shipped atiterations(200)on the evidence of three green runs. Eight runs on one machine missed twice; eight on another missed none. That is a bound on the edge, not a difference between boxes.Three runs cannot separate a 10% miss rate from a 0% one, so the follow-up measured the per-iteration probability instead of the bound's outcome, by deliberately starving the harness. The first attempt at that used 8 runs and got a materially wrong answer — review caught it, and re-measuring confirmed it:
iterations(25)An 8-run figure was not enough to condemn the 200 bound, and not enough to bless its replacement either — the same mistake this PR exists to catch, one level up.
The bound is now fitted by maximum likelihood over all 48 single-class runs (14/32 at 25, 8/8 at 200, 8/8 at 1,000): 2.33% per iteration, 95% profile-likelihood interval 1.38–3.72%. The independent-trials model is validated, not assumed — it predicts 17.7 misses of 32 at 25 against 18 observed, and 0.07 of 8 at 200 against 0, i.e. it holds across a 40x span of bounds.
The hit rate is machine-dependent, and that outlives the bound. The 2-in-8 at 200 came from a different machine, which is 3.4x slower to find the tear (0.69%/iteration against 2.33%); a likelihood-ratio test rejects equality (LR 6.42 on 1 df, p = 0.011). No single-machine calibration of a stress arm transfers.
1,000 stays, and the claim around it is a range rather than a point. On this machine the miss rate there is below 1e-8%; on the slower machine's own estimate, about 1 in 1,000. Only 8 runs exist from that machine, so the pessimistic end of its interval is roughly 1 in 14, and reaching 0.1% there would need ~2,700 iterations. That is recorded, not applied — inflating a bound to cover the tail of an 8-sample estimate from a machine nobody re-measured is unfounded precision pointing the other way. The open-items note carries the ~10 minutes of measurement that would settle it.
Raising
iterationsis free on the path that matters, measured: mean 11.1s at 1,000 against 12.9s at 200 — no increase, because Lincheck stops at the first violation. Only the run that was going to fail gets longer, at a measured 0.142s per iteration (18 exhausted starved runs, cross-checked at 0.140s against a near-exhaust at 200): ~33s at 200, ~2.4 min at 1,000. 8 whole-lane runs are 8/8 green with all five classes selected each time.The other two harnesses were probed the same way and are not marginal: starved to a tenth of their bounds (
ShardManager5 instead of 50,PartitionState30 instead of 300) both hit 8 in 8. Neither bound is changed. They inherit the same one-machine caveat.Still open, tracked in
docs/inflight/test-lincheck-lane-open-items.md18a61321bnow requires every red control to carry a green near-miss arm, andLincheckToolchainProbeTesthas none. It also omits the.actorsBefore(0)/.actorsAfter(0)that all four other harnesses set, so the init prefix can destroy its own fixture. Both change a control this PR calls settled, so they are left for the author.ProcessingShard/RetryQueue, plus the two things deliberately not next.Chaos Pain Suiteis red, and it is not this PRDiagnosed rather than assumed, because "unrelated" is exactly what a real regression also looks like:
src/mainfile at all. The diff from the head where chaos passed (257c4173a) to the head where it failed is six markdown files plus a comment and oneiterationsvalue in a@Tag("lincheck")class — andlincheckis excluded from the chaos suite's group filter.CLASS2_STALLat 154s against a 150s bound on 4 partitions, andNO_PROGRESSwith the fleet at 97896/100000 for 30s against a 30s bound. TheCLASS2_STALLprobe's own message says the bound "is a TIMING measurement, not a correctness verdict".Performance (optional)finished 03:10:36 onhighcpu-2; the failing tests ran 03:15:05–03:19:37 onhighcpu-6.Chaos Pain Suiteis not in master's required-checks ruleset. Every one of the 21 required checks passes exceptreview: human LGTM.It went red at three consecutive heads (3 of 4 chaos runs on this branch), on fresh seeds each time — so it is not one bad seed replaying. The third run added
ZOMBIE_MEMBERonChaosKeyOrderIT. In the same windowIntegration Testsalso failed on this box with the Kafka broker container exiting 126 — it never started, so no test executed — and passed on a straight re-run.Taken together that points at the runner or at master, not at any branch, and is worth attention independent of this PR. Recorded as the fourteenth sighting in
docs/inflight/bug-857-family.mdwith all four seeds, since the log artifacts expire in 14 days and that ledger's standing argument is that the seed is the asset. No seed has been replayed, so it is recorded rather than diagnosed. Nothing was loosened, weakened, or quarantined; the one re-run was of a container that never started.Checklist
AGENTS.mdforbids per-PR changelog entries; the commit bodies carry it.docs/testing.md, anddocs/inflight/test-lincheck-lane-open-items.md.docs/inflight/pr-347-handoff.mdis deleted, as its own first line required; its one cross-branch obligation - ticking the Lincheck arm of the evaluation note that lives on fix(core): the offset encoder read its range top after its snapshot, silently completing records (confluentinc#894 sibling) #344's branch - is executed and now recorded in the open-items note instead of in a file scheduled for deletion.bin/ci-unit-test.shselects zero Lincheck classes.Merge-order note: the operator intends the tooling to merge before the fix PRs. If a fix PR merges first, this lane goes red on master - that is the designed inversion arriving early, and the remedy is flipping the affected harness to assert-no-failure, not reverting the fix.