Merge branch 'sh-fix-pipeline-delete-caching' into 'master'

Properly expire all pipeline caches when pipeline is deleted

Closes #60469

See merge request gitlab-org/gitlab-ce!27334

6 files changed, 134 insertions, 78 deletions

diff --git a/app/services/ci/destroy_pipeline_service.rb b/app/services/ci/destroy_pipeline_service.rb
index 2292ec42b16..9aea20c45f7 100644
--- a/app/services/ci/destroy_pipeline_service.rb
+++ b/app/services/ci/destroy_pipeline_service.rb
@@ -5,9 +5,9 @@ module Ci
     def execute(pipeline)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :destroy_pipeline, pipeline)
 
-      pipeline.destroy!
+      Ci::ExpirePipelineCacheService.new.execute(pipeline, delete: true)
 
-      Gitlab::Cache::Ci::ProjectPipelineStatus.new(pipeline.project).delete_from_cache
+      pipeline.destroy!
     end
   end
 end
diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb
new file mode 100644
index 00000000000..d8d38128af6
--- /dev/null
+++ b/app/services/ci/expire_pipeline_cache_service.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+module Ci
+  class ExpirePipelineCacheService
+    def execute(pipeline, delete: false)
+      store = Gitlab::EtagCaching::Store.new
+
+      update_etag_cache(pipeline, store)
+
+      if delete
+        Gitlab::Cache::Ci::ProjectPipelineStatus.new(pipeline.project).delete_from_cache
+      else
+        Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline)
+      end
+    end
+
+    private
+
+    def project_pipelines_path(project)
+      Gitlab::Routing.url_helpers.project_pipelines_path(project, format: :json)
+    end
+
+    def project_pipeline_path(project, pipeline)
+      Gitlab::Routing.url_helpers.project_pipeline_path(project, pipeline, format: :json)
+    end
+
+    def commit_pipelines_path(project, commit)
+      Gitlab::Routing.url_helpers.pipelines_project_commit_path(project, commit.id, format: :json)
+    end
+
+    def new_merge_request_pipelines_path(project)
+      Gitlab::Routing.url_helpers.project_new_merge_request_path(project, format: :json)
+    end
+
+    def each_pipelines_merge_request_path(pipeline)
+      pipeline.all_merge_requests.each do |merge_request|
+        path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(merge_request.target_project, merge_request, format: :json)
+
+        yield(path)
+      end
+    end
+
+    # Updates ETag caches of a pipeline.
+    #
+    # This logic resides in a separate method so that EE can more easily extend
+    # it.
+    #
+    # @param [Ci::Pipeline] pipeline
+    # @param [Gitlab::EtagCaching::Store] store
+    def update_etag_cache(pipeline, store)
+      project = pipeline.project
+
+      store.touch(project_pipelines_path(project))
+      store.touch(project_pipeline_path(project, pipeline))
+      store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
+      store.touch(new_merge_request_pipelines_path(project))
+      each_pipelines_merge_request_path(pipeline) do |path|
+        store.touch(path)
+      end
+    end
+  end
+end
diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb
index 2c070d482a1..78e68d7bf46 100644
--- a/app/workers/expire_pipeline_cache_worker.rb
+++ b/app/workers/expire_pipeline_cache_worker.rb
@@ -11,56 +11,7 @@ class ExpirePipelineCacheWorker
     pipeline = Ci::Pipeline.find_by(id: pipeline_id)
     return unless pipeline
 
-    store = Gitlab::EtagCaching::Store.new
-
-    update_etag_cache(pipeline, store)
-
-    Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline)
+    Ci::ExpirePipelineCacheService.new.execute(pipeline)
   end
   # rubocop: enable CodeReuse/ActiveRecord
-
-  private
-
-  def project_pipelines_path(project)
-    Gitlab::Routing.url_helpers.project_pipelines_path(project, format: :json)
-  end
-
-  def project_pipeline_path(project, pipeline)
-    Gitlab::Routing.url_helpers.project_pipeline_path(project, pipeline, format: :json)
-  end
-
-  def commit_pipelines_path(project, commit)
-    Gitlab::Routing.url_helpers.pipelines_project_commit_path(project, commit.id, format: :json)
-  end
-
-  def new_merge_request_pipelines_path(project)
-    Gitlab::Routing.url_helpers.project_new_merge_request_path(project, format: :json)
-  end
-
-  def each_pipelines_merge_request_path(pipeline)
-    pipeline.all_merge_requests.each do |merge_request|
-      path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(merge_request.target_project, merge_request, format: :json)
-
-      yield(path)
-    end
-  end
-
-  # Updates ETag caches of a pipeline.
-  #
-  # This logic resides in a separate method so that EE can more easily extend
-  # it.
-  #
-  # @param [Ci::Pipeline] pipeline
-  # @param [Gitlab::EtagCaching::Store] store
-  def update_etag_cache(pipeline, store)
-    project = pipeline.project
-
-    store.touch(project_pipelines_path(project))
-    store.touch(project_pipeline_path(project, pipeline))
-    store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
-    store.touch(new_merge_request_pipelines_path(project))
-    each_pipelines_merge_request_path(pipeline) do |path|
-      store.touch(path)
-    end
-  end
 end
diff --git a/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml b/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml
new file mode 100644
index 00000000000..98846ea9825
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml
@@ -0,0 +1,5 @@
+---
+title: Properly expire all pipeline caches when pipeline is deleted
+merge_request: 27334
+author:
+type: fixed
diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb
new file mode 100644
index 00000000000..ff2d286465a
--- /dev/null
+++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::ExpirePipelineCacheService do
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
+  set(:pipeline) { create(:ci_pipeline, project: project) }
+  subject { described_class.new }
+
+  describe '#execute' do
+    it 'invalidates Etag caching for project pipelines path' do
+      pipelines_path = "/#{project.full_path}/pipelines.json"
+      new_mr_pipelines_path = "/#{project.full_path}/merge_requests/new.json"
+      pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json"
+
+      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipelines_path)
+      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(new_mr_pipelines_path)
+      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path)
+
+      subject.execute(pipeline)
+    end
+
+    it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do
+      pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master')
+      merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+      merge_request_pipelines_path = "/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines.json"
+
+      allow_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch)
+      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_pipelines_path)
+
+      subject.execute(pipeline)
+    end
+
+    it 'updates the cached status for a project' do
+      expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline)
+                                                            .with(pipeline)
+
+      subject.execute(pipeline)
+    end
+
+    context 'destroyed pipeline' do
+      let(:project_with_repo) { create(:project, :repository) }
+      let!(:pipeline_with_commit) { create(:ci_pipeline, :success, project: project_with_repo, sha: project_with_repo.commit.id) }
+
+      it 'clears the cache', :use_clean_rails_memory_store_caching do
+        create(:commit_status, :success, pipeline: pipeline_with_commit, ref: pipeline_with_commit.ref)
+
+        # Sanity check
+        expect(project_with_repo.pipeline_status.has_status?).to be_truthy
+
+        subject.execute(pipeline_with_commit, delete: true)
+
+        pipeline_with_commit.destroy!
+
+        # Need to use find to avoid memoization
+        expect(Project.find(project_with_repo.id).pipeline_status.has_status?).to be_falsey
+      end
+    end
+  end
+end
diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb
index c98b69ef76c..5652f5e8685 100644
--- a/spec/workers/expire_pipeline_cache_worker_spec.rb
+++ b/spec/workers/expire_pipeline_cache_worker_spec.rb
@@ -9,40 +9,17 @@ describe ExpirePipelineCacheWorker do
   subject { described_class.new }
 
   describe '#perform' do
-    it 'invalidates Etag caching for project pipelines path' do
-      pipelines_path = "/#{project.full_path}/pipelines.json"
-      new_mr_pipelines_path = "/#{project.full_path}/merge_requests/new.json"
-      pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json"
-
-      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipelines_path)
-      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(new_mr_pipelines_path)
-      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path)
-
-      subject.perform(pipeline.id)
-    end
-
-    it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do
-      pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master')
-      merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
-      merge_request_pipelines_path = "/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines.json"
-
-      allow_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch)
-      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_pipelines_path)
+    it 'executes the service' do
+      expect_any_instance_of(Ci::ExpirePipelineCacheService).to receive(:execute).with(pipeline).and_call_original
 
       subject.perform(pipeline.id)
     end
 
     it "doesn't do anything if the pipeline not exist" do
+      expect_any_instance_of(Ci::ExpirePipelineCacheService).not_to receive(:execute)
       expect_any_instance_of(Gitlab::EtagCaching::Store).not_to receive(:touch)
 
       subject.perform(617748)
     end
-
-    it 'updates the cached status for a project' do
-      expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline)
-                                                            .with(pipeline)
-
-      subject.perform(pipeline.id)
-    end
   end
 end