fix: paginate ListNotCompletedDeployments in legacy piped

[Goyamjain06]

What this PR does

Updates the legacy piped deployment store to follow the pagination cursor returned by ListNotCompletedDeployments.

Previously, sync() fetched only the first page and ignored the returned cursor. It now continues fetching until the cursor is empty, ensuring all pending/planned/running deployments are included.

Changes

• Update pkg/app/piped/apistore/deploymentstore/store.go to paginate ListNotCompletedDeployments using resp.Cursor.
• Add store_test.go with coverage for:
  • empty responses
  • deployment status classification
  • multi-page pagination
  • 3+ page pagination
  • ROLLING_BACK classification
  • errors during pagination
• Fix the server-side pagination contract in pkg/datastore/deploymentstore.go (see the Update section below).

Tests

• go test ./pkg/app/piped/apistore/deploymentstore/... — pass
• go test ./pkg/datastore/... ./pkg/app/pipedv1/apistore/deploymentstore/... ./pkg/app/server/grpcapi/... — pass (incl. -race)
• go vet ./pkg/app/piped/apistore/deploymentstore/... — pass
• make check — Go build and web build pass. The golangci-lint step is blocked by the pinned lint image using Go 1.25.0 while the repository requires Go 1.26.2.

Update: fixing the server-side pagination contract

Review feedback correctly pointed out that following the cursor is unsafe against the current server. PipedAPI.ListNotCompletedDeployments builds its datastore query with no Orders set. In that case deploymentStore.List was still asking the iterator for a cursor, and Iterator.Cursor() base64-encodes an empty ordering-key map to a non-empty string ("e30="). So the first page always came back with a non-empty cursor, the new client loop issued a second request carrying it, and both the Firestore and MySQL backends reject that with opts.Cursor also requires Orders to be set — meaning every sync tick would fail for any piped with at least one active deployment.

Rather than introduce an ordering plus a new PipedId composite Firestore index (a much larger change that is hard to verify without a live datastore), this fixes the contract at the source: deploymentStore.List now returns an empty cursor whenever the query has no Orders. Ordered, paginated callers (WebAPI / API ListDeployments) always set Orders, so their behaviour is unchanged. Unordered callers — ListNotCompletedDeployments, the inner lookup in ListDeploymentTraces, and the deployment-chain updater — already discard the cursor and receive the full result set in a single call.

The same loop was added to the pipedv1 deployment store in #6727 against this same unchanged handler, so this fix also removes a latent break there. A regression test is included on the v1 store.

Additional changes

• pkg/datastore/deploymentstore.go: return an empty cursor for unordered List queries.
• pkg/datastore/deploymentstore_test.go: cover the unordered case (no cursor, Iterator.Cursor() not called) and the ordered case (cursor passed through).
• store_test.go (v0 and v1): the fake API client now records each request's cursor, and the table tests assert the full "" -> "page2" -> "page3" threading.

Fixes #1937

[github-actions]

👋 Hi @Goyamjain06, welcome to PipeCD and thanks for opening your first pull request!

We’re really happy to have you here

Before your PR gets merged, please check a few important things below.

---

Helpful resources

• Contributing guide: https://github.com/pipe-cd/pipecd/blob/master/CONTRIBUTING.md
• Development setup: https://github.com/pipe-cd/pipecd/blob/master/CONTRIBUTING.md#development

---

DCO Sign-off

All commits must include a Signed-off-by line to comply with the Developer Certificate of Origin (DCO).

In case you forget to sign-off your commit(s), follow these steps:

For the last commit:

git commit --amend --signoff
git push --force-with-lease

For multiple commits:

git rebase --signoff origin/master
git push --force-with-lease

---

Run checks locally

Before pushing updates, please run:

make check

This runs the same checks as CI and helps catch issues early.

---

💬 Need help?

If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel.
You can get your Slack invite from: https://communityinviter.com/apps/cloud-native/cncf

Thanks for contributing to PipeCD! ❤️

[Goyamjain06]

Hi maintainers, just checking in on this one. All checks are passing and it's ready for review whenever you get a chance — no rush at all. Happy to make any changes if needed!

[vanshika2720]

Could we assert the request cursor in these tests? The fake currently ignores req.Cursor, so the tests don't verify that each response cursor is actually passed to the next request ("" → "page2" → "page3").

[Goyamjain06]

Good call — done. fakeAPIClient now records req.Cursor on every call, and each table case carries a wantCursors slice the test asserts against, so the "" -> "page2" -> "page3" threading is verified explicitly (and the mid-pagination error case checks that page2 is still forwarded to the failing request). Applied the same change to the pipedv1 store test. Thanks for the catch!

[vanshika2720]

This pagination assumes the server returns a cursor that can be passed back to this RPC, but ListNotCompletedDeployments currently doesn't set Orders/Limit on the datastore request. This can produce a non-empty cursor that fails on the next request with opts.Cursor also requires Orders to be set. Could we fix the server-side pagination contract before adding the client loop here?

[Goyamjain06]

You're right, thank you for catching this. ListNotCompletedDeployments builds its query with no Orders, and in that case deploymentStore.List was still returning a cursor — Iterator.Cursor() encodes an empty ordering-key map to "e30=", which is non-empty — so the follow-up request hit the opts.Cursor also requires Orders to be set check in both the Firestore and MySQL backends.

I've fixed it at the source: deploymentStore.List now returns an empty cursor when the query has no Orders. Ordered, paginated callers (ListDeployments) are unaffected since they always set Orders; unordered callers get the whole result set in one call, which is what this RPC already relied on. I went this route rather than adding an ordering plus a new PipedId composite Firestore index, which would be a much larger and harder-to-verify change. Full write-up is in the PR description's Update section.

Note the same loop landed for pipedv1 in #6727 against this same handler, so this also fixes a latent break there — I've added a regression test on the v1 store.

[netlify]

✅ Deploy Preview for pipecd-site canceled.

Name	Link
🔨 Latest commit	1958c5b
🔍 Latest deploy log	https://app.netlify.com/projects/pipecd-site/deploys/6a8ff59df5347300080ce774

[Copilot]

Pull request overview

This PR improves deployment synchronization reliability in legacy piped by correctly handling pagination for ListNotCompletedDeployments, and fixes a server-side cursor contract bug that would otherwise make cursor-following clients fail when the underlying datastore query is unordered.

Changes:

• Update legacy piped deployment-store sync loop to repeatedly call ListNotCompletedDeployments until the response cursor is empty.
• Fix pkg/datastore cursor behavior so unordered List queries never return a non-empty cursor (avoids invalid follow-up requests).
• Add/extend unit tests for both datastore cursor behavior and multi-page pagination in v0/v1 deployment stores.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/datastore/deploymentstore.go	Suppress cursor generation for unordered queries to enforce a safe pagination contract.
pkg/datastore/deploymentstore_test.go	Add coverage for unordered vs ordered cursor behavior.
pkg/app/pipedv1/apistore/deploymentstore/store_test.go	Extend pagination tests to assert request cursor threading and include a regression guard.
pkg/app/piped/apistore/deploymentstore/store.go	Implement cursor-following loop in legacy piped sync.
pkg/app/piped/apistore/deploymentstore/store_test.go	Add test coverage for pagination, status classification, and mid-pagination errors in legacy piped.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Goyamjain06]

Good catch, thanks! You're right — the loop only inspected the cursor, so a server bug that returned an empty page with a non-empty cursor would spin forever. I've added a safety guard that breaks out as soon as a page comes back with zero deployments, regardless of the cursor value. I also added a test case (stops_on_empty_page_with_non_empty_cursor) that reproduces exactly that scenario and asserts the bogus cursor is never threaded into a follow-up request.