Fix multipart file parts hanging when parser limits are exceeded mid-file - #7155
tarikermis wants to merge 2 commits into
Conversation
…file When a parser limit such as maxTotalSize is exceeded while a file part is in progress, the parser reports the error but never emits the part's terminating boundary. The file content stream only checked for that boundary, so consumers like Multipart.toPersisted waited forever. Propagate parser (and upstream) failures to in-progress file part streams so they fail instead of hanging, mirroring the upstream multipasta fix (tim-smart/multipasta#40). Also stop masking parser errors as InternalError in toPersisted's default file writer and in contentEffect, so limit violations surface with their actual reason (e.g. BodyTooLarge -> 413). Closes Effect-TS#6284
🦋 Changeset detectedLatest commit: 1e4026a The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One minor note inline — the fix itself is correct, well-tested, and I verified it empirically.
Reviewed changes
makeChannelin-progress part termination — parser errors and upstream stream failures are now routed to in-progress file part streams through a sharedfileExit, surfaced after buffered chunks drain; completed parts still end cleanly (finishedwins), and only the in-progress part fails. Mirrors the upstream multipasta fix (tim-smart/multipasta#40).- Error reason preservation —
FileImpl.contentEffectanddefaultWriteFileno longer blanket-wrap every failure asInternalError; onlyPlatformErroris wrapped, so limit errors keep their real reason (BodyTooLarge/FileTooLarge→ 413 instead of a masked 500). - Regression tests — three new cases (
maxTotalSizemid-file,maxFileSizemid-file, multi-file ordering showing a completed first part persists before the second fails).
Verification I ran: reverting Multipart.ts to main makes the new tests hang (killed by a 25 s shell timeout), confirming they genuinely reproduce the bug; with the fix, Multipart.test.ts (13), HttpServerRequest.test.ts + HttpEffect.test.ts (29), and @effect/platform-node MultipartParser.test.ts (84) all pass. I also traced for a scenario where parser.end()'s EndNotReached could overwrite the specific limit reason via the re-fired onError — it can't: once exit is set, the parts loop short-circuits before any further pump, so the EOF path never runs while an error is pending. The changeset is accurate and the @effect/platform-node path is a separate Node-stream implementation that pushes errors into the file Readable, so it has no hang.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Map the upstream cause's failure values to InternalError MultipartErrors (via Cause.map, preserving defects and interruptions) instead of routing the raw cause into fileExit, so File.content and File.contentEffect keep their declared MultipartError error type without a cast.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
fileExitupstream-error contract fix — in-progress file part streams now map upstream stream failures to anInternalErrorMultipartErrorviaCause.mapinstead of storing the raw cause, keepingFile.contentEffect's documentedEffect<Uint8Array, MultipartError>contract intact while parser limit reasons (BodyTooLarge/FileTooLarge) still pass through with their specific reason.- Binding regression test — a new case streams upload bytes from a
ReadableStreamthat errors mid-file and asserts the in-progress part'scontentEffectfails with anInternalErrorMultipartError. I verified it's genuinely binding: it fails with the rawError: boomon the pre-fix source and passes with the fix.
I ran the full Multipart.test.ts (14/14 pass), confirmed the new test fails on the pre-fix fileExit mapping, and typechecked packages/effect clean.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Addressed in another PR :) |
This branch had an error being deployed
2 participants
What & why
Multipart.toPersisted(andMultipart.persistedin@effect/platform-bun) hangs indefinitely when a multipart parser limit such asmaxTotalSizeis exceeded mid-file during an upload. The handler never completes and the request eventually times out without any response.Root cause: when the parser reports a limit violation, it drops the offending chunk without forwarding it to the boundary scanner, so the in-progress file part's terminating boundary is never seen and its content stream never closes. The file part's pull loop in
Multipart.makeChannelonly terminated on that boundary (finished), so it kept pumping forever (a busy loop after upstream EOF) without ever observing the parser error. This is the same failure mode that was fixed upstream in multipasta (tim-smart/multipasta#40); the fix there landed in the web-stream wrapper, whose equivalent here ismakeChannel.Note: on
mainthe external multipasta dependency is gone — the parser is vendored ineffect/unstable/http/MultipartParser— so instead of a dependency bump this fixes the vendored wrapper. The issue is labeled3.0; happy to backport or adjust if a 3.x-line fix (multipasta bump) is wanted separately.Changes
Multipart.makeChannel: propagate parser errors (and upstream stream failures) to in-progress file part streams via afileExitexit, checked after buffered chunks drain. A part that completed before the parser errored still ends cleanly (finishedwins), and only the in-progress part fails.toPersisted's default file writer: only wrapPlatformErrorasInternalError; parserMultipartErrors now keep their reason (e.g.BodyTooLarge→ 413 instead of a maskedInternalError→ 500). Same forFile.contentEffect, which previously wrapped every error asInternalError.maxTotalSizemid-file,maxFileSizemid-file, and a two-file case asserting a completed first file persists fully before the second file fails.effectpatch.Reproduction & verification
main(an event-loop-starving busy loop — verified by stashing the fix; no in-process timeout can preempt it) and pass with the fix.BunMultipart.persisted(request)withmaxTotalSize: 1024and a chunked 8 KB upload — hangs pre-fix (killed after 15 s), fails fast withMultipartError: BodyTooLargepost-fix.pnpm test --run test/unstable/http/Multipart.test.ts(13/13),HttpServerRequest.test.ts+HttpEffect.test.ts(40/40),@effect/platform-nodeMultipartParser.test.ts(84/84),bun node_modules/vitest/vitest.mjs run --project @effect/platform-bun(13/13),pnpm check(tsc -b, clean),pnpm lint-fix(0 errors).Review & limitations
Causethrough instead of squashing, multi-file ordering test added). Final pass: LGTM.@effect/platform-buntest project has no multipart coverage; Bun verification was done with an ad-hoc script (deleted after use) rather than a committed test.testTimeout) can fire — noted in a comment above the tests.Closes #6284