Skip to content

fix(BA-6025): add ON DELETE RESTRICT FK on images.registry_id#11643

Open
fregataa wants to merge 4 commits into
mainfrom
fix/BA-6025-images-registry-fk-restrict
Open

fix(BA-6025): add ON DELETE RESTRICT FK on images.registry_id#11643
fregataa wants to merge 4 commits into
mainfrom
fix/BA-6025-images-registry-fk-restrict

Conversation

@fregataa
Copy link
Copy Markdown
Member

Summary

  • Add a real DB-level foreign key from images.registry_id to container_registries.id with ON DELETE RESTRICT. Previously only an ORM-level foreign() join existed, so deleting a registry left dangling references that the sokovan scheduler later surfaced as a misleading "Image not found in database" error during session launch.
  • ON DELETE RESTRICT (over CASCADE) preserves audit traceability on kernels and deployment_revisions (whose image FKs use SET NULL) and forces operators to clear or purge images via the existing clear_images / admin_purge flows before removing a registry.
  • The migration hard-deletes any pre-existing dangling image rows before creating the constraint, since the column is NOT NULL and a sentinel reassignment would mask the original deletion intent.
  • Affected test fixtures using with_tables now include ContainerRegistryRow, and the two test_image fixtures in deployment tests seed a real container_registry row instead of fabricating a random registry_id.

Test plan

  • pants test for all with_tables-based image tests (~46 files) passes locally.
  • alembic upgrade head and round-trip downgrade -1 → upgrade head succeed on local DB.
  • CI green.
  • Manual: attempt to delete a registry that still has images via REST v2 — expect HTTP 409 ForeignKeyViolationError. (Follow-up ticket recommended to convert this into a domain-specific 400 with a clearer message.)

Resolves BA-6025

fregataa and others added 3 commits May 17, 2026 19:02
The column previously had no DB-level FK to container_registries.id,
which allowed dangling references after a registry was deleted. The
sokovan scheduler later surfaced this as a misleading "Image not
found in database" error during session launch.

Add a real FK with ON DELETE RESTRICT so registry deletion is blocked
while images still reference it; operators must clear or purge those
images first. RESTRICT is preferred over CASCADE to preserve audit
trails on kernels / deployment_revisions (whose image FKs use
SET NULL).

The migration hard-deletes any pre-existing dangling rows before
creating the constraint, since the column is NOT NULL and a sentinel
reassignment would mask the original deletion intent.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…using tests

with_tables creates only the listed tables, and the new FK on
images.registry_id requires container_registries to exist when the
images table is created. Add ContainerRegistryRow alongside ImageRow
in every affected test fixture.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fixtures

The two test fixtures were instantiating ImageRow with a random
registry_id that no longer satisfies the new FK constraint. Seed a
real ContainerRegistryRow first and reuse its id.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@fregataa fregataa requested a review from a team as a code owner May 17, 2026 10:44
Copilot AI review requested due to automatic review settings May 17, 2026 10:44
@github-actions github-actions Bot added size:L 100~500 LoC comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated labels May 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a real database-level foreign key from images.registry_id to container_registries.id with ON DELETE RESTRICT, preventing dangling image registry references and aligning test fixtures with the new constraint.

Changes:

  • Adds the ImageRow.registry_id FK and an Alembic migration to clean dangling rows before constraint creation.
  • Updates many with_tables fixtures to include ContainerRegistryRow before ImageRow.
  • Seeds real container registry rows in deployment-related image fixtures.

Reviewed changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/ai/backend/manager/models/image/row.py Adds FK constraint on images.registry_id.
src/ai/backend/manager/models/alembic/versions/045b0e0e4384_add_fk_from_images_registry_id_to_.py Adds migration to delete dangling images and create/drop the FK.
tests/unit/manager/test_query_userinfo.py Adds registry table to image-related fixture tables.
tests/unit/manager/repositories/vfolder/test_vfolder_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/user/test_user_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/user_resource_policy/test_user_resource_policy_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/test_utils.py Adds registry table to shared repository fixture setup.
tests/unit/manager/repositories/scheduling_history/test_scheduling_history_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/scheduler/test_termination.py Adds registry table to fixture setup.
tests/unit/manager/repositories/scheduler/test_session_network_propagation.py Adds registry table to fixture setup.
tests/unit/manager/repositories/scaling_group/test_scaling_group_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/resource_usage_history/test_usage_bucket_entries.py Adds registry table to fixture setup.
tests/unit/manager/repositories/resource_usage_history/test_resource_usage_history_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/resource_preset/test_resource_preset_cache_invalidation.py Adds registry table to fixture setup.
tests/unit/manager/repositories/resource_preset/test_check_presets.py Adds registry table to fixture setup.
tests/unit/manager/repositories/project_resource_policy/test_project_resource_policy.py Adds registry table to fixture setup.
tests/unit/manager/repositories/permission_controller/test_user_scope_sync.py Adds registry table to fixture setup.
tests/unit/manager/repositories/permission_controller/test_search_scopes.py Adds registry table to fixture setup.
tests/unit/manager/repositories/permission_controller/test_role_invitation.py Adds registry table to fixture setup.
tests/unit/manager/repositories/permission_controller/test_role_assignment.py Adds registry table to fixture setup.
tests/unit/manager/repositories/notification/test_notification_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/notification/test_notification_options.py Adds registry table to fixture setup.
tests/unit/manager/repositories/model_card/test_model_card_search_in_project.py Adds registry table to fixture setup.
tests/unit/manager/repositories/model_card/test_model_card_scan.py Adds registry table to fixture setup.
tests/unit/manager/repositories/model_card/test_model_card_delete.py Adds registry table to fixture setup.
tests/unit/manager/repositories/model_card/test_model_card_creator.py Adds registry table to fixture setup.
tests/unit/manager/repositories/keypair_resource_policy/test_keypair_resource_policy.py Adds registry table to fixture setup.
tests/unit/manager/repositories/group/test_unassign_users_from_project.py Adds registry table to fixture setup.
tests/unit/manager/repositories/group/test_group_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/group/test_group_db_source.py Adds registry table to fixture setup.
tests/unit/manager/repositories/group/test_assign_users_to_project.py Adds registry table to fixture setup.
tests/unit/manager/repositories/fair_share/test_fair_share_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/error_log/test_error_log_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/domain/test_domain_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/auth/test_auth_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/artifact/test_artifact_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/artifact_revision/test_artifact_revision_repository.py Adds registry table to fixture setup.
tests/unit/manager/repositories/app_config/test_app_config.py Adds registry table to fixture setup.
tests/unit/manager/repositories/agent/test_repository.py Adds registry table to fixture setup.
tests/unit/manager/models/user/test_conditions.py Adds registry table to fixture setup.
tests/unit/manager/models/test_vfolder_quota_scope.py Adds registry table to fixture setup.
tests/unit/manager/models/test_vfolder_prepare_mounts.py Adds registry table to fixture setup.
tests/unit/manager/models/test_session.py Adds registry table to fixture setup.
tests/unit/manager/models/test_deployment_revision.py Adds registry table and seeds a matching registry for test images.
tests/unit/manager/models/test_deployment_auto_scaling_policy.py Adds registry table and seeds a matching registry for test images.
tests/unit/manager/models/scaling_group/test_conditions.py Adds registry table to fixture setup.
tests/unit/manager/models/group/test_conditions.py Adds registry table to fixture setup.
tests/unit/manager/models/domain/test_conditions.py Adds registry table to fixture setup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +41 to +43
op.execute(
sa.text("DELETE FROM images WHERE registry_id NOT IN (SELECT id FROM container_registries)")
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to reflect Copilot's review

Comment on lines +337 to +343
registry_id: Mapped[UUID] = mapped_column(
"registry_id",
GUID,
sa.ForeignKey("container_registries.id", ondelete="RESTRICT"),
nullable=False,
index=True,
)
@jopemachine
Copy link
Copy Markdown
Member

Could you check what error is raised when the container registry delete mutation fails due to existing image rows? Also, it would be good to add a note to the delete mutation description indicating that this error can occur when image rows still exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants