Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions .agents/skills/generate-migration/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
15 changes: 12 additions & 3 deletions src/sentry/new_migrations/monkey/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down
38 changes: 38 additions & 0 deletions tests/sentry/new_migrations/monkey/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Loading