Check if the latest permanent file exists before remove file. Add tests.

2 files changed, 31 insertions, 16 deletions

diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb
index d31d8b63da6..00151f4bd1c 100644
--- a/app/services/ci/create_trace_artifact_service.rb
+++ b/app/services/ci/create_trace_artifact_service.rb
@@ -6,28 +6,27 @@ module Ci
       job.trace.read do |stream|
         return unless stream.file?
 
-        temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path|
-          FileUtils.cp(stream.path, temp_path)
-          create_job_trace!(job, temp_path)
-          FileUtils.rm(stream.path)
+        temp_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |temp_path|
+          job.create_job_artifacts_trace!(
+            project: job.project,
+            file_type: :trace,
+            file: UploadedFile.new(temp_path, 'job.log', 'application/octet-stream')
+          )
         end
+
+        raise 'Trace artifact not found' unless job.job_artifacts_trace.file.exists?
+
+        FileUtils.rm(stream.path)
       end
     end
 
     private
 
-    def create_job_trace!(job, path)
-      job.create_job_artifacts_trace!(
-        project: job.project,
-        file_type: :trace,
-        file: UploadedFile.new(path, 'job.log', 'application/octet-stream')
-      )
-    end
-
-    def temp_file!(temp_dir)
+    def temp_file!(src_file, temp_dir)
       FileUtils.mkdir_p(temp_dir)
-      temp_file = Tempfile.new('legacy-trace-tmp-', temp_dir)
+      temp_file = Tempfile.new('trace-tmp-', temp_dir)
       temp_file&.close
+      FileUtils.cp(src_file, temp_file.path)
       yield(temp_file.path)
     ensure
       temp_file&.close
diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb
index 0628e6d112e..f928deb8632 100644
--- a/spec/services/ci/create_trace_artifact_service_spec.rb
+++ b/spec/services/ci/create_trace_artifact_service_spec.rb
@@ -8,6 +8,9 @@ describe Ci::CreateTraceArtifactService do
       context 'when the job has a trace file' do
         let!(:job) { create(:ci_build, :trace_live) }
         let!(:legacy_path) { job.trace.read { |stream| return stream.path } }
+        let!(:legacy_checksum) { Digest::SHA256.file(legacy_path).hexdigest }
+        let(:new_path) { job.job_artifacts_trace.file.path }
+        let(:new_checksum) { Digest::SHA256.file(new_path).hexdigest }
 
         it { expect(File.exists?(legacy_path)).to be_truthy }
 
@@ -15,8 +18,9 @@ describe Ci::CreateTraceArtifactService do
           expect { subject }.to change { Ci::JobArtifact.count }.by(1)
 
           expect(File.exists?(legacy_path)).to be_falsy
-          expect(File.exists?(job.job_artifacts_trace.file.path)).to be_truthy
-          expect(job.job_artifacts_trace.exists?).to be_truthy
+          expect(File.exists?(new_path)).to be_truthy
+          expect(new_checksum).to eq(legacy_checksum)
+          expect(job.job_artifacts_trace.file.exists?).to be_truthy
           expect(job.job_artifacts_trace.file.filename).to eq('job.log')
         end
 
@@ -37,6 +41,18 @@ describe Ci::CreateTraceArtifactService do
             expect(job.job_artifacts_trace).to be_nil
           end
         end
+
+        context 'when migrated trace artifact file is not found' do
+          before do
+            allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?) { false }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error('Trace artifact not found')
+
+            expect(File.exists?(legacy_path)).to be_truthy
+          end
+        end
       end
 
       context 'when the job does not have a trace file' do