Use state machine for pipeline event processing

app/models/ci/pipeline.rb

@@ -19,6 +19,37 @@ module Ci
     after_save :keep_around_commits
+    state_machine :status, initial: :created do
+      event :skip do
+        transition any => :skipped
+      end
+
+      event :drop do
+        transition any => :failed
+      end
+
+      event :update_status do
+        transition any => :pending, if: ->(pipeline) { pipeline.can_transition_to?('pending') }
+        transition any => :running, if: ->(pipeline) { pipeline.can_transition_to?('running') }
+        transition any => :failed, if: ->(pipeline) { pipeline.can_transition_to?('failed') }
+        transition any => :success, if: ->(pipeline) { pipeline.can_transition_to?('success') }
+        transition any => :canceled, if: ->(pipeline) { pipeline.can_transition_to?('canceled') }
+        transition any => :skipped, if: ->(pipeline) { pipeline.can_transition_to?('skipped') }
+      end
+
+      after_transition [:created, :pending] => :running do |pipeline|
+        pipeline.update(started_at: Time.now)
+      end
+
+      after_transition any => [:success, :failed, :canceled] do |pipeline|
+        pipeline.update(finished_at: Time.now)
+      end
+
+      after_transition do |pipeline|
+        pipeline.update_duration
+      end
+    end
+
     # ref can't be HEAD or SHA, can only be branch/tag name
     scope :latest_successful_for, ->(ref = default_branch) do
       where(ref: ref).success.order(id: :desc).limit(1)
@@ -89,16 +120,12 @@ module Ci
     def cancel_running
       builds.running_or_pending.each(&:cancel)
-
-      reload_status!
     end
     def retry_failed(user)
       builds.latest.failed.select(&:retryable?).each do |build|
         Ci::Build.retry(build, user)
       end
-
-      reload_status!
     end
     def latest?
@@ -185,8 +212,6 @@ module Ci
     def process!
       Ci::ProcessPipelineService.new(project, user).execute(self)
-
-      reload_status!
     end
     def predefined_variables
@@ -195,22 +220,22 @@ module Ci
       ]
     end
-    def reload_status!
-      reload
-      self.status =
-        if yaml_errors.blank?
-          statuses.latest.status || 'skipped'
-        else
-          'failed'
-        end
-      self.started_at = statuses.started_at
-      self.finished_at = statuses.finished_at
-      self.duration = statuses.latest.duration
-      save
+    def can_transition_to?(expected_status)
+      latest_status == expected_status
+    end
+
+    def update_duration
+      update(duration: statuses.latest.duration)
     end
     private
+    def latest_status
+      return 'failed' unless yaml_errors.blank?
+
+      statuses.latest.status || 'skipped'
+    end
+
     def keep_around_commits
       return unless project

app/models/commit_status.rb

@@ -74,13 +74,13 @@ class CommitStatus < ActiveRecord::Base
     around_transition any => [:success, :failed, :canceled] do |commit_status, block|
       block.call
-      commit_status.pipeline.process! if commit_status.pipeline
+      commit_status.pipeline.try(:process!)
     end
-    around_transition any => [:pending, :running] do |commit_status, block|
-      block.call
-      commit_status.pipeline.reload_status! if commit_status.pipeline
+    # Try to update the pipeline status
+    after_transition do |commit_status, transition|
+      commit_status.pipeline.try(:update_status) unless transition.loopback?
     end
   end

app/services/ci/create_pipeline_service.rb

@@ -37,7 +37,8 @@ module Ci
       end
       if !ignore_skip_ci && skip_ci?
-        return error('Creation of pipeline is skipped', save: save_on_errors)
+        pipeline.skip if save_on_errors
+        return pipeline
       end
       unless pipeline.config_builds_attributes.present?
@@ -93,7 +94,7 @@ module Ci
     def error(message, save: false)
       pipeline.errors.add(:base, message)
-      pipeline.reload_status! if save
+      pipeline.drop if save
       pipeline
     end
   end

features/steps/shared/builds.rb

@@ -12,7 +12,6 @@ module SharedBuilds
   step 'project has a recent build' do
     @pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master')
     @build = create(:ci_build_with_coverage, pipeline: @pipeline)
-    @pipeline.reload_status!
   end
   step 'recent build is successful' do
@@ -25,7 +24,6 @@ module SharedBuilds
   step 'project has another build that is running' do
     create(:ci_build, pipeline: @pipeline, name: 'second build', status: 'running')
-    @pipeline.reload_status!
   end
   step 'I visit recent build details page' do

spec/features/pipelines_spec.rb

@@ -34,7 +34,6 @@ describe "Pipelines" do
       let!(:running) { create(:ci_build, :running, pipeline: pipeline, stage: 'test', commands: 'test') }
       before do
-        pipeline.reload_status!
         visit namespace_project_pipelines_path(project.namespace, project)
       end
@@ -53,7 +52,6 @@ describe "Pipelines" do
       let!(:failed) { create(:ci_build, :failed, pipeline: pipeline, stage: 'test', commands: 'test') }
       before do
-        pipeline.reload_status!
         visit namespace_project_pipelines_path(project.namespace, project)
       end
@@ -87,7 +85,6 @@ describe "Pipelines" do
         let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') }
         before do
-          pipeline.reload_status!
           visit namespace_project_pipelines_path(project.namespace, project)
         end
@@ -104,7 +101,6 @@ describe "Pipelines" do
         let!(:failed) { create(:generic_commit_status, status: 'failed', pipeline: pipeline, stage: 'test') }
         before do
-          pipeline.reload_status!
           visit namespace_project_pipelines_path(project.namespace, project)
         end

spec/lib/ci/charts_spec.rb

@@ -5,7 +5,6 @@ describe Ci::Charts, lib: true do
     before do
       @pipeline = FactoryGirl.create(:ci_pipeline)
       FactoryGirl.create(:ci_build, pipeline: @pipeline)
-      @pipeline.reload_status!
     end
     it 'returns build times in minutes' do

spec/models/ci/pipeline_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 describe Ci::Pipeline, models: true do
   let(:project) { FactoryGirl.create :empty_project }
-  let(:pipeline) { FactoryGirl.create :ci_pipeline, project: project }
+  let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
   it { is_expected.to belong_to(:project) }
   it { is_expected.to belong_to(:user) }
@@ -51,25 +51,6 @@ describe Ci::Pipeline, models: true do
     end
   end
-  describe "#finished_at" do
-    let(:pipeline) { FactoryGirl.create :ci_pipeline }
-
-    it "returns finished_at of latest build" do
-      build = FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 60
-      FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 120
-      pipeline.reload_status!
-
-      expect(pipeline.finished_at.to_i).to eq(build.finished_at.to_i)
-    end
-
-    it "returns nil if there is no finished build" do
-      FactoryGirl.create :ci_not_started_build, pipeline: pipeline
-      pipeline.reload_status!
-
-      expect(pipeline.finished_at).to be_nil
-    end
-  end
-
   describe "coverage" do
     let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" }
     let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
@@ -139,31 +120,20 @@ describe Ci::Pipeline, models: true do
     end
   end
-  describe '#reload_status!' do
+  describe '#update_counters' do
     let(:pipeline) { create :ci_empty_pipeline, project: project }
-    context 'dependent objects' do
-      let(:commit_status) { create :commit_status, :pending, pipeline: pipeline }
-
-      it 'executes reload_status! after succeeding dependent object' do
-        expect(pipeline).to receive(:reload_status!).and_return(true)
-
-        commit_status.success
-      end
-    end
-
     context 'updates' do
       let(:current) { Time.now.change(usec: 0) }
       let(:build) { FactoryGirl.create :ci_build, pipeline: pipeline, started_at: current - 120, finished_at: current - 60 }
       before do
-        build
-        pipeline.reload_status!
+        build.skip
       end
       [:status, :started_at, :finished_at, :duration].each do |param|
         it "#{param}" do
-          expect(pipeline.send(param)).to eq(build.send(param))
+          expect(pipeline.reload.send(param)).to eq(build.send(param))
         end
       end
     end

spec/requests/api/builds_spec.rb

@@ -238,10 +238,6 @@ describe API::API, api: true do
         it { expect(response.headers).to include(download_headers) }
       end
-      before do
-        pipeline.reload_status!
-      end
-
       context 'with regular branch' do
         before do
           pipeline.update(ref: 'master',

spec/services/ci/image_for_build_service_spec.rb

@@ -14,7 +14,6 @@ module Ci
       context 'branch name' do
         before { allow(project).to receive(:commit).and_return(OpenStruct.new(sha: commit_sha)) }
         before { build.run! }
-        before { pipeline.reload_status! }
         let(:image) { service.execute(project, ref: 'master') }
         it { expect(image).to be_kind_of(OpenStruct) }
@@ -32,7 +31,6 @@ module Ci
       context 'commit sha' do
         before { build.run! }
-        before { pipeline.reload_status! }
         let(:image) { service.execute(project, sha: build.sha) }
         it { expect(image).to be_kind_of(OpenStruct) }