Repository navigation
ci(lincheck): run the lane that has never run, and flip the arm it found dark - #404
Conversation
THE LANE EXISTED AND NOTHING EXECUTED IT Six Lincheck test classes and bin/lincheck-test.sh landed with #347 and were never wired into CI. No workflow referenced lincheck at all, and every CI script passed -Dexcluded.groups=...,lincheck, so the tag was excluded everywhere and included nowhere. That is the case AGENTS.md names directly - a test that never runs is not a passing test, and nothing goes red to tell you - and it held for the lane's whole life. The runner script's own header said "non-gating and opt-in", which read as a deliberate choice rather than as a description of a lane nobody had connected. Corrected in the same commit: a runner that describes a lane nobody runs is how that stays invisible. GATING, FOR THE REASON THE CHAOS ENTRY IS A Lincheck violation is a real finding rather than a timing wobble - the model checker explores interleavings deterministically instead of sampling them, so a reported violation reproduces. That is the same argument the Chaos Pain Suite entry makes for not being optional, and the lane costs about 2m30s. THE RISK, NAMED WHERE SOMEBODY WILL HIT IT The STRESS arms, not the model-checking ones. Their hit rate is machine-dependent - 3.4x across two machines, measured in docs/inflight/test-lincheck-lane-open-items.md - and their bounds were priced on the machine that wrote them rather than on a hosted runner. So if this entry flakes, that is the cause, and the fix is repricing the bound in the test. NOT a retry: this repo removed surefire reruns precisely because they retried failures into green and hid three flakes. Demote to advisory before masking anything.
…e diagnosed, not hidden The lane in this PR is RED on the machine that verified it, deliberately. Handing it over green would have meant either repricing a bound the lane's own notes say transfers to no other machine, or inverting an arm on a hypothesis that turns out to be wrong. WHAT THE EVIDENCE SAYS ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear misses eight times out of eight at its committed bound - consistent, so not a flake. bin/lincheck-test.sh exits 1, so the failure propagates and there is no silent-green problem underneath it. AND WHAT LOOKED TRUE AND IS NOT The obvious reading is that #336 fixed the check-then-act this arm's counterexample turns on - it rewrote 182 lines of ProcessingShard, the code now says "ADMIT FIRST ... never the read above", and it edited the lane's note in the same commit, which is why that note reads as current while describing a defect that commit addressed. Every part of that correlation holds. The control refutes it anyway. Restoring the genuine pre-astubbs#336 shape - get, branch on the read, put inside each branch - produces no violation either. If the fix were what silenced the arm, reintroducing the defect would restore the counterexample. So the harness is not exercising the seam on this machine, which is the OTHER branch of the message the harness itself prints. Recorded because the next agent will form the same hypothesis in the first ten minutes, and the control costs an hour to build. An invalid first control is recorded too: it kept the put atomic and only moved the accounting, so it never reintroduced a check-then-act at all - reading the real pre-astubbs#336 method before writing a control is the lesson, and git show 3e668a4^:<path> is how. WHAT IT HANDS OVER The recommendation is to gate the model-checking arms and run the stress arms advisory, because the commit's own gating argument - the model checker explores interleavings deterministically rather than sampling them, so a violation reproduces - is true of model checking and false of stress. It never covered the arm that failed. Repricing the bound and adding a retry are both named as the wrong moves, with the reasons, since both are what an agent under pressure to go green reaches for.
…e either, so the bound is not the cause The handoff note left one experiment running: whether a larger bound makes ShardManagerLincheckTest's stress arm find its violation. It does not - iterations(500) against a committed iterations(50), on the clean tree, missed both runs. That is worth more than a repeat of the earlier misses, because it closes the reading the lane's own notes make most attractive. The 3.4x machine-dependence recorded for these stress arms predicts that a bound priced on a fast machine misses on a slow one AND that spending more search finds it anyway. Ten times the budget finding nothing is inconsistent with that, and so is the control: reintroducing the defect changed nothing either. Neither more searching nor a present bug produces a violation on this box, which means the harness is not reaching the seam here at all rather than reaching it rarely. So the next agent is pointed at "why is it unreachable" rather than at a bound to tune - with the candidates named, cheapest first, and with the cheapest move of all stated plainly: confirm the arm still fires SOMEWHERE, on the machine that wrote it or in CI, before spending anything on the harness. Two of the three candidates only exist because #335 and #373 also rewrote ProcessingShard after the harness was written, which is exactly the thing the lane's note warns is not derivable by reasoning: re-run the lane, never reason about it.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
🧪🔒 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 |
🟢 Throughput — OK
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) How this is derived, and what it cannot tell youBy conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 3 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. |
|
…s there, the arm still does not The note handed over one cheap next move: confirm the Lincheck stress arm fires *anywhere* before spending anything on the harness. CI ran the lane on this PR's head for the first time and answered it. `ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear` missed again, with the identical message, on `ubuntu-latest` - four cores, the opposite end of the range from the 32-core box the earlier evidence came from. The decisive part is not the miss but what passed beside it. On the same runner in the same JVM, `LincheckToolchainProbeTest`'s stress arm and `PartitionStateLincheckTest`'s stress arm both found their violations. The toolchain probe exists to answer exactly that question: Lincheck's stress strategy works there, the JDK is adequate, and the `-Plincheck` flags are landing. So two of the three surviving candidates are ruled out - a JDK or Lincheck version difference, and a host property such as core count - and one is left standing: this arm's operation set no longer reaching the seam it was written against, after #335, #336 and #373 each rewrote `ProcessingShard`. That also re-reads the earlier control. Reintroducing the check-then-act produced no violation either, which was ambiguous under the three-candidate list and is corroborating under this one: if the operations no longer reach the seam, restoring the defect at the seam changes nothing, which is what was measured. Records the cost correction too - 7m42s hosted against ~2m50s locally, 364s of it `WorkManagerLincheckTest`. The matrix entry's `timeout: 20` still covers it; the `~2m30s` in its comment does not. Evidence: run 33511221453, job 99867204560, head 58c8c7c.
…he lane gate green The lane was wired up in this PR and landed RED on one arm: `ShardManagerLincheckTest`, an inverted harness asserting that Lincheck FINDS a violation, found nothing. The handoff this branch carried recorded that as "#336 REFUTED - do not re-run it". That verdict was wrong, and this commit replaces it with the experiment that settles it. Bisected rather than reasoned about. The harness has not changed since #345 and Lincheck is pinned at 3.7 in both trees, so only product code varies. Running the unchanged harness against each tree: #345 (0112c70) FIRES confluentinc#905 hot-shard metric FIRES #373 claim compare-and-set FIRES #336 (3e668a4) misses #336 is the sole commit touching core's main sources between the last hit and the first miss, so attribution is exact rather than merely bisected-to. The counterexample it removed is the one the lane's note recorded months ago - `revokeSweep(0)` in the prefix, then `addWork(0)` against `addWork(0)` - and #336 removed it by admitting to the population before the put and reading the outcome from the map instead of from the earlier read. Three hand-written controls had said otherwise, and all three were wrong: one kept the put atomic and never reintroduced a check-then-act at all, one restored the map's check-then-act alone, one restored the counter's bare increment alone. The defect was in neither half - it was deciding the accounting from the pre-put read, which #336 removed wholesale. A control assembled by hand tests the shape you believed the defect had, so its negative says nothing about the commit. The durable form of that lesson is in docs/solutions/best-practices/reverting-half-a-fix-is-not-a-control-2026-09-01.md. So the arm gets the disposition the lane's inversion contract prescribes: it is now `stressFindsNoWayToBreakTheShardMap`, asserting no violation, and #336 is what it regression-tests. The bound is deliberately unchanged - 50 x 5,000 is the budget the counterexample was FOUND at, which makes this 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, on a 32-core box with Temurin 17.0.20+8 and Lincheck 3.7. The whole lane is now green locally - 9 tests, 2m45s, all six harnesses selected - so the leg lands GATING as the matrix entry already declared, and the quarantine-or-advisory question the handoff left open does not arise. The matrix entry's own gating argument is corrected too. It claimed the model checker explores interleavings deterministically, which is true of model checking and false of this lane: no model-checking arm over a product class runs here at all, they are blocked on Lincheck rewriting a Lombok `callSuper` hashCode into a recursing self-call. What licenses gating is that the three assert-no-violation arms need no hit to pass. The residual flake risk is named where it actually lives - the still-inverted arms, `PartitionStateLincheckTest` and the toolchain probe, which do need a hit and whose rate was measured 3.4x apart across two machines. Also corrects the entry's ~2m30s estimate to the 7m42s measured on ubuntu-latest, and retires the handoff note: its open question is answered, and what outlives it has moved to the solutions write-up and to the lane's owning note.
|
@claude review this Focus, since the PR changed shape after it was opened: it was a handover PR carrying a deliberately Second thing worth an adversarial look: the matrix entry now argues gating is licensed because the |
|
Claude finished @astubbs's task in 5m 39s —— View job Review complete
Note up front: this sandbox's clone is shallow and network fetch is blocked here ( 1. The bisect attribution is in direct tension with #336's own commit message — this needs to be resolved before the flip standsThe flip commit (
But
Read plainly, "the latter" = the Lincheck lane, and "the violation it is calibrated to find" is exactly what This isn't just a wording quibble against a throwaway line: it's the PR author's own contemporaneous (2026-08-27) evidence about the exact commit this PR now uses as the bisection endpoint, and it's not addressed anywhere in the new commits (not in the javadoc, not in What makes this more than a stale-comment explanation: this exact lane already has a documented, measured machine-dependence finding — Concretely, before trusting the flip: re-run the bisection experiment (checkout 2. Gating with two known-risky, still-inverted arms left in the same job is a real, unaddressed tensionThe updated matrix comment ( The problem: all six harnesses run in one Given Smaller items
Bottom line: don't merge the flip as-is. Finding #1 is the one that matters — it's a direct, checkable contradiction between this PR's central causal claim and the cited commit's own words, and it happens to line up with a machine-dependence effect this same lane has already measured. Resolve it (re-run the bisection on a second machine, or explain why the commit-message line doesn't mean what it plainly says) before treating |
…he stale target/ that nearly reversed it Review asked for the #336 attribution to be defended against #336's own commit message, which says the Lincheck lane was green "still finding the violation it is calibrated to find". Replicating the bisect to answer that turned up a bigger problem with how it had been run. The first bisect checked out each commit inside ONE worktree. Re-running the same commits in a second worktree - one built at a different commit first - reported 0 hits out of 10 at a commit the first pass recorded as firing. Same commit, same machine, same harness; only the inherited `target/` differed. Maven's incremental compilation had left another commit's classes in place, so the harness ran against a tree that never existed, and nothing in the output says so. It fails silently in both directions, which is what makes it worth a section of its own rather than a footnote. Re-run with a FRESH worktree per commit, so no two trees can share class output: #345 (0112c70) fires 5/5 confluentinc#905 hot-shard metric fires 5/5 #373 (8000926) fires 5/5 #336 (3e668a4) misses 5/5 The attribution holds, now on numbers that are reproducible rather than single-shot. Each miss pays its full bound, so that row is 0 hits in 1,250,000 invocations. On the commit message: run on #336's own tree the whole lane is RED, on this arm alone, with the other five green. The message's own line - "adapted cherry-pick of fa4d1cf" - is the likely mechanism, the lane it reports on plausibly being the pre-adaptation one. Recorded in the javadoc rather than explained away: if that line is ever shown to describe the merged tree, the flip is wrong. On the machine-dependence alternative the review raised: the miss is confirmed on a second machine. Before the flip this arm also found nothing on ubuntu-latest, four cores against 32, while the toolchain probe's stress arm fired on that same runner. The lane's measured 3.4x cross-machine variance is a rate effect, and a rate effect does not turn 15 of 15 hits into 0 across one commit on one box, nor agree across two machines that far apart.
|
Finding #1 — addressed, and it was worth raising: chasing it found a real defect in how I ran the The bisect was unsound, and I nearly published a wrong number. The first pass checked out each Re-run with a fresh worktree per commit, in
The attribution holds, now on reproducible numbers rather than one shot each. On the commit message. I can't explain it away, so it is recorded in the javadoc instead: run on On machine-dependence as the alternative. The miss is confirmed on a second machine: before the Finding #2 — not addressed, and I'm not going to decide it unilaterally. You're right that the two |
astubbs
changed the title
|
Resolving the gating question the review raised (finding #2), before merge. Decision: gate everything as-is. The premise — that the inverted arms' bounds had never met a Two things worth recording rather than carving out:
If an inverted arm does flake, the response is |
Brings #404 (the Lincheck lane that had never run), whose maven.yml comment the next commit amends. Clean merge, no conflicts.
…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.
Four commits, none touching this branch's files. Read before merging, per AGENTS.md: 1001c69 ci: PR CI comments say what they mean, and survive their own failures (#407) 000de5d ci(lincheck): run the lane that has never run, and flip the arm it found dark (#404) b36ad94 fix(core): stop transactional batches failing and reprocessing records that succeeded (#257) 625a8c1 docs(solutions) #183: a defect matches a report on direction and on the reporter's build The package-rename rule is still live on master, and does not bite here: this branch touches no Java and no package paths, and the sweep grep finds io.confluent only in the rename, copyright and mutation scripts that carry the pattern as data. Worth recording rather than passing over: #404 lands the 2026-09-01 sightings into docs/inflight/test-load-tightness-flakes.md, so the committedOffsetRemoved flake this PR's CI hit is now tracked on master rather than only on sixteen unmerged branches. That was the worked example in docs/inflight-tool.md - the example stands as the record of what the tool found on the day, and the ledger has since caught up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTX8obQMsjs9kq2rkpU5cZ
1 participant
What this does
Six Lincheck test classes and
bin/lincheck-test.shlanded with #347 andwere 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. That isthe case
AGENTS.mdnames directly — a test that never runs is not a passing test, and nothing goesred to tell you — and it held for the lane's whole life.
This adds a
Lincheckleg to thetestmatrix in.github/workflows/maven.yml, corrects the runnerscript's header (it described the lane as "non-gating and opt-in" when the truth was that nobody had
connected it), and fixes the one arm that turned out to be dark.
The arm that was red, and what actually silenced it
Running the lane for the first time failed on exactly one arm:
ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear. It is inverted — it asserts thatLincheck finds a violation, and Lincheck found nothing.
The suspect was #336, which had rewritten
ProcessingShard#addWorkContainer.Three hand-written controls were built to test that, over two sessions, and all three came back
negative — which was written up as "#336 refuted, the harness has lost its
seam".
That verdict was wrong. Settled by bisect instead: the harness has not changed since
#345 and Lincheck is pinned at 3.7 in every tree, so only product code
varies.
0112c703e)revokeSweep(0)→addWork(0)‖addWork(0)3e668a448)#336 is the sole commit touching core's main sources between the last hit
and the first miss, so attribution is exact rather than merely bisected-to.
Why three controls in a row missed it. One kept the put atomic and never reintroduced a
check-then-act at all; one restored the map's check-then-act alone; one restored the counter's bare
increment alone. The defect was in neither half — it was deciding the accounting from the pre-put
read, which #336 removed wholesale. A control assembled by hand tests the
shape you believed the defect had, so its negative says nothing about the commit. That lesson is
written up in
docs/solutions/best-practices/reverting-half-a-fix-is-not-a-control-2026-09-01.md.So the arm gets flipped, and the lane gates green
It is now
stressFindsNoWayToBreakTheShardMap, asserting no violation, with#336 as what it regression-tests — the disposition the lane's inversion
contract prescribes. The bound is deliberately unchanged: 50 × 5,000 is the budget the counterexample
was found at, which makes this 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, on a 32-core box with Temurin
17.0.20+8 and Lincheck 3.7.
Whole lane green locally: 9 tests, 2m45s, all six harnesses selected.
Two corrections to the matrix entry
true of model checking, false of this lane, where no model-checking arm over a product class runs
at all (blocked on Lincheck rewriting a Lombok
callSuperhashCodeinto a recursing self-call).What licenses gating is that the three assert-no-violation arms need no hit to pass. The residual
flake risk is now named where it lives: the still-inverted arms —
PartitionStateLincheckTestandthe toolchain probe — which do need a hit, at a rate measured 3.4× apart across two machines.
~2m30scorrected to the 7m42s measured onubuntu-latest.Notes
The handoff note this branch carried is retired: its open question is answered, and what outlives it
moved to the solutions write-up and to
docs/inflight/test-lincheck-lane-open-items.md, which ownsthe lane. The artefact-or-defect question over the harness's parallel
addWork— productionregisters work from the broker-poll thread alone — is unchanged and stays there.
Checklist
docs/inflight/test-lincheck-lane-open-items.mdcorrected;bin/lincheck-test.sh's header corrected