Skip to content

fix: remove partial disk file on aborted uploads - #1406

Open
raashish1601 wants to merge 2 commits into
expressjs:mainfrom
raashish1601:fix/remove-partial-disk-files-on-abort
Open

fix: remove partial disk file on aborted uploads#1406
raashish1601 wants to merge 2 commits into
expressjs:mainfrom
raashish1601:fix/remove-partial-disk-files-on-abort

Conversation

@raashish1601

Copy link
Copy Markdown
Contributor

Fixes partial disk file cleanup when abort happens after a file write started; also stabilizes fixture-size assertions and temp path checks in tests.

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this on Windows 11 (Node 24, default core.autocrlf=true clone — i.e., what a Windows contributor actually gets), both this branch and current main, because CI here has no Windows lane. Two findings, one of them probably not visible from a Linux checkout.

1. The product half of this PR appears superseded on main. 9c801c7 ("fix: clean up in-progress disk writes on abort") landed after this branch was cut and implements the same mechanism this PR adds — a pending-files list, cleanup of in-progress writes on abort, an eager file.path, and a file.stream.destroyed guard. The two edits collide in the same regions of lib/make-middleware.js and storage/disk.js, which is why this PR now shows as conflicting. I compared the diffs side by side rather than inferring from titles.

2. The test half is independently valuable, and the numbers show why. On a default Windows clone:

  • current main (4203ccd): 71 passing, 12 failing — every failure is the same shape, e.g. should process parser/form-data POST request: 1803 !== 1778
  • this branch (fc39d3b): 73 passing, 0 failing

The mechanism: the .dat fixtures have no .gitattributes entry, so autocrlf smudges them to CRLF at checkout (git ls-files --eol test/files/small0.dati/lf w/crlf, 1778 → 1803 bytes on disk), and main's tests assert hardcoded byte sizes (assert.strictEqual(req.file.size, 1778)). This PR replaces the hardcoded sizes with sizes measured from the on-disk fixture, which makes the suite pass on a Windows clone as a side effect — an unadvertised benefit that seems worth keeping regardless of what happens to the storage change.

Disclosure: I have an open PR (#1450) that fixes the same 12 failures at the git level by adding .gitattributes for the fixtures. The two approaches are complementary rather than competing — measured sizes keep the tests robust even where a clone's smudge state is unexpected — but the maintainers should know I'm not neutral on that corner.

Given both points, the highest-value path for this PR might be: rebase over 9c801c7, drop the now-duplicated storage logic, and keep the measured-size test changes (plus the new abort assertions in test/error-handling.js, which ran green here).

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