[anyingiit]

Requested by winshyon

Description

Multipart uploads parsed by Rack::Multipart create tempfiles that are only cleaned up when something runs Rack::TempfileReaper. Rails includes it by default, but a Grape API served directly by Rack (or by any non-Rails host) left those tempfiles on disk until GC/process exit, as described in #2487.

This adds Rack::TempfileReaper as the outermost middleware in Grape::Endpoint#build_stack (before Rack::Head and Grape::Middleware::Error), so the files are closed and unlinked once the response body is closed. Because it sits outside the error middleware, files are also reaped when the endpoint ends with error!/a rescued exception, and the reaper itself closes them if an exception escapes.

Notes for reviewers:

• Mounted inside Rails (or another app that already uses the reaper): the middleware is idempotent — it does env['rack.tempfiles'] ||= [] and both reapers just close! the same tempfiles (Tempfile#close! on an already closed/unlinked file is a no-op). I checked this with an outer Rack::TempfileReaper wrapping a Grape API on Rack 3.2.7 and Rack 2.2.24: 201 response, tempfile unlinked, no error.
• The cost is one extra Rack::BodyProxy per request on the endpoint stack. I put it in the endpoint stack (rather than the API instance) to mirror Rack::Head, which already lives there; happy to move it if you prefer it elsewhere.
• The spec lives next to the existing Tempfile upload spec in spec/grape/integration/rack_spec.rb.

Motivation / disclosure: I had some spare AI-assistant quota (Claude Code) and am using it to try to
help projects with open good-first-issues. The change was prepared with Claude Code and verified as listed
below. If it doesn't fit, isn't up to your bar, or you'd simply rather not take it — please feel free to
close it, no hard feelings at all 🙂

Related issue

Closes #2487

Checklist

• Tests pass locally (bundle exec rake: RuboCop 358 files, no offenses; RSpec 3155 examples, 0 failures on current master. Before rebasing, also rspec spec --exclude-pattern=... with gemfiles/rack_2_2.gemfile (3079 examples, 0 failures) and gemfiles/rails_8_1.gemfile (3077 examples, 0 failures). The 2 new examples in spec/grape/integration/rack_spec.rb fail on master without the change on both Rack 3.2 and Rack 2.2)
• CHANGELOG.md is updated (if applicable) — one line under 4.1.0 Features
• Documentation is updated (if applicable) — n/a (no documented behavior changes; no UPGRADING entry since it is not a contract break)

🤖 Generated with Claude Code

[github-actions]

Danger Report

No issues found.

View run

[dblock]

We support multipart uploads by default, don’t we? If I take Grape "as is" right now with an API that doesn’t care about multiple downloads and just send it a billion multipart uploads, will it not cleanup those temp files? If so I think we need this on by default, otherwise it should just be documented.

Answer: standalone Grape supports multipart uploads but currently does not guarantee cleanup of upload tempfiles when the response closes. When request handling triggers multipart parsing, Rack creates tempfiles; without Rack::TempfileReaper, their cleanup relies on garbage collection/finalizers rather than response completion. This does not require the endpoint to explicitly declare an upload parameter, although merely sending a multipart request does not necessarily cause parsing. Rails already includes the reaper. Therefore, cleanup should be enabled by default in Grape rather than only documented as an opt-in.

— AI posting on behalf of @dblock.

[dblock]

I fixed danger-changelog in 0.10.1, dblock/danger-changelog#85

[ericproulx]

Thanks for picking this up! I looked into where the reaper has to sit, and the endpoint stack turns out to be the wrong level.

Within the endpoint stack, your position is right. Before or after Rack::Head behaves the same, but it has to stay outside Grape::Middleware::Error and Grape::Middleware::Formatter:

• inside Error, a throw :error (user code, the formatter's 400/415/406, the versioner's 404/406) unwinds through catch(:error) past the reaper's rescue, and the rendered error never goes through its BodyProxy, so the tempfile leaks;
• below Formatter, even a plain 201 leaks: format_response builds a new body from the endpoint's and never closes the original.

But being in the endpoint stack breaks cascading. When a route cascades, Router#close_body closes the declining endpoint's body, which fires that endpoint's reaper mid-request. Rack caches the parsed form in env, so the next route gets the same, now closed, tempfile:

declines = Class.new(Grape::API) do
  post('upload') do
    params[:file][:tempfile]
    error!('not mine', 404, 'X-Cascade' => 'pass')
  end
end
answers = Class.new(Grape::API) do
  post('upload') { params[:file][:tempfile].read }
end
app = Class.new(Grape::API) do
  mount declines
  mount answers
end

post '/upload', file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain')
# master:      201, the file's content
# this branch: IOError: closed stream

The same applies to the ANY (*) route, which closes the previous response too, and it happens regardless of an outer Rails reaper.

Suggestion: wrap the router once per API instead, in Grape::API::Instance#initialize next to the lint? wrapping (@app = Rack::TempfileReaper.new(@app)). That gives one reaper per request however many routes are tried. I checked that placement (as a reaper around the whole API) against the success, error!, throw :error, rescue_from, validation and cascade cases on Rack 3.2.7 and 2.2.24, and it reaps all of them without the IOError. Your specs only exercise a single endpoint, so here are two for the cases above.

Specs. These go inside your describe 'multipart tempfiles' block and reuse its uploaded_paths and upload. The second covers a Rack app mounted with mount: the router calls it directly (config.app || build_stack in Endpoint), so a reaper in the endpoint stack never runs for it either. #3042 moves Rack::Head around the router for the same reason.

context 'when the route that read the upload cascades' do
  let(:app) do
    paths = uploaded_paths
    declines = Class.new(Grape::API) do
      post 'upload' do
        paths << params[:file][:tempfile].path
        error!('not mine', 404, 'X-Cascade' => 'pass')
      end
    end
    answers = Class.new(Grape::API) do
      post('upload') { params[:file][:tempfile].read.bytesize.to_s }
    end
    Class.new(Grape::API) do
      mount declines
      mount answers
    end
  end

  it 'leaves the tempfile open for the next route and unlinks it once the response is closed' do
    post '/upload', file: upload
    expect(last_response.status).to eq(201)
    expect(last_response.body).to eq(File.size(__FILE__).to_s)
    expect(uploaded_paths.size).to eq(1)
    expect(File.exist?(uploaded_paths.first)).to be(false)
  end
end

context 'when a mounted Rack app reads the upload' do
  let(:app) do
    paths = uploaded_paths
    rack_app = lambda do |env|
      paths << Rack::Request.new(env).POST['file'][:tempfile].path
      [200, { 'content-type' => 'text/plain' }, ['ok']]
    end
    Class.new(Grape::API) do
      mount rack_app => '/rack'
    end
  end

  it 'unlinks the tempfile once the response is closed' do
    post '/rack', file: upload
    expect(last_response.body).to eq('ok')
    expect(uploaded_paths.size).to eq(1)
    expect(File.exist?(uploaded_paths.first)).to be(false)
  end
end

On this branch both fail: the cascade one with IOError: closed stream, and the mounted Rack app one with its tempfile still on disk. With the reaper moved out of build_stack and wrapped around the router in API::Instance#initialize (@app = Rack::TempfileReaper.new(@app) after the lint? line), all 8 examples in rack_spec.rb pass on Rack 3.2.7 and 2.2.24, and RuboCop is clean.

One correction for the description: "the reaper itself closes them if an exception escapes" only holds on Rack 3.x. Rack 2.2's TempfileReaper#call has no rescue branch, so an unrescued exception still leaks there (at any placement).