[IAmJSD]

Root cause

Almost every internal/config integration test started its own PostgreSQL container and then applied all 123 migrations to it. The helpers involved were setupTestContainerDB, setupLadderStore, setupRampStepStore, setupRecommendationsStore, and 14 direct RequirePostgresContainer/SetupPostgresContainer call sites. One local run started 239 containers, so the package's runtime grows with every container-backed test added. In CI that came to roughly 15 of the 20 minutes allowed by go test -timeout=20m (#202).

Fix

testhelpers.RequireMigratedDatabase(ctx, t, migrationsPath) (new, internal/database/postgres/testhelpers/shared.go):

• Starts one container per test binary, on first use. It runs detached from the calling test's context, so the first test's deadline can't reach it.
• Migrates a template database once using the real migrations.RunMigrations. It then seals the template with IS_TEMPLATE true ALLOW_CONNECTIONS false, so no test can connect to it and change the starting state of later tests.
• Gives each test its own CREATE DATABASE … TEMPLATE copy and closes the pool when the test ends. It then runs DROP DATABASE … WITH (FORCE), so a leaked connection can't keep the database around. A failed drop fails the test.
• Isolation is the same as before. Each test still starts from a freshly migrated database that nothing else writes to. Only the server process and the migrated template are shared.
• Skip and fail behavior matches RequirePostgresContainer (#1597). It skips only when SkipIfProviderIsNotHealthy says Docker is unusable. A container that won't start, or a migration that won't apply, fails the test. Both errors are remembered, so the rest of the binary fails immediately instead of each test waiting out its own startup timeout.
• Concurrent first callers are serialized by one mutex covering the container start, the template build, and the CREATE DATABASE. They share one container and one template, and each gets its own database.

internal/config changes:

• Every container call site goes through the new helper.
• A TestMain (integration tag) stops the shared container. Ryuk still reaps it if the process dies.
• TestIntegrationTestsShareOnePostgresContainer is a parse-only guard with no build tag. It fails if a test file in the package calls testhelpers.RequirePostgresContainer or SetupPostgresContainer again.

No test was removed, skipped, or weakened. The assertions are untouched. The diff only changes how each test gets its database.

One side effect, in the safe direction for CI: the 12 recommendations and store_postgres_test.go tests used the bare SetupPostgresContainer and failed when Docker was absent. Through the new helper they now skip on a missing Docker provider, like the rest of the package under #1597. CI has Docker, so CI behavior is unchanged.

Closes #982

Verification

All runs were local on macOS (Docker Desktop, 10 CPUs) with CI's flags. AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null was set because this machine's default AWS profile is invalid.

Runtime, before vs after. This is integration evidence against real PostgreSQL 16 in testcontainers:

go test -json -race -tags=integration -timeout=40m -count=1 ./internal/config/...

	package elapsed	containers started (docker events … event=start)	results
main @ ba38120, run 1	353.2s	—	1312 pass, 0 fail, 11 skip (674 top-level)
main @ ba38120, run 2	438.1s	239	1312 pass, 0 fail, 11 skip
this branch	27.4s	—	1313 pass, 0 fail, 11 skip (675 top-level, +1 = the new guard)
this branch, -shuffle=on (seed 1792454946712618000), -timeout=20m	23.1s	1	1313 pass, 0 fail, 11 skip

A per-test diff of the -json output shows the same test set with no status changes. The only addition is the new guard. The 11 skips are the same env-gated tests on both sides (*_TEST_DATABASE_URL, unset in CI too). CI's runner is slower than this machine (about 900s on main per #202/#984), but nearly all of that time was per-test container and migration setup, and that setup now happens once.

Fail-before / pass-after for the guard (unit, parse-only):

go test -count=1 -run TestIntegrationTestsShareOnePostgresContainer ./internal/config/

With main's test files it fails and lists 18 per-test container call sites. On this branch it passes.

Helper integration tests (internal/database/postgres/testhelpers/shared_integration_test.go, -race -tags=integration) all pass:

• IsolatedCopiesOnOneContainer: distinct databases on one server, both at the latest migration version and not dirty. A table and a row written in one copy are invisible to a sibling and to a copy made later, so the template stays clean.
• TemplateRefusesConnections: connecting to the template is refused.
• DropsDatabaseWhenTestEnds: the per-test database is gone after its test, even though the test leaked an open connection.
• ConcurrentFirstUse: starts cold, then 8 parallel callers. They get 8 distinct, fully migrated databases on 1 container from 1 template.

Mutation probes on shared.go. Each was reverted afterwards, and each one fails at least one of the tests above:

• Template not sealed (ALLOW_CONNECTIONS false removed): TemplateRefusesConnections fails.
• Drop without FORCE: DropsDatabaseWhenTestEnds fails.
• No drop at all: DropsDatabaseWhenTestEnds fails.
• One database handed to every caller instead of a per-test copy: all 4 tests fail.
• Mutex removed from newDatabase: ConcurrentFirstUse fails. 5 callers error out, and only 3 distinct databases are created for 8 callers.

Other checks, all clean:

• go build ./...
• go vet and go vet -tags=integration on ./internal/config/ and ./internal/database/.... The second confirms no import cycle now that testhelpers imports migrations; all migrations tests are in migrations_test.
• golangci-lint v2.10.1, with and without --build-tags integration, on ./internal/config/... ./internal/database/postgres/testhelpers/...: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" on both packages: silent.
• gofmt -l: clean.
• go test ./internal/mocks/ ./internal/database/postgres/testhelpers/ (repo-wide guards, including TestNoSkipOnDatabaseSetupFailure): pass.
• go test -short -race ./internal/config/: pass.
• git-secrets: the allowlist self-test (scripts/test-git-secrets-allowlist.sh) passes 42/42. A scan of the changed files in a fresh repo with an isolated HOME is clean.
• No containers were left running after any run.

Notes and out of scope

• Interaction with test(ri-exchange): run the daily-cap reservation Postgres tests in CI #1088 (now merged). test(ri-exchange): run the daily-cap reservation Postgres tests in CI #1088 added a per-test testhelpers.RequirePostgresContainer for the RI-exchange reservation tests in internal/config. After merging main, the guard flagged it, so a730b17 switches it to RequireMigratedDatabase. Its 7 tests pass on the shared database (go test -tags integration -race -run 'ReserveApprovedRIExchange|DailySpend|ReserveRIExchange' ./internal/config/).
• Tests in the package still run sequentially. With one shared server, t.Parallel would now be cheap to add, but it wasn't needed to get well under the timeout, and it would introduce cross-test interleavings. Before anyone adds it, the pg_stat_activity probes in store_postgres_ladder_amend_integration_test.go, store_postgres_marketplace_claim_integration_test.go, and the RI-exchange tests should filter on datname = current_database(), as testhelpers.WaitForLockWait already does.
• Other packages (internal/api, internal/server, internal/purchase, …) still start a container per test. They can switch to the same helper when they start to matter. I didn't change them here.
• The CI timeout and job layout are unchanged. With this change no split or timeout raise is needed.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/this-quarter, effort/s.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: a3fbac40-5429-4538-a3be-69ccd523eb45

📥 Commits

Reviewing files that changed from the base of the PR and between 7e6ec71 and a730b17.

📒 Files selected for processing: 14

• internal/config/main_integration_test.go
• internal/config/shared_postgres_guard_test.go
• internal/config/store_postgres_db_test.go
• internal/config/store_postgres_ladder_test.go
• internal/config/store_postgres_marketplace_claim_integration_test.go
• internal/config/store_postgres_ramp_step_integration_test.go
• internal/config/store_postgres_recommendation_source_account_test.go
• internal/config/store_postgres_recommendations_test.go
• internal/config/store_postgres_registrations_integration_test.go
• internal/config/store_postgres_ri_exchange_approved_reservation_integration_test.go
• internal/config/store_postgres_test.go
• internal/config/store_postgres_unassigned_test.go
• internal/database/postgres/testhelpers/shared.go
• internal/database/postgres/testhelpers/shared_integration_test.go

+----------------------------------------------------------------------+
| Stop swallowing exceptions. They don't become less real in the dark. |
+----------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE a730b17

PR #1165 (closes #982), head a730b17, base origin/main f4387e4 (9 commits ahead of the merge base). git merge-tree --write-tree origin/main a730b177 is clean (tree a87df9e2). The change fixes #982 with the issue's first option: one container per binary, migrations run once into a sealed template, and each test gets a CREATE DATABASE ... TEMPLATE copy that is dropped WITH (FORCE).

Findings (none blocking):

• nit internal/database/postgres/testhelpers/shared.go:144 - buildTemplate runs on the first caller's ctx, while start (shared.go:129) is detached on purpose. If the first caller's deadline (60s in setupTestContainerDB, store_postgres_db_test.go:38) expires mid-migration, s.templates caches a context error and every later test in the binary fails with it, pointing at the wrong cause. Detach it too (context.WithoutCancel plus its own timeout). This is not a regression: on main the same 60s covered container start plus migration.
• nit internal/config/ri_exchange_reservation_integration_gate_test.go:24,42 - the test(ri-exchange): daily-cap reservation Postgres tests are env-gated and skip in CI #724 guard's comment and error message still say "use testhelpers.RequirePostgresContainer", which the new guard (shared_postgres_guard_test.go:66) rejects for the two config files it covers. Point both at RequireMigratedDatabase for config files.
• nit branch history - 23ceb9a is a merge of origin/main. A squash merge makes this harmless, but rebasing keeps the history linear.
• nit shared_postgres_guard_test.go:51 - the guard matches the testhelpers identifier only, so an aliased import bypasses it. Acceptable for a tripwire.

Leakage and parallelism: per-test databases are distinct and dropped with FORCE. Roles, ALTER SYSTEM and per-database settings (ALTER DATABASE) do not appear in migrations; the one ALTER DATABASE in tests (store_postgres_ladder_lock_integration_test.go:220) targets the test's own copy, which is then dropped. Advisory locks are scoped per database. No integration-tagged config test uses t.Parallel. The unfiltered pg_stat_activity probes (ladder_amend:139, marketplace_claim:67, ri_exchange_*) are safe only while the tests stay sequential, as the PR body says. Connection budget: the admin pool (10) plus 8 concurrent pools of 10 stays under PG's default limit of 100.

Attacked:

• Local runs on macOS with Docker, at the PR head:
  • go vet -tags integration ./...: clean.
  • go test -race -tags=integration ./internal/database/postgres/testhelpers/: ok, 8.9s.
  • go test -race -tags=integration -shuffle=on ./internal/config/: ok, 31.7s.
  • Guard unit test: pass.
• Merged tree (PR plus current main): go test -race -tags=integration ./internal/config/ ok, 38.0s. No postgres containers were left running after the runs.
• Mutation probes on shared.go, each reverted afterwards. All four were killed:
  • Drop without FORCE: DropsDatabaseWhenTestEnds fails.
  • ALLOW_CONNECTIONS false removed: TemplateRefusesConnections fails.
  • Migrations skipped: IsolatedCopies and ConcurrentFirstUse fail.
  • Database created without TEMPLATE: IsolatedCopies and ConcurrentFirstUse fail.
• Guard probe: putting back a RequirePostgresContainer call in the marketplace claim test makes TestIntegrationTestsShareOnePostgresContainer fail with 1 offender.
• Read SetupPostgresContainer (postgres.go:26): it registers no t.Cleanup, so the first test's cleanup cannot stop the shared container.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE a730b17

Findings (none blocking):

• [non-blocking] internal/database/postgres/testhelpers/shared.go (template/buildTemplate) - Only the container start is detached from the caller's context (context.WithoutCancel). The template migration still runs under the first caller's ctx, and templates[key] keeps whatever error it returns for the life of the binary.
  • Failure scenario: if the first test's deadline or cancel lands mid-migration, every later test fails with "migrations failed to apply to a fresh database", which names the wrong cause.
  • The risk is low: the shortest caller ctx in the package is 60s, and the template builds in about 2s.
  • Fix: detach buildTemplate too, or do not cache ctx errors.
• [non-blocking] internal/config/ri_exchange_reservation_integration_gate_test.go:24, :42 - The doc comment and error text still say "use testhelpers.RequirePostgresContainer instead". The new guard (shared_postgres_guard_test.go) forbids that call in this package, so the two guards give contradictory advice. They should point to RequireMigratedDatabase.

Attacked:

• Isolation: each test gets its own CREATE DATABASE ... TEMPLATE copy, and the template is sealed with ALLOW_CONNECTIONS false.
  • Advisory locks and sequences are per database.
  • The only cluster-wide state is the unfiltered pg_stat_activity/pg_locks probes in the ladder_lock, ladder_amend, marketplace_claim and ri_exchange tests. They are safe because every DB-backed test in the package runs sequentially: t.Parallel is used only by pgxmock unit tests. The PR body already flags this for anyone adding t.Parallel later.
• Global objects: migrations add no roles or global objects, only per-database extensions. Both migrations-path helpers resolve to the same cleaned path, so there is one template.
• Ordering dependence: the full suite passes normally and with -shuffle=on, with identical counts.
• Fail vs skip: only SkipIfProviderIsNotHealthy skips. A container that fails to start fails the test, and the error is remembered.
• Cleanup: TestMain terminates the container, with Ryuk as backup, and no containers were left behind.
• The guard: copied into the base export, it fails and lists 19 call sites.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  1. The drop cleanup removed: DropsDatabaseWhenTestEnds FAILS.
  2. ALLOW_CONNECTIONS false removed: TemplateRefusesConnections FAILS.
  3. One shared, never-dropped database handed to every caller: 75 top-level config tests FAIL, so the suite does catch shared state.
  4. An image that cannot be pulled: 36 tests FAIL with the remembered start error, and 0 skip. A broken start fails loudly in CI.

Verified locally, on a git archive export of HEAD with no cloud credentials, with the integration runs under the heavy lock:

• go build ./...: ok. go vet, with and without -tags integration, on ./internal/config/ and ./internal/database/...: ok. gofmt -l: clean.
• go test -count=1 ./internal/config/ ./internal/database/postgres/testhelpers/: ok.
• go test -tags integration -race -count=1 ./internal/database/postgres/testhelpers/: ok, and all 4 new tests pass.
• go test -tags integration -race -count=1 -timeout=20m ./internal/config/...: 1347 pass, 0 fail, 3 skip (the skips are gated on env vars that are unset). With -shuffle=on, the same counts.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at a730b177f6934bd6a0af0f4d12a465cb94fad179 merged onto current main (7eac792b; merge-tree acec9ea3; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at a730b177f6934bd6a0af0f4d12a465cb94fad179).