Repository navigation
test(core): the Lincheck shard harness assigns the partition it already claims to model - #467
Merged
astubbs merged 2 commits intoSep 7, 2026
Conversation
…dy claims to model
CI red on the Lincheck lane, and the failure was the harness's fixture rather than the shard map.
ShardManagerLincheckTest.stressFindsNoWayToBreakTheShardMap reported an invalid execution -
revokeSweep(0) beside failure(0), then addWork(0): NullPointerException. Stress mode carries no
stack trace, so it was reproduced locally (1 of 1) to get one:
PartitionStateManager.getPartitionState returns null, dereferenced by
ProcessingShard.isWorkContainerStale from addWorkContainer's resident check.
Why it only appears now. This harness never assigned its partition, so getPartitionState answered
null for every container in it - and that was invisible because the branch that dereferences it needs
an ARRIVAL to find a RESIDENT at its offset. Until the declining revoke sweep landed on this branch,
a sweep always removed the resident and, under KEY ordering, the empty shard with it, so every
addWork was an insertion into nothing. A sweep that DECLINES - failure(0) holds the retry queue's
write lock through retryQueue.add, so the concurrent tryRemove refuses - leaves the resident in
place, and the next addWork takes the branch for the first time in this harness's life.
Not a product defect, checked rather than assumed: PartitionStateManager never removes from
partitionStates - onPartitionsRemoved REPLACES the entry with RemovedPartitionState (`partitionStates.put(removedPartition, RemovedPartitionState.getSingleton())`) - and a container only
reaches a shard through a poll for an assigned partition, so the map answers non-null for anything
resident. The unguarded read is real and is a known parked policy question, which
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.md owns and which this does not
touch; that note gains a dated paragraph recording that a harness has now reached the call, from a
state the product deliberately creates.
The fix is the fixture: the constructor assigns TP before planting the resident, which is what its
own comment already claims to model ("the ordinary steady state of a running consumer"). Same idiom
as WorkManagerLincheckTest's constructor.
Teeth checked, not assumed. Restoring the #345 defect this harness regression-tests against -
the containsKey-then-get pair in ShardManager.removeWorkFromShardFor with an unconditional deref -
turns it RED again on the fixed fixture, reporting revokeSweep(0): NullPointerException against
revokeSweep(0) (the sweep racing itself, the route the harness javadoc records). ShardManager was
restored byte-identical afterwards, verified by an empty diff.
Cost: 8.3s to 10.2s at the committed bounds. 4 of 4 green after (3 targeted repeats plus the whole
lane); 1 of 1 red before. Whole lane green in 2m53s (bin/lincheck-test.sh, build included):
ShardManagerLincheckTest 1/0/0/0, WorkManagerLincheckTest 2/0/0/0, RetryQueueLincheckTest 1/0/0/0,
PartitionStateLincheckTest 1/0/0/0, LincheckSuperHashCodeProbeTest 2/0/0/0,
LincheckToolchainProbeTest 2/0/0/0.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
(cherry picked from commit ea30a3e)
--- Re-verified on master, 2026-09-07, and one claim above does NOT hold here ---
Extracted from #431 as an independent change. Both claims re-measured on
this branch's base rather than carried over:
- THE NPE IS NOT REACHABLE ON MASTER. ShardManagerLincheckTest is GREEN with the fixture change
reverted (1 test / 0 failures, whole lane 37s). The branch that dereferences the null needs an
arrival to find a RESIDENT at its offset, and that needs the DECLINING revoke sweep, which is what
#431 adds and which has not landed. So on master this is latent: the
harness would go red on its own fixture the moment that PR merges, on the fixture rather than on
the shard map it exists to test. Fixing it here is deliberate - it is cheap, separable, and a
harness whose fixture contradicts its own comment ("the ordinary steady state of a running
consumer", which has its partition assigned) is a false green waiting for a schedule.
- THE TEETH DO STILL HOLD ON MASTER. Restoring the #345 defect this harness regression-tests
against - the containsKey-then-get pair in ShardManager.removeWorkFromShardFor with an
unconditional deref - turns it RED on the fixed fixture, reporting
"revokeSweep(0): NullPointerException ... Cannot invoke ProcessingShard.removeWorkAtOffset(long)
because the return value of Map.get(Object) is null" against revokeSweep(0) - the sweep racing
itself, the route the harness javadoc records. ShardManager restored, empty diff verified.
The fixture javadoc and the dated paragraph in
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.md are reworded accordingly: both
said the declining sweep HAD landed, which is true on that PR's branch and false here. The note keeps
the sighting - the datum it contributes is that the unguarded call is reachable from a state the
product deliberately creates, and that does not depend on the fix's outcome - and now says where it
was taken and that master was re-measured green.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
…readUtils Extracted from #431, where a ce-simplify pass found the same join-with-timeout-and-restore-the-interrupt wrapper hand-rolled in two tests under `state`. Only one of those tests exists on master, so what lands here is the hoist and not the deduplication: the wrapper moves to the shared test ThreadUtils, ShardPopulationRaceTest calls it, and its private copy goes. Hoisting a single-caller helper is deliberate and the javadoc says why rather than claiming a second caller that is not here. `join` with a timeout plus an interrupt restore is what every thread-racing test in `state` needs and what gets re-typed; the second private copy is how the drift starts, and putting it in the shared utility on the way past is cheaper than deduplicating two copies later. The null-thread no-op is kept for the same reason - it is what makes the helper callable from a `finally` block before it knows whether the thread was ever started, which is the shape a cleanup path needs. Behaviour unchanged: the call site's 500ms becomes Duration.ofMillis(500) and nothing else moves. Fresh surefire reports: ShardPopulationRaceTest 4/0/0/0, ShardManagerTest 5/0/0/0, RetryQueueTest 7/0/0/0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 1m 37s —— View job Review
Summary: two small, well-isolated test-harness changes, both check out against the production code they describe. No production code is touched. 1.
|
6 tasks done
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)
|
🧪🔒 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #467 +/- ##
============================================
+ Coverage 82.33% 82.88% +0.55%
- Complexity 1492 1509 +17
============================================
Files 95 95
Lines 5207 5211 +4
Branches 513 514 +1
============================================
+ Hits 4287 4319 +32
+ Misses 724 704 -20
+ Partials 196 188 -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 3% 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: c562df4, f502202, abd1d39, 60138dc, a532837, e8f7beb, 29f6a0f, a75400f, 12bf414, ce6f39a Updated for |
astubbs
marked this pull request as draft
September 7, 2026 10:08
astubbs
marked this pull request as ready for review
September 7, 2026 10:18
astubbs
deleted the
test/lincheck-shard-harness-assigns-its-partition
branch
September 7, 2026 10:19
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
One conflict, in `ShardPopulationRaceTest`, and both sides of it were superseded rather than one winning. #467 hoisted that test's private `joinQuietly` into the shared test `ThreadUtils`; this branch had extracted its `onNextStalenessCheck`, `shardWith` and `recordAt` helpers into `ShardSeamTestBase` so a second test could drive the same seam. The two edits are adjacent and disjoint, and git paired the deletions: HEAD's side of the hunk was the now-hoisted `joinQuietly`, master's was the three now-inherited helpers. Taking either would have restored a method whose owner is elsewhere, so the resolution deletes both - `ThreadUtils.joinQuietly` is already what the call site above it uses (from master), and the helpers are already inherited (from this branch). Neither `--ours` nor `--theirs` would have produced this. Nothing else in the three inherited commits touches this branch's area. #467's own subject - the Lincheck harness assigning its partition - is a fixture change in a different test, and its re-verification note says the NPE it fixes is not reachable on master. Verified: `bz.stub.parallelconsumer.state.*Test` green (162 tests, the one pre-existing skip in `WorkManagerTest`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…erit Base movement, four commits, three of them this PR's own dependencies: #465 (the widened ArchUnit walk, `@ControllerThreadOnly` and the control test), #467 (the Lincheck fixture fix and the `joinQuietly` hoist) and #451 (the revoke/epoch null work on the same rebalance path). WHAT THIS PR NOW IS. The gate landed ahead of the fix, RED, carrying twelve `KNOWN_BLOCKING_VIOLATIONS` entries with this PR named as the owner that deletes them. So this change's `ArchitectureTest` diff is no longer the widened walk - it is the DELETION of eighteen exemptions: the twelve #465 recorded plus the six `ReentrantReadWriteLock$WriteLock.lock()` entries that predated the widening. The list is back to one entry, confluentinc#857's transactional-revoke `Thread.sleep`, owned by #44. THE DELETION IS CHECKED, ONCE PER KIND OF REACH THE WIDENING ADDED - because a list that shrinks is otherwise indistinguishable from a rule that stopped looking. - Routing `ShardManager.removeWorkFromShardFor` back onto `retryQueue.remove`: green -> 12 violations, six roots (revoke and lost on all three classes) reported twice each, once as a `@ControllerThreadOnly call` and once as a `WriteLock.lock()` blocking call. - Restoring `.map(retryQueue::remove)` in `ShardManager.removeStaleContainers`: green -> 18, adding the three `onPartitionsAssigned` roots and reported as a METHOD REFERENCE - which is exactly the eighteen keys the merged list carried. - Both reverted, `git diff` empty afterwards, rule green again with the single entry. - The rule's own javadoc claim re-measured on the merged tree rather than carried over from the branch: annotating `RetryQueue.tryRemove` takes the rule from green to six violations, revoke and lost, through the annotation alone. EIGHT CONFLICTS, and the resolution rule was the same each time: master's copy is this branch's text extracted and reworded for a tree where the fix does not exist yet, so this branch's version wins wherever the fix is what makes it true, and master's wins where master's is later and still true. - `ArchitectureTest` (two hunks): this branch's. The exemption list is the single `Thread.sleep` entry, and the deleted javadoc paragraph is rewritten to the eighteen-entry deletion and the two measurements above. - `RetryQueue`: this branch's. Master's `remove` javadoc says "there is no declining alternative on this class yet" and points at this PR; `tryRemove` is that alternative and is in this diff. - `ControllerThreadOnly`: this branch's. Same sentence in the marker's javadoc, plus a `@see` that now names `tryRemove` rather than `remove`. - `ThreadUtils.joinQuietly`: this branch's. Master's javadoc argues at length that a single-caller hoist is still worth it; there are two callers here (`ShardPopulationRaceTest` and `RetryQueueRebalancePathTest`), so the argument it was defending against no longer applies. - `ShardManagerLincheckTest`: this branch's. Master's paragraph says the NPE is latent because the declining sweep "is still open" - it lands with this merge. - `ShardPopulationRaceTest`: master's (a trailing blank line). - `core-retry-queue-needs-a-runtime-controller-ownership-guard.md`: master's added sentence, which is later and still true, past-tensed and marked `post-merge: checked`. - `core-stale-arrival-guard-needs-a-null-safety-decision.md`: this branch's, which is the CORRECTED version - a Codex finding on this PR established the sighting is evidence for "test shortcut", not for production reachability. #467's dated re-measurement (green on master without the fixture fix) is preserved as one sentence rather than dropped with the paragraph. ONE SEMANTIC CONFLICT GIT DID NOT FLAG, which is the one worth reading. `docs/inflight/static-archunit-main-code-rules.md` auto-merged CLEAN and came out stating the same four things twice - the method-reference walk, the declared contract, why it is not Infer's `@ThreadConfined`, the constructor-call rejection - once in this branch's wording and once in #465's. Both sides had appended to the same section, at different offsets, so there was nothing for git to conflict. Reconciled to #465's copies (master owns that work now), with this PR's own dated update added: the eighteen entries gone, the two red proofs, and the note that deleting the original six first is what surfaced the unreported reach - the per-entry re-enable path this file recommends, being right about its own list. RECORDS. The solutions write-up now credits #465 for the walk and says the gate shipped red before the fix landed, and its closing measurement is the eighteen-entry deletion rather than six. 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; Lincheck lane 9/0/0/0 in 2m57s; core unit suite 882/0/0/8; `bin/ci-unit-test.sh` BUILD SUCCESS across every module; `bin/check-all.sh` 16 passed 0 failed; `bin/check-branch-self-reference.sh`, `bin/check-file-refs.sh` and `bin/check-issue-refs.sh` green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No fork issue exists for this work: it is test-harness hygiene extracted from #431, tracked only as in-flight notes. It depends on nothing.
Description
Two test-harness fixes extracted from #431 so they can land independently of the fix. Neither touches production code.
1.
ShardManagerLincheckTestassigns the partition it already claims to modelPartitionStateManager.getPartitionStateis an unguardedpartitionStates.get(tp), so a harness that never assigns its partition getsnullfor every container in it - andProcessingShard.addWorkContainerdereferences that, throughisWorkContainerStale, the moment an arrival finds a resident at its offset. The harness's own constructor comment claims to model "the ordinary steady state of a running consumer", which has its partition assigned. It did not. The fix is oneonPartitionsAssignedin the constructor, the same idiomWorkManagerLincheckTestalready uses.Re-measured on master rather than carried over, and one claim from the original commit does not hold here:
ShardManagerLincheckTestis green with the fixture change reverted (1 test / 0 failures, lane 37s). The branch needs a sweep that declines to leave the resident in place, and that is what fix(core): the rebalance callbacks decline the retry queue's write lock instead of waiting for it #431 adds. So on master this is latent - the harness would go red on its own fixture the moment that PR merges, on the fixture rather than on the shard map it exists to test. Fixing it here is deliberate: it is cheap, separable, and a harness whose fixture contradicts its own comment is a false green waiting for a schedule.containsKey-then-getpair inShardManager.removeWorkFromShardForwith an unconditional deref - turns it red on the fixed fixture:revokeSweep(0): NullPointerException ... Cannot invoke ProcessingShard.removeWorkAtOffset(long) because the return value of Map.get(Object) is null, againstrevokeSweep(0)- the sweep racing itself, the route the harness javadoc records.ShardManagerrestored, empty diff verified.The fixture javadoc and the dated paragraph in
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.mdare reworded accordingly - both said the declining sweep had landed, which is true on that PR's branch and false here. The note keeps the sighting, because the datum it contributes (the unguarded call is reachable from a state the product deliberately creates, not only from a test shortcut) does not depend on that PR's outcome. It does not touch the parked policy question that note owns.2. One
joinQuietlyfor the state testsThe same join-with-timeout-and-restore-the-interrupt wrapper was hand-rolled in two
statetests on #431's branch. Only one of them exists on master, so what lands here is the hoist, not a deduplication: the wrapper moves into the shared testThreadUtils,ShardPopulationRaceTestcalls it, its private copy goes, and the javadoc says why a single-caller helper is hoisted rather than claiming a second caller that is not here. Behaviour unchanged -500becomesDuration.ofMillis(500)and nothing else moves.Evidence
ShardManagerLincheckTest1/0/0/0,WorkManagerLincheckTest2/0/0/0,RetryQueueLincheckTest1/0/0/0,PartitionStateLincheckTest1/0/0/0,LincheckSuperHashCodeProbeTest2/0/0/0,LincheckToolchainProbeTest2/0/0/0.ShardPopulationRaceTest4/0/0/0,ShardManagerTest5/0/0/0,RetryQueueTest7/0/0/0 - fresh surefire.bin/ci-unit-test.sh.bin/check-all.sh: 19 ran, 16 passed, 0 failed.Checklist
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.mdgains the dated sighting, corrected for master.docs/inflight/working note - N/A as apr-/branch-note: nothing here thatghcannot show. The one durable finding (the unguarded deref is reachable from a real product state) is recorded in the note that already owns that question, rather than in a second note deleted at merge.@claude review thison the PR instead. The reuse finding behind commit 2 came from ace-simplifypass on the source branch.🤖 Generated with Claude Code
https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3