Repository navigation
fix(core): revoke sweep NPE when a shard is removed against its read - poller death under KEY ordering - #345
Merged
Conversation
…containsKey and get kills the poller Settles candidate 2 of the torn-read family dossier: REPRODUCED, deterministically. ShardManager.removeWorkFromShardFor checks containsKey, then calls get, and dereferences the result unconditionally. Under KEY ordering the control thread's onSuccess -> removeShardIfEmpty removes empty shards from the same map, while the revoke sweep runs on the broker-poll thread inside consumer.poll's rebalance callback - nothing serialises the two. A removal landing between the two adjacent reads makes get return null and the next line throw: NullPointerException: Cannot invoke ProcessingShard.remove(long) because "shard" is null out of the ConsumerRebalanceListener into consumer.poll - the poller-death family. The window sits inside the guard added for confluentinc#757 (testAssignedQuickRevokeNPE), which closed the shard-already-gone case but left the gap between its own two reads. Every other access in the file already uses the single-read getShard(key) Optional idiom. The reproduction (ShardManagerRevokeSweepNpeTest): - revokeSweepMustSurviveAShardRemovedBetweenItsTwoReads - RED on master, by design: the shard map is swapped for a double whose containsKey fires the interfering mutation at the seam, and the mutation is the real production path (sm.onSuccess on the shard's last container), not a hand-rolled map edit. 4/4 deterministic across runs; no load or repetition needed. - sameShardRemovalBeforeTheSweepIsHandledGracefully - GREEN: the identical mutation fired before the sweep instead of between the reads takes the guarded already-removed branch. Same magnitude, different position, so the failure is the interleaving's, not the fixture's. Seam guard (raceFired, set at firing time) proven by mutation: with the arm deleted, the armed test goes red at the guard (1/1) instead of silently green. NOTE this branch carries reproductions, not fixes - the armed test is EXPECTED RED here and becomes the regression test of the fix branch. Prediction record: prediction 1 (NPE at ProcessingShard.remove) held exactly; prediction 2 (early-fire control green) held; refutation prediction 3 was falsified - no hidden serialisation exists between the sweep and onSuccess. (cherry picked from commit 06e3c464b6c2c67ffe898ae89ca5b1b3b6b53dec)
…inst its read - single-read getShard replaces the torn containsKey/get pair Closes candidate 2 of the torn-read family dossier (docs/inflight/bug-torn-read-family.md, carried by the encoder-tear branch): the last check-then-get pair in ShardManager, reproduced deterministically by the previous commit's test. What a user saw: under KEY ordering, a record's success landing on the control thread while a rebalance revokes its partition could remove the now-empty shard between removeWorkFromShardFor's containsKey and its get. The sweep then dereferenced null and the NPE flew out of the ConsumerRebalanceListener into consumer.poll on the broker-poll thread - the poller-death family. Narrow window, hot path, fatal consequence. The guard added for confluentinc#757 closed the shard-already-gone case but sat on two reads of a moving map, leaving this gap between them. The fix is the single-read getShard(key) Optional idiom every other access in the file already uses: one read, and both absent cases - already removed before the sweep (the 757 case) and removed against the sweep's read (this one) - fall into the same guarded trace-logged branch. Operating on the shard reference after it left the map is benign: removing work from a detached shard mutates nothing shared, and removeShardIfEmpty re-reads through the same Optional idiom. Controls, all on this branch: - reproduction test RED on the unfixed tree (NPE at ProcessingShard.remove, the dossier's exact frame), control arm green - 4/4 deterministic on the hunt branch, 2/2 red-side runs here - fix applied: green; fix reverted (main code only, test untouched): red again 1/1 - the flip on a main-code-only change also proves the rebuilt class reached the run - test's seam-fired guard proven by mutation: arm deleted, armed test red at the guard 1/1 - full default unit suite green across all modules Also in this commit, inseparable from the fix's shape: the reproduction test's racing double now fires on the first read of the armed key (containsKey OR get) instead of only containsKey - the fix deletes the containsKey call the original seam hung on, and without the generalisation the seam-fired guard would go red on correct code. On the fixed code the seam models the removal landing immediately after the single read, which the sweep must (and now does) tolerate. Red side re-verified identical after the generalisation. Rejected alternatives: serialising the sweep against removeShardIfEmpty (a new lock on two hot paths to make a benign race impossible instead of harmless); null-checking after the existing pair (keeps two reads and stays off the file's idiom).
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)
|
|
🧪🔒 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 |
This was referenced Aug 24, 2026
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
Register item 13. bin/racerd-test.sh runs RacerD over parallel-consumer-core's main code: opt-in, non-gating, following the Lincheck lane's precedent. THE EARLIER "BLOCKED" VERDICT WAS WRONG, in a way worth recording. Infer's Maven integration runs the build under a JDK of its own choosing while this project requires 17 - correctly diagnosed with a two-arm control - and that was written up as RacerD being blocked. It was not blocked. The documented alternative, capturing javac directly and sidestepping the wrapper that picks the JDK, was simply never tried. A tool that fails through one integration has not failed. Thirteen findings from 73 files, in three groups. PCMetrics.registeredMeters, four: the plain ArrayList mutated from two threads that the Lincheck PoC turned up unprompted, already tracked and fixed by #57. RacerD names all four mutating entry points statically, with no harness. RetryQueue, seven: this.unique read via Map.size() and isEmpty() racing with writes, and RetryQueueIterator.closed read against write. The Lincheck lane's own open-items note ranks RetryQueue as the next thing to model and says it is not modelled at all, and docs/refactoring.md separately names RetryQueue.closed as a current offender. One corroboration plus six findings beyond it, in ground nothing else covers. AbstractParallelEoSStreamProcessor.lastCommitTime, two, and this one is NEW. A plain Instant written in one method and read unsynchronised by isTimeToCommitNow(). Checked before claiming it: no inflight note, no refactoring.md entry, and the only mention anywhere is a code excerpt inside an unrelated solutions write-up. It sits on the commit-timing path in a repo that tracks commit-timeout flakes. Recorded, not fixed. It does NOT find the four named torn-read races, and that is the wrong expectation rather than a failure: #345 and #346 are check-then-act on a map and #337 and #344 are two-read value divergence, while RacerD models unguarded access to shared state. Different class. fb-contrib reaches the first pair. No CI lane. Infer is a 220 MB toolchain published for linux-x86_64 and osx-arm64 only, so its home is the self-hosted highcpu runner where it persists between runs - the same argument the chaos and mutation sweeps already make. The script exits 2 for "cannot run" rather than 0, so a missing toolchain can never read as "no races", and that arm is verified.
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
`ShardManager.processingShards` is shared, unlocked, between the broker-poll thread (which adds and removes shards) and the controller thread (which reads how much work is queued). Both sides rely on the reference never moving - it is a ConcurrentHashMap and identity was never expected to change. Swapping it is strictly worse than any torn read of its contents: a reader holding the old reference keeps accounting work against a map nothing will ever drain. Two doors led to a replacement and nothing but review attention held either shut: the Lombok `@Setter(AccessLevel.PACKAGE)`, whose only caller anywhere is `ShardManagerTest`, and a direct assignment inside `ShardManager` that would bypass the setter entirely. `ShardMapIsNeverReplacedArchTest` shuts both, main code only - tests may still install a pre-populated map, which is the setter's whole purpose. Mutation evidence, three arms, each predicted before it was run: - a production caller of the setter added to `ShardManager` -> rule 1 red, rule 2 GREEN (the assignment is inside the setter, so only the call rule should see it); - a direct `this.processingShards = ...` in a non-constructor method -> rule 2 red, rule 1 GREEN. Same magnitude, different position: each arm flips exactly one rule; - both rules pointed at names that do not exist -> both rules PASS, having asserted nothing, and both reflective name-pins go red. That is the false-green the pins exist for, and it is now observed rather than assumed. Reverted after each; green on arrival and green after. The rule the build-hardening register asked for is NOT writable, which is the more durable finding and is why that entry now reads as corrected rather than merely done. It assumed `getShard(key)` was the idiom everywhere bar the one pair that NPE'd. It is not: exactly one main-code access is `getShard`, nearly all the rest are bulk operations over `values()` / `keySet()` that `getShard` cannot express, and the keyed ones that remain include both writes. So "no direct access outside the accessor" is red on arrival, and the only way to green it is an allowlist of the methods already there - a frozen baseline wearing a source-code disguise, which is the trap `static-archunit-main-code-rules.md` exists to refuse. Underneath that sits the granularity limit: ArchUnit sees a field ACCESS, never what is invoked on the value that access loaded, so it could not have told a keyed lookup from a bulk read even in principle. The check-then-get seam is fb-contrib's MUI_CONTAINSKEY_BEFORE_GET, already Tier 1 in the SpotBugs registry, and #345 - which fixes it - is still open, so that seam is on master today. Also recorded: four `// visible for testing` accessors on `RetryQueue` and `ShardManager` have zero callers in the whole tree, so an accessor rule over them would pass vacuously forever. Queued for deletion in docs/refactoring.md rather than policed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
The registry leads on MUI_CONTAINSKEY_BEFORE_GET naming ShardManager's check-then-get seam, which is the strongest single argument for adopting fb-contrib. #345 is open, not merged - the seam is on master, which is precisely why the detector sees it - and when that PR lands the finding reports zero. That does not weaken the case for the detector. It means the paragraph stops describing a live finding and starts describing history, and a reader would go hunting for a race that no longer exists. Recorded at the paragraph so whoever notices the count change rewords it instead of doubting the tool. Also merges the ArchUnit work, which established that register entry 8 was wrong twice over: it asserted #345 was already merged, and its stated rule was not expressible, because ArchUnit models a field ACCESS and never what is invoked on the value that access loaded.
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
… seeing nothing #356 flagged that this work's framing - "no static analysis this repo runs can see the torn-read class" - is measurably too strong. It is, and it is over-strong in exactly the way this PR already caught itself being once. WHAT WAS ACTUALLY MEASURED, AND WHAT WAS CLAIMED. The measurement was stock SpotBugs at `effort=Max`, which was the only static analysis the repo ran that day, reporting nothing on any of the four calibration targets - including the nine-line textbook probe. The claim generalised that one configuration to the whole tool class. #356 turns fb-contrib on and measured its `MUI_CONTAINSKEY_BEFORE_GET` naming `ShardManager.removeWorkFromShardFor` statically, in seconds, with no harness: #345's seam, and one of the four targets. Verified here independently rather than taken on report - that method is a literal `processingShards.containsKey(shardKey)` followed by `processingShards.get(shardKey)` with the result dereferenced, nothing held across the pair, so the detector has a real shape to match and the finding is credible on inspection. So the honest width is three of four, not four of four. #346's seam is a stale-check rather than `containsKey`-before-`get`, and the two value-divergence torn reads are not what a check-then-act detector looks for. What survives unqualified is the claim that actually carries the lane: the seam fb-contrib names is one somebody had already found by hand, whereas the defect Lincheck turned up was on nobody's list. WHY THE SAME SHAPE TWICE. This PR's own headline result is that the WorkManager hit rate is machine-dependent - 2.33% per iteration here against 0.69% on Antony's machine, a likelihood-ratio test rejecting equality at p = 0.011 - so no single-machine calibration transfers. A rate measured on one machine is not the rate; a family one analyser cannot see is not a family no analyser can see. Both are a sample of one generalised to a population, and the second slipped through while the first was being corrected. WHERE EACH EDIT LANDS. `docs/testing.md` owns the Lincheck lane and is durable reference, so the corrected width is stated there in place - it would otherwise be flatly false on master the moment #356 lands, and #356 `depends on` this PR, so this one lands first. `docs/plans/` is a dated record whose claims may not be rewritten (`docs/citations.md`), so the plan doc keeps its claim and gains a flag beside it naming `docs/testing.md` as the owner of the corrected version. ALSO FIXED: THE PLAN DOC CONTRADICTED ITSELF ON THE MACHINE QUESTION. Its interim correction paragraph read the 2-misses-in-8 / 0-in-8 split as "the signature of a bound sitting on the edge rather than of a difference between machines", and then instructed the reader to read two tables through that conclusion - which the 48-run fit later in the same section refutes with the likelihood-ratio test. A reader who stopped at the first complete-looking statement got the finding backwards. Restated as what the split actually shows: ambiguous alone, and settled by the fit as both a marginal bound and a real difference between machines. No measurement changed. No test or assertion changed, and the bound stays at 1,000 - the slower machine's refinement remains open and Antony's call (`docs/inflight/test-lincheck-lane-open-items.md`). Lincheck lane re-run on this tree to confirm the master merge left it intact: all five classes selected, 8 tests, fresh surefire reports, green - which for these harnesses means Lincheck still finds every seam. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…unaided (#347) Adds Lincheck to `parallel-consumer-core` and calibrates it the only way that means anything: against a master that still contained every torn-read bug found by hand this month, asking whether Lincheck refinds them **unaided** - no seams named, no latches, no ordering imposed. It refound four, one of them off-list. The `WorkManager.handleFutureResult` double lookup, the `ShardManager` check-then-get NPE and the confluentinc#894 two-read all fell out in seconds. The encoder tear came back HALF-FOUND and is not claimed: the model checker placed a switch between the two reads once in eight attempts but never showed the range-top divergence. And `PCMetrics.registeredMeters` - a plain `ArrayList` written from two threads - was found **unprompted**, having been on nobody's list. That last one is the result that matters, because it is evidence the lane finds seams nobody named rather than only confirming seams somebody did. Green currently means "Lincheck found the bug": each harness is `assertThrows(LincheckAssertionError)` because the fixes have not merged. When #337/#344/#345/#346 land the lane goes red **by design**, and the remedy is flipping the affected harness to assert-no-failure - never reverting a fix, never loosening a bound. That inversion is the point of adopting it, and it is why the lane must stay non-gating. `LincheckToolchainProbeTest` is 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, because `wiremock-jre8` pins ASM 9.4 while 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. **Two false-green shapes in the lane's own tooling were found and closed.** `bin/lincheck-test.sh` counted surefire reports from any previous run, so with stale reports present a nonexistent test class "passed"; it now deletes matching reports first. Then the guard that caught zero classes could not catch a *short* roster - three of five harnesses could silently stop being selected and the lane still printed a healthy number. It now pins the exact roster, and the number has to be hand-written: a derived count cannot work, because a dropped tag removes the class from both the run and the expectation, which cancel to green. **A bound is a claim about one machine.** The committed `iterations(1_000)` was originally justified by a point estimate from eight runs; forty more, fitted by maximum likelihood over 48, moved it by a factor of two to 2.33%/iteration (95% profile-likelihood interval 1.38-3.72%), and the independent-trials model is validated by prediction rather than assumed - 17.7 misses predicted of 32 at a starved `iterations(25)` against 18 observed. The finding that outlives the number: the hit rate is **machine-dependent**, 0.69%/iteration on a second machine against 2.33% here, with a likelihood-ratio test rejecting equality at p = 0.011. So no single-machine calibration of a stress arm transfers, and pricing that properly gates adding further harnesses. The bound for the slower machine is deliberately left unset rather than guessed - inflating it to cover the tail of an eight-sample estimate would be the same unfounded precision pointing the other way. Model checking is unavailable for now, established rather than assumed: it is blocked by a Lombok `@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. Two tripwire tests keep its viability under watch instead of dropping it. Jabel's 17-source/8-bytecode split is a non-issue, also established. Corrections made rather than defended, both of the same shape - generalising from one measurement. A claim that no static analysis can see this defect class was measured with stock SpotBugs alone; fb-contrib's `MUI_CONTAINSKEY_BEFORE_GET` does reach `ShardManager.removeWorkFromShardFor`, so the claim is narrowed. And the ranked "where to point it next" led with `ProcessingShard`'s drifting counter on the strength of a clamp commented `// in case of possible race condition` - #336 measured it and there is no race, both paths being single-threaded conditional mismatches. Recorded rather than quietly dropped, because the wrong inference was made twice by reading a comment instead of measuring. The lane is excluded from every gating suite at all five points, verified rather than asserted: a gating run selects zero Lincheck classes. Where to point it next, led by the selection rule rather than a list, is in `docs/inflight/test-lincheck-lane-open-items.md`.
…e-on-racing-shard-removal
…ole lane exits 1
`bin/lincheck-test.sh` asserts the exact number of harness classes selected, precisely so
a harness that silently stops being selected cannot pass as a healthy run. It expected 5.
There have been 6 tagged `@Tag("lincheck")` classes since `RetryQueueLincheckTest` arrived
with the RetryQueue off-lock fix, and that commit did not bump the pin - so
`bin/lincheck-test.sh` with no `LINCHECK_TEST` filter has exited 1 on master ever since,
after a green Maven run, printing "the lane selected the wrong roster".
Nothing caught it because nothing runs the lane: it is excluded from every gating suite
by design and no workflow invokes it, which
`docs/inflight/test-lincheck-lane-open-items.md` already records as an open item under
"Nothing runs the lane, so the tripwire it promises cannot fire". A guard whose only
caller is a human is exactly the guard that rots.
The hand-maintained number is deliberate and stays hand-maintained - the script's own
header explains why deriving it from a `grep` for the tag would make the guard blind to a
dropped tag, since the same absence removes the class from both the run and the
expectation. The friction IS the check; this is that friction being paid, late.
Verified: `bin/lincheck-test.sh` now exits 0, "Lincheck report files: 6", 9 tests across
6 classes green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
…at fixing it unmasked `ShardManagerLincheckTest` was the Lincheck lane's calibration arm for this branch's defect: it asserted that Lincheck FINDS a violation and that the report contains `NullPointerException`. This branch fixes that NPE, so the harness goes red - which the lane's own commit predicted, naming this PR, and prescribed flipping to assert-no-failure. THE PRESCRIBED FLIP DOES NOT PASS, measured rather than assumed. On the fixed tree Lincheck still reports `= Invalid execution results =`, with no `NullPointerException` anywhere in it. Controlled both ways in one sitting: revert only the main-code hunk and the NPE counterexample comes back and the harness is green (BUILD SUCCESS); restore it and the new counterexample appears (BUILD FAILURE on the `contains` assertion). So the second violation was always there, masked because Lincheck stops at the first thing it finds. Asserting no-failure would have been red on arrival; asserting nothing would have been the vacuous green this lane exists to prevent. The counterexample is `revokeSweep(0)` in the prefix, then `addWork(0)` against `addWork(0)` in parallel - the `entries.get` / `entries.put` + `incrementAndGet` check-then-act in `ProcessingShard.addWorkContainer`, i.e. the lane's own "concurrent collection plus a derived counter" signature. NOT claimed as a product defect: production registers work from the broker-poll thread alone, so two concurrent `addWork` calls are not an interleaving the library can take today, and the harness declares an operation set wider than the real thread model. That is a question about the harness's contract, and it is recorded in `docs/inflight/test-lincheck-lane-open-items.md` rather than answered here. So the harness asserts the strongest claim the evidence supports - that no report over these operations mentions `NullPointerException` again - which makes it a real regression detector for this fix instead of a calibration arm with nothing left to calibrate. The method is renamed to say so; nothing outside the file cited the old name. The note also narrows, rather than overturns, that file's existing ruling that `ProcessingShard.availableWorkContainerCnt` is not a Lincheck target. That ruling rests on #336 measuring the counter DRIFT as single-threaded conditional mismatches and it stands; what is new is a machine-produced interleaving over the same field, arriving unaided from a harness pointed somewhere else, which is a different claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
…ll belongs to which fix This branch's own javadoc and code comment said the torn `containsKey`/`get` pair "sits inside the guard that was itself added for confluentinc#757". It does not, and the record is worth getting right because the next reader will use it to decide what is already covered. WHAT THE HISTORY SAYS. `git log -S` on the two guards: - the `containsKey` test has been in `removeWorkFromShardFor` since upstream's 2022-01-15 batching work (7e26075), two years before confluentinc#757 was filed; - confluentinc#757 - "NullPointerException on partitions revoked", CLOSED - was fixed by upstream PR 758 (dc0b921, 2024-05-08), which added `Objects.nonNull(removedWC)` one line further down, against `retryQueue.remove(null)`, plus `ShardManagerTest.testAssignedQuickRevokeNPE` for it. That test installs a shard for the key, so `containsKey` is TRUE throughout: it never reaches the already-removed-shard branch at all. So the method carries three nulls on one revoke path - already-removed WORK (757), already-removed SHARD (2022), and the shard that was present for `containsKey` and gone by `get` (this branch) - and only the first two were previously guarded. Same symptom family as confluentinc#757, a different null, which is why the reference is still worth citing; it just is not the guard this one sat inside. Comments and javadoc only - no behaviour change, and both tests are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
Four documents and two build files describe `MUI_CONTAINSKEY_BEFORE_GET` naming `ShardManager.removeWorkFromShardFor` in the present tense. This branch deletes that seam, so from the moment it lands the rule reports ZERO there and every one of those sentences sends a reader hunting for a race that no longer exists. `docs/inflight/static-spotbugs-rule-registry.md` asked for this in writing - "this finding has a shelf life, and the note should not outlive it... somebody should reword it" - which is the instruction this commit is answering. The wording now says what the detector found, that it found it unaided and statically in seconds, and that reporting zero is the expected state rather than the rule having gone quiet. The case for keeping fb-contrib on is unchanged and is restated as forward-looking: the next instance of the shape is what it is there to catch, and `LincheckToolchainProbeTest` remains the red control proving it still fires. `bin/check-branch-self-reference.sh` is what forces this at the one moment it is cheap: the three `docs/inflight/` notes naming #345 all fail it after the merge, because a note is a claim about NOW and this merge falsifies theirs. Each rewritten passage carries `post-merge: checked` markers; the gate is green. The other four sites are not gated by anything and would have rotted silently: `pom.xml`'s fb-contrib justification, `config/spotbugs-exclude.xml`'s class-scoped suppression rationale, `docs/testing.md`'s Lincheck-lane section, and the registry's own "the one CLASS-scoped suppression" paragraph. The dated plan under `docs/plans/` is left alone deliberately - it records a measurement taken on a tree that had the seam, and `docs/citations.md` owns why those are not rewritten to match today's code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
… all four fixes it binds #345 and #346 each measured their own harness against a fixed tree and each wrote the result into this note independently. The two accounts disagreed: the one written from WorkManagerLincheckTest prescribed inverting ShardManagerLincheckTest alongside it, which the other had already measured as red. Two adjacent sections, one of them falsified by the other. Replaced by a single section that both branches carry byte-identically, so whichever lands first the second merges it cleanly rather than conflicting - and there is one place to correct when the remaining two fixes are measured. What it now records that neither half did alone: the contract from #347 assumed one bug per harness, and that assumption is what failed. A harness pointed at one seam explores the others, so it can stop finding its own bug without becoming quiet - ShardManagerLincheckTest surfaces a second violation with no NPE in it, WorkManagerLincheckTest passes while actually detecting #345's NPE through an assertion that cannot tell the two apart. The falsified prescription is kept as the argument for the rule rather than deleted: a harness's next assertion is not derivable from its own diff. It also names the half nobody has measured. PartitionStateLincheckTest is the harness #337 and #344 invert, it has never run against a tree carrying either fix, and its javadoc still promises an inversion - so the obligation is on whoever lands any of the four to re-run the whole lane, not only the harness their own PR is named for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
… not fight over it #345 and #346 each had to rewrite this paragraph - it named both PRs in the present tense as unfixed, which bin/check-branch-self-reference.sh fails on each branch - and they wrote two different replacements for the same four sentences. Whichever landed second would have conflicted. The kept wording is the one that does not need rewriting again: the blind spot is the CLASS, not the instances, so which of the four are still open does not move what RacerD can see. Naming the four by their fixing PRs keeps the citation valid after each lands, which is the property the previous phrasing lacked.
7 tasks done
…e-on-racing-shard-removal
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The draft response listed the four sibling fixes as `(#344, #345, #346, #349)` - qualified once and then elided. `bin/check-issue-refs.sh` reads each reference on its own, so the three elided ones are bare numbers below the threshold and it failed the branch. Writing all four out in full is also what the drafts need on their own terms: they are posted to GitHub, where the short `astubbs#NN` form does not autolink either.
… not the poller The shared Lincheck section argued that two concurrent addWork calls are unreachable because "production registers work from the broker-poll thread alone". The conclusion holds and the reason for it holds - exactly one thread registers - but the thread is misnamed. Traced: BrokerPollSystem calls pc.registerWork, which only enqueues onto workMailBox. The control thread reaches wm.registerWork after workMailBox.drainTo, inside processWorkCompleteMailBox. So registration is the control thread's, and the poller never touches ShardManager on that path. It matters beyond pedantry because the paragraph exists to tell a future harness author which operation set to model. A harness built against the wrong thread would constrain the wrong operations and reproduce nothing. The correction is left visible in the text rather than silently swapped, so a reader who met the old claim elsewhere can see it was retired - the same sentence is in commit 3ba30db's body, which is not rewritable now that reviews are open. Applied byte-identically on #345 and #346, so the two branches keep merging cleanly in either order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… and two notes had to read post-merge Three repairs the merge with master made necessary, none of them behaviour changes. THE INVERSION THIS BRANCH WAS INSTRUCTED TO DO IS BLOCKED, AND THAT IS MEASURED. #347's commit body says the Lincheck lane goes red by design when this fix lands and "the remedy is flipping the affected harness to assert-no-failure". Run against the merged tree - LINCHECK_TEST=WorkManagerLincheckTest bin/lincheck-test.sh, 2m05s - and Lincheck still reports a violation, so the harness never goes red. It is a DIFFERENT violation: the checkpoint-3 signature (PartitionState.onSuccess's assert removedFromIncompletes, thrown out of completeWork) is gone, and what surfaces is #345's NullPointerException from ShardManager.removeWorkFromShardFor, reached through the same revokeAndReassign operation. The harness's own assertion cannot separate them: contains("completeWork") matches the interleaving table, which names both operations whichever one threw. So the fix IS confirmed by the harness - just not by its assertion. Neither remedy works while #345 is open. Asserting no violation goes red on a defect this branch does not fix; asserting the checkpoint-3 signature also goes red, correctly, and would not be a durable pin either, since Lincheck stops at the first violation and which of the two it reaches is not ordered. Recording the override rather than performing it, per the inherit-and-override rule: the inversion belongs in the change that lands #345, together with ShardManagerLincheckTest's. The measurement and the two rejected moves are in docs/inflight/test-lincheck-lane-open-items.md; the harness javadoc, which said this test would go red, now says what actually happens and points there. TWO INFLIGHT NOTES DESCRIBED THIS BRANCH'S SEAM AS LIVE. check-branch-self-reference.sh fails on them once master is merged in, which is the moment it is designed to fire - static-infer-findings.md and static-spotbugs-rule-registry.md both stated in the present tense that this seam is on master and unfixed. Both rewritten to read correctly after this merges and marked post-merge: checked. The infer one gains the reason its claim survives the fix at all: RacerD's blind spot is the defect class, so which instances are still open does not move it.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… all four fixes it binds #345 and #346 each measured their own harness against a fixed tree and each wrote the result into this note independently. The two accounts disagreed: the one written from WorkManagerLincheckTest prescribed inverting ShardManagerLincheckTest alongside it, which the other had already measured as red. Two adjacent sections, one of them falsified by the other. Replaced by a single section that both branches carry byte-identically, so whichever lands first the second merges it cleanly rather than conflicting - and there is one place to correct when the remaining two fixes are measured. What it now records that neither half did alone: the contract from #347 assumed one bug per harness, and that assumption is what failed. A harness pointed at one seam explores the others, so it can stop finding its own bug without becoming quiet - ShardManagerLincheckTest surfaces a second violation with no NPE in it, WorkManagerLincheckTest passes while actually detecting #345's NPE through an assertion that cannot tell the two apart. The falsified prescription is kept as the argument for the rule rather than deleted: a harness's next assertion is not derivable from its own diff. It also names the half nobody has measured. PartitionStateLincheckTest is the harness #337 and #344 invert, it has never run against a tree carrying either fix, and its javadoc still promises an inversion - so the obligation is on whoever lands any of the four to re-run the whole lane, not only the harness their own PR is named for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… not fight over it #345 and #346 each had to rewrite this paragraph - it named both PRs in the present tense as unfixed, which bin/check-branch-self-reference.sh fails on each branch - and they wrote two different replacements for the same four sentences. Whichever landed second would have conflicted. The kept wording is the one that does not need rewriting again: the blind spot is the CLASS, not the instances, so which of the four are still open does not move what RacerD can see. Naming the four by their fixing PRs keeps the citation valid after each lands, which is the property the previous phrasing lacked.
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
…p fix The only conflict is the 857 ledger, where master and this branch each appended a dated section - both are kept, in the order they happened. `ShardManager` auto-merged: #345 rewrote `removeWorkFromShardFor` to the single-read `getShard(key)` idiom, which is the method that calls `ProcessingShard#remove` and therefore the `uncount` this branch added. The two changes are orthogonal - #345 fixes WHICH shard reference the sweep operates on, this branch fixes what removing an entry does to the count - and the merged form is right without adjustment: a detached shard still accounts its own removal correctly, because the counter it moves is its own. Worth stating because it was NOT obvious from the clean auto-merge: this branch is why `getNumberOfWorkQueuedInShardsAwaitingSelection`'s read-order comment differs from master's, and that hunk landing beside #345's changes in the same file is coincidence rather than interaction. The PR was CONFLICTING while this sat unmerged, which is why GitHub ran no `pull_request` workflow against the last two heads - the checks were not failing, they were never dispatched.
5 tasks done
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
…a copy each The duplicate-code check flagged 22 lines shared between this branch's WorkManagerStaleCheckDoubleLookupTest and ShardManagerRevokeSweepNpeTest, which arrived on master with #345. The clone is this PR's to remove: the other half was already on the base branch. Both needed the same fixture - assign the partition, register one record through the production path, take it as work - and both had asserted the same thing about it, that exactly one container comes back. Extracted verbatim to ModelUtils.registerOneRecordAndTakeIt, which is where this package's shared model helpers already live, rather than a third copy or a new utility class. Static and taking the work manager, because neither caller can use the module's own: one installs a WorkManager subclass that fires a rebalance inside the staleness seam, the other a ProcessingShard map that fires a completion between two reads. The shard-map caller keeps its own one-line wrapper, since its double must be installed BEFORE the assignment that creates the shard. The size assertion moves with the body deliberately. It is a property of the fixture, not of either test: a scenario that silently registered zero selectable records would make both reproductions vacuous, which is exactly the failure mode a drifted copy would introduce on one side only. Core unit suite green on the result: 437 tests, 0 failures, 8 pre-existing skips.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…efore its log expires `ChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedger` was killed fail-fast by the probe on this branch's CI, with the same `ZOMBIE_MEMBER/REBALANCE_BLOCKED` autopsy as the fourth, fifth and eighth entries in the family ledger - a group dwelling the full 15s bound in PreparingRebalance with a member not answering the protocol, and the whole assignment frozen for well over two minutes rather than a single shard. Recorded now because the ledger's own closing argument applies exactly: the sighting is easy to lose. The red belongs to a head that will not be the tip for long, the log expires, and the seed goes with it - which is how the entry above this one describes #345 losing its capture entirely. THE REASON THIS ONE IS NOT JUST ANOTHER TALLY MARK. Every previous capture of this arm predates the dispatch ceiling, which landed on master hours before this run. The commit immediately after the ceiling says in its own body that a leaked dispatch permit now takes the engine to a full stop rather than a stuck record, and a member that has stopped answering the rebalance protocol is what that would look like from outside. So the old intermittency and a new cause are both consistent with this autopsy, and the probe output cannot separate them. The entry says what would settle it, since nothing here does: replay the recorded seed at the ceiling commit and at its parent. Same seed, different position, which is the control this file keeps asking for and rarely gets. No test is quarantined and no assertion is touched. This commit adds a record and nothing else - the failing test is left failing, because a flake that fails the build is the deliberate design and the lever for changing that is quarantine with evidence, not a retry.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…d PartitionStateLincheckTest still does not invert Merging master brought #344 in beside #337, so both fixes the contract named for PartitionStateLincheckTest are now in one tree. The rule that note carries is re-run the lane and re-read EVERY harness's assertion, not only your own, so this is that re-run. MEASURED, whole lane green (2m41s), and the harness does NOT invert. It reports a violation on every run and `assertThat(report).contains("commit()")` still passes - on reports about neither tear it names, because the interleaving table names commit() whatever threw. Two shapes recur across runs: - ArrayIndexOutOfBoundsException out of ArrayList.add, i.e. the plain-ArrayList defect #57 fixes. The note recorded this as a frameless sighting one revision ago; it now has a frame naming it, so the lane has re-found it from a second harness pointed elsewhere. - PartitionState.onSuccess's assert, reached from two `succeed` operations in parallel - which production cannot perform, because only the control thread completes work. The second shape is the same artefact-or-defect question the note already parks over ShardManagerLincheckTest's addWork, arriving now in a second harness. If it is the artefact the fix is a non-parallel group over `succeed`, not a change to PartitionState. That decision is open, this arm's next assertion depends on it, and it should be taken for BOTH harnesses at once - so it is recorded rather than guessed at here. What IS fixed here is the javadoc, which until now told readers this arm would invert when #344 landed, and the note, which said #344 was still its outstanding trigger. Both are now false and both said so confidently. The javadoc also states plainly that this arm does not pin #337 or #344 - PartitionStateCommitEncodeShift894Test and OffsetEncoderWidenedRangeRaceTest do - so nobody reads its green as covering them. The contract's per-PR trigger list is now fully spent, and it went 0 for 4 on its predictions: three harnesses were left finding something other than the bug named, and the fourth inverted only once BOTH violations reachable through its operations were gone. The instruction was right; the one-bug-per-harness assumption behind it was not. Also settles the self-reference bug-torn-read-family.md now demands of this PR. That note arrived on master with the hunt's record and maps merges to sections; its #346 line is rewritten in the post-merge terms the #345 line already uses, and says why the candidate sections stay - the note remains open for the out-of-family stragglers, the #57 trigger and the next hunt iteration. Verified on the merged tree: core unit 451 tests / 0 failures / 8 pre-existing skips, full-reactor test-compile green, bin/check-all.sh 15/15, bin/infer-test.sh "all 22 finding(s) are known, none new", Lincheck lane green.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…at held is the interesting case Self-correction on the claim the previous commit wrote into this note. WorkManagerLincheckTest DID invert when its named trigger landed, so "0 for 4" overstates the failure and understates the more useful finding: it held only because #345 had already removed the other violation reachable through the same revokeAndReassign operation. Had the two landed in the other order the prediction would have failed as well, which is a sharper statement of the same one-bug-per-harness assumption than a bare miss count.
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 27, 2026
…than the ledger counts, and a testable lead on the last unexplained line Reading the merged commit bodies rather than the family's own summary, #119's `## Fork status` is badly out of date. It says three defects landed and one remains. At least six more have landed since, none of them in the family's accounting: - **#346** orphaned a retry-queue entry when a rebalance landed between a staleness checkpoint and its acting read, after which `workIsWaitingToBeProcessed()` read TRUE FOREVER. Its own body calls that a confluentinc#857-family permanent stall. Red-then-green on both arms, mutation-tested. - **#345** NPEd the broker-poll thread inside the rebalance listener via a `containsKey`-then-`get` race. Poller death under KEY ordering is paused consumption by another name. - **#373** found a sign-reversed overcount its body names the "confluentinc#857 stall signature". - **#336** rewrote the load gate whose drift kept intake paused. - **#344** let the offset encoder mark still-incomplete offsets complete. - **#349** fenced a cross-thread `dirty` flag, jcstress-measured. The bucket has been drained steadily from several directions and nobody re-totalled. That is the substantive reason #119 cannot be closed by reading this PR alone, and why its fork-status section needs rewriting rather than appending to. **A prediction is recorded for the one line still unexplained.** The `ChaosChurnStormIT` `NO_PROGRESS` sightings are all `PERIODIC_CONSUMER_ASYNCHRONOUS`, the mode where neither the AB-BA cycle nor the transactional wait can occur - which is why they were set aside as a possible fourth defect. #344's defect is live in that mode and only that mode, and its consequence - offsets marked complete while incomplete - has the `NO_PROGRESS` shape. So: replay the three async seeds either side of #344; if the mode match is the mechanism the pre arm reproduces and the post arm does not. Stated as a prediction rather than a finding, with its own counter-example beside it: #373 was already tested against this arm on a mode-matching hypothesis and fired anyway on that branch's own head. One plausible fix has failed here before.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…d gone out of date in eight ways The ledger is where agents log a sighting and move on, and where the next one reads before forming a hypothesis - so conclusions reached elsewhere are worth nothing until they land here. A week of findings had accumulated in a PR note that gets DELETED when that PR merges. Migrated. What the ledger now says that it did not: - Its own "three landed, one open" summary is stale. At least six more family defects have landed - #346, #345, #373, #336, #344, #349 - and none was counted. The bucket has been drained from several directions and nobody re-totalled. - The original deadlock is VERIFIED rather than diagnosed: an A/B on the deterministic probe, one term changed, control failing every repetition and the fix failing none while logging the contended decline throughout. - Replaying captured seeds does not reproduce that deadlock and should not be attempted again - the window opened zero times locally on either assignor arm, and two write-ups were retracted the same day for reading a green replay as evidence. - The async line has its first replayable seed, found by random hunting rather than replay, and #344 is refuted as its explanation. - A third of those failures went red with the NO_PROGRESS detector silent, which matters because the Class 2 demotion left the liveness claim resting on it. - The symptom is a bucket of at least four distinguishable behaviours, separated by upstream's own maintainer in one sentence, plus a fifth reported on #29 and never chased - a pause with no rebalance in the logs at all. - Upstream's own fix for this issue shipped, is carried here, and did not hold. - confluentinc#875 is filed in this family on a third party's assumption and looks like an offset-completion defect instead. Also records why a single beefier Docker host beats a distributed rig for the soak harness beyond sequencing: central log shipping comes free, and any node can be inspected AT RUNTIME while still wedged. Every sighting in this family that became a mechanism did so because something captured the process while it was stuck; every one that stayed a signature did not.
dhoard
pushed a commit
to dhoard/parallel-consumer
that referenced
this pull request
Aug 30, 2026
…he sweep tool a mode that could have caught it (astubbs#258) * docs(upstream-map): track the 28 issues closed by the 2023 admin sweep Upstream ran two bulk clearouts before going quiet, and neither was a triage: 2023-06-15 `eddyv` closed 35 unmerged PRs with "Closing - Stale." (34 of them ours), and 2023-07-07 `johnbyrnejb` closed 28 issues with "Closing Issue" - every one marked COMPLETED rather than "not planned". That state reason is why the cohort was invisible. GitHub renders `completed` as resolved, so the issues read as done at a glance, and `upstream-sweep.sh` searches `updated:>=last_swept`, so anything last touched in 2023 can never appear in a sweep. None of the 28 were in this manifest; the mirroring that produced the other 78 fork mirrors only ever sampled *open* upstream issues. All 28 were re-read in full - bodies plus all 58 comments - and each verified against fork source rather than trusted from its thread. That mattered: four issue bodies point at PRs that "fix" them (astubbs#372->astubbs#390, astubbs#319->astubbs#270, astubbs#203->astubbs#345, astubbs#191->astubbs#346) and every one of those PRs was itself swept unmerged. Result: 2 already fixed (astubbs#41 offset-scan removal, astubbs#319 shutdown CME), 6 partly addressed, 20 fully open. Grouped into work items rather than 28 near-duplicate entries, since several want designing together - per-topic handlers with separate consume/produce types, seek with subscription changes, retry expiry with stall detection. astubbs#57 folded into the existing log-noise entry. Swept PR head commits were checked and are all still reachable, so the drafts are recoverable; entries cite branch and SHA, not just the PR number. Upstream-Issue: confluentinc#154 Forwarded: no Applied-Upstream: no * docs(upstream-map): mirror the whole 2023 cohort, not the parts we rate The point of mirroring is a clean, known cutoff - "every issue upstream closed administratively is accounted for here" - not a curated pick of the good ones. A set filtered by our own value judgement is not a cutoff: the next person cannot tell "not mirrored because it was junk" from "not mirrored because it was missed", and that ambiguity is worth far more than the cost of carrying a few items nobody will ever action. Drops the decline recommendations from astubbs#53, astubbs#199, astubbs#246 and astubbs#57 in favour of neutral statements of the trade-off, and renames sweep-2023-decline-candidates to sweep-2023-small-items (status wontfix -> none). The facts that informed those recommendations are kept - they are useful to whoever picks the issue up - but they are now framed as input to that person's decision rather than as our verdict delivered in advance. Fork issues can always be closed later on their merits, which leaves a visible record; pruning before mirroring does not. Records the principle in the section header so a later pass does not re-prune. Upstream-Issue: confluentinc#154 Forwarded: no Applied-Upstream: no * docs(upstream-map): link the created fork mirrors back to their entries The 28 mirrors are live as astubbs#227-254 (label `upstream-admin-closed`), with confluentinc#41 -> astubbs#233 and confluentinc#319 -> astubbs#252 created and immediately closed as completed, carrying the code evidence that they are genuinely fixed. Records the fork numbers on each entry so the mapping is queryable rather than re-derived, which is the reason this file exists. Adds `fork.fork_issues` (the plural of the existing `fork_issue`) because several entries deliberately group upstream items that want designing together, and documents it in the schema block. Verified: all 28 mirrors are referenced, none missing, no strays. Also snapshots each upstream title verbatim into its mirror body header, dated 2026-08-07 - the fork titles may drift as these are worked, and the original wording is what makes an old thread findable. Upstream-Issue: confluentinc#154 Forwarded: no Applied-Upstream: no * fix(tooling): give upstream-sweep a mode that can see closures it never saw The default sweep is a window search - `updated:>=$since` - so it structurally cannot see an item whose last activity predates the window, and `last_swept` only moves forward, so the blind spot grows. That is why upstream's 2023 administrative closures went unnoticed for three years: 28 issues closed with "Closing Issue" and 35 unmerged PRs closed as "Closing - Stale.", all marked COMPLETED, none of it triage, none of it visible to us. Adds `--audit`: no time window, asks "which closed upstream items are neither tracked in the manifest nor mirrored in the fork?", and flags days where an implausible number of things closed at once. Run against the real repo it rediscovers both sweeps from scratch and reports zero unaccounted, which is also an end-to-end check that the new fork_issues linkage is correct. Bots are excluded from the PR analysis. Dependabot self-closes superseded bumps in batches that look exactly like a sweep - 2022-08-16, 2022-10-20, 2023-11-03 and 2024-01-25 are all dependabot - and unfiltered they buried the two real sweeps in noise. Filtering them surfaced two human PRs that had been sitting inside those batches (confluentinc#508, confluentinc#650), now recorded for review. The report says outright that a bulk day is not proof of a sweep, because a release triage looks identical from here, and that stateReason COMPLETED is not evidence of a fix - that assumption is what made this cohort invisible. Known remaining hole, recorded not fixed: a PR closed alone on a quiet day is still invisible, since detection keys on bulk. Upstream-Issue: confluentinc#154 Forwarded: no Applied-Upstream: no * fix(tooling): teach the audit about Discussions, a content type we never checked Discussions were invisible to everything: not in the manifest, not mirrored, not in any sweep mode. 74 of them upstream, and nothing we run would ever have mentioned one. They were NOT swept - no bulk closure, and the largest day (2024-04-02, six threads) is all answered=true housekeeping. So the failure mode is different from the 2023 cohort: not administrative closure, just questions nobody answered, in a place no issue search reaches. Handling differs accordingly - these want answering or converting, not mirroring wholesale. The audit now lists zero-reply discussions, excluding release-announcement threads by title: those legitimately have no replies, and counting them as neglect would put 9 false positives at the top of the report. Two finds justify the check on their own. Discussion 542 (zero replies) is a field report of a transactional consumer stuck in rebalance because it cannot acquire the produce lock, with the reporter suspecting the revoke-time flush - the same lock lifecycle as bug-producing-lock-double-release, and they raised the timeout without effect, which is evidence about the mechanism rather than the duration. Discussion 883 (zero replies) is the same complaint as fork mirror astubbs#187 but with a runnable reproducer, which astubbs#187 lacks. Upstream-Issue: confluentinc#154 Forwarded: no Applied-Upstream: no * docs(inflight): record the open obligation to account for every upstream item Two notes, both about what is NOT done. Discussions are not a queue to be converted. There is no pipeline turning threads into issues, and most should never become one. The rule is a judgement: if reading a discussion makes us think there is an issue, we raise one - a normal fork issue on its own merits, citing the discussion as where it came from. It exists because we believe the problem is real, not because a thread existed. That also keeps `upstream-mirror` meaning exactly "an upstream issue we carry", which is what makes the 2023 cutoff verifiable. The wider obligation is now written down: every upstream issue, PR and discussion must be accounted for - carried, declined, or genuinely resolved upstream - not sampled. The 2023 cohort was found only because it was a *bulk* event; `--audit` keys on bulk closures and zero-reply threads, and both are proxies that miss whole shapes of problem (a PR closed alone on a quiet day, a discussion with one dismissive reply, an issue marked COMPLETED with no linked PR). The audit narrows the field; only reading discharges the obligation. Also records what has been ruled out, so it is not re-investigated: wiki disabled, no advisories, all open-milestone issues already mirrored, the orphan branches accounted for (v0.6.x-dev is the lambda-actor work already captured via swept PRs; 0.5.3.x's regression fix IS on master as a908e16), and "upstream pushed today" being a pushed_at artefact rather than new activity. Left open: project boards need a read:project scope we do not have, and 169 forks are unexamined. * fix(upstream-map): use a group the schema actually allows Two new entries used `group: batching-ordering`, which is not in the GROUPS allow-list in scripts/upstream-map.py, so `upstream-map.py validate` failed on them. Caught in review on astubbs#258. My mistake was validating the wrong way: I checked the file with an ad-hoc yaml.safe_load plus a duplicate-id check, which passes happily on a group the schema rejects, instead of running the project's own validator that AGENTS.md tells contributors to run before committing manifest changes. Reassigned both to `features` rather than widening GROUPS. The rest of this cohort's feature entries already use `features`, so this keeps the cohort internally consistent, and the ordering/batching distinction is not lost - it lives where it belongs, on the issues themselves via the `area/batching-ordering` label (astubbs#244, astubbs#236). Adding a vocabulary term to a shared schema as a side effect of a mirroring PR is the larger, less reversible change. Also drops the redundant 345 from sweep-2023-broker-disconnect-commit's `related` list, where it already appears in `prs`. Verified: `upstream-map.py validate` reports OK on 27 entries, and `refs` and `table` both still render. * docs(upstream): graduate the durable parts of the sweep record out of inflight The three inflight notes stay - they hold undischarged work - but they were also carrying permanent content, and inflight is transient by its own charter. Moved to durable homes, each citing the manifest rather than restating it: - docs/upstream.md gains the upstream-admin-closed cohort note (with the "do not trust 2023-era closure states" rule), the discussions non-mirroring policy decided 2026-08-07, documentation of --audit and its known blind spots, and the ruled-out upstream surfaces (wiki, advisories, milestones, orphan branches, pushed_at). - docs/solutions/ gains the transferable lesson: a closure state is a rendering choice not a triage, "fixed by #N" must be checked against the merge bit, windowed watchers need a no-window audit, and bots must be filtered before hunting bulk events. - upstream-pr-analysis.adoc is corrected: it claimed issues were never bulk-closed, which the 2023-07-07 sweep disproves, and the PR sweep is now confirmed rather than suspected. The inflight notes now hold only open work, pointing at the new homes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(upstream): name the analysis doc's role (the plan) beside the manifest (the state) docs/upstream.md's opening enumerated everything it owns except the editorial analysis, and neither source-of-truth reference was a clickable link. Now the two roles are named explicitly: the .adoc is the plan, the manifest is the state tracker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(upstream): make the audit's discussion query actually paginate, and correct the sweep cohort Address PR review feedback (astubbs#258). - upstream-sweep.sh: `gh --paginate` only substitutes a variable named `endCursor`; calling it `$cursor` meant the cursor was never fed back and gh re-requested page 1 forever (reproduced: 469 identical pages at first:2). It only looked healthy because 74 discussions fit in one page of 100. The failure was also swallowed by `2>/dev/null || true`, which would have printed a clean "(none)" for a broken query - the one answer this mode must never invent. Fail loudly instead. - upstream-map.yaml: the sweep cohort listed 36 PRs against a documented count of 35. Upstream closed exactly 35 unmerged PRs on 2023-06-15; confluentinc#66 (closed 2022-10-19) was carried in by mistake. Listing a ref here marks it accounted for, so it was hiding an unrelated PR from every future audit. Recorded PR 258 and moved the entry to `pr-open` per the lifecycle rule. - next-upstream-coverage-completeness.md: dropped the cached counts - inflight notes must not record what a command can answer, and the "~100 partially mirrored" line already contradicted docs/upstream.md, which says all 78 open upstream issues are mirrored with backlinks. Kept only what the audit cannot say: what nobody has read yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(agents): let the manifest cache frozen upstream-issue facts, and say why The rule said "the manifest tracks upstream PRs only", so mirroring the 2023 sweep cohort into it read as a violation. The rule is what is out of date. An archived upstream's closed issue numbers and closure events are frozen. Caching them locally is a read-path optimisation, not a second tracker: grepping one file is instant, while the same answer from the mirrors costs dozens of API round-trips and burns rate limit shared across agents. The usual objection to duplication - the copy silently diverges - needs a source that can still move, and this one cannot. So the boundary is redrawn where it actually bites: the mirror owns an issue's *live state* and remains the only place you update it; the manifest may cache what is frozen (number, cohort, owning mirror). Cache what is frozen; never mirror what is moving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(upstream): put the manifest-caching rule in the doc that owns it The previous commit wrote the rule into AGENTS.md, which is the one place it does not belong. AGENTS.md's own preamble calls itself a router - "if it only matters once you are already in a topic... it goes in that topic's doc" - and docs/upstream.md line 7 already claims ownership explicitly: "AGENTS.md carries only the pointer and the one-line rule that manifest upkeep is the agent's job." The rule was in fact already stated twice before this branch (AGENTS.md:442 and docs/upstream.md:23), so growing the AGENTS.md copy deepened a fragmentation that was already there. Now stated once, where an agent editing the manifest will actually be looking, with AGENTS.md left holding the pointer and the one binding rule whose failure is silent (keep the fork side in sync). Net effect on the router: one line shorter than master, despite covering more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(agents): make a pointer say who owns the rule, not just what is nearby Diagnosed from a live miss. A reviewer cited AGENTS.md's "the manifest tracks upstream PRs only", so that section was read and treated as authoritative - while docs/upstream.md had been stating the same rule, and claiming ownership of it, the whole time. The duplicate was then grown in the router rather than fixed at source. Nothing in the reading path revealed the mistake. AGENTS.md stated a complete-looking rule and followed it with a pointer listing four adjacent mechanics ("the manifest schema, the mirrors, the commit trailers and the upstream sweep"), which reads as *further detail*, not as *this is a summary and that doc wins*. The contract was written down - docs/upstream.md:7 - but only on the side nobody enters from, since AGENTS.md is what loads every session. The anti-duplication rule already existed here ("Never state a fact twice"). What was missing is the half that makes it discoverable: a cross-reference must name the owner and say the owner wins, so a reader can tell a stub from the whole rule before editing the copy. Applied to the rule itself and to the upstream pointer that failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…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.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…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.
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 3, 2026
…dy claims to model
CI red on the Lincheck lane, and the failure was the harness's fixture rather than the shard map.
ShardManagerLincheckTest.stressFindsNoWayToBreakTheShardMap reported an invalid execution -
revokeSweep(0) beside failure(0), then addWork(0): NullPointerException. Stress mode carries no
stack trace, so it was reproduced locally (1 of 1) to get one:
PartitionStateManager.getPartitionState returns null, dereferenced by
ProcessingShard.isWorkContainerStale from addWorkContainer's resident check.
Why it only appears now. This harness never assigned its partition, so getPartitionState answered
null for every container in it - and that was invisible because the branch that dereferences it needs
an ARRIVAL to find a RESIDENT at its offset. Until the declining revoke sweep landed on this branch,
a sweep always removed the resident and, under KEY ordering, the empty shard with it, so every
addWork was an insertion into nothing. A sweep that DECLINES - failure(0) holds the retry queue's
write lock through retryQueue.add, so the concurrent tryRemove refuses - leaves the resident in
place, and the next addWork takes the branch for the first time in this harness's life.
Not a product defect, checked rather than assumed: PartitionStateManager never removes from
partitionStates - onPartitionsRemoved REPLACES the entry with RemovedPartitionState (`partitionStates.put(removedPartition, RemovedPartitionState.getSingleton())`) - and a container only
reaches a shard through a poll for an assigned partition, so the map answers non-null for anything
resident. The unguarded read is real and is a known parked policy question, which
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.md owns and which this does not
touch; that note gains a dated paragraph recording that a harness has now reached the call, from a
state the product deliberately creates.
The fix is the fixture: the constructor assigns TP before planting the resident, which is what its
own comment already claims to model ("the ordinary steady state of a running consumer"). Same idiom
as WorkManagerLincheckTest's constructor.
Teeth checked, not assumed. Restoring the #345 defect this harness regression-tests against -
the containsKey-then-get pair in ShardManager.removeWorkFromShardFor with an unconditional deref -
turns it RED again on the fixed fixture, reporting revokeSweep(0): NullPointerException against
revokeSweep(0) (the sweep racing itself, the route the harness javadoc records). ShardManager was
restored byte-identical afterwards, verified by an empty diff.
Cost: 8.3s to 10.2s at the committed bounds. 4 of 4 green after (3 targeted repeats plus the whole
lane); 1 of 1 red before. Whole lane green in 2m53s (bin/lincheck-test.sh, build included):
ShardManagerLincheckTest 1/0/0/0, WorkManagerLincheckTest 2/0/0/0, RetryQueueLincheckTest 1/0/0/0,
PartitionStateLincheckTest 1/0/0/0, LincheckSuperHashCodeProbeTest 2/0/0/0,
LincheckToolchainProbeTest 2/0/0/0.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
This was referenced Sep 3, 2026
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…dy claims to model (#467) test(core): the Lincheck shard harness assigns the partition it already claims to model CI red on the Lincheck lane, and the failure was the harness's fixture rather than the shard map. ShardManagerLincheckTest.stressFindsNoWayToBreakTheShardMap reported an invalid execution - revokeSweep(0) beside failure(0), then addWork(0): NullPointerException. Stress mode carries no stack trace, so it was reproduced locally (1 of 1) to get one: PartitionStateManager.getPartitionState returns null, dereferenced by ProcessingShard.isWorkContainerStale from addWorkContainer's resident check. Why it only appears now. This harness never assigned its partition, so getPartitionState answered null for every container in it - and that was invisible because the branch that dereferences it needs an ARRIVAL to find a RESIDENT at its offset. Until the declining revoke sweep landed on this branch, a sweep always removed the resident and, under KEY ordering, the empty shard with it, so every addWork was an insertion into nothing. A sweep that DECLINES - failure(0) holds the retry queue's write lock through retryQueue.add, so the concurrent tryRemove refuses - leaves the resident in place, and the next addWork takes the branch for the first time in this harness's life. Not a product defect, checked rather than assumed: PartitionStateManager never removes from partitionStates - onPartitionsRemoved REPLACES the entry with RemovedPartitionState (`partitionStates.put(removedPartition, RemovedPartitionState.getSingleton())`) - and a container only reaches a shard through a poll for an assigned partition, so the map answers non-null for anything resident. The unguarded read is real and is a known parked policy question, which docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.md owns and which this does not touch; that note gains a dated paragraph recording that a harness has now reached the call, from a state the product deliberately creates. The fix is the fixture: the constructor assigns TP before planting the resident, which is what its own comment already claims to model ("the ordinary steady state of a running consumer"). Same idiom as WorkManagerLincheckTest's constructor. Teeth checked, not assumed. Restoring the #345 defect this harness regression-tests against - the containsKey-then-get pair in ShardManager.removeWorkFromShardFor with an unconditional deref - turns it RED again on the fixed fixture, reporting revokeSweep(0): NullPointerException against revokeSweep(0) (the sweep racing itself, the route the harness javadoc records). ShardManager was restored byte-identical afterwards, verified by an empty diff. Green after on the whole Lincheck lane; red before. Reproduce: `bin/lincheck-test.sh`. RE-VERIFIED ON MASTER, 2026-09-07 - AND ONE CLAIM ABOVE DOES NOT HOLD HERE. Extracted from #431 as an independent change. Both claims re-measured on this branch's base rather than carried over: - THE NPE IS NOT REACHABLE ON MASTER. ShardManagerLincheckTest is GREEN with the fixture change reverted (1 test / 0 failures, whole lane 37s). The branch that dereferences the null needs an arrival to find a RESIDENT at its offset, and that needs the DECLINING revoke sweep, which is what #431 adds and which has not landed. So on master this is latent: the harness would go red on its own fixture the moment that PR merges, on the fixture rather than on the shard map it exists to test. Fixing it here is deliberate - it is cheap, separable, and a harness whose fixture contradicts its own comment ("the ordinary steady state of a running consumer", which has its partition assigned) is a false green waiting for a schedule. - THE TEETH DO STILL HOLD ON MASTER. Restoring the #345 defect this harness regression-tests against - the containsKey-then-get pair in ShardManager.removeWorkFromShardFor with an unconditional deref - turns it RED on the fixed fixture, reporting "revokeSweep(0): NullPointerException ... Cannot invoke ProcessingShard.removeWorkAtOffset(long) because the return value of Map.get(Object) is null" against revokeSweep(0) - the sweep racing itself, the route the harness javadoc records. ShardManager restored, empty diff verified. The fixture javadoc and the dated paragraph in docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.md are reworded accordingly: both said the declining sweep HAD landed, which is true on that PR's branch and false here. The note keeps the sighting - the datum it contributes is that the unguarded call is reachable from a state the product deliberately creates, and that does not depend on the fix's outcome - and now says where it was taken and that master was re-measured green. refactor(core): one joinQuietly for the state tests, in the shared ThreadUtils Extracted from #431, where a ce-simplify pass found the same join-with-timeout-and-restore-the-interrupt wrapper hand-rolled in two tests under `state`. Only one of those tests exists on master, so what lands here is the hoist and not the deduplication: the wrapper moves to the shared test ThreadUtils, ShardPopulationRaceTest calls it, and its private copy goes. Hoisting a single-caller helper is deliberate and the javadoc says why rather than claiming a second caller that is not here. `join` with a timeout plus an interrupt restore is what every thread-racing test in `state` needs and what gets re-typed; the second private copy is how the drift starts, and putting it in the shared utility on the way past is cheaper than deduplicating two copies later. The null-thread no-op is kept for the same reason - it is what makes the helper callable from a `finally` block before it knows whether the thread was ever started, which is the shape a cleanup path needs. Behaviour unchanged: the call site's 500ms becomes Duration.ofMillis(500) and nothing else moves. Reproduce: `./mvnw -Pci -pl parallel-consumer-core -am test -Dtest=ShardPopulationRaceTest`. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
Merged
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…ever holds the offset (#492) WHAT CHANGED FOR A USER: a rebalance can no longer remove a record that a later assignment had already re-registered. `ShardManager.removeWorkFromShardFor` - the revoke and lost path - removed by KEY with `removeWorkAtOffset(consumerRecord.offset())`, so whatever occupied the offset when the removal landed is what left the shard. It now removes only the container the revoked record was registered as, or a stale one, and never a live container from a later registration. THE DEFECT CLASS, AND WHERE THIS SITE SITS IN IT. #468 closed the same class at the poller's stale sweep: a removal keyed on the offset cannot say WHICH of two containers at that offset it meant, and the fix was to make `WorkContainer` equality identity so that `Map.remove(key, value)` is a true compare-and-remove. #483's defect-class sweep reported this site alongside it - "the one remaining unconditional by-key shard removal" - and deliberately left it, on the grounds that it belonged with #468's mechanism rather than with a reachability proof. This is that. No fork or upstream issue tracks it; it was reported in a commit body and on a `docs/inflight/` note, so this PR closes nothing. THE CALLER HOLDS NO CONTAINER, WHICH IS WHY THE FIX IS SHAPED DIFFERENTLY. The stale sweep inspects a container and then wants that one gone. This sweep is handed the `ConsumerRecord`s ONE generation was still carrying as incomplete (`PartitionState.onPartitionsRemoved` -> `removeAnyShardEntriesReferencedFrom`), so what it can name is the REGISTRATION, not an inspected container. That works because `PartitionState.maybeRegisterNewPollBatchAsWork` puts the same record instance into `incompleteOffsets` and into the `WorkContainer` it builds, in one loop body - so "the container this generation registered for this record" is `occupant.getCr() == revokedRecord`, and a later generation's re-delivery of the same offset is a different object because it came from a different fetch. That identity has one named exception, stated at the site: `addNewIncompleteRecord` puts unconditionally while `addWorkContainer` DROPS an arrival whose resident is not stale, so an in-generation replay of an already-registered offset leaves the partition naming record B while the shard holds the container over record A. The staleness condition below covers it. TWO CONDITIONS, AND THE MIDDLE OPTION WAS WRONG. Registration identity alone declines for a container from another registration that is nonetheless STALE, and declining there would leave a stale container holding an offset of a revoked partition - the state this sweep exists to prevent. Staleness alone declines for a live container built from the record the sweep was handed, which is what a revoke sweep driven before the partition's state has been swapped sees, and is what `ShardManagerLincheckTest.revokeSweep` models on every invocation: that harness's removal operation would have become a silent no-op while staying green. Verified rather than assumed - `incrementPartitionAssignmentEpoch` starts from `KAFKA_OFFSET_ABSENCE = -1`, so that harness's `EPOCH = 0L` containers are live against the epoch its constructor assigns. So the guard declines for exactly one thing - a LIVE container from another registration - which makes the change a no-op everywhere except in the defect case, and each condition has its own ablation arm. THE HARM, IF IT IS REACHED, IS THE LOST RECORD - #468's, not a misdirection. The live container is gone from the shard while its own `PartitionState` carries the offset as incomplete, so nothing selects it and nothing completes it, and the commit high-water mark cannot pass it until the partition is re-polled. The v6 scope note this came from said the cost was "misdirection bounded to one control-loop tick by #481's purge"; that is the DISPLACEMENT orphan's cost, not this one's. This removal is a shard removal, and the purge collects retry-queue entries. REACHABILITY: NOT TODAY, ON TWO INDEPENDENT ARGUMENTS, AND THE STRONGER ONE IS STRUCTURAL. - Thread ordering, which has to name BOTH production routes. On the ordinary route the poll thread runs both rebalance callbacks, so a generation's revoke sweep completes before the assignment that could register a replacement begins. On the CLOSE route the sweep runs on the CONTROL thread (`maybeCloseConsumer` -> `onLeavePrepare` -> `onPartitionsRevoked`), which `PartitionStateManager.resetOffsetMapAndRemoveWork`'s javadoc calls live in every commit mode - and that is also `addWorkContainer`'s thread, so the two still cannot interleave. "Both callbacks are on the poll thread" is the half-statement, and it was written that way here first. - `resetOffsetMapAndRemoveWork` installs `RemovedPartitionState` BEFORE it calls in, so `checkIfWorkIsStale` is true for EVERY occupant and the staleness condition carries all of them: on the shipped path this method is behaviourally identical to the by-key removal it replaces, and the decline branch is defence for a caller that sweeps before the swap. That says nothing about threads. Nothing checks either argument. The conditional form costs one reference comparison and rests on neither, which is exactly why #468 wrote `getWorkIfAvailable`'s last-resort sweep conditionally against a race it had shown was unreachable there. THE NPE GUARDS ON THIS PATH ARE UNTOUCHED, AND THE CLEARED SUSPICION NAMES THE RIGHT OBJECT. The single-read `getShard` idiom #345 put in stays where it was; the new work is inside `ProcessingShard`, below it. The staleness question resolves the partition state from the OCCUPANT, not from `revokedRecord` - and in the only branch that asks, the occupant is by definition a different registration - so the discriminator is two facts: the state is installed before the call (and a partition with no state `continue`s without sweeping at all), and every `ShardKey` variant is partition-scoped, so an occupant of this shard belongs to the revoked partition. WHAT WOULD REOPEN IT: a caller from a path that has not installed a state, or a topic-scoped shard key - the latter being the reopener `addWorkContainer`'s own cleared suspicion already lists, which reopens both at once. EVIDENCE. `ShardRevokeSweepReplacementEvictionTest`, three arms, RED-then-GREEN through `PartitionState.onPartitionsRemoved` so the argument list is built by production code - one level below the state swap, which is what makes the decline branch reachable at all. Ablation matrix, predicted before running, one arm red per ablation and no other: - whole guard removed, back to `removeWorkAtOffset(offset)` -> the displacement arm - staleness condition removed -> `aStaleContainerFromAnotherRegistrationIsStillSweptOut` - registration-identity condition removed -> `aRevokeSweepStillRemovesALiveContainerBuiltFromTheRecordItWasHanded` - decline retires the occupant instead of leaving it -> the displacement arm's accounting assertion That fourth ablation is why the accounting assertion exists: making the decline call `population.onRetired()` + `excludeFromSelection(occupant)` left every other assertion GREEN while creating a permanent `RecordPopulation` deficit, which is this class's recurring defect and has no clamp and nothing to reconcile it. TWO INSTRUMENTATION FAILURES, RECORDED RATHER THAN TIDIED AWAY. The registration-identity ablation reported NO failure on its first run: dropping the condition left an unused local, the module never recompiled, and the grep for failures matched nothing - a silent false negative in the experiment meant to prove the condition earns its place. Re-run with the build outcome asserted, it is red as predicted. Separately, `@SuppressWarnings("ReferenceEquality")` was written on the identity comparison and then measured: deleted, rebuilt under `-Pci`, `BUILD SUCCESS`. Neither `WorkContainer` nor Kafka's `ConsumerRecord` overrides `equals`, so Error Prone never fired - it suppressed nothing and disagreed with `isResident`'s bare `==` two methods away. Removed, with the reason written where it was. SAME-DEFECT-CLASS SWEEP over `ShardManager`, `ProcessingShard` and `PartitionState`. Fixed: the one above; no unconditional by-key removal of a container remains in main. Dismissed with discriminators: `ProcessingShard.onSuccess` (re-verified rather than inherited - `WorkManager.handleFutureResult` is the third staleness checkpoint and drops a stale result before `onSuccessResult`, so a returning stale container cannot remove the fresh one that replaced it); `addWorkContainer`'s by-key INSERTION (#483's leg 1, plus #336's admit-first, which reads the outcome from the map rather than the earlier read); `removeStaleWorkContainersFromShard` and `getWorkIfAvailable`'s last-resort sweep (already conditional, #468); `ShardManager.getShard` / `compute` / `computeIfPresent` (single-read Optional idiom, #345, and atomic per-key, #336 - not check-then-act); `RetryQueue.remove` / `removeAll` at every call site including `onSuccess`, `onFailure` and `purgeDepartedRetryEntries` (same shape, but it is the queue's keying model rather than a removal that forgot to name its target, so a fix is a change to that model - reported and left by #483, and left here); `PartitionState.incompleteOffsets.remove` and the truncation prune (offsets over records, control-thread-only - "this offset is complete" does not depend on which record object represents it). `removeWorkAtOffset` now has no production caller. Kept as the modelling primitive four test classes use for "a container departs its shard", with its javadoc rewritten: it used to open by naming the `onPartitionsRemoved` caller this change moved away, and now says what it is and which of the two conditional removals a main-code site should reach for instead. RECORDS. `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md` - the class's owner - gains a dated section for the second site, because its own sweep's only hit was a false positive and the real one was found a day later. `docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md` gets a pointer, not a narrative: its subject, state and `PROPOSED closed` marker are untouched, and what is recorded is only that a reader arriving at #483's four-item list can see which one moved. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -pl parallel-consumer-core -am test -Dtest='ShardRevokeSweepReplacementEvictionTest,ShardStaleSweepReplacementEvictionTest,ShardManagerRevokeSweepNpeTest,ShardDisplacementOrphanReachabilityTest'`; `bin/check-pr-analysis-surfaces.sh 492`. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a reproduced race: a shard removed between the revoke sweep's two map reads threw
NullPointerExceptionout of theConsumerRebalanceListenerintoconsumer.poll- the poller-death family.The defect
ShardManager.removeWorkFromShardFordidcontainsKeythengetonprocessingShardsand dereferenced unconditionally. Under KEY ordering the control thread'sonSuccess→removeShardIfEmptyremoves empty shards; landing between the two reads madegetreturn null.Which guard owns which null - corrected at merge prep, because this PR previously said it wrong. The body and the commits claimed the window sat inside the guard added for confluentinc#757.
git log -Ssays otherwise: thecontainsKeytest dates from7e2607560(2022-01-15, upstream batching work), two years before 757 was filed. What 757 actually added is upstream PR 758'sObjects.nonNull(removedWC)check one line lower, againstretryQueue.remove(null), withShardManagerTest.testAssignedQuickRevokeNPEfor it - and that test keeps a shard in the map throughout, so it never reaches the already-removed-shard branch. This is the third null on one revoke path: the same symptom family as confluentinc#757, a different null.Candidate 2 of the torn-read family (
docs/inflight/bug-torn-read-family.md, carried on #344's branch): multiple reads of moving shared state combined as one snapshot - same class as confluentinc#894, in a different structure.The fix
The single-read
getShard(key)Optional idiom every other access in the file already uses. Both absent cases - removed-before and removed-against-the-read - now fall into the same guarded, trace-logged branch. Operating on the reference after it left the map is benign: the shard is detached, andremoveShardIfEmptyre-reads via the same idiom.Evidence
ShardManagerRevokeSweepNpeTest, cherry-picked from the hunt branchtest/torn-read-candidates-reproductionwhere it was committed expected-red (deterministic 4/4 against unfixed master): fires the real production shard removal between the two reads via a racing map seam.containsKey- a call the fix deletes - so it was generalised to fire on the first read of the armed key, with the red side re-verified on unfixed code before the fix was applied. The verbatim cherry-pick is preserved as its own commit for provenance.Defect-class sweep
Whole main tree, untruncated. No other
containsKey-then-get-then-deref instance. Re-swept at merge prep, which turned up two live findings the first pass missed - recorded, not fixed here:ShardManager.removeShardIfEmptyis check-then-remove across both threads.getShard(key)→isEmpty()→processingShards.remove(key), reached from the control thread viaonSuccess/onFailureand from the broker-poll thread via the revoke sweep.addWorkContainercancomputeIfAbsentonto the shard between theisEmpty()and theremove, so the poll thread removes a shard that just gained work - silently orphaned, KEY ordering only. In no ledger; needs its own reproduction.ProcessingShard.addWorkContainerisget-then-puton the same shape, unreachable today only becauseregisterWorkis control-thread-only. Lincheck produced a counterexample over it - seedocs/inflight/test-lincheck-lane-open-items.md.Checked and dismissed, with where I looked:
PartitionStateManager.onPartitionsAssigned- same shape, but the load-bearing fact is that nothing ever removes frompartitionStates(revokeputs aRemovedPartitionState), so thegetcannot be null; the counterHashMaps (check-then-put, rebalance path, a duplicate registration at worst);OffsetMapCodecManager,PartitionState.polledOffsetsandProcessingShard.slowWork(all method-local collections, not shared);RetryQueue.containsandPartitionState.isRecordPreviouslyCompleted(one call, no pairedget).Checklist
AGENTS.mdforbids per-PR changelog entries; the commit bodies carry what the release-time generator reads.docs/inflight/test-lincheck-lane-open-items.md(the inversion contract, reconciled with fix(core): staleness checkpoint 3 checks and acts on one state lookup, closing a stall and a wrong-commit route #346 so both branches carry one identical section),docs/inflight/static-spotbugs-rule-registry.mdandstatic-infer-findings.mdandstatic-archunit-main-code-rules.md(six sites described this seam in the present tense; the rule now reports zero),docs/testing.md,config/spotbugs-exclude.xml. The family dossier on fix(core): the offset encoder read its range top after its snapshot, silently completing records (confluentinc#894 sibling) #344's branch flips this candidate's status when the branches reconcile.ShardManagerRevokeSweepNpeTest, controls both directions.docs/features/- N/A: an internal crash fix with no user-facing surface, option or behaviour change.b3f018048was merged.ce-simplifyandce-code-reviewlocally - N/A - asked for@codex reviewon the PR instead. Neither skill has been run on this branch.Merge-prep additions
Master merged (no conflicts). Beyond the corrections above:
bin/lincheck-test.shpinnedEXPECTED_LINCHECK_CLASSES=5while six harnesses carry@Tag("lincheck")- so the whole lane has exited 1 on master sinceRetryQueueLincheckTestlanded. Nothing invokes the lane, which is why nothing caught it.ShardManagerLincheckTestdid not invert as test(core): a Lincheck lane, calibrated by refinding four real races unaided #347 prescribed. Measured: with the fix in, Lincheck still reports a violation, and it contains noNullPointerException. Controlled both ways. The harness now asserts the NPE is gone - a real regression detector - and the residual is recorded in the reconciled Lincheck note.Chaos Pain Suitehistory on this PR, corrected — it went red twice for two different reasons, and the second one mattered. The early red was theCLASS2_STALL/LAG_STAGNATION154s-against-150ssignature on the sharedhighcpu-4box, which637ea3186demoted to non-gating and025d0b7earemoved the lane for. Chaos then passed on three later heads. It went red again onfa49683c0with an entirely different signature:ChaosRevokeUnderWorkCooperativeIT,Timeout waiting for commit response PT10S, with the ambient probe stating outright that the poll thread was BLOCKED on a monitor — contention or a lock-ordering defect, not a slow broker — thecommitCommandAtomicBooleanheld by the instance's ownpc-controlthread, reached throughonPartitionsRevoked. That is the AB-BA revoke-path cycle whose own write-up records its verification status as "Unproven" and says the fix "addresses a cycle the test cannot reach, while the block the test can reach is untouched". It is not this PR's — the diff touches no locking and noAbstractParallelEoSStreamProcessorline — and it is now recorded as the third capture indocs/inflight/bug-857-family.md, together with the fact that the very next head passed the same suite, which is that file's first direct intermittency datum. No test was loosened, retried or quarantined, and the suite was never re-run to get it green.EQ_DOESNT_OVERRIDE_EQUALSandSE_NO_SERIALVERSIONIDonRacingShardMap. Each rule had exactly one instance tree-wide and both were this PR's, so a rule exclusion would have been targeted suppression dressed as policy, whichspotbugs-exclude.xml's own header forbids. There is a genuine trilemma here — declare neither andEQ_DOESNT_OVERRIDE_EQUALSfires, declare both andCOM_PARENT_DELEGATED_CALLfires, declareequalsalone andHE_EQUALS_NO_HASHCODEfires — so the file is not finding-free: oneHE_EQUALS_NO_HASHCODEremains, naming a contract violation that provably is not present, since the inheritedhashCodeisAbstractMap's and consistent with the delegatedequals. The zero-finding route is a class-scopedMatch, deliberately not taken unilaterally at merge prep.upstream-map.yamlentry (the manifest maps upstream PRs).Note for merge prep: the two original commits lack the session trailers - a repo hook blocked the amend and bypassing it was correctly refused; add them if re-cutting, or fold into the squash sign-off.