Skip to content

Commit 7c95b75

Browse files
astubbsclaude
andauthored
fix(core)!: WorkContainer equality is identity, so the stale sweep removes only the container it inspected (#468)
BREAKING CHANGE: RecordContext equality changes. Its Lombok @EqualsAndHashCode covers the WorkContainer it wraps, so two contexts built from different containers for one record no longer compare equal - a Set, a Map key or an equals check over record contexts that used to collapse them now keeps both. Narrow in practice: ConsumerRecord does not override equals either, so two contexts from two different polls were never equal; what changes is two contexts over the same ConsumerRecord instance. WorkContainer itself is public in modifier only and is not part of the surface a user is expected to hold. THE DEFECT. The poller's stale sweep asked each occupant whether it was stale and then removed it by KEY, with removeWorkAtOffset(entry.getKey()). The staleness answer is about one container; the removal is about one offset. Between the two statements the occupant can change, and it does: the sweep runs on the broker-poll thread inside a rebalance callback, while addWorkContainer's stale-replacement branch runs on the controller, and nothing orders them. The harm is a LOST RECORD. The fresh container is evicted while PartitionState still carries its offset as incomplete, so nothing selects it and nothing completes it - the commit high-water mark for that partition cannot advance past it until the partition is re-polled. The accounting half was already closed (#336, #373), so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left. It is the same fresh-replacement-at-one-offset class as the confluentinc#909 write-up, reached from the other side. THE FIX IS IN THE VALUE TYPE, NOT THE REMOVAL. Map.remove(key, value) is the JDK's compare-and-remove, and it decides "still mapped to the value I inspected" with equals - so what that call MEANS is a property of the value type and not of the map. WorkContainer.equals was topic, partition and offset only, so a stale container and its fresh replacement compared equal and no map API could express which of the two was meant. The equals/hashCode pair is deleted, equality is reference identity, and workMap.remove(offset, inspected) is a true compare-and-remove: it evicts the container the sweep inspected or nothing. Nothing evicted is a correct outcome, not a failure: the replacement won the offset, so the call retires nothing and REPORTS nothing. ShardManager.removeStaleContainers feeds the returned list to the retry queue, and #437 pinned that the queue removal is reached only through a real shard removal; reporting a container this call did not remove would take the retry entry at coordinates the fresh container now owns. getWorkIfAvailable's last-resort stale removal has the same shape and is written the same way, with a dated cleared-suspicion note saying why the race is not reachable there today and what would reopen it. REJECTED, and why each would have shipped looking correct: - computeIfPresent with an identity check in the remapping function. ConcurrentSkipListMap commits the removal through doRemove(key, v), which RE-READS the node value and gates on value.equals(reRead) before its compare-and-set, so with offset-only equality the identity test does not survive to the commit. Reproduced: a put landing inside the remapping function left the map EMPTY under offset-only equals and kept the replacement under identity equals. It narrowed the window to a few instructions inside the JDK and closed nothing. Under identity equality the same arm shows the gate declining directly: the remapping function runs TWICE, the first pass planting the replacement and asking for a removal that doRemove refuses, the second pass being handed the replacement. That is recorded at the site rather than asserted, since an implementation reaching the same gate without looping would be equally correct. - A ProcessingShard.Residency token as the map value, overriding neither equals nor hashCode. An earlier revision of this branch shipped it. It bought exactly one correct removal and left the value type wrong, so the next value-conditional site would have had to remember to wrap. Replaced before merge on the maintainer's call: 0.6.0.0 is the breaking release and already carries a Breaking section. - A remove-then-put-back repair, and a claim the sweep takes before removing which the writer must wait out. Both reintroduce a window, one in the map and one on the controller. THE OBJECTIONS THAT HAD QUEUED IDENTITY EQUALITY AS FUTURE WORK, each checked and each fallen: Comparable only RECOMMENDS consistency between compareTo and equals; SortedSet and SortedMap REQUIRE it, and nothing in main code puts a raw container in either - RetryQueue sorts by its own WorkContainerSortKey and de-duplicates by its own WorkContainerKey, so compareTo and its comparator are unchanged with the inconsistency stated in the javadoc. The one collection keyed on containers elsewhere, ExternalEngine.holdingDispatchPermit, has been an IdentityHashMap-backed set since #342, so identity equality makes the container agree with that code rather than changing it. The public break is RecordContext, stated above. THE OTHER HALF OF THE CONTRACT IS ENFORCED, NOT WRITTEN DOWN. Nothing puts a raw container in a sorted collection today, and both RetryQueue and ProcessingShard.workMap are one refactor away from being written the other way round with nothing going red. WorkContainerIsNeverInASortedCollectionArchTest reads every field, return type and parameter in main code whose declared type is a sorted set, sorted map or priority queue and whose FIRST type argument - the element-or-key position for all of them - is WorkContainer, so a container in a sorted map's value position passes. Proved red on main code with a temporary TreeSet<WorkContainer> field on ProcessingShard, which reported that field and not the workMap declared beside it. A standing positive control, in the RebalanceCallbackRuleControlTest shape, hands a hand-imported fixture to the real rule object so the rule cannot go quietly blind. Its limits are in the test javadoc: a body-local collection is invisible, the comparator is invisible so a legitimate Comparator-taking construction is still reported, and it covers core's classpath - measured, no main source outside core declares any of those types. EVIDENCE. ShardStaleSweepReplacementEvictionTest drives the seam deterministically rather than by racing: ShardSeamTestBase's spied PartitionStateManager lands the controller's replacement at an exact instruction inside the sweep. The defect arm was red against master. Restoring the old coordinate-based equals/hashCode and changing nothing else sends the defect arm and the premise arm red - "expected specific instance ... but was: null" and "expected not to be" the same coordinates - while the control arm stays green, as it must: a sweep with nothing racing it never has to say which of two containers it meant. Restoring removeWorkAtOffset(entry.getKey()) in the sweep sends exactly the defect arm red. The premise arm, twoContainersAtOneOffsetMustNotBeInterchangeable, asserts the equality contract directly and is the tripwire for a reintroduced coordinate-based equals. TREE-WIDE SWEEP for what identity equality breaks: one test assertion, ShardManagerTest's isNotEqualTo between containers at different offsets, became vacuous and is restated against RetryQueue.WorkContainerKey, which is what the queue keys on. PollContext.getByTopicPartitionMap's Set de-duplication becomes a no-op with no observable change, said in an @implNote at the site. Checked and found empty: no ArgumentCaptor or Mockito matcher over a container, no Truth containment assertion over containers except on the same instance, no container as a Map key or Set element in any test helper, and every Lincheck harness returns primitives from its operations in stress mode. SAME-DEFECT-CLASS SWEEP over core main, with a correction to this branch's own earlier result: an earlier commit reported holdingDispatchPermit as a real unfixed hit and opened an inflight note for it. That defect does not exist - the sweep matched the declared type Set<WorkContainer> and stopped, and a collection's semantics live in its initialiser, not its declaration. The note is deleted and the lesson is in the solutions write-up. Dismissed with reasons: ProcessingShard.onSuccess, removeWorkAtOffset from the revocation path, RetryQueue, the metrics and counter maps, OffsetSimultaneousEncoder.activeEncoders, MDC.remove. CI SIGHTING RECORDED, NOT DIAGNOSED. Chaos Pain Suite shard 4 went red once on ChaosChurnStormIT's ZOMBIE_MEMBER/REBALANCE_BLOCKED arm with maxInstanceStall=0ms; that line has an open ledger on master, and seed 1053013618367208111 is appended to docs/inflight/test-857-churn-storm-async-stalls.md because the seed dies with the CI log. Nothing on the rebalance path reads WorkContainer.equals, and the shard was green on every later push. DOCS. docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md is resolved and deleted, its mechanism and rejected alternatives migrated to docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md, cross-linked with the confluentinc#909 write-up. docs/refactoring.md's queued-breaking-change section now states that an entry is deleted in the PR that lands it, because the commit message carries the release-note content and the section lists only what is still queued; the entries marked DONE there are removed, and the two citations that pointed at this branch's own entry - WorkContainer's class javadoc and the solutions write-up - name this commit as the record instead. docs/inflight-tool.md used the resolved note as the worked example for the vet command; the example now names a surviving note with the same data-loss signal, bug-run-length-plausibility-ceiling.md. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
1 parent 6aab3ff commit 7c95b75

16 files changed

Lines changed: 1066 additions & 192 deletions

docs/inflight-tool.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -325,14 +325,14 @@ nearly every file. Each row carries the cheap signals a script can see:
325325

326326
```
327327
data-loss
328-
bug-stale-sweep-iterator-evicts-fresh-replacement.md
329-
"The poller's stale sweep removes by KEY, so it can evict a fresh replacement"
330-
first added 2026-08-27, bug
331-
all-cited-numbers-settled: cites one fork number and it is settled: astubbs#373 MERGED (pull-request)
332-
bin/inflight.mjs docs show docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md
328+
bug-run-length-plausibility-ceiling.md
329+
"A structurally valid but implausibly large run length is still accepted"
330+
first added 2026-09-03, bug
331+
all-cited-numbers-settled: cites one fork number and it is settled: astubbs#207 MERGED (pull-request)
332+
bin/inflight.mjs docs show docs/inflight/bug-run-length-plausibility-ceiling.md
333333
```
334334

335-
**A signal is a reason to open the note, never a verdict.** That note may cite astubbs#373 because
335+
**A signal is a reason to open the note, never a verdict.** That note may cite astubbs#207 because
336336
that pull request is where the defect was found, or because it fixed it - the row cannot tell, and
337337
does not pretend to. The four signals: every fork number the note cites is merged or closed; the
338338
number in its filename is settled; a cited path or symbol (in backticks) no longer resolves on the

docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md

Lines changed: 0 additions & 52 deletions
This file was deleted.

docs/inflight/test-857-churn-storm-async-stalls.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,3 +690,32 @@ Instance 5 on the first seed is the case that matters for the detector: a member
690690
frozen, with two workers parked between tasks - spare hands and nothing to hand them, because the
691691
records it holds are on the eight busy ones. A rule that accuses on "a free worker beside held work"
692692
accuses it; the rule that lands with the stacked follow-up accuses only nobody-in-user-code.
693+
694+
## Sighting, 2026-09-07 - the `ZOMBIE_MEMBER` arm again, 4% over its bound
695+
696+
<!-- post-merge: checked-begin -->
697+
`ChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedger` failed on the `Chaos Pain Suite 4/4` shard
698+
of astubbs/parallel-consumer#468, on the equality change, with the same arm as the fourth sighting
699+
above:
700+
<!-- post-merge: checked-end -->
701+
702+
[chaos-probe] VIOLATION: ZOMBIE_MEMBER/REBALANCE_BLOCKED: group 'group-1-1645296212' dwelling in
703+
PreparingRebalance for 15s (bound 15s) - a member is not answering the rebalance
704+
(protocol-unresponsive)
705+
[chaos-probe] peaks: maxRebalanceDwell=15632ms maxDrainDuration=11664ms
706+
maxLagStagnation=50717ms maxInstanceStall=0ms
707+
708+
**Replay seed `1053013618367208111`** (`CHAOS W1 churn storm: seed=1053013618367208111`).
709+
710+
**Recorded, not diagnosed, and the branch is a weak suspect on a stated mechanism rather than on
711+
"looks unrelated".** That PR's commit deletes `WorkContainer.equals`/`hashCode` so equality becomes
712+
identity. Nothing on the rebalance path consults either: the shard's conditional removal is
713+
`Map.remove(key, value)` whose semantics are unchanged from the `Residency` token the same branch
714+
carried through a green run of this suite one commit earlier, and the only other collection of
715+
containers in the engine (`ExternalEngine.holdingDispatchPermit`) was already identity-keyed. The
716+
one behaviour that does move is the per-scan `slowWork` `HashSet`'s de-duplication, which feeds a
717+
rate-limited warning and nothing else.
718+
719+
`maxInstanceStall=0ms` also separates this from the line diagnosed above: no member was stalled
720+
holding work. It is the rebalance dwell alone, over its bound by 632ms of 15000 - the tail shape the
721+
`ci-disabled-jobs-and-runner-load.md` confound predicts, not a wedge.

docs/refactoring.md

Lines changed: 3 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -98,42 +98,15 @@ These change the public, user-visible surface, so they still may not be folded i
9898
patch** - that is what release-gating means, and it is the only thing it means. Unlike the internal
9999
refactors below, which are non-breaking and can land at any point in any line.
100100

101-
- **DONE, landed in astubbs/parallel-consumer#267: `InternalRuntimeException` renamed to
102-
`PCInternalRuntimeException`.** A user-visible break - it is what arrives from
103-
`getFailureCause()`, and it is the type named in upstream's own report text
104-
(`...internal.InternalRuntimeException: Timeout waiting for commit response PT30S`,
105-
confluentinc#833). Renamed because the old name reads like a JDK type: in a stack trace or an IDE
106-
exception picker that prints simple names, `InternalRuntimeException` could belong to anything, and
107-
the `PC` prefix says whose it is at a glance. Recorded here rather than only in the commit, because
108-
this section is what the release notes are assembled from.
109-
[`docs/inflight/core-exception-hierarchy-cleanup.md`](inflight/core-exception-hierarchy-cleanup.md)
110-
owns the rest of the naming work - `InternalException` and the two spellings of the PC prefix are
111-
untouched, so a later pass will be a second break unless it is done in this same release.
112-
- **DONE, landing with astubbs/parallel-consumer#201: an inverted `initialLoadFactor` /
113-
`maximumLoadFactor` pair is rejected instead of accepted.**
114-
`ParallelConsumerOptions#validate()` now throws `IllegalArgumentException` naming both options and
115-
both values. A break only for a configuration that never did what it said - today an initial factor
116-
above the maximum is accepted and pinned at the initial value, surfacing at best as an inverted
117-
`100/10` in the rate-limited saturation warning, so an application carrying the typo starts and
118-
runs; after this it fails at construction. Small blast radius, but "started yesterday, will not
119-
start today" is what a `=== Breaking` bullet exists for. Recorded here rather than only in the
120-
commit, because this section is what the release notes are assembled from.
101+
**An entry is deleted in the PR that lands it**, because the commit message carries the release-note
102+
content and this section only lists what is still queued.
103+
121104
- **Remove the deprecated `commitInterval` options** - `public void setTimeBetweenCommits` /
122105
`public Duration getTimeBetweenCommits` in `internal/AbstractParallelEoSStreamProcessor.java`.
123106
- **Remove the accreting deprecated `ParallelConsumerOptions` fields**
124107
(`public void setCommitInterval`, `private final Duration defaultMessageRetryDelay`,
125108
`isUsingTransactionalProducer`) **and retire the temporary Kafka-compat work-around flag**
126109
(`ignoreReflectiveAccessExceptionsForAutoCommitDisabledCheck`) - `ParallelConsumerOptions.java`.
127-
- **DONE, landing with astubbs/parallel-consumer#116: the `Stream` returned by
128-
`pollProduceAndStream` / `vertxHttpReqInfoStream` now blocks until the processor closes.** It used
129-
to return almost immediately, because the queue-to-`Stream` bridge ended the stream on the first
130-
momentarily-empty poll - which is what `Spliterator.tryAdvance` returning `false` means, and it is
131-
the confluentinc#912 OOM: results produced afterwards piled up behind a consumer that had already
132-
walked away. A caller that collected on the calling thread and read a size got whatever had been
133-
produced so far; the same caller now waits for close. **No compatibility path is offered and none
134-
should be** - the old shape did not deliver the caller's results, so there is no correct behaviour
135-
to preserve. Callers consume on their own thread, as the Vert.x example now shows. Recorded here
136-
rather than only in the commit, because this section is what the release notes are assembled from.
137110
- ~~**Remove the JStream API** (deprecate first)~~ - **WITHDRAWN 2026-09-03, owner's call.** The
138111
removal was queued while the API was broken in the way above; deprecating something because it does
139112
not work is a different argument from deprecating something that does. It works now, so it stays,

0 commit comments

Comments
 (0)