Enforce proper 416 support for runner trace patch endpoint

lib/api/runner.rb

@@ -153,9 +153,20 @@ module API
         content_range = request.headers['Content-Range']
         content_range = content_range.split('-')
 
-        stream_size = job.trace.append(request.body.read, content_range[0].to_i)
-        if stream_size < 0
-          break error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{-stream_size}" })
+        # TODO:
+        # it seems that `Content-Range` as formatted by runner is wrong,
+        # the `byte_end` should point to final byte, but it points byte+1
+        # that means that we have to calculate end of body,
+        # as we cannot use `content_length[1]`
+        # Issue: https://gitlab.com/gitlab-org/gitlab-runner/issues/3275
+
+        body_data = request.body.read
+        body_start = content_range[0].to_i
+        body_end = body_start + body_data.bytesize
+
+        stream_size = job.trace.append(body_data, body_start)
+        unless stream_size == body_end
+          break error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{stream_size}" })
         end
 
         status 202

lib/gitlab/ci/trace.rb

@@ -45,7 +45,7 @@ module Gitlab
       def append(data, offset)
         write('a+b') do |stream|
           current_length = stream.size
-          break -current_length unless current_length == offset
+          break current_length unless current_length == offset
 
           data = job.hide_secrets(data)
           stream.append(data, offset)

spec/requests/api/runner_spec.rb

@@ -906,20 +906,18 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
 
             context 'when we perform partial patch' do
               before do
-                patch_the_trace('hello', headers.merge({ 'Content-Range' => "28-32" }))
+                patch_the_trace('hello', headers.merge({ 'Content-Range' => "28-32/5" }))
               end
 
               it 'returns an error' do
-                expect(response.status).to eq(202)
-                expect(response.header).to have_key 'Range'
-                expect(response.header['Range']).to eq '0-0'
-                expect(job.reload.trace.raw).to eq ''
+                expect(response.status).to eq(416)
+                expect(response.header['Range']).to eq('0-0')
               end
             end
 
             context 'when we resend full trace' do
               before do
-                patch_the_trace('BUILD TRACE appended appended hello', headers.merge({ 'Content-Range' => "0-32" }))
+                patch_the_trace('BUILD TRACE appended appended hello', headers.merge({ 'Content-Range' => "0-34/35" }))
               end
 
               it 'succeeds with updating trace' do
@@ -945,7 +943,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
       end
 
       context 'when content-range start is too big' do
-        let(:headers_with_range) { headers.merge({ 'Content-Range' => '15-20' }) }
+        let(:headers_with_range) { headers.merge({ 'Content-Range' => '15-20/6' }) }
 
         it 'gets 416 error response with range headers' do
           expect(response.status).to eq 416
@@ -955,7 +953,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
       end
 
       context 'when content-range start is too small' do
-        let(:headers_with_range) { headers.merge({ 'Content-Range' => '8-20' }) }
+        let(:headers_with_range) { headers.merge({ 'Content-Range' => '8-20/13' }) }
 
         it 'gets 416 error response with range headers' do
           expect(response.status).to eq 416