Bring back signaling to process group

internal/command/command_inline_shell.go

@@ -49,11 +49,11 @@ func (c *inlineShellCommand) Wait() error {
 	err := c.internalCommand.Wait()
 
 	if c.envCollector != nil {
-		if cErr := c.collectEnv(); cErr != nil {
-			c.logger.Info("failed to collect the environment", zap.Error(cErr))
-			if err == nil {
-				err = cErr
-			}
+		c.logger.Info("collecting the environment after the script execution")
+		cErr := c.collectEnv()
+		c.logger.Info("collected the environment after the script execution", zap.Error(cErr))
+		if cErr != nil && err == nil {
+			err = cErr
 		}
 	}
 

internal/command/command_native.go

@@ -10,7 +10,7 @@ import (
 )
 
 // SignalToProcessGroup is used in tests to disable sending signals to a process group.
-var SignalToProcessGroup = false
+var SignalToProcessGroup = true
 
 type nativeCommand struct {
 	*base
@@ -69,14 +69,9 @@ func (c *nativeCommand) Start(ctx context.Context) (err error) {
 	c.cmd.Stdout = c.Stdout()
 	c.cmd.Stderr = c.Stderr()
 
-	// Set the process group ID of the program.
-	// It is helpful to stop the program and its
-	// children.
-	// Note that Setsid set in setSysProcAttrCtty()
-	// already starts a new process group.
-	// Warning: it does not work with interactive programs
-	// like "python", hence, it's commented out.
-	// setSysProcAttrPgid(c.cmd)
+	// Creating a new process group is required to properly replicate a behaviour
+	// similar to CTRL-C in the terminal, which sends a SIGINT to the whole group.
+	setSysProcAttrPgid(c.cmd)
 
 	c.logger.Info("starting a native command", zap.Any("config", redactConfig(c.ProgramConfig())))
 	if err := c.cmd.Start(); err != nil {

internal/command/command_unix.go

@@ -12,12 +12,19 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-func setSysProcAttrCtty(cmd *exec.Cmd) {
+// func setSysProcAttrCtty(cmd *exec.Cmd) {
+// 	if cmd.SysProcAttr == nil {
+// 		cmd.SysProcAttr = &syscall.SysProcAttr{}
+// 	}
+// 	cmd.SysProcAttr.Setctty = true
+// 	cmd.SysProcAttr.Setsid = true
+// }
+
+func setSysProcAttrPgid(cmd *exec.Cmd) {
 	if cmd.SysProcAttr == nil {
 		cmd.SysProcAttr = &syscall.SysProcAttr{}
 	}
-	cmd.SysProcAttr.Setsid = true
-	cmd.SysProcAttr.Setctty = true
+	cmd.SysProcAttr.Setpgid = true
 }
 
 func disableEcho(fd uintptr) error {

internal/command/command_unix_test.go

@@ -18,12 +18,6 @@ import (
 	"github.com/stateful/runme/v3/pkg/document/identity"
 )
 
-func init() {
-	// Set to false to disable sending signals to process groups in tests.
-	// This can be turned on if setSysProcAttrPgid() is called in Start().
-	SignalToProcessGroup = false
-}
-
 func TestCommand(t *testing.T) {
 	testCases := []struct {
 		name           string
@@ -488,7 +482,7 @@ func TestCommand_SimulateCtrlC(t *testing.T) {
 		require.NoError(t, err)
 	}
 
-	require.EqualError(t, cmd.Wait(), "exit status 130")
+	require.EqualError(t, cmd.Wait(), "signal: killed")
 }
 
 func TestCommand_StopWithSignal(t *testing.T) {

internal/command/command_virtual.go

@@ -81,7 +81,13 @@ func (c *virtualCommand) Start(ctx context.Context) (err error) {
 	c.cmd.Stdout = c.tty
 	c.cmd.Stderr = c.tty
 
-	setSysProcAttrCtty(c.cmd)
+	// TODO(adamb): creating a new session and setting a controling terminal
+	// might be required; find the use case and then implement it properly.
+	// The current implementation misses setting Ctty.
+	// setSysProcAttrCtty(c.cmd)
+	// Creating a new process group is required to properly replicate a behaviour
+	// similar to CTRL-C in the terminal, which sends a SIGINT to the whole group.
+	setSysProcAttrPgid(c.cmd)
 
 	c.logger.Info("starting a virtual command", zap.Any("config", redactConfig(c.ProgramConfig())))
 	if err := c.cmd.Start(); err != nil {

internal/command/command_windows.go

@@ -9,7 +9,7 @@ import (
 	"github.com/pkg/errors"
 )
 
-func setSysProcAttrCtty(cmd *exec.Cmd) {}
+// func setSysProcAttrCtty(cmd *exec.Cmd) {}
 
 func setSysProcAttrPgid(cmd *exec.Cmd) {}
 

internal/runnerv2service/execution.go

@@ -158,6 +158,8 @@ func (e *execution) Wait(ctx context.Context, sender sender) (int, error) {
 	waitErr := e.Cmd.Wait()
 	exitCode := exitCodeFromErr(waitErr)
 
+	e.logger.Info("command finished", zap.Int("exitCode", exitCode), zap.Error(waitErr))
+
 	e.closeIO()
 
 	// If waitErr is not nil, only log the errors but return waitErr.

internal/runnerv2service/service_execute.go

@@ -122,7 +122,6 @@ func (r *runnerService) Execute(srv runnerv2alpha1.RunnerService_ExecuteServer)
 	}(req)
 
 	exitCode, waitErr := exec.Wait(ctx, srv)
-
 	logger.Info("command finished", zap.Int("exitCode", exitCode), zap.Error(waitErr))
 
 	var finalExitCode *wrapperspb.UInt32Value

internal/runnerv2service/service_execute_test.go

@@ -26,10 +26,6 @@ import (
 )
 
 func init() {
-	// Set to false to disable sending signals to process groups in tests.
-	// This can be turned on if setSysProcAttrPgid() is called in Start().
-	command.SignalToProcessGroup = false
-
 	command.SetEnvDumpCommand("env -0")
 
 	// Server uses autoconfig to get necessary dependencies.
@@ -714,19 +710,22 @@ func TestRunnerServiceServerExecute_WithInput(t *testing.T) {
 
 		// cancel sleep
 		time.Sleep(time.Millisecond * 500)
-		err = stream.Send(&runnerv2alpha1.ExecuteRequest{InputData: []byte{0x03}})
+		err = stream.Send(&runnerv2alpha1.ExecuteRequest{InputData: []byte{0x04}})
 		assert.NoError(t, err)
 
-		// terminate shell
+		// terminate outer shell and its children
 		time.Sleep(time.Millisecond * 500)
-		err = stream.Send(&runnerv2alpha1.ExecuteRequest{InputData: []byte{0x04}})
+		err = stream.Send(&runnerv2alpha1.ExecuteRequest{
+			// TODO(adamb): figure out why SIGINT does not work
+			Stop: runnerv2alpha1.ExecuteStop_EXECUTE_STOP_KILL,
+		})
 		assert.NoError(t, err)
 
 		result := <-execResult
 
 		// TODO(adamb): This should be a specific gRPC error rather than Unknown.
-		assert.Contains(t, result.Err.Error(), "exit status 130")
-		assert.Equal(t, 130, result.ExitCode)
+		assert.Contains(t, result.Err.Error(), "signal: killed")
+		assert.Equal(t, 137, result.ExitCode)
 	})
 
 	t.Run("CloseSendDirection", func(t *testing.T) {
@@ -846,7 +845,6 @@ func TestRunnerServiceServerExecute_WithStop(t *testing.T) {
 			},
 			Interactive: true,
 		},
-		InputData: []byte("a\n"),
 	})
 	require.NoError(t, err)
 
@@ -859,7 +857,7 @@ func TestRunnerServiceServerExecute_WithStop(t *testing.T) {
 		})
 		errc <- err
 	}()
-	require.NoError(t, <-errc)
+	assert.NoError(t, <-errc)
 
 	result := <-execResult