Fix multipart parser continuing after maxParts - #7144
Conversation
fubhy
added
bug
🦋 Changeset detectedLatest commit: a376699 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new regression test triggers an unhandled ReadableStream rejection that fails the whole test suite, and the other fixed limits (maxFieldSize, maxPartSize) have no new regression coverage.
Reviewed changes
- Added a
stoppedflag to the low-level multipart parser and early returns aftermaxParts,maxPartSize, andmaxFieldSizeviolations. - Suppressed the
EndNotReachederror inend()when parsing has already stopped. - Added a test verifying that only the allowed parts are emitted before a
MaxPartserror.
⚠️ Missing test coverage for maxFieldSize / maxPartSize
The parser change also stops processing for maxFieldSize and maxPartSize violations, but the only new test exercises MaxParts. Adding analogous tests for the other two limits would prevent future regressions.
ℹ️ Nitpicks
- The new test's error assertion only checks
_tag; consider also assertingerror.reason._tag === "TooManyParts"to match the existing limit test.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
c015fe2 to
673bae1
Compare
fubhy
changed the title
There was a problem hiding this comment.
Important
The new regression test is clean and focused, but this PR cannot be merged as-is: it adds only a failing test with no implementation fix, so CI will fail.
Reviewed changes
- Rewrote the regression test to use the low-level parser. The test now constructs a raw
MultipartParser.makeinstance and feeds it an explicit byte stream, which fixes theReadableStreamunhandled-rejection problem from the prior review. - Dropped the parser implementation changes. The
stoppedflag and early-return logic that were present in the previously reviewed commit are no longer in the branch; the diff is now test-only. - Added a focused
maxPartsregression test. It verifies that only["a", "b"]are delivered before theMaxPartslimit error whenmaxParts: 2and the body contains four fields.
⚠️ PR is missing the actual fix
The title says "Fix multipart parser continues processing after limit violations", but the current diff does not modify the parser. Running the new test fails with fields = ["a", "b", "c", "d"], confirming the bug is still present. Either the fix should be restored to this PR, or the PR title/description should be updated to reflect that it is only a regression test.
⚠️ Still no regression coverage for maxFieldSize / maxPartSize
The parser reports limit errors for maxFieldSize and maxPartSize but continues processing those parts as well. The prior review asked for analogous tests for these limits; they are still missing. Adding them would prevent the same class of regression for the other limits.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Restored the parser stop behavior in
multipart.ts:77: astoppedflag on parse state now halts boundary processing aftermaxParts,maxPartSize, ormaxFieldSizeis exceeded, so later parts are no longer delivered and the limit error fires exactly once (previously each violating chunk re-calledonError). - Suppressed
EndNotReachedinend()when parsing already stopped, and resetstoppedon parser reset so the low-level parser remains reusable. - Added
it.eachregression coverage inMultipart.test.tsasserting only the allowed fields plus a single{ _tag: "ReachedLimit", limit }for all three limits. - Added an
effectpatch changeset.
I verified the three new tests fail against the pre-fix parser (repeated limit errors and over-delivered fields) and pass with the fix. Both concerns from the prior review — the missing parser fix and the missing maxFieldSize/maxPartSize coverage — are addressed.
ℹ️ maxTotalSize still "continues after limit"
The stopped mechanism fixes three of the four ReachedLimit branches, but the maxTotalSize branch in write() (multipart.ts:234) returns onError(errMaxTotalSize) without setting stopped, unlike its siblings — so a body exceeding the total-size cap is still fully consumed (and prior parts delivered) rather than halted. This is outside the PR's stated scope (the summary and changeset name only the three limits), so I'm flagging it only to confirm it's intentional.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Stop the low-level multipart parser after
maxParts,maxPartSize, ormaxFieldSizeis exceeded so later fields are not delivered and the limit error is reported only once.Changes
EndNotReachedafter parsing has intentionally stopped on a limit.Multipart.test.tsfor all three limits.effectpatch changeset.Validation
pnpm vitest packages/effect/test/unstable/http/Multipart.test.ts --runpnpm checkpnpm lintCloses EFF-568