diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de584..b4477fed 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -2414,7 +2414,8 @@ cmd_status() { cmd_sync_start() { local team="${1:?Usage: remote.sh sync start }" cfg connected_at disconnected_at \ engine_state engine_pid started_pid ready_pid startup_nonce ready=0 i=0 \ - logfile log_offset=1 + logfile log_offset=1 readiness_max="${AGMSG_SYNC_READY_TRIES:-1600}" \ + readiness_budget="${AGMSG_SYNC_READY_SECONDS:-16}" readiness_started readiness_elapsed [ $# -eq 1 ] || { echo "Usage: remote.sh sync start " >&2; exit 1; } agmsg_validate_team_name "$team" || exit 1 agmsg_lock_acquire "$TEAMS_DIR/$team" || exit 1 @@ -2489,8 +2490,15 @@ cmd_sync_start() { # deciding whether to start and starting, and nothing after -- so that a marker # that is late or missing for ANY reason costs this caller its own wait and # not the rest of the machine. + # + # BUDGETED IN TIME, NOT ITERATIONS (#779). The 1600 attempts were documented + # as roughly sixteen seconds, but every turn also starts the status probe, + # tail, awk and sleep. That arithmetic holds only where they are free. Keep + # the attempt ceiling as a safety cap, but make the stated wait a wall-clock + # budget: whichever arrives first ends readiness polling. + readiness_started="$(date +%s)" agmsg_lock_release - while [ "$i" -lt 1600 ]; do + while [ "$i" -lt "$readiness_max" ]; do IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team") if [ "$engine_state" = "running" ] && [ "$ready_pid" = "$started_pid" ] && tail -c "+$log_offset" "$logfile" 2>/dev/null | @@ -2502,6 +2510,8 @@ cmd_sync_start() { break fi i=$((i + 1)) + readiness_elapsed=$(( $(date +%s) - readiness_started )) + [ "$readiness_elapsed" -ge "$readiness_budget" ] && break sleep 0.01 done if [ "$ready" -ne 1 ]; then diff --git a/tests/test_remote_engine_start_refusal.bats b/tests/test_remote_engine_start_refusal.bats index f5e76731..8b48ecc6 100644 --- a/tests/test_remote_engine_start_refusal.bats +++ b/tests/test_remote_engine_start_refusal.bats @@ -391,3 +391,65 @@ skip_if_root() { rmdir "$lock" 2>/dev/null || true } + +@test "sync start: readiness cleanup is bounded by the wall clock (#779)" { + # The readiness loop starts a status probe, tail, awk and sleep on every + # turn. Counting 1600 turns as sixteen seconds is only true when all of that + # work is free. Slow the argv and liveness probes: after the engine is ended, + # status reaches the latter rather than the former. The old attempt-only loop + # would then need about thirty seconds; the clock budget reaches cleanup in + # five. + local slow_bin="$TEST_SKILL_DIR/slow-status-bin" + mkdir -p "$slow_bin" + printf '%s\n' '#!/usr/bin/env bash' \ + 'case " $* " in *" -o args= "*|*" -o stat= "*) sleep 0.1 ;; esac' \ + 'exec /bin/ps "$@"' > "$slow_bin/ps" + chmod +x "$slow_bin/ps" + + local lock="$TEST_SKILL_DIR/teams/testteam/.config.lock" + local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local starter engine i=0 j=0 released=0 err="$TEST_SKILL_DIR/bounded.err" + + # The clock budget is deliberately shorter than the 300-attempt ceiling can + # reach through the slow probe. The ceiling remains a finite fallback, so an + # implementation that drops the clock turns this into a bounded red test, + # not an unbounded CI hang. + env PATH="$slow_bin:$PATH" AGMSG_SYNC_READY_SECONDS=5 \ + AGMSG_SYNC_READY_TRIES=300 AGMSG_LOCK_SECONDS=2 \ + bash "$SCRIPTS/remote.sh" sync start testteam >"$err" 2>&1 & + starter=$! + + while [ ! -f "$pidfile" ] && [ "$i" -lt 100 ]; do i=$((i + 1)); sleep 0.05; done + [ -f "$pidfile" ] + engine="$(cat "$pidfile")" + + # Handshake 1: the initial lock is released while this caller is polling. + # A free lock after it returns would prove nothing, so require the starter to + # still be alive at the same moment. + while [ "$j" -lt 100 ]; do + if [ ! -d "$lock" ] && kill -0 "$starter" 2>/dev/null; then released=1; break; fi + j=$((j + 1)); sleep 0.05 + done + [ "$released" -eq 1 ] + + # Handshake 2: an external holder owns the lock before the engine ends. + # Handshake 3 follows immediately: the next branch this starter can take is + # the timeout cleanup, which must now fail to retake that holder's lock. + mkdir "$lock" + kill "$engine" 2>/dev/null || true + + # Assert the cleanup branch, not a fragile elapsed-time threshold. The + # helper's ten-second condition wait leaves headroom for a loaded runner; + # without the wall-clock bound the finite 300-attempt ceiling exceeds it. + if ! wait_for_file_contains "$err" 'could not retake the registry lock'; then + kill "$starter" 2>/dev/null || true + wait "$starter" 2>/dev/null || true + kill "$engine" 2>/dev/null || true + rmdir "$lock" 2>/dev/null || true + false + fi + + [ -f "$pidfile" ] + rmdir "$lock" 2>/dev/null || true + wait "$starter" 2>/dev/null || true +}