diff --git a/scripts/lib/compat.sh b/scripts/lib/compat.sh index 99c92370b..79e804fd2 100644 --- a/scripts/lib/compat.sh +++ b/scripts/lib/compat.sh @@ -38,6 +38,12 @@ compat_get_ppid() { } # Get Windows PID (WINPID) for an MSYS2 process. Internal helper. +# +# NOT memoised, though `compat_pid_gone` reaches it from a poll that can turn +# 1600 times. A cache here would be keyed on a pid, and a pid stops naming the +# same process the moment that process exits -- which is the reuse hazard the +# callers of this are built to survive. Paying a fork per turn on msys is the +# cost; it is the same failure path #779 is already open about. _compat_get_winpid() { local pid="$1" ps -l -p "$pid" 2>/dev/null | awk ' @@ -57,6 +63,103 @@ _compat_cim_cmdline() { | tr -d '\r' | tr '\\' '/' } +# Is this process gone? Only when every probe available SAYS SO. +# +# THE TWO WRONG ANSWERS DO NOT COST THE SAME. A probe that wrongly says "alive" +# costs a signal aimed at a pid whose ownership the caller still has to prove. A +# probe that wrongly says "gone" leaves a live process nobody stops -- and that +# is #831 exactly: on Windows 11 three `sync start` attempts each read a running +# engine as dead, reported failure, and walked away, leaving three engines +# pulling. +# +# SO "COULD NOT ASK" IS NOT "GONE". Under Git Bash the Windows side is reached +# through a WINPID lookup and `tasklist`, and either can be missing, fail, or +# answer something this cannot parse. An earlier version of this function let all +# three fall through to "gone", which is the same collapse #652 was about -- +# rebuilt here, in the function written to prevent it (raised in review on #840). +# Every one of them now answers "not gone", and the caller's cmdline check is +# what still keeps a dead pid from reading as a running engine. +# +# Under Git Bash the pid these shells minted is an MSYS pid, which `tasklist` +# does not report at all -- asking it about one is how #567 lost every codex +# bridge -- so the Windows side is asked about the WINPID. +# +# The WINPID may be passed in. A caller that resolved it while the process was +# provably its own must keep using THAT mapping: `ps` stops answering for a pid +# whose MSYS side has exited, and re-deriving it after a signal is how the +# lookup disappears exactly when it is needed (#840 review). +# +# `_agmsg_pid_alive_local` lives in instance-id.sh, which most callers of this +# file do not source. Its absence must not be answerable either: an undefined +# function exits 127, which is not 0, which fell straight through to "gone". +compat_pid_gone() { + local pid="$1" winpid="${2:-}" listing="" + if ! declare -f _agmsg_pid_alive_local >/dev/null 2>&1; then + printf 'agmsg: compat_pid_gone needs lib/instance-id.sh sourced\n' >&2 + return 1 + fi + _agmsg_pid_alive_local "$pid" && return 1 + _agmsg_detect_platform + [ "$_agmsg_platform" = "msys" ] || return 0 + [ -n "$winpid" ] || winpid="$(_compat_get_winpid "$pid" 2>/dev/null || true)" + # No mapping means the Windows side was never asked. Not an answer. + case "$winpid" in ''|*[!0-9]*) return 1 ;; esac + command -v tasklist >/dev/null 2>&1 || return 1 + listing="$(MSYS_NO_PATHCONV=1 tasklist /FI "PID eq $winpid" 2>/dev/null)" || return 1 + case "$listing" in *"$winpid"*) return 1 ;; esac + # tasklist ran, and did not list it. Both sides agree. + return 0 +} + +# End a process tree this codebase started, on whatever the host calls it. +# +# `kill` alone is not enough under Git Bash. The sync engine is launched as +# `bash remote-sync.sh`, which runs `node`; the MSYS signal reaches the MSYS-side +# process and the native `node.exe` under it survives. Measured on Windows 11: +# `sync start` had already aimed a kill at each of three engines that were still +# running half an hour later (#831). Windows has no signal to deliver, so the +# tree is ended by pid instead -- `/T` for the children, and `/F` only on the +# second pass, after the polite attempt has been made and waited on. +# +# THE MAPPING IS RESOLVED BEFORE THE SIGNAL, and this ordering is the whole +# point. `ps -l -p ` is what turns a pid into a WINPID, and it stops +# answering once the MSYS side has exited -- so a `kill` sent first can take away +# the only means of naming the native process still running underneath. That is +# not a hypothetical: it is the reported symptom, "MSYS kill returns and node.exe +# is still there", reproduced by the order of two lines (#840 review). A caller +# that already resolved the WINPID while it could prove the process was its own +# passes it in, and the same mapping carries through the signal, the taskkill and +# the confirmation that it went. +# +# Neither half is allowed to fail this function -- a signal that could not be +# delivered is not distinguishable here from one delivered to a process that had +# already exited, and the caller decides by asking whether it is gone. +# +# WHAT THIS DOES NOT CHECK is whether the pid is the caller's to end. `/T` ends a +# whole tree, so on a recycled number that is somebody else's tree. Ownership is +# proven before this is called, by the cmdline and not by the number. +compat_signal_pid_tree() { + local pid="$1" sig="$2" winpid="${3:-}" + # ONE PLATFORM DECISION, AND IT HAPPENS HERE. Written as two -- a guard on the + # lookup and a second guard before the taskkill -- either one alone could be + # removed with nothing to show for it, so neither was actually held by a + # control. Off msys there is no Windows name for this process, and saying that + # once is what makes it testable. + _agmsg_detect_platform + if [ "$_agmsg_platform" = "msys" ]; then + [ -n "$winpid" ] || winpid="$(_compat_get_winpid "$pid" 2>/dev/null || true)" + else + winpid="" + fi + kill "-$sig" "$pid" 2>/dev/null || true + case "$winpid" in ''|*[!0-9]*) return 0 ;; esac + case "$sig" in + KILL) MSYS_NO_PATHCONV=1 taskkill /PID "$winpid" /T /F >/dev/null 2>&1 || true ;; + *) MSYS_NO_PATHCONV=1 taskkill /PID "$winpid" /T >/dev/null 2>&1 || true ;; + esac + return 0 +} + # Get full command line of a process. Replaces: ps -o args= -p # Does name ? # diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de5846..1532fa7c8 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1681,15 +1681,25 @@ _remote_sync_engine_start_locked() { } _remote_sync_engine_stop() { - local team="$1" pidfile pid state + local team="$1" pidfile pid state reaped=0 pidfile="$(_remote_sync_engine_pidfile "$team")" [ -f "$pidfile" ] || return 0 IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") if [ "$state" = "running" ]; then - if ! _remote_sync_engine_reap_owned "$team" "$pid"; then - echo "agmsg: sync engine pid $pid did not stop" >&2 - return 1 - fi + # 0 (signalled and gone) and 2 (already gone) are both "not running now", + # which is all this function needs. 2 cannot actually arrive here -- the + # `state = running` above already required the pid to be alive -- and it is + # accepted rather than left to fall into the failure branch by accident. + # `|| reaped=$?` and not a bare call: this file runs under `set -e`, where a + # bare call returning 1 exits the script instead of reaching the message + # below. The cost of the idiom is that errexit is off while the reap runs; + # the reap states every one of its outcomes with an explicit `return`. + _remote_sync_engine_reap_owned "$team" "$pid" || reaped=$? + case $reaped in + 0|2) ;; + *) echo "agmsg: sync engine pid $pid did not stop" >&2 + return 1 ;; + esac fi rm -f "$pidfile" # The cycle record goes with the engine that made it. Left behind, the NEXT @@ -1721,7 +1731,15 @@ _remote_sync_engine_status() { printf 'stale\t\n' return fi - if ! _agmsg_pid_alive_local "$pid"; then + # EVERY PROBE, not the POSIX one alone. Under Git Bash a live engine reads as + # dead to `kill -0` (#652), and this line answered `stale` for it -- which is + # what sent `sync start` away from a running engine three times in a row and + # left three of them pulling (#831). `compat_pid_gone` asks the Windows side + # about the WINPID as well, and only calls it gone when both agree. + # + # The identity check below is unchanged and still does the work `kill -0` never + # could: a recycled pid passes liveness and fails the cmdline. + if compat_pid_gone "$pid"; then printf 'stale\t%s\n' "$pid" return fi @@ -1739,21 +1757,65 @@ _remote_sync_engine_status() { fi } +# Returns 0 when it SIGNALLED the engine and the engine went, 2 when the engine +# already read as gone and nothing was signalled, 1 when it could not stop it. +# +# THE DIFFERENCE BETWEEN 0 AND 2 IS WHO IS SPEAKING. 0 is this function's own +# act; 2 is a reading, and a reading can be wrong -- on Windows a live engine +# read as dead (#652), so this returned "reaped" for a process that was pulling +# at the time. The caller then deleted its pidfile and walked away, and every +# later `sync start` added another one beside it: three invocations, three live +# engines, and `status` reporting stopped (#831). +# +# #832 fixed that probe. The separation stays because the harm does not depend on +# WHICH misreading happens: a record deleted for a process that is still running +# cannot be recovered by the operator, while a stale record is what `status` +# already knows how to describe. +# How many turns the readiness poll takes before giving up. +# +# A FUNCTION SO THE SHIPPED DEFAULT CAN BE CHECKED WITHOUT RUNNING IT. Bound by +# a case that let the poll run to the ceiling, it cost 52 seconds -- half that +# test file, on a CI shard already near its 25-minute cap, to hold one constant. +# Asking the resolver costs nothing and pins the same two facts: unset means the +# shipped 1600, and the seam is honoured when it is set. +_remote_sync_ready_turns() { + case "${AGMSG_TEST_SYNC_READY_TURNS:-}" in + ''|*[!0-9]*) printf '1600' ;; + *) printf '%s' "$AGMSG_TEST_SYNC_READY_TURNS" ;; + esac +} + _remote_sync_engine_reap_owned() { - local team="$1" owned_pid="$2" state pid signal attempts + local team="$1" owned_pid="$2" state pid signal attempts owned_winpid="" for signal in TERM KILL; do + # OWNERSHIP IS RE-DERIVED EVERY PASS, AND IT IS NOT THE PID NUMBER. + # + # A pid stops being an identity token the moment the process behind it + # exits: the number is reused, and a `kill -0` -- or a `taskkill /T` -- then + # lands on somebody else's tree. `_remote_sync_engine_status` answers + # `running` only when the cmdline still names this team's engine, which is + # the check that survives reuse (raised in review on #840). IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") - if ! _agmsg_pid_alive_local "$owned_pid"; then return 0; fi + if compat_pid_gone "$owned_pid"; then return 2; fi [ "$state" = "running" ] && [ "$pid" = "$owned_pid" ] || return 1 - kill "-$signal" "$owned_pid" 2>/dev/null || true + # THE WINDOWS NAME IS TAKEN HERE, WHILE THE LINE ABOVE STILL PROVES IT IS + # OURS, AND IT IS KEPT. `ps` stops answering for a pid whose MSYS side has + # exited, so the POSIX signal below can remove the only means of naming the + # native process still running underneath -- which is the reported symptom, + # "the kill returns and node.exe is still there". Resolving it after the + # signal reproduces the bug this function exists to fix (#840 review). The + # same mapping then carries through the signal, the taskkill, and the + # confirmation that it went. + [ -n "$owned_winpid" ] || owned_winpid="$(_compat_get_winpid "$owned_pid" 2>/dev/null || true)" + compat_signal_pid_tree "$owned_pid" "$signal" "$owned_winpid" attempts=0 while [ "$attempts" -lt 100 ]; do - _agmsg_pid_alive_local "$owned_pid" || return 0 + compat_pid_gone "$owned_pid" "$owned_winpid" && return 0 attempts=$((attempts + 1)) sleep 0.01 done done - ! _agmsg_pid_alive_local "$owned_pid" + compat_pid_gone "$owned_pid" "$owned_winpid" } # Upgrade a team that predates local ids: mint a team_id AND a member_id for @@ -2490,7 +2552,14 @@ cmd_sync_start() { # that is late or missing for ANY reason costs this caller its own wait and # not the rest of the machine. agmsg_lock_release - while [ "$i" -lt 1600 ]; do + # Test seam: how many turns the readiness poll takes before giving up. No-op + # unless set, and the default below is the shipped one. The give-up path is + # what the #831 regressions drive, and reaching it costs the full ceiling every + # time -- six cases of that is minutes of CI for a number none of them are + # about. A case that IS about the shipped ceiling leaves this unset. + local ready_turns + ready_turns="$(_remote_sync_ready_turns)" + while [ "$i" -lt "$ready_turns" ]; do IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team") if [ "$engine_state" = "running" ] && [ "$ready_pid" = "$started_pid" ] && tail -c "+$log_offset" "$logfile" 2>/dev/null | @@ -2519,9 +2588,16 @@ cmd_sync_start() { # A lock that cannot be retaken must not swallow the diagnostic below, so # the failure is reported and the files are left rather than removed blind: # a stale pidfile is what `status` already knows how to describe. - local relocked=1 + # + # 0 (this call stopped it) and 2 (every probe says it is gone) both mean the + # process is not there, and only then are its records this call's to remove. + # 1 -- it is still running, or ownership could not be proven -- keeps them and + # is reported below, because a pidfile is the only thing that names an engine + # and deleting it is what left three of them unreachable on Windows (#831). + local relocked=1 reaped=0 stop_winpid="" agmsg_lock_acquire "$TEAMS_DIR/$team" || relocked=0 - if _remote_sync_engine_reap_owned "$team" "$started_pid"; then + _remote_sync_engine_reap_owned "$team" "$started_pid" || reaped=$? # `set -e`: see _remote_sync_engine_stop + if [ "$reaped" -eq 0 ] || [ "$reaped" -eq 2 ]; then if [ "$relocked" -eq 1 ]; then # AND ONLY IF IT IS STILL OURS. Retaking the lock stops the file from # changing under the removal; it does not make the file this call's to @@ -2573,16 +2649,43 @@ cmd_sync_start() { # it. Measured: one after the first failed attempt, two after the second. { echo "agmsg: sync engine for '$team' did not become ready, and this command did not stop it." - echo " pid $started_pid is still running. It cannot reach the server -- that is why" - echo " it never became ready -- and it will keep retrying on a backoff." + # THE CAUSE IS NOT KNOWN HERE; THE CONSEQUENCE IS. Removing the cause + # claim took a measured fact out with it -- that the engine goes on + # retrying -- and #731's test was pinning exactly that fact, because it is + # what turns "a command failed" into "something is still running on your + # machine". The backoff is the engine's own documented loop, not an + # inference about this run. + echo " pid $started_pid is still running, and it will keep retrying on a backoff." + # WHY IT IS NOT READY IS NOT KNOWN HERE, and this used to say it was: that + # it could not reach the server, and that nothing was syncing for the team. + # Neither was measured. The engines this text was written for were reaching + # the server and pulling the whole time (#831), and a readiness marker can + # also be missed while the engine works -- every engine appends to one log + # and the lines tear into each other, which is the half of #831 this does + # not fix. What is known is what the two clauses above say. echo " This shell either could not confirm the process was ours or could not signal it." echo " A sandboxed agent (Codex is one) produces both: signals to other processes are" echo " blocked inside it, and ownership cannot be confirmed from in there either." - echo " Nothing is syncing for this team meanwhile." echo " Running sync start again leaves another one behind, and only the newest" echo " is recorded in $(_remote_sync_engine_pidfile "$team")." echo " Stop it from a shell that can signal it:" - echo " kill $started_pid" + stop_winpid="$(_compat_get_winpid "$started_pid" 2>/dev/null || true)" + case "${MSYSTEM:-}" in + MINGW*|MSYS*|CLANGARM*) + # `kill` HERE IS THE THING THAT WAS MEASURED NOT TO WORK. It reaches + # the MSYS process and the native node.exe under it keeps running -- + # which is how the engines in #831 survived a kill that had already + # been aimed at them. So the tree is named by its Windows pid instead. + case "$stop_winpid" in + ''|*[!0-9]*) echo " taskkill /PID /T /F (ps could not read one here)" ;; + *) echo " taskkill /PID $stop_winpid /T /F" ;; + esac + echo " (MSYS 'kill $started_pid' reaches the shell, not the node process under it.)" + ;; + *) + echo " kill $started_pid" + ;; + esac echo " or give up the binding entirely:" echo " remote.sh disconnect $(agmsg_shq "$team")" } >&2 diff --git a/tests/test_remote_engine_start_refusal.bats b/tests/test_remote_engine_start_refusal.bats index f5e76731e..5cbee3c45 100644 --- a/tests/test_remote_engine_start_refusal.bats +++ b/tests/test_remote_engine_start_refusal.bats @@ -192,7 +192,13 @@ skip_if_root() { local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" local starter i=0 j=0 freed=0 - bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & + # A SHORT CEILING, BECAUSE THIS CASE IS NOT ABOUT THE CEILING. It needs the + # starter to still be polling while the lock is inspected, and nothing more. + # Left at the shipped 1600 turns it took 151 seconds -- the single most + # expensive test on the macOS shard, which was being cancelled at its + # 25-minute cap. At 40 turns the starter still lives ~2s against a 3s + # observation window, and the case passed 5/5 locally. + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & starter=$! # The engine existing is what says the START is over and the WAIT has begun. diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats new file mode 100644 index 000000000..2b9eb9347 --- /dev/null +++ b/tests/test_sync_start_orphans.bats @@ -0,0 +1,642 @@ +#!/usr/bin/env bats + +# `sync start` must not walk away from a process it started (#831). +# +# Measured on Windows 11: readiness could not be confirmed, `sync start` reported +# failure, deleted the pidfile and left. The engine kept running and kept pulling. +# Three invocations left three live engines; a follow-up reproduction reached six. +# `status` reported stopped for all of them, and the pidfile was the only thing +# that had ever named them. +# +# TWO PROBES DISAGREE ON WINDOWS, AND THE CODE BELIEVED THE WRONG ONE. `kill -0` +# cannot see a live engine there (#652), so `_remote_sync_engine_status` answered +# `stale`, the reap refused to signal a pid it could not confirm, and the caller +# deleted the record anyway. +# +# The asymmetry is the whole design. A probe that wrongly says ALIVE costs one +# signal aimed at a process whose cmdline still names this team's engine. A probe +# that wrongly says GONE leaves that engine running with nobody to stop it. So +# "gone" now requires every probe to agree (`compat_pid_gone`), and the signal +# goes through whatever the host uses to end a tree (`compat_signal_pid_tree`) -- +# on Windows `taskkill /PID /T`, because an MSYS kill reaches the shell +# and not the node process under it. +# +# OWNERSHIP IS STILL THE CMDLINE, NOT THE NUMBER. A pid stops being an identity +# token the moment its process exits, and `taskkill /T` on a reused number ends +# somebody else's tree. Every pass re-reads `_remote_sync_engine_status`, which +# answers `running` only while the cmdline names this team's engine. +# +# WHAT THIS FILE DOES NOT COVER. #831 names two independent directions and this +# is the first. The second -- every engine appending to one shared log, so the +# lines tear into each other and every tool that reads that log, including the +# readiness poll, reads fragments -- is untouched, and no case below says +# anything about it. +# +# AND WHAT THE WINDOWS CASES ARE WORTH. They drive the Windows ROUTE on this +# host: `MSYSTEM` set, and a `ps`/`tasklist`/`taskkill` on PATH that speak the +# shapes Git Bash speaks. That measures that the route is chosen and that the +# process dies through it. It is not a measurement of native `node.exe` dying on +# real Windows; only the Windows machine closes that, and it is asked to. + +load test_helper + +# A team name of this file's own, because these cases COUNT AND KILL processes by +# their `--team` argument -- the only part of an engine's argv that is not shared +# (the store is passed in the environment, and never appears there). +TEAM=orphan831 + +setup() { + setup_test_env + bash "$SCRIPTS/join.sh" "$TEAM" alice claude-code /tmp/project-orphan831 >/dev/null + + local cfg="$TEST_SKILL_DIR/teams/$TEAM/config.json" escaped updated + escaped="$(sed "s/'/''/g" "$cfg")" + updated="$(sqlite_mem " + SELECT json_set('$escaped', '\$.remote_binding', json_object( + 'endpoint', 'https://remote.example', + 'server_instance_id', '018f0000-0000-7000-8000-000000000001', + 'remote_team_id', '018f0000-0000-7000-8000-000000000002', + 'protocol_version', 1, + 'capabilities', json_object('write_allowed_ciphers', json_array('none')), + 'connected_at', '2026-07-30T00:00:00Z', + 'disconnected_at', null + ));")" + printf '%s\n' "$updated" > "$cfg" + mkdir -p "$TEST_SKILL_DIR/run" + + PIDFILE="$TEST_SKILL_DIR/run/remote-sync.$TEAM.pid" + PATTERN="remote-sync.mjs run --team $TEAM" + # A run that was interrupted leaves one of these behind -- this file's cases + # are ABOUT a process outliving its command, so that is not hypothetical. The + # name is this file's own, so anything answering to it is ours to end. + pkill -f "$PATTERN" 2>/dev/null || true + # Then assert, because every count below is meaningless if one survived. + [ -z "$(pgrep -f "$PATTERN")" ] +} + +teardown() { + # This file exists BECAUSE an engine can be left running, so it cannot rely on + # the code under test to clean up after its own cases. + pkill -f "$PATTERN" 2>/dev/null || true + teardown_test_env +} + +# A Git-Bash-shaped Windows side, on this host. +# +# `_AGMSG_COMPAT_NO_PROC=1` RIDES WITH `MSYSTEM` EVERYWHERE BELOW. Forcing the +# msys branch on a POSIX host sends `compat_get_cmdline` down a road that reads +# `/proc//cmdline` when it can -- which macOS cannot and Linux can. On Linux +# it therefore short-circuits past the stubbed Windows lookup and returns the +# real cmdline with a TRAILING SPACE (`tr '\0' ' '` converts the final NUL), so +# the ownership check's `*" run --team "` suffix no longer matches, the +# reap cannot prove the process is ours, and the engine survives. Green on macOS, +# red on ubuntu, from a `/proc` that only one of them has. +# +# ps -l -p a WINPID column, as MSYS2's ps prints one. The number is +# derived from the pid so each process has its own. +# tasklist answers ALIVE for any WINPID whose marker file exists -- +# this is the probe that has to override the POSIX one. +# taskkill records the arguments it was called with, then ends the +# process for real, which is what Windows would do. +# +# `stub_windows blind` additionally makes the POSIX liveness probe answer "gone" +# for everything, which is the #652 reading the whole defect rests on. +stub_windows() { + WINSTUB="$TEST_SKILL_DIR/winstub" + # taskkill ALONE, for the negative control: reachable on PATH, with the host + # still answering what it really is. Kept in its own directory because the + # `uname` stub below is what makes the msys branch run, and a control that + # shares a directory with it is not a control (measured: the off-Windows case + # called taskkill, because $WINSTUB put a Windows `uname` on its PATH too). + WINSTUB_POSIX="$TEST_SKILL_DIR/winstub-posix" + TASKKILL_LOG="$TEST_SKILL_DIR/taskkill.log" + ALIVE_DIR="$TEST_SKILL_DIR/winalive" + mkdir -p "$WINSTUB" "$WINSTUB_POSIX" "$ALIVE_DIR" + + # `MSYSTEM` alone is not the switch. `_agmsg_detect_platform` asks `uname -s`, + # so a host that answers Darwin takes the POSIX branch however MSYSTEM is set -- + # measured while writing this, on a run where every Windows assertion passed + # vacuously because the branch under test never executed. Both are set below. + cat > "$WINSTUB/uname" <<'EOF_UN' +#!/usr/bin/env bash +if [ "$1" = "-s" ]; then printf 'MINGW64_NT-10.0-22631\n'; exit 0; fi +exec /usr/bin/uname "$@" +EOF_UN + + cat > "$WINSTUB/ps" <<'EOF_PS' +#!/usr/bin/env bash +# Only the `-l -p ` form is used here; anything else falls through to the +# real ps, so nothing outside this stub's purpose is affected. +# +# AND IT ANSWERS ONLY FOR A PID THAT IS STILL THERE. MSYS2's ps lists processes; +# once the MSYS side has exited there is no row and no WINPID column to read. An +# earlier version of this stub returned `900000 + pid` whatever the state, which +# made the pid-to-WINPID mapping immortal -- and an immortal mapping hides the +# defect these cases are about, because the code could always find a WINPID no +# matter when it asked (raised in review on #840). +if [ "$1" = "-l" ] && [ "$2" = "-p" ]; then + /bin/kill -0 "$3" 2>/dev/null || exit 1 + printf 'PID WINPID PPID STATE\n' + printf '%s %s 1 S\n' "$3" "$((900000 + $3))" + exit 0 +fi +exec /bin/ps "$@" +EOF_PS + + cat > "$WINSTUB/tasklist" <". The filter is ONE argument, so the number is +# the last word of THAT argument. Not \${*##* }: that form applies the pattern to +# each positional separately and joins them, which yields "/FI " and +# matches nothing -- it read as "the process is gone", the answer this stub +# exists to contradict. +winpid="" +for a in "\$@"; do case "\$a" in *' eq '*) winpid="\${a##* }" ;; esac; done +if [ -e "$ALIVE_DIR/\$winpid" ]; then + printf 'node.exe %s Console 1 100 K\n' "\$winpid" +fi +exit 0 +EOF_TL + + cat > "$WINSTUB/taskkill" <> "$TASKKILL_LOG" +winpid="" +while [ \$# -gt 0 ]; do + case "\$1" in /PID) winpid="\$2"; shift 2 ;; *) shift ;; esac +done +case "\$winpid" in ''|*[!0-9]*) exit 1 ;; esac +rm -f "$ALIVE_DIR/\$winpid" +kill -KILL "\$((winpid - 900000))" 2>/dev/null || true +exit 0 +EOF_TK + + # The cmdline, which is how ownership is proven. Under Git Bash there is no + # /proc for a native process, so compat_get_cmdline asks Windows through CIM; + # this answers that question for the WINPID, from the real process, so the + # ownership check the reap performs is the real one and not a stubbed yes. + cat > "$WINSTUB/powershell.exe" <<'EOF_PWSH' +#!/usr/bin/env bash +args="$*" +winpid="${args##*ProcessId=}" +winpid="${winpid%%\"*}" +case "$winpid" in ''|*[!0-9]*) exit 0 ;; esac +/bin/ps -o args= -p "$((winpid - 900000))" 2>/dev/null +EOF_PWSH + + # `ps` comes WITH it. Without a WINPID column the platform check is not the + # thing keeping taskkill away -- there is simply no number to pass it, and the + # control passes for the wrong reason. Measured: dropping the msys guard + # entirely left all ten cases green, because macOS `ps -l` has no such column. + cp "$WINSTUB/ps" "$WINSTUB_POSIX/ps" + cp "$WINSTUB/taskkill" "$WINSTUB_POSIX/taskkill" + chmod +x "$WINSTUB/uname" "$WINSTUB/ps" "$WINSTUB/tasklist" "$WINSTUB/taskkill" \ + "$WINSTUB/powershell.exe" "$WINSTUB_POSIX/ps" "$WINSTUB_POSIX/taskkill" +} + +# Marks a pid alive to the stubbed Windows side. +win_mark_alive() { : > "$ALIVE_DIR/$((900000 + $1))"; } + +# The driver: sources remote.sh and calls the real `cmd_sync_start`. +# +# A file rather than `bash -c`, so the only difference between the callers below +# is the mode argument. +write_driver() { + DRIVER="$TEST_SKILL_DIR/drive-sync-start.sh" + cat > "$DRIVER" <<'EOF_DRIVER' +#!/usr/bin/env bash +# $1 = team, $2 = mode +# honest nothing overridden +# blind the POSIX liveness probe reads every pid as gone -- the #652 reading +. "$SCRIPTS/remote.sh" +if [ "$2" = "blind" ]; then + _agmsg_pid_alive_local() { return 1; } +fi +rc=0 +cmd_sync_start "$1" || rc=$? +echo "driver: cmd_sync_start rc=$rc" +EOF_DRIVER + chmod +x "$DRIVER" +} + +# Runs the driver under the stubbed Windows side, having marked the engine alive +# there once it exists. The marking has to happen after the fork, so the driver +# runs in the background and this waits for it. +run_windows_driver() { + local mode="$1" out="$TEST_SKILL_DIR/win-$mode.out" p i=0 + env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + AGMSG_TEST_SYNC_READY_TURNS=40 \ + bash "$DRIVER" "$TEAM" "$mode" >"$out" 2>&1 & + p=$! + while [ ! -f "$PIDFILE" ] && [ "$i" -lt 400 ]; do i=$((i + 1)); sleep 0.05; done + [ -f "$PIDFILE" ] + ENGINE="$(cat "$PIDFILE")" + win_mark_alive "$ENGINE" + wait "$p" 2>/dev/null || true + OUT="$(cat "$out")" +} + +@test "sync start: a blind POSIX probe no longer hides the engine from the reap (#831)" { + # THE DEFECT ITSELF. `kill -0` says gone -- the Windows reading -- and the + # engine is running. It has to be stopped anyway, and no orphan may remain. + stub_windows + write_driver + run_windows_driver blind + + grep -qF 'driver: cmd_sync_start rc=1' <<<"$OUT" + + # Nothing of this team is left running. Counted by what is on the machine, not + # by what the pidfile says -- the pidfile only ever names the most recent. + local running + running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" + [ "$running" -eq 0 ] + # And its records are gone with it, so the next `sync start` starts one engine. + [ ! -f "$PIDFILE" ] +} + +@test "sync start: it is the Windows route that ended it, named by WINPID (#831)" { + # Asserted separately from the case above, because "the process died" does not + # say WHAT killed it -- on this host the POSIX kill would do it on its own, and + # that is precisely the thing that does not work on Windows. + stub_windows + write_driver + run_windows_driver blind + + [ -f "$TASKKILL_LOG" ] + # The tree, by the WINPID the ps stub derived for this engine -- not the pid. + grep -qF "/PID $((900000 + ENGINE)) /T" "$TASKKILL_LOG" + # AND THE NATIVE SIDE IS GONE. Only taskkill clears this marker, so it is the + # thing that says the process under the MSYS shell ended rather than the shell. + # Asserted separately from the log line: a taskkill aimed at the wrong WINPID + # would still write a log entry (raised in review on #840). + [ ! -e "$ALIVE_DIR/$((900000 + ENGINE))" ] +} + +@test "compat_signal_pid_tree: off Windows it does not reach for taskkill (#831)" { + # THE NEGATIVE CONTROL FOR THE ROUTE. Without it, "always call taskkill" + # satisfies the case above, and every POSIX host would depend on a Windows + # binary being on PATH. + stub_windows + local live + sleep 30 & live=$! + win_mark_alive "$live" + + cat > "$TEST_SKILL_DIR/sig.sh" <<'EOF_SIG' +#!/usr/bin/env bash +. "$SCRIPTS/lib/compat.sh" +compat_signal_pid_tree "$1" TERM +EOF_SIG + # No MSYSTEM, and the real uname, so the msys branch must not run -- while both + # taskkill AND a ps that yields a WINPID are on PATH, so "it was not called" is + # a decision this code made and not something the environment prevented. + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB_POSIX:$PATH" bash "$TEST_SKILL_DIR/sig.sh" "$live" + [ "$status" -eq 0 ] + [ ! -f "$TASKKILL_LOG" ] + # It still signalled, through the route this platform has. + local i=0 + while kill -0 "$live" 2>/dev/null && [ "$i" -lt 100 ]; do i=$((i + 1)); sleep 0.01; done + run kill -0 "$live" + [ "$status" -ne 0 ] +} + +@test "compat_pid_gone: one probe saying gone is not enough (#831)" { + # The property the rest of it stands on. The POSIX probe says gone; the Windows + # side says alive; the answer must be "not gone". + # + # A REAL process, because the `ps` stub now answers only for a pid that is + # still there -- which is the point of it, and which makes an invented number + # unusable here. + stub_windows + local live + sleep 30 & live=$! + win_mark_alive "$live" + + cat > "$TEST_SKILL_DIR/gone.sh" <<'EOF_GONE' +#!/usr/bin/env bash +. "$SCRIPTS/lib/instance-id.sh" +. "$SCRIPTS/lib/compat.sh" +_agmsg_pid_alive_local() { return 1; } # the #652 reading +rc=0 +compat_pid_gone "$1" || rc=$? +echo "gone rc=$rc" +EOF_GONE + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/gone.sh" "$live" + grep -qF 'gone rc=1' <<<"$output" + + # NEGATIVE CONTROL, in the same case: with the Windows side also saying gone -- + # the native process ended while the MSYS one lingers -- the answer flips. + # Without this, "never gone" would satisfy the assertion above. + rm -f "$ALIVE_DIR/$((900000 + live))" + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/gone.sh" "$live" + grep -qF 'gone rc=0' <<<"$output" + + kill "$live" 2>/dev/null || true +} + +@test "compat_pid_gone: no WINPID means the Windows side was not asked, not gone (#831)" { + # "COULD NOT ASK" IS NOT "GONE". Every way of failing to reach the Windows side + # used to fall through to gone -- which is the collapse #652 was about, rebuilt + # inside the function written to prevent it (raised in review on #840). + # + # Here `ps` cannot produce a WINPID. The POSIX probe says gone. The answer must + # still be "not gone", because nothing has actually said the process ended. + stub_windows + local blind="$TEST_SKILL_DIR/no-winpid" + mkdir -p "$blind" + printf '%s\n' '#!/usr/bin/env bash' 'exit 1' > "$blind/ps" + cp "$WINSTUB/uname" "$WINSTUB/tasklist" "$blind/" + chmod +x "$blind/ps" "$blind/uname" "$blind/tasklist" + + cat > "$TEST_SKILL_DIR/gone2.sh" <<'EOF_GONE2' +#!/usr/bin/env bash +. "$SCRIPTS/lib/instance-id.sh" +. "$SCRIPTS/lib/compat.sh" +_agmsg_pid_alive_local() { return 1; } +rc=0 +compat_pid_gone "$1" || rc=$? +echo "gone rc=$rc" +EOF_GONE2 + run env SCRIPTS="$SCRIPTS" PATH="$blind:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/gone2.sh" 4242 + grep -qF 'gone rc=1' <<<"$output" +} + +@test "compat_pid_gone: no tasklist means the Windows side was not asked either (#831)" { + # The other way of not being able to ask. A WINPID is available and the probe + # itself is missing; the answer is the same, and it is asserted separately + # because these are two different failures on two different lines. + stub_windows + local notl="$TEST_SKILL_DIR/no-tasklist" + mkdir -p "$notl" + cp "$WINSTUB/uname" "$WINSTUB/ps" "$notl/" + chmod +x "$notl/uname" "$notl/ps" + local live + sleep 30 & live=$! + + cat > "$TEST_SKILL_DIR/gone3.sh" <<'EOF_GONE3' +#!/usr/bin/env bash +. "$SCRIPTS/lib/instance-id.sh" +. "$SCRIPTS/lib/compat.sh" +_agmsg_pid_alive_local() { return 1; } +rc=0 +compat_pid_gone "$1" || rc=$? +echo "gone rc=$rc" +EOF_GONE3 + # PATH without the real one either, so `tasklist` is genuinely absent. + run env SCRIPTS="$SCRIPTS" PATH="$notl:/usr/bin:/bin" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/gone3.sh" "$live" + grep -qF 'gone rc=1' <<<"$output" + + kill "$live" 2>/dev/null || true +} + +@test "compat_signal_pid_tree: the WINPID is taken BEFORE the signal (#831)" { + # THE ORDER OF TWO LINES, ON ITS OWN. + # + # `ps` is what turns a pid into a WINPID, and it stops answering once the MSYS + # side has exited. So a `kill` sent first can remove the only means of naming + # the native process still running underneath -- which is not a hypothetical, + # it is the reported symptom: the kill returns and node.exe is still there. + # + # Called with NO winpid argument deliberately. The reap resolves one while it + # can still prove ownership and passes it in, and that defends this function + # from its own ordering -- so a case that supplies it cannot see the bug. This + # one leaves the function to resolve it (raised in review on #840). + stub_windows + local live + sleep 30 & live=$! + win_mark_alive "$live" + + cat > "$TEST_SKILL_DIR/order.sh" <<'EOF_ORDER' +#!/usr/bin/env bash +. "$SCRIPTS/lib/instance-id.sh" +. "$SCRIPTS/lib/compat.sh" +compat_signal_pid_tree "$1" TERM +EOF_ORDER + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/order.sh" "$live" + [ "$status" -eq 0 ] + + # The MSYS side is gone -- the POSIX signal did that, and after it `ps` has no + # row to read a WINPID from. + local i=0 + while kill -0 "$live" 2>/dev/null && [ "$i" -lt 200 ]; do i=$((i + 1)); sleep 0.01; done + run kill -0 "$live" + [ "$status" -ne 0 ] + + # And the native side went WITH it, by the right name. + [ -f "$TASKKILL_LOG" ] + grep -qF "/PID $((900000 + live)) /T" "$TASKKILL_LOG" + [ ! -e "$ALIVE_DIR/$((900000 + live))" ] +} + +@test "compat_pid_gone: a tasklist that FAILS is not a tasklist that said gone (#831)" { + # The third way of not being able to ask, and the one that looks most like an + # answer: the probe is present and runs and exits non-zero. Asserted separately + # from "absent", because they are two different lines. + stub_windows + local broken="$TEST_SKILL_DIR/broken-tasklist" + mkdir -p "$broken" + cp "$WINSTUB/uname" "$WINSTUB/ps" "$broken/" + printf '%s\n' '#!/usr/bin/env bash' 'exit 1' > "$broken/tasklist" + chmod +x "$broken/uname" "$broken/ps" "$broken/tasklist" + local live + sleep 30 & live=$! + + cat > "$TEST_SKILL_DIR/gone4.sh" <<'EOF_GONE4' +#!/usr/bin/env bash +. "$SCRIPTS/lib/instance-id.sh" +. "$SCRIPTS/lib/compat.sh" +_agmsg_pid_alive_local() { return 1; } +rc=0 +compat_pid_gone "$1" || rc=$? +echo "gone rc=$rc" +EOF_GONE4 + run env SCRIPTS="$SCRIPTS" PATH="$broken:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/gone4.sh" "$live" + grep -qF 'gone rc=1' <<<"$output" + + kill "$live" 2>/dev/null || true +} + +@test "compat_pid_gone: without instance-id.sh it refuses rather than answering gone (#831)" { + # A missing dependency exits 127, which is not 0, which fell straight through + # to "gone" -- the one answer this function exists to make hard to reach. + cat > "$TEST_SKILL_DIR/nodep.sh" <<'EOF_NODEP' +#!/usr/bin/env bash +. "$SCRIPTS/lib/compat.sh" +rc=0 +compat_pid_gone 4242 || rc=$? +echo "gone rc=$rc" +EOF_NODEP + run env SCRIPTS="$SCRIPTS" bash "$TEST_SKILL_DIR/nodep.sh" + grep -qF 'gone rc=1' <<<"$output" + grep -qF 'needs lib/instance-id.sh sourced' <<<"$output" +} + +@test "sync start: with an honest probe the engine is stopped and its records cleared (#831)" { + # The ordinary path, unstubbed, so the Windows work above cannot have broken + # the thing that already worked. + # + # The shipped ceiling is NOT exercised here any more -- it cost 52 seconds, half + # this file, and the case below pins the same constant for nothing. + write_driver + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$DRIVER" "$TEAM" honest + + grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" + local running + running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" + [ "$running" -eq 0 ] + [ ! -f "$PIDFILE" ] +} + +@test "sync start: when it cannot stop it, it keeps the record and says so (#831)" { + # THE OTHER HALF. A reap that fails means an orphan exists, and the record is + # the only thing that names it -- so the failure path must not clear it, and + # must not be silent about which situation the operator is in. + # + # Driven by making the engine unkillable from the driver: the signal helper is + # replaced with one that does nothing, which is what a sandbox that refuses to + # signal looks like from in here (#730 measured that on Codex). + DRIVER="$TEST_SKILL_DIR/drive-unkillable.sh" + cat > "$DRIVER" <<'EOF_UNKILL' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +compat_signal_pid_tree() { return 0; } # every signal silently goes nowhere +rc=0 +cmd_sync_start "$1" || rc=$? +echo "driver: cmd_sync_start rc=$rc" +EOF_UNKILL + chmod +x "$DRIVER" + + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$DRIVER" "$TEAM" + grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" + + # It is still running, and it still has a name. + local running + running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" + [ "$running" -eq 1 ] + [ -f "$PIDFILE" ] + pgrep -f "$PATTERN" | grep -qxF "$(cat "$PIDFILE")" + grep -qF 'did not stop it' <<<"$output" +} + +@test "sync start: the give-up message does not name a cause it did not measure (#831)" { + # It used to say the engine could not reach the server and that nothing was + # syncing for the team. The engines this text was written for were reaching the + # server and pulling the whole time. `refute` and not `! grep`: a negated + # command cannot fail a bats test at all (#670). + DRIVER="$TEST_SKILL_DIR/drive-unkillable.sh" + cat > "$DRIVER" <<'EOF_UNKILL2' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +compat_signal_pid_tree() { return 0; } +rc=0 +cmd_sync_start "$1" || rc=$? +echo "driver: cmd_sync_start rc=$rc" +EOF_UNKILL2 + chmod +x "$DRIVER" + + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$DRIVER" "$TEAM" + refute grep -qF 'It cannot reach the server' <<<"$output" + refute grep -qF 'Nothing is syncing' <<<"$output" +} + +@test "sync start: on Windows the way out it prints is not the one that fails there (#831)" { + # `kill ` was the only manual stop offered, and #831 measured that it does + # not end the native node under the MSYS shell. Offering it as the way out + # sends the operator to do the thing that already did not work. + stub_windows + DRIVER="$TEST_SKILL_DIR/drive-unkillable-win.sh" + cat > "$DRIVER" <<'EOF_UNKILL3' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +compat_signal_pid_tree() { return 0; } +rc=0 +cmd_sync_start "$1" || rc=$? +echo "driver: cmd_sync_start rc=$rc" +EOF_UNKILL3 + chmod +x "$DRIVER" + + local out="$TEST_SKILL_DIR/winmsg.out" p i=0 + env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + AGMSG_TEST_SYNC_READY_TURNS=40 \ + bash "$DRIVER" "$TEAM" >"$out" 2>&1 & + p=$! + while [ ! -f "$PIDFILE" ] && [ "$i" -lt 400 ]; do i=$((i + 1)); sleep 0.05; done + [ -f "$PIDFILE" ] + ENGINE="$(cat "$PIDFILE")" + win_mark_alive "$ENGINE" + wait "$p" 2>/dev/null || true + + grep -qF "taskkill /PID $((900000 + ENGINE)) /T /F" "$out" + # And it names why, so the next reader does not put `kill` back. + grep -qF 'not the node process under it' "$out" +} + +@test "reap: a live pid whose cmdline is another process is never signalled (#831)" { + # THE OTHER HALF OF THE ASYMMETRY, and the one that stops it from becoming a + # new defect. Overriding "gone" with a second probe means more pids now reach + # the signalling code -- so what proves the pid is OURS has to be the thing + # that survives reuse, and a number does not. The moment a process exits its + # pid is reusable, and `taskkill /T` on a reused number ends a stranger's tree. + # + # Here the pidfile names a process that is alive on both probes and is not the + # engine. Nothing may be sent to it, by either route (raised in review on #840). + stub_windows + local other + sleep 30 & other=$! + win_mark_alive "$other" + printf '%s\n' "$other" > "$PIDFILE" + + cat > "$TEST_SKILL_DIR/reap-foreign.sh" <<'EOF_FOREIGN' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +rc=0 +_remote_sync_engine_reap_owned "$1" "$2" || rc=$? +echo "reap rc=$rc" +EOF_FOREIGN + + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ + bash "$TEST_SKILL_DIR/reap-foreign.sh" "$TEAM" "$other" + + # 1 = ownership not proven. Not 0, which would say this call stopped it, and + # not 2, which would say it is gone. + grep -qF 'reap rc=1' <<<"$output" + # Nothing was sent down the Windows route... + [ ! -f "$TASKKILL_LOG" ] + # ...and nothing down the POSIX one either: it is still running. + kill -0 "$other" + # And the record it could not act on is still there for `status` to describe. + [ -f "$PIDFILE" ] + + kill "$other" 2>/dev/null || true +} + +@test "the shipped readiness ceiling is still 1600 turns (#831)" { + # THE CONSTANT, WITHOUT PAYING FOR IT. Letting the poll run to the ceiling was + # the honest way to bind this and cost 52 seconds on a shard already at its + # 25-minute cap; the resolver answers the same two questions in milliseconds. + # + # Both halves matter and are asserted separately: unset must mean the SHIPPED + # number, and the seam must actually be honoured -- a resolver that ignored the + # variable would make every other case in this file run the full ceiling + # silently, which is how the shard timed out in the first place. + cat > "$TEST_SKILL_DIR/turns.sh" <<'EOF_TURNS' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +printf 'turns=%s\n' "$(_remote_sync_ready_turns)" +EOF_TURNS + + run env SCRIPTS="$SCRIPTS" bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=1600' <<<"$output" + + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=40' <<<"$output" + + # And a non-numeric value is not a ceiling of zero: it falls back to shipped. + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=oops bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=1600' <<<"$output" +}