fix: skip service containers with an empty image (#1074)

GitHub Actions skips services whose image evaluates to an empty string, enabling conditional services via expressions like `image: ${{ matrix.image || '' }}`. Here such a service failed the job at `docker create`. Skip them before container creation, using GitHub's log message verbatim, and add a regression test.

Reviewed-on: https://gitea.com/gitea/runner/pulls/1074
Reviewed-by: Nicolas <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>

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
@@ -380,6 +379,13 @@ func (rc *RunContext) startJobContainer() common.Executor {
 
 		// add service containers
 		for serviceID, spec := range rc.Run.Job().Services {
+			// GitHub compatibility: skip services whose image evaluates to an
+			// empty string, enabling conditional services via expressions
+			serviceImage := rc.ExprEval.Interpolate(ctx, spec.Image)
+			if serviceImage == "" {
+				logger.Infof("The service '%s' will not be started because the container definition has an empty image.", serviceID)
+				continue
+			}
 			// interpolate env
 			interpolatedEnvs := make(map[string]string, len(spec.Env))
 			for k, v := range spec.Env {
@@ -418,7 +424,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			c := container.NewContainer(&container.NewContainerInput{
 				Name:           serviceContainerName,
 				WorkingDir:     ext.ToContainerPath(rc.Config.Workdir),
-				Image:          rc.ExprEval.Interpolate(ctx, spec.Image),
+				Image:          serviceImage,
 				Username:       username,
 				Password:       password,
 				Cmd:            interpolatedCmd,
@@ -441,7 +447,37 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			rc.ServiceContainers = append(rc.ServiceContainers, c)
 		}
 
-		rc.cleanUpJobContainer = rc.cleanupJobContainer(logger, networkName, createAndDeleteNetwork)
+		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
+		}
 
 		// For Gitea, `jobContainerNetwork` should be the same as `networkName`
 		jobContainerNetwork := networkName
@@ -496,40 +532,6 @@ 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,7 +14,6 @@ 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"
 
@@ -364,21 +363,6 @@ 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.

act/runner/runner_test.go

@@ -303,6 +303,7 @@ func TestRunEvent(t *testing.T) {
 		// services
 		{workdir, "services", "push", "", platforms, secrets},
 		{workdir, "services-with-container", "push", "", platforms, secrets},
+		{workdir, "services-empty-image", "push", "", platforms, secrets},
 
 		// local remote action overrides
 		{workdir, "local-remote-action-overrides", "push", "", platforms, secrets},

act/runner/testdata/services-empty-image/push.yml

@@ -0,0 +1,10 @@
+name: services-empty-image
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    services:
+      db:
+        image: ${{ false && 'postgres:16' || '' }}
+    steps:
+      - run: echo "empty-image service was skipped"

go.mod

@@ -29,6 +29,7 @@ require (
 	github.com/opencontainers/selinux v1.15.1
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.23.2
+	github.com/prometheus/client_model v0.6.2
 	github.com/rhysd/actionlint v1.7.12
 	github.com/sirupsen/logrus v1.9.4
 	github.com/spf13/cobra v1.10.2
@@ -37,7 +38,7 @@ require (
 	github.com/timshannon/bolthold v0.0.0-20240314194003-30aac6950928
 	go.etcd.io/bbolt v1.5.0
 	go.yaml.in/yaml/v4 v4.0.0-rc.3
-	golang.org/x/sys v0.46.0
+	golang.org/x/sys v0.47.0
 	golang.org/x/term v0.44.0
 	google.golang.org/protobuf v1.36.11
 	gotest.tools/v3 v3.5.2
@@ -84,7 +85,6 @@ require (
 	github.com/opencontainers/go-digest v1.0.0 // indirect
 	github.com/pjbgf/sha1cd v0.6.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	github.com/prometheus/client_model v0.6.2 // indirect
 	github.com/prometheus/common v0.66.1 // indirect
 	github.com/prometheus/procfs v0.17.0 // indirect
 	github.com/robfig/cron/v3 v3.0.1 // indirect

go.sum

@@ -262,6 +262,8 @@ golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw=
 golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
+golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs=
+golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc=
 golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y=

internal/pkg/config/config.example.yaml

@@ -110,6 +110,11 @@ cache:
   dir: ""
   # Outbound IP or hostname that job containers use to reach this runner's cache server.
   # Leave empty to detect automatically. 0.0.0.0 is not valid here.
+  # If the runner itself runs in Docker, automatic detection can choose an
+  # address on the runner container's network that job containers cannot reach
+  # when the runner creates a separate per-job network. In that case, set this
+  # to a hostname/IP reachable from job containers, and set port to a fixed
+  # published port or put the job containers on a shared Docker network.
   # Ignored when external_server is set.
   host: ""
   # Port for the built-in cache server. 0 picks a random free port.
@@ -133,6 +138,8 @@ container:
   # Specifies the network to which the container will connect.
   # Could be host, bridge or the name of a custom network.
   # If it's empty, runner will create a network automatically.
+  # For dockerized runners using the built-in cache server, a custom shared
+  # network can be required so job containers can reach cache.host/cache.port.
   # Deprecated: `network_mode` is still accepted for old configs; use `network` instead.
   network: ""
   # network_create_options only apply when `network` is left empty and the runner