fix(daemon): recover stale service PID files safely

pkg/service/daemon/daemon.go

@@ -218,13 +218,21 @@ func validatePIDFileInfo(info os.FileInfo) error {
 	return nil
 }
 
-func servicePIDConflictError(pid int) error {
-	return fmt.Errorf(
+type servicePIDMismatchError struct {
+	pid int
+}
+
+func (e *servicePIDMismatchError) Error() string {
+	return fmt.Sprintf(
 		"service PID file points to live process %d that does not match the Zaparoo service binary",
-		pid,
+		e.pid,
 	)
 }
 
+func servicePIDConflictError(pid int) error {
+	return &servicePIDMismatchError{pid: pid}
+}
+
 // Running returns true if the service is running.
 func (s *Service) Running() (bool, error) {
 	pid, err := s.Pid()
@@ -237,7 +245,11 @@ func (s *Service) Running() (bool, error) {
 	}
 
 	if pidRunning(pid) {
-		if s.pidMatchesService(pid) {
+		matches, identityErr := s.serviceProcessIdentity(pid)
+		if identityErr != nil {
+			return false, fmt.Errorf("error identifying service PID %d: %w", pid, identityErr)
+		}
+		if matches {
 			return true, nil
 		}
 		log.Warn().
@@ -1089,6 +1101,23 @@ func (s *Service) Start() error {
 	defer release()
 
 	running, err := s.Running()
+	var conflict *servicePIDMismatchError
+	if errors.As(err, &conflict) {
+		// Only Start may recover a confirmed foreign PID, under the same gate
+		// that protects PID publication. Never signal the unrelated process.
+		pid, pidErr := s.Pid()
+		if pidErr != nil {
+			return pidErr
+		}
+		if pid != conflict.pid {
+			return errors.New("service PID changed during start")
+		}
+		if removeErr := s.removePidFile(); removeErr != nil {
+			return removeErr
+		}
+		log.Warn().Int("pid", pid).Msg("removed stale service PID file without signaling unrelated process")
+		err = nil
+	}
 	if err != nil {
 		return err
 	}
@@ -1283,27 +1312,45 @@ func pidIsZombie(pid int) bool {
 }
 
 func (s *Service) pidMatchesService(pid int) bool {
+	matches, _ := s.serviceProcessIdentity(pid)
+	return matches
+}
+
+// Unknown identity must not be treated as a confirmed PID reuse: removing its
+// PID file could let Start create a second service when procfs is inaccessible.
+func (s *Service) serviceProcessIdentity(pid int) (bool, error) {
 	if runtime.GOOS != "linux" {
-		return true
+		return true, nil
 	}
 
 	dataDir := helpers.DataDir(s.pl)
-	exePath, err := os.Readlink(filepath.Join(procDir(), strconv.Itoa(pid), "exe"))
-	if err == nil && pathLooksLikeServiceBinary(exePath, dataDir) {
-		return true
+	exePath, exeErr := os.Readlink(filepath.Join(procDir(), strconv.Itoa(pid), "exe"))
+	exePath = strings.TrimSuffix(exePath, " (deleted)")
+	if exeErr == nil && pathLooksLikeServiceBinary(exePath, dataDir) {
+		return true, nil
 	}
 
 	cmdlinePath := filepath.Join(procDir(), strconv.Itoa(pid), "cmdline")
 	cmdline, err := os.ReadFile(cmdlinePath) //nolint:gosec // reads process status for service management
 	if err != nil {
-		return false
+		return false, fmt.Errorf("reading process command line: %w", err)
 	}
-	for _, arg := range strings.Split(string(cmdline), "\x00") {
-		if pathLooksLikeServiceBinary(arg, dataDir) {
-			return true
-		}
+	if exeErr != nil {
+		return false, fmt.Errorf("reading process executable: %w", exeErr)
+	}
+	return serviceScriptIdentity(exePath, cmdline, dataDir), nil
+}
+
+func serviceScriptIdentity(exePath string, cmdline []byte, dataDir string) bool {
+	// A data argument to cat/tail/etc. is not service identity. The supported
+	// shell-backed caches are executed as interpreter argv[1] by the shebang.
+	switch filepath.Base(exePath) {
+	case "sh", "bash", "dash", "busybox":
+		args := strings.Split(string(cmdline), "\x00")
+		return len(args) > 1 && pathLooksLikeServiceBinary(args[1], dataDir)
+	default:
+		return false
 	}
-	return false
 }
 
 func procDir() string {

pkg/service/daemon/daemon_test.go

@@ -716,23 +716,69 @@ func TestRunningReturnsFalseForLiveUnrelatedPID(t *testing.T) {
 	assert.FileExists(t, pidFile)
 }
 
-func TestStartFailsForLiveUnrelatedPID(t *testing.T) {
+func TestStartRecoversLiveUnrelatedPID(t *testing.T) {
 	requireLinuxProc(t, "service PID identity checks")
 
 	svc := newTestService(t)
-	settings := svc.pl.Settings()
-	pidFile := filepath.Join(settings.TempDir, config.PidFile)
+	pidFile := filepath.Join(svc.pl.Settings().TempDir, config.PidFile)
+	eventLog := filepath.Join(t.TempDir(), "events.log")
+	t.Setenv(config.AppEnv, writeFakeServiceScript(t, pidFile, eventLog))
 
 	process := exec.CommandContext(context.Background(), "sleep", "1000")
 	require.NoError(t, process.Start())
-	t.Cleanup(func() { _ = process.Process.Kill() })
+	t.Cleanup(func() {
+		_ = process.Process.Kill()
+		_ = process.Wait()
+	})
 	require.NoError(t, os.WriteFile(pidFile, []byte(strconv.Itoa(process.Process.Pid)), 0o600))
+	t.Cleanup(func() {
+		pid, err := svc.Pid()
+		if err == nil && pid > 0 && pid != process.Process.Pid {
+			require.NoError(t, svc.Stop())
+		}
+	})
 
-	err := svc.Start()
+	// Competing ensures must recover the stale file and publish only one PID.
+	const racers = 4
+	results := make(chan error, racers)
+	for range racers {
+		go func() { results <- svc.Start() }()
+	}
+	for range racers {
+		require.NoError(t, <-results)
+	}
+	pid, err := svc.Pid()
+	require.NoError(t, err)
+	assert.NotEqual(t, process.Process.Pid, pid)
+	assert.True(t, requireServiceRunning(t, svc))
+	assert.True(t, pidRunning(process.Process.Pid), "unrelated process must not be signaled")
+
+	// An idempotent ensure after recovery must retain the same service.
+	require.NoError(t, svc.Start())
+	nextPID, err := svc.Pid()
+	require.NoError(t, err)
+	assert.Equal(t, pid, nextPID)
+}
+
+func TestServiceScriptIdentityRejectsDataArguments(t *testing.T) {
+	t.Parallel()
+
+	dataDir := t.TempDir()
+	service := filepath.Join(dataDir, "zaparoo.0123456789abcdef.sh")
+	assert.True(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00"+service+"\x00-service\x00exec\x00"), dataDir))
+	assert.True(t, serviceScriptIdentity("/bin/busybox", []byte("sh\x00"+service+"\x00"), dataDir))
+	assert.False(t, serviceScriptIdentity("/bin/tail", []byte("tail\x00"+service+"\x00"), dataDir))
+	assert.False(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00-c\x00"+service+"\x00"), dataDir))
+}
+
+func TestUnavailableProcessIdentityIsNotPIDConflict(t *testing.T) {
+	requireLinuxProc(t, "service PID identity checks")
+
+	matches, err := newTestService(t).serviceProcessIdentity(-1)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "does not match the Zaparoo service binary")
-	assert.True(t, pidRunning(process.Process.Pid))
-	assert.FileExists(t, pidFile)
+	assert.False(t, matches)
+	var conflict *servicePIDMismatchError
+	assert.NotErrorAs(t, err, &conflict, "unknown identity must not authorize PID-file removal")
 }
 
 func TestRestartFailsForLiveUnrelatedPID(t *testing.T) {