fix(live): stop an early-closed merged live stream from hanging - #7358
Open
harshal-96 wants to merge 1 commit into
Open
harshal-96 wants to merge 1 commit into
harshal-96 wants to merge 1 commit into
Conversation
_merge_live_event_streams feeds a one-slot queue from two pumps. When the caller closes the stream while that queue is full, cleanup cancels the queued-events pump, whose finally block then waits forever to put the done sentinel, since nothing reads the queue anymore. This hung both LiveKit run_live tests on some machines. Skip the sentinel once the consumer has stopped reading.
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
_merge_live_event_streams#7357Problem:
_merge_live_event_streamsmerges two pumps into a one-slot queue,merged. When the caller closes the stream whilemergedis full, the cleanup cancels_pump_queued_events. Itsfinallythen waits forever inawait merged.put(done_sentinel), because nothing readsmergedanymore, soaclose()never returns. Details and a reproduction script are in the issue.Solution:
The merge now sets a local
consumer_doneflag in itsfinally, before the cleanup starts._pump_queued_eventsonly puts the sentinel if the consumer is still reading. Once the consumer is gone, nobody is waiting for the sentinel.mergedkeeps its one-slot size, so the backpressure is unchanged.Testing Plan
Unit Tests:
Added
test_merge_live_event_streams_closes_while_merged_queue_is_fulltotests/unittests/live/test__runner_utils.py. It fails 20 of 20 runs without the fix and passes 20 of 20 with it.All results below are from environments built like CI (
uv sync --extra test --no-install-package lancedb):tests/unittests/live,tests/unittests/integrations/livekit,tests/unittests/runnersandtests/unittests/test_runners.py: 415 passed, 1 skipped, 4 xfailed on Python 3.10, 3.11, 3.12, 3.13 and 3.14. Without the fix, one or both LiveKitrun_livetests time out on each version.tests/unittestson Python 3.12: 16389 passed, 1 failed. The one failure istest_import_loading.py::test_entry_point_loads_only_allowlisted_packages[agent], which already fails onmainfor an unrelated reason.mainitself gives 16386 passed, 3 failed: that same test plus the two LiveKit tests fixed here.main): no new errors.Manual End-to-End (E2E) Tests:
closedand exits. Onmainit hangs atclosing....main, 131 of 500 hung on Linux and 157 of 500 on Windows (Python 3.12).Runnerthroughrun_liveare the closest check, and they pass.Checklist
Additional context
The change is small: 6 added lines and 1 changed line in
src/google/adk/live/_runner_utils.py, plus one new test.