fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

redeploy-fleetd.sh: four places where a failed command is reported as a clean result #504

New Issue
Open
opened 2026-09-12 05:04:28 +02:00 by ltms · 2 comments
ltms commented 2026-09-12 05:04:28 +02:00
Owner
Copy Link
Copy Source

Reported by the worker on PR #499 while sweeping for #497's shape, after fixing the detect_supervisor instance. Reported, not fixed — deliberately kept out of that PR. I checked each one in the tree at main.

All four are the same family as #497: a command that can fail for two different reasons, where both reasons produce the same answer, and the answer is the reassuring one.

1. A failed stop is reported as a successful stop — the worst of the four

scripts/redeploy-fleetd.sh:598-611, the "loaded but not currently running" branches:

elif [ "$SUPERVISOR_KIND" = "launchd" ]; then
  ...
  launchctl unload -w "$LAUNCHD_PLIST" 2>/dev/null || true
  ok "launchd agent unloaded (was already not running)"
elif [ "$SUPERVISOR_KIND" = "systemd" ]; then
  ...
  systemctl --user stop "$SYSTEMD_UNIT" 2>/dev/null || true
  ok "systemd --user unit stopped (was already not running)"

|| true swallows every failure, 2>/dev/null throws away the reason, and the next line prints ok unconditionally. The script then proceeds to the start step believing nothing is loaded.

This is the exact failure fleetd #492 exists to prevent: a supervisor that is still loaded revives the old jar underneath the new one. The other two stop paths (:567, :574) already || die — these two do not, and they are the branches reached when the state is already odd.

The comment says stop on an already-stopped unit is "a harmless no-op". That is true of the success case and says nothing about the failure case.

Fix: keep tolerating the genuine already-stopped answer, and die on anything else. Capture stderr and the exit status separately, exactly as systemd_loaded now does after PR #499 — the pattern is already in this file.

2. running_pid cannot tell "no match" from "pgrep failed"

:119:

running_pid() { pgrep -f "$PATTERN" || true; }

pgrep exits 1 for no match and non-zero for a usage or permission error. Both become an empty string, which every caller reads as "the daemon is not running". assert_single_daemon and the post-restart verification both depend on this.

3. The three zsh -lc probes

:469, :481, :497 — token and broker checks of the form zsh -lc '...' 2>/dev/null. If zsh itself fails to run, or the -lc string has a syntax error, the result is identical to "the variable is genuinely unset", and the script takes the warn path. This is the check that exists specifically because nothing else reports a missing WORKER_GITEA_TOKEN, so a silent failure here removes the only instrument.

4. curl's 000 sentinel

:693:

CODE="$(curl ... 2>/dev/null || echo 000)"

A transport failure — DNS, connection refused, timeout — and a real HTTP status are told apart only by the sentinel 000. Every downstream reader must special-case it, and one that does not will treat a network failure as an HTTP status. Better than the others, because at least a distinct value exists; worth checking that every consumer honours it.

Priority

Item 1 is the one to do. It can cause the two-daemon failure this file was written to prevent, and the fix is small and has a worked example in the same file. Items 2-4 are real and lower-risk.

Fix shape — same as #497

Do not add a ${VAR:-default} anywhere in the fix. This script runs under set -euo pipefail (:50), so an unset variable already fails loudly; a default converts a lost value into a confident wrong one. That exact mistake was in the first draft of the #497 fix and is written up on that ticket.

Related: #492, PR #499, #497, #493.

Reported by the worker on PR #499 while sweeping for #497's shape, after fixing the `detect_supervisor` instance. **Reported, not fixed** — deliberately kept out of that PR. I checked each one in the tree at `main`. All four are the same family as #497: a command that can fail for two different reasons, where both reasons produce the same answer, and the answer is the reassuring one. ### 1. A failed stop is reported as a successful stop — the worst of the four `scripts/redeploy-fleetd.sh:598-611`, the "loaded but not currently running" branches: ```bash elif [ "$SUPERVISOR_KIND" = "launchd" ]; then ... launchctl unload -w "$LAUNCHD_PLIST" 2>/dev/null || true ok "launchd agent unloaded (was already not running)" elif [ "$SUPERVISOR_KIND" = "systemd" ]; then ... systemctl --user stop "$SYSTEMD_UNIT" 2>/dev/null || true ok "systemd --user unit stopped (was already not running)" ``` `|| true` swallows every failure, `2>/dev/null` throws away the reason, and the next line prints `ok` unconditionally. The script then proceeds to the start step believing nothing is loaded. This is the exact failure fleetd #492 exists to prevent: a supervisor that is still loaded revives the old jar underneath the new one. The other two stop paths (`:567`, `:574`) already `|| die` — these two do not, and they are the branches reached when the state is already odd. The comment says `stop` on an already-stopped unit is "a harmless no-op". That is true of the success case and says nothing about the failure case. **Fix:** keep tolerating the genuine already-stopped answer, and die on anything else. Capture stderr and the exit status separately, exactly as `systemd_loaded` now does after PR #499 — the pattern is already in this file. ### 2. `running_pid` cannot tell "no match" from "pgrep failed" `:119`: ```bash running_pid() { pgrep -f "$PATTERN" || true; } ``` `pgrep` exits 1 for no match and non-zero for a usage or permission error. Both become an empty string, which every caller reads as "the daemon is not running". `assert_single_daemon` and the post-restart verification both depend on this. ### 3. The three `zsh -lc` probes `:469`, `:481`, `:497` — token and broker checks of the form `zsh -lc '...' 2>/dev/null`. If `zsh` itself fails to run, or the `-lc` string has a syntax error, the result is identical to "the variable is genuinely unset", and the script takes the `warn` path. This is the check that exists specifically because nothing else reports a missing `WORKER_GITEA_TOKEN`, so a silent failure here removes the only instrument. ### 4. `curl`'s `000` sentinel `:693`: ```bash CODE="$(curl ... 2>/dev/null || echo 000)" ``` A transport failure — DNS, connection refused, timeout — and a real HTTP status are told apart only by the sentinel `000`. Every downstream reader must special-case it, and one that does not will treat a network failure as an HTTP status. Better than the others, because at least a distinct value exists; worth checking that every consumer honours it. ## Priority Item 1 is the one to do. It can cause the two-daemon failure this file was written to prevent, and the fix is small and has a worked example in the same file. Items 2-4 are real and lower-risk. ## Fix shape — same as #497 Do not add a `${VAR:-default}` anywhere in the fix. This script runs under `set -euo pipefail` (`:50`), so an unset variable already fails loudly; a default converts a lost value into a confident wrong one. That exact mistake was in the first draft of the #497 fix and is written up on that ticket. Related: #492, PR #499, #497, #493.
ltms commented 2026-09-12 08:30:09 +02:00
Author
Owner
Copy Link
Copy Source

Every line number in this ticket is stale — re-measured on main at f1640f5

#534 inserted report_shutdown_drain and its helpers, so the whole file moved down. The file is now 1055 lines. Do not trust the citations in the ticket body; these are the current ones, measured just now:

what ticket said actually at
set -euo pipefail :50 :62
item 2, running_pid() :119 :138
item 1, launchctl unload … || true :598-611 :886, with its ok at :887
item 1, systemctl --user stop … || true (same range) :893, with its ok at :894
item 1's worked example, the || die pair :567, :574 :853-854 and :860-861
item 3, the three zsh -lc probes :469, :481, :497 :741, :753, :769
item 4, curl's 000 :693 :985

The set -euo pipefail number was wrong twice over. The ticket said :50; it was :54 when I checked during #534; it is :62 now. That is the argument for naming a mechanism instead of citing a line, which is already written into the comment block at :317 of the script itself:

The mechanism is named rather than cited by line number on purpose: a line number in a comment goes stale on the next insert above it, and this one already had — it said line 50 while the set line was at 54.

The ticket then went and did the same thing. Re-measure before you edit, and cite the function name rather than the line in anything you write.

Nothing else about the ticket changes: the four items are still real, item 1 is still the priority, and the fix shape is unchanged. I re-confirmed items 1-4 all still exist at the new positions — this is a numbering correction, not a re-scope.

## Every line number in this ticket is stale — re-measured on main at `f1640f5` #534 inserted `report_shutdown_drain` and its helpers, so the whole file moved down. The file is now 1055 lines. Do not trust the citations in the ticket body; these are the current ones, measured just now: | what | ticket said | actually at | |---|---|---| | `set -euo pipefail` | `:50` | **`:62`** | | item 2, `running_pid()` | `:119` | **`:138`** | | item 1, `launchctl unload … \|\| true` | `:598-611` | **`:886`**, with its `ok` at `:887` | | item 1, `systemctl --user stop … \|\| true` | (same range) | **`:893`**, with its `ok` at `:894` | | item 1's worked example, the `\|\| die` pair | `:567`, `:574` | **`:853-854`** and **`:860-861`** | | item 3, the three `zsh -lc` probes | `:469`, `:481`, `:497` | **`:741`**, **`:753`**, **`:769`** | | item 4, `curl`'s `000` | `:693` | **`:985`** | The `set -euo pipefail` number was wrong twice over. The ticket said `:50`; it was `:54` when I checked during #534; it is `:62` now. That is the argument for naming a mechanism instead of citing a line, which is already written into the comment block at `:317` of the script itself: > The mechanism is named rather than cited by line number on purpose: a line number in a comment goes stale on the next insert above it, and this one already had — it said line 50 while the `set` line was at 54. The ticket then went and did the same thing. **Re-measure before you edit, and cite the function name rather than the line in anything you write.** Nothing else about the ticket changes: the four items are still real, item 1 is still the priority, and the fix shape is unchanged. I re-confirmed items 1-4 all still exist at the new positions — this is a numbering correction, not a re-scope.
ltms commented 2026-09-12 09:12:08 +02:00
Author
Owner
Copy Link
Copy Source

Item 1 is done. PR #541, merged after I verified it myself. Items 2, 3 and 4 stay open, so
this ticket stays open.

Item 1 — what I ran on the merged tree

Check Result
bash scripts/test-redeploy-fleetd.sh exit 0, anchored ^FAIL: count 0, ends PASS: redeploy log classifier
unanchored FAIL: count 3 — the same 3 on main before this PR, so they are the suite's own internal mutation-cell fixtures
bash -n, both scripts clean under /bin/bash 3.2.57(1)-release and env bash 5.3.9(1)-release
test functions defined vs invoked 65 / 65, comm -3 empty (60 / 60 on main)

Item 1 — six mutations, all killed, each with its own message

Mutation The suite said
launchd helper swallows everything unload_launchd_if_loaded must die when launchctl exits non-zero AND writes to stderr
launchd helper over-corrects: dies on any non-zero ...must tolerate a clean already-unloaded answer (non-zero exit, empty stderr)
systemd helper swallows everything stop_systemd_if_loaded must die when systemctl exits non-zero AND writes to stderr
systemd helper over-corrects ...must tolerate a clean already-stopped answer (non-zero exit, empty stderr)
launchd call site back to bare || true could not find the main flow's call to unload_launchd_if_loaded in redeploy-fleetd.sh
systemd call site back to bare || true could not find the main flow's call to stop_systemd_if_loaded in redeploy-fleetd.sh

The two call sites were mutated separately, so neither half is riding on the other.

Testing the over-correction as well as the original defect is the part that matters here. The whole
point of #504 item 1 is telling a real failure from a clean already-stopped answer. A fix that dies
on both would pass a defect-only test and break every normal redeploy.

Restore sha db0513587d1696245fd879e13f6f0a25b6c7a1b6a239232f79e6adc8324ae8d2 matched the worker's
independently. Green control after the last restore: exit 0, 0 anchored ^FAIL:, clean
git status.

Found while verifying this, filed as #545

The mktemp -t NAME form the two new helpers copy from the existing house style fails on Linux.
GNU mktemp needs XXXXXX in the template; BSD mktemp on macOS does not. Six sites in this
script, four of which predate PR #541.

Measured consequence on Linux: detect_supervisor returns unclear and the script refuses to run —
and the refusal message blames the systemd user bus, which was never contacted. Full measurements in
#545.

This does not change item 1's verdict. The new helpers copy the surrounding style exactly, which
is what this ticket asked for, and #541's new tests are what make the Linux failure visible at all.

Still open here

  • Item 2 — running_pid.
  • Item 3 — the three zsh -lc probes.
  • Item 4 — curl … || echo 000.

Line numbers moved again when #541 merged. Cite function names, not line numbers, in any brief for
the rest of this ticket.

**Item 1 is done.** PR #541, merged after I verified it myself. **Items 2, 3 and 4 stay open**, so this ticket stays open. ## Item 1 — what I ran on the merged tree | Check | Result | |---|---| | `bash scripts/test-redeploy-fleetd.sh` | **exit 0**, anchored `^FAIL:` count **0**, ends `PASS: redeploy log classifier` | | unanchored `FAIL:` count | 3 — the same 3 on `main` before this PR, so they are the suite's own internal mutation-cell fixtures | | `bash -n`, both scripts | clean under `/bin/bash` 3.2.57(1)-release **and** `env bash` 5.3.9(1)-release | | test functions defined vs invoked | **65 / 65**, `comm -3` empty (60 / 60 on `main`) | ## Item 1 — six mutations, all killed, each with its own message | Mutation | The suite said | |---|---| | launchd helper swallows everything | `unload_launchd_if_loaded must die when launchctl exits non-zero AND writes to stderr` | | launchd helper over-corrects: dies on any non-zero | `...must tolerate a clean already-unloaded answer (non-zero exit, empty stderr)` | | systemd helper swallows everything | `stop_systemd_if_loaded must die when systemctl exits non-zero AND writes to stderr` | | systemd helper over-corrects | `...must tolerate a clean already-stopped answer (non-zero exit, empty stderr)` | | launchd call site back to bare `\|\| true` | `could not find the main flow's call to unload_launchd_if_loaded in redeploy-fleetd.sh` | | systemd call site back to bare `\|\| true` | `could not find the main flow's call to stop_systemd_if_loaded in redeploy-fleetd.sh` | The two call sites were mutated **separately**, so neither half is riding on the other. Testing the over-correction as well as the original defect is the part that matters here. The whole point of #504 item 1 is telling a real failure from a clean already-stopped answer. A fix that dies on both would pass a defect-only test and break every normal redeploy. Restore sha `db0513587d1696245fd879e13f6f0a25b6c7a1b6a239232f79e6adc8324ae8d2` matched the worker's independently. Green control after the last restore: exit 0, 0 anchored `^FAIL:`, clean `git status`. ## Found while verifying this, filed as #545 The `mktemp -t NAME` form the two new helpers copy from the existing house style **fails on Linux**. GNU `mktemp` needs `XXXXXX` in the template; BSD `mktemp` on macOS does not. Six sites in this script, four of which predate PR #541. Measured consequence on Linux: `detect_supervisor` returns `unclear` and the script refuses to run — and the refusal message blames the systemd user bus, which was never contacted. Full measurements in #545. **This does not change item 1's verdict.** The new helpers copy the surrounding style exactly, which is what this ticket asked for, and #541's new tests are what make the Linux failure visible at all. ## Still open here - **Item 2** — `running_pid`. - **Item 3** — the three `zsh -lc` probes. - **Item 4** — `curl … || echo 000`. Line numbers moved again when #541 merged. Cite function names, not line numbers, in any brief for the rest of this ticket.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#504
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?