Skip to content

fix: resource cleanup improvements in session teardown#4831

Open
SezginKahraman wants to merge 1 commit intolivekit:mainfrom
SezginKahraman:fix/resource-cleanup
Open

fix: resource cleanup improvements in session teardown#4831
SezginKahraman wants to merge 1 commit intolivekit:mainfrom
SezginKahraman:fix/resource-cleanup

Conversation

@SezginKahraman
Copy link
Contributor

  • Cancel video forwarding task during AgentSession close to prevent leaked async tasks
  • Unregister participant_disconnected event handler in RoomIO.aclose() to prevent callbacks on stale objects
  • Add session.closed check in _close_http_ctx to avoid creating a new session just to immediately close it
  • Fix "shadow copy" → "shallow copy" comment typo

- Cancel video forwarding task during AgentSession close to prevent
  leaked async tasks
- Unregister participant_disconnected event handler in RoomIO.aclose()
  to prevent callbacks on stale objects
- Add session.closed check in _close_http_ctx to avoid creating a new
  session just to immediately close it
- Fix "shadow copy" → "shallow copy" comment typo

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +58 to +60
session = val()
if not session.closed:
await session.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 _close_http_ctx still creates a new HTTP session just to close it

The PR adds a session.closed check in _close_http_ctx with the stated goal of "avoid creating a new session just to immediately close it." However, the check is ineffective because val() calls the _new_session factory, which at http_context.py:19 creates a new aiohttp.ClientSession whenever g_session is None or g_session.closed. The newly created session will never have closed == True, so the guard on line 59 never prevents the close.

Root Cause

The _new_session closure (line 17-34) always returns a non-closed session. When g_session is None (HTTP was never used during the job) or already closed, val() creates a brand new aiohttp.ClientSession with a TCPConnector. The new session's .closed property is False, so the check if not session.closed on line 59 passes and the session is closed immediately after creation.

Scenario: A job runs without ever calling http_session(). At shutdown, _close_http_ctx() is called (job_proc_lazy_main.py:367). val is not None (set by _new_session_ctx() at line 283), so val() is invoked, creating an unnecessary aiohttp.ClientSession and TCPConnector just to immediately close them.

The fix should avoid calling val() entirely when no session was ever created, or the factory should be modified to not create a session when one doesn't already exist.

Impact: Unnecessary resource allocation (TCP connector, HTTP session) during every job shutdown where HTTP was unused. Not a crash, but defeats the stated purpose of the fix.

Prompt for agents
The _close_http_ctx function needs to avoid calling val() (the factory) when no session has been created yet, because the factory always creates a new session if one doesn't exist. One approach: modify _new_session_ctx to expose a way to check if g_session exists without creating one. For example, add a separate function or attribute to the closure that returns g_session directly (without the lazy-creation logic). Alternatively, restructure _new_session_ctx to use a wrapper object that tracks whether a session was ever created, and check that in _close_http_ctx before calling val(). The simplest fix might be to change _new_session to not auto-create when g_session is None, and instead have a separate creation path, but that would break the existing http_session() API. A practical approach: store g_session as an attribute on the _new_session function itself (e.g., _new_session._session = g_session) so _close_http_ctx can check _new_session._session directly without triggering creation.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments