Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2965 +/- ##
==========================================
+ Coverage 91.57% 91.68% +0.10%
==========================================
Files 164 165 +1
Lines 12532 12718 +186
==========================================
+ Hits 11476 11660 +184
- Misses 1055 1057 +2
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:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new E2E helper can silently “succeed” even if it never finds the target watcher container to patch, and an error message inaccurately refers to an annotation as a label.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Follow-up fixes to Pipelines-as-Code’s concurrency queue and watcher, addressing a crash-loop on malformed execution-order annotations, preventing leaked concurrency slots when provider detection fails on completed PipelineRuns, and gating the /debug/queue endpoint behind an explicit opt-in.
Changes:
- Add a shared, defensive
namespace/namekey parser and apply it to execution-order/queue parsing paths to prevent panics and slot leaks. - Ensure transient provider-detection failures return errors (to trigger retries), while permanent “provider not configured” cases fail the run and release the queue slot.
- Disable
/debug/queueby default and requirePAC_ENABLE_QUEUE_DEBUG=trueto enable it; update E2E helpers, docs, and tests accordingly.
File summaries
| File | Description |
|---|---|
| test/pkg/kubestuff/watcher.go | Adds E2E opt-in helper to enable /debug/queue by patching the watcher Deployment and waiting for rollout. |
| test/gitea_concurrency_test.go | Enables queue debug endpoint before scenarios that assert drained queue state. |
| pkg/reconciler/reconciler.go | Differentiates transient vs permanent provider-detection failures; introduces abandonment path to release slots for done runs with no resolvable provider. |
| pkg/reconciler/reconciler_test.go | Adds regression tests for transient detectProvider errors vs permanent “not configured” behavior and queue-slot release ordering. |
| pkg/reconciler/queue_pipelineruns.go | Uses shared queue key parsing helper when fetching acquired PipelineRuns. |
| pkg/reconciler/finalizer.go | Routes finalizer promotion through the shared “start next from queue” path to avoid key-splitting panics. |
| pkg/reconciler/finalizer_test.go | Adds regression tests ensuring malformed queue keys don’t panic and don’t strand phantom running slots. |
| pkg/reconciler/event.go | Introduces ErrProviderNotConfigured and wraps “missing/unknown provider” errors to classify as permanent. |
| pkg/reconciler/event_test.go | Updates detectProvider tests to assert permanent classification via errors.Is. |
| pkg/queue/queue_manager.go | Skips malformed execution-order entries instead of panicking; logs a warning and normalizes returned keys. |
| pkg/queue/queue_manager_test.go | Adds coverage for malformed execution-order entries mixed with valid ones. |
| pkg/queue/queue_manager_interface.go | Adds SplitPrKey helper to defensively parse namespace/name keys with trimming/validation. |
| pkg/queue/queue_manager_interface_test.go | New unit tests for SplitPrKey validation behavior. |
| docs/content/docs/advanced/concurrency.md | Documents /debug/queue risk, default-off behavior, and explicit enable/disable steps. |
| cmd/pipelines-as-code-watcher/main.go | Gates /debug/queue handler registration behind PAC_ENABLE_QUEUE_DEBUG (fail-closed on invalid values). |
| cmd/pipelines-as-code-watcher/main_test.go | New tests covering PAC_ENABLE_QUEUE_DEBUG parsing and default-off behavior. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Paco Review 🔍This PR introduces an admission-based concurrency queue manager for Pipelines-as-Code. It adds a new Review difficulty: 5/5 (Very Hard) — The change touches the core concurrency slot management across five code paths (initial, resume, promotion, finalization, abandonment), introduces a new multi-phase admission state machine with lock-guarded invariants, changes error semantics for ErrPipelineRunNotStarted and slot release, and adds ~1400 lines of new code including complex recovery tests. 6 new inline comment(s) found. Reviewed commit: cd89bc1 |
pipelines-as-code
Bot
added
paco/review-hard
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
a754bb2 to
7e67624
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The provider-error behavior conflicts with the documented retry requirement, and the debug-toggle test is environment-dependent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
7e67624 to
d39ff62
Compare
d39ff62 to
9554126
Compare
|
The follow-up review I did with while sterring the harness agent found two more queue Both are fixed. Uncertain starts keep their reservations while the watcher These changes are folded into the queue and provider-recovery commits. The |
9554126 to
1c58ea1
Compare
|
More than i think about it and if we want to properly test this is to do some chaos monkey on the cluster (there is some k8s project doing this) and see how well we recover from the chaos. but that's a long and winded road to be able to get there. |
There was a problem hiding this comment.
🟡 Changes recommended
Graceful cancellation states remain startable during queue recovery and can be incorrectly reported as running.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
11b9ab4 to
13fea28
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Failed start readbacks can suppress required metadata retries, and terminal runs can retry forever after their Repository is deleted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 2
- Review effort level: Balanced
f40059b to
2ad680e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Queue-promotion retries can repeat final provider reporting and costly LLM analysis side effects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Malformed Kubernetes path segments can still indefinitely block watcher startup or queue recovery.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
2ad680e to
69c6f3b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The stateful concurrency recovery and cross-reconciliation retry behavior warrant final human review despite comprehensive tests.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
I’m experimenting with queue recovery through a chaos-testing branch, and it found a retry loop in this path. A PipelineRun can still report Queue promotion already treats |
69c6f3b to
8e4c853
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The security-sensitive route gating lacks a route-level regression test for its default-off guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
5c922bb to
17e643f
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The extensive concurrency state-machine and failure-recovery changes warrant final human validation despite strong test coverage.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/paco review |
pipelines-as-code
Bot
added
paco/review-very-hard
cd89bc1 to
67093a2
Compare
Malformed execution-order entries were skipped instead of crashing the watcher during startup or queue promotion. Queue recovery was changed to handle temporary API failures without losing waiting runs or starting too many at once. Kubernetes could start a run even when the watcher received an error. Reservations were kept until the run's state could be established. Retries were tracked so recovery no longer depended on a watcher restart. Temporary read failures preserved waiting order. Retried starts used updated concurrency limits, and stale start requests could not overwrite a newer run state. Completion and deletion returned promotion failures for retry before treating their queue work as finished. Fixes tektoncd#2945 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Temporary provider lookup failures on finished runs were returned for retry instead of silently leaving their concurrency slots occupied. Runs with missing or unknown providers released their slots before being marked failed. If starting the next waiting run failed, the error was returned before recording that final state, so the next attempt could resume the queue work. A successful handoff was remembered when saving the final state failed, preventing retries from starting another run for the same handoff. Fixes tektoncd#2946 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
The queue debug endpoint was registered unconditionally and is unauthenticated, disclosing namespace, Repository, and PipelineRun names to anything that can reach the pod. Gate it behind PAC_ENABLE_QUEUE_DEBUG (default disabled, fails closed on an unparseable value). Update the E2E helper to opt in and document the new toggle. Fixes tektoncd#2947 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Assisted-by: Claude
67093a2 to
268c7a2
Compare
📝 Description of the Change
Fixes concurrency queue recovery and malformed execution-order handling
following #2890, and makes the queue debug endpoint opt-in.
Malformed execution-order entries (#2945)
Execution-order entries now use a shared parser that rejects empty or
malformed
namespace/namepairs. Bad entries no longer panic the watcherduring startup or queue promotion. Finalization uses the same guarded
promotion path.
Queue recovery after API failures
A failed start response does not prove that Kubernetes left a run pending.
The watcher reads the run's current state and keeps its concurrency
reservation while the outcome is uncertain. Retries resume that reservation
instead of admitting another run or waiting for a watcher restart.
Temporary read failures keep candidates in their original waiting order.
Retries respect the current concurrency limit, including inherited settings.
Conditional start requests prevent delayed writes from overwriting a newer
run state or affecting a replacement run with the same name.
Completion and finalization return queue-promotion errors for retry.
Completion records its terminal state only after promotion succeeds, since
terminal states stop further reconciliation. Successful handoffs are
remembered while the final state is being saved.
Provider detection failures (#2946)
Temporary provider detection failures now return an error so the workqueue
retries. Previously, a finished run could keep its concurrency slot because
the failure was silently treated as success.
Missing or unknown providers take a separate path: release the finished run's
slot, promote the queue, then mark the run failed. If promotion fails, the
run stays retryable until its queue work finishes.
Queue debug endpoint (#2947)
/debug/queuewas registered unconditionally on the probe port and exposedRepository and PipelineRun names across namespaces without authentication.
It is now gated behind
PAC_ENABLE_QUEUE_DEBUG, disabled by default, andstays disabled for an invalid value.
The E2E helper opts in by updating the watcher Deployment and waiting for
rollout. The concurrency documentation describes the toggle.
🔗 Linked GitHub Issue
Fixes #2945
Fixes #2946
Fixes #2947
Jira epic: SRVKP-14316 — Concurrency queue robustness:
no lost or leaked slots under failure
fix: Keep queue recovery within concurrency limits/debug/queueunauthenticated and enabled by defaultfix(watcher): gate /debug/queue behind an env var🧪 Testing Strategy
Unit coverage includes lost start responses, transient read failures,
retry ownership, concurrency-limit changes, final-state write failures,
malformed queue keys, provider detection, and the debug toggle.
The existing Gitea concurrency E2E case covers debug-endpoint opt-in;
live E2E execution is outside this update's scope.
🤖 AI Assistance
✅ 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.