fix: stop leaking postgres testcontainers in coverage runs#2950
fix: stop leaking postgres testcontainers in coverage runs#2950omer-topal merged 2 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors the Postgres test infrastructure by introducing a PostgresInstance wrapper struct that centralizes shutdown behavior and idempotent Close() semantics, then updates Postgres-related tests to access the underlying database handle via db.Postgres instead of explicit type assertions to *PQDatabase.Postgres. ChangesPostgresInstance Wrapper & Test Harness Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/storage/postgres/utils/common_test.go (2)
499-517:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContainer leak: local
dbshadows outerdb, escapingAfterEachcleanup.Line 502 declares
db := databaseas a local variable that shadows the outerdbat line 380.AfterEachonly closes the outerdb(fromBeforeEach). If any assertion at lines 506–514 fails before reachingdb.Close()at line 516, the new container is never terminated.🐛 Proposed fix
database := testinstance.PostgresDB(version) -db := database -writePool := db.Postgres.WritePool +localDB := database +defer func() { _ = localDB.Close() }() +writePool := localDB.Postgres.WritePool versionStr, err := utils.EnsureDBVersion(writePool) Expect(err).ShouldNot(HaveOccurred()) versionNum, parseErr := strconv.Atoi(versionStr) Expect(parseErr).ShouldNot(HaveOccurred()) Expect(versionNum).Should(BeNumerically(">=", 100000)) Expect(versionNum).Should(BeNumerically("<=", 999999)) - -err = db.Close() -Expect(err).ShouldNot(HaveOccurred())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/utils/common_test.go` around lines 499 - 517, The test creates a new local variable "db" that shadows the outer test fixture and prevents AfterEach from cleaning the container; change the shadowing assignment in the block that calls testinstance.PostgresDB(version) so it reuses the outer db variable (or rename the local to a non-shadowing name) instead of "db := database", ensuring the container returned by testinstance.PostgresDB(version) is closed by the existing AfterEach cleanup; check the block around EnsureDBVersion, versionStr parsing and the final db.Close call to confirm the outer db is the same instance used throughout.
454-483:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContainer leak:
testDB.Close()won't run if any assertion fails.If any
Expectbetween lines 469–477 panics (Ginkgo test failure), execution jumps toAfterEach, which only closes the outerdb(fromBeforeEach).testDBcreated inside the loop is never closed, leaking the testcontainer — directly contradicting this PR's goal.Use
deferimmediately after allocation:🐛 Proposed fix
database := testinstance.PostgresDB(pgVersion) testDB := database +defer func() { _ = testDB.Close() }() testPool := testDB.Postgres.WritePool version, err := utils.EnsureDBVersion(testPool) Expect(err).ShouldNot(HaveOccurred()) Expect(version).ShouldNot(BeEmpty()) versionNum, parseErr := strconv.Atoi(version) Expect(parseErr).ShouldNot(HaveOccurred()) Expect(versionNum).Should(BeNumerically(">=", 130008)) -// Clean up -err = testDB.Close() -Expect(err).ShouldNot(HaveOccurred())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/utils/common_test.go` around lines 454 - 483, The loop creates per-version test containers with testinstance.PostgresDB (assigned to variable testDB) but calls testDB.Close() only at the end, so a failing Expect can leak a container; fix by deferring close immediately after creating the DB (i.e., call defer func() { _ = testDB.Close() }() or defer testDB.Close() right after testDB := database) so the container is always cleaned up even if assertions panic; keep the rest of the checks (EnsureDBVersion, strconv.Atoi, Expect calls) unchanged and reference testinstance.PostgresDB, testDB, testPool, utils.EnsureDBVersion, and testDB.Close in your change.internal/storage/postgres/tenant_writer_test.go (1)
332-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline
separateDBcontainers can leak if an assertion fails beforeseparateDB.Close().This is the exact problem the PR aims to fix. When a Ginkgo
Expect(...)assertion fails inside anItblock, Ginkgo recovers the panic and runsAfterEach— butAfterEachonly cleans up the outerdb. AnyseparateDBcreated inline without aDeferCleanupis silently leaked if the test fails between container creation andseparateDB.Close().For example, in the test at line 323, if
separateWriter.CreateTenant(...)fails,separateDB.Close()is never reached:separateDB := testinstance.PostgresDB("14") separateWriter := NewTenantWriter(separateDB.Postgres) _, err = separateWriter.CreateTenant(...) // ← fail here → separateDB leaks Expect(err).ShouldNot(HaveOccurred()) err = separateDB.Close() // ← never reachedApply this fix to all affected
Itblocks:🛡️ Proposed fix — register cleanup immediately after creation
separateDB := testinstance.PostgresDB("14") +DeferCleanup(separateDB.Close) separateWriter := NewTenantWriter(separateDB.Postgres)Affected locations: lines 332, 359, 407, 435, 463 (and similar patterns throughout the
DeleteTenant Error Handlingcontext).Also applies to: 359-372, 407-421, 435-449, 463-477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/tenant_writer_test.go` around lines 332 - 344, Multiple It blocks create an inline testinstance Postgres container (separateDB) and use it to build a NewTenantWriter but do not register cleanup immediately, risking leaked containers if an Expect fails; fix by calling t.DeferCleanup or Ginkgo's DeferCleanup on the separateDB.Close right after creating separateDB (immediately after the separateDB := testinstance.PostgresDB("...") line) in each affected block (references: separateDB, separateWriter := NewTenantWriter(separateDB.Postgres), separateWriter.CreateTenant(...), writerWithClosedDB) so the container is always closed even if the test assertion fails.internal/storage/postgres/gc/gc_test.go (2)
637-650:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
freshDBcan leak if thefreshGC.Run()assertion fails.Same pattern as the
separateDBissue intenant_writer_test.go. IffreshGC.Run()produces an unexpected error,freshDB.Close()is never called:freshDB := testinstance.PostgresDB("14") // ← new container freshGC := NewGC(freshDB.Postgres, ...) err := freshGC.Run() Expect(err).ShouldNot(HaveOccurred()) // ← fail here → freshDB leaks err = freshDB.Close() // ← never reached🛡️ Proposed fix
freshDB := testinstance.PostgresDB("14") +DeferCleanup(freshDB.Close) freshGC := NewGC(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/gc/gc_test.go` around lines 637 - 650, The test can leak the test Postgres container because freshDB.Close() is not guaranteed to run if Expect(err).ShouldNot(HaveOccurred()) fails; wrap the resource cleanup so it always runs (e.g., call freshDB.Close() via defer immediately after creating freshDB or register it with the test cleanup) so that freshDB is closed regardless of freshGC.Run() outcome; locate the freshDB variable and the NewGC/freshGC.Run() call in this test and add a guaranteed cleanup (defer freshDB.Close() or equivalent test teardown) right after freshDB := testinstance.PostgresDB("14") to ensure Close() runs even on assertion failures.
373-385:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
closedDBcontainer also benefits fromDeferCleanupfor consistency.While
closedDB.Close()is called immediately (before any further assertions), if thatClose()itself fails the assertion at line 374, Ginkgo catches the panic and the container leaks without a registered cleanup:🛡️ Proposed fix
closedDB := testinstance.PostgresDB("14") +DeferCleanup(closedDB.Close) err := closedDB.Close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/gc/gc_test.go` around lines 373 - 385, The test creates closedDB via testinstance.PostgresDB("14") then calls closedDB.Close() directly which can panic and leak the container; register a cleanup immediately after creating closedDB (e.g., t.DeferCleanup or Ginkgo's DeferCleanup) using closedDB.Close (or a wrapper) before calling closedDB.Close() and then perform the Close/assert; this ensures the container is always cleaned up even if the explicit Close() assertion in the test for closedDB fails; references: closedDB, PostgresDB("14"), NewGC, Run.
🧹 Nitpick comments (3)
internal/storage/postgres/watch_test.go (1)
114-336: ⚖️ Poor tradeoffDuplicate
Itbodies throughout the error-handling contexts.All the
Itblocks ingetRecentXIDs Error Handling(lines 186-228) andgetChanges Error Handling(lines 231-335) have identical test bodies — same setup, same assertion, testing the same error path. They don't exercise distinct code paths (all calldb.Close()first, which causes every subsequent operation to fail with the same error). Consider collapsing them into parameterized table tests or removing duplicates to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/watch_test.go` around lines 114 - 336, The tests duplicate identical bodies for many It blocks causing maintenance overhead; replace the repeated It cases in getRecentXIDs and getChanges Error Handling with a single parameterized/table-driven test (e.g., Ginkgo DescribeTable or a for-loop of cases) that enumerates the different case names but reuses the shared setup (close db, NewWatcher(closedDB)) and asserts the same error substring; target the repeated call sites getRecentXIDs(ctx, 0, "t1") and getChanges(ctx, PQDatabase.XID8{Uint: 1}, "t1") and consolidate their expectations into table entries so each logical case is represented without duplicating the test body.pkg/testinstance/postgres.go (1)
21-56: 💤 Low valueLGTM —
PostgresInstancewrapper is well-structured.The compile-time interface assertion at line 28,
sync.Once-backed idempotentClose(), and nil-guarded sub-closures are all correct. One optional hardening:GetEngineType()andIsReady()will panic if called on a zero-value&PostgresInstance{}(nilPostgres). Since onlyPostgresDB()constructs these and always populatesPostgres, this is safe in practice, but a nil guard would be defensive.🛡️ Optional nil guard
func (p *PostgresInstance) GetEngineType() string { + if p.Postgres == nil { + return "" + } return p.Postgres.GetEngineType() } func (p *PostgresInstance) IsReady(ctx context.Context) (bool, error) { + if p.Postgres == nil { + return false, nil + } return p.Postgres.IsReady(ctx) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/testinstance/postgres.go` around lines 21 - 56, The GetEngineType and IsReady methods on PostgresInstance can panic on a zero-value instance because p.Postgres may be nil; add defensive nil checks in the methods (PostgresInstance.GetEngineType and PostgresInstance.IsReady) to return a safe default (e.g., empty string for GetEngineType) or a clear error for IsReady when p.Postgres is nil, so callers never dereference a nil Postgres pointer; keep behavior unchanged when p.Postgres is present.internal/storage/postgres/gc/options_test.go (1)
15-18: ⚡ Quick winConsider sharing a single container across the entire options-test suite.
Every
Itblock in this file spins up and tears down a full Postgres testcontainer, yet none of them execute any database queries — they only testGCoption-setter functions. Moving the container lifecycle toBeforeSuite/AfterSuite(orSynchronizedBeforeSuite/SynchronizedAfterSuite) would cut the container startup overhead from O(number of tests) to O(1) for this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/postgres/gc/options_test.go` around lines 15 - 18, The tests currently create a fresh Postgres container in BeforeEach via testinstance.PostgresDB (see the BeforeEach block) which is wasteful; move the container lifecycle to a suite-level setup/teardown by creating the DB once in BeforeSuite (or SynchronizedBeforeSuite for parallel Ginkgo nodes) and terminating it in AfterSuite (or SynchronizedAfterSuite), make the db variable package-level so individual It blocks reuse it, and remove the per-test Terminate/creation in BeforeEach/AfterEach to eliminate repeated container startups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/storage/postgres/gc/gc_test.go`:
- Around line 637-650: The test can leak the test Postgres container because
freshDB.Close() is not guaranteed to run if
Expect(err).ShouldNot(HaveOccurred()) fails; wrap the resource cleanup so it
always runs (e.g., call freshDB.Close() via defer immediately after creating
freshDB or register it with the test cleanup) so that freshDB is closed
regardless of freshGC.Run() outcome; locate the freshDB variable and the
NewGC/freshGC.Run() call in this test and add a guaranteed cleanup (defer
freshDB.Close() or equivalent test teardown) right after freshDB :=
testinstance.PostgresDB("14") to ensure Close() runs even on assertion failures.
- Around line 373-385: The test creates closedDB via
testinstance.PostgresDB("14") then calls closedDB.Close() directly which can
panic and leak the container; register a cleanup immediately after creating
closedDB (e.g., t.DeferCleanup or Ginkgo's DeferCleanup) using closedDB.Close
(or a wrapper) before calling closedDB.Close() and then perform the
Close/assert; this ensures the container is always cleaned up even if the
explicit Close() assertion in the test for closedDB fails; references: closedDB,
PostgresDB("14"), NewGC, Run.
In `@internal/storage/postgres/tenant_writer_test.go`:
- Around line 332-344: Multiple It blocks create an inline testinstance Postgres
container (separateDB) and use it to build a NewTenantWriter but do not register
cleanup immediately, risking leaked containers if an Expect fails; fix by
calling t.DeferCleanup or Ginkgo's DeferCleanup on the separateDB.Close right
after creating separateDB (immediately after the separateDB :=
testinstance.PostgresDB("...") line) in each affected block (references:
separateDB, separateWriter := NewTenantWriter(separateDB.Postgres),
separateWriter.CreateTenant(...), writerWithClosedDB) so the container is always
closed even if the test assertion fails.
In `@internal/storage/postgres/utils/common_test.go`:
- Around line 499-517: The test creates a new local variable "db" that shadows
the outer test fixture and prevents AfterEach from cleaning the container;
change the shadowing assignment in the block that calls
testinstance.PostgresDB(version) so it reuses the outer db variable (or rename
the local to a non-shadowing name) instead of "db := database", ensuring the
container returned by testinstance.PostgresDB(version) is closed by the existing
AfterEach cleanup; check the block around EnsureDBVersion, versionStr parsing
and the final db.Close call to confirm the outer db is the same instance used
throughout.
- Around line 454-483: The loop creates per-version test containers with
testinstance.PostgresDB (assigned to variable testDB) but calls testDB.Close()
only at the end, so a failing Expect can leak a container; fix by deferring
close immediately after creating the DB (i.e., call defer func() { _ =
testDB.Close() }() or defer testDB.Close() right after testDB := database) so
the container is always cleaned up even if assertions panic; keep the rest of
the checks (EnsureDBVersion, strconv.Atoi, Expect calls) unchanged and reference
testinstance.PostgresDB, testDB, testPool, utils.EnsureDBVersion, and
testDB.Close in your change.
---
Nitpick comments:
In `@internal/storage/postgres/gc/options_test.go`:
- Around line 15-18: The tests currently create a fresh Postgres container in
BeforeEach via testinstance.PostgresDB (see the BeforeEach block) which is
wasteful; move the container lifecycle to a suite-level setup/teardown by
creating the DB once in BeforeSuite (or SynchronizedBeforeSuite for parallel
Ginkgo nodes) and terminating it in AfterSuite (or SynchronizedAfterSuite), make
the db variable package-level so individual It blocks reuse it, and remove the
per-test Terminate/creation in BeforeEach/AfterEach to eliminate repeated
container startups.
In `@internal/storage/postgres/watch_test.go`:
- Around line 114-336: The tests duplicate identical bodies for many It blocks
causing maintenance overhead; replace the repeated It cases in getRecentXIDs and
getChanges Error Handling with a single parameterized/table-driven test (e.g.,
Ginkgo DescribeTable or a for-loop of cases) that enumerates the different case
names but reuses the shared setup (close db, NewWatcher(closedDB)) and asserts
the same error substring; target the repeated call sites getRecentXIDs(ctx, 0,
"t1") and getChanges(ctx, PQDatabase.XID8{Uint: 1}, "t1") and consolidate their
expectations into table entries so each logical case is represented without
duplicating the test body.
In `@pkg/testinstance/postgres.go`:
- Around line 21-56: The GetEngineType and IsReady methods on PostgresInstance
can panic on a zero-value instance because p.Postgres may be nil; add defensive
nil checks in the methods (PostgresInstance.GetEngineType and
PostgresInstance.IsReady) to return a safe default (e.g., empty string for
GetEngineType) or a clear error for IsReady when p.Postgres is nil, so callers
never dereference a nil Postgres pointer; keep behavior unchanged when
p.Postgres is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9004d878-1c47-4320-a547-37a257701da9
📒 Files selected for processing (13)
internal/storage/postgres/bundle_reader_test.gointernal/storage/postgres/bundle_writer_test.gointernal/storage/postgres/data_reader_test.gointernal/storage/postgres/data_writer_test.gointernal/storage/postgres/gc/gc_test.gointernal/storage/postgres/gc/options_test.gointernal/storage/postgres/schema_reader_test.gointernal/storage/postgres/schema_writer_test.gointernal/storage/postgres/tenant_reader_test.gointernal/storage/postgres/tenant_writer_test.gointernal/storage/postgres/utils/common_test.gointernal/storage/postgres/watch_test.gopkg/testinstance/postgres.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2950 +/- ##
==========================================
+ Coverage 82.57% 82.60% +0.03%
==========================================
Files 74 74
Lines 8300 8300
==========================================
+ Hits 6853 6855 +2
+ Misses 911 909 -2
Partials 536 536 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storage/postgres/tenant_writer_test.go`:
- Around line 288-289: The test currently swallows errors by calling
DeferCleanup(func() { _ = separateDB.Close() }) and then immediately calling err
:= separateDB.Close(); instead, change the cleanup to check and report the
error: use DeferCleanup(func() { if err := separateDB.Close(); err != nil {
t.Errorf("closing separateDB: %v", err) } }) and remove the redundant immediate
err := separateDB.Close() call so cleanup failures for separateDB are surfaced;
reference the separateDB variable and the DeferCleanup call when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e7cadb5-0120-476c-9a5e-e858b5794c03
📒 Files selected for processing (3)
internal/storage/postgres/gc/gc_test.gointernal/storage/postgres/tenant_writer_test.gointernal/storage/postgres/utils/common_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/storage/postgres/utils/common_test.go
- internal/storage/postgres/gc/gc_test.go
| DeferCleanup(func() { _ = separateDB.Close() }) | ||
| err := separateDB.Close() |
There was a problem hiding this comment.
Don’t swallow cleanup errors in DeferCleanup.
On Line 288, ignoring separateDB.Close() errors (_ = ...) can hide cleanup failures and make container-leak regressions harder to detect when the test exits early.
Suggested fix
- DeferCleanup(func() { _ = separateDB.Close() })
+ DeferCleanup(func() {
+ Expect(separateDB.Close()).ShouldNot(HaveOccurred())
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DeferCleanup(func() { _ = separateDB.Close() }) | |
| err := separateDB.Close() | |
| DeferCleanup(func() { | |
| Expect(separateDB.Close()).ShouldNot(HaveOccurred()) | |
| }) | |
| err := separateDB.Close() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/storage/postgres/tenant_writer_test.go` around lines 288 - 289, The
test currently swallows errors by calling DeferCleanup(func() { _ =
separateDB.Close() }) and then immediately calling err := separateDB.Close();
instead, change the cleanup to check and report the error: use
DeferCleanup(func() { if err := separateDB.Close(); err != nil {
t.Errorf("closing separateDB: %v", err) } }) and remove the redundant immediate
err := separateDB.Close() call so cleanup failures for separateDB are surfaced;
reference the separateDB variable and the DeferCleanup call when making this
change.
1 participant
Summary by CodeRabbit
Refactor
Tests