Repository navigation
test(core): the shard-displacement retry-queue orphan is unreachable, and the guard is outside the class - #483
Merged
Conversation
…rite lock RED ON PURPOSE at this commit. RetryQueueRebalancePathTest holds RetryQueue's READ lock exactly the way the controller thread does - through a live iterator, which ShardManager.getLowestRetryTime keeps open for a whole scan - and then drives the production rebalance callback on a thread named broker-poll. The lock is constructed fair, so an arriving writer queues behind that scan instead of interleaving with it, and the wait is spent inside consumer.poll() out of max.poll.interval.ms. Five of the six arms fail against this tree: - aRevokeDoesNotTouchTheRetryQueueAndSoCannotWaitForItsWriteLock - the callback is still parked on the write lock when the ten-second deadline passes (ShardManager.removeWorkFromShardFor calls retryQueue.remove). - theStaleSweepDoesNotTouchTheRetryQueueEither - the same, through the epoch-change sweep. Driven by re-assigning an already-assigned partition, which is what increments the epoch and then runs ShardManager.removeStaleContainers in one callback; that method maps the METHOD REFERENCE retryQueue::remove, which is why this reach stayed invisible to ArchitectureTest until #465 taught the walk to follow references. - aDepartedEntryIsCollectedByTheNextControllerPass, aQueueOnlyEntryNoLongerHoldsTheDrainOpen and underOrderedProcessingADepartedEntryIsStillCollectedOnTheSameTick - all assert a controller-side collection that does not exist yet. anEntryWhoseContainerIsStillResidentIsNotCollected is the control and is green here and after: a parked-for-retry container that is still resident is ordinary state, not garbage. The contended-read-lock harness and the parked-for-retry fixture are adapted from #431, which built them for the design this branch supersedes. Its join placement is load-bearing and is kept: the poll thread is joined only after the try-with-resources releases the read lock, because on unfixed code it is parked until then. Reproduce: JAVA_HOME=<jdk17> ./mvnw -Pci -pl parallel-consumer-core -am test -Dtest=RetryQueueRebalancePathTest Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
…ller collects what it leaves The diagnosis. RetryQueue.remove takes an unbounded, FAIR writeLock().lock(), and every Kafka rebalance callback reached it on the broker-poll thread: by direct call through ShardManager.removeWorkFromShardFor on the revoke and lost paths, and by method reference (.map(retryQueue::remove)) through ShardManager.removeStaleContainers on all three, assigned included. The controller thread holds the matching READ lock for a whole scan - RetryQueue.iterator() hands it out and it is released only at close, which ShardManager.getLowestRetryTime does around an entire iteration - and the lock is fair, so an arriving writer queues behind that scan rather than interleaving. That wait sits inside consumer.poll() with the whole group waiting on it, spent out of max.poll.interval.ms. No cycle was found, so this is an unbounded wait rather than a deadlock. What changed for a user. A rebalance can no longer be delayed by however long the control loop's retry-queue scan takes, in any ordering mode or commit mode. The fix. The callbacks remove from the SHARDS only - a ConcurrentHashMap of ConcurrentSkipListMaps, with no lock to wait for and so nothing to decline. ShardManager.purgeDepartedRetryEntries() then collects, on the controller thread where waiting is allowed, every retry-queue entry whose container ProcessingShard.isResident says has left: it scans under the read lock, closes the iterator, and removes the collected entries under the write lock in one acquisition via removeAll. The invariant, stated once at the purge: a retry-queue entry with no resident container is garbage the controller collects; it may exist for at most one control-loop tick; the callbacks are free to create it. Where it runs, and why there. At the top of ShardManager.getWorkIfAvailable - the one point the control loop reaches unconditionally every pass through retrieveAndDistributeNewWork in both RUNNING and DRAINING, and before drain() reads getNumberOfWorkQueuedInShardsAwaitingSelection in that same pass. The drain path is the consequence that matters: #437 established that a queue-only orphan survives that figure's flooring once the pipeline is drained, so it reads true forever and a draining close hangs to its timeout. The bound is one control-loop tick in every ordering mode. The purge scans the retry queue rather than the shards, so the ordered modes' shard-scan break - which stops the scan at the first container handed out - cannot delay it. That is the one place this design is strictly better than the rejected one rather than merely smaller. Why the scan and the removal can be two steps. Departure is monotonic (isResident is reference identity and nothing re-inserts the same instance), and nothing else writes the queue's keys: RetryQueue.add is @ControllerThreadOnly, its only production caller is ShardManager.onFailure, and the purge runs on that same thread. The second half matters because RetryQueue removes by topic/partition/offset and so cannot say which container it meant - the defect class of #468. What would reopen it is a second writer. The rejected design, named. #431 kept the poll thread on the queue and made it DECLINE: tryRemove, abandon the paired shard removal on refusal, and after #437 a second ask after the shard removal with a put-back on refusal. It is correct and it is proven; it costs a second non-blocking entry point keyed differently from the blocking one, a queue reference threaded into a second ProcessingShard method, two asks per removal, and a retirement bound that varied by ordering mode. Over the same three main files it is +347 -72 against +185 -67 here, most of the latter javadoc. Also rejected: reusing #466's control-thread hand-off, which has the poll thread WAIT on a CompletableFuture - right for a revocation commit, wrong here, because the purge needs no hand-off at all and reusing that shape would reintroduce the wait being removed. Counters during the window, re-measured on this tree. Between a callback's shard removal and the purge, getWorkableRecords (inShards - parkedForRetry) reads one LOW - negative in the single-record case. Low is the safe direction: isSufficientlyLoaded() is workable > threshold, so it fetches sooner; high would pause the poller with nothing to process. WorkManagerTest's revocation arm was re-stated to pin the direction and that one pass settles it. #437's post-add residency confirmation in ShardManager.onFailure becomes belt-and-braces - the purge would collect the orphan it prevents a tick later - and is kept this round, with its javadoc saying so and naming it as a live question rather than a settled one. Evidence. Red-then-green: the five red arms of the previous commit are green, with the control arm green throughout. Ablations: deleting the purge call turns six arms red across three classes; inverting the residency predicate turns seven red - the same six plus the control. The ArchUnit rule was proven on merit rather than assumed: with KNOWN_BLOCKING_VIOLATIONS emptied and the callbacks still reaching the queue it reports 24 violations (18 exemption keys; the @ControllerThreadOnly half fires once per route), and green afterwards with the set still empty. All eighteen entries are deleted; the set, its root => target keying and the argument for keeping it remain, because an exemption is how a newly-found reach is recorded as a defect on the books. Also: RetryQueue.removeAll widens to Collection<? extends WorkContainer<?, ?>> - the body only reads topic/partition/offset, and no <K, V> signature accepts the WorkContainer<?, ?> the queue's own iterator yields without an unchecked cast. Error Prone's UnnecessaryTypeArgument found the three test call sites that carried a now-redundant type witness. Two existing arms are re-stated rather than deleted, because the sweep they described stopped existing: theProductionSweepsQueueRemovalIsGatedOnItsShardRemoval becomes theProductionSweepLeavesTheRetryQueueToTheController (the sweep touches no queue; the controller pass then collects exactly the departed entry and leaves the resident one), and theSameRebalanceOneSeamLaterTakesTheQueueEntryWithTheShardEntry becomes ...LeavesTheQueueEntryForThe ControllerToCollect. aQueueFirstSweepDefeatsTheOneShotConfirmation keeps its name and assertion - two write-ups cite it - and gains a javadoc saying it now models a superseded design rather than an incoming one. Collides textually with #468, #431, #410, #392 and #362 on the sweep, and #408 on ArchitectureTest; none of them conflicts in design. Reproduce: JAVA_HOME=<jdk17> ./mvnw -Pci -pl parallel-consumer-core -am test \ -Dtest='RetryQueueRebalancePathTest,RetryQueueRequeueWindowTest,WorkManagerTest,ArchitectureTest' Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
…ve records that named #431 The defect is fixed, so its in-flight note has stopped describing reality. Following the four outcomes in docs/inflight/AGENTS.md, what outlives the work is migrated first and only then is the file removed. New: docs/solutions/runtime-errors/retry-queue-write-lock-on-the-rebalance-path.md records BOTH designs. Design A (#431) prevents the split - decline with tryRemove, abandon the paired shard removal on refusal, ask again after it - with why it was correct, why it was proven, and why it lost on machinery rather than on correctness. Design B is what shipped: the poll thread never asks, and the controller collects retry-queue entries whose container is resident in no shard. It carries the invariant, the one-tick bound, the two-step scan's safety argument, the counter direction, the rejected alternatives (including reusing #466's hand-off) and the red-then-green evidence. The write-up is adapted from the one Design A wrote for itself, so its diagnosis sections are preserved rather than re-derived. Removed: docs/inflight/bug-retry-queue-write-lock-on-the-rebalance-path.md (retired into the above) and docs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard-removal.md, whose whole subject - #431's queue-first sweep needing a second removal - is a property of a design that will not land; its reasoning is in Design A's section. Added: docs/inflight/branch-retry-queue-controller-purge.md, for the three things no command will tell a reader - that #431 is still open and must not be merged, which open PRs collide textually on the sweep, and which decisions this deliberately leaves open. Corrected beside their dated text, never over it, because each is a record of a measurement or a state at a date: - static-archunit-main-code-rules.md - KNOWN_BLOCKING_VIOLATIONS is now empty on merit, measured by deleting it with the reaches still in place (24 violations). The paragraphs describing the list as it was stand; what is corrected is that its named owner is superseded, and that the tryLock sibling which would have made @ControllerThreadOnly the only discriminator does not exist. Its citation of the retired note is repaired in place as a history pointer, which is the repair docs/citations.md prescribes and the file-ref gate recognises. - test-retry-queue-behaviour-untested.md - two of its four items are now covered, by removal rather than by symmetry; the other two are still open and now have RetryQueueTest and RetryQueueLincheckTest to read first. - bug-shard-displacement-orphans-the-retry-queue-entry.md - the displaced container's entry is now collected on the next pass, so the FIGURE it names as the harm is bounded by one tick; the pairing gap itself and its production-reachability question are unchanged. - core-retry-queue-needs-a-runtime-controller-ownership-guard.md - its precedent is superseded, and the purge's safety resting on the queue having exactly one writer makes that guard more load-bearing, not less. - test-largenumberofinstances-residual-failures-measured-not-explained.md and static-infer-threadsafe-is-blocked-by-third-party-interfaces.md - both point a reader at #431 by number; the refutation in the first is untouched and explicitly not to be re-run. - docs/solutions/runtime-errors/retry-queue-orphan-window-between-the-requeue-check-and-the-add.md gains a dated correction: #431 will not land, so the assertion it says to invert stays as written, the confirmation it is about is now belt-and-braces, and "what would reopen it" is superseded in one direction and sharpened in the other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
That sweep stamped or restated nearly every in-flight note, including all seven this branch touches, so the conflicts are all in docs and all about the same fact: it recorded #431 as "the fix in flight", which this branch supersedes. Resolved: - bug-retry-queue-write-lock-on-the-rebalance-path.md and pr-431-must-pair-its-queue-removal-with-the-shard-removal.md - modify/delete, resolved as DELETE. Master's additions to both are a vetting stamp confirming the defect was intact on 2026-09-07/08 and a rewrite of "Fixing it" naming #431. Neither outlives the fix: the stamp's finding is the problem this branch closes, and the design it names is the one this branch rejects. Both designs and the evidence are in docs/solutions/runtime-errors/retry-queue-write-lock-on-the-rebalance-path.md. - test-retry-queue-behaviour-untested.md - content conflict, resolved by TAKING MASTER'S SHRUNK NOTE and re-stating this branch's correction against it. The sweep had already closed the two items the correction addressed, and better than the correction did; what it could not know is that two sentences in its own text name a mechanism this branch removes - `removeWorkFromShardFor`'s ordering, which no longer exists because that method no longer touches the retry queue. The correction is now only about those two sentences. Auto-merged and checked rather than trusted: bug-shard-displacement-orphans-the-retry-queue-entry.md, static-infer-threadsafe-is-blocked-by-third-party-interfaces.md and test-largenumberofinstances-residual-failures-measured-not-explained.md each gained a dated vet stamp above this branch's dated correction, which is the intended shape - the stamp records what was true on its date, the correction says what moved on 2026-09-08. Two stamps make claims this branch falsifies (#431 still open as the reason a clause has not gone stale; RetryQueueRebalancePathTest existing only on that branch) and are deliberately left standing, dated, with the correction beneath. No vet markers are added by this branch: docs/inflight/AGENTS.md's "Vetting a note" writes the marker only on the outcome where a note is still true AND unchanged, and every note here is shrunk, corrected or removed - which is its own record. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
… and one stale comment Three reviewers (reuse, quality, efficiency) over this branch's diff. Two findings applied, one recorded rather than applied, and the reason for that is the interesting part. APPLIED - the new test hand-rolled a harness BlockedThreadAsserter already is. It built its own thread, CountDownLatch, bounded await and join, which is exactly that class's machinery; the repo's rule is to extend the shared helper rather than grow a parallel copy, and a drifted copy of test setup has already been a flaky-CI source here. BlockedThreadAsserter gains a third idiom, `returnsWithin(function, threadName, budget)`, and `joinQuietly`. It is the NEGATIVE of the two it had - they assert a function blocks, this hands back whether one returned - and a caller proving a non-block usually cannot give up its own thread to an assertion helper, because it has to hold the thing the function would wait for and read state while the contention is on. That is why it returns the fact rather than asserting. What the reuse buys beyond deduplication: `runCapturingThrowable`. A callback that throws on entry returned promptly, so the hand-rolled version could not tell it from a callback that did its job - a false PASS for a test asserting promptness. `returnsWithin` raises it instead. The thread name is the caller's, because these tests are about which thread a call is on and `broker-poll` is what a stack dump has to say. Re-proved after the refactor rather than assumed: restoring the queue removal to both callback paths turns three arms red again, the two lock ones by timing out at the deadline. BlockedThreadAsserterTest (the negative control for the two existing idioms) and ProducerManagerTest (its main caller) are green. APPLIED - `ShardManager.onFailure`'s residency branch still described a race that no longer exists. Its comment and log said a `false` from `retryQueue.remove(wc)` means "the revoke sweep's own paired removal got there first", which was true when the sweep removed from the queue. It cannot happen now: the add is a few lines above, the sweep never touches this queue, and every other removal is on this same controller thread later in the pass. So TRUE is the only outcome production can produce, and FALSE became a SIGNAL - it would mean a second writer of the retry queue exists, which is the one thing that invalidates purgeDepartedRetryEntries()'s scan-then-remove. Said that way at the site, since nothing asserts it. RECORDED, NOT APPLIED - both the quality and the efficiency reviewer independently found that the purge is the first UNCONDITIONAL full scan of the retry queue on the control loop. Every other reader there early-stops on the retryDueAt sort order; shard residency has no relationship to that order, so this one cannot, and it finds nothing on the overwhelming majority of ticks. The fix they converge on is to gate the scan on a departure signal - and the review sharpened what that signal should be: a monotonic LongAdder in the shape RecordPopulation already uses at those very call sites, not the boolean flag the design rejected, which also keeps the one-tick bound trivially. It is not applied here because it puts poll-thread-written state back on the rebalance path, in the change whose whole point is taking the poll thread off this structure - and because the cost is unmeasured. DispatchScanMeter is this class's own precedent for metering exactly this, and the honest order is meter, then decide. Recorded in the write-up's rejected alternatives and in the branch note, with that order stated, so it is a queued decision rather than a lost review comment. Also from the same pass, and deliberately NOT acted on: the reviewers confirmed the ArrayList allocation on an empty tick is free (the no-arg constructor defers the backing array), and that the fair read lock held across the scan cannot hurt the poll thread any more, because the poll thread no longer touches this lock at all. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
… and the guard is outside the class Settles the open question on docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md: whether production can reach ProcessingShard.addWorkContainer's displacement branch with a queue-resident container. It cannot. Nothing user-visible changes; what changes is that a demonstrated pairing gap now has a recorded reachability verdict, a control-armed test, and a named list of what would reopen it. WHAT WAS ESTABLISHED An orphan needs four things true at once: a resident A at the offset; A observed stale, or the arrival is dropped; A holding a RetryQueue entry; and a second container B at the same coordinates. Each leg traced in source: - The displaced container is always the one inspected. workMap has two insertion sites, addWorkContainer (control-thread-only) and the test-only plantResident, so only removals interleave between the get and the put and a removal makes the put return null. A by-key REMOVAL can hit a different occupant - #468's class - but a by-key insertion cannot, with one inserting thread. - A queue entry implies non-stale-and-resident when it was made. RetryQueue.add has one production caller, ShardManager.onFailure, behind onFailureResult's live staleness check and, since #437, a post-add residency confirmation. couldBeTakenAsWork refuses a stale container outright, so one that is stale on arrival is never selected, never fails and never queues. - Staleness is monotone: both epochs are final longs, the fence is never cleared, a replaced state carries a higher epoch or is the removed singleton. - So A must cross the boundary in between, and only three transitions can do it - the removed-state swap, the assignment putAll, and fenceForRevocation. The first two each carry their sweep on the same thread inside the callback. The fence carries none, but a second container at those coordinates needs the offset delivered twice, which needs a re-assignment - the swept path. WHAT WAS REFUTED That incrementPartitionAssignmentEpoch opens the window. It changes no staleness answer: PartitionState's epoch is final and staleness is only asked through the state object, so the window opens at the state REPLACEMENT, which is the same statement as the sweep that closes it. Also refuted: that KEY ordering could collide two partitions' records on one offset inside a shard - ShardKey scopes every shard to a partition and says so. HOW ShardDisplacementOrphanReachabilityTest, three arms, control-armed because a single green arm here is green whether the sweep works or the branch is simply never reached: the production rebalance sequence (green, and the sweeps are what make it green); confluentinc#909's late drain, which fires the branch with an unqueued container; and a positive control that reaches stale-and-resident-and- queued through the production API via the fence and then injects the one arrival production cannot supply - the orphan appears immediately, asserted by reference. Ablation, predicted before running: dropping either sweep alone leaves arm one green (each covers the other); dropping both turns it red at exactly the load-bearing assertion. Observed as predicted. WHAT THIS DOES NOT CLAIM The last leg is a property of the Kafka consumer's fetch position, not of this engine, so the pairing gap is one arrival away rather than absent. An in-generation replay of an already-registered offset - a seek, an offset-reset or truncation replay - makes it reachable, and nothing goes red for that. The cleared suspicion is recorded on the branch itself per the engine's AGENTS.md rule, with what would reopen it. The inflight note is left OPEN with a PROPOSED-closed marker: its impact is misdirection, and docs/inflight/AGENTS.md reserves that state change for the owner. Its now-false reachability paragraph is corrected in place. Co-authored-by: Claude Opus <noreply@anthropic.com>
…es introduced `bin/check-pr-analysis-surfaces.sh 481` reports findings on lines the diff WROTE separately from ones inherited in files it touches. Three of the former, all DLS_DEAD_LOCAL_STORE, all from the same habit: naming a discarded return value. The repo has already settled this tension and the resolution was two feet away, at `ShardManager.onFailure`: "a dead store in main code trades one static-analysis finding for another". The root AGENTS.md rule - never discard a return value silently, name it and say why - is about a reader being unable to tell a decision from an oversight; LOGGING the value satisfies that better than an ignored local, and satisfies the analyser too. So each site now uses its value rather than parking it: - `removeWorkFromShardFor` logs what the shard gave up at trace, beside the trace its else-branch already writes. The line says the retry queue is deliberately untouched here, which is the fact a reader arrives at this method wondering about. - `purgeDepartedRetryEntries` returns void. Nothing in production read the count, and a caller that named one only to drop it is exactly the dead store; what is worth knowing it already logs. Said in its javadoc, along with where a meter would go if the scan's cost is ever measured - the deferred optimisation both reviewers asked for. - The purge's `removeAll` answer is folded into the log it already writes when something was collected, and it is the same signal as the one at `onFailure`: this thread is the queue's only writer, so `false` can only mean a second writer removed them first - which is precisely what would invalidate the scan-then-remove the purge is built on. Inherited findings in the same files are left alone, as that script's own output instructs: they belong to the registries under docs/inflight/. Reproduce: bin/check-pr-analysis-surfaces.sh 481 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Updated for Superseded by a newer quarantine lane report. |
🟢 Throughput — OKThis branch measured about 3% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 5 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Since the previous push: ratio 1.196 -> 0.973, share 1.596 -> 1.887, rate 75502 -> 75314 (-0.2%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #483 +/- ##
============================================
+ Coverage 82.44% 82.88% +0.43%
- Complexity 1554 1563 +9
============================================
Files 96 96
Lines 5377 5380 +3
Branches 534 535 +1
============================================
+ Hits 4433 4459 +26
+ Misses 750 729 -21
+ Partials 194 192 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nse what waited on it The owner closed #431 as superseded. Six places on this branch said it was open; each is now correct, and two of them were making a claim that mattered rather than merely naming a number. Retired: docs/inflight/branch-retry-queue-controller-purge.md. Its whole first purpose - "#431 is still OPEN and must not be merged" - is fulfilled, and the four outcomes in docs/inflight/AGENTS.md put what remains where it is owned rather than deleting it wholesale: - the collision list is answerable by `gh pr list` plus `gh pr diff --name-only`, and is in this PR's body where a reader of the change will meet it; - the belt-and-braces confirmation, the runtime ownership guard and the shard-displacement orphan are each already owned by ShardManager.onFailure's javadoc, the solutions write-up, and their own notes, all corrected in this branch's earlier commits; - the one item with no owner was a SPLIT, not a deletion, and is now docs/inflight/perf-retry-queue-purge-scan-is-unmetered.md - the purge's unconditional full scan, the LongAdder-gated shape both reviewers converged on, why it was not taken with the change, and the order that would settle it (meter first, using DispatchScanMeter's precedent, then decide). It carries `deferred` with that as its reason, so it sorts as a decision waiting on evidence rather than as work nobody did. Past-tensed beside the dated text, never over it: - ShardManagerLincheckTest's fixture javadoc rested on "would go red the moment that PR lands". That event will never happen, so the urgency is void - and the fixture change was still right, for the half of its argument that never depended on the PR: the reach is created by a state the product deliberately makes, and a harness whose fixture contradicts its own comment is a false green whatever the schedule. Said there, because the next reader of that comment is the one who would otherwise take the void premise for a live one. - core-stale-arrival-guard-needs-a-null-safety-decision.md carried the same premise for the same sighting. Its datum is explicitly unaffected - it is about that note's own question, not about the PR's outcome - which is why the paragraph stands with a correction rather than being cut. - The two 2026-09-07 `inflight-vetted` stamps that read "#431 is still OPEN" are left exactly as written - they were true on their date, and that is the point of a dated stamp. What each note's 2026-09-08 section now says is that the stamp's REASONING no longer holds: the shard-displacement clause is dead for a different reason than the stamp gives, and largeNumberOfInstances' "false positive because RetryQueueRebalancePathTest lives on #431" resolves because that test now lives here. - The orphan-window write-up's correction is sharpened from "superseded, not merged" to "CLOSED, never merged", with the consequence stated: the arms it promises to invert stay as written. Gates: check-branch-self-reference (now with PR #481 resolved, so the number phrases are checked too), check-file-refs, check-inflight-tags, check-issue-refs, and the full bin/check-all.sh - all green. ShardManagerLincheckTest re-run green after its javadoc edit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…rphan is known unreachable Two things moved under the burn-down on 2026-09-08. The owner's #481 supersedes #431 for the retry-queue lock on the rebalance path: the poll thread never touches the queue and the controller collects what it leaves. #431 is closed; tier 1 now lists #481, ready and green, and notes that #483 stacks on it. The sweep's reading, moved into this note earlier, still called #431 a draft; it is annotated rather than rewritten, since it is the sweep's dated reading. The shard-displacement orphan window is known unreachable by #483, with the one caveat worth carrying: the proof's last leg is a Kafka property (the fetch position never goes backwards within a generation), so an in-generation offset replay reopens the window and nothing goes red. #481's purge bounds the cost to one tick of misdirection. Two more by-key removals of the same shape are named and left for #468, whose identity-equals change is what makes conditional removal possible. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
…te leaves open #481 takes the rebalance callbacks off the retry queue and has the controller purge entries whose container has left. It edits the same three files this work does, and its "Update 2026-09-08" section on the shard displacement note ends by saying production reachability is still not established - which is exactly what this work answers. Merged so the two read as one argument rather than two adjacent ones. WHAT CONFLICTED, AND HOW IT WAS RESOLVED ONE textual conflict, in ShardManager.onFailure's javadoc. Both sides rewrote the same trailing paragraph about addWorkContainer. #481 said the purge now backstops the displacement orphan; this branch said the branch is unreachable. Both are true and neither implies the other, so #481's paragraph is kept verbatim and the verdict is layered underneath it: the purge bounds the harm whatever happens, the proof says the case does not arise, and the backstop is therefore genuine belt-and-braces rather than the only thing standing between that branch and an orphan. TWO auto-merges that were WRONG and needed hand repair - the reason a clean auto-merge was not taken as a resolved merge: - ProcessingShard.addWorkContainer. Both blocks survived, but the cleared- suspicion text asserted that each staleness transition is "followed by the sweep that removes it from BOTH structures". #481 makes that false - the callbacks remove from the shards only. Rewritten to say what the argument actually needs, which is the SHARD removal: a container the sweep unseated is not a resident, and the displacement branch needs a resident to displace. The queue half was never the load-bearing part. - The inflight note. Collapsing the two vet markers to one (the tag gate permits exactly one) deleted the 2026-09-07 stamp that #481's update paragraph refers to by date. That paragraph now records what the stamp said and that it has been superseded, so its point survives its source. WHAT CHANGED IN THE WORK ITSELF - Leg five of the proof is restated in terms of residence, in the solutions write-up, the test's class javadoc and the code comment. - Arm one is renamed aRebalanceUnseatsTheResidentBeforeAnyReplacementCanArrive and now asserts the two removals in #481's order: gone from the shard inside the callback, gone from the queue after one controller pass. It asserts the one-tick bound rather than assuming the old pairing. - Arm three additionally shows the orphan it constructs does not survive one control-loop pass. That assertion deliberately does NOT attribute the removal to the purge: under a fence the last-resort stale sweep would take the same key, and this arm cannot tell them apart. A first attempt asserted the replacement would be handed out on that pass; it was refuted on the run - the fence makes the whole partition stale, so the replacement is stale on arrival too. The assertion now records that. EVIDENCE AFTER THE MERGE ShardDisplacementOrphanReachabilityTest 3/3, #481's RetryQueueRebalancePathTest 6/6, RetryQueueRequeueWindowTest 9/9, ProcessingShardStaleReplacement909Test green. bin/check-all.sh 16 passed, 0 failed. The ablation was RE-RUN on the merged tree rather than carried over, because #481 changes what those sweeps do and an ablation measured against the old shape would be evidence about code that no longer exists. Same result: either sweep alone green, both removed red at the load-bearing assertion. Co-authored-by: Claude Opus <noreply@anthropic.com>
…rom the other side #468 merged to master (7c95b75) between this branch's #481 merge and now. It is the defect class this work's sweep reports on, so leaving it unmerged would have published a sweep naming an open PR as the owner of candidates that PR had already settled. WHAT CONFLICTED ONE conflict, in ProcessingShard.getWorkIfAvailable's last-resort stale sweep, where both sides rewrote the same comment. Neither side was a superset: - #481 replaced the "orphans it forever / the load gate reads low" claim, which its purge makes false, with the purge that now collects the entry. - #468 added why the removal is conditional on the inspected container, and a dated CLEARED SUSPICION for this site. Resolved by keeping BOTH - #481's purge sentence in place of the false claim, and #468's conditional-removal reasoning and cleared suspicion after it. Taking either whole side would have silently dropped the other's content, and a green build would not have shown it. THE RESULT WORTH RECORDING #468's cleared suspicion at that sweep rests on exactly the fact leg one of this work's proof rests on: addWorkContainer is the ONLY writer of workMap outside tests and runs on the controller. Two people reached the same single-writer discriminator from opposite directions, days apart, neither citing the other. That means the two clearances share ONE reopening condition - anything that puts into a shard off the controller thread - and nothing gates it. Said at all four places that now depend on it: both code comments, the solutions write-up, and the test's class javadoc, so a future change cannot invalidate one while reading only the other. CITATIONS BROKEN BY THE COMBINATION, AND REPAIRED #468 retired docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md when it merged; #481 cites that note in ShardManager.onFailure's javadoc and in its solutions write-up. Neither PR is wrong on its own and neither would have failed its own gate - the dangling reference exists only in the merge. Both repaired the way docs/citations.md and check-file-refs.sh prescribe, by pointing at the history that still holds the file (git show 7c95b75^:docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md). EVIDENCE ShardDisplacementOrphanReachabilityTest 3/3, RetryQueueRebalancePathTest 6/6, RetryQueueRequeueWindowTest 9/9, ProcessingShardStaleReplacement909Test 4/4, ShardManagerStaleContainerTest and ShardAvailableCountOwnershipTest green - 31 tests, 0 failures. bin/check-all.sh 16 passed, 0 failed. The ablation was re-run a THIRD time, on this tree. #468 changes each sweep removal into a compare-and-remove, so the previous run was again evidence about a shape that no longer exists. Same result: either sweep alone green, both removed red at the same assertion. The write-up now records all three runs rather than the latest one. Co-authored-by: Claude Opus <noreply@anthropic.com>
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…e orphan caveat is narrowed #484 refutes the hypothesis behind the "reset to earlier offset" branch of confluentinc#546 with a one-term experiment whose predictions held on both arms: the branch produces duplicates by construction, never loss, and the race the note proposed cannot happen because the rebalance listener runs strictly before the client resolves fetch positions. What remains under #162 is the false-truncation WARN, misdirection that wants an owner decision on message and level. The same PR narrows #483 caveat on the shard orphan: a non-stale resident makes a replayed record dropped, not displaced, so the window needs a fenced-but-unswept resident plus an in-generation replay of its offset - a route the bootstrap path cannot take and main has no seek for. Both agents were right about their own claims; the burn-down now states the intersection. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
One conflict, inherited from the #481 tip this branch carries: the residual-failures note #481 had appended to was deleted by #473, which migrated it to docs/solutions/test-flakiness/. The note stays deleted here; the dated update that followed it lives on #481 as a citation note in the write-up and arrives with that PR. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. No quarantined test changed outcome since the previous push. Updated for |
…s now canonical #481 landed on master as c2d35c0. This branch carried #481's original branch commits, so every region reconciled by hand last time came back as a conflict between #481-as-merged and #481-as-this-branch-had-it. Master wins those; only the reachability verdict, the test and the cleared-suspicion comment are layered on top. FIVE HUNKS, AND THE RULE APPLIED TO EACH - ShardManager.onFailure javadoc, the citation to the note #468 retired - MASTER. Both sides had repaired the same dangling reference the same way; master carries the wording added when #481 merged master. - The identical citation in docs/solutions/runtime-errors/retry-queue-write-lock- on-the-rebalance-path.md (add/add - the file is new on both sides) - MASTER, same reason. That file is now byte-identical to master. - ShardManager.onFailure javadoc, the paragraph after "What else the purge now backstops" - OURS. Master has #481's paragraph and nothing else there; this side is the verdict layered under it, which is the content this branch adds. - The inflight note's closing section - OURS. Master ends at "Production reachability is still not established"; this side answers it. - The inflight note's #431 paragraph - OURS, and this is the ONE hunk where master's text is not kept, so the reason is worth stating. Master says "The 2026-09-07 stamp ABOVE says ...". That stamp is not above it here: this branch replaced it with the 2026-09-08 PROPOSED marker, because bin/check-inflight-tags.sh refuses a note carrying two. Keeping master's sentence would leave a pointer to something the merged file does not contain, so this side's wording stands - it makes #481's point, and records what the superseded stamp said so the supersession loses nothing. THE TWO AUTO-MERGES THAT WERE WRONG LAST TIME, RE-CHECKED Both correct this round, because the corrections were already committed rather than being made against #481 for the first time: - ProcessingShard's cleared-suspicion block still reads "the sweep that takes it OUT OF ITS SHARD" and not "from BOTH structures" - the wording #481 falsifies. Its diff against master is exactly this branch's own block and nothing else. - The note carries exactly one inflight-vetted marker. NOT A LOSS, THOUGH IT LOOKED LIKE ONE A sweep for files differing from master that this branch never edited flagged #482's five (BatchTestMethods, CoreBatchTest, KafkaTestUtils and two docs). Cause: #482 landed on master DURING this resolution, so MERGE_HEAD is c2d35c0 and #482 is simply not in this merge. Nothing was dropped; it arrives in the next one. Recorded because "file differs from master and I never touched it" is normally the signature of a silent merge loss, and here it was not. EVIDENCE ShardDisplacementOrphanReachabilityTest 3/3, RetryQueueRebalancePathTest 6/6, RetryQueueRequeueWindowTest 9/9, RetryQueueTest 7/7, RetryQueueIteratorConfinementTest 5/5, ProcessingShardStaleReplacement909Test 4/4, ShardManagerStaleContainerTest 3/3 - 37 tests, 0 failures. bin/check-all.sh 16 passed, 0 failed. Co-authored-by: Claude Opus <noreply@anthropic.com>
…ent-orphan-reachability
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 2m 3s —— View job Review
SummaryThis is a docs/test-only PR (confirmed: no behavioural diff — the
Given those hold, the reachability argument (L1–L7 in the solutions doc) is sound: the displacement branch's Test quality
One thing worth double-checking (not blocking): arm 1's fixture Process notes
No correctness issues found. This is thorough, well-evidenced work — the proof is falsifiable (the ablation matrix shows the test can actually fail), the "what would reopen it" list is concrete and gate-checked against reality ( |
…now says so under test Addresses the one finding from the automated review of #483: the proof's leg seven cites a KEY-ordering fact (shards are partition-scoped) while every arm ran under PARTITION, so mode-independence was assumed rather than shown. THE HONEST ANSWER: INDEPENDENT, AND ONE LEG IS WHERE THE MODE IS EVEN VISIBLE Legs one to six never consult the shard key - they are about the inserting thread, the retry queue's single add site, the finality of two epoch fields, and which callbacks sweep. None of that varies with ProcessingOrder. The mode is visible only in leg seven's second half, and it is safe in every mode for one reason: a shard is partition-scoped throughout. PARTITION and UNORDERED make the shard a topic-partition, so an offset identifies one record by construction; KEY makes it topic-plus-partition-plus-key, so it is partition-scoped too. A topic-scoped key would break that half, and only that half, by letting two records collide on one offset with NO re-assignment anywhere in the story - which is the leg the whole argument leans on. SAID UNDER TEST RATHER THAN IN PROSE - The first arm is now parameterised over every ProcessingOrder, so the production-rebalance disproof runs under KEY, PARTITION and UNORDERED. - A new arm, underKeyOrderingOneOffsetOnTwoPartitionsDoesNotCollideInOneShard, covers the KEY half directly: two records sharing a key and an offset on different partitions must be held in two shards, not collide in one. - The statement is written once in the solutions write-up and once in the test's class javadoc, both naming which leg the mode touches. VERIFIED BY SABOTAGE, per docs/testing-at-write-time.md: making ShardKey's key-ordered form topic-scoped turns exactly the new arm red, on the assertion that carries the leg (expected 2 records held, got 1). Restored, and ShardKeyTest is green either way - it does not cover this property, which is why the new arm is not redundant with it. ONE MODE DIFFERENCE, NAMED SO IT IS NOT MISTAKEN FOR A GAP Under KEY, removeShardIfEmpty garbage-collects an emptied shard where the other two keep the object. That makes the conclusion stronger under KEY, not weaker: a collected shard holds no resident, so there is nothing to displace. The residentAtOffset helper now treats "no shard" and "no resident" as the same answer, which is the only question the arms ask. Six tests, 0 failures. bin/check-all.sh 16 passed, 0 failed. Co-authored-by: Claude Opus <noreply@anthropic.com>
…ent-orphan-reachability
Owner
Author
|
Thanks - the ordering-mode nit was a real gap, not just a documentation one, so it is now closed under test rather than in prose. The honest answer: mode-independent, and there is exactly one leg where the mode is even visible. Legs L1-L6 never consult the shard key - they are about the inserting thread, the retry queue's single add site, the finality of two epoch fields, and which callbacks sweep. None of that varies with The mode appears only in L7's second half, and it is safe in every mode for one underlying reason: a shard is partition-scoped throughout. What changed, so the test says it rather than the prose:
Verified by sabotage, per One mode difference, named so it is not mistaken for a gap: under Also merged master since the review: #469 changes |
astubbs
marked this pull request as ready for review
September 8, 2026 04:03
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Two more lines of the burn-down land on master on 2026-09-08: the shard-displacement orphan is known unreachable (#483, whose review round widened the regression test to every ordering mode), and the fifth 857 item is the group protocol (#486, carrying the PROPOSED close for the owner). The release-note draft said #408 adds the decline. It no longer does: #466 refuted declining as the fix by experiment, so the decline is only the deadline fallback, and what #408 now carries is the measurement of the bound and the held decline seam - the reading its reconciliation with master recorded today. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
The replay queue in the lead paragraph is drained into PRs, so it now points at them and at the still-open list rather than describing work as queued. Three gaps the resolved items exposed are added so the release note cannot paper over them: the per-shard liveness blind spot that #478 leaves standing, the one unconditional by-key shard removal #483's sweep reported and nobody fixed, and the flake register's remaining rows, one still undiagnosed, which gate a green master rather than the scope. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…branch is pinned by its accounting Review pass on the commit before this one. No behaviour changes except the removal of a suppression that suppressed nothing; the rest corrects claims that were wrong or incomplete, and closes two gaps where nothing would have gone red. THE ARGUMENT WAS ABOUT THE WRONG OBJECT. The cleared suspicion on removeWorkForRevokedRecord justified the staleness call's null-safety with a fact about `revokedRecord` - that it came out of the removed state's own tracking. But isWorkContainerStale resolves the state from the OCCUPANT (pm.getPartitionState(workContainer)), and in the only branch that asks, the occupant is by definition a DIFFERENT registration. What actually closes it is that every ShardKey variant is partition-scoped - KeyOrderedKey holds a TopicPartition, TopicPartitionKey is one - so an occupant of this shard necessarily belongs to the revoked partition. Both legs are now stated, and the reopener list gains the one that follows: A TOPIC-SCOPED SHARD KEY, which would admit an occupant from a partition that may never have been assigned, and which reopens addWorkContainer's cleared suspicion at the same time. That entry already named it; this one did not. A STRONGER UNREACHABILITY ARGUMENT WAS SITTING IN THE SAME PARAGRAPH, UNUSED. resetOffsetMapAndRemoveWork installs RemovedPartitionState BEFORE it calls in, so checkIfWorkIsStale is true for EVERY occupant and the staleness leg carries all of them: on the shipped path this method is behaviourally identical to the by-key removal it replaced, and the decline branch is defence for a caller that sweeps before the swap - which is exactly what ShardManagerLincheckTest.revokeSweep does. That is structural and says nothing about threads, so it is strictly stronger than the thread-ordering argument, and the previous commit used the same fact only as the NPE discriminator without noticing what else it decided. Now stated on the method. THE THREAD CLAIM WAS HALF TRUE, IN THREE PLACES. "Both rebalance callbacks run on the broker-poll thread" is contradicted by the javadoc on the method that calls into this sweep: the close route (maybeCloseConsumer -> onLeavePrepare -> onPartitionsRevoked) runs it on the CONTROL thread, and resetOffsetMapAndRemoveWork calls that live in every commit mode. The conclusion survives - on that route the sweep and addWorkContainer are on the same thread - but the stated reason was wrong for one of the two production routes. Corrected in the test javadoc and the solutions write-up; the commit body that also carries it is squashed away, and the squash message is written against this wording. THE REGISTRATION-IDENTITY CLAIM HAD A NAMED EXCEPTION IT DID NOT NAME. 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 - and the comparison is then false about a container that IS the right one. Harmless, because the staleness leg covers it, and it is the same reopener addWorkContainer's cleared suspicion already lists. Said at the site rather than left for the next reader to find. TWO TEST GAPS, ONE OF WHICH A MUTATION WALKED STRAIGHT THROUGH. - The decline branch is a new exit that must retire nothing and release nothing, and NOTHING went red if it did. Verified by ablation rather than asserted: making the decline call population.onRetired() + excludeFromSelection(occupant) left the offset assertion GREEN and nothing else noticed - a permanent RecordPopulation deficit, which is this class's recurring defect and has no clamp and nothing to reconcile it. The arm now samples population, the shard available-counter sum and the container's own claim before the sweep and asserts all three unchanged, and that mutation is red. - aStaleContainerFromAnotherRegistrationIsStillSweptOut asserted only that the occupant came from another registration, never that it was STALE - the property the arm exists to pin - leaning on the fixture's epoch instead. It now asserts staleness directly, the way its sibling arm asserts liveness, and its closing assertion gains the message it was the only one in the file to lack. REMOVED: @SuppressWarnings("ReferenceEquality"). Measured by deleting it and asserting 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; a suppression that suppresses nothing teaches the next reader something untrue. The reason it is absent is now written where it was. removeWorkAtOffset's javadoc still opened "From the onPartitionsRemoved callback", the caller the previous commit moved. It now says what it is - a test-only modelling primitive with no production caller - and names which of the two conditional removals a main-code site should reach for instead. The docs/inflight/ section is cut to a pointer. It restated the mechanism, which the solutions write-up owns, and that directory's rule is to track what is OPEN: the item is closed, so what belongs there is only that a reader arriving at #483's four-item list can see which one moved. ABLATION MATRIX, re-run against the strengthened arms - one arm red per ablation, no other: whole guard removed -> the displacement arm; staleness leg removed -> the stale-other-registration arm; identity leg removed -> the live-same-registration arm; decline retires the occupant -> the new accounting assertion. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…s red control comes first (#491) Closes the COMMIT half of the per-shard liveness gap that docs/inflight/test-per-shard-liveness-has-no-gate.md has owned since the Class 2 lag bound was demoted on 2026-08-25, in the order that note demands: the red control first, the detector only after it. WHAT WAS UNCOVERED. INSTANCE_STALL is per-INSTANCE and is re-armed by any successful work result, so an instance whose other shards keep completing never fires it, however long one partition's commits have been frozen. CLASS2_STALL is the only detector that looks at a partition, and since the demotion it does not gate - because elapsed stagnation is crossed just as readily by a slow record as by a wedge. The correctness ledger does not close it either: it counts records PROCESSED, not offsets durably COMMITTED, so a run balances while leaving exactly the uncommitted offsets a restart would redeliver. THE RED CONTROL, and it is a real engine rather than a script. WedgedPartitionRedControlIT runs one PC over three partitions with a LongPollingMockConsumer that ANSWERS every commit in full and APPLIES only the partitions that are not black-holed - a coordinator that acks and drops, the defect class #470 fixed inside PC. Broker-free, no wall-clock wait on any bound, three arms in one JVM: commits black-holed broker committed 0 PC local next-to-commit 60 FIRES healthy broker committed 60 PC local next-to-commit 60 silent pinned by an broker committed 9 PC local next-to-commit 9 silent incomplete record Every existing gating detector is shown green on the red arm mechanically, not by argument. All 180 records reach the user function, so NO_PROGRESS cannot fire and the ledger balances; the instance completes work throughout and holds nothing at the end, so INSTANCE_STALL re-arms - replayed across twenty times its bound, with an ARMED control that fires at the same spacing so the silence cannot be vacuous. Class 2 observes the frozen watermark and does not gate. THE SIGNAL, and why it is not the demoted bound renamed. UncommittedCompletionDetector compares two POSITIONS, not an elapsed time: a member's own next-offset-to-commit for a partition (offsetHighestSequentialSucceeded + 1, what PC would commit right now) against what the group has actually committed. The Class 2 false positive is excluded structurally rather than by calibration - an incomplete record pins offsetHighestSequentialSucceeded at exactly the offset the broker holds, so the difference is zero however long the stagnation runs. That is the third arm above, and it is why the two replay seeds the demotion was argued from cannot fire it. The consecutive-sample count debounces the COMMIT CADENCE - any commit that lands re-arms it - and the clock does not run while the group is rebalancing. Reachable with no main-code change: WorkManager#getPm, PartitionStateManager#getPartitionState and PartitionState#getOffsetHighestSequentialSucceeded are all public already, so the ShardManager accessor INSTANCE_STALL_BOUND's granularity note declines to add for a probe was not needed. WHAT WAS REFUTED, and it changed the design. The note's own prescription - "a watermark stagnant while completions advance and real backlog exists is the wedge" - does not discriminate as written. Completions advancing is an instance-wide fact, and the false-positive arm has completions advancing, real backlog and a stagnant watermark; a gate built on that wording would have fired on both seeds it was promised to spare. The term that had to change was the granularity of the second signal, from the instance's completions to that partition's own local watermark. WHAT IS STILL OPEN, and the note says so rather than being deleted. The signal reads a PARTITION, and any incomplete offset in it pins the local watermark - so a key-order SHARD that will never be dispatched again inside a partition still gates nothing. Separating that from a merely slow record needs per-shard in-flight state, and no red control exists for it; #483 found the nearest candidate mechanism unreachable, so it is a reachability question before it is a detector one. BOTH REPLAY SEEDS WERE RE-RUN, and they confirm it - the note's second bar. Full chaos suite per seed, on a loaded workstation (load average 19-25, other agents building alongside, which biases TOWARDS crossing a timing bound, not away): seed 6825864417772979246 10 tests 0 failures 0 violations 0 COMMIT_NOT_LANDING 30 Class 2 observations peak lagStagnation 151500ms - OVER the 150s bound peak uncommittedCompletions 3009 records seed 4044221734199516240 10 tests 0 failures 0 violations 0 COMMIT_NOT_LANDING 36 Class 2 observations peak lagStagnation 152020ms - OVER the 150s bound peak uncommittedCompletions 2757 records The lagStagnation peaks are what make this a result rather than an absence: on both seeds the Class 2 bound was genuinely CROSSED, so these are not runs where the false positive simply failed to occur. The new detector was silent through every one. The uncommittedCompletions peaks matter for the opposite reason - a healthy chaos run carries thousands of records of finished-but-not-yet-committed work at its widest, which is exactly why the detector gates on whether the committed offset MOVES rather than on how large the gap gets. ONE INSTRUMENT TRAP, created here and recorded for whoever replays next. The Class 2 interpretation text now names UNCOMMITTED_COMPLETIONS/COMMIT_NOT_LANDING, so a triager meeting a frozen watermark is told what the ABSENCE of that violation means. A log grep for the bare string therefore matches every Class 2 observation: it reads 33 and 39 on these two runs, both of which are zero. Count "COMMIT_NOT_LANDING: partition", or count "VIOLATION:". THE REVIEW'S ONE FINDING, verified before acting on it: localOffsetToCommit re-derives PartitionState#getOffsetToCommit(), which is the definition PC actually commits and could silently drift from the probe's copy. This branch's own two copies collapse into one shared helper naming the method it re-derives. The main-code half - widening getOffsetToCommit() to public, which removes the drift risk outright - is left to the owner as a docs/refactoring.md line rather than taken silently inside the PR that benefits from it, because this suite otherwise declines to widen main-code visibility for a probe. Proven by sabotage: with the detector's reporter never assigned, the red arm fails and both green arms do not. UncommittedCompletionProbeIT pins the re-arm rules a real run cannot reach on demand - a rebalance mid-stretch, a restart, a member that has lost the partition, a commit landing on the last sample before the threshold. The same-class sweep that follows a fix records its one surviving candidate - ProgressProbe#ledger's duplicate allowance is fleet-wide while redelivery is per-partition - and the five detectors it ruled out, in docs/inflight/test-duplicate-allowance-is-fleet-wide-while-redelivery-is-per-partition.md. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…ever holds the offset (#492) WHAT CHANGED FOR A USER: a rebalance can no longer remove a record that a later assignment had already re-registered. `ShardManager.removeWorkFromShardFor` - the revoke and lost path - removed by KEY with `removeWorkAtOffset(consumerRecord.offset())`, so whatever occupied the offset when the removal landed is what left the shard. It now removes only the container the revoked record was registered as, or a stale one, and never a live container from a later registration. THE DEFECT CLASS, AND WHERE THIS SITE SITS IN IT. #468 closed the same class at the poller's stale sweep: a removal keyed on the offset cannot say WHICH of two containers at that offset it meant, and the fix was to make `WorkContainer` equality identity so that `Map.remove(key, value)` is a true compare-and-remove. #483's defect-class sweep reported this site alongside it - "the one remaining unconditional by-key shard removal" - and deliberately left it, on the grounds that it belonged with #468's mechanism rather than with a reachability proof. This is that. No fork or upstream issue tracks it; it was reported in a commit body and on a `docs/inflight/` note, so this PR closes nothing. THE CALLER HOLDS NO CONTAINER, WHICH IS WHY THE FIX IS SHAPED DIFFERENTLY. The stale sweep inspects a container and then wants that one gone. This sweep is handed the `ConsumerRecord`s ONE generation was still carrying as incomplete (`PartitionState.onPartitionsRemoved` -> `removeAnyShardEntriesReferencedFrom`), so what it can name is the REGISTRATION, not an inspected container. That works because `PartitionState.maybeRegisterNewPollBatchAsWork` puts the same record instance into `incompleteOffsets` and into the `WorkContainer` it builds, in one loop body - so "the container this generation registered for this record" is `occupant.getCr() == revokedRecord`, and a later generation's re-delivery of the same offset is a different object because it came from a different fetch. That identity has one named exception, stated at the site: `addNewIncompleteRecord` puts unconditionally while `addWorkContainer` DROPS an arrival whose resident is not stale, so an in-generation replay of an already-registered offset leaves the partition naming record B while the shard holds the container over record A. The staleness condition below covers it. TWO CONDITIONS, AND THE MIDDLE OPTION WAS WRONG. Registration identity alone declines for a container from another registration that is nonetheless STALE, and declining there would leave a stale container holding an offset of a revoked partition - the state this sweep exists to prevent. Staleness alone declines for a live container built from the record the sweep was handed, which is what a revoke sweep driven before the partition's state has been swapped sees, and is what `ShardManagerLincheckTest.revokeSweep` models on every invocation: that harness's removal operation would have become a silent no-op while staying green. Verified rather than assumed - `incrementPartitionAssignmentEpoch` starts from `KAFKA_OFFSET_ABSENCE = -1`, so that harness's `EPOCH = 0L` containers are live against the epoch its constructor assigns. So the guard declines for exactly one thing - a LIVE container from another registration - which makes the change a no-op everywhere except in the defect case, and each condition has its own ablation arm. THE HARM, IF IT IS REACHED, IS THE LOST RECORD - #468's, not a misdirection. The live container is gone from the shard while its own `PartitionState` carries the offset as incomplete, so nothing selects it and nothing completes it, and the commit high-water mark cannot pass it until the partition is re-polled. The v6 scope note this came from said the cost was "misdirection bounded to one control-loop tick by #481's purge"; that is the DISPLACEMENT orphan's cost, not this one's. This removal is a shard removal, and the purge collects retry-queue entries. REACHABILITY: NOT TODAY, ON TWO INDEPENDENT ARGUMENTS, AND THE STRONGER ONE IS STRUCTURAL. - Thread ordering, which has to name BOTH production routes. On the ordinary route the poll thread runs both rebalance callbacks, so a generation's revoke sweep completes before the assignment that could register a replacement begins. On the CLOSE route the sweep runs on the CONTROL thread (`maybeCloseConsumer` -> `onLeavePrepare` -> `onPartitionsRevoked`), which `PartitionStateManager.resetOffsetMapAndRemoveWork`'s javadoc calls live in every commit mode - and that is also `addWorkContainer`'s thread, so the two still cannot interleave. "Both callbacks are on the poll thread" is the half-statement, and it was written that way here first. - `resetOffsetMapAndRemoveWork` installs `RemovedPartitionState` BEFORE it calls in, so `checkIfWorkIsStale` is true for EVERY occupant and the staleness condition carries all of them: on the shipped path this method is behaviourally identical to the by-key removal it replaces, and the decline branch is defence for a caller that sweeps before the swap. That says nothing about threads. Nothing checks either argument. The conditional form costs one reference comparison and rests on neither, which is exactly why #468 wrote `getWorkIfAvailable`'s last-resort sweep conditionally against a race it had shown was unreachable there. THE NPE GUARDS ON THIS PATH ARE UNTOUCHED, AND THE CLEARED SUSPICION NAMES THE RIGHT OBJECT. The single-read `getShard` idiom #345 put in stays where it was; the new work is inside `ProcessingShard`, below it. The staleness question resolves the partition state from the OCCUPANT, not from `revokedRecord` - and in the only branch that asks, the occupant is by definition a different registration - so the discriminator is two facts: the state is installed before the call (and a partition with no state `continue`s without sweeping at all), and every `ShardKey` variant is partition-scoped, so an occupant of this shard belongs to the revoked partition. WHAT WOULD REOPEN IT: a caller from a path that has not installed a state, or a topic-scoped shard key - the latter being the reopener `addWorkContainer`'s own cleared suspicion already lists, which reopens both at once. EVIDENCE. `ShardRevokeSweepReplacementEvictionTest`, three arms, RED-then-GREEN through `PartitionState.onPartitionsRemoved` so the argument list is built by production code - one level below the state swap, which is what makes the decline branch reachable at all. Ablation matrix, predicted before running, one arm red per ablation and no other: - whole guard removed, back to `removeWorkAtOffset(offset)` -> the displacement arm - staleness condition removed -> `aStaleContainerFromAnotherRegistrationIsStillSweptOut` - registration-identity condition removed -> `aRevokeSweepStillRemovesALiveContainerBuiltFromTheRecordItWasHanded` - decline retires the occupant instead of leaving it -> the displacement arm's accounting assertion That fourth ablation is why the accounting assertion exists: making the decline call `population.onRetired()` + `excludeFromSelection(occupant)` left every other assertion GREEN while creating a permanent `RecordPopulation` deficit, which is this class's recurring defect and has no clamp and nothing to reconcile it. TWO INSTRUMENTATION FAILURES, RECORDED RATHER THAN TIDIED AWAY. The registration-identity ablation reported NO failure on its first run: dropping the condition left an unused local, the module never recompiled, and the grep for failures matched nothing - a silent false negative in the experiment meant to prove the condition earns its place. Re-run with the build outcome asserted, it is red as predicted. Separately, `@SuppressWarnings("ReferenceEquality")` was written on the identity comparison and then measured: deleted, rebuilt under `-Pci`, `BUILD SUCCESS`. Neither `WorkContainer` nor Kafka's `ConsumerRecord` overrides `equals`, so Error Prone never fired - it suppressed nothing and disagreed with `isResident`'s bare `==` two methods away. Removed, with the reason written where it was. SAME-DEFECT-CLASS SWEEP over `ShardManager`, `ProcessingShard` and `PartitionState`. Fixed: the one above; no unconditional by-key removal of a container remains in main. Dismissed with discriminators: `ProcessingShard.onSuccess` (re-verified rather than inherited - `WorkManager.handleFutureResult` is the third staleness checkpoint and drops a stale result before `onSuccessResult`, so a returning stale container cannot remove the fresh one that replaced it); `addWorkContainer`'s by-key INSERTION (#483's leg 1, plus #336's admit-first, which reads the outcome from the map rather than the earlier read); `removeStaleWorkContainersFromShard` and `getWorkIfAvailable`'s last-resort sweep (already conditional, #468); `ShardManager.getShard` / `compute` / `computeIfPresent` (single-read Optional idiom, #345, and atomic per-key, #336 - not check-then-act); `RetryQueue.remove` / `removeAll` at every call site including `onSuccess`, `onFailure` and `purgeDepartedRetryEntries` (same shape, but it is the queue's keying model rather than a removal that forgot to name its target, so a fix is a change to that model - reported and left by #483, and left here); `PartitionState.incompleteOffsets.remove` and the truncation prune (offsets over records, control-thread-only - "this offset is complete" does not depend on which record object represents it). `removeWorkAtOffset` now has no production caller. Kept as the modelling primitive four test classes use for "a container departs its shard", with its javadoc rewritten: it used to open by naming the `onPartitionsRemoved` caller this change moved away, and now says what it is and which of the two conditional removals a main-code site should reach for instead. RECORDS. `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md` - the class's owner - gains a dated section for the second site, because its own sweep's only hit was a false positive and the real one was found a day later. `docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md` gets a pointer, not a narrative: its subject, state and `PROPOSED closed` marker are untouched, and what is recorded is only that a reader arriving at #483's four-item list can see which one moved. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -pl parallel-consumer-core -am test -Dtest='ShardRevokeSweepReplacementEvictionTest,ShardStaleSweepReplacementEvictionTest,ShardManagerRevokeSweepNpeTest,ShardDisplacementOrphanReachabilityTest'`; `bin/check-pr-analysis-surfaces.sh 492`. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…y-key removal are on master The intake-stall measurement landed with the retry-cadence arm and the interim latch warning as what remains, and its box is ticked. The last by-key shard removal the #483 sweep reported is fixed on master, red first with an ablation arm per leg of the guard. The known unknown that named it is struck through with the outcome, and the retry queue's same-shaped removal is recorded as that queue's keying model rather than a remaining instance. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Settles the one open question on
docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md:whether production can reach
ProcessingShard.addWorkContainer's displacement branch with aqueue-resident container. It cannot. No behaviour changes. What changes is that a demonstrated
pairing gap now carries a reachability verdict, a control-armed regression test, and a named list of
what would reopen it - instead of an open note nobody can price.
Its relationship to #481, now merged
That PR's own
Update 2026-09-08section on this note ended by saying production reachability wasstill not established - which is exactly what this PR answers, so the two belong in one argument
rather than two adjacent ones. It has since squash-merged to master as
c2d35c0aa; master is mergedin here and its version of that content is the canonical one, with only the verdict layered on top.
The dependency line is gone because the dependency is satisfied.
The two results are independent and both worth keeping. #481's purge
bounds the harm to one control-loop tick whatever happens; this PR says the case does not arise.
Neither implies the other, so the purge is a genuine backstop here rather than the only thing
standing between that branch and an orphan.
It also moved a leg of the argument, which is the part worth reviewing. #481
takes the rebalance callbacks off the retry queue, so they no longer pair a queue removal with the
shard removal. The proof turns on residence, not on the queue - a container the sweep unseated
is not a resident, and the displacement branch needs a resident to displace - so the leg holds, but
it had been written as "removes it from both structures" and that wording is now false. Restated
at all four places that state it. The test asserts the two removals in #481's
order: gone from the shard inside the callback, gone from the queue after one controller pass.
The argument, each leg traced in source
An orphan needs four things true at once when a container
Barrives at(topic, partition, offset):a resident
A;Aobserved stale, or the arrival is dropped;Aholding aRetryQueueentry;and
Bexisting at all.workMaphas two insertion sites -addWorkContainer, control-thread-only, and the test-onlyplantResident. Only removals caninterleave between the
getand theput, and a removal makes theputreturn null.RetryQueue.addhas exactly oneproduction caller,
ShardManager.onFailure, behindonFailureResult's live staleness check and -since fix(core): close the retry-queue orphan window by confirming shard residency after the add #437 - a post-add residency confirmation.
couldBeTakenAsWorkrefuses a stale container outright, so one that is stale on arrival is never selected, never fails,
and never queues.
final long, the fence is never cleared,a replaced state carries a higher epoch or is the removed singleton.
Amust cross the staleness boundary in between, and only three transitions can do it: theremoved-state swap in
resetOffsetMapAndRemoveWork, theputAllinonPartitionsAssigned, andfenceForRevocation. The first two each unseatAon the same thread inside the rebalancecallback. The fence carries no sweep at all - but a second container at those coordinates needs the
offset delivered twice, which needs a re-assignment, which is the swept path.
The independent corroboration #468 turned out to be
Merged to master (
7c95b75ce) while this branch was open, and merged in here. Its cleared suspicionat
getWorkIfAvailable's last-resort sweep rests on exactly the fact leg one rests on:addWorkContaineris the only writer ofworkMapoutside tests and runs on the controller. Twopeople reached the same single-writer discriminator from opposite directions, days apart, neither
citing the other.
The consequence is a shared failure mode, and it is now written down. Both clearances reopen on
one condition - anything that puts into a shard off the controller thread - and nothing gates it. Said
at all four places that depend on it, so a future change cannot invalidate one while reading only the
other.
What was refuted on the way
incrementPartitionAssignmentEpochchanges no stalenessanswer:
PartitionState's epoch isfinaland staleness is only asked through the state object.The window opens at the state replacement, which is the same statement as the sweep that closes it.
ShardKey.KeyOrderedKeyis partition-scoped and its javadoc says why.expected the replacement to be handed out on the controller pass after the fence. It is not - the
fence makes the whole partition stale, so the replacement is stale on arrival too.
Review round
The automated review came back clean - no correctness issues - with one non-blocking nit: leg seven
cites a KEY-ordering fact while every arm ran under
PARTITION, so mode-independence was assumedrather than shown. Closed under test rather than in prose (
42c449d96):the shard key. L7's second half does, and is safe in every mode because a shard is partition-scoped
throughout -
PARTITION/UNORDEREDmake it a topic-partition,KEYmakes ittopic-plus-partition-plus-key.
ProcessingOrder, and a new arm covers theKEYhalfdirectly: two records sharing a key and an offset on different partitions must be held in two
shards, not collide in one.
docs/testing-at-write-time.md: making the key-ordered shard keytopic-scoped turns exactly that arm red.
ShardKeyTeststays green under the same sabotage, whichis why the new arm is not redundant with it.
KEYan emptied shard is garbage-collected,which makes the conclusion stronger - a collected shard holds nothing to displace.
The one thing this does NOT claim
The last leg is a property of the Kafka consumer's fetch position, not of this engine. The
pairing gap is one arrival away from being real, not absent. An in-generation replay of an
already-registered offset - a
seek, or an offset-reset / truncation replay - makes it reachableimmediately, and nothing here goes red for that. Worth a look from whoever owns the offset-state
truncation replay work.
Defect-class sweep - by-key removals that can hit a different occupant
#468 has now fixed the shard side of this class with identity equality
and
ProcessingShard.evictIfStillResident, so this sweep is reported against the post-merge tree:ProcessingShard.removeStaleWorkContainersFromShardandgetWorkIfAvailable's last-resortsweep - both now conditional on the inspected container. Fixed by fix(core)!: WorkContainer equality is identity, so the stale sweep removes only the container it inspected #468.
ProcessingShard.onSuccess- stillworkMap.remove(offset), by key. fix(core)!: WorkContainer equality is identity, so the stale sweep removes only the container it inspected #468reasoned about it explicitly and left it, on the grounds that the container it removes cannot have
been replaced. Checked, and left to that argument rather than re-opened here.
ShardManager.removeWorkFromShardFor- the revoke path'sremoveWorkAtOffset(record.offset()),on the poll thread. The one remaining unconditional by-key shard removal. Reported, not fixed -
fixing it belongs with fix(core)!: WorkContainer equality is identity, so the stale sweep removes only the container it inspected #468's mechanism, not with a reachability proof.
RetryQueue.remove/removeAll- by(topic, partition, offset)and never by identity, atevery call site. Deliberate and documented, but it does mean a removal can clear a replacement's
entry:
ShardManager.onSuccessremoves by key before it looks up any shard, so a late success fora departed container clears a fresh one's parked-for-retry entry. The cost is an early retry, not a
stall or a loss. Noted, not raised as a defect.
ProcessingShard.addWorkContainer's insertion - checked and dismissed: single inserting thread,so the class does not apply. This is leg one above.
v6 recommendation
Not a v6 blocker, and it can come off the list. The note is left OPEN with a
PROPOSED closed - unreachable AND boundedvetted marker rather than closed here, because its impactis
misdirectionanddocs/inflight/AGENTS.mdreserves that state change for the owner.Checklist
docs/solutions/write-up owns the proof; the inflight note is corrected and carries the PROPOSED marker; the cleared suspicion is onaddWorkContainerand cross-referenced fromShardManager.onFailureand the last-resort sweepdocs/features/- N/A - no user-facing behaviour changesShardDisplacementOrphanReachabilityTest: four arms, six cases (arm one parameterised over everyProcessingOrder), an ablation matrix re-run after each merge, and the KEY-ordering arm verified by sabotagedocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - this PR resolves an existing note (bug-shard-displacement-orphans-the-retry-queue-entry.md) and carries nothing elseghcannot showce-simplifyandce-code-reviewlocally - N/A -ce-debugwas the workflow here, and the review is better spent as@claude review thison the PR, which is the only route that opens inline threads