fix: continue docker cleanup after a failed step

Teardown was built from `Then`, which stops on the previous step's error and
on a cancelled context. A volume that is still in use, or a flaky daemon, then
orphaned everything downstream: the service containers and the job network.

Replace the combinator chain with straight-line best-effort cleanup that runs
every step and joins the errors. Service containers are now removed before the
volumes, since a service can hold them via `--volumes-from`, and the network
stays last.

Co-Authored-By: Claude (Opus 4.8) <noreply@anthropic.com>

act/runner/run_context.go

@@ -32,7 +32,6 @@ import (
 
 	"github.com/docker/go-connections/nat"
 	"github.com/opencontainers/selinux/go-selinux"
-	"github.com/sirupsen/logrus"
 )
 
 // RunContext contains info about current job
@@ -441,7 +440,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			rc.ServiceContainers = append(rc.ServiceContainers, c)
 		}
 
-		rc.cleanUpJobContainer = rc.cleanupJobContainer(logger, networkName, createAndDeleteNetwork)
+		rc.cleanUpJobContainer = rc.cleanupJobResources(networkName, createAndDeleteNetwork)
 
 		// For Gitea, `jobContainerNetwork` should be the same as `networkName`
 		jobContainerNetwork := networkName
@@ -496,37 +495,38 @@ func (rc *RunContext) startJobContainer() common.Executor {
 	}
 }
 
-func (rc *RunContext) cleanupJobContainer(logger logrus.FieldLogger, networkName string, createAndDeleteNetwork bool) common.Executor {
+// cleanupJobResources removes everything the job created, continuing past failures.
+// Only job container and volume errors are returned, the rest are logged.
+func (rc *RunContext) cleanupJobResources(networkName string, createAndDeleteNetwork bool) common.Executor {
 	return func(ctx context.Context) error {
-		reuseJobContainer := func(ctx context.Context) bool {
+		logger := common.Logger(ctx)
-			return rc.Config.ReuseContainers
+		removeJobContainer := rc.JobContainer != nil && !rc.Config.ReuseContainers
-		}
 
-		cleanup := common.NewPipelineExecutor()
+		var errs []error
-		if rc.JobContainer != nil {
+		if removeJobContainer {
-			cleanup = rc.JobContainer.Remove().IfNot(reuseJobContainer).
+			errs = append(errs, rc.JobContainer.Remove()(ctx))
-				Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName(), false)).IfNot(reuseJobContainer).
-				Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName()+"-env", false)).IfNot(reuseJobContainer)
 		}
-		return cleanup.Then(func(ctx context.Context) error {
+		if len(rc.ServiceContainers) > 0 {
-			if len(rc.ServiceContainers) > 0 {
+			logger.Infof("Cleaning up services for job %s", rc.JobName)
-				logger.Infof("Cleaning up services for job %s", rc.JobName)
+			if err := rc.stopServiceContainers()(ctx); err != nil {
-				if err := rc.stopServiceContainers()(ctx); err != nil {
+				logger.Errorf("Error while cleaning services: %v", err)
-					logger.Errorf("Error while cleaning services: %v", err)
-				}
 			}
-			if createAndDeleteNetwork {
+		}
-				// clean network if it has been created by act
+		if removeJobContainer {
-				// if using service containers
+			// after the containers using them, services can hold these via `--volumes-from`
-				// it means that the network to which containers are connecting is created by `runner`,
+			name := rc.jobContainerName()
-				// so, we should remove the network at last.
+			errs = append(errs,
-				logger.Infof("Cleaning up network for job %s, and network name is: %s", rc.JobName, networkName)
+				container.NewDockerVolumeRemoveExecutor(name, false)(ctx),
-				if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil {
+				container.NewDockerVolumeRemoveExecutor(name+"-env", false)(ctx))
-					logger.Errorf("Error while cleaning network: %v", err)
+		}
-				}
+		if createAndDeleteNetwork {
+			// last, once every container has detached
+			logger.Infof("Cleaning up network for job %s, and network name is: %s", rc.JobName, networkName)
+			if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil {
+				logger.Errorf("Error while cleaning network: %v", err)
 			}
-			return nil
+		}
-		})(ctx)
+		return errors.Join(errs...)
 	}
 }
 

act/runner/run_context_test.go

@@ -7,6 +7,7 @@ package runner
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"runtime"
@@ -364,7 +365,7 @@ func TestRunContextValidVolumes(t *testing.T) {
 	assert.Len(t, rc.validVolumes(), len(got), "repeated calls must be stable, not accumulate")
 }
 
-func TestCleanupJobContainerCleansServicesWithoutJobContainer(t *testing.T) {
+func TestCleanupJobResourcesCleansServicesWithoutJobContainer(t *testing.T) {
 	service := &containerMock{}
 	service.On("Remove").Return(func(context.Context) error { return nil }).Once()
 	service.On("Close").Return(func(context.Context) error { return nil }).Once()
@@ -374,11 +375,36 @@ func TestCleanupJobContainerCleansServicesWithoutJobContainer(t *testing.T) {
 		ServiceContainers: []container.ExecutionsEnvironment{service},
 	}
 
-	err := rc.cleanupJobContainer(log.StandardLogger(), "external-network", false)(context.Background())
+	err := rc.cleanupJobResources("external-network", false)(context.Background())
 	require.NoError(t, err)
 	service.AssertExpectations(t)
 }
 
+// cleanup used to bail out on a previous step's error and on a cancelled context
+func TestCleanupJobResourcesContinuesAfterFailure(t *testing.T) {
+	t.Setenv("DOCKER_HOST", "unix:///nonexistent.sock")
+
+	jobContainer := &containerMock{}
+	jobContainer.On("Remove").Return(func(context.Context) error { return errors.New("removal failed") }).Once()
+	service := &containerMock{}
+	service.On("Remove").Return(func(context.Context) error { return nil }).Once()
+	service.On("Close").Return(func(context.Context) error { return nil }).Once()
+
+	rc := &RunContext{
+		Name:              "job",
+		Config:            &Config{},
+		Run:               &model.Run{Workflow: &model.Workflow{Name: "wf"}, JobID: "job"},
+		JobContainer:      jobContainer,
+		ServiceContainers: []container.ExecutionsEnvironment{service},
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	require.Error(t, rc.cleanupJobResources("job-network", true)(ctx))
+	jobContainer.AssertExpectations(t)
+	service.AssertExpectations(t)
+}
+
 // TestInterpolateOutputsIsPerMatrixCombo guards the matrix-output fix: combinations share one
 // *model.Job, so each must interpolate from its own pristine snapshot. Otherwise the first
 // combo's resolved value freezes the shared template and later combos can't resolve their own.