From 42820fbe75e717ca2e3ecac04c6ceec9237bdbd9 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 19 Sep 2026 15:18:32 +0700 Subject: [PATCH 1/2] fleetd #593 (pid-count half): running_pid() no longer matches the caller running_pid() was 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 (a hand-typed investigation, an ssh-shaped `sh -c '...; ...'`, or a pipeline) rather than being the daemon. That self-match turns a working redeploy into a reported "racing supervisor" failure via assert_single_daemon. pgrep -c does not exist on BSD/macOS, so this can't be fixed by switching flags. running_pid() now keeps pgrep to find candidates (portable), then drops any candidate whose process name (comm) names a shell -- the daemon is always `java`, so a self-matching wrapper of this shape is always excluded while a genuine second daemon-shaped process still counts. assert_single_daemon's die message no longer hands the operator a bare `pgrep -f "$PATTERN"` as remediation -- that was exactly the self-matching invocation -- and now says in words that a pattern can match the caller. Adds three tests: a self-matching wrapper shell must be excluded, a real second daemon-shaped process must still be found, and the die message must not recommend the self-matching command. Verified the first test fails against the pre-fix implementation (confirmed the regression is caught). Leaves instance 1 (the fleetd.out log source, systemd-only) for a Linux host, per the ticket's scope split. --- scripts/redeploy-fleetd.sh | 39 ++++++++++++++++++-- scripts/test-redeploy-fleetd.sh | 64 +++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh index 5f94df8..4be9e6a 100755 --- a/scripts/redeploy-fleetd.sh +++ b/scripts/redeploy-fleetd.sh @@ -169,7 +169,37 @@ 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. The daemon is always started as `java -jar target/fleetd.jar`, so its process name +# (comm) is always `java`; a self-matching wrapper of this shape is always a shell. So: keep pgrep +# to FIND candidates (that part is already portable), then drop any candidate whose `comm` names a +# shell — the wrapper is excluded, a genuine second daemon-shaped process (java, or in a test, +# anything that is not itself a shell) still counts. `comm` can carry a login shell's leading '-' +# (e.g. "-bash") or, on some `ps` builds, a full path — both are stripped before the comparison. +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#-}" + case "$comm" in + sh|bash|zsh|dash|ksh) continue ;; + esac + 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 +537,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 } diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh index 439e831..d4dd9d6 100755 --- a/scripts/test-redeploy-fleetd.sh +++ b/scripts/test-redeploy-fleetd.sh @@ -433,6 +433,67 @@ 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 ""` 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 instance 2, the other half — the fix above must not exclude too much. A genuine +# daemon-shaped process (comm is not a shell) whose own argv holds the pattern must still be +# found. `exec -a` gives a harmless `sleep` an argv[0] containing the pattern without starting a +# real JVM: copying a system binary into a scratch path and executing it from there was tried +# first and the OS killed it outright (SIGKILL, exit 137 — almost certainly a code-signing check), +# so this uses `exec -a` instead, which needs no binary of its own and nothing under a scratch +# directory. +test_running_pid_still_finds_a_real_second_process() { + local before after standin_pid + before="$(running_pid)" + ( exec -a "fleetd-593-test-standin-target/fleetd.jar" 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 daemon-shaped process (pid $standin_pid) whose own argv holds the pattern: before=[$before] after=[$after]" +} + +# 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 +1955,9 @@ 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_still_finds_a_real_second_process +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 -- 2.52.0 From 4b9ebda1b3471f7bd8b7805fee1288b0542e1b3f Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 19 Sep 2026 15:31:15 +0700 Subject: [PATCH 2/2] fleetd #593 CORRECTION 1: allowlist comm=java, not a denylist of shells The round-1 fix excluded known shell names (sh/bash/zsh/dash/ksh) from running_pid()'s pgrep candidates. Two holes remained, both the same false-positive shape the ticket exists to remove: 1. A pid pgrep lists can exit before the following `ps -o comm=` lookup runs. On a gone pid, ps prints nothing, comm is empty, and an empty string matches no denied shell name -- so a dead pid was still counted. 2. The denylist only knows the shells someone thought to name. ssh, perl, python3, ruby, tail -- anything else carrying the pattern in its own argv -- was still counted alongside the real daemon. The ticket names ssh as a live route. Both close with one change: allowlist comm=java instead of denying shells. The daemon is always `java -jar target/fleetd.jar`, so its comm is always `java`; an empty comm (hole 1) is not `java` either, closing that hole for free. Answers the objection in the code comment: an allowlist can under-count if fleetd ever stops being launched by `java` (a native image, a renamed launcher). That's a false negative, the worse direction for a guard -- but it is not a new assumption: PATTERN='target/fleetd.jar' already assumes a jar run by java, and that pattern breaks before this allowlist would. Replaces the round-1 "real second process" test (which gave its exec -a standin an argv[0] holding the pattern, but not comm=java) with one that forces comm=java via `exec -a java sh -c '...'`. Adds two stubbed pgrep/ps tests pinning the two holes directly (a non-java, non-shell comm such as perl; an empty comm from an already-exited pid) -- deterministic on every platform, unlike a live-process fixture, and immune to the BSD vs Linux difference in how `comm` is derived from a fabricated process. Adds a stubbed positive backstop (comm=java is counted). Confirmed the regression is caught: reverted to the round-1 denylist, reran the suite, watched the new non-shell-comm test fail at `set -e`'s first failure, then isolated the exited-pid test separately and confirmed it also fails against the same broken code. Restored the fix and reran green. Branch merged with origin/main (3 commits: hunter role + CLAUDE.md addendum) before this commit; unrelated, no conflicts. --- scripts/redeploy-fleetd.sh | 36 ++++++++++---- scripts/test-redeploy-fleetd.sh | 88 ++++++++++++++++++++++++++++----- 2 files changed, 104 insertions(+), 20 deletions(-) diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh index 4be9e6a..e505979 100755 --- a/scripts/redeploy-fleetd.sh +++ b/scripts/redeploy-fleetd.sh @@ -181,21 +181,39 @@ jar_id() { local f="${1:-$JAR}"; [ -f "$f" ] && hash256 "$f" || echo "absent"; } # 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. The daemon is always started as `java -jar target/fleetd.jar`, so its process name -# (comm) is always `java`; a self-matching wrapper of this shape is always a shell. So: keep pgrep -# to FIND candidates (that part is already portable), then drop any candidate whose `comm` names a -# shell — the wrapper is excluded, a genuine second daemon-shaped process (java, or in a test, -# anything that is not itself a shell) still counts. `comm` can carry a login shell's leading '-' -# (e.g. "-bash") or, on some `ps` builds, a full path — both are stripped before the comparison. +# 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#-}" - case "$comm" in - sh|bash|zsh|dash|ksh) continue ;; - esac + # 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" diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh index d4dd9d6..f2267ce 100755 --- a/scripts/test-redeploy-fleetd.sh +++ b/scripts/test-redeploy-fleetd.sh @@ -457,24 +457,87 @@ test_running_pid_excludes_self_matching_wrapper_shell() { || 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 instance 2, the other half — the fix above must not exclude too much. A genuine -# daemon-shaped process (comm is not a shell) whose own argv holds the pattern must still be -# found. `exec -a` gives a harmless `sleep` an argv[0] containing the pattern without starting a -# real JVM: copying a system binary into a scratch path and executing it from there was tried -# first and the OS killed it outright (SIGKILL, exit 137 — almost certainly a code-signing check), -# so this uses `exec -a` instead, which needs no binary of its own and nothing under a scratch -# directory. -test_running_pid_still_finds_a_real_second_process() { +# 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 "fleetd-593-test-standin-target/fleetd.jar" sleep 20 ) & + ( 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 daemon-shaped process (pid $standin_pid) whose own argv holds the pattern: before=[$before] after=[$after]" + || 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 ` 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 @@ -1956,7 +2019,10 @@ 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_still_finds_a_real_second_process +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 -- 2.52.0