Repository navigation
fix(core)!: WorkContainer equality is identity, so the stale sweep removes only the container it inspected - #468
Merged
Conversation
…a fresh replacement RED against master, deliberately. The fix is not in this commit. `ProcessingShard.removeStaleWorkContainersFromShard` walks `workMap` and removes by KEY (`removeWorkAtOffset(entry.getKey())`), so whatever occupies that offset when the removal lands is what leaves the shard - not necessarily the container the sweep inspected and judged stale. The sweep runs on the broker-poll thread inside a rebalance callback; `addWorkContainer`'s stale-replacement branch runs on the controller. Nothing orders them. The primary harm is the LOST RECORD, not the counter: the fresh container is gone from the shard while `PartitionState` still carries its offset as incomplete, so nothing selects it again until the partition is re-polled. The accounting half of this defect is already closed (#336, #373 - every exit path retires and releases the claim of whatever the map actually gave up), and the test asserts the counters too so a later change cannot trade one half back for the other. The reproduction is deterministic rather than timed. It reuses the staleness-check seam `ShardPopulationRaceTest` already had - a spied `PartitionStateManager.getPartitionState` that runs the other thread's action on the way past - so the controller's replacement lands at an exact instruction inside the sweep. That mechanism is now `ShardSeamTestBase`, extracted rather than copied, and `ShardPopulationRaceTest` is rebased onto it with no assertion, arm or scenario changed. Every assertion about WHICH container is resident uses reference identity, never Truth's `hasValue` or `containsExactly`. `WorkContainer.equals` is topic/partition/offset only, so the fresh container compares EQUAL to the stale one it replaced - which is the reason this defect exists in the first place, and it would silently make the equality form of these assertions pass on the defective behaviour. Red proof, `./mvnw -pl parallel-consumer-core -am test -Dtest=ShardStaleSweepReplacementEvictionTest`: [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0 [ERROR] ShardStaleSweepReplacementEvictionTest .theStaleSweepMustNotEvictAFreshReplacementThatLandedInsideIt:92 the fresh container is the only thing at this offset that is not stale, and the partition still carries its offset as incomplete - evicting it loses the record until the partition is re-polled value of : orElse(...) expected specific instance: WorkContainer(tp:topic-0:o:100:k:a-key) but was : null The control arm `aSweepWithNothingRacingItRemovesTheStaleContainerAndOnlyThat` is green in the same run - same magnitude, different position, so the responsible term is the replacement landing inside the sweep and not the sweep itself. Tracked by docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md, which this branch resolves. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
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)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #468 +/- ##
============================================
+ Coverage 82.17% 82.61% +0.43%
- Complexity 1536 1541 +5
============================================
Files 96 96
Lines 5353 5343 -10
Branches 533 529 -4
============================================
+ Hits 4399 4414 +15
+ Misses 758 739 -19
+ Partials 196 190 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
[superseded - a quarantined test changed outcome] 🧪🔒 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 Updated for Superseded by a newer quarantine lane report. |
🟢 Throughput — OKThis branch measured about 6% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: 6aab3ff, 745b1f6, 49f81f1, bb73082, 16b2e89, ed82e7f, c562df4, f502202, abd1d39, 60138dc Since the previous push: ratio 0.971 -> 0.938, share 1.752 -> 1.813, rate 80381 -> 79804 (-0.7%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
…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
One conflict, in `ShardPopulationRaceTest`, and both sides of it were superseded rather than one winning. #467 hoisted that test's private `joinQuietly` into the shared test `ThreadUtils`; this branch had extracted its `onNextStalenessCheck`, `shardWith` and `recordAt` helpers into `ShardSeamTestBase` so a second test could drive the same seam. The two edits are adjacent and disjoint, and git paired the deletions: HEAD's side of the hunk was the now-hoisted `joinQuietly`, master's was the three now-inherited helpers. Taking either would have restored a method whose owner is elsewhere, so the resolution deletes both - `ThreadUtils.joinQuietly` is already what the call site above it uses (from master), and the helpers are already inherited (from this branch). Neither `--ours` nor `--theirs` would have produced this. Nothing else in the three inherited commits touches this branch's area. #467's own subject - the Lincheck harness assigning its partition - is a fixture change in a different test, and its re-verification note says the NPE it fixes is not reachable on master. Verified: `bz.stub.parallelconsumer.state.*Test` green (162 tests, the one pre-existing skip in `WorkManagerTest`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
marked this pull request as ready for review
September 7, 2026 10:40
[superseded - a quarantined test changed outcome] 🧪🔒 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 No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
astubbs
marked this pull request as draft
September 7, 2026 10:46
…cts-fresh-replacement # Conflicts: # parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/ProcessingShard.java # parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/state/ShardPopulationRaceTest.java
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 2m 32s —— View job Review: the stale-sweep eviction fix
SummaryThis is a well-targeted concurrency fix, and the write-up is unusually rigorous about why the obvious alternatives ( The core mechanism holds up:
Tests:
Minor/non-blocking observations:
No correctness issues found. This looks ready modulo normal human sign-off (it's still a draft PR). |
… can name the container it means 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. Recorded in the docs/refactoring.md breaking-change section, which is what the 0.6.0.0 release notes are assembled from. WHAT CHANGED. The equals/hashCode pair on WorkContainer is deleted, so its equality is reference identity, and ProcessingShard.Residency - the token this branch introduced two commits ago to buy one identity-conditional removal - is deleted with it. The shard's map stores containers directly again and workMap.remove(offset, container) is a true compare-and-remove: the JDK's compare-and-remove decides "still mapped to the value I inspected" with equals, so what it means is a property of the VALUE TYPE and not of the map. Fixing the value type fixes every value-conditional operation on a container at once, instead of once per collection that remembers to wrap. THE DECISION, and it is the maintainer's rather than the code's. The token was the right shape for a patch release and the wrong shape for this one: 0.6.0.0 is a breaking release, already carries a === Breaking section, and WorkContainer is public in modifier only - it is not part of the surface a user is expected to hold. A token is a workaround the next site has to remember to repeat. THE OBJECTIONS THAT HAD QUEUED THIS AS FUTURE WORK, each checked and each fallen: - "compareTo would become inconsistent with equals." Comparable only RECOMMENDS that consistency; it is REQUIRED by SortedSet and SortedMap, 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 a container is only ever a map VALUE there. compareTo and its comparator are kept unchanged, with the inconsistency stated in the javadoc rather than left to be rediscovered. - "Collections keyed on containers elsewhere would silently change meaning." There is one, and it already agrees: ExternalEngine.holdingDispatchPermit is an IdentityHashMap-backed set, and has been since #342, with a javadoc saying it is by identity and why. Identity equality makes the container agree with that code rather than changing it. - "The public break." Real, and it is RecordContext, stated above and in the release-notes section. THAT SECOND OBJECTION ALSO CORRECTS THIS BRANCH'S OWN SAME-DEFECT-CLASS SWEEP. The previous commit reported holdingDispatchPermit as a real unfixed hit - a Set<WorkContainer> leaking a dispatch permit per offset collision - and opened docs/inflight/bug-dispatch-permit-set-cannot-tell-two-containers- at-one-offset-apart.md for it. That defect does not exist. The sweep matched the DECLARED TYPE Set<WorkContainer<K, V>> and stopped there; the initialiser is Collections.newSetFromMap(Collections.synchronizedMap(new IdentityHashMap<>())). A collection's semantics live in its initialiser, not its declaration, so a search over declarations reports safe and unsafe sites identically. The note is deleted - nothing in it was both true and unowned - and the lesson is migrated into the solutions write-up's Prevention section, where the sweep that produced it is recorded. RED PROOF. Restoring the old coordinate-based equals/hashCode on WorkContainer and changing nothing else, ShardStaleSweepReplacementEvictionTest goes 2 of 3 arms red: [ERROR] Tests run: 3, Failures: 2, Errors: 0, Skipped: 0 [ERROR] ...ShardStaleSweepReplacementEvictionTest .theStaleSweepMustNotEvictAFreshReplacementThatLandedInsideIt the fresh container is the only thing at this offset that is not stale, and the partition still carries its offset as incomplete - evicting it loses the record until the partition is re-polled value of : orElse(...) expected specific instance: WorkContainer(tp:topic-0:o:100:k:a-key) but was : null [ERROR] ...ShardStaleSweepReplacementEvictionTest.twoContainersAtOneOffsetMustNotBeInterchangeable WorkContainer equality is identity - a coordinate-based equals would make the stale container and its fresh replacement interchangeable in every value-conditional collection operation, which is the defect this class exists for expected not to be: WorkContainer(tp:topic-0:o:100:k:a-key) The control arm aSweepWithNothingRacingItRemovesTheStaleContainerAndOnlyThat stays GREEN, and must: a sweep with nothing racing it never has to say which of two containers it meant, so the equality contract is not a term in it. Same magnitude, different position. TESTS. All three arms are kept. The premise arm is renamed twoContainersAtOneOffsetMustNotBeInterchangeable and now asserts the equality contract DIRECTLY - two containers at one offset are not equal, are not the same instance, each hash code IS its identity hash, and the two therefore differ - before asserting the map behaviour that follows from it. It was previously a characterisation of the JDK that would have gone red on this change as a side effect; it is now the tripwire that goes red if anyone reintroduces an offset-only equals/hashCode. Its computeIfPresent form is kept and inverted: the re-read gate in doRemove(key, v) that used to pass and destroy the replacement now declines. One other test needed changing, and it is the only value-equality assertion on a container in the tree: ShardManagerTest.retryQueueOrdering asserted assertThat(w0).isNotEqualTo(w1) for containers at different offsets. Under identity equality that is true of any two objects and asserts nothing, so it is restated against RetryQueue.WorkContainerKey - which is what the queue actually keys on, and what the surrounding assertions are really about. Not weakened; the vacuous form is what was removed. A tree-wide sweep for the rest of the class found nothing else: no ArgumentCaptor<WorkContainer> and no Mockito verify/when passing a container as a matcher value (both match by equals); no containsExactly / hasValue / containsEntry over containers except where the SAME INSTANCE is asserted, which is equal under both semantics; no WorkContainer as a Map key or Set element in any test helper; and all four Lincheck harnesses return void/boolean/int from every @operation and run in stress rather than model-checking mode, so neither result comparison nor a now-nondeterministic hashCode can reach them. ONE MAIN-CODE SITE CHANGES SEMANTICS WITHOUT CHANGING BEHAVIOUR, and it is documented rather than altered: PollContext.getByTopicPartitionMap collects to a Set of freshly constructed RecordContexts, so its de-duplication becomes a no-op. No observable result moves - a poll set holds at most one record per offset because each is taken from a shard that holds at most one container per offset, so no two elements were ever equal under the old equality either. Said in an @implNote at the site. ProcessingShard.isResident and includeInSelection keep their explicit ==. The comparison is now a restatement rather than a workaround, and it stays spelled out because that is the one question in the class that must never quietly start meaning "a container at the same coordinates". DOCS. The solutions write-up docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md now records identity equality as the fix, with the Residency token as the interim shape replaced before merge and the three objections and how each fell; its rejected-alternatives section is scoped to the equality as it was, since that is what those alternatives were judged against. docs/refactoring.md's queued breaking change is marked DONE against #468 and carries the RecordContext break for the release notes, following that section's own convention rather than being deleted. VERIFICATION. Targeted state suite green (89 tests). Full unit suite green across every module (bin/ci-unit-test.sh). bin/check-all.sh: 16 gates passed, none failed. bin/check-issue-refs.sh and bin/check-file-refs.sh clean, the latter across the whole tree after the note deletion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
changed the title
… its seed `ChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedger` went red on the `Chaos Pain Suite 4/4` shard of this PR's CI with the REBALANCE_BLOCKED arm - `maxRebalanceDwell=15632ms` against a 15000ms bound, `maxInstanceStall=0ms`. Seed `1053013618367208111`, appended to docs/inflight/test-857-churn-storm-async-stalls.md, which is the ledger for this line. Recorded before the PR merges because the seed dies with the CI log, and this file already carries one entry that says so - the 2026-09-03 sighting whose seed survived only in an archive because #426 had merged before anyone looked. Not diagnosed, and the branch is argued against on a mechanism rather than dismissed as unrelated: nothing on the rebalance path reads `WorkContainer.equals`, the shard's conditional removal has the same semantics it had one commit earlier through the `Residency` token (which passed this same suite), and `ExternalEngine.holdingDispatchPermit` was already identity-keyed. `maxInstanceStall=0ms` also separates it from the worker-saturation line the file diagnosed on 2026-09-07: no member was stalled holding work. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
[superseded - a quarantined test changed outcome] 🧪🔒 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 Since the previous push: Updated for Superseded by a newer quarantine lane report. |
astubbs
marked this pull request as ready for review
September 8, 2026 00:02
…ollections
THE CONTRACT IT PINS. WorkContainer's class javadoc now states that equality is identity while
compareTo orders by topic, partition and offset, so the two are deliberately inconsistent - and that
a raw container may therefore not be an element or a key of a sorted collection. Comparable only
RECOMMENDS the consistency; SortedSet and SortedMap REQUIRE it, because a sorted collection decides
"is this already here" through the comparator, so it would find one flight of a record under another
flight's coordinates. That is the lost-record defect this branch fixes, reached from the other side.
Nothing violates it today, which is exactly why prose was not enough: RetryQueue sorts a
NavigableMap keyed on its own WorkContainerSortKey and ProcessingShard's workMap is keyed on the
offset, and both are one refactor away from being written the other way round, at which point
nothing goes red.
WHAT THE RULE READS. Every field, method return type, method parameter and constructor parameter in
main code whose declared type erases to something assignable to SortedSet, NavigableSet, SortedMap,
NavigableMap, PriorityQueue, ConcurrentSkipListSet or PriorityBlockingQueue - which covers TreeSet,
TreeMap and ConcurrentSkipListMap without naming them - and whose FIRST declared type argument
erases to WorkContainer, via JavaParameterizedType.getActualTypeArguments().
Argument 0 is the element-or-key position for every one of those types, which is why one index
serves the set-shaped and map-shaped ones alike, and why a container in a sorted map's VALUE
position passes. Checking every argument instead would report ProcessingShard.workMap, be red on
arrival against correct code, and be deleted rather than obeyed.
RED PROOF, on main code. Adding
`private final java.util.TreeSet<WorkContainer<?, ?>> tripwire = new java.util.TreeSet<>();` to
ProcessingShard and changing nothing else:
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
Architecture Violation [Priority: MEDIUM] - Rule 'no field, return type or parameter may be a
sorted collection whose elements or keys are raw WorkContainers, ...' was violated (1 times):
field bz.stub.parallelconsumer.state.ProcessingShard.tripwire is a java.util.TreeSet ordered by
a raw WorkContainer - its compareTo orders by topic, partition and offset while its equality is
identity, so a sorted collection would treat two flights of one record as the same element.
Exactly one violation, and the field one line above it - workMap, a NavigableMap holding containers
as values - was not reported. So that run is the positive and the negative control at once, on the
real tree. The field was then removed.
THE STANDING PROOF, because a measurement in a commit message protects nothing. A rule designed to
be green forever cannot distinguish "the code complies" from "the walk sees nothing" on its own
runs, which is the recurring ArchUnit failure recorded in
docs/inflight/static-archunit-main-code-rules.md. So the rule ships with the same shape
RebalanceCallbackRuleControlTest already uses: archfixture/SortedCollectionOfWorkContainers declares
the violation in all four positions plus the legitimate map-value case, and
WorkContainerSortedCollectionRuleControlTest hands it to the real rule object - not a copy - and
asserts it reports the four and not the fifth. The fixture is hand-imported because the rule's own
@AnalyzeClasses carries DoNotIncludeTests, which is what keeps a deliberate violation from turning
the production rule permanently red.
THE GAPS, stated in the test javadoc rather than left to be discovered:
- A sorted collection built inside a method body and never stored in a typed field or signature
declares nothing, so it is invisible. The rule covers the shapes that outlive a stack frame.
- The rule cannot see the comparator. A Comparator-taking constructor makes such a collection
legitimate, but only if the comparator is not compareTo - the requirement is that the ordering
agrees with equality, and equality is identity. The declaration is reported either way, so the
exemption costs a conversation instead of passing silently.
- It runs where core's test classpath reaches. The other modules' main code is not on it, and
covering them needs a wrapper per module in the TestConventionRules shape. Measured before
deciding against it: no main source outside core declares any of the seven types at all, and the
three non-core files naming WorkContainer pass it to a user function.
Every target is a Class literal rather than a string, so the rule cannot silently match nothing
after a rename - the failure mode that left both shard-map rules green when pointed at names that
do not exist.
DOCS, two edits.
docs/inflight/test-857-churn-storm-async-stalls.md: the 2026-09-07 sighting names this PR, and
check-branch-self-reference asks for an attestation that the sentence reads correctly after the
merge. It does - it is past tense about a permanent PR - so the paragraph takes the block marker.
docs/refactoring.md, "Breaking changes queued for next major version": the section's header did not
say what happens to an entry once its work lands, and four entries sat in it marked DONE. The rule
is now stated - an entry is deleted in the PR that lands it, because the commit message carries the
release-note content and the section only lists what is still QUEUED - and applied: the four DONE
entries are removed, three of them naming merged PRs (#267, #201 and #116,
each confirmed MERGED) and the fourth this branch's own.
That fourth entry was the recorded home of the RecordContext equality break, so the two citations
pointing at it are repaired rather than left dangling: WorkContainer's class javadoc and the
solutions write-up both now name the commit as the record, which is what the release notes are
generated from. The break itself is stated plainly in e491797's own body and in the PR
description.
VERIFICATION. Targeted run green (9 tests: the new rule, its two controls,
ShardStaleSweepReplacementEvictionTest and ArchitectureTest). Full unit suite green across every
module (bin/ci-unit-test.sh). bin/check-all.sh: 16 gates passed, none failed. bin/check-issue-refs.sh
and bin/check-file-refs.sh clean, the latter across the whole tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
🧪🔒 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 No quarantined test changed outcome since the previous push. Updated for |
astubbs
marked this pull request as draft
September 8, 2026 00:34
…presents `bin/check-pr-analysis-surfaces.sh` reported one finding on a line this PR wrote: fb-contrib's OCP_OVERLY_CONCRETE_PARAMETER on `SortedCollectionOfWorkContainers`'s constructor, asking for `byRetryTime` to be declared `java.util.Collection`, because the body only did `new TreeSet<>(byRetryTime)`. The detector is right about the body and wrong about what the class is for. The DECLARED type is the whole of what this fixture presents to `WorkContainerIsNeverInASortedCollectionArchTest.no_sorted_collection_is_ordered_by_a_raw_work_container` - widening it to `Collection` deletes the constructor-parameter arm of the control, since `Collection` is not one of the seven sorted types the rule looks for. So the body now reads `byRetryTime.comparator()`, which is `PriorityQueue`'s own method, and the parameter cannot be widened without a compile error. The finding does not arise rather than being suppressed - which is the better outcome than a `config/spotbugs-exclude.xml` entry plus a registry row, and leaves that mechanism for the cases where the code genuinely has to stay wrong (the `LincheckToolchainProbeTest` red control is the standing example). Targeted run green (9 tests). No behaviour changes; the rule and both its controls assert exactly what they did. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…its one differing step The duplicate-code bot flagged ten lines shared between ShardStaleSweepReplacementEvictionTest's defect arm and its control arm, three times across this PR's review. Reduced rather than dismissed: the shared setup is hoisted and each arm now reads as the fixture plus the single term it varies. WHAT MOVED. `givenAStaleResidentAtTheContestedOffset()` builds the population, the record, the shard, the first epoch's container, the revoke/assign pair that makes it stale, and the precondition that the epoch actually advanced. `assertTheCountersAgreeWithWhatIsHeld()` takes the one assertion that was character-for-character identical in both arms - and identical because it is about neither of them: the accounting half of this defect is already closed and must not be traded back for the eviction half whichever way the sweep goes. WHAT DELIBERATELY DID NOT MOVE. The interleaving itself - `onNextStalenessCheck(seam, ...)` arming the controller's replacement to land inside the sweep - stays inline in the defect arm, because it is the thing the class exists to express and a reader must meet it in place rather than follow it into a helper. So do the counts each arm expects, which genuinely differ (1 held versus 0). IT IS A HELPER CALLED BY EACH TEST, NOT A @beforeeach. The third test in the class, twoContainersAtOneOffsetMustNotBeInterchangeable, needs none of it - it is two containers and a bare ConcurrentSkipListMap - and a fixture that builds itself for a test that does not use it reads as though it matters there. THE CONTROL ARM IS NOW SPIED TOO, AND THAT STRENGTHENS IT. It previously took the real PartitionStateManager while the defect arm took a spy, so the collaborator was a second difference between them; only the defect arm arms the seam, so after the hoist the single term that differs is whether a replacement lands inside the sweep. That is what "same magnitude, different position" claims, and it was not quite true before. NO ASSERTION WEAKENED, and re-proved rather than asserted. Every identity assertion is still isSameInstanceAs, and the ablation the branch recorded was re-run against the hoisted class: restoring a coordinate-based equals/hashCode on WorkContainer and changing nothing else sends the same 2 of 3 arms red, with the same two messages - [ERROR] Tests run: 3, Failures: 2, Errors: 0, Skipped: 0 ...theStaleSweepMustNotEvictAFreshReplacementThatLandedInsideIt expected specific instance: WorkContainer(tp:topic-0:o:100:k:a-key) but was : null ...twoContainersAtOneOffsetMustNotBeInterchangeable expected not to be: WorkContainer(tp:topic-0:o:100:k:a-key) - and the control arm stays green in that run, as it must. So the hoist preserved the discrimination exactly, which a green run alone would not have shown. VERIFICATION. The class green (3 tests), the whole state package green (166 tests, 1 skipped), bin/check-all.sh 16 gates passed and none failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
Owner
Author
|
@codex review |
Master gained two commits since this branch last took it: #466's revoke-path drain fix (745b1f6), which moves the transactional revocation commit onto the control thread and touches PartitionState, PartitionStateManager and WorkManager, and #476's in-flight vetting sweep (6aab3ff), which stamped an `inflight-vetted` marker on every open note. Neither collides with the shard or the container: the drain fix merged clean against this branch's ProcessingShard and WorkContainer work. ONE CONFLICT, resolved by keeping the deletion. `docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md` is modify/delete: the sweep shrank and stamped it on master while this branch resolves and deletes it, its content having been migrated to `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md`. Master's stamp is not an argument for keeping the note - it is the reason to delete it. What the sweep recorded on 2026-09-08 was that the accounting half had closed and that "what is left open is the eviction of the fresh record itself"; that residue is precisely what this branch closes, by giving WorkContainer identity equality so `workMap.remove(offset, container)` is a true compare-and-remove. A vet stamp describes the note's remaining subject at the moment it was read. This PR removes the subject, so the stamp is falsified rather than inherited, and the note goes with it. The two paragraphs the sweep rewrote - the mechanism sentence and the #336 accounting note - are both already carried, correctly and in more detail, by the solutions write-up. Neither `--ours` nor `--theirs` was used anywhere in this merge; the one conflict was resolved with `git rm`, and the two auto-merges were left as git computed them and then read. ONE REPAIR MASTER'S SIDE NEEDED, because deleting a file does not change a line in what cites it. Master's `docs/inflight-tool.md` documents the new `vet` command with a worked example of its output, and the row it uses is this note. With the note gone, `bin/check-file-refs.sh` fails on `docs/inflight-tool.md:332` - the `bin/inflight.mjs docs show <path>` line of that example. The example is replaced with a real row for a note that survives and carries the same signal: `bug-run-length-plausibility-ceiling.md`, impact `data-loss` so the group heading is unchanged, first added 2026-09-03, one cited fork number and it is settled (#207 MERGED). The paragraph beneath it makes a point about the innocent reading of that signal, and #207 fits it better than the number it replaces: that pull request added the adjacent run-length validation rather than the ceiling the note asks for, so the row genuinely cannot tell whether the citation is a premise or a fix. Silencing the gate with a paragraph `file-refs: N/A` marker was rejected - the example would still have named a note that no longer exists, and a worked example that cannot be run is worse than a dangling path, because nothing else would ever catch it. AUDITED IN BOTH DIRECTIONS, per file. `git diff 6d2da40..HEAD` over the conflicted and auto-merged files shows only master's additions plus the inflight-tool.md repair: the whole `vet` section arriving in inflight-tool.md, the vetted marker arriving on `docs/inflight/test-857-churn-storm-async-stalls.md`, and master's removal of the `state/ProcessingShard.java` entry from `docs/refactoring.md` - which is byte-identical to 6aab3ff's own diff of that file. `git diff origin/master..HEAD` over the same files shows only this branch's changes: the note's deletion, the 2026-09-07 `ZOMBIE_MEMBER` sighting and its replay seed appended to the churn-storm ledger, the breaking-changes section rewrite in `docs/refactoring.md`, and the two-line example substitution. The file sets were compared as sets as well as read as diffs: the merge result differs from origin/master in exactly the files this branch changed against its merge base plus `docs/inflight-tool.md`, and differs from 6d2da40 in exactly the files master changed against that same base, minus the note it stamped and this branch deletes. Nothing was silently dropped in either direction. Verification: `bin/check-all.sh` 16 gates passed, 0 failed (5 skipped as PR-scoped, 3 nothing-in-scope); `bin/check-file-refs.sh` and `bin/check-issue-refs.sh` clean standalone; the state package on JDK 17 (`./mvnw -Pci -pl parallel-consumer-core -am test -Dtest='bz.stub.parallelconsumer.state.*Test'`) is 30 classes, 171 tests, 0 failures, 0 errors, including ShardStaleSweepReplacementEvictionTest and both halves of the sorted-collection ArchUnit rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d2da409d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5 of 6 tasks
…accounts for the put it made Two findings from the Codex review on #468, both against `ShardStaleSweepReplacementEvictionTest.twoContainersAtOneOffsetMustNotBeInterchangeable`, and both real. DROPPED: `assertThat(stale.hashCode()).isNotEqualTo(fresh.hashCode())`. Identity hash codes are stable per object and NOT unique across objects, so this asserted an accident of the run rather than a contract, and it could fail for a reason the class is not about - in a suite that deliberately has no rerun. It also added no coverage: the two assertions above it already pin `hashCode()` to `System.identityHashCode(this)` for both containers, which is what excludes a coordinate-derived hashCode; a coordinate hashCode makes these two collide BY CONSTRUCTION and fails those assertions first, so the tripwire is whole without it. Removed rather than reworded, with a comment saying why it is deliberately absent - the next reader would otherwise re-add it as an obvious omission. ACCOUNTED FOR: the `put` inside FORM 3's remapping function, which discarded the prior occupant it returns. That return value is the direct evidence that the replacement landed on the container the pass was deciding about, which is the precondition the whole form rests on, and a bare call could not be told from an oversight (AGENTS.md -> Code Style, "Never discard a return value silently"). Writing that assertion found something the test did not previously say. Asserting the displaced occupant was `stale` FAILED, reporting a non-equal instance of the same class - it was `fresh`, and the only way `put` returns `fresh` is on a pass after `fresh` was already planted. The remapping function runs TWICE: the first pass is handed `stale`, plants `fresh` and returns null to ask for a removal; `doRemove(key, stale)` re-reads the node, finds `fresh`, and its `equals` gate declines, so it removes nothing and `computeIfPresent` loops; the second pass is handed `fresh` and commits. That loop IS the declining gate, observed rather than inferred from the final state - under the old coordinate equality the gate passed on the first pass and the replacement was destroyed there. It is recorded in a comment and the assertion is against `resident`, the value each pass was handed, which holds on both. The pass count is deliberately not asserted: it is a property of this JDK's `ConcurrentSkipListMap`, and an implementation reaching the same declining gate without looping would be just as correct, so pinning it would buy a red on an unrelated upgrade. `ShardStaleSweepReplacementEvictionTest` 3/3 green, the `state` package 30 classes / 171 tests / 0 failures, `bin/check-all.sh` 16 gates passed and 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…rphan is known unreachable Two things moved under the burn-down on 2026-09-08. The owner's #481 supersedes #431 for the retry-queue lock on the rebalance path: the poll thread never touches the queue and the controller collects what it leaves. #431 is closed; tier 1 now lists #481, ready and green, and notes that #483 stacks on it. The sweep's reading, moved into this note earlier, still called #431 a draft; it is annotated rather than rewritten, since it is the sweep's dated reading. The shard-displacement orphan window is known unreachable by #483, with the one caveat worth carrying: the proof's last leg is a Kafka property (the fetch position never goes backwards within a generation), so an in-generation offset replay reopens the window and nothing goes red. #481's purge bounds the cost to one tick of misdirection. Two more by-key removals of the same shape are named and left for #468, whose identity-equals change is what makes conditional removal possible. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
One conflict, docs/inflight/test-857-churn-storm-async-stalls.md: #468 landed on master carrying its 2026-09-07 sighting of the ZOMBIE_MEMBER arm, appended at the tail of the ledger, and this branch had appended its own 2026-09-08 sighting of the outer-wait failure in the same place. Both are kept, in date order - master's 2026-09-07 entry first, this branch's 2026-09-08 entry after it. Neither side's text changes; the two sightings are different arms with different seeds and each already argues its own branch against on a mechanism. 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
… merged Two more lines of the burn-down move on 2026-09-08. The first known unknown - whether the six deadlock captures ever replay clean with #29's fix applied - turned out to be a question no replay could answer, and #485 says why: the captures identify the defect by a thread BLOCKED on a monitor, the fix replaced that monitor with a lock, and a thread waiting on a lock parks. A clean replay would have reported no BLOCKED frame with or without the deadlock. The fix is proven instead on the deterministic probe with a control arm: every run passes on both assignors with the fix, every run fails with the deadlock deliberately restored, and every dump from the control arm shows the waiting frame. The deadlock line now carries a PROPOSED-closed marker for the owner. Two side findings are kept: the JVM's deadlock detector cannot see this cycle, and one replay in two was void because the window never opened. #468 merged: WorkContainer equality is identity, so the stale sweep removes only the container it inspected. Its body records the harm as a lost record - the fresh container evicted while its offset stayed incomplete, so the commit high-water mark could not pass it - which is why it is in a bug release, and why it is marked breaking for the RecordContext equality change the release note must carry. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…rom the other side #468 merged to master (7c95b75) between this branch's #481 merge and now. It is the defect class this work's sweep reports on, so leaving it unmerged would have published a sweep naming an open PR as the owner of candidates that PR had already settled. WHAT CONFLICTED ONE conflict, in ProcessingShard.getWorkIfAvailable's last-resort stale sweep, where both sides rewrote the same comment. Neither side was a superset: - #481 replaced the "orphans it forever / the load gate reads low" claim, which its purge makes false, with the purge that now collects the entry. - #468 added why the removal is conditional on the inspected container, and a dated CLEARED SUSPICION for this site. Resolved by keeping BOTH - #481's purge sentence in place of the false claim, and #468's conditional-removal reasoning and cleared suspicion after it. Taking either whole side would have silently dropped the other's content, and a green build would not have shown it. THE RESULT WORTH RECORDING #468's cleared suspicion at that sweep rests on exactly the fact leg one of this work's proof rests on: addWorkContainer is the ONLY writer of workMap outside tests and runs on the controller. Two people reached the same single-writer discriminator from opposite directions, days apart, neither citing the other. That means the two clearances share ONE reopening condition - anything that puts into a shard off the controller thread - and nothing gates it. Said at all four places that now depend on it: both code comments, the solutions write-up, and the test's class javadoc, so a future change cannot invalidate one while reading only the other. CITATIONS BROKEN BY THE COMBINATION, AND REPAIRED #468 retired docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md when it merged; #481 cites that note in ShardManager.onFailure's javadoc and in its solutions write-up. Neither PR is wrong on its own and neither would have failed its own gate - the dangling reference exists only in the merge. Both repaired the way docs/citations.md and check-file-refs.sh prescribe, by pointing at the history that still holds the file (git show 7c95b75^:docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md). EVIDENCE ShardDisplacementOrphanReachabilityTest 3/3, RetryQueueRebalancePathTest 6/6, RetryQueueRequeueWindowTest 9/9, ProcessingShardStaleReplacement909Test 4/4, ShardManagerStaleContainerTest and ShardAvailableCountOwnershipTest green - 31 tests, 0 failures. bin/check-all.sh 16 passed, 0 failed. The ablation was re-run a THIRD time, on this tree. #468 changes each sweep removal into a compare-and-remove, so the previous run was again evidence about a shape that no longer exists. Same result: either sweep alone green, both removed red at the same assertion. The write-up now records all three runs rather than the latest one. Co-authored-by: Claude Opus <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…#479 after v6 The #468 tick said "this PR" where it meant #468, which the self-reference gate rightly refused. #479, the God-class decomposition plan opened since the first survey, is a refactor track that follows the release, so it joins the can-follow list by name. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…ntries Picks up #466 (revoke-path commit hands itself to the control thread), #468 (WorkContainer equality is identity, so the stale sweep removes only the container it inspected), #471 and #476. One conflict, comment-only, in ProcessingShard.getWorkIfAvailable's controller-side stale branch: #468's conditional-removal reasoning and its dated cleared suspicion are kept; its sentence "leaving the queue entry behind orphans it forever - nothing else removes an entry whose container is in no shard" is replaced by this branch's, because ShardManager.purgeDepartedRetryEntries() now does exactly that on the pass after. The code beneath merged clean: #468's evictIfStillResident plus the controller-thread retryQueue.remove. Two citations of docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md - which #468 retired - repaired to history pointers per docs/citations.md, in the solutions write-up and ShardManager's javadoc. #468 also settles the one premise this branch's purge rested on by argument: it removes entries by key while equality is identity, safe here only because the queue has one writer. Verified on the merged tree from fresh reports: RetryQueueRebalancePathTest, RetryQueueRequeueWindowTest, RetryQueueTest, RetryQueueIteratorConfinementTest, ShardManagerTest, ShardPopulationRaceTest, WorkManagerTest, WorkManagerStaleCheckDoubleLookupTest, ArchitectureTest, RebalanceCallbackRuleControlTest - all green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Master had moved 23 commits ahead, two of them landing in this branch's own area: #466 (the revoke-path commit hands itself to the control thread) and #468 (WorkContainer equality is identity). Eight conflicts, plus two duplicate declarations git merged cleanly into code that would not compile. Conflicts, and how each was resolved: - `internal/AbstractParallelEoSStreamProcessor.java`, `maybeAcquireCommitLock`. Master added the `revokeCommitRequested` parameter; this branch added the `!isProducerBeingReplaced()` gate. Both kept: the gate stays on the PERIODIC arm (`periodicCommitDue`) and a revocation request still overrides it, as master's javadoc directly above the method says it must - a pass that cannot commit fails the request through `controlLoopPass`'s existing catch, where declining silently would leave the callback to sit out `commitLockAcquisitionTimeout`. Stated in a comment at the site, because the interaction is not obvious from either side alone. - `internal/AbstractParallelEoSStreamProcessor.java`, the mailbox drain loop. Master added the `action.isWakeUp()` skip (the mailbox wake-up message #466 introduced); this branch wrapped each action so a throwing successful-work listener cannot strand the results behind it. Both kept: the wake-up skip is first, the try/catch and the MDC try/finally around it. - `state/PartitionState.java`, `onSuccess(long)`'s javadoc. Both sides added prose only. Master's account of the three closed assert shapes is kept as the description; this branch's `@see` to the container overload follows it as a block tag. - `TransactionalClaim.java`. Both sides appended to C14 `RESULTS_EXACTLY_ONCE_UNDER_FAILURE`, and this branch also added C15 `PRODUCER_INVALIDATION_RECOVERED`. Master's C14 evidence supersedes - it records the duplication route being observed and closed by #466 - and C15 follows it unchanged. - `docs/inflight/test-untracked-ci-flakes.md`. Master's four rewritten rows supersede this branch's older copies of them (the mutation lane's step-level timeout, a third `ManagedPCInstanceLifecycleTest` sighting, the `RegistrationRaceStaleResidentIT` row restated as "recurring", the `AmbientProbeExtensionTest` row folding in its third method). This branch's own measurements of the probe - the #225 runs and the three 2026-09-03 full runs on this branch and its unmodified base - are re-applied into master's row, and the four rows master never had (`LoadFactorCeilingReportingTest`, the two Maven-download lane rows, `TransactionTimeoutsTest`) are kept. - `docs/inflight/test-857-churn-storm-async-stalls.md`. Purely additive on both sides: this branch's 2026-09-05 `ZOMBIE_MEMBER` sighting, then master's 2026-09-07 and 2026-09-08 ones, in date order. - `docs/inflight/core-recoverable-producer-fencing.md`. Master's vetting sweep re-stamped the note; its `inflight-vetted` marker is kept. Its `inflight-impact: reliability` is NOT: this branch had moved the note to `crash`, which is what `docs/inflight/AGENTS.md` prescribes for a feature whose motivation is PC shutting down, so the branch's classification stands. - `config/infer-known-findings.txt`. Master retired the `NULLPTR_DEREFERENCE AbstractParallelEoSStreamProcessor.processWorkCompleteMailBox` line; this branch had kept it with a comment saying it fires again once the recovery pass moves into `ProducerRecoveryPass`. The branch's line is kept, because that measurement was taken on this branch's merged head and the code that brings the finding back is this branch's. The comment now records master's retirement and why this side differs. If the Infer lane reports the line as no longer firing on the merged tree, that is the signal to retire it here too. - `docs/inflight/bug-wedged-after-poisoned-transaction.md`, modify/delete. Master's grooming sweep (#476, owner-accepted 2026-09-08) folded this note into `bug-poisoned-transaction-not-aborted-while-running.md` and deleted it; this branch had appended its "Fixed, 2026-09-03" account to it. The deletion is accepted, because the fold is what this branch's own text already said should happen - the remainder "stays with bug-poisoned-transaction-not-aborted-while-running.md, which owns it" - and the surviving note auto-merged carrying BOTH master's fold-in and this branch's fixed-half section. Three citations the deletion left dangling are repaired rather than dropped: the surviving note's own sentence and `ProducerManagerDetectionTest`'s javadoc name the survivor, and the dated plan `docs/plans/2026-09-02-001-feat-recoverable-producer-fencing-plan.md` gets an address repair only (a `git show 6aab3ff^:` history pointer with a grep anchor, per docs/citations.md) - its claim is untouched. Two clean merges that did not compile, both duplicate declarations: - `assertOnControlThread(String)` was added independently by both sides. Master's copy survives; its message is the richer one and names the revoke path, and `ProducerRecoveryTest` asserts only on "confined to the control thread", which it contains. Its javadoc now also names `maybeRecoverProducer` and `replayWorkDiscardedByAbortedTransaction`, the callers this branch added. - `CONTROL_THREAD` existed twice with the same value - this branch's public `PartitionState.CONTROL_THREAD`, used by every `@ThreadConfined` annotation, and master's package-private field. Master's field and its javadoc are kept but now alias the public constant, so "pc-control" is written once. Verification: `./mvnw -Pci -pl parallel-consumer-core -am` compiles main, test and test-integration sources; 157 unit tests across `ProducerManagerTest`, `ProducerManagerDetectionTest`, `ProducerManagerRecoveryTest`, `ProducerRecoveryTest`, `PoisonedTransactionConditionTest`, `RecoverableProducerConditionTest`, `TransactionalClaimCoverageTest`, `TransactionalBatchProduceTest`, `TransactionalBulkCommitTest`, `RevokeOnTheControlThreadTest`, `MailboxDrainConfinementTest`, `AbortedTransactionReplayStepTest`, the `PartitionState*` classes, `WorkManagerTest`, `LastCommitTimeFenceTest` and `PollContextInternalTest` pass with 0 failures and 0 errors from fresh surefire reports. `bin/check-all.sh`: 16 passed, 0 failed. `bin/check-branch-self-reference.sh`, `bin/check-file-refs.sh` and `bin/check-issue-refs.sh` all clean. No Docker-backed test was run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
wins the transactional revoke path Master landed #466 (the revoke-path commit hands itself to the control thread), #468 (WorkContainer equality is identity) and #471, and this PR went CONFLICTING. #466 is a different answer to the question this branch answered: in transactional mode onPartitionsRevoked no longer commits on the poll thread at all - it posts a request, wakes the control loop through the mailbox, and waits, bounded by commitLockAcquisitionTimeout; the confluentinc#548 spin is gone; and it refutes this branch's decline by experiment (a revoke that commits nothing leaves its output in the open transaction for the next commit to publish without the offset - the same duplicate through a different door). Master's own re-premising of docs/inflight/bug-857-transactional-revoke-wait.md says what is left for this PR: not the absence of a bound, but whether the bound is the right value - five minutes, against a max.poll.interval.ms it can exceed. So the resolution is master's on that path, per file: - AbstractParallelEoSStreamProcessor: master's onPartitionsRevoked, commitOnRevokeViaTheControlThread and the no-argument consumer-commit tryCommitOffsetsOnRevoke replace this branch's parameterised decline, its performCommit extraction and its post-catch wake (moot: the served commit runs on the control thread, which recovers itself); commitOffsetsThatAreReady is master's again; the mailbox loop keeps master's wake-up message skip in front of #410's first-failure try; one of two identical assertOnControlThread helpers (#410's and master's) is kept - master's, which names the new design. - RebalanceEoSDeadlockTest: master's whole file. Its unamended assertion that committed offsets advance inside the callback holds again by construction under #466, and it now reads the output topic at read_committed for duplicates; this branch's decline amendment is superseded. - ArchitectureTest: master's whole file (#465). This branch's interface-hop widening does not merge onto it; its note now records that the blind spot is still open on master and the widening is to be re-applied on top of #465 as its own change. - ProducerManagerTest: master's revoke-request tests and this branch's five revocation tests, both kept; PartitionState: master's onSuccess(long) javadoc with #410's ledger cross-reference folded in; TransactionalClaim: master's scope note plus #410's C15. - config/infer-known-findings.txt, bug-857-family.md, core-recoverable-producer-fencing.md: master; bug-wedged-after-poisoned-transaction.md: master's deletion (the grooming sweep); the three vetted notes keep master's markers and this branch's concurrency label; test-untracked-ci-flakes keeps master's rows and this branch's three later sightings. - ProducerRecoveryTest's revoke-path fence test is retargeted to the served-commit shape: the fence now fires on the control thread inside the served pass, the callback returns promptly on the failed pass, nothing is stranded, and the replacement is built. Its wake assertion is gone with the wake. What is red, on purpose: Revoke857TransactionalWaitProbeIT, 5/5, with the callback at 19.2s of a 20s in-flight dwell against its 10s poll-interval budget - and never the 79s starvation the spin produced. That is the measurement of #466's bound, and it is this PR's remaining acceptance test, not a broken instrument. What is now dead main code, held for the owner's call: the three ProducerManager revocation lock helpers and the DeclineCountingProducerManager instrument, which count a decline the transactional path no longer makes. Verified: ProducerRecoveryTest, ProducerManagerTest, ProducerManagerDetectionTest, ArchitectureTest and the convention rules, PartitionStateAbortedTransactionReplayTest, TransactionalClaimCoverageTest; RebalanceEoSDeadlockTest 5/5, ProducerFencingRecoveryIT 2/2. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Master landed the async-commit acknowledgement (#470, silent loss on the default commit mode) and the quarantine clearance (#473), which empties docs/quarantined-tests.md so the release guard no longer blocks on it and moves the capacity profiles behind a measured tag. With #466 and #468 already in, tier 1 has four of its seven merged; what is left is #469, #481, #480 and #477. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…-fix Picks up #468, #470, #471 and #473. Read for whether any of them moves the ground under this branch's evidence, since that is exactly what #466 did to the 2026-08-31 grid and is why this branch had to re-calibrate: - #470 is the one that looks like it might. It changes when an ASYNC commit counts as committed - AbstractOffsetCommitter, ConsumerOffsetCommitter and PartitionState - so it is the commit path, but not the revoke path. This branch's grid measures the poll-versus-control lock cycle in tryCommitOffsetsOnRevoke, under the synchronous consumer mode, and the probe's failing edge is the commit-response timeout rather than the clean-marking that #470 rewrites. - #468 (WorkContainer identity equality), #471 (the commit-response timeout soak) and #473 (quarantine entries) touch neither. Checked rather than assumed: the diff of AbstractParallelEoSStreamProcessor.java across this merge contains no change to tryCommitOffsetsOnRevoke, commitLock, onPartitionsRevoked or commitOnRevokeViaTheControlThread - the file is not in the merged range's changed set at all. So the re-calibration stays pinned at 6aab3ff, and the head it names is still the right one to state. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…ller collects what it leaves (#481) WHAT CHANGED FOR A USER: a rebalance can no longer be delayed by however long the control loop's retry-queue scan takes, in any ordering or commit mode. `RetryQueue.remove` takes an unbounded, FAIR `writeLock().lock()`, and every Kafka rebalance callback reached it on the broker-poll thread - by direct call through `ShardManager.removeWorkFromShardFor` on revoke and lost, and by the method reference `.map(retryQueue::remove)` through `ShardManager.removeStaleContainers` on all three, assigned included. The controller thread holds the matching READ lock for a whole scan (`RetryQueue.iterator()` hands it out; `ShardManager.getLowestRetryTime` closes it only after a full iteration), and a fair lock queues an arriving writer behind that scan rather than interleaving. The wait sits inside `consumer.poll()` with the whole group waiting, spent out of `max.poll.interval.ms`. No cycle was found: an unbounded wait, not a deadlock. No fork issue exists; found by the confluentinc#857 defect-class sweep (#29). THE FIX: THE POLL THREAD NEVER TOUCHES THE RETRY QUEUE. The callbacks remove from the SHARDS only - a `ConcurrentHashMap` of `ConcurrentSkipListMap`s, no lock to wait for and so nothing to decline. `ShardManager.purgeDepartedRetryEntries()` then collects, on the controller thread where waiting is allowed, every retry-queue entry whose container `ProcessingShard.isResident` says has left: scan under the read lock, close the iterator, remove the collected entries under the write lock in one `removeAll`. The invariant, stated once at the purge: a retry-queue entry with no resident container is garbage the controller collects; it may exist for at most one control-loop tick; the callbacks are free to create it. Where it runs: at the top of `ShardManager.getWorkIfAvailable`, the one point the control loop reaches unconditionally every pass in both RUNNING and DRAINING, and before `drain()` reads `getNumberOfWorkQueuedInShardsAwaitingSelection` in the same pass - the consequence that matters, because #437 established that a queue-only orphan surviving that figure holds a draining close open to its timeout. The bound is one tick in every ordering mode: the purge scans the queue rather than the shards, so the ordered modes' shard-scan break cannot delay it. #468 landed while this was open and settles by construction what this branch argued: `WorkContainer` equality is now identity, so the controller-side stale branch's compare-and-remove evicts only the container it inspected, and the purge's by-key removal no longer rests on the single-writer argument alone. Why scan and remove may be two steps: departure is monotonic (`isResident` is reference identity and nothing re-inserts the same instance), and the queue has exactly one writer - `RetryQueue.add` is `@ControllerThreadOnly`, its only production caller is `ShardManager.onFailure`, and the purge runs on that same thread. A second writer is what would reopen this, and both sites now say so: a `false` from a removal there can only mean one exists. THE DESIGN IT SUPERSEDES, NAMED. #431 kept the poll thread on the queue and made it DECLINE: `tryRemove`, abandon the paired shard removal on refusal, and after #437 a second ask after the shard removal with a put-back on refusal. It was correct and proven, and it lost on machinery: a second non-blocking entry point keyed differently from the blocking one, a queue reference threaded into a second `ProcessingShard` method, two asks per removal, and a retirement bound that varied by ordering mode. Also rejected: reusing #466's control-thread hand-off, which has the poll thread WAIT on a future - right for a revocation commit, wrong here, because the purge needs no hand-off at all. COUNTERS DURING THE WINDOW, RE-MEASURED. Between a callback's shard removal and the purge, `getWorkableRecords` reads one LOW - the safe direction: `isSufficientlyLoaded()` reads false and the poller fetches sooner; high would have paused it with nothing to process. `WorkManagerTest`'s revocation arm pins the direction. #437's post-add residency confirmation in `ShardManager.onFailure` becomes belt-and-braces and is kept this round, its javadoc saying so. THE STATIC GATE GOES GREEN ON MERIT. `ArchitectureTest.rebalanceCallbacksMustNotBlock` (widened in #465 to follow method references and `@ControllerThreadOnly`) carried eighteen exemptions for exactly these reaches. With the set emptied and the callbacks still reaching the queue it reports every route; with the callbacks off the queue it is green and the set stays empty. The set, its `root => target` keying and the argument for keeping it remain - an exemption is how a newly found reach is recorded as a defect on the books. EVIDENCE, AS SHAPE AND REPRODUCE COMMANDS. `RetryQueueRebalancePathTest` holds the read lock the way the controller does - a live iterator on another thread - and drives the real callback on a thread named `broker-poll`: red on unfixed code, both callbacks parked on the write lock until the deadline; green after, with the departed entry collected on the next controller pass, a queue-only entry no longer holding drain open, the ordered-mode arm collected on the same tick, and a still-resident entry left alone as the control. Ablations: deleting the purge call reds arms across three classes; inverting the residency predicate reds the same plus the control. Two arms from #437's window test are re-stated rather than deleted, because the sweep they described stopped existing. The contended harness was then folded into the shared `BlockedThreadAsserter` (a `returnsWithin` idiom that returns the fact rather than asserting it, so a caller can hold the contention while it reads state), re-proved red by restoring the queue removal to both callback paths. The three `DLS_DEAD_LOCAL_STORE` findings this branch's own lines introduced are cleared by using the values - logged, or a `void` return - rather than parking them. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -Pci -pl parallel-consumer-core -am test -Dtest='RetryQueueRebalancePathTest,RetryQueueRequeueWindowTest,WorkManagerTest,ArchitectureTest'`; `bin/check-pr-analysis-surfaces.sh 481`. RECORDED, NOT APPLIED. The purge is the first unconditional full scan of the retry queue on the control loop; both simplify reviewers converged on gating it behind a departure signal (a monotonic `LongAdder` in `RecordPopulation`'s shape). Not done here: it would put poll-thread-written state back on the rebalance path, and the cost is unmeasured - `DispatchScanMeter` is the precedent for metering first. Filed as `docs/inflight/perf-retry-queue-purge-scan-is-unmetered.md` with that order stated. RECORDS. `docs/solutions/runtime-errors/retry-queue-write-lock-on-the-rebalance-path.md` records both designs. Retired: `docs/inflight/bug-retry-queue-write-lock-on-the-rebalance-path.md`, `pr-431-must-pair-its-queue-removal-with-the-shard-removal.md`, and the branch note that guarded the superseded PR while it was still open. Corrected beside their dated text: the ArchUnit rules note (the set is empty on merit), the RetryQueue coverage note, the shard-displacement note (its harm is now bounded by one tick), the runtime-ownership-guard note (the single-writer premise makes that guard more load-bearing), two notes that named #431 as the fix, and the orphan-window write-up (its assertion stays as written; the confirmation is belt-and-braces). Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…s now canonical #481 landed on master as c2d35c0. This branch carried #481's original branch commits, so every region reconciled by hand last time came back as a conflict between #481-as-merged and #481-as-this-branch-had-it. Master wins those; only the reachability verdict, the test and the cleared-suspicion comment are layered on top. FIVE HUNKS, AND THE RULE APPLIED TO EACH - ShardManager.onFailure javadoc, the citation to the note #468 retired - MASTER. Both sides had repaired the same dangling reference the same way; master carries the wording added when #481 merged master. - The identical citation in docs/solutions/runtime-errors/retry-queue-write-lock- on-the-rebalance-path.md (add/add - the file is new on both sides) - MASTER, same reason. That file is now byte-identical to master. - ShardManager.onFailure javadoc, the paragraph after "What else the purge now backstops" - OURS. Master has #481's paragraph and nothing else there; this side is the verdict layered under it, which is the content this branch adds. - The inflight note's closing section - OURS. Master ends at "Production reachability is still not established"; this side answers it. - The inflight note's #431 paragraph - OURS, and this is the ONE hunk where master's text is not kept, so the reason is worth stating. Master says "The 2026-09-07 stamp ABOVE says ...". That stamp is not above it here: this branch replaced it with the 2026-09-08 PROPOSED marker, because bin/check-inflight-tags.sh refuses a note carrying two. Keeping master's sentence would leave a pointer to something the merged file does not contain, so this side's wording stands - it makes #481's point, and records what the superseded stamp said so the supersession loses nothing. THE TWO AUTO-MERGES THAT WERE WRONG LAST TIME, RE-CHECKED Both correct this round, because the corrections were already committed rather than being made against #481 for the first time: - ProcessingShard's cleared-suspicion block still reads "the sweep that takes it OUT OF ITS SHARD" and not "from BOTH structures" - the wording #481 falsifies. Its diff against master is exactly this branch's own block and nothing else. - The note carries exactly one inflight-vetted marker. NOT A LOSS, THOUGH IT LOOKED LIKE ONE A sweep for files differing from master that this branch never edited flagged #482's five (BatchTestMethods, CoreBatchTest, KafkaTestUtils and two docs). Cause: #482 landed on master DURING this resolution, so MERGE_HEAD is c2d35c0 and #482 is simply not in this merge. Nothing was dropped; it arrives in the next one. Recorded because "file differs from master and I never touched it" is normally the signature of a silent merge loss, and here it was not. EVIDENCE ShardDisplacementOrphanReachabilityTest 3/3, RetryQueueRebalancePathTest 6/6, RetryQueueRequeueWindowTest 9/9, RetryQueueTest 7/7, RetryQueueIteratorConfinementTest 5/5, ProcessingShardStaleReplacement909Test 4/4, ShardManagerStaleContainerTest 3/3 - 37 tests, 0 failures. bin/check-all.sh 16 passed, 0 failed. Co-authored-by: Claude Opus <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… and the guard is outside the class (#483) Settles the one open question on docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md: whether production can reach ProcessingShard.addWorkContainer's displacement branch with a queue-resident container, which decides whether the pairing gap there is worth a design change. It cannot. No behaviour changes - the diff is a test, a solutions write-up, and comments. The branch itself IS reachable: confluentinc#909's late drain puts a stale resident in a shard and the next poll's fresh record displaces it. What cannot happen is that resident also holding a retry-queue entry. THE ARGUMENT, EACH LEG TRACED IN SOURCE An orphan needs four things true at once when a container B arrives at (topic, partition, offset): a resident A; A observed STALE, or the arrival is dropped; A holding a RetryQueue entry; and B existing at all. 1. The displaced container is always the one inspected. workMap has two insertion sites - addWorkContainer, which is control-thread-only, and the test-only plantResident. Only removals can interleave between the get and the put, and a removal makes the put return null. A by-key REMOVAL can hit a different occupant (#468's class); a by-key insertion cannot, with one inserting thread. 2. A queue entry implies non-stale-and-resident when it was made. RetryQueue.add has exactly one production caller, ShardManager.onFailure, behind onFailureResult's live staleness check and, since #437, a post-add residency confirmation. couldBeTakenAsWork refuses a stale container outright, so one that is stale on arrival is never selected, never fails, never queues. 3. Staleness is monotone: both epochs are final longs, the fence is set and never cleared, a replaced state carries a higher epoch or is the removed singleton. 4. So A must cross the staleness boundary in between, and only three transitions can do it - the removed-state swap, the assignment putAll, and fenceForRevocation. The first two each unseat A on the same thread inside the rebalance callback; the fence sweeps nothing but cannot be followed by a second container at the same offset, because a duplicate offset needs a re-assignment and that is the swept path. Bumping the epoch map alone is NOT one of those transitions, and this is the leg most easily got wrong by reading: PartitionState's epoch is final and staleness is only asked through the state object, so the window opens at the state REPLACEMENT - which is the same statement as the sweep that closes it. WHAT THIS DOES NOT CLAIM, AND IT IS THE POINT OF THE WRITE-UP The last leg is a property of the KAFKA CONSUMER's fetch position, not of this engine: within one assignment generation the position never goes backwards, so a duplicate offset needs a re-assignment. Nothing in main calls seek, and shards are partition-scoped in every ordering mode. So the pairing gap in that branch is one arrival away from being real rather than absent. An in-generation replay of an already-registered offset - a seek, an offset-reset or truncation replay - makes it reachable immediately, and nothing goes red for that. #481's purge would then collect it within one control-loop tick, which is the second, independent answer: the harm is bounded whatever happens, and this work says the case does not arise. Neither result implies the other. ORDERING-MODE INDEPENDENCE, RAISED IN REVIEW AND CLOSED UNDER TEST Legs one to six never consult the shard key. The mode is visible only in leg seven's second half, and is safe in every mode because a shard is partition-scoped throughout - PARTITION and UNORDERED make it a topic-partition, KEY makes it topic-plus-partition-plus-key. The disproof arm is parameterised over every ProcessingOrder, and a dedicated arm covers the KEY half: two records sharing a key and an offset on different partitions must be held in two shards, not collide in one. Under KEY an emptied shard is garbage-collected, which makes the conclusion stronger, not weaker - a collected shard holds nothing to displace. HOW IT WAS ESTABLISHED ShardDisplacementOrphanReachabilityTest, four arms over six cases, control-armed because a single green arm here is green whether the sweep works or the branch is simply never reached: the production rebalance sequence; confluentinc#909's late drain, which fires the branch with an unqueued container; a positive control that reaches stale-and-resident-and-queued through the production API via the fence and injects the one arrival production cannot supply, where the orphan appears immediately, asserted by reference; and the KEY-ordering collision arm. Ablation, predicted before running and re-run after each of the three merges this branch took, because #481 made the sweeps shard-only and #468 made each removal a compare-and-remove - an ablation against a shape that no longer exists is evidence about nothing. Same result every time: either sweep alone green, both removed red at the load-bearing assertion. The KEY arm was separately verified by sabotage - making the key-ordered shard key topic-scoped turns exactly that arm red, and ShardKeyTest stays green under the same sabotage, which is why the arm is not redundant with it. Two predictions were refuted and are kept visible rather than quietly fixed: that the epoch bump opens the window, and that the replacement would be handed out on the controller pass after the fence - it is not, because the fence makes the whole partition stale. DEFECT-CLASS SWEEP - BY-KEY REMOVALS THAT CAN HIT A DIFFERENT OCCUPANT Reported against the post-astubbs#468 tree rather than fixed here: - ProcessingShard.removeStaleWorkContainersFromShard and getWorkIfAvailable's last-resort sweep - both now conditional on the inspected container. Fixed by #468. - ProcessingShard.onSuccess - still removes by key. #468 reasoned about it explicitly and left it, on the grounds that the container it removes cannot have been replaced. Checked, and left to that argument. - ShardManager.removeWorkFromShardFor - the revoke path's removeWorkAtOffset on the poll thread, and the one remaining unconditional by-key shard removal. Reported, not fixed: it belongs with #468's mechanism, not with a reachability proof. - RetryQueue.remove / removeAll - by coordinates and never by identity, at every call site. Deliberate, but it means a removal can clear a replacement's entry; ShardManager.onSuccess removes by key before it looks up any shard, so a late success for a departed container clears a fresh one's parked-for-retry entry. The cost is an early retry, not a stall or a loss. Noted, not raised as a defect. A cleared suspicion #468 recorded independently at the last-resort sweep rests on the SAME single-writer fact as leg one, reached from the other direction. Both clearances therefore reopen on one ungated condition - anything that puts into a shard off the controller thread - and that is now said at every site that depends on it. WHAT IS LEFT FOR THE OWNER The inflight note stays OPEN with a PROPOSED closed - unreachable AND bounded vetted marker rather than being closed here: its impact is misdirection, and docs/inflight/AGENTS.md reserves that state change for the owner. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…nd the PC half is re-verified with a control arm (#486) The 857 register's fifth open item - a rebalance stall in MultiInstanceRebalanceTest.largeNumberOfInstances under PERIODIC_CONSUMER_ASYNCHRONOUS/UNORDERED - read as blocked on progress-tracker instrumentation that does not exist. It is not blocked, and has not been since #444: withDiagnostic is wired on master, and the stall has been reproduced repeatedly since, not never. Both stale claims were still in the section, so anyone picking the item up inherited an investigation that had already finished. The mechanism is measured, and its durable owner is the docs/solutions/test-flakiness write-up that #473 promoted: the chaos monkey restarts members faster than a join phase completes, a LeaveGroup sent while the member's own JoinGroup is pending is not answered until that phase completes, and one phase was observed held open for 17s. While it is open consumer.poll() returns nothing to any member, survivors included - the FLAT count. No coordinator request was slow in any failing run, and every PC-side candidate was refuted by measurement. So the verdict is neither wedge nor harness artefact: the freeze is real and fleet-wide, it recovers when the phase completes, and what turns it red is the detector's 12s window closing inside a real protocol freeze. Master has moved through the revoke and close seam since that measurement (#451, #466, #468), so the PC half was re-asked rather than inherited. Two arms on ClosingMemberRebalanceIT differing by exactly one term - a 20s sleep at the top of BrokerPollSystem's doClose(), which holds a member not-polling and not-left, the defect shape itself. Predictions written before each run: clean master - predicted PASS - 5/5 pass, 35.4s, load average 5.1-7.1 sabotaged - predicted FAIL on the close bound - 5/5 fail, "expected to be less than: 10.0 but was 20.009590667", load average 13.0 The sabotage arm is what makes the clean arm mean anything: every property this test asserts already holds on master, so a green alone cannot say whether the test can see the failure - the VOID-replay trap #485 recorded. The test's own javadoc records the same sabotage shape under its Proven red block, and the register names it as the owner of what the test is calibrated to detect; the rows here are an independent re-run on a later master answering a different question - whether the PC-side story still holds now - and are not that result cited twice. One finding that is new rather than a re-derivation: the sabotaged arm emits this section's own signature line verbatim - ZOMBIE_MEMBER/REBALANCE_BLOCKED, dwelling in PreparingRebalance for 15s. A PC-side hold produces it and so does a coordinator holding its join phase open with PC off the critical path, so that line does not discriminate between the two stories, and a future sighting has to be told apart by what the closing members' threads are in. Deliberately not run: a local replay of the capacity profile. It reproduces at 0 in 22 on an M2 desktop against about 1 in 15 on the Linux runner, so a green here would have carried no information, and the box was at load 5-14 throughout - a confound, not a control. No product code changes. The vetted marker carries a PROPOSED close for the fifth item only, per docs/inflight/AGENTS.md's owner gate for a stall-impact note, alongside #485's PROPOSED close for the revoke-path deadlock line; the fourth item and #119 are untouched. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
Merged
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…ever holds the offset (#492) WHAT CHANGED FOR A USER: a rebalance can no longer remove a record that a later assignment had already re-registered. `ShardManager.removeWorkFromShardFor` - the revoke and lost path - removed by KEY with `removeWorkAtOffset(consumerRecord.offset())`, so whatever occupied the offset when the removal landed is what left the shard. It now removes only the container the revoked record was registered as, or a stale one, and never a live container from a later registration. THE DEFECT CLASS, AND WHERE THIS SITE SITS IN IT. #468 closed the same class at the poller's stale sweep: a removal keyed on the offset cannot say WHICH of two containers at that offset it meant, and the fix was to make `WorkContainer` equality identity so that `Map.remove(key, value)` is a true compare-and-remove. #483's defect-class sweep reported this site alongside it - "the one remaining unconditional by-key shard removal" - and deliberately left it, on the grounds that it belonged with #468's mechanism rather than with a reachability proof. This is that. No fork or upstream issue tracks it; it was reported in a commit body and on a `docs/inflight/` note, so this PR closes nothing. THE CALLER HOLDS NO CONTAINER, WHICH IS WHY THE FIX IS SHAPED DIFFERENTLY. The stale sweep inspects a container and then wants that one gone. This sweep is handed the `ConsumerRecord`s ONE generation was still carrying as incomplete (`PartitionState.onPartitionsRemoved` -> `removeAnyShardEntriesReferencedFrom`), so what it can name is the REGISTRATION, not an inspected container. That works because `PartitionState.maybeRegisterNewPollBatchAsWork` puts the same record instance into `incompleteOffsets` and into the `WorkContainer` it builds, in one loop body - so "the container this generation registered for this record" is `occupant.getCr() == revokedRecord`, and a later generation's re-delivery of the same offset is a different object because it came from a different fetch. That identity has one named exception, stated at the site: `addNewIncompleteRecord` puts unconditionally while `addWorkContainer` DROPS an arrival whose resident is not stale, so an in-generation replay of an already-registered offset leaves the partition naming record B while the shard holds the container over record A. The staleness condition below covers it. TWO CONDITIONS, AND THE MIDDLE OPTION WAS WRONG. Registration identity alone declines for a container from another registration that is nonetheless STALE, and declining there would leave a stale container holding an offset of a revoked partition - the state this sweep exists to prevent. Staleness alone declines for a live container built from the record the sweep was handed, which is what a revoke sweep driven before the partition's state has been swapped sees, and is what `ShardManagerLincheckTest.revokeSweep` models on every invocation: that harness's removal operation would have become a silent no-op while staying green. Verified rather than assumed - `incrementPartitionAssignmentEpoch` starts from `KAFKA_OFFSET_ABSENCE = -1`, so that harness's `EPOCH = 0L` containers are live against the epoch its constructor assigns. So the guard declines for exactly one thing - a LIVE container from another registration - which makes the change a no-op everywhere except in the defect case, and each condition has its own ablation arm. THE HARM, IF IT IS REACHED, IS THE LOST RECORD - #468's, not a misdirection. The live container is gone from the shard while its own `PartitionState` carries the offset as incomplete, so nothing selects it and nothing completes it, and the commit high-water mark cannot pass it until the partition is re-polled. The v6 scope note this came from said the cost was "misdirection bounded to one control-loop tick by #481's purge"; that is the DISPLACEMENT orphan's cost, not this one's. This removal is a shard removal, and the purge collects retry-queue entries. REACHABILITY: NOT TODAY, ON TWO INDEPENDENT ARGUMENTS, AND THE STRONGER ONE IS STRUCTURAL. - Thread ordering, which has to name BOTH production routes. On the ordinary route the poll thread runs both rebalance callbacks, so a generation's revoke sweep completes before the assignment that could register a replacement begins. On the CLOSE route the sweep runs on the CONTROL thread (`maybeCloseConsumer` -> `onLeavePrepare` -> `onPartitionsRevoked`), which `PartitionStateManager.resetOffsetMapAndRemoveWork`'s javadoc calls live in every commit mode - and that is also `addWorkContainer`'s thread, so the two still cannot interleave. "Both callbacks are on the poll thread" is the half-statement, and it was written that way here first. - `resetOffsetMapAndRemoveWork` installs `RemovedPartitionState` BEFORE it calls in, so `checkIfWorkIsStale` is true for EVERY occupant and the staleness condition carries all of them: on the shipped path this method is behaviourally identical to the by-key removal it replaces, and the decline branch is defence for a caller that sweeps before the swap. That says nothing about threads. Nothing checks either argument. The conditional form costs one reference comparison and rests on neither, which is exactly why #468 wrote `getWorkIfAvailable`'s last-resort sweep conditionally against a race it had shown was unreachable there. THE NPE GUARDS ON THIS PATH ARE UNTOUCHED, AND THE CLEARED SUSPICION NAMES THE RIGHT OBJECT. The single-read `getShard` idiom #345 put in stays where it was; the new work is inside `ProcessingShard`, below it. The staleness question resolves the partition state from the OCCUPANT, not from `revokedRecord` - and in the only branch that asks, the occupant is by definition a different registration - so the discriminator is two facts: the state is installed before the call (and a partition with no state `continue`s without sweeping at all), and every `ShardKey` variant is partition-scoped, so an occupant of this shard belongs to the revoked partition. WHAT WOULD REOPEN IT: a caller from a path that has not installed a state, or a topic-scoped shard key - the latter being the reopener `addWorkContainer`'s own cleared suspicion already lists, which reopens both at once. EVIDENCE. `ShardRevokeSweepReplacementEvictionTest`, three arms, RED-then-GREEN through `PartitionState.onPartitionsRemoved` so the argument list is built by production code - one level below the state swap, which is what makes the decline branch reachable at all. Ablation matrix, predicted before running, one arm red per ablation and no other: - whole guard removed, back to `removeWorkAtOffset(offset)` -> the displacement arm - staleness condition removed -> `aStaleContainerFromAnotherRegistrationIsStillSweptOut` - registration-identity condition removed -> `aRevokeSweepStillRemovesALiveContainerBuiltFromTheRecordItWasHanded` - decline retires the occupant instead of leaving it -> the displacement arm's accounting assertion That fourth ablation is why the accounting assertion exists: making the decline call `population.onRetired()` + `excludeFromSelection(occupant)` left every other assertion GREEN while creating a permanent `RecordPopulation` deficit, which is this class's recurring defect and has no clamp and nothing to reconcile it. TWO INSTRUMENTATION FAILURES, RECORDED RATHER THAN TIDIED AWAY. The registration-identity ablation reported NO failure on its first run: dropping the condition left an unused local, the module never recompiled, and the grep for failures matched nothing - a silent false negative in the experiment meant to prove the condition earns its place. Re-run with the build outcome asserted, it is red as predicted. Separately, `@SuppressWarnings("ReferenceEquality")` was written on the identity comparison and then measured: deleted, rebuilt under `-Pci`, `BUILD SUCCESS`. Neither `WorkContainer` nor Kafka's `ConsumerRecord` overrides `equals`, so Error Prone never fired - it suppressed nothing and disagreed with `isResident`'s bare `==` two methods away. Removed, with the reason written where it was. SAME-DEFECT-CLASS SWEEP over `ShardManager`, `ProcessingShard` and `PartitionState`. Fixed: the one above; no unconditional by-key removal of a container remains in main. Dismissed with discriminators: `ProcessingShard.onSuccess` (re-verified rather than inherited - `WorkManager.handleFutureResult` is the third staleness checkpoint and drops a stale result before `onSuccessResult`, so a returning stale container cannot remove the fresh one that replaced it); `addWorkContainer`'s by-key INSERTION (#483's leg 1, plus #336's admit-first, which reads the outcome from the map rather than the earlier read); `removeStaleWorkContainersFromShard` and `getWorkIfAvailable`'s last-resort sweep (already conditional, #468); `ShardManager.getShard` / `compute` / `computeIfPresent` (single-read Optional idiom, #345, and atomic per-key, #336 - not check-then-act); `RetryQueue.remove` / `removeAll` at every call site including `onSuccess`, `onFailure` and `purgeDepartedRetryEntries` (same shape, but it is the queue's keying model rather than a removal that forgot to name its target, so a fix is a change to that model - reported and left by #483, and left here); `PartitionState.incompleteOffsets.remove` and the truncation prune (offsets over records, control-thread-only - "this offset is complete" does not depend on which record object represents it). `removeWorkAtOffset` now has no production caller. Kept as the modelling primitive four test classes use for "a container departs its shard", with its javadoc rewritten: it used to open by naming the `onPartitionsRemoved` caller this change moved away, and now says what it is and which of the two conditional removals a main-code site should reach for instead. RECORDS. `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md` - the class's owner - gains a dated section for the second site, because its own sweep's only hit was a false positive and the real one was found a day later. `docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md` gets a pointer, not a narrative: its subject, state and `PROPOSED closed` marker are untouched, and what is recorded is only that a reader arriving at #483's four-item list can see which one moved. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -pl parallel-consumer-core -am test -Dtest='ShardRevokeSweepReplacementEvictionTest,ShardStaleSweepReplacementEvictionTest,ShardManagerRevokeSweepNpeTest,ShardDisplacementOrphanReachabilityTest'`; `bin/check-pr-analysis-surfaces.sh 492`. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
… named, and five stale lines are corrected Owner's decisions, 2026-09-09: the merge queue is closed as of today, with later finds 0.6.0.x unless data loss on a default configuration; the poisoned-transaction wedge is the second named exception beside #44, and the release-note draft now carries it; the gate-latch warning #487 argued for is v6-sized and joins tier 1 as the last item; the upstream flat-counter reporters are not asked. Corrections from the owner's read of the note: the release page body is posted by hand on the day with gh release edit, because release.yml's exact heading match misses the unreleased heading on master - so #199 follows the tag rather than gating it, and the two lines that said the workflow already publishes the curated section are fixed; the #468 line no longer asks the reader to check a PR body for two by-key removals that #468 dismissed and #492 fixed; the vetting sweep's opening claim that the quarantine registry is non-empty is struck as the sweep's dated reading; and the disposition list names the three deferred bug notes it omitted. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
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
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
The poller's stale sweep asked each occupant whether it was stale and then removed it by key (
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, because the sweep runs on the broker-poll thread inside a rebalance callback whileaddWorkContainer's stale-replacement branch runs on the controller, with nothing ordering them.The harm is a lost record. The fresh container is evicted while
PartitionStatestill 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 by #336 and #373, so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left.This is the same fresh-replacement-at-one-offset class as the confluentinc#909 write-up, reached from the other side - there the fresh container was dropped on arrival, here it arrives successfully and is evicted moments later.
The fix is in the value type, not in the removal
Map.remove(key, value)is the JDK's compare-and-remove: remove the entry only if it is still mapped to the value I inspected. It decides "still mapped to" withequals, so what that call MEANS is a property of the value type and not of the map.WorkContainer.equalswas 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.So
WorkContainer'sequals/hashCodepair is deleted and its equality is reference identity. The shard stores containers directly andworkMap.remove(offset, container)is a true compare-and-remove: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.removeStaleContainersfeeds 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.getWorkIfAvailable's last-resort stale removal has the same shape and is written the same way, carrying a dated cleared-suspicion note: the race is not reachable there today becauseaddWorkContaineris the only writer and runs on the controller, which also runs that scan - and nothing would go red if that changed.RecordContextequalityRecordContext's Lombok@EqualsAndHashCodecovers theWorkContainerit wraps, so two contexts built from different containers for one record no longer compare equal - aSet, aMapkey or anequalscheck over record contexts that used to collapse them now keeps both. Narrow in practice:ConsumerRecorddoes not overrideequalseither, so two contexts from two different polls were never equal; what changes is two contexts over the sameConsumerRecordinstance. This paragraph and the body ofe4917973bare the record of the break -docs/refactoring.md's queued-breaking-change section lists only what is still queued, and an entry is deleted in the PR that lands it, because the release notes are generated from the commit log.WorkContaineritself is public in modifier only - not part of the surface a user is expected to hold.What this replaced, and why
An earlier revision of this branch stored a
ProcessingShard.Residencytoken as the map value - a wrapper overriding neitherequalsnorhashCode, so the map's comparison was identity. It bought exactly one correct removal and left the value type wrong. Replaced before merge on the maintainer's call:0.6.0.0is the breaking release and already carries a=== Breakingsection, and a token is a workaround the next site has to remember to repeat.The objections that had queued identity equality as future work were checked, and each fell:
compareTobecomes inconsistent withequalsComparableonly recommends that consistency;SortedSet/SortedMaprequire it, and nothing in main code puts a raw container in either.RetryQueuesorts by its ownWorkContainerSortKeyand de-duplicates by its ownWorkContainerKey.compareToand its comparator are unchanged, with the inconsistency stated in the javadoc.ExternalEngine.holdingDispatchPermitis anIdentityHashMap-backed set, and has been since #342. Identity equality makes the container agree with that code rather than changing it.RecordContext, above.Also rejected, both at the removal rather than the value type:
computeIfPresentwith an identity check in the remapping function, which narrowed the window and closed nothing (ConcurrentSkipListMapcommits throughdoRemove(key, v), which re-reads the node value and gates onvalue.equals(reRead)- so the identity test did not survive to the commit); a remove-then-put-back repair; and a claim the sweep takes before removing which the writer must wait out.Red and green evidence
ShardSeamTestBase's spiedPartitionStateManager.getPartitionStateso the controller's replacement lands at an exact instruction inside the sweep.equals/hashCodeonWorkContainerand changing nothing else sends 2 of the 3 arms red - the defect arm (expected specific instance: WorkContainer(tp:topic-0:o:100:k:a-key) / but was: null) and the premise arm (expected not to be: WorkContainer(tp:topic-0:o:100:k:a-key)).removeWorkAtOffset(entry.getKey())and changing nothing else sends exactly the defect arm red.equals.twoContainersAtOneOffsetMustNotBeInterchangeableasserts the contract directly - two containers at one offset are not equal, are not the same instance, eachhashCodeis its identity hash - before asserting the map behaviour that follows from it.bin/check-all.shall gates passing,bin/check-issue-refs.shandbin/check-file-refs.shclean. Reproduce:./mvnw -Pci -pl parallel-consumer-core -am test -Dtest=ShardStaleSweepReplacementEvictionTest,bin/ci-unit-test.sh,bin/check-all.sh.An ArchUnit rule keeps the other half of the contract
The javadoc now states that
compareTois deliberately inconsistent with equals, and that a rawWorkContainermay therefore not be an element or a key of a sorted collection -Comparableonly recommends the consistency,SortedSet/SortedMaprequire it. Nothing violates that today, which is exactly why prose was not enough:RetryQueuekeys onWorkContainerSortKeyandProcessingShard.workMapkeys on the offset, and both are one refactor away from being written the other way round with nothing going red.WorkContainerIsNeverInASortedCollectionArchTestreads every field, method return type, method parameter and constructor parameter in main code whose declared type erases to something assignable toSortedSet,NavigableSet,SortedMap,NavigableMap,PriorityQueue,ConcurrentSkipListSetorPriorityBlockingQueue- coveringTreeSet,TreeMapandConcurrentSkipListMapwithout naming them - and whose first declared type argument erases toWorkContainer, viaJavaParameterizedType.getActualTypeArguments().Argument 0 is the element-or-key position for every one of those types, which is why one index serves the set-shaped and the map-shaped ones alike, and why a container in a sorted map's value position passes. Checking every argument would report
ProcessingShard.workMap, be red on arrival against correct code, and be deleted rather than obeyed.Red proof, on main code. Adding
private final java.util.TreeSet<WorkContainer<?, ?>> tripwire = new java.util.TreeSet<>();toProcessingShardand changing nothing else:Exactly one violation, and the field one line above it -
workMap, holding containers as values - was not reported. So that run is the positive and the negative control at once, on the real tree. The field was then removed.A standing control ships with it, because a measurement in a commit message protects nothing. A rule designed to be green forever cannot tell "the code complies" from "the walk sees nothing" on its own runs - the recurring ArchUnit failure recorded in
docs/inflight/static-archunit-main-code-rules.md. So it followsRebalanceCallbackRuleControlTest's shape:archfixture/SortedCollectionOfWorkContainersdeclares the violation in all four positions plus the legitimate map-value case, andWorkContainerSortedCollectionRuleControlTesthands it to the real rule object, not a copy, asserting it reports the four and not the fifth. The fixture is hand-imported because the rule's own@AnalyzeClassescarriesDoNotIncludeTests, which is what stops a deliberate violation turning the production rule permanently red.Three gaps, stated in the test javadoc rather than left to be found: a sorted collection built inside a method body and never stored declares nothing, so it is invisible; the rule cannot see the comparator, so a
Comparator-taking constructor is reported even though it is legitimate when that comparator is notcompareTo(the exemption costs a conversation instead of passing silently); and it runs where core's test classpath reaches, so other modules' main code would need a wrapper per module in theTestConventionRulesshape - measured before deciding against it, no main source outside core declares any of the seven types at all.One red CI shard, recorded rather than diagnosed
Chaos Pain Suite 4/4went red onChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedgerwith theZOMBIE_MEMBER/REBALANCE_BLOCKEDarm -maxRebalanceDwell=15632msagainst a 15000ms bound,maxInstanceStall=0ms. That line has an open ledger on master (docs/inflight/test-857-churn-storm-async-stalls.md) with sightings on unrelated branches including a pom-only PR, and seed1053013618367208111is appended to it here, before this PR merges, because the seed dies with the CI log.The branch is argued against on a mechanism rather than dismissed as unrelated: nothing on the rebalance path reads
WorkContainer.equals; the shard's conditional removal has the same semantics it had one commit earlier through theResidencytoken, which passed this same suite; andholdingDispatchPermitwas already identity-keyed.maxInstanceStall=0msseparates it from the worker-saturation line that file diagnosed on 2026-09-07 - no member was stalled holding work. Not a claim it is a flake; a claim about which evidence exists.Tree-wide sweep for what identity equality breaks
One test assertion needed changing, and it is the only value-equality assertion on a container in the tree:
ShardManagerTest.retryQueueOrderingassertedassertThat(w0).isNotEqualTo(w1)for containers at different offsets. Under identity equality that is true of any two objects and asserts nothing, so it is restated againstRetryQueue.WorkContainerKey- what the queue actually keys on, and what the surrounding assertions are really about. The vacuous form is what was removed; nothing was weakened.One main-code site changes semantics without changing behaviour, and is documented rather than altered:
PollContext.getByTopicPartitionMapcollects freshly constructedRecordContexts to aSet, so its de-duplication becomes a no-op. No observable result moves - a poll set holds at most one record per offset, because each is taken from a shard that holds at most one container per offset, so no two elements were ever equal under the old equality either. Said in an@implNoteat the site.Checked and found empty, with the search named so "nothing" is worth reading: no
ArgumentCaptor<WorkContainer>and no Mockitoverify/whenpassing a container as a matcher value (both match byequals); no TruthcontainsExactly/hasValue/containsEntryover containers except where the same instance is asserted, which is equal under both semantics; noWorkContaineras aMapkey orSetelement in any test helper; everyretryQueue.contains(wc)assertion routes throughWorkContainerKey; and all four Lincheck harnesses returnvoid/boolean/intfrom every@Operationand run in stress rather than model-checking mode, so neither result comparison nor a now-nondeterministichashCodecan reach them.Correction to this branch's own same-defect-class sweep
An earlier commit on this branch reported
ExternalEngine.holdingDispatchPermitas a real unfixed instance - aSet<WorkContainer>leaking a dispatch permit per offset collision - and opened an inflight note for it. That defect does not exist. The field isCollections.newSetFromMap(Collections.synchronizedMap(new IdentityHashMap<>()))and has been since #342, with a javadoc saying it is by identity and why.The sweep matched the declared type
Set<WorkContainer<K, V>>and stopped there. A collection's semantics live in its initialiser, not its declaration, so a search over declarations reports safe and unsafe sites identically. The note is deleted and that lesson is migrated into the solutions write-up's Prevention section.Class dismissals that stand unchanged:
ProcessingShard.onSuccess(a non-stale resident is never replaced, and a stale result never reaches it -handleFutureResultshort-circuits);removeWorkAtOffsetfrom the revocation path (emptying an offset regardless of occupant is its purpose);RetryQueue(keys byWorkContainerKeydeliberately, 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);MDC.remove(thread-local).Docs
docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.mdis resolved and deleted, its mechanism, rejected alternatives and open decision migrated todocs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md, which cross-links the Replace the existing stale container when adding work container confluentinc/parallel-consumer#909 write-up as the other side of the same class. That write-up now records identity equality as the fix, the token as the interim shape, and the false-positive lesson above.docs/inflight/bug-dispatch-permit-set-cannot-tell-two-containers-at-one-offset-apart.mdis deleted - nothing in it was both true and unowned elsewhere.docs/refactoring.md's Breaking changes queued for next major version section now states what happens to an entry once its work lands - it 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. That rule is then applied: the four entries sitting there marked DONE are removed, three naming merged PRs (fix(core)!: user code must not be able to silently stop the consumer #267, fix(core) astubbs#155: stop the "Max loading factor steps reached" WARN on every control loop pass #201, fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue #116, each confirmedMERGED) and the fourth this branch's own. The two citations that pointed at that fourth entry -WorkContainer's class javadoc and the solutions write-up - are repaired to name the commit as the record rather than left dangling.docs/inflight/test-857-churn-storm-async-stalls.md's sighting paragraph carries thepost-merge: checkedblock marker: it names this PR, and it reads correctly after the merge because it is past tense about a permanent PR.Merge of master
One conflict, in
ShardPopulationRaceTest, where both sides were superseded rather than one winning: #467 hoisted that test's privatejoinQuietlyinto the shared testThreadUtils, while this branch had extracted its seam helpers intoShardSeamTestBase. Git paired the two deletions, so--oursor--theirswould each have restored a method whose owner is now elsewhere. Both sides were deleted; the merge commit records why.The second merge brought the inflight vetting sweep, which had stamped
bug-stale-sweep-iterator-evicts-fresh-replacement.mdwith the observation that what remained open was the eviction of the fresh record itself - which is what this PR closes, so the deletion stands over the stamp. Master'sdocs/inflight-tool.mdused that note as the worked example for the newvetcommand; the example now names a surviving note with the same signal (bug-run-length-plausibility-ceiling.md) rather than a file that no longer exists, which is why this PR touches that doc.Review
Codex found two things in the seam test, both this PR's doing and both fixed: the premise arm asserted that two containers' identity hash codes differ, which the JVM does not guarantee, so it was an unretriable flake and is removed with a comment saying why it is deliberately absent. And the
putinside the nestedcomputeIfPresentform discarded its return value; asserting it showed the remapping function runs twice - the first pass plants the replacement and asks for a removal,doRemovere-reads, finds the replacement, itsequalsgate declines, andcomputeIfPresentloops into a second pass that is handed the replacement. That is the declining gate observed directly rather than inferred from the final state, and it is recorded at the site.Checklist
docs/solutions/write-up, the false-positive note is deleted, anddocs/refactoring.mdrecords the landed breaking changedocs/features/-N/A - an internal concurrency fix; the one user-visible consequence is the RecordContext equality change, which belongs in the release notes and is recorded where they are assembled fromdocs/inflight/working note (pr-/branch-) started at the PR's first commit -N/A - this PR resolves a pre-existing bug note (bug-stale-sweep-iterator-evicts-fresh-replacement.md), which is the working note it inherited; nothing about the branch itself is unavailable to ghce-simplifyandce-code-reviewlocally -N/A - asked for @claude review on the PR instead; the identity/equality argument is exactly what an adversarial reviewer should attack, and inline threads are what block merge here🤖 Generated with Claude Code
https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK