Skip to content
This repository has been archived by the owner on May 8, 2024. It is now read-only.

Commit

Permalink
Don't hide cancellation reason (#363)
Browse files Browse the repository at this point in the history
* Don't hide cancellation reason

Otherwise, we assume a task has timeout when in reality it could've been externally interrupted.

* revert log

* Less changes

* Removed whitespace
  • Loading branch information
fkorotkov committed Apr 25, 2024
1 parent 5af6378 commit 28f4c3d
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 10 deletions.
5 changes: 3 additions & 2 deletions internal/executor/executor.go
Expand Up @@ -52,6 +52,7 @@ type StepResult struct {

var (
ErrStepExit = errors.New("executor step requested to terminate execution")
ErrTimedOut = errors.New("timed out")
)

func NewExecutor(
Expand Down Expand Up @@ -215,7 +216,7 @@ func (executor *Executor) RunBuild(ctx context.Context) {
// Normal timeout-bounded context
timeout := time.Duration(response.TimeoutInSeconds) * time.Second

timeoutCtx, timeoutCtxCancel := context.WithTimeout(ctx, timeout)
timeoutCtx, timeoutCtxCancel := context.WithTimeoutCause(ctx, timeout, ErrTimedOut)
defer timeoutCtxCancel()

// Like timeout-bounded context, but extended by 5 minutes
Expand Down Expand Up @@ -500,7 +501,7 @@ func (executor *Executor) performStep(ctx context.Context, currentStep *api.Comm
signaledToExit = ws.Signaled()
}
}
if err == TimeOutError {
if errors.Is(err, ErrTimedOut) {
signaledToExit = false
}
case *api.Command_BackgroundScriptInstruction:
Expand Down
8 changes: 4 additions & 4 deletions internal/executor/shell.go
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"os/exec"
"os/signal"
"strings"
"syscall"
"time"
)
Expand All @@ -23,8 +24,6 @@ type ShellOutputWriter struct {
handler ShellOutputHandler
}

var TimeOutError = errors.New("timed out")

func (writer ShellOutputWriter) Write(bytes []byte) (int, error) {
return writer.handler(bytes)
}
Expand Down Expand Up @@ -61,15 +60,16 @@ func ShellCommandsAndWait(

select {
case <-ctx.Done():
handler([]byte("\nTimed out!"))
errorMessage := fmt.Sprintf("%v!", context.Cause(ctx))
handler([]byte("\n" + strings.ToUpper(errorMessage[:1]) + errorMessage[1:]))

processdumper.Dump()

if err = sc.kill(); err != nil {
handler([]byte(fmt.Sprintf("\nFailed to kill a timed out shell session: %s", err)))
}

return cmd, TimeOutError
return cmd, context.Cause(ctx)
case <-done:
var forcePiperClosure bool

Expand Down
2 changes: 1 addition & 1 deletion internal/executor/shell_subunix_test.go
Expand Up @@ -16,7 +16,7 @@ import (
// the shell spawned in ShellCommandsAndGetOutput() has been placed into, thus killing
// it's children processes.
func TestProcessGroupTermination(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
ctx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Second, ErrTimedOut)
defer cancel()

success, output := ShellCommandsAndGetOutput(ctx, []string{"sleep 86400 & echo target PID is $! ; sleep 60"}, nil)
Expand Down
4 changes: 2 additions & 2 deletions internal/executor/shell_unix_test.go
Expand Up @@ -112,7 +112,7 @@ func Test_ShellCommands_CustomWorkingDir_Unix(t *testing.T) {
}

func Test_ShellCommands_Timeout_Unix(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
ctx, cancel := context.WithTimeoutCause(context.Background(), 5*time.Second, ErrTimedOut)
defer cancel()

_, output := ShellCommandsAndGetOutput(ctx, []string{"sleep 60"}, nil)
Expand All @@ -124,7 +124,7 @@ func Test_ShellCommands_Timeout_Unix(t *testing.T) {
}

func TestChildrenProcessesAreCancelled(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
ctx, cancel := context.WithTimeoutCause(context.Background(), time.Second*5, ErrTimedOut)
defer cancel()

success, output := ShellCommandsAndGetOutput(ctx, []string{"sleep 60 & sleep 10"}, nil)
Expand Down
2 changes: 1 addition & 1 deletion internal/executor/shell_windows_test.go
Expand Up @@ -53,7 +53,7 @@ func TestMain(m *testing.M) {
// TestProcessGroupTermination ensures that we terminate all processes we've automatically
// tainted by assigning a job object to a shell spawned in ShellCommandsAndGetOutput().
func TestJobObjectTermination(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
ctx, cancel := context.WithTimeoutCause(context.Background(), 10*time.Second, ErrTimedOut)
defer cancel()

success, output := ShellCommandsAndGetOutput(ctx, []string{os.Args[0]},
Expand Down

0 comments on commit 28f4c3d

Please sign in to comment.