fix: retry gh transient 404 on check-run update - #2922
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2922 +/- ##
==========================================
+ Coverage 85.56% 85.58% +0.02%
==========================================
Files 164 164
Lines 12475 12497 +22
==========================================
+ Hits 10674 10696 +22
Misses 1800 1800
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Paco Review ✅This PR adds a targeted retry mechanism in the GitHub provider so that a check-run update returning HTTP 404 immediately after Pipelines-as-Code itself created that check run is retried a few times with exponential backoff, instead of failing outright, since GitHub can briefly report a freshly-created check run as missing. It introduces Review difficulty: 3/5 (Moderate) — The change is moderate in size but touches a core, widely-used status-update path and introduces new retry/timing logic that needs careful validation of edge cases and test determinism. No new review comments found at this time. Nice work! Reviewed commit: 727bd28 |
There was a problem hiding this comment.
Pull request overview
This PR improves reliability of the GitHub provider by retrying only the check-run update call when GitHub returns a transient HTTP 404 immediately after Pipelines-as-Code created the check run in the same operation (to mitigate GitHub read-after-write lag), while keeping all other 404s terminal.
Changes:
- Add
isNotFoundErrorandupdateCheckRun(...)helper to retryChecks.UpdateCheckRunon explicit 404 with short exponential backoff (500ms, 1s, 2s) when the check run was created “now”. - Wire
getOrUpdateCheckRunStatusto call the new retrying helper only for newly-created check runs. - Add unit tests covering retry/no-retry behavior, context cancellation, and 404 detection; document this retry behavior in the configmap API docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/provider/github/status.go | Adds explicit 404 detection and a bounded backoff retry for check-run update only when the check run was created in the same operation. |
| pkg/provider/github/status_test.go | Adds focused tests for the new retry behavior and helper logic using a fake clock and GitHub test server. |
| docs/content/docs/api/configmap.md | Documents the always-on GitHub transient-404 retry for immediate post-create check-run updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
727bd28 to
e199e3a
Compare
e199e3a to
80ce43d
Compare
80ce43d to
46c35cd
Compare
b25c400 to
2f47f63
Compare
2f47f63 to
82afba7
Compare
9285af6 to
c395ac4
Compare
|
/retest |
c395ac4 to
4505d22
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Retry eligibility must be constrained by recency before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
pkg/provider/github/status_test.go:884
- The fake clock is advanced by a full minute for every retry, so this test only verifies the attempt count; it would still pass if the production backoff were constant or the 500ms/1s/2s sequence regressed. Please drive the clock in sub-threshold and threshold increments (or otherwise observe each timer deadline) and assert that the next API call occurs only after each expected backoff.
fc.Advance(time.Minute)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
I have switched all github.Ptr to new() across the code part of the same commit, just didn't think this warrant a whole new pr /commit since being so trivial |
fedf1f5 to
3d30212
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Retry behavior exceeds its intended scope and budget, and the exponential-backoff timing test is insufficient.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
pkg/provider/github/status.go:317
- Returning non-404 errors (or 404s for IDs found by lookup) here does not make them terminal as described: reconciler status reporting wraps
CreateStatusincreateStatusWithRetry, which calls it three times for every error (pkg/reconciler/status.go:25-29,112-123). Consequently, an annotated 404 can produce up to 12 PATCHes, while server/transport errors and lookup-derived 404s are also retried by the outer loop. Please propagate a non-retryable classification to that caller (or otherwise coordinate the retry layers) so only the intended annotated 404 receives the stated retry budget.
if !retryNotFound || !isNotFoundError(err) || attempt == checkRunUpdateMaxRetries {
return err
- Files reviewed: 47/47 changed files
- Comments generated: 1
- Review effort level: Balanced
| cancel() | ||
| return | ||
| } | ||
| fc.Advance(time.Minute) |
|
after deps PR, you've many conflicts now 🙃 |
3d30212 to
3960ae9
Compare
fixed them |
|
/retest |
|
/test linters |
|
linters. is running in cluster but status here is failure 😕 |
GitHub can answer with 404 for a check run that Pipelines as Code has just created and immediately updates. PAC treated that answer as terminal, so it aborted PipelineRun creation and left the pull request with a failed check that /retest could not recover when triggering is restricted to pull_request events. Create the check run fully formed, with its output, annotations, and conclusion when the run is already finished, so a freshly created check run needs no follow-up update at all. Retry the update briefly when the check-run id came from the PipelineRun annotation, since another reconcile may have created that check run moments earlier and GitHub can still report it as missing. Use three retries at 500ms, 1s, and 2s, which covers the create-to- update lag observed in the report while still failing quickly on a genuine error. Keep every other 404 terminal. An id discovered through a check-run lookup is not retried, so a deleted or inaccessible check run still surfaces as an error instead of being hidden behind repeated requests. Retry only an explicit 404 status response. Transport errors and timeouts are excluded because such a request may already have reached GitHub, and repeating it could apply the same update twice. Return the original error once the retries are exhausted. Leave the generic provider retry transport unchanged. Only this call site knows whether the identifier came from the PipelineRun annotation, which is the condition that makes the retry safe. Fixes tektoncd#2920 Jira: https://issues.redhat.com/browse/SRVKP-13334 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
a7adb03 to
2526e73
Compare
|
Merged since approved and green |
📝 Description of the Change
GitHub can answer with a 404 when Pipelines-as-Code updates a check run
it just created, causing PAC to abort PipelineRun creation and leave
the pull request with a failed check that
/retestcannot recoverwhen triggering is restricted to
pull_requestevents.This fix creates the check run fully formed (output, annotations, and
conclusion when already finished) in a single call, so a freshly
created check run no longer needs an immediate follow-up update. For
check runs whose id is restored from a PipelineRun annotation, the
update is retried on an explicit 404 with a short exponential backoff
(500ms, 1s, 2s), since another reconcile may have created that check
run only moments earlier and GitHub can still report it missing.
Every other 404 remains terminal: an id discovered through a
check-run lookup, or a transport error/timeout, is never retried, so
deleted or inaccessible check runs and requests that may have already
reached GitHub still surface as errors instead of being retried
unsafely.
🔗 Linked GitHub Issue
Fixes #2920
Jira: SRVKP-13334
🧪 Testing Strategy
Comprehensive unit tests were added covering:
🤖 AI Assistance
This PR involved AI-assisted code generation and refinement. The implementation has been thoroughly reviewed and tested to ensure it meets the project's standards and correctly handles the edge cases around check-run creation and update retries.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix any issues.