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 <codex@openai.com>

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)

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.

internal/app/run/runner.go

@@ -366,7 +366,6 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 	} else if t := task.Secrets["GITHUB_TOKEN"]; t != "" {
 		preset.Token = t
 	}
-	applyPullRequestTargetCheckoutContext(preset)
 
 	if actionsIDTokenRequestURL := taskContext["actions_id_token_request_url"].GetStringValue(); actionsIDTokenRequestURL != "" {
 		envs["ACTIONS_ID_TOKEN_REQUEST_URL"] = actionsIDTokenRequestURL
@@ -575,41 +574,6 @@ func postInternalCache(url, secret string, body map[string]string) error {
 	return nil
 }
 
-func applyPullRequestTargetCheckoutContext(preset *model.GithubContext) {
-	if preset == nil || preset.EventName != "pull_request_target" {
-		return
-	}
-	headSHA := nestedString(preset.Event, "pull_request", "head", "sha")
-	if headSHA == "" {
-		return
-	}
-	preset.Sha = headSHA
-	preset.Ref = headSHA
-	if headRef := nestedString(preset.Event, "pull_request", "head", "ref"); headRef != "" {
-		preset.HeadRef = headRef
-		preset.RefName = headRef
-	}
-}
-
-func nestedString(m map[string]any, keys ...string) string {
-	var current any = m
-	for _, key := range keys {
-		next, ok := current.(map[string]any)
-		if !ok {
-			return ""
-		}
-		current, ok = next[key]
-		if !ok {
-			return ""
-		}
-	}
-	value := current.(string)
-	if value == "" {
-		return ""
-	}
-	return value
-}
-
 func (r *Runner) RunningCount() int64 {
 	return r.runningCount.Load()
 }

internal/app/run/runner_test.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"testing"
 
-	"gitea.com/gitea/runner/act/model"
 	clientmocks "gitea.com/gitea/runner/internal/pkg/client/mocks"
 	"gitea.com/gitea/runner/internal/pkg/config"
 	"gitea.com/gitea/runner/internal/pkg/ver"
@@ -93,67 +92,6 @@ func TestNewRunnerInitializesLabelsAndEnvironment(t *testing.T) {
 	require.Nil(t, r.cacheHandler)
 }
 
-func TestApplyPullRequestTargetCheckoutContextUsesHeadSHA(t *testing.T) {
-	preset := &model.GithubContext{
-		EventName: "pull_request_target",
-		Sha:       "base-sha",
-		Ref:       "refs/heads/main",
-		RefName:   "main",
-		HeadRef:   "feature",
-		Event: map[string]any{
-			"pull_request": map[string]any{
-				"head": map[string]any{
-					"sha": "head-sha",
-					"ref": "contributor-branch",
-				},
-			},
-		},
-	}
-
-	applyPullRequestTargetCheckoutContext(preset)
-
-	require.Equal(t, "head-sha", preset.Sha)
-	require.Equal(t, "head-sha", preset.Ref)
-	require.Equal(t, "contributor-branch", preset.RefName)
-	require.Equal(t, "contributor-branch", preset.HeadRef)
-}
-
-func TestApplyPullRequestTargetCheckoutContextNoOpsWithoutHeadSHA(t *testing.T) {
-	preset := &model.GithubContext{
-		EventName: "pull_request_target",
-		Sha:       "base-sha",
-		Ref:       "refs/heads/main",
-		RefName:   "main",
-		Event:     map[string]any{},
-	}
-
-	applyPullRequestTargetCheckoutContext(preset)
-
-	require.Equal(t, "base-sha", preset.Sha)
-	require.Equal(t, "refs/heads/main", preset.Ref)
-	require.Equal(t, "main", preset.RefName)
-}
-
-func TestApplyPullRequestTargetCheckoutContextNoOpsForOtherEvents(t *testing.T) {
-	preset := &model.GithubContext{
-		EventName: "pull_request",
-		Sha:       "merge-sha",
-		Ref:       "refs/pull/1/merge",
-		Event: map[string]any{
-			"pull_request": map[string]any{
-				"head": map[string]any{
-					"sha": "head-sha",
-				},
-			},
-		},
-	}
-
-	applyPullRequestTargetCheckoutContext(preset)
-
-	require.Equal(t, "merge-sha", preset.Sha)
-	require.Equal(t, "refs/pull/1/merge", preset.Ref)
-}
-
 func taskWithDefaultActionsURL(url string) *runnerv1.Task {
 	return &runnerv1.Task{
 		Context: &structpb.Struct{