Skip to content

fix: don't abort uploads when file writes are in progress#1379

Closed
mario-duna wants to merge 2 commits intoexpressjs:mainfrom
mario-duna:fix/pending-writes-race-condition
Closed

fix: don't abort uploads when file writes are in progress#1379
mario-duna wants to merge 2 commits intoexpressjs:mainfrom
mario-duna:fix/pending-writes-race-condition

Conversation

@mario-duna
Copy link
Copy Markdown

Problem

Multer 2.1.0 introduced stricter request lifecycle handling (via the aborted and close event handlers) to prevent DoS from hung connections. However, this can cause legitimate uploads to fail with "Request aborted" or "Request closed" errors in certain environments.

When This Happens

This issue manifests when using multer with in-process HTTP testing tools like supertest. In these environments:

  1. There's no real network connection - the request and response are handled in the same process
  2. Event loop timing differs from real HTTP - events fire in a different order than with actual network requests
  3. The close or aborted event on the request can fire before req.readableEnded is set to true, even when the file data has been fully received and is being written to storage

The existing check if (req.readableEnded) return in the close handler is timing-dependent and doesn't protect against this race condition.

Reproduction

This is most reliably reproduced on ARM-based CI runners (e.g., GitHub Actions ARM64 runners, AWS Graviton) where event loop scheduling differs from x86. A typical test like this fails intermittently or consistently:

const request = require('supertest');
const express = require('express');
const multer = require('multer');

const app = express();
app.post('/upload', multer({ dest: 'uploads/' }).single('file'), (req, res) => {
  res.json({ filename: req.file.filename });
});

// This can fail with "Request aborted" even though the upload succeeded
request(app)
  .post('/upload')
  .attach('file', Buffer.from('test content'), 'test.txt')
  .expect(200);

Root Cause

In make-middleware.js, the request event handlers immediately trigger handleRequestFailure():

req.on('aborted', function () {
  handleRequestFailure(new Error('Request aborted'))
})

req.on('close', function () {
  if (req.readableEnded) return
  handleRequestFailure(new Error('Request closed'))
})

With supertest, the sequence can be:

  1. File data is fully piped to busboy
  2. storage._handleFile() is called and starts writing
  3. supertest considers the request "done" and the close/aborted event fires
  4. req.readableEnded may not be true yet (timing-dependent)
  5. handleRequestFailure() is called, which sets errorOccured = true and eventually deletes the uploaded file
  6. The storage engine finishes writing, but the upload is already marked as failed

Solution

Add a check for !pendingWrites.isZero() before aborting. If files are actively being written to storage, we should let them complete rather than aborting prematurely:

req.on('aborted', function () {
  if (!pendingWrites.isZero()) return  // Upload in progress, let it complete
  handleRequestFailure(new Error('Request aborted'))
})

req.on('close', function () {
  if (req.readableEnded) return
  if (!pendingWrites.isZero()) return  // Upload in progress, let it complete
  handleRequestFailure(new Error('Request closed'))
})

This is safe because:

  • If the write completes successfully → indicateDone() calls done() normally
  • If the write fails → the storage engine calls the error callback → abortWithError() handles it
  • If there are no pending writes and the request disconnects → we still abort (preserving the DoS protection from chore(ci): npm-publish via reusable workflows #1364)

Security Consideration

This change preserves the DoS protection added in the security fix (commit cccf0fe). The original issue was that clients could start uploads and disconnect, leaving multer waiting indefinitely. With this fix:

  • Requests with no pending writes that disconnect → still aborted (DoS protection intact)
  • Requests with active file writes that "disconnect" due to event timing → allowed to complete

@mario-duna
Copy link
Copy Markdown
Author

@UlisesGascon would this change make sense?

@ctcpip
Copy link
Copy Markdown
Member

ctcpip commented Mar 2, 2026

duplicate of #1373

@ctcpip ctcpip closed this Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants