Repository navigation
fix(core): the dirty flag crossed threads unfenced - one volatile, measured sufficient - #349
Merged
Merged
Conversation
…oss threads unfenced PartitionState.dirty is written on the control thread when a record completes and read on the broker-poll thread by the commit path's dirty-partition collection, in the default PERIODIC_CONSUMER_ASYNCHRONOUS mode, with no happens-before edge of any kind - not this field, not the offsets it publishes. Measured, not argued. jcstress on this pattern (probe module on the test/jcstress-poc-plain-long-visibility branch, write-up at docs/plans/2026-08-25-002-test-jcstress-poc-plain-long-visibility.md): as plain fields, the reader observed dirty set while offsetHighestSucceeded was still stale - at 1.9e-5 per sample on the reduced pattern, and still 1.4e-7 with the REAL surrounding ConcurrentSkipListMap accesses modelled on both sides, so the neighbouring concurrent code narrows the window two orders of magnitude and does not close it. The consequence errs safe: a burnt commit cycle, which on a partition that then goes idle holds the committed offset back until the next rebalance. Redelivery, not loss - but a stall-shaped symptom this library has enough of already. With only this flag volatile the anomalous outcome was 0 in 4.29e9 samples and declared FORBIDDEN: the release store publishes the preceding plain writes and the acquire load observes them. The longs stay plain deliberately - fencing them too buys nothing the flag does not provide, at a cost on every read. One release store per successful record is cheaper than the map remove already on that line, so no benchmark gate; the standing perf lane confirms after merge. The test pins the modifier by reflection, which is the honest shape here: the behaviour is a 1.4e-7 hardware effect no JUnit run can observe, and a modifier is exactly what an unrelated refactor drops silently. The behavioural evidence stays in the jcstress module where it can be re-run. Refs: confluentinc#200 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
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)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🧪🔒 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 |
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…ame was about to lie Antony's review on #348 said the note "needs extraction or deletion". Taking that as: extract the still-open items into a note named for the AREA, and delete what the PR discharged. docs/inflight/AGENTS.md is explicit that "A filename must never carry state", and `pr-` means "Context about an open PR that gh cannot tell you" - so pr-348-review-findings.md starts lying the second the PR merges, and every citation to it would then have to be repaired rather than left alone. The directory has already paid this: `parked-` and `next-` were removed for exactly this reason, all 23 `next-` files being `open`. The three sibling PRs hit the same wall and renamed the same way - #341 to ci-bsd-portability-gaps.md, #338 to ci-copyright-gate-review-leftovers.md, #347 to test-lincheck-lane-open-items.md. test-, because the note is about test infrastructure: a probe harness, its correspondence to product code, and what no check verifies about either. Re-checked every item rather than moving them wholesale. What changed: - The duplication-lane item is discharged, so its measurement is dropped (similarity peaked at 59.7% against the 80% fail line; both caps hold). What survives is the part nothing else records: the near-duplicate arms ARE the measurement - the ~130x suppression between reduced and faithful is the finding - so they must never be collapsed to quiet the scanner. The no-allowlist hazard stays owned by ci-dup-similarity-cannot-accept-known-duplication.md rather than restated. - "A vacuous run is indistinguishable from a clean one" is now "Nothing enforces reading the positive control" - jcstress-poc/pom.xml carries the warning durably as READ THE CALIBRATION BEFORE BELIEVING ANY ZERO, so the open part is only that it is prose no check reads. - The drift-check item and the "how does this grow" item were the same item from two sides - the proposed check is the mitigation for the growth problem - so they are one section. - Every pre-merge tense is gone: "on this branch", "did not close", "delete it when #348 lands" now read as they will after the merge, which is what bin/check-branch-self-reference.sh exists to force. The post-merge: checked block is narrowed to the header that names the PR. - Verified still open, not assumed: the plan doc cites neither docs/refactoring.md nor core-control-thread-contract-debts.md; no durable doc points at the module; #349 is open, so the priority tension stands. The other four inflight notes this PR touches stay separate files - one item per file is the directory's whole reason for existing.
…ses-threads-unfenced
…had to survive it Three things the merge of master falsified or exposed, fixed where they broke rather than left for whoever hits them next. THE JAVADOC CITED A BRANCH. `PartitionState.dirty`'s new javadoc pointed at "the jcstress-poc module on the test/jcstress-poc-plain-long-visibility branch" - true when written, false since #348 landed the module on master at `jcstress-poc/`. A branch name is not a link, says nothing about whether the work landed, and nobody comes back to upgrade it. It now names the module path, the PR, and `CommitPathVisibilityProbes` - the probe class modelling this exact flag/payload pair - so a reader can re-run the measurement instead of taking the numbers on trust. This also answers, for this one field, the "no durable pointer to the module exists" finding in `docs/inflight/test-jcstress-probe-module-open-items.md`: main code now points at it. THE INFLIGHT NOTE DESCRIBED THIS PR IN THE PRESENT TENSE. `bin/check-branch-self-reference.sh` failed on it after the merge - the note landed on master after this branch's head, so the gate had nothing to catch until now. Its "Priority tension" bullet said the plan defers the fix "while #349 implements it", a sentence this merge makes wrong. Rewritten to read as it will after landing, and narrowed while there: #349 settles ONE field, so the tension and the `core-control-thread-contract-debts.md` piecemeal caution both survive for the three sibling plain fields `docs/refactoring.md` still defers. THE CORRESPONDENCE DRIFT THAT NOTE PREDICTED HAS HAPPENED, and this is its first instance. `CommitPathVisibilityProbes` hand-copies `PartitionState`'s fields and imports nothing from it, so fencing `dirty` leaves `PlainDirtyPublishesSucceeded` annotated "Commit path as shipped" describing a shipped path that is now fenced, with the volatile control arm silently promoted to being the real one. Nothing went red - exactly the failure mode that section exists to name. Recorded rather than corrected: relabelling arms is a change to the instrument and belongs with whoever builds the correspondence check, not bolted onto a one-field fix. Both rewritten self-references carry `post-merge: checked` markers, which assert a human looked - those two claims are Antony's to confirm. The dated plan doc is deliberately NOT rewritten; `docs/citations.md` owns that, and the reconciliation belongs in the live note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
…ses-threads-unfenced
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The draft response listed the four sibling fixes as `(#344, #345, #346, #349)` - qualified once and then elided. `bin/check-issue-refs.sh` reads each reference on its own, so the three elided ones are bare numbers below the threshold and it failed the branch. Writing all four out in full is also what the drafts need on their own terms: they are posted to GitHub, where the short `astubbs#NN` form does not autolink either.
…nd nothing tracks it #349 fences `PartitionState.dirty` because jcstress measured that one pair. Walking the same class for the merge-prep "other instances of the same defect" sweep found a second cross-thread pair the fix does not cover, in the opposite direction: `allowedMoreRecords`, the offset-encoding back-pressure flag. It is written on the BROKER-POLL thread - `setAllowedMoreRecords` is reachable only from `tryToEncodeOffsets`/`updateBlockFromEncodingResult`, and those only from `getCommitDataIfDirty`, which in both consumer commit modes runs on the thread that claimed the committer - and read on the CONTROL thread in `couldBeTakenAsWork`, off `ProcessingShard.getWorkIfAvailable`. THE `dirty` VOLATILE DOES NOT COVER IT, and the reason is worth stating rather than assuming: `dirty`'s release store is on the control thread and its acquire load is on the poll thread, so the fence runs control-to-poll and this pair runs poll-to-control. The poll thread does release-store `dirty` after the write, via `setClean` on commit success, but the READER never performs the paired load - `couldBeTakenAsWork` does not touch `dirty`, and the only control-thread load of it, `wm.isDirty()` in `maybeAcquireCommitLock`, sits behind `isTimeToCommitNow()` in an `&&` and so does not run on non-commit-time iterations. Even when it does, an edge exists only if that read observes the poll thread's write rather than the control thread's own `setDirty(true)`. Incidental, not guaranteed. A SCANNER DOES SEE IT AND THE LEDGER DOES NOT, which inverts the expected finding. fb-contrib's `AT_STALE_THREAD_WRITE_OF_PRIMITIVE` names this field today, plus `stateChangedSinceCommitStart` and `bootstrapPhase` in the same class; the offender list `docs/refactoring.md` owns under that rule names three fields in other classes and none of these. The lane runs with `-Dspotbugs.failOnError=false`, so it annotates and never blocks. The signal was present and the record was wrong about it, which is worse than the analyser having been silent. NOT FIXED HERE, deliberately. #349 is one field with one measurement behind it; this pair has no measurement and a different reader path, so making it volatile on the same commit would ship an unmeasured edit under a measured one's justification. The note says what would settle it - a probe pair in `jcstress-poc/` in the shape `CommitPathVisibilityProbes` already uses - and points at `core-control-thread-contract-debts.md` and the shared-nothing caution (confluentinc#200, mirror #142) before anyone fixes these piecemeal. `stateChangedSinceCommitStart` is recorded as the more interesting of the two siblings and is NOT diagnosed: it is written from both threads, so it is a lost-update risk as well as a visibility one and `volatile` alone is not obviously the fix.
…hen this fix landed Fencing dirty falsified the probe module's own report prose. PlainDirtyPublishesSucceeded was annotated "Commit path as shipped" while the shipped path is now volatile, and VolatileDirtyPublishesPlainSucceeded still called itself the control arm when it had silently become the real one. Anyone reading a jcstress report would have taken the anomaly-producing arm for current code. This is the correspondence drift docs/inflight/test-jcstress-probe-module-open-items.md predicted, arriving on its first real occasion. Nothing went red, because nothing can: the module hand-copies PartitionState and imports nothing from it. ONLY REPORT PROSE CHANGED, and that was verified rather than assumed before touching it. org.openjdk.jcstress.annotations.Description declares a single String value() targeted at TYPE - it carries no Expect and no outcome semantics, so it cannot move a verdict. Every @outcome, every expect, every @State, @Actor and class name is untouched; the diff is five annotation strings and two javadoc passages. jcstress-poc compiles clean. THE CLASS NAMES ARE DELIBERATELY NOT RENAMED. They describe the memory model each arm uses - PlainDirtyPublishesSucceeded really does use a plain flag - which is what has not changed and what makes them stable citations. The commit body of the fix, the plan doc and the inflight note all name them. Renaming would be the change to the instrument that the note warned against; rewording its report text is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
Owner
Author
|
@codex review |
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 3m 19s —— View job Review
Tooling note: the checkout here is shallow and The fix itself
That ordering is exactly what makes I also checked the other cross-thread read the inflight note calls out — Test
Scope discipline — verified, not just taken on faithThe PR explicitly does not fix
This is a well-scoped, well-evidenced fix. I have no correctness findings to raise. Minor, non-blocking observations
No changes requested. |
![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: b42cc8866d
ℹ️ 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".
Five review findings from `@codex` on #349. All five were accepted; two were worse than reported. **The stale-`false` path is a stall, not a throughput cost.** The note argued `throughput` because the blocked direction "self-clears whenever the reader observes a later commit's write". Put against the code, that later write is not guaranteed to happen. Every write to `allowedMoreRecords` is inside `tryToEncodeOffsets`, which `getCommitDataIfDirty` reaches only when the partition is dirty. When the writer sets `true` because `incompleteOffsets` emptied, that same commit marks the partition clean; a control thread holding the stale `false` then admits nothing, because `isBlockingProgress` passes only offsets below `offsetHighestSucceeded` and an empty incomplete set puts every new record above it. No record is taken, none succeeds, nothing re-dirties the partition, and the encode never runs again. The partition stops and stays stopped until the next rebalance. Impact tag corrected to `stall`; the superseded reasoning is kept beside it, because it is the plausible-sounding version and the next reader will re-derive it otherwise. **The count was not merely stale-prone, it was false.** The note claimed "the three sibling plain fields `docs/refactoring.md` names under the same defect class". That entry names `AbstractParallelEoSStreamProcessor.lastWorkRequest- WasFulfilled`, `ConsumerManager.commitRequested` and `RetryQueue.closed` - no `PartitionState` field at all, which is the very gap the sibling note was written to record. Replaced with the reproduce command and a pointer to the owning entry, per docs/inflight/AGENTS.md's rule against writing down what a command can answer. The same wrong claim sat one paragraph up, pre-existing; fixed there too rather than leaving one instance of a defect this change removed the other half of. **The note contradicted its own diff.** It said relabelling the probe arms was "deliberately not corrected" here - while the previous commit corrected them. Rewritten to record that the instance closed and the hole did not: prose is the recoverable case, and a human reading the diff is what caught it, not the build. **A finding was filed where its own tracker would delete it.** `stateChangedSinceCommitStart` is independently actionable - written from both threads, so a lost update, so `volatile` alone is not obviously its fix - but it lived inside a note named for `allowedMoreRecords`, which is `git rm`'d when that field is fenced. Split into its own note. `bootstrapPhase` was an unwalked lead rather than a diagnosis, so it is left to the reproduce command instead of becoming a third file. **Production javadoc cited a dated plan.** `docs/plans/` is explicitly not for durable reference, and this note already records that the module's only pointer is a plan doc. The field javadoc and the fence test now cite `docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.md`, which carries these exact figures in its results table, plus the greppable `CommitPathVisibilityProbes.VolatileDirtyPublishesPlainSucceeded`. No behaviour change: the `volatile` modifier, every `@Outcome`/`expect`/`@State` /`@Actor` and every probe class name are untouched. Verified locally: `bin/check-all.sh` 15/15, `bin/ci-unit-test.sh -pl :parallel-consumer-core -am` 412 tests / 0 failures / 8 pre-existing skips, `jcstress-poc` compiles clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3UK42akdtYuRgd7jSdcVk
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
One conflict, in docs/inflight/static-infer-findings.md, on the paragraph describing what RacerD cannot see. Both sides rewrote the same four sentences for the same reason - the branch-self-reference gate fails a present-tense claim about an open PR - and reached different wordings. Kept this branch's, because it is the one that stays true as the family lands, and two of the four have now landed (#337 and #349 are on master). Master's wording says RacerD "misses the four named torn-read races" in the present tense, which reads as a claim about four open defects; this branch's says none of the four appears in its output and that the blind spot is the CLASS rather than the instances, so which of them are still unfixed does not move what the analyser can see. Naming them by their fixing PRs keeps the citations valid after each merges. No content from master's side is lost: its only change was re-tensing the same sentences and moving a post-merge marker, and the marker pair is balanced in the result.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
Master now carries #349 (dirty made volatile) and #337, so PartitionState.java was touched on both sides. Git auto-merged it and both changes survive intact: `private volatile boolean dirty` from #349 alongside this branch's caller-resolved onSuccess/onFailure. Verified by reading the merged file, not inferred from a clean exit. One real conflict, in docs/inflight/static-infer-findings.md, on the paragraph describing what RacerD cannot see. Both sides had rewritten the same four sentences to satisfy the branch-self-reference gate and reached different wordings. Kept this branch's, which is the wording that stays true as the torn-read family lands - two of the four are now on master - because it says the blind spot is the CLASS rather than the instances, so which of them remain unfixed does not change what the analyser can see. Resolved identically on #345, so the two branches still agree on this file.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…than the ledger counts, and a testable lead on the last unexplained line Reading the merged commit bodies rather than the family's own summary, #119's `## Fork status` is badly out of date. It says three defects landed and one remains. At least six more have landed since, none of them in the family's accounting: - **#346** orphaned a retry-queue entry when a rebalance landed between a staleness checkpoint and its acting read, after which `workIsWaitingToBeProcessed()` read TRUE FOREVER. Its own body calls that a confluentinc#857-family permanent stall. Red-then-green on both arms, mutation-tested. - **#345** NPEd the broker-poll thread inside the rebalance listener via a `containsKey`-then-`get` race. Poller death under KEY ordering is paused consumption by another name. - **#373** found a sign-reversed overcount its body names the "confluentinc#857 stall signature". - **#336** rewrote the load gate whose drift kept intake paused. - **#344** let the offset encoder mark still-incomplete offsets complete. - **#349** fenced a cross-thread `dirty` flag, jcstress-measured. The bucket has been drained steadily from several directions and nobody re-totalled. That is the substantive reason #119 cannot be closed by reading this PR alone, and why its fork-status section needs rewriting rather than appending to. **A prediction is recorded for the one line still unexplained.** The `ChaosChurnStormIT` `NO_PROGRESS` sightings are all `PERIODIC_CONSUMER_ASYNCHRONOUS`, the mode where neither the AB-BA cycle nor the transactional wait can occur - which is why they were set aside as a possible fourth defect. #344's defect is live in that mode and only that mode, and its consequence - offsets marked complete while incomplete - has the `NO_PROGRESS` shape. So: replay the three async seeds either side of #344; if the mode match is the mechanism the pre arm reproduces and the post arm does not. Stated as a prediction rather than a finding, with its own counter-example beside it: #373 was already tested against this arm on a mode-matching hypothesis and fired anyway on that branch's own head. One plausible fix has failed here before.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…d gone out of date in eight ways The ledger is where agents log a sighting and move on, and where the next one reads before forming a hypothesis - so conclusions reached elsewhere are worth nothing until they land here. A week of findings had accumulated in a PR note that gets DELETED when that PR merges. Migrated. What the ledger now says that it did not: - Its own "three landed, one open" summary is stale. At least six more family defects have landed - #346, #345, #373, #336, #344, #349 - and none was counted. The bucket has been drained from several directions and nobody re-totalled. - The original deadlock is VERIFIED rather than diagnosed: an A/B on the deterministic probe, one term changed, control failing every repetition and the fix failing none while logging the contended decline throughout. - Replaying captured seeds does not reproduce that deadlock and should not be attempted again - the window opened zero times locally on either assignor arm, and two write-ups were retracted the same day for reading a green replay as evidence. - The async line has its first replayable seed, found by random hunting rather than replay, and #344 is refuted as its explanation. - A third of those failures went red with the NO_PROGRESS detector silent, which matters because the Class 2 demotion left the liveness claim resting on it. - The symptom is a bucket of at least four distinguishable behaviours, separated by upstream's own maintainer in one sentence, plus a fifth reported on #29 and never chased - a pause with no rebalance in the logs at all. - Upstream's own fix for this issue shipped, is carried here, and did not hold. - confluentinc#875 is filed in this family on a third party's assumption and looks like an offset-completion defect instead. Also records why a single beefier Docker host beats a distributed rig for the soak harness beyond sequencing: central log shipping comes free, and any node can be inspected AT RUNTIME while still wedged. Every sighting in this family that became a mechanism did so because something captured the process while it was stuck; every one that stayed a signature did not.
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
… 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
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…, then fenced or redesigned (#469) PartitionState carried three fields that cross the broker-poll / control thread boundary. #349 fenced one of them, dirty, after measuring its window, and deliberately left the other two alone for want of a measurement. This measures them with jcstress and then gives them DIFFERENT fixes, because the measurement says they are different defects. The rule that falls out of it: volatile is the fix for a one-writer field, not for a shared one. THE MEASUREMENT, in jcstress-poc/, in the shape CommitPathVisibilityProbes already used for the dirty pair. Rates are per raced pair on one workstation, not production incidence, and the recorded figures are in each class's Calibration status block. BackPressureFlagVisibilityProbes measures allowedMoreRecords, written on the broker-poll thread inside the commit path's encode and read on the control thread in couldBeTakenAsWork. The plain field with its real surrounding ConcurrentSkipListMap accesses fires; with the neighbours stripped it fires about fifty times more often - so the map touch one statement earlier suppresses the window fiftyfold and does not close it, the same incidental-fencing shape the dirty pair measured; the volatile arm is FORBIDDEN at zero. What the arms do not show, said in the javadoc: this field's harm is unbounded staleness - a stale false admits nothing, so nothing succeeds, nothing re-dirties the partition and the encode never runs again - and "never" is not an outcome a bounded run observes. They measure the fence, not the stall. CommitWindowLostUpdateProbes measures stateChangedSinceCommitStart, written by BOTH threads, with the arbiter asserting the invariant itself: a completion landing inside the commit window must leave the partition dirty unless the commit covered it. As shipped it fires at around 1.6e-3. The arm that decides the design is the flag made volatile as well - the obvious next step, and the one the inflight note had recorded as next: statistically unmoved, because only one of the three mechanisms breaking the invariant is a visibility effect. The check-then-act in the clean-marking step and the lost update between two writers fire at interleaving rates on sequentially consistent hardware, and no modifier addresses either. VolatileStateChangedFlagAcrossTheCommitWindow exists solely to hold that line; re-run it before proposing the modifier again. The monotone-count protocol arm is FORBIDDEN at zero, with a known weakness stated below. THE TWO FIXES. allowedMoreRecords is volatile: 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, the OPPOSITE direction to the commit-path fence, so that fence never covered it. dirty and stateChangedSinceCommitStart are gone. In their place a monotone AtomicLong completionCount, the completionCountCommitted a successful commit publishes, and the held completionCountBeingCommitted. "Is this partition dirty" is 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. The release/acquire edge #349 measured is preserved: 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 longs it published stay plain for the reason that PR gave. The held count is volatile because which thread runs the commit cycle varies by mode, so a thread-confinement claim would have been a lie. COMPOSED WITH #470, which merged first and changed the same protocol from the other side: the partition remembers the commit offer it makes WHOLE and marks itself clean only on the broker's acknowledgement of that offer. The two rules do not overlap. getCommitDataIfDirty samples the completion count once, stashes it, and remembers the offer; onOffsetCommitSuccess records every acknowledged offset and publishes the stashed count as committed only when the acknowledged offer equals the last one made. The offer decides WHETHER an answer ends the commit window; the count decides HOW MUCH it covered. Under the async mode the acknowledgement is no longer part of the collect-and-commit unit, so a later cycle can overwrite both fields while an earlier request is outstanding, which is safe because only the latest offer's answer publishes anything. THE NAMES FOLLOW THE DESIGN. setDirty() is now recordCompletion(): nothing sets a flag any more, the completing thread stamps a new version on the partition, and the count is a version stamp rather than a quantity - its only use is inequality with what the last acknowledged commit covered. isDirty() carries the definition the code had been paraphrasing three ways: dirty means a completion has landed that no acknowledged commit has covered. The comparison lives once, in isDirtyAt(long): isDirty() passes a fresh load, and getCommitDataIfDirty() passes the sample it then stashes, because the collecting cycle must publish the very count it TESTED - a completion landing between two loads would be published as covered while its offset need not be in the metadata this cycle encodes. That burnt commit cycle is the defect this change closes. THE RED PROOF. PartitionStateCommitWindowSeamTest drives, deterministically, 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 in the one-shot-armed seam shape of RacingSeamWorkManager, with firing tracked separately from arming so a test that forgot to arm cannot pass while asserting nothing. The review searched for an existing hook at that instruction and found none. 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, reverting the protocol in PartitionState while keeping the seam turns the pinning arm red. Stashing the whole change does not produce that, because it removes the seam too. PartitionStateDirtyFlagFenceTest becomes PartitionStateCrossThreadFieldFenceTest, pinning the modifiers on all three fields, which is the move its own failure message asked for. KNOWN WEAKNESS, RULED ON RATHER THAN FIXED. The protocol arm's FORBIDDEN zero is unreachable by construction: the flag arms are seeded dirty so the poll actor always enters the commit window, while the protocol arm starts clean and enters the window only once it has already observed the completion, and the AtomicLong acquire that lets it in publishes everything the release wrote. The recorded run corroborates it - the flag arms reached all four outcomes, the protocol arm only two. Seeding it dirty is not a fix, for the same release/acquire reason; a real power check would need a deliberately broken protocol variant as a negative control. The maintainer ruled that the control would not be built and the weakness documented instead, so its zero is cited as "no anomaly observed" and never as "the window was measured shut". The argument this change rests on is the plain-versus-volatile pair, which none of this affects. Every other FORBIDDEN arm in the module earns its zero from a paired arm of the same state shape that fires; this is the only arm whose fixed version has a different state shape from its comparator. THE PROBE ARMS SHARE THEIR SCAFFOLDING and keep only the measured term apart, answering the duplicate-code check. The rule: a member may be hoisted only if it contains no access to the arm's measured field. The map and its setup, the neighbouring writes, the instrumentation payload, the encode read and the arbiter's result mapping moved into a package-private base class; the measured field's own declaration per @State class and each actor's literal access sequence stay in the arm, so plain versus volatile on one line is the term under test and the JIT compiles the shape production has. jcstress 0.16 permits a @State class to inherit - read out of its annotation processor, whose inheritance ban binds @Result classes only - and jcstress -l lists every arm after the change. The recorded rates are deliberately not overwritten with the re-run's. A LEDGER THAT WAS WRONG, and nothing went red: docs/refactoring.md's AT_STALE_THREAD_WRITE_OF_PRIMITIVE entry listed two fields while SpotBugs named six, three of them in PartitionState - the class #349 was fencing at the time - and one, ProgressTracker.highestRoundCountSeen, that no ledger carried. The lane runs spotbugs:check with failOnError=false, so the finding annotates and never blocks. The repair is the command that re-derives the membership, the spotbugs step's own line verbatim, not a better list. DOCS. The two docs/inflight/bug-* notes for these fields are retired, their durable content in docs/solutions/logic-errors/volatile-is-the-fix-for-a-one-writer-field-not-a-shared-one-2026-09-07.md, and the engine AGENTS.md ledger carries the rule. docs/inflight/test-jcstress-probe-module-open-items.md carries the vacuous-arm line item and the generalisation that a green FORBIDDEN tick is worth nothing unless some arm shows the outcome is reachable at all. Two local-only test failures seen while verifying are recorded in docs/inflight/test-untracked-ci-flakes.md with their same-base control arms; the row for ReactorPCTest.concurrencyTest no longer rules this change out as a contributor, because setAllowedMoreRecords(true) fires on two paths and the fence publishes unblocking sooner too. The master merges kept the retired notes deleted over the vetting sweep's stamps, which asserted the fields were still unfenced and unmeasured - precisely what this change falsifies. Co-authored-by: Claude Fable 5.1 (1M context) <noreply@anthropic.com>
This was referenced Sep 17, 2026
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.
Makes
PartitionState.dirtyvolatile - the one fix the jcstress evidence (#348) measured as sufficient, shipped separately so it can be reviewed against those tables.Why
The flag is written on the control thread when a record completes and read on the broker-poll thread by the commit path's dirty-partition collection, in the default
PERIODIC_CONSUMER_ASYNCHRONOUSmode, with no happens-before edge. As a plain field, jcstress observed the reader seeingdirtyset whileoffsetHighestSucceededwas still stale - at 1.4e-7 per sample even with the real surroundingConcurrentSkipListMapaccesses modelled on both sides, so the neighbouring concurrent code narrows the window two orders of magnitude and does not close it.Consequence errs safe - a burnt commit cycle; on a partition that then goes idle, the committed offset waits for the next rebalance. Redelivery, not loss - but a stall-shaped symptom this library has enough of already.
Why exactly this and nothing more
Measured, not argued: with only the flag volatile, the anomalous outcome was 0 in 4.29e9 samples, declared FORBIDDEN - the release store publishes the preceding plain writes, the acquire load observes them. The
longs stay plain deliberately; fencing them too buys nothing the flag does not provide, at a cost on every read. One release store per successful record is cheaper than the mapremovealready on that line, so no benchmark gate; the standing perf lane confirms post-merge.The test, and why it pins the modifier
PartitionStateDirtyFlagFenceTestasserts the field isvolatileby reflection. That is the honest shape: the behaviour is a 1.4e-7 hardware effect no JUnit run can observe, and a modifier is exactly what an unrelated refactor or Lombok change drops silently - nothing goes red, the fix is gone. The behavioural evidence stays in the re-runnable jcstress module (#348).Full core suite green on the merged tree: 412 tests, 0 failures, 8 pre-existing skips.
Other instances of the same defect
The sweep enumerated every non-final field in
parallel-consumer-core/src/mainand turned up one live instance covered by no ledger, filed here rather than fixed:PartitionState.allowedMoreRecords- the offset-encoding back-pressure flag - is written on the broker-poll thread inside the commit path's encode (tryToEncodeOffsets) and read on the control thread incouldBeTakenAsWork. This PR's volatile does not cover it: thedirtyfence runs control→poll and that pair runs poll→control, with the reader performing no acquire load ofdirtyon its path. Filed asdocs/inflight/bug-allowed-more-records-crosses-threads-unfenced.md.false"self-clears whenever the reader observes a later commit's write". Put against the code, there is no later write at all: every write lives insidetryToEncodeOffsets, whichgetCommitDataIfDirtyreaches only when the partition is dirty. When the writer setstruebecauseincompleteOffsetsemptied, that same commit marks the partition clean; a control thread holding a stalefalseadmits nothing (isBlockingProgresspasses only offsets belowoffsetHighestSucceeded, and an empty incomplete set puts every new record above it), so nothing succeeds, nothing re-dirties, and the encode never runs again. Re-taggedstall, with the superseded reasoning kept beside it rather than deleted.AT_STALE_THREAD_WRITE_OF_PRIMITIVE, along withstateChangedSinceCommitStartandbootstrapPhase- while the offender list indocs/refactoring.mdnames none of the three. The lane runsfailOnError=false, so it annotates and never blocks.stateChangedSinceCommitStartis written from both threads, so it carries a lost-update risk as well as a visibility one andvolatilealone is not obviously its fix. Recorded, not diagnosed - and in its own note,docs/inflight/bug-state-changed-since-commit-start-written-from-both-threads.md, rather than inside theallowedMoreRecordsone: that note getsgit rm'd when that field is fenced, which would take this finding with it.bootstrapPhase- an unwalked analyser row is not a diagnosis, and filing one would imply work that nobody has scoped.Not fenced here on purpose: this PR is one field with one measurement behind it, and that pair has neither.
Also from the original sweep:
offsetHighestSeenhas no cross-thread main-code reader except a gauge, and thelongs are deliberately left plain with the reasoning recorded.Checklist
docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.mdplus the greppableCommitPathVisibilityProbes.VolatileDirtyPublishesPlainSucceeded, rather than a dated plan;docs/inflight/test-jcstress-probe-module-open-items.mdupdated; two new notes -docs/inflight/bug-allowed-more-records-crosses-threads-unfenced.mdanddocs/inflight/bug-state-changed-since-commit-start-written-from-both-threads.md.docs/features/- N/A: an internal memory-visibility fix with no user-facing surface, option or behaviour change.PartitionStateDirtyFlagFenceTest, the modifier tripwire above.b3f018048was merged.ce-simplifyandce-code-reviewlocally - N/A - asked for@codex reviewon the PR instead. Neither skill has been run on this branch. The diff is one field modifier, one reflection test and documentation, so the local pass is the expensive option for what it would find here.Related: confluentinc#200 (the upstream note about this field family), mirrored as #142. Deliberately no closing keyword: this fences one field of a rework spanning the whole poll/control thread model, and closing that mirror would take the rest with it.
Merge-prep additions
Master
b3f018048merged clean.The probe module's own report prose said the wrong thing the moment this landed.
CommitPathVisibilityProbeshand-copiesPartitionStateand imports nothing from it, soPlainDirtyPublishesSucceededwas annotated "Commit path as shipped" while the shipped path is now fenced, and the arm labelled "control" had silently become the real one. Reworded here - verified first thatorg.openjdk.jcstress.annotations.Descriptiondeclares a singleString value()targeted atTYPE, carrying noExpectand no outcome semantics, so it cannot move a verdict. Every@Outcome,expect,@State,@Actorand class name is untouched, andjcstress-poccompiles clean. Class names deliberately kept: they describe the memory model each arm uses, which has not changed, and they are cited from the plan doc and the inflight note.Review round, and one claim it falsified. Codex raised five findings, all accepted and fixed in
0b1a6addc; Claude's review requested no changes. Two were worse than reported: thestall/throughputmisclassification above, and a sentence claiming "the three sibling plain fieldsdocs/refactoring.mdnames" - that entry nameslastWorkRequestWasFulfilled,commitRequestedandRetryQueue.closed, noPartitionStatefield at all, so the claim was false rather than merely stale-prone. Replaced with a reproduce command and an owner pointer, and the identical claim one paragraph up - which predates this PR - was fixed in the same pass rather than left as a twin of the defect just removed.Still open, deliberately out of scope:
docs/refactoring.md'sAT_STALE_THREAD_WRITE_OF_PRIMITIVEoffender list is stale in both count and membership - run locally, the rule names all threePartitionStatefields plusProgressTracker.highestRoundCountSeen, against an entry that says "(3)" and lists three others. Not corrected here; both new notes route the reader to re-run the analyser instead of trusting the list.