diff --git a/.agents/skills/generate-migration/SKILL.md b/.agents/skills/generate-migration/SKILL.md index 7cde51a098fb3c..0a47eb66b46695 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 @@ -51,11 +53,20 @@ 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) + +Two-phase process — the `historical_silo_assignments` entry must be added in phase 1. + +**Phase 1 — Remove the model class (`MOVE_TO_PENDING`)** 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 + +**Phase 2 — Drop the table (`DELETE`)** + +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 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)