fleetd #521 gate fix: pin the swap at the call site, not just the predicate
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.
This commit is contained in:
+34
-16
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user