Repository navigation
fix(core) astubbs#120: unbounded PCMetrics heap growth, with the confluentinc#905 hot-shard metric - #57
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
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57 +/- ##
============================================
+ Coverage 78.03% 78.30% +0.26%
- Complexity 55 1081 +1026
============================================
Files 81 81
Lines 4039 4060 +21
Branches 372 377 +5
============================================
+ Hits 3152 3179 +27
+ Misses 712 707 -5
+ Partials 175 174 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
❌ 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 |
Add a "Parallel-safe work while PR #57 is in flight" section to docs/inflight.md recording, for each in-flight track, whether it collides with PR #57's metrics/state files (857, 909, 51 -> sequence after) or is parallel-safe (912, release, logging cleanup, security bumps, contributor fixes, #40, confluentinc#915, DLQ), ranked by readiness. Also refresh the confluentinc#859 entry to the consolidated PR #57 (bundles the confluentinc#893/confluentinc#905 cherry-picks, supersedes the closed #42->#43->#45 stack) and expand the confluentinc#912 entry (ready, pushed, no PR, vertx-isolated). Bump the last-updated date. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "Parallel-safe work while PR #57 is in flight" section to docs/inflight.md recording, for each in-flight track, whether it collides with PR #57's metrics/state files (857, 909, 51 -> sequence after) or is parallel-safe (912, release, logging cleanup, security bumps, contributor fixes, #40, confluentinc#915, DLQ), ranked by readiness. Also refresh the confluentinc#859 entry to the consolidated PR #57 (bundles the confluentinc#893/confluentinc#905 cherry-picks, supersedes the closed #42->#43->#45 stack) and expand the confluentinc#912 entry (ready, pushed, no PR, vertx-isolated). Bump the last-updated date. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a661860 to
19dcf5a
Compare
Render backlink comments from an optional per-entry backlink field in
upstream-map.yaml (source of truth) instead of a separate body, with the same
{{FORK_REPO}}/{{FORK_REF}}/{{SUMMARY}}/{{ID}} placeholders; entries without it
fall back to the generic templates. bug-859 uses it to explain the two-cause
leak vs the already-merged upstream confluentinc#892.
Make fork status honest about landed-ness. The old "fixed" conflated "fix
written" with "shipped": every "fixed" entry is actually an OPEN, unmerged fork
PR (or a branch with no PR). Replace with a lifecycle vocabulary
(none|in-progress|ready|pr-open|merged|released|superseded|wontfix) and correct
the entries: confluentinc#859/confluentinc#893/confluentinc#905 -> pr-open (in open PR #57), confluentinc#857 -> ready
(branch-only). Add an optional per-entry todo: list for outstanding actions
(merge the open PR, post the backlink) surfaced by "upstream-map.py todo", so
"still to do" is explicit rather than implied by an open PR + null forwarded.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tus guard Audited PR #57 against issue confluentinc#859. The code matches the issue (registeredMeters List -> LinkedHashSet + prune, plus caching OffsetMapCodecManager in PartitionStateManager), but the wording framed the leak as rebalance-driven when the issue is commit-driven. Tighten bug-859 summary/notes/backlink: the List accumulated a duplicate Meter.Id on every registration (every commit); fork PR #57 fixes it via List->Set + PartitionStateManager caching (also closes #233); upstream confluentinc#892 covers the per-commit churn; confluentinc#893/confluentinc#905 are unrelated cherry-picks. Also fix upstream-backlink.sh: the fix-backlink status guard still checked the removed "fixed" value, so it refused every entry after the lifecycle-status change. Now allows ready|pr-open|merged|released and refuses none|in-progress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cache the fork<->upstream relationship once, machine-readably, so it stops being re-derived by hand every session. This fork (bz.stub.parallelconsumer) tracks the effectively-archived confluentinc/parallel-consumer, whose issues/PRs are a backlog worth mining and back-linking. Source of truth: - src/docs/development/upstream-map.yaml -- one entry per unit of work mapping fork branch/PR <-> upstream issue/PR, work group, lifecycle status (none|in-progress|ready|pr-open|merged|released), optional reconciliation, todo, and a public-facing backlink message. Header documents the schema; carries a last_swept date. Design follows Debian DEP-3 / Yocto Upstream-Status / OpenShift UPSTREAM. - src/docs/development/upstream-pr-analysis.adoc slimmed to editorial judgement (rankings/verdicts/merge order) with anchors the manifest links to; the manifest wins for facts. docs/inflight.md points at the manifest for the durable mapping. Tooling (scripts/): - upstream-map.py -- validate | table | refs | show | meta | tracked | posted-refs | todo - upstream-backlink.sh -- post a "fixed in the fork" / "maintained in a fork" comment to an upstream issue/PR, driven by the manifest. Dry-run by default; anti-spam: idempotent (skips already-forwarded), per-run cap, delay, status guard. Comment body comes from the entry's backlink field (single source of truth) or a template. - upstream-sweep.sh -- read-only check for NEW upstream activity since last_swept and drift on tracked refs; --publish updates a single fork tracking issue. Conventions: .gitmessage adds DEP-3-style upstream commit trailers (unforced); AGENTS.md documents the whole system. Seeded from the analysis doc, inflight notes, git and memory, and reconciled against a live gh sweep -- which caught drift (upstream confluentinc#541/confluentinc#548 now closed, confluentinc#866 is Kafka v4 not v7) and new items (confluentinc#892 merged, confluentinc#917/confluentinc#918/confluentinc#919/confluentinc#920/confluentinc#902). confluentinc#859 reconciled: upstream confluentinc#892 fixed the per-commit meter churn; fork PR #57 fixes the tracking-List (List->Set) plus assignment-path OffsetMapCodecManager caching. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 3m 48s —— View job Claude's review todo list
ReviewOverall this is a well-scoped, well-explained bundle. The 1. Synchronization gap left by the P1 follow-up — The This was already true pre-PR (the old 2. Minor: orphaned Javadoc block — /**
* Tracking of registered meters for removal from registry on shutdown.
*/
/**
* Using LinkedHashSet to prevent duplicate entries when the same meter is registered multiple times
* ...
*/
private Set<Meter.Id> registeredMeters = new LinkedHashSet<>();Two back-to-back Javadoc blocks — the original one wasn't merged/removed when the new explanatory comment was added. Only the second attaches semantically; the first becomes dead. Worth squashing into one block. 3. Minor: commit message doesn't match its diff — The 4. Test observations —
5. Cherry-picks ( Both look correct:
6. CI status jscpd flagged 4.20% duplication vs. a 4% threshold, but that's essentially unchanged from base (4.22%, i.e. this PR reduced duplication slightly) — pre-existing, not something this PR needs to fix. PIT mutation testing didn't produce a report (baseline test failure) — worth a look but likely unrelated to this diff; I didn't have CI log access to confirm root cause. Nothing here blocks merging — #1 is the only item I'd consider worth addressing before/soon after merge; the rest are nits. |
Cache the fork<->upstream relationship once, machine-readably, so it stops being re-derived by hand every session. This fork (bz.stub.parallelconsumer) tracks the effectively-archived confluentinc/parallel-consumer, whose issues/PRs are a backlog worth mining and back-linking. Source of truth: - src/docs/development/upstream-map.yaml -- one entry per unit of work mapping fork branch/PR <-> upstream issue/PR, work group, lifecycle status (none|in-progress|ready|pr-open|merged|released), optional reconciliation, todo, and a public-facing backlink message. Header documents the schema; carries a last_swept date. Design follows Debian DEP-3 / Yocto Upstream-Status / OpenShift UPSTREAM. - src/docs/development/upstream-pr-analysis.adoc slimmed to editorial judgement (rankings/verdicts/merge order) with anchors the manifest links to; the manifest wins for facts. docs/inflight.md points at the manifest for the durable mapping. Tooling (scripts/): - upstream-map.py -- validate | table | refs | show | meta | tracked | posted-refs | todo - upstream-backlink.sh -- post a "fixed in the fork" / "maintained in a fork" comment to an upstream issue/PR, driven by the manifest. Dry-run by default; anti-spam: idempotent (skips already-forwarded), per-run cap, delay, status guard. Comment body comes from the entry's backlink field (single source of truth) or a template. - upstream-sweep.sh -- read-only check for NEW upstream activity since last_swept and drift on tracked refs; --publish updates a single fork tracking issue. Conventions: .gitmessage adds DEP-3-style upstream commit trailers (unforced); AGENTS.md documents the whole system. Seeded from the analysis doc, inflight notes, git and memory, and reconciled against a live gh sweep -- which caught drift (upstream confluentinc#541/confluentinc#548 now closed, confluentinc#866 is Kafka v4 not v7) and new items (confluentinc#892 merged, confluentinc#917/confluentinc#918/confluentinc#919/confluentinc#920/confluentinc#902). confluentinc#859 reconciled: upstream confluentinc#892 fixed the per-commit meter churn; fork PR #57 fixes the tracking-List (List->Set) plus assignment-path OffsetMapCodecManager caching. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cache the fork<->upstream relationship once, machine-readably, so it stops being re-derived by hand every session. This fork (bz.stub.parallelconsumer) tracks the effectively-archived confluentinc/parallel-consumer, whose issues/PRs are a backlog worth mining and back-linking. Source of truth: - src/docs/development/upstream-map.yaml -- one entry per unit of work mapping fork branch/PR <-> upstream issue/PR, work group, lifecycle status (none|in-progress|ready|pr-open|merged|released), optional reconciliation, todo, and a public-facing backlink message. Header documents the schema; carries a last_swept date. Design follows Debian DEP-3 / Yocto Upstream-Status / OpenShift UPSTREAM. - src/docs/development/upstream-pr-analysis.adoc slimmed to editorial judgement (rankings/verdicts/merge order) with anchors the manifest links to; the manifest wins for facts. docs/inflight.md points at the manifest for the durable mapping. Tooling (scripts/): - upstream-map.py -- validate | table | refs | show | meta | tracked | posted-refs | todo - upstream-backlink.sh -- post a "fixed in the fork" / "maintained in a fork" comment to an upstream issue/PR, driven by the manifest. Dry-run by default; anti-spam: idempotent (skips already-forwarded), per-run cap, delay, status guard. Comment body comes from the entry's backlink field (single source of truth) or a template. - upstream-sweep.sh -- read-only check for NEW upstream activity since last_swept and drift on tracked refs; --publish updates a single fork tracking issue. Conventions: .gitmessage adds DEP-3-style upstream commit trailers (unforced); AGENTS.md documents the whole system. Seeded from the analysis doc, inflight notes, git and memory, and reconciled against a live gh sweep -- which caught drift (upstream confluentinc#541/confluentinc#548 now closed, confluentinc#866 is Kafka v4 not v7) and new items (confluentinc#892 merged, confluentinc#917/confluentinc#918/confluentinc#919/confluentinc#920/confluentinc#902). confluentinc#859 reconciled: upstream confluentinc#892 fixed the per-commit meter churn; fork PR #57 fixes the tracking-List (List->Set) plus assignment-path OffsetMapCodecManager caching. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
344ed77 to
090d74b
Compare
Record the user-visible changes this PR introduces under the unreleased 0.6.0.0 section: the PCMetrics memory-leak fix, the accurate-committed-offset fix, and the new shards.max.size metric. Follows the fork/upstream reference convention. Upstream-Issue: confluentinc#859 Upstream-PR: confluentinc#893 Upstream-PR: confluentinc#905 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review - went through all of it. Fixes pushed in 1. Synchronization gap (the one worth doing) - fixed. All four 2. Orphaned Javadoc - fixed. The two back-to-back blocks are merged into one. 3. 4. Tests - acknowledged, keeping as-is. The reflection into 5 / 6. Cherry-picks & CI - no action needed. Agreed the Also added the missing |
|
Follow-up note (not blocking this PR): the synchronization fix guards each |
…d(this) Replace the sprinkled synchronized(this) with a private metersLock monitor via Lombok @synchronized (the idiomatic dedicated-lock). The register* add goes through a narrow @synchronized track() helper so the slow Micrometer register() stays outside the lock; close() / removeMeter(Meter.Id) / removeMetersByPrefixAndCommonTags() are annotated whole-method. Locking on a private field rather than the publicly-reachable instance means external holders of a PCMetrics reference cannot interfere with the monitor (verified nothing synchronizes on the instance). The private removeMeter now self-locks via @synchronized instead of relying on callers holding the lock - also closing the hardening nit from review. Tests: 7 green. Addresses the PR #57 follow-up review note (dedicated lock over synchronized). Upstream-Issue: confluentinc#859 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the dedicated-lock follow-up in "Anywhere else the same fix is needed?" - checked:
|
…eal index Stop brushing over the pointers. Each abandoned refactor branch now has a specific one-liner (what it did, relevance, linked issue/PR) grouped by theme: thread-model/actor cluster (upstream #200), static-state removal, shard-count caching perf (confluentinc#530), engine/queue experiments (confluentinc#884), encoding, offsets/state classes (#233), API/interface, test infra. Records dead-ends explicitly (e.g. producer-facade, whose branch concluded it was not worthwhile) and supersessions (loom -> upstream confluentinc#908). The bulk verdicts for the ~53 prior closed PRs stay in upstream-pr-analysis.adoc; this index keeps the actionable pointers with issue links. Also added the two other synchronized(this) lock-hygiene sites (ProducerManager.syncBeginTransaction, DynamicLoadFactor.doStep) surfaced while checking the PR #57 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add docs/runbooks/pr57-post-merge.md - the exact, reviewed upstream comments to post after #57 merges: the confluentinc#859 fix-backlink (from the manifest backlink field), and tailored author-crediting notes for the carried confluentinc#893/confluentinc#905 PRs (the generic template is wrong for cherry-picked PRs), plus the manifest status flips. Update AGENTS.md backlink guidance: always pre-draft backlinks as a runbook committed to the PR - reviewable in-diff, tailored per target, context-aware across targets - rather than running the backlink script blind. Runbooks live in docs/runbooks/, deleted once executed; stragglers swept at the next major release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add docs/runbooks/pr57-post-merge.md - the exact, reviewed upstream comments to post after #57 merges: the confluentinc#859 fix-backlink (from the manifest backlink field), and tailored author-crediting notes for the carried confluentinc#893/confluentinc#905 PRs (the generic template is wrong for cherry-picked PRs), plus the manifest status flips. Update AGENTS.md backlink guidance: always pre-draft backlinks as a runbook committed to the PR - reviewable in-diff, tailored per target, context-aware across targets - rather than running the backlink script blind. Runbooks live in docs/runbooks/, deleted once executed; stragglers swept at the next major release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e8c3fd6 to
c316454
Compare
Add docs/runbooks/pr57-post-merge.md - the exact, reviewed upstream comments to post after #57 merges: the confluentinc#859 fix-backlink (from the manifest backlink field), and tailored author-crediting notes for the carried confluentinc#893/confluentinc#905 PRs (the generic template is wrong for cherry-picked PRs), plus the manifest status flips. Update AGENTS.md backlink guidance: always pre-draft backlinks as a runbook committed to the PR - reviewable in-diff, tailored per target, context-aware across targets - rather than running the backlink script blind. Runbooks live in docs/runbooks/, deleted once executed; stragglers swept at the next major release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0148625 to
283202e
Compare
|
@claude review this Please open inline review threads for anything blocking. What changed since your last review of this PR (which was clean at Scope: review only what this PR itself changes — The merge is where the risk is concentrated, so weight it accordingly:
Please also state what you checked and found clean, not only the findings. |
|
Claude finished @astubbs's task in 9m 39s —— View job Claude's review todo list
ReviewScoped to Item 1 — the inverse regression (branch work overwritten by master's side). Traced the merge commit Checked the inverse directly: Item 2 —
Item 3 — the never-throws teardown contract in
Confirmed (via No fourth hole found. One non-blocking subtlety worth recording rather than fixing: in Item 4 — Verification, not just inspection: compiled Nothing blocking. All four requested items check out clean, with one genuinely new (if trivial) finding — the missing blank line in |
|
@claude review this Please open inline review threads for anything blocking. Narrow scope this round: The library code is out of scope. You reviewed it clean at What to review, and why it earns a round of its own. A structured review found two P1s in A merge guard fails badly in both directions — silently not firing, or jamming a legitimate merge
Please also state what you checked and found clean, not only the findings. |
|
Claude finished @astubbs's task in 8m 31s —— View job Claude's review todo list
Environment note before the findingsThis sandbox blocked every attempt to execute the hook end-to-end: Item 1 — matcher coverage: no slip-through found, no over-match foundRan the exact
One real, narrow gap found — the PR-number extraction, not the regex. Item 2 — fail-open audit: every exit path is deliberateTraced every Item 3 — self-test cases: pin real behaviour, but the insertion point repeats the class of mistake it exists to catch
Item 4 — the
|
|
Both findings from the last review round were already fixed in d009a97, which landed after the head that round reviewed (
Fixed. VALUE_FLAGS = {"--body", "-b", "--body-file", "-F", "--subject", "-t",
"--match-head-commit", "--repo", "-R", "--author-email"}
...
if tok in VALUE_FLAGS: # `--body VALUE`: the value is the next token
skip_next = True
continue
if tok.startswith("-"): # `--body=VALUE` / `-tVALUE`: value is attached, not a token
continue
pr = _pr_number(tok)
if pr:
break
break # first positional is the PR argument; if not a number, give upThat covers both directions the finding named, and the exact
Also fixed in the same commit — the On Item 4, nothing to action — the round recorded the One thing this check turned up that is not this PR's, recorded here so it is not mistaken for a regression: |
astubbs
left a comment
astubbs
left a comment
There was a problem hiding this comment.
lgtm, just docs/inflight/pr-57-metrics-leak.md needs sync
|
Agent-written comment (posted via Antony's account while surveying where the Lincheck lane from #347 should point next).
|
…kill the poll thread Two faults in one subsystem, both confluentinc#859. The leak. PCMetrics kept a tracking list of registered meters and appended to it on every offset commit, re-registering identities it already held. Nothing removed them on revocation, so a long-lived consumer accumulated one duplicate Meter.Id per commit until the list dominated the heap. Registration is now deduplicated and revocation removes what it registered. The teardown. Meter removal ran on the path that closes the instance, and an exception from the registry there propagated into the caller - which is the poll thread. A metrics backend having a bad day could therefore take down consumption or fail a shutdown. Teardown now never throws, and the accessor between the two registry calls is guarded the same way. PartitionStateManager caches its OffsetMapCodecManager instead of constructing a throwaway per assignment, which is what made the duplicate registrations visible in the first place. Refs: confluentinc#859, #120 FOLDED IN AT MERGE PREP, from a later fix-up on this branch: the accessor between the two registry calls is guarded the same way as the calls themselves. It was found because the never-throws contract has three parts and only two had been written down - `removeQuietly`, the two-level guard in `removeMetersByPrefixAndCommonTags`, and the `getId()` guard in `removeMeter(Meter)`. A meter whose `getId()` throws dies during Micrometer's own stream enumeration, upstream of anything we can wrap, which is why the outer guard is not per-meter. Confirmed against micrometer-core 1.13.15 bytecode rather than assumed: `Search.meterStream()`'s filter lambdas call `Meter.getId()`.
…ey is visible Adds the shards.max.size gauge: the largest number of records queued in any single shard. shards.size already reported the total across all shards, which cannot distinguish an evenly loaded consumer from one where a single hot key is serialising all the work behind it - the case that actually hurts under KEY ordering. Cherry-pick of upstream confluentinc#905. Its test assertion travels in the un-quarantine commit rather than this one, because that commit rewrites the whole of PCMetricsTest and splitting the file between them would leave neither readable. Refs: confluentinc#905
…it on offsets The test was quarantined as flapping because it asserted PARTITION_LAST_COMMITTED_OFFSET against a shared completion COUNTER while the suite runs UNORDERED. Commits are contiguous and bounded by the lowest incomplete offset; completions are not ordered. Workers awaited a latch BEFORE incrementing, so a latched worker's offset never completed and the gap was permanent - the 120s atMost could not close it, only make the failure expensive. It passed only when the latched workers happened to hold the highest offsets, so a pass proved nothing. Rewritten to gate on each record's OWN offset rather than a shared counter, with two latched ranges held open per partition: a deliberate non-contiguous hole, and a freeze above a fixed offset. The pool is asserted wider than the workers the hole parks forever, because getting that wrong hangs rather than fails. Also carries the confluentinc#905 gauge assertion, since this commit rewrites the file wholesale and splitting it would leave neither half readable. Refs: #120, confluentinc#905 The two live citations of the deleted note are repointed here rather than later, so no commit in this branch leaves a dangling reference behind - `bin/check-file-refs.sh` would fail any intermediate checkout otherwise, and a bisect landing there would blame the wrong change.
…ne only silenced
`static: infer` went red once this branch caught up with master, and it is the ratchet working
rather than a break: five identities in `config/infer-known-findings.txt` stopped firing, and
`bin/infer-test.sh` fails on that by design - "an identity here that no longer fires means
somebody fixed something and did not ratchet, which is how a set quietly stops meaning
anything."
FOUR ARE GENUINE, and they are this PR's own subject:
1 THREAD_SAFETY_VIOLATION PCMetrics.gaugeFromMetricDef
1 THREAD_SAFETY_VIOLATION PCMetrics.getCounterFromMetricDef
1 THREAD_SAFETY_VIOLATION PCMetrics.getDistributionSummaryFromMetricDef
1 THREAD_SAFETY_VIOLATION PCMetrics.getTimerFromMetricDef
Those are the four registration paths that mutate `registeredMeters`, now behind
`@Synchronized("metersLock")`. Independent corroboration of the fix from a checker nobody
pointed at it - the ratchet file is master's, untouched by this branch, so the only thing
that could retire them is this diff.
THE FIFTH IS NOT FIXED, AND RETIRING IT QUIETLY WOULD HAVE BEEN THE DEFECT.
1 NULLPTR_DEREFERENCE OffsetMapCodecManager.lambda$loadPartitionStateForAssignment$2
`static-infer-findings.md` called this one "the cheapest thing on this page - start here",
still open. It stopped firing on a branch that touches neither `OffsetMapCodecManager`, nor
`PartitionState`'s constructor, nor `getEpochOfPartition`. Read the source and the defect is
still plainly there: `getEpochOfPartition` returns `Long` documented "or null if not yet
assigned", `decodePartitionState` passes it to `PartitionState(long newEpoch, ...)`, and the
auto-unbox IS the dereference.
The mechanism is this PR's `@Setter` removal on `PCModule.workManager`. With the setter
gone, `module.workManager()` is only ever the memoising provider, so the interprocedural
path Infer walked to reach that dereference collapsed - and the report with it. **A check
that went green because it stopped looking**, which is the class this repo names from the
other direction in a dozen places; here it is the analyser rather than a gate.
The ratchet has no state for it - "fixed" and "new" are its only two - so the line had to go
for a green lane. What stops that from becoming a lost defect:
`docs/inflight/bug-epoch-null-unboxes-on-partition-assignment.md` records the code, the
mechanism, and the fact that re-adding the line would NOT close it (Infer cannot reach the
dereference now, so it would be an identity that never fires - the same failure the other
way round). `static-infer-findings.md` loses its "needs no decision from anybody" claim,
which was wrong, and gains the general rule: a retirement is not by itself evidence of a fix,
so read the code an identity names before deleting its line.
NOT FIXED HERE ON PURPOSE. Null means *not yet assigned*, so the caller must decide what to
do with a partition decoded before its epoch exists - fail open or fail closed, on the offset
path, which is the same open question
`core-stale-arrival-guard-needs-a-null-safety-decision.md` carries for `getPartitionState`.
That is not a call to make inside a metrics PR.
Arithmetic checked rather than trusted: 22 findings described before, 17 after, matching the
run's own "17 finding(s)" - the sets reconcile only because one identity
(`RetryQueue$RetryQueueIterator.next`) carries a count of 2. The run reported no NEW
identities, so `current` is a subset of `expected` and deleting exactly the five reported
gone makes the two sets equal, which is what the gate compares.
Local: `bin/test-check-infer.sh` 8 of 8 (including its own red controls - a new identity
fails, a retired one fails, a same-count swap fails); `bin/check-all.sh` 15 of 15.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…that insists `src/docs/development/upstream-map.yaml` entries were going stale at exactly the moment nobody was looking: #204 merged while its entry still said `status: pr-open`, and checking the rest found two more already wrong. **The rule is: write `merged` in the branch and push it before you merge.** That reads like claiming something untrue and is not - branch content is visible to nobody until it lands, and the moment it lands the entry is correct. There is no observable instant where the manifest is wrong, and nothing to clean up afterwards. Doing it in the other order is what costs: the branch is gone, the correction becomes a commit straight to master, and until someone remembers, the manifest lies. `.claude/hooks/check-upstream-map-merged.sh` refuses a `gh pr merge <N>` while an entry naming that PR still says `pr-open`. It gates on the status rather than on mere mention, so it is silent once the entry is right - a guard that fires on correct behaviour teaches people to route around it. It fails open on every uncertainty (no PyYAML, unparseable manifest, no manifest in the CWD, no PR number on the command line), because a hook that blocks on its own bug jams the tool shut. Deliberately disposable: it exists only until the last upstream link is closed out, and then it is one file to delete. IT SHIPPED WITH TWO DEFECTS ITS SIBLINGS HAD ALREADY FIXED, and no self-test, which is the part worth reading. The command regex missed `gh -R owner/repo pr merge` - the exact form this repo trains - and the URL form fell open with the PR number in plain sight. Both are now the first cases in a 16-arm section of `bin/test-check-agent-hooks.sh`, run against a fixture manifest in a throwaway directory so a test's verdict cannot change when someone edits the real one. A third arm pins the PR argument as the first POSITIONAL rather than the first digit-shaped word: an unquoted numeric flag value ahead of it used to win, in both directions - denying the wrong PR, and quietly checking a PR nobody is merging. `scripts/upstream-map.py` gains the matching schema check, because the hook matches on the PR NUMBER and so cannot fire at all on an entry that names none: `fork.status: pr-open` with an empty `fork.prs` is unenforceable rather than merely untidy, and the state is one step away - split work onto a new branch and the entry names a status whose own PR does not exist yet. `docs/agent-harness.md` gains the hook's row and its counts. The registry self-test there compares the documented script and registration counts against `settings.json` and had gone red on this branch for the right reason - a hook registered and never documented. Its number-word map stopped at "twelve", so the thirteenth registration reported "the doc no longer states a count" instead of the mismatch; spelled out to twenty rather than to today's figure. Refs: #204 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot when its PR merges The rule added on 2026-08-25 said to draft a response to the issue a note maps to, "post only on explicit instruction", **and delete the drafts with the note**. Those last two clauses cannot both hold: a draft nobody happened to be asked about before the merge was destroyed *at the merge* - the exact moment nobody is looking, which is the failure this directory is organised against and the one the four-outcomes rule was written to stop. It was not theoretical. On #57 the owner was asked three separate times to post-or-lose a draft that was never meant to be at risk, because the rule as written left no third option. **The drafts accumulate, and one sweep before a release posts them together.** That is the point rather than a convenience: read as a set they get a common account of what shipped, instead of N replies written weeks apart from re-mined commit logs. `docs/releasing.md` gains that step, with the `ls` that finds them and the instruction to re-read each against what actually shipped - a draft written against a PR can be overtaken by a later change, and the sweep is the last cheap place to notice. **The file is named for the ISSUE, not the PR** - `issue-response-<NNN>.md`. A `pr-NN-` prefix names something that is gone by the time anyone posts it. It is a deliberate exception to "the prefix names an area": this one names a lifecycle stage with exactly one exit, and the sweep is what empties it. The never-post-unasked rule is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The knowledge half of the metrics work, kept out of the code commits so those stay readable as diffs. WRITTEN UP AS SOLVED, in `docs/solutions/`: - The `PCMetrics` registration leak itself - the measurement, the three-part never-throws teardown contract, the shutdown race that was nearly backlogged rather than fixed, and the one review nit that was declined with its reason. - The metrics test that compared a contiguous commit offset to an out-of-order completion counter under `UNORDERED`. The gap it produced was permanent, not slow, so the 120-second `atMost` could never close it - it only made each failure cost 140 seconds of CI. - Two workflow write-ups this branch paid for: `git diff A B` and `git diff B A` describe the same set of changes, and a brief's impossibility claim is the first thing to test rather than the premise to build on. STILL OPEN, so they get notes rather than write-ups: the `PCMetrics` lock held across registry calls, the `PCModule` injection seam the dead setter exposed, the shutdown/teardown race in its general form, the plain `HashMap` counter maps in `WorkManager` and `PartitionStateManager`, and the BSD-`stat` fail-open in the merge guard. RETIRED, because their work landed here: - `bug-pcmetrics-registered-meters-is-a-plain-arraylist.md` arrived from #347 with an instruction addressed to this PR - delete it here, and carry its reproduction in as the regression test's motivation. The Lincheck stack is now `PCMetrics859Test`'s class javadoc, stated as why `metersLock` exists, so a later reader deciding the lock is redundant meets the evidence first. The half this PR does NOT fix - those `HashMap` counter maps - moved to its own note rather than being deleted with it, and the note's three inbound citations are repointed by kind: the live ones to the surviving owner, the dated plan to a history pointer per `docs/citations.md`. - The offset-vs-completion note, whose defect is fixed and whose diagnosis is now a write-up. `bug-857-family.md` gains three `CLASS2_STALL` sightings seen on this branch's CI, numbered after master's sixteenth even though two of them predate it - master's ordinals are cited from two other notes and from inside the ledger, so renumbering those would break the citations. All three are superseded by the closure that file already carries; the standing advice is to cite the seed, never the ordinal. Every note on master that mentions this PR is rewritten to read correctly AFTER it merges, which `bin/check-branch-self-reference.sh` requires and which is cheap now and impossible later. Two of those were stale in the direction that matters, both asserting this PR could not merge cleanly: `Chaos Pain Suite` is required and gating rather than never-PR-gating, and `Check PR Dependencies` runs and passes rather than never having run. `upstream-map.yaml`: the entries this work owns move to `merged` before the merge, per the rule the tooling commit introduces. The `confluentinc#893` entry is left to #337, which owns that work since the 2026-08-24 split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Fixes #120 - confluentinc#859, unbounded
PCMetricsheap growth.Summary
Two metrics changes: the
PCMetricsleak and its teardown safety, and the hot-shard gauge carried from upstream. Plus an un-quarantine, a dead setter removal, and the records this work produced.Scope changed on 2026-08-24. This PR previously also carried the
confluentinc#893cherry-pick (#121, offset accuracy on assignment). That work has been split out ontofix/121-offset-accuracy-on-assignmentso it can be reviewed on its own: it fixes a correctness defect in committed offsets, and it has since grown a behavioural reproduction ofconfluentinc#894plus the discovery of a second, silent failure mode. Reviewing that inside a metrics PR would have buried it. The two halves share no main-code file, so the split is clean rather than a stack — no dependency line needed.fix(core) astubbs#120PCMetricsgrew without bound, and its teardown could kill the poll threadfeat(core) confluentinc#905test(metrics) astubbs#120metricsRegisterBindingby freezing it on offsetsrefactor(core) astubbs#120workManagersetter, which nothing could use correctlyfix(static) astubbs#57tooling(upstream-map) astubbs#204mergedbefore merging, and a guard that insistsdocs astubbs#120Re-cut at merge prep on 2026-08-26 into the seven above - linear, no merge commits, one
workstream each, on current master. Content is unchanged by the re-cut and was verified so:
git diff <old-tip> HEADis empty. Every one of the seven passesbin/check-file-refs.shindividually, not just the tip - two forward references (a commit citing a note another commit
created) were moved so a bisect cannot land on a red intermediate.
The changes
PCMetricsheap growth.registeredMeterswas anArrayListgaining a duplicateMeter.Idon every meter registration - which happens on every offset commit - even when an identical tag combination was already tracked. The reporter measured the retained list at 96% of heap after three days. Now aLinkedHashSet, withPartitionStateManagercaching itsOffsetMapCodecManagerinstead of creating a throwaway per partition assignment. Registration is guarded by a privatemetersLock, with the slow Micrometerregister()deliberately outside it, andtrack()handles the close race rather than orphaning a meter in a user-supplied registry.Metrics teardown can no longer take down consumption. Meter removal runs on the path that closes the instance, and an exception from the registry there propagated into the caller - which is the poll thread. A metrics backend having a bad day could fail a shutdown or stop consumption. Teardown now never throws, and the accessor between the two registry calls is guarded the same way. Same issue as the leak (
confluentinc#859), which is why it lands in the same commit.Hot-shard metric.
shards.max.sizereports the records queued in the most-loaded shard.shards.sizealone cannot distinguish evenly-spread work from one key monopolising a shard, which under KEY ordering is exactly what an operator needs to see. Cherry-pick ofconfluentinc#905.Un-quarantining
metricsRegisterBinding. It was quarantined as flapping because it assertedPARTITION_LAST_COMMITTED_OFFSETagainst a shared completion counter while the suite runs UNORDERED - commits are contiguous and bounded by the lowest incomplete offset, completions are not ordered, so the gap was permanent and a pass proved nothing. Rewritten to gate on each record's own offset, with the worker pool asserted wide enough that getting it wrong fails rather than hangs.Issues
Fixes #120 - confluentinc#859, the
PCMetricsleak.#121 is not in this PR's scope any more - it moves with the offset work to
#337, which carries the closing keyword for it. Worded without the keyword here on
purpose: GitHub matches
closes <ref>wherever it appears, so "no longer closes ..." would have closed it.Partly addresses, not closed here: #222 (metrics that expose what PC
delivers). Its item 3 asks for per-shard queue depth as "a distribution summary (or max/p99 shard
depth)", and the
confluentinc#905cherry-pick lands the max half -shards.max.size. Its items1 and 2, head-of-line-blocking-avoided and end-to-end record latency, are untouched, as is the
distribution summary itself. That issue also predicted this: it says per-shard depth "needs a
maintained counter rather than a live
size()call", and what ships here is the live walk, triagedas negligible with a
TODO(refactor)and the assessment indocs/refactoring.md.Related, not closed here: #117 (confluentinc#233, the
OffsetMapCodecManagerrefactor). This PR removes one throwaway-instantiation site; the structural refactor that issue asks for is untouched.Checklist
AGENTS.mdforbids a PR adding changelog entries, and the 0.6.0.0 section is regenerated from the commit log at release. The commit messages carry what the generator reads.docs/upstream.md,docs/testing.md(a new section making good a promiseAGENTS.mdalready made about silent test runs),docs/refactoring.md,docs/merge-checklist.md,src/docs/README_TEMPLATE.adoc(+ regeneratedREADME.adoc), and newdocs/solutions/write-upsPCMetrics859Test,RebalanceMetricsLeakTest,MetricsTeardownCannotBreakCloseTest, and the rewrittenPCMetricsTestupstream-map.yamlupdated - including removing this PR from theconfluentinc#893entry it no longer carriesNotes for review
git diff <old-tip> HEAD, which names only the offset-half files that moved out. The pre-recut tip is preserved as thebackup/pr57-pre-splittag.recorded rather than deleted because they were load-bearing for how this PR was read for weeks.
Chaos Pain Suitewas promoted to required and gating on 2026-08-26 - deliberately, andagainst the advice recorded at the time - so the old note that its header says "never PR-gating"
is stale. It is green on this head, and
mergeStateStatusis nowCLEAN.Check PR Dependenciesruns and passes, under a ruleset namedAll branches: PR dependency gate; the"expected" line still printed on a push is the ruleset evaluating a head whose checks have not
started yet, not a check that never runs.
review: human LGTMgate is head-insensitive bydesign (
docs/ci.md, "The second required check"), so the green tick says nothing about which headwas read -
gh api repos/astubbs/parallel-consumer/pulls/57/reviewsdoes. The last owner LGTM is2026-08-26 10:45:58 on
221a2bdd8, after the master merge, so the merge and the post-mergepass are covered. Not covered: the handoff extraction and the Infer ratchet shrink, both written in
answer to that same review round.
five identities; four are the
PCMetricsraces this PR fixed, and the fifth - a null dereference inOffsetMapCodecManager- stopped being reported because removing theworkManagersettercollapsed the path Infer walked to reach it. The defect is unchanged in the source. It is now
docs/inflight/bug-epoch-null-unboxes-on-partition-assignment.md; it is deliberately not fixedhere, because null means not yet assigned and the caller has to choose fail-open or fail-closed
on the offset path.