From 126b20bd2c494d842ef2e3719541ad39640e5399 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 31 Aug 2026 09:35:45 -0400 Subject: [PATCH] fix(#6702): use conditional requests for behaviour-suite polling GETs The harness-wait poll loop (and its diagnostics) re-request the same workflow-runs/jobs/artifacts URLs every few seconds, from up to a dozen concurrent scenarios sharing one installation token. #6705's instrumentation measured that traffic draining the primary quota ~235 req/min, exhausting it ~20 minutes into a suite run. GitHub does not count a 304 response against the primary rate-limit budget (verified against the live API: repeated If-None-Match requests left X-RateLimit-Remaining unchanged, only the initial uncached GET consumed one unit). This adds a small conditional-GET cache to LiveClient (etagCache, opt-in per path via getCached) and wires it into the five GET endpoints the harness-wait poll loop and its diagnostics use: ListWorkflowRuns, ListRecentWorkflowRuns, ListWorkflowRunJobs, ListWorkflowRunArtifacts, and ListRepositoryArtifacts. Unchanged results between polls now cost nothing; a status change still forces a full re-fetch, since GitHub issues a new ETag whenever the underlying data changes. do() grows a variadic requestHeader option so getConditional can set If-None-Match without touching its other 28 call sites. The cache is capped (etagCacheLimit) since a long suite run touches many distinct run/job/artifact URLs. Deliberately out of scope: a remaining-budget circuit breaker (the issue's second candidate). If post-merge behaviour runs still show #6698's 403 diagnostics after this lands, that's the next step, sized with real data instead of a guessed threshold. Assisted-by: Claude Signed-off-by: Wayne Sun --- internal/forge/github/github.go | 125 ++++++++++++++++++++++++--- internal/forge/github/github_test.go | 120 +++++++++++++++++++++++++ 2 files changed, 234 insertions(+), 11 deletions(-) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 49a57f989..ccae1708c 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -41,8 +41,30 @@ type LiveClient struct { rateMu sync.Mutex rate forge.RateLimit rateSeen bool + + // etagCache holds the last ETag and decoded body seen for GET paths + // that opt into conditional requests via getCached (#6702). A 304 + // response reusing a cached ETag does not count against the primary + // rate-limit budget, which is what makes this worth doing for the + // behaviour-test harness-wait poll loop: the same workflow-runs URL + // is requested every few seconds by up to a dozen concurrent + // scenarios sharing one installation token. + etagMu sync.Mutex + etagCache map[string]etagEntry +} + +// etagEntry is one cached (ETag, body) pair for a GET path. +type etagEntry struct { + etag string + body []byte } +// etagCacheLimit bounds etagCache so a long-lived client (the behaviour +// suite runs many scenarios against many distinct run/job/artifact URLs) +// cannot grow the map without bound. Crude but sufficient: clear it and +// let it refill rather than evicting individual entries. +const etagCacheLimit = 256 + // Compile-time interface checks. var _ forge.Client = (*LiveClient)(nil) var _ forge.GitHubExtensions = (*LiveClient)(nil) @@ -213,8 +235,20 @@ func IsPATForbiddenError(err error) bool { const maxRetries = 5 +// requestHeader is one extra header to set on a do() request. Only +// getCached uses this today (If-None-Match); it exists as a variadic +// option rather than a new do() overload so the other 28 call sites +// stay untouched. +type requestHeader struct { + key, value string +} + +func withHeader(key, value string) requestHeader { + return requestHeader{key: key, value: value} +} + // do performs an HTTP request against the GitHub API with retry on rate limits. -func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*http.Response, error) { +func (c *LiveClient) do(ctx context.Context, method, path string, body any, headers ...requestHeader) (*http.Response, error) { url := c.baseURL + path var bodyData []byte @@ -245,6 +279,9 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht if body != nil { req.Header.Set("Content-Type", "application/json") } + for _, h := range headers { + req.Header.Set(h.key, h.value) + } resp, err := c.http.Do(req) if err == nil { @@ -465,6 +502,72 @@ func (c *LiveClient) get(ctx context.Context, path string) (*http.Response, erro return resp, nil } +// getConditional performs a GET request, sending If-None-Match with etag +// when non-empty. notModified is true when the server confirmed the +// cached etag is still current (304); resp is nil in that case and the +// caller must reuse its previously cached body. GitHub does not count a +// 304 against the primary rate-limit budget (verified 2026-08-31: three +// consecutive conditional requests left X-RateLimit-Remaining unchanged). +func (c *LiveClient) getConditional(ctx context.Context, path, etag string) (resp *http.Response, notModified bool, err error) { + var headers []requestHeader + if etag != "" { + headers = append(headers, withHeader("If-None-Match", etag)) + } + resp, err = c.do(ctx, http.MethodGet, path, nil, headers...) + if err != nil { + return nil, false, err + } + if resp.StatusCode == http.StatusNotModified { + resp.Body.Close() + return nil, true, nil + } + if err := checkStatus(resp, http.StatusOK); err != nil { + return nil, false, err + } + return resp, false, nil +} + +// getCached performs a conditional GET against path, transparently +// reusing the cached body on a 304. Only worth it for GET paths a +// caller polls repeatedly with an unchanged result most of the time — +// see etagCache's doc comment. A response without an ETag header is +// returned but not cached (nothing to send next time). +func (c *LiveClient) getCached(ctx context.Context, path string) ([]byte, error) { + c.etagMu.Lock() + prev, ok := c.etagCache[path] + c.etagMu.Unlock() + + etag := "" + if ok { + etag = prev.etag + } + + resp, notModified, err := c.getConditional(ctx, path, etag) + if err != nil { + return nil, err + } + if notModified { + return prev.body, nil + } + defer resp.Body.Close() + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read response: %w", err) + } + if newETag := resp.Header.Get("ETag"); newETag != "" { + c.etagMu.Lock() + if len(c.etagCache) >= etagCacheLimit { + clear(c.etagCache) + } + if c.etagCache == nil { + c.etagCache = make(map[string]etagEntry) + } + c.etagCache[path] = etagEntry{etag: newETag, body: data} + c.etagMu.Unlock() + } + return data, nil +} + // post performs a POST request and checks for success. func (c *LiveClient) post(ctx context.Context, path string, body any) (*http.Response, error) { resp, err := c.do(ctx, http.MethodPost, path, body) @@ -3279,7 +3382,7 @@ func (c *LiveClient) awaitBranchUpdate(ctx context.Context, owner, repo string, // ListWorkflowRuns returns recent workflow runs for a workflow file. func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflowFile string) ([]forge.WorkflowRun, error) { - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/actions/workflows/%s/runs?per_page=10", owner, repo, workflowFile)) + data, err := c.getCached(ctx, fmt.Sprintf("/repos/%s/%s/actions/workflows/%s/runs?per_page=10", owner, repo, workflowFile)) if err != nil { return nil, fmt.Errorf("list workflow runs: %w", err) } @@ -3294,7 +3397,7 @@ func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflow CreatedAt string `json:"created_at"` } `json:"workflow_runs"` } - if err := decodeJSON(resp, &result); err != nil { + if err := json.Unmarshal(data, &result); err != nil { return nil, fmt.Errorf("decode workflow runs: %w", err) } runs := make([]forge.WorkflowRun, len(result.WorkflowRuns)) @@ -3320,7 +3423,7 @@ func (c *LiveClient) ListRecentWorkflowRuns(ctx context.Context, owner, repo str if perPage > 100 { perPage = 100 } - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs?per_page=%d", owner, repo, perPage)) + data, err := c.getCached(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs?per_page=%d", owner, repo, perPage)) if err != nil { return nil, fmt.Errorf("list recent workflow runs: %w", err) } @@ -3335,7 +3438,7 @@ func (c *LiveClient) ListRecentWorkflowRuns(ctx context.Context, owner, repo str CreatedAt string `json:"created_at"` } `json:"workflow_runs"` } - if err := decodeJSON(resp, &result); err != nil { + if err := json.Unmarshal(data, &result); err != nil { return nil, fmt.Errorf("decode recent workflow runs: %w", err) } runs := make([]forge.WorkflowRun, len(result.WorkflowRuns)) @@ -3355,7 +3458,7 @@ func (c *LiveClient) ListRecentWorkflowRuns(ctx context.Context, owner, repo str // ListWorkflowRunJobs returns the jobs within a workflow run. func (c *LiveClient) ListWorkflowRunJobs(ctx context.Context, owner, repo string, runID int) ([]forge.WorkflowJob, error) { - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs/%d/jobs?per_page=100", owner, repo, runID)) + data, err := c.getCached(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs/%d/jobs?per_page=100", owner, repo, runID)) if err != nil { return nil, fmt.Errorf("list workflow run jobs: %w", err) } @@ -3367,7 +3470,7 @@ func (c *LiveClient) ListWorkflowRunJobs(ctx context.Context, owner, repo string Conclusion string `json:"conclusion"` } `json:"jobs"` } - if err := decodeJSON(resp, &result); err != nil { + if err := json.Unmarshal(data, &result); err != nil { return nil, fmt.Errorf("decode workflow run jobs: %w", err) } jobs := make([]forge.WorkflowJob, len(result.Jobs)) @@ -3384,7 +3487,7 @@ func (c *LiveClient) ListWorkflowRunJobs(ctx context.Context, owner, repo string // ListWorkflowRunArtifacts returns artifacts uploaded by a workflow run. func (c *LiveClient) ListWorkflowRunArtifacts(ctx context.Context, owner, repo string, runID int) ([]forge.WorkflowArtifact, error) { - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs/%d/artifacts", owner, repo, runID)) + data, err := c.getCached(ctx, fmt.Sprintf("/repos/%s/%s/actions/runs/%d/artifacts", owner, repo, runID)) if err != nil { return nil, fmt.Errorf("list workflow run artifacts: %w", err) } @@ -3394,7 +3497,7 @@ func (c *LiveClient) ListWorkflowRunArtifacts(ctx context.Context, owner, repo s Name string `json:"name"` } `json:"artifacts"` } - if err := decodeJSON(resp, &result); err != nil { + if err := json.Unmarshal(data, &result); err != nil { return nil, fmt.Errorf("decode workflow run artifacts: %w", err) } artifacts := make([]forge.WorkflowArtifact, len(result.Artifacts)) @@ -3440,7 +3543,7 @@ func (c *LiveClient) ListRepositoryArtifacts(ctx context.Context, owner, repo st if perPage > 100 { perPage = 100 } - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/actions/artifacts?per_page=%d", owner, repo, perPage)) + data, err := c.getCached(ctx, fmt.Sprintf("/repos/%s/%s/actions/artifacts?per_page=%d", owner, repo, perPage)) if err != nil { return nil, fmt.Errorf("list repository artifacts: %w", err) } @@ -3455,7 +3558,7 @@ func (c *LiveClient) ListRepositoryArtifacts(ctx context.Context, owner, repo st } `json:"workflow_run"` } `json:"artifacts"` } - if err := decodeJSON(resp, &result); err != nil { + if err := json.Unmarshal(data, &result); err != nil { return nil, fmt.Errorf("decode repository artifacts: %w", err) } artifacts := make([]forge.RepositoryArtifact, 0, len(result.Artifacts)) diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index e1d51a643..7327d4edd 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -3637,6 +3637,126 @@ func TestListWorkflowRuns_IncludesEvent(t *testing.T) { assert.Equal(t, "issues", runs[0].Event) } +// TestGetCached_ConditionalRequestReuses304 exercises the #6702 fix +// through ListWorkflowRuns: the first request has no If-None-Match, the +// server returns 200 with an ETag; the second request must send that +// exact ETag back, and on 304 the client must decode the cached body +// rather than an empty one. +func TestGetCached_ConditionalRequestReuses304(t *testing.T) { + const etag = `W/"abc123"` + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + switch calls { + case 1: + assert.Empty(t, r.Header.Get("If-None-Match"), "first request must not send If-None-Match") + w.Header().Set("ETag", etag) + json.NewEncoder(w).Encode(map[string]any{ + "workflow_runs": []map[string]any{ + {"id": 1, "status": "in_progress", "created_at": "2024-01-01T00:00:00Z"}, + }, + }) + case 2: + assert.Equal(t, etag, r.Header.Get("If-None-Match"), "second request must echo the weak ETag verbatim") + w.Header().Set("ETag", etag) + w.WriteHeader(http.StatusNotModified) + default: + t.Fatalf("unexpected call %d", calls) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + first, err := client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + require.Len(t, first, 1) + + second, err := client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + require.Len(t, second, 1, "304 must decode to the cached body, not an empty one") + assert.Equal(t, first[0].ID, second[0].ID) + assert.Equal(t, "in_progress", second[0].Status) + assert.Equal(t, 2, calls) +} + +// TestGetCached_ChangedETagRefetchesBody guards against the flake class +// this fix could reintroduce if done wrong: a status change must always +// come with a new ETag from the (real) server, and the client must not +// keep serving a stale cached body once the ETag changes. +func TestGetCached_ChangedETagRefetchesBody(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + status := "in_progress" + etag := `"v1"` + if calls > 1 { + status = "completed" + etag = `"v2"` + } + w.Header().Set("ETag", etag) + json.NewEncoder(w).Encode(map[string]any{ + "workflow_runs": []map[string]any{ + {"id": 1, "status": status, "created_at": "2024-01-01T00:00:00Z"}, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + first, err := client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + require.Equal(t, "in_progress", first[0].Status) + + second, err := client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + require.Equal(t, "completed", second[0].Status) +} + +// TestGetCached_NoETagNotCached ensures a response without an ETag +// header is decoded normally and never triggers a conditional request +// on the next call — there is nothing to send. +func TestGetCached_NoETagNotCached(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + assert.Empty(t, r.Header.Get("If-None-Match")) + json.NewEncoder(w).Encode(map[string]any{ + "workflow_runs": []map[string]any{ + {"id": 1, "status": "in_progress", "created_at": "2024-01-01T00:00:00Z"}, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + _, err = client.ListWorkflowRuns(context.Background(), "org", "repo", "fullsend.yaml") + require.NoError(t, err) + assert.Equal(t, 2, calls) +} + +// TestEtagCache_Bounded ensures a long-lived client polling many +// distinct URLs (one per workflow run, in the behaviour suite) does not +// grow etagCache without bound. +func TestEtagCache_Bounded(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"`+r.URL.Path+`"`) + json.NewEncoder(w).Encode(map[string]any{"jobs": []map[string]any{}}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + for i := range etagCacheLimit * 2 { + _, err := client.ListWorkflowRunJobs(context.Background(), "org", "repo", i) + require.NoError(t, err) + } + client.etagMu.Lock() + size := len(client.etagCache) + client.etagMu.Unlock() + assert.LessOrEqual(t, size, etagCacheLimit) +} + func TestListWorkflowRunJobs(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/repos/org/repo/actions/runs/42/jobs", r.URL.Path)