Repository navigation
test: a backpressure test that only passed by racing its own setup - #98
Conversation
…st await, fix plan The 2026-07-31 highcpu-lane failure of brokerPollPausedWithEmptyShardsButHighInFlight (run 30603617471) is a test-design bug, not contention and not a main-code wedge: messageBufferSize=150 pins the load factor statically at 15, so with all workers latch-blocked at most 150 of the 200 unique-key records are ever taken - the first await's shards-drained==0 condition is unsatisfiable once records arrive, and green runs pass only by evaluating it vacuously before partition assignment. Plan rewrites the await to the satisfiable steady state (150 out / 50 awaiting) and records the finding; the inflight ledger entry is updated with the diagnosis and branch pointer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 3m 14s —— View job Code Review: docs(plan): BrokerPollerBackpressureTest flake diagnosed - vacuous first await, fix plan
SummaryDocs-only PR ( I cross-checked the core diagnostic claims against the current source rather than taking them at face value:
Everything I checked lines up with the code. This is a well-substantiated diagnosis, not just an assertion — the arithmetic proof (green-run duration making a non-vacuous pass "arithmetically impossible") is a nice piece of evidence and a good template for the new "vacuous await condition" flake class. Minor / non-blocking notes
No blocking issues found. The diagnosis is sound and the plan's proposed fix (await the satisfiable 150/50 steady state, rename the test, add a solutions doc) directly addresses the root cause rather than papering over it with a longer timeout. |
✅ 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). |
🧪🔒 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 |
astubbs
changed the title
…isfiable shards-drained condition Characterization-first: arrival-syncing the original awaitingSelection==0 await made it time out deterministically on a quiet box (RED, 10.0s at the exact predicted line), confirming the diagnosis - the static messageBufferSize load factor caps take-out at 150 of 200 records, so the condition only ever passed vacuously before partition assignment. Rewritten as brokerPollPausedWhenBlockedInFlightFillsBuffer: awaits the real steady state (150 out / 50 awaiting, from named constants - pinning the take-cap as a regression guard), then asserts the pause, which non-vacuously verifies upstream confluentinc#836's in-flight-counts-toward-backpressure behaviour for the first time. 30s bounds (conditions stronger, only wall-clock allowance grew). Solutions doc records the new 'vacuous await condition' flake class; changelog + README regenerated; plan units ticked; ledger entry DIAGNOSED+FIXED. Validation: RED reproducer deterministic; rewritten class green 3/3; full integration suite 110/110 (forkCount=4) + unit suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @astubbs's task in 3m 6s —— View job Code Review: fix(test): rewrite vacuous BrokerPollerBackpressureTest await
SummaryThis sync adds the actual test fix on top of the earlier docs-only diagnosis (previously reviewed). Changed files: I re-verified the core claim against source: await().atMost(30s).untilAsserted(() -> {
assertThat(pc.getWm().getNumberRecordsOutForProcessing()).isEqualTo(MESSAGE_BUFFER_SIZE); // 150
assertThat(pc.getWm().getNumberOfWorkQueuedInShardsAwaitingSelection()).isEqualTo(MESSAGE_COUNT - MESSAGE_BUFFER_SIZE); // 50
});is non-vacuous by construction: both metrics start at 0 at test start, so the assertion cannot pass before partition assignment/record arrival — unlike the old Minor / non-blocking note
No functional issues found in the test rewrite itself — the fix directly addresses the diagnosed vacuous-await bug rather than papering over it with a longer timeout, and the arithmetic backing it (150 = 15 × 10, 50 = 200 − 150) checks out against current source. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
astubbs
changed the title
…ckpressure-vacuous-await
…'s new scope rule Master's just-merged scope rule forbids root-cause narratives in the ledger - the write-up lives in the solutions doc; the entry now just points at PR #98 and instructs its own deletion on merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 54s —— View job Code Review: BrokerPollerBackpressureTest rewrite (sync)
SummaryRe-reviewed the current head (
Minor / non-blocking
No functional issues in the fix itself. The rewritten await correctly targets a satisfiable steady state derived from named constants, matches the diagnosed root cause, and the arithmetic backing it (150 = 15×10, 50 = 200−150) checks out against current source. |
|
Status of the two red advisory checks - neither is caused by this PR (gating checks are all green: Unit / Integration / Performance / PR Checklist).
Meanwhile the check most relevant to this PR - |
…folding only The rewritten test needs no separate arrival-sync await: the steady-state assertion (150 out AND 50 awaiting in one untilAsserted) is unreachable before records arrive, so it is non-vacuous by construction. Flagged by the automated review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 6s —— View job Code Review: BrokerPollerBackpressureTest rewrite (re-sync)
SummaryRe-reviewed the current head ( Re-verified the core mechanics once more against current source (all unchanged since last pass):
Non-blocking notes (already addressed / not new)
No functional issues in the fix itself, and no outstanding review comments remain unaddressed. The rewritten await correctly targets a satisfiable steady state derived from named constants, matches the diagnosed root cause, and all arithmetic backing it (150 = 15×10, 50 = 200−150) checks out against current source. |
|
Review response - all three open threads addressed and resolved, one code-review finding fixed in
No functional changes to the fix itself since the last green validation run (integration suite 110/110 locally; highcpu Integration green on the loaded box). |
The plan was created and fully executed inside this PR, so its forward-looking half (requirements trace, scope boundaries, implementation units, open questions, risks) is now inert, while its diagnosis narrative duplicated the solutions doc almost line for line. Keeps the durable artifact and folds in the one thing only the plan carried: the classification verdict - what this was NOT (not the tight-timeout contention pattern, not a main-code bug, not already tracked) - which is the part most likely to save a future reader from the wrong fix. Repoints the two inbound references: the test javadoc and the inflight ledger now cite the solutions doc, which is the artifact that still makes sense in a year. README regeneration also picks up #86's ambient-probe changelog entry from the master merge. Test re-verified green after the javadoc repoint (2/2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ckpressure-vacuous-await # Conflicts: # README.adoc
|
Claude finished @astubbs's task in 2m 20s —— View job Code Review: BrokerPollerBackpressureTest rewrite (re-sync)
SummaryRe-checked at the current head ( Since
Re-verified the fix itself once more against source:
All threads from the four prior review rounds on this PR (duplicate-code clones, the stale plan-doc note, the quarantine-lane thread) were already resolved by the author with sound reasoning, and nothing in this sync reopens them. No functional issues found. The fix directly addresses the diagnosed vacuous-await root cause, the arithmetic backing it still checks out against current source after the master merge, and no new code was introduced by this sync — it's a clean rebase. |
…ckpressure-vacuous-await
|
Claude finished @astubbs's task in 1m 40s —— View job Task list
Code Review: BrokerPollerBackpressureTest rewrite (re-sync at
|
BrokerPollerBackpressureTest, RetriesTest and DrainCloseTest each opened with the same 18 lines - consumer/pcOpts/pc fields, then setupTopic() -> createNewConsumer(NEW_GROUP) -> options builder -> new ParallelEoSStreamProcessor -> subscribe - differing only in which options the test actually cares about. The clone detector had been flagging the pair it could see on every push of #98. BrokerIntegrationTest.startPcOnNewTopic(options) now does it once, taking the builder pre-seeded with the consumer, so each setUp keeps only its distinctive line and the interesting configuration is no longer buried in boilerplate. Its javadoc is written to be found: it opens with the usage snippet and points at the sibling helpers - produceMessages, getTopic, getKcu, setupTopic, ensureTopic - so the next test author reuses the toolkit rather than re-deriving it, and gets the setup right by default. It returns the processor rather than assigning inherited fields on purpose. Five other subclasses (TransactionTimeoutsTest, TransactionMarkersTest, RebalanceEoSDeadlockTest, RebalanceTest, PartitionOrderProcessingTest) already declare their own consumer/pc fields, which base-class fields of the same name would silently shadow - legal, and exactly the sort of trap this refactor is meant to remove. The consumer and built options were scaffolding in all three classes anyway, used nowhere outside setUp. Additive to the base: no lifecycle change, no new @beforeeach, so the other 18 integration tests are untouched. Verified: the three refactored classes green (4 tests), plus the full integration suite.
|
Claude finished @astubbs's task in 2m 19s —— View job Code Review: BrokerPollerBackpressureTest rewrite (re-sync at
|
|
Review response ( Runtime confirmation the sandbox couldn't get. The review noted it could verify the refactor only by inspection, having no Docker for the Testcontainers-backed tests. Run locally since: the three refactored classes green (4 tests), then the full integration suite green ( The independent check on field shadowing is the useful one. The review verified the rationale rather than taking it from the commit message, finding Minor note, deliberately left as-is. The javadoc usage snippet ( On the duplicate-code thread: the review predicted it would stop recurring, and it already has - both duplication engines ran on this push and neither raised the inline thread, with PMD CPD showing clones 33 → 29 and duplicated lines 1001 → 874. |
astubbs
changed the title
…double copyright headers; ledger cleanup - Remove the concede optimizer that rode in via the pre-session ci/grumpy-runner-workflow merge: maven.yml restored to master (byte-identical), bin/ci-concede-check.sh deleted. It was removed from PR #75 by a 10-reviewer ce-review as a P0 gate-spoof (a PR could add a workflow named "highcpu" with a trivially-passing job and make the REQUIRED gate skip real tests) - inflight.md documents why it stays discarded. It should never have been in this PR. - Collapse the doubled Confluent copyright header (a maven-license-plugin confluentinc#888 artifact) in the three files that carry it: BrokerIntegrationTest, KafkaClientUtils (both touched by this PR) and RetriesTest. Checker still 0 violations across 232 files. - Ledger: delete the #98 entry (self-marked, #98 merged as 6d39081); shrink #80s own now-solved committedOffsetRemoved/drain narrative to a pointer + residual open tracking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018igSVt74wPQAbRNHkXS4R8
…s stop colliding (#112) The ledger had reached 677 lines and become a record of finished work. Entries told to "delete when #98 merges" outlived three merges; the rebalance-commit fix was still filed as awaiting PR after landing as #100; a static-state flake was still listed as deliberately-unfixed after #101 fixed it at source. Meanwhile #80, carrying the drain-zombie fix and the largest thing actually in flight, appeared only as a footnote about quarantine ownership. The file's own scope rule says entries vanish when work lands; it had stopped being applied. Every surviving claim is now checked against GitHub, git and the code. Obsolete entries are deleted rather than annotated: the jscpd cap is above baseline, no workflow has path filters so the docs-only inconsistency cannot happen, the ManagedPCInstance header carries its Modifications line, the CommitFailedException follow-up shipped in #108, and the stacked-PR gap is closed by a new all-branches ruleset. Upstream references now follow the convention the changelog already uses: fork numbering reaches #111, so every bare reference at or above #162 was silently upstream and read as ours. With the content correct, the file became a directory. It appeared in 26 of the last 30 master commits, so unrelated PRs conflicted on it constantly - not because they disagreed, but because their notes were adjacent, and the merge that resolves such a conflict silently drops one side (it happened between #108 and #110). Each item is now its own file, named <category>-<slug>.md; two PRs recording unrelated work touch disjoint files and cannot conflict, and finishing work is `git rm`, which never conflicts with an edit elsewhere. The prefix is the structure - `ls docs/inflight/` shows the shape of what is open without reading anything. There is deliberately no committed index: it would be edited by every PR, which is the problem the directory solves, and docs/TODO_INDEX.md is the cautionary case. This was parked on docs/inflight-as-directory with "migrating ~600 lines of existing entries" named as the reason not to do it. The audit is what made it cheap. docs/refactoring.md stays a single file - 2 of the last 30 commits touched it, so it has none of this problem. The manifest had drifted the same way, which matters more, because it is the declared source of truth that a future session trusts instead of re-deriving. Five entries disagreed with reality: bug-857 recorded no fork PR while #29 was open, #100 merged and #80 in review; fix-909 recorded no PR though #31 is open; bug-912 was in-progress when the schema's word for pushed-but-unPR'd is ready. Nothing catches this - upstream-map.py validate only checks the schema and upstream-sweep.sh only watches upstream, so "prs: []" beside an open PR passes every check we have. Hence the AGENTS.md rule to update it at every lifecycle transition, not only when starting. Four rules now live in docs/inflight/AGENTS.md so they are inherited rather than rediscovered: delete an entry in the PR that resolves it and never leave a "delete when #NN merges" marker on master; never record what gh or git can answer, which is why the open-PR table is gone; known code defects belong here even when an issue exists, because an agent scans this directory and will not read the tracker; and new guidance about how these notes are written goes into that file too. Four deferred code items moved to docs/refactoring.md, where deferred work lives: the SpotBugs thread-visibility findings, the produce-lock double release surfaced by #110, the commitAndWait message that misstates its own wait, and jacoco's single exec file under forked surefire. Also shares the agent-tooling gitignore rules that until now existed only in one checkout's .git/info/exclude, and deletes an empty duplicate-code-cross-check clone - a git init that never fetched - rather than ignoring it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewing the new solution doc surfaced a hole in the checklist that same PR had just added: it said to check open PRs, which is a collision check. The PR that already solved something in your file is by definition merged, so searching only the open list finds nothing and feels like diligence - worse than not looking, because it produces false confidence. Same for issues: the useful ones are usually closed. Searching merged PRs BY TOUCHED FILE - not by keywords in their titles - finds, for this investigation: - #110, the sibling flake on this same producerTransactionLock, fixed four days earlier and the source of the control-arm method - #86, which introduced the ambient probe whose "probe clean" verdict this work had to qualify - #98, the backpressure test that only passed by racing its own setup - the sibling rule this doc deliberately does not merge with None would surface from a symptom search or from the open list. All three are now cited in the solution doc, with #80, #115 and #68 from the same family, and the confluentinc#803 / confluentinc#809 / confluentinc#833 cluster upstream. Adds to the doc what the prior-art search actually covered, including what it did NOT (the highcpu and quarantine lanes), so the next reader knows where the gaps are instead of assuming there are none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…issue-ref gate TWO THINGS, BOTH FOUND BY VERIFYING RATHER THAN ASSERTING. 1. ReactorPCTest.concurrencyTest failed locally at "expected < 1200 but was 1266" while the full Maven reactor ran alongside two Docker image builds on a 12-core box. Diagnosed as contention WITH A CONTROL ARM, not by assumption, and recorded because no entry for it existed anywhere: not the inflight ledger, the quarantine registry, docs/solutions, docs/plans or the hardening audits. branch, uncontended: 5 pass / 0 fail 1000, 1073, 1000, 1062, 1000 master, uncontended: 5 pass / 0 fail 1000, 1000, 1076, 1000, 1000 Indistinguishable, both well under the threshold, run SEQUENTIALLY because running the arms together would reintroduce the variable under test. The control was warranted rather than skipped, and the note says why: this branch's stack modifies ShardManager, WorkManager, ProcessingShard and WorkContainer - the classes that bound concurrency - adding the language proxy's abandonment path. There is even a route from that change to this exact symptom: onAbandonedResult decrements numberRecordsOutForProcessing, and a DOUBLE decrement would under-count in-flight work, let extra records out, and read as concurrency exceeding its maximum. The author already guarded it, with an early return placed before an unconditional decrement. It is also unreachable from this test - nothing in core or reactor calls the abandon path - which is the half the master baseline confirms empirically. The note says what not to do: do not raise MAX_CONCURRENCY_OVERFLOW_ALLOWANCE and do not quarantine. Quarantine needs a diagnosis and is master-state; this fails on neither master nor an idle box. Loosening the allowance would delete the only signal that the bound is real, in the library whose whole purpose is bounded concurrency. 2. The gate was RED on a commit already pushed, and I had not re-run it after writing that file. It flagged five "unqualified issue references" that are JVM thread ids in a quoted jstack (#98, #99, #118, #119, #188). Wrapped in the gate's own documented exempt-begin/exempt-end markers for quoted source material - which is what that mechanism exists for, not a way around a red check. Gate now exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcdtMdmdhyYug3krZRWkZT
1 participant
What
Diagnoses AND fixes the
BrokerPollerBackpressureTest.brokerPollPausedWithEmptyShardsButHighInFlightfailure from the highcpu lane (run 30603617471), closing out the "diagnose, don't dismiss" inflight ledger entry. The test is rewritten asbrokerPollPausedWhenBlockedInFlightFillsBuffer- its first await could never be satisfied once records arrived, and only ever passed by evaluating vacuously before partition assignment.Fix verification (characterization-first): arrival-syncing the ORIGINAL await (3-line change) made it time out deterministically on a quiet box with zero load - RED at exactly the predicted line, 10.0s timeout - empirically confirming the diagnosis before the rewrite. The rewritten test awaits the real steady state (150 out-for-processing / 50 awaiting-selection, derived from named constants), which pins the static-buffer take-cap as a regression guard and verifies upstream confluentinc#836's in-flight-counts-toward-backpressure behaviour non-vacuously for the first time. Validation: rewritten class green across repeated runs; the three refactored classes green; full integration suite 110/110 (forkCount=4) + unit suites green locally, re-run after merging master so the result includes #100's broker-poll / rebalance-commit change.
Verdict: test-design bug (vacuous await) - not contention, not a main-code wedge, not a duplicate
messageBufferSize(150)pins the load factor statically at 15 (PCModule.initDynamicLoadFactor), so with all 10 workers latch-blocked, PC takes at most 10x15 = 150 of the test's 200 unique-key records.getNumberOfWorkQueuedInShardsAwaitingSelection()floors at 50 - the first await's== 0condition is unsatisfiable once records arrive. No timeout increase can fix it.docs/solutions/test-flakiness/parallel-integration-tests-flaky-under-concurrency-2026-07-28.md) - loosening bounds would mask a broken condition. No fork/upstream issue tracks it; not quarantined. Test inherited from upstream Make PC message buffer size configurable confluentinc/parallel-consumer#682/Fix buffer size include in flight messages in backpressure calculation logic confluentinc/parallel-consumer#836.Changes
BrokerPollerBackpressureTest.java- test rewritten: steady-state awaits from named constants, arrival-race-free, javadoc explaining why "paused with empty shards" is unreachable by design (take-cap == pause threshold), 30s bounds (conditions got STRONGER; only the wall-clock allowance grew). Sibling test untouched.docs/solutions/test-flakiness/vacuous-await-condition-brokerpoller-backpressure-2026-07-31.md- the single durable artifact for this fix: mechanism, the same-sha green/red arithmetic-impossibility proof technique, the classification verdict (what this was NOT), and the prevention rule ("awaitX == 0where X starts at 0 and only later becomes non-zero is a red flag"). The working plan doc was collapsed into this one - it was created and completed inside this PR, so its forward-looking half was inert and its diagnosis narrative duplicated this file.docs/inflight.md- ledger entry: DIAGNOSED + FIXED, with mechanism, verdicts, and pointers here.CHANGELOG.adoc+ regeneratedREADME.adoc- Unreleased entry for the fix.BrokerIntegrationTest.java(+RetriesTest.java,DrainCloseTest.java) - one way to stand up a PC integration test, instead of three copies. All three opened with the same 18 lines (consumer/pcOpts/pc fields, thensetupTopic()→createNewConsumer(NEW_GROUP)→ options builder →new ParallelEoSStreamProcessor→subscribe), varying only in the options each test cares about - the clone the duplication bot kept flagging on every push here.startPcOnNewTopic(options)now does it once, taking the builder pre-seeded with the consumer, so eachsetUpkeeps only its distinctive line. Its javadoc is written to be found - usage snippet first, then pointers to the sibling helpers (produceMessages,getTopic,getKcu,setupTopic,ensureTopic) - so the next test author reuses the toolkit instead of re-deriving the setup and getting it subtly wrong.TransactionTimeoutsTest,TransactionMarkersTest,RebalanceEoSDeadlockTest,RebalanceTest,PartitionOrderProcessingTest) already declare their ownconsumer/pcfields, which base-class fields of the same name would have silently shadowed - the exact trap the refactor exists to remove. Additive to the base (no lifecycle change), so the other 18 integration tests are untouched.🤖 Generated with Claude Code
Checklist
CHANGELOG.adoc, under== Unreleased; README regenerated from it)