From e4eb3dbed45ed01acd628a128253698f9a7abedb Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 13:42:27 +0700 Subject: [PATCH] fleetd #504 item 1: stop swallowing real launchctl/systemctl failures on the 'loaded but not running' path The two 'loaded but not currently running' branches in the stop step (launchd/systemd, reached when $OLD_PID is empty) ran 'launchctl unload'/'systemctl --user stop' with '2>/dev/null || true' and printed 'ok' unconditionally. That swallowed a real supervisor failure (e.g. launchd or the systemd user bus unreachable) exactly like a harmless already-stopped answer, and let the script proceed to start a new daemon believing nothing was loaded -- the two-daemons failure fleetd #492 exists to prevent. Adds unload_launchd_if_loaded/stop_systemd_if_loaded, applying systemd_loaded's own pattern (capture stderr separately; a non-zero exit WITH stderr is a real failure, a non-zero exit with empty stderr is a clean already-stopped answer) to the write side. The two call sites now use these functions instead of the bare '|| true'. Adds 5 tests: dies-on-real-failure and tolerates-clean-negative for each function, plus a source-text check that the main flow calls the new functions instead of the original bare '2>/dev/null || true'. All 5 verified by mutation (reintroducing the swallow, and separately over-correcting to die unconditionally) -- each goes red with its own message, restores byte-identical (full sha256), and passes a green control. --- scripts/redeploy-fleetd.sh | 51 ++++++++++++++- scripts/test-redeploy-fleetd.sh | 109 ++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh index 76c55d9..d862931 100755 --- a/scripts/redeploy-fleetd.sh +++ b/scripts/redeploy-fleetd.sh @@ -287,6 +287,49 @@ systemd_loaded() { return "$rc" } +# fleetd #504: the "loaded but not currently running" branches in the main stop step (case +# launchd/systemd, reached when $OLD_PID is empty) used to run `launchctl unload`/`systemctl --user +# stop` with `2>/dev/null || true` and then print `ok` unconditionally — the exact conflation +# systemd_loaded/systemd_installed above already fixed on the READ side (fleetd #492 follow-up): a +# genuine "already stopped" answer (nonzero exit, nothing on stderr) is harmless, but a real tool +# failure (nonzero exit WITH a stderr message — e.g. launchd or the systemd user bus is +# unreachable) is not, and reporting `ok` on THAT means the start step below can register a fresh +# load on top of a supervisor that never actually let go: the exact two-daemons failure fleetd #492 +# exists to prevent, reached from the one state (already odd) where a false `ok` is least +# affordable. These two functions apply the same "capture stderr separately, flag only a nonzero +# exit WITH stderr as a real failure" pattern to the WRITE side. No ${VAR:-default} anywhere here — +# see the #492 follow-up constraints comment above detect_supervisor for why a default would hide a +# lost value instead of surfacing it (fleetd #497's defect class). +unload_launchd_if_loaded() { + local err_file rc=0 + if ! err_file="$(mktemp -t launchd-unload-err)"; then + die "could not create a temp file to capture 'launchctl unload' stderr — cannot tell a real + failure from a clean already-unloaded answer, so refusing to guess. The daemon's + supervision state was NOT touched." + fi + launchctl unload -w "$LAUNCHD_PLIST" >/dev/null 2>"$err_file" || rc=$? + if [ "$rc" -ne 0 ] && [ -s "$err_file" ]; then + die "'launchctl unload -w $LAUNCHD_PLIST' failed: $(cat "$err_file") + The daemon may still be under supervision; investigate before retrying." + fi + rm -f "$err_file" +} + +stop_systemd_if_loaded() { + local err_file rc=0 + if ! err_file="$(mktemp -t systemd-stop-err)"; then + die "could not create a temp file to capture 'systemctl --user stop' stderr — cannot tell a + real failure from a clean already-stopped answer, so refusing to guess. The daemon's + supervision state was NOT touched." + fi + systemctl --user stop "$SYSTEMD_UNIT" >/dev/null 2>"$err_file" || rc=$? + if [ "$rc" -ne 0 ] && [ -s "$err_file" ]; then + die "'systemctl --user stop $SYSTEMD_UNIT' failed: $(cat "$err_file") + The daemon may still be under supervision; investigate before retrying." + fi + rm -f "$err_file" +} + # fleetd #492: three real answers, not two — launchd, systemd, or genuinely unsupervised — plus a # fourth, "ambiguous", for the one case this script cannot tell apart: both signals firing at once. # That is exactly "I cannot tell who supervises this process", and guessing wrong here is how two @@ -881,16 +924,20 @@ if [ -n "$OLD_PID" ]; then elif [ "$SUPERVISOR_KIND" = "launchd" ]; then # Loaded but not currently running (e.g. throttled after a crash loop). Unload it anyway so the # start step below does a clean load, never a load stacked on an already-loaded label. + # fleetd #504: unload_launchd_if_loaded (above) tolerates a genuine already-unloaded answer but + # dies on a real `launchctl` failure — never a bare `|| true` that would print `ok` either way. say "stop" RESTART_MARK="$(wc -l < "$OUT" 2>/dev/null || echo 0)" - launchctl unload -w "$LAUNCHD_PLIST" 2>/dev/null || true + unload_launchd_if_loaded ok "launchd agent unloaded (was already not running)" elif [ "$SUPERVISOR_KIND" = "systemd" ]; then # Same case for systemd: the unit is known/active-capable but not currently running. `stop` on an # already-stopped unit is a harmless no-op — kept for symmetry with the launchd branch above. + # fleetd #504: stop_systemd_if_loaded (above) tolerates that genuine no-op but dies on a real + # `systemctl` failure — never a bare `|| true` that would print `ok` either way. say "stop" RESTART_MARK="$(wc -l < "$OUT" 2>/dev/null || echo 0)" - systemctl --user stop "$SYSTEMD_UNIT" 2>/dev/null || true + stop_systemd_if_loaded ok "systemd --user unit stopped (was already not running)" else RESTART_MARK="$(wc -l < "$OUT" 2>/dev/null || echo 0)" diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh index c720272..dba8c2f 100755 --- a/scripts/test-redeploy-fleetd.sh +++ b/scripts/test-redeploy-fleetd.sh @@ -621,6 +621,110 @@ test_refuse_drain_gate_call_site_present() { || fail "could not find the main flow's refuse_drain_gate call site in redeploy-fleetd.sh" } +# fleetd #504 — the "loaded but not currently running" branches for launchd/systemd used to run +# `launchctl unload`/`systemctl --user stop` with `2>/dev/null || true` and print `ok` +# unconditionally, so a real supervisor failure (e.g. it cannot reach launchd/the systemd user bus) +# read exactly like a harmless already-stopped answer. unload_launchd_if_loaded/ +# stop_systemd_if_loaded (redeploy-fleetd.sh, right after systemd_loaded) apply systemd_loaded's own +# "capture stderr separately — only a non-zero exit WITH stderr is a real failure" pattern to the +# WRITE side. Both call the real `launchctl`/`systemctl` binaries directly (they are not overridable +# wrapper functions the way launchd_loaded/systemd_loaded are), so these tests put a stub binary +# first on PATH — the same technique test_detect_supervisor_systemd_probe_error_is_unclear above +# already uses for `systemctl`. +test_unload_launchd_if_loaded_dies_on_real_failure() { + local bin_dir output rc=0 saved_plist="$LAUNCHD_PLIST" + bin_dir="$TMP/stub-bin-launchctl-error"; mkdir -p "$bin_dir" + cat > "$bin_dir/launchctl" <<'STUB' +#!/usr/bin/env bash +echo "Could not find specified service" >&2 +exit 1 +STUB + chmod +x "$bin_dir/launchctl" + LAUNCHD_PLIST="$TMP/fake-fail.plist" + output="$(PATH="$bin_dir:$PATH" unload_launchd_if_loaded 2>&1)" || rc=$? + LAUNCHD_PLIST="$saved_plist" + [ "$rc" -ne 0 ] \ + || fail "unload_launchd_if_loaded must die when launchctl exits non-zero AND writes to stderr" + printf '%s' "$output" | grep -qF 'launchctl unload' \ + || fail "die message does not name the failing launchctl unload command" +} + +# Captured via $(...) rather than called bare: unload_launchd_if_loaded's own die() does a hard +# `exit`, and calling it directly at this level would let a regression that makes it die on this +# clean-negative case kill the WHOLE suite before the `|| fail` below ever ran — printing die's own +# message instead of this test's. Inside a command substitution, that `exit` only ends the subshell +# (a-guard-is-defeated-by-its-calling-context: the same reason the *_dies_on_real_failure tests +# above capture this way), so this test's own message is what actually reaches the report. +test_unload_launchd_if_loaded_tolerates_clean_negative() { + local bin_dir saved_plist="$LAUNCHD_PLIST" output rc=0 + bin_dir="$TMP/stub-bin-launchctl-noop"; mkdir -p "$bin_dir" + cat > "$bin_dir/launchctl" <<'STUB' +#!/usr/bin/env bash +exit 1 +STUB + chmod +x "$bin_dir/launchctl" + LAUNCHD_PLIST="$TMP/fake-noop.plist" + output="$(PATH="$bin_dir:$PATH" unload_launchd_if_loaded 2>&1)" || rc=$? + LAUNCHD_PLIST="$saved_plist" + [ "$rc" -eq 0 ] \ + || fail "unload_launchd_if_loaded must tolerate a clean already-unloaded answer (non-zero exit, empty stderr): $output" +} + +test_stop_systemd_if_loaded_dies_on_real_failure() { + local bin_dir output rc=0 + bin_dir="$TMP/stub-bin-systemctl-stop-error"; mkdir -p "$bin_dir" + cat > "$bin_dir/systemctl" <<'STUB' +#!/usr/bin/env bash +echo "Failed to connect to bus: No such file or directory" >&2 +exit 1 +STUB + chmod +x "$bin_dir/systemctl" + output="$(PATH="$bin_dir:$PATH" stop_systemd_if_loaded 2>&1)" || rc=$? + [ "$rc" -ne 0 ] \ + || fail "stop_systemd_if_loaded must die when systemctl exits non-zero AND writes to stderr" + printf '%s' "$output" | grep -qF 'systemctl --user stop' \ + || fail "die message does not name the failing systemctl --user stop command" +} + +# Same subshell-capture reasoning as test_unload_launchd_if_loaded_tolerates_clean_negative above: +# stop_systemd_if_loaded's own die() does a hard `exit`, so this must run inside $(...) or a +# regression here would kill the whole suite with die's message instead of this test's. +test_stop_systemd_if_loaded_tolerates_clean_negative() { + local bin_dir output rc=0 + bin_dir="$TMP/stub-bin-systemctl-stop-noop"; mkdir -p "$bin_dir" + cat > "$bin_dir/systemctl" <<'STUB' +#!/usr/bin/env bash +exit 1 +STUB + chmod +x "$bin_dir/systemctl" + output="$(PATH="$bin_dir:$PATH" stop_systemd_if_loaded 2>&1)" || rc=$? + [ "$rc" -eq 0 ] \ + || fail "stop_systemd_if_loaded must tolerate a clean already-stopped answer (non-zero exit, empty stderr): $output" +} + +# Closes the same gap test_refuse_drain_gate_call_site_present closes for the drain gate: the four +# tests above call unload_launchd_if_loaded/stop_systemd_if_loaded directly, and sourcing stops +# before the main flow ever runs (the SOURCED guard), so none of them can prove the main flow still +# CALLS these two functions instead of the original bare `2>/dev/null || true`. A source-text check, +# like test_swap_ordered_after_wait_and_before_start. The call-site needle is anchored (`^ name$`) +# so it cannot be satisfied by the comment lines above each call site that merely mention the +# function by name. +test_stop_branches_call_tolerant_helpers_not_bare_or_true() { + local src="$ROOT/scripts/redeploy-fleetd.sh" unload_call_line stop_call_line + unload_call_line="$(grep -n '^ unload_launchd_if_loaded$' "$src" | head -1 | cut -d: -f1 || true)" + stop_call_line="$(grep -n '^ stop_systemd_if_loaded$' "$src" | head -1 | cut -d: -f1 || true)" + [ -n "$unload_call_line" ] \ + || fail "could not find the main flow's call to unload_launchd_if_loaded in redeploy-fleetd.sh" + [ -n "$stop_call_line" ] \ + || fail "could not find the main flow's call to stop_systemd_if_loaded in redeploy-fleetd.sh" + if grep -qF 'launchctl unload -w "$LAUNCHD_PLIST" 2>/dev/null || true' "$src"; then + fail "the bare 'launchctl unload ... 2>/dev/null || true' defect (fleetd #504) is back in redeploy-fleetd.sh" + fi + if grep -qF 'systemctl --user stop "$SYSTEMD_UNIT" 2>/dev/null || true' "$src"; then + fail "the bare 'systemctl --user stop ... 2>/dev/null || true' defect (fleetd #504) is back in redeploy-fleetd.sh" + fi +} + test_no_errors() { cat > "$TMP/no-errors.log" <<'LOG' 2026-09-05 12:00:00 INFO fleetd listening @@ -1016,6 +1120,11 @@ test_refuse_drain_gate_build_ran_staged_absent test_refuse_drain_gate_no_build_staged_present test_refuse_drain_gate_no_build_staged_absent test_refuse_drain_gate_call_site_present +test_unload_launchd_if_loaded_dies_on_real_failure +test_unload_launchd_if_loaded_tolerates_clean_negative +test_stop_systemd_if_loaded_dies_on_real_failure +test_stop_systemd_if_loaded_tolerates_clean_negative +test_stop_branches_call_tolerant_helpers_not_bare_or_true test_no_errors test_recovery_patterns_match_source test_attributed_recovered_connection_error