From c89a375e5d61ad6521a32be30cda3d201123d170 Mon Sep 17 00:00:00 2001
From: Dai Ha <dai.ha@magnolia-cms.com>
Date: Sat, 12 Sep 2026 11:42:46 +0700
Subject: [PATCH 1/2] fleetd #521: extract should_swap so the swap guard can't
 be silently disabled

Mutating the swap step's guard (if [ "$DO_BUILD" = 1 ] -> if false) left the
whole test suite green: test_swap_ordered_after_wait_and_before_start only
checks source positions, which an in-place if-condition edit never moves.
Extracts the decision into should_swap(do_build), following the same shape as
#510's wait_for_daemon_exit and #517's drain_gate_refusal, with a direct test
for each value.
---
 scripts/redeploy-fleetd.sh      | 18 +++++++++++++++++-
 scripts/test-redeploy-fleetd.sh | 17 +++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh
index 36a423d..1cb18d5 100755
--- a/scripts/redeploy-fleetd.sh
+++ b/scripts/redeploy-fleetd.sh
@@ -178,6 +178,22 @@ swap_staged_jar() {
         may recover this once you find out why the move failed."
 }
 
+# fleetd #521: extracted so the suite can call this decision directly, the same way #510 extracted
+# wait_for_daemon_exit (so its ordering became checkable) and #517 extracted drain_gate_refusal (so
+# its abort branch became checkable) — see the comments above each. Before this, the only test of
+# the swap step was test_swap_ordered_after_wait_and_before_start, a SOURCE-POSITION test: it checks
+# where swap_staged_jar's call site sits relative to wait_for_daemon_exit and the start step, by
+# reading this script's own text. Mutating the swap step's guard (`if [ "$DO_BUILD" = 1 ]` -> `if
+# false`) leaves every one of those line positions unchanged, so that test stayed green while the
+# swap never ran — a live redeploy would then start (or try to start) with no jar at the live path,
+# since stage_built_jar already moved it out during the build step above, regardless of this guard.
+# Pure: decides only whether a swap should happen, no side effects, so a test can call it directly
+# with both values of do_build instead of driving the real build/stop/start flow.
+should_swap() {
+  local do_build="$1"
+  [ "$do_build" = 1 ]
+}
+
 # `launchctl list <label>` exits 0 iff the label is loaded (registered with launchd) — true whether
 # or not it is currently running, which is exactly "supervision is active" for our purposes. Read-
 # only: neither helper below changes anything, so both are also safe under --check.
@@ -720,7 +736,7 @@ fi
 # NOW is it safe to put the freshly built jar at the path the NEXT `java -jar` (direct, or via
 # launchd/systemd's ExecStart) will read from — this mv is the one and only write to $JAR anywhere
 # in this script's mutating flow. If it fails, do not start: die() below exits before "start" runs.
-if [ "$DO_BUILD" = 1 ]; then
+if should_swap "$DO_BUILD"; then
   say "swap"
   swap_staged_jar "$JAR_STAGED" "$JAR"
   ok "jar in place: $(jar_id)"
diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh
index 9647a3e..b2c479d 100755
--- a/scripts/test-redeploy-fleetd.sh
+++ b/scripts/test-redeploy-fleetd.sh
@@ -365,6 +365,21 @@ test_wait_for_daemon_exit_times_out_if_pid_never_clears() {
   source "$ROOT/scripts/redeploy-fleetd.sh"   # restore the real running_pid/sleep for later tests
 }
 
+# fleetd #521 — the swap step's own guard. Before this, `if [ "$DO_BUILD" = 1 ]` (the swap guard)
+# could be mutated to `if false` and every test here still passed: nothing called the decision
+# directly, and test_swap_ordered_after_wait_and_before_start below only checks source POSITIONS,
+# which a same-line `if [...]` -> `if false` edit never moves. These two tests call should_swap()
+# directly instead, so they fail if the swap guard becomes unreachable OR if its logic regresses.
+test_should_swap_true_when_build_ran() {
+  should_swap 1 || fail "should_swap 1 (a build ran and staged a jar) must return true"
+}
+
+test_should_swap_false_when_build_skipped() {
+  if should_swap 0; then
+    fail "should_swap 0 (--no-build; nothing was staged this run) must return false"
+  fi
+}
+
 # fleetd #493 item 2: "put the swap after that wait, before the start." Sourcing stops before the
 # main flow ever runs (see the SOURCED guard in redeploy-fleetd.sh), so the ordering guarantee
 # itself — as opposed to the pure functions it's built from — can only be checked by reading the
@@ -670,6 +685,8 @@ test_stage_built_jar_dies_when_build_produced_nothing
 test_swap_staged_jar_moves_staged_onto_live
 test_swap_staged_jar_dies_without_staged_file
 test_swap_staged_jar_dies_when_mv_fails
+test_should_swap_true_when_build_ran
+test_should_swap_false_when_build_skipped
 test_require_no_build_jar_dies_when_absent
 test_require_no_build_jar_accepts_present_jar
 test_wait_for_daemon_exit_returns_true_once_pid_clears

From 08771e270bab24407e14c1eac42948f673dbd40d Mon Sep 17 00:00:00 2001
From: Dai Ha <dai.ha@magnolia-cms.com>
Date: Sat, 12 Sep 2026 11:58:34 +0700
Subject: [PATCH 2/2] fleetd #521 gate fix: pin the swap at the call site, not
 just the predicate
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The extraction in the previous commit did what #521 asked for — a
should_swap() predicate with a test for each value — and I measured that
it does not close the defect. With the main flow reading
`if should_swap "$DO_BUILD"; then`, changing that line to `if false; then`
left the whole suite at exit 0 with zero FAIL lines. The swap still never
ran, and a redeploy would still report success while starting on no jar.

That is my ticket's fault, not the implementer's: "extract the decision so
the suite can call it" pins the decision and never the wiring. Extraction
moved the untested decision up one level instead of removing it.

Fix: the decision and the action now live together in swap_if_built(),
which the main flow calls unconditionally — there is no guard left in the
main flow to get wrong. should_swap() stays, because it is the decision
and is worth naming and testing on its own. Two new tests call
swap_if_built() with a recording stub in place of the real mv, so they
fail if the guard is removed, inverted, or stops being consulted.

Also fixed, found while verifying this:

* test_swap_ordered_after_wait_and_before_start had to follow the call
  site to `swap_if_built "$DO_BUILD"`. Left on the old needle it reported
  "swap_staged_jar (line 215) is not after wait_for_daemon_exit (line
  730)" — true of a function definition, and nothing about step order.
* That test's three `[ -n ... ] || fail "could not find ... call site"`
  guards were dead code. Under `set -euo pipefail` an absent needle fails
  the assignment and `set -e` kills the suite before the guard runs.
  Measured: deleting the swap call gave exit 1 with ZERO bytes of output,
  no FAIL line, nothing naming what was missing. Each grep now ends in
  `|| true` so the assignment succeeds empty and the guard can speak.

Verified by me on this revision:

* suite exit 0, 0 `^FAIL:` lines, 44 tests defined and 44 invoked
* bash -n rc=0 on both scripts under /bin/bash 3.2.57 and bash 5.3.9
* four mutations, each killed with its own named FAIL line, each restored
  byte-identical, green control after the battery:
  - guard removed inside swap_if_built -> "must not swap, but it did"
  - guard inverted                     -> "must perform the swap, and did not"
  - should_swap's comparison changed    -> "must return true"
  - main-flow call deleted              -> "could not find the swap call
    site in redeploy-fleetd.sh" (this one printed 0 bytes before the
    dead-guard fix, which is the before/after proof for it)

Not fixed here, filed separately: drain_gate_refusal has the same shape.
Replacing `die "$(drain_gate_refusal ...)"` with `die "aborted — nothing
changed"` leaves the suite at exit 0 with output byte-identical to a clean
run, which reinstates the exact wrong message #517 was filed to fix.
---
 scripts/redeploy-fleetd.sh      | 50 +++++++++++++++-------
 scripts/test-redeploy-fleetd.sh | 76 ++++++++++++++++++++++++++++-----
 2 files changed, 100 insertions(+), 26 deletions(-)

diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh
index 1cb18d5..e3b0fb6 100755
--- a/scripts/redeploy-fleetd.sh
+++ b/scripts/redeploy-fleetd.sh
@@ -178,22 +178,44 @@ swap_staged_jar() {
         may recover this once you find out why the move failed."
 }
 
-# fleetd #521: extracted so the suite can call this decision directly, the same way #510 extracted
-# wait_for_daemon_exit (so its ordering became checkable) and #517 extracted drain_gate_refusal (so
-# its abort branch became checkable) — see the comments above each. Before this, the only test of
-# the swap step was test_swap_ordered_after_wait_and_before_start, a SOURCE-POSITION test: it checks
-# where swap_staged_jar's call site sits relative to wait_for_daemon_exit and the start step, by
-# reading this script's own text. Mutating the swap step's guard (`if [ "$DO_BUILD" = 1 ]` -> `if
-# false`) leaves every one of those line positions unchanged, so that test stayed green while the
-# swap never ran — a live redeploy would then start (or try to start) with no jar at the live path,
-# since stage_built_jar already moved it out during the build step above, regardless of this guard.
-# Pure: decides only whether a swap should happen, no side effects, so a test can call it directly
-# with both values of do_build instead of driving the real build/stop/start flow.
+# fleetd #521 — the swap decision, and the step that acts on it.
+#
+# The defect: the swap step used to be guarded inline by `if [ "$DO_BUILD" = 1 ]` in the main flow.
+# Changing that to `if false` left the suite green and the swap never ran, so a redeploy reported
+# every step succeeding while the daemon started on no jar at all (stage_built_jar has already moved
+# the freshly built one to $JAR_STAGED by then) or on a stale one.
+# test_swap_ordered_after_wait_and_before_start could not catch it: it reads this script's own text
+# and compares line positions, and a same-line edit moves no line.
+#
+# Why these are TWO functions, and why the second one exists at all. Extracting only the predicate
+# — `should_swap`, which is what #521 asked for — is not enough, and this was measured, not guessed:
+# with the main flow calling `if should_swap "$DO_BUILD"; then`, changing THAT to `if false; then`
+# still left the whole suite at exit 0 with no failures. Tests that call a predicate directly prove
+# the predicate is right; nothing makes the code that does the work consult it. Extraction had moved
+# the untested decision one level up rather than removing it.
+#
+# So the decision and the action live together in swap_if_built, and the main flow has no guard of
+# its own to get wrong — it calls one function unconditionally. A test then calls swap_if_built with
+# both values of do_build and checks whether the swap actually happened, which fails if the guard is
+# removed, inverted, or stops being consulted. should_swap stays a separate predicate because it is
+# the decision itself and is worth naming and testing on its own.
+#
+# What this still does not pin: deleting the swap_if_built call from the main flow altogether. That
+# is the ordering test's job — its needle is that call site — and no test in this file can do better,
+# because sourcing stops before the main flow ever runs (see the SOURCED guard below).
 should_swap() {
   local do_build="$1"
   [ "$do_build" = 1 ]
 }
 
+swap_if_built() {
+  local do_build="$1"
+  should_swap "$do_build" || return 0
+  say "swap"
+  swap_staged_jar "$JAR_STAGED" "$JAR"
+  ok "jar in place: $(jar_id)"
+}
+
 # `launchctl list <label>` exits 0 iff the label is loaded (registered with launchd) — true whether
 # or not it is currently running, which is exactly "supervision is active" for our purposes. Read-
 # only: neither helper below changes anything, so both are also safe under --check.
@@ -736,11 +758,7 @@ fi
 # NOW is it safe to put the freshly built jar at the path the NEXT `java -jar` (direct, or via
 # launchd/systemd's ExecStart) will read from — this mv is the one and only write to $JAR anywhere
 # in this script's mutating flow. If it fails, do not start: die() below exits before "start" runs.
-if should_swap "$DO_BUILD"; then
-  say "swap"
-  swap_staged_jar "$JAR_STAGED" "$JAR"
-  ok "jar in place: $(jar_id)"
-fi
+swap_if_built "$DO_BUILD"
 
 # ------------------------------------------------------------------ start
 # Unsupervised: login shell (zsh -l) is what puts the secrets on the daemon's environment, and cwd
diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh
index b2c479d..dad2c2c 100755
--- a/scripts/test-redeploy-fleetd.sh
+++ b/scripts/test-redeploy-fleetd.sh
@@ -365,11 +365,22 @@ test_wait_for_daemon_exit_times_out_if_pid_never_clears() {
   source "$ROOT/scripts/redeploy-fleetd.sh"   # restore the real running_pid/sleep for later tests
 }
 
-# fleetd #521 — the swap step's own guard. Before this, `if [ "$DO_BUILD" = 1 ]` (the swap guard)
-# could be mutated to `if false` and every test here still passed: nothing called the decision
-# directly, and test_swap_ordered_after_wait_and_before_start below only checks source POSITIONS,
-# which a same-line `if [...]` -> `if false` edit never moves. These two tests call should_swap()
-# directly instead, so they fail if the swap guard becomes unreachable OR if its logic regresses.
+# fleetd #521 — the swap step's guard, at two levels.
+#
+# The first two tests call the predicate should_swap() directly. They pin its logic, and that is all
+# they pin. On their own they did NOT close #521, and this was measured rather than argued: with the
+# main flow reading `if should_swap "$DO_BUILD"; then`, changing that line to `if false; then` left
+# this whole suite at exit 0 with zero FAIL lines, because nothing here made the code that performs
+# the swap consult the predicate at all. Extracting the decision had moved the untested decision up
+# a level, not removed it.
+#
+# So the last two tests call swap_if_built() — the function the main flow actually calls, holding the
+# guard and the swap together — with a recording stub in place of the real `mv`. Those fail if the
+# guard is removed, inverted, or stops being consulted.
+#
+# What none of these four can catch: deleting the `swap_if_built "$DO_BUILD"` line from the main flow
+# altogether. That is test_swap_ordered_after_wait_and_before_start's job below, because sourcing
+# stops before the main flow runs, so no test in this file can invoke it.
 test_should_swap_true_when_build_ran() {
   should_swap 1 || fail "should_swap 1 (a build ran and staged a jar) must return true"
 }
@@ -380,23 +391,66 @@ test_should_swap_false_when_build_skipped() {
   fi
 }
 
+# Both of these re-source redeploy-fleetd.sh at the START, because a bash function definition is
+# global for the rest of the process and an earlier test may have left swap_staged_jar or jar_id
+# overridden (see the longer note on this at test_detect_supervisor_systemd_probe_error_is_unclear),
+# and again at the END, so their own stubs do not leak into every test that runs after them.
+test_swap_if_built_performs_the_swap_when_build_ran() {
+  source "$ROOT/scripts/redeploy-fleetd.sh"
+  local marker="$TMP/swap-if-built-ran"
+  rm -f "$marker"
+  swap_staged_jar() { printf '%s -> %s\n' "$1" "$2" > "$marker"; }
+  jar_id() { printf 'stubbed\n'; }
+  swap_if_built 1 > /dev/null
+  [ -f "$marker" ] \
+    || fail "swap_if_built 1 (a build ran and staged a jar) must perform the swap, and did not"
+  source "$ROOT/scripts/redeploy-fleetd.sh"
+}
+
+test_swap_if_built_skips_the_swap_when_build_skipped() {
+  source "$ROOT/scripts/redeploy-fleetd.sh"
+  local marker="$TMP/swap-if-built-skipped"
+  rm -f "$marker"
+  swap_staged_jar() { printf 'swapped\n' > "$marker"; }
+  jar_id() { printf 'stubbed\n'; }
+  swap_if_built 0 > /dev/null
+  if [ -f "$marker" ]; then
+    fail "swap_if_built 0 (--no-build; nothing was staged this run) must not swap, but it did"
+  fi
+  source "$ROOT/scripts/redeploy-fleetd.sh"
+}
+
 # fleetd #493 item 2: "put the swap after that wait, before the start." Sourcing stops before the
 # main flow ever runs (see the SOURCED guard in redeploy-fleetd.sh), so the ordering guarantee
 # itself — as opposed to the pure functions it's built from — can only be checked by reading the
 # script's own call sites, the same way test_recovery_patterns_match_source below checks Java
 # source shape instead of behavior it cannot invoke directly.
+#
+# Two details about the three greps below, both of which have already gone wrong here.
+#
+# The needle for the swap is the MAIN FLOW's call site, `swap_if_built "$DO_BUILD"` — not
+# `swap_staged_jar "$JAR_STAGED" "$JAR"`. Since fleetd #521 that second string lives inside
+# swap_if_built's body, which is defined near the top of the script, far ABOVE the stop step. Using
+# it made this test report "swap_staged_jar (line 215) is not after wait_for_daemon_exit (line 730)"
+# — a true statement about a function definition, and nothing at all about the order of the steps.
+#
+# Each grep ends in `|| true`. This file runs under `set -euo pipefail`, and `pipefail` makes the
+# pipeline's status grep's status, so a needle that is simply ABSENT failed the assignment and `set
+# -e` killed the whole suite on the spot — before reaching the `[ -n ... ] || fail` line written to
+# report exactly that. Measured: the suite exited 1 having printed zero bytes, no FAIL line and no
+# name of the missing call site. `|| true` lets the assignment succeed empty so the guard can speak.
 test_swap_ordered_after_wait_and_before_start() {
   local src="$ROOT/scripts/redeploy-fleetd.sh" wait_line swap_line start_line
-  wait_line="$(grep -Fn 'wait_for_daemon_exit "$STOP_WAIT"' "$src" | head -1 | cut -d: -f1)"
-  swap_line="$(grep -Fn 'swap_staged_jar "$JAR_STAGED" "$JAR"' "$src" | head -1 | cut -d: -f1)"
-  start_line="$(grep -Fn 'say "start"' "$src" | head -1 | cut -d: -f1)"
+  wait_line="$(grep -Fn 'wait_for_daemon_exit "$STOP_WAIT"' "$src" | head -1 | cut -d: -f1 || true)"
+  swap_line="$(grep -Fn 'swap_if_built "$DO_BUILD"' "$src" | head -1 | cut -d: -f1 || true)"
+  start_line="$(grep -Fn 'say "start"' "$src" | head -1 | cut -d: -f1 || true)"
   [ -n "$wait_line" ] || fail "could not find the wait-for-exit call site in redeploy-fleetd.sh"
   [ -n "$swap_line" ] || fail "could not find the swap call site in redeploy-fleetd.sh"
   [ -n "$start_line" ] || fail "could not find the start section in redeploy-fleetd.sh"
   [ "$swap_line" -gt "$wait_line" ] \
-    || fail "swap_staged_jar (line $swap_line) is not after wait_for_daemon_exit (line $wait_line)"
+    || fail "swap_if_built (line $swap_line) is not after wait_for_daemon_exit (line $wait_line)"
   [ "$swap_line" -lt "$start_line" ] \
-    || fail "swap_staged_jar (line $swap_line) is not before the start section (line $start_line)"
+    || fail "swap_if_built (line $swap_line) is not before the start section (line $start_line)"
 }
 
 # fleetd #511: the drain-gate abort message (fired when a build has staged a jar but the operator
@@ -687,6 +741,8 @@ test_swap_staged_jar_dies_without_staged_file
 test_swap_staged_jar_dies_when_mv_fails
 test_should_swap_true_when_build_ran
 test_should_swap_false_when_build_skipped
+test_swap_if_built_performs_the_swap_when_build_ran
+test_swap_if_built_skips_the_swap_when_build_skipped
 test_require_no_build_jar_dies_when_absent
 test_require_no_build_jar_accepts_present_jar
 test_wait_for_daemon_exit_returns_true_once_pid_clears