From 1b243eef088cf0342c26feae4fa1e6afad600b16 Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 13 Aug 2026 14:03:40 -0700 Subject: [PATCH 01/18] feat(sync): start a connected team's engine when an agent turns up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A machine restart leaves every sync engine dead and nothing restarts one. The agent keeps working, send keeps committing locally, and nothing reaches the other machines until a person happens to type remote sync start. #765 made that visible, which was the right first step and asks a person to do what the machine can do (#774). Two trigger points, both places an agent already establishes what it is: session-start.sh, where the monitor is started, and actas-claim.sh, where a session takes on a role and therefore a team. ONE ENGINE PER (MACHINE, TEAM) IS NOT ENFORCED HERE, DELIBERATELY. cmd_sync_start already takes the per-team lock, answers 'Sync engine already running' under it, and returns 0; the pidfile is per team. So this calls that command and inherits the invariant. Checking the pidfile here instead would put a second answer to 'is it running?' outside the lock that makes the first one true — and two answers diverge exactly when several sessions open at once, which is the case this feature exists for. The binding check is inherited the same way: the command refuses an unbound or disconnected team by name. A consequence worth stating: no mutation of this file can turn the 'exactly one engine' assertion red, because that property is not implemented here. The race test proves the inheritance, not an implementation. Had I written my own liveness check, that assertion WOULD be mutable — and its being mutable is what a second answer looks like from the outside. NOTHING HERE MAY FAIL A SESSION. Every path returns 0; a start that fails prints what the command said and the session continues, because an agent that will not open because a sync engine refused is worse than an engine that is down. A team already running produces no output at all — starting is a side effect nobody asked for in this moment, and a line would then appear on every session start for the rest of the machine's life. Tests, six, with the concurrent case as the centre: five callers race, every one returns 0, exactly one reports starting, four are silent, and one engine is alive — counted from the process table, not from the pidfile, which can only ever name one and so is the wrong witness. Mutations: treat 'already running' as a start 2 red (incl. the race) propagate the command's exit code 2 red drop 2>&1, losing the stated reason 1 red Interacts with #773: an engine that exits on a server refusal will now be restarted every session and exit again. That issue is next and is not made worse by this landing — today the engine is simply dead instead. --- scripts/actas-claim.sh | 29 +++++ scripts/lib/sync-autostart.sh | 93 ++++++++++++++++ scripts/session-start.sh | 58 +++++----- tests/test_sync_autostart.bats | 189 +++++++++++++++++++++++++++++++++ 4 files changed, 339 insertions(+), 30 deletions(-) create mode 100644 scripts/lib/sync-autostart.sh create mode 100644 tests/test_sync_autostart.bats diff --git a/scripts/actas-claim.sh b/scripts/actas-claim.sh index f66200a62..3ea8017f4 100755 --- a/scripts/actas-claim.sh +++ b/scripts/actas-claim.sh @@ -97,6 +97,35 @@ while IFS= read -r team; do agmsg_role_session_record "$team" "$NAME" "$BARE_SID" "$PROJECT_PHYS" "$TYPE" || true done <<< "$TEAMS" +# Start the engine for each claimed team, if one is not already up (#774). +# +# The second of the two trigger points. `actas` is where a session takes on a +# role and therefore a team, and a session that arrives this way never passes +# through session-start's block with that team in hand — a spawn's boot prompt +# is `actas`, so on a rebooted machine this is the first moment the team is +# known. +# +# AFTER the claim and BEFORE the status line: the claim is the thing the caller +# is waiting on, and nothing about starting an engine may delay or fail it. The +# helper returns 0 on every path and `|| true` says so a second time, because a +# session that will not open is worse than an engine that is down. +# +# Whether an engine is already running is not asked here — `sync start` answers +# it under the per-team lock, and the concurrent case (several sessions claiming +# roles at once) is exactly the one a second answer gets wrong. See +# scripts/lib/sync-autostart.sh. +if [ -x "$SKILL_DIR/scripts/remote.sh" ] && [ -r "$SKILL_DIR/scripts/lib/sync-autostart.sh" ]; then + # shellcheck source=scripts/lib/sync-autostart.sh + . "$SKILL_DIR/scripts/lib/sync-autostart.sh" + _autostart_teams=() + while IFS= read -r _t; do + [ -n "$_t" ] && _autostart_teams+=("$_t") + done <<< "$TEAMS" + if [ ${#_autostart_teams[@]} -gt 0 ]; then + agmsg_sync_autostart "$SKILL_DIR/scripts/remote.sh" "${_autostart_teams[@]}" || true + fi +fi + # Print a line describing each claimed team. One team per most projects but # the underlying model allows multi-team same-name registrations. printf 'status=ok' diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh new file mode 100644 index 000000000..411bf9d7a --- /dev/null +++ b/scripts/lib/sync-autostart.sh @@ -0,0 +1,93 @@ +# Start a connected team's sync engine when an agent turns up (#774). +# +# A machine restart leaves every sync engine dead and nothing restarts one. The +# agent keeps working, `send` keeps committing locally, and nothing reaches the +# other machines until a person happens to type `remote sync start`. #765 made +# that visible; a warning still asks a person to do what the machine can do. +# +# Sourced by the two places an agent establishes what it is: +# scripts/session-start.sh — where the monitor is started +# scripts/actas-claim.sh — where a session takes on a role, and a team +# +# ONE ENGINE PER (MACHINE, TEAM) IS NOT ENFORCED HERE, AND MUST NOT BE. +# +# `cmd_sync_start` already takes `agmsg_lock_acquire "$TEAMS_DIR/"`, and +# under that lock it answers `Sync engine already running (pid N).` and returns +# 0. The pidfile is per team. So the invariant holds by construction in the +# command, and this calls the command. +# +# The alternative — checking the pidfile here and starting only when it looks +# dead — puts a SECOND answer to "is it running?" in the tree, outside the lock +# that makes the first one true. Two answers to that question diverge exactly +# when several sessions open at once, which is the case this exists for: they +# race for the lock, one starts the engine, the rest are told `already running` +# and carry on. That behaviour is the command's, and it is inherited rather than +# reproduced. +# +# THE BINDING CHECK IS INHERITED TOO. `cmd_sync_start` refuses a team with no +# active binding and a disconnected team, by name, before it starts anything. +# Filtering on the binding here would be the same duplication one level up. +# +# NOTHING HERE MAY FAIL A SESSION. An agent that will not open because a sync +# engine refused is worse than a sync engine that is down, so every path returns +# 0 and the worst outcome is a line of text. + +# Usage: agmsg_sync_autostart ... +# +# Prints, at most, one block: the teams whose engines this call started, and the +# teams it could not start. A team whose engine was already running produces no +# output at all — starting is a side effect the person did not ask for in this +# moment, and "nothing changed" is not news. +agmsg_sync_autostart() { + local remote_sh="$1"; shift + [ -x "$remote_sh" ] || return 0 + [ $# -gt 0 ] || return 0 + + local team out rc started="" failed="" + for team in "$@"; do + [ -n "$team" ] || continue + # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that + # sentence is the useful half of a failure. Swallowing it would leave this + # printing "could not start" with the reason on the floor. + out="$("$remote_sh" sync start "$team" 2>&1)"; rc=$? + if [ "$rc" -eq 0 ] && printf '%s' "$out" | grep -q 'already running'; then + continue + fi + if [ "$rc" -eq 0 ]; then + started="$started$team"$'\n' + continue + fi + # The team name AND what the command said. A bare "could not start " + # sends the reader to a log to find a sentence this already had. + failed="$failed$team $(printf '%s' "$out" | tr '\n' ' ')"$'\n' + done + + if [ -n "$started" ]; then + # Written as a loop rather than a joined string: a team name may contain + # characters that make a one-line join ambiguous, and one line per team is + # what the #765 block already established as this hook's voice. + printf '%s\n' 'AGMSG: no sync engine was running; started one for:' + printf '%s' "$started" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' %s\n' "$t" + done + printf '\n' + fi + + if [ -n "$failed" ]; then + printf '%s\n' 'AGMSG: connected, but not syncing.' '' + printf '%s\n' \ + 'No sync engine is running for the team(s) below and starting one failed,' \ + 'so messages from other machines are not arriving. The session continues.' '' + printf '%s' "$failed" | while IFS=$'\t' read -r t reason; do + [ -n "$t" ] || continue + printf ' %s: %s\n' "$t" "$reason" + # The runnable line, unchanged from #765: the remedy is still a person + # running the command, and now they also know it has been tried. + printf ' bash %q sync start %q\n' "$remote_sh" "$t" + done + printf '\n' + fi + + return 0 +} diff --git a/scripts/session-start.sh b/scripts/session-start.sh index f66f345d3..fab4e078c 100755 --- a/scripts/session-start.sh +++ b/scripts/session-start.sh @@ -245,50 +245,48 @@ if [ -n "$CC_PID" ]; then printf '%s\n' "$INSTANCE_ID" > "$STATE" fi -# --- Say when a connected team has no engine (#761). --- +# --- Start the engine for a connected team that has none (#761, #774). --- # A reboot leaves every sync engine dead and nothing restarts one: the five # commands that start it are all operator actions. `connected` keeps printing, # `send` keeps succeeding locally, and the only symptom is messages not arriving # — which reads as "nobody wrote anything". This is the first moment after a -# reboot when anything of ours runs, so it is where the absence gets said. +# reboot when anything of ours runs, so it is where it gets started. +# +# #765 made the absence VISIBLE here, in this block, and that was the right +# first step and not enough: the warning appeared only when someone opened a +# session, and it asked the person for something the machine can do. Starting it +# is the same trigger doing the whole job. # # BEFORE the three directive blocks below rather than inside them: there are # three ways out of this script (a watcher already streaming, the actas variant, # and the default), and a line added to one of them is missing from the other # two. # +# WHICH TEAMS. `status` is asked for CONNECTED teams — the binding, not the +# engine. Whether an engine is running is `sync start`'s question, under the +# lock that makes the answer true; asking it here as well is the second answer +# that diverges (see scripts/lib/sync-autostart.sh). +# # Best-effort, and bounded. `status` is a subprocess and needs python3; if it -# cannot run, this says nothing rather than guessing. That is a real gap and not -# a silent one: the check reports what it saw, and what it could not see is the -# absence of a line, which is why the line names the teams rather than a count. +# cannot run, this does nothing rather than guessing. A failure to start never +# fails the session: an agent that will not open because a sync engine refused +# is worse than a sync engine that is down. # # Reaches `monitor` and `both` only — this hook is not installed for `turn` or # `off`, so those modes still get the absence only from `status`. -if [ -x "$SKILL_DIR/scripts/remote.sh" ]; then - _stale_teams="$("$SKILL_DIR/scripts/remote.sh" status 2>/dev/null \ - | awk -F'\t' '/engine (stopped|stale)/ {print $1}' || true)" - if [ -n "$_stale_teams" ]; then - printf '%s\n' "AGMSG: connected, but not syncing." '' - printf '%s\n' \ - 'No sync engine is running for the team(s) below, so messages from other' \ - 'machines are not arriving. Nothing restarts one after a reboot; run:' '' - # ONE RUNNABLE LINE PER TEAM, with no placeholder in it. - # - # The first version printed `sync start ` once. A reader has to - # replace `` before that works, and an unreplaced `` is a valid - # team name as far as validation is concerned — angle brackets are not among - # the characters it rejects. The names are already in hand here, so there is - # nothing to leave unfilled (the same reasoning #339 applied to the - # onboarding prompt). - # - # `printf %q` for both the path and the name: an install directory with a - # space in it, or a team name that needs quoting, otherwise produces a line - # that reads as runnable and is not. - printf '%s\n' "$_stale_teams" | while IFS= read -r _t; do - [ -n "$_t" ] || continue - printf ' bash %q sync start %q\n' "$SKILL_DIR/scripts/remote.sh" "$_t" - done - printf '\n' +if [ -x "$SKILL_DIR/scripts/remote.sh" ] && [ -r "$SKILL_DIR/scripts/lib/sync-autostart.sh" ]; then + # shellcheck source=scripts/lib/sync-autostart.sh + . "$SKILL_DIR/scripts/lib/sync-autostart.sh" + _connected_teams="$("$SKILL_DIR/scripts/remote.sh" status 2>/dev/null \ + | awk -F'\t' '$2 ~ /^connected/ {print $1}' || true)" + if [ -n "$_connected_teams" ]; then + # Word splitting on newlines only: a team name may contain a space, and + # `$(...)` unquoted would split it into two names that start nothing. + _old_ifs="$IFS"; IFS=$'\n' + # shellcheck disable=SC2086 + set -- $_connected_teams + IFS="$_old_ifs" + agmsg_sync_autostart "$SKILL_DIR/scripts/remote.sh" "$@" || true fi fi diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats new file mode 100644 index 000000000..939f82e82 --- /dev/null +++ b/tests/test_sync_autostart.bats @@ -0,0 +1,189 @@ +#!/usr/bin/env bats + +load test_helper + +# Starting a connected team's engine when an agent turns up (#774). +# +# The case this exists for is SEVERAL SESSIONS AT ONCE on one machine, in one +# team. They race for the per-team lock `cmd_sync_start` takes; one starts the +# engine and the rest are told `already running` and carry on. That behaviour +# belongs to the command, and these tests pin that the auto-start path inherits +# it rather than reproducing it — a second answer to "is it running?" diverges +# exactly under this race. + +setup() { + setup_test_env + bash "$SCRIPTS/join.sh" testteam alice claude-code /tmp/project-a + + local cfg="$TEST_SKILL_DIR/teams/testteam/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" + ENGINE_PIDS="" +} + +teardown() { + local pid + for pid in $ENGINE_PIDS; do + kill "$pid" 2>/dev/null || true + wait "$pid" 2>/dev/null || true + done + teardown_test_env +} + +# A node that becomes READY and then stays up. +# +# `cmd_sync_start` does not return when the process exists — it waits for the +# engine's `startup_nonce` to appear in the logfile, so a fake that only sleeps +# makes the command spin until its own timeout. Same shape as +# test_remote_status_liveness.bats's fake node, which is where this came from. +write_fake_node() { + local fake_node="$TEST_SKILL_DIR/fake-node" + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--version" ]; then' \ + ' echo v23.0.0' \ + ' exit 0' \ + 'fi' \ + 'echo "{\"event\":\"capabilities\",\"startup_nonce\":\"${AGMSG_SYNC_START_NONCE:-}\"}"' \ + 'trap "exit 0" TERM INT' \ + 'while :; do sleep 1; done' > "$fake_node" + chmod +x "$fake_node" + printf '%s\n' "$fake_node" +} + +# A node that fails to start at all. +write_failing_node() { + local fake_node="$TEST_SKILL_DIR/fake-node-bad" + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--version" ]; then echo v23.0.0; exit 0; fi' \ + 'echo "engine exploded" >&2' \ + 'exit 1' > "$fake_node" + chmod +x "$fake_node" + printf '%s\n' "$fake_node" +} + +collect_engine_pids() { + local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + [ -f "$pidfile" ] && ENGINE_PIDS="$ENGINE_PIDS $(cat "$pidfile")" + return 0 +} + +@test "starts an engine for a connected team that has none" { + export AGMSG_NODE="$(write_fake_node)" + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + collect_engine_pids + [ "$status" -eq 0 ] + [[ "$output" == *"started one for"* ]] + [[ "$output" == *"testteam"* ]] + # The artifact, not the sentence: a pidfile naming a live process. + [ -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] + kill -0 "$(cat "$TEST_SKILL_DIR/run/remote-sync.testteam.pid")" +} + +@test "says nothing at all when the engine is already running" { + export AGMSG_NODE="$(write_fake_node)" + bash "$SCRIPTS/remote.sh" sync start testteam + collect_engine_pids + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + [ "$status" -eq 0 ] + # Starting is a side effect nobody asked for in this moment; "nothing + # changed" is not news, and a line here would appear on every session start + # for the rest of the machine's life. + [ -z "$output" ] +} + +@test "several sessions at once leave exactly one engine, and none of them fails" { + # THE CASE THIS FEATURE IS FOR. Five callers race for the per-team lock. + export AGMSG_NODE="$(write_fake_node)" + source "$SCRIPTS/lib/sync-autostart.sh" + + local i outdir="$TEST_SKILL_DIR/race" + mkdir -p "$outdir" + for i in 1 2 3 4 5; do + ( + agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam > "$outdir/$i.out" 2>&1 + printf '%s\n' "$?" > "$outdir/$i.rc" + ) & + done + wait + collect_engine_pids + + # Every caller succeeded — the losers of the race are not failures. + for i in 1 2 3 4 5; do + [ "$(cat "$outdir/$i.rc")" = "0" ] + done + + # Exactly one of them reports having started it. The rest say nothing, which + # is what `already running` produces. + local started=0 quiet=0 + for i in 1 2 3 4 5; do + if grep -q "started one for" "$outdir/$i.out"; then + started=$((started + 1)) + elif [ ! -s "$outdir/$i.out" ]; then + quiet=$((quiet + 1)) + fi + done + [ "$started" -eq 1 ] + [ "$quiet" -eq 4 ] + + # And one engine exists, not five. Counted from the process table rather than + # from the pidfile: the pidfile can only ever name one, so asking it would be + # asking the wrong witness. + local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + [ -f "$pidfile" ] + kill -0 "$(cat "$pidfile")" + local live + live="$(pgrep -f "fake-node" 2>/dev/null | wc -l | tr -d ' ')" + [ "$live" = "1" ] +} + +@test "a team that is disconnected is not started, and the refusal is shown" { + export AGMSG_NODE="$(write_fake_node)" + local cfg="$TEST_SKILL_DIR/teams/testteam/config.json" escaped updated + escaped="$(sed "s/'/''/g" "$cfg")" + updated="$(sqlite_mem " + SELECT json_set('$escaped', '\$.remote_binding.disconnected_at', '2026-08-01T00:00:00Z');")" + printf '%s\n' "$updated" > "$cfg" + + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + [ "$status" -eq 0 ] + # The binding check is the COMMAND's, inherited: it refuses by name before it + # starts anything, and the reason it gave is repeated rather than replaced. + [[ "$output" == *"disconnected"* ]] + [ ! -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] +} + +@test "a start that fails does not fail the caller, and says what the command said" { + # An agent that will not open because a sync engine refused is worse than a + # sync engine that is down. + export AGMSG_NODE="$(write_failing_node)" + + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + [ "$status" -eq 0 ] + [[ "$output" == *"connected, but not syncing"* ]] + [[ "$output" == *"The session continues."* ]] + # The runnable remedy survives from #765 — the person now also knows it was + # tried. + [[ "$output" == *"sync start"* ]] +} + +@test "no teams, no output, no failure" { + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" + [ "$status" -eq 0 ] + [ -z "$output" ] +} From 8cf9c158c14b9370beda35a07138e0b252c4a607 Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 13 Aug 2026 19:35:51 -0700 Subject: [PATCH 02/18] fix(sync): cross each trigger, bound the wait, and make the assertions enforceable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, and the first one is this issue one level up. P1-1: the six cases drove agmsg_sync_autostart alone. Deleting the invocation from either trigger left all six green — and #774 IS the triggers, not the helper. The helper working shows nothing about it being called, and the two wirings are different code: session-start awks remote.sh status for connected teams, actas-claim array-ifies TEAMS after the claim. Four cases now drive the real scripts against a fake remote: a connected team is started, a disconnected one is never offered to the command, and the thing the session actually needs still comes out (the Monitor directive; status=ok). That is the third time tonight I tested a command and not its wiring. It is also what the per-trigger deletion mutation is for, and it earned its keep twice over: - 'actas does not wait for a start that hangs' passed with the invocation DELETED. Nothing to wait for is also fast, so the case could not tell a bound from an absence — it was measuring the feature's absence and calling it a bound. - the repair for that was itself timing-fragile: it grepped for the recorded call immediately after the helper gave up waiting, which passes on an idle machine and fails under load. Now it waits for the record, with the session's own bound measured separately from the outside. P1-2: 'nothing here may fail a session' held only for exit status. Both triggers ran sync start synchronously — actas before printing status=ok, session-start before the Monitor directive — and cmd_sync_start waits out a readiness loop of its own before giving up, per team, serially, with no bound if the child hangs. A release-blocker fix that can stop a session from starting is not a fix. The source comment even said 'Best-effort, and bounded' while being unbounded in time. Each start now runs in the background under a whole-call budget (AGMSG_SYNC_AUTOSTART_TIMEOUT_S, 5s). When it expires the child is LEFT RUNNING rather than killed — it may be seconds from success, and killing it could leave a half-made pidfile — and what stops is the waiting. The session says a start is in flight and goes on. A consequence, tested separately rather than folded in: a start that FAILS is only reported as a failure if the command notices within the budget. Past it, the honest sentence is 'still in flight'. Those are different facts. P1-3: five non-terminal [[ ... ]] assertions cannot fail a test under macOS bash 3.2, which is what CI runs. Replaced with printf | grep -q, the form the checker measures as enforced on both interpreters. Mutations, per trigger, each turning only its own side red: session-start's invocation removed its 2 cases red, actas 2 green actas-claim's invocation removed its 2 cases red, session-start 2 green --- scripts/actas-claim.sh | 11 ++- scripts/lib/sync-autostart.sh | 58 +++++++++++- scripts/session-start.sh | 11 ++- tests/test_sync_autostart.bats | 165 +++++++++++++++++++++++++++++++-- 4 files changed, 227 insertions(+), 18 deletions(-) diff --git a/scripts/actas-claim.sh b/scripts/actas-claim.sh index 3ea8017f4..d4684a9c9 100755 --- a/scripts/actas-claim.sh +++ b/scripts/actas-claim.sh @@ -106,9 +106,14 @@ done <<< "$TEAMS" # known. # # AFTER the claim and BEFORE the status line: the claim is the thing the caller -# is waiting on, and nothing about starting an engine may delay or fail it. The -# helper returns 0 on every path and `|| true` says so a second time, because a -# session that will not open is worse than an engine that is down. +# is waiting on, and nothing about starting an engine may delay or fail it. +# +# DELAY IS THE HALF THAT NEEDED WORK. Returning 0 is not enough — a synchronous +# `sync start` holds `status=ok` back for as long as the engine takes to become +# ready, which is up to ~16s per team before the command even gives up. The +# helper bounds the WAIT (`AGMSG_SYNC_AUTOSTART_TIMEOUT_S`, 5s for the whole +# call) and leaves a slow start running rather than killing it. `|| true` says +# the exit-status half a second time. # # Whether an engine is already running is not asked here — `sync start` answers # it under the per-team lock, and the concurrent case (several sessions claiming diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 411bf9d7a..41a475bf5 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -28,9 +28,22 @@ # active binding and a disconnected team, by name, before it starts anything. # Filtering on the binding here would be the same duplication one level up. # -# NOTHING HERE MAY FAIL A SESSION. An agent that will not open because a sync -# engine refused is worse than a sync engine that is down, so every path returns -# 0 and the worst outcome is a line of text. +# NOTHING HERE MAY FAIL A SESSION, AND FAILING INCLUDES BEING SLOW. +# +# Returning 0 on every path is only half of it — the first version did that and +# still ran `sync start` synchronously, which means `actas` did not print +# `status=ok` and session start did not emit the Monitor directive until the +# engine was ready. `cmd_sync_start` waits for a readiness nonce (~16s of its +# own before it gives up), takes a per-team lock others may be holding, and can +# be stuck for as long as its child is. Multiplied by the number of connected +# teams, in the critical path of an agent opening. A release-blocker fix that +# can stop a session from starting is not a fix (raised in review). +# +# So each start runs in the BACKGROUND and this waits, at most, for a whole-call +# budget shared by every team. When the budget runs out the child is LEFT +# RUNNING rather than killed: it may be seconds from having started the engine, +# and killing it could leave a half-made pidfile behind. What stops is the +# WAITING. The session goes on and the line says a start is still in flight. # Usage: agmsg_sync_autostart ... # @@ -43,13 +56,34 @@ agmsg_sync_autostart() { [ -x "$remote_sh" ] || return 0 [ $# -gt 0 ] || return 0 - local team out rc started="" failed="" + # Seconds, for the whole call. Overridable so a test can drive the deadline + # without waiting for it, and so an operator on a slow machine can raise it. + local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" + local elapsed_start=$SECONDS + + local team out rc pid tmp started="" failed="" slow="" for team in "$@"; do [ -n "$team" ] || continue + tmp="$(mktemp 2>/dev/null)" || tmp="" + [ -n "$tmp" ] || return 0 # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that # sentence is the useful half of a failure. Swallowing it would leave this # printing "could not start" with the reason on the floor. - out="$("$remote_sh" sync start "$team" 2>&1)"; rc=$? + "$remote_sh" sync start "$team" >"$tmp" 2>&1 & + pid=$! + while kill -0 "$pid" 2>/dev/null && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do + sleep 0.1 + done + if kill -0 "$pid" 2>/dev/null; then + # Budget spent. NOT killed — see the header. The temp file is left for the + # child to finish writing into; it is in the system temp dir and is the + # price of not truncating a start that may be about to succeed. + slow="$slow$team"$'\n' + continue + fi + wait "$pid"; rc=$? + out="$(cat "$tmp" 2>/dev/null)" + rm -f "$tmp" if [ "$rc" -eq 0 ] && printf '%s' "$out" | grep -q 'already running'; then continue fi @@ -74,6 +108,20 @@ agmsg_sync_autostart() { printf '\n' fi + if [ -n "$slow" ]; then + printf '%s\n' "AGMSG: a sync engine start is still in flight after ${budget}s; not waiting for it:" + printf '%s' "$slow" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' %s\n' "$t" + done + printf '%s\n' 'The session continues. Check it with:' '' + printf '%s' "$slow" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' bash %q status %q\n' "$remote_sh" "$t" + done + printf '\n' + fi + if [ -n "$failed" ]; then printf '%s\n' 'AGMSG: connected, but not syncing.' '' printf '%s\n' \ diff --git a/scripts/session-start.sh b/scripts/session-start.sh index fab4e078c..e9130d116 100755 --- a/scripts/session-start.sh +++ b/scripts/session-start.sh @@ -267,10 +267,13 @@ fi # lock that makes the answer true; asking it here as well is the second answer # that diverges (see scripts/lib/sync-autostart.sh). # -# Best-effort, and bounded. `status` is a subprocess and needs python3; if it -# cannot run, this does nothing rather than guessing. A failure to start never -# fails the session: an agent that will not open because a sync engine refused -# is worse than a sync engine that is down. +# Best-effort, and bounded IN TIME as well as in outcome. `status` is a +# subprocess and needs python3; if it cannot run, this does nothing rather than +# guessing. A start that fails never fails the session, and a start that is SLOW +# never delays the Monitor directive below: the helper waits for a whole-call +# budget (`AGMSG_SYNC_AUTOSTART_TIMEOUT_S`, 5s) and then leaves the start +# running and moves on. An agent that will not open because a sync engine was +# thinking is worse than a sync engine that is down. # # Reaches `monitor` and `both` only — this hook is not installed for `turn` or # `off`, so those modes still get the absence only from `status`. diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 939f82e82..3c4ad1050 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -13,6 +13,9 @@ load test_helper setup() { setup_test_env + # The trigger tests below run the real scripts, which read these two. + export SKILL_DIR="$TEST_SKILL_DIR" + export RUN_DIR="$SKILL_DIR/run" bash "$SCRIPTS/join.sh" testteam alice claude-code /tmp/project-a local cfg="$TEST_SKILL_DIR/teams/testteam/config.json" escaped updated @@ -72,6 +75,31 @@ write_failing_node() { printf '%s\n' "$fake_node" } +# Register a (team, agent) pair for the test project, as the actas tests do. +fake_register() { + local team="$1" agent="$2" proj="${3:-/tmp/p1}" + bash "$SCRIPTS/join.sh" "$team" "$agent" claude-code "$proj" >/dev/null 2>&1 || true +} + +# Wait briefly for a call to be RECORDED. +# +# The "does not wait" cases give the helper a 1s budget, so it returns while the +# child is still running — and the child records the team name as its first act. +# Grepping immediately is therefore a race with a process the test deliberately +# did not wait for: it passed on an idle machine and went red under load, which +# is a flaky assertion dressed as a strict one. The bound here is generous +# because it is not measuring speed; the SESSION's bound is measured separately, +# from the outside, in the same test. +wait_for_call() { + local file="$1" needle="$2" i=0 + while [ "$i" -lt 100 ]; do + grep -q "^$needle\$" "$file" 2>/dev/null && return 0 + sleep 0.1 + i=$((i + 1)) + done + return 1 +} + collect_engine_pids() { local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" [ -f "$pidfile" ] && ENGINE_PIDS="$ENGINE_PIDS $(cat "$pidfile")" @@ -84,8 +112,8 @@ collect_engine_pids() { run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam collect_engine_pids [ "$status" -eq 0 ] - [[ "$output" == *"started one for"* ]] - [[ "$output" == *"testteam"* ]] + printf '%s' "$output" | grep -q 'started one for' + printf '%s' "$output" | grep -q 'testteam' # The artifact, not the sentence: a pidfile naming a live process. [ -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] kill -0 "$(cat "$TEST_SKILL_DIR/run/remote-sync.testteam.pid")" @@ -162,23 +190,31 @@ collect_engine_pids() { [ "$status" -eq 0 ] # The binding check is the COMMAND's, inherited: it refuses by name before it # starts anything, and the reason it gave is repeated rather than replaced. - [[ "$output" == *"disconnected"* ]] + printf '%s' "$output" | grep -q 'disconnected' [ ! -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] } @test "a start that fails does not fail the caller, and says what the command said" { # An agent that will not open because a sync engine refused is worse than a # sync engine that is down. + # + # The budget is raised for this case on purpose. `cmd_sync_start` does not + # notice a dead engine immediately — it waits out its readiness loop — so + # under the default 5s this failure is reported as "still in flight", which + # is TRUE and is a different sentence. The two outcomes are tested + # separately rather than folded together: "it failed" and "it has not + # answered yet" are different facts and the tool says different things. + export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 export AGMSG_NODE="$(write_failing_node)" source "$SCRIPTS/lib/sync-autostart.sh" run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam [ "$status" -eq 0 ] - [[ "$output" == *"connected, but not syncing"* ]] - [[ "$output" == *"The session continues."* ]] + printf '%s' "$output" | grep -q 'connected, but not syncing' + printf '%s' "$output" | grep -q 'The session continues.' # The runnable remedy survives from #765 — the person now also knows it was # tried. - [[ "$output" == *"sync start"* ]] + printf '%s' "$output" | grep -q 'sync start' } @test "no teams, no output, no failure" { @@ -187,3 +223,120 @@ collect_engine_pids() { [ "$status" -eq 0 ] [ -z "$output" ] } + +# ── the two production triggers, driven for real ───────────────────────────── +# +# Everything above drives `agmsg_sync_autostart` directly, and deleting the +# wiring from either trigger leaves all of it green (raised in review). The +# wiring is the PR's whole point and it is different on each side: +# session-start awks `remote.sh status` for connected teams, actas-claim +# array-ifies `$TEAMS` after the claim. Neither follows from the helper being +# right. + +# A `remote.sh` this test controls, standing in for the real one so a trigger +# can be driven without a server. It records every call it was given. +write_fake_remote() { + local behaviour="$1" fake="$TEST_SKILL_DIR/fake-remote.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'calls="$AGMSG_FAKE_REMOTE_CALLS"' + printf '%s\n' 'if [ "${1:-}" = "status" ]; then' + printf '%s\n' ' printf "%s\tconnected (engine stopped — run: x) since 2026-07-30T00:00:00Z\n" testteam' + printf '%s\n' ' printf "%s\tdisconnected (was connected until 2026-08-01T00:00:00Z)\n" otherteam' + printf '%s\n' ' exit 0' + printf '%s\n' 'fi' + printf '%s\n' 'if [ "${1:-}" = "sync" ] && [ "${2:-}" = "start" ]; then' + printf '%s\n' ' printf "%s\n" "$3" >> "$calls"' + case "$behaviour" in + starts) printf '%s\n' ' echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0' ;; + hangs) printf '%s\n' ' while :; do sleep 1; done' ;; + esac + printf '%s\n' 'fi' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +@test "session-start starts the engine for a connected team, and still emits the directive" { + local fake calls="$TEST_SKILL_DIR/calls.txt" + fake="$(write_fake_remote starts)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + echo "sid-current" > "$RUN_DIR/cc-instance.$$" + + run env AGMSG_FAKE_REMOTE_CALLS="$calls" bash -c \ + 'printf "{\"session_id\":\"sid-current\"}" | bash "$1" claude-code /tmp/p1' _ \ + "$SCRIPTS/session-start.sh" + + # The connected team was started... + grep -q '^testteam$' "$calls" + # ...and the disconnected one was never offered to the command. + ! grep -q '^otherteam$' "$calls" + # ...and the thing the session actually needs still came out. + printf '%s' "$output" | grep -q 'AGMSG' + [ "$status" -eq 0 ] +} + +@test "session-start does not wait for a start that hangs" { + local fake calls="$TEST_SKILL_DIR/calls.txt" began ended + fake="$(write_fake_remote hangs)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + echo "sid-current" > "$RUN_DIR/cc-instance.$$" + + began=$SECONDS + run env AGMSG_FAKE_REMOTE_CALLS="$calls" AGMSG_SYNC_AUTOSTART_TIMEOUT_S=1 bash -c \ + 'printf "{\"session_id\":\"sid-current\"}" | bash "$1" claude-code /tmp/p1' _ \ + "$SCRIPTS/session-start.sh" + ended=$SECONDS + + # THAT IT WAS TRIED. Without this the case passes when the invocation is + # DELETED — nothing to wait for is also fast — so it would be measuring the + # absence of the feature and calling it a bound (found by the deletion + # mutation; the actas twin below had the same hole). + wait_for_call "$calls" testteam + # The bound, from the outside: a session that waits on a hung child is the + # release-blocker fix blocking a release. + [ $((ended - began)) -lt 10 ] + # It said a start is in flight rather than pretending nothing happened. + printf '%s' "$output" | grep -q 'still in flight' + [ "$status" -eq 0 ] +} + +@test "actas-claim starts the engine and still prints status=ok" { + local fake calls="$TEST_SKILL_DIR/calls.txt" + fake="$(write_fake_remote starts)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + + run env AGMSG_FAKE_REMOTE_CALLS="$calls" bash "$SCRIPTS/actas-claim.sh" \ + /tmp/project-a claude-code alice sid-actas + # The claim is what the caller is waiting on, and it still arrives. + printf '%s' "$output" | grep -q 'status=ok' + grep -q '^testteam$' "$calls" + [ "$status" -eq 0 ] +} + +@test "actas-claim does not wait for a start that hangs" { + local fake calls="$TEST_SKILL_DIR/calls.txt" began ended + fake="$(write_fake_remote hangs)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + + began=$SECONDS + run env AGMSG_FAKE_REMOTE_CALLS="$calls" AGMSG_SYNC_AUTOSTART_TIMEOUT_S=1 \ + bash "$SCRIPTS/actas-claim.sh" /tmp/project-a claude-code alice sid-actas + ended=$SECONDS + + # THAT IT WAS TRIED — see the session-start twin. Deleting the invocation + # made this case pass, which is the check measuring its own absence. + wait_for_call "$calls" testteam + [ $((ended - began)) -lt 10 ] + printf '%s' "$output" | grep -q 'status=ok' + [ "$status" -eq 0 ] +} From 3f4e89689b8924c798b817a4958889597484f682 Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 13 Aug 2026 21:13:25 -0700 Subject: [PATCH 03/18] fix(sync): enforce the negative, keep the remedy runnable, and reap the deliberate children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, and the first two are the CI failures on the previous head. 1. I removed five unenforceable assertions and added one in the same head. tests/test_sync_autostart.bats had a non-terminal '! grep -q' for the negative (a disconnected team is never offered to the command). A leading '!' does not trip errexit on either interpreter, so it reports ok whatever it finds. This harness already has 'refute'; used it. 2. The warning's runnable remedy was printed with a FOUR-space indent. #765 prints two, and tests/test_delivery.bats extracts the command with sed -n 's/^ bash //p' and then RUNS it. So the deeper indent hid the operator's remedy from the check that proves the remedy is runnable. Back to two spaces, with the reason written where someone might 'tidy' it again. That test is not a stale test. It pins the #761/#765 ruling — do not start anything, make the absence visible — which #774 REVERSES. What #765 built is not discarded: its warning, wording and remedy are what remain when the start fails, which is the case that test drives. The test name and its comment now say a decision was reversed, so this does not read as a test edited to fit new output. Its budget is raised so the failure path is deterministic: under the 5s default the command may not have finished failing when the hook stops waiting, and 'still in flight' is then the honest sentence — a different fact, tested separately. 3. The 'does not wait' cases leave a sync start child running on purpose, and nothing reaped it. A CI shard runs many files in one process tree, so a fake that loops forever becomes somebody else's flake — which fits both OSes failing the same shard numbers. teardown now kills them. My own measurement error made 2 reachable: I reported test_delivery as 'ok' having looked at tail -3. The last three lines being ok is not a suite passing. --- scripts/lib/sync-autostart.sh | 9 ++++++--- tests/test_delivery.bats | 24 +++++++++++++++++++++++- tests/test_sync_autostart.bats | 13 ++++++++++++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 41a475bf5..96d4f5bae 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -130,9 +130,12 @@ agmsg_sync_autostart() { printf '%s' "$failed" | while IFS=$'\t' read -r t reason; do [ -n "$t" ] || continue printf ' %s: %s\n' "$t" "$reason" - # The runnable line, unchanged from #765: the remedy is still a person - # running the command, and now they also know it has been tried. - printf ' bash %q sync start %q\n' "$remote_sh" "$t" + # The runnable line, UNCHANGED FROM #765 — including its two-space + # indent. That prefix is part of the contract: `test_delivery.bats` + # extracts the command with `sed -n 's/^ bash //p'` and runs it, so a + # deeper indent leaves the operator's remedy unrunnable by the check that + # proves it is runnable (measured: it failed on exactly that). + printf ' bash %q sync start %q\n' "$remote_sh" "$t" done printf '\n' fi diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index 1f1b76e86..b943866fd 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -491,11 +491,33 @@ eperm_pid() { [ "$3" = "$sp" ] } -@test "session-start: a connected team with no engine is said, and a silent one is not (#761)" { +@test "session-start: a connected team with no engine is started, and said when that fails (#761, #774)" { # A reboot kills every sync engine and nothing restarts one. `connected` keeps # printing and `send` keeps succeeding locally, so the only symptom is silence # — which reads as "nobody wrote anything". This hook is the first thing of # ours that runs afterwards. + # + # A RULING WAS REVERSED HERE, and this test is where it is recorded. + # + # #761/#765 decided: do NOT start anything, make the absence VISIBLE. That + # decision is what this test was written to hold. #774 reverses it — an agent + # arriving at a connected team now STARTS the engine — on the grounds that + # visibility asks a person for something the machine can do. + # + # What #765 built is not discarded: its warning, its wording and its runnable + # remedy are exactly what remains when the start FAILS, which is the case + # below. So the assertions about what the operator is told are unchanged, and + # only the reason they are reachable is new. This is a reversal of a decision, + # not a test edited to fit new output (raised in review). It fails here because there is no engine to start in this fixture, + # which is exactly the case the warning is for, so the assertions about what + # the operator is told are unchanged. What is new is the reason they are + # reachable. + # + # The budget is raised so this is the FAILURE path deterministically. Under + # the 5s default, `remote.sh sync start` may not have finished failing when + # the hook stops waiting, and the honest sentence is then "still in flight" — + # a different fact, tested separately in tests/test_sync_autostart.bats. + export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/join.sh" team alice claude-code "$TEST_PROJECT" >/dev/null # NEGATIVE FIRST, on the state every ordinary machine is in: no connected diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 3c4ad1050..2d0eb23db 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -41,6 +41,12 @@ teardown() { kill "$pid" 2>/dev/null || true wait "$pid" 2>/dev/null || true done + # The "does not wait" cases leave a `sync start` child running ON PURPOSE — + # that is the behaviour under test. It must not outlive the test: a CI shard + # runs many files in one process tree, and a fake that loops forever would + # then be somebody else's flake (raised in review). + pkill -f "$TEST_SKILL_DIR/fake-remote.sh" 2>/dev/null || true + pkill -f "$TEST_SKILL_DIR/fake-node" 2>/dev/null || true teardown_test_env } @@ -273,7 +279,12 @@ write_fake_remote() { # The connected team was started... grep -q '^testteam$' "$calls" # ...and the disconnected one was never offered to the command. - ! grep -q '^otherteam$' "$calls" + # + # `refute`, not `! grep`. A leading `!` does not trip errexit on either + # interpreter, so in a non-last position it reports ok whatever it finds — + # I removed five of those from this file and introduced this one in the same + # head (raised in review). + refute grep -q '^otherteam$' "$calls" # ...and the thing the session actually needs still came out. printf '%s' "$output" | grep -q 'AGMSG' [ "$status" -eq 0 ] From bd0d8fc5147adfb4ac742732b2cf384fdaba45a8 Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 13 Aug 2026 21:23:47 -0700 Subject: [PATCH 04/18] test(delivery): force the failed-start condition instead of inheriting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session-start case passed under --filter and failed in the full file. It relied on there being no engine to start, which is true when the file runs alone and not when it runs with its neighbours: what a start does depends on what other tests left behind. That is the same cross-test coupling this branch fixes in its own suite, arriving from the other direction — my children leaking outward there, other tests' leavings breaking my premise here. Both come from a shard running many files in one process tree. So the condition is stated rather than assumed: an unusable interpreter makes sync start fail immediately and for a named reason, which is what the warning under test is about. The case now measures what the operator is told when a start fails, and nothing about what happened to run before it. A --filter pass is not a suite pass, in either direction. --- tests/test_delivery.bats | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index b943866fd..3e8523c62 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -513,10 +513,21 @@ eperm_pid() { # the operator is told are unchanged. What is new is the reason they are # reachable. # - # The budget is raised so this is the FAILURE path deterministically. Under - # the 5s default, `remote.sh sync start` may not have finished failing when - # the hook stops waiting, and the honest sentence is then "still in flight" — - # a different fact, tested separately in tests/test_sync_autostart.bats. + # THE FAILURE IS FORCED, not inherited from the fixture. + # + # The first version relied on there being no engine to start, which is true + # when this file runs alone and NOT when it runs with its neighbours: the + # case passed under `--filter` and failed in the full file, because what the + # start does depends on what other tests left behind. That is the same + # cross-test coupling this PR fixes in its own suite, arriving from the other + # direction — so the condition is stated here instead of assumed. + # + # An unusable interpreter makes `sync start` fail immediately and for a named + # reason, which is what the warning below is about. The budget is raised as + # well: under the 5s default a slow refusal is reported as "still in flight", + # which is a different fact and is tested separately in + # tests/test_sync_autostart.bats. + export AGMSG_NODE="$TEST_SKILL_DIR/no-such-node-for-this-test" export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/join.sh" team alice claude-code "$TEST_PROJECT" >/dev/null From 6ae626fdcd534680577a186081f0fcd2504aeb0c Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 13 Aug 2026 22:21:00 -0700 Subject: [PATCH 05/18] docs(test): let the reversal record say one thing about why the start fails The comment that records the #761/#765 -> #774 reversal contradicted itself. One paragraph said the start fails because the fixture has no engine to start; two paragraphs later it said the failure is forced and inherited from nothing, and the code exports an unusable interpreter. The first sentence was the superseded explanation, left in place while the paragraph around it was rewritten. It is also the explanation this branch threw out: inheriting the failure from the fixture is what made the case pass under --filter and fail in the full file. A durable record of a reversed decision cannot hold both accounts. The forced condition is now the only cause given, with the inherited one described as what it replaced and why. Same shape as the PR body drifting behind its head, one level down: an artifact edited in layers, each layer true when written. --- tests/test_delivery.bats | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index 3e8523c62..83a869f79 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -505,28 +505,24 @@ eperm_pid() { # visibility asks a person for something the machine can do. # # What #765 built is not discarded: its warning, its wording and its runnable - # remedy are exactly what remains when the start FAILS, which is the case - # below. So the assertions about what the operator is told are unchanged, and - # only the reason they are reachable is new. This is a reversal of a decision, - # not a test edited to fit new output (raised in review). It fails here because there is no engine to start in this fixture, - # which is exactly the case the warning is for, so the assertions about what - # the operator is told are unchanged. What is new is the reason they are - # reachable. + # remedy are exactly what remains when the start FAILS, and that is the case + # driven below. The assertions about what the operator is told are therefore + # unchanged; only the reason they are reachable is new. This is a reversal of + # a decision, not a test edited to fit new output (raised in review). # - # THE FAILURE IS FORCED, not inherited from the fixture. + # THE FAILURE IS FORCED, AND THAT IS ITS ONLY CAUSE. An unusable interpreter + # makes `sync start` fail immediately and for a named reason. # - # The first version relied on there being no engine to start, which is true - # when this file runs alone and NOT when it runs with its neighbours: the - # case passed under `--filter` and failed in the full file, because what the - # start does depends on what other tests left behind. That is the same - # cross-test coupling this PR fixes in its own suite, arriving from the other - # direction — so the condition is stated here instead of assumed. + # It used to be inherited instead — the fixture simply had no engine to start + # — and that held only while this file ran alone: the case passed under + # `--filter` and failed in the full file, because what a start does depends on + # what other tests left behind. That is the same cross-test coupling this PR + # fixes in its own suite, arriving from the other direction. The condition is + # stated here so nothing about the surrounding file can decide it. # - # An unusable interpreter makes `sync start` fail immediately and for a named - # reason, which is what the warning below is about. The budget is raised as - # well: under the 5s default a slow refusal is reported as "still in flight", - # which is a different fact and is tested separately in - # tests/test_sync_autostart.bats. + # The budget is raised as well: under the 5s default a slow failure is + # reported as "still in flight", which is a different fact and is tested + # separately in tests/test_sync_autostart.bats. export AGMSG_NODE="$TEST_SKILL_DIR/no-such-node-for-this-test" export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/join.sh" team alice claude-code "$TEST_PROJECT" >/dev/null From 01f942c1228c12083b4deb3d1e3f0012d3021263 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 00:29:17 -0700 Subject: [PATCH 06/18] fix(sync): stop asking whether the child is alive, and let go of the caller's streams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI shards were red for three reasons. Two were the feature, not the tests. 1. A bare kill -0. tests/test_instance_id.bats forbids it outside scripts/lib/instance-id.sh, because liveness has to go through _agmsg_pid_alive, which is EPERM-aware and cross-checks ps. My poll asked whether the background child was alive. The question was wrong anyway. What this needs to know is whether the child has FINISHED, and kill -0 succeeds for one that has exited and not been reaped. The child now writes its exit status to a sentinel as its last act and this polls for that file. No pid is examined at all. 2. THE ABANDONED CHILD HELD THE CALLER'S STDOUT. A start that outruns the budget is deliberately left running — and it inherited the streams of whatever called the hook. Anything that CAPTURES that output (run in a test, , a piped hook) then waits for EOF, and a start that hangs hangs the session. That is the requirement this budget exists for, broken where no exit code and no timeout could see it. It surfaced as a suite whose cases were all green and which never finished. The child is now detached: ( ... ) /dev/null 2>&1 & 3. teardown reaped by the name the fake was WRITTEN as, while the hanging one runs as a copy at /remote.sh. Nothing was killed and the child outlived the file. Reaped by / now, which cannot reach anything outside the test's own tree. Also: the cases whose subject is not timing now drive a fake remote.sh that answers instantly. Using the real command made them slow and load-dependent, and raising their budgets only bought a suite that was slow AND fragile. The real command is kept for the race case, where inheriting its lock is the whole point. tests/test_sync_autostart.bats: 10 tests, 0 failures, exit 0. --- scripts/lib/sync-autostart.sh | 41 ++++++++++++++----- tests/test_sync_autostart.bats | 72 ++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 34 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 96d4f5bae..0cfbaf5ad 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -61,7 +61,7 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc pid tmp started="" failed="" slow="" + local team out rc tmp started="" failed="" slow="" for team in "$@"; do [ -n "$team" ] || continue tmp="$(mktemp 2>/dev/null)" || tmp="" @@ -69,21 +69,42 @@ agmsg_sync_autostart() { # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that # sentence is the useful half of a failure. Swallowing it would leave this # printing "could not start" with the reason on the floor. - "$remote_sh" sync start "$team" >"$tmp" 2>&1 & - pid=$! - while kill -0 "$pid" 2>/dev/null && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do + # THE QUESTION IS "HAS IT FINISHED?", NOT "IS IT ALIVE?". + # + # The child writes its exit status to a sentinel as its last act, and this + # polls for the sentinel. No pid is examined, so no liveness check is made + # — which is what `scripts/lib/instance-id.sh`'s `_agmsg_pid_alive` exists + # to own, and what a bare `kill -0` here would have duplicated badly (a + # repo-wide check catches that; mine reached CI before I did). + # + # It is also the more exact question. `kill -0` succeeds for a child that + # has exited and not been reaped, so polling liveness would have waited + # past the moment the answer was available. + # DETACHED FROM THIS CALLER'S STREAMS, and that is not tidiness. + # + # The child is deliberately allowed to outlive this function. If it still + # holds the caller's stdout, anything that CAPTURES that output — `run` in + # a test, `$(...)`, a hook whose output is piped — waits for EOF, and a + # start that hangs then hangs the session. That is the requirement this + # whole budget exists for, broken in a way no exit code and no timeout + # here could see: I measured it as a suite that stopped finishing. + # + # stdin too: a child left on a terminal can stop for input. + ( "$remote_sh" sync start "$team" >"$tmp" 2>&1; printf '%s\n' "$?" > "$tmp.rc" ) /dev/null 2>&1 & + while [ ! -f "$tmp.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do sleep 0.1 done - if kill -0 "$pid" 2>/dev/null; then - # Budget spent. NOT killed — see the header. The temp file is left for the - # child to finish writing into; it is in the system temp dir and is the - # price of not truncating a start that may be about to succeed. + if [ ! -f "$tmp.rc" ]; then + # Budget spent. The child is NOT killed — see the header — so its two + # temp files are left for it to finish writing into. They are in the + # system temp directory, and that is the price of not truncating a start + # that may be about to succeed. slow="$slow$team"$'\n' continue fi - wait "$pid"; rc=$? + rc="$(cat "$tmp.rc" 2>/dev/null || printf '1')" out="$(cat "$tmp" 2>/dev/null)" - rm -f "$tmp" + rm -f "$tmp" "$tmp.rc" if [ "$rc" -eq 0 ] && printf '%s' "$out" | grep -q 'already running'; then continue fi diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 2d0eb23db..7db21732e 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -45,8 +45,12 @@ teardown() { # that is the behaviour under test. It must not outlive the test: a CI shard # runs many files in one process tree, and a fake that loops forever would # then be somebody else's flake (raised in review). - pkill -f "$TEST_SKILL_DIR/fake-remote.sh" 2>/dev/null || true - pkill -f "$TEST_SKILL_DIR/fake-node" 2>/dev/null || true + # BY THE PATH THEY ACTUALLY RUN UNDER. The hanging fake is COPIED over + # `$SCRIPTS/remote.sh`, so matching the name it was written as reaps nothing + # and the child outlives the whole file — which is how this suite stopped + # exiting even with every case green. Both paths are inside the test's own + # skill dir, so the pattern cannot reach anything else. + pkill -f "$TEST_SKILL_DIR/" 2>/dev/null || true teardown_test_env } @@ -106,6 +110,29 @@ wait_for_call() { return 1 } +# A `remote.sh` that answers instantly, for the cases where the SUBJECT is what +# the helper does with an answer — not how long the real command takes. +# +# The real command is kept for the race case below, which is about inheriting +# its lock. Everywhere else it only made the suite slow and timing-coupled: +# raising the budget so a case could not be cut short is the same admission, +# with a worse failure mode (a 60s case that goes red when the machine is busy). +write_answering_remote() { + local answer="$1" fake="$TEST_SKILL_DIR/fake-remote-answer.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' '[ "${1:-}" = "sync" ] || exit 0' + case "$answer" in + started) printf '%s\n' 'echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0' ;; + running) printf '%s\n' 'echo "Sync engine already running (pid 4242)."; exit 0' ;; + refused) printf '%s\n' 'echo "agmsg: team '"'"'$3'"'"' is disconnected; connect or pull it before starting sync" >&2; exit 1' ;; + broken) printf '%s\n' 'echo "engine exploded" >&2; exit 1' ;; + esac + } > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + collect_engine_pids() { local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" [ -f "$pidfile" ] && ENGINE_PIDS="$ENGINE_PIDS $(cat "$pidfile")" @@ -113,6 +140,9 @@ collect_engine_pids() { } @test "starts an engine for a connected team that has none" { + # THE REAL COMMAND, because this case asserts on the artifact it leaves: a + # pidfile naming a live process. The sentence is not the evidence. + export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 export AGMSG_NODE="$(write_fake_node)" source "$SCRIPTS/lib/sync-autostart.sh" run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam @@ -120,17 +150,16 @@ collect_engine_pids() { [ "$status" -eq 0 ] printf '%s' "$output" | grep -q 'started one for' printf '%s' "$output" | grep -q 'testteam' - # The artifact, not the sentence: a pidfile naming a live process. [ -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] - kill -0 "$(cat "$TEST_SKILL_DIR/run/remote-sync.testteam.pid")" + # Liveness through the shipped helper, not a bare kill -0 (a repo-wide check + # forbids the latter, and it caught this branch once already). + run bash -c 'source "'"$SCRIPTS"'/lib/instance-id.sh"; _agmsg_pid_alive "$(cat "'"$TEST_SKILL_DIR"'/run/remote-sync.testteam.pid")"' + [ "$status" -eq 0 ] } @test "says nothing at all when the engine is already running" { - export AGMSG_NODE="$(write_fake_node)" - bash "$SCRIPTS/remote.sh" sync start testteam - collect_engine_pids source "$SCRIPTS/lib/sync-autostart.sh" - run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + run agmsg_sync_autostart "$(write_answering_remote running)" testteam [ "$status" -eq 0 ] # Starting is a side effect nobody asked for in this moment; "nothing # changed" is not news, and a line here would appear on every session start @@ -183,21 +212,19 @@ collect_engine_pids() { [ "$live" = "1" ] } -@test "a team that is disconnected is not started, and the refusal is shown" { - export AGMSG_NODE="$(write_fake_node)" - local cfg="$TEST_SKILL_DIR/teams/testteam/config.json" escaped updated - escaped="$(sed "s/'/''/g" "$cfg")" - updated="$(sqlite_mem " - SELECT json_set('$escaped', '\$.remote_binding.disconnected_at', '2026-08-01T00:00:00Z');")" - printf '%s\n' "$updated" > "$cfg" - +@test "a refusal from the command is repeated, not replaced" { + # THE SUBJECT IS THE HELPER'S HANDLING of a refusal, so the refusal is given + # to it directly. Driving the real command here made the case depend on how + # busy the machine was — it went green alone and red in the full file — and + # raising the budget only made it slow instead of wrong. + # + # The binding check itself belongs to `cmd_sync_start` and is tested where it + # lives; what is asserted here is that its sentence survives. source "$SCRIPTS/lib/sync-autostart.sh" - run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + run agmsg_sync_autostart "$(write_answering_remote refused)" testteam [ "$status" -eq 0 ] - # The binding check is the COMMAND's, inherited: it refuses by name before it - # starts anything, and the reason it gave is repeated rather than replaced. printf '%s' "$output" | grep -q 'disconnected' - [ ! -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] + printf '%s' "$output" | grep -q 'connected, but not syncing' } @test "a start that fails does not fail the caller, and says what the command said" { @@ -210,11 +237,8 @@ collect_engine_pids() { # is TRUE and is a different sentence. The two outcomes are tested # separately rather than folded together: "it failed" and "it has not # answered yet" are different facts and the tool says different things. - export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 - export AGMSG_NODE="$(write_failing_node)" - source "$SCRIPTS/lib/sync-autostart.sh" - run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + run agmsg_sync_autostart "$(write_answering_remote broken)" testteam [ "$status" -eq 0 ] printf '%s' "$output" | grep -q 'connected, but not syncing' printf '%s' "$output" | grep -q 'The session continues.' From 2e6c3a065691637015d0fb79fcaf6f12485cc4dd Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 04:39:15 -0700 Subject: [PATCH 07/18] fix(sync): close every inherited descriptor in the background start, not just 0/1/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shard check found it: 'every background spawn under scripts/ closes bats fd 3 and fd 4' failed on ubuntu 3/4 and macos 3/4. I had already found that the abandoned child held the caller's stdout and detached it with /dev/null 2>&1. That was necessary and not sufficient. bats hands a harness pipe down on fd 3 and 4, and a child holding those keeps the shard alive after every case in it has passed — the same symptom, one layer further out. scripts/lib/close-fds.sh exists for exactly this leak, and its own comment records the last time: the codex bridge closed 3 and 4 by name while remote-sync.sh had the range close, so the bridge kept the harness pipe and hung a shard. I fell into the same hole from a different spawn path. agmsg_close_inherited_fds is called INSIDE the subshell, which closes the child's copies and leaves this shell's own descriptors alone — the pattern that file's comment prescribes, and the reason the rule has no exceptions to audit. tests/test_close_fds.bats: 4 tests, 0 failures. --- scripts/lib/sync-autostart.sh | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 0cfbaf5ad..6d27d1dbb 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -53,6 +53,14 @@ # moment, and "nothing changed" is not news. agmsg_sync_autostart() { local remote_sh="$1"; shift + # Sourced here rather than at file scope: this file is sourced by two hooks, + # and pulling in a second file at their top level is a cost they pay whether + # or not anything is started. + if ! declare -F agmsg_close_inherited_fds >/dev/null 2>&1; then + local _lib_dir="${BASH_SOURCE[0]%/*}" + # shellcheck source=scripts/lib/close-fds.sh + [ -r "$_lib_dir/close-fds.sh" ] && . "$_lib_dir/close-fds.sh" + fi [ -x "$remote_sh" ] || return 0 [ $# -gt 0 ] || return 0 @@ -90,7 +98,22 @@ agmsg_sync_autostart() { # here could see: I measured it as a suite that stopped finishing. # # stdin too: a child left on a terminal can stop for input. - ( "$remote_sh" sync start "$team" >"$tmp" 2>&1; printf '%s\n' "$?" > "$tmp.rc" ) /dev/null 2>&1 & + ( + # EVERY INHERITED DESCRIPTOR, not just 0/1/2. + # + # Detaching stdin/stdout/stderr was necessary and not sufficient: bats + # hands a harness pipe down on fd 3 and 4, and a child that keeps them + # open holds the shard after every case has passed. `scripts/lib/close-fds.sh` + # exists because that exact leak hung a shard once already, from a + # different spawn path — and a repo-wide check found mine. + # + # Called INSIDE the subshell so it closes the child's copies and leaves + # this shell's own descriptors alone, which is the pattern that file's + # own comment prescribes. + agmsg_close_inherited_fds + "$remote_sh" sync start "$team" >"$tmp" 2>&1 + printf '%s\n' "$?" > "$tmp.rc" + ) /dev/null 2>&1 & while [ ! -f "$tmp.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do sleep 0.1 done From ae2a915162961ef36c7004e5f33c033b64f2a403 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 13:19:22 -0700 Subject: [PATCH 08/18] feat(sync): do not start a team whose server has refused (#773) --- scripts/lib/sync-autostart.sh | 40 +++++++++++++++- tests/test_sync_autostart.bats | 85 ++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 6d27d1dbb..3dd1be267 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,9 +69,31 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp started="" failed="" slow="" + local team out rc tmp refusal_line started="" failed="" slow="" refused="" for team in "$@"; do [ -n "$team" ] || continue + # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). + # + # Without this, auto-start is a restart loop: the engine exits on a refusal + # the caller has to act on, this starts it again on the next session, it + # exits again. #773 kept the engine up and recorded the refusal where a + # reader can find it; this is that reader, at the one moment a start would + # otherwise be attempted. + # + # The `refused:` line is `remote.sh status`'s, and it repeats what the + # server said and nothing else. Reading the human line rather than the JSON + # keeps this free of a JSON parser in the session's critical path, and that + # line is under test in `tests/test_remote_refusal.bats`, so it is a + # contract rather than a formatting accident. + # + # The record is already compared against the last successful cycle by the + # reader, so a refusal that a later success reversed does not appear here — + # this does not re-decide staleness, and must not. + refusal_line="$("$remote_sh" status "$team" 2>/dev/null | grep -E '^[[:space:]]*refused:' | head -1)" + if [ -n "$refusal_line" ]; then + refused="$refused$team $(printf '%s' "$refusal_line" | sed 's/^[[:space:]]*//')"$'\n' + continue + fi tmp="$(mktemp 2>/dev/null)" || tmp="" [ -n "$tmp" ] || return 0 # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that @@ -166,6 +188,22 @@ agmsg_sync_autostart() { printf '\n' fi + if [ -n "$refused" ]; then + # THE SERVER'S SENTENCE, AND NOTHING ADDED TO IT. + # + # This client talks to whatever remote it was pointed at — self-hosted, + # somebody else's, or a service — and it cannot know why that one refused. + # A sentence invented here is wrong for some server, and the operator of + # that server is the only one who can write the right one. So the line is + # repeated as `status` printed it, and the remedy offered is to ask them. + printf '%s\n' 'AGMSG: not starting a sync engine — the server refused:' '' + printf '%s' "$refused" | while IFS=$'\t' read -r t line; do + [ -n "$t" ] || continue + printf ' %s: %s\n' "$t" "$line" + done + printf '%s\n' '' 'The engine is not started while that stands. The session continues.' '' + fi + if [ -n "$failed" ]; then printf '%s\n' 'AGMSG: connected, but not syncing.' '' printf '%s\n' \ diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 7db21732e..35a68cd28 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -375,3 +375,88 @@ write_fake_remote() { printf '%s' "$output" | grep -q 'status=ok' [ "$status" -eq 0 ] } + +# --- #773: a team the server has refused is not started --------------------- +# +# Auto-start plus an engine that exits on a refusal is a restart loop: start, +# refuse, exit, start again next session. #773 kept the engine up and put the +# refusal where a reader can find it; this is the reader, at the one moment a +# start would otherwise be attempted. + +# A `remote.sh` that reports a refusal from `status`, and RECORDS whether it was +# ever asked to start anything. The record is the point: "did not start it" is +# the claim, and an output check alone cannot tell "not started" from "started +# and said nothing". +write_refusing_status_remote() { + local line="$1" fake="$TEST_SKILL_DIR/fake-remote-refusal.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'case "${1:-}" in' + printf '%s\n' " status) printf '%s\\n' \" $line\"; exit 0 ;;" + printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' + printf '%s\n' 'esac' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +@test "autostart: a team the server has refused is never offered to sync start (#773)" { + source "$SCRIPTS/lib/sync-autostart.sh" + export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" + : > "$AGMSG_TEST_START_CALLS" + local fake; fake="$(write_refusing_status_remote 'refused: the server answered 402 payment_required (sync.example.test)')" + + run agmsg_sync_autostart "$fake" testteam + [ "$status" -eq 0 ] + + # The claim is "it was not started", so the call record is what is asserted. + [ ! -s "$AGMSG_TEST_START_CALLS" ] + # And the reason reaches the operator, in the server's words. + printf '%s' "$output" | grep -q 'the server refused' + printf '%s' "$output" | grep -q '402 payment_required' + printf '%s' "$output" | grep -q 'sync.example.test' + # Nothing invented about what it MEANS. Each of these is a sentence only the + # operator of that server may write. + refute grep -qi 'subscri' <<<"$output" + refute grep -qi 'upgrade' <<<"$output" + refute grep -qi 'billing' <<<"$output" + refute grep -qi 'plan' <<<"$output" +} + +@test "autostart: a status this client never enumerated still stops the start (#773)" { + # BY THE LINE, NOT BY THE NUMBER. A self-hosted server refuses for its own + # reasons with codes nothing here has heard of. + source "$SCRIPTS/lib/sync-autostart.sh" + export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" + : > "$AGMSG_TEST_START_CALLS" + local fake; fake="$(write_refusing_status_remote 'refused: the server answered 451 tenant_suspended_by_operator (sync.example.test)')" + + run agmsg_sync_autostart "$fake" testteam + [ ! -s "$AGMSG_TEST_START_CALLS" ] + printf '%s' "$output" | grep -q '451 tenant_suspended_by_operator' +} + +@test "autostart: with no refusal recorded, the team IS started (#773 negative control)" { + # Without this, the two cases above are satisfied by a helper that never + # starts anything at all. + source "$SCRIPTS/lib/sync-autostart.sh" + export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" + : > "$AGMSG_TEST_START_CALLS" + local fake="$TEST_SKILL_DIR/fake-remote-clean.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'case "${1:-}" in' + printf '%s\n' ' status) printf "%s\n" " testteam connected since 2026-08-01"; exit 0 ;;' + printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' + printf '%s\n' 'esac' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + + run agmsg_sync_autostart "$fake" testteam + [ "$status" -eq 0 ] + grep -q '^testteam$' "$AGMSG_TEST_START_CALLS" + printf '%s' "$output" | grep -q 'started one for' + refute grep -q 'the server refused' <<<"$output" +} From ded2f76826ee6e61ce8a6a597048ab3fc73fe3cb Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 13:35:41 -0700 Subject: [PATCH 09/18] fix(sync): put the refusal lookup under the same budget as the start --- scripts/lib/sync-autostart.sh | 40 ++++++++++++++++++++++++++++++++-- tests/test_sync_autostart.bats | 30 +++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 3dd1be267..daafd649c 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,7 +69,7 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp refusal_line started="" failed="" slow="" refused="" + local team out rc tmp probe refusal_line started="" failed="" slow="" refused="" for team in "$@"; do [ -n "$team" ] || continue # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). @@ -89,7 +89,43 @@ agmsg_sync_autostart() { # The record is already compared against the last successful cycle by the # reader, so a refusal that a later success reversed does not appear here — # this does not re-decide staleness, and must not. - refusal_line="$("$remote_sh" status "$team" 2>/dev/null | grep -E '^[[:space:]]*refused:' | head -1)" + # UNDER THE SAME BUDGET AS THE START, for the same reason. + # + # The first version of this ran `status` synchronously, before the budget + # was consulted at all. `status` opens a pidfile, a cycle stamp and a + # refusal record — all local reads, all fast — but "fast" is a claim about + # a machine, and this sits in the path where a session prints its Monitor + # directive and where `actas` prints `status=ok`. An unbounded call there + # is exactly the defect the background start exists to prevent, put back + # one line above it (raised in review). + # + # On timeout the answer is UNKNOWN, and unknown proceeds to the start. The + # start is itself bounded, and a team that really is refused will be + # refused again — visibly, by the command, with the server's own sentence. + # Skipping the start instead would let a slow local read stop syncing, + # which is the failure #774 exists to remove. + probe="$(mktemp 2>/dev/null)" || probe="" + refusal_line="" + if [ -n "$probe" ]; then + ( + agmsg_close_inherited_fds + "$remote_sh" status "$team" >"$probe" 2>/dev/null + printf '%s\n' done > "$probe.rc" + ) /dev/null 2>&1 & + while [ ! -f "$probe.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do + sleep 0.1 + done + if [ -f "$probe.rc" ]; then + # `|| true` because a grep that matches nothing exits 1, and this + # helper's contract with both hooks is that it never returns non-zero + # and never trips a caller's errexit. Relying on the caller's own + # `|| true` would make that contract theirs to keep. + refusal_line="$(grep -E '^[[:space:]]*refused:' "$probe" 2>/dev/null | head -1 || true)" + rm -f "$probe" "$probe.rc" + fi + # No `rm` on the timeout path: the child still owns those two files, and + # removing them under it is how a half-written answer becomes a wrong one. + fi if [ -n "$refusal_line" ]; then refused="$refused$team $(printf '%s' "$refusal_line" | sed 's/^[[:space:]]*//')"$'\n' continue diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 35a68cd28..e3b5cb9ee 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -460,3 +460,33 @@ write_refusing_status_remote() { printf '%s' "$output" | grep -q 'started one for' refute grep -q 'the server refused' <<<"$output" } + +@test "autostart: a status that hangs does not hold the session (#773 under the same budget)" { + # The refusal lookup asks the same command the rest of this file drives, and + # it runs in the session's critical path. An unbounded call there is the + # defect the background start exists to prevent, one line above it. + # + # Bound checked from the OUTSIDE, by wall clock, because a bound that only + # exists in the source is not a bound. + source "$SCRIPTS/lib/sync-autostart.sh" + export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" + : > "$AGMSG_TEST_START_CALLS" + local fake="$TEST_SKILL_DIR/fake-remote-hanging-status.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'case "${1:-}" in' + printf '%s\n' ' status) while :; do sleep 1; done ;;' + printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' + printf '%s\n' 'esac' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + + local began=$SECONDS + AGMSG_SYNC_AUTOSTART_TIMEOUT_S=2 run agmsg_sync_autostart "$fake" testteam + local took=$((SECONDS - began)) + + [ "$status" -eq 0 ] + # The budget is 2s; anything near the fake's forever is the bound missing. + [ "$took" -lt 10 ] +} From 1ee8ed45c89398075eda2455a9abc512d1ce1d4e Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 14:26:23 -0700 Subject: [PATCH 10/18] fix(sync): reap a timed-out refusal probe instead of leaving it to the machine --- scripts/lib/sync-autostart.sh | 23 ++++++++++++++++++++--- tests/test_sync_autostart.bats | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index daafd649c..731a42210 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,7 +69,7 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp probe refusal_line started="" failed="" slow="" refused="" + local team out rc tmp probe probe_pid _pp refusal_line started="" failed="" slow="" refused="" for team in "$@"; do [ -n "$team" ] || continue # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). @@ -112,9 +112,28 @@ agmsg_sync_autostart() { "$remote_sh" status "$team" >"$probe" 2>/dev/null printf '%s\n' done > "$probe.rc" ) /dev/null 2>&1 & + probe_pid=$! while [ ! -f "$probe.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do sleep 0.1 done + if [ ! -f "$probe.rc" ]; then + # A TIMED-OUT PROBE IS REAPED. The start is not, and the difference is + # what each one is in the middle of: a start may be a moment away from + # having an engine and a pidfile, so killing it can leave half of one + # behind. A `status` READS — there is nothing half-made to protect, and + # a probe left running is a process and two temp paths added by every + # session and every actas, for as long as the machine is up (raised in + # review, and it was the start's rule applied where it does not belong). + # + # The command itself, then the subshell holding it. `pgrep` is absent + # on some runtimes; there the subshell is still reaped and the command + # is left to the same fate as any orphan, which is no worse than before. + for _pp in $(pgrep -P "$probe_pid" 2>/dev/null); do + kill "$_pp" 2>/dev/null || true + done + kill "$probe_pid" 2>/dev/null || true + rm -f "$probe" "$probe.rc" + fi if [ -f "$probe.rc" ]; then # `|| true` because a grep that matches nothing exits 1, and this # helper's contract with both hooks is that it never returns non-zero @@ -123,8 +142,6 @@ agmsg_sync_autostart() { refusal_line="$(grep -E '^[[:space:]]*refused:' "$probe" 2>/dev/null | head -1 || true)" rm -f "$probe" "$probe.rc" fi - # No `rm` on the timeout path: the child still owns those two files, and - # removing them under it is how a half-written answer becomes a wrong one. fi if [ -n "$refusal_line" ]; then refused="$refused$team $(printf '%s' "$refusal_line" | sed 's/^[[:space:]]*//')"$'\n' diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index e3b5cb9ee..a1b018e41 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -489,4 +489,22 @@ write_refusing_status_remote() { [ "$status" -eq 0 ] # The budget is 2s; anything near the fake's forever is the bound missing. [ "$took" -lt 10 ] + + # AND NOTHING IS LEFT BEHIND. Bounding the caller while the probe runs for + # ever adds a process and two temp paths to every session and every actas, + # which is a leak measured in machine uptime rather than in one run. A + # `status` reads, so there is nothing half-made to protect by leaving it. + # Matched by the path THIS TEST'S fake actually runs under, not by its bare + # name. A bare name matches a stray from any other run in the same process + # tree -- a CI shard runs many files in one -- and this assertion then goes + # red for somebody else's leftover. It did exactly that here, against a + # leftover from an earlier experiment of my own, and passed under `--filter` + # while failing in the full file: the same coupling this suite already fixed + # in the other direction. + local i alive=1 + for i in $(seq 1 50); do + pgrep -f "$TEST_SKILL_DIR/fake-remote-hanging-status" >/dev/null 2>&1 || { alive=0; break; } + sleep 0.1 + done + [ "$alive" -eq 0 ] } From db92641e7916a21f4ad1ea1926a4d4653c6505e7 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 14:35:54 -0700 Subject: [PATCH 11/18] fix(sync): let the probe bound itself, and reap it before removing its files --- scripts/lib/sync-autostart.sh | 42 ++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 731a42210..8971e72a7 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,7 +69,7 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp probe probe_pid _pp refusal_line started="" failed="" slow="" refused="" + local team out rc tmp probe probe_pid refusal_line started="" failed="" slow="" refused="" for team in "$@"; do [ -n "$team" ] || continue # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). @@ -109,7 +109,20 @@ agmsg_sync_autostart() { if [ -n "$probe" ]; then ( agmsg_close_inherited_fds - "$remote_sh" status "$team" >"$probe" 2>/dev/null + # THE PROBE BOUNDS ITSELF, so the bound does not depend on this side + # reaching it, and does not depend on `pgrep` existing to find the + # command afterwards. A watchdog sibling signals the read at the same + # budget; whichever finishes first, the sentinel is written and this + # subshell exits. The earlier version killed from the outside and, on + # a runtime without `pgrep`, left the actual `status` running for ever + # (raised in review) -- a leak measured in machine uptime. + "$remote_sh" status "$team" >"$probe" 2>/dev/null & + _read_pid=$! + ( sleep "$budget"; kill "$_read_pid" 2>/dev/null || true ) & + _dog_pid=$! + wait "$_read_pid" 2>/dev/null || true + kill "$_dog_pid" 2>/dev/null || true + wait "$_dog_pid" 2>/dev/null || true printf '%s\n' done > "$probe.rc" ) /dev/null 2>&1 & probe_pid=$! @@ -117,21 +130,17 @@ agmsg_sync_autostart() { sleep 0.1 done if [ ! -f "$probe.rc" ]; then - # A TIMED-OUT PROBE IS REAPED. The start is not, and the difference is - # what each one is in the middle of: a start may be a moment away from - # having an engine and a pidfile, so killing it can leave half of one - # behind. A `status` READS — there is nothing half-made to protect, and - # a probe left running is a process and two temp paths added by every - # session and every actas, for as long as the machine is up (raised in - # review, and it was the start's rule applied where it does not belong). + # The wrapper missed its own deadline, which means the wrapper itself + # is wedged rather than the read. Signal it, WAIT for it, and only then + # remove the paths: removing them while it may still be running is how + # a file comes back after it was deleted (raised in review). # - # The command itself, then the subshell holding it. `pgrep` is absent - # on some runtimes; there the subshell is still reaped and the command - # is left to the same fate as any orphan, which is no worse than before. - for _pp in $(pgrep -P "$probe_pid" 2>/dev/null); do - kill "$_pp" 2>/dev/null || true - done + # A timed-out probe is reaped; a timed-out START is not, and the reason + # is what each is in the middle of. A start may be a moment away from + # having an engine and a pidfile, so killing it can leave half of one. + # A `status` reads, and has nothing half-made to protect. kill "$probe_pid" 2>/dev/null || true + wait "$probe_pid" 2>/dev/null || true rm -f "$probe" "$probe.rc" fi if [ -f "$probe.rc" ]; then @@ -140,6 +149,9 @@ agmsg_sync_autostart() { # and never trips a caller's errexit. Relying on the caller's own # `|| true` would make that contract theirs to keep. refusal_line="$(grep -E '^[[:space:]]*refused:' "$probe" 2>/dev/null | head -1 || true)" + # The wrapper has written its sentinel, so it is finished; reaped here + # so a session that starts many teams does not accumulate zombies. + wait "$probe_pid" 2>/dev/null || true rm -f "$probe" "$probe.rc" fi fi From 0b6a99ad4e583065d4c6627ded6fbc8e4d88c387 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 14:40:35 -0700 Subject: [PATCH 12/18] fix(sync): give both sides of the probe the same remaining seconds --- scripts/lib/sync-autostart.sh | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 8971e72a7..b15e35986 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,7 +69,7 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp probe probe_pid refusal_line started="" failed="" slow="" refused="" + local team out rc tmp probe probe_pid remaining refusal_line started="" failed="" slow="" refused="" for team in "$@"; do [ -n "$team" ] || continue # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). @@ -104,8 +104,22 @@ agmsg_sync_autostart() { # refused again — visibly, by the command, with the server's own sentence. # Skipping the start instead would let a slow local read stop syncing, # which is the failure #774 exists to remove. - probe="$(mktemp 2>/dev/null)" || probe="" + # ONE DEADLINE, SHARED BY BOTH SIDES. + # + # The watchdog inside the probe used to sleep the WHOLE per-call budget + # while this loop allowed only what was left of it. On the second team, or + # at the first team's scheduling boundary, the outer deadline arrives first, + # kills the wrapper, and the read it was watching is orphaned with a + # watchdog that will never reach it — the leak returns exactly where a + # multi-team session needs it not to (raised in review). Both sides are + # given the same remaining seconds, computed once, from one origin. + # + # With nothing left, no probe is started at all: the answer is unknown, and + # unknown proceeds to the start, as it does on timeout. + remaining=$(( budget - (SECONDS - elapsed_start) )) + probe="" refusal_line="" + [ "$remaining" -gt 0 ] && { probe="$(mktemp 2>/dev/null)" || probe=""; } if [ -n "$probe" ]; then ( agmsg_close_inherited_fds @@ -118,7 +132,7 @@ agmsg_sync_autostart() { # (raised in review) -- a leak measured in machine uptime. "$remote_sh" status "$team" >"$probe" 2>/dev/null & _read_pid=$! - ( sleep "$budget"; kill "$_read_pid" 2>/dev/null || true ) & + ( sleep "$remaining"; kill "$_read_pid" 2>/dev/null || true ) & _dog_pid=$! wait "$_read_pid" 2>/dev/null || true kill "$_dog_pid" 2>/dev/null || true From 25e65ff75e10ec32202e47bc5d4f5709a5bd57b9 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 14:41:30 -0700 Subject: [PATCH 13/18] test(sync): the second team is where the two clocks separate --- tests/test_sync_autostart.bats | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index a1b018e41..b45f430cb 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -508,3 +508,39 @@ write_refusing_status_remote() { done [ "$alive" -eq 0 ] } + +@test "autostart: with several teams, a hanging status leaves nothing behind for any of them (#773)" { + # The per-team probe and the whole-call budget have to be measured from the + # same origin. When the watchdog inside the probe slept the FULL budget while + # this side allowed only what was left of it, the second team's outer + # deadline arrived first, killed the wrapper, and orphaned the read it was + # watching — with a watchdog that would never reach it. One team could not + # show that; the second is where the two clocks separate. + source "$SCRIPTS/lib/sync-autostart.sh" + export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" + : > "$AGMSG_TEST_START_CALLS" + local fake="$TEST_SKILL_DIR/fake-remote-hanging-status.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'case "${1:-}" in' + printf '%s\n' ' status) while :; do sleep 1; done ;;' + printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' + printf '%s\n' 'esac' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + + local began=$SECONDS + AGMSG_SYNC_AUTOSTART_TIMEOUT_S=2 run agmsg_sync_autostart "$fake" teamone teamtwo teamthree + local took=$((SECONDS - began)) + [ "$status" -eq 0 ] + # One budget for the whole call, not one per team. + [ "$took" -lt 10 ] + + local i alive=1 + for i in $(seq 1 50); do + pgrep -f "$TEST_SKILL_DIR/fake-remote-hanging-status" >/dev/null 2>&1 || { alive=0; break; } + sleep 0.1 + done + [ "$alive" -eq 0 ] +} From 7d809db0ac1bbbb30c70637cbeb676ed26923952 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 14:56:56 -0700 Subject: [PATCH 14/18] test(sync): say what this case cannot see, having run the mutation that proves it --- tests/test_sync_autostart.bats | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index b45f430cb..025a55b7f 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -510,12 +510,21 @@ write_refusing_status_remote() { } @test "autostart: with several teams, a hanging status leaves nothing behind for any of them (#773)" { - # The per-team probe and the whole-call budget have to be measured from the - # same origin. When the watchdog inside the probe slept the FULL budget while - # this side allowed only what was left of it, the second team's outer - # deadline arrived first, killed the wrapper, and orphaned the read it was - # watching — with a watchdog that would never reach it. One team could not - # show that; the second is where the two clocks separate. + # What this DOES measure: a call over several teams spends one budget, not + # one per team, and leaves nothing running afterwards. + # + # What it does NOT measure, stated because a reader will assume otherwise: + # it does not catch the two clocks drifting apart. Giving the probe's own + # watchdog the full budget while this side allows only the remainder leaves + # this test green — with a `status` that never returns, the FIRST team + # consumes the whole budget, every later team gets zero remaining and starts + # no probe at all, so there is no second probe for the two clocks to disagree + # about. Measured, not assumed: that mutation was run and stayed green. + # + # The shared deadline is therefore justified by reading the code, not by this + # control. A control that does discriminate would need a first team that is + # slow-but-finishing and a second that hangs, which is a timing construction + # of exactly the kind this file has twice been told not to build. source "$SCRIPTS/lib/sync-autostart.sh" export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" : > "$AGMSG_TEST_START_CALLS" From 2e559f27334be9309dac7694eb6924c60e329a56 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 15:05:57 -0700 Subject: [PATCH 15/18] revert(sync): drop the refusal lookup; the loop it prevented no longer exists --- scripts/lib/sync-autostart.sh | 131 +++-------------------- tests/test_sync_autostart.bats | 189 +++------------------------------ 2 files changed, 29 insertions(+), 291 deletions(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index b15e35986..1cdd970f0 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -69,110 +69,27 @@ agmsg_sync_autostart() { local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" local elapsed_start=$SECONDS - local team out rc tmp probe probe_pid remaining refusal_line started="" failed="" slow="" refused="" + local team out rc tmp started="" failed="" slow="" for team in "$@"; do [ -n "$team" ] || continue - # A TEAM THE SERVER HAS REFUSED IS NOT STARTED (#773). + # WHY THERE IS NO REFUSAL CHECK HERE, having had one (#773). # - # Without this, auto-start is a restart loop: the engine exits on a refusal - # the caller has to act on, this starts it again on the next session, it - # exits again. #773 kept the engine up and recorded the refusal where a - # reader can find it; this is that reader, at the one moment a start would - # otherwise be attempted. + # The version of this that read `remote.sh status` before each start + # existed to stop a restart loop: the engine used to EXIT when the server + # refused, so auto-start would raise it again on the next session and it + # would exit again. # - # The `refused:` line is `remote.sh status`'s, and it repeats what the - # server said and nothing else. Reading the human line rather than the JSON - # keeps this free of a JSON parser in the session's critical path, and that - # line is under test in `tests/test_remote_refusal.bats`, so it is a - # contract rather than a formatting accident. + # #792 removed that. The engine now records the refusal, backs off to its + # longest interval and keeps the loop — `sleepCall(MAX_BACKOFF_MS); + # continue;` — so starting a refused team costs one quiet process that + # reports the reason through `status`, and there is no loop to prevent. # - # The record is already compared against the last successful cycle by the - # reader, so a refusal that a later success reversed does not appear here — - # this does not re-decide staleness, and must not. - # UNDER THE SAME BUDGET AS THE START, for the same reason. - # - # The first version of this ran `status` synchronously, before the budget - # was consulted at all. `status` opens a pidfile, a cycle stamp and a - # refusal record — all local reads, all fast — but "fast" is a claim about - # a machine, and this sits in the path where a session prints its Monitor - # directive and where `actas` prints `status=ok`. An unbounded call there - # is exactly the defect the background start exists to prevent, put back - # one line above it (raised in review). - # - # On timeout the answer is UNKNOWN, and unknown proceeds to the start. The - # start is itself bounded, and a team that really is refused will be - # refused again — visibly, by the command, with the server's own sentence. - # Skipping the start instead would let a slow local read stop syncing, - # which is the failure #774 exists to remove. - # ONE DEADLINE, SHARED BY BOTH SIDES. - # - # The watchdog inside the probe used to sleep the WHOLE per-call budget - # while this loop allowed only what was left of it. On the second team, or - # at the first team's scheduling boundary, the outer deadline arrives first, - # kills the wrapper, and the read it was watching is orphaned with a - # watchdog that will never reach it — the leak returns exactly where a - # multi-team session needs it not to (raised in review). Both sides are - # given the same remaining seconds, computed once, from one origin. - # - # With nothing left, no probe is started at all: the answer is unknown, and - # unknown proceeds to the start, as it does on timeout. - remaining=$(( budget - (SECONDS - elapsed_start) )) - probe="" - refusal_line="" - [ "$remaining" -gt 0 ] && { probe="$(mktemp 2>/dev/null)" || probe=""; } - if [ -n "$probe" ]; then - ( - agmsg_close_inherited_fds - # THE PROBE BOUNDS ITSELF, so the bound does not depend on this side - # reaching it, and does not depend on `pgrep` existing to find the - # command afterwards. A watchdog sibling signals the read at the same - # budget; whichever finishes first, the sentinel is written and this - # subshell exits. The earlier version killed from the outside and, on - # a runtime without `pgrep`, left the actual `status` running for ever - # (raised in review) -- a leak measured in machine uptime. - "$remote_sh" status "$team" >"$probe" 2>/dev/null & - _read_pid=$! - ( sleep "$remaining"; kill "$_read_pid" 2>/dev/null || true ) & - _dog_pid=$! - wait "$_read_pid" 2>/dev/null || true - kill "$_dog_pid" 2>/dev/null || true - wait "$_dog_pid" 2>/dev/null || true - printf '%s\n' done > "$probe.rc" - ) /dev/null 2>&1 & - probe_pid=$! - while [ ! -f "$probe.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do - sleep 0.1 - done - if [ ! -f "$probe.rc" ]; then - # The wrapper missed its own deadline, which means the wrapper itself - # is wedged rather than the read. Signal it, WAIT for it, and only then - # remove the paths: removing them while it may still be running is how - # a file comes back after it was deleted (raised in review). - # - # A timed-out probe is reaped; a timed-out START is not, and the reason - # is what each is in the middle of. A start may be a moment away from - # having an engine and a pidfile, so killing it can leave half of one. - # A `status` reads, and has nothing half-made to protect. - kill "$probe_pid" 2>/dev/null || true - wait "$probe_pid" 2>/dev/null || true - rm -f "$probe" "$probe.rc" - fi - if [ -f "$probe.rc" ]; then - # `|| true` because a grep that matches nothing exits 1, and this - # helper's contract with both hooks is that it never returns non-zero - # and never trips a caller's errexit. Relying on the caller's own - # `|| true` would make that contract theirs to keep. - refusal_line="$(grep -E '^[[:space:]]*refused:' "$probe" 2>/dev/null | head -1 || true)" - # The wrapper has written its sentinel, so it is finished; reaped here - # so a session that starts many teams does not accumulate zombies. - wait "$probe_pid" 2>/dev/null || true - rm -f "$probe" "$probe.rc" - fi - fi - if [ -n "$refusal_line" ]; then - refused="$refused$team $(printf '%s' "$refusal_line" | sed 's/^[[:space:]]*//')"$'\n' - continue - fi + # The check was not free. It put a second command in the path where a + # session prints its Monitor directive, and bounding it correctly took a + # background wrapper, a watchdog, a shared deadline, a grace period and a + # reaping rule — seven review rounds of machinery to make a lookup safe + # that the thing it protected against no longer needs. Reading the engine + # is what settled it, not the review count. tmp="$(mktemp 2>/dev/null)" || tmp="" [ -n "$tmp" ] || return 0 # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that @@ -267,22 +184,6 @@ agmsg_sync_autostart() { printf '\n' fi - if [ -n "$refused" ]; then - # THE SERVER'S SENTENCE, AND NOTHING ADDED TO IT. - # - # This client talks to whatever remote it was pointed at — self-hosted, - # somebody else's, or a service — and it cannot know why that one refused. - # A sentence invented here is wrong for some server, and the operator of - # that server is the only one who can write the right one. So the line is - # repeated as `status` printed it, and the remedy offered is to ask them. - printf '%s\n' 'AGMSG: not starting a sync engine — the server refused:' '' - printf '%s' "$refused" | while IFS=$'\t' read -r t line; do - [ -n "$t" ] || continue - printf ' %s: %s\n' "$t" "$line" - done - printf '%s\n' '' 'The engine is not started while that stands. The session continues.' '' - fi - if [ -n "$failed" ]; then printf '%s\n' 'AGMSG: connected, but not syncing.' '' printf '%s\n' \ diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index 025a55b7f..dd5f49a9f 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -376,180 +376,17 @@ write_fake_remote() { [ "$status" -eq 0 ] } -# --- #773: a team the server has refused is not started --------------------- +# --- #773: why there is no refusal check here ------------------------------ # -# Auto-start plus an engine that exits on a refusal is a restart loop: start, -# refuse, exit, start again next session. #773 kept the engine up and put the -# refusal where a reader can find it; this is the reader, at the one moment a -# start would otherwise be attempted. - -# A `remote.sh` that reports a refusal from `status`, and RECORDS whether it was -# ever asked to start anything. The record is the point: "did not start it" is -# the claim, and an output check alone cannot tell "not started" from "started -# and said nothing". -write_refusing_status_remote() { - local line="$1" fake="$TEST_SKILL_DIR/fake-remote-refusal.sh" - { - printf '%s\n' '#!/usr/bin/env bash' - printf '%s\n' 'case "${1:-}" in' - printf '%s\n' " status) printf '%s\\n' \" $line\"; exit 0 ;;" - printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' - printf '%s\n' 'esac' - printf '%s\n' 'exit 0' - } > "$fake" - chmod +x "$fake" - printf '%s\n' "$fake" -} - -@test "autostart: a team the server has refused is never offered to sync start (#773)" { - source "$SCRIPTS/lib/sync-autostart.sh" - export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" - : > "$AGMSG_TEST_START_CALLS" - local fake; fake="$(write_refusing_status_remote 'refused: the server answered 402 payment_required (sync.example.test)')" - - run agmsg_sync_autostart "$fake" testteam - [ "$status" -eq 0 ] - - # The claim is "it was not started", so the call record is what is asserted. - [ ! -s "$AGMSG_TEST_START_CALLS" ] - # And the reason reaches the operator, in the server's words. - printf '%s' "$output" | grep -q 'the server refused' - printf '%s' "$output" | grep -q '402 payment_required' - printf '%s' "$output" | grep -q 'sync.example.test' - # Nothing invented about what it MEANS. Each of these is a sentence only the - # operator of that server may write. - refute grep -qi 'subscri' <<<"$output" - refute grep -qi 'upgrade' <<<"$output" - refute grep -qi 'billing' <<<"$output" - refute grep -qi 'plan' <<<"$output" -} - -@test "autostart: a status this client never enumerated still stops the start (#773)" { - # BY THE LINE, NOT BY THE NUMBER. A self-hosted server refuses for its own - # reasons with codes nothing here has heard of. - source "$SCRIPTS/lib/sync-autostart.sh" - export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" - : > "$AGMSG_TEST_START_CALLS" - local fake; fake="$(write_refusing_status_remote 'refused: the server answered 451 tenant_suspended_by_operator (sync.example.test)')" - - run agmsg_sync_autostart "$fake" testteam - [ ! -s "$AGMSG_TEST_START_CALLS" ] - printf '%s' "$output" | grep -q '451 tenant_suspended_by_operator' -} - -@test "autostart: with no refusal recorded, the team IS started (#773 negative control)" { - # Without this, the two cases above are satisfied by a helper that never - # starts anything at all. - source "$SCRIPTS/lib/sync-autostart.sh" - export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" - : > "$AGMSG_TEST_START_CALLS" - local fake="$TEST_SKILL_DIR/fake-remote-clean.sh" - { - printf '%s\n' '#!/usr/bin/env bash' - printf '%s\n' 'case "${1:-}" in' - printf '%s\n' ' status) printf "%s\n" " testteam connected since 2026-08-01"; exit 0 ;;' - printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' - printf '%s\n' 'esac' - printf '%s\n' 'exit 0' - } > "$fake" - chmod +x "$fake" - - run agmsg_sync_autostart "$fake" testteam - [ "$status" -eq 0 ] - grep -q '^testteam$' "$AGMSG_TEST_START_CALLS" - printf '%s' "$output" | grep -q 'started one for' - refute grep -q 'the server refused' <<<"$output" -} - -@test "autostart: a status that hangs does not hold the session (#773 under the same budget)" { - # The refusal lookup asks the same command the rest of this file drives, and - # it runs in the session's critical path. An unbounded call there is the - # defect the background start exists to prevent, one line above it. - # - # Bound checked from the OUTSIDE, by wall clock, because a bound that only - # exists in the source is not a bound. - source "$SCRIPTS/lib/sync-autostart.sh" - export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" - : > "$AGMSG_TEST_START_CALLS" - local fake="$TEST_SKILL_DIR/fake-remote-hanging-status.sh" - { - printf '%s\n' '#!/usr/bin/env bash' - printf '%s\n' 'case "${1:-}" in' - printf '%s\n' ' status) while :; do sleep 1; done ;;' - printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' - printf '%s\n' 'esac' - printf '%s\n' 'exit 0' - } > "$fake" - chmod +x "$fake" - - local began=$SECONDS - AGMSG_SYNC_AUTOSTART_TIMEOUT_S=2 run agmsg_sync_autostart "$fake" testteam - local took=$((SECONDS - began)) - - [ "$status" -eq 0 ] - # The budget is 2s; anything near the fake's forever is the bound missing. - [ "$took" -lt 10 ] - - # AND NOTHING IS LEFT BEHIND. Bounding the caller while the probe runs for - # ever adds a process and two temp paths to every session and every actas, - # which is a leak measured in machine uptime rather than in one run. A - # `status` reads, so there is nothing half-made to protect by leaving it. - # Matched by the path THIS TEST'S fake actually runs under, not by its bare - # name. A bare name matches a stray from any other run in the same process - # tree -- a CI shard runs many files in one -- and this assertion then goes - # red for somebody else's leftover. It did exactly that here, against a - # leftover from an earlier experiment of my own, and passed under `--filter` - # while failing in the full file: the same coupling this suite already fixed - # in the other direction. - local i alive=1 - for i in $(seq 1 50); do - pgrep -f "$TEST_SKILL_DIR/fake-remote-hanging-status" >/dev/null 2>&1 || { alive=0; break; } - sleep 0.1 - done - [ "$alive" -eq 0 ] -} - -@test "autostart: with several teams, a hanging status leaves nothing behind for any of them (#773)" { - # What this DOES measure: a call over several teams spends one budget, not - # one per team, and leaves nothing running afterwards. - # - # What it does NOT measure, stated because a reader will assume otherwise: - # it does not catch the two clocks drifting apart. Giving the probe's own - # watchdog the full budget while this side allows only the remainder leaves - # this test green — with a `status` that never returns, the FIRST team - # consumes the whole budget, every later team gets zero remaining and starts - # no probe at all, so there is no second probe for the two clocks to disagree - # about. Measured, not assumed: that mutation was run and stayed green. - # - # The shared deadline is therefore justified by reading the code, not by this - # control. A control that does discriminate would need a first team that is - # slow-but-finishing and a second that hangs, which is a timing construction - # of exactly the kind this file has twice been told not to build. - source "$SCRIPTS/lib/sync-autostart.sh" - export AGMSG_TEST_START_CALLS="$TEST_SKILL_DIR/start-calls" - : > "$AGMSG_TEST_START_CALLS" - local fake="$TEST_SKILL_DIR/fake-remote-hanging-status.sh" - { - printf '%s\n' '#!/usr/bin/env bash' - printf '%s\n' 'case "${1:-}" in' - printf '%s\n' ' status) while :; do sleep 1; done ;;' - printf '%s\n' ' sync) printf "%s\n" "$3" >> "$AGMSG_TEST_START_CALLS"; echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0 ;;' - printf '%s\n' 'esac' - printf '%s\n' 'exit 0' - } > "$fake" - chmod +x "$fake" - - local began=$SECONDS - AGMSG_SYNC_AUTOSTART_TIMEOUT_S=2 run agmsg_sync_autostart "$fake" teamone teamtwo teamthree - local took=$((SECONDS - began)) - [ "$status" -eq 0 ] - # One budget for the whole call, not one per team. - [ "$took" -lt 10 ] - - local i alive=1 - for i in $(seq 1 50); do - pgrep -f "$TEST_SKILL_DIR/fake-remote-hanging-status" >/dev/null 2>&1 || { alive=0; break; } - sleep 0.1 - done - [ "$alive" -eq 0 ] -} +# An earlier version of this suite had three cases asserting that a team whose +# server had refused was never offered to `sync start`, plus two more bounding +# and reaping the lookup that made that possible. +# +# They are gone with the mechanism. The refusal check existed to stop a restart +# loop — the engine used to EXIT on a refusal — and #792 ended that: the engine +# records the refusal, backs off to its longest interval and keeps looping. A +# refused team costs one quiet process that reports the reason through +# `status`, so there is nothing here to prevent, and nothing to test. +# +# Recorded rather than silently dropped, because "there used to be a check" +# reads as an oversight to whoever finds this next. From 47a9670990a05856dbca38171113f8ffb9f5c5ee Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 15:16:01 -0700 Subject: [PATCH 16/18] fix(sync): close fd 3 and 4 on the spawn line, where the repo-wide check reads --- scripts/lib/sync-autostart.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh index 1cdd970f0..96c761da3 100644 --- a/scripts/lib/sync-autostart.sh +++ b/scripts/lib/sync-autostart.sh @@ -131,7 +131,12 @@ agmsg_sync_autostart() { agmsg_close_inherited_fds "$remote_sh" sync start "$team" >"$tmp" 2>&1 printf '%s\n' "$?" > "$tmp.rc" - ) /dev/null 2>&1 & + # The literal `3>&- 4>&-` as well as the call inside, because the repo-wide + # check reads the spawn LINE (tests/test_spawn_fd_guard.bats). Belt and + # braces is the right answer here anyway: the call closes whatever the + # runtime handed down, and the redirections say so where a reader — and + # that check — can see it without following a function. + ) /dev/null 2>&1 3>&- 4>&- & while [ ! -f "$tmp.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do sleep 0.1 done From 83be882f20b3a234ef4715ba2ee963412d8fe82e Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 15:46:26 -0700 Subject: [PATCH 17/18] test(delivery): force the failed start through the command, not through the machine --- tests/test_delivery.bats | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index 83a869f79..d99f562d3 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -510,8 +510,18 @@ eperm_pid() { # unchanged; only the reason they are reachable is new. This is a reversal of # a decision, not a test edited to fit new output (raised in review). # - # THE FAILURE IS FORCED, AND THAT IS ITS ONLY CAUSE. An unusable interpreter - # makes `sync start` fail immediately and for a named reason. + # THE FAILURE IS FORCED BY THE COMMAND ITSELF, not by its environment. + # + # It used to be forced with an unusable interpreter, on the reasoning that + # `sync start` would then "fail immediately and for a named reason". That is + # a claim about a machine, and it was false on one: on a macOS CI runner the + # command had not returned after SIXTY seconds, so the hook printed "a start + # is still in flight" — a different fact, tested elsewhere — and this case + # failed for a reason that had nothing to do with what it asserts. + # + # So `remote.sh` is replaced, for this half of the test, by one that answers + # `status` with a connected team and refuses `sync start` at once. The real + # one is restored before the section below, which needs it to SUCCEED. # # It used to be inherited instead — the fixture simply had no engine to start # — and that held only while this file ran alone: the case passed under @@ -523,8 +533,6 @@ eperm_pid() { # The budget is raised as well: under the 5s default a slow failure is # reported as "still in flight", which is a different fact and is tested # separately in tests/test_sync_autostart.bats. - export AGMSG_NODE="$TEST_SKILL_DIR/no-such-node-for-this-test" - export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/join.sh" team alice claude-code "$TEST_PROJECT" >/dev/null # NEGATIVE FIRST, on the state every ordinary machine is in: no connected @@ -550,6 +558,23 @@ eperm_pid() { ));")" printf '%s\n' "$updated" > "$cfg" + # The stub goes in HERE, after the negative half has run against the real + # command. Installed any earlier it would report a connected team before one + # exists, and the negative assertion — the one that catches a line printed + # unconditionally — would be testing the stub instead of the hook. + cp "$SCRIPTS/remote.sh" "$TEST_SKILL_DIR/remote.real.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'if [ "${1:-}" = "status" ] && [ -z "${2:-}" ]; then' + printf '%s\n' ' printf "team\tconnected since 2026-08-12\n"; exit 0' + printf '%s\n' 'fi' + printf '%s\n' 'if [ "${1:-}" = "sync" ]; then' + printf '%s\n' ' echo "agmsg: cannot start the sync engine for '"'"'$3'"'"': no runtime" >&2; exit 1' + printf '%s\n' 'fi' + printf '%s\n' 'exit 0' + } > "$SCRIPTS/remote.sh" + chmod +x "$SCRIPTS/remote.sh" + run env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/session-start.sh" claude-code "$TEST_PROJECT" Date: Fri, 14 Aug 2026 16:28:20 -0700 Subject: [PATCH 18/18] test(sync): count engines in this test's tree, not by a name anyone can share --- tests/test_sync_autostart.bats | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_sync_autostart.bats b/tests/test_sync_autostart.bats index dd5f49a9f..1dbc7ef8c 100644 --- a/tests/test_sync_autostart.bats +++ b/tests/test_sync_autostart.bats @@ -207,8 +207,15 @@ collect_engine_pids() { local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" [ -f "$pidfile" ] kill -0 "$(cat "$pidfile")" + # COUNTED IN THIS TEST'S OWN TREE. `fake-node` as a bare name matches any + # leftover from another run in the same process tree — a CI shard runs many + # files in one — so the count would include somebody else's engine and this + # assertion would fail for their leak rather than a second engine here. + # Measured: with two strays present on the machine it read 3 and went red, + # green on the run before, which is what a global pattern looks like from + # the inside. local live - live="$(pgrep -f "fake-node" 2>/dev/null | wc -l | tr -d ' ')" + live="$(pgrep -f "$TEST_SKILL_DIR/fake-node" 2>/dev/null | wc -l | tr -d ' ')" [ "$live" = "1" ] }