fix(reconciler): unblock queue on provider failure - #2953
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2953 +/- ##
==========================================
+ Coverage 80.66% 80.82% +0.15%
==========================================
Files 164 164
Lines 13910 13919 +9
==========================================
+ Hits 11221 11250 +29
+ Misses 1967 1944 -23
- Partials 722 725 +3
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 ✅Fixes a bug in the reconciler where a failure to detect the git provider for a finished PipelineRun caused reconcileKind to return nil instead of propagating the error, silently dropping the workqueue key so reportFinalStatus never ran and the repository's concurrency slot was never released. The change now wraps and returns the detectProvider error so the standard controller retry mechanism kicks in. A new unit test verifies that ReconcileKind returns an error containing "detect provider" when the git-provider annotation is missing. Review difficulty: 2/5 (Easy) — The change is small and localized to a single error-handling branch, but it alters reconciler retry/requeue behavior, which has some blast radius even though it is well tested. No new review comments found at this time. Nice work! Reviewed commit: f67bef4 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation edge case in the PipelineRun watcher where failures during provider detection were previously swallowed, preventing reportFinalStatus from running and thereby leaving concurrency queue slots unreleased. By surfacing the provider-detection failure as a real reconcile error, Kubernetes/Knative can retry reconciliation and allow transient provider/client setup issues to self-heal.
Changes:
- Return an error (instead of
nil) whendetectProviderfails for a finished PipelineRun, enabling automatic retries. - Add a unit test asserting
ReconcileKindreturns an error when provider detection fails for a done PipelineRun.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/reconciler/reconciler.go | Return an error on provider detection failure so the reconcile is retried and reportFinalStatus isn’t skipped permanently. |
| pkg/reconciler/reconciler_test.go | Add a regression test ensuring provider detection failures now propagate as reconcile errors. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f67bef4 to
7ed269a
Compare
ffb72d7 to
c07927a
Compare
When a PipelineRun finishes, the watcher works out which Git provider it belongs to so it can report the result back. If that lookup failed, the watcher gave up and reported success. Two things followed from that: nothing ever tried again, and the run kept its place in the repository's concurrency queue. A repository with a concurrency limit would then sit waiting on a slot that was never coming back. Not every one of those failures is hopeless, so they are now told apart. A provider client that failed to start up, say because the API was briefly unreachable, is worth another go. The error is returned so Kubernetes retries the reconcile, and a passing hiccup sorts itself out instead of freezing the queue. A PipelineRun with no git-provider annotation, or one naming a provider we do not support, will never resolve no matter how many times we look. Retrying only spins forever while still holding the slot, so the run is marked failed and its slot handed to whoever is next in the queue. The slot is released before the run is marked failed. A run marked failed is skipped on later passes, so doing it the other way round would strand the slot for good if we died between the two writes. Fixes #2946 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Assisted-by: Claude Opus 5 (via Copilot CLI)
c07927a to
8eb1bbf
Compare
chmouel
changed the title
|
/retest |
📝 Description of the Change
When a PipelineRun finishes, the watcher needs to work out which Git provider it
came from so it can report the result back. If that lookup failed, the watcher
gave up and reported success. Nothing ever tried again, and the finished run kept
its place in the repository's concurrency queue. For a repository with a
concurrency limit, that meant waiting forever on a slot that was never coming
back.
These failures are not all alike, so this PR tells them apart.
Some are worth another go. A provider client that could not start up because the
API was briefly unreachable will probably work on the next attempt, so the error
is now reported properly and Kubernetes retries. A passing network hiccup sorts
itself out rather than freezing a repository's queue.
Others are hopeless. A PipelineRun with no git-provider annotation, or one naming
a provider Pipelines-as-Code does not support, will never resolve however many
times we look at it. Retrying those spins forever while still holding the slot,
so they are marked failed and the slot is handed to whatever is next in the
queue.
What you'd notice
started forever.
repository collecting them is visible.
🔗 Linked GitHub Issue
Fixes #2946
🧪 Testing Strategy
Unit tests cover three paths: a recoverable failure returns an error so the
reconcile is retried; a provider that can never be resolved gets its slot
released and the run marked failed; and if writing that final state fails, the
slot is still released so a retry does not promote a second run.
🤖 AI Assistance
Written with AI assistance following a post-merge review of #2890. The bug, the
fix and the tests were checked by hand: the stuck-queue behaviour was reproduced
against the merged code before the fix was written, and the fix and tests
confirmed to pass.
✅ 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.
make lint(goimports inpkg/customparams, staticcheck inpkg/informer/transform) andmake test(TestChangePipelineRun,TestParsePayload) failures seen locally reproduce unchanged onorigin/mainand are unrelated to this PR.