Repository navigation
test(core): clear two quarantine entries by fixing what they were about, not by waiving them - #473
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #473 +/- ##
============================================
+ Coverage 82.17% 82.78% +0.60%
- Complexity 1536 1545 +9
============================================
Files 96 96
Lines 5353 5343 -10
Branches 533 529 -4
============================================
+ Hits 4399 4423 +24
+ Misses 758 729 -29
+ Partials 196 191 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
⚪ Throughput — no comparable reference
10 baseline run(s) were found and none ran the same set of test cases as this run, so none is comparable. This is usually the PR's own doing: enabling or disabling a test in this lane changes the matrix.
The baseline rows are shown for orientation only. They ran a different workload, so a difference between them and this run is not a measurement of anything - do not read one as a regression or an improvement. Refusing to compare against a run whose workload differs, rather than averaging the difference in and calling the result a verdict. Updated for |
|
|
@claude review this |
|
Claude finished @astubbs's task in 4m 4s —— View job Review
This is a well-evidenced PR — the control-arm methodology on the
This isn't a real marker. The old, genuine Fix is simple: reword the javadoc sentence so it doesn't contain the bare marker word (e.g. "the attribution question this comment used to flag as unsettled is now answered by that measurement"), then re-run Minor, non-blocking nit: in Everything else checked out: the quarantine registry edits leave a single, well-justified remaining entry; the sighting-ledger updates in Note on tooling: I could not execute |
The automated review on #473 found that docs/todo-index.md carried a sentence fragment as an entry: the javadoc in MultiInstanceRebalanceTest described the history of a removed TODO(refactor) marker by quoting the marker word, and the index generator matches on the word alone. Reworded so the sentence says the same thing without the word, and the index regenerated. Also aligns one link label in docs/ci.md with how the sibling citations in this PR write theirs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
|
@codex review |
|
Response to the automated review. The phantom TODO entry is gone (342d899). The javadoc sentence in The link label in Nothing else changed. Both quarantine gates and |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 342d8998d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #473: moving cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance onto @tag("capacity") took them out of the required Performance Tests lane, but no scheduled or dispatchable runner ever targeted them - every pc_run_performance caller in bin/exp-*.sh and every choice in .github/workflows/experiments.yml names only MultiInstanceRebalanceTest#largeNumberOfInstances. Verified by grep: neither method appears in any workflow or bin/ script before this change. So the two profiles ran nowhere at all, silently - exactly the "test that never runs is not a passing test" trap AGENTS.md names. bin/exp-measure-capacity-profiles-failure-rate.sh gives both the same weekly cadence largeNumberOfInstances already has, reusing pc_run_performance and the same failsafe-report classification (both share MultiInstanceRebalanceTest's runScenario/ProgressTracker plumbing, so the same "No progress beyond N records" / "missing keys" extraction applies unchanged). Wired into experiments.yml as a second weekly schedule slot, routed via github.event.schedule since a scheduled run carries no workflow_dispatch input. Never added to a gating lane - it is a scheduled, non-required measurement, same as largeNumberOfInstances. docs/testing.md's experiment-runner table gets a row for the new script. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
…lag bullet Three more Codex findings on #473, all in docs: - docs/inflight/test-untracked-ci-flakes.md kept a SOLVED table row and a full "Closed" section for RegistrationRaceStaleResidentIT even though the durable diagnosis already lives under docs/solutions/. docs/inflight/AGENTS.md is explicit that a closed item does not get rewritten into a "FIXED/DONE" narrative - it gets removed, with what outlives it migrated first. Removed the row and the whole section (its own sighting-history subsection included); the file's own convention for a closed item (see the ProducerManagerTest/MDC entries in the intro paragraph) is a one-line mention with a link, which is what replaces it. - docs/inflight/test-largenumberofinstances-residual-failures-measured-not-explained.md said outright that its own subject had been acted on and that it stayed open only for the sibling cannot-gate-a-merge note's tracked items - the same "closed item kept open" shape. Migrated its durable content (the mechanism chain, the two refuted candidate fixes with their measured control arms, the deterministic reproducer, the generalisable lessons) into a new solutions write-up, docs/solutions/test-flakiness/large-instances-residual-is-a-join-phase-held-open-by-churn-2026-09-05.md, then deleted the note and repointed every citation (grepped the whole tree: bin/, .github/workflows, docs/inflight, docs/solutions, two Java files and a logback resource - ten citing locations besides the note itself). The @Quarantined annotation this note used to be tracked from is already gone from MultiInstanceRebalanceTest, removed earlier in this same PR. - docs/inflight/test-largenumberofinstances-cannot-gate-a-merge.md's "what implementing it involved" section still said bin/performance-test.sh passes an empty -Dexcluded.groups= and told readers to preserve that - directly contradicting the "What landed" section above it, which correctly says the required lane now passes -Dexcluded.groups=quarantined,capacity. Rewrote the bullet to state the current contract instead of the pre-implementation one, and updated the "What landed" paragraph and "Still open" list to record the capacity-profile-scheduling gap this PR's sibling commit closes. bin/check-file-refs.sh is clean; bin/check-all.sh's full sweep passes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. No quarantined test changed outcome since the previous push. Updated for |
The capacity-profile runner added for the Codex finding on #473 copied the iteration loop of exp-measure-large-instances-failure-rate.sh verbatim - the maven call, the DID-NOT-RUN guard, the progress and missing-keys extraction and the tally row. Two copies of a classifier drift silently, which is the exact history bin/lib/chaos-experiment-common.sh's header records for the trajectory parser. The loop now lives there as pc_measure_profile_failure_rate, with the row-stamping and coordinator-logger reasoning moved with it, and both runners call it. Exercised with stubbed pc_run_performance and pc_failsafe_stats: the tally rows come out in the same shape as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
…on to #408 #466 merged while the burn-down was being written: the revoke-path commit now drains the work mailbox first in transactional mode, its proof left quarantine with it, and the README caution is gone. The burn-down ticks it and moves it from "fixed in the queue" to "fixed on master". Its commit body records a collision with #408 on tryCommitOffsetsOnRevoke, to be resolved by whichever landed second. That is now #408, so its tier 2 line says so - it was already the one PR in the queue needing work rather than a merge. The #473 line no longer orders itself after #466; it clears the two quarantine entries that remain, which the release guard blocks on. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…eave the gating lane and the quarantine MultiInstanceRebalanceTest.largeNumberOfInstances was quarantined while its residual failure had no mechanism. #444 measured one: the Kafka consumer group protocol under this profile's churn rate, not a PC defect. The chaos monkey restarts members faster than a join phase completes, a LeaveGroup sent while the member's own JoinGroup is pending is answered only when the phase completes, and one phase was observed open for 17s - during which consumer.poll() returns nothing to ANY member, which is the FLAT count the detector fires on at 12s. No coordinator request was ever slow. Every PC-side candidate was refuted by measurement. 4 in 60 on the Linux runner, 0 in 22 on an M2 desktop. This implements what docs/inflight/test-largenumberofinstances-cannot-gate-a-merge.md decided (option 1, 2026-09-07): nothing about the test is repaired, because nothing about it is broken. Its legitimate output is a pass RATE over many runs, and a required check that fails one run in fifteen for a reason no change to PC can move blocks merges while carrying no information about the change being merged. WHAT MOVED. A second tag, @tag("capacity"), on all three capacity profiles - largeNumberOfInstances, cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance. They move together because they share the profile shape, and leaving two behind would put the same measurement on both sides of a gate. bin/performance-test.sh - the required Performance Tests check - now passes -Dexcluded.groups=quarantined,capacity. The tag is also in the pom's default excluded.groups and in the three CI wrappers' hardcoded copies of that list, so a future capacity profile that forgets the performance tag is still out of the gating lanes. The experiment runners are untouched and still select them: pc_run_performance passes -Dincluded.groups=performance with an EMPTY exclusion, which is why the profiles keep their performance tag rather than trading it for the new one. @Quarantined and the docs/quarantined-tests.md entry come off in the same commit. A profile that no longer gates does not need quarantining, and leaving both would say two different things about one test - "quarantined" reads as "broken" and this test is not. VERIFIED BY SELECTION, NOT BY THE FLAGS LOOKING RIGHT - the failure mode here is a lane that selects nothing and passes, which this repo has already had once: gating flags, -Dit.test=MultiInstanceRebalanceTest "No tests to run", Tests run: 0 gating flags, -Dit.test=LargeVolumeInMemoryTests Tests run: 6, the lane still measures experiment flags, MultiInstanceRebalanceTest#largeNumberOfInstances Tests run: 1, PASS in 164.4s A BUG THE TAG SPLIT WOULD OTHERWISE HAVE INTRODUCED SILENTLY. capacityProfileVerdict read m.getAnnotation(Tag.class), and @tag is @repeatable - getAnnotation returns null once a second one is present. Adding @tag("capacity") would therefore have reclassified every capacity profile as a correctness gate, so -Dperf.scale would have thrown on exactly the tests it exists for, with nothing red to say so until somebody ran a scaled measurement and read the AssertionError as a product problem. It now reads getAnnotationsByType, and the guard keys on the capacity tag. capacityProfilesAreOutOfTheGatingLane pins which profiles carry which tags, and is untagged so it runs in the gating integration lane. It reads annotations, so it proves the labelling and not the lane's flags - stated in its own javadoc, because the flags are the half no test can hold. NOT DONE, and named rather than left implied: the rate is not yet compared against a rolling median the way bin/check-throughput-regression.mjs does for throughput, so a drift would still have to be noticed by a human reading two artifacts. That is option 1's second half and wants its own change; a fixed bound must not be used for it. scriptedChurnRoundsCompleteWithoutStall, the deterministic correctness twin, is untouched and is what keeps these code paths gated. The profiles' parameters and churn are unchanged, deliberately: the residual rate #444 measured is the baseline to compare against, and backing the churn off would destroy it. The TODO(refactor) asking for the residual-failure attribution is removed - that measurement answered it - and docs/todo-index.md regenerated. TWO FOLLOW-UPS FROM THE CODEX REVIEW ON #473, landed in the same change because each is a consequence of the tag split above. The tag took cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance out of the required Performance Tests lane, but no scheduled or dispatchable runner ever targeted them - every pc_run_performance caller in bin/exp-*.sh and every choice in experiments.yml named only largeNumberOfInstances, verified by grep. So the two ran nowhere, silently - the "test that never runs is not a passing test" trap. bin/exp-measure-capacity-profiles-failure-rate.sh gives both the weekly cadence largeNumberOfInstances already has, wired into experiments.yml as a second schedule slot routed via github.event.schedule, never a gating lane. The iteration loop, the DID-NOT-RUN guard and the tally row live once, in bin/lib/chaos-experiment-common.sh as pc_measure_profile_failure_rate, and both runners call it - a verbatim copy drifts silently, which is the history that library's header records for the trajectory parser. Exercised with stubbed pc_run_performance and pc_failsafe_stats: the rows keep their shape. docs/inflight/test-largenumberofinstances-residual-failures-measured-not-explained.md said outright that its subject had been acted on. Its durable content - the mechanism chain, the two refuted candidate fixes with their control arms, the deterministic reproducer - is migrated to docs/solutions/test-flakiness/large-instances-residual-is-a-join-phase-held-open-by-churn-2026-09-05.md and the note deleted, every citation repointed (bin/, workflows, notes, two Java files and a logback resource). The cannot-gate note's implementation bullet, which still told readers to keep an empty -Dexcluded.groups= on bin/performance-test.sh, is rewritten to the contract the lane now has. A javadoc sentence that quoted the removed TODO marker is reworded so bin/todo-index.sh stops indexing it as one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
0614e84 to
62fafd2
Compare
…ack pressure had stopped fetching RegistrationRaceStaleResidentIT failed across CI for a fortnight on its own SETUP guard - "control thread must reach the mid-loop pause point (offset 25)" - never on the confluentinc#909 assertion it exists to make, so every failure was evidence about nothing. #440 quarantined it on a sighting ledger with no mechanism. This is the mechanism, and it is the test's staging, not PC. DIAGNOSIS. The test saturates the pipeline by gating the user function so nothing ever completes, which makes whatever stage 1 produced permanent: RecordPopulation counts a record as in-shards from insert until removal, and being out at a parked worker does not remove it. Stage 1 produced 12 records against an out-for-processing target of 8 ("with a margin", said the comment), so records-in-shards sat at 12 against WorkManager.isSufficientlyLoaded()'s threshold of 8 for the rest of the run. BrokerPollSystem paused the data partition for back pressure, and nothing could resume it - both resumeIfPaused and maybeWakeupPoller are conditioned on !isSufficientlyLoaded(). The stage-2 records carrying offset 25 were therefore never fetched. It passed at all only by racing. Pausing takes effect at the top of a poll iteration, so a consumer.poll() already in flight when the gate closed went on delivering for up to the 2s long-poll timeout. Winning that race gave a ~14s pass; losing it gave the mute 30s timeout CI kept seeing on unrelated branches, including documentation-only heads. CONTROL ARMS, predictions stated before running. baseline (unchanged) PASS 15.6s A wait for isSufficientlyLoaded() before stage 2 3/3 PASS - PREDICTION REFUTED A' wait for isSubscriptionsPausedForBackPressure() 3/3 FAIL, 36.8-37.4s, exact CI signature B stage 1 produces 8 rather than 12 10/10 PASS, 13.6-14.3s C arm B, ProcessingShard stale replacement disabled FAIL on the 909 signature, 104s Arm A is worth reading: forcing the intake gate shut did NOT reproduce the failure, because the gate closing is not the same event as the partition being paused - only the second closes the window. The refuted arm is what located it. Arms A' and B differ by one record either side of the threshold, in the same position, and the outcome flips. Arm C is what says the repair did not cost the reproduction: it still goes red against the defect, on the right assertion, naming exactly the 25 keys k-25..k-49 that the original write-up recorded. THE FIX. STAGE_1_RECORDS is derived from MESSAGE_BUFFER_SIZE rather than written as a literal, so the two cannot drift; at exactly the target the comparison is strictly greater-than and the gate stays open. The precondition is asserted immediately before the stage-2 produce, naming the gate and its arithmetic - it was implicit, and the failure was mute, which is why eleven sightings over three weeks could establish only that it was master-state. The guard now reads the intake state AFTER its wait and reports it either way, including the poller's own pause observation. The class javadoc records this as a fourth precondition belonging to the staging rather than to the defect. The anonymous PCModule is now a named local class so the test can reach BrokerPollSystem through the module's protected seam for that diagnostic. REJECTED. Raising messageBufferSize to 12 (same knife-edge, bigger numbers, and it moves the scan-suppression arithmetic the reproduction depends on). Producing all 50 records up front (the control thread would park before any work is taken, so saturation could never be established). Loosening the 30s guard, which would have hidden the fact that the records could not arrive at all. SAME-DEFECT-CLASS SWEEP - the class is "a test that saturates PC's pipeline and then expects more records from the broker". One instance, the one fixed. Checked and dismissed: BrokerPollerBackpressureTest (the inverse - it asserts the pause, then releases the gate and waits for the resume BEFORE producing 10 more); RetriesTest and PartitionOrderProcessingTest (buffer overrides but no second-stage produce while workers are blocked); DrainCloseTest, DrainingMemberRebalanceIT, TransactionTimeoutsTest, TransactionalEagerProcessingIT and Rebalance857CommitSyncDeadlockProbeBase (latch plus multiple produces, but no pinned static load factor and no permanently-held surplus). @Quarantined and the docs/quarantined-tests.md entry are deleted together, per rule 3. The flake's row and its closed section leave docs/inflight/test-untracked-ci-flakes.md in the same change: the durable diagnosis is the solutions write-up, and docs/inflight/AGENTS.md says a closed item is removed with what outlives it migrated first, not rewritten into a FIXED narrative. A one-line pointer, the file's own convention for a closed entry, is what replaces it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
…eave the gating lane and the quarantine MultiInstanceRebalanceTest.largeNumberOfInstances was quarantined while its residual failure had no mechanism. #444 measured one: the Kafka consumer group protocol under this profile's churn rate, not a PC defect. The chaos monkey restarts members faster than a join phase completes, a LeaveGroup sent while the member's own JoinGroup is pending is answered only when the phase completes, and one phase was observed open for 17s - during which consumer.poll() returns nothing to ANY member, which is the FLAT count the detector fires on at 12s. No coordinator request was ever slow. Every PC-side candidate was refuted by measurement. 4 in 60 on the Linux runner, 0 in 22 on an M2 desktop. This implements what docs/inflight/test-largenumberofinstances-cannot-gate-a-merge.md decided (option 1, 2026-09-07): nothing about the test is repaired, because nothing about it is broken. Its legitimate output is a pass RATE over many runs, and a required check that fails one run in fifteen for a reason no change to PC can move blocks merges while carrying no information about the change being merged. WHAT MOVED. A second tag, @tag("capacity"), on all three capacity profiles - largeNumberOfInstances, cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance. They move together because they share the profile shape, and leaving two behind would put the same measurement on both sides of a gate. bin/performance-test.sh - the required Performance Tests check - now passes -Dexcluded.groups=quarantined,capacity. The tag is also in the pom's default excluded.groups and in the three CI wrappers' hardcoded copies of that list, so a future capacity profile that forgets the performance tag is still out of the gating lanes. The experiment runners are untouched and still select them: pc_run_performance passes -Dincluded.groups=performance with an EMPTY exclusion, which is why the profiles keep their performance tag rather than trading it for the new one. @Quarantined and the docs/quarantined-tests.md entry come off in the same commit. A profile that no longer gates does not need quarantining, and leaving both would say two different things about one test - "quarantined" reads as "broken" and this test is not. VERIFIED BY SELECTION, NOT BY THE FLAGS LOOKING RIGHT - the failure mode here is a lane that selects nothing and passes, which this repo has already had once: gating flags, -Dit.test=MultiInstanceRebalanceTest "No tests to run", Tests run: 0 gating flags, -Dit.test=LargeVolumeInMemoryTests Tests run: 6, the lane still measures experiment flags, MultiInstanceRebalanceTest#largeNumberOfInstances Tests run: 1, PASS in 164.4s A BUG THE TAG SPLIT WOULD OTHERWISE HAVE INTRODUCED SILENTLY. capacityProfileVerdict read m.getAnnotation(Tag.class), and @tag is @repeatable - getAnnotation returns null once a second one is present. Adding @tag("capacity") would therefore have reclassified every capacity profile as a correctness gate, so -Dperf.scale would have thrown on exactly the tests it exists for, with nothing red to say so until somebody ran a scaled measurement and read the AssertionError as a product problem. It now reads getAnnotationsByType, and the guard keys on the capacity tag. capacityProfilesAreOutOfTheGatingLane pins which profiles carry which tags, and is untagged so it runs in the gating integration lane. It reads annotations, so it proves the labelling and not the lane's flags - stated in its own javadoc, because the flags are the half no test can hold. NOT DONE, and named rather than left implied: the rate is not yet compared against a rolling median the way bin/check-throughput-regression.mjs does for throughput, so a drift would still have to be noticed by a human reading two artifacts. That is option 1's second half and wants its own change; a fixed bound must not be used for it. scriptedChurnRoundsCompleteWithoutStall, the deterministic correctness twin, is untouched and is what keeps these code paths gated. The profiles' parameters and churn are unchanged, deliberately: the residual rate #444 measured is the baseline to compare against, and backing the churn off would destroy it. The TODO(refactor) asking for the residual-failure attribution is removed - that measurement answered it - and docs/todo-index.md regenerated. TWO FOLLOW-UPS FROM THE CODEX REVIEW ON #473, landed in the same change because each is a consequence of the tag split above. The tag took cooperativeStickyRebalanceShouldNotStall and gentleChaosRebalance out of the required Performance Tests lane, but no scheduled or dispatchable runner ever targeted them - every pc_run_performance caller in bin/exp-*.sh and every choice in experiments.yml named only largeNumberOfInstances, verified by grep. So the two ran nowhere, silently - the "test that never runs is not a passing test" trap. bin/exp-measure-capacity-profiles-failure-rate.sh gives both the weekly cadence largeNumberOfInstances already has, wired into experiments.yml as a second schedule slot routed via github.event.schedule, never a gating lane. The iteration loop, the DID-NOT-RUN guard and the tally row live once, in bin/lib/chaos-experiment-common.sh as pc_measure_profile_failure_rate, and both runners call it - a verbatim copy drifts silently, which is the history that library's header records for the trajectory parser. Exercised with stubbed pc_run_performance and pc_failsafe_stats: the rows keep their shape. docs/inflight/test-largenumberofinstances-residual-failures-measured-not-explained.md said outright that its subject had been acted on. Its durable content - the mechanism chain, the two refuted candidate fixes with their control arms, the deterministic reproducer - is migrated to docs/solutions/test-flakiness/large-instances-residual-is-a-join-phase-held-open-by-churn-2026-09-05.md and the note deleted, every citation repointed (bin/, workflows, notes, two Java files and a logback resource). The cannot-gate note's implementation bullet, which still told readers to keep an empty -Dexcluded.groups= on bin/performance-test.sh, is rewritten to the contract the lane now has. A javadoc sentence that quoted the removed TODO marker is reworded so bin/todo-index.sh stops indexing it as one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
2f15f41 to
5e412c8
Compare
|
@claude review this |
|
Claude finished @astubbs's task in 3m 2s —— View job Review
All four Codex P1 threads from the earlier round are genuinely resolved in the current tree, not just claimed: One new, concrete finding from reading the fix for the "profiles ran nowhere" gap:
Neither of the two new directories is in that list, so on the new Sunday-05:00 schedule slot, the job will run the real test iterations, produce a real Fix is a one-line addition to that loop: Note: I could not modify Minor, non-blocking: Everything else checked out: the sighting-ledger migrations are clean ( |
… the one runner's The second review round on #473 read the fix for the "profiles ran nowhere" gap and found that its output went nowhere instead: the new runner writes tally.tsv under /tmp/cooperative-sticky-rebalance and /tmp/gentle-chaos-rebalance, and the workflow's collect step scanned only the four directories the older runners use. On the new schedule slot the job would have run every iteration, produced both tallies, and uploaded an empty artifact with the "No experiment output directory was found" warning - the outcome the workflow's own header names as the worst one, because it is indistinguishable from a clean result. Both directories are in the loop now. The cannot-gate note's vetted marker still said the decision was not yet implemented, one heading above the section saying it was; refreshed to what is true on this branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
…ntine-registry # Conflicts: # bin/ci-build.sh # bin/ci-integration-test.sh # bin/ci-unit-test.sh # pom.xml
|
Response to the second review round. The collect loop now names both new tally directories (6795039). The finding was right and it was the worst shape: the new schedule slot would have run every iteration and uploaded an empty artifact with the "no output directory" warning, which reads as a clean result. The vetted marker on the cannot-gate note is refreshed to say the decision is implemented and only the trend line stays open, same commit. Nothing else changed. |
#473 promoted #473 merged while this branch was open. It deleted test-largenumberofinstances-residual-failures-measured-not-explained.md, having migrated the mechanism to docs/solutions/test-flakiness/, which is the correct outcome under docs/inflight/AGENTS.md - but it left this branch citing a file that no longer exists, and bin/check-file-refs.sh caught it. Repointed at the solutions write-up, which is now the durable owner, and reworded the two places that spoke of #473 in the future tense. The merge itself was clean; only the citation needed repair. Co-authored-by: Claude Opus <noreply@anthropic.com>
…ntries One conflict: the residual-failures note this branch had appended a dated update to was deleted by #473, which migrated it to docs/solutions/test-flakiness/. The note stays deleted; the update - that #431 closed as superseded by this branch and the refutation stands unchanged - follows the content as a citation note beside the #431 reference in the write-up, per docs/citations.md. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
One conflict, inherited from the #481 tip this branch carries: the residual-failures note #481 had appended to was deleted by #473, which migrated it to docs/solutions/test-flakiness/. The note stays deleted here; the dated update that followed it lives on #481 as a citation note in the write-up and arrives with that PR. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Master moved two further commits while the previous merge was being verified, both landing in this branch's area: #470 (an async commit counts as committed when the broker answers) and #473 (two quarantine entries cleared by fixing what they were about). Two conflicts, both resolved keeping each side's intent. - `state/PartitionState.java`, `onOffsetCommitSuccess` and `getCommitDataIfDirty`. #470 makes an acknowledgement record the offset always and mark the partition clean only when it answers the offer the partition last made; this branch had the completed-but-uncommitted ledger's `snapshotForCommit()` / `onCommitSuccess()` pair on the same two methods. Both kept. The ledger's snapshot is taken beside the offer, and the trim now sits INSIDE master's `committed.equals(offerLastMadeForCommit)` arm rather than running unconditionally: the snapshot belongs to that offer, so only the answer to that offer may retire what it carried - the same rule #470 introduces, applied to the ledger. This changes nothing in practice, because the ledger is a no-op outside transactional commit mode (`PartitionState`'s constructor gates it on `isUsingTransactionCommitMode`) and the transactional path acknowledges inline with the map it just collected, so that arm is always the one taken where the ledger is real. Said in a comment at the site. - `docs/inflight/test-untracked-ci-flakes.md`. #473 fixed `RegistrationRaceStaleResidentIT`'s setup guard and removed its row and its prose section, adding the "also fixed and out" paragraph; that removal is taken. This branch's `AmbientProbeExtension` row - master's own, with this branch's #225 and 2026-09-03 measurements folded in by the previous merge - is kept, as is its corroboration paragraph in the surviving prose section, and so are the four rows master has never had. Verification: `./mvnw -Pci -pl parallel-consumer-core -am` compiles; 170 unit tests across the producer, transactional, `PartitionState*` and work-manager classes pass with 0 failures and 0 errors from fresh surefire reports - including #470's own `PartitionStateAcknowledgedCommitOffsetTest` and `MockConsumerAsyncCommitCallbackFailsTest`. `bin/check-all.sh`: 16 passed, 0 failed. No Docker-backed test was run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
Master landed the async-commit acknowledgement (#470, silent loss on the default commit mode) and the quarantine clearance (#473), which empties docs/quarantined-tests.md so the release guard no longer blocks on it and moves the capacity profiles behind a measured tag. With #466 and #468 already in, tier 1 has four of its seven merged; what is left is #469, #481, #480 and #477. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
and #473 Two conflicts. PartitionState: #470 counts an async commit as committed only when the broker answers the latest offer, and #410's ledger trims on commit success; joined so the ledger snapshots before the offer is made and is trimmed only on the answer to that offer - a stale answer to an older offer leaves the newer snapshot in place, so a replay can only over-replay, never under. The flake ledger takes master's updated rows and #473's retirement of the section it fixed, and keeps this branch's three later sightings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
…s test's calibration Review finding on #486, raised as "worth knowing, not a defect": ClosingMemberRebalanceIT's javadoc already records the identical sabotage shape and near-identical numbers under its Proven red block, which arrived on this branch with #473's merge some minutes after the entry here was written. Two records of one shape is how a duplicate starts drifting, and the repo's rule is to name the owner in the pointer rather than let a reader who found one stop looking. So the entry now says which record owns what: the javadoc owns what the test is calibrated to detect, and the rows here are an independent re-run on a later master answering a different question - whether the PC-side story still holds now. It also warns the reader not to read the agreement between them as one result cited twice, which is the misreading the two-places shape invites. No claim changed and no number moved. Co-authored-by: Claude Opus <noreply@anthropic.com>
…-fix Picks up #468, #470, #471 and #473. Read for whether any of them moves the ground under this branch's evidence, since that is exactly what #466 did to the 2026-08-31 grid and is why this branch had to re-calibrate: - #470 is the one that looks like it might. It changes when an ASYNC commit counts as committed - AbstractOffsetCommitter, ConsumerOffsetCommitter and PartitionState - so it is the commit path, but not the revoke path. This branch's grid measures the poll-versus-control lock cycle in tryCommitOffsetsOnRevoke, under the synchronous consumer mode, and the probe's failing edge is the commit-response timeout rather than the clean-marking that #470 rewrites. - #468 (WorkContainer identity equality), #471 (the commit-response timeout soak) and #473 (quarantine entries) touch neither. Checked rather than assumed: the diff of AbstractParallelEoSStreamProcessor.java across this merge contains no change to tryCommitOffsetsOnRevoke, commitLock, onPartitionsRevoked or commitOnRevokeViaTheControlThread - the file is not in the merged range's changed set at all. So the re-calibration stays pinned at 6aab3ff, and the head it names is still the right one to state. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
#486 The scope note still said the async-unordered rebalance stall was blocked on progress-tracker instrumentation that did not exist. #486 shows it was never blocked: the diagnostic has been wired since #444, the mechanism was already measured and promoted to master by #473 (a join phase the coordinator holds open, during which poll returns nothing to any member, then recovers), and two claims in the family note were false against master. The PC half was re-verified with a one-term control arm after the revoke and close seam moved. Not a defect; struck from the list and from the replay queue. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…nd the PC half is re-verified with a control arm (#486) The 857 register's fifth open item - a rebalance stall in MultiInstanceRebalanceTest.largeNumberOfInstances under PERIODIC_CONSUMER_ASYNCHRONOUS/UNORDERED - read as blocked on progress-tracker instrumentation that does not exist. It is not blocked, and has not been since #444: withDiagnostic is wired on master, and the stall has been reproduced repeatedly since, not never. Both stale claims were still in the section, so anyone picking the item up inherited an investigation that had already finished. The mechanism is measured, and its durable owner is the docs/solutions/test-flakiness write-up that #473 promoted: the chaos monkey restarts members faster than a join phase completes, a LeaveGroup sent while the member's own JoinGroup is pending is not answered until that phase completes, and one phase was observed held open for 17s. While it is open consumer.poll() returns nothing to any member, survivors included - the FLAT count. No coordinator request was slow in any failing run, and every PC-side candidate was refuted by measurement. So the verdict is neither wedge nor harness artefact: the freeze is real and fleet-wide, it recovers when the phase completes, and what turns it red is the detector's 12s window closing inside a real protocol freeze. Master has moved through the revoke and close seam since that measurement (#451, #466, #468), so the PC half was re-asked rather than inherited. Two arms on ClosingMemberRebalanceIT differing by exactly one term - a 20s sleep at the top of BrokerPollSystem's doClose(), which holds a member not-polling and not-left, the defect shape itself. Predictions written before each run: clean master - predicted PASS - 5/5 pass, 35.4s, load average 5.1-7.1 sabotaged - predicted FAIL on the close bound - 5/5 fail, "expected to be less than: 10.0 but was 20.009590667", load average 13.0 The sabotage arm is what makes the clean arm mean anything: every property this test asserts already holds on master, so a green alone cannot say whether the test can see the failure - the VOID-replay trap #485 recorded. The test's own javadoc records the same sabotage shape under its Proven red block, and the register names it as the owner of what the test is calibrated to detect; the rows here are an independent re-run on a later master answering a different question - whether the PC-side story still holds now - and are not that result cited twice. One finding that is new rather than a re-derivation: the sabotaged arm emits this section's own signature line verbatim - ZOMBIE_MEMBER/REBALANCE_BLOCKED, dwelling in PreparingRebalance for 15s. A PC-side hold produces it and so does a coordinator holding its join phase open with PC off the critical path, so that line does not discriminate between the two stories, and a future sighting has to be told apart by what the closing members' threads are in. Deliberately not run: a local replay of the capacity profile. It reproduces at 0 in 22 on an M2 desktop against about 1 in 15 on the Linux runner, so a green here would have carried no information, and the box was at load 5-14 throughout - a confound, not a control. No product code changes. The vetted marker carries a PROPOSED close for the fifth item only, per docs/inflight/AGENTS.md's owner gate for a stall-impact note, alongside #485's PROPOSED close for the revoke-path deadlock line; the fourth item and #119 are untouched. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
Clears two of the three entries in
docs/quarantined-tests.mdby fixing what they were about, not bywaiving anything. The
0.6.0.0release guard blocks while that file is non-empty.After this PR the registry is empty. A third entry,
ProducerManagerTest.aRevokeTimeCommitIncludesTheOffsetOfEveryRecordItAlreadyProduced, stood in the registry when this work was scoped; #466 fixed that defect on master while this PR was in review, and master is merged in here. With the two entries below gone, no@Quarantinedannotation remains in the tree, and the release workflow's "no release while tests are quarantined" block clears.Description
1.
RegistrationRaceStaleResidentIT- diagnosed, fixed, un-quarantinedQuarantined by #440 on a sighting ledger with no mechanism. It failed on its
own SETUP guard -
control thread must reach the mid-loop pause point (offset 25)- and never on theconfluentinc#909 assertion it exists to make, so every failure was evidence about
nothing.
The mechanism is the test's staging, not PC. The test saturates the pipeline by gating the user
function so nothing ever completes, which makes whatever stage 1 produced permanent - a record out at
a parked worker is still counted in shards. Stage 1 produced 12 records against an out-for-processing
target of 8 ("with a margin", said the comment), so records-in-shards sat at 12 against
WorkManager.isSufficientlyLoaded()'s threshold of 8 for the rest of the run.BrokerPollSystempaused the partition for back pressure and nothing could resume it - both
resumeIfPausedandmaybeWakeupPollerare conditioned on!isSufficientlyLoaded(). The stage-2 records carrying offset25 were never fetched. It passed at all only by racing a
consumer.poll()already in flight when thegate closed, which kept delivering for up to the 2s long-poll timeout.
Control arms, predictions stated before running (M-series desktop, Docker):
isSufficientlyLoaded()before stage 2isSubscriptionsPausedForBackPressure()before stage 2ProcessingShardstale replacement disabledk-25..k-49Arm A is worth reading: forcing the intake gate shut did not reproduce the failure, because the gate
closing is not the same event as the partition being paused. The refuted arm is what located the real
window. A' and B differ by one record either side of the threshold and the outcome flips. C is what
says the repair did not cost the reproduction.
The fix:
STAGE_1_RECORDSis derived fromMESSAGE_BUFFER_SIZEso the two cannot drift; theprecondition is asserted before the stage-2 produce instead of left implicit; the guard reports the
intake state after its wait, so a future failure says which of "slow broker" and "records never
fetched" it was. Write-up:
docs/solutions/test-flakiness/the-setup-guard-was-waiting-on-records-back-pressure-had-stopped-fetching-2026-09-07.md.2.
MultiInstanceRebalanceTest.largeNumberOfInstances- measured, so it leaves the gateNothing here is repaired, because nothing is broken. #444 measured the
residual as the Kafka consumer group protocol under this profile's churn rate - 4 in 60 on the Linux
runner, 0 in 22 on an M2 desktop, no coordinator request ever slow, every PC-side candidate refuted.
Its legitimate output is a pass rate, and a required check that fails one run in fifteen for a reason
no change to PC can move blocks merges while carrying no information about the change being merged.
This implements the decision already recorded in
docs/inflight/test-largenumberofinstances-cannot-gate-a-merge.md(option 1): a second tag,@Tag("capacity"), on all three capacity profiles;bin/performance-test.shexcludes it; theexperiment runners, which pass an empty exclusion, still select them.
@Quarantinedand the registryentry come off in the same commit - a profile that no longer gates does not need quarantining, and
"quarantined" reads as "broken".
Verified by what the lanes actually selected, because the failure mode is a lane that selects
nothing and passes:
A bug the tag split would otherwise have introduced silently:
capacityProfileVerdictreadm.getAnnotation(Tag.class), and@Tagis@Repeatable- that returnsnullonce a second one ispresent, so every capacity profile would have been reclassified as a correctness gate and
-Dperf.scalewould have thrown on exactly the tests it exists for, with nothing red to say so. Itnow reads
getAnnotationsByType.scriptedChurnRoundsCompleteWithoutStallis untouched and is what keeps these code paths gated. Theprofiles' parameters and churn are unchanged, deliberately - the measured rate is the baseline.
Not done, and named rather than implied: the rate is not yet compared against a rolling median the
way
bin/check-throughput-regression.mjsdoes for throughput. That is option 1's second half and wantsits own change.
3. Codex review follow-ups, and the master merge
The Codex review raised four P1 threads, all verified and applied, each answered in its thread with the commit:
largeNumberOfInstanceshad a scheduled runner, socooperativeStickyRebalanceShouldNotStallandgentleChaosRebalancewould have run nowhere.bin/exp-measure-capacity-profiles-failure-rate.shand a second weekly slot inexperiments.ymlnow measure them on the same cadence, sharingbin/lib/chaos-experiment-common.shwith the existing runner.docs/solutions/test-flakiness/large-instances-residual-is-a-join-phase-held-open-by-churn-2026-09-05.mdand deleted, with every citation repointed.The branch was re-cut at merge prep into two atomic commits, one per workstream above, with the review fix-ups folded in; the pre-re-cut tip is preserved as
backup/pre-recut-473. Master is merged, not rebased, whenever it moves under the branch.Second review round: the collect step in
experiments.ymldid not scan the two directories the new runner writes to, so the new schedule slot would have uploaded an empty artifact with a warning that reads as a clean result. Both directories are in the loop now, and the cannot-gate note's vetted marker is refreshed to say the decision is implemented. The latest master merge carries master's newsoaktag besidecapacityin the four exclusion lists.Same-defect-class sweep
The class is "a test that saturates PC's pipeline and then expects more records from the broker".
One instance, the one fixed. Checked and dismissed:
BrokerPollerBackpressureTest(the inverse -asserts the pause, then releases the gate and waits for the resume before producing 10 more);
RetriesTestandPartitionOrderProcessingTest(buffer overrides, no second-stage produce whileworkers are blocked);
DrainCloseTest,DrainingMemberRebalanceIT,TransactionTimeoutsTest,TransactionalEagerProcessingIT,Rebalance857CommitSyncDeadlockProbeBase(latch plus multipleproduces, but no pinned static load factor and no permanently held surplus).
Prior art, including what returned nothing
node bin/inflight.mjs prior-art RegistrationRaceStaleResidentIT awaitPausePoint "pause point"over646 refs returned the 909 third-precondition solution doc, the integration-gate wall-time plan, and
four in-flight notes - all read.
node bin/inflight.mjs codecov testreturned nothing for bothRegistrationRaceStaleResidentITandlargeNumberOfInstancesin a 1446-test corpus, while the samecommand lists 226 integration tests and resolves the tool's own worked example - so the gap is
specific to those two, most likely because the quarantine and performance lanes do not upload.
docs/quarantined-tests.mdcited that command as this test's ledger; the ledger note now records thatit returns nothing.
Checklist
docs/quarantined-tests.md,docs/ci.md,docs/refactoring.md,docs/todo-index.md(regenerated), twodocs/inflight/notes, and a newdocs/solutions/test-flakiness/write-upRegistrationRaceStaleResidentIT(the fix plus its precondition assertion), andMultiInstanceRebalanceTest#capacityProfilesAreOutOfTheGatingLaneis newdocs/inflight/notes this work resolves already existed and are updated in place with the diagnosis and the outcome, which is what a working note is for; a third note would be a duplicate@claude review thison the PR insteadVerification
bin/check-all.sh- 16 passed, 0 failed (3 nothing-in-scope, 5 PR-scoped skips)bin/check-quarantine-registry.shandbin/check-quarantine-owners.shgreen on each commit'stree, not just the tip - the registry edit is split so neither commit leaves it inconsistent
bin/ci-unit-test.sh- full suite green across all 11 modulesRegistrationRaceStaleResidentIT10/10 on the fixed build; 3/3 red on the forced control arm; redon the 909 signature with the defect reinstated
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum