diff --git a/pipeline/backend/dummy/dummy.go b/pipeline/backend/dummy/dummy.go index f49939ed1..601a842d8 100644 --- a/pipeline/backend/dummy/dummy.go +++ b/pipeline/backend/dummy/dummy.go @@ -49,8 +49,18 @@ const ( stepStateStarted = "started" stepStateDone = "done" testServiceTimeout = 1 * time.Second + + // ExitCodeCanceled is the exit code returned when a step's context is + // canceled while it is sleeping. 130 matches the SIGINT shell convention + // (128 + signal 2) used by real container runtimes. + ExitCodeCanceled = 130 ) +// stepKey returns the kv-store key for a step's state. +func stepKey(taskUUID, stepUUID string) string { + return "task_" + taskUUID + "_step_" + stepUUID +} + // New returns a dummy backend. func New() backend_types.Backend { return &dummy{ @@ -94,7 +104,8 @@ func (e *dummy) StartStep(_ context.Context, step *backend_types.Step, taskUUID if !exist { return fmt.Errorf("expect env of workflow %s to exist but found none to destroy", taskUUID) } - stepState, stepExist := e.kv.Load(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID)) + key := stepKey(taskUUID, step.UUID) + stepState, stepExist := e.kv.Load(key) if stepExist { // Detect issues like https://github.com/woodpecker-ci/woodpecker/issues/3494 return fmt.Errorf("StartStep detected already started step '%s' (%s) in state: %s", step.Name, step.UUID, stepState) @@ -109,10 +120,31 @@ func (e *dummy) StartStep(_ context.Context, step *backend_types.Step, taskUUID return fmt.Errorf("expected step type '%s' but got '%s'", expectStepType, step.Type) } - e.kv.Store(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID), stepStateStarted) + e.kv.Store(key, stepStateStarted) return nil } +// canceledState returns the state for a step whose context was canceled. +func canceledState() *backend_types.State { + return &backend_types.State{ExitCode: ExitCodeCanceled, Exited: true} +} + +// sleepWithContext blocks for the given duration or until ctx is canceled. +// Returns true if canceled, false if the sleep completed normally. +func sleepWithContext(ctx context.Context, d time.Duration) (canceled bool) { + if ctx.Err() != nil { + return true + } + t := time.NewTimer(d) + defer t.Stop() + select { + case <-t.C: + return false + case <-ctx.Done(): + return true + } +} + func (e *dummy) WaitStep(ctx context.Context, step *backend_types.Step, taskUUID string) (*backend_types.State, error) { log.Trace().Str("taskUUID", taskUUID).Msgf("wait for step %s", step.Name) @@ -122,8 +154,10 @@ func (e *dummy) WaitStep(ctx context.Context, step *backend_types.Step, taskUUID return &backend_types.State{Error: err}, err } + key := stepKey(taskUUID, step.UUID) + // check state - stepState, stepExist := e.kv.Load(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID)) + stepState, stepExist := e.kv.Load(key) if !stepExist { err := fmt.Errorf("WaitStep expect step '%s' (%s) to be created but found none", step.Name, step.UUID) return &backend_types.State{Error: err}, err @@ -133,29 +167,28 @@ func (e *dummy) WaitStep(ctx context.Context, step *backend_types.Step, taskUUID return &backend_types.State{Error: err}, err } - // extend wait time logic if sleep, sleepExist := step.Environment[EnvKeyStepSleep]; sleepExist { toSleep, err := time.ParseDuration(sleep) if err != nil { err = fmt.Errorf("WaitStep fail to parse sleep duration: %w", err) return &backend_types.State{Error: err}, err } - time.Sleep(toSleep) - } else { - if step.Type == backend_types.StepTypeService { - select { - case <-time.NewTimer(testServiceTimeout).C: - err := fmt.Errorf("WaitStep fail due to timeout of service after 1 second") - return &backend_types.State{Error: err}, err - case <-ctx.Done(): - // context for service closed ... we can move forward - } - } else { - time.Sleep(time.Nanosecond) + if sleepWithContext(ctx, toSleep) { + e.kv.Store(key, stepStateDone) + return canceledState(), nil } + } else if step.Type == backend_types.StepTypeService { + if sleepWithContext(ctx, testServiceTimeout) { + // context for service closed — we can move forward + } else { + err := fmt.Errorf("WaitStep fail due to timeout of service after 1 second") + return &backend_types.State{Error: err}, err + } + } else { + time.Sleep(time.Nanosecond) } - e.kv.Store(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID), stepStateDone) + e.kv.Store(key, stepStateDone) oomKilled, _ := strconv.ParseBool(step.Environment[EnvKeyStepOOMKilled]) exitCode := 0 @@ -179,13 +212,15 @@ func (e *dummy) TailStep(_ context.Context, step *backend_types.Step, taskUUID s return nil, fmt.Errorf("expect env of workflow %s to exist but found none to destroy", taskUUID) } + key := stepKey(taskUUID, step.UUID) + // check state - stepState, stepExist := e.kv.Load(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID)) + stepState, stepExist := e.kv.Load(key) if !stepExist { - return nil, fmt.Errorf("WaitStep expect step '%s' (%s) to be created but found none", step.Name, step.UUID) + return nil, fmt.Errorf("TailStep expect step '%s' (%s) to be created but found none", step.Name, step.UUID) } if stepState != stepStateStarted { - return nil, fmt.Errorf("WaitStep expect step '%s' (%s) to be '%s' but it is: %s", step.Name, step.UUID, stepStateStarted, stepState) + return nil, fmt.Errorf("TailStep expect step '%s' (%s) to be '%s' but it is: %s", step.Name, step.UUID, stepStateStarted, stepState) } if tailShouldFail, _ := strconv.ParseBool(step.Environment[EnvKeyStepTailFail]); tailShouldFail { @@ -203,16 +238,20 @@ func (e *dummy) DestroyStep(_ context.Context, step *backend_types.Step, taskUUI return nil } + key := stepKey(taskUUID, step.UUID) + // check state - stepState, stepExist := e.kv.Load(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID)) + stepState, stepExist := e.kv.Load(key) if !stepExist { - return fmt.Errorf("WaitStep expect step '%s' (%s) to be created but found none", step.Name, step.UUID) + return fmt.Errorf("DestroyStep expect step '%s' (%s) to be created but found none", step.Name, step.UUID) } - if stepState != stepStateDone { - return fmt.Errorf("WaitStep expect step '%s' (%s) to be '%s' but it is: %s", step.Name, step.UUID, stepStateDone, stepState) + // Allow destroying a step in 'started' state: this happens when the + // workflow context is canceled before WaitStep completes. + if stepState != stepStateDone && stepState != stepStateStarted { + return fmt.Errorf("DestroyStep expect step '%s' (%s) to be '%s' or '%s' but it is: %s", step.Name, step.UUID, stepStateDone, stepStateStarted, stepState) } - e.kv.Delete(fmt.Sprintf("task_%s_step_%s", taskUUID, step.UUID)) + e.kv.Delete(key) return nil } diff --git a/pipeline/backend/dummy/dummy_test.go b/pipeline/backend/dummy/dummy_test.go index 90a3555b3..e3ef93ec6 100644 --- a/pipeline/backend/dummy/dummy_test.go +++ b/pipeline/backend/dummy/dummy_test.go @@ -15,10 +15,12 @@ package dummy_test import ( + "context" "io" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.woodpecker-ci.org/woodpecker/v3/pipeline/backend/dummy" "go.woodpecker-ci.org/woodpecker/v3/pipeline/backend/types" @@ -31,7 +33,7 @@ func TestSmalPipelineDummyRun(t *testing.T) { assert.True(t, dummyEngine.IsAvailable(ctx)) assert.EqualValues(t, "dummy", dummyEngine.Name()) _, err := dummyEngine.Load(ctx) - assert.NoError(t, err) + require.NoError(t, err) assert.Error(t, dummyEngine.SetupWorkflow(ctx, nil, dummy.WorkflowSetupFailUUID)) @@ -162,3 +164,37 @@ echo nein assert.NoError(t, dummyEngine.DestroyWorkflow(ctx, nil, workflowUUID)) }) } + +func TestWaitStepCanceledBySleep(t *testing.T) { + ctx, cancel := context.WithCancelCause(t.Context()) + + dummyEngine := dummy.New() + _, err := dummyEngine.Load(ctx) + require.NoError(t, err) + + const taskUUID = "cancel-task" + assert.NoError(t, dummyEngine.SetupWorkflow(ctx, nil, taskUUID)) + + step := &types.Step{ + Name: "slow-step", + UUID: "slow-uuid", + Type: types.StepTypeCommands, + Environment: map[string]string{ + dummy.EnvKeyStepSleep: "30s", + }, + } + assert.NoError(t, dummyEngine.StartStep(ctx, step, taskUUID)) + + // Cancel before WaitStep — the pre-select ctx.Err() check handles this deterministically. + cancel(nil) + + state, err := dummyEngine.WaitStep(ctx, step, taskUUID) + assert.NoError(t, err, "WaitStep should not return an error on cancellation") + assert.True(t, state.Exited, "step should be marked as exited") + assert.Equal(t, dummy.ExitCodeCanceled, state.ExitCode, + "canceled step must exit with code %d", dummy.ExitCodeCanceled) + + // DestroyStep must succeed even though the step was canceled mid-sleep. + assert.NoError(t, dummyEngine.DestroyStep(ctx, step, taskUUID)) + assert.NoError(t, dummyEngine.DestroyWorkflow(ctx, nil, taskUUID)) +} diff --git a/pipeline/runtime/runtime_test.go b/pipeline/runtime/runtime_test.go index fd7e2e577..0e9549541 100644 --- a/pipeline/runtime/runtime_test.go +++ b/pipeline/runtime/runtime_test.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -1154,3 +1155,71 @@ func TestWorkflowContextCancelWithServiceStep(t *testing.T) { assert.ErrorIs(t, err, pipeline_errors.ErrCancel) } + +// TestWorkflowCancelDuringStepSleep verifies that canceling the workflow context +// while a step is sleeping (via SLEEP env) causes the runtime to return ErrCancel +// promptly — without waiting the full sleep duration — and that subsequent stages +// are never executed. +// +// The tracer callback cancels the context the moment the first stage ("prepare") +// completes. The "slow" step uses a short sleep so that even if WaitStep enters +// the sleep select, the context cancellation unblocks it quickly. +// +// Note: we do not assert on the slow step's exit code here because Run() may +// return (via ctx.Done()) before the stage goroutine's WaitStep completes, +// causing DestroyWorkflow to clean up state that WaitStep still needs. The +// exit-code-130 behavior of a canceled sleep is verified at the backend unit +// level in TestWaitStepCanceledBySleep. +func TestWorkflowCancelDuringStepSleep(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancelCause(t.Context()) + + var prepareExited int + tracer := tracer_mocks.NewMockTracer(t) + tracer.On("Trace", mock.Anything).Run(func(args mock.Arguments) { + s, _ := args.Get(0).(*state.State) + if s == nil || s.CurrStep == nil { + return + } + // Cancel as soon as the first stage ("prepare") finishes. + if s.CurrStep.Name == "prepare" && s.CurrStepState.Exited { + prepareExited++ + if prepareExited >= 1 { + cancel(nil) + } + } + }).Return(nil).Maybe() + + r := New( + &backend_types.Config{ + Stages: []*backend_types.Stage{ + {Steps: []*backend_types.Step{ + cmdStep("prepare"), + }}, + {Steps: []*backend_types.Step{ + // Short sleep so the test doesn't hang if WaitStep enters the timer. + cmdStep("slow", func(s *backend_types.Step) { + s.Environment[dummy.EnvKeyStepSleep] = "100ms" + }), + }}, + {Steps: []*backend_types.Step{ + cmdStep("never-reached"), + }}, + }, + }, + dummy.New(), + WithTracer(tracer), + WithContext(ctx), + WithLogger(newTestLogger(t)), + ) + + err := r.Run(t.Context()) + assert.ErrorIs(t, err, pipeline_errors.ErrCancel, "canceled workflow must return ErrCancel") + + // Give the orphaned stage goroutine a moment to finish tracing (best effort). + time.Sleep(200 * time.Millisecond) + + assert.Nil(t, findFirstTraceByName(getTracerStates(tracer), "never-reached"), + "never-reached must not have been traced") +}