fix: classify a cancelled step as an interruption, not a failure (#1095)

`reportStepError` reported every step error as FAILURE, including a `context.Canceled` from a docker file-command read cancelled at job finalization — non-deterministic red CI. Classify `context.Canceled` as an interruption instead (deferring to the job context), so a genuine cancel reports cancelled and a stray teardown cancellation is ignored, never a failure.

---------

Co-authored-by: bircni <bircni@icloud.com>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1095
Reviewed-by: bircni <bircni@icloud.com>

.gitea/workflows/pull-pr-title.yml

@@ -19,7 +19,7 @@ jobs:
     timeout-minutes: 5
     steps:
     - uses: actions/checkout@v7
-    - uses: actions/setup-node@v6
+    - uses: actions/setup-node@v7
       with:
         node-version: 24
     - run: make lint-pr-title

act/container/docker_run.go

@@ -376,6 +376,11 @@ func (cr *containerReference) find() common.Executor {
 	}
 }
 
+// isContainerGone reports whether a failed remove still left the container gone (NotFound or Conflict).
+func isContainerGone(err error) bool {
+	return cerrdefs.IsNotFound(err) || cerrdefs.IsConflict(err)
+}
+
 func (cr *containerReference) remove() common.Executor {
 	return func(ctx context.Context) error {
 		if cr.id == "" {
@@ -387,7 +392,7 @@ func (cr *containerReference) remove() common.Executor {
 			RemoveVolumes: true,
 			Force:         true,
 		})
-		if err != nil {
+		if err != nil && !isContainerGone(err) {
 			logger.Error(fmt.Errorf("failed to remove container: %w", err))
 		}
 

act/container/docker_run_test.go

@@ -116,6 +116,11 @@ func (m *mockDockerClient) ContainerList(ctx context.Context, opts mobyclient.Co
 	return args.Get(0).(mobyclient.ContainerListResult), args.Error(1)
 }
 
+func (m *mockDockerClient) ContainerRemove(ctx context.Context, id string, opts mobyclient.ContainerRemoveOptions) (mobyclient.ContainerRemoveResult, error) {
+	args := m.Called(ctx, id, opts)
+	return args.Get(0).(mobyclient.ContainerRemoveResult), args.Error(1)
+}
+
 type endlessReader struct {
 	io.Reader
 }
@@ -381,6 +386,40 @@ func TestDockerCopyTarStreamErrorInMkdir(t *testing.T) {
 	client.AssertExpectations(t)
 }
 
+// A remove that raced the daemon's AutoRemove teardown is not a failure and must not
+// be logged as one.
+func TestRemoveIgnoresAutoRemoveRace(t *testing.T) {
+	removeOpts := mobyclient.ContainerRemoveOptions{RemoveVolumes: true, Force: true}
+	for _, tc := range []struct {
+		name     string
+		err      error
+		wantLogs bool
+	}{
+		{name: "removal in progress", err: cerrdefs.ErrConflict.WithMessage("removal of container abc is already in progress")},
+		{name: "already removed", err: cerrdefs.ErrNotFound.WithMessage("No such container: abc")},
+		{name: "removed cleanly", err: nil},
+		{name: "real failure", err: errors.New("driver failed to remove root filesystem"), wantLogs: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			logger, hook := test.NewNullLogger()
+			ctx := common.WithLogger(context.Background(), logger)
+			client := &mockDockerClient{}
+			client.On("ContainerRemove", ctx, "abc", removeOpts).Return(mobyclient.ContainerRemoveResult{}, tc.err)
+			cr := &containerReference{id: "abc", cli: client}
+
+			require.NoError(t, cr.remove()(ctx))
+			assert.Empty(t, cr.id)
+
+			if tc.wantLogs {
+				assert.Len(t, hook.AllEntries(), 1)
+			} else {
+				assert.Empty(t, hook.AllEntries())
+			}
+			client.AssertExpectations(t)
+		})
+	}
+}
+
 // find() must drop a stale cached id so later Copy/Exec don't hit the
 // daemon with a torn-down container.
 func TestFindRevalidatesStaleID(t *testing.T) {

act/runner/action.go

@@ -405,7 +405,7 @@ func execAsDocker(ctx context.Context, step actionStep, actionName, actionDir, b
 		stepContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
 		stepContainer.Start(true),
 	).Finally(
-		stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
+		stepContainer.Remove().IfBool(!rc.Config.ReuseContainers && !rc.Config.AutoRemove),
 	).Finally(stepContainer.Close())(ctx)
 }
 

act/runner/action_test.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 type closerMock struct {
@@ -150,6 +151,44 @@ runs:
 	}
 }
 
+// With AutoRemove the daemon reaps the container on exit, so act must not remove it afterwards.
+func TestExecAsDockerAutoRemove(t *testing.T) {
+	orig := ContainerNewContainer
+	defer func() { ContainerNewContainer = orig }()
+
+	for _, tc := range []struct {
+		autoRemove bool
+		removes    int
+	}{
+		{false, 2}, // stale + post-run
+		{true, 1},  // post-run skipped
+	} {
+		cm := &containerMock{}
+		ContainerNewContainer = func(*container.NewContainerInput) container.ExecutionsEnvironment { return cm }
+
+		step := &stepActionRemote{
+			Step: &model.Step{ID: "1", Uses: "org/action@v1"},
+			RunContext: &RunContext{
+				Config:       &Config{AutoRemove: tc.autoRemove},
+				Run:          &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}},
+				JobContainer: cm,
+			},
+			action: &model.Action{Runs: model.ActionRuns{Using: "docker", Image: "docker://node:14"}},
+		}
+
+		removes := 0
+		cm.On("Pull", false).Return(func(context.Context) error { return nil })
+		cm.On("Remove").Return(func(context.Context) error { removes++; return nil })
+		cm.On("Create", []string(nil), []string(nil)).Return(func(context.Context) error { return nil })
+		cm.On("Start", true).Return(func(context.Context) error { return nil })
+		cm.On("Close").Return(func(context.Context) error { return nil })
+
+		require.NoError(t, execAsDocker(context.Background(), step, "action", t.TempDir(), t.TempDir(), false))
+		cm.AssertExpectations(t)
+		assert.Equal(t, tc.removes, removes)
+	}
+}
+
 func TestActionRunner(t *testing.T) {
 	table := []struct {
 		name        string

act/runner/cancellation_test.go

@@ -283,3 +283,30 @@ func TestPostStepsContextDeadlinePreservesJobError(t *testing.T) {
 	require.NoError(t, postCtx.Err(), "post context must not carry the expired deadline")
 	assert.ErrorIs(t, common.JobError(postCtx), assert.AnError, "the timeout job error must be preserved")
 }
+
+// reportStepError must treat a context.Canceled (e.g. a teardown-cancelled read) as an
+// interruption, never a job failure.
+func TestReportStepErrorTreatsCancelAsInterruption(t *testing.T) {
+	rc := &RunContext{}
+
+	// stray read cancellation while the job context is live: ignored, not a failure
+	live := common.WithJobErrorContainer(context.Background())
+	reportStepError(live, rc, context.Canceled)
+	require.NoError(t, common.JobError(live))
+	assert.False(t, rc.jobFailed)
+	assert.False(t, rc.jobCancelled)
+
+	// genuine job cancellation: recorded as cancelled, still not a failure
+	cancelled, cancel := context.WithCancel(common.WithJobErrorContainer(context.Background()))
+	cancel()
+	reportStepError(cancelled, rc, context.Canceled)
+	require.NoError(t, common.JobError(cancelled))
+	assert.False(t, rc.jobFailed)
+	assert.True(t, rc.jobCancelled)
+
+	// a real error still fails the job
+	failed := common.WithJobErrorContainer(context.Background())
+	reportStepError(failed, rc, assert.AnError)
+	require.ErrorIs(t, common.JobError(failed), assert.AnError)
+	assert.True(t, rc.jobFailed)
+}

act/runner/job_executor.go

@@ -10,6 +10,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -56,9 +57,15 @@ type jobInfo interface {
 	result(result string)
 }
 
-// reportStepError emits the GitHub Actions ##[error] annotation and records
+// reportStepError records a step error so the job is reported failed — except a
-// the error against the job so the job is reported as failed.
+// cancellation, which is an interruption, not a failure.
 func reportStepError(ctx context.Context, rc *RunContext, err error) {
+	if errors.Is(err, context.Canceled) {
+		// Defer to the job context: a genuine cancel reports cancelled, a stray teardown
+		// cancellation on a live ctx is ignored — never a step FAILURE.
+		rc.markInterrupted(ctx.Err())
+		return
+	}
 	common.Logger(ctx).Errorf("##[error]%v", err)
 	common.SetJobError(ctx, err)
 	rc.markFailed()

act/runner/step_docker.go

@@ -85,7 +85,7 @@ func (sd *stepDocker) runUsesContainer() common.Executor {
 			stepContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
 			stepContainer.Start(true),
 		).Finally(
-			stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
+			stepContainer.Remove().IfBool(!rc.Config.ReuseContainers && !rc.Config.AutoRemove),
 		).Finally(stepContainer.Close())(ctx)
 	}
 }

act/runner/step_docker_test.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 func TestStepDockerMain(t *testing.T) {
@@ -118,6 +119,43 @@ func TestStepDockerMain(t *testing.T) {
 	cm.AssertExpectations(t)
 }
 
+// With AutoRemove the daemon reaps the container on exit, so act must not remove it afterwards.
+func TestStepDockerAutoRemove(t *testing.T) {
+	orig := ContainerNewContainer
+	defer func() { ContainerNewContainer = orig }()
+
+	for _, tc := range []struct {
+		autoRemove bool
+		removes    int
+	}{
+		{false, 2}, // stale + post-run
+		{true, 1},  // post-run skipped
+	} {
+		cm := &containerMock{}
+		ContainerNewContainer = func(*container.NewContainerInput) container.ExecutionsEnvironment { return cm }
+
+		sd := &stepDocker{
+			RunContext: &RunContext{
+				Config:       &Config{AutoRemove: tc.autoRemove},
+				Run:          &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}},
+				JobContainer: cm,
+			},
+			Step: &model.Step{ID: "1", Uses: "docker://node:14"},
+		}
+
+		removes := 0
+		cm.On("Pull", false).Return(func(context.Context) error { return nil })
+		cm.On("Remove").Return(func(context.Context) error { removes++; return nil })
+		cm.On("Create", []string(nil), []string(nil)).Return(func(context.Context) error { return nil })
+		cm.On("Start", true).Return(func(context.Context) error { return nil })
+		cm.On("Close").Return(func(context.Context) error { return nil })
+
+		require.NoError(t, sd.runUsesContainer()(context.Background()))
+		cm.AssertExpectations(t)
+		assert.Equal(t, tc.removes, removes)
+	}
+}
+
 func TestStepDockerNewStepContainerAllocatePTY(t *testing.T) {
 	for _, tc := range []struct {
 		name     string