Repository navigation
test(core) confluentinc#909: reproduce the stale-resident race through a shard-manager seam, and let evidence justify a quarantine - #322
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
|
| PR | Base | Change | |
|---|---|---|---|
| Clones | 85 | 85 | ➖ 0 |
| Duplicated lines | 1227 | 1227 | ➖ 0 |
| Duplication | 1.08% | 1.09% | 🙂 -0.01% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 2% | ✅ Pass (1.08%) |
| Max increase vs base | +0.1% | ✅ Pass (-0.01%) |
⚠️ 1 new clones introduced
- 23 lines:
parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/utils/KafkaClientUtils.java:465<->parallel-consumer-core/src/test-integration/java/bz/stub/parallelconsumer/integrationTests/utils/KafkaClientUtils.java:384
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 |
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…umented override reachable Review findings on #322, merge-guard group: - /usr/local/bin/gh pr merge sailed past the token scan: the triple match required toks[i] == "gh" exactly, while docs/agent-harness.md names the full-path shape as one this guard exists for - and the sibling squash guard already caught it. The gh token is now matched by basename; a binary merely ending in "gh" (sleigh) still passes. - The deny message's escape hatch was dead from inside a session: it said to re-run with MERGE_DESPITE_OUTSTANDING_WORK=1, but the hook read only its own process env - the HARNESS's env, which an agent cannot reach. An env prefix typed on the merge command arrives as command TOKENS, so the scan now honors NAME=VALUE assignments immediately preceding the gh token (=0 is not an override; the same token appearing after the command is not a prefix). The process-env form is kept for a human whose shell wraps the harness. The old self-test asserted only the env form - a delivery path the harness cannot produce - so it proved the wrong thing; the token form is now the primary case. - The live-build arm had zero self-test coverage despite having already shipped one bug (pgrep matching the hook's own subshell), which was caught by accident. A fake pgrep on PATH now pins the arm in both directions: a java MavenWrapperMain under .claude/worktrees denies; a non-java process mentioning the build, and a java build outside the worktrees, do not. Each new case was proven able to fail by mutating the hook and watching it go red (docs/agent-harness.md rule 3): exact-gh match, ignored override token, dropped awk java filter, dropped worktrees grep, disabled build arm - one red case each. - The stale-task ALLOW case was coupled to the machine's real process table: a genuine build under .claude/worktrees during a self-test run flipped it to DENY. The pgrep pattern is now overridable via MERGE_OUTSTANDING_BUILD_PATTERN, and the suite isolates itself with a never-matching pattern (and unsets any inherited override variable). - ow_verdict/ow_expect duplicated verdict/expect, differing only in hook path and env delivery; verdict() now drives $HOOK_UNDER_TEST and the clones are deleted. KNOWN LIMIT now stated in the header rather than implied away: a merge wrapped in another interpreter - bash -c "gh pr merge ..." - reaches the scan as one opaque token and is not seen; the squash guard shares the gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…d, never pass it Review findings on #322, IT-hardening group. The reproduction's quiescence gate runs only BEFORE the paused registration loop is released, so a rebalance landing after it (coordinator move, session-timeout blip) would sweep the stale residents and re-deliver - healing the staged collision and letting the final invariant pass green on defective code too. The test is genuinely red against the pre-fix behaviour today; this change is anti-rot, keeping silent degradation of that proof impossible: - the rebalance state (data-topic assignment count, last rebalance event clock) is captured at release and asserted unchanged after the final invariant - a healed run now fails as "run invalid" instead of passing; - a worker whose processing-gate await times out sets a flag that is asserted false at the end: a de-saturated pipeline re-opens the take-scan (precondition 3) and silently voids the experiment. Also two instrument fixes in PausableInsertShardManager: - the 60s park deadline started at the pause point but had to outlast parking-topic creation plus the 60s rebalance-settle await (whose 5s-quiet condition resets on every rebalance event) - on a slow CI box the park could expire INSIDE the budgets it must survive, killing a valid run as a misleading "wiring failure" in a repo where a flake fails the build. Now PARK_DEADLINE_SECONDS = 150, and the test derives its worker-gate budget from the same constant so the gate always outlasts the park; - the hand-rolled interrupt-swallowing park loop duplicated LatchTestUtils.awaitLatch (swallow the wake-up interrupt, re-wait, fail loudly on deadline; already imported by other integration tests) - it now delegates to the shared helper, rethrowing as the wiring-failure error. The javadoc's citation of the deleted inflight note now points at the solutions doc that retired it (the yaml and hook-header citations moved in the preceding commit). Red-proof re-run after these changes, per docs/testing.md's rule that a modified test must be re-proven to discriminate: with ProcessingShard.addWorkContainer's staleness check neutralised (if (false)), the IT fails in 104s with the 909 signature naming exactly the collided offsets' keys k-25..k-49 as missing; restored, it passes in 14s (failsafe XML: tests=1 errors=1 -> tests=1 errors=0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
changed the title
astubbs
force-pushed
the
fix/909-load-reproduction
branch
from
August 19, 2026 12:41
58dd52d to
b0acd8a
Compare
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…d, never pass it Review findings on #322, IT-hardening group. The reproduction's quiescence gate runs only BEFORE the paused registration loop is released, so a rebalance landing after it (coordinator move, session-timeout blip) would sweep the stale residents and re-deliver - healing the staged collision and letting the final invariant pass green on defective code too. The test is genuinely red against the pre-fix behaviour today; this change is anti-rot, keeping silent degradation of that proof impossible: - the rebalance state (data-topic assignment count, last rebalance event clock) is captured at release and asserted unchanged after the final invariant - a healed run now fails as "run invalid" instead of passing; - a worker whose processing-gate await times out sets a flag that is asserted false at the end: a de-saturated pipeline re-opens the take-scan (precondition 3) and silently voids the experiment. Also two instrument fixes in PausableInsertShardManager: - the 60s park deadline started at the pause point but had to outlast parking-topic creation plus the 60s rebalance-settle await (whose 5s-quiet condition resets on every rebalance event) - on a slow CI box the park could expire INSIDE the budgets it must survive, killing a valid run as a misleading "wiring failure" in a repo where a flake fails the build. Now PARK_DEADLINE_SECONDS = 150, and the test derives its worker-gate budget from the same constant so the gate always outlasts the park; - the hand-rolled interrupt-swallowing park loop duplicated LatchTestUtils.awaitLatch (swallow the wake-up interrupt, re-wait, fail loudly on deadline; already imported by other integration tests) - it now delegates to the shared helper, rethrowing as the wiring-failure error. The javadoc's citation of the deleted inflight note now points at the solutions doc that retired it (the yaml and hook-header citations moved in the preceding commit). Red-proof re-run after these changes, per docs/testing.md's rule that a modified test must be re-proven to discriminate: with ProcessingShard.addWorkContainer's staleness check neutralised (if (false)), the IT fails in 104s with the 909 signature naming exactly the collided offsets' keys k-25..k-49 as missing; restored, it passes in 14s (failsafe XML: tests=1 errors=1 -> tests=1 errors=0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
changed the title
Owner
Author
|
@claude review this Focus on the product code — this PR was just re-cut from a 206-file branch down to the 32 files of code it was named for, so it has never been reviewed in this shape. Priorities:
|
|
Claude finished @astubbs's task in 9m 28s —— View job Review of the re-cut PR (32 files, product code + tests)
Headline finding: most of
|
Owner
Author
Babysitter pass on the review aboveVerified each finding independently against the tree rather than taking the review's word for it. Two of the four need a decision from @astubbs; the CI red does not. Headline confirmed: the
|
| Cited by #323 | On master? |
|---|---|
ProcessingShardStaleReplacement909Test |
yes |
WorkContainerFutureIsWriteOnlyArchTest |
yes |
CommitResponseTimeoutSymptomTest |
yes |
ConsumerManagerCommitRetryBudgetTest |
yes |
SubmitWorkToPoolShutdownRaceTest |
yes |
ChaosSeed |
yes |
RegistrationRaceStaleResidentIT |
no — added by this PR |
So the ordering constraint is real but reduced to a single file. If #323 landed first, only its citation of RegistrationRaceStaleResidentIT would dangle — and the reverse dangle already exists in the other direction (this PR cites 909-needs-a-saturated-pipeline-the-third-precondition-2026-08-19.md, which lives in #323). Whichever merges first, one citation dangles until the other lands; the description's "must land before" is stated more strongly than the evidence supports.
Also worth knowing: the Lgtm review satisfying the required review: human LGTM check was submitted at 07:50, before the @claude review this round that produced the findings above. The gate is deliberately not head-fresh, so it stays green regardless.
astubbs
force-pushed
the
fix/909-load-reproduction
branch
from
August 19, 2026 21:25
b0acd8a to
f042664
Compare
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…d, never pass it Review findings on #322, IT-hardening group. The reproduction's quiescence gate runs only BEFORE the paused registration loop is released, so a rebalance landing after it (coordinator move, session-timeout blip) would sweep the stale residents and re-deliver - healing the staged collision and letting the final invariant pass green on defective code too. The test is genuinely red against the pre-fix behaviour today; this change is anti-rot, keeping silent degradation of that proof impossible: - the rebalance state (data-topic assignment count, last rebalance event clock) is captured at release and asserted unchanged after the final invariant - a healed run now fails as "run invalid" instead of passing; - a worker whose processing-gate await times out sets a flag that is asserted false at the end: a de-saturated pipeline re-opens the take-scan (precondition 3) and silently voids the experiment. Also two instrument fixes in PausableInsertShardManager: - the 60s park deadline started at the pause point but had to outlast parking-topic creation plus the 60s rebalance-settle await (whose 5s-quiet condition resets on every rebalance event) - on a slow CI box the park could expire INSIDE the budgets it must survive, killing a valid run as a misleading "wiring failure" in a repo where a flake fails the build. Now PARK_DEADLINE_SECONDS = 150, and the test derives its worker-gate budget from the same constant so the gate always outlasts the park; - the hand-rolled interrupt-swallowing park loop duplicated LatchTestUtils.awaitLatch (swallow the wake-up interrupt, re-wait, fail loudly on deadline; already imported by other integration tests) - it now delegates to the shared helper, rethrowing as the wiring-failure error. The javadoc's citation of the deleted inflight note now points at the solutions doc that retired it (the yaml and hook-header citations moved in the preceding commit). Red-proof re-run after these changes, per docs/testing.md's rule that a modified test must be re-proven to discriminate: with ProcessingShard.addWorkContainer's staleness check neutralised (if (false)), the IT fails in 104s with the 909 signature naming exactly the collided offsets' keys k-25..k-49 as missing; restored, it passes in 14s (failsafe XML: tests=1 errors=1 -> tests=1 errors=0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…, not a family member `PCMetricsTest.metricsRegisterBinding` was recorded as the family's signature on 2026-08-14. It is not. The test asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter while the suite runs UNORDERED: commits are contiguous and bounded by the lowest incomplete offset, completions are not ordered, and workers latch BEFORE incrementing, so a latched worker's offset never completes and the gap is permanent. No timeout closes it. That mechanism answers the question the entry itself left open - it asked for a full-suite run "repeated enough times to put a number on the rate" - and answers it better than a rate would. It explains why only loaded runs fail (concurrency is what produces out-of-order completion), why isolated runs pass, and both observed gaps: 2 records here, 7 on the later #322 sighting. Caught by a reviewing agent as a self-contradiction: this branch simultaneously recorded the test as an 857 sighting here and ruled it OUT of the family in bug-pcmetrics-committed-offset-vs-completion-count.md. The ruling-out is the correct half. The original assessment is retained rather than deleted - this file already records one contamination of exactly this kind (a transactional-mode failure logged as confirming a cycle impossible in that mode), and the reasoning that led here is worth seeing.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…r they need #322 carries ten unrelated workstreams over 66 commits and 206 files; the confluentinc#909 reproduction it is named for is 13 commits and 20 files of that. Records the feasibility MEASUREMENT rather than an estimate: each group cherry-picked onto master in a throwaway worktree. Four groups replay fully clean (CI gates 8/8, #209 4/4, the confluentinc#857 ledger 5/5, test logging 1/1); the harness+tracker group blocks after one commit. The governing constraint is that cherry-pick replays a commit's ORIGINAL diff. Master gained bin/test-check-agent-hooks.sh from #299 after these commits were written, and this branch's one merge already resolved that overlap - replay throws the resolution away and re-fights it. Merging master again cannot help, and there is nothing to merge: the branch is 0 behind. What the branch being current DOES buy is that `git diff master...HEAD` describes the resolved state and therefore applies to master by construction - verified, 35/35 exclusive files clean. Hence two methods (replay where clean, diff-extraction where not) and, more importantly, a STACK rather than ten independent branches: PR 2 adds tag headers to ~70 existing inflight notes while later groups edit those same notes' bodies, so extracting independently turns every one of those into a conflict, while extracting in order applies them on top. Also corrects a grouping error - "agent hooks" and "inflight tracker" are one workstream, not two; the merge guard and the session index are the same hook system. Adds a tenth PR that is new work rather than an extraction: a conflict-marker gate, prompted by finding 305 lines of docs/inflight/bug-857-family.md committed and pushed inside an unresolved conflict on the #29 branch, with no gate in bin/ looking for markers. No history is rewritten by this commit. Re-cutting #322 itself is a force-push to a pushed branch and is left as the owner's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
changed the title
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…at lands one PR up A simplify-and-verify pass over this PR's own diff (#323), applying the repo's never-state-a-fact-twice rule and repairing the citations the file-reference gate was failing on. - bug-857-family.md: the seventh sighting's SUPERSEDED block restated the full mechanism that bug-pcmetrics-committed-offset-vs-completion-count.md owns (UNORDERED vs completion counter, latch-before-increment, the permanent gap). Trimmed to the verdict plus a pointer naming the owner; the family-specific reasoning (do not count it as a sighting, the contamination precedent) is untouched. The mechanism itself was verified against PCMetricsTest and AbstractParallelEoSStreamProcessorTestBase before trimming - the supersession is correct. - check-merge-outstanding-work.sh is delivered by #324 (the tooling half of the #322 split), not by #322 as next-agent-self-review-as-blocking-pr-comments.md claimed - verified against both PRs' file lists. Corrected the attribution, added the same provenance where agent-harness.md and compound-engineering.md describe the hook, and closed the three citing paragraphs with file-refs N/A markers naming #324, which is the gate's own prescribed repair for a file that lives on another branch. bin/check-file-refs.sh was red on exactly these three citations (the PR Checklist lane's only real failure) and is green after. - compound-engineering.md: "all 71 notes" was already off by one (72 tagged notes on this branch) and drifts every time a note is added or closed - reworded to "every open note", which stays true. - bug-retry-queue-orphaned-by-inline-stale-removal.md: a [[wiki-style]] link nothing renders, rewritten as the markdown link every other note uses. Verified locally: bin/check-file-refs.sh and bin/check-issue-refs.sh both pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
Merged
4 of 6 tasks
Owner
Author
Automated review (ce-code-review, 7 lenses + validation pass) - 5 findings, verdict: Ready with fixesRoster: correctness, adversarial, testing, project-standards, maintainability, previous-comments, learnings-researcher; the 3 actionable findings below were re-verified by an independent validation pass against the worktree. Cross-model pass not run (no different-family CLI on this host). Apply queue (mechanical, fixes proposed):
Decisions for the author:
Notable non-findings, checked and dismissed: the DI seam's Residual risks worth a glance: |
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…lives only in a conversation Three PRs are open at once and their open items were being carried in chat. Each note records what is still open on that PR, what was already fixed so it is not re-litigated, and what nobody has verified. The item most at risk of being lost is #322's: `RegistrationRaceStaleResidentIT` rules out the two known heal paths but never observes the collision itself. The uncovered path is `ProcessingShard`'s take-scan stale eviction, suppressed today only by `WorkManager`'s `delta < 1` guard - if that arithmetic shifts, the reproduction goes permanently green with the defect branch unexercised. Proposed fix: count stale-resident collisions at insert and assert exactly 25, so the test asserts the mechanism rather than the absence of two known escapes. Needs a broker, so it is not applied. #323's note records that this PR breaks the session index until #324 lands: master's hook keys on `inflight-priority` and has zero references to `inflight-impact`, and this PR removes those markers, so the "open work" block goes silently empty in the window between them. Two ways out are written down; the decision is open. #324's note keeps the deliberately-red `test-check-agent-hooks.sh` visible - it asserts against the LIVE inflight corpus rather than a fixture, so it needs #323's notes - along with the over-triggering pre-commit hook and the five commits whose bodies could not be replayed. Written on this branch rather than #324 because they cite no not-yet-landed script. Note that `bin/check-inflight-tags.sh` cannot even be RUN from here to validate their tags - it lands one PR up - which is the window these notes describe, met in practice while writing them.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…, not a family member `PCMetricsTest.metricsRegisterBinding` was recorded as the family's signature on 2026-08-14. It is not. The test asserts PARTITION_LAST_COMMITTED_OFFSET equals a COMPLETION counter while the suite runs UNORDERED: commits are contiguous and bounded by the lowest incomplete offset, completions are not ordered, and workers latch BEFORE incrementing, so a latched worker's offset never completes and the gap is permanent. No timeout closes it. That mechanism answers the question the entry itself left open - it asked for a full-suite run "repeated enough times to put a number on the rate" - and answers it better than a rate would. It explains why only loaded runs fail (concurrency is what produces out-of-order completion), why isolated runs pass, and both observed gaps: 2 records here, 7 on the later #322 sighting. Caught by a reviewing agent as a self-contradiction: this branch simultaneously recorded the test as an 857 sighting here and ruled it OUT of the family in bug-pcmetrics-committed-offset-vs-completion-count.md. The ruling-out is the correct half. The original assessment is retained rather than deleted - this file already records one contamination of exactly this kind (a transactional-mode failure logged as confirming a cycle impossible in that mode), and the reasoning that led here is worth seeing.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…at lands one PR up A simplify-and-verify pass over this PR's own diff (#323), applying the repo's never-state-a-fact-twice rule and repairing the citations the file-reference gate was failing on. - bug-857-family.md: the seventh sighting's SUPERSEDED block restated the full mechanism that bug-pcmetrics-committed-offset-vs-completion-count.md owns (UNORDERED vs completion counter, latch-before-increment, the permanent gap). Trimmed to the verdict plus a pointer naming the owner; the family-specific reasoning (do not count it as a sighting, the contamination precedent) is untouched. The mechanism itself was verified against PCMetricsTest and AbstractParallelEoSStreamProcessorTestBase before trimming - the supersession is correct. - check-merge-outstanding-work.sh is delivered by #324 (the tooling half of the #322 split), not by #322 as next-agent-self-review-as-blocking-pr-comments.md claimed - verified against both PRs' file lists. Corrected the attribution, added the same provenance where agent-harness.md and compound-engineering.md describe the hook, and closed the three citing paragraphs with file-refs N/A markers naming #324, which is the gate's own prescribed repair for a file that lives on another branch. bin/check-file-refs.sh was red on exactly these three citations (the PR Checklist lane's only real failure) and is green after. - compound-engineering.md: "all 71 notes" was already off by one (72 tagged notes on this branch) and drifts every time a note is added or closed - reworded to "every open note", which stays true. - bug-retry-queue-orphaned-by-inline-stale-removal.md: a [[wiki-style]] link nothing renders, rewritten as the markdown link every other note uses. Verified locally: bin/check-file-refs.sh and bin/check-issue-refs.sh both pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…lives only in a conversation Three PRs are open at once and their open items were being carried in chat. Each note records what is still open on that PR, what was already fixed so it is not re-litigated, and what nobody has verified. The item most at risk of being lost is #322's: `RegistrationRaceStaleResidentIT` rules out the two known heal paths but never observes the collision itself. The uncovered path is `ProcessingShard`'s take-scan stale eviction, suppressed today only by `WorkManager`'s `delta < 1` guard - if that arithmetic shifts, the reproduction goes permanently green with the defect branch unexercised. Proposed fix: count stale-resident collisions at insert and assert exactly 25, so the test asserts the mechanism rather than the absence of two known escapes. Needs a broker, so it is not applied. #323's note records that this PR breaks the session index until #324 lands: master's hook keys on `inflight-priority` and has zero references to `inflight-impact`, and this PR removes those markers, so the "open work" block goes silently empty in the window between them. Two ways out are written down; the decision is open. #324's note keeps the deliberately-red `test-check-agent-hooks.sh` visible - it asserts against the LIVE inflight corpus rather than a fixture, so it needs #323's notes - along with the over-triggering pre-commit hook and the five commits whose bodies could not be replayed. Written on this branch rather than #324 because they cite no not-yet-landed script. Note that `bin/check-inflight-tags.sh` cannot even be RUN from here to validate their tags - it lands one PR up - which is the window these notes describe, met in practice while writing them.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…h, and retag two The three per-PR notes were all written here, which is wrong in a way that breaks a hook rather than merely being untidy: `check-merge-outstanding-work.sh` looks for `docs/inflight/pr-<N>-*.md` in the tree of the branch being merged. With #322's note living only here, merging #322 would find nothing and the guard would pass silently - the exact failure it exists to prevent. Each note now travels with its PR; this one keeps its own. Also retags two on review: - `perf-comparison-matrix` was `task`/`process`. Its own subject is the README performance test running in each proxy language - the double-click demo wanted before #293 - which is something a user watches. `feature`. - `process-quarantine-lane-foss` was `task`/`process`. It proposes extracting the quarantine lane as a separate published project; that is a thing to build, not a way of working. `feature`. `process-candidate-ranking` is left as `task`/`process` deliberately, against the suggestion of `release-gate`. Nothing in it blocks publishing - its own text has three mentions of ranking and one of release - and `release-gate` means "blocks a release" rather than "decides what goes in one". The objection that `task` reads as a single discrete action is fair and unresolved: it is a standing register, and the vocabulary has no type for that.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…survives its own PR check-branch-self-reference.sh flagged the migrated note for naming #322 in the present: the incident it records happened "on a branch during the #322 split", which is a claim about an arrangement that ends when this merges. The sentence now dates the incident and says the branch is gone and the PR has landed, so the citation records history rather than a live arrangement. Local runs of that gate cannot see this: it resolves the PR with `gh pr list --head <branch>`, and this worktree's local branch (merge/322-master-3) is not the PR's head ref (fix/909-load-reproduction), so the PR arm never armed and only the CI run had the number. Run it as `PR_NUMBER=322 bin/check-branch-self-reference.sh` from a worktree whose branch name differs from the PR head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…t a CI pass The gate resolves "this PR" with `gh pr list --head <branch>`, so in a worktree whose local branch is not the PR's head ref - a `merge/NNN-...` branch pushing to `fix/...`, which is how master merges are staged in this repo - the lookup returns nothing, the PR arm never arms, and every `#NNN` mention passes locally. CI sets GITHUB_HEAD_REF, arms it, and fails. That happened on this PR one push before this commit: a migrated note naming #322 was clean under a full local `bin/check-all.sh` sweep and red in Repo Hygiene. Nothing can detect this from inside the run - an empty PR lookup is indistinguishable from "no PR yet", which is a legitimate local state the gate deliberately supports. So the header now names the workaround, `PR_NUMBER=<n> bin/check-branch-self-reference.sh`, where a reader hits the trap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
marked this pull request as ready for review
August 26, 2026 12:30
5 tasks done
…ule, like every other collaborator `PCModule` already provides `workManager`, `brokerPoller`, `producerManager` and `consumerManager`; its own javadoc calls it the DI system, and `PCModuleTestEnv` is the established seam for substituting a collaborator in tests. `ShardManager` was the exception, constructed inline by `WorkManager`, so it was the one collaborator no test could substitute - which is why the broker-level confluentinc#909 reproduction later in this branch could not be written without it. This completes the existing pattern rather than adding a hook for the test that needs it: no test-only branch, no flag, no latch in production. The provider takes the owning `WorkManager` as a parameter because the two are mutually dependent and `WorkManager`'s constructor is mid-flight when it resolves this - `workManager()` would recurse. Not a new design. `d7eb0a3a03` ("fix: module: resolve recursive dependency", 2022-09-05, on `origin/improvements/remove-static`) makes the identical change for the same stated reason, so this is a rediscovery of a call already made once, four years ago, on a branch that never landed. The caching is a plain null-check rather than anything thread-safe, sufficient because `WorkManager` has exactly one construction site and construction is single-threaded. It does mean the method silently discards its argument once memoised - the hazard the next commit closes. Verified not to disturb anything: full core unit suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rather than sharing them A `PCModule` holds ONE parallel consumer's collaborator graph. Two of its getters memoise a collaborator while taking that collaborator's owner as a method parameter, so on the cache-hit path the argument is there to be ignored - and ignoring it hands a second owner an object still wired to the first. Neither failure surfaces where it is caused, and both are now rejected with an `IllegalStateException` that names the fix. `shardManager(WorkManager)`, introduced by the preceding commit, is the first. A second `WorkManager` built against one module would have been handed the first's `ShardManager`: its `PartitionStateManager` and its shard manager would then operate on different owners, and nothing would go red. Before that seam existed each `WorkManager` constructed its own, so the hazard is new with the seam. It is reachable through the public `WorkManager` constructor and `PCModule.setWorkManager`. `brokerPoller(AbstractParallelEoSStreamProcessor)` is the second, found by sweeping `PCModule` for the same shape once the class of defect was named rather than the symptom. A second processor would have received a `BrokerPollSystem` polling a broker on behalf of the first - registering the first's metrics gauges, delivering the newcomer nothing. Its only caller today is `AbstractParallelEoSStreamProcessor`'s constructor, once per module, so the collision is unreachable - but that is a property of the callers, not of the method, and "reachable only because callers behave" is the argument this commit rejects. BOTH GETTERS ARE NOW FINAL, and substitution goes through `createShardManager(WorkManager)`. This is not tidiness. The one place in the tree that substitutes a `ShardManager` - the confluentinc#909 reproduction two commits along - does it by subclassing the module, and written against the memoising getter it overrode the memoisation and the guard away with it, reinstating the exact defect in the same branch that guards against it. That is what the sweep found. Splitting the factory out puts the memoisation and the guard where an override cannot reach them, so the compiler now refuses the mistake instead of a reviewer having to catch it. Rejected alternatives: rebuilding the cache for the newcomer, which breaks the module's one-instance-per-collaborator contract and leaves the first owner silently orphaned; and leaving it with a javadoc note, which is what the seam commit originally parked in `docs/refactoring.md` - a "safe only while nothing does X" caveat wants a check that fails, not a paragraph. `PCModuleCollaboratorOwnershipTest` covers both sides of each guard plus the factory seam. It is named for the defect CLASS, not for one instance, so the next memoised-with-owner collaborator gets a test rather than its own file. Mutation-checked: deleting either guard turns exactly its own test red and leaves the rest green. Swept and dismissed, tree-wide, for "a memoised collaborator that ignores its owner argument": `producerWrap()`, `producerManager()`, `consumerManager()`, `workManager()`, `pc()`, `dynamicExtraLoadFactor()` and `pcMetrics()` are memoised but take no owner argument; every `computeIfAbsent`/`putIfAbsent` factory in the tree is key-scoped and captures no owner-like parameter; `SupplierUtils.memoize`'s only production use is instance-scoped inside the owner's own constructor; there are no `@Getter(lazy=true)` fields and no DI holder other than `PCModule`. Still open, deliberately out of scope: `PCModule.setParallelEoSStreamProcessor` is the mirror image - a second processor's constructor overwrites the module's `pc` reference BEFORE the `brokerPoller` guard fires, so the rejection path leaves the module pointing at the half-built newcomer. That is pre-existing behaviour and a separate change. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the code does The comment on `KeyOrderedKey` claimed "we use just the topic name here, and not the partition", and the field was named `topicName` to match. Both were wrong: the constructor passes `new TopicPartition(rec.topic(), rec.partition())`, so the shard has always been identified by (topic, partition, key). The CODE is right and the doc was the lie, which is the dangerous direction - anyone acting on the comment would "fix" the constructor to drop the partition and introduce silent data loss. `ProcessingShard`'s `entries` map is keyed by OFFSET, and offsets are unique only WITHIN a partition, so two records sharing a key on different partitions would land in one map under one key and one would vanish without a trace. The javadoc now says why partition scoping is load-bearing rather than merely asserting it, and the field is renamed to `topicPartition`. Compat-safe: the Lombok-generated `getTopicName()` has no callers anywhere in the tree, and 0.6.0.0 is unreleased and is itself the release introducing the `bz.stub.*` coordinates, so no shipped artifact ever exposed the old getter under this package. Retires the corresponding `docs/refactoring.md` entry in the same commit - an entry and the change that retires it are one change, or the backlog outlives the work it describes. Found while reviewing #31; pre-existing and independent of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n, and the third precondition it uncovered #31 shipped the fix with unit-level proof and an openly declared gap: no load-level evidence. This closes it with something stronger than a soak - a broker-real reproduction that discriminates every run, with no seed and no sleeps. Building it uncovered why nothing had reproduced the defect before. The loss needs THREE coincidences, not the two previously recorded: a rebalance inside the registration insert loop, the partition returning so the uncommitted tail re-delivers, and - the missing one - NO take-scan of the shard in between. `ProcessingShard.getWorkIfAvailable` lazily evicts stale containers as it scans, so an ordinary take-scan HEALS the collision before a fresh arrival can hit it. Only a consumer whose pipeline is at target keeps residents alive long enough to eat a fresh record. That explains the standing puzzle: the Chaos Pain Suite is green on defective master not through an unlucky seed but because its own churn is the healing mechanism. Random reproduction was fighting itself. It also narrows the production risk - this bites BUSY consumers, not idle ones. The test drives all three deliberately: a gated user function saturates the pool so the control loop's delta stays at or below zero, a fixed message-buffer size pins the load factor so the target cannot step mid-experiment, a second group member on another topic forces an eager revoke/re-assign, and the insert is parked mid-loop until re-delivery has fully registered. Every await is event-gated - latches, listener callbacks, instrument counters - so a failure is loud rather than slow. **Proven both ways, and re-proven after every change to it.** With `addWorkContainer`'s stale-replacement branch disabled - the pre-fix drop-on-collision behaviour, a local-only edit since reverted - the IT fails after the 90s invariant await on the confluentinc#909-signature assertion, "a fresh arrival colliding with a stale shard resident was dropped", and NOT on any run-invalid guard. Green arm passes in ~14.5s. Run before and after the review passes, per docs/testing.md's rule that a modified test must be re-proven to discriminate. **A healed run fails as invalid rather than passing.** The quiescence gate runs only BEFORE the paused loop is released, so a rebalance landing after it (coordinator move, session-timeout blip) would sweep the residents and heal the staged collision. Four run-invalid guards close that: rebalance state captured at release and asserted unchanged; a worker leaving the processing gate early - timeout or interrupt - sets a flag asserted false at the end, because a de-saturated pipeline re-opens the take-scan; and `getNumberRecordsOutForProcessing() == MESSAGE_BUFFER_SIZE` asserted both in the saturation await and again at gate-open, since that count is the actual term in the `delta = target - outForProcessing` arithmetic the suppression rests on. A future shift in the load factor now fails loudly instead of leaving the test green with the defect branch unexercised. `PausableInsertShardManager` parks the CONTROL thread at the head of exactly one designated insert, after that batch has passed the `epochIsStale` guard. Three things about it are load-bearing and were each got wrong once: the park must dwell THROUGH interrupts, because PC wakes its control thread by interrupting it, so it delegates to the shared `LatchTestUtils.awaitLatch` rather than a hand-rolled loop; `PARK_DEADLINE_SECONDS` is 150 rather than 60 because the park must outlast parking-topic creation plus a rebalance-settle await whose quiet condition resets on every rebalance event, and expiring inside those budgets would kill a valid run as a misleading "wiring failure" in a repo where a flake fails the build; and `freshEpochInsertCount` increments AFTER the insert, not before, so the counter means "registered" - which is what the test's gate-open condition, polled from the awaitility thread, already claimed. THE WORKER GATE IS ANCHORED TO THE CLASS BUDGET, not derived from an event that has not happened yet. The deadline was `PARK_DEADLINE_SECONDS + 60` = 210s, but a worker enters the gate in step 1 and the park does not start until step 2; everything staged between gate entry and gate open can spend 30 + 30 + 60 + 30 = 150s of awaits plus the final 90s invariant await, so on a slow broker a run still inside the 300s class budget could expire the gate, de-saturate the pipeline and fail as "run invalid" - a flake, and this build has no retry. It is now deliberately longer than the class budget, so `@Timeout` is the binding deadline and a too-slow run says so instead of blaming de-saturation. THE PRODUCE HELPER IS SHARED. `produceKeyedRange` was a private second implementation of produce-and-track, which docs/testing.md forbids: producer setup, ack waiting and error handling would have had to be fixed in two places. The deferral once recorded for this rested on needing a broker run over every existing caller - but the fix is an ADDITIVE overload, so no existing caller moves and that argument does not hold. `KafkaClientUtils.produceOffsetKeyedRange` now owns it, and asserts the key<->offset correspondence its javadoc claims rather than assuming it: a topic that cannot honour the contract fails at the produce instead of as a confusing missing-record assertion later. TEARDOWN ASSERTS THE SECOND MEMBER STOPPED. Its running flag is only read between polls, so a poll or close already in flight could outlast the 10s join, leaving a live non-daemon thread and a live group member to rebalance into whichever broker test ran next. Teardown now uses `wakeup()`, the one cross-thread-safe `KafkaConsumer` call, waits long enough to cover a slow close, and ASSERTS the thread stopped - the failure belongs here, not in the next test. Untagged, so it runs in the ordinary failsafe lane: it is deterministic, not chaos. **What it still cannot do**: it rules out both KNOWN heal paths but never OBSERVES the collision, so an unknown heal path that evicts residents without de-saturating would turn it silently green. That is deferred with its evidence and its design in `docs/inflight/test-909-reproduction-cannot-observe-the-collision.md` - including the finding that the fix needs no production change - because confirming the count needs a broker. The note states the acceptance shape rather than a hardcoded collision count, so retuning either constant cannot leave an obsolete literal reading as the agreed value. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ning Rule 1 was "no quarantine without diagnosis". It conflated two different unknowns - "we do not know the mechanism" and "we do not know whether it is ours" - and only the second justifies blocking. A test with a recorded sighting ledger IS a finding: it is known master-state flaky. It just has no root cause yet. The cost of the old wording was that an undiagnosed red stayed in the gating lane and blocked every unrelated PR, which trains everyone to read red as normal. This repo already deleted surefire retries for hiding flakes; a permanently-red gate destroys the same signal more thoroughly. The tell that the default was miscalibrated: its escape hatch was an owner-granted exception, and the exception had become the routine path - `test-untracked-ci-flakes.md` records the owner having to grant one for a test failing 4 in 45 runs, which is a ledger by any reading. Rule 1 now asks for EVIDENCE: a diagnosed mechanism, or a recorded sighting ledger (dates, runs, signature, and what makes it master-state rather than PR-state). What stays banned is a hunch - "it is red sometimes" is not evidence, and a single failure is a sighting, not a ledger. Nothing else moves: the lane still runs the test, the registry still keeps it loud, rule 5 still blocks a release while the list is non-empty. Quarantine defers; it does not forgive. It is explicitly not a licence to label something "just a flaky test" - the label the drain-zombie carried right up until it turned out to be a real product bug. The rule's other five statements - `docs/quarantined-tests.md` (its owner, and the fullest statement), `docs/testing.md`, `AGENTS.md`'s table cell, and `bin/check-quarantine-registry.sh`'s drift message - landed with #323, which merged first. What remains here is the annotation that asserts the rule and the contract test that holds it to it, so the javadoc and the registry it cites no longer contradict each other on master. It lands against two live registry entries - `PCMetricsTest.metricsRegisterBinding` and `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` - both of which already carry a diagnosis and so satisfy the stricter half of the rule either way. **The bar is human-reviewed, deliberately, and nothing mechanical enforces it**: the annotation declares required fields plus an audit summary, and `flaky - failed once` satisfies a non-blank check. Encoding the ledger/mechanism distinction over free text is a policy decision rather than a review fix, and is left open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…buried `docs/inflight/AGENTS.md` said "work your current PR resolves is tracked by that PR - delete its file in that PR", and the root router said "a file is deleted when its work lands". Read together those are a blanket instruction to destroy the note at merge time, and that is how they were read: this PR's own note carried a finding with no durable home anywhere else, scheduled for deletion by the rule. The intent was only ever to keep the notes in touch with the code's reality. So the rule now names four outcomes and puts deletion last: - migrate what outlives the work to its durable owner FIRST (`docs/refactoring.md`, `docs/solutions/`, `CONCEPTS.md`, the topic doc); - keep the note when live content remains, shrunk and renamed if its area changed; - split when what remains is a different item - a new note, and `git rm` the old; - `git rm` only when nothing left in it is both true and unowned elsewhere. The router's `docs/inflight/` row additionally listed `parked-` and `next-` as filename prefixes. Both were removed precisely because a filename must not carry state - all 23 `next-` files were open, so the prefix ranked nothing while implying a ranking. The row now points at the owner instead of keeping a copy that had already drifted away from it. APPLIED TO THIS PR'S OWN NOTES rather than only written down, since a rule that is merely stated is the rule that gets missed. #322 is the third and last of the split, so both `pr-322-*` notes are the new rule's first customers - and left on master they would present finished PR coordination as open work in every session index. MIGRATED, THEN DELETED: * the proposed `bin/check-conflict-markers.sh` gate, and the merge that shipped 305 lines of an unresolved conflict with nothing red, became `ci-conflict-marker-gate.md` - the incident is the part that would have been lost, which is why it is a note and not a `refactoring.md` line. Its incident sentence is dated and says the branch is gone and the PR landed, so the citation records history rather than a live arrangement that ends at merge; * the five stranded agent-hook commit bodies were already owned by `branch-agent-hook-commit-bodies-only-on-backup.md`, so only the second copy went; * "re-fetch master before measuring anything" is already owned by `docs/merge-checklist.md`; * the `awaitLatch` re-arm worst case moved into `PausableInsertShardManager`'s own javadoc, beside the constant it is about. PROMOTED OUT OF `docs/refactoring.md`. The `availableWorkContainerCnt` undercount carried evidence, an impact assessment and a deferral, which is a note's job - that file is a line or two, no owner, no state. It is now `bug-processing-shard-available-work-undercount.md`, and asks the prior question before the obvious fix: whether a counter nothing reads for correctness should exist at all. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… number now demands `bin/check-branch-self-reference.sh` (#341-era hygiene work, `da91f3f61`) arrived on this branch with the master merge. It fails when a `docs/inflight/` note mentions THIS branch or THIS PR without a human attesting the sentence still reads correctly AFTER the merge, and it reads whole files for the `#NNN` and branch-name arms, because your own PR number can only ever be about you. Run the way `repo-hygiene.yml` runs it - `PR_NUMBER=322 GITHUB_HEAD_REF=fix/909-load-reproduction` - it reported 17 unconfirmed self-references across eight notes. Six of those notes are master's, not this branch's; they mention #322, which is what makes them this merge's problem and nobody else's. TWO SENTENCES WERE ACTUALLY WRONG POST-MERGE, and both are rewritten rather than attested: - `pr-blockers-and-collisions.md` recorded the merge order AND a tally of which of the seven had merged. The tally goes stale at every merge in its own list - #322's included - so it is deleted rather than restated, and the reader is sent to `gh pr list -R astubbs/parallel-consumer`. The ORDER stays: it is the standing coordination fact, and it survives only here because #323's merge deleted the note that used to hold it. - `ci-agent-self-review-as-blocking-pr-comments.md` said the fix and its evidence "are in two PRs (#322)" - one number for a claim about two - and twice said the hook "lands with #324", which merged on 2026-08-20. Both are named in full and put in the tense they will be read in. THE REST ARE ATTESTATIONS, and each says why it holds: they cite #322 for what was OBSERVED or DECIDED on it - a sighting, a failed replay, a reproduction, a review finding - which is a claim about the past that a merge cannot falsify. `bug-pcmetrics-...md` cites the branch by name for a CI run; the run id resolves after the branch is deleted, which is what makes that line safe where the branch name alone would not be. Marker placement is not free-form: the gate accepts a marker on the flagged line, on the line above, or inside a `checked-begin`/`checked-end` block. Inside an ordered list or mid-paragraph both of the first two split the markdown, so those lines carry the marker inline at end-of-line and whole paragraphs take the block form - the shape `test-back-pressure-engage-point-is-unasserted.md` already uses. No note's meaning was weakened to pass the gate, and nothing was marked that this merge does not own. WHY A FULL LOCAL SWEEP MISSED THEM, recorded in the gate's own header where a reader hits the trap. The gate resolves "this PR" with `gh pr list --head <branch>`, so in a worktree whose local branch is not the PR's head ref - a `merge/NNN-...` branch pushing to `fix/...`, which is how master merges are staged in this repo - the lookup returns nothing, the PR arm never arms, and every `#NNN` mention passes locally. CI sets `GITHUB_HEAD_REF`, arms it, and fails; that happened on this PR, with a migrated note clean under a full local `bin/check-all.sh` sweep and red in Repo Hygiene. Nothing can detect it from inside the run - an empty PR lookup is indistinguishable from "no PR yet", which is a legitimate local state the gate deliberately supports - so the header now names the workaround, `PR_NUMBER=<n> bin/check-branch-self-reference.sh`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… than argued Whether the property propagates into FAILSAFE forks as well as surefire ones was an open claim. Structurally it should - both plugins run off the same fork machinery and neither declares `systemPropertyVariables` - but that is an argument, and this repo does not retire a claim on one. Observed instead, riding on a run that had to happen anyway: verifying `RegistrationRaceStaleResidentIT` against a real broker with `-Dpc.log.level=info` emitted `bz.stub.parallelconsumer` INFO lines that the warn default swallows. `bin/check-test-log-config.sh`'s own header owns the how-to for this property, so the observation and the integration-test invocation it licenses go there, next to the surefire one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
force-pushed
the
fix/909-load-reproduction
branch
from
August 26, 2026 13:05
ecc9d35 to
3dec5cd
Compare
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
The guard this PR adds binds a module's memoised collaborators to one owner. It cannot cover `setParallelEoSStreamProcessor`, because that setter runs earlier in the same constructor: a second processor overwrites the module's `pc` reference and is only then refused, so the rejection path leaves the module pointing at a half-built newcomer. On that one path the failure is worse after the guard than before it, which is exactly the kind of thing that must not be left only in a PR comment. Pre-existing and a main-code behaviour change on a constructor path, so it is out of scope for a PR whose subject is a broker-level race reproduction - but an unrecorded defect is one the next session rediscovers, or ships on top of, which is what this directory exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…ch stops existing The note described the guard in the present tense of its own PR - "the guard #322 adds", "why it was not fixed in #322" - which is false the moment the PR merges and the branch is gone. `bin/check-branch-self-reference.sh` caught both lines, and it was right: a note that is accurate only while its PR is open is a note that becomes wrong at the exact moment nobody is looking at it. Rewritten as it will read afterwards, and attested, so the gate can tell a checked line from an unchecked one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review Re-requesting: since your last pass this branch was re-cut (13 commits into 8) and gained main code Please focus on:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…rom ones I invented `inflight-impact: corruption` and `inflight-labels: lifecycle` are both outside their closed sets, so `check-inflight-tags.sh` failed the hygiene lane. Neither was a near miss worth widening a set for: - **impact is now `misdirection`.** The cost of not knowing is that the module answers later lookups with the wrong processor - the first one's collaborators end up addressing the second. That is what the axis means by misdirection, and it is more precise than "corruption", which described the mechanism rather than what it costs a reader. - **the label is dropped rather than replaced.** `concurrency` is the only value the set holds, and this is constructor ordering, not a race - a second processor on one module is misuse, not interleaving. Labels are optional, and a wrong one is worse than none: the set is the index, so a mislabelled note is found by the wrong search. Widening the set means editing `bin/lib/inflight-tags.sh` and `docs/inflight/AGENTS.md` together, which is a policy change and not something to slip into a PR about a race reproduction. `bin/check-all.sh` with the PR arm armed is now 15/15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The owner guard this PR adds binds a module's memoised collaborators to one owner. It cannot cover `setParallelEoSStreamProcessor`, because that setter runs earlier in the same constructor: `AbstractParallelEoSStreamProcessor`'s constructor calls `module.setParallelEoSStreamProcessor(this)` before it resolves `module.brokerPoller(this)`, so a second processor built against one module overwrites the module's `pc` reference **first, and is only then rejected**. The exception leaves the module pointing at a half-built newcomer that nobody owns - on that one path the failure is worse after the guard than before it, which is exactly the kind of thing that must not be left only in a PR comment. Recorded rather than fixed: it is pre-existing, and closing it is a main-code behaviour change on a constructor path, outside a change whose subject is a broker-level race reproduction. An unrecorded defect is one the next session rediscovers, or ships on top of, which is what this directory exists to prevent. The note is written as it will read once this branch is gone, not in the present tense of its own PR - a note that is accurate only while its PR is open becomes wrong at the moment nobody is looking at it - and the post-merge lines are attested so the gate can tell a checked line from an unchecked one. Tagged `inflight-impact: misdirection`: the cost of not knowing is that the module answers later lookups with the wrong processor, so the first processor's collaborators end up addressing the second. That is more precise than describing the mechanism as corruption. **No label**, deliberately: `concurrency` is the only value the closed set holds and this is constructor ordering, not a race - a second processor on one module is misuse, not interleaving. Labels are optional and a wrong one is worse than none, because the set is the index, so a mislabelled note is found by the wrong search. Widening the set means editing `bin/lib/inflight-tags.sh` and `docs/inflight/AGENTS.md` together, which is a policy change and not something to slip into a PR about a race reproduction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
force-pushed
the
fix/909-load-reproduction
branch
from
August 26, 2026 14:12
903f4b1 to
db3f84c
Compare
Owner
Author
|
@codex review Head moved again: The eight commits below it ( Note your last two invocations on this PR returned a usage-limit message rather than a review, so as far as this thread shows the branch has not had a Codex pass yet — a full review is welcome, not just a delta. Local verification on the new head: |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…the merge that just happened The note's START HERE still told a reader not to merge master, and said the branch was ~29 behind deliberately. Both were true when written and are now the opposite of the state, which is exactly the rot the note exists to prevent. **The overridden decision is recorded where it was overridden**, rather than quietly dropped. `81e08e100` decided on 2026-08-19 to merge master once at the end; this merge was asked for directly, ahead of #57, #322 and #267 landing. That decision's reasoning was that an early merge resolves the same conflicts repeatedly and differently from how the still-open PRs will - so the cost is real and falls on whoever merges next. The merge commit states each resolution and why, so a later merge can follow them rather than re-derive them. **`PCMetrics.java` is called out because the obvious reading of the instruction is wrong right now.** `835b593cf` says to take MASTER'S side - but conditionally, after #57 lands, and it has not. Master's `removeMeter` carries no guard at all today, so taking it now would delete this branch's teardown fix and reintroduce the throwing-registry defect that kills the poll thread. The file did not conflict, so the branch's version survived untouched, which is the correct state. The note now says so, and says to re-run the instruction when #57 lands. **It also records what was NOT done.** The tree compiles and every gate passes, but the deadlock probe, `ShardManagerStaleContainerTest`, `OutForProcessingCounterDriftProbeTest` and `InstanceStallProbeIT` have not been run against the merged tree. This branch's entire record is about not reading a green compile as evidence about behaviour, so leaving that unstated would have been the one omission it could least afford. The blocked status is unchanged and re-verified: all three dependencies are still open.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…rather than sharing them A `PCModule` holds ONE parallel consumer's collaborator graph. Two of its getters memoise a collaborator while taking that collaborator's owner as a method parameter, so on the cache-hit path the argument is there to be ignored - and ignoring it hands a second owner an object still wired to the first. Neither failure surfaces where it is caused, and both are now rejected with an `IllegalStateException` that names the fix. `shardManager(WorkManager)`, introduced by the preceding commit, is the first. A second `WorkManager` built against one module would have been handed the first's `ShardManager`: its `PartitionStateManager` and its shard manager would then operate on different owners, and nothing would go red. Before that seam existed each `WorkManager` constructed its own, so the hazard is new with the seam. It is reachable through the public `WorkManager` constructor and `PCModule.setWorkManager`. `brokerPoller(AbstractParallelEoSStreamProcessor)` is the second, found by sweeping `PCModule` for the same shape once the class of defect was named rather than the symptom. A second processor would have received a `BrokerPollSystem` polling a broker on behalf of the first - registering the first's metrics gauges, delivering the newcomer nothing. Its only caller today is `AbstractParallelEoSStreamProcessor`'s constructor, once per module, so the collision is unreachable - but that is a property of the callers, not of the method, and "reachable only because callers behave" is the argument this commit rejects. BOTH GETTERS ARE NOW FINAL, and substitution goes through `createShardManager(WorkManager)`. This is not tidiness. The one place in the tree that substitutes a `ShardManager` - the confluentinc#909 reproduction two commits along - does it by subclassing the module, and written against the memoising getter it overrode the memoisation and the guard away with it, reinstating the exact defect in the same branch that guards against it. That is what the sweep found. Splitting the factory out puts the memoisation and the guard where an override cannot reach them, so the compiler now refuses the mistake instead of a reviewer having to catch it. Rejected alternatives: rebuilding the cache for the newcomer, which breaks the module's one-instance-per-collaborator contract and leaves the first owner silently orphaned; and leaving it with a javadoc note, which is what the seam commit originally parked in `docs/refactoring.md` - a "safe only while nothing does X" caveat wants a check that fails, not a paragraph. `PCModuleCollaboratorOwnershipTest` covers both sides of each guard plus the factory seam. It is named for the defect CLASS, not for one instance, so the next memoised-with-owner collaborator gets a test rather than its own file. Mutation-checked: deleting either guard turns exactly its own test red and leaves the rest green. Swept and dismissed, tree-wide, for "a memoised collaborator that ignores its owner argument": `producerWrap()`, `producerManager()`, `consumerManager()`, `workManager()`, `pc()`, `dynamicExtraLoadFactor()` and `pcMetrics()` are memoised but take no owner argument; every `computeIfAbsent`/`putIfAbsent` factory in the tree is key-scoped and captures no owner-like parameter; `SupplierUtils.memoize`'s only production use is instance-scoped inside the owner's own constructor; there are no `@Getter(lazy=true)` fields and no DI holder other than `PCModule`. Still open, deliberately out of scope: `PCModule.setParallelEoSStreamProcessor` is the mirror image - a second processor's constructor overwrites the module's `pc` reference BEFORE the `brokerPoller` guard fires, so the rejection path leaves the module pointing at the half-built newcomer. That is pre-existing behaviour and a separate change. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…n, and the third precondition it uncovered #31 shipped the fix with unit-level proof and an openly declared gap: no load-level evidence. This closes it with something stronger than a soak - a broker-real reproduction that discriminates every run, with no seed and no sleeps. Building it uncovered why nothing had reproduced the defect before. The loss needs THREE coincidences, not the two previously recorded: a rebalance inside the registration insert loop, the partition returning so the uncommitted tail re-delivers, and - the missing one - NO take-scan of the shard in between. `ProcessingShard.getWorkIfAvailable` lazily evicts stale containers as it scans, so an ordinary take-scan HEALS the collision before a fresh arrival can hit it. Only a consumer whose pipeline is at target keeps residents alive long enough to eat a fresh record. That explains the standing puzzle: the Chaos Pain Suite is green on defective master not through an unlucky seed but because its own churn is the healing mechanism. Random reproduction was fighting itself. It also narrows the production risk - this bites BUSY consumers, not idle ones. The test drives all three deliberately: a gated user function saturates the pool so the control loop's delta stays at or below zero, a fixed message-buffer size pins the load factor so the target cannot step mid-experiment, a second group member on another topic forces an eager revoke/re-assign, and the insert is parked mid-loop until re-delivery has fully registered. Every await is event-gated - latches, listener callbacks, instrument counters - so a failure is loud rather than slow. **Proven both ways, and re-proven after every change to it.** With `addWorkContainer`'s stale-replacement branch disabled - the pre-fix drop-on-collision behaviour, a local-only edit since reverted - the IT fails after the 90s invariant await on the confluentinc#909-signature assertion, "a fresh arrival colliding with a stale shard resident was dropped", and NOT on any run-invalid guard. Green arm passes in ~14.5s. Run before and after the review passes, per docs/testing.md's rule that a modified test must be re-proven to discriminate. **A healed run fails as invalid rather than passing.** The quiescence gate runs only BEFORE the paused loop is released, so a rebalance landing after it (coordinator move, session-timeout blip) would sweep the residents and heal the staged collision. Four run-invalid guards close that: rebalance state captured at release and asserted unchanged; a worker leaving the processing gate early - timeout or interrupt - sets a flag asserted false at the end, because a de-saturated pipeline re-opens the take-scan; and `getNumberRecordsOutForProcessing() == MESSAGE_BUFFER_SIZE` asserted both in the saturation await and again at gate-open, since that count is the actual term in the `delta = target - outForProcessing` arithmetic the suppression rests on. A future shift in the load factor now fails loudly instead of leaving the test green with the defect branch unexercised. `PausableInsertShardManager` parks the CONTROL thread at the head of exactly one designated insert, after that batch has passed the `epochIsStale` guard. Three things about it are load-bearing and were each got wrong once: the park must dwell THROUGH interrupts, because PC wakes its control thread by interrupting it, so it delegates to the shared `LatchTestUtils.awaitLatch` rather than a hand-rolled loop; `PARK_DEADLINE_SECONDS` is 150 rather than 60 because the park must outlast parking-topic creation plus a rebalance-settle await whose quiet condition resets on every rebalance event, and expiring inside those budgets would kill a valid run as a misleading "wiring failure" in a repo where a flake fails the build; and `freshEpochInsertCount` increments AFTER the insert, not before, so the counter means "registered" - which is what the test's gate-open condition, polled from the awaitility thread, already claimed. THE WORKER GATE IS ANCHORED TO THE CLASS BUDGET, not derived from an event that has not happened yet. The deadline was `PARK_DEADLINE_SECONDS + 60` = 210s, but a worker enters the gate in step 1 and the park does not start until step 2; everything staged between gate entry and gate open can spend 30 + 30 + 60 + 30 = 150s of awaits plus the final 90s invariant await, so on a slow broker a run still inside the 300s class budget could expire the gate, de-saturate the pipeline and fail as "run invalid" - a flake, and this build has no retry. It is now deliberately longer than the class budget, so `@Timeout` is the binding deadline and a too-slow run says so instead of blaming de-saturation. THE PRODUCE HELPER IS SHARED. `produceKeyedRange` was a private second implementation of produce-and-track, which docs/testing.md forbids: producer setup, ack waiting and error handling would have had to be fixed in two places. The deferral once recorded for this rested on needing a broker run over every existing caller - but the fix is an ADDITIVE overload, so no existing caller moves and that argument does not hold. `KafkaClientUtils.produceOffsetKeyedRange` now owns it, and asserts the key<->offset correspondence its javadoc claims rather than assuming it: a topic that cannot honour the contract fails at the produce instead of as a confusing missing-record assertion later. TEARDOWN ASSERTS THE SECOND MEMBER STOPPED. Its running flag is only read between polls, so a poll or close already in flight could outlast the 10s join, leaving a live non-daemon thread and a live group member to rebalance into whichever broker test ran next. Teardown now uses `wakeup()`, the one cross-thread-safe `KafkaConsumer` call, waits long enough to cover a slow close, and ASSERTS the thread stopped - the failure belongs here, not in the next test. Untagged, so it runs in the ordinary failsafe lane: it is deterministic, not chaos. **What it still cannot do**: it rules out both KNOWN heal paths but never OBSERVES the collision, so an unknown heal path that evicts residents without de-saturating would turn it silently green. That is deferred with its evidence and its design in `docs/inflight/test-909-reproduction-cannot-observe-the-collision.md` - including the finding that the fix needs no production change - because confirming the count needs a broker. The note states the acceptance shape rather than a hardcoded collision count, so retuning either constant cannot leave an obsolete literal reading as the agreed value. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…buried `docs/inflight/AGENTS.md` said "work your current PR resolves is tracked by that PR - delete its file in that PR", and the root router said "a file is deleted when its work lands". Read together those are a blanket instruction to destroy the note at merge time, and that is how they were read: this PR's own note carried a finding with no durable home anywhere else, scheduled for deletion by the rule. The intent was only ever to keep the notes in touch with the code's reality. So the rule now names four outcomes and puts deletion last: - migrate what outlives the work to its durable owner FIRST (`docs/refactoring.md`, `docs/solutions/`, `CONCEPTS.md`, the topic doc); - keep the note when live content remains, shrunk and renamed if its area changed; - split when what remains is a different item - a new note, and `git rm` the old; - `git rm` only when nothing left in it is both true and unowned elsewhere. The router's `docs/inflight/` row additionally listed `parked-` and `next-` as filename prefixes. Both were removed precisely because a filename must not carry state - all 23 `next-` files were open, so the prefix ranked nothing while implying a ranking. The row now points at the owner instead of keeping a copy that had already drifted away from it. APPLIED TO THIS PR'S OWN NOTES rather than only written down, since a rule that is merely stated is the rule that gets missed. #322 is the third and last of the split, so both `pr-322-*` notes are the new rule's first customers - and left on master they would present finished PR coordination as open work in every session index. MIGRATED, THEN DELETED: * the proposed `bin/check-conflict-markers.sh` gate, and the merge that shipped 305 lines of an unresolved conflict with nothing red, became `ci-conflict-marker-gate.md` - the incident is the part that would have been lost, which is why it is a note and not a `refactoring.md` line. Its incident sentence is dated and says the branch is gone and the PR landed, so the citation records history rather than a live arrangement that ends at merge; * the five stranded agent-hook commit bodies were already owned by `branch-agent-hook-commit-bodies-only-on-backup.md`, so only the second copy went; * "re-fetch master before measuring anything" is already owned by `docs/merge-checklist.md`; * the `awaitLatch` re-arm worst case moved into `PausableInsertShardManager`'s own javadoc, beside the constant it is about. PROMOTED OUT OF `docs/refactoring.md`. The `availableWorkContainerCnt` undercount carried evidence, an impact assessment and a deferral, which is a note's job - that file is a line or two, no owner, no state. It is now `bug-processing-shard-available-work-undercount.md`, and asks the prior question before the obvious fix: whether a counter nothing reads for correctness should exist at all. Raised by @codex on #322. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… number now demands `bin/check-branch-self-reference.sh` (#341-era hygiene work, `da91f3f61`) arrived on this branch with the master merge. It fails when a `docs/inflight/` note mentions THIS branch or THIS PR without a human attesting the sentence still reads correctly AFTER the merge, and it reads whole files for the `#NNN` and branch-name arms, because your own PR number can only ever be about you. Run the way `repo-hygiene.yml` runs it - `PR_NUMBER=322 GITHUB_HEAD_REF=fix/909-load-reproduction` - it reported 17 unconfirmed self-references across eight notes. Six of those notes are master's, not this branch's; they mention #322, which is what makes them this merge's problem and nobody else's. TWO SENTENCES WERE ACTUALLY WRONG POST-MERGE, and both are rewritten rather than attested: - `pr-blockers-and-collisions.md` recorded the merge order AND a tally of which of the seven had merged. The tally goes stale at every merge in its own list - #322's included - so it is deleted rather than restated, and the reader is sent to `gh pr list -R astubbs/parallel-consumer`. The ORDER stays: it is the standing coordination fact, and it survives only here because #323's merge deleted the note that used to hold it. - `ci-agent-self-review-as-blocking-pr-comments.md` said the fix and its evidence "are in two PRs (#322)" - one number for a claim about two - and twice said the hook "lands with #324", which merged on 2026-08-20. Both are named in full and put in the tense they will be read in. THE REST ARE ATTESTATIONS, and each says why it holds: they cite #322 for what was OBSERVED or DECIDED on it - a sighting, a failed replay, a reproduction, a review finding - which is a claim about the past that a merge cannot falsify. `bug-pcmetrics-...md` cites the branch by name for a CI run; the run id resolves after the branch is deleted, which is what makes that line safe where the branch name alone would not be. Marker placement is not free-form: the gate accepts a marker on the flagged line, on the line above, or inside a `checked-begin`/`checked-end` block. Inside an ordered list or mid-paragraph both of the first two split the markdown, so those lines carry the marker inline at end-of-line and whole paragraphs take the block form - the shape `test-back-pressure-engage-point-is-unasserted.md` already uses. No note's meaning was weakened to pass the gate, and nothing was marked that this merge does not own. WHY A FULL LOCAL SWEEP MISSED THEM, recorded in the gate's own header where a reader hits the trap. The gate resolves "this PR" with `gh pr list --head <branch>`, so in a worktree whose local branch is not the PR's head ref - a `merge/NNN-...` branch pushing to `fix/...`, which is how master merges are staged in this repo - the lookup returns nothing, the PR arm never arms, and every `#NNN` mention passes locally. CI sets `GITHUB_HEAD_REF`, arms it, and fails; that happened on this PR, with a migrated note clean under a full local `bin/check-all.sh` sweep and red in Repo Hygiene. Nothing can detect it from inside the run - an empty PR lookup is indistinguishable from "no PR yet", which is a legitimate local state the gate deliberately supports - so the header now names the workaround, `PR_NUMBER=<n> bin/check-branch-self-reference.sh`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
This PR is not the confluentinc#909 fix — that is already on master. It is the
deterministic reproduction of the race, the module seam the reproduction needs, and a change to when a
test may be quarantined.
git diff origin/master HEAD -- '*ProcessingShard.java'is empty: the stale-resident replacementlanded via #31. Earlier versions of this description claimed the fix,
#177 and #209 were delivered here. None of them are.
The reproduction
RegistrationRaceStaleResidentITdrives the defect through the real registration path at brokerlevel, using
PausableInsertShardManagerto hold the control thread mid-batch so a rebalance landsbetween the stale inserts and the fresh arrival.
The third precondition is what makes it deterministic, and it is not in the original analysis: the
loss also needs no take-scan between the stale inserts and the fresh arrival, because
getWorkIfAvailableevicts stale containers as it scans — so ordinary churn heals the collision andonly a saturated pipeline (
delta <= 0) exposes it. Random chaos could never have found it.It fails a healed run as invalid rather than passing it. A reproduction that can silently stop
reproducing is worse than none.
Its produce step now goes through a new shared
KafkaClientUtils.produceOffsetKeyedRange, which keyseach record with the offset it lands on and asserts the correspondence rather than assuming it —
so a missing key names the lost offset, and a topic that cannot honour that contract fails at the
produce rather than as a confusing downstream assertion.
The seam, and the invariant it now enforces
PCModule.shardManager(WorkManager)resolvesShardManagerthrough the module like every othercollaborator, so a test can substitute one. It takes the owning
WorkManageras a parameter becausethe two are mutually dependent and
workManager()would recurse mid-construction.Memoising it opened a hazard of its own, since before this each
WorkManagerbuilt its ownShardManager: a secondWorkManageragainst one module would have been handed the first's, leavingits
PartitionStateManagerand its shard manager operating on different owners, with nothing red tosay so. It now rejects a mismatched owner instead of silently answering with the first's.
A same-defect-class sweep found the identical shape one method away.
brokerPolleris guardedtoo: memoised, takes its owning processor as a parameter, ignored it on the cache-hit path — so a
second processor would have received a
BrokerPollSystempolling on behalf of the first and neverbeen delivered a record. Only its own constructor calls it, once per module, so the collision is
unreachable today — but "reachable only because callers behave" is the argument the guard exists to
reject.
Both getters are now
final, and substitution goes throughcreateShardManager(WorkManager).That is not tidiness: the reproduction below substitutes a pausable
ShardManagerby subclassing themodule, and written against the memoising getter it overrode the guard away with it — reinstating, in
this very branch, the defect being guarded against. The compiler now refuses that mistake.
PCModuleCollaboratorOwnershipTestcovers both sides of each guard plus the factory seam, and isnamed for the defect class so the next one gets a test rather than its own file. Both guards are
mutation-checked: deleting either turns exactly its own test red.
Swept and dismissed for the same class:
producerWrap(),producerManager(),consumerManager(),workManager(),pc(),dynamicExtraLoadFactor()andpcMetrics()take no owner argument; everycomputeIfAbsent/putIfAbsentfactory in the tree is key-scoped;SupplierUtils.memoize's oneproduction use is instance-scoped inside its own owner's constructor; there are no
@Getter(lazy=true)fields and no DI holder other thanPCModule. Left open deliberately:setParallelEoSStreamProcessoris the mirror image — it overwrites the module'spcreferencebefore the
brokerPollerguard can fire — which is pre-existing behaviour and a separate change.ShardKey'sKeyOrderedKeyjavadoc described topic-only scoping while the constructor builds aTopicPartition— corrected, and itsdocs/refactoring.mdentry retired in the same commit, since anentry and the change that retires it are one change.
Quarantine rule: evidence, not diagnosis
@Quarantinedrequired a diagnosis before a test could leave the gating lane. That conflated "we donot know the mechanism" with "we do not know whether it is ours" — and only the second justifies
blocking. The cost was that an undiagnosed red blocked every unrelated PR, which trains everyone to
read red as normal; this repo already deleted surefire retries for hiding flakes, and a permanently
red gate destroys the same signal more thoroughly.
It now asks for evidence: a diagnosed mechanism or a recorded sighting ledger (dates, runs,
signature, and what makes it master-state). A hunch is still not evidence and one failure is a
sighting, not a ledger. Nothing else moves — the lane still runs the test, the registry keeps it loud,
and a non-empty list still blocks a release.
docs/quarantined-tests.mdcarries the rule text on master already — it landed with#323, so it is no longer part of this diff. The reasoning it was written for
still holds and is why the two were kept together at the time: the annotation asserts the rule and
cites that file as its reason, so landing them apart would have put a self-contradiction on master
with the javadoc pointing at the denial. That ordering worked out; the file simply arrived first.
The notes rule, applied to this PR's own notes
docs/inflight/AGENTS.mdgains the rule that a note whose work has landed has four outcomes, notone — migrate what outlives it, keep it shrunk, split it, or
git rmit — and that a"delete this when astubbs/parallel-consumer#NN merges" marker must never reach master, because the
merge is exactly when nobody is looking there.
This PR is the third and last of the split, so it applies that rule to itself. Both
pr-322-*coordination notes are deleted, their surviving items migrated to durable owners first:
bin/check-conflict-markers.shgate, and the incident behind it, becamedocs/inflight/ci-conflict-marker-gate.md;docs/inflight/branch-agent-hook-commit-bodies-only-on-backup.md, so only the duplicate went;docs/merge-checklist.md;availableWorkContainerCntundercount was promoted out ofdocs/refactoring.md— it carriedevidence and a pending decision, which is a note's job, not a one-line backlog entry's — and is now
docs/inflight/bug-processing-shard-available-work-undercount.md.The one unverified claim either note carried — whether
-Dpc.log.levelreaches failsafe forks aswell as surefire ones — was settled rather than migrated, riding on the broker run that verified
the IT: it emits
bz.stub.parallelconsumerINFO lines thewarndefault swallows, andbin/check-test-log-config.sh's header now records the observation and the integration-testinvocation it licenses.
bin/check-branch-self-reference.sh's header also gained the reason a local pass is not a CI pass —it resolves "this PR" by branch name, so a
merge/NNN-…worktree pushing to a differently-named headref never arms the PR arm, and every
#NNNmention passes locally and fails in CI. That is nothypothetical either; it happened on this PR, one push before the fix.
What a reviewer should look hardest at
The IT never observes the collision itself. It rules out the two known heal paths, but suppression
rests on
WorkManager.getWorkIfAvailable'srequestedMaxWorkToRetrieve < 1early return. If thatarithmetic shifts, the test goes permanently green with the defect branch unexercised. The proposed
fix — count stale-resident collisions at insert and assert the count equals the paused batch's tail —
needs a broker to verify and is not applied. It has its own note,
docs/inflight/test-909-reproduction-cannot-observe-the-collision.md, which owns it: the mechanism,the negative-control record proving the test is falsifiable today, and the finding that the
assertion needs no production change.
Merge
Behind #57 in the agreed order (57, 322, 267, 29 — 323, 324 and 325 have
landed). Independent — base
master, nodepends on.Rebase-merge. The thirteen working commits were re-cut into eight atomic units, one per
workstream, each body carrying its diagnosis, its experiment and its rejected alternatives;
git log origin/master..HEADis the current shape. The DI seam and the ownership guard are splitbecause they are separately revertable and separately meaningful in the release notes — the first is
an internal refactor, the second adds an exception a caller can hit. Squashing would need an "and
also" for each workstream, and master's history is predominantly rebase-merged — the
confluentinc#909 fix itself landed as several commits.
Checklist
docs/inflight/AGENTS.md's four-outcome rule, the two notes promoted or migratedout of retired PR coordination notes, the deferred collision-observation note, and the
docs/refactoring.mdentries retired by the code in this PRdocs/features/—N/A - a reproduction, a DI seam and a test-quarantine rule; no user-facing behaviourPausableInsertShardManager,PCModuleCollaboratorOwnershipTestfor the module-ownership invariant (both guardsmutation-checked), and
QuarantinedAnnotationContractTestfor the rule changece-simplifyandce-code-reviewlocally —N/A - asked for @codex review on the PR instead, and its seven findings are addressed in-thread