diff --git a/e2e/python/conftest.py b/e2e/python/conftest.py index 3c2bea236a..8e676dfeb0 100644 --- a/e2e/python/conftest.py +++ b/e2e/python/conftest.py @@ -95,6 +95,9 @@ def _create(*, spec: object | None = None, delete_on_exit: bool = True) -> Sandb delete_on_exit=delete_on_exit, # Allow time to pull an explicitly supplied workload fixture. ready_timeout_seconds=300.0, + # Kubernetes holds DeleteSandbox open until the pod terminates + # (30s grace plus API headroom), which can exceed the 30s default. + delete_timeout_seconds=120.0, ) return _create diff --git a/python/openshell/sandbox.py b/python/openshell/sandbox.py index e9c840e481..bba54996a8 100644 --- a/python/openshell/sandbox.py +++ b/python/openshell/sandbox.py @@ -626,9 +626,16 @@ def exec_python( timeout_seconds=timeout_seconds, ) - def delete(self, *, allow_missing: bool = False) -> DeletionResult: + def delete( + self, *, allow_missing: bool = False, timeout: float | None = None + ) -> DeletionResult: + # Forward `timeout` only on opt-in so older client subclasses still work. + extra = {} if timeout is None else {"timeout": timeout} return self._client.delete( - self.sandbox.name, workspace=self._workspace, allow_missing=allow_missing + self.sandbox.name, + workspace=self._workspace, + allow_missing=allow_missing, + **extra, ) def stop(self) -> SandboxRef: @@ -1072,15 +1079,21 @@ def list_ids_for_all_workspaces( ] def delete( - self, name: str, *, workspace: str, allow_missing: bool = False + self, + name: str, + *, + workspace: str, + allow_missing: bool = False, + timeout: float | None = None, ) -> DeletionResult: + """Delete a sandbox, waiting up to `timeout` (default: client timeout).""" response = self._stub.DeleteSandbox( openshell_pb2.DeleteSandboxRequest( workspace_scope=_workspace_scope(workspace), name=name, allow_missing=allow_missing, ), - timeout=self._timeout, + timeout=self._timeout if timeout is None else timeout, ) return DeletionResult( DeletionOutcome(response.outcome), response.sandbox_id or None @@ -1586,6 +1599,7 @@ def __init__( workload_template: str | None = None, timeout: float = 30.0, ready_timeout_seconds: float = 120.0, + delete_timeout_seconds: float | None = None, auto_refresh: bool = True, write_back: bool = True, insecure: bool = False, @@ -1593,6 +1607,10 @@ def __init__( ) -> None: """Bind a Sandbox context to the active gateway. + `delete_timeout_seconds` bounds the delete RPC on exit and defaults to + `timeout`. Raise it for drivers that hold the delete open while the + workload terminates. + OIDC kwargs (`auto_refresh`, `write_back`, `insecure`, and `client_credentials`) forward directly to `SandboxClient.from_active_cluster` and have the @@ -1612,6 +1630,7 @@ def __init__( self._workload_template = workload_template self._timeout = timeout self._ready_timeout_seconds = ready_timeout_seconds + self._delete_timeout_seconds = delete_timeout_seconds self._auto_refresh = auto_refresh self._write_back = write_back self._insecure = insecure @@ -1707,7 +1726,9 @@ def __exit__(self, *args: object) -> None: and self._session is not None and self._client is not None ): - result = self._session.delete(allow_missing=True) + result = self._session.delete( + allow_missing=True, timeout=self._delete_timeout_seconds + ) if result.outcome == DeletionOutcome.ACCEPTED: self._client.wait_deleted( self._session.sandbox.name, diff --git a/python/openshell/sandbox_cleanup_test.py b/python/openshell/sandbox_cleanup_test.py index 4cf558251c..47c4eb945f 100644 --- a/python/openshell/sandbox_cleanup_test.py +++ b/python/openshell/sandbox_cleanup_test.py @@ -3,6 +3,7 @@ """Cleanup status handling through the curated client's real gRPC interceptor.""" +import time from concurrent import futures from contextlib import nullcontext from types import SimpleNamespace @@ -20,6 +21,7 @@ def cleanup_client(): state = SimpleNamespace( exists=False, code=grpc.StatusCode.NOT_FOUND, + delete_delay=0.0, calls=[], closed=[], ) @@ -46,6 +48,7 @@ def delete(request, context): assert request.name == "cleanup-test" assert request.workspace_scope.workspace == "default" assert request.allow_missing + time.sleep(state.delete_delay) if state.code == grpc.StatusCode.NOT_FOUND: return openshell_pb2.DeleteSandboxResponse( outcome=openshell_pb2.DELETION_OUTCOME_ALREADY_ABSENT @@ -158,6 +161,36 @@ def test_context_cleanup_handles_absence_and_intercepted_errors( assert managed._session is None +@pytest.mark.parametrize( + ("delete_timeout_seconds", "expect_deadline"), [(None, True), (5, False)] +) +def test_context_cleanup_delete_timeout_is_opt_in( + cleanup_client, monkeypatch, delete_timeout_seconds, expect_deadline +): + client, state = cleanup_client + state.exists = True + state.delete_delay = 0.5 + client._timeout = 0.2 + monkeypatch.setattr( + SandboxClient, + "from_active_cluster", + classmethod(lambda _cls, **_kwargs: client), + ) + + managed = Sandbox( + workspace="default", + sandbox="cleanup-test", + delete_timeout_seconds=delete_timeout_seconds, + ) + expected = pytest.raises(GatewayError) if expect_deadline else nullcontext() + with expected as caught, managed: + state.exists = False + if caught is not None: + assert caught.value.code() == grpc.StatusCode.DEADLINE_EXCEEDED + assert state.closed == [True] + assert managed._client is None + + class _EntryFailureClient: """Fails the named client call and records close().""" diff --git a/python/openshell/sandbox_test.py b/python/openshell/sandbox_test.py index 019d3cc674..8dcd59e4d1 100644 --- a/python/openshell/sandbox_test.py +++ b/python/openshell/sandbox_test.py @@ -3090,3 +3090,25 @@ def test_sandbox_session_delete_passes_workspace() -> None: assert stub.delete_request is not None assert _request_workspace(stub.delete_request) == "staging" + + +def test_sandbox_session_delete_omits_timeout_for_legacy_clients() -> None: + from openshell.sandbox import SandboxSession + + calls: list[tuple[str, str, bool]] = [] + + class LegacyClient: + def delete(self, name, *, workspace, allow_missing=False): + calls.append((name, workspace, allow_missing)) + + ref = SandboxRef( + id="sandbox-1", + name="job-1", + workspace="staging", + status=SandboxStatusRef(phase=2, current_policy_version=0), + ) + session = SandboxSession(cast("Any", LegacyClient()), ref) + + session.delete(allow_missing=True) + + assert calls == [("job-1", "staging", True)]