Repository navigation
fix(core) confluentinc#857: the revoke path declines the commit lock instead of blocking on it - #29
Merged
astubbs merged 261 commits intoSep 2, 2026
Conversation
This was referenced Apr 13, 2026
|
@astubbs, is there any way to get a SNAPSHOT version so we could test? Or perhaps you will release a new version? |
Owner
Author
Hi, yup! I’m going to try get the build pipeline on my fork to start releasing tomorrow, everything’s in place I just need to wrap it all up. I will start releasing snapshots, and release a full new release too. |
astubbs
force-pushed
the
bugs/857-paused-consumption-multi-consumers-bug
branch
from
April 20, 2026 22:46
c054ca9 to
a6ce9bf
Compare
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)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-confluent #29 +/- ##
===================================================
Coverage ? 77.86%
Complexity ? 1201
===================================================
Files ? 83
Lines ? 4333
Branches ? 392
===================================================
Hits ? 3374
Misses ? 769
Partials ? 190
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
❌ Mutation Testing (PIT) ReportPIT did not produce a report. Most commonly this means a test failed in the baseline (PIT runs all tests unmodified first to establish green) and PIT aborted before mutating. See the "Run PIT mutation testing" step logs for the failing test, then either fix it or add it to |
astubbs
force-pushed
the
bugs/857-paused-consumption-multi-consumers-bug
branch
from
April 23, 2026 01:11
a6ce9bf to
262629a
Compare
|
|
@astubbs I see that it's still work in progress? Sorry for troubling you so many times but this issue started to re-appear more frequently. |
|
Hello @astubbs , I appreciate that you're investing your personal time to move this project forward, and I'd like to ask what your plan is. Is there a timeline for getting this fix merged into master and releasing a new version? |
Owner
Author
Sorry everyone for the false start / delay. A series of family things I had to take care off. I’m back in the city next week again and will resume work on this. 🙏 |
Owner
Author
|
@dmironowicz fyi I have now resumed coding work including PC. Just wanted to keep you informed. Btw have you managed to try the published snapshot? |
That's great news, thank you for letting me know! I use parallel-consumer at work, however our environment is locked down to only allow dependencies published to maven central - snapshots are not permitted. That said, the issue described here is not exactly what I'm experiencing, because in my case processing pauses for a small number of partitions assigned to the consumer and resumes after few hours. The similarity is high enough though that I was hoping this fix could resolve it as well. |
Owner
Author
|
Roger that! Yes quite possible that a lot of this work will help. Does it resume, possibly, around the time of a consumer rebalance? Or definitely not? |
|
Looking at the logs I don't see any rebalancing activity around that time so I don't think a rebalance is triggering the recovery. |
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…lures may be real bugs Records the rule for all agents: a test failing under parallel/contended load may be exposing a real main-code concurrency bug, not flakiness. Don't loosen timeouts/weaken assertions/serialize to go green until you've established contention-artifact vs library bug (e.g. run the test on an uncontended/own broker). Prompted by finding that the RebalanceEoSDeadlockTest contended failure maps to the real confluentinc#857 deadlock (PR #29). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…ep 1) [skip ci] Root cause of the parallel-integration flakiness is ONE shared TestContainers broker contended by thread-parallel tests. Fix: run failsafe forked (forkCount=4, own broker per fork), sequential within each fork -> uncontended. Reliable (5/5 green Mac; green GitHub-hosted) AND faster than sequential everywhere (Mac ~4:06, GitHub 6:33 vs 11:38). Does not mask anything - each test runs on an uncontended broker. - self-hosted-tests.yml: integration now uses forked mode (per-suite matrix args); performance keeps thread-parallel. - findings doc: forked-mode resolution + GitHub-vs-Mac numbers + the key discovery that the contended RebalanceEoSDeadlockTest failure maps to the REAL confluentinc#857 deadlock (PR #29) - so contention was exposing a real bug, not just flaky tests. Two-step road recorded: (1) forks now; (2) merge confluentinc#857 lock fixes and retry full thread-parallel to validate. [skip ci]: docs + runner-only workflow; no functional code change to test here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…onfluentinc#857 lands [skip ci] Per decision: ship Step 1 (forked parallelism) now; do not merge the ~454-line WIP confluentinc#857/#29 concurrency refactor just to retry full thread-parallel. Reproducer (-Dparallel-tests=true) recorded for after confluentinc#857 is finished on its own merits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv
astubbs
marked this pull request as draft
July 28, 2026 11:20
Owner
Author
|
FYI the snapshot build from main is being published to maven snapshot central, but not of this branch. You can however build this branch yourself and test it with a snapshot build. |
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…s (+ self-hosted runner & flakiness findings) (#68) * ci: run integration + performance suites on a self-hosted runner Adds an opt-in self-hosted GitHub Actions runner path for the heavy integration and performance suites, re-enabling JUnit's concurrent test execution that the `ci` profile disables (likely resource contention on GitHub's 2-core hosted runners; the tests run parallel by default off-CI, so they are parallel-safe). - self-hosted-tests.yml: manual/scheduled, OS-agnostic `performance` label so a Proxmox Linux VM or a Mac can serve it. Runs both suites with -Dparallel-tests=true. Never triggers on PRs (self-hosted + untrusted PR code on a home machine = RCE). - ci-integration-test.sh forwards extra Maven args so the parallel flag can be passed through. - docs/SELF_HOSTED_RUNNER.md: native config.sh setup for a Proxmox Linux VM and a Mac second runner, the trust/sandbox model, speedup measurement, and an appendix on remote power-on and dual-boot OS switching (Wake-on-LAN + efibootmgr BootNext + Home Assistant). - actionlint.yaml declares the custom `performance` runner label. - Removes the superseded Windows-only performance.yml. The GitHub-hosted PR gate in maven.yml is unchanged and remains the sole required merge check; the self-hosted workflow is purely additive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs: capture parallel-integration flakiness finding Records the measured finding that re-enabling JUnit parallel test execution is fast (~7-10x on real cores) but leaves integration tests flaky: ~2/104 fail per run, a different set each run, insensitive to parallelism factor (20->6) and Docker RAM (8->16GB). Root cause is test-level concurrency races (tight per-op timeouts, shared broker/topic state), not resource contention. On GitHub's 2-core runners the same parallelism produces ~28 timeout failures (CPU starvation), which is why the ci profile keeps parallel-tests=false. - New docs/solutions/test-flakiness/ finding doc (full frontmatter, the four measured runs, what-didn't-work, root cause, the fix = test hardening, prevention guidance). - SELF_HOSTED_RUNNER.md: slim the inline results to a summary that links to the canonical findings doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * ci: add sequential-on-laptop integration PoC workflow Runs the integration suite sequentially on the mac-laptop self-hosted runner. Sequential avoids the concurrency flakiness that parallel execution hits (see docs/solutions/test-flakiness/...), proving a reliable green integration run on dedicated hardware, offloaded from GitHub. Security: does NOT trigger on pull_request. On a public repo a pull_request trigger would let a fork edit this workflow file (the version used for pull_request comes from the PR head) and run arbitrary code on the self-hosted runner - an in-workflow same-repo guard cannot protect itself. Instead we trigger on push to this repo's own branch (write access required; forks cannot push) plus manual workflow_dispatch. continue-on-error so it's FYI, never a required gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs: record sequential-on-laptop result (offload-only) + keep runner as harness [skip ci] Apples-to-apples on the same commit, both sequential: mac-laptop 12m00s vs GitHub-hosted 11m38s - the laptop is ~3% SLOWER, both green. Sequential is I/O-bound so real cores sit idle; the runner gives no speed/reliability edge over GitHub for sequential, only offload. Its real advantage is parallel (~7-10x), which is the flaky path. So sequential-on-laptop isn't itself a win. Keep the self-hosted runner + the sequential PoC as the harness/building block for hardening the parallel tests (fast iteration on real hardware: a full parallel run is ~90s there). May not be needed once tests are hardened - back-pocket. The real unlock is test hardening. Also add paths-ignore (md/adoc/docs) to the PoC push trigger so future doc-only pushes don't spend a 12-minute laptop run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs(agents): be extremely careful modifying tests under stress — failures may be real bugs Records the rule for all agents: a test failing under parallel/contended load may be exposing a real main-code concurrency bug, not flakiness. Don't loosen timeouts/weaken assertions/serialize to go green until you've established contention-artifact vs library bug (e.g. run the test on an uncontended/own broker). Prompted by finding that the RebalanceEoSDeadlockTest contended failure maps to the real confluentinc#857 deadlock (PR #29). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * ci: run GitHub-hosted integration in forked per-broker mode (measure vs self-hosted) forkCount=4 + reuseForks: each JVM fork gets its own TestContainers broker, removing the shared-broker contention that made thread-parallel (-Dparallel-tests=true) flaky. 5/5 green locally on the Mac in ~4min. This runs it on GitHub-hosted to compare speed (rollout decision: run parallel integration on GitHub vs the self-hosted runner). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * test: forked per-broker parallelism for reliable+fast integration (Step 1) [skip ci] Root cause of the parallel-integration flakiness is ONE shared TestContainers broker contended by thread-parallel tests. Fix: run failsafe forked (forkCount=4, own broker per fork), sequential within each fork -> uncontended. Reliable (5/5 green Mac; green GitHub-hosted) AND faster than sequential everywhere (Mac ~4:06, GitHub 6:33 vs 11:38). Does not mask anything - each test runs on an uncontended broker. - self-hosted-tests.yml: integration now uses forked mode (per-suite matrix args); performance keeps thread-parallel. - findings doc: forked-mode resolution + GitHub-vs-Mac numbers + the key discovery that the contended RebalanceEoSDeadlockTest failure maps to the REAL confluentinc#857 deadlock (PR #29) - so contention was exposing a real bug, not just flaky tests. Two-step road recorded: (1) forks now; (2) merge confluentinc#857 lock fixes and retry full thread-parallel to validate. [skip ci]: docs + runner-only workflow; no functional code change to test here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs: mark Step 2 (full-parallel + confluentinc#857) DEFERRED until confluentinc#857 lands [skip ci] Per decision: ship Step 1 (forked parallelism) now; do not merge the ~454-line WIP confluentinc#857/#29 concurrency refactor just to retry full thread-parallel. Reproducer (-Dparallel-tests=true) recorded for after confluentinc#857 is finished on its own merits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs(inflight): record parallel-integration flakiness resolution (forked mode, PR #68) + deferred Step 2 [skip ci] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs: note forkCount portability/opt-in default (+ run required CI on head) Adds the forkCount portability note (opt-in via -DforkCount; failsafe default 1 preserves current behaviour; memory scales per fork). Also re-runs the required Unit/Integration/Performance checks on #68's head, which earlier docs commits (marked skip-ci, brackets omitted here on purpose) had left unrun. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs(inflight): record 'fork the unit tests' as the next follow-up After Step 1 forked the integration suite, unit tests (still sequential) are the CI long pole (~8.5min). Records the analysis - RunLengthEncoderTest 81s outlier + heavy batch/app tests across all modules - and the two follow-ups: fork unit tests (surefire forkCount, same per-fork-isolation pattern) and trim RunLengthEncoderTest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv * docs+ci: address PR #68 review - remove sequential PoC, fix stale forked-mode references Both @claude review passes flagged doc/comment staleness from the thread-parallel -> forked-per-broker switch; the mechanism itself verified clean. This makes the PR internally consistent. - Remove .github/workflows/laptop-sequential-poc.yml: it was only the sequential experiment (superseded by the forked mode shipping here), not the laptop runner (self-hosted-tests.yml, which stays). Its push trigger would be dead on master anyway. - Correct integration=forked-mode wording (was -Dparallel-tests=true, which now applies to the performance suite only): self-hosted-tests.yml top comment, bin/ci-integration-test.sh header, CHANGELOG.adoc, docs/SELF_HOSTED_RUNNER.md ("Where the speedup comes from" + "What we measured"). - bin/performance-test.sh: repoint header from the deleted performance.yml to self-hosted-tests.yml. - Flakiness doc frontmatter: resolution_type pending->config_change, severity medium->low (body documents a completed fix). - Reconcile GitHub-hosted forked wall-clock to 6:16 (the run-ID-backed value) in the flakiness doc and inflight.md (were 6:33). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks done
4 of 6 tasks
…uld have inverted its own result The script carried "KNOWN DEFECT: the declines==0 window gate is right for the FIXED arm and WRONG for the CONTROL arm. Fix before reusing on a marginal result." Three things about that turned out to be worse than the line admits. THE GATE WAS NEVER IMPLEMENTED IN EITHER ARM It was a comment above a printf. Nothing scored anything. Implemented per arm now, and the asymmetry is the whole point: a blocking revoke never reaches the decline, it deadlocks, so on the control arm declines==0 is what a SUCCESSFUL reproduction looks like. Scoring both arms by declines would have discarded every real control observation and kept the empty ones - the arm that proves the defect exists would have been scored "no data". FIXED needs declines > 0; CONTROL needs a failure or a timeout; either arm reports DID-NOT-RUN when the failsafe report says no test executed, so a build that ran nothing can never be counted as a clean run. IT COULD ONLY EVER WORK ON ONE MACHINE Both trees and the JDK were hardcoded absolute paths - one of them a worktree on experiment/857-deadlock-control-arm-do-not-merge, a branch whose name says it is temporary. Anywhere else it measured nothing and reported that only as an empty tally. Both trees are now required arguments, validated for an mvnw, with JAVA_HOME required. IT COUNTED WITH THE DEFECT THIS REPO ALREADY FIXED AND DOCUMENTED $(grep -c ... || echo 0) captures "0\n0" on no match, because grep -c prints 0 AND exits 1. The declines and timeout columns are the two the experiment reads, and they would have been corrupted in exactly the no-match case the probe exists to detect. bin/lib/chaos-experiment-common.sh has carried pc_count_matches and a note about this trap the whole time; this script used neither it nor pc_failsafe_stats, which it re-implemented inline and so had no notion of a run that executed nothing. OTHER INSTANCES OF THE COUNT DEFECT: ONE CANDIDATE, CHECKED AND DISMISSED bin/test-check-quarantine-registry.sh matches the pattern and is a RED CONTROL - it reproduces the broken form on purpose so two tests can prove the real implementation differs. It was "fixed" during this sweep and its own tests immediately went red, which is the control doing its job. Reverted, and it now carries a comment saying so, because the function name alone does not and the next mechanical sweep will land on it too. Everything else matching `grep -c` in bin/ uses `|| true`, which is safe: grep prints 0, true adds nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…sumption-multi-consumers-bug Corrects this PR's own note, so it has to land here: the note is deleted when this PR merges, and a correction living anywhere else would be deleted with the thing it corrects still wrong. #183 asked to be assessed on its own rather than counted in the confluentinc#857 family. That call held; both candidates the note nominated for the assessment did not. #344 corrupts committed offset metadata and so destroys records at restore, which cannot produce the record-returns- after-restart signal the report leads with. #108 records a rejected post-departure commit as successful, producing duplicates rather than skips. What settled it was an elimination against tag 0.5.3.1 - the build the report was filed against - rather than a shape match, which is why it is worth having: ProcessingShard's containsKey-then-drop is the only path in core that silently loses a single record for a still-assigned partition on that build. The two findings that closed the gap are recorded with it, including that the reported [1,2,3,5,6,7...] requires KEY ordering because PARTITION or UNORDERED would have frozen the whole partition at offset 4. Corrected beside the original rather than over it, so the reasoning that nominated the two wrong candidates stays readable - and the "no rebalance is mentioned anywhere in the report" observation is kept as the closure's residual rather than dropped, because the report describes no topology at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…o its instrument branch THE PLAN IS DELETED, AND THE ARGUMENT FOR KEEPING IT WAS WRONG It was defended here on the grounds that a reader arriving from one of the three PRs it produced would need its reasoning. Checked: none of #393, #336 or #376 references it. Its only durable citation was one line in the architecture-patterns write-up; the other three were in notes that are deleted when this PR lands. Nothing was lost, because that write-up already carried all four clusters as sections, in more detail than the plan had - the measured counter table, the eager-versus-cooperative reasoning. What it did NOT carry is the observation the plan could not have made either, so that is added: clusters 3 and 4 were the same defect twice. Each kept a local copy of a fact something else already owned, and the fix in both was to delete the copy rather than correct it - the load gate derived by conservation, the pause read from Kafka. The tell, stated for next time, is a field whose bugs are all of the form "X changed and Y did not": that is a second source of truth asking to be removed, not an accounting error to be balanced. Three citations repaired. Two quote or lean on the plan's own text, so they point at history (`git show a0ade8a:...`) as docs/citations.md prescribes, rather than being cut. THE A/B DEADLOCK PROBE MOVES TO #405 Same reasoning as the torture harness it now sits beside: an overnight instrument, run by hand, in this PR only because it was written here. Its A/B results stay - those are a finding about the deadlock, not a property of the instrument. That PR also gains a note on why neither instrument runs. The short version is that the obvious reading - "a scheduled soak lane needs building" - is wrong: experiments.yml already is one, and the real gap is that an A/B needs a control tree the lane cannot pass, and an eight-hour design does not fit a four-hour cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…t of its open list The note's open sections had accumulated entries already marked DONE, CLOSED or DECIDED - some for days - so the list read as far more outstanding than it was. Six are moved into a "Closed out on this PR" section below the "Already fixed" heading, and the review-deferral entry is closed with them now that the review has run green. Moved rather than deleted: each says what was decided and why, which is the part a reader arriving late actually needs, and deleting them would leave the decisions unexplained on a PR whose whole argument is that the reasoning matters as much as the diff. WHAT THIS DOES NOT DO, AND THE REASON IS IN THE GATE bin/check-pr-ready.sh reports "N item(s) above its 'Already fixed' heading", and that count did not move. It counts every line beginning with "- " or "**", not open items - so it fires on the handoff section, the paused-merge section and every prose bullet, and it can only reach zero if the note is emptied above that heading. Emptying it would destroy the record the note exists to be. So that blocker is structural rather than a to-do, and should be read as "this note has content" rather than "this PR has 52 outstanding tasks". Recorded here because the next person to run that check will otherwise try to clear it the same way and reach the same wall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…loed streams, and nothing could tell Review asked why the log streams are only in unit tests and not integration. The premise turned out to be inverted, and what was underneath it is worse than the question assumed. NOTHING SELECTS THE INTEGRATION PROFILE No pom, no bin/ script and no workflow passes -Dlogback.configurationFile. Both configs are copied to target/test-classes, and logback's default lookup takes logback-test.xml - so integration tests read the UNIT profile and have been getting the silos by accident, not by design. logback-test.xml asserted the opposite: "The integration suite has its own profile, selected explicitly". That was never true in practice, and believing it is how the integration profile came to have no file appenders at all without anyone noticing. THE TRAP THIS LEFT logback-integration-test.xml's own header instructs a reader to select it with -Dlogback.configurationFile. Doing that would have silently dropped every siloed stream - errors, warnings, probes, harness, and the three PC subsystem streams - because that profile defined only STDOUT. A logging config that loads and quietly writes nothing is the same silent-failure shape the file's own header warns about two paragraphs earlier. THE FIX The silos move to logback-silos.xml, included by both profiles, so they cannot drift apart and neither can lose them by being the one selected. Levels are still inherited, so pc.log.level still governs and bin/check-test-log-config.sh still holds - that gate scopes itself to the four modules' src/test/resources/logback-test.xml and never covered the integration profile, which is why nothing went red for this. MEASURED, WITH A CONTROL ARM, BECAUSE A FAILED INCLUDE IS SILENT An <include> that does not resolve logs a warning and carries on, which would have reproduced the exact defect being fixed. So both profiles were run with -Dpc.log.dir at a scratch path and the directory listed: default profile writes all seven silo files, integration profile writes all seven. Control arm on the pre-fix integration profile, same command: zero files. The unit profile's output is unchanged by the extraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…s its header requires The note carried `post-merge: exempt-file - this note IS the record of #29 ... and is deleted when the PR lands`. Merging without deleting it would land the in-flight scaffolding of a finished PR on master, which is the one thing docs/inflight/AGENTS.md says a note must not do. Its durable content already left over the course of the work, which is why this is a deletion rather than a loss: the deadlock mechanism and its four-cell evidence are in docs/solutions/runtime-errors/revoke-path-commit-deadlock-between-poll-and-control-threads.md, the throughput defect in the slf4j write-up, the four clusters in the two-threads-one-consumer architecture doc, and the open items in bug-857-family.md, which explicitly must NOT be deleted with this PR - two of its five open items reproduce on the fixed tree. The one citation to it, in branch-stall-experiments.md, already anticipated this: it named the note "while #29 is open, and its solutions write-up once it lands". Repointed to the write-up. Retrieve the note from the branch tip if the narrative is ever wanted: `git show fd8f5c2:docs/inflight/pr-29-857-deadlock-and-what-the-measuring-taught.md` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…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
… lacks The note was titled "as the closest thing to a production deployment" and opened by explaining that nobody here has a production deployment to run the library in, making the soak a "nearest available substitute". That frames a good instrument as a consolation prize, and it says more about the project's circumstances than about the test. The technical case does not need it: the defects this work keeps finding are the ones that need time and load to appear, so a suite that runs for minutes per PR is weakest against exactly the shape of defect that keeps turning up. A long soak supplies time and load, and the machine is idle overnight either way. That argument stands on its own. Renamed with it - the filename carried the same phrase, and nothing cited it. Swept the rest of docs/ for the same framing: this file was the only instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…s-bug Master gained one commit since this branch last merged it - #407, the CI comment work - and the only files touched on both sides are the two sighting registers. BOTH SIDES' SIGHTINGS SURVIVED, WHICH IS THE ONLY THING THAT MATTERED HERE These are append-style registers, so a resolution that picked a side would have silently dropped somebody's evidence. Checked rather than assumed after the merge: #407's sixth 857 capture and its seed are present, its committedOffsetRemoved sighting is present, and this branch's own fifth open item and its withdrawal of the "never a product defect" claim are both still there. ONE INHERITED LINE NEEDED AN ATTESTATION #407's capture entry proposes replaying a seed with this branch's tryLock() applied - a sentence that reads correctly after the merge, but which the self-reference gate cannot know is safe. Marked, and the reference fully qualified while there. The marker had to move: it sat above the paragraph's first line, and the gate's single-line form covers only the line immediately after it, which was not the one carrying the reference. The paragraph is rewrapped so the reference sits directly under its marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
⚪ Throughput — no comparable reference
5 baseline run(s) were found and none ran the same set of test cases as this run, so none is comparable. This is usually the PR's own doing: enabling or disabling a test in this lane changes the matrix.
The baseline rows are shown for orientation only. They ran a different workload, so a difference between them and this run is not a measurement of anything - do not read one as a regression or an improvement. Refusing to compare against a run whose workload differs, rather than averaging the difference in and calling the result a verdict. Updated for |
…ces sighting before its log expires CI run 33579370081 on head 650a823 failed `MultiInstanceRebalanceTest.largeNumberOfInstances` again - the second failure on the fixed tree, against one pass. The signature is the one this note already carries twice: detector verdict FLAT ("it stopped rather than ran out of time"), seven ZOMBIE_MEMBER/REBALANCE_BLOCKED violations with the group dwelling in PreparingRebalance at the 15s bound, and the whole assignment frozen with comparable lag. Recorded rather than chased, and rather than re-run. A green re-run of a test whose failure is not understood is not evidence the failure went away, and this note exists precisely because CI logs expire and take the only copy of the evidence with them. Nothing was disabled, loosened or retried. Two things this sighting adds that the earlier two could not. The scenario is PERIODIC_CONSUMER_ASYNCHRONOUS, and the deadlock this branch fixes closes only in PERIODIC_CONSUMER_SYNC - so the mode alone rules the revoke-path fix out as the cause, independently of the reasoning already recorded from the 2026-09-01 CI failure. The rebalance dwell peaks now read 15441ms, 15584ms and 15531ms across the three failures - under 150ms of spread on a quantity free to be anything, while the stagnation peaks beside them vary by six seconds. A dwell that lands on the same value every time looks like a bound being hit rather than a wait being measured, which would make rebalanceDwell a reading of some timer rather than of the member's actual delay. Recorded as an observation, not a finding: three points notice a pattern and cannot attribute one, and the probe's own violation bound is also 15s and may be doing some of the work. Identifying which bound costs reading the group's rebalance.timeout.ms against it. There is no seed to record. This test is not a chaos scenario and constructs no ChaosSeed, so no replay exists for it the way it does for the bug-857-family captures - the durable identifiers are the run and job ids, the head, and the group and topic ids, and the note now says so rather than leaving a future reader hunting for a seed that was never there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…ix the lane that would have ignored it The rebalance stall this test keeps producing is measured and unexplained: detector verdict FLAT - the record count stops rather than slows - with the ambient probe reporting ZOMBIE_MEMBER/REBALANCE_BLOCKED, the group dwelling in PreparingRebalance because a member stopped answering, and the whole assignment frozen at comparable lag. One failure in ten consecutive runs on an idle Linux box, plus repeated CI failures, always that signature. It reproduces on the tree carrying this branch's log-argument fix, so it is neither the revoke deadlock nor the SLF4J argument-evaluation defect but a third mechanism nobody has explained. Quarantine rather than disable, because the two are opposites. The lane keeps running the test on every PR push and every merge to master while holding it out of the gating suites, so the signal that would answer the open question survives and only the random merge-block goes away. Disabling it is what master does today, and it is why no master-state ledger for this test can exist. RULE 2 IS SATISFIED PROSPECTIVELY, AND THE TENSION IS STATED RATHER THAN GLOSSED. The discipline wants a ledger showing the failure is master-state, and today it is not: the test is disabled on master, so its failures exist only because this branch enables it. That inverts on merge - master would inherit a required check failing about one run in ten. The quarantine therefore lands in the same change as the enablement, at exactly the moment the failure becomes master-state, rather than after a merge spends a week teaching everyone to read that lane's red as normal. The annotation and the registry entry both say so in those terms. flapping = true, which is not decoration. The default is strict-xfail: a PASS on a deterministic quarantined test opens a merge-blocking review thread demanding the annotation be deleted. This test passes most runs, so without the flag its very next green run would have blocked a merge. Only largeNumberOfInstances. cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance have no recorded failure, and quarantining a test that has never failed spends the mechanism's credibility for nothing. THE ANNOTATION ALONE WOULD HAVE DONE NOTHING, WHICH IS THE PART WORTH READING. bin/performance-test.sh passed -Dexcluded.groups= - empty - and an override REPLACES the pom's default list rather than adding to it. Empty is correct for `performance`, since running those is the entire point of the lane, and silently wrong for `quarantined`. Because Performance Tests is a REQUIRED check, a quarantined performance test would have kept gating merges while the annotation compiled, the registry listed it and the lane report described it - none of which stops a merge. That is the same silent no-op the @Quarantined javadoc records having already happened once, when the exclusion was not wired for unit tests. bin/chaos-test.sh already pairs its included.groups with this exclusion; the performance lane was the one that had not. Verified rather than assumed, since a silent no-op is exactly what this class of change produces: running the perf lane's own flags against the test now reports "No tests matching pattern MultiInstanceRebalanceTest#largeNumberOfInstances were executed", with no broker container started. The pom's adjacent guidance comment claimed the script passes an empty value, so it is corrected in the same commit rather than left to drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…t criterion that matches what it became The file's "Delete when" said it could be retired once the deadlock PR landed and the remaining open item had a solutions write-up. That was written when it was a note about one investigation. It has since become a register: the place this family's mechanisms are kept apart from each other, because upstream's single symptom has several causes and every attempt to treat them as one defect has attributed one mechanism's evidence to another. So the criterion had stopped describing the file, and the previous fix for that was a paragraph underneath telling readers not to apply the rule above it - a criterion and a warning not to use it, which is worse than either alone. The rule now states what the register is for, what earns an entry, and what would actually retire it: every open mechanism in it being explained or closed. Not a PR merging. A fix retires its own mechanism's section, never the register, because what remains is by definition about the mechanisms that fix does not reach. The deadlock is folded in as the worked example rather than left as a contradiction beside the old rule: its write-up exists and its section may go, while two of the register's open items reproduce on trees already carrying that fix - so retiring the file on that merge would delete two live unexplained stalls. The superseded status paragraph is deleted, since the rewritten criterion says what it was there to warn about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…egration test actually runs The lane reported `MultiInstanceRebalanceTest.largeNumberOfInstances` as "not run" on a run that succeeded. It was not a stale comment: the test genuinely never executed. The lane runs `clean verify` across both suites. `ProducerManagerTest` is quarantined and fails by design, so surefire failed the module, `parallel-consumer-core` went FAILURE, the reactor stopped, and failsafe never ran. The quarantined INTEGRATION test therefore executed nowhere - excluded from the gating lane by its annotation and never run by the lane meant to keep its signal alive. That is `@Disabled` with extra steps, which is precisely the outcome quarantine exists to prevent. NOTHING WENT RED TO SAY SO, which is the part worth keeping. The workflow step is `continue-on-error`, so the job reported success; the registry check passed, the audit listed the test, and the lane comment said "not run" next to a green tick. Every surface agreed except the one that mattered. The hole was latent rather than new. With only quarantined UNIT tests, surefire ran them all before the build died, so stopping early cost nothing and the ordering dependency was invisible. It opens the moment a quarantined integration test exists, and this branch adds the first one. `-Dmaven.test.failure.ignore=true` lets both phases run to completion and write their XML, which is where the verdict is actually read from: `bin/quarantine-lane-report.sh` parses the reports, and the workflow's own comment already states that a green tick here does not mean the quarantined tests passed. So the exit code was never this lane's verdict; it is now explicitly not one, and the script's header says why rather than leaving the flag to look like a way of dodging a failure. Verified from the reports rather than the exit code, since a run that executes nothing also exits 0. Locally, after the change: `failsafe-reports/TEST-...MultiInstanceRebalanceTest.xml` carries `tests="1"` with `<testcase name="largeNumberOfInstances" time="204.285">`, and `surefire-reports/TEST-...ProducerManagerTest.xml` carries its expected error in the same run. Both phases execute; exactly one method of the rebalance class is selected, confirming the quarantine did not widen past the one test it names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
This was referenced Sep 2, 2026
Merged
Merged
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
4 participants
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.
Serves #119 (mirror of confluentinc#857) — deliberately not a closing keyword; see "What this does not close".
depends on #407
What this is
onPartitionsRevokedruns on the poll thread and tooksynchronized(commitCommand)to commit before giving up partitions.commitOffsetsThatAreReadyruns on the control thread, takes the same monitor, then waits on the poll thread to service the request. Each holds what the other needs — an AB-BA cycle whose symptom is the group going quiet after a rebalance and staying quiet until a restart.The monitor becomes a
ReentrantLockthat the revoke path declines rather than blocks on. If the control thread holds it, the revoke gives up its commit and proceeds; the offsets are re-derived after the rebalance. Losing a commit is safe, not returning from the callback is not.It closes only in
PERIODIC_CONSUMER_SYNC— the other modes never block on the commit path, so the second edge does not exist. That is why the family's sightings had to be split by commit mode before any could be attributed.The evidence
A deterministic probe forces the window open rather than waiting for chaos to find it; chaos seeds never opened it. Four cells, {fix, pre-fix} × {eager, cooperative}, twenty repetitions each — both pre-fix cells fail throughout, both fixed cells pass throughout. So the fix is not assignor-specific, which mattered because the family's twentieth capture was a cooperative revoke.
The probe is the deliverable, not the number: a green run of a probe that never opened the window is indistinguishable from a fix, and that is how this defect looked unproven for four months.
A second main-code fix, for a defect this branch introduced
The control loop passed a shard-wide sum as a plain
log.traceargument. SLF4J defers formatting, not argument evaluation, so it ran every pass at every log level — and the cost grows with in-flight key cardinality while the loop spins fastest under saturation, so both peak together.It cost roughly 40% of throughput on a constrained runner. Establishing that used the same discipline: one term changed, lane composition ruled out by measurement rather than argument, and the neighbouring test classes as the control for machine speed. Independently reproduced on a development box, arms interleaved so run order could not explain the gap.
What else is here
describeProgress()plus a coherence warning on the engine, and a pause observation on the broker poller.MultiInstanceRebalanceTestcapacity profiles now enabled in the performance lane rather than silently disabled.@Disabledwith extra steps. A quarantined unit test fails by design, surefire failed the module, the reactor stopped. Nothing went red to say so: the step iscontinue-on-error, so the job was green, the registry check passed, and the lane comment said “not run” beside a tick.-Dlogback.configurationFile, so integration tests fall back tologback-test.xmland were getting the silos by accident. Selecting the integration profile as its own header instructs would have silently dropped every stream. The silos now live in one shared include used by both profiles, verified at runtime against a control arm.What this does not close
confluentinc#857 has more than one mechanism; this fixes one. #119 stays open deliberately.
largeNumberOfInstancesstalls at a rebalance with a member not answering, reproduced on a tree carrying this fix — so neither the deadlock nor the logging defect. Open in the family register, and quarantined rather than disabled: the lane keeps running it on every PR push and every merge to master while holding it out of the gating suites, so the signal that would explain it survives and only the random merge-block goes. The quarantine lands in the same change as the enablement — at the moment the failure becomes master-state — rather than after a merge teaches everyone to read that lane's red as normal.History
What began as one April commit bundling four independent changes is now the one cluster that had evidence. The other three landed separately — #393 (
ThreadConfinedConsumer), #336 (the load-gate counter) and #376 (the back-pressure pause) — and two of them turned out to be the same defect twice: a local copy of a fact something else already owned, fixed by deleting the copy rather than correcting it.The merge order this PR waited behind — all now merged
Kept as the record of what this branch was carved into and what it waited for. Every one of these has merged, so the dependency gate is satisfied; the list is history, not a live constraint.
depends on #323
depends on #324
depends on #325
depends on #57
depends on #322
depends on #267
depends on #375
depends on #376
depends on #381
depends on #393
depends on #401
Two carried instructions that have since been discharged, recorded because a reader tracing the history will meet them in the commit log. #325 carried chaos instrumentation extracted from this branch, and had to merge first so it was not reviewed after its own content was already on master. #57 carried the metrics-teardown fix extracted from here in a strictly better form — it gained a
PCMetrics.close()guard this branch never had — so the instruction at the time was to take master'sPCMetrics.javaat the merge. Both happened.Checklist
docs/features/— N/A - a defect fix, not a featureHotPathLogArgumentsAreDeferredTest, and three capacity profiles enableddocs/inflight/working note started at the PR's first commit —docs/inflight/pr-29-857-deadlock-and-what-the-measuring-taught.mdce-simplifyandce-code-reviewlocally — N/A - asked for@claude review thison the PR instead