fleetd #492 follow-up: detect_supervisor must never read "could not tell" as "none" #499

Merged

ltms merged 3 commits from worker/492-followup-detect-unclear into main 2026-09-12 05:04:04 +02:00

Conversation 0 Commits 3 Files Changed 2

+568 -54

agent commented 2026-09-12 04:24:11 +02:00

Member

Copy Link

Copy Source

Follow-up to fleetd #492 / PR #495. Branch off origin/worker/492-209647-1 at dcd5052, not off main.

The defect

detect_supervisor() only read launchd_loaded/systemd_loaded. Anything short of a clean "yes" from either became none, and require_drivable_supervisor() accepted none without question — the raw kill + nohup fallback then races a real supervisor and produces two daemons on one herdr session (the exact failure fleetd #492 exists to prevent).

Two situations were landing in that unsafe none:

1. Installed but not currently loaded, on either supervisor. systemctl --user is-active answers "no" for activating, deactivating, failed, and while an auto-restart is pending — every one of those is a host that IS under systemd and about to act again. *_installed already knew the unit/plist existed; it was only ever consulted for a warning line, not by the decision.
2. The probe itself erroring (e.g. systemctl cannot reach the user bus over a non-lingering ssh session) — indistinguishable from a clean negative because both systemd_* probes threw stderr into /dev/null.

The fix

• detect_supervisor now returns a fifth answer, unclear, for both situations.
• systemd_loaded/systemd_installed capture systemctl's exit status and stderr separately via a temp file, and set their own *_ERRORED flag only when the call exits non-zero and wrote to stderr — a real tool failure, never a clean "not active"/"not installed" answer.
• none now means only: neither supervisor installed, neither loaded, neither probe errored.
• require_drivable_supervisor die()s on unclear exactly like it already does on ambiguous, naming the specific supervisor and reason via a new SUPERVISOR_UNCLEAR_DETAIL global.
• launchd_installed/launchd_loaded and the ambiguous/none logic, assert_single_daemon, count_daemon_pids, the systemd stop/start branches — all untouched, as scoped.

Tests added (4)

• test_detect_supervisor_systemd_installed_not_loaded_is_unclear
• test_detect_supervisor_launchd_installed_not_loaded_is_unclear
• test_detect_supervisor_systemd_probe_error_is_unclear — drives the REAL systemd_loaded body through a systemctl stub on PATH that exits non-zero and writes to stderr; asserts the result is unclear, not none.
• test_require_drivable_supervisor_refuses_unclear — asserts die() fires (subshell + exit status), and the refusal message names the systemd unit.

Mutation proof

Baseline (unmodified tree at dcd5052, fleetd/ symlinked in so the recovery-pattern test could resolve source paths): 19 test functions invoked, exit=0, 3 pre-existing FAIL: lines (the predecessor's own mutation-proof cells — expected).

Post-change: 23 test functions invoked, exit=0, same 3 pre-existing FAIL: lines, unchanged.

Three new guards, each mutated on the real file, proven applied with two greps (mutant text present + original text absent), suite run, then restored byte-identically with a green control run after each:

1. Removed the installed-but-not-loaded branches from detect_supervisor (the li=1 && ld=0 / si=1 && sd=0 elifs). Grep 1 confirmed the mutant (if [ "$ld" = 1 ] && [ "$sd" = 1 ] now directly follows the function's local line). Grep 2 confirmed the removed text ("is installed ($LAUNCHD_PLIST exists) but is not currently loaded") was gone. Suite: exit=1, FAIL: systemd installed-but-not-loaded must read as unclear, not none: expected unclear, got none. Restored; control exit=0.
2. Removed the probe-errored branch from detect_supervisor. Grep 1 confirmed the mutant (the ambiguous check is now the first if). Grep 2 confirmed the removed SYSTEMD_LOADED_ERRORED"... || ...SYSTEMD_INSTALLED_ERRORED text was gone. Suite: exit=1, FAIL: a systemd probe error must read as unclear, not none: expected unclear, got none. Restored; control exit=0.
3. Removed the unclear) case arm from require_drivable_supervisor (falls through to the generic *) catch-all, which still die()s but with a message that doesn't name the unit). Grep 1 confirmed the mutant (ambiguous) block now falls straight to *)). Grep 2 confirmed the specific wording ("actually drives this daemon") was gone. Suite: exit=1, FAIL: refusal message does not name the systemd unit it found installed-but-not-loaded — note the exit status alone does NOT catch this mutation (the catch-all still dies), only the message-content assertion does; that's why the test checks both. Restored; control exit=0.

Build

bash -n scripts/redeploy-fleetd.sh — clean.
bash scripts/test-redeploy-fleetd.sh — exit=0.
cd fleetd && mvn clean install — BUILD SUCCESS, Tests run: 1677, Failures: 0, Errors: 0, Skipped: 0.

Caveat for review

I could not directly measure real launchctl/systemctl output text on this host (macOS, no systemd; constraints also forbid running launchctl/systemctl against any real service). The systemd probe-error fix is based on documented systemctl behavior (a clean "inactive"/"failed" answer has no stderr; a bus-unreachable failure does) and verified via a systemctl stub on PATH, not a real systemd host. I deliberately did NOT apply the same stderr-based error-detection to launchd_loaded: launchctl list <label> is documented to write "Could not find service..." to stderr on the completely normal "not loaded" case too, so the same heuristic there would risk misreading a genuinely-unsupervised host as unclear. Only the installed-but-not-loaded fix (not the stderr-error fix) was applied symmetrically to launchd.

One more thing — same defect shape found elsewhere (NOT fixed, report only)

A probe whose failure to answer is recorded as a negative answer, found elsewhere in scripts/ and fleetd/src/main/java:

• scripts/rename-checkout.sh:308 — [ -n "$(git -C "$OLD_PATH" status --porcelain 2>/dev/null)" ] gates a pre-flight "working tree is clean" check before a destructive move. If git status itself errors, stderr is discarded and the empty output reads as "clean," so the move proceeds without ever having verified cleanliness.
• scripts/rename-checkout.sh:95 — launchd_loaded() { launchctl list "$LAUNCHD_LABEL" >/dev/null 2>&1; } treats any nonzero exit as "not loaded." A genuine launchctl failure is indistinguishable from "legitimately not installed," and this boolean picks the stop path (kill vs. safe unload -w) at line 324.
• fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java:130-136 — paneOwnsAnyOf catches HerdrException and returns false ("pane vanished, skip it"), merging a genuine herdr/transport error with "this pane doesn't own the pid." Per the class's own javadoc (lines 22-23), a pid matching no pane falls through to loopback-trust and resolves as primary — an error here can trigger a worker→primary escalation.
• fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java:53-56 — a clean "lsof found no peer" and "lsof failed to start/crashed" both return the identical sentinel -1. CallerResolver (fleetd #317, lines 240-247) relies on that sentinel to fall back to anonymous, so a tool crash and a clean negative are structurally the same signal at the source.
• fleetd/src/main/java/dev/ltms/fleet/mcp/LsofProcessCwdLookup.java:43-46 — same shape: a clean "no cwd line found" and an lsof exec failure both return null.
• fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:391-401 — remoteUrlsLeakUserInfo (a credential-leak detector) catches any RuntimeException from git remote get-url and returns false ("no leak found"), which could mask a genuine credential-bearing remote URL if the read happens to fail for an unrelated reason.

None of the above touched in this PR — scope was scripts/redeploy-fleetd.sh and scripts/test-redeploy-fleetd.sh only.

Follow-up to fleetd #492 / PR #495. Branch off `origin/worker/492-209647-1` at `dcd5052`, not off `main`. ## The defect `detect_supervisor()` only read `launchd_loaded`/`systemd_loaded`. Anything short of a clean "yes" from either became `none`, and `require_drivable_supervisor()` accepted `none` without question — the raw `kill` + `nohup` fallback then races a real supervisor and produces two daemons on one herdr session (the exact failure fleetd #492 exists to prevent). Two situations were landing in that unsafe `none`: 1. **Installed but not currently loaded**, on either supervisor. `systemctl --user is-active` answers "no" for `activating`, `deactivating`, `failed`, and while an auto-restart is pending — every one of those is a host that IS under systemd and about to act again. `*_installed` already knew the unit/plist existed; it was only ever consulted for a warning line, not by the decision. 2. **The probe itself erroring** (e.g. `systemctl` cannot reach the user bus over a non-lingering ssh session) — indistinguishable from a clean negative because both `systemd_*` probes threw stderr into `/dev/null`. ## The fix - `detect_supervisor` now returns a fifth answer, **`unclear`**, for both situations. - `systemd_loaded`/`systemd_installed` capture `systemctl`'s exit status and stderr separately via a temp file, and set their own `*_ERRORED` flag only when the call exits non-zero **and** wrote to stderr — a real tool failure, never a clean "not active"/"not installed" answer. - `none` now means only: neither supervisor installed, neither loaded, neither probe errored. - `require_drivable_supervisor` `die()`s on `unclear` exactly like it already does on `ambiguous`, naming the specific supervisor and reason via a new `SUPERVISOR_UNCLEAR_DETAIL` global. - `launchd_installed`/`launchd_loaded` and the ambiguous/none logic, `assert_single_daemon`, `count_daemon_pids`, the systemd stop/start branches — all untouched, as scoped. ## Tests added (4) - `test_detect_supervisor_systemd_installed_not_loaded_is_unclear` - `test_detect_supervisor_launchd_installed_not_loaded_is_unclear` - `test_detect_supervisor_systemd_probe_error_is_unclear` — drives the REAL `systemd_loaded` body through a `systemctl` stub on `PATH` that exits non-zero and writes to stderr; asserts the result is `unclear`, not `none`. - `test_require_drivable_supervisor_refuses_unclear` — asserts `die()` fires (subshell + exit status), and the refusal message names the systemd unit. ## Mutation proof Baseline (unmodified tree at `dcd5052`, `fleetd/` symlinked in so the recovery-pattern test could resolve source paths): 19 test functions invoked, `exit=0`, 3 pre-existing `FAIL:` lines (the predecessor's own mutation-proof cells — expected). Post-change: 23 test functions invoked, `exit=0`, same 3 pre-existing `FAIL:` lines, unchanged. Three new guards, each mutated on the real file, proven applied with two greps (mutant text present + original text absent), suite run, then restored byte-identically with a green control run after each: 1. **Removed the installed-but-not-loaded branches** from `detect_supervisor` (the `li=1 && ld=0` / `si=1 && sd=0` elifs). Grep 1 confirmed the mutant (`if [ "$ld" = 1 ] && [ "$sd" = 1 ]` now directly follows the function's `local` line). Grep 2 confirmed the removed text ("is installed ($LAUNCHD_PLIST exists) but is not currently loaded") was gone. Suite: `exit=1`, `FAIL: systemd installed-but-not-loaded must read as unclear, not none: expected unclear, got none`. Restored; control `exit=0`. 2. **Removed the probe-errored branch** from `detect_supervisor`. Grep 1 confirmed the mutant (the ambiguous check is now the first `if`). Grep 2 confirmed the removed `SYSTEMD_LOADED_ERRORED"... || ...SYSTEMD_INSTALLED_ERRORED` text was gone. Suite: `exit=1`, `FAIL: a systemd probe error must read as unclear, not none: expected unclear, got none`. Restored; control `exit=0`. 3. **Removed the `unclear)` case arm** from `require_drivable_supervisor` (falls through to the generic `*)` catch-all, which still `die()`s but with a message that doesn't name the unit). Grep 1 confirmed the mutant (`ambiguous)` block now falls straight to `*)`). Grep 2 confirmed the specific wording ("actually drives this daemon") was gone. Suite: `exit=1`, `FAIL: refusal message does not name the systemd unit it found installed-but-not-loaded` — note the exit status alone does NOT catch this mutation (the catch-all still dies), only the message-content assertion does; that's why the test checks both. Restored; control `exit=0`. ## Build `bash -n scripts/redeploy-fleetd.sh` — clean. `bash scripts/test-redeploy-fleetd.sh` — `exit=0`. `cd fleetd && mvn clean install` — `BUILD SUCCESS`, `Tests run: 1677, Failures: 0, Errors: 0, Skipped: 0`. ## Caveat for review I could not directly measure real `launchctl`/`systemctl` output text on this host (macOS, no systemd; constraints also forbid running `launchctl`/`systemctl` against any real service). The systemd probe-error fix is based on documented `systemctl` behavior (a clean "inactive"/"failed" answer has no stderr; a bus-unreachable failure does) and verified via a `systemctl` stub on `PATH`, not a real systemd host. I deliberately did NOT apply the same stderr-based error-detection to `launchd_loaded`: `launchctl list <label>` is documented to write "Could not find service..." to stderr on the completely normal "not loaded" case too, so the same heuristic there would risk misreading a genuinely-unsupervised host as `unclear`. Only the installed-but-not-loaded fix (not the stderr-error fix) was applied symmetrically to launchd. ## One more thing — same defect shape found elsewhere (NOT fixed, report only) A probe whose failure to answer is recorded as a negative answer, found elsewhere in `scripts/` and `fleetd/src/main/java`: - `scripts/rename-checkout.sh:308` — `[ -n "$(git -C "$OLD_PATH" status --porcelain 2>/dev/null)" ]` gates a pre-flight "working tree is clean" check before a destructive move. If `git status` itself errors, stderr is discarded and the empty output reads as "clean," so the move proceeds without ever having verified cleanliness. - `scripts/rename-checkout.sh:95` — `launchd_loaded() { launchctl list "$LAUNCHD_LABEL" >/dev/null 2>&1; }` treats any nonzero exit as "not loaded." A genuine `launchctl` failure is indistinguishable from "legitimately not installed," and this boolean picks the stop path (`kill` vs. safe `unload -w`) at line 324. - `fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java:130-136` — `paneOwnsAnyOf` catches `HerdrException` and returns `false` ("pane vanished, skip it"), merging a genuine herdr/transport error with "this pane doesn't own the pid." Per the class's own javadoc (lines 22-23), a pid matching no pane falls through to loopback-trust and resolves as primary — an error here can trigger a worker→primary escalation. - `fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java:53-56` — a clean "lsof found no peer" and "lsof failed to start/crashed" both return the identical sentinel `-1`. `CallerResolver` (fleetd #317, lines 240-247) relies on that sentinel to fall back to anonymous, so a tool crash and a clean negative are structurally the same signal at the source. - `fleetd/src/main/java/dev/ltms/fleet/mcp/LsofProcessCwdLookup.java:43-46` — same shape: a clean "no cwd line found" and an `lsof` exec failure both return `null`. - `fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:391-401` — `remoteUrlsLeakUserInfo` (a credential-leak detector) catches any `RuntimeException` from `git remote get-url` and returns `false` ("no leak found"), which could mask a genuine credential-bearing remote URL if the read happens to fail for an unrelated reason. None of the above touched in this PR — scope was `scripts/redeploy-fleetd.sh` and `scripts/test-redeploy-fleetd.sh` only.

agent added 2 commits 2026-09-12 04:24:11 +02:00

fleetd #492: teach redeploy-fleetd.sh systemd --user as a third supervisor ... dcd505286f

launchd, systemd --user, and unsupervised are three different answers, not two.
Refuse (die) rather than fall through to kill+nohup when a supervisor is
detected that this script cannot drive (e.g. both signals fire at once), and
add a post-restart check that fails the run if more than one fleetd process
is alive. detect_supervisor()/require_drivable_supervisor()/
count_daemon_pids()/assert_single_daemon() are pure, overridable functions so
scripts/test-redeploy-fleetd.sh can exercise them without a real launchd or
systemd.

fleetd #492 follow-up: detect_supervisor must never read "could not tell" as "none" ... b17f37a683

Two situations were silently landing in the "none" answer, which
require_drivable_supervisor accepts and the script then falls back to a raw
kill + nohup — exactly the wrong move when a supervisor actually IS present:

- installed-but-not-loaded, on either supervisor. `systemctl --user is-active`
  answers "no" for activating/deactivating/failed and while an auto-restart is
  pending too, and every one of those is a host that IS under systemd (or
  launchd) and about to act again. `*_installed` already knew this; it was
  only ever consulted for a warning line, never by the decision itself.
- a systemd probe that could not answer at all (e.g. systemctl cannot reach
  the user bus over a non-lingering ssh session) looked identical to a clean
  negative, because both probes redirected stderr straight to /dev/null.

detect_supervisor now returns a fifth answer, "unclear", for both cases.
systemd_loaded/systemd_installed capture systemctl's exit status and stderr
separately and set their own *_ERRORED flag only on a real tool failure
(non-zero exit WITH stderr), never on a clean negative. "none" now means only:
neither supervisor installed, neither loaded, neither probe errored.
require_drivable_supervisor die()s on "unclear" exactly like it already does
on "ambiguous", naming the specific supervisor and reason via the new
SUPERVISOR_UNCLEAR_DETAIL global.

Tests: 4 new cases (systemd/launchd installed-but-not-loaded, a real
systemd_loaded run through a systemctl stub that errors on stderr, and the
die() refusal for "unclear" naming the unit). All 3 new guards were verified
by mutation: each was removed from the real script, the suite caught it (a
new FAIL line naming the exact broken assertion), then the file was restored
byte-identically and the suite went green again.

agent added 1 commit 2026-09-12 04:58:54 +02:00

fleetd #492 follow-up: carry the unclear detail across detect_supervisor's subshell boundary ... 599419f9e6

b17f37a set SUPERVISOR_UNCLEAR_DETAIL as a global inside detect_supervisor, but the real call
site invokes it as $(detect_supervisor) — a subshell — so that global died with the subshell and
the die() message's ${VAR:-fallback} silently masked the loss with generic text.

- detect_supervisor now packs kind and detail onto its one stdout line (joined by the ASCII unit
  separator byte, $SUPERVISOR_DETAIL_SEP), the only channel that survives $( ). The real call site
  unpacks both with in-shell parameter expansion — no extra subshell.
- Dropped the ${SUPERVISOR_UNCLEAR_DETAIL:-...} fallback at the die() message: under set -u, a
  missing detail now fails loudly instead of silently defaulting (same defect class as #497).
- Added a constraints comment block above detect_supervisor for future callers: stdout-only,
  no ${VAR:-default} papering over a lost value, and every case on the return value needs an
  explicit *) arm.
- Added *) arms to the three `case "$SUPERVISOR_KIND"` switches (report/stop/start): report warns
  and continues (display-only), stop/start die naming the value (they act on it).
- Rewrote the "unclear" test to go through the real call-site shape ($(detect_supervisor) then
  the same split), not a hand-constructed value, and tightened its final assertion to check for
  the actual detail text rather than $SYSTEMD_UNIT alone (the die() boilerplate names the unit
  either way, so that check could pass on a lost value).

ltms merged commit 4f28da62a3 into main 2026-09-12 05:04:04 +02:00

ltms referenced this pull request 2026-09-12 05:04:28 +02:00

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

ltms referenced this pull request 2026-09-12 05:14:07 +02:00

fleetd #492: systemd --user as a third supervisor in redeploy-fleetd.sh #495

ltms referenced this pull request 2026-09-12 05:14:34 +02:00

redeploy-fleetd.sh is launchd-only: on fleet01 it kills the daemon and leaves TWO running #492

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#499