cmd/gitaly-wrapper: Add tests for finding processes by PID file

Now that `readPIDFile()` doesn't depend on global state anymore, it's
also a lot easier to test `findProcess()` via a PID file given that it
also doesn't depend on environment variables anymore, but instead
accepts a parameter. Add a bunch of tests to verify it works as
expected.

While at it, rename the function to `findProcess()` instead of
`findGitaly()`. The executable is used for both Praefect and Gitaly, so
the found process may not in fact be a Gitaly instance.

2 files changed, 79 insertions, 16 deletions

diff --git a/cmd/gitaly-wrapper/main.go b/cmd/gitaly-wrapper/main.go
index a70855bf5..7f3a93899 100644
--- a/cmd/gitaly-wrapper/main.go
+++ b/cmd/gitaly-wrapper/main.go
@@ -44,16 +44,16 @@ func main() {
 	if pidFilePath == "" {
 		logger.Fatalf("missing pid file ENV variable %q", bootstrap.EnvPidFile)
 	}
-	logger.WithField("pid_file", pidFilePath).Info("finding gitaly")
+	logger.WithField("pid_file", pidFilePath).Info("finding process")
 
-	gitaly, err := findGitaly(pidFilePath)
+	process, err := findProcess(pidFilePath)
 	if err != nil && !isRecoverable(err) {
-		logger.WithError(err).Fatal("find gitaly")
+		logger.WithError(err).Fatal("find process")
 	} else if err != nil {
-		logger.WithError(err).Error("find gitaly")
+		logger.WithError(err).Error("find process")
 	}
 
-	if gitaly != nil && isGitaly(gitaly, gitalyBin) {
+	if process != nil && isGitaly(process, gitalyBin) {
 		logger.Info("adopting a process")
 	} else {
 		logger.Info("spawning a process")
@@ -63,20 +63,20 @@ func main() {
 			logger.WithError(err).Fatal("spawn gitaly")
 		}
 
-		gitaly = proc
+		process = proc
 	}
 
-	logger = logger.WithField("gitaly", gitaly.Pid)
-	logger.Info("monitoring gitaly")
+	logger = logger.WithField("process", process.Pid)
+	logger.Info("monitoring process")
 
-	forwardSignals(gitaly, logger)
+	forwardSignals(process, logger)
 
 	// wait
-	for isAlive(gitaly) {
+	for isAlive(process) {
 		time.Sleep(1 * time.Second)
 	}
 
-	logger.Error("wrapper for gitaly shutting down")
+	logger.Error("wrapper for process shutting down")
 }
 
 func isRecoverable(err error) bool {
@@ -84,20 +84,20 @@ func isRecoverable(err error) bool {
 	return os.IsNotExist(err) || errors.As(err, &numError)
 }
 
-func findGitaly(pidFilePath string) (*os.Process, error) {
+func findProcess(pidFilePath string) (*os.Process, error) {
 	pid, err := readPIDFile(pidFilePath)
 	if err != nil {
 		return nil, err
 	}
 
 	// os.FindProcess on unix do not return an error if the process does not exist
-	gitaly, err := os.FindProcess(pid)
+	process, err := os.FindProcess(pid)
 	if err != nil {
 		return nil, err
 	}
 
-	if isAlive(gitaly) {
-		return gitaly, nil
+	if isAlive(process) {
+		return process, nil
 	}
 
 	return nil, nil
diff --git a/cmd/gitaly-wrapper/main_test.go b/cmd/gitaly-wrapper/main_test.go
index 8664c0bcb..5895fd60d 100644
--- a/cmd/gitaly-wrapper/main_test.go
+++ b/cmd/gitaly-wrapper/main_test.go
@@ -33,7 +33,7 @@ func TestStolenPid(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, pidFile.Close())
 
-	tail, err := findGitaly(pidFile.Name())
+	tail, err := findProcess(pidFile.Name())
 	require.NoError(t, err)
 	require.NotNil(t, tail)
 	require.Equal(t, cmd.Process.Pid, tail.Pid)
@@ -47,6 +47,69 @@ func TestStolenPid(t *testing.T) {
 	})
 }
 
+func TestFindProcess(t *testing.T) {
+	t.Run("nonexistent PID file", func(t *testing.T) {
+		_, err := findProcess("does-not-exist")
+		require.True(t, os.IsNotExist(err))
+	})
+
+	t.Run("PID file with garbage", func(t *testing.T) {
+		path := filepath.Join(testhelper.TempDir(t), "pid")
+		require.NoError(t, os.WriteFile(path, []byte("garbage"), 0o644))
+
+		_, err := findProcess(path)
+		_, expectedErr := strconv.Atoi("garbage")
+		require.Equal(t, expectedErr, err)
+	})
+
+	t.Run("nonexistent process", func(t *testing.T) {
+		// The below PID can exist, but chances are sufficiently low to hopefully not matter
+		// in practice.
+		path := filepath.Join(testhelper.TempDir(t), "pid")
+		require.NoError(t, os.WriteFile(path, []byte("7777777"), 0o644))
+
+		// The process isn't alive, so we expect neither an error nor a process to be
+		// returned.
+		process, err := findProcess(path)
+		require.Nil(t, process)
+		require.Nil(t, err)
+	})
+
+	t.Run("running process", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
+		// The process will exit immediately, but that's not much of a problem given that
+		// `findProcess()` also works with zombies.
+		executable := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "noop"), []byte(
+			`#!/usr/bin/env bash
+			echo ready
+		`))
+
+		cmd := exec.CommandContext(ctx, executable)
+		stdout, err := cmd.StdoutPipe()
+		require.NoError(t, err)
+
+		require.NoError(t, cmd.Start())
+		defer func() {
+			require.NoError(t, cmd.Wait())
+		}()
+
+		// Wait for the process to be ready such that we know it's started up successfully
+		// and is executing the bash shell.
+		_, err = stdout.Read(make([]byte, 10))
+		require.NoError(t, err)
+
+		path := filepath.Join(testhelper.TempDir(t), "pid")
+		require.NoError(t, os.WriteFile(path, []byte(strconv.FormatInt(int64(cmd.Process.Pid), 10)), 0o644))
+
+		process, err := findProcess(path)
+		require.NotNil(t, process)
+		require.Equal(t, cmd.Process.Pid, process.Pid)
+		require.Equal(t, nil, err)
+	})
+}
+
 func TestIsRecoverable(t *testing.T) {
 	_, numericError := strconv.Atoi("")