SEP-1955: Keep the Celery beat tables out of Alembic autogenerate

[yyyyyyyan]

Summary

The six sqlalchemy_celery_beat schedule tables are declared on the library's own
declarative base, so they never appear in the SQLModel.metadata that all three
Alembic tracks pass as target_metadata. On a database where Celery beat has run,
autogenerate therefore reads them as six tables that used to exist and should be
dropped. make makemigrations does not merely report that — it writes the revision,
folding op.drop_table('celery_periodictask') into whatever migration the developer
was actually authoring.

Two coupled halves:

• A shared include_object autogenerate filter in a new module
  app/core/celery/migrations.py, passed alongside the existing compare_type to
  both context.configure calls in each of app/sep/migrations/env.py,
  app/tasks/migrations/env.py and app/inventory/migrations/env.py. The excluded
  names are derived from the library's own metadata rather than written out, so a
  schedule type the library adds later is covered with no edit here, and a future SEP
  table whose name merely begins with celery_ is not swallowed.

  The filter reads each table object's .name. ModelBase.metadata.tables is keyed
  by the schema-qualified name (celery_schema.celery_periodictask, because the
  models carry __table_args__ = {"schema": "celery_schema"} and the library
  translates it at connect time), while autogenerate reflects the bare name — so a
  filter built from those keys would match nothing and silently exclude nothing.

  It stays deliberately narrow: only a reflected object of type table whose name
  is one of the six is excluded. A genuinely removed SEP table is still reported.

• make migrate drives the library's own bootstrap after the alembic loop, via
  python -m app.core.celery.bootstrap — the same entry point
  sidecar/supervisord.conf already uses. Nothing outside a container previously
  created those tables, so a freshly migrated local database left every SEP entry
  point waiting on tables only beat itself would create.

The tables stay the library's: no revision is added to any of the three tracks.

The module lives beside the Celery code rather than in app/core/db/utils.py (where
compare_type lives) because its only input is the beat library's metadata: a filter
meaningful only to the beat feature does not belong in a generic core utility module,
and app/core/celery/__init__.py carries nothing but its licence header, so the leaf
module is cheap to reach.

Import cost points the same way, though not at the migration path. Putting the filter
in utils.py would give that module a module-scope
from sqlalchemy_celery_beat.session import ModelBase, which pulls in celery and
kombu — measured at +175 modules on top of what app.core.db.utils imports today,
which does not include celery. That cost is per process rather than per importing
file, and it is not saved on the migration path: all three env.py files import
app.core.celery.migrations at module scope, and Alembic loads env.py before any
version file runs, so the side-car's three migration one-shots pay it either way. What
the placement spares is every process that imports app.core.db.utils without running
Alembic — the API and the workers.

Why the integration tests assert what they assert

The three per-track integration tests assert that no beat table is named in the
proposed operations, rather than that alembic check comes back empty. Under
pytest, target_metadata is the process-wide SQLModel.metadata and every track's
models are imported into it, so each track's sweep reports the other two tracks'
tables as missing — alert_backup shows up on the tasks track, for instance. That
pollution is absent when alembic --name <track> runs for real and is unrelated to
the beat tables either way, so the narrower assertion is the one that means
something. It still fails if the filter is removed from any single env.py.

Tested

Automated, run locally:

• The per-track autogenerate tests for all three tracks, the include_object
  unit tests, and the bootstrap suite — green. Removing include_object from
  both context.configure calls in app/inventory/migrations/env.py makes
  tests/app/inventory/migrations/test_beat_tables_ignored.py fail, so the
  assertion is exercising the filter rather than passing vacuously.
• tests/app/sep/migrations/test_alembic_integration.py and
  tests/app/inventory/migrations/test_mandatory_pmm_origin.py, which together
  cover the twelve tests that consume the fixture moved into the new conftest.

Manual, run by hand on this branch:

• make migrate creates all six beat tables.

Manual smoke tests still to run:

• make migrate on a checkout whose databases started empty, then
  python3 -m app.main --start-celery: the API becomes ready and beat starts
  without logging Starting Celery beat without a ready HTTP API. This is the
  only acceptance criterion with no automated coverage — the tests above prove
  the bootstrap creates the tables in the migrate order, but nothing exercises
  the startup path that consumes them.
• With the beat store pointed at a track's own database
  (CELERY__BEAT_DBURI="sqlite:///sep.db" python3 -m app.core.celery.bootstrap),
  confirm alembic --name sep check reports No new upgrade operations detected.
  Before this change the same state reports six remove_table operations.
• alembic --name tasks check and alembic --name inventory check are likewise
  clean after make migrate.
• make makemigrations on a database where beat has run offers no revision
  containing op.drop_table('celery_*').

Bundled fix

inventory_alembic_config moved from tests/app/inventory/migrations/test_mandatory_pmm_origin.py
into a new tests/app/inventory/migrations/conftest.py. The new Inventory-track test
needs the same fixture, and duplicating it is what the scaffolding-duplication check
blocks; promoting it also brings the Inventory track in line with the Tasks track
(tests/app/tasks/migrations/conftest.py) and the SEP track, which already keep this
fixture in a conftest. The body is unchanged, and all twelve tests that consumed it
pass.

BEAT_TABLES and table_names() moved out of
tests/app/core/celery/test_bootstrap.py into tests/app/beat_autogenerate.py for
the same reason: the per-track tests need both, and the alternative was importing a
constant out of a test_*.py module while a shared helper for exactly this material
already existed one directory up. table_names() is unchanged; BEAT_TABLES stays a
written-out literal so that BEAT_TABLE_NAMES == BEAT_TABLES still tests the
derivation instead of restating it.

Known limitations

• The make migrate recipe line is asserted by parsing the target's recipe text; no
  automated test executes the target, so a shell-level wiring fault (the line landing
  under the wrong target, a variable not expanding, a swallowed non-zero exit) would
  pass the suite. A test driving the real target needs isolated databases for all
  three services plus the beat store. The target was run by hand on this branch and
  creates all six tables.
• bootstrap_beat_schema waits for the beat store without a bound, and make migrate
  is a new caller of it (checkmigrations depends on migrate, so both migration
  workflows inherit it). An unreachable beat store therefore hangs the command,
  logging once per second, instead of failing it. No shipped profile reaches that
  state: the development profile uses a local SQLite file that connecting creates,
  and the production_docker and side-car profiles resolve the beat store to the same
  database the alembic loop just connected to. Neither CI workflow is exposed —
  python.yaml and release.yml both invoke make checkmigrations without setting
  CELERY__BEAT_DBURI or FASTAPI_ENV, so both run the development profile against a
  SQLite file. What does reach it is a hand-set CELERY__BEAT_DBURI naming a database
  that is not running, or one that rejects the credentials. _wait_for_store retries
  on OperationalError, which covers a refused connection and an unresolvable host —
  and also a rejected password, since psycopg2 raises OperationalError on
  FATAL: password authentication failed. That last case is the one that can never
  clear, so the retry is unbounded against a failure that will not resolve itself.
  Bounding the wait for this caller alone would mean changing a module this branch
  otherwise only calls, which was weighed and declined.
• make migrate's alembic loop reports only its last iteration's exit status, so
  a failed tasks or inventory upgrade does not fail the target. That is
  pre-existing, but the bootstrap step this branch appends now runs after it, against
  a store that may be only partly migrated. checkmigrations two rules below already
  uses the ret=0; … || ret=1 idiom that closes it.

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• Database migrations generated if models changed (make makemigrations) (N/A for this change — no model changes, and the point of the change is that no revision adopts these tables)
• User-facing changes documented (README, inline help, UI text) (N/A for this change — developer tooling only; deployments already create these tables)
• Configuration changes documented with examples (N/A for this change — no new or changed settings)

[Copilot]

🟡 Changes recommended

The new migration bootstrap can wait forever when the configured beat store remains unavailable.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a metadata-derived Alembic filter preventing library-owned Celery beat tables from being proposed for removal across all migration tracks.

Changes:

• Applies the shared filter to SEP, Tasks, and Inventory Alembic environments.
• Bootstraps beat tables after make migrate.
• Adds shared helpers, fixtures, and integration coverage.

File summaries

File	Description
app/core/celery/migrations.py	Defines the beat-table filter.
app/sep/migrations/env.py	Enables filtering for SEP migrations.
app/tasks/migrations/env.py	Enables filtering for Tasks migrations.
app/inventory/migrations/env.py	Enables filtering for Inventory migrations.
Makefile	Runs beat bootstrap after migrations.
tests/app/beat_autogenerate.py	Adds shared autogenerate helpers.
tests/app/core/celery/test_migrations.py	Tests filter scope and names.
tests/app/core/celery/test_bootstrap.py	Tests Makefile bootstrap wiring.
tests/app/sep/migrations/test_alembic_integration.py	Covers SEP filtering and bootstrap order.
tests/app/tasks/migrations/test_beat_tables_ignored.py	Covers Tasks filtering.
tests/app/inventory/migrations/test_beat_tables_ignored.py	Covers Inventory filtering.
tests/app/inventory/migrations/conftest.py	Centralizes Inventory migration setup.
tests/app/inventory/migrations/test_mandatory_pmm_origin.py	Removes the relocated fixture.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[yyyyyyyan]

Record when FBT001 may be suppressed for a third-party callback signature.

app/core/celery/migrations.py carries # noqa: FBT001 on include_object's reflected parameter. The suppression is forced rather than chosen: Alembic invokes object filters as fn(object_, name, type_, reflected, compare_to) with all five arguments positional, so reflected cannot be made keyword-only the way the boolean-trap rule normally asks for. The same PR makes it keyword-only at both sites where the calling convention is ours to choose (the test helper and the parametrized case), so the one remaining suppression is exactly the case the rule has no answer for.

The repo's ruff conventions already grant this shape for the unused-argument codes — "the signature is imposed from outside" — but the grant is written for ARG001-ARG004 only, so a boolean parameter whose position is fixed by a third-party contract has no sanctioned form. Every future Alembic hook, SQLAlchemy event listener, or framework callback with a boolean positional argument will re-litigate this.

Worth settling once, either as a documented condition alongside the unused-argument grant or as a per-file-ignores entry scoped to the modules that implement third-party callback contracts.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
app/core/celery
migrations.py
app/core/db
utils.py
app/inventory/migrations
env.py					76
app/sep/migrations
env.py					81
app/tasks/migrations
env.py					76
Project Total

This report was generated by python-coverage-comment-action