Merge branch 'remove-trace-chunks-when-archive-exists' into 'master'

Remove left over already archived live traces from build [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56353

app/models/ci/job_artifact.rb

@@ -290,8 +290,12 @@ module Ci
         end
     end
 
+    def archived_trace_exists?
+      file&.file&.exists?
+    end
+
     def self.archived_trace_exists_for?(job_id)
-      where(job_id: job_id).trace.take&.file&.file&.exists?
+      where(job_id: job_id).trace.take&.archived_trace_exists?
     end
 
     def self.max_artifact_size(type:, project:)

config/feature_flags/development/erase_traces_from_already_archived_jobs_when_archiving_again.yml

+---
+name: erase_traces_from_already_archived_jobs_when_archiving_again
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56353
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326679
+milestone: "13.11"
+type: development
+group: group::continuous integration
+default_enabled: true

lib/gitlab/ci/trace.rb

@@ -93,6 +93,10 @@ module Gitlab
         end
       end
 
+      def erase_trace_chunks!
+        job.trace_chunks.fast_destroy_all # Destroy chunks of a live trace
+      end
+
       def erase!
         ##
         # Erase the archived trace
@@ -100,7 +104,7 @@ module Gitlab
 
         ##
         # Erase the live trace
-        job.trace_chunks.fast_destroy_all # Destroy chunks of a live trace
+        erase_trace_chunks!
         FileUtils.rm_f(current_path) if current_path # Remove a trace file of a live trace
         job.erase_old_trace! if job.has_old_trace? # Remove a trace in database of a live trace
       ensure
@@ -180,9 +184,14 @@ module Gitlab
       end
 
       def unsafe_archive!
-        raise AlreadyArchivedError, 'Could not archive again' if trace_artifact
         raise ArchiveError, 'Job is not finished yet' unless job.complete?
 
+        if trace_artifact
+          unsafe_trace_cleanup! if Feature.enabled?(:erase_traces_from_already_archived_jobs_when_archiving_again, job.project, default_enabled: :yaml)
+
+          raise AlreadyArchivedError, 'Could not archive again'
+        end
+
         if job.trace_chunks.any?
           Gitlab::Ci::Trace::ChunkedIO.new(job) do |stream|
             archive_stream!(stream)
@@ -201,6 +210,18 @@ module Gitlab
         end
       end
 
+      def unsafe_trace_cleanup!
+        return unless trace_artifact
+
+        if trace_artifact.archived_trace_exists?
+          # An archive already exists, so make sure to remove the trace chunks
+          erase_trace_chunks!
+        else
+          # An archive already exists, but its associated file does not, so remove it
+          trace_artifact.destroy!
+        end
+      end
+
       def in_write_lock(&blk)
         lock_key = "trace:write:lock:#{job.id}"
         in_lock(lock_key, ttl: LOCK_TTL, retries: LOCK_RETRIES, sleep_sec: LOCK_SLEEP, &blk)

spec/lib/gitlab/ci/trace_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Trace, :clean_gitlab_redis_shared_state, factory_default: :keep do
   let_it_be(:project) { create_default(:project).freeze }
-  let_it_be_with_reload(:build) { create(:ci_build) }
+  let_it_be_with_reload(:build) { create(:ci_build, :success) }
   let(:trace) { described_class.new(build) }
 
   describe "associations" do

spec/models/ci/job_artifact_spec.rb

@@ -195,6 +195,22 @@ RSpec.describe Ci::JobArtifact do
     end
   end
 
+  describe '#archived_trace_exists?' do
+    subject { artifact.archived_trace_exists? }
+
+    context 'when the file exists' do
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when the file does not exist' do
+      before do
+        artifact.file.remove!
+      end
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '.for_sha' do
     let(:first_pipeline) { create(:ci_pipeline) }
     let(:second_pipeline) { create(:ci_pipeline, project: first_pipeline.project, sha: Digest::SHA1.hexdigest(SecureRandom.hex)) }

spec/services/ci/archive_trace_service_spec.rb

@@ -24,6 +24,52 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do
       it 'does not create an archived trace' do
         expect { subject }.not_to change { Ci::JobArtifact.trace.count }
       end
+
+      context 'when live trace chunks still exist' do
+        before do
+          create(:ci_build_trace_chunk, build: job)
+        end
+
+        context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is enabled' do
+          before do
+            stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: true)
+          end
+
+          it 'removes the trace chunks' do
+            expect { subject }.to change { job.trace_chunks.count }.to(0)
+          end
+
+          context 'when associated data does not exist' do
+            before do
+              job.job_artifacts_trace.file.remove!
+            end
+
+            it 'removes the trace artifact' do
+              expect { subject }.to change { job.reload.job_artifacts_trace }.to(nil)
+            end
+          end
+        end
+
+        context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is disabled' do
+          before do
+            stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: false)
+          end
+
+          it 'does not remove the trace chunks' do
+            expect { subject }.not_to change { job.trace_chunks.count }
+          end
+
+          context 'when associated data does not exist' do
+            before do
+              job.job_artifacts_trace.file.remove!
+            end
+
+            it 'does not remove the trace artifact' do
+              expect { subject }.not_to change { job.reload.job_artifacts_trace }
+            end
+          end
+        end
+      end
     end
 
     context 'when job does not have trace' do

spec/support/shared_examples/lib/gitlab/ci/ci_trace_shared_examples.rb

@@ -843,6 +843,17 @@ RSpec.shared_examples 'trace with enabled live trace feature' do
         expect { subject }.to raise_error(Gitlab::Ci::Trace::AlreadyArchivedError)
         expect(build.job_artifacts_trace.file.exists?).to be_truthy
       end
+
+      context 'when live trace chunks still exist' do
+        before do
+          create(:ci_build_trace_chunk, build: build)
+        end
+
+        it 'removes the traces' do
+          expect { subject }.to raise_error(Gitlab::Ci::Trace::AlreadyArchivedError)
+          expect(build.trace_chunks).to be_empty
+        end
+      end
     end
 
     context 'when job is not finished yet' do