fix(store): one local registry container per port, never removed

src/agent_env/store/image_store/local_registry_image_store.py

@@ -10,6 +10,13 @@
 from agent_env.store.image_store.oci_registry_credentials import OciRegistryCredentials
 
 _REGISTRY_CONTAINER = "agentenv-registry"
+_DEFAULT_PORT = 5000
+
+
+def _container_name(port: int) -> str:
+    """``agentenv-registry`` on the default port, ``agentenv-registry-<port>`` on any other, so
+    registries on different ports coexist and each restarts with its own images."""
+    return _REGISTRY_CONTAINER if port == _DEFAULT_PORT else f"{_REGISTRY_CONTAINER}-{port}"
 
 
 class LocalRegistryImageStore(OciRegistryImageStore):
@@ -34,37 +41,34 @@ def ensure_repository(self, repository: str) -> None:
         """A local registry creates repositories on push, so this instead lazily brings up the
         backing ``registry:2`` — idempotently, and safe to call from many concurrent pushes. A
         no-op when something already serves the registry at ``registry_host`` (a registry you
-        run yourself, or one a concurrent push already started); it never removes a container
-        that might be serving. Called from the image-push path, so no caller manages it."""
-        port = urlsplit(f"//{self.registry_host}").port or 5000
+        run yourself, or one a concurrent push already started); it never removes a container.
+        Called from the image-push path, so no caller manages it."""
+        port = urlsplit(f"//{self.registry_host}").port or _DEFAULT_PORT
         if self._registry_listening(port):
             return
-        run = self._run_registry(port)
+        name = _container_name(port)
+        run = self._run_registry(name, port)
         if run.returncode != 0:
             # `docker run` creates the named container atomically, so a failure is either a name
             # conflict (our container already exists) or something unrelated — disambiguate on
             # the existing container instead of blindly retrying, so a real error isn't masked.
-            existing_port = self._container_configured_port(_REGISTRY_CONTAINER)
+            existing_port = self._container_configured_port(name)
             if existing_port is None:
                 # No such container: the failure is real — the port is taken by another process,
                 # the image pull failed, or the daemon is down. Surface it now, don't wait 30s.
                 raise RuntimeError(f"could not start the local registry: {run.stderr.strip()}")
             if existing_port != port:
-                # A stale container of ours on a different port (registry_host changed since it
-                # was created). It isn't serving our port — the check above proved that — so
-                # replacing it is safe.
-                subprocess.run(["docker", "rm", "-f", _REGISTRY_CONTAINER], capture_output=True, text=True)
-                run = self._run_registry(port)
-                if run.returncode != 0:
-                    raise RuntimeError(f"could not start the local registry: {run.stderr.strip()}")
-            else:
-                # A concurrent push won the race, or a stopped container of ours is on our port:
-                # (re)start it (a no-op if it's already running) and surface a genuine failure.
-                start = subprocess.run(["docker", "start", _REGISTRY_CONTAINER], capture_output=True, text=True)
-                if start.returncode != 0:
-                    raise RuntimeError(
-                        f"local registry container exists but could not be started: {start.stderr.strip()}"
-                    )
+                # Only an older version's agentenv-registry, which it ran on any port; refuse rather than remove it.
+                raise RuntimeError(
+                    f"{name} is using port {existing_port}, not {port}: "
+                    f"point registry_host at port {existing_port}, or remove the container"
+                )
+            # A concurrent push won the race, or our stopped container is on this port: (re)start it.
+            start = subprocess.run(["docker", "start", name], capture_output=True, text=True)
+            if start.returncode != 0:
+                raise RuntimeError(
+                    f"local registry container exists but could not be started: {start.stderr.strip()}"
+                )
         deadline = time.time() + 30
         while time.time() < deadline:
             if self._registry_listening(port):
@@ -73,9 +77,9 @@ def ensure_repository(self, repository: str) -> None:
         raise RuntimeError(f"the local registry started but did not answer /v2/ on port {port} within 30s")
 
     @staticmethod
-    def _run_registry(port: int) -> subprocess.CompletedProcess:
+    def _run_registry(name: str, port: int) -> subprocess.CompletedProcess:
         return subprocess.run(
-            ["docker", "run", "-d", "--restart", "unless-stopped", "--name", _REGISTRY_CONTAINER,
+            ["docker", "run", "-d", "--restart", "unless-stopped", "--name", name,
              "-p", f"127.0.0.1:{port}:5000", "registry:2"],
             capture_output=True, text=True,
         )

tst/unit/store/local_registry_image_store_test.py

@@ -0,0 +1,95 @@
+"""LocalRegistryImageStore's lazy registry bring-up, with docker and the /v2/ ping faked."""
+
+import subprocess
+
+import pytest
+
+from agent_env.store.image_store import local_registry_image_store
+from agent_env.store.image_store.local_registry_image_store import LocalRegistryImageStore
+
+
+class _Docker:
+    """Answers the docker commands bring-up issues and records each one."""
+
+    def __init__(self, *, run_rc=0, existing_port=None, start_rc=0):
+        self.run_rc, self.existing_port, self.start_rc = run_rc, existing_port, start_rc
+        self.calls: list[list[str]] = []
+
+    def __call__(self, argv, **kwargs):
+        self.calls.append(argv)
+        command = argv[1]
+        if command == "run":
+            return subprocess.CompletedProcess(argv, self.run_rc, "", "Conflict" if self.run_rc else "")
+        if command == "inspect":
+            found = self.existing_port is not None
+            return subprocess.CompletedProcess(argv, 0 if found else 1, str(self.existing_port or ""), "")
+        if command == "start":
+            return subprocess.CompletedProcess(argv, self.start_rc, "", "boom" if self.start_rc else "")
+        raise AssertionError(f"unexpected docker call: {argv}")
+
+
+@pytest.fixture
+def docker(monkeypatch):
+    def install(**kwargs):
+        fake = _Docker(**kwargs)
+        monkeypatch.setattr(local_registry_image_store.subprocess, "run", fake)
+        return fake
+    return install
+
+
+@pytest.fixture
+def listening(monkeypatch):
+    """The registry answers only after bring-up has run a docker command."""
+    def install(fake):
+        monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: bool(fake.calls)))
+        monkeypatch.setattr(local_registry_image_store.time, "sleep", lambda seconds: None)
+    return install
+
+
+def test_the_default_port_runs_agentenv_registry(docker, listening):
+    fake = docker()
+    listening(fake)
+    LocalRegistryImageStore("localhost:5000").ensure_repository("r")
+    assert fake.calls == [["docker", "run", "-d", "--restart", "unless-stopped", "--name", "agentenv-registry",
+                           "-p", "127.0.0.1:5000:5000", "registry:2"]]
+
+
+def test_another_port_runs_its_own_container(docker, listening):
+    fake = docker()
+    listening(fake)
+    LocalRegistryImageStore("localhost:5001").ensure_repository("r")
+    assert fake.calls == [["docker", "run", "-d", "--restart", "unless-stopped", "--name", "agentenv-registry-5001",
+                           "-p", "127.0.0.1:5001:5000", "registry:2"]]
+
+
+def test_a_serving_registry_is_left_alone(docker, monkeypatch):
+    fake = docker()
+    monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: True))
+    LocalRegistryImageStore("localhost:5000").ensure_repository("r")
+    assert fake.calls == []
+
+
+@pytest.mark.parametrize("port, name", [(5000, "agentenv-registry"), (5001, "agentenv-registry-5001")])
+def test_a_stopped_container_on_this_port_is_started(docker, listening, port, name):
+    fake = docker(run_rc=125, existing_port=port)
+    listening(fake)
+    LocalRegistryImageStore(f"localhost:{port}").ensure_repository("r")
+    assert [call[1] for call in fake.calls] == ["run", "inspect", "start"]
+    assert fake.calls[1][-1] == name
+    assert fake.calls[-1] == ["docker", "start", name]
+
+
+def test_an_older_agentenv_registry_on_another_port_is_refused_not_removed(docker, monkeypatch):
+    fake = docker(run_rc=125, existing_port=5001)
+    monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: False))
+    with pytest.raises(RuntimeError, match="agentenv-registry is using port 5001, not 5000"):
+        LocalRegistryImageStore("localhost:5000").ensure_repository("r")
+    assert [call[1] for call in fake.calls] == ["run", "inspect"]
+
+
+def test_a_failed_run_with_no_container_surfaces_dockers_error(docker, monkeypatch):
+    fake = docker(run_rc=125)
+    monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: False))
+    with pytest.raises(RuntimeError, match="could not start the local registry: Conflict"):
+        LocalRegistryImageStore("localhost:5000").ensure_repository("r")
+    assert all(call[1] != "rm" for call in fake.calls)