fix: serve the whole results service from the cache server (#1141)

`ACTIONS_RESULTS_URL` names one origin serving every `github.actions.results.api.v1` service. Gitea serves the artifact half and this runner the cache half, so announcing `ACTIONS_CACHE_SERVICE_V2` while that URL pointed at Gitea was a promise the environment could not keep, and `docker buildx` posted its cache calls at Gitea and got a 404. The cache server now forwards the artifact half to the instance each job registers with, so it is the whole results service and jobs are pointed at it. The announcement follows, and the bundle patch follows the cache URL instead. Also fixes three things no JavaScript client reached: camelCase in the v2 responses where the Go clients read proto names, the missing `x-ms-request-id` on blob uploads that panics buildkit, and `cache.external_server` passed through without the trailing slash the v1 client concatenates onto. Tests run the real actions against the services they look for: `actions/cache` over both API versions, the artifact actions up and back down through the forwarding, and `setup-node`. The regression itself is covered by asserting that whatever a job is handed as `ACTIONS_RESULTS_URL` answers a cache service call. Fixes https://gitea.com/gitea/runner/issues/1139 Reviewed-on: https://gitea.com/gitea/runner/pulls/1141 Reviewed-by: bircni <bircni@icloud.com> Co-authored-by: silverwind <2021+silverwind@noreply.gitea.com>
2026-08-06 00:44:22 +02:00 · 2026-08-03 16:41:53 +00:00
parent 55a625f733
commit 3618385b28
15 changed files with 539 additions and 141 deletions
--- a/internal/app/run/runner.go
+++ b/internal/app/run/runner.go
@@ -89,7 +89,8 @@ func NewRunner(cfg *config.Config, reg *config.Registration, cli client.Client)
 	var cacheHandler *artifactcache.Handler
 	if cfg.Cache.Enabled == nil || *cfg.Cache.Enabled {
 		if cfg.Cache.ExternalServer != "" {
-			envs["ACTIONS_CACHE_URL"] = cfg.Cache.ExternalServer
+			// The v1 client appends its path to this without a separator, so the slash is required.
+			envs["ACTIONS_CACHE_URL"] = strings.TrimRight(cfg.Cache.ExternalServer, "/") + "/"
 		} else {
 			warnIgnoredCacheSecret(cfg)
 			handler, err := artifactcache.StartHandler(
@@ -109,12 +110,6 @@ func NewRunner(cfg *config.Config, reg *config.Registration, cli client.Client)
 		}
 	}

-	if envs["ACTIONS_CACHE_URL"] != "" && (cfg.Cache.V2 == nil || *cfg.Cache.V2) {
-		// act patches the GHES check out of an action's bundle when it sees this, so the client
-		// uses the cache service v2 API this server also answers; see act/runner/toolkit_patch.go.
-		envs[runner.CacheServiceV2Env] = "true"
-	}
-
 	// set artifact gitea api
 	artifactGiteaAPI := strings.TrimSuffix(cli.Address(), "/") + "/api/actions_pipeline/"
 	envs["ACTIONS_RUNTIME_URL"] = artifactGiteaAPI
@@ -440,7 +435,13 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 	// lifetime. Only applies to the embedded cache server; when the operator
 	// points the runner at an external cache via cfg.Cache.ExternalServer, it
 	// is that server's responsibility to authenticate requests.
-	defer r.registerCacheForTask(giteaRuntimeToken, preset.Repository, reporter)()
+	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.
+	if resultsURL != "" {
+		envs["ACTIONS_RESULTS_URL"], envs[runner.CacheServiceV2Env] = resultsURL, "true"
+	}

 	eventJSON, err := json.Marshal(preset.Event)
 	if err != nil {
@@ -481,6 +482,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(),

 		ReuseContainers:      false,
 		ForcePull:            r.cfg.Container.ForcePull,
@@ -553,6 +555,12 @@ 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)
+}
+
 // registerCacheForTask tells the cache server to accept requests authenticated
 // with the given runtime token for the duration of this task. Returns a
 // function the caller must invoke (typically via defer) to revoke the
@@ -565,18 +573,25 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 //     repo scoping over the network.
 //
 // Safe with an empty token (older Gitea did not issue one).
-func (r *Runner) registerCacheForTask(token, repo string, reporter *report.Reporter) func() {
+// It also returns the URL to advertise as ACTIONS_RESULTS_URL, which is the cache server itself
+// when it agreed to forward this instance's artifact service, and "" when it did not.
+func (r *Runner) registerCacheForTask(token, repo string, reporter *report.Reporter) (func(), string) {
 	if token == "" {
-		return func() {}
+		return func() {}, ""
+	}
+	cred := artifactcache.JobCredential{
+		Repo:        repo,
+		Results:     r.envs["ACTIONS_RESULTS_URL"], // the instance as the job would reach it
+		InsecureTLS: r.cfg.Runner.Insecure,
 	}
 	if r.cacheHandler != nil {
-		return r.cacheHandler.RegisterJob(token, repo)
+		return r.cacheHandler.RegisterJob(token, cred), r.cacheHandler.ResultsURL(cred)
 	}
 	if r.cfg.Cache.ExternalServer != "" && r.cfg.Cache.ExternalSecret != "" {
-		return r.registerExternalCacheJob(token, repo, reporter)
+		return r.registerExternalCacheJob(token, cred, reporter)
 	}
 	// No cache server to register against: caching is disabled, or the built-in server failed to start.
-	return func() {}
+	return func() {}, ""
 }

 // registerExternalCacheJob POSTs to the remote cache-server's control-plane.
@@ -584,47 +599,54 @@ func (r *Runner) registerCacheForTask(token, repo string, reporter *report.Repor
 // 401 the job's requests — better than failing the whole task for a cache
 // outage. The warning is mirrored to the job log so users can see why their
 // cache calls 401, instead of having to read the runner daemon's stderr.
-func (r *Runner) registerExternalCacheJob(token, repo string, reporter *report.Reporter) func() {
+func (r *Runner) registerExternalCacheJob(token string, cred artifactcache.JobCredential, reporter *report.Reporter) (func(), string) {
 	base := strings.TrimRight(r.cfg.Cache.ExternalServer, "/")
-	if err := postInternalCache(base+"/_internal/register", r.cfg.Cache.ExternalSecret,
-		map[string]string{"token": token, "repo": repo}); err != nil {
+	resultsURL := ""
+	if body, err := postInternalCache(base+"/_internal/register", r.cfg.Cache.ExternalSecret, map[string]any{
+		"token": token, "repo": cred.Repo, "results": cred.Results, "insecure_tls": cred.InsecureTLS,
+	}); err != nil {
 		log.Warnf("cache external_server register failed (%s): %v", base, err)
 		if reporter != nil {
 			reporter.Logf("::warning::cache external_server register failed (%s): %v — cache requests from this job will be unauthenticated and likely return 401", base, err)
 		}
+	} else {
+		resultsURL, _ = body["results_url"].(string) // absent from a server too old to forward
 	}
 	return func() {
-		if err := postInternalCache(base+"/_internal/revoke", r.cfg.Cache.ExternalSecret,
-			map[string]string{"token": token}); err != nil {
+		if _, err := postInternalCache(base+"/_internal/revoke", r.cfg.Cache.ExternalSecret,
+			map[string]any{"token": token}); err != nil {
 			log.Warnf("cache external_server revoke failed (%s): %v", base, err)
 			if reporter != nil {
 				reporter.Logf("::warning::cache external_server revoke failed (%s): %v", base, err)
 			}
 		}
-	}
+	}, resultsURL
 }

-func postInternalCache(url, secret string, body map[string]string) error {
+func postInternalCache(url, secret string, body map[string]any) (map[string]any, error) {
 	buf, err := json.Marshal(body)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	req, err := http.NewRequest(http.MethodPost, url, bytes.NewReader(buf))
 	if err != nil {
-		return err
+		return nil, err
 	}
 	req.Header.Set("Authorization", "Bearer "+secret)
 	req.Header.Set("Content-Type", "application/json")
 	client := &http.Client{Timeout: 5 * time.Second}
 	resp, err := client.Do(req)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode/100 != 2 {
-		return fmt.Errorf("status %d", resp.StatusCode)
+		return nil, fmt.Errorf("status %d", resp.StatusCode)
 	}
-	return nil
+	answer := map[string]any{}
+	// A server too old to answer with a body is not an error, it simply tells us nothing.
+	_ = json.NewDecoder(resp.Body).Decode(&answer)
+	return answer, nil
 }

 func (r *Runner) RunningCount() int64 {
--- a/internal/app/run/runner_cache_test.go
+++ b/internal/app/run/runner_cache_test.go
@@ -7,7 +7,9 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
+	"net/http/httptest"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -29,7 +31,7 @@ func TestRunner_registerCacheForTask(t *testing.T) {

 	r := &Runner{cfg: emptyCfg(), cacheHandler: handler}
 	token := "run-token-123"
-	unregister := r.registerCacheForTask(token, "owner/repo", nil)
+	unregister, _ := r.registerCacheForTask(token, "owner/repo", nil)

 	base := handler.ExternalURL() + "/_apis/artifactcache"
 	probe := func() int {
@@ -53,7 +55,7 @@ func TestRunner_registerCacheForTask(t *testing.T) {
 func TestRunner_registerCacheForTask_NoOps(t *testing.T) {
 	t.Run("nil cacheHandler", func(t *testing.T) {
 		r := &Runner{cfg: emptyCfg()}
-		unregister := r.registerCacheForTask("tok", "owner/repo", nil)
+		unregister, _ := r.registerCacheForTask("tok", "owner/repo", nil)
 		require.NotNil(t, unregister)
 		unregister()
 	})
@@ -65,7 +67,7 @@ func TestRunner_registerCacheForTask_NoOps(t *testing.T) {
 		defer handler.Close()

 		r := &Runner{cfg: emptyCfg(), cacheHandler: handler}
-		unregister := r.registerCacheForTask("", "owner/repo", nil)
+		unregister, _ := r.registerCacheForTask("", "owner/repo", nil)
 		require.NotNil(t, unregister)
 		unregister()
 	})
@@ -81,7 +83,7 @@ func TestRunner_CacheFullFlow_MatchesToolkit(t *testing.T) {

 	r := &Runner{cfg: emptyCfg(), cacheHandler: handler}
 	token := "full-flow-token"
-	unregister := r.registerCacheForTask(token, "owner/repo", nil)
+	unregister, _ := r.registerCacheForTask(token, "owner/repo", nil)
 	defer unregister()

 	base := handler.ExternalURL() + "/_apis/artifactcache"
@@ -154,18 +156,27 @@ func decodeJSON(resp *http.Response, v any) error {
 }

 // End-to-end against a remote cache-server: token unknown → 401, register →
-// reserve/upload/commit/find/download all OK, revoke → 401 again.
+// reserve/upload/commit/find/download all OK, revoke → 401 again. Registering also names the
+// instance, and the server answering with its own address is what makes a shared cache server the
+// whole results service, as the built-in one is.
 func TestRunner_ExternalCacheServer_RegisterRevoke(t *testing.T) {
 	dir := filepath.Join(t.TempDir(), "remote-cache")
 	const secret = "shared-secret-for-tests"
 	remote, err := artifactcache.StartHandler(dir, "127.0.0.1", 0, secret, nil)
 	require.NoError(t, err)
 	defer remote.Close()
+	gitea := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = io.WriteString(w, `{"ok":true}`)
+	}))
+	defer gitea.Close()

-	r := &Runner{cfg: &config.Config{Cache: config.Cache{
-		ExternalServer: remote.ExternalURL(),
-		ExternalSecret: secret,
-	}}}
+	r := &Runner{
+		cfg: &config.Config{Cache: config.Cache{
+			ExternalServer: remote.ExternalURL(),
+			ExternalSecret: secret,
+		}},
+		envs: map[string]string{"ACTIONS_RESULTS_URL": gitea.URL},
+	}

 	token := "external-task-token"
 	repo := "owner/repoX"
@@ -182,10 +193,21 @@ func TestRunner_ExternalCacheServer_RegisterRevoke(t *testing.T) {
 	require.Equal(t, http.StatusUnauthorized, probe(),
 		"token must be unknown to the remote server before registration")

-	unregister := r.registerCacheForTask(token, repo, nil)
+	unregister, resultsURL := r.registerCacheForTask(token, repo, nil)
 	require.NotEqual(t, http.StatusUnauthorized, probe(),
 		"token must be accepted after registerCacheForTask")

+	// The server took the results service over, so the artifact half reaches Gitea through it.
+	require.Equal(t, remote.ExternalURL(), resultsURL)
+	artifact, err := http.NewRequestWithContext(t.Context(), http.MethodPost,
+		resultsURL+"/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", nil)
+	require.NoError(t, err)
+	artifact.Header.Set("Authorization", "Bearer "+token)
+	forwarded, err := http.DefaultClient.Do(artifact)
+	require.NoError(t, err)
+	forwarded.Body.Close()
+	require.Equal(t, http.StatusOK, forwarded.StatusCode)
+
 	// Full reserve→upload→commit→find→download cycle, identical to what
 	// @actions/cache does, against the remote (external) server.
 	body := []byte("payload-from-task")
--- a/internal/app/run/runner_test.go
+++ b/internal/app/run/runner_test.go
@@ -5,6 +5,8 @@ package run

 import (
 	"context"
+	"net/http"
+	"strings"
 	"testing"

 	"gitea.com/gitea/runner/act/runner"
@@ -124,21 +126,57 @@ func taskWithDefaultActionsURL(url string) *runnerv1.Task {
 	}
 }

-// The cache service v2 API is announced to jobs unless it is turned off. Announcing it is what
-// makes act patch the GHES check out of an action's bundle, so the client can reach it.
+// The results service is decided per task, because that is where the job's token and its
+// instance are both known. NewRunner only leaves the address Gitea serves.
 func TestNewRunnerCacheServiceV2(t *testing.T) {
-	announced := func(v2 *bool) string {
-		cfg := &config.Config{}
-		cfg.Cache.V2, cfg.Cache.Dir, cfg.Cache.Host = v2, t.TempDir(), "127.0.0.1"
-		cli := clientmocks.NewClient(t)
-		cli.On("Address").Return("https://gitea.example/").Maybe()
+	cfg := &config.Config{}
+	cfg.Cache.Dir, cfg.Cache.Host = t.TempDir(), "127.0.0.1"
+	cli := clientmocks.NewClient(t)
+	cli.On("Address").Return("https://gitea.example/").Maybe()

-		r := NewRunner(cfg, &config.Registration{Name: "runner"}, cli)
-		t.Cleanup(func() { _ = r.Close() })
-		return r.envs[runner.CacheServiceV2Env]
-	}
+	r := NewRunner(cfg, &config.Registration{Name: "runner"}, cli)
+	t.Cleanup(func() { _ = r.Close() })
+	const token = "task-token"

-	off := false
-	assert.Equal(t, "true", announced(nil))
-	assert.Empty(t, announced(&off))
+	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.
+	revoke, resultsURL := r.registerCacheForTask(token, "owner/repo", nil)
+	defer revoke()
+	require.Equal(t, r.cacheHandler.ExternalURL(), resultsURL)
+
+	// And what is advertised has to answer the cache service. A client without the GHES escape
+	// hatch, docker buildx among them, posts its cache calls at exactly this URL and nowhere else,
+	// so a 404 here is the failure this whole change is about.
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodPost,
+		resultsURL+"/twirp/github.actions.results.api.v1.CacheService/GetCacheEntryDownloadURL",
+		strings.NewReader(`{"key":"k","version":"v"}`))
+	require.NoError(t, err)
+	req.Header.Set("Authorization", "Bearer "+token)
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	assert.Equal(t, http.StatusOK, resp.StatusCode, "the advertised results service serves no cache service")
+}
+
+// The v1 cache client appends its path to ACTIONS_CACHE_URL without a separator, so a configured
+// server that is missing the slash would send it to a URL whose port swallows the path.
+func TestNewRunnerNormalizesTheExternalCacheServer(t *testing.T) {
+	cfg := &config.Config{}
+	cfg.Cache.ExternalServer = "http://cache.local:8088//"
+	cli := clientmocks.NewClient(t)
+	cli.On("Address").Return("https://gitea.example/").Maybe()
+
+	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.
+	assert.Equal(t, "https://gitea.example", r.envs["ACTIONS_RESULTS_URL"])
+	assert.Empty(t, r.envs[runner.CacheServiceV2Env])
+	assert.True(t, r.patchToolkit())
 }