From cd3fe65605d0fb85612179c0147baa2d20fb201d Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 7 May 2026 10:38:48 -0700 Subject: [PATCH 1/3] fix(migrations): Catch missing historical_silo_assignments on MOVE_TO_PENDING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The historical_silo_assignments check in SafeDeleteModel only fired for DeletionAction.DELETE, with an early return for MOVE_TO_PENDING. This missed the dangerous case where the model class is deleted in the same PR as the MOVE_TO_PENDING migration: once the class is gone, the silo router can no longer resolve the table via the live app registry, and without a historical entry allow_migrate returns False so the migration silently no-ops in prod (see PR #114929 / dashboardlastvisited). The check now resolves the table for both deletion actions before the MOVE_TO_PENDING short-circuit. Existing MOVE_TO_PENDING migrations all have their tables in historical_silo_assignments today so replaying them from scratch still passes — verified with --create-db. Co-Authored-By: Claude Opus 4.7 (1M context) --- .agents/skills/generate-migration/SKILL.md | 17 ++++++++- src/sentry/new_migrations/monkey/models.py | 15 ++++++-- .../new_migrations/monkey/test_models.py | 38 +++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/.agents/skills/generate-migration/SKILL.md b/.agents/skills/generate-migration/SKILL.md index 7cde51a098fb3c..50704abe4a28a2 100644 --- a/.agents/skills/generate-migration/SKILL.md +++ b/.agents/skills/generate-migration/SKILL.md @@ -51,11 +51,24 @@ For large tables, set `is_post_deployment = True` on the migration as index crea 3. Replace `RemoveField` with `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` 4. Deploy, then create second migration with `SafeRemoveField(..., deletion_action=DeletionAction.DELETE)` -### Deleting Tables +### Removing a Model (and eventually its table) + +This is a two-phase process. **Phase 1 is where the historical_silo_assignments entry must be added** — not phase 2. + +**Phase 1 — Remove the model class (`MOVE_TO_PENDING`)** + +This is the PR that deletes the model file. The router can no longer resolve the table → silo via the app registry, so without an explicit historical entry the migration is silently skipped on deploy. 1. Remove all code references 2. Replace `DeleteModel` with `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` -3. Deploy, then create second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)` +3. **Add the table to `historical_silo_assignments` in `src/sentry/db/router.py`** (or `getsentry/db/router.py` for getsentry models). Pick the silo the model used — usually `SiloMode.CELL`. +4. Deploy + +> ⚠️ The CI check that fails when `historical_silo_assignments` is missing only fires on `DeletionAction.DELETE` (phase 2). It will **not** catch a missing entry in the `MOVE_TO_PENDING` PR. Adding the entry in phase 1 is a manual discipline — the test won't save you. Skipping it causes the migration to be silently no-op'd in prod (Django sees `allow_migrate` return `None`/False and moves on). See `src/sentry/new_migrations/monkey/models.py` for the check, and the `dashboardlastvisited` incident (PR #114929 / #114930) for what this looks like in practice. + +**Phase 2 — Drop the table (`DELETE`)** + +After phase 1 has deployed, create a second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)`. The historical entry from phase 1 stays in place — the table-drop migration relies on it to resolve the silo. ### Renaming Columns/Tables diff --git a/src/sentry/new_migrations/monkey/models.py b/src/sentry/new_migrations/monkey/models.py index c3548ea438cf4e..84208bfeb0cd1a 100644 --- a/src/sentry/new_migrations/monkey/models.py +++ b/src/sentry/new_migrations/monkey/models.py @@ -33,10 +33,16 @@ def database_forwards( from_state: SentryProjectState, # type: ignore[override] to_state: SentryProjectState, # type: ignore[override] ) -> None: + # The historical_silo_assignments check must run for MOVE_TO_PENDING as + # well as DELETE. MOVE_TO_PENDING typically ships in the same PR that + # deletes the model class, after which the silo router can no longer + # resolve the table via the live app registry. Without a historical + # entry, allow_migrate returns False and the migration silently skips + # in prod (see #114929/dashboardlastvisited for a real incident). if self.deletion_action == DeletionAction.MOVE_TO_PENDING: - return - - model = from_state.get_pending_deletion_model(app_label, self.name) + model = from_state.apps.get_model(app_label, self.name) + else: + model = from_state.get_pending_deletion_model(app_label, self.name) table = model._meta.db_table # Check if we can determine the model's database to detect missing @@ -58,6 +64,9 @@ def database_forwards( f"with the appropriate SiloMode before the deletion migration can run. " ) + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return + if self.allow_migrate_model(schema_editor.connection.alias, model): schema_editor.delete_model(model, is_safe=True) diff --git a/tests/sentry/new_migrations/monkey/test_models.py b/tests/sentry/new_migrations/monkey/test_models.py index 583c4e5fd322b2..4260d2c48d673c 100644 --- a/tests/sentry/new_migrations/monkey/test_models.py +++ b/tests/sentry/new_migrations/monkey/test_models.py @@ -57,3 +57,41 @@ def test_delete_model_without_historical_assignment_fails(self) -> None: assert "Cannot determine database for deleted model" in str(exc_info.value) assert "sentry_fake_deleted_table_not_in_router" in str(exc_info.value) assert "historical_silo_assignments" in str(exc_info.value) + + def test_move_to_pending_without_historical_assignment_fails(self) -> None: + """ + When marking a model for pending deletion that is not in + historical_silo_assignments and cannot be resolved via the live app + registry, SafeDeleteModel should raise a ValueError in test + environments. This catches the case where the model class is deleted + in the same PR as the MOVE_TO_PENDING migration without adding the + historical entry — which would silently skip the migration in prod. + """ + fake_meta = Mock() + fake_meta.db_table = "sentry_fake_pending_table_not_in_router" + fake_meta.app_label = "sentry" + fake_meta.model_name = "fakependingmodel" + + FakePendingModel = Mock() + FakePendingModel._meta = fake_meta + + from_state = Mock(spec=SentryProjectState) + from_state.apps.get_model.return_value = FakePendingModel + to_state = Mock(spec=SentryProjectState) + + operation = SafeDeleteModel( + name="FakePendingModel", deletion_action=DeletionAction.MOVE_TO_PENDING + ) + + with connection.schema_editor() as schema_editor: + with pytest.raises(ValueError) as exc_info: + operation.database_forwards( + "sentry", + cast(SafePostgresDatabaseSchemaEditor, schema_editor), + from_state, + to_state, + ) + + assert "Cannot determine database for deleted model" in str(exc_info.value) + assert "sentry_fake_pending_table_not_in_router" in str(exc_info.value) + assert "historical_silo_assignments" in str(exc_info.value) From db90c7217def668b42de84db764dc821c6ab4c06 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 7 May 2026 10:54:39 -0700 Subject: [PATCH 2/3] docs(skill): Note that auto-generated migration comments should be kept The is_post_deployment doc block in generated migrations is useful context for future migration authors and shouldn't be stripped when editing a generated file. Co-Authored-By: Claude Opus 4.7 (1M context) --- .agents/skills/generate-migration/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.agents/skills/generate-migration/SKILL.md b/.agents/skills/generate-migration/SKILL.md index 50704abe4a28a2..f63278ce07c366 100644 --- a/.agents/skills/generate-migration/SKILL.md +++ b/.agents/skills/generate-migration/SKILL.md @@ -32,6 +32,8 @@ sentry django makemigrations --empty 3. Run `sentry django sqlmigrate ` to verify the SQL 4. Apply the migration locally with `sentry django migrate ` — Sentry's migration framework runs its safety checks on apply, so this catches unsafe ops (missing `is_post_deployment`, unsafe column changes, etc.) before CI does. +When editing a generated migration (e.g. swapping `DeleteModel` for `SafeDeleteModel`), **leave the auto-generated `is_post_deployment` comment block in place**. It documents a non-obvious flag with concrete guidance for future migration authors — useful context, not fluff. Only remove a comment if it's stale or contradicts the code. + ## Guidelines ### Adding Columns From 8207ce99d4d0dafcde5a8d6f777330f91e4053dc Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 7 May 2026 10:57:08 -0700 Subject: [PATCH 3/3] docs(skill): Trim model-removal callout now that the check enforces it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check in this PR fires for MOVE_TO_PENDING, so the skill no longer needs the warning that the test won't catch a missing entry — just the correct procedure. Co-Authored-By: Claude Opus 4.7 (1M context) --- .agents/skills/generate-migration/SKILL.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/.agents/skills/generate-migration/SKILL.md b/.agents/skills/generate-migration/SKILL.md index f63278ce07c366..0a47eb66b46695 100644 --- a/.agents/skills/generate-migration/SKILL.md +++ b/.agents/skills/generate-migration/SKILL.md @@ -55,22 +55,18 @@ For large tables, set `is_post_deployment = True` on the migration as index crea ### Removing a Model (and eventually its table) -This is a two-phase process. **Phase 1 is where the historical_silo_assignments entry must be added** — not phase 2. +Two-phase process — the `historical_silo_assignments` entry must be added in phase 1. **Phase 1 — Remove the model class (`MOVE_TO_PENDING`)** -This is the PR that deletes the model file. The router can no longer resolve the table → silo via the app registry, so without an explicit historical entry the migration is silently skipped on deploy. - 1. Remove all code references 2. Replace `DeleteModel` with `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` -3. **Add the table to `historical_silo_assignments` in `src/sentry/db/router.py`** (or `getsentry/db/router.py` for getsentry models). Pick the silo the model used — usually `SiloMode.CELL`. +3. Add the table to `historical_silo_assignments` in `src/sentry/db/router.py` (or `getsentry/db/router.py` for getsentry models). Pick the silo the model used — usually `SiloMode.CELL`. 4. Deploy -> ⚠️ The CI check that fails when `historical_silo_assignments` is missing only fires on `DeletionAction.DELETE` (phase 2). It will **not** catch a missing entry in the `MOVE_TO_PENDING` PR. Adding the entry in phase 1 is a manual discipline — the test won't save you. Skipping it causes the migration to be silently no-op'd in prod (Django sees `allow_migrate` return `None`/False and moves on). See `src/sentry/new_migrations/monkey/models.py` for the check, and the `dashboardlastvisited` incident (PR #114929 / #114930) for what this looks like in practice. - **Phase 2 — Drop the table (`DELETE`)** -After phase 1 has deployed, create a second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)`. The historical entry from phase 1 stays in place — the table-drop migration relies on it to resolve the silo. +After phase 1 has deployed, create a second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)`. Leave the historical entry in place — the table-drop migration relies on it to resolve the silo. ### Renaming Columns/Tables