Merge branch 'update-trace-handling-code' into 'master'

Optimise trace handling code to use streaming instead of full read

See merge request !10311

app/assets/javascripts/build.js

@@ -84,10 +84,10 @@ window.Build = (function() {
     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);
         gl.utils.setCiStatusFavicon(`${this.pageUrl}/status.json`);
         if (window.location.hash === DOWN_BUILD_TRACE) {
           $("html,body").scrollTop(this.$buildTrace.height());

app/controllers/projects/builds_controller.rb

@@ -31,25 +31,25 @@ 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.read do |stream|
+      respond_to do |format|
+        format.json do
+          result = {
+            id: @build.id, status: @build.status, complete: @build.complete?
+          }
+
+          if stream.valid?
+            stream.limit
+            state = params[:state].presence
+            trace = stream.html_with_state(state)
+            result.merge!(trace.to_h)
+          end
+
+          render json: result
+        end
       end
     end
   end
@@ -86,10 +86,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.read do |stream|
+      if stream.file?
+        send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline'
+      else
+        render_404
+      end
     end
   end
 

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,136 +231,35 @@ module Ci
     end
 
     def update_coverage
-      coverage = extract_coverage(trace, coverage_regex)
+      coverage = trace.extract_coverage(coverage_regex)
       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
-      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?
+    def trace
+      Gitlab::Ci::Trace.new(self)
     end
 
     def has_trace?
-      raw_trace.present?
+      trace.exist?
     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
+    def trace=(data)
+      raise NotImplementedError
     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)
-    end
-
-    def trace(last_lines: nil)
-      hide_secrets(raw_trace(last_lines: last_lines))
+    def old_trace
+      read_attribute(:trace)
     end
 
-    def trace_length
-      if raw_trace
-        raw_trace.bytesize
-      else
-        0
-      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)
-      end
-    end
-    private :recreate_trace_dir
-
-    def append_trace(trace_part, offset)
-      recreate_trace_dir
-      touch if needs_touch?
-
-      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)
-      end
+    def erase_old_trace!
+      write_attribute(:trace, nil)
+      save
     end
 
     def needs_touch?
       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
     #
@@ -555,6 +441,15 @@ module Ci
       options[:dependencies]&.empty?
     end
 
+    def hide_secrets(trace)
+      return unless trace
+
+      trace = trace.dup
+      Ci::MaskSecret.mask!(trace, project.runners_token) if project
+      Ci::MaskSecret.mask!(trace, token)
+      trace
+    end
+
     private
 
     def update_artifacts_size
@@ -566,7 +461,7 @@ module Ci
     end
 
     def erase_trace!
-      self.trace = nil
+      trace.erase!
     end
 
     def update_erased!(user = nil)
@@ -628,15 +523,6 @@ module Ci
       pipeline.config_processor.build_attributes(name)
     end
 
-    def hide_secrets(trace)
-      return unless trace
-
-      trace = trace.dup
-      Ci::MaskSecret.mask!(trace, project.runners_token) if project
-      Ci::MaskSecret.mask!(trace, token)
-      trace
-    end
-
     def update_project_statistics
       return unless project
 

app/views/notify/pipeline_failed_email.html.haml

@@ -137,6 +137,6 @@
             - if build.has_trace?
               %td{ colspan: "2", style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:0 0 15px;" }
                 %pre{ style: "font-family:Monaco,'Lucida Console','Courier New',Courier,monospace;background-color:#fafafa;border-radius:3px;overflow:hidden;white-space:pre-wrap;word-break:break-all;font-size:13px;line-height:1.4;padding:12px;color:#333333;margin:0;" }
-                  = build.trace_html(last_lines: 10).html_safe
+                  = build.trace.html(last_lines: 10).html_safe
             - else
               %td{ colspan: "2" }

app/views/notify/pipeline_failed_email.text.erb

@@ -35,7 +35,7 @@ had <%= failed.size %> failed <%= 'build'.pluralize(failed.size) %>.
 Stage: <%= build.stage %>
 Name: <%= build.name %>
 <% if build.has_trace? -%>
-Trace: <%= build.trace_with_state(last_lines: 10)[:text] %>
+Trace: <%= build.trace.raw(last_lines: 10) %>
 <% end -%>
 
 <% end -%>

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

db/fixtures/development/14_pipelines.rb

@@ -130,7 +130,7 @@ class Gitlab::Seeder::Pipelines
 
   def setup_build_log(build)
     if %w(running success failed).include?(build.status)
-      build.trace = FFaker::Lorem.paragraphs(6).join("\n\n")
+      build.trace.set(FFaker::Lorem.paragraphs(6).join("\n\n"))
     end
   end
 

features/steps/project/builds/summary.rb

@@ -22,9 +22,9 @@ class Spinach::Features::ProjectBuildsSummary < Spinach::FeatureSteps
   end
 
   step 'recent build has been erased' do
+    expect(@build).not_to have_trace
     expect(@build.artifacts_file.exists?).to be_falsy
     expect(@build.artifacts_metadata.exists?).to be_falsy
-    expect(@build.trace).to be_empty
   end
 
   step 'recent build summary does not have artifacts widget' do

features/steps/shared/builds.rb

@@ -47,7 +47,7 @@ module SharedBuilds
   end
 
   step 'recent build has a build trace' do
-    @build.trace = 'job trace'
+    @build.trace.set('job trace')
   end
 
   step 'download of build artifacts archive starts' do

lib/api/jobs.rb

@@ -118,7 +118,7 @@ module API
         content_type 'text/plain'
         env['api.format'] = :binary
 
-        trace = build.trace
+        trace = build.trace.raw
         body trace
       end
 

lib/api/runner.rb

@@ -115,7 +115,7 @@ module API
       put '/:id' do
         job = authenticate_job!
 
-        job.update_attributes(trace: params[:trace]) if params[:trace]
+        job.trace.set(params[:trace]) if params[:trace]
 
         Gitlab::Metrics.add_event(:update_build,
                                   project: job.project.path_with_namespace)
@@ -145,16 +145,14 @@ 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}" })
+        stream_size = job.trace.append(request.body.read, content_range[0].to_i)
+        if stream_size < 0
+          return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{-stream_size}" })
         end
 
-        job.append_trace(request.body.read, content_range[0].to_i)
-
         status 202
         header 'Job-Status', job.status
-        header 'Range', "0-#{job.trace_length}"
+        header 'Range', "0-#{stream_size}"
       end
 
       desc 'Authorize artifacts uploading for job' do

lib/api/v3/builds.rb

@@ -120,7 +120,7 @@ module API
           content_type 'text/plain'
           env['api.format'] = :binary
 
-          trace = build.trace
+          trace = build.trace.raw
           body trace
         end
 

lib/ci/ansi2html.rb

@@ -132,34 +132,54 @@ 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, stream) 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 = @offset > 0
+        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 }
+        OpenStruct.new(
+          html: @out,
+          state: state,
+          append: append,
+          truncated: truncated,
+          offset: start_offset,
+          size: stream.tell - start_offset,
+          total: stream.size
+        )
       end
 
       def handle_sequence(s)
@@ -240,10 +260,10 @@ module Ci
         Base64.urlsafe_encode64(state.to_json)
       end
 
-      def restore_state(raw, new_state)
+      def restore_state(new_state, stream)
         state = Base64.urlsafe_decode64(new_state)
         state = JSON.parse(state, symbolize_names: true)
-        return if state[:offset].to_i > raw.length
+        return if state[:offset].to_i > stream.size
 
         STATE_PARAMS.each do |param|
           send("#{param}=".to_sym, state[param])

lib/ci/api/builds.rb

@@ -61,7 +61,7 @@ module Ci
 
           update_runner_info
 
-          build.update_attributes(trace: params[:trace]) if params[:trace]
+          build.trace.set(params[:trace]) if params[:trace]
 
           Gitlab::Metrics.add_event(:update_build,
                                     project: build.project.path_with_namespace)
@@ -92,16 +92,14 @@ 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}" })
+          stream_size = build.trace.append(request.body.read, content_range[0].to_i)
+          if stream_size < 0
+            return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{-stream_size}" })
           end
 
-          build.append_trace(request.body.read, content_range[0].to_i)
-
           status 202
           header 'Build-Status', build.status
-          header 'Range', "0-#{build.trace_length}"
+          header 'Range', "0-#{stream_size}"
         end
 
         # Authorize artifacts uploading for build - Runners only

lib/gitlab/ci/trace.rb

+module Gitlab
+  module Ci
+    class Trace
+      attr_reader :job
+
+      delegate :old_trace, to: :job
+
+      def initialize(job)
+        @job = job
+      end
+
+      def html(last_lines: nil)
+        read do |stream|
+          stream.html(last_lines: last_lines)
+        end
+      end
+
+      def raw(last_lines: nil)
+        read do |stream|
+          stream.raw(last_lines: last_lines)
+        end
+      end
+
+      def extract_coverage(regex)
+        read do |stream|
+          stream.extract_coverage(regex)
+        end
+      end
+
+      def set(data)
+        write do |stream|
+          data = job.hide_secrets(data)
+          stream.set(data)
+        end
+      end
+
+      def append(data, offset)
+        write do |stream|
+          current_length = stream.size
+          return -current_length unless current_length == offset
+
+          data = job.hide_secrets(data)
+          stream.append(data, offset)
+          stream.size
+        end
+      end
+
+      def exist?
+        current_path.present? || old_trace.present?
+      end
+
+      def read
+        stream = Gitlab::Ci::Trace::Stream.new do
+          if current_path
+            File.open(current_path, "rb")
+          elsif old_trace
+            StringIO.new(old_trace)
+          end
+        end
+
+        yield stream
+      ensure
+        stream&.close
+      end
+
+      def write
+        stream = Gitlab::Ci::Trace::Stream.new do
+          File.open(ensure_path, "a+b")
+        end
+
+        yield(stream).tap do
+          job.touch if job.needs_touch?
+        end
+      ensure
+        stream&.close
+      end
+
+      def erase!
+        paths.each do |trace_path|
+          FileUtils.rm(trace_path, force: true)
+        end
+
+        job.erase_old_trace!
+      end
+
+      private
+
+      def ensure_path
+        return current_path if current_path
+
+        ensure_directory
+        default_path
+      end
+
+      def ensure_directory
+        unless Dir.exist?(default_directory)
+          FileUtils.mkdir_p(default_directory)
+        end
+      end
+
+      def current_path
+        @current_path ||= paths.find do |trace_path|
+          File.exist?(trace_path)
+        end
+      end
+
+      def paths
+        [
+          default_path,
+          deprecated_path
+        ].compact
+      end
+
+      def default_directory
+        File.join(
+          Settings.gitlab_ci.builds_path,
+          job.created_at.utc.strftime("%Y_%m"),
+          job.project_id.to_s
+        )
+      end
+
+      def default_path
+        File.join(default_directory, "#{job.id}.log")
+      end
+
+      def deprecated_path
+        File.join(
+          Settings.gitlab_ci.builds_path,
+          job.created_at.utc.strftime("%Y_%m"),
+          job.project.ci_id.to_s,
+          "#{job.id}.log"
+        ) if job.project&.ci_id
+      end
+    end
+  end
+end

lib/gitlab/ci/trace/stream.rb

+module Gitlab
+  module Ci
+    class Trace
+      # This was inspired from: http://stackoverflow.com/a/10219411/1520132
+      class Stream
+        BUFFER_SIZE = 4096
+        LIMIT_SIZE = 50.kilobytes
+
+        attr_reader :stream
+
+        delegate :close, :tell, :seek, :size, :path, :truncate, to: :stream, allow_nil: true
+
+        delegate :valid?, to: :stream, as: :present?, allow_nil: true
+
+        def initialize
+          @stream = yield
+        end
+
+        def valid?
+          self.stream.present?
+        end
+
+        def file?
+          self.path.present?
+        end
+
+        def limit(last_bytes = LIMIT_SIZE)
+          stream_size = size
+          if stream_size < last_bytes
+            last_bytes = stream_size
+          end
+          stream.seek(-last_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 raw(last_lines: nil)
+          return unless valid?
+
+          if last_lines.to_i > 0
+            read_last_lines(last_lines)
+          else
+            stream.read
+          end
+        end
+
+        def html_with_state(state = nil)
+          ::Ci::Ansi2html.convert(stream, state)
+        end
+
+        def html(last_lines: nil)
+          text = raw(last_lines: last_lines)
+          stream = StringIO.new(text)
+          ::Ci::Ansi2html.convert(stream).html
+        end
+
+        def extract_coverage(regex)
+          return unless valid?
+          return unless regex
+
+          regex = Regexp.new(regex)
+
+          match = ""
+
+          stream.each_line do |line|
+            matches = line.scan(regex)
+            next unless matches.is_a?(Array)
+
+            match = matches.flatten.last
+            coverage = match.gsub(/\d+(\.\d+)?/).first
+            return coverage.to_f if coverage.present?
+          end
+        rescue
+          # if bad regex or something goes wrong we dont want to interrupt transition
+          # so we just silentrly ignore error for now
+        end
+
+        private
+
+        def read_last_lines(last_lines)
+          chunks = []
+          pos = lines = 0
+          max = stream.size
+
+          # We want an extra line to make sure fist line has full contents
+          while lines <= last_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(last_lines).join
+            .force_encoding(Encoding.default_external)
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/trace_reader.rb

-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

spec/factories/ci/builds.rb

@@ -111,7 +111,7 @@ FactoryGirl.define do
 
     trait :trace do
       after(:create) do |build, evaluator|
-        build.trace = 'BUILD TRACE'
+        build.trace.set('BUILD TRACE')
       end
     end
 

spec/features/projects/builds_spec.rb

@@ -205,21 +205,13 @@ feature 'Builds', :feature do
         it 'loads job trace' do
           expect(page).to have_content 'BUILD TRACE'
 
-          build.append_trace(' and more trace', 11)
+          build.trace.write do |stream|
+            stream.append(' and more trace', 11)
+          end
 
           expect(page).to have_content 'BUILD TRACE and more trace'
         end
       end
-
-      context 'when build does not have an initial trace' do
-        let(:build) { create(:ci_build, pipeline: pipeline) }
-
-        it 'loads new trace' do
-          build.append_trace('build trace', 0)
-
-          expect(page).to have_content 'build trace'
-        end
-      end
     end
 
     feature 'Variables' do
@@ -390,7 +382,7 @@ feature 'Builds', :feature do
         it 'sends the right headers' do
           expect(page.status_code).to eq(200)
           expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
-          expect(page.response_headers['X-Sendfile']).to eq(build.path_to_trace)
+          expect(page.response_headers['X-Sendfile']).to eq(build.trace.send(:current_path))
         end
       end
 
@@ -409,43 +401,24 @@ feature 'Builds', :feature do
 
     context 'storage form' do
       let(:existing_file) { Tempfile.new('existing-trace-file').path }
-      let(:non_existing_file) do
-        file = Tempfile.new('non-existing-trace-file')
-        path = file.path
-        file.unlink
-        path
-      end
 
-      context 'when build has trace in file' do
-        before do
-          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-          build.run!
-          visit namespace_project_build_path(project.namespace, project, build)
+      before do
+        Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
 
-          allow_any_instance_of(Project).to receive(:ci_id).and_return(nil)
-          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(existing_file)
-          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(non_existing_file)
+        build.run!
 
-          page.within('.js-build-sidebar') { click_link 'Raw' }
-        end
+        allow_any_instance_of(Gitlab::Ci::Trace).to receive(:paths)
+          .and_return(paths)
 
-        it 'sends the right headers' do
-          expect(page.status_code).to eq(200)
-          expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
-          expect(page.response_headers['X-Sendfile']).to eq(existing_file)
-        end
+        visit namespace_project_build_path(project.namespace, project, build)
       end
 
-      context 'when build has trace in old file' do
-        before do
-          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-          build.run!
-          visit namespace_project_build_path(project.namespace, project, build)
-
-          allow_any_instance_of(Project).to receive(:ci_id).and_return(999)
-          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file)
-          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(existing_file)
+      context 'when build has trace in file' do
+        let(:paths) do
+          [existing_file]
+        end
 
+        before do
           page.within('.js-build-sidebar') { click_link 'Raw' }
         end
 
@@ -457,20 +430,10 @@ feature 'Builds', :feature do
       end
 
       context 'when build has trace in DB' do
-        before do
-          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-          build.run!
-          visit namespace_project_build_path(project.namespace, project, build)
-
-          allow_any_instance_of(Project).to receive(:ci_id).and_return(nil)
-          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file)
-          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(non_existing_file)
-
-          page.within('.js-build-sidebar') { click_link 'Raw' }
-        end
+        let(:paths) { [] }
 
         it 'sends the right headers' do
-          expect(page.status_code).to eq(404)
+          expect(page.status_code).not_to have_link('Raw')
         end
       end
     end

spec/javascripts/build_spec.js

@@ -72,12 +72,14 @@ describe('Build', () => {
       it('displays the initial build trace', () => {
         expect($.ajax.calls.count()).toBe(1);
         const [{ url, dataType, success, context }] = $.ajax.calls.argsFor(0);
-        expect(url).toBe(`${BUILD_URL}.json`);
+        expect(url).toBe(
+          `${BUILD_URL}/trace.json`,
+        );
         expect(dataType).toBe('json');
         expect(success).toEqual(jasmine.any(Function));
         spyOn(gl.utils, 'setCiStatusFavicon').and.callFake(() => {});
 
-        success.call(context, { trace_html: '<span>Example</span>', status: 'running' });
+        success.call(context, { html: '<span>Example</span>', status: 'running' });
 
         expect($('#build-trace .js-build-output').text()).toMatch(/Example/);
       });

spec/lib/gitlab/ci/trace/stream_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Trace::Stream do
+  describe 'delegates' do
+    subject { described_class.new { nil } }
+
+    it { is_expected.to delegate_method(:close).to(:stream) }
+    it { is_expected.to delegate_method(:tell).to(:stream) }
+    it { is_expected.to delegate_method(:seek).to(:stream) }
+    it { is_expected.to delegate_method(:size).to(:stream) }
+    it { is_expected.to delegate_method(:path).to(:stream) }
+    it { is_expected.to delegate_method(:truncate).to(:stream) }
+    it { is_expected.to delegate_method(:valid?).to(:stream).as(:present?) }
+    it { is_expected.to delegate_method(:file?).to(:path).as(:present?) }
+  end
+
+  describe '#limit' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
+      end
+    end
+
+    it 'if size is larger we start from beggining' do
+      stream.limit(10)
+
+      expect(stream.tell).to eq(0)
+    end
+
+    it 'if size is smaller we start from the end' do
+      stream.limit(2)
+
+      expect(stream.tell).to eq(6)
+    end
+  end
+
+  describe '#append' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
+      end
+    end
+
+    it "truncates and append content" do
+      stream.append("89", 4)
+      stream.seek(0)
+
+      expect(stream.size).to eq(6)
+      expect(stream.raw).to eq("123489")
+    end
+  end
+
+  describe '#set' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12345678")
+      end
+    end
+
+    before do
+      stream.set("8901")
+    end
+
+    it "overwrite content" do
+      stream.seek(0)
+
+      expect(stream.size).to eq(4)
+      expect(stream.raw).to eq("8901")
+    end
+  end
+
+  describe '#raw' do
+    let(:path) { __FILE__ }
+    let(:lines) { File.readlines(path) }
+    let(:stream) do
+      described_class.new do
+        File.open(path)
+      end
+    end
+
+    it 'returns all contents if last_lines is not specified' do
+      result = stream.raw
+
+      expect(result).to eq(lines.join)
+      expect(result.encoding).to eq(Encoding.default_external)
+    end
+
+    context 'limit max lines' do
+      before do
+        # specifying BUFFER_SIZE forces to seek backwards
+        allow(described_class).to receive(:BUFFER_SIZE)
+          .and_return(2)
+      end
+
+      it 'returns last few lines' do
+        result = stream.raw(last_lines: 2)
+
+        expect(result).to eq(lines.last(2).join)
+        expect(result.encoding).to eq(Encoding.default_external)
+      end
+
+      it 'returns everything if trying to get too many lines' do
+        result = stream.raw(last_lines: lines.size * 2)
+
+        expect(result).to eq(lines.join)
+        expect(result.encoding).to eq(Encoding.default_external)
+      end
+    end
+  end
+
+  describe '#html_with_state' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("1234")
+      end
+    end
+
+    it 'returns html content with state' do
+      result = stream.html_with_state
+
+      expect(result.html).to eq("1234")
+    end
+
+    context 'follow-up state' do
+      let!(:last_result) { stream.html_with_state }
+
+      before do
+        stream.append("5678", 4)
+        stream.seek(0)
+      end
+
+      it "returns appended trace" do
+        result = stream.html_with_state(last_result.state)
+
+        expect(result.append).to be_truthy
+        expect(result.html).to eq("5678")
+      end
+    end
+  end
+
+  describe '#html' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new("12\n34\n56")
+      end
+    end
+
+    it "returns html" do
+      expect(stream.html).to eq("12<br>34<br>56")
+    end
+
+    it "returns html for last line only" do
+      expect(stream.html(last_lines: 1)).to eq("56")
+    end
+  end
+
+  describe '#extract_coverage' do
+    let(:stream) do
+      described_class.new do
+        StringIO.new(data)
+      end
+    end
+
+    subject { stream.extract_coverage(regex) }
+
+    context 'valid content & regex' do
+      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to eq(98.29) }
+    end
+
+    context 'valid content & bad regex' do
+      let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered\n' }
+      let(:regex) { 'very covered' }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'no coverage content & regex' do
+      let(:data) { 'No coverage for today :sad:' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'multiple results in content & regex' do
+      let(:data) { ' (98.39%) covered. (98.29%) covered' }
+      let(:regex) { '\(\d+.\d+\%\) covered' }
+
+      it { is_expected.to eq(98.29) }
+    end
+
+    context 'using a regex capture' do
+      let(:data) { 'TOTAL      9926   3489    65%' }
+      let(:regex) { 'TOTAL\s+\d+\s+\d+\s+(\d{1,3}\%)' }
+
+      it { is_expected.to eq(65) }
+    end
+  end
+end

spec/lib/gitlab/ci/trace_reader_spec.rb

-require 'spec_helper'
-
-describe Gitlab::Ci::TraceReader do
-  let(:path) { __FILE__ }
-  let(:lines) { File.readlines(path) }
-  let(:bytesize) { lines.sum(&:bytesize) }
-
-  it 'returns last few lines' do
-    10.times do
-      subject = build_subject
-      last_lines = random_lines
-
-      expected = lines.last(last_lines).join
-      result = subject.read(last_lines: last_lines)
-
-      expect(result).to eq(expected)
-      expect(result.encoding).to eq(Encoding.default_external)
-    end
-  end
-
-  it 'returns everything if trying to get too many lines' do
-    result = build_subject.read(last_lines: lines.size * 2)
-
-    expect(result).to eq(lines.join)
-    expect(result.encoding).to eq(Encoding.default_external)
-  end
-
-  it 'returns all contents if last_lines is not specified' do
-    result = build_subject.read
-
-    expect(result).to eq(lines.join)
-    expect(result.encoding).to eq(Encoding.default_external)
-  end
-
-  it 'raises an error if not passing an integer for last_lines' do
-    expect do
-      build_subject.read(last_lines: lines)
-    end.to raise_error(ArgumentError)
-  end
-
-  def random_lines
-    Random.rand(lines.size) + 1
-  end
-
-  def random_buffer
-    Random.rand(bytesize) + 1
-  end
-
-  def build_subject
-    described_class.new(__FILE__, buffer_size: random_buffer)
-  end
-end

spec/lib/gitlab/ci/trace_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Trace do
+  let(:build) { create(:ci_build) }
+  let(:trace) { described_class.new(build) }
+
+  describe "associations" do
+    it { expect(trace).to respond_to(:job) }
+    it { expect(trace).to delegate_method(:old_trace).to(:job) }
+  end
+
+  describe '#html' do
+    before do
+      trace.set("12\n34")
+    end
+
+    it "returns formatted html" do
+      expect(trace.html).to eq("12<br>34")
+    end
+
+    it "returns last line of formatted html" do
+      expect(trace.html(last_lines: 1)).to eq("34")
+    end
+  end
+
+  describe '#raw' do
+    before do
+      trace.set("12\n34")
+    end
+
+    it "returns raw output" do
+      expect(trace.raw).to eq("12\n34")
+    end
+
+    it "returns last line of raw output" do
+      expect(trace.raw(last_lines: 1)).to eq("34")
+    end
+  end
+
+  describe '#extract_coverage' do
+    let(:regex) { '\(\d+.\d+\%\) covered' }
+
+    before do
+      trace.set('Coverage 1033 / 1051 LOC (98.29%) covered')
+    end
+
+    it "returns valid coverage" do
+      expect(trace.extract_coverage(regex)).to eq(98.29)
+    end
+  end
+
+  describe '#set' do
+    before do
+      trace.set("12")
+    end
+
+    it "returns trace" do
+      expect(trace.raw).to eq("12")
+    end
+
+    context 'overwrite trace' do
+      before do
+        trace.set("34")
+      end
+
+      it "returns new trace" do
+        expect(trace.raw).to eq("34")
+      end
+    end
+
+    context 'runners token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.project.update(runners_token: token)
+        trace.set(token)
+      end
+
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
+    end
+
+    context 'hides build token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(token: token)
+        trace.set(token)
+      end
+
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
+    end
+  end
+
+  describe '#append' do
+    before do
+      trace.set("1234")
+    end
+
+    it "returns correct trace" do
+      expect(trace.append("56", 4)).to eq(6)
+      expect(trace.raw).to eq("123456")
+    end
+
+    context 'tries to append trace at different offset' do
+      it "fails with append" do
+        expect(trace.append("56", 2)).to eq(-4)
+        expect(trace.raw).to eq("1234")
+      end
+    end
+
+    context 'runners token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.project.update(runners_token: token)
+        trace.append(token, 0)
+      end
+
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
+    end
+
+    context 'build token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(token: token)
+        trace.append(token, 0)
+      end
+
+      it "hides token" do
+        expect(trace.raw).not_to include(token)
+      end
+    end
+  end
+
+  describe 'trace handling' do
+    context 'trace does not exist' do
+      it { expect(trace.exist?).to be(false) }
+    end
+
+    context 'new trace path is used' do
+      before do
+        trace.send(:ensure_directory)
+
+        File.open(trace.send(:default_path), "w") do |file|
+          file.write("data")
+        end
+      end
+
+      it "trace exist" do
+        expect(trace.exist?).to be(true)
+      end
+
+      it "can be erased" do
+        trace.erase!
+        expect(trace.exist?).to be(false)
+      end
+    end
+
+    context 'deprecated path' do
+      let(:path) { trace.send(:deprecated_path) }
+
+      context 'with valid ci_id' do
+        before do
+          build.project.update(ci_id: 1000)
+
+          FileUtils.mkdir_p(File.dirname(path))
+
+          File.open(path, "w") do |file|
+            file.write("data")
+          end
+        end
+
+        it "trace exist" do
+          expect(trace.exist?).to be(true)
+        end
+
+        it "can be erased" do
+          trace.erase!
+          expect(trace.exist?).to be(false)
+        end
+      end
+
+      context 'without valid ci_id' do
+        it "does not return deprecated path" do
+          expect(path).to be_nil
+        end
+      end
+    end
+
+    context 'stored in database' do
+      before do
+        build.send(:write_attribute, :trace, "data")
+      end
+
+      it "trace exist" do
+        expect(trace.exist?).to be(true)
+      end
+
+      it "can be erased" do
+        trace.erase!
+        expect(trace.exist?).to be(false)
+      end
+
+      it "returns database data" do
+        expect(trace.raw).to eq("data")
+      end
+    end
+  end
+end

spec/requests/api/jobs_spec.rb

@@ -320,7 +320,7 @@ describe API::Jobs, api: true do
     context 'authorized user' do
       it 'returns specific job trace' do
         expect(response).to have_http_status(200)
-        expect(response.body).to eq(build.trace)
+        expect(response.body).to eq(build.trace.raw)
       end
     end
 
@@ -408,7 +408,7 @@ describe API::Jobs, api: true do
 
       it 'erases job content' do
         expect(response).to have_http_status(201)
-        expect(build.trace).to be_empty
+        expect(build).not_to have_trace
         expect(build.artifacts_file.exists?).to be_falsy
         expect(build.artifacts_metadata.exists?).to be_falsy
       end

spec/requests/api/runner_spec.rb

@@ -592,7 +592,7 @@ describe API::Runner do
           update_job(trace: 'BUILD TRACE UPDATED')
 
           expect(response).to have_http_status(200)
-          expect(job.reload.trace).to eq 'BUILD TRACE UPDATED'
+          expect(job.reload.trace.raw).to eq 'BUILD TRACE UPDATED'
         end
       end
 
@@ -600,7 +600,7 @@ describe API::Runner do
         it 'does not override trace information' do
           update_job
 
-          expect(job.reload.trace).to eq 'BUILD TRACE'
+          expect(job.reload.trace.raw).to eq 'BUILD TRACE'
         end
       end
 
@@ -631,7 +631,7 @@ describe API::Runner do
       context 'when request is valid' do
         it 'gets correct response' do
           expect(response.status).to eq 202
-          expect(job.reload.trace).to eq 'BUILD TRACE appended'
+          expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Job-Status'
         end
@@ -642,7 +642,7 @@ describe API::Runner do
           it "changes the job's trace" do
             patch_the_trace
 
-            expect(job.reload.trace).to eq 'BUILD TRACE appended appended'
+            expect(job.reload.trace.raw).to eq 'BUILD TRACE appended appended'
           end
 
           context 'when Runner makes a force-patch' do
@@ -651,7 +651,7 @@ describe API::Runner do
             it "doesn't change the build.trace" do
               force_patch_the_trace
 
-              expect(job.reload.trace).to eq 'BUILD TRACE appended'
+              expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
             end
           end
         end
@@ -664,7 +664,7 @@ describe API::Runner do
           it 'changes the job.trace' do
             patch_the_trace
 
-            expect(job.reload.trace).to eq 'BUILD TRACE appended appended'
+            expect(job.reload.trace.raw).to eq 'BUILD TRACE appended appended'
           end
 
           context 'when Runner makes a force-patch' do
@@ -673,7 +673,7 @@ describe API::Runner do
             it "doesn't change the job.trace" do
               force_patch_the_trace
 
-              expect(job.reload.trace).to eq 'BUILD TRACE appended'
+              expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
             end
           end
         end
@@ -698,7 +698,7 @@ describe API::Runner do
 
         it 'gets correct response' do
           expect(response.status).to eq 202
-          expect(job.reload.trace).to eq 'BUILD TRACE appended'
+          expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Job-Status'
         end
@@ -738,9 +738,11 @@ describe API::Runner do
 
       def patch_the_trace(content = ' appended', request_headers = nil)
         unless request_headers
-          offset = job.trace_length
-          limit = offset + content.length - 1
-          request_headers = headers.merge({ 'Content-Range' => "#{offset}-#{limit}" })
+          job.trace.read do |stream|
+            offset = stream.size
+            limit = offset + content.length - 1
+            request_headers = headers.merge({ 'Content-Range' => "#{offset}-#{limit}" })
+          end
         end
 
         Timecop.travel(job.updated_at + update_interval) do

spec/requests/api/v3/builds_spec.rb

@@ -330,7 +330,7 @@ describe API::V3::Builds, api: true do
     context 'authorized user' do
       it 'returns specific job trace' do
         expect(response).to have_http_status(200)
-        expect(response.body).to eq(build.trace)
+        expect(response.body).to eq(build.trace.raw)
       end
     end
 
@@ -418,7 +418,7 @@ describe API::V3::Builds, api: true do
 
       it 'erases job content' do
         expect(response.status).to eq 201
-        expect(build.trace).to be_empty
+        expect(build).not_to have_trace
         expect(build.artifacts_file.exists?).to be_falsy
         expect(build.artifacts_metadata.exists?).to be_falsy
       end

spec/requests/ci/api/builds_spec.rb

@@ -285,7 +285,7 @@ describe Ci::API::Builds do
       end
 
       it 'does not override trace information when no trace is given' do
-        expect(build.reload.trace).to eq 'BUILD TRACE'
+        expect(build.reload.trace.raw).to eq 'BUILD TRACE'
       end
 
       context 'job has been erased' do
@@ -309,9 +309,11 @@ describe Ci::API::Builds do
 
       def patch_the_trace(content = ' appended', request_headers = nil)
         unless request_headers
-          offset = build.trace_length
-          limit = offset + content.length - 1
-          request_headers = headers.merge({ 'Content-Range' => "#{offset}-#{limit}" })
+          build.trace.read do |stream|
+            offset = stream.size
+            limit = offset + content.length - 1
+            request_headers = headers.merge({ 'Content-Range' => "#{offset}-#{limit}" })
+          end
         end
 
         Timecop.travel(build.updated_at + update_interval) do
@@ -335,7 +337,7 @@ describe Ci::API::Builds do
       context 'when request is valid' do
         it 'gets correct response' do
           expect(response.status).to eq 202
-          expect(build.reload.trace).to eq 'BUILD TRACE appended'
+          expect(build.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Build-Status'
         end
@@ -346,7 +348,7 @@ describe Ci::API::Builds do
           it 'changes the build trace' do
             patch_the_trace
 
-            expect(build.reload.trace).to eq 'BUILD TRACE appended appended'
+            expect(build.reload.trace.raw).to eq 'BUILD TRACE appended appended'
           end
 
           context 'when Runner makes a force-patch' do
@@ -355,7 +357,7 @@ describe Ci::API::Builds do
             it "doesn't change the build.trace" do
               force_patch_the_trace
 
-              expect(build.reload.trace).to eq 'BUILD TRACE appended'
+              expect(build.reload.trace.raw).to eq 'BUILD TRACE appended'
             end
           end
         end
@@ -368,7 +370,7 @@ describe Ci::API::Builds do
           it 'changes the build.trace' do
             patch_the_trace
 
-            expect(build.reload.trace).to eq 'BUILD TRACE appended appended'
+            expect(build.reload.trace.raw).to eq 'BUILD TRACE appended appended'
           end
 
           context 'when Runner makes a force-patch' do
@@ -377,7 +379,7 @@ describe Ci::API::Builds do
             it "doesn't change the build.trace" do
               force_patch_the_trace
 
-              expect(build.reload.trace).to eq 'BUILD TRACE appended'
+              expect(build.reload.trace.raw).to eq 'BUILD TRACE appended'
             end
           end
         end
@@ -403,7 +405,7 @@ describe Ci::API::Builds do
 
         it 'gets correct response' do
           expect(response.status).to eq 202
-          expect(build.reload.trace).to eq 'BUILD TRACE appended'
+          expect(build.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Build-Status'
         end