fix(ops): stop VMs before recreating them, with the real grace period

Dockerfile.test

@@ -25,7 +25,7 @@ COPY tests ./tests
 # docker-compose.yml.j2 is here because the compose-generation test renders the
 # REAL template — a stub would assert nothing about what actually ships.
 COPY requirements-api.txt requirements-mcpunifier.txt docker-compose.yml.example docker-compose.yml.j2 run.sh ./
-COPY scripts/config_helper.py scripts/start.bat scripts/check_health.py scripts/healthcheck.sh scripts/verify_binaries.py scripts/wickworks-healthcheck.py ./scripts/
+COPY scripts/config_helper.py scripts/start.bat scripts/check_health.py scripts/healthcheck.sh scripts/verify_binaries.py scripts/wickworks-healthcheck.py scripts/recreate-vm.sh ./scripts/
 COPY assets/binaries.lock.json ./assets/
 
 ENV PYTHONPATH=/app

scripts/recreate-vm.sh

@@ -33,10 +33,31 @@
 #   ./scripts/recreate-vm.sh mt5          # recreate mt5 + its sidecars
 #   ./scripts/recreate-vm.sh mt5 mt5-b    # recreate both VMs + their sidecars
 #
+# STOPPING BEFORE RECREATING
+# --------------------------
+# `docker compose up --force-recreate` stops each container using compose's OWN
+# --timeout, which defaults to 10 SECONDS, not the service's stop_grace_period.
+# A dockurr/windows VM needs far longer than that to shut down (ours declare
+# `stop_grace_period: 2m`), so compose gave up waiting and went straight to
+# removing a container that was still running:
+#
+#   Error response from daemon: cannot remove container "...":
+#   container is running: stop the container before removing or force remove
+#
+# The watchdog then recorded a failed recreate and backed off, leaving the VM
+# unhealthy and its sidecars stranded — the exact outcome this script exists to
+# prevent. So the targets are stopped explicitly first, with a timeout that
+# matches the grace period, and the same value is passed to `up` so its implicit
+# stop can never fall back to 10s.
+#
 # ENV
 # ---
-#   COMPOSE_FILE   compose file to read services from
-#                  (default: ./docker-compose.yml in the repo root)
+#   COMPOSE_FILE           compose file to read services from
+#                          (default: ./docker-compose.yml in the repo root)
+#   RECREATE_STOP_TIMEOUT  seconds to allow each container to stop
+#                          (default: 120, matching stop_grace_period: 2m).
+#                          Keep this below WATCHDOG_RECREATE_TIMEOUT (300s) or
+#                          the watchdog kills the script mid-recreate.
 #
 # The script never touches services it was not asked to recreate, and never
 # uses --no-deps in a way that skips the named sidecars.
@@ -47,6 +68,7 @@ trap 'echo "[ERROR] ${BASH_SOURCE[0]}:${LINENO} - command failed (exit $?)" >&2'
 
 DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
 COMPOSE_FILE="${COMPOSE_FILE:-${DIR}/docker-compose.yml}"
+STOP_TIMEOUT="${RECREATE_STOP_TIMEOUT:-120}"
 DRY_RUN=0
 
 usage() {
@@ -117,12 +139,17 @@ main() {
     done
 
     if [ "$DRY_RUN" = "1" ]; then
-        log "DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps ${targets[*]}'"
+        log "DRY-RUN: would run 'docker compose stop -t ${STOP_TIMEOUT} ${targets[*]}'"
+        log "DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps -t ${STOP_TIMEOUT} ${targets[*]}'"
         return 0
     fi
 
+    # Stop first, with the real grace period. See STOPPING BEFORE RECREATING.
+    log "stopping (timeout ${STOP_TIMEOUT}s): ${targets[*]}"
+    docker compose -f "$COMPOSE_FILE" stop -t "$STOP_TIMEOUT" "${targets[@]}"
+
     log "recreating: ${targets[*]}"
-    docker compose -f "$COMPOSE_FILE" up -d --force-recreate --no-deps "${targets[@]}"
+    docker compose -f "$COMPOSE_FILE" up -d --force-recreate --no-deps -t "$STOP_TIMEOUT" "${targets[@]}"
     log "recreate done"
 }
 

tests/test_recreate_vm_script.py

@@ -0,0 +1,118 @@
+"""Tests for scripts/recreate-vm.sh.
+
+The script had no direct coverage, and the gap hid a production failure: it
+recreated VMs with `docker compose up --force-recreate`, whose implicit stop
+uses compose's own --timeout (10s by default) rather than the service's
+stop_grace_period. A dockurr/windows VM cannot shut down in 10s, so compose
+tried to remove a still-running container and the recreate failed:
+
+    cannot remove container "...": container is running
+
+These exercise --dry-run, so they assert the planned command line without
+needing a Docker daemon.
+"""
+
+import os
+import subprocess
+from pathlib import Path
+
+_REPO = Path(__file__).resolve().parents[1]
+_SCRIPT = _REPO / "scripts" / "recreate-vm.sh"
+
+COMPOSE = """\
+services:
+  mt5:
+    image: dockurr/windows:5.14
+    stop_grace_period: 2m
+  wickworks:
+    image: psyb0t/wickworks
+    network_mode: "service:mt5"
+  mt5-b:
+    image: dockurr/windows:5.14
+  wickworks-b:
+    image: psyb0t/wickworks
+    network_mode: "service:mt5-b"
+  unrelated:
+    image: nginx
+"""
+
+
+def run(tmp_path, *args, env=None):
+    compose = tmp_path / "docker-compose.yml"
+    compose.write_text(COMPOSE, encoding="utf-8")
+    # Inherit the real PATH: the script's sidecar discovery shells out to
+    # python3, which is not at a fixed location across host and test image.
+    full_env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "COMPOSE_FILE": str(compose)}
+    full_env.update(env or {})
+    return subprocess.run(
+        [str(_SCRIPT), "--dry-run", *args],
+        capture_output=True,
+        text=True,
+        env=full_env,
+        timeout=60,
+        check=False,
+    )
+
+
+def test_stops_before_recreating(tmp_path):
+    """The stop must be explicit, or compose removes a running container."""
+    res = run(tmp_path, "mt5")
+    assert res.returncode == 0, res.stderr
+    assert "docker compose stop" in res.stdout
+    stop_at = res.stdout.index("docker compose stop")
+    up_at = res.stdout.index("docker compose up")
+    assert stop_at < up_at, "stop must be planned before the recreate"
+
+
+def test_stop_timeout_matches_the_grace_period_by_default(tmp_path):
+    """10s (compose's default) is far too short for a Windows VM."""
+    res = run(tmp_path, "mt5")
+    assert "-t 120" in res.stdout
+    assert "-t 10 " not in res.stdout
+
+
+def test_timeout_is_passed_to_up_as_well(tmp_path):
+    """`up --force-recreate` does its own stop; it must not use the 10s default."""
+    res = run(tmp_path, "mt5")
+    up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln)
+    assert "--force-recreate" in up_line
+    assert "--no-deps" in up_line
+    assert "-t 120" in up_line
+
+
+def test_timeout_is_overridable(tmp_path):
+    res = run(tmp_path, "mt5", env={"RECREATE_STOP_TIMEOUT": "45"})
+    assert "-t 45" in res.stdout
+    assert "-t 120" not in res.stdout
+
+
+def test_sidecars_are_recreated_with_their_vm(tmp_path):
+    """The whole point: the sidecar must rejoin the VM's new netns."""
+    res = run(tmp_path, "mt5")
+    up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln)
+    assert "mt5" in up_line
+    assert "wickworks" in up_line
+    assert "unrelated" not in up_line
+    assert "mt5-b" not in up_line
+
+
+def test_multiple_vms_expand_to_all_their_sidecars(tmp_path):
+    res = run(tmp_path, "mt5", "mt5-b")
+    up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln)
+    for expected in ("mt5", "wickworks", "mt5-b", "wickworks-b"):
+        assert expected in up_line
+    assert "unrelated" not in up_line
+
+
+def test_no_arguments_is_an_error(tmp_path):
+    compose = tmp_path / "docker-compose.yml"
+    compose.write_text(COMPOSE, encoding="utf-8")
+    res = subprocess.run(
+        [str(_SCRIPT)],
+        capture_output=True,
+        text=True,
+        env={"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "COMPOSE_FILE": str(compose)},
+        timeout=60,
+        check=False,
+    )
+    assert res.returncode != 0