Skip to content

fix(#3996): drop gateway SSE keepalive frames before the genai parser - #4022

Draft
aheritier wants to merge 2 commits into
title-generation-model-filterfrom
gemini-gateway-transport
Draft

fix(#3996): drop gateway SSE keepalive frames before the genai parser#4022
aheritier wants to merge 2 commits into
title-generation-model-filterfrom
gemini-gateway-transport

Conversation

@aheritier

@aheritier aheritier commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

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 test

Stream 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.

@aheritier aheritier added area/providers/gemini Google Gemini provider support kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Aug 22, 2026
@aheritier
aheritier force-pushed the gemini-gateway-transport branch from 30bb441 to 01ecbc9 Compare August 24, 2026 15:13
@aheritier
aheritier force-pushed the gemini-gateway-transport branch from 01ecbc9 to c3771e3 Compare August 25, 2026 07:58
@aheritier
aheritier force-pushed the gemini-gateway-transport branch from c3771e3 to e69c90a Compare August 25, 2026 20:24
@aheritier
aheritier force-pushed the gemini-gateway-transport branch from e69c90a to c064d65 Compare August 26, 2026 11:08
@aheritier
aheritier force-pushed the gemini-gateway-transport branch 2 times, most recently from 2aeb274 to c064d65 Compare September 1, 2026 16:16
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.
@aheritier
aheritier force-pushed the gemini-gateway-transport branch from c064d65 to a4ac3c3 Compare September 1, 2026 16:29

@aheritier aheritier left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.ImageOutputEnabledmodelinfo.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/providers/gemini Google Gemini provider support kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant