Merge branch 'curd-auto-merge-in-transaction' into 'master'

Wrap auto merge parameters update in database transaction

See merge request gitlab-org/gitlab!33471

app/services/auto_merge/base_service.rb

@@ -6,19 +6,18 @@ module AutoMerge
     include MergeRequests::AssignsMergeParams
 
     def execute(merge_request)
-      assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy))
-
-      merge_request.auto_merge_enabled = true
-      merge_request.merge_user = current_user
-
-      return :failed unless merge_request.save
-
-      yield if block_given?
+      ActiveRecord::Base.transaction do
+        register_auto_merge_parameters!(merge_request)
+        yield if block_given?
+      end
 
       # Notify the event that auto merge is enabled or merge param is updated
       AutoMergeProcessWorker.perform_async(merge_request.id)
 
       strategy.to_sym
+    rescue => e
+      track_exception(e, merge_request)
+      :failed
     end
 
     def update(merge_request)
@@ -30,23 +29,27 @@ module AutoMerge
     end
 
     def cancel(merge_request)
-      if clear_auto_merge_parameters(merge_request)
+      ActiveRecord::Base.transaction do
+        clear_auto_merge_parameters!(merge_request)
         yield if block_given?
-
-        success
-      else
-        error("Can't cancel the automatic merge", 406)
       end
+
+      success
+    rescue => e
+      track_exception(e, merge_request)
+      error("Can't cancel the automatic merge", 406)
     end
 
     def abort(merge_request, reason)
-      if clear_auto_merge_parameters(merge_request)
+      ActiveRecord::Base.transaction do
+        clear_auto_merge_parameters!(merge_request)
         yield if block_given?
-
-        success
-      else
-        error("Can't abort the automatic merge", 406)
       end
+
+      success
+    rescue => e
+      track_exception(e, merge_request)
+      error("Can't abort the automatic merge", 406)
     end
 
     def available_for?(merge_request)
@@ -65,7 +68,14 @@ module AutoMerge
       end
     end
 
-    def clear_auto_merge_parameters(merge_request)
+    def register_auto_merge_parameters!(merge_request)
+      assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy))
+      merge_request.auto_merge_enabled = true
+      merge_request.merge_user = current_user
+      merge_request.save!
+    end
+
+    def clear_auto_merge_parameters!(merge_request)
       merge_request.auto_merge_enabled = false
       merge_request.merge_user = nil
 
@@ -76,7 +86,11 @@ module AutoMerge
         'auto_merge_strategy'
       )
 
-      merge_request.save
+      merge_request.save!
+    end
+
+    def track_exception(error, merge_request)
+      Gitlab::ErrorTracking.track_exception(error, merge_request_id: merge_request&.id)
     end
   end
 end

changelogs/unreleased/curd-auto-merge-in-transaction.yml

+---
+title: Wrap auto merge parameters update in database transaction
+merge_request: 33471
+author:
+type: fixed

ee/spec/services/auto_merge/merge_train_service_spec.rb

@@ -58,13 +58,39 @@ describe AutoMerge::MergeTrainService do
 
     context 'when failed to save the record' do
       before do
-        allow(merge_request).to receive(:save) { false }
+        allow(merge_request).to receive(:save!) { raise PG::QueryCanceled.new }
       end
 
       it 'returns result code' do
         is_expected.to eq(:failed)
       end
     end
+
+    context 'when statement timeout happened on system note creation' do
+      before do
+        allow(SystemNoteService).to receive(:merge_train) { raise PG::QueryCanceled.new }
+      end
+
+      it 'returns failed status' do
+        is_expected.to eq(:failed)
+      end
+
+      it 'rollback the transaction' do
+        expect { subject }.not_to change { Note.count }
+
+        merge_request.reload
+        expect(merge_request).not_to be_auto_merge_enabled
+        expect(merge_request.merge_train).not_to be_present
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(PG::QueryCanceled),
+                                             merge_request_id: merge_request.id)
+
+        subject
+      end
+    end
   end
 
   describe '#process' do
@@ -187,6 +213,33 @@ describe AutoMerge::MergeTrainService do
         end
       end
     end
+
+    context 'when statement timeout happened on system note creation' do
+      before do
+        allow(SystemNoteService).to receive(:cancel_merge_train) { raise PG::QueryCanceled.new }
+      end
+
+      it 'returns error' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq("Can't cancel the automatic merge")
+      end
+
+      it 'rollback the transaction' do
+        expect { subject }.not_to change { Note.count }
+
+        merge_request.reload
+        expect(merge_request).to be_auto_merge_enabled
+        expect(merge_request.merge_train).to be_present
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(PG::QueryCanceled),
+                                             merge_request_id: merge_request.id)
+
+        subject
+      end
+    end
   end
 
   describe '#abort' do
@@ -246,6 +299,33 @@ describe AutoMerge::MergeTrainService do
         end
       end
     end
+
+    context 'when statement timeout happened on system note creation' do
+      before do
+        allow(SystemNoteService).to receive(:abort_merge_train) { raise PG::QueryCanceled.new }
+      end
+
+      it 'returns error' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq("Can't abort the automatic merge")
+      end
+
+      it 'rollback the transaction' do
+        expect { subject }.not_to change { Note.count }
+
+        merge_request.reload
+        expect(merge_request).to be_auto_merge_enabled
+        expect(merge_request.merge_train).to be_present
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(PG::QueryCanceled),
+                                             merge_request_id: merge_request.id)
+
+        subject
+      end
+    end
   end
 
   describe '#available_for?' do

spec/services/auto_merge/base_service_spec.rb

@@ -82,9 +82,9 @@ describe AutoMerge::BaseService do
       end
     end
 
-    context 'when failed to save' do
+    context 'when failed to save merge request' do
       before do
-        allow(merge_request).to receive(:save) { false }
+        allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new }
       end
 
       it 'does not yield block' do
@@ -94,6 +94,39 @@ describe AutoMerge::BaseService do
       it 'returns failed' do
         is_expected.to eq(:failed)
       end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(ActiveRecord::RecordInvalid),
+                                             merge_request_id: merge_request.id)
+
+        subject
+      end
+    end
+
+    context 'when exception happens in yield block' do
+      def execute_with_error_in_yield
+        service.execute(merge_request) { raise 'Something went wrong' }
+      end
+
+      it 'returns failed status' do
+        expect(execute_with_error_in_yield).to eq(:failed)
+      end
+
+      it 'rollback the transaction' do
+        execute_with_error_in_yield
+
+        merge_request.reload
+        expect(merge_request).not_to be_auto_merge_enabled
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(RuntimeError),
+                                             merge_request_id: merge_request.id)
+
+        execute_with_error_in_yield
+      end
     end
   end
 
@@ -162,7 +195,7 @@ describe AutoMerge::BaseService do
 
     context 'when failed to save' do
       before do
-        allow(merge_request).to receive(:save) { false }
+        allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new }
       end
 
       it 'does not yield block' do
@@ -178,9 +211,9 @@ describe AutoMerge::BaseService do
 
     it_behaves_like 'Canceled or Dropped'
 
-    context 'when failed to save' do
+    context 'when failed to save merge request' do
       before do
-        allow(merge_request).to receive(:save) { false }
+        allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new }
       end
 
       it 'returns error status' do
@@ -188,6 +221,33 @@ describe AutoMerge::BaseService do
         expect(subject[:message]).to eq("Can't cancel the automatic merge")
       end
     end
+
+    context 'when exception happens in yield block' do
+      def cancel_with_error_in_yield
+        service.cancel(merge_request) { raise 'Something went wrong' }
+      end
+
+      it 'returns error' do
+        result = cancel_with_error_in_yield
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Can't cancel the automatic merge")
+      end
+
+      it 'rollback the transaction' do
+        cancel_with_error_in_yield
+
+        merge_request.reload
+        expect(merge_request).to be_auto_merge_enabled
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(RuntimeError),
+                                             merge_request_id: merge_request.id)
+
+        cancel_with_error_in_yield
+      end
+    end
   end
 
   describe '#abort' do
@@ -200,7 +260,7 @@ describe AutoMerge::BaseService do
 
     context 'when failed to save' do
       before do
-        allow(merge_request).to receive(:save) { false }
+        allow(merge_request).to receive(:save!) { raise ActiveRecord::RecordInvalid.new }
       end
 
       it 'returns error status' do
@@ -208,5 +268,32 @@ describe AutoMerge::BaseService do
         expect(subject[:message]).to eq("Can't abort the automatic merge")
       end
     end
+
+    context 'when exception happens in yield block' do
+      def abort_with_error_in_yield
+        service.abort(merge_request, reason) { raise 'Something went wrong' }
+      end
+
+      it 'returns error' do
+        result = abort_with_error_in_yield
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Can't abort the automatic merge")
+      end
+
+      it 'rollback the transaction' do
+        abort_with_error_in_yield
+
+        merge_request.reload
+        expect(merge_request).to be_auto_merge_enabled
+      end
+
+      it 'tracks the exception' do
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception).with(kind_of(RuntimeError),
+                                             merge_request_id: merge_request.id)
+
+        abort_with_error_in_yield
+      end
+    end
   end
 end