feat(sync): start a connected team's engine when an agent turns up

scripts/actas-claim.sh

@@ -97,6 +97,40 @@ 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.
+#
+# 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
+# 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'

scripts/lib/sync-autostart.sh

@@ -0,0 +1,211 @@
+# 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/<team>"`, 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, 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 <remote.sh path> <team>...
+#
+# 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
+  # 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
+
+  # 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 tmp started="" failed="" slow=""
+  for team in "$@"; do
+    [ -n "$team" ] || continue
+    # WHY THERE IS NO REFUSAL CHECK HERE, having had one (#773).
+    #
+    # 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.
+    #
+    # #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 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
+    # sentence is the useful half of a failure. Swallowing it would leave this
+    # printing "could not start" with the reason on the floor.
+    # 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.
+    (
+      # 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"
+    # 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 >/dev/null 2>&1 3>&- 4>&- &
+    while [ ! -f "$tmp.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do
+      sleep 0.1
+    done
+    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
+    rc="$(cat "$tmp.rc" 2>/dev/null || printf '1')"
+    out="$(cat "$tmp" 2>/dev/null)"
+    rm -f "$tmp" "$tmp.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 <team>"
+    # 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 "$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' \
+      '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 — 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
+
+  return 0
+}

scripts/session-start.sh

@@ -245,50 +245,51 @@ 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.
 #
-# 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.
+# 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 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`.
-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 <team>` once. A reader has to
-    # replace `<team>` before that works, and an unreplaced `<team>` 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