diff --git a/internal/command/command_inline_shell.go b/internal/command/command_inline_shell.go index c95d1d77..539bb6ef 100644 --- a/internal/command/command_inline_shell.go +++ b/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 } } diff --git a/internal/command/command_native.go b/internal/command/command_native.go index 671f9558..b8692d9a 100644 --- a/internal/command/command_native.go +++ b/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 { diff --git a/internal/command/command_unix.go b/internal/command/command_unix.go index 6a3f846a..d27377fb 100644 --- a/internal/command/command_unix.go +++ b/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 { diff --git a/internal/command/command_unix_test.go b/internal/command/command_unix_test.go index 650b412b..62c16be4 100644 --- a/internal/command/command_unix_test.go +++ b/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) { diff --git a/internal/command/command_virtual.go b/internal/command/command_virtual.go index ad6a7eaa..ea27d4d7 100644 --- a/internal/command/command_virtual.go +++ b/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 { diff --git a/internal/command/command_windows.go b/internal/command/command_windows.go index 3a1301f7..a452600a 100644 --- a/internal/command/command_windows.go +++ b/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) {} diff --git a/internal/runnerv2service/execution.go b/internal/runnerv2service/execution.go index 65f1f4f9..b40b96fd 100644 --- a/internal/runnerv2service/execution.go +++ b/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. diff --git a/internal/runnerv2service/service_execute.go b/internal/runnerv2service/service_execute.go index eb872b4e..066a336d 100644 --- a/internal/runnerv2service/service_execute.go +++ b/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 diff --git a/internal/runnerv2service/service_execute_test.go b/internal/runnerv2service/service_execute_test.go index 15ccf8f6..733a2a22 100644 --- a/internal/runnerv2service/service_execute_test.go +++ b/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