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

redeploy-fleetd.sh exits non-zero after a SUCCESSFUL restart if the post-restart mktemp fails, reporting a working deployment as a failure #552

New Issue
Closed
opened 2026-09-12 09:39:13 +02:00 by ltms · 0 comments
ltms commented 2026-09-12 09:39:13 +02:00
Owner
Copy Link
Copy Source

Follow-up to #545 (merged as PR #548). #545 removed the known trigger for this; it did not remove
the hazard, and the hazard is about where the line sits, not about mktemp.

Measured on main at 0b032f5.

The shape

scripts/redeploy-fleetd.sh:62     set -euo pipefail
...
scripts/redeploy-fleetd.sh:~1002  launchctl load -w "$LAUNCHD_PLIST"      <- daemon restarted here
scripts/redeploy-fleetd.sh:~1015  systemctl --user start ...              <- or here
...
scripts/redeploy-fleetd.sh:1089   FRESH_LOG="$(mktemp -t fleetd-fresh-log.XXXXXX)"
scripts/redeploy-fleetd.sh:1090   trap 'rm -f "$FRESH_LOG"' EXIT
scripts/redeploy-fleetd.sh:1091   tail -n "+$((RESTART_MARK + 1))" "$OUT" > "$FRESH_LOG" 2>/dev/null || true
scripts/redeploy-fleetd.sh:1092   classify_amqp_connection_errors "$FRESH_LOG"

Line 1089 is a plain assignment from a command substitution, with no guard. Under
set -euo pipefail a failure there aborts the script.

By the time control reaches it, the daemon has already been stopped, the jar swapped, and the new
daemon started. So the abort produces:

  • a non-zero exit from a redeploy that actually succeeded,
  • the post-restart log verification never runs — classify_amqp_connection_errors is skipped, so
    nobody looks for errors in the fresh log region,
  • the abort happens on line 1089, one line before the trap on 1090 is installed.

The exit code is the channel a caller trusts. A lead — or anything automating this — reads non-zero
as "the redeploy failed" and the likely next action is to run it again, restarting a daemon that
was already correctly restarted.

Compare line 859, BUILD_LOG="$(mktemp -t fleetd-build.XXXXXX)". Same unguarded shape, but it sits
before anything is touched, so an abort there is harmless and the non-zero exit is honest.
Position is the whole severity difference between the two sites. The fleet01 lead made exactly
this distinction when they measured their own (older, two-site) copy of the file.

Why #545 does not close it

#545 fixed the one failure cause that made this fire every time on Linux: a -t template with no
Xs. That was the common case and it is gone. Any other cause still lands here — a full or
unwritable TMPDIR, a TMPDIR pointing at a path that no longer exists, a permissions change —
and the consequence is unchanged. The defect is that an irreversible step has already happened and
a later, non-essential step can still abort the whole run.

Suggested fix

Make everything after the restart non-fatal to the exit code, and say so when it is skipped:

  1. Guard the mktemp at 1089 the way unload_launchd_if_loaded guards its own
    (redeploy-fleetd.sh:320) — but warn instead of die, because after a successful restart
    there is nothing left to protect by refusing.
  2. If the fresh-log region cannot be captured, print a clear line saying the post-restart log check
    could not run, and keep the overall exit status reporting the restart's own outcome. That is
    the third state again: "no errors found" and "could not look for errors" must not both be
    silence.
  3. Install the trap before the assignment it cleans up, not after.

Acceptance

  • A test that stubs mktemp to fail (the technique PR #548 already added for the probe path) and
    asserts the script reports the post-restart check as skipped rather than aborting.
  • A test that the exit status after that stubbed failure still reflects the restart outcome.
  • A source-text check that no command substitution between the restart call and the end of the
    script is an unguarded plain assignment — the shape, so the next one added is caught too.
  • Mutation proof for each new test: red with that test's own message, restore, byte-identical
    hash, green control, and the proof cell run against the un-mutated tree first to confirm it
    reports not-applied.

Credit

Raised by the fleet01 lead, from their own container measurement on their (91-commit older) copy:

:410 sits AFTER the restart, and its trap 'rm -f "$FRESH_LOG"' is on the line below it, so the
abort happens before the trap is installed and after the daemon has been swapped: a redeployed,
unverified daemon and no cleanup. Site 2's position is a different severity from site 1's, which
fails before anything is touched.

Their line numbers are from their revision; the ones above are mine, measured on 0b032f5.

Related: #545, #550 (the same script, the same "cannot tell reported as a fact" family), #493.

Follow-up to #545 (merged as PR #548). #545 removed the known trigger for this; it did not remove the hazard, and the hazard is about **where the line sits**, not about `mktemp`. Measured on `main` at `0b032f5`. ## The shape ``` scripts/redeploy-fleetd.sh:62 set -euo pipefail ... scripts/redeploy-fleetd.sh:~1002 launchctl load -w "$LAUNCHD_PLIST" <- daemon restarted here scripts/redeploy-fleetd.sh:~1015 systemctl --user start ... <- or here ... scripts/redeploy-fleetd.sh:1089 FRESH_LOG="$(mktemp -t fleetd-fresh-log.XXXXXX)" scripts/redeploy-fleetd.sh:1090 trap 'rm -f "$FRESH_LOG"' EXIT scripts/redeploy-fleetd.sh:1091 tail -n "+$((RESTART_MARK + 1))" "$OUT" > "$FRESH_LOG" 2>/dev/null || true scripts/redeploy-fleetd.sh:1092 classify_amqp_connection_errors "$FRESH_LOG" ``` Line 1089 is a plain assignment from a command substitution, with no guard. Under `set -euo pipefail` a failure there **aborts the script**. By the time control reaches it, the daemon has already been stopped, the jar swapped, and the new daemon started. So the abort produces: - a **non-zero exit** from a redeploy that actually succeeded, - the post-restart log verification never runs — `classify_amqp_connection_errors` is skipped, so nobody looks for errors in the fresh log region, - the abort happens on line 1089, one line **before** the `trap` on 1090 is installed. The exit code is the channel a caller trusts. A lead — or anything automating this — reads non-zero as "the redeploy failed" and the likely next action is to run it again, restarting a daemon that was already correctly restarted. Compare line 859, `BUILD_LOG="$(mktemp -t fleetd-build.XXXXXX)"`. Same unguarded shape, but it sits **before** anything is touched, so an abort there is harmless and the non-zero exit is honest. **Position is the whole severity difference between the two sites.** The fleet01 lead made exactly this distinction when they measured their own (older, two-site) copy of the file. ## Why #545 does not close it #545 fixed the one failure cause that made this fire every time on Linux: a `-t` template with no `X`s. That was the common case and it is gone. Any other cause still lands here — a full or unwritable `TMPDIR`, a `TMPDIR` pointing at a path that no longer exists, a permissions change — and the consequence is unchanged. The defect is that an irreversible step has already happened and a later, non-essential step can still abort the whole run. ## Suggested fix Make everything after the restart non-fatal to the exit code, and say so when it is skipped: 1. Guard the `mktemp` at 1089 the way `unload_launchd_if_loaded` guards its own (`redeploy-fleetd.sh:320`) — but `warn` instead of `die`, because after a successful restart there is nothing left to protect by refusing. 2. If the fresh-log region cannot be captured, print a clear line saying the post-restart log check **could not run**, and keep the overall exit status reporting the restart's own outcome. That is the third state again: "no errors found" and "could not look for errors" must not both be silence. 3. Install the `trap` before the assignment it cleans up, not after. ## Acceptance - A test that stubs `mktemp` to fail (the technique PR #548 already added for the probe path) and asserts the script reports the post-restart check as **skipped** rather than aborting. - A test that the exit status after that stubbed failure still reflects the restart outcome. - A source-text check that no command substitution between the restart call and the end of the script is an unguarded plain assignment — the **shape**, so the next one added is caught too. - Mutation proof for each new test: red with that test's own message, restore, byte-identical hash, green control, and the proof cell run against the un-mutated tree first to confirm it reports not-applied. ## Credit Raised by the fleet01 lead, from their own container measurement on their (91-commit older) copy: > :410 sits AFTER the restart, and its `trap 'rm -f "$FRESH_LOG"'` is on the line below it, so the > abort happens before the trap is installed and after the daemon has been swapped: a redeployed, > unverified daemon and no cleanup. Site 2's position is a different severity from site 1's, which > fails before anything is touched. Their line numbers are from their revision; the ones above are mine, measured on `0b032f5`. Related: #545, #550 (the same script, the same "cannot tell reported as a fact" family), #493.
ltms closed this issue 2026-09-12 10:58:10 +02:00
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#552
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?