fix(install): stop agent kills matching a co-installed sibling agent

pkill -f matches the whole command line and "^" only anchors the start, so
"^/usr/local/bin/pulse-agent" also matches "/usr/local/bin/pulse-agent-prod".
On a host running a second agent whose binary name shares the prefix, every
install, every upgrade, and every restart of the generated Unraid wrapper
silently killed the other agent too. Confirmed on a live dual-agent Unraid
box: the old pattern matched both the dev agent and the production dogfood
agent, the bounded pattern matches only its own.

The wrapper is the worst of the three because restarting through it is the
documented runbook step, so the collateral kill repeats every time an
operator follows it.

Bound the far end of each binary-anchored pattern with ([[:space:]]|$), and
swap the bare pkill -9 -f "pulse-agent" for -x on the exact process name,
which keeps that site's deliberate path-agnostic intent while excluding the
sibling. The pkill -x sites were already safe and are unchanged.

Guarded by two tests: one pins that no binary-anchored pkill in the installer
is left unbounded, the other exercises POSIX ERE semantics against the two
command lines a dual-agent host presents, including a premise check that the
unbounded pattern really does match the sibling so the assertion cannot pass
vacuously.
This commit is contained in:
rcourtman
2026-08-06 14:07:32 +01:00
parent 7f0985dd39
commit dd72bd1490
4 changed files with 112 additions and 8 deletions
@@ -72,6 +72,18 @@ generates the operator-facing limit description consumed by server ingress
and every supported `pulse-agent` release target built from this source.
An agent's lifecycle operations are scoped to that agent. Installing,
upgrading, restarting, or uninstalling one agent must never terminate a
co-installed agent that happens to share a binary-name prefix, which is how a
host runs a development agent beside a production one. Process matching by
whole command line is prefix matching unless the pattern bounds its far end, so
every such pattern must be terminated (`pulse-agent` must not match
`pulse-agent-prod`); where the intent is genuinely path-agnostic, match the
process name exactly instead. This binds the generated NAS wrapper scripts as
well as the installer, and most sharply there, because restarting through the
wrapper is the documented runbook step and would otherwise repeat the
collateral kill on every use.
Mock mode is a clean room on the report-admission boundary. Mock mode already
suspends pull-based PVE/PBS/PMG collection by never building those clients, and
push-based agent reports are held to the same rule: `ApplyHostReport`,
@@ -369,6 +369,16 @@ upgrade, update, release, or artifact-selection behavior.
must preserve quoted argument values without evaluating service-file shell
content, and the rewritten rc.d service must use `--token-file` rather than
retaining a recovered raw token.
Every process-termination step the installer performs, and every one it
writes into a generated boot or watchdog wrapper, must match only the agent
that installer instance owns. `pkill -f` applies its pattern to the whole
command line and a leading `^` anchors only the start, so a pattern that
does not bound its far end also matches a co-installed agent whose binary
name extends the same prefix, and installs, upgrades, and wrapper restarts
then take that second agent down with no diagnostic. Binary-anchored
patterns must therefore terminate at a whitespace-or-end boundary, and a
deliberately path-agnostic sweep must match the process name exactly rather
than a bare command-line substring.
FreeBSD-family uninstall must stop the rc.d daemon(8) supervisor before
removing the binary, then remove service registration, rc.conf enablement,
boot wrappers, PID files, token/state, and residual processes before it can
+22 -8
View File
@@ -3645,8 +3645,12 @@ if [[ -x "${INSTALL_DIR}/${BINARY_NAME}" ]]; then
# Stop the existing agent service gracefully through the installer-owned helper.
stop_existing_agent_service || true
# Also kill any running process in case it was started manually
pkill -f "^${INSTALL_DIR}/${BINARY_NAME}" 2>/dev/null || true
# Also kill any running process in case it was started manually.
# The trailing boundary matters: pkill -f matches the whole command
# line and "^" only anchors the start, so an unbounded pattern also
# matches a co-installed agent whose binary name merely starts with
# this one (pulse-agent matching pulse-agent-prod).
pkill -f "^${INSTALL_DIR}/${BINARY_NAME}([[:space:]]|$)" 2>/dev/null || true
sleep 1
fi
elif command -v systemctl >/dev/null 2>&1 && systemctl is-enabled --quiet "${AGENT_NAME}" 2>/dev/null; then
@@ -3792,8 +3796,10 @@ if [[ -f /etc/unraid-version ]]; then
# Kill any existing pulse agents.
log_info "Stopping any existing pulse agents..."
# Use process name matching to avoid killing unrelated processes
pkill -f "^${RUNTIME_BINARY}" 2>/dev/null || true
# Use process name matching to avoid killing unrelated processes. The
# trailing boundary keeps a co-installed agent whose binary name starts
# with this one (pulse-agent vs pulse-agent-prod) out of the match.
pkill -f "^${RUNTIME_BINARY}([[:space:]]|$)" 2>/dev/null || true
sleep 2
# Create a wrapper script that will be called from /boot/config/go
@@ -3819,8 +3825,13 @@ trim_watchdog_log() {
fi
}
# Kill any existing pulse-agent processes
pkill -f "^${RUNTIME_BINARY}" 2>/dev/null || true
# Kill any existing pulse-agent processes.
# The trailing boundary is required: pkill -f matches the whole command line
# and "^" only anchors the start, so without it this also kills a co-installed
# agent whose binary name starts with this one (pulse-agent vs
# pulse-agent-prod), which on a host running both takes down the other agent
# every time this wrapper restarts.
pkill -f "^${RUNTIME_BINARY}([[:space:]]|\$)" 2>/dev/null || true
sleep 2
# Copy binary from persistent storage to RAM disk (needed after reboot)
@@ -3992,8 +4003,11 @@ if [[ "$TRUENAS" == true ]]; then
sleep 2
fi
fi
# Kill any remaining pulse-agent processes (may be running from different paths)
pkill -9 -f "pulse-agent" 2>/dev/null || true
# Kill any remaining pulse-agent processes (may be running from different
# paths). -x matches the process name exactly, which keeps the
# path-agnostic intent while excluding a co-installed agent whose name
# merely starts with this one (pulse-agent-prod).
pkill -9 -x "${BINARY_NAME}" 2>/dev/null || true
sleep 1
# Remove old runtime binaries that may be "text file busy"
rm -f /root/bin/pulse-agent 2>/dev/null || true
+68
View File
@@ -5079,3 +5079,71 @@ func TestInstallSHWarnAgentTokenRejectedIsActionable(t *testing.T) {
}
}
}
// TestInstallSHAgentKillPatternsExcludeSiblingAgents guards a real incident:
// the installer and the Unraid wrapper it generates both killed agents with
// pkill -f "^<binary>". pkill -f matches the whole command line and "^" only
// anchors the start, so on a host running a second, co-installed agent whose
// binary name merely starts with the same prefix (pulse-agent alongside
// pulse-agent-prod) every install, upgrade, and wrapper restart silently took
// down the other agent too.
func TestInstallSHAgentKillPatternsExcludeSiblingAgents(t *testing.T) {
content, err := os.ReadFile(repoFile("scripts", "install.sh"))
if err != nil {
t.Fatalf("read install.sh: %v", err)
}
// Every pkill -f whose pattern is anchored at the agent binary must also
// bound the far end, or it matches the sibling agent.
unbounded := regexp.MustCompile(`pkill[^\n]*-f "\^\$\{(?:RUNTIME_BINARY|INSTALL_DIR)\}[^"\n]*"`)
for _, match := range unbounded.FindAllString(string(content), -1) {
if !strings.Contains(match, "[[:space:]]") {
t.Errorf("unbounded agent pkill pattern would also match a sibling agent: %s", match)
}
}
// A bare substring kill is worse still: it needs no prefix anchor at all.
if strings.Contains(string(content), `pkill -9 -f "pulse-agent"`) {
t.Error(`pkill -9 -f "pulse-agent" matches pulse-agent-prod; use -x on the exact process name`)
}
}
// TestInstallSHAgentKillPatternSparesSiblingAgent proves the pattern semantics
// rather than its spelling. pkill -f applies a POSIX ERE to the whole command
// line, so the same engine is exercised here against the two command lines a
// dual-agent host actually presents.
func TestInstallSHAgentKillPatternSparesSiblingAgent(t *testing.T) {
const (
targetCmd = "/usr/local/bin/pulse-agent --url http://192.168.0.113:7655 --interval 30s"
siblingCmd = "/usr/local/bin/pulse-agent-prod --url http://192.168.0.220:7655 --interval 30s"
)
ereMatches := func(t *testing.T, pattern, line string) bool {
t.Helper()
cmd := exec.Command("grep", "-E", "-q", pattern)
cmd.Stdin = strings.NewReader(line + "\n")
err := cmd.Run()
if err == nil {
return true
}
var exitErr *exec.ExitError
if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 {
return false
}
t.Fatalf("grep -E %q: %v", pattern, err)
return false
}
unbounded := "^/usr/local/bin/pulse-agent"
if !ereMatches(t, unbounded, siblingCmd) {
t.Fatal("premise check failed: the unbounded pattern is supposed to match the sibling agent")
}
bounded := "^/usr/local/bin/pulse-agent([[:space:]]|$)"
if !ereMatches(t, bounded, targetCmd) {
t.Error("bounded pattern must still match its own agent")
}
if ereMatches(t, bounded, siblingCmd) {
t.Error("bounded pattern must not match pulse-agent-prod")
}
}