diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index f69826238..99031ad60 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -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`, diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 75a930803..3abc526e7 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -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 diff --git a/scripts/install.sh b/scripts/install.sh index 651cd1eea..6f90ea8bb 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -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 diff --git a/scripts/installtests/install_sh_test.go b/scripts/installtests/install_sh_test.go index e655fc5ed..d1b6b76a4 100644 --- a/scripts/installtests/install_sh_test.go +++ b/scripts/installtests/install_sh_test.go @@ -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 "^". 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") + } +}