Fix spec. Revert #truncate in stream (But still prevent redandant calls)

app/models/ci/build_trace_chunk.rb

@@ -24,7 +24,7 @@ module Ci
     ##
     # Data is memoized for optimizing #size and #end_offset
     def data
-      @data ||= get_data
+      @data ||= get_data.to_s
     end
 
     def truncate(offset = 0)
@@ -32,11 +32,10 @@ module Ci
     end
 
     def append(new_data, offset)
-      current_data = self.data.to_s
+      raise ArgumentError, 'Offset is out of range' if offset > data.bytesize || offset < 0
-      raise ArgumentError, 'Offset is out of bound' if offset > current_data.bytesize || offset < 0
+      raise ArgumentError, 'Chunk size overflow' if CHUNK_SIZE < (offset + new_data.bytesize)
-      raise ArgumentError, 'Outside of chunk size' if CHUNK_SIZE < offset + new_data.bytesize
 
-      set_data(current_data.byteslice(0, offset) + new_data)
+      set_data(data.byteslice(0, offset) + new_data)
     end
 
     def size

lib/gitlab/ci/trace/stream.rb

@@ -52,6 +52,7 @@ module Gitlab
 
           stream.seek(0, IO::SEEK_SET)
           stream.write(data)
+          stream.truncate(data.bytesize)
           stream.flush()
         end
 

spec/factories/ci/build_trace_chunks.rb

@@ -1,6 +1,6 @@
 FactoryBot.define do
   factory :ci_build_trace_chunk, class: Ci::BuildTraceChunk do
-    job factory: :ci_build
+    build factory: :ci_build
     chunk_index 0
     data_store :redis
   end

spec/models/ci/build_trace_chunk_spec.rb

@@ -46,7 +46,7 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
   end
 
   describe '#set_data' do
-    subject { build_trace_chunk.set_data(value) }
+    subject { build_trace_chunk.send(:set_data, value) }
 
     let(:value) { 'Sample data' }
 
@@ -131,13 +131,17 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       context 'when offset is negative' do
         let(:offset) { -1 }
 
-        it { expect { subject }.to raise_error('Offset is out of bound') }
+        it { expect { subject }.to raise_error('Offset is out of range') }
       end
 
       context 'when offset is bigger than data size' do
         let(:offset) { data.bytesize + 1 }
 
-        it { expect { subject }.to raise_error('Offset is out of bound') }
+        it do
+          expect_any_instance_of(described_class).not_to receive(:append) { }
+
+          subject
+        end
       end
 
       context 'when offset is 10' do
@@ -182,19 +186,19 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       context 'when offset is negative' do
         let(:offset) { -1 }
 
-        it { expect { subject }.to raise_error('Offset is out of bound') }
+        it { expect { subject }.to raise_error('Offset is out of range') }
       end
 
       context 'when offset is bigger than data size' do
         let(:offset) { data.bytesize + 1 }
 
-        it { expect { subject }.to raise_error('Offset is out of bound') }
+        it { expect { subject }.to raise_error('Offset is out of range') }
       end
 
       context 'when offset is bigger than data size' do
         let(:new_data) { 'a' * (described_class::CHUNK_SIZE + 1) }
 
-        it { expect { subject }.to raise_error('Outside of chunk size') }
+        it { expect { subject }.to raise_error('Chunk size overflow') }
       end
 
       context 'when offset is EOF' do
@@ -320,7 +324,7 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
   describe 'ExclusiveLock' do
     before do
       allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
-      stub_const('Ci::BuildTraceChunk::LOCK_RETRY', 1)
+      stub_const('Ci::BuildTraceChunk::WRITE_LOCK_RETRY', 1)
     end
 
     it 'raise an error' do
@@ -333,30 +337,31 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
 
     before do
       pipeline = create(:ci_pipeline, project: project)
-      create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
+      @build_ids = []
-      create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
+      @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id
-      create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
+      @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id
+      @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id
     end
 
     shared_examples_for 'deletes all build_trace_chunk and data in redis' do
       it do
-        project.builds.each do |build|
+        @build_ids.each do |build_id|
           Gitlab::Redis::SharedState.with do |redis|
-            redis.scan_each(match: "gitlab:ci:trace:#{build.id}:chunks:?") do |key|
+            redis.scan_each(match: "gitlab:ci:trace:#{build_id}:chunks:?") do |key|
               expect(redis.exists(key)).to be_truthy
             end
           end
         end
 
-        expect(described_class.count).not_to eq(0)
+        expect(described_class.count).to eq(3)
 
         subject
 
         expect(described_class.count).to eq(0)
 
-        project.builds.each do |build|
+        @build_ids.each do |build_id|
           Gitlab::Redis::SharedState.with do |redis|
-            redis.scan_each(match: "gitlab:ci:trace:#{build.id}:chunks:?") do |key|
+            redis.scan_each(match: "gitlab:ci:trace:#{build_id}:chunks:?") do |key|
               expect(redis.exists(key)).to be_falsey
             end
           end
@@ -364,14 +369,6 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       end
     end
 
-    context 'when build_trace_chunk is destroyed' do
-      let(:subject) do
-        project.builds.each { |build| build.chunks.destroy_all }
-      end
-
-      it_behaves_like 'deletes all build_trace_chunk and data in redis'
-    end
-
     context 'when build is destroyed' do
       let(:subject) do
         project.builds.destroy_all

spec/services/ci/retry_build_service_spec.rb

@@ -30,7 +30,7 @@ describe Ci::RetryBuildService do
        runner_id tag_taggings taggings tags trigger_request_id
        user_id auto_canceled_by_id retried failure_reason
        artifacts_file_store artifacts_metadata_store
-       metadata chunks].freeze
+       metadata trace_chunks].freeze
 
   shared_examples 'build duplication' do
     let(:another_pipeline) { create(:ci_empty_pipeline, project: project) }