scripts/redeploy-fleetd.sh

@@ -169,7 +169,55 @@ hash256() {
 # on PATH). "absent" must never be the answer for a file that exists — that conflation, on Linux,
 # was the whole defect this ticket fixes.
 jar_id() { local f="${1:-$JAR}"; [ -f "$f" ] && hash256 "$f" || echo "absent"; }
-running_pid() { pgrep -f "$PATTERN" || true; }
+
+# fleetd #593 — `pgrep -f "$PATTERN"` matches ANY process whose full command line CONTAINS the
+# pattern text, and that is not the same thing as "is the daemon". A shell that merely embeds the
+# pattern as literal text — a human typing this exact investigation by hand, an ssh-shaped
+# `sh -c '...; ...'`, a pipeline, or any other non-exec'ing shell that never replaced itself with
+# the pattern-holding command — still shows up in that match, and it is the INSTRUMENT, not the
+# daemon. Measured live on this Mac: `sh -c 'echo "target/fleetd.jar" >/dev/null; sleep 30' &`
+# leaves a real `sh` process alive (it forks for the `sleep`, it does not exec into it) whose own
+# `ps -o args` is `sh -c echo "target/fleetd.jar" >/dev/null; sleep 30` — `pgrep -f "$PATTERN"`
+# matches that line right alongside the real `java -jar target/fleetd.jar` process. `pgrep -c`
+# (an in-one-call count) does not exist on BSD/macOS at all, so this cannot be fixed by switching
+# pgrep flags — it has to filter what pgrep already found, after the fact, in a way that still
+# runs on BSD.
+#
+# fleetd #593 CORRECTION 1 — the first cut of this filter kept everything whose `comm` was NOT a
+# shell name (a denylist: sh/bash/zsh/dash/ksh). Two holes in that, both the same false-positive
+# shape the ticket exists to remove in the first place:
+#   1. a pid `pgrep` just listed can exit before the `ps -o comm=` lookup runs; on a gone pid `ps`
+#      prints nothing, `comm` ends up empty, and an empty string matches none of the denied shell
+#      names — so a pid that no longer exists was still counted.
+#   2. the denylist only knows the shells someone thought to name. `ssh`, `perl`, `python3`,
+#      `ruby`, `tail` — anything else that carries the pattern in its own argv — was still
+#      counted right along with the real daemon, and the ticket names `ssh` as a live route.
+# Both close with the same change: allowlist `comm = java` instead of denying shells. Measured on
+# the live daemon: `pid=30224 comm=java`. An empty comm (hole 1) is not `java` either, so it is
+# excluded for free — no separate "is this pid still alive" check needed.
+#
+# The objection, because it is real: an allowlist can UNDER-count. If fleetd ever stops being
+# launched as `java -jar ...` — a native image, a renamed launcher — `running_pid()` silently
+# returns nothing and `assert_single_daemon` stops noticing a second daemon at all. For a guard,
+# that false-negative direction is the worse one to be wrong in. This is not a new assumption,
+# though: `PATTERN='target/fleetd.jar'` two lines up already assumes the daemon is a jar, which
+# is only ever run by `java`. If that launch method changes, `PATTERN` stops matching anything
+# before this allowlist would ever get the chance to be wrong — the allowlist rides on the same
+# assumption that is already load-bearing, it does not add a new one. Whoever changes the launch
+# method needs to update both `PATTERN` and this allowlist together.
+running_pid() {
+  local pid comm out=''
+  for pid in $(pgrep -f "$PATTERN" 2>/dev/null || true); do
+    comm="$(ps -o comm= -p "$pid" 2>/dev/null || true)"
+    comm="${comm##*/}"
+    comm="${comm#-}"
+    # Allowlist, not a denylist of wrappers — see the CORRECTION 1 comment above. Anything that
+    # is not literally `java` is excluded, including an empty comm from a pid that already exited.
+    [ "$comm" = java ] || continue
+    out="$out$pid"$'\n'
+  done
+  printf '%s' "$out"
+}
 
 # fleetd #493 — three small, independently testable pieces of "never build into the path a
 # running process holds":
@@ -507,8 +555,11 @@ assert_single_daemon() {
     die "more than one fleetd process is running after this restart (pids: $(printf '%s' "$pids" | tr '\n' ' ')).
         This is the exact failure a racing supervisor produces: the OLD jar was revived by its
         supervisor while this script started a NEW copy. Two daemons on one herdr session kill
-        each other's members. Investigate with 'pgrep -f \"$PATTERN\"' and stop the wrong one by
-        hand — do not assume either pid is the one you want."
+        each other's members. Investigate with 'ps -eo pid,comm,args | grep -F \"$PATTERN\"' and
+        check the COMM column of each hit yourself before acting — a bare 'pgrep -f \"$PATTERN\"'
+        (fleetd #593) can match the very shell you type it into, not just the daemon, so it is not
+        safe remediation advice on its own. Stop the wrong one by hand — do not assume either pid
+        is the one you want."
   fi
 }
 

scripts/test-redeploy-fleetd.sh

@@ -433,6 +433,130 @@ test_assert_single_daemon_rejects_two_pids() {
   printf '%s' "$output" | grep -qF '4343' || fail "refusal message does not list the pids it found"
 }
 
+# fleetd #593 instance 2 — `running_pid()` used to be a bare `pgrep -f "$PATTERN"`, which matches
+# ANY process whose full command line contains the pattern TEXT, including a shell that merely
+# embeds it as literal text rather than being the daemon. Measured live on this Mac: `pgrep -c`
+# (a one-call count) does not exist on BSD at all, and `bash -c "<single command>"` execs in place
+# so no parent shell survives to hold the pattern — which is exactly why the defect did not
+# reproduce from a plain script and needs a wrapper shaped like this instead. A `sh -c '...; ...'`
+# with MORE THAN ONE statement does not get that exec-in-place treatment: the shell forks a child
+# for the second statement and stays alive itself, holding the whole `-c` string — pattern text
+# included — in its own `ps -o args`, for as long as it runs. That is the same shape an
+# `ssh host "…; …"` wrapper or a hand-typed pipeline leaves behind. Before the fix this test would
+# have found the wrapper's pid in running_pid()'s output; it must not.
+test_running_pid_excludes_self_matching_wrapper_shell() {
+  local before after wrapper_pid
+  before="$(running_pid)"
+  sh -c 'echo "target/fleetd.jar" >/dev/null; sleep 20' &
+  wrapper_pid=$!
+  sleep 0.3
+  after="$(running_pid)"
+  kill "$wrapper_pid" 2>/dev/null || true
+  wait "$wrapper_pid" 2>/dev/null || true
+  [ "$after" = "$before" ] \
+    || fail "running_pid() counted a self-matching wrapper shell (pid $wrapper_pid, holding the pattern as literal text in its own argv, not the daemon): before=[$before] after=[$after]"
+}
+
+# fleetd #593 CORRECTION 1 — the round-1 version of this test gave its standin an argv[0]
+# containing the pattern text (via `exec -a`) and left `comm` as whatever that override produced,
+# which was never `java`. That was fine for a denylist-of-shells filter, but the allowlist below
+# now requires `comm = java` specifically, so the standin here must actually carry that comm, not
+# just avoid being a shell. `exec -a java` overrides argv[0] to `java` while the process itself
+# stays a genuine, harmless `sh`; combining it with the same non-exec'ing multi-statement shape
+# the wrapper-shell test above uses keeps the pattern text in the process's own `ps -o args` for
+# as long as it runs. Measured live on this Mac (BSD/macOS: `ps -o comm=` here reflects argv[0]):
+# `comm=java`, `args` contains the pattern, `pgrep -f "$PATTERN"` finds it. Copying a real system
+# binary into a scratch path and executing it from there was tried first, for a more literal
+# stand-in daemon, and the OS killed it outright (SIGKILL, exit 137 — almost certainly a
+# code-signing check on a relocated binary); `exec -a` needs no binary of its own and nothing
+# under a scratch directory, and it is the technique CORRECTION 1 names as the right one.
+#
+# This is the one live-process test in this file whose result could differ on Linux: Linux sets
+# `comm` from the actually-executed binary's own path, not from `exec -a`'s argv[0] override (BSD
+# ties `comm` to argv[0], which is what makes this technique work here) — so on Linux this
+# specific fixture might report `comm=sh`, not `comm=java`, even though the REAL daemon (a literal
+# `java -jar target/fleetd.jar` process, never fabricated) is unaffected either way. I could not
+# verify this fixture's behavior on Linux, so test_running_pid_counts_a_pid_whose_comm_is_java
+# below backstops the same claim (the allowlist admits a pid whose comm is `java`) with a stubbed
+# `ps`, which is identical bash on every platform and carries no such platform question.
+test_running_pid_finds_a_real_java_named_second_process() {
+  local before after standin_pid
+  before="$(running_pid)"
+  ( exec -a java sh -c 'echo "target/fleetd.jar" >/dev/null; sleep 20' ) &
+  standin_pid=$!
+  sleep 0.3
+  after="$(running_pid)"
+  kill "$standin_pid" 2>/dev/null || true
+  wait "$standin_pid" 2>/dev/null || true
+  printf '%s\n' "$after" | grep -qxF "$standin_pid" \
+    || fail "running_pid() did not find a real second process (pid $standin_pid, comm forced to 'java' via exec -a) whose own argv holds the pattern: before=[$before] after=[$after]"
+}
+
+# fleetd #593 CORRECTION 1, hole 2 — the round-1 filter denied known shell names (sh/bash/zsh/
+# dash/ksh) and counted everything else. `ssh`, `perl`, `python3`, `ruby`, `tail` — anything not on
+# that list, carrying the pattern in its own argv — was still counted right alongside the real
+# daemon, and the ticket names `ssh` as a live route. Stubbing `pgrep`/`ps` (rather than spawning a
+# real perl/ssh process) pins the exact discriminator this correction is about — comm, not the
+# caller's shape — deterministically on every platform, with no dependency on perl/python3/ruby
+# being installed in whatever environment runs this suite, and no dependency on how a given OS
+# derives `comm` for a fabricated process (see the comment above
+# test_running_pid_finds_a_real_java_named_second_process for why that matters here).
+test_running_pid_drops_a_pid_whose_comm_is_not_java() {
+  pgrep() { printf '4242\n'; }
+  ps() { printf 'perl\n'; }
+  local found
+  found="$(running_pid)"
+  unset -f pgrep ps
+  [ -z "$found" ] \
+    || fail "running_pid() counted pid 4242 whose comm is 'perl', not 'java' — denying known shell names does not exclude a non-shell wrapper such as ssh or perl (fleetd #593 CORRECTION 1): found=[$found]"
+}
+
+# fleetd #593 CORRECTION 1, hole 1 — pgrep can list a pid that exits before the following
+# `ps -o comm=` lookup runs; on a gone pid `ps` prints nothing, so `comm` comes back empty. Under
+# the round-1 denylist an empty string matched none of the denied shell names, so the dead pid was
+# still counted — the exact false-positive shape the ticket exists to remove, just rarer. The
+# allowlist fixes this for free: an empty comm is not `java` either.
+test_running_pid_drops_a_pid_that_exited_before_the_comm_lookup() {
+  pgrep() { printf '4242\n'; }
+  ps() { :; }   # a pid that no longer exists: the real `ps -p <gone>` prints nothing and this mirrors that
+  local found
+  found="$(running_pid)"
+  unset -f pgrep ps
+  [ -z "$found" ] \
+    || fail "running_pid() counted pid 4242 whose comm lookup came back empty (the pid had already exited before the lookup ran) — an empty comm must not pass the allowlist (fleetd #593 CORRECTION 1): found=[$found]"
+}
+
+# The positive backstop for both stubbed tests above, and for
+# test_running_pid_finds_a_real_java_named_second_process on whatever platform that live fixture
+# does not itself carry comm=java: the allowlist must still ADMIT the one comm value the real
+# daemon actually has. Measured on the real, currently-running daemon on this Mac: `comm=java`.
+test_running_pid_counts_a_pid_whose_comm_is_java() {
+  pgrep() { printf '4242\n'; }
+  ps() { printf 'java\n'; }
+  local found
+  found="$(running_pid)"
+  unset -f pgrep ps
+  printf '%s\n' "$found" | grep -qxF '4242' \
+    || fail "running_pid() did not count pid 4242 whose comm is 'java' — the daemon's own name must pass the allowlist: found=[$found]"
+}
+
+# fleetd #593 instance 3 — assert_single_daemon's refusal message used to tell the operator to
+# "Investigate with 'pgrep -f \"\$PATTERN\"'", which — typed by hand or over ssh — is precisely the
+# self-matching invocation instance 2 above fixes. A source-text check, the same technique
+# test_no_error_lines_message_gated_by_drain_state uses: this is prose inside a die() call, never
+# reached by sourcing (the SOURCED guard stops before the main flow, and this text only prints
+# from inside a call assert_single_daemon makes when it is already refusing).
+test_die_message_does_not_recommend_bare_pgrep_as_remediation() {
+  local src="$ROOT/scripts/redeploy-fleetd.sh" block bad
+  block="$(grep -A6 -F 'racing supervisor produces' "$src" || true)"
+  [ -n "$block" ] || fail "could not find the assert_single_daemon refusal message in redeploy-fleetd.sh"
+  bad="$(printf '%s' "$block" | grep -F "Investigate with 'pgrep -f" || true)"
+  [ -z "$bad" ] \
+    || fail "assert_single_daemon's die message still hands the operator a bare 'pgrep -f \"\$PATTERN\"' as remediation (fleetd #593) — that is exactly the self-matching invocation"
+  printf '%s' "$block" | grep -qF 'fleetd #593' \
+    || fail "assert_single_daemon's die message does not say in words that a pattern can match the caller (fleetd #593)"
+}
+
 # fleetd #511 — jar_id()'s no-argument default was unpinned by any test: nothing proved it reports
 # $JAR (the live path) rather than $JAR_STAGED. Both halves matter, so this pins both: the bare call
 # must hash the live jar, and an explicit path argument must hash THAT file, not fall back to $JAR.
@@ -1894,6 +2018,12 @@ test_require_drivable_supervisor_accepts_known_kinds
 test_count_daemon_pids
 test_assert_single_daemon_accepts_one_pid
 test_assert_single_daemon_rejects_two_pids
+test_running_pid_excludes_self_matching_wrapper_shell
+test_running_pid_finds_a_real_java_named_second_process
+test_running_pid_drops_a_pid_whose_comm_is_not_java
+test_running_pid_drops_a_pid_that_exited_before_the_comm_lookup
+test_running_pid_counts_a_pid_whose_comm_is_java
+test_die_message_does_not_recommend_bare_pgrep_as_remediation
 test_jar_id_defaults_to_live_and_reports_explicit_path
 test_hash256_computes_a_real_sha256
 test_jar_id_reports_absent_for_missing_file