Repository navigation
fix(core): the two PartitionState flags that cross threads - measured, then fenced or redesigned - #469
Merged
Conversation
…ds unfenced Two probe pairs in `jcstress-poc/`, in the shape `CommitPathVisibilityProbes` uses for the `dirty` pair, for the two fields #349 deliberately left alone. Measurement only - no production code moves in this commit. `BackPressureFlagVisibilityProbes` - `allowedMoreRecords`, written on the broker-poll thread inside the commit path's encode and read on the control thread in `couldBeTakenAsWork`. * plain, with the real surrounding accesses: 1,643 anomalies in 179,391,234 samples (9.2e-6) * reduced, with the neighbours stripped: 213,452 in 455,930,388 (4.7e-4) - 51x higher, so the ConcurrentSkipListMap touch one statement earlier suppresses the window fiftyfold and does not close it. Same incidental-fencing shape the dirty pair measured at ~130x. * volatile flag: 0 in 201,927,188, FORBIDDEN. What that does NOT show, said plainly in the class javadoc: the note's harm for this field is *unbounded staleness* - a stale `false` admits nothing, so nothing succeeds, nothing re-dirties the partition, and the encode never runs again. "Never" is not an outcome a bounded run can observe. The real flag also publishes no payload, so the arms had to supply one, declared as instrumentation. They measure the fence, not the stall. `CommitWindowLostUpdateProbes` - `stateChangedSinceCommitStart`, written by both threads, with the arbiter reporting the invariant itself: a completion landing inside the commit window must leave the partition dirty unless the commit covered it. * as shipped (volatile dirty, plain flag): 142,177 in 91,295,031 (1.6e-3) * the flag made volatile as well - the obvious next step, and the arm that decides the design: 148,418 in 94,683,660 (1.6e-3). Statistically unmoved. Three mechanisms break the invariant and volatile addresses only one of them; the check-then-act in setClean() is reachable on sequentially consistent hardware, so it fires at interleaving rates, not store-buffer rates. * monotone completion count plus the count the commit covered: 0 in 121,707,028, FORBIDDEN. That middle arm is the reason the fix that follows is a protocol change and not a modifier, and it is the arm to re-run if anyone proposes going back. Calibration: CalibrationProbes.PlainFieldStoreLoadReordering fired at 50.82% in the same run, so the zeros are interpretable rather than vacuous. macOS 26.5.2, Apple M2 Pro, 12 CPUs, Temurin 17.0.18+8, `-m quick`. Rates are per raced pair, not production incidence. Nothing binds these arms to the real code - the module imports no bz.stub.parallelconsumer class, which docs/inflight/test-jcstress-probe-module-open-items.md already records as open. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…flags Names the two decisions on this branch a later reader is most likely to reverse on sight: that stateChangedSinceCommitStart is deliberately NOT fixed with volatile, and that the field collapse moves what #349 measured rather than discarding it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
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
|
| PR | Base | Change | |
|---|---|---|---|
| Clones | 106 | 104 | 🫤 +2 |
| Duplicated lines | 1499 | 1479 | :face_with_raised_eyebrow: +20 |
| Duplication | 0.85% | 0.84% | 🫤 +0.01% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 2% | ✅ Pass (0.85%) |
| Max increase vs base | +0.1% | ✅ Pass (+0.01%) |
⚠️ 2 new clones introduced
- 11 lines:
jcstress-poc/src/main/java/bz/stub/parallelconsumer/jcstress/BackPressureFlagVisibilityProbes.java:1<->jcstress-poc/src/main/java/bz/stub/parallelconsumer/jcstress/SeenSucceededOrderingProbes.java:1 - 11 lines:
jcstress-poc/src/main/java/bz/stub/parallelconsumer/jcstress/BackPressureFlagVisibilityProbes.java:6<->jcstress-poc/src/main/java/bz/stub/parallelconsumer/jcstress/CommitWindowLostUpdateProbes.java:7
Powered by astubbs/duplicate-code-cross-check
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
[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 #469 +/- ##
============================================
+ Coverage 82.16% 82.83% +0.67%
- Complexity 1535 1558 +23
============================================
Files 96 96
Lines 5343 5366 +23
Branches 529 533 +4
============================================
+ Hits 4390 4445 +55
+ Misses 761 729 -32
Partials 192 192
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 1 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: ad482ec Since the previous push: ratio 0.961 -> 0.928, share 1.718 -> 2.056, rate 62949 -> 58813 (-6.6%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
… completions instead of a shared flag The fix for the two `PartitionState` fields #349 measured and deliberately left alone. They get different fixes, because they are different defects, and the measurement in the previous commit is what decides which is which. WHAT THE PROBES ESTABLISHED `BackPressureFlagVisibilityProbes` - `allowedMoreRecords`, written on the broker-poll thread inside the commit path's encode and read on the control thread in `couldBeTakenAsWork`: * plain, with the real surrounding ConcurrentSkipListMap accesses: 1,643 anomalies in 179,391,234 samples (9.2e-6) * reduced, with the neighbours stripped: 213,452 in 455,930,388 (4.7e-4) - so the map touch one statement earlier suppresses the window fiftyfold and does not close it * volatile: 0 in 201,927,188, FORBIDDEN `CommitWindowLostUpdateProbes` - `stateChangedSinceCommitStart`, written by BOTH threads, arbiter asserting the invariant itself (a completion landing inside the commit window must leave the partition dirty unless the commit covered it): * as shipped (volatile `dirty`, plain flag): 142,177 in 91,295,031 (1.6e-3) * the flag made volatile as well: 148,418 in 94,683,660 (1.6e-3) - statistically unmoved * monotone completion count plus the count the commit covered: 0 in 121,707,028, FORBIDDEN Calibration: `CalibrationProbes.PlainFieldStoreLoadReordering` fired at 50.82% in the same run, so the zeros are interpretable rather than vacuous. THE TWO FIXES, AND WHY THEY DIFFER `allowedMoreRecords` is `volatile`. It has one writer, so a release/acquire edge is the whole fix, and its javadoc now carries the figures, the arm name and the direction of the crossing - written on the broker-poll thread, read on the control thread, which is the OPPOSITE direction to the commit-count fence, so that fence does not cover it. `stateChangedSinceCommitStart` and `dirty` are gone, replaced by a monotone `AtomicLong completionCount` and the `completionCountCommitted` a successful commit publishes. "Is this partition dirty" is a comparison of two values rather than a flag somebody clears. Making the second boolean `volatile` was the obvious step and the one the inflight note recorded as next. It was REJECTED on measurement, not on argument: the arm above moves the anomaly rate by nothing, because only one of the three mechanisms breaking the invariant is a visibility effect. The other two - a check-then-act in the clean-marking step, and a lost update between two writers - fire at interleaving rates on sequentially consistent hardware, and no modifier addresses either. `CommitWindowLostUpdateProbes.VolatileStateChangedFlagAcrossTheCommitWindow` exists solely to hold that line; re-run it before proposing the modifier again. The release/acquire edge #349 measured is preserved, not discarded: an `incrementAndGet` is a strictly stronger release than the volatile store it replaces, and the counter load on the commit path is the paired acquire, so the plain `long`s it published are published exactly as before. They stay plain for the same reason that PR gave. THE RED PROOF `PartitionStateCommitWindowSeamTest` drives the one interleaving the old protocol lost a completion to - inside the clean-marking step, between its read of the flag and its write of `dirty`. Master offered no override point between those two instructions, so `PartitionState.onCommitWindowClosing()` was added: an overridable no-op, the same one-shot-armed seam shape as `RacingSeamWorkManager`, with firing tracked separately from arming so a test that forgot to arm cannot pass while asserting nothing. Redness is shown twice. A permanent control arm reverts only the protocol and asserts the loss, so the defect stays runnable after the fix removed it. And transiently, against the production code: reverting the protocol in `PartitionState` itself while keeping the seam turns the pinning arm red. PartitionStateCommitWindowSeamTest.aCompletionInsideTheCleanMarkingStepLeavesThePartitionDirty a completion landing inside the clean-marking step must leave the partition dirty - the commit did not cover it, so nothing else will commit it until the next completion, and on a partition that then goes idle the offset waits for the next rebalance value of: isDirty() expected to be true Tests run: 3, Failures: 1, Errors: 0, Skipped: 0 Stashing the whole main-code change does NOT produce that - it removes the seam too, and the test stops compiling. Only the protocol may be reverted, which is why the control arm is in the tree. `PartitionStateDirtyFlagFenceTest` becomes `PartitionStateCrossThreadFieldFenceTest`, pinning `volatile` on `allowedMoreRecords`, `final AtomicLong` on the counter and `volatile` on the committed count. Moving that guard rather than deleting it is what its own failure message asked for when the fencing was redesigned. LEDGERS AND NOTES `docs/refactoring.md`'s `AT_STALE_THREAD_WRITE_OF_PRIMITIVE` entry was wrong for a month in the direction that matters: it listed two fields and a count while the analyser named six, three of them in `PartitionState` - the class #349 was fencing at the time - and one (`ProgressTracker.highestRoundCountSeen`) that no ledger anywhere carried. It now carries the reproduce command instead of a membership to maintain, because the lane runs `spotbugs:check` with `failOnError=false` and nothing goes red when the paragraph rots. The engine `AGENTS.md` ledger records the rule the pair of fields makes: a modifier is not the default answer to a shared field, it is the answer to a one-writer field. That rule and its evidence are written up in docs/solutions/logic-errors/volatile-is-the-fix-for-a-one-writer-field-not-a-shared-one-2026-09-07.md, which is where the two retired `docs/inflight/bug-*` notes' durable content went. Two local-only test failures seen while verifying are recorded as sightings in `docs/inflight/test-untracked-ci-flakes.md` rather than left unrecorded - `ReactorPCTest.concurrencyTest` and `CommitResponseTimeoutSymptomTest.aRebalanceStormUnderAHighFailureRateNeitherStallsNorKillsTheConsumer`, each with its same-base control arm and the reason neither is read as a regression of this change. Verified: full unit suite green (`bin/ci-unit-test.sh`), `bin/check-all.sh` 16 passed 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 3m 18s —— View job Review: PartitionState cross-thread flags (jcstress-measured fix)
OverallThis is a careful, well-evidenced fix. The core insight — "
On the new
I didn't find any leftover references to the removed Minor, non-blocking
No correctness issues found in the production change. Recommend proceeding, contingent on the two open duplicate-code threads getting an explicit dismissal per the repo's own PR-discipline rule. |
…equal Claude's review on #469 flagged the ternary/OR assignments in CommitWindowLostUpdateProbes and BackPressureFlagVisibilityProbes as dead branches: both sides produce the same value, which reads as a mistake to a reader who has not traced it against PartitionState. They are deliberate, not a mistake. tryToEncodeOffsets() in PartitionState.java computes offsetOfNextExpectedMessage before it branches on incompleteOffsets.isEmpty(), and returns that same value from both branches - so an arm that models the branch faithfully has to land on the same value either way, or it stops performing the read production performs. blockedAtPayloadLength = ignoredEmpty || true mirrors the same shape for the back-pressure arms, which model the over-threshold path production takes regardless of isEmpty(). Kept both, with a one-line comment at each of the five sites explaining why the branch stays and why its value does not vary. Also answers the duplicate-code bot's flags on both files: added one sentence to each class javadoc saying the plain/volatile arm pairs are deliberately near-identical copies with only the modifier under test varied, and must not be deduplicated - a jcstress arm has to be a copy of its neighbour with one term changed or the comparison between arms stops isolating what it measures. Verified jcstress-poc still compiles and packages per its own pom.xml recipe: JAVA_HOME=<jdk17> ./mvnw -f jcstress-poc/pom.xml clean package - clean, jar produced. Did not run the probes themselves (a separate, hours-long invocation). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
[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. |
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…et; only the clean mark waits for the highest one in flight The sequence guard the previous commit added ignored the answer to a superseded commitAsync request outright. That is the wrong shape, and the maintainer's call replaces it: a late acknowledgement is TRUE - the broker really did commit up to the offsets it names, for every partition it names - so dropping it throws away a fact the broker established. The only thing that must be withheld is the CLEAN mark, and only for a partition that still has a higher offset in flight, because that is the state that would leave nothing dirty to re-send if the newer request then failed or was dropped. THE RULE, PER PARTITION. ConsumerOffsetCommitter keeps highestOffsetInFlight, a map written on send (merged with Math::max, so it only rises while a request is outstanding) and cleared by the acknowledgement that matches it - an atomic compare-and-remove, because sends and answers are not always on the same thread. asyncCommitSequence and the sequence != latest check are gone. On an acknowledged success, each partition in the answer goes one of two ways: its offset IS the highest in flight record it, and mark the partition clean a newer request has passed it record it, and leave the partition dirty A request is routinely the newest word on one partition and superseded on another - the commit carries every dirty partition, and only the ones that completed more work move - which is why the decision cannot be per request. The recording half is PartitionState.onSupersededOffsetCommitSuccess, beside the existing onOffsetCommitSuccess; both now go through recordCommittedOffset, which advances lastCommittedOffset MONOTONICALLY. That guard lives where the field lives: the field is what pc.partition.latest.committed.offset reads, and an out-of-order answer would otherwise walk the gauge backwards. PartitionState's clean protocol is otherwise untouched - setClean still declines when state changed during the commit window, and the counter protocol on #469 is deliberately not pulled in. On a failure nothing moves, as before, and the line now says which promise it is making. ERROR - "these offsets stay dirty and are re-committed next cycle" - when the request still carries the highest in-flight offset for at least one partition, because for those partitions the promise is exactly true. WARN naming the supersession when it carries it for none, because then a newer request decides every offset on the line and the ERROR would be a claim this answer is not entitled to make. The #168 (confluentinc#629) bound applies to both: the offsets are summarised, never the map. Sync mode is untouched - commitOffsetsReturnsOnlyOnceAcknowledged() still returns isSync(), so the blocking path still marks success inline, and MockConsumerSyncCommitMarksCleanOnReturnTest stays green. RED PROOFS. The committer's decision rule, proved against the sequence guard still in place - the new WorkManager/PartitionState methods existed so the tests could compile, so the one term flipped is onAsyncCommitAnswered itself. Four of the five went red, each naming its own mechanism: anOlderAnswerIsRecordedAndLeavesThePartitionDirtyForTheHigherOffsetStillInFlight the broker committed up to 100, so that offset is recorded whether or not it is the newest word on the partition / expected: [100] but was: [] anAnswerThatArrivesAfterAHigherOneAppliesNothing expected: [100] but was: [] - the late answer was dropped instead of recorded theOlderAnswerMarksCleanTheOnePartitionTheNewerRequestDidNotSupersede Wanted but not invoked: workManager.onOffsetCommitSuccess({...-1 = offset=50 ...}); Actually, there were zero interactions with this mock - a whole-request guard cannot mark one partition clean and leave the other dirty aFailureStillCarryingTheHighestOffsetForOnePartitionKeepsTheReCommitPromise captured ERROR lines ... expected: 1 but was: 0 - reported as supersession although the promise was true for partition 1 The fifth, aFailureSupersededOnEveryPartitionSaysTheNewerRequestDecidesTheOutcome, passes under both rules; it is the previous commit's case, kept and reworded. PartitionStateAcknowledgedCommitOffsetTest asserts the other side of the seam on a real PartitionState - what each entry point does to the offset and the dirty flag - and was proved by sabotage, one term at a time: monotonic check removed (assign unconditionally) aLateAcknowledgementCannotWalkTheCommittedOffsetBackwards expected: 200 but was: 100 onSupersededOffsetCommitSuccess made to setClean() too aSupersededAcknowledgementRecordsItsOffsetAndLeavesThePartitionDirty expected to be true Both restored; all three green afterwards. ConsumerOffsetCommitterSupersededAsyncCommitTest is renamed to ConsumerOffsetCommitterOverlappingAsyncCommitTest - the subject is no longer a guard against superseded answers but what an overlapping pair of requests each moves - and AsyncCommitterFixture gained a two-partition commit, which is the only shape that distinguishes a per-partition rule from a whole-request one. WorkManager gained a three-line delegation beside its sibling rather than a new responsibility, so no decomposition was attempted here; the file's refactor candidacy is unchanged. Gates: bin/ci-unit-test.sh green across every module, bin/check-all.sh 16 passed 0 failed, bin/check-issue-refs.sh clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ans it, so the committer keeps no in-flight map The maintainer's call on the previous commit, which duplicated state the partition already tracks. ConsumerOffsetCommitter kept its own per-partition highestOffsetInFlight map, split every acknowledgement into two buckets, and plumbed a second success method - onSupersededOffsetCommitSuccess - through AbstractOffsetCommitter, WorkManager, PartitionStateManager and PartitionState, to reach an outcome the partition produces on its own. Only the CLEAN mark ever needed guarding, and the partition knows the offset it offered. All of that is replaced by one rule. THE RULE. PartitionState.getCommitDataIfDirty remembers the offset it hands to the committer, in one new field, offsetLastOfferedForCommit. onOffsetCommitSuccess then records every acknowledgement - the answer is TRUE, the broker committed up to the offset it names, and recordCommittedOffset keeps the higher of the two when answers arrive out of order - and marks the partition clean ONLY when the offset acknowledged is that offer. setClean's existing protocol is otherwise untouched: it still declines when state changed during the commit window, and the counter protocol on #469 is deliberately not pulled in. commitOffsetsReturnsOnlyOnceAcknowledged() and the sync path are unchanged. The committer's async callback is now two branches. On success it passes the acknowledgement through whole. On failure it logs ONE line at WARN saying the partitions stay dirty and are committed when a later request is acknowledged, with the #168 (confluentinc#629) summarisation and the DEBUG line carrying the full map, and returns. The previous ERROR-or-WARN split existed only because the in-flight map could tell the two cases apart; nobody on call needed the distinction, and a routine coordinator hiccup is not an ERROR when nothing was lost and nothing was marked clean. Deleted: highestOffsetInFlight, recordAsInFlight, acknowledgeTheHighestOffsetInFlight, stillCarriesTheHighestOffsetInFlightForAnyPartition, and onSupersededOffsetCommitSuccess in all four classes. Added: one long. THE PER-PARTITION OUTCOME SURVIVES, AND NOW FALLS OUT. A commit carries every dirty partition and only the ones that completed more work move, so a request is routinely the newest word on one partition and superseded on another. In the second round the partition that stood still is re-offered at exactly the same offset, so the first round's answer IS its latest offer and cleans it, while the partition that moved on stays dirty. Nothing implements this; each partition made its own offer, so each partition decides. Every edge lands on the dirty side, which costs at most one extra commit and cannot under-report: a partition state rebuilt by a rebalance starts at KAFKA_OFFSET_ABSENCE, so an acknowledgement addressed to the assignment before it cannot clean it either - better than the previous unconditional setClean(). THE FIELD'S THREAD MODEL, per the engine's AGENTS.md. Written in the commit path (getCommitDataIfDirty, reached from collectCommitDataForDirtyPartitions) and read back in onOffsetCommitSuccess, by the SAME thread in every commit mode: the broker-poll thread under both consumer commit modes - it sends the request and Kafka delivers the callback from the poll it drives - and the control thread under PERIODIC_TRANSACTIONAL_PRODUCER, where the commit blocks and the acknowledgement is recorded inline. The one hand-over is Consumer#close() flushing a pending callback after the poll loop has finished, which carries a happens-before edge rather than an overlap. So it is plain, for the reason the dirty field's javadoc gives from jcstress measurements: that flag is this class's fence and the longs stay plain deliberately. Same lifecycle as stateChangedSinceCommitStart, and the non-volatile ledger in docs/refactoring.md now records the pair so neither is "fixed" with volatile and so they are declared together if anyone does the @ThreadConfined work. Assigned rather than raised to a maximum: the offer rises with completed work, so the last offer is the highest in flight, while a monotonic maximum would also survive maybeTruncateBelowOrAbove's downward reset on a bootstrap poll - after which the partition could never match its own offer again and would be dirty forever, committing nothing. RED PROOF - the PartitionState rule reverted (unconditional setClean() on success restored), nothing else touched. Tests run: 7, Failures: 4: ConsumerOffsetCommitterOverlappingAsyncCommitTest .anOlderAnswerLeavesThePartitionDirtyForTheHigherOfferStillOutstanding offer 2 is still unanswered, so the next cycle must still be willing to send it / expected: {...-0=2} but was: {} ConsumerOffsetCommitterOverlappingAsyncCommitTest .oneAnswerCleansThePartitionItIsTheLatestOfferForAndLeavesTheOtherDirty only the partition a newer offer is outstanding for may stay dirty / expected: {...-0=2} but was: {} PartitionStateAcknowledgedCommitOffsetTest .anOlderAcknowledgementAfterAHigherOfferRecordsTheOffsetAndLeavesThePartitionDirty offer 201 is still unanswered ... expected to be true PartitionStateAcknowledgedCommitOffsetTest .oneAnswerCleansThePartitionItIsTheLatestOfferForAndLeavesTheOtherDirty a newer offer for this partition is still unanswered ... expected to be true The empty map is the partition marked clean at the older offset while the higher offer is still outstanding - exactly the state that leaves nothing to re-send. Restored, all seven green. The out-of-order case passes under both rules, since the clean mark is already taken by the newer answer; it is kept because it pins the other half - the late answer must not put the partition BACK into a re-commit of offsets the broker already has. TESTS. PartitionStateAcknowledgedCommitOffsetTest pins the rule where it lives, on a real PartitionState: older ack after a higher offer records and does not clean; ack of the offered offset cleans; a lower ack after a higher recorded offset moves nothing backwards; and the mixed case on two partitions. Every test reads the offer back rather than naming an offset - the offered offset is derived from the incomplete set, so a hard-coded one would pass against a partition that offered something else entirely. ConsumerOffsetCommitterOverlappingAsyncCommitTest drives the same three orderings through the committer with two captured callbacks, now over a real WorkManager and real partition states rather than a WorkManager mock: with the decision inside PartitionState, asserting at the mock boundary would assert almost nothing. It asserts what reached the broker (the captured commitAsync maps) and what the next cycle would still send (collectCommitDataForDirtyPartitions - the consequence rather than the flag, and the exact call whose emptiness was the original defect). AsyncCommitterFixture now hands back each send's offsets and callback together, so a test says what reached the broker rather than assuming it. ConsumerOffsetCommitterAsyncFailureLoggingTest moves from ERROR to WARN and additionally asserts NO ERROR, so a false alarm reappearing on a routine coordinator hiccup fails the build. Its per-entry length bound - the half guarding #168 - is untouched. The four MockConsumer async and sync tests are unchanged and stay green. The static: analysis lane was RED on the previous head over a ratchet identity, NULLPTR_DEREFERENCE PartitionStateManager.lambda$onSupersededOffsetCommitSuccess$4. This deletes the method the finding was in rather than adding a ratchet line for it; bin/infer-test.sh now reports 10 findings, all known, none new. Gates: bin/ci-unit-test.sh green across every module, bin/check-all.sh 16 passed 0 failed, bin/check-issue-refs.sh and bin/check-file-refs.sh clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
6 tasks done
…one predicate defines dirty Two maintainer questions on #469, and their answers. Behaviour is unchanged - no assertion in any test moved, and the full unit suite is green. QUESTION 1: setDirty() is a misnomer now that dirty is derived. Correct. Nothing sets a flag any more. The method's only caller is onSuccess, and what it does is increment a monotone counter that is a VERSION STAMP, not a quantity: no reader anywhere uses the number for its own sake, only its inequality with the count the last acknowledged commit covered. A name that says "set the dirty flag" describes a flag this branch removed, and it invites the next reader to look for the matching clear. It is now recordCompletion() - the method records that a completion landed and the partition's state moved on. Whether that leaves the partition dirty is isDirty()'s answer, not this method's. isDirty() stays as the derived query and gains the definition it was missing: DIRTY MEANS A COMPLETION HAS LANDED THAT NO ACKNOWLEDGED COMMIT HAS COVERED. That sentence is the thing three documents and two tests were each paraphrasing differently. Renamed everywhere the live tree names it for this class: the completionCount javadoc, the declaration and its one caller, the GenerationCountedCommitWindow arm's marginal comment in jcstress-poc (the arm that models the POST-fix protocol), the engine AGENTS.md ledger entry, the solutions write-up, the docs/inflight/ working note, and two integration-test comments (TransactionTimeoutsTest, CloseAndOpenOffsetTest) that named it while explaining why a commit is or is not attempted. RemovedPartitionState does not touch the commit protocol and needed nothing. Deliberately NOT renamed: the dated records under docs/plans/ and docs/solutions/ that describe the retired two-flag protocol, and the CommitWindowLostUpdateProbes / CommitPathVisibilityProbes class javadoc and plain/volatile arms, which model the code as it stood before this branch. Those say what was true when they were written; docs/citations.md permits repairing an address, not rewriting a claim, and the address they want is the old one. docs/inflight/bug-torn-read-family.md is the exception - a LIVE note whose fencing argument turns on "exactly one call site", so its anchor was repaired to the current name with the old one in parentheses. QUESTION 2: getCommitDataIfDirty() duplicates the dirty comparison. It did, and the duplication was load-bearing rather than lazy - which is exactly why it deserved a name instead of a second copy. The collecting cycle must stash the very count it TESTED, not a second load: a completion landing between two loads would be published as covered while its offset need not be in the metadata this cycle goes on to encode. That is the burnt commit cycle this PR exists to close, so the fix could not be "just call isDirty()". The comparison now lives once, in a private isDirtyAt(long sampledCompletionCount). isDirty() passes a fresh load; getCommitDataIfDirty() passes the sample it then stashes, with a comment at the sample site saying why the count is loaded once. Identical bytecode-level behaviour - same two reads in the same order, same field written. VERIFICATION - PartitionState*, LastCommitTimeFenceTest: 33 tests, 0 failures. PartitionStateCommitWindowSeamTest (3, including the control arm that asserts the old protocol's loss) and PartitionStateCrossThreadFieldFenceTest both re-ran fresh and green - the fence test still finds completionCount by reflection under its unchanged name. - bin/ci-unit-test.sh: BUILD SUCCESS, all 11 modules. - bin/check-all.sh: 19 ran, 16 passed, 0 failed. check-branch-self-reference.sh caught both new self-references before they could rot and both were fixed rather than marked around: the bug-torn-read-family repair dropped its PR number (the sentence reads correctly forever without it), and the working note's new first line moved inside the existing post-merge checked block. - bin/check-issue-refs.sh and bin/check-file-refs.sh: clean. WORKING NOTE docs/inflight/pr-partition-state-cross-thread-flags.md gains the new vocabulary and settles the first of its two reviewer push-backs. "Is the seam new production code, or one master already had" is answered: the review at #469 (comment) searched for an overridable hook on the commit path at that instruction and found none, reporting no counter-evidence, so onCommitWindowClosing() keeps its justification. It is recorded as a verdict with its citation rather than left standing as an open question. The note also now states in its first line that it is deleted in this PR's final commit at merge prep, per docs/inflight/AGENTS.md - the marker must never reach master, so the file must not either. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…-cross-thread-flags # Conflicts: # docs/inflight/test-untracked-ci-flakes.md
…nly the measured term apart Answers the duplicate-code check on #469, which flagged two clones - 54 lines inside CommitWindowLostUpdateProbes and 40 inside BackPressureFlagVisibilityProbes. The previous commit's answer was a javadoc sentence saying the duplication was deliberate. That was half right, and the half it got wrong is the half the bot was pointing at: only a small part of each cloned block is the experiment, and the rest was scaffolding copied two or three times. THE RULE THAT DECIDES WHAT MOVES A member may be hoisted if it contains no access to the arm's measured field. That single test sorts every line in both files, and it is written into both class javadocs so the next reader applies the same one rather than re-deriving it. STAYS DUPLICATED, DELIBERATELY - and the javadoc now says why, per file: * The measured field keeps its own declaration in its own @State class. plain versus volatile on that one line IS the term under test; hoisting it would erase the difference between the arms and the comparison would stop isolating anything. Each such declaration now carries a THE TERM UNDER TEST comment naming what varies. * Each actor's sequence of accesses stays literal in the arm, so the JIT compiles the shape production has, in the order production has it. That is why BackPressure's controlThread keeps both of its two lines rather than calling a shared result mapper - it is an actor, its two reads happen inside the raced window, and their order is the measurement. * ReducedPlainBackPressureFlag shares nothing and does not extend the base at all. Its whole point is the ABSENCE of the surrounding accesses; giving it the scaffolding would make it a third copy of the faithful arm and destroy the 51x comparison it exists to provide. HOISTED, because none of it is measured: CommitWindowLostUpdateProbes.CommitWindowState (all three arms) - the ConcurrentSkipListMap and the setup that seeds it, offsetHighestSucceeded, the offsetThisCycleCommitted instrumentation, applyCompletionToOffsetState() (onSuccess's map removal and highest-succeeded write), encodeAndCaptureCommittedOffset() (tryToEncodeOffsets' map touch and the plain read, carrying the equal-sided branch and its explanation once instead of three times), and reportInvariant(), the arbiter's result mapping. The arbiter may use a shared helper where an actor may not: it runs after both actors, outside the raced window. BackPressureFlagVisibilityProbes.SurroundedBackPressureState (the two faithful arms only) - the map and its setup, the neighbouring stateChangedSinceCommitStart write, the blockedAtPayloadLength instrumentation payload, and encodeOverThreshold(), everything the poll thread does before writing the flag under test. The three duplicated constructors are gone with the field declarations - each arm now inherits the base's, and gets its implicit public no-arg constructor, which is what jcstress requires. JCSTRESS 0.16 PERMITS THIS - VERIFIED, NOT ASSUMED Read out of the annotation processor rather than taken on faith: the checks on a @State class are that it is public and not final. The "should not inherit other classes" rule that does exist in 0.16 binds @Result classes, not @State - ZZ_Result is untouched here. Actors and arbiters stay declared on each arm regardless, which is what the literal-actor rule above requires anyway, so no annotated method is inherited. Confirmed empirically as well as by reading: jcstress -l lists all six arms after the change, so none was silently dropped from generation. VERIFICATION - the outcome KINDS are unchanged Re-ran the changed arms: -m quick, Temurin 17.0.18+8, macOS / Apple silicon. * Both forbidden arms still read zero. GenerationCountedCommitWindow and VolatileBackPressureFlagPublishesEncode have no "false, false" row at all across millions of samples per configuration - the FORBIDDEN outcome was never observed, and jcstress reports no failed and no error tests. * All four plain and reduced arms still fire, all four reported Interesting: PlainStateChangedFlagAcrossTheCommitWindow, VolatileStateChangedFlagAcrossTheCommitWindow (the arm whose whole point is that the anomaly SURVIVES the volatile - it still does), PlainBackPressureFlagPublishesNothing and ReducedPlainBackPressureFlag. Rates are not re-recorded, deliberately. A -m quick run is not the mode the recorded figures came from, and a rate is machine- and load-specific, so overwriting the recorded numbers with today's would trade a documented measurement for an incomparable one. Both class javadocs now say the recorded figures predate this refactor, that the re-run confirmed the outcome kinds rather than the rates, and that re-running is how to get today's numbers. Also verified: bin/ci-unit-test.sh BUILD SUCCESS with no failures, and bin/check-all.sh 19 ran, 16 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
Owner
Author
|
@codex review |
…not an oversight The working note's first section is "the decision #469 takes that the next reader is most likely to reverse on sight". The probe refactor in the previous commit created a second one, and it has the same shape as the first: a thing that looks like an obvious tidy-up, and is not. A dedupe pass reading the two probe classes now sees a base class holding the shared scaffolding and two or three arms that still repeat a field declaration and a short actor body. Finishing the job is the wrong move - the measured field's own declaration per @State class IS the term under test (plain versus volatile on that one line), and each actor's literal sequence of accesses is what makes the JIT compile the shape production has. Recorded as a pointer, not a copy. The two class javadocs own the split and the rule behind it - a member may be hoisted only if it contains no access to the arm's measured field - and the note names them as the owner rather than restating the reasoning, so the two cannot drift apart. bin/check-all.sh: 19 ran, 16 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
[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. |
![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: 41ed25a4b9
ℹ️ 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".
…op re-listing what a command derives Three of Codex's four findings on #469, verified against the code rather than taken on trust. Each was checked before acting; the fourth is answered in its thread and deliberately not acted on. 1. recordCompletion() discarded incrementAndGet()'s return as a bare statement (P1) Correct, and it broke a rule this repo states outright: never discard a return value silently, name it and say why, because a bare call reads identically whether ignoring the result was a decision or an oversight. It was a decision - the counter is a VERSION STAMP, not a quantity, and no reader anywhere wants the number for its own sake, only its inequality with completionCountCommitted, which isDirtyAt(long) asks on the committer thread. The value is now bound to ignoredNewCompletionCount with a comment carrying that reasoning. Behaviour is unchanged; the increment and its release edge are the same instruction. 2. docs/refactoring.md re-listed the SpotBugs membership it had just said not to list (P2) The AT_STALE_THREAD_WRITE_OF_PRIMITIVE entry says, in bold, "Re-derive the membership, do not trust a list here" - and then, two lines later, listed four fields. The paragraph beneath it is the record of a maintained list that was wrong for a month in the direction that reads as an all-clear, which is why the reproduce command replaced it. So the entry re-committed, in its own next sentence, the mistake it exists to document. The enumeration is gone; the command stays, with a line saying the omission is deliberate. ProgressTracker.highestRoundCountSeen survives in the paragraph below as the worked example of a field no ledger carried - a claim about the past, which does not rot. 3. A flake-ledger row carried a count a command answers (P1, in part) One clause did: "bin/inflight.mjs codecov test records 10 runs, all pass". That is the case docs/inflight/AGENTS.md names as the one that keeps slipping through, and it is now the command itself, with the test named and no total. The rest of that finding is refused, with the reasoning in the thread. The remaining figures are LOCAL run outcomes - "2 of 3 full unit-suite runs", "0 of 1 full-suite run on the same-base control worktree" - and no command can regenerate them: bin/inflight.mjs codecov reads CI history only, while these ran on a laptop and on a control worktree cut at this branch's own base commit. That control arm is the reason the rows are worth reading at all, because it separates "this branch caused it" from "this machine under load". Stripping the rates leaves "sometimes fails", which is the verdict-without-a-rate the root AGENTS.md rejects. Every pre-existing row in that file records local sightings the same way. NOT ACTED ON, and answered in its thread: deleting docs/inflight/pr-partition-state-cross-thread-flags.md Codex read the note's "deleted before merge" line as a promise left as prose. The rule it cites forbids a "delete this when it merges" marker ON MASTER, because the merge is when nobody is looking - and this note never reaches master: it is deleted in the final commit at merge prep, before the merge, so the file and the marker go together. Meanwhile docs/inflight/AGENTS.md positively REQUIRES the note, and requires it early - "a PR earns its working note at its first commit, not its last" - and the PR template has a checklist box asking for it. Deleting it now would strip the reviewer-facing context while the PR is still an open draft, which is exactly when the note is doing its job. VERIFICATION - PartitionState*, LastCommitTimeFenceTest: 38 tests, 0 failures. - bin/ci-unit-test.sh: BUILD SUCCESS, no failures. - bin/check-all.sh: 19 ran, 16 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3748baabe
ℹ️ 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".
… the protocol arm's zero may be vacuous Codex's second review round on #469, against e3748ba. Three findings, all three verified against the code and the recorded jcstress run before acting. No production code changes here - every one of them was a claim in prose that was wrong or unsupported, which is exactly the failure mode this repo keeps writing rules about. 1. THE PROTOCOL ARM'S FORBIDDEN ZERO MAY BE UNREACHABLE BY CONSTRUCTION (P1, confirmed, left OPEN) CommitWindowLostUpdateProbes.GenerationCountedCommitWindow reports 0 anomalies FORBIDDEN in 121.7M samples. Codex argued the corner cannot be reached at all, which would make that zero vacuous rather than evidence. Checked against the run rather than argued: the two flag arms each reached ALL FOUR outcomes, while the protocol arm reached TWO - it never even produced "covered and still dirty". The mechanism is an asymmetry between the arms that nobody had noticed. The flag arms are SEEDED DIRTY - dirty and stateChangedSinceCommitStart are both true at construction - so the poll actor always enters the commit window and the completion lands inside it. The protocol arm starts CLEAN, both counters zero, so the poll actor enters the window only if it has already seen the completion: sample 0 and it skips the window; sample 1 and that AtomicLong acquire also publishes the map removal and offset write that preceded the release, so the commit necessarily covers the completion. The arms are not comparing like with like. Not fixed here, deliberately, because the obvious fix is wrong. Seeding this arm dirty makes the window open reliably but leaves the corner unreachable for the same release/acquire reason - which says the protocol is correct, not that the probe could detect it being incorrect. Showing the probe has power needs a deliberately broken protocol variant as a negative control, the way PartitionStateCommitWindowSeamTest keeps a control arm that asserts the old defect. That is a design decision about what the arm should establish, so it goes to the maintainer rather than being guessed at, and its review thread is left open. Recorded where a reader will hit it: the arm's own javadoc now carries the caveat and the outcome-set evidence, and the working note has it as an open item. Until it is settled the zero is to be cited as "no anomaly observed", never as "the window was measured shut". The plain-versus- volatile pair that the PR's argument actually rests on - 1.6e-3 against 1.6e-3 - is untouched by this. 2. THE FLAKE ROW'S DIRECTION-OF-EFFECT ARGUMENT IS ONE-WAY REASONING ABOUT A TWO-WAY FIELD (P2) The ReactorPCTest.concurrencyTest row claimed allowedMoreRecords going volatile "can only make back-pressure's false visible sooner and therefore admit fewer records - it has no mechanism for admitting more". False, and the code says so plainly: tryToEncodeOffsets() calls setAllowedMoreRecords(TRUE) on the incompleteOffsets.isEmpty() path, and updateBlockFromEncodingResult does the same when the payload comes back under the pressure threshold. The fence publishes the UNBLOCKING transition sooner too, which admits more records sooner. Which direction dominates is unmeasured. That mattered: the argument was the only thing standing against this branch being a contributor to a flake the row itself says is unexplained, and the row instructs the next person to classify before touching. The claim is withdrawn in place, with the reason and the two call sites named, so nobody re-derives it. 3. THE SPOTBUGS REPRODUCE COMMAND COMPILED NOTHING (P1) The AT_STALE_THREAD_WRITE_OF_PRIMITIVE entry told the reader to re-derive membership with `./mvnw -o spotbugs:spotbugs -pl :parallel-consumer-core`. That analyses whatever bytecode already happens to be under target/ - nothing at all on a clean checkout, stale classes after an edit - and never writes target/test-classes, which the includeTests setting needs. A command that silently analyses nothing and exits clean is the same false all-clear the entry exists to warn about, so the entry was undermining itself in its own reproduce line. It is now the `static: spotbugs` step's own command, verbatim, with a note saying to run it as written and why -pl without -am reintroduces the trap docs/building.md owns. VERIFICATION - jcstress-poc packages clean; all six arms still generated. - bin/check-all.sh: 19 ran, 16 passed, 0 failed. - No main or test Java changed, so the suites from e3748ba still stand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…-cross-thread-flags # Conflicts: # docs/inflight/bug-allowed-more-records-crosses-threads-unfenced.md # docs/inflight/bug-state-changed-since-commit-start-written-from-both-threads.md # docs/inflight/bug-torn-read-family.md
[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. |
…y construction, and its green says so CommitWindowLostUpdateProbes.GenerationCountedCommitWindow reports its FORBIDDEN outcome at zero, and that zero is vacuous. Both counters start at zero, so the poll actor enters the commit window only once it has already observed the completion, and the AtomicLong acquire that lets it in also publishes the map removal and the offset write that preceded the release - so the commit necessarily covers the completion. The corner stays out of reach whether or not the protocol under it is correct, which means a green run there is not evidence the protocol is right. Confirmed from the 2026-09-07 recorded run rather than accepted on the argument: the two flag arms, which are seeded dirty, each reached all four outcomes; the protocol arm reached only "dirty, not covered" and "clean, covered", never producing "covered and still dirty". That asymmetry is the signature of a window that only ever opens over a completion it has already seen, and it means the arms are not comparing like with like. Seeding the protocol arm dirty is not a fix. It makes the window open reliably, but the corner stays unreachable for the same release/acquire reason - any sample that observes the increment observes everything published before it - so seeding restates that the protocol is correct rather than showing the probe could detect it being wrong. A real power check would need a deliberately broken protocol variant as a negative control, the way PartitionStateCommitWindowSeamTest keeps a control arm asserting the old defect and stays runnable after the fix removed it. The maintainer ruled on #469 that the negative control would not be built, and that the weakness would be documented instead so nobody reads the arm's green as proof. That is the change here, and nothing about the probes themselves moves. Three places now carry it: - GenerationCountedCommitWindow's javadoc OWNS the statement - the caveat is firmed from "may be unreachable" to "is unreachable", it now says what the arm does show as well as what it cannot, and it records the ruling so a later reader does not mistake it for an item awaiting a fix. This is the primary home because it is what a reader has open when they run the arm. - The class-level Calibration status block, which quoted the zero with no caveat attached, now carries a one-line pointer naming the arm as the owner. - docs/inflight/test-jcstress-probe-module-open-items.md gets the line item, so it survives the working note being deleted at merge prep, plus the generalisation that outlives this one arm: a green FORBIDDEN tick is worth nothing unless some arm shows the outcome is reachable at all. Looked for other instances of the class in the module and found none. Every other FORBIDDEN arm earns its zero from a paired arm of the same state shape that fires - CommitPathVisibilityProbes and SeenSucceededOrderingProbes ("1, 0" against a plain arm that reaches it), BackPressureFlagVisibilityProbes ("false, false" against a plain and a reduced arm) - each differing from its comparator by a modifier and nothing else. CalibrationProbes' word-tearing outcome has no pair but asserts a platform guarantee rather than a fix. The protocol arm is the only one whose fixed version has a different state shape from its comparator, which is precisely why the paired-arm licence does not extend to it. Raised by a Codex review on #469. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
astubbs
marked this pull request as ready for review
September 8, 2026 02:51
…ine said it would be docs/inflight/pr-partition-state-cross-thread-flags.md was the reviewer-facing note for this branch while it was open, and its own first line committed it to deletion in the final commit before merge, per docs/inflight/AGENTS.md: a "delete this when it merges" marker must never reach master, so neither may the file carrying it. Everything in it that outlives the review already has a durable owner, checked section by section before deleting: the volatile-versus-protocol decision and the retired notes' reasoning are in docs/solutions/logic-errors/volatile-is-the-fix-for-a-one-writer-field-not-a-shared-one-2026-09-07.md; the load-bearing duplication rule is owned by the two probe classes' javadocs; the vacuous protocol arm and the ruling on it are owned by GenerationCountedCommitWindow's javadoc and carried as a line item in docs/inflight/test-jcstress-probe-module-open-items.md; the new vocabulary is in PartitionState's own javadoc; the seam's justification and the review that searched for a pre-existing hook are in the PR discussion and the fix commit's body; and the collision note names two open drafts that will resolve the overlap when they merge, which is not knowledge worth a file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
A SEMANTIC merge, not a textual one. Master gained #470, which changed PartitionState's commit protocol from the other side while this branch was changing it from this one, and git's single content conflict is the smaller half of the work. WHAT 470 CHANGED. Under PERIODIC_CONSUMER_ASYNCHRONOUS the partition used to be marked clean on the line after commitOffsets() returned - that is, at the moment of SENDING, because commitAsync answers later through an OffsetCommitCallback. 470 defers the clean mark to the acknowledgement, which makes two async commits able to be in flight at once, and handles that with one rule inside PartitionState: getCommitDataIfDirty remembers the offer it makes WHOLE, as the OffsetAndMetadata it handed out, and onOffsetCommitSuccess records every acknowledgement's offset through a monotonic recordCommittedOffset but calls setClean() only when the acknowledged object equals that offer. Whole rather than the offset alone, because completing a record above the lowest incomplete one changes the encoded metadata and moves the offset not at all. WHAT THIS BRANCH CHANGED. The dirty flag and stateChangedSinceCommitStart are gone, replaced by a monotone AtomicLong completionCount, a completionCountBeingCommitted sampled once at commit start, and a completionCountCommitted that setClean() publishes. Dirty is isDirtyAt(completionCount.get()) - a comparison of two values rather than a flag somebody clears. HOW THEY COMPOSE. The two rules answer different questions about the same acknowledgement, so they compose rather than overlap: 470's offer decides WHETHER an answer ends the story, and 469's count decides HOW MUCH of it it ends. getCommitDataIfDirty - the only conflicted hunk - now opens the commit window by writing BOTH halves of the pair. This branch's side is kept whole: the count is loaded ONCE into collectedAt, tested with isDirtyAt(collectedAt) and stashed as completionCountBeingCommitted, before the offsets are captured. Master's side is kept whole too: createOffsetAndMetadata()'s result is bound to offered, stored as offerLastMadeForCommit and returned, so the caller and the field hold the same object. Master's reset of stateChangedSinceCommitStart is dropped, because the field it clears no longer exists - the count sampled one statement earlier is what carries that meaning now. onOffsetCommitSuccess is master's, unchanged, with this branch's setClean() underneath it: recordCommittedOffset(committed) always, then setClean() only when committed.equals( offerLastMadeForCommit), else a DEBUG line saying the offset is recorded and the partition stays dirty. setClean() is the count-publishing version, so the value published is the sample taken by the very cycle whose offer was just answered - both fields are written by one invocation of getCommitDataIfDirty, so they can never be crossed. BOTH GUARANTEES SURVIVE, and each has a test that would notice if it did not: 469 - a completion landing inside the commit window leaves the partition dirty, including one landing inside setClean() itself, because setClean publishes a value decided at commit start rather than reading anything (PartitionStateCommitWindowSeamTest, with its control arm replicating the old two-flag protocol and asserting the loss). 470 - an acknowledgement of a superseded offer, including one carrying the SAME offset and different metadata, records its offset and does not clean the partition, and an out-of-order acknowledgement cannot walk the committed offset backwards (PartitionStateAcknowledgedCommitOffsetTest, ConsumerOffsetCommitterOverlappingAsyncCommitTest, MockConsumerAsyncCommitCallbackFailsTest / DroppedTest / SyncCommitMarksCleanOnReturnTest). SENTENCES REPAIRED, each because the merge made it false rather than merely dated: offerLastMadeForCommit's javadoc ended by naming two fields this branch deleted - "a plain field, for the reason {@link #dirty} states" and "the same lifecycle as {@link #stateChangedSinceCommitStart}". Both are dangling javadoc links as well as false claims. The fence is now completionCount's incrementAndGet, and the lifecycle partner is completionCountBeingCommitted; the paragraph now names the pair as it is, says what each half decides, and notes in one parenthesis that the flag it used to name was the count's predecessor. Master's thread-model paragraph and its #466 citation are kept verbatim. completionCountBeingCommitted's javadoc rested on a premise 470 falsified: "AbstractOffsetCommitter #retrieveOffsetsAndCommit runs collect, commit and success callback as one sequential unit". Since 470 the success callback is NOT in that unit under the async mode, so a later cycle can overwrite this field while an earlier request is outstanding. The javadoc now says so, and says why that is safe - setClean() fires only for the LATEST offer, and the count beside it was sampled by that same cycle. Its @ThreadConfined paragraph is restated to match master's model (poll thread under both consumer modes, control thread under transactional, Consumer#close() the one hand-over) and to point at offerLastMadeForCommit as the owner rather than restate it; the volatile and its reasoning stand. onOffsetCommitSuccess's javadoc claimed setClean() "declines when the partition's state changed again while the commit was in flight" - the old check-then-act, which is exactly what this branch removed. It now says setClean publishes the sampled count, so a completion inside the window is not covered by it. getCommitDataIfDirty's javadoc named only the count it remembers; it now names both halves of the window it opens, and says why one is sampled before the offsets are captured and the other after. docs/refactoring.md - both sides edited the AT_STALE_THREAD_WRITE_OF_PRIMITIVE section. Master added a ledger entry describing the commit-window pair as stateChangedSinceCommitStart plus offerLastMadeForCommit, to be @ThreadConfined-declared together; this branch deleted the first of those. Reconciled to one entry naming the pair that exists after both: offerLastMadeForCommit (#470) and completionCountBeingCommitted (#469), with the note that their modifiers already differ deliberately - the offer plain and not to be "fixed" with volatile, the count volatile because no single thread is correct to name across the commit modes. Master's work item, declare the pair together or not at all, is unchanged. docs/inflight/bug-torn-read-family.md - both sides edited it and the two edits are disjoint: this branch's rewording of the dirty sentence and master's replacement of the link to the note 470 retired. It reads true after both; no repair was needed. Two live records master added name setDirty, a method this branch renamed to recordCompletion: docs/inflight/bug-177-commit-response-timeout-unreproduced.md's argument that a stalled instance cannot reach the exception being hunted, and the same argument in CommitResponseTimeoutSoakIT's javadoc. Both are anchors in LIVE documents that must resolve against today's tree, so both are repaired to the current name with the old one in parentheses - the treatment this branch already gave bug-torn-read-family.md. The dated records under docs/plans/ and docs/solutions/ that describe the retired protocol are deliberately left alone: they say what was true when they were written. PartitionStateAcknowledgedCommitOffsetTest's completeAndOffer javadoc explained that getCommitDataIfDirty is what clears stateChangedSinceCommitStart, without which its clean-versus-dirty assertions would be vacuous. The vacuity argument still holds and the mechanism is now the sample of completionCountBeingCommitted; the sentence says that instead. TWO-DIRECTION AUDIT. git diff 7b4691b -- <the three co-edited files> shows master's additions plus the repairs above and nothing else - the lastCommittedOffset javadoc and package getter, offerLastMadeForCommit, the onOffsetCommitSuccess rewrite, recordCommittedOffset, and the offer stash. git diff origin/master -- the same files removes only this branch's own removals - the dirty flag, stateChangedSinceCommitStart, the old setClean/setDirty - plus the repaired sentences. Nothing 470 wrote is lost and nothing 469 wrote is reverted. EVIDENCE. PartitionState*Test, LastCommitTimeFenceTest, ConsumerOffsetCommitter*Test and MockConsumer*Test all green on the merged tree, with both PRs' pinning tests among them: PartitionStateCommitWindowSeamTest 3/3 including its control arm, PartitionStateCrossThreadFieldFenceTest 3/3, PartitionStateAcknowledgedCommitOffsetTest 5/5 including the same-offset-different-metadata case, ConsumerOffsetCommitterOverlappingAsyncCommitTest 3/3, and the three MockConsumer async/sync arms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…-cross-thread-flags
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Master landed the async-commit acknowledgement (#470, silent loss on the default commit mode) and the quarantine clearance (#473), which empties docs/quarantined-tests.md so the release guard no longer blocks on it and moves the capacity profiles behind a measured tag. With #466 and #468 already in, tier 1 has four of its seven merged; what is left is #469, #481, #480 and #477. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. Since the previous push: Updated for |
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… left #469 landed the two PartitionState cross-thread flags, measured then fenced or redesigned, so tier 1 is down to #480 (being reworked off the wire) and #477 (with a babysitter). #485, the control-arm proof of the deadlock fix, is on master too. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
One conflict, in PartitionState: #469 removed the plain stateChangedSinceCommitStart flag (replaced by the completion counters), and this branch had inserted its loaded-map fields directly beneath it, so the hunk carried the dead flag on this side. Resolved by keeping the new fields and dropping the flag master removed. Main and test sources compile, and the three test classes this branch owns pass on the merged tree. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
, #481, #482, #485 Two conflicts, and one silent auto-merge artefact that only the compiler found. PartitionState. #469 replaced the pair of booleans with a version counter, deleting stateChangedSinceCommitStart; #410's ledger snapshots at the same point. Joined by taking master's counter whole - the single completionCount load, tested and stashed as completionCountBeingCommitted - and moving the ledger's snapshotForCommit() inside that same branch, still ahead of createOffsetAndMetadata(), so the ledger is still snapshotted before the offer is made and trimmed only on the answer to that offer. The artefact: restoreCompletedButUncommittedWork() still called setDirty(), which #469 deleted, and git merged both sides clean because neither touched the other's lines. It is recordCompletion() now - the replay has moved the partition's state on, so it stamps a new version and isDirty() derives the rest. PartitionStateAbortedTransactionReplayTest 7/7 and PartitionStateCommitWindowSeamTest 3/3 cover the join. The flake ledger takes both sides' rows and master's retirement of the simpleBatchTest section, which #482 fixed - expectation-versus-input, not the library. config/infer-known-findings.txt takes master's version whole. The "1 NULLPTR_DEREFERENCE AbstractParallelEoSStreamProcessor.processWorkCompleteMailBox" line this branch carried was a merge artefact: #346 added it, #466 retired it in the same change that reshaped the mailbox loop, and an earlier merge here re-took #410's older side. The static lane was red on it. Re-ran infer locally on the merged tree - 9 findings, all known, none new. Verified: ProducerManagerTest 22/22, ProducerRecoveryTest 15/15, the PartitionState family, the RetryQueue family, WorkManagerTest, ArchitectureTest, TestConventionsArchTest, TransactionalClaimCoverageTest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…-completions-ledger Brings the review fix on #472 (the start-up producer build no longer runs as user code) and the master it merged, which redesigned the two places the ledger hooks into: - #469 replaced the dirty flag pair with a completion count. The ledger snapshot now sits beside the count stash in getCommitDataIfDirty, and the restore path stamps recordCompletion() where it set the dirty flag, so the lowered commit frontier is re-offered. - #470 marks a partition clean only on the acknowledgement of the last offer it made. The ledger trims on the same condition and never on an older answer, since by then the snapshot is the newer offer's. - #466 gave processWorkCompleteMailBox its own control-thread assertion and a wake-up action; the ledger's drain hardening keeps its try/catch around both, and its duplicate assertOnControlThread helper is dropped for master's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…producer-fencing-brainstorm Brings the ledger rung's merge of #472, and with it the master that rung merged: the review fix on #472, the commit-window redesign (#469, #470), the revoke commit handed to the control thread (#466), and the grooming sweep (#476). Resolved by hand: - maybeAcquireCommitLock: master's revoke-commit override and this branch's replacement guard compose. A periodic commit waits out a replacement; a revoke commit is answered regardless, since its request was taken for the pass, and while no producer exists it completes with nothing committed - consistent, because the aborted transaction published nothing and no offset was committed, so the next owner reprocesses. controlLoopPass says so at WARN when it happens. - The branch's own assertOnControlThread helper is dropped for master's, which has the same contract. - PCModule.replacementProducerWrap keeps #472's javadoc on why only the replacement build runs as user code, with this rung's sentence that ProducerRecovery builds through it. - TransactionalClaim: master's settled scope note on the revoke-path claim and this branch's C15 recovery claim both stand. - The flake ledger takes master's table, which retired the entries whose tests master fixed or quarantined since, and keeps this branch's one sighting master never had (TransactionTimeoutsTest's long arm). - Two notes the grooming sweep retired are accepted as deleted; the plan's citation of one now points at the history that holds it. - The stack note keeps this branch's heading and impact and takes master's vetting record, attested post-merge-correct. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
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.
Description
PartitionStatecarried three fields that cross the broker-poll / control thread boundary.#349 fenced one of them (
dirty) and deliberately left the other two alonefor want of a measurement. This PR measures them, and then gives them different fixes, because
the measurement says they are different defects.
The rule that falls out of it, and the thing worth carrying away:
volatileis the fix for a one-writer field, not for a shared one.1. The measurement (commit 1,
jcstress-poc/, no production code)Two probe pairs, in the shape
CommitPathVisibilityProbesalready used for thedirtypair.BackPressureFlagVisibilityProbes-allowedMoreRecords, written on the broker-poll threadinside the commit path's encode, read on the control thread in
couldBeTakenAsWork:ConcurrentSkipListMapaccessesvolatileThe map touch one statement earlier suppresses the window fiftyfold and does not close it - the same
incidental-fencing shape the
dirtypair measured at ~130x.CommitWindowLostUpdateProbes-stateChangedSinceCommitStart, written by both threads,with the arbiter reporting the invariant itself (a completion landing inside the commit window must
leave the partition dirty unless the commit covered it):
volatile dirty, plain flagvolatiletoo - the obvious next stepCalibration:
CalibrationProbes.PlainFieldStoreLoadReorderingfired at 50.82% in the same run, sothe zeros are interpretable rather than vacuous. macOS 26.5.2, Apple M2 Pro, 12 CPUs, Temurin
17.0.18+8,
-m quick. Rates are per raced pair, not production incidence.The arms share their scaffolding and keep only the measured term apart. Answering the
duplicate-code check, which flagged a 54-line and a 40-line clone inside the two new probe files.
The rule that decided what moves: a member may be hoisted if it contains no access to the arm's
measured field. So the
ConcurrentSkipListMapand its setup, the neighbouring plain writes, theinstrumentation payload, the
tryToEncodeOffsetsread and the arbiter's result mapping all movedinto a package-private base class the arms extend. What stays duplicated, deliberately and with the
reasoning in each class javadoc: the measured field's own declaration in its own
@Stateclass(plain versus
volatileon that line is the term under test), and each actor's literal sequenceof accesses (so the JIT compiles the shape production has).
ReducedPlainBackPressureFlagsharesnothing and does not extend the base at all - its whole point is the absence of the surrounding
accesses, and inheriting them would destroy the 51x comparison it exists to provide.
jcstress 0.16 permits this, read out of its annotation processor rather than assumed: a
@Stateclass must be public and non-final, and the
should not inherit other classesrule binds@Resultclasses only.
jcstress -llists all six arms afterwards, so none was silently dropped. Re-runningthe changed arms confirmed the outcome kinds are unchanged - both FORBIDDEN arms still show no
false, falserow at all, and all four plain/reduced arms still fire. The recorded rates above aredeliberately not overwritten with the re-run's:
-m quickis not the mode they came from, and arate is machine- and load-specific.
2. The two fixes (commit 3)
allowedMoreRecordsis nowvolatile. One writer, so a release/acquire edge is the whole fix.Its javadoc carries the figures, the arm name, and the direction of the crossing - poll thread writes,
control thread reads, which is the opposite direction to the commit-path fence, so that fence
never covered it. The harm is unbounded staleness: a stale
falseadmits no work, so nothingsucceeds, nothing re-dirties the partition, and the encode that would clear the flag never runs
again. That stall is not what the probes measure, and the class javadoc says so - they measure the
fence.
dirtyandstateChangedSinceCommitStartare gone. In their place: a monotoneAtomicLong completionCount, and thecompletionCountCommittedan acknowledged commit publishes - since #470 that is the broker's answer to the partition's latest offer, not the return ofcommitOffsets()."Is this partition dirty" becomes a comparison of two values rather than a flag somebody clears -
which removes the defect rather than narrowing it: no check-then-act is left in the clean-marking
step, and no write either thread can lose to the other.
Making the second boolean
volatilewas rejected on measurement, not on argument - the middlearm above moves the anomaly rate by nothing, because only one of the three mechanisms breaking the
invariant is a visibility effect. The other two (a check-then-act in the clean-marking step, a lost
update between two writers) fire at interleaving rates on sequentially consistent hardware, and no
modifier addresses either.
CommitWindowLostUpdateProbes.VolatileStateChangedFlagAcrossTheCommitWindowexists solely to holdthat line; re-run it before proposing the modifier again.
The release/acquire edge #349 measured is preserved, not discarded: an
incrementAndGetis a strictly stronger release than the volatile store it replaces, and the counterload on the commit path is the paired acquire. The plain
longs stay plain, for the reason that PRgave.
The names follow the design, after maintainer review. Once dirty is derived,
setDirty()is amisnomer - there is no flag to set, and the name invites the next reader to look for the matching
clear. It is now
recordCompletion(): the completing thread's only write to the commit protocol,stamping a new version on the partition. The count is a version stamp rather than a quantity - the
only use of the number anywhere is its inequality with what the last acknowledged commit covered.
isDirty()stays as the derived query and carries the definition the code was missing in threedifferent paraphrases: dirty means a completion has landed that no acknowledged commit has
covered.
That comparison now lives in exactly one place, a private
isDirtyAt(long sampledCompletionCount).getCommitDataIfDirty()duplicated it for a load-bearing reason rather than a lazy one, which is whythe answer is a named predicate and not a call to
isDirty(): the collecting cycle must stash thevery count it tested, because a completion landing between two loads would be published as covered
while its offset need not be in the metadata this cycle encodes - the burnt commit cycle this PR
exists to close.
isDirty()passes a fresh load;getCommitDataIfDirty()passes the sample it thenstashes, with a comment at the sample site saying why. Behaviour is unchanged: same two reads in the
same order, same field written, no assertion in any test moved.
The held
completionCountBeingCommittedisvolatilefor a reason worth reading: making it plainraised a new
AT_NONATOMIC_64BIT_PRIMITIVEon a line this change wrote, and checking the premiserather than dismissing it showed the analyser was right. Collecting is mutually exclusive, but since
#470 the acknowledgement is no longer part of that unit under the async mode - a
later cycle can overwrite the field while an earlier request is outstanding, which is safe only because
setClean()fires for the latest offer alone. And which thread runs the cycle varies with the commitmode (broker-poll under both consumer modes, control thread under transactional), so no named thread is
correct and
@ThreadConfinedwould have been a lie.3. The seam test, and the red proof
PartitionStateCommitWindowSeamTestdrives, deterministically, the one interleaving the old protocollost a completion to: inside the clean-marking step, between its read of the flag and its write of
dirty. Every other position in a commit window was already reachable through public calls and theold protocol handled all of them (
PartitionStateCommittedOffsetTest.workCompletedDuringAsyncCommitShouldKeepStateAsDirtyis the between-collect-and-success case, and it passed before this change).
Master offered no override point between those two instructions, so
PartitionState.onCommitWindowClosing()was added - an overridable no-op, the same one-shot-armedseam shape as
RacingSeamWorkManager, with firing tracked separately from arming so a test thatforgot to arm cannot pass while asserting nothing. This is new production code, and a reviewer who
can see a seam master already had should say so - the addition loses its justification.
Asked and answered. The review at
#469 (comment) searched for an
existing overridable hook on the commit path at that instruction - between the flag-read and the
dirtywrite - and found none, reporting no counter-evidence. The seam keeps its justification. Thequestion is recorded as settled in the working note rather than left standing as an open ask.
Redness is shown two ways:
defect stays runnable after the fix removed it; and
PartitionStateitselfwhile keeping the seam turns the pinning arm red:
Stashing the whole main-code change does not produce that - it removes the seam too and the test
stops compiling. Only the protocol may be reverted, which is exactly why the control arm is in the
tree rather than being a claim about a build nobody can re-run.
PartitionStateDirtyFlagFenceTestbecomesPartitionStateCrossThreadFieldFenceTest, pinningvolatileonallowedMoreRecords,final AtomicLongon the counter, andvolatileon the committedcount. Moving that guard rather than deleting it is what its own failure message asked for when the
fencing was redesigned.
4. A ledger that was wrong, and nothing went red
docs/refactoring.md'sAT_STALE_THREAD_WRITE_OF_PRIMITIVEentry listed two fields and a countwhile SpotBugs named six - three of them in
PartitionState, the class #349 was fencing atthe time, and one (
ProgressTracker.highestRoundCountSeen) that no ledger anywhere carried. Thelane runs
spotbugs:checkwithfailOnError=false, so the finding annotates and never blocks. Thesignal was present and the record was wrong about it, which reads as an all-clear.
The repair is not a better list - a list rots the same way - but the command that re-derives it. The
engine
AGENTS.mdledger gets the rule; the evidence and the story go todocs/solutions/logic-errors/volatile-is-the-fix-for-a-one-writer-field-not-a-shared-one-2026-09-07.md,which is where the two retired
docs/inflight/bug-*notes' durable content went.Prior art, as recorded in the commit bodies
PartitionState.dirtyafter measuring the same class ofwindow, and left these two fields explicitly unfenced for want of a measurement. Its modifier
tripwire's failure message asked for the guard to be moved, not deleted, if the fencing was ever
redesigned; this PR is that move.
jcstress-poc/module and theCommitPathVisibilityProbesshape both probe pairs here follow.
lastWorkRequestWasFulfilledandlastCommitTime, theone-writer
volatile+ modifier-tripwire pattern thatallowedMoreRecordsnow joins.docs/inflight/bug-*notes this PR retires carried a deliberately preserved supersededreasoning (the
throughput-vs-stallmisclassification caught in review on fix(core): the dirty flag crossed threads unfenced - one volatile, measured sufficient #349); thesolution write-up carries what outlived them.
docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.md- what aprobe's zero and its rate are each worth, cited from every arm.
docs/inflight/test-jcstress-probe-module-open-items.md- already records that nothing binds thesearms to the real code (the module imports no
bz.stub.parallelconsumerclass); still open.Collisions, noted rather than dodged
#470 merged first and its overlap is resolved here: master is merged in, and
getCommitDataIfDirtynow opens the commit window with both rules - this PR's sampled completion countand that PR's whole remembered offer - while
onOffsetCommitSuccessrecords every acknowledgement andpublishes the sampled count as committed only on the answer to the latest offer. Open drafts
#410 and #460 also touch
PartitionState; neither isreshaped around.
Verification
bin/ci-unit-test.sh, all 11 modules SUCCESS.bin/check-all.sh: 16 passed, 0 failed (3 nothing-in-scope, 5 PR-scoped skips).docs/inflight/test-untracked-ci-flakes.md, each with its same-base control arm and the reasonneither reads as a regression of this change:
ReactorPCTest.concurrencyTest(the test hugs itsceiling in every run on both trees, green ones included) and
CommitResponseTimeoutSymptomTest.aRebalanceStormUnderAHighFailureRateNeitherStallsNorKillsTheConsumer(its await counts commit attempts, which stop accruing once the backlog drains).
Checklist
AGENTS.mdledger,docs/refactoring.md, threedocs/inflight/notes, and a newdocs/solutions/write-updocs/features/-N/A - an internal concurrency fix; no user-facing feature or API changePartitionStateCommitWindowSeamTest(new, with a control arm),PartitionStateCrossThreadFieldFenceTest(renamed and extended), plus thejcstress-poc/probe armsdocs/inflight/working note (pr-/branch-) started at the PR's first commit -pr-partition-state-cross-thread-flags.md, opened at the second commit on the branch, carried the reviewer-facing context while the PR was open, and is deleted in the final commit at merge prep as its first line said; everything that outlives it has a durable owner, named in that commit's bodyce-simplifyandce-code-reviewlocally -N/A - neither was run; the review is being spent on the PR instead, which is the route that can open inline threads. Say so plainly rather than tick it🤖 Generated with Claude Code
https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK