From d105da978d2f8f3db8dc79f9e9eadfef56c3f4b0 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 3 Oct 2026 21:28:59 +0200 Subject: [PATCH] fleetd #680: pin the JAR/BUILD_JAR split, check the plist's jar path, and widen the daemon-locator pattern Part 1: asserts JAR and BUILD_JAR as the script sources them (no test assigns them first), so reverting JAR to a path under target/ now fails the suite. Part 2: check_jar_path_matches_plist reads the installed launchd plist's ProgramArguments and refuses when its jar path does not resolve to $JAR, mirroring check_log_path_matches_plist. Wired into report_supervisor_state's launchd branch; unaffected when no plist is installed. Part 3 (added to the ticket after the brief, by comment): PATTERN narrowed to 'fleetd.jar' so running_pid()/assert_single_daemon see a daemon regardless of which build layout (run/ or target/) its jar sits under. The comm=java allowlist still excludes a self-matching shell. Brought .claude/skills/fleets-status/SKILL.md's pgrep pattern into agreement. Verified: bash scripts/test-redeploy-fleetd.sh exits 0. Mutation both directions for part 1 (JAR under target/ -> suite fails; JAR elsewhere -> suite passes), a positive control for part 2 (neutralizing the mismatch check makes the new test fail), and a positive control for part 3 (narrowing PATTERN back to run/fleetd.jar makes the new target/-dir test fail). mvn -o clean install: Tests run: 1929, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 172 surefire report files. --- .claude/skills/fleets-status/SKILL.md | 2 +- scripts/redeploy-fleetd.sh | 53 ++++++++++-- scripts/test-redeploy-fleetd.sh | 113 ++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 7 deletions(-) diff --git a/.claude/skills/fleets-status/SKILL.md b/.claude/skills/fleets-status/SKILL.md index 133a41e9..d2848163 100644 --- a/.claude/skills/fleets-status/SKILL.md +++ b/.claude/skills/fleets-status/SKILL.md @@ -59,7 +59,7 @@ as `matches HEAD`, `drift`, or `unknown`; do not turn an unclear timestamp into Report the process identifier (PID) and uptime too: ```bash -PIDS="$(pgrep -f 'run/fleetd.jar' || true)" +PIDS="$(pgrep -f 'fleetd.jar' || true)" if [ -z "$PIDS" ]; then printf '%s\n' 'fleetd: not running' else diff --git a/scripts/redeploy-fleetd.sh b/scripts/redeploy-fleetd.sh index 61f042b0..f5233bd7 100755 --- a/scripts/redeploy-fleetd.sh +++ b/scripts/redeploy-fleetd.sh @@ -81,11 +81,12 @@ MODULE="$REPO/fleetd" BUILD_JAR="$MODULE/target/fleetd.jar" JAR="$MODULE/run/fleetd.jar" OUT="$MODULE/fleetd.out" -# Matches BOTH the absolute form and the relative `java -jar run/fleetd.jar` a hand-start -# produces from inside fleetd/. Anchoring on the absolute path alone was a real bug: the daemon -# restarted correctly and the script still reported "no process appeared", because it launched with -# a relative path and then looked for an absolute one. -PATTERN='run/fleetd.jar' +# Matches a fleetd daemon's command line wherever its jar sits — absolute or relative, under +# run/, under target/, or anywhere else a build or a hand-start might point it. Detecting a +# daemon this script did not start, including one running from a jar outside $JAR's own +# directory, is this pattern's whole job; running_pid()'s `comm = java` allowlist below is what +# keeps that breadth from counting a shell that merely types the pattern as literal text. +PATTERN='fleetd.jar' HEALTH='http://127.0.0.1:8765/healthz' STOP_WAIT=30 # seconds to wait for a clean exit before reporting failure HEALTH_WAIT=60 # seconds to wait for /healthz to answer after start — fleetd #603: also the pid- @@ -231,7 +232,7 @@ report_jar_state() { # 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='run/fleetd.jar'` two lines up already assumes the daemon is a jar, which +# though: `PATTERN='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 @@ -627,6 +628,45 @@ check_log_path_matches_plist() { ok "log path check: script and plist agree ($resolved_out)" } +# Reads the launchd plist's ProgramArguments for the argument that follows "-jar", resolves it +# alongside $jar_path, and dies when the two differ. Call it only when the agent is loaded; it +# never touches launchd or the daemon itself. +check_jar_path_matches_plist() { + local jar_path="$1" plist_path="$2" + local plist_args plist_jar resolved_jar resolved_plist_jar + if ! plist_args="$(/usr/libexec/PlistBuddy -c 'Print :ProgramArguments' "$plist_path" 2>/dev/null)"; then + die "launchd agent is loaded but PlistBuddy could not read ProgramArguments from + $plist_path + — cannot verify which jar the supervised daemon launches. Fix the plist before + redeploying supervised." + fi + plist_jar="$(printf '%s\n' "$plist_args" | awk ' + { gsub(/^[ \t]+|[ \t]+$/, "") } + prev == "-jar" { print; exit } + { prev = $0 } + ')" + if [ -z "$plist_jar" ]; then + die "launchd agent is loaded but its ProgramArguments at + $plist_path + do not contain a '-jar ' pair — cannot verify which jar the supervised daemon + launches. Fix the plist before redeploying supervised." + fi + resolved_jar="$(cd "$(dirname "$jar_path")" 2>/dev/null && pwd -P)/$(basename "$jar_path")" || true + resolved_plist_jar="$(cd "$(dirname "$plist_jar")" 2>/dev/null && pwd -P)/$(basename "$plist_jar")" || true + if [ -z "$resolved_jar" ] || [ -z "$resolved_plist_jar" ] || [ "$resolved_jar" != "$resolved_plist_jar" ]; then + die "jar path mismatch — this script deploys to + $jar_path (resolved: ${resolved_jar:-}) + but the loaded plist's ProgramArguments names + $plist_jar (resolved: ${resolved_plist_jar:-}) + The swap renames the built jar into place, so the old path stops existing after a redeploy; + a launchd-initiated start from this plist (a reboot, or KeepAlive after a crash) would then + run java against a missing file. Reinstall the plist at + $plist_path + so its ProgramArguments names $jar_path before redeploying supervised." + fi + ok "jar path check: script and plist agree ($resolved_jar)" +} + # fleetd #552: the post-restart fresh-log capture, pulled out of the main flow so it is testable by # sourcing (the same reason systemd_installed/systemd_loaded above guard their OWN mktemp inline # instead of leaving it bare) even though its only caller sits below the SOURCED guard. By the time @@ -960,6 +1000,7 @@ report_supervisor_state() { SUPERVISED=1 ok "launchd agent loaded ($LAUNCHD_LABEL) — launchd supervises this daemon" check_log_path_matches_plist "$OUT" "$LAUNCHD_PLIST" + check_jar_path_matches_plist "$JAR" "$LAUNCHD_PLIST" ;; systemd) SUPERVISED=1 diff --git a/scripts/test-redeploy-fleetd.sh b/scripts/test-redeploy-fleetd.sh index 087ac825..87750ddd 100755 --- a/scripts/test-redeploy-fleetd.sh +++ b/scripts/test-redeploy-fleetd.sh @@ -492,6 +492,38 @@ test_running_pid_finds_a_real_java_named_second_process() { || 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]" } +# PATTERN matches a fleetd jar in either build layout, not only the run/ one: a process whose +# argv names a jar under target/ must be found too, the same way the run/ case above is. +test_running_pid_finds_a_real_java_named_process_from_target_dir() { + 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) naming a jar under target/: before=[$before] after=[$after]" +} + +# Broadening PATTERN to match both build layouts must not also broaden it into matching a +# non-exec'ing shell that merely holds the target/ text as a literal argument, the same +# self-matching shape test_running_pid_excludes_self_matching_wrapper_shell above excludes for +# the run/ text. +test_running_pid_excludes_self_matching_wrapper_shell_naming_target_dir() { + 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 'target/fleetd.jar' as literal text in its own argv, not the daemon): 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 @@ -702,6 +734,19 @@ test_report_jar_state_both_absent_is_not_a_mismatch() { fi } +# Reads JAR and BUILD_JAR exactly as the script sources them, with nothing here assigning +# either first. JAR must resolve outside $MODULE/target/, and JAR must differ from BUILD_JAR: +# the daemon's live path and Maven's own build output are never the same file. +test_jar_and_build_jar_are_sourced_outside_target_and_differ() { + source "$ROOT/scripts/redeploy-fleetd.sh" + case "$JAR" in + "$MODULE"/target/*) + fail "\$JAR must not live under \$MODULE/target/ — got $JAR" ;; + esac + [ "$JAR" != "$BUILD_JAR" ] \ + || fail "\$JAR and \$BUILD_JAR must not be the same path — got $JAR" +} + # fleetd #493/#664 — never build into the path a running process holds. swap_staged_jar is # exercised directly against real files on disk (not stubs), because the whole point is file # behavior (does the content move, does the source disappear, does a failure leave both sides @@ -1305,12 +1350,71 @@ test_run_drain_gate_declined_reply_refuses() { source "$ROOT/scripts/redeploy-fleetd.sh" } +# Writes a launchd-plist fixture naming jar_path as the ProgramArguments entry after "-jar", so +# check_jar_path_matches_plist has something real to read back. +write_launchd_plist_fixture() { + local path="$1" jar_path="$2" + cat > "$path" < + + + + Label + test.fixture + ProgramArguments + + /usr/bin/java + -jar + $jar_path + fleetd.yaml + + + +PLIST +} + +# Agreeing case: a plist whose ProgramArguments names the same jar, resolved, must proceed and +# say so, never die. +test_check_jar_path_matches_plist_agrees_ok() { + local dir jar plist output rc=0 + dir="$TMP/jar-path-agree"; mkdir -p "$dir/run" + jar="$dir/run/fleetd.jar" + plist="$dir/agree.plist" + write_launchd_plist_fixture "$plist" "$jar" + output="$(check_jar_path_matches_plist "$jar" "$plist" 2>&1)" || rc=$? + [ "$rc" -eq 0 ] \ + || fail "check_jar_path_matches_plist must succeed when the plist names the same jar: $output" + printf '%s' "$output" | grep -qF 'jar path check' \ + || fail "check_jar_path_matches_plist did not print the agreement line: $output" +} + +# The disagreeing case, and the positive control this check exists for: a plist naming a +# different jar must die, naming both paths. +test_check_jar_path_matches_plist_mismatch_dies() { + local dir jar other_jar plist output rc=0 + dir="$TMP/jar-path-mismatch"; mkdir -p "$dir/run" "$dir/target" + jar="$dir/run/fleetd.jar" + other_jar="$dir/target/fleetd.jar" + plist="$dir/mismatch.plist" + write_launchd_plist_fixture "$plist" "$other_jar" + output="$(check_jar_path_matches_plist "$jar" "$plist" 2>&1)" || rc=$? + [ "$rc" -ne 0 ] \ + || fail "check_jar_path_matches_plist must die when the plist names a different jar" + printf '%s' "$output" | grep -qF "$jar" \ + || fail "die message does not name this script's jar path: $output" + printf '%s' "$output" | grep -qF "$other_jar" \ + || fail "die message does not name the plist's jar path: $output" +} + # fleetd #555 item 4 — the report-state dispatch on $SUPERVISOR_KIND. Inverting this used to report # the wrong supervisor and, for the launchd arm specifically, skip check_log_path_matches_plist. CHECK_LOG_PATH_CALLED=0 +CHECK_JAR_PATH_CALLED=0 stub_check_log_path_recorder() { CHECK_LOG_PATH_CALLED=0 + CHECK_JAR_PATH_CALLED=0 check_log_path_matches_plist() { CHECK_LOG_PATH_CALLED=1; } + check_jar_path_matches_plist() { CHECK_JAR_PATH_CALLED=1; } } test_report_supervisor_state_launchd_sets_supervised_and_checks_log_path() { @@ -1321,6 +1425,8 @@ test_report_supervisor_state_launchd_sets_supervised_and_checks_log_path() { [ "$SUPERVISED" = 1 ] || fail "report_supervisor_state launchd must set SUPERVISED=1" [ "$CHECK_LOG_PATH_CALLED" = 1 ] \ || fail "report_supervisor_state launchd must call check_log_path_matches_plist" + [ "$CHECK_JAR_PATH_CALLED" = 1 ] \ + || fail "report_supervisor_state launchd must call check_jar_path_matches_plist" source "$ROOT/scripts/redeploy-fleetd.sh" } @@ -1332,6 +1438,8 @@ test_report_supervisor_state_systemd_sets_supervised_without_log_path_check() { [ "$SUPERVISED" = 1 ] || fail "report_supervisor_state systemd must set SUPERVISED=1" [ "$CHECK_LOG_PATH_CALLED" = 0 ] \ || fail "report_supervisor_state systemd must NOT call check_log_path_matches_plist" + [ "$CHECK_JAR_PATH_CALLED" = 0 ] \ + || fail "report_supervisor_state systemd must NOT call check_jar_path_matches_plist" source "$ROOT/scripts/redeploy-fleetd.sh" } @@ -2189,6 +2297,8 @@ 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_finds_a_real_java_named_process_from_target_dir +test_running_pid_excludes_self_matching_wrapper_shell_naming_target_dir 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 @@ -2200,6 +2310,7 @@ test_jar_id_reports_unhashable_when_no_hasher_on_path test_report_jar_state_agrees_when_hashes_match test_report_jar_state_warns_when_hashes_differ test_report_jar_state_both_absent_is_not_a_mismatch +test_jar_and_build_jar_are_sourced_outside_target_and_differ test_swap_staged_jar_moves_staged_onto_live test_swap_staged_jar_dies_without_staged_file test_swap_staged_jar_dies_when_mv_fails @@ -2241,6 +2352,8 @@ test_drain_confirmed_false_on_anything_else test_run_drain_gate_skips_prompt_when_not_required test_run_drain_gate_confirmed_reply_does_not_refuse test_run_drain_gate_declined_reply_refuses +test_check_jar_path_matches_plist_agrees_ok +test_check_jar_path_matches_plist_mismatch_dies test_report_supervisor_state_launchd_sets_supervised_and_checks_log_path test_report_supervisor_state_systemd_sets_supervised_without_log_path_check test_report_supervisor_state_none_leaves_supervised_zero