command: Fix race with setting command names and context cancellation

When spawning commands, callers can afterwards call `SetMetricsCmd()`
and `SetMetricsSubCmd()` to have the command be properly represented in
Prometheus metrics via its command and subcommand. But because we spawn
a Goroutine that logs these metrics as soon as the context is done, this
creates a race between setting those names and context cancellation.

Fix this race by converting these functions into options that can be
passed to `New()` so that they're set up correctly at construction time.

Changelog: fixed

6 files changed, 28 insertions, 25 deletions

diff --git a/internal/command/command.go b/internal/command/command.go
index 5782e75dc..e1f3f0458 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -185,10 +185,12 @@ func New(ctx context.Context, cmd *exec.Cmd, opts ...Option) (*Command, error) {
 	}()
 
 	command := &Command{
-		cmd:       cmd,
-		startTime: time.Now(),
-		context:   ctx,
-		span:      span,
+		cmd:           cmd,
+		startTime:     time.Now(),
+		context:       ctx,
+		span:          span,
+		metricsCmd:    cfg.commandName,
+		metricsSubCmd: cfg.subcommandName,
 	}
 
 	// Export allowed environment variables as set in the Gitaly process.
@@ -297,16 +299,6 @@ func (c *Command) SetCgroupPath(path string) {
 	c.cgroupPath = path
 }
 
-// SetMetricsCmd overrides the "cmd" label used in metrics
-func (c *Command) SetMetricsCmd(metricsCmd string) {
-	c.metricsCmd = metricsCmd
-}
-
-// SetMetricsSubCmd sets the "subcmd" label used in metrics
-func (c *Command) SetMetricsSubCmd(metricsSubCmd string) {
-	c.metricsSubCmd = metricsSubCmd
-}
-
 // This function should never be called directly, use Wait().
 func (c *Command) wait() {
 	if c.writer != nil {
diff --git a/internal/command/option.go b/internal/command/option.go
index 7e4c0c64f..ade36dbd5 100644
--- a/internal/command/option.go
+++ b/internal/command/option.go
@@ -7,6 +7,9 @@ type config struct {
 	stdout      io.Writer
 	stderr      io.Writer
 	environment []string
+
+	commandName    string
+	subcommandName string
 }
 
 // Option is an option that can be passed to `New()` for controlling how the command is being
@@ -49,3 +52,11 @@ func WithEnvironment(environment []string) Option {
 		cfg.environment = environment
 	}
 }
+
+// WithCommandName overrides the "cmd" and "subcmd" label used in metrics.
+func WithCommandName(commandName, subcommandName string) Option {
+	return func(cfg *config) {
+		cfg.commandName = commandName
+		cfg.subcommandName = subcommandName
+	}
+}
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index a18c9fd63..93242d1f3 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -397,13 +397,15 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 	execCommand := exec.Command(execEnv.BinaryPath, args...)
 	execCommand.Dir = dir
 
-	command, err := command.New(ctx, execCommand, append(config.commandOpts, command.WithEnvironment(env))...)
+	command, err := command.New(ctx, execCommand, append(
+		config.commandOpts,
+		command.WithEnvironment(env),
+		command.WithCommandName("git", sc.Subcommand()),
+	)...)
 	if err != nil {
 		return nil, err
 	}
 
-	command.SetMetricsSubCmd(sc.Subcommand())
-
 	if featureflag.RunCommandsInCGroup.IsEnabled(ctx) {
 		if err := cf.cgroupsManager.AddCommand(command, repo); err != nil {
 			return nil, err
diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go
index d5319304f..c1e220f43 100644
--- a/internal/git2go/executor.go
+++ b/internal/git2go/executor.go
@@ -93,13 +93,12 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re
 		command.WithStdout(&stdout),
 		command.WithStderr(log),
 		command.WithEnvironment(env),
+		command.WithCommandName("gitaly-git2go", subcmd),
 	)
 	if err != nil {
 		return nil, err
 	}
 
-	cmd.SetMetricsSubCmd(subcmd)
-
 	if err := cmd.Wait(); err != nil {
 		return nil, err
 	}
diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index 490409773..2c819dd3e 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -73,13 +73,12 @@ func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, ho
 				command.WithStdout(stdout),
 				command.WithStderr(stderr),
 				command.WithEnvironment(env),
+				command.WithCommandName("gitaly-hooks", hookName),
 			)
 			if err != nil {
 				return err
 			}
 
-			c.SetMetricsSubCmd(hookName)
-
 			if err = c.Wait(); err != nil {
 				// Custom hook errors need to be handled specially when we update
 				// refs via updateref.UpdaterWithHooks: their stdout and stderr must
diff --git a/internal/gitaly/linguist/linguist.go b/internal/gitaly/linguist/linguist.go
index 4bc9dbce1..da085b58a 100644
--- a/internal/gitaly/linguist/linguist.go
+++ b/internal/gitaly/linguist/linguist.go
@@ -95,14 +95,14 @@ func (inst *Instance) startGitLinguist(ctx context.Context, repoPath string, com
 	cmd := exec.Command(bundle, "exec", "bin/gitaly-linguist", "--repository="+repoPath, "--commit="+commitID)
 	cmd.Dir = inst.cfg.Ruby.Dir
 
-	internalCmd, err := command.New(ctx, cmd, command.WithEnvironment(env.AllowedRubyEnvironment(os.Environ())))
+	internalCmd, err := command.New(ctx, cmd,
+		command.WithEnvironment(env.AllowedRubyEnvironment(os.Environ())),
+		command.WithCommandName("git-linguist", "stats"),
+	)
 	if err != nil {
 		return nil, fmt.Errorf("creating command: %w", err)
 	}
 
-	internalCmd.SetMetricsCmd("git-linguist")
-	internalCmd.SetMetricsSubCmd("stats")
-
 	return internalCmd, nil
 }