tektoncd / tektoncd/pipelines-as-code
concurrency slot handling cannot recover from failed or ambiguous PipelineRun starts
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 214
- Forks
- 144
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 27
Description
📝 Description
Three places in the reconciler take a concurrency slot for a queued
PipelineRun and then try to start it. When starting fails, each of them
guesses what happened instead of checking, and each guesses differently.
The rule #2890 established is sound: give the slot back only when the run is
provably not going. The code approximates "provably not going" with "the start
patch returned an error" (ErrPipelineRunNotStarted). That approximation has
a hole. A Kubernetes patch can be committed by the API server while the client
times out waiting for the answer. The client sees an error, the code concludes
the run never started, releases the slot, and the queue admits one run more
than the limit allows. That is the exact bug #2890 set out to fix, surviving
in a narrower window.
The three call sites, with their individual problems:
pkg/reconciler/queue_pipelineruns.go:119. Initial admission. Releases the
slot onErrPipelineRunNotStarted, which is unsafe for the reason above.pkg/reconciler/reconciler.go:465. Promotion after a run completes. Drops
the candidate from the queue on any Get error, including a short network
blip, and nothing ever puts it back.pkg/reconciler/finalizer.go:80. Promotion after a run is deleted. Returns
on error without doing anything, leaving a still-pending run holding a slot
it can never give back. A repository atconcurrency_limit: 1is then stuck
until the watcher restarts.
The queue API makes a correct fix awkward. RemoveAndTakeItemFromQueue
releases one slot and reserves the next in a single call, and there is no
operation to put a reserved run back where it was. The internal running set is
a map[string]bool, so a reservation loses its position in the queue the
moment it is taken.
Found during a post-merge review of #2890, with reviewer input across three
rounds. The queue-level stall reproduces in a unit test.
🛠️ Suggested fix
Decide from fresh cluster state, not from the error. After a failed start
patch, fetch the PipelineRun again:
- still queued and pending: put it back in the queue, at its original position
- gone, finished or cancelled: drop the reservation and try the next one
- started: keep the slot
- cannot tell: keep the slot and report an error, so the limit is never
exceeded even if it costs a retry
Supporting work:
- keep the reserved queue item (with its ordering fields) instead of a bool,
and add a queue operation that returns a reservation to its original place - share one start-outcome helper across the three call sites
- in
reportFinalStatus, release and promote before writing the final state
annotation (pkg/reconciler/reconciler.go:395writes it first today, and
reconciler.go:137then blocks any retry of a failed promotion)
🧪 Testing Strategy
- Unit tests: with a real queue manager, cover failure before the patch, after the patch, and with an ambiguous patch result, in all three paths; assert the limit is never exceeded
- End-to-end tests where feasible
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the three call sites in pkg/reconciler/queue_pipelineruns.go:119, pkg/reconciler/reconciler.go:465, and pkg/reconciler/finalizer.go:80, then inspect the queue API and the reportFinalStatus flow at reconciler.go:395 and reconciler.go:137. Run the existing queue-manager unit tests covering failures before, after, and with ambiguous patch results across all three paths. Done means fresh cluster state determines the outcome and the concurrency limit is never exceeded, while pending runs can retry without a watcher restart.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- backend, distributed-systems, testing
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100