fix: job container, daemon startup, action cache and exec fixes (#1294)

- Keep `GITHUB_ENV`, `GITHUB_OUTPUT`, `GITHUB_STATE` and `GITHUB_PATH` when a job times out or is cancelled, so post steps still get their state - Retry the startup Declare while Gitea is unreachable instead of exiting, still failing fast on rejected registrations, auth and misconfiguration - Reuse a local image when a platform variant such as `linux/arm/v7` is requested instead of pulling again - Read `PATH` from the created job container so cross-architecture jobs get tool paths for the right architecture - Extract action copies relative to the act volume so they never cross an image's `/var/run` symlink, which rootless Podman rejects - Keep the job container's entrypoint when container options set `--entrypoint`, warning instead of appending it to the keep-alive command - Lock action cache entries across runner processes that share a cache directory - `exec`: omit `GITHUB_SERVER_URL` and `GITHUB_API_URL` without `--gitea-instance` instead of exporting a bare `https://`, and read owner and repository from any origin remote Reviewed-on: https://gitea.com/gitea/runner/pulls/1294 Reviewed-by: bircni <[email protected]> Co-authored-by: silverwind <[email protected]>
2026-11-04 22:37:55 +00:00 · 2026-10-10 18:11:03 +00:00
1 parent b741335222
commit ad9eeb668a
@@ -10,8 +10,10 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"net/url"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -29,8 +31,6 @@ import (
 var (
 	codeCommitHTTPRegex = regexp.MustCompile(`^https?://git-codecommit\.(.+)\.amazonaws.com/v1/repos/(.+)$`)
 	codeCommitSSHRegex  = regexp.MustCompile(`ssh://git-codecommit\.(.+)\.amazonaws.com/v1/repos/(.+)$`)
-	githubHTTPRegex     = regexp.MustCompile(`^https?://.*github.com.*/(.+)/(.+?)(?:.git)?$`)
-	githubSSHRegex      = regexp.MustCompile(`github.com[:/](.+)/(.+?)(?:.git)?$`)
 	hexRefRegex         = regexp.MustCompile(`^[0-9a-fA-F]+$`)

 	LocalEnvVars = []string{ // `git rev-parse --local-env-vars`, inherited ones point git at another repository
@@ -44,12 +44,29 @@ var (
 	ErrShortRef = errors.New("short SHA references are not supported")
 )

-// AcquireCloneLock returns an unlock function after locking the per-directory mutex for dir.
-// Only concurrent operations targeting the same directory are serialized; clones into different directories run in parallel.
-// Callers reading files inside dir (e.g. tarring a checked-out action into a job container) must hold this lock too,
-// otherwise a concurrent NewGitCloneExecutor on the same dir can mutate the worktree mid-read.
-func AcquireCloneLock(dir string) func() {
-	return cloneLocks.Lock(dir)
+// AcquireCloneLock serializes access to dir across runner processes, or within this one where no lock file can be created. Readers of dir must hold it too.
+func AcquireCloneLock(ctx context.Context, dir string) (func(), error) {
+	dir = filepath.Clean(dir)
+	if runtime.GOOS == "plan9" {
+		return cloneLocks.Lock(dir), nil
+	}
+	if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil {
+		return nil, err
+	}
+	for {
+		release, err := lock.TryLock(dir)
+		if err == nil {
+			return func() { _ = release() }, nil
+		}
+		if !errors.Is(err, lock.ErrLocked) {
+			return cloneLocks.Lock(dir), nil
+		}
+		select {
+		case <-ctx.Done():
+			return nil, ctx.Err()
+		case <-time.After(50 * time.Millisecond):
+		}
+	}
 }

 type Error struct {
@@ -109,13 +126,12 @@ func FindGitRef(ctx context.Context, file string) (string, error) {
 }

 // FindGithubRepo get the repo
-func FindGithubRepo(ctx context.Context, file, githubInstance string) (string, error) {
+func FindGithubRepo(ctx context.Context, file string) (string, error) {
 	url, err := findGitRemoteURL(ctx, file)
 	if err != nil {
 		return "", err
 	}
-	_, slug := findGitSlug(url, githubInstance)
-	return slug, nil
+	return findGitSlug(url)
 }

 func findGitRemoteURL(ctx context.Context, file string) (string, error) {
@@ -130,25 +146,25 @@ func findGitRemoteURL(ctx context.Context, file string) (string, error) {
 	return url, nil
 }

-func findGitSlug(url, githubInstance string) (string, string) {
-	if matches := codeCommitHTTPRegex.FindStringSubmatch(url); matches != nil {
-		return "CodeCommit", matches[2]
-	} else if matches := codeCommitSSHRegex.FindStringSubmatch(url); matches != nil {
-		return "CodeCommit", matches[2]
-	} else if matches := githubHTTPRegex.FindStringSubmatch(url); matches != nil {
-		return "GitHub", fmt.Sprintf("%s/%s", matches[1], matches[2])
-	} else if matches := githubSSHRegex.FindStringSubmatch(url); matches != nil {
-		return "GitHub", fmt.Sprintf("%s/%s", matches[1], matches[2])
-	} else if githubInstance != "github.com" {
-		gheHTTPRegex := regexp.MustCompile(fmt.Sprintf(`^https?://%s/(.+)/(.+?)(?:.git)?$`, githubInstance))
-		gheSSHRegex := regexp.MustCompile(githubInstance + "[:/](.+)/(.+?)(?:.git)?$")
-		if matches := gheHTTPRegex.FindStringSubmatch(url); matches != nil {
-			return "GitHubEnterprise", fmt.Sprintf("%s/%s", matches[1], matches[2])
-		} else if matches := gheSSHRegex.FindStringSubmatch(url); matches != nil {
-			return "GitHubEnterprise", fmt.Sprintf("%s/%s", matches[1], matches[2])
-		}
+func findGitSlug(remoteURL string) (string, error) {
+	if matches := codeCommitHTTPRegex.FindStringSubmatch(remoteURL); matches != nil {
+		return matches[2], nil
+	} else if matches := codeCommitSSHRegex.FindStringSubmatch(remoteURL); matches != nil {
+		return matches[2], nil
 	}
-	return "", url
+	if host, repoPath, ok := strings.Cut(remoteURL, ":"); ok && filepath.VolumeName(remoteURL) == "" && !strings.Contains(host, "/") && !strings.HasPrefix(repoPath, "//") {
+		remoteURL = "ssh://" + host + "/" + repoPath // scp-like [user@]host:path
+	}
+	parsed, err := url.Parse(remoteURL)
+	if err != nil {
+		return "", errors.New("cannot parse the origin remote URL")
+	}
+	owner, repo := path.Split(strings.TrimSuffix(strings.Trim(parsed.Path, "/"), ".git"))
+	owner = path.Base(owner)
+	if parsed.Host == "" || repo == "" || owner == "." || owner == "/" {
+		return "", errors.New("cannot determine owner and repository from the origin remote URL")
+	}
+	return owner + "/" + repo, nil
 }

 // NewGitCloneExecutorInput the input for the NewGitCloneExecutor
@@ -182,6 +198,9 @@ func CloneIfRequired(ctx context.Context, input NewGitCloneExecutorInput, logger
 	if err == nil && origin == input.URL {
 		return true, nil
 	}
+	if ctxErr := ctx.Err(); ctxErr != nil { // a cancelled origin lookup must not discard a valid clone
+		return false, ctxErr
+	}
 	if err == nil {
 		logger.Debugf("Removing cached clone at %s because origin URL changed from %s to %s", input.Dir, origin, input.URL)
 	} else if _, statErr := os.Stat(input.Dir); statErr == nil {
@@ -209,7 +228,11 @@ func NewGitCloneExecutor(input NewGitCloneExecutorInput) common.Executor {
 			logger.Infof("git clone '%s' # ref=%s", input.URL, input.Ref)
 		}
 		logger.Debugf("  cloning %s to %s", input.URL, input.Dir)
-		defer AcquireCloneLock(input.Dir)()
+		unlock, err := AcquireCloneLock(ctx, input.Dir)
+		if err != nil {
+			return err
+		}
+		defer unlock()

 		reused, err := CloneIfRequired(ctx, input, logger)
 		if err != nil {
@@ -18,9 +18,11 @@ import (
 	"sync"
 	"syscall"
 	"testing"
+	"testing/synctest"
 	"time"

 	"gitea.com/gitea/runner/act/common"
+	"gitea.com/gitea/runner/internal/pkg/lock"

 	log "github.com/sirupsen/logrus"
 	logrustest "github.com/sirupsen/logrus/hooks/test"
@@ -29,29 +31,33 @@ import (
 )

 func TestFindGitSlug(t *testing.T) {
-	assert := assert.New(t)
-
-	slugTests := []struct {
-		url      string // input
-		provider string // expected result
-		slug     string // expected result
+	for _, testcase := range []struct {
+		remoteURL string
+		slug      string
 	}{
-		{"https://git-codecommit.us-east-1.amazonaws.com/v1/repos/my-repo-name", "CodeCommit", "my-repo-name"},
-		{"ssh://git-codecommit.us-west-2.amazonaws.com/v1/repos/my-repo", "CodeCommit", "my-repo"},
-		{"[email protected]:nektos/act.git", "GitHub", "nektos/act"},
-		{"[email protected]:nektos/act", "GitHub", "nektos/act"},
-		{"https://github.com/nektos/act.git", "GitHub", "nektos/act"},
-		{"http://github.com/nektos/act.git", "GitHub", "nektos/act"},
-		{"https://github.com/nektos/act", "GitHub", "nektos/act"},
-		{"http://github.com/nektos/act", "GitHub", "nektos/act"},
-		{"git+ssh://[email protected]/owner/repo.git", "GitHub", "owner/repo"},
-		{"http://myotherrepo.com/act.git", "", "http://myotherrepo.com/act.git"},
-	}
-
-	for _, tt := range slugTests {
-		provider, slug := findGitSlug(tt.url, "github.com")
-		assert.Equal(tt.provider, provider)
-		assert.Equal(tt.slug, slug)
+		{"https://git-codecommit.us-east-1.amazonaws.com/v1/repos/my-repo-name", "my-repo-name"},
+		{"ssh://git-codecommit.us-west-2.amazonaws.com/v1/repos/my-repo", "my-repo"},
+		{"[email protected]:nektos/act.git", "nektos/act"},
+		{"[email protected]:nektos/act", "nektos/act"},
+		{"https://github.com/nektos/act.git", "nektos/act"},
+		{"http://github.com/nektos/act.git", "nektos/act"},
+		{"https://github.com/nektos/act", "nektos/act"},
+		{"http://github.com/nektos/act", "nektos/act"},
+		{"git+ssh://[email protected]/owner/repo.git", "owner/repo"},
+		{"https://example.com:3000/gitea/owner/repo.git/", "owner/repo"},
+		{"ssh://[email protected]:2222/gitea/owner/repo.git", "owner/repo"},
+		{"example.com:owner/repo.git", "owner/repo"},
+		{"http://myotherrepo.com/act.git", ""},
+		{"https://user:[email protected]/act.git", ""},
+		{"/tmp/owner/repo.git", ""},
+		{"./owner/repo:branch", ""},
+	} {
+		t.Run(testcase.remoteURL, func(t *testing.T) {
+			slug, err := findGitSlug(testcase.remoteURL)
+			assert.Equal(t, testcase.slug == "", err != nil)
+			assert.NotContains(t, fmt.Sprint(err), "secret")
+			assert.Equal(t, testcase.slug, slug)
+		})
 	}
 }

@@ -107,7 +113,7 @@ func TestFindGitMetadataOfSHA256RepositoryUsesOrigin(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, remoteURL, url)

-	slug, err := FindGithubRepo(context.Background(), basedir, "github.com")
+	slug, err := FindGithubRepo(context.Background(), basedir)
 	require.NoError(t, err)
 	require.Equal(t, "owner/repo", slug)
 }
@@ -310,6 +316,12 @@ func TestGitCloneExecutorReclonesWhenOriginURLChanges(t *testing.T) {
 	markerPath := filepath.Join(cacheDir, "stale-marker")
 	require.NoError(t, os.WriteFile(markerPath, []byte("stale"), 0o644))

+	cancelled, cancel := context.WithCancel(t.Context())
+	cancel()
+	_, err := CloneIfRequired(cancelled, NewGitCloneExecutorInput{URL: oldRemoteDir, Ref: "main", Dir: cacheDir}, log.New())
+	require.ErrorIs(t, err, context.Canceled)
+	require.FileExists(t, markerPath)
+
 	require.NoError(t, NewGitCloneExecutor(NewGitCloneExecutorInput{
 		URL: newRemoteDir,
 		Ref: "main",
@@ -631,14 +643,42 @@ func gitCmd(args ...string) error {
 }

 func TestAcquireCloneLock(t *testing.T) {
+	t.Run("blocks on a lock held by another process despite a trailing separator", func(t *testing.T) {
+		dir := filepath.Join(t.TempDir(), "cache")
+		release, err := lock.TryLock(dir)
+		require.NoError(t, err)
+		defer func() { require.NoError(t, release()) }()
+		synctest.Test(t, func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(t.Context(), time.Second)
+			defer cancel()
+			_, err := AcquireCloneLock(ctx, dir+string(filepath.Separator))
+			require.ErrorIs(t, err, context.DeadlineExceeded)
+		})
+	})
+
+	t.Run("read-only parent falls back to an in-process lock", func(t *testing.T) {
+		parent := t.TempDir()
+		dir := filepath.Join(parent, "cache")
+		require.NoError(t, os.Mkdir(dir, 0o755))
+		require.NoError(t, os.Chmod(parent, 0o555))
+		defer func() { require.NoError(t, os.Chmod(parent, 0o755)) }()
+		unlock, err := AcquireCloneLock(t.Context(), dir)
+		require.NoError(t, err)
+		unlock()
+	})
+
 	t.Run("same directory serializes", func(t *testing.T) {
 		dir := t.TempDir()

-		unlock1 := AcquireCloneLock(dir)
+		unlock1, err := AcquireCloneLock(t.Context(), dir)
+		require.NoError(t, err)

 		secondAcquired := make(chan struct{})
 		go func() {
-			unlock := AcquireCloneLock(dir)
+			unlock, err := AcquireCloneLock(t.Context(), dir)
+			if !assert.NoError(t, err) {
+				return
+			}
 			close(secondAcquired)
 			unlock()
 		}()
@@ -662,12 +702,16 @@ func TestAcquireCloneLock(t *testing.T) {
 		dirA := t.TempDir()
 		dirB := t.TempDir()

-		unlockA := AcquireCloneLock(dirA)
+		unlockA, err := AcquireCloneLock(t.Context(), dirA)
+		require.NoError(t, err)
 		defer unlockA()

 		done := make(chan struct{})
 		go func() {
-			unlock := AcquireCloneLock(dirB)
+			unlock, err := AcquireCloneLock(t.Context(), dirB)
+			if !assert.NoError(t, err) {
+				return
+			}
 			unlock()
 			close(done)
 		}()