From d0dcd7501f15c404f2af507efd5f32320c1ff338 Mon Sep 17 00:00:00 2001 From: Tejas Polakam Date: Thu, 1 Oct 2026 19:11:02 -0400 Subject: [PATCH 1/2] fix(store): refuse, never remove, a local registry container on another port When registry_host named a port other than the one the `agentenv-registry` container publishes, bring-up ran `docker rm -f` on it and started a fresh one, deleting every image pushed to the old registry. It now stops with an error naming both ports and the two ways out: point registry_host at the container's port, or remove the container yourself. Nothing else changes: a registry already answering is used, a missing one is created, and a stopped one on the right port is started. Co-Authored-By: Claude Opus 5.5 --- .../image_store/local_registry_image_store.py | 29 +++--- .../store/local_registry_image_store_test.py | 95 +++++++++++++++++++ 2 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 tst/unit/store/local_registry_image_store_test.py diff --git a/src/agent_env/store/image_store/local_registry_image_store.py b/src/agent_env/store/image_store/local_registry_image_store.py index e3f06cc9..e2242d6b 100644 --- a/src/agent_env/store/image_store/local_registry_image_store.py +++ b/src/agent_env/store/image_store/local_registry_image_store.py @@ -34,8 +34,8 @@ 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.""" + 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 5000 if self._registry_listening(port): return @@ -50,21 +50,16 @@ def ensure_repository(self, repository: str) -> None: # 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()}" - ) + raise RuntimeError( + f"{_REGISTRY_CONTAINER} 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", _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()}" + ) deadline = time.time() + 30 while time.time() < deadline: if self._registry_listening(port): diff --git a/tst/unit/store/local_registry_image_store_test.py b/tst/unit/store/local_registry_image_store_test.py new file mode 100644 index 00000000..158d3aa4 --- /dev/null +++ b/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_the_same_container_on_that_port(docker, listening): + fake = docker() + listening(fake) + LocalRegistryImageStore("localhost:5001").ensure_repository("r") + assert fake.calls[0][fake.calls[0].index("--name") + 1] == "agentenv-registry" + assert "127.0.0.1:5001:5000" in fake.calls[0] + + +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", [5000, 5001]) +def test_a_stopped_container_on_this_port_is_started(docker, listening, port): + 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] == ["docker", "start", "agentenv-registry"] + + +@pytest.mark.parametrize("existing, wanted", [(5000, 5001), (5001, 5000)]) +def test_a_container_on_another_port_is_refused_not_removed(docker, monkeypatch, existing, wanted): + fake = docker(run_rc=125, existing_port=existing) + monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: False)) + with pytest.raises(RuntimeError, match=f"agentenv-registry is using port {existing}, not {wanted}"): + LocalRegistryImageStore(f"localhost:{wanted}").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) From 66330c7a6664441b49be2f07423e1604972c2eb3 Mon Sep 17 00:00:00 2001 From: Tejas Polakam Date: Sat, 3 Oct 2026 10:18:49 -0400 Subject: [PATCH 2/2] fix(store): name the local registry container after its port agentenv-registry stays the default port's container; any other port gets agentenv-registry-, so registries on different ports coexist instead of the second being refused, and each restarts by name with its own images. The port-mismatch refusal remains for an older version's agentenv-registry, which it ran on any port. Co-Authored-By: Claude Opus 5.5 --- .../image_store/local_registry_image_store.py | 23 +++++++++++++------ .../store/local_registry_image_store_test.py | 22 +++++++++--------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/agent_env/store/image_store/local_registry_image_store.py b/src/agent_env/store/image_store/local_registry_image_store.py index e2242d6b..b9f91868 100644 --- a/src/agent_env/store/image_store/local_registry_image_store.py +++ b/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-`` 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): @@ -36,26 +43,28 @@ def ensure_repository(self, repository: str) -> None: 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. Called from the image-push path, so no caller manages it.""" - port = urlsplit(f"//{self.registry_host}").port or 5000 + 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: + # Only an older version's agentenv-registry, which it ran on any port; refuse rather than remove it. raise RuntimeError( - f"{_REGISTRY_CONTAINER} is using port {existing_port}, not {port}: " + 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", _REGISTRY_CONTAINER], capture_output=True, text=True) + 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()}" @@ -68,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, ) diff --git a/tst/unit/store/local_registry_image_store_test.py b/tst/unit/store/local_registry_image_store_test.py index 158d3aa4..0712e96d 100644 --- a/tst/unit/store/local_registry_image_store_test.py +++ b/tst/unit/store/local_registry_image_store_test.py @@ -54,12 +54,12 @@ def test_the_default_port_runs_agentenv_registry(docker, listening): "-p", "127.0.0.1:5000:5000", "registry:2"]] -def test_another_port_runs_the_same_container_on_that_port(docker, listening): +def test_another_port_runs_its_own_container(docker, listening): fake = docker() listening(fake) LocalRegistryImageStore("localhost:5001").ensure_repository("r") - assert fake.calls[0][fake.calls[0].index("--name") + 1] == "agentenv-registry" - assert "127.0.0.1:5001:5000" in fake.calls[0] + 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): @@ -69,21 +69,21 @@ def test_a_serving_registry_is_left_alone(docker, monkeypatch): assert fake.calls == [] -@pytest.mark.parametrize("port", [5000, 5001]) -def test_a_stopped_container_on_this_port_is_started(docker, listening, port): +@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] == ["docker", "start", "agentenv-registry"] + assert fake.calls[1][-1] == name + assert fake.calls[-1] == ["docker", "start", name] -@pytest.mark.parametrize("existing, wanted", [(5000, 5001), (5001, 5000)]) -def test_a_container_on_another_port_is_refused_not_removed(docker, monkeypatch, existing, wanted): - fake = docker(run_rc=125, existing_port=existing) +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=f"agentenv-registry is using port {existing}, not {wanted}"): - LocalRegistryImageStore(f"localhost:{wanted}").ensure_repository("r") + 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"]