fix(migrations): run bootstrap housekeeping on the migrator's held connection

[rubenfiszel]

Summary

During startup, migrate() and fix_flow_versioning_migration re-acquired a second connection from the pool while already holding one — the connection the migrator has checked out (and holds the migration advisory lock on). That's a self-deadlock against any backend limited to one connection at a time:

• connection-constrained managed Postgres (tight max_connections),
• PgBouncer in transaction-pooling mode,
• an embedded single-connection dev database (pglite/oliphaunt — see WIN-2130).

The runtime itself is fine on a single connection; this was purely a migration-bootstrap issue.

Fix

Add a CustomMigrator::connection() accessor that exposes the already-checked-out connection, and route the migration-phase housekeeping onto it instead of re-acquiring from the pool:

• windmill-api/src/db.rs — the two DELETE … _sqlx_migrations housekeeping queries in migrate() now run on custom_migrator.connection().
• windmill-api/src/live_migrations.rs — fix_flow_versioning_migration runs its existence check and its transaction on the migrator's connection (drops the now-unused pool parameter).

This reduces connection usage during migration, and for fix_flow_versioning the existence check and the write now happen on the same advisory-locked connection (tighter, not looser). Normal multi-connection deployments are unaffected — same SQL, same ordering, just a different (already-held) executor.

Test plan

• Default multi-connection dev backend rebuilds and boots healthy (workers_alive, database_healthy=true).
• Fresh Postgres DB with the runtime pool capped at one connection (DATABASE_CONNECTIONS=1): server+worker boot, a bash script and a 2-step flow complete, pg_stat_activity shows a single connection.
• End-to-end on an embedded single-connection Postgres (WIN-2130 experiment): migrations apply, server+worker boot, scripts and flows run.

Context / experiment that surfaced this: WIN-2130 (#9967).

🤖 Generated with Claude Code

---

Summary by cubic

Prevent self-deadlock during startup migrations by running bootstrap housekeeping on the migrator’s already-held connection instead of acquiring another. Unblocks single-connection backends (PgBouncer tx-pool, low max_connections, embedded dev DB) with no behavior change for multi-connection deployments; addresses WIN-2130.

• Bug Fixes
  • Added CustomMigrator::connection() to use the migrator’s checked-out, advisory-locked connection.
  • Routed housekeeping DELETEs in migrate() and the flow versioning fix to that connection; removed the DB/pool param from custom_migrations.
  • Existence check and write for fix_flow_versioning_2 now run on the same connection; prevents self-deadlock on single-connection setups (WIN-2130).

^{Written for commit 54ce0c4. Summary will update on new commits.}

[claude]

Claude finished @rubenfiszel's task in 1m 15s —— View job

---

Review

Good to merge

I traced the change through db.rs and live_migrations.rs against origin/main. It does exactly what the description says and I found no blocking issues.

What the fix does correctly

• CustomMigrator::connection() returns &mut *self.inner, i.e. the same PoolConnection<Postgres> the migrator checked out. That's precisely the connection lock()/unlock() operate on (db.rs:141-152, 188-191) via pg_try_advisory_lock. Advisory locks are session-scoped, so routing the housekeeping DELETEs and the fix_flow_versioning_2 existence-check + write onto this connection keeps them on the same locked session — tighter, not looser, as claimed.
• In migrate(), the two housekeeping DELETEs (db.rs:288, 326) previously ran on the pool (db) and still autocommit the same SQL in the same order — no behavior change on multi-connection deployments, and one fewer connection acquired.
• In fix_flow_versioning_migration, migrator.connection().begin() (live_migrations.rs:56) opens the transaction on the held connection. use sqlx::Acquire; was correctly added — &mut PgConnection: Acquire. Each connection() borrow is released across await points (sequential calls), so there's no double-&mut conflict with the interleaved migrator.lock() / list_applied_migrations() calls, which also target inner.
• PgConnection is already imported in db.rs (db.rs:13), so the new signature compiles cleanly.

Notes (non-blocking)

• The doc comment and inline comments are appropriately scoped to the invariant ("must run on the held connection, else self-deadlock on a single-connection backend") rather than narrating the code — consistent with the AGENTS.md comment guidance.
• The first housekeeping DELETE still runs before the advisory lock is taken (as it did before, when it used the pool). That's unchanged and fine — this PR isn't altering lock ordering.

Test coverage

Backend-only change to the migration-bootstrap path. This is executor-routing refactor of existing SQL, and the migration bootstrap runs on every startup, so the existing startup path exercises it — no new unit test is warranted, and the codebase has no harness for a single-connection-pool migration scenario. The PR's manual verification (default multi-connection boot, DATABASE_CONNECTIONS=1 boot with a single pg_stat_activity connection, and the embedded single-connection WIN-2130 run) is the right way to validate this and covers the regression that motivated it. Nothing further needed before merge.

[github-actions]

Pi Review

Good to merge — clean, well-scoped fix. No blocking issues or nits.

What's happening

The PR prevents a self-deadlock during startup migration bootstrap on single-connection Postgres backends. Two call sites (migrate() housekeeping queries and fix_flow_versioning_migration) were acquiring a second connection from the pool while the migrator already held one (with the migration advisory lock on it). On backends limited to one connection (embedded pglite/oliphaunt, PgBouncer tx-pooling, tight max_connections), that second acquire() blocks forever.

The fix exposes the migrator's already-checked-out PgConnection via CustomMigrator::connection() and routes all migration-phase housekeeping onto it. The db: &DB pool parameter is removed from the call chain.

Things I checked

• Borrow-checker: connection() returns &mut PgConnection. All callers in the diff use it sequentially (no overlapping borrows). The Transaction created via migrator.connection().begin() borrows the connection, but no subsequent migrator.connection() call appears while tx is alive.

• Lock + queries on the same connection: fix_flow_versioning_migration acquires migrator.lock() (an advisory lock on the migrator's connection) and now runs its queries on the same connection. This is actually a correctness improvement — the old code held the lock on connection A but queried on B (from the pool), so the lock didn't serialize data access. The new code is tighter.

• Lock lifecycle: The housekeeping DELETEs run before any lock is acquired. run_direct() acquires and releases the migration lock internally. custom_migrations() re-acquires its own lock afterward. No lock leaks across these phases.

• sqlx::query! / sqlx::query_scalar!: These compile-time-checked macros work with &mut PgConnection (it implements Executor). The compile-time SQL checks reference the same database, unaffected by which connection the query runs on at runtime.

Test coverage

Backend-only change touching migration bootstrap connection routing. The existing migration integration tests cover the normal multi-connection path. The PR description documents manual testing against three single-connection configurations (pool capped at 1, embedded pglite/oliphaunt, and default multi-connection). No new automated tests expected for connection-routing logic.

Manual verification needed before merge: none beyond what's already documented in the PR test plan — the tested scenarios (single-connection boot + script/flow execution) directly exercise the deadlock surface this PR removes.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	54ce0c4
Status:	⚡️  Build in progress...

View logs

[github-actions]

Pi Review

Good to merge — no changes since the prior Pi review on this same commit, and the analysis below confirms the earlier verdict.

What changed

The PR prevents a self-deadlock during startup migration bootstrap on single-connection Postgres backends (embedded pglite, PgBouncer tx-pooling, tight max_connections). Two migration-phase call sites were acquiring a second connection from the pool while the CustomMigrator already held one:

• Two housekeeping DELETE … FROM _sqlx_migrations queries in migrate() (db.rs:283, db.rs:321)
• The existence check and transaction in fix_flow_versioning_migration (live_migrations.rs:31, :47, :54)

The fix exposes the migrator's already-checked-out PgConnection via CustomMigrator::connection() and routes all migration-phase housekeeping onto it, removing the db: &DB pool parameter from the call chain.

Re-verified items

• Borrow checker: All connection() calls in fix_flow_versioning_migration are sequential. After migrator.connection().begin() creates the Transaction<'_ , Postgres> (live_migrations.rs:54), the tx variable is the only borrower used until it's dropped at the end of its block — no subsequent migrator.connection() or migrator.lock()/unlock() calls overlap with tx's lifetime.

• Lock + queries on the same connection: In fix_flow_versioning_migration, migrator.lock() acquires a pg_advisory_lock on the migrator's connection. All subsequent queries (fetch_one existence re-check at line 47, tx.begin() at line 54) now run on that same connection. This is actually a correctness improvement over the old code (lock on connection A, queries on B from the pool).

• Lock lifecycle in migrate(): Both housekeeping DELETEs run before any advisory lock is taken (the lock is acquired inside run_direct() and later inside custom_migrations()). The connection routing alone prevents the deadlock — no lock-ordering change.

• PgConnection import: Already present in db.rs:13; sqlx::Acquire correctly added in live_migrations.rs:14 for the begin() call on &mut PgConnection.

• Comments comply with AGENTS.md: Each comment states the invariant (must use held connection to avoid self-deadlock) in ≤4 lines, does not narrate code or address the reviewer.

Test coverage

Backend-only change to migration-bootstrap connection routing. The migration bootstrap runs on every startup, so the existing startup path exercises it. The codebase has no harness for a single-connection-pool migration scenario, and no new unit test is warranted for executor-routing refactoring of unchanged SQL. The PR's documented manual verification (default multi-connection boot, DATABASE_CONNECTIONS=1 boot with single-connection pg_stat_activity, and the embedded WIN-2130 run) covers the regression surface that motivated the fix. Nothing further needed before merge.

[github-actions]

Codex Review

Good to merge.

No issues found. Checked for bugs, security, public-surface concerns, AGENTS.md compliance, and prior PR discussion.

Test coverage

Backend-only migration-bootstrap change. I did not run automated tests for this read-only review. The PR’s documented manual coverage targets the relevant scenarios: normal multi-connection startup and single-connection migration startup. There is no in-app surface to exercise.