Refactor: Split UpdateBuildsMinute Service

We want to cancel pipelines before deleting them because minutes are not accumulated on deleted jobs that continue to run.  In order to cancel pipelines before deleting them and have minutes accumulate properly, we need to compute the minutes synchronously before the destroy to pass of to an async worker that can do the rest.

This change breaks apart the UpdateBuildsMinute service into 2 so that part of the logic can be executed async. There are no changes to the existing behavior in this code.

config/puma_actioncable.rb

+# frozen_string_literal: true
+
+# -----------------------------------------------------------------------
+# This file is used by the GDK to generate a default config/puma_actioncable.rb file
+# Note that `/Users/allisonbrowne/gitlab/gdk` will be substituted for the actual GDK root
+# directory when this file is generated
+# -----------------------------------------------------------------------
+
+# Load "path" as a rackup file.
+#
+# The default is "cable/config.ru".
+#
+rackup 'cable/config.ru'
+pidfile '/Users/allisonbrowne/gitlab/gdk/gitlab/tmp/pids/puma_actioncable.pid'
+state_path '/Users/allisonbrowne/gitlab/gdk/gitlab/tmp/pids/puma_actioncable.state'
+
+## Uncomment the lines if you would like to write puma stdout & stderr streams
+## to a different location than rails logs.
+## When using GitLab Development Kit, by default, these logs will be consumed
+## by runit and can be accessed using `gdk tail rails-actioncable`
+# stdout_redirect '/Users/allisonbrowne/gitlab/gdk/gitlab/log/puma_actioncable.stdout.log',
+#  '/Users/allisonbrowne/gitlab/gdk/gitlab/log/puma_actioncable.stderr.log',
+#  true
+
+# Configure "min" to be the minimum number of threads to use to answer
+# requests and "max" the maximum.
+#
+# The default is "0, 16".
+#
+threads 1, 4
+
+# By default, workers accept all requests and queue them to pass to handlers.
+# When false, workers accept the number of simultaneous requests configured.
+#
+# Queueing requests generally improves performance, but can cause deadlocks if
+# the app is waiting on a request to itself. See https://github.com/puma/puma/issues/612
+#
+# When set to false this may require a reverse proxy to handle slow clients and
+# queue requests before they reach puma. This is due to disabling HTTP keepalive
+queue_requests false
+
+# Bind the server to "url". "tcp://", "unix://" and "ssl://" are the only
+# accepted protocols.
+bind 'unix:///Users/allisonbrowne/gitlab/gdk/gitlab_actioncable.socket'
+
+workers 1
+
+require_relative "/Users/allisonbrowne/gitlab/gdk/gitlab/lib/gitlab/cluster/lifecycle_events"
+
+on_restart do
+  # Signal application hooks that we're about to restart
+  Gitlab::Cluster::LifecycleEvents.do_before_master_restart
+end
+
+before_fork do
+  # Signal to the puma killer
+  Gitlab::Cluster::PumaWorkerKillerInitializer.start @config.options unless ENV['DISABLE_PUMA_WORKER_KILLER']
+
+  # Signal application hooks that we're about to fork
+  Gitlab::Cluster::LifecycleEvents.do_before_fork
+end
+
+Gitlab::Cluster::LifecycleEvents.set_puma_options @config.options
+on_worker_boot do
+  # Signal application hooks of worker start
+  Gitlab::Cluster::LifecycleEvents.do_worker_start
+end
+
+# Preload the application before starting the workers; this conflicts with
+# phased restart feature. (off by default)
+
+preload_app!
+
+tag 'gitlab-actioncable-puma-worker'
+
+# Verifies that all workers have checked in to the master process within
+# the given timeout. If not the worker process will be restarted. Default
+# value is 60 seconds.
+#
+worker_timeout 60
+
+# https://github.com/puma/puma/blob/master/5.0-Upgrade.md#lower-latency-better-throughput
+wait_for_less_busy_worker ENV.fetch('PUMA_WAIT_FOR_LESS_BUSY_WORKER', 0.001).to_f
+
+# https://github.com/puma/puma/blob/master/5.0-Upgrade.md#nakayoshi_fork
+nakayoshi_fork unless ENV['DISABLE_PUMA_NAKAYOSHI_FORK'] == 'true'
+
+# Use json formatter
+require_relative "/Users/allisonbrowne/gitlab/gdk/gitlab/lib/gitlab/puma_logging/json_formatter"
+
+json_formatter = Gitlab::PumaLogging::JSONFormatter.new
+log_formatter do |str|
+  json_formatter.call(str)
+end

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

@@ -3,6 +3,9 @@
 module Ci
   module Minutes
     class UpdateBuildMinutesService < BaseService
+      # Calculates consumption and updates the project and namespace minutes based on the passed build
+      # Calculating the consumption syncronously before triggering an async job to update
+      # ensures the pipeline data still exists in the case where minutes are updated prior to pipeline deletion
       def execute(build)
         return unless build.shared_runners_minutes_limit_enabled?
         return unless build.complete?
@@ -10,45 +13,19 @@ module Ci
 
         consumption = ::Gitlab::Ci::Minutes::BuildConsumption.new(build, build.duration).amount
 
-        return unless consumption > 0
-
-        consumption_in_seconds = consumption.minutes.to_i
-        legacy_track_usage_of_monthly_minutes(consumption_in_seconds)
-
-        track_usage_of_monthly_minutes(consumption)
-
-        send_minutes_email_notification
+        Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute(consumption)
+        # TODO(Issue #335338): Introduce async worker UpdateMinutesByConsumptionWorker, example:
+        # if ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml)
+        #   Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute_async(consumption)
+        # else
+        #   Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute(consumption)
+        # end
 
         compare_with_live_consumption(build, consumption)
       end
 
       private
 
-      def send_minutes_email_notification
-        # `perform reset` on `project` because otherwise `Namespace#namespace_statistics` will return stale data.
-        ::Ci::Minutes::EmailNotificationService.new(@project.reset).execute if ::Gitlab.com?
-      end
-
-      def legacy_track_usage_of_monthly_minutes(consumption)
-        ProjectStatistics.update_counters(project_statistics,
-          shared_runners_seconds: consumption)
-
-        NamespaceStatistics.update_counters(namespace_statistics,
-          shared_runners_seconds: consumption)
-      end
-
-      def track_usage_of_monthly_minutes(consumption)
-        return unless Feature.enabled?(:ci_minutes_monthly_tracking, project, default_enabled: :yaml)
-
-        namespace_usage = ::Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace)
-        project_usage = ::Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project)
-
-        ActiveRecord::Base.transaction do
-          ::Ci::Minutes::NamespaceMonthlyUsage.increase_usage(namespace_usage, consumption)
-          ::Ci::Minutes::ProjectMonthlyUsage.increase_usage(project_usage, consumption)
-        end
-      end
-
       def compare_with_live_consumption(build, consumption)
         live_consumption = ::Ci::Minutes::TrackLiveConsumptionService.new(build).live_consumption
         return if live_consumption == 0
@@ -57,14 +34,6 @@ module Ci
         observe_ci_minutes_difference(difference, plan: namespace.actual_plan_name)
       end
 
-      def namespace_statistics
-        namespace.namespace_statistics || namespace.create_namespace_statistics
-      end
-
-      def project_statistics
-        project.statistics || project.create_statistics(namespace: project.namespace)
-      end
-
       def namespace
         project.shared_runners_limit_namespace
       end

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

+# frozen_string_literal: true
+
+module Ci
+  module Minutes
+    class UpdateMinutesByConsumptionService
+      def initialize(project, namespace)
+        @project = project
+        @namespace = namespace
+      end
+
+      # Updates the project and namespace minutes based on the passed consumption amount
+      def execute(consumption)
+        return unless consumption > 0
+
+        consumption_in_seconds = consumption.minutes.to_i
+        legacy_track_usage_of_monthly_minutes(consumption_in_seconds)
+
+        track_usage_of_monthly_minutes(consumption)
+        send_email_notification
+      end
+
+      private
+
+      def send_email_notification
+        # `perform reset` on `project` because `Namespace#namespace_statistics` will otherwise return stale data.
+        ::Ci::Minutes::EmailNotificationService.new(@project.reset).execute if ::Gitlab.com?
+      end
+
+      def legacy_track_usage_of_monthly_minutes(consumption_in_seconds)
+        ProjectStatistics.update_counters(project_statistics,
+          shared_runners_seconds: consumption_in_seconds)
+
+        NamespaceStatistics.update_counters(namespace_statistics,
+          shared_runners_seconds: consumption_in_seconds)
+      end
+
+      def track_usage_of_monthly_minutes(consumption)
+        return unless Feature.enabled?(:ci_minutes_monthly_tracking, @project, default_enabled: :yaml)
+
+        namespace_usage = ::Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(@namespace)
+        project_usage = ::Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(@project)
+
+        ActiveRecord::Base.transaction do
+          ::Ci::Minutes::NamespaceMonthlyUsage.increase_usage(namespace_usage, consumption)
+          ::Ci::Minutes::ProjectMonthlyUsage.increase_usage(project_usage, consumption)
+        end
+      end
+
+      def namespace_statistics
+        @namespace.namespace_statistics || @namespace.create_namespace_statistics
+      end
+
+      def project_statistics
+        @project.statistics || @project.create_statistics(namespace: @project.namespace)
+      end
+    end
+  end
+end

ee/lib/gitlab/ci/minutes/build_consumption.rb

@@ -20,7 +20,7 @@ module Gitlab
         private
 
         def cost_factor
-          @build.runner.minutes_cost_factor(project_visibility_level)
+          @build&.runner&.minutes_cost_factor(project_visibility_level)
         end
 
         def project_visibility_level

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::Minutes::UpdateMinutesByConsumptionService do
+  describe '#execute' do
+    let_it_be(:namespace) { create(:namespace, shared_runners_minutes_limit: 100) }
+    let_it_be(:project) { create(:project, :private, namespace: namespace) }
+
+    let(:consumption_minutes) { 120 }
+    let(:consumption_seconds) { 120 * 60 }
+    let(:namespace_amount_used) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace).amount_used }
+    let(:project_amount_used) { Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project).amount_used }
+
+    subject { described_class.new(project, namespace).execute(consumption_minutes) }
+
+    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
+
+          expect(project.statistics.reload.shared_runners_seconds)
+            .to eq(consumption_seconds)
+
+          expect(namespace.namespace_statistics.reload.shared_runners_seconds)
+            .to eq(consumption_seconds)
+        end
+
+        it 'updates monthly usage with consumption minutes' do
+          subject
+
+          expect(namespace_amount_used).to eq(consumption_minutes)
+          expect(project_amount_used).to eq(consumption_minutes)
+        end
+
+        context 'when feature flag ci_minutes_monthly_tracking is disabled' do
+          before do
+            stub_feature_flags(ci_minutes_monthly_tracking: false)
+          end
+
+          it 'does not update the usage on a monthly basis' do
+            subject
+
+            expect(namespace_amount_used).to eq(0)
+            expect(project_amount_used).to eq(0)
+          end
+        end
+      end
+
+      context 'when statistics and usage have existing values' do
+        let(:namespace) { create(:namespace, shared_runners_minutes_limit: 100) }
+        let(:project) { create(:project, :private, namespace: namespace) }
+        let(:existing_usage_in_seconds) { 100 }
+        let(:existing_usage_in_minutes) { (100.to_f / 60).round(2) }
+
+        before do
+          project.statistics.update!(shared_runners_seconds: existing_usage_in_seconds)
+          namespace.create_namespace_statistics(shared_runners_seconds: existing_usage_in_seconds)
+          create(:ci_namespace_monthly_usage, namespace: namespace, amount_used: existing_usage_in_minutes)
+          create(:ci_project_monthly_usage, project: project, amount_used: existing_usage_in_minutes)
+        end
+
+        it 'updates legacy statistics with consumption seconds' do
+          subject
+
+          expect(project.statistics.reload.shared_runners_seconds)
+            .to eq(existing_usage_in_seconds + consumption_seconds)
+
+          expect(namespace.namespace_statistics.reload.shared_runners_seconds)
+            .to eq(existing_usage_in_seconds + consumption_seconds)
+        end
+
+        it 'updates monthly usage with consumption minutes' do
+          subject
+
+          expect(namespace_amount_used).to eq(existing_usage_in_minutes + consumption_minutes)
+          expect(project_amount_used).to eq(existing_usage_in_minutes + consumption_minutes)
+        end
+
+        context 'when feature flag ci_minutes_monthly_tracking is disabled' do
+          before do
+            stub_feature_flags(ci_minutes_monthly_tracking: false)
+          end
+
+          it 'does not update usage' do
+            subject
+
+            expect(namespace_amount_used).to eq(existing_usage_in_minutes)
+            expect(project_amount_used).to eq(existing_usage_in_minutes)
+          end
+        end
+      end
+    end
+  end
+end