fix(reconciler): stop reporting pipelineRun status when check run id patch fails - #2930
Open
pujitha24 wants to merge 3 commits into
Open
fix(reconciler): stop reporting pipelineRun status when check run id patch fails#2930pujitha24 wants to merge 3 commits into
pujitha24 wants to merge 3 commits into
Conversation
…patch fails Motivation: GitHub App PipelineRuns can get permanently stuck showing "In progress" on GitHub even after they finish. Report: tektoncd#1828 Approach: ReconcileKind's checkRunID gate returned nil unconditionally for any GitHub App pipelineRun missing the CheckRunID annotation, with no way to ever unblock it. Since createCheckRunStatus creates the GitHub check run before action.PatchPipelineRun writes the CheckRunID annotation back onto the object, a resource-version conflict on that patch (more likely with many parallel tasks updating the pipelineRun) leaves the check run created on GitHub as "in_progress" but the annotation never lands. updatePipelineRunToInProgress swallows that error and already marks SCMReportingPLRStarted=true, so the reconciler never retries reporting "in progress" again, and once the pipelineRun finishes the gate blocks it forever: nothing ever reports the final status. Extracted the gate into waitingForCheckRunID and scoped it to only apply while the pipelineRun is still running: !pr.IsDone() && !pr.IsCancelled(). Once it is done or cancelled we proceed even without the annotation. getOrUpdateCheckRunStatus in pkg/provider/github/status.go already falls back to looking up the existing check run by external ID (getExistingCheckRunID) or creating a new one (createCheckRunStatus) when the annotation is missing, so this lets a finished pipelineRun's status land instead of leaving the check run stuck at "in progress" forever. User-visible behavior is unchanged for the common case where the patch succeeds. The fix only changes behavior for the specific failure path described above, where the final status previously never reported at all. Validation: - go build ./... - go test ./pkg/reconciler/... and go test ./pkg/provider/github/... (packages touched by this change and by the affected code path) - make test (full repo test suite) passes - Added TestWaitingForCheckRunID, a table-driven unit test covering: non-GitHub-App pipelineRuns, pipelineRuns with the annotation already set, a still-running pipelineRun missing the annotation (should keep waiting), and a finished pipelineRun missing the annotation (should no longer be gated). This was not validated against a live GitHub App installation reproducing the original resource-version conflict; the fix and its downstream fallback path (existing check-run lookup and creation) are covered by unit tests only. ```release-note Fix pipelineRun status getting permanently stuck as "in progress" on GitHub when the check run id fails to be patched onto the pipelineRun, by allowing a finished pipelineRun to still report its final status. ``` Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a GitHub App reconciliation edge case where a PipelineRun could get permanently blocked from reporting its final GitHub check status if patching the CheckRunID annotation fails (e.g., resource-version conflicts under high parallelism). The change scopes the “wait for CheckRunID” gate to only apply while the PipelineRun is still running, allowing completed runs to proceed and rely on the provider’s existing fallback logic (lookup/create check run without the annotation).
Changes:
- Extracted the “missing
CheckRunID” gate intowaitingForCheckRunIDand limited it to still-runningPipelineRuns. - Updated
ReconcileKindto use the new gate function. - Added a table-driven unit test for
waitingForCheckRunID.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/reconciler/reconciler.go | Adds waitingForCheckRunID and uses it to avoid blocking final status reporting when the annotation never lands. |
| pkg/reconciler/reconciler_test.go | Adds unit coverage for the new gating behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
+123
| // cancelled we stop waiting even if the check run id is still missing, so a | ||
| // pipelineRun whose check run id patch never landed (e.g. it lost a resource | ||
| // conflict) is not stuck reporting "in progress" forever: the provider can |
| }, | ||
| }, | ||
| } | ||
| assert.Equal(t, waitingForCheckRunID(pr), tt.want) |
Comment on lines
+154
to
+158
| name: "GitHub App pipelineRun done without check run id", | ||
| annotations: map[string]string{keys.InstallationID: "1234"}, | ||
| conditions: doneCondition, | ||
| want: false, | ||
| }, |
Two changes from the automated review on tektoncd#2930: - Reword the waitingForCheckRunID doc comment: use CheckRunID consistently rather than alternating with "check run id", and replace the unclear "it lost a resource conflict" with the concrete failure mode (the patch losing a resource-version conflict under high parallelism). - Cover the IsCancelled() branch in TestWaitingForCheckRunID. PipelineRun's IsCancelled() reads Spec.Status, not the Succeeded condition, so the table gains a specStatus field and the pipelineRun under test now sets Spec. Without that the branch was never exercised. Verified by removing the !pr.IsCancelled() guard: only the new case fails, and it passes again once restored. The third suggestion, reversing the assert.Equal arguments, is not applied. This file is mixed, and the adjacent table-driven tests at reconciler_test.go:561-562 use assert.Equal(t, actual, expected) exactly as this one does, so the change would make it less locally consistent, not more. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
Contributor
Author
|
Thanks — went through the automated review. Applied two of the three, and I want to be explicit about the one I didn't. Applied:
Not applied: reversing the Locally: |
zakisk
reviewed
Aug 25, 2026
Comment on lines
+200
to
+221
| // if its a GitHub App pipelineRun PR then process only if check run id is added otherwise wait | ||
| if _, ok := pr.Annotations[keys.InstallationID]; ok { | ||
| if _, ok := pr.Annotations[keys.CheckRunID]; !ok { | ||
| return nil | ||
| } | ||
| if waitingForCheckRunID(pr) { | ||
| return nil |
Member
There was a problem hiding this comment.
I think we should remove any gate here instead because getOrUpdateCheckRunStatus already checks for existing check-run id in checks on PR or via check-run id annotation so no need to check it, wdyt @chmouel ??
getOrUpdateCheckRunStatus already falls back to looking up or creating the check run when the CheckRunID annotation is missing, for both in-progress and final status reports, so the reconciler never actually needs to wait on it. Per review feedback on tektoncd#2930, drop waitingForCheckRunID and its call site instead of just scoping it to still-running PipelineRuns. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Member
|
See #2904 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description of the Change
ReconcileKind's check-run-ID gate returnednilunconditionally for any GitHub AppPipelineRunmissing theCheckRunIDannotation, with no way to ever unblock it. SincecreateCheckRunStatus/startPRcreate the GitHub check run beforeaction.PatchPipelineRunwrites theCheckRunIDannotation back onto the object, a resource-version conflict on that patch (more likely with many parallel tasks updating thePipelineRun) leaves the check run created on GitHub asin_progress/queuedbut the annotation never lands.updatePipelineRunToInProgressswallows that error and already marksSCMReportingPLRStarted=true, so the reconciler never retries reporting that state again, and once thePipelineRunfinishes the gate blocks it forever — nothing ever reports the final status.This PR started out extracting the gate into a
waitingForCheckRunIDhelper and scoping it to still-runningPipelineRuns only. Following review feedback, it now removes the gate entirely instead:getOrUpdateCheckRunStatusinpkg/provider/github/status.goalready falls back to looking up the existing check run by external ID (getExistingCheckRunID) or creating a new one (createCheckRunStatus) whenever theCheckRunIDannotation is missing, for both in-progress and final status reports, so there is no case where the reconciler actually needs to wait on the annotation before proceeding.User-visible behavior is unchanged for the common case where the patch succeeds. The fix only changes behavior for the specific failure path described above, where the final status previously never reported at all.
🔗 Linked GitHub Issue
Fixes #1828
🧪 Testing Strategy
The dedicated
waitingForCheckRunIDgate and its unit test were removed along with the gate itself. Existingpkg/reconcilerandpkg/provider/githubunit tests (which already exercisegetOrUpdateCheckRunStatus's fallback lookup/create path) continue to pass. This was not validated against a live GitHub App installation reproducing the original resource-version conflict.🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.
AI assistance: this change was drafted with Claude Code.