diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 04252fae..7060c762 100644 --- a/act/runner/run_context.go +++ b/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 { - return rc.Config.ReuseContainers - } + logger := common.Logger(ctx) + removeJobContainer := rc.JobContainer != nil && !rc.Config.ReuseContainers - cleanup := common.NewPipelineExecutor() - if rc.JobContainer != nil { - cleanup = rc.JobContainer.Remove().IfNot(reuseJobContainer). - Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName(), false)).IfNot(reuseJobContainer). - Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName()+"-env", false)).IfNot(reuseJobContainer) + var errs []error + if removeJobContainer { + errs = append(errs, rc.JobContainer.Remove()(ctx)) } - return cleanup.Then(func(ctx context.Context) error { - if len(rc.ServiceContainers) > 0 { - logger.Infof("Cleaning up services for job %s", rc.JobName) - if err := rc.stopServiceContainers()(ctx); err != nil { - logger.Errorf("Error while cleaning services: %v", err) - } + if len(rc.ServiceContainers) > 0 { + logger.Infof("Cleaning up services for job %s", rc.JobName) + if err := rc.stopServiceContainers()(ctx); err != nil { + logger.Errorf("Error while cleaning services: %v", err) } - if createAndDeleteNetwork { - // clean network if it has been created by act - // if using service containers - // it means that the network to which containers are connecting is created by `runner`, - // so, we should remove the network at last. - 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) - } + } + if removeJobContainer { + // after the containers using them, services can hold these via `--volumes-from` + name := rc.jobContainerName() + errs = append(errs, + container.NewDockerVolumeRemoveExecutor(name, false)(ctx), + container.NewDockerVolumeRemoveExecutor(name+"-env", false)(ctx)) + } + 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...) } } diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index ede0f781..b3c82f59 100644 --- a/act/runner/run_context_test.go +++ b/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.