refactor(craft): drop redundant sandbox manager singletons (#13627)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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:

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

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()
 

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

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)

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()

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

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

backend/tests/unit/onyx/server/features/craft/test_docker_sandbox_manager_singleton.py

@@ -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