fix(remix-node): stop `maxFileSize` being exceeded causing the server to exit

[uwuru]

I am not sure if this is all that is needed (as we're using await to call uploadHandler() so do we not need to do this?) or if I'm missing something and we should be doing more checks or something and still throwing. Would like to kick off fixing it though.

• [x] Docs - N/A
• [ ] Tests - Happy to look into how to approach this if needed and PR progresses

Fixes #2230

[remix-cla-bot[bot]]

Hi @uwuru,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot[bot]]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[swwind]

Surely, error was already thrown into busboy.emit("error", error) and there is no need to throw it again (which will only cause fileWorkQueue to be rejected but not awaited).

badly need this to be fixed

[mjackson]

Can you please include a test here? It would be nice to test what actually happens when a file upload is too large.

[uwuru]

@mjackson Done, though this still passes even when you revert the fix of removing throw error because it depends how the parent process handles the uncaught error.

Jest behaves as follows with the removed throw error put back, and carries on testing. In comparison to the dev server (in the case of this issue/PR, not tried it with others), whose process exits:

...
 PASS   node  packages/remix-node/__tests__/parseMultipartFormData-test.ts
(node:51136) UnhandledPromiseRejectionWarning: Error: Field "file" exceeded upload size of 1 bytes.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:51136) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:51136) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
 PASS   node  packages/remix-node/__tests__/sessions-test.ts
...

[uwuru]

I have noticed the integration tests are now failing and there was already a test there for this (with out javascript). Which was passing but is now failing 🤔 I will try and look over the weekend. Interested to hear if any one has any thoughts though.

There's also another one with javascript but that's being skipped as it isn't working as expected.

[xardit]

Already fixed it with this patch https://github.com/remix-run/remix/issues/2230#issuecomment-1100039432

[mikkpokk]

@uwuru File /integration/action-test.ts on line 73 you should modify action behaviour to respond with status code 500 when the action fails on some reason. Removing throw error; from the file parseMultipartFormData.ts also passes the request with status code 200 without self-handling the HTTP status code :)

That is the reason the test fails. It expects 500 but gets 200. I would suggest to replace 500 with 422. 500 - Internal Server Error - refers to an unexpected error.

And yes, @mjackson, that same throw error; causes node server shut down at the same time. Even wrapping the code with a try { } catch { } is useless. It catches the error but shutdowns the server at the same time.

[vedantroy]

While this PR is getting merged -- will my production server be affected? E.g; will it currently crash if a file upload is too large? I'm not in any position to request an ETA on the fix here, but I hope the fix comes soon given that the bug is breaking things on prod.

[machour]

@uwuru can you please update your PR according to @mikkpokk feedback? A 422 - Unprocessable Entity error seems like the way to go

[MichaelDeBoey]

@uwuru Could you please rebase your branch onto latest dev & resolve conflicts?

[chaance]

Looks like this has diverged a good deal since our current implementation of unstable_parseMultipartFormData moved to @remix-run/server-runtime. Closing for now, but feel free to open a new PR if the issue persists.