Avoid destroy_all. Hook ActiveRecord.delete_all and flush redis

6 files changed, 86 insertions, 26 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index b471fb80536..a7d2424508e 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -25,7 +25,7 @@ module Ci
     has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
     has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
 
-    has_many :chunks, class_name: 'Ci::JobTraceChunk', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :chunks, class_name: 'Ci::JobTraceChunk', foreign_key: :job_id
 
     has_one :metadata, class_name: 'Ci::BuildMetadata'
     delegate :timeout, to: :metadata, prefix: true, allow_nil: true
@@ -91,6 +91,7 @@ module Ci
     before_save :update_artifacts_size, if: :artifacts_file_changed?
     before_save :ensure_token
     before_destroy { unscoped_project }
+    before_destroy :flush_redis_chunks
 
     before_create :ensure_metadata
     after_create unless: :importing? do |build|
@@ -677,5 +678,9 @@ module Ci
         update_project_statistics
       end
     end
+
+    def flush_redis_chunks
+      chunks.redis.map(&:redis_delete_data)
+    end
   end
 end
diff --git a/app/models/ci/job_trace_chunk.rb b/app/models/ci/job_trace_chunk.rb
index 383c6596a51..a5f1734ddfd 100644
--- a/app/models/ci/job_trace_chunk.rb
+++ b/app/models/ci/job_trace_chunk.rb
@@ -4,8 +4,6 @@ module Ci
 
     belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
 
-    after_destroy :redis_delete_data, if: :redis?
-
     default_value_for :data_store, :redis
 
     WriteError = Class.new(StandardError)
@@ -21,6 +19,28 @@ module Ci
       db: 2
     }
 
+    # Hook delete_all method to flush redis before the record has been destoryed
+    def self.delete_all(*args)
+      if self.delete_all_truncate_condition(args.first) ||
+         self.delete_all_delete_condition(args.first)
+        Ci::JobTraceChunk.where(args.first).redis.map(&:redis_delete_data)
+      end
+
+      super
+    end
+
+    def destory
+      raise 'You are going to be punished by yorik'
+    end
+
+    def self.delete_all_truncate_condition(query)
+      query.is_a?(String) && /^job_id = \d+ AND chunk_index > \d+$/ =~ query
+    end
+
+    def self.delete_all_delete_condition(query)
+      query.is_a?(Hash) && query.keys.count == 1 && query.keys.first == :job_id
+    end
+
     def data
       if redis?
         redis_data
@@ -44,8 +64,9 @@ module Ci
         end
 
         save! if changed?
-        schedule_to_db if fullfilled?
       end
+
+      schedule_to_db if fullfilled?
     end
 
     def truncate(offset = 0)
@@ -86,6 +107,12 @@ module Ci
       end
     end
 
+    def redis_delete_data
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.del(redis_data_key)
+      end
+    end
+
     private
 
     def schedule_to_db
@@ -110,12 +137,6 @@ module Ci
       end
     end
 
-    def redis_delete_data
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.del(redis_data_key)
-      end
-    end
-
     def redis_data_key
       "gitlab:ci:trace:#{job_id}:chunks:#{chunk_index}:data"
     end
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index 6554c924e5c..abf9212d901 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -100,7 +100,7 @@ module Gitlab
           FileUtils.rm(trace_path, force: true)
         end
 
-        job.chunks.destroy_all
+        ::Ci::JobTraceChunk.delete_all(job_id: job.id)
         job.erase_old_trace!
       end
 
@@ -111,7 +111,7 @@ module Gitlab
         if job.chunks.any?
           Gitlab::Ci::Trace::ChunkedIO.new(job) do |stream|
             archive_stream!(stream)
-            stream.destroy!
+            stream.delete!
           end
         elsif current_path
           File.open(current_path) do |stream|
diff --git a/lib/gitlab/ci/trace/chunked_io.rb b/lib/gitlab/ci/trace/chunked_io.rb
index 2caebe3c95e..56d1f0c2fa4 100644
--- a/lib/gitlab/ci/trace/chunked_io.rb
+++ b/lib/gitlab/ci/trace/chunked_io.rb
@@ -141,7 +141,7 @@ module Gitlab
           @size = offset
 
           # remove all next chunks
-          job_chunks.where('chunk_index > ?', chunk_index).destroy_all
+          ::Ci::JobTraceChunk.delete_all("job_id = #{job.id} AND chunk_index > #{chunk_index}")
 
           # truncate current chunk
           current_chunk.truncate(chunk_offset) if chunk_offset != 0
@@ -157,8 +157,8 @@ module Gitlab
           true
         end
 
-        def destroy!
-          job_chunks.destroy_all
+        def delete!
+          ::Ci::JobTraceChunk.delete_all(job_id: job.id)
           @tell = @size = 0
         ensure
           invalidate_chunk_cache
diff --git a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
index bcef21d5f71..2ef72ee4d6f 100644
--- a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
+++ b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
@@ -366,8 +366,8 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
     end
   end
 
-  context "#destroy!" do
-    subject { chunked_io.destroy! }
+  context "#delete!" do
+    subject { chunked_io.delete! }
 
     before do
       job.trace.set(sample_trace_raw)
diff --git a/spec/models/ci/job_trace_chunk_spec.rb b/spec/models/ci/job_trace_chunk_spec.rb
index eb240de188f..d9d30a07f03 100644
--- a/spec/models/ci/job_trace_chunk_spec.rb
+++ b/spec/models/ci/job_trace_chunk_spec.rb
@@ -336,37 +336,71 @@ describe Ci::JobTraceChunk, :clean_gitlab_redis_shared_state do
       create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
       create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
       create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project)
+      @build_ids = project.builds.map(&:id)
     end
 
-    shared_examples_for 'deletes all job_trace_chunk and data in redis' do
+    shared_examples_for 'deletes all job_trace_chunk and data in redis' do |expected_count = 0|
       it do
-        project.builds.each do |build|
+        count = 0
+
+        @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:?:data") do |key|
               expect(redis.exists(key)).to be_truthy
+              count += 1
             end
           end
         end
 
+        raise 'Sample is not processed correctly' unless count == 3
+
         expect(described_class.count).not_to eq(0)
 
         subject
 
-        expect(described_class.count).to eq(0)
+        expect(described_class.count).to eq(expected_count)
 
-        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|
-              expect(redis.exists(key)).to be_falsey
+            redis.scan_each(match: "gitlab:ci:trace:#{build_id}:chunks:?:data") do |key|
+              raise 'Redis key still exists'
             end
           end
         end
       end
     end
 
-    context 'when job_trace_chunk is destroyed' do
+    context 'when trace is truncated' do
+      include ChunkedIOHelpers
+
+      let(:subject) do
+        project.builds.each { |build| build.trace.set(sample_trace_raw) }
+        project.builds.each { |build| build.trace.write { |stream| stream.truncate(0) } }
+      end
+
+      it_behaves_like 'deletes all job_trace_chunk and data in redis', 3
+    end
+
+    context 'when trace is erased' do
+      let(:subject) do
+        project.builds.each { |build| build.trace.erase! }
+      end
+
+      it_behaves_like 'deletes all job_trace_chunk and data in redis'
+    end
+
+    context 'when trace is archived' do
+      let(:subject) do
+        allow_any_instance_of(Ci::Build).to receive(:complete?) { true }
+        project.builds.each { |build| build.trace.archive! }
+      end
+
+      it_behaves_like 'deletes all job_trace_chunk and data in redis'
+    end
+
+    context 'when job_trace_chunk are deleted' do
       let(:subject) do
-        project.builds.each { |build| build.chunks.destroy_all }
+        project.builds.each { |build| ::Ci::JobTraceChunk.delete_all(job_id: build.id) }
       end
 
       it_behaves_like 'deletes all job_trace_chunk and data in redis'