Merge branch 'sh-project-exports-avoid-idling-in-transaction' into 'master'

Avoid idling in transaction while saving project export object

See merge request gitlab-org/gitlab!63350

app/controllers/groups_controller.rb

@@ -183,7 +183,12 @@ class GroupsController < Groups::ApplicationController
 
   def download_export
     if @group.export_file_exists?
-      send_upload(@group.export_file, attachment: @group.export_file.filename)
+      if @group.export_archive_exists?
+        send_upload(@group.export_file, attachment: @group.export_file.filename)
+      else
+        redirect_to edit_group_path(@group),
+                    alert: _('The file containing the export is not available yet; it may still be transferring. Please try again later.')
+      end
     else
       redirect_to edit_group_path(@group),
         alert: _('Group export link has expired. Please generate a new export from your group settings.')

app/controllers/projects_controller.rb

@@ -226,7 +226,14 @@ class ProjectsController < Projects::ApplicationController
 
   def download_export
     if @project.export_file_exists?
-      send_upload(@project.export_file, attachment: @project.export_file.filename)
+      if @project.export_archive_exists?
+        send_upload(@project.export_file, attachment: @project.export_file.filename)
+      else
+        redirect_to(
+          edit_project_path(@project, anchor: 'js-export-project'),
+          alert: _("The file containing the export is not available yet; it may still be transferring. Please try again later.")
+        )
+      end
     else
       redirect_to(
         edit_project_path(@project, anchor: 'js-export-project'),

app/models/group.rb

@@ -647,13 +647,17 @@ class Group < Namespace
   end
 
   def export_file_exists?
-    export_file&.file
+    import_export_upload&.export_file_exists?
   end
 
   def export_file
     import_export_upload&.export_file
   end
 
+  def export_archive_exists?
+    import_export_upload&.export_archive_exists?
+  end
+
   def adjourned_deletion?
     false
   end

app/models/import_export_upload.rb

@@ -11,10 +11,42 @@ class ImportExportUpload < ApplicationRecord
   mount_uploader :import_file, ImportExportUploader
   mount_uploader :export_file, ImportExportUploader
 
+  # This causes CarrierWave v1 and v3 (but not v2) to upload the file to
+  # object storage *after* the database entry has been committed to the
+  # database. This avoids idling in a transaction.
+  if Gitlab::Utils.to_boolean(ENV.fetch('ENABLE_STORE_EXPORT_FILE_AFTER_COMMIT', true))
+    skip_callback :save, :after, :store_export_file!
+    set_callback :commit, :after, :store_export_file!
+  end
+
   scope :updated_before, ->(date) { where('updated_at < ?', date) }
   scope :with_export_file, -> { where.not(export_file: nil) }
 
   def retrieve_upload(_identifier, paths)
     Upload.find_by(model: self, path: paths)
   end
+
+  def export_file_exists?
+    !!carrierwave_export_file
+  end
+
+  # This checks if the export archive is actually stored on disk. It
+  # requires a HEAD request if object storage is used.
+  def export_archive_exists?
+    !!carrierwave_export_file&.exists?
+  # Handle any HTTP unexpected error
+  # https://github.com/excon/excon/blob/bbb5bd791d0bb2251593b80e3bce98dbec6e8f24/lib/excon/error.rb#L129-L169
+  rescue Excon::Error => e
+    # The HEAD request will fail with a 403 Forbidden if the file does not
+    # exist, and the user does not have permission to list the object
+    # storage bucket.
+    Gitlab::ErrorTracking.track_exception(e)
+    false
+  end
+
+  private
+
+  def carrierwave_export_file
+    export_file&.file
+  end
 end

app/models/project.rb

@@ -1987,7 +1987,11 @@ class Project < ApplicationRecord
   end
 
   def export_file_exists?
-    export_file&.file
+    import_export_upload&.export_file_exists?
+  end
+
+  def export_archive_exists?
+    import_export_upload&.export_archive_exists?
   end
 
   def export_file

lib/api/group_export.rb

@@ -23,7 +23,11 @@ module API
         check_rate_limit! :group_download_export, [current_user, user_group]
 
         if user_group.export_file_exists?
-          present_carrierwave_file!(user_group.export_file)
+          if user_group.export_archive_exists?
+            present_carrierwave_file!(user_group.export_file)
+          else
+            render_api_error!('The group export file is not available yet', 404)
+          end
         else
           render_api_error!('404 Not found or has expired', 404)
         end

lib/api/project_export.rb

@@ -30,7 +30,11 @@ module API
         check_rate_limit! :project_download_export, [current_user, user_project]
 
         if user_project.export_file_exists?
-          present_carrierwave_file!(user_project.export_file)
+          if user_project.export_archive_exists?
+            present_carrierwave_file!(user_project.export_file)
+          else
+            render_api_error!('The project export file is not available yet', 404)
+          end
         else
           render_api_error!('404 Not found or has expired', 404)
         end

locale/gitlab.pot

@@ -32431,6 +32431,9 @@ msgstr ""
 msgid "The errors we encountered were:"
 msgstr ""
 
+msgid "The file containing the export is not available yet; it may still be transferring. Please try again later."
+msgstr ""
+
 msgid "The file has been successfully created."
 msgstr ""
 

spec/controllers/groups_controller_spec.rb

@@ -1065,14 +1065,13 @@ RSpec.describe GroupsController, factory_default: :keep do
 
   describe 'GET #download_export' do
     let(:admin) { create(:admin) }
+    let(:export_file) { fixture_file_upload('spec/fixtures/group_export.tar.gz') }
 
     before do
       enable_admin_mode!(admin)
     end
 
     context 'when there is a file available to download' do
-      let(:export_file) { fixture_file_upload('spec/fixtures/group_export.tar.gz') }
-
       before do
         sign_in(admin)
         create(:import_export_upload, group: group, export_file: export_file)
@@ -1085,6 +1084,22 @@ RSpec.describe GroupsController, factory_default: :keep do
       end
     end
 
+    context 'when the file is no longer present on disk' do
+      before do
+        sign_in(admin)
+
+        create(:import_export_upload, group: group, export_file: export_file)
+        group.export_file.file.delete
+      end
+
+      it 'returns not found' do
+        get :download_export, params: { id: group.to_param }
+
+        expect(flash[:alert]).to include('file containing the export is not available yet')
+        expect(response).to redirect_to(edit_group_path(group))
+      end
+    end
+
     context 'when there is no file available to download' do
       before do
         sign_in(admin)

spec/controllers/projects_controller_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe ProjectsController do
   include ProjectForksHelper
   using RSpec::Parameterized::TableSyntax
 
-  let_it_be(:project, reload: true) { create(:project, service_desk_enabled: false) }
+  let_it_be(:project, reload: true) { create(:project, :with_export, service_desk_enabled: false) }
   let_it_be(:public_project) { create(:project, :public) }
   let_it_be(:user) { create(:user) }
 
@@ -1349,7 +1349,7 @@ RSpec.describe ProjectsController do
       end
     end
 
-    describe '#download_export' do
+    describe '#download_export', :clean_gitlab_redis_cache do
       let(:action) { :download_export }
 
       context 'object storage enabled' do
@@ -1361,6 +1361,17 @@ RSpec.describe ProjectsController do
           end
         end
 
+        context 'when project export file is absent' do
+          it 'alerts the user and returns 302' do
+            project.export_file.file.delete
+
+            get action, params: { namespace_id: project.namespace, id: project }
+
+            expect(flash[:alert]).to include('file containing the export is not available yet')
+            expect(response).to have_gitlab_http_status(:found)
+          end
+        end
+
         context 'when project export is disabled' do
           before do
             stub_application_setting(project_export_enabled?: false)

spec/factories/groups.rb

@@ -58,6 +58,13 @@ FactoryBot.define do
       shared_runners_enabled { false }
     end
 
+    trait :with_export do
+      after(:create) do |group, _evaluator|
+        export_file = fixture_file_upload('spec/fixtures/group_export.tar.gz')
+        create(:import_export_upload, group: group, export_file: export_file)
+      end
+    end
+
     trait :allow_descendants_override_disabled_shared_runners do
       allow_descendants_override_disabled_shared_runners { true }
     end

spec/models/group_spec.rb

@@ -2613,4 +2613,16 @@ RSpec.describe Group do
       expect(group.activity_path).to eq(expected_path)
     end
   end
+
+  context 'with export' do
+    let(:group) { create(:group, :with_export) }
+
+    it '#export_file_exists returns true' do
+      expect(group.export_file_exists?).to be true
+    end
+
+    it '#export_archive_exists? returns true' do
+      expect(group.export_archive_exists?).to be true
+    end
+  end
 end

spec/models/import_export_upload_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe ImportExportUpload do
-  subject { described_class.new(project: create(:project)) }
+  let(:project) { create(:project) }
+
+  subject { described_class.new(project: project) }
 
   shared_examples 'stores the Import/Export file' do |method|
     it 'stores the import file' do
@@ -43,4 +45,80 @@ RSpec.describe ImportExportUpload do
       end
     end
   end
+
+  context 'ActiveRecord callbacks' do
+    let(:after_save_callbacks) { described_class._save_callbacks.select { |cb| cb.kind == :after } }
+    let(:after_commit_callbacks) { described_class._commit_callbacks.select { |cb| cb.kind == :after } }
+
+    def find_callback(callbacks, key)
+      callbacks.find { |cb| cb.instance_variable_get(:@key) == key }
+    end
+
+    it 'export file is stored in after_commit callback' do
+      expect(find_callback(after_commit_callbacks, :store_export_file!)).to be_present
+      expect(find_callback(after_save_callbacks, :store_export_file!)).to be_nil
+    end
+
+    it 'import file is stored in after_save callback' do
+      expect(find_callback(after_save_callbacks, :store_import_file!)).to be_present
+      expect(find_callback(after_commit_callbacks, :store_import_file!)).to be_nil
+    end
+  end
+
+  describe 'export file' do
+    it '#export_file_exists? returns false' do
+      expect(subject.export_file_exists?).to be false
+    end
+
+    it '#export_archive_exists? returns false' do
+      expect(subject.export_archive_exists?).to be false
+    end
+
+    context 'with export' do
+      let(:project_with_export) { create(:project, :with_export) }
+
+      subject { described_class.with_export_file.find_by(project: project_with_export) }
+
+      it '#export_file_exists? returns true' do
+        expect(subject.export_file_exists?).to be true
+      end
+
+      it '#export_archive_exists? returns false' do
+        expect(subject.export_archive_exists?).to be true
+      end
+
+      context 'when object file does not exist' do
+        before do
+          subject.export_file.file.delete
+        end
+
+        it '#export_file_exists? returns true' do
+          expect(subject.export_file_exists?).to be true
+        end
+
+        it '#export_archive_exists? returns false' do
+          expect(subject.export_archive_exists?).to be false
+        end
+      end
+
+      context 'when checking object existence raises a error' do
+        let(:exception) { Excon::Error::Forbidden.new('not allowed') }
+
+        before do
+          file = double
+          allow(file).to receive(:exists?).and_raise(exception)
+          allow(subject).to receive(:carrierwave_export_file).and_return(file)
+        end
+
+        it '#export_file_exists? returns true' do
+          expect(subject.export_file_exists?).to be true
+        end
+
+        it '#export_archive_exists? returns false' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception)
+          expect(subject.export_archive_exists?).to be false
+        end
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -4376,6 +4376,18 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  context 'with export' do
+    let(:project) { create(:project, :with_export) }
+
+    it '#export_file_exists? returns true' do
+      expect(project.export_file_exists?).to be true
+    end
+
+    it '#export_archive_exists? returns false' do
+      expect(project.export_archive_exists?).to be true
+    end
+  end
+
   describe '#forks_count' do
     it 'returns the number of forks' do
       project = build(:project)
@@ -6638,7 +6650,7 @@ RSpec.describe Project, factory_default: :keep do
     context 'when project export is completed' do
       before do
         finish_job(project_export_job)
-        allow(project).to receive(:export_file).and_return(double(ImportExportUploader, file: 'exists.zip'))
+        allow(project).to receive(:export_file_exists?).and_return(true)
       end
 
       it { expect(project.export_status).to eq :finished }
@@ -6649,7 +6661,7 @@ RSpec.describe Project, factory_default: :keep do
 
       before do
         finish_job(project_export_job)
-        allow(project).to receive(:export_file).and_return(double(ImportExportUploader, file: 'exists.zip'))
+        allow(project).to receive(:export_file_exists?).and_return(true)
       end
 
       it { expect(project.export_status).to eq :regeneration_in_progress }

spec/requests/api/group_export_spec.rb

@@ -64,6 +64,23 @@ RSpec.describe API::GroupExport do
             expect(response).to have_gitlab_http_status(:not_found)
           end
         end
+
+        context 'when object is not present' do
+          let(:other_group) { create(:group, :with_export) }
+          let(:other_download_path) { "/groups/#{other_group.id}/export/download" }
+
+          before do
+            other_group.add_owner(user)
+            other_group.export_file.file.delete
+          end
+
+          it 'returns 404' do
+            get api(other_download_path, user)
+
+            expect(response).to have_gitlab_http_status(:not_found)
+            expect(json_response['message']).to eq('The group export file is not available yet')
+          end
+        end
       end
 
       context 'when export file does not exist' do

spec/requests/api/project_export_spec.rb

@@ -196,6 +196,19 @@ RSpec.describe API::ProjectExport, :clean_gitlab_redis_cache do
         end
       end
 
+      context 'when export object is not present' do
+        before do
+          project_after_export.export_file.file.delete
+        end
+
+        it 'returns 404' do
+          get api(download_path_export_action, user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+          expect(json_response['message']).to eq('The project export file is not available yet')
+        end
+      end
+
       context 'when upload complete' do
         before do
           project_after_export.remove_exports