Skip to content

refactor(db): introduce Alembic migrations (replace boot-time _run_migrations) (#121) - #153

Open
lucastononro wants to merge 3 commits into
mainfrom
fix/121-alembic-migrations
Open

refactor(db): introduce Alembic migrations (replace boot-time _run_migrations) (#121)#153
lucastononro wants to merge 3 commits into
mainfrom
fix/121-alembic-migrations

Conversation

@lucastononro

@lucastononro lucastononro commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Summary

  • Scaffolds backend/alembic/ wired to the app's own async engine (config.settings.database_url) and db.Base.metadata — no second, hand-maintained DB-URL config.
  • Adds a single initial revision, autogenerated against models.py, that reflects today's full schema end-state (i.e. what create_all + _run_migrations produce together, since models.py already carries every column/nullability/FK change _run_migrations used to apply at runtime).
  • db.py:init_db() now runs Alembic on boot instead of create_all + _run_migrations: fresh DB → alembic upgrade head (creates everything + stamps); existing DB that already has today's schema but no alembic_version table → alembic stamp head (marks current, no DDL, no destructive re-run).
  • _run_migrations is left in place, unused, with a docstring explaining why (dead code / rollback reference — see git history to restore the call if Alembic ever needs to be rolled back).

Verified vs deferred

Verified:

  • Schema-equivalence: dumped table/column/type/nullability/FK/index metadata from (a) a DB built via alembic upgrade head from empty and (b) a DB built via the legacy create_all + _run_migrations path, on both SQLite and Postgres. Only difference found: the legacy path creates two redundant duplicate indexes under typo'd names (ix_dataset_versions_source_experiment, ix_registered_models_source_session — missing the _id suffix) that shadow the index SQLAlchemy already creates from index=True in models.py. No column, type, nullability, FK, or PK differences — the new initial revision does not recreate that duplication.
  • Full startup-wiring (the riskier part of this issue) end-to-end on both dialects: (1) fresh empty DB → init_db() creates the schema and stamps alembic_version; (2) pre-existing DB built via the legacy path (no alembic_version) → init_db() stamps head without touching schema; (3) repeated boots on an already-stamped DB → idempotent no-op upgrade.
  • alembic upgrade head / alembic downgrade -1 / --autogenerate all work from the CLI on both SQLite and Postgres.
  • Existing test suite (296 tests) passes unchanged — the test fixtures build schema directly via Base.metadata.create_all/drop_all and don't go through init_db(), so this refactor doesn't touch their setup path.
  • ruff check / ruff format --check clean on all changed files.

Deferred / known limitation: the stamp-vs-upgrade decision uses a heuristic (_pre_alembic_schema_present in db.py) — if a handful of "marker" tables (projects, sessions, experiments, registered_models, dataset_versions) all exist and there's no alembic_version table, it assumes the DB is fully current and stamps. This holds for any DB that's been through a boot of the old _run_migrations path (which ran on every startup), but an older, partially-migrated DB that somehow skipped that isn't handled — it would fall through to upgrade head and surface a loud DDL error rather than silently stamping an incomplete schema. Not expected in practice, but flagging it as an assumption rather than a proven invariant for every possible historical DB state.

Changes

  • backend/requirements.txt: add alembic>=1.13.0,<2.0.0.
  • backend/alembic.ini, backend/alembic/env.py, backend/alembic/script.py.mako: scaffold, DB URL driven from config.settings (async engine cookbook pattern — env.py calls asyncio.run() internally, safe both from the CLI and from db.py's asyncio.to_thread call since a fresh worker thread has no running loop).
  • backend/alembic/versions/0bae8e0f765d_initial_schema.py: the initial revision (autogenerated).
  • backend/db.py: _run_migrations marked dead/documented; new _pre_alembic_schema_present, _alembic_config, _run_alembic_sync helpers; init_db() rewritten to drive Alembic instead of create_all/_run_migrations.
  • backend/AGENTS.md: updated the migrations guidance and "before you ship" checklist to describe the Alembic workflow instead of "add a startup migration in db.py:init_db()".

Test plan

  • Existing: fresh docker compose up still builds the correct schema (verified via init_db() against an empty DB on both SQLite and Postgres)
  • Existing: existing DBs keep working, no destructive re-runs (verified stamp head path against a DB built by the legacy create_all + _run_migrations path)
  • Existing: pytest tests/ -v — 296 passed, 8 skipped (unchanged)
  • New: alembic upgrade head / alembic downgrade -1 / alembic revision --autogenerate all exercised on SQLite and Postgres
  • New: versioned revision committed (alembic/versions/0bae8e0f765d_initial_schema.py)

Closes #121

🤖 Generated with Claude Code

Greptile Summary

This PR replaces the hand-rolled _run_migrations boot-time migration path with a proper Alembic setup, wiring env.py to the app's existing async engine and Base.metadata. The initial revision is autogenerated from the current models.py end-state, and init_db() now runs alembic upgrade head (fresh DB) or alembic stamp head (legacy DB with schema but no version table) via asyncio.to_thread.

  • backend/db.py: _run_migrations marked dead code (retained as rollback reference); three new helpers (_pre_alembic_schema_present, _alembic_config, _run_alembic_sync) drive the stamp-vs-upgrade decision and Alembic execution.
  • backend/alembic/: Full scaffold — env.py follows the official async cookbook pattern (NullPool engine, run_sync wrapper, asyncio.run() in worker thread, disable_existing_loggers=False to preserve app logging); versions/0bae8e0f765d_initial_schema.py captures the complete current schema.
  • backend/tests/test_alembic_migrations.py: Four heuristic unit tests plus two end-to-end integration tests (upgrade head and stamp head) against a real SQLite file, using monkeypatch to redirect settings.database_url.

Confidence Score: 5/5

Safe to merge — the migration path is well-tested on both SQLite and Postgres, known limitations are explicitly documented in code and AGENTS.md, and no schema data is destroyed during the transition.

The stamp-vs-upgrade logic is straightforward and correct, the async/thread boundary is handled exactly as the Alembic cookbook prescribes, and the initial revision was verified schema-equivalent against the legacy path on both dialects. All previously flagged concerns are either addressed in documentation or acknowledged as intentional trade-offs for the single-instance deployment model.

Files Needing Attention: No files require special attention. backend/db.py carries the most complexity but is thoroughly commented and tested.

Important Files Changed

Filename Overview
backend/db.py Core migration refactor: _run_migrations marked dead code, three new Alembic helpers added, and init_db() rewritten to drive alembic upgrade/stamp via asyncio.to_thread; logic is sound and well-documented.
backend/alembic/env.py Correctly follows the Alembic async cookbook: NullPool engine, run_sync wrapper, asyncio.run() safe in worker thread; disable_existing_loggers=False preserves app logging during in-process boot migration.
backend/alembic/versions/0bae8e0f765d_initial_schema.py Autogenerated initial revision covering all 15 tables; FK ordering is correct; render_as_batch enabled for SQLite compatibility.
backend/tests/test_alembic_migrations.py Good coverage of the stamp-vs-upgrade heuristic plus two end-to-end integration tests against a real SQLite file.
backend/alembic.ini Standard Alembic config with placeholder URL overridden at runtime.
backend/AGENTS.md Migration guidance updated to document Alembic workflow and single-instance assumption.
backend/requirements.txt Adds alembic>=1.13.0,<2.0.0 with appropriate version range.
backend/alembic/script.py.mako Standard Alembic migration template, unmodified from the default scaffold.

Reviews (3): Last reviewed commit: "fix(review): batch-mode SQLite migration..." | Re-trigger Greptile

…grations) (#121)

Scaffolds backend/alembic/ wired to the app's async engine (settings.database_url)
and Base.metadata, with a single autogenerated initial revision reflecting today's
full schema. init_db() now runs `alembic upgrade head` on boot (in a worker thread,
since env.py drives its own asyncio.run()); a DB that already has the full schema
but no alembic_version table gets `stamp head` instead, so existing deployments
don't get destructive re-creation attempts. _run_migrations is kept in place but
unused, documented as dead code for rollback reference.

Verified schema-equivalence of `alembic upgrade head` (fresh DB) vs
`create_all + _run_migrations` (legacy path) on both SQLite and Postgres: only
difference found is two redundant duplicate indexes the legacy code created
under typo'd names, no logical schema divergence.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
Comment thread backend/db.py
- Document the single-instance boot-time-migration assumption (Greptile
  P2 on db.py:598): the stamp-vs-upgrade decision and the Alembic run
  are not atomic, so concurrent replicas booting against the same empty
  DB could race on CREATE TABLE. Noted in backend/AGENTS.md ("Database")
  and at the decision site in init_db(), with the recommended mitigation
  (one-off init job) if the app is ever scaled beyond one instance. No
  code-level locking added: docker-compose runs a single backend
  container, and DB-specific advisory locking would risk the proven
  startup behavior for a scenario the deployment doesn't have.

- Add tests/test_alembic_migrations.py: covers the stamp-vs-upgrade
  heuristic (_pre_alembic_schema_present) on empty / full-legacy /
  already-stamped / partial-legacy DBs, plus schema-sanity runs of
  `upgrade head` (fresh DB grows the full schema + alembic_version) and
  `stamp head` (records version without touching schema; next boot takes
  the upgrade path).

- Fix a latent logging bug the new tests surfaced: alembic/env.py's
  fileConfig() defaulted to disable_existing_loggers=True, which — since
  env.py runs in-process at app startup via init_db() — silently
  disabled every app logger created before migrations ran (and broke
  caplog-based tests ordered after an Alembic test). Now passes
  disable_existing_loggers=False.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
@lucastononro

Copy link
Copy Markdown
Owner Author

Greptile review follow-up (commit 7dfaf43)

1 finding assessed — addressed via docs + a targeted comment (no behavior change to the migration path), plus a test set and one latent bug fix the new tests surfaced.

Addressed

  • [P2] db.py:598 — stamp-vs-upgrade decision not atomic across concurrent instances. Valid observation, but not a code defect for this deployment: docker-compose runs a single backend container (no replicas anywhere in the compose files), and the finding itself notes the safest mitigation is an init job, not in-app locking. Fixed as the finding recommends — documented the single-instance assumption in backend/AGENTS.md ("Database") and at the decision site in init_db(), including the migration-as-init-job guidance if the app is ever scaled. Deliberately did not add DB-specific advisory locking: it would complicate the proven startup path (and sqlite has no equivalent) for a scenario the deployment doesn't have.

Test set added (backend/tests/test_alembic_migrations.py, 6 tests)

  • _pre_alembic_schema_present heuristic: empty DB → upgrade; full legacy schema w/o alembic_version → stamp; already-stamped → upgrade; partial legacy schema → upgrade (loud DDL error rather than silently stamping an incomplete schema).
  • Schema sanity: upgrade head on a fresh DB creates the full schema + alembic_version, and the next boot takes the upgrade path; stamp head records the version without creating/altering any tables.
  • The concurrent-boot race itself is intentionally not simulated — single-instance is now a documented assumption.

Bonus fix (surfaced by the new tests)

alembic/env.py called fileConfig() with the default disable_existing_loggers=True. Since env.py runs in-process at app startup (init_db()_run_alembic_sync), it silently disabled every app logger created before migrations ran — killing app logging after boot (and breaking any caplog-based test ordered after an Alembic test; that's how it was caught). Now disable_existing_loggers=False, the canonical setting for in-app Alembic.

Verification

  • Full backend suite: 302 passed, 8 skipped (296 pre-existing + 6 new; py3.12 venv).
  • CLI alembic upgrade head on a fresh SQLite file: 16 tables created (15 model tables + alembic_version), stamped at 0bae8e0f765d.
  • ruff check + ruff format --check clean on all touched files.

- render_as_batch=True in both offline and online alembic contexts so
  future ALTER-style migrations work on SQLite (Greptile 4/5 note);
  no-op on Postgres.
- ruff format (0.15.22) the generated initial-schema revision file,
  unblocking Backend Lint.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lucastononro added a commit that referenced this pull request Jul 29, 2026
)

Stack wave 5, PR 4/4. Greptile P2 stamp-vs-upgrade atomicity addressed
on the PR branch by author fixup 7dfaf43 (documented single-instance
assumption + regression tests + fileConfig logger fix). Outstanding
4/5 render_as_batch note fixed in 7fe06d7, plus ruff format of the
initial-schema revision unblocking Backend Lint.
lucastononro added a commit that referenced this pull request Jul 29, 2026
Also includes the post-merge ruff UP007 autofix on #153's initial
alembic revision (staging's pinned ruff config from #137 enables UP
rules the PR branch predates).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace hand-rolled boot-time migrations with Alembic

1 participant