fleetd #528: pin drain_gate_refusal's call site, not just the predicate #532

Merged

ltms merged 1 commits from worker/528-drain-gate-call-site-5de83d-7 into main 2026-09-12 07:41:57 +02:00

Conversation 0 Commits 1 Files Changed 2

+136 -4

agent commented 2026-09-12 07:39:53 +02:00

Member

Copy Link

Copy Source

Fixes item 1 of fleetd #528.

What was wrong

scripts/redeploy-fleetd.sh's main flow built its own die "$(drain_gate_refusal "$DO_BUILD" "$JAR_STAGED")" call. drain_gate_refusal itself is well tested (four cases), but nothing proved the main flow's abort actually consulted it. Replacing that whole line with a flat die "aborted — nothing changed" left the suite green — silently reinstating the exact defect #517 was filed to fix.

The fix

Same shape as #521/#526's should_swap/swap_if_built: the decision and the die() now live together in one function, refuse_drain_gate(do_build, staged_path), which the main flow calls unconditionally instead of building the die() call itself. drain_gate_refusal stays separate and separately tested for the message-selection logic — refuse_drain_gate is the only thing that ever dies. A comment above refuse_drain_gate states the remaining hole plainly (deleting its call from the main flow) and names the test that covers it.

Four new behavioural tests stub die() to record whether it was called and with what message, for each of the four cases — these fail if refuse_drain_gate stops consulting drain_gate_refusal, mangles what it passes it, or never calls die at all. A fifth test, test_refuse_drain_gate_call_site_present, is a source-text pin on the main flow's call site itself (same shape as test_swap_ordered_after_wait_and_before_start) — this is the one that actually kills the item-1 mutation, since sourcing stops before the main flow runs and the four behavioural tests never touch that call site.

One care point: my first draft of the "what this doesn't pin" comment literally quoted the call-site string (refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"), which would have collided with the source-text grep and made it match the comment instead of the real call site (head -1 grabs whichever line comes first). Reworded the comment to describe the gap without quoting the exact literal, and confirmed by grep that the literal now occurs exactly once in the file, at the real call site.

Acceptance — reported line by line

1. bash scripts/test-redeploy-fleetd.sh exits 0, counts match.

$ bash scripts/test-redeploy-fleetd.sh > out.txt 2>&1; echo "exit=$?"
exit=0
$ grep -c '^FAIL:' out.txt      # real FAIL lines only
0
$ grep -c 'FAIL:' out.txt       # naive count over-counts, as the ticket warned
3

Output content (the 3 "FAIL:" substrings are the suite's own internal self-tests of the log-classifier mutation detectors, not real failures):

Recovery mutation: FAIL: unrecovered AMQP errors: expected 0, got 1
Shared-counter mutation: FAIL: cross-attributed recovered: expected 0, got 2
Unattributable mutation: FAIL: cross-unattributable recovered: expected 0, got 2
PASS: redeploy log classifier

Test functions defined vs invoked:

$ grep -E '^test_[A-Za-z0-9_]*\(\) \{' scripts/test-redeploy-fleetd.sh | sed -E 's/\(\) \{//' | sort > defined.txt
$ grep -E '^test_[A-Za-z0-9_]+$' scripts/test-redeploy-fleetd.sh | sort > invoked.txt
$ diff defined.txt invoked.txt && echo "IDENTICAL SETS: $(wc -l < defined.txt) each"
IDENTICAL SETS: 49 each

2. bash -n passes under both bash versions.

$ /bin/bash --version | head -1
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin25)
$ env bash --version | head -1
GNU bash, version 5.3.9(1)-release (aarch64-apple-darwin25.1.0)

$ /bin/bash -n scripts/redeploy-fleetd.sh;      echo exit=$?      -> exit=0
$ env bash   -n scripts/redeploy-fleetd.sh;      echo exit=$?      -> exit=0
$ /bin/bash -n scripts/test-redeploy-fleetd.sh; echo exit=$?      -> exit=0
$ env bash   -n scripts/test-redeploy-fleetd.sh; echo exit=$?      -> exit=0

3. The item 1 mutation is killed.
Baseline hash: 0e5a99a22c9c65f72960d8f179ca5299307889e06bc42131f098a513e7b97bd6

Mutated scripts/redeploy-fleetd.sh:710 from

    refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"

to

    die "aborted — nothing changed"

Ran the suite:

$ bash scripts/test-redeploy-fleetd.sh; echo exit=$?
FAIL: could not find the main flow's refuse_drain_gate call site in redeploy-fleetd.sh
exit=1

Restored and confirmed byte-identical:

$ shasum -a 256 scripts/redeploy-fleetd.sh
0e5a99a22c9c65f72960d8f179ca5299307889e06bc42131f098a513e7b97bd6  scripts/redeploy-fleetd.sh

Green control run afterward: exit=0, 0 real FAIL: lines (same output as acceptance item 1 above).

4. Two different-string greps + a line re-read, proving the mutation was actually applied.

$ grep -cF 'die "aborted — nothing changed"' scripts/redeploy-fleetd.sh
2   # 1 real mutated call site (line 710) + 1 comment mentioning this exact mutation by name (line 525)
$ grep -cF 'refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"' scripts/redeploy-fleetd.sh
0   # original call-site literal now absent — confirms the mutation replaced it
$ grep -cF 'refuse_drain_gate() {' scripts/redeploy-fleetd.sh
1   # the function itself is untouched, only its call site was mutated
$ grep -n 'die "aborted — nothing changed"' scripts/redeploy-fleetd.sh
525:# flat `die "aborted — nothing changed"` left the whole suite at exit 0 with zero FAIL lines and
710:    die "aborted — nothing changed"

Two different search strings (the mutant text vs. the original call-site text), each nonzero/zero as expected — never the false 0-and-0 pair the ticket warned about.

5. Harness proof on my own invocation — a different mutation than item 1's.
Inverted should_swap's own body (scripts/redeploy-fleetd.sh:208) from [ "$do_build" = 1 ] to [ "$do_build" != 1 ]:

$ bash scripts/test-redeploy-fleetd.sh; echo exit=$?
FAIL: should_swap 1 (a build ran and staged a jar) must return true
exit=1

Restored, confirmed byte-identical again (same hash as above), and the suite went green again (exit=0, 0 real FAIL: lines).

6. No piped exit-code reporting. Every suite run above was redirected to a file with echo $? on its own line, never through | tail/| head.

Sweep (report-only, per the ticket — nothing outside my two files touched)

Searched every set -e + pipefail script in the repo for x="$(cmd | cmd)" followed by a now-dead emptiness check:

$ grep -rlE 'set -o pipefail|set -[a-zA-Z]*e[a-zA-Z]*u?[a-zA-Z]*o pipefail' --include='*.sh' .
scripts/rename-checkout.sh
scripts/test-probe-member-credentials.sh
scripts/redeploy-fleetd.sh
scripts/test-redeploy-fleetd.sh
scripts/fleetd-launchd-wrapper.sh

(deploy/herdr-inner.sh has no set -e at all; scripts/probe-member-credentials.sh has set -uo pipefail but deliberately no -e per its own comments — neither qualifies for this pattern, since without -e a failing pipeline can't abort the script early.)

Findings:

• scripts/rename-checkout.sh — every var="$(cmd | cmd)" assignment already carries its own || true/|| echo fallback (e.g. line 141, with a comment at line 139 noting this exact fix was already applied there).
• scripts/redeploy-fleetd.sh — same; the only pipe-into-assignment call sites (RESTART_MARK, CODE) already carry || echo 0 / || echo 000.
• scripts/test-redeploy-fleetd.sh — the pre-existing source-text greps (test_swap_ordered_after_wait_and_before_start, from #526) already carry || true; my new test_refuse_drain_gate_call_site_present grep also carries || true.
• scripts/test-probe-member-credentials.sh and scripts/fleetd-launchd-wrapper.sh — no pipe-into-assignment patterns at all.

No live instances of the dead-check pattern found outside what #526 already fixed. Nothing fixed in this PR beyond the two files in scope.

Untouched, per the ticket

The seven main-flow decisions with no test of any kind (the three case "$SUPERVISOR_KIND" dispatches, if [ "$CHECK_ONLY" = 1 ], the drain-gate's own entry condition and its if [ "$reply" != "yes" ], if [ -z "$HEALTH_BODY" ], and the ok/warn summary) are explicitly out of scope and untouched. My change touches the if [ "$reply" != "yes" ] block only insofar as replacing its body's die "$(...)" call with refuse_drain_gate ... — the if guard itself (one of the seven) is unchanged.

Files changed

• scripts/redeploy-fleetd.sh
• scripts/test-redeploy-fleetd.sh

Never ran scripts/redeploy-fleetd.sh itself, with any flag, against the live daemon — all verification was by sourcing the script (as test-redeploy-fleetd.sh already does) and by direct bash -n/grep/sed manipulation of a scratch copy.

Fixes item 1 of fleetd #528. ## What was wrong `scripts/redeploy-fleetd.sh`'s main flow built its own `die "$(drain_gate_refusal "$DO_BUILD" "$JAR_STAGED")"` call. `drain_gate_refusal` itself is well tested (four cases), but nothing proved the main flow's abort actually consulted it. Replacing that whole line with a flat `die "aborted — nothing changed"` left the suite green — silently reinstating the exact defect #517 was filed to fix. ## The fix Same shape as #521/#526's `should_swap`/`swap_if_built`: the decision and the `die()` now live together in one function, `refuse_drain_gate(do_build, staged_path)`, which the main flow calls unconditionally instead of building the `die()` call itself. `drain_gate_refusal` stays separate and separately tested for the message-selection logic — `refuse_drain_gate` is the only thing that ever dies. A comment above `refuse_drain_gate` states the remaining hole plainly (deleting its call from the main flow) and names the test that covers it. Four new behavioural tests stub `die()` to record whether it was called and with what message, for each of the four cases — these fail if `refuse_drain_gate` stops consulting `drain_gate_refusal`, mangles what it passes it, or never calls `die` at all. A fifth test, `test_refuse_drain_gate_call_site_present`, is a source-text pin on the main flow's call site itself (same shape as `test_swap_ordered_after_wait_and_before_start`) — this is the one that actually kills the item-1 mutation, since sourcing stops before the main flow runs and the four behavioural tests never touch that call site. One care point: my first draft of the "what this doesn't pin" comment literally quoted the call-site string (`refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"`), which would have collided with the source-text grep and made it match the comment instead of the real call site (`head -1` grabs whichever line comes first). Reworded the comment to describe the gap without quoting the exact literal, and confirmed by grep that the literal now occurs exactly once in the file, at the real call site. ## Acceptance — reported line by line **1. `bash scripts/test-redeploy-fleetd.sh` exits 0, counts match.** ``` $ bash scripts/test-redeploy-fleetd.sh > out.txt 2>&1; echo "exit=$?" exit=0 $ grep -c '^FAIL:' out.txt # real FAIL lines only 0 $ grep -c 'FAIL:' out.txt # naive count over-counts, as the ticket warned 3 ``` Output content (the 3 "FAIL:" substrings are the suite's own internal self-tests of the log-classifier mutation detectors, not real failures): ``` Recovery mutation: FAIL: unrecovered AMQP errors: expected 0, got 1 Shared-counter mutation: FAIL: cross-attributed recovered: expected 0, got 2 Unattributable mutation: FAIL: cross-unattributable recovered: expected 0, got 2 PASS: redeploy log classifier ``` Test functions defined vs invoked: ``` $ grep -E '^test_[A-Za-z0-9_]*\(\) \{' scripts/test-redeploy-fleetd.sh | sed -E 's/\(\) \{//' | sort > defined.txt $ grep -E '^test_[A-Za-z0-9_]+$' scripts/test-redeploy-fleetd.sh | sort > invoked.txt $ diff defined.txt invoked.txt && echo "IDENTICAL SETS: $(wc -l < defined.txt) each" IDENTICAL SETS: 49 each ``` **2. `bash -n` passes under both bash versions.** ``` $ /bin/bash --version | head -1 GNU bash, version 3.2.57(1)-release (arm64-apple-darwin25) $ env bash --version | head -1 GNU bash, version 5.3.9(1)-release (aarch64-apple-darwin25.1.0) $ /bin/bash -n scripts/redeploy-fleetd.sh; echo exit=$? -> exit=0 $ env bash -n scripts/redeploy-fleetd.sh; echo exit=$? -> exit=0 $ /bin/bash -n scripts/test-redeploy-fleetd.sh; echo exit=$? -> exit=0 $ env bash -n scripts/test-redeploy-fleetd.sh; echo exit=$? -> exit=0 ``` **3. The item 1 mutation is killed.** Baseline hash: `0e5a99a22c9c65f72960d8f179ca5299307889e06bc42131f098a513e7b97bd6` Mutated `scripts/redeploy-fleetd.sh:710` from ```bash refuse_drain_gate "$DO_BUILD" "$JAR_STAGED" ``` to ```bash die "aborted — nothing changed" ``` Ran the suite: ``` $ bash scripts/test-redeploy-fleetd.sh; echo exit=$? FAIL: could not find the main flow's refuse_drain_gate call site in redeploy-fleetd.sh exit=1 ``` Restored and confirmed byte-identical: ``` $ shasum -a 256 scripts/redeploy-fleetd.sh 0e5a99a22c9c65f72960d8f179ca5299307889e06bc42131f098a513e7b97bd6 scripts/redeploy-fleetd.sh ``` Green control run afterward: `exit=0`, 0 real `FAIL:` lines (same output as acceptance item 1 above). **4. Two different-string greps + a line re-read, proving the mutation was actually applied.** ``` $ grep -cF 'die "aborted — nothing changed"' scripts/redeploy-fleetd.sh 2 # 1 real mutated call site (line 710) + 1 comment mentioning this exact mutation by name (line 525) $ grep -cF 'refuse_drain_gate "$DO_BUILD" "$JAR_STAGED"' scripts/redeploy-fleetd.sh 0 # original call-site literal now absent — confirms the mutation replaced it $ grep -cF 'refuse_drain_gate() {' scripts/redeploy-fleetd.sh 1 # the function itself is untouched, only its call site was mutated $ grep -n 'die "aborted — nothing changed"' scripts/redeploy-fleetd.sh 525:# flat `die "aborted — nothing changed"` left the whole suite at exit 0 with zero FAIL lines and 710: die "aborted — nothing changed" ``` Two different search strings (the mutant text vs. the original call-site text), each nonzero/zero as expected — never the false 0-and-0 pair the ticket warned about. **5. Harness proof on my own invocation — a different mutation than item 1's.** Inverted `should_swap`'s own body (`scripts/redeploy-fleetd.sh:208`) from `[ "$do_build" = 1 ]` to `[ "$do_build" != 1 ]`: ``` $ bash scripts/test-redeploy-fleetd.sh; echo exit=$? FAIL: should_swap 1 (a build ran and staged a jar) must return true exit=1 ``` Restored, confirmed byte-identical again (same hash as above), and the suite went green again (`exit=0`, 0 real `FAIL:` lines). **6. No piped exit-code reporting.** Every suite run above was redirected to a file with `echo $?` on its own line, never through `| tail`/`| head`. ## Sweep (report-only, per the ticket — nothing outside my two files touched) Searched every `set -e` + `pipefail` script in the repo for `x="$(cmd | cmd)"` followed by a now-dead emptiness check: ``` $ grep -rlE 'set -o pipefail|set -[a-zA-Z]*e[a-zA-Z]*u?[a-zA-Z]*o pipefail' --include='*.sh' . scripts/rename-checkout.sh scripts/test-probe-member-credentials.sh scripts/redeploy-fleetd.sh scripts/test-redeploy-fleetd.sh scripts/fleetd-launchd-wrapper.sh ``` (`deploy/herdr-inner.sh` has no `set -e` at all; `scripts/probe-member-credentials.sh` has `set -uo pipefail` but deliberately **no** `-e` per its own comments — neither qualifies for this pattern, since without `-e` a failing pipeline can't abort the script early.) Findings: - `scripts/rename-checkout.sh` — every `var="$(cmd | cmd)"` assignment already carries its own `|| true`/`|| echo` fallback (e.g. line 141, with a comment at line 139 noting this exact fix was already applied there). - `scripts/redeploy-fleetd.sh` — same; the only pipe-into-assignment call sites (`RESTART_MARK`, `CODE`) already carry `|| echo 0` / `|| echo 000`. - `scripts/test-redeploy-fleetd.sh` — the pre-existing source-text greps (`test_swap_ordered_after_wait_and_before_start`, from #526) already carry `|| true`; my new `test_refuse_drain_gate_call_site_present` grep also carries `|| true`. - `scripts/test-probe-member-credentials.sh` and `scripts/fleetd-launchd-wrapper.sh` — no pipe-into-assignment patterns at all. **No live instances of the dead-check pattern found outside what #526 already fixed.** Nothing fixed in this PR beyond the two files in scope. ## Untouched, per the ticket The seven main-flow decisions with no test of any kind (the three `case "$SUPERVISOR_KIND"` dispatches, `if [ "$CHECK_ONLY" = 1 ]`, the drain-gate's own entry condition and its `if [ "$reply" != "yes" ]`, `if [ -z "$HEALTH_BODY" ]`, and the ok/warn summary) are explicitly out of scope and untouched. My change touches the `if [ "$reply" != "yes" ]` block only insofar as replacing its body's `die "$(...)"` call with `refuse_drain_gate ...` — the `if` guard itself (one of the seven) is unchanged. ## Files changed - `scripts/redeploy-fleetd.sh` - `scripts/test-redeploy-fleetd.sh` Never ran `scripts/redeploy-fleetd.sh` itself, with any flag, against the live daemon — all verification was by sourcing the script (as `test-redeploy-fleetd.sh` already does) and by direct `bash -n`/`grep`/`sed` manipulation of a scratch copy.

agent added 1 commit 2026-09-12 07:39:54 +02:00

fleetd #528: pin drain_gate_refusal's call site, not just the predicate ... 7c34e8f4f9

drain_gate_refusal composes the correct abort message and is well tested,
but the main flow built its own `die "$(drain_gate_refusal ...)"` call —
nothing proved that call site was ever consulted. Mutating it to a flat
`die "aborted -- nothing changed"` left the whole suite green, silently
reinstating the exact defect #517 was filed to fix.

Same shape as #521/#526's should_swap/swap_if_built: the decision and the
die() now live together in refuse_drain_gate, which the main flow calls
unconditionally. drain_gate_refusal stays separate and separately tested
for the message logic; four new behavioural tests stub die() to prove
refuse_drain_gate calls it correctly for all four cases, and a fifth
source-text test pins the main flow's call site itself (the only thing
that can catch deleting the call, since sourcing stops before the main
flow runs).

ltms merged commit 8335b12562 into main 2026-09-12 07:41:57 +02:00

ltms referenced this pull request 2026-09-12 07:42:26 +02:00

"Extract the decision so the suite can call it" pins the decision and never the wiring — drain_gate_refusal's call site can be bypassed with the suite green #528

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

blocked

Cannot start until something else lands. The body says what.

needs-live-proof

Merged and green, but never shown working on the running daemon. Not the same as done.

ready-to-delegate

Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.

silent-default

A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent kevin ltms

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#532