Skip to content

db: harden Postgres fixture cleanup - #867

Merged
sputn1ck merged 2 commits into
mainfrom
kon/pg-fixture-cleanup-followup
Jul 7, 2026
Merged

db: harden Postgres fixture cleanup#867
sputn1ck merged 2 commits into
mainfrom
kon/pg-fixture-cleanup-followup

Conversation

@sputn1ck

@sputn1ck sputn1ck commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

  • tear down a newly created Postgres fixture if opening the first store fails
  • evict path-cache entries before purging the fixture container during cleanup
  • document the duplicate-creator branch as a defensive invariant guard

Review comment analysis

  • Copilot was right: after NewTestPgFixture, a NewPostgresStore failure could skip TearDown and leak the fixture semaphore slot.
  • Gemini was directionally right: cleanup should delete the cache entry before Docker purge so no caller can observe a tearing-down fixture. This is defensive under the documented per-test path invariant, but cheap and safer.
  • Claude's nit about the double-check branch being defensive-only was also valid, so this adds a short comment.

Verification

  • go test -tags="dev test_postgres nolog" ./db -run '^$'
  • make fmt-changed
  • make lint-changed-local

Follow-up to #864.

Copilot AI review requested due to automatic review settings July 6, 2026 20:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request modifies db/test_postgres.go to improve resource cleanup and error handling during test database setup. Specifically, it ensures that sqlFixture.TearDown is called if NewPostgresStore fails, adds a defensive comment regarding sequential dbPath reopening, and reorders the operations in t.Cleanup so that the fixture is removed from the global map before being torn down. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Copilot AI left a comment

Copy link
Copy Markdown

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 hardens the lifecycle of Postgres test fixtures in db to prevent resource/semaphore leaks and reduce the chance that any code observes a fixture while it is tearing down, building on the fixture-cache changes introduced in #864.

Changes:

  • Ensure a newly created Postgres fixture is torn down if the initial NewPostgresStore open/migration step fails.
  • Evict dbPath cache entries before purging the fixture container during t.Cleanup.
  • Add a short comment clarifying the “duplicate creator” branch as a defensive invariant guard.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sputn1ck
sputn1ck merged commit 21282e6 into main Jul 7, 2026
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants