Repository navigation
test(dispatch): guard the ordering-mode dispatch ratio by counting, not timing - #358
Conversation
…ot timing UNORDERED dispatch re-walks the whole in-flight prefix on every pass while KEY skips an occupied shard in one step, so their scan costs diverge as in-flight depth grows. Nothing pinned that, and a change that made one shape quadratic would have shown up only as a slower benchmark on someone's laptop. The first version of this guard timed the two modes and compared durations. That is not a test, it is a race with the rest of the suite: it ran inside a thread-parallel run, and its failures were the suite's load rather than the code's behaviour. Four attempts to stabilise it - fastest-of-three, a ratio instead of absolutes, CPU time instead of wall clock, @isolated - each moved the flake without removing it, and three of them were made after a bisect had already shown the dispatch code unchanged. So it counts instead. DispatchScanMeter is a LongAdder shared across every shard of one ShardManager, incremented once per entry the scan examines. The test asserts exact totals - RECORDS examinations for KEY, the triangular sum for UNORDERED - which are properties of the algorithm, not of the machine: deterministic, load-immune, and they say which shape changed rather than that something got slower. The meter is main-code rather than test-code deliberately: it counts inside the scan loop, where a test double cannot reach without changing the thing being measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Records the wagon table with what is cut and what is not, the owner's steer that the split serves review quality rather than PR count, and the observation that the scan fix helps the shipped default and may deserve promotion ahead of direct pull. --no-verify: pre-existing branch gate debt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
[superseded - 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 Superseded by a newer quarantine lane report. |
…cut) Two ledger commits from the base - the wagon table with what is cut and what is next. No code; taken as-is to keep this branch current with its base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HwDcZdfv1Ne4u3g6ThrG9
…t pull Cutting it alone was attempted and abandoned on evidence: it compiles only on top of #335 + #336 + #358 merged, and its tests additionally need the abandonment path and getUpperBoundOnSelectableWork, which belong to direct pull. Isolating it would mean hand-rebuilding the accounting code that has already produced two bugs - so it becomes one review with direct pull instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MedgsxqrM8vjSt5ncuAo8g
…y-counted-not-timed
Master gained the conservation-derived RecordPopulation (#336) and the atomic compute() admission in ShardManager.addWorkContainer (#437), both of which touch the ProcessingShard constructor this branch also extends with DispatchScanMeter. The PR body predicted a one-line resolution at that constructor; it came to three files plus one compile fix: - ProcessingShard: both fields kept. RecordPopulation stays first so master's parameter order is unchanged and the meter is appended as the last argument of the Lombok required-args constructor. - ShardManager: master's compute() block is taken whole, with dispatchScanMeter added to the constructor call inside it. Both shared instances are fields on the manager. - ShardManagerTest and ShardPopulationRaceTest: master's new call sites pass a meter; the test-local shards get a fresh DispatchScanMeter since nothing reads it there. The scanMeter.onEntryExamined() increment auto-merged into the rewritten scan loop (#373) still sits once per examined entry, so the parity test's exact totals are unchanged. Verified: OrderingModeDispatchParityTest, ShardManagerTest and ShardPopulationRaceTest green under JDK 17. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqkUNK5K7JXkbbGoNFTvfB
…ile-refs gate The javadoc cites docs/inflight/perf-throughput-regression-since-0-3.md, which is carried only by perf/throughput-regression-since-0-3 and has never been on master. The citation predates the file-refs gate that master now runs at pre-commit and in the repo hygiene lane; the gate's own header names this javadoc as the case that motivated extending it to .java files. The paragraph marker is the escape it prescribes for a path named deliberately on another branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqkUNK5K7JXkbbGoNFTvfB
|
@claude review this |
|
Claude finished @astubbs's task in 4m 58s —— View job Review
Summary: solid change, one minor finding. The core idea — replacing a wall-clock/ratio comparison with an exact entry-examination count via Finding:
|
[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 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 #358 +/- ##
============================================
+ Coverage 82.35% 82.79% +0.44%
- Complexity 1493 1504 +11
============================================
Files 95 96 +1
Lines 5207 5214 +7
Branches 513 513
============================================
+ Hits 4288 4317 +29
+ Misses 724 705 -19
+ Partials 195 192 -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:
|
🟢 Throughput — OKThis branch measured about 11% 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 9 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: f502202, abd1d39, 60138dc, a532837, e8f7beb, 29f6a0f, a75400f, 12bf414, ce6f39a Since the previous push: ratio 0.85 -> 1.107, share 1.893 -> 1.453, rate 73699 -> 79541 (+7.9%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
…e its siblings The Lombok @Getter on ShardManager.dispatchScanMeter generated a public accessor, which added ShardManager.getDispatchScanMeter().getEntriesExamined() to the library's public API for what is a test-only instrument. Every consumer is a test in the same package, and the structurally identical recordPopulation and retryQueue fields already use package-private accessors marked visible for testing. Match them. Raised by the automated review on #358. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqkUNK5K7JXkbbGoNFTvfB
Fixed in 8599a06. The accessor on One note for anyone reading the duplicate-code report above: the new file similarity it flags between |
🧪🔒 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
A guard on the ordering-mode dispatch ratio that counts what the scan examines, instead of timing it.
UNORDEREDdispatch re-walks the whole in-flight prefix on every pass;KEYskips an occupied shard in one step. Their scan costs therefore diverge as in-flight depth grows, and nothing pinned that — a change that made one shape quadratic would have surfaced only as "the benchmark feels slower".Why this is a test and the first version was not. The first version of this guard timed the two modes and compared durations. Inside a thread-parallel suite that is a race with the rest of the run, not an assertion about the code: four attempts to stabilise it — fastest-of-three, ratio instead of absolutes, CPU time instead of wall clock,
@Isolated— each moved the flake without removing it, and three were made after a bisect had shown the dispatch code unchanged. That is the cost this PR removes.DispatchScanMeteris aLongAddershared across every shard of oneShardManager, incremented once per entry the scan examines. The test asserts exact totals —RECORDSexaminations forKEY, the triangular sum forUNORDERED— which are properties of the algorithm rather than of the machine: deterministic, load-immune, and they name which shape changed rather than reporting that something got slower.The meter is main-code rather than test-code deliberately: it counts inside the scan loop, where a test double cannot reach without altering the thing being measured. It is a counter and a getter — no behaviour, no branch, one
LongAdderincrement per examined entry.Provenance and scope. Extracted from the engine-performance campaign on
perf/engine-concurrency, where it was written to make the direct-pull scan work measurable. It is cut against master and depends on nothing else in that campaign; the branch's laterShardOccupancychange alters theUNORDEREDexpectation, and that change ships with its own PR, not this one. The conservation-counter work this commit sat beside on the branch is #336 and is deliberately not included here — where the two touch the same constructor, whichever merges second takes a one-line resolution.Verification.
OrderingModeDispatchParityTestandShardManagerTestgreen locally (5 tests).Checklist
docs/features/- N/A - an internal test instrument, no user-facing surface