Merge branch 'lock-trace-writes' into 'master'

Lock writes to trace stream

Closes #51502

See merge request gitlab-org/gitlab-ce!23332

5 files changed, 50 insertions, 18 deletions

diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb
index 4f49958dd33..38626f728d7 100644
--- a/spec/lib/gitlab/ci/trace/stream_spec.rb
+++ b/spec/lib/gitlab/ci/trace/stream_spec.rb
@@ -257,7 +257,8 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
         let!(:last_result) { stream.html_with_state }
 
         before do
-          stream.append("5678", 4)
+          data_stream.seek(4, IO::SEEK_SET)
+          data_stream.write("5678")
           stream.seek(0)
         end
 
@@ -271,25 +272,29 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
     end
 
     context 'when stream is StringIO' do
+      let(:data_stream) do
+        StringIO.new("1234")
+      end
+
       let(:stream) do
-        described_class.new do
-          StringIO.new("1234")
-        end
+        described_class.new { data_stream }
       end
 
       it_behaves_like 'html_with_states'
     end
 
     context 'when stream is ChunkedIO' do
-      let(:stream) do
-        described_class.new do
-          Gitlab::Ci::Trace::ChunkedIO.new(build).tap do |chunked_io|
-            chunked_io.write("1234")
-            chunked_io.seek(0, IO::SEEK_SET)
-          end
+      let(:data_stream) do
+        Gitlab::Ci::Trace::ChunkedIO.new(build).tap do |chunked_io|
+          chunked_io.write("1234")
+          chunked_io.seek(0, IO::SEEK_SET)
         end
       end
 
+      let(:stream) do
+        described_class.new { data_stream }
+      end
+
       it_behaves_like 'html_with_states'
     end
   end
diff --git a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb
index 2e3656b52fb..5107e1efbbd 100644
--- a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb
+++ b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb
@@ -11,6 +11,14 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do
 
     let(:options) { {} }
 
+    context 'when unique key is not set' do
+      let(:unique_key) { }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error ArgumentError
+      end
+    end
+
     context 'when the lease is not obtained yet' do
       before do
         stub_exclusive_lease(unique_key, 'uuid')
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 909703a8d47..b36087b86a7 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -830,6 +830,18 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           expect(job.trace.raw).to eq 'BUILD TRACE UPDATED'
           expect(job.job_artifacts_trace.open.read).to eq 'BUILD TRACE UPDATED'
         end
+
+        context 'when concurrent update of trace is happening' do
+          before do
+            job.trace.write('wb') do
+              update_job(state: 'success', trace: 'BUILD TRACE UPDATED')
+            end
+          end
+
+          it 'returns that operation conflicts' do
+            expect(response.status).to eq(409)
+          end
+        end
       end
 
       context 'when no trace is given' do
@@ -1022,6 +1034,18 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           end
         end
 
+        context 'when concurrent update of trace is happening' do
+          before do
+            job.trace.write('wb') do
+              patch_the_trace
+            end
+          end
+
+          it 'returns that operation conflicts' do
+            expect(response.status).to eq(409)
+          end
+        end
+
         context 'when the job is canceled' do
           before do
             job.cancel
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index 94e82b8ce90..377bd82b67e 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/spec/support/shared_examples/ci_trace_shared_examples.rb
@@ -272,16 +272,11 @@ shared_examples_for 'common trace features' do
           include ExclusiveLeaseHelpers
 
           before do
-            stub_exclusive_lease_taken("trace:archive:#{trace.job.id}", timeout: 1.hour)
+            stub_exclusive_lease_taken("trace:write:lock:#{trace.job.id}", timeout: 1.minute)
           end
 
           it 'blocks concurrent archiving' do
-            expect(Rails.logger).to receive(:error).with('Cannot obtain an exclusive lease. There must be another instance already in execution.')
-
-            subject
-
-            build.reload
-            expect(build.job_artifacts_trace).to be_nil
+            expect { subject }.to raise_error(::Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
           end
         end
       end
diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb
index 557934346c9..e09b8e5b964 100644
--- a/spec/workers/stuck_ci_jobs_worker_spec.rb
+++ b/spec/workers/stuck_ci_jobs_worker_spec.rb
@@ -5,7 +5,7 @@ describe StuckCiJobsWorker do
 
   let!(:runner) { create :ci_runner }
   let!(:job) { create :ci_build, runner: runner }
-  let(:trace_lease_key) { "trace:archive:#{job.id}" }
+  let(:trace_lease_key) { "trace:write:lock:#{job.id}" }
   let(:trace_lease_uuid) { SecureRandom.uuid }
   let(:worker_lease_key) { StuckCiJobsWorker::EXCLUSIVE_LEASE_KEY }
   let(:worker_lease_uuid) { SecureRandom.uuid }