diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index c895edd83e..af6ee4b2b9 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -1,4 +1,5 @@ import atexit +import contextlib import copy import uuid from collections.abc import Mapping @@ -994,23 +995,20 @@ def close(self) -> None: self.agent.close() except Exception as e: logger.warning(f"Error closing agent: {e}") - if self.delete_on_close: - try: - tools_map = self.agent.tools_map - except (AttributeError, RuntimeError): - # Agent not initialized or partially constructed - return - for tool in tools_map.values(): - try: - executable_tool = tool.as_executable() - executable_tool.executor.close() - except NotImplementedError: - # Tool has no executor, skip it without erroring - continue - except Exception as e: - logger.warning( - f"Error closing executor for tool '{tool.name}': {e}" - ) + # Always close tool executors — they hold runtime resources + # (subprocesses, connections, etc.) that must be released regardless + # of whether the conversation data is preserved (delete_on_close). + with contextlib.suppress(AttributeError, RuntimeError): + # Agent not initialized or partially constructed → skip + for tool in self.agent.tools_map.values(): + with contextlib.suppress(NotImplementedError): + try: + executable_tool = tool.as_executable() + executable_tool.executor.close() + except Exception as e: + logger.warning( + f"Error closing executor for tool '{tool.name}': {e}" + ) def ask_agent(self, question: str) -> str: """Ask the agent a simple, stateless question and get a direct LLM response. diff --git a/tests/tools/terminal/test_conversation_cleanup.py b/tests/tools/terminal/test_conversation_cleanup.py index f3ef43f91c..6723d19134 100644 --- a/tests/tools/terminal/test_conversation_cleanup.py +++ b/tests/tools/terminal/test_conversation_cleanup.py @@ -34,7 +34,6 @@ def _make_tool(conv_state, **params): llm=mock_llm, tools=[Tool(name="test_terminal")], ) - # delete_on_close=True is required to trigger executor cleanup conversation = Conversation( agent=agent, workspace=temp_dir, delete_on_close=True ) @@ -49,6 +48,34 @@ def _make_tool(conv_state, **params): terminal_executor.close.assert_called_once() +def test_conversation_close_calls_executor_close_without_delete(mock_llm): + """Executors are closed even when delete_on_close=False.""" + with tempfile.TemporaryDirectory() as temp_dir: + terminal_executor = TerminalExecutor( + working_dir=temp_dir, terminal_type="subprocess" + ) + terminal_executor.close = Mock() + + def _make_tool(conv_state, **params): + tools = TerminalTool.create(conv_state) + tool = tools[0] + return [tool.model_copy(update={"executor": terminal_executor})] + + register_tool("test_terminal", _make_tool) + + agent = Agent( + llm=mock_llm, + tools=[Tool(name="test_terminal")], + ) + conversation = Conversation( + agent=agent, workspace=temp_dir, delete_on_close=False + ) + conversation._ensure_agent_ready() + conversation.close() + + terminal_executor.close.assert_called_once() + + def test_conversation_del_calls_close(mock_llm): """Test that Conversation.__del__() calls close().""" with tempfile.TemporaryDirectory() as temp_dir: @@ -70,7 +97,6 @@ def _make_tool(conv_state, **params): llm=mock_llm, tools=[Tool(name="test_terminal")], ) - # delete_on_close=True is required to trigger executor cleanup conversation = Conversation( agent=agent, workspace=temp_dir, delete_on_close=True )