db: avoid Postgres fixture cache deadlock

[sputn1ck]

Summary

• avoid holding the path-keyed Postgres fixture cache mutex while creating Docker fixtures
• keep cache eviction guarded without blocking teardown behind fixture semaphore waits

Verification

• make fmt-changed
• make lint-changed-local
• go test -tags="dev test_postgres nolog" ./db -run "^$"
• env SYSTEST_PARALLEL=1 make systest db=postgres backend=lnd case=TestOORSubDustRecipientRejectedE2E timeout=30m from parent darepo

Parent PR: lightninglabs/darepo#643

[gemini-code-assist]

Code Review

This pull request refactors NewTestDBHandleFromPath in db/test_postgres.go to release the testPgFixtureMtx lock before creating a new Postgres fixture, preventing potential deadlocks or blocking on the global semaphore. It also introduces a double-check lock pattern to handle concurrent fixture creation for the same path. However, a race condition was identified in the cleanup function where sqlFixture.TearDown(t) is called before the fixture is evicted from the map, which could lead to concurrent callers accessing a stale, tearing-down fixture. It is recommended to evict the fixture from the map under lock before performing the teardown.

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.

[gemini-code-assist]

There is a race condition in the cleanup function: sqlFixture.TearDown(t) is called before the fixture is evicted from the testPgFixturesByPath map under the lock. Since TearDown stops and purges the Docker container (which can take several seconds), there is a window of time where a concurrent or subsequent call to NewTestDBHandleFromPath for the same dbPath will find the fixture in the map (ok == true), but the underlying container is already stopped or being stopped. This will result in connection failures.

To prevent this, we should lock the mutex and delete the fixture from the map before calling TearDown(t). This ensures that any subsequent caller will not see the stale fixture in the map and will instead spin up a new one, while the teardown of the old fixture can safely run outside the lock.

Suggested change

	t.Cleanup(func() {
	sqlFixture.TearDown(t)
	testPgFixtureMtx.Lock()
	delete(testPgFixturesByPath, dbPath)
	if testPgFixturesByPath[dbPath] == sqlFixture {
	delete(testPgFixturesByPath, dbPath)
	}
	testPgFixtureMtx.Unlock()
	})
	t.Cleanup(func() {
	testPgFixtureMtx.Lock()
	if testPgFixturesByPath[dbPath] == sqlFixture {
	delete(testPgFixturesByPath, dbPath)
	}
	testPgFixtureMtx.Unlock()
	sqlFixture.TearDown(t)
	})

[Copilot]

Pull request overview

This PR updates the Postgres test DB handle helper to avoid holding the path-keyed fixture-cache mutex while potentially blocking operations (docker fixture creation / semaphore acquisition) are in progress, reducing the risk of teardown stalls in parallel test runs.

Changes:

• Release testPgFixtureMtx immediately after the cache lookup to avoid blocking other goroutines while creating docker fixtures.
• Add a second “already created” check when re-acquiring the mutex to safely handle concurrent creators and tear down redundant fixtures.
• Guard cache eviction during cleanup by only deleting the entry if it still points at the same fixture instance.

---

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

[sputn1ck]

@claude review please

[claude]

Claude finished @sputn1ck's task in 1m 59s —— View job

---

Reviewing this PR

• Read db/test_postgres.go (before/after) and the fixture helpers
• Analyze the concurrency change and existing review comments
• Post review

---

Review

The core change is sound: releasing testPgFixtureMtx before calling NewTestPgFixture (which can block on the global testPgFixtureSem semaphore) removes the "hold the cache mutex while doing slow Docker work" hazard, and the double-checked-lock re-read after creation correctly handles the (invariant-forbidden but now defensively-guarded) case of two concurrent creators for the same path. Cleanup ordering (LIFO: pool close → TearDown+delete) is preserved. Nice, focused fix.

A few notes:

1. (medium) Fixture leak if NewPostgresStore fails — db/test_postgres.go:77-78

sqlFixture = NewTestPgFixture(t, DefaultPostgresFixtureLifetime, true)

store, err := NewPostgresStore(sqlFixture.GetConfig(), log)
require.NoError(t, err)   // <- FailNow here leaks sqlFixture

NewTestPgFixture acquires a semaphore slot (acquireTestPgFixtureSlot) that is only released by TearDown. At this point no t.Cleanup for sqlFixture has been registered yet, so if NewPostgresStore (e.g. a migration) fails, require.NoError → FailNow unwinds without ever calling TearDown. That permanently leaks a semaphore slot, which is exactly the kind of thing that later starves/blocks the rest of the test binary — the failure mode this PR is trying to avoid. Copilot flagged this too.

This is pre-existing (the base code had the same shape), but the PR is right on top of it, so worth folding in:

store, err := NewPostgresStore(sqlFixture.GetConfig(), log)
if err != nil {
	sqlFixture.TearDown(t)
	require.NoError(t, err)
}

(Note the parallel branch at test_postgres.go:90-91 is already safe — the redundant fixture is torn down at line 86 before that NewPostgresStore call.)

Fix this →

2. (low / defensive) Delete from the map before TearDown in cleanup — db/test_postgres.go:103-111

Gemini's suggestion to swap the order (delete under lock, then TearDown outside the lock) is a cheap improvement and is consistent with the double-check pattern you just added — it keeps the map from advertising a fixture whose container is mid-Purge. In practice the documented invariant (unique per-client temp-dir dbPath, reuse only sequentially within one test) means no concurrent caller can observe the stale entry during cleanup, so this is defensive rather than a live bug. Still, since it's free:

t.Cleanup(func() {
	testPgFixtureMtx.Lock()
	if testPgFixturesByPath[dbPath] == sqlFixture {
		delete(testPgFixturesByPath, dbPath)
	}
	testPgFixtureMtx.Unlock()

	sqlFixture.TearDown(t)
})

3. (nit) The double-check branch is effectively defensive-only. Per the function's own invariant, the same dbPath is never opened concurrently (only sequentially, original handle + restart), so alreadyCreated == true shouldn't occur in practice. That's fine — guarding it is cheap insurance now that the lock is dropped during creation — but a one-line comment noting it's a defensive guard against invariant violation (rather than an expected path) would help the next reader.

Verdict: Looks good to merge. I'd fold in #1 (the semaphore-slot leak is the same class of problem this PR targets); #2 and #3 are optional polish.
• kon/pg-fixture-cache-lock