fix: stop a failed step disabling the toolkit patch (#1177)

A step that failed, for any reason, made the runner restore the action's stock bundle and mark it never to be patched again. Every later `actions/upload-artifact` run then failed with `GHESNotSupportedError`, and nothing in the log said why.

The edit is now made as the action is copied into the job container, under the lock that guards the copy, and nothing reverts it. That also closes the race where another job's checkout reset the bundle mid-job.

`cache.v2` no longer decides whether the edit is made, it only withdraws the v2 advertisement, so artifacts work whatever the cache is set to.

Also added a new `runner.patch_actions` option to turn the edit off if it ever breaks an action.

Fixes https://gitea.com/gitea/runner/issues/1176

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

internal/app/cmd/exec.go

@@ -446,7 +446,6 @@ func runExec(ctx context.Context, execArgs *executeArgs) func(cmd *cobra.Command
 		config := &runner.Config{
 			Workdir:               execArgs.Workdir(),
 			BindWorkdir:           false,
-			PatchToolkit:          true, // the cache server started above is what the patch points at
 			ReuseContainers:       false,
 			ForcePull:             execArgs.forcePull,
 			ForceRebuild:          execArgs.forceRebuild,

internal/app/run/runner.go

@@ -461,10 +461,13 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 	// is that server's responsibility to authenticate requests.
 	revokeCache, resultsURL := r.registerCacheForTask(giteaRuntimeToken, preset.Repository, reporter)
 	defer revokeCache()
-	// A cache server that agreed to forward the artifact half is the whole results service, so
-	// the job is pointed at it and the v2 variable is finally true.
+	// A cache server that agreed to forward the artifact half is the whole results service, so the
+	// job is pointed at it.
 	if resultsURL != "" {
-		envs["ACTIONS_RESULTS_URL"], envs[runner.CacheServiceV2Env] = resultsURL, "true"
+		envs["ACTIONS_RESULTS_URL"] = resultsURL
+		if r.cacheServiceV2() {
+			envs[runner.CacheServiceV2Env] = "true"
+		}
 	}
 
 	eventJSON, err := json.Marshal(preset.Event)
@@ -515,7 +518,7 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 		AllocatePTY:       r.cfg.Runner.AllocatePTY,
 		ActionOfflineMode: r.cfg.Cache.OfflineMode,
 		ActionCloneDepth:  actionCloneDepth,
-		PatchToolkit:      r.patchToolkit(),
+		NoActionPatch:     r.cfg.Runner.PatchActions != nil && !*r.cfg.Runner.PatchActions,
 
 		ReuseContainers:      false,
 		ForcePull:            r.cfg.Container.ForcePull,
@@ -590,10 +593,10 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 	return execErr
 }
 
-// patchToolkit reports whether act should edit the toolkit bundled into an action. It follows the
-// cache URL, because that is what the edits point the client at; see act/runner/toolkit_patch.go.
-func (r *Runner) patchToolkit() bool {
-	return r.envs["ACTIONS_CACHE_URL"] != "" && (r.cfg.Cache.V2 == nil || *r.cfg.Cache.V2)
+// cacheServiceV2 reports whether jobs are told the cache service speaks v2. It is all cache.v2
+// turns off: the bundle edit that reaches it is what the artifact actions need too.
+func (r *Runner) cacheServiceV2() bool {
+	return r.cfg.Cache.V2 == nil || *r.cfg.Cache.V2
 }
 
 // registerCacheForTask tells the cache server to accept requests authenticated

internal/app/run/runner_test.go

@@ -169,7 +169,6 @@ func TestNewRunnerCacheServiceV2(t *testing.T) {
 
 	assert.Equal(t, "https://gitea.example", r.envs["ACTIONS_RESULTS_URL"])
 	assert.Empty(t, r.envs[runner.CacheServiceV2Env], "a promise the runner has not made yet")
-	assert.True(t, r.patchToolkit())
 
 	// The registration is what makes it true: the cache server takes the results service over,
 	// having been told which instance to forward the artifact half to.
@@ -190,6 +189,11 @@ func TestNewRunnerCacheServiceV2(t *testing.T) {
 	defer resp.Body.Close()
 
 	assert.Equal(t, http.StatusOK, resp.StatusCode, "the advertised results service serves no cache service")
+
+	// Turning v2 off withdraws the advertisement and nothing else.
+	assert.True(t, r.cacheServiceV2())
+	cfg.Cache.V2 = new(bool)
+	assert.False(t, r.cacheServiceV2())
 }
 
 // The v1 cache client appends its path to ACTIONS_CACHE_URL without a separator, so a configured
@@ -203,9 +207,8 @@ func TestNewRunnerNormalizesTheExternalCacheServer(t *testing.T) {
 	r := NewRunner(cfg, &config.Registration{Name: "runner"}, cli)
 
 	assert.Equal(t, "http://cache.local:8088/", r.envs["ACTIONS_CACHE_URL"])
-	// Nothing to front the results service with, so the variable stays unset, but the bundles are
-	// still patched: artifacts v4 need that, and the patch keeps the cache client on the cache URL.
+	// Nothing to front the results service with, so the variable stays unset and the client keeps
+	// to v1, which reads the cache URL first.
 	assert.Equal(t, "https://gitea.example", r.envs["ACTIONS_RESULTS_URL"])
 	assert.Empty(t, r.envs[runner.CacheServiceV2Env])
-	assert.True(t, r.patchToolkit())
 }