fix(#3996): drop gateway SSE keepalive frames before the genai parser - #4022
fix(#3996): drop gateway SSE keepalive frames before the genai parser#4022aheritier wants to merge 2 commits into
Conversation
30bb441 to
01ecbc9
Compare
01ecbc9 to
c3771e3
Compare
c3771e3 to
e69c90a
Compare
e69c90a to
c064d65
Compare
2aeb274 to
c064d65
Compare
Exhaustively cover wantsImageResponseModalities (added by the preceding compatibility-guard commit): true only on supported Gemini surfaces, only when output_capabilities.image resolves true (explicit override taking precedence over models.dev output modalities), and only outside title-generation/compaction utility calls. Also pin catalogue resolution, override precedence, request construction, and the existing request-shape diagnostics.
During long generations (e.g. Gemini image output) the Docker AI Gateway
emits `event: keepalive` + `data: {}` frames. google.golang.org/genai
treats any `event:` line as a fatal invalid stream chunk, so a single
keepalive killed the whole stream.
Add an opt-in httpclient.WithSSEKeepaliveFilter that makes the shared SSE
filter transport also drop payload-free keepalive frames, and enable it
only on the Gemini gateway HTTP client. The shared default path is
unchanged: Anthropic-style named events with meaningful data still pass
through verbatim, and only the gateway surface — never direct
Gemini/Vertex — gets the new behavior.
c064d65 to
a4ac3c3
Compare
aheritier
left a comment
There was a problem hiding this comment.
Reviewed the three-dot delta of this PR only (head a4ac3c343). CI is green for the head SHA (13 check-runs, 0 failures). Since this is a draft, this is a review comment rather than an approval.
The transport fix itself is right and well pinned. pkg/httpclient/sse_filter.go is byte-identical to the version I reviewed on 2026-07-30, and I re-verified the constraint it exists for: google.golang.org/genai@v1.70.0 iterateResponseStream (api_client.go:511-529) fails the whole stream with invalid stream chunk: <prefix>:<data> for any non-blank line whose field is not data, so an event: keepalive line is fatal. Verified locally on a throwaway checkout: go build ./..., go test -race on pkg/httpclient + pkg/model/provider/gemini, golangci-lint (0 issues). Also verified by probe that keepalive dropping is correct across fragmented Read boundaries (1/2/3/7/13/26-byte chunks) and with CRLF framing, that the opt-in is gateway-only (pkg/model/provider/gemini/client.go:186), and that non-SSE bodies — including application/json 400/500 error payloads — pass through byte-identical, so the #4017 diagnostics classifier still sees the original bytes. Mutation-testing the suite (go test -overlay) killed the mutants that matter: dropping the hasMeaningfulData guard, treating {} as meaningful, removing the opt-in gate, and removing the gateway wiring all fail tests.
[blocking] pkg/model/provider/gemini/image_response_modalities_test.go asserts a "declared-only" contract the code does not implement. The docstring (:23-26) claims the suite "exhaustively covers … only for a model explicitly declared image-output-capable via output_capabilities.image", and cases named direct Gemini API, declaration missing: never (:44) and gateway, declaration missing: absent (:243-249) claim missing-declaration means no image output. But production resolves the flag through base.Config.ImageOutputEnabled → modelinfo.ResolveOutputImage (pkg/modelinfo/modelinfo.go:681-702), which only returns false for a missing override when the models.dev store is nil/unreachable. TestWantsImageResponseModalities bypasses that resolution entirely (it passes the boolean in from the declaration), and the streaming cases pass no WithModelsDevStore, so they are vacuous: with a populated catalogue entry for gemini-2.5-flash-image the very cases named "never/absent" would flip to requesting TEXT+IMAGE. The commit message of a7d2d76a3 additionally claims it pins "catalogue resolution, override precedence", but the file references no models.dev store at all. This is not a re-report of the inference-vs-declaration behaviour raised on #4019 — the new defect is that this PR adds the artifact that makes that behaviour look pinned and epic AC ("models/routes without that declaration … remain text-only") look satisfied. Fix: rename/re-scope these cases to what they actually assert, and add at least one case with a populated WithModelsDevStore demonstrating the real resolution.
[should-fix] The keepalive matcher is keyed to one literal frame shape, with no recorded evidence of what the gateway actually emits. pkg/httpclient/sse_filter.go:120-126 drops a frame only when the event name is exactly keepalive and every data: payload is empty or {}. On this opt-in path any event: line is fatal to genai, so a payload-free event: ping / event: heartbeat — or a keepalive that ever gains a payload — silently returns to the original stream-killing bug. Nothing pins the choice either: mutating the name check to a keep prefix, to "any non-empty event name", or to a leading-space-tolerant matcher all leave the suite green. Two asks: (1) cite the evidence for the frame shape (captured stream / gateway version / upstream issue) in the comment at pkg/httpclient/client.go:197-204, and state that the durable remedy is gateway-side (or genai-side) with this as a scoped workaround; (2) consider dropping any payload-free named event on this gateway-only path, and add a test that pins whichever rule you choose.
[should-fix] Scope: the delta carries a 405-line test file for a different PR's feature. Commit a7d2d76a3 adds image_response_modalities_test.go, which tests wantsImageResponseModalities — code introduced by the preceding guard commit (#4020), as its own message says. Per AGENTS.md ("Make commits logical and atomic … avoid mixing unrelated changes") those tests belong with the code they pin, where they would also have exercised that PR's review findings; the PR body here describes only the SSE filter and does not mention them.
[optional] Pre-existing (not this delta), noted for context: an error body served with Content-Type: text/event-stream is swallowed entirely by the filter (probe: a 400 with {"error":{...}} arrives as an empty body) — harmless for the JSON error shape I verified, but worth confirming the gateway never returns event-stream-typed errors on the streaming route; and pending grows without bound while a frame has no terminating blank line (probe: 2.8 MB buffered, 0 bytes emitted, no error). Both predate this PR (pkg/httpclient/sse_filter.go from 1c80d4e31 on main).
Issue/plan checklist for this PR (epic #3996 AC "SSE keepalive frames are ignored before GenAI parsing"): keepalive frames dropped before genai ✅ (for the event: keepalive + payload-free shape); gateway-scoped only, direct Gemini/Vertex untouched ✅ (pinned by tests both ways); other providers' event: framing preserved ✅; non-SSE/error bodies unmodified ✅; unit-test evidence ✅ but with the misleading modalities suite above.
What
Filters gateway SSE keepalive frames before they reach the Gemini genai parser, while retaining normal response events.
Why
Gateway transport heartbeats are not model events and can otherwise cause parser failures or corrupt streaming behavior.
Validation
task test; HTTP SSE filter and Gemini transport tests.Test instructions
Run:
task testStream a Gemini response containing gateway SSE keepalive frames followed by content and a terminal event. Expected: keepalive frames are discarded before parsing, content remains intact, and the stream terminates normally.