Repository navigation
fix(core): close the retry-queue orphan window by confirming shard residency after the add - #437
Merged
Merged
Conversation
…sidency after the add WorkManager.onFailureResult re-validates staleness against the LIVE partition map immediately before sm.onFailure(wc), and its own comment said no epoch check there can ever be atomic with the actions that follow. A rebalance completing in that gap on the broker-poll thread left a queue-only orphan: the revoke sweep removed the container from its shard and then from the retry queue, finding nothing there yet; the controller then added it. Under PARTITION or UNORDERED ordering the emptied shard object survives - only KEY ordering garbage-collects one - so the shard-present guard passed. What that costs a user: work is handed out by scanning shards, so a container in no shard is never selected, never completed and never swept, and every route that removes a retry-queue entry reaches it through shard contents. The entry is there for the life of the instance. Once its retry delay elapses it survives the flooring in getNumberOfWorkQueuedInShardsAwaitingSelection on a drained pipeline, so isRecordsAwaitingProcessing() reads true forever and AbstractParallelEoSStreamProcessor.drain() never transitions to closing - A DRAINING CLOSE HANGS TO ITS TIMEOUT on an instance with no work in it. The recorded consequence was wrong, and is corrected here. Both the comment at onFailureResult and the in-flight note raised on #431 said the harm was the broker-poller load gate being misled via ShardManager.getWorkableRecords(). Measured, that does not hold in either half: parkedForRetry is queueSize - readyToRetry, so the orphan's contribution is exactly zero once its delay passes, and while it is non-zero it makes the figure read LOW, which fetches sooner rather than stalling. RetryQueueRequeueWindowTest#aQueueOnlyOrphanCostsTheDrainFigureAndNotTheLoadGate measures both halves so the claim cannot be re-derived. The fix, and why the obvious one was rejected. A residency test BEFORE the add only narrows the window - it is another check-then-act, and the sweep lands between that answer and the add exactly as it lands between the epoch check and the call. Moving the shard map and the queue under one lock would close it but puts the poll thread's revoke path behind the retry queue's fair lock, reversing what #431 established deliberately. Instead ShardManager.onFailure adds and THEN confirms residency, undoing the add if the container has left: the last action is a removal driven by a read taken after the add, and the sweep's own action is also a removal, so at least one of the two always observes the entry. All four interleavings settle with the pair whole; the argument is on the method. No lock added, nothing waits, and the poll thread's side is untouched. This is the shape ProcessingShard.includeInSelection already used for the selection claim, and the residency predicate is now shared as ProcessingShard.isResident. Evidence. RetryQueueRequeueWindowTest drives the interleave deterministically by overriding the live epoch check and running the full production revoke path inside it; every arm asserts that the race fired and that the check answered not-stale, so an arm that stops exercising the window fails rather than passing vacuously. Predictions were stated before the run and all held, including the refutation: 3 of 6 arms red on master (the two window arms and the drain arm), the load-gate characterisation and both control arms green. Control arms are same-magnitude-different-position - the same rebalance one seam earlier is caught by the live check, one seam later leaves the pair whole. Ablation: with the fix all six pass; removing only the post-add residency undo turns exactly those three red again and no others. Core unit suite 686 run / 0 failures / 8 skipped, whole-reactor unit build green, bin/check-all.sh 16 passed 0 failed. Defect-class sweep. Sites that remove a container from a shard were checked for a paired queue removal: ProcessingShard.onSuccess (paired by ShardManager.onSuccess), removeWorkAtOffset via removeWorkFromShardFor, via removeStaleContainers, and via getWorkIfAvailable's last-resort branch are all paired. One is NOT: ProcessingShard.addWorkContainer's displacement branch, which cannot remove from a queue the shard holds no reference to. The pairing gap is demonstrated; production reachability is not established, and that open question decides its urgency. Recorded at the site and in docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md, not fixed here. Records. The solved problem is written up in docs/solutions/runtime-errors/retry-queue-orphan-window-between-the-requeue-check-and-the-add.md. The in-flight note that tracked this defect lives only on #431's branch and should be deleted when either lands, since the knowledge now has a durable owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd pin what #431 must add Review raised that the four-interleaving argument on ShardManager.onFailure reads as ordering-independent when it is not, and that #431's queue-first sweep defeats it. Both are correct. Verified by demonstration rather than by reading the diff. The hole. #431 reverses the sweep's two removals - it asks the queue first, because a declined lock must abandon the paired shard removal so the pair never splits. RetryQueue.tryRemove returns true whenever it acquired the lock, present or not, deliberately: "there was no entry" and "I removed the entry" are the same answer to the caller's question. So the sweep's queue removal passes over an empty queue, the controller then adds and reads residency while the container is still resident, and the shard removal happens afterwards - neither party removes the entry. That is the orphan this fix exists to prevent, through the few instructions between the sweep's two steps, which is exactly the widening the original note attributed to that PR. Why the fix still stands on master, and what closes the rest. Master is shard-first, which is what makes the departure observable to the residency read BEFORE the sweep's queue removal, and every case in the argument turns on that. #431 needs to repeat the queue removal AFTER the shard removal: that closes the half where the controller's add lands inside the sweep, while the residency confirmation closes the half where the add arrives after the sweep has finished. Neither half is redundant and neither closes the other. No lock is added on either side, so nothing here reverses #431's decision to keep the broker-poll thread out of a wait. Evidence, and the instrument that stops the two changes passing each other silently. Two arms run the sweep's two removals AROUND the controller's re-queue, changing nothing but the order of those two removals: aQueueFirstSweepDefeatsTheOneShotConfirmation asserts the orphan APPEARS - red-by- design documentation, green while master is shard-first - and its matched control aShardFirstSweepIsCaughtByTheConfirmation asserts it does not. Same magnitude, different position, so the ordering is established as the responsible term rather than assumed. When #431 lands with the paired removal, that assertion has to be consciously inverted. Records. The javadoc now states the precondition instead of implying none; the solutions write-up carries the interaction; and docs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard- removal.md carries the coordination from #431's side, naming who owns the retirement. Also corrected: the original note described #431's declining sweep as shipped. It is an open draft and master is still shard-first. The defect was reproduced on master regardless, so nothing about its reachability depended on the wrong version. Core unit suite green, whole-reactor unit build green, bin/check-all.sh 16 passed 0 failed - two gates caught real problems on the way and are fixed here: bare #431 refs, and a "this branch" self-reference in the new note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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)
|
[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. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #437 +/- ##
============================================
+ Coverage 81.84% 82.40% +0.55%
- Complexity 1462 1474 +12
============================================
Files 95 95
Lines 5133 5148 +15
Branches 500 502 +2
============================================
+ Hits 4201 4242 +41
+ Misses 732 710 -22
+ Partials 200 196 -4
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:
|
🟢 Throughput — OKThis branch measured about 9% 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 9 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: e20e898, 9999144, cc36b64, 1941cdf, 05c02bb, 5706686, c668acb, fb5ea93, 6573781 Since the previous push: ratio 0.726 -> 0.91, share 2.215 -> 1.769, rate 27732 -> 64321 (+131.9%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
The dups: similarity check flagged one clone introduced by this PR - 19 lines shared between RetryQueueRequeueWindowTest and WorkManagerStaleCheckDoubleLookupTest. Real, and exactly the case the report exists for: the new test copied the existing racing-double harness rather than extending it, which is the "reuse test utilities - search before you add" rule going the wrong way. RacingSeamWorkManager now holds the part that was duplicated - the one-shot arming mechanism (arm, raceHasFired, fireOnceIfArmed) and the constructor. Each subclass keeps only what actually differs, which is its seam: the pre-existing double overrides checkpoint 3's two-argument staleness lookup, the new one overrides the live single-argument check and adds its own live-check bookkeeping. This edits WorkManagerStaleCheckDoubleLookupTest, a pre-existing master file this branch had not otherwise touched. That is the right place for the fix rather than a second copy, but it is worth knowing when reading the diff. The change is mechanical: no assertion, arm or scenario altered on either side, and the reasoning for keeping the fired flag separate from the armed slot moved onto the base class rather than being deleted - a cleared armed slot cannot tell "armed, then fired" from "never armed", so a precondition built on it would pass on a test that forgot to arm. Not deduplicated, deliberately: the module-construction block (PCModuleTestEnv + MockConsumer + PARTITION ordering) is pre-existing across nine test files on master, so it is base-branch duplication and out of this PR's scope. Both classes green with the same counts as before (8 and 5). Core unit suite 688 run / 0 failures / 8 skipped - unchanged, so nothing was lost in the move. Whole-reactor unit build green; bin/check-all.sh 16 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
This was referenced Sep 5, 2026
…ueue-orphan-window
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
6 tasks done
Owner
Author
|
@claude review this |
Owner
Author
|
@codex review |
|
Claude finished @astubbs's task in 4m 59s —— View job Review
SummaryThis is a careful, well-reasoned fix. I traced the core change and it holds up. The fix itself (
All three match the required ordering, so the confirmation is sound against current master. The Verified independently, not just read:
One thing I chased and ruled out, worth recording since it's exactly the kind of subtlety this PR is about: the undo path ( Scope discipline is good. The two things this PR explicitly does not fix - the Nothing here blocks merge. No correctness issues found in the shipped fix; the two known-open items are correctly scoped as follow-ups, not regressions. |
…ueue-orphan-window
💡 Codex Review Couple the queue-first arm to the production sweep
When #431 switches production to queue-first without the required trailing removal, this test will remain green: AGENTS.md reference: AGENTS.md:L361-L368 Record the cleanup removal result
AGENTS.md reference: AGENTS.md:L402-L407 Record the synthetic sweep results
Both synthetic sweep actions silently discard meaningful return values: the queue removal reports whether an entry existed, while the shard removal returns the container actually displaced. These values distinguish the intended interleaving from a no-op or wrong-container setup, so name them and state or assert why their outcomes are expected instead of relying on bare calls. AGENTS.md reference: AGENTS.md:L402-L407 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ifecycle, not permanent Review of the sibling defect this PR records but does not fix found its consequence over-claimed, and the over-claim repeated at the code site. Verified against this branch's head rather than read off the note. WHAT WAS WRONG. Both docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md and the KNOWN GAP comment at ProcessingShard.addWorkContainer said the displaced container's retry-queue entry is one "nothing can ever remove", holding a draining close open to its timeout - i.e. exactly the consequence of the window this PR fixes. WHY IT IS NOT. RetryQueue keys by topic, partition and offset alone (WorkContainerKey.of), never by container identity, and ShardManager.onSuccess removes by that key UNCONDITIONALLY, before it touches any shard. The container that displaced the stale one carries the same three coordinates, so the replacement's own first terminal event clears the entry: success removes it; failure re-adds the same key, and RetryQueue.add replaces the existing entry rather than duplicating it, so what is left is an ordinary retry entry; a revoke or stale sweep that finds the replacement in the shard removes it by that key too. The window is bounded by the replacement's lifecycle, not by the instance's, so nothing here holds a draining close open. WHAT IS STILL TRUE, and why the note stays open: the surviving entry carries the DISPLACED container's retry-due time, so the ready-to-retry count and RetryQueue.getLowestRetryTime read one entry high until that terminal event. A briefly wrong figure, so the note's impact tag moves from stall to misdirection. The demonstrated pairing gap and the "production reachability is NOT established" section are unchanged - the gap is real, only its price was overstated. This is the same shape of over-claim this PR corrects at WorkManager.onFailureResult: a bounded, self-clearing cost written up as a permanent one. Two in one area is a pattern, so the correction says so at both sites rather than only fixing the words. ALSO CORRECTED, found while checking the above: WorkManagerStaleCheckDoubleLookupTest's class javadoc still named the broker-poller load gate as where the re-queue orphan is paid for. That is the claim this PR measured and refuted, sitting in a file this PR already edits - so the PR's own claim that the wrong consequence "cannot be re-derived" was not true while a live copy of it stood in the test suite. ALSO FLAGGED: #431's branch hands the RetryQueue to removeStaleWorkContainersFromShard as well as to getWorkIfAvailable, so the note's and the comment's "passed in per-call to getWorkIfAvailable and nowhere else" goes stale when that PR lands. Recorded in both places; #431 owns the rest of what that changes. Confirmed while here, because the note asserts it: ProcessingShard.retire's javadoc does state the pairing invariant, and it does cover only the population and the selection claim. Core unit suite green, bin/check-all.sh 16 passed 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
…nd name the discarded removal results Three P1s from Codex review on the current head, all upheld. COUPLE THE SWEEP-ORDERING ARMS TO PRODUCTION. The two modelled arms hand-build the sweep's two removals - one of them models an ordering production does not have yet - so neither can go red when production's ordering changes. The claim that they stop this PR and #431 passing each other silently was therefore resting on a future author remembering to rewrite a test, which is "enforce the invariant, do not document it" going the wrong way. The arm's javadoc said it would "fail loudly at that moment"; it would not, and now says so. theProductionSweepsQueueRemovalIsGatedOnItsShardRemoval supplies what those arms cannot. It drives the REAL revoke sweep (WorkManager.onPartitionsRevoked -> ShardManager.removeWorkFromShardFor) across two partitions: one container still resident in its shard, one that has left it while keeping its queue entry. The resident one is the arm's own control - without it, "the departed one's entry survived" would also pass on a sweep that never ran at all. What it asserts is the single property of the sweep that the confirmation on ShardManager.onFailure depends on: the queue removal is reached only through a non-null shard removal, which is only true of a shard-first sweep. Its discriminating power is MEASURED, not asserted. With a queue-first removal spliced into removeWorkFromShardFor - modelling what #431's tryRemove(topic, partition, offset) does - exactly this arm goes red and no other, its message naming the paired removal that PR has to add. Ablation reverted; nothing of it remains in the tree. NAME THE DISCARDED REMOVAL RESULTS. ShardManager.onFailure's undo dropped RetryQueue.remove's answer, which cannot be told from a forgotten check. It is now LOGGED, and the log line says which of the two racing removals won - the only interesting thing about that branch, since both answers are correct and neither is actionable. Logged rather than assigned to an ignored local deliberately: a dead store in MAIN code trades an Error Prone finding for a SpotBugs DLS_DEAD_LOCAL_STORE one, which docs/inflight/static-error-prone-rule-registry.md already records as tried and rejected at another site. That rule is excluded for test classes, which is why the same treatment is fine inside the test. In the test's synthetic sweep, the shard removal's return is now ASSERTED to be the container under test - a null or a different instance there means the arm arranges no departure and everything after it is vacuous - and the queue removal's is named with why it is deliberately not asserted: whether an entry was present depends on which position the runnable is in, and both answers are the modelled behaviour. Core unit suite 712 run / 0 failures / 0 errors / 8 skipped, the new arm being the one addition. bin/check-all.sh 16 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
Owner
Author
|
@codex all three upheld and addressed in 3a2e331. Replying here rather than in-thread because the review posted as a single comment with no review threads attached, so there is nothing to reply into. 1. Couple the queue-first arm to the production sweep — agreed, and it was the sharpest of the three. You are right that New arm Discriminating power measured, not asserted: with a queue-first removal spliced into 2. Record the cleanup removal result — agreed, with one deviation worth stating. 3. Record the synthetic sweep results — agreed. The shard removal's return is now asserted to be the container under test (a null or a different instance means the arm arranges no departure and everything after it is vacuous). The queue removal's is named with why it is deliberately not asserted: whether an entry was present depends on which position the runnable is in — queue-first finds nothing because the controller has not added yet, which is the modelled behaviour, not a fault. Core unit suite 712 run / 0 failures / 0 errors / 8 skipped; 🤖 Generated with Claude Code |
|
To use Codex here, create an environment for this repo. |
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
Owner
Author
|
@claude review this Re-review requested because the head moved after your last pass ( Please look hardest at the new arm: whether its two-partition fixture really establishes what it claims, and whether the resident-container control is sufficient to rule out a vacuous pass. |
|
Claude finished @astubbs's task in 4m 6s —— View job Review
SummaryRe-reviewed at The call path claim in the javadoc is accurate, though compressed. The comment says this arm drives The fixture genuinely exercises the gating property, not a stand-in for it:
The resident-container control earns its place. Without Ablation claim checks out by inspection. If Spot-checked the two other Codex P1 fixes from the diff, both match the PR body's description:
Nothing found that blocks merge. The new arm establishes what its javadoc claims, the control is not decorative, and I did not find a vacuous-pass path through it. |
astubbs
marked this pull request as ready for review
September 7, 2026 00:28
…nstance The same over-claim 490d589 corrected on the displacement sibling was still standing in this defect's own write-up, in the body and in a frontmatter symptom: that every route removing a retry-queue entry reaches it through shard contents, so nothing could ever take it out again, and it is there for the life of the instance. VERIFIED AGAINST THE CODE, not read off the sibling. RetryQueue keys by topic, partition and offset via WorkContainerKey.of, never by container identity, and ShardManager.onSuccess calls retryQueue.remove(wc) as its FIRST statement - unconditionally, before it looks up any shard. That is a removal route which does not go through shard contents, so any later container at those three coordinates clears the entry. THE CONSEQUENCE IS UNCHANGED, and the reason is worth stating rather than leaving implicit. The orphan is created by a revocation, so nothing arrives at those coordinates again until the partition is reassigned to this instance - and a draining close is precisely the window in which no reassignment can occur. The drain still hangs to its timeout. What changes is the bound: an instance that keeps running and is given the partition back reaches the entry and clears it. Recorded as a correction rather than an edit, because the strong version is the plausible-sounding one and a later reader who re-derives it should find out here why it does not hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for |
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…pair with Base movement, 19 commits. #437 (440b9bd) closes the retry-queue orphan window by confirming shard residency AFTER the re-queue's add, and #433 (9999144) declares `RetryQueue.RetryQueueIterator` `@ThreadConfined(ANY)` with an `assertOnOwningThread` at every entry point. Both land in files this branch rewrites. NOTHING CONFLICTED, which is the case to be suspicious of rather than pleased about - the two sides touch adjacent regions of the same four files and git merged them without asking. Checked by hand afterwards, both sides' intent is present and distinct: - `RetryQueue`: #433's `@ThreadConfined(ThreadConfined.ANY)` on the iterator type plus its three `assertOnOwningThread` guards, AND this branch's `@ControllerThreadOnly` on `remove`/`add`/ `removeAll`/`clear`. They answer different questions - one is consumed by RacerD and paired with a runtime assertion, the other is read only by `ArchitectureTest.rebalanceCallbacksMustNotBlock` - and the marker's own javadoc says why it is deliberately not the Infer annotation. - `ShardManager`: #437's post-add `isResident` confirmation in `onFailure`, AND this branch's queue-first `tryRemove` in `removeWorkFromShardFor`. - `ProcessingShard`: #437's `isResident` predicate and its use in `includeInSelection`, AND this branch's `removeStaleWorkContainersFromShard(RetryQueue)`. THE RED IS EXPECTED AND IS THE NEXT COMMIT'S STARTING EVIDENCE. `RetryQueueRequeueWindowTest.theProductionSweepsQueueRemovalIsGatedOnItsShardRemoval` fails at its final assertion - "PRODUCTION IS SHARD-FIRST, and the confirmation in ShardManager.onFailure depends on it". That arm exists precisely to go red the moment production stops being shard-first, and this branch is what stops it. `RetryQueueRequeueWindowTest` 9 tests / 1 failure. Its sibling `aQueueFirstSweepDefeatsTheOneShotConfirmation` is GREEN, and that is not a reprieve: it hand-builds both orderings, so production moving cannot move it - the over-claim review caught on #437 and the reason the production arm was added. It still has to be re-stated against the paired removal. Everything else green on the merged tree: `ArchitectureTest` 3/0/0/0, `RetryQueueTest` 10/0/0/0, `RetryQueueRebalancePathTest` 6/0/0/0, `RebalanceCallbackRuleControlTest` 2/0/0/0, `RetryQueueIteratorConfinementTest` 5/0/0/0. `bin/check-branch-self-reference.sh` is red on 12 references in three notes that arrived from master and name #431 in the future tense - the note addressed to this PR, the shard displacement note, and the Infer `@ThreadSafe` note. All three are the next commit's work, because what they should say depends on what it does. 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 7, 2026
…shard removal #437 closed the re-queue orphan window by adding to the retry queue and THEN confirming the container is still shard-resident, undoing the add when it is not. That argument holds against a sweep that removes from the shard first and the queue second, which is what master did. This branch's sweeps ask the queue FIRST so that a refused lock can abandon a removal it has not started - and against that ordering a one-shot confirmation is defeated: the sweep's ask passes over an empty queue, the controller adds, its residency read still sees a resident container because the shard removal has not happened yet, and the shard removal then happens. Neither party removes the entry. That is the queue-only orphan #437 closed, back through a different door, and `docs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard-removal.md` was addressed to this PR saying so. WHAT CHANGED FOR A USER: nothing new that #437 did not already give them - a failed record whose partition is revoked mid-re-queue still cannot strand a retry-queue entry that no scan reaches, and so still cannot hold a draining close open to its timeout. This keeps that true now the sweeps decline. THE SECOND ASK, AND WHY A REFUSAL CANNOT ABANDON. Both broker-poll-thread sweeps - `ShardManager.removeWorkFromShardFor` and `ProcessingShard.removeStaleWorkContainersFromShard` - now ask the queue again once the container has left the shard. By then the first ask's option is gone: abandoning would leave exactly the orphan the first ask exists to avoid. So a refused second ask puts the container BACK, through the new `ProcessingShard.removeWorkAtOffsetPairedWith(offset, paired)` - population re-admitted, selection claim restored, `putIfAbsent` so a container that took the offset meanwhile is not displaced. What is left behind is then a whole stale pair, which is the state the engine tolerates and the controller's own sweep retires, rather than a half nothing reaches. The undo lives inside the shard because only the removal knows the one fact it needs: whether that retirement was the call that took the selection claim. It was not, if the record was already out at a worker - and asking the container afterwards cannot tell the two apart, which is the same unanswerable question `removeWorkAtOffset` records having been fixed to stop asking. `retireReversibly` reports it; `excludeFromSelection` now returns whether it won its compare-and-set to say so. REJECTED, and recorded at the site: ask the queue only ONCE, after the shard removal, and put the container back on a refusal. That restores master's shard-first ordering and needs no second ask - simpler on the page. It moves the undo from the rare path to the common one: refusals are frequent exactly when the controller holds the read lock for a whole scan, which is the contention this fix exists to survive, whereas a SECOND ask is only refused when the lock was free a moment earlier and has been taken since. THE SAME WINDOW AT THE OTHER SITE, SETTLED FROM THE INTERLEAVINGS RATHER THAN BY ANALOGY. `ProcessingShard.removeStaleWorkContainersFromShard` is the epoch-change sweep, reached from `onPartitionsAssigned` as well as `onPartitionsRemoved`. All three of the ingredients `ShardManager.onFailure` needs are present on it: both callers increment the assignment epoch BEFORE calling it, so a container that answered "not stale" to `WorkManager.onFailureResult`'s live check a moment earlier is exactly what this sweep then finds stale; `getShard` answers present because this sweep only empties shards and `removeShardIfEmpty` is not on its path at all; and the residency read sees a resident whenever the add lands before the `workMap` removal. Only the trigger differs, and a new assignment widens the window rather than narrowing it. The analysis is written at the site, not just here. EVIDENCE - RED FIRST, four ablations, each turning exactly the expected arm red and no others. `RetryQueueRequeueWindowTest` 12/0/0/0 green. - Second removal made but INERT (`departed -> true`): 12 tests / 2 failures - `theProductionSweepTakesOutAnEntryTheControllerAddedInsideTheSweep` on the orphan assertion, and `aRefusedSecondRemovalPutsTheContainerBackSoThePairStaysWhole` on the resident assertion. - Paired form dropped for plain `removeWorkAtOffset`: the same two, on their seam-never-fired PRECONDITIONS - which is the arms saying production stopped calling the method they instrument, rather than saying the orphan appeared. - Put-back omitted: 12 / 1, on "the refused second removal must put the container BACK in its shard". - Claim restore omitted: 12 / 1, on the awaiting-selection assertion. Every ablation reverted and the tree checked clean afterwards. THE TESTS, AND WHY TWO OF THEM NEEDED A NEW KIND OF SEAM. The hand-built arms #437 left behind model the sweep's removals by hand, so none of them can go red when production's ordering moves - the over-claim review caught on that PR. They are now three, one term changed between each: `aQueueFirstSweepWithoutItsPairedSecondRemovalWouldOrphanTheEntry` (the control, asserting the orphan appears), `aQueueFirstSweepWithItsPairedSecondRemovalKeepsThePairWhole` (the inversion #437's note asked for), and `aShardFirstSweepIsCaughtByTheConfirmation` (unchanged, the ordering master had). Coupling them to production needs the controller's re-queue to land INSIDE the real sweep, and the only production instruction between its two queue removals belongs to the shard. `SeamShard` is a `ProcessingShard` planted in the map before the assignment that would otherwise create one - so `addWorkContainer`'s `compute` finds it and production writes into it - overriding `removeWorkAtOffsetPairedWith` to fire `sm.onFailure` once. Built with the manager's own `RecordPopulation`, or the conservation figure would not see what it holds. `aRefusedSecondRemovalPutsTheContainerBackSoThePairStaysWhole` adds one term to that seam: the interference opens a live `RetryQueue` iterator and leaves it open, so the second `tryLock()` is refused - a `ReentrantReadWriteLock` grants no upgrade, so a thread holding the read lock is refused the write lock even though it is the same thread, which is what makes the refusal deterministic without a second thread. The add is made before the iterator is opened, because it takes the write lock and would otherwise deadlock against the reader on this thread. `theProductionSweepsQueueRemovalIsGatedOnItsShardRemoval` asserted a property this branch deliberately removes, so it is re-stated rather than deleted: as `theProductionSweepAsksTheQueueBeforeItTouchesTheShard` it pins the ordering the abandon-on-refusal design needs, with the same fixture and control and the assertion inverted. It is green on merit - production genuinely asks first - and red if anything reverses that. RECORDS. `ShardManager.onFailure`'s ordering caveat is gone, replaced by which half each mechanism covers and a five-case interleaving table that now includes the add landing inside a sweep; its first paragraph also loses the "nothing can ever take it out again" over-claim that PR's own squash body had already corrected. `removeWorkFromShardFor` gains the second-ask paragraph and a dated second reopen condition. `docs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard-removal.md` is retired, its instruction carried out; `bug-retry-queue-orphan-window-between-the-requeue-check-and-the-add.md` existed only on this branch and is deleted, the solutions write-up on master owning it now. That write-up's "#431 is an open draft" framing is corrected to what landed, including the new arm names and the reopen conditions. Two notes that arrived from master naming this PR in the future tense - the shard-displacement one and the Infer `@ThreadSafe` one - are rewritten as they will read after this merges. Full core unit suite 738/0/0/8, `bin/check-all.sh` 16 passed 0 failed, Lincheck lane 9/0/0/0 (`ShardManagerLincheckTest`, `WorkManagerLincheckTest`, `RetryQueueLincheckTest`, `PartitionStateLincheckTest` and the two toolchain probes). 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 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
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
6 tasks done
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
…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>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…ller collects what it leaves (#481) WHAT CHANGED FOR A USER: a rebalance can no longer be delayed by however long the control loop's retry-queue scan takes, in any ordering or commit mode. `RetryQueue.remove` takes an unbounded, FAIR `writeLock().lock()`, and every Kafka rebalance callback reached it on the broker-poll thread - by direct call through `ShardManager.removeWorkFromShardFor` on revoke and lost, and by the method reference `.map(retryQueue::remove)` through `ShardManager.removeStaleContainers` on all three, assigned included. The controller thread holds the matching READ lock for a whole scan (`RetryQueue.iterator()` hands it out; `ShardManager.getLowestRetryTime` closes it only after a full iteration), and a fair lock queues an arriving writer behind that scan rather than interleaving. The wait sits inside `consumer.poll()` with the whole group waiting, spent out of `max.poll.interval.ms`. No cycle was found: an unbounded wait, not a deadlock. No fork issue exists; found by the confluentinc#857 defect-class sweep (#29). THE FIX: THE POLL THREAD NEVER TOUCHES THE RETRY QUEUE. The callbacks remove from the SHARDS only - a `ConcurrentHashMap` of `ConcurrentSkipListMap`s, no lock to wait for and so nothing to decline. `ShardManager.purgeDepartedRetryEntries()` then collects, on the controller thread where waiting is allowed, every retry-queue entry whose container `ProcessingShard.isResident` says has left: scan under the read lock, close the iterator, remove the collected entries under the write lock in one `removeAll`. The invariant, stated once at the purge: a retry-queue entry with no resident container is garbage the controller collects; it may exist for at most one control-loop tick; the callbacks are free to create it. Where it runs: at the top of `ShardManager.getWorkIfAvailable`, the one point the control loop reaches unconditionally every pass in both RUNNING and DRAINING, and before `drain()` reads `getNumberOfWorkQueuedInShardsAwaitingSelection` in the same pass - the consequence that matters, because #437 established that a queue-only orphan surviving that figure holds a draining close open to its timeout. The bound is one tick in every ordering mode: the purge scans the queue rather than the shards, so the ordered modes' shard-scan break cannot delay it. #468 landed while this was open and settles by construction what this branch argued: `WorkContainer` equality is now identity, so the controller-side stale branch's compare-and-remove evicts only the container it inspected, and the purge's by-key removal no longer rests on the single-writer argument alone. Why scan and remove may be two steps: departure is monotonic (`isResident` is reference identity and nothing re-inserts the same instance), and the queue has exactly one writer - `RetryQueue.add` is `@ControllerThreadOnly`, its only production caller is `ShardManager.onFailure`, and the purge runs on that same thread. A second writer is what would reopen this, and both sites now say so: a `false` from a removal there can only mean one exists. THE DESIGN IT SUPERSEDES, NAMED. #431 kept the poll thread on the queue and made it DECLINE: `tryRemove`, abandon the paired shard removal on refusal, and after #437 a second ask after the shard removal with a put-back on refusal. It was correct and proven, and it lost on machinery: a second non-blocking entry point keyed differently from the blocking one, a queue reference threaded into a second `ProcessingShard` method, two asks per removal, and a retirement bound that varied by ordering mode. Also rejected: reusing #466's control-thread hand-off, which has the poll thread WAIT on a future - right for a revocation commit, wrong here, because the purge needs no hand-off at all. COUNTERS DURING THE WINDOW, RE-MEASURED. Between a callback's shard removal and the purge, `getWorkableRecords` reads one LOW - the safe direction: `isSufficientlyLoaded()` reads false and the poller fetches sooner; high would have paused it with nothing to process. `WorkManagerTest`'s revocation arm pins the direction. #437's post-add residency confirmation in `ShardManager.onFailure` becomes belt-and-braces and is kept this round, its javadoc saying so. THE STATIC GATE GOES GREEN ON MERIT. `ArchitectureTest.rebalanceCallbacksMustNotBlock` (widened in #465 to follow method references and `@ControllerThreadOnly`) carried eighteen exemptions for exactly these reaches. With the set emptied and the callbacks still reaching the queue it reports every route; with the callbacks off the queue it is green and the set stays empty. The set, its `root => target` keying and the argument for keeping it remain - an exemption is how a newly found reach is recorded as a defect on the books. EVIDENCE, AS SHAPE AND REPRODUCE COMMANDS. `RetryQueueRebalancePathTest` holds the read lock the way the controller does - a live iterator on another thread - and drives the real callback on a thread named `broker-poll`: red on unfixed code, both callbacks parked on the write lock until the deadline; green after, with the departed entry collected on the next controller pass, a queue-only entry no longer holding drain open, the ordered-mode arm collected on the same tick, and a still-resident entry left alone as the control. Ablations: deleting the purge call reds arms across three classes; inverting the residency predicate reds the same plus the control. Two arms from #437's window test are re-stated rather than deleted, because the sweep they described stopped existing. The contended harness was then folded into the shared `BlockedThreadAsserter` (a `returnsWithin` idiom that returns the fact rather than asserting it, so a caller can hold the contention while it reads state), re-proved red by restoring the queue removal to both callback paths. The three `DLS_DEAD_LOCAL_STORE` findings this branch's own lines introduced are cleared by using the values - logged, or a `void` return - rather than parking them. Reproduce: `bin/ci-unit-test.sh`; `bin/lincheck-test.sh`; `bin/check-all.sh`; `./mvnw -Pci -pl parallel-consumer-core -am test -Dtest='RetryQueueRebalancePathTest,RetryQueueRequeueWindowTest,WorkManagerTest,ArchitectureTest'`; `bin/check-pr-analysis-surfaces.sh 481`. RECORDED, NOT APPLIED. The purge is the first unconditional full scan of the retry queue on the control loop; both simplify reviewers converged on gating it behind a departure signal (a monotonic `LongAdder` in `RecordPopulation`'s shape). Not done here: it would put poll-thread-written state back on the rebalance path, and the cost is unmeasured - `DispatchScanMeter` is the precedent for metering first. Filed as `docs/inflight/perf-retry-queue-purge-scan-is-unmetered.md` with that order stated. RECORDS. `docs/solutions/runtime-errors/retry-queue-write-lock-on-the-rebalance-path.md` records both designs. Retired: `docs/inflight/bug-retry-queue-write-lock-on-the-rebalance-path.md`, `pr-431-must-pair-its-queue-removal-with-the-shard-removal.md`, and the branch note that guarded the superseded PR while it was still open. Corrected beside their dated text: the ArchUnit rules note (the set is empty on merit), the RetryQueue coverage note, the shard-displacement note (its harm is now bounded by one tick), the runtime-ownership-guard note (the single-writer premise makes that guard more load-bearing), two notes that named #431 as the fix, and the orphan-window write-up (its assertion stays as written; the confirmation is belt-and-braces). Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… and the guard is outside the class (#483) Settles the one open question on docs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md: whether production can reach ProcessingShard.addWorkContainer's displacement branch with a queue-resident container, which decides whether the pairing gap there is worth a design change. It cannot. No behaviour changes - the diff is a test, a solutions write-up, and comments. The branch itself IS reachable: confluentinc#909's late drain puts a stale resident in a shard and the next poll's fresh record displaces it. What cannot happen is that resident also holding a retry-queue entry. THE ARGUMENT, EACH LEG TRACED IN SOURCE An orphan needs four things true at once when a container B arrives at (topic, partition, offset): a resident A; A observed STALE, or the arrival is dropped; A holding a RetryQueue entry; and B existing at all. 1. The displaced container is always the one inspected. workMap has two insertion sites - addWorkContainer, which is control-thread-only, and the test-only plantResident. Only removals can interleave between the get and the put, and a removal makes the put return null. A by-key REMOVAL can hit a different occupant (#468's class); a by-key insertion cannot, with one inserting thread. 2. A queue entry implies non-stale-and-resident when it was made. RetryQueue.add has exactly one production caller, ShardManager.onFailure, behind onFailureResult's live staleness check and, since #437, a post-add residency confirmation. couldBeTakenAsWork refuses a stale container outright, so one that is stale on arrival is never selected, never fails, never queues. 3. Staleness is monotone: both epochs are final longs, the fence is set and never cleared, a replaced state carries a higher epoch or is the removed singleton. 4. So A must cross the staleness boundary in between, and only three transitions can do it - the removed-state swap, the assignment putAll, and fenceForRevocation. The first two each unseat A on the same thread inside the rebalance callback; the fence sweeps nothing but cannot be followed by a second container at the same offset, because a duplicate offset needs a re-assignment and that is the swept path. Bumping the epoch map alone is NOT one of those transitions, and this is the leg most easily got wrong by reading: PartitionState's epoch is final and staleness is only asked through the state object, so the window opens at the state REPLACEMENT - which is the same statement as the sweep that closes it. WHAT THIS DOES NOT CLAIM, AND IT IS THE POINT OF THE WRITE-UP The last leg is a property of the KAFKA CONSUMER's fetch position, not of this engine: within one assignment generation the position never goes backwards, so a duplicate offset needs a re-assignment. Nothing in main calls seek, and shards are partition-scoped in every ordering mode. So the pairing gap in that branch is one arrival away from being real rather than absent. An in-generation replay of an already-registered offset - a seek, an offset-reset or truncation replay - makes it reachable immediately, and nothing goes red for that. #481's purge would then collect it within one control-loop tick, which is the second, independent answer: the harm is bounded whatever happens, and this work says the case does not arise. Neither result implies the other. ORDERING-MODE INDEPENDENCE, RAISED IN REVIEW AND CLOSED UNDER TEST Legs one to six never consult the shard key. The mode is visible only in leg seven's second half, and is safe in every mode because a shard is partition-scoped throughout - PARTITION and UNORDERED make it a topic-partition, KEY makes it topic-plus-partition-plus-key. The disproof arm is parameterised over every ProcessingOrder, and a dedicated arm covers the KEY half: two records sharing a key and an offset on different partitions must be held in two shards, not collide in one. Under KEY an emptied shard is garbage-collected, which makes the conclusion stronger, not weaker - a collected shard holds nothing to displace. HOW IT WAS ESTABLISHED ShardDisplacementOrphanReachabilityTest, four arms over six cases, control-armed because a single green arm here is green whether the sweep works or the branch is simply never reached: the production rebalance sequence; confluentinc#909's late drain, which fires the branch with an unqueued container; a positive control that reaches stale-and-resident-and-queued through the production API via the fence and injects the one arrival production cannot supply, where the orphan appears immediately, asserted by reference; and the KEY-ordering collision arm. Ablation, predicted before running and re-run after each of the three merges this branch took, because #481 made the sweeps shard-only and #468 made each removal a compare-and-remove - an ablation against a shape that no longer exists is evidence about nothing. Same result every time: either sweep alone green, both removed red at the load-bearing assertion. The KEY arm was separately verified by sabotage - making the key-ordered shard key topic-scoped turns exactly that arm red, and ShardKeyTest stays green under the same sabotage, which is why the arm is not redundant with it. Two predictions were refuted and are kept visible rather than quietly fixed: that the epoch bump opens the window, and that the replacement would be handed out on the controller pass after the fence - it is not, because the fence makes the whole partition stale. DEFECT-CLASS SWEEP - BY-KEY REMOVALS THAT CAN HIT A DIFFERENT OCCUPANT Reported against the post-astubbs#468 tree rather than fixed here: - ProcessingShard.removeStaleWorkContainersFromShard and getWorkIfAvailable's last-resort sweep - both now conditional on the inspected container. Fixed by #468. - ProcessingShard.onSuccess - still removes by key. #468 reasoned about it explicitly and left it, on the grounds that the container it removes cannot have been replaced. Checked, and left to that argument. - ShardManager.removeWorkFromShardFor - the revoke path's removeWorkAtOffset on the poll thread, and the one remaining unconditional by-key shard removal. Reported, not fixed: it belongs with #468's mechanism, not with a reachability proof. - RetryQueue.remove / removeAll - by coordinates and never by identity, at every call site. Deliberate, but it means a removal can clear a replacement's entry; ShardManager.onSuccess removes by key before it looks up any shard, so a late success for a departed container clears a fresh one's parked-for-retry entry. The cost is an early retry, not a stall or a loss. Noted, not raised as a defect. A cleared suspicion #468 recorded independently at the last-resort sweep rests on the SAME single-writer fact as leg one, reached from the other direction. Both clearances therefore reopen on one ungated condition - anything that puts into a shard off the controller thread - and that is now said at every site that depends on it. WHAT IS LEFT FOR THE OWNER The inflight note stays OPEN with a PROPOSED closed - unreachable AND bounded vetted marker rather than being closed here: its impact is misdirection, and docs/inflight/AGENTS.md reserves that state change for the owner. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
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.
No fork issue exists for this defect. It was raised as a Codex P1 during review of #431, verified there as real and pre-existing, and left for its owner with that review thread deliberately open.
Description
WorkManager.onFailureResultre-validates staleness against the LIVE partition map immediately beforesm.onFailure(wc), and its own comment said that no epoch check placed there can ever be atomic with the actions that follow. A rebalance completing in that gap on the broker-poll thread left a queue-only orphan: the revoke sweep removed the container from its shard and then from the retry queue, finding nothing there yet, and the controller then added it. Under PARTITION or UNORDERED ordering the emptied shard object survives - only KEY ordering garbage-collects one - so the shard-present guard passed.What it costs a user. Work is handed out by scanning shards, so that container is never selected, never completed and never swept. Once its retry delay elapses the entry survives the flooring in
getNumberOfWorkQueuedInShardsAwaitingSelectionon a drained pipeline, soisRecordsAwaitingProcessing()reads true,AbstractParallelEoSStreamProcessor.drain()never transitions to closing, and a draining close hangs to its timeout on an instance with no work in it.The bound is the partition's absence, not the instance's lifetime - corrected in
48d193dfeafter the same over-claim was caught on the displacement sibling.RetryQueuekeys by topic, partition and offset, never by container identity, andShardManager.onSuccessremoves by that key unconditionally, as its first statement, before it looks up any shard - so it is a removal route that does not go through shard contents, and any later container at those coordinates clears the entry. The orphan is created by a revocation, so none arrives until that partition is reassigned here. What makes the consequence hold regardless is that a draining close is exactly the window in which no reassignment can occur.Reachability was established, not assumed.
RetryQueueRequeueWindowTestdrives the interleave deterministically -RequeueWindowWorkManageroverrides the live epoch check, computes the real answer, then runs the full production revoke path before returning it. Every arm asserts two preconditions (the race fired, and the check answered not stale), so an arm that stops exercising the window fails rather than passing vacuously. Predictions were stated before the run and all held. Against unmodified master, 3 of 6 arms are red: the two window arms and the drain arm.The recorded consequence was wrong, and is corrected here
Both the comment at
onFailureResultand the in-flight note raised on #431 said the harm was the broker-poller load gate being misled viaShardManager.getWorkableRecords()- a confluentinc#857-family stall. Measured, that does not hold in either half:parkedForRetryisqueueSize - readyToRetry, so the orphan contributes only while its retry delay is still running; once it passes, the orphan counts in both terms and the contribution is exactly zero.workableread LOW, soisSufficientlyLoaded()reads false and the consumer fetches MORE.RetryQueueRequeueWindowTest#aQueueOnlyOrphanCostsTheDrainFigureAndNotTheLoadGatemeasures both halves against an orphan built directly, so it stays checked whether or not the window is open, and the false claim cannot be re-derived.The fix, and why the obvious ones were rejected
A residency test before the add only narrows the window - it is another check-then-act, and the sweep lands between that answer and the add exactly as it lands between the epoch check and the call. That is what made this a note rather than a patch.
ShardManager.onFailurenow adds to the retry queue and then confirms the container is still resident in its shard, undoing the add if it is not. The last action is a removal driven by a read taken after the add, and the competing sweep's own action is also a removal, so at least one of the two always observes the entry. All four interleavings settle with the pair whole; the argument is on the method. Residency is by reference, so once the read sees a departed container the answer cannot go stale in the dangerous direction. This is the shapeProcessingShard.includeInSelectionalready used for the selection claim, and the predicate is now shared asProcessingShard.isResident.It does NOT collide with what #431 established
That PR establishes that the rebalance callbacks may not wait on the retry queue's lock - they run on the broker-poll thread inside
consumer.poll(), where the whole group waits - and makes them decline the lock rather than block on it. The textbook fix here, moving the shard map and the queue under one lock, would reverse that decision, because it puts the poll thread's revoke path behind the retry queue's fair lock. This change adds no lock and makes no thread wait, and touches nothing on the poll thread's side of the interleaving; the controller does one extra map read and, in the rare losing case, one extra queue removal.Correction to the record, which was repeated from the original note: #431 has NOT shipped. It is an open draft, so master is still shard-first. Anything describing the declining sweep as landed - including the in-flight note that first recorded this defect - is describing that branch, not master. The defect was reproduced on master regardless, so nothing about its reachability depended on the wrong version.
BUT the two PRs must meet: #431 needs a second queue removal
Raised in review and verified by demonstration, not by reading the diff. The confirmation above is ordering-dependent, and the ordering it depends on is the sweep's, not this method's. It holds against master's shard-first sweep. #431 reverses that order deliberately - to decline the lock safely it asks the queue first, and
tryRemovereturns true when it acquired the lock whether or not anything was present ("there was no entry" and "I removed the entry" are the same answer to the caller's question). Against that ordering:tryRemove- lock acquired, queue empty because the controller has not added yet, returns true;retryQueue.add(wc);isResident(wc)-> true, the shard removal has not happened yet, so the add stands;removeWorkAtOffset- the container leaves its shard.Orphan, through the few instructions between the sweep's two steps - which is precisely the widening the original note attributed to #431, and it matters more than that note thought.
What #431 must add is small: repeat the queue removal AFTER the shard removal. That closes the half where the controller's add lands inside the sweep; the residency confirmation closes the half where the add arrives after the sweep has finished. Neither half is redundant and neither closes the other.
Three arms pin this, and only one of them can actually fire.
aQueueFirstSweepDefeatsTheOneShotConfirmationmodels the queue-first ordering and asserts the orphan appears - red-by-design documentation, green while master is shard-first. Its matched controlaShardFirstSweepIsCaughtByTheConfirmationruns identical steps with only the sweep's internal order changed and asserts no orphan. Same magnitude, different position: the pair is what establishes ordering as the responsible term.Both of those hand-build the sweep, so neither can go red when production's ordering changes - review caught that the "cannot pass each other silently" claim was resting on a future author remembering, which is "enforce the invariant, don't document it" going the wrong way.
theProductionSweepsQueueRemovalIsGatedOnItsShardRemovalsupplies what they cannot: it drives the REAL revoke sweep over two partitions - one container still shard-resident (the control, without which "the departed one's entry survived" would also pass on a sweep that never ran) and one that has left its shard keeping its queue entry - and asserts the single property the confirmation depends on, that the queue removal is reached only through a non-null shard removal. Verified by ablation: splice a queue-first removal intoremoveWorkFromShardForand exactly that arm goes red, alone, with a message naming the paired removal #431 has to add.The javadoc on
ShardManager.onFailurenow states this precondition instead of reading as ordering-independent, anddocs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard-removal.mdcarries the coordination from the other side.Evidence
bin/check-all.shgate passing. Reproduce:./mvnw -Pci -pl parallel-consumer-core -am testandbin/check-all.sh.Duplication follow-up, and a file this PR otherwise did not touch
The
dups: clonesreport flagged one clone introduced by this PR: 19 lines shared betweenRetryQueueRequeueWindowTestandWorkManagerStaleCheckDoubleLookupTest. That is real - the new test copied the latter's racing-double harness, which is exactly the duplication the report exists to catch, as opposed to clones already on the base branch. It was fixed for that reason and not to clear a red check.dups: similaritywas red on an earlier head for an INHERITED breach -JStreamParallelEoSStreamProcessoragainstJStreamParallelStreamProcessor, neither file touched by this branch, both arrived with anorigin/mastermerge. Out of scope here by the same rule that put the harness clone in scope, and green again on the current head.Factored out into
RacingSeamWorkManager, a shared abstract test double holding the one-shot arming mechanism (arm,raceHasFired,fireOnceIfArmed) and the constructor. Each subclass keeps only its own seam: the pre-existing one overrides checkpoint 3's two-argument staleness lookup, the new one overrides the live single-argument check and adds its own live-check bookkeeping.This changes
WorkManagerStaleCheckDoubleLookupTest, a pre-existing master file this PR did not otherwise touch - flagged so a reviewer is not surprised by it. The change is mechanical: the double now extends the shared base instead of carrying its own copy, and the rationale for keeping the fired-flag separate from the armed slot moved to the base class rather than being deleted. No assertion, arm, or scenario was altered on either side, and both classes run green with the same test counts as before.The module-construction block (
PCModuleTestEnv+MockConsumer+ PARTITION ordering) is not deduplicated here: it is pre-existing across nine test files on master, so it is base-branch duplication and out of this PR's scope.Defect-class sweep
The class is a container leaving a shard without its paired retry-queue removal. Every site that removes a container from a shard was checked. Paired, and dismissed:
ProcessingShard.onSuccess(paired byShardManager.onSuccess, which removes from the queue first), andremoveWorkAtOffset's three callers -removeWorkFromShardFor,removeStaleContainersFromShardviaShardManager.removeStaleContainers, andgetWorkIfAvailable's last-resort branch.One is not paired:
ProcessingShard.addWorkContainer's displacement branch retires the container it displaced and releases its selection claim, but cannot remove its retry-queue entry - the shard holds no reference to the queue. The pairing gap is demonstrated by probe; production reachability is not established, and that open question is what decides its urgency. Recorded at the site and indocs/inflight/bug-shard-displacement-orphans-the-retry-queue-entry.md. Not fixed here - it needs a design choice about whether the shard should own the queue handle.Its cost was over-claimed in the first version of that note and of the code comment, and is corrected here. Both said the displaced container's entry was one nothing could ever remove, holding a draining close open - the same permanence as the defect this PR fixes. It is not:
RetryQueuekeys by topic, partition and offset alone (WorkContainerKey.of), andShardManager.onSuccessremoves by that key unconditionally, so the replacement admitted at the same offset clears the entry at its own first terminal event (success removes it, failure re-adds the same key whichaddreplaces rather than duplicates, a sweep that finds the replacement removes it by key). What is wrong inside that bounded window is the figure - the surviving entry carries the displaced container's retry-due time, so ready-to-retry andgetLowestRetryTimeread one high. The note's impact tag moves fromstalltomisdirection; the demonstrated pairing gap and the unestablished reachability are unchanged. Same shape of over-claim as the load-gate one below, which is why it is called out at both sites rather than quietly reworded.Records
The solved problem is written up in
docs/solutions/runtime-errors/retry-queue-orphan-window-between-the-requeue-check-and-the-add.md. The in-flight note that tracked this defect (docs/inflight/bug-retry-queue-orphan-window-between-the-requeue-check-and-the-add.md) exists only on #431's branch, not on master, so this PR cannot retire it; it should be deleted when either lands, since the knowledge now has a durable owner.Checklist
WorkManager.onFailureResultdocs/features/- N/A - internal concurrency fix; no user-facing option or behaviour change beyond a close that no longer hangsRetryQueueRequeueWindowTest(8 arms: 2 window, 1 drain, 1 characterisation, 2 control, plus the sweep-ordering pair)docs/inflight/working note (pr-/branch-) started at the PR's first commit -docs/inflight/pr-431-must-pair-its-queue-removal-with-the-shard-removal.mdcarries the cross-PR coordination; the defect's own record is the solutions write-up, and the sibling defect found has its ownbug-notece-simplifyandce-code-reviewlocally - N/A - this ran asce-debugend to end. Two dedupe passes were done in-flight rather than as a separate simplify run: the residency predicate intoProcessingShard.isResident, and the racing-double harness intoRacingSeamWorkManagerin response to thedups: clonesreport. Prefer@claude review thison the PR, which is the only route that opens inline review threads.Review rounds on this PR
3a2e331d4: couple the sweep-ordering arms to the production sweep (above); nameRetryQueue.remove's discarded result at the undo inShardManager.onFailure- now logged, saying which of the two racing removals won, rather than assigned to an ignored local, because a dead store in main code trades an Error Prone finding for a SpotBugsDLS_DEAD_LOCAL_STOREone (already recorded as tried-and-rejected indocs/inflight/static-error-prone-rule-registry.md); and name the synthetic sweep's discarded results in the test - the shard removal's return is now asserted to be the container under test, and the queue removal's is named with why it is deliberately not asserted.claude[bot]: no correctness issues, nothing blocking. It independently re-walked all three shard-removal sites against the ordering precondition and confirmed them, and ruled out a key-vs-reference concern at the undo (onFailureruns only on the controller thread; the only concurrent actor removes through this window, never adds).origin/mastermerged in (ada7a6c16). That is also what cleared the redIntegration Testslane at source: master quarantinedRegistrationRaceStaleResidentITin ci(build): check in the Maven wrapper jar, and quarantine a test that fails its own setup guard #440 as a master-state flapper that fails its own saturation/pause-point setup guard, which is the failure this branch's earlier heads hit - tracked with cross-branch evidence indocs/inflight/test-untracked-ci-flakes.md, and never this PR's regression.Open items for the reviewer, not blocking this diff:
ProcessingShard.addWorkContainer's displacement branch is reachable in production with a queue-resident container - if it is, it is the same defect at a second site and needs its own fix.Whether the commits should be re-cut as red-test-then-fix- answered: squash-merge, so no re-cut. One defect, one fix; the remaining commits are a javadoc/test precondition statement, a harness dedupe, a records correction and a review round.