Merge branch 'revert-f955266a' into 'master'

Revert "Merge branch 'jc-spawn-git-in-pgid' into 'master'"

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4908

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>

7 files changed, 4 insertions, 111 deletions

diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 62428a511..805ac336a 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -195,10 +195,7 @@ func run(cfg config.Cfg) error {
 	}
 
 	skipHooks, _ := env.GetBool("GITALY_TESTING_NO_GIT_HOOKS", false)
-	commandFactoryOpts := []git.ExecCommandFactoryOption{
-		git.WithGitalyPid(os.Getpid()),
-	}
-
+	var commandFactoryOpts []git.ExecCommandFactoryOption
 	if skipHooks {
 		commandFactoryOpts = append(commandFactoryOpts, git.WithSkipHooks())
 	}
diff --git a/internal/command/command.go b/internal/command/command.go
index 8862fe3eb..2881cf86f 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -201,16 +201,6 @@ func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, e
 
 	cmd := exec.Command(nameAndArgs[0], nameAndArgs[1:]...)
 
-	if featureflag.RunCmdsInProcessGroup.IsEnabled(ctx) {
-		cmd.SysProcAttr = &syscall.SysProcAttr{
-			Setpgid: true,
-			Pgid:    cfg.parentPid,
-		}
-	} else {
-		// Start the command in its own process group (nice for signalling)
-		cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-	}
-
 	command := &Command{
 		cmd:             cmd,
 		startTime:       time.Now(),
@@ -232,6 +222,9 @@ func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, e
 	// And finally inject environment variables required for tracing into the command.
 	cmd.Env = envInjector(ctx, cmd.Env)
 
+	// Start the command in its own process group (nice for signalling)
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+
 	// Three possible values for stdin:
 	//   * nil - Go implicitly uses /dev/null
 	//   * stdinSentinel - configure with cmd.StdinPipe(), allowing Write() to work
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 784b52e85..e10dadd4e 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -7,11 +7,9 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
-	"syscall"
 	"testing"
 	"time"
 
@@ -23,7 +21,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
@@ -515,64 +512,3 @@ func TestCommand_withFinalizer(t *testing.T) {
 		}
 	})
 }
-
-func TestCommand_withPgid(t *testing.T) {
-	t.Parallel()
-
-	ctx := testhelper.Context(t)
-	r, w := io.Pipe()
-	parentCmd, err := New(ctx, []string{"cat", "/dev/stdin"}, WithStdin(r))
-	require.NoError(t, err)
-
-	errCh := make(chan error)
-	go func() {
-		errCh <- parentCmd.Wait()
-	}()
-
-	parentPid := parentCmd.cmd.Process.Pid
-
-	childCmd, err := New(
-		ctx,
-		[]string{"cat", "/dev/stdin"},
-		WithStdin(r),
-		WithParent(parentPid),
-	)
-	require.NoError(t, err)
-	go func() {
-		errCh <- childCmd.Wait()
-	}()
-
-	childPid := childCmd.cmd.Process.Pid
-
-	// check that both processes are running
-	for _, pid := range []int{parentPid, childPid} {
-		process, err := os.FindProcess(pid)
-		require.NoError(t, err)
-		require.NoError(t, process.Signal(syscall.Signal(0)))
-	}
-
-	// check that killing the pgid kills the childpid as well
-	if featureflag.RunCmdsInProcessGroup.IsEnabled(ctx) {
-		require.NoError(t, syscall.Kill(-parentPid, syscall.SIGKILL))
-	} else {
-		require.NoError(t, syscall.Kill(parentPid, syscall.SIGKILL))
-		require.NoError(t, syscall.Kill(childPid, syscall.SIGKILL))
-	}
-
-	w.Close()
-
-	err = <-errCh
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "signal: killed")
-
-	err = <-errCh
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "signal: killed")
-
-	// check that both processes are not running
-	for _, pid := range []int{parentPid, childPid} {
-		process, err := os.FindProcess(pid)
-		require.NoError(t, err)
-		require.Error(t, process.Signal(syscall.Signal(0)))
-	}
-}
diff --git a/internal/command/option.go b/internal/command/option.go
index b91ff8dbd..cca8e62f1 100644
--- a/internal/command/option.go
+++ b/internal/command/option.go
@@ -21,8 +21,6 @@ type config struct {
 
 	cgroupsManager CgroupsManager
 	cgroupsRepo    repository.GitRepo
-
-	parentPid int
 }
 
 // Option is an option that can be passed to `New()` for controlling how the command is being
@@ -110,10 +108,3 @@ func WithFinalizer(finalizer func(*Command)) Option {
 		cfg.finalizer = finalizer
 	}
 }
-
-// WithParent sets a parent pid under which the command will be spawned
-func WithParent(pid int) Option {
-	return func(cfg *config) {
-		cfg.parentPid = pid
-	}
-}
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 53444b43b..dae719935 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -43,7 +43,6 @@ type CommandFactory interface {
 type execCommandFactoryConfig struct {
 	hooksPath     string
 	gitBinaryPath string
-	gitalyPid     int
 }
 
 // ExecCommandFactoryOption is an option that can be passed to NewExecCommandFactory.
@@ -70,14 +69,6 @@ func WithGitBinaryPath(path string) ExecCommandFactoryOption {
 	}
 }
 
-// WithGitalyPid will cause new commands to be started under the gitaly pid as
-// the pgid
-func WithGitalyPid(gitalyPid int) ExecCommandFactoryOption {
-	return func(cfg *execCommandFactoryConfig) {
-		cfg.gitalyPid = gitalyPid
-	}
-}
-
 type hookDirectories struct {
 	tempHooksPath string
 }
@@ -98,8 +89,6 @@ type ExecCommandFactory struct {
 
 	cachedGitVersionLock     sync.RWMutex
 	cachedGitVersionByBinary map[string]cachedGitVersion
-
-	gitalyPid int
 }
 
 // NewExecCommandFactory returns a new instance of initialized ExecCommandFactory. The returned
@@ -149,7 +138,6 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_
 		),
 		hookDirs:                 hookDirectories,
 		cachedGitVersionByBinary: make(map[string]cachedGitVersion),
-		gitalyPid:                factoryCfg.gitalyPid,
 	}
 
 	return gitCmdFactory, runCleanups, nil
@@ -425,7 +413,6 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 		command.WithCommandName("git", sc.Subcommand()),
 		command.WithCgroup(cf.cgroupsManager, repo),
 		command.WithCommandGitVersion(cmdGitVersion.String()),
-		command.WithParent(cf.gitalyPid),
 	)...)
 	if err != nil {
 		return nil, err
diff --git a/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go b/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go
deleted file mode 100644
index 055ec6798..000000000
--- a/internal/metadata/featureflag/ff_run_cmds_in_pgroup.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// RunCmdsInProcessGroup will run all git commands under the Gitaly pid as the
-// pgid.
-var RunCmdsInProcessGroup = NewFeatureFlag(
-	"run_cmds_in_process_group",
-	"v15.5.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4494",
-	false,
-)
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 63ef64cad..02440e7f0 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -196,7 +196,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	// deep in the call stack, so almost every test function would have to inject it into its
 	// context.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true)
-	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCmdsInProcessGroup, rnd.Int()%2 == 0)
 	// PraefectGeneratedReplicaPaths affects many tests as it changes the repository creation logic.
 	// Randomly enable the flag to exercise both paths to some extent.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectGeneratedReplicaPaths, rnd.Int()%2 == 0)