Repository navigation
fix(core): replace a stale container at a reused offset after rebalance - #31
Merged
Merged
Conversation
Open
1 of 2 tasks
cserspring
approved these changes
Apr 15, 2026
cserspring
left a comment
cserspring
left a comment
There was a problem hiding this comment.
Thanks, I reviewed it and it looks good. Feel free to merge.
astubbs
force-pushed
the
fix/909-stale-container-replacement
branch
from
April 20, 2026 22:47
6975295 to
18d1651
Compare
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 | 70 | 68 | 🫤 +2 |
| Duplicated lines | 977 | 949 | :face_with_raised_eyebrow: +28 |
| Duplication | 2.57% | 2.52% | 🫤 +0.05% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 5% | ✅ Pass (2.57%) |
| Max increase vs base | +0.1% | ✅ Pass (+0.05%) |
⚠️ 2 new clones introduced
- 13 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMapRaceTest.java:11<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/state/ProcessingShardStaleReplacement909Test.java:8 - 17 lines:
parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMapRaceTest.java:37<->parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/state/ProcessingShardStaleReplacement909Test.java:41
Powered by astubbs/duplicate-code-cross-check
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-confluent #31 +/- ##
===================================================
Coverage ? 77.02%
Complexity ? 1153
===================================================
Files ? 82
Lines ? 4195
Branches ? 387
===================================================
Hits ? 3231
Misses ? 772
Partials ? 192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
❌ Mutation Testing (PIT) ReportPIT did not produce a report. Most commonly this means a test failed in the baseline (PIT runs all tests unmodified first to establish green) and PIT aborted before mutating. See the "Run PIT mutation testing" step logs for the failing test, then either fix it or add it to |
astubbs
force-pushed
the
fix/909-stale-container-replacement
branch
from
April 22, 2026 23:55
18d1651 to
587b817
Compare
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
astubbs
marked this pull request as draft
July 28, 2026 11:19
5 tasks done
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
…anch experiment Checked both in-flight stall PRs against the zombie-drain root cause: - PR #29 (confluentinc#857): drain() still calls signalStop() first and the while(!shutdownRequested) short-circuit is intact - the ~10kHz spin and zombie partition hold survive unchanged. It fixes a SIBLING mechanism (onPartitionsAssigned resets pausedForThrottling - RUNNING-state throttle-pause) and even adds trace logging inside the loop that spins. Its 10-20% residual chaos-stall rate is a testable prediction: the chaos monkey's stop/start cycling is a drain-window factory. - PR #31 (confluentinc#909): main-code diff is only ProcessingShard.java - no overlap. All three efforts chase the same 'alive but not progressing' symptom via different, non-conflicting mechanisms, so the report now recommends an uber-branch experiment: merge #29 + #31 + the minimal drain fix (defer signalStop() to CLOSING), then measure #29's chaos run and this report's forkCount=16 recipe - if the residual stall rate drops to ~0, the zombie-drainer mechanism explains the residue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
…ring handover during drain Integration-level guard for the drain fix, calibrated against the planted defect on the experiment arms: - GREEN on this branch (fix present): B, joining mid-drain, consumed 200 records while A was still draining; A closed promptly on release; ledger: A=1100, B=200, duplicates=100 - exactly the parked in-flight tail (maxConcurrency), NOT a wholesale reprocess: the handover honoured A's commits. - RED on the no-fix arm (defect present): the group stayed frozen while A drained (B starved) until the worker-pool force-interrupt bailed A out - the mid-drain liveness window could never be established. Encodes the drain semantics explicitly (see report design review): a draining member CORRECTLY keeps its partitions to finish + commit (that is what bounds duplicates); what it must also do is stay rebalance-responsive so a mid-drain join never freezes the group. Report updated in the same spirit: design-review option 3 (eager LeaveGroup at drain start) REJECTED - it would guarantee duplicate reprocessing of in-flight work; explicit LeaveGroup belongs only in the deadline-expiry fail-safe (option 2). PR #29/#31 non-fix verification notes sharpened. Also the W1 skateboard scenario of the Chaos Pain Suite design (separate branch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
astubbs
added a commit
that referenced
this pull request
Jul 30, 2026
…(all guards green, zero conflicts) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
5 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
The rebuilt ledger opened with a table of every open PR, its branch, its worktree and its ahead/behind counts. That is a second PR tracker: `gh pr list` is always right, this table is wrong within a day, and a reader cannot tell which one they are looking at. Same for branch divergence and worktree paths, which `git` and bin/worktree-status.sh already report. Replaced with the part no command can answer - #29/#31 pointing at the pinned mirror and why the retarget is not mechanical, #38 blocked on a missing ArchUnit engine rather than the version bump, #51 colliding with #57 over PCMetrics, and which PR owns which files. Divergence counts are dropped throughout; what stays is the consequence ("rebase before review"), not the number behind it. Measured flake rates stay - an experiment's results are not re-derivable from a command. The rule is in the header, where the file's other writing rules live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
…s stop colliding (#112) The ledger had reached 677 lines and become a record of finished work. Entries told to "delete when #98 merges" outlived three merges; the rebalance-commit fix was still filed as awaiting PR after landing as #100; a static-state flake was still listed as deliberately-unfixed after #101 fixed it at source. Meanwhile #80, carrying the drain-zombie fix and the largest thing actually in flight, appeared only as a footnote about quarantine ownership. The file's own scope rule says entries vanish when work lands; it had stopped being applied. Every surviving claim is now checked against GitHub, git and the code. Obsolete entries are deleted rather than annotated: the jscpd cap is above baseline, no workflow has path filters so the docs-only inconsistency cannot happen, the ManagedPCInstance header carries its Modifications line, the CommitFailedException follow-up shipped in #108, and the stacked-PR gap is closed by a new all-branches ruleset. Upstream references now follow the convention the changelog already uses: fork numbering reaches #111, so every bare reference at or above #162 was silently upstream and read as ours. With the content correct, the file became a directory. It appeared in 26 of the last 30 master commits, so unrelated PRs conflicted on it constantly - not because they disagreed, but because their notes were adjacent, and the merge that resolves such a conflict silently drops one side (it happened between #108 and #110). Each item is now its own file, named <category>-<slug>.md; two PRs recording unrelated work touch disjoint files and cannot conflict, and finishing work is `git rm`, which never conflicts with an edit elsewhere. The prefix is the structure - `ls docs/inflight/` shows the shape of what is open without reading anything. There is deliberately no committed index: it would be edited by every PR, which is the problem the directory solves, and docs/TODO_INDEX.md is the cautionary case. This was parked on docs/inflight-as-directory with "migrating ~600 lines of existing entries" named as the reason not to do it. The audit is what made it cheap. docs/refactoring.md stays a single file - 2 of the last 30 commits touched it, so it has none of this problem. The manifest had drifted the same way, which matters more, because it is the declared source of truth that a future session trusts instead of re-deriving. Five entries disagreed with reality: bug-857 recorded no fork PR while #29 was open, #100 merged and #80 in review; fix-909 recorded no PR though #31 is open; bug-912 was in-progress when the schema's word for pushed-but-unPR'd is ready. Nothing catches this - upstream-map.py validate only checks the schema and upstream-sweep.sh only watches upstream, so "prs: []" beside an open PR passes every check we have. Hence the AGENTS.md rule to update it at every lifecycle transition, not only when starting. Four rules now live in docs/inflight/AGENTS.md so they are inherited rather than rediscovered: delete an entry in the PR that resolves it and never leave a "delete when #NN merges" marker on master; never record what gh or git can answer, which is why the open-PR table is gone; known code defects belong here even when an issue exists, because an agent scans this directory and will not read the tracker; and new guidance about how these notes are written goes into that file too. Four deferred code items moved to docs/refactoring.md, where deferred work lives: the SpotBugs thread-visibility findings, the produce-lock double release surfaced by #110, the commitAndWait message that misstates its own wait, and jacoco's single exec file under forked surefire. Also shares the agent-tooling gitignore rules that until now existed only in one checkout's .git/info/exclude, and deletes an empty duplicate-code-cross-check clone - a git init that never fetched - rather than ignoring it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
changed the title
4 tasks done
|
Claude finished @astubbs's task in 7m 48s —— View job Claude finished the review
|
astubbs
added a commit
that referenced
this pull request
Aug 5, 2026
A bare #NN means THIS fork. A bare number above the fork's own highest issue is therefore almost always an upstream reference pointing at the wrong repo - harmless-looking today, and wrong for real once the fork's numbering reaches it. Now that every upstream issue is mirrored, there is nearly always a fork number to use instead. The gate runs in the existing PR Checklist job, on ADDED lines only, and adapts as the repo grows: it reads the fork's current highest issue number from the API rather than hard-coding a threshold that rots. Three files are exempt because a bare number legitimately means upstream in them - CHANGELOG.adoc (its own header says entries below 0.6.0.0 predate the fork), upstream-map.yaml (every number is upstream by construction), and upstream-pr-analysis.adoc. Those are named in the module rather than left to judgement because a naive sweep on 2026-08-05 rewrote exactly those three wrongly. Same reason the rule skips javadoc member links, code spans and URLs. Escape hatch mirrors the changelog gate: "issue-refs: N/A - <reason>". Logic is a testable module with 11 assertions running before the gate, following changelog-ref-gate.js - that gate has shipped two real misjudgements, so this one proves its own rules first. PHASE 5 finished. Five single-mirror PRs carry both numbers in the title (#29, #31, #51, #57, #116). #53 and #106 span or only partly address several mirrors, so they get a Related issues section listing them WITHOUT a Fixes keyword - verified 0 closing references on each. A title listing three mirror numbers helps nobody, and Fixes on partial work is the error the #233 / #326 / confluentinc#857 mirrors already document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
4 tasks done
Claude is reviewing this PR
|
|
Claude finished @astubbs's task in 1m 59s —— View job Claude finished re-reviewing this PR
|
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…ot tooling The file already said everything here exists to "meta-program the agents working on this repo", which is the same idea in weaker words. This states what a hook IS rather than what it does, because the framing changes how the files get treated: not a lint layer bolted on the side, but the mechanism by which behaviour is actually determined at the moment it matters. The distinction is about WHEN each layer fires, not about documentation being important. A rule in a document takes effect only if someone opens it and thinks to apply it. docs/merge-checklist.md was injected into the very turn in which #31 was merged with work still outstanding, and it did not help - a checklist prompts for what you think to check against it, never for what you have forgotten you are waiting on. Records the four consequences that the rest of the file already enforces without saying why: a hook is production code and gets a negative control (this harness once shipped a suite that printed FAIL and exited 0); fail open on your own bugs, because a guard that blocks when broken is worse than the mistake it prevents; remove an arm rather than scope it when its claim cannot be made honest; and a hook that only reaches the shapes you thought of is a documented bypass - both merge guards here still miss `gh -R owner/repo pr merge`, recorded rather than quietly tolerated. Written into the repo rather than left in an agent's own memory. Memory is per-agent and per-machine - a Codex session, a CI reviewer, or a fresh session elsewhere never sees it. The repo is the only substrate every agent shares, which is the same reason CLAUDE.md exists to import AGENTS.md.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…d inject the plans index Two things aimed at the same failure: knowledge the agent already has, that does not survive the turn it was formed in. next-agent-self-review-as-blocking-pr-comments.md records the owner's proposal - the agent posts its own pre-merge must-dos as review comments, which GitHub blocks the merge on until each is explicitly resolved, mirroring how he reviews his own PRs. The admission test is the design, and it is recorded with the correction that produced it: "something I know must happen before this merges, that I am not WORKING ON RIGHT NOW". An earlier draft said "not doing in this PR", which is self-contradictory - work that is not in the PR cannot block the PR. Applied to #31 the rule admits exactly one comment, the outstanding load-level evidence, which is the thing that was lost. It also records the noise argument, because that is what kills such mechanisms: a blocking comment is only worth anything while resolving one is a considered act, so the agent proposes candidates and the maintainer approves before any are posted. And the prerequisite to check before building any of it - unresolved-conversation blocking is a repo setting, and if this repo lacks it the comments are advisory and the whole thing has no teeth. inject-recorded-knowledge.sh now NAMES the dated plans instead of counting them, and lists the test-hardening audits. A count tells you a shelf exists without naming anything on it, so an agent still had to know to run `ls` - which is the exact "never learns the document exists" failure the hook was written for. Measured while deciding scope: every path under docs/ is ~2.8k tokens, less than this hook's solutions block alone, so the constraint is signal rather than budget - docs/features/ and docs/data/ stay out as generated catalogues of shipped surface. Fixes a delimiter bug found reading that output: `paste -sd', '` treats ", " as a CYCLING delimiter list, joining items alternately by comma then space, so pairs of entries rendered welded together ("bug-857-family,bug-batch-quantity-over-request bug-max-failure-history-is-inert"). Anyone scanning for the second name of a pair would not find it.
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…t by what kind of work it is `feature` carried no impact, on the reasoning that proposed work has an opportunity rather than a consequence. That made it the bin for anything not yet broken: 21 of 72 notes were `feature` with no impact, including CI work, process notes, refactors and test debt. The tag stopped sorting anything. Two rules fix it, both from review: **A feature is something a USER of the library would notice.** CI automation, process, refactors and test work are not features however new they are - they are `task`, which forces an impact, which forces the question of what it costs to not do them. **A feature that exists to prevent a consequence carries that consequence.** The point of the tag is that work falls out in priority order, not that it is filed under the correct part of speech. A commit-failure seam whose motivation is PC shutting down is `feature` + `crash`, so it sorts with crashes rather than being buried among cosmetic features. Producer fencing likewise carries `reliability`. So `feature` may now carry an impact, and should whenever one applies. New impacts, all on the existing "what does it cost you not to know" axis: `crash` (the process dies or exhausts a resource until it does), `reliability` (survives less than it should, no single defect named yet), `refactor` (the code is harder to change than it needs to be), `ci`, `process`, and `test-debt`. `security` is extended from bugs to tasks, for the CVE backlog. 23 notes retagged. Two filenames were lying and are renamed: `parked-docs-site` is in planning draft, so it is open and its state tag is removed; `parked-stale-arrival-guard-...` was already tagged `blocked`, not parked. Citations to the old name in two dated plans and a solutions doc are repointed - a path repair, not a rewrite of what those records claim. Two judgements recorded because they went against the first reading: - `branch-stall-experiments` stays `stranded-work` rather than becoming `stall`. Its own first line is "one result worth keeping, then delete", and the composition experiment behind #80 has merged. It is stranded branches, not an open stall. - `parked-stale-arrival-guard-...` becomes `task`/`reliability`, not `bug`. Checked against master: `addWorkContainer` tests the RESIDENT for staleness, which is #31's merged half; the ARRIVAL check is still absent. But the note itself records that stale arrivals are harmless today - `couldBeTakenAsWork` refuses them and `getWorkIfAvailable` evicts them inline - so `bug` would overstate a live defect. It is work that prevents churn rather than cleans it up. The vocabulary itself and its gate live one PR up, and change with it.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…s as #31's, and what #31 actually settled The note explained the work and the decision it is blocked on, but not its provenance - so a reader who remembers it as "#31's" had no way to tell whether #31 left it undone, half done, or fixed it. All three readings were available, and the wrong one was taken in review. Records that #31 settled the RESIDENT half only, verified against master: `addWorkContainer` tests `isWorkContainerStale(existing)` and nothing tests the arrival. The arrival half is this note and is untouched. It surfaced during #31's review and was deferred out of it deliberately, which is why the number sticks to it. Also names the review question it came from - "why not just add a null check, no partition means the work is stale, so drop it?" - and points at the decision that question runs into, so the note reads as the unfinished half of a conversation rather than an isolated proposal.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…k in flight The mistake this exists to stop, in full. On 2026-08-19 #31 was merged about ten minutes before a spawned agent finished building the broker-level reproduction of confluentinc#909 - the exact gap that PR's own description declared open under "Known gap". The maintainer asked "are the commits ready?", the answer was given from the PR's state alone, and the background task nobody mentioned was never weighed. The fix and the evidence proving it now live in two PRs, and an inflight note went stale on master the moment it landed. Why a hook and not a line in a document: docs/merge-checklist.md already lists what to confirm before merging, and it WAS loaded - inject-merge-checklist.sh put it in that very turn. It did not help, because the question being asked was "is this PR ready?" while the thing that needed remembering was a task started an hour earlier. A checklist cannot prompt for what you have forgotten you are waiting on. The signal is Claude Code's own task output files under $CLAUDE_CODE_SESSION_ID, which are appended to while a task runs; a file touched inside the window means something is still producing output. Scoped by session id, so a sibling session never blocks this one. Also refuses while a maven build is live under .claude/worktrees. Registered with NO `if` matcher, for the reason docs/agent-harness.md already gives about the squash guard: a prefix `if` never sees `echo ready && gh pr merge`. It tokenises with shlex instead of grepping, so `gh pr comment --body "run gh pr merge later"` is not mistaken for a merge - the first draft did exactly that, which is why the case is now a test. Its limit is stated in the header and repeated in the denial the model reads: a STALLED agent writes nothing and is NOT detected, so this is evidence of live work, never proof of quiescence. Two agents stalled during the session that produced it. MERGE_DESPITE_OUTSTANDING_WORK=1 is the deliberate, loud override for when the outstanding work genuinely belongs elsewhere. Nine cases added to bin/test-check-agent-hooks.sh, negative controls included: a stale task file must NOT block, no session id fails open, an unparseable payload fails open, and the override releases it. Fails open on its own bugs throughout - a hook that jams the tool call shut is worse than the mistake it prevents.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…d inject the plans index Two things aimed at the same failure: knowledge the agent already has, that does not survive the turn it was formed in. next-agent-self-review-as-blocking-pr-comments.md records the owner's proposal - the agent posts its own pre-merge must-dos as review comments, which GitHub blocks the merge on until each is explicitly resolved, mirroring how he reviews his own PRs. The admission test is the design, and it is recorded with the correction that produced it: "something I know must happen before this merges, that I am not WORKING ON RIGHT NOW". An earlier draft said "not doing in this PR", which is self-contradictory - work that is not in the PR cannot block the PR. Applied to #31 the rule admits exactly one comment, the outstanding load-level evidence, which is the thing that was lost. It also records the noise argument, because that is what kills such mechanisms: a blocking comment is only worth anything while resolving one is a considered act, so the agent proposes candidates and the maintainer approves before any are posted. And the prerequisite to check before building any of it - unresolved-conversation blocking is a repo setting, and if this repo lacks it the comments are advisory and the whole thing has no teeth. inject-recorded-knowledge.sh now NAMES the dated plans instead of counting them, and lists the test-hardening audits. A count tells you a shelf exists without naming anything on it, so an agent still had to know to run `ls` - which is the exact "never learns the document exists" failure the hook was written for. Measured while deciding scope: every path under docs/ is ~2.8k tokens, less than this hook's solutions block alone, so the constraint is signal rather than budget - docs/features/ and docs/data/ stay out as generated catalogues of shipped surface. Fixes a delimiter bug found reading that output: `paste -sd', '` treats ", " as a CYCLING delimiter list, joining items alternately by comma then space, so pairs of entries rendered welded together ("bug-857-family,bug-batch-quantity-over-request bug-max-failure-history-is-inert"). Anyone scanning for the second name of a pair would not find it.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…eted inflight note became This branch deletes docs/inflight/test-909-not-reproducible-by-existing-chaos-scenario.md (settled by docs/solutions/logic-errors/909-needs-a-saturated-pipeline-the-third-precondition-2026-08-19.md), but three surviving files still cited it - bin/check-file-refs.sh is the gate that catches the class. The upstream-map entry and the merge guard's header now cite the surviving solutions doc; RegistrationRaceStaleResidentIT's javadoc is repointed in the accompanying hardening commit, which already rewrites the surrounding text. Also corrects two claims in the fix-909-stale-container manifest entry that predate this branch's base: fork PR #31 merged 2026-08-19 (the entry said "open and ready for review", status pr-open), and "no load-level reproduction exists" stopped being true the moment this branch's RegistrationRaceStaleResidentIT landed in review. 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
…ot tooling The file already said everything here exists to "meta-program the agents working on this repo", which is the same idea in weaker words. This states what a hook IS rather than what it does, because the framing changes how the files get treated: not a lint layer bolted on the side, but the mechanism by which behaviour is actually determined at the moment it matters. The distinction is about WHEN each layer fires, not about documentation being important. A rule in a document takes effect only if someone opens it and thinks to apply it. docs/merge-checklist.md was injected into the very turn in which #31 was merged with work still outstanding, and it did not help - a checklist prompts for what you think to check against it, never for what you have forgotten you are waiting on. Records the four consequences that the rest of the file already enforces without saying why: a hook is production code and gets a negative control (this harness once shipped a suite that printed FAIL and exited 0); fail open on your own bugs, because a guard that blocks when broken is worse than the mistake it prevents; remove an arm rather than scope it when its claim cannot be made honest; and a hook that only reaches the shapes you thought of is a documented bypass - both merge guards here still miss `gh -R owner/repo pr merge`, recorded rather than quietly tolerated. Written into the repo rather than left in an agent's own memory. Memory is per-agent and per-machine - a Codex session, a CI reviewer, or a fresh session elsewhere never sees it. The repo is the only substrate every agent shares, which is the same reason CLAUDE.md exists to import AGENTS.md.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…t by what kind of work it is `feature` carried no impact, on the reasoning that proposed work has an opportunity rather than a consequence. That made it the bin for anything not yet broken: 21 of 72 notes were `feature` with no impact, including CI work, process notes, refactors and test debt. The tag stopped sorting anything. Two rules fix it, both from review: **A feature is something a USER of the library would notice.** CI automation, process, refactors and test work are not features however new they are - they are `task`, which forces an impact, which forces the question of what it costs to not do them. **A feature that exists to prevent a consequence carries that consequence.** The point of the tag is that work falls out in priority order, not that it is filed under the correct part of speech. A commit-failure seam whose motivation is PC shutting down is `feature` + `crash`, so it sorts with crashes rather than being buried among cosmetic features. Producer fencing likewise carries `reliability`. So `feature` may now carry an impact, and should whenever one applies. New impacts, all on the existing "what does it cost you not to know" axis: `crash` (the process dies or exhausts a resource until it does), `reliability` (survives less than it should, no single defect named yet), `refactor` (the code is harder to change than it needs to be), `ci`, `process`, and `test-debt`. `security` is extended from bugs to tasks, for the CVE backlog. 23 notes retagged. Two filenames were lying and are renamed: `parked-docs-site` is in planning draft, so it is open and its state tag is removed; `parked-stale-arrival-guard-...` was already tagged `blocked`, not parked. Citations to the old name in two dated plans and a solutions doc are repointed - a path repair, not a rewrite of what those records claim. Two judgements recorded because they went against the first reading: - `branch-stall-experiments` stays `stranded-work` rather than becoming `stall`. Its own first line is "one result worth keeping, then delete", and the composition experiment behind #80 has merged. It is stranded branches, not an open stall. - `parked-stale-arrival-guard-...` becomes `task`/`reliability`, not `bug`. Checked against master: `addWorkContainer` tests the RESIDENT for staleness, which is #31's merged half; the ARRIVAL check is still absent. But the note itself records that stale arrivals are harmless today - `couldBeTakenAsWork` refuses them and `getWorkIfAvailable` evicts them inline - so `bug` would overstate a live defect. It is work that prevents churn rather than cleans it up. The vocabulary itself and its gate live one PR up, and change with it.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…s as #31's, and what #31 actually settled The note explained the work and the decision it is blocked on, but not its provenance - so a reader who remembers it as "#31's" had no way to tell whether #31 left it undone, half done, or fixed it. All three readings were available, and the wrong one was taken in review. Records that #31 settled the RESIDENT half only, verified against master: `addWorkContainer` tests `isWorkContainerStale(existing)` and nothing tests the arrival. The arrival half is this note and is untouched. It surfaced during #31's review and was deferred out of it deliberately, which is why the number sticks to it. Also names the review question it came from - "why not just add a null check, no partition means the work is stale, so drop it?" - and points at the decision that question runs into, so the note reads as the unfinished half of a conversation rather than an isolated proposal.
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
… 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>
astubbs
added a commit
that referenced
this pull request
Aug 24, 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. **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. Untagged, so it runs in the ordinary failsafe lane: it is deterministic, not chaos. Two disclosures routed to `docs/refactoring.md` rather than left in a note that dies with this PR: `produceKeyedRange` duplicating produce-and-track logic `KafkaClientUtils` should own, and `ProcessingShard`'s stale-replacement path leaving `availableWorkContainerCnt` undercounting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… 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>
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
…ale claims they found The merge landed four gates this branch predates. Three were mechanical; the fourth found real errors, which is the argument for it. **Duplicate notes.** The merge left two documents twice over, under both their old and new names - `next-truth-probes-for-internal-state.md` beside `test-truth-probes-for-internal-state.md`, and `parked-chaos-crash-fidelity-variant.md` beside `test-chaos-crash-fidelity-variant.md`. Identical content; git could not pair a rename on one side with an unchanged file on the other. `docs/inflight/AGENTS.md` decides which name survives: **a filename must never carry state**, and `next-` and `parked-` were retired for exactly that, so the `test-` copies stay and the three citations of the retired names are re-pointed. **`check-branch-self-reference.sh` - 117 mentions of #29 across 22 notes.** The gate asks that a note naming this PR be written as it will read after the PR lands, then attested. Working through them found five claims that were not merely mis-tensed but **already false**: - `pr-blockers-and-collisions.md` and `branch-stale-and-diagnostic.md` both said #29 and #31 target `master-confluent` and must be retargeted first. Both were retargeted onto `master` some time ago and #31 has merged, so the live coordination note and the branch-deletion guard were each telling a reader to do work already done - and the guard's reason for keeping a protected branch alive no longer held. - `pr-mirror-fixes-and-what-they-close.md` and `upstream-175-sporadic-commit-timeouts.md` described #29 as "still a draft" / "(draft, fix never observed working)". The draft status is a state claim; the substantive finding behind it - the reproducer runs in a commit mode the AB-BA cycle cannot reach - is kept and attributed to the file that owns it. - `test-857-revoke-under-work-sightings.md` recorded its A/B arms as "DEFECT = origin/master @ 438b09d; FIXED = **this branch** @ b8a335b". That is the spelling the gate calls the worst one: a stale `#29` still resolves and a reader can go and look, where "this branch" after the merge is unresolvable. Named. The rest state what #29's fix DOES - the AB-BA pair it replaces, the modes its cycle can close in, what its reproducer cannot settle - and stay correct after it lands, so they carry `post-merge: checked` rather than being reworded. `pr-29-857-deadlock-and-what-the-measuring-taught.md` takes `exempt-file`, the scope opt-out the gate documents for a PR note named after the very PR it describes. **`check-inflight-tags.sh`.** Twelve notes predate the tag scheme and are tagged by CONSEQUENCE per `docs/inflight/AGENTS.md`: the sightings file is a `register` (consulted, never completed, so filing it as a `task` would sort it among things somebody could finish), the Step-2 experiment is `deferred` on #29 landing, and the unmeasured-`largeNumberOfInstances` claim is a `blind-spot` rather than `misdirection` - nobody has measured it, as against it having measured wrongly. **`check-file-refs.sh`.** Three dangling references. The probe-critique note is deleted here as it already is on master - its critique was settled by the Class 2 demotion this merge brings - and the pointers to it now name the archived sha master's own records use. The other two are marked `file-refs: N/A`, where naming a retired file, or an abandoned 2022 branch's classes, is the point of the sentence rather than a citation. `bin/check-all.sh`: 15 gates, all passing.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… 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>
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
Ten commits, two conflicts, and both wanted keeping-both rather than a side. **`docs/inflight/AGENTS.md`.** Master replaces the blunt "when something closes, `git rm` its file" with a four-outcome rule - migrate what outlives the work first, keep the note when live content remains, split when what remains is a different item, delete only when nothing left is both true and unowned elsewhere (`6b5b9d044`, "a note is where knowledge is staged, not where it is buried"). That supersedes our side outright. But our side also carries a paragraph master has never seen - a PR earns its working note at its FIRST commit, with #29 as the worked example of what running for months without one costs - so master's rule is taken and that paragraph kept. **`docs/inflight/pr-blockers-and-collisions.md`, where the two sides are stale in opposite directions.** Master's first bullet is better: it deletes the merged/remaining tally, which went stale at every merge in the list, and sends the reader to `gh pr list` for what is still open while keeping the ORDER, which is the standing fact. Taken. Master's SECOND bullet is the one this branch already corrected - it still says #29 and #31 target `master-confluent` and must be retargeted, when both target `master` and #31 has merged. Ours wins, and the reason master still has the stale version is worth stating: #322's self-reference sweep (`8cadaeca0`) attested the first bullet and left the second, because that gate only ever looks at the branch's own PR number. A claim about #29 is invisible to #322's run and visible to this one. The gate is working as designed; the coverage gap is between branches, not inside one. **One dangling citation, newly visible.** Master's `6d1b55b92` taught the citation gate to read `.java` files and not only prose, and it immediately found one here: `ConsumerManagerCloseOwnershipTest` cited `docs/inflight/bug-shutdown-teardown-race.md`, a note that has never existed under that name in any commit on any branch. The exposure it describes - teardown running in a `finally` after `closeAndWait()` timed out, with the poll thread never joined - is real and is owned by `docs/solutions/architecture-patterns/two-threads-one-consumer-why-the-commit-seam-keeps-deadlocking.md`, which states it in full. Re-pointed there. `./mvnw test-compile` green across every module; `bin/check-all.sh` 15/15; `bin/todo-index.sh --check` clean.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…e extracted work comes home Fifteen commits, and three of them are this branch's own work returning in the form it was reviewed in: #375 (the probes and their brokerless harness), #376 (the back-pressure pause derived from Kafka), and #267, one of this PR's declared dependencies. #120's PCMetrics work landed with them. **Six of the nine conflicts had one right answer, already written down.** The rule recorded on this branch - take the EXTRACTED version, never this branch's copy, because the extracted one is what got reviewed on its own merits - covers `ThrowableUtils`, `PCMetrics`, `MetricsTeardownCannotBreakCloseTest`, `OutForProcessingCounterDriftProbeTest`, `BrokerPollSystemCooperativeRebalancePauseTest` and the mirror-of-state write-up. All six are now byte-identical to master, so the duplicate copies this branch was carrying are gone rather than merely reconciled. `BrokerPollSystem` is the same call one level down. Master's version is #376 as it landed, refined past what was extracted - a `shouldThrottle` local, a javadoc that explains `TopicPartitionState` rather than asserting the conclusion, and no reference to `ThreadConfinedConsumer`, which is not on master. Taking master's keeps the two from drifting; the cluster-2 note about consumer confinement can be re-added by the change that introduces it, which is where it will be true. **One correction had to be re-applied, and it is worth naming as a pattern.** `pr-blockers-and-collisions.md` was taken from master because master's version is genuinely better - it updates the file-ownership map for #337 and points the reader at `gh pr list` rather than a tally that goes stale at every merge. But master still carries the claim that #29 and #31 target `master-confluent` and must be retargeted, which this branch had already corrected against `gh`: both target `master` and #31 has merged. Taking master's file wholesale reinstated a claim I had already disproved. That is the cost of "take master's version" applied to a file rather than to a change, and the tell was the self-reference gate going red on a line I had fixed once. Re-applied on top of master's version rather than reverting to ours, so both improvements survive. `./mvnw test-compile` green across every module; `bin/check-all.sh` 15/15; `bin/todo-index.sh --check` clean.
astubbs
added a commit
that referenced
this pull request
Aug 31, 2026
…and migrate the #119 answer somewhere it survives Two jobs, and the second is the one that would have cost something. AUDITED: the "Still open" list was written 2026-08-19 and three of its items have since been done * The review deferral's PRECONDITION IS MET. It was deferred until #204, #31, #57 and #267 landed - all four have, master has been merged in, and the deadlock fix was re-verified afterwards on a four-cell control across both assignors. Review is now the right next step rather than a wasted cycle. Two NEW parents have appeared since that order was written - #381 and #393 - and the note says explicitly that neither is a reason to defer again, because neither changes this tree. * The roadmap edit is done, and the constraint the item stated is respected: it was not to be edited early in a way that asserts something not yet true, and the new wording says the fix is measured and unmerged, which is what is true now. * The merge strategy is recommended and the squash message written, in the scratchpad rather than the PR body. MIGRATED: the #119 answer now lives in the family ledger That answer existed only in this PR's own note, which is DELETED when the PR lands - so the established answer to "does #119 close with this?" would have gone with it, and the next person would have re-derived it from the same sightings. It is now in bug-857-family.md, which is on master and is the durable owner of this family's accounting. The answer, briefly: no, because #119 is a symptom bucket rather than a defect. The AB-BA cycle the fix breaks cannot close in PERIODIC_TRANSACTIONAL_PRODUCER at all - its second edge lives in a committer built only for the consumer-commit modes - which leaves the unbounded transactional revoke wait, #44, the only issue upstream ever labelled a verified bug, untouched. Closing #119 here would retire the remainder silently, which is the specific damage the merge checklist names. One mechanical note for whoever edits that ledger next: it carries a file-wide post-merge checked-begin/checked-end block, so a new section inside it must NOT bring its own pair. A nested pair breaks the outer one and the gate then flags lines far above the edit, which is a confusing way to find out.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…this family #183 (confluentinc#875) has carried a confluentinc#857 attribution since it was mirrored. The only thing that ever linked them is one hedged third-party comment on the upstream issue - "That's potentially the same issue as reported here: confluentinc#857" - which the mirror then made structural, splitting the report into a "stall half" credited to this family and an unexplained "skipped-record half". confluentinc#909 explains both halves at once, and its fix already landed here on 2026-08-19 as #31 (5e8ea66). Its own upstream reporter describes #183's symptoms with the offsets filled in: one offset never received while its neighbours complete, the committed offset pinned at exactly that offset, lag climbing on the partition, and a pod restart seeking back to it and processing it normally. The "eventually consumption stops" half follows from the same pin - offset-encoding back-pressure refuses more records for a partition whose base cannot advance. The defective addWorkContainer is byte-identical in the 0.5.3.1 both reports were filed against. Not asserted as a duplicate. confluentinc#909 needs three coincidences, and the third - a saturated pipeline, so no take-scan heals the stale resident before the fresh arrival hits it - means only a busy consumer rebalancing in the window is exposed. Whether that described #183's reporter is the one thing no code change can settle, and it has been asked. Two records are corrected rather than rewritten: - bug-857-family.md nominated the availableWorkContainerCnt drift for #183 "as well". That paragraph stands as written and is marked withdrawn for #183 beside it, with the reason. - The manifest's fix-909-stale-container entry now names fork issue 183 as its likely explanation, so the mapping is reachable from the fix as well as from the issue. Also retracted in the issue thread: an #344 shape match posted the same day. That one fails the report on three independent signals, all in the same direction - #344 destroys records at restore, so it cannot produce a record that comes back after a restart, which is the signal the earlier #207 triage had correctly identified as decisive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECopTRyeYJS1xY4rhRDWjZ (cherry picked from commit 0401ffd)
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
Checked every issue and PR the transactional registers name, against GitHub rather than against memory. Four claims had gone false, and one of them is the kind that stops a reader looking. **#177's mirror is closed while confluentinc#833 is open.** The map listed it as open; it was closed as completed on 2026-09-01 by the fork owner, with no reason on the issue and no linking commit, and the upstream report it mirrors is still open and still unanswered. The last comment on the mirror, five days earlier, argues the opposite - #267 renames the type in the title but does not fix the report - and ends "Left open". Nothing written down says what changed. That matters beyond bookkeeping: AGENTS.md makes the fork mirror the owner of an upstream issue's live status, so the mirror is currently asserting a status confluentinc#833 contradicts. Both `branch-transactional-battle-test.md` and `next-tell-809-833-they-are-likely-fixed.md` now say so, the latter with its own section, because that note's whole purpose is "tell the reporters" and closing the mirror is not telling anyone. Task 3 in it is half unreachable now - commenting on a closed mirror reaches nobody - and task 4 has grown to include reopening it or recording why it closed. No reason is invented here; only what is observable is written. **#29 targets `master`, not `master-confluent`, and is out of draft. #31 has merged.** The blockers register still said both target the pinned pre-rebrand mirror and needed retargeting. The retarget happened; what survives is the half that was never mechanical - #29's deadlock fix predates the internals #80 reshaped, so it needs reconciling rather than replaying. **Both of #262's parents have merged**, so the transactional stack section is rewritten from a land order into what it is now: history, plus one debt. **The debt is real and worth naming.** #257's merged commit message describes redelivery - "handed records back for a second delivery" - and never says *stall*. What #262 established is more severe: only a success marks a partition dirty, so every batch failing meant no commit was ever attempted and the source offset froze at 3 of 201. `git log -1 --format=%B b36ad94 | grep -i stall` returns nothing. Release notes are generated from the commit log, so 0.6.0.0 publishes the weaker claim unless the release section is amended or a follow-up commit says so. Also confirmed unchanged, so the map is not merely un-contradicted but checked: #44, #175, #189, #232 and #241 are all still open; confluentinc#809 and confluentinc#833 are both still open upstream; #100 is merged. `docs/inflight/handoff-262-master-merge.md` was already deleted in b99377b and is confirmed absent from the tree; this commit does not touch it.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
… 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 a silent data-loss bug. ProcessingShard's `entries` map is keyed by OFFSET, and offsets are only unique within a partition, so two records sharing a key on different partitions would land in one map under one key and one would be lost 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. Safe: the Lombok-generated getTopicName() had no callers anywhere in the tree, and the only reference was the constructor's own assignment. All modules compile and the core unit suite is green (387 tests). Found while reviewing #31; pre-existing and independent of it. Removes the corresponding entry from docs/refactoring.md.
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
3 participants
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.
Related PR: confluentinc#909
Description
A record polled after a rebalance is no longer silently lost. When a fresh
WorkContainerarrived at an offset a stale (wrong-epoch) container still occupied,addWorkContainerkept the stale entry and dropped the fresh one. The stale entry is never worked and is swept later — but the fresh record is already gone, and Kafka will not redeliver it while the consumer stays up, so that offset stays unprocessed until the pod restarts. That matches the upstream report: one offset never completing, the commit pinned behind it, cured only by a restart.Root cause: a check-then-act race between two threads with nothing serialising them. Rebalance callbacks fire inside
consumer.poll()onpc-broker-poll, bumping the epoch, installing a newPartitionStateand sweeping stale containers. Registration runs onpc-controlbehind a mailbox hop — the poll thread'sregisterWorkonly enqueues.WorkManagerholds no locks.maybeRegisterNewPollBatchAsWorkchecksepochIsStale()once and then loops, and every iteration inserts into the liveShardManager.A rebalance landing inside that loop admits old-epoch containers, in either of two ways — and the fix is indifferent to which, because it acts at the insert:
PartitionStatethe caller holds has itself gone stale, so it compares its own old epoch against the batch's old epoch and they match.The fix, and why at this layer.
addWorkContainernow tests whether the occupying entry is stale and replaces it, reusingisWorkContainerStale(). A non-stale duplicate is still dropped, unchanged.No epoch check earlier than the insert can close the window — it would still be check-then-act on the same unguarded pair of threads. And the design already tolerates stale entries by idiom:
getWorkIfAvailablefilters them viacouldBeTakenAsWorkand removes them inline, which is the authoritative check. The invariant actually broken was narrower — a stale resident must never block a fresh arrival at the same offset, because the resident is later swept while the arrival is already lost.entries.get→entries.putin one call is the last point where check and act are adjacent.That reading is the code's own:
9a966860b(confluentinc#219) removed foursynchronized (partitionStates)blocks on the grounds that "epoch tracking" replaced them — for the scheme as a whole, not for the registration check alone. Since two reviews in a row read one checkpoint in isolation and concluded the scheme was broken,PartitionState#epochIsStalenow documents all three checkpoints, why per-batch checking is correct rather than a shortcut, and what not to "fix".One accounting side effect, documented in place rather than fixed. Two paths reach the replacement having already spent this offset's decrement, so the shard undercounts available work — and not merely "until the next add", since that increments for its own entry. It resyncs when the zero-clamp in
dcrAvailableWorkContainerCntByDeltafires. Gauge-only, and it errs toward fetching sooner rather than starving, becausegetWorkIfAvailablescansentriesdirectly rather than gating on the counter.Also in this PR: both
addWorkContaineroverloads are now package-private (neither had a caller outside its package, andpublicadvertised a route that bypasses the batch guard); the three-checkpoint javadoc; and the written record — a reachability proof, two open items, three filed follow-ups, and a correctedupstream-map.yamlentry.Original fix by @cserspring, carried from
confluentinc/parallel-consumer#909with their authorship preserved on the fix commit.Test plan
ProcessingShardStaleReplacement909Test— 4 tests, greenexpected: first / but was: secondmaybeRegisterNewPollBatchAsWork), not around it — the others reach the shard directly, which skips the very guard whose failure is the bug. It asserts as a precondition that the stale batch reaches the shard, so it cannot silently stop testing the window; that precondition has already fired once, correctly, when an experimental arrival guard was prototypedChaosRevokeUnderWorkITat-Dchaos.seed=424242with the fix reverted passed (154.8s, read from the failsafe report XML, not an exit code) — so the existing chaos scenario does not reproduce this defect, which is consistent with the Chaos Pain Suite being green on master with the defect present. Recorded, with the replay command and an explicit note not to reroll seeds, indocs/inflight/test-909-not-reproducible-by-existing-chaos-scenario.mdChecklist
docs/inflight/open items, threedocs/refactoring.mdfollow-ups, and the corrected upstream manifest entrydocs/features/—N/A - internal correctness fix, no user-facing feature surfaceProcessingShardStaleReplacement909TestMerging
Five atomic commits, deliberately re-cut (verified content-identical: the tree hash before and after the rewrite is the same). Rebase-merge — squashing would bury @cserspring's authorship on the fix commit and fuse the fix, the package-private narrowing and the docs into one changelog entry.
cc @cserspring — your fix from the upstream PR has been adopted in this fork, with your authorship preserved on the commit. Thank you for the detailed race condition analysis!
🤖 Generated with Claude Code