fix(DB): check scheduled_task_runs independently in 0003 migration guard#4198
Open
DaoyuanLi2816 wants to merge 1 commit into
Open
Conversation
Revision 0003_scheduled_tasks creates both scheduled_tasks and
scheduled_task_runs, but its re-entry guard only checked the first
table (inspector.has_table("scheduled_tasks")) before returning early.
A DB that atypically already has scheduled_tasks but not its sibling
scheduled_task_runs (a partial restore, manual DB surgery, or legacy
provisioning) silently skips creating scheduled_task_runs -- alembic_version
still advances past 0003 with no error, so a later restart's upgrade head
does not self-heal it.
Mirror the per-object independent-check pattern already used in
0004_run_ownership.py's index creation: check has_table() for both
tables up front and gate each op.create_table() independently, instead
of gating the whole revision on the first table's existence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Revision
0003_scheduled_taskscreates bothscheduled_tasksandscheduled_task_runsin oneupgrade(), but its re-entry idempotency guard only checked the first table:On a DB that atypically already has
scheduled_tasksbut not its siblingscheduled_task_runs, the guard returns early and silently skips creatingscheduled_task_runs.alembic_versionstill advances past0003(and on to head) with no error, so there's no retry path — the gap is permanent once triggered, not transient. The next code path that touches the table (e.g.ScheduledTaskRunRepository.count_active_runs()) then fails withOperationalError: no such table: scheduled_task_runs.Honest scope note: this requires an atypical prior DB state
Reaching this bug requires the asymmetric state described above —
scheduled_taskspresent,scheduled_task_runsabsent. The two ordinary bootstrap branches inpersistence/bootstrap.py(empty-DB and legacy-DB) both provision the pair of tables atomically viaBase.metadata.create_all, so a normal fresh install or normal upgrade never hits this. This is a genuine, verified violation of the code's own documented idempotency guarantee, relevant to restores, manual DB interventions, or partial-provisioning edge cases — not a claim of common/default-path impact.Verification (real alembic chain, no mocks)
Per this repo's migration-authoring conventions (
backend/AGENTS.md's Schema Migrations section), I verified against the real alembic chain rather than mocks:0002_runs_token_usagewith onlyscheduled_tasksmanually pre-created (simulating the atypical prior state).alembic upgrade head(via the sameinit_engine→bootstrap_schemapath Gateway startup uses).scheduled_task_runswas never created, whilealembic_versionadvanced to0004_run_ownershipanyway (no error raised) — andScheduledTaskRunRepository.count_active_runs()raisedOperationalError: no such table: scheduled_task_runs.upgrade headdoes not self-heal it — the revision is already stamped as applied, so alembic never replays it.Fix
Mirrors the per-object independent-check pattern already used in
0004_run_ownership.py's index creation (if "ix_runs_lease" not in existing:, checked separately per index).0003_scheduled_tasks.py'supgrade()now snapshotshas_table()for both tables up front and gates eachop.create_table()independently, instead of gating the whole revision on just the first table's existence:Testing
Added
backend/tests/test_migration_0003_scheduled_task_runs_guard.py, following the same real-alembic-chain shape astest_migration_0004_run_ownership_dedupe.py:test_scheduled_task_runs_created_when_only_scheduled_tasks_preexists— the bug scenario: seeds at0002with onlyscheduled_tasks, runsupgrade head, confirmsscheduled_task_runsnow gets created and a real repository call (count_active_runs()) succeeds instead of raising. Fails before the fix, passes after (verified via patch-revert: reverted the fix, reran, confirmed the same assertion fails with the same error; reapplied, reran, confirmed green).test_neither_scheduled_table_exists_both_created_normally— ordinary path (neither table exists) still creates both.test_both_scheduled_tables_already_exist_upgrade_head_is_idempotent— fully-idempotent path (both already exist, e.g. via the hybrid bootstrap's atomiccreate_all) still no-ops cleanly with notable ... already existserror.Full local run (SQLite, real alembic chain, no mocks) — all passing:
test_migration_0003_scheduled_task_runs_guard.py(new, 3/3)test_persistence_bootstrap.py(31/31)test_persistence_bootstrap_concurrency.pytest_persistence_bootstrap_regression.pytest_persistence_bootstrap_pg_lock.py/test_persistence_bootstrap_sqlite_lock.py/test_persistence_bootstrap_url.pytest_persistence_migrations_env.pytest_migration_0004_run_ownership_dedupe.pytest_migration_user_isolation.pytest_scheduled_task_claims.py/test_scheduled_task_models.py/test_scheduled_task_repository.py/test_scheduled_task_schedules.pyruff check --line-length 240andruff format --check --line-length 240both pass repo-wide.Before submitting
ruff check/ruff format --checkclean (line-length 240)main