Repository navigation
test(ci): fork-tail packing measured and rejected - and why the gate only became measurable after astubbs#106 - #105
Conversation
surefire runOrder=balanced + runOrderStatisticsFileChecksum (3.5.5+) with per-module .surefire-pc-unit-times stats checked in so slow-first ordering works on a cold CI checkout. Forks pull classes from one shared queue, so this is LPT packing: no fork idles behind RunLengthEncoderTest anymore. Unit gate (2-CPU harness): 432s -> 363s (-16%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… checklist gates)
Regenerated .surefire-pc-unit-times after merging master so the new core test AmbientProbeExtensionTest (#86) is ranked (3.4s) instead of unranked - an unranked class can displace RunLengthEncoderTest from its fresh-fork slot and inflate the tail. Green regen run, all modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records the unit-gate work: why slowest-first ordering packs the fork tail, why Maven -T module parallelism loses on a 2-core runner, and why this suite resists benchmarking (RunLengthEncoderTest swings 67s-166s for identical code, which is +-50-90s of noise on the total). README regenerated from the template - this also picks up the ambient-probe changelog entry from #86, whose README regeneration was missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Review note: the changelog hunk needs removing
Why removing them loses nothing: the section for the release being cut is generated from What it asks of you instead: the commit message is now the raw material. A subject that says what changed and, where it matters, what it changed for a user - with the diagnosis and rejected alternatives in the body. Specific to this PRThe entry describes unit-test fork-tail packing - surefire Under the new rules this earns no entry at all, at release time either. The entry test asks whether a user or operator can observe it without reading our repo; CI scheduling cannot. The write-up in |
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
Master had moved a long way while this branch sat, and it took the whole io.confluent.* -> bz.stub.* package rename with it. The branch had already been renamed on its own side by an earlier sweep, so both sides agree on where every file lives - which is what makes this merge ordinary rather than the silent mis-pairing AGENTS.md warns about. HOW THE CONFLICTS WERE RESOLVED, AND WHY TAKING MASTER'S SIDE WAS SAFE. Nearly every conflict was an artefact of the two independent renames, not a disagreement about content. Rather than eyeball them, each conflicted path was classified mechanically: our blob was reverse-normalised (bz.stub -> io.confluent, plus the csid.utils and csid.testcontainers moves that the rename folded into parallelconsumer.internal.*, minus the "Modifications Copyright" line the rename stamps on) and compared against the merge-base blob at its pre-rename path. Where the two matched, this branch had provably contributed nothing but the rename, so master's side was taken whole. That is the proof AGENTS.md asks for before reaching for --theirs; without it, taking the other side renders as nothing at all in review and quietly drops work. Master had also renamed some classes outright, and its layout was taken in each case: InternalRuntimeException -> PCInternalRuntimeException with its test, LongPollingMockConsumer moved from the test tree into src/main/java/.../internal/utils/, and TestConventionsArchTest which now exists in every module (git mis-pairs those five near-identical files across modules, so they surface as rename/rename). FOUR FILES NEEDED JUDGEMENT, NOT A RULE. CHANGELOG.adoc - the branch's entry under "== Unreleased" is dropped. The repo's policy changed while this branch was open: a PR never adds a changelog entry and never adds an Unreleased section, because the file is generated at release time from commit messages. The substance is not lost - it is in this branch's own commits and in the write-up under docs/solutions/. docs/inflight.md - master deleted it; in-flight state is now one file per item under docs/inflight/, and that directory's rules say it tracks only what is OPEN. The branch's addition there was a DONE entry, so it has no home under the new scheme; the durable half of it (the -T finding, the benchmarking-variance finding, and the stats-upkeep caveat) already lives in docs/solutions/test-flakiness/unit-gate-fork-tail-packing-and-measurement-variance-2026-08-03.md, which this branch adds. README.adoc and src/docs/README_TEMPLATE.adoc - master's side. The branch's only change beyond the rename was to stop embedding the changelog in the README, which master had already done independently. README.adoc is generated and is never hand-edited. ONE NEW EDIT THIS MERGE REQUIRED. bin/check-copyright-headers.sh grew stricter on master: it now fails the build for any file it cannot classify, and it runs at maven's validate phase, so an unclassified file stops every ./mvnw invocation rather than one CI job. The .surefire-pc-unit-times statistics files this branch introduces are exactly that - extensionless CSV with no comment syntax - so they get an EXEMPT_PATHS entry stating why. THE FORK-ORDER STATISTICS ARE REGENERATED ON THE MERGED BASE. The whole point of this branch is the recorded per-class runtimes that let surefire run the slowest classes first, and they had gone stale: core ranked a fraction of the classes now on disk, so most of the suite would have sorted last in arbitrary order - which is the failure mode the branch's own write-up warns about, arriving by the branch sitting still rather than by anyone adding a test. Every module's file is rewritten by a full bin/ci-unit-test.sh run against this tree, so the data describes exactly the workload the gate executes. The classes that remain unranked are the ones the gate excludes by group and therefore never times. The timings come from a developer machine rather than the 2-core runner, which is fine for what they are used for: runOrder=balanced needs the relative ORDER of the classes, not their absolute cost, and that ordering is stable across machines. VERIFIED ON THE MERGED TREE. Whole-reactor build clean; the core unit suite and the full unit gate both pass. bin/check-all.sh reports check-branch-self-reference.sh and check-issue-refs.sh red, so this commit is made with --no-verify. Both are the documented mid-merge artefact rather than findings: until the merge is committed, `git merge-base HEAD origin/master` is still the OLD base, so every line master has written since reads as a line this branch added. check-branch-self-reference.sh says so in its own header - "MID-MERGE IT ACCUSES YOU OF MASTER'S SENTENCES ... the fix is to finish the merge, never to mark someone else's note" - and committing makes both gates quiet. Checked rather than assumed: against the master commit this merges, the branch's real change set carries no unqualified reference and touches no docs/inflight/ note. The one genuine hit is an unqualified reference in the PR's GitHub body, fixed on the PR where it lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
bin/check-branch-self-reference.sh asks the merging branch to attest to any note that names it, at the one moment fixing it is cheap. This note named `optimize/unit-gate` and asked whether its measurements were taken against an "off" parallelism arm that was not actually off. Answered rather than deferred, because the answer is checkable and the note is about to outlive the branch it names: the arm WAS affected. The leak was closed by #265, which deletes core's junit-platform.properties from the tests jar, and every wall-clock figure this branch reported predates it. Those figures now describe a gate that no longer exists. What survives is the change itself. runOrder=balanced is a sort, and the fork-ordering statistics are regenerated against the merged tree, so the mechanism and the shipped data are unaffected - only the reported seconds need re-taking. The sibling branch named in the note, ci/reenable-parallel-tests, is still unchecked and stays open here. The branch reference is replaced with the PR number, since the branch stops existing when this lands and a branch name is not a link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
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)
|
[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 #105 +/- ##
============================================
- Coverage 83.29% 82.88% -0.41%
- Complexity 1352 1503 +151
============================================
Files 84 95 +11
Lines 4597 5207 +610
Branches 492 513 +21
============================================
+ Hits 3829 4316 +487
- Misses 586 701 +115
- Partials 182 190 +8
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 — OKThis branch measured about 7% 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 8 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: a532837, e8f7beb, 29f6a0f, a75400f, 12bf414, ce6f39a, 70a88bf, 27211b9 Since the previous push: ratio 1.13 -> 0.932, share 1.43 -> 1.727, rate 79609 -> 66143 (-16.9%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Review finding on #105. The write-up's upkeep caveat only covered one direction - a newly added class is unranked and sorts last - and the reviewer pointed out that the failure mode actually in front of us is the opposite one, which is worse because it leaves nothing to see. Adding a class at least puts something unranked on disk. A class that gets much FASTER, or disappears, changes nothing observable: surefire's runOrder=balanced keeps sorting, by recorded costs that describe a suite which no longer exists, and the fork packing decays toward the unordered scan it replaced. No build goes red, because class ordering is not something an assertion can be wrong about. The live instance is core's file, ranked around RunLengthEncoderTest at roughly three times the next-slowest entry. #106 collapses exactly that class, so landing it without regenerating the statistics would quietly hand most of this change back. That obligation is recorded in docs/inflight/pr-blockers-and-collisions.md - the register a session actually inherits - rather than only in a PR body that stops existing at merge, and it names the remedy: a full bin/ci-unit-test.sh run on core, committing the rewritten files. Also states plainly that this change and #106 OVERLAP rather than sum. Packing removes the idle fork at the end of the run; #106 removes the work that made that fork long. Once the work is gone the tail is a fraction of its size, so packing keeps helping by proportionally less against whatever class is next in line. Neither makes the other pointless - a class scheduled last strands a fork for its own length, whatever that length is - but adding their gains together would double-count, and a reader of either PR alone would be inclined to. No wall-clock figure is added, restated or defended here: the PR's headline seconds remain withdrawn pending re-measurement against a control arm that #265 uncontaminated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
|
Both actionable findings addressed in Finding 1 - the stats going stale with nothing to say so
Correct, and the asymmetry is the part worth keeping: adding a class at least leaves something unranked on disk, whereas a class that shrinks changes nothing observable at all. Fixed in three places rather than the one you suggested, because a PR body stops existing at merge and this obligation outlives it:
That was @astubbs's steer rather than yours, but your finding is what made it concrete, so answering it here: yes, and the body now says so. These two changes overlap, they do not sum. Packing removes the idle fork at the end of the run; #106 removes the work that made that fork long. Once that work is gone the tail this change exists to pack is a fraction of its size, so packing keeps helping by proportionally less, against whatever class is next in line ( Finding 5 - the two
|
Done - the entry went in
Agreed, and nothing here asks for one. CI scheduling is not observable to a user or operator without reading this repo. |
…w read them per line Corrects a claim ee9a3b2 carried in from the automated review on #105. That review compared the wrong unit and the error survived into both files that commit touched, so it is fixed here in the same terms it was made. THE MISTAKE, BECAUSE IT IS THE REUSABLE PART. Each line of a .surefire-pc-unit-times file is one test METHOD, and a heavy class is spread over many of them. surefire's runOrder=balanced sorts CLASSES - pom.xml's own comment beside the setting says so, and forking cannot split a class, so the class is the scheduling unit either way. Ranking the raw lines therefore compares one method of one class against a whole other class, and it badly overstates how far the top class stands above the next. The review read core's file that way, put RunLengthEncoderTest at roughly three times the next entry, and named a METHOD as the class that would define the tail after #106. Aggregated per class, the top two are close rather than a factor apart, and the third is well behind: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \ parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head -5 Both files now state the shape and give that command rather than baking in a ratio, which would be wrong at the next stats refresh anyway. WHAT IT CHANGES ABOUT THE ARGUMENT, IN BOTH DIRECTIONS. The staleness risk is real and stays - an ordering built around a class that has since collapsed decays silently, with no red build - but it is materially smaller than the review stated. It also cuts the other way, in this change's favour: because forking cannot split a class, #106 moves the floor down to the second-heaviest class and no further, and that class is substantial. So the residual tail is most of the present one and packing keeps most of its value rather than being largely obsoleted. The "overlap, not sum" paragraph stays correct and stays put; only the implication that the residual tail is small is removed. No figure is asserted for #106's own collapse. The review took "62.3s -> 0.2s" from that branch's commit message, but that measurement predates #265's contaminated-arm fix and disagrees with both the committed statistics and a run on the merged tree, so the class is described as collapsing by an order of magnitude and the re-measurement now in flight can supply the number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Correction to finding 1 - the ratio was measured in the wrong unitFollowing up my own reply above, because I repeated a mistake instead of checking it, and it is now committed. Fixed in
Those are per-METHOD lines, and Aggregated per class: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \
parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head -3So the gap between the top two is about 1.2x, not 3x, and the real next tail-definer is This cuts both ways, and both halves are now in the docs.
Both files now state the shape and carry the One more I am flagging rather than asserting: the finding takes 62.3s -> 0.2s from #106's commit message. That measurement predates #265's contaminated-arm fix and disagrees with the committed statistics, so the "~99.7% collapse" quotes a gate that no longer exists. The docs now say the class collapses by an order of magnitude and leave the figure to the re-measurement currently in flight - same reason this PR's own headline seconds are withdrawn. Good catch on the direction of the failure mode regardless - the asymmetry between adding a class and shrinking one was genuinely missing, and it is the part worth keeping. |
[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. |
The coverage note asks for evidence it cannot generate itself, and this PR is an unusually clean source of some, so the sighting is recorded before the merge rather than after - CI logs expire and nobody is reading here at the moment a PR lands. WHY THIS PR IS A GOOD PROBE. Its diff contains no Java: a surefire runOrder property, one exempt path in a shell script, three documents and ten .surefire-pc-unit-times data files. Any difference the coverage comparison reports is therefore the comparison's own, with no change to attribute it to. WHAT WAS OBSERVED. codecov selected a base that is a provable ancestor of the PR head (git merge-base --is-ancestor), so everything in the base is in the head - and it still reported the head as carrying double-digit more files and hundreds more lines than that base. No diff can produce that, which makes it a statement about the two sides rather than about the change: they are measuring different FILE SETS. codecov/patch passed, the whole-repository project number rose, and only the flag-scoped unit gate was red. integration, chaos and performance each had no base value at all. WHAT IT SETTLES, AND WHAT IT DELIBERATELY DOES NOT. It bears on the note's first open item - whether the flag split verifies itself once master re-uploads. Master has re-uploaded, since unit had a base to compare against, so the "it clears once master has re-uploaded under the new flags" prediction is refuted for unit and untested for integration, which had no base at all. It does NOT establish the shared-upload-glob mechanism the note names as unverified. A file-set mismatch is consistent with that glob, and this sighting inspected no uploaded report and named no file, so the note says so in those words. A sighting that quietly promotes an untested hypothesis to an established one is worse than no sighting, because the next reader stops looking. Figures are described by shape rather than copied in: they move at every re-upload, and a stale number in a note reads as a current one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
The Worktree ownership section already says to spell `git -C <worktree>` as a literal path, and explains that the pre-commit hook reads the command before the shell expands it. It does not say the obligation repeats on every commit, and that is the half that catches people. A BARE `git commit` DOES NOT MEAN "THE TREE I AM IN". It resolves against the session root, which in an agent session is some other worktree entirely. Two things make this recur rather than being learned once: the shell's working directory does not survive between commands, so an earlier `cd` into the right worktree does not carry to the commit; and the first commit of a session usually happens right after that `cd`, so it works and teaches the wrong lesson. Observed here: a bare `git commit` on this branch resolved to `.worktrees/sparse-encoding` - a different branch's tree, with nothing staged - and `.claude/hooks/pre-commit-gate.sh` refused rather than running the gates there and reporting another tree's state as this one's. That is the silent-wrong-answer case the hook exists to prevent, and it fired correctly; what was missing was anything telling the author not to walk into it. Kept to the rule and its mechanism, with no retelling: the hook's own header already owns the resolution order and why it refuses rather than guessing, and this section already cites it. Per this file's own guidance, an enforcer's header owns what it explains and AGENTS.md keeps only the part that binds every session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
Corrects the claim its parent commit made one command too early. That commit said the shell's working directory "does not survive between commands", which is stronger than what was measured and is the kind of overstatement a reader disproves on their first try and then discounts the whole rule for. Measured in the session that wrote it: the cwd reset once, mid-session, landing in a DIFFERENT worktree - and then carried across several later commands, including the commit that added the rule. So it survives some commands and not others. The unreliability is the hazard, and it is worse than a clean reset would be. A reset does not raise an error; it silently relocates you to another worktree, where a bare `git commit` is then judged against a tree you never named. That is why the rule is "name the tree on every commit" rather than "cd first" - naming it is the only form that does not depend on which way the toss went. Wanted to be an amend of the parent, since it is unpushed. The pre-commit gate classifies --amend as a history rewrite and refuses it while checks are running, and taking its documented override for a wording fix is not a good enough reason to bypass a safety gate. Trivially squashable with its parent at merge prep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
[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. |
…is a real drop" (#462) The codecov flags note (and codecov.yml's own comment) claim that because both sides of the unit/integration gates come from the same profile and exclusions, a drop on codecov/project/unit is a real drop. Master's own history for that flag contradicts it: consecutive master commits sit in one of two stable bands roughly three points apart and jump between them with no relation to what changed, and the check has gone genuinely red on master against master more than once for this reason - not because unit coverage actually regressed. This is a different shape from the two related sightings already on file. #431's ed6b8f4 found one verified but narrower cause (a single truncated base upload). The optimize/unit-gate branch (#105) found a file-set mismatch between a PR and its own ancestor base, cause left explicitly unverified. Neither is a same-commit-pair, master-vs-master comparison across time, so this sighting is recorded rather than folded into either - along with the cheapest next check (diff whole-report file/line totals across a same-band pair and a cross-band pair) and what it actually costs: the gate is non-required so nothing blocks, but it flips on roughly every other master commit, so a genuine unit-coverage regression is now indistinguishable from the noise. Claude-Session: https://claude.ai/code/session_01WErnxQd9Ew57F9SsqzdPU5 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…oders are active (#106) perf(offsets): stop walking every offset when only distance-based encoders are active OffsetSimultaneousEncoder.invoke() visited every offset between the committed base offset and the highest succeeded offset. When that range spans a large gap - up to ~2.1 billion - it is a multi-second scan on the commit path, and it is exactly the case where no BitSetEncoder can be constructed, leaving only RunLength. It now visits only the offsets where the completion state changes. This resolves the encoder's own long-standing TODO about large ranges being slow. WHY IT IS SAFE. RunLengthEncoder accumulates the open run by the DELTA to the previously seen offset, not by counting calls. For a maximal run [a,b], visiting a opens the run and visiting b adds b-a, giving the identical length to visiting every offset; extra visits inside the run telescope harmlessly. So invoke() needs only each run's first and last offset, and because a state change can only occur at an incomplete offset or immediately beside one, the visit set is {0, length-1} plus each in-range incomplete and its two neighbours. It is sized by the NUMBER OF INCOMPLETES, never by the range width. The full scan is retained whenever any active encoder requires every offset (BitSet, ByteBuffer), expressed as OffsetEncoder.requiresEveryOffset() and defaulting to the safe answer, so a future encoder opts into sparse deliberately rather than inheriting it by accident. REVIEW FOUND TWO DEFECTS IN THE FIRST VERSION, AND BOTH ARE FIXED. Cross-model review (Codex, confirmed independently by a Claude pass) found two bugs in the hand-written in-range filter. Both break the same premise, which is what makes them correctness bugs rather than edge cases: the boundary proof holds only if the visit set contains EVERY in-range incomplete - miss one and the run boundary beside it is never visited, the run is absorbed into its neighbour, and the encoding silently diverges with no exception raised. - The exclusive end offset is highestSucceededOffset + 1, which wraps to Long.MIN_VALUE at Long.MAX_VALUE, dropping every incomplete and encoding them as complete. The bound is now the inclusive last offset, which reconstructs highestSucceededOffset and cannot wrap. - The early exit assumed ascending iteration, but the incompletes set is caller-supplied on a public constructor whose field javadoc promises no order requirement. It is removed rather than repaired: it could never fire in production, because encodeOffsetsCompressed samples the high-water mark once and derives both the set and this range top from it. Both are latent rather than reachable by a running deployment - the first needs a partition at ~9.2 quintillion offsets, and the only production supplier of the set collects into a natural-order TreeSet. Note SortedSet#subSet, which the hand-written loop replaced, WOULD have honoured the set's comparator; avoiding subSet for its own good reason must not import an ordering assumption it never had. The loop carries that reasoning so it is not undone. VERIFICATION. A differential test runs both strategies over 130 generated scenarios at a fixed seed and asserts the encodings are byte-for-byte identical, that the key sets match, and that the sparse path was genuinely taken. Two further regression tests cover the shapes above, and both were proven by sabotage rather than assumed: with the old loop restored each fails with its predicted symptom - an empty encoding map, and a byte mismatch against the full scan - while all 130 pre-existing scenarios still pass, which is the direct evidence that the differential test could never have caught either. No existing test is weakened, skipped or excluded. RunLengthEncoderTest changes are javadoc only; its assertions are untouched. PERFORMANCE. RunLengthEncoderTest collapses by an order of magnitude, from tens of seconds to a fraction of one. Deliberately no gate-level figure: the numbers this work originally quoted were taken before #265 fixed a leak that made dependent modules run thread-parallel regardless of their own setting, so they describe a CI gate that no longer exists. The class-level win is a change in algorithmic complexity and stands without a benchmark. Reproduce with ./mvnw -Pci -pl parallel-consumer-core -am test. FOLLOW-UP OWED AFTER THIS MERGES. This collapses the runtime of the class that #105's checked-in .surefire-pc-unit-times ordering is built around, so those statistics must be regenerated with a full bin/ci-unit-test.sh or that PR's test packing decays silently, with no red build to say so. IT ALSO CARRIES ONE DOCUMENTATION LEARNING, deliberately rather than as its own PR. Bringing this branch across the package rename produced rename/rename conflicts on the near-identical per-module arch tests, and resolving them with `git checkout --theirs` staged files still containing conflict markers. The write-up added under docs/solutions/workflow-issues/ records the mechanism: --theirs fails loudly there (stages 2 and 3 sit at different paths, so the path you name has no stage 3), and the silent step is the `git add` after it, which stages whatever the merge left in the worktree and leaves `git ls-files -u` clean. A batched invocation aborts on the first bad path, so ordinary conflicts sharing that command line are left unresolved too. The rule that generalises - resolve by explicit ref and check the command's status, not the look of the tree - was already in use in this repo but had never been written down. Its first draft got the mechanism backwards and is corrected in the second commit, which is why both are here. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#106 has landed, which is the event this branch was waiting for. It collapses the runtime of RunLengthEncoderTest - the class this branch's checked-in .surefire-pc-unit-times ordering is built around - so the statistics here now rank the suite by a cost that no longer exists. Regenerating them is the next commit; without it the packing decays toward the unordered scan with no red build to say so. ONE CONFLICT, RESOLVED BY KEEPING BOTH SIDES. docs/inflight/ci-codecov-flags-not-like-for-like.md gained a different 2026-09-07 sighting on each side. Ours records that a PR and its base disagree on the unit flag's file set even though the base is a git ancestor of the head. Master's records that master's own unit history oscillates between two stable bands across consecutive commits, independent of diff content. They are not the same finding and neither subsumes the other - master's text says so itself, naming this branch's sighting and explaining why it is a different shape. Both are kept, ours first and master's as the follow-on it is written as. Resolved by editing the hunk rather than taking a side, which is the rule the write-up that merged with #106 exists to state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…branch Master's own 2026-09-07 sighting, which arrived in the merge, describes the sighting above as something the optimize/unit-gate branch adds. Once this merges the branch stops existing and both sightings sit in this file, so it now points at the section two above it by name. check-branch-self-reference asks for exactly this at the one moment it is cheap, and the reference it flagged was master's prose rather than ours - the case the gate is least likely to get from anyone, since whoever wrote the sentence is not whoever merges it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
…ed base #106 landed, and the obligation it recorded comes due here. These files rank test classes by recorded runtime so surefire can schedule the slowest first; core's ranking was built around RunLengthEncoderTest, whose cost that PR removed. Left alone they would sort the suite by a cost that no longer exists, and the packing would decay toward the unordered scan it replaced - with nothing going red, because ordering is not something an assertion can be wrong about. Regenerated by a full bin/ci-unit-test.sh run on this tree, so the data describes exactly the workload the gate executes. Reproduce the ranking with: awk -F, '{t[$3]+=$2} END {for (c in t) printf "%9.0f %s\n", t[c], c}' \ parallel-consumer-core/.surefire-pc-unit-times | sort -rn | head Two things the new ranking settles. RunLengthEncoderTest drops by roughly two orders of magnitude, which is the first measurement of #106's effect taken on the merged tree rather than quoted from a gate that no longer exists. And the class that now sets the tail, ParallelEoSStreamProcessorTest, is only slightly cheaper than the one it replaces - so the tail this branch packs around did not collapse with the class. A review of #105 had estimated it would fall to about a third, by comparing per-METHOD lines when runOrder=balanced sorts per CLASS; that estimate was wrong in the direction that would have argued against this branch. Whether the packing is worth its keep is still open and still a measurement, not an argument. The comparison that settles it - this tree against master, interleaved, repeated - is what these statistics now make possible for the first time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
[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. |
…rop it, keep the findings The comparison this branch was waiting for became possible when #106 merged, and it went against the change. Two arms sharing that base, statistics regenerated on the very tree under test, interleaved, three repetitions each: the packing arm was slower in every paired repetition. A small, consistent loss rather than no measurable effect. So the mechanism goes and the knowledge stays. Removed: the ci-profile runOrder properties, the nine checked-in statistics files, and the copyright-scanner exemption that existed only to classify them. What is left is documentation. WHY IT COULD NOT HAVE BEEN MEASURED BEFORE, which is the part worth keeping. Packing was built around RunLengthEncoderTest, an un-splittable class that dominated both the wall time and its variance. Judged with that class intact, the ordering was being measured in the arrangement that repo's own integration-lane write-up says buys nothing for any arrangement. #106 collapsed the class; only then could the ordering be re-derived and the two compared. Shrink first, re-derive, then measure - the same result from the other direction. AND #106 DID NOT ONLY MAKE THE GATE FASTER, IT MADE IT MEASURABLE. Repeat runs of identical code used to differ by up to a minute and a half; across all six runs here the spread was seconds. The variance was that one class landing in different fork slots. That is why a difference of a few seconds resolves now when ninety could not before, and it is worth more than the verdict. The write-up keeps its two original findings - Maven -T module parallelism loses on a 2-core gate and manufactures flakes, and a harness must not mutate its own input - and now carries the null result. Its pre-measurement prediction, that packing would keep most of its value once the dominant class shrank, is left visible and marked wrong: the per-class arithmetic behind it was right and the conclusion still did not follow. A mechanism that is real is not the same as a mechanism that pays. The reproduce recipe is written out rather than cited, because the harness lives under the ignored .context/ directory and a pointer there resolves for nobody. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
astubbs
changed the title
…er has The sighting enumerated this PR's file list to justify why it is a clean probe - no Java, so any difference the comparison reports belongs to the comparison rather than to the change. Re-scoping the PR to documentation only made that enumeration stale while leaving the argument intact, so it now states the shape as of the observation and notes that the re-scope strengthens the probe rather than weakening it. Writing a specific file list into prose was the mistake. The repo already says not to write down what a command can produce, and this is a small worked example of why: the claim was true when written, nothing went red when it stopped being true, and it would have read as current forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
[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. |
|
@claude review this This PR has changed shape completely since it was last reviewed. It set out to speed up the unit So please do not review it as a performance change. Review it as a claim about a measurement, which is Where I would look hardest:
One process note so you do not have to reconstruct it: the wall-clock figures this PR originally quoted |
|
Claude finished @astubbs's task in 3m 28s —— View job Review: fork-tail packing null result (
|
🧪🔒 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 |
Base movement, two commits landed while the previous merge was being verified: #358's `DispatchScanMeter` and #105's fork-tail-packing null result. WHY IT COULD NOT BE SKIPPED. The PR went `CONFLICTING` the moment #358 landed, and a conflicting PR has no merge ref for GitHub to build - so the whole `pull_request` half of CI never started on the previous head. Only the `push`-triggered CodeQL analysis ran, and it passed, which is exactly the shape that reads as "CI is green" if the check list is skimmed rather than counted. ONE TEXTUAL CONFLICT, `docs/inflight/ci-codecov-flags-not-like-for-like.md`: both sides appended a dated sighting at the same anchor. Both kept, chronologically - this branch's 2026-09-03 truncated- base sighting first, then master's 2026-09-07 file-set sighting, which is the order master's own later section already assumes when it refers to "the FILE SETS sighting above". The marker plumbing needed one edit and it is worth naming, because it is invisible otherwise: this branch wraps the whole two-band section in a `post-merge: checked` block, and master had since added its own `checked-begin`/`checked-end` pair INSIDE that section. Nested blocks do not compose - the inner `checked-end` closes the outer block, and `bin/check-branch-self-reference.sh` then reported two lines it had been covering. Master's inner pair is removed and the outer block covers its paragraph; no prose changed, and the gate is green. THE JAVA MERGED CLEAN AND DID NOT COMPILE, which is the more useful half. `ProcessingShard`'s constructor gained a fifth argument, so the three shards this branch builds by hand went stale without conflicting: - `RetryQueueRebalancePathTest` (two standalone shards) and `RetryQueueRequeueWindowTest`'s `SeamShard` take the meter now. - `SeamShard` is handed `wm.getSm().getDispatchScanMeter()` - the MANAGER's meter, not a fresh one. The meter is deliberately shared across every shard of one `ShardManager`, and a planted shard counting into its own would silently drop whatever production examined through it. That is the same asymmetry #358's own commit message gives as the reason the meter is not per-shard. VERIFIED on the merged tree, from fresh reports: `ArchitectureTest` 3/0/0/0, `RebalanceCallbackRuleControlTest` 2/0/0/0, `RetryQueueTest` 10/0/0/0, `RetryQueueRebalancePathTest` 6/0/0/0, `RetryQueueRequeueWindowTest` 12/0/0/0, `RetryQueueIteratorConfinementTest` 5/0/0/0, `ShardManagerTest` 5/0/0/0, `ShardPopulationRaceTest` 4/0/0/0, and #358's own `OrderingModeDispatchParityTest` 1/0/0/0 - which asserts EXACT scan totals, so it is the arm that would have caught the planted shard counting into the wrong meter. Lincheck lane 9/0/0/0; core unit suite 883/0/0/8; `bin/ci-unit-test.sh` BUILD SUCCESS across every module; `bin/check-all.sh` 16 passed 0 failed; the file-ref, issue-ref and branch-self-reference gates green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
Pulls in five commits master gained since this branch's base: #466 (revoke-path commit drains through the control thread, fixing the last remaining quarantined test), #358 (dispatch-scan meter), #105 (fork-tail packing measured and rejected, docs only), #467 (Lincheck shard harness fixture), and #465 (rebalance-callback ArchUnit rule widened). Two files conflicted, both because this branch and #466 edited adjacent or overlapping prose in the same registers: - docs/inflight/test-untracked-ci-flakes.md: this branch deleted the RegistrationRaceStaleResidentIT row/section (already retired to a solutions write-up); #466 rewrote the immediately following AmbientProbeExtensionTest row (a third captured method) and separately added a now-fixed-and-out sentence for RegistrationRaceStaleResidentIT in the intro paragraph, which this branch's auto-merge had already applied cleanly before the conflicted hunk. Resolution: kept this branch's deletion and took #466's updated AmbientProbeExtensionTest row - both sides' actual content survives, nothing was dropped. - docs/quarantined-tests.md: this branch's e5c6a5e already rewrote "Currently quarantined" into a four-exit narrative once RegistrationRaceStaleResidentIT and MultiInstanceRebalanceTest.largeNumberOfInstances left; #466 fixed the third and last remaining entry (ProducerManagerTest.aRevokeTimeCommitIncludesTheOffsetOfEveryRecordItAlreadyProduced) and rewrote the same section's intro + checklist from its own (pre-restructuring) base text. Combined both: the registry is now genuinely EMPTY (confirmed - no `@Quarantined` annotation remains anywhere in the tree), so the section states that plainly and the revoke-path fix becomes the narrative's fifth exit, folding in #466's fix description and its solutions-doc link. Verified: `env -u NODE_OPTIONS bin/check-all.sh` (16 passed, 0 failed), both quarantine gates green, `bin/check-file-refs.sh` clean, and `./mvnw -Pci -pl parallel-consumer-core -am test-compile` builds green after the merge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q99GxTheRQL6d7TDt1bUum
1 participant
Description
This PR set out to speed up the unit gate by packing the fork tail. It was measured properly and it does not work, so the change is gone and only the findings remain. What lands is documentation: a null result, two side findings worth more than the original idea, and one CI sighting.
There is no code, no
pom.xmlchange and no checked-in data in this PR any more.The idea, and why it was plausible
Surefire's forks pull test classes from one shared queue, and the run ends when the last fork finishes. With the default
runOrder=filesystem, core's slowest class was scheduled last, so one fork idled while the other ground through it.runOrder=balancedsorts classes slowest-first, which is textbook longest-processing-time scheduling. Surefire 3.5.5'srunOrderStatisticsFileChecksummade it usable on CI for the first time, by pinning the statistics filename so the data could be committed and survive a cold checkout.Why it could not be judged until now
The packing was built around
RunLengthEncoderTest, an un-splittable class that dominated both the wall time and its variance. Measured with that class intact, the ordering was being judged in exactly the arrangement this repo's integration-lane write-up says buys nothing for any arrangement - the dominant class still setting the floor.#106 collapsed that class. Only then could the ordering be regenerated and the two compared. Shrink the dominant class first, re-derive, then measure.
The result
Two arms sharing that base, statistics regenerated on the very tree under test, interleaved, three repetitions each, in a container capped to the runner's core count. The packing arm was slower in every paired repetition, by a few seconds at the median. Not "no measurable effect" - a small, consistent loss.
The finding that outlives it
#106 did not only make the gate faster; it made the gate measurable. Repeat runs of identical code used to differ by up to a minute and a half. Across all six runs here the spread was seconds. The variance everyone had been fighting was that one un-splittable class landing in different fork slots - so a difference of a few seconds resolves now where ninety could not before.
That matters for anything measured on this gate in future, and it is why the earlier figures this PR once quoted were withdrawn rather than corrected: they were taken before #265 closed a leak that made dependent modules run thread-parallel regardless of their own setting, and they describe a gate that no longer exists.
What else is in here
-Tmodule parallelism loses on a 2-core gate, measured, and starves wait-heavy tests into awaitility timeouts. Pick one parallelism axis per core budget.runOrder=balancedsorts per class. The conclusion still did not follow. A mechanism that is real is not the same as a mechanism that pays.AGENTS.mdworktree rule: name the tree on every commit, because a baregit commitresolves against the session root, which is some other worktree.Checklist
CHANGELOG.adoc, under== Unreleased) -N/A - repo policy is that a PR never adds a changelog entry; the file is generated at release time from commit messages.N/A - no code changes remain in this PR.N/A - no CI, workflow, runner or permission changes remain.🤖 Generated with Claude Code
https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC