From 0a7fd16c537238adf332e4b841fd6eeadbe8dad4 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 00:02:23 -0700 Subject: [PATCH 1/7] fix(remote): sync start keeps the record of an engine it did not stop Reported from the field: repeated `sync start` attempts left sync engines running that nothing pointed at. Three invocations, three live engines, and `status` reporting stopped for every one of them. The give-up path is where they came from. When the engine never becomes ready the starter reaps it and clears its records, and `_remote_sync_engine_reap_owned` answered 0 -- "reaped" -- for a pid it had only READ as gone, having signalled nothing. On Windows a live engine reads as dead (#652; #505 gives one way, a DWORD pid above the POSIX ceiling that kill(1) will not parse), so the records of a RUNNING engine were deleted. The pidfile is the only thing naming it, so from then on no `status` could see it and no `stop` could reach it, while it kept retrying on a backoff. So the reap now separates its own act from a reading: 0 it signalled the engine and the engine went 2 the engine read as gone; nothing was signalled 1 it could not stop it and `cmd_sync_start` clears the pidfile and the cycle stamp on 0 only. On 2 it leaves them and says which situation it is in: a stale record is what `status` already knows how to describe, and a live process with no record is not something the operator can act on at all. #832 fixed the probe that misread. This does not stand on that fix, because the harm does not depend on which misreading happens. Two smaller things came with it. Both call sites capture the status with `|| rc=$?` rather than calling bare -- this file runs under `set -e`, where a bare call returning non-zero exits the script instead of reaching the message below it. And the "could not retake the registry lock" line no longer says "the engine is stopped" on a path where nothing was signalled; that is the one thing that path does not establish, and the line is all the operator gets. Six cases in tests/test_sync_start_orphans.bats, driven against a real engine with the local liveness probe forced to the reading Windows produces. Each has its negative control in the file: 2 is answered for a reading, 0 for an act, records survive when blind and are cleared when honest. Closes #831 --- scripts/remote.sh | 68 +++++++-- tests/test_sync_start_orphans.bats | 217 +++++++++++++++++++++++++++++ 2 files changed, 275 insertions(+), 10 deletions(-) create mode 100644 tests/test_sync_start_orphans.bats diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de5846..ba50ac698 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 @@ -1739,11 +1749,25 @@ _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. _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") - if ! _agmsg_pid_alive_local "$owned_pid"; then return 0; fi + if ! _agmsg_pid_alive_local "$owned_pid"; then return 2; fi [ "$state" = "running" ] && [ "$pid" = "$owned_pid" ] || return 1 kill "-$signal" "$owned_pid" 2>/dev/null || true attempts=0 @@ -2519,9 +2543,19 @@ 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 + # + # AND ONLY WHEN THIS CALL ACTUALLY STOPPED IT. `_remote_sync_engine_reap_owned` + # answers 0 when it signalled the engine and the engine went, and 2 when the + # engine merely READ as gone. Clearing the records on 2 is what left orphans: + # a live engine misread as dead had its pidfile deleted, `status` then said + # stopped, and the next `sync start` added another engine beside it -- three + # invocations, three live engines (#831). On 2 the records are left, because + # a stale pidfile is something `status` describes and an operator can act on, + # while a running process with no record is not. + local relocked=1 reaped=0 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 @@ -2531,16 +2565,30 @@ cmd_sync_start() { # points at, which is the shape `set-endpoint` already warns about. local recorded recorded="$(cat "$(_remote_sync_engine_pidfile "$team")" 2>/dev/null || true)" - if [ "$recorded" = "$started_pid" ]; then + if [ "$recorded" = "$started_pid" ] && [ "$reaped" -eq 0 ]; then rm -f "$(_remote_sync_engine_pidfile "$team")" rm -f "$(_remote_sync_engine_cycle_stamp "$team")" # same reason as in _remote_sync_engine_stop fi agmsg_lock_release else echo "agmsg: could not retake the registry lock to clear the engine's records for '$team'" >&2 - echo " the engine is stopped; its pidfile is left, and 'remote.sh status' reads it as stale." >&2 + if [ "$reaped" -eq 0 ]; then + echo " the engine is stopped; its pidfile is left, and 'remote.sh status' reads it as stale." >&2 + else + # NOT "the engine is stopped". Nothing was signalled on this path, so + # whether it is running is exactly what is not known here. The line + # said it unconditionally, and a message that names the wrong state is + # worse than none: it is the only thing the operator has. + echo " its pidfile is left, and it is the only thing naming that pid." >&2 + fi fi echo "agmsg: sync engine for '$team' did not become ready" >&2 + if [ "$reaped" -eq 2 ]; then + echo "agmsg: pid $started_pid read as already gone, so nothing was signalled." >&2 + echo "agmsg: its records are left in place: if that reading was wrong the" >&2 + echo "agmsg: engine is still running, and this is the only thing naming it." >&2 + echo "agmsg: 'remote.sh status $(agmsg_shq "$team")' reads them." >&2 + fi return 1 fi [ "$relocked" -eq 1 ] && agmsg_lock_release diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats new file mode 100644 index 000000000..eaf6118ba --- /dev/null +++ b/tests/test_sync_start_orphans.bats @@ -0,0 +1,217 @@ +#!/usr/bin/env bats + +# `sync start` must not walk away from a process it started (#831). +# +# Reported from the field: repeated `sync start` attempts left engines running +# that nothing pointed at. Three invocations, three live engines, and `status` +# reporting stopped for all of them. +# +# The mechanism is in the give-up path. When the engine never becomes ready the +# starter reaps it and clears its records -- and the reap answered "reaped" for a +# pid it had merely READ as gone, without signalling anything. On Windows a live +# engine reads as dead (#652, #505: a DWORD pid above the POSIX ceiling is not a +# number `kill` will parse), so the records of a RUNNING engine were deleted. The +# pidfile is the only thing that names it, so after that no `status` could see it +# and no `stop` could reach it -- while it kept retrying on a backoff. +# +# The separation these cases hold is between an act and a reading: +# +# 0 this call signalled the engine and the engine went +# 2 the engine read as gone; nothing was signalled +# +# and the records are cleared on 0 only. #832 fixed the probe that misread; this +# does not depend on that fix, because the harm does not depend on WHICH +# misreading happens. A record deleted for a live process cannot be recovered by +# the operator. A record left for a dead one is exactly what `status` describes. + +load test_helper + +# A team name of this file's own, because these cases COUNT AND KILL processes +# by their `--team` argument, which is the only part of an engine's argv that is +# not shared (the store is passed in the environment). Scoping to $TEST_SKILL_DIR +# would not work; a name nobody else uses does. +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" + # Nothing of this name may exist yet, or every count below means nothing. + [ -z "$(pgrep -f "$PATTERN")" ] +} + +teardown() { + # This file's cases exist BECAUSE an engine can be left running, so it cannot + # rely on the code under test to clean up after them. + pkill -f "$PATTERN" 2>/dev/null || true + teardown_test_env +} + +# Drives the real `cmd_sync_start` with the local liveness probe made to answer +# "gone" for everything -- the reading Windows produces for a live engine. +# +# A driver script and not `run bash -c`, so the override is written once and the +# only difference between the two callers below is its presence. +write_driver() { + DRIVER="$TEST_SKILL_DIR/drive-sync-start.sh" + cat > "$DRIVER" <<'EOF_DRIVER' +#!/usr/bin/env bash +# $1 = team, $2 = "blind" to make the local liveness probe read every pid as gone +. "$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" +} + +@test "reap: a pid that only READS as gone is answered 2, and nothing is signalled (#831)" { + # 2 is the whole distinction. A dead pid gives the same reading a live engine + # gives on Windows, and the answer must not be the one that means "I stopped + # it" -- the caller clears records on that answer. + local dead + bash -c 'exit 0' & dead=$! + wait "$dead" 2>/dev/null || true + + cat > "$TEST_SKILL_DIR/reap.sh" <<'EOF_REAP' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +rc=0 +_remote_sync_engine_reap_owned "$1" "$2" || rc=$? +echo "reap rc=$rc" +EOF_REAP + + run bash "$TEST_SKILL_DIR/reap.sh" "$TEAM" "$dead" + [ "$status" -eq 0 ] + grep -qF 'reap rc=2' <<<"$output" +} + +@test "reap: a pid it actually signals is answered 0 (#831)" { + # THE NEGATIVE CONTROL. Without it "always answer 2" satisfies the case above, + # and no record would ever be cleared again. + local live + sleep 30 & live=$! + + # The reap refuses a pid it cannot prove is this team's, so the records have to + # name it -- which is also the state the real give-up path is in. + printf '%s\n' "$live" > "$PIDFILE" + cat > "$TEST_SKILL_DIR/reap0.sh" <<'EOF_REAP0' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +# The engine here is a `sleep`, not the real script, so the cmdline half of the +# status probe cannot match. Ownership is what this case is not about; the +# liveness probe underneath is the real one, and it is what says the kill landed. +LIVE="$2" +_remote_sync_engine_status() { printf 'running\t%s\n' "$LIVE"; } +rc=0 +_remote_sync_engine_reap_owned "$1" "$LIVE" || rc=$? +echo "reap rc=$rc" +EOF_REAP0 + + run bash "$TEST_SKILL_DIR/reap0.sh" "$TEAM" "$live" + [ "$status" -eq 0 ] + grep -qF 'reap rc=0' <<<"$output" + # It really went: 0 is an act, and the act has to have happened. + run kill -0 "$live" + [ "$status" -ne 0 ] +} + +@test "sync start: a blind liveness probe leaves the engine RECORDED, not orphaned (#831)" { + # THE DEFECT ITSELF, end to end. The engine is real, the give-up path is real; + # only the liveness reading is forced, to the value Windows produces. + write_driver + run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind + + # It failed to become ready, which is the path under test. + grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" + + # The engine is still running -- the reap never signalled it, because it read + # it as gone. + local running + running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" + [ "$running" -eq 1 ] + + # AND IT IS STILL REACHABLE. This is the assertion the issue is about: the + # pidfile is the only thing that names that process, so it has to survive. + [ -f "$PIDFILE" ] + local recorded + recorded="$(cat "$PIDFILE")" + pgrep -f "$PATTERN" | grep -qxF "$recorded" +} + +@test "sync start: it says the engine read as gone and was not signalled (#831)" { + # A record left behind with no explanation reads as a bug. The operator has to + # be told which of the two situations they are in, because the actions differ. + write_driver + run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind + + grep -qF 'read as already gone, so nothing was signalled' <<<"$output" + grep -qF "remote.sh status" <<<"$output" +} + +@test "sync start: with an honest probe the engine IS stopped and its records cleared (#831)" { + # THE NEGATIVE CONTROL FOR THE CASE ABOVE. Without it, "never clear anything" + # passes both, and every failed start would leave a pidfile naming nothing -- + # which is the state `status` then reports as stale forever. + write_driver + run env SCRIPTS="$SCRIPTS" 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: a jammed lock on the blind path does not claim the engine stopped (#831)" { + # THE ONE MESSAGE THAT IS NOT ABOUT A FILE, AND THE ONLY THING THE OPERATOR GETS. + # + # Two conditions have to hold at once: the cleanup cannot retake the lock, and + # the reap only READ the engine as gone. The line printed there used to say + # "the engine is stopped" unconditionally, which on this path is the one thing + # nothing established -- nothing was signalled. + # + # The lock is taken the way the library takes it, after the engine exists, so + # the starter has already let go of it and finds it held on the way back. + write_driver + local lock="$TEST_SKILL_DIR/teams/$TEAM/.config.lock" + local err="$TEST_SKILL_DIR/jammed.err" + local driver_pid i=0 + + env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind >"$err" 2>&1 & + driver_pid=$! + while [ ! -f "$PIDFILE" ] && [ "$i" -lt 400 ]; do i=$((i + 1)); sleep 0.05; done + [ -f "$PIDFILE" ] + mkdir "$lock" + + wait "$driver_pid" 2>/dev/null || true + + grep -qF 'could not retake the registry lock' "$err" + grep -qF 'the only thing naming that pid' "$err" + # And it does NOT say the thing it cannot know. Asserted separately, because a + # message can gain a true sentence and keep the false one. + ! grep -qF 'the engine is stopped' "$err" + [ -f "$PIDFILE" ] + + rmdir "$lock" 2>/dev/null || true +} From 6ecfec4f5d0c24d23cb68c1a37215edd2e847125 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 00:03:42 -0700 Subject: [PATCH 2/7] docs(tests): say which half of #831 this covers, and which it does not The previous commit ended with `Closes #831`, and that was wrong. #831 names two independent directions and this branch is the first of them. The second -- every engine appending to one shared log, so lines tear into each other and every tool that reads that log reads fragments -- is untouched, and no case in the file says anything about it. The keyword does not fire from `integration/remote` anyway, so nothing was closed; the claim was still in the record, where the next reader would have believed it. It also does not reduce the number of engines that can exist. Two `sync start` calls that both misread still start two. What it stops is the second half of the harm, where the record of one is deleted and nothing can name it after. The scope now sits in the test file's own header rather than only in a PR body, because that is where someone reads it when they come looking for a case that is not there. Refs #831 --- tests/test_sync_start_orphans.bats | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats index eaf6118ba..9d529111e 100644 --- a/tests/test_sync_start_orphans.bats +++ b/tests/test_sync_start_orphans.bats @@ -23,6 +23,15 @@ # does not depend on that fix, because the harm does not depend on WHICH # misreading happens. A record deleted for a live process cannot be recovered by # the operator. A record left for a dead one is exactly what `status` describes. +# +# WHAT THIS FILE DOES NOT COVER. #831 names two independent directions and this +# is the first of them. The second -- every engine appending to one shared log, +# so lines tear into each other and every tool that reads that log, including the +# readiness poll, reads fragments -- is untouched here, and no case below says +# anything about it. Nothing here reduces the number of engines that CAN exist +# either: two `sync start` calls that both misread still start two. What it +# stops is the second half, where the record of one is deleted and nothing can +# name it afterwards. load test_helper From bfea8e3e235d5cbbdec04ec7d0014b310f02a60b Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 02:04:47 -0700 Subject: [PATCH 3/7] fix(remote): reap the engine on Windows, where the kill did not reach it The previous two commits on this branch hardened the give-up path -- an engine that could not be stopped kept its record instead of becoming anonymous -- and called that direction 1 of #831. Review was right that it is not. Direction 1 asks that the engine be reaped before failure is reported, and that the reap work on Windows, which is where every measured orphan came from. Keeping a name on an orphan is a smaller claim than not leaving one. Two things were missing, and they are the same shape: a reading was allowed to end the matter. "Gone" was one probe's answer. Under Git Bash `kill -0` cannot see a live engine (#652), so `_remote_sync_engine_status` reported `stale` before it ever reached the check that proves ownership, and the reap declined to signal a pid it could not confirm. `compat_pid_gone` now requires every probe available to agree, and on msys asks the Windows side about the WINPID -- not the MSYS pid, which `tasklist` does not report at all and which answered "dead" for every running codex bridge in #567. And the signal was `kill` alone. The engine runs as `bash remote-sync.sh`, which runs `node`; the MSYS signal reaches the shell and the native node.exe under it survives, which is how the engines in #831 outlived a kill already aimed at them. `compat_signal_pid_tree` sends the POSIX signal first and unconditionally, then on msys ends the tree by pid: `taskkill /PID /T`, with `/F` only on the second pass, after the polite attempt has been made and waited on. Ownership is unchanged, and deliberately so. A pid stops naming the same process the moment that process exits, and `/T` on a recycled number ends a stranger's tree; overriding a false "gone" brings more pids to that check, so the check has to be the one that survives reuse. It is the cmdline, re-read every pass, and an unproven pid is still reported as "could not stop" rather than signalled. That leaves the caller simple: the records are cleared when the process is not there, kept when it could not be stopped, and the difference is reported. Two diagnostics said more than was measured. The give-up message asserted the engine could not reach the server and that nothing was syncing for the team -- the engines it was written for were reaching the server and pulling the whole time. And the only manual way out it offered was `kill `, which #831 measured does not end that process; on msys it now prints the taskkill form with the WINPID, and says why. The readiness poll's ceiling gains a test seam, defaulting to the shipped 1600 turns. Reaching the give-up path costs the whole ceiling, and the regressions that drive it are not about that number; one case leaves the seam unset and asserts the sixteen-second floor, so the default stays on the path of something. Ten cases in tests/test_sync_start_orphans.bats, ten mutations, each landing on its own assertion. The Windows cases drive the Windows route on a POSIX host -- `uname`, `ps`, `tasklist`, `taskkill` and `powershell.exe` stubbed to the shapes Git Bash speaks, with the ownership cmdline coming from the real process. That measures that the route is chosen and that the engine dies through it. It is not a measurement of native node.exe dying on real Windows; the Windows machine is asked for that. MSYSTEM alone does not select the platform -- `_agmsg_detect_platform` asks `uname -s` -- so the first version of these cases passed without ever entering the branch under test. Refs #831 --- scripts/lib/compat.sh | 77 +++++ scripts/remote.sh | 99 ++++-- tests/test_sync_start_orphans.bats | 502 +++++++++++++++++++++-------- 3 files changed, 514 insertions(+), 164 deletions(-) diff --git a/scripts/lib/compat.sh b/scripts/lib/compat.sh index 99c92370b..822ef0245 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,77 @@ _compat_cim_cmdline() { | tr -d '\r' | tr '\\' '/' } +# Is this process gone? Answered by EVERY probe available, not by one. +# +# 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. +# +# Under Git Bash the pid these shells minted is an MSYS pid, which `tasklist` +# does not report at all -- asking it about one answers "dead" for a running +# process, which is how #567 lost every codex bridge -- so the Windows side is +# asked about the WINPID instead, which is the same process under the name that +# subsystem uses. +# +# `_agmsg_pid_alive_local` lives in instance-id.sh, which most callers of this +# file do not source. Its absence must not be answerable: an undefined function +# exits 127, which is not 0, which would have fallen straight through to "gone" -- +# the one answer this whole function exists to make hard to reach. +compat_pid_gone() { + local pid="$1" winpid + 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 + if [ "$_agmsg_platform" = "msys" ]; then + winpid="$(_compat_get_winpid "$pid" 2>/dev/null || true)" + case "$winpid" in + ''|*[!0-9]*) ;; + *) MSYS_NO_PATHCONV=1 tasklist /FI "PID eq $winpid" 2>/dev/null | grep -q "$winpid" && return 1 ;; + esac + fi + 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 POSIX signal goes first and unconditionally, including on msys: it is the +# only thing that reaches the MSYS-side process, and everywhere else it is the +# whole story. 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 + kill "-$sig" "$pid" 2>/dev/null || true + _agmsg_detect_platform + [ "$_agmsg_platform" = "msys" ] || return 0 + winpid="$(_compat_get_winpid "$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 ba50ac698..8b1b76029 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1731,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 @@ -1766,18 +1774,25 @@ _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 + # 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 2; 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 + compat_signal_pid_tree "$owned_pid" "$signal" attempts=0 while [ "$attempts" -lt 100 ]; do - _agmsg_pid_alive_local "$owned_pid" || return 0 + compat_pid_gone "$owned_pid" && return 0 attempts=$((attempts + 1)) sleep 0.01 done done - ! _agmsg_pid_alive_local "$owned_pid" + compat_pid_gone "$owned_pid" } # Upgrade a team that predates local ids: mint a team_id AND a member_id for @@ -2514,7 +2529,17 @@ 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=1600 + case "${AGMSG_TEST_SYNC_READY_TURNS:-}" in + ''|*[!0-9]*) ;; + *) ready_turns="$AGMSG_TEST_SYNC_READY_TURNS" ;; + esac + 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 | @@ -2544,15 +2569,12 @@ cmd_sync_start() { # the failure is reported and the files are left rather than removed blind: # a stale pidfile is what `status` already knows how to describe. # - # AND ONLY WHEN THIS CALL ACTUALLY STOPPED IT. `_remote_sync_engine_reap_owned` - # answers 0 when it signalled the engine and the engine went, and 2 when the - # engine merely READ as gone. Clearing the records on 2 is what left orphans: - # a live engine misread as dead had its pidfile deleted, `status` then said - # stopped, and the next `sync start` added another engine beside it -- three - # invocations, three live engines (#831). On 2 the records are left, because - # a stale pidfile is something `status` describes and an operator can act on, - # while a running process with no record is not. - local relocked=1 reaped=0 + # 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 _remote_sync_engine_reap_owned "$team" "$started_pid" || reaped=$? # `set -e`: see _remote_sync_engine_stop if [ "$reaped" -eq 0 ] || [ "$reaped" -eq 2 ]; then @@ -2565,30 +2587,16 @@ cmd_sync_start() { # points at, which is the shape `set-endpoint` already warns about. local recorded recorded="$(cat "$(_remote_sync_engine_pidfile "$team")" 2>/dev/null || true)" - if [ "$recorded" = "$started_pid" ] && [ "$reaped" -eq 0 ]; then + if [ "$recorded" = "$started_pid" ]; then rm -f "$(_remote_sync_engine_pidfile "$team")" rm -f "$(_remote_sync_engine_cycle_stamp "$team")" # same reason as in _remote_sync_engine_stop fi agmsg_lock_release else echo "agmsg: could not retake the registry lock to clear the engine's records for '$team'" >&2 - if [ "$reaped" -eq 0 ]; then - echo " the engine is stopped; its pidfile is left, and 'remote.sh status' reads it as stale." >&2 - else - # NOT "the engine is stopped". Nothing was signalled on this path, so - # whether it is running is exactly what is not known here. The line - # said it unconditionally, and a message that names the wrong state is - # worse than none: it is the only thing the operator has. - echo " its pidfile is left, and it is the only thing naming that pid." >&2 - fi + echo " the engine is stopped; its pidfile is left, and 'remote.sh status' reads it as stale." >&2 fi echo "agmsg: sync engine for '$team' did not become ready" >&2 - if [ "$reaped" -eq 2 ]; then - echo "agmsg: pid $started_pid read as already gone, so nothing was signalled." >&2 - echo "agmsg: its records are left in place: if that reading was wrong the" >&2 - echo "agmsg: engine is still running, and this is the only thing naming it." >&2 - echo "agmsg: 'remote.sh status $(agmsg_shq "$team")' reads them." >&2 - fi return 1 fi [ "$relocked" -eq 1 ] && agmsg_lock_release @@ -2621,16 +2629,37 @@ 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." + echo " pid $started_pid is still running." + # 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_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats index 9d529111e..f87db6aa1 100644 --- a/tests/test_sync_start_orphans.bats +++ b/tests/test_sync_start_orphans.bats @@ -2,43 +2,47 @@ # `sync start` must not walk away from a process it started (#831). # -# Reported from the field: repeated `sync start` attempts left engines running -# that nothing pointed at. Three invocations, three live engines, and `status` -# reporting stopped for all of them. +# 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. # -# The mechanism is in the give-up path. When the engine never becomes ready the -# starter reaps it and clears its records -- and the reap answered "reaped" for a -# pid it had merely READ as gone, without signalling anything. On Windows a live -# engine reads as dead (#652, #505: a DWORD pid above the POSIX ceiling is not a -# number `kill` will parse), so the records of a RUNNING engine were deleted. The -# pidfile is the only thing that names it, so after that no `status` could see it -# and no `stop` could reach it -- while it kept retrying on a backoff. +# 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 separation these cases hold is between an act and a reading: +# 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. # -# 0 this call signalled the engine and the engine went -# 2 the engine read as gone; nothing was signalled -# -# and the records are cleared on 0 only. #832 fixed the probe that misread; this -# does not depend on that fix, because the harm does not depend on WHICH -# misreading happens. A record deleted for a live process cannot be recovered by -# the operator. A record left for a dead one is exactly what `status` describes. +# 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 of them. The second -- every engine appending to one shared log, -# so lines tear into each other and every tool that reads that log, including the -# readiness poll, reads fragments -- is untouched here, and no case below says -# anything about it. Nothing here reduces the number of engines that CAN exist -# either: two `sync start` calls that both misread still start two. What it -# stops is the second half, where the record of one is deleted and nothing can -# name it afterwards. +# 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, which is the only part of an engine's argv that is -# not shared (the store is passed in the environment). Scoping to $TEST_SKILL_DIR -# would not work; a name nobody else uses does. +# 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() { @@ -62,27 +66,131 @@ setup() { PIDFILE="$TEST_SKILL_DIR/run/remote-sync.$TEAM.pid" PATTERN="remote-sync.mjs run --team $TEAM" - # Nothing of this name may exist yet, or every count below means nothing. + # 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's cases exist BECAUSE an engine can be left running, so it cannot - # rely on the code under test to clean up after them. + # 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 } -# Drives the real `cmd_sync_start` with the local liveness probe made to answer -# "gone" for everything -- the reading Windows produces for a live engine. +# A Git-Bash-shaped Windows side, on this host. +# +# 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. # -# A driver script and not `run bash -c`, so the override is written once and the -# only difference between the two callers below is its presence. +# `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. +if [ "$1" = "-l" ] && [ "$2" = "-p" ]; then + 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 = "blind" to make the local liveness probe read every pid as gone +# $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; } @@ -94,133 +202,269 @@ EOF_DRIVER chmod +x "$DRIVER" } -@test "reap: a pid that only READS as gone is answered 2, and nothing is signalled (#831)" { - # 2 is the whole distinction. A dead pid gives the same reading a live engine - # gives on Windows, and the answer must not be the one that means "I stopped - # it" -- the caller clears records on that answer. - local dead - bash -c 'exit 0' & dead=$! - wait "$dead" 2>/dev/null || true +# 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_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")" +} - cat > "$TEST_SKILL_DIR/reap.sh" <<'EOF_REAP' -#!/usr/bin/env bash -. "$SCRIPTS/remote.sh" -rc=0 -_remote_sync_engine_reap_owned "$1" "$2" || rc=$? -echo "reap rc=$rc" -EOF_REAP +@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 - run bash "$TEST_SKILL_DIR/reap.sh" "$TEAM" "$dead" - [ "$status" -eq 0 ] - grep -qF 'reap rc=2' <<<"$output" + 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 "reap: a pid it actually signals is answered 0 (#831)" { - # THE NEGATIVE CONTROL. Without it "always answer 2" satisfies the case above, - # and no record would ever be cleared again. +@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" +} + +@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" - # The reap refuses a pid it cannot prove is this team's, so the records have to - # name it -- which is also the state the real give-up path is in. - printf '%s\n' "$live" > "$PIDFILE" - cat > "$TEST_SKILL_DIR/reap0.sh" <<'EOF_REAP0' + cat > "$TEST_SKILL_DIR/sig.sh" <<'EOF_SIG' #!/usr/bin/env bash -. "$SCRIPTS/remote.sh" -# The engine here is a `sleep`, not the real script, so the cmdline half of the -# status probe cannot match. Ownership is what this case is not about; the -# liveness probe underneath is the real one, and it is what says the kill landed. -LIVE="$2" -_remote_sync_engine_status() { printf 'running\t%s\n' "$LIVE"; } -rc=0 -_remote_sync_engine_reap_owned "$1" "$LIVE" || rc=$? -echo "reap rc=$rc" -EOF_REAP0 - - run bash "$TEST_SKILL_DIR/reap0.sh" "$TEAM" "$live" +. "$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 ] - grep -qF 'reap rc=0' <<<"$output" - # It really went: 0 is an act, and the act has to have happened. + [ ! -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 "sync start: a blind liveness probe leaves the engine RECORDED, not orphaned (#831)" { - # THE DEFECT ITSELF, end to end. The engine is real, the give-up path is real; - # only the liveness reading is forced, to the value Windows produces. - write_driver - run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind - - # It failed to become ready, which is the path under test. - grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" +@test "compat_pid_gone: one probe saying gone is not enough (#831)" { + # The property the rest of it stands on, on its own. The POSIX probe says gone; + # the Windows side says alive; the answer must be "not gone". + stub_windows + local pid=4242 + win_mark_alive "$pid" - # The engine is still running -- the reap never signalled it, because it read - # it as gone. - local running - running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" - [ "$running" -eq 1 ] + 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 \ + bash "$TEST_SKILL_DIR/gone.sh" "$pid" + grep -qF 'gone rc=1' <<<"$output" - # AND IT IS STILL REACHABLE. This is the assertion the issue is about: the - # pidfile is the only thing that names that process, so it has to survive. - [ -f "$PIDFILE" ] - local recorded - recorded="$(cat "$PIDFILE")" - pgrep -f "$PATTERN" | grep -qxF "$recorded" + # NEGATIVE CONTROL, in the same case: with the Windows side also saying gone, + # the answer flips. Otherwise "never gone" would pass the assertion above. + rm -f "$ALIVE_DIR/$((900000 + pid))" + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + bash "$TEST_SKILL_DIR/gone.sh" "$pid" + grep -qF 'gone rc=0' <<<"$output" } -@test "sync start: it says the engine read as gone and was not signalled (#831)" { - # A record left behind with no explanation reads as a bug. The operator has to - # be told which of the two situations they are in, because the actions differ. - write_driver - run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind - - grep -qF 'read as already gone, so nothing was signalled' <<<"$output" - grep -qF "remote.sh status" <<<"$output" +@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 NEGATIVE CONTROL FOR THE CASE ABOVE. Without it, "never clear anything" - # passes both, and every failed start would leave a pidfile naming nothing -- - # which is the state `status` then reports as stale forever. +@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. + # + # AND THE ONLY CASE THAT LEAVES THE READINESS CEILING ALONE. The others set + # AGMSG_TEST_SYNC_READY_TURNS, because reaching the give-up path costs the full + # 1600 turns and none of them are about that number. This one pays it, so the + # shipped default is on the path of something. write_driver + local began ended + began="$(date +%s)" run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" honest + ended="$(date +%s)" grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" + # AND THE SHIPPED CEILING IS STILL THE SHIPPED ONE. The seam defaults to 1600 + # turns of a 0.01s sleep, so this path cannot return in under sixteen seconds. + # Asserted on the floor the sleep puts there, not on how long the forks take. + # Without this, the seam's default could be lowered and nothing would notice. + [ "$((ended - began))" -ge 15 ] local running running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" [ "$running" -eq 0 ] [ ! -f "$PIDFILE" ] } -@test "sync start: a jammed lock on the blind path does not claim the engine stopped (#831)" { - # THE ONE MESSAGE THAT IS NOT ABOUT A FILE, AND THE ONLY THING THE OPERATOR GETS. - # - # Two conditions have to hold at once: the cleanup cannot retake the lock, and - # the reap only READ the engine as gone. The line printed there used to say - # "the engine is stopped" unconditionally, which on this path is the one thing - # nothing established -- nothing was signalled. +@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. # - # The lock is taken the way the library takes it, after the engine exists, so - # the starter has already let go of it and finds it held on the way back. - write_driver - local lock="$TEST_SKILL_DIR/teams/$TEAM/.config.lock" - local err="$TEST_SKILL_DIR/jammed.err" - local driver_pid i=0 + # 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" - env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" blind >"$err" 2>&1 & - driver_pid=$! + 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_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" ] - mkdir "$lock" + 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 - wait "$driver_pid" 2>/dev/null || true + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + bash "$TEST_SKILL_DIR/reap-foreign.sh" "$TEAM" "$other" - grep -qF 'could not retake the registry lock' "$err" - grep -qF 'the only thing naming that pid' "$err" - # And it does NOT say the thing it cannot know. Asserted separately, because a - # message can gain a true sentence and keep the false one. - ! grep -qF 'the engine is stopped' "$err" + # 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" ] - rmdir "$lock" 2>/dev/null || true + kill "$other" 2>/dev/null || true } From 10bca8391748e41b0edb454951e3bf1d4605e140 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 02:57:08 -0700 Subject: [PATCH 4/7] fix(compat): a probe that cannot answer is not a probe that said gone Two defects in the previous commit, both found in review, and both the same shape as the bug this branch exists to fix. `compat_pid_gone` let every way of failing to reach the Windows side fall through to "gone": no WINPID, no `tasklist`, a `tasklist` that ran and exited non-zero. So a single POSIX false-gone was enough to declare a live native engine dead -- which is #652's collapse, rebuilt inside the function written to prevent it. Each of those now answers "not gone", `tasklist`'s exit status is checked and not only its text, and the caller's cmdline check is what still keeps a dead pid from reading as a running engine. `compat_signal_pid_tree` sent the POSIX signal and then looked up the WINPID. `ps -l -p ` stops answering once the MSYS side has exited, so the kill could remove the only means of naming the native process still running underneath -- the reported symptom, "the kill returns and node.exe is still there", produced by the order of two lines. The mapping is now resolved before the signal, and `_remote_sync_engine_reap_owned` resolves it once while the cmdline check still proves the process is its own, then carries that same mapping through the signal, the taskkill and the confirmation that it went. The platform decision in `compat_signal_pid_tree` was made twice -- a guard on the lookup and another before the taskkill -- so either could be removed with nothing to show for it, and no control held it. It is made once now. The fixture hid the ordering defect: its `ps` returned `900000 + pid` whatever the process state, so the mapping was immortal and the code could always find a WINPID no matter when it asked. It answers only for a pid that is still there, which is what MSYS2's `ps` does. Four cases were missing and none of them were reachable through the reap: - The ordering, driven on `compat_signal_pid_tree` directly with no WINPID passed in. Every case that reaches it through the reap is blind to this by construction, because the reap supplies the mapping and thereby defends the function from its own order. Restoring the old order left twelve cases green. - "Cannot ask" split by which line failed: no WINPID, no `tasklist`, and a `tasklist` present that exits non-zero -- that last being the one that most looks like an answer. Fourteen cases, fourteen mutations, every case reddened by at least one and eight landing on exactly one. Refs #831 --- scripts/lib/compat.sh | 78 +++++++++----- scripts/remote.sh | 17 ++- tests/test_sync_start_orphans.bats | 165 +++++++++++++++++++++++++++-- 3 files changed, 221 insertions(+), 39 deletions(-) diff --git a/scripts/lib/compat.sh b/scripts/lib/compat.sh index 822ef0245..79e804fd2 100644 --- a/scripts/lib/compat.sh +++ b/scripts/lib/compat.sh @@ -63,7 +63,7 @@ _compat_cim_cmdline() { | tr -d '\r' | tr '\\' '/' } -# Is this process gone? Answered by EVERY probe available, not by one. +# 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 @@ -72,31 +72,42 @@ _compat_cim_cmdline() { # 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 answers "dead" for a running -# process, which is how #567 lost every codex bridge -- so the Windows side is -# asked about the WINPID instead, which is the same process under the name that -# subsystem uses. +# 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: an undefined function -# exits 127, which is not 0, which would have fallen straight through to "gone" -- -# the one answer this whole function exists to make hard to reach. +# 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 + 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 - if [ "$_agmsg_platform" = "msys" ]; then - winpid="$(_compat_get_winpid "$pid" 2>/dev/null || true)" - case "$winpid" in - ''|*[!0-9]*) ;; - *) MSYS_NO_PATHCONV=1 tasklist /FI "PID eq $winpid" 2>/dev/null | grep -q "$winpid" && return 1 ;; - esac - fi + [ "$_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 } @@ -110,22 +121,37 @@ compat_pid_gone() { # 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 POSIX signal goes first and unconditionally, including on msys: it is the -# only thing that reaches the MSYS-side process, and everywhere else it is the -# whole story. 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. +# 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 - kill "-$sig" "$pid" 2>/dev/null || true + 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 - [ "$_agmsg_platform" = "msys" ] || return 0 - winpid="$(_compat_get_winpid "$pid" 2>/dev/null || true)" + 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 ;; diff --git a/scripts/remote.sh b/scripts/remote.sh index 8b1b76029..2c338fbe3 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1772,7 +1772,7 @@ _remote_sync_engine_status() { # cannot be recovered by the operator, while a stale record is what `status` # already knows how to describe. _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. # @@ -1784,15 +1784,24 @@ _remote_sync_engine_reap_owned() { IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") if compat_pid_gone "$owned_pid"; then return 2; fi [ "$state" = "running" ] && [ "$pid" = "$owned_pid" ] || return 1 - compat_signal_pid_tree "$owned_pid" "$signal" + # 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 - compat_pid_gone "$owned_pid" && return 0 + compat_pid_gone "$owned_pid" "$owned_winpid" && return 0 attempts=$((attempts + 1)) sleep 0.01 done done - compat_pid_gone "$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 diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats index f87db6aa1..86c2a7514 100644 --- a/tests/test_sync_start_orphans.bats +++ b/tests/test_sync_start_orphans.bats @@ -118,7 +118,15 @@ EOF_UN #!/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 @@ -248,6 +256,11 @@ run_windows_driver() { [ -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)" { @@ -278,11 +291,16 @@ EOF_SIG } @test "compat_pid_gone: one probe saying gone is not enough (#831)" { - # The property the rest of it stands on, on its own. The POSIX probe says gone; - # the Windows side says alive; the answer must be "not gone". + # 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 pid=4242 - win_mark_alive "$pid" + local live + sleep 30 & live=$! + win_mark_alive "$live" cat > "$TEST_SKILL_DIR/gone.sh" <<'EOF_GONE' #!/usr/bin/env bash @@ -294,15 +312,144 @@ compat_pid_gone "$1" || rc=$? echo "gone rc=$rc" EOF_GONE run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ - bash "$TEST_SKILL_DIR/gone.sh" "$pid" + 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 answer flips. Otherwise "never gone" would pass the assertion above. - rm -f "$ALIVE_DIR/$((900000 + pid))" + # 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 \ - bash "$TEST_SKILL_DIR/gone.sh" "$pid" + 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 \ + 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 \ + 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 \ + 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 \ + 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)" { From 9117acddf21154589dd405c9e8783f84d823240e Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 18:39:05 -0700 Subject: [PATCH 5/7] fix(remote): keep the fact the operator acts on, and stop /proc deciding the test Three CI failures on the previous head, all of this branch's doing, and the first is the one worth reading. Removing the unmeasured cause from the give-up message took a MEASURED fact out with it. The line said the engine could not reach the server -- not established, correctly deleted -- and, in the same breath, that it will keep retrying on a backoff, which is the engine's own documented loop and the thing that turns "a command failed" into "something is still running on this machine". #731's test was pinning exactly that half. A deletion removes somebody's premise, and this one was not swept for readers before it was made; the test found what a grep would have. The consequence is restored, the cause is still gone. The other two were my new cases failing on ubuntu and passing on macOS, which is the shape that should have been suspicious on its own. Forcing the msys branch on a POSIX host sends `compat_get_cmdline` to `/proc//cmdline` when one exists. macOS has none, so the stubbed Windows lookup answered. Linux has one, so it short-circuited past every stub and returned the real cmdline with a TRAILING SPACE -- `tr '\0' ' '` converts the final NUL -- and the ownership check's `*" run --team "` suffix stopped matching. Ownership then could not be proven, the reap declined, and the engine survived: exactly the two assertions that failed. Measured directly rather than inferred: that suffix matches the cmdline without the trailing space and does not match it with one. So `_AGMSG_COMPAT_NO_PROC=1` now rides with `MSYSTEM` at all nine stub sites, which is what that variable exists for -- a native Windows process has no MSYS /proc entry either, so this is the fixture modelling the platform rather than the host it happens to run on. 14/14 here, and test_remote_status_liveness 31/31. --- scripts/remote.sh | 8 +++++++- tests/test_sync_start_orphans.bats | 27 ++++++++++++++++++--------- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 2c338fbe3..4985669d1 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -2638,7 +2638,13 @@ 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." + # 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 diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats index 86c2a7514..3fab4ae5e 100644 --- a/tests/test_sync_start_orphans.bats +++ b/tests/test_sync_start_orphans.bats @@ -83,6 +83,15 @@ teardown() { # 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 -- @@ -215,7 +224,7 @@ EOF_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 \ + 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=$! @@ -311,7 +320,7 @@ rc=0 compat_pid_gone "$1" || rc=$? echo "gone rc=$rc" EOF_GONE - run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + 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" @@ -319,7 +328,7 @@ EOF_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 \ + 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" @@ -349,7 +358,7 @@ rc=0 compat_pid_gone "$1" || rc=$? echo "gone rc=$rc" EOF_GONE2 - run env SCRIPTS="$SCRIPTS" PATH="$blind:$PATH" MSYSTEM=MINGW64 \ + 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" } @@ -376,7 +385,7 @@ 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 \ + 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" @@ -406,7 +415,7 @@ EOF_GONE3 . "$SCRIPTS/lib/compat.sh" compat_signal_pid_tree "$1" TERM EOF_ORDER - run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 _AGMSG_COMPAT_NO_PROC=1 \ bash "$TEST_SKILL_DIR/order.sh" "$live" [ "$status" -eq 0 ] @@ -445,7 +454,7 @@ rc=0 compat_pid_gone "$1" || rc=$? echo "gone rc=$rc" EOF_GONE4 - run env SCRIPTS="$SCRIPTS" PATH="$broken:$PATH" MSYSTEM=MINGW64 \ + 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" @@ -562,7 +571,7 @@ EOF_UNKILL3 chmod +x "$DRIVER" local out="$TEST_SKILL_DIR/winmsg.out" p i=0 - env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + 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=$! @@ -600,7 +609,7 @@ _remote_sync_engine_reap_owned "$1" "$2" || rc=$? echo "reap rc=$rc" EOF_FOREIGN - run env SCRIPTS="$SCRIPTS" PATH="$WINSTUB:$PATH" MSYSTEM=MINGW64 \ + 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 From 278df6759d92983669453b992067b66334d85dfe Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 21:00:17 -0700 Subject: [PATCH 6/7] perf(tests): stop paying 52 seconds to hold one constant The macOS 2/4 shard did not fail an assertion on the last head -- it was cancelled at the job's 25-minute cap, and this file was a large part of why. Measured per case: 102 seconds for fourteen, and 52 of them were the single case that left the readiness seam unset so the poll would run the shipped 1600 turns. That case existed to stop the default being lowered unnoticed, which is worth holding -- but not at half a test file on a shard already at its ceiling, and the sixteen-second floor it asserted was the sleep alone; the rest was 1600 forks of `ps`. The resolution moves into `_remote_sync_ready_turns`, so the constant can be asked for instead of run. The new case pins both halves separately -- unset means the shipped 1600, the seam is honoured when set -- plus a third: a non-numeric value falls back to shipped rather than to a ceiling of zero. That last one matters because a resolver that quietly returned nothing would make every other case in this file run the full ceiling, which is the shape that timed the shard out to begin with. Mutation: default changed 1600 -> 40 reddens the new case and nothing else. 39 seconds for fifteen cases, from 102 for fourteen. Behaviour is unchanged -- the shipped default is still 1600 and the seam is still test-only. --- scripts/remote.sh | 21 +++++++++++---- tests/test_sync_start_orphans.bats | 42 +++++++++++++++++++++--------- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 4985669d1..1532fa7c8 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1771,6 +1771,20 @@ _remote_sync_engine_status() { # 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 owned_winpid="" for signal in TERM KILL; do @@ -2543,11 +2557,8 @@ cmd_sync_start() { # 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=1600 - case "${AGMSG_TEST_SYNC_READY_TURNS:-}" in - ''|*[!0-9]*) ;; - *) ready_turns="$AGMSG_TEST_SYNC_READY_TURNS" ;; - esac + 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" ] && diff --git a/tests/test_sync_start_orphans.bats b/tests/test_sync_start_orphans.bats index 3fab4ae5e..2b9eb9347 100644 --- a/tests/test_sync_start_orphans.bats +++ b/tests/test_sync_start_orphans.bats @@ -480,22 +480,12 @@ EOF_NODEP # The ordinary path, unstubbed, so the Windows work above cannot have broken # the thing that already worked. # - # AND THE ONLY CASE THAT LEAVES THE READINESS CEILING ALONE. The others set - # AGMSG_TEST_SYNC_READY_TURNS, because reaching the give-up path costs the full - # 1600 turns and none of them are about that number. This one pays it, so the - # shipped default is on the path of something. + # 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 - local began ended - began="$(date +%s)" - run env SCRIPTS="$SCRIPTS" bash "$DRIVER" "$TEAM" honest - ended="$(date +%s)" + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$DRIVER" "$TEAM" honest grep -qF 'driver: cmd_sync_start rc=1' <<<"$output" - # AND THE SHIPPED CEILING IS STILL THE SHIPPED ONE. The seam defaults to 1600 - # turns of a 0.01s sleep, so this path cannot return in under sixteen seconds. - # Asserted on the floor the sleep puts there, not on how long the forks take. - # Without this, the seam's default could be lowered and nothing would notice. - [ "$((ended - began))" -ge 15 ] local running running="$(pgrep -f "$PATTERN" | wc -l | tr -d ' ')" [ "$running" -eq 0 ] @@ -624,3 +614,29 @@ EOF_FOREIGN 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" +} From a8d0210d1f8630007b450f3898d683ada03847fd Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 22:36:49 -0700 Subject: [PATCH 7/7] perf(tests): the #817 lock case does not need the shipped ceiling Measured on the cancelled macOS shard, from the gaps between consecutive `ok` lines: five tests held 12m24s of a 22.7-minute run, and this was the largest at 151 seconds. Sixth place was 18. It polls the readiness loop only to have something still running while it checks that the registry lock is free. The ceiling's value is irrelevant to it; the loop merely has to outlast a 3-second observation window. At 40 turns the starter lives about 2 seconds, which clears that, and the case passed 5 of 5 runs rather than the 1 that would have proved nothing. Only this case is changed. The other four expensive ones belong to other seats (#730, #731, and the ready-timeout child), and whether to touch them is not mine to decide -- the same one-line seam would take all four down the same way, and the measurement for both files together is 387s -> 66s with all 40 tests still passing. File goes 242s -> 192s here; the saving on the runner should be larger, since that machine spent 151s on what costs 50s locally. --- tests/test_remote_engine_start_refusal.bats | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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.