From 4fe095c7a4b19382e10acbb4ef4064013191466a Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 20 Aug 2026 00:14:54 +0100 Subject: [PATCH] fix: daytona timeouts --- application/requirements.txt | 2 +- application/sandbox/base.py | 11 ++ application/sandbox/daytona.py | 111 +++++++++++++----- application/sandbox/manager.py | 14 ++- tests/sandbox/test_daytona.py | 155 +++++++++++++++++++++++++- tests/sandbox/test_sandbox_manager.py | 24 ++++ 6 files changed, 287 insertions(+), 30 deletions(-) diff --git a/application/requirements.txt b/application/requirements.txt index bd368613..d914ef83 100644 --- a/application/requirements.txt +++ b/application/requirements.txt @@ -10,7 +10,7 @@ celery-redbeat==2.4.2 croniter==6.2.4 cryptography==50.0.0 dataclasses-json==0.6.7 -daytona==0.203.0 +daytona==0.205.1 defusedxml==0.7.1 docling==2.119.0 rapidocr==3.9.2 diff --git a/application/sandbox/base.py b/application/sandbox/base.py index 3fa892ce..e551d697 100644 --- a/application/sandbox/base.py +++ b/application/sandbox/base.py @@ -47,6 +47,17 @@ class ExecResult: return self.status == "ok" +class SandboxGoneError(IOError): + """The cloud runtime behind a session no longer exists (deleted upstream). + + File-operation counterpart of ``ExecResult.runtime_invalidated``: backends + raise it (instead of a plain ``IOError``) when a put/get/list failed because + the runtime is gone, so managers can drop their cached session and the next + ``open`` performs a cold start instead of replaying the dead handle. + Subclasses ``IOError`` so callers that only handle ``IOError`` keep working. + """ + + class CodeSandbox(ABC): """Common interface every sandbox backend (Jupyter, Daytona, ...) implements.""" diff --git a/application/sandbox/daytona.py b/application/sandbox/daytona.py index 37fd7682..a4ec1f9d 100644 --- a/application/sandbox/daytona.py +++ b/application/sandbox/daytona.py @@ -10,6 +10,7 @@ from application.sandbox.base import ( CodeSandbox, ExecResult, Plot, + SandboxGoneError, ) logger = logging.getLogger(__name__) @@ -150,8 +151,17 @@ class DaytonaSandbox(CodeSandbox): # instead of leaking it behind a brand-new create. reattached = self._reattach_existing(session_id) if reattached is not None: - self._prime(reattached) - return reattached.sandbox_id + if self._prime(reattached): + return reattached.sandbox_id + # The labelled sandbox is gone in the cloud (deleted between the + # list and its first use — seen in prod as toolbox 404 "it has + # been deleted"). Forget it and fall through to a fresh create. + logger.warning( + "Reattached Daytona sandbox %s for session %s is gone; creating a fresh one", + reattached.sandbox_id, + session_id, + ) + self._forget_handle(session_id, reattached) with self._lock: if len(self._handles) >= self._max_sandboxes: @@ -194,7 +204,7 @@ class DaytonaSandbox(CodeSandbox): labels={_SESSION_LABEL: session_id}, states=[SandboxState.STARTED, SandboxState.STOPPED], ) - matches = list(self._client.list(query)) + matches = list(self._client.list(query, request_timeout=self._default_timeout)) except Exception as exc: # noqa: BLE001 - listing must never block opening a session logger.warning("Daytona list for session %s failed; will create fresh: %s", session_id, exc) return None @@ -227,7 +237,7 @@ class DaytonaSandbox(CodeSandbox): def _ensure_started(self, handle: "_Handle") -> bool: """Refresh the handle's sandbox; start it if auto-stopped. True only if it was woken.""" try: - fresh = self._client.get(handle.sandbox_id) + fresh = self._client.get(handle.sandbox_id, request_timeout=self._default_timeout) except Exception as exc: # noqa: BLE001 - a failed refresh just means we don't retry logger.warning("Daytona get for %s failed while ensuring started: %s", handle.sandbox_id, exc) return False @@ -316,7 +326,7 @@ class DaytonaSandbox(CodeSandbox): evict-then-concurrent-reopen path where only the captured id is still known. """ try: - sandbox = self._client.get(sandbox_id) + sandbox = self._client.get(sandbox_id, request_timeout=self._default_timeout) self._client.delete(sandbox) except Exception as exc: # noqa: BLE001 - teardown is best-effort, never raise logger.warning("Failed to delete Daytona sandbox by id %s: %s", sandbox_id, exc) @@ -340,7 +350,7 @@ class DaytonaSandbox(CodeSandbox): def _delete_remote(self, handle: "_Handle", remote: str) -> None: """Recursively delete ``remote`` inside the sandbox, with a contained-command fallback.""" try: - handle.sandbox.fs.delete_file(remote, recursive=True) + handle.sandbox.fs.delete_file(remote, recursive=True, request_timeout=self._default_timeout) return except TypeError: # Older SDK without the recursive kwarg: fall back to a contained rm -rf. @@ -351,12 +361,43 @@ class DaytonaSandbox(CodeSandbox): quoted = "'" + remote.replace("'", "'\\''") + "'" handle.sandbox.process.exec(f"rm -rf {quoted}", timeout=int(self._default_timeout)) - def _prime(self, handle: _Handle) -> None: - """Create the per-session workspace directory inside the sandbox.""" + def _prime(self, handle: _Handle) -> bool: + """Create the per-session workspace directory inside the sandbox (bounded). + + The toolbox proxy of a just-created/just-woken sandbox can accept a + request and never answer (no container IP registered yet), and the SDK + default is NO client timeout — in prod one hung ``create_folder`` here + pinned a stream for ~16 minutes until the idle auto-stop dropped the + connection. Bound the call, refresh/wake the sandbox, and retry once. + + ``create_folder`` is idempotent on an existing directory (verified + against Daytona 0.205.1), so any failure here is real. Returns False + when the sandbox no longer exists in the cloud — the caller must + discard the handle and start fresh. A still-alive sandbox that fails + to prime stays usable (True): ``exec`` prepends ``makedirs`` and + ``put_file`` creates parent dirs, so the workspace materializes on + first use anyway. + """ try: - handle.sandbox.fs.create_folder(handle.workspace, "755") - except Exception as exc: # noqa: BLE001 - folder may already exist on a reused snapshot - logger.debug("Workspace folder prime returned: %s", exc) + handle.sandbox.fs.create_folder(handle.workspace, "755", request_timeout=self._default_timeout) + return True + except Exception as exc: # noqa: BLE001 - hang/transport/stopped: refresh and retry once + logger.debug("Workspace folder prime failed; refreshing sandbox and retrying once: %s", exc) + self._ensure_started(handle) # refreshes the handle; wakes an auto-stopped sandbox + try: + handle.sandbox.fs.create_folder(handle.workspace, "755", request_timeout=self._default_timeout) + return True + except Exception as exc: # noqa: BLE001 - classify gone-vs-degraded below + if self._sandbox_gone(handle): + logger.warning("Workspace prime found sandbox %s gone: %s", handle.sandbox_id, exc) + return False + logger.warning( + "Workspace prime failed twice for live sandbox %s " + "(continuing; exec/put_file create the workspace on demand): %s", + handle.sandbox_id, + exc, + ) + return True # -- Execution ------------------------------------------------------- @@ -401,7 +442,7 @@ class DaytonaSandbox(CodeSandbox): handle). A ``get`` that cannot resolve the id, or a terminal state, means gone. """ try: - fresh = self._client.get(handle.sandbox_id) + fresh = self._client.get(handle.sandbox_id, request_timeout=self._default_timeout) except Exception: # noqa: BLE001 - unresolvable id => the sandbox is gone return True state = getattr(fresh, "state", None) @@ -511,14 +552,13 @@ class DaytonaSandbox(CodeSandbox): return except Exception: # noqa: BLE001 - second failure -> generic IOError below pass - logger.warning("put_file failed for %r: %s", dest_path, exc) - raise IOError(f"put_file failed: {type(exc).__name__}") from exc + self._raise_file_error("put_file", dest_path, session_id, handle, exc) def _upload(self, handle: "_Handle", remote: str, parent: str, data: bytes) -> None: """Create the parent folder (best-effort) and upload ``data`` to ``remote``.""" if parent and parent != handle.workspace: try: - handle.sandbox.fs.create_folder(parent, "755") + handle.sandbox.fs.create_folder(parent, "755", request_timeout=self._default_timeout) except Exception as folder_exc: # noqa: BLE001 - folder may already exist logger.debug("put_file parent folder create returned: %s", folder_exc) handle.sandbox.fs.upload_file(data, remote) @@ -534,15 +574,13 @@ class DaytonaSandbox(CodeSandbox): except Exception as exc: # noqa: BLE001 - log detail server-side, return a generic error # A cached handle may point at an auto-stopped sandbox; wake it and retry once. if not self._ensure_started(handle): - logger.warning("get_file failed for %r: %s", path, exc) - raise IOError(f"get_file failed: {type(exc).__name__}") from exc + self._raise_file_error("get_file", path, session_id, handle, exc) try: data = self._download(handle, remote) except IOError: raise - except Exception: # noqa: BLE001 - second failure -> generic IOError - logger.warning("get_file failed for %r: %s", path, exc) - raise IOError(f"get_file failed: {type(exc).__name__}") from exc + except Exception: # noqa: BLE001 - second failure -> classified below + self._raise_file_error("get_file", path, session_id, handle, exc) if data is None: raise IOError(f"get_file produced no payload for {path!r}") data = data if isinstance(data, bytes) else bytes(data) @@ -552,9 +590,29 @@ class DaytonaSandbox(CodeSandbox): raise IOError(f"file too large: {len(data)} > {self._max_file_bytes} bytes") return data + def _raise_file_error(self, op: str, path: str, session_id: str, handle: "_Handle", exc: Exception) -> None: + """Classify and raise a failed file op: ``SandboxGoneError`` vs plain ``IOError``. + + A deleted-but-cached sandbox (auto-delete, or a stale reattach candidate + the cloud list still returned) would otherwise 404 every subsequent call + on this handle. Forget the handle so the next ``open`` reattaches or + creates fresh, and raise ``SandboxGoneError`` so the manager drops its + session too (the file-op mirror of ``exec``'s ``runtime_invalidated``). + A failure on a still-live sandbox stays a plain ``IOError`` and keeps + the handle. Always raises; error text carries only the exception type, + never backend URLs/payloads. + """ + logger.warning("%s failed for %r: %s", op, path, exc) + if self._sandbox_gone(handle): + self._forget_handle(session_id, handle) + raise SandboxGoneError(f"{op} failed: sandbox gone ({type(exc).__name__})") from exc + if isinstance(exc, IOError): + raise exc + raise IOError(f"{op} failed: {type(exc).__name__}") from exc + def _download(self, handle: "_Handle", remote: str) -> object: """Fetch ``remote``'s bytes, rejecting a file whose declared size exceeds the cap.""" - info = handle.sandbox.fs.get_file_info(remote) + info = handle.sandbox.fs.get_file_info(remote, request_timeout=self._default_timeout) size = getattr(info, "size", None) if size is not None and size > self._max_file_bytes: raise IOError(f"file too large: {size} > {self._max_file_bytes} bytes") @@ -565,11 +623,14 @@ class DaytonaSandbox(CodeSandbox): handle = self._get_handle(session_id) try: return self._list_all(handle) - except Exception: # noqa: BLE001 - transport/stopped fault: wake and retry once, else re-raise + except Exception as exc: # noqa: BLE001 - transport/stopped fault: wake and retry once # A cached handle may point at an auto-stopped sandbox; wake it and retry once. if self._ensure_started(handle): - return self._list_all(handle) - raise + try: + return self._list_all(handle) + except Exception: # noqa: BLE001 - second failure -> classified below + pass + self._raise_file_error("list_files", ".", session_id, handle, exc) def _list_all(self, handle: "_Handle") -> List[str]: """Walk the workspace subtree for ``handle`` and return workspace-relative file paths.""" @@ -581,7 +642,7 @@ class DaytonaSandbox(CodeSandbox): """Recurse one workspace subtree, appending workspace-relative file paths to ``out``.""" abs_dir = posixpath.join(root, rel_dir) if rel_dir else root try: - entries = sandbox.fs.list_files(abs_dir) + entries = sandbox.fs.list_files(abs_dir, request_timeout=self._default_timeout) except Exception as exc: # noqa: BLE001 - log detail server-side, return a generic error logger.warning("list_files failed for %r: %s", rel_dir or ".", exc) raise IOError(f"list_files failed: {type(exc).__name__}") from exc diff --git a/application/sandbox/manager.py b/application/sandbox/manager.py index 9007e2f5..fb08c442 100644 --- a/application/sandbox/manager.py +++ b/application/sandbox/manager.py @@ -6,7 +6,7 @@ import time from dataclasses import dataclass, field from typing import Dict, List, Optional, Tuple -from application.sandbox.base import CodeSandbox, ExecResult +from application.sandbox.base import CodeSandbox, ExecResult, SandboxGoneError logger = logging.getLogger(__name__) @@ -208,6 +208,12 @@ class SandboxManager: session = self._enter(session_id) try: self._backend.put_file(session_id, dest_path, data) + except SandboxGoneError: + # File-op mirror of ``exec``'s runtime_invalidated: the cloud runtime + # is gone, so drop the cached session — the next open cold-starts + # instead of replaying the dead handle into more failures. + self._drop_invalidated_session(session_id, session) + raise finally: self._leave(session_id, expected=session) @@ -216,6 +222,9 @@ class SandboxManager: session = self._enter(session_id) try: return self._backend.get_file(session_id, path) + except SandboxGoneError: + self._drop_invalidated_session(session_id, session) + raise finally: self._leave(session_id, expected=session) @@ -224,6 +233,9 @@ class SandboxManager: session = self._enter(session_id) try: return self._backend.list_files(session_id) + except SandboxGoneError: + self._drop_invalidated_session(session_id, session) + raise finally: self._leave(session_id, expected=session) diff --git a/tests/sandbox/test_daytona.py b/tests/sandbox/test_daytona.py index 658607e8..aeef2e4a 100644 --- a/tests/sandbox/test_daytona.py +++ b/tests/sandbox/test_daytona.py @@ -55,7 +55,7 @@ class _FakeFS: self.tree = {} self.list_files = mock.Mock(side_effect=self._list_files) - def _list_files(self, path): + def _list_files(self, path, depth=None, request_timeout=None): if self.tree: return self.tree.get(path, []) return [_FakeFileInfo("a.txt")] @@ -84,7 +84,7 @@ class _FakeDaytonaClient: self.start = mock.Mock(side_effect=self._start) self.get = mock.Mock(side_effect=self._get) - def _get(self, sandbox_id_or_name): + def _get(self, sandbox_id_or_name, request_timeout=None): for _, sandbox in self.created: if sandbox.id == sandbox_id_or_name: return sandbox @@ -102,7 +102,7 @@ class _FakeDaytonaClient: def _delete(self, sandbox, timeout=60): self.deleted.append(sandbox) - def _list(self, query=None): + def _list(self, query=None, request_timeout=None): wanted = getattr(query, "labels", None) or {} for sandbox in self.existing: labels = getattr(sandbox, "labels", {}) @@ -822,3 +822,152 @@ def test_manager_cold_starts_after_daytona_sandbox_deleted(fake_sdk): mgr.open("conv-1") # recovery assert backend._client.create.call_count == 2 # a NEW sandbox, not the tombstone + + +# --- #46: bounded toolbox calls + gone-sandbox hygiene -------------------- +# Prod evidence (2026-08-19): the Daytona toolbox proxy can accept a request +# for a just-created/woken sandbox and never answer (no container IP yet), and +# the SDK's fs metadata calls default to NO client timeout — one hung +# create_folder pinned a stream for ~16 min until auto-stop dropped the +# connection. Every toolbox/control-plane call must carry request_timeout, and +# an operation against a deleted-but-still-listed sandbox must invalidate the +# handle instead of burning a failed round per call. + + +def test_prime_passes_request_timeout(sandbox): + """The workspace prime (the exact call seen hanging in prod) is bounded.""" + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + assert created.fs.create_folder.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_prime_retries_once_after_transport_error(sandbox): + """A hung/failed prime is retried once (bounded) instead of being dropped.""" + from application.sandbox.daytona import DaytonaSandbox # noqa: F401 - fixture import parity + + client_cls = type(sandbox._client) + orig_create = client_cls._create + + def _create(client_self, params=None, timeout=60): + sb = orig_create(client_self, params, timeout) + sb.fs.create_folder.side_effect = [RuntimeError("read timed out"), None] + return sb + + with mock.patch.object(client_cls, "_create", _create): + sandbox._client.create = mock.Mock(side_effect=lambda p=None, timeout=60: _create(sandbox._client, p, timeout)) + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + assert created.fs.create_folder.call_count == 2 + + +def test_upload_parent_folder_passes_request_timeout(sandbox): + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + created.fs.create_folder.reset_mock() + sandbox.put_file("conv-1", "out/data.csv", b"x") + assert created.fs.create_folder.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_download_metadata_passes_request_timeout(sandbox): + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + sandbox.get_file("conv-1", "a.txt") + assert created.fs.get_file_info.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_list_files_walk_passes_request_timeout(sandbox): + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + sandbox.list_files("conv-1") + assert created.fs.list_files.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_remove_path_delete_passes_request_timeout(sandbox): + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + sandbox.remove_path("conv-1", "artifacts/tok") + assert created.fs.delete_file.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_ensure_started_get_passes_request_timeout(sandbox): + sandbox.open("conv-1") + handle = sandbox._handles["conv-1"] + sandbox._ensure_started(handle) + assert sandbox._client.get.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_reattach_list_passes_request_timeout(fake_sdk): + from application.sandbox.daytona import DaytonaSandbox + + box = DaytonaSandbox(api_key="dtn_test", language="python") + existing = _FakeSandbox("sbx-old", labels={"docsgpt_session_id": "conv-1"}) + box._client.existing.append(existing) + box.open("conv-1") + assert box._client.list.call_args.kwargs.get("request_timeout") == 60.0 + + +def test_open_reattach_gone_falls_through_to_fresh_create(fake_sdk): + """A labelled-but-deleted sandbox (prod: 404 'it has been deleted') must not + win reattach: prime detects it is gone and open() creates a fresh one.""" + from application.sandbox.daytona import DaytonaSandbox + + box = DaytonaSandbox(api_key="dtn_test", language="python") + ghost = _FakeSandbox("sbx-ghost", labels={"docsgpt_session_id": "conv-1"}) + ghost.fs.create_folder = mock.Mock(side_effect=RuntimeError("not found: sandbox sbx-ghost")) + box._client.existing.append(ghost) + box._client.get = mock.Mock(side_effect=KeyError("gone")) # control plane: deleted + + handle_id = box.open("conv-1") + + assert handle_id == "sbx-1" # the fresh create, not the ghost + assert box._client.create.call_count == 1 + assert box._handles["conv-1"].sandbox_id == "sbx-1" + + +def test_put_file_gone_sandbox_raises_sandbox_gone_and_forgets(sandbox): + from application.sandbox.base import SandboxGoneError + + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + created.fs.upload_file.side_effect = RuntimeError("not found (it has been deleted)") + sandbox._client.get = mock.Mock(side_effect=KeyError("gone")) + with pytest.raises(SandboxGoneError): + sandbox.put_file("conv-1", "data.csv", b"x") + assert "conv-1" not in sandbox._handles + + +def test_get_file_gone_sandbox_raises_sandbox_gone_and_forgets(sandbox): + from application.sandbox.base import SandboxGoneError + + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + created.fs.get_file_info.side_effect = RuntimeError("not found (it has been deleted)") + sandbox._client.get = mock.Mock(side_effect=KeyError("gone")) + with pytest.raises(SandboxGoneError): + sandbox.get_file("conv-1", "a.txt") + assert "conv-1" not in sandbox._handles + + +def test_list_files_gone_sandbox_raises_sandbox_gone_and_forgets(sandbox): + from application.sandbox.base import SandboxGoneError + + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + created.fs.list_files.side_effect = RuntimeError("not found (it has been deleted)") + sandbox._client.get = mock.Mock(side_effect=KeyError("gone")) + with pytest.raises(SandboxGoneError): + sandbox.list_files("conv-1") + assert "conv-1" not in sandbox._handles + + +def test_put_file_alive_sandbox_keeps_plain_ioerror_and_handle(sandbox): + """A transport error on a LIVE sandbox stays a plain IOError; handle kept.""" + from application.sandbox.base import SandboxGoneError + + sandbox.open("conv-1") + _, created = sandbox._client.created[0] + created.fs.upload_file.side_effect = RuntimeError("transient") + with pytest.raises(IOError) as exc: + sandbox.put_file("conv-1", "data.csv", b"x") + assert not isinstance(exc.value, SandboxGoneError) + assert "conv-1" in sandbox._handles diff --git a/tests/sandbox/test_sandbox_manager.py b/tests/sandbox/test_sandbox_manager.py index 60533bba..7e47c16e 100644 --- a/tests/sandbox/test_sandbox_manager.py +++ b/tests/sandbox/test_sandbox_manager.py @@ -742,3 +742,27 @@ def test_exec_completes_despite_concurrent_close(): # The deferred close ran on the exec's own _leave. assert not mgr.has_session("conv-1") assert backend.torn_down == ["conv-1"] + + +def test_sandbox_gone_during_file_op_drops_session_and_next_open_is_cold(): + """#46 hygiene: a file op hitting a deleted cloud sandbox must invalidate the + manager session too, so the next open cold-opens instead of replaying the + cached handle into 404s.""" + from application.sandbox.base import SandboxGoneError + + class _GoneOnPutBackend(FakeBackend): + def put_file(self, session_id, dest_path, data): + self._handles.pop(session_id, None) # backend already forgot its handle + raise SandboxGoneError("put_file failed: sandbox gone") + + backend = _GoneOnPutBackend() + mgr = SandboxManager(backend, max_ttl=600) + old_handle = mgr.open("conv-1") + + with pytest.raises(SandboxGoneError): + mgr.put_file("conv-1", "a.txt", b"x") + + assert not mgr.has_session("conv-1") + new_handle = mgr.open("conv-1") + assert new_handle != old_handle + assert backend.open_calls == ["conv-1", "conv-1"]