Merge branch 'fix-cancelling-pipelines' into 'master'

Improve how we could cancel pipelines:

Improve how we could cancel pipelines:

* Introduce `HasStatus.cancelable` which we might be able to cancel
* Cancel and check upon `cancelable`
* Also cancel on `CommitStatus` rather than just `Ci::Build`

Fixes #23635

Fixes #17845

See merge request !7508

8 files changed, 288 insertions, 10 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 3fee6c18770..4294a10e9e3 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -161,23 +161,27 @@ module Ci
     end
 
     def retryable?
-      builds.latest.any? do |build|
-        (build.failed? || build.canceled?) && build.retryable?
-      end
+      builds.latest.failed_or_canceled.any?(&:retryable?)
     end
 
     def cancelable?
-      builds.running_or_pending.any?
+      statuses.cancelable.any?
     end
 
     def cancel_running
-      builds.running_or_pending.each(&:cancel)
+      Gitlab::OptimisticLocking.retry_lock(
+        statuses.cancelable) do |cancelable|
+          cancelable.each(&:cancel)
+        end
     end
 
     def retry_failed(user)
-      builds.latest.failed.select(&:retryable?).each do |build|
-        Ci::Build.retry(build, user)
-      end
+      Gitlab::OptimisticLocking.retry_lock(
+        builds.latest.failed_or_canceled) do |failed_or_canceled|
+          failed_or_canceled.select(&:retryable?).each do |build|
+            Ci::Build.retry(build, user)
+          end
+        end
     end
 
     def mark_as_processable_after_stage(stage_idx)
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index ef3e73a4072..2f5aa91a964 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -73,6 +73,11 @@ module HasStatus
     scope :skipped, -> { where(status: 'skipped')  }
     scope :running_or_pending, -> { where(status: [:running, :pending]) }
     scope :finished, -> { where(status: [:success, :failed, :canceled]) }
+    scope :failed_or_canceled, -> { where(status: [:failed, :canceled]) }
+
+    scope :cancelable, -> do
+      where(status: [:running, :pending, :created])
+    end
   end
 
   def started?
diff --git a/changelogs/unreleased/fix-cancelling-pipelines.yml b/changelogs/unreleased/fix-cancelling-pipelines.yml
new file mode 100644
index 00000000000..c21e663093a
--- /dev/null
+++ b/changelogs/unreleased/fix-cancelling-pipelines.yml
@@ -0,0 +1,4 @@
+---
+title: Fix cancelling created or external pipelines
+merge_request: 7508
+author:
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index eb20bd7dd58..c443af09075 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -38,6 +38,10 @@ FactoryGirl.define do
       status 'canceled'
     end
 
+    trait :skipped do
+      status 'skipped'
+    end
+
     trait :running do
       status 'running'
     end
diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb
index 995f2080f10..756b341ecba 100644
--- a/spec/factories/commit_statuses.rb
+++ b/spec/factories/commit_statuses.rb
@@ -19,6 +19,10 @@ FactoryGirl.define do
       status 'canceled'
     end
 
+    trait :skipped do
+      status 'skipped'
+    end
+
     trait :running do
       status 'running'
     end
diff --git a/spec/features/projects/pipelines_spec.rb b/spec/features/projects/pipelines_spec.rb
index 002c6f6b359..10e5466fc85 100644
--- a/spec/features/projects/pipelines_spec.rb
+++ b/spec/features/projects/pipelines_spec.rb
@@ -90,13 +90,20 @@ describe "Pipelines" do
           visit namespace_project_pipelines_path(project.namespace, project)
         end
 
-        it 'is not cancelable' do
-          expect(page).not_to have_link('Cancel')
+        it 'is cancelable' do
+          expect(page).to have_link('Cancel')
         end
 
         it 'has pipeline running' do
           expect(page).to have_selector('.ci-running')
         end
+
+        context 'when canceling' do
+          before { click_link('Cancel') }
+
+          it { expect(page).not_to have_link('Cancel') }
+          it { expect(page).to have_selector('.ci-canceled') }
+        end
       end
 
       context 'when failed' do
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index ea022e03608..3b12f16b4db 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -402,6 +402,160 @@ describe Ci::Pipeline, models: true do
     end
   end
 
+  describe '#cancelable?' do
+    %i[created running pending].each do |status0|
+      context "when there is a build #{status0}" do
+        before do
+          create(:ci_build, status0, pipeline: pipeline)
+        end
+
+        it 'is cancelable' do
+          expect(pipeline.cancelable?).to be_truthy
+        end
+      end
+
+      context "when there is an external job #{status0}" do
+        before do
+          create(:generic_commit_status, status0, pipeline: pipeline)
+        end
+
+        it 'is cancelable' do
+          expect(pipeline.cancelable?).to be_truthy
+        end
+      end
+
+      %i[success failed canceled].each do |status1|
+        context "when there are generic_commit_status jobs for #{status0} and #{status1}" do
+          before do
+            create(:generic_commit_status, status0, pipeline: pipeline)
+            create(:generic_commit_status, status1, pipeline: pipeline)
+          end
+
+          it 'is cancelable' do
+            expect(pipeline.cancelable?).to be_truthy
+          end
+        end
+
+        context "when there are generic_commit_status and ci_build jobs for #{status0} and #{status1}" do
+          before do
+            create(:generic_commit_status, status0, pipeline: pipeline)
+            create(:ci_build, status1, pipeline: pipeline)
+          end
+
+          it 'is cancelable' do
+            expect(pipeline.cancelable?).to be_truthy
+          end
+        end
+
+        context "when there are ci_build jobs for #{status0} and #{status1}" do
+          before do
+            create(:ci_build, status0, pipeline: pipeline)
+            create(:ci_build, status1, pipeline: pipeline)
+          end
+
+          it 'is cancelable' do
+            expect(pipeline.cancelable?).to be_truthy
+          end
+        end
+      end
+    end
+
+    %i[success failed canceled].each do |status|
+      context "when there is a build #{status}" do
+        before do
+          create(:ci_build, status, pipeline: pipeline)
+        end
+
+        it 'is not cancelable' do
+          expect(pipeline.cancelable?).to be_falsey
+        end
+      end
+
+      context "when there is an external job #{status}" do
+        before do
+          create(:generic_commit_status, status, pipeline: pipeline)
+        end
+
+        it 'is not cancelable' do
+          expect(pipeline.cancelable?).to be_falsey
+        end
+      end
+    end
+  end
+
+  describe '#cancel_running' do
+    let(:latest_status) { pipeline.statuses.pluck(:status) }
+
+    context 'when there is a running external job and created build' do
+      before do
+        create(:ci_build, :running, pipeline: pipeline)
+        create(:generic_commit_status, :running, pipeline: pipeline)
+
+        pipeline.cancel_running
+      end
+
+      it 'cancels both jobs' do
+        expect(latest_status).to contain_exactly('canceled', 'canceled')
+      end
+    end
+
+    context 'when builds are in different stages' do
+      before do
+        create(:ci_build, :running, stage_idx: 0, pipeline: pipeline)
+        create(:ci_build, :running, stage_idx: 1, pipeline: pipeline)
+
+        pipeline.cancel_running
+      end
+
+      it 'cancels both jobs' do
+        expect(latest_status).to contain_exactly('canceled', 'canceled')
+      end
+    end
+  end
+
+  describe '#retry_failed' do
+    let(:latest_status) { pipeline.statuses.latest.pluck(:status) }
+
+    context 'when there is a failed build and failed external status' do
+      before do
+        create(:ci_build, :failed, name: 'build', pipeline: pipeline)
+        create(:generic_commit_status, :failed, name: 'jenkins', pipeline: pipeline)
+
+        pipeline.retry_failed(create(:user))
+      end
+
+      it 'retries only build' do
+        expect(latest_status).to contain_exactly('pending', 'failed')
+      end
+    end
+
+    context 'when builds are in different stages' do
+      before do
+        create(:ci_build, :failed, name: 'build', stage_idx: 0, pipeline: pipeline)
+        create(:ci_build, :failed, name: 'jenkins', stage_idx: 1, pipeline: pipeline)
+
+        pipeline.retry_failed(create(:user))
+      end
+
+      it 'retries both builds' do
+        expect(latest_status).to contain_exactly('pending', 'pending')
+      end
+    end
+
+    context 'when there are canceled and failed' do
+      before do
+        create(:ci_build, :failed, name: 'build', stage_idx: 0, pipeline: pipeline)
+        create(:ci_build, :canceled, name: 'jenkins', stage_idx: 1, pipeline: pipeline)
+
+        pipeline.retry_failed(create(:user))
+      end
+
+      it 'retries both builds' do
+        expect(latest_status).to contain_exactly('pending', 'pending')
+      end
+    end
+  end
+
   describe '#execute_hooks' do
     let!(:build_a) { create_build('a', 0) }
     let!(:build_b) { create_build('b', 1) }
diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb
index 87bffbdc54e..9defb17dc92 100644
--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -123,4 +123,100 @@ describe HasStatus do
       it_behaves_like 'build status summary'
     end
   end
+
+  context 'for scope with one status' do
+    shared_examples 'having a job' do |status|
+      %i[ci_build generic_commit_status].each do |type|
+        context "when it's #{status} #{type} job" do
+          let!(:job) { create(type, status) }
+
+          describe ".#{status}" do
+            it 'contains the job' do
+              expect(CommitStatus.public_send(status).all).
+                to contain_exactly(job)
+            end
+          end
+
+          describe '.relevant' do
+            if status == :created
+              it 'contains nothing' do
+                expect(CommitStatus.relevant.all).to be_empty
+              end
+            else
+              it 'contains the job' do
+                expect(CommitStatus.relevant.all).to contain_exactly(job)
+              end
+            end
+          end
+        end
+      end
+    end
+
+    %i[created running pending success
+       failed canceled skipped].each do |status|
+      it_behaves_like 'having a job', status
+    end
+  end
+
+  context 'for scope with more statuses' do
+    shared_examples 'containing the job' do |status|
+      %i[ci_build generic_commit_status].each do |type|
+        context "when it's #{status} #{type} job" do
+          let!(:job) { create(type, status) }
+
+          it 'contains the job' do
+            is_expected.to contain_exactly(job)
+          end
+        end
+      end
+    end
+
+    shared_examples 'not containing the job' do |status|
+      %i[ci_build generic_commit_status].each do |type|
+        context "when it's #{status} #{type} job" do
+          let!(:job) { create(type, status) }
+
+          it 'contains nothing' do
+            is_expected.to be_empty
+          end
+        end
+      end
+    end
+
+    describe '.running_or_pending' do
+      subject { CommitStatus.running_or_pending }
+
+      %i[running pending].each do |status|
+        it_behaves_like 'containing the job', status
+      end
+
+      %i[created failed success].each do |status|
+        it_behaves_like 'not containing the job', status
+      end
+    end
+
+    describe '.finished' do
+      subject { CommitStatus.finished }
+
+      %i[success failed canceled].each do |status|
+        it_behaves_like 'containing the job', status
+      end
+
+      %i[created running pending].each do |status|
+        it_behaves_like 'not containing the job', status
+      end
+    end
+
+    describe '.cancelable' do
+      subject { CommitStatus.cancelable }
+
+      %i[running pending created].each do |status|
+        it_behaves_like 'containing the job', status
+      end
+
+      %i[failed success skipped canceled].each do |status|
+        it_behaves_like 'not containing the job', status
+      end
+    end
+  end
 end