Repository navigation
fix(core): derive the broker-poller load gate by conservation, and pair every count change with a real map mutation - #336
Merged
Conversation
…lete the clamp ProcessingShard.availableWorkContainerCnt was corrected after every decrement by a clamp to zero, commented "in case of possible race condition". That number was summed across shards and added to numberRecordsOutForProcessing to decide, in WorkManager.isSufficientlyLoaded(), whether the broker poller pauses or resumes - so a wrong value did not misreport, it mis-gated record intake. And the clamp only caught the low side. The comment was wrong about the mechanism. There was no race. There were two conditional decrements whose conditions did not match the condition the counter was incremented on, both reproducible on a single thread: - ProcessingShard.remove(long), reached from the revocation sweep, deducted only for a record that was isAvailableToTakeAsWork() - which additionally requires the retry delay to have passed. A record parked in retry back-off had already been counted in by markAvailableAgain(), so revoking it left the increment behind, permanently, high, in the direction nothing corrected. The retry-queue entry that normally nets that increment out is removed on the same path, so nothing was left to cancel it. revokingARecordParkedInRetryBackoffLeavesNoPhantomAwaitingSelection is that sequence; it fails on the old code with the counter reading 1 against an empty shard. - Both stale-container sweeps deducted unconditionally, so sweeping a record that was out at a worker - already deducted at selection - deducted it a second time. That is the low-side drift the clamp existed to hide. The gate only ever consumed the SUM of "awaiting selection" and "out for processing", never the split, and that sum is just "records inside the system". RecordPopulation derives it by conservation - admitted - retired, two monotonic LongAdders - where a record is admitted when its WorkContainer enters a shard's entry map and retired when it leaves by any path. The conservation walk test drives every departure path in one sequence and holds the O(1) figure against an O(n) scan of the shards. The high-side drift is rebalance-shaped - revocation of a parked retry is what leaves the phantom - so it is recorded as a CANDIDATE fourth mechanism in the paused-consumption family (#119, confluentinc#857), flagged in docs/inflight/bug-857-family.md as a hypothesis rather than a diagnosis: enough phantoms keep intake paused until a restart clears the in-memory counter. Adapted from fa4d1cf on perf/engine-concurrency: the verdict-free abandonment departure stays on that branch with its WorkManager side (the conservation walk notes where the branch extends it), and ProcessingShardStaleReplacement909Test - which postdates the original commit - is reconciled the way that branch's merge reconciled it, through the accounted addWorkContainer path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…cate it The extraction survey proposed cutting bce044b as its own PR; checking prior art found #201 fixing the same two defects independently. The load-gate fix is now #336. --no-verify: same pre-existing branch gate debt as the prior two commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
astubbs
marked this pull request as draft
August 24, 2026 05:35
3 of 4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…t pull Cutting it alone was attempted and abandoned on evidence: it compiles only on top of #335 + #336 + #358 merged, and its tests additionally need the abandonment path and getUpperBoundOnSelectableWork, which belong to direct pull. Isolating it would mean hand-rebuilding the accounting code that has already produced two bugs - so it becomes one review with direct pull instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
This was referenced Aug 25, 2026
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…this list said it was The ranked "where to point it next" led with `ProcessingShard.availableWorkContainerCnt` on the strength of the signature - an `AtomicLong` that must track a `ConcurrentSkipListMap` - and of the clamp in `dcrAvailableWorkContainerCntByDelta`, whose own comment reads `// in case of possible race condition`. The list concluded from that comment that a clamp to zero for a race is an admission of one. #336 measured it. It is not a race: both drift paths are conditional mismatches reproducible on a single thread, the clamp's comment is wrong, and that PR deletes the clamp and derives the gate by conservation instead. Lincheck decides interleavings; this defect needs none, and #336 already carries the single-threaded tests that catch it. Replaced as the top entry by the work claim in `getWorkIfAvailable`, which is what the signature was pointing at all along - a check-then-act over two fields that must move together, becoming one `AtomicReference<ExecutionState>` CAS in #335. That one is also time-sensitive: it is unreachable here only because the control loop is the sole selector, and #361 gives every worker its own. The wrong entry is recorded rather than quietly deleted, because the inference was made twice by reading a comment that claimed a race instead of measuring - which is the same error this lane's own calibration caught in itself when a point estimate from eight runs was mistaken for a rate.
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…unaided (#347) Adds Lincheck to `parallel-consumer-core` and calibrates it the only way that means anything: against a master that still contained every torn-read bug found by hand this month, asking whether Lincheck refinds them **unaided** - no seams named, no latches, no ordering imposed. It refound four, one of them off-list. The `WorkManager.handleFutureResult` double lookup, the `ShardManager` check-then-get NPE and the confluentinc#894 two-read all fell out in seconds. The encoder tear came back HALF-FOUND and is not claimed: the model checker placed a switch between the two reads once in eight attempts but never showed the range-top divergence. And `PCMetrics.registeredMeters` - a plain `ArrayList` written from two threads - was found **unprompted**, having been on nobody's list. That last one is the result that matters, because it is evidence the lane finds seams nobody named rather than only confirming seams somebody did. Green currently means "Lincheck found the bug": each harness is `assertThrows(LincheckAssertionError)` because the fixes have not merged. When #337/#344/#345/#346 land the lane goes red **by design**, and the remedy is flipping the affected harness to assert-no-failure - never reverting a fix, never loosening a bound. That inversion is the point of adopting it, and it is why the lane must stay non-gating. `LincheckToolchainProbeTest` is a deliberately broken red control and it earned its place immediately: the first run reported a clean pass against code that cannot survive two threads, because `wiremock-jre8` pins ASM 9.4 while Lincheck needs 9.6+, and its transformer failed per-class while reporting success. Every calibration verdict would have read "not found". A detector without a red control is a green lamp wired to nothing. **Two false-green shapes in the lane's own tooling were found and closed.** `bin/lincheck-test.sh` counted surefire reports from any previous run, so with stale reports present a nonexistent test class "passed"; it now deletes matching reports first. Then the guard that caught zero classes could not catch a *short* roster - three of five harnesses could silently stop being selected and the lane still printed a healthy number. It now pins the exact roster, and the number has to be hand-written: a derived count cannot work, because a dropped tag removes the class from both the run and the expectation, which cancel to green. **A bound is a claim about one machine.** The committed `iterations(1_000)` was originally justified by a point estimate from eight runs; forty more, fitted by maximum likelihood over 48, moved it by a factor of two to 2.33%/iteration (95% profile-likelihood interval 1.38-3.72%), and the independent-trials model is validated by prediction rather than assumed - 17.7 misses predicted of 32 at a starved `iterations(25)` against 18 observed. The finding that outlives the number: the hit rate is **machine-dependent**, 0.69%/iteration on a second machine against 2.33% here, with a likelihood-ratio test rejecting equality at p = 0.011. So no single-machine calibration of a stress arm transfers, and pricing that properly gates adding further harnesses. The bound for the slower machine is deliberately left unset rather than guessed - inflating it to cover the tail of an eight-sample estimate would be the same unfounded precision pointing the other way. Model checking is unavailable for now, established rather than assumed: it is blocked by a Lombok `@EqualsAndHashCode(callSuper = true)` interaction, settled by a two-arm control experiment that doubles as an upstream-fix tripwire, and by replay non-determinism on the commit path. Two tripwire tests keep its viability under watch instead of dropping it. Jabel's 17-source/8-bytecode split is a non-issue, also established. Corrections made rather than defended, both of the same shape - generalising from one measurement. A claim that no static analysis can see this defect class was measured with stock SpotBugs alone; fb-contrib's `MUI_CONTAINSKEY_BEFORE_GET` does reach `ShardManager.removeWorkFromShardFor`, so the claim is narrowed. And the ranked "where to point it next" led with `ProcessingShard`'s drifting counter on the strength of a clamp commented `// in case of possible race condition` - #336 measured it and there is no race, both paths being single-threaded conditional mismatches. Recorded rather than quietly dropped, because the wrong inference was made twice by reading a comment instead of measuring. The lane is excluded from every gating suite at all five points, verified rather than asserted: a gating run selects zero Lincheck classes. Where to point it next, led by the selection rule rather than a list, is in `docs/inflight/test-lincheck-lane-open-items.md`.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…at fixing it unmasked `ShardManagerLincheckTest` was the Lincheck lane's calibration arm for this branch's defect: it asserted that Lincheck FINDS a violation and that the report contains `NullPointerException`. This branch fixes that NPE, so the harness goes red - which the lane's own commit predicted, naming this PR, and prescribed flipping to assert-no-failure. THE PRESCRIBED FLIP DOES NOT PASS, measured rather than assumed. On the fixed tree Lincheck still reports `= Invalid execution results =`, with no `NullPointerException` anywhere in it. Controlled both ways in one sitting: revert only the main-code hunk and the NPE counterexample comes back and the harness is green (BUILD SUCCESS); restore it and the new counterexample appears (BUILD FAILURE on the `contains` assertion). So the second violation was always there, masked because Lincheck stops at the first thing it finds. Asserting no-failure would have been red on arrival; asserting nothing would have been the vacuous green this lane exists to prevent. The counterexample is `revokeSweep(0)` in the prefix, then `addWork(0)` against `addWork(0)` in parallel - the `entries.get` / `entries.put` + `incrementAndGet` check-then-act in `ProcessingShard.addWorkContainer`, i.e. the lane's own "concurrent collection plus a derived counter" signature. NOT claimed as a product defect: production registers work from the broker-poll thread alone, so two concurrent `addWork` calls are not an interleaving the library can take today, and the harness declares an operation set wider than the real thread model. That is a question about the harness's contract, and it is recorded in `docs/inflight/test-lincheck-lane-open-items.md` rather than answered here. So the harness asserts the strongest claim the evidence supports - that no report over these operations mentions `NullPointerException` again - which makes it a real regression detector for this fix instead of a calibration arm with nothing left to calibrate. The method is renamed to say so; nothing outside the file cited the old name. The note also narrows, rather than overturns, that file's existing ruling that `ProcessingShard.availableWorkContainerCnt` is not a Lincheck target. That ruling rests on #336 measuring the counter DRIFT as single-threaded conditional mismatches and it stands; what is new is a machine-produced interleaving over the same field, arriving unaided from a harness pointed somewhere else, which is a different claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
…fix, and the Lincheck lane this PR's classes now sit under Clean merge - no textual conflicts. `docs/inflight/bug-857-family.md` was edited on both sides and auto-merged: master's labels axis and CLASS2_STALL replay verdict, this branch's candidate-fourth-mechanism block, both intact. What arrived that bears on this PR's area: - `parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.md` (5fc21c6) makes the concurrency rules arrive with the file. Two bind here: annotate a race with `@GuardedBy` in the change that fixes it, and do NOT annotate ReadWriteLock-guarded state because the check is silently inert on it. `RecordPopulation` needs neither - it establishes its invariant by removing the sharing (two monotonic `LongAdder`s, no lock to name), which that doc explicitly allows. - `ShardMapIsNeverReplacedArchTest` (5fc21c6) pins `ShardManager.processingShards` against reassignment outside the constructor and against production callers of its package-private setter. This PR's `ShardManager` edits account through the existing map and add no assignment, so the rule holds unchanged - verified by running it. - `RetryQueue.removeAll` no longer reads `unique` off-lock (d2e00fa); the guard moved to the caller's own list. `ProcessingShard.getWorkIfAvailable` is that caller and is rewritten here, so the two changes meet in one method - the accounting added here does not reinstate the off-lock read. - `ShardManagerLincheckTest` and `WorkManagerLincheckTest` (ce4ee76) now exercise both classes under model checking. `docs/inflight/test-lincheck-lane-open-items.md` records the matching decision from master's side: `availableWorkContainerCnt` is NOT a Lincheck target, because this PR measured both drift paths as conditional mismatches reproducible on a single thread rather than races. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…od this branch does not have TWO REPAIRS TO `bug-available-work-counter-is-still-an-approximation.md`, neither touching a claim it makes. **The label.** It was written carrying `inflight-labels: needs-measurement`, before the labels axis existed. #324 landed that axis on master as a CLOSED set with one value, and `bin/check-inflight-tags.sh` fails the tree on anything else - so the note arrives red on the merge. Corrected to `concurrency` rather than widening the set. `docs/inflight/AGENTS.md` is explicit on both halves of that call: a label names a MECHANISM, never a state ("needs measurement" answers "where is this?", which is what the axis exists not to be), and a second value must come from reading the corpus and finding a group the existing ones cannot express - not from one note needing somewhere to sit. `concurrency` fits on its own terms: the note's subject is a counter and the collection it describes, mutated from the control and broker-poll threads with no single owner of the predicate - the parallel-state shape it names in its own body. **The citation.** It named `markAvailableAgain()` as what counts a failed record back in. That method exists on `perf/engine-concurrency` - `git log -S markAvailableAgain` finds it in 8124feb - and this work was adapted from that branch, carrying the name across. Here the method is `ProcessingShard.onFailure()`, which is what the note now says. An address repair, which `docs/citations.md` requires; the finding is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng callers, and correct two comments the fix falsified
Behaviour-preserving cleanup over this PR's own diff. Full unit suite green before and
after (`bin/ci-unit-test.sh`, 11 modules, BUILD SUCCESS), and `bin/check-all.sh` is
15/15.
**The two "visible for testing" scans in `ShardManager` were clones of production code
already in the file.** `countRecordsInShardsByScan()` is character-for-character the
`tracked` expression in the under-served-retrieval diagnostic, and
`sumOfShardAvailableCounters()` is both the `awaitingSelection` expression beside it and
the sum inside `getNumberOfWorkQueuedInShardsAwaitingSelection()`. Three copies of two
one-line streams, added by a PR whose whole subject is a number maintained in more than
one place. All three call sites now go through the helpers, which also stops them being
test-only fictions - their javadoc says who reads them instead of claiming they are not
for production. The retry-queue tuple is still read BEFORE the shard sum in
`getNumberOfWorkQueuedInShardsAwaitingSelection()`, which its comment says the result
depends on.
**`addWorkContainer`'s stale-replacement comment still described the clamp.** It said the
undercount "resyncs only when the shard drains far enough for the clamp in
`dcrAvailableWorkContainerCntByDelta()` to floor the counter at zero" - a mechanism this
PR deletes two methods below. It now says the truth (only shard removal resyncs it, and
nothing needs it non-negative any more) and states what the reader of a new invariant
will ask next: the replacement branch is population-neutral, one container leaving the
map as another takes its place.
**`retireAndDeductIfStillCounted`'s javadoc linked `#markAvailableAgain()`**, which does
not exist here - it is `perf/engine-concurrency`'s name for `onFailure()`, carried over
with the adaptation. A dangling `{@link}` compiles and then fails the javadoc build. The
same name reached the solution write-up this PR adds, repaired there to the same address
under `docs/citations.md` - the claim it makes is untouched.
**`dcrAvailableWorkContainerCntByDelta` kept `-1L * byNum`** from the deleted clamp's
arithmetic; `-byNum` is the same value.
**`WorkManagerTest.setupUnordered()` was a fourth copy, not a helper.** Three tests
already spelled the same builder out inline, so the new tests' helper was a parallel one
by the time it was written. It moves up beside `registerSomeWork()` and the three
existing sites use it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review Since the last review this branch merged The simplify pass folded three duplicated shard scans onto the two helpers this PR added ( Please focus on whether the conservation invariant still holds against everything master brought in - particularly the reworked Verified locally on the merged tree: |
![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: 9a38646daf
ℹ️ 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".
…ally did, not from a read that has already gone stale Review round on #336. The conservation figure only holds if every admission and every retirement pairs with a real insertion or removal of the one map that holds the records. Three places decided from something else, and each drifts the figure permanently - there is no clamp and nothing reconciles it against the shards. ProcessingShard.addWorkContainer decided "replacement, so do not admit" from the resident it read before the insertion. A stale sweep landing in between removes and retires that resident, so the container being inserted is the only thing at the offset - an insertion, not a replacement - and skipping its admission leaves the population one below the truth for good. It now admits first and lets the map's own return value say what happened, compensating with a retirement when the put really did displace something. Admitting before the insertion also preserves RecordPopulation's ordering invariant, which is what makes getInSystem() non-negative by construction. Both stale sweeps retired unconditionally after an iterator.remove() that discards the map's return value, so a concurrent revocation sweep that won the race had its single admission retired twice. Removal now goes through removeAndRetire(), which retires exactly what the map gave up. A value-conditional remove(offset, container) would not work here: WorkContainer.equals is topic/partition/offset only, so a fresh container that replaced the stale one compares equal to it. ShardManager handed a shard back from computeIfAbsent and then wrote to it as a separate step. Under KEY ordering removeShardIfEmpty can collect that shard in between, and the record is then admitted into a map no scan reaches. The record was already lost that way; what is new is that the admission is never retired, so the figure reads permanently high - the direction that pauses broker polling for good, where the old gate simply stopped summing a shard it no longer held. Choosing the shard and writing to it are now one compute(), against the same per-key lock removeShardIfEmpty's computeIfPresent takes. getWorkIfAvailable's last-resort stale eviction removed the container from the shard but left its retry-queue entry behind, which nothing else ever removes. The workable figure the load gate reads subtracts the parked-for-retry count from the population, so an orphan is subtracted from a total that no longer contains it and the poller is told it holds less than it does, until the retry deadline passes. It now cleans both structures, the way ShardManager.removeStaleContainers already did - closing the defect tracked in docs/inflight/bug-retry-queue-orphaned-by-inline-stale-removal.md, which this deletes. isSufficientlyLoaded read its operands once for the decision and then re-read all three for the diagnostic, so the logged equation could print numbers that never held together. That line exists to tell a real stall apart from counter drift, which it cannot do if its own arithmetic is inconsistent. It now logs the snapshot it decided on. Each fix has a regression test in ShardPopulationRaceTest that fails without it. The interleavings are driven deterministically rather than raced for, by making a collaborator act on the way past - PartitionStateManager.getPartitionState for the two inside ProcessingShard, ConsumerRecord.offset() for the one between choosing a shard and writing to it. Confirmed red on the unfixed tree: all four fail, each on its own assertion. Not fixed here, and recorded rather than patched: retireAndDeductIfStillCounted infers "was this record counted" from isNotInFlight() after the map has already given the container up, so a controller-side endFlight() for a revoked record can land in between and deduct a unit selection already spent. That window is real - correcting a claim in docs/inflight/bug-available-work-counter-is-still-an-approximation.md that said it was closed - but it belongs to availableWorkContainerCnt, which this PR removes from the load gate, and closing it means giving the container an ownership flag rather than patching one removal site. The note now carries the mechanism and the shape of the fix. Also drops a dead local in getWorkIfAvailable that nothing has read since it was written.
astubbs
changed the title
… after it merges check-branch-self-reference.sh failed on the review-round push. All three lines are present-tense claims about this open PR, which its own merge falsifies. The one added by that push cited the PR by number for who raised the finding; it now cites the work and says the load gate no longer reads the counter, which stays true. The two inherited with the master merge described the clamp as something "that PR deletes" and the tests as something it "already has"; both are now past tense, and the paragraph gains the pointer it was missing to the note that owns what is left of that counter. Each is marked `post-merge: checked` per the gate's own contract. Worth recording because a local `bin/check-all.sh` cannot catch this: the gate resolves the PR number from PR_NUMBER or GITHUB_HEAD_REF, and in a worktree whose branch is named for the merge rather than the PR it finds neither - so it reports "NO PR NUMBER RESOLVED" and skips every #NNN check while still printing ok. Reproduce CI's answer with `PR_NUMBER=<n> GITHUB_HEAD_REF=<head-branch> bin/check-branch-self-reference.sh`.
5 tasks done
astubbs
commented
Aug 26, 2026
…at owns both operands, and say what the pair is not Review on #336 asked two questions that turn out to be one. "Aren't you still reading them twice, just closer together?" - yes. Reading each operand once into a local means the decision and the log line can no longer disagree, which is what the comment claimed, but `inShards` and `parkedForRetry` are still two reads of two structures at two instants, so `inShards - parkedForRetry` can be a number that was never simultaneously true. The javadoc answered a different question than the one asked. "Why doesn't ShardManager have a method that does this for you?" - it does now. It owns both operands, so it owns the difference; WorkManager reaching across for the pair to do the arithmetic itself spread one figure's definition across two classes. `getWorkableRecords()` returns the difference and both operands, so `isSufficientlyLoaded()` still decides and logs from one read. It is NOT an atomic snapshot and cannot become one here. The population is two LongAdders in RecordPopulation; the parked figure comes from RetryQueue under that queue's own fair read/write lock. Nothing spans both, and a lock that did would put the broker-poll thread's admission path behind the retry queue's fair lock - a redesign this figure does not need. The javadoc now states the residual instead of implying it closed: - Retry-queue movement in the window costs nothing. Parking a record and its delay expiring both leave the population untouched, so the difference stays exactly right as of the later read - which is why the population is read first. - Population movement in the window is the whole of the skew, bounded by what another thread admits or retires while the retry-queue read runs (a fair read-lock acquisition plus an O(n)-worst-case head scan). A missed admission reads low and fetches sooner than needed; a missed retirement reads high, and high is what pauses the poller - the confluentinc#857 silent-stall direction. - It cannot accumulate. The gate is resampled every control-loop tick and both figures are exact once mutation stops, so a skewed sample moves one fetch by one tick. That is the difference from the defect this figure replaced, where separately maintained counters drifted permanently with nothing to reconcile them. Also from the same review: `resident` is now `residentBeforePut`, which is the name saying why the big comment beside it insists the read is only advice and the map's own return value is the authority; `getWorkContainerAt` is now `getWorkContainerAtOffset`. The noun "resident" is kept - it is already the house spelling in PartitionState, the inflight notes and RegistrationRaceStaleResidentIT. While here: the truth-probe note still said the shard-depth aggregate feeds `isSufficientlyLoaded()`. It has not since this work, and the note now names the consumers it does still have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XC9N7S8gSd6fCYNNe64D6j
…re, and two citations this branch's own deletion orphaned One content conflict, in ShardManager.initMetrics(). Master's confluentinc#905 cherry-pick routed SHARDS_SIZE through a new shardEntryCounts() scan so the new SHARDS_MAX_SIZE gauge could share it; this branch had already moved SHARDS_SIZE to the O(1) conservation figure. Resolved on the merits: SHARDS_SIZE keeps the conservation figure - it is O(1) rather than O(total queued records) and it cannot disagree with the shards the way a sum of drifting per-shard counters can - and SHARDS_MAX_SIZE keeps its scan, because a maximum cannot be conserved the way a total can. shardEntryCounts() survives with one caller. Three claims the resolution falsified, corrected where they live: - shardEntryCounts()'s javadoc said it kept "the two gauges below" from drifting apart. - Its TODO(refactor) said the scrape walks the shard queues "twice"; it is now once, and docs/refactoring.md, which owns that assessment, said the same thing. - PCMetricsTest said SHARDS_MAX_SIZE "reads the same single counter snapshot SHARDS_SIZE does". The two are now derived differently and agree only because nothing completes while those assertions run - which is what the test actually relies on. Master also brought in two citations of a note this branch deletes: WorkManager#onFailureResult and WorkManagerStaleCheckDoubleLookupTest both point at docs/inflight/bug-retry-queue-orphaned-by-inline-stale-removal.md for the count mechanism. Neither is stale in substance - they name the same defect family through a different door, and both were written against a tree where the note still existed. The mechanism is now stated inline (the parked figure is subtracted from a population that no longer contains the orphan), with the full trace as the history pointer docs/citations.md prescribes. The sha is a80f2bb, which is on master, rather than the deleting commit on this branch, which a squash-merge would not preserve. Nothing else in the 30 inherited commits touched the load gate. #342's dispatch ceiling changes what ExternalEngine requests, not how the gate answers; confluentinc#909's work-claim and revoke-sweep fixes merged into ProcessingShard cleanly around the population calls. 471 core unit tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XC9N7S8gSd6fCYNNe64D6j
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…et, instead of inferring it (#373) ## Description `ProcessingShard` decided whether it had already spent a container's unit of the shard count by reading that container's observable state after the fact - `isNotInFlight()` on an entry already removed from the map. No state at time T records *who* spent the unit, so the read cannot answer the question however well synchronised it is. A container now carries an `AtomicBoolean` claim, the shard claims and releases it by compare-and-set, and **only the CAS winner moves the counter**. The floor-at-zero clamp is deleted: the count cannot go negative by construction, so a clamp would only hide drift. **Four instances of the class, not the one that was reported.** The class is *a counter whose spend is inferred from observable state rather than owned by an atomic outcome*, and all four were in this one file: the reported double-deduction on revocation; `addWorkContainer`'s stale-replacement branch; `onFailure` not idempotent despite its contract; and a **sign-reversed** case where revoking a failed record inside its retry delay deducted nothing, leaving a permanent OVERcount - which is the confluentinc#857 stall signature. **Keeping the counter was a decision, not an assumption.** Of its four readers, three could derive their answer by scanning: `drain()` needs only a boolean, the `WAITING_RECORDS` gauge is per-scrape, the under-served diagnostic is debug-gated. The load gate cannot - measured at ~4.5 ns/entry, a scan costs 45 µs at 10k buffered records and 236 µs at 50k, which at control-loop cadence is 4-24% of a core. **Deleting the counter becomes viable once #336 lands**, since that removes the only reader needing O(1); nothing here blocks it. Note the counter is not merely a gauge: `drain()` reads it, so reading low closes the consumer early. Every guard is proven red-then-green by reverting the main code, and mutation-checked three ways. ## Known cost, accepted deliberately The claim and the count are two atomics, so a reader can transiently observe the count at **-1**. It settles correct and every consumer floors, except the debug stall diagnostic - clamping the one line that exists to expose drift would be incoherent, so the javadoc names it as the exception rather than the code hiding it. Collapsing both into #335's `Execution` transition removes the transient rather than documenting it, and is unblocked now #335 has landed. **This PR makes one thing worse, and it is recorded.** `ProcessingShard`'s stale sweep calls `iterator.remove()`, which on a `ConcurrentSkipListMap` removes by *key*, so it can evict a fresh replacement written by the controller between `next()` and `remove()`. The lost record predates this change; the counter drift's direction does not. It was one-low-and-clamped, and is now one-high-and-permanent - the direction that keeps `workIsWaitingToBeProcessed()` true and stops `drain()` reaching `transitionToClosing()`. There is no identity-keyed removal to reach for, since `WorkContainer.equals` is offset-only, so closing it means deciding how the sweep and the replacement branch coordinate at all. Open, with the deterministic test it needs, in `docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md`. ## Naming The operations are spelled from the vocabulary already in the file: `includeInSelection` / `excludeFromSelection` on the shard, `claimSelection` / `releaseSelection` on the container, and the field is `workAwaitingSelectionCount`. "count"/"uncount" used *count* in the regard-as sense beside a field using it in the enumerate sense, and the field's old name borrowed "available" from `isAvailableToTakeAsWork()`, which means something different - a caveat paragraph existed purely to explain that collision, and has been cut down to the fact it was hiding. ## Also here A capture of the revoke-path commit deadlock in `docs/inflight/bug-857-family.md`: this branch went green on the arm it had been red on, three heads later. Recorded because the sighting is otherwise lost when the branch goes green.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…he reports and the ledger, and answer no The question this PR exists to answer had not been asked in one place: given everything that has landed, does the paused-consumption issue close? Recorded in the PR's own note, where whoever lands it will look. **#119 is a symptom bucket, not a defect.** Read from upstream confluentinc#857's comments rather than the mirror's summary - which does not carry them - there are at least four distinguishable behaviours, and upstream's own maintainer separated two of them in one sentence: the April log is a paused subscription, the July log from the same reporter is *"not paused, but somehow polling 0 records"*. Also on file: an out-of-range fetch position with records stuck permanently through redeploys, and lag that will not clear although the records were processed. **A fifth is on this PR and was never chased.** dmironowicz, 2026-07-24, has processing pause for a few partitions and resume after hours, and answered the direct question with *"I don't see any rebalancing activity around that time"*. The AB-BA cycle needs `onPartitionsRevoked` to be running, so it cannot produce that. **The best candidate for it is the load gate, and it is already fixed - by work that is not in this PR.** Neither the pause mirror nor the paused-partition cache fits: the cache refreshes on every poll and a paused consumer still polls. What does fit is `maybeWakeupPoller`'s `!wm.isSufficientlyLoaded()` term - if the load side is stuck high, a paused poller is never woken, with no rebalance and no error. #336 says exactly that of the defect it fixes: the drift *"mis-gated record intake, and enough phantoms keep intake paused until a restart clears the counter"*. **Reading the ledger end to end, the family is far smaller than its length.** About half is withdrawn: the entire `CLASS2_STALL` line went on 2026-08-25 when both nominated seeds crossed the bound and then drained, and with it the ~154s-constant corroboration; the seventh sighting was a test defect; the fourth was a harness double-start; the `ChaosKeyOrderIT` zombie was a calibration gap. **What is new, and it favours this PR.** Six thread-dump captures on 2026-08-26/27 show the poll thread BLOCKED on the `commitCommand` monitor held by the control thread, frames resolving to `commitOffsetsThatAreReady` / `onPartitionsRevoked` - this PR's AB-BA pair, observed rather than inferred, across both assignor arms and five branches. The deadlock is real and is being hit. **And the verification this PR owes has not been done.** Not one of those six seeds has been replayed with the `tryLock()` fix applied. That is the cheapest high-value experiment available here and it beats more chaos runs: it turns "the soak says the symptom stops" into "the mechanism we photographed is gone". The seeds are listed. The post-landing prediction already on record is restated so it is not dropped: the Class 2 findings should CONTINUE at the same rate afterwards, because they are the bound meeting the load. If they drop off instead, the 2026-08-25 reading is wrong. Verdict recorded: land it, verify it against a captured seed, and do not close #119 on it - four mechanisms remain, one unexplained by every known member of the family. The issue's `## Fork status` also needs correcting first: it still cites `RebalanceEoSDeadlockTest` as live evidence the deadlock is present, and this branch established that test is inverted. Still to fold in: the merged-PR mapping and this PR's 32 timeline cross-references, which include open mirrors describing adjacent symptoms (#183, #173, #177, #44) that the family ledger does not account for.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…than the ledger counts, and a testable lead on the last unexplained line Reading the merged commit bodies rather than the family's own summary, #119's `## Fork status` is badly out of date. It says three defects landed and one remains. At least six more have landed since, none of them in the family's accounting: - **#346** orphaned a retry-queue entry when a rebalance landed between a staleness checkpoint and its acting read, after which `workIsWaitingToBeProcessed()` read TRUE FOREVER. Its own body calls that a confluentinc#857-family permanent stall. Red-then-green on both arms, mutation-tested. - **#345** NPEd the broker-poll thread inside the rebalance listener via a `containsKey`-then-`get` race. Poller death under KEY ordering is paused consumption by another name. - **#373** found a sign-reversed overcount its body names the "confluentinc#857 stall signature". - **#336** rewrote the load gate whose drift kept intake paused. - **#344** let the offset encoder mark still-incomplete offsets complete. - **#349** fenced a cross-thread `dirty` flag, jcstress-measured. The bucket has been drained steadily from several directions and nobody re-totalled. That is the substantive reason #119 cannot be closed by reading this PR alone, and why its fork-status section needs rewriting rather than appending to. **A prediction is recorded for the one line still unexplained.** The `ChaosChurnStormIT` `NO_PROGRESS` sightings are all `PERIODIC_CONSUMER_ASYNCHRONOUS`, the mode where neither the AB-BA cycle nor the transactional wait can occur - which is why they were set aside as a possible fourth defect. #344's defect is live in that mode and only that mode, and its consequence - offsets marked complete while incomplete - has the `NO_PROGRESS` shape. So: replay the three async seeds either side of #344; if the mode match is the mechanism the pre arm reproduces and the post arm does not. Stated as a prediction rather than a finding, with its own counter-example beside it: #373 was already tested against this arm on a mode-matching hypothesis and fired anyway on that branch's own head. One plausible fix has failed here before.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…d gone out of date in eight ways The ledger is where agents log a sighting and move on, and where the next one reads before forming a hypothesis - so conclusions reached elsewhere are worth nothing until they land here. A week of findings had accumulated in a PR note that gets DELETED when that PR merges. Migrated. What the ledger now says that it did not: - Its own "three landed, one open" summary is stale. At least six more family defects have landed - #346, #345, #373, #336, #344, #349 - and none was counted. The bucket has been drained from several directions and nobody re-totalled. - The original deadlock is VERIFIED rather than diagnosed: an A/B on the deterministic probe, one term changed, control failing every repetition and the fix failing none while logging the contended decline throughout. - Replaying captured seeds does not reproduce that deadlock and should not be attempted again - the window opened zero times locally on either assignor arm, and two write-ups were retracted the same day for reading a green replay as evidence. - The async line has its first replayable seed, found by random hunting rather than replay, and #344 is refuted as its explanation. - A third of those failures went red with the NO_PROGRESS detector silent, which matters because the Class 2 demotion left the liveness claim resting on it. - The symptom is a bucket of at least four distinguishable behaviours, separated by upstream's own maintainer in one sentence, plus a fifth reported on #29 and never chased - a pause with no rebalance in the logs at all. - Upstream's own fix for this issue shipped, is carried here, and did not hold. - confluentinc#875 is filed in this family on a third party's assumption and looks like an offset-completion defect instead. Also records why a single beefier Docker host beats a distributed rig for the soak harness beyond sequencing: central log shipping comes free, and any node can be inspected AT RUNTIME while still wedged. Every sighting in this family that became a mechanism did so because something captured the process while it was stuck; every one that stayed a signature did not.
3 of 5 tasks
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…Observe/Explain/Act shape Fourth bit of the follow-up Codex strategy conversation (2026-08-29/30), carrying the insight the conversation itself flags as worth protecting. core-execution-opportunity-model.md: nearly every feature the two reviews produced is one internal model asked a different question - adaptive concurrency, autoscaling, attribution, the partition advisor, rate-limit governance, SLO control and the GUI are all projections of "which available work is not executing, and at which gate did it stop". Filed as a constraint on future work, not a feature: build the gate ladder once, preserving the stop reason at each gate, or every feature grows its own Map of the same information - the repo's own recorded bug-recurrence pattern. The ground is ready because the conservation accounting (#336) already enforces the property the model needs most: every owned record is somewhere and the numbers reconcile. web-control-plane.md: the dashboard's third layer - Observe / Explain / Act, every action beside the evidence that justifies it, and every manual intervention expiring (bound + duration + reason, control returns to the adaptive system) so an emergency override cannot silently become permanent configuration. Four instruments named as buildable early because the engine already knows the answers: the concurrency-gap explainer, the hot-key detector, retry impact (what a retry blocks, not how many there were), and the reconciling execution-state breakdown. Two consequences kept visible: Act destroys #268's stated read-only-no-auth security argument, so authn/authz is a prerequisite of that layer, not hardening; and each button is an engine API before it is a button (DLQ, per-key pause, self-reverting options). Amendments: distributed-throttling gains per-service contracts shared across functions (the min-composition decision restated with the service as scope); the content series gains the category framing (the real competitor is manual concurrency management) and a note that the follow-up transcript used the floated rename as though decided - an assumption, not a decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PFA3CXkGc2m32cBJMAWTK
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…ut consuming a retry Core has two ways for work to come back and neither fits a worker that vanished. onSuccessResult commits the offset. onFailureResult records a failure, queues a retry and bills the record an attempt. But an external engine whose worker process dies mid-record has no verdict to report: nothing failed, so charging a retry attempt is wrong, and a record that reliably outlives its worker would burn through its budget without ever being processed. Today handleFutureResult throws IllegalStateException on a verdict-free return, so the engine cannot hand the record back at all. This adds the third path. An abandoned marker on WorkContainer, distinct from the user function verdict, selects a branch that does what the stale-work branch does - endFlight and exactly one decrement - and then restores shard availability without a retry-queue insert, so the record is immediately selectable rather than waiting out a delay it never earned. An empty verdict with the marker unset still throws; that is the bug it always was. Two things worth pointing at. The in-flight counter is the danger. It gates the broker poller through isSufficientlyLoaded, and drift in it stalls the consumer silently while it still looks alive - this repository has that failure documented already. So onAbandonedResult ignores a repeat return rather than decrementing twice, which is a real case: a disconnect detected while a report is already in flight returns the same record twice. And the tests assert the exact counter value, not merely that work keeps flowing. The subtler one is that redelivery has to clear the previous delivery's state, because a redelivery is a fresh attempt. That was originally two clears; core has since made one of them structural, which is the first of the changes below. WHAT THE REBASE ONTO CURRENT MASTER CHANGED This is #295 resurrected. It was closed as folded into the language-proxy PR #293 while that PR was the only home for it, and is being landed on its own again because a core scheduling primitive should not first arrive inside a remote-runtime PR. The commit is the same change; three things had to move to fit master, and all three are master's design winning rather than this one being watered down. The verdict no longer needs clearing on redelivery. #335 collapsed `inFlight` plus `maybeUserFunctionSucceeded` into one atomic ExecutionState, so the verdict is now derived from the state and a won claim lands on IN_FLIGHT, which has none. The original explicitly reset `maybeUserFunctionSucceeded` in onQueueingForExecution; that field is gone and the reset with it. Only the abandon marker is cleared, in actOnClaim - after the compare-and-set, so it is the claim WINNER that clears it. A new test, aFreshClaimClearsThePreviousDeliverysAbandonMarker, covers what that clear now protects on its own: without it, a record that had ever been abandoned would have every later verdict-free return silently forgiven instead of reported. The marker stays its own field rather than becoming a seventh ExecutionState, and it is an AtomicBoolean rather than the plain boolean it was. The argument for putting it in the state machine is real - "one field, because two were the bug" - but the hazard that collapsing fixed was a claim DECISION reading one field and being contradicted by the other, and nothing reads this one to decide a claim. It is a note left by the returner for handleFutureResult, which is the same reason selectionClaimed is its own field. The field javadoc carries that argument, so the next reader finds it rather than taking the field for an oversight. Atomic because the marker is written by whichever thread noticed the worker was gone and read by the controller. The shard-level restore goes through the selection claim rather than a raw counter increment. #336 replaced availableWorkContainerCnt with a compare-and-set-owned claim, so ProcessingShard.onAbandoned calls includeInSelection, and idempotence now comes from the compare-and-set instead of being argued. Master's conservation walk reserved this in its own javadoc - "the branch developing external engines adds a further departure". On contact it turns out not to be a departure at all: abandonment retires nothing, because the record stays resident in its shard. So the reservation is answered by a test that asserts the conservation figure is unmoved in both directions, and the javadoc corrected to say so. Teeth checked twice, each with a control arm. Removing the handleFutureResult branch fails all seven behavioural tests and leaves workWithNeitherVerdictNorAbandonMarkerStillThrows correctly green, since that one guards the pre-existing behaviour. Removing the marker clear in actOnClaim fails exactly aFreshClaimClearsThePreviousDeliverysAbandonMarker and nothing else. Full reactor unit build green. (cherry picked from commit 4b4ff19) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…an all be true at once Every probe in this repo watches liveness: is the system still progressing, sampled as one number over time. A system can be perfectly live while lying about what it holds, and it will report healthy right up until the lie matters. This watches something else - whether PC's separate accounts of its own state are mutually consistent. The state it flags: work queued for execution while PC believes no offsets are incomplete and nothing is out for processing. That is reachable without anything throwing, because getNumberOfIncompleteOffsets() sums over ASSIGNED partitions and returns zero whenever that map is empty, however much work is queued. It was observed on three consecutive log lines during the transactional throughput investigation - an executor queue of 319 against a target of 320, with both counters reading zero. describeProgress() also now reports the executor queue depth, which is the only one of these numbers sourced from OUTSIDE PC's own bookkeeping. The others are things PC believes; that one is a thing that is true, and the asymmetry is what tells a reader which number to doubt. IT REPORTS, IT DOES NOT ASSERT - and it is UNARMED The two reads are not atomic, so a queue draining between them is a false positive, and one sample is not evidence. Turning it into a gate needs the contradiction to PERSIST across samples and needs it shown firing on a tree that should fail. Neither has happened. The run that would have exercised it PASSED - this branch's failure is load-dependent - so the check has never fired. Its silence currently means nothing, which is the same standard applied to the ambient probe that this whole thread came from, and it is recorded that way rather than presented as working. WHAT THE DIRECT-PULL STACK DOES TO IT #361 hands work SELECTION to the workers - they block on the shards and take their own records instead of the control loop pushing into an executor queue - so executorQueueDepth() would read zero permanently and this condition would become unreachable. The check would not fail; it would go QUIET, which is worse, and is the exact failure class this session has been documenting. A version meant to survive that stack must source "work PC is holding" from something direct pull still has. The finding may also not be novel. #336 derives the poller load gate by conservation rather than from maintained totals, and numberRecordsOutForProcessing is described in its own note as the last counter of its family after the others were dissolved into derived views; #335 adds ExecutionState for concurrent claims. The disagreement sits inside the area that stack is rebuilding and must be checked against it before being called a new defect. The throughput regression is measured separately and stands either way.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…s engine back Merges `feats/proxy-dispatch-clients` (#390, the top of the Wagon A stack) into this branch, so that #293 can be retargeted onto that rung and its displayed diff collapse to the engine residue. This is the plan's "retarget move" - `docs/plans/2026-08-31-001-process-god-branch-decomposition-plan.md` - and it is an ordinary merge: no history was rewritten and nothing was force-pushed. The pre-merge tip is preserved as `origin/backup/pre-stack-merge-293`. The merge also brings `master` forward, because the stack is current with it and this branch was not. That is most of the volume and most of the conflicts. ## Which side won, and why The rule was: the god branch's SEMANTIC content, expressed on the stack's refined STRUCTURE. - **Engine-only files stayed** - `ProxyProcessor`, `ConfigureHandler`, the dispatch waves, epochs, leases, reconnect and the produce path. They are the residue this campaign exists to expose. - **Stack-only files were taken** - the ten foreign clients, the runner registry, the sidecar shim, the self-retiring guards, `Main` and `ParentDeathWatchdog`. - **Records lost, everywhere.** The stack converted five value types from Java records to plain final classes because Jabel rewrites a record into a class with no source positions and Error Prone 2.42.0 crashes rather than reporting on it. This branch never met that only because its root pom had no Error Prone; master's does now, so the conversion is load-bearing here. - **Dependency pins are the stack's**: protobuf 3.25.8 (`requireUpperBoundDeps` refuses 3.25.5 against grpc-protobuf's transitive ask), gson 2.14.0, `error_prone_annotations` 2.48.0 in the proxy and the Java client aggregator and 2.47.0 in the protocol module - deliberately unequal, because their highest requesters differ. - **`clients.yml` is this branch's**, not the stack's. The stack's copy is a smaller earlier file whose own header says it has no conformance lane, no per-language static analysis and no dependency audit; only its `--component clippy` fix and its workflow-level `PC_FOREIGN_CLIENTS_STRICT` were folded in. - **Master's copy won for everything master owns** - the gate scripts, the copyright checker and its self-test, `repo-hygiene.yml`, `docs/ci.md`, `docs/copyright.md`. This branch's only delta in most of those was a copyright header in a pre-canonical form, written before #338 landed the canonical one on master. - **The two ledgers were merged as unions**, not resolved to a side. `docs/inflight/bug-857-family.md` keeps both sides' sightings, this branch's renumbered per the file's own stated convention; `docs/quarantined-tests.md` drops three entries whose owning PRs retired the annotation on master, because a registry entry without a `@Quarantined` test is a hard gate failure. ## The verdict-free work return, re-expressed on master's shape Unit U4 is still on this branch as `WorkManager.onAbandonedResult` and its call sites, and `WorkManager.java` auto-merged keeping them - so resolving `WorkContainer`, `ProcessingShard` and `ShardManager` to master alone would have compiled to nothing. Master moved underneath it: #335 collapsed `inFlight` and `maybeUserFunctionSucceeded` into one atomic `ExecutionState`, and #336 and #373 replaced the shard's raw `availableWorkContainerCnt` with a compare-and-set claim. `ProcessingShard.onAbandoned` and `ShardManager.onAbandoned` are therefore taken verbatim from `feats/proxy-verdict-free-return` (#295), which is cut on current master and is the authoritative re-expression - it routes the restore through `includeInSelection` rather than incrementing a counter that no longer exists. `WorkContainer` is NOT #295's, and the difference is deliberate: that branch marks abandonment with an `AtomicBoolean` cleared by the claim winner, while this branch keys it by delivery (`markAbandoned(long)`, `isAbandonedForCurrentDelivery`, `isReturnForSupersededDelivery`). The delivery-keyed form is strictly more: it tells a late return for delivery n from a live return for delivery n+1, which the boolean cannot, and acting on that confusion ends a running flight and decrements `numberRecordsOutForProcessing` twice. It also needs no clearing on redelivery, so it does not race the claim. Master's own `deliveryCount` - already an `AtomicLong` incremented only by a won claim - supplies the identity, so the field is the one thing added. ## The reconciliation duties #387 and #390 recorded Both are discharged, and the guards they left behind now pass because the gap closed rather than because anything was edited. **`ProxyHarness` and `ConformanceHarness` are one class again.** #387 cut the latter out of the former with the engine lane removed, and wrote into the class header, the module pom and a deferred inflight note that whoever landed the engine must merge them - leaving open which module it ends up in, on the grounds that "the engine lane decides". It decides the sidecar module: the lane needs `ConfigureHandler` and `ProxyProcessor`, and `TestModeMain` - the process a spawned foreign runner actually starts - is in that same test tree and would otherwise have to import the conformance module and close a cycle. So the NAME is the extracted rung's and the MODULE is this branch's, and `parallel-consumer-proxy-conformance` reaches it through the proxy test-jar its pom already declared. The stack's refinements to the class survive: `Delivery` stays a plain final class, and the `startEmbeddedClient` lane stays beside the restored `startEngine`. **`ConformanceDriver.spawnAgainst` is back**, with `LanguageRunner` a `ConformanceBinding` again - it stopped being one on #390 for exactly as long as there was no engine to spawn against - and the ten foreign bindings are registered in `ConformanceBindings.selectable()`. `ConformanceBindings.deferredUntilTheEngineArrives()` is kept and now returns an empty list, which is the honest reading: nothing is deferred. It is not deleted because `TheEngineArrivingMustBringTheForeignCellsTest` reads it, and because the next capability to arrive without a cell needs that list rather than a new one. ## Two duplications the merge surfaced, resolved rather than left - **The protocol documents existed twice.** #383 moved `protocol-specification.md` and `client-authoring-guide.md` into `parallel-consumer-proxy-protocol/docs/` and predicted the rename conflict here. The protocol module wins on the merits - all three artifacts are the contract rather than the server - but the CONTENT is this branch's, because the stack's copies carry "no client generates from this schema yet" and a `git show` fallback for a Go generator script that exists here. `SpecificationCoverageTest` moved with them; every citation was re-pointed. - **`HarnessScenario` existed twice**, once per harness. The stack's plain-class version won, in the sidecar module beside the class it serves. ## Five Java records had to become plain final classes, or nothing compiled `master` gained Error Prone after this branch forked, and Error Prone 2.42.0 does not report on a Jabel-desugared record - it crashes, failing the whole compilation with a message attributed to line 1 of an unrelated file. #387 hit this one rung down and converted its five value types; the same thing was waiting here for `ProxyProcessor.ManifestOutcome`, `ManifestReconciler.Reconciliation`, `InFlightRegistry.InFlight`, `LivenessSettings` and `TestModeMainTest.Run`. Neither term can move - the root pom pins Error Prone at 2.42.0 because 2.43.0 needs a JVM this build cannot use, and Jabel serves the release 8 target - so the records lost, which is what the rest of the repository already does. `grep -rn "record "` now finds none. ## Two defects this merge introduced and then fixed - **The default lane ran a test that needs a profile.** Keeping BOTH sidecar-spawning tests - this branch's engine-backed `OneRecordThroughTheSidecarTest` and the stack's engine-less `SidecarHandshakeTest` - left the Kotlin and Scala exclusion property naming only the first, so the second ran in an ordinary build with no `sidecar-classpath.txt` and failed. Both are now behind one regex, which is also the arrangement that makes the pairing legible. - **Every proxy module's data record existed twice.** This branch keeps module-maturity and testing-evidence records in per-module `docs/data/*.d/` fragments; the stack, which never imported that mechanism, wrote them into the monolithic files. The merge kept both and `bin/check-docs-data.sh` went from green to 37 structural problems. The fragments win, and deleting the monolithic copies also deleted eleven rows asserting "NO RECORD HAS CROSSED THE WIRE" - true on a stack whose sidecar hosts no engine, false here. ## How it was verified JDK 17, macOS. Full default reactor `./mvnw --fail-at-end test`: **BUILD SUCCESS**, every module green including the conformance suite - so the `java-grpc` cell runs against a live engine for the first time, and `GrpcSpikeConformanceTest` answers all five scenarios over a real gRPC stream. The four self-retiring guards #387 and #390 left behind all pass, and **none of them was edited**: `TheEngineArrivingMustBringTheGrpcBindingTest`, `TheEngineArrivingMustBringTheGrpcCellTest`, `TheEngineArrivingMustBringTheForeignCellsTest` (which asserts about every registered language, not one) and `SelectorMatchingNothingFailsTest`. `bin/check-all.sh` real exit code 1, with three gates failing and all three inherited, established against the pre-merge tip rather than assumed: `check-inflight-tags` reports the same 53 problems before and after; `check-file-refs` was already red for the `@`-prefixed bridge imports that #378 fixes; and `check-branch-self-reference` did not exist on this branch before the merge - its findings are on notes inherited from `master` byte-identical. `check-proto-lint` and `check-proto-breaking` report CANNOT RUN for want of `buf`. **Not run locally: `-Dpc.foreignClients`.** It builds C++ and Swift inside containers, and this box is under a low-disk warning that names a container build as what would tip it over. CI's per-language rows are where that lane runs. ## Two findings recorded rather than fixed - `ParallelEoSStreamProcessorTest.processInKeyOrder` failed its own preamble sanity check once here - and the control arm reddened `master` HARDER: unmodified `master`, uncontended, failed all three parameterisations where this branch under concurrent load failed one. That refutes the existing ledger's "the base branch was green" row, and is the first evidence the flake is master-state. Recorded in `docs/inflight/test-processinkeyorder-sanity-check-races-the-first-poll.md`. - One `java-grpc` conformance run left its last record unsettled, 1 of 2 full-reactor runs and green 3 of 3 in isolation. `core` and `java-direct` were green in the same run, so the suspect is the engine's settle path under a full ceiling rather than the scenario. The scenario was NOT weakened - `docs/inflight/proxy-a-java-grpc-ceiling-run-left-its-last-record-unsettled.md`. ## What is deliberately NOT done here `Main#sessionServiceFactory` still returns `NoEngineSessionService`, so the production entry point hosts no engine. That is unit U10's, which #384 named as still having no PR, and it owns the drain that must land with the engine - plus eight cross-language handshake tests assert the `UNIMPLEMENTED` refusal by status code specifically. Recorded rather than faked, in `docs/inflight/proxy-the-production-entry-point-still-hosts-no-engine.md`. The engine is exercised end to end through `TestModeMain` regardless.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…am-memory-leak Master gained 144 commits since cae88c7. Four textual conflicts, and one semantic conflict that auto-merged clean and would have shipped wrong. THE SILENT ONE: master's #120 (de1620d) rewrote `PCMetricsTest.metricsRegisterBinding` wholesale and un-quarantined it. Git kept master's rewritten test body and this branch's `@Quarantined` annotation on top of it, so the merge compiled and the test stayed dark. The annotation is removed: master fixed exactly the defect this branch's quarantine reason described. This branch said the failure was "the metric behind and never converging... whether the un-committed tail is a wrong test assumption or real commit behaviour is the open task"; master answered it - workers awaited a latch BEFORE incrementing, so a latched worker's offset never completed and the gap was permanent, not slow. The write-up is `docs/solutions/test-issues/metrics-test-compared-a-commit-offset-to-a-completion-counter-2026-08-20.md`. That retires this branch's eb602ae and 16a456e in full - the re-quarantine and the cross-branch sighting that justified it. `PCMetricsTest.java` is now identical to master's. `docs/quarantined-tests.md`, `docs/inflight/test-untracked-ci-flakes.md`: master's side taken. It drops both of this branch's entries - `metricsRegisterBinding` above, and `OffsetEncodingBackPressureTest.backPressureShouldPrevent...`, which master diagnosed and fixed in #351/#336 (`back-pressure-freezes-the-frontier-the- test-asserted-2026-08-24.md`). Both quarantines were honest when written and are now superseded by fixes, not by wishing. `docs/inflight/next-candidates.md`: master's #323 renamed it to `process-candidate-ranking.md`, so the delete wins and this branch's one edit - dropping the `confluentinc#912` ready-pick, since it is this PR - is re-applied to the renamed file. A modify/delete conflict hides that kind of rename: taking either side alone loses the other's work silently. `docs/inflight/branch-912-vertx-leak.md`: deleted, as this branch did in 2dc76c9 - it is this PR's own pre-PR ledger entry. Master only touched it to add priority metadata. Its two inbound citations are repaired here rather than left dangling: the ready-pick above, and the naming example in `docs/inflight/AGENTS.md`, which now cites only `bug-857-family.md` for a confluentinc-numbered note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…e diagnosed, not hidden The lane in this PR is RED on the machine that verified it, deliberately. Handing it over green would have meant either repricing a bound the lane's own notes say transfers to no other machine, or inverting an arm on a hypothesis that turns out to be wrong. WHAT THE EVIDENCE SAYS ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear misses eight times out of eight at its committed bound - consistent, so not a flake. bin/lincheck-test.sh exits 1, so the failure propagates and there is no silent-green problem underneath it. AND WHAT LOOKED TRUE AND IS NOT The obvious reading is that #336 fixed the check-then-act this arm's counterexample turns on - it rewrote 182 lines of ProcessingShard, the code now says "ADMIT FIRST ... never the read above", and it edited the lane's note in the same commit, which is why that note reads as current while describing a defect that commit addressed. Every part of that correlation holds. The control refutes it anyway. Restoring the genuine pre-astubbs#336 shape - get, branch on the read, put inside each branch - produces no violation either. If the fix were what silenced the arm, reintroducing the defect would restore the counterexample. So the harness is not exercising the seam on this machine, which is the OTHER branch of the message the harness itself prints. Recorded because the next agent will form the same hypothesis in the first ten minutes, and the control costs an hour to build. An invalid first control is recorded too: it kept the put atomic and only moved the accounting, so it never reintroduced a check-then-act at all - reading the real pre-astubbs#336 method before writing a control is the lesson, and git show 3e668a4^:<path> is how. WHAT IT HANDS OVER The recommendation is to gate the model-checking arms and run the stress arms advisory, because the commit's own gating argument - the model checker explores interleavings deterministically rather than sampling them, so a violation reproduces - is true of model checking and false of stress. It never covered the arm that failed. Repricing the bound and adding a retry are both named as the wrong moves, with the reasons, since both are what an agent under pressure to go green reaches for.
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…s there, the arm still does not The note handed over one cheap next move: confirm the Lincheck stress arm fires *anywhere* before spending anything on the harness. CI ran the lane on this PR's head for the first time and answered it. `ShardManagerLincheckTest.stressMustNotRediscoverTheShardTear` missed again, with the identical message, on `ubuntu-latest` - four cores, the opposite end of the range from the 32-core box the earlier evidence came from. The decisive part is not the miss but what passed beside it. On the same runner in the same JVM, `LincheckToolchainProbeTest`'s stress arm and `PartitionStateLincheckTest`'s stress arm both found their violations. The toolchain probe exists to answer exactly that question: Lincheck's stress strategy works there, the JDK is adequate, and the `-Plincheck` flags are landing. So two of the three surviving candidates are ruled out - a JDK or Lincheck version difference, and a host property such as core count - and one is left standing: this arm's operation set no longer reaching the seam it was written against, after #335, #336 and #373 each rewrote `ProcessingShard`. That also re-reads the earlier control. Reintroducing the check-then-act produced no violation either, which was ambiguous under the three-candidate list and is corroborating under this one: if the operations no longer reach the seam, restoring the defect at the seam changes nothing, which is what was measured. Records the cost correction too - 7m42s hosted against ~2m50s locally, 364s of it `WorkManagerLincheckTest`. The matrix entry's `timeout: 20` still covers it; the `~2m30s` in its comment does not. Evidence: run 33511221453, job 99867204560, head 58c8c7c.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…all four were decided Its status line said "cluster 1 measured, clusters 2-4 undecided". That stopped being true on 2026-08-27 and nothing updated it, so a reader arriving today would think three open decisions were waiting for them. WHAT LANDED, WITH THE COMMIT FOR EACH Cluster 2, ThreadConfinedConsumer, became #393 (951ae1e) - the plan said do not land as-is, last and separately, and that is exactly how it went. Cluster 3, the counter adjustment, became #336 (3e668a4). Cluster 4, the pausedForThrottling reset, became #376 (b2e6c19). Cluster 1 is this PR and is still open. THE OUTCOME WAS BETTER THAN THE PLAN IMAGINED, IN THE SAME WAY TWICE Clusters 3 and 4 were framed here as "half a fix" and "probably a regression", which invites patching or reverting. Neither happened. Each turned out to mirror a fact something else already owned, and both were fixed by deleting the mirror: the load gate is now derived by conservation, and the back-pressure pause is read from Kafka instead of tracked alongside it. Recorded because the plan's framing would have led somewhere worse, and that is worth a future planner knowing. WHAT WAS NOT DONE The document is not rewritten to match today's code. The decomposition it argued is the reason those three PRs exist, and anyone arriving from one of them needs the reasoning as it stood on 2026-08-18. Only the status line and a closing section were added, per docs/citations.md's rule about dated records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…r this PR to close Four problems, found by reading it against what is now known rather than by any gate. A MANGLED HEADING THIS PR INTRODUCED `## Delete when` at the end.)` sat mid-file, a corrupted fragment that created a phantom second "Delete when" section a thousand lines above the real one. Master has one; this branch had two. A stray `##` is not cosmetic in a file whose retirement criterion is found by that exact heading. THE COUNTER-DRIFT FIX HAD LANDED AND THE FILE STILL SPOKE OF IT IN THE FUTURE TENSE The opening paragraph said the conservation fix "lands independently of this attribution; if reports persist after it ships, this mechanism is ruled out". It shipped - #336, 3e668a4 - and two later paragraphs in the same file already refer to it in the past tense. The test that paragraph proposed is therefore live in the wild and nobody has checked it, which is now what it says. A FIFTH OPEN ITEM WAS MISSING, AND IT IS THE MOST INTERESTING ONE largeNumberOfInstances stalls at a rebalance with a member not answering, reproduced twice - one in ten on Linux, and on CI at 55edffa. The mode table in this file rules the #29 cycle out by construction: PERIODIC_CONSUMER_ASYNCHRONOUS never blocks on commit, so the AB-BA pair has no second edge. It is also not the throughput defect found the same week, because the CI reproduction carries that fix. It is distinguished from the fourth open item by being protocol-VISIBLE - the group cannot converge because a member went silent, where the fourth is a stall with the group STABLE and heartbeats flowing. Recorded with what stops it being a diagnosis: the profile kills instances continuously, so a silent member may be one the harness stopped, and the question is identity rather than rate. ITS OWN RETIREMENT CRITERION WOULD NOW DESTROY EVIDENCE "May be retired once #29 lands and the remaining open item - the original deadlock - has its own solutions write-up." The write-up exists, so only the merge was outstanding. But "the remaining open item" stopped being singular: a fourth and a fifth were added after that sentence was written, and the fifth reproduces on a tree carrying the fix. Applying the criterion on merge would delete two live unexplained stalls, so it is marked as needing a rewrite rather than an application - what retires on the merge is the deadlock's section, not the register. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…he lane gate green The lane was wired up in this PR and landed RED on one arm: `ShardManagerLincheckTest`, an inverted harness asserting that Lincheck FINDS a violation, found nothing. The handoff this branch carried recorded that as "#336 REFUTED - do not re-run it". That verdict was wrong, and this commit replaces it with the experiment that settles it. Bisected rather than reasoned about. The harness has not changed since #345 and Lincheck is pinned at 3.7 in both trees, so only product code varies. Running the unchanged harness against each tree: #345 (0112c70) FIRES confluentinc#905 hot-shard metric FIRES #373 claim compare-and-set FIRES #336 (3e668a4) misses #336 is the sole commit touching core's main sources between the last hit and the first miss, so attribution is exact rather than merely bisected-to. The counterexample it removed is the one the lane's note recorded months ago - `revokeSweep(0)` in the prefix, then `addWork(0)` against `addWork(0)` - and #336 removed it by admitting to the population before the put and reading the outcome from the map instead of from the earlier read. Three hand-written controls had said otherwise, and all three were wrong: one kept the put atomic and never reintroduced a check-then-act at all, one restored the map's check-then-act alone, one restored the counter's bare increment alone. The defect was in neither half - it was deciding the accounting from the pre-put read, which #336 removed wholesale. A control assembled by hand tests the shape you believed the defect had, so its negative says nothing about the commit. The durable form of that lesson is in docs/solutions/best-practices/reverting-half-a-fix-is-not-a-control-2026-09-01.md. So the arm gets the disposition the lane's inversion contract prescribes: it is now `stressFindsNoWayToBreakTheShardMap`, asserting no violation, and #336 is what it regression-tests. The bound is deliberately unchanged - 50 x 5,000 is the budget the counterexample was FOUND at, which makes this zero worth more than one measured at a bound picked for cost. 0 in 250,000 invocations, and 0 in 2,500,000 at ten times it, on a 32-core box with Temurin 17.0.20+8 and Lincheck 3.7. The whole lane is now green locally - 9 tests, 2m45s, all six harnesses selected - so the leg lands GATING as the matrix entry already declared, and the quarantine-or-advisory question the handoff left open does not arise. The matrix entry's own gating argument is corrected too. It claimed the model checker explores interleavings deterministically, which is true of model checking and false of this lane: no model-checking arm over a product class runs here at all, they are blocked on Lincheck rewriting a Lombok `callSuper` hashCode into a recursing self-call. What licenses gating is that the three assert-no-violation arms need no hit to pass. The residual flake risk is named where it actually lives - the still-inverted arms, `PartitionStateLincheckTest` and the toolchain probe, which do need a hit and whose rate was measured 3.4x apart across two machines. Also corrects the entry's ~2m30s estimate to the 7m42s measured on ubuntu-latest, and retires the handoff note: its open question is answered, and what outlives it has moved to the solutions write-up and to the lane's owning note.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…o its instrument branch THE PLAN IS DELETED, AND THE ARGUMENT FOR KEEPING IT WAS WRONG It was defended here on the grounds that a reader arriving from one of the three PRs it produced would need its reasoning. Checked: none of #393, #336 or #376 references it. Its only durable citation was one line in the architecture-patterns write-up; the other three were in notes that are deleted when this PR lands. Nothing was lost, because that write-up already carried all four clusters as sections, in more detail than the plan had - the measured counter table, the eager-versus-cooperative reasoning. What it did NOT carry is the observation the plan could not have made either, so that is added: clusters 3 and 4 were the same defect twice. Each kept a local copy of a fact something else already owned, and the fix in both was to delete the copy rather than correct it - the load gate derived by conservation, the pause read from Kafka. The tell, stated for next time, is a field whose bugs are all of the form "X changed and Y did not": that is a second source of truth asking to be removed, not an accounting error to be balanced. Three citations repaired. Two quote or lean on the plan's own text, so they point at history (`git show a0ade8a:...`) as docs/citations.md prescribes, rather than being cut. THE A/B DEADLOCK PROBE MOVES TO #405 Same reasoning as the torture harness it now sits beside: an overnight instrument, run by hand, in this PR only because it was written here. Its A/B results stay - those are a finding about the deadlock, not a property of the instrument. That PR also gains a note on why neither instrument runs. The short version is that the obvious reading - "a scheduled soak lane needs building" - is wrong: experiments.yml already is one, and the real gap is that an A/B needs a control tree the lane cannot pass, and an eight-hour design does not fit a four-hour cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
Merged
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…he stale target/ that nearly reversed it Review asked for the #336 attribution to be defended against #336's own commit message, which says the Lincheck lane was green "still finding the violation it is calibrated to find". Replicating the bisect to answer that turned up a bigger problem with how it had been run. The first bisect checked out each commit inside ONE worktree. Re-running the same commits in a second worktree - one built at a different commit first - reported 0 hits out of 10 at a commit the first pass recorded as firing. Same commit, same machine, same harness; only the inherited `target/` differed. Maven's incremental compilation had left another commit's classes in place, so the harness ran against a tree that never existed, and nothing in the output says so. It fails silently in both directions, which is what makes it worth a section of its own rather than a footnote. Re-run with a FRESH worktree per commit, so no two trees can share class output: #345 (0112c70) fires 5/5 confluentinc#905 hot-shard metric fires 5/5 #373 (8000926) fires 5/5 #336 (3e668a4) misses 5/5 The attribution holds, now on numbers that are reproducible rather than single-shot. Each miss pays its full bound, so that row is 0 hits in 1,250,000 invocations. On the commit message: run on #336's own tree the whole lane is RED, on this arm alone, with the other five green. The message's own line - "adapted cherry-pick of fa4d1cf" - is the likely mechanism, the lane it reports on plausibly being the pre-adaptation one. Recorded in the javadoc rather than explained away: if that line is ever shown to describe the merged tree, the flip is wrong. On the machine-dependence alternative the review raised: the miss is confirmed on a second machine. Before the flip this arm also found nothing on ubuntu-latest, four cores against 32, while the toolchain probe's stress arm fired on that same runner. The lane's measured 3.4x cross-machine variance is a rate effect, and a rate effect does not turn 15 of 15 hits into 0 across one commit on one box, nor agree across two machines that far apart.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…und dark (#404) Six Lincheck harnesses and bin/lincheck-test.sh landed with #347 and were never wired into CI: no workflow referenced them, and every CI script passed -Dexcluded.groups=...,lincheck, so the tag was excluded everywhere and included nowhere. A test that never runs is not a passing test, and nothing went red to say so for the lane's whole life. Running it found one arm dark. ShardManagerLincheckTest's stress arm is inverted - it asserts Lincheck FINDS a violation - and found nothing. Three hand-written controls said #336 was not the cause, and all three were wrong: each reverted one half of it onto today's tree, and the defect was in neither half alone. It was deciding the accounting from the pre-put read, which #336 removed wholesale by admitting to the population first and reading the outcome from the map. Settled by bisect with a FRESH worktree per commit, which turned out to matter: reusing one working copy reported 0 hits out of 10 at a commit that in fact fires 5 of 5, because Maven's incremental compilation left another commit's classes in place. A shared target/ hands you a clean, wrong bisect and nothing in the output says so. Re-run cleanly: fires 5/5 at #345, at confluentinc#905's hot-shard metric and at #373's claim compare-and-set; misses 5/5 at #336, the sole commit touching core's main sources in that interval. So the arm is flipped to stressFindsNoWayToBreakTheShardMap, asserting no violation, with #336 as what it regression-tests. The bound is unchanged at 50 x 5,000 because that is the budget the counterexample was FOUND at, which makes the zero worth more than one measured at a bound picked for cost: 0 in 250,000 invocations, and 0 in 2,500,000 at ten times it. #336's own commit message claims the lane was green "still finding the violation it is calibrated to find". Run on that tree it is RED, on this arm alone. The likely mechanism is in the same message - it is an adapted cherry-pick of fa4d1cf - and it is recorded in the javadoc rather than explained away. The matrix entry's gating argument is corrected too. It claimed the model checker explores interleavings deterministically, which is false of this lane: no model-checking arm over a product class runs at all, they are blocked on Lincheck rewriting a Lombok callSuper hashCode into a recursing self-call. What licenses gating is that the assert-no-violation arms need no hit to pass. The residual risk is named where it lives - PartitionStateLincheckTest and the toolchain probe still need a hit, and both fired on ubuntu-latest on every CI run of this PR. Method lesson in docs/solutions/best-practices/reverting-half-a-fix-is-not-a-control-2026-09-01.md.
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
Master gained the conservation-derived RecordPopulation (#336) and the atomic compute() admission in ShardManager.addWorkContainer (#437), both of which touch the ProcessingShard constructor this branch also extends with DispatchScanMeter. The PR body predicted a one-line resolution at that constructor; it came to three files plus one compile fix: - ProcessingShard: both fields kept. RecordPopulation stays first so master's parameter order is unchanged and the meter is appended as the last argument of the Lombok required-args constructor. - ShardManager: master's compute() block is taken whole, with dispatchScanMeter added to the constructor call inside it. Both shared instances are fields on the manager. - ShardManagerTest and ShardPopulationRaceTest: master's new call sites pass a meter; the test-local shards get a fresh DispatchScanMeter since nothing reads it there. The scanMeter.onEntryExamined() increment auto-merged into the rewritten scan loop (#373) still sits once per examined entry, so the parity test's exact totals are unchanged. Verified: OrderingModeDispatchParityTest, ShardManagerTest and ShardPopulationRaceTest green under JDK 17. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqkUNK5K7JXkbbGoNFTvfB
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ever a fresh replacement The poller's stale sweep asked each occupant whether it was stale and then removed it with `removeWorkAtOffset(entry.getKey())` - by KEY. The staleness answer is about one container; the removal is about one offset. Between the two statements the occupant can change, and it does: the sweep runs on the broker-poll thread inside a rebalance callback, while `addWorkContainer`'s stale-replacement branch runs on the controller, and nothing orders them. The harm is a LOST RECORD. The fresh container is evicted while `PartitionState` still carries its offset as incomplete, so nothing selects it and nothing completes it - the commit high-water mark for that partition cannot advance past it until the partition is re-polled. The accounting half was already closed (#336, #373), so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left. THE OBVIOUS FIX IS WRONG, AND IT WOULD HAVE SHIPPED LOOKING CORRECT. `computeIfPresent(offset, (k, v) -> v == inspected ? null : v)` reads as airtight - the function only asks for a removal if the current value IS the container the sweep inspected. Measured against the JDK 17 source and then by experiment, it is not. `ConcurrentSkipListMap.computeIfPresent` commits a removal through `doRemove(key, v)`, which RE-READS the node's value and gates on `value.equals(reRead)` before its compare-and-set: else if (value != null && !value.equals(v)) // v was just re-read from the node break outer; else if (VAL.compareAndSet(n, v, null)) { ... } `WorkContainer.equals` is topic, partition and offset only, so a replacement landing in that gap is equal-by-offset, passes the gate, and is removed. Reproduced directly: with an offset-only-equals value type, a put landing inside the remapping function leaves the map EMPTY - the replacement destroyed - while the identical program with an identity-equals value type returns the replacement and keeps it. So the identity check narrows the window from "the whole staleness check" to "a few instructions inside the JDK" and closes nothing. No production-seam test can tell the two apart, which is why this is written down rather than merely avoided. `workMap.remove(offset, container)` fails for the same reason one level up: the JDK's compare-and-remove is DEFINED by `equals`, so with offset-only equality no map API can express which container was meant. The value type's equality IS the contract. THE FIX. The shard's map stores a `Residency` token rather than the container. It overrides neither `equals` nor `hashCode`, so its equality is reference identity, and `Map.remove(key, value)` - one atomic step, fully specified, independent of which side the implementation compares - means exactly "remove this occupancy, or nothing". There is no interval for a writer to land in, so there is no window left to narrow. Nothing evicted is a correct outcome, not a failure: the replacement won the offset, so the call retires nothing and REPORTS nothing. That matters downstream - `ShardManager.removeStaleContainers` feeds the returned list to the retry queue, and #437 pinned that the queue removal is reached only through a real shard removal. Reporting a container this call did not remove would break that gating and take the retry entry at coordinates the fresh container now owns. The red test's assertion that the sweep reports the inspected container is corrected accordingly; the assertion that pins the defect - the fresh container survives, by reference identity - is untouched. `getWorkIfAvailable`'s last-resort stale removal has the same shape and is written the same way, with a dated cleared-suspicion note saying why the race is not reachable there today (`addWorkContainer` is the only writer and runs on the controller, which also runs that scan) and what would reopen it. REJECTED: giving `WorkContainer` identity equality, which would delete the token and fix every value-conditional operation on a container at once. It is breaking - `WorkContainer` is public and its `compareTo` orders by offset, which would become inconsistent with equals - so it is queued in docs/refactoring.md under the next major. Also rejected: a remove-then-put-back repair, and a claim the sweep takes before removing which the writer must then wait out; both reintroduce a window, one in the map and one on the controller. EVIDENCE. The defect arm was red against master and is green here. Ablation: restoring `removeWorkAtOffset(entry.getKey())` in the sweep and changing nothing else sends exactly that arm red - the control arm and the premise arm stay green. A third arm pins the premise itself, that no removal keyed on container equality can say which container it meant, `computeIfPresent` included; it is also the tripwire that goes red the day `WorkContainer.equals` becomes identity-based, saying the token can then be deleted. Whole `state` package green, full unit suite green across every module, every `bin/check-all.sh` gate passing. SAME-DEFECT-CLASS SWEEP over core main - a removal keyed on something that does not identify the element that was inspected. One real hit, not fixed here: `ExternalEngine.holdingDispatchPermit` is a `Set<WorkContainer>` whose permit accounting is per RECORD while its membership is per OFFSET, so two containers at one offset leak a dispatch permit permanently. Recorded with its reachability argument in docs/inflight/bug-dispatch-permit-set-cannot-tell-two-containers-at-one-offset-apart.md. Dismissed with reasons: `ProcessingShard.onSuccess` (a non-stale resident is never replaced, and a stale result never reaches it - `handleFutureResult` short-circuits); `removeWorkAtOffset` from the revocation path (emptying an offset regardless of occupant is its purpose); `RetryQueue` (keys by `WorkContainerKey` deliberately, and removes the value it just looked up, under its write lock); the metrics and counter maps (keyed by an identifier that fully identifies the thing); `OffsetSimultaneousEncoder.activeEncoders` (per-instance, single-threaded encoding path). Resolves docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md, whose mechanism, rejected alternatives and open decision are migrated to docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md - the other side of the same class as the confluentinc#909 write-up, which is cross-linked from it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ot timing (#358) A guard on the ordering-mode dispatch ratio that counts what the scan examines instead of timing it. UNORDERED dispatch re-walks the whole in-flight prefix on every pass; KEY skips an occupied shard in one step. Their scan costs diverge as in-flight depth grows, and nothing pinned that: a change that made one shape quadratic would have surfaced only as "the benchmark feels slower". DispatchScanMeter is a LongAdder shared across every shard of one ShardManager, incremented once per entry the scan examines. It is shared rather than per-shard because an emptied shard is removed from the map, which would discard a per-shard count and undercount KEY far more than UNORDERED - the exact asymmetry the guard is meant to detect. OrderingModeDispatchParityTest asserts exact totals - one examination per record for KEY, the triangular sum for UNORDERED - which are properties of the algorithm rather than of the machine, so the assertion is deterministic and load-immune and names which shape changed. Why a counter and not a stopwatch: the first version timed the two modes and compared durations. Inside a thread-parallel suite that is a race with the rest of the run, not an assertion about the code. Four attempts to stabilise it - fastest-of-three, ratio instead of absolutes, CPU time instead of wall clock, @isolated - each moved the flake without removing it, three of them after a bisect had shown the dispatch code unchanged. The meter is main-code deliberately: it counts inside the scan loop, where a test double cannot reach without altering the thing being measured. Its accessor on ShardManager is package-private, matching the other test-visible instruments in that class, so the public API does not grow. Extracted from the engine-performance campaign on perf/engine-concurrency; the direct- pull scan work in #361 measures with this meter. Reconciled with master's RecordPopulation conservation counter (#336) and the atomic shard admission (#437) at the ProcessingShard constructor, where the meter is the last argument. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Each of these notes carried a PROPOSED vetting marker from the 2026-09-07/08 sweep. The owner accepted each proposal as stated; this applies them, and every claim was re-checked against the tree at this HEAD before the edit, not against the marker's own reading. Each surviving note's marker now records what was applied and what was verified. bug-shared-collections-across-the-poll-boundary.md - closed. Both named defects are fixed on master: PCMetrics.registeredMeters is a final LinkedHashSet with every add, walk and remove under @synchronized("metersLock") and close() walking it through the never-throwing removeQuietly (758744b, #120 / #57), and RemovedPartitionState.READ_ONLY_EMPTY_SET is Collections.emptySortedSet() (6aae299, #267). The file is kept rather than removed because static-infer-findings.md and core-139-public-api-thread-safety-contract.md both cite it; its "Live on master" headline is corrected and the branch section reduced to what is still true of fix/concurrent-collection-sweep, which still exists on origin with no PR. bug-torn-read-family.md - shrunk to the racing-double unification, the next hunt iteration and the out-of-family pointers. All three candidates are fixed and verified in the tree, so their dossiers were a FIXED narrative; they are replaced by a summary plus a history pointer, with candidate 1's bootstrapPhase-versus-dirty disjointness argument kept because it is the safety argument for the encoder's single-sample read and is recorded nowhere else. bug-177-commit-response-timeout-unreproduced.md and bug-857-mirror-attributions-unconfirmed.md - each shrunk to its #175 half. #177 was closed as completed on 2026-09-01, one second after #399 merged, with no comment naming either candidate mechanism - the outcome both notes existed to prevent. That is recorded once, in the attributions note, which owns the attribution question; the other cross-references it. Candidate 3 is closed: #29 and #57 have merged and PCMetrics.removeQuietly now swallows everything meterRegistry.remove throws. The filename keeps #177 so citations of it survive. bug-stale-sweep-iterator-evicts-fresh-replacement.md - shrunk to the remove-by-key defect, which is live: removeStaleWorkContainersFromShard still removes by key, so a fresh container the controller put at that offset is what leaves. The mechanism sentence is re-anchored on removeWorkAtOffset, and the #373 counter/stall block is removed - #336 (3e668a4) closed it and the note's own last paragraph already contradicted its middle. bug-857-transactional-revoke-wait.md - re-premised on #466. The unbounded spin is gone: in transactional mode the revoke callback posts to the control thread and waits bounded by commitLockAcquisitionTimeout (commitOnRevokeViaTheControlThread). What survives is whether that bound is right against confluentinc#803's complaint that it burns max.poll.interval.ms, which #408 holds along with its RebalanceEoSDeadlockTest amendment. Four stale citations repaired: the "no open PR addresses it" claim, the dead fix/bound-revoke-transaction-wait branch, the renamed core-recoverable-producer-fencing.md, and the two file:line citations, replaced with greppable anchors. bug-retry-queue-write-lock-on-the-rebalance-path.md - the defect is intact and stays the subject; "not started, and no design agreed" is replaced with #431, the open fix, which RetryQueue.remove's own javadoc already names as the tryLock() path being added. bug-wedged-after-poisoned-transaction.md - merged into bug-poisoned-transaction-not-aborted-while-running.md, the survivor the owner chose, and removed. Folded in: the answered recovery question and its answer (isTransactionOpen() is producerState.equals(BEGIN), so lazyMaybeBeginTransaction never opens a replacement and nothing in band aborts the poisoned one), the observed wedge, the two mitigations, and the freeze-behaviour test suggestion. Two citations repointed at the survivor; the third, in process-candidate-ranking.md, names the merge itself and is correct as written. bug-857-family.md - shrunk and retyped as a register, which is what its own closing sections describe it as; the impact stays stall. The revoke-path deadlock's mechanism section is retired per the file's own criterion now that #29 has merged; the "reproducer cannot settle anything" section is replaced with what the rewritten RebalanceEoSDeadlockTest and its purpose-built sibling Rebalance857CommitSyncDeadlockProbeIT establish; and the fifth item's claim that ProgressTracker.withDiagnostic has never been wired is corrected - MultiInstanceRebalanceTest now supplies describeFleet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W511R2TZFAqEiqgS5yogTv
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
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
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…moves only the container it inspected (#468) BREAKING CHANGE: RecordContext equality changes. Its Lombok @EqualsAndHashCode covers the WorkContainer it wraps, so two contexts built from different containers for one record no longer compare equal - a Set, a Map key or an equals check over record contexts that used to collapse them now keeps both. Narrow in practice: ConsumerRecord does not override equals either, so two contexts from two different polls were never equal; what changes is two contexts over the same ConsumerRecord instance. WorkContainer itself is public in modifier only and is not part of the surface a user is expected to hold. THE DEFECT. The poller's stale sweep asked each occupant whether it was stale and then removed it by KEY, with removeWorkAtOffset(entry.getKey()). The staleness answer is about one container; the removal is about one offset. Between the two statements the occupant can change, and it does: the sweep runs on the broker-poll thread inside a rebalance callback, while addWorkContainer's stale-replacement branch runs on the controller, and nothing orders them. The harm is a LOST RECORD. The fresh container is evicted while PartitionState still carries its offset as incomplete, so nothing selects it and nothing completes it - the commit high-water mark for that partition cannot advance past it until the partition is re-polled. The accounting half was already closed (#336, #373), so the counters settled correct while the record was being lost: correct counters are not evidence that the right object left. It is the same fresh-replacement-at-one-offset class as the confluentinc#909 write-up, reached from the other side. THE FIX IS IN THE VALUE TYPE, NOT THE REMOVAL. Map.remove(key, value) is the JDK's compare-and-remove, and it decides "still mapped to the value I inspected" with equals - so what that call MEANS is a property of the value type and not of the map. WorkContainer.equals was topic, partition and offset only, so a stale container and its fresh replacement compared equal and no map API could express which of the two was meant. The equals/hashCode pair is deleted, equality is reference identity, and workMap.remove(offset, inspected) is a true compare-and-remove: it evicts the container the sweep inspected or nothing. Nothing evicted is a correct outcome, not a failure: the replacement won the offset, so the call retires nothing and REPORTS nothing. ShardManager.removeStaleContainers feeds the returned list to the retry queue, and #437 pinned that the queue removal is reached only through a real shard removal; reporting a container this call did not remove would take the retry entry at coordinates the fresh container now owns. getWorkIfAvailable's last-resort stale removal has the same shape and is written the same way, with a dated cleared-suspicion note saying why the race is not reachable there today and what would reopen it. REJECTED, and why each would have shipped looking correct: - computeIfPresent with an identity check in the remapping function. ConcurrentSkipListMap commits the removal through doRemove(key, v), which RE-READS the node value and gates on value.equals(reRead) before its compare-and-set, so with offset-only equality the identity test does not survive to the commit. Reproduced: a put landing inside the remapping function left the map EMPTY under offset-only equals and kept the replacement under identity equals. It narrowed the window to a few instructions inside the JDK and closed nothing. Under identity equality the same arm shows the gate declining directly: the remapping function runs TWICE, the first pass planting the replacement and asking for a removal that doRemove refuses, the second pass being handed the replacement. That is recorded at the site rather than asserted, since an implementation reaching the same gate without looping would be equally correct. - A ProcessingShard.Residency token as the map value, overriding neither equals nor hashCode. An earlier revision of this branch shipped it. It bought exactly one correct removal and left the value type wrong, so the next value-conditional site would have had to remember to wrap. Replaced before merge on the maintainer's call: 0.6.0.0 is the breaking release and already carries a Breaking section. - A remove-then-put-back repair, and a claim the sweep takes before removing which the writer must wait out. Both reintroduce a window, one in the map and one on the controller. THE OBJECTIONS THAT HAD QUEUED IDENTITY EQUALITY AS FUTURE WORK, each checked and each fallen: Comparable only RECOMMENDS consistency between compareTo and equals; SortedSet and SortedMap REQUIRE it, and nothing in main code puts a raw container in either - RetryQueue sorts by its own WorkContainerSortKey and de-duplicates by its own WorkContainerKey, so compareTo and its comparator are unchanged with the inconsistency stated in the javadoc. The one collection keyed on containers elsewhere, ExternalEngine.holdingDispatchPermit, has been an IdentityHashMap-backed set since #342, so identity equality makes the container agree with that code rather than changing it. The public break is RecordContext, stated above. THE OTHER HALF OF THE CONTRACT IS ENFORCED, NOT WRITTEN DOWN. Nothing puts a raw container in a sorted collection today, and both RetryQueue and ProcessingShard.workMap are one refactor away from being written the other way round with nothing going red. WorkContainerIsNeverInASortedCollectionArchTest reads every field, return type and parameter in main code whose declared type is a sorted set, sorted map or priority queue and whose FIRST type argument - the element-or-key position for all of them - is WorkContainer, so a container in a sorted map's value position passes. Proved red on main code with a temporary TreeSet<WorkContainer> field on ProcessingShard, which reported that field and not the workMap declared beside it. A standing positive control, in the RebalanceCallbackRuleControlTest shape, hands a hand-imported fixture to the real rule object so the rule cannot go quietly blind. Its limits are in the test javadoc: a body-local collection is invisible, the comparator is invisible so a legitimate Comparator-taking construction is still reported, and it covers core's classpath - measured, no main source outside core declares any of those types. EVIDENCE. ShardStaleSweepReplacementEvictionTest drives the seam deterministically rather than by racing: ShardSeamTestBase's spied PartitionStateManager lands the controller's replacement at an exact instruction inside the sweep. The defect arm was red against master. Restoring the old coordinate-based equals/hashCode and changing nothing else sends the defect arm and the premise arm red - "expected specific instance ... but was: null" and "expected not to be" the same coordinates - while the control arm stays green, as it must: a sweep with nothing racing it never has to say which of two containers it meant. Restoring removeWorkAtOffset(entry.getKey()) in the sweep sends exactly the defect arm red. The premise arm, twoContainersAtOneOffsetMustNotBeInterchangeable, asserts the equality contract directly and is the tripwire for a reintroduced coordinate-based equals. TREE-WIDE SWEEP for what identity equality breaks: one test assertion, ShardManagerTest's isNotEqualTo between containers at different offsets, became vacuous and is restated against RetryQueue.WorkContainerKey, which is what the queue keys on. PollContext.getByTopicPartitionMap's Set de-duplication becomes a no-op with no observable change, said in an @implNote at the site. Checked and found empty: no ArgumentCaptor or Mockito matcher over a container, no Truth containment assertion over containers except on the same instance, no container as a Map key or Set element in any test helper, and every Lincheck harness returns primitives from its operations in stress mode. SAME-DEFECT-CLASS SWEEP over core main, with a correction to this branch's own earlier result: an earlier commit reported holdingDispatchPermit as a real unfixed hit and opened an inflight note for it. That defect does not exist - the sweep matched the declared type Set<WorkContainer> and stopped, and a collection's semantics live in its initialiser, not its declaration. The note is deleted and the lesson is in the solutions write-up. Dismissed with reasons: ProcessingShard.onSuccess, removeWorkAtOffset from the revocation path, RetryQueue, the metrics and counter maps, OffsetSimultaneousEncoder.activeEncoders, MDC.remove. CI SIGHTING RECORDED, NOT DIAGNOSED. Chaos Pain Suite shard 4 went red once on ChaosChurnStormIT's ZOMBIE_MEMBER/REBALANCE_BLOCKED arm with maxInstanceStall=0ms; that line has an open ledger on master, and seed 1053013618367208111 is appended to docs/inflight/test-857-churn-storm-async-stalls.md because the seed dies with the CI log. Nothing on the rebalance path reads WorkContainer.equals, and the shard was green on every later push. DOCS. docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md is resolved and deleted, its mechanism and rejected alternatives migrated to docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md, cross-linked with the confluentinc#909 write-up. docs/refactoring.md's queued-breaking-change section now states that an entry is deleted in the PR that lands it, because the commit message carries the release-note content and the section lists only what is still queued; the entries marked DONE there are removed, and the two citations that pointed at this branch's own entry - WorkContainer's class javadoc and the solutions write-up - name this commit as the record instead. docs/inflight-tool.md used the resolved note as the worked example for the vet command; the example now names a surviving note with the same data-loss signal, bug-run-length-plausibility-ceiling.md. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
Merged
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…ever holds the offset (#492) WHAT CHANGED FOR A USER: a rebalance can no longer remove a record that a later assignment had already re-registered. `ShardManager.removeWorkFromShardFor` - the revoke and lost path - removed by KEY with `removeWorkAtOffset(consumerRecord.offset())`, so whatever occupied the offset when the removal landed is what left the shard. It now removes only the container the revoked record was registered as, or a stale one, and never a live container from a later registration. THE DEFECT CLASS, AND WHERE THIS SITE SITS IN IT. #468 closed the same class at the poller's stale sweep: a removal keyed on the offset cannot say WHICH of two containers at that offset it meant, and the fix was to make `WorkContainer` equality identity so that `Map.remove(key, value)` is a true compare-and-remove. #483's defect-class sweep reported this site alongside it - "the one remaining unconditional by-key shard removal" - and deliberately left it, on the grounds that it belonged with #468's mechanism rather than with a reachability proof. This is that. No fork or upstream issue tracks it; it was reported in a commit body and on a `docs/inflight/` note, so this PR closes nothing. THE CALLER HOLDS NO CONTAINER, WHICH IS WHY THE FIX IS SHAPED DIFFERENTLY. The stale sweep inspects a container and then wants that one gone. This sweep is handed the `ConsumerRecord`s ONE generation was still carrying as incomplete (`PartitionState.onPartitionsRemoved` -> `removeAnyShardEntriesReferencedFrom`), so what it can name is the REGISTRATION, not an inspected container. That works because `PartitionState.maybeRegisterNewPollBatchAsWork` puts the same record instance into `incompleteOffsets` and into the `WorkContainer` it builds, in one loop body - so "the container this generation registered for this record" is `occupant.getCr() == revokedRecord`, and a later generation's re-delivery of the same offset is a different object because it came from a different fetch. That identity has one named exception, stated at the site: `addNewIncompleteRecord` puts unconditionally while `addWorkContainer` DROPS an arrival whose resident is not stale, so an in-generation replay of an already-registered offset leaves the partition naming record B while the shard holds the container over record A. The staleness condition below covers it. TWO CONDITIONS, AND THE MIDDLE OPTION WAS WRONG. Registration identity alone declines for a container from another registration that is nonetheless STALE, and declining there would leave a stale container holding an offset of a revoked partition - the state this sweep exists to prevent. Staleness alone declines for a live container built from the record the sweep was handed, which is what a revoke sweep driven before the partition's state has been swapped sees, and is what `ShardManagerLincheckTest.revokeSweep` models on every invocation: that harness's removal operation would have become a silent no-op while staying green. Verified rather than assumed - `incrementPartitionAssignmentEpoch` starts from `KAFKA_OFFSET_ABSENCE = -1`, so that harness's `EPOCH = 0L` containers are live against the epoch its constructor assigns. So the guard declines for exactly one thing - a LIVE container from another registration - which makes the change a no-op everywhere except in the defect case, and each condition has its own ablation arm. THE HARM, IF IT IS REACHED, IS THE LOST RECORD - #468's, not a misdirection. The live container is gone from the shard while its own `PartitionState` carries the offset as incomplete, so nothing selects it and nothing completes it, and the commit high-water mark cannot pass it until the partition is re-polled. The v6 scope note this came from said the cost was "misdirection bounded to one control-loop tick by #481's purge"; that is the DISPLACEMENT orphan's cost, not this one's. This removal is a shard removal, and the purge collects retry-queue entries. REACHABILITY: NOT TODAY, ON TWO INDEPENDENT ARGUMENTS, AND THE STRONGER ONE IS STRUCTURAL. - Thread ordering, which has to name BOTH production routes. On the ordinary route the poll thread runs both rebalance callbacks, so a generation's revoke sweep completes before the assignment that could register a replacement begins. On the CLOSE route the sweep runs on the CONTROL thread (`maybeCloseConsumer` -> `onLeavePrepare` -> `onPartitionsRevoked`), which `PartitionStateManager.resetOffsetMapAndRemoveWork`'s javadoc calls live in every commit mode - and that is also `addWorkContainer`'s thread, so the two still cannot interleave. "Both callbacks are on the poll thread" is the half-statement, and it was written that way here first. - `resetOffsetMapAndRemoveWork` installs `RemovedPartitionState` BEFORE it calls in, so `checkIfWorkIsStale` is true for EVERY occupant and the staleness condition carries all of them: on the shipped path this method is behaviourally identical to the by-key removal it replaces, and the decline branch is defence for a caller that sweeps before the swap. That says nothing about threads. Nothing checks either argument. The conditional form costs one reference comparison and rests on neither, which is exactly why #468 wrote `getWorkIfAvailable`'s last-resort sweep conditionally against a race it had shown was unreachable there. THE NPE GUARDS ON THIS PATH ARE UNTOUCHED, AND THE CLEARED SUSPICION NAMES THE RIGHT OBJECT. The single-read `getShard` idiom #345 put in stays where it was; the new work is inside `ProcessingShard`, below it. The staleness question resolves the partition state from the OCCUPANT, not from `revokedRecord` - and in the only branch that asks, the occupant is by definition a different registration - so the discriminator is two facts: the state is installed before the call (and a partition with no state `continue`s without sweeping at all), and every `ShardKey` variant is partition-scoped, so an occupant of this shard belongs to the revoked partition. WHAT WOULD REOPEN IT: a caller from a path that has not installed a state, or a topic-scoped shard key - the latter being the reopener `addWorkContainer`'s own cleared suspicion already lists, which reopens both at once. EVIDENCE. `ShardRevokeSweepReplacementEvictionTest`, three arms, RED-then-GREEN through `PartitionState.onPartitionsRemoved` so the argument list is built by production code - one level below the state swap, which is what makes the decline branch reachable at all. Ablation matrix, predicted before running, one arm red per ablation and no other: - whole guard removed, back to `removeWorkAtOffset(offset)` -> the displacement arm - staleness condition removed -> `aStaleContainerFromAnotherRegistrationIsStillSweptOut` - registration-identity condition removed -> `aRevokeSweepStillRemovesALiveContainerBuiltFromTheRecordItWasHanded` - decline retires the occupant instead of leaving it -> the displacement arm's accounting assertion That fourth ablation is why the accounting assertion exists: making the decline call `population.onRetired()` + `excludeFromSelection(occupant)` left every other assertion GREEN while creating a permanent `RecordPopulation` deficit, which is this class's recurring defect and has no clamp and nothing to reconcile it. TWO INSTRUMENTATION FAILURES, RECORDED RATHER THAN TIDIED AWAY. The registration-identity ablation reported NO failure on its first run: dropping the condition left an unused local, the module never recompiled, and the grep for failures matched nothing - a silent false negative in the experiment meant to prove the condition earns its place. Re-run with the build outcome asserted, it is red as predicted. Separately, `@SuppressWarnings("ReferenceEquality")` was written on the identity comparison and then measured: deleted, rebuilt under `-Pci`, `BUILD SUCCESS`. Neither `WorkContainer` nor Kafka's `ConsumerRecord` overrides `equals`, so Error Prone never fired - it suppressed nothing and disagreed with `isResident`'s bare `==` two methods away. Removed, with the reason written where it was. SAME-DEFECT-CLASS SWEEP over `ShardManager`, `ProcessingShard` and `PartitionState`. Fixed: the one above; no unconditional by-key removal of a container remains in main. Dismissed with discriminators: `ProcessingShard.onSuccess` (re-verified rather than inherited - `WorkManager.handleFutureResult` is the third staleness checkpoint and drops a stale result before `onSuccessResult`, so a returning stale container cannot remove the fresh one that replaced it); `addWorkContainer`'s by-key INSERTION (#483's leg 1, plus #336's admit-first, which reads the outcome from the map rather than the earlier read); `removeStaleWorkContainersFromShard` and `getWorkIfAvailable`'s last-resort sweep (already conditional, #468); `ShardManager.getShard` / `compute` / `computeIfPresent` (single-read Optional idiom, #345, and atomic per-key, #336 - not check-then-act); `RetryQueue.remove` / `removeAll` at every call site including `onSuccess`, `onFailure` and `purgeDepartedRetryEntries` (same shape, but it is the queue's keying model rather than a removal that forgot to name its target, so a fix is a change to that model - reported and left by #483, and left here); `PartitionState.incompleteOffsets.remove` and the truncation prune (offsets over records, control-thread-only - "this offset is complete" does not depend on which record object represents it). `removeWorkAtOffset` now has no production caller. Kept as the modelling primitive four test classes use for "a container departs its shard", with its javadoc rewritten: it used to open by naming the `onPartitionsRemoved` caller this change moved away, and now says what it is and which of the two conditional removals a main-code site should reach for instead. RECORDS. `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md` - the class's owner - gains a dated section for the second site, because its own sweep's only hit was a false positive and the real one was found a day later. `docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md` gets a pointer, not a narrative: its subject, state and `PROPOSED closed` marker are untouched, and what is recorded is only that a reader arriving at #483's four-item list can see which one moved. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -pl parallel-consumer-core -am test -Dtest='ShardRevokeSweepReplacementEvictionTest,ShardStaleSweepReplacementEvictionTest,ShardManagerRevokeSweepNpeTest,ShardDisplacementOrphanReachabilityTest'`; `bin/check-pr-analysis-surfaces.sh 492`. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #119 (confluentinc#857) - recorded there as a candidate mechanism, not claimed as the root cause.
Description
The broker-poller load gate was fed by a drifting counter. It is now derived by conservation, the clamp that hid the drift is deleted, and every change to that conservation figure is paired with a mutation the map actually performed.
The original defect: a counter whose decrements did not match its increment
ProcessingShard.availableWorkContainerCnthad two conditional decrements whose conditions did not match the increment's - both reproducible on a single thread, no race involved despite the clamp's "in case of possible race condition" comment:isAvailableToTakeAsWork()- which requires the retry delay to have passed - so the incrementProcessingShard.onFailure()had just added was left behind permanently.revokingARecordParkedInRetryBackoffLeavesNoPhantomAwaitingSelectionis that sequence; on the old code the counter reads 1 against an empty shard.The summed counter fed
WorkManager.isSufficientlyLoaded(), which decides whether the broker poller pauses - so the drift did not misreport, it mis-gated record intake. Enough phantom records keep intake paused until a restart clears the in-memory counter. Because the reproducible high-side path is exactly rebalance-shaped, this is recorded indocs/inflight/bug-857-family.mdas a candidate fourth mechanism behind the paused-consumption family (#119, and possibly #183) - a hypothesis to be confirmed or ruled out, stated as such.What replaced it
The gate only ever consumed the sum of "awaiting selection" and "out for processing", and that sum is just "records inside the system" - so
RecordPopulationderives it by conservation:admitted - retired, two monotonicLongAdders, admitted when aWorkContainerenters a shard's entry map, retired when it leaves by any path. The per-shard counter survives for theWAITING_RECORDSmetric and diagnostics; what it no longer does is gate intake. With no steady-state reader left, deleting it outright becomes viable - verified and recorded indocs/inflight/core-shard-selection-counter-can-now-be-derived-by-scan.md.Making the pairing hold under concurrency
A conservation figure is only as good as the pairing between its counters and the map they describe, and there is no clamp and nothing that reconciles the two - so every mispairing drifts permanently and silently. Three places decided from something other than what the map did, and are fixed here (raised by Codex on review; regression tests below):
addWorkContainerdecided "replacement, so do not admit" from the resident it read before the insertion. A stale sweep landing in between removes and retires that resident, making this an insertion rather than a replacement - and the skipped admission leaves the population permanently one below the truth. It now admits first and derives fromput's own return value, compensating with a retirement when the insertion really did displace something. Admitting before inserting also preserves the ordering invariant that makesgetInSystem()non-negative by construction rather than by clamp.iterator.remove()that discards the map's return value, so a concurrent revocation sweep that won the race had one admission retired twice. Removal now goes throughremoveAndRetire, which retires exactly what the map gave up. A value-conditionalremove(offset, container)would be wrong here -WorkContainer.equalsis topic/partition/offset only, so a fresh container that replaced the stale one compares equal to it - and the javadoc says so.ShardManagerhanded a shard back fromcomputeIfAbsentand then wrote to it as a separate step. Under KEY orderingremoveShardIfEmptycan collect that shard in between, admitting the record into a map no scan reaches. The record was already lost that way; what is new is that the admission is never retired, so the figure reads permanently high - the direction that pauses polling for good, where the old gate simply stopped summing a shard it no longer held. Choosing the shard and writing to it are now onecompute(), against the same per-key lockremoveShardIfEmpty'scomputeIfPresenttakes. This is distinct from thecontainsKey-then-gettear inremoveWorkFromShardFor(fix(core): revoke sweep NPE when a shard is removed against its read - poller death under KEY ordering #345) thatShardManagerLincheckTestcalibrates against; that NPE is still reachable and the Lincheck lane still finds it.Two further review findings landed with them:
getWorkIfAvailable's last-resort stale eviction now cleans the retry queue as well as the shard, the wayShardManager.removeStaleContainersalways has. An orphaned queue entry is removed by nothing, ever, and it biases the workable figure the gate reads. This closes a pre-existing tracked defect, whose in-flight note is deleted here.ShardManager.getWorkableRecords()returns a carrier, soisSufficientlyLoaded()decides and logs from one call andgetNumberOfWorkableRecordsInSystem()is a delegate. A consistent pair is not obtainable - the population reduces lock-freeLongAdders while the retry queue reads under its own fair lock, and spanning both would put the poll thread's admission path behind that lock - so the javadoc now states the residual honestly: retry-queue movement in the window costs nothing, population movement is the whole skew, it is bounded by admissions during the retry-queue read, and it cannot accumulate because the gate resamples every tick.isSufficientlyLoadedlogs the snapshot it decided on. It capturedworkableonce and then re-read all three operands for the diagnostic, so the printed equation could show numbers that never held together - in the one line whose job is to tell a genuine silent stall from counter drift.One review finding was left open through review and is now closed:
retireAndDeductIfStillCountedinferred "was this record counted" fromisNotInFlight()after the map had given the container up, so a controller-sideendFlight()could land in between. Closing it needed the container to own its own accounting rather than a patch at one removal site - which is exactly what #373 landed, collapsing that helper and its sibling into oneretire(). The window is gone rather than deferred.Verification
theConservationFigureMatchesTheShardsThroughEveryKindOfDeparture) drives every departure path in one sequence - success, failure into back-off, retry redelivery, a stale sweep under live work, a revocation taking a parked record and one out at a worker, and the stale result handed back afterwards - holding the O(1) figure against an O(n) scan computed by different means. Plus the failing-on-old-code phantom test and the load-gate agreement test.ShardPopulationRaceTestadds one regression test per concurrency finding above. Each interleaving is driven deterministically rather than raced for, by making a collaborator act on the way past (PartitionStateManager.getPartitionStatefor the two insideProcessingShard,ConsumerRecord.offset()for the one between choosing a shard and writing to it). All four confirmed red on the tree without the fixes, each on its own assertion, and green with them.bin/ci-unit-test.sh),bin/check-all.shgreen, and the Lincheck lane green - includingShardManagerLincheckTest, which still finds the violation it is calibrated to find.Related
Same defect class: the clamp-hides-conditional-decrement pattern is written up in
docs/solutions/logic-errors/counter-clamp-hid-a-conditional-decrement-bug-2026-08-21.md, which ships here. The related-but-distinct dispatch-layer instance (check-then-act work claim) is #335.Provenance. Adapted cherry-pick of
fa4d1cf251fromperf/engine-concurrency: the verdict-free abandonment departure stays on that branch with itsWorkManagerside (the conservation walk's javadoc notes where the branch extends it), andProcessingShardStaleReplacement909Test- which postdates the original commit - is reconciled the way that branch's own merge reconciled it, through an accounted insertion rather than a raw map put.Composes with #373, which landed on master while this was open and rewrote the same class. That one makes the shard's selection count owned by a compare-and-set on a per-container claim; this one makes the shard's population derived from the map's own mutations. They count different things and neither is derivable from the other, so the merged code keeps both, with the rule written on the population field: the map's return value drives the population, the CAS outcome drives the selection count, and neither is conditioned on the other. Three consequences worth calling out - #373 collapses the two retirement helpers this PR introduced into one and closes the
isNotInFlight()window left open in review; the shard's map stays private, withplantResident()replacing the public getter #373 added for white-box tests; and #373's claim that the shard counter becomes deletable once this lands is verified and recorded indocs/inflight/core-shard-selection-counter-can-now-be-derived-by-scan.md.Checklist
bug-number-records-out-for-processing-is-a-plain-int.mdandcore-shard-selection-counter-can-now-be-derived-by-scan.mddocs/features/- N/A - internal correctness fix, no user-facing featureShardPopulationRaceTest's four red-then-green race regressionsce-simplifyandce-code-reviewlocally - N/A - reviewed instead by the automated@claude review thisroute and by the owner's own inline review, whose five findings are addressed in23cdcaaec; the merge resolutions since were argued on the merits in their commit bodies rather than run through a simplify pass