pipe-cd / pipe-cd/pipecd

bug: pipedv1 deployment store returns newest deployment as head, not oldest

Open
#6,780 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
1.4k
Forks
364
Avg merge
1d 4h
Merged PRs (30d)
84

Description

What happened:

While reviewing the deployment store sync logic in pipedv1, the head-deployment selection in pkg/app/pipedv1/apistore/deploymentstore/store.go:125-134 looks inverted relative to the Lister interface's docstring.

The interface promises:

ListAppHeadDeployments returns the map from application ID to its head of deploying deployments.
Head deployment is same with the oldest uncompleted one.

But the implementation writes pendings → planneds → runnings into the same map[string]*model.Deployment, so for an application with both a pending and a running deployment, the running one wins by overwrite order:

for _, d := range pendings { headDeployments[d.ApplicationId] = d }
for _, d := range planneds { headDeployments[d.ApplicationId] = d }
for _, d := range runnings { headDeployments[d.ApplicationId] = d }

A PENDING deployment is logically newer than a RUNNING one for the same application (it was created later, after the running one started), so this code returns the newest uncompleted deployment, not the oldest.

The same inversion exists at the corresponding location in the v0 store (pkg/app/piped/apistore/deploymentstore/store.go).

What you expected to happen:

Either:

  1. The docstring is updated to match the implementation ("head = newest uncompleted"), or
  2. The implementation is corrected to match the docstring — either reverse the write order or pick the head explicitly by created_at.

I don't know which side is correct, since I haven't traced where ListAppHeadDeployments is consumed downstream. Whichever way it ends up, the docstring and the code should agree.

How to reproduce it:

Unit test against the store with two *model.Deployment instances sharing one ApplicationId, one DEPLOYMENT_PENDING and one DEPLOYMENT_RUNNING. Lister.ListAppHeadDeployments()[appID] returns the running one — the opposite of what the docstring promises.

Environment:

  • piped version: master @ 78fdbeb7c (both v0 and v1 affected)
  • control-plane version: same
  • Others: surfaced during self-review of #6727 while writing tests for ListAppHeadDeployments

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with ListAppHeadDeployments in pkg/app/pipedv1/apistore/deploymentstore/store.go:125-134 and the corresponding implementation in pkg/app/piped/apistore/deploymentstore/store.go. Trace where the method is consumed to determine whether the head should be the oldest or newest uncompleted deployment, then add the two-deployment regression test described in the issue for both stores. Done means the implementation and Lister docstring agree.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devops, infrastructure
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.