Skip to content

Close datastore iterators to release underlying rows and Firestore resources on decode errors - #7226

Open
vanshika2720 wants to merge 1 commit into
pipe-cd:masterfrom
vanshika2720:fix/7219-iterator-resource-leak
Open

Close datastore iterators to release underlying rows and Firestore resources on decode errors#7226
vanshika2720 wants to merge 1 commit into
pipe-cd:masterfrom
vanshika2720:fix/7219-iterator-resource-leak

Conversation

@vanshika2720

Copy link
Copy Markdown
Contributor

What this PR does:

Adds explicit resource cleanup to datastore.Iterator so underlying MySQL and Firestore iterators are closed when datastore iteration completes or exits early due to an error. All List() callers now defer iterator cleanup, and tests verify that Close() is called on both successful and error paths.

Why we need it:

Previously, datastore.Iterator had no Close() method, so an early return caused by a decode/scan error could leave the underlying sql.Rows or Firestore DocumentIterator open. Repeated failures could accumulate unreleased database resources and potentially exhaust available connections.

Which issue(s) this PR fixes:

Fixes #7219

Does this PR introduce a user-facing change?:

  • How are users affected by this change: No direct user-facing behavior change. This fixes datastore resource cleanup and prevents database/Firestore resources from remaining open after iterator errors.
  • Is this breaking change: No.
  • How to migrate (if breaking change): Not applicable.

…sources on decode errors

Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
@vanshika2720
vanshika2720 requested a review from a team as a code owner August 23, 2026 11:35
@vanshika2720
vanshika2720 requested review from armistcxy, ffjlabo and t-kikuc and a lite review from Copilot August 23, 2026 11:35
@netlify

netlify Bot commented Aug 23, 2026

Copy link
Copy Markdown

Deploy Preview for pipecd-site ready!

Name Link
🔨 Latest commit 99da25f
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a8adb1bb285fd0008a5434c
😎 Deploy Preview https://deploy-preview-7226--pipecd-site.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves PipeCD’s datastore layer by adding explicit iterator cleanup, ensuring underlying MySQL sql.Rows and Firestore DocumentIterator resources are released even when iteration exits early due to decode/scan errors. This aligns iterator lifecycle management with how PipeCD continuously polls datastore lists and reduces risk of connection/resource exhaustion under repeated failures.

Changes:

  • Extend datastore.Iterator with Close() error and implement it for MySQL and Firestore iterators.
  • Ensure datastore store List() implementations and integration test iteration helpers defer it.Close() after successful Find().
  • Update gomock iterator mock and unit tests to assert Close() is invoked on both success and error paths.

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/datastore/datastore.go Adds Close() error to the Iterator interface with lifecycle guidance.
pkg/datastore/mysql/iterator.go Implements Close() by closing underlying sql.Rows.
pkg/datastore/firestore/iterator.go Implements Close() by stopping the underlying Firestore DocumentIterator.
pkg/datastore/mock.go Regenerates/updates gomock to include Iterator.Close().
pkg/datastore/projectstore.go Defers iterator close in List() to ensure cleanup.
pkg/datastore/pipedstore.go Defers iterator close in List().
pkg/datastore/eventstore.go Defers iterator close in List().
pkg/datastore/deploymenttracestore.go Defers iterator close in List().
pkg/datastore/deploymentstore.go Defers iterator close in List().
pkg/datastore/deploymentchainstore.go Defers iterator close in List().
pkg/datastore/commandstore.go Defers iterator close in List().
pkg/datastore/applicationstore.go Defers iterator close in List().
pkg/datastore/apikey.go Defers iterator close in List().
pkg/datastore/projectstore_test.go Updates expectations to assert Close() is called.
pkg/datastore/pipedstore_test.go Updates expectations to assert Close() is called.
pkg/datastore/deploymentstore_test.go Updates expectations to assert Close() is called.
pkg/datastore/commandstore_test.go Updates expectations to assert Close() is called.
pkg/datastore/applicationstore_test.go Updates expectations to assert Close() is called.
pkg/datastore/apikey_test.go Updates expectations to assert Close() is called.
test/integration/datastore/mysql/project_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/piped_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/event_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/deploymentchain_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/deployment_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/command_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/application_test.go Defers iterator close in test helper iteration.
test/integration/datastore/mysql/apikey_test.go Defers iterator close in test helper iteration.
test/integration/datastore/firestore/firestore_test.go Defers iterator close in test helper iteration.
Files not reviewed (1)
  • pkg/datastore/mock.go: Generated file

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

@vanshika2720

Copy link
Copy Markdown
Contributor Author

@mohammedfirdouss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

datastore.Iterator leaks database/Firestore resources on decode errors

2 participants