chore(lint): enable RUF006 so dangling asyncio tasks fail CI - #7054
chore(lint): enable RUF006 so dangling asyncio tasks fail CI#7054Rehansanjay wants to merge 7 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| @@ -251,13 +252,20 @@ async def _task() -> None: | |||
| logger.debug(f"Soniox TTS prewarm failed: {e}") | |||
There was a problem hiding this comment.
🟨 Prewarm exceptions expose provider data
A Soniox prewarm failure logs the original exception through exc_info. Provider URLs, headers, or response details can reach unredacted logs.
Was this helpful? React with 👍 or 👎 to provide feedback.
abe12a7 to
90714f9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
5c67463 to
7249e54
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7249e54 to
52a30b9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
52a30b9 to
c673b74
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c673b74 to
9c39386
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 new potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ Background example work can disappear
Without a strong owner, bare create_task calls can lose cart pushes, front-desk updates, and translator cleanup before completion. Users see stale UI or lingering translation resources.
⚠️ Dangling tasks pass CI
With RUF006 absent from select, lint accepts discarded asyncio tasks. Future task-loss regressions pass CI silently.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| self._tool_recycle_running = False | ||
| self._tool_recycle_pending = False | ||
| # Permanent, unlike _is_sess_active, which a recycle clears and sets again. | ||
| self._closing = False |
There was a problem hiding this comment.
|
|
||
| def prewarm(self) -> None: | ||
| asyncio.create_task(self._prewarm_impl()) | ||
| self._prewarm_task = asyncio.create_task(self._prewarm_impl()) |
There was a problem hiding this comment.
here still replaces _prewarm_task on a second prewarm, soniox has the guard, and inworld needs it too
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)
Two follow-ups from the review. `prewarm()` overwrote `_prewarm_task` unconditionally, so calling it twice dropped the only reference to the first task while it was still running. `aclose()` cancels just the latest one, which left the earlier attempt free to open a WebSocket after shutdown — the leak this PR is meant to close. It now starts a new task only when there is no live one, which is how `utils.ConnectionPool.prewarm` guards the same case. The prewarm failure log also interpolated the exception into the message body, where it cannot be redacted. It now goes through `exc_info`, as the send and recv loops in this file already do. (cherry picked from commit a1569b1)
`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)
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. (cherry picked from commit c3dc61c)
Both were created and discarded, so the event loop's weak reference was the only thing keeping them alive and either could be collected before it finished. Each now goes into a set that discards on completion. Reference-holding only: no cancellation, no coalescing, no change to shutdown. The lifecycle of these two tasks is the subject of livekit#7052 and is reviewed there.
Follow-up to livekit#7050. `prewarm()` overwrites `_prewarm_task` unconditionally, so a second call drops the only reference to a task still in flight, and `aclose()` cancels just the latest one — leaving the earlier attempt free to build a connection pool after shutdown, which is the leak livekit#7050 set out to close. Starts a task only when there is no live one, matching the soniox plugin and `utils.ConnectionPool.prewarm`.
c3dc61c to
7fe421a
Compare
|
Thanks — all three done in 7fe421a. Consolidated. #7050 merged in the meantime, so it has dropped out of the stack on its own. #7051 and #7053 are unchanged here and I am closing both, pointing at this PR. aws cut back. Worth flagging that the first commit was not purely handles — it also added a replace-in-place cancel in inworld guard. Correct, and it undercut the point of #7050. Added as a follow-up commit here, matching the soniox guard and One consequence to flag: #7052 currently builds on the older aws commit, so once this lands it will need a rebase. Happy to do that as soon as this is settled — I would rather not churn it while you are looking at this one. Verified after the rework: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`push_cart` gated on `push_running`, but that flag is set inside `_push_runner`, which does not run until the loop schedules it. Several order changes in the same tick therefore each saw it clear and started a runner, and concurrent runners can deliver a stale cart after a newer one. The task retained for RUF006 is a better guard: it exists as soon as `create_task` returns. Using it removes the flag entirely, and `push_pending` still carries the trailing edge, so a change during an in-flight RPC is sent once that RPC completes.
|
One more, in 50289ab — and I want to flag it as a judgment call against the line you drew, so you can push back. Devin caught that I fixed it rather than only holding the handle, because here the two are the same thing: the task retained for RUF006 exists as soon as It is still a behaviour change, so if you would rather this PR only hold the reference and leave the race for its own change, say so and I will cut it back. My reasoning for treating it differently from the aws commits: that was a rewrite of session shutdown in a plugin, this is a one-line guard in an example, and examples get copied.
|
2 participants
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.