Raised from CodeRabbit's review of #574. Real for scaled deployments, not for the shipped default — kept out of the release because the fix is a deployment-architecture decision, not a patch.
The gap
run_migrations() in app/infrastructure/database/migrations.py wraps command.stamp() and command.upgrade() in sync_engine.begin(). A transaction serialises statements on one connection; it does not serialise Alembic runs across processes. app/main.py calls it from the lifespan handler, so every instance runs it on startup:
await asyncio.to_thread(run_migrations)
Two instances starting against one database can both observe pending revisions and both attempt the same DDL. Depending on the backend that surfaces as a failed startup, a duplicate-object error, or one instance sitting in a broken migration transaction.
Scope — smaller than it first looks
The shipped image runs a single worker:
ENTRYPOINT ["uv", "run", "--no-sync", "gunicorn", ..., "--workers", "1", ...]
So the default configuration does not race. This bites when someone raises the worker count, or runs more than one replica against a shared database — both reasonable things to do, and neither currently documented as unsafe.
Suggested direction
Either:
- Take a database-scoped lock around stamp/upgrade — a Postgres advisory lock is the usual shape. Losers wait, observe no pending revisions, and continue. Needs a backend-specific path, since SQLite has no equivalent.
- Move migrations out of application startup into an explicit deployment step (init container, release job,
task db:upgrade in the pipeline). Cleaner separation, but it gives up the "just boot it" property that made startup migration attractive.
Worth deciding deliberately rather than defaulting. Whichever way it goes, the constraint should be written down in docs/database/migrations.md, which currently does not mention concurrency at all.
Interim
Until then, deployments running multiple replicas or workers against one database should run migrations once before rolling instances.
Original review comment: #574 (comment)
Raised from CodeRabbit's review of #574. Real for scaled deployments, not for the shipped default — kept out of the release because the fix is a deployment-architecture decision, not a patch.
The gap
run_migrations()inapp/infrastructure/database/migrations.pywrapscommand.stamp()andcommand.upgrade()insync_engine.begin(). A transaction serialises statements on one connection; it does not serialise Alembic runs across processes.app/main.pycalls it from the lifespan handler, so every instance runs it on startup:Two instances starting against one database can both observe pending revisions and both attempt the same DDL. Depending on the backend that surfaces as a failed startup, a duplicate-object error, or one instance sitting in a broken migration transaction.
Scope — smaller than it first looks
The shipped image runs a single worker:
So the default configuration does not race. This bites when someone raises the worker count, or runs more than one replica against a shared database — both reasonable things to do, and neither currently documented as unsafe.
Suggested direction
Either:
task db:upgradein the pipeline). Cleaner separation, but it gives up the "just boot it" property that made startup migration attractive.Worth deciding deliberately rather than defaulting. Whichever way it goes, the constraint should be written down in
docs/database/migrations.md, which currently does not mention concurrency at all.Interim
Until then, deployments running multiple replicas or workers against one database should run migrations once before rolling instances.
Original review comment: #574 (comment)