fix: surface streaming errors faithfully (mid-stream on errCh + pre-first-message status) - #14
Merged
Merged
Conversation
jiachengxu
approved these changes
Jul 7, 2026
… surface their real status
jiachengxu
added a commit
that referenced
this pull request
Jul 23, 2026
…t errors (#15) 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 limit** — `ReadString('\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:
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
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.
Two related fixes so streaming errors surface faithfully instead of being lost.
Both are in
DoStreamingRequest's error handling; neither requires ago.modchange (
codes/statuscome from the already-requiredgoogle.golang.org/grpc),and the full suite passes under the repo's CI Go 1.20 toolchain.
1. Mid-stream errors are swallowed (returned as silent success)
DoStreamingRequest's decode loop only looks for theresultkey:grpc-gateway frames a server-streaming RPC that fails after emitting one or
more results as a terminal
data: {"error": <google.rpc.Status>}event. Thatevent has no
resultkey, so the loopcontinues past it, the nextDecode()returns
io.EOF, and the goroutineclose(resCh)s — so the caller sees a clean,successful end-of-stream and silently receives a truncated result. The
streamingResponseErrorKeyconstant was defined but never consulted in the loop.Fix: check for the
errorkey before theresultkey; if present, unmarshalthe
google.rpc.Statusand deliver it onerrCh.resChis intentionally leftun-closed on this path so a consumer's
selectdeterministically observes theerror rather than an EOF.
2. Pre-first-message errors collapse to
codes.UnknownWhen a streaming handler fails before its first message, grpc-gateway sets the
HTTP error status and writes the error through the SSE marshaller, so the body is
framed as
data: {"error": ...}.wrapStreamingResponseErrorcalledjson.Unmarshalon the raw body, which fails on thedata:prefix and returns ageneric error — so the real gRPC status (
NotFound,PermissionDenied, …)collapses to
codes.Unknown.Fix: strip the SSE
data:framing before parsing. This is a no-op fornon-streaming (plain JSON) error bodies such as routing 404s, which start with
{rather thandata:.Tests
TestDoStreamingRequest_ErrorAfterResults—result, result, error: asserts bothresults are delivered and then the
Internalstatus arrives onerrCh(not aclean EOF).
TestDoStreamingRequest_ErrorBeforeResults— SSE-framed pre-first-message error:asserts the returned error carries the real
NotFoundstatus, notUnknown.Both were verified RED before their respective fix and GREEN after. The test
service's
TrackInvitationgained two id-keyed hooks (fail-after-events,fail-before-events); the existing streaming test is unchanged.Compatibility
errChwas already the loop's channel for decode/unmarshal errors, so callersthat drain it (the canonical
select { case <-resCh; case <-errCh }) simply nowreceive the real error instead of a truncated success. Fix (2) only changes the
initial error for SSE-framed bodies (
Unknown→ real code); it is a no-op for theplain-JSON routing-404 case. No new contract for callers.