From 6d1c5761cd520f3cb7fa4dbb1a1937c29f468884 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 17 Nov 2016 00:57:50 +0800
Subject: [PATCH 01/12] 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
---
 app/models/ci/pipeline.rb                     |  4 +-
 app/models/concerns/has_status.rb             |  4 ++
 .../unreleased/fix-cancelling-pipelines.yml   |  4 ++
 spec/features/projects/pipelines_spec.rb      |  4 +-
 spec/models/ci/pipeline_spec.rb               | 40 +++++++++++++++++++
 5 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 changelogs/unreleased/fix-cancelling-pipelines.yml

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 3fee6c18770..4eb85f62ee4 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -167,11 +167,11 @@ module Ci
     end
 
     def cancelable?
-      builds.running_or_pending.any?
+      statuses.cancelable.any?
     end
 
     def cancel_running
-      builds.running_or_pending.each(&:cancel)
+      statuses.cancelable.each(&:cancel)
     end
 
     def retry_failed(user)
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index ef3e73a4072..1332743429d 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -73,6 +73,10 @@ module HasStatus
     scope :skipped, -> { where(status: 'skipped')  }
     scope :running_or_pending, -> { where(status: [:running, :pending]) }
     scope :finished, -> { where(status: [:success, :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/features/projects/pipelines_spec.rb b/spec/features/projects/pipelines_spec.rb
index db56a50e058..b3ef9a11d56 100644
--- a/spec/features/projects/pipelines_spec.rb
+++ b/spec/features/projects/pipelines_spec.rb
@@ -90,8 +90,8 @@ 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
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 71b7628ef10..d013dc7b31a 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -402,6 +402,46 @@ describe Ci::Pipeline, models: true do
     end
   end
 
+  describe '#cancelable?' do
+    subject { pipeline.cancelable? }
+
+    %i[created running pending].each do |status|
+      context "when there is a build #{status}" do
+        before do
+          create(:ci_build, status, pipeline: pipeline)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context "when there is an external job #{status}" do
+        before do
+          create(:generic_commit_status, status, pipeline: pipeline)
+        end
+
+        it { is_expected.to be_truthy }
+      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_expected.to be_falsey }
+      end
+
+      context "when there is an external job #{status}" do
+        before do
+          create(:generic_commit_status, status, pipeline: pipeline)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
   describe '#execute_hooks' do
     let!(:build_a) { create_build('a', 0) }
     let!(:build_b) { create_build('b', 1) }

From 9a0201473e24e5036506f0cc8761290da1ca743b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 23:16:51 +0800
Subject: [PATCH 02/12] Add when cancelling for external jobs, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18622182
---
 spec/features/projects/pipelines_spec.rb | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/spec/features/projects/pipelines_spec.rb b/spec/features/projects/pipelines_spec.rb
index b3ef9a11d56..544372108ed 100644
--- a/spec/features/projects/pipelines_spec.rb
+++ b/spec/features/projects/pipelines_spec.rb
@@ -97,6 +97,13 @@ describe "Pipelines" do
         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

From 100076ecbbdf3eae361a6356ddfb55b1694e4741 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 18 Nov 2016 23:27:06 +0800
Subject: [PATCH 03/12] Add tests against two jobs having different status

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18622469
---
 spec/models/ci/pipeline_spec.rb | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index d013dc7b31a..2cc6d1be606 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -421,6 +421,18 @@ describe Ci::Pipeline, models: true do
 
         it { is_expected.to be_truthy }
       end
+
+      %i[success failed canceled].each do |status2|
+        context "when there are two builds for #{status} and #{status2}" do
+          before do
+            build = %i[ci_build generic_commit_status]
+            create(build.sample, status, pipeline: pipeline)
+            create(build.sample, status2, pipeline: pipeline)
+          end
+
+          it { is_expected.to be_truthy }
+        end
+      end
     end
 
     %i[success failed canceled].each do |status|

From b6a7a4783435a7fa34f26dbf3b16ab8e7ed21b88 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 19 Nov 2016 01:02:49 +0800
Subject: [PATCH 04/12] Add a lot of tests for scopes, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18622499
---
 spec/factories/ci/builds.rb             |  4 ++
 spec/factories/commit_statuses.rb       |  4 ++
 spec/models/concerns/has_status_spec.rb | 77 +++++++++++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 0c93bbdfe26..e7fe489e5eb 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/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb
index 87bffbdc54e..24cd435256e 100644
--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -123,4 +123,81 @@ describe HasStatus do
       it_behaves_like 'build status summary'
     end
   end
+
+  def self.random_type
+    %i[ci_build generic_commit_status].sample
+  end
+
+  context 'for scope with one status' do
+    shared_examples 'having a job' do |type, status|
+      context "when it's #{status} #{type} job" do
+        let!(:job) { create(type, status) }
+
+        describe ".#{status}" do
+          subject { CommitStatus.public_send(status).all }
+
+          it { is_expected.to contain_exactly(job) }
+        end
+
+        describe '.relevant' do
+          subject { CommitStatus.relevant.all }
+
+          it do
+            case status
+            when :created
+              is_expected.to be_empty
+            else
+              is_expected.to contain_exactly(job)
+            end
+          end
+        end
+      end
+    end
+
+    %i[created running pending success
+       failed canceled skipped].each do |status|
+      it_behaves_like 'having a job', random_type, status
+    end
+  end
+
+  context 'for scope with more statuses' do
+    shared_examples 'having a job' do |type, status, excluded_status|
+      context "when it's #{status} #{type} job" do
+        let!(:job) { create(type, status) }
+
+        it do
+          case status
+          when excluded_status
+            is_expected.to be_empty
+          else
+            is_expected.to contain_exactly(job)
+          end
+        end
+      end
+    end
+
+    describe '.running_or_pending' do
+      subject { CommitStatus.running_or_pending }
+
+      %i[running pending created].each do |status|
+        it_behaves_like 'having a job', random_type, status, :created
+      end
+    end
+
+    describe '.finished' do
+      subject { CommitStatus.finished }
+
+      %i[success failed canceled created].each do |status|
+        it_behaves_like 'having a job', random_type, status, :created
+      end
+    end
+
+    describe '.cancelable' do
+      subject { CommitStatus.cancelable }
+
+      %i[running pending created failed].each do |status|
+        it_behaves_like 'having a job', random_type, status, :failed
+      end
+    end
+  end
 end

From 92a83aaee1c5715bf95715649aa8ced50a70dea8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 19 Nov 2016 01:17:40 +0800
Subject: [PATCH 05/12] Add a test for Ci::Pipeline#cancel_running:

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18622559

I didn't write an exhaustive one because we already have it on
HasStatus, from:

https://gitlab.com/gitlab-org/gitlab-ce/commit/b6a7a4783435a7fa34f26dbf3b16ab8e7ed21b88
---
 spec/models/ci/pipeline_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 2cc6d1be606..74579e0c832 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -454,6 +454,22 @@ describe Ci::Pipeline, models: true do
     end
   end
 
+  describe '#cancel_running' do
+    context 'when there is a running external job and created build' do
+      before do
+        create(:generic_commit_status, :running, pipeline: pipeline)
+        create(:ci_build, :created, pipeline: pipeline)
+
+        pipeline.cancel_running
+      end
+
+      it 'cancels both jobs' do
+        expect(pipeline.statuses.pluck(:status)).
+          to contain_exactly('canceled', 'canceled')
+      end
+    end
+  end
+
   describe '#execute_hooks' do
     let!(:build_a) { create_build('a', 0) }
     let!(:build_b) { create_build('b', 1) }

From ca639c9b824d6c8effb620bc71255eb0895ab2cc Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Sat, 19 Nov 2016 14:04:11 +0100
Subject: [PATCH 06/12] Allow to retry failed or canceled builds and fix cancel
 running specs failure

---
 app/models/ci/pipeline.rb         | 14 ++++---
 app/models/concerns/has_status.rb |  1 +
 spec/models/ci/pipeline_spec.rb   | 63 +++++++++++++++++++++++++++++--
 3 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 4eb85f62ee4..c0f2c8ba787 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -161,9 +161,7 @@ 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?
@@ -171,12 +169,16 @@ module Ci
     end
 
     def cancel_running
-      statuses.cancelable.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)
+      Gitlab::OptimisticLocking.retry_lock(builds.latest.failed_or_canceled) do |failed|
+        failed.select(&:retryable?).each do |build|
+          Ci::Build.retry(build, user)
+        end
       end
     end
 
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index 1332743429d..2f5aa91a964 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -73,6 +73,7 @@ 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])
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 74579e0c832..af619a02ed9 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -455,17 +455,74 @@ describe Ci::Pipeline, models: true do
   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)
-        create(:ci_build, :created, pipeline: pipeline)
 
         pipeline.cancel_running
       end
 
       it 'cancels both jobs' do
-        expect(pipeline.statuses.pluck(:status)).
-          to contain_exactly('canceled', 'canceled')
+        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(nil)
+      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(nil)
+      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(nil)
+      end
+
+      it 'retries both builds' do
+        expect(latest_status).to contain_exactly('pending', 'pending')
       end
     end
   end

From 1edb1746a51a19fae24c976c329e80a1dbd6062a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 21 Nov 2016 17:59:57 +0800
Subject: [PATCH 07/12] Prefer a description for it and split the case:

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18730091
---
 spec/models/ci/pipeline_spec.rb         | 22 ++++++---
 spec/models/concerns/has_status_spec.rb | 61 +++++++++++++++----------
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index af619a02ed9..29e5693d5ab 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -403,15 +403,15 @@ describe Ci::Pipeline, models: true do
   end
 
   describe '#cancelable?' do
-    subject { pipeline.cancelable? }
-
     %i[created running pending].each do |status|
       context "when there is a build #{status}" do
         before do
           create(:ci_build, status, pipeline: pipeline)
         end
 
-        it { is_expected.to be_truthy }
+        it 'is cancelable' do
+          expect(pipeline.cancelable?).to be_truthy
+        end
       end
 
       context "when there is an external job #{status}" do
@@ -419,7 +419,9 @@ describe Ci::Pipeline, models: true do
           create(:generic_commit_status, status, pipeline: pipeline)
         end
 
-        it { is_expected.to be_truthy }
+        it 'is cancelable' do
+          expect(pipeline.cancelable?).to be_truthy
+        end
       end
 
       %i[success failed canceled].each do |status2|
@@ -430,7 +432,9 @@ describe Ci::Pipeline, models: true do
             create(build.sample, status2, pipeline: pipeline)
           end
 
-          it { is_expected.to be_truthy }
+          it 'is cancelable' do
+            expect(pipeline.cancelable?).to be_truthy
+          end
         end
       end
     end
@@ -441,7 +445,9 @@ describe Ci::Pipeline, models: true do
           create(:ci_build, status, pipeline: pipeline)
         end
 
-        it { is_expected.to be_falsey }
+        it 'is not cancelable' do
+          expect(pipeline.cancelable?).to be_falsey
+        end
       end
 
       context "when there is an external job #{status}" do
@@ -449,7 +455,9 @@ describe Ci::Pipeline, models: true do
           create(:generic_commit_status, status, pipeline: pipeline)
         end
 
-        it { is_expected.to be_falsey }
+        it 'is not cancelable' do
+          expect(pipeline.cancelable?).to be_falsey
+        end
       end
     end
   end
diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb
index 24cd435256e..788c84bf5de 100644
--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -134,20 +134,20 @@ describe HasStatus do
         let!(:job) { create(type, status) }
 
         describe ".#{status}" do
-          subject { CommitStatus.public_send(status).all }
-
-          it { is_expected.to contain_exactly(job) }
+          it 'contains the job' do
+            expect(CommitStatus.public_send(status).all).
+              to contain_exactly(job)
+          end
         end
 
         describe '.relevant' do
-          subject { CommitStatus.relevant.all }
-
-          it do
-            case status
-            when :created
-              is_expected.to be_empty
-            else
-              is_expected.to contain_exactly(job)
+          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
@@ -161,17 +161,22 @@ describe HasStatus do
   end
 
   context 'for scope with more statuses' do
-    shared_examples 'having a job' do |type, status, excluded_status|
+    shared_examples 'containing the job' do |type, status|
       context "when it's #{status} #{type} job" do
         let!(:job) { create(type, status) }
 
-        it do
-          case status
-          when excluded_status
-            is_expected.to be_empty
-          else
-            is_expected.to contain_exactly(job)
-          end
+        it 'contains the job' do
+          is_expected.to contain_exactly(job)
+        end
+      end
+    end
+
+    shared_examples 'not containing the job' do |type, status|
+      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
@@ -179,25 +184,31 @@ describe HasStatus do
     describe '.running_or_pending' do
       subject { CommitStatus.running_or_pending }
 
-      %i[running pending created].each do |status|
-        it_behaves_like 'having a job', random_type, status, :created
+      %i[running pending].each do |status|
+        it_behaves_like 'containing the job', random_type, status
       end
+
+      it_behaves_like 'not containing the job', random_type, :created
     end
 
     describe '.finished' do
       subject { CommitStatus.finished }
 
-      %i[success failed canceled created].each do |status|
-        it_behaves_like 'having a job', random_type, status, :created
+      %i[success failed canceled].each do |status|
+        it_behaves_like 'containing the job', random_type, status
       end
+
+      it_behaves_like 'not containing the job', random_type, :created
     end
 
     describe '.cancelable' do
       subject { CommitStatus.cancelable }
 
-      %i[running pending created failed].each do |status|
-        it_behaves_like 'having a job', random_type, status, :failed
+      %i[running pending created].each do |status|
+        it_behaves_like 'containing the job', random_type, status
       end
+
+      it_behaves_like 'not containing the job', random_type, :failed
     end
   end
 end

From d09d6ad01da722b3558565b8b78f0476b529a91c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 21 Nov 2016 22:39:58 +0800
Subject: [PATCH 08/12] Test against all types and more status:

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18748231
---
 spec/models/concerns/has_status_spec.rb | 84 ++++++++++++++-----------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb
index 788c84bf5de..9defb17dc92 100644
--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -124,30 +124,28 @@ describe HasStatus do
     end
   end
 
-  def self.random_type
-    %i[ci_build generic_commit_status].sample
-  end
-
   context 'for scope with one status' do
-    shared_examples 'having a job' do |type, status|
-      context "when it's #{status} #{type} job" do
-        let!(:job) { create(type, status) }
+    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
+          describe ".#{status}" do
             it 'contains the job' do
-              expect(CommitStatus.relevant.all).to contain_exactly(job)
+              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
@@ -156,27 +154,31 @@ describe HasStatus do
 
     %i[created running pending success
        failed canceled skipped].each do |status|
-      it_behaves_like 'having a job', random_type, status
+      it_behaves_like 'having a job', status
     end
   end
 
   context 'for scope with more statuses' do
-    shared_examples 'containing the job' do |type, status|
-      context "when it's #{status} #{type} job" do
-        let!(:job) { create(type, status) }
+    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)
+          it 'contains the job' do
+            is_expected.to contain_exactly(job)
+          end
         end
       end
     end
 
-    shared_examples 'not containing the job' do |type, status|
-      context "when it's #{status} #{type} job" do
-        let!(:job) { create(type, status) }
+    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
+          it 'contains nothing' do
+            is_expected.to be_empty
+          end
         end
       end
     end
@@ -185,30 +187,36 @@ describe HasStatus do
       subject { CommitStatus.running_or_pending }
 
       %i[running pending].each do |status|
-        it_behaves_like 'containing the job', random_type, status
+        it_behaves_like 'containing the job', status
       end
 
-      it_behaves_like 'not containing the job', random_type, :created
+      %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', random_type, status
+        it_behaves_like 'containing the job', status
       end
 
-      it_behaves_like 'not containing the job', random_type, :created
+      %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', random_type, status
+        it_behaves_like 'containing the job', status
       end
 
-      it_behaves_like 'not containing the job', random_type, :failed
+      %i[failed success skipped canceled].each do |status|
+        it_behaves_like 'not containing the job', status
+      end
     end
   end
 end

From c7c4850d0b9d6751a5f6fdaa1f9c34aee6728676 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 22 Nov 2016 01:21:15 +0800
Subject: [PATCH 09/12] Test against all possible cases, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18755739
---
 spec/models/ci/pipeline_spec.rb | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 29e5693d5ab..cbf25c23b05 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -403,10 +403,10 @@ describe Ci::Pipeline, models: true do
   end
 
   describe '#cancelable?' do
-    %i[created running pending].each do |status|
-      context "when there is a build #{status}" do
+    %i[created running pending].each do |status0|
+      context "when there is a build #{status0}" do
         before do
-          create(:ci_build, status, pipeline: pipeline)
+          create(:ci_build, status0, pipeline: pipeline)
         end
 
         it 'is cancelable' do
@@ -414,9 +414,9 @@ describe Ci::Pipeline, models: true do
         end
       end
 
-      context "when there is an external job #{status}" do
+      context "when there is an external job #{status0}" do
         before do
-          create(:generic_commit_status, status, pipeline: pipeline)
+          create(:generic_commit_status, status0, pipeline: pipeline)
         end
 
         it 'is cancelable' do
@@ -424,16 +424,19 @@ describe Ci::Pipeline, models: true do
         end
       end
 
-      %i[success failed canceled].each do |status2|
-        context "when there are two builds for #{status} and #{status2}" do
-          before do
-            build = %i[ci_build generic_commit_status]
-            create(build.sample, status, pipeline: pipeline)
-            create(build.sample, status2, pipeline: pipeline)
-          end
+      %i[success failed canceled].each do |status1|
+        %i[ci_build generic_commit_status].each do |type0|
+          %i[ci_build generic_commit_status].each do |type1|
+            context "when there are #{type0} and #{type1} for #{status0} and #{status1}" do
+              before do
+                create(type0, status0, pipeline: pipeline)
+                create(type1, status1, pipeline: pipeline)
+              end
 
-          it 'is cancelable' do
-            expect(pipeline.cancelable?).to be_truthy
+              it 'is cancelable' do
+                expect(pipeline.cancelable?).to be_truthy
+              end
+            end
           end
         end
       end

From 3566965417b7921cdb301c44cfb308551cdc1e82 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 22 Nov 2016 18:55:00 +0800
Subject: [PATCH 10/12] Passing a user to retry_failed in tests

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18794547
---
 spec/models/ci/pipeline_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index cbf25c23b05..03924a436de 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -503,7 +503,7 @@ describe Ci::Pipeline, models: true do
         create(:ci_build, :failed, name: 'build', pipeline: pipeline)
         create(:generic_commit_status, :failed, name: 'jenkins', pipeline: pipeline)
 
-        pipeline.retry_failed(nil)
+        pipeline.retry_failed(create(:user))
       end
 
       it 'retries only build' do
@@ -516,7 +516,7 @@ describe Ci::Pipeline, models: true 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(nil)
+        pipeline.retry_failed(create(:user))
       end
 
       it 'retries both builds' do
@@ -529,7 +529,7 @@ describe Ci::Pipeline, models: true 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(nil)
+        pipeline.retry_failed(create(:user))
       end
 
       it 'retries both builds' do

From cc43c9acc29117cd9ec3c25805e6c5ea874e5969 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 22 Nov 2016 19:07:12 +0800
Subject: [PATCH 11/12] Expand the loop and reduce overlapped conditions

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18794681
---
 spec/models/ci/pipeline_spec.rb | 40 ++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 03924a436de..438810d8206 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -425,18 +425,36 @@ describe Ci::Pipeline, models: true do
       end
 
       %i[success failed canceled].each do |status1|
-        %i[ci_build generic_commit_status].each do |type0|
-          %i[ci_build generic_commit_status].each do |type1|
-            context "when there are #{type0} and #{type1} for #{status0} and #{status1}" do
-              before do
-                create(type0, status0, pipeline: pipeline)
-                create(type1, status1, pipeline: pipeline)
-              end
+        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
+          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

From bd3a544ab0417b758cf5e8e5bcd4ae9c0fed1bae Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 23 Nov 2016 19:43:56 +0800
Subject: [PATCH 12/12] Wrap against 80 chars and rename failed_or_canceled

to better reflect the status. Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7508#note_18858398
---
 app/models/ci/pipeline.rb | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index c0f2c8ba787..4294a10e9e3 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -169,17 +169,19 @@ module Ci
     end
 
     def cancel_running
-      Gitlab::OptimisticLocking.retry_lock(statuses.cancelable) do |cancelable|
-        cancelable.each(&:cancel)
-      end
+      Gitlab::OptimisticLocking.retry_lock(
+        statuses.cancelable) do |cancelable|
+          cancelable.each(&:cancel)
+        end
     end
 
     def retry_failed(user)
-      Gitlab::OptimisticLocking.retry_lock(builds.latest.failed_or_canceled) do |failed|
-        failed.select(&:retryable?).each do |build|
-          Ci::Build.retry(build, user)
+      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
     end
 
     def mark_as_processable_after_stage(stage_idx)