only use setpgid for 'sh -c' wrapped subshell calls

Previously setpgid was set for all commands. This had an unintended
side effect of breaking certain use cases, like using an interactive
'bash' as the command to be executed/managed as a way to debug/test your
setup.

This was also added as part of the conversion from the old (broken)
shell parser to using 'sh -c' subshells for those calls. After that
I added support for single commands and command lists that don't get
wrapped. Using setpgid is not needed on the non-wrapped calls as it was
added to ensure backwards compatibility with signal propagation for
the wrapped commands (the wrapping shell discards some signals).

child/child.go

@@ -119,11 +119,14 @@ type NewInput struct {
 	Splay time.Duration
 
 	// Setsid flag, if set to true will create the child processes with their own
-	// session ID and Process group ID. The default value of false will cause the
-	// child processes to have their own PGID but they will have the same SID as
-	// that of their parent
+	// session ID and Process group ID. Note this overrides Setpgid below.
 	Setsid bool
 
+	// Setpgid flag, if set to true will create the child processes with their
+	// own Process group ID. If set the child processes to have their own PGID
+	// but they will have the same SID as that of their parent
+	Setpgid bool
+
 	// an optional logger that can be used for messages pertinent to the child process
 	Logger *log.Logger
 }
@@ -157,7 +160,7 @@ func New(i *NewInput) (*Child, error) {
 		killTimeout:  i.KillTimeout,
 		splay:        i.Splay,
 		stopCh:       make(chan struct{}, 1),
-		setpgid:      !i.Setsid,
+		setpgid:      i.Setpgid && !i.Setsid,
 		setsid:       i.Setsid,
 		logger:       i.Logger,
 	}

child/child_test.go

@@ -370,6 +370,7 @@ func TestStop_noWaitForSplay(t *testing.T) {
 	c.splay = 100 * time.Second
 	c.reloadSignal = nil
 	c.killSignal = syscall.SIGUSR1
+	c.setpgid = true
 
 	out := gatedio.NewByteBuffer()
 	c.stdout = out

child/command-prep.go

@@ -8,7 +8,7 @@ import (
 	"strings"
 )
 
-func CommandPrep(command []string) ([]string, error) {
+func CommandPrep(command []string) ([]string, bool, error) {
 	switch {
 	case len(command) == 1 && len(strings.Fields(command[0])) > 1:
 		// command is []string{"command using arguments or shell features"}
@@ -24,15 +24,15 @@ func CommandPrep(command []string) ([]string, error) {
 			}
 		}
 		if shell == "" {
-			return []string{}, exec.ErrNotFound
+			return []string{}, false, exec.ErrNotFound
 		}
 		cmd := []string{shell, "-c", command[0]}
-		return cmd, nil
+		return cmd, true, nil
 	case len(command) >= 1 && len(strings.TrimSpace(command[0])) > 0:
 		// command is already good ([]string{"foo"}, []string{"foo", "bar"}, ..)
-		return command, nil
+		return command, false, nil
 	default:
 		// command is []string{} or []string{""}
-		return []string{}, exec.ErrNotFound
+		return []string{}, false, exec.ErrNotFound
 	}
 }

child/command-prep_test.go

@@ -9,28 +9,33 @@ import (
 func Test_CommandPrep(t *testing.T) {
 	type cmd []string
 	cases := []struct {
-		n   string
-		in  cmd
-		out cmd
-		err error
+		n     string
+		in    cmd
+		out   cmd
+		subsh bool
+		err   error
 	}{
-		{n: "0", in: cmd{}, out: cmd{}, err: exec.ErrNotFound},
+		{n: "empty", in: cmd{}, out: cmd{}, err: exec.ErrNotFound},
 		{n: "''", in: cmd{""}, out: cmd{}, err: exec.ErrNotFound},
 		{n: "' '", in: cmd{" "}, out: cmd{}, err: exec.ErrNotFound},
 		{n: "'f'", in: cmd{"foo"}, out: cmd{"foo"}, err: nil},
-		{n: "'f b'", in: cmd{"foo bar"}, out: cmd{"sh", "-c", "foo bar"}, err: nil},
+		{n: "'f b'", in: cmd{"foo bar"}, subsh: true, out: cmd{"sh", "-c", "foo bar"}, err: nil},
 		{n: "'f','b'", in: cmd{"foo", "bar"}, out: cmd{"foo", "bar"}, err: nil},
 		{n: "'f','b','z'", in: cmd{"foo", "bar", "zed"}, out: cmd{"foo", "bar", "zed"}, err: nil},
 	}
 	for _, tc := range cases {
 		t.Run(tc.n, func(t *testing.T) {
-			out, err := CommandPrep(tc.in)
+			out, subsh, err := CommandPrep(tc.in)
 			if !reflect.DeepEqual(cmd(out), tc.out) {
-				t.Errorf("bad prepCommand output. wanted: %#v, got %#v", tc.out, out)
+				t.Errorf("bad commandPrep command output;"+
+					"wanted: %#v, got %#v", tc.out, out)
 			}
 			if err != tc.err {
 				t.Errorf("bad prepCommand error. wanted: %v, got %v", tc.err, err)
 			}
+			if tc.subsh != subsh {
+				t.Errorf("incorrectly marked as using subshell")
+			}
 		})
 	}
 }

child/command-prep_windows.go

@@ -8,16 +8,16 @@ import (
 	"strings"
 )
 
-func CommandPrep(command []string) ([]string, error) {
+func CommandPrep(command []string) ([]string, bool, error) {
 	switch {
 	case len(command) == 1 && len(strings.Fields(command[0])) == 1:
 		// command is []string{"foo"}
-		return []string{command[0]}, nil
+		return []string{command[0]}, false, nil
 	case len(command) > 1:
 		// command is []string{"foo", "bar"}
-		return command, nil
+		return command, false, nil
 	default:
 		// command is []string{}, []string{""}, []string{"foo bar"}
-		return []string{}, exec.ErrNotFound
+		return []string{}, false, exec.ErrNotFound
 	}
 }

manager/runner.go

@@ -1199,7 +1199,7 @@ type spawnChildInput struct {
 // spawnChild spawns a child process with the given inputs and returns the
 // resulting child.
 func spawnChild(i *spawnChildInput) (*child.Child, error) {
-	args, err := child.CommandPrep(i.Command)
+	args, subshell, err := child.CommandPrep(i.Command)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed parsing command")
 	}
@@ -1215,6 +1215,7 @@ func spawnChild(i *spawnChildInput) (*child.Child, error) {
 		KillSignal:   i.KillSignal,
 		KillTimeout:  i.KillTimeout,
 		Splay:        i.Splay,
+		Setpgid:      subshell, // setpgid for subshells to propagate signals
 	})
 	if err != nil {
 		return nil, errors.Wrap(err, "error creating child")

manager/runner_test.go

@@ -1114,7 +1114,7 @@ func TestRunner_command(t *testing.T) {
 	os.Setenv("FOO", "bar")
 
 	parseTest := func(tc testCase) {
-		out, err := child.CommandPrep(tc.input)
+		out, _, err := child.CommandPrep(tc.input)
 		mismatchErr := "bad command parse\n   got: '%#v'\nwanted: '%#v'"
 		switch {
 		case err != nil && len(tc.input) > 0 && tc.input[0] != "":
@@ -1128,7 +1128,7 @@ func TestRunner_command(t *testing.T) {
 	runTest := func(tc testCase) {
 		stdout := new(bytes.Buffer)
 		stderr := new(bytes.Buffer)
-		args, err := child.CommandPrep(tc.input)
+		args, _, err := child.CommandPrep(tc.input)
 		switch {
 		case err == exec.ErrNotFound && len(args) == 0:
 			return // expected
@@ -1265,7 +1265,7 @@ func TestRunner_commandPath(t *testing.T) {
 	PATH := os.Getenv("PATH")
 	defer os.Setenv("PATH", PATH)
 	os.Setenv("PATH", "")
-	cmd, err := child.CommandPrep([]string{"echo hi"})
+	cmd, _, err := child.CommandPrep([]string{"echo hi"})
 	if err != nil && err != exec.ErrNotFound {
 		t.Fatal(err)
 	}