chore(lint): enable RUF006 so dangling asyncio tasks fail CI - #7054
Open
Rehansanjay wants to merge 5 commits into
Open
chore(lint): enable RUF006 so dangling asyncio tasks fail CI#7054Rehansanjay wants to merge 5 commits into
Rehansanjay wants to merge 5 commits into
Conversation
…lose `prewarm()` discarded the task it created. Two things follow from that. The task is only weakly referenced by the event loop, so it can be garbage collected mid-execution and the prewarm silently never happens. The worse one is ordering. `_prewarm_impl` calls `_get_pool`, which builds a new `_ConnectionPool` whenever `self._pool` is None — and `aclose()` sets it to None after closing. A prewarm still in flight when the TTS is closed therefore recreates the pool afterwards, leaving a live connection that nothing owns and nothing will close. Store the task and cancel it at the top of `aclose()`, which is what `openai/tts.py` does with its own hand-rolled prewarm, and what `utils.ConnectionPool.prewarm()` does for the fifteen plugins that use it. Inworld was the only plugin calling `asyncio.create_task` in `prewarm()` without holding on to the result. (cherry picked from commit 553b904)
Five `asyncio.create_task` calls in this module threw the task away. The event loop only holds a weak reference to a bare task, so it can be garbage collected before it finishes. `_Connection` self-closes from four places — `mark_non_current`, `unregister_stream`, and the `finally` of both the send and recv loops. Each one discarded the task that closes the WebSocket, so a collected task means the socket stays open. The class already stores its send, recv and keepalive tasks; these were the exception. They now go through `_schedule_close`, which also skips scheduling a second close while one is still in flight. `TTS.prewarm` had the ordering problem as well: its task calls `_current_connection`, which opens a new connection when there is none, and `aclose()` sets `__current_connection` to None. A prewarm in flight when the TTS closed would reconnect afterwards and leave a live WebSocket that nothing owns. It is now cancelled at the top of `aclose()`. (cherry picked from commit 42fb296)
`RealtimeSession` tracks and cancels four background tasks in `aclose()` — the recycle timer, the response task, the audio input task and the main task. Two others were created and dropped on the floor. The event loop keeps only a weak reference to a bare task, so either can be garbage collected before it finishes, and neither was cancelled when the session closed. `_send_user_text` waits on `_stream_ready` before sending. Collected there, the message is never sent and `_pending_generation_fut` is never resolved, so a `generate_reply(user_input=...)` waits forever. Surviving until after close is no better: it wakes up and sends on a stream that is gone. The tasks are now kept in a set that discards on completion, and cancelled on close. `_deferred_tool_recycle` sleeps 0.15s and then recycles the session. Collected during that sleep, the recycle never happens and the new tool set is silently never applied, even though `update_tools` returned normally. It also had no replace-in-place guard, so two quick `update_tools` calls raced two recycles against each other — which is exactly what `_start_session_recycle_timer` already avoids by cancelling the previous timer first. It now follows that same pattern. (cherry picked from commit ed28311)
`SpeechStream.update_options` applies the new model, language or extras to `self._opts` and then fires `asyncio.ensure_future` to tell the server about them. The event loop only holds a weak reference to that future, so it can be garbage collected before the message is sent. When that happens the failure is silent and one-sided: local options say the model changed, the server was never told, and the stream keeps transcribing with the old settings. Nothing raises and nothing logs. Hold the task in a set that discards on completion, and cancel whatever is still pending when the run loop tears the websocket down, next to the existing `gracefully_cancel` of the send/recv/vad tasks. Caught by ruff's RUF006 (asyncio-dangling-task), which is not currently enabled in this repo. (cherry picked from commit 15dc5bc)
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
+1935
to
+1936
| if self._deferred_tool_recycle_task and not self._deferred_tool_recycle_task.done(): | ||
| self._deferred_tool_recycle_task.cancel() |
Contributor
There was a problem hiding this comment.
🔴 Concurrent tool updates strand sessions
A tool update arriving after deferred recycling starts cancels _deferred_tool_recycle_task midway through shutdown. The replacement sees an inactive session and exits, leaving it unable to process further audio.
Prompt for agents
In livekit-plugins/livekit-plugins-aws/livekit/plugins/aws/experimental/realtime/realtime_model.py, update_tools cancels _deferred_tool_recycle_task whenever it is unfinished. That task may already be inside _graceful_session_recycle, where cancellation after _is_sess_active.clear() leaves the session stopped; the replacement task then skips because the session is inactive. Coalesce tool changes without cancelling a recycle that has begun, ensuring the latest self._tools value is used when the restarted session initializes. Also ensure aclose can still cancel and await the task.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| try: | ||
| asyncio.create_task(_task(), name="soniox-tts-prewarm") | ||
| self._prewarm_task = asyncio.create_task(_task(), name="soniox-tts-prewarm") |
Contributor
There was a problem hiding this comment.
🟡 Repeated prewarm leaks a connection
Repeated prewarm() calls replace _prewarm_task while an earlier connection attempt continues. aclose() cancels only the latest task, so the earlier task can open a WebSocket after shutdown.
Suggested change
| self._prewarm_task = asyncio.create_task(_task(), name="soniox-tts-prewarm") | |
| if self._prewarm_task is None or self._prewarm_task.done(): | |
| self._prewarm_task = asyncio.create_task(_task(), name="soniox-tts-prewarm") |
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -251,13 +252,20 @@ async def _task() -> None: | |||
| logger.debug(f"Soniox TTS prewarm failed: {e}") | |||
Contributor
There was a problem hiding this comment.
🟨 Prewarm errors bypass log redaction
A failed prewarm interpolates e into the log message. Provider exceptions can contain sensitive request data, which cannot be redacted from message bodies.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
The event loop keeps only a weak reference to a task, so a bare `asyncio.create_task(...)` or `ensure_future(...)` whose result is thrown away can be garbage collected before it finishes. Ruff has a rule for exactly this — RUF006, asyncio-dangling-task — but `RUF` is not in this repo's select list, so it has never run here. The rule reports fifteen findings across the repo. Nine in the library are fixed in the commits this is stacked on. The remaining six are fixed here: the hamming test-reset helper, three examples, and one in tests/test_ipc.py. The examples matter more than their line count suggests — people copy them, so an example that drops a task teaches the bug. Each now keeps the task in a set that discards on completion, which is the pattern the asyncio docs ask for. Selecting `RUF006` on its own rather than all of `RUF`: the full ruleset adds ~460 unrelated findings (RUF100 unused-noqa alone is 251), which is a separate conversation. `ruff check .` passes clean across the whole repo with the rule enabled, and still reports the error if a dangling task is reintroduced.
Rehansanjay
force-pushed
the
chore/enable-ruf006
branch
from
August 31, 2026 13:16
6c5bcd3 to
abe12a7
Compare
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
1 participant
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.
Why
The event loop keeps only a weak reference to a task. A bare
asyncio.create_task(...)orasyncio.ensure_future(...)whose result is thrown away can therefore be garbage collected before it finishes, and the failures are quiet — a websocket that never closes, asession.updatethe server never receives, a tool set that silently never applies.Ruff has a rule for precisely this: RUF006,
asyncio-dangling-task. The repo's select list isso
RUFis absent and the rule has never run here.What it found
Fifteen findings across the repo:
livekit-plugins-soniox/…/tts.pylivekit-plugins-aws/…/realtime_model.pylivekit-agents/…/inference/stt.pylivekit-plugins-inworld/…/tts.pylivekit-plugins-hamming/…/_plugin.pyexamples/(drive_thru, frontdesk, translation ×2)tests/test_ipc.pyThe examples matter more than their line count suggests — people copy them, so an example that drops a task teaches the bug. Each now holds the task in a set that discards on completion, which is the pattern the asyncio docs ask for.
Why
RUF006and notRUFEnabling the whole ruleset adds roughly 460 unrelated findings —
RUF100unused-noqa alone is 251,RUF022unsorted-dunder-all is 77. That is a separate conversation and I did not want to smuggle it in here.Checks
ruff checkpasses clean across both trees with the rule enabled.ruff format --checkclean on the changed file.One note in case it is useful to others:
ensure_futurehas the same weak-reference semantics ascreate_task, and RUF006 covers both. A hand-rolled grep or AST scan looking only forcreate_taskmisses them — that is how the coreinference/stt.pycase in #7053 stayed hidden.