Repository navigation
test(core): act on the inactive-test audit - the tests that did not run, assert or exist now do - #264
Conversation
Answers four questions with per-test evidence: 5 tests are @disabled (not the 7 a raw grep returns - one hit is javadoc, one is a @DisabledOnOs platform guard), 1 has an empty body, 4 are placeholders, and 4 more were deleted rather than implemented by confluentinc#493 while the branch that would have written them never merged. Only 1 of the 5 disabled tests records why. The two core ones were both disabled by c1fefbc "Create and commit offset map" in 2020 and have been dark since; the real gap is the end-to-end per-CommitMode assertion that offset commits respect key-order blocking. Also records two categories nobody asked about because an annotation grep cannot see them: 15 of 289 tests assert nothing, and OffsetEncodingTests reports green with most of its assertions branched away by a helper named assumeWorkingCodec that is not an assumption. Absorbs docs/test-hardening/disabled-and-weakened-tests-audit-2026-04-22.md, which existed only on the unmerged refactor/test-hardening branch, and corrects the two reasons its own git history refutes. Findings are keyed by class and method rather than line number - that predecessor's line numbers had drifted ~22 lines in four months. Records only. No test behaviour, assertion, timeout or volume is changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
A 455-line audit of disabled, kneecapped and weakened tests sat unread on
refactor/test-hardening for four months. Not because it was wrong - because
docs/refactoring.md described that branch by its OTHER commit ("OOM diagnostics
for LargeVolumeInMemoryTests at 1M") and never mentioned the audit, while
docs/inflight/branch-stale-and-diagnostic.md filed the branch under Superseded,
i.e. safe to delete. The only copy of unique content was on the delete list,
indexed under a description that did not describe it.
Both entries now say what the branch actually holds and where the content went.
The branch is genuinely safe to delete once its 1M/OOM commits are salvaged.
Also corrects the origin/refactor/empty-tests entry: its removal half landed on
master via confluentinc#493, so only the implement half is still open, and the
four tests it would restore are now named.
Adds the deferred work the audit found to the refactoring backlog - the
@timeout(60000L) unit bug, the assumeWorkingCodec misnomer, the JUnit 4
Assume on the Jupiter classpath, and the key-order commit coverage gap - plus
an AGENTS.md pointer so the next agent finds docs/test-hardening/ without
knowing it exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
An independent falsification pass against the audit found the evidentiary spine sound - every git attribution survived, including the two hardest - but the counting was not publishable. Corrected here: - confluentinc#493 deleted TEN tests, not four. The six omitted were the most substantive intent in the commit: dead-letter queue, retry ordering, and in-flight backpressure. Also names confluentinc#494, the second follow-up that never merged, and records that only 3 of the 10 ever got NotImplementedException stubs on the restore branch. - Denominator was 289; it is 292 (301 with @archtest). @CartesianTest contributes 3 methods in TransactionAndCommitModeTest and is meta-annotated @testtemplate, so "zero @testtemplate" was true only of the literal string. - LargeVolumeInMemoryTests was recorded as fixed by PR #49. It is NOT fixed: master still runs 500 messages with 1_000_000 commented out, and PR #49 never touched the file. The predecessor audit carried a caveat to re-check this before flagging, and dropping that caveat is how an open defect became a closed one. This was the worst error in the document. - Quarantine script subclasses hold 27 tests, not 29; assumeWorkingCodec has 5 call sites, not 6; assumption aborts are 5 sites, not 6; the example modules have 6 test methods, not 9. - 9.4 claimed ~15s of unfailable sleep; ProgressBarTest is @disabled and costs nothing, so it is ~5s. - 8.5 refuted a "Short superseded Long" claim the predecessor never made - that was a hypothesis raised during investigation, now labelled as such. Three of the wrong counts came from one habit: counting raw grep hits instead of enumerating what matched, which is the exact error the document opens by warning about. The reproduction command has been replaced with one that enumerates annotation shapes rather than trusting a hard-coded whitelist. Also stops underselling existing key-order coverage: TransactionAndCommitModeTest runs the full CommitMode x ProcessingOrder cross-product including KEY. The central gap claim survives anyway - those tests assert counts, not committed offsets, and run on one partition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
…osition Three research passes changed what the audit says. The two core tests disabled in 2020 are no longer "no reason recorded". An abandoned branch, bugs/turn-on-commit-tests @009bb7122, deletes exactly those two @disabled lines and says why: "Turn back on offset commit tests which were dibbled when the offset map feature was added". Same author, contemporaneous. The annotations still carry no message, so 4 of 5 remain undocumented - but the cause is now evidenced rather than inferred, and somebody had already tried the cheap un-disable and abandoned it at WIP. Section 4 is rewritten. Of the ten tests confluentinc#493 deleted, only ONE is still worth writing: userSucceedsButProduceToBrokerFails. Five were effectively written under other names, each now cited. Three describe features the library has never had - there is no dead-letter queue, no per-partition limit and no per-topic limit anywhere in main source - so they are issue references, not tests. maxPerPartition and maxPerTopic were written as a trio with maxOverall, and only the global scope ever shipped; it is WorkManagerTest.maxInFlight today. The last, truncationOnCommit, is obsolete by construction. Four claims inherited from the 2026-04-22 predecessor were wrong and are fixed: the @timeout unit bug is already solved by open #206 (and its 120s beats the 60s this audit recommended, which would have raced two 45s/50s Awaitility budgets); 30_000_00 is a reduction from 10M, not a 30M typo; VeryLargeMessage- VolumeTest was never kneecapped; and roundsAllowed masks nothing - it is dead. LoadTest is untagged, so it runs in the gating lane rather than being excluded. Salvages the LargeVolumeInMemoryTests OOM diagnostics, which existed only on a branch marked safe-to-delete and are the sole measured evidence that restoring 1M exhausts the heap in the close path. Corrects PartitionStateManager.onOffsetCommitSuccess's javadoc, which still described discarding tracked offsets below the committed offset. Nothing is discarded, and nothing below the committed offset is tracked to discard. Javadoc only - no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
…d anything - VertxTest.handleHttpResponseCodes: body was assertThat(true).isFalse() behind @disabled. Born disabled in 2020, never ran green, could not have. Its @disabled import goes with it - it was the only user. - StreamTest.test: @test commented out over an intact infinite Stream.generate printer. The class keeps testStreamSpliterators. - SampleTestingFailsafePluginInclusionCore: an empty {} body. It was not proving failsafe picks up src/test-integration - that source root is added declaratively in the root pom by build-helper, and failsafe selects by package pattern, which 28 other classes in that package already satisfy. - JavaEnvTest: one log.error, no assertions, javadoc conceding it is for manual inspection. Deleting it orphans StringTestUtils, whose only method it was the only caller of, so that goes too. Core unit suite: 320 tests, 0 failures. All four are absent from the surefire reports afterwards and no other count moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
Every volume the audit flagged turned out to be already correct, a deliberate increase misread as a reduction, or gated by a wait that would have to move with it. So this removes the dead alternatives and records why the live values stay. - MultiInstanceHighVolumeTest: 30_000_00 rewritten as 3_000_000. Byte-identical at runtime - the digits were always three million. History shows it was REDUCED from 10_000_000, so the commented 10M was the superseded value, not the intent; that line goes. Raising it is blocked by a hard-coded 60s wait in the same method anyway. - VeryLargeMessageVolumeTest: drops a stale // 2_000_000. The live value never moved - it went 1_000_000 to 100_0000, the same number, to 1_000_000. - LoadTest: drops four dead alternatives, keeps 4_000, and records why it must not be raised casually - it is untagged so it runs in the gating lane, and it is a listed member of the load-tightness flake family at this volume. - TransactionAndCommitModeTest: drops the dead numThreads alternatives and the roundsAllowed variable, which was assigned and never read. ProgressTracker is built with null rounds and rejects being given both rounds and a timeout, so that mechanism was structurally off. The design question its TODO carried is real, so it moves to docs/refactoring.md rather than being deleted with it. TODO index regenerated in the same commit (87 -> 86 markers); --check passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
…g around OffsetEncoding has 12 values. Four were skipped by an assumption, five were branched around by a helper named assumeWorkingCodec that was really `return !encodingsThatFail.contains(encoding)` - a predicate wearing an assumption's name - and only three ran the full assertion path. The test read as covering every codec and materially checked a quarter of them. The five are not broken: v1 BitSet and RunLength encode length as a short and overflow past 32,767 against this test's 72,770 offsets, and uncompressed BitSetV2 exceeds the max metadata size. Graceful degradation is what the test is named for, so the fix is to assert the degradation rather than skip it. - assumeWorkingCodec -> isWorkingCodec, and the five call sites gain else branches asserting the degraded contract. Observed, not guessed: committed metadata is empty, reloaded state resets to zero with only highestSeen recovering, and every record from the committed offset up is redelivered. Work is repeated, nothing is lost. - The hard-coded failing list and the switch statement encoded the same knowledge; the switch is gone and CODECS_THAT_DEGRADE owns it, with a javadoc recording both overflow mechanisms. - Deletes a vacuous assertion: doesNotContain(2500L), where 2500 is never a record offset in this test, so it passed for working and failing codecs alike. Replaced with a positive containsExactlyElementsIn(...).inOrder(). Same defect class as docs/solutions/test-flakiness/vacuous-await-condition-*. Also removes the last JUnit 4 from the test compile path - OffsetEncodingTests used org.junit.Assume with Hamcrest matchers, which only worked by accident of junit4 arriving transitively via testcontainers, and CoreAppMetricsIntegrationTest used org.junit.Assert with its expected/actual arguments reversed. Both now AssertJ. This unblocks the JUnit 6 bump in #38. Counts unchanged at 18 run / 0 failures / 4 skipped before and after, so the assumption migration is like-for-like. Anti-vacuity control: mutating two terms of the new degraded assertions reds exactly the 5 degraded parameters and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 15m 54s —— View job Claude's review of PR #264
Overall: this is a well-verified PR. Multiple independent review passes (correctness, removed-behavior, reuse, efficiency/altitude) cross-checked the new degraded-codec assertions against the actual encoder/state source (
4 findings posted as inline comments on
All are low-severity (doc accuracy / maintainability / a pre-existing flake risk), not blocking correctness issues. |
✅ 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 | 1060 | 1060 | ➖ 0 |
| Duplication | 3.04% | 3.09% | 🙂 -0.05% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 5% | ✅ Pass (3.04%) |
| Max increase vs base | +0.1% | ✅ Pass (-0.05%) |
⚠️ 1 new clones introduced
- 22 lines:
parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/VeryLargeMessageVolumeTest.java:123<->parallel-consumer-vertx/src/test-integration/java/bz/stub/parallelconsumer/vertx/integrationTests/VertxConcurrencyIT.java:149
Powered by astubbs/duplicate-code-cross-check
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
The issue-reference gate caught three bare numbers below #1000 on added lines, and it is right to: the fork's numbering sits inside upstream's range, so a bare number is a coin flip. - "PR #49" (twice, in the audit and the backlog) is a fork PR: #49. - "(#190)" was quoted verbatim from a squashed commit subject, which is exactly how an upstream number leaks in looking innocent. Reworded so the quote stops at the subject and the number is cited as confluentinc#190 outside it. bin/check-issue-refs.sh now passes on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
… test/inactive-test-remediation # Conflicts: # docs/refactoring.md # docs/test-hardening/inactive-tests-audit-2026-08-08.md
Same class as the previous commit, three more sites the local gate found once the audit-branch fix merged up: - "#149, #160, #236" - the shorthand after the first qualified number still reads as a bare reference to the gate, and it is right to: nothing about position makes #160 unambiguous. - The salvaged OOM diagnostics carried "PR #49" verbatim from its branch. A salvage is not a reason to import a coin-flip reference, so it is qualified here too. - The plan's dependency diagram had "#106 / #207" for the same reason as the first item. bin/check-issue-refs.sh passes across all 20 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
|
Claude finished @astubbs's task in 1m 32s —— View job Claude's review of PR #264 (re-run after synchronize)
What changed since the last review: I checked each edit against its source: Code findings: unchanged from the previous pass, since no code files were touched. The 4 inline comments already posted on No new findings this pass. Nothing here blocks merge — all outstanding items are low-severity (doc-link accuracy, maintainability, a pre-existing flake risk, test dead-weight). |
…e-tests # Conflicts: # AGENTS.md
…ediation Brings master in transitively via 263's merge commit f5e72f4, reusing the conflict resolution already made there. Repairs stale addresses only: docs/TODO_INDEX.md -> docs/todo-index.md in the 002 remediation plan, following the rename master made. Per docs/citations.md a dated record's claims are not rewritten, but a reference that no longer resolves is repaired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L6PnsPmbwjazSu13M8JYm5
Both branches now have master merged. Records the merge order, the AGENTS.md and TODO_INDEX rename conflicts already resolved (so they are not re-litigated), the three units blocked on #260, and two things a reader would otherwise get wrong: the audit's quarantine count has drifted since it was written, and the core unit suite was not re-run locally after the merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L6PnsPmbwjazSu13M8JYm5
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
The plan's R19 asked for a pointer to `docs/test-hardening/` inside AGENTS.md's `## Testing` section, "in the style of the existing todo-index.md and quarantine pointers, so the next agent finds it without knowing it exists". What shipped was only the docs-map table row under "Where things live" - the Testing section, which is where an agent debugging a dark test actually reads, said nothing about the audit. The plan's Definition of Done nonetheless claimed the pointer was added. That is the exact discoverability failure this audit exists to fix: its 2026-04-22 predecessor rotted unread for four months because nothing pointed at it from the path an agent walks. Adds the bullet alongside the quarantine one, and names the current dated audit so the reader lands on a file rather than a directory listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The handoff note added in 2d5c5b3 cited both PRs as bare `#263`/`#264`, which is exactly what the PR Checklist issue-ref gate forbids: this fork's numbers sit inside confluentinc's range, so a bare number is a coin flip on which repo it means. The gate flagged 13 of them and went red on this PR - a document about the stack's merge readiness was itself the thing blocking the merge. Qualifies every reference as `astubbs#NN`, the form the gate strips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…failure one that was missing These were the last three items the stack left open. They were blocked on #260, which rewrote both files they touch; that merged and is now merged into this branch, so the blocker is gone and they are finished here rather than deferred again. Both dark tests failed 100% deterministically, and in both cases the LIBRARY was right and the test was wrong. `offsetsAreNeverCommittedForMessagesStillInFlightLong` asserted, through the transactional producer spy, that NOTHING commits while work is in flight. PC does commit - the base offset - which is a statement about where to resume, not progress. Rewritten onto the mode-agnostic helpers so it means the same thing in all three commit modes, asserting the frontier cumulatively: {3}, then {3,4}, then {3,4,6}. 3 rather than 2 after 0,1,2 complete, because a committed offset is exclusive. `processInKeyOrder` asserted flattened offsets {0,2,3,6,8}. Two separate errors: the same exclusive-offset confusion, and partition 1's base offset is 4 (record creation uses a global counter), which the flattened helper cannot tell from progress because it only trims a genesis of 0. Now asserted per-partition with assertCommitLists, ending (p0=[3,4], p1=[4,7,9]). The invariant it exists for is step E: partition 1 will not advance past 4 while key-2's record 4 is in flight even though 5, 6 and 8 are done, and then jumps to 7 rather than 9, because 8 is complete but not contiguous. `userSucceedsButProduceToBrokerFails` is new - a path the audit found reachable and untested. It asserts the consequence rather than the throw: the offset does not advance, and the record is retried. No producer-callback-thread variant: that is #261's, and it is unreachable here because the harness gives every test one auto-completing MockProducer that calls back on the calling thread. Each was mutation-checked rather than trusted for being green - breaking one expected value (of(3,4,6)->of(3,4,5); p1 4,7->4,9; removing the injected produce failure) reds all three modes. The `@Disabled` import goes with the annotations, its last users. `docs/test-hardening/dark-core-tests-measured-commit-sequences-2026-08-12.md` is deleted in the same change. It was salvaged one commit ago to stop these measurements dying with the plan that held them; they now live in the tests that assert them, with the reasoning in comments. A prose copy of numbers an assertion already pins is the drift this PR removed from `docs/todo-index.md`. Core unit suite: 345 tests, 0 failures under -Pci, up 7 with 2 fewer skips. All four repo gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ath it took CI red on `processInKeyOrder[1]`, and the defect was in the test I had just written, not in the library. It asserted the exact SET of commits per partition. The CI runner is slower, and there partition 0 read `[1, 3]` where three local runs read `[3]`. Both are correct: `1` means "record 0 done, resume at 1", an intermediate commit that lands when a commit tick falls between record 0 completing and 1 and 2 completing. On a fast machine the whole run 0-2 completes inside one tick and only `3` is ever committed. So the assertion was a statement about where the wall-clock commit ticks fell, which is exactly what #260 established a test cannot do - it collapsed PC's repeat commits inside `assertCommits` for that reason. I reproduced that lesson in a different shape: not a repeat, an extra intermediate step on the way to the same place. Three green local runs did not reveal it, and could not have. Now asserted through `awaitFrontier`: the highest committed offset per partition - where consumption would resume - which is the invariant, while the path to it is not. `max([1, 3]) == 3`, so this specific failure cannot recur by construction rather than by timing luck. The assertion stays sharp. The frontier only moves when work completes contiguously, so a partition that advanced past in-flight work still fails - mutation-checked by asserting partition 1 reaches 9 rather than 7 at the key-order checkpoint, which reds all three commit modes. The first checkpoint keeps `assertCommitLists`: nothing has completed on partition 0 there, so no intermediate value is possible and the exact-set form is stable. Core unit suite: 345 tests, 0 failures under -Pci, and processInKeyOrder green 3/3 consecutive runs. All four repo gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
left a comment
There was a problem hiding this comment.
docs/inflight/pr-263-264-inactive-test-stack.md needs trimming
…me it #263 is merged and GitHub retargeted this PR onto master, so the top of the note was false where it mattered most: it still said #264 was stacked, that #263 must merge first, and that `Check PR Dependencies` fails by design. That check passes. `docs/inflight/AGENTS.md` says what to do with the rest: when something closes, do not rewrite it into a FIXED/DONE narrative - shrink the file to the open follow-ups and rename it. The "Settled" section was exactly that wrong move, and it duplicated the commit messages. 80 lines to 38, carrying the two things that are actually open: the undecided LoadTest auto-run question, and the audit's known drift. Every surviving claim re-checked against the tree and the API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…correct the perf-lane comment The new `asyncConsumeAndProcessAtVolume` runs automatically in the required Performance Tests leg at ten times the gating volume, on a class listed in the load-tightness flake family at 1/20 and undiagnosed. AGENTS.md says to separate test-infra contention from a real concurrency bug before touching a deadline, so that is what was done rather than guessing. Given an uncontended broker, the case ran 5/5 green at 40,000, each in ~52s against its derived 600s ceiling. The code is fine at this volume and is nowhere near the deadline, so by that rule this family's failures are contention. The rung stays automatic. What the measurement could NOT clear is recorded at the site, because it is the thing to reach for if this ever does flake: the performance lane does not give it an uncontended broker. Failsafe declares no forkCount, so all four @tag("performance") classes share one JVM and therefore the one static BrokerIntegrationTest.kafkaContainer, with <parallel>methods</parallel>. The gating integration lane already solves exactly this by forking (-DforkCount=4 -DreuseForks=true); this lane never got that treatment. Also corrects two falsehoods in the failsafe comment that sent readers wrong: - It cited a `performance.yml` workflow on dedicated hardware. Neither exists. Performance tests run in maven.yml's leg on ubuntu-latest - a required check on every PR - and in pr-highcpu-fast-feedback.yml's highcpu lane. - It told readers to override with `-Dgroups=performance`, which does nothing: the <groups> element is pinned to ${included.groups}, so the explicit pom value wins and the bare property is silently ignored. Same trap the repo already documents for -DforkCount vs -Dsurefire.forkCount. The working flag is -Dincluded.groups=performance. Reactor test-compile clean; all four repo gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ity comment The comment claimed "0, 1 and 2 are all key-1 on partition 0". They are not. `primeFirstRecord` and `sendSecondRecord` both make **key-0**, so offsets 0 and 1 share key-0 - which is why 1 is blocked by 0, the thing this part of the test is demonstrating. Offset 2 is key-1, and so completed independently rather than being freed by 0. The mechanism the comment describes was right and the assertion is unchanged; the key naming was wrong, and it contradicted the assertion four lines above it that says 1 is "blocked by 0 ... due to key order processing". Found by an adversarial read of the surrounding evidence, not by a failing test - nothing executes a comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… left behind Two loose ends the compounding pass surfaced. `docs/inflight/test-untracked-ci-flakes.md` still listed `queuedMessagesNotProcessedOrCommittedIfSubmittedDuringShutdown` at 3/45 as an open regression, with a whole section on why to start there. #260 fixed it - its commit message names that test and that rate - and is on master. Per `docs/inflight/AGENTS.md` the entry is removed rather than rewritten into a FIXED narrative; the header now says which of the scan's three closed and that no product change was needed, because the extra commit was correct behaviour and the assertion was wrong. The learning doc listed the remaining strict call sites as an unswept remainder. They are now swept, and none is exposed - which sharpens the rule rather than weakening it: the exact form is only unsafe where a partition can pass through a NON-GENESIS intermediate frontier that the expected value omits, which needs three or more records on one partition advancing the frontier in two steps between checkpoints. The four `assertCommitLists` sites outside `processInKeyOrder` are all two-records-per-partition, so partition 0 goes genesis-0 (trimmed) straight to its frontier and partition 1's expected values are the successive frontiers themselves. Every `awaitForCommitExact(int)` call site commits a single record. `processInKeyOrder` was the only three-record shape - offsets 0, 1 and 2 with 2 completing before 0 - which is why it was the only one that failed. Relaxing the others would have cost assertion strength for no defect. Also carries the ce-compound output: the learning itself and its CONCEPTS.md entries (commit frontier, tick-path assertion). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ling deadline scaling Three cleanups from a four-angle quality pass. No behaviour change: 345 tests, 0 failures. `awaitForCommitExact(int partition, int offset)` is deleted. This PR replaced its only five call sites - all in processInKeyOrder - with the frontier assertions, which left a shared base-class method nothing in the tree calls. Worth being explicit that this is not the same defect the method was written for: it verified `sendOffsetsToTransaction` on the producer spy, so it only ever worked under PERIODIC_TRANSACTIONAL_PRODUCER, and processInKeyOrder is parameterised across all three commit modes - the old call was silently valid for one of them. Its javadoc about tick placement was right and is not lost; that reasoning now lives in docs/solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.md. Considered and rejected: rebuilding the frontier assertions on the existing `hasCommittedToPartition(tp)` truth subject. That reads the mock CONSUMER's commit history, and under PERIODIC_TRANSACTIONAL_PRODUCER commits go to the producer - which is exactly why `getCommitHistory()` dispatches on commit mode. The two existing helpers each cover one side of that split; neither spans it. `TransactionAndCommitModeTest.timeoutFor` now calls `completionCeiling` instead of hand-rolling the scaling. The hand-rolled form truncated, and worst precisely where the recovered ladder is meant to be used: at 40 threads `GATING_CONCURRENCY / threads` is 64/40 = 1 in integer division, so that rung got no scaling at all against a true ratio of 1.6 - a 60s deadline where 96s was owed. `completionCeiling` scales in Duration nanos, floors at the baseline (preserving "raising concurrency never shortens it"), rejects a non-positive knob rather than flooring it, and is unit-tested. `awaitFrontier` calls are now in a consistent (partition 0, partition 1) order at all four checkpoints; two of them had the pair reversed for no reason, which reads as significant when it is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e the review's four P1s A seven-reviewer pass found four P1s in this PR. Two were reproduced by running the tests, not by reading them, and the first is the one that matters. `offsetsAreNeverCommittedForMessagesStillInFlightLong` asserted the exact commit history, which is a claim about where the wall-clock commit tick fell. Measured: it failed 3 of 10 runs, as [1, 3] where [3] was expected and [3, 4, 5, 6] where [3, 4, 6] was, in two different commit modes. Both are correct PC behaviour - releasing 4 and 5 together does not guarantee one advance, and completing 0 does not guarantee 1 and 2 drain before the next tick. This is exactly the defect this PR diagnoses, documents in docs/solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.md, and fixes in processInKeyOrder - applied to one test and not to its sibling twenty lines away. Every record here is on partition 0, so it takes the same awaitFrontier treatment. My own verification could not have caught it: 3/3 green locally, four green full-suite runs, two green CI runs. A 1-in-6 flake survives all of that, which is the sentence the learning doc already contains. Now 12/12 clean on the same full-class runs that failed 3 of 10. Also from the review: - processInKeyOrder point-checked `processedState.get(2)` where the worker thread writes it after waking; the control loop can turn twice first. Failed 1 of 10 runs. Now awaited, matching what the same test already does for records 5 and 6. - Two of the four recovered knobs were documented with commands that select ZERO tests and exit BUILD SUCCESS: both classes are @tag("performance"), the default excluded.groups contains performance, and exclusion beats inclusion - so `-Dincluded.groups=performance` alone selects nothing. Verified: Tests run: 0. Both now point at bin/performance-test.sh, which passes -Dexcluded.groups= too. The pom comment this PR added said "or -Dexcluded.groups=" and taught the same trap; it now says BOTH. The other two knobs were checked and are fine - TransactionAndCommitModeTest is @tag("transactions"), not excluded by default. - redact() masked on the key only, so a credential inside a VALUE under an innocuous key printed verbatim to CI logs - a JDBC URL with ?password=, or a URL with userinfo. It now scans the value as well, and `passphrase` is a marker (this repo signs releases with maven-gpg-plugin, and "passphrase" is not "password"). NOT `pat`, which was suggested: it substring-matches java.library.path and would mask most of the dump. A test pins that. - The perf-lane diagnosis was wrong and disproved at bytecode level: Surefire's JUnit Platform provider never reads failsafe's <parallel> element, and -Pci sets parallel-tests=false regardless. The lane is sequential in one fork, so the first suspects are heap accumulated under reuseForks, container start-up and the 60-minute cap - not broker contention between concurrent tests. - Dropped the `assertCommits(of())` that raced partition 1's bootstrap commit; the awaited per-partition assertion below already covers it without racing. Core unit suite: 347 tests, 0 failures under -Pci. All five repo gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…would have merged stale The code-review pass left findings that were deliberately not folded into the fixes: each is a change to shared machinery rather than to the work under review, and burying them in a P1 commit would have hidden them. They are now tracked where a future branch will find them. `test-required-perf-lane-scope.md` - what belongs in the REQUIRED Performance Tests leg. Records the settled part (the 40,000 rung stays automatic, on 5/5 uncontended green at ~52s against a 600s ceiling, and it then passed the real lane) and the part that is not settled: `LoadTest extends DbTest`, whose static initialiser starts a Postgres container that no test in that lane reads or writes, and which is itself listed at 2/20 for "postgres container start under contention". Also records that `-Dload.total` moves the untagged gating tests too, which is a trap if that flag ever reaches a lane. `test-inactive-test-review-followups.md` - the deferred code findings. Chiefly: `awaitFrontier` should live in the shared test base, not private to one class. The learning this work produced argues that #260's rule failed to generalise because it sat somewhere you only read if you already held the helper - and the fix for it was then put somewhere only one test class can reach. The note carries the two blind alleys already checked (the deleted `awaitForCommitExact(int,int)` was transactional-only; `hasCommittedToPartition` reads the consumer, so neither spans the three commit modes), the holding-variant fix for the frontier's negative checkpoints, and two `CompletionCeiling` items. `pr-264-inactive-test-remediation.md` is deleted rather than carried. Per `docs/inflight/AGENTS.md` a `pr-` note is context about an OPEN pr, and work a PR resolves is deleted in that PR - so a note added by #264 describing #264 lands on master describing a closed one, which is the exact failure that file warns about. Its two surviving items moved into the notes above: the perf-lane question and the audit's known drift. One P3 fixed rather than filed: the `timeoutFor` comment cited "a 60s deadline where 96s was owed" when `defaultTimeout` is 30s, so the real figures are 30s and 48s. The ratio argument was right; the absolute numbers were not. Reactor test-compile clean; all five repo gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude please review this PR's head Two Where to look hardest. A seven-reviewer pass already covered the older commits; the four newest are what needs scrutiny:
Please run the tests yourself rather than trusting my numbers - the whole point of finding 1 is that green local runs are not evidence for a timing-sensitive assertion. |
|
Claude finished @astubbs's task in 7m 7s —— View job Claude's review of PR #264 head
|
|
@claude please review head The previous run (job 31661308740) spent 68 turns / 7m07s and ended having ticked the four analysis items but with "Post findings" still unticked - it backgrounded Do not run any test suite, any
Write your findings into your own tracking comment ( What I want judged, by reading the diff and the surrounding code only:
If you find nothing blocking, say so plainly and tick the boxes - a clean read is a useful result. |
|
Claude finished @astubbs's task in 5m 22s —— View job Claude's review of PR #264 head
|
…d-waits Brings in #264 (now merged), which was this PR's declared dependency, plus #277, #260 and #263. One conflict, in docs/todo-index.md - a generated file. Master's bin/todo-index.sh changed the header format (the marker count moved out of the prose) while this branch still carried the old shape, so both sides had edited the same generated line. Resolved by regenerating from the merged tree rather than picking a side: 84 markers, and `bin/todo-index.sh --check` reports it current. Master did not touch BlockedThreadAsserter or junit-platform.properties in these four commits, so this PR's two central changes had no semantic collision to resolve. Verified after merging: junit-platform.properties still deleted, core's pom still carries the surefire/failsafe configurationParameters, and the asserter rewrite plus its negative-control test are intact. Local verification: core 354 tests and vertx 18, 0 failures, 0 errors. bin/check-quarantine-registry.sh and bin/check-copyright-headers.sh both clean.
…and stop two smaller lies next to it A sweep of the other pollDelay sites, prompted by the produceTimeout fix. The two in CloseAndOpenOffsetTest are not the same defect as each other, and only one of them is the same class. correctOffsetVerySimple IS an instance, of the vacuity variety. It asserted that a freshly opened PC reads nothing back after a flat 1s wait, with nothing establishing that the PC had joined the group - so "nothing was read" was equally true of a consumer that never finished joining. Control arm, pointing that PC at a decoy topic: the original assertion passes, the anchored one fails at the assignment await. It now waits on PartitionStateManager#getPartitionState being non-null - a ConcurrentHashMap, so unlike Consumer#assignment it is safe to read from the test thread while the poll thread runs - and then produces one record AFTER the absence window that must be read. Assignment proves it joined; the trailing record proves its poller reached the topic. Same rule as TransactionalPartialResultSetIT#proveVerifierIsActuallyReading: prove the detector works before trusting what it did not detect. readByThree becomes a ConcurrentLinkedQueue, since the test thread now reads it while a worker writes. largeNumberOfMessagesSmallOffsetBitmap is NOT an instance - its assertion retries, so a slow start costs time, not correctness. Two smaller things were still true of it: - atLeast(500ms) behind pollDelay(1000ms) can never fire. Measured: raising the atLeast to 1500ms reports the condition "evaluated in 1 seconds 6 milliseconds", so at 500ms it was dead. Removed. - The trailing hasSize was commented "double check after closing" while sitting inside the try-with-resources, so it ran before close. Moving it out does not earn the claim: produce an extra record after the await and flush it to the broker before close, and PC never delivers it - it stops fetching at close - so both positions still pass. The comment now says what the assertion is, a restatement, rather than the assertion being relocated on a theory the control refuted. Also cross-links the frontier/tick-path rule from #264, and names this change's deliberately-unclosed residual as an instance of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…298) Two merged PRs, #264 and #277, carry no LGTM in any form - no review, no comment. The requirement is that the owner reviews everything that goes in, and the gap was not a lapse of intent but of visibility: "have I read this one myself yet?" is a thing to carry across a dozen open PRs, and carrying it does not work. So it becomes a check with its own name. `bin/check-human-lgtm.sh` asserts one thing: the owner has left a REVIEW whose body contains "lgtm", any case, anywhere in the body. The reviews endpoint is the rule rather than an implementation detail - "lgtm" typed into the ordinary comment box is not the deliberate act being asked for - and the endpoint gives that distinction for free. The matching rule was settled by looking rather than by taste. All 50 owner LGTMs this repo has received, across 38 PRs from #63 to #292, are COMMENTED reviews; 49 are lower-case `lgtm` and one, on #84, is `Lgtm`. Four carry a trailing clause, one of them ending in a question mark. A case-sensitive or whole-word rule would have gone red on real history, so both of those real spellings are pinned as cases. NOT HEAD-SENSITIVE, deliberately: an LGTM on any commit counts for the whole PR, permanently, because the owner only stamps a PR once it is near merge. That is why review STATE is not consulted at all - the master ruleset sets dismiss_stale_reviews_on_push, so reading state would silently un-stamp a PR the owner had already stamped. A SEPARATE JOB, NOT A SECOND STEP IN claude-review, and that is the substance of this change rather than a packaging choice. Two checks say WHICH half is missing straight from the checks list - "claude-review" red means no automated review, "review: human LGTM" red means the owner has not read it yet - and one combined check is red either way, which is exactly the question the human half exists to answer. It also leaves claude-review byte-identical to master: no rename, so no transitional duplicate job and no ordered ruleset swap, and a required check matched by name cannot be left pointing at nothing. Making the new context required is additive, once it has merged and is reporting. NO BOT EXEMPTION, unlike the automated half. A Dependabot PR does not need an AUTOMATED review, but it is still a change going in. Having no guard also means there is no job to skip - and a skipped job satisfies a required check, so a guard keyed on github.event.sender would have let a human PR synchronized by a bot go green having asserted nothing. The checker is 15 lines. An earlier revision was 593, with 855 lines of self-test, parsing code fences, blockquotes, negation forms, glued repeats, CRLF and typographic apostrophes - defences against an attacker who is also the only person the check protects. Filtering the reviews with jq on .user.login before any text is read removed both the attack and the unguessable per-run marker token it needed, since a stranger's body can no longer be attributed to the owner. Verified against real data, not only fixtures: #206 reads LGTM-present, #298 and #299 read absent. The self-test carries a negative control proving the check can fail at all.
…ponse-timeout 61 commits of master, including the tree-wide package rename that this branch had already run for itself. Both sides renaming independently is what made the merge look enormous: git paired only some of the moves, so the same file arrived as a rename/delete plus an add/add rather than as one rename/rename. 45 conflicts over 34 files, none of them a real disagreement about behaviour. The add/add pairs were re-merged by hand as proper three-way merges, using the pre-rename io.confluent blob at the merge base as the common ancestor - which is the ancestor git could not find on its own once master rewrote several of these files after moving them (#206 dropped MockConsumerTest and friends below the rename-detection threshold). Every one of those eight then resolved to master's content exactly: this branch had contributed nothing to them but the rename. Resolutions worth knowing about: - The eight logback-test.xml files were both sides deleting the same dead csid logger lines - master in #289, this branch in 76dc3ed mirroring it. Master's copy taken wholesale. - docs/inflight/ci-review-agent.md: master's rewrite taken wholesale. Every entry this branch still carried is now either fixed (the SIGPIPE gate - story kept in docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md) or stale (the actionlint and bin/todo-index.sh grants are both in the allowlists on master). The file's own contract is that it holds open gaps only. - docs/inflight/test-load-tightness-flakes.md: both sides kept. Master reclassified commitTimeout[1] out of the family for an unforceable trigger; this branch's entry is commitTimeout[2], about the assertion rather than the trigger. Neither supersedes the other, and the section now says so instead of leaving a reader to find two verdicts on the same test name. Its ConsumerOffsetCommitter:96 citation is re-pointed at a greppable anchor, per the rule that arrived in #283. - JavaEnvTest, StringTestUtils, junit-platform.properties, SampleTestingFailsafePluginInclusionCore and the two .idea run configurations: master deleted them in #264 and #265, this branch had only renamed them. Deletions accepted; nothing in the tree still refers to them. - bin/rename-packages.sh: master's copy. This branch took the tooling from an earlier state of it and never modified it. - The README summary paragraph: master's, which is frozen against the rename script precisely because it has to name the namespace a reader is moving off. What this branch contributes on top of master is unchanged and now stands at nine files: the three core classes, the two new tests, and four documents. Verified: full unit suite, and bin/check-copyright-headers.sh, bin/check-issue-refs.sh, bin/todo-index.sh --check, bin/check-action-versions.sh, bin/check-docs-data.sh, bin/check-quarantine-registry.sh, bin/check-quarantine-owners.sh, bin/check-shell-sigpipe.sh, bin/check-cve-exclusions.sh all green locally.
…owners THE NOTE HAD FALLEN BEHIND `pr-offset-encoding-policy.md` still described the state three rounds of work ago. Added what merge prep turned up and what no gate caught: the falsified `docs/features/` entry, the javadoc the merge dropped, two dangling citations to a class deleted in #267, and the SpotBugs overload finding. Also records what was harvested from the stood-down `recut/207-offset-policy-bypass` and what was deliberately not. THE JSTREAM FLAKE IS NOT UNDIAGNOSED - #116 OWNS IT Recorded as UNDIAGNOSED when it was carried across. Chasing it found #116, "a result stream that ends before the results arrive", whose own description says eight tests "passed ONLY because the stream quit early - they encoded the defect". `testConsumeAndProduce` and `testFlatMapProduce` are two of them, and they are the two seen failing empty. So the sighting is not test infrastructure at all: it is that product defect observed racing the other way, which explains both why the two failed together and why the class passes in isolation. Entry now carries the mechanism and says to delete it when #116 lands rather than diagnose or quarantine it separately. Posted the sighting on #116 too, since it is evidence for a claim that PR currently only asserts. A SECOND, DIFFERENT `processInKeyOrder` FLAKE Hit while running the full core suite with CI running on the same box. Recorded because it is NOT the `processInKeyOrder` flake `docs/solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.md` already covers - that one is a 30s ConditionTimeoutException in `assertCommitLists` and its fix merged with #264. This one fails in ~0.6s, before any commit assertion: the test's own `[sanity check input data]` precondition finds `polled` empty, because `awaitForOneLoopCycle()` is a fixed number of control cycles standing in for "the consumer has polled its records". Control-armed rather than assumed, which is the only reason it is filed as contention: green 559/0 on a fresh worktree with the box idle at both this branch's tip AND the commit before it, 2 failures twice with the box loaded, and green 68/68 for the class alone under that same load. The failing parameters differ between runs, which rules out a deterministic break from this branch - #207 touches offset decoding and nothing that test exercises. Diagnosed from the assertion, not demonstrated. The entry says so and defers to the ledger's classify-before-touching rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj

Description
Acts on the audit in #263. That PR is merged, so this one is no longer
stacked - it targets
masterdirectly, andmaster'sio.confluent.*→bz.stub.*rename(#294) is merged in.
All nine of the plan's units. The last three were blocked on #260; it
merged, so they are finished here rather than left as follow-on.
What landed
Five codecs that were silently unchecked are now asserted.
OffsetEncodinghas 12 values. Fourwere skipped by an assumption, five were branched around by a helper called
assumeWorkingCodecthatwas really
return !encodingsThatFail.contains(encoding)- a predicate wearing an assumption's name -and only three ran the full assertion path. The test read as covering every codec and materially
checked a quarter of them.
The five are not broken: the four v1 codecs (
BitSet,BitSetCompressed,RunLength,RunLengthCompressed) encode length as ashortand overflow past 32,767 against this test's 72,770offsets, and uncompressed
BitSetV2exceeds the max metadata size. The overflow guards are namedwhere they actually live -
Short.MAX_VALUEinBitSetEncoder'sinitV1, andMathUtils.toShortExactinRunLengthEncoder.serialise()- and notBitSetEncoder.MAX_LENGTH_ENCODABLE(v2 only) or
OffsetRunLength(decode side, no overflow detection), which an earlier draft of thatjavadoc cited. Graceful degradation is what the test is named for, so the fix is to assert the
degradation, not to skip it. The degraded contract - observed, not guessed - is work is repeated,
nothing is lost.
Also cleans up the same test's assertion hygiene: the vacuous
doesNotContain(2500L)is gone (2500 isnever a record offset there, so it passed for working and failing codecs alike - same defect class as
docs/solutions/test-flakiness/vacuous-await-condition-brokerpoller-backpressure-2026-07-31.md), thetautological
doesNotContainSequence(expected)that sat behind an exact-contents assertion is gone,and three checkpoints that re-verified state nothing between them could have mutated are collapsed
into one.
The last JUnit 4 is gone from the test compile path.
OffsetEncodingTestsusedorg.junit.Assumewith Hamcrest matchers, working only by accident of junit4 arriving transitively via Testcontainers;
CoreAppMetricsIntegrationTestusedorg.junit.Assertwith its expected/actual arguments reversed.Both now AssertJ. A tree-wide grep for
org.junit.{Assert,Assume,Test,Before,After,Rule}importsreturns nothing. This is a prerequisite unblocker for the JUnit 6 bump in
#38, not a conflict with it.
Four dead tests deleted - a stub that never ran green and could not have
(
VertxTest.handleHttpResponseCodes, bodyassertThat(true).isFalse()behind@Disabled), acommented-out
@Testover an infinite stream (StreamTest.test), an empty{}body(
SampleTestingFailsafePluginInclusionCore), and alog.errorwith no assertions(
JavaEnvTest). PlusStringTestUtils, which the last of those solely orphaned.Commented-out alternatives removed - and this analysis was incomplete. See the addendum below. No
live value changed:
30_000_00is rewritten as3_000_000, byte-identical at runtime, and everyvolume flagged turned out to be already correct, a deliberate increase misread as a reduction, or
gated by a wait that would have to move with it.
One main-source correction, javadoc only.
PartitionStateManager.onOffsetCommitSuccessclaimed totruncate tracked offsets below the committed offset. It does not, and cannot:
PartitionStatetracksonly incomplete offsets and the committed offset is the lowest incomplete one, so there is nothing
below it to throw away. The truncation that does exist happens on the bootstrap poll. Found while
re-deriving what the two dark core tests should assert; no behaviour changed.
The audit itself corrected, including four claims it had inherited from its 2026-04-22 predecessor
without re-verifying them, and all ten never-written stubs given a disposition.
The finding worth reading
Of the ten tests deleted rather than written, only one is still worth writing.
userSucceedsButProduceToBrokerFailsThe three that cannot be written are the interesting ones. Parallel-consumer has never had a
dead-letter queue - zero occurrences in any
src/main/java- and it is the most-demanded missingfeature, already tracked as #149.
maxPerPartitionandmaxPerTopicwerewritten as a trio with
maxOverall; only the global scope was ever built, and it isWorkManagerTest.maxInFlighttoday. So those stubs record an aspiration for limits at three scopes, ofwhich one shipped.
The three units that were blocked are now done
#260 was the hard prerequisite - it rewrites both files these touch and
collapses the repeat base-offset commits they would otherwise assert on. It merged, so the two
long-dark tests are re-enabled and the missing one is written, here.
Both dark tests had been
@Disabledsince 2020 and failed 100% deterministically (12 executions,zero variance). The library is correct - direct measurement shows key-order blocking holding
exactly. The tests were wrong, in the same way twice: a committed offset is exclusive, so it
names where to resume, not the last record done.
offsetsAreNeverCommittedForMessagesStillInFlightLongasserted that nothing commits whilework is in flight, via
verify(producerSpy, after(...).never()).commitTransaction(). PC does committhe base offset; that is a starting point, not progress. Now asserts the frontier, cumulatively:
{},{},{3},{3,4},{3,4,6}.processInKeyOrderasserted a flattened{0, 2, 3, 6, 8}. Two things were wrong: the exclusiveoffset above, and partition 1's base offset of 4, which the flattened
assertCommitsreads asprogress because it only trims a genesis of 0. Now per-partition - one
assertCommitListscheckpoint, then frontier assertions ending at
p0=4,p1=9. The frontier rather than the exactset is a follow-up correction: the exact-set form passed locally and failed in CI, because which
intermediate offsets appear depends on where the wall-clock commit tick falls relative to work
completing (
[1, 3]on a slow runner,[3]on a fast one). That is the same traptest(core): tolerate PC's repeat commit of the same base offset #260 fixed for repeat commits, in a different shape. The frontier is the
real invariant: it can only move when work completes contiguously.
userSucceedsButProduceToBrokerFailsis new. The produce half ofproduceMessageFlow: the userfunction succeeds and the produce that follows it fails. It asserts the consequence, not the throw -
the offset does not advance and the record is retried. No producer-callback-thread variant; that
asymmetry is owned by open fix(core): stop a terminally failed send from publishing half a result set under transactions #261 and is unreachable in this harness anyway.
All three run across all three
CommitModes, and each was mutation-checked: breaking the expectedvalue reds it.
Addendum: the commented-out analysis asked the wrong question
Review of this PR objected to the deletions at nine sites. Re-tracing each one shows the objection
lands, and shows why: the deletions asked whether a commented-out line was dead, and never asked
whether it had ever been alive.
That is the question that decides what the line is. A line that was never live is more likely to
record a manual run, not less - uncommenting a rung, running it, and commenting it back leaves nothing
in git, so the parked ladder is the only place the volumes someone actually exercised are written
down. A superseded live value is the opposite: history already holds it.
MultiInstanceHighVolumeTest//10_000_00004cd4d81, commented atad3636a5StreamTest//@Test61f4c0e4VeryLargeMessageVolumeTest//2_000_0002b0ab66bas the live value was loweredLoadTest8_000_0 / 4_000_00 / 4_000_0 / 8af1fa5deTransactionAndCommitModeTest2 / 100 / 10002b0ab66bTwo more of the same shape.
JavaEnvTest's javadoc said it was "used to manually inspect the javaenvironment at runtime - particularly useful for CI environments" - a manual diagnostic, deleted for
asserting nothing, which removed the tool without automating what it was for. And
VertxTest.handleHttpResponseCodeswas called "a deletion candidate... nothing here to restore"; trueof the stub, silent about the gap -
send.onSuccesscallsonUserFunctionSuccess, and aWebClientfuture succeeds on any HTTP response, so a 5xx appears to commit the offset, untested either way.
The provenance is recorded as a dated addendum in audit §11 rather than by rewriting the 2026-08-08
claims, per
docs/citations.md.The recovery, and what running it found
Each ladder comes back as a system-property knob -
-D<concern>.<knob>, matching the existingchaos.seedandambient.probe- rather than as restored comments:LoadTest.RECOVERED_VOLUMES(
-Dload.total), a-Dtransactions.concurrencyladder inTransactionAndCommitModeTest,-Dvolume.messagesinVeryLargeMessageVolumeTest, and-Dmultiinstance.messagesinMultiInstanceHighVolumeTest.A fixed deadline was the mechanism at five of five sites. Not a quirk of one test: it is how
these procedures stopped being runnable. Someone parks a higher rung, the hard-coded wait fails it
regardless of whether the run is healthy, the rung becomes decoration, and the next reader deletes it
as dead.
MultiInstanceHighVolumeTestsays so in its own history - the volume was reduced because"the 60s wait below cannot be met at higher volumes". Every ceiling now derives from the size, anchored
so the gating value is provably unchanged: exactly 60s at
LoadTest's 4,000, multiplier exactly 1 at64 threads, exactly 120s at 1M, exactly 60s at 3M. Pinned in milliseconds by
CompletionCeilingTest(4tests, no Docker) rather than only by Docker-bound integration runs.
Two rungs abandoned as unreachable both pass on first attempt.
VeryLargeMessageVolumeTestat the2,000,000 someone wanted in 2020: green in 22.5s.
MultiInstanceHighVolumeTestat the 10,000,000reduced away in 2021: green in 62s. Nothing was too ambitious - the constraint dissolved as hardware
got faster, and nobody rechecked because the only record of the intent was a comment nobody executes.
One rung could never have run, and only executing it showed that.
LoadTest's parked8derivesits key range as
volume / 100, so eight records gave zero keys andpublishMessagesthrewIndexOutOfBoundsExceptionon the first send - true ofaf1fa5deas authored. So "born commentedmeans someone ran it by hand" holds for the volume rungs and fails for this one. Provenance narrows
what a parked line was; it does not settle it. The key range is floored at one, and the setting now
does what it was for.
Two exposures are stated rather than hidden.
LoadTestgainsasyncConsumeAndProcessAtVolume,@Tag("performance")- but that tag is not opt-in:bin/performance-test.shpasses-Dincluded.groups=performanceand is the required "PerformanceTests" leg of
maven.yml, so it runs on every PR at 40,000, ten times the gating volume, with noretry.
LoadTestis a listed 1/20 undiagnosed member of the load-tightness flake family at thegating volume (
docs/inflight/test-load-tightness-flakes.md); the rate at 40,000 has never beenmeasured. And the top recovered rung,
-Dload.total=400000, derives a 100-minute ceiling againstCI lanes that cap at 60, so it must be run outside them or the job timeout raised first - inside,
a genuinely hung run is killed 40 minutes before the diagnostic
ConditionTimeoutExceptionfires. Bothare stated in the javadoc; the first is left open for a human call in
docs/inflight/pr-263-264-inactive-test-stack.md.JavaEnvTest's environment dump is automated into theAMBIENT PROBE AUTOPSYblock, whereAGENTS.mdalready sends a reader when a broker integration test fails - correlated with the failure,which the manual version never was. It is emitted once per JVM, not once per run: the integration
lane forks several, so a run can carry one dump per fork. Because the autopsy goes straight to CI logs,
values of properties whose key reads like a credential (
password,secret,token,apikey, …)are masked while the key still prints - nothing in this repo passes a secret as
-Dtoday, so thisstops a future one rather than fixing a present leak. Masking by name beats an allowlist, which would
silently drop the next knob somebody adds.
The Vert.x gap is asserted. A delivered 500 marks the record processed and commits the offset
(
serverErrorStatusStillCommits); a transport failure retries it(
transportFailureIsDistinctFromANonSuccessStatus). Both halves pinned, and the contract stated onVertxParallelEoSStreamProcessorwhere someone configuring aWebClientwill meet it.A pre-existing flake in
AmbientProbeExtensionTestis fixed on the way past. EverybuildAutopsycall mutates the same static once-per-JVM dump guard whether or not the test caresabout it, and this module runs JUnit thread-parallel outside
-Pci, so any two unserialised callscould red each other. Every test that calls
buildAutopsynow holds the same@ResourceLock, notjust the ones asserting on the dump.
Documentation lifecycle
2026-08-08-001,2026-08-08-002,2026-08-12-001): their work landed, andAGENTS.mdsays a plan goes stale once its work lands. Theone thing that outlived them - the measured commit sequences for the three formerly-blocked units -
is now in those tests as assertions with the reasoning in comments, rather than as a prose copy
that would drift.
LargeVolumeInMemoryTestsOOM diagnostics are salvaged intodocs/test-hardening/. Theyexisted only on
origin/refactor/test-hardening, which is on the safe-to-delete list, and they arethe sole measured evidence that restoring 1M exhausts the heap in the close path.
docs/refactoring.mdnow scopes that restore as three changes, not one.docs/todo-index.mdno longer carries a marker count, and the count is removed from thegenerator
bin/todo-index.sh, not just from the generated file - a second, drifting statement ofwhat the list below already says exactly. The script still prints one to the console, where it
cannot go stale.
docs/inflight/pr-263-264-inactive-test-stack.mdrecords the stack's state, the ten renameconflicts and how they went, and the open items.
Verification
CI is green on
970c521, every required check included. Core unit suite 347 tests, 0 failures, 8skipped. Vertx module 18 tests, 0 failures.
OffsetEncodingTestssteady at 18 run / 0 failures/ 4 skipped before and after, so the assumption migration is like-for-like.
todo-index --check,copyright headers, docs-data and quarantine registry all pass.
Every recovered knob was executed, not merely made selectable:
LoadTestin the gating lane and under-Dincluded.groups=performanceand at-Dload.total=8; the concurrency ladder at 64 and at 2; thevolume ladders at 5,000 / 1M / 2M and 3M / 10M. Anti-vacuity control: each run logs its own count
(
8/8,4000/4000,Producing 10000000 messages) and the concurrency runs logmaxConcurrency=64/maxConcurrency=2, so the properties demonstrably drive the runs rather than merely being accepted.Now established:
AGENTS.mdasks which cause was determined before a timeout moves - contention ora genuine concurrency bug. The uncontended-broker diagnostic has since been run at the 40,000 rung:
5/5 green at ~52s against its derived 600s ceiling, so the code is sound at that volume and nowhere
near its deadline. By that rule this family's failures are contention. What the measurement could not
clear - the lane shares one fork, one heap and an unused Postgres container - is recorded in
docs/inflight/test-required-perf-lane-scope.md.Anti-vacuity control: mutating two terms of the new degraded assertions reds exactly the 5
degraded parameters and nothing else - so the new assertions can fail for the right reason, rather
than merely being green.
Nothing was weakened to get green. The one assertion removed for weakness was the vacuous one, and it
was replaced with a strictly stronger positive check.
Review round: four P1s found and fixed
A seven-reviewer pass over this PR found four P1s. Two were reproduced by running the tests rather
than reading them, and both were introduced here:
offsetsAreNeverCommittedForMessagesStillInFlightLongpinned the exact commit history, which failed 3 of 10 runs -
[1, 3]where[3]was expected and[3, 4, 5, 6]where[3, 4, 6]was, in two commit modes. Both are correct PC behaviour. This is thedefect this PR documents and fixes in
processInKeyOrder, applied to one test and not its siblingtwenty lines away. Now on the same
awaitFrontiertreatment: 12/12 clean on the full-class runsthat previously failed 3 of 10.
processInKeyOrderpoint-checkedprocessedState.get(2)where the worker writes it after waking;failed 1 of 10 runs. Now awaited, matching what the same test already did for records 5 and 6.
@Tag("performance"), the defaultexcluded.groupscontains it, and exclusion beats inclusion. Bothnow use
bin/performance-test.sh. Thepom.xmlcomment this PR added said "or-Dexcluded.groups="and taught the same trap; it now says BOTH.
redact()masked on the key only, so a credential inside a value under an innocuous key printedverbatim to CI logs. It now scans values too, and
passphraseis a marker. NOTpat, which wassuggested: it substring-matches
java.library.pathand would blank most of the dump. A test pins that.Two corrections to this PR's own claims came out of the same pass. The perf-lane javadoc blamed
<parallel>methods</parallel>for contention; Surefire's JUnit Platform provider never reads thatelement and
-Pcidisables parallelism anyway, so the lane is sequential in one fork and the firstsuspects are heap and the 60-minute cap. And
awaitForCommitExact(int,int)was left dead by this PR'sown rewrite - it verified
sendOffsetsToTransaction, so it only ever worked under one of the threecommit modes - and is deleted.
Deferred deliberately, with reasoning recorded rather than lost:
docs/inflight/test-inactive-test-review-followups.md(chiefly hoistingawaitFrontierinto the sharedtest base, and giving its negative checkpoints a hold rather than a sample) and
docs/inflight/test-required-perf-lane-scope.md.Checklist
docs/refactoring.md, the inflight stack note, and the generatedtodo index
docs/features/-N/A- no newuser-facing feature; the Vert.x status-code contract was already the behaviour and is now stated
in
VertxParallelEoSStreamProcessor's javadocbut two things are worth a reviewer's eye: the ambient probe now dumps system properties into CI
logs (values masked by credential-looking key name), and
asyncConsumeAndProcessAtVolumerunsautomatically in the required Performance Tests leg at ten times the gating volume