fix(bin): identify herdr lab viewers by kernel start time so stop works on WSL2

bin/fm-herdr-lab-viewer.py

@@ -78,29 +78,34 @@ def _child(slave, master, session):
     os._exit(127)
 
 
-def _process_start(pid):
+# bin/fm-herdr-lab.sh owns the process identity format the stop guard
+# compares, so the record takes its values from that same function.
+IDENTITY_HELPER = os.path.join(os.path.dirname(os.path.abspath(__file__)), "fm-herdr-lab.sh")
+
+
+def _process_identity(pid):
     result = subprocess.run(
-        ["ps", "-p", str(pid), "-o", "lstart="],
+        ["bash", "-c", '. "$1" && fm_herdr_lab_process_identity "$2"',
+         "fm-herdr-lab-viewer", IDENTITY_HELPER, str(pid)],
         check=True,
         capture_output=True,
         text=True,
-        env={**os.environ, "LC_ALL": "C"},
     )
     value = result.stdout.strip()
-    if not value:
-        raise RuntimeError("process start time unavailable")
+    if not value or "\n" in value:
+        raise RuntimeError("process identity unavailable")
     return value
 
 
 def _write_pidfile(path, launcher_pid, viewer_pid):
-    launcher_start = _process_start(launcher_pid)
-    viewer_start = _process_start(viewer_pid)
+    launcher_identity = _process_identity(launcher_pid)
+    viewer_identity = _process_identity(viewer_pid)
     temporary = "%s.%d.tmp" % (path, launcher_pid)
     with open(temporary, "w", encoding="utf-8") as handle:
         handle.write("launcher_pid=%d\n" % launcher_pid)
-        handle.write("launcher_start=%s\n" % launcher_start)
+        handle.write("launcher_identity=%s\n" % launcher_identity)
         handle.write("viewer_pid=%d\n" % viewer_pid)
-        handle.write("viewer_start=%s\n" % viewer_start)
+        handle.write("viewer_identity=%s\n" % viewer_identity)
     os.rename(temporary, path)
 
 

bin/fm-herdr-lab.sh

@@ -199,8 +199,40 @@ fm_herdr_lab_viewer_reason() { # <session>
   printf '%s' "$out" | jq -r '.result.reason // empty' 2>/dev/null
 }
 
-fm_herdr_lab_process_start() { # <pid>
-  LC_ALL=C ps -p "$1" -o lstart= 2>/dev/null | sed 's/^[[:space:]]*//;s/[[:space:]]*$//'
+# Prints one line identifying a live process across PID reuse, the value the
+# viewer record stores. A Linux-compatible /proc supplies stat field 22
+# (starttime, clock ticks since boot) qualified by the kernel boot id when that
+# is readable. The kernel fixes starttime at fork, while ps derives lstart from
+# the wall clock and a boot time that WSL2 re-renders seconds apart for the same
+# live process, so an lstart identity wrongly disowns the lab's own viewer.
+# Hosts without that /proc fall back to the locale-pinned lstart, which their
+# kernels record directly. FM_PROC_ROOT_OVERRIDE replaces /proc for tests.
+fm_herdr_lab_process_identity() { # <pid>
+  local pid=$1 proc_root stat_line starttime boot_id value
+  local -a stat_fields
+  case "$pid" in ''|*[!0-9]*) return 1 ;; esac
+  proc_root=${FM_PROC_ROOT_OVERRIDE:-/proc}
+  if [ -r "$proc_root/$pid/stat" ]; then
+    stat_line=$(cat "$proc_root/$pid/stat" 2>/dev/null) || return 1
+    # After the final comm delimiter, array index 19 is proc stat field 22.
+    read -r -a stat_fields <<< "${stat_line##*)}"
+    [ "${#stat_fields[@]}" -ge 20 ] || return 1
+    starttime=${stat_fields[19]}
+    case "$starttime" in ''|*[!0-9]*) return 1 ;; esac
+    boot_id=
+    [ ! -r "$proc_root/sys/kernel/random/boot_id" ] \
+      || boot_id=$(tr -cd 'a-f0-9-' < "$proc_root/sys/kernel/random/boot_id" 2>/dev/null) || boot_id=
+    if [ -n "$boot_id" ]; then
+      printf 'boot=%s starttime=%s\n' "$boot_id" "$starttime"
+    else
+      printf 'starttime=%s\n' "$starttime"
+    fi
+    return 0
+  fi
+  value=$(LC_ALL=C ps -p "$pid" -o lstart= 2>/dev/null | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+  [ -n "$value" ] || return 1
+  case "$value" in *$'\n'*|*$'\r'*) return 1 ;; esac
+  printf 'lstart=%s\n' "$value"
 }
 
 fm_herdr_lab_process_parent() { # <pid>
@@ -217,18 +249,18 @@ fm_herdr_lab_viewer_recorded_value() { # <session> <key>
 }
 
 fm_herdr_lab_viewer_owned_pair() { # <session>
-  local launcher_pid viewer_pid launcher_start viewer_start current_start parent_pid
+  local launcher_pid viewer_pid launcher_identity viewer_identity current_identity parent_pid
   launcher_pid=$(fm_herdr_lab_viewer_recorded_value "$1" launcher_pid) || return 1
   viewer_pid=$(fm_herdr_lab_viewer_recorded_value "$1" viewer_pid) || return 1
   case "$launcher_pid:$viewer_pid" in
     *[!0-9:]*) return 1 ;;
   esac
-  launcher_start=$(fm_herdr_lab_viewer_recorded_value "$1" launcher_start) || return 1
-  viewer_start=$(fm_herdr_lab_viewer_recorded_value "$1" viewer_start) || return 1
-  current_start=$(fm_herdr_lab_process_start "$launcher_pid") || return 1
-  [ -n "$current_start" ] && [ "$current_start" = "$launcher_start" ] || return 1
-  current_start=$(fm_herdr_lab_process_start "$viewer_pid") || return 1
-  [ -n "$current_start" ] && [ "$current_start" = "$viewer_start" ] || return 1
+  launcher_identity=$(fm_herdr_lab_viewer_recorded_value "$1" launcher_identity) || return 1
+  viewer_identity=$(fm_herdr_lab_viewer_recorded_value "$1" viewer_identity) || return 1
+  current_identity=$(fm_herdr_lab_process_identity "$launcher_pid") || return 1
+  [ -n "$current_identity" ] && [ "$current_identity" = "$launcher_identity" ] || return 1
+  current_identity=$(fm_herdr_lab_process_identity "$viewer_pid") || return 1
+  [ -n "$current_identity" ] && [ "$current_identity" = "$viewer_identity" ] || return 1
   parent_pid=$(fm_herdr_lab_process_parent "$viewer_pid") || return 1
   [ "$parent_pid" = "$launcher_pid" ] || return 1
   printf '%s %s' "$launcher_pid" "$viewer_pid"

tests/fm-herdr-lab.test.sh

@@ -279,11 +279,11 @@ start_viewer_fixture() {
 }
 
 write_viewer_record() {
-  local record=$1 launcher_pid=$2 viewer_pid=$3 launcher_start viewer_start
-  launcher_start=$(fm_herdr_lab_process_start "$launcher_pid") || fail "could not identify launcher fixture process"
-  viewer_start=$(fm_herdr_lab_process_start "$viewer_pid") || fail "could not identify viewer fixture process"
-  printf 'launcher_pid=%s\nlauncher_start=%s\nviewer_pid=%s\nviewer_start=%s\n' \
-    "$launcher_pid" "$launcher_start" "$viewer_pid" "$viewer_start" > "$record"
+  local record=$1 launcher_pid=$2 viewer_pid=$3 launcher_identity viewer_identity
+  launcher_identity=$(fm_herdr_lab_process_identity "$launcher_pid") || fail "could not identify launcher fixture process"
+  viewer_identity=$(fm_herdr_lab_process_identity "$viewer_pid") || fail "could not identify viewer fixture process"
+  printf 'launcher_pid=%s\nlauncher_identity=%s\nviewer_pid=%s\nviewer_identity=%s\n' \
+    "$launcher_pid" "$launcher_identity" "$viewer_pid" "$viewer_identity" > "$record"
 }
 
 test_viewer_start_cancels_an_unrecorded_launcher() {
@@ -381,7 +381,7 @@ test_viewer_stop_only_signals_owned_processes() {
 
   sleep 20 &
   holder_pid=$!
-  printf 'launcher_pid=%s\nlauncher_start=not-this-process\nviewer_pid=%s\nviewer_start=not-this-process\n' \
+  printf 'launcher_pid=%s\nlauncher_identity=not-this-process\nviewer_pid=%s\nviewer_identity=not-this-process\n' \
     "$holder_pid" "$holder_pid" > "$record"
   printf '%s\n' no_foreground_client > "$FAKE_STATE/$name.foreground"
   run_with_fake fm_herdr_lab_viewer_stop "$name" || fail "stop rejected a stale process record"
@@ -415,6 +415,153 @@ test_viewer_stop_requires_the_recorded_parent() {
   pass "fm-herdr-lab: viewer ownership requires the recorded parent"
 }
 
+# Hosts without a Linux-compatible /proc get a synthesized one for the fixture
+# pids, so the kernel-identity logic under test is the same on every platform.
+viewer_fixture_proc_root() { # <dir> <pid>...
+  local dir=$1 pid
+  shift
+  if [ -r "/proc/$$/stat" ]; then
+    printf '%s' /proc
+    return
+  fi
+  for pid in "$@"; do
+    mkdir -p "$dir/$pid"
+    printf '%s (sleep) S 1 1 1 0 -1 0 0 0 0 0 0 0 0 0 20 0 1 0 %s 0 0\n' "$pid" "$((1000 + pid))" > "$dir/$pid/stat"
+  done
+  printf '%s' "$dir"
+}
+
+# Prints the date one second after an LC_ALL=C lstart value.
+lstart_plus_one_second() { # <lstart>
+  local epoch
+  if epoch=$(LC_ALL=C date -d "$1" +%s 2>/dev/null); then
+    LC_ALL=C date -d "@$((epoch + 1))" '+%a %b %e %H:%M:%S %Y'
+  else
+    epoch=$(LC_ALL=C date -j -f '%a %b %e %T %Y' "$1" +%s) || return 1
+    LC_ALL=C date -j -r "$((epoch + 1))" '+%a %b %e %H:%M:%S %Y'
+  fi
+}
+
+# WSL2 re-renders `ps -o lstart` one to two seconds apart for the same live
+# process because ps derives it from a wall-clock boot time. A drifted lstart
+# must not disown the lab's own viewer, or teardown refuses while it is attached.
+test_viewer_stop_survives_lstart_drift() {
+  local name="fm-lab-viewer-drift-$$" record pair="$TMP_ROOT/viewer-drift-pair"
+  local driftbin="$TMP_ROOT/driftbin" real_ps real_lstart drifted_lstart proc_root waited
+  run_with_fake fm_herdr_lab_provision "$name" || fail "viewer-drift fixture provision failed"
+  record=$(run_with_fake fm_herdr_lab_viewer_record_path "$name")
+  start_viewer_fixture "$pair"
+  proc_root=$(viewer_fixture_proc_root "$TMP_ROOT/drift-proc" "$FIXTURE_LAUNCHER_PID" "$FIXTURE_VIEWER_PID")
+  FM_PROC_ROOT_OVERRIDE="$proc_root" write_viewer_record "$record" "$FIXTURE_LAUNCHER_PID" "$FIXTURE_VIEWER_PID"
+
+  real_ps=$(command -v ps)
+  mkdir -p "$driftbin"
+  cat > "$driftbin/ps" <<SH
+#!/usr/bin/env bash
+case " \$* " in
+  *" lstart= "*)
+    value=\$(LC_ALL=C "$real_ps" "\$@") || exit \$?
+    value=\$(printf '%s' "\$value" | sed 's/^[[:space:]]*//;s/[[:space:]]*\$//')
+    if epoch=\$(LC_ALL=C date -d "\$value" +%s 2>/dev/null); then
+      LC_ALL=C date -d "@\$((epoch + 1))" '+%a %b %e %H:%M:%S %Y'
+    else
+      epoch=\$(LC_ALL=C date -j -f '%a %b %e %T %Y' "\$value" +%s) || exit 1
+      LC_ALL=C date -j -r "\$((epoch + 1))" '+%a %b %e %H:%M:%S %Y'
+    fi
+    ;;
+  *) exec "$real_ps" "\$@" ;;
+esac
+SH
+  chmod +x "$driftbin/ps"
+  real_lstart=$(LC_ALL=C ps -p "$FIXTURE_VIEWER_PID" -o lstart= | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+  drifted_lstart=$(PATH="$driftbin:$PATH" ps -p "$FIXTURE_VIEWER_PID" -o lstart=)
+  [ -n "$real_lstart" ] && [ -n "$drifted_lstart" ] && [ "$real_lstart" != "$drifted_lstart" ] \
+    || fail "the drift fixture did not shift lstart (real '$real_lstart', drifted '$drifted_lstart')"
+
+  printf '%s\n' no_foreground_client > "$FAKE_STATE/$name.foreground"
+  PATH="$driftbin:$PATH" FM_PROC_ROOT_OVERRIDE="$proc_root" run_with_fake fm_herdr_lab_viewer_stop "$name" \
+    || fail "viewer stop failed under a one-second lstart drift"
+  # The fixture would exit on its own after 20 seconds, so only a prompt exit
+  # proves that stop signalled it.
+  waited=0
+  while kill -0 "$FIXTURE_VIEWER_PID" 2>/dev/null && [ "$waited" -lt 20 ]; do
+    "$REAL_SLEEP" 0.1
+    waited=$((waited + 1))
+  done
+  kill -0 "$FIXTURE_VIEWER_PID" 2>/dev/null && fail "a one-second lstart drift disowned the lab's own viewer"
+  wait "$FIXTURE_LAUNCHER_PID" 2>/dev/null || true
+  assert_absent "$record" "a confirmed detach left the viewer record behind"
+  run_with_fake fm_herdr_lab_teardown "$name" || fail "viewer-drift fixture teardown failed"
+  pass "fm-herdr-lab: a drifting ps lstart still identifies the lab's own viewer"
+}
+
+# A recorded pid now held by a different process must stay unsignalled: a
+# different kernel start time or boot on the kernel identity, and a different
+# lstart on the portable fallback, each mean a different process.
+test_viewer_stop_refuses_a_reused_pid() {
+  local name="fm-lab-viewer-reuse-$$" record pair="$TMP_ROOT/viewer-reuse-pair" proc_root
+  local launcher_identity viewer_identity starttime case_label forged_launcher forged_viewer
+  run_with_fake fm_herdr_lab_provision "$name" || fail "viewer-reuse fixture provision failed"
+  record=$(run_with_fake fm_herdr_lab_viewer_record_path "$name")
+  start_viewer_fixture "$pair"
+  proc_root=$(viewer_fixture_proc_root "$TMP_ROOT/reuse-proc" "$FIXTURE_LAUNCHER_PID" "$FIXTURE_VIEWER_PID")
+  launcher_identity=$(FM_PROC_ROOT_OVERRIDE="$proc_root" fm_herdr_lab_process_identity "$FIXTURE_LAUNCHER_PID") \
+    || fail "could not identify the reuse launcher fixture"
+  viewer_identity=$(FM_PROC_ROOT_OVERRIDE="$proc_root" fm_herdr_lab_process_identity "$FIXTURE_VIEWER_PID") \
+    || fail "could not identify the reuse viewer fixture"
+  case "$viewer_identity" in
+    *starttime=*) ;;
+    *) fail "a Linux-compatible proc root did not yield a kernel identity: $viewer_identity" ;;
+  esac
+  printf '%s\n' no_foreground_client > "$FAKE_STATE/$name.foreground"
+
+  for case_label in starttime boot; do
+    case "$case_label" in
+      starttime)
+        starttime=${launcher_identity##*starttime=}
+        forged_launcher="${launcher_identity%starttime=*}starttime=$((starttime + 1))"
+        starttime=${viewer_identity##*starttime=}
+        forged_viewer="${viewer_identity%starttime=*}starttime=$((starttime + 1))"
+        ;;
+      boot)
+        forged_launcher="boot=00000000-0000-0000-0000-000000000000 starttime=${launcher_identity##*starttime=}"
+        forged_viewer="boot=00000000-0000-0000-0000-000000000000 starttime=${viewer_identity##*starttime=}"
+        ;;
+    esac
+    [ "$forged_viewer" != "$viewer_identity" ] || fail "the $case_label reuse case did not change the identity"
+    printf 'launcher_pid=%s\nlauncher_identity=%s\nviewer_pid=%s\nviewer_identity=%s\n' \
+      "$FIXTURE_LAUNCHER_PID" "$forged_launcher" "$FIXTURE_VIEWER_PID" "$forged_viewer" > "$record"
+    FM_PROC_ROOT_OVERRIDE="$proc_root" run_with_fake fm_herdr_lab_viewer_stop "$name" \
+      || fail "stop rejected a $case_label-mismatched record"
+    kill -0 "$FIXTURE_VIEWER_PID" 2>/dev/null \
+      || fail "stop signalled a reused viewer pid with a different $case_label"
+    kill -0 "$FIXTURE_LAUNCHER_PID" 2>/dev/null \
+      || fail "stop signalled a reused launcher pid with a different $case_label"
+  done
+
+  # Without a compatible /proc the lstart fallback stays an exact match.
+  launcher_identity=$(FM_PROC_ROOT_OVERRIDE="$TMP_ROOT/no-proc" fm_herdr_lab_process_identity "$FIXTURE_LAUNCHER_PID") \
+    || fail "the lstart fallback could not identify the launcher fixture"
+  case "$launcher_identity" in
+    lstart=?*) ;;
+    *) fail "a host without /proc did not fall back to lstart: $launcher_identity" ;;
+  esac
+  viewer_identity=$(FM_PROC_ROOT_OVERRIDE="$TMP_ROOT/no-proc" fm_herdr_lab_process_identity "$FIXTURE_VIEWER_PID") \
+    || fail "the lstart fallback could not identify the viewer fixture"
+  forged_launcher=$(lstart_plus_one_second "${launcher_identity#lstart=}") || fail "could not shift the launcher lstart"
+  forged_viewer=$(lstart_plus_one_second "${viewer_identity#lstart=}") || fail "could not shift the viewer lstart"
+  printf 'launcher_pid=%s\nlauncher_identity=lstart=%s\nviewer_pid=%s\nviewer_identity=lstart=%s\n' \
+    "$FIXTURE_LAUNCHER_PID" "$forged_launcher" "$FIXTURE_VIEWER_PID" "$forged_viewer" > "$record"
+  FM_PROC_ROOT_OVERRIDE="$TMP_ROOT/no-proc" run_with_fake fm_herdr_lab_viewer_stop "$name" \
+    || fail "stop rejected an lstart-mismatched record"
+  kill -0 "$FIXTURE_VIEWER_PID" 2>/dev/null || fail "the lstart fallback signalled a reused viewer pid"
+
+  kill "$FIXTURE_VIEWER_PID" 2>/dev/null || true
+  wait "$FIXTURE_LAUNCHER_PID" 2>/dev/null || true
+  run_with_fake fm_herdr_lab_teardown "$name" || fail "viewer-reuse fixture teardown failed"
+  pass "fm-herdr-lab: a reused viewer pid with a different identity is never signalled"
+}
+
 test_interrupted_viewer_start_cancels_launcher() {
   local name="fm-lab-viewer-interrupt-$$" command_pid launcher_pid status=0
   local started="$TMP_ROOT/viewer-interrupt-started" attached="$TMP_ROOT/viewer-interrupt-attached"
@@ -473,7 +620,7 @@ test_viewer_stop_retains_record_when_detach_is_unreadable() {
   local name="fm-lab-viewer-unreadable-$$" record status=0
   run_with_fake fm_herdr_lab_provision "$name" || fail "unreadable-detach fixture provision failed"
   record=$(run_with_fake fm_herdr_lab_viewer_record_path "$name")
-  printf 'launcher_pid=99999999\nlauncher_start=stale\nviewer_pid=99999999\nviewer_start=stale\n' > "$record"
+  printf 'launcher_pid=99999999\nlauncher_identity=stale\nviewer_pid=99999999\nviewer_identity=stale\n' > "$record"
   FM_FAKE_HERDR_FAST_POLL=1 FM_FAKE_HERDR_TITLE_FAIL=1 \
     run_with_fake fm_herdr_lab_viewer_stop "$name" >/dev/null 2>&1 || status=$?
   expect_code 1 "$status" "an unreadable detach result on a running session must fail closed"
@@ -511,6 +658,8 @@ test_viewer_timeout_allows_launcher_escalation
 test_viewer_start_requires_its_owned_process
 test_viewer_stop_only_signals_owned_processes
 test_viewer_stop_requires_the_recorded_parent
+test_viewer_stop_survives_lstart_drift
+test_viewer_stop_refuses_a_reused_pid
 test_interrupted_viewer_start_cancels_launcher
 test_teardown_refuses_while_viewer_attached
 test_viewer_stop_retains_record_when_detach_is_unreadable