fix(#6702): use conditional requests for behaviour-suite polling GETs

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))

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)