Fix orphaned uploads from vulnerability export FK cleanup

Ryan Wellsrequested to merge
fix/vulnerability-export-loose-fk-upload-cleanup into master

What does this MR do and why?

This MR fixes a gap where LFK cleanup may miss deleting Upload records for Vulnerabilities::Export jobs, specifically when deleting an org, namespace, project or user. The current behaviour of LooseForeignKeys::CleanerService just issues a raw bulk DELETE, which doesn't cascade down to the associated Upload record. The uploads partitions have no sharding-key foreign key back to organizations, so deleting an organization does not cascade those upload rows away either.

This follows the precedent already set for file-backed tables: p_ci_job_artifacts opts out of the generic bulk DELETE with a cleaner_class key in config/gitlab_loose_foreign_keys.yml, handled by Ci::JobArtifacts::LooseForeignKeyCleanerService.

This MR:

  • Adds Vulnerabilities::Exports::LooseForeignKeyCleanerService, which routes the delete through Vulnerabilities::Exports::BatchDestroyService so the exports' uploads are removed rather than orphaned.
  • Adds Vulnerabilities::Exports::PartsLooseForeignKeyCleanerService for the vulnerability_export_parts entry. It is needed separately because the YAML is ordered alphabetically, so vulnerability_export_parts is processed before vulnerability_exports when an organization is deleted.
  • Wires both into the five affected loose foreign key definitions via cleaner_class.
  • Changes Gitlab::Database::LooseForeignKeys.build_definition to resolve cleaner_class through a new private resolve_cleaner_class method. On FOSS, where ee/ is removed, it uses safe_constantize, and an unresolvable EE-only cleaner falls back to the generic cleaner, which is the current FOSS behaviour anyway. On EE it still uses constantize, so a genuine load failure raises rather than silently reverting to the bulk delete. ALLOWED_CLEANER_CLASSES still rejects unknown names.

Design notes

  • Neither cleaner opens a transaction around its delete, because the cleanup also writes to uploads, which lives in a different database. That differs from the job artifacts cleaner, which can hold its rows with FOR UPDATE SKIP LOCKED inside a transaction. Since SKIP LOCKED outside a transaction releases the locks as soon as the SELECT ends, these cleaners do not use it at all; concurrent runs are kept apart upstream, where LooseForeignKeys::ProcessDeletedRecordsService claims the parent records.
  • Like the job artifacts cleaner, these subclasses always delete and do not read loose_foreign_key_definition.options[:conditions] or branch on on_delete. No conditions are configured for these tables, but adding one, or switching an entry to async_nullify while leaving cleaner_class set, would not behave as expected.

Depends on

Vulnerabilities::Exports::BatchDestroyService only deletes the exports' parts and their uploads once !254286 (merged) merges. Until then this MR fixes the orphaned uploads owned by the exports themselves, and the parts' uploads are still orphaned by the cascade. No further code change is needed here once that merges.

Not covered by this MR

References

How to set up and validate locally

  1. Open a rails console.
  2. Run the following. On master the last line returns 1; with this MR it returns 0.
project = FactoryBot.create(:project)
export = FactoryBot.create(:vulnerability_export, :with_csv_file, project: project)
Upload.for_model_type_and_id(Vulnerabilities::Export, export.id).count # => 1
project.destroy!
LooseForeignKeys::CleanupWorker.new.perform
Vulnerabilities::Export.exists?(export.id) # => false
Upload.for_model_type_and_id(Vulnerabilities::Export, export.id).count # => 0

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Ryan Wells

Merge request reports