Skip to content

fix(realtime): release the websocket when the initial session config fails - #4161

Closed
LeSingh1 wants to merge 1 commit into
openai:mainfrom
LeSingh1:fix/realtime-connect-cleanup
Closed

fix(realtime): release the websocket when the initial session config fails#4161
LeSingh1 wants to merge 1 commit into
openai:mainfrom
LeSingh1:fix/realtime-connect-cleanup

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

OpenAIRealtimeWebSocketModel.connect() opens the websocket and starts the listener task, then sends the initial session config. If that send fails, neither the socket nor the task is released.

This is reachable from ordinary agent configuration, not just transport errors: _tools_to_session_tools raises UserError("Tool ... is unsupported. Must be a function tool.") for any non-FunctionTool, and RealtimeSession._get_updated_model_settings_from_agent does not reject hosted tools beforehand. So a realtime agent configured with, say, a WebSearchTool leaks a live connection. ensure_tool_choice_supports_backend and a ConnectionClosed on send reach the same point.

The caller cannot clean up after it either. RealtimeSession.__aenter__ only calls remove_listener, and Python does not invoke __aexit__ when __aenter__ raises, so the usual async with await runner.run() as session: form gives no cleanup hook. On top of the leaked socket and background task, connect() opens with assert self._websocket is None, so the model instance can never be reconnected.

Fix: wrap the initial _update_session_config call and await self.close() before re-raising, restoring the same clean state that test_connect_websocket_failure_propagates already asserts for a failure one step earlier. .agents/references/realtime-session-lifecycle.md states this ownership rule; this is the untested half of it.

Test plan

Added test_connect_session_config_failure_releases_websocket: connects with a WebSearchTool in initial_model_settings, asserts the UserError propagates, and asserts _websocket is None, _websocket_task is None, and that close() was awaited on the socket.

Fails on main (assert <AsyncMock> is None), passes with the fix. Full tests/realtime/ suite: 378 passed. .agents/skills/code-change-verification/scripts/run.sh passed.

Issue number

N/A — found while auditing connection ownership on realtime failure paths.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the focused fix. The leak is real, and cleaning up when the initial session update fails is the correct direction.

Before we merge this, please make the existing single-connection invariant effective across the whole connection attempt. Two connect() calls can currently pass the assertions before either assigns _websocket; if A's initial send is pending, B installs newer connection state, and A then fails, A's unconditional self.close() can close B's socket and listener task. Please mark an attempt active before the first await, reject a second attempt before it acquires resources, and clear that guard on every failure path.

Please also add a controlled interleaving test that pauses A's initial send, verifies B is rejected without opening another socket, then fails A and confirms its original exception and resource cleanup.

@seratch

seratch commented Aug 5, 2026

Copy link
Copy Markdown
Member

Closing this PR in favor of #4189; your contribution is included in the commit.

@seratch seratch closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants