From 18f589e34e0b57b4ebb040414e59b2a64f4e38e6 Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Mon, 29 Sep 2025 22:19:02 -0300 Subject: [PATCH] Fix stale ref cache in pipeline repository validation Implements Gitaly fallback when Redis branch/tag cache is stale, preventing pipeline creation failures for valid branches. Adds cache-miss detection with automatic refresh while maintaining performance for cache hits. --- ...nches_and_tags_without_cache_pipelines.yml | 10 + .../two_merge_requests_on_train_spec.rb | 4 + lib/gitlab/ci/pipeline/chain/command.rb | 71 +++++ .../gitlab/ci/pipeline/chain/command_spec.rb | 283 +++++++++++++++++- 4 files changed, 365 insertions(+), 3 deletions(-) create mode 100644 config/feature_flags/beta/branches_and_tags_without_cache_pipelines.yml diff --git a/config/feature_flags/beta/branches_and_tags_without_cache_pipelines.yml b/config/feature_flags/beta/branches_and_tags_without_cache_pipelines.yml new file mode 100644 index 00000000000000..86236450995d4b --- /dev/null +++ b/config/feature_flags/beta/branches_and_tags_without_cache_pipelines.yml @@ -0,0 +1,10 @@ +--- +name: branches_and_tags_without_cache_pipelines +description: Use Gitaly calls directly for branches and tags in pipelines creation +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/572341 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/206806 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/573848 +milestone: '18.6' +group: group::source code +type: beta +default_enabled: false diff --git a/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb b/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb index 0064e4e74e1c44..bff97b50528479 100644 --- a/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb +++ b/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb @@ -29,6 +29,10 @@ end before do + # The FF branches_and_tags_without_cache_pipelines needs to be disabled + # until we can diagnose and avoid the multiple ListRefs Gitaly calls + # when moving away from branch_names cache from Redis. + stub_feature_flags(branches_and_tags_without_cache_pipelines: false) project.add_maintainer(maintainer_1) project.add_maintainer(maintainer_2) stub_licensed_features(merge_pipelines: true, merge_trains: true) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 31ceb0ffb76f1e..c4b60220b1e654 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -31,12 +31,70 @@ def linting? end def branch_exists? + if ::Feature.enabled?(:branches_and_tags_without_cache_pipelines, project) + branch_exists_without_cache? + else + branch_exists_with_cache? + end + end + + def branch_exists_without_cache? + strong_memoize(:is_branch_without_cache) do + if branch_ref? + # First check if branch exists using the faster branch_exists? method + if project.repository.branch_exists?(ref) + true + else + # If branch doesn't exist in cache, expire the branches cache + # and then check again with fresh data from Gitaly + project.repository.expire_branches_cache + result = project.repository.branch_exists?(ref) + log_cache_mismatch(branch_ref: true) if result + + result + end + else + false + end + end + end + + def branch_exists_with_cache? strong_memoize(:is_branch) do branch_ref? && project.repository.branch_exists?(ref) end end def tag_exists? + if ::Feature.enabled?(:branches_and_tags_without_cache_pipelines, project) + tag_exists_without_cache? + else + tag_exists_with_cache? + end + end + + def tag_exists_without_cache? + strong_memoize(:is_tag_without_cache) do + if tag_ref? + # First check if tag exists using the faster tag_exists? method + if project.repository.tag_exists?(ref) + true + else + # If tag doesn't exist in cache, expire the tags cache + # and then check again with fresh data from Gitaly + project.repository.expire_tags_cache + result = project.repository.tag_exists?(ref) + log_cache_mismatch(tag_ref: true) if result + + result + end + else + false + end + end + end + + def tag_exists_with_cache? strong_memoize(:is_tag) do tag_ref? && project.repository.tag_exists?(ref) end @@ -136,6 +194,19 @@ def increment_pipeline_failure_reason_counter(reason) private + def log_cache_mismatch(branch_ref: false, tag_ref: false) + Gitlab::AppJsonLogger.info( + message: 'Branch or tag exists with cache mismatch', + class: self.class.name, + namespace_id: project.namespace_id, + project_id: project.id, + ref: ref, + branch_ref: branch_ref, + tag_ref: tag_ref, + **Gitlab::ApplicationContext.current + ) + end + # Verifies that origin_ref is a fully qualified tag reference (refs/tags/) # # Fallbacks to `true` for backward compatibility reasons diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 3cf17dbf90362b..719cfff69827fa 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -74,7 +74,23 @@ context 'for existing branch' do let(:origin_ref) { 'master' } - it { is_expected.to eq(true) } + it 'uses non-cached branch check' do + expect(command).to receive(:branch_exists_without_cache?).and_call_original + expect(command).not_to receive(:branch_exists_with_cache?) + + is_expected.to eq(true) + end + end + + context 'for non-existing branch' do + let(:origin_ref) { 'non-existing-branch' } + + it 'uses non-cached branch check' do + expect(command).to receive(:branch_exists_without_cache?).and_call_original + expect(command).not_to receive(:branch_exists_with_cache?) + + is_expected.to eq(false) + end end context 'for fully described tag ref' do @@ -94,15 +110,154 @@ it { is_expected.to eq(false) } end + + context 'when branches_and_tags_without_cache_pipelines feature flag is disabled' do + before do + stub_feature_flags(branches_and_tags_without_cache_pipelines: false) + end + + context 'for existing branch' do + let(:origin_ref) { 'master' } + + it 'uses cached branch check' do + expect(command).to receive(:branch_exists_with_cache?).and_call_original + expect(command).not_to receive(:branch_exists_without_cache?) + + is_expected.to eq(true) + end + end + + context 'for non-existing branch' do + let(:origin_ref) { 'non-existing-branch' } + + it 'uses cached branch check' do + expect(command).to receive(:branch_exists_with_cache?).and_call_original + expect(command).not_to receive(:branch_exists_without_cache?) + + is_expected.to eq(false) + end + end + end + end + + describe '#branch_exists_with_cache?' do + subject { command.branch_exists_with_cache? } + + context 'when origin_ref is a branch reference' do + let(:origin_ref) { 'refs/heads/master' } + + it 'checks branch existence using cached method' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(true) + + is_expected.to eq(true) + end + end + + context 'when origin_ref is not a branch reference' do + let(:origin_ref) { 'refs/tags/v1.0.0' } + + it { is_expected.to eq(false) } + end + + context 'when origin_ref is a short ref' do + let(:origin_ref) { 'master' } + + it 'falls back to true for backward compatibility' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(true) + + is_expected.to eq(true) + end + end + end + + describe '#branch_exists_without_cache?' do + let(:origin_ref) { 'refs/heads/master' } + + subject { command.branch_exists_without_cache? } + + context 'when origin_ref is a branch reference' do + context 'when branch exists in cache' do + it 'returns true without expiring cache' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(true) + expect(project.repository).not_to receive(:expire_branches_cache) + + is_expected.to eq(true) + end + end + + context 'when branch does not exist in cache' do + it 'expires cache and checks again' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(false, true) + expect(project.repository).to receive(:expire_branches_cache).once + + is_expected.to eq(true) + end + end + + context 'when branch does not exist at all' do + it 'expires cache and returns false' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(false, false) + expect(project.repository).to receive(:expire_branches_cache).once + + is_expected.to eq(false) + end + end + end + + context 'when origin_ref is not a branch reference' do + let(:origin_ref) { 'refs/tags/v1.0.0' } + + it 'returns false without checking repository' do + expect(project.repository).not_to receive(:branch_exists?) + expect(project.repository).not_to receive(:expire_branches_cache) + + is_expected.to eq(false) + end + end + + context 'when origin_ref is a short ref' do + let(:origin_ref) { 'master' } + + it 'falls back to true for backward compatibility and checks repository' do + expect(project.repository).to receive(:branch_exists?).with('master').and_return(true) + + is_expected.to eq(true) + end + end + + it 'memoizes the result' do + allow(project.repository).to receive(:branch_exists?).and_return(true) + + expect(command.branch_exists_without_cache?).to eq(true) + expect(command.branch_exists_without_cache?).to eq(true) + + expect(project.repository).to have_received(:branch_exists?).once + end end describe '#tag_exists?' do subject { command.tag_exists? } - context 'for existing ref' do + context 'for existing tag' do let(:origin_ref) { 'v1.0.0' } - it { is_expected.to eq(true) } + it 'uses non-cached tag check' do + expect(command).to receive(:tag_exists_without_cache?).and_call_original + expect(command).not_to receive(:tag_exists_with_cache?) + + is_expected.to eq(true) + end + end + + context 'for non-existing tag' do + let(:origin_ref) { 'non-existing-tag' } + + it 'uses non-cached tag check' do + expect(command).to receive(:tag_exists_without_cache?).and_call_original + expect(command).not_to receive(:tag_exists_with_cache?) + + is_expected.to eq(false) + end end context 'for fully described tag ref' do @@ -122,6 +277,128 @@ it { is_expected.to eq(false) } end + + context 'when branches_and_tags_without_cache_pipelines feature flag is disabled' do + before do + stub_feature_flags(branches_and_tags_without_cache_pipelines: false) + end + + context 'for existing tag' do + let(:origin_ref) { 'v1.0.0' } + + it 'uses cached tag check' do + expect(command).to receive(:tag_exists_with_cache?).and_call_original + expect(command).not_to receive(:tag_exists_without_cache?) + + is_expected.to eq(true) + end + end + + context 'for non-existing tag' do + let(:origin_ref) { 'non-existing-tag' } + + it 'uses cached tag check' do + expect(command).to receive(:tag_exists_with_cache?).and_call_original + expect(command).not_to receive(:tag_exists_without_cache?) + + is_expected.to eq(false) + end + end + end + end + + describe '#tag_exists_with_cache?' do + subject { command.tag_exists_with_cache? } + + context 'when origin_ref is a tag reference' do + let(:origin_ref) { 'refs/tags/v1.0.0' } + + it 'checks tag existence using cached method' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(true) + + is_expected.to eq(true) + end + end + + context 'when origin_ref is not a tag reference' do + let(:origin_ref) { 'refs/heads/master' } + + it { is_expected.to eq(false) } + end + + context 'when origin_ref is a short ref' do + let(:origin_ref) { 'v1.0.0' } + + it 'falls back to true for backward compatibility' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(true) + + is_expected.to eq(true) + end + end + end + + describe '#tag_exists_without_cache?' do + let(:origin_ref) { 'refs/tags/v1.0.0' } + + subject { command.tag_exists_without_cache? } + + context 'when origin_ref is a tag reference' do + context 'when tag exists in cache' do + it 'returns true without expiring cache' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(true) + expect(project.repository).not_to receive(:expire_tags_cache) + + is_expected.to eq(true) + end + end + + context 'when tag does not exist in cache' do + it 'expires cache and checks again' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(false, true) + expect(project.repository).to receive(:expire_tags_cache).once + + is_expected.to eq(true) + end + end + + context 'when tag does not exist at all' do + it 'expires cache and returns false' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(false, false) + expect(project.repository).to receive(:expire_tags_cache).once + + is_expected.to eq(false) + end + end + end + + context 'when origin_ref is not a tag reference' do + let(:origin_ref) { 'refs/heads/master' } + + it 'returns false without checking repository' do + expect(project.repository).not_to receive(:tag_exists?) + + is_expected.to eq(false) + end + end + + context 'when origin_ref is a short ref' do + let(:origin_ref) { 'v1.0.0' } + + it 'falls back to true for backward compatibility and checks repository' do + expect(project.repository).to receive(:tag_exists?).with('v1.0.0').and_return(true) + + is_expected.to eq(true) + end + end + + it 'memoizes the result' do + allow(project.repository).to receive(:tag_exists?).and_return(true) + + expect(command.tag_exists_without_cache?).to eq(true) + expect(command.tag_exists_without_cache?).to eq(true) + + expect(project.repository).to have_received(:tag_exists?).once + end end describe '#merge_request_ref_exists?' do -- GitLab