diff --git a/internal/act/common/git/git.go b/internal/act/common/git/git.go index 952247b4..a1bfaab0 100644 --- a/internal/act/common/git/git.go +++ b/internal/act/common/git/git.go @@ -458,10 +458,14 @@ func NewGitCloneExecutor(input NewGitCloneExecutorInput) common.Executor { logger.Debugf("Unable to pull %s: %v", refName, err) } case isOfflineMode && reused: - reusedMsg = " (reused in offline mode)" + reusedMsg = " (offline mode)" } - logger.Debugf("Cloned %s to %s%s", input.URL, input.Dir, reusedMsg) + if reused { + logger.Debugf("Reused %s at %s%s", input.URL, input.Dir, reusedMsg) + } else { + logger.Debugf("Cloned %s to %s", input.URL, input.Dir) + } if hash.String() != input.Ref && refType == "branch" { logger.Debugf("Provided ref is not a sha. Updating branch ref after pull") diff --git a/internal/act/common/git/git_test.go b/internal/act/common/git/git_test.go index 6a1d1254..6f31c0ab 100644 --- a/internal/act/common/git/git_test.go +++ b/internal/act/common/git/git_test.go @@ -373,22 +373,28 @@ func TestGitCloneExecutorOfflineMode(t *testing.T) { // Prime the cache with an online clone of main. cacheDir := t.TempDir() + logger, hook := logrustest.NewNullLogger() + logger.SetLevel(log.DebugLevel) + ctx := common.WithLogger(context.Background(), logger.WithField("job", "j1")) require.NoError(t, NewGitCloneExecutor(NewGitCloneExecutorInput{ URL: remoteDir, Ref: "main", Dir: cacheDir, - })(context.Background())) + })(ctx)) + assert.Contains(t, logMessages(hook), "Cloned "+remoteDir+" to "+cacheDir) t.Run("cached branch resolves without fetching", func(t *testing.T) { // Offline reuse of a cached branch must succeed even though ResolveRevision(input.Ref) // finds no local refs/heads/. + hook.Reset() err := NewGitCloneExecutor(NewGitCloneExecutorInput{ URL: remoteDir, Ref: "main", Dir: cacheDir, OfflineMode: true, - })(context.Background()) + })(ctx) require.NoError(t, err) + assert.Contains(t, logMessages(hook), "Reused "+remoteDir+" at "+cacheDir+" (offline mode)") out, err := exec.Command("git", "-C", cacheDir, "log", "--oneline", "-1", "--format=%s").Output() require.NoError(t, err) @@ -445,6 +451,14 @@ func TestGitCloneExecutorQuietDemotesCloneLine(t *testing.T) { } } +func logMessages(hook *logrustest.Hook) []string { + messages := []string{} + for _, entry := range hook.AllEntries() { + messages = append(messages, entry.Message) + } + return messages +} + func TestGitCloneExecutorShallow(t *testing.T) { // Build a local "remote" with several commits on main plus a tag, so a full clone would pull noticeably more history than a shallow one. remoteDir := t.TempDir() diff --git a/internal/act/runner/action.go b/internal/act/runner/action.go index cb93e013..ab2e572a 100644 --- a/internal/act/runner/action.go +++ b/internal/act/runner/action.go @@ -293,7 +293,9 @@ func removeGitIgnore(ctx context.Context, directory string) error { // same `act-dockeraction:latest` image on a shared docker daemon. A subsequent // repository would then silently run the image built for an earlier one. // Including the repository keeps the tag stable for caching within a repository -// while preventing cross-repository collisions. +// while preventing cross-repository collisions. A remote action needs the same +// treatment, because its actionName is the shared checkout of its repository and +// ref plus the action's path inside it. // See https://gitea.com/gitea/runner/issues/1039. func dockerActionImageTag(repository, actionName string, localAction bool) string { name := actionName @@ -302,11 +304,10 @@ func dockerActionImageTag(repository, actionName string, localAction bool) strin } // The human-readable name is sanitized by collapsing every non-alphanumeric character to "-". sanitized := regexp.MustCompile("[^a-zA-Z0-9]").ReplaceAllString(name, "-") - if localAction { - // For local actions a short hash of the raw repository and action path is appended so the tag stays unique per repository. - sum := sha256.Sum256([]byte(repository + "\x00" + actionName)) - sanitized += "-" + hex.EncodeToString(sum[:])[:12] - } + // Sanitizing is lossy, so a short hash of the raw repository and action path is appended, keeping + // the tag unique per repository and per action inside it. + sum := sha256.Sum256([]byte(repository + "\x00" + actionName)) + sanitized += "-" + hex.EncodeToString(sum[:])[:12] // "-dockeraction" ensures that "./", "./test " won't get converted to "act-:latest", "act-test-:latest" which are invalid docker image names image := fmt.Sprintf("%s-dockeraction:%s", sanitized, "latest") image = "act-" + strings.TrimLeft(image, "-") diff --git a/internal/act/runner/action_test.go b/internal/act/runner/action_test.go index 9de0d49c..fabe053c 100644 --- a/internal/act/runner/action_test.go +++ b/internal/act/runner/action_test.go @@ -496,11 +496,11 @@ func TestExecAsDockerHoldsCloneLockForRemoteUncached(t *testing.T) { } func TestDockerActionImageTag(t *testing.T) { - // Remote actions already carry a unique, ref-scoped actionName (the uses - // hash), so the tag must be left untouched for backwards compatibility. - assert.Equal(t, - "act-abc123-dockeraction:latest", - dockerActionImageTag("owner/repo", "abc123", false), + // A remote action's actionName is the checkout of its repository and ref plus its path inside it, + // and paths that sanitize alike share the readable prefix, so siblings must stay apart. + assert.NotEqual(t, + dockerActionImageTag("owner/repo", "abc123/a-b", false), + dockerActionImageTag("owner/repo", "abc123/a_b", false), ) // Local actions keep a human-readable, repository-namespaced prefix and gain a short hash suffix that makes the tag unique per (repository, actionName). diff --git a/internal/act/runner/job_executor_test.go b/internal/act/runner/job_executor_test.go index db44762a..2f9568ec 100644 --- a/internal/act/runner/job_executor_test.go +++ b/internal/act/runner/job_executor_test.go @@ -146,7 +146,8 @@ func TestPrintPrepareActionsGolden(t *testing.T) { &actionPreparerMock{reference: "actions/checkout@v7", sha: "9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", ok: true}, // A resolved commit is best effort; the ref alone is reported when it is unknown. &actionPreparerMock{reference: "actions/setup-go@v6", ok: true}, - // A step that downloads nothing, such as the checkout of the workflow's own repository. + // A step that downloads nothing, such as the checkout of the workflow's own repository or a + // second step on an action the job already downloaded. &actionPreparerMock{ok: false}, } require.NoError(t, printPrepareActions(&RunContext{}, preparers)(ctx)) diff --git a/internal/act/runner/run_context.go b/internal/act/runner/run_context.go index 8af9ee40..76c53fd9 100644 --- a/internal/act/runner/run_context.go +++ b/internal/act/runner/run_context.go @@ -65,7 +65,8 @@ type RunContext struct { Parent *RunContext Masks []string cleanUpJobContainer common.Executor - caller *caller // job calling this RunContext (reusable workflows) + caller *caller // job calling this RunContext (reusable workflows) + actionDownloads map[string]string // resolved commit per action repository, downloaded once per job // summaryFileInitialized tracks which per-step summary files (workflow/step-summary-N.md) // have already been created on the JobContainer. The runner sets up file-command files // via JobContainer.Copy at the start of every phase, which truncates them — fine for @@ -1010,6 +1011,17 @@ func (rc *RunContext) topLevelRunContext() *RunContext { return top } +// downloadedActions returns the action repositories this job has downloaded, keyed by repository +// and ref. Composite actions reach the job's map through their Parent chain, and a job runs its +// steps one at a time, so the map needs no synchronization. +func (rc *RunContext) downloadedActions() map[string]string { + top := rc.topLevelRunContext() + if top.actionDownloads == nil { + top.actionDownloads = map[string]string{} + } + return top.actionDownloads +} + // Executor returns a pipeline executor for all the steps in the job func (rc *RunContext) Executor() (common.Executor, error) { var executor common.Executor diff --git a/internal/act/runner/step_action_remote.go b/internal/act/runner/step_action_remote.go index 71f9ec57..4942e657 100644 --- a/internal/act/runner/step_action_remote.go +++ b/internal/act/runner/step_action_remote.go @@ -35,6 +35,7 @@ type stepActionRemote struct { remoteAction *remoteAction cacheDir string resolvedSha string + downloaded bool // this step fetched the action repository, rather than reusing a fetch of the job } var stepActionRemoteNewCloneExecutor = git.NewGitCloneExecutor @@ -76,19 +77,28 @@ func (sar *stepActionRemote) prepareActionExecutor() common.Executor { github.Token = sar.RunContext.Config.ReplaceGheActionTokenWithGithubCom } } + + downloads := sar.RunContext.downloadedActions() + downloadKey := sar.downloadKey() + // Actions served from the action cache are read out of a git object store rather than a // directory, so they never reach the bundle patch below and keep to the v1 cache API. if sar.RunContext.Config.ActionCache != nil { cache := sar.RunContext.Config.ActionCache - var err error sar.cacheDir = fmt.Sprintf("%s/%s", sar.remoteAction.Org, sar.remoteAction.Repo) - repoURL := sar.remoteAction.URL + "/" + sar.cacheDir - repoRef := sar.remoteAction.Ref - sar.resolvedSha, err = cache.Fetch(ctx, sar.cacheDir, repoURL, repoRef, github.Token) - if err != nil { - return fmt.Errorf("failed to fetch \"%s\" version \"%s\": %w", repoURL, repoRef, err) + sha, downloaded := downloads[downloadKey] + if !downloaded { + var err error + repoURL := sar.remoteAction.URL + "/" + sar.cacheDir + repoRef := sar.remoteAction.Ref + if sha, err = cache.Fetch(ctx, sar.cacheDir, repoURL, repoRef, github.Token); err != nil { + return fmt.Errorf("failed to fetch \"%s\" version \"%s\": %w", repoURL, repoRef, err) + } + downloads[downloadKey] = sha + sar.downloaded = true } + sar.resolvedSha = sha remoteReader := func(ctx context.Context) actionYamlReader { return func(filename string) (io.Reader, io.Closer, error) { @@ -122,48 +132,53 @@ func (sar *stepActionRemote) prepareActionExecutor() common.Executor { } actionDir := sar.actionDir() - defaultActionURL := sar.RunContext.Config.DefaultActionURL() - // For Gitea - // A composite RunContext nils Config.Secrets, so getGitCloneToken would yield an - // empty token and clone the action anonymously (401 against the authenticated - // instance). github.Token survives the composite config copy and matches the - // top-level token; keep the shouldCloneURLUseToken host gate to avoid leaking it. - cloneURL := sar.remoteAction.CloneURL(defaultActionURL) - token := "" - if shouldCloneURLUseToken(sar.RunContext.Config.GitHubInstance, sar.RunContext.Config.trustedActionInstance(), cloneURL) { - token = github.Token - } - gitClone := stepActionRemoteNewCloneExecutor(git.NewGitCloneExecutorInput{ - URL: cloneURL, - Ref: sar.remoteAction.Ref, - Dir: actionDir, - Token: token, - OfflineMode: sar.RunContext.Config.ActionOfflineMode, - Depth: sar.RunContext.Config.ActionCloneDepth, - // printPrepareActions reports the download with its resolved commit. - Quiet: true, - - InsecureSkipTLS: sar.cloneSkipTLS(), // For Gitea - }) var ntErr common.Executor - if err := gitClone(ctx); err != nil { - var refErr *git.Error - if errors.As(err, &refErr) && errors.Is(err, git.ErrShortRef) { - return fmt.Errorf("Unable to resolve action `%s`, the provided ref `%s` is the shortened version of a commit SHA, which is not supported. Please use the full commit SHA `%s` instead", - sar.Step.Uses, sar.remoteAction.Ref, refErr.Commit()) - } else if errors.Is(err, gogit.ErrForceNeeded) { // TODO: figure out if it will be easy to shadow/alias go-git err's - ntErr = common.NewInfoExecutor("Non-terminating error while running 'git clone': %v", err) - } else { - return err + sha, downloaded := downloads[downloadKey] + if !downloaded { + // For Gitea + // A composite RunContext nils Config.Secrets, so getGitCloneToken would yield an + // empty token and clone the action anonymously (401 against the authenticated + // instance). github.Token survives the composite config copy and matches the + // top-level token; keep the shouldCloneURLUseToken host gate to avoid leaking it. + cloneURL := sar.remoteAction.CloneURL(sar.RunContext.Config.DefaultActionURL()) + token := "" + if shouldCloneURLUseToken(sar.RunContext.Config.GitHubInstance, sar.RunContext.Config.trustedActionInstance(), cloneURL) { + token = github.Token } - } + gitClone := stepActionRemoteNewCloneExecutor(git.NewGitCloneExecutorInput{ + URL: cloneURL, + Ref: sar.remoteAction.Ref, + Dir: actionDir, + Token: token, + OfflineMode: sar.RunContext.Config.ActionOfflineMode, + Depth: sar.RunContext.Config.ActionCloneDepth, + // printPrepareActions reports the download with its resolved commit. + Quiet: true, - // Best effort: the download report falls back to the ref alone when the commit is unknown. - if _, sha, err := git.FindGitRevision(ctx, actionDir); err != nil { - common.Logger(ctx).Debugf("unable to resolve the commit of %s: %v", sar.remoteAction.Reference(), err) - } else { - sar.resolvedSha = sha + InsecureSkipTLS: sar.cloneSkipTLS(), // For Gitea + }) + if err := gitClone(ctx); err != nil { + var refErr *git.Error + if errors.As(err, &refErr) && errors.Is(err, git.ErrShortRef) { + return fmt.Errorf("Unable to resolve action `%s`, the provided ref `%s` is the shortened version of a commit SHA, which is not supported. Please use the full commit SHA `%s` instead", + sar.Step.Uses, sar.remoteAction.Ref, refErr.Commit()) + } else if errors.Is(err, gogit.ErrForceNeeded) { // TODO: figure out if it will be easy to shadow/alias go-git err's + ntErr = common.NewInfoExecutor("Non-terminating error while running 'git clone': %v", err) + } else { + return err + } + } + + // Best effort: the download report falls back to the ref alone when the commit is unknown. + if _, resolved, err := git.FindGitRevision(ctx, actionDir); err != nil { + common.Logger(ctx).Debugf("unable to resolve the commit of %s: %v", sar.remoteAction.Reference(), err) + } else { + sha = resolved + } + downloads[downloadKey] = sha + sar.downloaded = true } + sar.resolvedSha = sha remoteReader := func(ctx context.Context) actionYamlReader { //nolint:unparam // pre-existing issue from nektos/act return func(filename string) (io.Reader, io.Closer, error) { @@ -187,13 +202,14 @@ func (sar *stepActionRemote) prepareActionExecutor() common.Executor { } } -// actionDownloadInfo reports the action this step downloaded and the commit it resolved to. ok is -// false when nothing was fetched, as for the local checkout of the workflow's own repository. +// actionDownloadInfo reports the repository this step downloaded and the commit it resolved to. ok +// is false when nothing was fetched, as for the local checkout of the workflow's own repository or +// for an action another step of the job already downloaded. func (sar *stepActionRemote) actionDownloadInfo() (reference, sha string, ok bool) { - if sar.remoteAction == nil || sar.action == nil { + if sar.remoteAction == nil || sar.action == nil || !sar.downloaded { return "", "", false } - return sar.remoteAction.Reference(), sar.resolvedSha, true + return sar.remoteAction.RepoReference(), sar.resolvedSha, true } func (sar *stepActionRemote) pre() common.Executor { @@ -234,21 +250,23 @@ func (sar *stepActionRemote) post() common.Executor { return runStepExecutor(sar, stepStagePost, sar.revertToolkitOnFailure(runPostStep(sar))).If(hasPostStep(sar)).If(shouldRunPostStep(sar)) } -// toolkitBundles is the action directory and the entrypoints the toolkit may live in. -func (sar *stepActionRemote) toolkitBundles() (string, []string) { +// toolkitBundles is the repository's checkout, the action inside it, and the entrypoints the +// toolkit may live in. +func (sar *stepActionRemote) toolkitBundles() (dir, location string, scripts []string) { if sar.remoteAction == nil { - return "", nil + return "", "", nil } - dir := sar.actionDir() - return dir, actionScriptPaths(filepath.Join(dir, sar.remoteAction.Path), sar.action) + dir = sar.actionDir() + location = filepath.Join(dir, sar.remoteAction.Path) + return dir, location, actionScriptPaths(location, sar.action) } // patchActionToolkit edits the bundled toolkit so it works against Gitea: the artifact actions // stop refusing, and the cache client keeps to the cache server whichever API version it picks. func (sar *stepActionRemote) patchActionToolkit(ctx context.Context) error { if sar.RunContext.Config.PatchToolkit { - dir, scripts := sar.toolkitBundles() - patchToolkit(ctx, dir, scripts) + dir, location, scripts := sar.toolkitBundles() + patchToolkit(ctx, dir, location, scripts) } return nil } @@ -259,20 +277,21 @@ func (sar *stepActionRemote) revertToolkitOnFailure(exec common.Executor) common return func(ctx context.Context) error { err := exec(ctx) if err != nil { - dir, scripts := sar.toolkitBundles() - revertToolkit(ctx, dir, scripts) + dir, location, scripts := sar.toolkitBundles() + revertToolkit(ctx, dir, location, scripts) } return err } } +// downloadKey is the clone URL and ref a step downloads, shared by every action path inside that +// repository, and distinct per instance serving it. +func (sar *stepActionRemote) downloadKey() string { + return sar.remoteAction.CloneURL(sar.RunContext.Config.DefaultActionURL()) + "@" + sar.remoteAction.Ref +} + func (sar *stepActionRemote) actionDir() string { - uses := sar.Step.Uses - if strings.HasPrefix(uses, selfRepoPrefix) { - // The same `$/x` names a different action per enclosing repo, so key the cache on what it resolved to. - uses = sar.remoteAction.URL + "/" + sar.remoteAction.Reference() - } - return fmt.Sprintf("%s/%s", sar.RunContext.ActionCacheDir(), model.UsesHash(uses)) + return fmt.Sprintf("%s/%s", sar.RunContext.ActionCacheDir(), model.UsesHash(sar.downloadKey())) } func (sar *stepActionRemote) getRunContext() *RunContext { @@ -388,11 +407,16 @@ func (ra *remoteAction) CloneURL(u string) string { // Reference renders the action as {org}/{repo}[/path]@{ref}, omitting the download source, which // can be interpolated from a secret. func (ra *remoteAction) Reference() string { - repo := fmt.Sprintf("%s/%s", ra.Org, ra.Repo) - if ra.Path != "" { - repo = fmt.Sprintf("%s/%s", repo, ra.Path) + if ra.Path == "" { + return ra.RepoReference() } - return fmt.Sprintf("%s@%s", repo, ra.Ref) + return fmt.Sprintf("%s/%s/%s@%s", ra.Org, ra.Repo, ra.Path, ra.Ref) +} + +// RepoReference renders the downloaded repository as {org}/{repo}@{ref}, the unit actions/runner +// downloads and reports, which carries no path inside the repository. +func (ra *remoteAction) RepoReference() string { + return fmt.Sprintf("%s/%s@%s", ra.Org, ra.Repo, ra.Ref) } func (ra *remoteAction) IsCheckout() bool { diff --git a/internal/act/runner/step_action_remote_test.go b/internal/act/runner/step_action_remote_test.go index e46b9424..8a5cdd2f 100644 --- a/internal/act/runner/step_action_remote_test.go +++ b/internal/act/runner/step_action_remote_test.go @@ -41,6 +41,13 @@ func (sarm *stepActionRemoteMocks) runAction(step actionStep, actionDir string, return args.Get(0).(func(context.Context) error) } +// actionDirMatcher matches the cache directory the step clones into, known once it resolved its `uses:`. +func actionDirMatcher(sar *stepActionRemote) any { + return mock.MatchedBy(func(actionDir string) bool { + return actionDir == sar.actionDir() + }) +} + func TestStepActionRemote(t *testing.T) { table := []struct { name string @@ -170,17 +177,11 @@ func TestStepActionRemote(t *testing.T) { } sar.RunContext.ExprEval = sar.RunContext.NewExpressionEvaluator(ctx) - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - if tt.mocks.read { - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) } if tt.mocks.run { - sarm.On("runAction", sar, suffixMatcher(sar.Step.UsesHash()), newRemoteAction(sar.Step.Uses)).Return(func(ctx context.Context) error { return tt.runError }) + sarm.On("runAction", sar, actionDirMatcher(sar), newRemoteAction(sar.Step.Uses)).Return(func(ctx context.Context) error { return tt.runError }) cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error { return nil @@ -266,13 +267,7 @@ func TestStepActionRemotePre(t *testing.T) { readAction: sarm.readAction, } - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) err := sar.pre()(ctx) @@ -284,6 +279,58 @@ func TestStepActionRemotePre(t *testing.T) { } } +func TestStepActionRemotePrepareDownloadsRepositoryOncePerJob(t *testing.T) { + ctx := context.Background() + sarm := &stepActionRemoteMocks{} + cloned := 0 + + origNewCloneExecutor := stepActionRemoteNewCloneExecutor + stepActionRemoteNewCloneExecutor = func(git.NewGitCloneExecutorInput) common.Executor { + return func(context.Context) error { + cloned++ + return nil + } + } + defer func() { + stepActionRemoteNewCloneExecutor = origNewCloneExecutor + }() + + rc := newTestRC(&model.Workflow{Jobs: map[string]*model.Job{"job1": {}}}, nil) + rc.Config.ActionCacheDir = t.TempDir() + rc.ExprEval = rc.NewExpressionEvaluator(ctx) + + rootAction := &model.Action{Name: "root"} + subAction := &model.Action{Name: "sub"} + sarm.On("readAction", mock.Anything, mock.Anything, "", mock.Anything, mock.Anything).Return(rootAction, nil).Times(3) + sarm.On("readAction", mock.Anything, mock.Anything, "sub", mock.Anything, mock.Anything).Return(subAction, nil).Once() + + steps := make([]*stepActionRemote, 0, 4) + for _, uses := range []string{"org/repo@v1", "org/repo@v1", "org/repo/sub@v1", "org/repo@v2"} { + sar := &stepActionRemote{ + Step: &model.Step{Uses: uses}, + RunContext: rc, + readAction: sarm.readAction, + } + require.NoError(t, sar.prepareActionExecutor()(ctx)) + steps = append(steps, sar) + } + + assert.Equal(t, 2, cloned) // one per ref, shared by both paths of v1 + assert.Same(t, rootAction, steps[1].action) + assert.Same(t, subAction, steps[2].action) + sarm.AssertExpectations(t) + + reference, _, ok := steps[0].actionDownloadInfo() + assert.True(t, ok) + assert.Equal(t, "org/repo@v1", reference) + for _, reusing := range steps[1:3] { + _, _, ok := reusing.actionDownloadInfo() + assert.False(t, ok) + } + _, _, ok = steps[3].actionDownloadInfo() + assert.True(t, ok) +} + func TestStepActionRemotePreThroughAction(t *testing.T) { table := []struct { name string @@ -337,13 +384,7 @@ func TestStepActionRemotePreThroughAction(t *testing.T) { readAction: sarm.readAction, } - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) err := sar.pre()(ctx) @@ -413,13 +454,7 @@ func TestStepActionRemotePreThroughActionToken(t *testing.T) { readAction: sarm.readAction, } - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "path", mock.Anything, mock.Anything).Return(&model.Action{}, nil) err := sar.pre()(ctx) @@ -476,13 +511,7 @@ func TestStepActionRemoteUsesGitHubInstanceWhenDefaultActionInstanceEmpty(t *tes }, readAction: sarm.readAction, } - - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) require.NoError(t, sar.prepareActionExecutor()(ctx)) assert.Equal(t, "https://gitea.example/actions/setup-go", actualURL) @@ -962,6 +991,7 @@ func TestStepActionRemoteActionDownloadInfo(t *testing.T) { remoteAction: newRemoteAction("actions/checkout@v7"), action: &model.Action{}, resolvedSha: "9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0", + downloaded: true, } reference, sha, ok := sar.actionDownloadInfo() @@ -1065,13 +1095,7 @@ func TestStepActionRemoteCloneTokenSurvivesNilSecrets(t *testing.T) { readAction: sarm.readAction, } sar.RunContext.ExprEval = sar.RunContext.NewExpressionEvaluator(ctx) - - suffixMatcher := func(suffix string) any { - return mock.MatchedBy(func(actionDir string) bool { - return strings.HasSuffix(actionDir, suffix) - }) - } - sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + sarm.On("readAction", sar.Step, actionDirMatcher(sar), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) err := sar.prepareActionExecutor()(ctx) require.NoError(t, err) diff --git a/internal/act/runner/toolkit_patch.go b/internal/act/runner/toolkit_patch.go index a4f4b521..717a4075 100644 --- a/internal/act/runner/toolkit_patch.go +++ b/internal/act/runner/toolkit_patch.go @@ -111,11 +111,11 @@ func actionScriptPaths(dir string, action *model.Action) []string { // patchToolkit edits the toolkit in an action's bundles, keeping each original beside them. Every // failure is silent and leaves the bundle as it was, which costs the cache client the v2 API and // an artifact action nothing at all. -func patchToolkit(ctx context.Context, actionDir string, scripts []string) { +func patchToolkit(ctx context.Context, actionDir, actionLocation string, scripts []string) { if len(scripts) == 0 { return } - if _, err := os.Stat(filepath.Join(sidecarDir(actionDir), skipMarker)); err == nil { + if _, err := os.Stat(skipMarkerFor(actionDir, actionLocation)); err == nil { return } defer git.AcquireCloneLock(actionDir)() @@ -130,7 +130,7 @@ func patchToolkit(ctx context.Context, actionDir string, scripts []string) { // revertToolkit puts the originals back and stops this action being patched again, so the next job // runs it exactly as shipped. Called when a step failed with a patched bundle; it does not re-run // the step, because a step's outputs and env-file writes are already recorded by then. -func revertToolkit(ctx context.Context, actionDir string, scripts []string) { +func revertToolkit(ctx context.Context, actionDir, actionLocation string, scripts []string) { if len(scripts) == 0 { return } @@ -150,16 +150,22 @@ func revertToolkit(ctx context.Context, actionDir string, scripts []string) { } } if reverted { - _ = os.WriteFile(filepath.Join(sidecarDir(actionDir), skipMarker), nil, 0o600) - common.Logger(ctx).Warnf("actions toolkit: restored the original %s, it will not be patched again", filepath.Base(actionDir)) + _ = os.WriteFile(skipMarkerFor(actionDir, actionLocation), nil, 0o600) + common.Logger(ctx).Warnf("actions toolkit: restored the original %s, it will not be patched again", filepath.Base(actionLocation)) } } -// sidecarDir holds an action's untouched bundles, and the marker that stops it being patched. +// sidecarDir holds a checkout's untouched bundles, and the markers that stop actions being patched. func sidecarDir(actionDir string) string { return actionDir + sidecarSuffix } +// skipMarkerFor stops one action being patched again. It sits among that action's own originals, so +// a sibling action sharing the checkout keeps being patched. +func skipMarkerFor(actionDir, actionLocation string) string { + return originalFor(actionDir, filepath.Join(actionLocation, skipMarker)) +} + // originalFor is where a script's untouched copy lives, or "" for a script the action's own // `runs` keys placed outside its directory, which is not this runner's to rewrite. func originalFor(actionDir, script string) string { diff --git a/internal/act/runner/toolkit_patch_e2e_test.go b/internal/act/runner/toolkit_patch_e2e_test.go index edb0dfa5..2b442cd8 100644 --- a/internal/act/runner/toolkit_patch_e2e_test.go +++ b/internal/act/runner/toolkit_patch_e2e_test.go @@ -118,7 +118,7 @@ func patchedAction(t *testing.T, repo, ref, entrypoint string) string { dir := tempDirPath(t) script := filepath.Join(dir, filepath.Base(entrypoint)) require.NoError(t, os.WriteFile(script, body, 0o600)) - patchToolkit(t.Context(), dir, []string{script}) + patchToolkit(t.Context(), dir, dir, []string{script}) return script } diff --git a/internal/act/runner/toolkit_patch_test.go b/internal/act/runner/toolkit_patch_test.go index 4ba2a66a..183a39e5 100644 --- a/internal/act/runner/toolkit_patch_test.go +++ b/internal/act/runner/toolkit_patch_test.go @@ -234,18 +234,18 @@ func TestRevertToolkit(t *testing.T) { dir, script := bundleFile(t, gateTSC) scripts := []string{script} - patchToolkit(t.Context(), dir, scripts) + patchToolkit(t.Context(), dir, dir, scripts) body, err := os.ReadFile(script) require.NoError(t, err) require.True(t, gateOpened(string(body)), "precondition: the bundle is patched") - revertToolkit(t.Context(), dir, scripts) + revertToolkit(t.Context(), dir, dir, scripts) body, err = os.ReadFile(script) require.NoError(t, err) assert.Equal(t, gateTSC, string(body), "the original bundle is back") // The skip marker survives, so the action stays unpatched from now on. - patchToolkit(t.Context(), dir, scripts) + patchToolkit(t.Context(), dir, dir, scripts) body, err = os.ReadFile(script) require.NoError(t, err) assert.Equal(t, gateTSC, string(body), "a reverted action stays unpatched") @@ -258,11 +258,11 @@ func TestPatchBundleAfterTheActionMoved(t *testing.T) { original := originalFor(dir, script) scripts := []string{script} - patchToolkit(t.Context(), dir, scripts) + patchToolkit(t.Context(), dir, dir, scripts) require.NoError(t, os.WriteFile(script, []byte(gateWebpack), 0o600)) // the new version lands // Reverting must not roll the action back to the version the original came from. - revertToolkit(t.Context(), dir, scripts) + revertToolkit(t.Context(), dir, dir, scripts) body, err := os.ReadFile(script) require.NoError(t, err) assert.Equal(t, gateWebpack, string(body)) @@ -282,25 +282,25 @@ func TestPatchBundleAfterTheActionMoved(t *testing.T) { // The wiring: a step patches its own bundles only when the runner serves the v2 API, and a step // that fails gets them back. The action's path inside its repository is part of where they live. func TestStepActionRemoteToolkitPatch(t *testing.T) { - newStep := func(t *testing.T, patch bool) (*stepActionRemote, string) { + newStep := func(t *testing.T, cacheDir, actionPath string, patch bool) (*stepActionRemote, string) { t.Helper() sar := &stepActionRemote{ - Step: &model.Step{Uses: "owner/repo/sub@v1"}, - remoteAction: &remoteAction{Org: "owner", Repo: "repo", Path: "sub", Ref: "v1"}, + Step: &model.Step{Uses: "owner/repo/" + actionPath + "@v1"}, + remoteAction: &remoteAction{Org: "owner", Repo: "repo", Path: actionPath, Ref: "v1"}, action: &model.Action{Runs: model.ActionRuns{Using: "node20", Main: "index.js"}}, RunContext: &RunContext{ - Config: &Config{ActionCacheDir: t.TempDir(), PatchToolkit: patch}, + Config: &Config{ActionCacheDir: cacheDir, PatchToolkit: patch}, }, } - script := filepath.Join(sar.actionDir(), "sub", "index.js") + script := filepath.Join(sar.actionDir(), actionPath, "index.js") require.NoError(t, os.MkdirAll(filepath.Dir(script), 0o755)) require.NoError(t, os.WriteFile(script, []byte(gateTSC), 0o600)) return sar, script } t.Run("left alone when the runner does not patch", func(t *testing.T) { - sar, script := newStep(t, false) + sar, script := newStep(t, t.TempDir(), "sub", false) require.NoError(t, sar.patchActionToolkit(t.Context())) body, err := os.ReadFile(script) @@ -309,7 +309,8 @@ func TestStepActionRemoteToolkitPatch(t *testing.T) { }) t.Run("patched, and put back when the step fails", func(t *testing.T) { - sar, script := newStep(t, true) + cacheDir := t.TempDir() + sar, script := newStep(t, cacheDir, "sub", true) require.NoError(t, sar.patchActionToolkit(t.Context())) body, err := os.ReadFile(script) @@ -322,5 +323,13 @@ func TestStepActionRemoteToolkitPatch(t *testing.T) { body, err = os.ReadFile(script) require.NoError(t, err) assert.Equal(t, gateTSC, string(body)) + + // A sibling action shares the repository's checkout, and must not be marked off with it. + sibling, siblingScript := newStep(t, cacheDir, "other", true) + require.NoError(t, sibling.patchActionToolkit(t.Context())) + + body, err = os.ReadFile(siblingScript) + require.NoError(t, err) + assert.True(t, gateOpened(string(body))) }) }