Repository navigation
fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue - #116
Merged
Conversation
…luentinc#912) The ConcurrentLinkedDeque backing JStream result streams was never cleared, causing unbounded memory growth when the stream was not actively consumed. Add close() override to drain the deque, backlog warnings at 10k items, and @deprecated annotations directing users to the callback-based API. Fixes confluentinc#912 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The header scanner landed after this branch was cut, and rejects it three ways: the new regression test is fork-original but claimed Confluent copyright, and the two JStream processors are upstream-derived files modified since the fork point without the Modifications line. Policy is in AGENTS.md; bin/check-copyright-headers.sh now reports 0 violations across 233 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude Code is working… I'll analyze this and get back to you. |
✅ 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)
|
… entry Applies the two rules this repo just adopted. The manifest moves to `status: pr-open` with PR #116 recorded, since opening a PR is a lifecycle transition and nothing automated notices one. And the ledger entry goes: the work is tracked by its PR now, so keeping a "branch with no PR" file would be exactly the stale note docs/inflight/AGENTS.md forbids. The manifest note also corrects what the entry claimed - this is not vertx-only. The same unbounded deque is in core's JStreamParallelEoSStreamProcessor and both are fixed, so the collision assessment was right for the wrong reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 5m 43s —— View job Code Review
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…) size() per result Three defects in the previous commit, two of them caught by this repo's own guards. The regression test was named JStreamMemoryLeakTest912, which matches none of surefire's default includes (Test*, *Test, *Tests, *TestCase), so the suite would never have run it - the exact failure #101's ArchUnit rule exists to catch, and it caught it. It passed locally only because -Dtest= bypasses collection. Renamed to JStreamMemoryLeak912Test; the full core suite now collects and runs it (320 tests green), and reverting the close() still turns it RED. The backlog warning and the clear-on-close block were copy-pasted into both JStream processors, which tripped the similarity check - it passes on other PRs, so the duplication was ours. They share no supertype, so the shared behaviour now lives in JStreamResultDeques and both call it. And the warning called ConcurrentLinkedDeque.size() twice per result. That is O(n): every added result walked the whole deque, so each addition got more expensive as the backlog grew - the cost scaling with the leak the warning exists to report. The size check is now sampled once per 10,000 additions via an AtomicLong. Also fixes a race the loaded parallel suite exposed in the test itself (it passed 3/3 in isolation, failed once under load): it waited on control-loop cycles after a latch that fires INSIDE the user function, but the wrapper only enqueues the result after that function returns, so the cycles raced the enqueue. It now waits for the result to reach the deque. Nothing drains it there - the stream is deliberately never consumed - so the condition is stable once true. No timeout loosened, no assertion weakened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Both red checks were real defects in this PR, not noise. Fixed in 1.
Correct, and worth stating plainly: my "verified RED→GREEN" claim in the PR body was checked with This is exactly what #101's rule was built for, on its first real catch. 2. 3. Found while de-duplicating: the warning was O(n) per result. 4. A race the loaded suite exposed in the test. It failed once in the full parallel run while passing 3/3 in isolation. Diagnosed rather than masked, per AGENTS.md: the latch fires inside the user function, but the wrapper only enqueues the result after that function returns, so Still not covered, deliberately: there is no vertx-specific test for the |
|
Claude finished @astubbs's task in 7m 6s —— View job Code Review
|
4 tasks done
Review caught that the override only covered close(). Every other shutdown entry point bypassed it, so the leak persisted for the common cases - including closeDrainFirst(), which is what the shipped Vertx example app calls. The right seam is close(DrainingMode), not close(Duration, DrainingMode) as first suggested: the Duration variant only sets shutdownTimeout and delegates (AbstractParallelEoSStreamProcessor:585), so overriding it would still miss closeDrainFirst() -> close(DrainingMode.DRAIN). Everything funnels through close(DrainingMode); both processors now override that. Fixing it exposed an ordering bug of my own: the clear ran BEFORE super.close(). A draining close keeps processing in-flight records while it shuts down, and each one enqueues another result, so clearing first left the drain's own output behind. It now clears after shutdown completes. The test only exercised close(), which is why the bypass survived it. Added closeDrainFirstShouldAlsoClearResultDeque and checked it earns its place - restoring the no-arg-only override makes it fail with "expected to be empty", while the funnel override passes 3/3. Core 321 tests and vertx 17 green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…e tests The clone check flagged 24 duplicated lines - both close tests carried the same copy-pasted preamble, which was mine and new in this PR. Extracted to produceOneResultAndAwaitIt(), which also gives the tricky part of that setup one place to be explained rather than two. The guard still earns its place after the refactor: restoring the no-arg-only override fails closeDrainFirstShouldAlsoClearResultDeque, 3/3 green with the funnel override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 5m 53s —— View job Code Review
|
astubbs
changed the title
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
…am-memory-leak # Conflicts: # src/docs/development/upstream-map.yaml
Master mirrored all 78 confluentinc issues into this fork, so the number ranges now overlap completely and a bare `confluentinc#912` is a coin flip - there is a fork issue 912 too. The new issue-ref gate flags exactly this on added lines. The test's javadoc now cites #122 / confluentinc#912, fork first, per AGENTS.md -> Issue references. #122 is the mirror, and is where status for this work belongs now that upstream-map.yaml tracks upstream PRs only - which is why the merge correctly dropped the bug-912 entry I had moved to pr-open rather than restoring it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 2m 48s —— View job Code Review
|
… described nowhere TWO CORRECTIONS AND ONE FINDING, all from the same question: was `refactor/deprecate-jstream` really unrecorded? CORRECTION. The entry added when that branch was deleted said it was "named in no ledger". Not true, and worth fixing in the record rather than only in conversation: `docs/refactoring.md` carried a one-line description and its tip in the branch inventory. What it lacked was a `branch_accounting` record, and that is the distinction that matters here - this file is the source of truth, so a future audit finding the branch gone had nothing HERE to tell a deliberate deletion from an accident. A refactor backlog is not a branch ledger. THE FINDING. Asking how many others are in that position turns up a set with no mention anywhere in the repository - not the manifest, not a ledger, not a note. Most pre-fork branches missing from `branch_accounting` are still described somewhere under `docs/`, which is the audit working; the residue recorded here is the part where nobody can say what a branch was for without reading its commits. Recorded as a re-derivable procedure plus a snapshot, not as a count, because the number is wrong the first time anyone acts on it. The commands are in the note. DELIBERATELY NOT TRIAGED. Several are obviously spent - `editor-config`, `refactors/mvnw`, `m1-changes`. A few read like they could carry real work against things that now exist: `features/health-check` against the shipped health-check API, `improvements/remove-static-use-pcmodule` against the `PCModule` seam #322 built, `refactor/chaos-broker-challage-test` against the chaos suite. Guessing from names is what this directory's rules warn against, and the value here is that the next audit starts from a list rather than from scratch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…branch control `Integration Tests` went red on this PR with `RegistrationRaceStaleResidentIT` failing its setup guard - `control thread must reach the mid-loop pause point (offset 25)`, `awaitPausePoint(...) expected to be true`. Same signature the ledger already describes, so this is a sighting rather than a new defect, and the row is updated rather than a second entry opened. THE EVIDENCE IS BETTER THAN ANOTHER TALLY, which is why it goes in as a control rather than an incremented count. `bin/inflight.mjs codecov test freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed` shows the test failing on `docs/225-producer-fencing-brainstorm` and on this branch within the same hour, with the identical signature, while PASSING on both of those branches at neighbouring commits. Two unrelated branches, same day, same guard, interleaved with passes: master-state and test-side, not any one branch's regression. Recorded history, no builds re-run. That satisfies rule 2, and this note now says so - the entry had the sightings but never drew the conclusion, so a reader still had to decide whether it was a rule-1 undiagnosed case. It is not. NOT QUARANTINED HERE, deliberately. The test reproduces confluentinc#909 through the real registration path, so parking it costs a real reproduction, and trading that against a required check that fails a few times a day is the owner's call. The classification this section already asks for comes first: the guard is a SETUP step, so the open question is whether the control thread is merely slow to reach offset 25 under CI load or something is preventing it - and only the second is a product defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…her it landed
Every branch on origin predating the 2026-03-24 fork point now has a
`branch_accounting` record. Before this, most had none, and the ones that did had a
name and a SHA rather than an account of what they contain.
READ, NOT GUESSED. Four agents read each branch's COMMITS and diff, then checked master
for what would supersede it. Judging by name is what this directory's rules forbid, and
it would have failed concretely here: `improvements/privacy-restriction` is a
shared-nothing-architecture spike - `ControllerEventBus`, `PartitionEpochTracker` - and
name-based triage would have filed it under privacy and lost it a second time.
Entries carry a `class` answering the question that prompted the sweep - has the idea
already landed?
absorbed shipped, by this branch or another route; the note names what supersedes it
superseded the problem is gone, or is solved differently
open-idea real, unabsorbed work someone might still want
spent scaffolding of its era
Nothing came back `unclear`. Counts are deliberately not written down - they are wrong
the first time anyone acts on them - so the note gives the grep instead.
THE FINDING. A large share are `open-idea`, and several are the direct answer to a defect
this repository documents as STILL LIVE. `features/partial-batch-failure` carries the
`TerminalFailureReaction` that `core-189-batch-failure-granularity.md` describes the
absence of. `features/key-partition-combine` carries the `KeyIsolation` configurability
`ShardKey` shipped without, so KEY ordering is silently locked to per-partition
isolation. `features/consumer-interface` carries the facade that `MultiTopicTest`'s
commented-out `assertCommit` names in its own source as what it waits for.
Two verdicts came from master's own code rather than from judgement:
`ExternalEngine.checkPipelinePressure()` is a deliberate no-op whose comment records the
pressure-stepping experiment being run and rejected, which settles that branch; and
`OffsetMapCodecManager` carries `// todo refactored to constant in the remove statics
branch`, a pointer whose referent nobody could name until now.
NOT A WORKLIST, and the manifest says so. `open-idea` means the code never landed, not
that it should - several are breaking changes belonging in the release-gated queue,
several are stacked on an unfinished actor base, and some are worth less than reviving
them costs.
`branch-audit-orphans.md`'s "described nowhere" list is replaced rather than extended:
the manifest holds the per-branch answer now, and keeping the names in two places is how
they drift.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
… the pre-push sweep could not see it `PR Checklist` went red on `bin/test-todo-index.sh` - `--check on a current index passes (expected '0', got '1')` - while `bin/check-all.sh` reported clean. Two separate defects, and the second is the one that matters. THE IMMEDIATE CAUSE. `list_files()` scans `*.yaml`, and `src/docs/development/upstream-map.yaml` is DOCUMENTATION that happens to be YAML. Its branch-accounting notes quote markers as evidence - "still carries `// todo refactored to constant in the remove statics branch`" is the point being made about a branch - so the scan indexed the quotation. That is precisely the pathology this script's own header already documents for `.adoc`/`.md`: the index lists the same marker from its .java source, so the entry duplicates and the count inflates. The extension list simply did not cover a doc written in YAML. `src/docs/` is now out of scope, and it contributed nothing before: `git show HEAD:docs/todo-index.md | grep src/docs/` was empty. THE GAP THAT LET IT REACH CI, recorded in `docs/inflight/ci-hygiene-gaps.md` rather than patched here. `check-all.sh` discovers gates by globbing `bin/check-*` and runs each with NO ARGUMENTS. `todo-index.sh` fails both halves: the name does not match, and run bare it REGENERATES the index - a pre-push sweep that mutates the working tree is the one thing it must not do. `--check` is the read-only mode and the uniform contract has no way to ask for it. So this is a design constraint rather than an oversight, which is why it gets a note with three costed options instead of a special case wedged into the glob. It matters because `check-all.sh`'s promise is load-bearing: AGENTS.md sends every agent to it precisely so the gate set cannot drift from what someone remembered. A sweep that says "no gate failed" while a tree-health gate is red is the same failure #356 motivated the script with, arriving through a door it does not watch. Not fixed here because changing the gate every agent runs before every push is not a ride-along for a JStream fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…oth directions The branch sweep found the ideas; this connects them to where the decision now lives, so neither side has to be rediscovered again. WHAT THE LINKS ESTABLISHED. Two of them answered a question worth asking - did our current work know? Both did. #333's `core-auto-scaling.md` already cites `features/dynamic-concurrency-control` by SHA and summarises it, and #392's `core-distributed-throttling.md` already names `features/rate-limiting` among related abandoned branches. So the ideas were not lost; they were catalogued in `docs/refactoring.md`'s idea bank and found from there. What was missing was the REVERSE link - a branch had no way to point at the work that superseded it - and a record in the ledger, which is why the same branches still had to be rediscovered by reading commits. `client-factory` -> #420. Same shape for the consumer that #420 builds for the producer, and more relevant now than when written: master already enforces exclusive consumer ownership at RUNTIME through `ThreadConfinedConsumer`, so a factory makes structural what is currently a guard. `refactor/control-loop` -> the God-class section, with the five names it cuts along: `ControlLoop` / `Controller` / `StateMachine` / `PCWorkerPool` / `WorkMailbox`. That is the useful part - the section argues about WHETHER to split without recording what a working split actually cut along, so anyone starting fresh re-derives seams that were already shown to compile. `features/partial-batch-failure` -> `core-189-batch-failure-granularity.md`, which describes "per-record result correlation was started and abandoned" WITHOUT naming the branch that holds it. It is named now, with its sibling and what they carry - `TerminalFailureReaction` and the rest, none of which reached master. FOUR OF THE SIX NEEDED NO NEW NOTE, and that is the point rather than a shortcut: their owners already exist, and a second note would be the duplicate this repository's rules forbid. `features/consumer-interface`'s owner - `core-alternate-api-facades.md` - is in flight on #367 across a dozen refs, so writing a copy here would collide when that lands; the ledger links both instead. Only the consumer-side client factory was genuinely unowned, and it gets `core-pc-owns-the-clients-it-uses.md`. A FALSIFIED ARGUMENT, found while removing a stale line count. The actor-collection ideation reasons from "the file has moved one line in 3.5 years, which is the clearest evidence that branch-shaped goals don't move it", and proposes tracking progress by that count. Measured: the God class was 1534 lines at that ideation's own date and is over a thousand larger now - it moved in a fortnight what the argument said it had not moved in three and a half years. The dated record is left as written per `docs/citations.md`; the correction sits with the live decision. It does not weaken the case for decomposing, it inverts the reason: the class is accreting, not inert. Sizes are no longer written down in the section that kept getting them wrong. The command is there instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…am-memory-leak 7a8a7f0 rewrites the README paragraph on commit intervals; this branch rewrote the result-stream paragraphs of the same two files. Different sections, merged clean, and both verified present afterwards rather than assumed - a prose loss fails no test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…that was withdrawn The `bounded-buffers` entry justified itself with "the streaming results API is being deprecated for exactly that, so the two are one piece of work". That stopped being true when the owner withdrew the JStream removal on 2026-09-03 - and the entry is one of the few places the old plan was still asserted as current. Rewritten to stand on its own: an unbounded buffer is a memory-safety problem whether or not the API around it is being retired. The JStream result queue is the worked example and is STILL unbounded after this PR - which fixed the consumer that walked away, not a consumer genuinely slower than the producer, and says so in its own description. What is missing is a decision about what should happen when a bound is reached, not the bound. Found by the merge checklist's "is the PR description still true" pass, applied to the records this PR's decisions touch rather than only to the PR body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…owners, and retire the PR note `docs/inflight/pr-116-review-followups.md` said in its own header that it is deleted when the last item closes. Two review threads were resolved and the untested path was covered; what remained were three items that OUTLIVE the PR, and a PR-shaped note is the wrong home for those - this directory's rule is that knowledge migrates to its durable owner rather than dying with the branch that found it. `controlThreadFuture` still being a plain field goes to `core-control-thread-contract-debts.md`, which already owns the shutdown-signal discussion - **and which claimed `state` is non-volatile.** That is no longer true: #342 made it `private volatile State state` for unrelated reasons. So the note was carrying a stale fact about the very field its item 1 turns on, and correcting it is half the value of moving the item there. What survives is one keyword on `controlThreadFuture`, whose failure mode is now benign rather than a hang. The `shutdownNow()` javadoc overstatement goes to `docs/refactoring.md` - a sentence to narrow, too small for its own note, which is exactly what that file is for. It claims queued results are delivered rather than discarded; true for an ordinary close, not for the branch that can make `isClosedOrFailed()` true while a worker can still enqueue. The unbounded queue was already migrated: `docs/data/roadmap.yaml`'s `bounded-buffers` entry names the JStream result queue as its worked example and records that it is still unbounded after this PR. Nothing cited the note, checked before deleting rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
…eleted the note without 8e517e7 deleted `pr-116-review-followups.md` and landed NOTHING ELSE. Its `git add` listed the already-`git rm`ed path, git refused the whole pathspec, and the two files carrying the migrated content were never staged - so for one commit the branch said "migrated to their owners" while the owners had not been written. That is precisely the "knowledge goes to die" failure the message claimed to prevent, committed by the commit claiming it. The content, now actually present: `core-control-thread-contract-debts.md` takes the `controlThreadFuture` item, and its claim that `state` is non-volatile is corrected - #342 made it `private volatile State state`, so the note held a stale fact about the field its own item 1 turns on. `docs/refactoring.md` takes the `shutdownNow()` javadoc overstatement: queued results are promised as delivered rather than discarded, which holds for an ordinary close but not for the branch that can make `isClosedOrFailed()` true while a worker can still enqueue. Caught by reading `git show --stat` on my own push instead of trusting that a commit which exited 0 contained what was intended. The `git add` error was printed and I had already moved on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for Superseded by a newer quarantine lane report. |
…am-memory-leak Two commits, one conflict, and it is the flake ledger again - the third time this branch and master have both written to the same row, which is what a shared ledger looks like when it is being used rather than ignored. #419 raised `RegistrationRaceStaleResidentIT` from five sightings to six, naming #419's own two among them. This branch's version of the row carried something master's does not: a same-day cross-branch control, taken from recorded codecov history rather than by re-running builds, showing the identical `mid-loop pause point (offset 25)` signature on two unrelated branches within an hour while both passed at neighbouring commits. Resolved by keeping master's sighting record - it is strictly the better count - and grafting this branch's control into it, then re-appending the `AmbientProbeExtensionTest` row master does not have. Taking either side whole would have dropped either two sightings or the evidence that makes them mean something. `docs/inflight/AGENTS.md` and `docs/refactoring.md` auto-merged; both sides verified present afterwards rather than assumed, in both directions - master's additions and this branch's - because a prose loss in a merge fails no test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Since the previous push: Updated for |
astubbs
added a commit
that referenced
this pull request
Sep 4, 2026
Inherited: #116's jstream result-stream fix (breaking, and it WITHDRAWS the queued JStream deprecation - `docs/refactoring.md` loses those entries here as a result, which is master's change and not this branch's), and #419's docs context query, which moves `bin/lib/inflight-tags.sh` to `.mjs` and rewrites the prior-art section of the root `AGENTS.md`. Neither touches the infer lane, the retry queue or the commit-timing path. One conflict, in `docs/inflight/test-untracked-ci-flakes.md`, on the registration-race row both sides had just edited. Resolved by keeping BOTH controls rather than either side's: - master's same-day CROSS-branch control (the same guard red on two unrelated branches within an hour, green at neighbouring commits on both), from #419; - this branch's WITHIN-branch control (the same content passing at `01fb0c4` and failing at `53ef4d6`), which is the stronger of the two because no cross-branch comparison can hold content fixed. The count is the union of the two sighting lists, and master's new `AmbientProbeExtensionTest` row is kept - it arrived in the same hunk and taking either side whole would have dropped it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 4, 2026
… correct the record The previous commit filed `dups: clones` as a flake-ledger row saying the lane "tips over on a slow runner". A fourth attempt, on a fresh five minutes from a new head, failed identically, and the framing does not survive it. The row is retired and replaced with a note that says what the evidence actually supports. WHAT THE EVIDENCE SAYS Outcomes split cleanly by DATE, not by branch or runner: two attempts on 2026-09-03 succeeded and posted the full report; four on 2026-09-04 were cancelled. One of those four is on an unrelated PR, which is what rules out branch content - a second PR failing the same way at the same time cannot be explained by either branch's diff. The boundary is master gaining #419 and #116. Each cancelled attempt dies a few seconds past the five-minute cap with jscpd still running, named by the runner's own cleanup as an orphan process. The action runs PMD CPD and jscpd over the whole tree twice - PR and base - inside one five-minute budget, so corpus growth is paid four times. WHY THE FILING MATTERS AS MUCH AS THE DIAGNOSIS A deterministic breakage sitting in a flake ledger invites the response a flake deserves - re-run until green - which here would burn runner time and hide a required check that no longer works. It also is not the failure `ci-duplication-report-can-fail-to-post.md` already owns: that one is the lane FINDING a clone and failing to post it. A cancelled tick and a failed tick look alike in the checks list and mean opposite things. WHAT IT COSTS RIGHT NOW `dups: clones` is a required check, so while this holds no PR in the repository can merge. That is why the note is `impact: stall` rather than a CI-hygiene item, and why the fix belongs on master rather than on the feature branch that happened to find it. Master history cannot be used to attribute it: the job is `if: pull_request`, so on master it is `skipped`, which reads as green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
4 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 5, 2026
…d honours the caller's Duration
Three review findings on the `close(DrainingMode)` override this branch
introduced, each pinned red-first. Red proofs below were produced by reverting
only that item's main-code change against the final tests, running
`./mvnw -pl parallel-consumer-vertx -am test -Dtest='VertxClose*Test'`, then
restoring the change; the same run is green with all three in place.
1. Exception safety. The Vert.x teardown ran in an unguarded `finally`, so a
`TimeoutException`, `InterruptedException` or a failing `webClient.close()`
or `vertx.close()` REPLACED whatever `super.close(...)` was already throwing -
the same replacing shape `AbstractParallelEoSStreamProcessor.doClose` guards
each step of its own `finally` against. `close(DrainingMode)` now captures
the close failure, always runs the teardown, and if the teardown throws too,
attaches it to the close failure as suppressed; the teardown's own exception
is thrown only when there is no close failure to outrank it. Written as a
sequence rather than a guarded `finally`, because a `throw` inside a
`finally` is exactly the shape the fix exists to prevent (and Error Prone's
`Finally` check said so on the first draft).
Red: `VertxCloseReleasesEngineTest.aFailingTeardownIsSuppressedOnTheCloseFailureRatherThanReplacingIt`
against the unguarded finally -
Unexpected exception type thrown, expected: <java.util.NoSuchElementException>
but was: <java.util.concurrent.TimeoutException>
Green: the `NoSuchElementException` propagates with the `TimeoutException`
as its single suppressed exception, and the held-open probe verticle's stop
was requested, proving the teardown reached `vertx.close()`.
2. Engine ownership. The three-arg constructor's javadoc says to use it to
share a Vertx runtime with other systems, but the teardown closed the
`Vertx` and `WebClient` unconditionally - so closing PC closed the caller's
shared runtime. Ownership now follows construction: two final flags,
`ownsVertx` and `ownsWebClient`, decided in the one constructor that builds
(`null` in means PC builds and owns that one; an instance in stays the
caller's), and `releaseOwnedVertxEngine` closes only what the flags say PC
built. Two flags rather than one because the two are supplied independently
and the mixed case is real: `JStreamVertxParallelEoSStreamProcessor`'s
one-arg constructor passed `(null, null)`, while this class's one-arg
constructor passed `(Vertx.vertx(), null)` - a caller's engine with a
PC-built client. That one-arg constructor now passes `(null, null)` too, so
its engine is built by the same path, is owned, and gets the
worker-pool-sized `VertxOptions` the three-arg constructor computed but,
for that path, never applied. Every constructor and the two factory
interfaces (`VertxParallelStreamProcessor.createEosStreamProcessor`,
`JStreamVertxParallelStreamProcessor.createEosStreamProcessor`) route to
one of these, so the flags are right for each. Constructor javadoc states
the contract one sentence each way, on both classes.
Red: `VertxCloseLeavesACallerSuppliedEngineRunningTest` against an
unconditional teardown - all 7 cases (every `DrainingCloseable` entry point
plus the mixed case) failed with
org.mockito.exceptions.verification.NeverWantedButInvoked:
webClientBase.close(); / vertx.close();
Green: none of the six entry points touches a caller-supplied engine or
client; a client PC built on the caller's engine is closed (the next
request is refused with `IllegalStateException: Client is closed`) while
the engine underneath stays running.
3. Timeout. `close(Duration, DrainingMode)` no longer honoured its `Duration`
for the Vert.x wait, which was bounded by `options.getShutdownTimeout()`
because the base class's recorded timeout was private - defended in a
javadoc as a deliberate trade-off. The preceding commit exposes it through
`@Getter(PROTECTED)`, so the wait is now bounded by `getShutdownTimeout()`:
the caller's `Duration` when they gave one, the configured default
otherwise. The trade-off javadoc is gone with the trade-off.
Red: `VertxCloseReleasesEngineTest.closeWithADurationBoundsTheWaitForTheEngineByThatDuration`
against the `options.getShutdownTimeout()` bound, with `close(300ms,
DONT_DRAIN)` on an engine held open by a verticle that never completes its
stop -
expected to be less than: PT5S
but was : PT10.078311S
Green: elapsed is a few hundred milliseconds.
Test shape. The engine is now observed through public API rather than a spy,
because Mockito cannot get in front of an instance the processor builds for
itself: an `EngineProbeVerticle` deployed on the engine under test has its
`stop` run when - and only when - the engine begins closing, since Vert.x
undeploys every verticle before shutting down. The same verticle, told to hold
its stop promise, makes `vertx.close()` hang on real API, which is the
sleep-free timeout seam items 1 and 3 needed. `Vertx#nettyEventLoopGroup()`
would have served but is deprecated in 4.5.31. The processor gains
package-private getters for `vertx` and `webClient` for these same-package
tests (the base class's `@Setter(PACKAGE)` precedent, and its reasoning: not
PROTECTED, so no test-only seam on a public class's extension surface).
`VertxCloseTestBase` holds the shared `CloseEntryPoint` enum, the probe, and
the never-started-drain tolerance, so the two ownership halves do not restate
each other.
Also on review:
- `VertxBaseUnitTest` closes the engine and client it builds for the sibling
tests, after the processor closes - whoever builds, closes, and the
processor now correctly leaves a supplied engine alone. The engine goes
second so nothing in flight is cut off under a still-closing processor. Its
`initAsyncConsumer` now carries the typed signature the abstract base
declares, which removes the unchecked-conversion warnings the raw one
produced in every subclass. `VertxBatchTest` runs 49-56s both before and
after this change (measured at the branch tip with the change stashed:
51.4s), so its length is pre-existing.
- The `@AfterEach` in the close tests catches `NoSuchElementException` only -
the exception the never-started-drain defect actually throws - instead of
`Exception`, so any other teardown failure fails the test again.
- That defect's diagnosis is no longer restated in the test javadoc: one
sentence and a pointer to
docs/inflight/bug-close-drain-first-on-a-never-started-instance.md, which
arrives with branch fix/close-shuts-down-worker-pool (commit de616c4 holds
it until then; the javadoc carries the `git show` form so
`bin/check-file-refs.sh` accepts the path).
- The rationale for the `createVertx`/`createWebClient` seams lives once, on
`VertxBaseUnitTest`; the overrides in the close tests say only what they
substitute and why.
Warnings on lines this commit wrote, for `bin/check-pr-analysis-surfaces.sh`:
NullAway reports the `null` arguments in the one-arg constructor's
`this(null, null, options)`. That is the shape `JStreamVertxParallelEoSStreamProcessor`'s
one-arg constructor already had (same warning, pre-existing), and the
repository has no nullability annotation library on its classpath to declare
the parameters `@Nullable` with, so the contract is stated in the constructor
javadoc instead. Left deliberately rather than papered over.
Verified: `bin/ci-unit-test.sh` green across the reactor (core 691 run, 8
skipped - the pre-existing quarantine/disabled set; vertx 36; reactor 23;
mutiny 18; examples all green); `bin/check-all.sh` green except
`check-issue-refs.sh`, which fails on a bare `#116` in the current PR body and
is fixed with the body, not in this tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
added a commit
that referenced
this pull request
Sep 6, 2026
Inherited #439, #445 and #442. The first and third matter to this branch: they fix the `dups: clones` lane that was blocking it, and they do so with a cause this branch got wrong. THE DUPS NOTE THIS BRANCH ADDED IS DELETED, NOT UPDATED `ci-dups-clones-lane-times-out-and-blocks-every-merge.md` described a problem #442 has fixed, so keeping it - even rewritten as a was-fixed narrative - is what `docs/inflight/AGENTS.md` forbids. Nothing in it survives as both true and unowned: master's workflow now carries the explanation at the point of use, and states it better. **Its causal claim was also wrong, which is the part worth recording.** The note attributed the timeout to corpus growth from #419 and #116, reasoning from a clean date boundary - every attempt on 2026-09-03 passed, every attempt on 2026-09-04 failed, corroborated on an unrelated PR. The mechanism was right (the job exceeding its five-minute cap with jscpd still running) and the attribution was not. The real cause, per #442's workflow comment: the action installed jscpd with `npx jscpd@4` inside the step it measures, so the base-branch engine paid a cold install the PR-branch step did not - 288s against 15s on the same tree - and npm's 300s default fetch-timeout let one stalled registry request consume the whole budget. The date boundary tracked registry conditions, not master's content. That is a reasoning failure worth naming rather than quietly dropping: a correlation across two branches felt like a controlled comparison, and it was not - both branches shared the runner and the registry, which is exactly the variable that moved. The control that would have settled it was re-running an OLD head, which this branch never did. THE FLAKE LEDGER'S REGISTRATION-RACE ROW, CONFLICTED AND UNIONED Both sides had grown it independently and neither is a superset. Master's version carries four #442 sightings - two of them on documentation-only heads - and counts a `ce-optimize` sighting where this branch counts #433; those are different runs, so the count is their union rather than either side's. This branch's WITHIN-branch control is restored, because master's row never had it and it is the strongest evidence in the row: the same tree produced both outcomes. Master's documentation-only heads are that same argument from the other direction, so they are kept beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3KnLcX4KibVMCP6sMfNgM
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…per when #116 merged #216 asks for gauges over the buffers that grow when the user stops draining them, and had no note in this directory - so a session scanning docs/inflight/ saw nothing, and the reasoning lived only on the tracker. The note deliberately does not restate the issue. What it carries is the part the issue cannot know: three of the four premises it reasons from stopped being true when #116 merged, and each moved in the issue's favour. The backing collection is a LinkedBlockingQueue rather than a ConcurrentLinkedDeque, whose size() is a maintained counter, and the field's own javadoc names the gauge as why it was chosen - so the issue's central design caveat about keeping a counter alongside the collection is dissolved. The close-time WARN a comment describes as "what is left in its place" never shipped; the merge removed it with the other symptom mitigations, so there is now no signal at all between the backlog growing and the heap ending. And the JStream API is no longer deprecated, so the argument that a repair could not earn its place on an API being deleted has lost its premise. Still true, and stated so nobody reads the above as the issue being resolved: the queue has no capacity by design, so a consumer merely slower than the producer still grows it, and observability remains the only mitigation available. Both claims are cited to sources that state them from their own side - the javadoc on userProcessResultsStream and #116's squash commit - rather than asserted here, because this note goes stale the moment either changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132Mx5qpSYmhDEref6Fgxjn
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
The ERROR line PC logs when an asynchronous offset commit fails
("Error committing offsets: ...") interpolated the whole
Map<TopicPartition, OffsetAndMetadata>. Each entry's metadata is PC's
base64-encoded offset map, up to OffsetMapCodecManager.DefaultMaxMetadataSize
characters per partition, so the line grew to partitions x 4KB and log tooling
truncated it on precisely the occasion an operator needed it whole. This is the
third instance of the class #203 fixed the first two of.
For a user: the line now reads
Error committing offsets: 2 partitions: orders-0: offset 1000, 812 chars of
metadata; orders-1: offset 5, no metadata, exception: ...
naming EVERY partition and its offset - which is what #168
(confluentinc#629) asked for, and why no partition cap is applied here even
though the batch renderings in the same class carry one - and reducing only
the metadata to its length, which is itself the diagnostic when a commit is
rejected for its size. The unabridged map is logged at DEBUG immediately after
("Failed commit in full: ..."), where it has to be asked for.
Rendering lives in the shared RecordBatchSummary (new summariseCommit), not a
second format string; its comparator is generalised so both renderings sort
the same way, and the class javadoc now says which rendering caps partitions
and which deliberately does not. The callback's behaviour is unchanged; the
TODO about marking async commits successful before the broker answers is a
separate recorded bug.
Tests read the emitted line through LogCapture rather than the format string,
so re-interpolating the map fails a test. The new
ConsumerOffsetCommitterAsyncFailureLoggingTest was observed red against
master's line with the whole 4096-char metadata in the failure output. Three
mutations then proved the guards: dropping the sort, moving the DEBUG dump
outside the exception guard (which would put the whole map on the hot path of
every successful commit), and dropping the exception argument each failed
exactly the assertion written for it. The line-length bound is derived from
what the rendering emits rather than read off a run. Two concurrency hazards
were found and recorded at the annotations: two captures of one logger race on
LogCapture's level restore, so the capturing methods take SAME_THREAD; and
DefaultMaxMetadataSize is a mutable static two back-pressure tests write, so
readers take the READ side of their resource lock.
This corrects a claim the issue itself makes. Its Fork status section says
there is no equivalent gap on the sync path; there is. In PERIODIC_CONSUMER_SYNC
a RebalanceInProgressException or a CommitFailedException is logged as "Offset
commit deferred (postponed, not dropped)" naming no partition and no offset at
all. The reporter's mode was the asynchronous one, so the fixed line is the one
their scenario hits, and the sync gap is recorded in
docs/inflight/bug-deferred-commit-warn-names-no-offsets.md rather than fixed
here: the offsets are a local one frame down, so the clean fix needs a field,
and #352 is already adding exactly that field to that method. The
drafted reply in docs/inflight/issue-response-168.md carries the correction to
the issue and is deliberately not posted by this merge.
Riding along, unrelated to the log line and doc-only:
docs/inflight/core-216-unbounded-buffer-gauges.md gives #216 the
in-flight note it never had, so a session scanning that directory finds it
rather than needing to think of querying the tracker. It records the part the
issue cannot know - three of the four premises it reasons from stopped being
true when #116 merged, each in the issue's favour. The result queue is
now a LinkedBlockingQueue whose size() is a maintained counter, and the field
javadoc names the gauge as why it was chosen, dissolving the issue's central
design caveat; the close-time WARN a comment there calls "what is left in its
place" never shipped, so nothing now sits between the backlog growing and the
heap ending; and the JStream API is no longer deprecated, so the argument that
a repair could not earn its place on a doomed API has lost its premise. What
has not changed is the unboundedness itself, so observability remains the only
mitigation.
Also records a second sighting of ManagedPCInstanceLifecycleTest's
rapidToggleShouldNotCreateDuplicateInstances, which took this branch's
integration lane red once and passed on a re-run of the same head. It is
cleared on mechanism - the changed line is inside the async callback's
exception guard, the symptom is zero records consumed after toggling an
instance - and what the sighting adds is the condition the two share: both
fired on the first run of a branch that had just taken a change to how that
lane runs, which makes the test a load-shape canary rather than a random
flake.
The upstream manifest's upstream-pr-log-noise entry moves to `merged` in this
branch rather than after it, which is the rule docs/upstream.md states and the
merge gate enforces: branch content is invisible until it lands and correct the
instant it does, so recording it afterwards means a commit straight to master.
`merged` describes the fork work that has shipped, not the family closing - the
confluentinc#57 umbrella is untouched and the deferral siblings wait on
#352.
Upstream-Issue: confluentinc#629
Forwarded: no
Applied-Upstream: no
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…ollections
THE CONTRACT IT PINS. WorkContainer's class javadoc now states that equality is identity while
compareTo orders by topic, partition and offset, so the two are deliberately inconsistent - and that
a raw container may therefore not be an element or a key of a sorted collection. Comparable only
RECOMMENDS the consistency; SortedSet and SortedMap REQUIRE it, because a sorted collection decides
"is this already here" through the comparator, so it would find one flight of a record under another
flight's coordinates. That is the lost-record defect this branch fixes, reached from the other side.
Nothing violates it today, which is exactly why prose was not enough: RetryQueue sorts a
NavigableMap keyed on its own WorkContainerSortKey and ProcessingShard's workMap is keyed on the
offset, and both are one refactor away from being written the other way round, at which point
nothing goes red.
WHAT THE RULE READS. Every field, method return type, method parameter and constructor parameter in
main code whose declared type erases to something assignable to SortedSet, NavigableSet, SortedMap,
NavigableMap, PriorityQueue, ConcurrentSkipListSet or PriorityBlockingQueue - which covers TreeSet,
TreeMap and ConcurrentSkipListMap without naming them - and whose FIRST declared type argument
erases to WorkContainer, via JavaParameterizedType.getActualTypeArguments().
Argument 0 is the element-or-key position for every one of those types, which is why one index
serves the set-shaped and map-shaped ones alike, and why a container in a sorted map's VALUE
position passes. Checking every argument instead would report ProcessingShard.workMap, be red on
arrival against correct code, and be deleted rather than obeyed.
RED PROOF, on main code. Adding
`private final java.util.TreeSet<WorkContainer<?, ?>> tripwire = new java.util.TreeSet<>();` to
ProcessingShard and changing nothing else:
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
Architecture Violation [Priority: MEDIUM] - Rule 'no field, return type or parameter may be a
sorted collection whose elements or keys are raw WorkContainers, ...' was violated (1 times):
field bz.stub.parallelconsumer.state.ProcessingShard.tripwire is a java.util.TreeSet ordered by
a raw WorkContainer - its compareTo orders by topic, partition and offset while its equality is
identity, so a sorted collection would treat two flights of one record as the same element.
Exactly one violation, and the field one line above it - workMap, a NavigableMap holding containers
as values - was not reported. So that run is the positive and the negative control at once, on the
real tree. The field was then removed.
THE STANDING PROOF, because a measurement in a commit message protects nothing. A rule designed to
be green forever cannot distinguish "the code complies" from "the walk sees nothing" on its own
runs, which is the recurring ArchUnit failure recorded in
docs/inflight/static-archunit-main-code-rules.md. So the rule ships with the same shape
RebalanceCallbackRuleControlTest already uses: archfixture/SortedCollectionOfWorkContainers declares
the violation in all four positions plus the legitimate map-value case, and
WorkContainerSortedCollectionRuleControlTest hands it to the real rule object - not a copy - and
asserts it reports the four and not the fifth. The fixture is hand-imported because the rule's own
@AnalyzeClasses carries DoNotIncludeTests, which is what keeps a deliberate violation from turning
the production rule permanently red.
THE GAPS, stated in the test javadoc rather than left to be discovered:
- A sorted collection built inside a method body and never stored in a typed field or signature
declares nothing, so it is invisible. The rule covers the shapes that outlive a stack frame.
- The rule cannot see the comparator. A Comparator-taking constructor makes such a collection
legitimate, but only if the comparator is not compareTo - the requirement is that the ordering
agrees with equality, and equality is identity. The declaration is reported either way, so the
exemption costs a conversation instead of passing silently.
- It runs where core's test classpath reaches. The other modules' main code is not on it, and
covering them needs a wrapper per module in the TestConventionRules shape. Measured before
deciding against it: no main source outside core declares any of the seven types at all, and the
three non-core files naming WorkContainer pass it to a user function.
Every target is a Class literal rather than a string, so the rule cannot silently match nothing
after a rename - the failure mode that left both shard-map rules green when pointed at names that
do not exist.
DOCS, two edits.
docs/inflight/test-857-churn-storm-async-stalls.md: the 2026-09-07 sighting names this PR, and
check-branch-self-reference asks for an attestation that the sentence reads correctly after the
merge. It does - it is past tense about a permanent PR - so the paragraph takes the block marker.
docs/refactoring.md, "Breaking changes queued for next major version": the section's header did not
say what happens to an entry once its work lands, and four entries sat in it marked DONE. The rule
is now stated - an entry is deleted in the PR that lands it, because the commit message carries the
release-note content and the section only lists what is still QUEUED - and applied: the four DONE
entries are removed, three of them naming merged PRs (#267, #201 and #116,
each confirmed MERGED) and the fourth this branch's own.
That fourth entry was the recorded home of the RecordContext equality break, so the two citations
pointing at it are repaired rather than left dangling: WorkContainer's class javadoc and the
solutions write-up both now name the commit as the record, which is what the release notes are
generated from. The break itself is stated plainly in e491797's own body and in the PR
description.
VERIFICATION. Targeted run green (9 tests: the new rule, its two controls,
ShardStaleSweepReplacementEvictionTest and ArchitectureTest). Full unit suite green across every
module (bin/ci-unit-test.sh). bin/check-all.sh: 16 gates passed, none failed. bin/check-issue-refs.sh
and bin/check-file-refs.sh clean, the latter across the whole tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…a week ago (#490) Working docs/inflight/test-untracked-ci-flakes.md, undiagnosed row first. Four rows retired, one new instance found and fixed, four left open with the reason stated in each. THE UNDIAGNOSED ROW WAS NOT UNDIAGNOSED, IT WAS STALE processInKeyOrder failing its own `[sanity check input data]` has a mechanism, a fix, and a measurement - all of them on master since 2026-09-02. The wait in front of that assertion counted CONTROL loop cycles, while `polled` is filled by the POLL thread from a doAnswer on consumerSpy.poll; nothing orders the two, so the loop can turn before the poll thread has been scheduled and the list is empty. All-or-nothing, because MockConsumer hands over the whole batch at once. #29 replaced the cycle count with a wait on the data (`551f40011`, merged as `a6941020f`), measured matched-pair at 5 failures in 31 versus 0 in 31. The provenance check is what settles the row rather than another measurement: EVERY recorded sighting predates that commit reaching master, including the one whose control arm reddened "unmodified master" - that arm was at `b2e6c190d`, five days before the fix landed. There is no sighting on a tree carrying it. A second, branch-side ledger of this same failure exists on refs that never merged (`test-processinkeyorder-sanity-check-races-the-first-poll.md`, findable with `node bin/inflight.mjs prior-art`), which is why the working tree looked like it had less evidence than the corpus held. A NEW INSTANCE OF THE SAME CLASS, CAUGHT BY THE CONTROL ARM `551f40011` named the other assert-after-N-cycles waits and left them as master-side hardening tracked in this register. Running the control arm for the above - master with only that await removed - reddened one of them: inFlightMessagesCommittedIfProcessedDuringShutdown, whose `awaitForSomeLoopCycles(2)` stands in for "the record is in flight". Under load close() can find nothing to complete and the commit assertion reads []. It now waits on the user function actually being entered, which is a STRONGER precondition, not a looser deadline: a [] after it means close() genuinely failed to complete in-flight work, which is a product signal. AMBIENTPROBEEXTENSIONTEST - DIAGNOSED IN THE REGISTER, NOW FIXED AND PROVEN LogCapture attaches to a process-global logger, and three headroom methods each assert the exact set of lines they saw while running concurrently, so each captures its siblings'. Control: master, class alone, red 4 of 4. Treatment: same commit plus three @ResourceLock annotations, green 6 of 6. Sabotage-checked per docs/testing-at-write-time.md - deleting reportDeadlineHeadroom's PASSED call from AmbientProbeExtension.testSuccessful reds exactly headroomIsReportedOnAPassingTestToo, so the lock isolates without blunting. A lock rather than @execution(SAME_THREAD) on the class: the two other classes with this shape capture in most of their methods, this one in three of many. The class already used that idiom for ENVIRONMENT_DUMP_LOCK. Never red on CI because the ci profile sets parallel-tests=false, so only local runs were exposed - which is worse, since local runs are what a developer reads. DEFECT-CLASS SWEEP - two classes, candidates named Two test methods capturing the same logger concurrently: every LogCapture.of call site grouped by (test class, captured logger). Only this class was exposed. Dismissed with reasons: ConsumerOffsetCommitterAsyncFailureLoggingTest and LoadFactorCeilingReportingTest (both @execution(SAME_THREAD)); RevokeOnTheControlThreadTest, UserFunctionFailureLoggingTest, RemovedPartitionStateTest and this class's own ProgressProbe capture (one capturing method each). Cross-class collision is excluded by mode.classes.default being JUnit's same_thread. An assertion gated on a cycle count from another thread: both assertThat(polled) sites are awaited; queuedMessagesNotProcessed... was fixed by #101; executorThreadsInterruptedOnShutdownTimeout waits on the latch the user function holds, not on a cycle count, and is left alone. TWO ROWS RETIRED THAT THEIR OWNERS SHOULD HAVE RETIRED The JStream row said outright that its mechanism was #116's and that whoever merged it owned the retirement; #116 merged on 2026-09-03 and JStreamLiveResultStreamTest is in the tree. The ProducerManagerTest row was describing finished work - #265 and #262 both merged. WHAT STAYS OPEN, AND WHY The PIT lane, rapidToggleShouldNotCreateDuplicateInstances, ReactorPCTest and CommitResponseTimeoutSymptomTest keep their rows and their stated reasons. Nothing is quarantined: docs/quarantined-tests.md is empty and none of these has the rate or the diagnosis rule 1 wants. Two new local sightings from this pass's own runs are recorded as their own row before the logs expire - queuedMessagesNotProcessedOrCommittedIfSubmittedDuring- Shutdown and closeAfterSingleMessageShouldBeEventBasedFast, one sighting each, which is a sighting and not a rate. THE SABOTAGE TOOK FOUR ATTEMPTS, AND THE FIRST THREE WERE FALSE NEGATIVES The shutdown fix in the previous commit needed the mandatory break-it-first check from docs/testing-at-write-time.md, and the first THREE main-code mutations left inFlightMessagesCommittedIfProcessedDuringShutdown green. Taken at face value that reads as "this test asserts nothing about committing". It does not - the mutations had not reached the commit: 1. closeDrainFirst() -> DONT_DRAIN. Never ran: the test calls close(), which IS closeDontDrainFirst() already, and DONT_DRAIN finishes in-flight work anyway. 2. The close-sequence call to commitOffsetsThatAreReady() removed. Green. 3. commitOffsetsThatAreReady() neutered outright. Green - and this one printed a marker proving it ran, so the offset this test asserts is not committed by that method at all. 4. AbstractOffsetCommitter#retrieveOffsetsAndCommit's call to commitOffsets removed. RED on all three parameters, on the assertion under test. The marker is the whole point. Without it, three green sabotages would have made a strong and wrong case that the test was dark, and I would have recorded a defect that does not exist. The honest reading of a green sabotage is "I have not yet shown the test still guards its property", never "the property is unguarded". Recorded in the solution doc rather than in a commit nobody greps, with the attempts that failed kept rather than dropped - a reader who tries mutation 2 or 3 on this test will otherwise reach the same wrong conclusion. The register's entry now points at that section before anyone touches the test. Also adds the matched-pair number for the shutdown fix itself: red twice in fourteen class runs under heavy load on the unfixed arm, none in fourteen on the fixed one, the fix being the only difference between the two trees. Small n, said so - the fix rests on the mechanism and on mutation 4, not on that count. THE NEW SIGHTINGS ROW WAS OUTSIDE THE TABLE IT WAS ADDED TO A blank line sat between the last existing row and the row added above for the two shutdown-path sightings, so GitHub rendered it as a separate one-row table with the sighting text as its header: present in the file, absent from the register a reader sees. Nothing checks markdown table continuity, and the added row looks correct in a diff, which is how it survived a review. That row's reason for recording rather than diagnosing also said "the seeds die with the logs", borrowed from the chaos suite; these are MockConsumer unit tests with no seed, and what expires is the local run's own output. VERIFIED AFTER THE MERGE, WITH THE FAILING CONDITION ACTUALLY PRESENT Both fixed tests were looped on the merged tree - AmbientProbeExtensionTest class alone and ParallelEoSStreamProcessorTest#inFlightMessagesCommittedIfProcessed- DuringShutdown - twelve alternating rounds before the master merge and ten after, green every time on both. Run WITHOUT -Pci on purpose: the `ci` profile sets parallel-tests=false, so under it JUnit thread parallelism is off and the AmbientProbe defect cannot occur - a green there would have proven nothing. The surefire reports were read rather than the exit codes trusted, confirming each round reached 28 methods and 3 parameters instead of no-opping. The class-scope SpotBugs finding attributed to this PR's diff was checked rather than assumed inherited: its annotation spans lines 67 to 1729, the whole class, and its cause is a `org.mockito.internal` static import written in 2021. The same finding fires on JStreamParallelEoSStreamProcessorTest, which this branch does not touch. Left alone. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This fixes the reported behaviour.
astubbs#122(mirrorsconfluentinc#912) was an OOM fromvertxHttpReqInfoStream, and the cause was not the one the earlier revision of this PR described.The bridge from the result queue to the returned
Streamended the stream the first time the queue polled empty:Spliterator.tryAdvancehas two outcomes — deliver an element, or returnfalse, which means no more, ever. There is no way to say "nothing right now". So returningfalseon a momentary gap reports permanent completion: the caller'sforEachfinished almost immediately, usually before the first record had even been processed, and everything produced afterwards accumulated behind a consumer that had already walked away.That is the leak. It was never a backpressure problem — nobody was draining the queue at all.
The fix
tryAdvancewaits for the next result, and reservesfalsefor genuine completion. That is the only shape theSpliteratorcontract permits for a live source, and it is whatStream.generate(queue::take)does.isClosedOrFailed(), so it covers a normal close and the control thread self-closing on an error.LinkedBlockingQueue, so waiting parks instead of spinning — and itssize()is O(1), unlikeConcurrentLinkedDeque's.What this removes
An earlier revision of this branch added a clear-on-close, a close-time warning and a deprecation. All are gone.
Clearing on close never fixed the reported OOM — that happens long before close. It only mattered if something still referenced the processor or the spent stream afterwards, and it cost a null-into-
NONNULLrace, a "best-effort on a failed close" caveat, and a shutdown path it silently did not cover. It was a mitigation for a symptom of the broken stream. With the stream fixed there is nothing to clear.The deprecation is removed from all four types and from the docs, and that is now settled rather than deferred: the JStream API is not being deprecated, and its queued removal is withdrawn. The removal was queued while the API was broken in the way described above — deprecating something because it does not work is a different argument from deprecating something that does, and only the first was ever made. It works now, so it stays.
docs/refactoring.mdanddocs/data/roadmap.yamlrecord the withdrawal.Verification
Java8StreamUtilsTest— the bridge, at its own level. An empty queue does not end the stream while the source runs; queued results are still delivered when it does; a result queued in the window between the poll giving up and the source reporting finished is still delivered; an interrupted consumer ends its stream leaving the flag set; the stream is not splittable.JStreamLiveResultStreamTest— the same properties through a real processor. A consumer that starts before any result exists still receives results produced later; a consumer parked waiting is released by close; and the stream ends when the control thread closes itself on an unhandled error, with no caller having called close — the case the completion predicate exists for, and the one a poison pill could not cover.The empty queue is established, not raced for: the user function is held shut until the consumer is demonstrably parked, so no result can exist before the consumer has met an empty queue.
Confirmed RED, and reproducible rather than asserted. Restore the old early-terminating behaviour — make
tryAdvancereturnfalseas soon as a poll comes back empty — and the regression tests in both classes go red, two of them by timing out on a consumer that never parks:The wait that fails names itself:
Condition with alias 'the result consumer to park on an empty queue' didn't complete within 10 seconds— because against the old bridge that consumer isTERMINATED, not parked.Three branches whose failure mode is a silently dropped result are mutation-checked individually, because a green test that never reaches its branch proves nothing: deleting the second poll's delivery, swallowing the interrupt flag, and removing the completion signal each turn the matching test red. One mutation deliberately survived and is recorded on the test: cutting
isClosedOrFailed()down tostate == CLOSEDleaves the self-close test green, because the supervisor runsdoClosefrom its own catch block before rethrowing. That test pins "something must tell the stream the source is finished when no caller can" — not which half does it.The breaking change, and how it ships
The returned
Streamwent from "returns almost immediately" to blocks until close. That is a user-visible break on a published API, and it ships as one:0.6.0.0is a breaking major and is the release being cut, so it is recorded indocs/refactoring.mdunder Breaking changes queued for next major version — the section the release notes are assembled from.No compatibility path, deliberately. No bounded/timeout overload, no long-wait WARN, no version-gated opt-in. The old shape did not deliver the caller's results, so there is no correct behaviour to preserve and nothing to fall back to. Callers consume on their own thread, as the Vert.x example now shows.
A behaviour change worth reviewing
Eight existing tests across core and vertx collected the stream on the calling thread and asserted a size. They passed only because the stream quit early — they encoded the defect. Each now closes first, and says so at the site. One (
testVertxFunctionFail) readworkRemaining()after collecting; closing drains that away, so it reads while the processor is still running.The example returns to
vertxHttpReqInfoStream, the call the report was filed against, consuming on its own thread withclose()joining it.A second workstream rides along: the pre-fork branch accounting
This is separable from the fix and did not have to land here — it is recorded plainly rather than buried, because a reviewer should decide whether it belongs. It began as one line of merge hygiene and grew: retiring this PR's own deprecation raised the question of what
origin/refactor/deprecate-jstreamwas, which turned out to be a 2022 branch with live citations and no ledger record — the exact gap the fork-wide branch audit exists to catch.What landed as a result:
originnow has abranch_accountingrecord insrc/docs/development/upstream-map.yaml, read from its commits rather than guessed from its name, with aclasssaying whether the idea already landed.grep 'class: open-idea'there, orbin/inflight.mjs branch <name>from any checkout.refactor/deprecate-jstream(tag pushed and verified at the exact tip before deleting) andrefactor/controller-extract-base(an ancestor of master, so nothing could be lost and no tag was needed).core-189-batch-failure-granularity.md, which described work "started and abandoned" without naming the branch that holds it.src/docs/**, so a ledger that quotes// todocomments as evidence was indexed as if it contained them.docs/inflight/ci-hygiene-gaps.mdrecords the larger gap that let it reach CI —bin/check-all.shcannot reachtodo-index.sh --check, and could not simply be given it, because run bare that script rewrites the index.Split it out if you would rather — it touches no main code and no test, so lifting it is mechanical.
Other instances of the same defect — none, and here is where I looked
The defect class is a
Spliteratorreporting permanent completion to signal a momentary gap — treating "nothing right now" as "no more, ever". Two candidates in the tree, both ruled out:PollContext.spliterator()delegates toIterable.super.spliterator()over an already-materialised result set. The defect needs a source that is still producing; a finished collection has none.ConsumerOffsetCommitter.maybeDoCommit()pollscommitRequestQueueand gets null when no commit was requested. It guards withif (poll != null)and simply returns, and the control loop calls it again next cycle — null is read as "nothing right now", which is the opposite of the defect.No other
Spliteratorimplementations and no other queue-to-Streambridges exist insrc/main.Still open
The queue has no capacity, so a consumer genuinely slower than the producer still grows it. What is fixed is the consumer that walked away.
LinkedBlockingQueueaccepts a bound whenever someone wants to decide what should happen when it is reached.docs/inflight/bug-vertx-close-skips-engine-cleanup.mdrecords a pre-existing, unrelated defect found on the way:close()andcloseDrainFirst()never reachwebClient.close()/vertx.close(), which live only on theDuration-taking override.Issues
Fixes #122.
The keyword is qualified deliberately —
Fixes astubbs#122closes nothing.It was withheld until the record on that issue was corrected, because the issue's last word contradicted this PR. It had been closed as won't-fix on the reasoning that the growth is inherent to a pull-based
Stream, then reopened, and its "Fork status" still described the clear-on-close that this PR removes. Closing it on merge would have left "not a fix" standing as the permanent explanation of the change that fixes it.The correction is now posted there, including the part worth reading: that same won't-fix comment already contained the real diagnosis, filed as a secondary "trap worth knowing about" — the stream ending on the first transiently-empty poll. It was the whole defect, not a second one. The headline reasoning was wrong; the aside was right.
Checklist
result-models.yaml, the in-flight note, and the decision records: the v6 break indocs/refactoring.md, the withdrawn deprecation indocs/data/roadmap.yaml, and both SpotBugs findings answered instatic-spotbugs-rule-registry.mdN/A - no CI or runner changes