Optimise trace handling code to use streaming instead of full read

# Conflicts:
#	app/assets/javascripts/build.js

9 files changed, 230 insertions, 228 deletions

diff --git a/app/assets/javascripts/build.js b/app/assets/javascripts/build.js
index 6e6e9b18686..ece9af5c455 100644
--- a/app/assets/javascripts/build.js
+++ b/app/assets/javascripts/build.js
@@ -85,10 +85,10 @@
       var removeRefreshStatuses = ['success', 'failed', 'canceled', 'skipped'];
 
       return $.ajax({
-        url: this.buildUrl,
+        url: this.pageUrl + "/trace.json",
         dataType: 'json',
         success: function(buildData) {
-          $('.js-build-output').html(buildData.trace_html);
+          $('.js-build-output').html(buildData.html);
           if (window.location.hash === DOWN_BUILD_TRACE) {
             $("html,body").scrollTop(this.$buildTrace.height());
           }
diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index f1e4246e7fb..c74e1afa567 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -31,25 +31,20 @@ class Projects::BuildsController < Projects::ApplicationController
     @builds = @project.pipelines.find_by_sha(@build.sha).builds.order('id DESC')
     @builds = @builds.where("id not in (?)", @build.id)
     @pipeline = @build.pipeline
-
-    respond_to do |format|
-      format.html
-      format.json do
-        render json: {
-          id: @build.id,
-          status: @build.status,
-          trace_html: @build.trace_html
-        }
-      end
-    end
   end
 
   def trace
-    respond_to do |format|
-      format.json do
-        state = params[:state].presence
-        render json: @build.trace_with_state(state: state).
-          merge!(id: @build.id, status: @build.status)
+    build.trace.use do |trace|
+      return render_404 unless trace.valid?
+
+      trace.limit
+
+      respond_to do |format|
+        format.json do
+          state = params[:state].presence
+          render json: trace.html_with_state(state).to_h.
+            merge!(id: @build.id, status: @build.status, complete: @build.complete?)
+        end
       end
     end
   end
@@ -84,10 +79,12 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def raw
-    if @build.has_trace_file?
-      send_file @build.trace_file_path, type: 'text/plain; charset=utf-8', disposition: 'inline'
-    else
-      render_404
+    build.trace.use do |trace|
+      if trace.file?
+        send_file trace.path, type: 'text/plain; charset=utf-8', disposition: 'inline'
+      else
+        render_404
+      end
     end
   end
 
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index ad0be70c32a..410f3bf5257 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -171,19 +171,6 @@ module Ci
       latest_builds.where('stage_idx < ?', stage_idx)
     end
 
-    def trace_html(**args)
-      trace_with_state(**args)[:html] || ''
-    end
-
-    def trace_with_state(state: nil, last_lines: nil)
-      trace_ansi = trace(last_lines: last_lines)
-      if trace_ansi.present?
-        Ci::Ansi2html.convert(trace_ansi, state)
-      else
-        {}
-      end
-    end
-
     def timeout
       project.build_timeout
     end
@@ -244,85 +231,66 @@ module Ci
     end
 
     def update_coverage
-      coverage = extract_coverage(trace, coverage_regex)
+      coverage = trace_data.transaction do
+        coverage(coverage_regex)
+      end
       update_attributes(coverage: coverage) if coverage.present?
     end
 
-    def extract_coverage(text, regex)
-      return unless regex
-
-      matches = text.scan(Regexp.new(regex)).last
-      matches = matches.last if matches.is_a?(Array)
-      coverage = matches.gsub(/\d+(\.\d+)?/).first
-
-      if coverage.present?
-        coverage.to_f
+    def trace_paths
+      [
+        File.join(
+          Settings.gitlab_ci.builds_path,
+          created_at.utc.strftime("%Y_%m"),
+          project_id.to_s,
+          "#{id}.log"
+        ),
+        project&.ci_id && File.join(
+          Settings.gitlab_ci.builds_path,
+          created_at.utc.strftime("%Y_%m"),
+          project.ci_id.to_s,
+          "#{id}.log"
+        )
+      ].compact
+    end
+
+    def current_trace_path
+      @current_trace_path ||= trace_paths.find do |trace_path|
+        File.exist?(trace_path)
       end
-    rescue
-      # if bad regex or something goes wrong we dont want to interrupt transition
-      # so we just silentrly ignore error for now
-    end
-
-    def has_trace_file?
-      File.exist?(path_to_trace) || has_old_trace_file?
     end
 
     def has_trace?
-      raw_trace.present?
+      current_trace_path.present? || read_attribute(:trace).present?
     end
 
-    def raw_trace(last_lines: nil)
-      if File.exist?(trace_file_path)
-        Gitlab::Ci::TraceReader.new(trace_file_path).
-          read(last_lines: last_lines)
-      else
-        # backward compatibility
-        read_attribute :trace
-      end
-    end
-
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    def has_old_trace_file?
-      project.ci_id && File.exist?(old_path_to_trace)
+    def db_trace_stream
+      trace = read_attribute(:trace)
+      StringIO.new(trace) if trace
     end
 
-    def trace(last_lines: nil)
-      hide_secrets(raw_trace(last_lines: last_lines))
-    end
-
-    def trace_length
-      if raw_trace
-        raw_trace.bytesize
-      else
-        0
+    def trace
+      Gitlab::Ci::Trace.new do
+        if current_trace_path
+          File.open(current_trace_path, "rb")
+        else
+          db_trace_stream
+        end
       end
     end
 
-    def trace=(trace)
-      recreate_trace_dir
-      trace = hide_secrets(trace)
-      File.write(path_to_trace, trace)
-    end
-
-    def recreate_trace_dir
-      unless Dir.exist?(dir_to_trace)
-        FileUtils.mkdir_p(dir_to_trace)
+    def trace=(new_trace)
+      writeable_trace.use do |trace|
+        trace.replace(new_trace)
       end
     end
-    private :recreate_trace_dir
 
-    def append_trace(trace_part, offset)
-      recreate_trace_dir
-      touch if needs_touch?
+    def writeable_trace
+      Gitlab::Ci::Trace.new do
+        trace_path = current_trace_path || trace_paths.first
+        FileUtils.mkdir_p(trace_dir) unless Dir.exist?(trace_dir)
 
-      trace_part = hide_secrets(trace_part)
-
-      File.truncate(path_to_trace, offset) if File.exist?(path_to_trace)
-      File.open(path_to_trace, 'ab') do |f|
-        f.write(trace_part)
+        File.open(trace_path, "wb")
       end
     end
 
@@ -330,50 +298,6 @@ module Ci
       Time.now - updated_at > 15.minutes.to_i
     end
 
-    def trace_file_path
-      if has_old_trace_file?
-        old_path_to_trace
-      else
-        path_to_trace
-      end
-    end
-
-    def dir_to_trace
-      File.join(
-        Settings.gitlab_ci.builds_path,
-        created_at.utc.strftime("%Y_%m"),
-        project.id.to_s
-      )
-    end
-
-    def path_to_trace
-      "#{dir_to_trace}/#{id}.log"
-    end
-
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    # Should be removed in 8.4, after CI files migration has been done.
-    #
-    def old_dir_to_trace
-      File.join(
-        Settings.gitlab_ci.builds_path,
-        created_at.utc.strftime("%Y_%m"),
-        project.ci_id.to_s
-      )
-    end
-
-    ##
-    # Deprecated
-    #
-    # This is a hotfix for CI build data integrity, see #4246
-    # Should be removed in 8.4, after CI files migration has been done.
-    #
-    def old_path_to_trace
-      "#{old_dir_to_trace}/#{id}.log"
-    end
-
     ##
     # Deprecated
     #
@@ -560,7 +484,8 @@ module Ci
     end
 
     def erase_trace!
-      self.trace = nil
+      self.write_attribute(trace: nil)
+      File.rm(current_trace_path) if current_trace_path
     end
 
     def update_erased!(user = nil)
diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml
index 4192013eab5..b0c548d7794 100644
--- a/app/views/projects/builds/_sidebar.html.haml
+++ b/app/views/projects/builds/_sidebar.html.haml
@@ -68,7 +68,7 @@
         - elsif @build.runner
           \##{@build.runner.id}
       .btn-group.btn-group-justified{ role: :group }
-        - if @build.has_trace_file?
+        - if @build.has_trace?
           = link_to 'Raw', raw_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default'
         - if @build.active?
           = link_to "Cancel", cancel_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default', method: :post
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index d288369e362..2d3ae10de5c 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -145,16 +145,18 @@ module API
         content_range = request.headers['Content-Range']
         content_range = content_range.split('-')
 
-        current_length = job.trace_length
-        unless current_length == content_range[0].to_i
-          return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{current_length}" })
-        end
+        job.writeable_trace.use do |trace|
+          current_length = trace.size
+          unless current_length == content_range[0].to_i
+            return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{current_length}" })
+          end
 
-        job.append_trace(request.body.read, content_range[0].to_i)
+          trace.append(request.body.read, content_range[0].to_i)
 
-        status 202
-        header 'Job-Status', job.status
-        header 'Range', "0-#{job.trace_length}"
+          status 202
+          header 'Job-Status', job.status
+          header 'Range', "0-#{trace.size}"
+        end
       end
 
       desc 'Authorize artifacts uploading for job' do
diff --git a/lib/ci/ansi2html.rb b/lib/ci/ansi2html.rb
index b3ccad7b28d..8ebc05601dc 100644
--- a/lib/ci/ansi2html.rb
+++ b/lib/ci/ansi2html.rb
@@ -23,6 +23,8 @@ module Ci
       cross:      0x10,
     }.freeze
 
+    Result = Struct.new(:html, :state, :append, :truncated, :offset, :size, :total_size)
+
     def self.convert(ansi, state = nil)
       Converter.new.convert(ansi, state)
     end
@@ -132,34 +134,47 @@ module Ci
 
       STATE_PARAMS = [:offset, :n_open_tags, :fg_color, :bg_color, :style_mask].freeze
 
-      def convert(raw, new_state)
+      def convert(stream, new_state)
         reset_state
-        restore_state(raw, new_state) if new_state.present?
-
-        start = @offset
-        ansi = raw[@offset..-1]
+        restore_state(new_state) if new_state.present?
+
+        append = false
+        truncated = false
+
+        cur_offset = stream.tell
+        if cur_offset > @offset
+          @offset = cur_offset
+          truncated = true
+        else
+          stream.seek(@offset)
+          append = true
+        end
+        start_offset = @offset
 
         open_new_tag
 
-        s = StringScanner.new(ansi)
-        until s.eos?
-          if s.scan(/\e([@-_])(.*?)([@-~])/)
-            handle_sequence(s)
-          elsif s.scan(/\e(([@-_])(.*?)?)?$/)
-            break
-          elsif s.scan(/</)
-            @out << '&lt;'
-          elsif s.scan(/\r?\n/)
-            @out << '<br>'
-          else
-            @out << s.scan(/./m)
+        stream.each_line do |line|
+          s = StringScanner.new(line)
+          until s.eos?
+            if s.scan(/\e([@-_])(.*?)([@-~])/)
+              handle_sequence(s)
+            elsif s.scan(/\e(([@-_])(.*?)?)?$/)
+              break
+            elsif s.scan(/</)
+              @out << '&lt;'
+            elsif s.scan(/\r?\n/)
+              @out << '<br>'
+            else
+              @out << s.scan(/./m)
+            end
+            @offset += s.matched_size
           end
-          @offset += s.matched_size
         end
 
         close_open_tags()
 
-        { state: state, html: @out, text: ansi[0, @offset - start], append: start > 0 }
+        Result.new(@out, state, append, truncated,
+          start_offset, stream.tell - start_offset, stream.size)
       end
 
       def handle_sequence(s)
@@ -240,7 +255,7 @@ module Ci
         Base64.urlsafe_encode64(state.to_json)
       end
 
-      def restore_state(raw, new_state)
+      def restore_state(new_state)
         state = Base64.urlsafe_decode64(new_state)
         state = JSON.parse(state, symbolize_names: true)
         return if state[:offset].to_i > raw.length
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index 95cc6308c3b..c7bdf67534e 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -92,16 +92,18 @@ module Ci
           content_range = request.headers['Content-Range']
           content_range = content_range.split('-')
 
-          current_length = build.trace_length
-          unless current_length == content_range[0].to_i
-            return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{current_length}" })
-          end
+          build.writeable_trace.use do |trace|
+            current_length = trace.size
+            unless current_length == content_range[0].to_i
+              return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{current_length}" })
+            end
 
-          build.append_trace(request.body.read, content_range[0].to_i)
+            trace.append(request.body.read, content_range[0].to_i)
 
-          status 202
-          header 'Build-Status', build.status
-          header 'Range', "0-#{build.trace_length}"
+            status 202
+            header 'Build-Status', build.status
+            header 'Range', "0-#{trace.size}"
+          end
         end
 
         # Authorize artifacts uploading for build - Runners only
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
new file mode 100644
index 00000000000..85a21566635
--- /dev/null
+++ b/lib/gitlab/ci/trace.rb
@@ -0,0 +1,111 @@
+module Gitlab
+  module Ci
+    # This was inspired from: http://stackoverflow.com/a/10219411/1520132
+    class Trace
+      BUFFER_SIZE = 4096
+      LIMIT_SIZE = 60
+
+      attr_accessor :stream
+
+      delegate :close, :tell, :seek, :size, :path, :truncate, to: :stream, allow_nil: true
+
+      def initialize()
+        self.stream = yield
+      end
+
+      def use
+        yield self
+      ensure
+        close if valid?
+      end
+
+      def valid?
+        self.stream&.ready?
+      end
+
+      def file?
+        self.path.present?
+      end
+
+      def limit(max_bytes = LIMIT_SIZE)
+        stream_size = size
+        if stream_size < max_bytes
+          max_bytes = stream_size
+        end
+        stream.seek(-max_bytes, IO::SEEK_END)
+      end
+
+      def append(data, offset)
+        stream.truncate(offset)
+        stream.seek(0, IO::SEEK_END)
+        stream.write(data)
+        stream.flush()
+      end
+
+      def set(data)
+        truncate(0)
+        stream.write(data)
+        stream.flush()
+      end
+
+      def read_last_lines(max_lines)
+        chunks = []
+        pos = lines = 0
+        max = file.size
+
+        # We want an extra line to make sure fist line has full contents
+        while lines <= max_lines && pos < max
+          pos += BUFFER_SIZE
+
+          buf =
+            if pos <= max
+              stream.seek(-pos, IO::SEEK_END)
+              stream.read(BUFFER_SIZE)
+            else # Reached the head, read only left
+              stream.seek(0)
+              stream.read(BUFFER_SIZE - (pos - max))
+            end
+
+          lines += buf.count("\n")
+          chunks.unshift(buf)
+        end
+
+        chunks.join.lines.last(max_lines).join
+          .force_encoding(Encoding.default_external)
+      end
+
+      def html_with_state(state = nil)
+        ::Ci::Ansi2html.convert(stream, state)
+      end
+
+      def html_last_lines(max_lines)
+        text = read_last_lines(max_lines)
+        stream = StringIO.new(text)
+        ::Ci::Ansi2html.convert(stream).html
+      end
+
+      def coverage(regex)
+        return unless valid?
+        return unless regex
+
+        regex = Regexp.new(regex)
+
+        match = ""
+
+        stream.each_line do |line|
+          matches = line.scan(regex).last
+          match = matches.last if matches.is_a?(Array)
+        end
+
+        coverage = match.gsub(/\d+(\.\d+)?/).first
+
+        if coverage.present?
+          coverage.to_f
+        end
+      rescue
+        # if bad regex or something goes wrong we dont want to interrupt transition
+        # so we just silentrly ignore error for now
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/trace_reader.rb b/lib/gitlab/ci/trace_reader.rb
deleted file mode 100644
index 1d7ddeb3e0f..00000000000
--- a/lib/gitlab/ci/trace_reader.rb
+++ /dev/null
@@ -1,50 +0,0 @@
-module Gitlab
-  module Ci
-    # This was inspired from: http://stackoverflow.com/a/10219411/1520132
-    class TraceReader
-      BUFFER_SIZE = 4096
-
-      attr_accessor :path, :buffer_size
-
-      def initialize(new_path, buffer_size: BUFFER_SIZE)
-        self.path = new_path
-        self.buffer_size = Integer(buffer_size)
-      end
-
-      def read(last_lines: nil)
-        if last_lines
-          read_last_lines(last_lines)
-        else
-          File.read(path)
-        end
-      end
-
-      def read_last_lines(max_lines)
-        File.open(path) do |file|
-          chunks = []
-          pos = lines = 0
-          max = file.size
-
-          # We want an extra line to make sure fist line has full contents
-          while lines <= max_lines && pos < max
-            pos += buffer_size
-
-            buf = if pos <= max
-                    file.seek(-pos, IO::SEEK_END)
-                    file.read(buffer_size)
-                  else # Reached the head, read only left
-                    file.seek(0)
-                    file.read(buffer_size - (pos - max))
-                  end
-
-            lines += buf.count("\n")
-            chunks.unshift(buf)
-          end
-
-          chunks.join.lines.last(max_lines).join
-            .force_encoding(Encoding.default_external)
-        end
-      end
-    end
-  end
-end