Repository navigation
fix(core): own the shard's available-count spend with a compare-and-set, instead of inferring it - #373
Merged
Conversation
…et, instead of inferring it `ProcessingShard.remove()` decided whether to deduct a unit of the shard's available-work count by asking the container `isAvailableToTakeAsWork()`. That predicate cannot answer the question being put to it. It describes what a container IS at an instant; the question is who already SPENT its unit, which no instantaneous state records. Two containers with opposite accounting histories - one never taken as work, one taken and then released by `endFlight()` - are indistinguishable through it. The reported instance: a record is out at a worker, so selection has already spent its unit. Its partition is revoked, the controller drops the now-stale result (`handleFutureResult` -> `endFlight()`, which tells the shard nothing), and the poller's revocation sweep then removes the entry - deducting a SECOND unit, because the container reads as available again. The deficit is permanent: it survives every later add, and resyncs only if the shard drains far enough for the floor-at-zero clamp to absorb it. While it stands, records that genuinely await selection are hidden from the `WAITING_RECORDS` metric and from `drain()`'s check that nothing is still awaiting processing - so PC can transition to CLOSING with work still queued. The same unsound inference had two more instances in this class, found by naming the defect class rather than the symptom: - Sign reversed: a failed record holds a unit again but is not `isAvailableToTakeAsWork()` until its retry delay passes, so revoking it inside that window deducted nothing and left the count one too high - forever. An inflated count keeps the poller's load gate closed, which is the confluentinc#857 silent-stall signature. - `addWorkContainer`'s stale-replacement branch deducted nothing "since we're replacing, not adding", which is true of `entries` and false of the counter: a replaced entry that had been taken as work already spent its unit, so the fresh entry arrived uncounted. Recorded as a known defect on #322's branch. - `ProcessingShard.onFailure()` took no argument and incremented unconditionally, while `ShardManager.onFailure` documents itself as idempotent. It was not. The fix makes the unit an owned thing rather than a deduced one. `WorkContainer` carries an `AtomicBoolean` recording whether it currently holds a unit of its shard's count; the shard claims and releases it by compare-and-set, and only the caller that WINS the transition moves the counter. Every site then acts on a fact it owns. `countAsSelectable` claims first and confirms residency second - deliberately, since the reverse order is the same check-then-act - and both halves being CAS-guarded means the unit is returned exactly once however the two interleave. Two consequences worth stating. The clamp is gone: the counter cannot go negative by construction, because a decrement happens only where a unit was held. A clamp is not a safety net here, it is what absorbed exactly the drift that would have exposed this defect. And the accounting no longer reads `WorkContainer.inFlight` at all, which was a plain non-volatile boolean read across threads - so the unfenced half of the finding is closed for this path without waiting on the wider work in #335. Rejected: deleting the counter and deriving the answer by scanning `entries`. Measured at ~4.5ns per entry, which is 45us per call at 10k buffered records and 236us at 50k. `isSufficientlyLoaded()` reads it on the broker-poll path at control-loop cadence, so that is 4-24% of a core spent answering "is the pipeline loaded". Three of its four readers could afford a scan; the load gate cannot, and #361 is concurrently building an O(1) occupancy index precisely because scans are the cost. Deletion becomes viable once #336 removes the load gate's dependence on this counter - it is not blocked by anything here. Also rejected: clamping or flooring the drift, which is what hid this class of bug in the first place. Found by review on #336 (thread 3862087445), left open there because the proper fix is a redesign rather than a review edit.
…ix invalidated Follow-up 1 of the confluentinc#909 write-up cited `ProcessingShard.java:70` and `ProcessingShard.java:195` - the form AGENTS.md forbids, written before that rule. Both targets have now been deleted outright, so the addresses are replaced with greppable anchors plus the `git log -S` that recovers the code the document was reading, per docs/citations.md. The claim itself is left exactly as written: it was true of the code it describes, and a dated record may not be rewritten to match today's tree. What is added beside it is a status flag saying what happened to the thing it asked for.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
Owner
Author
|
@claude review this Focus, in order:
|
|
Claude finished @astubbs's task in 7m 3s —— View job Review
Note: the checked-out tree here is a shallow clone ( 1. The ownership argument — I could not break itI traced every interleaving of
One thing that looked like a break at first glance but isn't: 2. Sites I checked for a seventhGrepping every call into a One thing worth flagging even though it isn't a bug in this PR: 3. The delete-vs-fix call — the reasoning holds, with one nuance worth stating explicitly
The nuance: I didn't independently re-run the JMH-style numbers in the PR body, but the shape (a Everything else
No correctness bugs found. The two items above (§2's |
🧪🔒 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 |
|
Owner
Author
|
@codex review Concurrency change: Please look hardest at:
Report only - do not push commits. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…claiming the invariant Review pass over the ownership change. Three reviewers plus the automated review on the PR found no correctness defect in the CAS protocol itself; what follows is what they did find. THE ONE MISSING TEST, AND IT WAS THE LOAD-BEARING ONE. countAsSelectable's residency recheck - `if (entries.get(wc.offset()) != wc) uncount(wc)` - is the single line that makes claim-then-confirm safe when the two threads actually interleave rather than merely run in the damaging order, and nothing exercised it. It is reachable without racing, through the same kind of seam the other four tests use: a revocation landing between handleFutureResult's staleness check and sm.onFailure has the controller count a container selectable again after the poller has taken it out of the shard. Deleting the recheck makes the new test red (expected 1, got 2, with the ground-truth scan disagreeing); the other four stay green, which is what makes this test the one that covers it. THE INVARIANT WAS STATED MORE STRONGLY THAN IT HOLDS. "Equals the units held, and is never negative ... by construction rather than by clamping" is true at quiescence and false instant-by-instant: countAsSelectable claims and then increments, which is two atomics, so a reader in that window sees the scan one ahead - and if a concurrent uncount wins the release there, the decrement lands first and the counter momentarily reads -1. Every such interleaving still settles correct and every consumer reads the aggregate, which floors at zero, so this is a documentation defect rather than a behaviour one. But "no clamp, by construction" is the headline claim of the change, so it is qualified where it is made rather than defended in a PR comment. Collapsing the two atomics into one is the follow-up that arrives with #335's Execution transition. A COMMENT THIS CHANGE FALSIFIED. getNumberOfWorkQueuedInShardsAwaitingSelection reads the retry-queue size before the shard sum, and the only record of why was "normally retry queue is updated before shard counters are on work taken". That stopped being true here: the decrement moved out of a single batch at the end of getWorkIfAvailable and into the selection loop, so the shard counter now drops first and the window spans the rest of the scan. The read order is still right; the reason is now re-derived in place, with the arithmetic, and lands on the same conclusion - the skew is nil or high, never low, and low is the direction that closes drain() early. A REAL DEFECT FOUND, NOT FIXED HERE, AND RECORDED. The poller's stale sweep calls iterator.remove() on a ConcurrentSkipListMap entry set, which removes by KEY - so a fresh container the controller put at that offset since next() returned is what actually leaves, while the uncount that follows releases the stale object. The primary harm is the lost record, and it predates this change; the secondary harm is the counter, and its DIRECTION is this change's doing - formerly one low and resynced by the clamp, now one high and permanent, which is the direction that stops drain() reaching CLOSING. Not fixed here because there is no identity-keyed removal to reach for: entries.remove(key, value) and entrySet().remove(entry) both compare with equals, and WorkContainer.equals is topic/partition/offset only, so the replacement compares EQUAL to what it replaced - the same reason countAsSelectable compares with !=. Written up with the decision it needs in docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md, and the call site now points at it. Also: the @GuardedBy carve-out claim gave the tree's AGENTS.md more than it actually says - it names volatile, not atomics - so the javadoc now gives the reason directly and quotes what the rule does ask for. A dead local in getWorkIfAvailable is gone. The 2026-08-26 Chaos Pain Suite red is logged in the 857 ledger as a NEGATIVE result: this is the branch on which the fleet NO_PROGRESS arm should have stopped firing if that defect were what it caught, and it fired anyway. One backlog line for the state package's copied test harness. Verified: bin/ci-unit-test.sh BUILD SUCCESS across all 11 modules; bin/check-all.sh 15 ran, 15 passed. Nothing loosened, weakened or quarantined.
#335 - the per-attempt `Execution` identity behind an `AtomicReference` - merged while this was in review, and it rewrote the exact method this change touches. Both conflicts are in that seam and both resolve to a union rather than a choice: - `ProcessingShard#getWorkIfAvailable` - master turned the claim into `if (workContainer.onQueueingForExecution())`, one call that evaluates the whole decision and claims from the state it evaluated. The `uncount()` this branch added now sits INSIDE that branch, so the unit is spent only by the caller that WON the claim. That is strictly better than the pre-merge form: a refused claim leaves the container resident and still holding its unit, which is what `docs/inflight/core-a-lost-claim-means-two-different-things.md` says the refusal must do. - `WorkContainer` - `holdsShardAvailableUnit` and its three accessors sit alongside master's `Execution`/`deliveryCount`. `maybeUserFunctionSucceeded` was NOT carried over: master folded it into `Execution` and deleted it, and git offered it back only because it was adjacent to the new field. FOLDING THE UNIT INTO `Execution` IS NOW UNBLOCKED, AND STILL NOT DONE HERE. This PR's description says the flag should become part of #335's transition once that lands, so that the claim and the unit move as one atomic step instead of two. That is now possible rather than hypothetical - and it is the same follow-up as the transient this branch documented on `availableWorkContainerCnt` (a reader can see the counter at -1 between the claim and its increment), because one transition is exactly what removes it. Left for its own change: it rewrites `Execution`'s contract, and this branch's value does not depend on it. Verified after the merge: `ShardAvailableCountOwnershipTest` 5, `WorkClaimStateMachineTest` 10, `ShardManagerTest` 4, `ProcessingShardStaleReplacement909Test` 4, `WorkManagerTest` 26 - 49 tests, 0 failures.
Owner
Author
|
@claude review this Re-requesting: the head has moved twice since your last pass ( Focus, in order:
Report only - do not push commits. |
|
Claude finished @astubbs's task in 3m 37s —— View job Review (re-run at
|
Review finding on #373, and it is right: the field javadoc claimed "every consumer reads the aggregate ... which floors at zero", and one does not. The under-served-retrieval diagnostic in ShardManager#getWorkIfAvailable sums getCountOfWorkAwaitingSelection() straight across the shards behind log.isDebugEnabled(), with no max(0, ...) - so the transient this branch documented can surface as awaitingSelection=-1 in that one debug line. It drives no control flow, and it is left unfloored deliberately rather than overlooked: that line exists so a human can tell a genuine stall from normal back-pressure by reading what the accounting actually says, and clamping it would hide exactly the drift it was added to expose - the same argument this change makes for deleting the clamp on the counter itself. So the javadoc now names the exception instead of the diagnostic acquiring a floor. Also states, where the transient is described, that folding the flag into #335's Execution removes it rather than masking it - the distinction that makes it a real follow-up rather than a note. Verified: ShardAvailableCountOwnershipTest 5/5, bin/check-all.sh 15 passed.
Owner
Author
|
Addressed in The one finding — the debug diagnostic that does not floor — is correct, and the javadoc was wrong. Fixed on the javadoc side rather than by flooring the diagnostic, and deliberately. That line exists so a human can tell a genuine stall from normal back-pressure by reading what the accounting actually says; adding a clamp to it would hide exactly the drift it was added to expose — which is the argument this PR makes for deleting the clamp on the counter itself, so applying the opposite reasoning one method away would be incoherent. The field javadoc now says which readers floor (the aggregate behind On the other four: thank you for re-deriving the read-order arithmetic for the k-container general case rather than just checking the single-container illustration in the comment — that generalisation is not something the comment claims, and it is more useful than the case it does claim. Verified after the change: Two things still open for the human reviewer, neither introduced by this change:
|
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
…cted by inference The review pass named this as a stated gap rather than closing it, on the grounds that the sweep calls the same `uncount` helper already covered at `remove()`. That was the wrong call: the helper being shared is exactly what a regression at this call site would not tell you, because the site is what decides WHETHER to call it. The old code here was `dcrAvailableWorkContainerCntByDelta(1)` - an unconditional deduction, the same unsound inference as the reported defect reached by the poller's path instead of the controller's. `thePollersStaleSweepMustNotDeductForAnEntryThatAlreadySpentItsUnit` drives `sm.removeStaleContainers()` over a resident that was taken as work and then went stale across a rebalance, so it holds no unit and the sweep must deduct nothing. Restoring the unconditional decrement makes it red, along with the stale-replacement case - two failures, which is the honest shape: the two sites share the defect. Restored, both green. Also asserts the sweep actually removed the planted resident before believing the count, so the test cannot pass by sweeping nothing. The remaining stated gap is `getWorkIfAvailable`'s inline stale-eviction branch, which needs a stale entry at the HEAD of an order-restricted shard scan - and that fixture's realism is the thing in doubt, not the coverage. Left named in the PR body rather than faked. Verified: ShardAvailableCountOwnershipTest 6/6, bin/check-all.sh 15 passed.
…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.
… heads later The sighting recorded earlier on this branch reads as a negative result about the counter fix - the fleet NO_PROGRESS arm firing on the very branch that should have quietened it. That is still what it is, but stated alone it invites the stronger reading, that the fix made no difference to a stall the file attributes to it. `Chaos Pain Suite` passed on `b5feb36b0`, three heads later on the same branch, on a fresh seed, with no change to the accounting in between. One red and one green over the same code is evidence about the ARM, not about the counter - and it is this file's own seed-dependence finding observed within a single branch rather than across several, which none of the sixteen sightings before it could show.
astubbs
marked this pull request as draft
August 26, 2026 14:45
astubbs
commented
Aug 26, 2026
astubbs
left a comment
astubbs
left a comment
Owner
Author
There was a problem hiding this comment.
great work, minor adjustment suggestions
…larity Antony's review on #373 flagged 11 naming/javadoc findings in ProcessingShard.java and WorkContainer.java - all mechanical, no behaviour change. - ProcessingShard: `entries` -> `workMap`; `availableWorkContainerCnt` -> `availableWorkContainerCount`; `addWorkContainer`'s `wc` -> `incomingWorkContainer` (and `existing` -> `existingWorkContainer`, to pair with it); `onSuccess`'s `wc` -> `successfulWork` and its `removed` -> `removedContainer`; `onFailure`'s `wc` -> `failedWork`; `remove` -> `removeWorkAtOffset`; `uncount` -> `uncountAsSelectable` (pairs with the existing `countAsSelectable`); `countHeldUnitsByScan` -> `countClaimedAsSelectableByScan` to match. - WorkContainer: added `@see ProcessingShard#countAsSelectable` / `#uncountAsSelectable` links to the field javadoc so a reader lands on the mechanism. Renamed the "AvailableUnit" concept throughout - field and methods - to "claimed as selectable": `holdsShardAvailableUnit` -> `claimedAsSelectable`, `claimShardAvailableUnit()` -> `claimAsSelectable()`, `releaseShardAvailableUnit()` -> `releaseAsSelectable()`, `holdsShardAvailableUnit()` -> `isClaimedAsSelectable()`. "Unit" was the reviewer's specific complaint (esoteric); "claimed as selectable" says what the boolean actually answers - whether this container currently holds the shard's claim to count it toward the available-work count - and mirrors ProcessingShard's own `countAsSelectable`/`uncountAsSelectable` pairing rather than inventing a second vocabulary for the same mechanism. Also fixed the "Take a unit" javadoc (and its release-side twin) to say "the" - there is only ever one. Swept the whole tree (main, tests, and the two docs/inflight/ notes that named the old identifiers as live state) rather than only the reviewed line numbers, per AGENTS.md. Left docs/solutions/logic-errors/stale-container-blocks-fresh-work-same- offset-after-rebalance-2026-08-07.md untouched: it quotes a since-removed code comment verbatim as history, and docs/citations.md forbids rewriting a dated record's claims to match today's code. Verified with `mise exec java@temurin-17 -- bin/ci-unit-test.sh` (full reactor, BUILD SUCCESS) and `PR_NUMBER=373 GITHUB_HEAD_REF=fix/available-count-ownership bin/check-all.sh` (15/15 gates pass).
5 tasks done
…match its own vocabulary "count" was doing two different jobs in ProcessingShard.java - the regard-as sense in countAsSelectable, and the enumerate sense in the availableWorkContainerCount field beside it - and "uncount" is not a word. What the operation actually does is change set membership: the container joins or leaves the population whose size the counter reports. "selection" is the noun the codebase already uses for that population (getCountOfWorkAwaitingSelection, ShardManager#getNumberOfWorkQueuedInShardsAwaitingSelection), so this reuses it literally: - ProcessingShard: countAsSelectable -> includeInSelection, uncountAsSelectable -> excludeFromSelection, countClaimedAsSelectableByScan -> countSelectionClaimedByScan, availableWorkContainerCount -> workAwaitingSelectionCount (matches the getter it already backs). - WorkContainer: aligned the container-level claim pair to the same noun so the two layers visibly pair up - claimAsSelectable/releaseAsSelectable/isClaimedAsSelectable and the claimedAsSelectable field become claimSelection/releaseSelection/isSelectionClaimed and selectionClaimed. The two layers are genuinely different and both names survive: the container-level pair claims the right to be counted, the shard-level pair performs the membership change. Swept main, the test, and the one docs/inflight/ note that spelled an old identifier out in prose (bug-stale-sweep-iterator-evicts-fresh-replacement.md). Left the other docs/inflight/ mentions of the prior availableWorkContainerCnt -> availableWorkContainerCount rename (bug-857-family.md, test-lincheck-lane-open-items.md) alone: those are dated, reviewed (post-merge: checked) analyses of a specific historical measurement pinned to the name as it stood at the time, not live technical citations - renaming them there would misstate what was actually measured. docs/solutions/ untouched, per docs/citations.md. Pure rename plus javadoc - no behaviour change. Verified with `mise exec java@temurin-17 -- bin/ci-unit-test.sh` (full reactor, BUILD SUCCESS) and `PR_NUMBER=373 GITHUB_HEAD_REF=fix/available-count-ownership bin/check-all.sh` (15/15 gates pass). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XC9N7S8gSd6fCYNNe64D6j
astubbs
marked this pull request as ready for review
August 26, 2026 23:52
…-ownership # Conflicts: # parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/ShardManager.java
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
… population, the claim CAS drives the selection count #373 rewrote ProcessingShard under this branch. It replaces an INFERRED spend of the shard's selectable-work counter with an OWNED one: each container carries an AtomicBoolean claim and only the compare-and-set winner moves the counter. This branch pairs every change of the shard's POPULATION with a real map mutation. The two meet in addWorkContainer, both sweeps and onFailure, and the resolution is not textual. They compose, because they count different things and neither is derivable from the other. A record can leave the selection population without leaving the shard (taken as work) and can leave the shard holding no claim (revoked at a worker). So the rule the merged code follows is: the MAP's own return value drives the population, the CAS outcome drives the selection count, and neither is ever conditioned on the other. That is now stated on the population field. Per site: - addWorkContainer keeps this branch's admit-first shape - population.onAdmitted() then put(), and the put's return value, never the read above it, decides whether anything was displaced. master's version had gone back to deciding from the pre-read. On top of it, the arrival is offered a claim and the displaced container gives one back, each settled by its own CAS. Include runs before exclude so the transient is +1 rather than -1; -1 is the direction #373 documents as the one a reader can see, and reading low is what closes drain() early. Verified against the interleavings both designs worry about: if a sweep removes the arrival between the put and includeInSelection, the residency recheck hands the claim straight back and the sweep's retirement pairs with the admission - nothing double-counts either way. - retireAlreadyDeducted and retireAndDeductIfStillCounted COLLAPSE INTO ONE. That was this branch's own prescription for the counter - "an ownership flag the shard claims and releases with a CAS ... collapses the two into one method" - and #373 is that fix. There is now a single exit path, retire(), driven entirely by what the map gave up. - the isNotInFlight() window is closed, not deferred. It was the open review finding on this PR, left unfixed because the load gate had stopped reading the counter. The predicate is gone. - both stale sweeps keep this branch's removal-by-return-value, so the population retirement, the claim release and the container handed to the caller all follow the object that actually LEFT rather than the one that was inspected. That closes half of bug-stale-sweep-iterator-evicts-fresh-replacement.md - the accounting half. The by-key eviction of a fresh replacement is untouched and still open; its note now says which half is which. - the last-resort eviction in getWorkIfAvailable keeps this branch's retry-queue cleanup, so it cannot orphan a queue entry, on top of master's claim release. The map stays PRIVATE. master added @Getter for three white-box test plants; a public handle on the map falsifies the invariant this branch's javadoc claims and lets a test insert without admitting, which drifts the population silently and fails nothing. plantResident() replaces it - same white-box reach, pairing kept, no claim offered because those tests are planting containers that have already spent one. The two accessors also collapse: one getWorkContainerAtOffset(), returning #335's Optional under the name this PR's review asked for. getWorkableRecords() is unaffected - it reads the population and the retry queue, and never read the selection counter. #373's claim that deleting the shard counter becomes viable once this lands HOLDS, and is now recorded rather than left in a merged commit body (docs/inflight/core-shard-selection-counter-can-now-be-derived-by-scan.md). Grepping every reader in core main: the load gate is gone, and what is left is drain() at shutdown cadence, the WAITING_RECORDS gauge per scrape, and three lines behind isDebugEnabled/isTraceEnabled. No steady-state reader remains, so the scan cost #373 measured is no longer paid per control loop - though the note asks for that number to be re-taken at the new cadence rather than reusing a verdict that answered a different question. Notes brought back in touch with reality, per docs/inflight/AGENTS.md's four outcomes: bug-available-work-counter-is-still-an-approximation.md is retired - #373 landed the derive-it fix it prescribed and shipped the invariant check it asked for. What outlived it is a different item and gets its own note, the second counter in the same calculation (bug-number-records-out-for-processing-is-a-plain-int.md). The lincheck lane's entry for this counter is rewritten: the drift it described is gone, and what is interleaving-shaped now is the two-atomics transient the claim protocol accepts. Also repaired: bug-stale-sweep-iterator-evicts-fresh-replacement.md arrived citing the note this branch deletes, the same way two java files did last merge; same fix, the history pointer docs/citations.md prescribes at a sha on master. 477 core unit tests green, including #373's ShardAvailableCountOwnershipTest through the new seam and this branch's ShardPopulationRaceTest unchanged in substance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XC9N7S8gSd6fCYNNe64D6j
5 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…t on mechanism rather than on its diff The Chaos Pain Suite went red on ChaosChurnStormIT with a single ZOMBIE_MEMBER/REBALANCE_BLOCKED violation (seed 8468027865395802787). It is NOT the BLOCKED-on-monitor discriminator the five 2026-08-26 captures record - no commit-response timeout appears anywhere in the job - so it is filed as the fourth sighting's arm rather than a sixth capture of that pair. What is new is the ruling-out. Every earlier capture clears its branch by pointing at a diff of markdown and shell; this one cannot, because the branch rewrites the shard accounting and had just merged #373's claim protocol. So the argument is made on mechanism: ProgressProbe's rebalance-dwell detector is keyed on group-protocol unresponsiveness, and the only path from the load gate to the protocol is partition pausing, which leaves the poll loop calling consumer.poll() - so a drifted count cannot make a member rebalance-blind, it can only stall. The stall-shaped detectors all stayed quiet and the fleet finished its 100000 records, which is the corroboration. Also recorded, because it would otherwise be lost with its artifact: a sixth BLOCKED-on-monitor capture from another PR's chaos job ten minutes earlier (seed 818084281700661522), and the observation that three overlapping drains covered the whole dwell window against a bound calibrated on one drain - as an experiment to run, explicitly not as a diagnosis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XC9N7S8gSd6fCYNNe64D6j
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…ir every count change with a real map mutation (#336) Relates to #119 (confluentinc#857) - recorded there as a candidate mechanism, not claimed as the root cause. ## Description The broker-poller load gate was fed by a counter whose two conditional decrements did not match its increment's condition. Both drifts reproduce **single-threaded**, despite the clamp's "in case of possible race condition" comment: - **High side, corrected by nothing:** revoking a record parked in retry back-off deducted only if `isAvailableToTakeAsWork()`, which requires the delay to have passed - so the increment `onFailure()` had just added stayed forever. - **Low side, which the clamp existed to hide:** both stale sweeps deducted unconditionally, double-deducting records already deducted at selection. That sum fed `isSufficientlyLoaded()`, which decides whether the broker poller pauses - so the drift did not misreport, it **mis-gated record intake**, and enough phantoms keep intake paused until a restart clears the counter. The gate only ever consumed "records inside the system", so `RecordPopulation` now derives that by conservation - `admitted - retired`, two monotonic `LongAdder`s - and **the clamp is deleted**: a clamp on a conservation figure hides the only signal that the pairing broke. ## Making the pairing hold under concurrency A conservation figure is exactly as good as its pairing with the map, and nothing reconciles the two, so every mispairing drifts permanently and silently. Three sites decided from something other than what the map did: - **`addWorkContainer` decided "replacement, do not admit" from the resident read *before* the insert.** A sweep landing in between makes it an insertion, and the skipped admission leaves the population permanently low. It now admits first and derives from `put`'s own return value. - **Both stale sweeps retired after an `iterator.remove()` that discards the return value.** Removal goes through `removeAndRetire`, retiring exactly what the map gave up. A value-conditional `remove(offset, container)` would be *wrong* here: `WorkContainer.equals` is offset-only, so a fresh replacement compares equal to the stale entry it replaced. - **`ShardManager` took a shard from `computeIfAbsent` and wrote to it as a separate step.** Under KEY ordering `removeShardIfEmpty` can collect it in between; the record was already lost that way, but the admission is never retired, so the figure reads permanently **high** - the direction that pauses polling for good. Choosing and writing are now one `compute()` against the same per-key lock. `getWorkIfAvailable`'s last-resort eviction now cleans the retry queue too - an orphaned entry is removed by nothing, ever - and `isSufficientlyLoaded` logs the snapshot it decided on rather than re-reading, in the one line whose job is telling a real stall from counter drift. ## The operand pair, stated honestly `ShardManager.getWorkableRecords()` owns the subtraction and returns a carrier, so the gate decides and logs from one call. A **consistent** pair is not obtainable: the population reduces lock-free `LongAdder`s while the retry queue reads under its own fair lock, and spanning both would put the poll thread's admission path behind that lock. The javadoc therefore states the residual rather than implying a guarantee - retry-queue movement in the window costs nothing, population movement is the whole skew, it is bounded by admissions during the retry-queue read, and it cannot accumulate, because the gate resamples every control-loop tick. That last clause is the difference from the permanent drift this removes. ## Composition with #373 #373 landed on master mid-flight and rewrote the same class. The two count **different** things and neither is derivable from the other: a record can leave the selection population without leaving the shard (taken as work), and can leave the shard holding no claim (revoked at a worker). The merged rule, written on the population field: **the map's return value drives the population; the CAS outcome drives the selection count; neither is ever conditioned on the other.** Consequences: #373's claim collapse closes the `isNotInFlight()` window this PR's review left open - gone, not deferred; the shard's map stays private, with `plantResident()` replacing the public getter #373 added for white-box tests; and #373's claim that the selection counter becomes deletable once this lands is **verified** - no steady-state reader remains - and recorded, asking for the scan measurement to be re-taken at the new cadence rather than reusing a verdict that answered a different question. ## Verification The conservation walk drives every departure path in one sequence - success, failure into back-off, retry redelivery, a sweep under live work, a revocation taking both a parked record and one out at a worker, and the stale result handed back afterwards - holding the O(1) figure against an O(n) scan computed by different means. `ShardPopulationRaceTest` adds one regression per concurrency finding, each interleaving driven deterministically through a collaborator rather than raced for, and **all four confirmed red on the tree without the fixes**, each on its own assertion. Full reactor suite, `bin/check-all.sh`, and the Lincheck lane green - the latter still finding the violation it is calibrated to find. Same defect class written up in `docs/solutions/logic-errors/counter-clamp-hid-a-conditional-decrement-bug-2026-08-21.md`, which ships here. Adapted cherry-pick of `fa4d1cf251` from `perf/engine-concurrency`.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…d why it is not a speed win (#377) Every rename here is a text sweep checked only by "it still compiles". `bin/rename-packages.sh` is the biggest instance and its header already records what that costs: freeze regions silently ignored on macOS, and a sweep that has to be spelled as an escaped-regex alternation because some files encode the package that way. The script asserts the renames git recorded *and* their pairing purely because the renamer cannot be trusted. OpenRewrite's `rewrite-maven-plugin` works off the compiled LST, so its rename recipes are type-aware. It is in no pom here. The note deliberately argues against the obvious pitch. Measured on #373's two rename passes, identifiers were about a third of the work; the rest was javadoc prose, test method names, notes in this directory, and a caveat paragraph that needed rewriting rather than renaming - none of which a recipe touches. So the case for adopting it is correctness on wide renames, not token or time savings on review-sized ones, and the note says so plainly rather than leaving someone to discover it after wiring the plugin in. Filed as `ci-` because the harness owns tooling, and left as a task with the three questions that have to be settled first - Jabel cross-compilation, whether recipes get reviewed like the shell gates, and whether it becomes a required lane. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…doc already claimed BrokerlessWorkManagerTestBase was extracted saying two tests had built the same fixture independently, and it named both by role - one that assigns, and EpochAndRecordsMapRaceTest, which must not. Only the first was migrated, so the duplication cited as the motivation was still in the tree and the base's javadoc described a sibling that did not extend it. The review on #375 caught it. Fixed by doing the migration rather than softening the claim: the test drops its own mu/wm/sm/pm/topic/tp and setup(), which were the base's buildWorkManagerFixture() minus the assignment, and inherits them instead. Not assigning is the base's default, so nothing is overridden. That default being silent is the one risk in the move - the deleted setup() carried a comment saying the missing onPartitionsAssigned WAS the race, and inheriting a default loses that. The reasoning is now in the class javadoc as an explicit statement that not overriding assignPartitionsIfWanted() is load-bearing here, so a later reader adding an assignment for convenience has to read why first. docs/refactoring.md carried the wider version of this - four classes opening with the same wiring, noticed on #373. Its entry no longer lists this test as a copier, records that the base now exists and where, and says why the three left are not deletions: ShardAvailableCountOwnershipTest names its topic constant TOPIC, and ShardManagerTest also builds a PartitionState and declares no sm/pm.
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
…eed that replays, and it is not branch-caused Seed 9086872209853284830, ChaosChurnStormIT, found by random-seed hunting in CI rather than by replaying anything. Replayed locally on two trees, twice each: the fix branch failed both times, master passed then failed. Three of four runs reproduced, every failure the same shape - NO_PROGRESS, the fleet reaching roughly 95% of its target and stopping. **It fires on master, so it is not branch-caused.** The first two runs read fix-red/master-green and suggested otherwise; that reading was wrong and the second master run refuted it. Recorded rather than quietly dropped, because the same inference had already been ruled out for this line - these sightings have appeared on branches carrying no Java at all, and #373 was tested against this arm on a mode-matching hypothesis and fired anyway on its own head. A second seed from the same sweep, 6078190770998307147, reproduced in none of four runs. Kept as the control that makes the first one mean something: seeds from this line are mostly one-offs, and this is the one that is not. Why it matters more than another sighting: every entry in that file names replay as its deciding experiment and none had ever been run - the file says so in as many words. This line now has a handle that works on a laptop in minutes against unmodified master, which is what the deadlock lacked until a purpose-built probe was written for it. The next experiment is stated before running it, including the reason it is likely to fail: #344 fixed an offset-encoder double-read live only in this commit mode, so it was the obvious candidate - but the post-astubbs#344 arm IS current master, and master already reproduces, so on the face of it #344 is not the explanation.
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.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…s engine back Merges `feats/proxy-dispatch-clients` (#390, the top of the Wagon A stack) into this branch, so that #293 can be retargeted onto that rung and its displayed diff collapse to the engine residue. This is the plan's "retarget move" - `docs/plans/2026-08-31-001-process-god-branch-decomposition-plan.md` - and it is an ordinary merge: no history was rewritten and nothing was force-pushed. The pre-merge tip is preserved as `origin/backup/pre-stack-merge-293`. The merge also brings `master` forward, because the stack is current with it and this branch was not. That is most of the volume and most of the conflicts. ## Which side won, and why The rule was: the god branch's SEMANTIC content, expressed on the stack's refined STRUCTURE. - **Engine-only files stayed** - `ProxyProcessor`, `ConfigureHandler`, the dispatch waves, epochs, leases, reconnect and the produce path. They are the residue this campaign exists to expose. - **Stack-only files were taken** - the ten foreign clients, the runner registry, the sidecar shim, the self-retiring guards, `Main` and `ParentDeathWatchdog`. - **Records lost, everywhere.** The stack converted five value types from Java records to plain final classes because Jabel rewrites a record into a class with no source positions and Error Prone 2.42.0 crashes rather than reporting on it. This branch never met that only because its root pom had no Error Prone; master's does now, so the conversion is load-bearing here. - **Dependency pins are the stack's**: protobuf 3.25.8 (`requireUpperBoundDeps` refuses 3.25.5 against grpc-protobuf's transitive ask), gson 2.14.0, `error_prone_annotations` 2.48.0 in the proxy and the Java client aggregator and 2.47.0 in the protocol module - deliberately unequal, because their highest requesters differ. - **`clients.yml` is this branch's**, not the stack's. The stack's copy is a smaller earlier file whose own header says it has no conformance lane, no per-language static analysis and no dependency audit; only its `--component clippy` fix and its workflow-level `PC_FOREIGN_CLIENTS_STRICT` were folded in. - **Master's copy won for everything master owns** - the gate scripts, the copyright checker and its self-test, `repo-hygiene.yml`, `docs/ci.md`, `docs/copyright.md`. This branch's only delta in most of those was a copyright header in a pre-canonical form, written before #338 landed the canonical one on master. - **The two ledgers were merged as unions**, not resolved to a side. `docs/inflight/bug-857-family.md` keeps both sides' sightings, this branch's renumbered per the file's own stated convention; `docs/quarantined-tests.md` drops three entries whose owning PRs retired the annotation on master, because a registry entry without a `@Quarantined` test is a hard gate failure. ## The verdict-free work return, re-expressed on master's shape Unit U4 is still on this branch as `WorkManager.onAbandonedResult` and its call sites, and `WorkManager.java` auto-merged keeping them - so resolving `WorkContainer`, `ProcessingShard` and `ShardManager` to master alone would have compiled to nothing. Master moved underneath it: #335 collapsed `inFlight` and `maybeUserFunctionSucceeded` into one atomic `ExecutionState`, and #336 and #373 replaced the shard's raw `availableWorkContainerCnt` with a compare-and-set claim. `ProcessingShard.onAbandoned` and `ShardManager.onAbandoned` are therefore taken verbatim from `feats/proxy-verdict-free-return` (#295), which is cut on current master and is the authoritative re-expression - it routes the restore through `includeInSelection` rather than incrementing a counter that no longer exists. `WorkContainer` is NOT #295's, and the difference is deliberate: that branch marks abandonment with an `AtomicBoolean` cleared by the claim winner, while this branch keys it by delivery (`markAbandoned(long)`, `isAbandonedForCurrentDelivery`, `isReturnForSupersededDelivery`). The delivery-keyed form is strictly more: it tells a late return for delivery n from a live return for delivery n+1, which the boolean cannot, and acting on that confusion ends a running flight and decrements `numberRecordsOutForProcessing` twice. It also needs no clearing on redelivery, so it does not race the claim. Master's own `deliveryCount` - already an `AtomicLong` incremented only by a won claim - supplies the identity, so the field is the one thing added. ## The reconciliation duties #387 and #390 recorded Both are discharged, and the guards they left behind now pass because the gap closed rather than because anything was edited. **`ProxyHarness` and `ConformanceHarness` are one class again.** #387 cut the latter out of the former with the engine lane removed, and wrote into the class header, the module pom and a deferred inflight note that whoever landed the engine must merge them - leaving open which module it ends up in, on the grounds that "the engine lane decides". It decides the sidecar module: the lane needs `ConfigureHandler` and `ProxyProcessor`, and `TestModeMain` - the process a spawned foreign runner actually starts - is in that same test tree and would otherwise have to import the conformance module and close a cycle. So the NAME is the extracted rung's and the MODULE is this branch's, and `parallel-consumer-proxy-conformance` reaches it through the proxy test-jar its pom already declared. The stack's refinements to the class survive: `Delivery` stays a plain final class, and the `startEmbeddedClient` lane stays beside the restored `startEngine`. **`ConformanceDriver.spawnAgainst` is back**, with `LanguageRunner` a `ConformanceBinding` again - it stopped being one on #390 for exactly as long as there was no engine to spawn against - and the ten foreign bindings are registered in `ConformanceBindings.selectable()`. `ConformanceBindings.deferredUntilTheEngineArrives()` is kept and now returns an empty list, which is the honest reading: nothing is deferred. It is not deleted because `TheEngineArrivingMustBringTheForeignCellsTest` reads it, and because the next capability to arrive without a cell needs that list rather than a new one. ## Two duplications the merge surfaced, resolved rather than left - **The protocol documents existed twice.** #383 moved `protocol-specification.md` and `client-authoring-guide.md` into `parallel-consumer-proxy-protocol/docs/` and predicted the rename conflict here. The protocol module wins on the merits - all three artifacts are the contract rather than the server - but the CONTENT is this branch's, because the stack's copies carry "no client generates from this schema yet" and a `git show` fallback for a Go generator script that exists here. `SpecificationCoverageTest` moved with them; every citation was re-pointed. - **`HarnessScenario` existed twice**, once per harness. The stack's plain-class version won, in the sidecar module beside the class it serves. ## Five Java records had to become plain final classes, or nothing compiled `master` gained Error Prone after this branch forked, and Error Prone 2.42.0 does not report on a Jabel-desugared record - it crashes, failing the whole compilation with a message attributed to line 1 of an unrelated file. #387 hit this one rung down and converted its five value types; the same thing was waiting here for `ProxyProcessor.ManifestOutcome`, `ManifestReconciler.Reconciliation`, `InFlightRegistry.InFlight`, `LivenessSettings` and `TestModeMainTest.Run`. Neither term can move - the root pom pins Error Prone at 2.42.0 because 2.43.0 needs a JVM this build cannot use, and Jabel serves the release 8 target - so the records lost, which is what the rest of the repository already does. `grep -rn "record "` now finds none. ## Two defects this merge introduced and then fixed - **The default lane ran a test that needs a profile.** Keeping BOTH sidecar-spawning tests - this branch's engine-backed `OneRecordThroughTheSidecarTest` and the stack's engine-less `SidecarHandshakeTest` - left the Kotlin and Scala exclusion property naming only the first, so the second ran in an ordinary build with no `sidecar-classpath.txt` and failed. Both are now behind one regex, which is also the arrangement that makes the pairing legible. - **Every proxy module's data record existed twice.** This branch keeps module-maturity and testing-evidence records in per-module `docs/data/*.d/` fragments; the stack, which never imported that mechanism, wrote them into the monolithic files. The merge kept both and `bin/check-docs-data.sh` went from green to 37 structural problems. The fragments win, and deleting the monolithic copies also deleted eleven rows asserting "NO RECORD HAS CROSSED THE WIRE" - true on a stack whose sidecar hosts no engine, false here. ## How it was verified JDK 17, macOS. Full default reactor `./mvnw --fail-at-end test`: **BUILD SUCCESS**, every module green including the conformance suite - so the `java-grpc` cell runs against a live engine for the first time, and `GrpcSpikeConformanceTest` answers all five scenarios over a real gRPC stream. The four self-retiring guards #387 and #390 left behind all pass, and **none of them was edited**: `TheEngineArrivingMustBringTheGrpcBindingTest`, `TheEngineArrivingMustBringTheGrpcCellTest`, `TheEngineArrivingMustBringTheForeignCellsTest` (which asserts about every registered language, not one) and `SelectorMatchingNothingFailsTest`. `bin/check-all.sh` real exit code 1, with three gates failing and all three inherited, established against the pre-merge tip rather than assumed: `check-inflight-tags` reports the same 53 problems before and after; `check-file-refs` was already red for the `@`-prefixed bridge imports that #378 fixes; and `check-branch-self-reference` did not exist on this branch before the merge - its findings are on notes inherited from `master` byte-identical. `check-proto-lint` and `check-proto-breaking` report CANNOT RUN for want of `buf`. **Not run locally: `-Dpc.foreignClients`.** It builds C++ and Swift inside containers, and this box is under a low-disk warning that names a container build as what would tip it over. CI's per-language rows are where that lane runs. ## Two findings recorded rather than fixed - `ParallelEoSStreamProcessorTest.processInKeyOrder` failed its own preamble sanity check once here - and the control arm reddened `master` HARDER: unmodified `master`, uncontended, failed all three parameterisations where this branch under concurrent load failed one. That refutes the existing ledger's "the base branch was green" row, and is the first evidence the flake is master-state. Recorded in `docs/inflight/test-processinkeyorder-sanity-check-races-the-first-poll.md`. - One `java-grpc` conformance run left its last record unsettled, 1 of 2 full-reactor runs and green 3 of 3 in isolation. `core` and `java-direct` were green in the same run, so the suspect is the engine's settle path under a full ceiling rather than the scenario. The scenario was NOT weakened - `docs/inflight/proxy-a-java-grpc-ceiling-run-left-its-last-record-unsettled.md`. ## What is deliberately NOT done here `Main#sessionServiceFactory` still returns `NoEngineSessionService`, so the production entry point hosts no engine. That is unit U10's, which #384 named as still having no PR, and it owns the drain that must land with the engine - plus eight cross-language handshake tests assert the `UNIMPLEMENTED` refusal by status code specifically. Recorded rather than faked, in `docs/inflight/proxy-the-production-entry-point-still-hosts-no-engine.md`. The engine is exercised end to end through `TestModeMain` regardless.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…e either, so the bound is not the cause The handoff note left one experiment running: whether a larger bound makes ShardManagerLincheckTest's stress arm find its violation. It does not - iterations(500) against a committed iterations(50), on the clean tree, missed both runs. That is worth more than a repeat of the earlier misses, because it closes the reading the lane's own notes make most attractive. The 3.4x machine-dependence recorded for these stress arms predicts that a bound priced on a fast machine misses on a slow one AND that spending more search finds it anyway. Ten times the budget finding nothing is inconsistent with that, and so is the control: reintroducing the defect changed nothing either. Neither more searching nor a present bug produces a violation on this box, which means the harness is not reaching the seam here at all rather than reaching it rarely. So the next agent is pointed at "why is it unreachable" rather than at a bound to tune - with the candidates named, cheapest first, and with the cheapest move of all stated plainly: confirm the arm still fires SOMEWHERE, on the machine that wrote it or in CI, before spending anything on the harness. Two of the three candidates only exist because #335 and #373 also rewrote ProcessingShard after the harness was written, which is exactly the thing the lane's note warns is not derivable by reasoning: re-run the lane, never reason about it.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…s there, the arm still does not The note handed over one cheap next move: confirm the Lincheck stress arm fires *anywhere* before spending anything on the harness. CI ran the lane on this PR's head for the first time and answered it. `ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear` missed again, with the identical message, on `ubuntu-latest` - four cores, the opposite end of the range from the 32-core box the earlier evidence came from. The decisive part is not the miss but what passed beside it. On the same runner in the same JVM, `LincheckToolchainProbeTest`'s stress arm and `PartitionStateLincheckTest`'s stress arm both found their violations. The toolchain probe exists to answer exactly that question: Lincheck's stress strategy works there, the JDK is adequate, and the `-Plincheck` flags are landing. So two of the three surviving candidates are ruled out - a JDK or Lincheck version difference, and a host property such as core count - and one is left standing: this arm's operation set no longer reaching the seam it was written against, after #335, #336 and #373 each rewrote `ProcessingShard`. That also re-reads the earlier control. Reintroducing the check-then-act produced no violation either, which was ambiguous under the three-candidate list and is corroborating under this one: if the operations no longer reach the seam, restoring the defect at the seam changes nothing, which is what was measured. Records the cost correction too - 7m42s hosted against ~2m50s locally, 364s of it `WorkManagerLincheckTest`. The matrix entry's `timeout: 20` still covers it; the `~2m30s` in its comment does not. Evidence: run 33511221453, job 99867204560, head 58c8c7c.
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.
5 tasks done
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.
This was referenced Sep 5, 2026
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
Master gained the conservation-derived RecordPopulation (#336) and the atomic compute() admission in ShardManager.addWorkContainer (#437), both of which touch the ProcessingShard constructor this branch also extends with DispatchScanMeter. The PR body predicted a one-line resolution at that constructor; it came to three files plus one compile fix: - ProcessingShard: both fields kept. RecordPopulation stays first so master's parameter order is unchanged and the meter is appended as the last argument of the Lombok required-args constructor. - ShardManager: master's compute() block is taken whole, with dispatchScanMeter added to the constructor call inside it. Both shared instances are fields on the manager. - ShardManagerTest and ShardPopulationRaceTest: master's new call sites pass a meter; the test-local shards get a fresh DispatchScanMeter since nothing reads it there. The scanMeter.onEntryExamined() increment auto-merged into the rewritten scan loop (#373) still sits once per examined entry, so the parity test's exact totals are unchanged. Verified: OrderingModeDispatchParityTest, ShardManagerTest and ShardPopulationRaceTest green under JDK 17. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqkUNK5K7JXkbbGoNFTvfB
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ever a fresh replacement The poller's stale sweep asked each occupant whether it was stale and then removed it with `removeWorkAtOffset(entry.getKey())` - by KEY. The staleness answer is about one container; the removal is about one offset. Between the two statements the occupant can change, and it does: the sweep runs on the broker-poll thread inside a rebalance callback, while `addWorkContainer`'s stale-replacement branch runs on the controller, and nothing orders them. The harm is a LOST RECORD. The fresh container is evicted while `PartitionState` still carries its offset as incomplete, so nothing selects it and nothing completes it - the commit high-water mark for that partition cannot advance past it until the partition is re-polled. The accounting half was already closed (#336, #373), so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left. THE OBVIOUS FIX IS WRONG, AND IT WOULD HAVE SHIPPED LOOKING CORRECT. `computeIfPresent(offset, (k, v) -> v == inspected ? null : v)` reads as airtight - the function only asks for a removal if the current value IS the container the sweep inspected. Measured against the JDK 17 source and then by experiment, it is not. `ConcurrentSkipListMap.computeIfPresent` commits a removal through `doRemove(key, v)`, which RE-READS the node's value and gates on `value.equals(reRead)` before its compare-and-set: else if (value != null && !value.equals(v)) // v was just re-read from the node break outer; else if (VAL.compareAndSet(n, v, null)) { ... } `WorkContainer.equals` is topic, partition and offset only, so a replacement landing in that gap is equal-by-offset, passes the gate, and is removed. Reproduced directly: with an offset-only-equals value type, a put landing inside the remapping function leaves the map EMPTY - the replacement destroyed - while the identical program with an identity-equals value type returns the replacement and keeps it. So the identity check narrows the window from "the whole staleness check" to "a few instructions inside the JDK" and closes nothing. No production-seam test can tell the two apart, which is why this is written down rather than merely avoided. `workMap.remove(offset, container)` fails for the same reason one level up: the JDK's compare-and-remove is DEFINED by `equals`, so with offset-only equality no map API can express which container was meant. The value type's equality IS the contract. THE FIX. The shard's map stores a `Residency` token rather than the container. It overrides neither `equals` nor `hashCode`, so its equality is reference identity, and `Map.remove(key, value)` - one atomic step, fully specified, independent of which side the implementation compares - means exactly "remove this occupancy, or nothing". There is no interval for a writer to land in, so there is no window left to narrow. Nothing evicted is a correct outcome, not a failure: the replacement won the offset, so the call retires nothing and REPORTS nothing. That matters downstream - `ShardManager.removeStaleContainers` feeds the returned list to the retry queue, and #437 pinned that the queue removal is reached only through a real shard removal. Reporting a container this call did not remove would break that gating and take the retry entry at coordinates the fresh container now owns. The red test's assertion that the sweep reports the inspected container is corrected accordingly; the assertion that pins the defect - the fresh container survives, by reference identity - is untouched. `getWorkIfAvailable`'s last-resort stale removal has the same shape and is written the same way, with a dated cleared-suspicion note saying why the race is not reachable there today (`addWorkContainer` is the only writer and runs on the controller, which also runs that scan) and what would reopen it. REJECTED: giving `WorkContainer` identity equality, which would delete the token and fix every value-conditional operation on a container at once. It is breaking - `WorkContainer` is public and its `compareTo` orders by offset, which would become inconsistent with equals - so it is queued in docs/refactoring.md under the next major. Also rejected: a remove-then-put-back repair, and a claim the sweep takes before removing which the writer must then wait out; both reintroduce a window, one in the map and one on the controller. EVIDENCE. The defect arm was red against master and is green here. Ablation: restoring `removeWorkAtOffset(entry.getKey())` in the sweep and changing nothing else sends exactly that arm red - the control arm and the premise arm stay green. A third arm pins the premise itself, that no removal keyed on container equality can say which container it meant, `computeIfPresent` included; it is also the tripwire that goes red the day `WorkContainer.equals` becomes identity-based, saying the token can then be deleted. Whole `state` package green, full unit suite green across every module, every `bin/check-all.sh` gate passing. SAME-DEFECT-CLASS SWEEP over core main - a removal keyed on something that does not identify the element that was inspected. One real hit, not fixed here: `ExternalEngine.holdingDispatchPermit` is a `Set<WorkContainer>` whose permit accounting is per RECORD while its membership is per OFFSET, so two containers at one offset leak a dispatch permit permanently. Recorded with its reachability argument in docs/inflight/bug-dispatch-permit-set-cannot-tell-two-containers-at-one-offset-apart.md. Dismissed with reasons: `ProcessingShard.onSuccess` (a non-stale resident is never replaced, and a stale result never reaches it - `handleFutureResult` short-circuits); `removeWorkAtOffset` from the revocation path (emptying an offset regardless of occupant is its purpose); `RetryQueue` (keys by `WorkContainerKey` deliberately, and removes the value it just looked up, under its write lock); the metrics and counter maps (keyed by an identifier that fully identifies the thing); `OffsetSimultaneousEncoder.activeEncoders` (per-instance, single-threaded encoding path). Resolves docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md, whose mechanism, rejected alternatives and open decision are migrated to docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md - the other side of the same class as the confluentinc#909 write-up, which is cross-linked from it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Each of these notes carried a PROPOSED vetting marker from the 2026-09-07/08 sweep. The owner accepted each proposal as stated; this applies them, and every claim was re-checked against the tree at this HEAD before the edit, not against the marker's own reading. Each surviving note's marker now records what was applied and what was verified. bug-shared-collections-across-the-poll-boundary.md - closed. Both named defects are fixed on master: PCMetrics.registeredMeters is a final LinkedHashSet with every add, walk and remove under @synchronized("metersLock") and close() walking it through the never-throwing removeQuietly (758744b, #120 / #57), and RemovedPartitionState.READ_ONLY_EMPTY_SET is Collections.emptySortedSet() (6aae299, #267). The file is kept rather than removed because static-infer-findings.md and core-139-public-api-thread-safety-contract.md both cite it; its "Live on master" headline is corrected and the branch section reduced to what is still true of fix/concurrent-collection-sweep, which still exists on origin with no PR. bug-torn-read-family.md - shrunk to the racing-double unification, the next hunt iteration and the out-of-family pointers. All three candidates are fixed and verified in the tree, so their dossiers were a FIXED narrative; they are replaced by a summary plus a history pointer, with candidate 1's bootstrapPhase-versus-dirty disjointness argument kept because it is the safety argument for the encoder's single-sample read and is recorded nowhere else. bug-177-commit-response-timeout-unreproduced.md and bug-857-mirror-attributions-unconfirmed.md - each shrunk to its #175 half. #177 was closed as completed on 2026-09-01, one second after #399 merged, with no comment naming either candidate mechanism - the outcome both notes existed to prevent. That is recorded once, in the attributions note, which owns the attribution question; the other cross-references it. Candidate 3 is closed: #29 and #57 have merged and PCMetrics.removeQuietly now swallows everything meterRegistry.remove throws. The filename keeps #177 so citations of it survive. bug-stale-sweep-iterator-evicts-fresh-replacement.md - shrunk to the remove-by-key defect, which is live: removeStaleWorkContainersFromShard still removes by key, so a fresh container the controller put at that offset is what leaves. The mechanism sentence is re-anchored on removeWorkAtOffset, and the #373 counter/stall block is removed - #336 (3e668a4) closed it and the note's own last paragraph already contradicted its middle. bug-857-transactional-revoke-wait.md - re-premised on #466. The unbounded spin is gone: in transactional mode the revoke callback posts to the control thread and waits bounded by commitLockAcquisitionTimeout (commitOnRevokeViaTheControlThread). What survives is whether that bound is right against confluentinc#803's complaint that it burns max.poll.interval.ms, which #408 holds along with its RebalanceEoSDeadlockTest amendment. Four stale citations repaired: the "no open PR addresses it" claim, the dead fix/bound-revoke-transaction-wait branch, the renamed core-recoverable-producer-fencing.md, and the two file:line citations, replaced with greppable anchors. bug-retry-queue-write-lock-on-the-rebalance-path.md - the defect is intact and stays the subject; "not started, and no design agreed" is replaced with #431, the open fix, which RetryQueue.remove's own javadoc already names as the tryLock() path being added. bug-wedged-after-poisoned-transaction.md - merged into bug-poisoned-transaction-not-aborted-while-running.md, the survivor the owner chose, and removed. Folded in: the answered recovery question and its answer (isTransactionOpen() is producerState.equals(BEGIN), so lazyMaybeBeginTransaction never opens a replacement and nothing in band aborts the poisoned one), the observed wedge, the two mitigations, and the freeze-behaviour test suggestion. Two citations repointed at the survivor; the third, in process-candidate-ranking.md, names the merge itself and is correct as written. bug-857-family.md - shrunk and retyped as a register, which is what its own closing sections describe it as; the impact stays stall. The revoke-path deadlock's mechanism section is retired per the file's own criterion now that #29 has merged; the "reproducer cannot settle anything" section is replaced with what the rewritten RebalanceEoSDeadlockTest and its purpose-built sibling Rebalance857CommitSyncDeadlockProbeIT establish; and the fifth item's claim that ProgressTracker.withDiagnostic has never been wired is corrected - MultiInstanceRebalanceTest now supplies describeFleet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W511R2TZFAqEiqgS5yogTv
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…moves only the container it inspected (#468) BREAKING CHANGE: RecordContext equality changes. Its Lombok @EqualsAndHashCode covers the WorkContainer it wraps, so two contexts built from different containers for one record no longer compare equal - a Set, a Map key or an equals check over record contexts that used to collapse them now keeps both. Narrow in practice: ConsumerRecord does not override equals either, so two contexts from two different polls were never equal; what changes is two contexts over the same ConsumerRecord instance. WorkContainer itself is public in modifier only and is not part of the surface a user is expected to hold. THE DEFECT. The poller's stale sweep asked each occupant whether it was stale and then removed it by KEY, with removeWorkAtOffset(entry.getKey()). The staleness answer is about one container; the removal is about one offset. Between the two statements the occupant can change, and it does: the sweep runs on the broker-poll thread inside a rebalance callback, while addWorkContainer's stale-replacement branch runs on the controller, and nothing orders them. The harm is a LOST RECORD. The fresh container is evicted while PartitionState still carries its offset as incomplete, so nothing selects it and nothing completes it - the commit high-water mark for that partition cannot advance past it until the partition is re-polled. The accounting half was already closed (#336, #373), so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left. It is the same fresh-replacement-at-one-offset class as the confluentinc#909 write-up, reached from the other side. THE FIX IS IN THE VALUE TYPE, NOT THE REMOVAL. Map.remove(key, value) is the JDK's compare-and-remove, and it decides "still mapped to the value I inspected" with equals - so what that call MEANS is a property of the value type and not of the map. WorkContainer.equals was topic, partition and offset only, so a stale container and its fresh replacement compared equal and no map API could express which of the two was meant. The equals/hashCode pair is deleted, equality is reference identity, and workMap.remove(offset, inspected) is a true compare-and-remove: it evicts the container the sweep inspected or nothing. Nothing evicted is a correct outcome, not a failure: the replacement won the offset, so the call retires nothing and REPORTS nothing. ShardManager.removeStaleContainers feeds the returned list to the retry queue, and #437 pinned that the queue removal is reached only through a real shard removal; reporting a container this call did not remove would take the retry entry at coordinates the fresh container now owns. getWorkIfAvailable's last-resort stale removal has the same shape and is written the same way, with a dated cleared-suspicion note saying why the race is not reachable there today and what would reopen it. REJECTED, and why each would have shipped looking correct: - computeIfPresent with an identity check in the remapping function. ConcurrentSkipListMap commits the removal through doRemove(key, v), which RE-READS the node value and gates on value.equals(reRead) before its compare-and-set, so with offset-only equality the identity test does not survive to the commit. Reproduced: a put landing inside the remapping function left the map EMPTY under offset-only equals and kept the replacement under identity equals. It narrowed the window to a few instructions inside the JDK and closed nothing. Under identity equality the same arm shows the gate declining directly: the remapping function runs TWICE, the first pass planting the replacement and asking for a removal that doRemove refuses, the second pass being handed the replacement. That is recorded at the site rather than asserted, since an implementation reaching the same gate without looping would be equally correct. - A ProcessingShard.Residency token as the map value, overriding neither equals nor hashCode. An earlier revision of this branch shipped it. It bought exactly one correct removal and left the value type wrong, so the next value-conditional site would have had to remember to wrap. Replaced before merge on the maintainer's call: 0.6.0.0 is the breaking release and already carries a Breaking section. - A remove-then-put-back repair, and a claim the sweep takes before removing which the writer must wait out. Both reintroduce a window, one in the map and one on the controller. THE OBJECTIONS THAT HAD QUEUED IDENTITY EQUALITY AS FUTURE WORK, each checked and each fallen: Comparable only RECOMMENDS consistency between compareTo and equals; SortedSet and SortedMap REQUIRE it, and nothing in main code puts a raw container in either - RetryQueue sorts by its own WorkContainerSortKey and de-duplicates by its own WorkContainerKey, so compareTo and its comparator are unchanged with the inconsistency stated in the javadoc. The one collection keyed on containers elsewhere, ExternalEngine.holdingDispatchPermit, has been an IdentityHashMap-backed set since #342, so identity equality makes the container agree with that code rather than changing it. The public break is RecordContext, stated above. THE OTHER HALF OF THE CONTRACT IS ENFORCED, NOT WRITTEN DOWN. Nothing puts a raw container in a sorted collection today, and both RetryQueue and ProcessingShard.workMap are one refactor away from being written the other way round with nothing going red. WorkContainerIsNeverInASortedCollectionArchTest reads every field, return type and parameter in main code whose declared type is a sorted set, sorted map or priority queue and whose FIRST type argument - the element-or-key position for all of them - is WorkContainer, so a container in a sorted map's value position passes. Proved red on main code with a temporary TreeSet<WorkContainer> field on ProcessingShard, which reported that field and not the workMap declared beside it. A standing positive control, in the RebalanceCallbackRuleControlTest shape, hands a hand-imported fixture to the real rule object so the rule cannot go quietly blind. Its limits are in the test javadoc: a body-local collection is invisible, the comparator is invisible so a legitimate Comparator-taking construction is still reported, and it covers core's classpath - measured, no main source outside core declares any of those types. EVIDENCE. ShardStaleSweepReplacementEvictionTest drives the seam deterministically rather than by racing: ShardSeamTestBase's spied PartitionStateManager lands the controller's replacement at an exact instruction inside the sweep. The defect arm was red against master. Restoring the old coordinate-based equals/hashCode and changing nothing else sends the defect arm and the premise arm red - "expected specific instance ... but was: null" and "expected not to be" the same coordinates - while the control arm stays green, as it must: a sweep with nothing racing it never has to say which of two containers it meant. Restoring removeWorkAtOffset(entry.getKey()) in the sweep sends exactly the defect arm red. The premise arm, twoContainersAtOneOffsetMustNotBeInterchangeable, asserts the equality contract directly and is the tripwire for a reintroduced coordinate-based equals. TREE-WIDE SWEEP for what identity equality breaks: one test assertion, ShardManagerTest's isNotEqualTo between containers at different offsets, became vacuous and is restated against RetryQueue.WorkContainerKey, which is what the queue keys on. PollContext.getByTopicPartitionMap's Set de-duplication becomes a no-op with no observable change, said in an @implNote at the site. Checked and found empty: no ArgumentCaptor or Mockito matcher over a container, no Truth containment assertion over containers except on the same instance, no container as a Map key or Set element in any test helper, and every Lincheck harness returns primitives from its operations in stress mode. SAME-DEFECT-CLASS SWEEP over core main, with a correction to this branch's own earlier result: an earlier commit reported holdingDispatchPermit as a real unfixed hit and opened an inflight note for it. That defect does not exist - the sweep matched the declared type Set<WorkContainer> and stopped, and a collection's semantics live in its initialiser, not its declaration. The note is deleted and the lesson is in the solutions write-up. Dismissed with reasons: ProcessingShard.onSuccess, removeWorkAtOffset from the revocation path, RetryQueue, the metrics and counter maps, OffsetSimultaneousEncoder.activeEncoders, MDC.remove. CI SIGHTING RECORDED, NOT DIAGNOSED. Chaos Pain Suite shard 4 went red once on ChaosChurnStormIT's ZOMBIE_MEMBER/REBALANCE_BLOCKED arm with maxInstanceStall=0ms; that line has an open ledger on master, and seed 1053013618367208111 is appended to docs/inflight/test-857-churn-storm-async-stalls.md because the seed dies with the CI log. Nothing on the rebalance path reads WorkContainer.equals, and the shard was green on every later push. DOCS. docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md is resolved and deleted, its mechanism and rejected alternatives migrated to docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md, cross-linked with the confluentinc#909 write-up. docs/refactoring.md's queued-breaking-change section now states that an entry is deleted in the PR that lands it, because the commit message carries the release-note content and the section lists only what is still queued; the entries marked DONE there are removed, and the two citations that pointed at this branch's own entry - WorkContainer's class javadoc and the solutions write-up - name this commit as the record instead. docs/inflight-tool.md used the resolved note as the worked example for the vet command; the example now names a surviving note with the same data-loss signal, bug-run-length-plausibility-ceiling.md. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracted from the review thread on #336 (thread 3862087445), deliberately left open there because fixing it properly is a redesign rather than a review edit. Based on
master, not on #336 or #335.The defect
ProcessingShard.remove()decided whether to deduct a unit of the shard's available-work count by asking the containerisAvailableToTakeAsWork(). That predicate cannot answer the question being put to it. It describes what a container is at an instant; the question is who already spent its unit, which no instantaneous state records. Two containers with opposite accounting histories - one never taken as work, one taken and then released byendFlight()- are indistinguishable through it. A perfectly fenced read would not help.The reported instance: a record is out at a worker, so selection already spent its unit. Its partition is revoked, the controller drops the now-stale result (
handleFutureResult->endFlight(), which tells the shard nothing), and the poller's revocation sweep then removes the entry - deducting a second unit, because the container reads as available again. The deficit is permanent: it survives every later add and resyncs only if the shard drains far enough for the floor-at-zero clamp to absorb it. While it stands, records that genuinely await selection are hidden fromWAITING_RECORDSand fromdrain()'s check that nothing is still awaiting processing - so PC can transition to CLOSING with work still queued.First question: should the counter exist at all? — No, but not yet, and not here
Every reader of
availableWorkContainerCnt, and what each needs:WorkManager.isSufficientlyLoaded()->shouldThrottle()-> broker-poller pause/resumeAbstractParallelEoSStreamProcessor.drain()viaWorkManager.isRecordsAwaitingProcessing()> 0, rarelyanyMatch, and only during drainWAITING_RECORDSgaugeShardManager.getWorkIfAvailablelog.isDebugEnabled()So three of four readers could derive. The load gate cannot, and it is not optional: it is read on the broker-poll path at control-loop cadence.
Corrected after review: "the load gate needs O(1)" is slightly overstated as written.
getNumberOfWorkQueuedInShardsAwaitingSelection()already callsretryQueue.getQueueSizeAndNumberReadyToBeRetried(), which this PR's own sweep classifies as a scan - so the poll path already pays O(retry-queue-size). The precise claim is that the shard counter is what keeps it from ALSO being O(total queued records), and the retry queue holds only currently-failed records while the shard entries are the 10k-50k regime the benchmark measures. The conclusion is unchanged.Measured, not assumed - real
WorkContainers in aConcurrentSkipListMap, scanningisAvailableToTakeAsWork(), againstAtomicLong.get():~4.5ns per entry: 45us per call at 10k buffered records, 236us at 50k. At control-loop cadence that is roughly 4-24% of a core spent answering "is the pipeline loaded", for something that costs nothing today. #361 is concurrently building an O(1)
ShardOccupancyindex precisely because shard scans are the cost - a scan-based replacement would be superseded on arrival and collide with it head-on.The counter is also not merely a gauge:
drain()reads it, and reading low closes early. So deletion could not be justified as "removing a gauge" either.Deletion becomes viable once #336 lands - that PR removes the load gate's dependence on this counter, which is the only reader that needs O(1). Nothing in this PR blocks it; if anything it makes it easier, because the counter now has one coherent meaning to remove.
So: fix the ownership
WorkContainercarries anAtomicBooleanrecording whether it currently holds a unit of its shard's count. The shard claims and releases it by compare-and-set, and only the caller that wins the transition moves the counter. Every site then acts on a fact it owns rather than a state it reads.countAsSelectableclaims first and confirms residency second - deliberately, since the reverse order is the same check-then-act - and both halves being CAS-guarded means the unit is returned exactly once however the two interleave.This is the shape #336's own note proposed ("move 'does this container hold a unit of its shard's available count' onto the container as an ownership flag the shard claims and releases with a CAS").
Two consequences worth stating:
WorkContainer.inFlight, which is a plain non-volatilebooleanread across threads. The unfenced half of the finding is closed for this path without waiting on fix(core): make the work claim one atomic transition on a per-attempt identity #335.inFlightitself is still plain -getCountWorkInFlight()andisAvailableToTakeAsWork()still read it - and fix(core): make the work claim one atomic transition on a per-attempt identity #335 remains the fix for that.Rejected alternatives: deleting the counter (measured above); clamping or flooring the drift, which is what hid this class of bug in the first place.
Relationship to #335. This would be simpler on top of #335's
AtomicReference<Execution>- the claim CAS's outcome would already tell the caller whether it owned the transition, so the unit could ride on the same transition instead of needing its own flag. It is implemented standalone anyway, per the brief. When #335 lands,holdsShardAvailableUnitshould be folded intoExecutionso there is one atomic transition rather than two; the shard-side call sites (countAsSelectable/uncount) do not change shape.Same-defect-class sweep
Class: a shared counter whose spend is inferred from observable state rather than owned by an atomic outcome.
Instances found and fixed here, all in
ProcessingShard:remove()- the reported double deduction.remove()again, sign reversed - a failed record holds a unit but is notisAvailableToTakeAsWork()until its retry delay passes, so revoking it inside that window deducted nothing and left the count one too high, permanently. An inflated count keeps the poller's load gate closed, which is the Paused consumption across multiple consumers confluentinc/parallel-consumer#857 silent-stall signature.addWorkContainer's stale-replacement branch - deducted nothing "since we're replacing, not adding", true ofentriesand false of the counter. Recorded as a known defect indocs/inflight/bug-processing-shard-available-work-undercount.mdon test(core) confluentinc#909: reproduce the stale-resident race through a shard-manager seam, and let evidence justify a quarantine #322's branch.onFailure()took no argument and incremented unconditionally, whileShardManager.onFailuredocuments itself as idempotent. It was not.Checked and dismissed, with why:
WorkManager.numberRecordsOutForProcessing- the sibling counter in the same throttle calculation. Its five mutation sites pair 1:1 with hand-out and result-return; the branch inhandleFutureResultselects which handler runs, it does not ask "was this already counted". Same family (a drifting counter feeding a throttle) but a different class - unfenced non-atomic access, already tracked indocs/refactoring.md.ConsumerManager.pendingRequests-addAndGet(+1)/addAndGet(-1)paired bytry/finally, so the pairing is structural, not inferred.PartitionState.getNumberOfIncompleteOffsets()- derived fromincompleteOffsets.size(), no separate counter to drift.PartitionStateis in fact the good example:boolean removedFromIncompletes = this.incompleteOffsets.remove(offset) != nulltakes its answer from the atomic map operation's outcome, which is exactly the pattern applied here.RetryQueue.getQueueSizeAndNumberReadyToBeRetried()- the ready count is a scan, so it cannot drift by inference. Its known defect (docs/inflight/bug-retry-queue-orphaned-by-inline-stale-removal.md) is an unpaired removal between two structures - a different class, untouched here.PartitionStateManager.epoch++- a monotonic version, not a spend.WorkContainer.numberOfFailedAttempts++- per-container, single writer.slowWorkCounters,succeededRecordsCounters,failedRecordsCounters- monotonic Micrometer meters, nothing to spend.BrokerPollSystem.pausedForThrottling- a check-then-act, but on a boolean and confined to the poll thread. Not a counter, no drift possible.parallel-consumer-vertx,-reactor,-mutiny: no counters at all in main code (grep -rnE '(\+\+|--|incrementAndGet|decrementAndGet|getAndAdd)'over theirsrc/mainreturns nothing outside loops and logging).Prior art (all six checks, including the empty ones)
ls docs/plans/then grep - nothing on this counter or on shard available-work accounting.grep -rloverdocs/solutions/- one hit,stale-container-blocks-fresh-work-same-offset-after-rebalance-2026-08-07.md, whose "Follow-up 1" records instance 3 above and was never actioned. Its twofile:linecitations pointed at code this PR deletes; repaired in the second commit perdocs/citations.md, with the claim left untouched.ls docs/inflight/and grep -bug-retry-queue-orphaned-by-inline-stale-removal.md(different class, see sweep),test-truth-probes-for-internal-state.md(names shard queue depth as the top candidate for exactly this kind of probe -ShardAvailableCountOwnershipTestis one),test-lincheck-lane-open-items.md(records that this counter is not a Lincheck target).ProcessingShard|ShardManager) - test(core): a Lincheck lane, calibrated by refinding four real races unaided #347, test(core): stop the core tests jar configuring other modules, and remove the sleeps that stood in for events #265, docs+ci: finish the issue-reference migration, and close the gate behind it #219, fix(core): draining PC stops polling - 10kHz busy-spin + zombie partition hold (#857-family silent stall) #80, fix(core): replace a stale container at a reused offset after rebalance #31. fix(core): replace a stale container at a reused offset after rebalance #31 is the one that introduced the stale-replacement branch and recorded the follow-up; none of them touched the ownership question.--state all(300) - nothing on this counter. The two adjacent ones are Metrics: expose what PC delivers - head-of-line blocking avoided, end-to-end record latency, per-shard queue depth #222 (per-shard queue depth as a metric) and confluentinc#540: Apply backpressure per partition instead of the entire assignment #160 (per-partition backpressure), neither of which is this.Verification
expected 1 but was 0for the reported double deduction,expected 0 but was 1for the sign-reversed case,expected 2 but was 1for the stale replacement,expected 3 but was 4for the idempotence sequence. With the fix restored, 4 of 4 pass.remove()-> 2 red. Dropping the CAS guard on the decrement (unconditionaldecrementAndGet) -> 2 red. Dropping the claim guard on the increment -> 1 red. The guard is load-bearing at every point it is stated.mise exec java@temurin-17 -- bin/ci-unit-test.sh- BUILD SUCCESS, full reactor, all 11 modules.bin/check-all.sh- 15 ran, 15 passed, 0 failed.bin/check-issue-refs.sh- clean.@GuardedByparallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.mdrequires annotating a race fixed in the same change, with a stated carve-out when the fix uses an atomic rather than a lock: "If the right fix isvolatileor removing the sharing outright, there is no lock to name and no annotation to write." That carve-out applies here - the fix is anAtomicBooleanwhose CAS outcome is the ownership record, so there is no lock expression to name. The invariant it establishes is recorded on theavailableWorkContainerCntfield instead, and enforced byShardAvailableCountOwnershipTest#assertCount, which checks the counter against ground truth (countHeldUnitsByScan()) at every step rather than against the expected number alone.Review pass — what it changed, and what it found and did not fix
Three simplification reviewers, three code reviewers, plus the
@claudereview on this PR. Nocorrectness defect was found in the CAS protocol itself - every reviewer that tried to break the
claim/release interleaving failed to. What they did find:
countAsSelectable's residencyrecheck -
if (entries.get(wc.offset()) != wc) uncount(wc)- is the single line that makesclaim-then-confirm safe when the two threads genuinely interleave, and nothing exercised it.
countingBackInARecordThatHasAlreadyLeftTheShardMustHandTheUnitStraightBacknow does, through theproduction seam: a revocation landing between
handleFutureResult's staleness check andsm.onFailurehas the controller count a container selectable again after the poller took it outof the shard. Deleting the recheck makes it red (
expected 1, got 2, ground-truth scandisagreeing) while the other four stay green.
than by clamping" is true at quiescence and false instant-by-instant:
countAsSelectableclaimsand then increments, so a reader in that window sees the scan one ahead - and if a concurrent
uncountwins the release there, the counter momentarily reads -1. Every interleaving stillsettles correct and every consumer reads the aggregate, which floors at zero, so this is a
documentation defect. It is now qualified where the claim is made, not defended in a comment.
getNumberOfWorkQueuedInShardsAwaitingSelectionreads theretry-queue size before the shard sum, justified by "normally retry queue is updated before shard
counters are on work taken". Moving the decrement into the selection loop made that false. The
read order is still right; the reason is re-derived in place with the arithmetic, and the skew is
nil or high - never low, which is the direction that closes
drain()early.A real defect found here and NOT fixed -
docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md.removeStaleWorkContainersFromShardcallsiterator.remove()on aConcurrentSkipListMapentryset, which removes by key - so a fresh container the controller put at that offset since
next()returned is what actually leaves, while the followinguncountreleases the stale object.The primary harm is the lost record and predates this PR. The secondary harm's DIRECTION is this
PR's doing: formerly one low and resynced by the clamp, now one high and permanent, which is the
direction that stops
drain()reaching CLOSING. Not fixed here because there is no identity-keyedremoval to reach for -
entries.remove(key, value)andentrySet().remove(entry)both compare withequals, andWorkContainer.equalsis topic/partition/offset only, so the replacement comparesequal to what it replaced. Closing it is a redesign, the same call that produced this PR.
Stated coverage gaps, rather than quietly left:
drives a seam. The seams are the production windows and the mutation checks show each guard is
load-bearing, but "safe under arbitrary interleaving" is reasoned, not executed.
getWorkIfAvailable's inline stale-eviction branch has no counter assertion. It needs a staleentry at the HEAD of an order-restricted shard scan, and that fixture's realism is the thing in
doubt, not the coverage - so it is named rather than faked. (
removeStaleWorkContainersFromShardWAS in this list and is no longer:
thePollersStaleSweepMustNotDeductForAnEntryThatAlreadySpentItsUnitcovers it, because "it calls the same helper" was the wrong reason to skip a site whose job is to
decide whether to call it. Restoring the unconditional deduction reddens it.)
ShardManager.onFailureadds to the retry queue unconditionally whileshard.onFailurenow netsto zero for a container that has left
entries, so the netting can skew by one in that window.Floored by
max(0, ...), and changing retry-queue membership on a revocation race wants its ownchange with its own test.
Base moved twice: #335 and #345 landed under this branch
masterwas merged in twice. #335 rewrotegetWorkIfAvailable's claim intoone atomic transition, so
uncount()now sits inside the won-claim branch - strictly better, since arefused claim must leave the container resident and still holding its unit. Folding
holdsShardAvailableUnitintoExecutionis therefore unblocked rather than hypothetical, and isthe same follow-up that removes the transient -1 above, because one transition is exactly what closes
that window. Left for its own change: it rewrites
Execution's contract.#345 then rewrote
ShardManager.removeWorkFromShardFor- the method thatcalls
ProcessingShard#remove, and therefore theuncountthis PR added - to the single-readgetShard(key)idiom. Orthogonal, and the auto-merge is right without adjustment: #345fixes WHICH shard reference the sweep operates on, this PR fixes what removing an entry does to the
count, and a detached shard still accounts its own removal correctly because the counter it moves is
its own.
Chaos Pain Suite, and a negative result worth stating
ChaosChurnStormITfired the fleetNO_PROGRESSarm on this branch's own head - logged indocs/inflight/bug-857-family.md. That arm is a long-running,seed-dependent red this ledger has recorded repeatedly on branches that change no
src/mainat all,so it is not caused here. It is worth its own entry because this is the branch on which it should
have stopped firing if the counter overcount were what it had been catching. It fired anyway: this
fix is necessary, not sufficient, for the confluentinc#857 family.
Follow-ups, not done here
docs/inflight/bug-processing-shard-available-work-undercount.mdlives ontest(core) confluentinc#909: reproduce the stale-resident race through a shard-manager seam, and let evidence justify a quarantine #322's branch and is resolved by this PR. It is not on
master, so itcannot be deleted here; whoever lands test(core) confluentinc#909: reproduce the stale-resident race through a shard-manager seam, and let evidence justify a quarantine #322 must delete it rather than
merge it in stale.
availableWorkContainerCntentirely becomes viable oncefix(core): derive the broker-poller load gate by conservation, and pair every count change with a real map mutation #336 lands - that PR removes the load gate's dependence on this counter,
which is the only reader needing O(1). So this fix is not an argument for keeping the counter
forever; it makes the deletion easier, because the counter now has one coherent meaning to remove.
holdsShardAvailableUnitinto fix(core): make the work claim one atomic transition on a per-attempt identity #335'sExecution, now that it haslanded.
statepackage's copied test harness(
docs/refactoring.md).Checklist
docs/solutions/logic-errors/stale-container-blocks-fresh-work-same-offset-after-rebalance-2026-08-07.mdcitation repair; the invariant itself is documented on the field it constrains and enforced by a testShardAvailableCountOwnershipTest, six tests, red before / green after / mutation-checked; the fifth covers the residency recheck and the sixth the poller's stale sweep, both added by the review pass@claude review thison the PR, which is the only route that opens inline review threads (docs/ci.md).@codex reviewwas also requested and returned "you have reached your Codex usage limits for code reviews", so that arm did not run