revert: Revert "fix(toolbox-core): Prevent ToolboxClient from closing externally ma…"

This reverts commit 7520adf.

Author: anubhav756

packages/toolbox-core/src/toolbox_core/client.py

@@ -141,21 +141,21 @@ async def __aexit__(self, exc_type, exc_val, exc_tb):
         """
         await self.close()
 
-    def __del__(self):
-        # This method is a "best-effort" safety net.
-        # It should NOT be relied upon for guaranteed resource cleanup.
-        # Explicitly using "async with" or calling "await client.close()" is the correct way.
-        if self.__manage_session:
-            try:
-                loop = get_running_loop()
-            except RuntimeError:
-                loop = None
-
-            if loop and loop.is_running():
-                # If a loop is running, try to schedule the close operation.
-                # This is "fire-and-forget"; there's no guarantee it will complete
-                # before the loop or interpreter shuts down.
-                loop.create_task(self.__session.close())
+    # def __del__(self): TODO: Remove
+    #     # This method is a "best-effort" safety net.
+    #     # It should NOT be relied upon for guaranteed resource cleanup.
+    #     # Explicitly using "async with" or calling "await client.close()" is the correct way.
+    #     if self.__manage_session:
+    #         try:
+    #             loop = get_running_loop()
+    #         except RuntimeError:
+    #             loop = None
+
+    #         if loop and loop.is_running():
+    #             # If a loop is running, try to schedule the close operation.
+    #             # This is "fire-and-forget"; there's no guarantee it will complete
+    #             # before the loop or interpreter shuts down.
+    #             loop.create_task(self.__session.close())
 
     async def close(self):
         """

packages/toolbox-core/src/toolbox_core/sync_client.py

@@ -68,10 +68,11 @@ def close(self):
         any tools created by this Client to cease to function.
         """
         coro = self.__async_client.close()
-        run_coroutine_threadsafe(coro, self.__loop).result(timeout=5)
+        # run_coroutine_threadsafe(coro, self.__loop).result(timeout=5) TODO: Remove
+        run_coroutine_threadsafe(coro, self.__loop).result()
 
-    def __del__(self):
-        self.close()
+    # def __del__(self): TODO: Remove
+    #     self.close()
 
     def load_tool(
         self,

packages/toolbox-core/tests/conftest.py

@@ -22,16 +22,13 @@
 import subprocess
 import tempfile
 import time
-from asyncio import run_coroutine_threadsafe
 from typing import Generator
 
 import google
 import pytest_asyncio
 from google.auth import compute_engine
 from google.cloud import secretmanager, storage
 
-from toolbox_core import ToolboxSyncClient
-
 
 #### Define Utility Functions
 def get_env_var(key: str) -> str:
@@ -95,36 +92,36 @@ def get_auth_token(client_id: str) -> str:
 
 
 #### Define Fixtures
-@pytest_asyncio.fixture(autouse=True)
-def patch_sync_client_for_deadlock(monkeypatch):
-    """
-    Automatically replaces the blocking `ToolboxSyncClient.close()` method
-    with a non-blocking version for the entire test run.
-
-    The original `ToolboxSyncClient.close()` is a blocking method because it
-    calls `.result()`. In the pytest environment, this blocking call creates a
-    deadlock during the test teardown phase when it conflicts with other fixtures
-    (like `sync_client_environment` or `toolbox_server`) that are also managing
-    background processes and threads.
-
-    By replacing `close` with this safe, non-blocking version, we prevent the
-    deadlock and allow the test suite's fixtures to tear down cleanly.
-    This change is only active during the test run.
-    """
-
-    def non_blocking_close(self):
-        """A replacement for close() that doesn't block."""
-        if hasattr(self.__class__, "_ToolboxSyncClient__loop") and hasattr(
-            self, "_ToolboxSyncClient__async_client"
-        ):
-            loop = self.__class__._ToolboxSyncClient__loop
-            async_client = self._ToolboxSyncClient__async_client
-
-            if loop and loop.is_running():
-                coro = async_client.close()
-                run_coroutine_threadsafe(coro, loop)
-
-    monkeypatch.setattr(ToolboxSyncClient, "close", non_blocking_close)
+# @pytest_asyncio.fixture(autouse=True) TODO: Remove
+# def patch_sync_client_for_deadlock(monkeypatch):
+#     """
+#     Automatically replaces the blocking `ToolboxSyncClient.close()` method
+#     with a non-blocking version for the entire test run.
+
+#     The original `ToolboxSyncClient.close()` is a blocking method because it
+#     calls `.result()`. In the pytest environment, this blocking call creates a
+#     deadlock during the test teardown phase when it conflicts with other fixtures
+#     (like `sync_client_environment` or `toolbox_server`) that are also managing
+#     background processes and threads.
+
+#     By replacing `close` with this safe, non-blocking version, we prevent the
+#     deadlock and allow the test suite's fixtures to tear down cleanly.
+#     This change is only active during the test run.
+#     """
+
+#     def non_blocking_close(self):
+#         """A replacement for close() that doesn't block."""
+#         if hasattr(self.__class__, "_ToolboxSyncClient__loop") and hasattr(
+#             self, "_ToolboxSyncClient__async_client"
+#         ):
+#             loop = self.__class__._ToolboxSyncClient__loop
+#             async_client = self._ToolboxSyncClient__async_client
+
+#             if loop and loop.is_running():
+#                 coro = async_client.close()
+#                 run_coroutine_threadsafe(coro, loop)
+
+#     monkeypatch.setattr(ToolboxSyncClient, "close", non_blocking_close)
 
 
 @pytest_asyncio.fixture(scope="session")

packages/toolbox-core/tests/test_sync_client.py

@@ -47,6 +47,7 @@ def sync_client_environment():
     if original_loop and original_loop.is_running():
         original_loop.call_soon_threadsafe(original_loop.stop)
         if original_thread and original_thread.is_alive():
+            # original_thread.join(timeout=5) TODO: Remove
             original_thread.join()
         ToolboxSyncClient._ToolboxSyncClient__loop = None
         ToolboxSyncClient._ToolboxSyncClient__thread = None
@@ -63,6 +64,7 @@ def sync_client_environment():
     if test_loop and test_loop.is_running():
         test_loop.call_soon_threadsafe(test_loop.stop)
     if test_thread and test_thread.is_alive():
+        # test_thread.join(timeout=5) TODO Remove
         test_thread.join()
 
     # Explicitly set to None to ensure a clean state for the next fixture use/test.
@@ -288,6 +290,30 @@ def test_sync_client_creation_in_isolated_env(self, sync_client):
             sync_client._ToolboxSyncClient__async_client, ToolboxClient
         ), "Async client should be ToolboxClient instance"
 
+    # @pytest.mark.usefixtures("sync_client_environment") TODO: Remove
+    # def test_sync_client_close_method(self):
+    #     """
+    #     Tests the close() method of ToolboxSyncClient when manually created.
+    #     The sync_client_environment ensures loop/thread cleanup.
+    #     """
+    #     mock_async_client_instance = AsyncMock(spec=ToolboxClient)
+    #     # AsyncMock methods are already AsyncMocks
+    #     # mock_async_client_instance.close = AsyncMock(return_value=None)
+
+    #     with patch(
+    #         "toolbox_core.sync_client.ToolboxClient",
+    #         return_value=mock_async_client_instance,
+    #     ) as MockedAsyncClientConst:
+    #         client = ToolboxSyncClient(TEST_BASE_URL)
+    #         # The sync client passes its internal loop to the async client.
+    #         MockedAsyncClientConst.assert_called_once_with(
+    #             TEST_BASE_URL, client_headers=None
+    #         )
+
+    #         client.close()  # This call closes the async_client's session.
+    #         mock_async_client_instance.close.assert_awaited_once()
+    #     # The sync_client_environment fixture handles stopping the loop/thread.
+
     @pytest.mark.usefixtures("sync_client_environment")
     def test_sync_client_context_manager(self, aioresponses, tool_schema_minimal):
         """