fix(backend): serialize concurrent migrations with a Postgres advisory lock - #2574
fix(backend): serialize concurrent migrations with a Postgres advisory lock#2574feiiiiii5 wants to merge 2 commits into
Conversation
…y lock Every backend replica runs migrate.sh at container start, so a rollout with multiple instances fires 'alembic upgrade head' against the same database simultaneously and only survives on row-level lock timing plus idempotent migration guards. Acquire a session-scoped pg_advisory_lock in alembic's env.py so exactly one process performs the upgrade while the others wait, then find the database already at head. The lock lives on a dedicated autocommit connection for the whole upgrade, releases in finally, auto-releases if a replica crashes mid-upgrade (session end), and is skipped entirely for non-PostgreSQL databases. ALEMBIC_LOCK_TIMEOUT (default 600s) bounds the wait so a stuck holder fails loudly instead of hanging replicas forever. Fixes rhesis-ai#2545 Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
27c2d64 to
a1d74bc
Compare
There was a problem hiding this comment.
[Improvement] The non-Postgres skip isn’t fully safe: _acquire_migration_advisory_lock() applies execution_options(isolation_level="AUTOCOMMIT") before checking dialect.name, which may fail on SQLite/other dialects. Please move the dialect guard ahead of the autocommit connection/options so non-Postgres setups truly bypass the locking path.
| keeps the lock beyond ``ALEMBIC_LOCK_TIMEOUT`` seconds so orchestrators | ||
| can retry instead of racing through an upgrade. | ||
| """ | ||
| lock_connection = connectable.connect().execution_options(isolation_level="AUTOCOMMIT") |
There was a problem hiding this comment.
_acquire_migration_advisory_lock() calls connectable.connect().execution_options(isolation_level="AUTOCOMMIT") before the dialect guard. On non-Postgres (e.g. SQLite), that AUTOCOMMIT isolation level can raise and would contradict the “skipped entirely” promise in the PR description.
Fix: check
connectable.dialect.namefirst, and only then open the lock connection + apply the Postgres-specificAUTOCOMMIToption.
There was a problem hiding this comment.
Looks fixed in c367047: dialect is checked at the engine level before opening the lock connection / applying AUTOCOMMIT, so non-Postgres setups should now skip locking cleanly.
There was a problem hiding this comment.
Fixed in c367047 — the dialect guard now runs at engine level (connectable.dialect.name) before the lock connection is opened, so non-PostgreSQL setups never touch execution_options(...) at all. Re-ran the full verification against Postgres 16 (fresh upgrade to head, contention wait/acquire, timeout path) after the change.
There was a problem hiding this comment.
Yep, that addresses my concern — checking connectable.dialect.name before opening the connection / applying AUTOCOMMIT means non-Postgres setups truly skip the lock path. Thanks for the quick fix + re-test.
There was a problem hiding this comment.
Improvement: The current implementation still sets isolation_level="AUTOCOMMIT" before the Postgres dialect check in _acquire_migration_advisory_lock(). SQLAlchemy may reject that option on some non-Postgres dialects, which would break the “skip entirely for non-PostgreSQL” guarantee.
Fix: check
connectable.dialect.name(engine-level) before opening/configuring the lock connection, or guard theexecution_options(...)call so non-Postgres never sees it.
Move the PostgreSQL dialect check to engine level ahead of execution_options(isolation_level=AUTOCOMMIT): some dialects reject that option, which would break the guarantee that non-PostgreSQL setups bypass the locking path entirely. Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
Fixes #2545
Problem
Every backend replica runs
migrate.shat container start, so rolling out a new version with multiple Cloud Run Job instances firesalembic upgrade headagainst the same database simultaneously. As reported in #2545, during the v0.13.0 rollout three backend pods ran the 582-line org migration concurrently and only avoided corruption because Postgres row-level locks serialized the writes and the migration guards happened to be idempotent.Solution
Acquire a session-scoped
pg_advisory_lockin Alembic'senv.py(online mode) so exactly one process performs the upgrade while any other replica waits, then finds the database already at head:pg_try_advisory_lockpolling withALEMBIC_LOCK_TIMEOUT(default 600s); on timeout it raises a clear RuntimeError instead of hanging replicas forever or racing through the upgrade.env.py, manualalembic upgrade, CI jobs, andmigrate.share all serialized — not just the shell script path.migrate.shgot a short comment pointing at the mechanism for discoverability.Lock key is the fixed constant
crc32(b"rhesis-backend-migrations") = 1260693429.Testing
Verified against Postgres 16 (local Docker, full
alembic upgrade headfrom an empty DB to head with the change active):pg_locks)ALEMBIC_LOCK_TIMEOUT=6ALEMBIC_LOCK_TIMEOUT=60)Also:
ruff check+ruff format --checkclean,bash -n migrate.shclean.Note
LLM-assisted contribution, per repo guidelines.