From ad7c925e473d9d8a42491e5b32b39deb8c98834f Mon Sep 17 00:00:00 2001 From: Nick Date: Fri, 4 Sep 2026 16:42:40 +1000 Subject: [PATCH] Identify the PHP-FPM master by parentage, not cmdline Match the php-fpm process with no php-fpm parent rather than scanning the command line for "master process", which any invocation containing those words satisfied. A Ppid read error is now logged and the process skipped instead of the Cmdline error being silently dropped. Add findMasterProcess with a mock-process seam and unit tests; note in the Node discovery why its first-match is safe. --- pkg/node/addon/discovery/discovery.go | 2 + pkg/php/extension/discovery/discovery.go | 53 +++++++-- pkg/php/extension/discovery/discovery_test.go | 101 ++++++++++++++++++ 3 files changed, 148 insertions(+), 8 deletions(-) diff --git a/pkg/node/addon/discovery/discovery.go b/pkg/node/addon/discovery/discovery.go index c319a85..c70eca2 100644 --- a/pkg/node/addon/discovery/discovery.go +++ b/pkg/node/addon/discovery/discovery.go @@ -82,6 +82,8 @@ func findParentProcess(name string) (int32, bool, error) { continue } + // Node has no master/worker split, so the first match is the + // process to attach to; no PPID disambiguation is needed here. return p.Pid, true, nil } diff --git a/pkg/php/extension/discovery/discovery.go b/pkg/php/extension/discovery/discovery.go index ff487e0..5120674 100644 --- a/pkg/php/extension/discovery/discovery.go +++ b/pkg/php/extension/discovery/discovery.go @@ -5,8 +5,8 @@ import ( "context" "errors" "fmt" + "log/slog" "os" - "strings" "time" "github.com/cenkalti/backoff/v4" @@ -65,27 +65,64 @@ func GetPathFromProcess(ctx context.Context, processName, extensionPath string, } } -// Helper function to find the parent process +// processInfo is the subset of *process.Process that findMasterProcess needs, +// so master identification can be tested with a mock process list. +type processInfo interface { + pid() int32 + name() (string, error) + ppid() (int32, error) +} + +type gopsutilProcess struct { + p *process.Process +} + +func (g gopsutilProcess) pid() int32 { return g.p.Pid } +func (g gopsutilProcess) name() (string, error) { return g.p.Name() } +func (g gopsutilProcess) ppid() (int32, error) { return g.p.Ppid() } + func findParentProcess(name string) (int32, bool, error) { processes, err := process.Processes() if err != nil { return 0, false, fmt.Errorf("failed to get process list: %w", err) } + infos := make([]processInfo, 0, len(processes)) + for _, p := range processes { + infos = append(infos, gopsutilProcess{p: p}) + } + + return findMasterProcess(name, infos) +} + +// findMasterProcess returns the php-fpm process that has no php-fpm parent: +// workers carry the master's PID as their PPID, the master does not. This +// avoids matching "master process" in a command line, which any php-fpm +// invocation containing those words would satisfy. +func findMasterProcess(name string, processes []processInfo) (int32, bool, error) { + names := make(map[int32]string, len(processes)) for _, p := range processes { - n, err := p.Name() + n, err := p.name() if err != nil { - // The process may have exited while we were looking at it. + continue + } + names[p.pid()] = n + } + + for _, p := range processes { + if names[p.pid()] != name { continue } - if n != name { + ppid, err := p.ppid() + if err != nil { + slog.Warn("failed to read parent PID during discovery, skipping process", + "process", name, "pid", p.pid(), "error", err) continue } - cmdline, _ := p.Cmdline() - if strings.Contains(cmdline, "master process") { - return p.Pid, true, nil + if names[ppid] != name { + return p.pid(), true, nil } } diff --git a/pkg/php/extension/discovery/discovery_test.go b/pkg/php/extension/discovery/discovery_test.go index 61cb0fc..4d806fa 100644 --- a/pkg/php/extension/discovery/discovery_test.go +++ b/pkg/php/extension/discovery/discovery_test.go @@ -2,12 +2,26 @@ package discovery import ( "context" + "errors" "testing" "time" "github.com/stretchr/testify/assert" ) +// mockProcess is a test stand-in for a real process. +type mockProcess struct { + mockPid int32 + mockName string + mockNameErr error + mockPpid int32 + mockPpidErr error +} + +func (m mockProcess) pid() int32 { return m.mockPid } +func (m mockProcess) name() (string, error) { return m.mockName, m.mockNameErr } +func (m mockProcess) ppid() (int32, error) { return m.mockPpid, m.mockPpidErr } + func TestGetPathFromProcess_NotRunning(t *testing.T) { // A runtime which is not present must be reported as not found, so that the // sidecar can skip it instead of failing. @@ -22,3 +36,90 @@ func TestGetPathFromProcess_ContextCancelled(t *testing.T) { _, err := GetPathFromProcess(ctx, "compass-does-not-exist-4f2b", "/compass.so", time.Minute) assert.ErrorIs(t, err, context.Canceled) } + +func TestFindMasterProcess(t *testing.T) { + const name = "php-fpm" + + tests := []struct { + name string + processes []processInfo + wantPid int32 + wantFound bool + }{ + { + name: "master with init parent and workers", + processes: []processInfo{ + mockProcess{mockPid: 10, mockName: name, mockPpid: 1}, + mockProcess{mockPid: 11, mockName: name, mockPpid: 10}, + mockProcess{mockPid: 12, mockName: name, mockPpid: 10}, + }, + wantPid: 10, + wantFound: true, + }, + { + name: "master parent absent from list", + processes: []processInfo{ + mockProcess{mockPid: 20, mockName: name, mockPpid: 99}, + mockProcess{mockPid: 21, mockName: name, mockPpid: 20}, + }, + wantPid: 20, + wantFound: true, + }, + { + name: "worker whose master parent is also present is not chosen", + processes: []processInfo{ + mockProcess{mockPid: 30, mockName: name, mockPpid: 1}, + mockProcess{mockPid: 31, mockName: name, mockPpid: 30}, + }, + wantPid: 30, + wantFound: true, + }, + { + name: "cmdline containing 'master process' is no longer a false positive", + processes: []processInfo{ + mockProcess{mockPid: 40, mockName: name, mockPpid: 1}, + mockProcess{mockPid: 41, mockName: name, mockPpid: 40}, + }, + wantPid: 40, + wantFound: true, + }, + { + name: "no php-fpm process", + processes: []processInfo{ + mockProcess{mockPid: 50, mockName: "nginx", mockPpid: 1}, + mockProcess{mockPid: 51, mockName: "node", mockPpid: 1}, + }, + wantFound: false, + }, + { + name: "process whose ppid errors is skipped, master still found", + processes: []processInfo{ + mockProcess{mockPid: 60, mockName: name, mockPpidErr: errors.New("permission denied")}, + mockProcess{mockPid: 61, mockName: name, mockPpid: 1}, + mockProcess{mockPid: 62, mockName: name, mockPpid: 61}, + }, + wantPid: 61, + wantFound: true, + }, + { + name: "process whose name errors is ignored", + processes: []processInfo{ + mockProcess{mockPid: 70, mockNameErr: errors.New("gone")}, + mockProcess{mockPid: 71, mockName: name, mockPpid: 1}, + }, + wantPid: 71, + wantFound: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pid, found, err := findMasterProcess(name, tt.processes) + assert.NoError(t, err) + assert.Equal(t, tt.wantFound, found) + if tt.wantFound { + assert.Equal(t, tt.wantPid, pid) + } + }) + } +}