Repository navigation
fix(ai): close a LiveSession when it's released - #17045
andrewheard wants to merge 1 commit into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
LiveSession.close() is documented as being called automatically when the session is deinitialized, but LiveSession had no deinit. It relied on LiveSessionService.deinit, which never ran while the session was connected: the service's response and message queue tasks hold strong references to it until they're cancelled. A LiveSession released without calling close() therefore kept its WebSocket connection open. Add a LiveSession deinit that closes the service. Closing cancels the tasks, so the service deinitializes, disconnects its WebSocket, and finishes the responses stream. Also correct the close() and responses docs: after close(), responses delivers no more messages unless the session is resumed, rather than throwing LiveSessionUnexpectedClosureError, and it finishes when the session is released. Add an integration test that releases a session without calling close() and expects its responses stream to finish.
6fb6626 to
7e052b1
Compare
Fixes a leak where a
LiveSessionreleased without callingclose()kept its WebSocket connection to the server open.Root cause
LiveSession.close()is documented as being called automatically when the session is deinitialized, butLiveSessionhad nodeinit. It relied onLiveSessionService.deinitto cancel its tasks and disconnect the WebSocket, and thatdeinitnever ran while the session was connected:responsesTaskandmessageQueueTaskare unstructuredTasks whose closures use the actor's state and methods, so they capture the service strongly.close()cancels them, or when their streams finish. The message queue stream is only finished in the service'sdeinit.So releasing a connected
LiveSessionleft the service, its two tasks, and the WebSocket alive until the server closed the connection (for example, at the session's time limit). I confirmed this against the real backend: three seconds after releasing a session, the service was still allocated, with its WebSocket open and both tasks running.Because open sessions count toward the backend's concurrent session limits, apps that reconnect without calling
close()could accumulate open connections.Changes
LiveSessiondeinitthat closes its service in aTask, sincedeinitcan'tawait.close()cancels both tasks and disconnects the WebSocket. Once the tasks end, nothing else holds the service, so it deinitializes and finishesresponses. (close()is idempotent.)close()docs. They said that receiving from a closed session throwsLiveSessionUnexpectedClosureError. In practice,responsesreceives no new messages unless the session is resumed, and finishes shortly after the session is released. I also dropped "Permanently" from the summary, sinceresumeSessioncan reopen a closed session.responsesthat the stream finishes, at the latest, shortly after the session is deinitialized, so apps need to keep a reference to the session while consuming it.Testing
Added
releasedSession_isClosedtoLiveSessionTests. It connects, keeps onlyresponses, releases the session without callingclose(), and expects the stream to finish within 5 seconds. It only uses public API.This branch: the new test uses the suite's shared
arguments(Gemini 2.5 Live on Agent Platform and Google AI), so it has 2 cases. Both pass.On top of #17044 (adds Gemini 3.8 Live), 4 cases:
With the fix, it also passed 3 more iterations there (12/12).
The Live unit test suites (
LiveSessionServiceTests,LiveServerVoiceActivityTests,LiveGenerationConfigTests) pass. A deterministic unit test isn't practical here, becauseMockURLProtocolonly handles HTTP and the service only spawns its tasks after a real setup handshake.Known issues not addressed in this PR
Possible race when the client closes the WebSocket
In
AsyncWebSocket,close(code:reason:underlyingError:)records the close error, cancels theURLSessionWebSocketTask, and then finishes the message stream. Meanwhile, thestartReceiving()loop's pendingreceive()throws as soon as the task is cancelled, and itscatchcallscloseagain withwebSocketTask.closeCode. If that second call finishes the stream first, andcloseCodeisn't.goingAwayat that point, the stream ends with a different error.LiveSessionServicewould then surface it as aLiveSessionUnexpectedClosureError, soresponseswould throw instead of finishing normally on release (or staying open after an explicitclose()). The second call also overwrites the recorded close error, so later sends would throw the wrong error.This was found by reading the code. I haven't seen it happen in any of the passing runs above; the new test would fail with a thrown error if it did. I'm not sure what
closeCodereports right after a client-side cancel. A possible fix is to skip thecatchpath when the client has already started closing, since the close error is recorded before the cancel.A server-sent
.goingAwayclose is treated as a user closeLiveSessionService.mapWebsocketErrorreturnsnilfor.goingAway(1001), on the assumption that the user calledclose(). If the server closes the connection with.goingAway,responsesTaskexits without callingclose()or finishingresponses.messageQueueTaskkeeps the service alive, soresponsesnever finishes while the app still holds the session (it now does finish when the session is released), and sends fail with only a log message.Alternatives considered
Breaking the cycle inside
LiveSessionService, by having its tasks capture the service weakly. This would fix the cycle where it originates and let the service tear down synchronously in its owndeinit. But both tasks rely on running on the actor (to readmessageQueue, decode and encode messages, and callclose()), so they'd need to be restructured, and aguard let selfheld across thefor try awaitloops would recreate the strong reference. Fixing it inLiveSession, the only owner of the service, is much smaller and makes the existing documentation true. If anything other thanLiveSessionever holds a service, it would also need to close it.