fix: always close tool executors on conversation close

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.

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
         )