fix(microvm): reap the manager's whole process group on teardown

infra/firecracker/integration-test.sh

@@ -82,6 +82,10 @@ command -v firecracker >/dev/null 2>&1 || [ -n "${FC_BINARY_PATH:-}" ] \
 [ -s "${FC_ROOTFS_PATH:-}" ] || fail "FC_ROOTFS_PATH unset/empty — run build-image.sh first"
 
 WORK="$(mktemp -d)"
+# This script's own process group — cleanup refuses to signal it (see cleanup).
+SELF_PGID="$(ps -o pgid= -p $$ 2>/dev/null | tr -dc '0-9')"
+# Where the manager wrapper publishes its process group id (see the launch).
+MANAGER_PGID_FILE="${WORK}/manager.pgid"
 MANAGER_ADDR="127.0.0.1:50054"
 # The guest reaches the manager over its point-to-point TAP link (172.16.x.1),
 # never loopback, so the manager has to bind all interfaces. grpcurl still
@@ -94,13 +98,129 @@ SECRET_VALUE="itest-secret-value-do-not-log"
 # manager rejects an untenanted spec outright.
 ITEST_TENANT_ID="00000000-0000-0000-0000-000000000001"
 
+# Is something listening on the manager's gRPC port?
+#
+# Defined up here because cleanup uses it too: the port — not the pid — is what
+# the NEXT run actually needs back. No `nc` dependency; bash /dev/tcp is enough.
+port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; }
+
+# Reap the manager AND everything it spawned.
+#
+# `kill "${MANAGER_PID}"` was not enough, in two ways that compound: the manager
+# does not reliably exit on SIGTERM, and its firecracker children are reparented
+# rather than killed when it does. A failed run therefore left a live manager
+# holding :50054, and every LATER run then failed to bind and reported
+# "Firecracker backend did not report available" — a symptom that points at the
+# backend, or at whatever was last edited, rather than at the leaked process.
+# That cost three runs and briefly looked like a code regression.
+#
+# The manager is started in its own process group (see `setsid` below) so the
+# whole tree can be signalled by group id. Deliberately NOT a host-wide
+# `pkill -f firecracker`: that would kill unrelated VMs, and on the usual dev
+# host — a Lima VM named `firecracker-dev` — the pattern also matches the
+# caller's own shell command line and kills the session running this script.
 cleanup() {
-  [ -n "${MANAGER_PID:-}" ] && kill "${MANAGER_PID}" 2>/dev/null || true
-  # Best-effort teardown of any leaked TAPs / sockets from a failed boot.
+  # 1. Stop the VM through the manager FIRST, while the manager is still alive.
+  #
+  # Killing the manager's process group does NOT reap its VMs: the backend
+  # launches each firecracker with `pre_exec(libc::setsid())` (see
+  # services/runtime-manager/src/backends/firecracker.rs), so every VM leaves
+  # the manager's session before exec and survives a group kill. A run that
+  # failed after Spawn but before the explicit Stop would leave a live VM — and
+  # since the manager itself does die, the port check below would come back
+  # clean and report success while the VM leaked.
+  #
+  # Stop is also the only path that releases the VM's TAP, so this is a clean
+  # shutdown rather than a kill. Best-effort: before the certs exist, or with
+  # the manager already gone, grpcurl simply fails and the kill below covers it.
+  if [ -n "${VM_ID:-}" ] && [ -s "${WORK}/client.crt" ] && port_open; then
+    grpcurl -cacert "${WORK}/ca.crt" -cert "${WORK}/client.crt" -key "${WORK}/client.key" \
+      -servername localhost -import-path "${PROTO_DIR}" -proto "${PROTO_FILE}" \
+      -d "$(jq -n --arg id "${VM_ID}" '{vm_id:$id, reason:"integration-test cleanup"}')" \
+      "${MANAGER_ADDR}" lantern.v1.RuntimeManager/Stop >/dev/null 2>&1 || true
+  fi
+
+  # The pgid may have been published after the handshake loop gave up, so read
+  # the file again rather than trusting only the variable. Without this, a
+  # handshake that times out while the manager IS running leaks it — the very
+  # thing `MANAGER_PID=$!` used to cover.
+  if [ -z "${MANAGER_PGID:-}" ] && [ -s "${MANAGER_PGID_FILE:-/nonexistent}" ]; then
+    MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)"
+  fi
+
+  # Never signal our own process group, whatever went wrong upstream: that would
+  # kill this script (and, under CI, the runner step) instead of the manager.
+  # The handshake below should make this unreachable; it is here because the
+  # cost of being wrong is losing the run and its diagnostics.
+  if [ -n "${MANAGER_PGID:-}" ] && [ "${MANAGER_PGID}" = "${SELF_PGID:-}" ]; then
+    printf '\033[1;33m[integration] WARN:\033[0m %s\n' \
+      "refusing to signal process group ${MANAGER_PGID} — it is this script's own" >&2
+    MANAGER_PGID=""
+  fi
+  if [ -n "${MANAGER_PGID:-}" ]; then
+    kill -TERM -- "-${MANAGER_PGID}" 2>/dev/null || true
+    # Give a graceful shutdown a bounded chance (~6s), then stop asking.
+    for _ in $(seq 1 30); do
+      port_open || break
+      sleep 0.2
+    done
+    kill -KILL -- "-${MANAGER_PGID}" 2>/dev/null || true
+  elif [ -n "${MANAGER_LAUNCH_PID:-}" ]; then
+    # No usable group id — the wrapper never published one. Fall back to the pid
+    # `&` gave us, which is what the pre-handshake code always used. It is the
+    # manager itself when setsid exec'd, and a already-exited wrapper when setsid
+    # forked, so this is safe either way and strictly better than leaking.
+    kill -TERM "${MANAGER_LAUNCH_PID}" 2>/dev/null || true
+    sleep 1
+    kill -KILL "${MANAGER_LAUNCH_PID}" 2>/dev/null || true
+  fi
+
+  # Backstop for any VM that outlived the manager (each firecracker setsid()s
+  # into its own session, so nothing above reaches it). Matched on THIS run's
+  # vm_id in the api socket path — a uuid, so it cannot match another run's VM,
+  # an unrelated hypervisor, or this script's own command line. That last part
+  # matters: a broad `pkill -f firecracker` also matches the usual dev host's
+  # Lima VM name and the shell running this script.
+  # Matched on the vm_id ANYWHERE in a firecracker process's argv, not on the
+  # bare `/run/firecracker/<id>.sock` path: under jailer mode the socket lives
+  # inside the jail root instead, so a path match would miss it and leak the VM.
+  # The id is a uuid minted for this run, and only processes actually named
+  # `firecracker` are considered, so this cannot match another run, an unrelated
+  # hypervisor, or this script.
+  if [ -n "${VM_ID:-}" ]; then
+    for pid in $(pgrep -x firecracker 2>/dev/null); do
+      [ "${pid}" = "$$" ] && continue
+      if tr '\0' ' ' <"/proc/${pid}/cmdline" 2>/dev/null | grep -qF "${VM_ID}"; then
+        kill -KILL "${pid}" 2>/dev/null || true
+      fi
+    done
+  fi
+
+  # Confirm the port came back. If it did not, say so HERE — the next run would
+  # otherwise fail with the misleading message described above.
+  for _ in $(seq 1 25); do
+    port_open || break
+    sleep 0.2
+  done
+  if port_open; then
+    printf '\033[1;33m[integration] WARN:\033[0m %s\n' \
+      "port 50054 still held after teardown; the next run will fail to bind. \
+Find it with: sudo ss -lntp | grep 50054" >&2
+  fi
+
   rm -rf "${WORK}" 2>/dev/null || true
 }
 trap cleanup EXIT
 
+# Fail fast, and for the RIGHT reason, if the port is already taken — by a
+# manager leaked from an earlier run, or by anything else. Without this the
+# script starts a manager that cannot bind and reports a backend problem.
+if port_open; then
+  fail "something is already listening on 127.0.0.1:50054 (likely a runtime-manager
+  leaked by an earlier run). This script cannot start its own manager until that
+  is gone. Find and stop it with:  sudo ss -lntp | grep 50054"
+fi
+
 # ---------------------------------------------------------------------------
 # 1. Mint a manager mTLS CA + server cert. The Firecracker backend issues a
 #    per-VM client cert (CN=vm_id) signed by this CA at spawn; the harness
@@ -160,7 +280,29 @@ fi
 [ -x "${MANAGER_BIN}" ] || fail "manager binary not found at ${MANAGER_BIN}"
 
 log "Starting runtime-manager (RUNTIME_BACKEND=firecracker) on ${MANAGER_ADDR}"
-env \
+# `setsid` puts the manager in its own process group so cleanup can reap it and
+# every firecracker VM it spawns as one unit, without signalling anything else
+# on the host.
+#
+# The wrapper publishes the group id ITSELF rather than letting us infer it,
+# because `setsid` behaves two different ways and neither is safe to guess at:
+#
+#   - caller is NOT already a group leader (a script under CI, job control off):
+#     setsid execs in place, so the leader keeps the pid `$!` gave us — but only
+#     AFTER setsid() has run. Reading the pgid right after `&` is a race, and
+#     losing it returns the SCRIPT'S OWN pgid. Cleanup would then `kill` the
+#     test script's process group: the same self-kill this file already warns
+#     about for `pkill -f firecracker`, arrived at from the other direction.
+#   - caller IS a group leader (sourced from an interactive shell): setsid
+#     forks, `$!` is the short-lived wrapper, and the pid we captured exits at
+#     once — so the health check below would declare the manager dead, and a
+#     pgid derived from it would be wrong.
+#
+# Inside the wrapper, `$$` is the session leader in BOTH cases, so it is the one
+# value that is always right. It writes that, then execs the manager, which
+# inherits the pid and stays the leader.
+setsid bash -c 'echo $$ >"$1"; shift; exec "$@"' _ "${MANAGER_PGID_FILE}" \
+  env \
   RUNTIME_BACKEND=firecracker \
   LANTERN_RUNTIME_BACKEND=firecracker \
   LISTEN_ADDR="${MANAGER_LISTEN_ADDR}" \
@@ -175,11 +317,27 @@ env \
   LANTERN_VM_SIGNING_CA_KEY="${WORK}/ca.key" \
   "${SECRET_ENV_KEY}=${SECRET_VALUE}" \
   "${MANAGER_BIN}" >"${MANAGER_LOG}" 2>&1 &
-MANAGER_PID=$!
+# Recorded BEFORE the handshake so an early failure still has something to kill:
+# every `fail` from here on runs cleanup, and until the pgid is published this is
+# the only handle on the manager.
+MANAGER_LAUNCH_PID=$!
+
+# Wait for the wrapper to publish its pid — this is a handshake, not a guess, so
+# there is no race left to lose.
+for _ in $(seq 1 50); do
+  [ -s "${MANAGER_PGID_FILE}" ] && break
+  sleep 0.1
+done
+MANAGER_PGID="$(tr -dc '0-9' <"${MANAGER_PGID_FILE}" 2>/dev/null)"
+[ -n "${MANAGER_PGID}" ] || fail "manager wrapper never published its process group id"
+# The wrapper exec'd into the manager, so the leader's pid IS the manager's pid.
+# Use it for liveness rather than `$!`, which is the setsid wrapper when setsid
+# forks and would look dead immediately.
+MANAGER_PID="${MANAGER_PGID}"
 
 # Wait for the gRPC port to accept connections. The manager has no gRPC
-# reflection, so probe the raw TCP port via bash /dev/tcp (no nc dependency).
-port_open() { (exec 3<>/dev/tcp/127.0.0.1/50054) 2>/dev/null && exec 3>&- 3<&-; }
+# reflection, so probe the raw TCP port (port_open is defined near cleanup,
+# which needs it too).
 for _ in $(seq 1 100); do
   port_open && break
   kill -0 "${MANAGER_PID}" 2>/dev/null || fail "manager exited early; log:\n$(cat "${MANAGER_LOG}")"