diff --git a/scripts/remote.sh b/scripts/remote.sh index 178ce9da..997b73d0 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1959,7 +1959,7 @@ _remote_sync_engine_start_locked() { # Stop only an engine whose argv proves that it owns this team. A stale # pidfile may point at a recycled, unrelated process and must never authorize # signalling that process. - IFS=$'\t' read -r old_state old_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r old_state old_pid < <(_remote_sync_engine_status "$team" --pidfile-only) if [ "$old_state" = "running" ]; then kill "$old_pid" 2>/dev/null || true fi @@ -2026,7 +2026,7 @@ _remote_sync_engine_stop() { local team="$1" pidfile pid state pidfile="$(_remote_sync_engine_pidfile "$team")" [ -f "$pidfile" ] || return 0 - IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team" --pidfile-only) if [ "$state" = "running" ]; then if ! _remote_sync_engine_reap_owned "$team" "$pid"; then echo "agmsg: sync engine pid $pid did not stop" >&2 @@ -2048,11 +2048,84 @@ _remote_sync_engine_stop() { rm -f "$(_remote_sync_engine_cycle_stamp "$team")" 2>/dev/null || true } +# Return the systemd user-unit state as "statepid". +# +# A unit that is not installed is not a systemd-managed team here, so callers +# retain the pidfile behavior. An existing unit is different: an active unit +# whose MainPID cannot be authenticated is UNKNOWN and must not be shadowed by +# a new unmanaged engine. The command can be replaced in tests; no host +# systemd query is needed there. +_remote_systemd_engine_status() { + local team="$1" unit show load active sub pid command systemctl_bin + unit="agmsg-remote-sync-$team.service" + systemctl_bin="${AGMSG_SYSTEMCTL:-systemctl}" + command -v "$systemctl_bin" >/dev/null 2>&1 || { printf 'unavailable\t\n'; return; } + show="$($systemctl_bin --user show "$unit" -p LoadState -p ActiveState -p SubState -p MainPID 2>/dev/null)" || { + printf 'absent\t\n' + return + } + load="$(printf '%s\n' "$show" | sed -n 's/^LoadState=//p')" + active="$(printf '%s\n' "$show" | sed -n 's/^ActiveState=//p')" + sub="$(printf '%s\n' "$show" | sed -n 's/^SubState=//p')" + pid="$(printf '%s\n' "$show" | sed -n 's/^MainPID=//p')" + [ "$load" = "not-found" ] && { printf 'absent\t\n'; return; } + case "$active:$sub" in + active:running) + if _agmsg_pid_valid "$pid" && _agmsg_pid_alive_local "$pid"; then + command="$(compat_get_cmdline "$pid" 2>/dev/null || true)" + if agmsg_cmdline_names_path "$command" "$SCRIPT_DIR/internal/remote-sync.mjs" && + case "$command" in *" run --team $team") true ;; *) false ;; esac; then + printf 'running\t%s\n' "$pid" + else + printf 'unknown\t%s\n' "$pid" + fi + else + printf 'unknown\t%s\n' "$pid" + fi + ;; + active:starting|active:reloading|active:auto-restart|activating:*|deactivating:*) + printf 'starting\t%s\n' "$pid" + ;; + inactive:*|failed:*) + printf 'inactive\t%s\n' "$pid" + ;; + *) + printf 'unknown\t%s\n' "$pid" + ;; + esac +} + # Print "\t", where pid is empty when no valid pid is available. # A live PID is not enough: PID reuse can make an unrelated process pass # kill -0, so running requires the exact engine script/team suffix in argv. +# The optional --pidfile-only mode is internal: lifecycle operations use it +# when they must inspect only the unmanaged engine represented by this +# command's pidfile. Human/JSON status and sync-start admission deliberately +# omit it so an exported environment value cannot hide a systemd-owned engine. _remote_sync_engine_status() { - local team="$1" pidfile pid command expected + local team="$1" mode="${2:-}" pidfile pid command expected systemd_state systemd_pid + case "$mode" in + ""|--pidfile-only) ;; + *) + echo "agmsg: internal error: unknown sync engine status mode '$mode'" >&2 + return 2 + ;; + esac + REMOTE_SYNC_ENGINE_SUPERVISOR="" + REMOTE_SYNC_ENGINE_SUPERVISOR_PID="" + if [ "$mode" != --pidfile-only ]; then + IFS=$'\t' read -r systemd_state systemd_pid < <(_remote_systemd_engine_status "$team") + else + systemd_state=absent + fi + case "$systemd_state" in + running|starting|inactive|unknown) + REMOTE_SYNC_ENGINE_SUPERVISOR="systemd" + REMOTE_SYNC_ENGINE_SUPERVISOR_PID="$systemd_pid" + printf '%s\t%s\n' "$systemd_state" "$systemd_pid" + return + ;; + esac pidfile="$(_remote_sync_engine_pidfile "$team")" if [ ! -f "$pidfile" ]; then printf 'stopped\t\n' @@ -2084,7 +2157,7 @@ _remote_sync_engine_status() { _remote_sync_engine_reap_owned() { local team="$1" owned_pid="$2" state pid signal attempts for signal in TERM KILL; do - IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team" --pidfile-only) if ! _agmsg_pid_alive_local "$owned_pid"; then return 0; fi [ "$state" = "running" ] && [ "$pid" = "$owned_pid" ] || return 1 kill "-$signal" "$owned_pid" 2>/dev/null || true @@ -2496,6 +2569,12 @@ _remote_status_one() { case "$engine_state" in running) echo "$team connected (engine running, pid $engine_pid) since $connected_at" ;; + starting) + echo "$team connected (engine starting under systemd, pid $engine_pid; do not run sync start) since $connected_at" ;; + inactive) + echo "$team connected (engine inactive under systemd; run: systemctl --user restart agmsg-remote-sync-$team.service) since $connected_at" ;; + unknown) + echo "$team connected (engine state unknown under systemd; do not run sync start; inspect systemctl --user status agmsg-remote-sync-$team.service) since $connected_at" ;; stopped) echo "$team connected (engine stopped — run: bash $(agmsg_shq "$SKILL_DIR/scripts/remote.sh") sync start $(agmsg_shq "$team")) since $connected_at" ;; stale) @@ -2870,11 +2949,18 @@ cmd_sync_start() { fi IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team") - if [ "$engine_state" = "running" ]; then - echo "Sync engine already running (pid $engine_pid)." - agmsg_lock_release - return - fi + case "$engine_state" in + running) + echo "Sync engine already running (pid $engine_pid)." + agmsg_lock_release + return + ;; + starting|inactive|unknown) + echo "agmsg: systemd owns team '$team' in state '$engine_state'; inspect or restart the user unit instead of sync start" >&2 + agmsg_lock_release + return 1 + ;; + esac logfile="$CONNECTION_ROOT/run/remote-sync.$team.log" [ -f "$logfile" ] && log_offset=$(( $(wc -c < "$logfile" | tr -d ' ') + 1 )) @@ -2924,7 +3010,7 @@ cmd_sync_start() { # not the rest of the machine. agmsg_lock_release while [ "$i" -lt "$ready_ceiling" ]; do - IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team" --pidfile-only) if [ "$engine_state" = "running" ] && [ "$ready_pid" = "$started_pid" ] && tail -c "+$log_offset" "$logfile" 2>/dev/null | awk -v nonce="\"startup_nonce\":\"$startup_nonce\"" ' @@ -3419,7 +3505,7 @@ cmd_set_endpoint() { done fi - IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team" --pidfile-only) [ "$engine_state" = "running" ] && was_running=1 _remote_sync_engine_stop "$team" || { echo "agmsg: the sync engine did not stop; refusing to move the endpoint under it" >&2 @@ -3450,7 +3536,7 @@ cmd_set_endpoint() { # command ran is restarted too (never silently left stopped, and a restart # is what hands it the moved address -- a running engine keeps its old # config in memory). _remote_sync_engine_start kills a live engine first. - IFS=$'\t' read -r end_state end_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r end_state end_pid < <(_remote_sync_engine_status "$team" --pidfile-only) if [ "$was_running" -eq 1 ] || [ "$end_state" = "running" ]; then # Same rule as cmd_pull and cmd_connect: the move is this command's purpose # and it is done by here, so a start failure reports rather than fails -- diff --git a/tests/test_remote_status_liveness.bats b/tests/test_remote_status_liveness.bats index f6253289..0bf3e39d 100644 --- a/tests/test_remote_status_liveness.bats +++ b/tests/test_remote_status_liveness.bats @@ -889,3 +889,124 @@ write_unownable_ps_fixture() { run bash -c "grep -v '^[[:space:]]*#' \"\$1\" | sed 's/_agmsg_pid_alive_local//g' | grep -c '_agmsg_pid_alive'" _ "$SCRIPTS/remote.sh" [ "$output" = "0" ] } + + +# systemd-supervised engine detection for #894. The process itself is real; +# only systemctl is replaced, so these tests exercise the same argv/liveness +# checks used on a host while remaining independent of the test runner's user bus. +write_systemd_show_fixture() { + local state="$1" pid="$2" sub=dead fake="$TEST_SKILL_DIR/fake-systemctl" + [ "$state" = active ] && sub=running + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--user" ] && [ "${2:-}" = "show" ]; then' \ + " printf '%s\n' 'LoadState=loaded' 'ActiveState=$state' 'SubState=$sub' 'MainPID=$pid'" \ + ' exit 0' \ + 'fi' \ + 'exit 1' > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +legacy_skip_systemd_probe_name() { + # Build the removed name without retaining it as a discoverable setting in + # the tree. These tests prove that an old wrapper exporting it is harmless. + printf '%s%s\n' AGMSG_SKIP_SYSTEMD _PROBE +} + +@test "status: exported skip variable cannot hide a verified systemd engine (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$ENGINE_PID")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + "$(legacy_skip_systemd_probe_name)=1" \ + bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "connected (engine running, pid $ENGINE_PID)" + refute grep -qi 'stale\|stopped' <<<"$output" +} + +@test "JSON status: exported skip variable cannot hide a verified systemd engine (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$ENGINE_PID")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + "$(legacy_skip_systemd_probe_name)=1" \ + bash "$SCRIPTS/remote.sh" status testteam --json + [ "$status" -eq 0 ] + [ "$(sqlite_mem "SELECT json_extract('$(printf '%s' "$output" | sed "s/'/''/g")', '\$.engine_state');")" = running ] + [ "$(sqlite_mem "SELECT json_extract('$(printf '%s' "$output" | sed "s/'/''/g")', '\$.engine_pid');")" = "$ENGINE_PID" ] +} + +@test "sync start: exported skip variable cannot duplicate an active systemd engine (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$ENGINE_PID")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + "$(legacy_skip_systemd_probe_name)=1" \ + bash "$SCRIPTS/remote.sh" sync start testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "already running (pid $ENGINE_PID)" + [ "$(find "$TEST_SKILL_DIR/run" -maxdepth 1 -name 'remote-sync.testteam.pid' | wc -l)" -eq 0 ] +} + +@test "engine status: pidfile-only mode does not invoke the systemd probe (#894)" { + start_matching_engine + local fake_bin fake_systemctl probe_log="$TEST_SKILL_DIR/systemctl-called" + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$TEST_SKILL_DIR/fake-systemctl-counting" + printf '%s\n' '#!/usr/bin/env bash' \ + ': > "$AGMSG_TEST_SYSTEMCTL_PROBE_LOG"' \ + "printf '%s\n' 'LoadState=loaded' 'ActiveState=active' 'SubState=running' 'MainPID=$ENGINE_PID'" \ + > "$fake_systemctl" + chmod +x "$fake_systemctl" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + AGMSG_TEST_SYSTEMCTL_PROBE_LOG="$probe_log" \ + bash -c 'source "$1/remote.sh"; _remote_sync_engine_status testteam --pidfile-only' _ "$SCRIPTS" + [ "$status" -eq 0 ] + [ "$output" = $'running\t'"$ENGINE_PID" ] + [ ! -e "$probe_log" ] +} + +@test "engine status: rejects an unknown internal mode (#894)" { + run bash -c 'source "$1/remote.sh"; _remote_sync_engine_status testteam --unknown-mode' _ "$SCRIPTS" + [ "$status" -eq 2 ] + [[ "$output" == *"unknown sync engine status mode '--unknown-mode'"* ]] +} + +@test "sync start: refuses an active systemd unit with unverified identity (#894)" { + sleep 30 & + local foreign_pid=$! + ENGINE_PIDS="$ENGINE_PIDS $foreign_pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$foreign_pid")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + bash "$SCRIPTS/remote.sh" sync start testteam + [ "$status" -eq 1 ] + printf '%s\n' "$output" | grep -q -F -- "systemd owns team 'testteam'" + refute test -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" +} + + +@test "status: reports an inactive systemd unit with restart guidance (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_systemctl + fake_systemctl="$(write_systemd_show_fixture inactive 0)" + + run env AGMSG_SYSTEMCTL="$fake_systemctl" bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "engine inactive under systemd" + printf '%s\n' "$output" | grep -q -F -- "systemctl --user restart agmsg-remote-sync-testteam.service" +}