Fix triggering multiple children pipeline with the same artifact

When two jobs try to read the same artifact at the same time,
exclusive lock raises error.
With this change, it will retry for 2.5 seconds.

app/uploaders/object_storage.rb

@@ -210,6 +210,20 @@ module ObjectStorage
       end
     end
 
+    class OpenFile
+      extend Forwardable
+
+      # Explicitly exclude :path, because rubyzip uses that to detect "real" files.
+      def_delegators :@file, *(Zip::File::IO_METHODS - [:path])
+
+      # Even though :size is not in IO_METHODS, we do need it.
+      def_delegators :@file, :size
+
+      def initialize(file)
+        @file = file
+      end
+    end
+
     # allow to configure and overwrite the filename
     def filename
       @filename || super || file&.filename # rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -259,6 +273,24 @@ module ObjectStorage
       end
     end
 
+    def use_open_file(&blk)
+      Tempfile.open(path) do |file|
+        file.unlink
+        file.binmode
+
+        if file_storage?
+          IO.copy_stream(path, file)
+        else
+          streamer = lambda { |chunk, _, _| file.write(chunk) }
+          Excon.get(url, response_block: streamer)
+        end
+
+        file.seek(0, IO::SEEK_SET)
+
+        yield OpenFile.new(file)
+      end
+    end
+
     #
     # Move the file to another store
     #

config/feature_flags/development/ci_new_artifact_file_reader.yml

+---
+name: ci_new_artifact_file_reader
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40268
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/249588
+group: group::pipeline authoring
+type: development
+default_enabled: false

lib/gitlab/ci/artifact_file_reader.rb

@@ -45,6 +45,31 @@ module Gitlab
       end
 
       def read_zip_file!(file_path)
+        if ::Gitlab::Ci::Features.new_artifact_file_reader_enabled?(job.project)
+          read_with_new_artifact_file_reader(file_path)
+        else
+          read_with_legacy_artifact_file_reader(file_path)
+        end
+      end
+
+      def read_with_new_artifact_file_reader(file_path)
+        job.artifacts_file.use_open_file do |file|
+          zip_file = Zip::File.new(file, false, true)
+          entry = zip_file.find_entry(file_path)
+
+          unless entry
+            raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!"
+          end
+
+          if entry.name_is_directory?
+            raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!"
+          end
+
+          zip_file.read(entry)
+        end
+      end
+
+      def read_with_legacy_artifact_file_reader(file_path)
         job.artifacts_file.use_file do |archive_path|
           Zip::File.open(archive_path) do |zip_file|
             entry = zip_file.find_entry(file_path)

lib/gitlab/ci/features.rb

@@ -78,6 +78,10 @@ module Gitlab
         ::Feature.enabled?(:ci_enable_live_trace, project) &&
           ::Feature.enabled?(:ci_accept_trace, project, type: :ops, default_enabled: false)
       end
+
+      def self.new_artifact_file_reader_enabled?(project)
+        ::Feature.enabled?(:ci_new_artifact_file_reader, project, default_enabled: false)
+      end
     end
   end
 end

spec/lib/gitlab/ci/artifact_file_reader_spec.rb

@@ -18,6 +18,17 @@ RSpec.describe Gitlab::Ci::ArtifactFileReader do
         expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
       end
 
+      context 'when FF ci_new_artifact_file_reader is disabled' do
+        before do
+          stub_feature_flags(ci_new_artifact_file_reader: false)
+        end
+
+        it 'returns the content at the path' do
+          is_expected.to be_present
+          expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
+        end
+      end
+
       context 'when path does not exist' do
         let(:path) { 'file/does/not/exist.txt' }
         let(:expected_error) do

spec/uploaders/object_storage_spec.rb

@@ -210,6 +210,27 @@ RSpec.describe ObjectStorage do
         end
       end
 
+      describe '#use_open_file' do
+        context 'when file is stored locally' do
+          it "returns the file" do
+            expect { |b| uploader.use_open_file(&b) }.to yield_with_args(an_instance_of(ObjectStorage::Concern::OpenFile))
+          end
+        end
+
+        context 'when file is stored remotely' do
+          let(:store) { described_class::Store::REMOTE }
+
+          before do
+            stub_artifacts_object_storage
+            stub_request(:get, %r{s3.amazonaws.com/#{uploader.path}}).to_return(status: 200, body: '')
+          end
+
+          it "returns the file" do
+            expect { |b| uploader.use_open_file(&b) }.to yield_with_args(an_instance_of(ObjectStorage::Concern::OpenFile))
+          end
+        end
+      end
+
       describe '#migrate!' do
         subject { uploader.migrate!(new_store) }
 
@@ -844,4 +865,19 @@ RSpec.describe ObjectStorage do
       end
     end
   end
+
+  describe 'OpenFile' do
+    subject { ObjectStorage::Concern::OpenFile.new(file) }
+
+    let(:file) { double(read: true, size: true, path: true) }
+
+    it 'delegates read and size methods' do
+      expect(subject.read).to eq(true)
+      expect(subject.size).to eq(true)
+    end
+
+    it 'does not delegate path method' do
+      expect { subject.path }.to raise_error(NoMethodError)
+    end
+  end
 end