Skip to content

fix: decode SSE events without line-length limit and surface transport errors - #15

Merged
jiachengxu merged 3 commits into
mainfrom
fix-sse-decoder-line-limit
Jul 23, 2026
Merged

fix: decode SSE events without line-length limit and surface transport errors#15
jiachengxu merged 3 commits into
mainfrom
fix-sse-decoder-line-limit

Conversation

@jiachengxu

Copy link
Copy Markdown
Member

This PR fixes two silent-truncation defects in DoStreamingRequest's SSE decoding path:

  1. Events larger than 64KiB were silently dropped. The alevinval/sse decoder reads
    lines with a default bufio.Scanner, which caps a line at bufio.MaxScanTokenSize
    (64KiB). Each streamed message arrives as a single data: {json} line, so any message
    whose JSON exceeds ~64KiB made Scan() fail with bufio.ErrTooLong.
  2. Every scanner failure was flattened into io.EOF. The decoder's read loop never
    checks scanner.Err(); on any failure — the 64KiB overflow above, or a transport error
    (connection reset) mid-stream — it returns io.EOF, which DoStreamingRequest treats
    as successful completion (close(resCh)).

Combined effect: a stream carrying one oversized message, or a stream cut mid-flight,
ends exit-0 with partial data. The caller cannot distinguish it from a complete,
successful stream. #14 fixed the case where the server terminates the stream with an
{"error": ...} event; these two failures happen below that layer, on the client's own
read path, so no error event is ever seen.

How

Replace the alevinval/sse decoder with a small internal bufio.Reader-based SSE event
decoder (sseEventDecoder in pkg/grpc/gateway/request.go):

  • No line-length limitReadString('\n') grows as needed.
  • Errors propagate — read errors surface on errCh instead of closing resCh.
  • Truncation is never success — an EOF that interrupts a partially-read event returns
    io.ErrUnexpectedEOF; only an EOF at a clean event boundary is a normal end-of-stream.
  • SSE framing behavior is preserved: multiple data: lines of one event join with \n,
    a single leading space after the colon is trimmed, comment/heartbeat lines (: ...)
    and non-data fields are ignored, and both \n and \r\n line endings are accepted
    (the alevinval decoder handled these the same way).

Scope: only the SSE decode path used by server-streaming RPC responses. DoRequest
(unary), doHTTPStreamingRequest (google.api.HttpBody streams, which use io.Copy),
and wrapStreamingResponseError are untouched. alevinval/sse remains a test-only
dependency (marshaller_test.go uses it to decode the marshaller's output).

Testing

New regression tests in pkg/grpc/gateway/request_test.go:

Test Covers
TestDoStreamingRequest_LargeEvents 3 × 300KiB messages through the real gRPC → grpc-gateway → SSE pipeline; each must arrive intact
TestDoStreamingRequest_TruncatedStreamSurfacesError connection cut mid-event: the already-complete first event is delivered, then an error (never a clean close)
TestDoStreamingRequest_SSEFraming comment/heartbeat lines, CRLF endings, one event split across multiple data: lines

All three fail against the previous decoder and pass with the fix:

…t errors

The alevinval/sse decoder capped lines at bufio.MaxScanTokenSize (64KiB) and
returned io.EOF for every scanner failure. A streamed message whose SSE data
line exceeded 64KiB - or any transport error mid-stream - silently ended the
stream as if it had completed, handing the caller partial data with no error.

Replace it with an internal bufio.Reader-based event decoder: no line-length
limit, read errors propagate, and an EOF that interrupts a partially-read
event surfaces as io.ErrUnexpectedEOF instead of a clean close.
The SSE grammar permits LF, CRLF, or bare CR as end-of-line. The previous
ReadString('\n')-based line reader only recognized LF, so a CR-only stream
was accumulated as one growing line: no events were delivered while the
connection was open, and closure surfaced as io.ErrUnexpectedEOF. Replace
it with a CR-aware readLine that consumes all three terminators, matching
the alevinval/sse decoder this branch replaced.
Peek(1) after a bare CR blocks on the underlying connection when the CR is
the last byte the server has flushed, deferring event delivery until the
next event or EOF. Track a pending CR on the decoder and swallow a leading
LF on the next read instead, matching the WHATWG SSE parsing algorithm.
The CR-only regression test now flushes one event and holds the connection
open until it is received.

@yhxlele yhxlele left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jiachengxu
jiachengxu merged commit ba27dc2 into main Jul 23, 2026
5 checks passed
@jiachengxu
jiachengxu deleted the fix-sse-decoder-line-limit branch July 23, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants