From d96c24d81590dd6da237f131d4c074b70e64e030 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Fri, 9 Aug 2019 18:09:06 +0800 Subject: Invalidate branches cache on PostReceive Whenever `PostReceive` is enqueued, `UpdateMergeRequestsWorker` is enqueued and `MergeRequests::RefreshService` is called, it'll check if the source branch of each MR asssociated to the push exists or not via `MergeRequest#source_branch_exists?`. The said method will call `Repository#branch_exists?` which is cached in `Rails.cache`. When the cache contains outdated data and the source branch actually exists, the `MergeRequests#RefreshService` job will close associated MRs which is not correct. The fix is to expire the branches cache of the project so we have updated data during the post receive hook which will help in the accuracy of the check if we need to close associated MRs or not. --- app/models/repository.rb | 8 ++-- app/services/git/branch_hooks_service.rb | 4 +- app/workers/post_receive.rb | 3 ++ .../65803-invalidate-branches-cache-on-refresh.yml | 5 +++ lib/gitlab/git_post_receive.rb | 8 ++++ spec/lib/gitlab/git_post_receive_spec.rb | 52 ++++++++++++++++++++++ spec/models/repository_spec.rb | 12 +++++ spec/workers/post_receive_spec.rb | 25 ++++++++--- 8 files changed, 105 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml create mode 100644 spec/lib/gitlab/git_post_receive_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index a89f573e3d6..58abfaef801 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -460,8 +460,8 @@ class Repository end # Runs code after a new branch has been created. - def after_create_branch - expire_branches_cache + def after_create_branch(expire_cache: true) + expire_branches_cache if expire_cache repository_event(:push_branch) end @@ -474,8 +474,8 @@ class Repository end # Runs code after an existing branch has been removed. - def after_remove_branch - expire_branches_cache + def after_remove_branch(expire_cache: true) + expire_branches_cache if expire_cache end def method_missing(msg, *args, &block) diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb index c41f445c3c4..431a5aedf2e 100644 --- a/app/services/git/branch_hooks_service.rb +++ b/app/services/git/branch_hooks_service.rb @@ -63,7 +63,7 @@ module Git end def branch_create_hooks - project.repository.after_create_branch + project.repository.after_create_branch(expire_cache: false) project.after_create_default_branch if default_branch? end @@ -78,7 +78,7 @@ module Git end def branch_remove_hooks - project.repository.after_remove_branch + project.repository.after_remove_branch(expire_cache: false) end # Schedules processing of commit messages diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index dba7837bd12..0865c2f0e0e 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -42,6 +42,9 @@ class PostReceive user = identify_user(post_received) return false unless user + # Expire the branches cache so we have updated data for this push + post_received.project.repository.expire_branches_cache if post_received.branches_exist? + post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index| service_klass = if Gitlab::Git.tag_ref?(ref) diff --git a/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml b/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml new file mode 100644 index 00000000000..217db8aa05a --- /dev/null +++ b/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml @@ -0,0 +1,5 @@ +--- +title: Invalidate branches cache on PostReceive +merge_request: 31653 +author: +type: fixed diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index d98b85fecc4..307ec8b2cfb 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -27,6 +27,14 @@ module Gitlab end end + def branches_exist? + changes_refs do |_oldrev, _newrev, ref| + return true if Gitlab::Git.branch_ref?(ref) # rubocop:disable Cop/AvoidReturnFromBlocks + end + + false + end + private def deserialize_changes(changes) diff --git a/spec/lib/gitlab/git_post_receive_spec.rb b/spec/lib/gitlab/git_post_receive_spec.rb new file mode 100644 index 00000000000..f4a10d8d984 --- /dev/null +++ b/spec/lib/gitlab/git_post_receive_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::Gitlab::GitPostReceive do + let(:project) { create(:project) } + + subject { described_class.new(project, "project-#{project.id}", changes.dup, {}) } + + describe '#branches_exist?' do + context 'with no branches' do + let(:changes) do + <<~EOF + 654321 210987 refs/nobranches/tag1 + 654322 210986 refs/tags/test1 + 654323 210985 refs/merge-requests/mr1 + EOF + end + + it 'returns false' do + expect(subject.branches_exist?).to be_falsey + end + end + + context 'with branches' do + let(:changes) do + <<~EOF + 654322 210986 refs/heads/test1 + 654321 210987 refs/tags/tag1 + 654323 210985 refs/merge-requests/mr1 + EOF + end + + it 'returns true' do + expect(subject.branches_exist?).to be_truthy + end + end + + context 'with malformed changes' do + let(:changes) do + <<~EOF + ref/heads/1 a + somebranch refs/heads/2 + EOF + end + + it 'returns false' do + expect(subject.branches_exist?).to be_falsey + end + end + end +end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 12dff440ce2..fa243876632 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1781,6 +1781,12 @@ describe Repository do repository.after_create_branch end + + it 'does not expire the branch caches when specified' do + expect(repository).not_to receive(:expire_branches_cache) + + repository.after_create_branch(expire_cache: false) + end end describe '#after_remove_branch' do @@ -1789,6 +1795,12 @@ describe Repository do repository.after_remove_branch end + + it 'does not expire the branch caches when specified' do + expect(repository).not_to receive(:expire_branches_cache) + + repository.after_remove_branch(expire_cache: false) + end end describe '#after_create' do diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 39f1beb4efa..01ccae194fb 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -57,12 +57,19 @@ describe PostReceive do context 'with changes' do before do allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner) + allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT]) end context "branches" do - let(:changes) { "123456 789012 refs/heads/tést" } + let(:changes) { '123456 789012 refs/heads/tést' } + + it 'expires the branches cache' do + expect(project.repository).to receive(:expire_branches_cache) + + described_class.new.perform(gl_repository, key_id, base64_changes) + end - it "calls Git::BranchPushService" do + it 'calls Git::BranchPushService' do expect_next_instance_of(Git::BranchPushService) do |service| expect(service).to receive(:execute).and_return(true) end @@ -73,16 +80,22 @@ describe PostReceive do end end - context "tags" do - let(:changes) { "123456 789012 refs/tags/tag" } + context 'tags' do + let(:changes) { '123456 789012 refs/tags/tag' } - it "calls Git::TagPushService" do - expect(Git::BranchPushService).not_to receive(:execute) + it 'does not expire branches cache' do + expect(project.repository).not_to receive(:expire_branches_cache) + described_class.new.perform(gl_repository, key_id, base64_changes) + end + + it 'calls Git::TagPushService' do expect_next_instance_of(Git::TagPushService) do |service| expect(service).to receive(:execute).and_return(true) end + expect(Git::BranchPushService).not_to receive(:new) + described_class.new.perform(gl_repository, key_id, base64_changes) end end -- cgit v1.2.1 From afe867921cab046a34bc463840c6e9f5d51f1f70 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 9 Aug 2019 15:41:07 -0700 Subject: Rename branches_exist? -> includes_branches? --- app/workers/post_receive.rb | 2 +- lib/gitlab/git_post_receive.rb | 8 +++----- spec/lib/gitlab/git_post_receive_spec.rb | 8 ++++---- spec/workers/post_receive_spec.rb | 11 ++++++++--- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 0865c2f0e0e..d8dfbc0faf7 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -43,7 +43,7 @@ class PostReceive return false unless user # Expire the branches cache so we have updated data for this push - post_received.project.repository.expire_branches_cache if post_received.branches_exist? + post_received.project.repository.expire_branches_cache if post_received.includes_branches? post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index| service_klass = diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index 307ec8b2cfb..6c7f23a673c 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -27,12 +27,10 @@ module Gitlab end end - def branches_exist? - changes_refs do |_oldrev, _newrev, ref| - return true if Gitlab::Git.branch_ref?(ref) # rubocop:disable Cop/AvoidReturnFromBlocks + def includes_branches? + enum_for(:changes_refs).any? do |_oldrev, _newrev, ref| + Gitlab::Git.branch_ref?(ref) end - - false end private diff --git a/spec/lib/gitlab/git_post_receive_spec.rb b/spec/lib/gitlab/git_post_receive_spec.rb index f4a10d8d984..1911e954df9 100644 --- a/spec/lib/gitlab/git_post_receive_spec.rb +++ b/spec/lib/gitlab/git_post_receive_spec.rb @@ -7,7 +7,7 @@ describe ::Gitlab::GitPostReceive do subject { described_class.new(project, "project-#{project.id}", changes.dup, {}) } - describe '#branches_exist?' do + describe '#includes_branches?' do context 'with no branches' do let(:changes) do <<~EOF @@ -18,7 +18,7 @@ describe ::Gitlab::GitPostReceive do end it 'returns false' do - expect(subject.branches_exist?).to be_falsey + expect(subject.includes_branches?).to be_falsey end end @@ -32,7 +32,7 @@ describe ::Gitlab::GitPostReceive do end it 'returns true' do - expect(subject.branches_exist?).to be_truthy + expect(subject.includes_branches?).to be_truthy end end @@ -45,7 +45,7 @@ describe ::Gitlab::GitPostReceive do end it 'returns false' do - expect(subject.branches_exist?).to be_falsey + expect(subject.includes_branches?).to be_falsey end end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 01ccae194fb..081d95d4d79 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -61,16 +61,21 @@ describe PostReceive do end context "branches" do - let(:changes) { '123456 789012 refs/heads/tést' } + let(:changes) do + <<~EOF + '123456 789012 refs/heads/tést1' + '123456 789012 refs/heads/tést2' + EOF + end it 'expires the branches cache' do - expect(project.repository).to receive(:expire_branches_cache) + expect(project.repository).to receive(:expire_branches_cache).once described_class.new.perform(gl_repository, key_id, base64_changes) end it 'calls Git::BranchPushService' do - expect_next_instance_of(Git::BranchPushService) do |service| + expect_any_instance_of(Git::BranchPushService) do |service| expect(service).to receive(:execute).and_return(true) end -- cgit v1.2.1