Repository navigation
fix(core): stop transactional batches failing and reprocessing records that succeeded - #257
Merged
Merged
Conversation
…s that succeeded With a transactional producer and batchSize >= 2, pollAndProduceMany handed records back for a second delivery even though the user function had processed them fine, and logged an ERROR blaming the user function for every batch. Only transactional produce flows are affected; batchSize 1 and the non-transactional commit modes are not. One produce lock is acquired per PollContextInternal, whatever the batch size, but the release ran once per *record*: WorkContainer#onPostAddToMailBox fired from addToMailbox for each container in the batch. The second record's release found its thread holding zero read locks, so ProducerManager#ensureProduceStarted threw "Need to call #beginProducing first". That landed in runUserFunction's catch-all, which marks the whole batch failed and returns it to the mailbox - and if the controller drained the mailbox after that marking, the record came round again. cleanUpContext is now the single release point, and the lock is handed over by PollContextInternal#takeProducingLock rather than merely read, so a second release is a no-op instead of an IllegalMonitorStateException on a read lock the thread no longer holds. It still goes through ProducerManager#finishProducing, keeping the ensureProduceStarted check on the one remaining path. Releasing from the finally is also strictly more correct than the per-record hook it replaces: it runs after the *whole* batch reaches the controller's inbound queue, where the per-record release freed the lock after the first record and left the rest of the batch exposed to the commit window this lock exists to close. Rejected: clearing the Optional at each of the two old release sites. That makes the second call harmless but keeps the release point at the first record of the batch, so it fixes the crash and leaves the EOS window open. Diagnosis. The prior investigation counted 340 acquires against 340 releases with zero exceptions and concluded no double release happened. Both halves of that were instrumentation artefacts. ProducingLock#unlock logs *after* produceLock.unlock(), so a release that throws leaves no line - the count saw only the successful ones. And the exception went into the Future from workerThreadPool.submit(), which is parked on WorkContainer#future and read by nothing in main, so it never reached a log either. Experiments, each against a control arm differing only in whether the lock was handed to the context. Unit: with the lock, the worker future carries IllegalMonitorStateException; without it, the future is clean. End to end through the real produce wrapper at 200 records: 5/5 runs fail without this fix (duplicate delivery plus 112 spurious failure logs) and 5/5 pass with it. The record count is load-bearing - at 6 records the redelivery race was lost 3 times out of 3 and an end-to-end test passed against the bug, while the ERROR log still fired on every batch. Both figures are recorded in the tests. Other instances of the class, a lock whose ownership crosses method boundaries with more than one release site: none. All 12 unlock() calls in main were checked - the 10 in RetryQueue are lock/try/finally within a single method, and releaseCommitLock already guards with isWriteLockedByCurrentThread(). This was the only one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
submitWorkToPoolInner parks each batch's Future on WorkContainer#future, which has a getter and is read by nothing in src/main. Anything thrown out of runUserFunction that the catch does not handle - including from inside the failure handler itself, and from the finally - therefore vanishes without a log line. This is the mechanism that hid the produce-lock double release for its whole life, so it is worth recording separately from the fix: the next exception thrown from that finally will be just as invisible. The two other submit() sites are supervised (BrokerPollSystem#supervise, and controlThreadFuture via isClosedOrFailed/waitForClose); the worker future is the only unsupervised one. Left open rather than fixed here. Making framework escapes visible is a behaviour change to failure reporting, not part of a bug fix, and the choice between reading the future somewhere useful and dropping the field entirely wants deciding on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…index misleads ParallelEoSStreamProcessorTest.inFlightMessagesCommittedIfProcessedDuringShutdown[1] failed once in a full-suite run and did not reproduce: 6/6 in isolation, 1 failure in 15 runs on the branch it appeared on, 0/4 on unmodified master in a same-magnitude sequential control. Recorded mainly for the trap. [1] is PERIODIC_TRANSACTIONAL_PRODUCER, not the consumer-commit arm, so this flake lands on whatever transactional work is in flight and reads as that work's regression - which is how it presented here. The branch was cleared on mechanism rather than on the counts: the test uses poll(), which never reaches processAndProduceResults, so no produce lock is ever set on its contexts. The note says plainly that 0/4 cannot rule out a ~7% flake, so nobody later cites the control as a clean bill of health for master. It shows the absence of an elevated rate, which is a different claim. 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)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
Claude finished @astubbs's task in 4m 33s —— View job Review
SummaryThe fix itself is sound. Making One concrete, verified issue: this PR fails the repo's own copyright header gate. Running This is a dedicated CI gate (
Everything else checked out
|
|
The Copyright header check was failing on all three files this PR added or touched, for two different reasons: PollContextInternal.java is upstream-derived and modified here, so it needs the "Modifications Copyright (C) 2026 Antony Stubbs and contributors" line alongside the Confluent header - the way this PR already did for WorkContainer.java. ProduceLockReleaseTest.java and TransactionalBatchProduceTest.java are new fork-original files, but were created from an existing file's header and so claimed a Confluent copyright they have no basis for. They now carry the fork header alone. Using 2026 rather than 2020-2026 because they are genuinely new this year; Quarantined.java is the precedent. bin/check-copyright-headers.sh: 3 violations -> 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
Fixed in 43f6a34. I reproduced it locally with Two different mistakes, worth separating:
One deviation from your suggestion: I used Thanks for actually running the checker rather than eyeballing the headers; that's what made this unambiguous. |
Owner
Author
Assessed, not changing anything. Reasoning, so it's on the record rather than silently ignored: The two clone engines that actually measure duplicated blocks — PMD CPD and jscpd — both report no new clones introduced by this PR (duplication went 2.45% → 2.45% and 3.20% → 3.17%). This report measures whole-file similarity, which for JUnit tests in the same package is dominated by shared imports and scaffolding. The 36.5% between my own two test files is almost entirely that: they don't even share a harness ( The one genuinely repeated idiom is real: the anonymous Unrelated observation while I was in there, pre-existing on |
Review in progress
|
|
Claude finished @astubbs's task in 1m 5s —— View job Tasks
SummaryRe-reviewed the branch as of
No other files changed in this commit (
No blocking issues remain on this branch. |
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
The fix branch now has a PR, so the references become durable anchors instead of a local commit SHA that only exists on one machine. Several of the surrounding statements were stale rather than merely imprecise - the plan asserted the branch was unpushed and had no PR, and used that to argue the quarantine hatch was unavailable. Both are now false. Also records the half of the calibration that was still owed. Applying only #257's three src/main files to this branch and forcing the disabled arm to run takes the class from one error in 178s to 5/5 passing in 72s. So the RED/GREEN pair is now observed on both sides rather than assumed from a shared root cause - which mattered, because that PR was written for the duplicate symptom before the stall was known, and nothing had established it cured the stall too. The inflight entry carries two notes for whoever lands it: its PollContextInternal.java fails bin/check-copyright-headers.sh for a missing "Modifications Copyright" line, which is the whole of that PR's red Copyright check; and the 340/340 measurement recorded there should be deleted rather than corrected, because it was taken at batchSize = 1 where the defect cannot fire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nJJX2zU3hqAh89rNWSvkq
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
…C14 to proved #257 is merged into this branch, so the arm it blocked is switched back on. The whole class now runs 5/5 in 72s where the batched scenario previously took 178s to fail with the source offset frozen at 3 of 201. C14 moves from REFUTED to PROVED, and the note keeps the history rather than tidying it away: the RED was observed before the fix existed on this branch and the GREEN after, so the defect itself served as the negative control. That is stronger evidence than an injected fault, because nobody chose it to be convenient - it was found by the test and then fixed. STRATEGY.md follows the register rather than leading it. The headline is promoted, with the caveat that must travel with it: C2 and C7 are still refuted, because a terminally failed send in a pollAndProduceMany result set leaves a PARTIAL set visible. So the claim we can make today is exactly-once with per-key ordered concurrency verified across crash and replay - not unqualified all-or-none for multi-record result sets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nJJX2zU3hqAh89rNWSvkq
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
C13 says enabling eager processing "may cause side effect replay when the record is retried, otherwise there is no replay". Both halves are now observed against one identical forced trigger, which is what makes it a proof of the OPTION rather than of the trigger. The trigger holds the write side of producerTransactionLock shut and waits until a record's acquisition is OBSERVED to time out, so the hold's length is a measurement rather than a guessed sleep. Taking the lock from outside, instead of stalling a real commit, is deliberate: a stalled commit occupies the control thread, which would then never dispatch the victim to a worker at all. The hazard that creates - acquireCommitLock throws ConcurrentModificationException if another thread holds the write lock - is closed by taking the hold only while the arm is quiescent, plus an interlock that counts any commit attempted during it, so a mistake reads as a legible message instead of an instance death. Observed: strict takes the produce lock BEFORE the user function (hold count 1 sampled inside it) and runs the victim once; eager takes it AFTER (hold count 0) and runs the victim twice. Both arms must be seen to have raised the produce-lock timeout before any count is believed, so "no replay" is a statement about a record that demonstrably was retried. Three controls, each predicted first. Flipping the eager arm's one term goes red on lock placement. Flipping it and removing that assertion so the run reaches the counts collapses the replay to 1 while the trigger fires identically - which is the control that attributes the replay to the option rather than the timeout. Removing the hold entirely fails the non-vacuity guard rather than passing green on a record that was never retried, and the interlock caught the commit attempt, proving that diagnostic is live. Worth recording because it is easy to conflate: the discarded attempt fails before produceMessages, so the output topic holds one result per input record in both arms. What eager processing costs is the user's side effects, not duplicate output. Also repoints three references from the inflight entry the #257 merge deleted to the solutions write-up that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nJJX2zU3hqAh89rNWSvkq
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
…l be found #29 is the cautionary example this entry exists to avoid: a real fix for the only upstream-verified bug, blocked for months because the next step lived in someone's head rather than in the ledger. Records the three-PR chain and why the dependency gate holds it in order, the two pre-existing main-code holes that need their own PR, the register hardening ranked by how much false assurance each item buys, and the one place this work meets the #29 backlog: Phase B's chaos scenario must follow it, because calibrating SLOs against a master that still deadlocks would fold that defect into the baseline and then report the live bug as in-SLO. Also notes #257's own description understates its severity - it says duplicates, the battle test established a stall - which matters because releases after 0.6.0.0 generate their notes from the commit log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nJJX2zU3hqAh89rNWSvkq
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
…his proved Eight issues across two trackers touch transactional mode. Anyone arriving at #262 would otherwise have to rediscover which of them this work answers, which it leaves open, and which it deliberately did not test. The table says so per issue, including the uncomfortable one: #44 / confluentinc#803, the only issue upstream ever labelled a verified bug, is NOT tested here. The chaos scenario that would reproduce it is deferred behind #29, because calibrating SLOs against a master that still deadlocks folds the defect into the baseline. It also records three hang-shaped failures that are genuinely different and have already been confused once: the #44 commit-lock deadlock, the batching stall #257 fixed, and the poisoned-transaction wedge. The distinguishing evidence is what the logs do NOT contain - the stall has no commit-path errors at all, because commits were never attempted rather than attempted and failing. Two adjacencies worth someone's time, neither tested: confluentinc#887 reports a failed batch always retried with the same records, contradicting the batching docs and distinct from C3's crash-and-replay recombination; and confluentinc#144 asks for exactly the transaction-failure handling whose two mishandlings this work found. Written because the last gap of this kind was one unopened link away - the #44 mirror names confluentinc#809 and confluentinc#833 and I had not read either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nJJX2zU3hqAh89rNWSvkq
2 of 4 tasks
This was referenced Aug 10, 2026
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
astubbs
added a commit
that referenced
this pull request
Aug 13, 2026
Master's package rename (#294) and this branch's rename sweep did the same move independently, so most of the 24 conflicts were duplication artefacts rather than disagreements. Those took master: the eight logback configs (this branch had replicated master's csid deletion in a32d3cf, and master additionally dropped a commented logger for the ParallelConsumerTestBase it deleted), bin/rename-packages.sh, Documentation.java, TestConventionRules.java, OffsetEncodingBackPressureTest.java, the metrics TestConventionsArchTest, and the two .idea run configurations master removed. Both README files take master's fork-summary paragraph. This branch's line claimed the Java package was unchanged from upstream, which the rename made false, and master's replacement carries a rename-packages freeze marker so the old spelling survives the script. STRATEGY.md followed the instruction its own stub carried: #223's full document has landed on master, so this takes that version and re-applies the positioning section that depends on this branch's test results. The stub also asked that "Target problem" name the second gap - Share Groups cannot acknowledge inside a transaction, not just deliver out of order - so that sentence is amended rather than left naming only the ordering half. WorkContainer drops master's PollContextInternal and ProducerManager imports; the release they served moved to cleanUpContext here, and nothing in the merged body references them. docs/refactoring.md keeps this branch's deletion of the produce-lock double-release section, which is the open question this branch answered, and docs/inflight/bug-producing-lock-double-release.md stays deleted for the same reason - #257's fix is merged into this branch. The quarantine lift is the substantive part. Master quarantined ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect with fixedBy = "#262", and this is that PR: 517afd0 anchors the elapsed measurement to a nanos stamp taken before schedule(), which is the helper defect the entry describes. Per the annotation's rule 3, the owning PR deletes the annotation and its registry entry in the same commit as the master merge, atomically restoring the test to the gating lane. The test-untracked-ci-flakes note is updated to record the lift, and to hand the #265 collision to that PR now this one has landed first. Verified locally on Temurin 17: all modules test-compile, ProducerManagerTest 6/6 with 0 skipped (so the un-quarantined test genuinely runs in the default lane), the quarantine contract and convention suites 36/36, and check-quarantine-registry, check-copyright-headers, check-docs-data, check-issue-refs, check-shell-sigpipe and check-action-versions all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Heads-up from #29, expected to land before this one. Two things that touch the produce/commit lock area you are working in. 1. Two different locks are both called "commit lock", and they are ~800 lines apart in one file.
They are contended by different threads for different reasons, so a report of "commit lock timeout" is ambiguous between them. That ambiguity has already caused a real mis-triage: #44 (confluentinc#803), titled "Transactional Producer instance gets timeout getting commit lock while second instance starts", was attributed to #29's fix. It is the producer lock, and #29 does not touch it. Re-triaged 2026-08-18. The underlying failure is an unbounded 2. #29 changes close-path behaviour you may be relying on.
If #29 lands first, none of this should conflict with the produce-lock work — but the second point changes what can throw from |
astubbs
marked this pull request as draft
August 20, 2026 04:08
3 of 4 tasks
Owner
Author
|
Agent-written comment (posted via Antony's account while surveying where the Lincheck lane from #347 should point next). The produce/commit lock pair is a Lincheck target, and it is the one #335 explicitly does not cover#335 fixes duplicate processing in dispatch (the check-then-act work claim) and says of this defect:
Sharing no code means it needs its own harness — closing the dispatch claim does nothing for this one. Why this is a good Lincheck shape rather than just an important bug:
For ranking: I would put this behind #335's Caveat from #347 so a harness is not priced wrongly: its stress-arm hit rates were machine-dependent by 3.4x (likelihood-ratio p = 0.011), so a bound priced on one machine is only a claim about that machine. Prefer the deterministic model checker for a lock protocol; it needs no bound priced at all. |
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…order this PR's next tasks Puts the detector question at the top of the PR note's task list, with the reason it outranks the rest: the 2026-08-25 Class 2 demotion left the gating liveness claim resting on NO_PROGRESS and INSTANCE_STALL, and 2026-08-28 then showed NO_PROGRESS also over-fires on runs that are merely slow. Over-firing and under-firing together are worse than an absent detector, because the suite reports green on its silence. The script classifies every FAILING replay by what actually caught it - the detector itself, a different probe, or nothing but the end-of-run correctness ledger. That last category is the one the question is about: something went wrong and the liveness detectors said nothing. A passing run is explicitly not a data point, and the classifier is not treated as an oracle - an unclassified failure is flagged to be read by hand. It reads the siloed probes.log rather than the raw run log, which is the first use of the streams landed earlier today for the purpose they were built for. The task list also records what NOT to do next. The transactional revoke wait carries #44 - the only issue upstream ever labelled a verified bug - but #257 and #262 are open over the same code, and #262 sets out to prove or falsify every documented transactional guarantee. Chasing the wait first means resolving against a moving target and possibly answering a question those PRs reframe.
Brings in 191 commits, including the io.confluent -> bz.stub package rename that both sides had already run, so the TestConventionsArchTest files conflicted rename/rename on the right paths rather than pairing across modules silently - the case AGENTS.md's IN FLIGHT section describes. 87 conflicts. 82 were rename artefacts and resolved to master's side, on the rule that master's rename is applied to current content while this branch's was applied to content from 191 commits ago. Checked rather than assumed: the merged tree now differs from origin/master by exactly this PR's own files and nothing else. THE MERGE WAS SEMANTIC, NOT MECHANICAL #267 landed on master while this branch was open and built a safety net AROUND this bug while it was still open: ProduceLockNotHeldException, the failFatallyOnUnmailboxableRecord guards, and the addToMailbox throws contract, all naming WorkContainer#onPostAddToMailBox and docs/inflight/bug-producing-lock-double-release.md as the live route. This PR deletes both, so those references would have been left pointing at nothing - and bin/check-file-refs.sh would have failed on the note. - cleanUpContext uses PCInternalRuntimeException; master renamed InternalRuntimeException. - The guard comments in core, ExternalEngine and vert.x now say the route was closed by this PR rather than naming it as reachable. The guards themselves stay: addToMailbox is a protected extension point and ExternalEngine overrides it to return the dispatch permit, so a subclass can still throw there. What changed is that PC no longer has a KNOWN invariant on that path, not that the path is infallible. - Five live inflight notes repaired. Two of them argued FROM the open question, so they are rewritten to say what is now true rather than silently relinked - core-unmailboxed-container-recovery's option 1 gets weaker once addToMailbox is a bare queue add, and test-lincheck-lane-open-items loses the live bug that was most of its item 3's appeal. - The dated plan doc gets a history pointer, not a rewrite, per docs/citations.md; its sections stay as written because they are a record of what was true then. VERIFICATION Full unit suite green across all modules, zero failures. Ablation re-run on the merged tree, because master's changes could have made these tests vacuous: with the fix reverted, ProduceLockReleaseTest fails with the exact mechanism (IllegalMonitorStateException: attempt to unlock read lock, not locked by current thread) and TransactionalBatchProduceTest times out with records unprocessed. They still test what they claim. ReactorPCTest logs a dispatch-ceiling overshoot from master's own #342 work and passes. Cleared on mechanism, not on counts: its base is PERIODIC_CONSUMER_SYNC, so no produce lock is ever set on its contexts and both the old and new release paths are no-ops for it. The defect-class sweep was re-run against merged master: 14 unlock() calls in src/main now, not the 12 the PR body reports. The new one is ProducerManager#releaseProduceLock, a protected seam with a single caller inside finishProducing, so it is not a second release site and the "none found" conclusion holds. Six self-references to #257 and to this branch are rewritten in post-merge terms and marked, two of them inherited from master and only reachable because this is the PR whose number they name. pr-strategy-doc-merge-triggers listed the branch as a pending trigger; it now records the trigger as fired. bin/check-all.sh: 15 gates run, 15 pass.
🧪🔒 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 |
🟢 Throughput — OK
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) How this is derived, and what it cannot tell youBy 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 1 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: 54301eb |
Owner
Author
Read per
|
… failure did NOT prove RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed failed once on this branch's CI, 1 of 161 integration tests. Recorded per the ledger's own rule - the evidence expires with the logs, so a sighting has to be written down before the branch that saw it merges. WHAT FAILED IS THE PRECONDITION, NOT THE REPRODUCTION. The assertion was the stage-2 setup guard "control thread must reach the mid-loop pause point (offset 25)"; awaitPausePoint(30, SECONDS) returned false, so the confluentinc#909 stale-resident assertion the IT exists for was never evaluated. That is worth stating explicitly because the note next door, test-909-reproduction-cannot-observe-the-collision.md, is the opposite worry - the same test going silently green with the defect branch unexercised - and conflating the two would send the next reader at the wrong mechanism. RULED OUT AS THIS BRANCH'S DOING, ON MECHANISM RATHER THAN COUNTS. The IT is PERIODIC_CONSUMER_SYNC and drives pc.poll(...), never pollAndProduceMany, so beginProducing is never called and no produce lock is ever set on its contexts - both paths this branch changed are Optional-guarded and are no-ops here, before and after. The IT is untouched by this branch. master passed this lane at 54301eb, the exact base the failing run merged against; one green run per commit cannot rule out a low-rate flake, so that is corroboration and the mechanism is what clears the branch. The ambient probe reported "probe clean - no rebalance dwell, no lag stagnation, no frozen partitions", pointing away from broker contention and at the test's own 30s budget for a hand-orchestrated race, under forkCount=4 on a shared runner.
astubbs
marked this pull request as ready for review
September 1, 2026 14:19
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…mode-battle-test #262's declared parent had moved a long way: #257 has since merged master up to #403, so taking it also takes nearly all of master, and 262's own CONFLICTING state against master goes with it. 846 files, nine conflicts. Package rename: both sides were already on `bz.stub.*`, so the merge was ordinary and `bin/rename-packages.sh` was not needed. Verified by `git ls-tree -d` on all three refs before starting, not assumed. Conflicts, and which side won: - ProducerManager - TAKE 257. Master has already absorbed 262's produce-callback fix and improved it: the callback is hoisted to a `sendCallback` field with the reasoning in javadoc rather than a comment, and it carries the fact 262 established last (Kafka's own ProducerBatch catches and logs whatever a callback throws, so the throw was only ever observable on the synchronous doSend path). 262's local copy is deleted as the duplicate it now is. `InternalRuntimeException` is `PCInternalRuntimeException` on master; the three places 262 still named the old class are renamed. - AbstractParallelEoSStreamProcessor#cleanUpContext - BOTH. Code takes 262's catch-and-log, which master does not have: this runs in runUserFunction's `finally`, and a throw from a finally REPLACES the exception the catch above is propagating, destroying the user function's real failure. Javadoc takes 262's correction (an ExternalEngine can never reach here holding a produce lock - its constructor rejects PERIODIC_TRANSACTIONAL_PRODUCER, so 257's claim that this is that path's only release is wrong) plus 257's new paragraph on the lock being taken rather than read. - KafkaClientUtils#createNewProducer - BOTH, and neither could just win. 262 added a typed (mode, transactionTimeout, stableTransactionalId) overload; master added (mode, Properties overrides). The auto-merged body already used all three parameters, so both overloads now delegate to one private four-arg builder. - TransactionalPartialResultSetIT - add/add, TAKE 257 plus one thing. 257's is the reviewed descendant of the same test: it pins MAX_REQUEST_SIZE_CONFIG rather than inheriting it, holds POISON_KEY to NONE rather than all-or-none (its oversized result can never be sent, so isAnyOf(0, n) would also accept a full set that cannot physically exist), and uses commons-lang3 `repeat` instead of a local Java-8 helper. Only 262's `@Timeout(600)` guard is carried over. - ProducerManagerTest - TAKE 262's extracted `acquireProduceLockInto` / `assertProduceLockStillOwnedByContext` helpers over 257's inline copies of the same code and comments; the fact 257's comment had and the helper javadoc did not (#257 made cleanUpContext the ONLY release point) is folded into the javadoc. - ProduceLockReleaseTest, WorkContainer - imports only. - docs/quarantined-tests.md is now EMPTY, by two independent routes meeting. #351 diagnosed and fixed OffsetEncodingBackPressureTest on master (it asserted an offset back pressure exists to stop advancing), and this branch performs its own rule-3 re-enable of ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect. No @Quarantined annotation remains anywhere in the tree and the registry check confirms 0 entries. - docs/inflight/test-untracked-ci-flakes.md - master's rows (both entries 262 knew about are fixed and gone; simpleBatchTest and processInKeyOrder are new) plus 262's now-current account of the BlockedThreadAsserter collision. Fixed a blank line that was splitting the table in two. Worth flagging for the handoff: master's new `processInKeyOrder` section is about one of the three unexplained failures this branch pushed with at 48d210f, and says it is a solved flake still firing rather than a fresh one. - docs/inflight/pr-blockers-and-collisions.md - master's collision list, plus 262's transactional-stack section updated for reality: #261 merged on 2026-08-14, so the chain is now two PRs, not three. 261 is named rather than deleted because a reader who knows the stack as three links needs telling which one is already master. Verified locally: full-reactor `test-compile` green (main and both test source roots), check-quarantine-registry 0 entries, check-issue-refs clean. The unit suite has not run yet.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…ound Merging #257 brought master's newer repo-hygiene gates onto this branch, and they immediately found real debt in this branch's own notes - not in master's. `bin/check-all.sh` goes from 4 failing gates to 0. **check-inflight-tags** - nine notes here predate the tag scheme and carried no `inflight-type`, so the session index could not sort them and they were invisible at session start. Tagged by CONSEQUENCE per docs/inflight/AGENTS.md, not by filename prefix: the swallowed produce failures are `data-loss` (a batch marked succeeded for records never produced), the commit-interval override is `config-lie`, the poisoned transaction is `stall`, the unclosed Kafka clients are `crash`, and the two "tell someone" notes are `stranded-work`. Register hardening is `task` + `test-debt` rather than `misdirection`: the false-green at the top of its ranking is the strongest item, but `misdirection` is a bug impact and the note is a list of decisions. **check-branch-self-reference** - 24 sentences described #262 or its branch in the present tense, which the merge falsifies. Rewritten in post-merge terms and attested, not marked-and-left: the transactional-stack section now states what each PR IS rather than whether its check is red, the flakes ledger records the #265 collision and the rule-3 lift as history, and two "on this branch" phrases that named nothing are gone. `branch-transactional-battle-test.md` takes `post-merge: exempt-file` because being #262's issue map is the file's whole subject. One claim was wrong rather than merely stale, and is corrected: `pr-strategy-doc-merge-triggers.md` would have read as though the exactly-once headline had to move. It does not. Two claims were refuted against a master without #257 and both read PROVED with it merged in, which is what STRATEGY.md already says. **check-file-refs** - 22 path citations in the dated battle-test plan still said `io/confluent/`, broken by this branch's own rename commits. Repaired to `bz/stub/`, which docs/citations.md requires: the address of the thing the record pointed at may be fixed, its claims may not. Three of them name files the plan proposes to create for the deferred Phase B, so those paragraphs carry `file-refs: N/A` instead - a proposal is not a citation. **Deleted `handoff-262-master-merge.md`.** It was written for an agent picking this branch up cold, and this merge answers three of its four open items: master's #351 fixed the back-pressure flake, its de1620d un-quarantined PCMetricsTest, and it absorbed the produce-callback fix the one unresolved review thread was about. #261 merged, so the fourth is half gone. Its one piece of evidence that was not recorded elsewhere - the three `(CommitMode)[3]` failures seen locally on 2026-08-13 - is folded into the flakes ledger beside master's own independent `[3]` sighting, where someone looking for that failure will actually find it. The two corroborate: master saw `[3]` fail on a branch carrying no main Java, and this branch's own main-code changes are ruled out by grep. The assertion messages from the local run were not kept, so it is NOT established that the two are the same failure, and the note says so. The control that would settle it - the same suite on plain `origin/master` - is still unrun.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
#257 merged while this branch was merging its BRANCH, so master now carries the same work as a squash (b36ad94) that this branch already has as a real merge. Six conflicts, every one of them the same content arriving twice - and in five of them this branch's copy is the one with the later edits, because it was reconciled against master an hour ago and the squash was not. - **PollContextInternal** - keep this branch's overwrite guard on `setProducingLock`. #257 does not have it; it was added here, and the squash naturally reads as a deletion. - **cleanUpContext** - keep this branch's catch-and-log and the ExternalEngine javadoc correction, for the reasons recorded in the #257 merge: a throw from a `finally` replaces the user function's real failure, and an ExternalEngine can never reach that code holding a produce lock because its constructor rejects PERIODIC_TRANSACTIONAL_PRODUCER. - **ProducerManagerTest** - keep the extracted `acquireProduceLockInto` / `assertProduceLockStillOwnedByContext` helpers over the squash's inline copies. - **ProduceLockReleaseTest** - add/add. Keep this branch's, which is the same test asserting through `ManagedTruth`'s `hasNoProduceLockHolders()` instead of reaching into `getProducerTransactionLock().getReadLockCount()`. Same assertion, checked the subject to be sure: `hasNoProduceLockHolders` delegates to `hasProduceLockHoldCount(0)`, which reads that same count. - **docs/inflight/test-untracked-ci-flakes.md** - take master's new `RegistrationRaceStaleResidentIT` row, minus the blank line it arrived with, which would have split the markdown table in two. - **docs/inflight/core-241-tx-commit-failure-taxonomy.md** - keep this branch's post-merge rewrite and record that #257 has now merged as well. Also inherited: #401's throughput check (a sixteenth gate, reporting nothing-in-scope here) and an #183 solutions write-up. Neither touches this work. Verified locally: full-reactor `test-compile` green, `bin/check-all.sh` 15 passed / 0 failed. The unit suite is re-running against this head.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
Checked every issue and PR the transactional registers name, against GitHub rather than against memory. Four claims had gone false, and one of them is the kind that stops a reader looking. **#177's mirror is closed while confluentinc#833 is open.** The map listed it as open; it was closed as completed on 2026-09-01 by the fork owner, with no reason on the issue and no linking commit, and the upstream report it mirrors is still open and still unanswered. The last comment on the mirror, five days earlier, argues the opposite - #267 renames the type in the title but does not fix the report - and ends "Left open". Nothing written down says what changed. That matters beyond bookkeeping: AGENTS.md makes the fork mirror the owner of an upstream issue's live status, so the mirror is currently asserting a status confluentinc#833 contradicts. Both `branch-transactional-battle-test.md` and `next-tell-809-833-they-are-likely-fixed.md` now say so, the latter with its own section, because that note's whole purpose is "tell the reporters" and closing the mirror is not telling anyone. Task 3 in it is half unreachable now - commenting on a closed mirror reaches nobody - and task 4 has grown to include reopening it or recording why it closed. No reason is invented here; only what is observable is written. **#29 targets `master`, not `master-confluent`, and is out of draft. #31 has merged.** The blockers register still said both target the pinned pre-rebrand mirror and needed retargeting. The retarget happened; what survives is the half that was never mechanical - #29's deadlock fix predates the internals #80 reshaped, so it needs reconciling rather than replaying. **Both of #262's parents have merged**, so the transactional stack section is rewritten from a land order into what it is now: history, plus one debt. **The debt is real and worth naming.** #257's merged commit message describes redelivery - "handed records back for a second delivery" - and never says *stall*. What #262 established is more severe: only a success marks a partition dirty, so every batch failing meant no commit was ever attempted and the source offset froze at 3 of 201. `git log -1 --format=%B b36ad94 | grep -i stall` returns nothing. Release notes are generated from the commit log, so 0.6.0.0 publishes the weaker claim unless the release section is amended or a follow-up commit says so. Also confirmed unchanged, so the map is not merely un-contradicted but checked: #44, #175, #189, #232 and #241 are all still open; confluentinc#809 and confluentinc#833 are both still open upstream; #100 is merged. `docs/inflight/handoff-262-master-merge.md` was already deleted in b99377b and is confirmed absent from the tree; this commit does not touch it.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…d I got its closure wrong Correcting my own claim from cfca837, which said #177's mirror was closed "with no reason recorded and no linking commit". That was true of the issue page and false as a conclusion: `git log --format='%s' origin/master | grep #177` resolves it in one command, and I stopped at GitHub instead of running it. Four commits titled #177 landed on 2026-08-19; the mirror closed on 2026-09-01. The fix landed and the closure follows from it. **The note's own diagnosis was also incomplete, and the fix says so in its own words.** It argued these reports are "plausibly already fixed by #100". `fe45fddfa` states that #100 and #108 removed two ways to kill the broker-poll thread, but broker-down, offset-encoding and authorization failures still produced the same symptom, and *fixing exceptions one at a time was never going to close the class*. The mechanism the note identified was right - the only producer of commit responses dies, so every later sync commit reports a timeout describing nothing - and the inference from it was wrong. The fix is not one more exception handled; it is that the death is published: `BrokerPollSystem`'s exceptional exit calls `ConsumerOffsetCommitter#notifyPollerDied`, so a waiter is released at that moment with the poller's own exception as the cause. The original reasoning is kept in the note rather than deleted, because it is what found the mechanism. **Its task 1 is done.** The message no longer interpolates the unrelated `DEFAULT_TIMEOUT` constant - it reports `commitTimeout`, the budget it actually waited on. That is not bookkeeping: both reporters quote `PT30S` and reason from it, and the shipped default was three times smaller, so their own analysis rests on a figure the code was inventing. `d9a9a4408` went further and added `PollThreadStallDiagnosis`, so a recurrence now arrives with DEADLOCK / BLOCKED / WAITING / RUNNABLE attached instead of needing this note's argument repeated. **What is actually left is the telling.** confluentinc#833 and confluentinc#809 are both still open upstream with no reply. Replying is now backed by a named fix rather than a hypothesis, which is exactly the condition AGENTS.md sets for commenting upstream at all. **confluentinc#809 is separately unclaimed** - no commit on master names #175 or confluentinc#809, and its stack is inside `close()` -> `doClose` -> `commitOffsetsThatAreReady`, a different entry point. `notifyPollerDied` plausibly covers it for the same reason it covers the other, but that is the inference this note already got wrong once, so it is left as read-the-close-path rather than asserted. Renamed `next-tell-809-833-they-are-likely-fixed.md` to `upstream-tell-809-833-the-hang-is-fixed.md`. `next-` is a status, and docs/inflight/AGENTS.md's prefix table - which arrived with the #257 merge - says a prefix names an AREA, never a status; `upstream-` is the row for mirrors and coverage obligations. The title asserted "plausibly" and "nobody has said so", both now wrong in different directions. Nothing enforces that prefix vocabulary, and four other notes are still off it - `next-`, `issue-` and `handoff-` prefixes, all inherited rather than added here. Left alone: a sweep is its own change, and this one is about what the note SAYS.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…s-bug Master moved during merge prep, and one of the three new commits touches this PR's core file. #257 changes AbstractParallelEoSStreamProcessor: it makes cleanUpContext the single produce-lock release point and removes the release from addToMailbox, rewriting the javadoc around that path. This PR changes the COMMIT lock in the same class. Different locks, adjacent code - which is exactly the case where a clean textual merge proves nothing. Merged here rather than left to the squash, so the combination is resolved and tested by someone rather than assembled at merge time and shipped untried. VERIFIED ON BOTH SIDES, NOT JUST MINE Mine: the ReentrantLock commitLock and tryCommitOffsetsOnRevoke are present, and the eager log.trace form has not come back (the supplier form survived). Theirs: onPostAddToMailBox is gone as #257 deleted it, its rewritten javadoc is present, and PollContextInternal#takeProducingLock is intact. Then run, not assumed: ProduceLockReleaseTest - #257's own regression test - passes against this merged tree, alongside HotPathLogArgumentsAreDeferredTest and ProducerManagerTest. Checked from the surefire reports rather than the exit code, because a run that executes no test also exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
Four commits, none touching this branch's files. Read before merging, per AGENTS.md: 1001c69 ci: PR CI comments say what they mean, and survive their own failures (#407) 000de5d ci(lincheck): run the lane that has never run, and flip the arm it found dark (#404) b36ad94 fix(core): stop transactional batches failing and reprocessing records that succeeded (#257) 625a8c1 docs(solutions) #183: a defect matches a report on direction and on the reporter's build The package-rename rule is still live on master, and does not bite here: this branch touches no Java and no package paths, and the sweep grep finds io.confluent only in the rename, copyright and mutation scripts that carry the pattern as data. Worth recording rather than passing over: #404 lands the 2026-09-01 sightings into docs/inflight/test-load-tightness-flakes.md, so the committedOffsetRemoved flake this PR's CI hit is now tracked on master rather than only on sixteen unmerged branches. That was the worked example in docs/inflight-tool.md - the example stands as the record of what the tool found on the day, and the ledger has since caught up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTX8obQMsjs9kq2rkpU5cZ
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
Ten commits, no Java overlap - master's core work (#257's produce-lock handover, #393's thread-confined consumer) is nowhere near the offsets package this branch changes. All three conflicts were in docs, and all three were two ledgers being appended to from both sides on the same day. WHAT THE RESOLUTION KEPT Both flake ledgers gained entries from master AND from this branch on 2026-09-01, so every conflict resolution keeps both sides rather than choosing one: - test-load-tightness-flakes.md: master's inFlightMessagesCommittedIfProcessed- DuringShutdown[1] paragraph and this branch's committedOffsetRemoved[1] latest recurrence are about different tests and neither supersedes the other. - test-untracked-ci-flakes.md: both sides added a processInKeyOrder row for sightings on the same day from different branches. Merged into one entry rather than two, because two rows under one test name is exactly the conflation master's own section warns against. This branch's four sightings are folded in as a sub-section of master's [3] bullet, where they turn "seen once, not reproduced" into a control-armed contention finding - and master's isolated-run result stops reading as a contradiction, since isolation removing the failure is what the contention reading predicts. - refactoring.md auto-merged: master deleted the produce-lock double-release OPEN QUESTION entry because #257 answered it. TWO REPAIRS MADE WHILE IN THERE Master's table had a blank line between two rows, which markdown renders as two tables with the second missing its header. Closed up. The JStream entry said "delete this entry when #116 lands" - a marker docs/inflight/AGENTS.md explicitly forbids on master, because the merge is the moment nobody is looking here. Replaced with the migrate-rather-than-delete outcome that doc names, and the attestation block it opened is now closed rather than running into the next section's. INHERITED AND READ #403 makes Node the default for new bin/ scripts and deletes check-shell-sigpipe.sh; #382 rewrites the hook guards to derive identity from the command rather than the session; a new core AGENTS.md binds @GuardedBy to any field change in the engine. This branch adds no scripts and changes no shared field, so none of the three change what it does - recorded because a green build is not evidence the ground under a design held still. AND ONE FIX THE MERGE COULD NOT BE COMMITTED WITHOUT #403's new bin/check-source-patterns.mjs refused this merge commit, reporting seven of #381's experiment runners as new shell scripts and advising --no-verify. They are master's, already grandfathered there, and this branch never touched them. The cause is that its `added-files` scope is the union of committed and staged additions against the merge base - and while a merge is staged but not yet committed, HEAD is still the pre-merge tip, so every file master added since the branch was cut is an addition against that base. It corrects itself the moment the merge commit exists, which is why nothing on master has seen it: it fires only in the window where the fix has to be applied. Membership of origin/master settles it without needing to know about merges at all - a path already there is not new to this repository, whoever staged it - so the subtraction lives in bin/lib/added-files.mjs as a pure function of three lists. That placement is what lets it be tested by control pair like every rule in the table, instead of standing up a fixture repository, which is the reason the runner had no test to break. The four must-NOT-match cases are red-proven against the pre-fix implementation. Carried here rather than split out because the alternative was committing this merge with --no-verify, and a gate that fires on somebody else's merged work teaches exactly that bypass.
Merged
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
… is master (#417) #262 merged - `999c1c5c0`, `c6c2f41fc`, `eb9585260`, `beb01e1ce` are all on `master`, rebase-merged so each workstream kept its own commit. `branch-transactional-battle-test.md` was written for a reader arriving at that open PR, and `pr-blockers-and-collisions.md` still described the three-PR chain as having one member open. Both stopped describing reality at the merge. `docs/inflight/AGENTS.md` makes deletion the LAST of four outcomes, and gates it on nothing being left that is both true and unowned elsewhere. So each section was taken separately. **Migrated, because it was true and had no owner.** - *Three hang-shaped failures, told apart* -> `docs/solutions/runtime-errors/hang-shaped-failures-in- transactional-mode-told-apart-2026-09-02.md`. This was the note's only genuinely unowned content: the discriminator that an empty commit-error log is POSITIVE evidence for the batching stall, because commits were never attempted rather than attempted and failing, while the poisoned wedge fails every send loudly and the commit-lock hang blocks rather than fails. It is a solved diagnostic problem even though two of the three underlying defects are open, and `docs/solutions/` is what the prior-art checks grep. - confluentinc#830 -> `bug-eos-swallowed-produce-failures.md`. The closed upstream report on `InvalidPidMappingException` is field evidence that hole 1's trigger fires in production. Read the issue rather than carrying the note's one-line summary: it is `batchSize(1)`, `UNORDERED`, 0.5.2.8, and it asks for the producer to be closed and replaced - a larger design question than the rethrow that note proposes, and worth recording as distinct from it. - The C3 near-miss -> `core-189-batch-failure-granularity.md`. The register's C3 covers recombination across a crash and replay; #189 is recombination on an in-process retry. Without that stated, C3 reading `PROVED` reads as coverage #189 does not have. - confluentinc#40's new input -> `upstream-2023-sweep-manual-review.md`, which already flags that issue's design notes as the least examined of its four. The claim register is now the machine-checked baseline any per-record transaction design has to beat. **Deleted, because an owner note already says it - and says it better.** The issue map's remaining rows all duplicated a note that owns the issue, which is what `AGENTS.md` forbids on the grounds that duplicates drift apart. They already had: #44 -> `bug-857-transactional-revoke-wait.md`, #175 -> `upstream-175-sporadic-commit-timeouts.md`, #177 -> `upstream-tell-809-833-the-hang-is-fixed.md`, confluentinc#541 -> `docs/solutions/runtime-errors/revoke-path-commit-deadlock-between-poll-and-control-threads.md`, #241 -> `core-241-tx-commit-failure-taxonomy.md`. **The drift was not hypothetical, and the migration corrects it rather than carrying it.** The map said #44's fix "has now MERGED" in #29. Its owner note records that #44 was re-triaged OFF #29 on 2026-08-18: the AB-BA cycle's second edge lives in `ConsumerOffsetCommitter`, which `BrokerPollSystem` constructs only for the consumer-commit modes, so it cannot close in transactional mode at all - what #44 actually matches is the unbounded `while (isTransactionCommittingInProgress())` revoke wait, which #29 does not touch and which is still open. Reading #29's merge as closing #44 is now the first thing the new write-up warns against. *What the PR should carry* was deleted outright: it was instructions to the merge that has happened. **Kept, shrunk, in `pr-blockers-and-collisions.md`.** The transactional-stack section now records all three PRs as master instead of one as open. The debt it names is untouched and still live - #257's merged commit message describes redelivery and never says *stall*, and release notes are generated from the log, so the generator will publish the weaker claim until someone amends the 0.6.0.0 section or lands a correcting commit. Every other entry in that file, and the decisions-waiting-on-a-human subsection, are left alone. **Dangling references.** `grep -rn branch-transactional-battle-test` found exactly one citation outside the file itself, in `pr-strategy-doc-merge-triggers.md`, pointing at the note as "the issue map". It now points at `TransactionalClaim` / `TransactionalClaimCoverageTest`, which is what that sentence was really reaching for - the standing gate the same paragraph already names. No heading anchors were cited anywhere. `bin/check-all.sh` and `bin/check-issue-refs.sh` are green. Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 3, 2026
…esidentIT setup-guard flake What changed for a user: nothing - a ledger entry. The integration lane went red on this branch with one failure in 201 tests: RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResident\ MustStillGetProcessed, on "control thread must reach the mid-loop pause point (offset 25)". The ledger already carries that test from a single 2026-09-01 sighting on #257, with the same assertion and the same reading - the saturation/pause-point setup guard timing out, so the confluentinc#909 assertion the IT exists for is never evaluated. Why it is master-state and not this branch's: the identical failure hit feat/225-pc-built-producer at e35db1e seventeen minutes earlier, and four other branches passed the same lane inside the same 25 minutes (#428, fix/422-commit-interval-unset-detection, optimize/chaos-ci-perf, feat/225-producer-config). Two unrelated branches failing and four passing in one half-hour rules out both "this diff caused it" and "the runner was slow". Mechanism clears this branch independently, which is the part that actually settles it: the IT is PERIODIC_CONSUMER_SYNC driving pc.poll(...), so it never produces, never takes a produce lock, and never reaches the InvalidPidMappingException arm, ProducerManager#close or innerDoClose that this PR changes - and it fails in stage 2 of its own setup, before any close. Recorded rather than diagnosed or fixed, per this ledger's rule and the no-retry rule: the evidence expires with the logs, and the failing runs' 30.8s and 30.9s against 14-19s on the passing ones is the shape of a 30s budget waited out in full. Not quarantined - quarantine is master-state with evidence, and that call is not this PR's to make.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
With a transactional producer and
batchSize >= 2,pollAndProduceManyhanded records back for asecond delivery even though the user function had processed them successfully, and logged an ERROR
blaming the user function on every batch. Only transactional produce flows are affected -
batchSize1 and the non-transactional commit modes are not.
One produce lock is acquired per
PollContextInternal, whatever the batch size, but the release ranonce per record:
WorkContainer#onPostAddToMailBoxfired fromaddToMailboxfor each containerin the batch. The second record's release found its thread holding zero read locks, so
ProducerManager#ensureProduceStartedthrew"Need to call #beginProducing first". That landed inrunUserFunction's catch-all, which marks the batch failed and returns it to the mailbox - and whenthe controller drained the mailbox after that marking, the record came round again.
cleanUpContextis now the single release point, and the lock is handed over byPollContextInternal#takeProducingLock()rather than merely read, so a second release is a no-opinstead of an
IllegalMonitorStateExceptionon a read lock the thread no longer holds. It still goesthrough
ProducerManager#finishProducing, keeping theensureProduceStartedsanity check on the oneremaining path.
Releasing from the
finallyis also strictly more correct than the per-record hook it replaces: itruns after the whole batch has reached the controller's inbound queue, where the per-record
release freed the lock after the first record and left the rest of the batch exposed to exactly the
commit window this lock exists to close.
Rejected alternative: clearing the
Optionalat each of the two old release sites. That makes thesecond call harmless but keeps the release point at the first record of the batch - it fixes the crash
and leaves the EOS window open.
How this was hidden
The open question this closes had been measured before, as 340 acquires against 340 releases with zero
exceptions, and concluded that no double release happened. Both halves of that were instrumentation
artefacts:
ProducingLock#unlock()logs afterproduceLock.unlock(), so a release that throws leaves nolog line. The count saw only the successful releases.
FuturefromworkerThreadPool.submit(), which is parked onWorkContainer#futureand read by nothing insrc/main, so it never reached a log either.That second point is a live diagnostic hole independent of this fix, and is recorded in
docs/inflight/bug-worker-future-swallows-framework-exceptions.mdrather than fixed here - makingframework escapes visible is a behaviour change to failure reporting, not part of a bug fix.
Evidence
Each experiment ran against a control arm differing only in whether the produce lock was handed to the
context.
IllegalMonitorStateExceptionfailedAttempts=1End to end through the real produce wrapper (
TransactionalBatchProduceTest, 200 records):Deterministic in both directions. The record count is load-bearing: duplicate delivery needs the
controller to drain the mailbox after the worker marks the record failed, which is a race - at 6
records it lost that race 3 times out of 3 and an end-to-end test passed against the bug, while the
spurious ERROR log still fired on every batch. Both figures are documented in the test.
Other instances of the defect class
The class is a lock whose ownership crosses method boundaries with more than one release site.
None found. Every
unlock()call insrc/mainwas checked, and each is ruled out by shape ratherthan by tally - the ones in
RetryQueuearelock(); try {} finally { unlock(); }within a singlemethod;
ProducerManager#releaseCommitLockguards withisWriteLockedByCurrentThread()and throws ifnot held; and
ProducerManager#releaseProduceLockis aprotectedseam whose only caller isfinishProducing, so it is an override point rather than a second release site. The produce lock wasthe only one.
The related class exceptions swallowed by an unread
Futurehas a single instance, the workerfuture above. Every other
submit()site is supervised (BrokerPollSystem#supervise, andcontrolThreadFutureviaisClosedOrFailed/waitForClose).The sweep is stated by shape on purpose. An earlier version of this section counted the call sites,
and the count went stale the moment
mastermoved - it was right when written and wrong by the timethis branch merged
master, with nothing to signal the difference.Merging
masterwidened this PR, and the extra diff is not incidentalWhile this branch was open, #267 landed a safety net around this bug as an open question:
ProduceLockNotHeldException, thefailFatallyOnUnmailboxableRecordguards, andaddToMailbox'sthrowscontract all nameWorkContainer#onPostAddToMailBoxanddocs/inflight/bug-producing-lock-double-release.mdas the live route. This PR deletes both, so thosereferences would have been left pointing at nothing - and
bin/check-file-refs.shfails on thedeleted note. Repairing them is part of the merge, not separate work:
ExternalEngineand vert.x now record the route as closed rather thannaming it as reachable.
ProducerManagerTest's comments and one assertion message move with them:they named
onPostAddToMailBoxas the sanctioned release point, which is nowcleanUpContext. Itsassertion is unchanged and still passes - the lock must still be owned by the context when the user
function returns; only the name of the release point it defers to moved. The guards themselves stay.
addToMailboxis aprotectedextensionpoint that
ExternalEngineoverrides to return the dispatch permit, so a subclass can still throwthere. What changed is that PC no longer has a known invariant on that path - not that the path
became infallible.
cleanUpContextraisesPCInternalRuntimeException;masterrenamedInternalRuntimeException.merely linking it, so they are rewritten to say what is now true:
core-unmailboxed-container-recovery's "leave it and state the bound" option gets weaker onceaddToMailboxis a bare queue add, andtest-lincheck-lane-open-itemsloses the live bug that wasmost of its item 3's appeal.
docs/citations.md- itssections stay as written, because they record what was true then.
The ablation was re-run after the merge, because
master's changes could have made these testsvacuous. With the fix reverted,
ProduceLockReleaseTeststill fails with the exact mechanism(
IllegalMonitorStateException: attempt to unlock read lock, not locked by current thread) andTransactionalBatchProduceTeststill times out with records unprocessed.One CI flake seen after the merge, recorded rather than retried to green
The first post-merge CI run failed
RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed,1 of 161 integration tests. It is not this PR's doing, and the argument is mechanism rather than
counts: that IT is
PERIODIC_CONSUMER_SYNCand drivespc.poll(...), neverpollAndProduceMany, sobeginProducingis never called and no produce lock is ever set on its contexts - both paths this PRchanges are
Optional-guarded and are no-ops there, before and after. The IT is untouched by this PR,and
masterpassed the same lane at54301ebd, the exact base the failing run merged against.What failed was the stage-2 setup guard (
awaitPausePoint, "control thread must reach the mid-looppause point"), so the confluentinc#909 assertion the IT exists for was never evaluated - worth stating,
because
docs/inflight/test-909-reproduction-cannot-observe-the-collision.mdtracks the oppositeworry about that same test, and conflating them would send the next reader at the wrong mechanism. The
ambient probe reported
probe clean.It is recorded in
docs/inflight/test-untracked-ci-flakes.mdrather than retried away, per thatledger's own rule that a sighting has to be written down before the branch that saw it merges - the
evidence expires with the logs. The re-run passed it, so the sighting stands as a first sighting,
not a diagnosis.
Notes for the reviewer
addToMailbox(PollContextInternal, WorkContainer)now has an unused first parameter. It onlyever existed to reach the produce lock via the deleted
onPostAddToMailBoxcall. It is aprotectedextension point called from the vertx, mutiny and reactor modules, so removing it wouldedit three sibling modules for a signature change unrelated to this defect. Left deliberately;
happy to do it separately.
WorkContainer#onPostAddToMailBoxis deleted. It ispublic, but its only caller wasaddToMailboxand it did nothing except release the produce lock. There is no binary-compatibilityplugin in this build.
CHANGELOG.adocentry, perAGENTS.md: 0.6.0.0's section is generated from the commit log atrelease time and a PR never adds one. The fix commit body is written to be that raw material.
ParallelEoSStreamProcessorTest.inFlightMessagesCommittedIfProcessedDuringShutdown[1]failed oncein a full-suite run.
[1]isPERIODIC_TRANSACTIONAL_PRODUCER, so it looks like this PR'sregression. It is not: 6/6 in isolation, 1 failure in 15 runs on this branch, and 0/4 on unmodified
masterin a same-magnitude sequential control. What clears this branch is the mechanism, not thecounts - the test uses
poll(), which never reachesprocessAndProduceResults, so no produce lockis ever set on its contexts and both the old and new release paths are no-ops for it. Added to the
existing family in
docs/inflight/test-load-tightness-flakes.md, which states explicitly that 0/4cannot rule out a ~7% flake on
masterand should not be read as a clean bill of health.Checklist
docs/inflight/bug-producing-lock-double-release.mdremoved (perdocs/inflight/AGENTS.md, closing meansgit rm), the OPEN QUESTION entry dropped fromdocs/refactoring.md, the answer appended to the originating plan doc's §11, a new inflight note filed for the unread-Futurehole, and the shutdown-commit flake added totest-load-tightness-flakes.md. Mergingmaster: five live inflight notes repaired where they cited the deleted note oronPostAddToMailBox(core-exception-hierarchy-cleanup,core-unmailboxed-container-recovery,pr-strategy-doc-merge-triggers,test-lincheck-lane-open-items,upstream-discussions-unanswered), plus a one-line post-merge marker oncore-241-tx-commit-failure-taxonomy. Post-merge CI: the flake sighting intest-untracked-ci-flakes.md.ProduceLockReleaseTest(unit, both release paths) andTransactionalBatchProduceTest(end to end throughpollAndProduceMany); both verified to fail for the right reasons with the fix stashed