Upload external diffs before the MergeRequestDiff save transaction

Related to #631751

• MergeRequestDiff uploaded the external diff to object storage while the save! transaction was still open. The database session sat idle in transaction for the whole upload.
• The upload now happens in preload_gitaly_data, which every production creation path already calls before save!, outside any transaction. New objects are named diff-<uuid> since the row has no id yet; reads already use the stored external_diff identifier, so old diff-<id> objects keep working.
• Behind the external_diff_upload_before_save feature flag, default off. With the flag off, and for callers that do not preload (specs, factories), the upload timing and the diff-<id> object name are exactly as before.

Detailed context for AI agents

Problem

MergeRequestDiff has after_create :save_git_content, which runs inside the save! transaction.

For external diffs on object storage (GitLab.com config: external diffs enabled on GCS, when defaults to always), save_diffs does this:

1. Writes the diff text to a tempfile.
2. Assigns it to the CarrierWave uploader external_diff.
3. Deletes the diff text from the merge_request_diff_files rows.
4. Does a nested save.

CarrierWave's after_save :store_external_diff! then uploads the file to object storage while the transaction is still open. The DB session stays idle in transaction for the whole upload. It holds a PgBouncer connection and row locks.

The last statement before the idle period in the logs is UPDATE "merge_request_diffs" SET "state" = $1, ... "external_diff" = $4, "stored_externally" = $5 ....

This hits every diff creation. MergeRequestMergeabilityCheckWorker hits it most, because each recheck that ends mergeable recreates the merge head diff through MergeRequests::ReloadMergeHeadDiffService.

Fix (app/models/merge_request_diff.rb and app/uploaders/external_diff_uploader.rb)

All three production creation paths already call MergeRequestDiff#preload_gitaly_data before save!, outside any transaction:

• MergeRequest#create_merge_request_diff(preload_gitaly: true), used by MergeRequests::AfterCreateService (via ensure_merge_request_diff).
• The same method, used by MergeRequests::ReloadDiffsService on push.
• MergeRequests::ReloadMergeHeadDiffService.

preload_gitaly_data now also calls a new private method, store_external_diff_before_save.

store_external_diff_before_save returns early unless the record is new, use_external_diff? is true, and the preloaded compare has commits and diff files. Otherwise it:

1. Calls ensure_project_id.
2. Builds the external diff with an explicit object name, diff-<uuid>, because a new record has no id and the old name was diff-<id>. ExternalDiffUploader#filename now returns the name given at upload time or the stored identifier, falling back to diff-<id>. CarrierWave writes that name into the external_diff column and reads resolve the object from the column (retrieve_from_store!(identifier) -> store_path(identifier)), so existing diff-<id> objects are unaffected. migrate_files_to_external_storage! and non-preloaded creates have an id and still produce diff-<id>.
3. Builds the diff file rows and the external diff tempfile, using the existing build_merge_request_diff_files and build_external_merge_request_diff_files. The rows are now memoized in merge_request_diff_file_rows.
4. Calls external_diff.store! to upload.
5. Sets external_diff_store from the uploader so the INSERT carries it. The existing after_save :update_external_diff_store does not fire for the create, because save_git_content reloads the record before the outer after_save hooks run.

save_diffs now stamps merge_request_diff_id on the prebuilt rows (nil when built before the INSERT), bulk-inserts them and clears the memo. When nothing was preloaded (specs, factories, any other caller), the rows and the upload still happen inside the transaction exactly as before, so behaviour is unchanged for those paths.

The already-uploaded file is a no-op for CarrierWave's after_save store hook inside the transaction, because the uploader's cache id is cleared after the first store!. This is the same pattern MergeRequestDiff#migrate_files_to_external_storage! already uses ("Perform carrierwave activity before entering the database transaction").

old_version? change

old_version? is now nil-safe on id (latest_id && id && id < latest_id). use_external_diff? with when: outdated is now evaluated before the record has an id, and an unsaved diff is never an old version.

Feature flag

store_external_diff_before_save first calls ensure_project_id, then returns unless Feature.enabled?(:external_diff_upload_before_save, Project.actor_from_id(project_id)). With the flag off that is the only extra work in preload_gitaly_data: no rows are memoized, so save_diffs builds the rows, tempfile and upload inside the transaction as on master, and the record has an id by then so the object is named diff-<id>.

• Type gitlab_com_derisk, actor project, rollout issue #631918.
• Rolling back is safe: diffs created while the flag was on keep their diff-<uuid> identifier in the external_diff column and read fine with the flag off.

Why not an after-commit hook

The upload is not moved to an after-commit hook on purpose. That would commit a row with stored_externally = true before the file exists, and a failed upload would leave a permanently broken diff.

Trade-offs

• If save! fails or rolls back after the upload, the object storage file is orphaned.
• An orphan on rollback already existed before this change. The upload happened inside the transaction, so any later statement failing also rolled back around an already-uploaded object. The window is now a bit wider.
• A cleanup path was deliberately left out of this MR.
• Object naming: existing objects stay merge_request_diffs/mr-<merge_request_id>/diff-<id>; new preloaded uploads are merge_request_diffs/mr-<merge_request_id>/diff-<uuid>. Nothing looks objects up by id; the external_diff column is the source of truth. The nextval id-reservation alternative was rejected as too unusual for model code.

Verification

New model specs in spec/models/merge_request_diff_spec.rb, under "external diffs in object storage always enabled":

• One asserts that the upload happens at transaction depth 0, after preload_gitaly_data and before save!. It also asserts that the file exists in the (stubbed) object store, and that after save! the row is stored_externally, collected, with external_diff_store REMOTE, the object path ending in /mr-<mr id>/<stored identifier> with a diff-<uuid> identifier, and 20 diff file rows.
• One, with the flag disabled, asserts that preload_gitaly_data uploads nothing, that the upload then happens inside the save! transaction, and that the object is named diff-<id>.
• One asserts that the non-preloaded create! path still stores externally, named diff-<id>.

Spec files run locally:

• spec/models/merge_request_diff_spec.rb
• spec/services/merge_requests/reload_merge_head_diff_service_spec.rb
• spec/services/merge_requests/reload_diffs_service_spec.rb
• spec/services/merge_requests/after_create_service_spec.rb
• spec/uploaders/external_diff_uploader_spec.rb
• spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

All passed: 401, 5, 8, 26, 19 (10 pre-existing pending) and 15 examples, 0 failures.

Also ran a throwaway probe spec that called MergeRequestMergeabilityCheckWorker#perform with object storage stubbed and recorded the open transaction count at upload time. It was 0 after the change and 1 before.

Related

• Issue: #631751
• Rollout issue: #631918
• Investigation issue, item A4: #631600
• #628868 (closed)

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.