diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 1f85029f..9b00c287 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -454,37 +454,7 @@ func (rc *RunContext) startJobContainer() common.Executor { rc.ServiceContainers = append(rc.ServiceContainers, c) } - rc.cleanUpJobContainer = func(ctx context.Context) error { - reuseJobContainer := func(ctx context.Context) bool { - return rc.Config.ReuseContainers - } - - if rc.JobContainer != nil { - return rc.JobContainer.Remove().IfNot(reuseJobContainer). - Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName(), false)).IfNot(reuseJobContainer). - Then(container.NewDockerVolumeRemoveExecutor(rc.jobContainerName()+"-env", false)).IfNot(reuseJobContainer). - 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 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) - } - } - return nil - })(ctx) - } - return nil - } + rc.cleanUpJobContainer = rc.cleanupJobResources(networkName, createAndDeleteNetwork) // For Gitea, `jobContainerNetwork` should be the same as `networkName` jobContainerNetwork := networkName @@ -539,6 +509,41 @@ func (rc *RunContext) startJobContainer() 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 { + logger := common.Logger(ctx) + removeJobContainer := rc.JobContainer != nil && !rc.Config.ReuseContainers + + var errs []error + if removeJobContainer { + errs = append(errs, rc.JobContainer.Remove()(ctx)) + } + 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 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 errors.Join(errs...) + } +} + func (rc *RunContext) execJobContainer(cmd []string, env map[string]string, user, workdir string) common.Executor { //nolint:unparam // pre-existing issue from nektos/act return func(ctx context.Context) error { return rc.JobContainer.Exec(cmd, env, user, workdir)(ctx) diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 46b67a8f..5cbb0d0a 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" @@ -451,6 +452,46 @@ func TestRunContextValidVolumes(t *testing.T) { assert.Len(t, rc.validVolumes(), len(got), "repeated calls must be stable, not accumulate") } +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() + + rc := &RunContext{ + Config: &Config{}, + ServiceContainers: []container.ExecutionsEnvironment{service}, + } + + 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.