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