[IAmJSD]

Root cause

Several comments explained the migration path wrongly. They said the golang-migrate driver "wraps every migration in BEGIN ... COMMIT". It doesn't. The real mechanism, checked against the pinned code:

• Driver execution. newCancelableMigrator (internal/database/postgres/migrations/migrate_cancel.go:118-148) builds the golang-migrate v4.19.1 pgx/v5 driver with Config{DatabaseName: ...} only, so MultiStatementEnabled is false. No DSN in the repo sets x-multi-statement, and buildMigrateDSN (migrate.go:582) only adds sslmode. With that setting, Postgres.Run (database/pgx/v5/pgx.go:252-270) calls io.ReadAll on the file and passes it to runStatement (pgx.go:272-302), which makes a single p.conn.ExecContext(ctx, query) call with no arguments. The driver adds no BEGIN/COMMIT.
• Protocol. pgx v5.9.2 stdlib ExecContext calls Conn.Exec, and conn.go:515-517 forces QueryExecModeSimpleProtocol when there are no arguments. The whole file goes to PostgreSQL as one simple Query message.
• Atomicity. PostgreSQL runs a multi-statement simple Query as one implicit transaction, unless the text has its own transaction control. 000040 and the 000101 down migration wrap the whole file in BEGIN/COMMIT, which is also atomic. A one-statement file is atomic because it is a single statement.
• Dirty state. golang-migrate runMigrations (migrate.go:723-772) calls SetVersion(N, true) in its own committed transaction (pgx.go:339-371) before Run, and calls SetVersion(N, false) only after Run succeeds. A failed or interrupted file therefore rolls back completely but leaves dirty=true at N. Up() refuses a dirty DB with ErrDirty, whose text is "Dirty database version N. Fix and force version." (migrate.go:52-54, :276). Our maybeAutoHealDirty (internal/database/postgres/migrations/migrate.go:407) fails loud and never clears the flag.

Some recovery comments had also gone stale or were wrong:

• newMigratorWithRecovery's inline comment still described the old Force-and-continue auto-heal.
• The AUTOHEAL=false error text was quoted as "migration: N is dirty".
• The force-down target was given as "N-1". The numbering has gaps (no 000061/000062, 000068/000069, 000082, 000084/000085), and golang-migrate readUp (migrate.go:532-541) fails with "no migration found for version" when the forced version has no file.
• The guard pointed at deploy-runner "issue fix(scripts): detect bare AWS secret access keys in git-secrets setup #1125". In this repository fix(scripts): detect bare AWS secret access keys in git-secrets setup #1125 is an unrelated merged git-secrets PR. The predecessor repo's issue was transferred here as Move DB migrations off the request/connect path into a one-shot deploy-time runner #31 ("Move DB migrations off the request/connect path into a one-shot deploy-time runner", open), so the reference now points there.

Fix (comments only)

• internal/database/postgres/migrations/migration_transactional_test.go: the package comment now separates driver execution, PostgreSQL file atomicity, the policy of keeping non-transactional statements out of the request-time path (RunMigrations from ensureDB), and manual fail-loud dirty-state recovery. It also states three facts that were missing before:

  • the -- migrate:no-transaction marker is read only by this test, not by the runner;
  • no file uses the marker;
  • Move DB migrations off the request/connect path into a one-shot deploy-time runner #31 doesn't exist yet as a runner.

  The forbiddenPatterns comments now say that plain REINDEX and REFRESH MATERIALIZED VIEW CONCURRENTLY can run in a transaction block and are listed as policy. The dollar-body comment no longer claims a DO body runs "later, outside the migration transaction". The t.Logf/t.Errorf message text changed to match. The guard's logic, patterns and opt-out are unchanged.

• migrate.go: corrected comments on RunMigrations, newMigratorWithRecovery, maybeForceMigrationVersion and maybeAutoHealDirty, covering the atomicity source, the fail-loud dirty check, the force-down target with gaps, and the real ErrDirty text. No code or error-string changes.

• migrate_autoheal_test.go: two comments that quoted the wrong ErrDirty text.

• 000109_...up.sql, 000111_...up.sql: the "runner wraps every migration in a transaction" sentence is replaced. Only SQL comment lines changed; executable statements are identical. This is safe for already-applied files: golang-migrate stores only (version, dirty) (pgx.go:357) and never checksums or re-reads applied files. No repo test or CI step hashes migration files. The pre-commit migration hook only checks for duplicate numbers.

Closes #874

Verification

Commands, run on this branch (fix/issue-874, 1720539):

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true
go build ./...                                   # ok
go vet ./... && go vet -tags integration ./...   # ok
go test -count=1 ./internal/database/postgres/migrations/   # ok
go test -count=1 -v -run TestMigrationsAreTransactional ./internal/database/postgres/migrations/
  # transactional-safety scan: 254 migration files scanned, 0 skipped (opt-out); PASS
go test ./internal/mocks/ -count=1               # ok
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/database/postgres/migrations/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/database/postgres/migrations/migrate.go   # no output

No regression test was added. This is a comments-only change and has no fail-before/pass-after. To check the guard still behaves the same, I ran a mutation probe with a temporary, uncommitted 999999_*.up.sql file (deleted afterwards):

• A file with CREATE INDEX CONCURRENTLY x ON y (z); still fails TestMigrationsAreTransactional (with the new message).
• The same file with the -- migrate:no-transaction marker is skipped with the warning (1 skipped).

I checked the claims empirically on real Postgres (postgres:16-alpine) with a throwaway integration test that went through migrations.RunMigrations with temp migration dirs (not committed):

• CREATE TABLE t874_a ...; SELECT 1/0; returned an error, left version=1 dirty=true, and the table was absent. A failing later statement rolls back the earlier one.
• The same with a whole-file BEGIN; ... COMMIT; also left the table absent and dirty=true.
• CREATE TABLE ...; CREATE INDEX CONCURRENTLY ... failed with "CREATE INDEX CONCURRENTLY cannot run inside a transaction block", left the table absent and dirty=true.
• A file containing only CREATE INDEX CONCURRENTLY ... succeeded (dirty=false, index exists). The guard rejects this case as policy, not because PostgreSQL refuses it.
• With CUDLY_MIGRATION_AUTOHEAL=false the dirty DB gave failed to run migrations: Dirty database version 1. Fix and force version. With the default, it gave the auto-heal: migration 1 is dirty; ... instructions.
• With migrations 000001 and 000003 and dirty at 3, CUDLY_FORCE_MIGRATION_VERSION=2 gave no migration found for version 2. =1 re-applied 3 and ended clean.

I also ran psql against my own postgres:16 container:

• REFRESH MATERIALIZED VIEW CONCURRENTLY and plain REINDEX TABLE succeed inside BEGIN ... COMMIT.
• REINDEX ... CONCURRENTLY, VACUUM, ALTER SYSTEM, CREATE TABLESPACE and DROP INDEX CONCURRENTLY are refused in a multi-statement query.
• EXECUTE 'CREATE INDEX CONCURRENTLY ...' in a DO block fails with "cannot be executed from a function".

The existing TestRunMigrations_DeadlineStopsMigrationSQLBlockedByForeignLock (migrate_cancel_integration_test.go:102) already covers the interrupted-migration case (rolled back, dirty at N). I didn't re-run it; the combined-tree run covers it.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope (noted, not changed)

• 000074_repair_partial_migration_058_067.up.sql:18-19 repeats "golang-migrate wraps the file in a single transaction". The issue only named 000109 and 000111, so I left the other applied migrations alone. A maintainer may want the same one-line comment fix there.
• 000003_analytics_partitions.up.sql:177 says REFRESH MATERIALIZED VIEW CONCURRENTLY "cannot run inside a transaction block". PostgreSQL 16 allows it; it is only refused on a never-populated view. Comment only, left as is.
• maybeAutoHealDirty's runtime error message always prints CUDLY_FORCE_MIGRATION_VERSION=N-1 as the force-down target, which is wrong when N-1 has no file. The comments now explain this. Changing the message is a behavior change for its own issue.
• The guard's REINDEX and REFRESH ... CONCURRENTLY entries are broader than PostgreSQL's rule. They were kept and documented as policy. Narrowing them is a guard change and needs a maintainer decision.
• Real support for non-transactional migrations needs the deploy-time runner design in Move DB migrations off the request/connect path into a one-shot deploy-time runner #31.

Maintainers: please mirror the issue's triage labels onto this PR: impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/s, type/docs.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

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.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 4fe15da8c84a3e2a2a6463edcdbb968ca5b685b6

Blocking:

• none

Non-blocking:

• internal/database/postgres/migrations/migrate.go:388: the comment now admits that the maybeAutoHealDirty error "always prints N-1". When there is a numbering gap (000061/62, 000068/69, 000082, 000084/85), that printed force target is wrong, and following it gives no migration found for version. Fixing the printed target to the previous version that has a file is a small code change, better as a follow-up issue than as a comment.

Attacked:

• Behavior change: confirmed the diff touches only comments and t.Logf/t.Errorf text. Every regex in forbiddenPatterns and every executable SQL line in 000109/000111 is byte-identical. golang-migrate stores only (version, dirty). No CI step, hook or test checksums migration files: the only migration hook is the duplicate-number check in .pre-commit-config.yaml.
• Driver and pgx claims: checked against the pinned module cache (go.mod pins golang-migrate v4.19.1 and pgx v5.9.2):
  • database/pgx/v5/pgx.go Run uses io.ReadAll and then one runStatement → ExecContext when MultiStatementEnabled is false.
  • pgx conn.go forces QueryExecModeSimpleProtocol when there are no arguments.
  • ErrDirty.Error() is "Dirty database version %v. Fix and force version.".
• Migration-file claims: checked against the tree:
  • 000109 and 000111 each have exactly two statements.
  • Only 000040 (up and down) and the 000101 down migration contain BEGIN/COMMIT.
  • No file uses -- migrate:no-transaction.
  • The numbering gaps are exactly 000061, 000062, 000068, 000069, 000082, 000084 and 000085.
• Checks run: go build ./..., then go vet with and without -tags integration, then go test -race -short -count=1 on ./internal/database/postgres/migrations/ (includes TestMigrationsAreTransactional). golangci-lint v2.10.1 with --build-tags integration: 0 issues. go test -race -tags integration with -run TestMigrationsAreTransactional|TestMigrations_AutoHealDirty (real Postgres): pass, including the AUTOHEAL=false subtest whose quoted error text the comments now cite. CI integration jobs are green on this head.
• Fail-before: not applicable. This is a comments-only change.

Body-stale:

• none

_{Reviewer: rB5. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 4fe15da8c84a3e2a2a6463edcdbb968ca5b685b6 merged onto main (1ceaafd1), together with #1281 in that order (combined tree 73e9c97a, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 4fe15da8, green CI, and the combined-tree verification above. Squash-merged at exactly 4fe15da8c84a3e2a2a6463edcdbb968ca5b685b6.