From 1bfbcaa1cbec82cb6b5939f536b2a31075680cce Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 18 Nov 2025 16:48:42 -0800 Subject: [PATCH 1/5] fix(migrations): Fail migrations that delete models if no historical_silo_assignment is found We have been silently failing the final deletion of tables for a while, because we need to add them to `historical_silo_assignments` for it to work correctly. This pr makes sure that we fail in CI if we aren't able to find an entry there. --- src/sentry/db/router.py | 7 ++ src/sentry/new_migrations/monkey/models.py | 23 ++++++ .../new_migrations/monkey/test_models.py | 81 +++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 tests/sentry/new_migrations/monkey/test_models.py diff --git a/src/sentry/db/router.py b/src/sentry/db/router.py index d86bfab3b93230..29f8d1f16aff59 100644 --- a/src/sentry/db/router.py +++ b/src/sentry/db/router.py @@ -68,8 +68,13 @@ class SiloRouter: historical_silo_assignments = { "authidentity_duplicate": SiloMode.CONTROL, "authprovider_duplicate": SiloMode.CONTROL, + "feedback_feedback": SiloMode.REGION, + "releases_commit": SiloMode.REGION, + "releases_commitfilechange": SiloMode.REGION, "sentry_actor": SiloMode.REGION, "sentry_alertruleactivations": SiloMode.REGION, + "sentry_dashboardwidgetsnapshot": SiloMode.REGION, + "sentry_datasecrecywaiver": SiloMode.REGION, "sentry_incidentseen": SiloMode.REGION, "sentry_incidentsubscription": SiloMode.REGION, "sentry_monitorlocation": SiloMode.REGION, @@ -78,6 +83,8 @@ class SiloRouter: "sentry_projectavatar": SiloMode.REGION, "sentry_scheduledjob": SiloMode.CONTROL, "sentry_teamavatar": SiloMode.REGION, + "uptime_projectuptimesubscription": SiloMode.REGION, + "workflow_engine_actiongroupstatus": SiloMode.REGION, "workflow_engine_workflowaction": SiloMode.REGION, } """ diff --git a/src/sentry/new_migrations/monkey/models.py b/src/sentry/new_migrations/monkey/models.py index e744e11b356e70..c3548ea438cf4e 100644 --- a/src/sentry/new_migrations/monkey/models.py +++ b/src/sentry/new_migrations/monkey/models.py @@ -1,8 +1,10 @@ +from django.db import router from django.db.migrations import DeleteModel from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState +from sentry.utils.env import in_test_environment class SafeDeleteModel(DeleteModel): @@ -35,6 +37,27 @@ def database_forwards( return 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 + # historical_silo_assignments entries + resolved_db = None + for db_router in router.routers: + if hasattr(db_router, "_db_for_table"): + resolved_db = db_router._db_for_table(table, app_label) + if resolved_db is not None: + break + + # If we can't determine the database and we're in CI/tests, fail loudly + # This indicates the table is missing from historical_silo_assignments + if resolved_db is None and in_test_environment(): + raise ValueError( + f"Cannot determine database for deleted model {app_label}.{self.name} " + f"(table: {table}). This table must be added to historical_silo_assignments " + f"in src/sentry/db/router.py (or getsentry/db/router.py for getsentry models) " + f"with the appropriate SiloMode before the deletion migration can run. " + ) + 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 new file mode 100644 index 00000000000000..a810a58d880f7b --- /dev/null +++ b/tests/sentry/new_migrations/monkey/test_models.py @@ -0,0 +1,81 @@ +""" +Tests for SafeDeleteModel validation that ensures deleted models are added to +historical_silo_assignments. +""" + +import pytest +from django.db import connection, models + +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState +from sentry.testutils.cases import TestCase + + +class SafeDeleteModelTest(TestCase): + """ + Tests that SafeDeleteModel fails loudly when a deleted model is not in + historical_silo_assignments. + """ + + def test_delete_model_without_historical_assignment_fails(self): + """ + When deleting a model that is not in historical_silo_assignments and + cannot be found, SafeDeleteModel should raise a ValueError in test + environments. + """ + # Create a mock model that mimics a deleted model not in the Django registry + # This simulates what happens when a model class has been removed but + # migrations still reference it + from unittest.mock import Mock + + fake_meta = Mock() + fake_meta.db_table = "sentry_fake_deleted_table_not_in_router" + fake_meta.app_label = "sentry" + fake_meta.model_name = "fakedeletedmodel" + + FakeDeletedModel = Mock() + FakeDeletedModel._meta = fake_meta + + # Create project states + from_state = SentryProjectState() + to_state = SentryProjectState() + + # Manually add the model to pending deletion in the from_state + # This simulates what happens when a model was marked for pending deletion + # and is now being deleted + from_state.pending_deletion_models[("sentry", "fakedeletedmodel")] = FakeDeletedModel + + # Create the SafeDeleteModel operation + operation = SafeDeleteModel(name="FakeDeletedModel", deletion_action=DeletionAction.DELETE) + + # Get schema editor + with connection.schema_editor() as schema_editor: + # This should raise ValueError because the table is not in historical_silo_assignments + with pytest.raises(ValueError) as exc_info: + operation.database_forwards("sentry", schema_editor, from_state, to_state) + + 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_delete_model_with_move_to_pending_skips_validation(self): + """ + MOVE_TO_PENDING operations should not trigger the validation check + since the model class still exists at that point. + """ + + class FakeModel(models.Model): + class Meta: + app_label = "sentry" + db_table = "sentry_fake_table" + + from_state = SentryProjectState() + to_state = SentryProjectState() + + operation = SafeDeleteModel( + name="FakeModel", deletion_action=DeletionAction.MOVE_TO_PENDING + ) + + with connection.schema_editor() as schema_editor: + # Should not raise - MOVE_TO_PENDING exits early + operation.database_forwards("sentry", schema_editor, from_state, to_state) From 3d2ea928d370b6062d3b1bb7cd7ceba0adb5ae42 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 20 Nov 2025 10:23:35 -0800 Subject: [PATCH 2/5] mypy --- .../new_migrations/monkey/test_models.py | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/sentry/new_migrations/monkey/test_models.py b/tests/sentry/new_migrations/monkey/test_models.py index a810a58d880f7b..1cc9091075894e 100644 --- a/tests/sentry/new_migrations/monkey/test_models.py +++ b/tests/sentry/new_migrations/monkey/test_models.py @@ -3,9 +3,12 @@ historical_silo_assignments. """ +from typing import cast + import pytest from django.db import connection, models +from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor from sentry.new_migrations.monkey.models import SafeDeleteModel from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState from sentry.testutils.cases import TestCase @@ -17,7 +20,7 @@ class SafeDeleteModelTest(TestCase): historical_silo_assignments. """ - def test_delete_model_without_historical_assignment_fails(self): + def test_delete_model_without_historical_assignment_fails(self) -> None: """ When deleting a model that is not in historical_silo_assignments and cannot be found, SafeDeleteModel should raise a ValueError in test @@ -52,13 +55,18 @@ def test_delete_model_without_historical_assignment_fails(self): with connection.schema_editor() as schema_editor: # This should raise ValueError because the table is not in historical_silo_assignments with pytest.raises(ValueError) as exc_info: - operation.database_forwards("sentry", schema_editor, from_state, to_state) + 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_deleted_table_not_in_router" in str(exc_info.value) assert "historical_silo_assignments" in str(exc_info.value) - def test_delete_model_with_move_to_pending_skips_validation(self): + def test_delete_model_with_move_to_pending_skips_validation(self) -> None: """ MOVE_TO_PENDING operations should not trigger the validation check since the model class still exists at that point. @@ -78,4 +86,9 @@ class Meta: with connection.schema_editor() as schema_editor: # Should not raise - MOVE_TO_PENDING exits early - operation.database_forwards("sentry", schema_editor, from_state, to_state) + operation.database_forwards( + "sentry", + cast(SafePostgresDatabaseSchemaEditor, schema_editor), + from_state, + to_state, + ) From 2156efce97cc7edcd3f2efd3e8b6cad910872ca8 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 20 Nov 2025 14:11:05 -0800 Subject: [PATCH 3/5] try out fixing flaky test --- src/sentry/testutils/helpers/backups.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/sentry/testutils/helpers/backups.py b/src/sentry/testutils/helpers/backups.py index 9e3993638a436b..27429cc5689417 100644 --- a/src/sentry/testutils/helpers/backups.py +++ b/src/sentry/testutils/helpers/backups.py @@ -269,8 +269,12 @@ def export_to_encrypted_tarball( def is_control_model(model): - meta = model._meta - return not hasattr(meta, "silo_limit") or SiloMode.CONTROL in meta.silo_limit.modes + # Check where the router actually sends this model instead of relying on metadata. + # Models without silo_limit (like Django's built-in models) may route to different + # databases depending on the router configuration. + using = router.db_for_write(model) + # In test environments, "control" database = control silo, "default" = region silo + return using == "control" def clear_model(model, *, reset_pks: bool): From 18627451cc0f283072b1dcc78d764d32b252cc16 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 21 Nov 2025 10:40:03 -0800 Subject: [PATCH 4/5] fix tests --- .../new_migrations/monkey/test_models.py | 39 +------------------ 1 file changed, 2 insertions(+), 37 deletions(-) diff --git a/tests/sentry/new_migrations/monkey/test_models.py b/tests/sentry/new_migrations/monkey/test_models.py index 1cc9091075894e..583c4e5fd322b2 100644 --- a/tests/sentry/new_migrations/monkey/test_models.py +++ b/tests/sentry/new_migrations/monkey/test_models.py @@ -4,9 +4,10 @@ """ from typing import cast +from unittest.mock import Mock import pytest -from django.db import connection, models +from django.db import connection from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor from sentry.new_migrations.monkey.models import SafeDeleteModel @@ -26,11 +27,6 @@ def test_delete_model_without_historical_assignment_fails(self) -> None: cannot be found, SafeDeleteModel should raise a ValueError in test environments. """ - # Create a mock model that mimics a deleted model not in the Django registry - # This simulates what happens when a model class has been removed but - # migrations still reference it - from unittest.mock import Mock - fake_meta = Mock() fake_meta.db_table = "sentry_fake_deleted_table_not_in_router" fake_meta.app_label = "sentry" @@ -39,7 +35,6 @@ def test_delete_model_without_historical_assignment_fails(self) -> None: FakeDeletedModel = Mock() FakeDeletedModel._meta = fake_meta - # Create project states from_state = SentryProjectState() to_state = SentryProjectState() @@ -48,12 +43,9 @@ def test_delete_model_without_historical_assignment_fails(self) -> None: # and is now being deleted from_state.pending_deletion_models[("sentry", "fakedeletedmodel")] = FakeDeletedModel - # Create the SafeDeleteModel operation operation = SafeDeleteModel(name="FakeDeletedModel", deletion_action=DeletionAction.DELETE) - # Get schema editor with connection.schema_editor() as schema_editor: - # This should raise ValueError because the table is not in historical_silo_assignments with pytest.raises(ValueError) as exc_info: operation.database_forwards( "sentry", @@ -65,30 +57,3 @@ 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_delete_model_with_move_to_pending_skips_validation(self) -> None: - """ - MOVE_TO_PENDING operations should not trigger the validation check - since the model class still exists at that point. - """ - - class FakeModel(models.Model): - class Meta: - app_label = "sentry" - db_table = "sentry_fake_table" - - from_state = SentryProjectState() - to_state = SentryProjectState() - - operation = SafeDeleteModel( - name="FakeModel", deletion_action=DeletionAction.MOVE_TO_PENDING - ) - - with connection.schema_editor() as schema_editor: - # Should not raise - MOVE_TO_PENDING exits early - operation.database_forwards( - "sentry", - cast(SafePostgresDatabaseSchemaEditor, schema_editor), - from_state, - to_state, - ) From 4f46a62cbbc691fbfb94e9c509026d18aae90884 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 21 Nov 2025 10:47:08 -0800 Subject: [PATCH 5/5] revert dumb change --- src/sentry/testutils/helpers/backups.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/sentry/testutils/helpers/backups.py b/src/sentry/testutils/helpers/backups.py index 27429cc5689417..9e3993638a436b 100644 --- a/src/sentry/testutils/helpers/backups.py +++ b/src/sentry/testutils/helpers/backups.py @@ -269,12 +269,8 @@ def export_to_encrypted_tarball( def is_control_model(model): - # Check where the router actually sends this model instead of relying on metadata. - # Models without silo_limit (like Django's built-in models) may route to different - # databases depending on the router configuration. - using = router.db_for_write(model) - # In test environments, "control" database = control silo, "default" = region silo - return using == "control" + meta = model._meta + return not hasattr(meta, "silo_limit") or SiloMode.CONTROL in meta.silo_limit.modes def clear_model(model, *, reset_pks: bool):