Repository navigation
test(core): the revoke-path commit omits offsets for records it already produced - #436
Conversation
…dy produced Settles the one thing an earlier note left open, and settles it the other way. The note recording this gap said the whole question was "whether the window is reachable in practice", and offered a reason it might not be: the produce read lock is held across the send and its acks, and a commit cannot start while any read lock is held, so the mailbox might always be empty of produced work whenever a commit can begin. That argument is backwards. The ordering is send -> addToMailbox -> release the read lock, not send -> release -> mailbox: cleanUpContext is the single release point, runs in runUserFunction's finally, and its javadoc states the contract outright. So a returned produce lock GUARANTEES the work is already queued, and a commit granted the write lock always has undrained work in front of it. The lock discipline does not close the window - it guarantees the window is open. What closes it is the drain, and only the control loop performs one. The gap bites because PartitionState#onSuccess - the only thing marking a partition dirty on a success - is reachable from processWorkCompleteMailBox and nowhere else in main. The control loop takes the commit lock, drains, then collects offsets. tryCommitOffsetsOnRevoke takes the lock and collects, skipping the drain, so it publishes a transaction containing a record whose source offset it omits: output committed, input not, next owner reprocesses it. Exactly-once degrades to at-least-once on that path. Controlled experiment, prediction stated before the run and held exactly. Treatment - the revoke as it stands - sends offset 1 where 2 is required, RED 5/5, deterministic, no broker and no timing. Control arm - identical but for a processWorkCompleteMailBox call inserted immediately before the revoke, nothing else moved - passes. Same magnitude, different position, so the drain is the term. Both arms live in ProducerManagerTest beside the C9 proofs they extend. NOT FIXED HERE, on purpose. The revoke callback runs on the broker-poll thread and the drain mutates control-thread-confined WorkManager state, so the obvious one-line fix is the same cross-thread mutation that corrupted numberRecordsOutForProcessing in #29 (measured -8, -16, -20, -20, -20 against a truth of 0). The fix is a thread-ownership decision at a seam this project has patched four times and never restructured; the tracking note lists the candidates and what each costs. Precedent for splitting it out is #262's own residuals commit: a main-code correctness fix deserves its own change and its own reviewer. The red arm is @Quarantined rather than @disabled, so the proof keeps executing and the lane will demand its removal the day it passes. It is deliberately NOT @ProvesClaim - the coverage guard rejects a proof the gating lanes exclude, and is right to, so C9 keeps its enforced coverage from the two control-loop proofs that really run. C9 NO_PRODUCE_WITHOUT_ITS_OFFSET moves PROVED -> REFUTED: the claim is written as a property of the system and one reachable path breaks it. The register records both halves rather than replacing one with the other. The documented sentence in ParallelConsumerOptions is left alone - REFUTED says the disposition is a triage decision, and the note is the defect being filed; softening the promise is the wrong half of that choice to take unilaterally. STRATEGY.md said in terms that no claim in the register is refuted, and the README's machine-checked list carried the guarantee unqualified. Both are corrected, because a register written to fire against us is worth nothing if the finding gets softened instead of published. Not established, and said so in the note: field impact (the proof is in-process against a mocked producer), whether #408 narrows or widens it, and whether this is the mechanism behind #173 / confluentinc#777, which reports this symptom from the field - a candidate cause, not an attribution.
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
|
| PR | Base | Change | |
|---|---|---|---|
| Clones | 96 | 96 | ➖ 0 |
| Duplicated lines | 1361 | 1361 | ➖ 0 |
| Duplication | 0.80% | 0.81% | 🙂 -0.01% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 2% | ✅ Pass (0.80%) |
| Max increase vs base | +0.1% | ✅ Pass (-0.01%) |
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 No quarantined test changed outcome since the previous push. 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 #436 +/- ##
============================================
+ Coverage 81.70% 82.71% +1.00%
- Complexity 1324 1494 +170
============================================
Files 84 95 +11
Lines 4597 5189 +592
Branches 492 508 +16
============================================
+ Hits 3756 4292 +536
- Misses 648 706 +58
+ Partials 193 191 -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:
|
🟢 Throughput — OKThis branch measured about 19% faster than master, on the one test this measures. That is larger than this test's own run-to-run spread of about 17%, so it is worth looking at.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: 29f6a0f, a75400f, 12bf414, ce6f39a, 70a88bf, 27211b9, c5dde06, 92364a1, 867c407, 7a8dd92 Since the previous push: ratio 1.072 -> 1.194, share 1.621 -> 1.4, rate 69814 -> 80081 (+14.7%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
[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. |
…ect yet Asked at merge prep: can the concurrency annotations help here, or expose this better? Recorded because the intuitive answer is wrong in a way that would cost something. A DECLARATION HERE WOULD BE FALSE, AND FALSE IS WORSE THAN ABSENT. RacerD reads com.facebook.infer.annotation.ThreadConfined and consumes declarations without checking them, so @ThreadConfined(CONTROL_THREAD) on the state the revoke-path drain would mutate does not expose this defect - it silences the detector that might otherwise find it. RetryQueue's own usage already states the rule: an annotation nobody enforces is a comment that silences a detector. INDEPENDENT CORROBORATION, WHICH IS THE PART WORTH KEEPING. #433 reached this same seam from the other direction while putting the infer annotations on the compile classpath. It evaluated lastCommitTime as a confinement candidate, found every record of the field claiming two control-thread accessors, grepped the writers, and found tryCommitOffsetsOnRevoke() writes it from inside onPartitionsRevoked on the broker poll thread. It made the field volatile instead and said so in the field's javadoc. One investigation started from the mailbox drain and one from a field's writers; both landed on the revoke path running control-thread work on the poll thread, and neither knew about the other. WHAT THE ANNOTATIONS ARE FOR HERE IS THE FIX. @GuardedBy does not fit - this is confinement rather than lock discipline, and the core AGENTS.md records that it silently checks nothing on a ReadWriteLock. The shape that fits is RetryQueueIterator's: once the thread-ownership decision at this seam is taken, declare the confinement AND assert it on the owning thread in the same change, so the decision cannot rot back. Until that decision exists there is nothing truthful to declare, which is why this note stays open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
|
@codex review Steer, because this PR is unusual in what it asks you to check. It is a diagnosis, not a fix, and it publishes a caveat about the product's headline guarantee. It flips claim C9 Push hardest on the reachability argument, because it inverts the note it came from. The original note guessed the window might be unreachable: the produce read lock is held across the send and its acks, so the mailbox might be empty of produced work whenever a commit can begin. This PR argues the opposite - that the ordering is send, then The deliberate non-fix. The one-line fix (drain from Two things already verified, so treat a contradiction as significant rather than as a fresh finding. The red proof is Not claimed, and please do not treat as claimed: field impact, whether #408 narrows or widens the window, and whether this is the mechanism behind #173. |
…, and name what the fix owes The previous commit answered the annotation question by writing the answer into this note. Most of that answer was already owned, in terms, by the core main-code AGENTS.md under "Declare thread confinement with @ThreadConfined, and assert it at the entry point" - the consume-without-checking property, the unenforced-annotation warning, the RetryQueueIterator pattern, and even the lastCommitTime premise check that reaches this same seam. Two copies of a rule drift apart, and that file is the one delivered to anyone changing core main code, which is where the fixer will be standing. So the section is now a pointer that names its owner, plus the two things the owner cannot carry because they are about this defect rather than about the rule: - there is nothing truthful to declare here yet, because a confinement claim over the state the drain would mutate is precisely the claim this note says is violated; - the obligation on whoever closes this note - the declaration and its runtime assertion go in the SAME change as the thread-ownership fix, for the reason the owner gives for @GuardedBy: the decision is obvious while you are making it and archaeology a month later. That last one is the part worth tracking. Closing this note without the pair leaves the seam free to rot back to exactly its current state, and nothing would fail when it did - which is the failure mode this whole note exists to describe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
[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. |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5236ddae25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The revoke-path experiment's helper asserted `getReadLockCount() == 0` immediately after awaiting that the worker's completion had reached the mailbox. That await does not establish the lock is back: the ordering is `addToMailbox` and THEN `cleanUpContext`, which is `runUserFunction`'s `finally` and the single produce-lock release point, so a worker can sit preempted between the two while the mailbox already reads 1. The assertion could therefore go red against correct production - in `aRevokeTimeCommitIncludesThatOffsetWhenTheMailboxIsDrainedFirst`, the control arm, which is NOT quarantined and so would have flaked a gating lane. Zero-read-lock is a precondition of the experiment rather than its result, so waiting for it costs the proof nothing: the same message still fails loudly if the lock is never returned, and the window under test is unchanged. Verified both arms after the change - the control arm passes, and the quarantined arm still fails at its own offset assertion (`expected: 2 but was: 1`) rather than at the precondition, so it still exercises the window it exists to expose. Raised by Codex review as a P1; the suite has already produced two controls that proved nothing, so a proof that can pass or fail without exercising its window is the failure mode this branch exists to expose in others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
… candidate reading as safe Three corrections to the records this branch wrote, all from Codex review. The note carried `<!-- inflight-state: open - ... -->`. Openness follows the PRESENCE of a state marker, not its words - `bin/lib/inflight-tags.mjs` sets `open: !STATE_MARKER_RE.test(text)` - so a note declaring itself open parsed as neither open nor deferred and fell into the index's "not shown" group. A diagnosed data-loss defect was therefore invisible to the session-start open-work index it exists to reach. `classifyNote` now returns `open: true` for it. Nothing is lost with the marker: the reason it carried is what the note's own headings already say. The first candidate disposition presented a post-write-lock mailbox-emptiness check as closing the window. It does not. Taking the write lock stabilises the mailbox against NEW produced work, not against the control thread EMPTYING it: `processWorkCompleteMailBox` does `drainTo(results, size)` into a local queue and only then loops calling `wm.handleFutureResult`, which is what reaches `PartitionState#onSuccess` and marks the partition dirty. A poll-thread check landing in that gap sees an empty mailbox and an undirty partition and commits the same incomplete offset map - and nothing gates that drain during a rebalance, which the note already says one paragraph earlier. The candidate now names the gap and states what it would actually take: decline unconditionally, or coordinate with the control-thread drain. The note's own rule was being broken by the note: a fix that narrows a data-loss window without closing it is worse than none, because it reads as closed. The reproduction counts are gone. `docs/inflight/AGENTS.md` forbids writing down what a command can answer, counts included, and says the rule applies hardest when writing up a measurement just taken. The durable finding is that the arm is deterministic - hand-driven control loop, mocked producer, no broker, no load, no timing - so the shape and the command to re-run it replace the tally. The register's own proof notes keep their observed rates deliberately: there the count IS the negative control's evidence, which is a different rule and a different document. Separately, `docs/quarantined-tests.md` said "Two entries" while the checklist below it held three - `RegistrationRaceStaleResidentIT` arrived from master after that paragraph was written, and its kind was left out of the classification. Rewritten to describe the two kinds and cover all three entries without a count, so the paragraph cannot go stale the next time the checklist changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
…oved Codex found the revoke-path proof does not only falsify C9. Both candidates were investigated and they come out differently, which is the point of this commit. C4 OFFSET_AND_RECORDS_ATOMIC goes REFUTED. Leaving it PROVED while C9 reads REFUTED would make the register assert and deny a single observation, not merely under-report: the two claims are about the same commit, and aRevokeTimeCommitIncludesTheOffsetOfEveryRecordItAlreadyProduced shows a transaction carrying offset 1's output while committing offset 1 rather than 2 - the atomic set broken, observed rather than inferred. C4's own exemption does not cover it: that exemption is about marker-delivery ordering WITHIN one commit, whereas here the offset is never committed by that transaction at all. Its control-loop proof is kept verbatim rather than deleted, because that half still holds. C14 RESULTS_EXACTLY_ONCE_UNDER_FAILURE stays PROVED, with a scope note. The route from an omitted offset to a duplicated result - redelivery, re-produce, duplicate - is sound, and no duplicate has been observed. C14's own record says its RED and its GREEN were each seen rather than argued, so refuting it on a reasoning chain would make it the register's first argued status and break the observed-versus-argued distinction that is the whole reason the register is worth more than prose. It would also repeat, inside the register, the mistake this branch exists to correct: a plausible argument about reachability that turned out backwards once somebody measured it. The claim now records the route and names what would settle it - a broker-level rebalance reproduction showing a duplicated result. THE PUBLISHED WORDING FOLLOWS THE REGISTER. The README caution now names both refuted guarantees and states that the front-page exactly-once promise is NOT refuted, so a reader is not left inferring the widest reading. Edited in the template and regenerated, never in README.adoc. The STRATEGY paragraph now NAMES the refuted claims instead of counting the statuses. The count it carried would have been wrong the first time any status moved, which is this commit; the register is the tally and this paragraph says so. TransactionalClaimCoverageTest stays green: REFUTED keeps coverage enforced and both claims already carry proofs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
|
@claude review this |
|
Claude finished @astubbs's task in 2m 12s —— View job Review
The central technical claim checks out against the code, not just the PR's own argument for it. I traced it independently rather than trusting the narrative:
So the diagnosis is sound, not just plausible-sounding prose. All six Codex findings from the last review round appear genuinely fixed, not just claimed fixed in a reply — I checked the current file contents rather than trusting the response comments:
One thing worth double-checking before merge, not a defect: the new quarantined test's Everything else is exactly what the PR claims to be: a diagnosis with no fix, and the non-fix is justified by a real, cited precedent (#29's cross-thread counter corruption) rather than hand-waved. The "not established" section is honest about scope (no broker-level reproduction, no claim about #173's root cause). I found nothing to add beyond what the very thorough Codex pass and the PR author's own responses already surfaced and resolved. LGTM — consistent with the human reviewer's assessment. No blocking issues found. |
…ion that decides it Checked against #408 before deciding where the fix belongs, and the note's guess about that PR was wrong in a way that matters. #408 declines the revoke commit only when the transaction lock is CONTENDED; its own amended RebalanceEoSDeadlockTest accepts "committed inline if the dwell had already ended" as a resolved outcome. That uncontended inline commit is exactly this defect's path, so #408 neither narrows nor widens the reachable case - it leaves it alone. What it does establish is the machinery a fix reuses: a decline branch documented safe, and a test contract that already counts declining as success. THE POSITION. Take the first candidate unconditionally, in transactional mode only. A mailbox-emptiness test does not close the window wherever it sits; unconditional has no gap to land in, and it adds no cross-thread mutation, so the #29 hazard is sidestepped rather than handled. Consumer-commit mode is untouched: it produces nothing inside a transaction, and an undrained success there is just a redelivery, which is that lane's published contract. WHERE. Its own pull request from master, not on #408 - that PR's subject is the wait, and it is stacked two deep. The two will collide on tryCommitOffsetsOnRevoke and that is resolved at merge, not dodged. THE QUESTION THAT DECIDES CORRECTNESS IS NOT YET ESTABLISHED, and the note now says so in terms rather than asserting the answer. On decline, what becomes of the open transaction's already-produced output? Aborted, and exactly-once is preserved - the decline is correct, not a compromise. Committed later by the control thread without the revoked partition's offset, and it is the same defect through a different door. #408's record is silent on the produced output, so the design's own account does not settle it. It has to be run. The un-quarantined proof plus a broker-level no-duplicate check is the instrument, and it already exists. Also recorded: the cost to measure first (how often a revoke lands with undrained produced work - the entire price of going unconditional), that the fix makes the confinement declaration truthful and owes it in the same change, and the acceptance shape: the proof leaves quarantine, C9 and C4 return to PROVED, the README caution comes out. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
The drain note named #408's note; #408's note did not name the drain defect. AGENTS.md's rule for a superseding or adjacent record is to link both directions, because a reader arrives from whichever side they know about and a one-way link strands the other half. The two defects share one method, tryCommitOffsetsOnRevoke, and are independent: the wait note bounds the CONTENDED case, the drain note is a correctness hole in the UNCONTENDED commit. Their fixes will meet on that method, and each note now says so and names the other, so whoever lands second knows what to resolve. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
… recorded deadlock The solutions write-up for the confluentinc#857 AB-BA deadlock was not cited by the note that records this defect's fix candidates, and it binds them. It has the poll thread parked in onPartitionsRevoked against the control thread holding the same monitor inside a blocking commit - this exact seam. It is reachable only in PERIODIC_CONSUMER_SYNC, so it is not this defect; it is a constraint on the fix. Two of the three candidates put the poll thread in precisely that position: the second in its naive form, and the "coordinate with the control-thread drain" variant of the first. Recorded beside them so nobody rebuilds the deadlock while fixing the drain. It is a second, independent argument for the unconditional decline, which waits on nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
…encing-brainstorm Fifteen commits on top of the sixteen the previous merge took, three conflicts: - PartitionState: master's javadoc on onSuccess(long) - the three closed shapes of its assert - with this branch's cross-reference to the container overload beneath it. - TransactionalClaim: master's 2026-09-07 scope note ends C14; this branch's C15 follows it. C4 and C9 move to REFUTED as master decided (#436); C15 is untouched by that finding. - The flake ledger: master's rows where both sides have one (the registration-race guard is now quarantined on master, #440), plus the three rows only this branch carries. One inherited instruction is NOT applied here: #450 asks this branch's replay loop to take the same register-then-publish swap it made in maybeRegisterNewPollBatchAsWork, and records that in the refactoring backlog. The commit after this one does it, with the pin. The paragraph in master's revoke-drain note that names this PR's stacking is attested post-merge: it records where #408 sat when the split was decided. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…the control thread (#466) In PERIODIC_TRANSACTIONAL_PRODUCER mode the revocation-time commit ran inline on the broker-poll thread and never drained the controller's work mailbox, so it could publish a transaction whose committed offsets omitted records that transaction contained: output committed, input offset not, and the partition's next owner reprocessed the input and produced the output again. #436 diagnosed it, refuted claims C9 and C4 of the transactional claim register, and quarantined the proof with a passing control arm. This is the fix. What changed for a user: exactly-once holds across a rebalance in transactional mode. RebalanceEoSDeadlockTest reads the output topic with a read_committed consumer after the revoked partitions return and fails on a repeated result; against the old code it found three to five duplicated results in about a hundred and ten, five runs of five, and reads zero five of five with the fix. C9 and C4 read PROVED again with their one documented exception, the proof left quarantine and carries both claims, and the README caution is gone. The fix. In transactional mode onPartitionsRevoked no longer commits on the poll thread. It posts a request, wakes the control loop with a mailbox message (never an interrupt, which a periodic commit's timed lock wait would read as a shutdown), and waits, bounded by commitLockAcquisitionTimeout. The control loop takes the request at the top of a pass and runs its ordinary sequence - write lock, flush, drain, fence, commit - then completes the request; a pass that throws fails it; a request the control thread has already taken is waited through to completion so truncation strictly follows the commit; the close serves whatever is pending at its own commit and the callback declines at once while the instance is closing. Waiting on the control thread is safe in this mode and only this mode: the deadlock behind confluentinc#857 is the control thread blocking on the poll thread, and a transactional commit needs nothing from it. The consumer-commit modes keep the inline tryLock commit. The fence. With the drain alone the broker-level check still read two or three duplicates per run: once the served commit released the write lock, a worker parked on the produce lock resumed with a record of a partition about to be truncated. PartitionState.fenceForRevocation is set by the served pass inside the write lock, for the assignment epoch the request was posted with, so a late pass cannot fence a re-assignment; the produce wrapper checks it right after the produce lock in both transactional modes and refuses with PCRetriableException. Rejected before writing code, by experiment: the position recorded at #436's merge prep was to decline the commit unconditionally. Two arms in ProducerManagerTest showed that a revoke which commits nothing leaves the output in the open transaction for the next commit to publish without its offset - the same defect through a different door. Declining is the deadline fallback, logged at WARN with that cost named, never the fix. Also: processWorkCompleteMailBox declares @ThreadConfined to the control thread with a runtime assertion; the confluentinc#548 sleep spin and its ArchUnit exemption are gone; the revoke-drain inflight note is retired into docs/solutions/logic-errors/the-revoke-path-commit-did-not-drain-the-mailbox-2026-09-07.md; STRATEGY.md's account of the register is brought current. Collides with #408 on tryCommitOffsetsOnRevoke; whichever lands second resolves it. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
1 participant
Relates to #173 (confluentinc#777) as a candidate cause only - see "Not established" below. This PR closes nothing.
Description
Diagnosis, not a fix. It settles a question an earlier working note left open, and settles it the opposite way to the guess the note offered.
The question
The note recording this gap was explicit that one thing was not established and that it was the whole question - "whether the window is reachable in practice". It offered a reason it might not be: the produce read lock is held across the send and its acks, and a commit cannot start while any read lock is held, so the mailbox may in fact be empty of produced work whenever a commit can begin.
The answer: reachable, and the lock discipline is what makes it so
That argument is backwards. The ordering is send ->
addToMailbox-> release the read lock, not send -> release -> mailbox.cleanUpContextis the single release point, runs inrunUserFunction'sfinally, and its own javadoc states the contract: the lock is released only once everyWorkContainerof the context "has been safely returned to the controller's inbound queue".So a returned produce lock guarantees the work is already queued, and a commit granted the write lock always has undrained work in front of it. The lock discipline does not close the window - it guarantees the window is open. What closes it is the drain, and only the control loop performs one.
It bites because
PartitionState#onSuccess- the only thing that marks a partition dirty on a success - is reachable fromprocessWorkCompleteMailBoxand nowhere else in main. The control loop takes the commit lock, drains, then collects offsets.tryCommitOffsetsOnRevoketakes the lock and collects, skipping the drain. So it publishes a transaction containing a record whose source offset it omits: the output is committed, the input is not, and the next owner reprocesses that input and produces the output again. Exactly-once degrades to at-least-once on that path.The controlled experiment
Prediction stated before the run, and it held exactly.
aRevokeTimeCommitIncludesTheOffsetOfEveryRecordItAlreadyProducedaRevokeTimeCommitIncludesThatOffsetWhenTheMailboxIsDrainedFirstprocessWorkCompleteMailBox(ZERO)inserted immediately before the revoke, nothing elseSame magnitude, different position, so the outcome is attributable to the drain and not to added latency or to anything else the revoke path does. Deterministic - hand-driven control loop on a mocked producer, no broker, no load, no timing. This is not a flake and must not be treated as one.
Both arms live in
ProducerManagerTest, beside the C9 proofs they extend, rather than in a new file.Why there is no fix here
The obvious one-line fix - drain from
onPartitionsRevoked- is a trap this repo has already paid for. The revoke callback runs on the broker-poll thread, and the drain mutatesWorkManager/PartitionStateManagerstate that every other mutation reaches from the control thread. That is the same shape of change that corruptednumberRecordsOutForProcessingin #29, measured at-8, -16, -20, -20, -20against a truth of 0.docs/solutions/architecture-patterns/two-threads-one-consumer-why-the-commit-seam-keeps-deadlocking.mdnames this exact hazard, and records that the seam has been patched four times and never restructured.Note also that the control loop already declines to commit during a rebalance, but the drain is not gated by that flag - the control thread keeps draining while a revoke is in progress, so a poll-thread drain would race it.
The tracking note lists three candidate dispositions and what each costs, including why the cheap "check the mailbox first" version leaves a narrower version of the same race - and a fix that narrows a data-loss window without closing it is worse than none, because it reads as closed. Picking between them is a thread-ownership decision at the commit seam. The precedent for splitting it out is this suite's own: #262's residuals commit, "a main-code correctness fix deserves its own change and its own reviewer".
Consequences carried in the same change
NO_PRODUCE_WITHOUT_ITS_OFFSETand C4OFFSET_AND_RECORDS_ATOMICboth movePROVED->REFUTED. Review found the proof does not only falsify C9, and the reason C4 follows is stronger than under-reporting: the two claims are about the same commit, and one run falsifies both - a transaction carrying offset 1's output while committing offset 1 rather than 2. Leaving C4PROVEDbeside a refuted C9 would have had the register assert and deny one observation. C4's own exemption does not cover it: that exemption is marker-delivery ordering within a commit, and here the offset is never committed by that transaction at all. Both keep their control-loop proofs verbatim, because that half still holds.RESULTS_EXACTLY_ONCE_UNDER_FAILUREis deliberately leftPROVED, with a scope note. The route from the omitted offset to a duplicated result is sound - redelivery, re-produce, duplicate - but no duplicate has been observed, and C14's own record says its RED and GREEN were each seen rather than argued. Refuting it on reasoning would make it the register's first argued status and break the observed-versus-argued distinction that is the register's whole value. The claim now names what would settle it: a broker-level rebalance reproduction showing a duplicated result. Operator ruling.ParallelConsumerOptionsis deliberately left alone.Status.REFUTEDsays the disposition (correct the docs or file the defect) is a triage decision; the note is the defect being filed, and softening the promise is the wrong half of that choice to take unilaterally.@Quarantined, not@Disabled, so the proof keeps executing and the lane will demand its removal the day it passes. It is deliberately not@ProvesClaim: the coverage guard rejects a proof the gating lanes exclude, and is right to, so C9 keeps its enforced coverage from the two control-loop proofs that really run. Registry entry added, with the unusual "why quarantined rather than fixed" spelled out.STRATEGY.mdsaid in terms that no claim in the register is refuted, and the README's machine-checked list carried the guarantee unqualified. Both corrected - a register written to fire against us is worth nothing if the finding gets softened instead of published. The README caution names both refuted guarantees and states that the front-page exactly-once promise is not refuted, so nobody infers the widest reading; theSTRATEGY.mdparagraph names the refuted claims instead of counting statuses, because the count it carried would have been wrong the first time one moved. README regenerated from the template, never edited directly.Not established, and deliberately not claimed
onPartitionsLostdoes not commit at all so it looks unaffected, but it was not tested.Settled after review, and recorded in the note rather than only here
Where the fix belongs, checked against #408 rather than guessed. That PR declines the revoke commit only when the transaction lock is contended - its own amended test accepts "committed inline if the dwell had already ended". The uncontended inline commit is exactly this defect's path, so #408 neither narrows nor widens it. What it establishes is machinery a fix reuses. The recorded position: decline unconditionally in transactional mode, as its own PR cut from master, resolving the collision on
tryCommitOffsetsOnRevokeat merge rather than by relocating either change.The one question that decides whether that fix is correct is not established, and the note says so in terms. On decline, what becomes of the transaction's already-produced output - aborted (exactly-once preserved) or committed later without the revoked offset (the same defect through a different door)? #408's record is silent on it. It has to be run, not argued; the un-quarantined proof plus a broker-level no-duplicate check is the instrument.
The concurrency annotations cannot express this yet, and declaring one would make it worse. RacerD consumes a
@ThreadConfineddeclaration without checking it, so a confinement claim over the state the drain mutates would silence the detector rather than inform it. #433 hit this same seam from the other direction while confininglastCommitTimeand backed off for the same reason - independent corroboration. The rule lives in the coreAGENTS.md; the note carries only what is specific to this defect, including the obligation on whoever closes it: the declaration and its runtime assertion go in the same change as the fix.A review-route finding rode along, because this PR is where it recurred. The dispatch route to the automated reviewer concluded
successand posted nothing, for the second recorded time; the mention route posted within minutes. Recorded in the solutions write-up with what the second occurrence isolates, anddocs/ci.mdnow carries the consequence beside the dispatch command.Checklist
STRATEGY.md,src/docs/README_TEMPLATE.adoc(+ regeneratedREADME.adoc),docs/quarantined-tests.md, and the tracking notedocs/features/- N/A - no feature; the user-facing consequence is a CAUTION in the README's transactional section, which is where that guarantee is publishedProducerManagerTest: the quarantined proof and its controldocs/inflight/working note started at the PR's first commit -docs/inflight/core-revoke-commit-skips-the-work-mailbox-drain.md, carrying the settled diagnosis. It shares a slug with the pre-existing note onorigin/fix/803-bound-transactional-revoke-wait, deliberately: same subject, so the branches conflict onto the settled version rather than silently duplicatingce-simplifyandce-code-reviewlocally - N/A - would rather spend the review on the PR;@claude review thishas not been requested yet, and is the user's callVerification run locally:
bin/check-all.shclean;bin/ci-unit-test.shBUILD SUCCESS with the quarantined arm correctly excluded from the gating lane; the quarantine lane re-run confirms it still executes and fails-as-expected.