fleetd #555: lift 8 main-flow decisions into tested predicate/dispatch functions
CI / shell-tests (pull_request) Successful in 11s
CI / contract (pull_request) Successful in 1m31s
CI / build (pull_request) Successful in 1m46s

redeploy-fleetd.sh's main flow had 8 bare if/case decisions (CHECK_ONLY
short-circuit, drain-gate entry+confirm, supervisor report/stop/start
dispatch, health-poll decision, HAD_OLD_PID computation) that lived outside
any function, so the 67-test suite could not reach them and any one could be
silently inverted with the whole suite green.

Follows the existing swap_if_built/refuse_drain_gate pattern: each bare
guard becomes a small predicate or dispatch function (should_stop_for_check,
drain_gate_required/drain_confirmed/run_drain_gate, report_supervisor_state,
dispatch_stop, dispatch_start, health_is_up/report_health,
compute_had_old_pid), called unconditionally by the main flow so the
decision itself is unit-testable in isolation.

Adds a structural guard, test_no_untested_main_flow_conditionals, that scans
the main flow (everything after the SOURCED guard) for bare if/elif/case
lines outside any function body, tracking function boundaries via this
file's one consistent name() { / } convention. It fails on any new bare
conditional not covered by MAIN_FLOW_ALLOWED_CONDITIONALS, an explicit
exact-text allowlist of the report-only/display conditionals and the two
#504-family supervisor elif branches that stay out of scope for this
ticket. This is the "shape, not the eight sites" guard the ticket asked
for: a ninth bare decision fails immediately, naming its line.

Out of scope, not touched: #504 items 2/3/4 and #528 item 2 (same
untested-main-flow family) — the seam here generalizes to make them
testable too, but lifting them was left for their own tickets.
This commit is contained in:
Dai Ha
2026-09-12 16:46:53 +07:00
parent 4f9aba40e7
commit 8d79d229ff
2 changed files with 755 additions and 145 deletions
+253 -138
View File
@@ -806,6 +806,229 @@ refuse_drain_gate() {
die "$(drain_gate_refusal "$do_build" "$staged_path")"
}
# fleetd #555 — the main flow itself was almost entirely untestable: sourcing this file (the SOURCED
# guard below) stops before a single line of the report/build/drain/stop/swap/start/verify sequence
# ever runs, and every decision in that sequence used to live as a bare `if`/`case` written directly
# into the main flow rather than inside a function. Three tests reached that flow at all
# (test_swap_ordered_after_wait_and_before_start, test_refuse_drain_gate_call_site_present,
# test_report_shutdown_drain_call_site_present), and all three do it by grepping this file's own
# source for a call site — which proves the call site exists, never that the guard around it still
# reaches it. Every function below follows the swap_if_built/refuse_drain_gate shape those two
# tickets (#521/#528) already established: the decision (a small, separately-tested predicate) and
# the action it gates live together in ONE function, and the main flow calls that function
# unconditionally — so there is no bare guard left in the main flow for a future edit to invert
# silently. See test_no_untested_main_flow_conditionals in test-redeploy-fleetd.sh for the structural
# guard that keeps a ninth bare conditional from arriving the same way these eight did.
# Ticket item 1 — `--check` must stay genuinely read-only. Inverting the guard used to mean --check
# performs a real redeploy; now the guard is should_stop_for_check, and stop_if_check_only is the
# only thing the main flow calls.
should_stop_for_check() {
local check_only="$1"
[ "$check_only" = 1 ]
}
stop_if_check_only() {
local check_only="$1"
should_stop_for_check "$check_only" || return 0
say "--check: nothing changed"
exit 0
}
# Ticket items 2 and 3 — the drain-gate entry (`-n "$OLD_PID" && "$ASSUME_YES" = 0`) and the reply
# comparison (`"$reply" != "yes"`). drain_gate_required and drain_confirmed are the two predicates;
# run_drain_gate is the only thing the main flow calls, and it is the one place that reads $reply at
# all, so a test can drive it end-to-end over stdin.
drain_gate_required() {
local old_pid="$1" assume_yes="$2"
[ -n "$old_pid" ] && [ "$assume_yes" = 0 ]
}
drain_confirmed() {
[ "$1" = "yes" ]
}
run_drain_gate() {
local old_pid="$1" assume_yes="$2" do_build="$3" staged_path="$4" reply
drain_gate_required "$old_pid" "$assume_yes" || return 0
say "drain check"
echo " A restart drops every in-flight ticket and rendezvous. A member's report"
echo " is NOT recoverable once its ticket is gone."
echo
echo " Confirm with fleet_list that no members are live, and fleet_poll anything"
echo " you still want, BEFORE continuing."
echo
read -r -p " Fleet drained? type yes to restart: " reply
drain_confirmed "$reply" || refuse_drain_gate "$do_build" "$staged_path"
}
# Ticket item 4 — the report-state dispatch on $SUPERVISOR_KIND. Inverting this used to report the
# wrong supervisor and, for the launchd arm specifically, skip check_log_path_matches_plist — whose
# own comment already says every check after it is worthless if it never runs.
report_supervisor_state() {
local kind="$1"
SUPERVISED=0
case "$kind" in
launchd)
SUPERVISED=1
ok "launchd agent loaded ($LAUNCHD_LABEL) — launchd supervises this daemon"
check_log_path_matches_plist "$OUT" "$LAUNCHD_PLIST"
;;
systemd)
SUPERVISED=1
ok "systemd --user unit active ($SYSTEMD_UNIT) — systemd supervises this daemon"
;;
none)
warn "no supervisor loaded — this script is the only thing that will restart the daemon."
;;
*)
warn "unrecognised supervisor kind: '$kind' — detect_supervisor returned a value this block does not know; continuing to report the rest of the state."
;;
esac
}
# Ticket item 5 — the stop dispatch on $SUPERVISOR_KIND. Inverting this kills a supervised daemon
# with a raw `kill` instead of `launchctl unload`/`systemctl --user stop`, so the supervisor revives
# the OLD jar — the exact CB-594/#492 failure both branches exist to prevent. Each mechanism is its
# own thin, overridable function (same seam launchd_installed/launchd_loaded already use) so a test
# can prove dispatch_stop picks the right one without ever calling real launchctl/systemctl/kill.
stop_via_launchd() {
echo " supervision is ON (launchd): using 'launchctl unload' (not kill) so launchd's own"
echo " KeepAlive cannot restart the OLD jar out from under this script — see the CB-594"
echo " comment above."
launchctl unload -w "$LAUNCHD_PLIST" \
|| die "launchctl unload failed — the daemon may still be under supervision; investigate before retrying"
}
stop_via_systemd() {
echo " supervision is ON (systemd --user): using 'systemctl --user stop' (not kill) so"
echo " systemd's own Restart=on-failure cannot restart the OLD jar out from under this"
echo " script — see the fleetd #492 comment above."
systemctl --user stop "$SYSTEMD_UNIT" \
|| die "'systemctl --user stop $SYSTEMD_UNIT' failed — the daemon may still be under supervision; investigate before retrying"
}
stop_via_kill() {
local pid="$1"
kill "$pid"
}
dispatch_stop() {
local kind="$1" pid="$2"
case "$kind" in
launchd) stop_via_launchd ;;
systemd) stop_via_systemd ;;
none) stop_via_kill "$pid" ;;
*)
die "detect_supervisor returned an unrecognized value '$kind' at the stop step —
refusing to guess how to stop a daemon under an unknown supervisor. The daemon was NOT
stopped." ;;
esac
}
# Ticket item 6 — the start dispatch on $SUPERVISOR_KIND. Inverting this starts the daemon by the
# wrong mechanism; for launchd the branch also carries the retry that keeps a failed load from
# leaving the agent stopped-and-disabled. Same shape as the stop dispatch above.
start_via_launchd() {
echo " supervision is ON (launchd): using 'launchctl load' so launchd starts and keeps"
echo " supervising this process, instead of a manual nohup that launchd would know nothing"
echo " about."
# CB-600: 'launchctl unload -w' above already persisted Disabled=true for this label. A load -w
# that succeeds clears it; a load -w that FAILS leaves the agent both stopped and disabled — worse
# than before this script ran, because a later reboot or login will not bring it back either. One
# retry covers a transient race (e.g. launchd not yet fully done deregistering); if it still fails,
# die with the exact recovery command rather than a bare "failed".
if ! launchctl load -w "$LAUNCHD_PLIST" 2>/dev/null; then
warn "launchctl load failed on the first attempt — retrying once after a short pause"
sleep 2
launchctl load -w "$LAUNCHD_PLIST" || die "launchctl load failed twice.
The agent is now STOPPED and DISABLED — it will NOT come back on its own, not even after a
reboot or login, because 'launchctl unload -w' above persisted Disabled=true and load -w
never got the chance to clear it. Recover with:
launchctl load -w \"$LAUNCHD_PLIST\"
If that still fails, check 'launchctl list $LAUNCHD_LABEL', validate the plist with
'plutil -lint \"$LAUNCHD_PLIST\"', and check $OUT before assuming a retry will succeed."
fi
}
start_via_systemd() {
echo " supervision is ON (systemd --user): using 'systemctl --user start' so systemd starts"
echo " and keeps supervising this process, instead of a manual nohup it would know nothing"
echo " about."
systemctl --user start "$SYSTEMD_UNIT" || die "'systemctl --user start $SYSTEMD_UNIT' failed.
Check 'systemctl --user status $SYSTEMD_UNIT' and $OUT before assuming a retry will succeed."
}
start_via_none() {
# Absolute jar path so `ps` names which checkout is running.
( cd "$MODULE" && zsh -lc "nohup java -jar '$JAR' >> fleetd.out 2>&1 &" )
}
dispatch_start() {
local kind="$1"
case "$kind" in
launchd) start_via_launchd ;;
systemd) start_via_systemd ;;
none) start_via_none ;;
*)
die "detect_supervisor returned an unrecognized value '$kind' at the start step —
refusing to guess how to start a daemon under an unknown supervisor. The daemon was NOT
started." ;;
esac
}
# Ticket item 7 — the HEALTH_BODY poll loop and the `[ -z "$HEALTH_BODY" ]` branch. Inverting the
# latter (health_is_up) makes a dead daemon report /healthz 200 or a live one report failure.
# poll_health_body is pulled out too so the loop's own `if ...; then break; fi` is inside a function
# rather than sitting bare in the main flow.
poll_health_body() {
local url="$1" wait_s="$2" body="" _i
for _i in $(seq "$wait_s"); do
if body="$(curl -fsS --max-time 2 "$url" 2>/dev/null)"; then
printf '%s' "$body"
return 0
fi
sleep 1
done
return 1
}
health_is_up() {
[ -n "$1" ]
}
report_health() {
local body="$1" code="$2" out_file="$3" wait_s="$4"
if health_is_up "$body"; then
ok "/healthz 200 — $body"
warn "healthz green only proves herdr ANSWERS. If its protocol number changed, spawns can still"
warn "fail — prove a real spawn before trusting the fleet."
return 0
fi
# 503 still means the daemon is up — it means herdr is unreachable. Say which.
if [ "$code" = "503" ]; then
warn "/healthz answers 503 degraded — the daemon is up but herdr is unreachable."
warn "Spawns will fail. Check herdr before delegating anything."
curl -s --max-time 2 "$HEALTH" 2>/dev/null | head -3 || true
else
die "/healthz never answered within ${wait_s}s (last code: $code). Last lines of $out_file:
$(tail -30 "$out_file" 2>/dev/null)"
fi
}
# Ticket item 8 — `HAD_OLD_PID=0; [ -n "$OLD_PID" ] && HAD_OLD_PID=1`. Measured safe under `set -e`
# at both bash 3.2.57 and 5.x (see the header comment trap 9 discussion in the ticket) — not a `set
# -e` hazard, but still an untested computation feeding report_shutdown_drain's own four-way
# decision. compute_had_old_pid makes the mapping itself directly testable.
compute_had_old_pid() {
local old_pid="$1"
if [ -n "$old_pid" ]; then
echo 1
else
echo 0
fi
}
# CB-600: sourceable for testing. When this file is SOURCED (not executed) it stops here — nothing
# below runs — so a test harness can `source` it to call check_log_path_matches_plist (or the
# other pure helpers above) against a throwaway plist fixture without ever reaching the mutating
@@ -854,30 +1077,11 @@ SUPERVISOR_KIND="${SUPERVISOR_RAW%%"$SUPERVISOR_DETAIL_SEP"*}"
SUPERVISOR_UNCLEAR_DETAIL="${SUPERVISOR_RAW#*"$SUPERVISOR_DETAIL_SEP"}"
require_drivable_supervisor "$SUPERVISOR_KIND"
ok "supervisor detected: $SUPERVISOR_KIND"
SUPERVISED=0
case "$SUPERVISOR_KIND" in
launchd)
SUPERVISED=1
ok "launchd agent loaded ($LAUNCHD_LABEL) — launchd supervises this daemon"
# CB-600: fail loudly here, before ANY other check runs, if this script and the loaded plist
# would read different log files — every check after this point is worthless otherwise.
check_log_path_matches_plist "$OUT" "$LAUNCHD_PLIST"
;;
systemd)
SUPERVISED=1
ok "systemd --user unit active ($SYSTEMD_UNIT) — systemd supervises this daemon"
;;
none)
warn "no supervisor loaded — this script is the only thing that will restart the daemon."
;;
*)
# fleetd #492 follow-up: this block only DISPLAYS state, it changes nothing yet — so a value
# it doesn't recognise gets reported, not an abort that goes silent on exactly the state most
# worth seeing. (Unreachable today: require_drivable_supervisor above already died on
# "ambiguous"/"unclear" before this case runs. Guards the value nobody has invented yet.)
warn "unrecognised supervisor kind: '$SUPERVISOR_KIND' — detect_supervisor returned a value this block does not know; continuing to report the rest of the state."
;;
esac
# fleetd #555: the dispatch below used to be a bare `case` written directly here — see
# report_supervisor_state above for why it is now the only thing this line calls. (Unreachable
# today for the `*)` arm: require_drivable_supervisor above already died on "ambiguous"/"unclear"
# before this call runs. Guards the value nobody has invented yet.)
report_supervisor_state "$SUPERVISOR_KIND"
# The trap with no log line. Checked in a LOGIN shell, because that is how the daemon is started
# below. Never prints the value — only whether it resolved.
@@ -917,10 +1121,9 @@ else
warn "Replies stop surviving a restart — a held report is lost, not delayed."
fi
if [ "$CHECK_ONLY" = 1 ]; then
say "--check: nothing changed"
exit 0
fi
# fleetd #555: should_stop_for_check/stop_if_check_only above — --check must stay genuinely
# read-only, and this is the only place that decides whether to stop here.
stop_if_check_only "$CHECK_ONLY"
# ---------------------------------------------------------------------- build
# Deliberately before the stop: a failed build must never leave the fleet down.
@@ -954,23 +1157,11 @@ fi
# ----------------------------------------------------------------- drain gate
if [ -n "$OLD_PID" ] && [ "$ASSUME_YES" = 0 ]; then
say "drain check"
echo " A restart drops every in-flight ticket and rendezvous. A member's report"
echo " is NOT recoverable once its ticket is gone."
echo
echo " Confirm with fleet_list that no members are live, and fleet_poll anything"
echo " you still want, BEFORE continuing."
echo
read -r -p " Fleet drained? type yes to restart: " reply
if [ "$reply" != "yes" ]; then
# fleetd #493 / #517 / #528: "nothing changed" would be a lie once a build has run and staged a
# jar — see drain_gate_refusal above for the full decision and why each of its four cases reads
# the way it does. refuse_drain_gate composes that message AND calls die itself, so this guard
# has nothing left of its own to get wrong beyond whether it calls refuse_drain_gate at all.
refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"
fi
fi
# fleetd #555: run_drain_gate above is drain_gate_required + the prompt + drain_confirmed, called
# unconditionally — it returns immediately when the gate is not required, and composes/dies through
# refuse_drain_gate itself when the reply does not confirm. See #493/#517/#528 for why "nothing
# changed" would be a lie once a build has staged a jar.
run_drain_gate "$OLD_PID" "$ASSUME_YES" "$DO_BUILD" "$JAR_STAGED"
# ------------------------------------------------------------------ stop
#
@@ -988,33 +1179,9 @@ fi
if [ -n "$OLD_PID" ]; then
say "stop"
RESTART_MARK="$(wc -l < "$OUT" 2>/dev/null || echo 0)" # verify a FRESH line appears later
case "$SUPERVISOR_KIND" in
launchd)
echo " supervision is ON (launchd): using 'launchctl unload' (not kill) so launchd's own"
echo " KeepAlive cannot restart the OLD jar out from under this script — see the CB-594"
echo " comment above."
launchctl unload -w "$LAUNCHD_PLIST" \
|| die "launchctl unload failed — the daemon may still be under supervision; investigate before retrying"
;;
systemd)
echo " supervision is ON (systemd --user): using 'systemctl --user stop' (not kill) so"
echo " systemd's own Restart=on-failure cannot restart the OLD jar out from under this"
echo " script — see the fleetd #492 comment above."
systemctl --user stop "$SYSTEMD_UNIT" \
|| die "'systemctl --user stop $SYSTEMD_UNIT' failed — the daemon may still be under supervision; investigate before retrying"
;;
none)
kill "$OLD_PID"
;;
*)
# fleetd #492 follow-up: this block ACTS (stops the daemon one specific way per kind) — an
# unrecognised value must never fall through to a default action, silently picking the wrong
# one (or none at all) while reporting success. (Unreachable today: require_drivable_
# supervisor already died before this runs. Guards the value nobody has invented yet.)
die "detect_supervisor returned an unrecognized value '$SUPERVISOR_KIND' at the stop step —
refusing to guess how to stop a daemon under an unknown supervisor. The daemon was NOT
stopped." ;;
esac
# fleetd #555: dispatch_stop above — the launchd/systemd/none/* case lives there now, so this
# line is the only thing left in the main flow to get wrong.
dispatch_stop "$SUPERVISOR_KIND" "$OLD_PID"
if ! wait_for_daemon_exit "$STOP_WAIT"; then
die "pid $OLD_PID still alive after ${STOP_WAIT}s. Not escalating to kill -9 automatically:
the shutdown hook releases sessions and worktrees in order, and killing it hard can
@@ -1063,48 +1230,9 @@ swap_if_built "$DO_BUILD"
# above) and WorkingDirectory is already pinned to fleetd/.
say "start"
case "$SUPERVISOR_KIND" in
launchd)
echo " supervision is ON (launchd): using 'launchctl load' so launchd starts and keeps"
echo " supervising this process, instead of a manual nohup that launchd would know nothing"
echo " about."
# CB-600: 'launchctl unload -w' above already persisted Disabled=true for this label. A load -w
# that succeeds clears it; a load -w that FAILS leaves the agent both stopped and disabled — worse
# than before this script ran, because a later reboot or login will not bring it back either. One
# retry covers a transient race (e.g. launchd not yet fully done deregistering); if it still fails,
# die with the exact recovery command rather than a bare "failed".
if ! launchctl load -w "$LAUNCHD_PLIST" 2>/dev/null; then
warn "launchctl load failed on the first attempt — retrying once after a short pause"
sleep 2
launchctl load -w "$LAUNCHD_PLIST" || die "launchctl load failed twice.
The agent is now STOPPED and DISABLED — it will NOT come back on its own, not even after a
reboot or login, because 'launchctl unload -w' above persisted Disabled=true and load -w
never got the chance to clear it. Recover with:
launchctl load -w \"$LAUNCHD_PLIST\"
If that still fails, check 'launchctl list $LAUNCHD_LABEL', validate the plist with
'plutil -lint \"$LAUNCHD_PLIST\"', and check $OUT before assuming a retry will succeed."
fi
;;
systemd)
echo " supervision is ON (systemd --user): using 'systemctl --user start' so systemd starts"
echo " and keeps supervising this process, instead of a manual nohup it would know nothing"
echo " about."
systemctl --user start "$SYSTEMD_UNIT" || die "'systemctl --user start $SYSTEMD_UNIT' failed.
Check 'systemctl --user status $SYSTEMD_UNIT' and $OUT before assuming a retry will succeed."
;;
none)
# Absolute jar path so `ps` names which checkout is running.
( cd "$MODULE" && zsh -lc "nohup java -jar '$JAR' >> fleetd.out 2>&1 &" )
;;
*)
# fleetd #492 follow-up: this block ACTS (starts the daemon one specific way per kind) — an
# unrecognised value must never fall through to a default action, silently picking the wrong
# one (or none at all) while reporting success. (Unreachable today: require_drivable_
# supervisor already died before this runs. Guards the value nobody has invented yet.)
die "detect_supervisor returned an unrecognized value '$SUPERVISOR_KIND' at the start step —
refusing to guess how to start a daemon under an unknown supervisor. The daemon was NOT
started." ;;
esac
# fleetd #555: dispatch_start above — the launchd/systemd/none/* case (and the launchd retry) live
# there now, so this line is the only thing left in the main flow to get wrong.
dispatch_start "$SUPERVISOR_KIND"
for _ in $(seq 10); do
NEW_PID="$(running_pid)"
@@ -1120,29 +1248,14 @@ ok "started, pid $NEW_PID"
say "verify"
HEALTH_BODY=""
for _ in $(seq "$HEALTH_WAIT"); do
if HEALTH_BODY="$(curl -fsS --max-time 2 "$HEALTH" 2>/dev/null)"; then break; fi
HEALTH_BODY=""
sleep 1
done
if [ -z "$HEALTH_BODY" ]; then
# 503 still means the daemon is up — it means herdr is unreachable. Say which.
CODE="$(curl -s -o /dev/null -w '%{http_code}' --max-time 2 "$HEALTH" 2>/dev/null || echo 000)"
if [ "$CODE" = "503" ]; then
warn "/healthz answers 503 degraded — the daemon is up but herdr is unreachable."
warn "Spawns will fail. Check herdr before delegating anything."
curl -s --max-time 2 "$HEALTH" 2>/dev/null | head -3 || true
else
die "/healthz never answered within ${HEALTH_WAIT}s (last code: $CODE). Last lines of $OUT:
$(tail -30 "$OUT" 2>/dev/null)"
fi
else
ok "/healthz 200 — $HEALTH_BODY"
warn "healthz green only proves herdr ANSWERS. If its protocol number changed, spawns can still"
warn "fail — prove a real spawn before trusting the fleet."
fi
# fleetd #555: poll_health_body/health_is_up/report_health above. HEALTH_CODE is only ever
# consulted by report_health when the body came back empty; `|| true` on both assignments is the
# same "an absent/failing command substitution must not kill the script under set -e" idiom the
# swap/drain helpers already rely on (see poll_health_body's own comment).
HEALTH_BODY="$(poll_health_body "$HEALTH" "$HEALTH_WAIT")" || true
HEALTH_CODE="000"
[ -n "$HEALTH_BODY" ] || HEALTH_CODE="$(curl -s -o /dev/null -w '%{http_code}' --max-time 2 "$HEALTH" 2>/dev/null || echo 000)"
report_health "$HEALTH_BODY" "$HEALTH_CODE" "$OUT" "$HEALTH_WAIT"
# A fresh listening line, strictly after the restart mark. An old daemon that never died would
# otherwise let an old line pass for a new one.
@@ -1185,7 +1298,9 @@ classify_amqp_connection_errors "$FRESH_LOG"
# "cannot tell"). HAD_OLD_PID crosses in whether a previous daemon was actually stopped this run;
# see the function's own comment for why that matters.
say "previous daemon's shutdown drain"
HAD_OLD_PID=0; [ -n "$OLD_PID" ] && HAD_OLD_PID=1
# fleetd #555: compute_had_old_pid above — the mapping itself is now directly testable rather than
# an inline `&&` idiom sitting bare in the main flow.
HAD_OLD_PID="$(compute_had_old_pid "$OLD_PID")"
report_shutdown_drain "$FRESH_LOG" "$HAD_OLD_PID"
# fleetd #492: checked here, after healthz and the fresh-log check have both had time to run, so a