feat(core) astubbs#225: recover the producer when the broker invalidates it, rather than dying - #410
Conversation
… issue's own premise The issue proposes raising a recoverable exception on ProducerFencedException, aborting the transaction, and letting the consumer rejoin. Reading the kafka-clients 3.9.2 and Kafka Streams 3.9.2 sources against PC's code refuted two of its three steps and found a third trigger it did not know about. WHAT THE RESEARCH CHANGED - Abort is not available on the fenced producer. KafkaProducer#abortTransaction lists ProducerFencedException as a fatal @throws, the same as commitTransaction. Streams' answer is to call it anyway and swallow, on the grounds that the broker has already aborted. - Recovery needs a new producer, which PC cannot build. ParallelConsumerOptions holds a finished Producer instance and ProducerWrapper assigns it once from options.getProducer(); PC cannot read a KafkaProducer's config back out. initTransactions() runs once from the ProducerManager constructor and abortTransaction() is reachable only from close. So the plan's first requirement is that PC takes producer config and builds its own producer through an overridable factory - Streams' KafkaClientSupplier shape, with DefaultKafkaClientSupplier's one-line default. - "Rejoin" is not the hard part; the transactional.id is. Streams derives it from applicationId + processId, so two live instances cannot share one and re-initialisation cannot start a fencing war. PC's is whatever the user configured. PC therefore owns the id wherever PC builds the producer. - The quiescence I expected to need already exists. Producing takes the read lock of ProducerManager's producerTransactionLock and committing takes the write lock, with preAcquireOffsetsToCommit acquiring and flushing before commitOffsets runs. On the commit path no worker can be inside send(). Only the produce path needs a worker-to-controller escalation. THE EVIDENCE THE ISSUE LACKED The issue reasons entirely from code and KIP-447. confluentinc#830 is a real field report: transactional mode, UNORDERED, a per-instance UUID transactional.id, and an InvalidPidMapping after two days of producer inactivity that put PC in an infinite retry loop. The reporter asked for exactly this feature - "close the producer that is causing this error and create a new producer instance". What shipped was confluentinc#839, converting the spin into a shutdown, on the maintainer's stated uncertainty about losing in-flight work. That uncertainty is answerable: the offsets stay dirty and the records are redelivered. The plan absorbs that case and reverses the shutdown. Neither upstream issue has a fork mirror. DEFECT-CLASS SWEEP produceMessages is referenced only in parallel-consumer-core. The vertx, reactor and mutiny processors extend ExternalEngine and inherit the core produce path rather than duplicating it, so the recovery lands in one place. The Kafka Streams execution seam is not affected either - PcTaskDispatcher builds PC with a stub consumer and no producer, taking only the work manager. SEQUENCING #352 merges first and unchanged. Its R6 - a fenced transactional producer stays immediately fatal without consulting its handler - is true of the behaviour it was written against. This work makes it untrue and owns the update, including the two tests in ProducerManagerCommitBudgetTest that pin it. Requirements-only. No product code changes here.
… design it was proposing that does not work The note carried #225's proposal verbatim - raise a recoverable exception, abort the transaction, let the consumer rejoin. Two of those three steps do not work, so a session designing from it would build the wrong thing. Replaced with a pointer to the plan plus the three corrections, kept here rather than only in the plan because this note is what arrives in an agent's context at session start: - abortTransaction() documents ProducerFencedException as a fatal @throws exactly as commitTransaction does, so the abort step is not available on the fenced producer; - recovery needs a NEW producer, which PC cannot build while ParallelConsumerOptions holds a finished instance, so the change is an ownership change rather than an exception swap; - "whether rejoin is expressible in PC's lifecycle", which the note named as the thing to investigate first, is answered: the produce/commit lock pair already gives the control thread exclusive access when fencing is detected, so no state-machine addition is needed there. Also retained here because nothing else records them: confluentinc#830 and confluentinc#839 have no fork mirrors, and the sequencing behind #352. Two stale cross-references fixed - the unbounded revoke wait and the commit-failure taxonomy are now cited as the notes that own them rather than as "the branch that carries this note". Impact retagged reliability -> crash, matching the vocabulary's own worked example.
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 | 111 | 107 | 🫤 +4 |
| Duplicated lines | 1557 | 1514 | :face_with_raised_eyebrow: +43 |
| Duplication | 0.83% | 0.82% | 🫤 +0.01% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 2% | ✅ Pass (0.83%) |
| Max increase vs base | +0.1% | ✅ Pass (+0.01%) |
⚠️ 4 new clones introduced
- 12 lines:
parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/ProducerFencingRecoveryIT.java:24<->parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/TransactionalPartialResultSetIT.java:20 - 16 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerRecoveryTest.java:508<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerRecoveryTest.java:466 - 12 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerManagerRecoveryTest.java:179<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerManagerRecoveryTest.java:145 - 7 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerManagerDetectionTest.java:19<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerManagerRecoveryTest.java:31
Powered by astubbs/duplicate-code-cross-check
#411 mirrors confluentinc#830, the only field report behind this work. Created on demand rather than by a sweep: the 2026-08-05 bulk import covered all 78 OPEN upstream issues and a separate cohort covered the 28 closed by the 2023 administrative sweep, and confluentinc#830 is in neither - it was closed in 2024 by a genuinely merged fix. Closed-and-actually-fixed issues were out of scope by design, on the reasoning that they have nothing outstanding. This one does: the fork intends to reverse the fix, which makes the original report load-bearing evidence. docs/inflight/upstream-coverage-completeness.md already registers the general obligation. confluentinc#839 deliberately gets neither a mirror nor a manifest entry. Mirrors are for issues, and the manifest keys on FORK work (fork.branches, fork.prs, fork_issue) - confluentinc#839 is an upstream PR that merged upstream and is already carried here as 1dbc015, so there is no fork-side mapping to record. It is covered inside the #411 body instead.
|
[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 Superseded by a newer quarantine lane report. |
🟢 Throughput — OKThis branch measured about 8% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 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: a37d148, e8bd2cb, 1743297, 4bc6e7a, b62c310, c381310, c79424a, 9c67c89, f1aa5eb, eb9fdb0 Since the previous push: ratio 0.958 -> 0.918, share 1.931 -> 1.979, rate 79778 -> 64471 (-19.2%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/225-uncommitted-completions-ledger #410 +/- ##
=============================================================================
+ Coverage 83.30% 84.12% +0.82%
- Complexity 1639 1762 +123
=============================================================================
Files 102 104 +2
Lines 5559 5884 +325
Branches 557 599 +42
=============================================================================
+ Hits 4631 4950 +319
+ Misses 733 730 -3
- Partials 195 204 +9
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:
|
…quirements that rested on them Six-persona document review of the plan. Two findings were verified against the tree and both overturn something the plan asserted; the rest close gaps the plan left. WHAT WAS WRONG - "Aborting leaves the offsets dirty and the records are redelivered" is false for an instance that SURVIVES. PartitionState.onSuccess removes the offset from the incomplete set and raises the succeeded watermark at success time; onOffsetCommitSuccess only records the committed offset and clears the dirty flag. Redelivery today is a consequence of the instance DYING and rebuilding its offset state from the broker. Recovery in place would commit offsets whose output the abort discarded - silent result loss, which is precisely the doubt confluentinc#839 was settled on. The plan told the reader that doubt was answerable; it is not, without an explicit un-completion step, which is the pairing Kafka Streams makes between resetProducer and closeRunningTasksDirty. Now R13. - The produce-path catch does not fire on the ack wait. FutureRecordMetadata.valueOrError throws new ExecutionException(exception), so futureSend.get() can only surface an ExecutionException, which falls past the typed InvalidPidMappingException catch into the generic handler - PCInternalRuntimeException("Error while waiting for produce results", e), verbatim the stack trace in confluentinc#830. closePCWhenInvalidPidMappingException mocks a synchronous throw from produceMessages, so it passes without covering the reported path. Detection must unwrap first (now R9), and confluentinc#839's shutdown appears not to fire for the case it was written for - recorded as a defect in its own right rather than assumed handled. WHAT WAS MISSING - Producer configuration would print at INFO on every startup: ParallelConsumerOptions carries a bare @tostring and is logged whole; today the field is a Producer whose toString is an identity hash, a config map is SASL and keystore secrets. Now R7. - The derived transactional.id had no ACL story - substituting it stops an operator's existing TransactionalId grant matching, and initTransactions fails fatally. Now R6 (documented stable group-derived prefix) and R19 (re-grant as an upgrade step). - Failure of the recovery attempt itself was unspecified, so the natural implementation hot-loops on the control thread through a coordinator outage. Now R14. - The single response omitted the group rejoin, so the group-generation member of the condition set recurs immediately and never converges. Now in R10. - The transactional.id had to be stable and reused, or the replacement never fences its predecessor and the old transaction blocks read_committed consumers until it times out. Now in R4. - Unbounded recovery had no non-progress signal, so a permanently fencing instance looks healthy. Now R22, inside the settled logs-and-metrics boundary. - The configuration path was scoped to transactional flows while the deprecation covered every flow, stranding non-transactional producers. R1 widened; recovery stays transactional-only. Requirements renumbered R1-R22; every Governs, Covers and Trigger reference updated. Four new acceptance examples. One open question raised rather than settled: whether the deprecated producer-instance option's removal is queued for the major being cut now.
…cation has an end date The plan deprecates the `Producer`-instance option without saying when it goes, which leaves the two paths permanently non-equivalent: only the PC-built one can rebuild a producer the broker has invalidated, so every future change to producer handling gets written twice - once on a path that recovers and once on a path that cannot. Queued in `docs/refactoring.md` under the next-major section, whose gate is open: 0.6.0.0 is unreleased and is the release being cut now. The plan gains R20 (the deprecation javadoc names that release) and a Scope Boundaries line saying the removal itself lands in the refactoring queue rather than in this work. Requirements renumbered R20-R23 in the Observability group.
… outcomes where mechanisms cannot be validated Round 1 corrected the plan's claims. It wrote those corrections at requirements level without checking they were implementable, and two were not. Both were caught by two independent reviewers and verified against the tree. THE REJOIN DEADLOCKS ProducerManager.isTransactionCommittingInProgress() is `return producerTransactionLock.isWriteLocked()`, and onPartitionsRevoked opens `while (isTransactionCommittingInProgress()) Thread.sleep(100)`. Round 1 had recovery rejoin the group BEFORE releasing that write lock, so the revoke callback the rejoin triggers spins forever on a lock the rejoining thread will not release. unsubscribe() runs the callback on the calling thread (self-deadlock); enforceRebalance() defers it to the poll thread (cross-thread). It is also not issuable from there at all: ThreadConfinedConsumer confines consumer calls to the poll thread, and the two rejoin verbs are unguarded delegates that throw ConcurrentModificationException from KafkaConsumer. Streams can do this inline only because one thread owns both the consumer and the processing; PC splits them. R14 now states the OUTCOME - recovery ends with PC a full member on a live generation, and the first commit after it does not carry pre-recovery metadata - and sends the mechanism to planning. UN-COMPLETION ALONE STALLS THE PARTITION incompleteOffsets maps offset -> the ConsumerRecord, and onSuccess removes the entry, so the record goes with the offset; ProcessingShard.onSuccess then retires the WorkContainer. Restoring a bare offset yields one that is incomplete with no work that can complete it, pinning the committable offset at firstIncomplete-1 forever. Nothing seeks, and under the cooperative assignor retained partitions are never re-fetched. R13 now states that no aborted-transaction offset is committed and the work runs again, names why restoring the offset is not sufficient, and defers what state PC retains. ALSO CORRECTED - R8's four types were Kafka Streams' COMMIT-path set. Its send path differs: StreamsProducer.isRecoverable adds UnknownProducerIdException and omits CommitFailedException; RecordCollectorImpl adds OutOfOrderSequenceException. UnknownProducerIdException is what a leader returns after producer state expires from inactivity - the shape of the one field report we have. - AE2 asserted the instance stays alive, which the plan's own reasoning shows is already true on master. It now asserts progress. - Prefixed Kafka ACLs match literally, so granting `orders` also authorises `orders-eu` - enough to fence another application. R6 now requires a non-aliasing prefix. - R7's redaction keyed on Kafka's PASSWORD type missed untyped serializer and Schema Registry secrets. Now deny-by-default. - "Discards the producer" never said close it, leaking sender threads and SASL sessions under unbounded recovery. R10 now closes with a bounded timeout. - R15 now says producing is suspended while no usable producer exists. - R2 gives the factory a contract; a caller returning a cached producer would otherwise defeat recovery permanently. - The #411 reporter supplies a Producer instance, so the fix does not reach them without a migration. Said in the Problem Frame; R20 widened to the whole migration. - R19's WARN moved from failure time to options validation. - R21 and the refactoring.md entry named the release that DEPRECATES the option as the one that removes it, contradicting R16. Both now name the major after it. - The #352 supersession decision cited the produce-path requirement; that PR's R6 is about the commit path. Requirements R1-R24. Dependencies records three constraints the plan was leaning on silently: consumer thread confinement plus the revoke wait, zombie fencing coming from group metadata rather than the id, and per-run id uniqueness not fencing a restart's predecessor.
…that defeated its own requirements Third review round, six personas. The trend is the result worth recording: round 1 falsified two of the plan's claims, round 2 falsified both of round 1's repairs, round 3 falsified nothing. The feasibility pass - the persona that broke the plan twice - returned zero findings and went further than accepting the two deferrals, locating a working mechanism for each: PartitionState already has an insertion route carrying the ConsumerRecord, and ConsumerManager.updateCache refreshes group metadata after every poll, with the classic protocol rejoining automatically on the next poll after a generation loss. WHAT ROUND 2 GOT WRONG - R8's "the two before it are send-path only" is false. TransactionManager maps UNKNOWN_PRODUCER_ID and INVALID_PRODUCER_ID_MAPPING through abortableErrorIfPossible in three handlers, and maybeFailWithError rethrows the stored error wrapped in KafkaException from the NEXT transactional call - sendOffsetsToTransaction and commitTransaction included. Only CommitFailedException is single-path. Also UnknownProducerIdException extends OutOfOrderSequenceException, so the list named a subclass beside its parent; the parent now covers both. - R14's freshness clause made the flagship scenario unpassable. An expired producer id after two days of inactivity involves no rebalance, so the generation never changes and the first commit after recovery necessarily carries metadata predating it - and must succeed. Reworded to freshness against the group's LIVE generation. - F1 undid R13's own deferral by ordering restoration before lock release, which forecloses the seek-based candidate: that needs the consumer, confined to the poll thread, which may be spinning on the very lock F1 holds. Taken literally it rebuilds the round-2 deadlock. - R19 claimed the instance path "keeps its current terminal behaviour" when this plan's own Problem Frame establishes that behaviour is the spin. The defect was named and then owned by no requirement and no boundary; it is now stated in R19, in F3's outcome, and as an explicit Scope Boundaries exclusion. - The Goal Capsule said "Open blockers: None" while the two mechanisms the feature stands on were deferred precisely because every concrete candidate had failed. ALSO CORRECTED R7 contradicted itself - "never rendered" followed by "any diagnostic that does render it" - and "redacts by default" implied a reveal mode that would ship credentials; now one unconditional rule. AE1 and AE4 claimed coverage of the membership and prefix requirements while asserting neither. R14 arrived with no governing decision, repeating R13's round-2 pattern. R15's suspension collided with the existing produce-lock and send timeouts and released nothing on the terminal path. F3 and AE5 still emitted and asserted the deprecation WARN at failure time after R19 moved it to options validation. The Outstanding Questions derivation entry now records the trap two reviewers found independently: no prefix-plus-group.id-plus-delimiter scheme satisfies R6, since "app" yields a literal prefix of "app-x". Scope-guardian confirms the 16 -> 24 growth is corrective accretion rather than scope drift, and that a split is not warranted: the ownership requirements are load-bearing for recovery, so splitting would sever cross-references without removing coupling.
…e the roadmap entry it belongs to TWO THINGS THE #225 REVIEW SURFACED THAT ITS OWN PLAN DELIBERATELY DOES NOT FIX bug-411-wrapped-send-failure-spins-forever.md. ParallelEoSStreamProcessor catches InvalidPidMappingException around the produce-and-ack block and closes PC. That catch was confluentinc#839, written to end the infinite retry loop #411 (confluentinc#830) reported. It cannot fire for that report: FutureRecordMetadata.valueOrError throws new ExecutionException(exception), so the ack wait can only surface an ExecutionException, which falls past the typed catch into the generic handler producing PCInternalRuntimeException("Error while waiting for produce results", e) - verbatim the stack trace in the report, from a build that already carried the fix. From there the record is marked failed and re-dispatched onto the same invalid producer, which is the reported spin. The typed catch fires only on a synchronous throw from produceMessages, which is exactly what closePCWhenInvalidPidMappingException mocks - so the test is green over a path the reporter never took. Not fixed by the #225 work: that plan requires unwrapping before matching, but only where PC builds its own producer, and every user today supplies a Producer instance. The plan excludes it in Scope Boundaries rather than absorbing it, and now cites this note as the owner. Two ways out are on the page; choosing between them is a product call, so the note records both rather than assuming one. Deliberately NOT claimed: that the spin reproduces on master today. The reasoning is from the current tree and the kafka-clients 3.9.2 sources; nobody has run it. The note says what a reproduction needs. ROADMAP survive-producer-fencing advances idea -> requirements-drafted. Its stage_detail said "no design yet", which stopped being true when the requirements plan landed, and product review flagged the entry as underselling the banked design. done_when is rewritten to what the contract actually commits to - a replaced producer, no offset from the aborted transaction committed, its work processed again - and to say the guarantee applies where PC builds the producer, since the deprecated path keeps today's behaviour. Adds the field report and the PR as related links.
…is directory carries docs/inflight/AGENTS.md: a note that maps to a GitHub issue carries a DRAFT response to that issue before its PR merges, because the agents who did the work hold the best context at merge time and by release time it has to be re-mined from commit logs. Two notes on this branch map to issues and had none. issue-response-225.md states the three things scoping the plan overturned in the issue's own proposal - abort is not available on an invalid producer, recovery needs a new producer PC cannot build from a supplied instance, and the "is rejoin expressible" question the issue named as the thing to investigate first is answered without a state-machine addition - plus the field evidence the issue did not have. issue-response-411.md tells the reporter what their report now means: the upstream fix that closed it cannot fire for the failure they described, their own suggested remedy is the direction the fork took, and it will not reach their configuration until they move off the deprecated producer-instance option. It says plainly that the spin has not been reproduced on a current build and what a reproduction needs. Both tagged and shaped like issue-response-120.md: task + coordination, deferred until the sweep or an explicit instruction, and post-merge exempt because a draft deliberately outlives the PR that wrote it. Neither is posted. Per the directory's rule a draft is deleted when it is POSTED, not when its PR merges - the merge is exactly when nobody is looking.
…ubject is written about `docs/solutions/` front matter carries `related_components` and `applies_when` - the classes a write-up is about, and the situations it should be read in. NOTHING READ EITHER. `grep -rln applies_when bin/ .claude/` returned nothing while a majority of write-ups carried one: retrieval metadata written, reviewed, and inert. WHY IT MATTERS, MEASURED RATHER THAN ASSERTED two-threads-one-consumer-why-the-commit-seam-keeps-deadlocking.md records that THREE separate 2026 investigations each re-derived part of it before acting. A fourth followed on #225: a review proposed that fencing recovery rejoin the consumer group from the control thread while holding the produce/commit write lock, which deadlocks against `onPartitionsRevoked`'s spin on `isTransactionCommittingInProgress()` - literally `producerTransactionLock.isWriteLocked()`. A review round caught it. One paragraph of that write-up would have, and its `applies_when` names the situation twice over. WHAT SHIPS A PreToolUse hook on Write|Edit|MultiEdit naming the write-ups whose `related_components` appear in the text being written. Once per write-up per session; never blocks. THE DESIGN DECISION, AND THE MEASUREMENT THAT SETTLED IT The obvious hook fires when you edit a file a write-up names. Measured against the incident it was built for, that design fires ZERO times - the whole episode was spent writing a requirements document and no Java file was touched at any point. Matching the TEXT fires five, including both write-ups that would have prevented the defect. So the trigger is the text. It reads `related_components` only. `applies_when` is free prose, and a fuzzy match on it rebuilds the noise that makes a flat session-start list ineffective - which is the thing this hook exists to be the complement of, not a second copy of. PROVEN ABLE TO FAIL `bin/test-check-solutions-hook.mjs` - eight cases, every positive paired with a negative control that must stay silent, plus a regression case carrying the incident's own shape. Two mutants were run against it and both were killed: removing the word-boundary match reddens the substring control, and removing self-suppression reddens the author control. The suite is idempotent - the hook's per-session memory made the first draft pass once and fail forever after, which is a red that means nothing. WRITTEN IN NODE, AND THE FIRST DRAFT SHOWS WHY The first draft was bash driving python through two heredocs; a second stdin redirection silently clobbered the first, so the hook emitted nothing - indistinguishable from "no write-up matched". That is the exact class `bin/AGENTS.md` cites for the Node-first ruling. `check-source-patterns.mjs` also caught a live `printf | grep -q` under `pipefail` in the shell self-test, which inverts its own answer above one pipe buffer and survives only because fixtures are small. Coverage is bounded by how many write-ups name a real Java type - a minority, since the field also carries concepts like `documentation` that match no filename. Stated in docs/agent-harness.md rather than left to rot unmeasured.
astubbs
changed the title
|
@claude review this Two halves, and they want different scrutiny. The plan has already been through three review rounds with six personas each. Round 1 falsified two of its claims, round 2 falsified both of round 1's repairs, round 3 falsified no mechanism. So the interesting question is not "are there findings" but whether anything in the current text is still wrong after that much editing — particularly:
The tooling is new and unreviewed. One thing I would rather have challenged than agreed with: the hook matches text against |
|
Claude finished @astubbs's task in 5m 27s —— View job Review complete
The planR13/R14 — is deferring the mechanism honest, or does it hide the problem? Honest. The Goal Capsule's "Open blockers" states outright that every concrete mechanism tried so far failed against the lock pair and thread confinement, and that "a failed candidate reopens the recovery shape rather than merely the requirement" — that's a real commitment, not a hedge. The same caveat is repeated verbatim-in-spirit at R14 itself, in Dependencies/Assumptions ( Renumbering integrity. Walked every R8's exception list. Internally consistent (5 named + One drift I did find, outside what was asked about: The toolingRead On the logic itself: word-boundary regex, self-referential exclusion (both the write-up-editing-itself case and the fixture-name check), per-session dedupe keyed off a hashed session id, and the hidden-count-honesty behavior all read correctly and match their test cases. What a 7th/8th case would catch, concretely:
On the SummaryNo blocking findings on either half. The plan's requirement structure holds up under a full cross-reference walk; the one real inconsistency found is cosmetic (roadmap title vs. its own updated body). The tooling's core logic and self-suppression are sound by reading; its test suite would be more durable against unrelated corpus edits with a couple of fixture-based cases, and |
…en its subject is written about" This reverts commit 0b4b11a, which cherry-picked the solutions hook onto this branch. The hook keeps its own PR (#413) after all. It was folded in here on a misreading and is being separated again; nothing about the hook changed, and this branch returns to the requirements plan it is named for.
astubbs
changed the title
|
Correction to my review request above: the tooling half is not in this PR. The solutions hook was briefly cherry-picked here and has been reverted; it lives in #413, which is open again. Please disregard the "tooling" section of that comment — What stands from the request: this PR is the requirements plan, three review rounds deep, and the questions worth pressing are whether deferring R13/R14's mechanisms is honest, whether the cross-references survived two mid-list renumbers, and whether R8's six-type split across the two paths is right — that list has been wrong once already. |
…ody said rejoin is open Caught by the automated review on the PR. This branch rewrote the survive-producer-fencing entry's stage_detail to say group-membership refresh is one of two mechanisms deliberately left open, and its done_when to what the contract actually commits to - but left the title reading "Abort and rejoin when the producer is fenced", which bakes in as settled the very thing the body beneath it says is not. Same class as the plan-doc drift the three review rounds were watching for: the title did not get the memo the body did. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
|
Thanks — all four points land, and the one real drift is fixed. Roadmap title — fixed in 8fc2645. It now reads "Recover the producer when the broker invalidates it, rather than dying", matching the body beneath it. Exactly the title-didn't-get-the-memo class, and it was mine. R8's exception list — you couldn't re-verify without network; I did, against the kafka-clients 3.9.2 sources jar, before it went in. The tooling findings — all three are right, and they now belong on #413, where the hook lives after the un-fold. Applied there: the literal R13/R14 fourfold placement — agreed it stays. Each copy serves a different reader and they say the same thing; I'll hold that line if a later edit makes them drift. |
…ake the self-test visible to the harness Three changes, all from the automated review that read this hook while it was briefly folded into #410. THE CI RED. bin/test-check-agent-hooks.sh proves every registered hook is self-tested by finding the literal `.claude/hooks/<name>` in test CODE - deliberately code, so a hook cannot buy coverage with a sentence in a comment. This suite built the path with path.join(root, '.claude', 'hooks', name), three segments the regex never sees, so the harness reported the hook untested however thoroughly it was. One literal now. The doc's stated counts moved with it: fifteen scripts, seventeen registrations - the check verifies both against settings.json rather than trusting the next editor. APPLIES_WHEN IS DISPLAYED, STILL NOT MATCHED. The review's challenge: matching on related_components only is right, but render() never showed applies_when even for a write-up that had ALREADY matched - where it costs no false positives and is the field carrying the retrieval intent. Parsed alongside related_components now, shown capped at three lines per matched write-up. The header prose that said the hook "does not read applies_when" now says precisely which half. SIX CASES THE REVIEW ASKED FOR. Every prior case drove the real docs/solutions/ tree, so a retitled write-up or a moved class would redden them for reasons unrelated to hook logic. Five fixture-based cases now pin the LOGIC through the types/docs parameters match() exposes for that purpose - exact match, the three boundary shapes, a concept-only entry staying inert, applies_when render, hidden count. And one for the MultiEdit shape: the hook parsed edits[].new_string and nothing exercised it, so a wrong property name there would have shipped silently. Fourteen cases; idempotent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…producer-fencing-brainstorm This PR now stacks on the ledger rung (#474), itself on the groundwork rung (#472), both cut out of this branch's tree today. Resolved by hand: the processor and the module resolve to this branch's versions, which are the rungs' text plus recovery; the README template takes the ledger rung's new retention section and points the recovery section's retention sentence at it; a restore entry point the merge placed twice in WorkManager is kept once. The ledger rung also carries two master commits this branch had not merged yet (the rebalance-callback rule and the Lincheck shard harness), which arrive with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…ut today Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…m this branch already uses The merge of #410 brought the groundwork rung's three module tests in their rung-1 shape (a caller-supplied transactional.id, the rung-1 helpers). On this branch the id is derived and the module is built through the factory, so the tests are re-expressed with this file's helpers: the derived id is asserted by prefix, and every build is shown to receive the same one. The instance-path case was already covered here and is not duplicated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…left behind Moving the reactor and vertx examples onto the configuration path added an import block on top of one that already imported the same type, so each file ended up importing java.util.Map twice, and VertxApp imported JStreamVertxParallelStreamProcessor twice as well. Nothing fails on it - javac accepts a repeated single-type import, -Xlint:all does not warn, and the examples compile and their tests pass either way - so it would have merged silently and stayed. Removed the added copy in each case, leaving HashMap and Map in the order the rest of the file uses. No behaviour changes; test-compile of both example modules and their dependencies is green, and bin/check-all.sh passes. Swept the whole tree for the same defect class rather than the two files the duplicate-code bot named. Four files carry a repeated import; two are these, introduced by this branch and fixed here. The other two are inherited and deliberately left alone: WorkManager's ThreadConfined, added by the base rung #410 in f0af594 and f6bc1f8, and CommitHistorySubject's two truth-generator imports, which predate master's fork point. Neither is on a line this PR wrote. Nothing enforces this - no gate, no compiler warning - which is why the sweep found instances on two branches at once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…y it explained 8027778 deleted `1 THREAD_SAFETY_VIOLATION AbstractParallelEoSStreamProcessor.controlLoop` from the baseline, because the infer lane on the merged head listed it among the known races that no longer fire - master's 9999144 declared the control-thread confinement RacerD now reads. The five-line comment block that 73bdd13 had written to justify that entry was not deleted with it. What it leaves behind is worse than clutter. The block now sits immediately above `1 NULLPTR_DEREFERENCE AbstractParallelEoSStreamProcessor.processWorkCompleteMailBox`, so it reads as that line's justification while describing a THREAD_SAFETY_VIOLATION on a different method - and its claim, that the controlLoop race "came back when the recovery pass moved into ProducerRecoveryPass", is the claim 8027778's own measurement overturned. Deleting it is lossless: the reasoning for adding the entry is in 73bdd13's body and the reasoning for retiring it is in 8027778's, which is where the file's header says a retirement's evidence belongs. With the block gone, this branch's `config/infer-known-findings.txt` is byte-identical to its base, which is the honest state - the branch adds no baseline identity and retires none. No gate catches this: the ratchet parses entries, and a comment for an entry that is not there cannot fail it. Found by reading the PR's own diff against its base while babysitting #410. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…nder it grew to five ab86e34 rewrote the list in this section for today's re-cut - #472 and #474 cut out from under this PR - and left the heading above it saying "three PRs (2026-09-03)". A reader who trusts the heading over the list it introduces gets the stack's shape wrong, and this note exists precisely to tell a later PR which rung it has to reconcile against. Verified against GitHub rather than the list: #426 MERGED, then #472 -> #474 -> #410 -> #420 all open, each based on the one before it. Five rungs, one landed, which is what the heading now says. The list's own contents were already correct and are untouched. Nothing enforces a heading against the block it introduces; grepped the tree and every ref first, as AGENTS.md requires before moving a labelled block - the string is cited nowhere, it only appears as this heading on the branches that carry their own copy of the note. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[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. |
[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. |
The ledger rung and the confinement commit on this branch each added the import; the merge kept both. javac accepts a repeated single-type import and no lint warns, so nothing went red. The sweep of every module's source for repeated import lines finds one other, in the generated Truth subject, which predates the fork and is not this branch's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
|
@claude review this |
|
Claude finished @astubbs's task in 5m 15s —— View job Review complete
The four newest commits are all legitimate small fixes
The re-scope since the last full review is real and consistentThe PR body's 2026-09-07 note says two rungs ( The recovery mechanism itself holds upRead
No new correctness issues found in this pass. Given how many rounds of adversarial review (ce-simplify, the 11-lens ce-code-review, an independent validation pass, two Codex passes) this has already been through and how narrowly this pass's changes are scoped (four commits, all non-behavioral), that's the expected outcome rather than a surprising one. One thing worth a plain look, not a blocker
CI / quarantine status at HEAD (
|
…thdrawn, rejoin is expressible Two of the burn-down's open items closed on 2026-09-08, each by its own agent, and the note records both as things that happened rather than things to do. The eager-mode PERIODIC_CONSUMER_SYNC stall (the confluentinc#857 family's fourth open item) is withdrawn by #478: four replays of the recorded seed on today's master drained completely with zero loss, and the "stall" was the Class 2 timing bound, whose crossing flips with the processor count at a fixed seed and tree. The grid it was opened on was also never a one-term A/B. Nothing to ship or to name; what survives is the per-shard liveness gap, already tracked and deferred. Whether "rejoin" after producer fencing is expressible in the lifecycle is known by a read of the #472/#474/#410 diffs against the engine's ownership rules: it is, and the question dissolves, because the instance never leaves the group - rejoin reduces to abort, replace and replay on the control thread under the write lock. The stack respects the @GuardedBy ledger, thread confinement and the lock pair. What it leaves is review-sized: #420's territory, one wire-level test nobody wrote, and the plan's one open question about declining the lock during a rebalance. None of it changes the tier 2 decision. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
wins the transactional revoke path Master landed #466 (the revoke-path commit hands itself to the control thread), #468 (WorkContainer equality is identity) and #471, and this PR went CONFLICTING. #466 is a different answer to the question this branch answered: in transactional mode onPartitionsRevoked no longer commits on the poll thread at all - it posts a request, wakes the control loop through the mailbox, and waits, bounded by commitLockAcquisitionTimeout; the confluentinc#548 spin is gone; and it refutes this branch's decline by experiment (a revoke that commits nothing leaves its output in the open transaction for the next commit to publish without the offset - the same duplicate through a different door). Master's own re-premising of docs/inflight/bug-857-transactional-revoke-wait.md says what is left for this PR: not the absence of a bound, but whether the bound is the right value - five minutes, against a max.poll.interval.ms it can exceed. So the resolution is master's on that path, per file: - AbstractParallelEoSStreamProcessor: master's onPartitionsRevoked, commitOnRevokeViaTheControlThread and the no-argument consumer-commit tryCommitOffsetsOnRevoke replace this branch's parameterised decline, its performCommit extraction and its post-catch wake (moot: the served commit runs on the control thread, which recovers itself); commitOffsetsThatAreReady is master's again; the mailbox loop keeps master's wake-up message skip in front of #410's first-failure try; one of two identical assertOnControlThread helpers (#410's and master's) is kept - master's, which names the new design. - RebalanceEoSDeadlockTest: master's whole file. Its unamended assertion that committed offsets advance inside the callback holds again by construction under #466, and it now reads the output topic at read_committed for duplicates; this branch's decline amendment is superseded. - ArchitectureTest: master's whole file (#465). This branch's interface-hop widening does not merge onto it; its note now records that the blind spot is still open on master and the widening is to be re-applied on top of #465 as its own change. - ProducerManagerTest: master's revoke-request tests and this branch's five revocation tests, both kept; PartitionState: master's onSuccess(long) javadoc with #410's ledger cross-reference folded in; TransactionalClaim: master's scope note plus #410's C15. - config/infer-known-findings.txt, bug-857-family.md, core-recoverable-producer-fencing.md: master; bug-wedged-after-poisoned-transaction.md: master's deletion (the grooming sweep); the three vetted notes keep master's markers and this branch's concurrency label; test-untracked-ci-flakes keeps master's rows and this branch's three later sightings. - ProducerRecoveryTest's revoke-path fence test is retargeted to the served-commit shape: the fence now fires on the control thread inside the served pass, the callback returns promptly on the failed pass, nothing is stranded, and the replacement is built. Its wake assertion is gone with the wake. What is red, on purpose: Revoke857TransactionalWaitProbeIT, 5/5, with the callback at 19.2s of a 20s in-flight dwell against its 10s poll-interval budget - and never the 79s starvation the spin produced. That is the measurement of #466's bound, and it is this PR's remaining acceptance test, not a broken instrument. What is now dead main code, held for the owner's call: the three ProducerManager revocation lock helpers and the DeclineCountingProducerManager instrument, which count a decline the transactional path no longer makes. Verified: ProducerRecoveryTest, ProducerManagerTest, ProducerManagerDetectionTest, ArchitectureTest and the convention rules, PartitionStateAbortedTransactionReplayTest, TransactionalClaimCoverageTest; RebalanceEoSDeadlockTest 5/5, ProducerFencingRecoveryIT 2/2. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
…3-bound-transactional-revoke-wait The base this PR is stacked on moved again - its own master merge and a re-cut into five rungs - and GitHub reads the PR against that base. Four conflicts: the infer ratchet takes #410's one new entry (the lane is the judge); the revoke-drain note takes master's retirement into docs/solutions; the flake ledger's rows were identical on both sides; the transactional claim register keeps master's newer wording of the same scope note ahead of #410's C15. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
and #473 Two conflicts. PartitionState: #470 counts an async commit as committed only when the broker answers the latest offer, and #410's ledger trims on commit success; joined so the ledger snapshots before the offer is made and is trimmed only on the answer to that offer - a stale answer to an older offer leaves the newer snapshot in place, so a replay can only over-replay, never under. The flake ledger takes master's updated rows and #473's retirement of the section it fixed, and keeps this branch's three later sightings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
, #481, #482, #485 Two conflicts, and one silent auto-merge artefact that only the compiler found. PartitionState. #469 replaced the pair of booleans with a version counter, deleting stateChangedSinceCommitStart; #410's ledger snapshots at the same point. Joined by taking master's counter whole - the single completionCount load, tested and stashed as completionCountBeingCommitted - and moving the ledger's snapshotForCommit() inside that same branch, still ahead of createOffsetAndMetadata(), so the ledger is still snapshotted before the offer is made and trimmed only on the answer to that offer. The artefact: restoreCompletedButUncommittedWork() still called setDirty(), which #469 deleted, and git merged both sides clean because neither touched the other's lines. It is recordCompletion() now - the replay has moved the partition's state on, so it stamps a new version and isDirty() derives the rest. PartitionStateAbortedTransactionReplayTest 7/7 and PartitionStateCommitWindowSeamTest 3/3 cover the join. The flake ledger takes both sides' rows and master's retirement of the simpleBatchTest section, which #482 fixed - expectation-versus-input, not the library. config/infer-known-findings.txt takes master's version whole. The "1 NULLPTR_DEREFERENCE AbstractParallelEoSStreamProcessor.processWorkCompleteMailBox" line this branch carried was a merge artefact: #346 added it, #466 retired it in the same change that reshaped the mailbox loop, and an earlier merge here re-took #410's older side. The static lane was red on it. Re-ran infer locally on the merged tree - 9 findings, all known, none new. Verified: ProducerManagerTest 22/22, ProducerRecoveryTest 15/15, the PartitionState family, the RetryQueue family, WorkManagerTest, ArchitectureTest, TestConventionsArchTest, TransactionalClaimCoverageTest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…producer-fencing-brainstorm Brings the ledger rung's merge of #472, and with it the master that rung merged: the review fix on #472, the commit-window redesign (#469, #470), the revoke commit handed to the control thread (#466), and the grooming sweep (#476). Resolved by hand: - maybeAcquireCommitLock: master's revoke-commit override and this branch's replacement guard compose. A periodic commit waits out a replacement; a revoke commit is answered regardless, since its request was taken for the pass, and while no producer exists it completes with nothing committed - consistent, because the aborted transaction published nothing and no offset was committed, so the next owner reprocesses. controlLoopPass says so at WARN when it happens. - The branch's own assertOnControlThread helper is dropped for master's, which has the same contract. - PCModule.replacementProducerWrap keeps #472's javadoc on why only the replacement build runs as user code, with this rung's sentence that ProducerRecovery builds through it. - TransactionalClaim: master's settled scope note on the revoke-path claim and this branch's C15 recovery claim both stand. - The flake ledger takes master's table, which retired the entries whose tests master fixed or quarantined since, and keeps this branch's one sighting master never had (TransactionTimeoutsTest's long arm). - Two notes the grooming sweep retired are accepted as deleted; the plan's citation of one now points at the history that holds it. - The stack note keeps this branch's heading and impact and takes master's vetting record, attested post-merge-correct. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…add today's The merge of the ledger rung took master's flake table, and master never carried the LoadFactorCeilingReportingTest row: every sighting of that capture leak is local to this stack, so taking master's table dropped the row and its section rather than superseding them. Both are restored as they were, with a sixth sighting from today's full core run on the merged tree - the same captured-line shape, passing alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. No quarantined test changed outcome since the previous push. Updated for |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var fenced = new ProducerFencedException("fenced by another producer with the same transactionalId"); | ||
| doThrow(fenced).when(wrapper).sendOffsetsToTransaction(anyMap(), any(ConsumerGroupMetadata.class)); | ||
| var manager = managerThatCanRecover(); | ||
| assertThat(manager.canRecover()).isTrue(); |
There was a problem hiding this comment.
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ProducerManagerDetectionTest.java:81
…producer-fencing-brainstorm Brings master as of #496 up through the stack, for the CVE exclusion (#493) and the scan split (#489) that turned the required check red on every rung. Two docs conflicts, resolved by hand: - The flake ledger keeps every row: master's updated ManagedPCInstanceLifecycleTest count and this branch's three rows master never carried. - docs/refactoring.md: master added a bullet asking this branch to swap its replay loop to register-then-publish when it lands. The swap is already on this branch (c0a1ef3, pinned by the completion-on-publish case in PartitionStateAbortedTransactionReplayTest), so the bullet is retired here rather than carried to master as a stale instruction. The two bullets beside it stand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…producer-fencing-brainstorm # Conflicts: # docs/inflight/test-untracked-ci-flakes.md
…ke, on the recovery rung The Integration Tests lane failed once at 6ff294e on the fourth repetition of ManagedPCInstanceLifecycleTest's rapid-toggle case. The head differs from the passing one before it by a docs commit, the rung above passed with the same code, and the arm runs the default consumer-commit mode, which none of this stack's transactional-mode code touches. Both sightings today came in the hour every lane of the stack was pushed at once to one runner - the load the test's fixed sleep is already recorded as not tolerating. Recorded as a sighting; nothing is retried. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
… the recovery rung, seed recorded Chaos Pain Suite 4/4 failed once at a8768a1 on the churn storm's outer Awaitility wait, with zero gating violations and lag-stagnation observations on every partition - the signature the note's 2026-09-08 entry names and the per-shard-liveness note owns. The head differs from the passing one before it by a docs commit, and the rung above passed the shard on the same code; the runner was saturated at the time. The seed is recorded before the log expires; nothing is replayed or retried. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…ard's next run, seed recorded Chaos Pain Suite 4/4 failed again on the next head of the recovery rung, which differs from the previous one by the sighting it records and nothing else. This time the churn storm tripped the ZOMBIE_MEMBER/REBALANCE_BLOCKED arm, 747ms over its 15s bound with no member stalled holding work - the 2026-09-07 sighting's shape. The rung above passed the shard four times on the same code in the same window, on a saturated runner. Seed recorded before the log expires; a quiet-runner replay is owed and named. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
1 participant
Closes #225.
depends on #472
depends on #474
Description
A transactional Parallel Consumer died the moment the broker invalidated its producer - fenced, epoch bumped, pid mapping lost, or the group generation moved under an offset commit. Kafka Streams treats every one of those as a migration signal, aborts, rebuilds the producer and carries on. This PR makes PC do the same.
The premise of the issue was wrong, and so was the upstream fix
#225 asked PC to rejoin the consumer group after fencing. Traced against kafka-clients 3.9.2, rejoin is not the repair: the poll thread's coordinator already rejoins on generation loss, and a stale-generation commit after recovery returns a
CommitFailedException, which is itself in the recoverable set, so recovery simply repeats until a poll has refreshed the generation cache. Rejoining explicitly from the control thread - what the first review proposed - deadlocks againstonPartitionsRevoked's spin on the produce/commit write lock. Upstream's confluentinc#839 caught the wrong exception shape: the send future throwsExecutionException(cause), so its catch never fires. That spin on the producer-instance path is pinned here as #411, not fixed.What changes for a user
ParallelConsumerOptions.producerConfig(Map)- feat(core) astubbs#225: build the producer from producerConfig, so PC owns the producer it uses #426, the base of this PR - and rebuilds it from the same map,transactional.idincluded, when the broker invalidates it. (The derived per-instance id, the factory and redaction are feat(core) astubbs#225: derive the transactional.id, build the producer through a factory PC enforces, and redact its configuration #420, above.)pc.producer.recoveries{condition}andpc.producer.consecutive.recoveries.How it is proven
Every unit landed test-first, one commit each, with a mutation check per unit recorded in its commit body.
ProducerFencingRecoveryITfences a PC-built producer three times on a real coordinator and observes three recoveries, with a rogue-fenced-in-turn control and the recovery counter as the non-vacuity anchor; with recovery disabled it fails at the first fence. The transactional-guarantee register from #262 gains claim C15,PRODUCER_INVALIDATION_RECOVERED, and its coverage test accepts it. The aborted-transaction replay is proven at unit level (PartitionStateAbortedTransactionReplayTest); the broker IT's fences land between phases, so the replay path is not what it exercises - stated so it is not read as covered twice.bin/build.shis green across all eleven modules; the core unit suite is 662 tests with 8 pre-existing skips.Inherited from #262, which merged mid-flight
Its commit body records four defects found and deliberately not fixed there. Two are fixed here: a throwing
abortTransaction()no longer skips closing the producer, and the instance-pathInvalidPidMappingExceptionclose no longer marks the batch succeeded with offsets committed for records never produced. Its open question - "PC has no recovery path from a poisoned transaction short ofclose(), design decision left open" - is the decision this PR takes; the plan links both directions. The commit-interval identity-compare defect is out of scope and noted in the plan.Sequencing with the PRs open beside it
docs/inflight/core-recoverable-producer-fencing.mdcarries the two merge-time tasks, keyed to the plan's decisions: #352's two fencing tests and its R6 line assume fencing stays fatal, and #408's revoke-path rethrow becomes record-and-decline once recovery exists. Whichever lands second reconciles, in that PR's terms. The bounded revoke wait that recovery makes viable is named in the plan and deliberately not taken here.The review rounds, and what they changed
A
ce-simplifypass folded three hand-rolled cause-chain walks and detect-and-record steps into one each, and its efficiency reviewer found a defect rather than a simplification: detection ran in every commit mode while recovery ran only in the transactional one, so a PC-built producer used under a consumer-commit mode that met a recoverable condition was marked replacing, never replaced, and parked every worker for good.canRecover()now also requires the transactional commit mode.An eleven-lens
ce-code-review(correctness, standards, testing, maintainability, learnings, security, performance, API contract, reliability, adversarial, prior comments), with an independent validation pass over its top eight findings, found and this PR fixed:initTransactionsfailed was never closed (one producer leaked per attempt); the harness's wake-up interrupt during the write-lock wait closed the instance; anErrorfrom a user factory, or a factory that passed start-up but violated its contract on every rebuild, parked workers forever; recovery ran ahead of a close. All four are fixed and tested.PartitionState(which had the static analyser reporting thirty unrelated accessors) - the ledger is now its own class.Left for the owner, recorded where each belongs: the availability state machine's extraction (
docs/refactoring.md), and a pre-existing sibling of the interrupt defect on the commit path's own lock wait (docs/inflight/bug-commit-lock-wait-closes-on-wake-up-interrupt.md).The question the ledger rung's review carried here
The review of #474 asked, of
UncommittedCompletions.forget(), whether "the commit it belonged to was the aborted one" still holds once the replay is wired in - specifically whether a commit can be mid-flight, with a snapshot taken for a different, still-live attempt, when the replay runs. It holds, by thread confinement rather than by lock ordering: the ledger retains only in the transactional commit mode, where every commit runs on the control thread and returns only once the broker has answered, and the replay is@ThreadConfinedto that same thread and asserted so. A commit and a replay therefore never overlap; the transaction the snapshot belonged to is the one the recovery aborted, because it was the only one open. The one commit that could arrive from elsewhere - the revocation commit - is handed to the control thread as well (#466), so it serialises with the replay in the same way. The commit-window redesign that landed meanwhile (#470) changes the trim, not the premise: the ledger trims only on the acknowledgement of the last offer the partition made, the same rule the clean mark follows, so an older answer cannot trim a newer snapshot even in the modes where answers can arrive out of order.The analysis surfaces, read and judged
bin/check-pr-analysis-surfaces.sh 410reports twelve SpotBugs findings on lines this PR wrote. None is a defect, and the verdict is recorded here rather than left in a channel nobody reads:ProducerManager.commitOffsets—EXS_EXCEPTION_SOFTENING_NO_CHECKED. The softening is this PR's subject:invalidatedOrRethrowis what turns a broker verdict into aProducerInvalidatedExceptionthe control thread can recover from.ProducerRecovery.recordInvalidation—LO_TOSTRING_PARAMETER. Thecondition.toString()is load-bearing, not redundant, and the detector does not model why. Checked against the slf4j-api 2.0.18 this build resolves:MessageFormatter.arrayFormat("cond: {}", new Object[]{ throwable })returns the messagecond: {}with the throwable consumed as the stack-trace argument, so dropping thetoString()would render a literal{}and lose the condition the debug line exists to name.ProducerManagerRecoveryTest.sourceFailure—UWF_UNWRITTEN_FIELD. A deliberate artefact of the stack, not dead code to delete: the test that writes it is theProducerFactorycontract-violation case, which lives one rung above in feat(core) astubbs#225: derive the transactional.id, build the producer through a factory PC enforces, and redact its configuration #420. Removing the field here would break that rung.OCP_OVERLY_CONCRETE_PARAMETER-family widenings, onePSC_PRESIZE_COLLECTIONS, onePRMC_POSSIBLY_REDUNDANT_METHOD_CALLS(Mockito stubbing of the same mock twice), oneIICU_INCORRECT_INTERNAL_CLASS_USEon a test whose subject is the internal API, and the pair onProducerRecoveryTest'sLoggerFactory.getLogger(ProducerRecovery.class), which is the log capture the "logged louder" assertion needs.Where the requirements live
docs/plans/2026-09-02-001-feat-recoverable-producer-fencing-plan.md: twenty-four requirements, ten acceptance evidence items each mapped to a named test, eleven key technical decisions (nine session-settled), three document-review rounds before implementation and five reviewer passes on the implementation-ready plan.Checklist
docs/refactoring.mdremoval queue,docs/data/roadmap.yamlentry, the plan, the in-flight notedocs/features/-N/A - the feature is documented in the README sections above; no docs/features/ entry exists for transactional mode to extendProducerFencingRecoveryIT, register claim C15, five rewrittenclosePCWhenInvalidPidMappingExceptionvariantsdocs/inflight/working note (pr-/branch-) started at the PR's first commit -docs/inflight/core-recoverable-producer-fencing.md, opened at the brainstorm and rewritten as the work movedce-simplifyandce-code-reviewlocally - both run on Fable; what they found and changed is in the section above🤖 Generated with Claude Code
https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg