proxy: gate /readyz on a real Fetch round-trip (BUG-0026) - #14
Open
kamir wants to merge 6 commits into
Open
Conversation
AppendBatch returns an AppendResult (the produce ACK basis) as soon as the batch is buffered, but flush-to-segment only happens when a WriteBuffer threshold trips, and flushing is evaluated only inside AppendBatch (no background flusher). Read serves flushed segments only and returns ErrOffsetOutOfRange for buffered offsets. So a just-acked record whose partition then goes quiet stays unreadable (and is lost on broker restart, since the buffer is in-memory), violating Kafka read-after-ack. This test appends 10 batches with flush thresholds set so nothing flushes, then asserts every acked offset is readable. It FAILS on the current code (offset 0 -> ErrOffsetOutOfRange) and must pass once Read serves the buffer (or produce flushes before acking under acks=all). Existing tests use MaxBytes:1 so every append flushes immediately, which is why this path was never exercised. Refs: scalytics UPSTREAM/2026-06-04-kafscale-consume-readpath.md
…sumable PartitionLog.Read served only flushed segments and returned ErrOffsetOutOfRange for any offset still in the in-memory WriteBuffer. Because flush is append-triggered (ShouldFlush is evaluated only inside AppendBatch; there is no background flusher), a partition that goes quiet below the flush threshold keeps its just-acked tail in the buffer, where it was unreadable — breaking Kafka's read-after-ack contract (observed end-to-end as 1015 acked -> 588 readable on v1.6.0). Read now falls back to the buffer when the offset is not in a flushed segment: new WriteBuffer.RecordsFrom(offset, maxBytes) returns the buffered batch bytes for the requested offset onward, non-destructively. The fetch handler (cmd/broker fetch -> plog.Read) picks this up unchanged. Makes TestPartitionLogReadAfterAckBeforeFlush pass; full pkg/storage, pkg/broker and cmd/broker suites stay green. Note: this fixes READABILITY (read-after-ack). Durability-on-restart is separate — the buffer is still in-memory, so acked-but-unflushed records are lost if the broker restarts before flush. A complete acks=all guarantee additionally needs flush-before-ack or a WAL; tracked separately. Refs: scalytics UPSTREAM/2026-06-04-kafscale-consume-readpath.md
…ross rotations Appends 30 batches with MaxBatches=3 (frequent flush rotations) and asserts every acked offset stays readable. Passes over MemoryS3, isolating the end-to-end '1019 acked -> ~32 readable' loss OUT of the pkg/storage state machine (no segment overwrite, no drop across rotations). The live loss is therefore in the real S3 client / proxy fetch-forward / concurrency, not the storage logic.
checkReady was shallow — Ready as soon as a backend was *known*. But the proxy is a full Fetch codec (decode/merge/re-encode), so a proxy<->broker v13 Fetch (de)serialization or connection-state mismatch — the post-uncoordinated-reboot state in BUG-0026 — returns EOF/short frames the proxy can't decode while metadata/ListOffsets stay fine. The shallow check passed anyway and the proxy served traffic that silently returned 0 records. Add fetchProbe: dial a backend (fresh conn, so a stale/half-open connection is caught too), send a minimal v13 Fetch (no topics), require the response decodes. checkReady now gates on it (haveBackend + fetchProbe). Result: the proxy stays NotReady until the broker actually serves Fetch, enforcing coordinated startup ordering on a reboot. Toggle via KAFSCALE_PROXY_READYZ_FETCH_PROBE (default on). Built on fix/broker-ack-but-lost-v1.6.0 (= v1.6.0 + read-after-ack) so a matched broker+proxy pair built from this branch carries both fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mpty topics The empty-topics Fetch made the broker close the connection (read frame size: EOF), wedging readiness permanently (observed live on er1-bdr: new proxy NotReady while the broker served the old proxy fine). v13 Fetch keys by topic ID; send one partition under a non-zero unknown TopicID -> broker returns a decodable UNKNOWN_TOPIC_ID response, round-tripping the full v13 codec (the BUG-0026 path) without needing a real topic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…chResponse Live on er1-bdr the probe still got 'read frame size: EOF'; broker logged 'parse request: decode Produce v112 (payload bytes=116)'. Root cause: kmsg AppendRequest already prepends the 4-byte length, and forwardToBackend's WriteFrame prepends another -> double-framed request; the broker read the inner length bytes as api key/version (112 = body length) and closed. The real single-backend path never hits this because it forwards the original client payload (length already stripped by ReadFrame); only the re-encode path (multi-backend fanout) builds via encodeFetchRequest -- which is a latent double-frame bug there too, noted for upstream. Fix the probe: strip the inner length (framed[4:]) so WriteFrame adds exactly one, and decode the reply via parseFetchResponse (the same SkipResponseHeader + ReadFrom path the proxy uses for real fetches; also yields the canonical BUG-0026 'decode fetch response' error when the broker is in the broken state). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
novatechflow
force-pushed
the
fix/broker-ack-but-lost-v1.6.0
branch
from
July 7, 2026 08:33
b35ebe6 to
5d2e21f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
/readyz(checkReady) was shallow — Ready as soon as a backend was known. But the proxy is a full Fetch codec (decode/merge/re-encode), so a proxy↔broker v13 Fetch (de)serialization or connection-state mismatch — the post-uncoordinated-reboot state in BUG-0026 — returns EOF/short frames the proxy can't decode while metadata/ListOffsets stay fine. The shallow check passed anyway and the proxy served traffic that silently returned 0 records.Change
Add
fetchProbe: dial a backend (fresh conn, so a stale/half-open connection is caught too), send a minimal v13 Fetch for a dummy unknown-topic partition, require the response decodes via the proxy's ownparseFetchResponse.checkReadynow gates on it (haveBackend+fetchProbe). The proxy stays NotReady until the broker actually serves Fetch, enforcing coordinated startup ordering on a reboot. Toggle viaKAFSCALE_PROXY_READYZ_FETCH_PROBE(default on).Live-verified (er1-bdr, broker v1.6.0-readfix-based + this proxy)
Note (separate latent bug, NOT fixed here)
encodeFetchRequest(kmsg AppendRequest) already prepends the 4-byte length;forwardToBackend's WriteFrame prepends another. The single-backend path forwards the original client payload (length already stripped) so it never hits this, but the multi-backendfanOutFetchre-encode path would send a double-framed request. Worth a follow-up on a multi-broker topology.🤖 Generated with Claude Code