From 26f9fb12af1063f2e2453eb291bb8ee747fe77d5 Mon Sep 17 00:00:00 2001 From: bircni Date: Fri, 24 Jul 2026 16:36:09 +0000 Subject: [PATCH] fix: clean service containers after failed job setup (#1066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://gitea.com/gitea/runner/issues/659 Docker teardown was chained with `Then`, which stops on the previous step's error and on a cancelled context, so the first failure orphaned everything downstream. A volume that is still in use, or a flaky daemon, left the service containers and the job network behind — matching the reports in the issue, where the leaks show up after failed or overloaded jobs. Cleanup is now straight-line and best-effort: every step runs regardless of what failed before it, and the errors are joined. Two things follow from that: - Service containers are removed before the job volumes, since a service can hold one via `--volumes-from` in its options. The network stays last, once every container has detached. - Only job container and volume errors are returned. Service and network errors are logged, as before, because `stopJobContainer()` also runs as the pre-flight step of job start, where a network cleanup error must not abort the job. The nil check on `rc.JobContainer` is kept, but it is not the leak reporters are hitting: `container.NewContainer` never returns nil in the docker build, so cleanup with no job container is only reachable in the `WITHOUT_DOCKER` stub. Addresses https://gitea.com/gitea/runner/pulls/1066#issuecomment-1239073. Not covered here: the `buildx_buildkit_*` volumes reported in the issue are created by buildx inside the job, not by the runner, so no runner-side teardown removes them. --------- Co-authored-by: silverwind <2021+silverwind@noreply.gitea.com> Co-authored-by: silverwind Reviewed-on: https://gitea.com/gitea/runner/pulls/1066 Reviewed-by: silverwind <2021+silverwind@noreply.gitea.com> --- act/runner/run_context.go | 67 ++++++++++++++++++---------------- act/runner/run_context_test.go | 41 +++++++++++++++++++++ 2 files changed, 77 insertions(+), 31 deletions(-) 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.