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

recreate-vm.sh recreates with `docker compose up -d --force-recreate`, whose
implicit stop uses compose's own --timeout -- 10 SECONDS by default -- rather
than the service's declared stop_grace_period. A dockurr/windows guest cannot
shut down in ten seconds, so compose stops waiting and goes straight to
removing a container that is still running:

  Error response from daemon: cannot remove container "1d5e3c2f...":
  container is running: stop the container before removing or force remove

The script then exits 1 and the VM is left unhealthy with its network_mode
sidecar stranded on a dead netns -- the exact outcome recreate-vm.sh exists to
prevent.

It is timing-dependent, which is why it can look fine for a while. On the
deployment where this was found the script recreated two VMs successfully three
times inside one hour, then failed on the fourth attempt when the guest took
longer than ten seconds to go down.

The targets are now stopped explicitly first with a timeout that matches the
grace period, and the same value is passed to `up` so its implicit stop cannot
fall back to 10s. RECREATE_STOP_TIMEOUT overrides the 120s default; anything
calling this script on a timeout of its own should stay above it.

The script had no direct test coverage. tests/test_recreate_vm_script.py covers
it through --dry-run, so it needs no Docker daemon: stop-before-up ordering, the
timeout default and its override, and the sidecar expansion that is the reason
the script exists. Four of the seven fail against the current version.

Worth noting for #15: that watchdog delegates recovery to this script, so
merging it without this fix ships an automated recovery path that hits the
failure above.

Dockerfile.test copies a named subset of scripts/ and recreate-vm.sh was not in
it, so the new tests could not see the script. It is added to that COPY line;
nothing else about the image changes.

Author: Marinski

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