Merge branch 'pks-command-exit-goroutine-early' into 'master'

command: Fix Goroutines sticking around until context cancellation

Closes #4188

See merge request gitlab-org/gitaly!4719

17 files changed, 175 insertions, 113 deletions

diff --git a/cmd/gitaly-lfs-smudge/main_test.go b/cmd/gitaly-lfs-smudge/main_test.go
index 200746414..86662a823 100644
--- a/cmd/gitaly-lfs-smudge/main_test.go
+++ b/cmd/gitaly-lfs-smudge/main_test.go
@@ -6,7 +6,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"io"
-	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -184,8 +183,7 @@ func TestGitalyLFSSmudge(t *testing.T) {
 			env, logFile := tc.setup(t)
 
 			var stdout, stderr bytes.Buffer
-			cmd, err := command.New(ctx,
-				exec.Command(binary),
+			cmd, err := command.New(ctx, []string{binary},
 				command.WithStdin(tc.stdin),
 				command.WithStdout(&stdout),
 				command.WithStderr(&stderr),
diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go
index faf43435a..efa15b0bf 100644
--- a/internal/cgroups/v1_linux_test.go
+++ b/internal/cgroups/v1_linux_test.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"hash/crc32"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -82,8 +81,7 @@ func TestAddCommand(t *testing.T) {
 	require.NoError(t, v1Manager1.Setup())
 	ctx := testhelper.Context(t)
 
-	cmd1 := exec.Command("ls", "-hal", ".")
-	cmd2, err := command.New(ctx, cmd1)
+	cmd2, err := command.New(ctx, []string{"ls", "-hal", "."})
 	require.NoError(t, err)
 	require.NoError(t, cmd2.Wait())
 
@@ -179,11 +177,11 @@ func TestMetrics(t *testing.T) {
 	logger.SetLevel(logrus.DebugLevel)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := command.New(ctx, exec.Command("ls", "-hal", "."), command.WithCgroup(v1Manager1, repo))
+	cmd, err := command.New(ctx, []string{"ls", "-hal", "."}, command.WithCgroup(v1Manager1, repo))
 	require.NoError(t, err)
-	gitCmd1, err := command.New(ctx, exec.Command("ls", "-hal", "."), command.WithCgroup(v1Manager1, repo))
+	gitCmd1, err := command.New(ctx, []string{"ls", "-hal", "."}, command.WithCgroup(v1Manager1, repo))
 	require.NoError(t, err)
-	gitCmd2, err := command.New(ctx, exec.Command("ls", "-hal", "."), command.WithCgroup(v1Manager1, repo))
+	gitCmd2, err := command.New(ctx, []string{"ls", "-hal", "."}, command.WithCgroup(v1Manager1, repo))
 	require.NoError(t, err)
 	defer func() {
 		require.NoError(t, gitCmd2.Wait())
diff --git a/internal/command/command.go b/internal/command/command.go
index f7c1936c6..2881cf86f 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -136,8 +136,9 @@ type Command struct {
 	context      context.Context
 	startTime    time.Time
 
-	waitError error
-	waitOnce  sync.Once
+	waitError       error
+	waitOnce        sync.Once
+	processExitedCh chan struct{}
 
 	finalizer func(*Command)
 
@@ -149,14 +150,19 @@ type Command struct {
 	cmdGitVersion string
 }
 
-// New creates a Command from an exec.Cmd. On success, the Command contains a running subprocess.
-// When ctx is canceled the embedded process will be terminated and reaped automatically.
-func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
+// New creates a Command from the given executable name and arguments On success, the Command
+// contains a running subprocess. When ctx is canceled the embedded process will be terminated and
+// reaped automatically.
+func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, error) {
 	if ctx.Done() == nil {
 		panic(contextWithoutDonePanic("command spawned with context without Done() channel"))
 	}
 
-	if err := checkNullArgv(cmd); err != nil {
+	if len(nameAndArgs) == 0 {
+		panic("command spawned without name")
+	}
+
+	if err := checkNullArgv(nameAndArgs); err != nil {
 		return nil, err
 	}
 
@@ -167,8 +173,8 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
 
 	span, ctx := opentracing.StartSpanFromContext(
 		ctx,
-		cmd.Path,
-		opentracing.Tag{Key: "args", Value: strings.Join(cmd.Args, " ")},
+		nameAndArgs[0],
+		opentracing.Tag{Key: "args", Value: strings.Join(nameAndArgs[1:], " ")},
 	)
 
 	spawnStartTime := time.Now()
@@ -177,7 +183,7 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
 		return nil, err
 	}
 	service, method := methodFromContext(ctx)
-	cmdName := path.Base(cmd.Path)
+	cmdName := path.Base(nameAndArgs[0])
 	spawnTokenAcquiringSeconds.
 		WithLabelValues(service, method, cmdName, cfg.gitVersion).
 		Add(getSpawnTokenAcquiringSeconds(spawnStartTime))
@@ -188,22 +194,27 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
 	defer func() {
 		ctxlogrus.Extract(ctx).WithFields(logrus.Fields{
 			"pid":  logPid,
-			"path": cmd.Path,
-			"args": cmd.Args,
+			"path": nameAndArgs[0],
+			"args": nameAndArgs[1:],
 		}).Debug("spawn")
 	}()
 
+	cmd := exec.Command(nameAndArgs[0], nameAndArgs[1:]...)
+
 	command := &Command{
-		cmd:           cmd,
-		startTime:     time.Now(),
-		context:       ctx,
-		span:          span,
-		finalizer:     cfg.finalizer,
-		metricsCmd:    cfg.commandName,
-		metricsSubCmd: cfg.subcommandName,
-		cmdGitVersion: cfg.gitVersion,
+		cmd:             cmd,
+		startTime:       time.Now(),
+		context:         ctx,
+		span:            span,
+		finalizer:       cfg.finalizer,
+		metricsCmd:      cfg.commandName,
+		metricsSubCmd:   cfg.subcommandName,
+		cmdGitVersion:   cfg.gitVersion,
+		processExitedCh: make(chan struct{}),
 	}
 
+	cmd.Dir = cfg.dir
+
 	// Export allowed environment variables as set in the Gitaly process.
 	cmd.Env = AllowedEnvironment(os.Environ())
 	// Append environment variables explicitly requested by the caller.
@@ -268,17 +279,24 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
 	// We thus defer spawning the Goroutine.
 	defer func() {
 		go func() {
-			<-ctx.Done()
-
-			if process := cmd.Process; process != nil && process.Pid > 0 {
-				//nolint:errcheck // TODO: do we want to report errors?
-				// Send SIGTERM to the process group of cmd
-				syscall.Kill(-process.Pid, syscall.SIGTERM)
+			select {
+			case <-ctx.Done():
+				// If the context has been cancelled and we didn't explicitly reap
+				// the child process then we need to manually kill it and release
+				// all associated resources.
+				if cmd.Process.Pid > 0 {
+					//nolint:errcheck // TODO: do we want to report errors?
+					// Send SIGTERM to the process group of cmd
+					syscall.Kill(-cmd.Process.Pid, syscall.SIGTERM)
+				}
+
+				// We do not care for any potential error code, but just want to
+				// make sure that the subprocess gets properly killed and processed.
+				_ = command.Wait()
+			case <-command.processExitedCh:
+				// Otherwise, if the process has exited via a call to `wait()`
+				// already then there is nothing we need to do.
 			}
-
-			// We do not care for any potential error code, but just want to make sure that the
-			// subprocess gets properly killed and processed.
-			_ = command.Wait()
 		}()
 	}()
 
@@ -326,6 +344,8 @@ func (c *Command) Wait() error {
 
 // This function should never be called directly, use Wait().
 func (c *Command) wait() {
+	defer close(c.processExitedCh)
+
 	if c.writer != nil {
 		// Prevent the command from blocking on waiting for stdin to be closed
 		c.writer.Close()
@@ -533,12 +553,11 @@ func methodFromContext(ctx context.Context) (service string, method string) {
 	return "", ""
 }
 
-// Command arguments will be passed to the exec syscall as
-// null-terminated C strings. That means the arguments themselves may not
-// contain a null byte. The go stdlib checks for null bytes but it
+// Command arguments will be passed to the exec syscall as null-terminated C strings. That means the
+// arguments themselves may not contain a null byte. The go stdlib checks for null bytes but it
 // returns a cryptic error. This function returns a more explicit error.
-func checkNullArgv(cmd *exec.Cmd) error {
-	for _, arg := range cmd.Args {
+func checkNullArgv(args []string) error {
+	for _, arg := range args {
 		if strings.IndexByte(arg, 0) > -1 {
 			// Use %q so that the null byte gets printed as \x00
 			return fmt.Errorf("detected null byte in command argument %q", arg)
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index c3c7b85e9..e10dadd4e 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -7,7 +7,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os/exec"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -33,7 +32,7 @@ func TestNew_environment(t *testing.T) {
 	extraVar := "FOOBAR=123456"
 
 	var buf bytes.Buffer
-	cmd, err := New(ctx, exec.Command("/usr/bin/env"), WithStdout(&buf), WithEnvironment([]string{extraVar}))
+	cmd, err := New(ctx, []string{"/usr/bin/env"}, WithStdout(&buf), WithEnvironment([]string{extraVar}))
 
 	require.NoError(t, err)
 	require.NoError(t, cmd.Wait())
@@ -121,7 +120,7 @@ func TestNew_exportedEnvironment(t *testing.T) {
 			t.Setenv(tc.key, tc.value)
 
 			var buf bytes.Buffer
-			cmd, err := New(ctx, exec.Command("/usr/bin/env"), WithStdout(&buf))
+			cmd, err := New(ctx, []string{"/usr/bin/env"}, WithStdout(&buf))
 			require.NoError(t, err)
 			require.NoError(t, cmd.Wait())
 
@@ -138,7 +137,7 @@ func TestNew_unexportedEnv(t *testing.T) {
 	t.Setenv(unexportedEnvKey, unexportedEnvVal)
 
 	var buf bytes.Buffer
-	cmd, err := New(ctx, exec.Command("/usr/bin/env"), WithStdout(&buf))
+	cmd, err := New(ctx, []string{"/usr/bin/env"}, WithStdout(&buf))
 	require.NoError(t, err)
 	require.NoError(t, cmd.Wait())
 
@@ -149,7 +148,7 @@ func TestNew_rejectContextWithoutDone(t *testing.T) {
 	t.Parallel()
 
 	require.PanicsWithValue(t, contextWithoutDonePanic("command spawned with context without Done() channel"), func() {
-		_, err := New(testhelper.ContextWithoutCancel(), exec.Command("true"))
+		_, err := New(testhelper.ContextWithoutCancel(), []string{"true"})
 		require.NoError(t, err)
 	})
 }
@@ -172,7 +171,7 @@ func TestNew_spawnTimeout(t *testing.T) {
 
 	errCh := make(chan error)
 	go func() {
-		_, err := New(ctx, exec.Command("true"))
+		_, err := New(ctx, []string{"true"})
 		errCh <- err
 	}()
 
@@ -194,11 +193,11 @@ func TestCommand_Wait_contextCancellationKillsCommand(t *testing.T) {
 
 	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
-	cmd, err := New(ctx, exec.CommandContext(ctx, "sleep", "1h"))
+	cmd, err := New(ctx, []string{"sleep", "1h"})
 	require.NoError(t, err)
 
 	// Cancel the command early.
-	go cancel()
+	cancel()
 
 	err = cmd.Wait()
 	require.Error(t, err)
@@ -215,7 +214,7 @@ func TestNew_setupStdin(t *testing.T) {
 	stdin := "Test value"
 
 	var buf bytes.Buffer
-	cmd, err := New(ctx, exec.Command("cat"), WithSetupStdin(), WithStdout(&buf))
+	cmd, err := New(ctx, []string{"cat"}, WithSetupStdin(), WithStdout(&buf))
 	require.NoError(t, err)
 
 	_, err = fmt.Fprintf(cmd, "%s", stdin)
@@ -230,7 +229,7 @@ func TestCommand_read(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 
-	cmd, err := New(ctx, exec.Command("echo", "test value"))
+	cmd, err := New(ctx, []string{"echo", "test value"})
 	require.NoError(t, err)
 
 	output, err := io.ReadAll(cmd)
@@ -245,7 +244,7 @@ func TestNew_nulByteInArgument(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 
-	cmd, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"))
+	cmd, err := New(ctx, []string{"sh", "-c", "hello\x00world"})
 	require.Equal(t, fmt.Errorf("detected null byte in command argument %q", "hello\x00world"), err)
 	require.Nil(t, cmd)
 }
@@ -255,7 +254,7 @@ func TestNew_missingBinary(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 
-	cmd, err := New(ctx, exec.Command("command-non-existent"))
+	cmd, err := New(ctx, []string{"command-non-existent"})
 	require.EqualError(t, err, "starting process [command-non-existent]: exec: \"command-non-existent\": executable file not found in $PATH")
 	require.Nil(t, cmd)
 }
@@ -276,7 +275,7 @@ func TestCommand_stderrLogging(t *testing.T) {
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 	var stdout bytes.Buffer
-	cmd, err := New(ctx, exec.Command(binaryPath), WithStdout(&stdout))
+	cmd, err := New(ctx, []string{binaryPath}, WithStdout(&stdout))
 	require.NoError(t, err)
 
 	require.EqualError(t, cmd.Wait(), "exit status 1")
@@ -300,7 +299,7 @@ func TestCommand_stderrLoggingTruncation(t *testing.T) {
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 	var stdout bytes.Buffer
-	cmd, err := New(ctx, exec.Command(binaryPath), WithStdout(&stdout))
+	cmd, err := New(ctx, []string{binaryPath}, WithStdout(&stdout))
 	require.NoError(t, err)
 
 	require.Error(t, cmd.Wait())
@@ -321,7 +320,7 @@ func TestCommand_stderrLoggingWithNulBytes(t *testing.T) {
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 	var stdout bytes.Buffer
-	cmd, err := New(ctx, exec.Command(binaryPath), WithStdout(&stdout))
+	cmd, err := New(ctx, []string{binaryPath}, WithStdout(&stdout))
 	require.NoError(t, err)
 
 	require.Error(t, cmd.Wait())
@@ -344,7 +343,7 @@ func TestCommand_stderrLoggingLongLine(t *testing.T) {
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 	var stdout bytes.Buffer
-	cmd, err := New(ctx, exec.Command(binaryPath), WithStdout(&stdout))
+	cmd, err := New(ctx, []string{binaryPath}, WithStdout(&stdout))
 	require.NoError(t, err)
 
 	require.Error(t, cmd.Wait())
@@ -391,7 +390,7 @@ func TestCommand_stderrLoggingMaxBytes(t *testing.T) {
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 	var stdout bytes.Buffer
-	cmd, err := New(ctx, exec.Command(binaryPath), WithStdout(&stdout))
+	cmd, err := New(ctx, []string{binaryPath}, WithStdout(&stdout))
 	require.NoError(t, err)
 	require.Error(t, cmd.Wait())
 
@@ -413,7 +412,7 @@ func TestCommand_logMessage(t *testing.T) {
 
 	ctx := ctxlogrus.ToContext(testhelper.Context(t), logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("echo", "hello world"),
+	cmd, err := New(ctx, []string{"echo", "hello world"},
 		WithCgroup(mockCgroupManager("/sys/fs/cgroup/1"), nil),
 	)
 	require.NoError(t, err)
@@ -443,7 +442,7 @@ func TestNew_commandSpawnTokenMetrics(t *testing.T) {
 	tags.Set("grpc.request.fullMethod", "/test.Service/TestRPC")
 	ctx = grpcmwtags.SetInContext(ctx, tags)
 
-	cmd, err := New(ctx, exec.Command("echo", "goodbye, cruel world."))
+	cmd, err := New(ctx, []string{"echo", "goodbye, cruel world."})
 
 	require.NoError(t, err)
 	require.NoError(t, cmd.Wait())
@@ -460,3 +459,56 @@ gitaly_command_spawn_token_acquiring_seconds_total{cmd="echo",git_version="",grp
 		),
 	)
 }
+
+func TestCommand_withFinalizer(t *testing.T) {
+	t.Parallel()
+
+	t.Run("context cancellation runs finalizer", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		finalizerCh := make(chan struct{})
+		_, err := New(ctx, []string{"echo"}, WithFinalizer(func(*Command) {
+			close(finalizerCh)
+		}))
+		require.NoError(t, err)
+
+		cancel()
+
+		<-finalizerCh
+	})
+
+	t.Run("Wait runs finalizer", func(t *testing.T) {
+		ctx := testhelper.Context(t)
+
+		finalizerCh := make(chan struct{})
+		cmd, err := New(ctx, []string{"echo"}, WithFinalizer(func(*Command) {
+			close(finalizerCh)
+		}))
+		require.NoError(t, err)
+
+		require.NoError(t, cmd.Wait())
+
+		<-finalizerCh
+	})
+
+	t.Run("process exit does not run finalizer", func(t *testing.T) {
+		ctx := testhelper.Context(t)
+
+		finalizerCh := make(chan struct{})
+		_, err := New(ctx, []string{"echo"}, WithFinalizer(func(*Command) {
+			close(finalizerCh)
+		}))
+		require.NoError(t, err)
+
+		select {
+		case <-finalizerCh:
+			// Command finalizers should only be running when we have either explicitly
+			// called `Wait()` on the command, or when the context has been cancelled.
+			// Otherwise we may run into the case where finalizers have already been ran
+			// on the exited process even though we may still be busy handling the
+			// output of that command, which may result in weird races.
+			require.FailNow(t, "finalizer should not have been ran")
+		case <-time.After(50 * time.Millisecond):
+		}
+	})
+}
diff --git a/internal/command/option.go b/internal/command/option.go
index d5ec59635..cca8e62f1 100644
--- a/internal/command/option.go
+++ b/internal/command/option.go
@@ -10,6 +10,7 @@ type config struct {
 	stdin       io.Reader
 	stdout      io.Writer
 	stderr      io.Writer
+	dir         string
 	environment []string
 
 	finalizer func(*Command)
@@ -56,6 +57,13 @@ func WithStderr(stderr io.Writer) Option {
 	}
 }
 
+// WithDir will set up the command to be ran in the specific directory.
+func WithDir(dir string) Option {
+	return func(cfg *config) {
+		cfg.dir = dir
+	}
+}
+
 // WithEnvironment sets up environment variables that shall be set for the command.
 func WithEnvironment(environment []string) Option {
 	return func(cfg *config) {
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index de4ee857e..210288dd4 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"sync"
 
@@ -338,7 +337,7 @@ func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) {
 	// Furthermore, note that we're not using `newCommand()` but instead hand-craft the command.
 	// This is required to avoid a cyclic dependency when we need to check the version in
 	// `newCommand()` itself.
-	cmd, err := command.New(ctx, exec.Command(execEnv.BinaryPath, "version"), command.WithEnvironment(execEnv.EnvironmentVariables))
+	cmd, err := command.New(ctx, []string{execEnv.BinaryPath, "version"}, command.WithEnvironment(execEnv.EnvironmentVariables))
 	if err != nil {
 		return Version{}, fmt.Errorf("spawning version command: %w", err)
 	}
@@ -391,16 +390,14 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 
 	env = append(env, execEnv.EnvironmentVariables...)
 
-	execCommand := exec.Command(execEnv.BinaryPath, args...)
-	execCommand.Dir = dir
-
 	cmdGitVersion, err := cf.GitVersion(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("getting Git version: %w", err)
 	}
 
-	command, err := command.New(ctx, execCommand, append(
+	command, err := command.New(ctx, append([]string{execEnv.BinaryPath}, args...), append(
 		config.commandOpts,
+		command.WithDir(dir),
 		command.WithEnvironment(env),
 		command.WithCommandName("git", sc.Subcommand()),
 		command.WithCgroup(cf.cgroupsManager, repo),
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 2639cc48e..44c940dd2 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -6,7 +6,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os/exec"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -234,7 +233,7 @@ func (o *ObjectPool) logStats(ctx context.Context, when string) error {
 
 func sizeDir(ctx context.Context, dir string) (int64, error) {
 	// du -k reports size in KB
-	cmd, err := command.New(ctx, exec.Command("du", "-sk", dir))
+	cmd, err := command.New(ctx, []string{"du", "-sk", dir})
 	if err != nil {
 		return 0, err
 	}
diff --git a/internal/git/packfile/index.go b/internal/git/packfile/index.go
index e8915e1d3..2fcf985eb 100644
--- a/internal/git/packfile/index.go
+++ b/internal/git/packfile/index.go
@@ -10,7 +10,6 @@ import (
 	"io"
 	"math"
 	"os"
-	"os/exec"
 	"regexp"
 	"sort"
 	"strconv"
@@ -93,7 +92,7 @@ func ReadIndex(idxPath string) (*Index, error) {
 		return nil, err
 	}
 
-	showIndex, err := command.New(ctx, exec.Command("git", "show-index"), command.WithStdin(f))
+	showIndex, err := command.New(ctx, []string{"git", "show-index"}, command.WithStdin(f))
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go
index 264f34a56..f1e594779 100644
--- a/internal/git2go/executor.go
+++ b/internal/git2go/executor.go
@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os/exec"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
@@ -84,7 +83,7 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re
 	}, args...)
 
 	var stdout bytes.Buffer
-	cmd, err := command.New(ctx, exec.Command(b.binaryPath, args...),
+	cmd, err := command.New(ctx, append([]string{b.binaryPath}, args...),
 		command.WithStdin(stdin),
 		command.WithStdout(&stdout),
 		command.WithStderr(log),
diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index 2c819dd3e..9db59adae 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strings"
 
@@ -66,9 +65,8 @@ func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, ho
 		}
 
 		for _, hookFile := range hookFiles {
-			cmd := exec.Command(hookFile, args...)
-			cmd.Dir = repoPath
-			c, err := command.New(ctx, cmd,
+			c, err := command.New(ctx, append([]string{hookFile}, args...),
+				command.WithDir(repoPath),
 				command.WithStdin(bytes.NewReader(stdinBytes)),
 				command.WithStdout(stdout),
 				command.WithStderr(stderr),
diff --git a/internal/gitaly/linguist/linguist.go b/internal/gitaly/linguist/linguist.go
index 2a1a92c60..145e4f8b9 100644
--- a/internal/gitaly/linguist/linguist.go
+++ b/internal/gitaly/linguist/linguist.go
@@ -107,10 +107,10 @@ func (inst *Instance) startGitLinguist(ctx context.Context, repoPath string, com
 		return nil, fmt.Errorf("finding bundle executable: %w", err)
 	}
 
-	cmd := exec.Command(bundle, "exec", "bin/gitaly-linguist", "--repository="+repoPath, "--commit="+commitID)
-	cmd.Dir = inst.cfg.Ruby.Dir
+	cmd := []string{bundle, "exec", "bin/gitaly-linguist", "--repository=" + repoPath, "--commit=" + commitID}
 
 	internalCmd, err := command.New(ctx, cmd,
+		command.WithDir(inst.cfg.Ruby.Dir),
 		command.WithEnvironment(env.AllowedRubyEnvironment(os.Environ())),
 		command.WithCommandName("git-linguist", "stats"),
 	)
diff --git a/internal/gitaly/service/repository/archive.go b/internal/gitaly/service/repository/archive.go
index 959064237..b2688fcb7 100644
--- a/internal/gitaly/service/repository/archive.go
+++ b/internal/gitaly/service/repository/archive.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"strings"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -24,18 +23,18 @@ import (
 )
 
 type archiveParams struct {
-	writer      io.Writer
-	in          *gitalypb.GetArchiveRequest
-	compressCmd *exec.Cmd
-	format      string
-	archivePath string
-	exclude     []string
-	loggingDir  string
+	writer       io.Writer
+	in           *gitalypb.GetArchiveRequest
+	compressArgs []string
+	format       string
+	archivePath  string
+	exclude      []string
+	loggingDir   string
 }
 
 func (s *server) GetArchive(in *gitalypb.GetArchiveRequest, stream gitalypb.RepositoryService_GetArchiveServer) error {
 	ctx := stream.Context()
-	compressCmd, format := parseArchiveFormat(in.GetFormat())
+	compressArgs, format := parseArchiveFormat(in.GetFormat())
 	repo := s.localrepo(in.GetRepository())
 
 	repoRoot, err := repo.Path()
@@ -83,24 +82,24 @@ func (s *server) GetArchive(in *gitalypb.GetArchiveRequest, stream gitalypb.Repo
 	ctxlogrus.Extract(ctx).WithField("request_hash", requestHash(in)).Info("request details")
 
 	return s.handleArchive(ctx, archiveParams{
-		writer:      writer,
-		in:          in,
-		compressCmd: compressCmd,
-		format:      format,
-		archivePath: path,
-		exclude:     exclude,
-		loggingDir:  s.loggingCfg.Dir,
+		writer:       writer,
+		in:           in,
+		compressArgs: compressArgs,
+		format:       format,
+		archivePath:  path,
+		exclude:      exclude,
+		loggingDir:   s.loggingCfg.Dir,
 	})
 }
 
-func parseArchiveFormat(format gitalypb.GetArchiveRequest_Format) (*exec.Cmd, string) {
+func parseArchiveFormat(format gitalypb.GetArchiveRequest_Format) ([]string, string) {
 	switch format {
 	case gitalypb.GetArchiveRequest_TAR:
 		return nil, "tar"
 	case gitalypb.GetArchiveRequest_TAR_GZ:
-		return exec.Command("gzip", "-c", "-n"), "tar"
+		return []string{"gzip", "-c", "-n"}, "tar"
 	case gitalypb.GetArchiveRequest_TAR_BZ2:
-		return exec.Command("bzip2", "-c"), "tar"
+		return []string{"bzip2", "-c"}, "tar"
 	case gitalypb.GetArchiveRequest_ZIP:
 		return nil, "zip"
 	}
@@ -229,8 +228,8 @@ func (s *server) handleArchive(ctx context.Context, p archiveParams) error {
 		return err
 	}
 
-	if p.compressCmd != nil {
-		command, err := command.New(ctx, p.compressCmd,
+	if len(p.compressArgs) > 0 {
+		command, err := command.New(ctx, p.compressArgs,
 			command.WithStdin(archiveCommand), command.WithStdout(p.writer),
 		)
 		if err != nil {
diff --git a/internal/gitaly/service/repository/backup_custom_hooks.go b/internal/gitaly/service/repository/backup_custom_hooks.go
index e434e6f65..9a44385d7 100644
--- a/internal/gitaly/service/repository/backup_custom_hooks.go
+++ b/internal/gitaly/service/repository/backup_custom_hooks.go
@@ -2,7 +2,6 @@ package repository
 
 import (
 	"os"
-	"os/exec"
 	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
@@ -29,7 +28,7 @@ func (s *server) BackupCustomHooks(in *gitalypb.BackupCustomHooksRequest, stream
 	}
 
 	ctx := stream.Context()
-	tar := exec.Command("tar", "-c", "-f", "-", "-C", repoPath, customHooksDir)
+	tar := []string{"tar", "-c", "-f", "-", "-C", repoPath, customHooksDir}
 	cmd, err := command.New(ctx, tar, command.WithStdout(writer))
 	if err != nil {
 		return status.Errorf(codes.Internal, "%v", err)
diff --git a/internal/gitaly/service/repository/create_repository_from_snapshot.go b/internal/gitaly/service/repository/create_repository_from_snapshot.go
index f665faa87..9a2e5bdc0 100644
--- a/internal/gitaly/service/repository/create_repository_from_snapshot.go
+++ b/internal/gitaly/service/repository/create_repository_from_snapshot.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"net"
 	"net/http"
-	"os/exec"
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
@@ -67,7 +66,7 @@ func untar(ctx context.Context, path string, in *gitalypb.CreateRepositoryFromSn
 		return status.Errorf(codes.Internal, "HTTP server: %v", rsp.Status)
 	}
 
-	cmd, err := command.New(ctx, exec.Command("tar", "-C", path, "-xvf", "-"), command.WithStdin(rsp.Body))
+	cmd, err := command.New(ctx, []string{"tar", "-C", path, "-xvf", "-"}, command.WithStdin(rsp.Body))
 	if err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go
index 6c66bcd74..67a52eca3 100644
--- a/internal/gitaly/service/repository/replicate.go
+++ b/internal/gitaly/service/repository/replicate.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strings"
 
@@ -188,8 +187,9 @@ func (s *server) extractSnapshot(ctx context.Context, source, target *gitalypb.R
 	}
 
 	stderr := &bytes.Buffer{}
-	cmd, err := command.New(ctx, exec.Command("tar", "-C", targetPath, "-xvf", "-"),
-		command.WithStdin(snapshotReader), command.WithStderr(stderr),
+	cmd, err := command.New(ctx, []string{"tar", "-C", targetPath, "-xvf", "-"},
+		command.WithStdin(snapshotReader),
+		command.WithStderr(stderr),
 	)
 	if err != nil {
 		return fmt.Errorf("create tar command: %w", err)
diff --git a/internal/gitaly/service/repository/restore_custom_hooks.go b/internal/gitaly/service/repository/restore_custom_hooks.go
index 572181ffe..a3e2289e7 100644
--- a/internal/gitaly/service/repository/restore_custom_hooks.go
+++ b/internal/gitaly/service/repository/restore_custom_hooks.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -62,7 +61,7 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus
 	}
 
 	ctx := stream.Context()
-	cmd, err := command.New(ctx, exec.Command("tar", cmdArgs...), command.WithStdin(reader))
+	cmd, err := command.New(ctx, append([]string{"tar"}, cmdArgs...), command.WithStdin(reader))
 	if err != nil {
 		return status.Errorf(codes.Internal, "RestoreCustomHooks: Could not untar custom hooks tar %v", err)
 	}
@@ -149,7 +148,7 @@ func (s *server) restoreCustomHooksWithVoting(stream gitalypb.RepositoryService_
 		customHooksDir,
 	}
 
-	cmd, err := command.New(ctx, exec.Command("tar", cmdArgs...), command.WithStdin(reader))
+	cmd, err := command.New(ctx, append([]string{"tar"}, cmdArgs...), command.WithStdin(reader))
 	if err != nil {
 		return helper.ErrInternalf("RestoreCustomHooks: Could not untar custom hooks tar %w", err)
 	}
diff --git a/internal/gitaly/service/repository/size.go b/internal/gitaly/service/repository/size.go
index 95bf22402..3eb0f83a5 100644
--- a/internal/gitaly/service/repository/size.go
+++ b/internal/gitaly/service/repository/size.go
@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os/exec"
 	"strconv"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -67,7 +66,7 @@ func (s *server) GetObjectDirectorySize(ctx context.Context, in *gitalypb.GetObj
 }
 
 func getPathSize(ctx context.Context, path string) int64 {
-	cmd, err := command.New(ctx, exec.Command("du", "-sk", path))
+	cmd, err := command.New(ctx, []string{"du", "-sk", path})
 	if err != nil {
 		ctxlogrus.Extract(ctx).WithError(err).Warn("ignoring du command error")
 		return 0