Repository navigation
fix(offsets)! astubbs#197: the metadata policy governs every unreadable payload, and defaults to IGNORE - #207
Merged
Merged
Conversation
…eadable commit metadata
ParallelConsumerOptions#invalidOffsetMetadataPolicy exists so a consumer that
finds commit metadata it cannot read can be told to drop it and carry on rather
than die. It was unreachable for exactly that case.
EncodedOffsetPair.unwrap resolved the leading magic byte through
OffsetEncoding.decode, which threw a bare RuntimeException("Unexpected magic:
...") when the byte matched no known encoding. That happened *before* the policy
was consulted - the policy was only read further down, inside
getDecodedIncompletes. So a PC reading a commit written by a NEWER PC (a magic
byte that did not exist when this version was built) died on partition
assignment no matter how the option was configured. Confirmed by probe against
master with invalidOffsetMetadataPolicy(IGNORE) set:
RuntimeException: Unexpected magic: -128
The switch's `default ->` branch had the same shape, throwing
UnsupportedOperationException("Encoding (...) not supported") without consulting
errorPolicy - reachable today via OffsetEncoding.ByteArray, which has a magic
byte but no decoder. Same probe:
UnsupportedOperationException: Encoding (ByteArray:v1) not supported
Meanwhile the KafkaStreams branch immediately above did honour the policy, which
is what the intended design looks like.
This matters now rather than later because the fork owns this wire format. Only
a version that ALREADY has this fix can degrade gracefully when it meets a new
encoding, so the fix has to ship before the encoding that would trigger it -
every release without it is a release that will hard-fail on downgrade or in a
mixed-version consumer group.
What changed:
- EncodedOffsetPair.decodeToIncompletes is the new production decode entry
point, and every "this build cannot read this payload" path - unknown magic
byte, known-but-undecodable encoding, Kafka Streams metadata - now goes
through one handler that applies the policy. Under IGNORE it warns with the
magic byte / encoding, the partition and the base offset, and falls back to
HighestOffsetAndIncompletes.of(baseOffset), i.e. treats the metadata as
absent. Under FAIL it still throws.
- Typed exceptions instead of RuntimeException/UnsupportedOperationException:
UnknownOffsetMetadataMagicException and UnsupportedOffsetEncodingException,
both under the package's existing EncodingNotSupportedException parent
alongside KafkaStreamsEncodingNotSupported. Their messages name the remedy
(upgrade, fresh consumer group, or set the policy to IGNORE).
- Plumbing: the policy is read per-instance from PCModule options and passed
down explicitly. OffsetMapCodecManager.errorPolicy was a mutable static
written by the constructor, so the last codec manager constructed in the JVM
decided the policy for every other one and leaked across tests; it is now a
final field. The static decode helpers kept for tests default explicitly to
FAIL. TopicPartition is threaded down as diagnostic context only.
Tests: four cases over the two failure paths x both policy values, plus one that
drives the real partition-assignment path (decodePartitionState) to prove the
option actually reaches the magic-byte decode. The unknown magic byte is derived
from OffsetEncoding.values() rather than hard coded, so adding an encoding
cannot silently turn a forward-compatibility test into a test of that new
encoding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 9m 12s —— View job Claude's Review of PR #207Todo list
SummaryThis is a well-scoped, well-verified fix. I traced both bug paths in the diff and confirmed they match the PR description:
The fix funnels all three "can't read this payload" cases (unknown magic byte, known-but-undecodable encoding, Kafka Streams metadata) through one Verification performed
Minor, non-blocking observation
No other issues found. The exception hierarchy placement, lazy |
✅ 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)
|
|
Found while routing the two new unreadable-metadata paths through the shared handler: the IGNORE branch returned HighestOffsetAndIncompletes.of(baseOffset), but baseOffset is the COMMITTED offset - the next offset to be polled, and the one offset we know for certain was not completed (that is why it is the commit point). Claiming it as seen-and-succeeded means offsetHighestSucceeded == committed, so PartitionState#isRecordPreviouslyCompleted skips that record as already done and getOffsetToCommit() returns committed + 1. Discarding metadata we cannot read would therefore silently discard one record along with it. The no-metadata-at-all branch of decodeCompressedOffsets - the same situation, two lines away - always got this right with nextExpectedOffset - 1. All three unreadable-metadata paths now agree with it. This corrects the pre-existing Kafka Streams IGNORE branch too, since all three now share one handler; its two tests asserted the off-by-one value and are updated with the reasoning. Adds an assertion on the real invariant - that discarding unreadable metadata does not advance the commit point. Also keeps OffsetEncoding.decode(byte) source compatible: it is public and previously threw an unchecked RuntimeException, so declaring the now-checked UnknownOffsetMetadataMagicException would break existing callers. Uses @SneakyThrows as the rest of the package already does, so the signature is unchanged and the caller still gets the better typed exception. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Owner
Author
Review responseThanks - and thank you especially for actually running the mutation-style check (short-circuiting There were no inline review threads on this PR, so there is nothing to reply to in-thread; this comment addresses the one observation raised in the review summary, plus a defect I found myself while re-reading the change. 1.
|
|
Claude finished @astubbs's task in 4m 54s —— View job Claude's Review of PR #207 (re-review after push)Todo list
SummaryCommit 77b3892 addresses both points from the earlier review round cleanly. I re-traced the change independently rather than just trusting the write-up, and it holds up. 1. The off-by-one fix is correct. 2. Verification performed
No further issues found. This is a solid, well-verified fix on top of an already well-scoped PR — the author caught and fixed a real bug (a silently skipped record) while re-reading their own change, which is exactly the kind of self-review this size of change deserves. |
Merged
4 tasks done
master's new issue-reference gate (.github/scripts/issue-ref-gate.js, landed in 735b1d3) flags an unqualified #NN below #1000 on added lines, because the fork's numbers sit entirely inside confluentinc's range and a bare number is a coin flip. This doc's added line was the only hit across the branch. #197 is "Release 0.6.0.0 - the fork's first release", which is the one meant. The commit subject and PR title keep the bare `#197` deliberately - the subject convention in AGENTS.md reserves the leading bare number for fork issues and words upstream ones instead, and the gate reads file content, not subjects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 3m 58s —— View job Claude's Review of PR #207 (re-review after push)Todo list
SummaryI re-reviewed this independently from scratch (read the diff against Core mechanism. The off-by-one is correct and I verified it two ways:
Exception design. Plumbing. Verification performed
Nothing further to addBoth points raised in the two earlier review rounds (the checked-exception source-compatibility concern, and the off-by-one the author found on self-review) were fixed correctly in commit |
|
Claude finished @astubbs's task in 2m 30s —— View job Claude's Review of PR #207 (re-review after push)Todo list
SummaryNothing new to review on the substance of this PR. This push ( The offsets/policy diff is identical to what was already reviewed twice in depth on this PR — including the Verification performed on this push specifically
No issues foundThe merge is inert with respect to this PR's content. No new findings; the fix remains correct and well-tested as established in the two prior review passes. |
astubbs
added a commit
that referenced
this pull request
Aug 8, 2026
Same class as the previous commit, three more sites the local gate found once the audit-branch fix merged up: - "#149, #160, #236" - the shorthand after the first qualified number still reads as a bare reference to the gate, and it is right to: nothing about position makes #160 unambiguous. - The salvaged OOM diagnostics carried "PR #49" verbatim from its branch. A salvage is not a reason to import a coin-flip reference, so it is qualified here too. - The plan's dependency diagram had "#106 / #207" for the same reason as the first item. bin/check-issue-refs.sh passes across all 20 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SizQDD2hVUjD7EhESe9Hkb
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
3 of 4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 17, 2026
Four learnings from the PR #306 density work, each grounded against the tree and cross-linked as same-PR siblings: - best-practices/sneaky-thrown-checked-exceptions-defeat-spotbugs-dataflow: @SneakyThrows makes a checked exception reach a catch(Exception) block SpotBugs proves unreachable; key the check on the data that determines the exception (the payload's OffsetEncoding), not the exception's type. - best-practices/benchmark-first-wire-format-decisions: forever-decodable wire formats get measured before they are frozen - minimal layout writers, the identical zstd+string pipeline as incumbents, and a ship threshold at the engagement points where the metadata cap bites. Two candidates got a measured case-against; the delta-list shipped on +19.8-22.7%. - logic-errors/boundary-claim-tested-only-on-friendly-samples: the "Z85 wins from 22 bytes" prose was arithmetically false and the crossover test sampled only agreeing sizes; the fix implemented the documented intent (the 22-byte floor) and pinned the falsifying sizes n=1,4,7,13. Formula claims in prose get the full domain or the exact boundary, never a friendly sample. - logic-errors/all-or-nothing-conditional-registration-suppresses-competitors: zstd twins were gated on NO encoder being small, so one tiny encoding suppressed every other encoder's twin and a better competitor would have made winning payloads larger; fixed per-encoder with the R11 no-regression property test. CONCEPTS.md gains an "Offset metadata encoding" cluster (offset map, offset encoding, outer codec, simultaneous encoding, compressed twin, metadata back-pressure, invalid offset metadata policy), each entry verified against the defining source. Every claim in the docs passed a mechanical path/SHA/link check and a read-only semantic grounding validation against the tree and gh; the three contradicted claims those passes surfaced (report-only SpotBugs job, PR #207 unmerged attribution, the "exactly one character" slip) were corrected before commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SmQxQG496knygJYa5KM6Y
astubbs
marked this pull request as draft
August 19, 2026 03:11
…oad, and defaults to IGNORE Merges origin/master (204 commits) and settles a collision it exposed. WHAT THE MERGE FOUND #217 (ed78d6c, Fixes #118, mirroring confluentinc#326) fixed the same defect this branch exists to fix, independently, and merged first. It deleted this branch's item from the release ledger - docs/inflight/release-0.6.0.0.md item 5, "an OffsetEncoding magic-byte hazard, was fixed in #217". Neither PR referenced the other; nothing linked them in either direction. The two disagree about what invalidOffsetMetadataPolicy MEANS, so the three conflicted files in offsets/ were a design decision rather than a textual resolution: - #217: decode() throws checked OffsetDecodingError, caught by loadPartitionStateForAssignment, offset map dropped. The policy governs only bytes positively identified as Kafka Streams'; its javadoc was narrowed to say undecodable metadata "is never fatal under either policy". - #207: every "this build cannot read this payload" path routes through EncodedOffsetPair#handleUnreadableMetadata, which applies the policy. #207's routing is kept. Item 5 recorded the defect as an older PC dying "regardless of how the policy is configured" - the complaint is that the policy does not govern. #217 made the ungoverned path benign and documented the resulting inconsistency instead of removing it: under its default FAIL, Kafka Streams metadata shut the consumer down while every other unreadable byte silently recovered. WHY THE DEFAULT MOVED - THIS IS THE BREAKING PART Once the policy genuinely governs every unreadable path, the default decides what happens to someone who configures nothing, and that person is the #118 reporter: they pointed PC at a consumer group Kafka Streams had previously owned. Leaving the default at FAIL would have made that exact report fatal again. FAIL stays available and now means what it says - stop rather than silently discard an offset map, because discarding it replays records that completed but were not committed. #217's ForeignOffsetMetadataOnAssignmentTest asserted the old contract directly: that an unknown magic byte does not escape onPartitionsAssigned under an EXPLICIT FAIL. That is the assertion this overturns, so the test was rewritten rather than deleted - the #118 regression is now pinned under the default policy (the reporter's actual configuration), and a second test pins FAIL as genuinely fatal. Also fixed here and still live on master: the IGNORE branch resumed from HighestOffsetAndIncompletes.of(baseOffset), but baseOffset is the committed offset - the next one to be polled. Claiming to have seen and succeeded it makes PartitionState#isRecordPreviouslyCompleted skip that record. All unreadable- metadata paths now agree on baseOffset - 1. TWO OF MASTER'S CHANGES RE-APPLIED ON TOP Not part of the competing fix, and lost by taking this branch's side of the three files, so they were re-applied deliberately: - 6aae299 InternalRuntimeException -> PCInternalRuntimeException. The old class is deleted on master, so the references would not compile. - 7eeecc9 (#121) sample the high-water mark once in encodeOffsetsCompressed, deriving both the incomplete-offsets snapshot and the encoder's range top from it. Dropping this silently loses records. RESOLUTION AUDIT 81 of the 84 conflicts were the two independent package renames cross-pairing the five near-identical TestConventionsArchTest.java files, exactly as AGENTS.md predicts; those took master's state wholesale. Per the --ours/--theirs rule, the whole-file takes were audited rather than trusted: the branch's own delta (merge-base..pre-merge tip, excluding the two pure-rename commits) touches 21 files, 12 of which are the final delta. Of the other nine, two are the rename tooling this branch had taken FROM master, and the remaining seven were verified byte-for-byte present on master - the re-pointed KafkaSanityTests javadoc reference, and the dead csid logger entries deleted in the eight logback files. Nothing of this branch's was silently discarded. Verified: 539 core unit tests pass, 0 failures. bin/check-all.sh is green except check-branch-self-reference.sh, whose 32 findings are all in inflight notes inherited from master and none in this diff. LEFT OPEN, recorded in docs/inflight/pr-offset-encoding-policy.md - FAIL stops the consumer by letting a checked exception escape Kafka's rebalance callback, surfacing as a generic "User rebalance callback throws an error". Opt-in now rather than the default, but a deliberate stop should go through PC's own fatal-error path. Queued in docs/refactoring.md. - The same defect class survives one step further in: a magic byte that resolves fine but a corrupt payload body still bypasses the policy (OffsetRunLength's IllegalArgumentException, OffsetBitSet's "Invalid state", decompressZstd on a malformed stream). Not fixed here - it needs a separate decision about corrupt bodies from recognised encodings. - Back-links, missing in every direction: #207 -> #118 / #217 / confluentinc#326; #118 -> #207; #217 -> #197; and #197's own triage bullet, still carrying item 5 unticked and unlinked even though the ledger file's copy was deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
… not an answer
Extends invalidOffsetMetadataPolicy to the third case it did not reach. It
governed an unrecognised magic byte and a known encoding with no decoder; a
payload whose encoding IS decodable but whose bytes are not something any encoder
here could have produced still went straight to the decoders.
Probing that case found something worse than the escape it was filed as. The
decoders trusted their own header - a length field is read out of the payload and
then used to drive a loop - so metadata PC did not write produced ANSWERS:
BitSet declaring 32767 bits, empty body -> 32767 fabricated incomplete offsets,
every one of them re-delivered
BitSetV2 declaring Integer.MAX_VALUE -> OutOfMemoryError, from metadata any
previous owner of the group can write
BitSet/BitSetV2 negative length -> highest-seen offset BELOW the
committed one
RunLengthV2 negative run -> same, walks currentOffset backwards
RunLength body one byte too long -> asShortBuffer() drops the trailing
partial entry, rest decodes as if fine
Truncated length field -> BufferUnderflowException escaped
onPartitionsAssigned
Compressed body that is not zstd -> ZstdIOException escaped the same way
A wrong offset map is worse than a failed one: nothing downstream can tell it from
a real one, so the outcomes are mass replay or - via
PartitionState#isRecordPreviouslyCompleted - mass silent skipping.
WHAT IS CHECKED, AND WHY ONLY THIS
Only claims the buffer can settle on its own, so nothing here guesses at what a
plausible offset map looks like:
- a declared bit length must be non-negative AND backed by bytes that are
actually present (ceil(bits/8)). This is also what fixes the OOM: 2^31 bits
would need 256MB of body, which is not there.
- a run length is a count, so it is never negative.
- a run-length body must be a whole number of entries. asShortBuffer() and
asIntBuffer() silently drop a trailing partial element, which is exactly how
the odd-length body decoded into five invented incompletes.
Everything they reject, plus BufferUnderflowException and IOException from the
decoders, is routed through EncodedOffsetPair#handleUnreadableMetadata as
CorruptOffsetMetadataException - so it is the same user-visible event, under the
same option, as the two cases already covered.
NOT FIXED, DELIBERATELY
A run length that is structurally valid but implausibly large (RunLengthV2 of
Integer.MAX_VALUE) still moves the highest-seen offset ~2e9 forward. Nothing in
the payload proves it wrong: a long run of completed offsets is what run-length
encoding is FOR, and the decoder has no partition end offset to check against.
Bounding it means choosing a plausibility ceiling, which is a product decision
rather than a correctness one. Queued in docs/refactoring.md
(offsets/OffsetRunLength.java) with the reasoning.
Covered by CorruptOffsetMetadataTest - each shape pinned individually under both
policies, because each one produced a different wrong answer rather than a shared
failure. 559 core unit tests pass.
Also corrects a citation this branch introduced: three offsets exceptions pointed
at InternalRuntimeException for why their constructors are hand-written, and that
class was renamed to PCInternalRuntimeException and deleted in #267.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
astubbs
changed the title
[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 Superseded by a newer quarantine lane report. |
…erent clocks The note named `codecov/project` because that is the one that had gone red. Two more turned up on the same branch afterwards - `codecov/project/unit` and `codecov/project/integration` - and they are the same artefact under names different enough to read as unrelated problems. Each waits on its own suite's upload, so they go red and recover at different moments: seeing the total clear tells you nothing about the flags. The flag pair is the more alarming to look at, and the note now carries the measurement that defuses it. At one point on this branch the total was UP 0.59% - hits +78, misses -14 - while `unit` reported -4.14% and `integration` -16.32%. Sixty-one added lines out of about 4,800 cannot move a flag four points, so a flag delta that size beside a positive total is the baseline talking rather than the diff. All three were green once every upload landed. Written now rather than after the merge because this is the third time in one afternoon that a codecov red has had to be talked down, and twice it nearly sent me looking for a coverage regression that did not exist.
#262's transactional-guarantee register and its two produce-lock guards. Three core main files - PollContextInternal, ProducerManager, AbstractParallelEoSStreamProcessor - and none of them is on this branch's path: this branch changes offset DECODING, and the produce lock is a commit-path concern with no offset metadata anywhere near it. THE CONFLICT, AND WHY NEITHER SIDE COULD JUST BE TAKEN test-untracked-ci-flakes.md, both sides having edited the same table. Each side was stale in a different row, so taking either wholesale would have lost something real: - ProducerManagerTest: MASTER's row wins outright and this branch's is deleted. Ours still said "quarantined, owner #262"; master's records that #265 fixed it by deleting the wall-clock assertion and that #262, as owner, LIFTED the quarantine and deleted the registry entry. The quarantine lane report on this PR was still printing the old state, which is exactly the kind of stale-but-plausible signal this merge corrects. - processInKeyOrder: OURS wins. Master still carries the two-sighting version; this branch merged master's two with its own four an hour earlier, so the six-sighting row across two branches is the superset. - MdcContextPropagationTest: new from master, kept. Its own note owns the diagnosis. - JStream, the PIT lane and ManagedPCInstanceLifecycleTest are this branch's and master has never seen them. Composed rather than resolved by side, which is the only way a shared ledger survives two writers. Verified: unit lane BUILD SUCCESS across all eleven modules, 696 tests over the four library modules, 0 failures. bin/check-all.sh 15/15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
[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. |
…e of its own failure mode
The note was titled "is red for most of a run, AND CLEARS ITSELF" and opened by
saying it turns green on its own, three times observed. Then a fourth red
survived to a terminal run and did not clear - and the section recording that
was appended UNDER a heading still promising self-recovery.
A reader who trusts the heading stops there and dismisses the one occurrence
that is telling them something. That is exactly the misdirection this note
exists to prevent, so the note had become an example of it.
Rewritten around the fact that there are TWO causes with one symptom, which the
earlier revisions conflated into a mechanism section describing only the first:
- mid-run, the HEAD is partial because a suite has not uploaded. Clears
itself when the last upload lands. Wait.
- at a terminal run, the BASE is partial - master's own report is incomplete.
Does NOT clear. Run the files check.
The Files count separates them in one line, so the note now leads with the table
and says to start there rather than deciding which case you are in from the
percentages.
"What to do about it" was also one revision behind its own evidence: it said
"nothing, while a run is in flight - wait for checks_terminal", which is only
half an answer once a red can outlive terminal. It now answers both cases and
says what stays true in either - codecov/patch is the check that answers whether
this PR's own code is covered, and it was green through all four reds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
Audited every note this branch owns against the tree rather than re-reading them for plausibility. Two claims in one note were wrong, both in the direction that matters: they described the PRE-astubbs#207 behaviour, so a reader would have been sent to a route the PR removed. core-bytearray-encodings-have-no-codec.md said the ByteArray pair is "absent from EncodedOffsetPair#decodeBody and falls to the default arm". That was true before. getDecodedIncompletes now carries an explicit arm for both constants routing them to handleUnreadableMetadata, so they never reach decodeBody and its throwing default is unreachable for them - which is the whole point of the change and the opposite of what the note claimed. The same note cited `no advantage over BitSet encoding` as the anchor for the commented-out encoder. That greps to NOTHING. The real text is `there doesn't seem to be an advantage over`, in OffsetSimultaneousEncoder's javadoc. bin/check-file-refs.sh gates the PATH half of a citation and cannot check the anchor half, which is how it stayed broken; the corrected anchor was grepped in both directions before being written. WHAT THE AUDIT CONFIRMED RATHER THAN CHANGED, so the next reader does not redo it bug-no-metric-for-discarded-offset-metadata: the metrics grep it rests on still returns nothing. bug-run-length-plausibility-ceiling: the rejections it credits #207 with are present in OffsetBitSet and OffsetRunLength. core-fail-policy-escapes-the-rebalance-callback: handleUnreadableMetadata does throw a Supplier<? extends InternalException> under @SneakyThrows, and loadPartitionStateForAssignment does catch OffsetDecodingError and drop the map. pr-207-offset-encoding-policy: errorPolicy is a final field, the static helpers still default to FAIL, and IGNORE resumes from baseOffset - 1. docs/features/invalid-offset-metadata-policy.yaml says IGNORE is the default and the code agrees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
…ving asked the question properly The note said the base was "partial" and left it there. Asked why, twice, the answer turns out to be two things, and neither was written down. MASTER'S RUN AT THE BASE COMMIT WAS CANCELLED `gh run list --branch master --workflow CI` shows `completed/cancelled` at the exact commit codecov compares against, superseded by the next push to master. A cancelled run uploads nothing, so codecov has no report for that commit and falls back to older master data - which is why `node bin/inflight.mjs codecov` still reports master at 83 files while the head has 93. That is the missing "why" behind the files tell the note already carried. THE FLAG BASELINE IS NOT THE SAME QUANTITY AS THE FLAG The job uploading `flags: unit` / `integration` is gated `if: github.event_name == 'pull_request'`, so master never publishes per-suite flags at all. Codecov carries forward whatever it last had, and the proof is in its own output: on the default branch `unit` and `integration` both read exactly the TOTAL, 81.45%. A PR's head then reports a genuine unit-only 76.03% and integration-only 64.46% against that, and unit tests alone legitimately cover less than every suite combined. The delta is a category error, not a regression. AND A CLAIM OF MINE THAT WAS WRONG IN BOTH DIRECTIONS The note said all three checks "were green once every upload had landed". Per head, from the check-runs API: green on the head whose base was a master commit with a COMPLETED run, red on the head whose base was the cancelled one. Same PR, same code. So they are neither reliably self-clearing nor permanently broken - the base decides, and the note now says which observation distinguishes them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
…nd got the cause wrong `node bin/inflight.mjs prior-art codecov` returns four codecov notes and marks exactly one as NOT ON origin/master: this one. The other three arrived with #409 in the master merge this branch already carries, hours before the note was written. ci-codecov-flags-not-like-for-like.md owns the flag-set mismatch and its fix in #400. ci-the-coverage-uploads-still-use-the-inert-glob.md owns what is actually wrong with the per-flag gates - the uploader does not expand `**`, so the pattern matches nothing and the CLI falls back to a tree-wide search; master's full build therefore hands the WHOLE tree to both the unit and the integration flag, while a PR's suite job has only its own half. That is why the two flags report an identical number on the default branch, which both that note and this one used as evidence for different conclusions. Its collision check also lists #414, open since before the diagnosis started here, making exactly that fix with `disable_search: true` and expanded globs - and proving it from the upload logs rather than by inference. So the note was a third statement of a fact this repo already had twice, and its own mechanism - "the base report is partial" - was wrong. A partial base is real and separate (a CANCELLED master run uploads nothing, which is why the base totals here read 83 files against a head of 93), but it is not what makes the per-flag gates red. That observation is offered on #414 rather than kept here, because that PR owns the file it belongs in. The rule this broke is in AGENTS.md and I quoted it in a merge commit two hours before ignoring it: grep and find read the working tree, prior-art reads every ref. Both offending notes were IN the working tree by then, so even the weaker check would have worked - what failed was searching at all before writing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
6 tasks done
Four commits, no Java on master's side, so nothing here reaches this branch's code. The one that matters to this PR is #414: the codecov glob fix has landed, so the two per-flag gates this branch has been carrying as red are now fixed at their source rather than merely attributed. #414 ALSO CARRIES THE OBSERVATION THIS BRANCH OFFERED IT Its note now holds "A second, independent way the same gates go red - and the one-line tell that separates them": master's run at beb01e1 was cancelled, so Codecov holds no report for that commit and every PR comparing against it falls back to older master data. That was offered as a comment rather than written into a third note here, and it landed in the file that owns the subject - which is the outcome the never-state-a-fact-twice rule is for. THE CONFLICT test-untracked-ci-flakes.md again, and trivially: master raised simpleBatchTest from four sightings to five. Master's row is the superset and is taken whole; this branch's four rows that master has never seen are kept beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
…control that narrows its hypothesis
The core unit suite failed once on this branch immediately after merging master,
at `anEmptyCallerContextIsHandledAndNothingLeaks` with `expected: null but was:
{}` - the same line and the same message master's note already records from
#262 the day before.
WHY THE SECOND SIGHTING IS WORTH MORE THAN A TALLY
The two branches have no code in common: #262 is transactional, this one
is offset decoding and documentation, and neither touches anything MDC-related.
The note previously had to argue master-state from a neighbouring run of the same
tree; two branches failing the same way is the branch-independence that argument
wanted.
AND A CONTROL THAT BEARS ON THE HYPOTHESIS RATHER THAN REPEATING IT
The class alone passed 5 of 5, on the same tree, minutes after the suite lost.
The note's hypothesis is that a sibling in this class does `MDC.put` and arms the
precondition depending on JUnit's method order - but if that were so, running the
class by itself should still lose whenever a putting method was ordered first. It
did not. One green isolated run cannot prove within-class ordering never arms it,
so the note says that; what it does is point the search at a different class
sharing the fork's JUnit thread, which is cheaper to falsify than the original
wording implies.
The note's "deliberately not quarantined" section now reads two sightings rather
than one. The disposition is unchanged and deliberately so: rule 1 wants a
diagnosis or a ledger, and a second sighting with an untested hypothesis is still
not a diagnosis.
Also attests the #207 reference #414's note picked up from this
branch's comment. The gate's PR-number arm reads whole files by design - a PR
number can only be about its own PR - so master's sentence about this one needs
this branch's attestation, which is exactly the branch that supplied it. The
sentence was already past tense; only the marker was missing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
[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. |
…s fix Master's own `full build (master)` job at ebd8bcb failed: 581 tests, 1 failure, `MdcContextPropagationTest.anEmptyCallerContextIsHandledAndNothingLeaks`, the same assertion the two branch sightings above carry. That ends the master-state argument outright. The note had been making it from branch evidence - #262 first, this branch second, neither touching anything MDC-related - and a failure on master needs no attribution at all. THE PART THAT IS NOT ABOUT THIS TEST The `build` job is the only one uploading coverage on master. It fails here, so nothing uploads. #414 merged the corrected coverage globs and named its own post-merge proof - master's `unit` and `integration` flags no longer reporting one identical figure - and that proof HAS NOT HAPPENED: `node bin/inflight.mjs codecov` still shows both at 79.12%, because the job that would produce the corrected upload has not completed since the fix landed. Three of the four master runs after it were cancelled by the next push and the fourth is this failure. So every open PR keeps comparing against a base assembled the old way and keeps seeing red per-flag gates that read as its own fault. The cost of this test is no longer one confusing red in a suite: it is master red, no coverage upload, a merged fix that cannot take effect, and a misleading gate on every PR. Recorded rather than acted on. Quarantining is still the owner's call and the note's own argument against guessing stands - what changed is the price of leaving it, which the next person deciding should be able to read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
Four commits, no Java touching this branch's paths. Three conflicts, all the same shape as before - two writers appending sightings to the same ledgers - and this time both writers were recording the SAME two flakes, so neither side's count was right and taking either whole would have lost real observations. MdcContextPropagationTest: master had three sightings (one local on #262, two on #416's CI across consecutive heads); this branch had a fourth, its own local suite, and a fifth that matters more than the rest - MASTER'S OWN `full build` at ebd8bcb. Unioned to five, with the master one called out, because it is the one that stops being a test problem: `build` is master's only coverage uploader, so its failure is why #414's merged glob fix has not taken effect and why every open PR still sees red per-flag gates. committedOffsetRemoved[1] latest: master had #407, #409 and #416; this branch had #407, #207 and #409. Union is four, and the row now says which of them captured what - #407 the surviving record, #409 a rate, the other two neither. Master also re-added a processInKeyOrder row at two sightings where this branch already carries the six-sighting version from an earlier merge; its copy is dropped rather than landing a duplicate row under one test name. bin/check-all.sh 15/15, core compiles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
#418 and #203. The first FIXES the MdcContextPropagationTest flake this branch had been recording sightings of, and retires the note they went into - so most of this merge is accepting that master's answer beats what was staged here. THE MDC NOTE IS DELETED, DELIBERATELY, AND NOTHING IS MIGRATED OUT OF IT modify/delete: master retired the note into docs/solutions/test-flakiness/mdc-null-precondition-armed-by-an-earlier-class-in-the-same-fork-2026-09-02.md while this branch was adding a fifth sighting to it. The deletion is accepted whole, and per the four outcomes in docs/inflight/AGENTS.md the question was whether anything in it was still true and unowned. Nothing is: - #418's diagnosis REFUTES the note's hypothesis. The note guessed MDC.clear() leaves an empty map; the real mechanism is MDC.remove(lastKey), with ProducerManagerTest and TransactionalBulkCommitTest leaving {} on the shared runner thread and fork placement deciding the order. It probed that against the build's jars rather than reasoning about it. - The one thing this branch contributed beyond sightings was a control - the class alone passed 5/5, so the polluter is not a sibling method - and that is the conclusion #418 reached by naming the two polluting classes. Superseded, not lost. - The fifth sighting adds nothing to a fixed flake, and master's summary line already carries the one that mattered, master's own build. The ledger's MDC row goes with it: master's "fixed and out" line above the table now owns that test. processInKeyOrder: master's row and section carry a control arm this branch could not run - 1 of 8 failures on a detached worktree at UNMODIFIED master - which is what makes it master-state rather than a branch's problem, plus the same awaitForOneLoopCycle lead this branch reached independently. Master's prose is taken whole and this branch's three load arms are folded in beneath it as corroboration, because they measure the load dependence master's control does not. Verified after resolving: the #197 default flip survived the ParallelConsumerOptions auto-merge (still IGNORE), unit lane green at 709 tests across the four library modules, bin/check-all.sh 15/15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…am-memory-leak Master gained 30 commits since b2e6c19. One conflict, and it is the interesting kind: both sides were fixing the SAME wrong sentence, on different axes. `AmbientProbeExtension.buildAutopsy` used to print "the fault is likely in the test itself, not consumer-group progress" whenever the probe saw nothing. Both sides had already agreed that claim is unearned; they disagreed about why. - This branch (aba0330) found it printing under `ContainerLaunchException: Container startup failed for image confluentinc/cp-kafka:7.9.0` - the group never formed, so the detectors had nothing to SAMPLE. Fixed in prose, and left a TODO(refactor) asking for the distinction to be made mechanically. - Master (#381) asked whether the detectors had TIME to fire, and answered it with code: `appendUnfireableDetectors` resolves the test's own `@Timeout` and names every detector whose bound is at or past that ceiling. Nothing-to-sample and no-time-to-fire are two different reasons a clean probe means nothing, so taking either side alone would have re-armed a wrong verdict on the other axis. Resolved by keeping both: the weaker leading claim this branch established ("nothing in consumer-group progress explains this failure"), master's conditional and its `appendUnfireableDetectors` call, and the container/Docker caveat. Master's "IF YOU TRUST THIS TEST" clause is dropped rather than merged - it qualified the "fault is in the test" claim, which no longer appears. `AmbientProbeExtensionTest` asserts only on the `probe clean - no rebalance dwell...` prefix, unchanged on both sides. `docs/testing.md` auto-merged with both caveats intact and complementary, and its closing sentence still describes what the code now prints. The TODO(refactor) stays: master answered detector REACH, not the "never sampled a group" vs "sampled and saw nothing" distinction it asks for. Also carried in, and this PR now owns retiring it: #207 landed a ledger entry in `docs/inflight/test-untracked-ci-flakes.md` recording `JStreamParallelEoSStreamProcessorTest.testConsumeAndProduce` and `.testFlatMapProduce` failing TOGETHER on produced-record count. That is this PR's defect observed rather than inferred, and the entry says explicitly not to diagnose it separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
6 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
The branch has sat since early August while master gained several hundred commits, including the whole-repo io.confluent.* -> bz.stub.* package rename. The rename had already been applied to this branch by an earlier sweep, so both sides agree on where every file lives - which is what made this merge tractable rather than a rewrite. It still produced 115 conflicts, and all but a handful were artefacts of that rename landing on both sides independently. What master brings that this branch needs ----------------------------------------- #207, the offsets decode-path rework this PR declares a dependency on, is now in the tree: EncodedOffsetPair routes every unreadable payload through handleUnreadableMetadata, and the new typed exceptions (CorruptOffsetMetadataException, UnknownOffsetMetadataMagicException, UnsupportedOffsetEncodingException) arrive with it. Master also fixed three log statements in OffsetSimultaneousEncoder - they now pass the exception rather than its getMessage(), and the ByteBuffer arm no longer names BitSetEncoder in its own warning. All three merged into the branch's rewritten invoke() cleanly. How the mechanical majority was resolved, and why that was safe --------------------------------------------------------------- 82 of the 115 conflicts were proved to be rename-only before anything was touched: our side of each was normalised back to the pre-rename spelling (which has to collapse more than the top-level package, because the rename also folded io.confluent.csid.utils into ...parallelconsumer.internal.utils and io.confluent.csid.testcontainers into ...internal.testcontainers) and compared byte-for-byte against the merge base at that file's pre-rename path. Where the two matched, our side contributed nothing but the rename and master's outcome was taken - master's blob where master still has the path, a delete where master had removed or moved the file. This is the only condition under which taking the other side wholesale is defensible: a merge that takes theirs renders as nothing at all in review, so a silent loss would leave no trace to find. The remaining conflicts were resolved individually: Master's outright renames. InternalRuntimeException -> PCInternalRuntimeException and its test, and LongPollingMockConsumer moving from the test tree into src/main/java/.../internal/utils/. Each of these is a rename/rename conflict, and git's ort strategy had already content-merged them: the stage-2 blob at our path was byte-identical to master's blob at master's path, which is the proof that our side carried only the package rename. Master's path and blob kept; ours deleted. TestConventionsArchTest. Git mis-paired the near-identical per-module copies - it matched mutiny's base against reactor's ours and examples-core's theirs, and reactor's base against vertx's ours and examples-vertx's theirs. A checkout --theirs here writes a file that still contains conflict markers, and the markers carry paths rather than the plain form, so the habitual grep for '<<<<<<< HEAD' cannot see them. Master's blob was taken verbatim at all nine module paths and each was then diffed against master to prove no mis-pair survived - a mis-paired arch test compiles happily while pointing ArchUnit at another module's packages, which is silent loss of coverage rather than a build failure. The eight logback test configs. Our side's only non-rename change was deleting the dead io.confluent.csid logger entries; master made the same deletion in #289 and then rebuilt these files around the pc.log.level property and the shared logback-silos.xml include. Confirmed no csid logger survives on master's side, so master's is a strict superset. bin/check-copyright-headers.sh, bin/ci-mutation-test.sh, bin/rename-packages.sh. The branch took the rename tooling from master; master has since moved on. The copyright script's blob was found verbatim in master's own history, and the other two carried nothing our side originated - ci-mutation-test.sh differed from the merge base by the package regex alone, which master's version already spells bz.stub. Master's taken for all three. .idea/runConfigurations/. Master deleted these; our only change was inside a file that no longer exists. Master's deletion accepted. The two files carrying the branch's own work were resolved hunk by hunk rather than by taking a side. OffsetSimultaneousEncoder.java had a single conflict, on the lombok.AccessLevel import the sparse-path test seam needs - ours kept. RunLengthEncoderTest.java had one javadoc hunk where master had re-pointed the "needs a delta-aware invoke()" tracking citation from docs/inflight.md to docs/refactoring.md; ours kept, because the paragraph master was editing describes a limitation this branch removes. OffsetEncoder.java, RunLengthEncoder.java and OffsetSimultaneousEncoderSparseIterationTest.java did not conflict at all. The result differs from the merged master commit by exactly the branch's own change plus a regenerated docs/todo-index.md - nothing else survived the merge from our side, which is the check that the rename-only resolutions neither lost nor smuggled anything. CHANGELOG.adoc: the branch's entry is dropped --------------------------------------------- Repo policy changed while this branch sat. #113 moved release notes to generation from the commit log at release time, and AGENTS.md's Changelog section now states that a PR never adds an entry and never adds an Unreleased section. The review note on #106 asks for exactly this. Nothing is lost: the generator reads the commit log, and this branch's commit message already carries the full substance - the delta-accumulation argument, the byte-identical guarantee, the differential test, and the RunLengthEncoderTest 62.3s -> 0.2s measurement. README.adoc is generated from src/docs/README_TEMPLATE.adoc, so master's is taken. The template's only non-rename change on this branch - stop embedding the changelog in the README - had already been made independently on master, word for word, so master's side loses nothing there either. docs/inflight.md: master's deletion accepted -------------------------------------------- Master deleted the single-file ledger; in-flight state is now one file per item under docs/inflight/, and that directory tracks only what is OPEN. This branch's only edits to the old file were closing two items out as DONE - the RunLengthEncoderTest ~59s floor, and the delta-aware invoke() follow-up - both of which this PR resolves. A DONE entry has no home in the new directory, and no open item was carried on those lines, so the deletion loses nothing. None of master's docs/inflight/ notes track this mechanism as open. docs/todo-index.md is generated, and bin/todo-index.sh --check went stale because the branch removed two markers from OffsetSimultaneousEncoder.java: the "VERY large offset ranges is slow" TODO this PR resolves, and the "refactor this loop into the encoders" note attached to the loop it rewrote. Regenerated. Verified on JDK 17: whole-reactor ./mvnw clean install -DskipTests succeeds, and the core unit suite runs 833 tests with 0 failures and 0 errors (8 pre-existing assumption-based skips, none added here). The tree carries no conflict markers - swept for the marker anchored at line start, since a rename/rename marker carries paths rather than the plain form - and no io.confluent residue outside the copyright script's deliberate upstream-path tables and AGENTS.md's own prose about the rename. No test was weakened, skipped, excluded or quarantined. bin/check-issue-refs.sh is red on this commit and its findings are all inherited. The gate scopes itself to files changed since the merge base, and mid-merge that base is still the OLD one, so every file master touched reads as a line this branch added - the same artefact bin/check-branch-self-reference.sh documents in its own header ("MID-MERGE IT ACCUSES YOU OF MASTER'S SENTENCES ... the fix is to finish the merge"). Its three tree findings are the bare confluentinc#857 in parallel-consumer-core/src/test/resources/logback-test.xml and the bare #101/#110 in pom.xml; both files are byte-identical here to the master commit being merged, and neither line existed at the branch tip before it. The remaining six are in the PR body, which predates the gate and is the author's to settle. Once this commit exists the merge base is master's tip and the tree arm has nothing to report. Every other gate passes, including the copyright header check run with COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Xx8GAnzc8F7ksrvpYj8hC
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…, and the condition both share The integration lane went red on this branch with one failure, repetition 1 of 5 of rapidToggleShouldNotCreateDuplicateInstances, consumeCount 0 and the probe clean with detector reach UNKNOWN. The ledger already carried one sighting of exactly that shape, so this is the second and it gets recorded before this PR merges, because the evidence expires with the logs. Cleared on mechanism rather than on timing, the way the first sighting cleared #207: this branch's whole main-code change is one argument on the commit-failure ERROR line plus a DEBUG line beside it, both inside the exception guard of the async callback, and the symptom is zero records consumed after toggling an instance. What the second sighting adds is the condition rather than the count. Both fired on the first run of a branch that had just taken a change to how this lane runs - pr-integration-gate-wall-time.md already records a first-ever timeout failure in this same class when forkCount went 4 to 6, and this run was the branch's first under the newly sharded integration and chaos lanes. That makes it a load-shape canary rather than a random flake, which is a sharper claim than one sighting could support and points at the fix the existing entry already names: every wait in the test is a fixed sleep, so the sleeps are what lose when the runner gets busier. The same head was re-run once and passed, which separates always-red from not-always-red and establishes nothing about the rate - the method that solutions doc owns, cited rather than restated. 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
…eclares the whole checked family, and the rider tests speak Truth Codex's first pass on #460, Claude's summary, and the two CI failures that were this branch's own. THE ONE THAT MATTERED. encodeOffsetsToInnerBytes re-read hasIncompleteOffsets() after tryToEncodeOffsets had already decided the caught-up case on the read it commits against. The control thread can complete the last incomplete record between those two reads, and the re-check then answered "nothing to encode" for a commit already pinned to the older offset - no map, and a crash before the next commit replayed records this build had recorded as complete. That is the confluentinc#893/894 tear class the javadoc two lines away warns about, reintroduced one call deeper. The method now encodes whatever the encoder's own single sample holds, as the pre-rider path always did; a map that emptied in between encodes as a complete map, which resumes correctly. The test that pinned the old answer now pins the new one and says why. decodeRider declared OffsetDecodingError and CorruptOffsetMetadataException but not the rest of the checked FAIL family - KafkaStreamsEncodingNotSupported and UnsupportedOffsetEncodingException arrive sneaky-thrown just as the unknown-magic case does. It now declares their parent, EncodingNotSupportedException, so a caller can catch the family in one clause or any member by its own type, and the javadoc names all three. The overhead test read "not engaged at the top of the domain" as "never engages", which would have skipped every boundary assertion for exactly the non-monotone compressed shape it exists to measure. It now walks a 64-point grid before believing the endpoint; the residual is a transition narrower than one grid step, named in the javadoc, rather than an unobserved interior. House rules: the three new rider suites that had followed their AssertJ neighbours now use Google Truth; discarded Set/Map returns in the rider tests are named with their reason; the release decision moved out of the merged #207 note into release-offset-rider-write-side-split.md; the encoding tie-break sighting outgrew a refactoring.md line and is now core-offset-encoding-tie-break-is-not-deterministic.md; two stacked javadocs before warnOversizedRider are one; the setup the two codec tests duplicated (CPD's one new clone) is RiderTestFixtures.moduleWithNoSupplier and stateOver. CI: CloseAndOpenOffsetTest forced every OffsetEncoding as a codec, including the envelope constant that names none; it is skipped like the other non-encoders, and the reset of the forced statics moved to @AfterEach, because the failing arm had leaked forcedCodec into the next test in the class. Infer's BUFFER_OVERRUN_L1 on OffsetRiderEnvelope.wrap and its two callers is a guarded inner[0] read that the interval domain cannot see past; the three identities are ratcheted with that reason. Left open on the PR, deliberately: whether the rider should ride alone on the two strip paths (the no-encoding fallback Codex flagged, and the ladder's own strip rung), which changes the ladder's documented last rung and the dropped-rider accounting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkRtYPnDuAPLEmHFYSnAiJ
astubbs
added a commit
that referenced
this pull request
Sep 7, 2026
…ely ignores Thirty-nine items are in flight - 32 open PRs plus seven with no PR yet - and the ordering had been re-derived from scratch three times in one session, differently each time. This writes it down once. Ordered by value to the release, explicitly not by merge mechanics: conflicts, red checks and stale bases are not inputs to it. Records the two hard constraints (#199 cannot be applied after the tag, #207 decides what already deployed v6 readers will tolerate forever), the one item with schedule risk (the Connect PoC is not written yet), and the three open decisions the order cannot settle on its own. Per the directory's rules it names no PR titles or states - `gh pr list` owns those - and gives the command to re-check coverage instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Master gained two commits since this branch last took it: #466's revoke-path drain fix (745b1f6), which moves the transactional revocation commit onto the control thread and touches PartitionState, PartitionStateManager and WorkManager, and #476's in-flight vetting sweep (6aab3ff), which stamped an `inflight-vetted` marker on every open note. Neither collides with the shard or the container: the drain fix merged clean against this branch's ProcessingShard and WorkContainer work. ONE CONFLICT, resolved by keeping the deletion. `docs/inflight/bug-stale-sweep-iterator-evicts-fresh-replacement.md` is modify/delete: the sweep shrank and stamped it on master while this branch resolves and deletes it, its content having been migrated to `docs/solutions/logic-errors/a-by-key-removal-cannot-say-which-container-it-meant-2026-09-07.md`. Master's stamp is not an argument for keeping the note - it is the reason to delete it. What the sweep recorded on 2026-09-08 was that the accounting half had closed and that "what is left open is the eviction of the fresh record itself"; that residue is precisely what this branch closes, by giving WorkContainer identity equality so `workMap.remove(offset, container)` is a true compare-and-remove. A vet stamp describes the note's remaining subject at the moment it was read. This PR removes the subject, so the stamp is falsified rather than inherited, and the note goes with it. The two paragraphs the sweep rewrote - the mechanism sentence and the #336 accounting note - are both already carried, correctly and in more detail, by the solutions write-up. Neither `--ours` nor `--theirs` was used anywhere in this merge; the one conflict was resolved with `git rm`, and the two auto-merges were left as git computed them and then read. ONE REPAIR MASTER'S SIDE NEEDED, because deleting a file does not change a line in what cites it. Master's `docs/inflight-tool.md` documents the new `vet` command with a worked example of its output, and the row it uses is this note. With the note gone, `bin/check-file-refs.sh` fails on `docs/inflight-tool.md:332` - the `bin/inflight.mjs docs show <path>` line of that example. The example is replaced with a real row for a note that survives and carries the same signal: `bug-run-length-plausibility-ceiling.md`, impact `data-loss` so the group heading is unchanged, first added 2026-09-03, one cited fork number and it is settled (#207 MERGED). The paragraph beneath it makes a point about the innocent reading of that signal, and #207 fits it better than the number it replaces: that pull request added the adjacent run-length validation rather than the ceiling the note asks for, so the row genuinely cannot tell whether the citation is a premise or a fix. Silencing the gate with a paragraph `file-refs: N/A` marker was rejected - the example would still have named a note that no longer exists, and a worked example that cannot be run is worse than a dangling path, because nothing else would ever catch it. AUDITED IN BOTH DIRECTIONS, per file. `git diff 6d2da40..HEAD` over the conflicted and auto-merged files shows only master's additions plus the inflight-tool.md repair: the whole `vet` section arriving in inflight-tool.md, the vetted marker arriving on `docs/inflight/test-857-churn-storm-async-stalls.md`, and master's removal of the `state/ProcessingShard.java` entry from `docs/refactoring.md` - which is byte-identical to 6aab3ff's own diff of that file. `git diff origin/master..HEAD` over the same files shows only this branch's changes: the note's deletion, the 2026-09-07 `ZOMBIE_MEMBER` sighting and its replay seed appended to the churn-storm ledger, the breaking-changes section rewrite in `docs/refactoring.md`, and the two-line example substitution. The file sets were compared as sets as well as read as diffs: the merge result differs from origin/master in exactly the files this branch changed against its merge base plus `docs/inflight-tool.md`, and differs from 6d2da40 in exactly the files master changed against that same base, minus the note it stamped and this branch deletes. Nothing was silently dropped in either direction. Verification: `bin/check-all.sh` 16 gates passed, 0 failed (5 skipped as PR-scoped, 3 nothing-in-scope); `bin/check-file-refs.sh` and `bin/check-issue-refs.sh` clean standalone; the state package on JDK 17 (`./mvnw -Pci -pl parallel-consumer-core -am test -Dtest='bz.stub.parallelconsumer.state.*Test'`) is 30 classes, 171 tests, 0 failures, 0 errors, including ShardStaleSweepReplacementEvictionTest and both halves of the sorted-collection ArchUnit rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…first batch Owner's decision on #480: keep the bound, do not buy it with a broker request. This replaces the ListOffsets-at-assignment version of the same check, commit by commit, and covers strictly more. WHAT THE CHECK IS. A structurally valid run-length entry of Integer.MAX_VALUE moves the highest-seen offset about two billion forward, and PartitionState#isRecordPreviouslyCompleted then reads every real record in that range as already succeeded - PC skips them without ever calling the user function. Silent non-processing, not replay, from metadata anything sharing the consumer group can write. #207 rejects everything the payload itself proves wrong and stops exactly here, because a long run of completed offsets is what run-length encoding is FOR. WHY NOT AT DECODE. Decoding happens inside the rebalance callback, where the only way to learn where a partition ends is a blocking ListOffsets request. Two objections, and both are recorded rather than reasoned about again later: it puts a broker round trip on the most fragile path in this codebase, and it fails open precisely when the broker is unhealthy - which is when rebalances storm and offset maps get rewritten. The previous commits on this branch did exactly that; this one deletes it. WHERE IT LANDS INSTEAD. The fetch that delivers the first batch already carries the partition's high watermark. ConsumerManager#logEndOffsetIfKnownWithoutBlocking reconstructs it as position + Consumer#currentLag (KIP-695, kafka-clients 3.0+), both read in one breath on the thread that owns the consumer, with a zero-timeout position read so nothing can block or send a request; EpochAndRecordsMap carries the answer with the records. PartitionState checks the loaded map against it in maybeRegisterNewPollBatchAsWork - after the epoch fence, BEFORE the truncation and the per-record loop, which are the only two things that consult the map. So the map is settled before anything can be skipped against it, and waiting costs nothing: nothing consults the map until a record arrives to be consulted about. ABSENCE IS NOT EVIDENCE. currentLag is empty until a fetch has happened and the position read raises rather than blocking, so "not established" is a real answer - the check then defers to the next batch. In the meantime the batch's own records are a floor, because a record that arrived certainly exists; a floor can only confirm a claim, never refute one, which is why it is safe to act on with no watermark at all. SAME POLICY, MORE COVERAGE. A refused map goes through EncodedOffsetPair#handleUnreadableMetadata like every other payload this build cannot read: under the default IGNORE the map is discarded, processing resumes from the committed offset and the existing warn names the partition, the base offset and this reason; under FAIL it throws. Nothing counts the discard - that is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md, cited from the code. Because the check now reads the DECODED claim rather than the bytes, one line covers run-length and bitset alike, and any encoding added later. THE COMMITTED OFFSET IS PASSED, NOT DERIVED. PartitionState needs the offset its map was loaded against, both to fall back to and to know whether there is a claim at all. Deriving it from getOffsetToCommit() is wrong for a map with no incomplete offsets - it answers highestSucceeded + 1 - and that silently disarmed the check for every fully-completed map, which the bitset tests caught. Proven by sabotage: with the call to maybeVerifyLoadedOffsetMapAgainstThePartition removed and nothing else changed, five of the eight cases in ImplausibleOffsetMapOnFirstBatchTest fail, including the one that reads a record the partition really holds as completed - while the three control arms (a map inside the partition, a bitset at the boundary, a claim the records themselves corroborate) stay green, which is what tells you the controls do not depend on the guard. EncoderOutputSurvivesDecodeValidationTest keeps the encoder side: every shape it produces is measured against the rule with the tightest honest bound there is, a partition ending exactly on the last offset encoded. MockConsumer, verified rather than assumed (kafka-clients 3.9.1): currentLag answers endOffsets - position when updateEndOffsets has been called, and OptionalLong.of(0) when it has not - not empty, as a real consumer answers before its first fetch. An unprimed mock therefore understates where a partition ends; nothing in the suite is affected, because the check only engages for a partition whose committed metadata claims offsets above the committed one, and ConsumerManagerLogEndOffsetTest pins all four behaviours so the next test to reach for it does not rediscover them. Left open, and recorded rather than carried silently: the decode-side ceiling this branch previously had also stopped a huge INCOMPLETE run from being walked into a TreeSet, and the lazy check cannot - it runs after the decode. That is master's status quo rather than a regression, and it now has a note: docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md. Co-authored-by: Claude Opus <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…old is refused at the first batch (#480) A structurally valid run-length entry of Integer.MAX_VALUE moves the highest-seen offset about two billion forward. PartitionState#isRecordPreviouslyCompleted then reads every real record in that range as already succeeded, so PC skips them without ever calling the user function - silent non-processing, not replay, from metadata anything sharing the consumer group can write. #207 rejects everything a payload itself proves wrong and stops exactly here, because a long run of completed offsets is what run-length encoding is FOR. THE BOUND IS THE PARTITION. The only bound a legitimate map provably cannot cross is the partition's log end offset: PC encodes only offsets it has polled, an offset the partition does not hold cannot have been polled, and a Kafka log end offset only grows. Two alternatives were refuted rather than disliked. A ceiling derived from configuration - concurrency, the in-flight target - fails because back-pressure keys off the encoded PAYLOAD size, so a map with one stuck offset stays three entries wide however far the partition runs ahead of it: one poison record legitimately produces an enormous range, and any such ceiling eventually discards a true map and replays everything in it. A round number is the same objection, failing silently the day it is wrong. IT IS READ OFF THE FETCH, NOT OFF THE WIRE. An earlier revision of this work bought the end offset with a ListOffsets request at assignment. That was rejected for two reasons worth keeping: it put a blocking broker round trip inside the rebalance callback, the most fragile path in this codebase, and it failed open exactly when the broker was unhealthy - which is when rebalances storm and offset maps get rewritten. The fetch that delivers the first batch already carries the high watermark, so ConsumerManager#logEndOffsetIfKnownWithoutBlocking reconstructs it as position + Consumer#currentLag (KIP-695), both read in one breath on the thread that owns the consumer, with a zero-timeout position read so nothing can block or send a request; EpochAndRecordsMap carries the answer with the records. THE CHOKE POINT is PartitionState#maybeRegisterNewPollBatchAsWork, after the epoch fence and before both maybeTruncateOrPruneTrackedOffsets and the per-record loop - the only two things that consult the loaded map. So the claim is settled before anything can be skipped against it, and waiting costs nothing: nothing consults the map until a record arrives to be consulted about. ABSENCE IS NOT EVIDENCE. currentLag is empty until a fetch has happened, so "not established" is a real answer and the check looks again at the next batch. In the meantime the batch's own records are a floor, because a record that arrived certainly exists - and a floor can only confirm a claim, never refute one, which is why it is safe to act on with no watermark at all. SAME POLICY, WIDER COVERAGE. A refused map goes through EncodedOffsetPair#handleUnreadableMetadata like every other payload this build cannot read: the default IGNORE discards it, resumes from the committed offset and warns with the partition, the base offset and the reason; FAIL throws. Because the check reads the DECODED claim rather than the bytes, one line covers run-length and bitset alike, and any encoding added later - where a decode-side version had to be threaded through each decoder. Nothing counts the discard; that is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md. EVIDENCE. Proven by sabotage: with the call to maybeVerifyLoadedOffsetMapAgainstThePartition removed and nothing else changed, five of the eight cases in ImplausibleOffsetMapOnFirstBatchTest fail - including the one that reads a record the partition really holds as completed - while the three control arms stay green. EncoderOutputSurvivesDecodeValidationTest keeps the encoder side: every shape it produces is measured against the rule with the tightest honest bound there is, a partition ending exactly on the last offset it encoded, so anything that made the encoder write one offset past what it saw lands as a false rejection rather than as silence. ConsumerManagerLogEndOffsetTest pins the non-blocking read, including that MockConsumer answers lag 0 rather than empty when it has no end offsets - a mock that understates where a partition ends, which the next test to reach for this would otherwise rediscover. Retires docs/inflight/bug-run-length-plausibility-ceiling.md; the reasoning about the bound outlives it in docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md. Left open and recorded rather than carried silently: a huge INCOMPLETE run is still walked into a TreeSet during the decode, which this check runs after - master's status quo, now docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md. 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.
Serves #197 — item 5 of its triage list. No closing keyword: that is the release tracking issue, not this defect.
Description
ParallelConsumerOptions#invalidOffsetMetadataPolicyexists so a consumer that finds commit metadatait cannot read can be told to drop it and carry on instead of failing. It was unreachable for
most of the cases it exists for.
The offset metadata field is free-form. A previous owner of the consumer group — a Kafka Streams app,
another framework, operator tooling — can leave bytes there. So can a newer PC, writing an encoding
that did not exist when the reading build was compiled: the fork owns this wire format now, and only
a version that already has this fix can degrade gracefully when the next encoding lands.
There are three ways "this build cannot read this payload" happens. The policy governed one.
RuntimeException("Unexpected magic")fromOffsetEncoding.decode, upstream of anywhere the policy was knownByteArraypair)UnsupportedOperationException, policy not consultedAll three now route through one
EncodedOffsetPair#handleUnreadableMetadata, with typed exceptions(
UnknownOffsetMetadataMagicException,UnsupportedOffsetEncodingException,CorruptOffsetMetadataException) alongside the existingKafkaStreamsEncodingNotSupported.The third case produced answers, not errors
The decoders trusted their own header — a length field is read out of the payload and then used to
drive a loop. Probed on this branch:
BitSetdeclaring 32767 bits, empty bodyBitSetV2declaringInteger.MAX_VALUEbitsOutOfMemoryError, from metadata any previous owner of the group can writeBitSet/BitSetV2negative lengthRunLengthV2negative runcurrentOffsetbackwardsRunLengthbody one byte too longasShortBuffer()drops the trailing partial entry, the rest decodes as if fineBufferUnderflowExceptionescapedonPartitionsAssignedZstdIOExceptionescaped the same wayA wrong offset map is worse than a failed one — nothing downstream can tell it from a real one, so
the outcomes are mass replay, or mass silent skipping via
PartitionState#isRecordPreviouslyCompleted.The checks added are only ones the buffer can settle by itself: a declared bit length must be
non-negative and backed by bytes actually present (
ceil(bits/8)— this is also what fixes the OOM,since 2^31 bits would need 256MB of body); a run length is a count, so never negative; a run-length
body must be a whole number of entries.
Relationship to #217 — same defect, fixed twice
#217 (
ed78d6cc9,Fixes astubbs/parallel-consumer#118, mirroringconfluentinc#326) fixed the first case independently and merged first. It
deleted this PR's item from the release ledger. Neither PR referenced the other.
The two disagree about what the option means, so merging master here was a design decision:
decode()throws checkedOffsetDecodingError, caught byloadPartitionStateForAssignment, map dropped. The policy governs only bytes positively identifiedas Kafka Streams'; its javadoc was narrowed to say undecodable metadata "is never fatal under
either policy".
This PR's routing is kept. Item 5 recorded the defect as an older PC dying "regardless of how the
policy is configured" — the complaint is that the policy does not govern. #217 made the ungoverned
path benign and documented the resulting inconsistency instead of removing it: under its default
FAIL, Kafka Streams metadata shut the consumer down while every other unreadable byte silentlyrecovered. One option, two opposite behaviours, picked by which flavour of unreadable you hit.
Two of #217's changes are not part of that competing fix and were re-applied on top when resolving:
InternalRuntimeException→PCInternalRuntimeException(#267 — the oldclass is deleted, so references would not compile), and the #121
sample-the-high-water-mark-once fix in
encodeOffsetsCompressed(dropping it silently loses records).FAILtoIGNOREForced by the above, not incidental. Once the policy genuinely governs every unreadable path, the
default decides what happens to someone who configures nothing — and that is the
#118 reporter, who pointed PC at a group Kafka Streams had previously owned.
Leaving it at
FAILwould make that exact report fatal again.FAILstays, opt-in, and now means what it says: stop rather than silently discard an offset map,because discarding it replays records that completed but were not committed.
#217's
ForeignOffsetMetadataOnAssignmentTestasserted the old contract directly — that an unknownmagic byte does not escape
onPartitionsAssignedunder an explicitFAIL. That is the assertionthis PR overturns, so the test was rewritten rather than deleted: the #118 regression is now pinned
under the default policy (the reporter's actual configuration), and a second test pins
FAILasgenuinely fatal.
Also fixed here, still live on master
IGNOREresumed fromHighestOffsetAndIncompletes.of(baseOffset), butbaseOffsetis thecommitted offset — the next one to be polled. Claiming to have seen and succeeded it makes
PartitionState#isRecordPreviouslyCompletedskip that record and pushes the next commit tobaseOffset + 1. All unreadable-metadata paths now agree onbaseOffset - 1, matching theno-metadata-at-all branch of
decodeCompressedOffsetstwo lines away, which always got it right.Left open, recorded not forgotten
RunLengthV2ofInteger.MAX_VALUEmoves the highest-seen offset ~2e9 forward. Nothing in the payload proves itwrong — a long run of completed offsets is what RLE is for, and the decoder has no partition end
offset to check against. Bounding it means picking a plausibility ceiling, a product decision.
Queued in
docs/refactoring.md.FAILstops the consumer by letting a checked exception escape Kafka's rebalance callback,surfacing as a generic
KafkaException: User rebalance callback throws an error. Opt-in now ratherthan the default, but a deliberate stop should go through PC's own fatal-error path. Queued in
docs/refactoring.md.ByteArray/ByteArrayCompressedremain encoder-less and decoder-less enum constants. They nowfail cleanly through the policy; whether to implement or delete them is a wire-format decision.
Folded in from a parallel re-cut
A second session independently re-cut this PR onto master as
recut/207-offset-policy-bypass. It is stood down — pushed only so its handoff pointed at realcommits, no PR, never force-pushed over this head — and its design disposition is not adopted. Two
things from it are folded in here:
magicByteOfAnEncodingThatDoesNotExistYet()helper. It was applied inWorkManagerOffsetMapCodecManagerTestbut not inForeignOffsetMetadataOnAssignmentTest, whichstill wrote
(byte) 42— in the test whose entire subject is the unknown-magic path. Now derivedfrom
OffsetEncoding.values()there too, so adding an encoding cannot silently retarget that test.found its owner.
JStreamParallelEoSStreamProcessorTest's two produce tests failed together withan empty result stream. That is fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue #116's defect, not test
infrastructure: its own description says eight tests "passed only because the stream quit early —
they encoded the defect", and these are two of them. Recorded in
docs/inflight/test-untracked-ci-flakes.mdwith the mechanism and instructions to delete the entrywhen fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue #116 lands; the sighting is also posted there, since it is evidence
for a claim that PR currently only asserts.
Review follow-ups
Raised on review and fixed here; none of them was caught by a gate.
OffsetMapCodecManagerdropped master's parameter and
@throwsdocumentation from four methods — bothdeserialiseIncompleteOffsetMapFromBase64overloads and bothdecodeCompressedOffsetsoverloads.Restored as coverage, not text: master's wording asserted the fix(core) #118: foreign offset metadata no longer kills the consumer on assignment #217
contract ("Metadata that cannot be decoded at all raises
OffsetDecodingErrorunder eitherpolicy"), which is what this PR overturns, so restoring it verbatim would have re-documented
behaviour the code no longer has. This is the whole-file-resolution hazard
AGENTS.mdwarns about —prose vanishes and nothing goes red.
getDecodedIncompleteshad no javadoc on any of its three overloads. The three-arg form nowstates the point of the method: the three failure routes are one user-visible event, and names the
fourth outcome it deliberately cannot cover.
MOM_MISLEADING_OVERLOAD_MODEL. A private instance method shared its name with twostatics; the shape predates this branch, but this branch gave it a new signature so it landed as
a finding on a line this diff wrote. Renamed to
decodeOffsetMapForPartition— the honester name,since the instance form is the only one of the three that consults the user's configured policy,
and sharing a name made that argument read at the call site as merely optional.
default: break;now states its assumption inline, and names what enforces it:decodeBody's own default throws, so an encoding added with neither a decoder nor a policy armfails loudly rather than being decoded as something else.
InternalRuntimeExceptionfor why", and that class was renamed and deleted infix(core)!: user code must not be able to silently stop the consumer #267.
check-file-refs.shvalidates cited paths, not class names incomments.
A second flake, recorded not fixed
Running the full core suite while the box was also running CI surfaced
ParallelEoSStreamProcessorTest.processInKeyOrderfailing — not theprocessInKeyOrderflakedocs/solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.mdalready covers, whosefix merged with #264. That one is a 30s
ConditionTimeoutException; thisfails in ~0.6s at the test's own
[sanity check input data]precondition, withpolledempty,because
awaitForOneLoopCycle()is a fixed cycle count standing in for "the consumer has polled".Control-armed before filing it as contention: green 559/0 on a fresh worktree with the box idle at
both this branch's tip and the commit before it; 2 failures twice with the box loaded; green
68/68 for that class alone under the same load; and the failing parameters differ between runs. This
PR touches offset decoding and nothing that test exercises. Recorded in the flake ledger,
diagnosed-not-demonstrated, deferred to the ledger's classify-before-touching rule.
Other instances of the same defect class — swept, and what was ruled out
The class, named rather than described by its symptom: a payload-derived read on the partition-assignment
decode path that can fail, or answer wrongly, without reaching
invalidOffsetMetadataPolicy. Two shapes —a throw that bypasses
handleUnreadableMetadata, and a decoder trusting a header field the payload does notback up.
Hits on the production path: none left. Every
throw newinparallel-consumer-core/.../offsets/wasread, not just the ones near the fix.
The one residual worth naming, because it is deliberate and could still be read as an oversight:
decodeBody'sdefault -> throw new PCInternalRuntimeException(...)is not in the catch list that routesfailures to the policy (
CorruptOffsetMetadataException | BufferUnderflowException | IOException). NoOffsetEncodingconstant can reach it today — the codec-lessByteArraypair returns through the policybefore
decodeBodyis called — but a future constant added without a decoder would escape. It is keptthrowing on purpose: it is the enforcer for the
default: break;indecodeToIncompletes, and the decisionabout those constants is owned by
docs/inflight/core-bytearray-encodings-have-no-codec.md.Dismissed, with the reason each is not an instance:
OffsetRunLength'sIllegalArgumentExceptionrunLengthEncode). Its input is PC's own bitmap string, never a committed payloadOffsetBitSet'sPCInternalRuntimeException("Invalid state")OffsetEncodingthe caller passed, not over payload bytesOffsetEncoding.decode(byte)'sOffsetDecodingErrorEncodedOffsetPair.unwrap, which has zero callers insrc/main— tests only, and its javadoc says production goes throughdecodeToIncompletesEncodedOffsetPair.getDecodedString()'s throwingdefaultBitSetEncoder,RunLengthEncoder,OffsetSimultaneousEncoderthrowsOffsetMapCodecManager's "continuously woken up" throwFor the second shape, the sweep was for payload-supplied lengths and widths:
OffsetBitSet's two lengthreads and
OffsetRunLength's two buffer widths are the complete set, and all four are now validated.OffsetSimpleSerialisation.deserialiseByteArrayToBitMapStringreads no header — its loop is bounded byhasRemaining().Carried by the merges, not by the offsets work
Four things landed here that the sections above do not describe. Each is small, none is offsets, and
all four are in the diff — so they are listed rather than left for a reviewer to wonder about.
bin/check-source-patterns.mjsrefused this PR's own merge commit, reporting seven oftest(chaos) confluentinc#857: test-log silos, a scenario-wide recovery diagnostic, and unattended experiment runners #381's experiment runners as new shell scripts and advising
--no-verify.Its
added-filesscope is committed-plus-staged additions against the merge base, and while a mergeis staged but uncommitted HEAD is still the pre-merge tip — so everything master added since the
branch was cut counts as an addition. It self-corrects once the merge commit exists, which is why
master has never seen it: it fires only in the window where the fix has to be applied. Now subtracts
membership of
origin/master, inbin/lib/added-files.mjsas a pure function of three lists so itgets control-pair tests like every rule in the table; the four must-NOT-match cases are red-proven
against the pre-fix implementation.
The IGNORE advice sentence is held once, on
EncodingNotSupportedException, instead ofcopy-pasted into three exception messages — character-identical in two of them. The duplicate-code
check flagged the classes as 34–50% similar; that part is Java exception boilerplate and the classes
stay separate.
CorruptOffsetMetadataExceptionreads the constant without joining the hierarchy: acorrupt body is not an unsupported encoding, and re-parenting it to tidy a metric would change
which
catchblocks see it.Two coverage gaps closed, both this PR's own. The empty-input guard added to
decodeToIncompleteshad no test — a guard whose own comment argues that an unenforced invariantgets broken, left unenforced one level up; it is now the thirteenth row of
corruptPayloads(), so itruns under both policies. And
UnknownOffsetMetadataMagicException(byte)had no callers, which iswhy it was uncovered; the class is new here and never released, so removing it breaks nothing.
One note that outlives this PR, and one that was written and then deleted.
docs/inflight/issue-response-118.mdis a draft reply to confluentinc#326: "Unexpected magic" - offset metadata PC did not write crashes the consumer on assignment #118 coveringthe corrupt-body half its existing comments predate; per
docs/inflight/AGENTS.mdit is posted onlyon explicit instruction and deleted when posted, not when this merges.
The deleted one is worth naming because the diff shows the churn. A codecov note was added here and
then removed once
node bin/inflight.mjs prior-art codecovshowed the repo already had two onmaster —
ci-codecov-flags-not-like-for-like.mdandci-the-coverage-uploads-still-use-the-inert-glob.md— plus an open PR fixing the cause, ci(codecov): expand the coverage globs the uploader never expanded #414, since merged. Its diagnosis was
also wrong: the per-flag gates go red because
codecov-actionnever expands**, so master's buildhands the whole tree to both flags, not because a base was partial. What survived was contributed to
ci(codecov): expand the coverage globs the uploader never expanded #414's note instead, where it now sits as the cancelled-master-run case.
Flake sightings recorded, none of them this PR's.
ManagedPCInstanceLifecycleTest(arrived withfix(core) confluentinc#857: the revoke path declines the commit lock instead of blocking on it #29, control-armed to not always red), the PIT lane's 30-minute cap, and
MdcContextPropagationTest— the last now at five sightings including master's ownfull build,which is why ci(codecov): expand the coverage globs the uploader never expanded #414's merged fix has not yet taken effect.
Sequencing
No parents — master already carries #217. Master is merged in up to and including #29's confluentinc#857 revoke-path fix; run
git rev-list --left-right --count origin/master...HEADfor the live divergence rather than trusting this line.#306 and #106 are performance work in the same
decode path and will conflict textually. Landing this first is the cheaper order: it is the smaller,
correctness-only change, and the semantic point they must preserve is that every "cannot read this
payload" path goes through
handleUnreadableMetadatarather than throwing directly.Checklist
docs/inflight/pr-207-offset-encoding-policy.md, plus the notes listed underCarried by the merges, not by the offsets work (the fix(core) #118: foreign offset metadata no longer kills the consumer on assignment #217 collision, the default
decision, the corrupt-body findings) and
docs/refactoring.md(the two deferred items above).CHANGELOG.adocdeliberately untouched — perAGENTS.mdit is generated at release time.docs/features/invalid-offset-metadata-policy.yamlcorrected. This box was originally answered"N/A - no user-facing feature added", which was wrong: that file is published, user-facing
documentation of this exact option, and four of its claims became untrue here — most bluntly
"FAIL is the default and stops the application", and a
boundariesentry stating thatunrecognizable metadata "is not made fatal by this option", which is precisely the
fix(core) #118: foreign offset metadata no longer kills the consumer on assignment #217 contract this PR overturns.
check-docs-data.shand thedocs data: auditlane both stayed green over it, because they validate structure, not truth.CorruptOffsetMetadataTest(12 corrupt shapes × both policies plus twobase64 cases, 26 tests; each shape pinned individually because each produced a different
wrong answer),
EncoderOutputSurvivesDecodeValidationTest(25 round trips proving the newvalidation rejects nothing this build's own encoders emit — the expensive direction to get
wrong, since a false rejection discards a real offset map and replays completed work),
ForeignOffsetMetadataOnAssignmentTestrewritten to the new contract, plus the existingWorkManagerOffsetMapCodecManagerTestadditions. 614 core unit tests pass, 670 across thefour library modules.
@claude review thison the PR instead, which is the route that can openinline review threads.