Repository navigation
test(core): stop the core tests jar configuring other modules, and remove the sleeps that stood in for events - #265
Merged
astubbs merged 14 commits intoAug 13, 2026
Conversation
…ing on clocks Two defects found while running Apache Kafka's own test suite against a patched build (#255). Both make the suite mean something other than what it says. 1. The core tests jar was configuring every downstream module's JUnit runner. src/test/resources/junit-platform.properties is packaged at the ROOT of the core tests jar. Eight modules depend on that jar - the four integrations plus every example - and JUnit reads the first junit-platform.properties on the classpath. So all eight silently ran with parallel.enabled and dynamic.factor=20 baked in at CORE's build time; their own setting was never consulted, and there was no way to opt out. That inverts the root pom's intent: the ci profile sets parallel-tests=false deliberately, because JUnit thread parallelism contends on static state and CI parallelises by forking JVMs instead. A locally-built jar bakes true, so a downstream module could inherit enabled=true, factor=20 exactly where the project decided it did not want it. It surfaced as 159 of 188 Kafka tests erroring on shared state. Moved to surefire/failsafe configurationParameters in core's pom. As build config it is per-module by construction: each module evaluates its own ${parallel-tests}, nothing is packaged, nothing leaks, and it outranks a properties file if one is reintroduced. Core keeps its parallelism - measured 102 worker threads before and 103 after. The eight downstream modules revert to the JUnit default, which is what they would always have had. 2. BlockedThreadAsserter asserted on a clock instead of on causality. assertUnblocksAfter scheduled the unblocker on a timer, then asserted the blocked function's wall-clock elapsed time was at least as long. It failed at 19.985s >= 20s - a 15ms scheduler wobble that says nothing about the behaviour under test - and it deliberately slept 20s to do it. The property it wants is an ordering fact, so it is now asserted as one: both events take a tick from a shared monotonic sequence, and the return must come after the unblock. No clocks, no sleeps, no timer. It also catches more than before - a function that never blocked takes the lower tick and fails, where previously it could pass if the timer had not fired. ProducerManagerTest goes from 23.06s to 3.32s. Also removes a redundant Thread.sleep(1000) in WorkManagerTest: it advances a mock clock, which is the only time source the retry path reads, then slept a real second anyway. The same idiom appears twice earlier in the file without one. Verified over 5 consecutive runs. Full core suite green: 321 tests, 0 failures.
…are the test Surveyed every Thread.sleep under src/test and src/test-integration across all modules against one rule: a sleep that waits and hopes a thing happened before asserting is a correctness sleep and goes; a sleep that IS the behaviour or the scenario stays. Eight removed, across seven files. The clearest two: ShardManagerTest slept 10ms of WALL CLOCK three times purely to advance a MOCK clock, then set it to Instant.now(). Replaced with clock.add(10, MILLIS) - and the two remaining setInstant(now) calls had to go with them, or the clock would jump backwards. CloseAndOpenOffsetTest slept 500ms for a commit, its own comment saying "sleep is lazy but much much simpler" with a TODO for an event. The event already existed: dirty is set on success and cleared by the commit, so !isDirty() IS commit-happened. TODO resolved. Also: PCMetricsTest's 1s anti-race sleep became an await on the trailing meters; MockConsumerEarlyCloseTest now counts auth failures actually served; OffsetEncodingBackPressureTest's sleep sat directly in front of the await that was already the real gate; VertxTest gained a latch inside the user function; RetriesTest awaits the retry drain and now VERIFIES the retried half succeeds, which nothing did before. Three correctness-adjacent sleeps deliberately kept, because there the elapsed time is the instrument rather than a guess: a poll-RATE assertion needs a window; "still paused after 1s" is a negative property; and a 3s soak gives the invariant sampler many chances to catch a violation. Everything else is fixture: simulated processing delay, deliberately blocked workers, drip-feeds, chaos cadences, deadlock windows. Notably OffsetCommittingSanityTest's JUST_SLEEP arm is a deliberately unverified dwell - awaiting there would collapse it into CHECK_CONSUMED and silently delete the boundary case. One behavioural finding: the SASL poll retry back-off is 5s, so the old sleep(5000) landed close() mid-back-off by luck rather than design. The await threshold is 2 served failures - enough to prove the poll loop retries, while close() still lands mid-back-off. Logs confirm close() breaks the back-off via closeInProgressSignal in ~90ms. No assertion deleted, nothing disabled, no coverage lost - two tests gained assertions. Core 321/0/10 matches baseline exactly; vertx 17/0/1; each changed class run 5 consecutive times.
…us commits The BlockedThreadAsserter rewrite made the assertion WEAKER in exactly the dimension it was meant to strengthen. Four reviewers found it independently; one reproduced it - a non-blocking function passed the new ordering assertion 177 times in 200. The old wall-clock version was flaky by 15ms; this was unsound by 88%. Cause: blockedFunctionEntered.countDown() was the first statement in the thread body, before the function ran, so the latch meant "thread started", not "reached the blocking call". The main thread usually won the race to tick, so a function that never blocked still took the higher tick. Second P1, found only by the adversarial lens: a blocked function that THREW was scored as a pass. The catch logged, then the return tick and methodReturned were set regardless. The timing version failed that case. Both fixed: the function is now proven still parked before the unblocker ticks, and its throwable is captured and asserted null. The tick comparison stays as a second line of defence, and the return wait is bounded at 20s to match its callers rather than silently taking Awaitility's 10s default in a file tuned for PIT. Adds BlockedThreadAsserterTest - the negative control AGENTS.md requires and that neither previous commit had. Building it refuted the obvious design: with only "returns immediately", removing the guard failed just 10 of 20 runs. The deterministic case is a function that returns on its own timer AFTER the unblocker ticks, which the ordering check provably cannot catch. Guard removed: red 20/20. Guard restored: green 20/20. Also from the review: the pom's duplicated parallelism config is now one property; MockConsumerEarlyCloseTest counts only poll-path auth failures, so ">= 2" means what its comment claims; RetriesTest wraps its await in try/finally with a daemon sampler so a timeout cannot leak the thread; CloseAndOpenOffsetTest requires the last success to have registered, not just a clean dirty flag; and the copyright, TODO-index and SELF_HOSTED_RUNNER gates are green again. The inflight note becomes branch-parallel-measurement-contamination.md, carrying only the still-open item. docs/inflight/AGENTS.md forbids the "delete this when it merges" marker the old one had, and the solved-work narrative belongs in a commit message, which is where it now is. Prior art I should have read before starting, and did not: docs/solutions/test-flakiness/unit-tests-parallelise-by-forking-not-threading had already measured this - fork x12 is 1:38 and reliable, thread x20 is ~2:32 and intermittent. So the leak was not merely "modules inherited a setting", it was forcing eight modules into the configuration this repo had measured as unreliable and deliberately switched off. Core: 328 tests, 0 failures, 10 skipped.
Dependency ReviewThe following issues were found:
License Issuesparallel-consumer-core/pom.xml
OpenSSF Scorecard
Scanned Files
|
|
Claude finished @astubbs's task in 12m 32s —— View job Claude's Code Review of PR #265Tasks
SummaryRan the code-review skill over the diff (
All three are minor/moderate robustness concerns rather than correctness bugs in the PR's stated fixes — the core defects described in the PR body (junit-platform.properties leaking into downstream modules, the two No repo-level or user-level |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
|
| PR | Base | Change | |
|---|---|---|---|
| Clones | 75 | 75 | ➖ 0 |
| Duplicated lines | 1052 | 1060 | 👍 -8 |
| Duplication | 2.98% | 3.04% | 🙂 -0.06% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 5% | ✅ Pass (2.98%) |
| Max increase vs base | +0.1% | ✅ Pass (-0.06%) |
⚠️ 2 new clones introduced
- 8 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/MockConsumerEarlyCloseTest.java:9<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/MockConsumerSaslAuthenticationTest.java:8 - 13 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/CommitRejectionTestBase.java:12<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/MockConsumerEarlyCloseTest.java:15
Powered by astubbs/duplicate-code-cross-check
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Both tests went red on this PR's own CI while it changed no Java at all, and both have a written fix sitting in an open PR. That is precisely the case the quarantine lane exists for: "tests that are red on master's gating CI when the fix lives in another, open PR". Leaving them red is the option the docs explicitly rule out - ambiguous checks and error-prone merge decisions - and @disabled is the other, which loses the signal entirely. Quarantine keeps them running in the non-gating lane while taking them out of the gates. WHY THEY QUALIFY Rule 1, no quarantine without diagnosis - both are diagnosed, in docs/inflight/test-untracked-ci-flakes.md: PCMetricsTest.metricsRegisterBinding compares a registry gauge against an expectation built from a test-side counter snapshot taken earlier in the method. Two independently-advancing values, read at different instants, with nothing holding processing still between them. Seen as expected 203.0 but was 207.0 - four more records completed in the gap, so the metric was MORE current than the expectation testing it. OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing sleeps out the static retry delay instead of awaiting the retry event, then asserts on a count still in motion. Fails as ConditionTimeout, expected 139 but was 136 within 30 seconds, when the runner is loaded enough that the sleep expires first. Rule 2, master-state not PR-state - both fail regardless of PR content. This PR contains no Java and no pom.xml, and the pair fired on two consecutive runs of one unchanged commit, each time a different one of them. fixedBy is #265 for both, verified against its diff rather than its title: it replaces the Thread.sleep(1000) above the metrics assertions with an await().untilAsserted(...) on the trailing meters, and swaps sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) for await().atMost(ofSeconds(30)).until(() -> attempts.get() >= 2). flapping = true on both. They pass most runs - the unit lane was green on eight consecutive PR runs across three branches the same day - so a pass proves nothing and must not post the merge-blocking "your fix landed" thread that deterministic quarantines get. WHY THEY ARE VISIBLE NOW #224 removed the surefire retry that was absorbing them. They were always failing at this rate; the retry was buying the green. This is the ledger entry from that PR getting its first real use. The copyright header on OffsetEncodingBackPressureTest gains the modifications line, required once an upstream-derived file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Two enforced gates disagreed, and the registry could not satisfy both. `bin/check-quarantine-owners.sh` and `bin/quarantine-lane-report.sh` both parsed the owner marker with `grep -oE 'Owner: PR #[0-9]+'`, so the registry had to write a BARE number. `bin/check-issue-refs.sh` rejects a bare `#NN` on an added line, because the fork's numbers sit inside confluentinc's range and a bare one is a coin flip about which repo is meant. Adding the first two entries to this registry therefore failed the issue-ref gate by construction - not through carelessness, but because the two formats were mutually exclusive. The previous commit worked around that with the issue-ref gate's documented `issue-refs: N/A` opt-out. That was the wrong instrument: the opt-out exists for references that genuinely need no qualifier, not for a format conflict that can be fixed. An N/A left in place would also have quietly become permanent, since every future entry hits the same wall. Both parsers now accept `Owner: PR #NN`, `Owner: PR astubbs#NN`, and `Owner: PR astubbs/parallel-consumer#NN`, and the number is extracted from the `#NN` tail rather than the whole match, so a digit inside a qualifier cannot win. The bare form still parses, so existing entries keep working; the registry's format spec now asks for the qualified one and says why. Verified three ways rather than by inspection: qualified `Owner: PR #265` -> resolves, owner claim verified bare `Owner: PR #265` -> still resolves (back-compat intact) bogus `Owner: PR astubbs#999999` -> ERROR, PR does not exist The third is the one that matters. A parser that silently failed to match would also report no error, and would look identical to a pass - so the control had to show a wrong number being CAUGHT, not just a right number being accepted. Both parsers are changed together and carry a comment saying so; they read the same marker and drifting apart would mean the lane reporter attributing an entry to a different PR than the audit checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Both tests went red on this PR's own CI while it changed no Java at all, and both have a written fix sitting in an open PR. That is precisely the case the quarantine lane exists for: "tests that are red on master's gating CI when the fix lives in another, open PR". Leaving them red is the option the docs explicitly rule out - ambiguous checks and error-prone merge decisions - and @disabled is the other, which loses the signal entirely. Quarantine keeps them running in the non-gating lane while taking them out of the gates. WHY THEY QUALIFY Rule 1, no quarantine without diagnosis - both are diagnosed, in docs/inflight/test-untracked-ci-flakes.md: PCMetricsTest.metricsRegisterBinding compares a registry gauge against an expectation built from a test-side counter snapshot taken earlier in the method. Two independently-advancing values, read at different instants, with nothing holding processing still between them. Seen as expected 203.0 but was 207.0 - four more records completed in the gap, so the metric was MORE current than the expectation testing it. OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing sleeps out the static retry delay instead of awaiting the retry event, then asserts on a count still in motion. Fails as ConditionTimeout, expected 139 but was 136 within 30 seconds, when the runner is loaded enough that the sleep expires first. Rule 2, master-state not PR-state - both fail regardless of PR content. This PR contains no Java and no pom.xml, and the pair fired on two consecutive runs of one unchanged commit, each time a different one of them. fixedBy is #265 for both, verified against its diff rather than its title: it replaces the Thread.sleep(1000) above the metrics assertions with an await().untilAsserted(...) on the trailing meters, and swaps sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) for await().atMost(ofSeconds(30)).until(() -> attempts.get() >= 2). flapping = true on both. They pass most runs - the unit lane was green on eight consecutive PR runs across three branches the same day - so a pass proves nothing and must not post the merge-blocking "your fix landed" thread that deterministic quarantines get. WHY THEY ARE VISIBLE NOW #224 removed the surefire retry that was absorbing them. They were always failing at this rate; the retry was buying the green. This is the ledger entry from that PR getting its first real use. The copyright header on OffsetEncodingBackPressureTest gains the modifications line, required once an upstream-derived file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Two enforced gates disagreed, and the registry could not satisfy both. `bin/check-quarantine-owners.sh` and `bin/quarantine-lane-report.sh` both parsed the owner marker with `grep -oE 'Owner: PR #[0-9]+'`, so the registry had to write a BARE number. `bin/check-issue-refs.sh` rejects a bare `#NN` on an added line, because the fork's numbers sit inside confluentinc's range and a bare one is a coin flip about which repo is meant. Adding the first two entries to this registry therefore failed the issue-ref gate by construction - not through carelessness, but because the two formats were mutually exclusive. The previous commit worked around that with the issue-ref gate's documented `issue-refs: N/A` opt-out. That was the wrong instrument: the opt-out exists for references that genuinely need no qualifier, not for a format conflict that can be fixed. An N/A left in place would also have quietly become permanent, since every future entry hits the same wall. Both parsers now accept `Owner: PR #NN`, `Owner: PR astubbs#NN`, and `Owner: PR astubbs/parallel-consumer#NN`, and the number is extracted from the `#NN` tail rather than the whole match, so a digit inside a qualifier cannot win. The bare form still parses, so existing entries keep working; the registry's format spec now asks for the qualified one and says why. Verified three ways rather than by inspection: qualified `Owner: PR #265` -> resolves, owner claim verified bare `Owner: PR #265` -> still resolves (back-compat intact) bogus `Owner: PR astubbs#999999` -> ERROR, PR does not exist The third is the one that matters. A parser that silently failed to match would also report no error, and would look identical to a pass - so the control had to show a wrong number being CAUGHT, not just a right number being accepted. Both parsers are changed together and carry a comment saying so; they read the same marker and drifting apart would mean the lane reporter attributing an entry to a different PR than the audit checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…was wrong Three review findings, all correct, two of them mine from the previous commit. THE QUARANTINE THAT SHOULD NOT HAVE HAPPENED OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing is un-quarantined, and its registry entry removed. It was tagged with the diagnosis "sleeps out the static retry delay instead of awaiting the retry event" and owned by #265, which replaces exactly that sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) with an await. The narrative fitted. The line numbers do not. The failure is at line 211 of the commit CI ran: the waitAtMost(defaultTimeout).untilAsserted(...) block asserting committed offset metadata, specifically Truth8.assertThat(incompletes.getHighestSeenOffset()).hasValue(expectedHighestSeen) - the "value of: optional.get()" in the failure text is that Optional. It runs BEFORE the retry section #265 rewrites, and a change downstream of a failing assertion cannot fix it. So the real failure is a timeout waiting for the high-water mark to reach 139, stuck at 136, and nothing currently explains why three records never arrive. Rule 1 applies: no quarantine without diagnosis, undiagnosed red stays red and blocks, on purpose. Taking it out of the gating lane on a diagnosis that does not describe it is worse than leaving it red - it removes the pressure while pretending the cause is known. The error worth remembering: the fix PR was matched to the failure by subject-matter resemblance - both concern this test, both concern waiting - instead of by checking that the changed lines execute before the failing assertion. Match a fixedBy to a stack line, not to a theme. PCMetricsTest keeps its quarantine. Its diagnosis is a source-level read of two values sampled at different instants, and #265 inserts its await BEFORE the failing assertion, so it can actually gate it. A CROSS-CHECK THAT HAD SILENTLY STOPPED CHECKING check-quarantine-owners.sh compares the annotation's fixedBy against the registry's Owner line, and parsed it as `fixedBy = "PR #NN"`. The annotations added here write `fixedBy = "#265"`, so the grep matched nothing, the declared value came back empty, and the advisory could never fire. Adding the qualified form to one side quietly disabled the check that keeps the two sides honest. It now accepts the same three forms as the Owner marker. Proven by negative control rather than inspection: setting the annotation to astubbs#999 against a registry Owner of #265 now reports "annotation says fixedBy PR #999 but the registry Owner line says PR #265 - align them". Before the fix it reported nothing at all. A WITHDRAWN INFERENCE The ledger claimed the rerun failing at a DIFFERENT test was "the strongest evidence" against a regression, reasoning that a regression fails the same way twice. That does not hold: under concurrent execution one defect can perturb timing enough to surface different tests and different failure modes, so two dissimilar failures show only that the first did not reproduce. Withdrawn, with the load-bearing evidence restated as the source-level read it always should have been. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
4 tasks done
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…undiagnosed entry diagnosed Two Codex review findings on #288, both correct. THE fixedBy CROSS-CHECK WAS STILL SILENTLY DISABLED The branch cherry-picked the Owner-marker parser fix from #286 but not the fixedBy one - that fix travelled in the same commit as the un-quarantine this PR deliberately skipped, and the handoff's claim that "the cross-check now accepts three forms" described that branch, not this one. So the extraction still matched only `fixedBy = "PR #NN"`, and PCMetricsTest's qualified `fixedBy = "#265"` parsed as empty - the annotation-versus-registry advisory could never fire, exactly the silent-disable #286 documented. The parser hunk is taken from #286 (8a7e1a0) verbatim, and this time it gets the regression test that commit lacked: a qualified fixedBy disagreeing with a qualified Owner line must produce the mismatch advisory. Against the old parser that test fails (empty declared, no advisory), so the silent-disable class is now fenced, not just fixed. THE ADVISORY REPORTED THE OPPOSITE OF THE ENTRY'S KEY STATE check-quarantine-owners.sh printed "diagnosed-but-unowned, find it an owner" for every unowned entry - including the backpressure one, whose defining property is being UNDIAGNOSED. Telling a maintainer to find an owner for a failure nobody understands points them at the wrong task. The no-owner branch now distinguishes: an entry recording a rule-1 exception reports "UNDIAGNOSED (recorded rule-1 exception); completing the diagnosis is the open task", and quarantined-test.sh's audit banner no longer claims every entry must be diagnosed. Both wordings are pinned by tests, including that the exception advisory does NOT contain "diagnosed-but-unowned". Verified: owner check green with the corrected advisories against the live registry, and the 40 quarantine contract/script tests pass (two new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…xplicit rule-1 exception (#288) Both tests were hidden by the surefire retry until #224 removed it, and both then failed outright on the gating unit lane, blocking every PR in the fork. They are quarantined here on deliberately different terms, because their evidence is different. PCMetricsTest.metricsRegisterBinding is diagnosed and owned. It builds its expectation from a test-side counter snapshot taken earlier in the method, then asserts it against a registry gauge - two independently-advancing values sampled at different instants with nothing holding processing still between them. Seen as PARTITION_HIGHEST_COMPLETED_OFFSET expected 203.0 but was 207.0: four more records completed in the gap, so the metric was MORE current than the expectation testing it. Owner is #265, verified against that PR's diff rather than its title - its await() replaces a Thread.sleep upstream of the failing assertion, so it genuinely gates it. On merge, that PR must delete the annotation and the registry entry in the same commit. OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing is UNDIAGNOSED, and is quarantined as an explicit rule-1 exception by owner decision. It fails as a ConditionTimeout at the getHighestSeenOffset() assertion, where the committed high-water mark never reaches expectedHighestSeen (139), with a different actual each run - 136 and 132 both observed. An earlier attempt attributed it to the retry-delay sleep and was reverted: that code runs AFTER the failing assertion, so the PR rewriting it cannot be the fix. The lesson is recorded with the entry - match a fixedBy to a stack line, not to a theme. The entry carries no owner, so the audit reports it as unowned rather than passing it silently, and the open hypothesis is written down with its falsification path: the expectation may be unreachable rather than late, because the extra records it counts on are the ones whose encoding crosses the threshold that stops intake. Rule 1 said undiagnosed red stays red. Taking the exception was a judgement that at 4/45 - the most frequent tracked flake - the blocking cost outweighed the pressure the redness was applying. It is recorded as an exception everywhere it appears, so the diagnosis remains an open task rather than a closed one. Two audit-tooling defects surfaced in review and are fixed with regression tests. The fixedBy cross-check parsed only `fixedBy = "PR #NN"`, so a qualified astubbs#NN reference matched nothing, the declared value came back empty, and the annotation-versus-registry advisory could never fire - a check that had silently stopped checking the moment the qualified form was introduced. And the no-owner advisory reported "diagnosed-but-unowned, find it an owner" for every unowned entry, including one whose defining property is being undiagnosed; it now distinguishes the two, so CI points at completing the diagnosis rather than at finding an owner for a failure nobody understands. The first version of that second fix was itself caught by the repo's own guard: `echo "$block" | grep -qi ...` reports failure precisely when it MATCHES, because grep -q exits early, the writer takes EPIPE, and pipefail promotes that to the pipeline's status. It only bites once the writer has more than 64 KiB left to send, so a small registry passes and production breaks. Rewritten as a herestring; the tree was swept for other instances of the class and there are none. The registry rules are also brought into line with what the checks actually enforce. Rule 4 claimed every entry needs an owning fix PR; the checks have always treated a missing owner as an advisory and hard-failed only on owner claims that are WRONG - a closed-unmerged owner, or a merged owner with the test still quarantined. An owning PR is the goal, not a precondition, and unowned entries stay visible through the per-PR lane report and the release guard. For a user: no production behaviour changes. The gating unit lane stops failing on these two tests while they remain visible and executed on every PR, and a release cannot ship while either is still quarantined. Co-authored-by: Claude Fable 5 (1M context) <noreply@anthropic.com>
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
… the write-up where it belongs Two things, neither about the review system this PR otherwise changes. QUARANTINE ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect. Met while babysitting this PR - which is the point, flakes get quarantined as they are met rather than waiting for the next sweep. It would not have been found by a sweep anyway: the 2026-08-07 scan read surefire `Flakes:` markers, which only exist when a retry re-ran a test and it then passed, and this one failed its run outright. Rule 1 is satisfied rather than excepted - the mechanism is known and the fix is already written. The defect is in the shared BlockedThreadAsserter#assertUnblocksAfter helper, not in this test: the unblocker is scheduled BEFORE the elapsed clock starts, so the measured window begins after the delay it is compared against and is short by however long arming the scheduler takes. Under load that exceeds a millisecond and `isAtLeast` fails a correct implementation. Seen as `expected to be at least PT20S but was PT19.998S` - two milliseconds on a twenty-second bound, which is the shape of a measurement error, not a behavioural one. Rule 2 is settled without needing a rate: it failed on a PR whose diff contained no Java at all, so nothing in the change could have caused it. Owner #262, which stamps the arming instant and measures from there; `flapping = true` because it passes most runs. Filed against the helper in the tracker, since any test using it can show the same signature - and noting that #265 deletes the same assertion instead, so whichever lands second has a real decision to make rather than a mechanical merge. Verified with both arms rather than assumed: under the gating exclusions the method reports `Tests run: 0`, with `-Dexcluded.groups=` it runs and passes in 21.04s, and the three sibling tests in the class still run in the gating lane - so the exclusion is method-granular and the lane still watches it. Registry and owner checks pass. FILE THE DUPLICATION WRITE-UP UNDER workflow-issues. It was in documentation-gaps, contradicting its own frontmatter, which already said `root_cause: inadequate_tooling_scope` and `resolution_type: workflow_improvement`. Nothing was missing from the docs - the opposite, one contract written nine times - and the causes are scanner scope and review granularity. It sits next to negative-results-need-an-instrument-that-could-have-said-yes.md, which it already cross-references as the same shape.
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…n out of it, and ask the gate a simpler question (#287) #284 split the code review into a cheap always-on gate and an on-demand reviewer. The gate worked. The reviewer did not: `track_progress` does not support `workflow_dispatch`, so the action refused before reviewing anything. With the auto-review already removed, that left no working route and a required check nothing could turn green. MAKING IT WORK The obvious fix - synthesise a `pull_request` payload so the action sees a supported event - is impossible, and was proved on a runner rather than argued. It passes locally: the action's own parser yields the right PR in tag mode. But the runner writes the `GITHUB_*` context over every step's environment AFTER the step's own `env:` block, and `event_name` is on that allowlist. The payload file is writable; the event name is not, and the name alone picks the mode. That commit was dropped. So the dispatch route runs in agent mode, and the three losses are paid out loud rather than discovered later: no tracking comment (the summary comment is now mandatory in the prompt), no inline-comment tool, and no CI MCP server (replaced with `gh run list -c <sha>` under a scope that is actually documented). The inline-comment loss turned out to be confined to that route. `claude.yml` was already in tag mode, and the inline-comment server installs for any ENTITY event - `issue_comment` is one, `workflow_dispatch` is not - so recovering it there was a one-item addition to a list this change was already writing. The guidance therefore inverts rather than degrades: ask by comment when you want findings that mechanically block (real threads, which required_review_thread_resolution enforces), dispatch when you want `-f focus` or the packaged procedure. TAKING THE WRITE TOKEN OUT OF REACH #284 shipped a privilege escalation. The reviewer job held `actions: write`, checked out `refs/pull/<n>/head` with credentials persisted by default, and by design executes this repo's `bin/` scripts from that checkout. A same-repo PR could edit an allowed script, recover the token from `.git/config`, and dispatch any workflow - `release.yml` has no actor gate. Now `actions: read` with `persist-credentials: false`, and `actions: write` isolated in a checkout-free `refresh-gate` job whose inputs cross as job outputs, so the head-moved refusal still compares against the SHA the reviewer actually read. The same gap existed on the comment route, traced live on #288: a review posted at 08:29 satisfied every rule and the required check sat red until a hand-run rerun at 08:37. That route now has the same split. Granting the curated allowlist there needed two carve-outs, both found in review: the execution tools are withheld from fork heads and from untrusted commenters. The first attempt at that guard was written as `${{ fork && '' || format(...) }}`, which needs a truthy middle operand - so it returned the FULL allowlist while the step above logged "WITHOUT execution tools". A guard that did not guard and said it had. ASKING THE GATE A SIMPLER QUESTION The gate required a review NEWER than the head. It now accepts any finished `claude[bot]` review on the PR. What that gives up is real - a review of the first commit vouches for the twentieth - and it is affordable only because Codex auto-reviews every push, so per-commit coverage comes from there. If that ever stops being true, per-commit coverage stops coming from anywhere; that trigger is recorded, with the strict implementation preserved at the tag `archive/review-gate-strict-head-freshness`. Deleted with it: the head/review timestamp comparison, preferring the server-side timestamp over the contributor-controlled committer date, same-second tie handling, the reviewed-SHA plumbing, the head-moved guard, and `checks: read`. The two surviving rules - identity and completion - are mutation-tested, six mutants all killed, including one that re-adds a freshness comparison, so restoring strictness silently now breaks the suite. One correction, because the first version of this change claimed otherwise: leniency does NOT retire `actions: write`. The gate fires on `pull_request` only, so the terminal flow - push, ask for review, merge - raises no event after the review lands. The refresh jobs stay. That scope is the cost of the check being produced by a `pull_request` workflow, not of strictness, and retiring it needs the reviewer to raise a check run on the reviewed SHA. Tracked. Leniency also opened a hole and closed it: the gate now asks only whether a finished review exists, and the comment route answers `@claude` on fork PRs - so a maintainer asking a QUESTION would have greened a fork PR. Fork heads are refused before any comment is read. ALSO - The action exits 0 on a workflow-validation refusal, so the job reported success. It now fails when the action's own `conclusion` output is empty. - The contract was stated in nine files, fourteen times, in nine different sentences. Collapsed to one canonical paragraph, with the rest reduced to pointers. Why no tool caught it is written up: `dups: clones` and `dups: similarity` scan `parallel-consumer-*/src` only, so docs, workflows and `bin/` are scanned by neither - and even aimed at them, token-based clone detection cannot see paraphrase. The scanners catch copy-paste; what bit here was restatement. - `.github/workflows/README.md`, one line per workflow, leading with the fact that `claude-code-review.yml` is the one that does not review. - A flapping `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` quarantined, pointing at its fix in #262. Note that #265 takes an incompatible approach to the same line - it deletes the assertion rather than anchoring the measurement - so whichever lands second will conflict there. - A cooperative-variant chaos sighting captured against the confluentinc#857 family (fork mirror #119) before its seed expired, recorded as consistent with that family rather than confirmed as it: the eager variant was already logged with a clean ProgressProbe kill, while this one shows only the shutdown-path timeout. Verified end to end on this PR: the review was requested by comment, posted as `claude[bot]`, and the gate went green - then stayed green across two further pushes with no new review. Under the old rule each push would have reddened it. This is the first PR in this series to merge without an admin bypass. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 11, 2026
astubbs
added a commit
that referenced
this pull request
Aug 18, 2026
…hdrawn Three additions to the family ledger, all found while working #209. A fifth sighting, and the one that moves triage furthest: the same ZOMBIE_MEMBER signature with #292 present and no double-start anywhere in the log. The harness was ruled out by counting rather than by assertion - 0 concurrent double-runs across 41 run() invocations over 16 instances, 0 guard engagements, so the race window was never even entered, and 0 teardown indicators. That retires the fourth sighting from the family as answered, and rewrites the family's triage rule: a ZOMBIE_MEMBER violation is no longer evidence of the double-start, so grep the log for two "Running consumer instance N" lines on different pool workers within ~2s - present, the guards regressed; absent, it is the other cause and belongs in this file. A sixth sighting with a full autopsy, and the retrieval trap that nearly hid it: the console log for that job was truncated mid-run, so grepping it returned zero for every probe signature - which reads exactly like a clean run. The violations, the autopsy and the seed all came from the uploaded artifact. A zero from the console is not a zero. A seventh sighting, and the first bisected against a clean baseline rather than argued about. PCMetricsTest.metricsRegisterBinding lost two records on a full multi-module run - the family's signature, on a branch whose subject is work being taken and not accounted for, so it was either the best evidence yet or noise. A detached worktree at the pre-change commit settled it: the failure reproduces without the branch, so it is not attributable to #209. The dismissal that came with it is withdrawn. The entry originally said "load, not the test" and moved on. That is the move AGENTS.md names explicitly - a test failing under concurrent load may be exposing a real bug that only manifests under stress - and it was self-serving, because the load was partly self-inflicted by concurrent Maven runs. "Only fails under load" is what a flaky test is, and CI runs the suite under load too. Since #265 un-quarantined that test the day before, this is a post-un-quarantine failure and therefore evidence the un-quarantine was premature. The entry now says so, and names what would settle it: a repeated full-suite run on a CI runner rather than a developer box. The method is the reusable part. Neither "it passed on retry" nor "it failed once" is evidence alone; a worktree at the pre-change commit running the same command is cheap, and it is what turns "probably a flake" into an answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ahSzHD72EvTGTYkmpTsx6
astubbs
added a commit
that referenced
this pull request
Aug 18, 2026
…hdrawn Three additions to the family ledger, all found while working #209. A fifth sighting, and the one that moves triage furthest: the same ZOMBIE_MEMBER signature with #292 present and no double-start anywhere in the log. The harness was ruled out by counting rather than by assertion - 0 concurrent double-runs across 41 run() invocations over 16 instances, 0 guard engagements, so the race window was never even entered, and 0 teardown indicators. That retires the fourth sighting from the family as answered, and rewrites the family's triage rule: a ZOMBIE_MEMBER violation is no longer evidence of the double-start, so grep the log for two "Running consumer instance N" lines on different pool workers within ~2s - present, the guards regressed; absent, it is the other cause and belongs in this file. A sixth sighting with a full autopsy, and the retrieval trap that nearly hid it: the console log for that job was truncated mid-run, so grepping it returned zero for every probe signature - which reads exactly like a clean run. The violations, the autopsy and the seed all came from the uploaded artifact. A zero from the console is not a zero. A seventh sighting, and the first bisected against a clean baseline rather than argued about. PCMetricsTest.metricsRegisterBinding lost two records on a full multi-module run - the family's signature, on a branch whose subject is work being taken and not accounted for, so it was either the best evidence yet or noise. A detached worktree at the pre-change commit settled it: the failure reproduces without the branch, so it is not attributable to #209. The dismissal that came with it is withdrawn. The entry originally said "load, not the test" and moved on. That is the move AGENTS.md names explicitly - a test failing under concurrent load may be exposing a real bug that only manifests under stress - and it was self-serving, because the load was partly self-inflicted by concurrent Maven runs. "Only fails under load" is what a flaky test is, and CI runs the suite under load too. Since #265 un-quarantined that test the day before, this is a post-un-quarantine failure and therefore evidence the un-quarantine was premature. The entry now says so, and names what would settle it: a repeated full-suite run on a CI runner rather than a developer box. The method is the reusable part. Neither "it passed on retry" nor "it failed once" is evidence alone; a worktree at the pre-change commit running the same command is cheap, and it is what turns "probably a flake" into an answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ahSzHD72EvTGTYkmpTsx6
astubbs
added a commit
that referenced
this pull request
Aug 18, 2026
…hdrawn Three additions to the family ledger, all found while working #209. A fifth sighting, and the one that moves triage furthest: the same ZOMBIE_MEMBER signature with #292 present and no double-start anywhere in the log. The harness was ruled out by counting rather than by assertion - 0 concurrent double-runs across 41 run() invocations over 16 instances, 0 guard engagements, so the race window was never even entered, and 0 teardown indicators. That retires the fourth sighting from the family as answered, and rewrites the family's triage rule: a ZOMBIE_MEMBER violation is no longer evidence of the double-start, so grep the log for two "Running consumer instance N" lines on different pool workers within ~2s - present, the guards regressed; absent, it is the other cause and belongs in this file. A sixth sighting with a full autopsy, and the retrieval trap that nearly hid it: the console log for that job was truncated mid-run, so grepping it returned zero for every probe signature - which reads exactly like a clean run. The violations, the autopsy and the seed all came from the uploaded artifact. A zero from the console is not a zero. A seventh sighting, and the first bisected against a clean baseline rather than argued about. PCMetricsTest.metricsRegisterBinding lost two records on a full multi-module run - the family's signature, on a branch whose subject is work being taken and not accounted for, so it was either the best evidence yet or noise. A detached worktree at the pre-change commit settled it: the failure reproduces without the branch, so it is not attributable to #209. The dismissal that came with it is withdrawn. The entry originally said "load, not the test" and moved on. That is the move AGENTS.md names explicitly - a test failing under concurrent load may be exposing a real bug that only manifests under stress - and it was self-serving, because the load was partly self-inflicted by concurrent Maven runs. "Only fails under load" is what a flaky test is, and CI runs the suite under load too. Since #265 un-quarantined that test the day before, this is a post-un-quarantine failure and therefore evidence the un-quarantine was premature. The entry now says so, and names what would settle it: a repeated full-suite run on a CI runner rather than a developer box. The method is the reusable part. Neither "it passed on retry" nor "it failed once" is evidence alone; a worktree at the pre-change commit running the same command is cheap, and it is what turns "probably a flake" into an answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ahSzHD72EvTGTYkmpTsx6
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
#320 added a gate for cited paths that do not exist, and deferred the 87 already in the tree. Deferring them is a chicken-and-egg: the gate cannot run whole-tree - the mode that also catches a file being deleted out from under its citations - until the count is zero. This clears the ones that are documentation defects, and identifies the rest as gate defects rather than documentation ones. WHAT WAS ACTUALLY WRONG, ONCE READ ONE AT A TIME * 26 addresses stale from the io.confluent -> bz.stub rename. Every target still exists at the renamed path; repairing the address is what docs/citations.md requires ("a citation that no longer resolves makes the record LESS faithful"). Slash-form PATHS only - dot-form package prose in dated documents is untouched. * 2 genuine breakages: docs/ci.md's citation of the never-written bin/check-review-gate-contract.sh (the same edit as #319, so the two merge cleanly), and `example-metrics/CoreApp.java`, a shorthand that names no real path - now `examples/metrics/CoreApp.java`, which resolves as a real path tail. * ~20 paths named DELIBERATELY: a proposal for a script yet to be written, a generated file a plan decided against, artefacts a plan records as deleted, documents living on another branch. Each keeps its wording and gains `<!-- file-refs: N/A - <reason> -->`, which the gate reads as covering the paragraph above it. * 1 deleted file given the history pointer docs/citations.md prescribes: junit-platform.properties, removed by #265, now read via `git show 15f1ebe^:...` - verified to resolve. AN INHERITED RULE THIS DELIBERATELY NARROWS docs/plans/2026-08-11-001-refactor-package-rename-plan.md says dated documents "must not be rewritten to say something they did not say... expect the answer to be 'leave it' for everything under docs/plans/". That governs CLAIMS, and it is obeyed: no finding, conclusion or measurement changed. It does not govern ADDRESSES, which docs/citations.md - the owner of citation repair - requires be kept working. The line drawn is slash-form path citations get repaired; dot-form package names in prose do not. The first sweep proved why that line matters. A blanket substitution turned "Move `src/main/java/io/confluent/...` to `.../bz/stub/...`" into "Move bz/stub to bz/stub", and rewrote a `git show <sha>^:<path>` pointer to a path that did not exist at that commit - breaking the one thing it was for. Both were caught by reading the diff rather than the count, and both are now preserved with a stated reason. WHAT REMAINS IS THE GATE'S FAULT, NOT THE DOCS' 31 of the original 87 still report, and every one is a false positive: * 30 are `docs/inflight.md`, cited across 8 documents that ALREADY carry `git show 0de96fc^:docs/inflight.md`. That is precisely the repair docs/citations.md prescribes, correctly applied - and the gate reports it as 30 defects, penalising the documented fix. * 1 is a `<sha>^:<path>` history pointer, correct because the path no longer exists. Both are gate rules, queued for #320: a path preceded by a git revision is a history pointer, and a path is repaired when its own document carries a pointer naming it. They remove 31 findings without editing a single document - which is the right split of the work. Verified: check-issue-refs, check-docs-data, check-copyright-headers, todo-index --check and the gate's 34 assertions all pass; the two history pointers confirmed to resolve with `git show`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
Eighty-seven paths in the tree resolved to nothing. Read one at a time they were
four different problems, and only two of them were defects:
* 26 addresses stale from the io.confluent -> bz.stub rename. Every target
still exists at the renamed path, so repairing the address is what
docs/citations.md requires - "a citation that no longer resolves makes the
record LESS faithful". Slash-form PATHS only; dot-form package prose in
dated documents is untouched.
* 2 genuinely broken: docs/ci.md's citation of the never-written
bin/check-review-gate-contract.sh, and `example-metrics/CoreApp.java`, a
shorthand naming no real path - now `examples/metrics/CoreApp.java`, which
resolves as a real path tail.
* ~20 named DELIBERATELY - a proposed script, a generated file a plan decided
against, artefacts recorded as deleted, documents living on another branch.
Each keeps its wording and gains a marker with a stated reason.
* 1 deleted file given the history pointer docs/citations.md prescribes:
junit-platform.properties, removed by #265, read
via `git show 15f1ebe^:...` - verified to resolve.
AN INHERITED RULE THIS DELIBERATELY NARROWS.
docs/plans/2026-08-11-001-refactor-package-rename-plan.md says dated documents
"must not be rewritten to say something they did not say... expect the answer to
be 'leave it' for everything under docs/plans/". That governs CLAIMS, and it is
obeyed: no finding, conclusion or measurement changed. It does not govern
ADDRESSES, which docs/citations.md - the owner of citation repair - requires be
kept working. The line drawn is slash-form path citations get repaired,
dot-form package names in prose do not.
The first sweep proved why that line matters, and why this was done by reading
each diff rather than by counting. A blanket substitution turned "Move
`src/main/java/io/confluent/...` to `.../bz/stub/...`" into "Move bz/stub to
bz/stub", and rewrote a `git show <sha>^:<path>` pointer to a path that did not
exist at that commit - breaking the one thing it was for. Both are preserved
with a stated reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
Eighty-seven paths in the tree resolved to nothing. Read one at a time they were
four different problems, and only two of them were defects:
* 26 addresses stale from the io.confluent -> bz.stub rename. Every target
still exists at the renamed path, so repairing the address is what
docs/citations.md requires - "a citation that no longer resolves makes the
record LESS faithful". Slash-form PATHS only; dot-form package prose in
dated documents is untouched.
* 2 genuinely broken: docs/ci.md's citation of the never-written
bin/check-review-gate-contract.sh, and `example-metrics/CoreApp.java`, a
shorthand naming no real path - now `examples/metrics/CoreApp.java`, which
resolves as a real path tail.
* ~20 named DELIBERATELY - a proposed script, a generated file a plan decided
against, artefacts recorded as deleted, documents living on another branch.
Each keeps its wording and gains a marker with a stated reason.
* 1 deleted file given the history pointer docs/citations.md prescribes:
junit-platform.properties, removed by #265, read
via `git show 15f1ebe^:...` - verified to resolve.
AN INHERITED RULE THIS DELIBERATELY NARROWS.
docs/plans/2026-08-11-001-refactor-package-rename-plan.md says dated documents
"must not be rewritten to say something they did not say... expect the answer to
be 'leave it' for everything under docs/plans/". That governs CLAIMS, and it is
obeyed: no finding, conclusion or measurement changed. It does not govern
ADDRESSES, which docs/citations.md - the owner of citation repair - requires be
kept working. The line drawn is slash-form path citations get repaired,
dot-form package names in prose do not.
The first sweep proved why that line matters, and why this was done by reading
each diff rather than by counting. A blanket substitution turned "Move
`src/main/java/io/confluent/...` to `.../bz/stub/...`" into "Move bz/stub to
bz/stub", and rewrote a `git show <sha>^:<path>` pointer to a path that did not
exist at that commit - breaking the one thing it was for. Both are preserved
with a stated reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we don't know the mechanism" and "we don't know whether it's ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - test-untracked-ci-flakes.md records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it's red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - that is the label the drain-zombie carried right up until it turned out to be a real product bug. Updated in all six places that stated it, so they cannot drift: docs/quarantined-tests.md (owner, and the fullest statement), docs/testing.md, AGENTS.md's table cell, the @Quarantined javadoc, bin/check-quarantine-registry.sh's drift message, and QuarantinedAnnotationContractTest's assertion message. The #265 entry in test-untracked-ci-flakes.md, which reasoned FROM the old rule, is rewritten rather than find-replaced: it now qualifies on the rule itself instead of needing the exception. Also records the failure that prompted this, which turned out not to need quarantining at all: docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md. PCMetricsTest.metricsRegisterBinding asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter, while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. The observed 1214 vs 1207 is 214 completions against 207 contiguous - 7 records finished past a gap - and the gap is PERMANENT, because workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes. No timeout can close it. That is a wrong expectation, not a flake, and the note says so along with the fix (compare against PARTITION_HIGHEST_SEQUENTIAL_SUCCEEDED_OFFSET) and a warning not to widen the timeout instead. It is also NOT a confluentinc#857 sighting, and the note says why: every sighting in that family is a chaos/rebalance integration test with a commit mode and a seed, whereas this is a MockConsumer unit test with no broker, rebalance, revoke path or mode. bug-857-family.md already records one contamination of exactly this kind, so the ruling-out is written down rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we don't know the mechanism" and "we don't know whether it's ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - test-untracked-ci-flakes.md records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it's red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - that is the label the drain-zombie carried right up until it turned out to be a real product bug. Updated in all six places that stated it, so they cannot drift: docs/quarantined-tests.md (owner, and the fullest statement), docs/testing.md, AGENTS.md's table cell, the @Quarantined javadoc, bin/check-quarantine-registry.sh's drift message, and QuarantinedAnnotationContractTest's assertion message. The #265 entry in test-untracked-ci-flakes.md, which reasoned FROM the old rule, is rewritten rather than find-replaced: it now qualifies on the rule itself instead of needing the exception. Also records the failure that prompted this, which turned out not to need quarantining at all: docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md. PCMetricsTest.metricsRegisterBinding asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter, while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. The observed 1214 vs 1207 is 214 completions against 207 contiguous - 7 records finished past a gap - and the gap is PERMANENT, because workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes. No timeout can close it. That is a wrong expectation, not a flake, and the note says so along with the fix (compare against PARTITION_HIGHEST_SEQUENTIAL_SUCCEEDED_OFFSET) and a warning not to widen the timeout instead. It is also NOT a confluentinc#857 sighting, and the note says why: every sighting in that family is a chaos/rebalance integration test with a commit mode and a seed, whereas this is a MockConsumer unit test with no broker, rebalance, revoke path or mode. bug-857-family.md already records one contamination of exactly this kind, so the ruling-out is written down rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we don't know the mechanism" and "we don't know whether it's ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - test-untracked-ci-flakes.md records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it's red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - that is the label the drain-zombie carried right up until it turned out to be a real product bug. Updated in all six places that stated it, so they cannot drift: docs/quarantined-tests.md (owner, and the fullest statement), docs/testing.md, AGENTS.md's table cell, the @Quarantined javadoc, bin/check-quarantine-registry.sh's drift message, and QuarantinedAnnotationContractTest's assertion message. The #265 entry in test-untracked-ci-flakes.md, which reasoned FROM the old rule, is rewritten rather than find-replaced: it now qualifies on the rule itself instead of needing the exception. Also records the failure that prompted this, which turned out not to need quarantining at all: docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md. PCMetricsTest.metricsRegisterBinding asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter, while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. The observed 1214 vs 1207 is 214 completions against 207 contiguous - 7 records finished past a gap - and the gap is PERMANENT, because workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes. No timeout can close it. That is a wrong expectation, not a flake, and the note says so along with the fix (compare against PARTITION_HIGHEST_SEQUENTIAL_SUCCEEDED_OFFSET) and a warning not to widen the timeout instead. It is also NOT a confluentinc#857 sighting, and the note says why: every sighting in that family is a chaos/rebalance integration test with a commit mode and a seed, whereas this is a MockConsumer unit test with no broker, rebalance, revoke path or mode. bug-857-family.md already records one contamination of exactly this kind, so the ruling-out is written down rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we don't know the mechanism" and "we don't know whether it's ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - test-untracked-ci-flakes.md records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it's red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - that is the label the drain-zombie carried right up until it turned out to be a real product bug. Updated in all six places that stated it, so they cannot drift: docs/quarantined-tests.md (owner, and the fullest statement), docs/testing.md, AGENTS.md's table cell, the @Quarantined javadoc, bin/check-quarantine-registry.sh's drift message, and QuarantinedAnnotationContractTest's assertion message. The #265 entry in test-untracked-ci-flakes.md, which reasoned FROM the old rule, is rewritten rather than find-replaced: it now qualifies on the rule itself instead of needing the exception. Also records the failure that prompted this, which turned out not to need quarantining at all: docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md. PCMetricsTest.metricsRegisterBinding asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter, while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. The observed 1214 vs 1207 is 214 completions against 207 contiguous - 7 records finished past a gap - and the gap is PERMANENT, because workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes. No timeout can close it. That is a wrong expectation, not a flake, and the note says so along with the fix (compare against PARTITION_HIGHEST_SEQUENTIAL_SUCCEEDED_OFFSET) and a warning not to widen the timeout instead. It is also NOT a confluentinc#857 sighting, and the note says why: every sighting in that family is a chaos/rebalance integration test with a commit mode and a seed, whereas this is a MockConsumer unit test with no broker, rebalance, revoke path or mode. bug-857-family.md already records one contamination of exactly this kind, so the ruling-out is written down rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we don't know the mechanism" and "we don't know whether it's ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - test-untracked-ci-flakes.md records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it's red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - that is the label the drain-zombie carried right up until it turned out to be a real product bug. Updated in all six places that stated it, so they cannot drift: docs/quarantined-tests.md (owner, and the fullest statement), docs/testing.md, AGENTS.md's table cell, the @Quarantined javadoc, bin/check-quarantine-registry.sh's drift message, and QuarantinedAnnotationContractTest's assertion message. The #265 entry in test-untracked-ci-flakes.md, which reasoned FROM the old rule, is rewritten rather than find-replaced: it now qualifies on the rule itself instead of needing the exception. Also records the failure that prompted this, which turned out not to need quarantining at all: docs/inflight/bug-pcmetrics-committed-offset-vs-completion-count.md. PCMetricsTest.metricsRegisterBinding asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter, while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. The observed 1214 vs 1207 is 214 completions against 207 contiguous - 7 records finished past a gap - and the gap is PERMANENT, because workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes. No timeout can close it. That is a wrong expectation, not a flake, and the note says so along with the fix (compare against PARTITION_HIGHEST_SEQUENTIAL_SUCCEEDED_OFFSET) and a warning not to widen the timeout instead. It is also NOT a confluentinc#857 sighting, and the note says why: every sighting in that family is a chaos/rebalance integration test with a commit mode and a seed, whereas this is a MockConsumer unit test with no broker, rebalance, revoke path or mode. bug-857-family.md already records one contamination of exactly this kind, so the ruling-out is written down rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 20, 2026
#265 settled the question this branch had left open in the flake ledger - measure the block correctly, or stop measuring it. It took the second answer, deleting BlockedThreadAsserter's wall-clock assertion for a causal one plus BlockedThreadAsserterTest, and it reached master first. So this takes master's helper wholesale and drops #262's armedAtNanos anchoring (517afd0). That is the better outcome rather than a concession: anchoring shrank the error and kept the helper sleeping out its own timeout, while removing the assertion ends the whole class of scheduler-jitter failure. test-untracked-ci-flakes.md now credits #265 for the fix; #262 still carries the re-enable, being the entry's owner. docs/quarantined-tests.md is the union of two rule-3 lifts. Each side read as deleting the other's entry - this branch had removed ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect, #265 had removed PCMetricsTest.metricsRegisterBinding - and both removals are correct, so neither survives. PCMetricsTest.java takes master's version; this branch never authored its content, which arrived via the rename commits into a region #265 rewrote. The C10 test needed a real repair, not just a merge. #265 moved assertUnblocksAfter's blocked function from the calling thread onto its own, and ReentrantReadWriteLock read holds are per-thread - so producingIsBlockedForTheDurationOfTheCommitAndResumesOnRelease was releasing a lock the test thread never held, and ensureProduceStarted threw "Need to call #beginProducing first". It now acquires and releases inside the blocked function, which strengthens it: reaching released == true proves finishProducing passed its own ensureProduceStarted check, so the lock was genuinely held by the thread that took it - something the cross-thread version never established. Its third argument also changed meaning underneath it, from "must block at least this long" to "return budget" at the same arity, so ofSeconds(1) would have become a 1s budget against a 20s default sized for PIT's instrumented JVM. Switched to the 2-arg overload. Pushed with three failures unexplained, which is why the handoff note lands with it: ParallelEoSStreamProcessorTest's executorThreadsInterruptedOnShutdownTimeout, inFlightMessagesCommittedIfProcessedDuringShutdown and processInKeyOrder, all on CommitMode[3], all around 10s. Neither of this branch's main-code changes fired - the produce-lock guard and the cleanUpContext report are both absent from the run log - and each test passes in isolation while a different set fails per run, which reads as contention. Not established: the decisive check of whether [3] fails the same way on plain master was not run. docs/inflight/handoff-262-master-merge.md carries that, the produce-callback decision reached but not acted on, and the #265 PCMetricsTest exposure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
PCMetricsTest carried one @Quarantined annotation and two registry entries, which bin/check-quarantine-registry.sh fails on and which blocked this repo's own pre-commit hook, not only CI. This was NOT master-state, and an earlier note in this branch wrongly said it was. origin/master has one entry against the same single annotation. The second arrived with this stack via f4a16a6, the cherry-pick of the re-quarantine from #116 (originally eb602ae) - a branch that did not carry master's entry, so it appended rather than folded, and nothing on either side could see the collision until the two met here. They were describing the same failure from two directions. Master's entry is the diagnosed mechanism: PARTITION_LAST_COMMITTED_OFFSET is asserted against a completion counter under UNORDERED, workers call latch.await() before counter.incrementAndGet(), so a latched worker's offset never completes and the gap is permanent. The cherry-pick's entry is what that looks like from outside - the metric behind and never converging, with a shortfall that varies run to run, which is why no wait closes it. Folded with nothing dropped, and the merge is asserted rather than assumed: every sighting, both failure transcripts, the #265 release history and why that fix addressed the opposite direction, the proposed comparand and why it is not a one-line change, the OffsetEncodingBackPressureTest cross-reference still worth ruling in or out as one phenomenon, and all four document links are present in the merged text. The entry keeps master's position and framing so the diff against master stays small. A note in the merged entry records that it was two entries until today and why, so a reader who remembers seeing two does not conclude one was silently lost. Also corrects the #334 handoff, which filed this red under "standing debt, pre-existing" - it was neither. bin/check-quarantine-registry.sh now exits 0.
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…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>
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…per test, qualified refs, retired links Three corpus repairs behind red gate lanes on this branch: - docs/quarantined-tests.md carried two live entries for PCMetricsTest.metricsRegisterBinding - the original mechanism diagnosis and the #265 re-quarantine - which the registry check rightly reads as drift. The re-quarantine entry is the current one; it now carries a pointer to the first diagnosis (which still stands, in bug-pcmetrics-committed-offset-vs-completion-count.md) and the superseded block is gone. History keeps the original. - docs/data/landing-page.yaml still linked docs/inflight/next-landing-page.md, retired by b0a5da3 when the owner dropped the workstream; both mentions are reworded inline, the same repair that commit applied to the six notes it caught. - Six inflight notes carried bare #NN refs on lines this stack adds vs master: each now names its repo (confluentinc#346/#390/#315/confluentinc#606, #51 - each resolved in both repos first), a quoted upstream issue body takes the line-scoped exempt marker, and two ordinals that were never issue refs at all ("problem 1", "hypothesis 6") are written as ordinals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HwDcZdfv1Ne4u3g6ThrG9
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
Merges `feats/ks-streams-reconciled` - the reconciliation of #395, #396, #398 and #391 on top of the refreshed #394 - into this branch, so that #271 can be retargeted onto that rung and its displayed diff collapse to the residue. This is the plan's "retarget move", `docs/plans/2026-08-31-001-process-god-branch-decomposition-plan.md`, and it is an ordinary merge: no history was rewritten and nothing was force-pushed. The pre-merge tip is preserved as `origin/backup/pre-stack-merge-271`. The merge also brings `master` forward, because the stack is current with it and this branch was not. That is most of the volume and all but a handful of the conflicts. ## Which side won, and the check that made it safe to say so The rule was: the rungs' refined versions win wherever both sides have a file; the spike's residue is kept. **The second half of that is only safe if the spike contributed nothing to the shared files, and that was measured rather than assumed.** Both trees were exported with package paths normalised (`io.confluent.parallelconsumer` and `bz.stub.parallelconsumer` folded to one token, and `io.confluent.csid.utils` to its new home) and compared file by file against the branch's own merge base. The answer is clean: outside `parallel-consumer-streams` and the streams example, **every difference this branch carries is the package rename itself plus a copyright-header line** - the same rename `master` performed independently, which is why taking the rungs' side loses nothing. The one substantive exception is the streams example's stock-baseline fixtures, which are already on the stack because #398 depends on them. - **Files on both sides: the rungs' version, 93 of them.** They carry current master, the rename, and every fix the five rungs landed. - **Files only on the spike: kept, 32 of them.** The plan and result documents, the branch handover, the ranked next-work notes, the eight `docs/solutions/` write-ups, and two streams tests the stack deliberately left behind - `HeadOfLineBlockingBenchmarkTest`, which #398 says it is leaving on the forest, and `ProcessorContextConfinementTest`. That residue is what this PR still exists to review. - **Files master deleted: they stay deleted, 12 of them** - `.semaphore/`, `.travis-archived.yml`, `service.yml`, two `.idea` run configurations, the uppercase `docs/` names master lowercased, and the `InternalRuntimeException` pair master renamed to `PCInternalRuntimeException`. - **Two shared ledgers merged as unions, not resolved to a side**: `docs/inflight/pr-strategy-doc-merge-triggers.md` and `docs/inflight/release-0.6.0.0.md` keep this branch's paragraphs alongside master's. **The strongest check is the one that makes a mis-paired rename impossible to hide.** Rather than counting recorded renames, the merged tree was differenced against `feats/ks-streams-reconciled` directly: the only differences are the 32 additions and the two union-merged ledgers. Nothing else survived on the wrong side, in the wrong module, or under an `io/confluent` path - `git ls-files | grep io/confluent` is empty, and the prescribed `grep -rnE 'io[\./]*conflu'` sweep returns only prose in dated documents. **Both sides were already renamed, which is not what the plan predicted.** It expected the spike to be pre-rename; `origin/feats/ks-on-pc-spike` had in fact moved five commits past the worktree's copy and carries the rename. That is the good case AGENTS.md describes - the mis-pairing surfaces as rename/rename conflicts on the right files instead of silently applying one module's edit to another - and it did: three `TestConventionsArchTest.java` files were offered paired across modules, and the right file was taken into each module rather than accepting git's pairing. ## One collision the merge produced, and the reason it is a good one `HeadOfLineBlockingBenchmarkTest` carried a private `sleep(Duration)` helper. Since #395 hoisted the identical helper onto `BrokerStreamsIntegrationTest` - because two arms that simulate cost differently are not comparable - the two now collide, and the compiler said so rather than either winning silently. The local copy is deleted and the shared one inherited, which is what the hoist was for. ## The gates, and the one that is a scoping artefact `bin/check-all.sh` was run in this worktree. Four gates failed on first pass and **three of the four did not exist on this branch before the merge** - they arrived with master, and every finding was on content that predates them. - **`check-inflight-tags`** flagged sixteen notes with no `inflight-type`, all of them this branch's own and all older than the tag scheme. Tagged - a first pass a reviewer should correct where it reads wrong. `bug-core-tests-jar-junit-parallelism-leak.md` additionally carries a `closed` state, because #265 removed the file it is about. - **`check-file-refs`** flagged citations broken by the rename - the exact class [`docs/citations.md`](docs/citations.md) says must be repaired rather than left. Forty-seven paths were re-pointed to their successors, **each verified to exist before the edit was written**; the claims around them are untouched, which is the line that document draws. What genuinely has no repo path - Apache Kafka's own sources inside the published jar, a placeholder in a shell recipe, a class that was only ever proposed - carries a line-scoped `file-refs: N/A` with its reason. Two targets that moved out of the tree point at the history holding them instead. - **`check-issue-refs`** flagged two bare `#NNN` in the handover; qualified. - **`check-branch-self-reference`** is the artefact, and the gate's own header names this exact situation: mid-merge the merge base is still the old one, so master's sentences read as this branch's. Twenty-five of the twenty-eight files it flagged are on `feats/ks-streams-reconciled` too and twenty-four are byte-identical to it, so marking them would be attesting to somebody else's notes - which that header explicitly says not to do. Committing the merge is the documented fix. ## Verified JDK 17, macOS. The whole reactor compiles, main and test. The reconciled branch it merges was verified in its own right before this: seam-off oracle unchanged, module unit suite green, and all 35 broker-backed integration arms green against a real broker.
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.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
#29 landed - the confluentinc#857 revoke-path deadlock, which this branch's records name repeatedly as the thing Phase B waits on. Two conflicts, both docs, both resolved as a union rather than a pick. **docs/quarantined-tests.md - union, and both halves matter.** Master's side gained `MultiInstanceRebalanceTest.largeNumberOfInstances`, quarantined with #29: a rebalance stall measured but not explained, unowned because no diagnosis exists. That entry is master-state and stays. This branch's rule-3 lift of `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` still applies, so that one comes out - #265 deleted the wall-clock assertion it flaked on, and #262 owns the re-enable. Taking either side wholesale would have lost one of the two. Registry check confirms 1 entry, method-granularity. **docs/inflight/pr-blockers-and-collisions.md - took master's.** Its rewrite of the `master-confluent` retarget bullet says the same fact better and is already post-merge attested. **The merge also falsified claims elsewhere, which is the part a conflict list does not show.** Both live registers this branch owns asserted #29 was unmerged and that Phase B was blocked behind it. Corrected in `branch-transactional-battle-test.md` and `pr-blockers-and-collisions.md`: Phase B's blocker is discharged and the work is startable. Both corrections carry the caveat the commit body supplies, so the next reader does not over-claim what the deferral bought: **the fix closes the AB-BA cycle only in `PERIODIC_CONSUMER_SYNC`**, because the other commit modes never block on the commit path and the second edge does not exist there. The transactional mode this suite covers was therefore never exposed to that edge - so waiting for #29 protected Phase B's SLO baseline from a defect that would not have appeared in it anyway. Worth knowing before anyone treats the deferral as having been load-bearing. Deliberately NOT swept: `docs/inflight/test-857-parallel-integration-proof.md` carries `inflight-state: deferred - cannot start until #29 lands`, which is now satisfied. It is master's note for someone else's work, so unblocking it is theirs to do rather than something to widen this diff with. Dated records under `docs/plans/` that say #29 is "still open" are left alone on purpose - docs/citations.md permits repairing an address, never rewriting a claim, and those claims were true when written. Verified: full-reactor `test-compile` clean, `bin/check-all.sh` 15 passed / 0 failed, quarantine registry consistent.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
#262's transactional-guarantee register and its two produce-lock guards. Three core main files - PollContextInternal, ProducerManager, AbstractParallelEoSStreamProcessor - and none of them is on this branch's path: this branch changes offset DECODING, and the produce lock is a commit-path concern with no offset metadata anywhere near it. THE CONFLICT, AND WHY NEITHER SIDE COULD JUST BE TAKEN test-untracked-ci-flakes.md, both sides having edited the same table. Each side was stale in a different row, so taking either wholesale would have lost something real: - ProducerManagerTest: MASTER's row wins outright and this branch's is deleted. Ours still said "quarantined, owner #262"; master's records that #265 fixed it by deleting the wall-clock assertion and that #262, as owner, LIFTED the quarantine and deleted the registry entry. The quarantine lane report on this PR was still printing the old state, which is exactly the kind of stale-but-plausible signal this merge corrects. - processInKeyOrder: OURS wins. Master still carries the two-sighting version; this branch merged master's two with its own four an hour earlier, so the six-sighting row across two branches is the superset. - MdcContextPropagationTest: new from master, kept. Its own note owns the diagnosis. - JStream, the PIT lane and ManagedPCInstanceLifecycleTest are this branch's and master has never seen them. Composed rather than resolved by side, which is the only way a shared ledger survives two writers. Verified: unit lane BUILD SUCCESS across all eleven modules, 696 tests over the four library modules, 0 failures. bin/check-all.sh 15/15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
One conflict, in docs/inflight/test-untracked-ci-flakes.md: master rewrote the ProducerManagerTest row (#265 fixed it, #262 lifted the quarantine) on the line above the simpleBatchTest row this branch rewrote for its fifth sighting. Both edits kept; the stray blank line master removed stays removed. Resolved by editing the markers, and the removals in the file were read against both parents - each is the other side's own change. master's edit to .github/workflows/maven.yml (a comment block on the Lincheck lane) auto-merged in a region this branch does not touch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
bin/check-branch-self-reference.sh asks the merging branch to attest to any note that names it, at the one moment fixing it is cheap. This note named `optimize/unit-gate` and asked whether its measurements were taken against an "off" parallelism arm that was not actually off. Answered rather than deferred, because the answer is checkable and the note is about to outlive the branch it names: the arm WAS affected. The leak was closed by #265, which deletes core's junit-platform.properties from the tests jar, and every wall-clock figure this branch reported predates it. Those figures now describe a gate that no longer exists. What survives is the change itself. runOrder=balanced is a sort, and the fork-ordering statistics are regenerated against the merged tree, so the mechanism and the shipped data are unaffected - only the reported seconds need re-taking. The sibling branch named in the note, ci/reenable-parallel-tests, is still unchecked and stays open here. The branch reference is replaced with the PR number, since the branch stops existing when this lands and a branch name is not a link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
Review finding on #105. The write-up's upkeep caveat only covered one direction - a newly added class is unranked and sorts last - and the reviewer pointed out that the failure mode actually in front of us is the opposite one, which is worse because it leaves nothing to see. Adding a class at least puts something unranked on disk. A class that gets much FASTER, or disappears, changes nothing observable: surefire's runOrder=balanced keeps sorting, by recorded costs that describe a suite which no longer exists, and the fork packing decays toward the unordered scan it replaced. No build goes red, because class ordering is not something an assertion can be wrong about. The live instance is core's file, ranked around RunLengthEncoderTest at roughly three times the next-slowest entry. #106 collapses exactly that class, so landing it without regenerating the statistics would quietly hand most of this change back. That obligation is recorded in docs/inflight/pr-blockers-and-collisions.md - the register a session actually inherits - rather than only in a PR body that stops existing at merge, and it names the remedy: a full bin/ci-unit-test.sh run on core, committing the rewritten files. Also states plainly that this change and #106 OVERLAP rather than sum. Packing removes the idle fork at the end of the run; #106 removes the work that made that fork long. Once the work is gone the tail is a fraction of its size, so packing keeps helping by proportionally less against whatever class is next in line. Neither makes the other pointless - a class scheduled last strands a fork for its own length, whatever that length is - but adding their gains together would double-count, and a reader of either PR alone would be inclined to. No wall-clock figure is added, restated or defended here: the PR's headline seconds remain withdrawn pending re-measurement against a control arm that #265 uncontaminated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…w read them per line Corrects a claim ee9a3b2 carried in from the automated review on #105. That review compared the wrong unit and the error survived into both files that commit touched, so it is fixed here in the same terms it was made. THE MISTAKE, BECAUSE IT IS THE REUSABLE PART. Each line of a .surefire-pc-unit-times file is one test METHOD, and a heavy class is spread over many of them. surefire's runOrder=balanced sorts CLASSES - pom.xml's own comment beside the setting says so, and forking cannot split a class, so the class is the scheduling unit either way. Ranking the raw lines therefore compares one method of one class against a whole other class, and it badly overstates how far the top class stands above the next. The review read core's file that way, put RunLengthEncoderTest at roughly three times the next entry, and named a METHOD as the class that would define the tail after #106. Aggregated per class, the top two are close rather than a factor apart, and the third is well behind: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \ parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head -5 Both files now state the shape and give that command rather than baking in a ratio, which would be wrong at the next stats refresh anyway. WHAT IT CHANGES ABOUT THE ARGUMENT, IN BOTH DIRECTIONS. The staleness risk is real and stays - an ordering built around a class that has since collapsed decays silently, with no red build - but it is materially smaller than the review stated. It also cuts the other way, in this change's favour: because forking cannot split a class, #106 moves the floor down to the second-heaviest class and no further, and that class is substantial. So the residual tail is most of the present one and packing keeps most of its value rather than being largely obsoleted. The "overlap, not sum" paragraph stays correct and stays put; only the implication that the residual tail is small is removed. No figure is asserted for #106's own collapse. The review took "62.3s -> 0.2s" from that branch's commit message, but that measurement predates #265's contaminated-arm fix and disagrees with both the committed statistics and a run on the merged tree, so the class is described as collapsing by an order of magnitude and the re-measurement now in flight can supply the number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…oders are active (#106) perf(offsets): stop walking every offset when only distance-based encoders are active OffsetSimultaneousEncoder.invoke() visited every offset between the committed base offset and the highest succeeded offset. When that range spans a large gap - up to ~2.1 billion - it is a multi-second scan on the commit path, and it is exactly the case where no BitSetEncoder can be constructed, leaving only RunLength. It now visits only the offsets where the completion state changes. This resolves the encoder's own long-standing TODO about large ranges being slow. WHY IT IS SAFE. RunLengthEncoder accumulates the open run by the DELTA to the previously seen offset, not by counting calls. For a maximal run [a,b], visiting a opens the run and visiting b adds b-a, giving the identical length to visiting every offset; extra visits inside the run telescope harmlessly. So invoke() needs only each run's first and last offset, and because a state change can only occur at an incomplete offset or immediately beside one, the visit set is {0, length-1} plus each in-range incomplete and its two neighbours. It is sized by the NUMBER OF INCOMPLETES, never by the range width. The full scan is retained whenever any active encoder requires every offset (BitSet, ByteBuffer), expressed as OffsetEncoder.requiresEveryOffset() and defaulting to the safe answer, so a future encoder opts into sparse deliberately rather than inheriting it by accident. REVIEW FOUND TWO DEFECTS IN THE FIRST VERSION, AND BOTH ARE FIXED. Cross-model review (Codex, confirmed independently by a Claude pass) found two bugs in the hand-written in-range filter. Both break the same premise, which is what makes them correctness bugs rather than edge cases: the boundary proof holds only if the visit set contains EVERY in-range incomplete - miss one and the run boundary beside it is never visited, the run is absorbed into its neighbour, and the encoding silently diverges with no exception raised. - The exclusive end offset is highestSucceededOffset + 1, which wraps to Long.MIN_VALUE at Long.MAX_VALUE, dropping every incomplete and encoding them as complete. The bound is now the inclusive last offset, which reconstructs highestSucceededOffset and cannot wrap. - The early exit assumed ascending iteration, but the incompletes set is caller-supplied on a public constructor whose field javadoc promises no order requirement. It is removed rather than repaired: it could never fire in production, because encodeOffsetsCompressed samples the high-water mark once and derives both the set and this range top from it. Both are latent rather than reachable by a running deployment - the first needs a partition at ~9.2 quintillion offsets, and the only production supplier of the set collects into a natural-order TreeSet. Note SortedSet#subSet, which the hand-written loop replaced, WOULD have honoured the set's comparator; avoiding subSet for its own good reason must not import an ordering assumption it never had. The loop carries that reasoning so it is not undone. VERIFICATION. A differential test runs both strategies over 130 generated scenarios at a fixed seed and asserts the encodings are byte-for-byte identical, that the key sets match, and that the sparse path was genuinely taken. Two further regression tests cover the shapes above, and both were proven by sabotage rather than assumed: with the old loop restored each fails with its predicted symptom - an empty encoding map, and a byte mismatch against the full scan - while all 130 pre-existing scenarios still pass, which is the direct evidence that the differential test could never have caught either. No existing test is weakened, skipped or excluded. RunLengthEncoderTest changes are javadoc only; its assertions are untouched. PERFORMANCE. RunLengthEncoderTest collapses by an order of magnitude, from tens of seconds to a fraction of one. Deliberately no gate-level figure: the numbers this work originally quoted were taken before #265 fixed a leak that made dependent modules run thread-parallel regardless of their own setting, so they describe a CI gate that no longer exists. The class-level win is a change in algorithmic complexity and stands without a benchmark. Reproduce with ./mvnw -Pci -pl parallel-consumer-core -am test. FOLLOW-UP OWED AFTER THIS MERGES. This collapses the runtime of the class that #105's checked-in .surefire-pc-unit-times ordering is built around, so those statistics must be regenerated with a full bin/ci-unit-test.sh or that PR's test packing decays silently, with no red build to say so. IT ALSO CARRIES ONE DOCUMENTATION LEARNING, deliberately rather than as its own PR. Bringing this branch across the package rename produced rename/rename conflicts on the near-identical per-module arch tests, and resolving them with `git checkout --theirs` staged files still containing conflict markers. The write-up added under docs/solutions/workflow-issues/ records the mechanism: --theirs fails loudly there (stages 2 and 3 sit at different paths, so the path you name has no stage 3), and the silent step is the `git add` after it, which stages whatever the merge left in the worktree and leaves `git ls-files -u` clean. A batched invocation aborts on the first bad path, so ordinary conflicts sharing that command line are left unresolved too. The rule that generalises - resolve by explicit ref and check the command's status, not the look of the tree - was already in use in this repo but had never been written down. Its first draft got the mechanism backwards and is corrected in the second commit, which is why both are here. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…only became measurable after #106 (#105) test(ci): fork-tail packing measured and rejected - and why the gate only became measurable after #106 This branch set out to cut wall time off the required Unit Tests gate by packing the surefire fork tail: run the slowest test classes first so no fork idles at the end. The mechanism was built, the plumbing that makes it work on CI was found, and it was then measured properly and REJECTED. What lands is documentation - a null result and the findings around it. No Java, no pom.xml change, no checked-in data. WHY IT LOOKED RIGHT. Surefire's forks pull classes from one shared queue and the run ends when the last fork finishes, so scheduling the longest job first is textbook longest-processing-time packing. Surefire 3.5.5's runOrderStatisticsFileChecksum made it usable on CI for the first time by pinning the statistics filename, so the recorded runtimes could be committed and survive a cold checkout. WHY IT COULD NOT BE JUDGED UNTIL NOW, which is the part worth keeping. The packing was built around one un-splittable class that dominated both the gate's wall time and its variance. Measured with that class intact, the ordering was being judged in exactly the arrangement this repo's integration-lane write-up says buys nothing for ANY arrangement - the dominant class still setting the floor. #106 collapsed that class; only then could the ordering be regenerated on the merged tree and the two compared. Shrink the dominant class first, re-derive, then measure. THE RESULT. Two arms sharing that base, statistics regenerated on the very tree under test, interleaved, three repetitions each, in a container capped to the runner's core count: the packing arm was slower in every paired repetition, by a few seconds at the median. A small consistent loss rather than no measurable effect. The write-up states the shape and the reproduce recipe rather than the numbers, which go stale on the next run. THE FINDING THAT OUTLIVES IT. #106 did not only make the gate faster, it made the gate MEASURABLE. Repeat runs of identical code used to differ by up to a minute and a half; across these runs the spread was seconds. The variance everyone had been fighting was that one class landing in different fork slots. That is why a difference of a few seconds resolves now where ninety could not before, and it changes what any future measurement on this gate is worth. ALSO HERE. Maven -T module parallelism loses on a 2-core gate, measured, and starves wait-heavy tests into awaitility timeouts - pick one parallelism axis per core budget. A benchmark harness must not mutate its own input, since surefire rewrites the ordering statistics in place and a run then inherits the previous run's fork placement. A codecov sighting where a PR containing no Java, compared against a base that is a proven git ancestor of its head, was still reported as carrying more files and more lines than that base - recorded with its evidence and explicit about which half of the existing note it does not test. And an AGENTS.md worktree rule: name the tree on every commit, because a bare git commit resolves against the session root, which is some other worktree. The write-up keeps its own pre-measurement prediction, marked WRONG and left visible: it argued packing would keep most of its value once the dominant class shrank. The per-class arithmetic behind that was right and the conclusion still did not follow. A mechanism that is real is not the same as a mechanism that pays, and only a measurement tells the two apart. The wall-clock figures this work originally quoted are withdrawn rather than corrected: they predate #265 closing a leak that made dependent modules run thread-parallel regardless of their own setting, so they describe a gate that no longer exists. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…a week ago (#490) Working docs/inflight/test-untracked-ci-flakes.md, undiagnosed row first. Four rows retired, one new instance found and fixed, four left open with the reason stated in each. THE UNDIAGNOSED ROW WAS NOT UNDIAGNOSED, IT WAS STALE processInKeyOrder failing its own `[sanity check input data]` has a mechanism, a fix, and a measurement - all of them on master since 2026-09-02. The wait in front of that assertion counted CONTROL loop cycles, while `polled` is filled by the POLL thread from a doAnswer on consumerSpy.poll; nothing orders the two, so the loop can turn before the poll thread has been scheduled and the list is empty. All-or-nothing, because MockConsumer hands over the whole batch at once. #29 replaced the cycle count with a wait on the data (`551f40011`, merged as `a6941020f`), measured matched-pair at 5 failures in 31 versus 0 in 31. The provenance check is what settles the row rather than another measurement: EVERY recorded sighting predates that commit reaching master, including the one whose control arm reddened "unmodified master" - that arm was at `b2e6c190d`, five days before the fix landed. There is no sighting on a tree carrying it. A second, branch-side ledger of this same failure exists on refs that never merged (`test-processinkeyorder-sanity-check-races-the-first-poll.md`, findable with `node bin/inflight.mjs prior-art`), which is why the working tree looked like it had less evidence than the corpus held. A NEW INSTANCE OF THE SAME CLASS, CAUGHT BY THE CONTROL ARM `551f40011` named the other assert-after-N-cycles waits and left them as master-side hardening tracked in this register. Running the control arm for the above - master with only that await removed - reddened one of them: inFlightMessagesCommittedIfProcessedDuringShutdown, whose `awaitForSomeLoopCycles(2)` stands in for "the record is in flight". Under load close() can find nothing to complete and the commit assertion reads []. It now waits on the user function actually being entered, which is a STRONGER precondition, not a looser deadline: a [] after it means close() genuinely failed to complete in-flight work, which is a product signal. AMBIENTPROBEEXTENSIONTEST - DIAGNOSED IN THE REGISTER, NOW FIXED AND PROVEN LogCapture attaches to a process-global logger, and three headroom methods each assert the exact set of lines they saw while running concurrently, so each captures its siblings'. Control: master, class alone, red 4 of 4. Treatment: same commit plus three @ResourceLock annotations, green 6 of 6. Sabotage-checked per docs/testing-at-write-time.md - deleting reportDeadlineHeadroom's PASSED call from AmbientProbeExtension.testSuccessful reds exactly headroomIsReportedOnAPassingTestToo, so the lock isolates without blunting. A lock rather than @execution(SAME_THREAD) on the class: the two other classes with this shape capture in most of their methods, this one in three of many. The class already used that idiom for ENVIRONMENT_DUMP_LOCK. Never red on CI because the ci profile sets parallel-tests=false, so only local runs were exposed - which is worse, since local runs are what a developer reads. DEFECT-CLASS SWEEP - two classes, candidates named Two test methods capturing the same logger concurrently: every LogCapture.of call site grouped by (test class, captured logger). Only this class was exposed. Dismissed with reasons: ConsumerOffsetCommitterAsyncFailureLoggingTest and LoadFactorCeilingReportingTest (both @execution(SAME_THREAD)); RevokeOnTheControlThreadTest, UserFunctionFailureLoggingTest, RemovedPartitionStateTest and this class's own ProgressProbe capture (one capturing method each). Cross-class collision is excluded by mode.classes.default being JUnit's same_thread. An assertion gated on a cycle count from another thread: both assertThat(polled) sites are awaited; queuedMessagesNotProcessed... was fixed by #101; executorThreadsInterruptedOnShutdownTimeout waits on the latch the user function holds, not on a cycle count, and is left alone. TWO ROWS RETIRED THAT THEIR OWNERS SHOULD HAVE RETIRED The JStream row said outright that its mechanism was #116's and that whoever merged it owned the retirement; #116 merged on 2026-09-03 and JStreamLiveResultStreamTest is in the tree. The ProducerManagerTest row was describing finished work - #265 and #262 both merged. WHAT STAYS OPEN, AND WHY The PIT lane, rapidToggleShouldNotCreateDuplicateInstances, ReactorPCTest and CommitResponseTimeoutSymptomTest keep their rows and their stated reasons. Nothing is quarantined: docs/quarantined-tests.md is empty and none of these has the rate or the diagnosis rule 1 wants. Two new local sightings from this pass's own runs are recorded as their own row before the logs expire - queuedMessagesNotProcessedOrCommittedIfSubmittedDuring- Shutdown and closeAfterSingleMessageShouldBeEventBasedFast, one sighting each, which is a sighting and not a rate. THE SABOTAGE TOOK FOUR ATTEMPTS, AND THE FIRST THREE WERE FALSE NEGATIVES The shutdown fix in the previous commit needed the mandatory break-it-first check from docs/testing-at-write-time.md, and the first THREE main-code mutations left inFlightMessagesCommittedIfProcessedDuringShutdown green. Taken at face value that reads as "this test asserts nothing about committing". It does not - the mutations had not reached the commit: 1. closeDrainFirst() -> DONT_DRAIN. Never ran: the test calls close(), which IS closeDontDrainFirst() already, and DONT_DRAIN finishes in-flight work anyway. 2. The close-sequence call to commitOffsetsThatAreReady() removed. Green. 3. commitOffsetsThatAreReady() neutered outright. Green - and this one printed a marker proving it ran, so the offset this test asserts is not committed by that method at all. 4. AbstractOffsetCommitter#retrieveOffsetsAndCommit's call to commitOffsets removed. RED on all three parameters, on the assertion under test. The marker is the whole point. Without it, three green sabotages would have made a strong and wrong case that the test was dark, and I would have recorded a defect that does not exist. The honest reading of a green sabotage is "I have not yet shown the test still guards its property", never "the property is unguarded". Recorded in the solution doc rather than in a commit nobody greps, with the attempts that failed kept rather than dropped - a reader who tries mutation 2 or 3 on this test will otherwise reach the same wrong conclusion. The register's entry now points at that section before anyone touches the test. Also adds the matched-pair number for the shutdown fix itself: red twice in fourteen class runs under heavy load on the unfixed arm, none in fourteen on the fixed one, the fix being the only difference between the two trees. Small n, said so - the fix rests on the mechanism and on mutation 4, not on that count. THE NEW SIGHTINGS ROW WAS OUTSIDE THE TABLE IT WAS ADDED TO A blank line sat between the last existing row and the row added above for the two shutdown-path sightings, so GitHub rendered it as a separate one-row table with the sighting text as its header: present in the file, absent from the register a reader sees. Nothing checks markdown table continuity, and the added row looks correct in a diff, which is how it survived a review. That row's reason for recording rather than diagnosing also said "the seeds die with the logs", borrowed from the chaos suite; these are MockConsumer unit tests with no seed, and what expires is the local run's own output. VERIFIED AFTER THE MERGE, WITH THE FAILING CONDITION ACTUALLY PRESENT Both fixed tests were looped on the merged tree - AmbientProbeExtensionTest class alone and ParallelEoSStreamProcessorTest#inFlightMessagesCommittedIfProcessed- DuringShutdown - twelve alternating rounds before the master merge and ten after, green every time on both. Run WITHOUT -Pci on purpose: the `ci` profile sets parallel-tests=false, so under it JUnit thread parallelism is off and the AmbientProbe defect cannot occur - a green there would have proven nothing. The surefire reports were read rather than the exit codes trusted, confirming each round reached 28 methods and 3 parameters instead of no-opping. The class-scope SpotBugs finding attributed to this PR's diff was checked rather than assumed inherited: its annotation spans lines 67 to 1729, the whole class, and its cause is a `org.mockito.internal` static import written in 2021. The same finding fires on JStreamParallelEoSStreamProcessorTest, which this branch does not touch. Left alone. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
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.
Depends on #264
Description
Two defects found while running Apache Kafka's own Streams test suite against a patched build
(#255), plus the sweep that followed from the second one. Both defects make
the test suite mean something other than what it says.
1. The core tests jar was configuring every downstream module's JUnit runner
src/test/resources/junit-platform.propertiesis packaged at the root of the core tests jar.Eight modules depend on that jar - the four integrations plus every example - and JUnit reads the
first
junit-platform.propertieson the classpath. So all eight silently ran withparallel.enabledanddynamic.factor=20baked in at core's build time. Their own setting wasnever consulted, and there was no way to opt out.
That inverts the root pom's intent: the
ciprofile setsparallel-tests=falsedeliberately,because JUnit thread parallelism contends on static state and CI parallelises by forking JVMs
instead. A locally-built jar bakes
true, so a downstream module could inheritenabled=true, factor=20exactly where the project had decided it did not want it.docs/solutions/test-flakiness/unit-tests-parallelise-by-forking-not-threadinghad already measuredthis - fork x12 is 1:38 and reliable, thread x20 is ~2:32 and intermittent. So this was not merely
"modules inherited a setting": it forced eight modules into the configuration this repo had measured
as unreliable and deliberately switched off. It surfaced as 159 of 188 Kafka tests erroring on
shared state.
Moved to surefire/failsafe
configurationParametersin core's pom. As build config it is per-moduleby construction: each module evaluates its own
${parallel-tests}, nothing is packaged, and itoutranks a properties file if one is ever reintroduced. Core keeps its parallelism - measured 102
worker threads before and 103 after. The eight downstream modules revert to the JUnit default, which
is what they would always have had.
2.
BlockedThreadAsserterasserted on a clock instead of on causalityassertUnblocksAfterscheduled the unblocker on a timer, then asserted the blocked function'swall-clock elapsed time was at least that long. It failed in CI at
19.985s >= 20s- a 15msscheduler wobble that says nothing about the behaviour under test - and it deliberately slept 20s to
do it. The property it wants is an ordering fact, so it is now asserted as one: both events take a
tick from a shared monotonic sequence, and the return must come after the unblock.
ProducerManagerTestgoes from 23.06s to 3.32s.That rewrite was itself defective, and the fix is in this PR. A six-reviewer pass found two P1s;
four reviewers found the first independently, and one reproduced it. The entered-latch was counted
down before the function ran, so it meant "thread started", not "reached the blocking call" - the
main thread usually won the race to tick, and a function that never blocked passed the new ordering
assertion 177 times in 200. The old wall-clock version was flaky by 15ms; the replacement was
unsound by 88%. The second P1, found only by the adversarial lens: a blocked function that threw
was scored as a pass.
Both fixed - the function is now proven still parked before the unblocker ticks, and its throwable is
captured and asserted null.
BlockedThreadAsserterTestis added as the negative control AGENTS.mdrequires and that neither previous commit had. Building it refuted the obvious design: with only a
"returns immediately" case, removing the guard failed just 10 of 20 runs. The deterministic case is a
function returning on its own timer after the unblocker ticks, which the ordering check provably
cannot catch. Guard removed: red 20/20. Guard restored: green 20/20.
3. The sweep: sleeps that stood in for events
Defect 2 named a class, so the class was searched rather than the symptom. Every
Thread.sleepundersrc/testandsrc/test-integrationacross all modules was surveyed against one rule: a sleep thatwaits and hopes a thing happened before asserting is a correctness sleep and goes; a sleep that is
the behaviour or the scenario stays.
Eight removed, across seven files. The clearest two:
ShardManagerTestslept 10ms of wall clockthree times purely to advance a mock clock;
CloseAndOpenOffsetTestslept 500ms for a commit withits own comment saying "sleep is lazy but much much simpler" and a TODO for an event - the event
already existed, since
dirtyis set on success and cleared by the commit, so!isDirty()iscommit-happened.
Three correctness-adjacent sleeps deliberately kept, because there the elapsed time is the
instrument rather than a guess: a poll-rate assertion needs a window, "still paused after 1s" is a
negative property, and a 3s soak gives the invariant sampler many chances to catch a violation.
Everything else is fixture. Notably
OffsetCommittingSanityTest'sJUST_SLEEParm is a deliberatelyunverified dwell - awaiting there would collapse it into
CHECK_CONSUMEDand silently delete theboundary case.
One behavioural finding: the SASL poll retry back-off is 5s, so the old
sleep(5000)landedclose()mid-back-off by luck rather than design. Logs confirmclose()breaks the back-off viacloseInProgressSignalin ~90ms.No assertion was deleted, nothing disabled, no coverage lost - two tests gained assertions
(
RetriesTestnow verifies the retried half actually succeeds, which nothing did before).Verification
new negative-control class was added).
Checklist
docs/SELF_HOSTED_RUNNER.md,docs/TODO_INDEX.md, and the inflight notereduced to the one still-open item (
branch-parallel-measurement-contamination.md)BlockedThreadAsserterTestadded as a negative control; 7 test fileschanged; 2 gained assertions
docs/SELF_HOSTED_RUNNER.mdis touchedonly to keep its gate green; no workflow or runner configuration changes