diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh index bffed1e..08be7af 100755 --- a/scripts/redeploy-fleetd.sh +++ b/scripts/redeploy-fleetd.sh @@ -75,6 +75,12 @@ LAUNCHD_PLIST="$HOME/Library/LaunchAgents/$LAUNCHD_LABEL.plist" # "dev.ltms.fleetd" — systemd user units here are not namespaced the way the launchd label is). SYSTEMD_UNIT='fleetd' +# fleetd #492 follow-up: detect_supervisor packs TWO values (kind, detail) onto the one stdout +# line that survives its $(...) call — see the constraints comment above that function. This is +# the separator between them: the ASCII "unit separator" byte, chosen because it never occurs in +# any of the prose detail strings and needs no escaping in a `case`/glob pattern. +SUPERVISOR_DETAIL_SEP=$'\x1f' + # fleetd #492 follow-up: set by systemd_loaded/systemd_installed when the underlying `systemctl` # call could not answer cleanly — it exited non-zero AND wrote something to stderr, which is a real # tool failure (e.g. it cannot reach the user bus over a non-lingering ssh session), never the same @@ -84,10 +90,14 @@ SYSTEMD_UNIT='fleetd' # reads a deterministic 0 rather than whatever a previous probe call left behind. SYSTEMD_LOADED_ERRORED=0 SYSTEMD_INSTALLED_ERRORED=0 -# fleetd #492 follow-up: set by detect_supervisor alongside an "unclear" answer, naming the specific -# supervisor and reason so require_drivable_supervisor's die() message is not just the bare word -# "unclear". Initialized empty for the same `set -u` reason as above. -SUPERVISOR_UNCLEAR_DETAIL="" +# fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL is the specific supervisor/reason that +# require_drivable_supervisor's die() names on an "unclear" answer. Deliberately NOT pre-declared +# here (unlike the two flags above): it is set only by the real call site, right after it unpacks +# detect_supervisor's stdout (see the constraints comment above detect_supervisor). If that call +# site is ever skipped or broken, a bare `set -u` reference to this variable in +# require_drivable_supervisor must fail loudly with "unbound variable" — a pre-declared empty +# default would instead silently print an empty reason, hiding exactly the value this ticket +# exists to surface. DO_BUILD=1; ASSUME_YES=0; CHECK_ONLY=0 for arg in "$@"; do @@ -187,17 +197,33 @@ systemd_loaded() { # message — a real tool failure, e.g. it cannot reach the user bus over a non-lingering ssh # session — never conflated with a clean negative answer. # "none" now means only: neither supervisor is installed, neither is loaded, and neither probe -# errored. SUPERVISOR_UNCLEAR_DETAIL is set alongside "unclear" so require_drivable_supervisor's -# die() can name the specific supervisor and reason, not just the bare word "unclear". +# errored. # -# Pure and side-effect-free besides setting the two globals above: reads the four probes and -# decides — never mutates anything, so it is safe under --check and testable by overriding +# fleetd #492 follow-up — constraints every caller of this function depends on (learned the hard +# way: an earlier version of this fix set a SUPERVISOR_UNCLEAR_DETAIL global from inside here and +# it was silently lost, because every real call site invokes this as `$(detect_supervisor)`): +# 1. It is called as `$(detect_supervisor)`, so ONLY STDOUT crosses back to the caller. Anything +# this function needs to tell its caller — the "unclear" detail included — must be printed, +# never assigned to a global: a global set inside a `$( )` subshell dies with that subshell. +# This function packs BOTH values (kind and detail) onto that one stdout line, joined by +# $SUPERVISOR_DETAIL_SEP, and the caller unpacks them on its own side of the subshell boundary. +# 2. This script runs under `set -euo pipefail` (line 50), so an unset variable is a loud +# failure. Do not add a `${VAR:-default}` anywhere downstream to paper over a value that +# should always be there — that hides a lost value instead of surfacing it (fleetd #497's +# defect class). +# 3. Every `case` on this function's return value needs an explicit final `*)` arm, chosen by +# whether that caller ACTS on the value (`die` — an unrecognised value must never be silently +# driven) or only DISPLAYS it (`echo`/`warn` and continue — a diagnostic must not go silent on +# exactly the value it most needs to report). +# +# Pure and side-effect-free besides the two *_ERRORED flags (read back within this same call, never +# by the caller — see the constraints above): reads the four probes and decides — never mutates +# anything, so it is safe under --check and testable by overriding # launchd_installed/launchd_loaded/systemd_installed/systemd_loaded after sourcing. detect_supervisor() { - local ld=0 sd=0 li=0 si=0 + local ld=0 sd=0 li=0 si=0 kind detail="" SYSTEMD_LOADED_ERRORED=0 SYSTEMD_INSTALLED_ERRORED=0 - SUPERVISOR_UNCLEAR_DETAIL="" launchd_loaded && ld=1 systemd_loaded && sd=1 @@ -205,23 +231,25 @@ detect_supervisor() { systemd_installed && si=1 if [ "$SYSTEMD_LOADED_ERRORED" = 1 ] || [ "$SYSTEMD_INSTALLED_ERRORED" = 1 ]; then - SUPERVISOR_UNCLEAR_DETAIL="the systemd --user probe for '$SYSTEMD_UNIT' could not answer cleanly (systemctl exited non-zero and reported an error on stderr, not a clean negative — e.g. it cannot reach the user bus)" - echo "unclear" + detail="the systemd --user probe for '$SYSTEMD_UNIT' could not answer cleanly (systemctl exited non-zero and reported an error on stderr, not a clean negative — e.g. it cannot reach the user bus)" + kind="unclear" elif [ "$ld" = 1 ] && [ "$sd" = 1 ]; then - echo "ambiguous" + kind="ambiguous" elif [ "$li" = 1 ] && [ "$ld" = 0 ]; then - SUPERVISOR_UNCLEAR_DETAIL="the launchd agent ($LAUNCHD_LABEL) is installed ($LAUNCHD_PLIST exists) but is not currently loaded" - echo "unclear" + detail="the launchd agent ($LAUNCHD_LABEL) is installed ($LAUNCHD_PLIST exists) but is not currently loaded" + kind="unclear" elif [ "$si" = 1 ] && [ "$sd" = 0 ]; then - SUPERVISOR_UNCLEAR_DETAIL="the systemd --user unit ($SYSTEMD_UNIT) is installed but not currently active — it may be activating, deactivating, failed, or waiting on an auto-restart" - echo "unclear" + detail="the systemd --user unit ($SYSTEMD_UNIT) is installed but not currently active — it may be activating, deactivating, failed, or waiting on an auto-restart" + kind="unclear" elif [ "$ld" = 1 ]; then - echo "launchd" + kind="launchd" elif [ "$sd" = 1 ]; then - echo "systemd" + kind="systemd" else - echo "none" + kind="none" fi + + printf '%s%s%s' "$kind" "$SUPERVISOR_DETAIL_SEP" "$detail" } # fleetd #492: turns anything detect_supervisor returns that is NOT exactly one of the two @@ -240,16 +268,18 @@ require_drivable_supervisor() { prevent. Stop one of the two supervisors by hand, confirm only one remains loaded, then rerun." ;; unclear) - # fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL is set by detect_supervisor right before - # it returns "unclear"; the fallback text below only fires if this is ever called directly - # (as a test does) without going through detect_supervisor first. + # fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL crosses back from detect_supervisor's + # subshell via its stdout, unpacked by the caller BEFORE it calls this function (see the + # constraints comment above detect_supervisor). No ${VAR:-default} here on purpose: if the + # detail is somehow missing, `set -u` makes this reference fail loudly instead of silently + # naming nothing — a default that hides a lost value is the same defect class as fleetd + # #497. die "a supervisor looks present but this script cannot tell whether it actually drives this - daemon: ${SUPERVISOR_UNCLEAR_DETAIL:-launchd ($LAUNCHD_LABEL) or systemd --user - ($SYSTEMD_UNIT) reported something other than a clean 'loaded' or a clean 'not loaded'}. - Guessing wrong here is the same failure 'ambiguous' above exists to prevent: driving the - daemon while an unseen supervisor revives the OLD jar out from under it (fleetd #492). - Check 'launchctl list $LAUNCHD_LABEL' and 'systemctl --user status $SYSTEMD_UNIT' by - hand, resolve whichever looks unclear, then rerun." ;; + daemon: $SUPERVISOR_UNCLEAR_DETAIL. Guessing wrong here is the same failure 'ambiguous' + above exists to prevent: driving the daemon while an unseen supervisor revives the OLD + jar out from under it (fleetd #492). Check 'launchctl list $LAUNCHD_LABEL' and + 'systemctl --user status $SYSTEMD_UNIT' by hand, resolve whichever looks unclear, then + rerun." ;; *) die "detect_supervisor returned an unrecognized value '$kind' — refusing to guess which supervisor, if any, controls this daemon." ;; @@ -401,7 +431,12 @@ fi # outright — before touching anything — if that cannot be told apart (see require_drivable_ # supervisor above). --check reaches this same line, so a host with an undrivable supervisor is # reported as a failure even in --check, without ever reaching the build/stop/start steps. -SUPERVISOR_KIND="$(detect_supervisor)" +# fleetd #492 follow-up: detect_supervisor runs as $(...), so only the printed line survives — +# unpack kind and detail from it HERE, in this shell, before calling anything downstream. See the +# constraints comment above detect_supervisor for why this cannot be done any other way. +SUPERVISOR_RAW="$(detect_supervisor)" +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 @@ -420,6 +455,13 @@ case "$SUPERVISOR_KIND" in 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 # The trap with no log line. Checked in a LOGIN shell, because that is how the daemon is started @@ -534,6 +576,14 @@ if [ -n "$OLD_PID" ]; then 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 for _ in $(seq "$STOP_WAIT"); do [ -z "$(running_pid)" ] && break @@ -607,6 +657,14 @@ case "$SUPERVISOR_KIND" in # 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 for _ in $(seq 10); do diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh index 8e452b7..62d7d00 100755 --- a/scripts/test-redeploy-fleetd.sh +++ b/scripts/test-redeploy-fleetd.sh @@ -25,6 +25,17 @@ classify_fixture() { classify_amqp_connection_errors "$TMP/$name" } +# fleetd #492 follow-up: detect_supervisor's stdout is now "kinddetail" (see the constraints +# comment above detect_supervisor in redeploy-fleetd.sh) — every test below that only cares about +# the kind must split it out with the SAME in-shell parameter expansion the real call site (:438) +# uses, never a bare string comparison against the raw output. +supervisor_kind_of() { + printf '%s' "${1%%"$SUPERVISOR_DETAIL_SEP"*}" +} +supervisor_detail_of() { + printf '%s' "${1#*"$SUPERVISOR_DETAIL_SEP"}" +} + # fleetd #492 — supervisor detection. Detect_supervisor() reads launchd_loaded/systemd_loaded, so # each test overrides BOTH pairs (installed + loaded) explicitly, rather than relying on either @@ -37,7 +48,7 @@ test_detect_supervisor_launchd_only() { launchd_loaded() { return 0; } systemd_installed() { return 1; } systemd_loaded() { return 1; } - assert_equals "launchd" "$(detect_supervisor)" "launchd-only detection" + assert_equals "launchd" "$(supervisor_kind_of "$(detect_supervisor)")" "launchd-only detection" } test_detect_supervisor_systemd_only() { @@ -45,7 +56,7 @@ test_detect_supervisor_systemd_only() { launchd_loaded() { return 1; } systemd_installed() { return 0; } systemd_loaded() { return 0; } - assert_equals "systemd" "$(detect_supervisor)" "systemd-only detection" + assert_equals "systemd" "$(supervisor_kind_of "$(detect_supervisor)")" "systemd-only detection" } test_detect_supervisor_none() { @@ -53,7 +64,7 @@ test_detect_supervisor_none() { launchd_loaded() { return 1; } systemd_installed() { return 1; } systemd_loaded() { return 1; } - assert_equals "none" "$(detect_supervisor)" "unsupervised detection" + assert_equals "none" "$(supervisor_kind_of "$(detect_supervisor)")" "unsupervised detection" } # fleetd #492 follow-up — detect_supervisor must never answer "none" when the truth is "could not @@ -66,7 +77,11 @@ test_detect_supervisor_systemd_installed_not_loaded_is_unclear() { launchd_loaded() { return 1; } systemd_installed() { return 0; } # the unit file IS there systemd_loaded() { return 1; } # is-active says no — could be activating/failed/pending restart - assert_equals "unclear" "$(detect_supervisor)" "systemd installed-but-not-loaded must read as unclear, not none" + local raw + raw="$(detect_supervisor)" + assert_equals "unclear" "$(supervisor_kind_of "$raw")" "systemd installed-but-not-loaded must read as unclear, not none" + printf '%s' "$(supervisor_detail_of "$raw")" | grep -qF "$SYSTEMD_UNIT" \ + || fail "detail does not name the systemd unit it found installed-but-not-loaded" } # Same fact, the launchd side: a plist on disk that is not currently loaded (unloaded without being @@ -76,7 +91,11 @@ test_detect_supervisor_launchd_installed_not_loaded_is_unclear() { launchd_loaded() { return 1; } # launchctl list says not loaded systemd_installed() { return 1; } systemd_loaded() { return 1; } - assert_equals "unclear" "$(detect_supervisor)" "launchd installed-but-not-loaded must read as unclear, not none" + local raw + raw="$(detect_supervisor)" + assert_equals "unclear" "$(supervisor_kind_of "$raw")" "launchd installed-but-not-loaded must read as unclear, not none" + printf '%s' "$(supervisor_detail_of "$raw")" | grep -qF "$LAUNCHD_LABEL" \ + || fail "detail does not name the launchd label it found installed-but-not-loaded" } # Drives the REAL systemd_loaded/systemd_installed bodies (never stubbed) through a `systemctl` @@ -108,24 +127,42 @@ STUB launchd_installed() { return 1; } launchd_loaded() { return 1; } result="$(PATH="$bin_dir:$PATH" detect_supervisor)" - assert_equals "unclear" "$result" "a systemd probe error must read as unclear, not none" + assert_equals "unclear" "$(supervisor_kind_of "$result")" "a systemd probe error must read as unclear, not none" + printf '%s' "$(supervisor_detail_of "$result")" | grep -qF "$SYSTEMD_UNIT" \ + || fail "detail does not name the systemd unit whose probe errored" } -# The other half of the ticket: an "unclear" supervisor must refuse exactly like "ambiguous" does — -# die(), never fall through to the `kill` path — and the refusal must name the specific supervisor -# and reason detect_supervisor found, not just the bare word "unclear". +# fleetd #492 follow-up (Item 1): this must go through the REAL call-site shape at :437-440, not a +# hand-constructed "unclear" value — a test that builds "unclear" directly proves the switch, not +# the handoff, and that is exactly the gap that let SUPERVISOR_UNCLEAR_DETAIL never reach the real +# caller in b17f37a. detect_supervisor runs as $(detect_supervisor): a subshell. Only stdout +# survives that boundary, so kind AND detail must both cross on it — this test proves they do. test_require_drivable_supervisor_refuses_unclear() { launchd_installed() { return 1; } launchd_loaded() { return 1; } systemd_installed() { return 0; } systemd_loaded() { return 1; } - local kind output rc=0 - kind="$(detect_supervisor)" - assert_equals "unclear" "$kind" "setup: expected unclear before testing the refusal" - output="$(require_drivable_supervisor "$kind" 2>&1)" || rc=$? + + local SUPERVISOR_RAW SUPERVISOR_KIND SUPERVISOR_UNCLEAR_DETAIL output rc=0 + # Exactly what :437-439 does — do not shortcut this by constructing "unclear" by hand. + SUPERVISOR_RAW="$(detect_supervisor)" + SUPERVISOR_KIND="${SUPERVISOR_RAW%%"$SUPERVISOR_DETAIL_SEP"*}" + SUPERVISOR_UNCLEAR_DETAIL="${SUPERVISOR_RAW#*"$SUPERVISOR_DETAIL_SEP"}" + + assert_equals "unclear" "$SUPERVISOR_KIND" "setup: expected unclear before testing the refusal" + [ -n "$SUPERVISOR_UNCLEAR_DETAIL" ] \ + || fail "detail did not survive the \$(...) call-site boundary — SUPERVISOR_UNCLEAR_DETAIL is empty in the parent shell" + printf '%s' "$SUPERVISOR_UNCLEAR_DETAIL" | grep -qF "$SYSTEMD_UNIT" \ + || fail "detail that crossed the subshell boundary does not name the systemd unit it found installed-but-not-loaded" + + output="$(require_drivable_supervisor "$SUPERVISOR_KIND" 2>&1)" || rc=$? [ "$rc" -ne 0 ] || fail "require_drivable_supervisor accepted an unclear (undrivable) supervisor" - printf '%s' "$output" | grep -qF "$SYSTEMD_UNIT" \ - || fail "refusal message does not name the systemd unit it found installed-but-not-loaded" + # Check for the ACTUAL DETAIL TEXT, not just "$SYSTEMD_UNIT" — the die() message's boilerplate + # recovery instructions name the unit unconditionally either way ("systemctl --user status + # $SYSTEMD_UNIT"), so a bare unit-name grep here would pass even on a lost/fallback detail. Only + # the specific detail string proves the crossed value, not the boilerplate, reached the message. + printf '%s' "$output" | grep -qF "$SUPERVISOR_UNCLEAR_DETAIL" \ + || fail "refusal message does not contain the specific detail that crossed the subshell boundary" } # The heart of the ticket: a supervisor this script cannot drive must refuse, never fall through to