Merge branch 'mc/feature/keep-latest-artifact-for-ref-fix' into 'master'

Refactor artifact locking

See merge request gitlab-org/gitlab!30741

app/models/ci/job_artifact.rb

@@ -7,10 +7,13 @@ module Ci
     include UpdateProjectStatistics
     include UsageStatistics
     include Sortable
+    include IgnorableColumns
     extend Gitlab::Ci::Model
 
     NotSupportedAdapterError = Class.new(StandardError)
 
+    ignore_columns :locked, remove_after: '2020-07-22', remove_with: '13.4'
+
     TEST_REPORT_FILE_TYPES = %w[junit].freeze
     COVERAGE_REPORT_FILE_TYPES = %w[cobertura].freeze
     ACCESSIBILITY_REPORT_FILE_TYPES = %w[accessibility].freeze
@@ -108,10 +111,6 @@ module Ci
 
     PLAN_LIMIT_PREFIX = 'ci_max_artifact_size_'
 
-    # This is required since we cannot add a default to the database
-    # https://gitlab.com/gitlab-org/gitlab/-/issues/215418
-    attribute :locked, :boolean, default: false
-
     belongs_to :project
     belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
 
@@ -130,7 +129,6 @@ module Ci
     scope :with_files_stored_locally, -> { where(file_store: ::JobArtifactUploader::Store::LOCAL) }
     scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
     scope :for_sha, ->(sha, project_id) { joins(job: :pipeline).where(ci_pipelines: { sha: sha, project_id: project_id }) }
-    scope :for_ref, ->(ref, project_id) { joins(job: :pipeline).where(ci_pipelines: { ref: ref, project_id: project_id }) }
     scope :for_job_name, ->(name) { joins(:job).where(ci_builds: { name: name }) }
 
     scope :with_file_types, -> (file_types) do
@@ -167,8 +165,7 @@ module Ci
 
     scope :expired, -> (limit) { where('expire_at < ?', Time.current).limit(limit) }
     scope :downloadable, -> { where(file_type: DOWNLOADABLE_TYPES) }
-    scope :locked, -> { where(locked: true) }
-    scope :unlocked, -> { where(locked: [false, nil]) }
+    scope :unlocked, -> { joins(job: :pipeline).merge(::Ci::Pipeline.unlocked).order(expire_at: :desc) }
 
     scope :scoped_project, -> { where('ci_job_artifacts.project_id = projects.id') }
 

app/models/ci/pipeline.rb

@@ -113,6 +113,8 @@ module Ci
     # extend this `Hash` with new values.
     enum failure_reason: ::Ci::PipelineEnums.failure_reasons
 
+    enum locked: { unlocked: 0, artifacts_locked: 1 }
+
     state_machine :status, initial: :created do
       event :enqueue do
         transition [:created, :manual, :waiting_for_resource, :preparing, :skipped, :scheduled] => :pending
@@ -247,6 +249,14 @@ module Ci
 
         pipeline.run_after_commit { AutoDevops::DisableWorker.perform_async(pipeline.id) }
       end
+
+      after_transition any => [:success] do |pipeline|
+        next unless Gitlab::Ci::Features.keep_latest_artifacts_for_ref_enabled?(pipeline.project)
+
+        pipeline.run_after_commit do
+          Ci::PipelineSuccessUnlockArtifactsWorker.perform_async(pipeline.id)
+        end
+      end
     end
 
     scope :internal, -> { where(source: internal_sources) }
@@ -260,6 +270,12 @@ module Ci
     scope :for_id, -> (id) { where(id: id) }
     scope :for_iid, -> (iid) { where(iid: iid) }
     scope :created_after, -> (time) { where('ci_pipelines.created_at > ?', time) }
+    scope :created_before_id, -> (id) { where('ci_pipelines.id < ?', id) }
+    scope :before_pipeline, -> (pipeline) { created_before_id(pipeline.id).outside_pipeline_family(pipeline) }
+
+    scope :outside_pipeline_family, ->(pipeline) do
+      where.not(id: pipeline.same_family_pipeline_ids)
+    end
 
     scope :with_reports, -> (reports_scope) do
       where('EXISTS (?)', ::Ci::Build.latest.with_reports(reports_scope).where('ci_pipelines.id=ci_builds.commit_id').select(1))
@@ -801,12 +817,16 @@ module Ci
     end
 
     # If pipeline is a child of another pipeline, include the parent
-    # and the siblings, otherwise return only itself.
+    # and the siblings, otherwise return only itself and children.
     def same_family_pipeline_ids
       if (parent = parent_pipeline)
-        [parent.id] + parent.child_pipelines.pluck(:id)
+        Ci::Pipeline.where(id: parent.id)
+          .or(Ci::Pipeline.where(id: parent.child_pipelines.select(:id)))
+          .select(:id)
       else
-        [self.id]
+        Ci::Pipeline.where(id: self.id)
+          .or(Ci::Pipeline.where(id: self.child_pipelines.select(:id)))
+          .select(:id)
       end
     end
 
@@ -897,6 +917,10 @@ module Ci
       end
     end
 
+    def has_archive_artifacts?
+      complete? && builds.latest.with_existing_job_artifacts(Ci::JobArtifact.archive.or(Ci::JobArtifact.metadata)).exists?
+    end
+
     def has_exposed_artifacts?
       complete? && builds.latest.with_exposed_artifacts.exists?
     end

app/services/branches/delete_service.rb

@@ -19,6 +19,7 @@ module Branches
       end
 
       if repository.rm_branch(current_user, branch_name)
+        unlock_artifacts(branch_name)
         ServiceResponse.success(message: 'Branch was deleted')
       else
         ServiceResponse.error(
@@ -28,5 +29,11 @@ module Branches
     rescue Gitlab::Git::PreReceiveError => ex
       ServiceResponse.error(message: ex.message, http_status: 400)
     end
+
+    private
+
+    def unlock_artifacts(branch_name)
+      Ci::RefDeleteUnlockArtifactsWorker.perform_async(project.id, current_user.id, "#{::Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}")
+    end
   end
 end

app/services/ci/create_job_artifacts_service.rb

@@ -104,11 +104,6 @@ module Ci
                               expire_in: expire_in)
                           end
 
-      if Feature.enabled?(:keep_latest_artifact_for_ref, project)
-        artifact.locked = true
-        artifact_metadata&.locked = true
-      end
-
       [artifact, artifact_metadata]
     end
 
@@ -128,7 +123,6 @@ module Ci
       Ci::JobArtifact.transaction do
         artifact.save!
         artifact_metadata&.save!
-        unlock_previous_artifacts!
 
         # NOTE: The `artifacts_expire_at` column is already deprecated and to be removed in the near future.
         job.update_column(:artifacts_expire_at, artifact.expire_at)
@@ -146,12 +140,6 @@ module Ci
       error(error.message, :bad_request)
     end
 
-    def unlock_previous_artifacts!
-      return unless Feature.enabled?(:keep_latest_artifact_for_ref, project)
-
-      Ci::JobArtifact.for_ref(job.ref, project.id).locked.update_all(locked: false)
-    end
-
     def sha256_matches_existing_artifact?(artifact_type, artifacts_file)
       existing_artifact = job.job_artifacts.find_by_file_type(artifact_type)
       return false unless existing_artifact

app/services/ci/destroy_expired_job_artifacts_service.rb

@@ -28,7 +28,7 @@ module Ci
     private
 
     def destroy_batch
-      artifact_batch = if Feature.enabled?(:keep_latest_artifact_for_ref)
+      artifact_batch = if Gitlab::Ci::Features.destroy_only_unlocked_expired_artifacts_enabled?
                          Ci::JobArtifact.expired(BATCH_SIZE).unlocked
                        else
                          Ci::JobArtifact.expired(BATCH_SIZE)

app/services/ci/unlock_artifacts_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class UnlockArtifactsService < ::BaseService
+    BATCH_SIZE = 100
+
+    def execute(ci_ref, before_pipeline = nil)
+      query = <<~SQL.squish
+        UPDATE "ci_pipelines"
+        SET    "locked" = #{::Ci::Pipeline.lockeds[:unlocked]}
+        WHERE  "ci_pipelines"."id" in (
+            #{collect_pipelines(ci_ref, before_pipeline).select(:id).to_sql}
+            LIMIT  #{BATCH_SIZE}
+            FOR  UPDATE SKIP LOCKED
+        )
+        RETURNING "ci_pipelines"."id";
+      SQL
+
+      loop do
+        break if ActiveRecord::Base.connection.exec_query(query).empty?
+      end
+    end
+
+    private
+
+    def collect_pipelines(ci_ref, before_pipeline)
+      pipeline_scope = ci_ref.pipelines
+      pipeline_scope = pipeline_scope.before_pipeline(before_pipeline) if before_pipeline
+
+      pipeline_scope.artifacts_locked
+    end
+  end
+end

app/services/git/branch_push_service.rb

@@ -29,6 +29,7 @@ module Git
       perform_housekeeping
 
       stop_environments
+      unlock_artifacts
 
       true
     end
@@ -60,6 +61,12 @@ module Git
       Ci::StopEnvironmentsService.new(project, current_user).execute(branch_name)
     end
 
+    def unlock_artifacts
+      return unless removing_branch?
+
+      Ci::RefDeleteUnlockArtifactsWorker.perform_async(project.id, current_user.id, ref)
+    end
+
     def execute_related_hooks
       BranchHooksService.new(project, current_user, params).execute
     end

app/services/git/tag_push_service.rb

@@ -10,7 +10,25 @@ module Git
       project.repository.before_push_tag
       TagHooksService.new(project, current_user, params).execute
 
+      unlock_artifacts
+
       true
     end
+
+    private
+
+    def unlock_artifacts
+      return unless removing_tag?
+
+      Ci::RefDeleteUnlockArtifactsWorker.perform_async(project.id, current_user.id, ref)
+    end
+
+    def removing_tag?
+      Gitlab::Git.blank_ref?(newrev)
+    end
+
+    def tag_name
+      Gitlab::Git.ref_name(ref)
+    end
   end
 end

app/services/tags/destroy_service.rb

@@ -18,6 +18,8 @@ module Tags
           .new(project, current_user, tag: tag_name)
           .execute
 
+        unlock_artifacts(tag_name)
+
         success('Tag was removed')
       else
         error('Failed to remove tag')
@@ -33,5 +35,11 @@ module Tags
     def success(message)
       super().merge(message: message)
     end
+
+    private
+
+    def unlock_artifacts(tag_name)
+      Ci::RefDeleteUnlockArtifactsWorker.perform_async(project.id, current_user.id, "#{::Gitlab::Git::TAG_REF_PREFIX}#{tag_name}")
+    end
   end
 end

app/workers/all_queues.yml

@@ -859,6 +859,22 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: pipeline_background:ci_pipeline_success_unlock_artifacts
+  :feature_category: :continuous_integration
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
+- :name: pipeline_background:ci_ref_delete_unlock_artifacts
+  :feature_category: :continuous_integration
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: pipeline_cache:expire_job_cache
   :feature_category: :continuous_integration
   :has_external_dependencies:

app/workers/ci/pipeline_success_unlock_artifacts_worker.rb

+# frozen_string_literal: true
+
+module Ci
+  class PipelineSuccessUnlockArtifactsWorker
+    include ApplicationWorker
+    include PipelineBackgroundQueue
+
+    idempotent!
+
+    def perform(pipeline_id)
+      ::Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline|
+        break unless pipeline.has_archive_artifacts?
+
+        ::Ci::UnlockArtifactsService
+          .new(pipeline.project, pipeline.user)
+          .execute(pipeline.ci_ref, pipeline)
+      end
+    end
+  end
+end

app/workers/ci/ref_delete_unlock_artifacts_worker.rb

+# frozen_string_literal: true
+
+module Ci
+  class RefDeleteUnlockArtifactsWorker
+    include ApplicationWorker
+    include PipelineBackgroundQueue
+
+    idempotent!
+
+    def perform(project_id, user_id, ref_path)
+      ::Project.find_by_id(project_id).try do |project|
+        ::User.find_by_id(user_id).try do |user|
+          ::Ci::Ref.find_by_ref_path(ref_path).try do |ci_ref|
+            ::Ci::UnlockArtifactsService
+              .new(project, user)
+              .execute(ci_ref)
+          end
+        end
+      end
+    end
+  end
+end

db/migrate/20200527211605_add_locked_to_ci_pipelines.rb

+# frozen_string_literal: true
+
+class AddLockedToCiPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :ci_pipelines, :locked, :integer, limit: 2, null: false, default: 0
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :ci_pipelines, :locked
+    end
+  end
+end

db/migrate/20200625174052_add_partial_index_to_locked_pipelines.rb

+# frozen_string_literal: true
+
+class AddPartialIndexToLockedPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :ci_pipelines, [:ci_ref_id, :id], name: 'idx_ci_pipelines_artifacts_locked', where: 'locked = 1'
+  end
+
+  def down
+    remove_concurrent_index :ci_pipelines, 'idx_ci_pipelines_artifacts_locked'
+  end
+end

db/structure.sql

@@ -10091,7 +10091,8 @@ CREATE TABLE public.ci_pipelines (
     source_sha bytea,
     target_sha bytea,
     external_pull_request_id bigint,
-    ci_ref_id bigint
+    ci_ref_id bigint,
+    locked smallint DEFAULT 0 NOT NULL
 );
 
 CREATE TABLE public.ci_pipelines_config (
@@ -18478,6 +18479,8 @@ CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_and_note_id_index ON public.ep
 
 CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_index ON public.epic_user_mentions USING btree (epic_id) WHERE (note_id IS NULL);
 
+CREATE INDEX idx_ci_pipelines_artifacts_locked ON public.ci_pipelines USING btree (ci_ref_id, id) WHERE (locked = 1);
+
 CREATE INDEX idx_deployment_clusters_on_cluster_id_and_kubernetes_namespace ON public.deployment_clusters USING btree (cluster_id, kubernetes_namespace);
 
 CREATE UNIQUE INDEX idx_deployment_merge_requests_unique_index ON public.deployment_merge_requests USING btree (deployment_id, merge_request_id);
@@ -23635,6 +23638,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200527152657
 20200527170649
 20200527211000
+20200527211605
 20200528054112
 20200528123703
 20200528125905
@@ -23710,6 +23714,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200625045442
 20200625082258
 20200625113337
+20200625174052
 20200625190458
 20200626060151
 20200626130220

ee/spec/requests/api/jobs_spec.rb

@@ -8,9 +8,9 @@ RSpec.describe API::Jobs do
   end
 
   let_it_be(:pipeline) do
-    create(:ci_empty_pipeline, project: project,
-                               sha: project.commit.id,
-                               ref: project.default_branch)
+    create(:ci_pipeline, project: project,
+                         sha: project.commit.id,
+                         ref: project.default_branch)
   end
 
   let(:developer) { create(:user) }

lib/gitlab/ci/features.rb

@@ -53,6 +53,14 @@ module Gitlab
       def self.raise_job_rules_without_workflow_rules_warning?
         ::Feature.enabled?(:ci_raise_job_rules_without_workflow_rules_warning)
       end
+
+      def self.keep_latest_artifacts_for_ref_enabled?(project)
+        ::Feature.enabled?(:keep_latest_artifacts_for_ref, project, default_enabled: false)
+      end
+
+      def self.destroy_only_unlocked_expired_artifacts_enabled?
+        ::Feature.enabled?(:destroy_only_unlocked_expired_artifacts, default_enabled: false)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline/chain/build.rb

@@ -20,7 +20,11 @@ module Gitlab
               pipeline_schedule: @command.schedule,
               merge_request: @command.merge_request,
               external_pull_request: @command.external_pull_request,
-              variables_attributes: Array(@command.variables_attributes)
+              variables_attributes: Array(@command.variables_attributes),
+              # This should be removed and set on the database column default
+              # level when the keep_latest_artifacts_for_ref feature flag is
+              # removed.
+              locked: ::Gitlab::Ci::Features.keep_latest_artifacts_for_ref_enabled?(@command.project) ? :artifacts_locked : :unlocked
             )
           end
 

lib/gitlab/import_export/project/import_export.yml

@@ -309,6 +309,7 @@ excluded_attributes:
     - :merge_request_id
     - :external_pull_request_id
     - :ci_ref_id
+    - :locked
   stages:
     - :pipeline_id
   merge_access_levels:

spec/models/ci/job_artifact_spec.rb

@@ -174,18 +174,6 @@ RSpec.describe Ci::JobArtifact do
     end
   end
 
-  describe '.for_ref' do
-    let(:first_pipeline) { create(:ci_pipeline, ref: 'first_ref') }
-    let(:second_pipeline) { create(:ci_pipeline, ref: 'second_ref', project: first_pipeline.project) }
-    let!(:first_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: first_pipeline)) }
-    let!(:second_artifact) { create(:ci_job_artifact, job: create(:ci_build, pipeline: second_pipeline)) }
-
-    it 'returns job artifacts for a given pipeline ref' do
-      expect(described_class.for_ref(first_pipeline.ref, first_pipeline.project.id)).to eq([first_artifact])
-      expect(described_class.for_ref(second_pipeline.ref, first_pipeline.project.id)).to eq([second_artifact])
-    end
-  end
-
   describe '.for_job_name' do
     it 'returns job artifacts for a given job name' do
       first_job = create(:ci_build, name: 'first')

spec/models/ci/pipeline_spec.rb

@@ -219,6 +219,50 @@ RSpec.describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '.outside_pipeline_family' do
+    subject(:outside_pipeline_family) { described_class.outside_pipeline_family(upstream_pipeline) }
+
+    let(:upstream_pipeline) { create(:ci_pipeline, project: project) }
+    let(:child_pipeline) { create(:ci_pipeline, project: project) }
+
+    let!(:other_pipeline) { create(:ci_pipeline, project: project) }
+
+    before do
+      create(:ci_sources_pipeline,
+             source_job: create(:ci_build, pipeline: upstream_pipeline),
+             source_project: project,
+             pipeline: child_pipeline,
+             project: project)
+    end
+
+    it 'only returns pipelines outside pipeline family' do
+      expect(outside_pipeline_family).to contain_exactly(other_pipeline)
+    end
+  end
+
+  describe '.before_pipeline' do
+    subject(:before_pipeline) { described_class.before_pipeline(child_pipeline) }
+
+    let!(:older_other_pipeline) { create(:ci_pipeline, project: project) }
+
+    let!(:upstream_pipeline) { create(:ci_pipeline, project: project) }
+    let!(:child_pipeline) { create(:ci_pipeline, project: project) }
+
+    let!(:other_pipeline) { create(:ci_pipeline, project: project) }
+
+    before do
+      create(:ci_sources_pipeline,
+             source_job: create(:ci_build, pipeline: upstream_pipeline),
+             source_project: project,
+             pipeline: child_pipeline,
+             project: project)
+    end
+
+    it 'only returns older pipelines outside pipeline family' do
+      expect(before_pipeline).to contain_exactly(older_other_pipeline)
+    end
+  end
+
   describe '#merge_request?' do
     let(:pipeline) { create(:ci_pipeline, merge_request: merge_request) }
     let(:merge_request) { create(:merge_request) }
@@ -2635,6 +2679,55 @@ RSpec.describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#same_family_pipeline_ids' do
+    subject(:same_family_pipeline_ids) { pipeline.same_family_pipeline_ids }
+
+    context 'when pipeline is not child nor parent' do
+      it 'returns just the pipeline id' do
+        expect(same_family_pipeline_ids).to contain_exactly(pipeline)
+      end
+    end
+
+    context 'when pipeline is child' do
+      let(:parent) { create(:ci_pipeline, project: pipeline.project) }
+      let(:sibling) { create(:ci_pipeline, project: pipeline.project) }
+
+      before do
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: parent),
+               source_project: parent.project,
+               pipeline: pipeline,
+               project: pipeline.project)
+
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: parent),
+               source_project: parent.project,
+               pipeline: sibling,
+               project: sibling.project)
+      end
+
+      it 'returns parent sibling and self ids' do
+        expect(same_family_pipeline_ids).to contain_exactly(parent, pipeline, sibling)
+      end
+    end
+
+    context 'when pipeline is parent' do
+      let(:child) { create(:ci_pipeline, project: pipeline.project) }
+
+      before do
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: pipeline),
+               source_project: pipeline.project,
+               pipeline: child,
+               project: child.project)
+      end
+
+      it 'returns self and child ids' do
+        expect(same_family_pipeline_ids).to contain_exactly(pipeline, child)
+      end
+    end
+  end
+
   describe '#stuck?' do
     before do
       create(:ci_build, :pending, pipeline: pipeline)
@@ -3179,6 +3272,32 @@ RSpec.describe Ci::Pipeline, :mailer do
         end
       end
     end
+
+    context 'when transitioning to success' do
+      context 'when feature is enabled' do
+        before do
+          stub_feature_flags(keep_latest_artifacts_for_ref: true)
+        end
+
+        it 'calls the PipelineSuccessUnlockArtifactsWorker' do
+          expect(Ci::PipelineSuccessUnlockArtifactsWorker).to receive(:perform_async).with(pipeline.id)
+
+          pipeline.succeed!
+        end
+      end
+
+      context 'when feature is disabled' do
+        before do
+          stub_feature_flags(keep_latest_artifacts_for_ref: false)
+        end
+
+        it 'does not call the PipelineSuccessUnlockArtifactsWorker' do
+          expect(Ci::PipelineSuccessUnlockArtifactsWorker).not_to receive(:perform_async)
+
+          pipeline.succeed!
+        end
+      end
+    end
   end
 
   describe '#default_branch?' do

spec/requests/api/jobs_spec.rb

@@ -36,9 +36,9 @@ RSpec.describe API::Jobs do
   end
 
   let_it_be(:pipeline, reload: true) do
-    create(:ci_empty_pipeline, project: project,
-                               sha: project.commit.id,
-                               ref: project.default_branch)
+    create(:ci_pipeline, project: project,
+                         sha: project.commit.id,
+                         ref: project.default_branch)
   end
 
   let!(:job) do

spec/services/branches/delete_service_spec.rb

@@ -10,6 +10,10 @@ RSpec.describe Branches::DeleteService do
   subject(:service) { described_class.new(project, user) }
 
   shared_examples 'a deleted branch' do |branch_name|
+    before do
+      allow(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async)
+    end
+
     it 'removes the branch' do
       expect(branch_exists?(branch_name)).to be true
 
@@ -18,6 +22,12 @@ RSpec.describe Branches::DeleteService do
       expect(result.status).to eq :success
       expect(branch_exists?(branch_name)).to be false
     end
+
+    it 'calls the RefDeleteUnlockArtifactsWorker' do
+      expect(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async).with(project.id, user.id, "refs/heads/#{branch_name}")
+
+      service.execute(branch_name)
+    end
   end
 
   describe '#execute' do

spec/services/ci/create_job_artifacts_service_spec.rb

@@ -30,26 +30,6 @@ RSpec.describe Ci::CreateJobArtifactsService do
   describe '#execute' do
     subject { service.execute(artifacts_file, params, metadata_file: metadata_file) }
 
-    context 'locking' do
-      let(:old_job) { create(:ci_build, pipeline: create(:ci_pipeline, project: job.project, ref: job.ref)) }
-      let!(:latest_artifact) { create(:ci_job_artifact, job: old_job, locked: true) }
-      let!(:other_artifact) { create(:ci_job_artifact, locked: true) }
-
-      it 'locks the new artifact' do
-        subject
-
-        expect(Ci::JobArtifact.last).to have_attributes(locked: true)
-      end
-
-      it 'unlocks all other artifacts for the same ref' do
-        expect { subject }.to change { latest_artifact.reload.locked }.from(true).to(false)
-      end
-
-      it 'does not unlock artifacts for other refs' do
-        expect { subject }.not_to change { other_artifact.reload.locked }.from(true)
-      end
-    end
-
     context 'when artifacts file is uploaded' do
       it 'saves artifact for the given type' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(1)

spec/services/ci/destroy_expired_job_artifacts_service_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     context 'when artifact is expired' do
       context 'when artifact is not locked' do
         before do
-          artifact.update!(locked: false)
+          artifact.job.pipeline.unlocked!
         end
 
         it 'destroys job artifact' do
@@ -24,7 +24,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
 
       context 'when artifact is locked' do
         before do
-          artifact.update!(locked: true)
+          artifact.job.pipeline.artifacts_locked!
         end
 
         it 'does not destroy job artifact' do

spec/services/ci/unlock_artifacts_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::UnlockArtifactsService do
+  describe '#execute' do
+    subject(:execute) { described_class.new(pipeline.project, pipeline.user).execute(ci_ref, before_pipeline) }
+
+    before do
+      stub_const("#{described_class}::BATCH_SIZE", 1)
+    end
+
+    [true, false].each do |tag|
+      context "when tag is #{tag}" do
+        let(:ref) { 'master' }
+        let(:ref_path) { tag ? "#{::Gitlab::Git::TAG_REF_PREFIX}#{ref}" : "#{::Gitlab::Git::BRANCH_REF_PREFIX}#{ref}" }
+        let(:ci_ref) { create(:ci_ref, ref_path: ref_path) }
+
+        let!(:old_unlocked_pipeline) { create(:ci_pipeline, ref: ref, tag: tag, project: ci_ref.project, locked: :unlocked) }
+        let!(:older_pipeline) { create(:ci_pipeline, ref: ref, tag: tag, project: ci_ref.project, locked: :artifacts_locked) }
+        let!(:older_ambiguous_pipeline) { create(:ci_pipeline, ref: ref, tag: !tag, project: ci_ref.project, locked: :artifacts_locked) }
+        let!(:pipeline) { create(:ci_pipeline, ref: ref, tag: tag, project: ci_ref.project, locked: :artifacts_locked) }
+        let!(:child_pipeline) { create(:ci_pipeline, ref: ref, tag: tag, project: ci_ref.project, locked: :artifacts_locked) }
+        let!(:newer_pipeline) { create(:ci_pipeline, ref: ref, tag: tag, project: ci_ref.project, locked: :artifacts_locked) }
+        let!(:other_ref_pipeline) { create(:ci_pipeline, ref: 'other_ref', tag: tag, project: ci_ref.project, locked: :artifacts_locked) }
+
+        before do
+          create(:ci_sources_pipeline,
+                 source_job: create(:ci_build, pipeline: pipeline),
+                 source_project: ci_ref.project,
+                 pipeline: child_pipeline,
+                 project: ci_ref.project)
+        end
+
+        context 'when running on a ref before a pipeline' do
+          let(:before_pipeline) { pipeline }
+
+          it 'unlocks artifacts from older pipelines' do
+            expect { execute }.to change { older_pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+          end
+
+          it 'does not unlock artifacts for tag or branch with same name as ref' do
+            expect { execute }.not_to change { older_ambiguous_pipeline.reload.locked }.from('artifacts_locked')
+          end
+
+          it 'does not unlock artifacts from newer pipelines' do
+            expect { execute }.not_to change { newer_pipeline.reload.locked }.from('artifacts_locked')
+          end
+
+          it 'does not lock artifacts from old unlocked pipelines' do
+            expect { execute }.not_to change { old_unlocked_pipeline.reload.locked }.from('unlocked')
+          end
+
+          it 'does not unlock artifacts from the same pipeline' do
+            expect { execute }.not_to change { pipeline.reload.locked }.from('artifacts_locked')
+          end
+
+          it 'does not unlock artifacts for other refs' do
+            expect { execute }.not_to change { other_ref_pipeline.reload.locked }.from('artifacts_locked')
+          end
+
+          it 'does not unlock artifacts for child pipeline' do
+            expect { execute }.not_to change { child_pipeline.reload.locked }.from('artifacts_locked')
+          end
+        end
+
+        context 'when running on just the ref' do
+          let(:before_pipeline) { nil }
+
+          it 'unlocks artifacts from older pipelines' do
+            expect { execute }.to change { older_pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+          end
+
+          it 'unlocks artifacts from newer pipelines' do
+            expect { execute }.to change { newer_pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+          end
+
+          it 'unlocks artifacts from the same pipeline' do
+            expect { execute }.to change { pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+          end
+
+          it 'does not unlock artifacts for tag or branch with same name as ref' do
+            expect { execute }.not_to change { older_ambiguous_pipeline.reload.locked }.from('artifacts_locked')
+          end
+
+          it 'does not lock artifacts from old unlocked pipelines' do
+            expect { execute }.not_to change { old_unlocked_pipeline.reload.locked }.from('unlocked')
+          end
+
+          it 'does not unlock artifacts for other refs' do
+            expect { execute }.not_to change { other_ref_pipeline.reload.locked }.from('artifacts_locked')
+          end
+        end
+      end
+    end
+  end
+end

spec/services/git/branch_push_service_spec.rb

@@ -635,6 +635,37 @@ RSpec.describe Git::BranchPushService, services: true do
     end
   end
 
+  describe 'artifacts' do
+    context 'create branch' do
+      let(:oldrev) { blankrev }
+
+      it 'does nothing' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async)
+
+        execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+      end
+    end
+
+    context 'update branch' do
+      it 'does nothing' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async)
+
+        execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+      end
+    end
+
+    context 'delete branch' do
+      let(:newrev) { blankrev }
+
+      it 'unlocks artifacts' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker)
+          .to receive(:perform_async).with(project.id, user.id, "refs/heads/#{branch}")
+
+        execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+      end
+    end
+  end
+
   describe 'Hooks' do
     context 'run on a branch' do
       it 'delegates to Git::BranchHooksService' do

spec/services/git/tag_push_service_spec.rb

@@ -10,9 +10,11 @@ RSpec.describe Git::TagPushService do
   let(:project) { create(:project, :repository) }
   let(:service) { described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) }
 
-  let(:oldrev) { Gitlab::Git::BLANK_SHA }
+  let(:blankrev) { Gitlab::Git::BLANK_SHA }
+  let(:oldrev) { blankrev }
   let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
-  let(:ref) { 'refs/tags/v1.1.0' }
+  let(:tag)   { 'v1.1.0' }
+  let(:ref) { "refs/tags/#{tag}" }
 
   describe "Push tags" do
     subject do
@@ -58,4 +60,35 @@ RSpec.describe Git::TagPushService do
       end
     end
   end
+
+  describe 'artifacts' do
+    context 'create tag' do
+      let(:oldrev) { blankrev }
+
+      it 'does nothing' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
+    context 'update tag' do
+      it 'does nothing' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
+    context 'delete tag' do
+      let(:newrev) { blankrev }
+
+      it 'unlocks artifacts' do
+        expect(::Ci::RefDeleteUnlockArtifactsWorker)
+          .to receive(:perform_async).with(project.id, user.id, "refs/tags/#{tag}")
+
+        service.execute
+      end
+    end
+  end
 end

spec/services/tags/destroy_service_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe Tags::DestroyService do
   describe '#execute' do
     subject { service.execute(tag_name) }
 
+    before do
+      allow(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async)
+    end
+
     it 'removes the tag' do
       expect(repository).to receive(:before_remove_tag)
       expect(service).to receive(:success)
@@ -18,6 +22,12 @@ RSpec.describe Tags::DestroyService do
       service.execute('v1.1.0')
     end
 
+    it 'calls the RefDeleteUnlockArtifactsWorker' do
+      expect(Ci::RefDeleteUnlockArtifactsWorker).to receive(:perform_async).with(project.id, user.id, 'refs/tags/v1.1.0')
+
+      service.execute('v1.1.0')
+    end
+
     context 'when there is an associated release on the tag' do
       let(:tag) { repository.tags.first }
       let(:tag_name) { tag.name }

spec/workers/ci/pipeline_success_unlock_artifacts_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineSuccessUnlockArtifactsWorker do
+  describe '#perform' do
+    subject(:perform) { described_class.new.perform(pipeline_id) }
+
+    include_examples 'an idempotent worker' do
+      subject(:idempotent_perform) { perform_multiple(pipeline.id, exec_times: 2) }
+
+      let!(:older_pipeline) do
+        create(:ci_pipeline, :success, :with_job, locked: :artifacts_locked).tap do |pipeline|
+          create(:ci_job_artifact, job: pipeline.builds.first)
+        end
+      end
+
+      let!(:pipeline) do
+        create(:ci_pipeline, :success, :with_job, ref: older_pipeline.ref, tag: older_pipeline.tag, project: older_pipeline.project, locked: :unlocked).tap do |pipeline|
+          create(:ci_job_artifact, job: pipeline.builds.first)
+        end
+      end
+
+      it 'unlocks the artifacts from older pipelines' do
+        expect { idempotent_perform }.to change { older_pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+      end
+    end
+
+    context 'when pipeline exists' do
+      let(:pipeline) { create(:ci_pipeline, :success, :with_job) }
+      let(:pipeline_id) { pipeline.id }
+
+      context 'when pipeline has artifacts' do
+        before do
+          create(:ci_job_artifact, job: pipeline.builds.first)
+        end
+
+        it 'calls the service' do
+          service = spy(Ci::UnlockArtifactsService)
+          expect(Ci::UnlockArtifactsService).to receive(:new).and_return(service)
+
+          perform
+
+          expect(service).to have_received(:execute)
+        end
+      end
+
+      context 'when pipeline does not have artifacts' do
+        it 'does not call service' do
+          expect(Ci::UnlockArtifactsService).not_to receive(:new)
+
+          perform
+        end
+      end
+    end
+
+    context 'when pipeline does not exist' do
+      let(:pipeline_id) { non_existing_record_id }
+
+      it 'does not call service' do
+        expect(Ci::UnlockArtifactsService).not_to receive(:new)
+
+        perform
+      end
+    end
+  end
+end

spec/workers/ci/ref_delete_unlock_artifacts_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::RefDeleteUnlockArtifactsWorker do
+  describe '#perform' do
+    subject(:perform) { described_class.new.perform(project_id, user_id, ref) }
+
+    let(:ref) { 'refs/heads/master' }
+
+    let(:project) { create(:project) }
+
+    include_examples 'an idempotent worker' do
+      subject(:idempotent_perform) { perform_multiple([project_id, user_id, ref], exec_times: 2) }
+
+      let(:project_id) { project.id }
+      let(:user_id) { project.creator.id }
+
+      let(:pipeline) { create(:ci_pipeline, ref: 'master', project: project, locked: :artifacts_locked) }
+
+      it 'unlocks the artifacts from older pipelines' do
+        expect { idempotent_perform }.to change { pipeline.reload.locked }.from('artifacts_locked').to('unlocked')
+      end
+    end
+
+    context 'when project exists' do
+      let(:project_id) { project.id }
+
+      context 'when user exists' do
+        let(:user_id) { project.creator.id }
+
+        context 'when ci ref exists' do
+          before do
+            create(:ci_ref, ref_path: ref)
+          end
+
+          it 'calls the service' do
+            service = spy(Ci::UnlockArtifactsService)
+            expect(Ci::UnlockArtifactsService).to receive(:new).and_return(service)
+
+            perform
+
+            expect(service).to have_received(:execute)
+          end
+        end
+
+        context 'when ci ref does not exist' do
+          it 'does not call the service' do
+            expect(Ci::UnlockArtifactsService).not_to receive(:new)
+
+            perform
+          end
+        end
+      end
+
+      context 'when user does not exist' do
+        let(:user_id) { non_existing_record_id }
+
+        it 'does not call service' do
+          expect(Ci::UnlockArtifactsService).not_to receive(:new)
+
+          perform
+        end
+      end
+    end
+
+    context 'when project does not exist' do
+      let(:project_id) { non_existing_record_id }
+      let(:user_id) { project.creator.id }
+
+      it 'does not call service' do
+        expect(Ci::UnlockArtifactsService).not_to receive(:new)
+
+        perform
+      end
+    end
+  end
+end