fix(migrations): Catch missing historical_silo_assignments on MOVE_TO_PENDING#115087
Merged
fix(migrations): Catch missing historical_silo_assignments on MOVE_TO_PENDING#115087
Conversation
…_PENDING 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) <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Backend
markstory
approved these changes
May 7, 2026
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) <noreply@anthropic.com>
Member
Author
|
Test PR here: https://github.com/getsentry/sentry/pull/115091/changes |
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) <noreply@anthropic.com>
Member
Author
Ok, this now fails if we don't add the historical assignments, should be good to go |
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
2 participants
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.
The
historical_silo_assignmentscheck inSafeDeleteModelonly fired forDeletionAction.DELETE, with an early return forMOVE_TO_PENDING. That missed the dangerous case where the model class is deleted in the same PR as theMOVE_TO_PENDINGmigration: once the class is gone, the silo router can no longer resolve the table via the live app registry, and without a historical entryallow_migratereturns False so the migration silently no-ops in prod.This is what bit us in #114929 (
dashboardlastvisited). This caused an error in production where we couldn't delete related rows fromOrganizationMember, and this was only fixed when we dropped the table fully in #114930.This pr makes sure that the test fails if we move a model to pending and don't add the historical routing.
Also update the skill to guide llms to always add this historical entry.