Respond with distinct error when file not found

…as opposed to record not found.

db/schema.rb

@@ -1077,6 +1077,9 @@ ActiveRecord::Schema.define(version: 20180405101928) do
     t.integer "repositories_verification_failed_count"
     t.integer "wikis_verified_count"
     t.integer "wikis_verification_failed_count"
+    t.integer "lfs_objects_synced_missing_on_primary_count"
+    t.integer "job_artifacts_synced_missing_on_primary_count"
+    t.integer "attachments_synced_missing_on_primary_count"
   end
 
   add_index "geo_node_statuses", ["geo_node_id"], name: "index_geo_node_statuses_on_geo_node_id", unique: true, using: :btree

ee/app/services/geo/file_upload_service.rb

 module Geo
+  # This class is responsible for:
+  #   * Handling file requests from the secondary over the API
+  #   * Returning the necessary response data to send the file back
   class FileUploadService < FileService
     attr_reader :auth_header
 

ee/lib/api/geo.rb

@@ -23,8 +23,7 @@ module API
           file = response[:file]
           present_disk_file!(file.path, file.filename)
         else
-          status response[:code]
-          response
+          error! response, response.delete(:code)
         end
       end
 

ee/lib/gitlab/geo/file_transfer.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Requesting an Upload file from the primary
+    #   * Saving it in the right place on successful download
+    #   * Returning a detailed Result object
     class FileTransfer < Transfer
       def initialize(file_type, upload)
         @file_type = file_type

ee/lib/gitlab/geo/file_uploader.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Finding an Upload record
+    #   * Returning the necessary response data to send the file back
+    #
+    # TODO: Rearrange things so this class not inherited by JobArtifactUploader and LfsUploader
+    # Maybe rename it so it doesn't seem generic. It only works with Upload records.
     class FileUploader
+      include LogHelpers
+
+      FILE_NOT_FOUND_GEO_CODE = 'FILE_NOT_FOUND'.freeze
+
       attr_reader :object_db_id, :message
 
       def initialize(object_db_id, message)
@@ -11,8 +21,9 @@ module Gitlab
       def execute
         recorded_file = Upload.find_by(id: object_db_id)
 
-        return error unless recorded_file&.exist?
-        return error unless valid?(recorded_file)
+        return error('Upload not found') unless recorded_file
+        return file_not_found(recorded_file) unless recorded_file.exist?
+        return error('Upload not found') unless valid?(recorded_file)
 
         success(CarrierWave::SanitizedFile.new(recorded_file.absolute_path))
       end
@@ -37,9 +48,20 @@ module Gitlab
         { code: :ok, message: 'Success', file: file }
       end
 
-      def error(message = 'File not found')
+      def error(message)
         { code: :not_found, message: message }
       end
+
+      # A 404 implies the client made a mistake requesting that resource.
+      # In this case, we know that the resource should exist, so it is a 500 server error.
+      # We send a special "geo_code" so the secondary can mark the file as synced.
+      def file_not_found(resource)
+        {
+          code: :not_found,
+          geo_code: FILE_NOT_FOUND_GEO_CODE,
+          message: "#{resource.class.name} ##{resource.id} file not found"
+        }
+      end
     end
   end
 end

ee/lib/gitlab/geo/job_artifact_transfer.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Requesting an ::Ci::JobArtifact file from the primary
+    #   * Saving it in the right place on successful download
+    #   * Returning a detailed Result object
     class JobArtifactTransfer < Transfer
       def initialize(job_artifact)
         @file_type = :job_artifact

ee/lib/gitlab/geo/job_artifact_uploader.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Finding an ::Ci::JobArtifact record
+    #   * Returning the necessary response data to send the file back
+    #
+    # TODO: Rearrange things so this class does not inherit from FileUploader
     class JobArtifactUploader < ::Gitlab::Geo::FileUploader
       def execute
         job_artifact = ::Ci::JobArtifact.find_by(id: object_db_id)
@@ -9,7 +14,9 @@ module Gitlab
         end
 
         unless job_artifact.file.present? && job_artifact.file.exists?
-          return error('Job artifact does not have a file')
+          log_error("Could not upload job artifact because it does not have a file", id: job_artifact.id)
+
+          return file_not_found(job_artifact)
         end
 
         success(job_artifact.file)

ee/lib/gitlab/geo/lfs_transfer.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Requesting an LfsObject file from the primary
+    #   * Saving it in the right place on successful download
+    #   * Returning a detailed Result object
     class LfsTransfer < Transfer
       def initialize(lfs_object)
         @file_type = :lfs

ee/lib/gitlab/geo/lfs_uploader.rb

 module Gitlab
   module Geo
+    # This class is responsible for:
+    #   * Finding an LfsObject record
+    #   * Returning the necessary response data to send the file back
+    #
+    # TODO: Rearrange things so this class does not inherit from FileUploader
     class LfsUploader < FileUploader
       def execute
         lfs_object = LfsObject.find_by(id: object_db_id)
 
-        return error unless lfs_object.present?
-        return error if message[:checksum] != lfs_object.oid
+        return error('LFS object not found') unless lfs_object
+        return error('LFS object not found') if message[:checksum] != lfs_object.oid
 
         unless lfs_object.file.present? && lfs_object.file.exists?
-          return error('LFS object does not have a file')
+          log_error("Could not upload LFS object because it does not have a file", id: lfs_object.id)
+
+          return file_not_found(lfs_object)
         end
 
         success(lfs_object.file)

ee/lib/gitlab/geo/transfer.rb

@@ -14,25 +14,39 @@ module Gitlab
         @request_data = request_data
       end
 
-      # Returns number of bytes downloaded or -1 if unsuccessful.
+      # Returns Result object with success boolean and number of bytes downloaded.
       def download_from_primary
-        return unless Gitlab::Geo.secondary?
-        return if File.directory?(filename)
+        return failure unless Gitlab::Geo.secondary?
+        return failure if File.directory?(filename)
 
         primary = Gitlab::Geo.primary_node
 
-        return unless primary
+        return failure unless primary
 
         url = primary.geo_transfers_url(file_type, file_id.to_s)
         req_headers = TransferRequest.new(request_data).headers
 
-        return unless ensure_path_exists
+        return failure unless ensure_path_exists
 
         download_file(url, req_headers)
       end
 
+      class Result
+        attr_reader :success, :bytes_downloaded, :primary_missing_file
+
+        def initialize(success:, bytes_downloaded:, primary_missing_file: false)
+          @success = success
+          @bytes_downloaded = bytes_downloaded
+          @primary_missing_file = primary_missing_file
+        end
+      end
+
       private
 
+      def failure(primary_missing_file: false)
+        Result.new(success: false, bytes_downloaded: 0, primary_missing_file: primary_missing_file)
+      end
+
       def ensure_path_exists
         path = Pathname.new(filename)
         dir = path.dirname
@@ -55,7 +69,7 @@ module Gitlab
         file_size = -1
         temp_file = open_temp_file(filename)
 
-        return unless temp_file
+        return failure unless temp_file
 
         begin
           response = Gitlab::HTTP.get(url, allow_local_requests: true, headers: req_headers, stream_body: true) do |fragment|
@@ -65,13 +79,13 @@ module Gitlab
           temp_file.flush
 
           unless response.success?
-            log_error("Unsuccessful download", filename: filename, response_code: response.code, response_msg: response.msg, url: url)
-            return file_size
+            log_error("Unsuccessful download", filename: filename, response_code: response.code, response_msg: response.try(:msg), url: url)
+            return failure(primary_missing_file: primary_missing_file?(response, temp_file))
           end
 
           if File.directory?(filename)
             log_error("Destination file is a directory", filename: filename)
-            return file_size
+            return failure
           end
 
           FileUtils.mv(temp_file.path, filename)
@@ -85,7 +99,18 @@ module Gitlab
           temp_file.unlink
         end
 
-        file_size
+        Result.new(success: file_size > -1, bytes_downloaded: [file_size, 0].max)
+      end
+
+      def primary_missing_file?(response, temp_file)
+        body = File.read(temp_file.path) if File.exist?(temp_file.path)
+
+        if response.code == 404 && body.present?
+          json_response = JSON.parse(body)
+          json_response['geo_code'] == Gitlab::Geo::FileUploader::FILE_NOT_FOUND_GEO_CODE
+        end
+      rescue JSON::ParserError
+        false
       end
 
       def default_permissions

ee/spec/lib/gitlab/geo/file_uploader_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Geo::FileUploader, :geo do
+  shared_examples_for 'returns necessary params for sending a file from an API endpoint' do
+    subject { @subject ||= uploader.execute }
+
+    context 'when the upload exists' do
+      let(:uploader) { described_class.new(upload.id, message) }
+
+      before do
+        expect(Upload).to receive(:find_by).with(id: upload.id).and_return(upload)
+      end
+
+      context 'when the upload has a file' do
+        before do
+          FileUtils.mkdir_p(File.dirname(upload.absolute_path))
+          FileUtils.touch(upload.absolute_path) unless File.exist?(upload.absolute_path)
+        end
+
+        context 'when the message parameters match the upload' do
+          let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+
+          it 'returns the file in a success hash' do
+            expect(subject).to include(code: :ok, message: 'Success')
+            expect(subject[:file].file).to eq(upload.absolute_path)
+          end
+        end
+
+        context 'when the message id does not match the upload model_id' do
+          let(:message) { { id: 10000, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+
+          it 'returns an error hash' do
+            expect(subject).to include(code: :not_found, message: "Upload not found")
+          end
+        end
+
+        context 'when the message type does not match the upload model_type' do
+          let(:message) { { id: upload.model_id, type: 'bad_type', checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+
+          it 'returns an error hash' do
+            expect(subject).to include(code: :not_found, message: "Upload not found")
+          end
+        end
+
+        context 'when the message checksum does not match the upload checksum' do
+          let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'doesnotmatch' } }
+
+          it 'returns an error hash' do
+            expect(subject).to include(code: :not_found, message: "Upload not found")
+          end
+        end
+      end
+
+      context 'when the upload does not have a file' do
+        let(:message) { { id: upload.model_id, type: upload.model_type, checksum: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' } }
+
+        it 'returns an error hash' do
+          expect(subject).to include(code: :not_found, geo_code: 'FILE_NOT_FOUND', message: match(/Upload #\d+ file not found/))
+        end
+      end
+    end
+
+    context 'when the upload does not exist' do
+      it 'returns an error hash' do
+        result = described_class.new(10000, {}).execute
+
+        expect(result).to eq(code: :not_found, message: "Upload not found")
+      end
+    end
+  end
+
+  describe '#execute' do
+    context 'user avatar' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, model: build(:user)) }
+      end
+    end
+
+    context 'group avatar' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, model: build(:group)) }
+      end
+    end
+
+    context 'project avatar' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, model: build(:project)) }
+      end
+    end
+
+    context 'with an attachment' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, :attachment_upload) }
+      end
+    end
+
+    context 'with a snippet' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, :personal_snippet_upload) }
+      end
+    end
+
+    context 'with file upload' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, :issuable_upload) }
+      end
+    end
+
+    context 'with namespace file upload' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, :namespace_upload) }
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/job_artifact_uploader_spec.rb

@@ -2,7 +2,8 @@ require 'spec_helper'
 
 describe Gitlab::Geo::JobArtifactUploader, :geo do
   context '#execute' do
-    subject { described_class.new(job_artifact.id, {}).execute }
+    let(:uploader) { described_class.new(job_artifact.id, {}) }
+    subject { uploader.execute }
 
     context 'when the job artifact exists' do
       before do
@@ -29,7 +30,13 @@ describe Gitlab::Geo::JobArtifactUploader, :geo do
         let(:job_artifact) { create(:ci_job_artifact) }
 
         it 'returns an error hash' do
-          expect(subject).to eq(code: :not_found, message: "Job artifact does not have a file")
+          expect(subject).to include(code: :not_found, geo_code: 'FILE_NOT_FOUND', message: match(/JobArtifact #\d+ file not found/))
+        end
+
+        it 'logs the missing file' do
+          expect(uploader).to receive(:log_error).with("Could not upload job artifact because it does not have a file", id: job_artifact.id)
+
+          subject
         end
       end
     end

ee/spec/lib/gitlab/geo/lfs_uploader_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Geo::LfsUploader, :geo do
+  context '#execute' do
+    subject { uploader.execute }
+
+    context 'when the LFS object exists' do
+      let(:uploader) { described_class.new(lfs_object.id, message) }
+
+      before do
+        expect(LfsObject).to receive(:find_by).with(id: lfs_object.id).and_return(lfs_object)
+      end
+
+      context 'when the LFS object has a file' do
+        let(:lfs_object) { create(:lfs_object, :with_file) }
+        let(:message) { { checksum: lfs_object.oid } }
+
+        context 'when the message checksum matches the LFS object oid' do
+          it 'returns the file in a success hash' do
+            expect(subject).to eq(code: :ok, message: 'Success', file: lfs_object.file)
+          end
+        end
+
+        context 'when the message checksum does not match the LFS object oid' do
+          let(:message) { { checksum: 'foo' } }
+
+          it 'returns an error hash' do
+            expect(subject).to include(code: :not_found, message: "LFS object not found")
+          end
+        end
+      end
+
+      context 'when the LFS object does not have a file' do
+        let(:lfs_object) { create(:lfs_object) }
+        let(:message) { { checksum: lfs_object.oid } }
+
+        it 'returns an error hash' do
+          expect(subject).to include(code: :not_found, geo_code: 'FILE_NOT_FOUND', message: match(/LfsObject #\d+ file not found/))
+        end
+
+        it 'logs the missing file' do
+          expect(uploader).to receive(:log_error).with("Could not upload LFS object because it does not have a file", id: lfs_object.id)
+
+          subject
+        end
+      end
+    end
+
+    context 'when the LFS object does not exist' do
+      let(:uploader) { described_class.new(10000, {}) }
+
+      it 'returns an error hash' do
+        expect(subject).to eq(code: :not_found, message: 'LFS object not found')
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/transfer_spec.rb

@@ -23,36 +23,68 @@ describe Gitlab::Geo::Transfer do
       stub_current_geo_node(secondary_node)
     end
 
-    it 'when the destination filename is a directory' do
-      transfer = described_class.new(:lfs, lfs_object.id, '/tmp', { sha256: lfs_object.id })
+    context 'when the destination filename is a directory' do
+      it 'returns a failed result' do
+        transfer = described_class.new(:lfs, lfs_object.id, '/tmp', { sha256: lfs_object.id })
 
-      expect(transfer.download_from_primary).to eq(nil)
+        result = transfer.download_from_primary
+
+        expect_result(result, success: false, bytes_downloaded: 0, primary_missing_file: false)
+      end
     end
 
-    it 'when the HTTP response is successful' do
-      expect(FileUtils).to receive(:mv).with(anything, lfs_object.file.path).and_call_original
-      response = double(success?: true)
-      expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
+    context 'when the HTTP response is successful' do
+      it 'returns a successful result' do
+        expect(FileUtils).to receive(:mv).with(anything, lfs_object.file.path).and_call_original
+        response = double(:response, success?: true)
+        expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
 
-      expect(subject.download_from_primary).to eq(size)
-      stat = File.stat(lfs_object.file.path)
-      expect(stat.size).to eq(size)
-      expect(stat.mode & 0777).to eq(0666 - File.umask)
-      expect(File.binread(lfs_object.file.path)).to eq(content)
+        result = subject.download_from_primary
+
+        expect_result(result, success: true, bytes_downloaded: size, primary_missing_file: false)
+        stat = File.stat(lfs_object.file.path)
+        expect(stat.size).to eq(size)
+        expect(stat.mode & 0777).to eq(0666 - File.umask)
+        expect(File.binread(lfs_object.file.path)).to eq(content)
+      end
     end
 
-    it 'when the HTTP response is unsuccessful' do
-      expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
-      response = double(success?: false, code: 404, msg: 'No such file')
-      expect(Gitlab::HTTP).to receive(:get).and_return(response)
+    context 'when the HTTP response is unsuccessful' do
+      context 'when the HTTP response indicates a missing file on the primary' do
+        it 'returns a failed result indicating primary_missing_file' do
+          expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
+          response = double(:response, success?: false, code: 404, msg: "No such file")
+          expect(File).to receive(:read).and_return("{\"geo_code\":\"#{Gitlab::Geo::FileUploader::FILE_NOT_FOUND_GEO_CODE}\"}")
+          expect(Gitlab::HTTP).to receive(:get).and_return(response)
+
+          result = subject.download_from_primary
+
+          expect_result(result, success: false, bytes_downloaded: 0, primary_missing_file: true)
+        end
+      end
+
+      context 'when the HTTP response does not indicate a missing file on the primary' do
+        it 'returns a failed result' do
+          expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
+          response = double(:response, success?: false, code: 404, msg: 'No such file')
+          expect(Gitlab::HTTP).to receive(:get).and_return(response)
 
-      expect(subject.download_from_primary).to eq(-1)
+          result = subject.download_from_primary
+
+          expect_result(result, success: false, bytes_downloaded: 0)
+        end
+      end
     end
 
-    it 'when Tempfile fails' do
-      expect(Tempfile).to receive(:new).and_raise(Errno::ENAMETOOLONG)
+    context 'when Tempfile fails' do
+      it 'returns a failed result' do
+        expect(Tempfile).to receive(:new).and_raise(Errno::ENAMETOOLONG)
 
-      expect(subject.download_from_primary).to eq(nil)
+        result = subject.download_from_primary
+
+        expect(result.success).to eq(false)
+        expect(result.bytes_downloaded).to eq(0)
+      end
     end
 
     context "invalid path" do
@@ -62,8 +94,17 @@ describe Gitlab::Geo::Transfer do
         allow(FileUtils).to receive(:mkdir_p) { raise Errno::EEXIST }
 
         expect(subject).to receive(:log_error).with("unable to create directory /foo: File exists")
-        expect(subject.download_from_primary).to be_nil
+        result = subject.download_from_primary
+
+        expect(result.success).to eq(false)
+        expect(result.bytes_downloaded).to eq(0)
       end
     end
   end
+
+  def expect_result(result, success:, bytes_downloaded:, primary_missing_file: nil)
+    expect(result.success).to eq(success)
+    expect(result.bytes_downloaded).to eq(bytes_downloaded)
+    expect(result.primary_missing_file).to eq(primary_missing_file)
+  end
 end

ee/spec/requests/api/geo_spec.rb

@@ -103,17 +103,30 @@ describe API::Geo do
       expect(response).to have_gitlab_http_status(401)
     end
 
-    context 'file file exists' do
-      it 'responds with 200 with X-Sendfile' do
-        get api("/geo/transfers/file/#{upload.id}"), nil, req_header
+    context 'when the Upload record exists' do
+      context 'when the file exists' do
+        it 'responds with 200 with X-Sendfile' do
+          get api("/geo/transfers/file/#{upload.id}"), nil, req_header
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(response.headers['Content-Type']).to eq('application/octet-stream')
+          expect(response.headers['X-Sendfile']).to end_with('dk.png')
+        end
+      end
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.headers['Content-Type']).to eq('application/octet-stream')
-        expect(response.headers['X-Sendfile']).to end_with('dk.png')
+      context 'file does not exist' do
+        it 'responds with 404 and a specific geo code' do
+          File.unlink(upload.absolute_path)
+
+          get api("/geo/transfers/file/#{upload.id}"), nil, req_header
+
+          expect(response).to have_gitlab_http_status(404)
+          expect(json_response['geo_code']).to eq(Gitlab::Geo::FileUploader::FILE_NOT_FOUND_GEO_CODE)
+        end
       end
     end
 
-    context 'file does not exist' do
+    context 'when the Upload record does not exist' do
       it 'responds with 404' do
         get api("/geo/transfers/file/100000"), nil, req_header
 
@@ -139,13 +152,26 @@ describe API::Geo do
       expect(response).to have_gitlab_http_status(401)
     end
 
-    context 'LFS file exists' do
-      it 'responds with 200 with X-Sendfile' do
-        get api("/geo/transfers/lfs/#{lfs_object.id}"), nil, req_header
+    context 'LFS object exists' do
+      context 'file exists' do
+        it 'responds with 200 with X-Sendfile' do
+          get api("/geo/transfers/lfs/#{lfs_object.id}"), nil, req_header
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.headers['Content-Type']).to eq('application/octet-stream')
-        expect(response.headers['X-Sendfile']).to eq(lfs_object.file.path)
+          expect(response).to have_gitlab_http_status(200)
+          expect(response.headers['Content-Type']).to eq('application/octet-stream')
+          expect(response.headers['X-Sendfile']).to eq(lfs_object.file.path)
+        end
+      end
+
+      context 'file does not exist' do
+        it 'responds with 404 and a specific geo code' do
+          File.unlink(lfs_object.file.path)
+
+          get api("/geo/transfers/lfs/#{lfs_object.id}"), nil, req_header
+
+          expect(response).to have_gitlab_http_status(404)
+          expect(json_response['geo_code']).to eq(Gitlab::Geo::FileUploader::FILE_NOT_FOUND_GEO_CODE)
+        end
       end
     end