From 5a98252565a4a7397de7be055aefea7c39b2da34 Mon Sep 17 00:00:00 2001 From: bircni Date: Fri, 3 Jul 2026 21:54:46 +0200 Subject: [PATCH] fix: clean service containers after failed job setup Ensure service containers are still stopped when job setup fails before the job container has been assigned, so runner-created resources do not linger after failed starts. Fixes: https://gitea.com/gitea/runner/issues/659 Co-Authored-By: GPT-5 Codex --- act/runner/run_context.go | 67 ++++++++++++++++++---------------- act/runner/run_context_test.go | 16 ++++++++ 2 files changed, 52 insertions(+), 31 deletions(-) diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 68998801..04252fae 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -32,6 +32,7 @@ import ( "github.com/docker/go-connections/nat" "github.com/opencontainers/selinux/go-selinux" + "github.com/sirupsen/logrus" ) // RunContext contains info about current job @@ -440,37 +441,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.cleanupJobContainer(logger, networkName, createAndDeleteNetwork) // For Gitea, `jobContainerNetwork` should be the same as `networkName` jobContainerNetwork := networkName @@ -525,6 +496,40 @@ func (rc *RunContext) startJobContainer() common.Executor { } } +func (rc *RunContext) cleanupJobContainer(logger logrus.FieldLogger, networkName string, createAndDeleteNetwork bool) common.Executor { + return func(ctx context.Context) error { + reuseJobContainer := func(ctx context.Context) bool { + return 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) + } + 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 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) + } +} + 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 8b2563af..ede0f781 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -14,6 +14,7 @@ import ( "testing" "gitea.com/gitea/runner/act/common" + "gitea.com/gitea/runner/act/container" "gitea.com/gitea/runner/act/exprparser" "gitea.com/gitea/runner/act/model" @@ -363,6 +364,21 @@ func TestRunContextValidVolumes(t *testing.T) { assert.Len(t, rc.validVolumes(), len(got), "repeated calls must be stable, not accumulate") } +func TestCleanupJobContainerCleansServicesWithoutJobContainer(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.cleanupJobContainer(log.StandardLogger(), "external-network", false)(context.Background()) + require.NoError(t, err) + 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.