fix(llminternal): guard RunLive errChan sends so abandoned producers exit on reconnect#1167
Open
chethanuk wants to merge 1 commit into
Open
fix(llminternal): guard RunLive errChan sends so abandoned producers exit on reconnect#1167chethanuk wants to merge 1 commit into
chethanuk wants to merge 1 commit into
Conversation
…exit on reconnect RunLive's reader and sender goroutines did bare sends on the unbuffered, per-attempt errChan. Once the consumer loop stops reading that channel -- after a resumable-error reconnect, a parent context cancellation, or any other teardown -- cleanup() closes the connection, the producer's in-flight call fails, and its send blocks forever, leaking one goroutine (plus its captured connection state) per occurrence. Long-lived /run_live servers accumulate these across sessions and reconnects. Guard all three sends with the same select-on-connCtx idiom the function already uses for eventsChan. cleanup() always cancels connCtx before the next attempt (and before returning), so a guarded send is guaranteed to unblock. connCtx, not ctx: ctx outlives reconnects and would not unblock an abandoned producer. The regression test fakes the Gemini Live wire protocol over a local websocket server and fails deterministically on the unfixed code with goroutines parked in 'chan send' at the bare-send sites. Fixes google#1152
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.
Fixes #1152
Long-lived
/run_liveservers leak one goroutine (plus its captured connection state) every time a live session reconnects — and, it turns out, on ordinary teardown too. The producers insideFlow.RunLivedo bare sends on the unbuffered, per-attempterrChan; once the consumer loop has stopped reading that channel, the producer blocks on the send forever.Root cause
internal/llminternal/base_flow.gospawns two producer goroutines per connection attempt (a reader and a sender) that report failures viaerrChan <- errat three sites (:389,:430,:439). The consumer readserrChanat most once per attempt: on a resumable error it setsreconnect = true, breaks out, andcleanup()cancelsconnCtxand closes the connection. The other producer's in-flightRecv/Send*call now fails against the closed connection, and its bare send on the abandoned channel blocks permanently:sequenceDiagram participant R as reader participant C as consumer participant S as sender R->>C: errChan <- resumable err C->>C: reconnect = true, break C->>C: cleanup: cancelConn + Close conn Note over C: next attempt gets fresh errChan S--xS: SendContent fails (conn closed) S--xS: errChan <- err on abandoned channel Note over S: blocks forever - leakedThe same mechanics fire without any reconnect: cancel the invocation context while the reader is parked in
Recv(the everyday session-teardown path), and the consumer exits viactx.Done()→cleanup()closes the connection → the reader'sRecvfails → its bare send strands it. So every normal live-session teardown leaks the reader goroutine, which makes this slightly broader than the reconnect-only symptom in the issue.One small correction to the issue text, respectfully:
cleanup()does run on the reconnect path — it is called unconditionally after the consumer loop (:541), before thereconnectcheck. Only the send-site guards are missing, which makes the fix smaller than proposed.The fix
Guard all three
errChansends with the exact idiom the same function already uses foreventsChan(:409-413):Guarding on
connCtx, notctx, is load-bearing:cleanup()always cancelsconnCtx— before the next attempt and before every return — so a guarded send is guaranteed to unblock an abandoned producer.ctxoutlives reconnects and would leave the producer blocked across attempts.Deliberately untouched: the
eventsChansend (already guarded), the consumer's double-break flow at:521-537, andcleanup()placement.Why not a buffered
make(chan error, 2)? It also fixes the current leak, but as discussed on the issue it silently caps the number of background workers and masks the lifecycle instead of modeling it; theselectguard extends the function's existing convention. Anerrgrouprestructure would be a rewrite, out of proportion for this bug.Tests
TestRunLiveNoGoroutineLeak(newbase_flow_live_test.go) fakes the Gemini Live wire protocol with anhttptest+gorilla/websocketserver (the same approach as go-genai's ownlive_test.go), pointed at viaHTTPOptions.BaseURLwith aws://scheme — no new dependencies, no production test hooks.Sendsucceeds on conn 2, no leakThe leak assertion is dependency-free (goleak is not in go.mod): it snapshots
runtime.Stack(all=true), counts goroutines still insidellminternal.(*Flow).RunLive, and polls until the count returns to the subtest's baseline or a 5s deadline expires, failing with the captured stacks.Testing Plan
Behavior-change evidence — the new test on the unfixed code fails deterministically, with the leaked producers parked in
chan sendat the exact bare-send site:With the fix applied, all four cases pass 100/100 under
-race -count=20.