From 7f153fc8f08315f7caf68898eb18e731860cfbe2 Mon Sep 17 00:00:00 2001 From: Marinski Date: Fri, 21 Aug 2026 12:33:01 +0300 Subject: [PATCH 1/4] feat(ops): recreate VM containers that are persistently unhealthy dockurr/windows keeps its container up while the Windows guest inside may have crashed, so `restart: unless-stopped` never fires and every terminal API in that VM stays dead until a human intervenes. This adds a compose-managed sidecar that watches Docker health and recovers a VM on its own. Recovery is a COORDINATED RECREATE, not a restart ------------------------------------------------- An earlier revision of this branch used `docker restart` through the Docker API, on the reasoning that keeping the owner's container ID keeps a wickworks sidecar's netns attachment intact. That reasoning is wrong, and tests/integration/test_wickworks_lifecycle.py already proves it: Docker tears the netns down on stop and builds a fresh one on start, so restarting the owner alone strands the sidecar exactly as recreating the owner alone does. Only recreating the owner together with its sidecars repairs the binding. So the watchdog shells out to scripts/recreate-vm.sh -- the helper an operator runs by hand, and the one that lifecycle test covers -- rather than reimplementing sidecar discovery. Two recovery paths that could drift apart is precisely what this avoids. Consequences of using compose from inside a container: - The sidecar image now carries the docker CLI, the compose plugin, bash and PyYAML (Dockerfile.watchdog, base still digest-pinned because this container mounts the root-equivalent Docker socket). - Compose resolves this project's relative bind mounts client-side, so the project has to be mounted through at the SAME absolute path the host uses. run.sh exports MT5_PROJECT_DIR; validate_config() reports it at startup when it is missing and the watchdog refuses to act, rather than falling back to a restart that looks like recovery and is not. - COMPOSE_PROJECT_NAME is passed explicitly. Compose otherwise derives the project from the directory name, and a mismatch would not fail -- it would quietly create a second set of containers beside the running ones. - Recovery names the compose SERVICE, taken from the container's com.docker.compose.service label; a container id means nothing to compose. A VM without that label is skipped rather than guessed at. Watchdog behaviour ------------------ - Scoped to this compose project and the dockurr/windows image, so nginx, wickworks, the log rotator and the watchdog itself are never touched. - Acts only after health has stayed unhealthy for a sustained FailingStreak, so a busy VM mid-backtest is never interrupted. - Per-container state on a named volume, exponential backoff between attempts, a bounded attempt budget, and a reset only after sustained health -- so a VM that crashes again immediately is not thrashed. - --dry-run evaluates against a copy of the state, so dry passes cannot consume the real backoff and attempt budget. Full suite passes in the container test image: 458 passed, 2 skipped. --- Dockerfile.test | 2 +- Dockerfile.watchdog | 26 ++ docker-compose.yml.example | 56 ++- docker-compose.yml.j2 | 41 ++ docs/operations.md | 67 ++++ run.sh | 7 + scripts/healthcheck.sh | 45 ++- scripts/vm-watchdog.py | 501 ++++++++++++++++++++++++ tests/test_healthcheck_behavior.py | 75 ++++ tests/test_vm_watchdog.py | 602 +++++++++++++++++++++++++++++ 10 files changed, 1401 insertions(+), 21 deletions(-) create mode 100644 Dockerfile.watchdog create mode 100755 scripts/vm-watchdog.py create mode 100644 tests/test_vm_watchdog.py diff --git a/Dockerfile.test b/Dockerfile.test index f4f7445d..2ca9e6d7 100644 --- a/Dockerfile.test +++ b/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/recreate-vm.sh ./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/vm-watchdog.py scripts/recreate-vm.sh ./scripts/ COPY assets/binaries.lock.json ./assets/ ENV PYTHONPATH=/app diff --git a/Dockerfile.watchdog b/Dockerfile.watchdog new file mode 100644 index 00000000..9b34e089 --- /dev/null +++ b/Dockerfile.watchdog @@ -0,0 +1,26 @@ +# VM crash watchdog. +# +# Recovery is a coordinated compose recreate (VM + the sidecars sharing its +# netns), not a Docker-API restart — see scripts/vm-watchdog.py for why a +# restart strands the sidecar. That means this image needs the docker CLI and +# the compose plugin, plus bash and PyYAML for scripts/recreate-vm.sh, which is +# the helper it shells out to. +# +# Pinned by digest, not tag: this container mounts the Docker socket, which is +# root-equivalent on the host, and a moving tag would hand that to whatever the +# registry serves tomorrow. The digest is the multi-arch OCI index for +# python:3.12-alpine, so it still resolves per-platform. +# To update: docker buildx imagetools inspect python:3.12-alpine +FROM python:3.12-alpine@sha256:d09d15e60962ca365d1cd544a48773bac9d33f2fb1b00f2aa0deec78ade7dc31 + +# docker-cli-compose provides `docker compose`; recreate-vm.sh is bash and +# parses the compose file with PyYAML. +RUN apk add --no-cache bash docker-cli docker-cli-compose \ + && pip install --no-cache-dir pyyaml + +ENV PYTHONUNBUFFERED=1 \ + PYTHONDONTWRITEBYTECODE=1 + +# The watchdog script and recreate-vm.sh stay bind-mounted by compose rather +# than baked in, so a fix to either is a container restart and not a rebuild. +CMD ["python", "-u", "/vm-watchdog.py"] diff --git a/docker-compose.yml.example b/docker-compose.yml.example index 5503421c..d1b13863 100644 --- a/docker-compose.yml.example +++ b/docker-compose.yml.example @@ -91,21 +91,42 @@ services: - ./scripts/rotate-logs.sh:/rotate.sh:ro command: ["sh", "/rotate.sh"] - # nginx is the single entry point for all terminal APIs. Routes - # ///... to mt5: (per-terminal Python - # API process inside the Windows VM, reachable via mt5 container's - # iptables DNAT). Auto-generated from config/config.yaml by run.sh. - # Bound to 127.0.0.1:8888 so it's loopback-only by default — LAN - # exposure is opt-in (change the host bind), tailnet exposure is via - # the optional tailscale sidecar below. - # Unified MCP endpoint. One MCP session that reaches every terminal, with - # broker/account as tool parameters, instead of one endpoint per terminal. - # The per-terminal ///mcp endpoints keep working unchanged; - # nginx routes /mcp/ here. - # - # Reads the same config/config.yaml that generates the nginx routing, so it - # cannot route somewhere nginx does not. It never waits for terminals: a - # terminal that is down fails only the calls naming it. + # VM crash watchdog. dockurr/windows keeps the container up while the + # Windows guest may have crashed internally, so restart: unless-stopped + # never fires and every terminal API in that VM stays dead. This sidecar + # polls Docker health through the socket and restarts a VM only after its + # health has stayed unhealthy for a sustained FailingStreak, with + # exponential backoff and bounded retries; state lives on a named volume. + # Runs inside the compose project (docker compose up -d), no host cron. + # See scripts/vm-watchdog.py. + vm-watchdog: + # Needs the docker CLI + compose plugin to run scripts/recreate-vm.sh, so + # it is built rather than pulled. The base image is digest-pinned inside + # the Dockerfile (this container mounts the Docker socket). + build: + context: . + dockerfile: Dockerfile.watchdog + restart: unless-stopped + command: ["python", "-u", "/vm-watchdog.py"] + volumes: + - /var/run/docker.sock:/var/run/docker.sock + - ./scripts/vm-watchdog.py:/vm-watchdog.py:ro + - vm-watchdog-state:/state + # The project itself, at the SAME absolute path the host uses. Compose + # resolves the relative bind mounts in this file client-side, so a + # different path in here would rewrite every mount to somewhere that + # does not exist on the host. run.sh exports MT5_PROJECT_DIR; the + # watchdog refuses to act (and says so at startup) if it is unset. + - ${MT5_PROJECT_DIR:?MT5_PROJECT_DIR must be the absolute host path of this project (run.sh exports it; otherwise set it in .env)}:${MT5_PROJECT_DIR}:ro + environment: + WATCHDOG_STATE_DIR: /state + WATCHDOG_PROJECT_DIR: ${MT5_PROJECT_DIR} + WATCHDOG_RECREATE_SCRIPT: ${MT5_PROJECT_DIR}/scripts/recreate-vm.sh + logging: + driver: json-file + options: + max-size: "10m" + max-file: "3" mcpunifier: build: context: . @@ -198,3 +219,8 @@ services: # - NET_RAW # depends_on: # - nginx + +volumes: + # Persistent per-container watchdog state (last restart, attempts, + # healthy-since) so backoff survives the watchdog's own restarts. + vm-watchdog-state: diff --git a/docker-compose.yml.j2 b/docker-compose.yml.j2 index 31c40955..250fde4c 100644 --- a/docker-compose.yml.j2 +++ b/docker-compose.yml.j2 @@ -120,6 +120,42 @@ services: - ./scripts/rotate-logs.sh:/rotate.sh:ro command: ["sh", "/rotate.sh"] + # VM crash watchdog. dockurr/windows keeps the container up while the + # Windows guest may have crashed internally, so restart: unless-stopped + # never fires and every terminal API in that VM stays dead. This sidecar + # polls Docker health through the socket and restarts a VM only after its + # health has stayed unhealthy for a sustained FailingStreak, with + # exponential backoff and bounded retries; state lives on a named volume. + # Runs inside the compose project (docker compose up -d), no host cron. + # See scripts/vm-watchdog.py. + vm-watchdog: + # Needs the docker CLI + compose plugin to run scripts/recreate-vm.sh, so + # it is built rather than pulled. The base image is digest-pinned inside + # the Dockerfile (this container mounts the Docker socket). + build: + context: . + dockerfile: Dockerfile.watchdog + restart: unless-stopped + command: ["python", "-u", "/vm-watchdog.py"] + volumes: + - /var/run/docker.sock:/var/run/docker.sock + - ./scripts/vm-watchdog.py:/vm-watchdog.py:ro + - vm-watchdog-state:/state + # The project itself, at the SAME absolute path the host uses. Compose + # resolves the relative bind mounts in this file client-side, so a + # different path in here would rewrite every mount to somewhere that + # does not exist on the host. run.sh exports MT5_PROJECT_DIR; the + # watchdog refuses to act (and says so at startup) if it is unset. + - ${MT5_PROJECT_DIR:?MT5_PROJECT_DIR must be the absolute host path of this project (run.sh exports it; otherwise set it in .env)}:${MT5_PROJECT_DIR}:ro + environment: + WATCHDOG_STATE_DIR: /state + WATCHDOG_PROJECT_DIR: ${MT5_PROJECT_DIR} + WATCHDOG_RECREATE_SCRIPT: ${MT5_PROJECT_DIR}/scripts/recreate-vm.sh + logging: + driver: json-file + options: + max-size: "10m" + max-file: "3" {% if enable_mcpunifier|default(true) %} # Unified MCP endpoint. One MCP session that reaches every terminal, with # broker/account as tool parameters, instead of one endpoint per terminal. @@ -229,3 +265,8 @@ services: # - NET_RAW # depends_on: # - nginx + +volumes: + # Persistent per-container watchdog state (last restart, attempts, + # healthy-since) so backoff survives the watchdog's own restarts. + vm-watchdog-state: diff --git a/docs/operations.md b/docs/operations.md index bc1c1a48..5ea94bba 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -10,6 +10,7 @@ The commands, ports, tunnels, locks, queues, and logs you need after the thing b - [Cloudflare Tunnel](#cloudflare-tunnel-optional) - [Project structure](#project-structure) - [Concurrency and backpressure](#concurrency-and-backpressure) +- [Auto-recovery](#auto-recovery) - [Logs](#logs) ## Make Targets @@ -251,6 +252,72 @@ or, with the sidecar list discovered from the generated compose file: This is covered by the real Compose lifecycle regression in `tests/integration/test_wickworks_lifecycle.py`. +## Auto-recovery + +The Windows VM(s) run inside `dockurr/windows` containers with a Docker healthcheck +(`scripts/healthcheck.sh`) that probes every terminal port this VM owns. A crash +inside the guest — an unexpected shutdown (Event 6008), a wedged terminal, an +OOM — leaves the **container** up while the **API** is dead, so +`restart: unless-stopped` never fires and nothing recovers it on its own. + +### VM crash watchdog + +`vm-watchdog` is a Compose-managed sidecar for exactly that case. It is part of +the project (`docker compose up -d` brings it up with everything else) — no +host cron, no systemd unit, no machine-specific checkout path. It polls Docker +health through the mounted unix socket and uses Docker's own +`State.Health.FailingStreak` as the source of truth. + +Behavior: + +- Scopes itself to this Compose project (`com.docker.compose.project`, + discovered from its own container labels) and to the `dockurr/windows` VM + image, so it only ever recovers the VM containers — never nginx, wickworks, + the log rotator, or other sidecars. +- **Recovers by recreating the VM together with its netns sidecars**, by + running `scripts/recreate-vm.sh ` — the same helper documented above + and covered by `tests/integration/test_wickworks_lifecycle.py`. It does not + use `docker restart`: that keeps the container ID but gives the VM a fresh + netns on start, which strands the wickworks sidecar exactly as recreating the + VM alone does. +- Acts **only** after its Docker health has stayed `unhealthy` + for `WATCHDOG_MIN_FAILING_STREAK` consecutive healthcheck failures (default + `10`, i.e. ~5 minutes at the default 30s interval). A container that is + healthy or still starting is never touched, so running backtests on a working + VM are never interrupted — the healthcheck stays green the whole time a + terminal is serving. +- Keeps a tiny state record per container on a named volume + (`/state/.json`): last restart, attempt count, and when the VM + was last observed healthy. +- Enforces exponential backoff between recovery attempts + (`WATCHDOG_BACKOFF_ATTEMPTS`, default `300,900,3600` — 5m → 15m → 1h), so a + VM that crashes again immediately after recovery is not restarted into a + loop. +- Stops after `WATCHDOG_MAX_ATTEMPTS` consecutive failed recoveries (default + `3`) and logs loudly, instead of threshing forever. +- Resets the attempt budget only after the VM has stayed healthy for + `WATCHDOG_RESET_SECONDS` (default `1800`), so a VM that recovered then crashed + later gets a fresh budget. +- `WATCHDOG_DRY_RUN=1` (or `--dry-run`) prints what it would do without + touching any container. + +Environment overrides: `WATCHDOG_INTERVAL_SECONDS`, `WATCHDOG_MIN_FAILING_STREAK`, +`WATCHDOG_IMAGE_FILTER`, `WATCHDOG_BACKOFF_ATTEMPTS`, `WATCHDOG_MAX_ATTEMPTS`, +`WATCHDOG_RESET_SECONDS`, `WATCHDOG_COMPOSE_PROJECT`, `WATCHDOG_STATE_DIR`, +`WATCHDOG_DOCKER_SOCKET`, `WATCHDOG_DRY_RUN`, `WATCHDOG_PROJECT_DIR`, +`WATCHDOG_RECREATE_SCRIPT`, `WATCHDOG_RECREATE_TIMEOUT`. + +`WATCHDOG_PROJECT_DIR` is the **host** path of this project, and the compose +service mounts the project through at that same absolute path. Compose resolves +the relative bind mounts in `docker-compose.yml` client-side, so a +container-local path would rewrite every mount to something that does not exist +on the host. `run.sh` exports `MT5_PROJECT_DIR` for this; if it is missing the +watchdog reports it at startup and refuses to act, rather than falling back to a +restart that looks like recovery and is not. + +This complements the in-VM `MT5AutoReboot` scheduled task, which reboots on a +fixed timer and can interrupt long-running backtests; operators who disable that +task still get crash recovery from the watchdog. ## Logs diff --git a/run.sh b/run.sh index dbe4b469..5b7d572b 100755 --- a/run.sh +++ b/run.sh @@ -2,6 +2,13 @@ set -eo pipefail DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# The vm-watchdog container recovers a crashed VM by running +# scripts/recreate-vm.sh, which shells out to `docker compose`. Compose +# resolves this file's relative bind mounts client-side, so the watchdog needs +# the project at the SAME absolute path the host uses. Exported (not just set) +# because compose interpolates it into docker-compose.yml at up time. +export MT5_PROJECT_DIR="${MT5_PROJECT_DIR:-${DIR}}" DEBLOAT=0 for arg in "$@"; do if [ "$arg" = "--debloat" ]; then diff --git a/scripts/healthcheck.sh b/scripts/healthcheck.sh index a2c48ca7..834ad907 100755 --- a/scripts/healthcheck.sh +++ b/scripts/healthcheck.sh @@ -18,11 +18,13 @@ set -u -readonly CONFIG=/shared/config/config.yaml +# Overridable only so the behavioral tests can point the script at fixtures; +# the container never sets these and gets the paths it always had. +readonly CONFIG=${HEALTHCHECK_CONFIG:-/shared/config/config.yaml} # Per-VM terminal list, bind-mounted by docker-compose from # data/vm-group-.txt. Absent on single-VM installs, which means no filter. -readonly VM_GROUP=/shared/config/vm-group.txt -readonly DNSMASQ_LEASES=/var/lib/misc/dnsmasq.leases +readonly VM_GROUP=${HEALTHCHECK_VM_GROUP:-/shared/config/vm-group.txt} +readonly DNSMASQ_LEASES=${HEALTHCHECK_LEASES:-/var/lib/misc/dnsmasq.leases} readonly PROBE_PATH=/ping readonly PROBE_TIMEOUT_SECONDS=3 # Tried after the leased VM IP so a probe still works before the lease lands. @@ -96,6 +98,7 @@ HOSTS="" HOSTS="$HOSTS $FALLBACK_HOSTS" dead="" +busy="" for p in $PORTS; do found=0 for host in $HOSTS; do @@ -105,8 +108,13 @@ for p in $PORTS; do # dead port as UP. This check could therefore never fail: a total # outage sat behind a green healthcheck for hours because Docker was # told everything was fine. - code=$(curl -s --max-time "$PROBE_TIMEOUT_SECONDS" -o /dev/null \ - -w '%{http_code}' "http://$host:$p$PROBE_PATH" 2>/dev/null) + # + # time_connect comes back alongside the status because the two ways a + # probe fails need opposite verdicts — see the busy branch below. + probe=$(curl -s --max-time "$PROBE_TIMEOUT_SECONDS" -o /dev/null \ + -w '%{http_code} %{time_connect}' "http://$host:$p$PROBE_PATH" 2>/dev/null) + code=${probe%% *} + connect=${probe##* } # Whitelist the valid shape instead of blacklisting one bad string, so # any future malformed value fails CLOSED rather than open. Empty means # curl is missing or crashed. Any real HTTP status — including 4xx/5xx, @@ -117,6 +125,26 @@ for p in $PORTS; do break ;; esac + # A BUSY VM IS NOT A DEAD VM. + # + # If the TCP handshake completed, something is listening on that port — + # the process is alive, it just did not answer within the probe window. + # That happens whenever the guest is CPU-saturated: a compile, a + # Strategy Tester run, or a backtest is enough. Reporting it DOWN makes + # a supervisor restart a VM that was merely working, which turns a slow + # batch into an outage and loses whatever was running. + # + # Nothing listening refuses the connection instead, leaving + # time_connect at 0.000 — that is the case this healthcheck exists to + # catch, and it still fails. + case "$connect" in + 0.000000 | 0.000 | 0 | "") ;; + *) + busy="$busy $p" + found=1 + break + ;; + esac done [ "$found" -eq 0 ] && dead="$dead $p" done @@ -126,6 +154,13 @@ if [ -n "$dead" ]; then exit 1 fi +# Healthy, but say so out loud: a port that only ever answers this way is worth +# looking at even though it is not a restart-worthy fault. +if [ -n "$busy" ]; then + echo "ok (slow but listening:$busy) all ports up:" $PORTS "(vm_ip=$VM_IP)" + exit 0 +fi + # Unquoted on purpose: collapses the newline-separated list onto one line, so # the verdict `docker inspect` shows stays readable. echo "ok all ports up:" $PORTS "(vm_ip=$VM_IP)" diff --git a/scripts/vm-watchdog.py b/scripts/vm-watchdog.py new file mode 100755 index 00000000..6643dff6 --- /dev/null +++ b/scripts/vm-watchdog.py @@ -0,0 +1,501 @@ +#!/usr/bin/env python3 +"""Compose-managed VM crash watchdog. + +dockurr/windows keeps the container "up" while the Windows guest may have +crashed internally (e.g. Event ID 6008 "The previous system shutdown ... was +unexpected"). Docker's healthcheck then reports ``unhealthy``, but because the +container never exits, ``restart: unless-stopped`` never fires and every +terminal API inside that VM stays dead until a human restarts it. This sidecar +is that human, running as a normal Compose service instead of a host cron job. + +It polls Docker health through the mounted unix socket and restarts a VM +container ONLY after its Docker health has stayed ``unhealthy`` for a sustained +``FailingStreak`` — the same source of truth ``docker inspect`` exposes. A +healthy or merely-starting VM is never touched, so long-running backtests on a +working VM are never interrupted; the healthcheck goes green the whole time a +terminal is serving, and only a genuinely dead VM stays red. + +Restart policy is stateful, not a fixed cooldown: + +- A tiny JSON state record per container lives on a named volume + (``/state/.json``): last restart time, attempt count, and when + the VM was last observed healthy. +- After the sustained-unhealthy threshold, the VM is restarted. Attempts gate + an exponential backoff (default 5m -> 15m -> 1h): a VM that crashes again + immediately after recovery is not restarted into a loop. +- After ``WATCHDOG_MAX_ATTEMPTS`` consecutive failed recoveries the watchdog + stops trying and logs loudly, so a genuinely broken VM does not thresh forever. +- Attempts reset only after the VM has stayed healthy for + ``WATCHDOG_RESET_SECONDS``, so a VM that recovered then crashed again later + gets a fresh budget instead of inheriting old failures. + +Scope is strict: only containers in this Compose project running the +``dockurr/windows`` image are considered, so nginx, wickworks, the log rotator +and the watchdog itself are never touched. + +Recovery is a COORDINATED RECREATE, not a restart. A sidecar sharing the VM's +network namespace (``network_mode: service:``, i.e. wickworks) resolves +that binding once, at its own start, into an immutable +``NetworkMode=container:``. Restarting the owner keeps its id, but +Docker tears the netns down on stop and builds a fresh one on start, so the +sidecar is left holding a dead namespace. This module previously claimed the +opposite; ``tests/integration/test_wickworks_lifecycle.py`` disproves it, +asserting that BOTH "recreate owner alone" and "restart owner alone" strand +the sidecar, and that only recreating the owner together with its sidecars +repairs the binding. + +So the watchdog does not restart through the Docker API. It shells out to +``scripts/recreate-vm.sh`` — the same helper an operator runs by hand, and the +one that lifecycle test covers — which discovers each VM's sidecars from the +generated Compose file and recreates them together. + +That is why this container needs the compose project mounted at the SAME +absolute path the host uses: Compose resolves relative bind mounts +client-side, so ``./scripts/x`` in the compose file has to land on the host's +``/scripts/x``, not on some path inside this container. ``run.sh`` +exports ``MT5_PROJECT_DIR`` for that. Without it the watchdog refuses to act +and says so, rather than falling back to a restart that looks like recovery +and is not. + +Runs forever, restarting itself if it crashes: keepalive is the Compose +``restart: unless-stopped`` on this service, not a supervisor inside the loop. +""" + +from __future__ import annotations + +import copy +import http.client +import json +import os +import socket +import subprocess +import sys +import time +from pathlib import Path + +# ── Configuration (env-overridable, defaults match the sidecar in compose) ── + +# Misconfiguration is collected rather than raised, so importing this module +# never explodes and validate_config() can report EVERY problem at once. An +# operator fixing one variable at a time from successive tracebacks is a worse +# afternoon than one message listing all of them. +CONFIG_ERRORS: list[str] = [] + + +def _env_int(name: str, default: str, minimum: int) -> int: + """Integer from the environment, or the default plus a recorded error. + + Falling back to the default keeps the module importable; validate_config() + is what refuses to run. Blank is rejected explicitly: an unset variable and + one set to "" mean different things to the person who wrote the compose + file, and only the second is a mistake worth naming. + """ + raw = os.environ.get(name) + if raw is None: + raw = default + text = raw.strip() + if not text: + CONFIG_ERRORS.append(f"{name} is set but empty; expected an integer >= {minimum}") + return int(default) + try: + value = int(text) + except ValueError: + CONFIG_ERRORS.append(f"{name}={raw!r} is not an integer") + return int(default) + if value < minimum: + CONFIG_ERRORS.append(f"{name}={value} is below the minimum of {minimum}") + return int(default) + return value + + +def _env_int_list(name: str, default: str, minimum: int) -> list[int]: + """Comma-separated positive integers, never empty. + + The empty case is the one that mattered: WATCHDOG_BACKOFF_ATTEMPTS= parsed + to [], which survived startup and then raised IndexError inside decide() + on the first unhealthy pass after a recorded restart - a crash loop in the + thing that exists to recover from crashes. + """ + raw = os.environ.get(name) + if raw is None: + raw = default + parts = [p.strip() for p in raw.split(",") if p.strip()] + if not parts: + CONFIG_ERRORS.append( + f"{name}={raw!r} parsed to an empty list; expected at least one integer >= {minimum}" + ) + return [int(p) for p in default.split(",")] + values: list[int] = [] + for part in parts: + try: + value = int(part) + except ValueError: + CONFIG_ERRORS.append(f"{name} entry {part!r} is not an integer") + continue + if value < minimum: + CONFIG_ERRORS.append(f"{name} entry {value} is below the minimum of {minimum}") + continue + values.append(value) + if not values: + return [int(p) for p in default.split(",")] + return values + + +def _env_str(name: str, default: str, *, allow_empty: bool) -> str: + raw = os.environ.get(name) + if raw is None: + return default + if not raw.strip() and not allow_empty: + CONFIG_ERRORS.append(f"{name} is set but empty; expected a value") + return default + return raw + + +DOCKER_SOCKET = _env_str("WATCHDOG_DOCKER_SOCKET", "/var/run/docker.sock", allow_empty=False) + +# The coordinated recreate helper, and the compose project it must act on. +# PROJECT_DIR is the HOST path: compose resolves the relative bind mounts in +# docker-compose.yml against it, so a container-local path would rewrite every +# mount to somewhere that does not exist on the host. Empty means "not wired +# up" and is reported by validate_config() rather than guessed at. +RECREATE_SCRIPT = _env_str( + "WATCHDOG_RECREATE_SCRIPT", "/project/scripts/recreate-vm.sh", allow_empty=False +) +PROJECT_DIR = _env_str("WATCHDOG_PROJECT_DIR", "", allow_empty=True) +RECREATE_TIMEOUT_SECONDS = _env_int("WATCHDOG_RECREATE_TIMEOUT", "300", minimum=30) +STATE_DIR = _env_str("WATCHDOG_STATE_DIR", "/state", allow_empty=False) +INTERVAL_SECONDS = _env_int("WATCHDOG_INTERVAL_SECONDS", "30", minimum=1) + +# Restart only after this many consecutive failed healthchecks. +MIN_FAILING_STREAK = _env_int("WATCHDOG_MIN_FAILING_STREAK", "10", minimum=1) + +# Only containers whose image starts with this are considered VM containers. +# Empty is refused rather than treated as "no filter": "".startswith() matches +# every image, so a blank value would make every container in the project a +# restart candidate - including this watchdog and any database sharing it. +IMAGE_FILTER = _env_str("WATCHDOG_IMAGE_FILTER", "dockurr/windows", allow_empty=False) + +# Exponential backoff per attempt: first restart after 5m, then 15m, then 1h. +BACKOFF_ATTEMPTS = _env_int_list("WATCHDOG_BACKOFF_ATTEMPTS", "300,900,3600", minimum=1) + +# Give up (and log loudly) after this many consecutive failed recoveries. +MAX_ATTEMPTS = _env_int("WATCHDOG_MAX_ATTEMPTS", "3", minimum=1) + +# A VM must stay healthy this long before its attempt budget resets. Zero is +# refused: the budget would reset on the first healthy poll after a restart, +# which silently makes MAX_ATTEMPTS unreachable and the give-up path dead code. +RESET_SECONDS = _env_int("WATCHDOG_RESET_SECONDS", "1800", minimum=1) + +# Compose project to scope to. Empty = auto-discover from this container's own +# labels (the watchdog runs as a service in the same project). +COMPOSE_PROJECT = os.environ.get("WATCHDOG_COMPOSE_PROJECT", "") + +# This container's own ID; Docker sets the container hostname to it. +SELF_ID = os.environ.get("WATCHDOG_SELF_ID", socket.gethostname()) + + +def log(message: str, *args) -> None: + stamp = time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) + print(f"[{stamp}] [vm-watchdog] {message}" if not args else f"[{stamp}] [vm-watchdog] {message % args}") + + +# ── Minimal Docker Engine API client over the unix socket (stdlib only) ────── + + +class _UnixSocketConnection(http.client.HTTPConnection): + """HTTPConnection that talks to the Docker Engine over a unix socket.""" + + def __init__(self, socket_path: str): + super().__init__("localhost") + self._socket_path = socket_path + + def connect(self) -> None: + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + sock.connect(self._socket_path) + self.sock = sock + + +class DockerClient: + """A tiny read-only Docker API client. + + Only the endpoints the watchdog needs are implemented: list containers and + inspect one. Recovery deliberately does NOT go through this client — see + recreate_vm() and the module docstring. Anything else the daemon replies + with is surfaced as a RuntimeError carrying the status + body. + """ + + def __init__(self, socket_path: str = DOCKER_SOCKET): + self._socket_path = socket_path + + def _request(self, method: str, path: str, body: str | None = None) -> bytes: + conn = _UnixSocketConnection(self._socket_path) + try: + conn.request(method, path, body=body) + resp = conn.getresponse() + data = resp.read() + status = resp.status + finally: + conn.close() + if not (200 <= status < 300): + raise RuntimeError( + f"docker {method} {path}: HTTP {status}: {data[:300]!r}" + ) + return data + + def list_containers(self) -> list[dict]: + """Running containers (Docker's default ``/containers/json``).""" + data = self._request("GET", "/containers/json") + return json.loads(data) if data else [] + + def inspect(self, container_id: str) -> dict: + data = self._request("GET", f"/containers/{container_id}/json") + return json.loads(data) + + + +def recreate_vm(service: str, project: str) -> None: + """Recreate one VM together with every sidecar sharing its netns. + + Delegates to scripts/recreate-vm.sh instead of reimplementing it: that + script is what operators run, what the lifecycle regression test exercises, + and the only place that knows how to find a VM's sidecars in the generated + compose file. Reimplementing the discovery here would give the fleet two + recovery paths that could drift apart. + + COMPOSE_PROJECT_NAME is passed explicitly. Compose otherwise derives the + project from the directory name, and a mismatch there would not fail — it + would quietly create a SECOND set of containers alongside the running ones. + """ + if not PROJECT_DIR: + raise RuntimeError( + "WATCHDOG_PROJECT_DIR is unset, so the compose project cannot be " + "recreated. Start the stack through run.sh (it exports " + "MT5_PROJECT_DIR), or set it to the host path of the project." + ) + env = dict(os.environ, COMPOSE_PROJECT_NAME=project) + result = subprocess.run( + [RECREATE_SCRIPT, service], + cwd=PROJECT_DIR, + env=env, + capture_output=True, + text=True, + timeout=RECREATE_TIMEOUT_SECONDS, + check=False, + ) + if result.returncode != 0: + detail = (result.stderr or result.stdout or "").strip()[-500:] + raise RuntimeError( + f"{RECREATE_SCRIPT} {service} exited {result.returncode}: {detail}" + ) + + +# ── Pure policy: state + health in, action out ─────────────────────────────── + + +def load_state(container_id: str, state_dir: str | None = None) -> dict: + """The per-container state record, or a fresh one when absent/corrupt.""" + path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{container_id}.json" + if not path.exists(): + return {"last_restart": 0, "attempts": 0, "healthy_since": 0} + try: + return json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError): + return {"last_restart": 0, "attempts": 0, "healthy_since": 0} + + +def save_state(container_id: str, state: dict, state_dir: str | None = None) -> None: + path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{container_id}.json" + path.parent.mkdir(parents=True, exist_ok=True) + tmp = path.with_suffix(".tmp") + tmp.write_text(json.dumps(state), encoding="utf-8") + tmp.replace(path) + + +def decide(state: dict, health_status: str, failing_streak: int, now: int) -> tuple[str, str]: + """One container's next action given its state and Docker health. + + Mutates ``state`` (attempts / timestamps) and returns ``(action, reason)`` + where action is one of ``wait``, ``restart`` or ``give_up``. + """ + if health_status != "unhealthy": + # Healthy or starting: never touch. Track when we last saw it healthy + # so a recovery that holds long enough resets the attempt budget. + if health_status == "healthy": + state["healthy_since"] = state.get("healthy_since") or now + if now - state["healthy_since"] >= RESET_SECONDS: + state["attempts"] = 0 + state["last_restart"] = 0 + return "wait", "healthy long enough - attempt budget reset" + else: + # starting / none: nothing to do, do not start a healthy clock. + state["healthy_since"] = state.get("healthy_since") or 0 + return "wait", f"health={health_status}" + + # Unhealthy from here on. + if failing_streak < MIN_FAILING_STREAK: + return "wait", f"unhealthy streak {failing_streak} < {MIN_FAILING_STREAK}" + + attempts = state.get("attempts", 0) + if attempts >= MAX_ATTEMPTS: + return "give_up", f"attempts {attempts} >= MAX_ATTEMPTS {MAX_ATTEMPTS}" + + last_restart = state.get("last_restart", 0) + # Delay before the NEXT restart, indexed by how many restarts already + # happened: after the 1st restart wait BACKOFF[0], after the 2nd wait + # BACKOFF[1], etc. First restart has no prior wait. + delay = BACKOFF_ATTEMPTS[max(0, min(attempts - 1, len(BACKOFF_ATTEMPTS) - 1))] if attempts else 0 + if last_restart and (now - last_restart) < delay: + return "wait", f"backoff {now - last_restart}s < {delay}s (attempt {attempts + 1})" + + state["attempts"] = attempts + 1 + state["last_restart"] = now + state["healthy_since"] = 0 + return "restart", f"unhealthy streak {failing_streak} >= {MIN_FAILING_STREAK} (attempt {attempts + 1})" + + +# ── Docker-facing logic ────────────────────────────────────────────────────── + + +def _compose_project(client: DockerClient) -> str | None: + """This project's name, from our own container's compose labels.""" + try: + info = client.inspect(SELF_ID) + except (RuntimeError, OSError): + return None + return (info.get("Config", {}).get("Labels", {}) or {}).get("com.docker.compose.project") + + +def _scoped_containers(client: DockerClient, project: str) -> list[dict]: + """Running containers in this project running the VM image.""" + out = [] + for c in client.list_containers(): + labels = c.get("Labels", {}) or {} + if project and labels.get("com.docker.compose.project") != project: + continue + if not (c.get("Image") or "").startswith(IMAGE_FILTER): + continue + out.append(c) + return out + + +def _health_of(client: DockerClient, container_id: str) -> tuple[str, int]: + """(State.Health.Status, State.Health.FailingStreak) or ('none', 0).""" + info = client.inspect(container_id) + health = info.get("State", {}).get("Health", {}) or {} + status = health.get("Status", "none") + streak = health.get("FailingStreak", 0) + return status, int(streak or 0) + + +def sweep_once(client: DockerClient, project: str, dry_run: bool = False, now: int | None = None) -> int: + """One pass over the project's VM containers. Returns recovery count.""" + now = now if now is not None else int(time.time()) + restarted = 0 + for c in _scoped_containers(client, project): + cid = c["Id"] + name = (c.get("Names") or ["?"])[0] + try: + status, streak = _health_of(client, cid) + except (RuntimeError, OSError) as exc: + log(f"{name}: cannot inspect health ({exc}); skipping") + continue + state = load_state(cid) + # decide() MUTATES the state it is handed - it increments attempts and + # stamps last_restart. Under --dry-run that must not reach disk: a dry + # pass would consume the real backoff and attempt budget without + # restarting anything, so enough dry passes leave the VM at GIVING UP + # the moment dry-run is switched off. Evaluate against a copy instead, + # and persist nothing. + working = copy.deepcopy(state) if dry_run else state + action, reason = decide(working, status, streak, now) + if not dry_run: + save_state(cid, working) + if action == "restart": + # Recreate by COMPOSE SERVICE, not container id: the helper has to + # name the VM and its sidecars as compose services to recreate them + # together, and a container id means nothing to compose. + service = (c.get("Labels") or {}).get("com.docker.compose.service") + if not service: + log(f"{name}: no compose service label; cannot recreate, skipping") + continue + if dry_run: + log(f"DRY-RUN: would recreate {service} (+ its sidecars) - {reason}") + else: + try: + recreate_vm(service, project) + log(f"recreated {service} and its sidecars - {reason}") + except (RuntimeError, OSError, subprocess.SubprocessError) as exc: + log(f"{name}: recreate failed ({exc}); state kept for backoff") + restarted += 1 + elif action == "give_up": + log(f"GIVING UP on {name} after {working.get('attempts', 0)} attempts - {reason}") + return restarted + + +def validate_config() -> list[str]: + """Every configuration problem found at import, as human-readable lines. + + Separate from main() so a test can assert on the messages without running + the daemon, and so an operator can see all of them in one go. + + The recovery wiring is checked here rather than at the moment a VM dies: + finding out that the watchdog cannot act only once something has already + crashed is the worst possible time to learn it. + """ + errors = list(CONFIG_ERRORS) + if not PROJECT_DIR: + errors.append( + "WATCHDOG_PROJECT_DIR is empty - recovery needs the compose project's " + "HOST path (run.sh exports MT5_PROJECT_DIR). Without it a crashed VM " + "cannot be recreated." + ) + elif not os.path.isdir(PROJECT_DIR): + errors.append( + f"WATCHDOG_PROJECT_DIR={PROJECT_DIR!r} is not a directory in this " + "container - mount the project through at the same absolute path the " + "host uses, or compose will rewrite every relative bind mount." + ) + if not os.path.exists(RECREATE_SCRIPT): + errors.append( + f"WATCHDOG_RECREATE_SCRIPT={RECREATE_SCRIPT!r} not found - the " + "coordinated recreate helper must be mounted into this container." + ) + return errors + + +def main() -> int: + problems = validate_config() + if problems: + # Refuse to start rather than run on defaults. This daemon restarts + # containers; quietly substituting values an operator did not choose is + # the wrong failure mode for something holding the Docker socket. + log(f"invalid configuration ({len(problems)} problem(s)); refusing to start") + for problem in problems: + log(f" {problem}") + return 2 + + dry_run = "--dry-run" in sys.argv + if dry_run: + log("dry-run mode: will report decisions without restarting") + client = DockerClient(DOCKER_SOCKET) + + project = COMPOSE_PROJECT or _compose_project(client) or "" + if not project: + log("cannot determine compose project (no WATCHDOG_COMPOSE_PROJECT and " + "self-inspect found no compose labels); refusing to run un-scoped") + return 1 + log(f"scoped to compose project '{project}' (image filter '{IMAGE_FILTER}')") + + while True: + try: + sweep_once(client, project, dry_run=dry_run) + except (RuntimeError, OSError) as exc: + log(f"sweep failed: {exc}") + time.sleep(INTERVAL_SECONDS) + + +if __name__ == "__main__": + try: + sys.exit(main()) + except KeyboardInterrupt: + sys.exit(0) diff --git a/tests/test_healthcheck_behavior.py b/tests/test_healthcheck_behavior.py index 1f32aeb7..6642ab74 100644 --- a/tests/test_healthcheck_behavior.py +++ b/tests/test_healthcheck_behavior.py @@ -189,3 +189,78 @@ def test_group_file_ignores_comments_and_blank_lines(awk_prog, tmp_path): ports = _run_awk(awk_prog, str(groupfile), config) assert ports == ["6002", "6005"] + + +# ── Verdict: is the VM dead, or just busy? ─────────────────────────────────── +# +# These run the whole script with a stub `curl` on PATH, so they exercise the +# real verdict logic rather than the awk filter alone. + +def _run_healthcheck(tmp_path, config, curl_body): + """Run healthcheck.sh with a fake curl that emits `curl_body` on stdout. + + The stub mimics curl closely enough for the script: it writes what a real + `-w '%{http_code} %{time_connect}'` would print, and exits non-zero when + the status is 000, exactly as curl does on a failed request. + """ + bindir = tmp_path / "bin" + bindir.mkdir(exist_ok=True) + curl = bindir / "curl" + curl.write_text( + "#!/bin/sh\n" + f"printf '%s' '{curl_body}'\n" + f"case '{curl_body}' in 000*) exit 7 ;; esac\n" + "exit 0\n", + encoding="utf-8", + ) + curl.chmod(0o755) + + env = { + "PATH": f"{bindir}:/usr/bin:/bin", + "HEALTHCHECK_CONFIG": str(config), + "HEALTHCHECK_VM_GROUP": str(tmp_path / "no-such-group.txt"), + "HEALTHCHECK_LEASES": str(tmp_path / "no-such-leases"), + } + return subprocess.run( + ["sh", str(HEALTHCHECK_PATH)], + capture_output=True, text=True, env=env, timeout=60, + ) + + +ONE_TERMINAL = [{"broker": "darwinex", "account": "live", "port": "6001"}] + + +def test_a_port_that_answers_is_healthy(tmp_path): + config = _write_config(tmp_path, ONE_TERMINAL) + result = _run_healthcheck(tmp_path, config, "200 0.000181") + assert result.returncode == 0, result.stdout + result.stderr + + +def test_a_port_with_nothing_listening_is_unhealthy(tmp_path): + """Connection refused leaves time_connect at zero. This is the outage the + healthcheck exists to catch, and it must still fail.""" + config = _write_config(tmp_path, ONE_TERMINAL) + result = _run_healthcheck(tmp_path, config, "000 0.000000") + assert result.returncode == 1, result.stdout + result.stderr + assert "DOWN" in result.stdout + + +def test_a_listening_but_slow_port_is_healthy(tmp_path): + """The regression this pair exists for. + + A completed TCP handshake with no HTTP response in the probe window means + the process is alive and the guest is merely CPU-saturated - a compile or a + Strategy Tester run will do it. Reporting DOWN here makes a supervisor + restart a VM that was working, turning a slow batch into an outage. + """ + config = _write_config(tmp_path, ONE_TERMINAL) + result = _run_healthcheck(tmp_path, config, "000 0.001204") + assert result.returncode == 0, result.stdout + result.stderr + assert "slow but listening" in result.stdout + + +def test_a_missing_curl_fails_closed(tmp_path): + """An empty probe result must read as DOWN, not as 'not zero, so busy'.""" + config = _write_config(tmp_path, ONE_TERMINAL) + result = _run_healthcheck(tmp_path, config, "") + assert result.returncode == 1, result.stdout + result.stderr diff --git a/tests/test_vm_watchdog.py b/tests/test_vm_watchdog.py new file mode 100644 index 00000000..55dd7b4f --- /dev/null +++ b/tests/test_vm_watchdog.py @@ -0,0 +1,602 @@ +"""Behavioural coverage for the compose-managed vm-watchdog sidecar. + +These tests exercise the decision policy against a fake Docker transport, so +they run in the offline suite (Dockerfile.test) with no docker daemon. They +cover the behaviours psyb0t asked for on PR #15: healthy/starting exclusion, +sustained unhealthy restart, image/label scoping, cooldown/backoff, bounded +retries, and reset after stable health. +""" + +import importlib.util +import json +import os +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "vm-watchdog.py" + + +def _load(): + spec = importlib.util.spec_from_file_location("vm_watchdog_test", SCRIPT) + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + return module + + +@pytest.fixture +def wd(): + module = _load() + module.STATE_DIR = "unused-in-policy" + return module + + +class _Recorder: + """Captures recreate_vm() calls in place of the old restart log.""" + + def __init__(self): + self.calls = [] + self.raises = None + + @property + def services(self): + return [service for service, _project in self.calls] + + def __call__(self, service, project): + if self.raises: + raise self.raises + self.calls.append((service, project)) + + +@pytest.fixture +def recorder(wd, monkeypatch): + rec = _Recorder() + monkeypatch.setattr(wd, "recreate_vm", rec) + return rec + + +# ── Fake Docker transport ──────────────────────────────────────────────────── + + +class _FakeClient: + """Scripted docker client: containers list and per-id health. + + Recovery is deliberately NOT on this client any more: a restart through the + Docker API strands the wickworks sidecar (fresh netns on start), so the + watchdog shells out to scripts/recreate-vm.sh instead. The recreate calls + it makes are captured by the `recorder` fixture below. + """ + + def __init__(self, containers, health=None, project="mt5-httpapi"): + self.containers = containers + self.health = health or {} + self.project = project + + def list_containers(self): + return self.containers + + def inspect(self, cid): + if cid == "self-watchdog-id": + return { + "Config": { + "Labels": {"com.docker.compose.project": self.project} + } + } + if cid in self.health: + return {"State": {"Health": self.health[cid]}} + return {"State": {}} + + + +def _container(cid, name, image="dockurr/windows:5.14", labels=None): + return { + "Id": cid, + "Names": [f"/{name}"], + "Image": image, + # The service label is what a recreate names; default it to the id so + # the assertions below can keep talking about one identifier. + "Labels": labels or { + "com.docker.compose.project": "mt5-httpapi", + "com.docker.compose.service": cid, + }, + } + + +def _health(status, streak): + return {"Status": status, "FailingStreak": streak} + + +# ── Healthy / starting exclusion ───────────────────────────────────────────── + +def test_healthy_vm_is_never_restarted(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5")], + health={cid: _health("healthy", 0)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi") == 0 + assert recorder.services == [] + # healthy clock starts, but nothing is reset yet (below reset window). + state = json.loads((tmp_path / f"{cid}.json").read_text()) + assert state["healthy_since"] > 0 + assert state["attempts"] == 0 + + +def test_starting_vm_is_never_restarted(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5")], + health={cid: _health("starting", 0)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi") == 0 + assert recorder.services == [] + # starting must not start the healthy clock either. + state = json.loads((tmp_path / f"{cid}.json").read_text()) + assert state["healthy_since"] == 0 + + +# ── Sustained unhealthy threshold ──────────────────────────────────────────── + +def test_unhealthy_below_streak_threshold_waits(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 10 + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5")], + health={cid: _health("unhealthy", 4)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi") == 0 + assert recorder.services == [] + + +def test_unhealthy_at_streak_threshold_restarts(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 10 + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5")], + health={cid: _health("unhealthy", 10)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi") == 1 + assert recorder.services == [cid] + + +# ── Image / label scoping ──────────────────────────────────────────────────── + +def test_only_this_projects_vm_image_containers_are_restarted(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + vm = _container("vm1", "mt5") + other_project = _container("other1", "some-other-vm", labels={"com.docker.compose.project": "other"}) + sidecar = _container("side1", "wickworks", image="psyb0t/wickworks:v0.3.1") + logrotator = _container("rot1", "log-rotator", image="alpine:3.20") + client = _FakeClient( + [vm, other_project, sidecar, logrotator], + health={ + "vm1": _health("unhealthy", 99), + "other1": _health("unhealthy", 99), + "side1": _health("unhealthy", 99), + "rot1": _health("unhealthy", 99), + }, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi") == 1 + assert recorder.services == ["vm1"] + + +# ── Backoff ────────────────────────────────────────────────────────────────── + +def test_exponential_backoff_between_restarts(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.BACKOFF_ATTEMPTS = [300, 900, 3600] + wd.MAX_ATTEMPTS = 99 + cid = "aaa" + client = _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 99)}) + + now = 1_000_000 + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + # First restart is immediate at threshold. + assert wd.sweep_once(client, "mt5-httpapi", now=now) == 1 + # After 1st restart, backoff tier 0 = 300s: too soon at +299, ok at +301. + assert wd.sweep_once(client, "mt5-httpapi", now=now + 299) == 0 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 301) == 1 + # After 2nd restart (at +301), backoff tier 1 = 900s. + assert wd.sweep_once(client, "mt5-httpapi", now=now + 302) == 0 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 301 + 901) == 1 + # After 3rd restart (at +1202), backoff tier 2 = 3600s. + assert wd.sweep_once(client, "mt5-httpapi", now=now + 1203) == 0 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 1202 + 3601) == 1 + + assert recorder.services == [cid, cid, cid, cid] + state = json.loads((tmp_path / f"{cid}.json").read_text()) + assert state["attempts"] == 4 + + +# ── Bounded retries ────────────────────────────────────────────────────────── + +def test_gives_up_after_max_attempts(wd, tmp_path, capsys, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.MAX_ATTEMPTS = 3 + wd.BACKOFF_ATTEMPTS = [0, 0, 0] # no backoff: three rapid attempts + cid = "aaa" + client = _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 99)}) + + now = 1_000_000 + assert wd.sweep_once(client, "mt5-httpapi", now=now) == 1 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 1) == 1 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 2) == 1 + # Attempt budget exhausted: no more restarts, and it logs loudly. + assert wd.sweep_once(client, "mt5-httpapi", now=now + 3) == 0 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 999_999) == 0 + + assert recorder.services == [cid, cid, cid] + assert "GIVING UP" in capsys.readouterr().out + + +# ── Reset after stable health ──────────────────────────────────────────────── + +def test_attempts_reset_after_sustained_healthy_period(wd, tmp_path): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.MAX_ATTEMPTS = 1 + wd.BACKOFF_ATTEMPTS = [0] + wd.RESET_SECONDS = 1800 + cid = "aaa" + + def make_client(status, streak): + return _FakeClient([_container(cid, "mt5")], health={cid: _health(status, streak)}) + + now = 1_000_000 + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + # Two unhealthy sweeps: first restarts, second exceeds MAX_ATTEMPTS. + assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now) == 1 + assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now + 1) == 0 + assert "give up" in json.loads((tmp_path / f"{cid}.json").read_text()) or True + + # VM recovers and stays healthy long enough to reset the budget. + assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100) == 0 + assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100 + 1800) == 0 + state = json.loads((tmp_path / f"{cid}.json").read_text()) + assert state["attempts"] == 0 + assert state["last_restart"] == 0 + + # A later crash gets a fresh budget again. + assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now + 100 + 1801) == 1 + + +# ── Dry-run ────────────────────────────────────────────────────────────────── + +def test_dry_run_reports_without_restarting(wd, tmp_path, recorder): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "aaa" + client = _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 99)}) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", dry_run=True) == 1 + assert recorder.services == [] + + +def test_dry_run_writes_no_state(wd, tmp_path, recorder): + """A dry pass must not consume the real backoff or attempt budget. + + decide() mutates the state it is handed, and sweep_once used to persist it + before branching on dry_run. So --dry-run spent attempts and stamped + last_restart without restarting anything, and enough dry passes left the VM + at GIVING UP the moment dry-run was switched off - the supervisor refusing + to act precisely when it was finally allowed to. + """ + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "aaa" + client = _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 10)}) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", dry_run=True, now=1000000) == 1 + + assert recorder.services == [] + written = list(tmp_path.iterdir()) + assert written == [], f"dry-run persisted state: {[p.name for p in written]}" + + +def test_repeated_dry_runs_do_not_exhaust_the_attempt_budget(wd, tmp_path, recorder): + """The consequence, end to end: dry passes then a real one. + + Whatever the attempt cap is, running dry that many times and then switching + dry-run off must still restart. Asserting on the restart rather than on a + number keeps this honest if the cap ever moves. + """ + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "aaa" + + def fresh(): + return _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 10)}) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + for i in range(wd.MAX_ATTEMPTS + 2): + wd.sweep_once(fresh(), "mt5-httpapi", dry_run=True, now=1000000 + i * 100000) + wd.sweep_once(fresh(), "mt5-httpapi", dry_run=False, now=2000000) + + assert recorder.services == [cid], "dry runs consumed the budget for the real one" + + +def test_a_real_run_still_persists_state(wd, tmp_path, recorder): + """The other direction: skipping persistence must be dry-run only.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "aaa" + client = _FakeClient([_container(cid, "mt5")], health={cid: _health("unhealthy", 10)}) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + wd.sweep_once(client, "mt5-httpapi", dry_run=False, now=1000000) + + assert recorder.services == [cid] + state = wd.load_state(cid) + assert state["attempts"] == 1 + assert state["last_restart"] == 1000000 + + +# ── Project self-discovery ─────────────────────────────────────────────────── + +def test_compose_project_discovered_from_own_labels(wd): + client = _FakeClient([], project="mt5-httpapi") + wd.SELF_ID = "self-watchdog-id" + assert wd._compose_project(client) == "mt5-httpapi" + + +def test_recreate_failure_keeps_state_for_backoff(wd, tmp_path, recorder): + """A failed recreate must still record the attempt + timestamp so the + backoff logic prevents an immediate retry loop.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.BACKOFF_ATTEMPTS = [300] + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5")], + health={cid: _health("unhealthy", 99)}, + ) + recorder.raises = RuntimeError("boom") + now = 1_000_000 + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", now=now) == 1 + # Recreate failed, but the state now enforces the backoff window. + assert wd.sweep_once(client, "mt5-httpapi", now=now + 10) == 0 + assert wd.sweep_once(client, "mt5-httpapi", now=now + 301) == 1 + assert recorder.services == [] + + +# ── Configuration parsing ──────────────────────────────────────────────────── +# +# The daemon holds the Docker socket, so bad configuration has to stop it at +# startup rather than surface as a crash mid-sweep. Reported by psyb0t: an +# empty WATCHDOG_BACKOFF_ATTEMPTS survived startup as [] and then raised +# IndexError inside decide() on the first unhealthy pass after a restart. + +_REPO = SCRIPT.resolve().parents[1] + + +def _load_with(monkeypatch, **env): + for key in list(os.environ): + if key.startswith("WATCHDOG_"): + monkeypatch.delenv(key, raising=False) + # The recovery wiring is supplied by the compose service, not defaulted in + # the script, so provide it here the way the shipped compose does. Tests + # that care about it missing pass it explicitly. + env.setdefault("WATCHDOG_PROJECT_DIR", str(_REPO)) + env.setdefault("WATCHDOG_RECREATE_SCRIPT", str(_REPO / "scripts" / "recreate-vm.sh")) + for key, value in env.items(): + monkeypatch.setenv(key, value) + return _load() + + +def test_empty_backoff_list_is_refused_at_startup(monkeypatch): + """The reported crash. [] survived startup, then IndexError'd in decide().""" + wd = _load_with(monkeypatch, WATCHDOG_BACKOFF_ATTEMPTS="") + problems = wd.validate_config() + assert any("WATCHDOG_BACKOFF_ATTEMPTS" in p for p in problems), problems + assert wd.main() == 2 + + +def test_a_refused_config_never_reaches_the_indexing_that_crashed(monkeypatch): + """Defence in depth: even if validation were bypassed, the fallback value + must be usable rather than empty, so decide() cannot raise IndexError.""" + wd = _load_with(monkeypatch, WATCHDOG_BACKOFF_ATTEMPTS="") + assert wd.BACKOFF_ATTEMPTS, "fell back to an empty list" + state = {"attempts": 1, "last_restart": 1000, "healthy_since": 0} + wd.decide(state, "unhealthy", 99, 1000) # must not raise + + +@pytest.mark.parametrize("value", ["", " ", "abc", "0", "-1"]) +def test_bad_interval_values_are_reported(monkeypatch, value): + wd = _load_with(monkeypatch, WATCHDOG_INTERVAL_SECONDS=value) + assert any("WATCHDOG_INTERVAL_SECONDS" in p for p in wd.validate_config()), value + + +@pytest.mark.parametrize( + "name,value", + [ + ("WATCHDOG_MIN_FAILING_STREAK", "0"), + ("WATCHDOG_MAX_ATTEMPTS", "0"), + ("WATCHDOG_RESET_SECONDS", "0"), + ("WATCHDOG_BACKOFF_ATTEMPTS", "300,0"), + ("WATCHDOG_BACKOFF_ATTEMPTS", "300,-5"), + ("WATCHDOG_BACKOFF_ATTEMPTS", "300,abc"), + ], +) +def test_zero_and_negative_are_rejected_where_they_make_no_sense(monkeypatch, name, value): + wd = _load_with(monkeypatch, **{name: value}) + assert any(name in p for p in wd.validate_config()), (name, value) + + +def test_an_empty_image_filter_is_refused(monkeypatch): + """Not in the report, same class of bug and worse consequences. + + "".startswith() matches every image, so a blank filter would make every + container in the project a restart candidate - the watchdog itself, and any + database sharing the project. + """ + wd = _load_with(monkeypatch, WATCHDOG_IMAGE_FILTER="") + assert any("WATCHDOG_IMAGE_FILTER" in p for p in wd.validate_config()) + assert wd.main() == 2 + + +def test_every_problem_is_reported_at_once(monkeypatch): + """One message listing all of them beats fixing them one traceback at a time.""" + wd = _load_with( + monkeypatch, + WATCHDOG_BACKOFF_ATTEMPTS="", + WATCHDOG_INTERVAL_SECONDS="0", + WATCHDOG_MAX_ATTEMPTS="nope", + ) + problems = wd.validate_config() + assert len(problems) >= 3, problems + + +def test_a_valid_configuration_reports_nothing(monkeypatch): + wd = _load_with( + monkeypatch, + WATCHDOG_BACKOFF_ATTEMPTS="60, 120 ,240", + WATCHDOG_INTERVAL_SECONDS="15", + WATCHDOG_MAX_ATTEMPTS="5", + ) + assert wd.validate_config() == [] + assert wd.BACKOFF_ATTEMPTS == [60, 120, 240] + assert wd.INTERVAL_SECONDS == 15 + + +def test_defaults_alone_are_valid(monkeypatch): + """The shipped compose sets almost nothing, so the defaults must pass.""" + wd = _load_with(monkeypatch) + assert wd.validate_config() == [] + + +# ── Coordinated recreate ───────────────────────────────────────────────────── +# +# Why recovery is a recreate and not a restart: a sidecar sharing the VM's netns +# (`network_mode: service:`) resolves that binding once, at its own start. +# Restarting the owner keeps its container id but gives it a FRESH netns, so the +# sidecar is left on a dead one — tests/integration/test_wickworks_lifecycle.py +# asserts exactly that. Only recreating the owner together with its sidecars +# repairs it, which is what scripts/recreate-vm.sh does. + + +def test_recovery_names_the_compose_service_not_the_container_id(wd, tmp_path, recorder): + """recreate-vm.sh takes compose service names; a container id means nothing + to compose, and passing one would recreate nothing at all.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "9f3c1a2b4d5e" + client = _FakeClient( + [_container(cid, "mt5", labels={ + "com.docker.compose.project": "mt5-httpapi", + "com.docker.compose.service": "mt5-fast", + })], + health={cid: _health("unhealthy", 99)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", now=1_000_000) == 1 + assert recorder.calls == [("mt5-fast", "mt5-httpapi")] + + +def test_a_vm_without_a_service_label_is_skipped_not_guessed(wd, tmp_path, recorder): + """Compose always sets the label; something hand-run may not. Skipping is + the honest outcome — inventing a service name would recreate the wrong + thing, or silently nothing.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + cid = "aaa" + client = _FakeClient( + [_container(cid, "mt5", labels={"com.docker.compose.project": "mt5-httpapi"})], + health={cid: _health("unhealthy", 99)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", now=1_000_000) == 0 + assert recorder.calls == [] + + +def test_recreate_refuses_without_the_project_path(wd, monkeypatch): + """Compose resolves this project's relative bind mounts client-side, so + without the host path the recreate would rewrite every mount. Refuse loudly + rather than run a compose that quietly mounts the wrong things.""" + monkeypatch.setattr(wd, "PROJECT_DIR", "") + with pytest.raises(RuntimeError, match="WATCHDOG_PROJECT_DIR"): + wd.recreate_vm("mt5", "mt5-httpapi") + + +def test_recreate_passes_the_project_name_explicitly(wd, monkeypatch, tmp_path): + """Compose derives the project from the directory name otherwise, and a + mismatch does not fail — it creates a SECOND set of containers beside the + running ones.""" + seen = {} + + def fake_run(cmd, **kwargs): + seen["cmd"] = cmd + seen["cwd"] = kwargs.get("cwd") + seen["project"] = (kwargs.get("env") or {}).get("COMPOSE_PROJECT_NAME") + + class R: + returncode = 0 + stdout = "" + stderr = "" + + return R() + + monkeypatch.setattr(wd, "PROJECT_DIR", str(tmp_path)) + monkeypatch.setattr(wd, "RECREATE_SCRIPT", "/project/scripts/recreate-vm.sh") + monkeypatch.setattr(wd.subprocess, "run", fake_run) + + wd.recreate_vm("mt5", "mt5-httpapi") + + assert seen["cmd"] == ["/project/scripts/recreate-vm.sh", "mt5"] + assert seen["cwd"] == str(tmp_path) + assert seen["project"] == "mt5-httpapi" + + +def test_recreate_surfaces_the_scripts_failure(wd, monkeypatch, tmp_path): + def fake_run(cmd, **kwargs): + class R: + returncode = 1 + stdout = "" + stderr = "no such service: mt5" + + return R() + + monkeypatch.setattr(wd, "PROJECT_DIR", str(tmp_path)) + monkeypatch.setattr(wd.subprocess, "run", fake_run) + with pytest.raises(RuntimeError, match="no such service"): + wd.recreate_vm("mt5", "mt5-httpapi") + + +def test_missing_project_dir_is_reported_at_startup(monkeypatch): + """Finding out recovery cannot work only once a VM has crashed is the worst + possible time to learn it.""" + wd = _load_with(monkeypatch, WATCHDOG_PROJECT_DIR="") + assert any("WATCHDOG_PROJECT_DIR" in p for p in wd.validate_config()) + + +def test_missing_recreate_script_is_reported_at_startup(monkeypatch): + wd = _load_with(monkeypatch, WATCHDOG_RECREATE_SCRIPT="/nope/recreate-vm.sh") + assert any("WATCHDOG_RECREATE_SCRIPT" in p for p in wd.validate_config()) From c29b289c68c50020fccddff41f5097d5a703884f Mon Sep 17 00:00:00 2001 From: Marinski Date: Thu, 27 Aug 2026 09:09:43 +0300 Subject: [PATCH 2/4] fix(watchdog): key recovery state by compose service, not container id; pin pyyaml by hash Recovery is a recreate, which replaces the container - so state keyed by container id was orphaned by the very recovery that wrote it. The next poll saw a fresh id, loaded a fresh record at attempts=0, and the attempt cap and backoff reset themselves on every recovery they were meant to bound: a persistently broken VM was recovered forever, always at 'attempt 1'. State is now keyed by stable compose identity (project + service label), which survives the recreate. The service label is resolved before state is touched; a container without one is skipped up front, since it can neither be recreated nor tracked. Labels are sanitized before becoming a file name. Two regression tests drive the exact replacement-id scenario from review: the attempt cap and the backoff window must both survive the recreate they triggered, with the same service returning under a new container id each pass. Both fail against the previous script. Also from review: Dockerfile.watchdog installed unpinned pyyaml at build time in an image that mounts the root-equivalent Docker socket. The dependency is now pinned by version and hash (requirements-watchdog.txt, pip --require-hashes: musllinux cp312 wheels for x86_64/aarch64 plus the sdist), same trust argument as the digest-pinned base image. --- Dockerfile.watchdog | 8 +++- docs/operations.md | 6 ++- requirements-watchdog.txt | 16 ++++++++ scripts/vm-watchdog.py | 50 ++++++++++++++++------- tests/test_vm_watchdog.py | 86 ++++++++++++++++++++++++++++++++++++--- 5 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 requirements-watchdog.txt diff --git a/Dockerfile.watchdog b/Dockerfile.watchdog index 9b34e089..2fcbd605 100644 --- a/Dockerfile.watchdog +++ b/Dockerfile.watchdog @@ -14,9 +14,13 @@ FROM python:3.12-alpine@sha256:d09d15e60962ca365d1cd544a48773bac9d33f2fb1b00f2aa0deec78ade7dc31 # docker-cli-compose provides `docker compose`; recreate-vm.sh is bash and -# parses the compose file with PyYAML. +# parses the compose file with PyYAML. The Python dependency is pinned by +# version and hash (see requirements-watchdog.txt for why); --require-hashes +# makes pip fail closed on anything not in that file. +COPY requirements-watchdog.txt /tmp/requirements-watchdog.txt RUN apk add --no-cache bash docker-cli docker-cli-compose \ - && pip install --no-cache-dir pyyaml + && pip install --no-cache-dir --require-hashes -r /tmp/requirements-watchdog.txt \ + && rm /tmp/requirements-watchdog.txt ENV PYTHONUNBUFFERED=1 \ PYTHONDONTWRITEBYTECODE=1 diff --git a/docs/operations.md b/docs/operations.md index 5ea94bba..f7d7ca29 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -286,8 +286,10 @@ Behavior: healthy or still starting is never touched, so running backtests on a working VM are never interrupted — the healthcheck stays green the whole time a terminal is serving. -- Keeps a tiny state record per container on a named volume - (`/state/.json`): last restart, attempt count, and when the VM +- Keeps a tiny state record per VM on a named volume, keyed by compose + project + service (`/state/..json` — stable across the + recreate that recovery performs, unlike a container id): last restart, + attempt count, and when the VM was last observed healthy. - Enforces exponential backoff between recovery attempts (`WATCHDOG_BACKOFF_ATTEMPTS`, default `300,900,3600` — 5m → 15m → 1h), so a diff --git a/requirements-watchdog.txt b/requirements-watchdog.txt new file mode 100644 index 00000000..106de9fa --- /dev/null +++ b/requirements-watchdog.txt @@ -0,0 +1,16 @@ +# Dependencies baked into the vm-watchdog image (Dockerfile.watchdog). +# +# Pinned by version AND hash: this image mounts the root-equivalent Docker +# socket, so "whatever PyPI serves today" is not an acceptable input to it - +# same argument as the base-image digest pin above it. --require-hashes makes +# pip refuse anything not listed here, including a compromised or yanked +# re-upload under the same version number. +# +# The three hashes are the two musllinux cp312 wheels (python:3.12-alpine on +# x86_64 / aarch64) plus the sdist as a fallback for any other platform. +# To update: pick the new version on https://pypi.org/pypi/PyYAML/json and +# copy the sha256 digests for the matching artifacts. +pyyaml==6.0.2 \ + --hash=sha256:8b9c7197f7cb2738065c481a0461e50ad02f18c78cd75775628afb4d7137fb3b \ + --hash=sha256:0833f8694549e586547b576dcfaba4a6b55b9e96098b36cdc7ebefe667dfed48 \ + --hash=sha256:d584d9ec91ad65861cc08d42e834324ef890a082e591037abe114850ff7bbc3e diff --git a/scripts/vm-watchdog.py b/scripts/vm-watchdog.py index 6643dff6..a87a768b 100755 --- a/scripts/vm-watchdog.py +++ b/scripts/vm-watchdog.py @@ -17,9 +17,13 @@ Restart policy is stateful, not a fixed cooldown: -- A tiny JSON state record per container lives on a named volume - (``/state/.json``): last restart time, attempt count, and when - the VM was last observed healthy. +- A tiny JSON state record per VM lives on a named volume, keyed by STABLE + COMPOSE IDENTITY (``/state/..json``): last restart time, + attempt count, and when the VM was last observed healthy. Never keyed by + container id: recovery here is a recreate, which REPLACES the container, so + an id-keyed record is orphaned by the very action that wrote it and the next + poll would start the replacement at ``attempts = 0`` - every recovery + silently refunding the attempt budget and resetting backoff. - After the sustained-unhealthy threshold, the VM is restarted. Attempts gate an exponential backoff (default 5m -> 15m -> 1h): a VM that crashes again immediately after recovery is not restarted into a loop. @@ -292,9 +296,21 @@ def recreate_vm(service: str, project: str) -> None: # ── Pure policy: state + health in, action out ─────────────────────────────── -def load_state(container_id: str, state_dir: str | None = None) -> dict: - """The per-container state record, or a fresh one when absent/corrupt.""" - path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{container_id}.json" +def state_key(project: str, service: str) -> str: + """Stable identity for one VM's state record. + + Compose project + service survives a recreate; the container id does not. + Sanitized defensively so a hostile-looking label cannot become a path - + both values come from compose labels, but this file name is the only place + they touch the filesystem. + """ + raw = f"{project}.{service}" + return "".join(c if c.isalnum() or c in "._-" else "_" for c in raw) + + +def load_state(key: str, state_dir: str | None = None) -> dict: + """The per-VM state record, or a fresh one when absent/corrupt.""" + path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{key}.json" if not path.exists(): return {"last_restart": 0, "attempts": 0, "healthy_since": 0} try: @@ -303,8 +319,8 @@ def load_state(container_id: str, state_dir: str | None = None) -> dict: return {"last_restart": 0, "attempts": 0, "healthy_since": 0} -def save_state(container_id: str, state: dict, state_dir: str | None = None) -> None: - path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{container_id}.json" +def save_state(key: str, state: dict, state_dir: str | None = None) -> None: + path = Path(state_dir if state_dir is not None else STATE_DIR) / f"{key}.json" path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(".tmp") tmp.write_text(json.dumps(state), encoding="utf-8") @@ -399,7 +415,17 @@ def sweep_once(client: DockerClient, project: str, dry_run: bool = False, now: i except (RuntimeError, OSError) as exc: log(f"{name}: cannot inspect health ({exc}); skipping") continue - state = load_state(cid) + # Resolve the compose service BEFORE touching state: it is both what a + # recreate names and the stable half of the state key. A recreate + # replaces the container, so state keyed by container id was orphaned + # by every successful recovery - the replacement arrived with a fresh + # id, loaded a fresh record, and the attempt cap and backoff never + # carried across the one boundary they exist to police. + service = (c.get("Labels") or {}).get("com.docker.compose.service") + if not service: + log(f"{name}: no compose service label; cannot recreate, skipping") + continue + state = load_state(state_key(project, service)) # decide() MUTATES the state it is handed - it increments attempts and # stamps last_restart. Under --dry-run that must not reach disk: a dry # pass would consume the real backoff and attempt budget without @@ -409,15 +435,11 @@ def sweep_once(client: DockerClient, project: str, dry_run: bool = False, now: i working = copy.deepcopy(state) if dry_run else state action, reason = decide(working, status, streak, now) if not dry_run: - save_state(cid, working) + save_state(state_key(project, service), working) if action == "restart": # Recreate by COMPOSE SERVICE, not container id: the helper has to # name the VM and its sidecars as compose services to recreate them # together, and a container id means nothing to compose. - service = (c.get("Labels") or {}).get("com.docker.compose.service") - if not service: - log(f"{name}: no compose service label; cannot recreate, skipping") - continue if dry_run: log(f"DRY-RUN: would recreate {service} (+ its sidecars) - {reason}") else: diff --git a/tests/test_vm_watchdog.py b/tests/test_vm_watchdog.py index 55dd7b4f..fa806ec3 100644 --- a/tests/test_vm_watchdog.py +++ b/tests/test_vm_watchdog.py @@ -121,7 +121,7 @@ def test_healthy_vm_is_never_restarted(wd, tmp_path, recorder): assert wd.sweep_once(client, "mt5-httpapi") == 0 assert recorder.services == [] # healthy clock starts, but nothing is reset yet (below reset window). - state = json.loads((tmp_path / f"{cid}.json").read_text()) + state = json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) assert state["healthy_since"] > 0 assert state["attempts"] == 0 @@ -138,7 +138,7 @@ def test_starting_vm_is_never_restarted(wd, tmp_path, recorder): assert wd.sweep_once(client, "mt5-httpapi") == 0 assert recorder.services == [] # starting must not start the healthy clock either. - state = json.loads((tmp_path / f"{cid}.json").read_text()) + state = json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) assert state["healthy_since"] == 0 @@ -222,7 +222,7 @@ def test_exponential_backoff_between_restarts(wd, tmp_path, recorder): assert wd.sweep_once(client, "mt5-httpapi", now=now + 1202 + 3601) == 1 assert recorder.services == [cid, cid, cid, cid] - state = json.loads((tmp_path / f"{cid}.json").read_text()) + state = json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) assert state["attempts"] == 4 @@ -267,12 +267,12 @@ def make_client(status, streak): # Two unhealthy sweeps: first restarts, second exceeds MAX_ATTEMPTS. assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now) == 1 assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now + 1) == 0 - assert "give up" in json.loads((tmp_path / f"{cid}.json").read_text()) or True + assert "give up" in json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) or True # VM recovers and stays healthy long enough to reset the budget. assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100) == 0 assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100 + 1800) == 0 - state = json.loads((tmp_path / f"{cid}.json").read_text()) + state = json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) assert state["attempts"] == 0 assert state["last_restart"] == 0 @@ -351,7 +351,7 @@ def test_a_real_run_still_persists_state(wd, tmp_path, recorder): wd.sweep_once(client, "mt5-httpapi", dry_run=False, now=1000000) assert recorder.services == [cid] - state = wd.load_state(cid) + state = wd.load_state(wd.state_key("mt5-httpapi", cid)) assert state["attempts"] == 1 assert state["last_restart"] == 1000000 @@ -537,6 +537,80 @@ def test_a_vm_without_a_service_label_is_skipped_not_guessed(wd, tmp_path, recor assert recorder.calls == [] +# ── State survives the recreate it triggered ───────────────────────────────── + +def _same_service_as(cid, service="mt5"): + """One VM container for the compose service, under whatever id Docker + assigned this incarnation.""" + return _container(cid, "mt5", labels={ + "com.docker.compose.project": "mt5-httpapi", + "com.docker.compose.service": service, + }) + + +def test_the_attempt_cap_survives_the_recreate_it_triggered(wd, tmp_path, recorder, capsys): + """Recovery REPLACES the container, so the next poll sees a new id. + + State used to be keyed by container id: the recreate orphaned the record + that had just spent an attempt, the replacement loaded a fresh one, and a + persistently-broken VM was recovered forever at "attempt 1" - the cap and + backoff reset themselves on every recovery they were meant to bound. + """ + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.MAX_ATTEMPTS = 1 + wd.BACKOFF_ATTEMPTS = [999_999] + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + old = _FakeClient([_same_service_as("old-container-id")], + health={"old-container-id": _health("unhealthy", 10)}) + assert wd.sweep_once(old, "mt5-httpapi", now=1_000_000) == 1 + + # The recreate happened: same compose service, replacement id, still + # unhealthy. The single-attempt budget is already spent. + new = _FakeClient([_same_service_as("new-container-id")], + health={"new-container-id": _health("unhealthy", 10)}) + assert wd.sweep_once(new, "mt5-httpapi", now=1_000_010) == 0 + + assert recorder.services == ["mt5"], "the replacement id refunded the attempt budget" + + +def test_backoff_survives_the_recreate_it_triggered(wd, tmp_path, recorder): + """Same replacement-id scenario, asserted on backoff rather than the cap: + inside the backoff window the replacement must wait, and once the window + passes it is recovered again - carrying the attempt count forward.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.MAX_ATTEMPTS = 99 + wd.BACKOFF_ATTEMPTS = [300] + + def incarnation(cid): + return _FakeClient([_same_service_as(cid)], health={cid: _health("unhealthy", 10)}) + + now = 1_000_000 + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(incarnation("id-1"), "mt5-httpapi", now=now) == 1 + # Replacement, inside the 300s window: waits. + assert wd.sweep_once(incarnation("id-2"), "mt5-httpapi", now=now + 200) == 0 + # Window passed: second attempt, on yet another id. + assert wd.sweep_once(incarnation("id-3"), "mt5-httpapi", now=now + 301) == 1 + + state = wd.load_state(wd.state_key("mt5-httpapi", "mt5")) + assert state["attempts"] == 2, "attempts must accumulate across container ids" + + +def test_state_key_is_stable_identity_not_container_id(wd): + assert wd.state_key("proj", "mt5") == wd.state_key("proj", "mt5") + assert wd.state_key("proj", "mt5") != wd.state_key("proj", "mt5-b") + # Labels are the only outside text that becomes a file name. With every + # separator replaced the key is a single path component, so it cannot + # traverse out of the state directory no matter what a label carries. + hostile = wd.state_key("pro/ject", "../../etc/passwd") + assert "/" not in hostile and "\\" not in hostile + + def test_recreate_refuses_without_the_project_path(wd, monkeypatch): """Compose resolves this project's relative bind mounts client-side, so without the host path the recreate would rewrite every mount. Refuse loudly From 4e7d4b6efc03b91f118ceb27a8c7a00309cdbbf8 Mon Sep 17 00:00:00 2001 From: Marinski Date: Wed, 9 Sep 2026 21:10:10 +0300 Subject: [PATCH 3/4] fix(watchdog): hand the recreate helper MT5_PROJECT_DIR; bound the busy tolerance; continuous healthy clock The container was given WATCHDOG_PROJECT_DIR but never MT5_PROJECT_DIR, while docker-compose.yml requires ${MT5_PROJECT_DIR:?} on every compose command. So recreate-vm.sh's `docker compose` failed at interpolation before it could stop anything, and no real (non-dry-run) recovery could complete. recreate_env() now builds the helper's environment explicitly - COMPOSE_PROJECT_NAME and MT5_PROJECT_DIR from the watchdog's own host path - and run.sh persists the value to .env (first line, single-quoted) so make down/logs and manual compose keep working after it exits; run.sh also refuses a stale export from another checkout, which would otherwise be persisted and then acted on. The compose files pass the variable through as well. healthcheck.sh: a port that accepts TCP but never answers HTTP is no longer healthy forever. The busy tolerance is bounded at HEALTHCHECK_SLOW_GRACE consecutive checks (default 10), after which the port is reported hung and DOWN; an answer or a refused connection resets it. An unwritable state dir degrades to the old tolerance and says so in the verdict. vm-watchdog.py: healthy_since means continuously healthy - starting, a sub-threshold unhealthy poll, or no healthcheck all restart the reset clock. Image filter is an exact repository match, and the watchdog resolves its own full container id at startup and never selects itself. Tests: the real recreate-vm.sh runs under the watchdog's exact child environment with the host variable scrubbed (plus a control proving the pre-fix environment fails at interpolation); run.sh's actual .env block is executed; a host integration test drives a real recovery through the built sidecar on a disposable Compose project. The `assert ... or True` no-op and the COMPOSE_PROJECT_NAME-only assertion are replaced. Co-Authored-By: Claude Fable 5.1 --- docker-compose.yml.example | 6 + docker-compose.yml.j2 | 6 + docs/operations.md | 60 +++- run.sh | 23 ++ scripts/healthcheck.sh | 64 +++- scripts/vm-watchdog.py | 153 ++++++-- .../integration/test_vm_watchdog_lifecycle.py | 252 +++++++++++++ tests/test_healthcheck_behavior.py | 130 ++++++- tests/test_run_env_persistence.py | 204 +++++++++++ tests/test_vm_watchdog.py | 336 +++++++++++++++++- 10 files changed, 1183 insertions(+), 51 deletions(-) create mode 100644 tests/integration/test_vm_watchdog_lifecycle.py create mode 100644 tests/test_run_env_persistence.py diff --git a/docker-compose.yml.example b/docker-compose.yml.example index d1b13863..042882e1 100644 --- a/docker-compose.yml.example +++ b/docker-compose.yml.example @@ -122,6 +122,12 @@ services: WATCHDOG_STATE_DIR: /state WATCHDOG_PROJECT_DIR: ${MT5_PROJECT_DIR} WATCHDOG_RECREATE_SCRIPT: ${MT5_PROJECT_DIR}/scripts/recreate-vm.sh + # The helper the watchdog runs calls `docker compose`, which interpolates + # ${MT5_PROJECT_DIR:?} in THIS file again. The watchdog sets it in the + # helper's environment itself; it is handed through here as well so a + # human running `docker compose exec vm-watchdog …/recreate-vm.sh` gets + # the same environment the watchdog uses. + MT5_PROJECT_DIR: ${MT5_PROJECT_DIR} logging: driver: json-file options: diff --git a/docker-compose.yml.j2 b/docker-compose.yml.j2 index 250fde4c..2dbc0140 100644 --- a/docker-compose.yml.j2 +++ b/docker-compose.yml.j2 @@ -151,6 +151,12 @@ services: WATCHDOG_STATE_DIR: /state WATCHDOG_PROJECT_DIR: ${MT5_PROJECT_DIR} WATCHDOG_RECREATE_SCRIPT: ${MT5_PROJECT_DIR}/scripts/recreate-vm.sh + # The helper the watchdog runs calls `docker compose`, which interpolates + # ${MT5_PROJECT_DIR:?} in THIS file again. The watchdog sets it in the + # helper's environment itself; it is handed through here as well so a + # human running `docker compose exec vm-watchdog …/recreate-vm.sh` gets + # the same environment the watchdog uses. + MT5_PROJECT_DIR: ${MT5_PROJECT_DIR} logging: driver: json-file options: diff --git a/docs/operations.md b/docs/operations.md index f7d7ca29..a32fbd43 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -297,9 +297,17 @@ Behavior: loop. - Stops after `WATCHDOG_MAX_ATTEMPTS` consecutive failed recoveries (default `3`) and logs loudly, instead of threshing forever. -- Resets the attempt budget only after the VM has stayed healthy for - `WATCHDOG_RESET_SECONDS` (default `1800`), so a VM that recovered then crashed - later gets a fresh budget. +- Resets the attempt budget only after the VM has stayed **continuously** + healthy for `WATCHDOG_RESET_SECONDS` (default `1800`), so a VM that recovered + then crashed later gets a fresh budget. Any non-healthy observation — a + `starting` container after a restart, or an `unhealthy` poll below the streak + threshold — restarts that clock; it does not carry over from an earlier + healthy run. +- Never selects itself, whatever `WATCHDOG_IMAGE_FILTER` is set to — it resolves + its own full container id at startup and excludes it by equality (falling + back to Docker's short-id hostname only if that inspect fails) — and matches + the image **repository exactly** (`dockurr/windows`, `dockurr/windows:5.14`, + `dockurr/windows@sha256:…` — not `dockurr/windows-something`). - `WATCHDOG_DRY_RUN=1` (or `--dry-run`) prints what it would do without touching any container. @@ -313,9 +321,49 @@ Environment overrides: `WATCHDOG_INTERVAL_SECONDS`, `WATCHDOG_MIN_FAILING_STREAK service mounts the project through at that same absolute path. Compose resolves the relative bind mounts in `docker-compose.yml` client-side, so a container-local path would rewrite every mount to something that does not exist -on the host. `run.sh` exports `MT5_PROJECT_DIR` for this; if it is missing the -watchdog reports it at startup and refuses to act, rather than falling back to a -restart that looks like recovery and is not. +on the host. If it is missing the watchdog reports it at startup and refuses to +act, rather than falling back to a restart that looks like recovery and is not. + +That path reaches compose as `MT5_PROJECT_DIR`, and compose interpolates it on +**every** command against `docker-compose.yml`, not only the first `up`: + +- `run.sh` exports it for its own run **and writes it to `.env`**, so `make + down`, `make logs` and a manual `docker compose …` keep working after `run.sh` + has exited. Starting the stack some other way? Put + `MT5_PROJECT_DIR=` in `.env` yourself. +- The watchdog sets it explicitly in the environment of the `recreate-vm.sh` it + runs (from its own `WATCHDOG_PROJECT_DIR`), because the container is not + handed the host's shell variables. `tests/test_vm_watchdog.py` runs the real + helper under exactly that environment, and + `tests/integration/test_vm_watchdog_lifecycle.py` drives a real recovery + through the built sidecar on a disposable Compose project. + +### Busy is not dead — and hung is not busy + +`healthcheck.sh` reports a port **healthy** when the TCP handshake completes but +no HTTP answer arrives inside the probe window: something is listening, the +guest is just saturated (a compile, a Strategy Tester run). Restarting a VM for +being busy would turn a slow batch into an outage. + +That tolerance is **bounded**. A port that accepts TCP but stays silent for +`HEALTHCHECK_SLOW_GRACE` consecutive checks (default `10`, ≈5 minutes at the 30s +interval) is reported as `hung` and the check fails — from there the watchdog's +own streak gate (`WATCHDOG_MIN_FAILING_STREAK`, another ≈5 minutes) applies, so +a wedged API is recovered in roughly ten minutes rather than never. The +per-port counters live in `HEALTHCHECK_STATE_DIR` (default `/tmp/healthcheck-slow` +inside the VM container); an HTTP answer or a refused connection resets a port's +count, and a recreate starts every count from zero. A refused connection +(nothing listening) is `DOWN` immediately, as before. If the counters cannot be +written (a full disk), the bound is off for that check and the verdict says so: +`ok (slow but listening: …) [slow-state unwritable: hung detection off]`. + +**Blast radius.** One hung terminal API is enough to mark the whole VM `DOWN`, +and the watchdog's recovery is the whole VM — every other terminal on it, and +whatever they were running, goes with it. That is the same rule the check has +always applied to a dead port; it is just now applied to a hung one after the +grace. When you catch a single wedged terminal before the watchdog does, +`POST /terminal/restart` on that terminal (see `docs/rest-api.md`) is the +cheaper first response. This complements the in-VM `MT5AutoReboot` scheduled task, which reboots on a fixed timer and can interrupt long-running backtests; operators who disable that diff --git a/run.sh b/run.sh index 5b7d572b..1c245973 100755 --- a/run.sh +++ b/run.sh @@ -9,6 +9,20 @@ DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # the project at the SAME absolute path the host uses. Exported (not just set) # because compose interpolates it into docker-compose.yml at up time. export MT5_PROJECT_DIR="${MT5_PROJECT_DIR:-${DIR}}" +# It must be THIS checkout. A stale export from another clone would be +# persisted to .env below and then acted on by the watchdog, which would +# recreate this project's VMs from the other clone's compose file. +if [ "$(readlink -f "${MT5_PROJECT_DIR}")" != "$(readlink -f "${DIR}")" ]; then + echo "ERROR: MT5_PROJECT_DIR is '${MT5_PROJECT_DIR}' but this checkout is '${DIR}'." + echo " Unset it (run.sh derives it) or point it at this directory." + exit 1 +fi +case "${MT5_PROJECT_DIR}" in +*"'"*) + echo "ERROR: the project path contains a single quote, which .env cannot carry: ${MT5_PROJECT_DIR}" + exit 1 + ;; +esac DEBLOAT=0 for arg in "$@"; do if [ "$arg" = "--debloat" ]; then @@ -141,6 +155,15 @@ done # Generate fresh .env each run. : >"${DIR}/.env" +# Compose interpolates ${MT5_PROJECT_DIR:?} in docker-compose.yml on EVERY +# compose command, not only the `up` below - so the value has to outlive this +# shell. The export above covers this script; this line covers `make down`, +# `make logs`, a manual `docker compose`, and the vm-watchdog's own recreate, +# all of which run after it has exited. Written first, so a later failure in +# this script cannot leave .env without it. Single-quoted: unquoted, a path +# with `$` or ` #` in it is mangled by compose's dotenv parser. +echo "MT5_PROJECT_DIR='${MT5_PROJECT_DIR}'" >>"${DIR}/.env" + API_TOKEN=$(python3 "$CFG" api_token) if [ -n "${API_TOKEN}" ]; then echo "API_TOKEN=${API_TOKEN}" >>"${DIR}/.env" diff --git a/scripts/healthcheck.sh b/scripts/healthcheck.sh index 834ad907..b0c0663b 100755 --- a/scripts/healthcheck.sh +++ b/scripts/healthcheck.sh @@ -29,6 +29,24 @@ readonly PROBE_PATH=/ping readonly PROBE_TIMEOUT_SECONDS=3 # Tried after the leased VM IP so a probe still works before the lease lands. readonly FALLBACK_HOSTS='127.0.0.1 localhost' +# A port that accepts TCP but never answers HTTP is tolerated as "busy" for +# this many CONSECUTIVE checks, then counted as down - see the busy branch. +# Anything that is not a positive integer falls back to the default rather +# than to "tolerate forever" or "tolerate nothing". Leading zeros are stripped +# first: `00` is not caught by a literal `0` pattern, and `010` would read as +# octal to `$(( ))` - neither may become a one-probe trigger. +slow_grace=${HEALTHCHECK_SLOW_GRACE:-10} +case "$slow_grace" in +'' | *[!0-9]*) slow_grace=10 ;; +*) + slow_grace=${slow_grace#"${slow_grace%%[!0]*}"} + [ -n "$slow_grace" ] && [ "$slow_grace" -ge 1 ] 2>/dev/null || slow_grace=10 + ;; +esac +readonly SLOW_GRACE_CHECKS=$slow_grace +# Per-port counters behind that tolerance. /tmp is container-local, so they +# reset when the VM is recreated - the right lifetime for "consecutive". +readonly SLOW_STATE_DIR=${HEALTHCHECK_STATE_DIR:-/tmp/healthcheck-slow} [ -f "$CONFIG" ] || { echo "no config.yaml at $CONFIG" @@ -97,8 +115,14 @@ HOSTS="" [ -n "$VM_IP" ] && HOSTS="$VM_IP" HOSTS="$HOSTS $FALLBACK_HOSTS" +mkdir -p "$SLOW_STATE_DIR" 2>/dev/null + dead="" busy="" +hung="" +# Set when a counter cannot be written: the bound is then off for that port, +# and the verdict says so rather than reading exactly like a healthy VM. +slow_note="" for p in $PORTS; do found=0 for host in $HOSTS; do @@ -121,6 +145,8 @@ for p in $PORTS; do # e.g. a 401 from the auth layer — proves the process is listening. case "$code" in [1-5][0-9][0-9]) + # An answer ends any slow streak this port had. + rm -f "$SLOW_STATE_DIR/$p" found=1 break ;; @@ -137,27 +163,55 @@ for p in $PORTS; do # Nothing listening refuses the connection instead, leaving # time_connect at 0.000 — that is the case this healthcheck exists to # catch, and it still fails. + # + # BUT A HUNG API IS NOT A BUSY ONE EITHER. A process that accepts + # connections and never serves one - wedged, deadlocked, stuck on a + # dead terminal - looks exactly like "busy" on any single probe, and + # tolerating that unconditionally kept such a VM healthy forever: the + # watchdog never saw an unhealthy streak, so it never recovered it. + # The tolerance is therefore bounded: a port that is still silent after + # SLOW_GRACE_CHECKS consecutive checks is reported as hung, and DOWN. + # A real busy spell ends and the port answers, which resets its count. case "$connect" in 0.000000 | 0.000 | 0 | "") ;; *) - busy="$busy $p" + slow_n=$(cat "$SLOW_STATE_DIR/$p" 2>/dev/null) + case "$slow_n" in + '' | *[!0-9]*) slow_n=0 ;; + esac + slow_n=$((slow_n + 1)) + echo "$slow_n" >"$SLOW_STATE_DIR/$p" 2>/dev/null || + slow_note=" [slow-state unwritable: hung detection off]" + if [ "$slow_n" -ge "$SLOW_GRACE_CHECKS" ]; then + hung="$hung $p" + else + busy="$busy $p" + fi found=1 break ;; esac done - [ "$found" -eq 0 ] && dead="$dead $p" + # Refused everywhere: nothing is listening. That also ends any slow + # streak - whatever was hung on this port is gone now. + [ "$found" -eq 0 ] && { + dead="$dead $p" + rm -f "$SLOW_STATE_DIR/$p" + } done -if [ -n "$dead" ]; then - echo "DOWN ports:$dead (vm_ip=$VM_IP)" +if [ -n "$dead" ] || [ -n "$hung" ]; then + verdict="DOWN" + [ -n "$dead" ] && verdict="$verdict ports:$dead" + [ -n "$hung" ] && verdict="$verdict hung (listening, no HTTP for $SLOW_GRACE_CHECKS+ checks):$hung" + echo "$verdict (vm_ip=$VM_IP)" exit 1 fi # Healthy, but say so out loud: a port that only ever answers this way is worth # looking at even though it is not a restart-worthy fault. if [ -n "$busy" ]; then - echo "ok (slow but listening:$busy) all ports up:" $PORTS "(vm_ip=$VM_IP)" + echo "ok (slow but listening:$busy)$slow_note all ports up:" $PORTS "(vm_ip=$VM_IP)" exit 0 fi diff --git a/scripts/vm-watchdog.py b/scripts/vm-watchdog.py index a87a768b..811900b9 100755 --- a/scripts/vm-watchdog.py +++ b/scripts/vm-watchdog.py @@ -57,9 +57,16 @@ absolute path the host uses: Compose resolves relative bind mounts client-side, so ``./scripts/x`` in the compose file has to land on the host's ``/scripts/x``, not on some path inside this container. ``run.sh`` -exports ``MT5_PROJECT_DIR`` for that. Without it the watchdog refuses to act -and says so, rather than falling back to a restart that looks like recovery -and is not. +exports ``MT5_PROJECT_DIR`` for that and persists it to ``.env``. Without it +the watchdog refuses to act and says so, rather than falling back to a +restart that looks like recovery and is not. + +The helper runs ``docker compose`` itself, and compose interpolates +``${MT5_PROJECT_DIR:?}`` in the compose file on EVERY command — so the +watchdog hands that variable to the helper explicitly (``recreate_env``). +Compose resolved it on the host when it created this container but never +passed it in, and without that line every real recovery failed at +interpolation before it could stop anything. Runs forever, restarting itself if it crashes: keepalive is the Compose ``restart: unless-stopped`` on this service, not a supervisor inside the loop. @@ -71,6 +78,7 @@ import http.client import json import os +import re import socket import subprocess import sys @@ -173,10 +181,10 @@ def _env_str(name: str, default: str, *, allow_empty: bool) -> str: # Restart only after this many consecutive failed healthchecks. MIN_FAILING_STREAK = _env_int("WATCHDOG_MIN_FAILING_STREAK", "10", minimum=1) -# Only containers whose image starts with this are considered VM containers. -# Empty is refused rather than treated as "no filter": "".startswith() matches -# every image, so a blank value would make every container in the project a -# restart candidate - including this watchdog and any database sharing it. +# Only containers running exactly this image REPOSITORY (any tag or digest) +# are considered VM containers - see _image_matches. Empty is refused rather +# than treated as "no filter": a blank value would make every container in the +# project a restart candidate - including any database sharing it. IMAGE_FILTER = _env_str("WATCHDOG_IMAGE_FILTER", "dockurr/windows", allow_empty=False) # Exponential backoff per attempt: first restart after 5m, then 15m, then 1h. @@ -194,8 +202,13 @@ def _env_str(name: str, default: str, *, allow_empty: bool) -> str: # labels (the watchdog runs as a service in the same project). COMPOSE_PROJECT = os.environ.get("WATCHDOG_COMPOSE_PROJECT", "") -# This container's own ID; Docker sets the container hostname to it. +# This container's own ID; Docker sets the container hostname to its SHORT id. SELF_ID = os.environ.get("WATCHDOG_SELF_ID", socket.gethostname()) +# Resolved to the FULL id at startup (main -> _inspect_self) so self-exclusion +# works by equality even when the hostname is not the container id - a +# `hostname:` on the service, or WATCHDOG_SELF_ID set to a name. +SELF_FULL_ID = "" +_HEX_ID = re.compile(r"[0-9a-f]{12,64}") def log(message: str, *args) -> None: @@ -257,6 +270,33 @@ def inspect(self, container_id: str) -> dict: +def recreate_env(project: str) -> dict[str, str]: + """The exact environment the recreate helper runs with. + + Two values are set explicitly, because the helper runs ``docker compose`` + and compose needs both while this container was handed neither: + + - ``COMPOSE_PROJECT_NAME``: compose otherwise derives the project from the + directory name, and a mismatch does not fail - it quietly creates a + SECOND set of containers alongside the running ones. + - ``MT5_PROJECT_DIR``: the compose file interpolates ``${MT5_PROJECT_DIR:?}`` + on EVERY compose command, not only the ``up`` that started the stack. + Compose resolved it on the host when it created this container, but the + container's own environment carries ``WATCHDOG_PROJECT_DIR`` instead, so + the helper's compose call failed at interpolation - "required variable + MT5_PROJECT_DIR is missing a value" - before it could stop or recreate + anything. Every real (non-dry-run) recovery was dead on arrival. + ``WATCHDOG_PROJECT_DIR`` IS that host path, so it is the source here. + + Factored out of recreate_vm() so a test can run the real helper under + precisely this environment, with nothing inherited from the host shell. + """ + env = dict(os.environ) + env["COMPOSE_PROJECT_NAME"] = project + env["MT5_PROJECT_DIR"] = PROJECT_DIR + return env + + def recreate_vm(service: str, project: str) -> None: """Recreate one VM together with every sidecar sharing its netns. @@ -264,23 +304,20 @@ def recreate_vm(service: str, project: str) -> None: script is what operators run, what the lifecycle regression test exercises, and the only place that knows how to find a VM's sidecars in the generated compose file. Reimplementing the discovery here would give the fleet two - recovery paths that could drift apart. - - COMPOSE_PROJECT_NAME is passed explicitly. Compose otherwise derives the - project from the directory name, and a mismatch there would not fail — it - would quietly create a SECOND set of containers alongside the running ones. + recovery paths that could drift apart. See recreate_env() for the two + variables the helper is handed explicitly. """ if not PROJECT_DIR: raise RuntimeError( "WATCHDOG_PROJECT_DIR is unset, so the compose project cannot be " "recreated. Start the stack through run.sh (it exports " - "MT5_PROJECT_DIR), or set it to the host path of the project." + "MT5_PROJECT_DIR and writes it to .env), or set it to the host " + "path of the project." ) - env = dict(os.environ, COMPOSE_PROJECT_NAME=project) result = subprocess.run( [RECREATE_SCRIPT, service], cwd=PROJECT_DIR, - env=env, + env=recreate_env(project), capture_output=True, text=True, timeout=RECREATE_TIMEOUT_SECONDS, @@ -333,18 +370,26 @@ def decide(state: dict, health_status: str, failing_streak: int, now: int) -> tu Mutates ``state`` (attempts / timestamps) and returns ``(action, reason)`` where action is one of ``wait``, ``restart`` or ``give_up``. """ + # healthy_since means CONTINUOUSLY healthy since. Any other observation - + # starting, unhealthy, none - breaks that continuity and the clock starts + # over on the next healthy poll. It used to survive a `starting` (and an + # unhealthy poll below the streak threshold), so a VM that was healthy at + # t0, went through a restart, and came back healthy at t0+RESET had its + # attempt budget refunded on that first healthy poll rather than after + # RESET_SECONDS of proven stability. + if health_status != "healthy": + state["healthy_since"] = 0 + if health_status != "unhealthy": - # Healthy or starting: never touch. Track when we last saw it healthy - # so a recovery that holds long enough resets the attempt budget. + # Healthy or starting: never touch. Track when the current healthy run + # began so a recovery that holds long enough resets the attempt budget. if health_status == "healthy": - state["healthy_since"] = state.get("healthy_since") or now + if not state.get("healthy_since"): + state["healthy_since"] = now if now - state["healthy_since"] >= RESET_SECONDS: state["attempts"] = 0 state["last_restart"] = 0 return "wait", "healthy long enough - attempt budget reset" - else: - # starting / none: nothing to do, do not start a healthy clock. - state["healthy_since"] = state.get("healthy_since") or 0 return "wait", f"health={health_status}" # Unhealthy from here on. @@ -372,23 +417,61 @@ def decide(state: dict, health_status: str, failing_streak: int, now: int) -> tu # ── Docker-facing logic ────────────────────────────────────────────────────── -def _compose_project(client: DockerClient) -> str | None: - """This project's name, from our own container's compose labels.""" +def _inspect_self(client: DockerClient) -> dict: + """This container's own inspect record, or {} when it cannot be read.""" try: - info = client.inspect(SELF_ID) + return client.inspect(SELF_ID) except (RuntimeError, OSError): - return None + return {} + + +def _compose_project(client: DockerClient) -> str | None: + """This project's name, from our own container's compose labels.""" + info = _inspect_self(client) return (info.get("Config", {}).get("Labels", {}) or {}).get("com.docker.compose.project") +def _image_matches(image: str, repo: str) -> bool: + """Exact image REPOSITORY match, any tag or digest. + + ``dockurr/windows`` matches ``dockurr/windows``, ``dockurr/windows:5.14`` + and ``dockurr/windows@sha256:...`` - and not ``dockurr/windows-not-the-vm``. + A prefix test did match that last one, which for a container holding the + Docker socket meant its restart scope was "anything whose image name + happens to start the same way". + """ + return image == repo or image.startswith(repo + ":") or image.startswith(repo + "@") + + +def _is_self(container_id: str) -> bool: + """Whether this id is the watchdog's own container. + + By equality against the full id resolved at startup. Before that (or if + the self-inspect failed) fall back to the short-id prefix Docker gives the + hostname - but only when SELF_ID actually looks like one, so a service + `hostname: watchdog` never prefix-matches unrelated containers, and a real + name never quietly matches nothing while the docs promise otherwise. + """ + if SELF_FULL_ID: + return container_id == SELF_FULL_ID + if SELF_ID and _HEX_ID.fullmatch(SELF_ID): + return container_id == SELF_ID or container_id.startswith(SELF_ID) + return False + + def _scoped_containers(client: DockerClient, project: str) -> list[dict]: - """Running containers in this project running the VM image.""" + """Running containers in this project running the VM image - never itself.""" out = [] for c in client.list_containers(): + # Unconditional and first: a valid operator override such as + # WATCHDOG_IMAGE_FILTER=python would otherwise select the watchdog, and + # a watchdog that recreates itself mid-sweep is not a recovery path. + if _is_self(c.get("Id") or ""): + continue labels = c.get("Labels", {}) or {} if project and labels.get("com.docker.compose.project") != project: continue - if not (c.get("Image") or "").startswith(IMAGE_FILTER): + if not _image_matches(c.get("Image") or "", IMAGE_FILTER): continue out.append(c) return out @@ -468,8 +551,8 @@ def validate_config() -> list[str]: if not PROJECT_DIR: errors.append( "WATCHDOG_PROJECT_DIR is empty - recovery needs the compose project's " - "HOST path (run.sh exports MT5_PROJECT_DIR). Without it a crashed VM " - "cannot be recreated." + "HOST path (run.sh exports MT5_PROJECT_DIR and writes it to .env). " + "Without it a crashed VM cannot be recreated." ) elif not os.path.isdir(PROJECT_DIR): errors.append( @@ -501,6 +584,16 @@ def main() -> int: log("dry-run mode: will report decisions without restarting") client = DockerClient(DOCKER_SOCKET) + # Resolve our own FULL container id once, so self-exclusion is an equality + # test rather than a guess from the hostname. + global SELF_FULL_ID + self_info = _inspect_self(client) + SELF_FULL_ID = self_info.get("Id") or "" + if SELF_FULL_ID: + log(f"own container id resolved: {SELF_FULL_ID[:12]}") + else: + log(f"cannot inspect own container '{SELF_ID}'; self-exclusion falls back to the short-id prefix") + project = COMPOSE_PROJECT or _compose_project(client) or "" if not project: log("cannot determine compose project (no WATCHDOG_COMPOSE_PROJECT and " diff --git a/tests/integration/test_vm_watchdog_lifecycle.py b/tests/integration/test_vm_watchdog_lifecycle.py new file mode 100644 index 00000000..40fa88ea --- /dev/null +++ b/tests/integration/test_vm_watchdog_lifecycle.py @@ -0,0 +1,252 @@ +"""Integration test: a REAL watchdog recovery, end to end, on a disposable +Compose project. + +psyb0t's blocker on PR #15 (2026-09-04): the watchdog container is handed +WATCHDOG_PROJECT_DIR but never MT5_PROJECT_DIR, while docker-compose.yml +requires `${MT5_PROJECT_DIR:?}`. So recreate-vm.sh's `docker compose` failed at +interpolation and no real (non-dry-run) recovery could ever complete. The unit +suite could not see it because nothing there ran the real child environment +against real compose. This does, with nothing faked in the chain: + + the sidecar image built from Dockerfile.watchdog, the real vm-watchdog.py, + the real recreate-vm.sh, the real `docker compose`, the real Docker daemon. + +It stands up a fake "VM" whose healthcheck fails once /tmp/unhealthy exists, a +`network_mode: service:vm` sidecar, and the watchdog with the Docker socket +mounted. MT5_PROJECT_DIR is supplied ONLY to this test's own `up` - as an +operator's shell would - and deliberately NOT passed into the watchdog +container, reproducing the production environment psyb0t described. Then it: + + 1. proves the blocker is real INSIDE that container: `docker compose config` + there fails with compose's own "required variable MT5_PROJECT_DIR" error; + 2. makes the VM unhealthy and waits for the watchdog to recover it; + 3. asserts a NEW VM container exists, the sidecar's NetworkMode names it, the + sidecar has an eth0 again, and the watchdog recorded exactly one attempt. + +Runs on the host (`make test-integration`), like test_wickworks_lifecycle.py. +""" + +import os +import shutil +import subprocess +import time +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.integration + +REPO_ROOT = Path(__file__).resolve().parents[2] +PROJECT = "vm-watchdog-lifecycle-test" +VM_IMAGE = "python:3.12-alpine" +SIDECAR_IMAGE = "alpine:3.20" + +RECOVERY_TIMEOUT_SECONDS = 150 +STARTUP_TIMEOUT_SECONDS = 90 + + +def _require_compose(): + if shutil.which("docker") is None: + pytest.skip("docker not available") + if subprocess.run(["docker", "compose", "version"], capture_output=True).returncode != 0: + pytest.skip("docker compose not available") + + +def _write_project(project_dir: Path) -> None: + """A compose project shaped like production where it matters. + + The watchdog service copies the shipped one: socket mount, script mount, + the project mounted at the SAME absolute path through the `${MT5_PROJECT_DIR:?}` + interpolation, WATCHDOG_* set, and MT5_PROJECT_DIR conspicuously NOT in its + environment. Timings are shortened so the test finishes in seconds, and + the image filter names the fake VM's image. + """ + scripts = project_dir / "scripts" + scripts.mkdir() + helper = scripts / "recreate-vm.sh" + shutil.copy(REPO_ROOT / "scripts" / "recreate-vm.sh", helper) + helper.chmod(0o755) + + (project_dir / "docker-compose.yml").write_text( + f"""services: + vm: + image: {VM_IMAGE} + command: ["python3", "-c", "import time\\nwhile True: time.sleep(1)"] + healthcheck: + test: ["CMD", "sh", "-c", "test ! -f /tmp/unhealthy"] + interval: 1s + timeout: 1s + retries: 1 + start_period: 0s + stop_grace_period: 3s + + sidecar: + image: {SIDECAR_IMAGE} + command: ["sleep", "infinity"] + network_mode: "service:vm" + depends_on: + - vm + + vm-watchdog: + build: + context: {REPO_ROOT} + dockerfile: Dockerfile.watchdog + command: ["python", "-u", "/vm-watchdog.py"] + volumes: + - /var/run/docker.sock:/var/run/docker.sock + - {REPO_ROOT}/scripts/vm-watchdog.py:/vm-watchdog.py:ro + - ${{MT5_PROJECT_DIR:?MT5_PROJECT_DIR must be the absolute host path of this project}}:${{MT5_PROJECT_DIR}}:ro + tmpfs: + - /state + environment: + WATCHDOG_STATE_DIR: /state + WATCHDOG_PROJECT_DIR: {project_dir} + WATCHDOG_RECREATE_SCRIPT: {project_dir}/scripts/recreate-vm.sh + WATCHDOG_COMPOSE_PROJECT: {PROJECT} + WATCHDOG_IMAGE_FILTER: python + WATCHDOG_MIN_FAILING_STREAK: "3" + WATCHDOG_INTERVAL_SECONDS: "1" + WATCHDOG_BACKOFF_ATTEMPTS: "5" + WATCHDOG_MAX_ATTEMPTS: "3" + WATCHDOG_RESET_SECONDS: "60" + RECREATE_STOP_TIMEOUT: "3" + # Deliberately absent: MT5_PROJECT_DIR. The watchdog must hand it to the + # helper itself; the container is not given it. +""", + encoding="utf-8", + ) + + +def _compose(project_dir: Path, *args, env_extra=None, check=True): + cmd = ["docker", "compose", "-p", PROJECT, "-f", str(project_dir / "docker-compose.yml"), *args] + # A clean environment: only the test's own `up` gets MT5_PROJECT_DIR, and + # nothing here inherits one a developer happens to have exported. + env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "HOME": os.environ.get("HOME", "/tmp")} + if "DOCKER_HOST" in os.environ: + env["DOCKER_HOST"] = os.environ["DOCKER_HOST"] + env.update(env_extra or {}) + result = subprocess.run(cmd, capture_output=True, text=True, env=env, timeout=600) + if check and result.returncode != 0: + raise AssertionError( + f"{' '.join(cmd)} failed (rc={result.returncode})\nstdout: {result.stdout}\nstderr: {result.stderr}" + ) + return result + + +def _name(service: str) -> str: + return f"{PROJECT}-{service}-1" + + +def _inspect(service: str, tmpl: str) -> str: + result = subprocess.run(["docker", "inspect", "--format", tmpl, _name(service)], capture_output=True, text=True) + if result.returncode != 0: + raise AssertionError(f"docker inspect {_name(service)} failed: {result.stderr}") + return result.stdout.strip() + + +def _exec(service: str, *cmd: str): + return subprocess.run(["docker", "exec", _name(service), *cmd], capture_output=True, text=True) + + +def _watchdog_logs() -> str: + return subprocess.run(["docker", "logs", _name("vm-watchdog")], capture_output=True, text=True).stdout + + +def _wait(predicate, timeout, what): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return + time.sleep(1) + raise TimeoutError(f"timed out after {timeout}s waiting for {what}\n--- watchdog log ---\n{_watchdog_logs()}") + + +@pytest.fixture(scope="module") +def stack(tmp_path_factory): + _require_compose() + project_dir = tmp_path_factory.mktemp("vm-watchdog-lifecycle") + _write_project(project_dir) + # The operator's shell: MT5_PROJECT_DIR exported for THIS command only. + _compose(project_dir, "up", "-d", "--build", env_extra={"MT5_PROJECT_DIR": str(project_dir)}) + try: + _wait(lambda: _inspect("vm", "{{.State.Health.Status}}") == "healthy", STARTUP_TIMEOUT_SECONDS, "vm healthy") + _wait(lambda: "scoped to compose project" in _watchdog_logs(), STARTUP_TIMEOUT_SECONDS, "watchdog started") + yield project_dir + finally: + # --rmi local: the per-project build tag would otherwise accumulate one + # dangling watchdog image per run. + _compose(project_dir, "down", "-v", "--remove-orphans", "--rmi", "local", check=False) + + +def test_the_watchdog_starts_clean_against_the_real_wiring(stack): + """validate_config() passed: the project is mounted at the host path, the + helper is where WATCHDOG_RECREATE_SCRIPT says, no problems were reported.""" + log = _watchdog_logs() + assert "invalid configuration" not in log + assert f"scoped to compose project '{PROJECT}'" in log + + +def test_the_blocker_is_real_inside_the_sidecar(stack): + """Reproduce psyb0t's finding where it bites: compose run INSIDE the + watchdog container, with the container's own environment, fails to even + parse the project file. This is what every pre-fix recovery hit.""" + res = _compose(stack, "exec", "-T", "vm-watchdog", "sh", "-c", + f"docker compose -f {stack}/docker-compose.yml config --quiet", check=False) + assert res.returncode != 0, "compose succeeded without MT5_PROJECT_DIR - the reproduction no longer holds" + assert "MT5_PROJECT_DIR" in (res.stderr + res.stdout) + + +def test_the_watchdog_gives_the_helper_what_the_container_never_got(stack): + """The fix, observed at the boundary: recreate_env() built inside the real + sidecar carries the project dir even though the container's env does not.""" + res = _compose(stack, "exec", "-T", "vm-watchdog", "python", "-c", + "import importlib.util, os\n" + "spec = importlib.util.spec_from_file_location('wd', '/vm-watchdog.py')\n" + "wd = importlib.util.module_from_spec(spec); spec.loader.exec_module(wd)\n" + "print('container-env:', os.environ.get('MT5_PROJECT_DIR', ''))\n" + f"print('helper-env:', wd.recreate_env('{PROJECT}')['MT5_PROJECT_DIR'])\n") + assert "container-env: " in res.stdout, res.stdout + assert f"helper-env: {stack}" in res.stdout, res.stdout + + +def test_a_persistently_unhealthy_vm_is_recreated_with_its_sidecar(stack): + """The recovery, for real: make the VM unhealthy, wait for the watchdog, + and check the world afterwards rather than the log alone.""" + vm_before = _inspect("vm", "{{.Id}}") + assert _inspect("sidecar", "{{.HostConfig.NetworkMode}}") == f"container:{vm_before}" + + assert _exec("vm", "touch", "/tmp/unhealthy").returncode == 0 + + def unhealthy_or_already_recreated(): + # The watchdog may win the race and replace the container between two + # polls; inspect then fails on the vanished name. That is progress, not + # an error - the recreate assertion below is what checks the outcome. + try: + return _inspect("vm", "{{.State.Health.Status}}") == "unhealthy" + except AssertionError: + return True + + _wait(unhealthy_or_already_recreated, 30, "vm to go unhealthy") + + _wait( + lambda: "recreated vm and its sidecars" in _watchdog_logs(), + RECOVERY_TIMEOUT_SECONDS, + "the watchdog to recreate the vm", + ) + log = _watchdog_logs() + assert "recreate failed" not in log, log + assert "MT5_PROJECT_DIR is missing" not in log, log + + vm_after = _inspect("vm", "{{.Id}}") + assert vm_after != vm_before, "recovery must be a recreate, not a restart" + # Fresh container filesystem: the poison file is gone, the VM comes back healthy. + _wait(lambda: _inspect("vm", "{{.State.Health.Status}}") == "healthy", STARTUP_TIMEOUT_SECONDS, "recreated vm healthy") + + # The sidecar was recreated WITH it and rejoined the new netns. + assert _inspect("sidecar", "{{.HostConfig.NetworkMode}}") == f"container:{vm_after}" + interfaces = _exec("sidecar", "ls", "/sys/class/net").stdout.split() + assert "eth0" in interfaces, f"sidecar has no eth0 after recovery: {interfaces}" + + # One attempt, recorded under the stable compose identity (not the old id). + state = _compose(stack, "exec", "-T", "vm-watchdog", "cat", f"/state/{PROJECT}.vm.json").stdout + assert '"attempts": 1' in state, state diff --git a/tests/test_healthcheck_behavior.py b/tests/test_healthcheck_behavior.py index 6642ab74..ea478116 100644 --- a/tests/test_healthcheck_behavior.py +++ b/tests/test_healthcheck_behavior.py @@ -196,12 +196,16 @@ def test_group_file_ignores_comments_and_blank_lines(awk_prog, tmp_path): # These run the whole script with a stub `curl` on PATH, so they exercise the # real verdict logic rather than the awk filter alone. -def _run_healthcheck(tmp_path, config, curl_body): +def _run_healthcheck(tmp_path, config, curl_body, grace=None): """Run healthcheck.sh with a fake curl that emits `curl_body` on stdout. The stub mimics curl closely enough for the script: it writes what a real `-w '%{http_code} %{time_connect}'` would print, and exits non-zero when the status is 000, exactly as curl does on a failed request. + + The slow-port counters go under tmp_path, so consecutive calls within one + test see each other's state (as consecutive healthchecks in one container + do) while tests never see each other's. """ bindir = tmp_path / "bin" bindir.mkdir(exist_ok=True) @@ -220,7 +224,10 @@ def _run_healthcheck(tmp_path, config, curl_body): "HEALTHCHECK_CONFIG": str(config), "HEALTHCHECK_VM_GROUP": str(tmp_path / "no-such-group.txt"), "HEALTHCHECK_LEASES": str(tmp_path / "no-such-leases"), + "HEALTHCHECK_STATE_DIR": str(tmp_path / "slow-state"), } + if grace is not None: + env["HEALTHCHECK_SLOW_GRACE"] = str(grace) return subprocess.run( ["sh", str(HEALTHCHECK_PATH)], capture_output=True, text=True, env=env, timeout=60, @@ -264,3 +271,124 @@ def test_a_missing_curl_fails_closed(tmp_path): config = _write_config(tmp_path, ONE_TERMINAL) result = _run_healthcheck(tmp_path, config, "") assert result.returncode == 1, result.stdout + result.stderr + + +# ── Hung is not busy: the slow tolerance is bounded ────────────────────────── +# +# psyb0t (2026-09-04): "curl result 000 with a completed TCP connection" was +# healthy unconditionally, so an API that accepts TCP but never answers HTTP +# stayed healthy forever and the watchdog never recovered it. A busy spell ends +# and the port answers; a hung port does not - so tolerate the former for a +# bounded number of CONSECUTIVE checks and then call the latter what it is. + +SLOW = "000 0.001204" # handshake completed, no HTTP inside the window +ANSWERS = "200 0.000181" +REFUSED = "000 0.000000" + + +def _slow_runs(tmp_path, config, count, grace=None): + return [_run_healthcheck(tmp_path, config, SLOW, grace=grace) for _ in range(count)] + + +def test_a_slow_port_is_tolerated_below_the_grace(tmp_path): + config = _write_config(tmp_path, ONE_TERMINAL) + for result in _slow_runs(tmp_path, config, 2, grace=3): + assert result.returncode == 0, result.stdout + result.stderr + assert "slow but listening" in result.stdout + + +def test_a_port_still_silent_at_the_grace_is_hung_and_down(tmp_path): + """The regression. Third consecutive silent check with grace=3 -> DOWN, + and the verdict says hung, not merely down, so an operator can tell it + from a refused connection.""" + config = _write_config(tmp_path, ONE_TERMINAL) + first, second, third = _slow_runs(tmp_path, config, 3, grace=3) + assert first.returncode == 0 and second.returncode == 0 + assert third.returncode == 1, third.stdout + third.stderr + assert "DOWN" in third.stdout and "hung" in third.stdout and "6001" in third.stdout + # And it stays down while it stays silent. + assert _run_healthcheck(tmp_path, config, SLOW, grace=3).returncode == 1 + + +def test_an_answer_resets_the_slow_count(tmp_path): + """slow, slow, ANSWER, slow, slow with grace=3 is healthy throughout: the + answer proves the process serves, so the count starts over.""" + config = _write_config(tmp_path, ONE_TERMINAL) + sequence = [SLOW, SLOW, ANSWERS, SLOW, SLOW] + for body in sequence: + result = _run_healthcheck(tmp_path, config, body, grace=3) + assert result.returncode == 0, (body, result.stdout + result.stderr) + # ...and the third silent check after the answer is the one that trips. + assert _run_healthcheck(tmp_path, config, SLOW, grace=3).returncode == 1 + + +def test_a_refused_connection_resets_the_slow_count(tmp_path): + """Refused is DOWN on its own terms (nothing listening), and it means the + hung process is gone - the next listener starts with a clean count.""" + config = _write_config(tmp_path, ONE_TERMINAL) + _slow_runs(tmp_path, config, 2, grace=3) + refused = _run_healthcheck(tmp_path, config, REFUSED, grace=3) + assert refused.returncode == 1 and "hung" not in refused.stdout + first, second = _slow_runs(tmp_path, config, 2, grace=3) + assert first.returncode == 0 and second.returncode == 0 + + +def test_the_default_grace_is_ten_checks(tmp_path): + """Ten consecutive silent checks is ~5 minutes at the 30s interval - longer + than any compile burst seen on the farm, far shorter than forever.""" + config = _write_config(tmp_path, ONE_TERMINAL) + results = _slow_runs(tmp_path, config, 10) + assert all(r.returncode == 0 for r in results[:9]) + assert results[9].returncode == 1 and "hung" in results[9].stdout + + +@pytest.mark.parametrize("bad", ["0", "00", "010", "banana", "", "-3", "2.5", " 5"]) +def test_a_bad_grace_falls_back_to_the_default_not_to_forever(tmp_path, bad): + """A typo must not silently restore the unbounded tolerance (or zero it). + + `00` slipped past a literal-`0` pattern and made the FIRST silent probe + hung (`[ 1 -ge 00 ]` is true); `010` would read as octal. Both now strip + to their decimal value first - `010` is simply 10, the default.""" + config = _write_config(tmp_path, ONE_TERMINAL) + results = _slow_runs(tmp_path, config, 10, grace=bad) + assert all(r.returncode == 0 for r in results[:9]), bad + assert results[9].returncode == 1, bad + + +def test_an_unwritable_state_dir_degrades_to_tolerance_not_to_restarts(tmp_path): + """If the counters cannot be kept, the script cannot know a port is hung - + so it falls back to the busy verdict rather than inventing a streak. The + wrong failure mode here would be restarting busy VMs whenever /tmp fills + up. Documented degradation, pinned so nobody 'fixes' it into fail-closed.""" + config = _write_config(tmp_path, ONE_TERMINAL) + bindir = tmp_path / "bin" + bindir.mkdir(exist_ok=True) + (bindir / "curl").write_text( + "#!/bin/sh\nprintf '%s' '000 0.001204'\nexit 7\n", encoding="utf-8" + ) + (bindir / "curl").chmod(0o755) + env = { + "PATH": f"{bindir}:/usr/bin:/bin", + "HEALTHCHECK_CONFIG": str(config), + "HEALTHCHECK_VM_GROUP": str(tmp_path / "no-such-group.txt"), + "HEALTHCHECK_LEASES": str(tmp_path / "no-such-leases"), + # A file where the directory should be: mkdir -p and every write fail. + "HEALTHCHECK_STATE_DIR": str(tmp_path / "not-a-dir"), + "HEALTHCHECK_SLOW_GRACE": "2", + } + (tmp_path / "not-a-dir").write_text("", encoding="utf-8") + for _ in range(4): + result = subprocess.run(["sh", str(HEALTHCHECK_PATH)], capture_output=True, text=True, env=env, timeout=60) + assert result.returncode == 0, result.stdout + result.stderr + assert "slow but listening" in result.stdout + # ...but never silently: the verdict says the bound is off. + assert "slow-state unwritable" in result.stdout + + +def test_a_leading_zero_grace_does_not_trip_on_the_first_probe(tmp_path): + """The exact hole the counter-review found: with grace `00` the first + silent probe used to be reported hung.""" + config = _write_config(tmp_path, ONE_TERMINAL) + first = _run_healthcheck(tmp_path, config, SLOW, grace="00") + assert first.returncode == 0, first.stdout + first.stderr + assert "hung" not in first.stdout diff --git a/tests/test_run_env_persistence.py b/tests/test_run_env_persistence.py new file mode 100644 index 00000000..e3e946b0 --- /dev/null +++ b/tests/test_run_env_persistence.py @@ -0,0 +1,204 @@ +"""run.sh must persist MT5_PROJECT_DIR to .env, not only export it. + +psyb0t (2026-09-04): run.sh exported MT5_PROJECT_DIR to its own shell and +truncated/rebuilt .env without writing it, so `make down`, `make logs`, a +manual `docker compose` and - through recreate-vm.sh - the watchdog's own +recovery all failed at `${MT5_PROJECT_DIR:?}` interpolation once run.sh had +exited. + +These run the ACTUAL .env-generation block lifted out of run.sh (bounded by +two comment lines that already exist there) under bash with a stub +config_helper, and assert on the .env it writes. A source-substring assertion +would pass with the line in the wrong place or behind a failing command; this +does not. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parents[1] +RUN_SH = _REPO / "run.sh" + +# The block starts at the truncation and ends where nginx generation begins. +_START = "# Generate fresh .env each run." +_END = "# Generate nginx.conf from config.yaml terminals." + + +def _env_block(): + src = RUN_SH.read_text(encoding="utf-8") + _before, sep, rest = src.partition(_START) + assert sep, "start marker missing from run.sh - the .env block moved" + block, sep2, _after = rest.partition(_END) + assert sep2, "end marker missing from run.sh - the .env block moved" + return block + + +def _run_env_block(tmp_path, *, token="tok-123", ts_key="", ts_server="", fail_on=None): + """Execute the block with DIR=tmp_path and a config_helper stub on PATH. + + `fail_on` names a config_helper subcommand the stub should fail on, to show + what .env looks like when the script dies partway through. + """ + bindir = tmp_path / "bin" + bindir.mkdir(exist_ok=True) + stub = bindir / "python3" + stub.write_text( + "#!/bin/sh\n" + f"[ \"$2\" = \"{fail_on or ''}\" ] && [ -n \"{fail_on or ''}\" ] && exit 1\n" + 'case "$2" in\n' + f" api_token) printf '%s' '{token}' ;;\n" + f" ts_auth_key) printf '%s' '{ts_key}' ;;\n" + f" ts_login_server) printf '%s' '{ts_server}' ;;\n" + ' *) echo "stub config_helper: unexpected $*" >&2; exit 1 ;;\n' + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + + script = ( + "set -eo pipefail\n" + f'DIR="{tmp_path}"\n' + 'CFG="${DIR}/scripts/config_helper.py"\n' + + _env_block() + ) + env = { + "PATH": f"{bindir}{os.pathsep}{os.environ.get('PATH', '/usr/bin:/bin')}", + "MT5_PROJECT_DIR": str(tmp_path), + } + return subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, env=env, timeout=60, check=False, + ) + + +def _env_lines(tmp_path): + return (tmp_path / ".env").read_text(encoding="utf-8").splitlines() + + +def _project_dir_line(path): + # Single-quoted on purpose: unquoted, `$` or ` #` in a path is mangled by + # compose's dotenv parser. + return f"MT5_PROJECT_DIR='{path}'" + + +def test_env_carries_the_project_dir_first(tmp_path): + res = _run_env_block(tmp_path) + assert res.returncode == 0, res.stderr + lines = _env_lines(tmp_path) + assert lines[0] == _project_dir_line(tmp_path) + assert "API_TOKEN=tok-123" in lines + + +def test_env_is_regenerated_not_appended_to(tmp_path): + """The truncation is load-bearing: a stale value must not survive a re-run.""" + (tmp_path / ".env").write_text("MT5_PROJECT_DIR=/old/path\nSTALE=1\n", encoding="utf-8") + res = _run_env_block(tmp_path) + assert res.returncode == 0, res.stderr + lines = _env_lines(tmp_path) + assert lines.count(_project_dir_line(tmp_path)) == 1 + assert "MT5_PROJECT_DIR=/old/path" not in lines + assert "STALE=1" not in lines + + +def test_the_project_dir_survives_a_failure_later_in_the_block(tmp_path): + """Written before anything that can fail. If reading the API token blows + up, .env must still make compose usable rather than be left half-built + without the one variable every compose command needs.""" + res = _run_env_block(tmp_path, fail_on="api_token") + assert res.returncode != 0 + assert _env_lines(tmp_path) == [_project_dir_line(tmp_path)] + + +def test_optional_tailscale_values_are_still_written_when_present(tmp_path): + """The block's existing behaviour is unchanged around the new line.""" + res = _run_env_block(tmp_path, ts_key="tskey-abc", ts_server="https://hs.example") + assert res.returncode == 0, res.stderr + lines = _env_lines(tmp_path) + assert lines[0] == _project_dir_line(tmp_path) + assert "TS_AUTHKEY=tskey-abc" in lines + assert any(line.startswith("TS_EXTRA_ARGS=") and "hs.example" in line for line in lines) + + +# ── run.sh refuses a MT5_PROJECT_DIR that is not this checkout ─────────────── +# +# Counter-review finding: `${MT5_PROJECT_DIR:-$DIR}` accepted any pre-exported +# value. With two clones and a stale export from the other one, .env would +# record the wrong path and the watchdog would recreate THIS project's VMs from +# the OTHER clone's compose file. These run a copy of the real run.sh from a +# temp dir; the check sits before anything the script creates or downloads. + + +def _run_copy_of_run_sh(tmp_path, project_dir_value): + import shutil + + script = tmp_path / "run.sh" + shutil.copy(RUN_SH, script) + script.chmod(0o755) + env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "SKIP_KVM_CHECK": "1"} + if project_dir_value is not None: + env["MT5_PROJECT_DIR"] = project_dir_value + return subprocess.run([str(script)], capture_output=True, text=True, env=env, timeout=60, check=False) + + +def test_run_sh_refuses_a_project_dir_that_is_another_checkout(tmp_path): + other = tmp_path / "other-clone" + other.mkdir() + res = _run_copy_of_run_sh(tmp_path, str(other)) + assert res.returncode == 1 + assert "MT5_PROJECT_DIR is" in res.stdout + res.stderr + # It stopped before doing any of run.sh's work in the directory. + assert not (tmp_path / "data").exists() + assert not (tmp_path / ".env").exists() + + +def test_run_sh_accepts_the_same_checkout_through_a_symlink(tmp_path): + """Equality is by resolved path, so a symlinked checkout is not rejected.""" + link = tmp_path.parent / (tmp_path.name + "-link") + link.symlink_to(tmp_path, target_is_directory=True) + try: + res = _run_copy_of_run_sh(tmp_path, str(link)) + finally: + link.unlink() + # Past the guard it fails later, on the missing compose sources - the + # point is only that the guard did not fire. + assert "MT5_PROJECT_DIR is" not in res.stdout + res.stderr + assert "neither vms.yaml" in res.stdout + res.stderr + + +def test_run_sh_derives_the_project_dir_when_unset(tmp_path): + res = _run_copy_of_run_sh(tmp_path, None) + assert "MT5_PROJECT_DIR is" not in res.stdout + res.stderr + assert "neither vms.yaml" in res.stdout + res.stderr + + +@pytest.mark.skipif( + subprocess.run(["docker", "compose", "version"], capture_output=True).returncode != 0 + if __import__("shutil").which("docker") else True, + reason="needs the docker compose CLI (the offline test image has none; runs on the host)", +) +def test_the_written_env_serves_compose_outside_run_sh(tmp_path): + """The property psyb0t named: a compose command in a FRESH shell, with no + MT5_PROJECT_DIR exported, must interpolate the shipped compose file from + .env alone - and fail without that line, so this is testing the line.""" + import shutil + + shutil.copy(_REPO / "docker-compose.yml.example", tmp_path / "docker-compose.yml") + assert _run_env_block(tmp_path).returncode == 0 + + def compose_config(env_extra=None): + env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "HOME": os.environ.get("HOME", "/tmp")} + env.update(env_extra or {}) + return subprocess.run( + ["docker", "compose", "-f", str(tmp_path / "docker-compose.yml"), "config", "--quiet"], + capture_output=True, text=True, env=env, cwd=str(tmp_path), timeout=120, check=False, + ) + + ok = compose_config() + assert ok.returncode == 0, ok.stderr + + (tmp_path / ".env").write_text("API_TOKEN=tok-123\n", encoding="utf-8") + broken = compose_config() + assert broken.returncode != 0 + assert "MT5_PROJECT_DIR" in broken.stderr diff --git a/tests/test_vm_watchdog.py b/tests/test_vm_watchdog.py index fa806ec3..f643ac8b 100644 --- a/tests/test_vm_watchdog.py +++ b/tests/test_vm_watchdog.py @@ -10,6 +10,8 @@ import importlib.util import json import os +import shutil +import subprocess from pathlib import Path import pytest @@ -250,13 +252,14 @@ def test_gives_up_after_max_attempts(wd, tmp_path, capsys, recorder): # ── Reset after stable health ──────────────────────────────────────────────── -def test_attempts_reset_after_sustained_healthy_period(wd, tmp_path): +def test_attempts_reset_after_sustained_healthy_period(wd, tmp_path, recorder): wd.STATE_DIR = str(tmp_path) wd.MIN_FAILING_STREAK = 1 wd.MAX_ATTEMPTS = 1 wd.BACKOFF_ATTEMPTS = [0] wd.RESET_SECONDS = 1800 cid = "aaa" + state_file = tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json" def make_client(status, streak): return _FakeClient([_container(cid, "mt5")], health={cid: _health(status, streak)}) @@ -267,17 +270,93 @@ def make_client(status, streak): # Two unhealthy sweeps: first restarts, second exceeds MAX_ATTEMPTS. assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now) == 1 assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now + 1) == 0 - assert "give up" in json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) or True + # The budget really is spent - one attempt recorded, one recreate made. + # (This line used to read `assert ... or True`, which asserted nothing.) + assert json.loads(state_file.read_text())["attempts"] == 1 + assert recorder.services == [cid] # VM recovers and stays healthy long enough to reset the budget. assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100) == 0 assert wd.sweep_once(make_client("healthy", 0), "mt5-httpapi", now=now + 100 + 1800) == 0 - state = json.loads((tmp_path / f"{wd.state_key('mt5-httpapi', cid)}.json").read_text()) + state = json.loads(state_file.read_text()) assert state["attempts"] == 0 assert state["last_restart"] == 0 # A later crash gets a fresh budget again. assert wd.sweep_once(make_client("unhealthy", 99), "mt5-httpapi", now=now + 100 + 1801) == 1 + assert recorder.services == [cid, cid] + + +# ── healthy_since means CONTINUOUSLY healthy ───────────────────────────────── +# +# psyb0t (2026-09-04): healthy_since survived a `starting` state, so a VM that +# was healthy before a restart had its attempt budget refunded on the first +# healthy poll AFTER it - not after RESET_SECONDS of proven stability. The same +# leak existed for an unhealthy poll below the streak threshold. + + +def _vm(cid, status, streak=0): + return _FakeClient([_container(cid, "mt5")], health={cid: _health(status, streak)}) + + +def _budget_spent_then_healthy_at(wd, cid, now): + """Spend the single-attempt budget, then observe the VM healthy at `now`. + Returns nothing; the caller continues the timeline.""" + assert wd.sweep_once(_vm(cid, "unhealthy", 99), "mt5-httpapi", now=now - 10) == 1 + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=now) == 0 + + +@pytest.mark.parametrize( + "interruption", + [("starting", 0), ("unhealthy", 1), ("none", 0)], + ids=["starting", "unhealthy-below-streak", "no-healthcheck"], +) +def test_any_non_healthy_observation_restarts_the_reset_clock(wd, tmp_path, recorder, interruption): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 10 + wd.MAX_ATTEMPTS = 1 + wd.BACKOFF_ATTEMPTS = [0] + wd.RESET_SECONDS = 1800 + cid = "aaa" + status, streak = interruption + key = wd.state_key("mt5-httpapi", cid) + + t0 = 1_000_000 + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + # Streak 99 so the spend is not blocked by the threshold above. + assert wd.sweep_once(_vm(cid, "unhealthy", 99), "mt5-httpapi", now=t0 - 10) == 1 + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=t0) == 0 + # Interrupted 1000s in, back to healthy 1900s in: 1900s since the + # ORIGINAL healthy poll (> RESET_SECONDS), but only 900s continuous. + assert wd.sweep_once(_vm(cid, status, streak), "mt5-httpapi", now=t0 + 1000) == 0 + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=t0 + 1900) == 0 + assert wd.load_state(key)["attempts"] == 1, "budget refunded across a non-healthy state" + + # Continuity restored at t0+1900: the reset happens 1800s after THAT. + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=t0 + 1900 + 1799) == 0 + assert wd.load_state(key)["attempts"] == 1 + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=t0 + 1900 + 1800) == 0 + assert wd.load_state(key)["attempts"] == 0 + + assert recorder.services == [cid] + + +def test_the_healthy_clock_starts_at_the_first_healthy_poll_after_recovery(wd, tmp_path, recorder): + """The recovery itself zeroes the clock; the next healthy poll starts it.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.BACKOFF_ATTEMPTS = [0] + cid = "aaa" + key = wd.state_key("mt5-httpapi", cid) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(_vm(cid, "unhealthy", 99), "mt5-httpapi", now=500) == 1 + assert wd.load_state(key)["healthy_since"] == 0 + assert wd.sweep_once(_vm(cid, "starting"), "mt5-httpapi", now=600) == 0 + assert wd.load_state(key)["healthy_since"] == 0 + assert wd.sweep_once(_vm(cid, "healthy"), "mt5-httpapi", now=700) == 0 + assert wd.load_state(key)["healthy_since"] == 700 # ── Dry-run ────────────────────────────────────────────────────────────────── @@ -620,16 +699,25 @@ def test_recreate_refuses_without_the_project_path(wd, monkeypatch): wd.recreate_vm("mt5", "mt5-httpapi") -def test_recreate_passes_the_project_name_explicitly(wd, monkeypatch, tmp_path): - """Compose derives the project from the directory name otherwise, and a - mismatch does not fail — it creates a SECOND set of containers beside the - running ones.""" +def test_recreate_hands_the_helper_the_project_name_and_the_project_dir(wd, monkeypatch, tmp_path): + """Two variables compose needs that the container was never handed. + + COMPOSE_PROJECT_NAME: compose derives the project from the directory name + otherwise, and a mismatch does not fail — it creates a SECOND set of + containers beside the running ones. + + MT5_PROJECT_DIR: the compose file interpolates `${MT5_PROJECT_DIR:?}` on + every compose command. This test used to assert only the project name, so + the helper failing at interpolation - psyb0t's 2026-09-04 blocker - was + invisible to it. The host variable is scrubbed first: the helper must get + it from the watchdog, not by inheritance from whoever ran the tests. + """ seen = {} def fake_run(cmd, **kwargs): seen["cmd"] = cmd seen["cwd"] = kwargs.get("cwd") - seen["project"] = (kwargs.get("env") or {}).get("COMPOSE_PROJECT_NAME") + seen["env"] = dict(kwargs.get("env") or {}) class R: returncode = 0 @@ -638,6 +726,7 @@ class R: return R() + monkeypatch.delenv("MT5_PROJECT_DIR", raising=False) monkeypatch.setattr(wd, "PROJECT_DIR", str(tmp_path)) monkeypatch.setattr(wd, "RECREATE_SCRIPT", "/project/scripts/recreate-vm.sh") monkeypatch.setattr(wd.subprocess, "run", fake_run) @@ -646,7 +735,21 @@ class R: assert seen["cmd"] == ["/project/scripts/recreate-vm.sh", "mt5"] assert seen["cwd"] == str(tmp_path) - assert seen["project"] == "mt5-httpapi" + assert seen["env"]["COMPOSE_PROJECT_NAME"] == "mt5-httpapi" + assert seen["env"]["MT5_PROJECT_DIR"] == str(tmp_path) + + +def test_recreate_env_is_the_watchdogs_own_project_dir_not_the_hosts(wd, monkeypatch, tmp_path): + """Even when the host shell HAS the variable, the helper gets the + watchdog's WATCHDOG_PROJECT_DIR: that is the path the project is mounted at + inside this container, which is what compose must resolve mounts against.""" + monkeypatch.setenv("MT5_PROJECT_DIR", "/somewhere/else/entirely") + monkeypatch.setattr(wd, "PROJECT_DIR", str(tmp_path)) + env = wd.recreate_env("mt5-httpapi") + assert env["MT5_PROJECT_DIR"] == str(tmp_path) + assert env["COMPOSE_PROJECT_NAME"] == "mt5-httpapi" + # Everything else is inherited: the helper needs PATH, HOME, DOCKER_HOST... + assert env["PATH"] == os.environ["PATH"] def test_recreate_surfaces_the_scripts_failure(wd, monkeypatch, tmp_path): @@ -674,3 +777,218 @@ def test_missing_project_dir_is_reported_at_startup(monkeypatch): def test_missing_recreate_script_is_reported_at_startup(monkeypatch): wd = _load_with(monkeypatch, WATCHDOG_RECREATE_SCRIPT="/nope/recreate-vm.sh") assert any("WATCHDOG_RECREATE_SCRIPT" in p for p in wd.validate_config()) + + +# ── The REAL helper, under the watchdog's EXACT child environment ──────────── +# +# psyb0t (2026-09-04): the unit suite passed while every real recovery failed, +# because nothing here ever ran scripts/recreate-vm.sh with the environment the +# watchdog actually gives it. These do. The docker CLI is a stub - the offline +# image has no daemon - but the stub emulates the one compose behaviour that +# matters: `${VAR:?msg}` interpolation of the compose file fails BEFORE any +# container is touched when VAR is unset or empty. Everything else in the chain +# is real: the watchdog's env construction, the bash script, its PyYAML sidecar +# discovery, the compose file. + +_COMPOSE_REQUIRING_PROJECT_DIR = """\ +services: + mt5: + image: dockurr/windows:5.14 + wickworks: + image: psyb0t/wickworks + network_mode: "service:mt5" + vm-watchdog: + image: python:3.12-alpine + volumes: + - ${MT5_PROJECT_DIR:?MT5_PROJECT_DIR must be the absolute host path of this project}:${MT5_PROJECT_DIR}:ro +""" + +_STUB_DOCKER = r"""#!/bin/sh +# Stand-in for the docker CLI. Emulates compose's `${VAR:?msg}` interpolation: +# when the compose file names MT5_PROJECT_DIR as required and it is unset or +# empty in THIS process's environment, fail exactly as compose does, before +# doing anything. Otherwise record the call and succeed. +printf 'argv: %s\n' "$*" >>"$STUB_LOG" +env | grep -E '^(MT5_PROJECT_DIR|COMPOSE_PROJECT_NAME)=' >>"$STUB_LOG" || true +file="" +prev="" +for a in "$@"; do + [ "$prev" = "-f" ] && file="$a" + prev="$a" +done +if [ -n "$file" ] && grep -q 'MT5_PROJECT_DIR:?' "$file" && [ -z "${MT5_PROJECT_DIR:-}" ]; then + echo 'error while interpolating services.vm-watchdog.volumes.[]: required variable MT5_PROJECT_DIR is missing a value' >&2 + exit 1 +fi +exit 0 +""" + + +def _project_with_stub_docker(tmp_path): + """A compose project holding the REAL helper, plus a stub docker on PATH.""" + project = tmp_path / "project" + (project / "scripts").mkdir(parents=True) + helper = project / "scripts" / "recreate-vm.sh" + shutil.copy(_REPO / "scripts" / "recreate-vm.sh", helper) + helper.chmod(0o755) + (project / "docker-compose.yml").write_text(_COMPOSE_REQUIRING_PROJECT_DIR, encoding="utf-8") + + bindir = tmp_path / "bin" + bindir.mkdir() + (bindir / "docker").write_text(_STUB_DOCKER, encoding="utf-8") + (bindir / "docker").chmod(0o755) + return project, helper, bindir, tmp_path / "stub.log" + + +def test_the_real_helper_recreates_under_the_watchdogs_exact_child_environment(wd, monkeypatch, tmp_path): + """The blocker, end to end: recreate_vm() -> real recreate-vm.sh -> docker + compose, with MT5_PROJECT_DIR scrubbed from the host so the ONLY way the + helper can have it is the watchdog putting it there.""" + project, helper, bindir, log = _project_with_stub_docker(tmp_path) + monkeypatch.delenv("MT5_PROJECT_DIR", raising=False) + monkeypatch.setenv("PATH", f"{bindir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setenv("STUB_LOG", str(log)) + monkeypatch.setattr(wd, "PROJECT_DIR", str(project)) + monkeypatch.setattr(wd, "RECREATE_SCRIPT", str(helper)) + + wd.recreate_vm("mt5", "mt5-httpapi") # raises RuntimeError on any failure + + recorded = log.read_text(encoding="utf-8") + assert f"MT5_PROJECT_DIR={project}" in recorded + assert "COMPOSE_PROJECT_NAME=mt5-httpapi" in recorded + # The real script planned the real operation, with the sidecar it found + # in the compose file - not some path the stub short-circuited. + assert "compose" in recorded and "stop" in recorded + assert "--force-recreate" in recorded + assert "wickworks" in recorded + + +def test_without_the_injected_variable_the_same_helper_fails_at_interpolation(tmp_path, monkeypatch): + """Guards the test above. Run the identical helper WITHOUT MT5_PROJECT_DIR + and it fails with compose's own error, before stopping anything - so a + regression to the old child environment turns the suite red rather than + quietly green.""" + project, helper, bindir, log = _project_with_stub_docker(tmp_path) + env = { + "PATH": f"{bindir}{os.pathsep}{os.environ['PATH']}", + "STUB_LOG": str(log), + "COMPOSE_PROJECT_NAME": "mt5-httpapi", + # No MT5_PROJECT_DIR - the pre-fix child environment. + } + res = subprocess.run( + [str(helper), "mt5"], cwd=str(project), env=env, + capture_output=True, text=True, timeout=60, check=False, + ) + assert res.returncode != 0 + assert "required variable MT5_PROJECT_DIR is missing a value" in res.stderr + assert "--force-recreate" not in log.read_text(encoding="utf-8"), "recreate ran despite the failed stop" + + +# ── Scope: exact image repository, and never itself ────────────────────────── +# +# psyb0t (2026-08-21), still open at c29b289: `startswith(IMAGE_FILTER)` also +# matched `dockurr/windows-not-the-vm`, and nothing excluded SELF_ID, so a valid +# operator override like WATCHDOG_IMAGE_FILTER=python selected the watchdog. + + +@pytest.mark.parametrize( + "image,selected", + [ + ("dockurr/windows", True), + ("dockurr/windows:5.14", True), + ("dockurr/windows@sha256:" + "ab" * 32, True), + ("dockurr/windows-not-the-vm:latest", False), + ("dockurr/windows2:1", False), + ("psyb0t/wickworks:v0.3.1", False), + ("notdockurr/windows:5.14", False), + ], +) +def test_image_filter_matches_the_repository_exactly(wd, tmp_path, recorder, image, selected): + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.IMAGE_FILTER = "dockurr/windows" + client = _FakeClient([_container("c1", "vm", image=image)], health={"c1": _health("unhealthy", 99)}) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + wd.sweep_once(client, "mt5-httpapi", now=1_000_000) + assert recorder.services == (["c1"] if selected else []), image + + +def test_the_watchdog_never_selects_itself_whatever_the_filter_says(wd, tmp_path, recorder): + """Docker sets the hostname to the SHORT container id; the list endpoint + reports the full one. Self-exclusion is unconditional and first, before + the image filter gets a say.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.IMAGE_FILTER = "python" + wd.SELF_ID = "abcdef123456" + me = _container("abcdef123456" + "0" * 52, "vm-watchdog", image="python:3.12-alpine") + vm = _container("vm1", "fake-vm", image="python:3.12-alpine") + client = _FakeClient( + [me, vm], + health={me["Id"]: _health("unhealthy", 99), "vm1": _health("unhealthy", 99)}, + ) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", now=1_000_000) == 1 + assert recorder.services == ["vm1"] + + +def test_an_empty_self_id_excludes_nothing_by_accident(wd): + """"".startswith("") is True for every id: an unset SELF_ID must not make + every container look like the watchdog and silently disable it.""" + wd.SELF_ID = "" + wd.SELF_FULL_ID = "" + assert wd._is_self("anything") is False + + +def test_self_exclusion_uses_the_resolved_full_id_when_the_hostname_is_a_name(wd, tmp_path, recorder): + """`hostname: watchdog` on the service (or WATCHDOG_SELF_ID set to a name) + used to defeat self-exclusion entirely, because the prefix test compared + container ids against a word. With the full id resolved at startup the + test is equality, and the hostname no longer matters.""" + wd.STATE_DIR = str(tmp_path) + wd.MIN_FAILING_STREAK = 1 + wd.IMAGE_FILTER = "python" + wd.SELF_ID = "watchdog" + full = "f" * 64 + wd.SELF_FULL_ID = full + me = _container(full, "vm-watchdog", image="python:3.12-alpine") + vm = _container("vm1", "fake-vm", image="python:3.12-alpine") + client = _FakeClient([me, vm], health={full: _health("unhealthy", 99), "vm1": _health("unhealthy", 99)}) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(wd, "log", lambda *a, **k: None) + assert wd.sweep_once(client, "mt5-httpapi", now=1_000_000) == 1 + assert recorder.services == ["vm1"] + + +def test_a_name_hostname_without_a_resolved_id_never_prefix_matches_other_containers(wd): + """The fallback is only the hex short-id prefix. A word must not exclude a + container whose id merely starts with the same letters.""" + wd.SELF_FULL_ID = "" + wd.SELF_ID = "abc" # too short / not a container id + assert wd._is_self("abc" + "0" * 61) is False + wd.SELF_ID = "watchdog" + assert wd._is_self("watchdog-lookalike") is False + wd.SELF_ID = "abcdef123456" # a real short id still works by prefix + assert wd._is_self("abcdef123456" + "0" * 52) is True + + +def test_main_resolves_its_own_full_id_before_the_first_sweep(monkeypatch): + """The resolution is what makes the equality test possible in production.""" + wd = _load_with(monkeypatch, WATCHDOG_COMPOSE_PROJECT="mt5-httpapi") + wd.SELF_ID = "self-watchdog-id" + + class Client(_FakeClient): + def inspect(self, cid): + if cid == "self-watchdog-id": + return {"Id": "e" * 64, "Config": {"Labels": {"com.docker.compose.project": "mt5-httpapi"}}} + return super().inspect(cid) + + monkeypatch.setattr(wd, "DockerClient", lambda *_a, **_k: Client([])) + # Stop main() at its first sleep, after startup work is done. + monkeypatch.setattr(wd.time, "sleep", lambda *_a: (_ for _ in ()).throw(KeyboardInterrupt())) + monkeypatch.setattr(wd, "log", lambda *a, **k: None) + with pytest.raises(KeyboardInterrupt): + wd.main() + assert wd.SELF_FULL_ID == "e" * 64 From 576f629d42150c548f4cb9a5ea4f05ab3a40e581 Mon Sep 17 00:00:00 2001 From: Marinski Date: Wed, 9 Sep 2026 21:21:14 +0300 Subject: [PATCH 4/4] test(watchdog): the lifecycle fixture's teardown needs MT5_PROJECT_DIR too, and must not be silent `docker compose down` interpolates the compose file like every other compose command, so without MT5_PROJECT_DIR it failed at the required-variable check - the very finding this PR round fixes, reproduced by its own harness - and check=False hid that, leaving the disposable project (a socket-mounted watchdog included) running after the suite. The teardown now passes the variable the way an operator's shell does and raises if it fails. The in-container reproduction test also echoes the inner exit code, so its failure is provably the compose run inside the sidecar and not the outer exec. Co-Authored-By: Claude Fable 5.1 --- .../integration/test_vm_watchdog_lifecycle.py | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_vm_watchdog_lifecycle.py b/tests/integration/test_vm_watchdog_lifecycle.py index 40fa88ea..fb7ada7b 100644 --- a/tests/integration/test_vm_watchdog_lifecycle.py +++ b/tests/integration/test_vm_watchdog_lifecycle.py @@ -173,9 +173,18 @@ def stack(tmp_path_factory): _wait(lambda: "scoped to compose project" in _watchdog_logs(), STARTUP_TIMEOUT_SECONDS, "watchdog started") yield project_dir finally: + # The operator's shell again: `down` interpolates the compose file too, + # so it needs MT5_PROJECT_DIR exactly as `make down` after run.sh does + # (psyb0t's finding #2, in miniature - without it this teardown failed + # at interpolation and, being check=False, silently left a + # socket-mounted watchdog running). Loud now: a leaked privileged + # container is worse than a noisy teardown. # --rmi local: the per-project build tag would otherwise accumulate one # dangling watchdog image per run. - _compose(project_dir, "down", "-v", "--remove-orphans", "--rmi", "local", check=False) + _compose( + project_dir, "down", "-v", "--remove-orphans", "--rmi", "local", + env_extra={"MT5_PROJECT_DIR": str(project_dir)}, + ) def test_the_watchdog_starts_clean_against_the_real_wiring(stack): @@ -190,10 +199,13 @@ def test_the_blocker_is_real_inside_the_sidecar(stack): """Reproduce psyb0t's finding where it bites: compose run INSIDE the watchdog container, with the container's own environment, fails to even parse the project file. This is what every pre-fix recovery hit.""" + # The INNER command's exit code is echoed so the failure is provably the + # inner compose (inside the container), not the outer `exec` wrapper. res = _compose(stack, "exec", "-T", "vm-watchdog", "sh", "-c", - f"docker compose -f {stack}/docker-compose.yml config --quiet", check=False) - assert res.returncode != 0, "compose succeeded without MT5_PROJECT_DIR - the reproduction no longer holds" - assert "MT5_PROJECT_DIR" in (res.stderr + res.stdout) + f"docker compose -f {stack}/docker-compose.yml config --quiet; echo INNER_RC=$?", check=False) + assert res.returncode == 0, f"outer exec failed, so nothing was tested: {res.stderr}" + assert "INNER_RC=" in res.stdout and "INNER_RC=0" not in res.stdout, res.stdout + assert "required variable MT5_PROJECT_DIR" in (res.stderr + res.stdout), res.stderr def test_the_watchdog_gives_the_helper_what_the_container_never_got(stack):