Merge branch 'georgekoltsov/196188-cleanup-temp-exports' into 'master'

Ensure temp export data is removed if Group/Project export failed

See merge request gitlab-org/gitlab!25828

app/services/groups/import_export/export_service.rb

@@ -18,6 +18,8 @@ module Groups
         end
 
         save!
+      ensure
+        cleanup
       end
 
       private
@@ -28,7 +30,7 @@ module Groups
         if savers.all?(&:save)
           notify_success
         else
-          cleanup_and_notify_error!
+          notify_error!
         end
       end
 
@@ -44,14 +46,12 @@ module Groups
         Gitlab::ImportExport::Saver.new(exportable: @group, shared: @shared)
       end
 
-      def cleanup_and_notify_error
-        FileUtils.rm_rf(shared.export_path)
-
-        notify_error
+      def cleanup
+        FileUtils.rm_rf(shared.archive_path) if shared&.archive_path
       end
 
-      def cleanup_and_notify_error!
-        cleanup_and_notify_error
+      def notify_error!
+        notify_error
 
         raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence)
       end

app/services/projects/import_export/export_service.rb

@@ -14,6 +14,8 @@ module Projects
 
         save_all!
         execute_after_export_action(after_export_strategy)
+      ensure
+        cleanup
       end
 
       private
@@ -24,7 +26,7 @@ module Projects
         return unless after_export_strategy
 
         unless after_export_strategy.execute(current_user, project)
-          cleanup_and_notify_error
+          notify_error
         end
       end
 
@@ -33,7 +35,7 @@ module Projects
           Gitlab::ImportExport::Saver.save(exportable: project, shared: shared)
           notify_success
         else
-          cleanup_and_notify_error!
+          notify_error!
         end
       end
 
@@ -73,16 +75,12 @@ module Projects
         Gitlab::ImportExport::LfsSaver.new(project: project, shared: shared)
       end
 
-      def cleanup_and_notify_error
-        Rails.logger.error("Import/Export - Project #{project.name} with ID: #{project.id} export error - #{shared.errors.join(', ')}") # rubocop:disable Gitlab/RailsLogger
-
-        FileUtils.rm_rf(shared.export_path)
-
-        notify_error
+      def cleanup
+        FileUtils.rm_rf(shared.archive_path) if shared&.archive_path
       end
 
-      def cleanup_and_notify_error!
-        cleanup_and_notify_error
+      def notify_error!
+        notify_error
 
         raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence)
       end
@@ -92,6 +90,8 @@ module Projects
       end
 
       def notify_error
+        Rails.logger.error("Import/Export - Project #{project.name} with ID: #{project.id} export error - #{shared.errors.join(', ')}") # rubocop:disable Gitlab/RailsLogger
+
         notification_service.project_not_exported(project, current_user, shared.errors)
       end
     end

changelogs/unreleased/georgekoltsov-196188-cleanup-temp-exports.yml

+---
+title: Ensure temp export data is removed if Group/Project export failed
+merge_request: 25828
+author:
+type: fixed

spec/services/groups/import_export/export_service_spec.rb

@@ -7,7 +7,7 @@ describe Groups::ImportExport::ExportService do
     let!(:user) { create(:user) }
     let(:group) { create(:group) }
     let(:shared) { Gitlab::ImportExport::Shared.new(group) }
-    let(:export_path) { shared.export_path }
+    let(:archive_path) { shared.archive_path }
     let(:service) { described_class.new(group: group, user: user, params: { shared: shared }) }
 
     before do
@@ -15,7 +15,7 @@ describe Groups::ImportExport::ExportService do
     end
 
     after do
-      FileUtils.rm_rf(export_path)
+      FileUtils.rm_rf(archive_path)
     end
 
     it 'saves the models' do
@@ -29,7 +29,7 @@ describe Groups::ImportExport::ExportService do
         service.execute
 
         expect(group.import_export_upload.export_file.file).not_to be_nil
-        expect(File.directory?(export_path)).to eq(false)
+        expect(File.directory?(archive_path)).to eq(false)
         expect(File.exist?(shared.archive_path)).to eq(false)
       end
     end
@@ -46,25 +46,42 @@ describe Groups::ImportExport::ExportService do
       end
     end
 
-    context 'when saving services fail' do
-      before do
-        allow(service).to receive_message_chain(:tree_exporter, :save).and_return(false)
+    context 'when export fails' do
+      context 'when file saver fails' do
+        it 'removes the remaining exported data' do
+          allow_next_instance_of(Gitlab::ImportExport::Saver) do |saver|
+            allow(saver).to receive(:save).and_return(false)
+          end
+
+          expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+
+          expect(group.import_export_upload).to be_nil
+          expect(File.exist?(shared.archive_path)).to eq(false)
+        end
       end
 
-      it 'removes the remaining exported data' do
-        allow_any_instance_of(Gitlab::ImportExport::Saver).to receive(:compress_and_save).and_return(false)
+      context 'when file compression fails' do
+        before do
+          allow(service).to receive_message_chain(:tree_exporter, :save).and_return(false)
+        end
 
-        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+        it 'removes the remaining exported data' do
+          allow_next_instance_of(Gitlab::ImportExport::Saver) do |saver|
+            allow(saver).to receive(:compress_and_save).and_return(false)
+          end
 
-        expect(group.import_export_upload).to be_nil
-        expect(File.directory?(export_path)).to eq(false)
-        expect(File.exist?(shared.archive_path)).to eq(false)
-      end
+          expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+
+          expect(group.import_export_upload).to be_nil
+          expect(File.exist?(shared.archive_path)).to eq(false)
+        end
 
-      it 'notifies logger' do
-        expect_any_instance_of(Gitlab::Import::Logger).to receive(:error)
+        it 'notifies logger' do
+          allow(service).to receive_message_chain(:tree_exporter, :save).and_return(false)
+          expect(shared.logger).to receive(:error)
 
-        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+          expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+        end
       end
     end
   end

spec/services/projects/import_export/export_service_spec.rb

@@ -91,10 +91,10 @@ describe Projects::ImportExport::ExportService do
         end
 
         it 'removes the remaining exported data' do
-          allow(shared).to receive(:export_path).and_return('whatever')
+          allow(shared).to receive(:archive_path).and_return('whatever')
           allow(FileUtils).to receive(:rm_rf)
 
-          expect(FileUtils).to receive(:rm_rf).with(shared.export_path)
+          expect(FileUtils).to receive(:rm_rf).with(shared.archive_path)
         end
 
         it 'notifies the user' do
@@ -121,10 +121,10 @@ describe Projects::ImportExport::ExportService do
       end
 
       it 'removes the remaining exported data' do
-        allow(shared).to receive(:export_path).and_return('whatever')
+        allow(shared).to receive(:archive_path).and_return('whatever')
         allow(FileUtils).to receive(:rm_rf)
 
-        expect(FileUtils).to receive(:rm_rf).with(shared.export_path)
+        expect(FileUtils).to receive(:rm_rf).with(shared.archive_path)
       end
 
       it 'notifies the user' do
@@ -142,6 +142,21 @@ describe Projects::ImportExport::ExportService do
       end
     end
 
+    context 'when one of the savers fail unexpectedly' do
+      let(:archive_path) { shared.archive_path }
+
+      before do
+        allow(service).to receive_message_chain(:uploads_saver, :save).and_return(false)
+      end
+
+      it 'removes the remaining exported data' do
+        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+
+        expect(project.import_export_upload).to be_nil
+        expect(File.exist?(shared.archive_path)).to eq(false)
+      end
+    end
+
     context 'when user does not have admin_project permission' do
       let!(:another_user) { create(:user) }