Repository navigation
refactor(core): run the infer-annotation experiments - two confinement declarations, and a measurement of the rest - #433
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Owner
Author
|
@claude review this Steer, if useful: the load-bearing claims are (1) that |
|
Claude finished @astubbs's task in 6m 35s —— View job Review: both load-bearing claims verified
Claim 1 - I also reproduced the non-vacuity claim live rather than taking the PR body's word for it: built the reactor, temporarily commented out all three rather than the expected Claim 2 - Everything else checked out:
Minor observations, non-blocking:
No correctness issues found. The two "load-bearing" claims the author asked me to scrutinize both hold up under independent verification, including one of them under direct reproduction of the negative-control test run. |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its 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 #433 +/- ##
=========================================
Coverage ? 82.30%
Complexity ? 1470
=========================================
Files ? 95
Lines ? 5142
Branches ? 501
=========================================
Hits ? 4232
Misses ? 712
Partials ? 198
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🟢 Throughput — OKThis branch measured about 3% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: cc36b64, 1941cdf, 05c02bb, 5706686, c668acb, fb5ea93, 6573781, c30aaee, ca9c21b, 77fbba8 Since the previous push: ratio 1.017 -> 1.034, share 1.583 -> 1.557, rate 77335 -> 70450 (-8.9%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
… so declare it and assert it The infer lane (`bin/infer-test.sh`) runs RacerD, pulse and starvation over core main code and gates on an identity ratchet, and every one of those checkers reads annotations from `com.facebook.infer.annotation` - a jar that was not on the compile classpath at all. So the analysis ran on what it could *infer* and nothing the code *declared*. This puts it there (`provided`, CLASS-retention, same reasoning as the Error Prone annotations beside it) and writes the first declaration. Four of the ratchet's known `THREAD_SAFETY_VIOLATION` identities are one boolean, `RetryQueueIterator.closed`, read by `hasNext()`/`next()` and written by `close()` with no synchronisation. They are confined, and not by convention: `RetryQueue.iterator()` takes the queue's READ LOCK on the calling thread and `close()` releases it, and a `ReentrantReadWriteLock` read lock may only be released by its holder. An iterator that escaped to another thread could therefore never be closed there at all - the unlock throws `IllegalMonitorStateException` and strands the read lock, blocking every writer for the life of the process. The lock discipline already required the confinement; nothing said so, so RacerD had no way to know. WHAT CHANGES - `@ThreadConfined(ThreadConfined.ANY)` on `RetryQueueIterator` - ANY rather than a named thread because which one it is varies by caller, and the invariant is that it is only ever the one. - `assertOnOwningThread`, the runtime half. An annotation nobody enforces is a comment that silences a detector, which is worse than no annotation: RacerD believes it and stops looking. It runs BEFORE the closed check in all three methods - a foreign thread reading `closed` is exactly the unsynchronised read the declaration promises cannot happen, so answering it, even to refuse, would honour the wrong contract first. - The engine's `AGENTS.md` gains the rule: declare confinement, assert it at the entry point, and check the premise before writing either. - The four identities are deleted from `config/infer-known-findings.txt` in this commit, which is the ratchet's contract. HOW IT IS PROVEN, both directions - `RetryQueueIteratorConfinementTest`, five cases. The decisive one is the foreign `close()`: it must be refused BEFORE the unlock so the read lock stays with its owner, which the test asserts positively by then taking the WRITE lock from another thread - a still-held read lock makes that hang rather than fail, and `ForeignThread.catching` reports the timeout naming the fixture rather than returning "nothing was thrown". - Non-vacuity arm, run: with the three `assertOnOwningThread` calls commented out, three of the five go red, and the close case fails as `IllegalMonitorStateException` rather than the expected `IllegalStateException` - the stranded-lock failure the guard exists to replace, observed rather than argued. - `bin/infer-test.sh` (Infer v1.3.0, JDK 17): the four `RetryQueue$RetryQueueIterator` identities no longer fire. The register's own warning applies and was honoured - a retirement is not evidence of a fix, so the code was read before the lines came out. TEST FIXTURE, EXTRACTED RATHER THAN COPIED Every confinement test needs a second thread it can name, and `ThreadConfinedConsumerTestBase`'s javadoc already records that two hand-rolled copies of that fixture had drifted - only one unwrapped the `ExecutionException`, so an assertion failing over there surfaced here as a wrapper naming neither the assertion nor the thread. This test is in another package and would have been the third, so the thread half is now `ForeignThread` and that base delegates to it. It adds `catching`, which returns what the action threw instead of rethrowing, because for a confinement test the refusal IS the assertion. WHAT THIS DOES NOT FIX, and is now wrong on purpose SpotBugs reports `AT_STALE_THREAD_WRITE_OF_PRIMITIVE` on the same field and reads no confinement annotation, so it will keep reporting it. `docs/refactoring.md` now says so and says not to "fix" it with `volatile`, which would assert a sharing that does not exist. The finding is baselined, so nothing goes red either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
…and not confined
The second candidate for a `@ThreadConfined` declaration, and it came out the other way - which is
the more useful of the two outcomes, because the value was in checking the premise.
Every record of this field said it was written by `commitOffsetsThatAreReady()` and read by
`isTimeToCommitNow()`, both control-thread methods, which made confining it look like a free
retirement of two known `THREAD_SAFETY_VIOLATION` identities. `tryCommitOffsetsOnRevoke()` writes
it too, from inside `onPartitionsRevoked` - which the broker POLL thread runs, as that method's own
comment says two lines further down ("this runs inside the poll thread's rebalance callback").
Grepping every writer of the field is all it took to see it.
So the field is genuinely shared across the two threads this engine's defects are all about, and
`@ThreadConfined(CONTROL_THREAD)` would have been a FALSE declaration - one RacerD consumes and
does not check, so it would have believed it and gone quiet over a real race.
WHAT CHANGES
- `lastCommitTime` is `volatile`. Both writes are under `commitLock`; the read in
`isTimeToCommitNow()` is not, so the modifier is the only happens-before edge available without
moving the read under the lock. Same shape and same fix as `lastWorkRequestWasFulfilled`
(#201) and `failureReason`.
- The field's javadoc carries the poll-thread writer by name, because the reading every record got
wrong is the one the next person will form too.
- Both identities are deleted from `config/infer-known-findings.txt` in this commit.
- `docs/refactoring.md`'s entry is deleted rather than edited: the finding it staged is now fixed,
and what outlives it - which writer, which thread, why not confined - is in the register row and
the field's own javadoc.
WHAT IT MEANS FOR A USER
Nothing observable. The control thread could miss a commit the poll thread had just made on the
revoke path and commit again immediately - a redundant commit, not a wrong one. Worth fixing anyway
because it is a cross-thread field on the commit-timing path in a repo that tracks commit-timeout
flakes, and because the alternative was to declare it confined and stop looking.
HOW IT IS PROVEN
- `bin/infer-test.sh` (Infer v1.3.0, JDK 17): the `controlLoop` and `isTimeToCommitNow` identities
no longer fire. The register's warning that a retirement is not evidence of a fix applies, and the
code was read before the lines came out - the modifier establishes the edge, it does not merely
hide the report.
- `LastCommitTimeFenceTest` pins the modifier, following `PartitionStateDirtyFlagFenceTest`. No
functional test can see this race - its effect is an extra commit - so without a tripwire an
unrelated refactor drops the keyword and nothing goes red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
… of them this repo can use Two commits below put the annotation jar on the classpath and wrote the first two declarations. This is the survey of everything else in it, so the next reader does not re-derive the list: every verdict was MEASURED - the annotation applied, `bin/infer-test.sh` re-run, the report read - rather than reasoned about from Infer's documentation. None of the annotations measured here is kept in the tree; what is kept is what each one did. `@ThreadSafe` - THE ONE THAT WOULD MOVE THE LANE, AND WHY IT CANNOT YET RacerD only reports on code it believes is meant to be thread-safe and infers that from lock use, so a class that shares state without locks is SILENT rather than clean. Applied to `WorkManager`, `ShardManager`, `ProcessingShard`, `ConsumerManager`, `BrokerPollSystem`, `ProducerManager` and `PCMetrics` together, it took the lane's finding count up roughly tenfold. Two things stop it being adopted, and both are concrete: - Over half the new findings are `INTERFACE_NOT_THREAD_SAFE`, and the interfaces are `org.slf4j.Logger` and micrometer's `Meter`. Both are thread-safe, neither is annotated, and neither is ours to annotate - so every log call site in an annotated class is a finding. The lever is Infer's `--disable-issue-type`, which is a change to a hosted CI lane's checker set. - The largest remaining group is every non-private `ShardManager` method reading `processingShards` "racing with the write in `setProcessingShards`" - a package-private setter that exists for tests and whose production callers `ShardMapIsNeverReplacedArchTest` already forbids. RacerD cannot see an ArchUnit rule, so the group is right about the seam and wrong about the code. The rest of the triage is in the note, group by group. Two are worth naming here: RacerD independently refinds `ConsumerManager.commitRequested`, the wakeup counters and `BrokerPollSystem.longPollTimeout`, all already listed in `docs/refactoring.md` by SpotBugs - corroboration rather than new ground; and `ConsumerManager.closeInProgressSignal` is the `@Initializer` case in the flesh, a false positive that exists only because the annotation is on. `@Lockless` - IT SEES THE MONITOR THE ARCHUNIT RULE CANNOT `clearCommitCommand()` carries a cleared suspicion dated 2026-08-31 whose last paragraph says no gate protects the invariant, because `ArchitectureTest.rebalanceCallbacksMustNotBlock` matches method calls and a `synchronized` block is a `MONITORENTER` it cannot see. Measured: `@Lockless` on `onPartitionsRevoked` reports exactly that monitor, by name, through exactly that method - and does NOT report `commitLock.tryLock()`, so it agrees with this repo's decline-rather-than-wait rule. It is still not adopted, and the reason is worth having written down: the monitor it reports is one the design deliberately keeps, so annotating the callback leaves a violation standing whether the invariant holds or not - the same complaint the cleared suspicion makes of the ArchUnit rule. What it is good for is a query: it is the only thing here that can enumerate the monitors a rebalance callback reaches, which has now been done by hand twice - that suspicion, and the `PCMetrics.removeMeter` monitor #431 found on the same path and left alone. The javadoc is amended in place with a dated paragraph rather than rewritten, because the 2026-08-31 conclusion is unchanged; only what is now known about checking it again. That is also the boundary with #431, which is open on the same rule: it closes the rule's METHOD-REFERENCE blind spot and deletes six `KNOWN_BLOCKING_VIOLATIONS`, while this is about the `MONITORENTER` one, which widening the walk cannot reach. Neither makes the other wrong. `docs/inflight/static-archunit-main-code-rules.md` enumerates that rule's blind spots and names neither; #431 was rewriting that file concurrently, so the entry is left for whoever touches it next rather than written into a conflict - stated in the note so it reads as a decision. `@NonBlocking` - INERT On the same method, over the same `Thread.sleep(100)`, it reported nothing. WHAT IS RECORDED WHERE - `parallel-consumer-core/.../AGENTS.md` gains "Which of Infer's annotations this repo uses, and which it does not" - the durable half, next to the `@ThreadConfined` rule it completes. `@Initializer`, `@Functional` and `@SynchronizedCollection` are on it as not-yet-writable, with the reason: all three exist to SUPPRESS a report, and none of those reports is raised on an unannotated class, so they become worth writing the day `@ThreadSafe` goes on and not before. - The one open question gets the note - `docs/inflight/static-infer-threadsafe-is-blocked-by-third-party-interfaces.md` - tagged `blind-spot` so it arrives at session start rather than needing to be remembered. - The measured counts are deliberately NOT written into the note. They are a command's answer, the reproduce line is there instead, and the first draft's numbers were stale before it was committed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
astubbs
force-pushed
the
feat/infer-annotation-sweep
branch
from
September 3, 2026 10:52
01fb0c4 to
53ef4d6
Compare
astubbs
changed the base branch from
docs/225-producer-fencing-brainstorm
to
master
September 3, 2026 10:52
Draft
6 tasks done
Owner
Author
|
@claude review this Re-requesting: the branch was re-cut off #410 and onto master, so this is a different head from the earlier request - three commits now, and the Steer, if useful: the load-bearing claims are (1) that |
|
Claude finished @astubbs's task in 6m 10s —— View job Review: both load-bearing claims re-verified on the re-cut branch
This is a re-request on a branch re-cut off #410 and onto master (HEAD Claim 1 — I reproduced the non-vacuity claim live rather than trusting the PR body: commented out all three rather than the expected Claim 2 — Ratchet integrity verified independently, not just asserted: Both new tests run green on this tree (via surefire reports, not just
Everything else checked out:
No correctness issues found. Both "load-bearing" claims hold up under independent verification against the actual re-cut tree, including direct reproduction of the negative control. |
[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. |
…from branch content `RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed` went red on this branch's CI, failing its saturation/pause-point setup guard - "control thread must reach the mid-loop pause point (offset 25)" - which is the shape the ledger already records, not the confluentinc#909 signature assertion the test exists for. Recorded rather than re-run silently, because this sighting says something the previous four could not. The branch ran the SAME code twice: it PASSED at `01fb0c4` and FAILED at `53ef4d6`, and the only difference between those heads is a re-cut that changed the commit split and merged one unrelated reactor-test commit. Nothing this test drives changed between the two. The four earlier sightings were each on a different branch, so none of them could separate "this branch's content" from "this test"; a pass/fail pair on one content set can. Attribution was checked rather than assumed, since this branch does touch concurrency: - The confinement guard it adds has exactly one production caller chain - `AbstractParallelEoSStreamProcessor.getTimeToBlockFor` to `WorkManager.getLowestRetryTime` to `ShardManager.getLowestRetryTime`, which opens and closes the iterator inside one method on one thread. It cannot fire across threads, and it is not on the consumer-sync registration path this test drives. - `bin/inflight.mjs codecov test <name>` shows the same guard red on an unrelated branch within the same hour and green on many heads either side, across branches that carry the test and branches that do not. - The ambient probe reports 10% of the deadline used - elapsed 30.8s against 300s - so what expires is the guard's own wait, not the test running out of time. Left in the ledger rather than quarantined: quarantine is master-state and wants a diagnosis or a sighting ledger, and this is the ledger doing its job. The diagnosis is still open, and the section below the table owns it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
[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. |
astubbs
marked this pull request as draft
September 3, 2026 11:31
Inherited: #116's jstream result-stream fix (breaking, and it WITHDRAWS the queued JStream deprecation - `docs/refactoring.md` loses those entries here as a result, which is master's change and not this branch's), and #419's docs context query, which moves `bin/lib/inflight-tags.sh` to `.mjs` and rewrites the prior-art section of the root `AGENTS.md`. Neither touches the infer lane, the retry queue or the commit-timing path. One conflict, in `docs/inflight/test-untracked-ci-flakes.md`, on the registration-race row both sides had just edited. Resolved by keeping BOTH controls rather than either side's: - master's same-day CROSS-branch control (the same guard red on two unrelated branches within an hour, green at neighbouring commits on both), from #419; - this branch's WITHIN-branch control (the same content passing at `01fb0c4` and failing at `53ef4d6`), which is the stronger of the two because no cross-branch comparison can hold content fixed. The count is the union of the two sighting lists, and master's new `AmbientProbeExtensionTest` row is kept - it arrived in the same hunk and taking either side whole would have dropped it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
… object, recorded not fixed `AGENTS.md` asks for a search for other instances of a defect before its fix merges, and for the result to be stated either way. Both fixes on this branch were swept. **The confined-object shape - one more instance found.** `ProducerManager.ProducingLock` wraps a `ReentrantReadWriteLock.ReadLock` taken in `acquireProduceLock` and released by whoever calls its `unlock()`: structurally identical to `RetryQueueIterator`, and carrying the same constraint, since a read lock may only be released by its holder. Its own javadoc already describes what an unreleasable hold costs - "the same permanent block on the next commit's write-lock acquisition". Recorded rather than fixed, because the recipe must not be applied blind: the premise is that the acquirer is always the releaser, and being wrong about exactly that premise is what the `lastCommitTime` commit on this branch is about. Where I looked: every `ReentrantReadWriteLock` in core main code, and every method returning an `Iterator`/`Closeable`. `PollContext.iterator()` and `Range.iterator()` were checked and dismissed - they iterate local or immutable data and hold no lock. **The assumed-confinement shape - one instance, already tracked.** `AbstractParallelEoSStreamProcessor.numberOfAssignedPartitions` is a plain `int` assigned in all three rebalance callbacks, so it is the same class as `lastCommitTime` and already sits in this file under SpotBugs' `AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE`. It is the only other field written by those callbacks - checked by extracting each callback body and resolving every assignment against its declaration, rather than by eye. Neither is fixed here. Naming where the search ran matters as much as the hits: a first pass over `readLock().lock()` call sites and iterator-returning methods found nothing, and that was the pattern being wrong rather than the tree being clean - `ProducerManager` holds its `ReadLock` in a field, not at a call site the grep could see. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
Owner
Author
|
@claude review this Third request, on a materially changed head: master is merged in (#116 jstream, #419 docs-context-query) and two records were added since the last review — a registration-race flake sighting, and the defect-class sweep result in The load-bearing claims are unchanged and were verified last round; what is new and worth scrutiny:
|
|
Claude finished @astubbs's task in 3m 4s —— View job Review: both new claims verified, nothing lost in the merge
This is the third review, on HEAD 1. The merge conflict resolution — verified clean, nothing dropped from either side. I diffed both parent tips against the merge result directly rather than trusting the commit message's account:
Nothing from either side is missing. 2.
3. Read all three callback bodies in No correctness issues found. Both falsifiable claims hold up, and the merge conflict resolution preserved the full content of both sides. |
astubbs
marked this pull request as ready for review
September 4, 2026 05:24
…ead reasoning The duplication reports on #433 flagged four new file similarities. Two are this branch's doing and are fixed here; the other two are not. **`RetryQueueIteratorConfinementTest` carried a verbatim copy of `RetryQueueTest`'s fixture** - the `TOPIC`/`PARTITION`/`EPOCH` constants, the `PCModuleTestEnv`, the queue, and `workFor(long)`, differing only in the topic literal. That is precisely the parallel copy `AGENTS.md` forbids when it says to reuse test utilities, and the shape behind the drifted topic-creation helper that became a flaky-CI source. Both suites now extend `RetryQueueTestBase`. The topic is one constant for both on purpose: neither suite asserts on it, so two names bought nothing and were the part most likely to drift. **The foreign-thread fixture's reasoning was stated in full in two places.** `ForeignThread` was extracted from `ThreadConfinedConsumerTestBase` earlier on this branch, and both javadocs then explained the same drift incident - the two copies where only one unwrapped the `ExecutionException`. `ForeignThread` is the owner now and the base cross-references it, per `AGENTS.md`'s rule that a fact stated twice is two facts that drift apart. This is the same duplication in prose that the first item is in code. NOT FIXED, AND WHY - **`LastCommitTimeFenceTest` and `PartitionStateDirtyFlagFenceTest` at 40%.** The shared part is one Truth assertion over `Modifier.isVolatile`, three lines, and what each test is actually FOR is its message - the field, the two threads, and what a future editor should do when it fails. Extracting the assertion would move that message one level away from the failure and buy three lines. Following an existing precedent deliberately is not duplication to remove. - **`RetryQueueLincheckTest` and `RetryQueueTest` at 30.1%.** Pre-existing on master; this branch touches neither, and `AGENTS.md` scopes the duplication follow-up to clones this PR introduced. The two engines disagreed and both were right: PMD CPD and jscpd reported no new clones, because they count token-level clones and none of this is one. The platisd tool measures text similarity including comments, which is what caught the copied fixture and the twice-told story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
`dups: clones` came back CANCELLED on three consecutive heads of #433. It is a required check, so a cancelled row blocks the merge - and a re-run that went green would have looked identical to a fix, which is why this is recorded before re-running rather than after. WHAT IT ACTUALLY IS The job carries `timeout-minutes: 5`. The cancelled attempt ran 05:30:41 to 05:35:48 - 5m07s - and the runner's cleanup names what was still going: `Terminate orphan process: pid (2693) (npm exec jscpd@)`. The action runs PMD CPD **and** jscpd over the whole tree, twice (PR and base), inside those five minutes, so the cap has no meaningful headroom and the lane tips over on a slow runner. WHY IT IS NOT THIS BRANCH - The same lane **completed on an earlier head of this same PR** and posted its full report, with both engines' tables and "No new clones introduced by this PR" - so the tree is analysable inside the cap when the runner cooperates. - Its sibling `dups: similarity` succeeded in the same run, so this was not a superseded workflow. - The job is `if: github.event_name == 'pull_request'`, so it never runs on master. Master history shows `skipped`, which is the absence of evidence rather than evidence of health - worth stating, because the reflex is to compare against master and read those skips as green. Not to be confused with the failure `ci-duplication-report-can-fail-to-post.md` already owns: that one is `dups: clones` FINDING a real clone and being unable to post it. This is the lane never finishing. A cancelled tick and a failed tick look alike in the checks list and mean opposite things. The durable fix is a CI change on master - raise the cap, or split the two engines into separate jobs so neither pays the other's time - so it is recorded here rather than attempted from a feature branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
… correct the record The previous commit filed `dups: clones` as a flake-ledger row saying the lane "tips over on a slow runner". A fourth attempt, on a fresh five minutes from a new head, failed identically, and the framing does not survive it. The row is retired and replaced with a note that says what the evidence actually supports. WHAT THE EVIDENCE SAYS Outcomes split cleanly by DATE, not by branch or runner: two attempts on 2026-09-03 succeeded and posted the full report; four on 2026-09-04 were cancelled. One of those four is on an unrelated PR, which is what rules out branch content - a second PR failing the same way at the same time cannot be explained by either branch's diff. The boundary is master gaining #419 and #116. Each cancelled attempt dies a few seconds past the five-minute cap with jscpd still running, named by the runner's own cleanup as an orphan process. The action runs PMD CPD and jscpd over the whole tree twice - PR and base - inside one five-minute budget, so corpus growth is paid four times. WHY THE FILING MATTERS AS MUCH AS THE DIAGNOSIS A deterministic breakage sitting in a flake ledger invites the response a flake deserves - re-run until green - which here would burn runner time and hide a required check that no longer works. It also is not the failure `ci-duplication-report-can-fail-to-post.md` already owns: that one is the lane FINDING a clone and failing to post it. A cancelled tick and a failed tick look alike in the checks list and mean opposite things. WHAT IT COSTS RIGHT NOW `dups: clones` is a required check, so while this holds no PR in the repository can merge. That is why the note is `impact: stall` rather than a CI-hygiene item, and why the fix belongs on master rather than on the feature branch that happened to find it. Master history cannot be used to attribute it: the job is `if: pull_request`, so on master it is `skipped`, which reads as green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
6 tasks done
Inherited #439, #445 and #442. The first and third matter to this branch: they fix the `dups: clones` lane that was blocking it, and they do so with a cause this branch got wrong. THE DUPS NOTE THIS BRANCH ADDED IS DELETED, NOT UPDATED `ci-dups-clones-lane-times-out-and-blocks-every-merge.md` described a problem #442 has fixed, so keeping it - even rewritten as a was-fixed narrative - is what `docs/inflight/AGENTS.md` forbids. Nothing in it survives as both true and unowned: master's workflow now carries the explanation at the point of use, and states it better. **Its causal claim was also wrong, which is the part worth recording.** The note attributed the timeout to corpus growth from #419 and #116, reasoning from a clean date boundary - every attempt on 2026-09-03 passed, every attempt on 2026-09-04 failed, corroborated on an unrelated PR. The mechanism was right (the job exceeding its five-minute cap with jscpd still running) and the attribution was not. The real cause, per #442's workflow comment: the action installed jscpd with `npx jscpd@4` inside the step it measures, so the base-branch engine paid a cold install the PR-branch step did not - 288s against 15s on the same tree - and npm's 300s default fetch-timeout let one stalled registry request consume the whole budget. The date boundary tracked registry conditions, not master's content. That is a reasoning failure worth naming rather than quietly dropping: a correlation across two branches felt like a controlled comparison, and it was not - both branches shared the runner and the registry, which is exactly the variable that moved. The control that would have settled it was re-running an OLD head, which this branch never did. THE FLAKE LEDGER'S REGISTRATION-RACE ROW, CONFLICTED AND UNIONED Both sides had grown it independently and neither is a superset. Master's version carries four #442 sightings - two of them on documentation-only heads - and counts a `ce-optimize` sighting where this branch counts #433; those are different runs, so the count is their union rather than either side's. This branch's WITHIN-branch control is restored, because master's row never had it and it is the strongest evidence in the row: the same tree produced both outcomes. Master's documentation-only heads are that same argument from the other direction, so they are kept beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
Inherited #440, #447 and #435. Two of them touch this branch's ground. #440 QUARANTINES the registration-race test this branch has been recording sightings of. Nothing here needs to change for that - the ledger row and the quarantine registry are different instruments, and master keeps both - but it means a future red from that test is no longer this branch's problem to attribute. THE LEDGER ROW, CONFLICTED A THIRD TIME AND UNIONED AGAIN Both sides said "12 seen" and neither was a superset: they count DIFFERENT seventh sightings on 2026-09-03 - master's is #438, this branch's is #433 - so the union is thirteen. A count that matches on both sides is the easiest kind of conflict to resolve wrongly, because taking either side whole looks like a no-op and silently drops one real sighting. Master's #438 control is the stronger of the two and is kept as such: that branch's whole diff is `bin/` and `docs/`, compiling no Java, so the tree built there IS master's - branch content is excluded by construction rather than by comparison. This branch's within-branch control is kept beside it because it establishes something that one cannot: one tree, both outcomes, which excludes the tree itself and leaves only the runner. They are different arguments, not two tellings of one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
🧪🔒 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 |
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…this branch's sighting #433 and #443. One conflict, the RegistrationRaceStaleResidentIT ledger row, and both sides had reached "13 seen" while counting different sets - so either taken whole would have dropped a sighting under an authoritative-looking number, for the third time on this file. Master's side is the base and deserves to be: it carries the #442 and #438 sightings and, more importantly, a WITHIN-branch control this branch had no way to know about - #433 ran the same content twice, green then red after a re-cut that changed only the commit split, so one tree produced both outcomes and the tree itself is ruled out, leaving the runner. That is a stronger claim than the cross-branch controls it joins. Added to it: this branch's 2026-09-04 #444 sighting with its same-head re-run, and one clause noting that sighting is another of the same-day cross-branch kind. 14 seen. Recorded because the first attempt at this resolution asserted on an anchor that did not exist, wrote nothing, and the file was then staged WITH its conflict markers - caught by grepping the markers rather than trusting the exit code. The second attempt carries an anchor-free fallback that announces itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WErnxQd9Ew57F9SsqzdPU5
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ot only a JDK deny list `RetryQueue.remove`, `add`, `removeAll` and `clear` take the write lock unconditionally, and the rule that they are for the controller thread only was prose on one javadoc plus a deny-list of JDK blocking signatures. Neither is a check on the caller: the prose is not read by anything, and the deny list answers a narrower question than the rule asks - `RetryQueue.tryRemove` takes the SAME lock through `tryLock()`, which is correctly absent from the list, so no entry in it can tell a waiting acquire from a declining one when the method itself is ours. `@ControllerThreadOnly` (new, in `state`, beside the class it governs - main code declares no annotation type of its own until now) makes the contract a declaration, and `ArchitectureTest.rebalanceCallbacksMustNotBlock` now reports a reach INTO an annotated method exactly as it reports a deny-listed call: same `root => target` exemption key, same message shape, for a direct call and a method reference alike. `KNOWN_BLOCKING_VIOLATIONS` semantics are unchanged. The owner type is checked as well as the method, because the annotation targets TYPE too and a class-level declaration the walk ignored would be a silent gap rather than a narrower rule. **Deliberately NOT Infer's `@ThreadConfined`**, and the marker's javadoc says so. The engine's own rule - `parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.md`, "Declare thread confinement with `@ThreadConfined`, and assert it at the entry point" - requires that one to be paired with a runtime assertion because RacerD CONSUMES it, so an unpaired declaration silences a detector and is worse than nothing. This marker is read by no analyser and silences nothing, so the pairing rationale does not reach it; it is checked by ArchUnit reachability and nowhere else. The runtime half - a named-thread `@ThreadConfined` plus an `assertOnOwningThread`, in the shape #433 established for `RetryQueue.RetryQueueIterator` and `ThreadConfinedConsumer` - is deliberately not implemented here and is tracked in the note that follows this commit. **The premise was checked rather than assumed, per that same AGENTS.md rule.** Every production caller of the four annotated methods, on this branch: - `ProcessingShard.getWorkIfAvailable` -> `retryQueue.remove(removed)` (last-resort stale sweep) and `retryQueue.removeAll(..)` (work taken), reached from `ShardManager.getWorkIfAvailable` -> `WorkManager.getWorkIfAvailable` -> `AbstractParallelEoSStreamProcessor.retrieveAndDistributeNewWork` - the control loop. - `ShardManager.onSuccess` -> `retryQueue.remove(wc)` and `ShardManager.onFailure` -> `retryQueue.add(wc)`, reached from `WorkManager.handleFutureResult` -> `AbstractParallelEoSStreamProcessor.processWorkCompleteMailBox` - the control loop. - `clear()` has no production caller at all. - The two rebalance-path sweeps that used to reach `remove` - `ShardManager.removeWorkFromShardFor` and `ProcessingShard.removeStaleWorkContainersFromShard` - both go through `tryRemove` since 83115d6, which is why the rule is green with the annotations on. **RED FIRST, twice, because a rule that is green on arrival has proved nothing.** 1. With the rule UNCHANGED, routing a callback back onto the waiting method (one temporary `this.retryQueue.remove(ignoredRemovedWC)` in `ShardManager.removeWorkFromShardFor`) already turns it red: ArchitectureTest 3 tests / 1 failure, six violations, every one "reaches blocking call java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock.lock() via bz.stub.parallelconsumer.state.RetryQueue.remove(..)", from `onPartitionsRevoked` and `onPartitionsLost` on `AbstractParallelEoSStreamProcessor`, `PartitionStateManager` and `WorkManager`. That is the deny list doing its job, and it is the control arm for the second proof. 2. With the production code untouched and `tryRemove` temporarily annotated, the same rule reports the same six roots through the ANNOTATION - "reaches @ControllerThreadOnly call bz.stub.parallelconsumer.state.RetryQueue.tryRemove(java.lang.String, int, long) via bz.stub.parallelconsumer.state.ShardManager.removeWorkFromShardFor(..)" - even though `tryLock()` is not deny-listed and the walk has nothing else to match. That is the whole point: the annotation path works independently of the JDK deny list. Both temporary edits reverted, empty diffs verified. The standing proof is a second fixture beside `archfixture/BlockingReachThroughAMethodReference`: `ControllerThreadOnlyReachFromARebalanceCallback` reaches one annotated method from a callback-named method twice, once by call and once by method reference, and its annotated method WAITS FOR NOTHING - a field write - so no entry in the deny list can match it and the annotation check is the only thing that can report it. `RebalanceCallbackRuleControlTest` asserts both kinds are reported. Checked red: with the annotation block removed from `inspectReach`, the rule reports one violation (the sibling blocking fixture) and the new test fails on the missing `onPartitionsAssigned` reach. Records: the rule's javadoc gains the paragraph and corrects its "a green run means" sentence, which now names the declaration as well as `BLOCKING_CALLS`; `inspectReach`'s javadoc says why the deny check runs on a name and the annotation check on the resolved member; `docs/inflight/static-archunit-main-code-rules.md` gains the measurement beside its dated text. `RetryQueue.remove`'s prose contract is replaced by a pointer at the annotation, so it is stated once. Fresh surefire: ArchitectureTest 3/0/0/0, RebalanceCallbackRuleControlTest 2/0/0/0, RetryQueueTest 10/0/0/0, RetryQueueRebalancePathTest 6/0/0/0. Core unit suite 672/0/0/8, whole reactor 752/0/0/8, `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_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
… contract does not cover The commit before this one made "only the controller thread may call these" a declaration checked by ArchUnit reachability. That check is static, and its own javadoc enumerates three ways a poll-thread call can still arrive at a waiting acquire with the rule green: a reference that is STORED rather than invoked (ArchUnit's model gives a gauge or an executor task the same shape as a stream stage, so the walk cannot say WHEN a reach happens), a user-supplied `ConsumerRebalanceListener` (dynamic dispatch through an interface is out of reach of any deny list, and it is code the walk cannot start from), and a `synchronized` BLOCK (a MONITORENTER is not an access - the reason the rule would not have caught confluentinc#857 itself). None of those is closable by widening the rule. The note describes the guard that closes them, and it is NOT implemented here: the #393 ownership shape applied to `RetryQueue`. That PR's pieces are named so the next reader copies rather than designs - `ConsumerOffsetCommitter`'s volatile `Optional<Thread> owningThread`, set once by `claim()` when the owning thread's loop starts and read by `isOwner()`, and `PCModule.consumerManager()`'s thread-confinement wrapper, whose own comment states the lifecycle rule this would follow. Applied here: the controller claims the queue when the control loop starts (a lifecycle step, not a constructor argument, because the object exists before the thread that will own it), the `@ControllerThreadOnly` methods assert the current thread is the owner and fail loudly instead of taking the lock, and the guard is UNARMED while nobody has claimed - which is what leaves unit tests that drive a `RetryQueue` directly, and init-time use, unaffected. Cost: one reference compare per call, on a path already taking a lock. It is positioned against the convention on master rather than beside it. `parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.md` owns "Declare thread confinement with `@ThreadConfined`, and assert it at the entry point", and the note cites it: the two existing patterns are `RetryQueue.RetryQueueIterator` (`@ThreadConfined(ThreadConfined.ANY)` plus `assertOnOwningThread`, with `RetryQueueIteratorConfinementTest` failing when the two disagree) and `ThreadConfinedConsumer`, both from #433. This guard is that shape with a NAMED thread - the control thread - which the rule says is the right value when the code really does pin one, and which is what gives the assertion something specific to compare against. Two design questions are what defer it, and both are stated rather than guessed. What "fail loudly" does inside `consumer.poll()`: a throw leaves a rebalance callback abnormally and its blast radius is the group, while "log an error and decline" is the behaviour the queue-first sweep already treats as safe - the better signal in a test is the worse one in production. And claim/release across a restart: `claim()` is called once and never released, so a controller that stops and starts, or a second processor in one JVM, needs a decision on whether a claim can be replaced, refused or dropped at close. Filed per `docs/inflight/AGENTS.md`: `core-` because it is product behaviour and the prefix names an area, `task` + `reliability` because no single defect is named yet, `concurrency` label, and `deferred` with what it waits on. No counts and nothing a command could answer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ect yet Asked at merge prep: can the concurrency annotations help here, or expose this better? Recorded because the intuitive answer is wrong in a way that would cost something. A DECLARATION HERE WOULD BE FALSE, AND FALSE IS WORSE THAN ABSENT. RacerD reads com.facebook.infer.annotation.ThreadConfined and consumes declarations without checking them, so @ThreadConfined(CONTROL_THREAD) on the state the revoke-path drain would mutate does not expose this defect - it silences the detector that might otherwise find it. RetryQueue's own usage already states the rule: an annotation nobody enforces is a comment that silences a detector. INDEPENDENT CORROBORATION, WHICH IS THE PART WORTH KEEPING. #433 reached this same seam from the other direction while putting the infer annotations on the compile classpath. It evaluated lastCommitTime as a confinement candidate, found every record of the field claiming two control-thread accessors, grepped the writers, and found tryCommitOffsetsOnRevoke() writes it from inside onPartitionsRevoked on the broker poll thread. It made the field volatile instead and said so in the field's javadoc. One investigation started from the mailbox drain and one from a field's writers; both landed on the revoke path running control-thread work on the poll thread, and neither knew about the other. WHAT THE ANNOTATIONS ARE FOR HERE IS THE FIX. @GuardedBy does not fit - this is confinement rather than lock discipline, and the core AGENTS.md records that it silently checks nothing on a ReadWriteLock. The shape that fits is RetryQueueIterator's: once the thread-ownership decision at this seam is taken, declare the confinement AND assert it on the owning thread in the same change, so the decision cannot rot back. Until that decision exists there is nothing truthful to declare, which is why this note stays open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MWLkoGdvM2CHQCsYpgUMAR
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…pair with Base movement, 19 commits. #437 (440b9bd) closes the retry-queue orphan window by confirming shard residency AFTER the re-queue's add, and #433 (9999144) declares `RetryQueue.RetryQueueIterator` `@ThreadConfined(ANY)` with an `assertOnOwningThread` at every entry point. Both land in files this branch rewrites. NOTHING CONFLICTED, which is the case to be suspicious of rather than pleased about - the two sides touch adjacent regions of the same four files and git merged them without asking. Checked by hand afterwards, both sides' intent is present and distinct: - `RetryQueue`: #433's `@ThreadConfined(ThreadConfined.ANY)` on the iterator type plus its three `assertOnOwningThread` guards, AND this branch's `@ControllerThreadOnly` on `remove`/`add`/ `removeAll`/`clear`. They answer different questions - one is consumed by RacerD and paired with a runtime assertion, the other is read only by `ArchitectureTest.rebalanceCallbacksMustNotBlock` - and the marker's own javadoc says why it is deliberately not the Infer annotation. - `ShardManager`: #437's post-add `isResident` confirmation in `onFailure`, AND this branch's queue-first `tryRemove` in `removeWorkFromShardFor`. - `ProcessingShard`: #437's `isResident` predicate and its use in `includeInSelection`, AND this branch's `removeStaleWorkContainersFromShard(RetryQueue)`. THE RED IS EXPECTED AND IS THE NEXT COMMIT'S STARTING EVIDENCE. `RetryQueueRequeueWindowTest.theProductionSweepsQueueRemovalIsGatedOnItsShardRemoval` fails at its final assertion - "PRODUCTION IS SHARD-FIRST, and the confirmation in ShardManager.onFailure depends on it". That arm exists precisely to go red the moment production stops being shard-first, and this branch is what stops it. `RetryQueueRequeueWindowTest` 9 tests / 1 failure. Its sibling `aQueueFirstSweepDefeatsTheOneShotConfirmation` is GREEN, and that is not a reprieve: it hand-builds both orderings, so production moving cannot move it - the over-claim review caught on #437 and the reason the production arm was added. It still has to be re-stated against the paired removal. Everything else green on the merged tree: `ArchitectureTest` 3/0/0/0, `RetryQueueTest` 10/0/0/0, `RetryQueueRebalancePathTest` 6/0/0/0, `RebalanceCallbackRuleControlTest` 2/0/0/0, `RetryQueueIteratorConfinementTest` 5/0/0/0. `bin/check-branch-self-reference.sh` is red on 12 references in three notes that arrived from master and name #431 in the future tense - the note addressed to this PR, the shard displacement note, and the Infer `@ThreadSafe` note. All three are the next commit's work, because what they should say depends on what it does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rEzrWYFr6oEzy6porczd3
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
Master moved 21 commits under this branch in two days. Three touch the rider's own files and none argues with it: #450 registers a record's offset before publishing it to its shard (PartitionState, adjacent to but not inside the commit path this branch changed); #452 makes the codec's encodingCounters a ConcurrentHashMap (the encode-path static this branch's tryToEncodeOffsets calls through); #106 walks only the offsets where completion state changes when no encoder needs every offset (the overhead table's domain is unchanged by it, and its tests pass on the merge). Two text conflicts, both in append-only records, both resolved in master's favour plus this branch's additions: config/infer-known-findings.txt keeps master's retirement of every THREAD_SAFETY_VIOLATION identity (#433) and this branch's three BUFFER_OVERRUN_L1 identities; docs/refactoring.md keeps master's rewritten thread-safety entry for the codec (#452 replaced the "not thread-safe" heading and body) and this branch's two entries above it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkRtYPnDuAPLEmHFYSnAiJ
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…dy produced (#436) The control loop commits like this: take the producer write lock, drain the work mailbox, then commit. Draining is what makes the transactional guarantee hold - a record already produced into the open transaction has its success sitting in the mailbox, and only the drain marks its partition dirty so the offset collection includes it. The revoke path takes the same lock and commits, and never drains. So a revoke-time commit can publish a transaction whose offsets omit work whose success is still queued: output committed, input offset not, next owner reprocesses that input and produces the output again. Exactly-once degrades to at-least-once on that path. THE WINDOW IS REACHABLE, AND THE ARGUMENT THAT IT MIGHT NOT BE WAS BACKWARDS. The note this started from guessed the produce read lock, held across the send and its acks, might leave the mailbox empty of produced work whenever a commit could begin. The ordering is send, then addToMailbox, then release: cleanUpContext is the single release point and runs in runUserFunction's finally, strictly after the batch is mailboxed. A returned produce lock therefore GUARANTEES the work is already queued, and a commit granted the write lock always has undrained work in front of it. The lock discipline does not close the window; it guarantees the window is open. Controlled experiment, prediction stated first: the revoke as it stands is red, sending offset 1 where 2 is required; the identical arm with processWorkCompleteMailBox inserted before the revoke is green. Same magnitude, different position, so the drain is the term. Deterministic - hand-driven control loop, mocked producer, no broker, no load. WHAT IT DOES TO THE REGISTER. C9 NO_PRODUCE_WITHOUT_ITS_OFFSET and C4 OFFSET_AND_RECORDS_ATOMIC both move PROVED to REFUTED: they are two statements about the same commit and one run falsifies both, so leaving C4 proved beside a refuted C9 would have had the register assert and deny one observation. C4's exemption does not cover it - that exemption is marker-delivery ordering within a commit, and here the offset is never committed by that transaction at all. Both keep their control-loop proofs, because that half still holds. C14 RESULTS_EXACTLY_ONCE_UNDER_FAILURE is deliberately left PROVED with a scope note: the route to a duplicate is sound but no duplicate has been observed, and C14's own record says its red and green were each seen rather than argued. Refuting it on reasoning would make it the register's first argued status and break the observed-versus-argued distinction that is the register's whole value. The README caution names both refuted guarantees and states the front-page promise is not refuted; STRATEGY.md names the refuted claims instead of counting statuses. NOT FIXED HERE, DELIBERATELY. The one-line fix - drain from onPartitionsRevoked - runs on the broker-poll thread and mutates control-thread-confined WorkManager state, the shape that corrupted numberRecordsOutForProcessing in #29. The control loop also keeps draining during a rebalance, so a poll-thread drain would race it. The red arm is quarantined rather than deleted, so the proof keeps executing and the lane demands its removal the day it passes; it is not @ProvesClaim, so C9 keeps enforced coverage from the two control-loop proofs that run. THE FIX'S POSITION IS RECORDED, checked against #408 rather than guessed. That PR declines the revoke commit only when the lock is contended; the uncontended inline commit is this defect's path, so it neither narrows nor widens it. Recorded: decline unconditionally in transactional mode, as its own change from master. The question that decides its correctness is not established and is stated as such - on decline, is the transaction's produced output aborted, or committed later without the revoked offset. It must be run, not argued. A confinement declaration cannot yet be written truthfully at this seam; #433 reached the same conclusion independently from lastCommitTime's writers. The fix owes the declaration and its runtime assertion in the same change. Reviewed by Codex and by Claude, six findings fixed and one escalated to the operator, whose ruling is the C4/C14 split above. A test-soundness finding was worse than it looked: the sampled assertion sat in the shared helper, so the flake would have landed in the control arm, which gates. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…rd to its shard, as #370 established on the poll path #450 swapped maybeRegisterNewPollBatchAsWork to register-then-publish and left this branch an instruction in the refactoring backlog: the replay loop repeats the old order. It did. A scanner that selects and completes a replayed container the instant it is reachable through its shard completed an offset the partition had not yet re-registered, and onSuccess's assert fired from inside the replay - latent for the same reason it was latent on the poll path, one control thread doing both, and the same defect the day there is a second selector. Pinned the way #450 pinned its half: a shard manager installed through the module's seam that selects and completes the container on publish. RED on the old order (the AssertionError escaped the replay, observed before the swap), GREEN after, with the seam checked to have fired so a test that never raced cannot pass by accident. The backlog line that asked for this is retired. Also in this commit, because it was staged with it: the Infer annotations survey note is retired. #433 did what it proposed and more - confined the retry-queue iterator, showed the lastCommitTime confinement would have been a false declaration (the poll thread writes it too), measured every other annotation in the jar, and put the verdicts in the engine's AGENTS.md with the one open item in its own note. Two records of the same question would drift. 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
The infer lane (
bin/infer-test.sh) runs RacerD, pulse and starvation over core main code and gateson an identity ratchet - and every one of those checkers reads annotations from
com.facebook.infer.annotation, a jar that was not on the compile classpath at all. So the analysishas been running on what it could infer and nothing the code declared. This PR puts the jar
there, writes the two declarations the code turns out to justify, and measures the rest of the jar
so the next reader does not re-derive the list.
Every verdict here was measured - annotation applied,
bin/infer-test.shre-run, report read -not reasoned about from Infer's documentation.
Result: the ratchet goes from 17 findings to 11, and every
THREAD_SAFETY_VIOLATIONin it isretired. What remains is the pulse null-deref group, which
docs/inflight/core-stale-arrival-guard-needs-a-null-safety-decision.mdowns, and one integeroverflow.
Two
@ThreadConfinedcandidates, and they came out opposite waysRetryQueueIterator- confined, and the lock discipline already required it. Four knownidentities were one boolean,
closed, read byhasNext()/next()and written byclose()unsynchronised.
RetryQueue.iterator()takes the queue's READ LOCK on the calling thread andclose()releases it, and aReentrantReadWriteLockread lock may only be released by its holder -so an iterator that escaped to another thread could never be closed there at all: the unlock throws
IllegalMonitorStateExceptionand strands the read lock, blocking every writer for the life of theprocess.
@ThreadConfined(ANY)says so,assertOnOwningThreadasserts it, and the four identitiescome out of the ratchet.
lastCommitTime- NOT confined, and every record of it said otherwise. This row of the inferregister and
docs/refactoring.mdboth had it written by one control-thread method and read byanother, which made confining it look like a free retirement of two more identities.
tryCommitOffsetsOnRevoke()writes it too, from insideonPartitionsRevoked, which the brokerpoll thread runs - as that method's own comment says two lines further down. Grepping every
writer was all it took.
@ThreadConfined(CONTROL_THREAD)would have been a false declaration, andRacerD consumes declarations without checking them: it would have believed it and gone quiet over a
real race. The field is
volatileinstead.What it changes for a user: nothing observable. The control thread could miss a commit the poll
thread had just made on the revoke path and commit again immediately - a redundant commit, not a
wrong one. Worth fixing because it is a cross-thread field on the commit-timing path in a repo that
tracks commit-timeout flakes, and because the alternative was to declare it confined and stop
looking.
@ThreadSafe- measured, triaged, deliberately not adoptedRacerD infers thread-safety intent from lock use, so a class that shares state without locks is
silent rather than clean. Applied to
WorkManager,ShardManager,ProcessingShard,ConsumerManager,BrokerPollSystem,ProducerManagerandPCMetricstogether, the lane's findingcount went up roughly tenfold. Two concrete blockers:
INTERFACE_NOT_THREAD_SAFE, namingorg.slf4j.Loggerandmicrometer's
Meter- both thread-safe, neither annotated, neither ours to annotate. Every logcall site in an annotated class is one finding. The lever is Infer's
--disable-issue-type,which is a change to a hosted CI lane's checker set with its own review surface.
ShardManagermethod readingprocessingShards"racing with the write in
setProcessingShards" - a package-private setter that exists for testsand whose production callers
ShardMapIsNeverReplacedArchTestalready forbids. RacerD cannot seean ArchUnit rule, so the group is right about the seam and wrong about the code.
Group-by-group triage is in the note. Two worth naming: RacerD independently refinds
ConsumerManager.commitRequested, the wakeup counters andBrokerPollSystem.longPollTimeout- allalready in
docs/refactoring.mdvia SpotBugs, so corroboration rather than new ground - andConsumerManager.closeInProgressSignalis the@Initializercase in the flesh, a false positivethat exists only because the annotation is on.
@Locklesssees the monitor the ArchUnit rule cannot, and that was not plannedclearCommitCommand()carries a cleared suspicion dated 2026-08-31 whose last paragraph says nogate protects the invariant, because
ArchitectureTest.rebalanceCallbacksMustNotBlockmatchesmethod calls and a
synchronizedblock is aMONITORENTERit cannot see. Measured:@LocklessononPartitionsRevokedreports exactly that monitor, by name, through exactly that method - and doesNOT report
commitLock.tryLock(), so it agrees with this repo's decline-rather-than-wait rule.Still not adopted, and the reason is recorded rather than left implicit: the monitor it reports is
one the design deliberately keeps, so annotating the callback leaves a violation standing whether the
invariant holds or not - the same complaint the cleared suspicion makes of the ArchUnit rule. What it
is good for is a query: it is the only thing here that can enumerate the monitors a rebalance
callback reaches, which has now been done by hand twice (that suspicion, and the
PCMetrics.removeMetermonitor #431 found on the same path and left alone).The javadoc is amended in place with a dated paragraph rather than rewritten - the 2026-08-31
conclusion is unchanged, only what is now known about checking it again.
@NonBlocking, on the same method over the sameThread.sleep(100), reported nothing.How it is proven
config/infer-known-findings.txtfails on a new identity AND onone that stopped firing, so each retirement is made in the same commit as its change. The
register's own warning - a retirement is not by itself evidence of a fix - was honoured: the code
behind each identity was read before its line came out.
assertOnOwningThreadcalls commented out,three of
RetryQueueIteratorConfinementTest's five cases go red, and the close case fails asIllegalMonitorStateExceptionrather thanIllegalStateException- the stranded-lock failure theguard exists to replace, observed rather than argued.
LastCommitTimeFenceTestfollowsPartitionStateDirtyFlagFenceTest. No functional test can see that race - its effect is an extracommit - so without a tripwire an unrelated refactor drops the keyword and nothing goes red.
BUILD SUCCESSacross all modules.bin/check-all.sh: 16 passed, 0 failed. Inferlane green, all findings known, none new. Infer v1.3.0, JDK 17.
Test fixture, extracted rather than copied
Every confinement test needs a second thread it can name, and
ThreadConfinedConsumerTestBase'sjavadoc already records that two hand-rolled copies of that fixture had drifted - only one unwrapped
the
ExecutionException, so an assertion failing over there surfaced here as a wrapper namingneither the assertion nor the thread. The new test is in another package and would have been the
third, so the thread half is now
ForeignThreadand that base delegates to it. It addscatching,which returns what the action threw instead of rethrowing, because for a confinement test the refusal
IS the assertion. Registered in
docs/testing.md.Relationship to the two PRs beside it
#410 - this PR was stacked on it and no longer is. That PR also adds the
infer-annotationdependency and a@ThreadConfinedrule section, for its own recovery pass. ThisPR now carries both independently, written for what exists on master (
ThreadConfined.ANYandassertOnOwningThread, not the named-thread variant that arrives with the recovery pass). Whichevermerges second will conflict in
pom.xmland in the engine'sAGENTS.md, and both are one-hunkresolutions.
One thing must not be resolved mechanically: #410 carries
docs/inflight/static-infer-annotations-are-mostly-unused.md, the survey note that asked for thiswork. It is superseded by
docs/inflight/static-infer-threadsafe-is-blocked-by-third-party-interfaces.mdhere, which is thatnote rewritten around the one question the survey left open. If #410 merges
after this PR without dropping it, master ends up with both - a stale note describing work that is
done, beside the note that says so. Flagged on that PR too, so the pointer exists from both sides.
#431 is open on
RetryQueueand onrebalanceCallbacksMustNotBlock, andreaches that rule from the opposite side: it closes the rule's method-reference blind spot and
deletes six
KNOWN_BLOCKING_VIOLATIONS, while the@Locklessmeasurement here is about theMONITORENTERone, which widening the walk cannot reach. Neither makes the other wrong.docs/inflight/static-archunit-main-code-rules.mdenumerates that rule's blind spots and namesneither; #431 was rewriting that file concurrently, so the entry is left for
whoever touches it next rather than written into a conflict - stated in the note so it reads as a
decision rather than an omission.
Where the records went
parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/AGENTS.mdgains two sections:"Declare thread confinement with
@ThreadConfined, and assert it at the entry point" - includingcheck the premise before you write it, which is the
lastCommitTimelesson - and "Which ofInfer's annotations this repo uses, and which it does not".
@Initializer,@Functionaland@SynchronizedCollectionare on the second as not-yet-writable, with the reason: all three existto suppress a report, and none of those reports is raised on an unannotated class.
docs/refactoring.md: thelastCommitTimeentry is deleted (fixed, and what outlives it is in theregister and the field's javadoc);
RetryQueue.closedSTAYS on the SpotBugs list, relabelled as afalse positive that must not be "fixed" with
volatile- SpotBugs reads no confinement annotationand will keep reporting it. That finding is baselined, so nothing goes red either way.
reproduce line is there instead, and the first draft's numbers were stale before it was committed.
Added since the first review, and why the diff touches two files the summary above does not
docs/inflight/test-untracked-ci-flakes.md- CI went red onRegistrationRaceStaleResidentIT, which fails its saturation/pause-point setup guard rather thanthe Replace the existing stale container when adding work container confluentinc/parallel-consumer#909 assertion it exists for. Recorded rather than re-run silently, because this
sighting carries something the earlier ones could not: this branch ran the same content twice,
passing at
01fb0c4and failing at53ef4d6, so the same tree produced both outcomes. Everyearlier sighting was on a different branch and so could not separate branch content from the test.
Attribution was checked, not assumed: the confinement guard added here has exactly one production
caller chain, control-thread-confined inside a single method, and is not on the registration path
this test drives.
docs/refactoring.md- the defect-class sweepAGENTS.mdrequires before a fix merges, withits result. One more instance of the confined-object shape found and recorded rather than
fixed:
ProducerManager.ProducingLockwraps a read lock taken in one method and released bywhoever calls its
unlock(). The recipe is the one this PR establishes, but applying it blindwould repeat the
lastCommitTimemistake - the premise (acquirer is always releaser) has to beestablished first. For the other fix's shape,
numberOfAssignedPartitionsis the only other fieldwritten by the rebalance callbacks, and it is already tracked in that file.
docs-context-query). One conflict, on the registration-race row both sides had just edited;
resolved keeping both controls and master's new
AmbientProbeExtensionTestrow rather than takingeither side whole.
Also since the first review
RetryQueueTestBase/ForeignThreadownership - the duplication report flagged two thingsthis branch had introduced, and both are fixed.
RetryQueueIteratorConfinementTestcarried averbatim copy of
RetryQueueTest's fixture (constants, module, queue,workFor, differing only inthe topic literal), which is the parallel copy
AGENTS.mdforbids; both suites now extend one base.And the foreign-thread drift incident was told in full in two javadocs, so
ForeignThreadowns itnow and the base cross-references. Two other flagged pairs were declined with reasons in the
commit: the two fence tests share one three-line Truth assertion whose value is its message, and
the
RetryQueueLincheckTest/RetryQueueTestpair predates this branch.conflict is one row of the flake ledger, and the last one was the subtlest: both sides read
"12 seen" while counting different seventh sightings, so taking either side whole would have
looked like a no-op and dropped a real one. Resolved as a union each time, keeping master's
stronger by-construction control alongside this branch's within-branch one.
Checklist
AGENTS.md,docs/testing.md,docs/refactoring.md, the inferregister, the new note, and dated javadoc at every changed site
docs/features/-N/A - internal concurrency declarations and static-analysis records; no user-facing option or behaviour changesRetryQueueIteratorConfinementTest(5),LastCommitTimeFenceTest,ForeignThreadextracted andThreadConfinedConsumerTestBasemoved onto itdocs/inflight/working note (pr-/branch-) started at the PR's first commit -N/A - an in-flight note is the SUBJECT of this PR rather than a working note about it, and there is nothing about the branch itself that gh cannot showce-simplifyandce-code-reviewlocally -N/A - asked for @claude review this on the PR instead: it is the only route that can open the inline threads that mechanically block a merge here, and the diff is small enough that the expensive local pass buys little over it🤖 Generated with Claude Code
https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM