Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions bin/backends/herdr.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1442,21 +1442,51 @@ fm_backend_herdr_projection_order_best_effort() { # <session> <created-workspac
return 0
}

# Bash command substitutions may expose their result pipe on a descriptor above
# 2, so the server child closes every inherited non-stdio descriptor before exec.
fm_backend_herdr_close_inherited_fds() {
local fd
for fd in /dev/fd/*; do
fd=${fd##*/}
case "$fd" in
''|*[!0-9]*|0|1|2) continue ;;
esac
eval "exec ${fd}>&-"
done
}

# fm_backend_herdr_server_ensure: start the herdr server for <session>
# headless (no TUI client) if not already running, mirroring tmux's `tmux
# has-session || tmux new-session -d`. Verified: a bare socket CLI call does
# NOT auto-start the server, so this must run before any workspace/tab/pane
# call. Bounded poll for the server to report running.
# A readiness failure terminates and reaps only the direct start child; successful
# readiness leaves the server running without adding general liveness ownership.
fm_backend_herdr_server_ensure() { # <session>
local session=$1 running out i
local session=$1 running i start_pid
running=$(fm_backend_herdr_cli "$session" status --json 2>/dev/null | jq -r '.server.running // false' 2>/dev/null)
[ "$running" = "true" ] && return 0
( fm_backend_herdr_cli "$session" server >/dev/null 2>&1 & ) || return 1
(
fm_backend_herdr_close_inherited_fds
HERDR_SESSION="$session" exec herdr server --session "$session"
) </dev/null >/dev/null 2>&1 &
start_pid=$!
for i in $(seq 1 20); do
running=$(fm_backend_herdr_cli "$session" status --json 2>/dev/null | jq -r '.server.running // false' 2>/dev/null)
[ "$running" = "true" ] && return 0
sleep 0.5
done
if kill -0 "$start_pid" 2>/dev/null; then
kill -TERM "$start_pid" 2>/dev/null || true
for _ in $(seq 1 20); do
kill -0 "$start_pid" 2>/dev/null || break
sleep 0.1
done
if kill -0 "$start_pid" 2>/dev/null; then
kill -KILL "$start_pid" 2>/dev/null || true
fi
fi
wait "$start_pid" 2>/dev/null || true
echo "error: herdr server for session '$session' did not report running within 10s" >&2
return 1
}
Expand Down
3 changes: 3 additions & 0 deletions docs/herdr-backend.md
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ Workspace and tab ids support verification and cleanup but are not inferred from
## Current transport behavior

The adapter starts and polls a named server before workspace, tab, pane, or agent calls.
Starting a missing named server uses a background start child with stdin, stdout, and stderr redirected explicitly and with inherited descriptors above 2 closed before `exec`, so a caller can return after readiness without a Bash command-substitution pipe keeping the server attached.
The existing bounded 10-second readiness poll returns with the server running on success; on failure it sends `TERM`, escalates to `KILL` within a bounded window, and reaps only the direct start child.
That cleanup is limited to a failed start and does not introduce general Herdr liveness, identity, PID-ownership, or lifecycle tracking.
Every Herdr invocation goes through `fm_backend_herdr_cli`, which sets the environment and passes an explicit trailing `--session <name>`.
An environment variable alone is not reliable when another Herdr server is running.

Expand Down
2 changes: 2 additions & 0 deletions docs/remote-secondmates.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ bin/fm-on.sh <secondmate-id|ssh-alias> fm-remote-doctor.sh --fix

Over the plain SSH doctor bootstrap, it writes and reloads the Firstmate-owned `dev.firstmate.remote-job` and `dev.firstmate.herdr.fm-remote` launch agents on macOS, both scoped with `LimitLoadToSessionType=Aqua` and bootstrapped in `gui/<uid>`.
It starts the same workers directly on Linux, recreates the `~/.local/bin/fm-remote-entrypoint.sh` symlink when it is absent, and creates only Firstmate-owned required-tool wrappers that it can prove resolve to a version-manager target, stopping after one harness satisfies the at-least-one requirement.
On Linux, `--fix` starts the `fm-remote` Herdr server through the [bounded named-server path](herdr-backend.md#current-transport-behavior), so a ready server remains running after the doctor returns and a never-ready start is reported as failed.
It never installs packages or overwrites a non-Firstmate file at a reserved wrapper path.
The dedicated Herdr launch agent owns only the remote-secondmate `fm-remote` server and does not inspect, rewrite, start, stop, or require the user's interactive `default` session or its `dev.firstmate.herdr` launch agent.
It re-derives every check from the host afterwards, so what it prints is the state after the repair rather than the intent of one.
Expand Down Expand Up @@ -240,6 +241,7 @@ bin/fm-test-run.sh tests/fm-remote-secondmate-lifecycle-e2e.test.sh
bin/fm-test-run.sh tests/fm-remote-secondmate-trace-context.test.sh
```

The remote-doctor regression keeps its foreground-server cases host-gated to actual Linux: it checks both ready and never-ready process-group paths there while leaving Darwin on the existing fixture coverage.
The account-level checks the doctor performs - a real Aqua login session, a real `launchctl` domain, and a real herdr server - are only ever exercised against fixtures here, so the readiness gate's behavior on a genuine Mac remains an operator-run smoke test.

For a real-host smoke test, provision a disposable remote account and project, run the doctor and its repair against that account, launch the second mate, send one marked request, verify its correlated reply and structured fleet projection, simulate an unreachable host to confirm unknown-without-failover behavior, then retire only after the remote queue is empty.
Expand Down
195 changes: 194 additions & 1 deletion tests/fm-remote-doctor.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,66 @@ TMP_ROOT=$(cd "$TMP_ROOT" && pwd -P)
JOB_LABEL=dev.firstmate.remote-job
CASE_N=0
DOCTOR_WORKER_PID=
trap 'if [ -n "$DOCTOR_WORKER_PID" ]; then kill "$DOCTOR_WORKER_PID" 2>/dev/null || true; fi; fm_test_cleanup || true' EXIT
DOCTOR_GROUP_ID=
DOCTOR_GROUP_LEADER=

doctor_pid_alive() {
local pid=${1:-} stat=''
case "$pid" in
''|*[!0-9]*) return 1 ;;
esac
stat=$(ps -p "$pid" -o stat= 2>/dev/null | tr -d '[:space:]')
case "$stat" in
''|Z*) return 1 ;;
*) return 0 ;;
esac
}

doctor_group_alive() {
case "${1:-}" in
''|*[!0-9]*) return 1 ;;
esac
ps -eo pgid=,stat= 2>/dev/null |
awk -v group="$1" '$1 == group && $2 !~ /^Z/ { found = 1; exit } END { exit(found ? 0 : 1) }'
}

cleanup_owned_doctor_group() {
local group_id=${DOCTOR_GROUP_ID:-} leader=${DOCTOR_GROUP_LEADER:-}
local server_pid=''
case "$group_id:$leader" in
*[!0-9:]*) return 1 ;;
:|*:|:*) return 0 ;;
esac
[ "$group_id" = "$leader" ] || return 1
server_pid=$(cat "${CASE_HERDR_SERVER_PID:-}" 2>/dev/null || true)
case "$server_pid" in
''|*[!0-9]*) server_pid= ;;
esac

if doctor_group_alive "$group_id"; then
kill -TERM -- "-$group_id" 2>/dev/null || return 1
for _ in $(seq 1 20); do
doctor_group_alive "$group_id" || break
sleep 0.1
done
if doctor_group_alive "$group_id"; then
kill -KILL -- "-$group_id" 2>/dev/null || return 1
fi
fi
wait "$leader" 2>/dev/null || true
for _ in $(seq 1 50); do
if ! doctor_group_alive "$group_id" \
&& { [ -z "$server_pid" ] || ! doctor_pid_alive "$server_pid"; }; then
DOCTOR_GROUP_ID=
DOCTOR_GROUP_LEADER=
return 0
fi
sleep 0.1
done
return 1
}

trap 'if [ -n "$DOCTOR_WORKER_PID" ]; then kill "$DOCTOR_WORKER_PID" 2>/dev/null || true; fi; cleanup_owned_doctor_group >/dev/null 2>&1 || true; fm_test_cleanup || true' EXIT

# A fixture must be able to present a host with NO herdr, so the doctor never
# sees the runner's own PATH. Only the two required tools are re-exposed, by
Expand Down Expand Up @@ -48,11 +107,15 @@ new_case() {
CASE_LAUNCHCTL_LOG="$CASE_STATE/launchctl.log"
CASE_FORBIDDEN_LOG="$CASE_STATE/forbidden.log"
CASE_HERDR_RUNNING="$CASE_STATE/herdr.running"
CASE_HERDR_SERVER_PID="$CASE_STATE/herdr.server.pid"
CASE_PLIST="$CASE_HOME/Library/LaunchAgents/$LABEL.plist"
CASE_INTERACTIVE_PLIST="$CASE_HOME/Library/LaunchAgents/$INTERACTIVE_LABEL.plist"
CASE_JOB_PLIST="$CASE_HOME/Library/LaunchAgents/$JOB_LABEL.plist"
mkdir -p "$CASE_BIN" "$CASE_HOME" "$CASE_PROJECT_HOME" "$CASE_STATE"
printf 'false\n' > "$CASE_HERDR_RUNNING"
: > "$CASE_HERDR_SERVER_PID"
CASE_HERDR_FOREGROUND=0
CASE_HERDR_NEVER_READY=0
: > "$CASE_LAUNCHCTL_LOG"
: > "$CASE_FORBIDDEN_LOG"
[ "$want_gui" != gui ] || touch "$CASE_STATE/gui-session"
Expand Down Expand Up @@ -181,6 +244,13 @@ case "${1:-} ${2:-}" in
printf '{"client":{"version":"0.7.5","protocol":16},"server":{"running":%s}}\n' "$running"
;;
"server "*|"server ")
if [ "${FM_FAKE_HERDR_FOREGROUND:-0}" = 1 ]; then
printf '%s\n' "$$" > "$FM_FAKE_HERDR_SERVER_PID"
[ "${FM_FAKE_HERDR_NEVER_READY:-0}" = 1 ] \
|| printf 'true\n' > "$FM_FAKE_HERDR_RUNNING"
trap '' TERM
exec tail -f /dev/null
fi
printf 'true\n' > "$FM_FAKE_HERDR_RUNNING"
;;
esac
Expand Down Expand Up @@ -224,6 +294,9 @@ doctor() {
FM_FAKE_LAUNCHCTL_LOG="$CASE_LAUNCHCTL_LOG" \
FM_FAKE_FORBIDDEN_LOG="$CASE_FORBIDDEN_LOG" \
FM_FAKE_HERDR_RUNNING="$CASE_HERDR_RUNNING" \
FM_FAKE_HERDR_FOREGROUND="${CASE_HERDR_FOREGROUND:-0}" \
FM_FAKE_HERDR_NEVER_READY="${CASE_HERDR_NEVER_READY:-0}" \
FM_FAKE_HERDR_SERVER_PID="$CASE_HERDR_SERVER_PID" \
FM_FAKE_HERDR_BIN="$CASE_BIN/herdr" \
FM_FAKE_PLIST="$CASE_PLIST" \
FM_FAKE_JOB_PLIST="$CASE_JOB_PLIST" \
Expand Down Expand Up @@ -516,6 +589,126 @@ assert_contains "$DOCTOR_OUT" 'check herdr-server=ok:' "the started server was n
[ ! -s "$CASE_LAUNCHCTL_LOG" ] || fail "the linux path invoked launchctl"
pass "a non-darwin host skips launch agents and starts its herdr server directly"

# --- Linux doctor must detach or reap its foreground Herdr child -------------

run_doctor_with_watchdog() {
local output_file="$CASE_STATE/bounded-doctor.out"
local actual_group='' doctor_pid
DOCTOR_TIMED_OUT=1
DOCTOR_RC=124
DOCTOR_OUT=
rm -f "$output_file"
# The single-quoted script is expanded by the child bash, not this test shell.
# shellcheck disable=SC2016
HOME="$CASE_HOME" \
FM_HOME="$CASE_PROJECT_HOME" \
PATH="$CASE_HOME/.local/bin:$CASE_BIN:$BASE_PATH" \
FM_FAKE_STATE="$CASE_STATE" \
FM_FAKE_LAUNCHCTL_LOG="$CASE_LAUNCHCTL_LOG" \
FM_FAKE_FORBIDDEN_LOG="$CASE_FORBIDDEN_LOG" \
FM_FAKE_HERDR_RUNNING="$CASE_HERDR_RUNNING" \
FM_FAKE_HERDR_FOREGROUND="${CASE_HERDR_FOREGROUND:-0}" \
FM_FAKE_HERDR_NEVER_READY="${CASE_HERDR_NEVER_READY:-0}" \
FM_FAKE_HERDR_SERVER_PID="$CASE_HERDR_SERVER_PID" \
FM_FAKE_HERDR_BIN="$CASE_BIN/herdr" \
FM_FAKE_PLIST="$CASE_PLIST" \
FM_FAKE_JOB_PLIST="$CASE_JOB_PLIST" \
FM_FAKE_JOB_WORKER="$ROOT/bin/fm-remote-job-worker.sh" \
FM_FAKE_LAUNCH_AGENT_LOG="$CASE_HOME/Library/Logs/$LABEL.log" \
FM_REMOTE_JOB_PLATFORM_OVERRIDE="${CASE_PLATFORM_OVERRIDE-}" \
FM_REMOTE_JOB_ACTIVE="${CASE_REMOTE_JOB_ACTIVE-1}" \
setsid bash -c '
set +e
output=$("$1" --fix 2>&1)
rc=$?
printf "%s" "$output"
exit "$rc"
' _ "$ROOT/bin/fm-remote-doctor.sh" > "$output_file" 2>&1 &
doctor_pid=$!
DOCTOR_GROUP_LEADER=$doctor_pid
# `$!` is visible before the child has necessarily completed exec into
# setsid. Wait for the kernel PGID transition instead of racing one snapshot.
for _ in $(seq 1 100); do
actual_group=$(ps -p "$DOCTOR_GROUP_LEADER" -o pgid= 2>/dev/null | tr -d '[:space:]')
[ "$actual_group" = "$DOCTOR_GROUP_LEADER" ] && break
doctor_pid_alive "$DOCTOR_GROUP_LEADER" || break
sleep 0.01
done
[ "$actual_group" = "$DOCTOR_GROUP_LEADER" ] \
|| fail "the Linux doctor watchdog did not own a dedicated process group"
DOCTOR_GROUP_ID=$actual_group

for _ in $(seq 1 200); do
doctor_pid_alive "$DOCTOR_GROUP_LEADER" || { DOCTOR_TIMED_OUT=0; break; }
sleep 0.1
done
if [ "$DOCTOR_TIMED_OUT" -eq 1 ]; then
cleanup_owned_doctor_group \
|| fail "the Linux doctor watchdog could not reap its owned process group after timeout"
fi
set +e
wait "$doctor_pid" 2>/dev/null
DOCTOR_RC=$?
set -e
[ "$DOCTOR_TIMED_OUT" -eq 0 ] || DOCTOR_RC=124
DOCTOR_OUT=$(cat "$output_file")
}

if [ "$(uname -s)" = Linux ]; then
command -v setsid >/dev/null 2>&1 \
|| fail "the Linux doctor regression requires setsid"

new_case Linux with-herdr no-gui
CASE_HERDR_FOREGROUND=1
rm -f "$CASE_BIN/sleep"
run_doctor_with_watchdog
expect_code 0 "$DOCTOR_TIMED_OUT" \
"fm-remote-doctor --fix exceeded its watchdog with a ready foreground Linux Herdr server"
expect_code 0 "$DOCTOR_RC" "the bounded Linux doctor run did not complete successfully"
assert_contains "$DOCTOR_OUT" 'fix herdr-server=applied:' \
"the bounded Linux doctor run did not report starting Herdr"
assert_contains "$DOCTOR_OUT" 'check herdr-server=ok:' \
"the bounded Linux doctor run did not confirm Herdr readiness"
server_pid=$(cat "$CASE_HERDR_SERVER_PID")
case "$server_pid" in
''|*[!0-9]*) fail "the bounded Linux doctor run did not leave an owned Herdr server pid" ;;
esac
doctor_pid_alive "$server_pid" \
|| fail "the bounded Linux doctor run reported readiness after its Herdr server exited"
[ "$(ps -p "$server_pid" -o pgid= 2>/dev/null | tr -d '[:space:]')" = "$DOCTOR_GROUP_ID" ] \
|| fail "the ready fake Herdr server escaped the watchdog-owned process group"
cleanup_owned_doctor_group \
|| fail "the ready fake Herdr server or its process group survived cleanup"
pass "Linux doctor returns while its foreground Herdr server stays ready"

new_case Linux with-herdr no-gui
CASE_HERDR_FOREGROUND=1
CASE_HERDR_NEVER_READY=1
rm -f "$CASE_BIN/sleep"
run_doctor_with_watchdog
expect_code 0 "$DOCTOR_TIMED_OUT" \
"fm-remote-doctor --fix exceeded its watchdog when Herdr never became ready"
expect_code 1 "$DOCTOR_RC" "a never-ready Linux Herdr server did not fail the doctor"
assert_contains "$DOCTOR_OUT" 'fix herdr-server=failed:' \
"the never-ready Linux doctor run did not report the failed start"
assert_contains "$DOCTOR_OUT" 'check herdr-server=fixable:' \
"the never-ready Linux doctor run did not preserve the readiness gap"
server_pid=$(cat "$CASE_HERDR_SERVER_PID")
case "$server_pid" in
''|*[!0-9]*) fail "the never-ready Linux doctor run did not record its owned Herdr server pid" ;;
esac
if doctor_pid_alive "$server_pid" || doctor_group_alive "$DOCTOR_GROUP_ID"; then
cleanup_owned_doctor_group \
|| fail "the never-ready fake Herdr server survived and fallback cleanup failed"
fail "the never-ready fake Herdr server survived the doctor's bounded cleanup"
fi
cleanup_owned_doctor_group \
|| fail "the never-ready fake Herdr server or its process group survived cleanup"
pass "Linux doctor reaps a foreground Herdr server that never becomes ready"
else
pass "Linux foreground Herdr regression is covered on Linux only"
fi

# --- --fix may add only owned wrappers for version-manager tools -------------

new_case Linux with-herdr no-gui
Expand Down
Loading