fix(core) astubbs#121: commit the offset the encoded payload was written against (confluentinc#894) - #337
Merged
Conversation
…t a later one `createOffsetAndMetadata()` sampled the committable offset twice: once inside `tryToEncodeOffsets()` to build the incomplete-offset payload, and again afterwards for the offset to commit. Work completing between the two reads made them disagree, so the metadata could be encoded relative to one base while a higher offset was committed - shifting every decoded incomplete offset by the difference. On the receiving side that reads as an offset reset, which is what the upstream reporter saw under frequent rebalancing. `tryToEncodeOffsets()` now samples `getOffsetToCommit()` once, up front, and returns it alongside the payload in a `ParallelConsumer.Tuple`, so the committed offset and the payload's decode base are the same number by construction. The remaining race is in the safe direction: work completing between the sample and the encode only makes the commit more conservative - a lower offset, at-least-once replay - and the next commit cycle catches up. There is no path that commits ahead of what the payload describes. `PartitionStateCommittedOffsetTest` asserts `getOffsetToCommit()` is computed exactly once per commit cycle, which fails against the old two-read version. This is a cherry-pick of confluentinc#893, whose own body names confluentinc#894 as the issue it fixes. Fixes #121 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J2KjzpiUKg2tGFT7B2D2e
tryToEncodeOffsets() now returns the offset alongside the payload, so its @return described a value it no longer produces. Comment only; no behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
…ss beside it confluentinc#894 was reported in October 2025 and never reproduced. The reviewer asked for a reproducible example on 2025-10-26; none was produced, and confluentinc#893 - the fix carried here - ships no test at all. These two classes are that reproduction, written against unfixed code and green against the fix. The reported symptom. A commit carries an encoded note listing which offsets above it are still outstanding, and that note is written RELATIVE to the offset being committed. The unfixed code reads that offset twice - once to encode the note, once to commit - so a completion landing between the two reads stores a note against a base lower than the offset it is filed under. Restored after a rebalance every offset in it decodes too high, and the consumer eventually asks the broker for a position that was never produced: fetch position out of range, and auto.offset.reset fires. Reproduced at three altitudes, ending on that step. What nobody reported. At traffic rates where real records keep overtaking the fabricated ones, the committed offset tracks the log end offset exactly - no overshoot, no reset, nothing red anywhere - while real records are dismissed by isRecordPreviouslyCompleted against a fabricated offsetHighestSucceeded. They are never processed and never retried. Measured at one record per cycle on a narrow payload and nine to ten on a wide one, continuing indefinitely. That is silent data loss, and it is the more dangerous of the two: the reported fault is loud and recoverable, and this one is neither. It shares the reported fault's root cause and the same fix closes it - zero records dropped across every control-arm cycle. The reviewer's "after multiple rebalances it ends up committing not offset 10 but offset 11" is measured here rather than argued, including the correction that the first sweep got it wrong. Overshoot growth is set by the encoded payload width against the traffic rate, so a sweep holding the width fixed answers a question about its own fixture. At width 10 the overshoot climbs +0 through +9 over ten consecutive rebalances. Controls run in both directions: every assertion flips against the fix, and the same tests with the injected completion disarmed pass on the UNFIXED code, so the failures are caused by the race rather than by the fixtures. The red arm lives on test/894-reproduce-offset-encode-shift and is deliberately never merged - a reproduction has to run against code that still contains the bug, and here the fix is present. Refs: confluentinc#894, confluentinc#893, #121 Co-authored-by: Antony Stubbs <antony.stubbs@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
…side it confluentinc#894 now has a durable write-up rather than living in commit messages: the mechanism, the four preconditions that all have to hold, why the existing round-trip coverage could not see it, the fix, and the prevention rules the investigation actually earned. Two things in it are for whoever reported this. The reproducible example asked for on the upstream PR in October 2025 exists now and is named. And the attachment on that PR - unread for nine months - turns out to hold both the production logs and a "how to make it happen" recipe, which is recorded so the next person does not repeat the search. It also carries the second failure mode plainly: at traffic rates at or above the encoded payload width the committed offset tracks the log end exactly, nothing goes red, and real records are dismissed against a fabricated offsetHighestSucceeded - never processed, never retried. Same root cause, same fix. Stated without dramatising it, and bounded by the preconditions, because the useful thing for a reader deciding whether to upgrade is the shape of the risk rather than alarm. Checked by a grounding pass rather than trusted. That pass swept createOffsetAndMetadata at all 16 released 0.5 tags and confirmed the two-read shape in every one - the method did not exist before 0.5.0.0, so that is the complete set, not a sample. It also caught the doc contradicting itself: it said drops continue indefinitely two paragraphs after saying the run halts. Both are true, in different regimes, and the doc now says which is which. The two measured figures are attributed to this session's run, because the tests report them rather than assert them. Refs: confluentinc#894, confluentinc#893, #121
The javadoc said "one below the highest sequentially succeeded offset" while the body returns getOffsetHighestSequentialSucceeded() + 1. Kafka commits the next offset to read rather than the last one processed, so the code was right and the comment was its opposite. Found by the grounding pass over the confluentinc#894 write-up, which quotes this method's real semantics - a comment stating the inverse of the return statement is exactly the kind of thing someone reasons from when deciding whether an off-by-one is a bug.
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)
|
|
The manifest is the source of truth for fork-to-upstream mapping and the first thing the next agent reads, so it has to name the PR that actually carries the work. Opening #337 is the lifecycle transition, and AGENTS.md asks for the entry to move in the same commit that causes it. Records what the entry did not say before: that the same root cause produces a silent record-loss mode no bug report describes, which is the stronger reason to take the fix; that the approving reviewer was rkolesnev, on 2025-11-17; and where the reproduction and diagnosis now live. Also corrects the contributor attribution - upstream confluentinc#893 is by sangreal, not the name this entry carried. Note for whoever merges either PR: #57 edits this same entry, to stop claiming the work. The two will conflict, and the resolution is this version - the branch that owns the work owns the entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
🧪🔒 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 |
astubbs
marked this pull request as draft
August 24, 2026 05:35
…racy-on-assignment
The release register is what gets read when the 0.6.0.0 notes are written, so this is where the fact has to sit if it is not to be rediscovered. There is no inflight tag that means "revisit at release time" - the only release-adjacent impact is `release-gate`, which means blocks publishing - so the register entry is the mechanism, not a label. What it asks for is that the note not be phrased as the issue title. Reported as an offset reset under frequent rebalancing, the same root cause also has a mode where the committed offset tracks the log end exactly, nothing goes red, and real records are dismissed as already-completed and never processed. Silent record loss, in every released 0.5.x line, undescribed by any bug report because nobody could have noticed it. "Offset accuracy on assignment" would understate that to precisely the readers deciding whether to upgrade. Bounded rather than alarming: it needs four preconditions together, so it is uncommon to trigger - and persistent once triggered, which is the other half a reader needs. Refs: #121, confluentinc#894 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
5 tasks done
check-branch-self-reference.sh fails an inflight note that names its own branch or PR without a human confirming the sentence still reads correctly once the merge lands. The entry added for #337 named it and had no marker, so `inflight: tags` was red and the PR could not merge. The prose needed no rewriting - it is a record of what shipped in the release, which is exactly how it should read afterwards - so this is the block marker around it and nothing else. Verified only by reading the gate, not by running it: the script uses `mapfile`, which needs bash 4, and macOS ships bash 3.2. It dies with `mapfile: command not found` and still exits 0, so a local run reports success having checked nothing. That is the same silent-local-false-negative class as the BSD `stat` fail-open recorded on #57; CI runs it on ubuntu and is the real check here. Refs: #121 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
astubbs
changed the title
…y fired The two reproductions carried near-identical test doubles - same getOffsetToCommit override, same read ledger, same firstOffsetToCommitRead, and a snapshot override differing only in one-shot vs re-armable. The compounding class's javadoc defended the split as "state the sibling does not need", which does not hold: arm once and never re-arm IS the one-shot behaviour, so the re-armable version is a strict superset. Extracted to one package-private class and deleted both copies. Its javadoc no longer claims what it cannot. The old text said a completion after the snapshot "can change neither the payload nor offsetHighestSucceeded". That is true of these fixtures, because every offset they race is the lowest outstanding one and therefore already below the high-water mark - not of the seam. encodeOffsetsCompressed takes its own second read of getOffsetHighestSucceeded() after the snapshot, so a completion ABOVE the mark would widen the encoder's range against a stale set. These tests do not reach that interleaving and now say so. Added the guard the review asked for: an assertion that the injected race actually fired. Without it, both classes pass on the fixed code for a trivial reason - both numbers come from one sample - and would keep passing if the seam silently stopped being called. The first version of that guard was itself tautological, and mutation-checking is what caught it. It inferred firing from `armedRacingOffset == null`, which cannot distinguish "armed, then fired" from "never armed": deleting the arm left the suite green. Firing is now tracked explicitly, and removing the arm turns the precondition red. Verified in both directions after the refactor, because a deduplication that quietly disarms the tests would look exactly like a successful one: 7 of 7 red against master's unfixed PartitionState, 7 of 7 green against the fix. Refs: confluentinc#894, #121 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
The write-up claimed the residual race is in the safe direction and that no remaining path commits ahead of what the payload describes. That is true of createOffsetAndMetadata, which is what the fix changes, and it was written as though it were true of the commit path. It is not. OffsetMapCodecManager.encodeOffsetsCompressed takes two further unsynchronised reads of the same moving state - the incomplete set, then getOffsetHighestSucceeded() as the encoder's range top - and the second can widen the range past what the first was filtered against, which would encode genuinely-incomplete offsets as complete. Same shape as the defect this document is about, one layer down. That code is untouched here, identical to master, and predates the fork (2022), so this is a scoping error in the claim rather than a defect this branch introduced. Found by review, not by me: I named the defect class while writing this up and did not grep for it one level down, which is the exact check AGENTS.md asks for at merge prep. Narrowed rather than deleted. What the fix does establish - the offset to commit is sampled once and cannot disagree with the payload it is filed under - still stands and is worth stating. What it does not establish is whether the payload's own CONTENTS can be built from a torn read. That has its own branch and is being reproduced before anything is changed there, on this document's own principle: a fix without a failing control arm is not evidence. The commit message on f4d4dcb carries the same overstatement and cannot be corrected without rewriting pushed history; this commit is the correction of record. Refs: confluentinc#894, #121 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…tes, recorded before they evaporate The defect-class sweep this branch's merge prep required went wider than the commit path: a three-way audit traced every multiple-read-of-moving-state candidate thread by thread. Three survived and none is reproduced - they are worked interleavings, which this family has twice shown to be worth nothing either way until a control arm settles them. Recording them now because agent report artifacts expire and worked reachability analysis is exactly the kind of knowledge that gets re-derived at full cost. Ranked by consequence: a bootstrap-reset tear whose worst case is a wrong commit cancelling a broker-mandated replay; a check-then-get NPE thrown out of the rebalance listener under KEY ordering; and a staleness check acting on a different lookup than it validated, whose worst traced harm is a permanent retry-queue orphan rather than loss. Also pins the cross-branch seam coupling with #337: this branch's fix removes the encoder call that PR's racing double overrides, and one of its tests can go silently green with a dead seam on a merged tree. Whichever merges second inherits that, and the note says what to do about it. The ~47 dismissals are summarised by class rather than restated - the per-candidate call paths live in the hunt reports; the note is the durable index. Refs: confluentinc#894, #337 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…ds another All three torn-read candidates now have control arms, one day after the note recorded them as worked interleavings. Two reproduced deterministically; the bootstrap-reset tear is refuted as independently reachable - the commit path collects only dirty states, setDirty has exactly one call site, and the only production route that dirties a bootstrap-phase state is candidate 3's own success-path tear. The family composes: fixing candidate 3 closes candidate 1's only door, which was invisible while both were unsettled. Renamed from bug-torn-read-family-three-unconfirmed-candidates.md - the directory's rule is that a filename never carries state, and "unconfirmed" is now false. Nothing cited the old name. Also records the next iteration explicitly: one pass found four actionable items, so once the fixes land and the open PR set merges, the hunt runs again with the same lens - including the out-of-family stragglers and whatever the fixes themselves change. Refs: confluentinc#894, #337, confluentinc#857 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…ouncement material Two additions to the promotional-material list in the release register, at the operator's direction. The campaign as a story: five concurrency defects sharing one root shape found in a week, two of them silent record loss carried by every released 0.5.x line and invisible to users and to every static analyser tried, each reproduced deterministically with control arms, each fixed. That says the fork's priority is correctness more credibly than a feature list, and it speaks directly to the people who reported confluentinc#894 and confluentinc#857. Register: plain and without alarm, matching the release-note guidance recorded for #337. The detectors as a testing feature: the racing-double seam technique, plus the Lincheck/jcstress lane if the in-flight calibration holds - scheduler-controlled concurrency testing as a standing lane is something upstream never had. If the PoCs land pre-release they join docs/data/testing-evidence.yaml; if not they are roadmap material, which the when-is-v6-good-enough note argues is exactly what takes the cramming pressure off the release. Refs: confluentinc#894, confluentinc#857, #337 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
4 tasks done
…erge prep Ground truth for a fresh agent: what the branch is after the session that built it, the decisions that remain and whose they are, the cross-branch couplings that gh cannot show, and the traps already sprung so they are not re-learnt. Pointer- first per the handoff contract - the authoritative artifacts are the PR body, the dossier, and the commit bodies; this file only routes to them and carries what none of them can: the relationships between open branches. Wrapped in post-merge markers because a handoff is by nature a claim the merge falsifies - the file's own first instruction is its deletion in this PR at merge prep, which is what keeps it off master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
This was referenced Aug 25, 2026
New rule, operator ruling 2026-08-25, recorded in this directory's AGENTS.md per its own update-this-file instruction: an inflight note that maps to a GitHub issue carries a DRAFT response to that issue before its PR merges. The agents who did the work hold the best context at merge time; by release time it has to be re-mined from commit logs. Drafts are posted only on explicit instruction - the never-post-unasked rule is untouched - and deleted with the note. This PR is the first application: drafts for #121, confluentinc#894 and confluentinc#893, each carrying what the operator wants the reporters to see - not just "fixed", but the class-level assurance: the family hunt across frontier models, four sibling defects found and fixed, cross-model adversarial review, Lincheck calibrated against the pre-fix code and refinding the bugs unaided, jcstress measuring the residuals on hardware. Credit to sangreal stated unchanged in every draft. The handoff gains the at-merge step pointing at the drafts. Refs: #121, confluentinc#894, confluentinc#893 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
…racy-on-assignment
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
One conflict, in docs/inflight/static-infer-findings.md, on the paragraph describing what RacerD cannot see. Both sides rewrote the same four sentences for the same reason - the branch-self-reference gate fails a present-tense claim about an open PR - and reached different wordings. Kept this branch's, because it is the one that stays true as the family lands, and two of the four have now landed (#337 and #349 are on master). Master's wording says RacerD "misses the four named torn-read races" in the present tense, which reads as a claim about four open defects; this branch's says none of the four appears in its output and that the blind spot is the CLASS rather than the instances, so which of them are still unfixed does not move what the analyser can see. Naming them by their fixing PRs keeps the citations valid after each merges. No content from master's side is lost: its only change was re-tensing the same sentences and moving a post-merge marker, and the marker pair is balanced in the result.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
Master now carries #349 (dirty made volatile) and #337, so PartitionState.java was touched on both sides. Git auto-merged it and both changes survive intact: `private volatile boolean dirty` from #349 alongside this branch's caller-resolved onSuccess/onFailure. Verified by reading the merged file, not inferred from a clean exit. One real conflict, in docs/inflight/static-infer-findings.md, on the paragraph describing what RacerD cannot see. Both sides had rewritten the same four sentences to satisfy the branch-self-reference gate and reached different wordings. Kept this branch's, which is the wording that stays true as the torn-read family lands - two of the four are now on master - because it says the blind spot is the CLASS rather than the instances, so which of them remain unfixed does not change what the analyser can see. Resolved identically on #345, so the two branches still agree on this file.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…hot, and discharge the seam re-hook it inherited Takes #371, so this branch now carries the fixed PIT lane, and #337, which is what made the rest of this commit necessary. THE MERGE BROKE SEVEN TESTS, AND THAT IS THE GUARD WORKING. All three PartitionStateCommitEncodeShift894Test cases and all four PartitionStateCommitShiftCompounding894Test cases failed at their fired-assertions, each naming both the cause and the remedy: the encoder no longer calls getIncompleteOffsetsBelowHighestSucceeded(), which is the exact seam #337's RacingCommitCycleState overrode to inject its race, so every armed race landed on a dead seam. bug-torn-read-family.md predicted this and said whichever of the two landed second would owe the re-hook. #337 landed first, so this branch did it: the double now overrides the bounded getIncompleteOffsetsBelow(long), which catches BOTH entry points because the no-arg convenience method delegates through it in the base class - the same property this PR established for RemovedPartitionState, applied to the sibling double. Worth recording rather than quietly benefiting from: the dossier also said PartitionStateCommitShiftCompounding894Test armed races with no fired-assertion and could go silently green on a dead seam. That was true when written and #337 had fixed it before merging. Had it not, this signal would have been three failures instead of seven, and the other four would have passed while proving nothing. RacingCommitCycleState's javadoc also carried a claim this PR falsifies: that encodeOffsetsCompressed takes a second read of getOffsetHighestSucceeded() after the snapshot, so a completion above the high-water mark would widen the range against a stale set, and that its fixtures never reach that interleaving. The second read is what this PR removes. The caveat is kept rather than deleted - a reader of those fixtures still needs to know they establish the base/payload tear and not the widened-range one - but it now points at OffsetEncoderWidenedRangeRaceTest, which reproduces the interleaving it warned about. One content conflict, in docs/inflight/static-infer-findings.md, resolved toward master: this branch had wrapped one line in an inner `post-merge: checked-begin` to satisfy the self-reference gate, and master has since wrapped the whole surrounding paragraph in an outer block that already covers it, so the inner marker was a redundant nesting and was dropped. Verified: core unit suite 426 tests, 0 failures (417 before this merge - the 9 new tests are #337's, and all 7 regressions are cleared); bin/check-all.sh 15 passed, 0 failed, 0 could not run.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… times, not twice The duplicate-code bot flagged eleven lines shared between OffsetEncoderWidenedRangeRaceTest and PartitionStateCommitEncodeShift894Test, a clone that only exists on a tree carrying both #337 and this branch. Checking before acting turned the finding inside out: the shared code is a private `decode` wrapper around deserialiseIncompleteOffsetMapFromBase64, and grepping for that call finds the same wrapper in SEVEN test files across two packages. The flagged pair is not the duplication; it is the two members of it that happened to be new enough for the bot to notice. That makes extraction a seven-file, two-package change, which does not belong in a concurrency-fix PR - AGENTS.md scopes the duplication follow-up to clones this PR introduced, and this pattern predates it on master. Recorded in refactoring.md instead, which is where a lightweight refactor with no decision or blocker attached goes, with all seven sites named so whoever takes it does not have to re-derive the list. Distinguished there from the racing-double unification already tracked in bug-torn-read-family.md: that one is about the two Racing*State doubles, this one is about the helper, and conflating them would send the next reader at the wrong mechanism.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…hot, silently completing records OffsetMapCodecManager.encodeOffsetsCompressed read moving partition state twice and treated the two reads as one snapshot: the incomplete-offsets set was filtered on its own internal read of offsetHighestSucceeded, then a SECOND read became the encoder's range top. OffsetSimultaneousEncoder marks every offset in [base, rangeTop] absent from the set as completed, so a completion landing between the two reads - of an offset ABOVE the high-water mark - widened the range around a stale set, and every still-incomplete offset in the widened stretch encoded as complete. On restore they are dismissed by isRecordPreviouslyCompleted and never processed: silent record loss, with no error and no lag anomaly. Live in the DEFAULT commit mode. The encode runs on the broker-poll thread while completions land from the control thread; PERIODIC_CONSUMER_SYNC blocks on the commit response and PERIODIC_TRANSACTIONAL_PRODUCER shares a thread, so the window is specific to PERIODIC_CONSUMER_ASYNCHRONOUS. The code dates to 2022 and predates the fork, so every 0.5.x line carries it. THE FIX IS ONE SAMPLE. getOffsetHighestSucceeded() is read once and both the snapshot and the range top derive from it, so they cannot disagree by construction - the same shape as the confluentinc#894 fix in #337, one call below it. PartitionState gains the bounded filter getIncompleteOffsetsBelow(long); the existing getIncompleteOffsetsBelowHighestSucceeded() delegates to it, so no existing signature changed. RemovedPartitionState overrides only the bounded overload, which guards both entry points precisely because of that delegation. THE RESIDUAL CLAIM WAS NARROWED TWICE, AND THAT IS THE MOST IMPORTANT PART. It began as "residual races at this seam are all safe-direction". Review showed that is false for a concurrent bootstrap reset: sampling the mark first pairs the old high bound with a map initStateFromOffsetData may have since replaced, encoding a stale range as complete and cancelling the mandated replay - where the old second read would have observed the reset and narrowed the range instead. So this change is better against the completion seam it targets and worse against that one. It is nevertheless UNREACHABLE, by construction rather than by a gate that happens to be shut: bootstrapPhase has exactly one write site, the first line of maybeTruncateBelowOrAbove, which flips it false with an early return otherwise; that call is reached from maybeRegisterNewPollBatchAsWork BEFORE its addNewIncompleteRecord loop; dirty is set only by onSuccess, which needs an offset registered by that loop; and onPartitionsAssigned always builds fresh instances rather than reusing one. The reset window and the dirty-encode window are therefore temporally disjoint on any instance. An earlier version of this argument made the safety contingent on #346 landing first; it is not. THE TESTS ARE PROVEN AGAINST THEIR OWN MUTANTS, because two of them could not fail. OffsetEncoderWidenedRangeRaceTest's guard proved the injected completion was ATTEMPTED, not that it moved the high-water mark - fixture drift would have left both reads equal and the tests green against the very ordering they exist to catch; the double now records the mark either side and the tests require it to advance. RemovedPartitionStateTest asserted the singleton returns an empty set, which the base filter also does, so deleting the override under test left it green; it now populates a removed state through the un-overridden base path, with a live-partition control arm proving the arrangement discriminates. Each control was demonstrated RED against its own mutant and restored. The two entry points needed different techniques, which is not over-engineering: the bounded overload is guarded once so state discriminates it, while the convenience method is guarded twice - getOffsetHighestSucceeded() is also overridden to KAFKA_OFFSET_ABSENCE - so no arrangement of state can separate delegation from coincidence, and that claim is pinned with an observable dispatch seam instead. RacingCommitCycleState is re-hooked to the bounded overload. #337 landed first, and the fixed encoder no longer calls the seam that double overrode, so all seven of its tests failed loudly at their fired-assertions - the guard working as designed rather than going silently green on a dead seam. Overriding the bounded method catches both entry points for the same reason RemovedPartitionState does. Defect-class sweep, two reads of moving state around one operation: createOffsetAndMetadata is #337's and deliberately not re-fixed here; tryToEncodeOffsets' second read feeds a metrics denominator only, dismissed; isRecordPreviouslyCompleted and getOffsetHighestSequentialSucceeded checked, every interleaving lands safe. No other correctness instances found. Verified: core unit suite 426 tests, 0 failures; bin/check-all.sh 15 passed, 0 failed, 0 could not run; the PR-scoped mutation lane scored 34 evaluated mutants of 34 generated, 30 killed, 88% test strength, 0 mutations with no coverage.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…hot, silently completing records OffsetMapCodecManager.encodeOffsetsCompressed read moving partition state twice and treated the two reads as one snapshot: the incomplete-offsets set was filtered on its own internal read of offsetHighestSucceeded, then a SECOND read became the encoder's range top. OffsetSimultaneousEncoder marks every offset in [base, rangeTop] absent from the set as completed, so a completion landing between the two reads - of an offset ABOVE the high-water mark - widened the range around a stale set, and every still-incomplete offset in the widened stretch encoded as complete. On restore they are dismissed by isRecordPreviouslyCompleted and never processed: silent record loss, with no error and no lag anomaly. Live in the DEFAULT commit mode. The encode runs on the broker-poll thread while completions land from the control thread; PERIODIC_CONSUMER_SYNC blocks on the commit response and PERIODIC_TRANSACTIONAL_PRODUCER shares a thread, so the window is specific to PERIODIC_CONSUMER_ASYNCHRONOUS. The code dates to 2022 and predates the fork, so every 0.5.x line carries it. THE FIX IS ONE SAMPLE. getOffsetHighestSucceeded() is read once and both the snapshot and the range top derive from it, so they cannot disagree by construction - the same shape as the confluentinc#894 fix in #337, one call below it. PartitionState gains the bounded filter getIncompleteOffsetsBelow(long); the existing getIncompleteOffsetsBelowHighestSucceeded() delegates to it, so no existing signature changed. RemovedPartitionState overrides only the bounded overload, which guards both entry points precisely because of that delegation. THE RESIDUAL CLAIM WAS NARROWED TWICE, AND THAT IS THE MOST IMPORTANT PART. It began as "residual races at this seam are all safe-direction". Review showed that is false for a concurrent bootstrap reset: sampling the mark first pairs the old high bound with a map initStateFromOffsetData may have since replaced, encoding a stale range as complete and cancelling the mandated replay - where the old second read would have observed the reset and narrowed the range instead. So this change is better against the completion seam it targets and worse against that one. It is nevertheless UNREACHABLE, by construction rather than by a gate that happens to be shut: bootstrapPhase has exactly one write site, the first line of maybeTruncateBelowOrAbove, which flips it false with an early return otherwise; that call is reached from maybeRegisterNewPollBatchAsWork BEFORE its addNewIncompleteRecord loop; dirty is set only by onSuccess, which needs an offset registered by that loop; and onPartitionsAssigned always builds fresh instances rather than reusing one. The reset window and the dirty-encode window are therefore temporally disjoint on any instance. An earlier version of this argument made the safety contingent on #346 landing first; it is not. THE TESTS ARE PROVEN AGAINST THEIR OWN MUTANTS, because two of them could not fail. OffsetEncoderWidenedRangeRaceTest's guard proved the injected completion was ATTEMPTED, not that it moved the high-water mark - fixture drift would have left both reads equal and the tests green against the very ordering they exist to catch; the double now records the mark either side and the tests require it to advance. RemovedPartitionStateTest asserted the singleton returns an empty set, which the base filter also does, so deleting the override under test left it green; it now populates a removed state through the un-overridden base path, with a live-partition control arm proving the arrangement discriminates. Each control was demonstrated RED against its own mutant and restored. The two entry points needed different techniques, which is not over-engineering: the bounded overload is guarded once so state discriminates it, while the convenience method is guarded twice - getOffsetHighestSucceeded() is also overridden to KAFKA_OFFSET_ABSENCE - so no arrangement of state can separate delegation from coincidence, and that claim is pinned with an observable dispatch seam instead. RacingCommitCycleState is re-hooked to the bounded overload. #337 landed first, and the fixed encoder no longer calls the seam that double overrode, so all seven of its tests failed loudly at their fired-assertions - the guard working as designed rather than going silently green on a dead seam. Overriding the bounded method catches both entry points for the same reason RemovedPartitionState does. Defect-class sweep, two reads of moving state around one operation: createOffsetAndMetadata is #337's and deliberately not re-fixed here; tryToEncodeOffsets' second read feeds a metrics denominator only, dismissed; isRecordPreviouslyCompleted and getOffsetHighestSequentialSucceeded checked, every interleaving lands safe. No other correctness instances found. Verified: core unit suite 426 tests, 0 failures; bin/check-all.sh 15 passed, 0 failed, 0 could not run; the PR-scoped mutation lane scored 34 evaluated mutants of 34 generated, 30 killed, 88% test strength, 0 mutations with no coverage.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…its green is #347 committed this harness asserting the checkpoint-3 tear EXISTS, naming this fix as the one that inverts it. The previous head could not: #345's NullPointerException out of ShardManager.removeWorkFromShardFor was reached through the same revokeAndReassign operation, and `assertThat(report).contains("completeWork")` cannot tell one from the other. #345 has landed, so that blocker is gone and the lane was re-run rather than reasoned about, as docs/inflight/test-lincheck-lane-open-items.md requires. MEASURED, on this tree, at the committed iterations(1_000): - Seven valid runs, every one exhausting the whole bound with no violation, 120.3s-143.1s. A hit used to land in 9-13s, so none of these is a near miss. The designed-red arm therefore fails every run and cannot stay as it is. - CONTROL, the same sitting: a compiling mutant that re-resolves the partition state at handleFutureResult's two action sites - the checkpoint-3 tear put back, nothing else changed - hit ONCE IN SIX runs. The hit is exactly the calibrated signature: AssertionError at PartitionState.onSuccess, through PartitionStateManager.onSuccess and WorkManager.onSuccessResult out of handleFutureResult, completeWork racing revokeAndReassign, and no NullPointerException anywhere. So the harness still reaches its seam - it has not gone quiet. The honest reading of those two together, which the javadoc now carries: seven clean runs is WEAK evidence of absence when the positive control itself misses five runs in six. What pins this fix is WorkManagerStaleCheckDoubleLookupTest, which forces the interleaving deterministically; the Lincheck arm is a search running beside it, and a pass here must never be read as a proof. So: renamed to stressMustNotRediscoverTheCheckpointThreeTear and flipped to Lincheck's own linearizability check, the shape RetryQueueLincheckTest already uses. The bound is left exactly where it was measured - re-pricing needs the starve-and-count procedure the note prescribes, and neither raising nor lowering it on this handful of runs would be better founded than the estimate it replaces. THE COST, because it is now the lane's largest number. An inverted arm cannot stop at a first violation, so it always pays its whole bound: this arm is ~2 minutes every run and the whole lane went from 26-29s to 2m32s-2m37s (measured twice, green both times). bin/lincheck-test.sh's header and docs/testing.md said "well under a minute" and now say what it costs. THE OTHER HARNESSES, re-checked in the same sitting as that rule requires: - ShardManagerLincheckTest is unchanged by #335 landing - same counterexample the note records, revokeSweep(0) then addWork(0) against addWork(0). Nothing in the note's item 1 moves. - PartitionStateLincheckTest has now been run against a tree carrying #337's fix for the first time, and does NOT invert. It still reports a violation naming commit(), but the report is not stable between runs - mostly a value divergence between commit() and succeed(), once an ArrayIndexOutOfBoundsException out of two concurrent commit() calls printed with no frames. #344 remains its trigger; recorded as a sighting, not a finding. - RetryQueueLincheckTest and both toolchain controls green and unchanged - the model checker is still blocked on the Lombok callSuper defect, so the note's item 8 arms are still not free. docs/testing.md's "every harness asserts a bug EXISTS" bullet was already wrong before this change (RetryQueueLincheckTest never did, ShardManagerLincheckTest stopped when #345 landed); it now names the three shapes in the lane and why the third exists.
7 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…d PartitionStateLincheckTest still does not invert Merging master brought #344 in beside #337, so both fixes the contract named for PartitionStateLincheckTest are now in one tree. The rule that note carries is re-run the lane and re-read EVERY harness's assertion, not only your own, so this is that re-run. MEASURED, whole lane green (2m41s), and the harness does NOT invert. It reports a violation on every run and `assertThat(report).contains("commit()")` still passes - on reports about neither tear it names, because the interleaving table names commit() whatever threw. Two shapes recur across runs: - ArrayIndexOutOfBoundsException out of ArrayList.add, i.e. the plain-ArrayList defect #57 fixes. The note recorded this as a frameless sighting one revision ago; it now has a frame naming it, so the lane has re-found it from a second harness pointed elsewhere. - PartitionState.onSuccess's assert, reached from two `succeed` operations in parallel - which production cannot perform, because only the control thread completes work. The second shape is the same artefact-or-defect question the note already parks over ShardManagerLincheckTest's addWork, arriving now in a second harness. If it is the artefact the fix is a non-parallel group over `succeed`, not a change to PartitionState. That decision is open, this arm's next assertion depends on it, and it should be taken for BOTH harnesses at once - so it is recorded rather than guessed at here. What IS fixed here is the javadoc, which until now told readers this arm would invert when #344 landed, and the note, which said #344 was still its outstanding trigger. Both are now false and both said so confidently. The javadoc also states plainly that this arm does not pin #337 or #344 - PartitionStateCommitEncodeShift894Test and OffsetEncoderWidenedRangeRaceTest do - so nobody reads its green as covering them. The contract's per-PR trigger list is now fully spent, and it went 0 for 4 on its predictions: three harnesses were left finding something other than the bug named, and the fourth inverted only once BOTH violations reachable through its operations were gone. The instruction was right; the one-bug-per-harness assumption behind it was not. Also settles the self-reference bug-torn-read-family.md now demands of this PR. That note arrived on master with the hunt's record and maps merges to sections; its #346 line is rewritten in the post-merge terms the #345 line already uses, and says why the candidate sections stay - the note remains open for the out-of-family stragglers, the #57 trigger and the next hunt iteration. Verified on the merged tree: core unit 451 tests / 0 failures / 8 pre-existing skips, full-reactor test-compile green, bin/check-all.sh 15/15, bin/infer-test.sh "all 22 finding(s) are known, none new", Lincheck lane green.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…, closing a stall and a wrong-commit route (#346) WorkManager.handleFutureResult's staleness checkpoint 3 validated one partitionStates.get(tp) lookup and acted on another. Nothing serialises the two against the broker-poll thread, so a revoke(+reassign) completing in the gap meant the check passed against the OLD state object while the actions ran against its replacement. Both harms were reproduced deterministically before any fix: - FAILURE PATH, a confluentinc#857-family permanent stall. The stale container was re-queued into the retry queue AFTER the revoke sweep had emptied its shard. removeStaleContainers reaches retry-queue entries only through shard contents, so nothing could remove the orphan again; once its retry delay elapsed, workIsWaitingToBeProcessed() read true forever with nothing assigned. - SUCCESS PATH, the only production route into a wrong commit. The completion landed on the freshly assigned state, tripping PartitionState.onSuccess's assert under -ea; without -ea it silently dirtied a bootstrap-phase state with a dead epoch's completion, which was the gate-opener making the bootstrap-reset commit tear reachable. THE FIX ACCEPTS THAT NO EPOCH CHECK HERE CAN BE ATOMIC WITH THE ACTIONS, and makes the actions safe by construction instead. The state is resolved ONCE and the staleness answer shares that reference with the success action, so a completion can only ever mutate the state object whose epoch matched the container; a concurrently revoked object is already unlinked from the live map, so the write is invisible to the commit path and the record is redelivered to the partition's next owner. At-least-once holds in every interleaving. The failure path additionally re-validates against the LIVE map immediately before the retry re-queue, because that action targets ShardManager structures the revoke sweep cleans and no captured PartitionState reference can protect; skipping the re-queue is the safe direction. PartitionStateManager.onSuccess/onFailure now take the caller-resolved state - WorkManager was their only production caller, and the single-arg overloads that did their own second lookup are reached from tests alone. HONEST RESIDUAL, FAILURE PATH ONLY. The re-queue re-validation is still check-then-act, shrunk from a whole-checkpoint gap a deterministic revoke reproducibly fit inside to a one-context-switch sliver. The class-level closure is the retryQueue sweep tracked in docs/inflight/bug-retry-queue-orphaned-by-inline-stale-removal.md. The success path has no residual. CONTROLS, run rather than assumed. WorkManagerStaleCheckDoubleLookupTest, five tests: both race arms red on the unfixed tree and green fixed, both serialized control arms green on both, so the failures belong to the interleaving and not the mutation. A mutant re-resolving the state at the action sites flips exactly the success arm and the bootstrap-gate probe red; deleting the re-queue re-validation flips exactly the failure arm red; deleting a test's arm call fails its raceHasFired guard, so the seam cannot go silently dead. THE LINCHECK INVERSION IS DISCHARGED, AND THE MEASUREMENT SAYS DO NOT TRUST IT ALONE. #347 committed WorkManagerLincheckTest asserting the tear EXISTS, to be inverted when this landed. On the fixed tree it missed all seven runs, each exhausting 1000 iterations - but a control carrying the defect re-introduced hit only once in six, producing exactly the checkpoint-3 signature and no NullPointerException now that #345 has landed. So the harness still reaches its seam and is simply a poor detector at this bound: seven clean runs is weak evidence of absence. The arm is inverted, the bound left where it was measured, and the javadoc and note both say the deterministic test is what pins this fix. The inverted arm cannot exit early, so the lane went from about 27s to about 2m35s - corrected in bin/lincheck-test.sh's header and docs/testing.md, which both claimed well under a minute. THE CONTRACT'S ONE-BUG-PER-HARNESS ASSUMPTION IS DEAD, and PartitionStateLincheckTest is now a VACUOUS GREEN. With #337 and #344 both landed it still passes contains("commit()") - on reports about neither tear it names, because the interleaving table names commit() whatever threw. Two shapes recur: an ArrayIndexOutOfBoundsException out of ArrayList.add, the plain-ArrayList defect #57 fixes, which the lane has now re-found from a second harness pointed elsewhere and with a frame this time; and PartitionState.onSuccess's assert reached from two parallel succeed operations, which production cannot perform because only the control thread completes work. Whether that second shape is artefact or defect is the same question already parked over ShardManagerLincheckTest's addWork, and it should be settled for both harnesses at once and AFTER #57 lands, since that PR removes one of the two shapes. Recorded rather than guessed at; the assertion is deliberately not re-pointed here. ALSO IN THIS CHANGE. The infer ratchet's gone-arm fired on WorkManager.onFailureResult; a three-tree control with pinned Infer v1.3.0 showed Pulse re-attributing an unchanged deref, with the identical shape in the neighbouring overload reported at neither head, so the line is deleted rather than a guard added. The same commit pins -encoding UTF-8 on the lane's hand-rolled javac, which had been inheriting the developer locale and made bin/infer-test.sh exit 2 under LANG=C. The duplicate-code thread is closed by extracting the shared fixture to ModelUtils.registerOneRecordAndTakeIt. WorkManager's pm and sm fields carry TODO(refactor) markers plus a docs/refactoring.md entry for the rename raised in review, not folded in here because both are @Getter(PUBLIC) and it would reach main, unit and integration sources far outside this seam. A CHAOS RED ON THE WAY THROUGH, DIAGNOSED AND RECORDED RATHER THAN RE-RUN. ChaosRevokeUnderWorkDrainIT failed once with the AB-BA revoke-path monitor block - the poll thread BLOCKED on the commitCommand AtomicBoolean held by its own pc-control thread, through onPartitionsRevoked - which is master-state and not this change's: the diff touches no locking and no AbstractParallelEoSStreamProcessor line, and the three earliest captures predate #335's merge with the identical monitor, holder and method pair. Filed as the fifth capture in docs/inflight/bug-857-family.md. The lag-stagnation observations in the same run were non-gating, so the CLASS2 demotion held. Nothing was quarantined, loosened, or re-run to hide it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…e extracted work comes home Fifteen commits, and three of them are this branch's own work returning in the form it was reviewed in: #375 (the probes and their brokerless harness), #376 (the back-pressure pause derived from Kafka), and #267, one of this PR's declared dependencies. #120's PCMetrics work landed with them. **Six of the nine conflicts had one right answer, already written down.** The rule recorded on this branch - take the EXTRACTED version, never this branch's copy, because the extracted one is what got reviewed on its own merits - covers `ThrowableUtils`, `PCMetrics`, `MetricsTeardownCannotBreakCloseTest`, `OutForProcessingCounterDriftProbeTest`, `BrokerPollSystemCooperativeRebalancePauseTest` and the mirror-of-state write-up. All six are now byte-identical to master, so the duplicate copies this branch was carrying are gone rather than merely reconciled. `BrokerPollSystem` is the same call one level down. Master's version is #376 as it landed, refined past what was extracted - a `shouldThrottle` local, a javadoc that explains `TopicPartitionState` rather than asserting the conclusion, and no reference to `ThreadConfinedConsumer`, which is not on master. Taking master's keeps the two from drifting; the cluster-2 note about consumer confinement can be re-added by the change that introduces it, which is where it will be true. **One correction had to be re-applied, and it is worth naming as a pattern.** `pr-blockers-and-collisions.md` was taken from master because master's version is genuinely better - it updates the file-ownership map for #337 and points the reader at `gh pr list` rather than a tally that goes stale at every merge. But master still carries the claim that #29 and #31 target `master-confluent` and must be retargeted, which this branch had already corrected against `gh`: both target `master` and #31 has merged. Taking master's file wholesale reinstated a claim I had already disproved. That is the cost of "take master's version" applied to a file rather than to a change, and the tell was the self-reference gate going red on a line I had fixed once. Re-applied on top of master's version rather than reverting to ours, so both improvements survive. `./mvnw test-compile` green across every module; `bin/check-all.sh` 15/15; `bin/todo-index.sh --check` clean.
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…d the invariant that keeps it shut is pinned (#484) WHAT WAS OPEN. confluentinc#546 holds three defects behind one warning string. One is fixed by inheritance (confluentinc#563, already in this tree and guarded in OffsetRunLength). One is live. The third - the "Bootstrap polled offset has been reset to an earlier offset" branch of PartitionState#maybeTruncateBelowOrAbove - was never diagnosed upstream, and this fork's note carried an UNTESTED hypothesis for it, proposing a control-arm test nobody had run. This change runs it, and the hypothesis is refuted. THE HYPOTHESIS, AND WHY IT NAMES THE WRONG BRANCH. The note proposed a race: PC issues its own consumer.committed() in loadPartitionStateForAssignment, separate from the consumer's own position resolution for a newly assigned partition, so a commit landing between the two reads would leave PC one commit ahead of where the fetcher starts. It is not a race at all. In kafka-clients 3.9.2 - the version this tree builds on - ClassicKafkaConsumer#updateAssignmentMetadataIfNeeded runs coordinator.poll, which invokes the rebalance listener and therefore PC's committed(), and only afterwards updateFetchPositions, whose ConsumerCoordinator#initWithCommittedOffsetsIfNeeded is what gives an initializing partition its position. PC's read strictly PRECEDES the fetcher's, so a commit landing between the two is the one the FETCHER sees: the polled offset comes out at or above PC's expectation. The proposed mechanism can only produce the other warning. WHAT IS LEFT, AND THE INVARIANT THAT SHUTS IT. One route into the branch belongs to PC rather than to the broker: a commit whose encoded payload decodes to an expectation ABOVE the offset that commit was filed under - the shape #337 closed. PartitionStateBootstrapTruncation162Test round-trips a commit PC itself wrote back through MockConsumer, WorkManager#onPartitionsAssigned and the decoders, over four shapes PC can genuinely commit (fully drained, contiguous tail incomplete, a completed island above the commit frontier, only the base offset incomplete), and asserts the bootstrap expectation equals the committed offset exactly. Hold that invariant and a partition PC hands to itself cannot take either truncation branch. Every remaining way in is the broker handing back a position below the committed offset - an offset reset, a manual rewind, another member committing lower, a stale OffsetFetch across a coordinator failover - and there replaying is the right answer, not a defect. THE TWO BRANCHES AS A CONTROLLED PAIR. One fixture, one differing term - the offset the first poll batch starts at - so the outcome that flips is the branch's own. Below expected: every loaded incomplete is discarded, including one above the batch that the batch does not re-register, and offsetHighestSucceeded resets, so records the previous owner completed and committed are processed again. Above expected, the control: only incompletes beneath the polled offset are pruned, and the one above the batch survives. The note's "duplicate processing, not loss" claim is asserted in BOTH halves - nothing tracked can be stranded below the polled offset, because the expectation the branch fires against IS the lowest tracked incomplete. NEGATIVE CONTROLS, ONE PER ASSERTION, so no arm can pass by accident. Removing the reset fails only the below-expected arm; removing the prune fails only the above-expected arm; dropping the payload's lowest incomplete on decode flips the two round-trip shapes that carry a payload and leaves untouched the two that do not. AN INSTRUMENT THAT LIED FIRST, recorded because it read as a product defect. The round trip initially reported an expectation of 0 for three of the four shapes. MockConsumer#committed hands back a bare OffsetAndMetadata(0) - indistinguishable from a real commit at offset 0 - for any partition its own SubscriptionState does not hold, and MockConsumer#assign clears the committed map, so the assignment has to precede the commit. The arm now proves itself, and the comment at that line says why. A test harness that fabricates a plausible wrong answer is worth writing down: it cost a diagnosis before it was caught. WHAT REMAINS OPEN ON #162, and why this does not close it. The false warning: maybeTruncateBelowOrAbove never asks whether commit data existed, so a partition with no committed offset reports "expected 0 from loaded commit data" and truncates nothing. Misdirection, still untested, and unchanged here. The inflight note is shrunk to that one case and the draft mirror reply is corrected to name it, so the upstream thread is not told that both remaining cases are covered. src/docs/development/upstream-map.yaml records the lifecycle transition in the commit that caused it. CROSS-CHECKED AGAINST THE MASTER THIS LANDS ON. #470 rewrote PartitionState's commit-acknowledgement path while this branch was open, and its offerLastMadeForCommit javadoc names this same downward reset from the other side - only the latest commit offer is kept rather than a set, because the bootstrap reset is "exactly the event no pruning rule would see". The two records agree: the reset is real, reachable only in bootstrapPhase, and destructive of anything keyed to the pre-reset expectation. Nothing in the refutation is disturbed, because the refutation is about how the branch is ENTERED, not what it does once entered. Re-run after merging master rather than assumed. No production code changes: one new test class, the inflight note, and the upstream manifest. Upstream-Issue: confluentinc#546 Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #121 - confluentinc#894, "Offset reset when frequent rebalancing".
Summary
Carries upstream confluentinc#893 (author
sangreal, approved byrkolesnev, never merged upstream), and adds the behavioural reproduction that upstream asked for in October 2025 and never got.Split out of #57 on 2026-08-24. It was riding inside a metrics PR, where the evidence below would not have been read. The two share no main-code file, so this is independent rather than stacked - no dependency line, and it can merge in either order.
Since it was split out, the branch has taken master five times and been through a
simplify pass and two independent review rounds (Codex, then the automated Claude review). The
work now falls into four groups:
PartitionStateCommitEncodeShift894Test,PartitionStateCommitShiftCompounding894Test, the sharedRacingCommitCycleStatedouble, and thedocs/solutions/write-upupstream-map.yamlcarrier corrected, and the drafted issue responsesThe defect
A commit carries an encoded note listing which offsets above it are still outstanding, and that note is written relative to the committed offset - the committed offset is its decode base. The unfixed code derived those two numbers from two separate reads:
A completion landing between them makes the two disagree, so the commit describes a partition state that never existed. Restored after a rebalance, every offset in that note decodes too high.
The fix samples once and returns both values together, so the payload and the committed offset are the same number by construction, and the residual race in that method is conservative - a lower offset and at-least-once replay.
Scoped deliberately, because an earlier version of this description overstated it. One layer down,
OffsetMapCodecManager.encodeOffsetsCompressedtakes two more unsynchronised reads - the incomplete set, thengetOffsetHighestSucceeded()as the encoder's range top - so the payload's contents may still be buildable from a torn read. That code is untouched by this PR, identical to master, and dates to 2022; it is being reproduced on its own branch before anything is changed. This PR closes the offset-to-commit read, not every read on the commit path.Two failure modes, not one
The reported one is loud. The fabricated state drives the committed offset past the log end, the next poll asks for a position that was never produced, and
auto.offset.resetfires. That is confluentinc#894 as reported.The other is quiet, and was not reported by anyone. At traffic rates at or above the encoded payload width, the committed offset tracks the log end exactly - no overshoot, no reset, nothing red - while real records are dismissed by
isRecordPreviouslyCompletedagainst a fabricatedoffsetHighestSucceeded. They are never processed and never retried.That second mode is record loss with no error surface, and it is the stronger argument for taking this fix. It shares the reported fault's root cause and the same change closes it - zero records dropped across every control-arm cycle.
Both need the same four preconditions, so neither is routine: incompletes present at commit time, a completion of the lowest incomplete inside a microsecond window, a rebalance reading that commit back, and (for the quiet mode) continued traffic. Rare per commit cycle; not rare over time in the low-traffic, rebalance-heavy shape the original report describes, which is why the reporter saw it every few days.
Evidence
Written against code that still contains the bug, on
test/894-reproduce-offset-encode-shift(deliberately never merged - a reproduction has to run against the defect), then verified green here.Full detail:
docs/solutions/logic-errors/commit-offset-read-twice-shifts-every-encoded-incomplete-offset.md.Two things there are worth surfacing for anyone returning to the original report. The reproducible example requested on the upstream PR on 2025-10-26 now exists. And the flow diagram attached to that PR on 2025-10-31 - unread for nine months - contains both the production logs and an explicit "how to make it happen" recipe.
Scope
The two-read shape is present in every released 0.5.x line - verified by reading
createOffsetAndMetadata()at all 16 released0.5.*tags; before 0.5.2.4 the second read is spelledgetNextExpectedPolledOffset(). The method did not exist before 0.5.0.0, so that is the complete set rather than a sample.Checklist
AGENTS.mdforbids a PR adding changelog entries; the section is regenerated from the commit log at release, and the commit bodies carry what the generator reads.docs/solutions/logic-errors/write-up; two javadoc corrections inPartitionStatePartitionStateCommitEncodeShift894Test,PartitionStateCommitShiftCompounding894Test, plus the call-count assertion inPartitionStateCommittedOffsetTestupstream-map.yamlupdated -cherry-pick-893-offset-resetcarriesprs: [337]andfork_issue: 121; the contradicting claim incherry-pick-892-pcmetrics-leakthat fix(core) astubbs#120: unbounded PCMetrics heap growth, with the confluentinc#905 hot-shard metric #57 still bundles make committed offset accurate when partition assigned to avoid offset reset confluentinc/parallel-consumer#893 has been correctedOpen, and stated rather than buried
That the compounding is bounded by the payload width is a regularity measured across two widths and nine sweep points with a stated mechanism - not a proof over the parameter. It does not affect whether to take the fix, since the second read of the offset to commit is gone, but it is the sentence to revisit rather than cite - and see the scoping note above for what "closed" does and does not cover.
Review rounds, and what they changed
Two independent reviews after the branch settled, both acted on in full.
Codex raised three P2s, all real:
K >= Larm was green by construction, including the regime this description calls the stronger argument for the fix. Both regimes are now asserted as one pair.upstream-map.yamlcontradicted itself over whether fix(core) astubbs#120: unbounded PCMetrics heap growth, with the confluentinc#905 hot-shard metric #57 or fix(core) astubbs#121: commit the offset the encoded payload was written against (confluentinc#894) #337 carries make committed offset accurate when partition assigned to avoid offset reset confluentinc/parallel-consumer#893.The automated review independently re-ran the controls and found no blocking issues, plus one further instance of finding 2's class: two of the three tests in the sibling file were unguarded too. Fixed.
Every assertion added here was mutation-checked rather than assumed:
[0, 13]and[0, 152]; under the old single assertion those runs were green while losing 152 recordsTwo SpotBugs findings deliberately declined
bin/check-pr-analysis-surfaces.shreports two on lines this PR wrote. Both are fb-contrib style advisories rather than defects, and both are left alone:sorted(List)"could be declared as java.util.Collection" - the identical advisory stands unfixed onAbstractParallelEoSStreamProcessorTestBase.assertCommitsContains, soListis the established convention here rather than an oversight.orElse(Long.MIN_VALUE)autoboxing - the suggestedorElseGetallocates a lambda per cycle instead of aLong, so it is not an improvement.Both were put to the automated reviewer explicitly, which agreed.