diff --git a/tools/lib_kill_emulator.sh b/tools/lib_kill_emulator.sh new file mode 100755 index 0000000..d3c4568 --- /dev/null +++ b/tools/lib_kill_emulator.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# Terminate running emulator instances, and nothing else. +# +# Source this and call kill_emulators. +# +# Why this exists instead of a one-line pkill: `pkill -f 'M uv-k5-v3'` matches the +# whole command line, which includes +# +# - the shell running the pkill, because the pattern sits in its own argv +# - any test script or editor whose command line happens to mention the machine +# - a webui.py-owned QEMU, killing the web UI out from under a user; that produced +# a 502 through the reverse proxy twice in one session +# +# Matching the binary name exactly with pgrep -x, then confirming the machine type +# in /proc/PID/cmdline, keeps the blast radius to actual emulator processes. + +# Kill emulator processes matching a QMP socket path, or all of them if none given. +# +# Scoping by socket matters even once the pattern is safe. A test that clears out +# "any emulator" still kills the one a running webui.py owns: the process survives +# but its guest is gone, so the user gets a dead screen and +# {"status":"unreachable"}. Passing the socket the caller is about to use leaves +# other people's instances alone. +kill_emulators() { + local want_sock="${1:-}" pid cmdline self=$$ + + for pid in $(pgrep -x qemu-system-arm 2>/dev/null || true); do + [ "$pid" = "$self" ] && continue + cmdline=$(tr '\0' ' ' < "/proc/$pid/cmdline" 2>/dev/null) || continue + + # Must be one of ours. + case "$cmdline" in + *uv-k5-v3*) ;; + *) continue ;; + esac + + # If a socket was named, only touch the instance serving it. + if [ -n "$want_sock" ]; then + case "$cmdline" in + *"$want_sock"*) ;; + *) continue ;; + esac + fi + + kill "$pid" 2>/dev/null || true + done +} diff --git a/tools/run.sh b/tools/run.sh index 6a33dd9..b5c286a 100755 --- a/tools/run.sh +++ b/tools/run.sh @@ -12,7 +12,11 @@ ELF="${1:-$HOME/uvk5-port/uvk5-sat/build/CW/nr7y.cw.elf}" FLASH="$HOME/uvk5-port/sim/assets/flash.img" QMP=/tmp/uvk5-qmp.sock -pkill -f 'M uv-k5-v3' 2>/dev/null || true +# Never `pkill -f 'M uv-k5-v3'` here: see tools/lib_kill_emulator.sh for why that +# takes down an unrelated webui.py along with it. +# shellcheck source=tools/lib_kill_emulator.sh +. "$(dirname "$0")/lib_kill_emulator.sh" +kill_emulators "$QMP" rm -f "$QMP" sleep 1 diff --git a/tools/test_kill_emulator.sh b/tools/test_kill_emulator.sh new file mode 100755 index 0000000..7b739d1 --- /dev/null +++ b/tools/test_kill_emulator.sh @@ -0,0 +1,111 @@ +#!/usr/bin/env bash +# The cleanup in run.sh must not kill anything other than an emulator. +# +# Regression test for a real incident: `pkill -f 'M uv-k5-v3'` matched a running +# webui.py's QEMU child and the shell issuing the pkill, taking the web UI offline +# and returning 502 through the reverse proxy. Twice. +set -u + +HERE="$(cd "$(dirname "$0")" && pwd)" +# shellcheck source=tools/lib_kill_emulator.sh +. "$HERE/lib_kill_emulator.sh" + +fails=0 +note() { printf ' %s\n' "$1"; } +fail() { printf 'FAIL %s\n' "$1"; fails=$((fails + 1)); } + +# A decoy whose command line mentions the machine type but is not the emulator. +# The old pkill pattern would have killed this. +sleep 120 & +decoy=$! +# Rename via a wrapper so the cmdline contains the trap string. +bash -c 'exec -a "sleep -M uv-k5-v3 decoy" sleep 120' & +decoy2=$! +sleep 1 + +note "decoy pids: $decoy $decoy2" + +kill_emulators + +if ! kill -0 "$decoy" 2>/dev/null; then + fail "killed a plain sleep" +else + note "PASS plain process untouched" +fi + +if ! kill -0 "$decoy2" 2>/dev/null; then + fail "killed a process merely mentioning uv-k5-v3 in its command line" +else + note "PASS non-emulator process mentioning uv-k5-v3 untouched" +fi + +# The script running this must survive too; the old pattern matched its own argv. +note "PASS this script survived (pid $$)" + +kill "$decoy" "$decoy2" 2>/dev/null || true +wait 2>/dev/null || true + +# And a real emulator must actually be terminated. +QEMU="$HOME/qemu-build/qemu-7.2+dfsg/build/qemu-system-arm" +ELF="$HOME/uvk5-port/uvk5-sat/build/CW/nr7y.cw.elf" +IMG=$(mktemp /tmp/kill-test-XXXX.img) +gzip -dc "$HERE/../assets/pristine/flash-pristine.img.gz" > "$IMG" + +IMG2=$(mktemp /tmp/kill-test2-XXXX.img) +gzip -dc "$HERE/../assets/pristine/flash-pristine.img.gz" > "$IMG2" + +if [ -x "$QEMU" ] && [ -f "$ELF" ]; then + # Two instances on different sockets, standing in for "the test's own" and + # "the one a running web UI owns". + "$QEMU" -M "uv-k5-v3,flash-image=$IMG" -nographic -monitor none \ + -qmp "unix:/tmp/kill-test-a.sock,server=on,wait=off" -kernel "$ELF" \ + >/dev/null 2>&1 & + mine=$! + "$QEMU" -M "uv-k5-v3,flash-image=$IMG2" -nographic -monitor none \ + -qmp "unix:/tmp/kill-test-b.sock,server=on,wait=off" -kernel "$ELF" \ + >/dev/null 2>&1 & + theirs=$! + sleep 3 + + if ! kill -0 "$mine" 2>/dev/null || ! kill -0 "$theirs" 2>/dev/null; then + fail "test emulators did not start" + else + kill_emulators /tmp/kill-test-a.sock + sleep 2 + + if kill -0 "$mine" 2>/dev/null; then + fail "the targeted emulator was left running" + else + note "PASS the emulator on the named socket is terminated" + fi + + # This is the property that protects a live session: someone else's + # instance must survive a scoped cleanup. + if kill -0 "$theirs" 2>/dev/null; then + note "PASS an emulator on another socket survives" + else + fail "killed an emulator belonging to a different socket" + fi + + # Unscoped still clears everything. + kill_emulators + sleep 2 + if kill -0 "$theirs" 2>/dev/null; then + fail "unscoped cleanup left an emulator running" + kill "$theirs" 2>/dev/null || true + else + note "PASS unscoped cleanup terminates all emulators" + fi + fi +else + note "SKIP emulator binary or firmware missing" +fi + +rm -f "$IMG" "$IMG2" /tmp/kill-test-a.sock /tmp/kill-test-b.sock +wait 2>/dev/null || true + +if [ "$fails" -gt 0 ]; then + echo "$fails check(s) failed" + exit 1 +fi +echo "cleanup only ever kills emulators" diff --git a/tools/test_uvk5_supervisor.py b/tools/test_uvk5_supervisor.py index eb643fa..bd172ed 100644 --- a/tools/test_uvk5_supervisor.py +++ b/tools/test_uvk5_supervisor.py @@ -3,6 +3,7 @@ import os import socket import unittest +import unittest.mock from uvk5_supervisor import Supervisor @@ -237,5 +238,58 @@ class TestWaitForSocket(unittest.TestCase): self.assertFalse(wait_for_socket(self.path + ".missing", timeout=0.3)) +class TestRecoversFromAnExternalKill(unittest.TestCase): + """Power on must work again after the emulator dies behind the supervisor's back. + + Hit for real, twice: a stray cleanup killed the QEMU that webui.py owned. The + supervisor kept its QMP client object, so is_running() reported a broken pipe and + power_on() returned False immediately without relaunching -- the Power button was + dead until the whole service was restarted. + """ + + def _supervisor(self, clients): + launched = [] + + def launch(): + launched.append(1) + proc = unittest.mock.MagicMock() + proc.poll.return_value = None + proc.stderr = None + return proc + + def connect(): + return clients.pop(0) + + sup = Supervisor(launch, connect) + return sup, launched + + def test_power_on_relaunches_when_the_client_is_dead(self): + dead = unittest.mock.MagicMock() + dead.command.side_effect = BrokenPipeError("dead") + fresh = unittest.mock.MagicMock() + fresh.command.return_value = {"status": "running"} + + sup, launched = self._supervisor([dead, fresh]) + self.assertTrue(sup.power_on()) + self.assertEqual(len(launched), 1) + + # An external kill: the process is gone, so poll() reports an exit status. + sup._proc.poll.return_value = -15 + self.assertFalse(sup.is_running()) + + # Power on must notice the client is unusable and start a new emulator. + self.assertTrue(sup.power_on(), "power_on refused to relaunch a dead guest") + self.assertEqual(len(launched), 2, "no new emulator was launched") + self.assertTrue(sup.is_running()) + + def test_power_on_still_refuses_when_genuinely_running(self): + live = unittest.mock.MagicMock() + live.command.return_value = {"status": "running"} + sup, launched = self._supervisor([live]) + self.assertTrue(sup.power_on()) + self.assertFalse(sup.power_on(), "started a second emulator over a live one") + self.assertEqual(len(launched), 1) + + if __name__ == "__main__": unittest.main() diff --git a/tools/trace_run.sh b/tools/trace_run.sh index 42ccec0..b15d45a 100755 --- a/tools/trace_run.sh +++ b/tools/trace_run.sh @@ -12,7 +12,11 @@ FLASH="$HOME/uvk5-port/sim/assets/flash.img" LOG=/tmp/uvk5-trace.log QMP=/tmp/uvk5-qmp.sock -pkill -f 'M uv-k5-v3' 2>/dev/null || true +# Never `pkill -f 'M uv-k5-v3'` here: see tools/lib_kill_emulator.sh for why that +# takes down an unrelated webui.py along with it. +# shellcheck source=tools/lib_kill_emulator.sh +. "$(dirname "$0")/lib_kill_emulator.sh" +kill_emulators "$QMP" rm -f "$QMP" "$LOG" sleep 1 diff --git a/tools/uvk5_supervisor.py b/tools/uvk5_supervisor.py index a28edb8..c9dfb69 100644 --- a/tools/uvk5_supervisor.py +++ b/tools/uvk5_supervisor.py @@ -110,6 +110,18 @@ class Supervisor: def power_on(self) -> bool: with self._lock: + # A client object is not proof of a live guest. If the process died + # behind our back -- crashed, OOM-killed, or caught by someone else's + # cleanup -- the stale client made this return False forever, so the + # Power button did nothing until the whole service was restarted. + # Observed twice for real. + if self._client is not None and self._proc is not None \ + and self._proc.poll() is not None: + self._note(f"emulator exited on its own " + f"(status {self._proc.returncode}); restarting") + self._client = None + self._proc = None + if self._client is not None: return False self._proc = self._launch()