fix(#6702): use conditional requests for behaviour-suite polling GETs - #6797
fix(#6702): use conditional requests for behaviour-suite polling GETs#6797waynesun09 wants to merge 1 commit into
Conversation
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 <gsun@redhat.com>
PR Summary by QodoUse conditional requests for GitHub Actions polling
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 1:38 PM UTC · Completed 1:55 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.54 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Cache lacks byte bound
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if c.etagCache == nil { | ||
| c.etagCache = make(map[string]etagEntry) | ||
| } | ||
| c.etagCache[path] = etagEntry{etag: newETag, body: data} |
There was a problem hiding this comment.
1. Concurrent cache state regresses 🐞 Bug ≡ Correctness
Concurrent getCached calls for the same path can both snapshot an old entry and then write responses out of order, allowing an older ETag/body to overwrite a newer one or an old 304 snapshot to be returned after newer data was cached. The shared behaviour-suite client is used by concurrent polling scenarios, so this can transiently regress observed workflow state and force another quota-consuming 200 to repair the cache.
Agent Prompt
## Issue description
Concurrent same-path requests can overwrite a newer cache entry with an older response because the network request is outside the lock and the final assignment is unconditional.
## Issue Context
The behaviour suite shares one GitHub client across concurrent scenarios, and multiple polling paths can overlap.
## Fix Focus Areas
- internal/forge/github/github.go[535-566]
- e2e/behaviour/suite_test.go[126-135]
- pkg/behaviourtest/drivers/ci/githubactions/githubactions.go[115-128]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if c.etagCache == nil { | ||
| c.etagCache = make(map[string]etagEntry) | ||
| } | ||
| c.etagCache[path] = etagEntry{etag: newETag, body: data} |
There was a problem hiding this comment.
2. Invalid payloads become sticky 🐞 Bug ☼ Reliability
getCached stores every ETagged 200 response before any endpoint validates its JSON, so a corrupted or malformed representation can be replayed on every subsequent 304 and repeatedly fail decoding. This turns a one-request payload failure into a persistent polling failure until GitHub changes the ETag or the entry is evicted.
Agent Prompt
## Issue description
An ETagged body is committed to the cache before callers validate that it can be decoded, allowing malformed content to remain sticky across 304 responses.
## Issue Context
All five converted list methods unmarshal only after `getCached` has already stored the bytes.
## Fix Focus Areas
- internal/forge/github/github.go[545-568]
- internal/forge/github/github.go[3385-3401]
- internal/forge/github/github.go[3426-3442]
- internal/forge/github/github.go[3461-3474]
- internal/forge/github/github.go[3490-3501]
- internal/forge/github/github.go[3546-3562]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return prev.body, nil | ||
| } | ||
| defer resp.Body.Close() | ||
| data, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
3. Cache lacks byte bound 🐞 Bug ➹ Performance
getCached reads each successful response into an unbounded byte slice and retains up to 256 such slices, so etagCacheLimit bounds entry count but not memory consumption. Large GitHub or intermediary responses can therefore cause substantial transient and long-lived memory growth compared with the previous streaming decode.
Agent Prompt
## Issue description
Successful response bodies are read without a byte limit and retained in an entry-count-only cache, leaving total cache memory unbounded.
## Issue Context
The five listing endpoints previously decoded directly from response streams; the new implementation retains raw bodies for conditional reuse.
## Fix Focus Areas
- internal/forge/github/github.go[553-566]
- internal/forge/github/github.go[62-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Risk Assessment: moderate (2/5) DetailsModerate risk: a focused 245-line fix with 50% test coverage targeting a well-scoped issue, but modifying a high-churn core file (github.go) with 8 recent contributors and extensive fix history; the narrow scope and strong test ratio keep overall risk contained. |
ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
1 participant
What
Follow-up to #6705's instrumentation. That PR measured the harness-wait poll loop draining the shared installation token's primary quota at ~235 req/min, exhausting it ~20 minutes into a suite run (#6702). This is candidate 1 of the two the issue proposed, in order of leverage: conditional requests.
Why this works
GitHub does not count a
304 Not Modifiedagainst the primary rate-limit budget. Verified against the live API before writing any code:Only the original uncached
200consumed a unit. The harness-wait poll loop re-requests the same workflow-runs/jobs/artifacts URLs every few seconds, from up to a dozen concurrent scenarios — most of those polls see an unchanged result while a run is still queued or in progress, so this is the highest-leverage fix for exactly the traffic pattern that exhausts the budget.What changed
LiveClientgains a small conditional-GET cache (etagCache, capped atetagCacheLimit) and agetCached(ctx, path)helper: sendsIf-None-Matchwhen a cached ETag exists, returns the cached body on304, otherwise decodes and caches the new body + ETag.do()grows a variadicrequestHeaderoption sogetConditionalcan setIf-None-Matchwithout touching any of its other 28 call sites.ListWorkflowRuns,ListRecentWorkflowRuns,ListWorkflowRunJobs,ListWorkflowRunArtifacts,ListRepositoryArtifacts. Nothing else opts in —get()(used by writes, one-shot reads) is untouched.What's deliberately not in this PR
403 retryablediagnostics, that's the next PR, sized from real post-fix numbers.Correctness risk worth naming
If GitHub's ETag on a listing ever failed to change when a run's
statuschanged, the poller would silently see stale data — the exact flake class this fixes, reintroduced worse. Not unit-testable; the behaviour job on this PR is the real check (a stuck poll would time out, and #6698's diagnostics now name errors instead of swallowing them).Testing
go build ./...,go vet ./...,gofmt -lcleango test -race ./internal/forge/... ./pkg/behaviourtest/...— all passinternal/forge/github/github_test.go: conditional request reuses a304and decodes the cached body; a changed ETag forces a real re-fetch (not stale data); a response without an ETag is never cached;etagCachestays bounded across many distinct URLs