fix(core) #177: report the poll thread's real error, not the commit-response timeout - #204
fix(core) #177: report the poll thread's real error, not the commit-response timeout#204astubbs wants to merge 9 commits into
Conversation
…esponse timeout "InternalRuntimeException: Timeout waiting for commit response" is what users report (#177, upstream confluentinc#833). It is a symptom. The broker-poll thread is the only producer of commit responses, so when it dies every later sync commit waits out offsetCommitTimeout and throws a message naming neither the failing subsystem nor the failure. Verified first, then fixed. The triage that called this already-fixed-unreleased by #100/#108 is right about the TRIGGER: with the RebalanceInProgressException catch removed, the reported chain reproduces exactly. But those PRs removed two ways to kill that thread, not the misleading symptom - broker down, offset encoding and authorization all still produced it, and fixing exceptions one at a time was never going to close that. - AbstractParallelEoSStreamProcessor: supervise the poller when a commit fails, so its actual exception becomes the reported cause and the commit timeout is retained as suppressed. Neither is lost. This had to go on the commit FAILURE path: supervising earlier in controlLoop() was tried and measured, and does nothing, because the poller dies while servicing the commit the control thread is already blocked on, so that thread never reaches the top of the loop. - ConsumerOffsetCommitter: the message interpolated the unrelated constant DEFAULT_TIMEOUT, so it claimed PT30S no matter how offsetCommitTimeout was configured - overstating the shipped default by 3x. The reporter's "PT30S" therefore said nothing about their settings. Report the timeout actually waited, and say what a timeout here usually means. - ConsumerManager.commitSync: the retry budget was captured INSIDE the retry loop, so every attempt reset it. Whenever an attempt failed faster than the budget the comparison could never become false and it retried forever with no backoff - measured at 51 attempts against a 500ms budget. PC then neither committed nor failed: the poll thread was stranded inside one commitSync while the control thread waited out its commit response, which is this same bug arriving from the broker-down direction rather than the rebalance one. Capture it once per call, matching poll()'s pollStarted - the same budget in the same class, done correctly, which is the strongest evidence the difference was accidental. This makes PC give up where it previously hung; it does not make it keep running anywhere it previously stopped. Tests, each confirmed discriminating by reverting the fix and watching it fail: - CommitResponseTimeoutSymptomTest reproduces the reported workload - 1000 keys, ~50% failing, KEY ordering, rejections recurring for the whole run rather than only at start-up, fed over time because the report is of a consumer that runs for a while. Without the catch it dies with the reported message after 123s; with it, 20 commit cycles and 6 deferrals and a full drain in 3.4s. Its second test pins the diagnosability half: without the supervise path the real cause is absent from the chain entirely. - ConsumerManagerCommitRetryBudgetTest bounds attempts rather than hanging when the budget is not enforced, so the unbounded case fails instead of wedging CI. Deliberately NOT done, recorded in docs/inflight: releasing waiters at the moment the poller dies, rather than after offsetCommitTimeout. That needs new cross-thread liveness state, and the diagnosis is already correct without it. Verified: full unit suite green, 11 modules; core 321 tests. Upstream-Issue: confluentinc#833 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 8m 44s —— View job Claude finished the reviewTasks
SummaryThis is a well-diagnosed, well-tested fix. I verified the two central claims directly rather than trusting the PR body:
Design review
Minor, non-blocking
No correctness issues found. The fix is narrowly scoped, the tests are genuinely discriminating (verified, not just claimed), and documentation/AGENTS.md conventions are followed correctly aside from the one stale manifest field noted above. |
✅ 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)
|
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
The entry was written when the work was still on a branch, so it claimed `prs: []` / `status: in-progress` while PR #204 was open against it. AGENTS.md requires the transition in the same commit that causes it, precisely because nothing automated catches this: `upstream-map.py validate` only checks the schema, so a manifest that says `prs: []` with a live fork PR passes every gate and rots silently - the same drift a 2026-08-04 audit found in five entries. Raised by the automated review on #204. Upstream-Issue: confluentinc#833
Review responseAddressing the automated review, the two duplication reports, and the red 1. Stale manifest entry — valid, fixed in 70a1f1aCorrect finding, and
This is exactly the drift the AGENTS.md note warns about — 2. Duplicate-code report — 2 new clones, both import preambles. No refactor.Both flagged clones are the mandatory Java header/import block, not logic:
Two tests that both drive a 3. File-similarity report — pushing back on the 39.7%
The classes are not the same test:
Merging them would mean parameterising the base over workload, feed strategy, rejection schedule and assertion set — four axes to save an import block and a 4. The red
|
Claude is reviewing PR #204
Tasks
|
The ledger already warns that a GREEN review check can mean the reviewer never ran. The inverse also holds and cost this PR a full diagnosis pass, so it is worth writing down rather than rediscovering. check-review-posted.sh pipes comment bodies into `grep -q` under pipefail. `grep -q` exits on first match, `printf` takes EPIPE on the remainder, and pipefail turns that into the pipeline's status - so locating the review early is precisely what makes the gate report "no review posted". Deterministic at the 64 KiB pipe buffer: measured on this PR at 65536 bytes -> PASS, 70000 -> FAIL, with the run id present and correctly cited throughout. The failure mode is backwards - it fires when the reviewer works and the PR is busy - and worsens as comment payloads grow. The error text also blames the workflow-validation guard, which sends the reader after a .github/ diff that is not there. Not fixing the gate here: a CI gate patched by the PR it is blocking certifies nothing. Recorded so it can be fixed on its own.
|
Claude finished @astubbs's task in 9m 33s —— View job Code review
SummaryI re-verified this independently rather than re-reading the prior review's conclusions. Both central regression claims check out:
I also checked On the follow-up commits (review response)
On the flagged
|
…ize-based The first write-up said the failure was deterministic on total payload size, measured by truncating this PR's comment list. That was the wrong variable: truncating from the end also shrinks the bytes AFTER the match, which is what actually matters. `grep -q` stops at the match, so what decides the outcome is whether `printf`'s remaining write fits the 64 KiB pipe buffer. Measured against the gate: 65000 bytes after the match PASSes, 66000 FAILs, and a 500 KB payload with the match at the end PASSes. This PR demonstrated both ends - red when the review comment sat 2nd of 5 behind the ~130 KB similarity report, green on the next run at a LARGER 149 KB total once the newest review landed at the end with 4 KB behind it. The practical consequence is worse than "it fails on big PRs": it is positional and intermittent, so a single green run is not evidence the gate is sound.
Correction: my
|
| bytes after the match | gate |
|---|---|
| 65000 | PASS |
| 66000 | FAIL |
| 200000 | FAIL |
| 500 KB payload, match at the end | PASS |
This PR demonstrated both ends, which is the useful part. It failed at 137 KB total with the reviewer's comment 2nd of 5 and the ~130 KB file-similarity report behind it. It passed on the very next run at a larger 149 KB total — because the newest review comment was appended at the end, leaving only 4142 bytes after the match:
total bytes = 149502
first match byte offset = 145360
bytes remaining after match = 4142 -> under 64 KiB -> PASS
Why this matters more than the original claim. The bug is positional and intermittent, not size-monotonic. It fires whenever a review lands early in a comment list with bulky bot reports behind it — which is the normal shape whenever the reviewer edits its comment in place rather than appending a new one. So:
- A green
claude-reviewis not evidence the gate is sound. This PR went red then green with the review correct and correctly cited both times; nothing about the reviewer changed. - Re-running a red
claude-reviewcan flip it green for reasons unrelated to the review, which is the worst property a gate can have — it trains people to hit retry.
Corrected in docs/inflight/ci-review-agent.md in 9cb5e58. The fix is still the one-liner (grep -qE … <<<"$comment_bodies", no pipe, no EPIPE), and still belongs in its own PR — I have not touched the gate here.
Everything else from my previous comment stands: the manifest entry is fixed (70a1f1a), and the two duplication findings are import preambles and a package-floor identifier overlap respectively, which I am not refactoring.
On the ConsumerManager.commitSync behaviour change — the second review's recommendation is to keep it in this PR, on the grounds that it restores what offsetCommitTimeout already documents rather than introducing new behaviour, and that an opt-in flag would mean adding a knob to preserve a bug. I think that reasoning is right, but it is still a maintainer call, not a reviewer one, and I have flagged it rather than treating the review as sign-off. The cost stands as stated: a broker outage longer than offsetCommitTimeout where a later attempt would have succeeded now shuts PC down instead of retrying forever.
|
Claude finished @astubbs's task in 8m 7s —— View job Claude finished @astubbs's task in 9m — View jobCode reviewTasks
What changed since the last reviewOnly Independent verification (not re-trusting the prior passes)I reverted each fix in the working tree, rebuilt, and ran the discriminating test — then restored and re-ran to confirm green:
AssessmentBoth load-bearing regression claims hold up under independent reversion, not just re-reading the prior reviews' conclusions. The three source fixes ( On the flagged |
Scope note for whoever picks this upThis PR deliberately does not close #177, and there is no closing link in the description. On merge, #177 stays open. Intentional, and the reasoning matters more here than on the sibling PRs. Done here: the reader-facing half. The error now names the real failing subsystem instead of Not done, and it is a class rather than a bug: the broker-poll thread is the only producer of commit responses, so any exception that kills it turns every later sync commit into the same message. #100/#108 removed two such exceptions. Broker-down, offset-encoding and authorization failures still produce it. Fixing exceptions one at a time never closes this - it wants a supervision change, which is out of scope here. Two things needing a maintainer decision before merge, not a reviewer's:
|
check-review-posted.sh:94 ran `printf | grep -q` under `set -o pipefail`. grep exits the instant it matches, printf then dies with EPIPE (141), and pipefail promotes that to the pipeline's status - so a comment citing the run made the check fail. It only bites when more than one pipe buffer (64 KiB) of comment data follows the match, which is why it looked intermittent. Observed on #198, #199, #204 and #210. On #210 the review comment sat 4.7 KB into the stream with a 127 KB similarity report behind it; the job log carries `printf: write error: Broken pipe` directly above the failure. The emitted error blamed the workflow-validation guard, which none of those PRs had tripped. The fix is the herestring this repo already prescribes: the same bug class is guarded against in bin/test-check-copyright-headers.sh, whose comment records it "seen live in CI". check-review-posted.sh shipped in that same commit without the guard. Two tests, both verified to fail against the old line: - functional: match found, then >64 KiB of further comments. The existing cases dance around this - case 5 buries the match but keeps it small, case 6 puts it last so nothing follows to fill the buffer. - structural: the checker may not pipe into grep -q or awk at all, mirroring the copyright scanner's guard, so the next instance of the class is caught rather than the next occurrence of this one. The error text is left alone: it misdiagnosed those four PRs only because of this bug, and becomes accurate again once the SIGPIPE path is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
#211 added a rule to bin/AGENTS.md - never pipe into `grep -q` under pipefail - while the repo broke it in four places. A rule shipped with known violations is not a rule. `writer | grep -q PATTERN` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Adds bin/check-shell-sigpipe.sh, run in CI beside the copyright self-test (seconds, no JDK) and granted to the reviewer. Verified both directions: clean on this tree, exit 1 when the old line is reinstated. It skips itself, since its failure message necessarily contains the anti-pattern as the "wrong" half of a worked example. shellcheck does NOT detect this - run against the known-bad line, it passed clean. Hence a bespoke grep rather than adopting a linter. Note check-review-posted.sh:94 is also fixed in #210, which adds the functional regression test for it. Conflict expected and cheap; this PR fixes it because it is this PR that states the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
`writer | grep -q` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error, sending the reader nowhere near the cause. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Kept separate from the guard that enforces this, so the fixes can be reviewed - and reverted - on their own. shellcheck does NOT detect this: run against the known-bad line, it passed clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
`writer | grep -q` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error, sending the reader nowhere near the cause. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Kept separate from the guard that enforces this, so the fixes can be reviewed - and reverted - on their own. shellcheck does NOT detect this: run against the known-bad line, it passed clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
…changed Merging master brought three things that bear on this branch. The issue-reference gate (735b1d3) flags a bare `#NN` below #1000 on added lines, because the fork's numbers sit entirely inside confluentinc's range. This branch added 12 such refs across javadoc, tests and the inflight note. Now owner-qualified: #177/#100/#108/#186/#204 for the fork, confluentinc#833 for upstream. #177 is itself the mirror of confluentinc#833, so the pair reads correctly. Left `// fixes github issue confluentinc#809` alone - it is a pre-existing line, not one this PR adds, and rewriting it would be scope creep. The SIGPIPE gate bug this PR diagnosed is FIXED on master (cb8b118) and guarded repo-wide (7586b8c), so the inflight entry claiming it still needs its own PR was stale. Rewritten as resolved, keeping the measurements and the lesson: the failure is positional rather than size-monotonic, shellcheck does not catch it, and small fixtures never reproduce it. The manifest lost the backlink tooling and the `todo` subcommand, so this entry's `forwarded:` block and `todo:` list were writing into fields nothing reads any more. Dropped both; the release-time note moved into `notes:`, and `fork_issue: 177` added to match the shape master's entries now use. Worth recording: cb8b118 called out check-quarantine-owners.sh as latent with 351 bytes of headroom "on a file two open PRs are adding lines to". This is one of those PRs, and it takes AbstractParallelEoSStreamProcessor.java to 67,168 bytes - 1,632 past the 64 KiB buffer. Without that fix this branch would have failed as "annotation missing".
Applies 16ac63b's rule to this branch's new test. The rule landed after these tests were written: await the value you are about to assert, never a proxy that leads it. CommitResponseTimeoutSymptomTest had one real instance. It awaited a Set populated INSIDE the user function, then read commitsRejected, which the broker-poll thread increments inside commitSync. Two threads, neither ordering the other. The margin is genuinely large - feeding spans tens of commit intervals - which is what every latent race looks like right up until it loses. Now awaited on the value actually asserted, which masks nothing: if the rejections never arrive it still fails. The other await-then-assert in the same class is the SAFE direction and is now commented as such, because the next reader applying this rule would otherwise "fix" it into a weaker test. supervisorLoop() assigns failureReason BEFORE doClose() sets state=CLOSED and before the throw that completes controlThreadFuture, and isClosedOrFailed() reads only those two later signals - so the awaited signal lags the asserted value. Awaiting a non-null getFailureCause() instead would hide exactly the regression this test exists to catch, since a null cause is that regression's signature. Recorded as a third instance on the existing write-up rather than a new file. Its value is that one class now holds one site of each direction, so the refined rule is concrete: establish which value is WRITTEN first. If the awaited signal is written after the asserted one, the await is sound and tightening it weakens the test. Discriminating power is unchanged - the first await (succeeded.hasSize(KEYS)) is untouched and still runs first.
Updated from masterMerged 1. Issue-reference gate — 12 refs qualified
Two judgement calls worth flagging:
The PR body is qualified too, using the fully-qualified 2. The SIGPIPE bug diagnosed on this PR is fixed on master
One thing worth putting on the record. 3.
|
|
Claude finished @astubbs's task in 3m 5s —— View job Code reviewTasks
What changed since the last full reviewSince the previous pass (which covered through
Verification of
|
CI failed it once on this branch with incompletes [8] where the multiple=50 parameter pins [8, 12]. Recorded rather than reacted to, because the test's name collides with what this PR changes and that invites the wrong conclusion. It is the family signature: the test's own javadoc names BOTH outcomes as physically possible - "just the failed offset (processing finishes during shutdown timeout)" vs "both ... when sleep is longer than the shutdown timeout" - and the parameter pins one. That encodes a timing outcome as if deterministic. The ambient probe reached the same verdict unprompted. Ruled out as a regression from the commitSync retry-budget change on four independent grounds, the first two of which are decisive on their own: - Wrong path. This test is PERIODIC_TRANSACTIONAL_PRODUCER, and AbstractParallelEoSStreamProcessor picks committer = producerManager for transactional mode. ConsumerManager.commitSync has exactly one caller in main, ConsumerOffsetCommitter:96, reached only in the consumer-sync modes. - Wrong direction. The change gives up EARLIER, which leaves MORE incomplete and pushes toward [8, 12]. The failure is [8] - fewer - so the effect points away from it even if the path were shared. - Passes locally with the change in place, 3 tests 0 failures. - The class is already named load-sensitive in two existing write-ups, and its sibling produceTimeout is already in the table. Deliberately not touched: widening the expected set to accept both outcomes would make the assertion vacuous, since those are the only two possibilities.
Integration Tests went red once — diagnosed, not retried away
It is not a regression. Four independent grounds, the first two decisive on their own: 1. Wrong code path — the changed method is never called here. The test runs if (options.isUsingTransactionalProducer())
this.committer = this.producerManager.get();
else
this.committer = this.brokerPollSubsystem;
2. Wrong direction. The change makes 3. Passes locally with the change in place — 4. Pre-existing member of a documented family. The class is already named as load-sensitive in 5. The rerun of the same commit passed — no code changed between the two runs. Why the test is non-deterministicIts own javadoc names both outcomes as physically possible — "just the failed offset (for case where processing finishes during shutdown timeout)" versus "both offsetToError and offsetToGoVerySlow ... when sleep is longer than the shutdown timeout" — and the parameterisation pins one. So the assertion encodes a timing outcome as if it were deterministic. That is the family signature: right when quiet, arbitrary under contention. The ambient probe reached the same verdict unprompted:
What I did not doI did not touch the test, widen a timeout, or add a retry. Widening the expected set to accept both outcomes would make the assertion vacuous, since |
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @astubbs's task in 1m 47s —— View job Code reviewTasks
What changed since the last full reviewOnly one substantive commit landed since the last full review (which covered through I independently verified the classification's claims rather than trusting the write-up:
VerdictNo correctness issues. This increment is docs-only and its reasoning holds up against the actual source (committer selection, single call site, direction of the behaviour change). Combined with the prior rounds' independent verification of the three source fixes ( |
1 participant
Description
Closes the reader-facing half of #177 (confluentinc/parallel-consumer#833):
InternalRuntimeException: Timeout waiting for commit response PT30S, reported on 0.5.3.1 with ~50% of records failing across 1000 keys.This started as a verification task, and the triage it was checking was half right.
The prior pass concluded the bug was already fixed on master by #100/#108, just unreleased. That is confirmed for the trigger: with the
RebalanceInProgressExceptioncatch removed fromConsumerOffsetCommitter.commitDeferringOnRebalance(), the reported chain reproduces verbatim, message and all.But it is not the whole bug. The broker-poll thread is the only producer of commit responses, so any exception that kills it turns every later sync commit into that same message — one that names neither the failing subsystem nor the failure. #100/#108 removed two ways to kill that thread. Broker down, offset encoding and authorization still produced the identical unactionable error, and fixing exceptions one at a time was never going to close that class.
What changed
AbstractParallelEoSStreamProcessor— supervise the poller when a commit fails, so the poller's actual exception becomes the reported cause, with the commit timeout retained assuppressed. Neither is lost.This had to go on the commit failure path. Supervising earlier in
controlLoop()is the obvious move, was tried, was measured, and does nothing: the poller dies while servicing the commit the control thread is already blocked on, so that thread never reaches the top of the loop. Recorded as falsified indocs/inflight/so nobody retries it.ConsumerOffsetCommitter— the message interpolated the unrelated constantDEFAULT_TIMEOUT, so it claimedPT30Sregardless of howoffsetCommitTimeoutwas configured, overstating the shipped default (10s) by 3x. The reporter'sPT30Stherefore told us nothing about their settings. It now reports the timeout actually waited, and says what a timeout here usually means.ConsumerManager.commitSync— the retry budget was captured inside the retry loop, so every attempt reset it. Whenever an attempt failed faster than the budget the comparison could never become false, and it retried forever with no backoff — measured at 51 attempts against a 500ms budget. PC then neither committed nor failed: the poll thread was stranded inside onecommitSyncwhile the control thread waited out its commit response. That is this same bug arriving from the broker-down direction instead of the rebalance one, which is why it is in this PR rather than a separate one.poll()capturespollStartedoutside its retry loop for the very same SASL budget — the same knob, in the same class, done correctly. That is the strongest evidence the difference was accidental rather than designed.Evidence the tests actually catch it
Every test was confirmed discriminating by reverting the fix and watching it fail — a test that passes both ways proves nothing.
RebalanceInProgressExceptioncatchCommitResponseTimeoutSymptomTest#aRebalanceStormUnderAHighFailureRateNeitherStallsNorKillsTheConsumerCommitResponseTimeoutSymptomTest#aDeadPollThreadReportsItsOwnCauseNotTheCommitResponseTimeoutstartedTimeback inside the loopConsumerManagerCommitRetryBudgetTestCommitResponseTimeoutSymptomTestreproduces the reported workload rather than a minimal one: 1000 keys, ~50% failing,KEYordering, and rejections recurring for the whole run instead of only at start-up — fed over time, because the report is of a consumer that "runs for a while". An earlier single-batch version drained in 4 commit cycles and saw 1 rejection, which is not a storm; it now runs 20 cycles with 6 deferrals. A fix that deferred a commit but never re-requested one passes the existingCommitRejectionTestBaseand stalls here.It is deliberately not a subclass of
CommitRejectionTestBase— that base pins a different property (offsets not recorded as successful, one rejection reason, rejected only at start-up) on a workload chosen to isolate it. The reasoning is in the class javadoc.ConsumerManagerCommitRetryBudgetTestbounds attempts rather than hanging when the budget is not enforced, so an unbounded regression fails instead of wedging CI.Deliberately not done
Releasing waiters at the moment the poller dies, rather than after
offsetCommitTimeout. That needs new cross-thread liveness state (thesetCloseInProgressSignalpattern is the precedent), and the diagnosis is already correct without it. Recorded indocs/inflight/bug-177-commit-response-timeout.md.Verified: full unit suite green across all 11 modules; core 321 tests, 0 failures. Copyright scanner clean.
Updated from master
Merged
origin/mastercleanly (no conflicts). Three things on master bore on this branch:The issue-reference gate (
735b1d3a, #114) fails a bare#NNbelow 1000 on added lines, because the fork's numbers sit entirely inside confluentinc's range. This branch added 12 such references across javadoc, both new tests, and the inflight note; all are now owner-qualified (astubbs#177,confluentinc#833, …).astubbs#177is itself the mirror ofconfluentinc#833, so the pair reads correctly.// fixes github issue #809was left alone — a pre-existing line, not one this PR adds.The SIGPIPE bug this PR diagnosed is fixed on master (
cb8b1182) and now guarded repo-wide (7586b8c7), with the herestring fix suggested here. Two consequences: theclaude-reviewred this PR spent a pass diagnosing is gone at the root, anddocs/inflight/ci-review-agent.mdhas been rewritten from "needs its own PR" to resolved, keeping the measurements and the lesson.16ac63b1("await the metric, not a counter that leads it") does apply here, and found one real site. Details below.Applying
16ac63b1's rule to these testsThe rule — await the value you are about to assert, never a proxy that leads it — landed while this PR was open. Both directions turned out to be present in
CommitResponseTimeoutSymptomTest, which makes the distinction concrete, so it is recorded as a third instance in the existing write-up rather than a new file.Setpopulated inside the user function, then read a rejection counter incremented on the broker-poll thread. Two threads, neither ordering the other. The margin is large — feeding spans tens of commit intervals — but that is what a latent race looks like until it loses. Now awaited on the value actually asserted. Nothing is masked: if the rejections never arrive, it still fails.isClosedOrFailed()then assertinggetFailureCause()is sound becausesupervisorLoop()assignsfailureReasonbeforedoClose()setsstate=CLOSEDand before the throw that completes the future — so the awaited signal lags the asserted value. Converting it to await a non-null cause would be a real loss of strength, since a null cause is exactly what a regression here produces. Commented at the site so nobody "fixes" it.Discriminating power is unaffected: the reverting evidence below still holds, because the first await (
succeeded.hasSize(KEYS)) is unchanged and comes first.Checklist
docs/inflight/bug-177-commit-response-timeout.md(including the falsified approach),src/docs/development/upstream-map.yamlentrybug-833-commit-response-timeout(prs: [204]/status: pr-open,fork_issue: 177; dropped the now-deadforwarded:/todo:fields after master removed the tooling that read them),docs/inflight/ci-review-agent.md(rewritten as resolved),docs/solutions/test-flakiness/vacuous-await-condition-brokerpoller-backpressure-2026-07-31.md(third instance), and javadoc on each changed method.CHANGELOG.adocdeliberately untouched per AGENTS.md (generated at release time, not a per-PR chore)Relates to #177 and confluentinc/parallel-consumer#833. Same family as #100 / #108 (confluentinc#857).
🤖 Generated with Claude Code
https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA