Merge branch 'id-link-existing-lfs' into 'master'

Verify only 1mb of existing LFS object on download

See merge request gitlab-org/gitlab!41770

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -6,6 +6,9 @@ module Projects
     class LfsDownloadService < BaseService
       SizeError = Class.new(StandardError)
       OidError = Class.new(StandardError)
+      ResponseError = Class.new(StandardError)
+
+      LARGE_FILE_SIZE = 1.megabytes
 
       attr_reader :lfs_download_object
       delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs
@@ -19,6 +22,7 @@ module Projects
       def execute
         return unless project&.lfs_enabled? && lfs_download_object
         return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid?
+        return link_existing_lfs_object! if Feature.enabled?(:lfs_link_existing_object, project, default_enabled: true) && lfs_size > LARGE_FILE_SIZE && lfs_object
 
         wrap_download_errors do
           download_lfs_file!
@@ -29,7 +33,7 @@ module Projects
 
       def wrap_download_errors(&block)
         yield
-      rescue SizeError, OidError, StandardError => e
+      rescue SizeError, OidError, ResponseError, StandardError => e
         error("LFS file with oid #{lfs_oid} could't be downloaded from #{lfs_sanitized_url}: #{e.message}")
       end
 
@@ -56,15 +60,13 @@ module Projects
 
       def download_and_save_file!(file)
         digester = Digest::SHA256.new
-        response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment|
+        fetch_file do |fragment|
           digester << fragment
           file.write(fragment)
 
           raise_size_error! if file.size > lfs_size
         end
 
-        raise StandardError, "Received error code #{response.code}" unless response.success?
-
         raise_size_error! if file.size != lfs_size
         raise_oid_error! if digester.hexdigest != lfs_oid
       end
@@ -78,6 +80,12 @@ module Projects
         end
       end
 
+      def fetch_file(&block)
+        response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers, &block)
+
+        raise ResponseError, "Received error code #{response.code}" unless response.success?
+      end
+
       def with_tmp_file
         create_tmp_storage_dir
 
@@ -123,6 +131,29 @@ module Projects
 
         super
       end
+
+      def lfs_object
+        @lfs_object ||= LfsObject.find_by_oid(lfs_oid)
+      end
+
+      def link_existing_lfs_object!
+        existing_file = lfs_object.file.open
+        buffer_size = 0
+        result = fetch_file do |fragment|
+          unless fragment == existing_file.read(fragment.size)
+            break error("LFS file with oid #{lfs_oid} cannot be linked with an existing LFS object")
+          end
+
+          buffer_size += fragment.size
+          break success if buffer_size > LARGE_FILE_SIZE
+        end
+
+        project.lfs_objects << lfs_object
+
+        result
+      ensure
+        existing_file&.close
+      end
     end
   end
 end

changelogs/unreleased/id-link-existing-lfs.yml

+---
+title: Verify only 1mb of existing LFS object to improve LfsDownloadService performance
+merge_request: 41770
+author:
+type: performance

config/feature_flags/development/lfs_link_existing_object.yml

+---
+name: lfs_link_existing_object
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/41770
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/249246
+group: group::source code
+type: development
+default_enabled: true

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -4,7 +4,8 @@ require 'spec_helper'
 RSpec.describe Projects::LfsPointers::LfsDownloadService do
   include StubRequests
 
-  let(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
+
   let(:lfs_content) { SecureRandom.random_bytes(10) }
   let(:oid) { Digest::SHA256.hexdigest(lfs_content) }
   let(:download_link) { "http://gitlab.com/#{oid}" }
@@ -14,9 +15,11 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do
 
   subject { described_class.new(project, lfs_object) }
 
-  before do
+  before_all do
     ApplicationSetting.create_from_defaults
+  end
 
+  before do
     stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting)
     allow(project).to receive(:lfs_enabled?).and_return(true)
   end
@@ -226,5 +229,55 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do
         subject.execute
       end
     end
+
+    context 'when a large lfs object with the same oid already exists' do
+      let!(:existing_lfs_object) { create(:lfs_object, :with_file, :correct_oid) }
+
+      before do
+        stub_const("#{described_class}::LARGE_FILE_SIZE", 500)
+        stub_full_request(download_link).to_return(body: lfs_content)
+      end
+
+      context 'and first fragments are the same' do
+        let(:lfs_content) { existing_lfs_object.file.read }
+
+        context 'when lfs_link_existing_object feature flag disabled' do
+          before do
+            stub_feature_flags(lfs_link_existing_object: false)
+          end
+
+          it 'does not call link_existing_lfs_object!' do
+            expect(subject).not_to receive(:link_existing_lfs_object!)
+
+            subject.execute
+          end
+        end
+
+        it 'returns success' do
+          expect(subject.execute).to eq({ status: :success })
+        end
+
+        it 'links existing lfs object to the project' do
+          expect { subject.execute }
+            .to change { project.lfs_objects.include?(existing_lfs_object) }.from(false).to(true)
+        end
+      end
+
+      context 'and first fragments diverges' do
+        let(:lfs_content) { SecureRandom.random_bytes(1000) }
+        let(:oid) { existing_lfs_object.oid }
+
+        it 'raises oid mismatch error' do
+          expect(subject.execute).to eq({
+            status: :error,
+            message: "LFS file with oid #{oid} cannot be linked with an existing LFS object"
+          })
+        end
+
+        it 'does not change lfs objects' do
+          expect { subject.execute }.not_to change { project.lfs_objects }
+        end
+      end
+    end
   end
 end