fleetd #593 (pid-count half): running_pid() no longer matches the caller #597

Merged

ltms merged 3 commits from worker/593-1a8025-5 into main 2026-09-19 10:37:50 +02:00

Conversation 0 Commits 3 Files Changed 2

+184 -3

agent commented 2026-09-19 10:19:15 +02:00

Member

Copy Link

Copy Source

Fixes the pid-count half of #593 only (instance 2 + instance 3). Instance 1 (the fleetd.out log source, systemd-only) is left for a Linux host per the ticket's own scope split.

Instance 2 -- the pid count can match 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. 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 across BSD and Linux), then drops any candidate whose process name (comm) names a shell (sh/bash/zsh/dash/ksh) -- the daemon is always java, so a self-matching wrapper of this shape is excluded while a genuine second daemon-shaped process still counts. assert_single_daemon (the >1 check) is unchanged and still dies on a real race.

Instance 3 -- the remediation text reproduced the defect. The die message told the operator to "Investigate with 'pgrep -f "$PATTERN"'" -- typed by hand or over ssh, exactly the self-matching invocation. It now points at ps -eo pid,comm,args | grep -F "$PATTERN" plus checking the COMM column, and says in words that a bare pgrep can match the caller.

Tests added (scripts/test-redeploy-fleetd.sh): a self-matching wrapper shell (sh -c 'echo "target/fleetd.jar" >/dev/null; sleep 20', a non-exec'ing shell that forks for its second statement and stays alive holding the pattern in its own argv) must be excluded from running_pid()'s output; a real second daemon-shaped process (simulated safely via exec -a on a harmless sleep, never a real daemon) must still be found; and the die message must not recommend the self-matching pgrep -f command. I confirmed the first test fails against the pre-fix implementation (reverted running_pid() locally, reran, watched it fail, then restored the fix) -- the regression is genuinely caught, not vacuous.

Verification run in the worktree:

• bash scripts/test-redeploy-fleetd.sh -> exit 0, PASS: redeploy log classifier (the interspersed mktemp/mutation-test FAIL lines are expected noise from existing mutation tests, present in the baseline run too, unrelated to this change).
• mvn clean install in fleetd/ -> Tests run: 1789, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS (this bash test script is not wired into the Maven build; it's run separately, matching .gitea/workflows/ci.yml).
• Manually verified both fixture shapes live against this Mac's real, currently-running fleetd daemon (pid present via pgrep) -- the wrapper shell was excluded and the real daemon still counted.

Caveat for review: everything above was verified on macOS/BSD only, per the ticket's own note that instance 2 is latent (not reproducible) on this Mac without a deliberately constructed wrapper. I could not verify on a real Linux/systemd host -- that's instance 1's territory anyway, which this PR does not touch. The shell-name exclusion list (sh/bash/zsh/dash/ksh) covers the common shells; an unusual shell not on that list would still be excluded from nothing (fail open, matching the pre-fix behavior for anything not on the list), not a new failure mode.

Never ran scripts/redeploy-fleetd.sh itself, and never touched the live daemon.

Fixes the pid-count half of #593 only (instance 2 + instance 3). Instance 1 (the fleetd.out log source, systemd-only) is left for a Linux host per the ticket's own scope split. **Instance 2 -- the pid count can match 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. `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 across BSD and Linux), then drops any candidate whose process name (`comm`) names a shell (sh/bash/zsh/dash/ksh) -- the daemon is always `java`, so a self-matching wrapper of this shape is excluded while a genuine second daemon-shaped process still counts. `assert_single_daemon` (the >1 check) is unchanged and still dies on a real race. **Instance 3 -- the remediation text reproduced the defect.** The die message told the operator to "Investigate with 'pgrep -f \"$PATTERN\"'" -- typed by hand or over ssh, exactly the self-matching invocation. It now points at `ps -eo pid,comm,args | grep -F "$PATTERN"` plus checking the COMM column, and says in words that a bare pgrep can match the caller. **Tests added** (scripts/test-redeploy-fleetd.sh): a self-matching wrapper shell (`sh -c 'echo "target/fleetd.jar" >/dev/null; sleep 20'`, a non-exec'ing shell that forks for its second statement and stays alive holding the pattern in its own argv) must be excluded from `running_pid()`'s output; a real second daemon-shaped process (simulated safely via `exec -a` on a harmless `sleep`, never a real daemon) must still be found; and the die message must not recommend the self-matching `pgrep -f` command. I confirmed the first test fails against the pre-fix implementation (reverted running_pid() locally, reran, watched it fail, then restored the fix) -- the regression is genuinely caught, not vacuous. **Verification run in the worktree:** - `bash scripts/test-redeploy-fleetd.sh` -> exit 0, `PASS: redeploy log classifier` (the interspersed `mktemp`/mutation-test FAIL lines are expected noise from existing mutation tests, present in the baseline run too, unrelated to this change). - `mvn clean install` in fleetd/ -> `Tests run: 1789, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS` (this bash test script is not wired into the Maven build; it's run separately, matching .gitea/workflows/ci.yml). - Manually verified both fixture shapes live against this Mac's real, currently-running fleetd daemon (pid present via pgrep) -- the wrapper shell was excluded and the real daemon still counted. **Caveat for review:** everything above was verified on macOS/BSD only, per the ticket's own note that instance 2 is latent (not reproducible) on this Mac without a deliberately constructed wrapper. I could not verify on a real Linux/systemd host -- that's instance 1's territory anyway, which this PR does not touch. The shell-name exclusion list (sh/bash/zsh/dash/ksh) covers the common shells; an unusual shell not on that list would still be excluded from nothing (fail open, matching the pre-fix behavior for anything not on the list), not a new failure mode. Never ran scripts/redeploy-fleetd.sh itself, and never touched the live daemon.

agent added 1 commit 2026-09-19 10:19:16 +02:00

fleetd #593 (pid-count half): running_pid() no longer matches the caller ... 42820fbe75

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.

ltms referenced this pull request 2026-09-19 10:24:05 +02:00

redeploy-fleetd.sh knows which supervisor it is talking to (#492) but not which one it is watching: the log source and the pid count are both still macOS-shaped, and on systemd they fail in opposite directions #593

agent added 2 commits 2026-09-19 10:31:22 +02:00

Merge origin/main into worker/593-1a8025-5 1e68d7ee39

fleetd #593 CORRECTION 1: allowlist comm=java, not a denylist of shells ... 4b9ebda1b3

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.

ltms merged commit 7084d99b89 into main 2026-09-19 10:37:50 +02:00

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