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

CB-600: blockers before the launchd agent is ever installed — the redeploy script can report "ok" while the daemon crash-loops #91

New Issue
Closed
opened 2026-08-16 17:33:27 +02:00 by ltms · 1 comment
ltms commented 2026-08-16 17:33:27 +02:00
Owner
Copy Link
Copy Source

Found by the reviewer on PR #86 (CB-594), which is merged. None of this bites today, because the launchd agent is deliberately not installed. Every item here becomes live the moment someone runs launchctl load -w. Treat this ticket as the gate on that action.

1. The script and the plist can disagree about the log path — and the disagreement is silent

scripts/redeploy-bridged.sh computes its log path from where the script file sits:

REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"

deploy/dev.ltms.bridged.plist hard-codes an absolute StandardOutPath. Nothing checks that the two agree.

If the script is ever run from a different checkout than the one baked into the plist — a worktree, a renamed clone, a future move — then launchd starts the daemon correctly and logs to the real path, while the script's post-restart checks read a different, empty or stale file. The "fresh bridged listening line" check and the ERROR-count check both resolve cleanly (the trailing || true keeps set -euo pipefail happy) and the script prints its all-clear.

So the script reports "ok — no ERROR lines since restart" while the daemon may be crash-looping. That defeats the single thing this script exists to prove: that it started is not that it works. This is the highest-value item here.

Fix: when supervised, derive the log path from the loaded plist, or verify the computed path matches the plist's StandardOutPath and fail loudly if not. Do not paper over it with a warning — a warning in a script whose whole job is to be trusted is the same as no check.

2. A crash loop is unbounded, and the plist's comment understates it

Direct answer to the question the review was asked. ThrottleInterval: 10 paces restarts to at most one per 10 seconds; it does not cap how many times launchd retries. If bridged fails fast on every start — a bad config, for example auth.mode: token with the token unset, which throws at startup — launchd restarts it forever until a person runs launchctl unload or fixes the config.

The plist comment says this keeps it "not in a tight loop", which is true and incomplete: it prevents a tight loop, not an unbounded one. LaunchAgents have no "give up after N tries" primitive, so this is not something PR #86 did wrong. But nothing else in the change compensates for it, and the comment should say plainly what the operator is signing up for.

Consider whether a fail-fast config error should exit in a way that stops the loop rather than inviting an infinite one.

3. A failed load leaves the agent stopped and persistently disabled

launchctl unload -w sets the job's persisted Disabled flag to true; the paired load -w clears it. In redeploy-bridged.sh the supervised path unloads, then loads. If the unload succeeds and the load then fails for any reason, the script correctly dies — but it leaves the agent both stopped and persistently disabled.

That is worse than the state before the redeploy: a later reboot or login will not bring the daemon back, because the disabled flag survives. Someone has to notice and re-run load -w.

Fix: on a failed load, either retry, or clear the disabled flag before dying, or at minimum print the exact recovery command. The reviewer could not exercise this path live — it needs the agent installed, which was out of scope.

4. A misleading comment (no behaviour change)

The javadoc on requiredSecretEnvVars in Bridged.java says auth.tokenEnv is excluded because it is "already enforced loudly, by a startup throw, a few lines above this method's call site." That throw is roughly 370 lines below the reportRequiredSecrets call site in main(), and it only fires when auth.mode: token is set — not in the default loopback-trust mode. The exclusion is correct; only the comment is wrong. Fix the comment.

Noted, not a defect

Profile's canonical constructor defaults tokenEnv to "BRIDGED_WORKER_TOKEN" for any profile that never set it, including a non-subscription one. Such a profile is therefore reported MISSING BRIDGED_WORKER_TOKEN at every startup. That is arguably catching a real misconfiguration rather than a false positive, since nobody sets that literal name as a real secret. Worth knowing the mechanism, because a future profile using some other auth path could trip it for real. No action unless that happens.

Acceptance criteria

  1. The supervised path cannot report a clean restart while reading a log file the daemon is not writing to. A deliberate path mismatch makes the script fail, not warn.
  2. The plist documents honestly that a crash loop is unbounded, and says what stops it.
  3. A failed load after a successful unload does not leave the agent persistently disabled without telling the operator exactly how to recover.
  4. The requiredSecretEnvVars comment matches where the throw actually is and when it fires.
  5. The unsupervised path stays byte-for-byte equivalent in behaviour. It is the live restart path today and it is verified working — do not regress it while fixing the supervised one.
  6. mvn -f bridged/pom.xml clean install green, run unpiped.

Do not do this as part of the fix

Do not install or load the agent. That is the operator's decision. This ticket makes installing it safe; it does not install it.

Found by the reviewer on PR #86 (CB-594), which is merged. **None of this bites today**, because the launchd agent is deliberately not installed. Every item here becomes live the moment someone runs `launchctl load -w`. Treat this ticket as the gate on that action. ## 1. The script and the plist can disagree about the log path — and the disagreement is silent `scripts/redeploy-bridged.sh` computes its log path from where the script file sits: ```sh REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" ``` `deploy/dev.ltms.bridged.plist` hard-codes an absolute `StandardOutPath`. Nothing checks that the two agree. If the script is ever run from a different checkout than the one baked into the plist — a worktree, a renamed clone, a future move — then launchd starts the daemon correctly and logs to the real path, while the script's post-restart checks read a different, empty or stale file. The "fresh `bridged listening` line" check and the ERROR-count check both resolve cleanly (the trailing `|| true` keeps `set -euo pipefail` happy) and the script prints its all-clear. **So the script reports "ok — no ERROR lines since restart" while the daemon may be crash-looping.** That defeats the single thing this script exists to prove: that it started is not that it works. This is the highest-value item here. Fix: when supervised, derive the log path from the loaded plist, or verify the computed path matches the plist's `StandardOutPath` and fail loudly if not. Do not paper over it with a warning — a warning in a script whose whole job is to be trusted is the same as no check. ## 2. A crash loop is unbounded, and the plist's comment understates it Direct answer to the question the review was asked. `ThrottleInterval: 10` paces restarts to at most one per 10 seconds; it does not cap how many times launchd retries. If `bridged` fails fast on every start — a bad config, for example `auth.mode: token` with the token unset, which throws at startup — launchd restarts it forever until a person runs `launchctl unload` or fixes the config. The plist comment says this keeps it "not in a tight loop", which is true and incomplete: it prevents a *tight* loop, not an *unbounded* one. LaunchAgents have no "give up after N tries" primitive, so this is not something PR #86 did wrong. But nothing else in the change compensates for it, and the comment should say plainly what the operator is signing up for. Consider whether a fail-fast config error should exit in a way that stops the loop rather than inviting an infinite one. ## 3. A failed `load` leaves the agent stopped *and* persistently disabled `launchctl unload -w` sets the job's persisted `Disabled` flag to true; the paired `load -w` clears it. In `redeploy-bridged.sh` the supervised path unloads, then loads. If the unload succeeds and the load then fails for any reason, the script correctly dies — but it leaves the agent both stopped and persistently disabled. That is worse than the state before the redeploy: a later reboot or login will **not** bring the daemon back, because the disabled flag survives. Someone has to notice and re-run `load -w`. Fix: on a failed load, either retry, or clear the disabled flag before dying, or at minimum print the exact recovery command. The reviewer could not exercise this path live — it needs the agent installed, which was out of scope. ## 4. A misleading comment (no behaviour change) The javadoc on `requiredSecretEnvVars` in `Bridged.java` says `auth.tokenEnv` is excluded because it is "already enforced loudly, by a startup throw, a few lines above this method's call site." That throw is roughly 370 lines *below* the `reportRequiredSecrets` call site in `main()`, and it only fires when `auth.mode: token` is set — not in the default loopback-trust mode. The exclusion is correct; only the comment is wrong. Fix the comment. ## Noted, not a defect `Profile`'s canonical constructor defaults `tokenEnv` to `"BRIDGED_WORKER_TOKEN"` for any profile that never set it, including a non-subscription one. Such a profile is therefore reported `MISSING BRIDGED_WORKER_TOKEN` at every startup. That is arguably catching a real misconfiguration rather than a false positive, since nobody sets that literal name as a real secret. Worth knowing the mechanism, because a future profile using some other auth path could trip it for real. No action unless that happens. ## Acceptance criteria 1. The supervised path cannot report a clean restart while reading a log file the daemon is not writing to. A deliberate path mismatch makes the script fail, not warn. 2. The plist documents honestly that a crash loop is unbounded, and says what stops it. 3. A failed `load` after a successful `unload` does not leave the agent persistently disabled without telling the operator exactly how to recover. 4. The `requiredSecretEnvVars` comment matches where the throw actually is and when it fires. 5. The **unsupervised** path stays byte-for-byte equivalent in behaviour. It is the live restart path today and it is verified working — do not regress it while fixing the supervised one. 6. `mvn -f bridged/pom.xml clean install` green, run unpiped. ## Do not do this as part of the fix Do not install or load the agent. That is the operator's decision. This ticket makes installing it safe; it does not install it.
ltms added this to the 1.1 — single-host close-out milestone 2026-08-16 17:33:27 +02:00
ltms closed this issue 2026-08-16 18:17:13 +02:00
ltms commented 2026-08-16 18:17:44 +02:00
Author
Owner
Copy Link
Copy Source

Merged (PR #99). All four items addressed.

This one touched scripts/redeploy-bridged.sh, which is my only route to restart the live daemon and the one command the operator allow-listed, so I checked the risky parts myself rather than reading the report.

The unsupervised path still works. I ran scripts/redeploy-bridged.sh --check from the merged tree. Full state report, both WORKER_GITEA_TOKEN and AI_GATEWAY_TOKEN resolve, --check: nothing changed, exit 0. That is the behaviour I depend on and it is unchanged.

The log-path check fails, and does not merely warn. It only runs when the agent is loaded, and the agent is not installed on this host — installing it is the operator's call, not mine. So I tested the function directly and safely: I cut a copy of the script at the report state banner, confirmed no mutating command survived in the copy, sourced that copy and called check_log_path_matches_plist against fixtures.

case result
paths match ok, exit 0
paths differ FAIL naming both resolved paths, exit 1
plist missing FAIL, exit 1

The third case matters more than it looks: PlistBuddy exits nonzero on a missing file but still writes File Doesn't Exist, Will Create: ... to stdout, so a check testing only for an empty string would treat that text as a real path. The implementation checks the exit status. The worker found that in its own first draft and said so, which is the kind of reporting I want.

On the sourced-for-testing guard. Adding a test hook to production infrastructure deserves a second look. It sits above every mutating line, so sourcing stops before the build, the drain prompt, the stop and the start — I confirmed its position in the diff, and --check proves the executed path falls straight through it. The trade is worth it: this script had no test coverage at all, and the alternative was leaving its most important check unexercisable. Whoever edits this file next: keep that guard above the report state banner. Moving mutating code above it would make sourcing dangerous.

Item 2 — I accept the answer that the crash loop stays unbounded. LaunchAgents have no "give up after N attempts" primitive. Having bridged disable its own launchd job on a fail-fast config error would need the same manual launchctl load -w to recover, and adds a new way to be silently down. The plist now states plainly that ThrottleInterval paces restarts but does not cap them, and names the two things that actually stop the loop. Saying it honestly is the right outcome here.

Not done, on purpose: the agent is still not installed. This ticket makes installing it safe; it does not install it. That remains the operator's decision.

Trial merge onto main: Tests run: 829, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

Merged (PR #99). All four items addressed. This one touched `scripts/redeploy-bridged.sh`, which is my only route to restart the live daemon and the one command the operator allow-listed, so I checked the risky parts myself rather than reading the report. **The unsupervised path still works.** I ran `scripts/redeploy-bridged.sh --check` from the merged tree. Full state report, both `WORKER_GITEA_TOKEN` and `AI_GATEWAY_TOKEN` resolve, `--check: nothing changed`, exit 0. That is the behaviour I depend on and it is unchanged. **The log-path check fails, and does not merely warn.** It only runs when the agent is loaded, and the agent is not installed on this host — installing it is the operator's call, not mine. So I tested the function directly and safely: I cut a copy of the script at the `report state` banner, confirmed no mutating command survived in the copy, sourced that copy and called `check_log_path_matches_plist` against fixtures. | case | result | |---|---| | paths match | `ok`, exit 0 | | paths differ | `FAIL` naming both resolved paths, exit 1 | | plist missing | `FAIL`, exit 1 | The third case matters more than it looks: `PlistBuddy` exits nonzero on a missing file but still writes `File Doesn't Exist, Will Create: ...` to stdout, so a check testing only for an empty string would treat that text as a real path. The implementation checks the exit status. The worker found that in its own first draft and said so, which is the kind of reporting I want. **On the sourced-for-testing guard.** Adding a test hook to production infrastructure deserves a second look. It sits above every mutating line, so sourcing stops before the build, the drain prompt, the stop and the start — I confirmed its position in the diff, and `--check` proves the executed path falls straight through it. The trade is worth it: this script had no test coverage at all, and the alternative was leaving its most important check unexercisable. Whoever edits this file next: **keep that guard above the `report state` banner.** Moving mutating code above it would make sourcing dangerous. **Item 2 — I accept the answer that the crash loop stays unbounded.** LaunchAgents have no "give up after N attempts" primitive. Having `bridged` disable its own launchd job on a fail-fast config error would need the same manual `launchctl load -w` to recover, and adds a new way to be silently down. The plist now states plainly that `ThrottleInterval` paces restarts but does not cap them, and names the two things that actually stop the loop. Saying it honestly is the right outcome here. **Not done, on purpose:** the agent is still not installed. This ticket makes installing it safe; it does not install it. That remains the operator's decision. Trial merge onto `main`: `Tests run: 829, Failures: 0, Errors: 0, Skipped: 0`, BUILD SUCCESS, unpiped.
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 1.1 — single-host close-out
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#91
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?