Merge branch 'remove-tempfile-unlink-env-var' into 'master'

Remove GITLAB_TEMPFILE_IMMEDIATE_UNLINK env var

See merge request gitlab-org/gitlab!60110

config/initializers/puma_client_tempfile_patch.rb

@@ -3,99 +3,97 @@
 if Gitlab::Runtime.puma?
   raise "Remove this monkey patch: #{__FILE__}" unless Puma::Const::VERSION == '5.1.1'
 
-  if ENV['GITLAB_TEMPFILE_IMMEDIATE_UNLINK'] == '1'
-    # This is copied from https://github.com/puma/puma/blob/v5.1.1/lib/puma/client.rb,
-    # with two additions: both times we create a temporary file, we immediately
-    # call `#unlink`. This means that if the process gets terminated without being
-    # able to clean up itself, the temporary file will not linger on the file
-    # system. We will try to get this patch accepted upstream if it works for us
-    # (we just need to check if the temporary file responds to `#unlink` as that
-    # won't work on Windows, for instance).
-    module Puma
-      class Client
-        private
-
-        def setup_body
-          @body_read_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond)
-
-          if @env[HTTP_EXPECT] == CONTINUE
-            # TODO allow a hook here to check the headers before
-            # going forward
-            @io << HTTP_11_100
-            @io.flush
-          end
+  # This is copied from https://github.com/puma/puma/blob/v5.1.1/lib/puma/client.rb,
+  # with two additions: both times we create a temporary file, we immediately
+  # call `#unlink`. This means that if the process gets terminated without being
+  # able to clean up itself, the temporary file will not linger on the file
+  # system. We will try to get this patch accepted upstream if it works for us
+  # (we just need to check if the temporary file responds to `#unlink` as that
+  # won't work on Windows, for instance).
+  module Puma
+    class Client
+      private
+
+      def setup_body
+        @body_read_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond)
+
+        if @env[HTTP_EXPECT] == CONTINUE
+          # TODO allow a hook here to check the headers before
+          # going forward
+          @io << HTTP_11_100
+          @io.flush
+        end
 
-          @read_header = false
+        @read_header = false
 
-          body = @parser.body
+        body = @parser.body
 
-          te = @env[TRANSFER_ENCODING2]
+        te = @env[TRANSFER_ENCODING2]
 
-          if te
-            if te.include?(",")
-              te.split(",").each do |part|
-                if CHUNKED.casecmp(part.strip) == 0 # rubocop:disable Metrics/BlockNesting
-                  return setup_chunked_body(body)
-                end
+        if te
+          if te.include?(",")
+            te.split(",").each do |part|
+              if CHUNKED.casecmp(part.strip) == 0 # rubocop:disable Metrics/BlockNesting
+                return setup_chunked_body(body)
               end
-            elsif CHUNKED.casecmp(te) == 0
-              return setup_chunked_body(body)
             end
+          elsif CHUNKED.casecmp(te) == 0
+            return setup_chunked_body(body)
           end
+        end
 
-          @chunked_body = false
-
-          cl = @env[CONTENT_LENGTH]
-
-          unless cl
-            @buffer = body.empty? ? nil : body
-            @body = EmptyBody
-            set_ready
-            return true
-          end
-
-          remain = cl.to_i - body.bytesize
+        @chunked_body = false
 
-          if remain <= 0
-            @body = StringIO.new(body)
-            @buffer = nil
-            set_ready
-            return true
-          end
-
-          if remain > MAX_BODY
-            @body = Tempfile.new(Const::PUMA_TMP_BASE)
-            @body.binmode
-            @body.unlink # This is the changed part
-            @tempfile = @body
-          else
-            # The body[0,0] trick is to get an empty string in the same
-            # encoding as body.
-            @body = StringIO.new body[0,0] # rubocop:disable Layout/SpaceAfterComma
-          end
+        cl = @env[CONTENT_LENGTH]
 
-          @body.write body
+        unless cl
+          @buffer = body.empty? ? nil : body
+          @body = EmptyBody
+          set_ready
+          return true
+        end
 
-          @body_remain = remain
+        remain = cl.to_i - body.bytesize
 
-          return false # rubocop:disable Style/RedundantReturn
+        if remain <= 0
+          @body = StringIO.new(body)
+          @buffer = nil
+          set_ready
+          return true
         end
 
-        def setup_chunked_body(body)
-          @chunked_body = true
-          @partial_part_left = 0
-          @prev_chunk = ""
-
+        if remain > MAX_BODY
           @body = Tempfile.new(Const::PUMA_TMP_BASE)
           @body.binmode
           @body.unlink # This is the changed part
           @tempfile = @body
-          @chunked_content_length = 0
+        else
+          # The body[0,0] trick is to get an empty string in the same
+          # encoding as body.
+          @body = StringIO.new body[0,0] # rubocop:disable Layout/SpaceAfterComma
+        end
 
-          if decode_chunk(body)
-            @env[CONTENT_LENGTH] = @chunked_content_length
-            return true # rubocop:disable Style/RedundantReturn
-          end
+        @body.write body
+
+        @body_remain = remain
+
+        return false # rubocop:disable Style/RedundantReturn
+      end
+
+      def setup_chunked_body(body)
+        @chunked_body = true
+        @partial_part_left = 0
+        @prev_chunk = ""
+
+        @body = Tempfile.new(Const::PUMA_TMP_BASE)
+        @body.binmode
+        @body.unlink # This is the changed part
+        @tempfile = @body
+        @chunked_content_length = 0
+
+        if decode_chunk(body)
+          @env[CONTENT_LENGTH] = @chunked_content_length
+          return true # rubocop:disable Style/RedundantReturn
         end
       end
     end

lib/gitlab/middleware/rack_multipart_tempfile_factory.rb

@@ -14,9 +14,7 @@ module Gitlab
       end
 
       def call(env)
-        if ENV['GITLAB_TEMPFILE_IMMEDIATE_UNLINK'] == '1'
-          env[Rack::RACK_MULTIPART_TEMPFILE_FACTORY] = FACTORY
-        end
+        env[Rack::RACK_MULTIPART_TEMPFILE_FACTORY] = FACTORY
 
         @app.call(env)
       end

spec/lib/gitlab/middleware/rack_multipart_tempfile_factory_spec.rb

@@ -42,44 +42,20 @@ RSpec.describe Gitlab::Middleware::RackMultipartTempfileFactory do
   context 'for a multipart request' do
     let(:env) { Rack::MockRequest.env_for('/', multipart_fixture) }
 
-    context 'when the environment variable is enabled' do
-      before do
-        stub_env('GITLAB_TEMPFILE_IMMEDIATE_UNLINK', '1')
-      end
-
-      it 'immediately unlinks the temporary file' do
-        tempfile = Tempfile.new('foo')
-
-        expect(tempfile.path).not_to be(nil)
-        expect(Rack::Multipart::Parser::TEMPFILE_FACTORY).to receive(:call).and_return(tempfile)
-        expect(tempfile).to receive(:unlink).and_call_original
+    it 'immediately unlinks the temporary file' do
+      tempfile = Tempfile.new('foo')
 
-        subject.call(env)
+      expect(tempfile.path).not_to be(nil)
+      expect(Rack::Multipart::Parser::TEMPFILE_FACTORY).to receive(:call).and_return(tempfile)
+      expect(tempfile).to receive(:unlink).and_call_original
 
-        expect(tempfile.path).to be(nil)
-      end
+      subject.call(env)
 
-      it 'processes the request as normal' do
-        expect(subject.call(env)).to eq([200, { 'Content-Type' => 'image/jpeg' }, [file_contents]])
-      end
+      expect(tempfile.path).to be(nil)
     end
 
-    context 'when the environment variable is disabled' do
-      it 'does not immediately unlink the temporary file' do
-        tempfile = Tempfile.new('foo')
-
-        expect(tempfile.path).not_to be(nil)
-        expect(Rack::Multipart::Parser::TEMPFILE_FACTORY).to receive(:call).and_return(tempfile)
-        expect(tempfile).not_to receive(:unlink).and_call_original
-
-        subject.call(env)
-
-        expect(tempfile.path).not_to be(nil)
-      end
-
-      it 'processes the request as normal' do
-        expect(subject.call(env)).to eq([200, { 'Content-Type' => 'image/jpeg' }, [file_contents]])
-      end
+    it 'processes the request as normal' do
+      expect(subject.call(env)).to eq([200, { 'Content-Type' => 'image/jpeg' }, [file_contents]])
     end
   end
 

spec/requests/api/projects_spec.rb

@@ -1634,7 +1634,6 @@ RSpec.describe API::Projects do
     end
 
     it "does not leave the temporary file in place after uploading, even when the tempfile reaper does not run" do
-      stub_env('GITLAB_TEMPFILE_IMMEDIATE_UNLINK', '1')
       tempfile = Tempfile.new('foo')
       path = tempfile.path