From 56d4265e2789d13a3f96f6ef3231d9c856f52a82 Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Sat, 19 Apr 2025 19:21:07 -0400 Subject: [PATCH 1/4] Pull branch ref from Gitaly if not present in Redis Call Gitaly RefExists RPC when there is a branch ref not found in Redis cache. This will work as a fallback to validate the latest repository state. Track when we trigger the fallback to investigate why the cache is stale in some scenarios. Changelog: fixed --- app/models/concerns/has_repository.rb | 4 ++ app/services/merge_requests/build_service.rb | 24 +++++++- .../pull_ref_directly_from_gitaly.yml | 9 +++ lib/gitlab/ci/pipeline/chain/command.rb | 11 ++-- .../merge_requests/build_service_spec.rb | 56 +++++++++++++++++++ .../has_repository_shared_examples.rb | 15 +++++ 6 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml diff --git a/app/models/concerns/has_repository.rb b/app/models/concerns/has_repository.rb index c6f93f14b1d60d..770777b88e7fae 100644 --- a/app/models/concerns/has_repository.rb +++ b/app/models/concerns/has_repository.rb @@ -47,6 +47,10 @@ def branch_exists?(branch) repository.branch_exists?(branch) end + def ref_exists?(ref) + repository.ref_exists?(ref) + end + def commit_by(oid:) repository.commit_by(oid: oid) end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 9643457f5f5d98..67271909e6acd9 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -197,7 +197,29 @@ def same_source_and_target_project? end def source_branch_exists? - source_branch.blank? || source_project.branch_exists?(source_branch) + source_branch.blank? || source_project.branch_exists?(source_branch) || ref_exists_in_gitaly? + end + + def ref_exists_in_gitaly? + return false unless Feature.enabled?(:pull_ref_directly_from_gitaly, project) + + # The following check acts as a fallback if there is a mismatch between + # the cache and repository state. If the branch ref does not exist in the cache + # then we validate the repository state with Gitaly to avoid an inconsistent response. + exists = source_project.ref_exists?("refs/heads/#{source_branch}") + + return false unless exists + + # Only log when we find a ref that exists in Gitaly but was not found elsewhere + Gitlab::AppJsonLogger.info( + class: self.class.to_s, + method: __method__, + project_id: project.id, + source_branch: source_branch, + **Gitlab::ApplicationContext.current + ) + + true end def target_branch_exists? diff --git a/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml b/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml new file mode 100644 index 00000000000000..4c6044077cae3a --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml @@ -0,0 +1,9 @@ +--- +name: pull_ref_directly_from_gitaly +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/348565 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188492 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/536824 +milestone: '18.0' +group: group::source code +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index b1ef229c02227f..df784a3b9ea00a 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -43,10 +43,9 @@ def tag_exists? end def merge_request_ref_exists? - strong_memoize(:merge_request_ref_exists) do - MergeRequest.merge_request_ref?(origin_ref) && - project.repository.ref_exists?(origin_ref) - end + return check_merge_request_ref if Feature.enabled?(:pull_ref_directly_from_gitaly, project) + + strong_memoize(:merge_request_ref_exists) { check_merge_request_ref } end def ref @@ -166,6 +165,10 @@ def full_git_ref_name_unavailable? def gitlab_org_project? project.full_path == 'gitlab-org/gitlab' end + + def check_merge_request_ref + MergeRequest.merge_request_ref?(origin_ref) && project.repository.ref_exists?(origin_ref) + end end end end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 1c7ba90442a043..248af2d91f471a 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -14,6 +14,7 @@ let(:issue) { create(:issue, project: project, title: 'A bug', confidential: issue_confidential) } let(:description) { nil } let(:source_branch) { 'feature' } + let(:ref_path) { "refs/heads/#{source_branch}" } let(:target_branch) { 'master' } let(:milestone_id) { nil } let(:label_ids) { [] } @@ -136,6 +137,8 @@ def stub_compare end it 'assigns force_remove_source_branch' do + allow(source_project).to receive(:ref_exists?).with(ref_path).and_return(false) + expect(merge_request.force_remove_source_branch?).to be_truthy end @@ -145,6 +148,8 @@ def stub_compare end it 'assigns force_remove_source_branch' do + allow(source_project).to receive(:ref_exists?).with(ref_path).and_return(false) + expect(merge_request.force_remove_source_branch?).to be_truthy end end @@ -543,6 +548,7 @@ def stub_compare before do allow(project).to receive(:branch_exists?).with(target_branch).and_return(true) allow(project).to receive(:branch_exists?).with(source_branch).and_return(false) + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) end it_behaves_like 'forbids the merge request from being created' do @@ -550,6 +556,55 @@ def stub_compare end end + context 'when a source branch ref exists in Gitaly but not in the ref cache' do + let(:log_arguments) do + { + class: 'MergeRequests::BuildService', + method: :ref_exists_in_gitaly?, + project_id: project.id, + source_branch: source_branch + } + end + + before do + allow(service).to receive(:source_project).and_return(project) + allow(project).to receive(:branch_exists?).with(target_branch).and_return(true) + allow(project).to receive(:branch_exists?).with(source_branch).and_return(false) + end + + context 'and the pull_ref_directly_from_gitaly FF is disabled' do + before do + stub_feature_flags(pull_ref_directly_from_gitaly: false) + end + + it 'does not check for refs in Gitaly' do + expect(project).not_to receive(:ref_exists?).with(ref_path) + + service.execute + end + end + + context 'and the pull_ref_directly_from_gitaly FF is enabled' do + it 'logs info when the ref exists in Gitaly' do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(true) + + expect(project).to receive(:ref_exists?).with(ref_path) + expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(**log_arguments)) + + service.execute + end + + it 'does not log info when the ref does not exist in Gitaly' do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) + + expect(project).to receive(:ref_exists?).with(ref_path) + expect(Gitlab::AppJsonLogger).not_to receive(:info).with(hash_including(**log_arguments)) + + service.execute + end + end + end + context 'target branch does not exist' do before do allow(project).to receive(:branch_exists?).with(target_branch).and_return(false) @@ -563,6 +618,7 @@ def stub_compare context 'both source and target branches do not exist' do before do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) allow(project).to receive(:branch_exists?).and_return(false) end diff --git a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb index a127451fdb68a9..070864d673b17e 100644 --- a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb @@ -139,6 +139,21 @@ it { expect(container.repo_exists?).to be(true) } end + describe '#ref_exists?' do + let(:ref_path) { 'refs/heads/master' } + let(:non_existent_ref_path) { 'refs/heads/foo' } + + before do + allow_next_instance_of(Gitlab::GitalyClient::RefService) do |ref_service| + allow(ref_service).to receive(:ref_exists?).with(ref_path).and_return(true) + allow(ref_service).to receive(:ref_exists?).with(non_existent_ref_path).and_return(false) + end + end + + it { expect(container.ref_exists?(ref_path)).to be(true) } + it { expect(container.ref_exists?(non_existent_ref_path)).to be(false) } + end + describe '#root_ref' do let(:root_ref) { container.repository.root_ref } -- GitLab From 5bb795aeadda925d6b9ea5b8d41044d304f13c81 Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Tue, 22 Apr 2025 18:14:29 -0400 Subject: [PATCH 2/4] Add pull_ref_directly_from_gitaly validation in command_spec.rb --- .../gitlab/ci/pipeline/chain/command_spec.rb | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index d1b3de4c41827b..ae7568118dcedd 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -126,17 +126,36 @@ subject { command.merge_request_ref_exists? } let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:origin_ref) { merge_request.source_branch } - context 'for existing merge request ref' do - let(:origin_ref) { merge_request.ref_path } + context 'when pull_ref_directly_from_gitaly feature flag is enabled' do + context 'for existing merge request ref' do + let(:origin_ref) { merge_request.ref_path } - it { is_expected.to eq(true) } + it { is_expected.to eq(true) } + end + + context 'for branch ref' do + it { is_expected.to eq(false) } + end + + it 'does not memoize the result' do + expect(command).to receive(:check_merge_request_ref).twice + + 2.times { command.merge_request_ref_exists? } + end end - context 'for branch ref' do - let(:origin_ref) { merge_request.source_branch } + context 'when pull_ref_directly_from_gitaly feature flag is disabled' do + before do + stub_feature_flags(pull_ref_directly_from_gitaly: false) + end - it { is_expected.to eq(false) } + it 'memoizes the result' do + expect(command).to receive(:check_merge_request_ref).once + + 2.times { command.merge_request_ref_exists? } + end end end -- GitLab From 7db142fb88760be3cdf108ca77cdd845f21bb36a Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Fri, 2 May 2025 09:21:29 -0400 Subject: [PATCH 3/4] Apply suggestions to build_service_spec.rb --- .../merge_requests/build_service_spec.rb | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 248af2d91f471a..ebc94af25753a7 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -134,11 +134,11 @@ def stub_compare before do project.add_reporter(user) - end - it 'assigns force_remove_source_branch' do allow(source_project).to receive(:ref_exists?).with(ref_path).and_return(false) + end + it 'assigns force_remove_source_branch' do expect(merge_request.force_remove_source_branch?).to be_truthy end @@ -148,8 +148,6 @@ def stub_compare end it 'assigns force_remove_source_branch' do - allow(source_project).to receive(:ref_exists?).with(ref_path).and_return(false) - expect(merge_request.force_remove_source_branch?).to be_truthy end end @@ -572,33 +570,39 @@ def stub_compare allow(project).to receive(:branch_exists?).with(source_branch).and_return(false) end - context 'and the pull_ref_directly_from_gitaly FF is disabled' do + context 'when the ref exists in Gitaly' do before do - stub_feature_flags(pull_ref_directly_from_gitaly: false) + allow(project).to receive(:ref_exists?).with(ref_path).and_return(true) end - it 'does not check for refs in Gitaly' do - expect(project).not_to receive(:ref_exists?).with(ref_path) + it 'logs info' do + expect(project).to receive(:ref_exists?).with(ref_path) + expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(**log_arguments)) service.execute end end - context 'and the pull_ref_directly_from_gitaly FF is enabled' do - it 'logs info when the ref exists in Gitaly' do - allow(project).to receive(:ref_exists?).with(ref_path).and_return(true) + context 'when the ref does not exist in Gitaly' do + before do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) + end + it 'does not log info' do expect(project).to receive(:ref_exists?).with(ref_path) - expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(**log_arguments)) + expect(Gitlab::AppJsonLogger).not_to receive(:info).with(hash_including(**log_arguments)) service.execute end + end - it 'does not log info when the ref does not exist in Gitaly' do - allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) + context 'and the pull_ref_directly_from_gitaly FF is disabled' do + before do + stub_feature_flags(pull_ref_directly_from_gitaly: false) + end - expect(project).to receive(:ref_exists?).with(ref_path) - expect(Gitlab::AppJsonLogger).not_to receive(:info).with(hash_including(**log_arguments)) + it 'does not check for refs in Gitaly' do + expect(project).not_to receive(:ref_exists?).with(ref_path) service.execute end -- GitLab From 090729b4a0f7aa1d83f64e14828b2cb2f11373ba Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Fri, 2 May 2025 09:46:55 -0400 Subject: [PATCH 4/4] Apply suggestions to command_spec.rb --- .../gitlab/ci/pipeline/chain/command_spec.rb | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index ae7568118dcedd..fd94bc72202a67 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -128,22 +128,20 @@ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:origin_ref) { merge_request.source_branch } - context 'when pull_ref_directly_from_gitaly feature flag is enabled' do - context 'for existing merge request ref' do - let(:origin_ref) { merge_request.ref_path } + context 'for existing merge request ref' do + let(:origin_ref) { merge_request.ref_path } - it { is_expected.to eq(true) } - end + it { is_expected.to eq(true) } + end - context 'for branch ref' do - it { is_expected.to eq(false) } - end + context 'for branch ref' do + it { is_expected.to eq(false) } + end - it 'does not memoize the result' do - expect(command).to receive(:check_merge_request_ref).twice + it 'does not memoize the result' do + expect(command).to receive(:check_merge_request_ref).twice - 2.times { command.merge_request_ref_exists? } - end + 2.times { command.merge_request_ref_exists? } end context 'when pull_ref_directly_from_gitaly feature flag is disabled' do -- GitLab