[IAmJSD]

The Integration Tests job spends most of its ~17 minutes starting Postgres containers. Main's last green run started 459 of them, one per test (two per test in a few tables). Each costs about 2s to start, plus a full migration of all 127 migrations. A test that does nothing else takes 4-5s, so roughly 2000s of the run's 2784s of package time was setup.

The fix was already in the repo but almost unused: testhelpers.RequireMigratedDatabase (#982) starts one server per test binary and migrates a template database once. Each test then gets its own CREATE DATABASE ... TEMPLATE copy, which takes milliseconds and is still a fresh, fully migrated database. Only internal/config used it. This PR moves every other package onto it.

What changed

• testhelpers.RequireEmptyDatabase(ctx, t) (new): a fresh, empty database (from template0) on the same shared server, dropped when the test ends. It's for tests that need an unmigrated database: migration up/down/dirty/heal tests, step-by-step migration fixtures, and connection-only tests. Integration tests cover that it's empty, isolated, dropped on cleanup, and distinct for concurrent callers.

• Migrated templates are keyed by absolute path, so different relative paths to the same migrations directory share one template.

• SetupPostgresContainer / RequirePostgresContainer: the doc comments now point to the shared helpers and say when a dedicated server is still right. Neither helper is removed.

• Converted packages, each with a TestMain that stops the shared server:

  • internal/api
  • internal/purchase
  • internal/database/postgres/migrations
  • internal/scheduler
  • internal/auth
  • internal/analytics
  • internal/server
  • internal/commitmentopts
  • internal/database
  • cmd/cleanup-lambda

  Packages that already had an untagged TestMain (api, purchase) get an afterTests hook, so the untagged and integration builds both compile.

• Lock checks: pg_stat_activity lock-wait polls are now limited to current_database(), so another test's database on the same server can't satisfy them.

• One test keeps its own container: TestPGPasswordAndKeywordDSN_ConnectWithHostilePassword (migrations). It changes the password of the login role every shared database uses. A comment at the call site says so. Every other test runs on the shared server. Each package was read for server-wide operations (roles, ALTER SYSTEM, connection limits, killing sessions, unscoped catalog queries), and none were found besides that one.

• Docs: the integration-test example in docs/DEVELOPMENT.md now shows the shared pattern.

Results

Whole suite, run locally the way CI runs it (go test -race -tags=integration ./..., 16-core box):

	Before	This branch
Wall time	1023s on CI	346s locally
Postgres containers	459	15

The wall-time cells come from different machines (the CI runner vs a 16-core box), so they aren't directly comparable. The container count is the real driver, and it doesn't depend on the machine.

Per package, compiled -race -tags integration test binary. Containers were counted with docker events per testcontainers session. Times were taken while other jobs loaded the machine, so they're noisy; the container counts are exact.

Package	Before	After
internal/api	927s, 151 containers	119s, 1
internal/purchase	1035s, 116	95s, 1
internal/database/postgres/migrations	885s, 92	286s, 2
internal/scheduler	317s, 29	20s, 1
internal/auth	365s, 23	121s, 1
internal/analytics	192s, 19	22s, 1
internal/server	306s, 13	115s, 1
internal/commitmentopts	57s, 5	15s, 1
cmd/cleanup-lambda	47s, 4	11s, 1
internal/database	51s, 4	23s, 1

What remains is mostly the tests themselves:

• password hashing in auth
• TestApplicationRateLimiter in server
• TestOpenAPI403OnPermissionGatedRoutes in api
• walking all 127 migrations in the migrations package

Verification

Run on this branch's combined tree, with each package's work merged together:

• go build ./..., go vet ./... and go vet -tags integration ./...: pass.
• go test -race -short ./...: pass.
• go test -race -tags=integration -timeout=30m ./...: pass, in 346s, with 15 containers.
• Each package was also run on its own with -shuffle=on (seeds in the commit notes): pass.
• golangci-lint on the changed packages: 0 issues.
• The testhelpers false-green skip guards still pass: a database setup failure is a FAIL, never a skip (#1597).
• git-secrets scan: clean, and the planted key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS.

Notes for review

• Behavior change for 8 migration tests: these use a migrated copy where a full migration run was only setup (000064, 000065, 000096, 000102, 000103). They no longer re-run the chain on their own database. The other 74 migration-package call sites, the idempotency test and the down-all round trip still run the real chain on fresh databases.
• Connection limit: the shared server keeps Postgres's default of 100 connections. No converted package runs DB tests in parallel, and the largest single test uses about 40 connections. If a future package parallelizes, raise the limit for the shared server only.
• Not fixed here (pre-existing):
  • TestHandler_getConfig_SourceIdentity_AdminOnly (internal/api) reaches the AWS metadata endpoint when it runs early under some shuffle seeds, and the no-network guard then fails the binary. This reproduces on main.
  • .gitignore's cleanup-lambda entry also ignores the cmd/cleanup-lambda/ directory, so a new file there is silently untracked. That's why cleanup-lambda's TestMain sits in its existing main_integration_test.go.
  • internal/testutil.SetupPostgresContainer now has no callers and could be deleted in a follow-up.
• Commit history: commit e0384ad1 (internal/database) also carries the cleanup-lambda conversion because of that .gitignore rule. A squash merge makes this moot.
• Note for cristim: this is test infrastructure, which you own. It changes how every integration package gets its database, so please look at the helper (internal/database/postgres/testhelpers/shared.go) and the migrations-package conversions in particular.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 110 billable files and costs up to $27.50.}

View limit details

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 53e7a78
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b0ca7c42-399e-456a-a876-46ed43d1623e

Commits

Reviewing files that changed from the base of the PR and between 53e7a78 and 4ef5787.

Files selected for processing: 110

• cmd/cleanup-lambda/main_integration_test.go
• docs/DEVELOPMENT.md
• internal/analytics/main_integration_test.go
• internal/analytics/postgres_analytics_account_filter_db_test.go
• internal/analytics/postgres_analytics_db_test.go
• internal/analytics/postgres_analytics_integration_test.go
• internal/api/db_rate_limiter_integration_test.go
• internal/api/handler_analytics_scope_provider_integration_test.go
• internal/api/handler_config_propagation_integration_test.go
• internal/api/handler_ladder_timeline_integration_test.go
• internal/api/handler_marketplace_compensation_integration_test.go
• internal/api/handler_plans_concurrency_integration_test.go
• internal/api/handler_purchases_submit_dedupe_integration_test.go
• internal/api/handler_recommendations_global_filters_integration_test.go
• internal/api/handler_ri_exchange_history_scope_integration_test.go
• internal/api/handler_ri_exchange_integration_test.go
• internal/api/handler_users_update_race_integration_test.go
• internal/api/main_integration_test.go
• internal/api/main_nonet_test.go
• internal/api/nullable_purchase_cost_integration_test.go
• internal/api/permission_constraints_config_integration_test.go
• internal/api/permission_constraints_marketplace_integration_test.go
• internal/api/recommendation_identity_integration_test.go
• internal/api/ri_exchange_four_eyes_system_integration_test.go
• internal/api/ri_utilization_cache_integration_test.go
• internal/auth/main_integration_test.go
• internal/auth/store_postgres_db_test.go
• internal/commitmentopts/main_integration_test.go
• internal/commitmentopts/store_postgres_test.go
• internal/database/advisory_lock_holder_integration_test.go
• internal/database/advisory_lock_integration_test.go
• internal/database/main_integration_test.go
• internal/database/pool_floor_integration_test.go
• internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go
• internal/database/postgres/migrations/000057_drop_user_role_to_groups_test.go
• internal/database/postgres/migrations/000060_cleanup_universal_plans_test.go
• internal/database/postgres/migrations/000064_relocate_purchaser_group_test.go
• internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go
• internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go
• internal/database/postgres/migrations/000086_remove_approve_own_from_standard_users_test.go
• internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go
• internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go
• internal/database/postgres/migrations/000089_rename_cancelled_to_canceled_test.go
• internal/database/postgres/migrations/000093_recommendations_state_owner_id_test.go
• internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go
• internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go
• internal/database/postgres/migrations/000098_backfill_execution_step_number_test.go
• internal/database/postgres/migrations/000099_user_deactivated_at_test.go
• internal/database/postgres/migrations/000100_hash_approval_tokens_at_rest_test.go
• internal/database/postgres/migrations/000102_purchase_plan_update_token_test.go
• internal/database/postgres/migrations/000103_api_key_password_version_test.go
• internal/database/postgres/migrations/000104_user_mfa_last_totp_counter_test.go
• internal/database/postgres/migrations/000105_purchase_history_listing_attempt_test.go
• internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units_test.go
• internal/database/postgres/migrations/000107_ladder_tranche_amendments_test.go
• internal/database/postgres/migrations/000109_recommendations_availability_zone_test.go
• internal/database/postgres/migrations/000110_drop_recommendations_8col_natural_key_test.go
• internal/database/postgres/migrations/000111_recommendations_platform_tenancy_test.go
• internal/database/postgres/migrations/000112_plan_step_cap_auto_purchase_switch_test.go
• internal/database/postgres/migrations/000113_purchase_executions_no_submission_test.go
• internal/database/postgres/migrations/000114_ladder_tranches_status_reason_test.go
• internal/database/postgres/migrations/000115_auto_purchase_switch_stamps_test.go
• internal/database/postgres/migrations/000117_ladder_tranches_settled_at_test.go
• internal/database/postgres/migrations/000118_purchase_plans_ramp_baseline_test.go
• internal/database/postgres/migrations/000119_drop_recommendations_9col_natural_key_test.go
• internal/database/postgres/migrations/000120_recommendations_source_account_test.go
• internal/database/postgres/migrations/000121_ladder_tranche_amount_provenance_test.go
• internal/database/postgres/migrations/000123_purchase_execution_fan_out_shapes_test.go
• internal/database/postgres/migrations/000125_lock_default_user_groups_test.go
• internal/database/postgres/migrations/000126_purchase_history_active_expiry_idx_test.go
• internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at_test.go
• internal/database/postgres/migrations/000129_recommendations_state_partial_test.go
• internal/database/postgres/migrations/000130_purchase_executions_created_by_system_test.go
• internal/database/postgres/migrations/000131_ri_exchange_direct_executions_test.go
• internal/database/postgres/migrations/000132_savings_partition_utc_bounds_test.go
• internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class_test.go
• internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go
• internal/database/postgres/migrations/backfill_admin_group_ids_test.go
• internal/database/postgres/migrations/ensure_admin_user_test.go
• internal/database/postgres/migrations/main_integration_test.go
• internal/database/postgres/migrations/migrate_autoheal_test.go
• internal/database/postgres/migrations/migrate_cancel_integration_test.go
• internal/database/postgres/migrations/migrate_down_all_test.go
• internal/database/postgres/migrations/migrate_idempotency_test.go
• internal/database/postgres/migrations/migrate_pgpassword_integration_test.go
• internal/database/postgres/migrations/migrate_security_integration_test.go
• internal/database/postgres/migrations/savings_snapshots_pk_test.go
• internal/database/postgres/migrations/split_savingsplans_test.go
• internal/database/postgres/testhelpers/postgres.go
• internal/database/postgres/testhelpers/shared.go
• internal/database/postgres/testhelpers/shared_integration_test.go
• internal/database/query_logging_integration_test.go
• internal/purchase/azure_pricing_integration_test.go
• internal/purchase/main_integration_test.go
• internal/purchase/main_nonet_test.go
• internal/purchase/plan_step_ledger_migration_integration_test.go
• internal/purchase/ramp_step_progress_integration_test.go
• internal/scheduler/main_integration_test.go
• internal/scheduler/recommendation_completeness_integration_test.go
• internal/scheduler/recommendation_source_account_integration_test.go
• internal/scheduler/scheduler_global_filters_integration_test.go
• internal/scheduler/suppression_variant_integration_test.go
• internal/server/app_islambda_integration_test.go
• internal/server/app_rate_limiter_integration_test.go
• internal/server/handler_ladder_immediate_integration_test.go
• internal/server/handler_ladder_lock_integration_test.go
• internal/server/handler_ri_exchange_reservation_integration_test.go
• internal/server/integration_test.go
• internal/server/main_integration_test.go
• internal/server/permission_constraints_config_integration_test.go

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

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context), done after the merge: APPROVE 4ef5787760ce0fa823dcf63e8867ec7712835011

Blocking: none.

Non-blocking:

1. internal/database/postgres/testhelpers/shared.go:198-206 (buildTemplate) and :192.
   • The template migration runs on the first caller's ctx. start() detaches the ctx with WithoutCancel, but buildTemplate does not.
   • The result, including any error, is cached for the whole binary.
   • Scenario: the first test to ask for a migrated database has a short deadline, or is canceled while the chain is applying. The template is left half-built and every later RequireMigratedDatabase test in that package fails with the same error.
   • It fails, it doesn't skip, so it's not a false green. It's a cascade risk though. Today the shortest deadline before a call is 60s (internal/config/store_postgres_db_test.go:39), so it's latent.
   • Fix: build the template under context.WithoutCancel(ctx) with its own timeout.
2. shared.go:89. SkipIfProviderIsNotHealthy runs on every call, even after the shared server is already up and answering.
   • Scenario: under CI load, a transient Docker health-probe failure turns a test into a SKIP while its database is perfectly reachable. That's a (#1597-style) false-green path.
   • Not a regression: RequirePostgresContainer probed on every test too.
   • Fix: skip the probe once shared.admin != nil.
3. shared.go:222-240 together with :120.
   • dropDatabase reads s.admin without the mutex.
   • TerminateSharedPostgres sets it to nil, and shared_integration_test.go:164/:312 call Terminate in the middle of the run.
   • Scenario: a database whose drop cleanup is still pending when Terminate runs (a parent test holding one around a subtest that resets the server) gets a nil dereference in cleanup.
   • Only reachable inside the testhelpers package today.
4. Pre-existing, unchanged: pool.Close in the t.Cleanup (requireSharedDatabase) blocks if a test leaks an acquired pgx connection or open tx. The old container.Cleanup did the same.

Attacked (held up):

• Isolation:
  • Every per-test database is a CREATE DATABASE ... TEMPLATE clone.
  • The template is sealed (IS_TEMPLATE, ALLOW_CONNECTIONS false). That applies to the superuser too, so nothing can write to it or block clones.
  • Migrations have no CREATE ROLE, ALTER SYSTEM, ALTER DATABASE, or current_database dependence. The extensions they create are per-database.
  • Advisory locks are keyed per database. Production AdvisoryLockHeldFor filters on l.database.
  • Every pg_stat_activity and pg_locks poll in the converted packages is scoped to the current database:
    • api plans_concurrency
    • purchase ramp_step_progress
    • purchase plan_step_fail_atomic
    • server ladder_lock
    • migrate_cancel (scoped by datname)
    • 000132 (scoped by pid)
  • The one role-mutating test (ALTER ROLE PASSWORD) keeps a dedicated container. ALTER DATABASE SET (000132) targets the test's own database.
  • No converted DB test uses t.Parallel. Env-var tests (CUDLY_MIGRATION_AUTOHEAL, DB_LOG_BIND_PARAMETERS) set the variable after taking their database, or don't affect the template.
  • Log-capture tests only assert on specific substrings. Template-build logs that land in the first test's capture can't flip them.
• Cleanup ordering:
  • The drop is registered before connecting, so conn.Close runs first (LIFO).
  • DROP ... WITH (FORCE) kills leaked sessions. The DropsDatabaseWhenTestEnds test pins this.
  • Clones are serialized under the mutex.
  • Names (cudly_test_N) are unique per binary because seq survives Terminate.
• #1597:
  • SkipIfProviderIsNotHealthy is the only skip.
  • A start, template, clone or connect error goes through require.NoError and FAILs.
  • A start error is cached, so a broken Docker fails fast.
  • RequireMigratedDatabase with an empty path FAILs.
• Lost coverage:
  • Every hunk is a mechanical setup swap. No assertion was removed or changed.
  • The 7-8 RunMigrations→RequireMigratedDatabase sites (000064, 000065 x2, 000096 x2, 000102, 000103) are acceptable. The template is built by the identical RunMigrations(path, "", "") call on a fresh database, and 000096's idempotency subtest re-executes the .up.sql directly anyway.
  • Step-by-step and down tests, idempotency, down-all, autoheal, ensure_admin and migrate_cancel all still walk the real chain on empty template0 databases.
  • migrate_cancel's advisory-lock ID is still derived from the actual database name.
• TestMain wiring:
  • go vet ./... and go vet -tags integration ./... are both clean, so there's no duplicate TestMain.
  • api and purchase register their teardown through afterTests from init. The teardown runs before the no-network guard stops.
  • All 11 packages that use the shared helpers have a terminate path.
  • Verbose purchase run: 1 postgres container started and was terminated after the last test.
  • None of my test containers were left running afterwards.
• Runs, all -race -tags integration -count=1 -shuffle=on, all PASS:
  • testhelpers 37s
  • database 22s
  • commitmentopts 13s
  • scheduler 21s
  • cleanup-lambda 12s
  • migrations 265s
  • server 127s
  • analytics 21s
  • purchase 114s (seed 1792895711586530634)
• Peak client backends sampled on the migrations container: 11 of 100.
• CI: -timeout=20m applies per binary; the largest (migrations) took about 265s here.
• Ryuk keeps the container for the life of the session, which is the same pattern internal/config already used.

The three non-blocking testhelpers items (the template built under the first caller's context, the health probe running on every call, and s.admin read without the lock) are being fixed in the follow-up PR that parallelizes the migrations tests.