From 85ef9f3416035ff5a4d1dd8ab74e2a93e0f43174 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 13:35:35 +1200 Subject: [PATCH 1/8] fix(offsets): a run length the partition cannot hold is an unreadable 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. astubbs#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 - astubbs#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 --- .../bug-run-length-plausibility-ceiling.md | 25 -- .../inflight/pr-207-offset-encoding-policy.md | 2 - ...truth-not-a-plausible-number-2026-09-08.md | 126 ++++++++ .../offsets/EncodedOffsetPair.java | 61 +++- .../offsets/OffsetBitSet.java | 25 +- .../offsets/OffsetMapCodecManager.java | 124 +++++++- .../offsets/OffsetRunLength.java | 55 +++- ...derOutputSurvivesDecodeValidationTest.java | 30 +- .../ImplausibleOffsetMapOnAssignmentTest.java | 298 ++++++++++++++++++ 9 files changed, 686 insertions(+), 60 deletions(-) delete mode 100644 docs/inflight/bug-run-length-plausibility-ceiling.md create mode 100644 docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md create mode 100644 parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java diff --git a/docs/inflight/bug-run-length-plausibility-ceiling.md b/docs/inflight/bug-run-length-plausibility-ceiling.md deleted file mode 100644 index 47f2070ab..000000000 --- a/docs/inflight/bug-run-length-plausibility-ceiling.md +++ /dev/null @@ -1,25 +0,0 @@ -# A structurally valid but implausibly large run length is still accepted - - - - - - -A `RunLengthV2` entry of `Integer.MAX_VALUE` moves the highest-seen offset about two billion forward. -That marks the whole range as already succeeded, so `PartitionState#isRecordPreviouslyCompleted` skips -every record in it - silent non-processing, not replay. - -**Why astubbs#207 did not fix it while fixing its neighbours.** That PR rejects what the payload itself -proves wrong: a negative length or run, a declared bit length with no bytes behind it, a run-length body -that is not a whole number of entries, an empty run-length body. A long run of *completed* offsets is -exactly what run-length encoding is **for**, so nothing in the bytes distinguishes an absurd run from a -legitimate one, and the decoder has no partition end offset to check it against. - -**The decision.** Bounding it means choosing a plausibility ceiling - a number with no principled -derivation, which would reject a real offset map if set too low. That is a product call, not a -correctness one, and guessing it is worse than leaving the gap recorded. - -Reachable today by anything that can write offset metadata for the consumer group, so it is not purely -theoretical - but it needs metadata that is well-formed as far as the encoder is concerned. - - diff --git a/docs/inflight/pr-207-offset-encoding-policy.md b/docs/inflight/pr-207-offset-encoding-policy.md index 08464adb8..c9a6ab884 100644 --- a/docs/inflight/pr-207-offset-encoding-policy.md +++ b/docs/inflight/pr-207-offset-encoding-policy.md @@ -151,8 +151,6 @@ Each of these outlived this PR and now has its own note, so nothing is restated - [`core-bytearray-encodings-have-no-codec.md`](core-bytearray-encodings-have-no-codec.md) - the two encoder-less, decoder-less `OffsetEncoding` constants, and the wire-format decision about them. -- [`bug-run-length-plausibility-ceiling.md`](bug-run-length-plausibility-ceiling.md) - a structurally - valid but implausibly large run length, which nothing in the payload proves wrong. - [`core-fail-policy-escapes-the-rebalance-callback.md`](core-fail-policy-escapes-the-rebalance-callback.md) - `FAIL` stopping via Kafka's generic callback wrapper rather than PC's own fatal path. diff --git a/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md b/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md new file mode 100644 index 000000000..4a8ef5e00 --- /dev/null +++ b/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md @@ -0,0 +1,126 @@ +--- +title: "Bound a guard with ground truth, not a plausible number - the run-length ceiling that stayed open for a year" +date: 2026-09-08 +category: architecture-patterns +module: parallel-consumer-core/offsets +problem_type: architecture_pattern +component: offset_encoding +root_cause: the_only_available_bound_was_a_guess_so_the_guard_was_never_written +resolution_type: code_fix +severity: high +applies_when: + - A validator must reject an input that is structurally legal but semantically impossible + - The obvious bound is a constant somebody has to choose, and choosing it wrong rejects real data + - A guard has been parked as "a product call" because nobody could justify the number + - Deciding whether a check may make a network call, and what it should do when that call fails +related_components: + - OffsetRunLength + - OffsetBitSet + - OffsetMapCodecManager + - EncodedOffsetPair +tags: + - guard-predicate + - ground-truth + - fail-open + - offset-encoding + - input-validation + - wire-format + - issue-197 +--- + +# Bound a guard with ground truth, not a plausible number + +## Context + +PC stores its offset map in the commit metadata, and one encoding is run-length: a list of counts of +completed and incomplete offsets. astubbs#207 hardened that 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 routed every rejection through the user's `invalidOffsetMetadataPolicy`. + +One case was left, and recorded as parked: a run length of `Integer.MAX_VALUE` is structurally +perfect. It moves the highest-seen offset about two billion forward, and `PartitionState`'s +`isRecordPreviouslyCompleted` then reads every real record in that range as already succeeded. Not +replay - **silent non-processing**, from metadata anything sharing the consumer group can write. + +The note that tracked it said, correctly, that a long run of completed offsets is exactly what +run-length encoding is *for*, so nothing in the payload proves an absurd one wrong; and it concluded +that bounding it "means choosing a plausibility ceiling - a number with no principled derivation", +which is a product call rather than a correctness one. That framing is what kept the defect open: it +treats "pick a constant" as the only available shape for the guard. + +## Guidance + +**When a check needs a bound, ask what in the system already knows the answer before you invent a +number.** Three candidate bounds were considered here, and only one is not a guess: + +- **A configured window** - concurrency, the in-flight target, "nobody legitimately gets this far + ahead". Refuted by the product itself: PC's 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. A single poison record therefore produces a legitimately enormous range, so any such + ceiling eventually discards a true offset map - and discarding one replays every record it covers. +- **A round number** ("no partition legitimately runs a billion ahead of its commit"). The same + objection with less honesty about it, and it fails silently: the day it is wrong, a real map is + thrown away and nothing distinguishes that from the corrupt case it was written for. +- **The partition's log end offset** - the one bound a legitimate map provably cannot cross. PC only + ever encodes offsets it has polled, an offset the partition does not hold cannot have been polled, + and Kafka's end offset only grows, so a bound read *now* still holds for a map written earlier. + There is no false-positive case to trade off, which is what makes it a correctness bound rather + than a tuning parameter. + +**The cost of ground truth is usually a call somebody was avoiding - price it rather than assume +it.** The end offset is not free: it is a `ListOffsets` round trip, and the natural place to make it +is inside the rebalance callback, which in this codebase is the most fragile path there is. What +made it affordable was scoping: one batched request for the whole assignment, skipped entirely when +no assigned partition carries metadata, on a rebalance that has already paid for a `JoinGroup`, a +`SyncGroup` and an `OffsetFetch`. A per-partition or per-decode lookup would not have been. + +**A guard with no ground truth must stand down, not guess.** When the lookup fails - an unavailable +leader, a timeout, a consumer implementation that will not answer - the decoders are handed +"unknown" and refuse nothing. Failing *closed* would mean a broker hiccup discards honest offset +maps, which is a far more likely event than the corrupt payload the guard exists for; the check +would then cause more data churn than it prevents. The failure is logged with the partitions it +covers, so the stand-down is visible rather than assumed. + +**Check the producing side, and pin it at the boundary.** A decode-side bound is only safe if the +encoder can never write what it rejects. Here it cannot, because the encoder's range top is an +offset PC actually saw - but the argument is worth a test rather than a paragraph: every encoder +round-trip case is now decoded twice, once unbounded and once against a partition ending exactly on +the last offset encoded. Anything that made the encoder run one offset past what it saw surfaces as +a false rejection instead of as silence. + +**Take the whole defect class while the parameter is in your hand.** The bitset decoder has the same +shape - a structurally valid length field the format cannot prove absurd - and is merely *less* +exposed, because its declared bit count has to be backed by bytes that are present, so a full 4KB +metadata field buys tens of thousands of skipped records rather than two billion. "Bounded" is not +"true", and the same ceiling now applies there. + +## Why This Matters + +The defect was correctly diagnosed, correctly filed, and then sat open - not because it was hard to +fix but because the fix had been framed as a number nobody could justify. That framing is the durable +lesson: **"this needs a constant we cannot derive" is a signal to go looking for an authority, not a +reason to park the work.** The authority here was one method call away on an object the code already +held. + +It also inverts the usual reason a guard is refused. The objection to bounding a run length was that +the check might be wrong; the check that was finally written *cannot* be wrong in the expensive +direction, and the only thing traded away is a round trip on a path that already makes several. + +## When to Apply + +- Writing any validator for a value whose legality depends on something outside the payload: a + length, an offset, a count, a timestamp, an id range. +- Reviewing a parked bug whose blocker is "we would have to pick a threshold". +- Deciding what a guard does when the fact it needs is unavailable - name the direction it fails in + and say why that direction is the cheaper wrong answer. + +## Related + +- [a-guard-must-assert-what-it-means-not-what-is-easy-to-check.md](a-guard-must-assert-what-it-means-not-what-is-easy-to-check.md) - + the sibling rule, and the step before this one: this document assumes you already know what the + guard should mean, and is about where the number it compares against comes from. +- [`docs/inflight/pr-207-offset-encoding-policy.md`](../../inflight/pr-207-offset-encoding-policy.md) - + the policy this guard routes through, why the default is `IGNORE`, and the corrupt-payload family + it already covers. +- [`docs/inflight/bug-no-metric-for-discarded-offset-metadata.md`](../../inflight/bug-no-metric-for-discarded-offset-metadata.md) - + still open: every discard here is logged and nothing counts it. diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java index 7342c9c3d..c7b2b955b 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java @@ -129,6 +129,22 @@ static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, long baseOffset, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) { + return decodeToIncompletes(input, baseOffset, errorPolicy, tp, OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING); + } + + /** + * @param highestOffsetPartitionCanHold the last offset the partition actually holds, which is the only thing that + * can prove an otherwise well-formed offset map absurd - see + * {@link OffsetRunLength#runLengthDecodeToIncompletes}, which owns that + * reasoning. {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when + * the caller could not find out + * @see #decodeToIncompletes(byte[], long, InvalidOffsetMetadataHandlingPolicy, TopicPartition) + */ + static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, + long baseOffset, + InvalidOffsetMetadataHandlingPolicy errorPolicy, + TopicPartition tp, + long highestOffsetPartitionCanHold) { if (input.length == 0) { // Not reachable from production today: decodeCompressedOffsets branches on an empty payload before // calling here, because "no metadata committed" is a legitimate state rather than a corrupt one. That @@ -152,7 +168,7 @@ static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, tp); } return new EncodedOffsetPair(encoding.get(), wrap.slice()) - .getDecodedIncompletes(baseOffset, errorPolicy, tp); + .getDecodedIncompletes(baseOffset, errorPolicy, tp, highestOffsetPartitionCanHold); } /** @@ -261,8 +277,11 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, Invali * {@link CorruptOffsetMetadataException}, which is the case that used to return a fabricated offset map * instead of failing. * - * A fourth outcome - a payload that decodes cleanly into a wrong-but-plausible map - is not covered, and - * cannot be: nothing in such a payload proves it wrong. + * A fourth outcome - a payload that decodes cleanly into a wrong-but-plausible map - is covered only as far as + * something outside the payload can prove it wrong, because nothing in such a payload does. Today that + * is the partition itself: a map naming offsets the partition does not hold is refused, given a caller that + * passes {@code highestOffsetPartitionCanHold}. A map that is merely wrong about offsets which do exist remains + * indistinguishable from a real one. * * @param baseOffset the committed offset the payload is relative to, and what {@code IGNORE} falls back to * @param errorPolicy what to do when this build cannot read the payload @@ -270,10 +289,24 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, Invali * the caller does not know it * @return the highest offset seen, and the incomplete offsets below it */ - @SneakyThrows public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) { + return getDecodedIncompletes(baseOffset, errorPolicy, tp, OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING); + } + + /** + * @param highestOffsetPartitionCanHold the last offset the partition actually holds, or + * {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when unknown. It + * settles the fourth outcome the overload below calls uncoverable: a payload + * that decodes cleanly into a map no partition could produce + * @see #getDecodedIncompletes(long, InvalidOffsetMetadataHandlingPolicy, TopicPartition) + */ + @SneakyThrows + public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, + InvalidOffsetMetadataHandlingPolicy errorPolicy, + TopicPartition tp, + long highestOffsetPartitionCanHold) { switch (encoding) { case KafkaStreams: case KafkaStreamsV2: @@ -305,7 +338,7 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, // ZstdIOException from a body that is not a zstd frame, neither of which is an OffsetDecodingError, so // loadPartitionStateForAssignment's recovery never saw them and they escaped onPartitionsAssigned. try { - return decodeBody(baseOffset); + return decodeBody(baseOffset, highestOffsetPartitionCanHold); } catch (CorruptOffsetMetadataException | BufferUnderflowException | IOException e) { return handleUnreadableMetadata(baseOffset, errorPolicy, @@ -327,16 +360,16 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, * every way this can fail is caught in one place and routed through the user's policy, rather than each decoder * having to know about it. */ - private HighestOffsetAndIncompletes decodeBody(long baseOffset) throws CorruptOffsetMetadataException, IOException { + private HighestOffsetAndIncompletes decodeBody(long baseOffset, long ceiling) throws CorruptOffsetMetadataException, IOException { return switch (encoding) { - case BitSet -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data); - case BitSetCompressed -> deserialiseBitSetWrapToIncompletes(BitSet, baseOffset, decompressZstd(data)); - case RunLength -> runLengthDecodeToIncompletes(encoding, baseOffset, data); - case RunLengthCompressed -> runLengthDecodeToIncompletes(RunLength, baseOffset, decompressZstd(data)); - case BitSetV2 -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data); - case BitSetV2Compressed -> deserialiseBitSetWrapToIncompletes(BitSetV2, baseOffset, decompressZstd(data)); - case RunLengthV2 -> runLengthDecodeToIncompletes(encoding, baseOffset, data); - case RunLengthV2Compressed -> runLengthDecodeToIncompletes(RunLengthV2, baseOffset, decompressZstd(data)); + case BitSet -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data, ceiling); + case BitSetCompressed -> deserialiseBitSetWrapToIncompletes(BitSet, baseOffset, decompressZstd(data), ceiling); + case RunLength -> runLengthDecodeToIncompletes(encoding, baseOffset, data, ceiling); + case RunLengthCompressed -> runLengthDecodeToIncompletes(RunLength, baseOffset, decompressZstd(data), ceiling); + case BitSetV2 -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data, ceiling); + case BitSetV2Compressed -> deserialiseBitSetWrapToIncompletes(BitSetV2, baseOffset, decompressZstd(data), ceiling); + case RunLengthV2 -> runLengthDecodeToIncompletes(encoding, baseOffset, data, ceiling); + case RunLengthV2Compressed -> runLengthDecodeToIncompletes(RunLengthV2, baseOffset, decompressZstd(data), ceiling); default -> throw new PCInternalRuntimeException( msg("no decoder for {}, and it was not routed to the policy handler", encoding)); }; diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java index 2341f5c5a..b845007da 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java @@ -56,7 +56,20 @@ static String deserialiseBitSet(int originalBitsetSize, ByteBuffer s) { return result.toString(); } - static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEncoding encoding, long baseOffset, ByteBuffer wrap) + /** + * @param highestOffsetPartitionCanHold the last offset the partition actually holds, or + * {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when the caller + * could not find out. Same ground truth, and same reasoning, as + * {@link OffsetRunLength#runLengthDecodeToIncompletes}, which owns the + * explanation. A bitset cannot claim as far as a run length can - the + * declared bit count has to be backed by bytes that are present, so the + * claim is capped by the payload's own size - but "capped" is not "true", + * and a full metadata field still buys tens of thousands of skipped records. + */ + static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEncoding encoding, + long baseOffset, + ByteBuffer wrap, + long highestOffsetPartitionCanHold) throws CorruptOffsetMetadataException { wrap.rewind(); int originalBitsetSize = switch (encoding) { @@ -78,8 +91,16 @@ static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEnco "bitset declares {} bit(s), needing {} byte(s), but only {} byte(s) follow the length field", originalBitsetSize, bytesNeeded, slice.remaining())); } - SortedSet incompletes = deserialiseBitSetToIncompletes(baseOffset, originalBitsetSize, slice); long highestSeenOffset = baseOffset + originalBitsetSize - 1; + // Checked before the set is built, for the same reason the run-length guard is checked before its run is + // walked: a payload we are about to refuse should not be allowed to allocate first. + if (highestOffsetPartitionCanHold != OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING + && highestSeenOffset > highestOffsetPartitionCanHold) { + throw new CorruptOffsetMetadataException(msg( + "bitset declares {} bit(s), reaching offset {}, but the partition holds nothing above offset {}", + originalBitsetSize, highestSeenOffset, highestOffsetPartitionCanHold)); + } + SortedSet incompletes = deserialiseBitSetToIncompletes(baseOffset, originalBitsetSize, slice); return HighestOffsetAndIncompletes.of(highestSeenOffset, incompletes); } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java index 566fe2c7f..aa60d476f 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java @@ -17,6 +17,7 @@ import io.micrometer.core.instrument.Timer; import lombok.Value; import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.Consumer; import org.apache.kafka.clients.consumer.OffsetAndMetadata; import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.errors.WakeupException; @@ -25,6 +26,7 @@ import java.nio.charset.Charset; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; import static bz.stub.parallelconsumer.internal.utils.StringUtils.msg; import static java.nio.charset.StandardCharsets.UTF_8; @@ -70,6 +72,16 @@ public class OffsetMapCodecManager { public static final Charset CHARSET_TO_USE = UTF_8; + /** + * Passed as the plausibility ceiling when nobody could establish where the partition ends, and read by the + * decoders as "refuse nothing on these grounds". + *

+ * A guard with no ground truth must stand down rather than invent one: failing closed here would discard honest + * offset maps whenever a broker is slow to answer, which is a far more likely event than the corrupt payload the + * ceiling exists for. {@link OffsetRunLength#runLengthDecodeToIncompletes} owns what the ceiling means. + */ + public static final long UNKNOWN_PARTITION_CEILING = Long.MAX_VALUE; + private final PCModule module; private Timer offsetEncodingTimer; @@ -202,10 +214,12 @@ public Map> loadPartitionStateForAssignment } var partitionStates = new HashMap>(); + var highestOffsetsHeld = findHighestOffsetsHeld(partitionLastCommittedOffsets); partitionLastCommittedOffsets.forEach((tp, offsetAndMeta) -> { if (offsetAndMeta != null) { try { - PartitionState state = decodePartitionState(tp, offsetAndMeta); + PartitionState state = decodePartitionState(tp, offsetAndMeta, + highestOffsetsHeld.getOrDefault(tp, UNKNOWN_PARTITION_CEILING)); partitionStates.put(tp, state); } catch (OffsetDecodingError offsetDecodingError) { log.error("Error decoding offsets from assigned partition, dropping offset map (will replay previously completed messages - partition: {}, data: {})", @@ -228,6 +242,61 @@ public Map> loadPartitionStateForAssignment return partitionStates; } + /** + * Asks the broker where each partition carrying an offset map actually ends, so a decoded map can be checked + * against something it cannot forge. + *

+ * This is the ground truth the plausibility ceiling needs, and there is nowhere cheaper to get it: PC holds no + * end offset of its own at assignment time, and a constant derived from configuration would eventually discard a + * true offset map - {@link OffsetRunLength#runLengthDecodeToIncompletes} carries that argument in full. + *

+ * One request, and only when there is something to check. It is batched across the whole assignment and + * skipped entirely when no assigned partition has metadata, so it costs one {@code ListOffsets} round trip on a + * rebalance that already paid for several - and nothing at all for a group that has never committed a map. The + * timeout is the consumer's own {@code default.api.timeout.ms}, the same one governing the + * {@link Consumer#committed(Set)} call above it, because it is the same kind of call in the same frame. + *

+ * Every failure is swallowed on purpose, loudly. A guard must not be able to turn a readable offset map - + * or an entire partition assignment - into a failure: an unavailable leader, a timeout, or a consumer + * implementation that does not answer this question all mean "no ground truth", which the decoders read as + * "refuse nothing". The warning is the visible half; the invisible half would be a rebalance that dies for the + * sake of a check. + * + * @return the last offset each partition holds, keyed by partition and absent where it could not be established + */ + private Map findHighestOffsetsHeld(Map partitionLastCommittedOffsets) { + var partitionsCarryingAMap = partitionLastCommittedOffsets.entrySet().stream() + .filter(entry -> entry.getValue() != null) + .filter(entry -> entry.getValue().metadata() != null && !entry.getValue().metadata().isEmpty()) + .map(Map.Entry::getKey) + .collect(Collectors.toSet()); + if (partitionsCarryingAMap.isEmpty()) { + return Collections.emptyMap(); + } + + try { + var endOffsets = module.consumer().endOffsets(partitionsCarryingAMap); + var highestOffsetsHeld = new HashMap(); + endOffsets.forEach((tp, endOffset) -> { + if (endOffset != null) { + // the end offset is the next offset to be WRITTEN, so the last one the partition holds is below it + highestOffsetsHeld.put(tp, endOffset - 1); + } + }); + return highestOffsetsHeld; + } catch (Exception e) { + // The reason goes in the warning; the stack trace goes to debug, because this fires once per rebalance + // whenever a consumer will not answer the question - including every MockConsumer that was never told + // where its partitions end - and a trace per rebalance is how a warning stops being read. + log.warn("Could not read the end offsets of {} while loading their committed offset maps ({}), so those " + + "maps cannot be checked against the offsets the partitions actually hold - decoding them " + + "as they are. An offset map claiming offsets that do not exist would be accepted this time.", + partitionsCarryingAMap, e.toString()); + log.debug("End offset lookup failed", e); + return Collections.emptyMap(); + } + } + /** * The epoch {@link PartitionState} is built with on assignment - and the one place * {@link bz.stub.parallelconsumer.state.PartitionStateManager#epochOfPartitionIfAssigned} is narrowed to a @@ -274,8 +343,11 @@ private long epochOfPartitionBeingAssigned(TopicPartition tp) { * @param offsetData the committed offset and its free-form metadata field * @throws OffsetDecodingError if the metadata is not valid base64 */ - private HighestOffsetAndIncompletes decodeOffsetMapForPartition(TopicPartition tp, OffsetAndMetadata offsetData) throws OffsetDecodingError { - return deserialiseIncompleteOffsetMapFromBase64(offsetData.offset(), offsetData.metadata(), errorPolicy, tp); + private HighestOffsetAndIncompletes decodeOffsetMapForPartition(TopicPartition tp, + OffsetAndMetadata offsetData, + long highestOffsetPartitionCanHold) throws OffsetDecodingError { + return deserialiseIncompleteOffsetMapFromBase64(offsetData.offset(), offsetData.metadata(), errorPolicy, tp, + highestOffsetPartitionCanHold); } /** @@ -333,6 +405,21 @@ public static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase String base64EncodedOffsetPayload, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) throws OffsetDecodingError { + return deserialiseIncompleteOffsetMapFromBase64(committedOffsetForPartition, base64EncodedOffsetPayload, + errorPolicy, tp, UNKNOWN_PARTITION_CEILING); + } + + /** + * @param highestOffsetPartitionCanHold the last offset the partition holds, against which an otherwise + * well-formed map is checked, or {@link #UNKNOWN_PARTITION_CEILING} when + * that could not be established + * @see #deserialiseIncompleteOffsetMapFromBase64(long, String, InvalidOffsetMetadataHandlingPolicy, TopicPartition) + */ + static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase64(long committedOffsetForPartition, + String base64EncodedOffsetPayload, + InvalidOffsetMetadataHandlingPolicy errorPolicy, + TopicPartition tp, + long highestOffsetPartitionCanHold) throws OffsetDecodingError { byte[] decodedBytes; try { decodedBytes = OffsetSimpleSerialisation.decodeBase64(base64EncodedOffsetPayload); @@ -349,11 +436,22 @@ public static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase EncodedOffsetPair.describeSource(tp, committedOffsetForPartition)), tp); } - return decodeCompressedOffsets(committedOffsetForPartition, decodedBytes, errorPolicy, tp); + return decodeCompressedOffsets(committedOffsetForPartition, decodedBytes, errorPolicy, tp, + highestOffsetPartitionCanHold); } PartitionState decodePartitionState(TopicPartition tp, OffsetAndMetadata offsetData) throws OffsetDecodingError { - HighestOffsetAndIncompletes incompletes = decodeOffsetMapForPartition(tp, offsetData); + return decodePartitionState(tp, offsetData, UNKNOWN_PARTITION_CEILING); + } + + /** + * @param highestOffsetPartitionCanHold the last offset this partition holds, or {@link #UNKNOWN_PARTITION_CEILING} + * when {@link #findHighestOffsetsHeld} could not establish it + */ + PartitionState decodePartitionState(TopicPartition tp, + OffsetAndMetadata offsetData, + long highestOffsetPartitionCanHold) throws OffsetDecodingError { + HighestOffsetAndIncompletes incompletes = decodeOffsetMapForPartition(tp, offsetData, highestOffsetPartitionCanHold); log.debug("Loaded incomplete offsets from offset payload {}", incompletes); long epoch = epochOfPartitionBeingAssigned(tp); return new PartitionState<>(epoch, module, tp, incompletes); @@ -462,6 +560,19 @@ static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffs byte[] decodedBytes, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) { + return decodeCompressedOffsets(nextExpectedOffset, decodedBytes, errorPolicy, tp, UNKNOWN_PARTITION_CEILING); + } + + /** + * @param highestOffsetPartitionCanHold the last offset the partition holds, or {@link #UNKNOWN_PARTITION_CEILING} + * when that could not be established + * @see #decodeCompressedOffsets(long, byte[], InvalidOffsetMetadataHandlingPolicy, TopicPartition) + */ + static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffset, + byte[] decodedBytes, + InvalidOffsetMetadataHandlingPolicy errorPolicy, + TopicPartition tp, + long highestOffsetPartitionCanHold) { // if no offset bitmap data if (decodedBytes.length == 0) { @@ -470,7 +581,8 @@ static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffs long highestSeenOffsetIsThen = nextExpectedOffset - 1; return HighestOffsetAndIncompletes.of(highestSeenOffsetIsThen); } else { - return EncodedOffsetPair.decodeToIncompletes(decodedBytes, nextExpectedOffset, errorPolicy, tp); + return EncodedOffsetPair.decodeToIncompletes(decodedBytes, nextExpectedOffset, errorPolicy, tp, + highestOffsetPartitionCanHold); } } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java index fa448a2e7..b401c46af 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java @@ -77,9 +77,42 @@ static String runLengthDecodeToString(final List in) { /** + * Decodes a run-length body into the incomplete offsets it names and the highest offset it claims to have seen. + * + *

The plausibility ceiling, and why it is the partition's end offset. Every other check in this method + * is one the buffer can settle by itself. A run length cannot be: a long run of completed offsets is precisely + * what run-length encoding is for, so nothing in the bytes distinguishes a legitimate one from an absurd + * one. Unchecked, a single four-byte entry of {@link Integer#MAX_VALUE} moves the highest-seen offset about two + * billion forward, and {@code PartitionState#isRecordPreviouslyCompleted} then treats every real record in that + * range as already succeeded - silent non-processing, not replay, from metadata anything sharing the consumer + * group can write. + * + *

What a legitimate long run looks like, so the guard cannot reject one. One record stuck at the + * committed offset while everything above it succeeds is an ordinary shape here, and it is not self-limiting: + * PC's back-pressure keys off the encoded payload size, and this payload stays three entries wide however + * far the partition runs ahead of the stuck record. So a run of hundreds of millions of completed offsets is real + * data, and any ceiling derived from a configured window - concurrency, the in-flight target, a round number - + * would eventually discard a true offset map and replay everything in it. That is why this method takes ground + * truth instead of a constant: the partition's log end offset is the one bound a legitimate map provably cannot + * cross, because PC only ever encodes offsets it has polled, and an offset the partition does not hold cannot + * have been polled. Kafka's end offset only ever grows, so a bound read now is still valid for a map written + * earlier. + * + *

The ceiling is checked before each run is applied rather than after the decode, so an incomplete + * run past the end of the partition is refused rather than walked - the same five-byte payload would otherwise + * ask for two billion boxed longs. + * + * @param baseOffset the committed offset the runs are relative to + * @param highestOffsetPartitionCanHold the last offset the partition actually holds (its log end offset minus + * one), or {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when the + * caller could not find out - in which case no run is refused on these + * grounds, because a guard with no ground truth must not invent one * @see #runLengthEncode */ - static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding encoding, final long baseOffset, final ByteBuffer in) + static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding encoding, + final long baseOffset, + final ByteBuffer in, + final long highestOffsetPartitionCanHold) throws CorruptOffsetMetadataException { in.rewind(); // asShortBuffer()/asIntBuffer() silently DROP a trailing partial element, so a body of the wrong width decodes @@ -159,6 +192,26 @@ static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding e throw new CorruptOffsetMetadataException(msg("negative run length ({}) at offset {}", runLength, currentOffset)); } + // The partition is the only thing that can prove a run absurd - see this method's javadoc. Expressed + // as the room left rather than as (currentOffset + runLength), which would overflow for a payload + // built to make it do so. Discarding the map here is logged by + // EncodedOffsetPair#handleUnreadableMetadata with the partition, the base offset and this reason; + // it is deliberately not counted, which is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md. + if (highestOffsetPartitionCanHold != OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING) { + // Arithmetic that cannot wrap, which is not fussiness: the unknown ceiling IS Long.MAX_VALUE, so + // a "currentOffset + runLength" or a "+ 1" on the room left overflows to a negative number and + // rejects EVERY payload, the encoder's own included - and a base offset of -1 (no commit yet) + // reaches this code, so the subtraction is not safe from the other side either. Hence: skip + // entirely when there is no ceiling, then measure the run as a DISTANCE from currentOffset. + long offsetsThePartitionStillHolds = highestOffsetPartitionCanHold - currentOffset; + if (runLength.longValue() > 0 && runLength.longValue() - 1 > offsetsThePartitionStillHolds) { + throw new CorruptOffsetMetadataException(msg( + "{} run of {} offset(s) at offset {} reaches past the end of the partition, which " + + "holds nothing above offset {} - no offset map this build wrote could name " + + "an offset that has never existed", + encoding.description(), runLength, currentOffset, highestOffsetPartitionCanHold)); + } + } if (currentRunLengthIsComplete) { log.trace("Ignoring {} completed offset(s) (offset:{})", runLength, currentOffset); currentOffset += runLength.longValue(); diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java index 0f316fae9..271c97144 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java @@ -46,6 +46,13 @@ class EncoderOutputSurvivesDecodeValidationTest { /** * Encodes {@code length} offsets from {@link #BASE_OFFSET} where only {@code incompleteIndexes} are incomplete, * then hands the bytes back through the production decode path exactly as a rebalance would. + *

+ * Every shape is decoded twice, once with no plausibility ceiling and once with the tightest honest one - + * a partition that ends exactly at the last offset encoded. That second pass is what pins the encoder side of + * {@link OffsetRunLength#runLengthDecodeToIncompletes}'s guard: the encoder only ever encodes offsets PC has + * polled, so its output always ends on an offset the partition holds, and the guard must therefore never see it + * as a claim past the end. Anything that made the encoder write one offset further than it saw would land here + * as a false rejection rather than as silence. */ private static void roundTrip(OffsetEncoding encoding, int length, SortedSet incompletes) throws Exception { long highestSucceeded = BASE_OFFSET + length - 1; @@ -62,16 +69,19 @@ private static void roundTrip(OffsetEncoding encoding, int length, SortedSet { - var decoded = EncodedOffsetPair.decodeToIncompletes(withMagic.array(), BASE_OFFSET, - InvalidOffsetMetadataHandlingPolicy.FAIL, TP); - assertThat(decoded.getIncompleteOffsets()) - .as("a payload this build wrote must decode back to the offsets it encoded") - .isEqualTo(incompletes); - }) - .as("FAIL must not reject %s output at length %s - a false rejection discards a real offset map " - + "and replays completed work", encoding, length) - .doesNotThrowAnyException(); + for (long ceiling : new long[]{OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING, highestSucceeded}) { + assertThatCode(() -> { + var decoded = EncodedOffsetPair.decodeToIncompletes(withMagic.array(), BASE_OFFSET, + InvalidOffsetMetadataHandlingPolicy.FAIL, TP, ceiling); + assertThat(decoded.getIncompleteOffsets()) + .as("a payload this build wrote must decode back to the offsets it encoded") + .isEqualTo(incompletes); + }) + .as("FAIL must not reject %s output at length %s against a partition ending at %s - a false " + + "rejection discards a real offset map and replays completed work", + encoding, length, ceiling) + .doesNotThrowAnyException(); + } } /** diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java new file mode 100644 index 000000000..4e8a06dc1 --- /dev/null +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java @@ -0,0 +1,298 @@ +package bz.stub.parallelconsumer.offsets; + +/*- + * Copyright (C) 2026 Antony Stubbs and contributors + */ + +import bz.stub.parallelconsumer.ParallelConsumerOptions; +import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; +import bz.stub.parallelconsumer.internal.PCModuleTestEnv; +import bz.stub.parallelconsumer.state.PartitionState; +import bz.stub.parallelconsumer.state.WorkManager; +import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.ConsumerRecord; +import org.apache.kafka.clients.consumer.MockConsumer; +import org.apache.kafka.clients.consumer.OffsetAndMetadata; +import org.apache.kafka.clients.consumer.OffsetResetStrategy; +import org.apache.kafka.common.TopicPartition; +import org.junit.jupiter.api.Test; +import pl.tlinkowski.unij.api.UniLists; +import pl.tlinkowski.unij.api.UniMaps; + +import java.nio.ByteBuffer; +import java.util.Base64; +import java.util.function.UnaryOperator; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * An offset map whose decoded range reaches past the end of the partition claims offsets that do not exist, and must + * be treated as unreadable rather than decoded. + *

+ * This is the fourth outcome {@link EncodedOffsetPair#getDecodedIncompletes(long, + * InvalidOffsetMetadataHandlingPolicy, TopicPartition)} used to document as uncoverable - "a payload that decodes + * cleanly into a wrong-but-plausible map". Nothing in the bytes proves a long run wrong, because a long run + * of completed offsets is exactly what run-length encoding is for; what proves it wrong is the partition, which + * cannot hold the offsets the run claims. + *

+ * Left unchecked, one four-byte entry of {@link Integer#MAX_VALUE} moves the highest-seen offset about two billion + * forward, and {@link PartitionState#isRecordPreviouslyCompleted} then reads every real record in that range as + * already succeeded - silent non-processing, not replay, from metadata anything sharing the consumer group can write. + * + * @author Antony Stubbs + * @see OffsetRunLength#runLengthDecodeToIncompletes + */ +@Slf4j +class ImplausibleOffsetMapOnAssignmentTest { + + static final TopicPartition TP = new TopicPartition("myTopic", 0); + + static final long COMMITTED_OFFSET = 100L; + + /** + * The partition holds offsets {@code 0..199}, so 199 is the highest offset any honest offset map can name. + */ + static final long PARTITION_END_OFFSET = 200L; + + /** + * Comfortably inside the partition, and inside the range the corrupt payload claims - so it is a record that + * really exists and would really be skipped. + */ + static final long REAL_RECORD_IN_THE_CLAIMED_RANGE = 150L; + + MockConsumer mockConsumer; + + /** + * Base64 of a {@link OffsetEncoding#RunLengthV2} payload with the given runs, the first of which is a run of + * incomplete offsets (the decoder starts there, because the committed offset is the lowest incomplete + * one). + */ + private static String runLengthV2Metadata(int... runs) { + ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + runs.length * Integer.BYTES); + body.put(OffsetEncoding.RunLengthV2.magicByte); + for (int run : runs) { + body.putInt(run); + } + return Base64.getEncoder().encodeToString(body.array()); + } + + /** + * Base64 of a {@link OffsetEncoding#BitSetV2} payload declaring {@code declaredBits} offsets, every one of them + * completed, with the body that length actually requires - so it passes astubbs#207's bytes-must-back-the-length + * check and reaches the ceiling on its merits. + */ + private static String bitSetV2Metadata(int declaredBits) { + int bodyBytes = (declaredBits + 7) / 8; + ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + Integer.BYTES + bodyBytes); + body.put(OffsetEncoding.BitSetV2.magicByte); + body.putInt(declaredBits); + for (int i = 0; i < bodyBytes; i++) { + body.put((byte) 0xFF); // every offset in the range marked completed + } + return Base64.getEncoder().encodeToString(body.array()); + } + + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata) { + return moduleWithCommittedMetadata(metadata, PARTITION_END_OFFSET, builder -> builder); + } + + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, InvalidOffsetMetadataHandlingPolicy policy) { + return moduleWithCommittedMetadata(metadata, PARTITION_END_OFFSET, + builder -> builder.invalidOffsetMetadataPolicy(policy)); + } + + /** + * Builds a module whose consumer has {@code metadata} committed against {@link #TP} and reports {@code endOffset} + * as that partition's log end offset. + * + * @param endOffset the log end offset to report, or a negative number to leave it unprimed - which makes + * {@link MockConsumer#endOffsets} throw, standing in for a broker that will not answer + */ + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, + long endOffset, + UnaryOperator> configure) { + mockConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(TP)); + mockConsumer.commitSync(UniMaps.of(TP, new OffsetAndMetadata(COMMITTED_OFFSET, metadata))); + if (endOffset >= 0) { + mockConsumer.updateEndOffsets(UniMaps.of(TP, endOffset)); + } + + var options = configure.apply(ParallelConsumerOptions.builder() + .consumer(mockConsumer)).build(); + return new PCModuleTestEnv(options); + } + + private static ConsumerRecord recordAt(long offset) { + return new ConsumerRecord<>(TP.topic(), TP.partition(), offset, "key", "value"); + } + + /** + * The defect, at the frame where it does its damage: the offset map is loaded on assignment, and every record in + * the range it invented is then skipped without ever reaching the user function. + */ + @Test + void aRunPastTheEndOfThePartitionDoesNotMarkRealRecordsAsCompleted() { + // one incomplete offset (the committed one), then a completed run of about two billion + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1)); + WorkManager wm = module.workManager(); + + wm.onPartitionsAssigned(UniLists.of(TP)); + + PartitionState state = wm.getPm().getPartitionState(TP); + assertThat(state.isRecordPreviouslyCompleted(recordAt(REAL_RECORD_IN_THE_CLAIMED_RANGE))) + .as("a record the partition really holds must not be treated as completed because a payload claimed " + + "a run reaching past the end of the partition") + .isFalse(); + assertThat(state.getOffsetHighestSucceeded()) + .as("the metadata is unreadable, so it is discarded and we resume from the committed offset") + .isEqualTo(COMMITTED_OFFSET - 1); + } + + /** + * The other half of the policy contract: {@code FAIL} exists so a deployment can refuse to carry on with an offset + * map this build cannot believe, rather than silently discarding it. + */ + @Test + void failPolicyRejectsARunPastTheEndOfThePartition() { + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = module.workManager(); + + assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("FAIL must not silently accept an offset map claiming offsets the partition cannot hold") + .isInstanceOf(CorruptOffsetMetadataException.class) + .hasMessageContaining(TP.toString()); + } + + /** + * A run of incomplete offsets is the same lie with a different cost: the decoder used to walk it one + * offset at a time into a {@link java.util.TreeSet}, so a five-byte payload asked for two billion boxed longs. The + * guard has to fire before the loop, not after it. + */ + @Test + void anIncompleteRunPastTheEndOfThePartitionIsRejectedBeforeItIsWalked() { + var module = moduleWithCommittedMetadata(runLengthV2Metadata(Integer.MAX_VALUE), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = module.workManager(); + + assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("an incomplete run past the end of the partition must be rejected rather than materialised") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * The guard must not reject a real offset map, and a long run of completed offsets is a real shape: one record + * stuck at the committed offset while every offset above it succeeds is exactly what run-length encoding is for, + * and PC's back-pressure does not stop it - the payload stays three entries wide however far the partition runs + * ahead. + *

+ * Pinned at the boundary: the run ends on the last offset the partition holds, which is the largest map that is + * still honest. + */ + @Test + void aLongRunThatStaysInsideThePartitionIsDecodedNormally() { + long partitionEndOffset = 1_000_000_000L; + long highestOffsetHeld = partitionEndOffset - 1; + int completedRun = (int) (highestOffsetHeld - COMMITTED_OFFSET); + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, completedRun), + partitionEndOffset, + builder -> builder); + WorkManager wm = module.workManager(); + + assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("a run ending on the last offset the partition holds is honest, and must decode") + .doesNotThrowAnyException(); + + PartitionState state = wm.getPm().getPartitionState(TP); + assertThat(state.getOffsetHighestSucceeded()) + .as("the whole run was accepted, right up to the last offset the partition holds") + .isEqualTo(highestOffsetHeld); + assertThat(state.isRecordPreviouslyCompleted(recordAt(COMMITTED_OFFSET))) + .as("the one incomplete offset in the map is still incomplete") + .isFalse(); + assertThat(state.isRecordPreviouslyCompleted(recordAt(COMMITTED_OFFSET + 1))) + .as("everything above it really did succeed - the long run is data, not corruption") + .isTrue(); + } + + /** + * One offset further than the partition can hold, and nothing else changed - the control arm for the test above. + */ + @Test + void aRunOneOffsetPastTheLastOffsetHeldIsRejected() { + long partitionEndOffset = 1_000_000_000L; + int completedRun = (int) (partitionEndOffset - COMMITTED_OFFSET); // one past the last offset held + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, completedRun), + partitionEndOffset, + builder -> builder.invalidOffsetMetadataPolicy(InvalidOffsetMetadataHandlingPolicy.FAIL)); + WorkManager wm = module.workManager(); + + assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("the first offset the partition does not hold is the first one a map may not claim") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * When the broker will not say where the partition ends, the guard has no ground truth and must not invent one: + * it stands down and the payload decodes exactly as it did before this check existed. + *

+ * Pinned because it is the behaviour every deployment gets when a leader is unavailable during a rebalance, and + * because failing closed here would discard honest offset maps on a broker hiccup - a far more likely + * event than the corrupt payload this guard is for. + */ + @Test + void anUnknownEndOffsetLeavesTheDecodeAloneRatherThanRejectingIt() { + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, 1_000), + -1, // end offsets left unprimed: MockConsumer refuses to answer, as a broker that times out would + builder -> builder.invalidOffsetMetadataPolicy(InvalidOffsetMetadataHandlingPolicy.FAIL)); + WorkManager wm = module.workManager(); + + assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("a guard with no ground truth must stand down, not reject") + .doesNotThrowAnyException(); + + assertThat(wm.getPm().getPartitionState(TP).getOffsetHighestSucceeded()) + .as("the payload decoded as it always did") + .isEqualTo(COMMITTED_OFFSET + 1_000); + } + + /** + * The same defect one encoding over, and the reason the ceiling is applied to both decoders rather than only to + * the one the bug was reported against. A bitset is less exposed - astubbs#207 made its declared length + * prove itself against bytes that are present, so a full metadata field buys tens of thousands of skipped records + * rather than two billion - but bounded is not true. + */ + @Test + void aBitSetReachingPastTheEndOfThePartitionIsRejected() { + var module = moduleWithCommittedMetadata(bitSetV2Metadata(300), // 100 + 300 - 1, well past offset 199 + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = module.workManager(); + + assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("a bitset claiming offsets the partition does not hold is as unreadable as a run length doing it") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * The bitset control arm, at the boundary: a map covering every offset from the committed one to the last the + * partition holds is the widest honest bitset there is, and must decode. + */ + @Test + void aBitSetEndingOnTheLastOffsetHeldIsDecodedNormally() { + int bitsToTheEndOfThePartition = (int) (PARTITION_END_OFFSET - COMMITTED_OFFSET); + var module = moduleWithCommittedMetadata(bitSetV2Metadata(bitsToTheEndOfThePartition), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = module.workManager(); + + assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) + .as("a bitset ending exactly on the last offset the partition holds is honest") + .doesNotThrowAnyException(); + + assertThat(wm.getPm().getPartitionState(TP).getOffsetHighestSucceeded()) + .as("the whole declared range was accepted") + .isEqualTo(PARTITION_END_OFFSET - 1); + } +} From 0402dbdae849c31e438ca2848512157bc32abef8 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 13:36:50 +1200 Subject: [PATCH 2/8] docs(features): the metadata policy now governs a map claiming offsets 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 --- docs/features/invalid-offset-metadata-policy.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/features/invalid-offset-metadata-policy.yaml b/docs/features/invalid-offset-metadata-policy.yaml index 35126c246..973796b7e 100644 --- a/docs/features/invalid-offset-metadata-policy.yaml +++ b/docs/features/invalid-offset-metadata-policy.yaml @@ -28,7 +28,9 @@ opt_in: isolated_from_stable_modules: false boundaries: - The policy governs every payload this build cannot read - a magic byte no encoding claims, an encoding with no decoder here, metadata recognized as Kafka Streams', and a body that is not something any encoder here could have produced. - - It does not govern a payload that decodes successfully into a wrong-but-plausible offset map; nothing in such a payload proves it wrong. + - A decoded map claiming offsets the partition does not hold is governed too: the end offsets of the partitions carrying metadata are read once per assignment, and a map reaching past the last offset a partition holds is treated as unreadable. + - When those end offsets cannot be read - an unavailable leader, a timeout - the check stands down for that assignment and logs it, rather than discarding maps it cannot judge. + - It still does not govern a payload that decodes into a map which is merely wrong about offsets that do exist; nothing in such a payload proves it wrong. - Under IGNORE the offset map is discarded, not repaired, so records completed but not yet committed before that point are replayed. references: - label: Offset-metadata policy From cc0d887a61641b3d3885106c38225798dc6ac94b Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 14:05:52 +1200 Subject: [PATCH 3/8] fix(review): clear the three static-analysis findings this PR put on 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 --- .../parallelconsumer/offsets/OffsetMapCodecManager.java | 8 ++++---- .../offsets/ImplausibleOffsetMapOnAssignmentTest.java | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java index aa60d476f..287a6fc6c 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java @@ -265,17 +265,17 @@ public Map> loadPartitionStateForAssignment * @return the last offset each partition holds, keyed by partition and absent where it could not be established */ private Map findHighestOffsetsHeld(Map partitionLastCommittedOffsets) { - var partitionsCarryingAMap = partitionLastCommittedOffsets.entrySet().stream() + var partitionsCarryingMetadata = partitionLastCommittedOffsets.entrySet().stream() .filter(entry -> entry.getValue() != null) .filter(entry -> entry.getValue().metadata() != null && !entry.getValue().metadata().isEmpty()) .map(Map.Entry::getKey) .collect(Collectors.toSet()); - if (partitionsCarryingAMap.isEmpty()) { + if (partitionsCarryingMetadata.isEmpty()) { return Collections.emptyMap(); } try { - var endOffsets = module.consumer().endOffsets(partitionsCarryingAMap); + var endOffsets = module.consumer().endOffsets(partitionsCarryingMetadata); var highestOffsetsHeld = new HashMap(); endOffsets.forEach((tp, endOffset) -> { if (endOffset != null) { @@ -291,7 +291,7 @@ private Map findHighestOffsetsHeld(Map builder); + return moduleWithCommittedMetadata(metadata, PARTITION_END_OFFSET, UnaryOperator.identity()); } private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, InvalidOffsetMetadataHandlingPolicy policy) { @@ -199,7 +199,7 @@ void aLongRunThatStaysInsideThePartitionIsDecodedNormally() { int completedRun = (int) (highestOffsetHeld - COMMITTED_OFFSET); var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, completedRun), partitionEndOffset, - builder -> builder); + UnaryOperator.identity()); WorkManager wm = module.workManager(); assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) From 9235306e73bb965417b4993dc8bf606e0f45c8b7 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 15:45:31 +1200 Subject: [PATCH 4/8] fix(offsets): the plausibility check moves off the wire and onto the first batch Owner's decision on astubbs/parallel-consumer#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. astubbs#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 --- ...-allocates-before-anything-can-check-it.md | 28 ++ ...truth-not-a-plausible-number-2026-09-08.md | 78 ++-- .../internal/BrokerPollSystem.java | 7 +- .../internal/ConsumerManager.java | 36 ++ .../internal/EpochAndRecordsMap.java | 35 +- .../offsets/EncodedOffsetPair.java | 72 ++-- .../offsets/OffsetBitSet.java | 25 +- .../offsets/OffsetMapCodecManager.java | 129 +------ .../offsets/OffsetRunLength.java | 55 +-- .../state/PartitionState.java | 151 ++++++++ .../ConsumerManagerLogEndOffsetTest.java | 105 ++++++ ...derOutputSurvivesDecodeValidationTest.java | 42 ++- .../ImplausibleOffsetMapOnAssignmentTest.java | 298 --------------- .../ImplausibleOffsetMapOnFirstBatchTest.java | 341 ++++++++++++++++++ 14 files changed, 812 insertions(+), 590 deletions(-) create mode 100644 docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md create mode 100644 parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java delete mode 100644 parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java create mode 100644 parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java diff --git a/docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md b/docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md new file mode 100644 index 000000000..f91844068 --- /dev/null +++ b/docs/inflight/bug-a-huge-incomplete-run-allocates-before-anything-can-check-it.md @@ -0,0 +1,28 @@ +# A run-length payload can ask for two billion boxed longs before any check runs + + + + +`OffsetRunLength.runLengthDecodeToIncompletes` walks an *incomplete* run one offset at a time into a +`TreeSet`, so a five-byte payload whose first run is `Integer.MAX_VALUE` asks for two billion boxed +longs and exhausts the heap during the rebalance callback. Same family as the `BitSetV2` case +astubbs#207 closed by requiring the declared bit length to be backed by bytes that are present - but +run-length is compact by design, so nothing in the payload bounds what one entry may demand. + +**The plausibility check does not cover this one, by construction.** That check compares the +*decoded* claim against the partition's log end offset, and the log end offset is not knowable until +the first fetch delivers it - by which time this decode has already happened. The claim-side defect +(a completed run marking a range as done, so PC silently skips it) is what it closes. + +**A decode-side bound would need a number nobody can derive**, which is exactly what +`bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md` argues against for the claim +case. Two shapes worth considering when this is picked up, neither costed yet: + +- Cap the *materialisation* rather than the claim: decode incompletes lazily, or refuse to build a + set larger than the partition could deliver in flight, which is a memory-safety limit rather than + a plausibility one and can be stated as such. +- Carry the claim without expanding it, so a range only becomes a set when records arrive to justify + it. + +Reachable by anything that can write offset metadata for the consumer group. It fails loudly (an +`OutOfMemoryError`, not a silent skip), which is why it is not a v6 gate on its own. diff --git a/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md b/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md index 4a8ef5e00..42c3cef21 100644 --- a/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md +++ b/docs/solutions/architecture-patterns/bound-a-guard-with-ground-truth-not-a-plausible-number-2026-09-08.md @@ -13,15 +13,19 @@ applies_when: - The obvious bound is a constant somebody has to choose, and choosing it wrong rejects real data - A guard has been parked as "a product call" because nobody could justify the number - Deciding whether a check may make a network call, and what it should do when that call fails + - A validator wants a fact the process does not have yet, but will have shortly, for free related_components: + - PartitionState + - ConsumerManager + - EpochAndRecordsMap - OffsetRunLength - - OffsetBitSet - - OffsetMapCodecManager - EncodedOffsetPair tags: - guard-predicate - ground-truth - fail-open + - lazy-validation + - rebalance-path - offset-encoding - input-validation - wire-format @@ -67,32 +71,54 @@ number.** Three candidate bounds were considered here, and only one is not a gue There is no false-positive case to trade off, which is what makes it a correctness bound rather than a tuning parameter. -**The cost of ground truth is usually a call somebody was avoiding - price it rather than assume -it.** The end offset is not free: it is a `ListOffsets` round trip, and the natural place to make it -is inside the rebalance callback, which in this codebase is the most fragile path there is. What -made it affordable was scoping: one batched request for the whole assignment, skipped entirely when -no assigned partition carries metadata, on a rebalance that has already paid for a `JoinGroup`, a -`SyncGroup` and an `OffsetFetch`. A per-partition or per-decode lookup would not have been. - -**A guard with no ground truth must stand down, not guess.** When the lookup fails - an unavailable -leader, a timeout, a consumer implementation that will not answer - the decoders are handed -"unknown" and refuse nothing. Failing *closed* would mean a broker hiccup discards honest offset -maps, which is a far more likely event than the corrupt payload the guard exists for; the check -would then cause more data churn than it prevents. The failure is logged with the partitions it -covers, so the stand-down is visible rather than assumed. - -**Check the producing side, and pin it at the boundary.** A decode-side bound is only safe if the +**Having found the authority, ask what it costs to consult - and whether the answer is already in +the building.** The first implementation of this fix bought the end offset with a `ListOffsets` +request at assignment, batched across the partitions carrying metadata. It worked, and it was +rejected on review for two reasons worth keeping: + +- **It put a blocking broker round trip inside the rebalance callback**, which in this codebase is + the most fragile path there is - the one whose stalls and deadlocks have consumed more + investigation than anything else. A check against corrupt metadata is not worth a new way for a + rebalance to hang. +- **It failed open exactly when the broker was unhealthy.** An unreachable leader means no end + offset, which means the guard stands down - so the check was reliably absent in precisely the + conditions that make rebalances storm and offset maps get rewritten. + +**The same fact was already arriving for free, slightly later.** Every fetch response carries the +partition's high watermark, and the consumer exposes it as `Consumer.currentLag` (KIP-695) - no +request of its own, computed from the last fetch. `position + currentLag` reconstructs the watermark +exactly when both are read in one breath on the thread that owns the consumer. So the check moved +off the wire and onto the first batch: the decode accepts the map and *retains what it claims*, and +the claim is settled before any record is consulted against it. Nothing is lost by waiting, because +nothing consults the map until a record arrives to be consulted about. + +**"Later, for free" beats "now, at a cost" whenever the deadline is not real.** The instinct is to +validate at the point of parsing, and here that instinct is what forced the wire call: the data +needed to judge the payload simply is not in the process yet at decode time. Ask when the judgement +is actually *needed* - the first consult, not the parse - and the cost can disappear. + +**A guard with no ground truth must stand down, not guess - and standing down is deferral, not +acceptance.** `currentLag` is empty until a fetch has happened, and the position read is asked for +with a zero timeout so it raises rather than sends a request. Every such outcome means *not +established*, and the check simply looks again at the next batch. The batch's own records serve as a +floor in the meantime - 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 use with no watermark at all. Failing closed +would discard honest offset maps on a broker hiccup, a far more likely event than the corrupt +payload the guard exists for. + +**Check the producing side, and pin it at the boundary.** A bound like this is only safe if the encoder can never write what it rejects. Here it cannot, because the encoder's range top is an offset PC actually saw - but the argument is worth a test rather than a paragraph: every encoder -round-trip case is now decoded twice, once unbounded and once against a partition ending exactly on -the last offset encoded. Anything that made the encoder run one offset past what it saw surfaces as -a false rejection instead of as silence. +round-trip case is measured against the rule with the tightest honest bound there is, a partition +ending exactly on the last offset encoded. Anything that made the encoder run one offset past what +it saw surfaces as a false rejection instead of as silence. -**Take the whole defect class while the parameter is in your hand.** The bitset decoder has the same -shape - a structurally valid length field the format cannot prove absurd - and is merely *less* -exposed, because its declared bit count has to be backed by bytes that are present, so a full 4KB -metadata field buys tens of thousands of skipped records rather than two billion. "Bounded" is not -"true", and the same ceiling now applies there. +**A check moved to where the meaning lives often covers more than the one it replaced.** The +decode-side version had to be threaded into each decoder and applied per encoding; the lazy one +reads the *decoded claim*, so run-length and bitset are covered by the same line, and any encoding +added later is covered without being told. The bitset case is worth naming because it is easy to +dismiss: its declared bit count must be backed by bytes that are present, so a full 4KB metadata +field buys tens of thousands of skipped records rather than two billion. Bounded is not true. ## Why This Matters @@ -113,6 +139,8 @@ direction, and the only thing traded away is a round trip on a path that already - Reviewing a parked bug whose blocker is "we would have to pick a threshold". - Deciding what a guard does when the fact it needs is unavailable - name the direction it fails in and say why that direction is the cheaper wrong answer. +- Being tempted to buy a fact with a network call on a latency- or liveness-critical path: ask what + already arrives carrying it, and when the answer is first *needed* rather than first available. ## Related diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/BrokerPollSystem.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/BrokerPollSystem.java index 1a96e3bd5..0c3e188a7 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/BrokerPollSystem.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/BrokerPollSystem.java @@ -280,8 +280,11 @@ private EpochAndRecordsMap pollBrokerForRecords() { log.debug("Poll completed"); - // build records map - return new EpochAndRecordsMap<>(poll, wm.getPm()); + // Build the records map, and take the fetch's own high watermark with it while we are still on the thread + // that owns the consumer - see ConsumerManager#logEndOffsetIfKnownWithoutBlocking. The control thread cannot + // ask this question later: the consumer is confined here, and by then the answer would belong to a different + // fetch. + return new EpochAndRecordsMap<>(poll, wm.getPm(), consumerManager::logEndOffsetIfKnownWithoutBlocking); } private void checkStateForPausingSubscriptions() { diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/ConsumerManager.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/ConsumerManager.java index d8335acf3..c84dd630a 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/ConsumerManager.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/ConsumerManager.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.OptionalLong; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; @@ -494,4 +495,39 @@ public void resume(final Set pausedTopics) { public void onCommitRequested() { this.commitRequested = true; } + + /** + * Where the partition ends (exclusive), if the consumer can answer out of what the last fetch already told + * it - the high watermark that came back with the records, at no cost and with no request of its own. + *

+ * {@link Consumer#currentLag(TopicPartition)} (KIP-695, kafka-clients 3.0+) is the high watermark minus the + * current position, computed from the last fetch response, so {@code position + lag} reconstructs the watermark + * exactly - both read here in one breath, on the thread that owns the consumer, so they cannot describe + * different moments. + *

+ * Nothing here may block or throw. The position is asked for with a zero timeout, so a partition with no + * position yet raises {@link TimeoutException} instead of sending an {@code OffsetFetch}; the lag is empty until + * a fetch has happened. Every such outcome is the same answer - not established - and the caller's + * contract (see {@code PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition}) is to try again on the + * next batch rather than to assume anything. An offset-map check must never be able to slow a poll down, let + * alone fail one. + * + * @return the log end offset, exclusive, or empty when it is not knowable without asking the broker + */ + public OptionalLong logEndOffsetIfKnownWithoutBlocking(TopicPartition partition) { + try { + OptionalLong lag = consumer.currentLag(partition); + if (!lag.isPresent()) { + return OptionalLong.empty(); + } + long position = consumer.position(partition, Duration.ZERO); + return OptionalLong.of(position + lag.getAsLong()); + } catch (Exception e) { + // Includes TimeoutException (no position without a request), IllegalStateException (the partition went + // away under a rebalance) and any consumer implementation that does not answer this question at all. + log.debug("Consumer cannot say where {} ends without blocking, so nothing is checked against it this " + + "time round", partition, e); + return OptionalLong.empty(); + } + } } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMap.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMap.java index eab327dfd..a052fe5fd 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMap.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/internal/EpochAndRecordsMap.java @@ -14,6 +14,7 @@ import org.apache.kafka.common.TopicPartition; import java.util.*; +import java.util.function.Function; /** * For tagging polled records with our epoch @@ -26,7 +27,29 @@ public class EpochAndRecordsMap { Map recordMap = new HashMap<>(); + /** + * A batch that carries no log end offset - every partition's is {@link OptionalLong#empty()}. + *

+ * Correct for any caller that is not the poll loop, because the log end offset is only knowable to the thread + * that owns the consumer, and only from the fetch that produced these very records. A reader of it must treat + * absence as "not established yet" and try again on the next batch, never as a bound. + */ public EpochAndRecordsMap(ConsumerRecords poll, PartitionStateManager pm) { + this(poll, pm, partition -> OptionalLong.empty()); + } + + /** + * @param logEndOffsetAtPoll where each partition ended according to the fetch that returned these records, + * or {@link OptionalLong#empty()} when the consumer could not say without blocking. + * Read on the poll thread, at the poll, because that is the only thread that may touch + * the consumer and the only moment the answer belongs to this batch - + * {@code ConsumerManager#logEndOffsetIfKnownWithoutBlocking} is what supplies it, and + * {@code PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition} is what it is + * for + */ + public EpochAndRecordsMap(ConsumerRecords poll, + PartitionStateManager pm, + Function logEndOffsetAtPoll) { poll.partitions().forEach(partition -> { var records = poll.records(partition); Optional epochOfPartition = pm.epochOfPartitionIfAssigned(partition); @@ -40,7 +63,8 @@ public EpochAndRecordsMap(ConsumerRecords poll, PartitionStateManager> records; + + /** + * Where the partition ended when this batch was fetched (exclusive), if the consumer could say so without a + * request of its own - the high watermark the fetch response already carried. + *

+ * Empty is not "zero" and not "unbounded": it means nobody has established it yet, and the only correct + * response is to look again at the next batch. + */ + @NonNull OptionalLong logEndOffsetAtPoll; } } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java index c7b2b955b..e597a7d02 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/EncodedOffsetPair.java @@ -129,22 +129,6 @@ static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, long baseOffset, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) { - return decodeToIncompletes(input, baseOffset, errorPolicy, tp, OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING); - } - - /** - * @param highestOffsetPartitionCanHold the last offset the partition actually holds, which is the only thing that - * can prove an otherwise well-formed offset map absurd - see - * {@link OffsetRunLength#runLengthDecodeToIncompletes}, which owns that - * reasoning. {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when - * the caller could not find out - * @see #decodeToIncompletes(byte[], long, InvalidOffsetMetadataHandlingPolicy, TopicPartition) - */ - static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, - long baseOffset, - InvalidOffsetMetadataHandlingPolicy errorPolicy, - TopicPartition tp, - long highestOffsetPartitionCanHold) { if (input.length == 0) { // Not reachable from production today: decodeCompressedOffsets branches on an empty payload before // calling here, because "no metadata committed" is a legitimate state rather than a corrupt one. That @@ -168,7 +152,7 @@ static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, tp); } return new EncodedOffsetPair(encoding.get(), wrap.slice()) - .getDecodedIncompletes(baseOffset, errorPolicy, tp, highestOffsetPartitionCanHold); + .getDecodedIncompletes(baseOffset, errorPolicy, tp); } /** @@ -189,8 +173,11 @@ static HighestOffsetAndIncompletes decodeToIncompletes(byte[] input, * @param toThrow the typed exception for the strict policy, built lazily so its (longer) advice text costs * nothing on the IGNORE path */ + // public, not package-private: the plausibility check in PartitionState is the one caller outside this package, + // and it must apply the SAME policy through the SAME handler rather than grow a second answer to "this build + // cannot believe this payload" - see PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition. @SneakyThrows - static HighestOffsetAndIncompletes handleUnreadableMetadata(long baseOffset, + public static HighestOffsetAndIncompletes handleUnreadableMetadata(long baseOffset, InvalidOffsetMetadataHandlingPolicy errorPolicy, String problem, Supplier toThrow, @@ -222,7 +209,7 @@ private static String problemOf(Exception e) { * * @param tp may be null when the caller did not know the partition */ - static String describeSource(TopicPartition tp, long baseOffset) { + public static String describeSource(TopicPartition tp, long baseOffset) { return msg("partition: {}, base offset: {}", tp, baseOffset); } @@ -277,11 +264,12 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, Invali * {@link CorruptOffsetMetadataException}, which is the case that used to return a fabricated offset map * instead of failing. * - * A fourth outcome - a payload that decodes cleanly into a wrong-but-plausible map - is covered only as far as - * something outside the payload can prove it wrong, because nothing in such a payload does. Today that - * is the partition itself: a map naming offsets the partition does not hold is refused, given a caller that - * passes {@code highestOffsetPartitionCanHold}. A map that is merely wrong about offsets which do exist remains - * indistinguishable from a real one. + * A fourth outcome - a payload that decodes cleanly into a wrong-but-plausible map - is not covered here, + * and cannot be: nothing in such a payload proves it wrong. Something outside it can, and one thing does: + * {@code PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition} measures the decoded map against the + * partition itself at the first batch, and refuses one claiming offsets the partition does not hold - through + * {@link #handleUnreadableMetadata}, so it is the same user-visible event as the three above. A map that is + * merely wrong about offsets which do exist stays indistinguishable from a real one. * * @param baseOffset the committed offset the payload is relative to, and what {@code IGNORE} falls back to * @param errorPolicy what to do when this build cannot read the payload @@ -289,24 +277,10 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, Invali * the caller does not know it * @return the highest offset seen, and the incomplete offsets below it */ - public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, - InvalidOffsetMetadataHandlingPolicy errorPolicy, - TopicPartition tp) { - return getDecodedIncompletes(baseOffset, errorPolicy, tp, OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING); - } - - /** - * @param highestOffsetPartitionCanHold the last offset the partition actually holds, or - * {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when unknown. It - * settles the fourth outcome the overload below calls uncoverable: a payload - * that decodes cleanly into a map no partition could produce - * @see #getDecodedIncompletes(long, InvalidOffsetMetadataHandlingPolicy, TopicPartition) - */ @SneakyThrows public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, InvalidOffsetMetadataHandlingPolicy errorPolicy, - TopicPartition tp, - long highestOffsetPartitionCanHold) { + TopicPartition tp) { switch (encoding) { case KafkaStreams: case KafkaStreamsV2: @@ -338,7 +312,7 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, // ZstdIOException from a body that is not a zstd frame, neither of which is an OffsetDecodingError, so // loadPartitionStateForAssignment's recovery never saw them and they escaped onPartitionsAssigned. try { - return decodeBody(baseOffset, highestOffsetPartitionCanHold); + return decodeBody(baseOffset); } catch (CorruptOffsetMetadataException | BufferUnderflowException | IOException e) { return handleUnreadableMetadata(baseOffset, errorPolicy, @@ -360,16 +334,16 @@ public HighestOffsetAndIncompletes getDecodedIncompletes(long baseOffset, * every way this can fail is caught in one place and routed through the user's policy, rather than each decoder * having to know about it. */ - private HighestOffsetAndIncompletes decodeBody(long baseOffset, long ceiling) throws CorruptOffsetMetadataException, IOException { + private HighestOffsetAndIncompletes decodeBody(long baseOffset) throws CorruptOffsetMetadataException, IOException { return switch (encoding) { - case BitSet -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data, ceiling); - case BitSetCompressed -> deserialiseBitSetWrapToIncompletes(BitSet, baseOffset, decompressZstd(data), ceiling); - case RunLength -> runLengthDecodeToIncompletes(encoding, baseOffset, data, ceiling); - case RunLengthCompressed -> runLengthDecodeToIncompletes(RunLength, baseOffset, decompressZstd(data), ceiling); - case BitSetV2 -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data, ceiling); - case BitSetV2Compressed -> deserialiseBitSetWrapToIncompletes(BitSetV2, baseOffset, decompressZstd(data), ceiling); - case RunLengthV2 -> runLengthDecodeToIncompletes(encoding, baseOffset, data, ceiling); - case RunLengthV2Compressed -> runLengthDecodeToIncompletes(RunLengthV2, baseOffset, decompressZstd(data), ceiling); + case BitSet -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data); + case BitSetCompressed -> deserialiseBitSetWrapToIncompletes(BitSet, baseOffset, decompressZstd(data)); + case RunLength -> runLengthDecodeToIncompletes(encoding, baseOffset, data); + case RunLengthCompressed -> runLengthDecodeToIncompletes(RunLength, baseOffset, decompressZstd(data)); + case BitSetV2 -> deserialiseBitSetWrapToIncompletes(encoding, baseOffset, data); + case BitSetV2Compressed -> deserialiseBitSetWrapToIncompletes(BitSetV2, baseOffset, decompressZstd(data)); + case RunLengthV2 -> runLengthDecodeToIncompletes(encoding, baseOffset, data); + case RunLengthV2Compressed -> runLengthDecodeToIncompletes(RunLengthV2, baseOffset, decompressZstd(data)); default -> throw new PCInternalRuntimeException( msg("no decoder for {}, and it was not routed to the policy handler", encoding)); }; diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java index b845007da..2341f5c5a 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetBitSet.java @@ -56,20 +56,7 @@ static String deserialiseBitSet(int originalBitsetSize, ByteBuffer s) { return result.toString(); } - /** - * @param highestOffsetPartitionCanHold the last offset the partition actually holds, or - * {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when the caller - * could not find out. Same ground truth, and same reasoning, as - * {@link OffsetRunLength#runLengthDecodeToIncompletes}, which owns the - * explanation. A bitset cannot claim as far as a run length can - the - * declared bit count has to be backed by bytes that are present, so the - * claim is capped by the payload's own size - but "capped" is not "true", - * and a full metadata field still buys tens of thousands of skipped records. - */ - static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEncoding encoding, - long baseOffset, - ByteBuffer wrap, - long highestOffsetPartitionCanHold) + static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEncoding encoding, long baseOffset, ByteBuffer wrap) throws CorruptOffsetMetadataException { wrap.rewind(); int originalBitsetSize = switch (encoding) { @@ -91,16 +78,8 @@ static HighestOffsetAndIncompletes deserialiseBitSetWrapToIncompletes(OffsetEnco "bitset declares {} bit(s), needing {} byte(s), but only {} byte(s) follow the length field", originalBitsetSize, bytesNeeded, slice.remaining())); } - long highestSeenOffset = baseOffset + originalBitsetSize - 1; - // Checked before the set is built, for the same reason the run-length guard is checked before its run is - // walked: a payload we are about to refuse should not be allowed to allocate first. - if (highestOffsetPartitionCanHold != OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING - && highestSeenOffset > highestOffsetPartitionCanHold) { - throw new CorruptOffsetMetadataException(msg( - "bitset declares {} bit(s), reaching offset {}, but the partition holds nothing above offset {}", - originalBitsetSize, highestSeenOffset, highestOffsetPartitionCanHold)); - } SortedSet incompletes = deserialiseBitSetToIncompletes(baseOffset, originalBitsetSize, slice); + long highestSeenOffset = baseOffset + originalBitsetSize - 1; return HighestOffsetAndIncompletes.of(highestSeenOffset, incompletes); } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java index 287a6fc6c..a6dccd2c5 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetMapCodecManager.java @@ -17,7 +17,6 @@ import io.micrometer.core.instrument.Timer; import lombok.Value; import lombok.extern.slf4j.Slf4j; -import org.apache.kafka.clients.consumer.Consumer; import org.apache.kafka.clients.consumer.OffsetAndMetadata; import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.errors.WakeupException; @@ -26,7 +25,6 @@ import java.nio.charset.Charset; import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.stream.Collectors; import static bz.stub.parallelconsumer.internal.utils.StringUtils.msg; import static java.nio.charset.StandardCharsets.UTF_8; @@ -72,16 +70,6 @@ public class OffsetMapCodecManager { public static final Charset CHARSET_TO_USE = UTF_8; - /** - * Passed as the plausibility ceiling when nobody could establish where the partition ends, and read by the - * decoders as "refuse nothing on these grounds". - *

- * A guard with no ground truth must stand down rather than invent one: failing closed here would discard honest - * offset maps whenever a broker is slow to answer, which is a far more likely event than the corrupt payload the - * ceiling exists for. {@link OffsetRunLength#runLengthDecodeToIncompletes} owns what the ceiling means. - */ - public static final long UNKNOWN_PARTITION_CEILING = Long.MAX_VALUE; - private final PCModule module; private Timer offsetEncodingTimer; @@ -214,12 +202,10 @@ public Map> loadPartitionStateForAssignment } var partitionStates = new HashMap>(); - var highestOffsetsHeld = findHighestOffsetsHeld(partitionLastCommittedOffsets); partitionLastCommittedOffsets.forEach((tp, offsetAndMeta) -> { if (offsetAndMeta != null) { try { - PartitionState state = decodePartitionState(tp, offsetAndMeta, - highestOffsetsHeld.getOrDefault(tp, UNKNOWN_PARTITION_CEILING)); + PartitionState state = decodePartitionState(tp, offsetAndMeta); partitionStates.put(tp, state); } catch (OffsetDecodingError offsetDecodingError) { log.error("Error decoding offsets from assigned partition, dropping offset map (will replay previously completed messages - partition: {}, data: {})", @@ -242,61 +228,6 @@ public Map> loadPartitionStateForAssignment return partitionStates; } - /** - * Asks the broker where each partition carrying an offset map actually ends, so a decoded map can be checked - * against something it cannot forge. - *

- * This is the ground truth the plausibility ceiling needs, and there is nowhere cheaper to get it: PC holds no - * end offset of its own at assignment time, and a constant derived from configuration would eventually discard a - * true offset map - {@link OffsetRunLength#runLengthDecodeToIncompletes} carries that argument in full. - *

- * One request, and only when there is something to check. It is batched across the whole assignment and - * skipped entirely when no assigned partition has metadata, so it costs one {@code ListOffsets} round trip on a - * rebalance that already paid for several - and nothing at all for a group that has never committed a map. The - * timeout is the consumer's own {@code default.api.timeout.ms}, the same one governing the - * {@link Consumer#committed(Set)} call above it, because it is the same kind of call in the same frame. - *

- * Every failure is swallowed on purpose, loudly. A guard must not be able to turn a readable offset map - - * or an entire partition assignment - into a failure: an unavailable leader, a timeout, or a consumer - * implementation that does not answer this question all mean "no ground truth", which the decoders read as - * "refuse nothing". The warning is the visible half; the invisible half would be a rebalance that dies for the - * sake of a check. - * - * @return the last offset each partition holds, keyed by partition and absent where it could not be established - */ - private Map findHighestOffsetsHeld(Map partitionLastCommittedOffsets) { - var partitionsCarryingMetadata = partitionLastCommittedOffsets.entrySet().stream() - .filter(entry -> entry.getValue() != null) - .filter(entry -> entry.getValue().metadata() != null && !entry.getValue().metadata().isEmpty()) - .map(Map.Entry::getKey) - .collect(Collectors.toSet()); - if (partitionsCarryingMetadata.isEmpty()) { - return Collections.emptyMap(); - } - - try { - var endOffsets = module.consumer().endOffsets(partitionsCarryingMetadata); - var highestOffsetsHeld = new HashMap(); - endOffsets.forEach((tp, endOffset) -> { - if (endOffset != null) { - // the end offset is the next offset to be WRITTEN, so the last one the partition holds is below it - highestOffsetsHeld.put(tp, endOffset - 1); - } - }); - return highestOffsetsHeld; - } catch (Exception e) { - // The reason goes in the warning; the stack trace goes to debug, because this fires once per rebalance - // whenever a consumer will not answer the question - including every MockConsumer that was never told - // where its partitions end - and a trace per rebalance is how a warning stops being read. - log.warn("Could not read the end offsets of {} while loading their committed offset maps ({}), so those " + - "maps cannot be checked against the offsets the partitions actually hold - decoding them " + - "as they are. An offset map claiming offsets that do not exist would be accepted this time.", - partitionsCarryingMetadata, e.toString()); - log.debug("End offset lookup failed", e); - return Collections.emptyMap(); - } - } - /** * The epoch {@link PartitionState} is built with on assignment - and the one place * {@link bz.stub.parallelconsumer.state.PartitionStateManager#epochOfPartitionIfAssigned} is narrowed to a @@ -343,11 +274,8 @@ private long epochOfPartitionBeingAssigned(TopicPartition tp) { * @param offsetData the committed offset and its free-form metadata field * @throws OffsetDecodingError if the metadata is not valid base64 */ - private HighestOffsetAndIncompletes decodeOffsetMapForPartition(TopicPartition tp, - OffsetAndMetadata offsetData, - long highestOffsetPartitionCanHold) throws OffsetDecodingError { - return deserialiseIncompleteOffsetMapFromBase64(offsetData.offset(), offsetData.metadata(), errorPolicy, tp, - highestOffsetPartitionCanHold); + private HighestOffsetAndIncompletes decodeOffsetMapForPartition(TopicPartition tp, OffsetAndMetadata offsetData) throws OffsetDecodingError { + return deserialiseIncompleteOffsetMapFromBase64(offsetData.offset(), offsetData.metadata(), errorPolicy, tp); } /** @@ -405,21 +333,6 @@ public static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase String base64EncodedOffsetPayload, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) throws OffsetDecodingError { - return deserialiseIncompleteOffsetMapFromBase64(committedOffsetForPartition, base64EncodedOffsetPayload, - errorPolicy, tp, UNKNOWN_PARTITION_CEILING); - } - - /** - * @param highestOffsetPartitionCanHold the last offset the partition holds, against which an otherwise - * well-formed map is checked, or {@link #UNKNOWN_PARTITION_CEILING} when - * that could not be established - * @see #deserialiseIncompleteOffsetMapFromBase64(long, String, InvalidOffsetMetadataHandlingPolicy, TopicPartition) - */ - static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase64(long committedOffsetForPartition, - String base64EncodedOffsetPayload, - InvalidOffsetMetadataHandlingPolicy errorPolicy, - TopicPartition tp, - long highestOffsetPartitionCanHold) throws OffsetDecodingError { byte[] decodedBytes; try { decodedBytes = OffsetSimpleSerialisation.decodeBase64(base64EncodedOffsetPayload); @@ -436,25 +349,17 @@ static HighestOffsetAndIncompletes deserialiseIncompleteOffsetMapFromBase64(long EncodedOffsetPair.describeSource(tp, committedOffsetForPartition)), tp); } - return decodeCompressedOffsets(committedOffsetForPartition, decodedBytes, errorPolicy, tp, - highestOffsetPartitionCanHold); + return decodeCompressedOffsets(committedOffsetForPartition, decodedBytes, errorPolicy, tp); } PartitionState decodePartitionState(TopicPartition tp, OffsetAndMetadata offsetData) throws OffsetDecodingError { - return decodePartitionState(tp, offsetData, UNKNOWN_PARTITION_CEILING); - } - - /** - * @param highestOffsetPartitionCanHold the last offset this partition holds, or {@link #UNKNOWN_PARTITION_CEILING} - * when {@link #findHighestOffsetsHeld} could not establish it - */ - PartitionState decodePartitionState(TopicPartition tp, - OffsetAndMetadata offsetData, - long highestOffsetPartitionCanHold) throws OffsetDecodingError { - HighestOffsetAndIncompletes incompletes = decodeOffsetMapForPartition(tp, offsetData, highestOffsetPartitionCanHold); + HighestOffsetAndIncompletes incompletes = decodeOffsetMapForPartition(tp, offsetData); log.debug("Loaded incomplete offsets from offset payload {}", incompletes); long epoch = epochOfPartitionBeingAssigned(tp); - return new PartitionState<>(epoch, module, tp, incompletes); + // The committed offset travels with the decoded map: PartitionState checks the map's claim against the + // partition itself at the first batch, and falls back to this offset when the claim turns out to be one no + // partition could have produced - see PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition. + return new PartitionState<>(epoch, module, tp, incompletes, offsetData.offset()); } public String makeOffsetMetadataPayload(long baseOffsetForPartition, PartitionState state) throws NoEncodingPossibleException { @@ -560,19 +465,6 @@ static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffs byte[] decodedBytes, InvalidOffsetMetadataHandlingPolicy errorPolicy, TopicPartition tp) { - return decodeCompressedOffsets(nextExpectedOffset, decodedBytes, errorPolicy, tp, UNKNOWN_PARTITION_CEILING); - } - - /** - * @param highestOffsetPartitionCanHold the last offset the partition holds, or {@link #UNKNOWN_PARTITION_CEILING} - * when that could not be established - * @see #decodeCompressedOffsets(long, byte[], InvalidOffsetMetadataHandlingPolicy, TopicPartition) - */ - static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffset, - byte[] decodedBytes, - InvalidOffsetMetadataHandlingPolicy errorPolicy, - TopicPartition tp, - long highestOffsetPartitionCanHold) { // if no offset bitmap data if (decodedBytes.length == 0) { @@ -581,8 +473,7 @@ static HighestOffsetAndIncompletes decodeCompressedOffsets(long nextExpectedOffs long highestSeenOffsetIsThen = nextExpectedOffset - 1; return HighestOffsetAndIncompletes.of(highestSeenOffsetIsThen); } else { - return EncodedOffsetPair.decodeToIncompletes(decodedBytes, nextExpectedOffset, errorPolicy, tp, - highestOffsetPartitionCanHold); + return EncodedOffsetPair.decodeToIncompletes(decodedBytes, nextExpectedOffset, errorPolicy, tp); } } diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java index b401c46af..fa448a2e7 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/offsets/OffsetRunLength.java @@ -77,42 +77,9 @@ static String runLengthDecodeToString(final List in) { /** - * Decodes a run-length body into the incomplete offsets it names and the highest offset it claims to have seen. - * - *

The plausibility ceiling, and why it is the partition's end offset. Every other check in this method - * is one the buffer can settle by itself. A run length cannot be: a long run of completed offsets is precisely - * what run-length encoding is for, so nothing in the bytes distinguishes a legitimate one from an absurd - * one. Unchecked, a single four-byte entry of {@link Integer#MAX_VALUE} moves the highest-seen offset about two - * billion forward, and {@code PartitionState#isRecordPreviouslyCompleted} then treats every real record in that - * range as already succeeded - silent non-processing, not replay, from metadata anything sharing the consumer - * group can write. - * - *

What a legitimate long run looks like, so the guard cannot reject one. One record stuck at the - * committed offset while everything above it succeeds is an ordinary shape here, and it is not self-limiting: - * PC's back-pressure keys off the encoded payload size, and this payload stays three entries wide however - * far the partition runs ahead of the stuck record. So a run of hundreds of millions of completed offsets is real - * data, and any ceiling derived from a configured window - concurrency, the in-flight target, a round number - - * would eventually discard a true offset map and replay everything in it. That is why this method takes ground - * truth instead of a constant: the partition's log end offset is the one bound a legitimate map provably cannot - * cross, because PC only ever encodes offsets it has polled, and an offset the partition does not hold cannot - * have been polled. Kafka's end offset only ever grows, so a bound read now is still valid for a map written - * earlier. - * - *

The ceiling is checked before each run is applied rather than after the decode, so an incomplete - * run past the end of the partition is refused rather than walked - the same five-byte payload would otherwise - * ask for two billion boxed longs. - * - * @param baseOffset the committed offset the runs are relative to - * @param highestOffsetPartitionCanHold the last offset the partition actually holds (its log end offset minus - * one), or {@link OffsetMapCodecManager#UNKNOWN_PARTITION_CEILING} when the - * caller could not find out - in which case no run is refused on these - * grounds, because a guard with no ground truth must not invent one * @see #runLengthEncode */ - static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding encoding, - final long baseOffset, - final ByteBuffer in, - final long highestOffsetPartitionCanHold) + static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding encoding, final long baseOffset, final ByteBuffer in) throws CorruptOffsetMetadataException { in.rewind(); // asShortBuffer()/asIntBuffer() silently DROP a trailing partial element, so a body of the wrong width decodes @@ -192,26 +159,6 @@ static HighestOffsetAndIncompletes runLengthDecodeToIncompletes(OffsetEncoding e throw new CorruptOffsetMetadataException(msg("negative run length ({}) at offset {}", runLength, currentOffset)); } - // The partition is the only thing that can prove a run absurd - see this method's javadoc. Expressed - // as the room left rather than as (currentOffset + runLength), which would overflow for a payload - // built to make it do so. Discarding the map here is logged by - // EncodedOffsetPair#handleUnreadableMetadata with the partition, the base offset and this reason; - // it is deliberately not counted, which is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md. - if (highestOffsetPartitionCanHold != OffsetMapCodecManager.UNKNOWN_PARTITION_CEILING) { - // Arithmetic that cannot wrap, which is not fussiness: the unknown ceiling IS Long.MAX_VALUE, so - // a "currentOffset + runLength" or a "+ 1" on the room left overflows to a negative number and - // rejects EVERY payload, the encoder's own included - and a base offset of -1 (no commit yet) - // reaches this code, so the subtraction is not safe from the other side either. Hence: skip - // entirely when there is no ceiling, then measure the run as a DISTANCE from currentOffset. - long offsetsThePartitionStillHolds = highestOffsetPartitionCanHold - currentOffset; - if (runLength.longValue() > 0 && runLength.longValue() - 1 > offsetsThePartitionStillHolds) { - throw new CorruptOffsetMetadataException(msg( - "{} run of {} offset(s) at offset {} reaches past the end of the partition, which " + - "holds nothing above offset {} - no offset map this build wrote could name " + - "an offset that has never existed", - encoding.description(), runLength, currentOffset, highestOffsetPartitionCanHold)); - } - } if (currentRunLengthIsComplete) { log.trace("Ignoring {} completed offset(s) (offset:{})", runLength, currentOffset); currentOffset += runLength.longValue(); diff --git a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/PartitionState.java b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/PartitionState.java index 0f9c4e99a..970723b3a 100644 --- a/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/PartitionState.java +++ b/parallel-consumer-core/src/main/java/bz/stub/parallelconsumer/state/PartitionState.java @@ -6,11 +6,14 @@ */ import bz.stub.parallelconsumer.ParallelConsumer; +import bz.stub.parallelconsumer.ParallelConsumerOptions; import bz.stub.parallelconsumer.internal.BrokerPollSystem; import bz.stub.parallelconsumer.internal.EpochAndRecordsMap; import bz.stub.parallelconsumer.internal.PCModule; import bz.stub.parallelconsumer.metrics.PCMetrics; import bz.stub.parallelconsumer.metrics.PCMetricsDef; +import bz.stub.parallelconsumer.offsets.CorruptOffsetMetadataException; +import bz.stub.parallelconsumer.offsets.EncodedOffsetPair; import bz.stub.parallelconsumer.offsets.NoEncodingPossibleException; import bz.stub.parallelconsumer.offsets.OffsetMapCodecManager; import io.micrometer.core.instrument.DistributionSummary; @@ -21,6 +24,7 @@ import lombok.Setter; import lombok.ToString; import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.Consumer; import org.apache.kafka.clients.consumer.ConsumerRecord; import org.apache.kafka.clients.consumer.KafkaConsumer; import org.apache.kafka.clients.consumer.OffsetAndMetadata; @@ -31,6 +35,7 @@ import java.util.stream.Collectors; import static bz.stub.parallelconsumer.internal.utils.JavaUtils.*; +import static bz.stub.parallelconsumer.internal.utils.StringUtils.msg; import static bz.stub.parallelconsumer.offsets.OffsetMapCodecManager.DefaultMaxMetadataSize; import static java.util.Optional.empty; import static java.util.Optional.of; @@ -228,17 +233,70 @@ public class PartitionState { */ private boolean stateChangedSinceCommitStart = false; + /** + * The offset the loaded offset map was committed against - the first offset this partition will be polled from, + * and so the state to fall back to if the map turns out to be one we cannot believe. + *

+ * Captured at construction because it is only readable then: once records arrive, the frontier moves. + * + * @see #maybeVerifyLoadedOffsetMapAgainstThePartition + */ + private final long committedOffsetTheMapWasLoadedAgainst; + + /** + * Passed as {@link #committedOffsetTheMapWasLoadedAgainst} by any caller that did not load this state from a + * committed offset map - in which case there is no claim to corroborate and nothing to check. + */ + public static final long NO_OFFSET_MAP_WAS_LOADED = KAFKA_OFFSET_ABSENCE; + + /** + * True while the loaded map claims to have seen offsets that nothing has corroborated yet. + *

+ * Cleared the moment the partition either confirms the claim (it holds at least that far) or refutes it. A map + * claiming nothing above the committed offset starts out with nothing to check, so this starts false for it. + * + * @see #maybeVerifyLoadedOffsetMapAgainstThePartition + */ + private boolean loadedOffsetMapClaimIsUncorroborated; + /** + * For state that did not come from a committed offset map - a partition with no commit history, and every test + * that builds a state directly. Nothing is claimed, so nothing is checked against the partition. + * + * @see #PartitionState(long, PCModule, TopicPartition, OffsetMapCodecManager.HighestOffsetAndIncompletes, long) + */ public PartitionState(long newEpoch, PCModule pcModule, TopicPartition topicPartition, OffsetMapCodecManager.HighestOffsetAndIncompletes offsetData) { + this(newEpoch, pcModule, topicPartition, offsetData, NO_OFFSET_MAP_WAS_LOADED); + } + + /** + * @param committedOffsetTheMapWasLoadedAgainst the offset {@code offsetData} was decoded against, or + * {@link #NO_OFFSET_MAP_WAS_LOADED}. It has to be passed rather than + * derived: {@link #getOffsetToCommit()} answers + * {@code highestSucceeded + 1} for a map with no incomplete offsets + * in it, which is the committed offset only by coincidence and is + * wrong by the whole width of the map otherwise - deriving it there + * silently switched the check below off for every fully-completed + * map + * @see #maybeVerifyLoadedOffsetMapAgainstThePartition + */ + public PartitionState(long newEpoch, + PCModule pcModule, + TopicPartition topicPartition, + OffsetMapCodecManager.HighestOffsetAndIncompletes offsetData, + long committedOffsetTheMapWasLoadedAgainst) { this.module = pcModule; this.tp = topicPartition; this.partitionsAssignmentEpoch = newEpoch; this.pcMetrics = module.pcMetrics(); initStateFromOffsetData(offsetData); + this.committedOffsetTheMapWasLoadedAgainst = committedOffsetTheMapWasLoadedAgainst; + this.loadedOffsetMapClaimIsUncorroborated = committedOffsetTheMapWasLoadedAgainst != NO_OFFSET_MAP_WAS_LOADED + && offsetHighestSeen >= committedOffsetTheMapWasLoadedAgainst; initMetrics(); this.om = new OffsetMapCodecManager<>(pcModule); } @@ -438,6 +496,10 @@ public void maybeRegisterNewPollBatchAsWork(@NonNull EpochAndRecordsMap.Re return; } + // BEFORE anything consults the loaded map - truncation reads it, and the loop below asks it about every + // record - settle whether the map is one this partition could have produced at all. + maybeVerifyLoadedOffsetMapAgainstThePartition(recordsAndEpoch); + // maybeTruncateOrPruneTrackedOffsets(recordsAndEpoch); @@ -465,6 +527,95 @@ public void maybeRegisterNewPollBatchAsWork(@NonNull EpochAndRecordsMap.Re } + /** + * Settles whether the offset map loaded on assignment describes this partition, using the first batch + * that arrives - and treats one that cannot as unreadable metadata, through the same + * {@link ParallelConsumerOptions#getInvalidOffsetMetadataPolicy()} that governs every other payload this build + * cannot read. + * + *

The claim, and why the bytes cannot settle it. A run-length entry of {@link Integer#MAX_VALUE} is + * structurally perfect: it moves the highest-seen offset about two billion forward, and + * {@link #isRecordPreviouslyCompleted} then reads every real record in that range as already succeeded, so they + * are skipped without ever reaching the user's function - silent non-processing, not replay, from metadata + * anything sharing the consumer group can write. A long run of completed offsets is also exactly what run-length + * encoding is for, so no decoder can tell the two apart: astubbs#207 rejects everything the payload + * itself proves wrong, and stops precisely here. The check is encoding-agnostic on purpose - it reads the + * decoded claim, so a bitset making the same claim is caught by the same line. + * + *

The bound is the partition, and nothing else will do. A ceiling derived from configuration - + * concurrency, the in-flight target, a round number - eventually discards a true map and replays + * everything it covered, because PC's back-pressure 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. The partition's log end offset is + * the one bound a legitimate map provably cannot cross: PC only ever encodes offsets it has polled, an offset + * the partition does not hold cannot have been polled, and a Kafka log end offset only grows, so a watermark + * read now still bounds a map written earlier. + * + *

Why here and not at decode. Decoding happens inside the rebalance callback, where the only way to + * learn where a partition ends is a blocking {@code ListOffsets} request - a broker round trip on the most + * fragile path this project has, which would also fail open exactly when the broker is unhealthy, i.e. when + * rebalances are already storming. The fetch that delivers the first batch carries the high watermark anyway + * ({@link Consumer#currentLag}, KIP-695), so the answer is already in the process by the time it is needed. The + * cost of waiting for it is nil: nothing consults the map until a record arrives to be consulted about, + * and this runs before that. + * + *

Absence is not evidence. An empty {@code logEndOffsetAtPoll} means nobody has established where the + * partition ends - never that it ends here. The batch's own records are then used as a floor, because a record + * that arrived certainly exists: a floor can only ever confirm a claim, never refute one, so if it does + * not reach the claim we simply look again at the next batch. + */ + private void maybeVerifyLoadedOffsetMapAgainstThePartition(EpochAndRecordsMap.RecordsAndEpoch recordsAndEpoch) { + if (!loadedOffsetMapClaimIsUncorroborated) { + return; + } + + OptionalLong logEndOffsetExclusive = recordsAndEpoch.getLogEndOffsetAtPoll(); + if (!logEndOffsetExclusive.isPresent()) { // Optional#isEmpty is Java 11 - this module compiles against Java 8's API + var lastOfBatch = getLast(recordsAndEpoch.getRecords()); + boolean batchReachesTheClaim = lastOfBatch.isPresent() + && lastOfBatch.get().offset() >= offsetHighestSeen; + if (batchReachesTheClaim) { + // Records this far up exist, so the map claimed nothing the partition does not hold. + loadedOffsetMapClaimIsUncorroborated = false; + } + return; + } + + long highestOffsetThePartitionHolds = logEndOffsetExclusive.getAsLong() - 1; + loadedOffsetMapClaimIsUncorroborated = false; + if (claimsOffsetsThePartitionDoesNotHold(offsetHighestSeen, logEndOffsetExclusive.getAsLong())) { + var problem = msg("the offset map claims to have seen offset {}, but the partition holds nothing above " + + "offset {} - no map this build wrote could name an offset that has never existed", + offsetHighestSeen, highestOffsetThePartitionHolds); + // The same handler, and so the same user-visible event, as every other unreadable payload: under IGNORE + // it warns with the partition, the base offset and this reason and answers with the state we would have + // had if the commit had carried no map at all; under FAIL it throws. Nothing counts the discard - that + // is docs/inflight/bug-no-metric-for-discarded-offset-metadata.md, and out of scope here. + var withoutTheMap = EncodedOffsetPair.handleUnreadableMetadata(committedOffsetTheMapWasLoadedAgainst, + module.options().getInvalidOffsetMetadataPolicy(), + problem, + () -> new CorruptOffsetMetadataException(problem, + EncodedOffsetPair.describeSource(tp, committedOffsetTheMapWasLoadedAgainst)), + tp); + initStateFromOffsetData(withoutTheMap); + } + } + + /** + * The rule, in one expression: an offset map may not name an offset the partition does not hold. + *

+ * Public and static so the encoder-side round trip can assert the other direction - that output this build + * produced, measured against a partition ending exactly on the last offset it encoded, is never refused. That is + * the property which makes this safe to apply at all, and it is asserted rather than argued in + * {@code EncoderOutputSurvivesDecodeValidationTest}. + * + * @param highestSeenOffset what the decoded map claims to have seen, inclusive + * @param logEndOffsetExclusive where the partition ends - the next offset to be written, so the last one it + * holds is below it + */ + public static boolean claimsOffsetsThePartitionDoesNotHold(long highestSeenOffset, long logEndOffsetExclusive) { + return highestSeenOffset > logEndOffsetExclusive - 1; + } + /** * Used for adding work to, if it's been successfully added to our tracked state * diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java new file mode 100644 index 000000000..1cbdf799c --- /dev/null +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java @@ -0,0 +1,105 @@ +package bz.stub.parallelconsumer.internal; + +/*- + * Copyright (C) 2026 Antony Stubbs and contributors + */ + +import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.MockConsumer; +import org.apache.kafka.clients.consumer.OffsetResetStrategy; +import org.apache.kafka.common.TopicPartition; +import org.junit.jupiter.api.Test; +import pl.tlinkowski.unij.api.UniLists; +import pl.tlinkowski.unij.api.UniMaps; + +import java.time.Duration; +import java.util.OptionalLong; + +import static com.google.common.truth.Truth.assertThat; + +/** + * {@link ConsumerManager#logEndOffsetIfKnownWithoutBlocking} is the ground truth behind the offset-map plausibility + * check ({@code PartitionState#maybeVerifyLoadedOffsetMapAgainstThePartition}), and it has exactly one hard + * requirement beyond being right: it must never block, never throw, and never send a request of its own - it runs on + * the poll thread, inside the poll loop, for a check that must not be able to slow ingestion down. + *

+ * What {@link MockConsumer} actually does here, verified rather than assumed (kafka-clients 3.9.1): its + * {@code currentLag} answers {@code endOffsets.get(tp) - position(tp)} when the partition has been given an end + * offset with {@link MockConsumer#updateEndOffsets}, and {@code OptionalLong.of(0)} when it has not - not + * empty, as a real consumer answers before its first fetch. Lag zero reads as "the partition ends at the current + * position", so an unprimed mock understates where the partition ends, and a test that needs this check armed has to + * prime end offsets. Nothing in the suite was affected (the check only engages for a partition whose committed + * metadata claims offsets above the committed one), but the next test to reach for it will meet this, so it is + * pinned here rather than left to be rediscovered. + * + * @author Antony Stubbs + */ +@Slf4j +class ConsumerManagerLogEndOffsetTest { + + static final TopicPartition TP = new TopicPartition("myTopic", 0); + + static final Duration TIMEOUT = Duration.ofSeconds(1); + + private ConsumerManager managerFor(MockConsumer mockConsumer) { + return new ConsumerManager<>(new ThreadConfinedConsumer<>(mockConsumer), TIMEOUT, TIMEOUT, TIMEOUT); + } + + /** + * The reconstruction that matters: the log end offset is {@code position + lag}, and both are read together. + */ + @Test + void theLogEndOffsetIsThePositionPlusTheLagTheLastFetchReported() { + var mockConsumer = new MockConsumer(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(TP)); + mockConsumer.updateBeginningOffsets(UniMaps.of(TP, 0L)); + mockConsumer.updateEndOffsets(UniMaps.of(TP, 500L)); + mockConsumer.seek(TP, 100L); + + assertThat(managerFor(mockConsumer).logEndOffsetIfKnownWithoutBlocking(TP)) + .isEqualTo(OptionalLong.of(500L)); + } + + /** + * A partition the consumer has no position for cannot be answered without asking the broker, and asking is + * exactly what this method may not do. The answer is "not established", which the caller re-checks on the next + * batch - it is never read as a bound. + */ + @Test + void aPartitionWithNoPositionAnswersNotEstablishedRatherThanBlocking() { + var mockConsumer = new MockConsumer(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(TP)); + mockConsumer.updateEndOffsets(UniMaps.of(TP, 500L)); // an end offset, but nowhere to measure it from + + assertThat(managerFor(mockConsumer).logEndOffsetIfKnownWithoutBlocking(TP)) + .isEqualTo(OptionalLong.empty()); + } + + /** + * The mock's answer with no end offset given to it, pinned because the prose above rests on it and because a + * test that assumed {@code empty()} here would be quietly wrong: the mock says lag zero, so this method answers + * "the partition ends at the position", which is a bound - just a mistaken one. Prime end offsets in any test + * that needs this check to engage truthfully. + */ + @Test + void anUnprimedMockConsumerClaimsZeroLagRatherThanNotKnowing() { + var mockConsumer = new MockConsumer(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(TP)); + mockConsumer.updateBeginningOffsets(UniMaps.of(TP, 0L)); + mockConsumer.seek(TP, 100L); + + assertThat(managerFor(mockConsumer).logEndOffsetIfKnownWithoutBlocking(TP)) + .isEqualTo(OptionalLong.of(100L)); // position + 0, not empty - MockConsumer.currentLag's default + } + + /** + * And a partition that is not ours at all - which happens when a rebalance lands between the poll and this read. + */ + @Test + void anUnassignedPartitionAnswersNotEstablished() { + var mockConsumer = new MockConsumer(OffsetResetStrategy.EARLIEST); + + assertThat(managerFor(mockConsumer).logEndOffsetIfKnownWithoutBlocking(TP)) + .isEqualTo(OptionalLong.empty()); + } +} diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java index 271c97144..ed0870aa3 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java @@ -6,6 +6,7 @@ import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; import lombok.extern.slf4j.Slf4j; +import bz.stub.parallelconsumer.state.PartitionState; import org.apache.kafka.common.TopicPartition; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -35,6 +36,7 @@ * @author Antony Stubbs * @see OffsetBitSet#deserialiseBitSetWrapToIncompletes * @see OffsetRunLength#runLengthDecodeToIncompletes + * @see PartitionState#claimsOffsetsThePartitionDoesNotHold */ @Slf4j class EncoderOutputSurvivesDecodeValidationTest { @@ -47,12 +49,12 @@ class EncoderOutputSurvivesDecodeValidationTest { * Encodes {@code length} offsets from {@link #BASE_OFFSET} where only {@code incompleteIndexes} are incomplete, * then hands the bytes back through the production decode path exactly as a rebalance would. *

- * Every shape is decoded twice, once with no plausibility ceiling and once with the tightest honest one - - * a partition that ends exactly at the last offset encoded. That second pass is what pins the encoder side of - * {@link OffsetRunLength#runLengthDecodeToIncompletes}'s guard: the encoder only ever encodes offsets PC has - * polled, so its output always ends on an offset the partition holds, and the guard must therefore never see it - * as a claim past the end. Anything that made the encoder write one offset further than it saw would land here - * as a false rejection rather than as silence. + * Each shape is also measured against the plausibility rule + * ({@link PartitionState#claimsOffsetsThePartitionDoesNotHold}), with the tightest honest bound there is: a + * partition that ends exactly on the last offset encoded. That is the encoder-side half of the rule - the + * encoder only ever encodes offsets PC has polled, so its output always ends on an offset the partition holds, + * and the check applied to the first batch must therefore never see it as a claim past the end. Anything that + * made the encoder write one offset further than it saw lands here as a false rejection instead of as silence. */ private static void roundTrip(OffsetEncoding encoding, int length, SortedSet incompletes) throws Exception { long highestSucceeded = BASE_OFFSET + length - 1; @@ -69,19 +71,21 @@ private static void roundTrip(OffsetEncoding encoding, int length, SortedSet { - var decoded = EncodedOffsetPair.decodeToIncompletes(withMagic.array(), BASE_OFFSET, - InvalidOffsetMetadataHandlingPolicy.FAIL, TP, ceiling); - assertThat(decoded.getIncompleteOffsets()) - .as("a payload this build wrote must decode back to the offsets it encoded") - .isEqualTo(incompletes); - }) - .as("FAIL must not reject %s output at length %s against a partition ending at %s - a false " - + "rejection discards a real offset map and replays completed work", - encoding, length, ceiling) - .doesNotThrowAnyException(); - } + assertThatCode(() -> { + var decoded = EncodedOffsetPair.decodeToIncompletes(withMagic.array(), BASE_OFFSET, + InvalidOffsetMetadataHandlingPolicy.FAIL, TP); + assertThat(decoded.getIncompleteOffsets()) + .as("a payload this build wrote must decode back to the offsets it encoded") + .isEqualTo(incompletes); + assertThat(PartitionState.claimsOffsetsThePartitionDoesNotHold( + decoded.getHighestSeenOffset().orElse(BASE_OFFSET - 1), highestSucceeded + 1)) + .as("%s output at length %s claims nothing beyond the last offset it encoded, so a partition " + + "ending exactly there must not refuse it - a false rejection discards a real offset " + + "map and replays completed work", encoding, length) + .isFalse(); + }) + .as("FAIL must not reject %s output at length %s", encoding, length) + .doesNotThrowAnyException(); } /** diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java deleted file mode 100644 index 2051820f5..000000000 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnAssignmentTest.java +++ /dev/null @@ -1,298 +0,0 @@ -package bz.stub.parallelconsumer.offsets; - -/*- - * Copyright (C) 2026 Antony Stubbs and contributors - */ - -import bz.stub.parallelconsumer.ParallelConsumerOptions; -import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; -import bz.stub.parallelconsumer.internal.PCModuleTestEnv; -import bz.stub.parallelconsumer.state.PartitionState; -import bz.stub.parallelconsumer.state.WorkManager; -import lombok.extern.slf4j.Slf4j; -import org.apache.kafka.clients.consumer.ConsumerRecord; -import org.apache.kafka.clients.consumer.MockConsumer; -import org.apache.kafka.clients.consumer.OffsetAndMetadata; -import org.apache.kafka.clients.consumer.OffsetResetStrategy; -import org.apache.kafka.common.TopicPartition; -import org.junit.jupiter.api.Test; -import pl.tlinkowski.unij.api.UniLists; -import pl.tlinkowski.unij.api.UniMaps; - -import java.nio.ByteBuffer; -import java.util.Base64; -import java.util.function.UnaryOperator; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatCode; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -/** - * An offset map whose decoded range reaches past the end of the partition claims offsets that do not exist, and must - * be treated as unreadable rather than decoded. - *

- * This is the fourth outcome {@link EncodedOffsetPair#getDecodedIncompletes(long, - * InvalidOffsetMetadataHandlingPolicy, TopicPartition)} used to document as uncoverable - "a payload that decodes - * cleanly into a wrong-but-plausible map". Nothing in the bytes proves a long run wrong, because a long run - * of completed offsets is exactly what run-length encoding is for; what proves it wrong is the partition, which - * cannot hold the offsets the run claims. - *

- * Left unchecked, one four-byte entry of {@link Integer#MAX_VALUE} moves the highest-seen offset about two billion - * forward, and {@link PartitionState#isRecordPreviouslyCompleted} then reads every real record in that range as - * already succeeded - silent non-processing, not replay, from metadata anything sharing the consumer group can write. - * - * @author Antony Stubbs - * @see OffsetRunLength#runLengthDecodeToIncompletes - */ -@Slf4j -class ImplausibleOffsetMapOnAssignmentTest { - - static final TopicPartition TP = new TopicPartition("myTopic", 0); - - static final long COMMITTED_OFFSET = 100L; - - /** - * The partition holds offsets {@code 0..199}, so 199 is the highest offset any honest offset map can name. - */ - static final long PARTITION_END_OFFSET = 200L; - - /** - * Comfortably inside the partition, and inside the range the corrupt payload claims - so it is a record that - * really exists and would really be skipped. - */ - static final long REAL_RECORD_IN_THE_CLAIMED_RANGE = 150L; - - MockConsumer mockConsumer; - - /** - * Base64 of a {@link OffsetEncoding#RunLengthV2} payload with the given runs, the first of which is a run of - * incomplete offsets (the decoder starts there, because the committed offset is the lowest incomplete - * one). - */ - private static String runLengthV2Metadata(int... runs) { - ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + runs.length * Integer.BYTES); - body.put(OffsetEncoding.RunLengthV2.magicByte); - for (int run : runs) { - body.putInt(run); - } - return Base64.getEncoder().encodeToString(body.array()); - } - - /** - * Base64 of a {@link OffsetEncoding#BitSetV2} payload declaring {@code declaredBits} offsets, every one of them - * completed, with the body that length actually requires - so it passes astubbs#207's bytes-must-back-the-length - * check and reaches the ceiling on its merits. - */ - private static String bitSetV2Metadata(int declaredBits) { - int bodyBytes = (declaredBits + 7) / 8; - ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + Integer.BYTES + bodyBytes); - body.put(OffsetEncoding.BitSetV2.magicByte); - body.putInt(declaredBits); - for (int i = 0; i < bodyBytes; i++) { - body.put((byte) 0xFF); // every offset in the range marked completed - } - return Base64.getEncoder().encodeToString(body.array()); - } - - private PCModuleTestEnv moduleWithCommittedMetadata(String metadata) { - return moduleWithCommittedMetadata(metadata, PARTITION_END_OFFSET, UnaryOperator.identity()); - } - - private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, InvalidOffsetMetadataHandlingPolicy policy) { - return moduleWithCommittedMetadata(metadata, PARTITION_END_OFFSET, - builder -> builder.invalidOffsetMetadataPolicy(policy)); - } - - /** - * Builds a module whose consumer has {@code metadata} committed against {@link #TP} and reports {@code endOffset} - * as that partition's log end offset. - * - * @param endOffset the log end offset to report, or a negative number to leave it unprimed - which makes - * {@link MockConsumer#endOffsets} throw, standing in for a broker that will not answer - */ - private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, - long endOffset, - UnaryOperator> configure) { - mockConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); - mockConsumer.assign(UniLists.of(TP)); - mockConsumer.commitSync(UniMaps.of(TP, new OffsetAndMetadata(COMMITTED_OFFSET, metadata))); - if (endOffset >= 0) { - mockConsumer.updateEndOffsets(UniMaps.of(TP, endOffset)); - } - - var options = configure.apply(ParallelConsumerOptions.builder() - .consumer(mockConsumer)).build(); - return new PCModuleTestEnv(options); - } - - private static ConsumerRecord recordAt(long offset) { - return new ConsumerRecord<>(TP.topic(), TP.partition(), offset, "key", "value"); - } - - /** - * The defect, at the frame where it does its damage: the offset map is loaded on assignment, and every record in - * the range it invented is then skipped without ever reaching the user function. - */ - @Test - void aRunPastTheEndOfThePartitionDoesNotMarkRealRecordsAsCompleted() { - // one incomplete offset (the committed one), then a completed run of about two billion - var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1)); - WorkManager wm = module.workManager(); - - wm.onPartitionsAssigned(UniLists.of(TP)); - - PartitionState state = wm.getPm().getPartitionState(TP); - assertThat(state.isRecordPreviouslyCompleted(recordAt(REAL_RECORD_IN_THE_CLAIMED_RANGE))) - .as("a record the partition really holds must not be treated as completed because a payload claimed " + - "a run reaching past the end of the partition") - .isFalse(); - assertThat(state.getOffsetHighestSucceeded()) - .as("the metadata is unreadable, so it is discarded and we resume from the committed offset") - .isEqualTo(COMMITTED_OFFSET - 1); - } - - /** - * The other half of the policy contract: {@code FAIL} exists so a deployment can refuse to carry on with an offset - * map this build cannot believe, rather than silently discarding it. - */ - @Test - void failPolicyRejectsARunPastTheEndOfThePartition() { - var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1), - InvalidOffsetMetadataHandlingPolicy.FAIL); - WorkManager wm = module.workManager(); - - assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("FAIL must not silently accept an offset map claiming offsets the partition cannot hold") - .isInstanceOf(CorruptOffsetMetadataException.class) - .hasMessageContaining(TP.toString()); - } - - /** - * A run of incomplete offsets is the same lie with a different cost: the decoder used to walk it one - * offset at a time into a {@link java.util.TreeSet}, so a five-byte payload asked for two billion boxed longs. The - * guard has to fire before the loop, not after it. - */ - @Test - void anIncompleteRunPastTheEndOfThePartitionIsRejectedBeforeItIsWalked() { - var module = moduleWithCommittedMetadata(runLengthV2Metadata(Integer.MAX_VALUE), - InvalidOffsetMetadataHandlingPolicy.FAIL); - WorkManager wm = module.workManager(); - - assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("an incomplete run past the end of the partition must be rejected rather than materialised") - .isInstanceOf(CorruptOffsetMetadataException.class); - } - - /** - * The guard must not reject a real offset map, and a long run of completed offsets is a real shape: one record - * stuck at the committed offset while every offset above it succeeds is exactly what run-length encoding is for, - * and PC's back-pressure does not stop it - the payload stays three entries wide however far the partition runs - * ahead. - *

- * Pinned at the boundary: the run ends on the last offset the partition holds, which is the largest map that is - * still honest. - */ - @Test - void aLongRunThatStaysInsideThePartitionIsDecodedNormally() { - long partitionEndOffset = 1_000_000_000L; - long highestOffsetHeld = partitionEndOffset - 1; - int completedRun = (int) (highestOffsetHeld - COMMITTED_OFFSET); - var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, completedRun), - partitionEndOffset, - UnaryOperator.identity()); - WorkManager wm = module.workManager(); - - assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("a run ending on the last offset the partition holds is honest, and must decode") - .doesNotThrowAnyException(); - - PartitionState state = wm.getPm().getPartitionState(TP); - assertThat(state.getOffsetHighestSucceeded()) - .as("the whole run was accepted, right up to the last offset the partition holds") - .isEqualTo(highestOffsetHeld); - assertThat(state.isRecordPreviouslyCompleted(recordAt(COMMITTED_OFFSET))) - .as("the one incomplete offset in the map is still incomplete") - .isFalse(); - assertThat(state.isRecordPreviouslyCompleted(recordAt(COMMITTED_OFFSET + 1))) - .as("everything above it really did succeed - the long run is data, not corruption") - .isTrue(); - } - - /** - * One offset further than the partition can hold, and nothing else changed - the control arm for the test above. - */ - @Test - void aRunOneOffsetPastTheLastOffsetHeldIsRejected() { - long partitionEndOffset = 1_000_000_000L; - int completedRun = (int) (partitionEndOffset - COMMITTED_OFFSET); // one past the last offset held - var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, completedRun), - partitionEndOffset, - builder -> builder.invalidOffsetMetadataPolicy(InvalidOffsetMetadataHandlingPolicy.FAIL)); - WorkManager wm = module.workManager(); - - assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("the first offset the partition does not hold is the first one a map may not claim") - .isInstanceOf(CorruptOffsetMetadataException.class); - } - - /** - * When the broker will not say where the partition ends, the guard has no ground truth and must not invent one: - * it stands down and the payload decodes exactly as it did before this check existed. - *

- * Pinned because it is the behaviour every deployment gets when a leader is unavailable during a rebalance, and - * because failing closed here would discard honest offset maps on a broker hiccup - a far more likely - * event than the corrupt payload this guard is for. - */ - @Test - void anUnknownEndOffsetLeavesTheDecodeAloneRatherThanRejectingIt() { - var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, 1_000), - -1, // end offsets left unprimed: MockConsumer refuses to answer, as a broker that times out would - builder -> builder.invalidOffsetMetadataPolicy(InvalidOffsetMetadataHandlingPolicy.FAIL)); - WorkManager wm = module.workManager(); - - assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("a guard with no ground truth must stand down, not reject") - .doesNotThrowAnyException(); - - assertThat(wm.getPm().getPartitionState(TP).getOffsetHighestSucceeded()) - .as("the payload decoded as it always did") - .isEqualTo(COMMITTED_OFFSET + 1_000); - } - - /** - * The same defect one encoding over, and the reason the ceiling is applied to both decoders rather than only to - * the one the bug was reported against. A bitset is less exposed - astubbs#207 made its declared length - * prove itself against bytes that are present, so a full metadata field buys tens of thousands of skipped records - * rather than two billion - but bounded is not true. - */ - @Test - void aBitSetReachingPastTheEndOfThePartitionIsRejected() { - var module = moduleWithCommittedMetadata(bitSetV2Metadata(300), // 100 + 300 - 1, well past offset 199 - InvalidOffsetMetadataHandlingPolicy.FAIL); - WorkManager wm = module.workManager(); - - assertThatThrownBy(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("a bitset claiming offsets the partition does not hold is as unreadable as a run length doing it") - .isInstanceOf(CorruptOffsetMetadataException.class); - } - - /** - * The bitset control arm, at the boundary: a map covering every offset from the committed one to the last the - * partition holds is the widest honest bitset there is, and must decode. - */ - @Test - void aBitSetEndingOnTheLastOffsetHeldIsDecodedNormally() { - int bitsToTheEndOfThePartition = (int) (PARTITION_END_OFFSET - COMMITTED_OFFSET); - var module = moduleWithCommittedMetadata(bitSetV2Metadata(bitsToTheEndOfThePartition), - InvalidOffsetMetadataHandlingPolicy.FAIL); - WorkManager wm = module.workManager(); - - assertThatCode(() -> wm.onPartitionsAssigned(UniLists.of(TP))) - .as("a bitset ending exactly on the last offset the partition holds is honest") - .doesNotThrowAnyException(); - - assertThat(wm.getPm().getPartitionState(TP).getOffsetHighestSucceeded()) - .as("the whole declared range was accepted") - .isEqualTo(PARTITION_END_OFFSET - 1); - } -} diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java new file mode 100644 index 000000000..b54e36df9 --- /dev/null +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java @@ -0,0 +1,341 @@ +package bz.stub.parallelconsumer.offsets; + +/*- + * Copyright (C) 2026 Antony Stubbs and contributors + */ + +import bz.stub.parallelconsumer.ParallelConsumerOptions; +import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; +import bz.stub.parallelconsumer.internal.EpochAndRecordsMap; +import bz.stub.parallelconsumer.internal.PCModuleTestEnv; +import bz.stub.parallelconsumer.state.PartitionState; +import bz.stub.parallelconsumer.state.WorkManager; +import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.ConsumerRecord; +import org.apache.kafka.clients.consumer.ConsumerRecords; +import org.apache.kafka.clients.consumer.MockConsumer; +import org.apache.kafka.clients.consumer.OffsetAndMetadata; +import org.apache.kafka.clients.consumer.OffsetResetStrategy; +import org.apache.kafka.common.TopicPartition; +import org.junit.jupiter.api.Test; +import pl.tlinkowski.unij.api.UniLists; +import pl.tlinkowski.unij.api.UniMaps; + +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Base64; +import java.util.List; +import java.util.OptionalLong; +import java.util.function.UnaryOperator; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * An offset map whose decoded range reaches past the end of the partition claims offsets that do not exist, and is + * refused the moment the partition can say so - which is the first batch of records, not the assignment. + *

+ * This is the fourth outcome {@link EncodedOffsetPair#getDecodedIncompletes(long, + * InvalidOffsetMetadataHandlingPolicy, TopicPartition)} documents as one nothing in the payload can settle. + * A run-length entry of {@link Integer#MAX_VALUE} is structurally perfect and moves the highest-seen offset about two + * billion forward; {@link PartitionState#isRecordPreviouslyCompleted} then reads every real record in that range as + * already succeeded, so PC skips them without ever calling the user's function - silent non-processing, not replay. + *

+ * These tests drive the real path: assignment loads the map, then a batch arrives through + * {@link WorkManager#registerWork}, carrying the log end offset the way the poll loop supplies it from + * {@code ConsumerManager#logEndOffsetIfKnownWithoutBlocking}. + * + * @author Antony Stubbs + * @see PartitionState#claimsOffsetsThePartitionDoesNotHold + */ +@Slf4j +class ImplausibleOffsetMapOnFirstBatchTest { + + static final TopicPartition TP = new TopicPartition("myTopic", 0); + + static final long COMMITTED_OFFSET = 100L; + + /** + * The partition holds offsets {@code 0..199}, so 199 is the highest offset any honest offset map can name. + */ + static final long PARTITION_END_OFFSET = 200L; + + /** + * Comfortably inside the partition, and inside the range a corrupt payload claims - records that really exist and + * would really be skipped. + */ + static final long FIRST_RECORD_OF_BATCH = 140L; + + static final long LAST_RECORD_OF_BATCH = 150L; + + static final int RECORDS_IN_BATCH = (int) (LAST_RECORD_OF_BATCH - FIRST_RECORD_OF_BATCH + 1); + + MockConsumer mockConsumer; + + /** + * Base64 of a {@link OffsetEncoding#RunLengthV2} payload with the given runs, the first of which is a run of + * incomplete offsets (the decoder starts there, because the committed offset is the lowest incomplete + * one). + */ + private static String runLengthV2Metadata(int... runs) { + ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + runs.length * Integer.BYTES); + body.put(OffsetEncoding.RunLengthV2.magicByte); + for (int run : runs) { + body.putInt(run); + } + return Base64.getEncoder().encodeToString(body.array()); + } + + /** + * Base64 of a {@link OffsetEncoding#BitSetV2} payload declaring {@code declaredBits} offsets, every one of them + * completed, with the body that length actually requires - so it passes astubbs#207's bytes-must-back-the-length + * check and reaches this check on its merits. + */ + private static String bitSetV2Metadata(int declaredBits) { + int bodyBytes = (declaredBits + 7) / 8; + ByteBuffer body = ByteBuffer.allocate(Byte.BYTES + Integer.BYTES + bodyBytes); + body.put(OffsetEncoding.BitSetV2.magicByte); + body.putInt(declaredBits); + for (int i = 0; i < bodyBytes; i++) { + body.put((byte) 0xFF); // every offset in the range marked completed + } + return Base64.getEncoder().encodeToString(body.array()); + } + + /** + * A run-length map that claims one incomplete offset at the committed one, then {@code completedRun} completed + * offsets above it - so its highest-seen offset is {@code COMMITTED_OFFSET + completedRun}. + */ + private static String mapClaimingUpTo(long highestSeenOffset) { + return runLengthV2Metadata(1, (int) (highestSeenOffset - COMMITTED_OFFSET)); + } + + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata) { + return moduleWithCommittedMetadata(metadata, UnaryOperator.identity()); + } + + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, InvalidOffsetMetadataHandlingPolicy policy) { + return moduleWithCommittedMetadata(metadata, builder -> builder.invalidOffsetMetadataPolicy(policy)); + } + + /** + * Builds a module whose consumer has {@code metadata} committed against {@link #TP}, as a previous owner of the + * consumer group would have left it. + */ + private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, + UnaryOperator> configure) { + mockConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(TP)); + mockConsumer.commitSync(UniMaps.of(TP, new OffsetAndMetadata(COMMITTED_OFFSET, metadata))); + + var options = configure.apply(ParallelConsumerOptions.builder() + .consumer(mockConsumer)).build(); + return new PCModuleTestEnv(options); + } + + private static List> batchOfRecords(long fromInclusive, long toInclusive) { + List> records = new ArrayList<>(); + for (long offset = fromInclusive; offset <= toInclusive; offset++) { + records.add(new ConsumerRecord<>(TP.topic(), TP.partition(), offset, "key-" + offset, "value")); + } + return records; + } + + /** + * A batch exactly as the poll loop delivers one: records, plus where the fetch said the partition ends. + * + * @param logEndOffsetExclusive what {@code ConsumerManager#logEndOffsetIfKnownWithoutBlocking} answered - + * {@link OptionalLong#empty()} when the consumer could not say without blocking + */ + private static EpochAndRecordsMap batch(WorkManager wm, + List> records, + OptionalLong logEndOffsetExclusive) { + var poll = new ConsumerRecords<>(UniMaps.of(TP, records)); + return new EpochAndRecordsMap<>(poll, wm.getPm(), partition -> logEndOffsetExclusive); + } + + private static WorkManager assignedWorkManager(PCModuleTestEnv module) { + WorkManager wm = module.workManager(); + wm.onPartitionsAssigned(UniLists.of(TP)); + return wm; + } + + /** + * The defect, at the frame where it does its damage: every record in the range the map invented is skipped + * without ever reaching the user's function. + */ + @Test + void aMapClaimingOffsetsThePartitionDoesNotHoldIsDiscardedBeforeAnyRecordIsConsulted() { + // one incomplete offset (the committed one), then a completed run of about two billion + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1)); + WorkManager wm = assignedWorkManager(module); + + wm.registerWork(batch(wm, batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.of(PARTITION_END_OFFSET))); + + PartitionState state = wm.getPm().getPartitionState(TP); + assertThat(state.getNumberOfIncompleteOffsets()) + .as("every record the partition really holds must have become work, rather than being skipped " + + "against a map claiming a range reaching past the end of the partition") + .isEqualTo(RECORDS_IN_BATCH); + assertThat(state.isRecordPreviouslyCompleted(recordAt(LAST_RECORD_OF_BATCH))) + .as("nothing in the discarded map may still be answering questions about real records") + .isFalse(); + } + + /** + * The other half of the policy contract: {@code FAIL} exists so a deployment can refuse to carry on with an + * offset map this build cannot believe, rather than silently discarding it. + */ + @Test + void failPolicyRejectsAMapClaimingOffsetsThePartitionDoesNotHold() { + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + assertThatThrownBy(() -> wm.registerWork(batch(wm, + batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.of(PARTITION_END_OFFSET)))) + .as("FAIL must not silently accept an offset map claiming offsets the partition cannot hold") + .isInstanceOf(CorruptOffsetMetadataException.class) + .hasMessageContaining(TP.toString()); + } + + /** + * The guard must not reject a real offset map, and a long run of completed offsets is a real shape: one record + * stuck at the committed offset while every offset above it succeeds is exactly what run-length encoding is for, + * and PC's back-pressure does not stop it - the payload stays three entries wide however far the partition runs + * ahead. + *

+ * Pinned at the boundary: the map ends on the last offset the partition holds, the largest claim that is still + * honest. + */ + @Test + void aMapEndingOnTheLastOffsetThePartitionHoldsIsKept() { + long highestOffsetHeld = PARTITION_END_OFFSET - 1; + var module = moduleWithCommittedMetadata(mapClaimingUpTo(highestOffsetHeld), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + assertThatCode(() -> wm.registerWork(batch(wm, + batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.of(PARTITION_END_OFFSET)))) + .as("a map ending on the last offset the partition holds is honest, and must be kept") + .doesNotThrowAnyException(); + + PartitionState state = wm.getPm().getPartitionState(TP); + assertThat(state.getOffsetHighestSucceeded()) + .as("the map was kept, so it still says everything up to the last offset held succeeded") + .isEqualTo(highestOffsetHeld); + assertThat(state.isRecordPreviouslyCompleted(recordAt(LAST_RECORD_OF_BATCH))) + .as("a kept map still answers about the records below its claim - that is the behaviour a false " + + "rejection would destroy") + .isTrue(); + } + + /** + * One offset further than the partition can hold, and nothing else changed - the control arm for the test above. + */ + @Test + void aMapClaimingOneOffsetPastTheLastOneHeldIsRefused() { + var module = moduleWithCommittedMetadata(mapClaimingUpTo(PARTITION_END_OFFSET), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + assertThatThrownBy(() -> wm.registerWork(batch(wm, + batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.of(PARTITION_END_OFFSET)))) + .as("the first offset the partition does not hold is the first one a map may not claim") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * When the consumer cannot say where the partition ends without blocking, the check has no ground truth and must + * not invent one - and must not give up either: it looks again at the next batch. + */ + @Test + void anUnknownLogEndOffsetDefersTheCheckToTheNextBatch() { + var module = moduleWithCommittedMetadata(runLengthV2Metadata(1, Integer.MAX_VALUE - 1), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + assertThatCode(() -> wm.registerWork(batch(wm, batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.empty()))) + .as("a check with no ground truth must stand down, not reject") + .doesNotThrowAnyException(); + + assertThatThrownBy(() -> wm.registerWork(batch(wm, + batchOfRecords(LAST_RECORD_OF_BATCH + 1, LAST_RECORD_OF_BATCH + 2), + OptionalLong.of(PARTITION_END_OFFSET)))) + .as("standing down is deferral, not acceptance - the next batch that can answer must still refuse it") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * A record that arrived certainly exists, so a batch reaching the map's claim corroborates it without any + * watermark at all - and having been corroborated, the claim is never questioned again. + */ + @Test + void aBatchReachingTheClaimCorroboratesItWithoutAWatermark() { + long modestClaim = FIRST_RECORD_OF_BATCH; // inside the batch below, so the records themselves prove it + var module = moduleWithCommittedMetadata(mapClaimingUpTo(modestClaim), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + wm.registerWork(batch(wm, batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), OptionalLong.empty())); + + assertThatCode(() -> wm.registerWork(batch(wm, + batchOfRecords(LAST_RECORD_OF_BATCH + 1, LAST_RECORD_OF_BATCH + 2), + OptionalLong.of(modestClaim)))) // would refuse the claim, had the records not already proven it + .as("a claim the records themselves proved is settled, and must not be re-litigated") + .doesNotThrowAnyException(); + } + + /** + * The check reads the decoded claim, not the bytes, so it covers every encoding at once - here the bitset, whose + * declared length astubbs#207 already forces to be backed by bytes that are present. Bounded is not true: a full + * metadata field still buys tens of thousands of skipped records. + */ + @Test + void aBitSetMapClaimingOffsetsThePartitionDoesNotHoldIsRefused() { + var module = moduleWithCommittedMetadata(bitSetV2Metadata(300), // 100 + 300 - 1, well past offset 199 + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + assertThatThrownBy(() -> wm.registerWork(batch(wm, + batchOfRecords(FIRST_RECORD_OF_BATCH, LAST_RECORD_OF_BATCH), + OptionalLong.of(PARTITION_END_OFFSET)))) + .as("a bitset claiming offsets the partition does not hold is as unreadable as a run length doing it") + .isInstanceOf(CorruptOffsetMetadataException.class); + } + + /** + * The bitset control arm at the boundary: a map covering every offset from the committed one to the last the + * partition holds is the widest honest bitset there is. + */ + @Test + void aBitSetEndingOnTheLastOffsetHeldIsKept() { + int bitsToTheEndOfThePartition = (int) (PARTITION_END_OFFSET - COMMITTED_OFFSET); + var module = moduleWithCommittedMetadata(bitSetV2Metadata(bitsToTheEndOfThePartition), + InvalidOffsetMetadataHandlingPolicy.FAIL); + WorkManager wm = assignedWorkManager(module); + + // This map has no incomplete offsets at all, so PC expects its next record at PARTITION_END_OFFSET - the + // batch has to start there, or the pre-existing bootstrap truncation resets the state for reasons that have + // nothing to do with this check. + assertThatCode(() -> wm.registerWork(batch(wm, + batchOfRecords(PARTITION_END_OFFSET, PARTITION_END_OFFSET + 10), + OptionalLong.of(PARTITION_END_OFFSET + 11)))) + .as("a bitset ending exactly on the last offset the partition held is honest") + .doesNotThrowAnyException(); + + assertThat(wm.getPm().getPartitionState(TP).getOffsetHighestSucceeded()) + .as("the whole declared range was kept") + .isEqualTo(PARTITION_END_OFFSET - 1); + } + + private static ConsumerRecord recordAt(long offset) { + return new ConsumerRecord<>(TP.topic(), TP.partition(), offset, "key", "value"); + } +} From 8148b1b027272d275a4f82faa00402f98d3ccef0 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 15:58:58 +1200 Subject: [PATCH 5/8] docs(inflight-tool): the vet sample output names a note astubbs#480 retires 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 --- docs/inflight-tool.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/inflight-tool.md b/docs/inflight-tool.md index 033e70dce..f2d2a19da 100644 --- a/docs/inflight-tool.md +++ b/docs/inflight-tool.md @@ -331,6 +331,7 @@ nearly every file. Each row carries the cheap signals a script can see: all-cited-numbers-settled: cites one fork number and it is settled: astubbs#207 MERGED (pull-request) bin/inflight.mjs docs show docs/inflight/bug-run-length-plausibility-ceiling.md ``` + **A signal is a reason to open the note, never a verdict.** That note may cite astubbs#207 because that pull request is where the defect was found, or because it fixed it - the row cannot tell, and From af20b0885b1abf384c587b341d0333a4ab6e9868 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 16:23:42 +1200 Subject: [PATCH 6/8] test(offsets): one shared builder for a module with committed metadata, not two copies The duplicate-code check on astubbs/parallel-consumer#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 --- ...ForeignOffsetMetadataOnAssignmentTest.java | 34 +++---------- .../ImplausibleOffsetMapOnFirstBatchTest.java | 31 +++-------- .../offsets/OffsetCodecTestUtils.java | 51 +++++++++++++++++++ 3 files changed, 65 insertions(+), 51 deletions(-) diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ForeignOffsetMetadataOnAssignmentTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ForeignOffsetMetadataOnAssignmentTest.java index cd54ef61f..5643c0743 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ForeignOffsetMetadataOnAssignmentTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ForeignOffsetMetadataOnAssignmentTest.java @@ -8,16 +8,11 @@ import bz.stub.parallelconsumer.internal.PCModuleTestEnv; import bz.stub.parallelconsumer.state.WorkManager; import lombok.extern.slf4j.Slf4j; -import org.apache.kafka.clients.consumer.MockConsumer; -import org.apache.kafka.clients.consumer.OffsetAndMetadata; -import org.apache.kafka.clients.consumer.OffsetResetStrategy; import org.apache.kafka.common.TopicPartition; import org.junit.jupiter.api.Test; import pl.tlinkowski.unij.api.UniLists; -import pl.tlinkowski.unij.api.UniMaps; import java.util.Base64; -import java.util.function.UnaryOperator; import static bz.stub.parallelconsumer.offsets.OffsetCodecTestUtils.magicByteOfAnEncodingThatDoesNotExistYet; import static org.assertj.core.api.Assertions.assertThat; @@ -47,34 +42,19 @@ class ForeignOffsetMetadataOnAssignmentTest { static final long COMMITTED_OFFSET = 100L; - MockConsumer mockConsumer; - - /** - * Builds a PC module whose consumer already has {@code metadata} committed against {@link #TP}, as if a previous - * owner of this consumer group had left it behind. - */ - private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, - ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy policy) { - return moduleWithCommittedMetadata(metadata, builder -> builder.invalidOffsetMetadataPolicy(policy)); - } - /** - * The same, but leaving {@link ParallelConsumerOptions#getInvalidOffsetMetadataPolicy()} unset - which is the - * configuration the astubbs#118 reporter actually ran, and so the one the regression must be pinned under. + * A module whose consumer has {@code metadata} committed against {@link #TP} - the shared setup in + * {@link OffsetCodecTestUtils}, which owns it because {@code ImplausibleOffsetMapOnFirstBatchTest} needs exactly + * the same thing. Leaving the policy unset is not incidental here: it is the configuration the astubbs#118 + * reporter actually ran, and so the one the regression must be pinned under. */ private PCModuleTestEnv moduleWithCommittedMetadata(String metadata) { - return moduleWithCommittedMetadata(metadata, builder -> builder); + return OffsetCodecTestUtils.moduleWithCommittedMetadata(TP, COMMITTED_OFFSET, metadata); } private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, - UnaryOperator> configure) { - mockConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); - mockConsumer.assign(UniLists.of(TP)); - mockConsumer.commitSync(UniMaps.of(TP, new OffsetAndMetadata(COMMITTED_OFFSET, metadata))); - - var options = configure.apply(ParallelConsumerOptions.builder() - .consumer(mockConsumer)).build(); - return new PCModuleTestEnv(options); + ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy policy) { + return OffsetCodecTestUtils.moduleWithCommittedMetadata(TP, COMMITTED_OFFSET, metadata, policy); } /** diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java index b54e36df9..462bd1483 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/ImplausibleOffsetMapOnFirstBatchTest.java @@ -4,7 +4,6 @@ * Copyright (C) 2026 Antony Stubbs and contributors */ -import bz.stub.parallelconsumer.ParallelConsumerOptions; import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; import bz.stub.parallelconsumer.internal.EpochAndRecordsMap; import bz.stub.parallelconsumer.internal.PCModuleTestEnv; @@ -13,9 +12,6 @@ import lombok.extern.slf4j.Slf4j; import org.apache.kafka.clients.consumer.ConsumerRecord; import org.apache.kafka.clients.consumer.ConsumerRecords; -import org.apache.kafka.clients.consumer.MockConsumer; -import org.apache.kafka.clients.consumer.OffsetAndMetadata; -import org.apache.kafka.clients.consumer.OffsetResetStrategy; import org.apache.kafka.common.TopicPartition; import org.junit.jupiter.api.Test; import pl.tlinkowski.unij.api.UniLists; @@ -26,7 +22,6 @@ import java.util.Base64; import java.util.List; import java.util.OptionalLong; -import java.util.function.UnaryOperator; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; @@ -71,8 +66,6 @@ class ImplausibleOffsetMapOnFirstBatchTest { static final int RECORDS_IN_BATCH = (int) (LAST_RECORD_OF_BATCH - FIRST_RECORD_OF_BATCH + 1); - MockConsumer mockConsumer; - /** * Base64 of a {@link OffsetEncoding#RunLengthV2} payload with the given runs, the first of which is a run of * incomplete offsets (the decoder starts there, because the committed offset is the lowest incomplete @@ -111,27 +104,17 @@ private static String mapClaimingUpTo(long highestSeenOffset) { return runLengthV2Metadata(1, (int) (highestSeenOffset - COMMITTED_OFFSET)); } + /** + * A module whose consumer has {@code metadata} committed against {@link #TP}, as a previous owner of the consumer + * group would have left it - {@link OffsetCodecTestUtils} owns the setup, shared with + * {@link ForeignOffsetMetadataOnAssignmentTest}. + */ private PCModuleTestEnv moduleWithCommittedMetadata(String metadata) { - return moduleWithCommittedMetadata(metadata, UnaryOperator.identity()); + return OffsetCodecTestUtils.moduleWithCommittedMetadata(TP, COMMITTED_OFFSET, metadata); } private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, InvalidOffsetMetadataHandlingPolicy policy) { - return moduleWithCommittedMetadata(metadata, builder -> builder.invalidOffsetMetadataPolicy(policy)); - } - - /** - * Builds a module whose consumer has {@code metadata} committed against {@link #TP}, as a previous owner of the - * consumer group would have left it. - */ - private PCModuleTestEnv moduleWithCommittedMetadata(String metadata, - UnaryOperator> configure) { - mockConsumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); - mockConsumer.assign(UniLists.of(TP)); - mockConsumer.commitSync(UniMaps.of(TP, new OffsetAndMetadata(COMMITTED_OFFSET, metadata))); - - var options = configure.apply(ParallelConsumerOptions.builder() - .consumer(mockConsumer)).build(); - return new PCModuleTestEnv(options); + return OffsetCodecTestUtils.moduleWithCommittedMetadata(TP, COMMITTED_OFFSET, metadata, policy); } private static List> batchOfRecords(long fromInclusive, long toInclusive) { diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/OffsetCodecTestUtils.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/OffsetCodecTestUtils.java index ddfbc992c..e364a68f3 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/OffsetCodecTestUtils.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/OffsetCodecTestUtils.java @@ -5,13 +5,22 @@ * Modifications Copyright (C) 2026 Antony Stubbs and contributors */ +import bz.stub.parallelconsumer.ParallelConsumerOptions; +import bz.stub.parallelconsumer.internal.PCModuleTestEnv; import bz.stub.parallelconsumer.state.PartitionState; import lombok.experimental.UtilityClass; import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.MockConsumer; +import org.apache.kafka.clients.consumer.OffsetAndMetadata; +import org.apache.kafka.clients.consumer.OffsetResetStrategy; +import org.apache.kafka.common.TopicPartition; +import pl.tlinkowski.unij.api.UniLists; +import pl.tlinkowski.unij.api.UniMaps; import java.util.Arrays; import java.util.Set; import java.util.TreeSet; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import static bz.stub.parallelconsumer.internal.utils.Range.range; @@ -87,4 +96,46 @@ static byte magicByteOfAnEncodingThatDoesNotExistYet() { throw new IllegalStateException("Every possible magic byte is claimed by an encoding - the wire format is full"); } + /** + * A module whose consumer already has {@code metadata} committed against {@code tp}, as a previous owner of the + * consumer group would have left it behind - the setup every offset-metadata regression test needs, and the one + * two of them had a copy of each. + *

+ * The consumer is owned here rather than handed back: neither caller does anything with it after construction, + * and a test that needs to reach it can read {@code module.options().getConsumer()}. + * + * @param committedOffset the offset the metadata is committed against - what the payload is decoded relative to + */ + static PCModuleTestEnv moduleWithCommittedMetadata(TopicPartition tp, long committedOffset, String metadata) { + return moduleWithCommittedMetadata(tp, committedOffset, metadata, UnaryOperator.identity()); + } + + /** + * The same, with {@link ParallelConsumerOptions#getInvalidOffsetMetadataPolicy()} set explicitly. Leaving it unset + * is not the same test: the default is what a reporter actually runs, so a regression has to be pinned under it. + */ + static PCModuleTestEnv moduleWithCommittedMetadata(TopicPartition tp, + long committedOffset, + String metadata, + ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy policy) { + return moduleWithCommittedMetadata(tp, committedOffset, metadata, + builder -> builder.invalidOffsetMetadataPolicy(policy)); + } + + /** + * @param configure anything else the test needs on the options builder + */ + static PCModuleTestEnv moduleWithCommittedMetadata(TopicPartition tp, + long committedOffset, + String metadata, + UnaryOperator> configure) { + var mockConsumer = new MockConsumer(OffsetResetStrategy.EARLIEST); + mockConsumer.assign(UniLists.of(tp)); + mockConsumer.commitSync(UniMaps.of(tp, new OffsetAndMetadata(committedOffset, metadata))); + + var options = configure.apply(ParallelConsumerOptions.builder() + .consumer(mockConsumer)).build(); + return new PCModuleTestEnv(options); + } + } From 97f1cd327d18621979fcfe1f9a09d992bdc9c59d Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 16:49:30 +1200 Subject: [PATCH 7/8] docs(features): the metadata policy's boundary describes the batch check, not the assignment one Review finding on astubbs/parallel-consumer#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 --- docs/features/invalid-offset-metadata-policy.yaml | 4 ++-- .../offsets/EncoderOutputSurvivesDecodeValidationTest.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/features/invalid-offset-metadata-policy.yaml b/docs/features/invalid-offset-metadata-policy.yaml index 973796b7e..7e7252cea 100644 --- a/docs/features/invalid-offset-metadata-policy.yaml +++ b/docs/features/invalid-offset-metadata-policy.yaml @@ -28,8 +28,8 @@ opt_in: isolated_from_stable_modules: false boundaries: - The policy governs every payload this build cannot read - a magic byte no encoding claims, an encoding with no decoder here, metadata recognized as Kafka Streams', and a body that is not something any encoder here could have produced. - - A decoded map claiming offsets the partition does not hold is governed too: the end offsets of the partitions carrying metadata are read once per assignment, and a map reaching past the last offset a partition holds is treated as unreadable. - - When those end offsets cannot be read - an unavailable leader, a timeout - the check stands down for that assignment and logs it, rather than discarding maps it cannot judge. + - A decoded map claiming offsets the partition does not hold is governed too: the first batch of records for a partition carries where its log ended when it was fetched, and a map reaching past the last offset the partition holds is treated as unreadable before any record is checked against it. + - No broker request is made for this: the watermark comes from the fetch that delivered the records. Until one has, the check defers to the next batch rather than judging a map it has no ground truth for. - It still does not govern a payload that decodes into a map which is merely wrong about offsets that do exist; nothing in such a payload proves it wrong. - Under IGNORE the offset map is discarded, not repaired, so records completed but not yet committed before that point are replayed. references: diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java index ed0870aa3..0027ec446 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/offsets/EncoderOutputSurvivesDecodeValidationTest.java @@ -5,8 +5,8 @@ */ import bz.stub.parallelconsumer.ParallelConsumerOptions.InvalidOffsetMetadataHandlingPolicy; -import lombok.extern.slf4j.Slf4j; import bz.stub.parallelconsumer.state.PartitionState; +import lombok.extern.slf4j.Slf4j; import org.apache.kafka.common.TopicPartition; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; From acf29b6d8ee16b211a216c3feed45b0fae7d02b5 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 8 Sep 2026 17:11:36 +1200 Subject: [PATCH 8/8] test(offsets): the log-end helper takes the Consumer interface, not the 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 --- .../internal/ConsumerManagerLogEndOffsetTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java index 1cbdf799c..3df3e86e0 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/internal/ConsumerManagerLogEndOffsetTest.java @@ -5,6 +5,7 @@ */ import lombok.extern.slf4j.Slf4j; +import org.apache.kafka.clients.consumer.Consumer; import org.apache.kafka.clients.consumer.MockConsumer; import org.apache.kafka.clients.consumer.OffsetResetStrategy; import org.apache.kafka.common.TopicPartition; @@ -41,8 +42,8 @@ class ConsumerManagerLogEndOffsetTest { static final Duration TIMEOUT = Duration.ofSeconds(1); - private ConsumerManager managerFor(MockConsumer mockConsumer) { - return new ConsumerManager<>(new ThreadConfinedConsumer<>(mockConsumer), TIMEOUT, TIMEOUT, TIMEOUT); + private ConsumerManager managerFor(Consumer consumer) { + return new ConsumerManager<>(new ThreadConfinedConsumer<>(consumer), TIMEOUT, TIMEOUT, TIMEOUT); } /**