Stop leaking helper containers that fail to start (0.66.1)

docker-py's containers.run(remove=True) only removes a container after it
ran. A helper whose start failed - an NFS destination refusing the mount -
stayed behind in "Created", holding the destination's volume. Every Test
click and scheduled backup added one (a user found 18), the volume could
not be deleted even with everything stopped, and changed mount options
never took effect because the volume could not be recreated with them.

- docker_client.run_container: create, start, wait, collect output and
  always remove, also when the start fails; pulls a missing image. Used
  for NFS destination I/O, bind-path inspection, the volume wipe before a
  restore and the Trivy scanner.
- Every helper container is labelled stackpilot.helper; on startup the
  stopped ones are removed, together with anything left on an NFS
  destination volume by older versions.
- When an NFS destination's options change, stopped containers holding
  its volume are removed before it is recreated; a running one is now a
  clear error instead of silently keeping the old options.
- A refused mount explains itself: NFS enabled and reachable from the
  Docker host, NFSv3 needing rpcbind and mountd, and nfsvers=4 needing
  only port 2049.
- The scanner waits for Trivy's own 10-minute timeout instead of giving
  up after the default client's 60 seconds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/docker_client.py

@@ -6,11 +6,11 @@ to the frontend.
 from __future__ import annotations
 
 import functools
-from typing import Any
+from typing import Any, Optional
 
 import docker
 import requests
-from docker.errors import APIError, DockerException, NotFound
+from docker.errors import APIError, DockerException, ImageNotFound, NotFound
 
 from config import settings
 
@@ -57,6 +57,70 @@ def reset_client() -> None:
     get_long_client.cache_clear()
 
 
+#: Label on every throwaway container StackPilot starts (backup helpers, NFS
+#: I/O, the scanner), so leftovers can be found and removed.
+HELPER_LABEL = "stackpilot.helper"
+
+
+def run_container(
+    client: docker.DockerClient,
+    image: str,
+    command: Any,
+    *,
+    volumes: Optional[dict] = None,
+    timeout: Optional[float] = None,
+) -> bytes:
+    """``docker run --rm`` that also cleans up a container that cannot start.
+
+    docker-py's ``containers.run(remove=True)`` only removes a container after
+    it ran. One whose start fails — a volume that cannot be mounted, say an
+    NFS share that refuses the connection — stays behind in "Created" and
+    keeps its volumes in use; every retry adds another. Returns stdout; a
+    non-zero exit raises ``DockerError("container_failed", <stderr>)``.
+    """
+    kwargs = {"command": command, "volumes": volumes or {}, "labels": {HELPER_LABEL: "1"}}
+    try:
+        container = client.containers.create(image, **kwargs)
+    except ImageNotFound:
+        safe_call(client.images.pull, image)
+        container = safe_call(client.containers.create, image, **kwargs)
+    except DockerException as exc:
+        raise DockerError("docker_api_error", str(exc)) from exc
+    try:
+        safe_call(container.start)
+        code = safe_call(container.wait, timeout=timeout).get("StatusCode", 1)
+        out = container.logs(stdout=True, stderr=False) or b""
+        if code != 0:
+            err = (container.logs(stdout=False, stderr=True) or b"").decode("utf-8", "replace")
+            raise DockerError("container_failed", err.strip() or f"exit status {code}")
+        return out
+    finally:
+        try:
+            container.remove(force=True)
+        except Exception:  # noqa: BLE001 - best effort, never mask the real error
+            pass
+
+
+def remove_stale_helpers(client: docker.DockerClient) -> int:
+    """Remove StackPilot's helper containers that are not running.
+
+    Called at startup, when nothing of ours can be in flight. Returns how many
+    were removed.
+    """
+    removed = 0
+    for container in safe_call(
+        client.containers.list, all=True, filters={"label": HELPER_LABEL}
+    ):
+        if container.status == "running":
+            continue
+        try:
+            container.remove(force=True)
+            removed += 1
+        except Exception:  # noqa: BLE001
+            pass
+    return removed
+
+
 def safe_call(fn, *args, **kwargs) -> Any:
     """Run a docker-py call, converting exceptions into DockerError."""
     try:

backend/main.py

@@ -13,6 +13,7 @@ from sqlmodel import Session
 from config import settings
 from version import APP_VERSION
 from database import engine, init_db
+import docker_client
 from docker_client import DockerError
 from routers import (
     audit,
@@ -79,6 +80,17 @@ async def lifespan(app: FastAPI):
             logger.info("Migrated %d custom template(s) from the database to folders", moved)
     except Exception as exc:  # noqa: BLE001
         logger.warning("Legacy template migration failed: %s", exc)
+    # Throwaway containers a previous run left behind (a helper whose start
+    # failed, e.g. on an NFS mount, used to stay in "Created" and keep its
+    # volume in use). Nothing of ours is in flight yet, so all can go.
+    try:
+        client = docker_client.get_client()
+        cleared = docker_client.remove_stale_helpers(client)
+        cleared += backup_destination_service.clear_nfs_leftovers(client)
+        if cleared:
+            logger.info("Removed %d leftover helper container(s)", cleared)
+    except Exception as exc:  # noqa: BLE001 - Docker may not be up yet
+        logger.warning("Could not clean up helper containers: %s", exc)
     # Runtime state that used to live in module dicts and was lost on restart.
     try:
         with Session(engine) as session:

backend/services/backup_destination_service.py

@@ -16,7 +16,7 @@ import posixpath
 import stat
 import tarfile
 import tempfile
-from typing import Any
+from typing import Any, Optional
 
 from models.backup_destination import BackupDestination
 from services import crypto_service
@@ -309,10 +309,21 @@ def _nfs_volume(dest: BackupDestination, cfg: dict) -> str:
     client = get_client()
     try:
         vol = safe_call(client.volumes.get, name)
-        if (vol.attrs.get("Options") or {}) != driver_opts:
-            safe_call(vol.remove)
-            raise DockerError("recreate", "options changed")
     except DockerError:
+        vol = None
+    if vol is not None and (vol.attrs.get("Options") or {}) != driver_opts:
+        # Stopped containers still hold the volume; before 0.66.1 every failed
+        # mount left one behind, and then new settings never took effect.
+        clear_nfs_leftovers(client, name)
+        try:
+            safe_call(vol.remove)
+        except DockerError as exc:
+            raise DestinationError(
+                f"The NFS settings changed, but the Docker volume '{name}' is still in use "
+                f"and cannot be recreated with them: {exc.detail or exc.error}"
+            ) from exc
+        vol = None
+    if vol is None:
         safe_call(
             client.volumes.create,
             name=name,
@@ -323,35 +334,81 @@ def _nfs_volume(dest: BackupDestination, cfg: dict) -> str:
     return name
 
 
+def clear_nfs_leftovers(client, volume: Optional[str] = None) -> int:
+    """Remove stopped containers holding an NFS destination volume.
+
+    Only StackPilot's helpers ever mount these volumes, so any container on
+    one that is not running is a leftover (older versions left one behind per
+    failed mount). ``volume=None`` covers every NFS destination.
+    """
+    from docker_client import DockerError, safe_call
+
+    if volume:
+        names = [volume]
+    else:
+        try:
+            vols = safe_call(
+                client.volumes.list, filters={"label": "stackpilot.nfs-destination"}
+            )
+        except DockerError:
+            return 0
+        names = [v.name for v in vols]
+    removed = 0
+    for name in names:
+        try:
+            containers = safe_call(client.containers.list, all=True, filters={"volume": name})
+        except DockerError:
+            continue
+        for container in containers:
+            if container.status == "running":
+                continue
+            try:
+                container.remove(force=True)
+                removed += 1
+            except Exception:  # noqa: BLE001
+                pass
+    return removed
+
+
 def _nfs_target(cfg: dict) -> str:
     sub = _nfs_subdir(cfg)
     return f"/nfs/{sub}" if sub else "/nfs"
 
 
+def _mount_error(exc: Exception) -> DestinationError:
+    """A mount failure, with what to do about the common one."""
+    text = str(exc)
+    message = f"NFS mount failed: {text}"
+    if "connection refused" in text.lower():
+        message += (
+            " — the NFS server refused the Docker host's connection. Check that NFS is "
+            "enabled on the server and reachable from the Docker host. NFSv3 also needs "
+            "rpcbind (port 111) and mountd; mount options 'rw,nfsvers=4' need only port 2049."
+        )
+    return DestinationError(message)
+
+
 def _nfs_run(volume: str, command: list[str]) -> str:
     """Run a helper container with the NFS volume at /nfs; return stdout."""
-    import docker.errors
-
     from config import settings
-    from docker_client import DockerError, get_client
+    from docker_client import DockerError, get_long_client, run_container
     from services.stack_assets_service import ensure_helper_image
 
-    client = get_client()
+    client = get_long_client()
     ensure_helper_image(client)
     try:
-        out = client.containers.run(
+        out = run_container(
+            client,
             settings.BACKUP_HELPER_IMAGE,
             command,
             volumes={volume: {"bind": "/nfs", "mode": "rw"}},
-            remove=True,
         )
-        return (out or b"").decode("utf-8", "replace")
-    except docker.errors.ContainerError as exc:
-        stderr = (exc.stderr or b"").decode("utf-8", "replace").strip()
-        raise DestinationError(f"NFS operation failed: {stderr or exc}") from exc
-    except (docker.errors.APIError, DockerError) as exc:
+        return out.decode("utf-8", "replace")
+    except DockerError as exc:
+        if exc.error == "container_failed":
+            raise DestinationError(f"NFS operation failed: {exc.detail}") from exc
         # Mount errors surface here (unreachable server, bad export, ...).
-        raise DestinationError(f"NFS mount failed: {exc}") from exc
+        raise _mount_error(exc) from exc
 
 
 def _nfs_helper(volume: str, command: list[str] | str = "true"):
@@ -359,7 +416,7 @@ def _nfs_helper(volume: str, command: list[str] | str = "true"):
     import docker.errors
 
     from config import settings
-    from docker_client import DockerError, get_client, safe_call
+    from docker_client import HELPER_LABEL, DockerError, get_client, safe_call
     from services.stack_assets_service import ensure_helper_image
 
     client = get_client()
@@ -370,9 +427,10 @@ def _nfs_helper(volume: str, command: list[str] | str = "true"):
             settings.BACKUP_HELPER_IMAGE,
             command=command,
             volumes={volume: {"bind": "/nfs", "mode": "rw"}},
+            labels={HELPER_LABEL: "1"},
         )
     except (docker.errors.APIError, DockerError) as exc:
-        raise DestinationError(f"NFS mount failed: {exc}") from exc
+        raise _mount_error(exc) from exc
 
 
 def _nfs_upload(dest: BackupDestination, cfg: dict, local_path: str, filename: str) -> str:

backend/services/scan_service.py

@@ -26,7 +26,7 @@ from typing import Optional
 from sqlmodel import Session, select
 
 from config import settings
-from docker_client import DockerError, get_client, safe_call
+from docker_client import DockerError, get_client, get_long_client, run_container, safe_call
 from models.image_scan import SEVERITIES, ImageScan
 
 logger = logging.getLogger("stackpilot.scan")
@@ -55,9 +55,15 @@ class ScanError(Exception):
 # --------------------------------------------------------------------------- #
 
 
+#: Wait for the scanner a little longer than its own --timeout of 10m.
+SCAN_WAIT_SECONDS = 720
+
+
 def _run_trivy(image: str) -> str:
     """Run Trivy against a local image and return its JSON report."""
-    client = get_client()
+    # Trivy may take its full --timeout (the first run also downloads its
+    # vulnerability database); the 60s default client gave up long before.
+    client = get_long_client()
     socket_path = settings.DOCKER_SOCKET
     command = [
         "image",
@@ -70,21 +76,19 @@ def _run_trivy(image: str) -> str:
         image,
     ]
     try:
-        output = safe_call(
-            client.containers.run,
+        output = run_container(
+            client,
             settings.SCANNER_IMAGE,
             command,
             volumes={
                 socket_path: {"bind": "/var/run/docker.sock", "mode": "ro"},
                 CACHE_VOLUME: {"bind": "/root/.cache/trivy", "mode": "rw"},
             },
-            remove=True,
-            stdout=True,
-            stderr=False,
+            timeout=SCAN_WAIT_SECONDS,
         )
     except DockerError as exc:
-        raise ScanError(str(exc)) from exc
-    except Exception as exc:  # noqa: BLE001 - docker-py raises ContainerError etc.
+        raise ScanError(_readable(Exception(exc.detail or exc.error))) from exc
+    except Exception as exc:  # noqa: BLE001
         raise ScanError(_readable(exc)) from exc
     return (output or b"").decode("utf-8", "replace")
 

backend/services/stack_assets_service.py

@@ -24,7 +24,14 @@ from typing import Optional
 import yaml
 
 from config import settings
-from docker_client import DockerError, get_client, get_long_client, safe_call
+from docker_client import (
+    HELPER_LABEL,
+    DockerError,
+    get_client,
+    get_long_client,
+    run_container,
+    safe_call,
+)
 from services import compose_service
 
 logger = logging.getLogger("stackpilot.assets")
@@ -97,6 +104,7 @@ def _create_helper(client, volumes: dict):
         settings.BACKUP_HELPER_IMAGE,
         command="true",
         volumes=volumes,
+        labels={HELPER_LABEL: "1"},
     )
 
 
@@ -157,6 +165,7 @@ def _create_helper_with(client, volumes: dict, command: list[str]):
         settings.BACKUP_HELPER_IMAGE,
         command=command,
         volumes=volumes,
+        labels={HELPER_LABEL: "1"},
     )
 
 
@@ -192,14 +201,8 @@ def inspect_paths(paths: list[str]) -> dict[str, dict]:
         )
     script = "; ".join(script_parts)
     try:
-        out = safe_call(
-            client.containers.run,
-            settings.BACKUP_HELPER_IMAGE,
-            ["sh", "-c", script],
-            volumes=mounts,
-            remove=True,
-            stdout=True,
-            stderr=False,
+        out = run_container(
+            client, settings.BACKUP_HELPER_IMAGE, ["sh", "-c", script], volumes=mounts
         )
     except DockerError as exc:
         logger.warning("Path inspection failed: %s", exc)
@@ -341,12 +344,11 @@ def import_volume(
         )
     if existed and wipe:
         # Restore means "back to the snapshot": drop files created since.
-        safe_call(
-            client.containers.run,
+        run_container(
+            client,
             settings.BACKUP_HELPER_IMAGE,
             ["sh", "-c", "find /v -mindepth 1 -delete"],
             volumes={full_name: {"bind": "/v", "mode": "rw"}},
-            remove=True,
         )
     mounts = {full_name: {"bind": "/v", "mode": "rw"}}
     container = _create_helper(client, mounts)

backend/tests/test_backup_restore.py

@@ -648,6 +648,8 @@ def test_download_removes_its_temp_file(stack, as_admin, db):
 
 
 class _HelperClient:
+    """Stands in for the helper container that runs the inspection script."""
+
     def __init__(self, output: str):
         self.output = output
         self.images = self
@@ -656,8 +658,20 @@ class _HelperClient:
     def get(self, image):
         return object()
 
-    def run(self, image, command, **kwargs):
-        return self.output.encode()
+    def create(self, image, **kwargs):
+        return self
+
+    def start(self):
+        pass
+
+    def wait(self, timeout=None):
+        return {"StatusCode": 0}
+
+    def logs(self, stdout=True, stderr=True):
+        return self.output.encode() if stdout else b""
+
+    def remove(self, force=False):
+        pass
 
 
 def test_inspect_paths_flags_remote_and_unmeasured(monkeypatch):

backend/tests/test_helper_containers.py

@@ -0,0 +1,208 @@
+"""Throwaway containers must not outlive their job.
+
+docker-py's ``containers.run(remove=True)`` only removes a container after it
+ran. A helper whose start failed — an NFS destination that refuses the mount —
+stayed behind in "Created", holding the destination's volume. Every retry of
+"Test" or of a scheduled backup added one (a user found 18), and because the
+volume stayed in use, changing the destination's mount options never took
+effect: the volume could not be recreated with them.
+"""
+from __future__ import annotations
+
+import pytest
+
+from docker_client import HELPER_LABEL, DockerError
+
+
+class FakeContainer:
+    def __init__(self, client, image, command=None, volumes=None, labels=None, status="created"):
+        self.client, self.image, self.command = client, image, command
+        self.volumes, self.labels, self.status = volumes or {}, labels or {}, status
+        self.removed = False
+
+    def start(self):
+        if self.client.start_error:
+            raise DockerError("docker_api_error", self.client.start_error)
+        self.status = "exited"
+
+    def wait(self, timeout=None):
+        self.client.wait_timeouts.append(timeout)
+        return {"StatusCode": self.client.exit_code}
+
+    def logs(self, stdout=True, stderr=True):
+        return b"out" if stdout else b"boom"
+
+    def remove(self, force=False):
+        self.removed = True
+        self.client.all.remove(self)
+
+
+class FakeClient:
+    def __init__(self, start_error=None, exit_code=0):
+        self.start_error, self.exit_code = start_error, exit_code
+        self.all: list[FakeContainer] = []
+        self.wait_timeouts: list = []
+        self.containers = self
+        self.volumes = FakeVolumes(self)
+        self.images = self
+
+    def create(self, image, command=None, volumes=None, labels=None):
+        c = FakeContainer(self, image, command, volumes, labels)
+        self.all.append(c)
+        return c
+
+    def get(self, image):
+        return object()
+
+    def list(self, all=False, filters=None):
+        filters = filters or {}
+        out = list(self.all)
+        if "label" in filters:
+            out = [c for c in out if filters["label"] in c.labels]
+        if "volume" in filters:
+            out = [c for c in out if filters["volume"] in c.volumes]
+        return out
+
+
+class FakeVolume:
+    def __init__(self, client, name, options, labels):
+        self.client, self.name = client, name
+        self.attrs = {"Options": options, "Labels": labels}
+
+    def remove(self):
+        if any(self.name in c.volumes for c in self.client.all):
+            raise DockerError("docker_api_error", "volume is in use")
+        del self.client.volumes.items[self.name]
+
+
+class FakeVolumes:
+    def __init__(self, client):
+        self.client = client
+        self.items: dict[str, FakeVolume] = {}
+
+    def get(self, name):
+        if name not in self.items:
+            raise DockerError("not_found", name)
+        return self.items[name]
+
+    def create(self, name, driver, driver_opts, labels):
+        self.items[name] = FakeVolume(self.client, name, driver_opts, labels)
+        return self.items[name]
+
+    def list(self, filters=None):
+        label = (filters or {}).get("label")
+        return [v for v in self.items.values() if not label or label in v.attrs["Labels"]]
+
+
+def test_container_that_cannot_start_is_removed():
+    from docker_client import run_container
+
+    client = FakeClient(start_error="failed to mount local volume: connection refused")
+    with pytest.raises(DockerError, match="connection refused"):
+        run_container(client, "alpine", ["true"], volumes={"nfs": {"bind": "/nfs"}})
+    assert client.all == []
+
+
+def test_failed_command_is_removed_and_reports_stderr():
+    from docker_client import run_container
+
+    client = FakeClient(exit_code=2)
+    with pytest.raises(DockerError) as info:
+        run_container(client, "alpine", ["false"])
+    assert info.value.error == "container_failed" and info.value.detail == "boom"
+    assert client.all == []
+
+
+def test_successful_run_returns_stdout_and_is_labelled():
+    from docker_client import run_container
+
+    client = FakeClient()
+    seen = []
+    original = client.create
+    client.create = lambda *a, **k: seen.append(k["labels"]) or original(*a, **k)
+    assert run_container(client, "alpine", ["echo"], timeout=900) == b"out"
+    assert seen == [{HELPER_LABEL: "1"}] and client.all == []
+    assert client.wait_timeouts == [900]
+
+
+def test_startup_removes_stopped_helpers_only():
+    from docker_client import remove_stale_helpers
+
+    client = FakeClient()
+    stale = client.create("alpine", labels={HELPER_LABEL: "1"})
+    busy = client.create("alpine", labels={HELPER_LABEL: "1"})
+    busy.status = "running"
+    foreign = client.create("nginx", labels={})
+
+    assert remove_stale_helpers(client) == 1
+    assert stale.removed and not busy.removed and not foreign.removed
+
+
+def _nfs_dest(options="rw"):
+    from models.backup_destination import BackupDestination
+
+    return BackupDestination(id=1, name="nas", type="nfs", config="{}"), {
+        "server": "10.10.1.80", "path": "/mnt/user/Share", "options": options,
+    }
+
+
+def test_changed_nfs_options_take_effect_despite_leftovers(monkeypatch):
+    from services import backup_destination_service as svc
+
+    client = FakeClient()
+    monkeypatch.setattr("docker_client.get_client", lambda: client)
+    dest, cfg = _nfs_dest("rw")
+    name = svc._nfs_volume(dest, cfg)
+    # 18 helpers left behind by failed mounts under the old version.
+    for _ in range(18):
+        client.create("alpine", volumes={name: {"bind": "/nfs"}})
+
+    dest, cfg = _nfs_dest("rw,nfsvers=4")
+    assert svc._nfs_volume(dest, cfg) == name
+    assert client.volumes.items[name].attrs["Options"]["o"] == "addr=10.10.1.80,rw,nfsvers=4"
+    assert client.all == []
+
+
+def test_volume_held_by_a_running_container_is_a_clear_error(monkeypatch):
+    from services import backup_destination_service as svc
+
+    client = FakeClient()
+    monkeypatch.setattr("docker_client.get_client", lambda: client)
+    dest, cfg = _nfs_dest("rw")
+    name = svc._nfs_volume(dest, cfg)
+    client.create("alpine", volumes={name: {"bind": "/nfs"}}).status = "running"
+
+    dest, cfg = _nfs_dest("rw,nfsvers=4")
+    with pytest.raises(svc.DestinationError, match="still in use"):
+        svc._nfs_volume(dest, cfg)
+
+
+def test_clear_nfs_leftovers_covers_every_destination():
+    from services import backup_destination_service as svc
+
+    client = FakeClient()
+    client.volumes.create("stackpilot-nfs-dest-1", "local", {}, {"stackpilot.nfs-destination": "1"})
+    client.volumes.create("stackpilot-nfs-dest-2", "local", {}, {"stackpilot.nfs-destination": "2"})
+    client.volumes.create("app_data", "local", {}, {})
+    client.create("alpine", volumes={"stackpilot-nfs-dest-1": {}})
+    client.create("alpine", volumes={"stackpilot-nfs-dest-2": {}})
+    keep = client.create("postgres", volumes={"app_data": {}})
+
+    assert svc.clear_nfs_leftovers(client) == 2
+    assert client.all == [keep]
+
+
+def test_refused_mount_says_what_to_do(monkeypatch):
+    from services import backup_destination_service as svc
+
+    client = FakeClient(start_error=(
+        "error while mounting volume: failed to mount local volume: mount "
+        ":/mnt/user/Share: data: addr=10.10.1.80: connection refused"
+    ))
+    monkeypatch.setattr("docker_client.get_long_client", lambda: client)
+    monkeypatch.setattr("services.stack_assets_service.ensure_helper_image", lambda c: None)
+
+    with pytest.raises(svc.DestinationError) as info:
+        svc._nfs_run("stackpilot-nfs-dest-1", ["mkdir", "-p", "/nfs"])
+    assert "nfsvers=4" in str(info.value) and "port 2049" in str(info.value)
+    assert client.all == []  # and nothing is left behind

backend/version.py

@@ -1,3 +1,3 @@
 """Single source of truth for the StackPilot release version."""
 
-APP_VERSION = "0.66.0"
+APP_VERSION = "0.66.1"

frontend/package.json

@@ -1,7 +1,7 @@
 {
   "name": "stackpilot-frontend",
   "private": true,
-  "version": "0.66.0",
+  "version": "0.66.1",
   "type": "module",
   "scripts": {
     "dev": "vite",