Merge branch 'fix/gb/remove-subtransactions-ci-minutes-calculation' into 'master'

Do not use subtransactions when updating ci minutes usage

See merge request gitlab-org/gitlab!68471

ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb

@@ -15,6 +15,9 @@ module Ci
       # Updates the project and namespace usage based on the passed consumption amount
       def execute(consumption)
         legacy_track_usage_of_monthly_minutes(consumption)
+
+        preload_minutes_usage_data!
+
         ApplicationRecord.transaction do
           track_usage_of_monthly_minutes(consumption)
 
@@ -24,6 +27,13 @@ module Ci
 
       private
 
+      def preload_minutes_usage_data!
+        return unless monthly_tracking_enabled?
+
+        project_usage
+        namespace_usage
+      end
+
       def send_minutes_email_notification
         # `perform reset` on `project` because `Namespace#namespace_statistics` will otherwise return stale data.
         # TODO(issue 335885): Remove @project
@@ -38,8 +48,7 @@ module Ci
       end
 
       def track_usage_of_monthly_minutes(consumption)
-        # TODO(issue 335885): Remove @project
-        return unless Feature.enabled?(:ci_minutes_monthly_tracking, @project, default_enabled: :yaml)
+        return unless monthly_tracking_enabled?
 
         ::Ci::Minutes::NamespaceMonthlyUsage.increase_usage(namespace_usage, consumption) if namespace_usage
         ::Ci::Minutes::ProjectMonthlyUsage.increase_usage(project_usage, consumption) if project_usage
@@ -83,6 +92,11 @@ module Ci
         rescue ActiveRecord::NotNullViolation, ActiveRecord::RecordInvalid
         end
       end
+
+      def monthly_tracking_enabled?
+        # TODO(issue 335885): Remove @project
+        Feature.enabled?(:ci_minutes_monthly_tracking, @project, default_enabled: :yaml)
+      end
     end
   end
 end

ee/spec/services/ci/minutes/update_project_and_namespace_usage_service_spec.rb

@@ -3,21 +3,20 @@
 require 'spec_helper'
 
 RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
-  let_it_be(:project) { create(:project, :private) }
-  let_it_be(:namespace) { project.namespace }
-
+  let(:project) { create(:project, :private) }
+  let(:namespace) { project.namespace }
   let(:consumption_minutes) { 120 }
   let(:consumption_seconds) { consumption_minutes * 60 }
   let(:namespace_amount_used) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace_id: namespace.id).amount_used }
   let(:project_amount_used) { Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project_id: project.id).amount_used }
 
   describe '#execute' do
-    subject { described_class.new(project.id, namespace.id).execute(consumption_minutes) }
+    subject { described_class.new(project.id, namespace.id) }
 
     context 'with shared runner' do
       context 'when statistics and usage do not have existing values' do
         it 'updates legacy statistics with consumption seconds' do
-          subject
+          subject.execute(consumption_minutes)
 
           expect(project.statistics.reload.shared_runners_seconds)
             .to eq(consumption_seconds)
@@ -28,9 +27,10 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
 
         context 'when project deleted' do
           let(:project) { double(id: non_existing_record_id) }
+          let(:namespace) { create(:namespace) }
 
           it 'will complete successfully and increment namespace statistics' do
-            subject
+            subject.execute(consumption_minutes)
 
             expect(ProjectStatistics.find_by_project_id(project.id)).to be_nil
             expect(NamespaceStatistics.find_by_namespace_id(namespace.id).shared_runners_seconds).to eq(consumption_seconds)
@@ -43,7 +43,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           let(:namespace) { double(id: non_existing_record_id) }
 
           it 'will complete successfully' do
-            subject
+            subject.execute(consumption_minutes)
 
             expect(ProjectStatistics.find_by_project_id(project.id).shared_runners_seconds).to eq(consumption_seconds)
             expect(NamespaceStatistics.find_by_namespace_id(namespace.id)).to be_nil
@@ -57,7 +57,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           let(:namespace) { double(id: non_existing_record_id) }
 
           it 'will complete successfully' do
-            subject
+            subject.execute(consumption_minutes)
 
             expect(ProjectStatistics.find_by_project_id(project.id)).to be_nil
             expect(NamespaceStatistics.find_by_namespace_id(namespace.id)).to be_nil
@@ -67,7 +67,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
         end
 
         it 'updates monthly usage with consumption minutes' do
-          subject
+          subject.execute(consumption_minutes)
 
           expect(namespace_amount_used).to eq(consumption_minutes)
           expect(project_amount_used).to eq(consumption_minutes)
@@ -79,7 +79,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           end
 
           it 'does not update the usage on a monthly basis' do
-            subject
+            subject.execute(consumption_minutes)
 
             expect(namespace_amount_used).to eq(0)
             expect(project_amount_used).to eq(0)
@@ -96,7 +96,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
               expect(service).to receive(:execute)
             end
 
-            subject
+            subject.execute(consumption_minutes)
           end
         end
 
@@ -108,7 +108,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           it 'does not send a minute notification email' do
             expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
 
-            subject
+            subject.execute(consumption_minutes)
           end
         end
       end
@@ -126,8 +126,24 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           create(:ci_project_monthly_usage, project: project, amount_used: existing_usage_in_minutes)
         end
 
+        it 'does not create nested transactions', :delete do
+          expect(ApplicationRecord.connection.transaction_open?).to be false
+
+          service = described_class.new(project.id, namespace.id)
+
+          queries = ActiveRecord::QueryRecorder.new do
+            service.execute(consumption_minutes)
+          end
+
+          savepoints = queries.occurrences.keys.flatten.select do |query|
+            query.include?('SAVEPOINT')
+          end
+
+          expect(savepoints).to be_empty
+        end
+
         it 'updates legacy statistics with consumption seconds' do
-          subject
+          subject.execute(consumption_minutes)
 
           expect(project.statistics.reload.shared_runners_seconds)
             .to eq(existing_usage_in_seconds + consumption_seconds)
@@ -137,7 +153,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
         end
 
         it 'updates monthly usage with consumption minutes' do
-          subject
+          subject.execute(consumption_minutes)
 
           expect(namespace_amount_used).to eq(existing_usage_in_minutes + consumption_minutes)
           expect(project_amount_used).to eq(existing_usage_in_minutes + consumption_minutes)
@@ -149,7 +165,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
           end
 
           it 'does not update usage' do
-            subject
+            subject.execute(consumption_minutes)
 
             expect(namespace_amount_used).to eq(existing_usage_in_minutes)
             expect(project_amount_used).to eq(existing_usage_in_minutes)