Repository navigation
fix(offsets): an offset map claiming offsets the partition does not hold is refused at the first batch - #480
Merged
Merged
Conversation
… payload, not a completed range A structurally valid run-length entry of Integer.MAX_VALUE moves the highest-seen offset about two billion forward. PartitionState's 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 rather than replay, reachable through any metadata written against the consumer group. The same entry as an INCOMPLETE run was worse in a different direction: the decoder walked it one offset at a time into a TreeSet, so five bytes asked for two billion boxed longs. #207 hardened this decoder against everything the bytes can settle - a negative run, a body that is not a whole number of entries, a declared bit length with no bytes behind it - and could not settle this one, because a long run of completed offsets is exactly what run-length encoding is for. THE BOUND, AND THE TWO REJECTED ALTERNATIVES The check now compares the decoded range against the partition's log end offset, read once per assignment and passed into the decoders. It is the only bound that cannot reject a real offset map: PC encodes only offsets it has polled, an offset the partition does not hold cannot have been polled, and a Kafka end offset only grows, so a bound read at assignment still holds for a map written earlier. Rejected - a ceiling derived from configuration (concurrency, the in-flight target). Refuted by the product: back-pressure keys off the encoded PAYLOAD size, and a run-length map with one stuck offset stays three entries wide however far the partition runs ahead of it. One poison record therefore produces a legitimately enormous range, and any such ceiling eventually discards a true map - which replays every record it covered. Rejected - a round number ("nobody legitimately runs a billion ahead"). The same objection, failing silently the day it is wrong. Both are what left this defect open: the ceiling was framed as a number somebody had to pick, when the authority was one call away on an object the code already held. WHAT A LEGITIMATE LONG RUN LOOKS LIKE, so the guard cannot reject one: one record stuck at the committed offset while every offset above it succeeds. That is ordinary here and self-limiting only in payload size, never in range - which is why the javadoc on runLengthDecodeToIncompletes states it, and why every encoder round-trip case in EncoderOutputSurvivesDecodeValidationTest is now decoded twice: unbounded, and against a partition ending exactly on the last offset encoded. Anything that made the encoder write one offset past what it saw now lands as a false rejection instead of as silence. The encoder needs no change: RunLengthEncoder's range top is OffsetSimultaneousEncoder's highestSucceededOffset, an offset PC actually saw. ROUTED THROUGH THE EXISTING POLICY, not a parallel one. An implausible payload is an unreadable payload: CorruptOffsetMetadataException, caught by EncodedOffsetPair#getDecodedIncompletes and handed to invalidOffsetMetadataPolicy. Under the default IGNORE the map is discarded and processing resumes from the committed offset, with the existing warn naming the partition, the base offset and this reason; under FAIL it stops. Nothing counts the discard - that is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md, cited from the code and deliberately out of scope. GROUND TRUTH, AND WHAT HAPPENS WITHOUT IT. OffsetMapCodecManager asks the consumer for the end offsets of the partitions that actually carry metadata, once, batched, on a rebalance that has already paid for a JoinGroup, a SyncGroup and an OffsetFetch, and skipped entirely when no assigned partition has a map to check. Every failure - an unavailable leader, a timeout, a consumer that will not answer - is caught and logged, and the decoders are handed "unknown", which refuses nothing. Failing closed would discard honest maps on a broker hiccup, an event far more likely than the corrupt payload the guard is for. THE SAME SHAPE IN THE BITSET DECODER is fixed too, because the parameter was already in hand: a declared bit count the format cannot prove absurd. It is less exposed - #207 made the length prove itself against bytes that are present, so a full 4KB metadata field buys tens of thousands of skipped records rather than two billion - but bounded is not true. Proven red before the fix: with the main code reverted, aRunPastTheEndOfThePartitionDoesNotMarkRealRecordsAsCompleted read a record the partition really holds as completed, and failPolicyRejectsARunPastTheEndOfThePartition raised nothing, while both control arms (a long run inside the partition, and an unknown end offset) passed unchanged. The bitset half was proven by sabotage - disabling its check alone turns aBitSetReachingPastTheEndOfThePartitionIsRejected red. The round-trip test earned its place immediately: it caught an overflow in the first version of the guard, where Long.MAX_VALUE as the unknown ceiling wrapped the arithmetic and rejected every payload including the encoder's own. docs/inflight/bug-run-length-plausibility-ceiling.md is retired; 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, and pr-207's left-open list drops the entry. Co-authored-by: Claude Opus <noreply@anthropic.com>
…s the partition cannot hold The feature record's boundaries said the policy "does not govern a payload that decodes successfully into a wrong-but-plausible offset map; nothing in such a payload proves it wrong". The plausibility ceiling falsifies half of that: the partition proves it wrong, for any map reaching past the last offset the partition holds. Recorded here as well as in the code because check-docs-data.sh validates structure and not truth - the same way this file was falsified once before, by the PR that wrote it. Co-authored-by: Claude Opus <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
|
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #480 +/- ##
============================================
+ Coverage 82.50% 83.04% +0.54%
- Complexity 1563 1583 +20
============================================
Files 96 96
Lines 5383 5421 +38
Branches 537 544 +7
============================================
+ Hits 4441 4502 +61
+ Misses 747 724 -23
Partials 195 195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🟢 Throughput — OKThis branch measured the same speed as master, on the one test this measures.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: b654cb2, a055248, 65e11e3, e05b399, ee3d8a9, 890aa53, fd3f913, 21e8728, 9302a89, c2d35c0 Since the previous push: ratio 1.011 -> 0.995, share 1.822 -> 1.85, rate 66922 -> 62665 (-6.4%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
…new lines bin/check-pr-analysis-surfaces.sh reports three SpotBugs findings on lines this PR wrote, and the point of reading that output before review is to act on it: - the set of partitions to look up was called partitionsCarryingAMap, where "map" meant the offset map and SpotBugs read it as the collection type. It is a Set, so the name now says metadata instead. - two identity lambdas (builder -> builder) become UnaryOperator.identity(). No behaviour change. The seven findings inherited on lines this PR did not write are left alone, per the script's own split. Co-authored-by: Claude Opus <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… tier 1 The one item the burn-down had filed under "owner's decision" is decided by the owner asking for it to be built: #480 bounds every decoded run and bitset by the partition's log end offset, the one bound that cannot reject a real map because PC only encodes offsets it polled, and routes the rejection through invalidOffsetMetadataPolicy with no parallel policy, failing open with a warn if the broker will not answer. The same defect class was found and fixed in the bitset decoder; the simple serialisation has no declared count and is clean. Proven red through the real assignment path, mutation lane green on the bound. The burn-down moves it into tier 1 and stops linking the inflight note, which #480 retires into docs/solutions/ - the link would dangle the moment that PR lands. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… offsets - note it under confluentinc#233 The registry already says OffsetMapCodecManager conflates encode and decode and needs a Consumer only for one decode-on-assignment method. #480 adds a second use of that consumer in the same method - a blocking ListOffsets round trip inside the rebalance callback, batched and failing open - to bound decoded runs and bitsets by the log end offset. The entry records that the ground-truth lookup moves out with the decode side when the split happens. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
Master landed the async-commit acknowledgement (#470, silent loss on the default commit mode) and the quarantine clearance (#473), which empties docs/quarantined-tests.md so the release guard no longer blocks on it and moves the capacity profiles behind a measured tag. With #466 and #468 already in, tier 1 has four of its seven merged; what is left is #469, #481, #480 and #477. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…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
changed the title
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
… left #469 landed the two PartitionState cross-thread flags, measured then fenced or redesigned, so tier 1 is down to #480 (being reworked off the wire) and #477 (with a babysitter). #485, the control-arm proof of the deadlock fix, is on master too. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
One conflict, in PartitionState: #469 removed the plain stateChangedSinceCommitStart flag (replaced by the completion counters), and this branch had inserted its loaded-map fields directly beneath it, so the hunk carried the dead flag on this side. Resolved by keeping the new fields and dropping the flag master removed. Main and test sources compile, and the three test classes this branch owns pass on the merged tree. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
The worked example in the tooling doc was recorded from a real vet run and shows the run-length plausibility note, which this branch deletes. The sample is kept as the shape of the output and marked as not a citation, per the file-refs paragraph marker, rather than rewritten to a different note that would not match the rest of the recorded block. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. No quarantined test changed outcome since the previous push. Updated for |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…s down to the run-length guard The poller-death fix squash-merged, and its branch had rewritten item 2 of the sweep gating list in process-candidate-ranking.md, which this branch had already moved into release-v6-scope.md. The conflict resolves to the pointer here and the rewritten item in the moved copy. #477 is ticked in tier 1, six of seven on master; #480 is the last, reworked off the wire and level with master. #486 settled the fifth 857 item as the group protocol and is green with the owner LGTM; one new fact from it is kept, that the zombie-member probe line does not discriminate a PC-side hold from a coordinator join phase. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
marked this pull request as ready for review
September 8, 2026 04:11
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 6m 38s —— View job Review: fix(offsets) — plausibility check moves to the first batch
Note up front: I confirmed via DesignThe core idea is sound and the write-up in
Findings1. Stale doc —
That's the earlier 2. Duplicate-code report — the bot flagged 23 lines shared between 3. Minor / non-blocking:
Not a findingThe javadoc on Overall: the core logic is correct and well-tested (eight cases covering both encodings, both policies, both sides of the boundary, deferral and corroboration). The one thing I'd want fixed before merge is the stale YAML doc, since it actively misdescribes the shipped design. |
…a, not two copies The duplicate-code check on #480 flagged 23 lines shared between ImplausibleOffsetMapOnFirstBatchTest and ForeignOffsetMetadataOnAssignmentTest: the setup that gives a MockConsumer a committed offset with metadata on it and wraps it in a PCModuleTestEnv. This PR introduced the second copy, so it is this PR's to remove. Lifted into OffsetCodecTestUtils, which already owns the shared helpers for these tests, rather than into a new util - docs/testing.md's rule is to extend the existing one. The consumer is now owned by the helper: neither caller touched it after construction, and a test that needs it can read module.options().getConsumer(). Both tests keep a one-line private wrapper naming their own partition and committed offset, so the call sites read as they did. Co-authored-by: Claude Opus <noreply@anthropic.com>
…eck, not the assignment one Review finding on #480: the feature record still said the end offsets of the partitions carrying metadata are read once per assignment, and that the check stands down for that assignment when they cannot be read. That was the ListOffsets revision this branch replaced; the shipped check reads the watermark the first batch already carries and defers to the next batch when it has none. The file was correct when it was written and went stale inside the same PR, which is the drift docs/features/ exists to catch and check-docs-data.sh cannot: it validates structure, never truth. Third time this record has been falsified by the work it describes - the first two are in docs/inflight/pr-207-offset-encoding-policy.md. Also moves the PartitionState import in EncoderOutputSurvivesDecodeValidationTest into its group, which the same review flagged. Co-authored-by: Claude Opus <noreply@anthropic.com>
Owner
Author
|
All three findings addressed - thank you, the first one was real and I had missed it. 1. Stale 2. Duplicate code - fixed in af20b08, before this review landed; the thread is resolved. The shared 3. Minor:
On the "not a finding" note about |
…he mock bin/check-pr-analysis-surfaces.sh reported it on a line this PR wrote, and it is right: nothing in the helper needs MockConsumer, and the wider type is what the production wrapper takes anyway. The other two findings on this PR's lines are answered in the PR conversation rather than changed: ConsumerManager's commitRequested is the known non-volatile field docs/refactoring.md tracks - it moved lines because this PR added a method after it - and the not-initialised-in-constructor report on PartitionState's new constructor is the detector failing to see through initStateFromOffsetData, where a null would be a loud failure and the field-initialiser that silences it would be a silent one. Co-authored-by: Claude Opus <noreply@anthropic.com>
Owner
Author
|
Fixed - acf29b6: Not changed - Not changed - |
Owner
Author
|
Duplication follow-up on the final head, since both engines re-ran:
|
astubbs
added a commit
that referenced
this pull request
Sep 8, 2026
…ency The registry entry for OffsetMapCodecManager said #480 added a blocking endOffsets round trip to decode-on-assignment. That was its first revision; the rework moved the plausibility check off the wire into PartitionState, settled lazily against a watermark ConsumerManager reads without blocking. The codec only passes the committed offset through, so the planned encode/decode split inherits no broker lookup. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 9, 2026
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
The last tier 1 item landed on 2026-09-09: the run-length plausibility guard, reworked off the wire before merge so the claim is settled at the first poll batch against a watermark read without blocking. The INSTANCE_STALL load arm merged the same hour, closing that line's box; what the experiment runner stays for is the ZOMBIE_MEMBER seeds and the busy-observation calibration, named in its row. Every box in tier 1 is ticked. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…d already answers this #480 landed on master while this branch was open and added committedOffsetTheMapWasLoadedAgainst to the same constructor, defaulting to NO_OFFSET_MAP_WAS_LOADED - which is KAFKA_OFFSET_ABSENCE again. So the next reader arrives at two fields that look like the same fact and deletes one. The merge is clean and the two do not conflict; what was missing was the record of why they are different. THE DISCRIMINATOR is that field's own javadoc: NO_OFFSET_MAP_WAS_LOADED is also what "any caller that did not load this state from a committed offset map" passes, which includes every test that builds a PartitionState directly. commitDataWasLoadedOnAssignment is derived from the offset data itself, so it answers what the codec produced rather than what the caller remembered to say. MEASURED, NOT ARGUED. Substituting committedOffsetTheMapWasLoadedAgainst == NO_OFFSET_MAP_WAS_LOADED for the field fails six assertions across three classes - both genuine truncation branches in PartitionStateBootstrapTruncation162Test, two arms of PartitionStateCommittedOffsetTest, and two arms of PartitionStateAbsentCommitData162Test - because those partitions DID load commit data and would go quiet. The javadoc records that, and names what would reopen it: the four-argument constructor gaining the committed offset, at which point the two facts really would be one and this field should be deleted rather than kept in sync. ALSO RECORDED: #480's refusal path calls initStateFromOffsetData a third time, and deliberately does not re-sample this field either. A refused map still leaves a real committed offset to bootstrap against - that is exactly what #480 falls back to - so the truncation branches stay correct for it, which is the opposite of the absent-commit-data case. VERIFIED ON THE MERGED TREE, not assumed: PartitionStateAbsentCommitData162Test, PartitionStateBootstrapTruncation162Test, PartitionStateCommittedOffsetTest, #480's ImplausibleOffsetMapOnFirstBatchTest and EncoderOutputSurvivesDecodeValidationTest, ForeignOffsetMetadataOnAssignmentTest, ConsumerManagerLogEndOffsetTest and the partition-state and offsets unit suites - 273 tests, all green. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
This branch is docs-only - two markdown files differ from master - so neither red it drew can be its own. Both are recorded against the notes that own them, and reading each occurrence turned up a correction the note needed. INTEGRATION TESTS, on this PR's own head. The shape in ci-broker-container-exit-126-is-undiagnosable.md, exactly: one class fell slowly at the container-start timeout and every other broker class fell in milliseconds with NoClassDefFoundError on BrokerIntegrationTest. Codecov renders that as "20 Tests Failed"; it is one failure. What is new is that the cause was in the log all along. Testcontainers prints the failed container's own output at GenericContainer#tryStart, one line below the "Wait strategy failed" line the note's signature block quotes, and it reads "sh: /tmp/testcontainers_start.sh: Text file busy" - ETXTBSY, exec refused because the starter script was still open for writing. The container command waits for that script to EXIST and then executes it, so a file the daemon has created but not finished extracting is executable-shaped and not executable, and the shell reports the refusal as exit 126. A Testcontainers start race, widened by a busy runner; nothing in the product, the image or the Kafka configuration. Refetching #347's 2026-08-25 job, the run this note was written from, shows the identical two lines. So the note's premise under item 1 - "the container's stdout is nowhere in the job log" - was false of its own founding evidence. The instrument was fine; the triage stopped one line short. That correction is proposed in the note's vetting marker rather than applied, because the note's impact is misdirection and those are the owner's to close. CHAOS PAIN SUITE 4/4, on #495 - also docs-only, one README paragraph. ChaosChurnStormIT NO_PROGRESS at 96632/100000 for 30s against a 30s bound, seed 3717713223451201639. It goes in test-no-progress-window-may-not-transfer-to-w1.md as one appended row, with the part that makes it worth having: the fleet KEPT CONSUMING, reaching 99569 by the settle summary, so the outstanding count fell from 3368 to 431 - inside the TAIL_SLACK of 500. That is the "drains" branch of the deciding experiment the note states. It is the weak form and the row says so: no recovery diagnostic, so the counter compared is the ledger's rather than the probe's, and the conductor's churn ended 10s after the firing, so it is recovery-once-churn-stops. The bigger finding is that the deciding experiment had already been answered twice and this note never took delivery. test-857-churn-storm-async-stalls.md drained six for six on seed 9086872209853284830 with the diagnostic engaged, and its 2026-09-08 sighting drained seed 5650361238717170909 from 93487 to 101070/100000 at an outstanding count of 6513 - larger than every row in the table. That sighting says outright that this note owns the question; the pointer was written and nobody followed it. Proposed in the vetting marker for the same reason as above. RULED OUT, with a control arm rather than an argument. The six merges that landed on master today - #480, #487, #488, #491, #492 and #493 - are the obvious suspects for a chaos red, and #491 does touch ProgressProbe.java. Its diff does not touch the NO_PROGRESS path at all - it adds the UNCOMMITTED_COMPLETIONS detector, edits javadoc, and refactors the finding sink - and the same test PASSED on two heads that carry every one of those merges, four and six minutes either side of the failing run. A deterministic regression is excluded; a rate change is not, and one failure could not establish one. Nothing quarantined. The container fault has no test to quarantine and the exit-126 note says a re-run is the correct response there. The chaos firing has no rate that rule 1 would accept, and docs/quarantined-tests.md is empty - which is the state to preserve. Co-Authored-By: Claude Opus <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
astubbs
added a commit
that referenced
this pull request
Sep 9, 2026
…bsence is recorded rather than inferred (#494) WHAT WAS WRONG. PartitionState#maybeTruncateBelowOrAbove took its bootstrap expectation from getOffsetToCommit() and never asked whether commit data existed. A partition assigned with no committed offset is built from OffsetMapCodecManager.HighestOffsetAndIncompletes.of(), so offsetHighestSucceeded is KAFKA_OFFSET_ABSENCE and the expectation computes to 0 - and any first poll above offset 0 took the above-expected branch, logged "Truncating state - removing records lower than ..." at WARN, and pruned nothing, because there was nothing loaded to prune. False in both halves. It fired for the normal state of a new consumer group, and for a group whose committed offset had aged out of the offsets topic. That is the shape reported upstream on 0.5.2.7 - after confluentinc#563 shipped and fixed the OTHER form of the same string - alongside a broker CLI screenshot showing an empty CURRENT-OFFSET for exactly the partitions that warned. THE BEHAVIOUR NOW. Absent commit data reports at INFO and takes no truncation branch: "No committed offset for partition N of topic T - starting from the first polled offset X. Nothing was loaded, so nothing is being removed. Expected for a new consumer group, or where the group's committed offset has aged out of the offsets topic." The WARN is unchanged for the genuine cases - a committed offset the broker no longer has records for, compaction, a raised committed offset - so an operator alerting on "Truncating state" keeps exactly the alert they had, minus the false firings. WHY NOT THE SENTINEL, WHICH IS THE WHOLE OF THE DESIGN. The cheap check reads offsetHighestSucceeded == KAFKA_OFFSET_ABSENCE, and it is WRONG: a real commit filed at offset 0 with no offset map decodes to a highest-seen of 0 - 1, which is that same sentinel, with commit data that genuinely existed and an expectation of 0 that is genuinely meant. Testing the sentinel would silence the true truncation report for exactly the partition most likely to be caught by retention - the one that has processed nothing yet. -1 means "nothing succeeded", not "no commit arrived". So the absence is carried from where it is known: every decode path yields a PRESENT HighestOffsetAndIncompletes#getHighestSeenOffset, because every decode has a committed offset to be relative to, and only the default entry yields an empty one. PartitionState samples that into a final boolean in its constructor, before initStateFromOffsetData collapses the Optional into the sentinel - the engine AGENTS.md collapse-parallel-state rule applied to a fact rather than a cache. The field being final is also the whole thread-safety story: publication is the constructor's freeze, and it is read on the control thread in the same statement that closes bootstrapPhase. RED FIRST, THEN THE CONTROL ARM. PartitionStateAbsentCommitData162Test was red on master in three arms - the first poll above 0 warned, the assignment path through a consumer reporting no committed offset warned, and the at-offset-0 arm had no INFO line to find - and green after. The fourth arm is the control: same sentinel, same expectation of 0, same first polled offset, one term differing (commit data existed), and it must still warn. Prediction stated before running: substituting the sentinel shorthand for the recorded fact fails ONLY that arm. Run with the shorthand substituted, it did exactly that - one failure, the control arm, with the absent-data arms and PartitionStateBootstrapTruncation162Test's two genuine branches still green. A CLAIM CORRECTED ON THE WAY PAST. The inflight note said #217's foreign-metadata recovery lands on the same default entry through catch (OffsetDecodingError), and so inherits the false warning. It does not. Under the runtime-default IGNORE policy, EncodedOffsetPair#handleUnreadableMetadata returns of(baseOffset - 1) - commit data carrying the committed offset - so such a partition bootstraps with a real expectation and the truncation branches stay correct for it; under FAIL the typed exception propagates instead. Neither reaches the default entry, and the only production caller of OffsetEncoding#decode is EncodedOffsetPair#unwrap, which has test callers only. Asserted by the test's last arm; the now-dead catch is recorded in docs/refactoring.md. DEFECT-CLASS SWEEP - other places deriving "no commit data" from sentinel arithmetic. Every reader of KAFKA_OFFSET_ABSENCE and of the offset accessors in main was checked, and none is the same defect: OffsetSimultaneousEncoder's highestSucceededOffset == KAFKA_OFFSET_ABSENCE asks about SUCCESS, which the sentinel answers faithfully; isRecordPreviouslyCompleted's recOffset <= offsetHighestSucceeded is correct under both readings; RemovedPartitionState returns the sentinel as a deliberate no-op contract and never reaches truncation; RunLengthEncoder's use is a loop initialiser. Two sites borrow the constant for a second meaning without being defects, and are recorded in docs/refactoring.md: getOffsetHighestSequentialSucceeded uses it as a below-every-offset floor for ceiling(), and PartitionStateManager#incrementPartitionAssignmentEpoch defaults an EPOCH from an OFFSET constant. WHY THIS CLOSES THE ISSUE. All three defects behind confluentinc#546's one warning string are settled here: the "expected 1" RunLength decode by inheritance from confluentinc#563; the "expected 0 from loaded commit data" form by this change; and the "reset to an earlier offset" branch refuted as a PC-reachable defect by #484. Upstream's own issue stays open - that repository is no longer maintained. RECORDS. docs/inflight/bug-162-offset-state-truncation.md is deleted, which is its own "Delete when" condition. What outlives it - the three-defects reading, the sentinel-versus-fact discriminator and the #217 correction - is migrated to docs/solutions/logic-errors/. The drafted mirror reply moves to docs/inflight/issue-response-162.md, held for the pre-release sweep rather than deleted with this PR. Citations in process-candidate-ranking.md and upstream-mirror-bodies-are-stale.md are repaired, and src/docs/development/upstream-map.yaml records the lifecycle transition. Closes #162 Upstream-Issue: confluentinc#546 A SIBLING FIELD, AND WHY IT IS NOT THE SAME FACT. #480 landed while this was open and added committedOffsetTheMapWasLoadedAgainst to the same constructor, defaulting to the same absence sentinel, so the next reader meets two fields that look like one and deletes one. They differ: that field records what the caller passed, and NO_OFFSET_MAP_WAS_LOADED is also what every direct constructor call passes; this one is derived from the offset data itself, so it answers what the codec produced. Measured, not argued - substituting one for the other fails six assertions across three test classes, because partitions that did load commit data would go quiet. The javadoc records that and names what would collapse the two into one: the constructor gaining the committed offset, at which point this field is deleted rather than kept in sync. #480's refusal path, which re-initialises from the offset data a third time, deliberately does not re-sample this field either: a refused map still leaves a real committed offset to bootstrap against. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Sep 10, 2026
…ut it (#475) 0.6.0.0 is a bugs-only stability release, and it is overdue: the fork has carried the fixes for upstream's most-reported defects for months while the release waited on features. This note is the source of truth for cutting it - the owner's decisions, the merge queue from those decisions to the tag, every open question, and the checks that make the published artefacts true on the day. #197 is the tracking handle and its body points here; nothing is maintained on the issue. THE DECISIONS, 2026-09-07 and confirmed since. The bar is the stability release and nothing else; Streams and Connect move to the next-0x horizon in the roadmap data; the producer-recovery stack is outside v6. The release claim carries two named exceptions rather than waiting on them: the transactional revoke wait (#44, bounded since #466, not yet declined) and, from 2026-09-09, the poisoned-transaction wedge, both in the transactional producer mode only. The merge queue closed on 2026-09-09; later finds are 0.6.0.x unless they are data loss on a default configuration. THE BURN-DOWN, recorded as each merge landed. Tier 1, the self-contained fixes, is complete: the last two to join were the batchSize bound (#496) and the gate-latch warning (#497), both decided v6-sized on the day the queue closed. Tier 3, the plumbing, has the changelog section finalised as the release notes and the claim amended (#498) and the release page body posted verbatim from CHANGELOG.md by release.yml (#501, closing #199); what remains is the tag-day checks, the drafted issue responses, and the tag. A can-follow list names what is deliberately not v6. WHAT THE RELEASE NOTE SAYS ABOUT THE confluentinc#857 FAMILY, each line with the PR that settled it: the revoke-path deadlock proven by control arm, the eager stall withdrawn as a timing bound that flips with the processor count, the fifth item measured as the consumer-group protocol under churn rather than PC, the poller death fixed, the instance-stall sightings classified as worker saturation from the load side. The intake stall #471 found has its verdict from #487: the record-intake load gate is what stops intake, head-of-line blocking is not why, and any instance that retries forever while a fraction of its stream never succeeds latches eventually at a computable threshold, idle or not. There is no gate fix; the fix bounds the failures (#149's dead-letter queue), and until then #497 makes the state visible. One arm stays unattributed and is named as such. DATA LOSS AND DUPLICATES: the bug-162 replay branch refuted and the false truncation warning fixed (#494, closing #162). KNOWN UNKNOWNS, split in two so nothing is papered over: what is still unknown at the cut - the shard half of the per-shard liveness blind spot, the flake rows kept open with reasons, the maturity claim - and, under its own heading, the unknowns made known on 2026-09-08 and how each was settled. TAG-DAY CHECKS, folded in from the retired blockers note: master green with the lanes known to lie named, the churn scenario's no-progress window settled by replay and widened in #499 with the rebalance-dwell bound named as that class's survivor, the Lincheck lane's timeout raised against runner-speed variance, the rename named in both groupId and packages, the README's trademark wording claiming nothing it does not have (#495), and the changelog section as the release notes since #498, posted as the release body by release.yml since #501. ONE CHANGELOG EDIT, on the owner's decision of 2026-09-10: the "size of this release" table of merged-PR and line counts is removed. Measured on a branch, carrying its own re-measure instruction, stale from the next merge on; the notes make their claim through the fixes they name. Also here: a ci- note from this PR's own last review round - the file-refs gate reads a token as a path only with two segments, so the changelog rename left this branch-only note naming the old file with nothing to go red, and the note records the allow-list that would close it; the vetting sweep's reading and every open bug note's disposition, moved into the ranking note where the tiers override them; a dated survey of upstream items with no fix and no response as its own deferred note; the refactoring registry's codec entry corrected for what #480 did and did not change; and the confluentinc#546 manifest entry marked merged. Two notes retired with their content migrated: the blockers register and the merge-order plan for a far larger v6. The question this note began as, "when is v6 good enough?", was answered on 2026-09-08 and the file renamed. Serves #197; closes nothing. The tracker closes when the tag is cut. Co-authored-by: Claude Fable 5.1 (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.
Part of #197 (the v6 release), and the last open item from
#207's offset-metadata work. There is no fork issue for this defect - it
was tracked as
docs/inflight/bug-run-length-plausibility-ceiling.md, which this PR retires.Description
A structurally valid run-length entry of
Integer.MAX_VALUEmoves the highest-seen offset about twobillion forward.
PartitionState#isRecordPreviouslyCompletedthen reads every real record in thatrange as already succeeded, so PC skips them without ever calling the user function - silent
non-processing rather than replay, reachable through any metadata written against the consumer
group. #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.
The bound is the partition - and it is read off the fetch, not off the wire
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 merely disliked:
keys off the encoded payload size, and a map with one stuck offset stays three entries wide
however far the partition runs ahead of it - so one poison record legitimately produces an
enormous range, and any such ceiling eventually discards a true map and replays everything in it.
An earlier revision of this PR bought the end offset with a
ListOffsetsrequest at assignment,and that is what the current revision removes. Two objections, both recorded in the code and in
the solutions write-up so they are not re-litigated: 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 same fact already arrives for free, slightly later: every fetch response carries the partition's
high watermark, and
Consumer#currentLag(KIP-695, kafka-clients 3.0+) exposes it without a requestof its own.
ConsumerManager#logEndOffsetIfKnownWithoutBlockingreconstructs it asposition + currentLag, both read in one breath on the thread that owns the consumer, with azero-timeout position read so nothing can block;
EpochAndRecordsMapcarries the answer alongsidethe records.
The choke point
PartitionState#maybeRegisterNewPollBatchAsWork- after the epoch fence, beforemaybeTruncateOrPruneTrackedOffsetsand before the per-record loop. Those two are the only thingsthat consult the loaded map, so the claim is settled before anything can be skipped against it.
Waiting costs nothing: nothing consults the map until a record arrives to be consulted about.
Absence is not evidence.
currentLagis empty until a fetch has happened, so "not established"is a real answer and the check simply looks again at the next batch. In the meantime the batch's own
records act as a floor - a record that arrived certainly exists - and a floor can only ever
confirm a claim, never refute one, which is why it is safe to act on with no watermark at all.
Same policy, wider coverage than the decode-side version
A refused map goes through
EncodedOffsetPair#handleUnreadableMetadatalike every other payloadthis build cannot read: the default
IGNOREdiscards it, resumes from the committed offset andwarns with the partition, the base offset and the reason;
FAILthrows. Nothing counts the discarddocs/inflight/bug-no-metric-for-discarded-offset-metadata.md, cited from the code andout of scope here.
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) - where the decode-side version had to be threaded
through each decoder.
OffsetSimpleSerialisationwas checked and carries none of this defect class:base64, the zstd/gzip/snappy codecs, and a byte-per-offset bitmap whose length is the payload
length, with no declared count anywhere.
Evidence
Red by sabotage: with the call to
maybeVerifyLoadedOffsetMapAgainstThePartitionremoved andnothing else changed, five of the eight cases in
ImplausibleOffsetMapOnFirstBatchTestfail -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 shows the controls do not depend on the guard.
Encoder side kept: every shape
EncoderOutputSurvivesDecodeValidationTestproduces is measuredagainst the rule with the tightest honest bound there is - a partition ending exactly on the last
offset encoded - so anything that made the encoder write one offset past what it saw lands as a
false rejection instead of as silence.
MockConsumer, verified rather than assumed (kafka-clients 3.9.1):currentLaganswersendOffsets - positionwhenupdateEndOffsetshas been called, andOptionalLong.of(0)when ithas 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; all four behaviours
are pinned in
ConsumerManagerLogEndOffsetTestso the next test to reach for this does notrediscover them.
The full core unit suite is green locally.
Left open, deliberately
The decode-side ceiling this branch previously carried also stopped a huge incomplete run from
being walked into a
TreeSetbefore anything could check it; the lazy check cannot, because it runsafter the decode. That is master's status quo rather than a regression introduced here, and it now
has a note:
docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md.Checklist
docs/features/invalid-offset-metadata-policy.yaml(its boundary claim wasfalsified by this change), the retired in-flight note, the new
docs/solutions/entry (thebound's anchor and why the wire call was rejected), the new note for what is left open, and
the javadoc on the check itself
docs/features/- the existinginvalid-offset-metadata-policy.yamlrecord is corrected rather than a new one addedImplausibleOffsetMapOnFirstBatchTest(eight cases through the realpoll path, both encodings, both policies, both sides of the boundary, deferral and
corroboration),
ConsumerManagerLogEndOffsetTest(the non-blocking watermark read and theMockConsumer findings), and
EncoderOutputSurvivesDecodeValidationTestnow measures everyencoder shape against the rule
docs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - thisPR closes a note rather than opening one, and carries nothing about itself that
ghcannotshow; what outlives it is in
docs/solutions/and the one note abovece-simplifyandce-code-reviewlocally - N/A - asking for@claude review thison thePR instead, which is the route that can open inline threads