From bb4e162d63d0700e85d89ff5d2cf3e194da3765f Mon Sep 17 00:00:00 2001 From: Danelegend <43459662+Danelegend@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:06:01 -0700 Subject: [PATCH] refactor(craft): drop redundant sandbox manager singletons (#13627) Co-authored-by: Claude Opus 5 (1M context) --- .../sandbox/docker/docker_sandbox_manager.py | 20 +-------- .../server/features/build/sandbox/factory.py | 9 +++- .../kubernetes/kubernetes_sandbox_manager.py | 16 +------ .../craft/test_prompt_slot_distributed.py | 2 +- .../craft/test_provisioning_lock.py | 2 +- .../sandbox/test_docker_serve_plumbing.py | 7 ++- .../sandbox/test_k8s_push_error_mapping.py | 2 +- .../sandbox/test_sandbox_backend_selection.py | 45 +++++++++++++++++-- .../test_docker_sandbox_manager_singleton.py | 36 --------------- 9 files changed, 58 insertions(+), 81 deletions(-) delete mode 100644 backend/tests/unit/onyx/server/features/craft/test_docker_sandbox_manager_singleton.py diff --git a/backend/onyx/server/features/build/sandbox/docker/docker_sandbox_manager.py b/backend/onyx/server/features/build/sandbox/docker/docker_sandbox_manager.py index 9e686454b3..cf1624f355 100644 --- a/backend/onyx/server/features/build/sandbox/docker/docker_sandbox_manager.py +++ b/backend/onyx/server/features/build/sandbox/docker/docker_sandbox_manager.py @@ -628,28 +628,12 @@ def build_container_create_kwargs( class DockerSandboxManager(SandboxManager): """Sandbox manager that drives the host Docker Engine. - Singleton; use :func:`get_sandbox_manager` to obtain the instance. + Process-wide instance is cached by :func:`get_sandbox_manager`. """ - _instance: "DockerSandboxManager | None" = None - _lock = threading.Lock() - supports_opencode_history_persistence = True - def __new__(cls) -> "DockerSandboxManager": - if cls._instance is None: - with cls._lock: - if cls._instance is None: - # Publish to the cache only after _initialize() succeeds, so - # a transient init failure (e.g. the Docker socket briefly - # unavailable) can't leave a half-built singleton that every - # later caller reuses; the next call retries instead. - instance = super().__new__(cls) - instance._initialize() - cls._instance = instance - return cls._instance - - def _initialize(self) -> None: + def __init__(self) -> None: # Mirrors the K8s posture from #11604: the proxy is mandatory whenever # craft is enabled. if not SANDBOX_PROXY_HOST: diff --git a/backend/onyx/server/features/build/sandbox/factory.py b/backend/onyx/server/features/build/sandbox/factory.py index 19453aac2e..7b6acb9281 100644 --- a/backend/onyx/server/features/build/sandbox/factory.py +++ b/backend/onyx/server/features/build/sandbox/factory.py @@ -30,6 +30,9 @@ def get_sandbox_manager() -> SandboxManager: if _sandbox_manager_instance is None: with _sandbox_manager_lock: if _sandbox_manager_instance is None: + # Assign only after the constructor returns, so a failed init + # isn't cached; the next call retries. + manager: SandboxManager if SANDBOX_BACKEND == SandboxBackend.KUBERNETES: # Deferred: avoid loading the kubernetes client stack on # docker deployments (and vice versa). @@ -37,16 +40,18 @@ def get_sandbox_manager() -> SandboxManager: KubernetesSandboxManager, ) - _sandbox_manager_instance = KubernetesSandboxManager() + manager = KubernetesSandboxManager() logger.info("Using KubernetesSandboxManager for sandbox operations") elif SANDBOX_BACKEND == SandboxBackend.DOCKER: from onyx.server.features.build.sandbox.docker.docker_sandbox_manager import ( DockerSandboxManager, ) - _sandbox_manager_instance = DockerSandboxManager() + manager = DockerSandboxManager() logger.info("Using DockerSandboxManager for sandbox operations") else: raise ValueError(f"Unknown sandbox backend: {SANDBOX_BACKEND}") + _sandbox_manager_instance = manager + return _sandbox_manager_instance diff --git a/backend/onyx/server/features/build/sandbox/kubernetes/kubernetes_sandbox_manager.py b/backend/onyx/server/features/build/sandbox/kubernetes/kubernetes_sandbox_manager.py index 3ead4843b2..b8213084da 100644 --- a/backend/onyx/server/features/build/sandbox/kubernetes/kubernetes_sandbox_manager.py +++ b/backend/onyx/server/features/build/sandbox/kubernetes/kubernetes_sandbox_manager.py @@ -47,7 +47,6 @@ import secrets import shlex import tarfile import tempfile -import threading import time from collections.abc import Iterator, Sequence from contextlib import contextmanager @@ -272,23 +271,12 @@ class KubernetesSandboxManager(SandboxManager): IMPORTANT: This manager does NOT interface with the database directly. All database operations should be handled by the caller. - This is a singleton class - use get_sandbox_manager() to get the instance. + Process-wide instance is cached by get_sandbox_manager(). """ supports_opencode_history_persistence = True - _instance: "KubernetesSandboxManager | None" = None - _lock = threading.Lock() - - def __new__(cls) -> "KubernetesSandboxManager": - if cls._instance is None: - with cls._lock: - if cls._instance is None: - cls._instance = super().__new__(cls) - cls._instance._initialize() - return cls._instance - - def _initialize(self) -> None: + def __init__(self) -> None: """Initialize Kubernetes client and configuration.""" load_kube_config() diff --git a/backend/tests/external_dependency_unit/craft/test_prompt_slot_distributed.py b/backend/tests/external_dependency_unit/craft/test_prompt_slot_distributed.py index 53d118d0f4..7437194aeb 100644 --- a/backend/tests/external_dependency_unit/craft/test_prompt_slot_distributed.py +++ b/backend/tests/external_dependency_unit/craft/test_prompt_slot_distributed.py @@ -25,7 +25,7 @@ from onyx.server.features.build.sandbox.kubernetes.kubernetes_sandbox_manager im def _make_replica() -> KubernetesSandboxManager: """A fresh manager (its own state, like a separate pod); skips - ``_initialize`` so no kube config is needed.""" + ``__init__`` so no kube config is needed.""" m: KubernetesSandboxManager = object.__new__(KubernetesSandboxManager) m._init_serve_state() return m diff --git a/backend/tests/external_dependency_unit/craft/test_provisioning_lock.py b/backend/tests/external_dependency_unit/craft/test_provisioning_lock.py index 0fbe2f8bd9..f5ac8e1c52 100644 --- a/backend/tests/external_dependency_unit/craft/test_provisioning_lock.py +++ b/backend/tests/external_dependency_unit/craft/test_provisioning_lock.py @@ -59,7 +59,7 @@ def _make_replica( on_restore: Callable[[], None] | None = None, ) -> KubernetesSandboxManager: """A fresh manager (like a separate api_server pod) with all k8s I/O - faked against the shared cluster state; skips ``_initialize`` so no kube + faked against the shared cluster state; skips ``__init__`` so no kube config is needed. The restore handshake and pod creation are recorded so tests can assert exactly-once semantics.""" m: KubernetesSandboxManager = object.__new__(KubernetesSandboxManager) diff --git a/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_serve_plumbing.py b/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_serve_plumbing.py index 74a2163790..81eb24b074 100644 --- a/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_serve_plumbing.py +++ b/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_serve_plumbing.py @@ -2,7 +2,7 @@ These tests exercise the pure logic added by the docker-sandbox-serve port — no Docker engine required. We use -``object.__new__(DockerSandboxManager)`` to bypass ``_initialize`` +``object.__new__(DockerSandboxManager)`` to bypass ``__init__`` (which would try to open the Docker socket) and verify: - ``_load_serve_connection_info`` produces the expected container-name @@ -48,10 +48,9 @@ def _skip_api_url_probe(monkeypatch: pytest.MonkeyPatch) -> None: def _bare_manager() -> DockerSandboxManager: """ DockerSandboxManager without touching the Docker socket: skips - ``_initialize`` so contributors without docker running can still run + ``__init__`` so contributors without docker running can still run these tests. """ - DockerSandboxManager._instance = None mgr: DockerSandboxManager = object.__new__(DockerSandboxManager) mgr._agent_instructions_template_path = ( # type: ignore[attr-defined] dsm.Path(dsm.__file__).parent.parent.parent / "AGENTS.template.md" @@ -333,7 +332,7 @@ def test_attachments_section_is_inserted_before_connectable_apps( def test_init_serve_state_is_idempotent() -> None: """ - ``_init_serve_state`` is called from ``_initialize``; provision paths must + ``_init_serve_state`` is called from ``__init__``; provision paths must not blow up if called twice. """ mgr = _bare_manager() diff --git a/backend/tests/unit/onyx/server/features/craft/sandbox/test_k8s_push_error_mapping.py b/backend/tests/unit/onyx/server/features/craft/sandbox/test_k8s_push_error_mapping.py index 84fe4965ad..b4b8db8205 100644 --- a/backend/tests/unit/onyx/server/features/craft/sandbox/test_k8s_push_error_mapping.py +++ b/backend/tests/unit/onyx/server/features/craft/sandbox/test_k8s_push_error_mapping.py @@ -106,7 +106,7 @@ def _service_host(mgr: KubernetesSandboxManager) -> Callable[[UUID], str]: def _make_manager() -> KubernetesSandboxManager: - """Construct a manager without invoking _initialize (which needs a K8s config). + """Construct a manager without invoking __init__ (which needs a K8s config). ``write_files_to_sandbox`` resolves the sidecar host via the Service FQDN, then POSTs over the mocked ``httpx.Client``. Bypass ``__new__`` cache with diff --git a/backend/tests/unit/onyx/server/features/craft/sandbox/test_sandbox_backend_selection.py b/backend/tests/unit/onyx/server/features/craft/sandbox/test_sandbox_backend_selection.py index 9ef70e3413..43c7c8a0ab 100644 --- a/backend/tests/unit/onyx/server/features/craft/sandbox/test_sandbox_backend_selection.py +++ b/backend/tests/unit/onyx/server/features/craft/sandbox/test_sandbox_backend_selection.py @@ -30,11 +30,9 @@ def test_docker_backend_returns_docker_manager(monkeypatch: pytest.MonkeyPatch) """Backend=docker must dispatch to ``DockerSandboxManager``, not raise NotImplementedError.""" monkeypatch.setattr(factory_module, "SANDBOX_BACKEND", SandboxBackend.DOCKER) - # Don't try to talk to /var/run/docker.sock in a unit test — stub _initialize. + # Don't try to talk to /var/run/docker.sock in a unit test — stub __init__. from onyx.server.features.build.sandbox.docker import docker_sandbox_manager - monkeypatch.setattr(docker_sandbox_manager.DockerSandboxManager, "_instance", None) - def _fake_init(self: Any) -> None: self._image = "fake" self._network_name = "fake" @@ -46,7 +44,46 @@ def test_docker_backend_returns_docker_manager(monkeypatch: pytest.MonkeyPatch) self._agent_instructions_template_path = Path("/tmp/AGENTS.template.md") # noqa: S108 monkeypatch.setattr( - docker_sandbox_manager.DockerSandboxManager, "_initialize", _fake_init + docker_sandbox_manager.DockerSandboxManager, "__init__", _fake_init ) mgr = factory_module.get_sandbox_manager() assert mgr.__class__.__name__ == "DockerSandboxManager" + + +def test_repeated_calls_return_the_same_instance( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(factory_module, "SANDBOX_BACKEND", SandboxBackend.DOCKER) + from onyx.server.features.build.sandbox.docker import docker_sandbox_manager + + monkeypatch.setattr( + docker_sandbox_manager.DockerSandboxManager, + "__init__", + lambda _self: None, + ) + assert factory_module.get_sandbox_manager() is factory_module.get_sandbox_manager() + + +def test_failed_construction_is_not_cached(monkeypatch: pytest.MonkeyPatch) -> None: + """A failed init must not be cached; the next call retries.""" + monkeypatch.setattr(factory_module, "SANDBOX_BACKEND", SandboxBackend.DOCKER) + from onyx.server.features.build.sandbox.docker import docker_sandbox_manager + + attempts = {"count": 0} + + def _flaky_init(_self: Any) -> None: + attempts["count"] += 1 + if attempts["count"] == 1: + raise RuntimeError("transient docker socket failure") + + monkeypatch.setattr( + docker_sandbox_manager.DockerSandboxManager, "__init__", _flaky_init + ) + + with pytest.raises(RuntimeError, match="transient docker socket failure"): + factory_module.get_sandbox_manager() + assert factory_module._sandbox_manager_instance is None + + mgr = factory_module.get_sandbox_manager() + assert mgr is factory_module._sandbox_manager_instance + assert attempts["count"] == 2 diff --git a/backend/tests/unit/onyx/server/features/craft/test_docker_sandbox_manager_singleton.py b/backend/tests/unit/onyx/server/features/craft/test_docker_sandbox_manager_singleton.py deleted file mode 100644 index 8dae9525fe..0000000000 --- a/backend/tests/unit/onyx/server/features/craft/test_docker_sandbox_manager_singleton.py +++ /dev/null @@ -1,36 +0,0 @@ -from unittest.mock import patch - -import pytest - -from onyx.server.features.build.sandbox.docker.docker_sandbox_manager import ( - DockerSandboxManager, -) - - -def test_failed_initialize_does_not_poison_singleton() -> None: - """A failed ``_initialize()`` must not cache a half-built singleton: ``__new__`` - publishes to the class cache only after init succeeds, and a later construction - retries once the transient condition clears.""" - DockerSandboxManager._instance = None - try: - attempts = {"count": 0} - - def flaky_initialize(_self: DockerSandboxManager) -> None: - attempts["count"] += 1 - if attempts["count"] == 1: - raise RuntimeError("transient docker socket failure") - # second attempt succeeds (no-op) - - with patch.object(DockerSandboxManager, "_initialize", flaky_initialize): - with pytest.raises(RuntimeError, match="transient docker socket failure"): - DockerSandboxManager() - - # failed init left nothing cached - assert DockerSandboxManager._instance is None - - # next construction retries and becomes the cached instance - manager = DockerSandboxManager() - assert manager is DockerSandboxManager._instance - assert attempts["count"] == 2 - finally: - DockerSandboxManager._instance = None