Sweep upload records carrierwave leaves behind on destroy

Smriti Gargrequested to merge
smriti-602881/deleted-user-records-leave-behind-orphaned-upload-records into master

What does this MR do and why?

CarrierWave and RecordsUploads guard Upload deletion with two independent gates; either alone can strand the row. RecordsUploads::Concern#destroy_upload returns early when file.exists? is false, so a row naming a file missing from disk or object storage never gets deleted — the common case. Mounter#remove! also filters out blank uploaders, so an empty mounted column means the before :remove hook never fires. Either way the Upload row outlives its model, and Geo verification later fails with The model which owns this upload is missing.

The sweep deletes only Upload rows; file deletion stays with carrierwave, and DeleteStoredFilesWorker is untouched. When carrierwave already cleaned up, delete_all removes nothing, so the healthy path is unchanged. Rows-only is sufficient since every reported case already has a missing file, and file deletion is unrecoverable if the scope were ever wrong. Known gap: uploads with a blank mounted column never ran carrierwave, so their files stay orphaned too; no evidence this occurs in reported data.

Existing orphaned rows are not touched; the issue documents a console workaround.

Prior art

Fixing this in the shared concern, not per-model, because per-model fixes haven't stopped the pattern. This is roughly the fifth filing of the same bug; one sibling is still open, and the one that was closed took six MRs including a batched background migration.

Feature flag

  • Name: sweep_orphaned_mounted_uploads
  • Type: gitlab_com_derisk, default_enabled: false, milestone 19.4, group group::authentication
  • Rollout issue: #621779

The flag is instance-scoped, not actor-scoped, because no actor type is common to every model that includes the concern; it's an on/off kill switch, not a percentage rollout. Plan: enable on GitLab.com after deploy, remove the flag in 19.5. Dedicated won't get the fix until the flag is removed.

How to set up and validate locally

Feature.enable(:sweep_orphaned_mounted_uploads)   # off by default

user = create(:user, :with_avatar)
FileUtils.rm_f(user.avatar.path)   # simulate the missing file
user.destroy!

Upload.for_model_type_and_id('User', user.id).count   # 0 with this change, 1 without

Database review

What's new

sweep_mounted_uploads in app/models/concerns/with_uploads.rb runs mounted_uploads.delete_all from the after-commit queue, so carrierwave's own hook goes first and only the rows it skipped remain. That delete_all is the query that triggered the database label. app/models/upload.rb is not part of this diff.

delete_all skips callbacks, so no Geo deletion event is published for the swept rows. That matches carrierwave, whose RecordsUploads::Concern#destroy_upload also ends in delete_all, so no mounted upload deletion publishes an event today. On a secondary the registry is reconciled by Geo::Secondary::RegistryConsistencyWorker, which drops registries whose replicable no longer exists.

Queries per destroy

One new query per destroy, the delete_all, behind the sweep_orphaned_mounted_uploads flag; with the flag off nothing changes from master. Examples use Project, whose uploader list is ('AvatarUploader', 'AttachmentUploader').

Pre-existing and unchanged in count: capture_mounted_remote_uploaders runs one SELECT when uploads_cascade_deleted_on_destroy? is true. This MR only rewrote its scope.

DELETE FROM uploads
WHERE uploads.model_type = 'Project' AND uploads.model_id = ?
  AND uploads.uploader IN ('AvatarUploader', 'AttachmentUploader')

Row counts and indexes

uploads is partitioned by model_type; each partition has an index on (model_id, model_type, uploader, created_at). Every query filters on model_type and model_id, so all of them prune to one partition. Rows returned are bounded by how many uploaders a model mounts — one to four in practice.

An earlier version scoped with uploader NOT IN (FILE_UPLOADERS). A negated IN can't be an index condition, so uploader became a filter and the scan covered the model's entire upload count — on gitlab-org/gitlab, roughly 509,788 rows, all reported as Rows Removed by Filter: 509787. Rewritten to select the model's own mounted uploaders positively, the IN list becomes part of the index condition and nothing is removed by filter.

Execution time only, excluding planning:

Query Before After
Scope load 3.66 min, ~445,500 buffers 5.0 ms, 8 buffers
Delete 5.87 s, ~445,500 buffers 7.8 ms, 14 buffers

This also improves a query already on master: capture_mounted_remote_uploaders uses the same scope, so the 3.66-minute figure is current production behaviour when destroying a large project, not something this MR introduces. A User was never affected — its plans were sub-10 ms throughout, since cost scales with how many uploads the model owns, and a user owns one avatar.

Query plans

All against gitlab-production-main, project 278964 (gitlab-org/gitlab).

Before — scope load, 3.66 min 
 Index Scan using project_uploads_model_id_model_type_uploader_created_at_idx on public.project_uploads uploads  (cost=0.57..592852.10 rows=14561 width=297) (actual time=9.230..219552.981 rows=1 loops=1)
   Index Cond: ((uploads.model_id = 278964) AND (uploads.model_type = 'Project'::text))
   Filter: (uploads.uploader <> ALL ('{PersonalFileUploader,NamespaceFileUploader,FileUploader}'::text[]))
   Rows Removed by Filter: 509787
   Buffers: shared hit=45726 read=399797 dirtied=22084 written=260
   I/O Timings: read=215532.707 write=8.799

Time: 3.659 min
After — scope load, 4.9 ms 
 Index Scan using project_uploads_model_id_model_type_uploader_created_at_idx on public.project_uploads uploads  (cost=0.57..18787.40 rows=14561 width=297) (actual time=4.953..4.955 rows=1 loops=1)
   Index Cond: ((uploads.model_id = 278964) AND (uploads.model_type = 'Project'::text) AND (uploads.uploader = ANY ('{AvatarUploader,AttachmentUploader}'::text[])))
   Buffers: shared hit=4 read=4
   I/O Timings: read=4.851 write=0.000

Time: 95.209 ms
  - planning: 90.203 ms
  - execution: 5.006 ms
Before — delete, 5.87 s 
 Delete on public.uploads  (cost=0.57..592852.10 rows=0 width=0) (actual time=5839.447..5839.449 rows=0 loops=1)
   Buffers: shared hit=45812 read=399718 dirtied=2
   WAL: records=1 fpi=1 bytes=8067
   ->  Index Scan using project_uploads_model_id_model_type_uploader_created_at_idx on public.project_uploads uploads_1  (cost=0.57..592852.10 rows=14561 width=10) (actual time=0.399..5837.429 rows=1 loops=1)
         Index Cond: ((uploads_1.model_id = 278964) AND (uploads_1.model_type = 'Project'::text))
         Filter: (uploads_1.uploader <> ALL ('{PersonalFileUploader,NamespaceFileUploader,FileUploader}'::text[]))
         Rows Removed by Filter: 509787
Trigger uploads_loose_fk_trigger: time=17.737 calls=1
Trigger RI_ConstraintTrigger_a_309468023 for constraint fk_rails_a21cb2b8a2: time=8.309 calls=1

Time: 5.873 s
After — delete, 2.08 ms 
 Delete on public.uploads  (cost=0.57..18787.40 rows=0 width=0) (actual time=2.079..2.080 rows=0 loops=1)
   Buffers: shared hit=11 read=3 dirtied=1
   WAL: records=1 fpi=1 bytes=8067
   I/O Timings: read=1.900 write=0.000
   ->  Index Scan using project_uploads_model_id_model_type_uploader_created_at_idx on public.project_uploads uploads_1  (cost=0.57..18787.40 rows=14561 width=10) (actual time=0.079..0.081 rows=1 loops=1)
         Index Cond: ((uploads_1.model_id = 278964) AND (uploads_1.model_type = 'Project'::text) AND (uploads_1.uploader = ANY ('{AvatarUploader,AttachmentUploader}'::text[])))
         Buffers: shared hit=8
         I/O Timings: read=0.000 write=0.000
Trigger uploads_loose_fk_trigger: time=1.548 calls=1
Trigger RI_ConstraintTrigger_a_309468023 for constraint fk_rails_a21cb2b8a2: time=0.620 calls=1

References

Resolves #602881 (closed)

Feature flag rollout: #621779

Edited by Smriti Garg

Merge request reports