A prefix away from deleting a live file: fixing Geo's in-use check

GitLab Geo keeps a secondary site in sync with a primary. When a file is deleted on the primary, the secondary has to delete its copy too. That sounds simple, until two records share a path, or the path the secondary stored is not quite the path the "is this still in use?" check looks at.

This is the story of gitlab-org/gitlab!265937, which stopped Geo from treating a perfectly live object-storage file as unused. I'll cover the issue, the plan, the two-path check, a local-path trap that made a one-sided fix dangerous, and the review that tightened the specs.

The issue: a safety check that never matched

The issue was gitlab-org/gitlab#636176, opened by Chloe Fons. It came out of work to roll out a feature flag, geo_blob_removal_uses_registry_location (#635220).

Some background. On a Geo secondary, each blob registry row stores a blob_path: the file's path relative to its uploader root, or its object storage key. When a delete event arrives without a path, Geo::DestroyWorker falls back to that stored location and Geo::FileRegistryRemovalService deletes the file. Before it does, blob_location_in_use? asks whether a newer record reuses the same path, so a live file is never removed.

With object storage replication and a bucket_prefix, that check broke down. ObjectStorage::Concern#store_path adds the prefix, so the path saved on the registry is <bucket_prefix>/<key>. The values the in-use overrides compare against never include it:

Blob type	What the in-use check compares
Uploads	`Upload#path`
LFS objects	the LFS oid
Helm metadata caches, NuGet symbols	`object_storage_key`

So the check never matched for those types. Terraform state versions only read the last two path segments, so they were not affected. For the others, the fallback could delete an object that a newer record still used.

This had to land before the flag was enabled by default. A wrong "in use" result only leaves an orphaned file. A wrong "not in use" result deletes a file that a live record uses.

The plan, before writing any code

I commented on the issue with a plan that followed Chloe's write-up:

Planned approach, following the implementation plan: in ee/app/models/concerns/geo/blob_replicator_strategy.rb, registry_location_params will call a new private stored_location_in_use?(blob_path, uploader_class) that returns true when blob_location_in_use? matches either the stored path or the path with the uploader's "#{bucket_prefix}/" removed (checking both, since a local path can start with the prefix by chance). The returned location keeps the prefix for the removal service. I'll add specs with a bucket prefix for an upload replicator and an object storage key type (Helm metadata cache / NuGet symbol).

Chloe assigned me and highlighted one detail:

Your plan sounds good. If you could also follow the Specs section as written, especially the third user-upload example, that'd be awesome.

That third example is the reason the fix checks both paths, and it's the one I was most careful to keep.

The two-path check

The production change is small, and it sits in one method. registry_location_params used to call blob_location_in_use? on the stored path alone. It now calls a new private method, and the hash it returns still carries the prefixed path, because the removal service needs that key to find the object:

# Object storage keys start with the bucket prefix, but the columns the in-use checks match never include it. # A local path can start with the prefix too, so a match with or without it counts. def stored_location_in_use?(blob_path, uploader_class) bucket_prefix = uploader_class.constantize.object_store_options.bucket_prefix paths = [blob_path] paths << blob_path.delete_prefix("#{bucket_prefix}/") if bucket_prefix.present? paths.uniq.any? { |path| blob_location_in_use?(path, uploader_class) } end

Why both paths, not only the stripped one? A local path can start with the prefix by chance. If remote_directory is gitlab/uploads, the prefix is uploads, and a local avatar path looks like uploads/-/system/.... If you only check the shorter path, a live local file whose path is the stored path can look unused. The issue is explicit about the trade-off: fail safe (leave an orphan) rather than fail loose (delete a live file).

The existing ObjectStorage::Concern.without_bucket_prefix helper does not fit here. For a path outside the prefix, Pathname#relative_path_from returns ../../..., which is not a useful comparison key. The default blob_location_in_use? and its replicator overrides stayed unchanged.

Specs that fail without the fix

The issue laid out the examples to add, and I followed that list.

The shared example a blob replicator whose blob path can be reused is already used by the LFS, Helm metadata cache, NuGet symbol, and Terraform replicator specs. I added a #replicate_destroy with a bucket prefix block that stubs the prefix to my/prefix, stores the prefixed path on the registry, deletes the original record, and then checks two outcomes:

When a record reuses the unprefixed path, Geo::FileRegistryRemovalService is constructed with nil, nil — no location, so the file is left alone.
When nothing reuses the path, the service receives the prefixed absolute path and the uploader class, so the object can actually be found and removed.

User uploads needed their own copy of those two examples in user_upload_replicator_spec.rb, because avatars use AvatarUploader and can reuse a path. The third example is the local-path trap:

context 'when a local path starts with the prefix' do let(:bucket_prefix) { 'uploads' } let(:stored_blob_path) { local_path } it 'passes no location when an upload reuses the stored path' do expect(local_path).to start_with("#{bucket_prefix}/") create(:upload, model: model_record.model, path: local_path) expect_removal_service_with(nil, nil) secondary_side_replicator.replicate_destroy({}) end end

The prefix is uploads, the stored path is the local avatar path unchanged, and a new upload reuses it. If the code only checked the stripped path, this example would fail.

I was honest in the MR about what I had and hadn't run. I had a sparse checkout without the GitLab Rails and database environment, so I ran RuboCop on the three changed files and left the new examples to CI. The command from the issue is bin/rspec ee/spec/replicators/geo/ ee/spec/workers/geo/destroy_worker_spec.rb. I also wrote down how to confirm the nil, nil examples fail without the production change: revert the strategy file and rerun.

The review: cleaner specs, and one more example

Chloe's first pass was "a great start," with a handful of spec suggestions and a question:

I wonder if we should also add one example that creates an upload with the :object_storage factory trait and the prefix stubbed, and asserts that replicator.deleted_params[:blob_path] starts with the prefix.

That's a different path from replicate_destroy. deleted_params is what gets written to the registry at sync time. If the stored path never starts with the prefix, the in-use fix has nothing to strip. I added a #deleted_params with a bucket prefix example that creates the upload with :remote_store (the :object_storage trait), stubs object storage and the prefix, and checks the blob path starts with my/prefix/.

The other comments were about making the shared examples leaner, and I applied all of them in 55222358:

Remove uploader_class: and the attribute select from the shared-example registry. Those examples are only used by LFS, Helm, NuGet, and Terraform, none of which have an uploader class.
Move blob_path and uploader_class out of the inner describe, so they're defined once.
Drop explicit model_record loads that weren't needed, because later lines instantiate the model anyway.
In the user-upload examples, don't read model_record.model into a let before delete. After model_record.delete, model_record.model still loads, and the specs now use create(:upload, model: model_record.model, path: local_path).

Chloe reviewed the new version and approved it. Chloe also merged the MR, on 30 October 2026.

What I took away

1. Asymmetry matters more than symmetry. A check that protects deletions should bias toward "in use." An orphaned file can be cleaned up later. A deleted live file cannot.

2. Strip a prefix, but also keep the original. The bug was "the stored path has a prefix the comparison column doesn't." The trap is "sometimes the stored path is a local path that starts with the same letters." Both comparisons have to pass.

3. Follow the issue's spec plan, especially the awkward example. The third user-upload example exists to fail a one-sided fix. Chloe asked for it by name, and it earned its place.

4. A review that deletes setup is as useful as one that adds it. Moving lets up, dropping unused registry fields, and not pre-loading associations made the examples easier to read and less brittle.

5. Say what you couldn't run locally. I didn't have a full GitLab Rails environment. Writing that down, plus the exact rspec command and how to prove the examples fail without the fix, made the review easier to start.