test: don't report GC-scheduled close tasks as leaks - #6790
Open
hassannaftabb wants to merge 1 commit into
Open
Conversation
fail_on_leaked_tasks samples asyncio.all_tasks() around the test body, so a task that garbage-collector finalization schedules onto the running loop is attributed to whichever test happened to be executing. google-genai's AsyncClient.__del__ calls aclose() that way, which makes test_output_streams_close_on_generation_complete fail intermittently in full-suite runs while passing in isolation. Give the tasks that are about to be reported a bounded moment to finish before failing. A GC-scheduled close completes in a turn or two; a real leak is still pending afterwards, so detection is unchanged. Closes livekit#6789
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.
Closes #6789.
Problem
fail_on_leaked_tasksdiffsasyncio.all_tasks()around the test body on its non-concurrent path. Garbage-collector finalization schedules short-lived close tasks onto whichever loop is running, so such a task can be pending at the instant the loop is sampled without having been leaked by the test that was executing. google-genai'sAsyncClient.__del__callsaclose()exactly this way —test_plugin_google_realtime.pyalready documents the behaviour in_make_session:That mitigation only covers the client that test constructs. The result is
test_output_streams_close_on_generation_completeerroring at teardown withBaseApiClient.aclose/AsyncClient.aclosein the leak list — only in full-suite runs, never in isolation. I saw four consecutive failures and then two clean runs with no code change in between.Change
Before failing, give the tasks that are about to be reported a bounded moment to finish. A GC-scheduled close finishes in a turn or two; a genuine leak is still pending afterwards. The wait only happens on the path that was about to fail, so a passing run does no extra work.
Why not widen the ignore list
The issue offered ignoring genai's close coroutines in
_is_ignorable_taskas one option. I went the other way: an ignore entry would suppress these tasks permanently, including a real leak of a genai client from the plugin itself. Settling distinguishes the two by behaviour rather than by name, and needs no per-library entries as more SDKs get added.Tests
tests/test_leak_check.pycovers both directions, and both failed before the change (onImportError, the helper did not exist):test_settle_drops_a_task_that_finishes_on_its_own— a task pending at sample time but finishing immediately after is not reportedtest_settle_keeps_a_task_that_never_finishes— a task that never completes is still reportedI also checked detection end-to-end rather than only through the helper: running a test module that genuinely leaks a task still produces the
Test leaked taskserror with this change applied.Verification
pytest --unitgives 1927 passed over three consecutive runs,ruff checkandruff format --checkclean. Run in a Linux container with the docker socket mounted sotests/lk_server.pycan start livekit-server;tests/test_room.pycannot pass without that.Worth saying plainly: this is an intermittent failure, so a run of clean passes is not proof on its own. The deterministic tests above are the real evidence — they pin the mechanism rather than the symptom.