pkg/testfixtures/storage/postgres.go (2)

130-138: Template database readiness depends on bootstrap completion.

The waitForDatabase on Line 131 only verifies TCP connectivity to postgresTemplateDB, but doesn't guarantee that migrations have completed. If a parallel test process finds the container mid-bootstrap (after container start but before goose.Up completes at Line 222), the createdb will succeed but the template may lack schema, causing subsequent migration version checks to fail.

Consider having the bootstrap process create a sentinel (e.g., a marker table or file) after migrations complete, and have consumers wait for that sentinel.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/postgres.go` around lines 130 - 138, The current
readiness check uses waitForDatabase(...) which only verifies TCP/connectivity
to postgresTemplateDB before running createdb; instead add a post-bootstrap
sentinel (e.g., a marker table or file) created after migrations complete in the
bootstrap path (where goose.Up runs) and replace or augment the waitForDatabase
call in this setup flow with a new waitForTemplateReady (or waitForSentinel)
call that polls the sentinel via the same connection URI (use
testCont.GetConnectionURI(true)) before running container.ExecOptions createdb;
ensure the bootstrap code that runs goose.Up creates the sentinel and the test
fixture uses that sentinel check so createdb only proceeds after migrations
finish.

---

112-117: Potential race condition when multiple test binaries run in parallel.

When multiple test processes (e.g., different packages running with go test ./...) execute simultaneously, they may all observe "not found" from FindRunningContainer and attempt to bootstrap containers concurrently. While each container gets a unique ULID suffix preventing name collisions, this defeats the container reuse goal and wastes resources.

Consider adding a file-based lock or using Docker's container creation idempotently by catching "container already exists" errors and retrying the find operation.

💡 Suggested approach

 	dockerCont, found, err := docker.FindRunningContainer(t.Context(), "^"+postgresContainerPrefix, postgresImage)
 	require.NoError(t, err, "find running postgres container")

 	if !found {
-		dockerCont = bootstrapPostgresContainer(t, docker)
+		dockerCont, err = bootstrapPostgresContainerWithRetry(t, docker)
+		require.NoError(t, err, "bootstrap postgres container")
 	}

Where bootstrapPostgresContainerWithRetry catches "container name conflict" errors and re-checks for an existing container.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/postgres.go` around lines 112 - 117, The current
logic using docker.FindRunningContainer and bootstrapPostgresContainer can race
when multiple test processes see "not found" and all attempt to create
containers; update the flow by introducing a retry-or-lock around container
creation: implement a wrapper (e.g., bootstrapPostgresContainerWithRetry) that
first tries docker.FindRunningContainer(t.Context(),
"^"+postgresContainerPrefix, postgresImage), and if not found attempts
bootstrapPostgresContainer but catches Docker "container already exists" / name
conflict errors, then re-runs FindRunningContainer (with a short backoff) before
failing; alternatively use a simple file-based lock around the create path to
ensure only one process calls bootstrapPostgresContainer at a time—apply this
change where dockerCont is set so container creation becomes idempotent and
concurrent test runs reuse an existing container.

pkg/testfixtures/storage/postgres.go (2)

228-234: Consider passing DockerClient as parameter instead of creating a new one.

A new DockerClient is created for each replica, though the caller already has one. This is minor for test code but could be cleaner.

♻️ Optional: Pass existing client

-func runPostgresReplica(t testing.TB, primary *postgresTestContainer) *postgresReplicaContainer {
-	docker, err := testutils.NewDockerClient()
-	require.NoError(t, err)
-
-	t.Cleanup(func() {
-		docker.Close()
-	})
+func runPostgresReplica(t testing.TB, docker *testutils.DockerClient, primary *postgresTestContainer) *postgresReplicaContainer {

Then update CreateSecondary to pass the client.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/postgres.go` around lines 228 - 234,
runPostgresReplica creates a new DockerClient via testutils.NewDockerClient for
each replica even though the caller already has one; change runPostgresReplica
to accept a DockerClient parameter and reuse it instead of calling
testutils.NewDockerClient, and update CreateSecondary (and any call sites
constructing postgresReplicaContainer) to accept and forward that DockerClient
so the same client instance is reused when creating
postgresReplicaContainer/secondary replicas.

---

352-360: Use sql.NullString.Valid for clearer NULL handling.

The pg_last_wal_receive_lsn() function returns NULL when the replica hasn't received any WAL. While checking String == "" works because sql.NullString sets String to empty when Valid is false, it's more semantically correct to check !Valid.

♻️ Clearer NULL check

-	if replicaLSN.String == "" {
+	if !replicaLSN.Valid || replicaLSN.String == "" {
 		return fmt.Errorf("replica has not received any WAL yet")
 	}
 
-	t.Logf("Replica is synchronized and receiving WAL at LSN: %s", replicaLSN.String)
+	t.Logf("Replica is synchronized and receiving WAL at LSN: %s", replicaLSN.String)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/postgres.go` around lines 352 - 360, Replace the
empty-string check on the sql.NullString with a semantic NULL check: after
scanning into replicaLSN (the sql.NullString returned from db.QueryRow("SELECT
pg_last_wal_receive_lsn()").Scan(&replicaLSN)), use replicaLSN.Valid (i.e., if
!replicaLSN.Valid) to detect the NULL/absence of WAL instead of comparing
replicaLSN.String == ""; keep the same error message and return path when the
value is not valid.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/testfixtures/storage/storage.go (1)

13-16: Consider removing redundant Goose setup in mysql.go.

The init() centralizes Goose configuration, but pkg/testfixtures/storage/mysql.go still calls goose.SetLogger() at line 140 and goose.SetBaseFS() at line 158, which override this global state. While functionally harmless (they set the same values), removing the redundant calls would improve consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/storage.go` around lines 13 - 16, Remove the
redundant Goose setup in mysql.go by deleting the extra calls to
goose.SetLogger(...) and goose.SetBaseFS(...) so the global initialization in
init() within storage.go is the single source of truth; locate and remove the
goose.SetLogger invocation (around the call in mysql.go) and the goose.SetBaseFS
invocation (the later call in mysql.go) so they no longer override the central
init() configuration.

pkg/testfixtures/storage/postgres.go (1)

126-126: Consider sanitizing the database name for SQL safety.

The database name is constructed using postgresDBPrefix + ulid.Make().String(). While ULIDs are alphanumeric and safe, the DROP DATABASE statement at line 145 uses string interpolation with fmt.Sprintf. If the prefix or format ever changes, this could become an injection risk.

The current implementation is safe because ULID characters are [0-9A-Z], but consider using pq.QuoteIdentifier or similar for defense-in-depth.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testfixtures/storage/postgres.go` at line 126, The database name is built
as postgresDBPrefix + ulid.Make().String() and later interpolated into a DROP
DATABASE string using fmt.Sprintf, so update the code to quote/sanitize the
identifier before interpolation: import and use pq.QuoteIdentifier (or an
equivalent identifier-quoting helper) to wrap the composed database name when
constructing any SQL that embeds it (e.g., the DROP DATABASE fmt.Sprintf call
and any other ad-hoc SQL), and keep the original composed name for internal
tracking; reference postgresDBPrefix, ulid.Make().String(), and the DROP
DATABASE fmt.Sprintf usage to locate where to apply pq.QuoteIdentifier.

pkg/testutils/dockerclient.go (1)

119-122: Port readiness timeout may be too aggressive compared to other test infrastructure timeouts.

The 5-second MaxElapsedTime here is significantly shorter than waitForDatabase (30s) and waitForMigrationVersion (1 minute) in pkg/testfixtures/storage/readiness.go. While Docker port binding is typically a fast local operation, slow CI environments may hit this limit. Consider either increasing the timeout or documenting why Docker port binding requires a shorter window than database/migration operations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testutils/dockerclient.go` around lines 119 - 122, The current backoff
policy created by backoff.NewExponentialBackOff sets MaxElapsedTime to
5*time.Second via the backoffPolicy variable which is much shorter than other
test readiness timeouts (waitForDatabase 30s and waitForMigrationVersion 1m);
update the MaxElapsedTime to a longer duration (e.g., 30*time.Second) or make it
configurable so Docker port readiness aligns with CI flakiness expectations, and
adjust the call site that uses backoffPolicy accordingly to use the new value or
configuration.

pkg/testutils/dockerclient_test.go (1)

101-135: Assert the actual ContainerList filters in these cases.

These FindRunningContainer tests only validate method/path, so they still go green if the request is built with the wrong name / ancestor / status filters. Since the shared-container lookup depends on those arguments, I'd add a per-step assertion hook and validate the serialized request in the /containers/json cases.

🧪 Minimal hook to let mock steps validate request details

 type dockerMockStep struct {
 	Method string
 	Path   string
 	Body   []byte
 	Error  string
+	Assert func(*testing.T, *http.Request)
 }
@@
 		if !strings.HasSuffix(r.URL.Path, step.Path) {
 			t.Fatalf("unexpected path %s", r.URL.Path)
 		}
+		if step.Assert != nil {
+			step.Assert(t, r)
+		}
 
 		if step.Body != nil {
 			_, _ = w.Write(step.Body)
 			return

Also applies to: 360-374

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/testutils/dockerclient_test.go` around lines 101 - 135, The tests for
TestFindRunningContainer_Fail call FindRunningContainer but only assert HTTP
method/path; add a validation hook to the docker mock (via dockerMockStep or
newDockerClientMock) so that when a step with Path "/containers/json" is invoked
it decodes the actual request query/body into the Docker ContainerList filters
and asserts the expected name, ancestor (alpineImage) and status filters are
present; update the three subtests ("not_found", "list_containers",
"inspect_container") to supply that assertion hook for the /containers/json step
(and mirror the same change in the other failing test range referenced) so the
mock fails if the request is built with wrong filters while leaving existing
method/path checks intact and keeping FindRunningContainer, newDockerClientMock
and dockerMockStep usage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes