Repository navigation
ci(integration): shard the gate into a heavy set and a catch-all, and split the 857 probe - #442
Conversation
What changed for a user: nothing yet - an investigation, two corrected claims, and a flake
sighting. The measured optimisation follows on this branch.
The Integration Tests lane is the PR build's critical path. This records what it is actually
made of, and the result of the first experiment against it.
THE FINDING. Raising failsafe forkCount from 4 to 6 makes the lane SLOWER - 469s of failsafe
against a 420s baseline - and it destabilised a test that had never failed before. A 2x2
factorial over forkCount {4,6} x the 857 probe {whole, split}, each arm's prediction written
into its commit message before it ran:
baseline 4 forks, whole 420s failsafe 1528s work 339s tail pass
H1 6 forks, whole 469s 1691s 332s FAIL
H2 4 forks, split 424s 1533s 161s pass
H3 6 forks, split 379s 1710s 160s FAIL
The model predicted H1 340-380 and H3 260-300. It was wrong because it assumed class_time_total
is invariant to forkCount. It is not: the same tests cost 11% more CPU time at six forks. More
forks manufacture work, and contention degrades packing on top of that.
So both parallelism directions for this lane are now measured and closed - thread-parallelism in
2026-07, fork oversubscription here. The remaining lever is work reduction, not scheduling.
A TRAP WORTH THE COMMIT MESSAGE. H1 and H3 failed at verify, so Maven never built the other ten
modules and their job_seconds omit ~120s the passing runs paid. H1's job_seconds reads 548
against the 620s baseline while its failsafe was 49s worse. Ranked on job_seconds, the most
harmful arm of the four wins. Rank this lane on core_failsafe_seconds, and refuse to rank a
sample whose build did not succeed.
TWO STALE CLAIMS, corrected here because both are load-bearing and silent:
- parallel-consumer-core/pom.xml withheld a per-fork log directory from failsafe because "it
declares no forkCount, so it runs at the default of 1". maven.yml passes -DforkCount=4 and
failsafe pins no <forkCount>, so it takes that bare property - the -DforkCount vs
-Dsurefire.forkCount trap the same comment warns about. Four JVMs already share one flat
pc.log.dir, into appenders the comment says lose writes under exactly that condition. The flat
path stays, on the half of the reason that survives; the false premise does not.
- bin/ci-integration-test.sh said "the ci profile runs sequentially on GitHub-hosted 2-core
runners", which describes the unparameterised script rather than what CI invokes.
Also records a seventh sighting of the RegistrationRaceStaleResidentIT setup-guard flake, seen on
one of the measurement arms. It is deliberately NOT attributed to fork pressure: codecov shows the
identical assertion failing earlier the same day on an unrelated branch at forkCount=4. The
genuine fork-pressure casualty was ManagedPCInstanceLifecycleTest on the sibling arm, which had
passed every recorded run before this. Conflating the two would waste the next reader's time.
An unverified premise is flagged rather than asserted: the runner reports ubuntu-24.04, and
GitHub moved public-repo runners to 4 vCPU in early 2024, while docs/self-hosted-runner.md,
docs/ci.md and the prior unit-gate run all reason from "2-core" - that run built a 2-CPU Docker
replica to imitate this runner. Nothing here read nproc, so it is named as cheap to settle, not
concluded.
…nd what is still open What changed for a user: nothing - a working note. Carries the part of the ce-optimize run that is still moving, so the next session inherits it rather than re-deriving it: which levers are now closed by measurement (both parallelism directions), which changes are built and unmerged and on which branches, and the two traps that cost real time here - rank this lane on core_failsafe_seconds rather than job_seconds, because a build failing at verify skips ten modules and its job_seconds omits ~120s; and the scratch space lives under .context/, which is gitignored and does not travel with the branch. Separate commit rather than an amend: the first commit is unpushed and amending it would have been safe, but the repo's history-rewrite gate asks before rewriting and there was no reason to answer it when an ordinary commit does the same job.
…der matters What changed for a user: nothing - a working note. Sharding the Integration Tests lane across runner jobs, the way the Chaos Pain Suite already is, is the lever that would actually halve the PR build's critical path. It is deferred rather than undecided, and the note records the measurement that sets the order. Within-job overlap is exhausted: 1528s of test time runs in 420s on four forks, about 91% parallel efficiency, and six forks fell to 75% while inflating total work 11%. So more overlap means more JOBS. What stops that being the first move is that each shard re-pays the ~136s of serial build - testCompile 60s, compile 42s, javadoc 14s, delombok 8s, Truth codegen 7s - so a 4-way split spends roughly 400 extra runner-seconds before saving anything. And the conversion rates differ: because four forks already overlap the waiting, cutting 3.6s of test time buys 1s of wall, while cutting 1s of serial build buys a full second. The build reduction is worth ~3.6x per second AND it is exactly the cost sharding multiplies. Doing it first is not a detour from sharding - it is what makes sharding pay.
What changed for a user: nothing - two sentences reworded in a working note. The note said "This branch carries a ce-optimize run" and "What has LANDED on this branch", both of which name nothing once the branch is gone. Rewritten to name #439 in the past tense, and marked post-merge: checked so the gate can tell an attestation from an oversight. Caught by check-branch-self-reference.sh in the repo: hygiene lane, which failed on both pushes of this branch while every other branch that day was green - so the attribution needed no guessing. WHY IT REACHED CI, since the gate is not obscure and AGENTS.md already names the sweep that runs it: bin/check-all.sh was run and passed - and THEN two more files were added, with only check-inflight-tags.sh and check-file-refs.sh run against them. A sweep whose whole value is that the set cannot drift from what you remembered was made to drift by remembering which gates the new files "needed". The rule is the sweep runs last, after the final edit, not once per session.
What changed for a user: the Integration Tests lane no longer builds a javadoc jar, a sources jar or delombok output for eleven modules on every run. Nothing in a test lane consumes any of them. TAKEN ON FIRST PRINCIPLES, NOT ON A MEASUREMENT, and that distinction is the finding. Verified by counting artefacts in the CI log: a run with these flags builds 0 javadoc jars against 9 without, and delombok reports skipping 11 times against 2. It provably does less work, so it cannot be slower. The saving is around 10s of a ~600s job and this lane's wall time CANNOT resolve it - three concurrent samples of identical code spread 119 seconds, roughly eleven times the effect. The first cut of this change carried a bug that no timing could have caught: -Dsource.skip=true is not a property maven-source-plugin reads - it wants maven.source.skip - so sources jars were still built in both arms while the numbers looked the same either way. Found by counting jars. The script's header now says to verify these three properties by counting artefacts rather than by timing the job. WHAT ELSE THIS SETTLES, all recorded in docs/plans/2026-09-03-001-investigate-integration-gate-wall-time.md: - The noise floor of this lane is 119s across three CONCURRENT samples of identical code. The postscript retracts an earlier claim in the same document that prebuild_seconds cleanly resolved a 10s effect; the control has since moved further than the effect. Incremental optimisation of this job is not measurable, which is an argument for sharding rather than more tuning. - Replacing the 20s compaction sleep with a poll on the log start offset is a proven NO-OP: it logged "Compaction did NOT advance" four times out of four, never fired, and paid the full deadline anyway. Dropped rather than merged - log compaction need not advance the log start offset, so the detector was wrong. The blind spot it exposed is real and now tracked in docs/inflight/test-compaction-wait-has-no-observable.md: that sleep asserts nothing, so a run where compaction never happened is indistinguishable from one where it did. - compiler:testCompile, 60s and the largest remaining serial cost, cannot be skipped: the integration sources import 19 classes out of src/test/java. The reason is written into the script so it is not re-attempted.
What changed for a user: the Integration Tests lane runs as two jobs instead of one. Projected critical path ~498s against ~620s, about -20%, at the cost of one extra runner's worth of minutes. THE SHAPE, and why it is not the chaos suite's four balanced bins. `bin/ci-integration-test.sh` owns a single list, HEAVY_CLASSES; INTEGRATION_SHARD=heavy runs exactly those, and INTEGRATION_SHARD=rest runs everything else by NEGATION of the same list. Both matrix entries go through the one script, so the two shards cannot disagree about what is where, and unset still runs the whole suite for local callers. A balanced N-way split has to be re-sized whenever the suite changes and fails silently when it is not: a new class belongs to no bin, stops running, and nothing goes red. Defining the catch-all by subtraction inverts that - a new test runs by default, and the only way to lose one is to name it in HEAVY_CLASSES and then delete it, which the report check turns into a hard failure. It is also a list short enough to reason about, which is the property that makes it maintainable. THE HEAVY SET IS ONE CLASS, because one class is the constraint. Rebalance857CommitSyncDeadlockProbeIT is @RepeatedTest(20) at ~342s and forks cannot split a class, so whichever shard holds it cannot finish sooner than that - which is also why the saving is ~20% and not more. Splitting that class four ways (measured green on optimize/ig-exp002-probesplit, not adopted) moves the floor to PartitionStateCommittedOffsetIT's ~160s and roughly doubles the benefit, at which point the heavy set should be RE-DERIVED from measured per-class times rather than extended by guess. A SHARD THAT RAN NOTHING MUST NOT READ AS A PASS. -Dfailsafe.failIfNoSpecifiedTests=false is required here - the reactor builds ten modules containing none of these classes, and without it the first one fails the build before the requested tests run, the same trap bin/chaos-test.sh documents. That flag also means Maven exits 0 whether the selection matched everything or nothing, so the script asserts afterwards: heavy must have produced a failsafe report for every named class, rest must have produced some reports and none for the named ones. The two assertions are deliberately opposite - a renamed class fails the heavy shard loudly while the catch-all quietly keeps running the test, which is the safe direction. BLOCKING PREREQUISITE, NOT IN THIS DIFF. "Integration Tests" is a required status check in the master ruleset and "Integration Tests (heavy)" is not. Adding a job does not add a requirement, so merging this without updating the ruleset makes the 857 probe NON-GATING - red on it, mergeable anyway. There is no naming trick that avoids it: each matrix entry gets its own check context, so whichever job keeps the required name, the other is ungated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
What changed for a user: the catch-all integration shard now runs 42 classes instead of 168.
FOUND BY MEASURING, NOT BY REVIEW, AND IT PASSED THE WHOLE TIME. The first cut selected the
catch-all with -Dit.test=!Rebalance857CommitSyncDeadlockProbeIT. Setting it.test REPLACES
failsafe's <includes>, and both test source roots compile into the same target/test-classes - so
the negated form silently dropped the `**/integrationTest*/**` restriction and ran the entire UNIT
suite under failsafe as well.
Measured on CI: 168 classes and 981 tests instead of 42 and 204, 2283s of test work instead of
~1470s, critical path 782s against a 620s single-job baseline. So the split made the lane WORSE,
and the shard reported success throughout - running more tests than you meant to fails nothing.
Nothing would have caught this. The shard's own checks ask whether the right tests RAN, and the
failure was 126 extra ones. The harness gate is `test_classes_run >= 42`, and 168 satisfies it -
a floor cannot see a ceiling. RunLengthEncoderTest, a unit test, appearing in the failsafe
per-class timings is what gave it away, on a run whose only visible symptom was being slow.
THE FIX, in three parts:
- The root pom's failsafe block gains <excludes> driven by ${it.excluded.classes}, defaulted to a
pattern matching nothing. An <excludes> leaves <includes> intact, so the catch-all is the
complement of HEAVY_CLASSES *within the integration packages*, by construction rather than by
hoping. bin/ci-integration-test.sh is its only caller.
- The rest shard uses that property instead of a negated it.test.
- A provenance guard: every failsafe report in either shard must be for a class in an
integrationTest package. This is the check that would have caught it, and it asks about where
the tests came from rather than how many there were - the axis the count-based gates cannot see.
The heavy shard still uses -Dit.test, which has the same include-replacing behaviour, but its
list is exact class names and the provenance guard now covers it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
Dependency ReviewThe following issues were found:
License Issues.github/workflows/maven.yml
OpenSSF Scorecard
Scanned Files
|
[superseded - a quarantined test changed outcome] 🧪🔒 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 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 #442 +/- ##
============================================
+ Coverage 81.99% 82.25% +0.25%
- Complexity 1465 1473 +8
============================================
Files 95 95
Lines 5133 5133
Branches 500 500
============================================
+ Hits 4209 4222 +13
+ Misses 724 714 -10
+ Partials 200 197 -3
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:
|
|
[superseded - status changed to FLAG] 🟢 Throughput — OKThis branch measured about 8% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: fb5ea93, 6573781, c30aaee, ca9c21b, 77fbba8, 7a8a7f0, 2e2705a, 28c6be6, c813942, 558fcfb Since the previous push: ratio 0.918 -> 0.919, share 1.629 -> 1.628, rate 65929 -> 71694 (+8.7%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for Superseded by a newer throughput report. |
…th having What changed for a user: the Integration Tests lane runs as four jobs, and Rebalance857CommitSyncDeadlockProbeIT's twenty repetitions are spread over four classes instead of one. Modelled critical path ~337s against 620s for the single job. FOUR SHARDS WOULD HAVE BOUGHT NOTHING ON THEIR OWN, which is why the probe split is in the same commit rather than deferred again. Modelled from measured per-class times - and the model reproduced the two-shard measurement to within 3s, predicting 516s against 519s measured: 2 shards, probe intact 516s critical, 870s runner 4 shards, probe intact 516s critical, 1417s runner <- +547 runner-seconds, zero gain 2 shards, probe split 355s critical, 709s runner 4 shards, probe split 337s critical, 1158s runner The probe was @RepeatedTest(20) in one ~356s class and forks cannot split a class, so whichever shard held it WAS the critical path at 516s for any shard count. Splitting it is worth 161s; going from two shards to four is worth 18s. The twenty repetitions, every assertion and the 4s revoke dwell are unchanged - the logic moved to an abstract base and four *IT subclasses declare @RepeatedTest(5) each. THE SHAPE: three NAMED shards plus a catch-all defined by SUBTRACTION, sized by longest-processing-time packing over measured times. A new test lands in the catch-all by default and can never belong to no shard, which is the inversion of N explicit bins - whose failure mode is a new class running nowhere with nothing going red. WHAT KEEPS IT FROM DECAYING, since a partition is only right on the day it was measured: - bin/check-integration-shard-balance.mjs recomputes the optimal partition from RECORDED per-class times and reports how much wall-clock the checked-in one leaves on the table. The same runs that make the lists stale also measure their staleness, so drift becomes a number rather than quiet decay. Advisory by default - 119s of measured noise on this lane is no basis for blocking a merge - with --fail-over <seconds> for a caller that wants it blocking. It also names classes that are in a list but have no recorded history, which is what a rename looks like before a build catches it. Run against live data it already flags the three new probe classes as unrecorded, correctly. - Each named shard asserts a failsafe report for every class it was assigned, so a rename fails that shard loudly WHILE the catch-all keeps running the test. Failing in the safe direction. - No class may appear in two lists, checked on every invocation: it would run twice, be paid for twice, and both shards would pass. - Every failsafe report in every shard must come from an integrationTest package. This is the guard that caught -Dit.test silently running the whole unit suite, and it is the only one that could have - that failure was 126 EXTRA tests, and no "ran at least N" gate can see extra. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
What changed for a user: shards 1 and 2 stop failing on tests that ran and passed. The membership check globbed parallel-consumer-core/target/failsafe-reports. Integration tests also live in parallel-consumer-vertx and the example modules, so VertxConcurrencyIT and CoreAppMetricsIntegrationTest were reported as "produced no failsafe report" while both had run and passed in their own module. Two of four shards went red on the first four-way run for that reason alone - a false RED, in a check whose entire job is to distinguish "did not run" from "ran". Now scans every module's failsafe-reports. The same helper backs all three checks - membership, the catch-all's emptiness test, and the integrationTest-package provenance guard - so none of them can drift back to a single module. Worth noting which way this failed. The guard was written against core because that is where the timings that sized the shards came from, and core is where 41 of the 45 classes are - so it was right about the classes it was tested on and wrong about the four it was not. A check that is correct for the common case and silently wrong for the rest is the shape this repo keeps finding; here it announced itself immediately, which is the good version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
What changed for a user: nothing yet - the bug was introduced and fixed before this branch ran again. Recording it because the mechanism is worth more than the diff. `all_reports | grep -q PATTERN` under `set -o pipefail` reports FAILURE when it MATCHES. grep -q exits the instant it finds one, find takes EPIPE and dies with 141, and pipefail promotes that to the pipeline's status. So report_exists() would have said "no report" for classes that had one - the exact false RED the previous commit was written to fix, reintroduced two lines below it. Caught by bin/check-source-patterns.mjs, whose rule exists because check-review-posted.sh shipped this and reported "no review posted" on four PRs that had one. ShellCheck does not catch it, and it hides well: the writer only reaches EPIPE with more than a pipe buffer left to write, so small inputs pass forever. This tree has 45 report files and would have crossed that line as the suite grew, not on the run that introduced it. Fixed with a herestring - no pipeline, so nothing for pipefail to promote. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…m it What changed for a user: Rebalance857CommitSyncDeadlockProbeIT's twenty repetitions run as four classes instead of one, and the heavy shard is re-sized around that. Modelled critical path ~440s against the 519s this branch's parent measured, and 620s for the single job. WHY THE PROBE SPLIT IS THE WHOLE POINT. The probe was @RepeatedTest(20) in one ~356s class and forks cannot split a class, so whichever shard held it WAS the critical path. Two shards or four made no difference while it was intact - both modelled at 516s. The twenty repetitions, every assertion and the 4s revoke dwell are unchanged; the logic moved to an abstract base and four *IT subclasses declare @RepeatedTest(5) each. THE SPLIT COSTS MORE THAN ARITHMETIC SAYS, and this is the correction worth carrying forward. 356s over four classes was modelled as four 89s classes. Measured, they are 138-166s: the repetitions carry per-class fixed cost - broker and fixture setup - that used to be paid once and is now paid four times. Total test work rose from 1545s to ~2000s. It still wins decisively, because that work spreads across shards while the old 356s tail could not spread at all, but splitting FURTHER would pay less again. FOUR SHARDS WERE BUILT, MEASURED GREEN AT 355s, AND NOT TAKEN. Against two shards' ~440s that is 85s of critical path, for 1318s of runner time against ~880s and ~500s of manufactured work - per-shard fixed costs are paid per shard, the same shape as per-fork costs when forkCount went 4->6. Not worth a 50% machine-time increase and four lists to keep straight instead of one. The arrangement is preserved on ci/shard-integration-four. THE HEAVY SET IS NOW FIVE CLASSES, sized from measured post-split times, and the script carries the guide for maintaining it: move a class only when its own wall exceeds the catch-all's work/forks (bin/check-integration-shard-balance.mjs prints that number live - currently 240s), balance WALLS rather than counts, prefer a set that packs evenly across four forks, treat removal as equal to addition, and re-derive from measurement rather than from reading the code. Every sizing guess in this lane's history has been wrong, including this commit's own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…d and idle What changed for a user: two classes move from the catch-all into the heavy shard. Predicted critical path ~403s against the 450s measured before, at NO extra runner cost - same two jobs, same machine time, less idle. THE HEAVY SHARD HAD FREE CAPACITY AND THE NUMBERS SAID SO. Measured on run 33829977814: heavy 330s against the catch-all's 450s. Heavy was TAIL-BOUND - its slowest class, PartitionStateCommittedOffsetIT at 208s, exceeded its own work/4 of 178s, so three of its four forks were partly idle. Work added to a tail-bound shard costs nothing until its total reaches forkCount x its slowest class: 834s here, against the 710s it was carrying. The catch-all is work-bound at 975s/4, so every second removed from it comes straight off the critical path. RebalanceEoSDeadlockTest and Rebalance857CommitSyncDeadlockProbeIT move across - the two largest in the catch-all, and the modelled optimum. Moving four or more makes it WORSE, because heavy stops being tail-bound and starts paying for the work. This is the guide in this file being used rather than illustrated: rule 1 says move a class only when it bounds the catch-all, and bin/check-integration-shard-balance.mjs prints the threshold. EXPECT ~415-425s RATHER THAN 403s. The model treats per-shard overhead as fixed, and it is not: the catch-all's overhead measured 203s against heavy's 122s, because it runs 40 classes and pays more fixed setup. Moving classes moves some of that with them. The direction is solid; the exact number is not, and the measurement is what settles it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
… no report What changed for a user: the catch-all shard now fails if any integration test class in the tree produced no failsafe report anywhere, in either shard. WHY THIS ONE IS DIFFERENT FROM THE OTHER GUARDS. The existing checks all start from the RUN and ask whether what ran was right - every named class present, no named class in the catch-all, every report from an integrationTest package. None of them can see a test that ran NOWHERE, because a test that runs nowhere leaves no trace to inspect. This check starts from the SOURCE instead, which is the only direction that can find it. AGENTS.md: "a test that never runs is not a passing test, and nothing goes red to tell you." IT NEARLY SHIPPED AS A GATE THAT FAILED EVERY BUILD. The first draft demanded 55 classes against the 38 that actually run, because an integrationTest package holds helpers and fixtures as well as tests. It needs four subtractions - HEAVY_CLASSES, abstract bases, chaos/performance groups, @Quarantined - plus "must declare @Test/@RepeatedTest/@ParameterizedTest". Dry-run against run 33831283169: 37 required, 0 missing. The comment says to dry-run any change to that filter, because getting it wrong makes this a permanent red rather than a check. NOT ALSO ADDED, having been considered and dropped: a static bin/check-*.mjs asserting every integration test sits in a package failsafe collects. TestConventionRules already ArchUnit-enforces exactly that, in every unit run. A second implementation of a rule that already has one is a place for the two to disagree. Also folds the four-shard measurements into docs/inflight/ci-shard-the-integration-gate.md as deferred work rather than a rejected idea - it is faster (355s against 416s) and costs +526 runner-seconds, which is a trade someone could reasonably revisit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
astubbs
changed the title
|
@claude review this Most of the risk here is in Worth your attention, in order:
Lower priority but genuinely uncertain: Not looking for comment on the shard membership itself; it is derived from measured times and will |
|
Claude finished @astubbs's task in 5m 23s —— View job Review: the five guards in
|
…and three modelled figures What changed for a user: nothing - corrections to claims in the note itself. The note said the probe split landed "in the PR stacked on it". That stopped being true when the stacked branch was folded into #442, so it described an arrangement that never shipped, and "the measurements live with those PRs" was plural about one PR. It also compared four shards against two shards' "~440s", which was the MODEL. The measured figure is 416s, so the stated gap of 85s is really 61s and the runner comparison was against ~880s rather than the measured 792s. The table now marks each row modelled or measured, and the shipped row is labelled cumulative rather than reading as a third alternative. Adds the dependency the note only implied: the probe split and the rebalance are not independent wins. The rebalance moves Rebalance857CommitSyncDeadlockProbeIT into the heavy shard, which is a 134s class only BECAUSE the split happened - at its pre-split 356s it would have blown the heavy shard past the catch-all immediately. The split created the granularity the rebalance needed. Found by check-branch-self-reference.sh, which fired for a formal reason - the line named this PR without a post-merge attestation - and the re-reading it forced turned up the substantive errors. The marker is now there with its reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
Three conflicts, resolved hunk by hunk rather than by taking either whole file - `--theirs`/`--ours` take everything and a merge that discards the other side renders as nothing at all in a diff-vs-base review. - `bin/ci-integration-test.sh`: kept THIS branch's sharding blocks (master has no sharding) but took MASTER's javadoc wording, which #439 corrected after review - this branch still carried the "~10s" figure that #439's own postscript retracts as drift. Verified all three survive: the corrected size estimate, #439's testCompile-is-closed note, and the shard guide. - `docs/plans/2026-09-03-...md`: took MASTER for both hunks. Same reason - master carries #439's review fixes (testCompile closed, the size relabelled as an estimate) and this branch predates them. - `docs/inflight/ci-shard-the-integration-gate.md`: kept THIS branch for both. Master describes sharding as "deliberately deferred", which was true when #439 was written and is not now. Checked rather than assumed after resolving: the duplicate-code action pin #439 added survived the auto-merge of maven.yml, and both integration matrix entries are still there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
✅ 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)
|
…our copies What changed for a user: nothing - the same twenty repetitions run as the same four classes. The method body now exists once. `dups: similarity` failed this PR at 89.6% on the four probe subclasses, and it was right to. Each redeclared an identical `@RepeatedTest(5)` method whose only job was to call the parent's `runOneProbeIteration()`, so four files differed by a class name and a javadoc line. That is duplication, not an artefact of the tool's threshold. The fix is the obvious one I had missed: the annotation and the body go in the abstract parent, and JUnit collects inherited non-private test methods, so each concrete subclass runs its five without declaring anything. The subclasses are now one line each. Verified all four still compile to separate classes, which is what makes them independently schedulable. They also share ONE FILE now. Java allows several package-private top-level classes per file, and failsafe collects compiled CLASSES rather than source filenames, so scheduling is unchanged - four forks can still take one each. Four files holding one line each would still have tripped the similarity bound, and would have been duplication for the filesystem's benefit rather than a reader's. Not done instead, and worth saying why: `ignore_directories` is the only exemption this check offers, and pointing it at the integration package would blind it to every integration test to excuse four shims. Raising `fail_above` would weaken the gate globally for the same reason. The check found real duplication; the answer was to remove it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…guarded The check exists so that a class silently ceasing to run fails the build. Three separate filters were letting classes past it, and one of the three was opened by this branch's own parent-class refactor. Established against ground truth, not by inspection: the classes failsafe actually ran in the green run at 4ac1128 (heavy 7, catch-all 38). Before, the check required 37 of those 38. Now it requires exactly 38, with no class demanded that does not run - the failure mode that matters most here, since one false demand fails every build. 1. THE @Quarantined/@tag GREP WAS FILE-WIDE, so one method carrying the annotation exempted every other method in its class. Raised in review as a latent risk; it was not latent. It exempted LoadTest and MultiInstanceRebalanceTest, both of which carry a method-level @tag("performance") beside ordinary tests and both of which run. MultiInstanceRebalanceTest is named in HEAVY_CLASSES so the membership check covered it; LoadTest is in the catch-all and had nothing. Now anchored at column 0, which separates a class-level annotation from an indented method-level one. 2. A TEST CLASS NEED NOT DECLARE ITS TEST METHODS. Requiring a literal @test in the file skipped any class that inherits them - which is what the refactor two commits back created, by moving the probe's @RepeatedTest onto the shared base. The check now also accepts a class extending a type in this tree that declares one. 3. THE CLASS NAME CAME FROM THE FILENAME. Java allows several package-private top-level classes per file and the four probe subclasses share one, but failsafe collects compiled CLASSES - so the filename saw only the first and was blind to the other three. Now every top-level declaration in the file is enumerated, with abstract judged per declaration rather than per file so a file holding a base and a concrete test does not lose the concrete one. 2 and 3 compounded on the same class. Rebalance857CommitSyncDeadlockProbe4IT is in the catch-all rather than HEAVY_CLASSES, so no membership check covered it either: it could have stopped running with nothing going red - precisely the hole this check exists to close, reopened by the commit that removed the duplication. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
|
@codex review this |
check-branch-self-reference flagged the maven.yml comment 0e52229 corrected: it names #442 and carried no post-merge marker. The wording was already past tense - the ruleset change it records is done - so this attests it rather than rewriting it. Second time this gate has passed locally and failed in CI on this branch, for the reason its previous fix recorded: the worktree branch is ci/shard-two-with-probe-split and the PR head is ci/shard-integration-gate, so locally there is no PR for the head and nothing is flagged. GITHUB_HEAD_REF=ci/shard-integration-gate reproduces CI's view - red before, green after, same command. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
[superseded - a quarantined test changed outcome] 🧪🔒 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 Since the previous push: Updated for Superseded by a newer quarantine lane report. |
… setup-guard flake Same guard, same signature - `control thread must reach the mid-loop pause point (offset 25)` - in the catch-all shard at 2ee8419, three heads after the eighth at 7075b26 on the same PR. That is two of the four catch-all runs this PR has had, which is worth writing down as a rate rather than a count: on this lane it is now closer to one in two than to the one in several the earlier sightings suggested. Recorded, not diagnosed, and not retried in-build; the ledger is where the signal goes, per AGENTS.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
🧪🔒 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 No quarantined test changed outcome since the previous push. Updated for |
[superseded - status changed to OK] 🟡 Throughput — FLAGThis branch measured about 32% slower than master, on the one test this measures. That is larger than this test's own run-to-run spread of about 17%, so it is worth looking at.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: fb5ea93, 6573781, c30aaee, ca9c21b, 77fbba8, 7a8a7f0, 2e2705a, 28c6be6, c813942, 558fcfb Since the previous push: status OK -> FLAG, ratio 0.919 -> 0.679, share 1.628 -> 2.202, rate 71694 -> 32394 (-54.8%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for Superseded by a newer throughput report. |
…a-varying-machine note MultiInstanceHighVolumeTest struck its 60-second GATING_CEILING at 1,948,661 of 3,000,000 records on the Performance Tests lane at b23c13d, and the throughput report read it as 32% slower than master. The code is identical to four earlier heads of the same PR on which this lane passed; the head differs from the previous one by a single markdown line. Recorded in the note that already argues this shape is the instrument, not the code: a 1.54x spread on identical code against a fixed ceiling, and a failing run reporting records-reached-over-sixty-seconds as if it were a rate. The push that carries this is the re-run: `gh run rerun --failed` is refused while the rest of the run is still in flight, and a fresh run on a new head is the same thing with the sighting recorded before the lane goes green again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
🟢 Throughput — OKThis branch measured about 5% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: fb5ea93, 6573781, c30aaee, ca9c21b, 77fbba8, 7a8a7f0, 2e2705a, 28c6be6, c813942, 558fcfb Since the previous push: ratio 0.95 -> 1.048, share 1.574 -> 1.427, rate 86933 -> 80038 (-7.9%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
… is now four for four ReactorBatchTest.simpleBatchTest[3] on the Unit Tests lane at f1e87d8: same Awaitility alias, same 30 seconds, expected 3 batches and got 4. Identical code had passed that lane at the three previous heads of the same PR, and no commit on the branch touches main Java. What makes this one worth more than a tally mark: the failing batch dump shows key 66 at offsets 0, 1 and 2 - three of five records on one key - which is the three-way collision the third, fourth and fifth sightings each carried. That lead in the ledger has now held on every sighting that recorded its batch contents. Still undiagnosed, still not classified, and still the shared BatchTestMethods lambda; recorded here so the next person starts from four-for-four rather than from zero. Re-run on the same head as the control. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…decided The learning behind the coverage gate's rewrite, captured while the evidence is fresh. The completeness guard began as a shell scan of .java files and accumulated six defects over three review rounds, every one a text-matching defect - a file-wide annotation grep, a class name taken from the filename, inherited test methods invisible in the subclass's file, an `extends` matched inside a javadoc sentence, one-hop inheritance, a same-named class picked by filesystem order. Each fix was another fragment of a Java parser written in grep, and a grep-parser's failure mode is silent in both directions, which is exactly what a guard must never be. The practice: when a guard reasons about compiled artefacts - class identity, modifiers, inheritance, annotation scope, which tests exist - read the compiler's output through javap rather than the source text, pin the contract with fixtures compiled by the real javac, and make a tool that did not run exit 2 rather than read as a suite with nothing to demand. It names the deliberate exception too: the drift checker still regexes source because its job has no build to read, and its consequence is advisory. Grounded against the tree by a validator: twenty-seven claims checked, one corrected (the quoted historical regex), the PR's open state confirmed live. Filed beside its sibling, the guard that lexes shell commands, under the corpus's own vocabulary. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
… setup-guard flake Same guard, same offset, same lane: `control thread must reach the mid-loop pause point (offset 25)` in the catch-all shard, on the head that added the learning doc and nothing else. That makes this the cleanest control arm the ledger has: 4fc15b8 differs from the green 69ecd0d immediately before it by one markdown file, so the Java under test was identical across a green run and a red one twenty minutes apart. Recorded before the branch merges, per the sighting discipline - the log expires, the entry does not. The rate claim is unchanged: three reds in the catch-all shard on this PR against roughly as many greens, which is the one-in-two the row already stated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
|
@codex review this Second Codex round. Since your review at 07dcddc the completeness guard was rewritten as a Node gate that reads bytecode through javap ( |
![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: 87fe5ba6e2
ℹ️ 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".
…ore the split lands docs/inflight/ci-shard-the-integration-gate.md described work this PR itself lands, which the directory's first rule forbids - track only what is OPEN. It held three different things, and each now lives where the next reader will look for it: - docs/ci.md gains "The Integration Tests lane runs as two shards": the shape and why it is not the chaos suite's four balanced bins, the two required checks and why the catch-all kept the old name, the four guards with what each one caught, how to read a red shard, and the forkCount ceiling. The workflow bullet points at it. - docs/solutions/performance-issues/shard-count-buys-nothing-while-one-class-sets-the-floor holds the measurement record: the cumulative table from 620s to 416s, the model that matched two shards to within 3s and then mispredicted the probe split, the two arithmetic traps (splitting a repeated class does not divide its time; shards manufacture work), the 1:3.6 conversion that put the serial-build cut first, and the two-not-four decision. - docs/inflight/ci-four-shard-integration-gate.md is the one item still open: the four-shard arrangement, built and measured and not taken, deferred with the reason and what would change it - re-derive rather than restore, and the four-way calculator it needs first. The 2026-09-03 plan cited the old note for the ordering argument; per docs/citations.md the address is repaired to the solutions doc that now contains that exact item, with a line saying where it was when the plan was written. The PR body's three references follow. bin/check-all.sh is clean, file refs and inflight tags included. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…- method, class, ancestor
The second Codex review found the case the fixtures never covered: a class whose EVERY test
method is tagged into an excluded group (`@Tag("performance")`, or `@Quarantined` on the method)
is filtered whole by failsafe, runs nothing, and writes no XML - and the gate, which checked
exclusions only against class-level tags, would have demanded a report from it. A permanent false
RED on the catch-all shard, in the safe direction but still wrong. No class in the tree has that
shape today (the method-level performance and quarantine tags in LoadTest and
MultiInstanceRebalanceTest sit beside untagged tests, which is why CI reported 45 of 45); the guard
is for the one that will.
The same defect class had a second instance the review did not name: `@Tag` is `@Inherited`
(read off the junit-jupiter-api 5.14.4 jar, not assumed), so a `@Tag("chaos")` on a BASE class
filters every subclass, and the gate read only the concrete class's own tags.
bin/lib/compiled-classes.mjs now reads a member's annotations from the block javap indents under
that member - the same column discipline that already separated a class-level tag from a method's -
so every record carries its test methods with their tags, meta-annotations included. Exclusion is
then decided where JUnit decides it: classTagsInScope() walks the ancestry for class-level tags,
hasRunnableTest() asks whether at least one test method in the class or its ancestors survives the
excluded groups, and a class fails either test is not demanded. The test-annotation match moved
from a substring search of the constant pool to the per-method annotation list, which is what made
the method-level reading possible at all.
Pinned by compiled fixtures: AllFilteredIT (both tags, one plain and one meta), FilteredBase with an
undecorated FilteredLeafIT (nothing runnable inherited) beside FilteredPlusIT (one plain test of its
own - demanded), and a @tag("chaos") ChaosBase with an untagged InheritsChaosIT. The stub Tag now
carries @inherited and the stub Quarantined targets METHOD, as the real ones do. Negative control:
the previous library, run over the AllFilteredIT fixture, demands it; this one does not.
If failsafe turned out to write an empty XML for a wholly-filtered class, this change would still be
correct - the gate would merely stop demanding a report that exists, for a class that by
construction runs no test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
|
@claude review this Third round. Since the second review at 5ebc4e8 the completeness guard was rewritten as a Node gate reading bytecode through javap ( |
|
Claude finished @astubbs's task in 5m 43s —— View job Review: third round — bytecode gate rewrite, tag-scope fix, doc extraction
Verified HEAD is 1. The tag-scope fix (
|
…-correct check-branch-self-reference flagged the one line in the extracted note that names this PR. The sentence already reads as it will after the merge - the two-shard lane it describes is what lands - so it only needed the marker saying someone judged it so. Reproduced locally under the PR's head branch name, since the worktree branch is spelled differently and the gate passes there by construction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
The third Claude review read the new "guards, and what each one caught" list against bin/ci-integration-test.sh and found it covered completeness, package provenance, the no-duplicates rule and drift, but not the check the heavy shard runs on itself: every class in HEAVY_CLASSES must have produced a report, so a rename or deletion fails the LIST loudly while the catch-all keeps running the class. A reader relying on the doc alone would not have learned that guard exists; it is the one that makes the subtraction shape safe in the other direction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
…tIT setup-guard flake Same guard, same offset, catch-all shard, on eb1e88a - a head whose only change from its parent is one documentation bullet. The last catch-all run that completed before it was 87fe5ba, green; the three heads between were cancelled by their successors' pushes. Nothing under the Java test roots changed between 87fe5ba and eb1e88a - the diff is docs, the Node gate library and its self-test - so the green run stands as the same-code control arm for this red one. Recorded before the branch merges. The rate claim on this lane is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh
Inherited #439, #445 and #442. The first and third matter to this branch: they fix the `dups: clones` lane that was blocking it, and they do so with a cause this branch got wrong. THE DUPS NOTE THIS BRANCH ADDED IS DELETED, NOT UPDATED `ci-dups-clones-lane-times-out-and-blocks-every-merge.md` described a problem #442 has fixed, so keeping it - even rewritten as a was-fixed narrative - is what `docs/inflight/AGENTS.md` forbids. Nothing in it survives as both true and unowned: master's workflow now carries the explanation at the point of use, and states it better. **Its causal claim was also wrong, which is the part worth recording.** The note attributed the timeout to corpus growth from #419 and #116, reasoning from a clean date boundary - every attempt on 2026-09-03 passed, every attempt on 2026-09-04 failed, corroborated on an unrelated PR. The mechanism was right (the job exceeding its five-minute cap with jscpd still running) and the attribution was not. The real cause, per #442's workflow comment: the action installed jscpd with `npx jscpd@4` inside the step it measures, so the base-branch engine paid a cold install the PR-branch step did not - 288s against 15s on the same tree - and npm's 300s default fetch-timeout let one stalled registry request consume the whole budget. The date boundary tracked registry conditions, not master's content. That is a reasoning failure worth naming rather than quietly dropping: a correlation across two branches felt like a controlled comparison, and it was not - both branches shared the runner and the registry, which is exactly the variable that moved. The control that would have settled it was re-running an OLD head, which this branch never did. THE FLAKE LEDGER'S REGISTRATION-RACE ROW, CONFLICTED AND UNIONED Both sides had grown it independently and neither is a superset. Master's version carries four #442 sightings - two of them on documentation-only heads - and counts a `ce-optimize` sighting where this branch counts #433; those are different runs, so the count is their union rather than either side's. This branch's WITHIN-branch control is restored, because master's row never had it and it is the strongest evidence in the row: the same tree produced both outcomes. Master's documentation-only heads are that same argument from the other direction, so they are kept beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
…either goes #442's bin/lib/compiled-classes.mjs and this branch's TransactionalClaimCoverageTest.effectiveTagsOf both answer "which tags would JUnit apply to this test?", and both had to get the same three cases right - method, class, and a tag reached only through a meta-annotation such as @Quarantined. They arrived independently, days apart, and agree. Recorded rather than queued, because consolidating them is the wrong move: one reads javap output from Node before any JVM starts, the other resolves annotations inside a running test JVM through JUnit's own AnnotationSupport. Neither can call the other, and a shared derivation would be a third implementation rather than one fewer. What the entry asks for instead is that a change to either is applied to both or explained - the rule is JUnit's, so it moves only when JUnit's does, and a divergence is silent in both directions. Also repairs a citation this branch broke itself: the inflight note named the new check by the name it carried before review renamed it. Caught by the merge checklist's rename sweep rather than by anything that goes red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o
…ion (#447) RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed failed on #438's CI with the same mid-loop pause-point signature every other sighting in that row carries - its saturation setup guard, not the confluentinc#909 assertion the test exists to make. The row already argued master-state and test-side by comparison: the same test passing on the same branch at a neighbouring commit, and two documentation-only heads on #442. This sighting reaches the same conclusion by construction instead. Those documentation-only heads still sit on branches whose earlier commits changed Java, so the tree that was built carries those changes; #438's whole branch diff is bin/ and docs/, compiling no Java and changing no test, so the tree built there is master's exactly. A branch that could not have caused an integration failure produced one, and no neighbouring commit is needed to see it. The reproduce line is asked of the pull request rather than of the branch, because the branch is deleted on merge and the check has to outlive it - the answer is identical either way. Recorded before the observing PR merges, per AGENTS.md: seeds and job logs expire, and while `bin/inflight.mjs codecov test` holds the per-commit outcome independently, no command produces the interpretation. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… new IT lands in the catch-all shard by design Two conflicts, both resolved by keeping both sides rather than picking one. docs/quarantined-tests.md: #440 quarantined RegistrationRaceStaleResidentIT on the recorded ledger (rule 1), which this branch had cited as master-state twice but not quarantined. That entry stays verbatim. Beside it, this branch's rewrite of the largeNumberOfInstances entry replaces master's "measured but not explained" text - the mechanism is measured now, and the registry is one of three places that said otherwise. docs/inflight/test-untracked-ci-flakes.md: master's row had reached 12 sightings, including #447's by-construction one on a docs-only branch; this branch's 2026-09-04 sighting on #444 and its same-head re-run control were not among them. Merged to 13 with both. Either side taken whole would have dropped a sighting under an authoritative-looking count - the same trap the previous merge of this file recorded. #442 sharded the integration lane; ClosingMemberRebalanceIT is not in HEAVY_CLASSES and lands in the catch-all by subtraction, which its design calls the safe direction: a new class runs by default. At ~52s for five cases it does not belong in the heavy set. ChaosScenarioBase auto-merged (#435's classification beside this branch's getMessage type fix); test-compile confirms it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WErnxQd9Ew57F9SsqzdPU5
…this branch's sighting #433 and #443. One conflict, the RegistrationRaceStaleResidentIT ledger row, and both sides had reached "13 seen" while counting different sets - so either taken whole would have dropped a sighting under an authoritative-looking number, for the third time on this file. Master's side is the base and deserves to be: it carries the #442 and #438 sightings and, more importantly, a WITHIN-branch control this branch had no way to know about - #433 ran the same content twice, green then red after a re-cut that changed only the commit split, so one tree produced both outcomes and the tree itself is ruled out, leaving the runner. That is a stronger claim than the cross-branch controls it joins. Added to it: this branch's 2026-09-04 #444 sighting with its same-head re-run, and one clause noting that sighting is another of the same-day cross-branch kind. 14 seen. Recorded because the first attempt at this resolution asserted on an anchor that did not exist, wrote nothing, and the file was then staged WITH its conflict markers - caught by grepping the markers rather than trusting the exit code. The second attempt carries an anchor-free fallback that announces itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WErnxQd9Ew57F9SsqzdPU5
56 commits of master, none of which touched this PR's mechanism: master changed neither `.github/workflows/release.yml`, `bin/release-notes.py`, `bin/test-release-notes.sh` nor `.github/workflows/mutation-full-sweep.yml` since the branch's last master merge, so the renderer and the workflow step that calls it merge unchanged. Master's `release.yml` still carries the inline `awk`/`sed` converter this PR replaces. Master also changed six of the files this PR touches. Four auto-merged: `docs/releasing.md` and `bin/AGENTS.md` (#457's job batching, #442's integration sharding), `.gitignore` (#378, #440), and `docs/inflight/release-0.6.0.0.md` (#448, #476). Two conflicted. `docs/inflight/release-0600-blockers.md` - the #197 triage bullet, inside the `post-merge: checked` markers. This branch (2026-09-01, 2026-09-03) rewrote it in post-merge terms as "all four landed"; master's grooming sweep (#476, 2026-09-08, correcting on 2026-09-07) rewrote the same bullet to say the empty-release-body item **was already false when written** - #72 gave the workflow a `--notes-file` built from the `CHANGELOG.adoc` section on 2026-07-29, with `--generate-notes` only as a fallback. Master's correction is right and is the later decision, so it is kept; master's claim that MDC is still open is not - #205 merged 2026-08-27, which is why this branch corrected it, and that correction is kept. Resolved as one bullet carrying both later facts, and it now states what #199 actually does (replace the inline converter, and fail the release on a missing or unrenderable section) rather than the retired claim that it restores a body that was never absent. `docs/refactoring.md` - two unrelated new sections appended at the same point. Both kept: this branch's "`bin/release-notes.py` is Python in a Node-default `bin/`" and master's "JUnit tag resolution is implemented twice". No content from either side dropped. Neither `--ours` nor `--theirs` was used. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XS64Xttx4vF5datYh7fmk
Built on #439, which has merged - master is now the base and has been
merged in. The
dups: clonescheck that #439 unblocked is what letsthis PR's own CI complete at all.
Description
Splits the
Integration Testslane into one named heavy set and a catch-all defined bysubtraction, and splits
Rebalance857CommitSyncDeadlockProbeITso the split is worth having.Measured, every row
620s → 416s, −33% critical path for +28% runner-minutes.
Why the probe split is the load-bearing part
Rebalance857CommitSyncDeadlockProbeITwas@RepeatedTest(20)in one ~356s class, and forks cannotsplit a class — so whichever shard held it was the critical path. Two shards or four made no
difference while it was intact (both modelled at 516s). The twenty repetitions, every assertion and
the 4s revoke dwell are unchanged; the logic moved to an abstract base with four
@RepeatedTest(5)subclasses.
The split costs more than arithmetic says, and that correction is worth carrying: 356s over four
classes was modelled as four 89s classes; measured they are 138-166s, because the repetitions
carry per-class fixed cost that used to be paid once. Splitting further would pay less again.
Why a catch-all rather than N balanced bins
A balanced split must be re-sized as the suite changes and fails silently when it is not — a new
class belongs to no bin, stops running, nothing goes red. Defining the catch-all by subtraction
inverts that: a new test runs there by default, and the only way to lose one is to name it in
HEAVY_CLASSESand delete it, which fails the heavy shard loudly while the catch-all keeps runningthe test. Failing in the safe direction is deliberate.
bin/ci-integration-test.shcarries the guide for when to add a class, andbin/check-integration-shard-balance.mjsrecomputes the optimal partition from recorded per-classtimes so drift is a number rather than quiet decay. That checker reads Codecov, so its network call
is opt-in behind
SHARD_BALANCE_NETWORK=1and onlyRepo Hygienesets it —check-*isblanket-granted to the PR reviewer, and
bin/AGENTS.mdforbids that prefix a network read. Thevariable is deliberately not
CI: the reviewer holding the grant runs inside Actions, soCIwould disable the read where it is harmless and leave it enabled where the agent is.
Five guards, and what each one alone would miss
Four are name matching in
bin/ci-integration-test.sh. The fifth — every class produced a reportsomewhere — is
bin/check-integration-shard-coverage.mjs, and reads bytecode throughjavaprather than Java source:
abstractis an access flag, the supertype is a field,@Nestedclasses areOuter$Inner,@Tagvalues are annotation attributes. The shell version it replaces had fivedefects, every one a text-matching defect (below), so the guard is now something that can be
unit-tested: its self-test compiles fixtures with
javacand pins each of those shapes.integrationTestpackageit.testbug belowBugs this found, all silent, none visible in timings
-Dit.test=!Classran the entire unit suite under failsafe. Settingit.testreplacesfailsafe's
<includes>, and both test source roots compile into the sametarget/test-classes:168 classes and 981 tests instead of 42 and 204, critical path 782s — and it passed, because
running more tests than you meant to fails nothing. Fixed with a pom-level
<excludes>.VertxConcurrencyITandCoreAppMetricsIntegrationTestlive elsewhere in the reactor and were reported missing while passing. Two shards went red on it.
report_existsinverted its answer on a match.find | grep -qunderset -o pipefailreports failure when it matches. Caught by
bin/check-source-patterns.mjs, whose rule existsbecause
check-review-posted.shshipped the same bug.What the review rounds changed
The completeness guard — the one that fails a shard when a class ran nowhere — was itself letting
classes through, verified against the classes a green run actually executed rather than by
inspection. Three separate filters, one of them opened by this PR's own de-duplication refactor:
@Quarantined/@Taggrep exempted a whole class when one method carried theannotation, which un-guarded
LoadTest;@Testin the file skipped classes that inherit their tests — exactlywhat moving the probe's
@RepeatedTestonto a shared base created;saw only the first.
The last two compounded on
Rebalance857CommitSyncDeadlockProbe4IT, which is in the catch-all: itcould have stopped running with nothing going red.
The second review round found a sixth text-matching defect in the same guard — an unanchored
extendsgrep that a javadoc sentence would satisfy, demanding a report for a helper that can neverrun — and that is what turned it into the bytecode gate above rather than a sixth patch. The
self-test caught two more before CI could:
@ParameterizedTestlives inorg.junit.jupiter.params,not
.api(the source-based check shared that blind spot), and a class whose only tests are@Nestedcarries no test descriptor of its own.
A third round - eight review lenses plus an independent validation pass - found the gate's own
green-while-red: a
javapthat could not run left an empty index, zero required classes, and apassing verdict over nothing. It exits 2 now, with a fixture pinning it. The same round caught the
Repo Hygiene step making a required check depend on Codecov's uptime (now advisory as its comment
claimed), the gate's error text promising to catch a class moved out of its package while the
candidate filter excluded it first (it now demands any test class whose ancestry reaches an
integration package), and two regressions from the simplify pass an hour earlier.
The drift checker also had two silent-pass bugs: a malformed
--fail-overproducedNaN, so a jobasking to block exited 0 having gated on nothing, and its could-not-run diagnostic read a field
lib/codecov.mjsnever sets, printing(unknown)every time. Branch-scoping its timings wasraised and refuted by measurement — Codecov reports no rows at all for
masterormainhere,so scoping would make the checker exit 3 forever; the timings are intersected against the checked-in
suite instead.
A second Codex round on the bytecode gate found that tag exclusions were read at class level
only, so a class whose every test method is tagged into an excluded group - which failsafe filters
whole, running nothing and writing no XML - would have been demanded: a permanent false red on the
catch-all. The library now reads each method's own annotation block, and
@Tagbeing@Inherited(checked on the JUnit jar) a base class's tag is read for its subclasses too. Three compiled fixtures
pin it, and a control arm shows the previous library demanding the all-filtered fixture. No class in
the tree has the shape today. A third Claude round then traced that fix by hand through all three
levels and found nothing blocking; its one observation, that
docs/ci.md's guard list omitted theheavy shard's own per-class report check, is applied.
Required check
Integration Tests (heavy)produces its own status context, and adding a job does not add arequirement — so the heavy shard's classes would have been non-gating. It has been added to the
masterruleset's required checks, alongsideIntegration Tests.Deferred, not rejected
Four shards is built and measured green at 355s on
ci/shard-integration-four, and kept as adeferred note,
docs/inflight/ci-four-shard-integration-gate.md, with its numbers; the measurementrecord and the ordering argument are in
docs/solutions/performance-issues/shard-count-buys-nothing-while-one-class-sets-the-floor-2026-09-07.md,and
docs/ci.mdnow owns the lane as it runs. It buys 61s over this PR for +526runner-seconds — a 15% gain for 66% more machine time, plus four lists instead of one. Revisit if
runner-minutes stop mattering or the suite grows enough that the catch-all dominates again.
Checklist
docs/ci.md(the lane as it runs), adocs/solutions/write-up of the measurements, the deferred four-shard note, plus the guide and the trap notes inbin/ci-integration-test.shdocs/features/-N/A - a CI lane split; no user-facing featureN/A - changes which job runs which existing tests. It adds five guards asserting the selection did what it claims, which is the testing this change needsdocs/inflight/working note (pr-/branch-) started at the PR's first commit -docs/inflight/ci-shard-the-integration-gate.md, extracted before merge intodocs/ci.md,docs/solutions/and the deferreddocs/inflight/ci-four-shard-integration-gate.mdce-simplifyandce-code-reviewlocally -N/A - asked for @claude review on the PR instead, the route that can open inline review threads🤖 Generated with Claude Code
https://claude.ai/code/session_01WUpkpGoTw9GGpC8J9CpUoh