Repository navigation
test(core): the back-pressure test asserted an offset it had frozen - #351
Merged
Merged
Conversation
OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing was
the fork's most frequent tracked flake (4/45) and was quarantined UNDIAGNOSED as an explicit rule-1
exception because it blocked every PR. It is a test-design defect, not a product defect, and it is
now diagnosed, fixed and back in the gating lane.
The mechanism, readable in the source before anything was run. The encoded high-water mark is the
highest SUCCEEDED offset - PartitionState carries the comment `use offsetHighestSucceeded instead of
offsetHighestSeen`, and encodeOffsetsCompressed takes its range top from getOffsetHighestSucceeded().
Offset-encoding back pressure does not pause the poller (that is the separate load gate,
wm.shouldThrottle()); it gates couldBeTakenAsWork, which refuses every record at or above the highest
succeeded offset. So the instant the partition blocks, the succeeded frontier FREEZES at the top of
the last batch already claimed as work, and the test's expectedHighestSeen - the last offset it sent
- becomes unreachable rather than late. It passed 41 times in 45 only because the mock consumer
usually hands the whole extra batch over in one poll, so every extra record was already in flight
when the block fired.
Measured, not inferred. A deterministic probe replaying the test's exact configuration with no
threads at all - prime 100, hold {0,2}, add 40, forcedCodec BitSetV2, DefaultMaxMetadataSize 40 so
the pressure threshold is 30 - takes and succeeds one record at a time. The payload is 28 chars up to
offset 119, 29 to 127, 30 to 135, and 31 at 136, where the partition blocks and offsets 137-139 are
refused forever. 136 is exactly the actual CI reported. The real test was then made to fail
deterministically by splitting the extra send 37 + 3 with the block confirmed between them, producing
"expected: 139 but was : 136 within 30 seconds" - character-for-character the CI signature.
Control arm on the reason, not just the symptom: move the claim boundary and the threshold and the
frozen frontier moves with them - 136, 124, 129, 136 across four configurations, never 139. That is
the recorded falsification path's first arm firing. Its second arm ("it is just slow") is dead: the
probe reaches the frozen state with no time pressure at all.
The fix, test-side and in three parts. The exact constant 139 moves to where it IS true -
wm.getPm().getHighestSeenOffset(topicPartition), which registration raises and back pressure does not
gate; that check previously read isGreaterThan(numberOfRecordsToPrimeWith), so this is a
strengthening, not a relaxation. In-flight is then awaited down to the two records the test itself
holds, which is a genuinely quiescent state once blocked, so getOffsetHighestSucceeded() is read as a
still value rather than a moving one. Finally the committed payload's high-water mark is asserted
equal to that settled frontier, preserving what the section is about - "assert blocked, but can still
write payload" - without demanding an offset the awaited event exists to prevent.
Explicitly ruled out, so the next reader does not re-derive them: the torn-read family
(#344 widens the encoded range, pushing the decoded value UP, and this is a
shortfall; #337 cannot shift a read that passes base 0 explicitly); the
retry-delay sleep (the earlier, reverted diagnosis - that code runs after the failing assertion); and
slowness. Sweep for other instances of the defect class - a test asserting records SENT against a
value derived from records SUCCEEDED where a mechanism stops the frontier rising - found NONE.
OffsetEncodingBackPressureUnitTest carries the same expression but asserts getOffsetHighestSeen() and
drives every completion synchronously before encoding, which is why it never flaked.
Verified: original assertion plus forced split send RED; fixed assertion plus the same split GREEN;
stock test GREEN 5/5; four control-arm configurations GREEN 4/4; whole core unit suite 450 tests,
0 failures.
Registry rule 3 satisfied - the @Quarantined annotation and the docs/quarantined-tests.md entry are
deleted in this commit, restoring the test to the gating lane. This also clears one of the 0.6.0.0
release blockers (registry rule 5). Diagnosis history and the two ledger lessons behind it are in
docs/solutions/test-flakiness/back-pressure-freezes-the-frontier-the-test-asserted-2026-08-24.md; the
ledger entry in docs/inflight/test-untracked-ci-flakes.md is removed, per that directory's rule that
closed items are deleted rather than rewritten as done.
Also qualifies two bare issue references in this branch's own
sneaky-thrown-checked-exceptions-defeat-spotbugs-dataflow.md, which the pre-commit issue-reference
gate was failing on.
Release-Note: none - test-only change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
4 tasks done
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🧪🔒 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 |
…nd cite the write-up rather than retell it Simplification pass over the previous commit's assertions - no behaviour change, the test is green 5/5 either way. Three findings from the reuse and quality reviewers: Reuse. `wm.getPm().getHighestSeenOffset(topicPartition)` is a one-line delegate to `getPartitionState(tp).getOffsetHighestSeen()`, and the `partitionState` local is already in scope two lines above (it is what the adjacent `isBlocked()` assertion reads). Both new assertions now go through the generated `ManagedTruth` subjects - `assertWithMessage(...).that(partitionState) .getOffsetHighestSeen()` and `.that(wm).getNumberRecordsOutForProcessing()` - which is the idiom the sibling `OffsetEncodingBackPressureUnitTest` and `OffsetEncodingTests` already use, matches the repo's Google-Truth convention, and names the getter in the failure message rather than printing a bare number. Quality. The mechanism comment retold the diagnosis that docs/solutions/test-flakiness/back-pressure-freezes-the-frontier-the-test-asserted-2026-08-24.md already owns, against AGENTS.md's "cite incidents, never retell them". It keeps the invariant a reader needs at the callsite - encoded high-water mark is the highest SUCCEEDED offset, back pressure freezes it, so the last polled offset is unreachable rather than late - and cites the write-up for the measurements, the control arms and the CI history. The hard-coded "measured at offset 136" figure goes with it: it is configuration-dependent (this tree encodes 32 chars, not the 31 the probe recorded), the ledger also records a 132, and a number in a comment nothing checks is exactly the citation AGENTS.md warns goes stale silently. Verified: OffsetEncodingBackPressureTest green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sumed - and the ledger row that contradicted itself Review pass over the two commits below. One real defect fixed, one proposed fix REFUTED by measurement rather than applied, and the gap it was aimed at recorded where it can actually be closed. Removes the `PCMetricsTest.metricsRegisterBinding` row this branch added to docs/inflight/test-untracked-ci-flakes.md. Three independent reviewers flagged it and it is wrong three ways: it claims `owner #265` while docs/quarantined-tests.md's entry for the same test reads "No Owner yet"; the header rewritten in the same commit says #265 is "fixed and out of this ledger", naming a different, closed item; and its "- see below" promises a section the file does not contain. The test is already tracked by that registry entry and by docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md, so the row also duplicated a fact this directory's rules say must live in exactly one place. It was absent at the diff base - nothing outside this branch depended on it. The loose lower bound on the settled frontier STAYS, and this is the substantive result. Two reviewers proposed tightening `isGreaterThan(numberOfRecordsToPrimeWith - 1)` so that back pressure engaging early would go red - one wanted a numeric floor at the measured block point (136), the other wanted the committed payload asserted over the pressure threshold. Both were implemented and mutation-tested against the guard they exist to protect, and BOTH SURVIVE. Mutating `updateBlockFromEncodingResult` from `metaPayloadLength > getPressureThresholdValue()` to `... - 2` leaves the test green, logging the identical `Payload size 32 higher than threshold 30.0` as the control - because the mock consumer hands the whole extra batch over in one poll, so `offsetHighestSucceeded` reaches 139 before the first encode that follows the batch. The settled frontier is decided by the claim boundary, not by where the threshold sits, so no assertion on it can see the threshold move. The stronger `... - 10` mutant IS killed, but by the pre-existing `isAllowedMoreRecords()` assertion above the section, not by anything a tightened bound would add. Adding an assertion that provably kills no mutant another assertion does not already kill is the thing docs/testing.md's method exists to prevent, so neither proposal is in this commit. What is left is the bound restated as the band it actually describes - `isBetween(numberOfRecordsToPrimeWith, lastOffsetSent)` - which drops an awkward cast and says "into the extra batch and no further", at identical strength. The gap is real even though this test cannot close it, so it is recorded rather than lost: docs/inflight/test-back-pressure-engage-point-is-unasserted.md carries the mutation result, why neither this test nor OffsetEncodingBackPressureUnitTest can host the coverage, and the fact that the deterministic single-threaded probe built for the diagnosis - the one instrument that controls the claim boundary - is what would make a premature-engage regression red. The related blind spot rides with it: nothing pins that getOffsetHighestSucceeded() and getOffsetHighestSeen() diverge while blocked, so a regression collapsing the two accessors is invisible to the round-trip assertion. Also corrects a claim in the preceding commit's own body: it says "this tree encodes 32 chars, not the 31 the probe recorded", which reads as a contradiction and is not one. 31 chars is the payload at frontier 136, where the probe steps one offset at a time; 32 is the payload at frontier 139, which is what a real run reaches. Both are true and they measure different states. Ruled out and not changed: the bare `await()` on quiescence uses Awaitility's 10s default while its sibling uses the 30s defaultTimeout. Left alone - bare await() is this file's existing convention, the records being drained only sleepQuietly(1), and a miss is a ConditionTimeout red rather than a silent green. Two reviewers reached that independently. Verified: OffsetEncodingBackPressureTest green 5/5; core unit suite 389 tests, 0 failures, 8 skipped; check-quarantine-registry.sh, check-issue-refs.sh, check-copyright-headers.sh, check-file-refs.sh and check-inflight-tags.sh all green. Release-Note: none - test-only change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 7m 31s —— View job Review
SummaryThis is a solid, well-diagnosed fix. The core claim — that the encoded high-water mark tracks Independently verified:
Minor, non-blocking observations:
Nothing here blocks merge. The diagnosis is measured (deterministic probe reproducing the exact CI signature |
Owner
Author
Followed up per AGENTS.md's "remove duplication introduced by this PR" rule. Nothing to remove — no duplication was introduced.
+0.3 on a token-similarity metric with no new clone in either engine is drift from the changed assertions naming the same identifiers, not a clone this PR added. |
…his merges `check-branch-self-reference.sh` flagged both mentions of the PR in `test-back-pressure-engage-point-is-unasserted.md`. Neither needed rewording - both are past-tense references to the change the note came out of, so they already read correctly once it has landed - but the gate exists precisely because "it reads fine" is a judgement nothing else records, so it demands the confirmation be explicit rather than assumed. Marked the intro with a checked-begin/checked-end block and the coverage paragraph with a single checked marker, and said in the text why the tense is deliberate, so a later reader does not "fix" it back into present tense.
…lay nobody has run Chaos Pain Suite went red on this branch with the drain arm's now-familiar CLASS2_STALL/LAG_STAGNATION signature - 27 violations, 154s of stagnation against the 150s bound, group STABLE throughout. Seed 2445946824654755330, recorded because the artifact expires and every prior entry argues the seed is the only part no command can recover. The branch is a control, not a suspect: its single code file is a surefire unit test the chaos lane does not run, and everything else is markdown. Two things make it worth more than a tally mark. The drain arm has now drawn this signature on four separate seeds while the experiment that would say what it means - one replay with -Dchaos.diagnoseStallRecovery=true - has still never been run; this is the third consecutive drain-arm entry written with that sentence true, so the ledger is accumulating sample size against a question sample size cannot answer. And it offers a discriminator the thirteenth sighting could not: Performance and Chaos overlapped in time but reported different runner names, highcpu-4 and highcpu-2. That entry raised contention because the two shared a box. Whether these are distinct hosts is not established anywhere in the docs, and settling it decides whether contention can be invoked for any overlapping highcpu pair or only for same-runner ones. Retrieval was the documented trap again - no verdict in the console, a bare "exit code 1" annotation, every number from the uploaded failsafe artifact.
astubbs
marked this pull request as draft
August 25, 2026 02:49
…ose-only diff The head that added the fourteenth sighting to this ledger went red on the same arm with the same signature and a different seed - 19 CLASS2_STALL, 153s against the 150s bound, seed 8791285396374198974. The diff between the two red heads is one markdown file. The twelfth sighting recorded the mirror of this: two adjacent commits, prose-only diff, red then green, read as proof the signature is drawn per seed and not per tree. This pair is red then red. Both readings hold at once - the tree still explains nothing, and the drain arm went two for two on this box tonight. Recorded as an addendum rather than a fifteenth sighting because it is the same branch and the adjacent head; the pair is the observation, not either half. Six drain-arm seeds now sit in this ledger across three sightings with the one replay that would interpret them still unrun. Said so explicitly, because the failure mode here is adding a seventh instead.
…rom a poll thread that had not died The third chaos red on this branch is a different arm and a different failure from the two above it, which is only visible because the signature was checked rather than assumed. ChaosRevokeUnderWorkIT failed its "no instance may end the run with an unclassified failure cause" assertion - a correctness assertion, not the CLASS2_STALL timing bound - on a commit request that timed out after 10s from a broker-poll thread the runtime had positively established was alive and not throwing. One ZOMBIE_MEMBER/REBALANCE_BLOCKED alongside it, 15s dwell. Seed 8584935079849032188. This matters because everything else recorded tonight is the timing bound that the probe's own message disclaims as unable to tell a wedge from slowness. This one is not that: the conclusion "the thread is blocked" is reached by elimination in the runtime, not inferred from a stagnant watermark. The entry deliberately stops short of identifying it as the open deadlock. A poll thread not answering while a rebalance goes unanswered is the shape synchronized(commitCommand) contention would present from outside, but the same symptom comes from a poll thread merely slower than offsetCommitTimeout, and nothing captured here separates them. Recorded the step that would: replay the seed and take a thread dump of the poll thread at the timeout.
… is unclassified, not evidence test-chaos-class2-red-was-runner-contention.md is on master, owns exactly this question, and was not consulted before the entry was written. Read against it, neither the fourteenth sighting nor its addendum is family evidence. Its finding is that ~154s CLASS2_STALL is a signature and not a diagnosis - roughly what a 150s bound produces once crossed, whatever crossed it - so the 154s and 153s figures recorded above discriminate nothing, and treating four drain-arm seeds as accumulating evidence was the error. The contention precondition that note measures was also present and unchecked: Performance and Chaos Pain Suite overlapped in both runs, the same pairing that inflated one seed from 121.3s to 154.5s there. Neither seed above has been replayed uncontended, which is the only discriminator, so both entries are unclassified rather than weak. Corrected beside the originals rather than over them, per the ledger's own practice with the superseded seventh sighting. What survives is narrower and worth keeping: both runs reported DIFFERENT runner names while overlapping, where that note's measured case had the jobs sharing a box. That is still worth settling, and the fifteenth sighting is untouched - it failed a correctness assertion, not the timing bound this note governs.
…rozen-frontier # Conflicts: # docs/inflight/bug-857-family.md
astubbs
marked this pull request as ready for review
August 26, 2026 01:25
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
365 is the fix for something this branch hit twice today: a gate was truncating the shared clone's history on every sweep, so `git merge-base` answered exit 1 where it had answered a commit an hour earlier, and half the gates would have judged a truncated graft while reporting green. Taking it here stops that recurring mid-merge. 366 brings the hosted-chaos trial and, with it, #351 - which diagnosed `OffsetEncodingBackPressureTest` (the 4/45 flake, the most frequent tracked one) as a test that froze the offset it then asserted. FOUR CONFLICTS, and the two that needed a decision: - docs/quarantined-tests.md - 366 removes the back-pressure entry because #351 released it. Taken. But the PCMetricsTest entry that stays cited it as "the entry above" for the shared shape of their failures, so that cross-reference is repaired rather than left pointing at nothing: the rhyme is now stated in the past tense and names #351 as the first place to look for whether the two are one phenomenon or two. Registry re-verified, 2 entries. - docs/inflight/test-untracked-ci-flakes.md - 366 retires the whole "NOT diagnosed - quarantined anyway" section. Taken whole rather than re-adding a section its author deliberately removed, after confirming the durable part - why quarantine rule 1 changed - survives in the solutions write-up that replaced it. - docs/ci.md and maven.yml - both sides edited the same paragraph and the same matrix. Kept both edits: the proto-breaking gate this branch gained from master stays listed beside 366's hosted-chaos job, and `continue-on-error` now honours both `advisory` (the execution-mode axis) and `optional` (366's experiment). Two names for one meaning, deliberately not unified - renaming either would edit the other branch's vocabulary and re-conflict when it lands. Also fixes two SpotBugs findings the merge surfaced in SessionEndTest: an ignored `CountDownLatch.await(timeout)` return in each of two user functions. A discarded await cannot be told from a call whose result nobody needs, and false there means the release never came inside the budget. Named per the repo convention rather than thrown from, since throwing on that thread would fail the record for a reason the test is not measuring. Verified on the merged tree: full reactor test-compile green, bin/check-all.sh 18/18 with only check-shell-lint CANNOT (shellcheck is not installed on this box). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HwDcZdfv1Ne4u3g6ThrG9
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…-reference gate this branch must now satisfy Master moved 55 commits under this branch since `0e21b903d`. What matters to a PR about quarantine evidence and a broker-level confluentinc#909 reproduction: - #351 (`test(core): the back-pressure test asserted an offset it had frozen`) diagnosed `OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessages...` as a test-design defect and removed its registry entry. **The registry is not empty** - `PCMetricsTest.metricsRegisterBinding` and `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` remain, the second owned by #57/#262 - so this PR's `Quarantined` rewording (evidence rather than a root cause) lands against two live entries, both of which already carry a diagnosis and so satisfy the stricter half of the rule either way. `bin/check-quarantine-registry.sh` confirms: 2 entries, consistent. - #347 (`test(core): a Lincheck lane`) edited `QuarantinedAnnotationContractTest`, the same file this PR rewords. Auto-merged cleanly; both edits are present, and the merged file's blank-attribute scan now reads "no quarantine without evidence". - **A self-reference gate arrived that this branch is not yet clean against**: `bin/check-branch-self-reference.sh` (`da91f3f61`) fails a `docs/inflight/` note that mentions THIS branch or THIS PR without an attestation that it reads correctly after the merge. It reports 17 findings here. The next commit settles them; they are not folded in, so the merge stays reviewable as a merge. - `docs/inflight/AGENTS.md` gained the issue-index carve-out (`docs/inflight/issue-index.md`) beside this PR's four-outcome retirement rule. Auto-merged, no overlap. - `docs/refactoring.md` gained entries from master's static-analysis work beside this PR's three. Auto-merged, no overlap. ONE CONFLICT, resolved as the union. `AGENTS.md`, the topic-doc routing table. Both sides edited the `docs/inflight/AGENTS.md` row's neighbourhood: this branch reworded that row (retiring a note is four outcomes, not just deletion) and master added a new row routing `parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.md` directly beneath it. Neither edit contradicts the other - one rewords a description, the other adds a destination - so both are kept, in master's ordering. Taking either side alone would have silently dropped a live route or reverted a rule this PR argues for. VERIFIED LOCALLY rather than deferred to CI: `bin/ci-unit-test.sh` on JDK 17 is BUILD SUCCESS over all eleven modules (core 411 tests, 0 failures, 8 skipped), and the two files this PR adds under `src/test-integration/` compile - `build-helper` adds that root at `generate-test-sources`, so `test` covers them. No test was loosened, no assertion weakened and no quarantine entry added to reconcile the two sides.
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…am-memory-leak Master gained 144 commits since cae88c7. Four textual conflicts, and one semantic conflict that auto-merged clean and would have shipped wrong. THE SILENT ONE: master's #120 (de1620d) rewrote `PCMetricsTest.metricsRegisterBinding` wholesale and un-quarantined it. Git kept master's rewritten test body and this branch's `@Quarantined` annotation on top of it, so the merge compiled and the test stayed dark. The annotation is removed: master fixed exactly the defect this branch's quarantine reason described. This branch said the failure was "the metric behind and never converging... whether the un-committed tail is a wrong test assumption or real commit behaviour is the open task"; master answered it - workers awaited a latch BEFORE incrementing, so a latched worker's offset never completed and the gap was permanent, not slow. The write-up is `docs/solutions/test-issues/metrics-test-compared-a-commit-offset-to-a-completion-counter-2026-08-20.md`. That retires this branch's eb602ae and 16a456e in full - the re-quarantine and the cross-branch sighting that justified it. `PCMetricsTest.java` is now identical to master's. `docs/quarantined-tests.md`, `docs/inflight/test-untracked-ci-flakes.md`: master's side taken. It drops both of this branch's entries - `metricsRegisterBinding` above, and `OffsetEncodingBackPressureTest.backPressureShouldPrevent...`, which master diagnosed and fixed in #351/#336 (`back-pressure-freezes-the-frontier-the- test-asserted-2026-08-24.md`). Both quarantines were honest when written and are now superseded by fixes, not by wishing. `docs/inflight/next-candidates.md`: master's #323 renamed it to `process-candidate-ranking.md`, so the delete wins and this branch's one edit - dropping the `confluentinc#912` ready-pick, since it is this PR - is re-applied to the renamed file. A modify/delete conflict hides that kind of rename: taking either side alone loses the other's work silently. `docs/inflight/branch-912-vertx-leak.md`: deleted, as this branch did in 2dc76c9 - it is this PR's own pre-PR ledger entry. Master only touched it to add priority metadata. Its two inbound citations are repaired here rather than left dangling: the ready-pick above, and the naming example in `docs/inflight/AGENTS.md`, which now cites only `bug-857-family.md` for a confluentinc-numbered note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…mode-battle-test #262's declared parent had moved a long way: #257 has since merged master up to #403, so taking it also takes nearly all of master, and 262's own CONFLICTING state against master goes with it. 846 files, nine conflicts. Package rename: both sides were already on `bz.stub.*`, so the merge was ordinary and `bin/rename-packages.sh` was not needed. Verified by `git ls-tree -d` on all three refs before starting, not assumed. Conflicts, and which side won: - ProducerManager - TAKE 257. Master has already absorbed 262's produce-callback fix and improved it: the callback is hoisted to a `sendCallback` field with the reasoning in javadoc rather than a comment, and it carries the fact 262 established last (Kafka's own ProducerBatch catches and logs whatever a callback throws, so the throw was only ever observable on the synchronous doSend path). 262's local copy is deleted as the duplicate it now is. `InternalRuntimeException` is `PCInternalRuntimeException` on master; the three places 262 still named the old class are renamed. - AbstractParallelEoSStreamProcessor#cleanUpContext - BOTH. Code takes 262's catch-and-log, which master does not have: this runs in runUserFunction's `finally`, and a throw from a finally REPLACES the exception the catch above is propagating, destroying the user function's real failure. Javadoc takes 262's correction (an ExternalEngine can never reach here holding a produce lock - its constructor rejects PERIODIC_TRANSACTIONAL_PRODUCER, so 257's claim that this is that path's only release is wrong) plus 257's new paragraph on the lock being taken rather than read. - KafkaClientUtils#createNewProducer - BOTH, and neither could just win. 262 added a typed (mode, transactionTimeout, stableTransactionalId) overload; master added (mode, Properties overrides). The auto-merged body already used all three parameters, so both overloads now delegate to one private four-arg builder. - TransactionalPartialResultSetIT - add/add, TAKE 257 plus one thing. 257's is the reviewed descendant of the same test: it pins MAX_REQUEST_SIZE_CONFIG rather than inheriting it, holds POISON_KEY to NONE rather than all-or-none (its oversized result can never be sent, so isAnyOf(0, n) would also accept a full set that cannot physically exist), and uses commons-lang3 `repeat` instead of a local Java-8 helper. Only 262's `@Timeout(600)` guard is carried over. - ProducerManagerTest - TAKE 262's extracted `acquireProduceLockInto` / `assertProduceLockStillOwnedByContext` helpers over 257's inline copies of the same code and comments; the fact 257's comment had and the helper javadoc did not (#257 made cleanUpContext the ONLY release point) is folded into the javadoc. - ProduceLockReleaseTest, WorkContainer - imports only. - docs/quarantined-tests.md is now EMPTY, by two independent routes meeting. #351 diagnosed and fixed OffsetEncodingBackPressureTest on master (it asserted an offset back pressure exists to stop advancing), and this branch performs its own rule-3 re-enable of ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect. No @Quarantined annotation remains anywhere in the tree and the registry check confirms 0 entries. - docs/inflight/test-untracked-ci-flakes.md - master's rows (both entries 262 knew about are fixed and gone; simpleBatchTest and processInKeyOrder are new) plus 262's now-current account of the BlockedThreadAsserter collision. Fixed a blank line that was splitting the table in two. Worth flagging for the handoff: master's new `processInKeyOrder` section is about one of the three unexplained failures this branch pushed with at 48d210f, and says it is a solved flake still firing rather than a fresh one. - docs/inflight/pr-blockers-and-collisions.md - master's collision list, plus 262's transactional-stack section updated for reality: #261 merged on 2026-08-14, so the chain is now two PRs, not three. 261 is named rather than deleted because a reader who knows the stack as three links needs telling which one is already master. Verified locally: full-reactor `test-compile` green (main and both test source roots), check-quarantine-registry 0 entries, check-issue-refs clean. The unit suite has not run yet.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…ound Merging #257 brought master's newer repo-hygiene gates onto this branch, and they immediately found real debt in this branch's own notes - not in master's. `bin/check-all.sh` goes from 4 failing gates to 0. **check-inflight-tags** - nine notes here predate the tag scheme and carried no `inflight-type`, so the session index could not sort them and they were invisible at session start. Tagged by CONSEQUENCE per docs/inflight/AGENTS.md, not by filename prefix: the swallowed produce failures are `data-loss` (a batch marked succeeded for records never produced), the commit-interval override is `config-lie`, the poisoned transaction is `stall`, the unclosed Kafka clients are `crash`, and the two "tell someone" notes are `stranded-work`. Register hardening is `task` + `test-debt` rather than `misdirection`: the false-green at the top of its ranking is the strongest item, but `misdirection` is a bug impact and the note is a list of decisions. **check-branch-self-reference** - 24 sentences described #262 or its branch in the present tense, which the merge falsifies. Rewritten in post-merge terms and attested, not marked-and-left: the transactional-stack section now states what each PR IS rather than whether its check is red, the flakes ledger records the #265 collision and the rule-3 lift as history, and two "on this branch" phrases that named nothing are gone. `branch-transactional-battle-test.md` takes `post-merge: exempt-file` because being #262's issue map is the file's whole subject. One claim was wrong rather than merely stale, and is corrected: `pr-strategy-doc-merge-triggers.md` would have read as though the exactly-once headline had to move. It does not. Two claims were refuted against a master without #257 and both read PROVED with it merged in, which is what STRATEGY.md already says. **check-file-refs** - 22 path citations in the dated battle-test plan still said `io/confluent/`, broken by this branch's own rename commits. Repaired to `bz/stub/`, which docs/citations.md requires: the address of the thing the record pointed at may be fixed, its claims may not. Three of them name files the plan proposes to create for the deferred Phase B, so those paragraphs carry `file-refs: N/A` instead - a proposal is not a citation. **Deleted `handoff-262-master-merge.md`.** It was written for an agent picking this branch up cold, and this merge answers three of its four open items: master's #351 fixed the back-pressure flake, its de1620d un-quarantined PCMetricsTest, and it absorbed the produce-callback fix the one unresolved review thread was about. #261 merged, so the fourth is half gone. Its one piece of evidence that was not recorded elsewhere - the three `(CommitMode)[3]` failures seen locally on 2026-08-13 - is folded into the flakes ledger beside master's own independent `[3]` sighting, where someone looking for that failure will actually find it. The two corroborate: master saw `[3]` fail on a branch carrying no main Java, and this branch's own main-code changes are ruled out by grep. The assertion messages from the local run were not kept, so it is NOT established that the two are the same failure, and the note says so. The control that would settle it - the same suite on plain `origin/master` - is still unrun.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Un-quarantines
OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing— the most frequent tracked flake (4/45), which had been quarantined undiagnosed. It is a test-design defect, not a product defect: the assertion demanded an offset that back pressure exists to stop advancing.Mechanism. The encoded high-water mark written into commit metadata is the highest succeeded offset, not the highest polled one —
PartitionState, grepuse offsetHighestSucceeded instead of offsetHighestSeen, andencodeOffsetsCompressedtakes its range top fromgetOffsetHighestSucceeded(). Offset-encoding back pressure does not pause the poller; it gatesPartitionState#couldBeTakenAsWork, which once the partition is blocked refuses every record at or above the highest succeeded offset. So the instant back pressure engages, the succeeded frontier freezes.expectedHighestSeen— the last offset sent — is therefore unreachable, not late, which is why no wait ever closed it and why the actual varied run to run.It passed 41 of 45 times only because the mock consumer usually hands the whole extra batch over in one poll, so those records were already claimed as work before the block fired.
Evidence, and a control arm on the reason. A deterministic probe with no threads replaying the exact config shows the payload crossing the size threshold at offset 136 — exactly the reported actual. The real test was then made to fail deterministically by splitting the extra send 37 + 3, reproducing the CI signature character-for-character:
expected: 139 but was : 136 within 30 seconds. Moving the claim boundary and the threshold moves the frozen frontier with them — 136 / 124 / 129 / 136 across four configurations, never 139. The competing explanation (slowness) is dead: the frozen state is reached with zero time pressure.The fix, which strengthens rather than weakens.
139moves to where it is true — the partition'sgetOffsetHighestSeen(), awaited. Record registration is not gated by back pressure, so every extra record is seen. That check previously readisGreaterThan(numberOfRecordsToPrimeWith); it is now an exact equality.getNumberRecordsOutForProcessing() == numberOfBlockedMessages) so the succeeded frontier is read as a still value rather than one of two moving ones.Ruled out, so it is not re-derived. The torn-read family does not explain this: #344's tear widens the encoded range and pushes the decoded value up, while this failure is a shortfall — wrong direction; and the test passes base
0explicitly on deserialise, so #337's committed-base/payload tear cannot shift what it reads. The retry-delay sleep was ruled out previously and is recorded in the write-up: it runs after the failing assertion.Defect-class sweep — none found elsewhere.
OffsetEncodingBackPressureUnitTestcarries the identical expression but assertsgetOffsetHighestSeen()and succeeds every extra synchronously before encoding, which is exactly why it never flaked. The codec-level tests compare againsthighestSucceededby name.Residual, stated rather than buried. The ledger's other recorded actual, 132, is below what today's constants allow — the frozen frontier sits at or above the 136 block point, so 136/137/138. It implies a lower effective threshold in that run, most likely an older tree; the failing sha was never recorded. The fix removes the sensitivity either way. Recorded in the write-up.
Provenance carried across. The retired ledger entry was the reason quarantine rule 1 changed from no quarantine without diagnosis to accepting a sighting ledger. That fact would have been lost when the entry was deleted, so it now lives in the solution write-up's Prevention section, citing
docs/quarantined-tests.md, grepNo quarantine without evidence.Origin and scope. Diagnosed on the #306 branch, whose density benchmark measures the offset count at which each encoding hits the back-pressure threshold — the instrument that made the falsification arm testable. Extracted here as a standalone commit because #306's encoding work ships after 0.6.0.0, while this entry blocks that release under quarantine registry rule 5. Nothing here touches the delta-list or Z85 work.
Release impact. With #57 and #262, this empties the quarantine registry and clears the rule-5 release gate.
Verified at
b522c078e:bin/check-all.sh— 13 gates pass, 0 fail, 1 could not run (check-shell-lint.sh, noshellcheckon the box; this PR changes no shell). The test green post-merge, in the gating lane with the annotation removed. On CI at this head every required check passes, includingUnit Tests,Integration TestsandPerformance Tests.Review pass (
5b1e4a010), and one result worth reading. Two reviewers asked for the lower bound on the settled frontier to be tightened so that back pressure engaging early would go red — one wanted a numeric floor at the measured block point, the other wanted the committed payload asserted over the pressure threshold. Both were implemented and mutation-tested against the guard they protect, and both survive: mutatingupdateBlockFromEncodingResultfrommetaPayloadLength > getPressureThresholdValue()to... - 2leaves the test green, logging the identicalPayload size 32 higher than threshold 30.0as the control. The mock consumer hands the whole extra batch over in one poll, so the succeeded frontier reaches 139 before the first encode that follows the batch — it is decided by the claim boundary, not by where the threshold sits, so no assertion on it can observe the threshold moving. (The stronger... - 10mutant is killed, but by the pre-existingisAllowedMoreRecords()assertion above the section.) Adding an assertion that kills no mutant another assertion does not already kill is whatdocs/testing.md's method exists to prevent, so neither proposal landed; the bound is only restated as the band it describes,isBetween(numberOfRecordsToPrimeWith, lastOffsetSent), at identical strength. The gap is real but belongs to the deterministic probe, and is recorded indocs/inflight/test-back-pressure-engage-point-is-unasserted.mdwith the mutation evidence.Also in that pass: the assertions now read through the generated
ManagedTruthsubjects (d8de8082f), matching the sibling tests and this repo's Google-Truth convention; and thePCMetricsTest.metricsRegisterBindingrow this branch had added todocs/inflight/test-untracked-ci-flakes.mdis removed — it claimed an owner the registry contradicts, pointed "see below" at a section that does not exist, and duplicated a note that already tracks that test.Also carried here: a confluentinc#857 chaos sighting — and it is not the one this PR originally recorded.
Chaos Pain Suitewent red on this branch and is not this PR's: the only code file changed is a surefire unit test the chaos lane does not run. Three entries were written up before #354 landed on master, which ran the discriminator and closed theCLASS2_STALLline of that ledger as one timing proxy — and took the numbers fourteenth and fifteenth for different events. What survives the merge is one section, and it is the entry that got stronger rather than weaker:ChaosRevokeUnderWorkIT, seed8584935079849032188, failing a correctness assertion rather than a timing bound: a commit request timing out afterPT10Sfrom a broker-poll thread the runtime has positively established is alive and not throwing. test(chaos) confluentinc#857: the Class 2 lag bound measures speed, so it reports instead of gating #354 independently recorded the same signature from theIntegration Testslane the same day, on all three preconditions for the AB-BA cycle. Two lanes, two suites, one signature — so this is corroboration, not a tally mark. Neither identifies the deadlock; the discriminator both entries want is a thread dump at the moment of the timeout.CLASS2_STALLentries (seeds2445946824654755330and8791285396374198974, 154s and 153s against the 150s bound). Retracted on this branch as unclassified ind063fabda, then dropped in the merge: master's discriminator section explains the ~154s constant as arithmetic — the probe samples every 5s and the scenario fail-fasts, so the peak is always bound plus detection latency and carries no severity. The bound is now non-gating. Both seeds are kept in one paragraph so the pair is not rediscovered and re-argued.One citation was repaired in passing: this branch pointed at
test-class2-probe-asserts-timing-not-correctness.md, which master deleted, and now routes to its successor indocs/solutions/best-practices/the way master's own entries do.Checklist
docs/solutions/test-flakiness/back-pressure-freezes-the-frontier-the-test-asserted-2026-08-24.mdand newdocs/inflight/test-back-pressure-engage-point-is-unasserted.md; the closed entry retired fromdocs/inflight/test-untracked-ci-flakes.mdanddocs/quarantined-tests.mddocs/features/- N/A - a test-design fix with no user-facing surface