fix: upload handler loading file contents to memory twice #37550
Conversation
🦋 Changeset detectedLatest commit: 860f117 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
WalkthroughRefactors getUploadFormData to require a web Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getUploadFormData
participant NodeReadable as Readable.fromWeb
participant Busboy
participant resultPromise as Promise
Caller->>getUploadFormData: call(request)
getUploadFormData->>getUploadFormData: assert request.body is ReadableStream
alt invalid body
getUploadFormData-->>Caller: reject(MeteorError)
else valid body
getUploadFormData->>resultPromise: Promise.withResolvers()
getUploadFormData->>NodeReadable: Readable.fromWeb(request.body)
NodeReadable->>Busboy: pipe(stream)
par Error paths
Busboy-->>resultPromise: error / limits / invalid field -> reject(err)
FileStream-->>resultPromise: file too large -> reject(err)
and Success
Busboy-->>resultPromise: finish -> resolve(uploadedFile)
end
resultPromise-->>Caller: return Promise
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Looks like this PR is ready to merge! 🎉 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/meteor/app/api/server/lib/getUploadFormData.spec.ts (1)
1-1: ReadableStream-based mock body matches implementation; minor nit onpullUsing
ReadableStreamfrom'stream/web'and enqueuing the multipartbufferonce is a good match for the newgetUploadFormDatabehavior and avoids any blob-based double buffering in the test harness. Only minor nit:pulldoesn’t useawait, so you can drop theasynckeyword if you want to avoid an unnecessary microtask.Also applies to: 41-44
apps/meteor/app/api/server/lib/getUploadFormData.ts (3)
2-2: Stream pipeline fix removes double buffering; confirmPromise.withResolvers/ReadableStreamsupportRouting
request.bodythroughReadable.fromWeb(...)after an explicitReadableStreamguard is a solid way to avoid the previousrequest.blob()+ second buffering step, and returning the singleresultPromisekeeps control-flow straightforward. The only caveat is platform/tooling support: bothPromise.withResolversandReadableStreamfrom'stream/web'assume a relatively modern Node/TS setup; if you still target older environments, consider a hand-rolled promise as a fallback:- const { promise: resultPromise, resolve, reject } = Promise.withResolvers<UploadResultWithOptionalFile<K>>(); + let resolve!: (value: UploadResultWithOptionalFile<K>) => void; + let reject!: (reason?: unknown) => void; + const resultPromise = new Promise<UploadResultWithOptionalFile<K>>((res, rej) => { + resolve = res; + reject = rej; + });Also applies to: 69-71, 91-91, 167-170
81-89:uploadedFileis always initialized, so'No file or fields were uploaded'branch is effectively deadBecause
uploadedFileis created up front as a stub object (withfields,file, etc. allundefined) and never set back toundefined, theif (!uploadedFile)check inonEndwill never trigger. The only actual “no data” failure path is now the'No file uploaded'branch whenfileOptionalis false. If you don’t need the distinct'No file or fields were uploaded'error any more, you could drop that first condition; otherwise, you’d need to change the initialization strategy (e.g., start withuploadedFileundefined and set it when you see the first field/file).Also applies to: 97-108
110-141: Normalize rejection reasons for busboy errors/limitsMost error paths now reject with
MeteorError, but a few are inconsistent:partsLimitandfieldsLimitcallreject()with no reason, andfilesLimitrejects with a bare string. For consumers expecting an Error-like shape (or specificMeteorErrorcodes), that makes error handling more brittle. Consider standardizing these toMeteorError(or at leastError) and optionally callingcleanup()to detach listeners:bb.on('error', (err: Error) => { - reject(err); + reject(err); }); bb.on('partsLimit', () => { - reject(); + reject(new MeteorError('error-parts-limit-reached')); }); bb.on('filesLimit', () => { - reject('Just 1 file is allowed'); + reject(new MeteorError('error-too-many-files')); }); bb.on('fieldsLimit', () => { - reject(); + reject(new MeteorError('error-fields-limit-reached')); });(Adjust error codes/messages to whatever convention you already use for upload errors.)
Also applies to: 153-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/api/server/lib/getUploadFormData.spec.ts(2 hunks)apps/meteor/app/api/server/lib/getUploadFormData.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-16T19:09:43.823Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37496
File: packages/apps-engine/tests/server/runtime/deno/LivenessManager.spec.ts:3-3
Timestamp: 2025-11-16T19:09:43.823Z
Learning: In Node.js, `EventEmitter` can be imported from either the 'events' module or the 'stream' module—both export the same reference. While 'events' is the canonical module, importing from 'stream' is valid and works correctly.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 📦 Meteor Build (coverage)
d-gubert
force-pushed
the
fix/upload-file-double-buffer
branch
from
9d5f0d2 to
1a99c43
Compare
d-gubert
force-pushed
the
fix/upload-file-double-buffer
branch
from
1a99c43 to
f820324
Compare
d-gubert
force-pushed
the
fix/upload-file-double-buffer
branch
from
1620b8d to
6388dd4
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 19:17", "11/19 19:43 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 5 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 19:43:15 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37550 +/- ##
===========================================
- Coverage 68.97% 68.96% -0.02%
===========================================
Files 3359 3359
Lines 114214 114177 -37
Branches 20535 20533 -2
===========================================
- Hits 78784 78745 -39
- Misses 33335 33344 +9
+ Partials 2095 2088 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
69-71: Consider using MeteorError for consistency.The error handling here uses a generic
Errorwhile the rest of the function usesMeteorError. For consistency across error handling paths, consider usingMeteorErrorhere as well.Apply this diff:
- if (!(request.body instanceof ReadableStream)) { - return Promise.reject(new Error('Invalid request body')); - } + if (!(request.body instanceof ReadableStream)) { + return Promise.reject(new MeteorError('invalid-request-body', 'Invalid request body')); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/tame-crabs-grin.md(1 hunks)apps/meteor/app/api/server/lib/getUploadFormData.spec.ts(2 hunks)apps/meteor/app/api/server/lib/getUploadFormData.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/lib/getUploadFormData.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-16T19:09:43.823Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37496
File: packages/apps-engine/tests/server/runtime/deno/LivenessManager.spec.ts:3-3
Timestamp: 2025-11-16T19:09:43.823Z
Learning: In Node.js, `EventEmitter` can be imported from either the 'events' module or the 'stream' module—both export the same reference. While 'events' is the canonical module, importing from 'stream' is valid and works correctly.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
packages/core-services/src/index.ts (1)
MeteorError(56-56)
🔇 Additional comments (4)
apps/meteor/app/api/server/lib/getUploadFormData.ts (3)
97-108: LGTM!The refactored error handling using
rejectandresolveis clear and correct.
167-168: Excellent fix - eliminates double buffering!This change correctly addresses the PR objective by using
Readable.fromWeb()to directly pipe the web stream to busboy, avoiding the previous approach that calledrequest.blob()(which would buffer the entire content in memory first). The file contents are now only buffered once in busboy'sonFilehandler.
91-91: Promise.withResolvers() is fully supported in this project.The project's Node.js version is set to 22.16.0 (per package.json engines), which exceeds the minimum v22.0.0 requirement for Promise.withResolvers() support. No action is needed.
.changeset/tame-crabs-grin.md (1)
1-5: LGTM!The changeset correctly documents the patch fix for the double buffering issue.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/app/api/server/lib/getUploadFormData.ts (2)
115-118: Callcleanup()before rejecting on invalid field and truncated file to avoid lingering listeners and extra busboy events.These two early‑error branches still don’t invoke
cleanup(), so busboy keeps its event listeners attached and can continue to fireend/finish(and possibly limit) events after the promise has already been rejected. That was flagged previously and is still unresolved in this revision.A minimal fix is to invoke
cleanup()before rejecting in both cases:function onFile( fieldname: string, file: Readable & { truncated: boolean }, { filename, encoding, mimeType: mimetype }: { filename: string; encoding: string; mimeType: string }, ) { if (options.field && fieldname !== options.field) { - file.resume(); - return reject(new MeteorError('invalid-field')); + cleanup(); + file.resume(); + return reject(new MeteorError('invalid-field')); } const fileChunks: Uint8Array[] = []; file.on('data', (chunk) => { fileChunks.push(chunk); }); file.on('end', () => { if (file.truncated) { - fileChunks.length = 0; - return reject(new MeteorError('error-file-too-large')); + cleanup(); + fileChunks.length = 0; + return reject(new MeteorError('error-file-too-large')); }This ensures busboy’s listeners are removed as soon as you know the request should fail, preventing further callbacks after rejection and reducing the risk of subtle memory/behavior issues.
Also applies to: 125-129
153-165: Normalize busboy error/limit handlers to reject withMeteorErrorinstead of bare values.These handlers still reject with either
undefinedor a bare string, which was already called out earlier and leads to inconsistent error handling downstream (some callers will seeErrorinstances, others raw strings orundefined).Align them with the rest of the function by always rejecting with
MeteorErrorinstances and clear codes/reasons, e.g.:bb.on('error', (err: Error) => { - reject(err); + reject(err); }); bb.on('partsLimit', () => { - reject(); + reject(new MeteorError('error-parts-limit', 'Too many parts in multipart form')); }); bb.on('filesLimit', () => { - reject('Just 1 file is allowed'); + reject(new MeteorError('error-files-limit', 'Just 1 file is allowed')); }); bb.on('fieldsLimit', () => { - reject(); + reject(new MeteorError('error-fields-limit', 'Too many fields')); });If you prefer, you can also call
cleanup()in these handlers before rejecting to mirror theonFileerror paths and ensure all listeners are removed on any terminal failure.
🧹 Nitpick comments (2)
apps/meteor/app/api/server/lib/getUploadFormData.ts (2)
91-91:Promise.withResolverspattern is fine;asyncis now redundant and runtime support should be confirmed.Managing completion via
Promise.withResolversgives you clear resolve/reject control; since you just returnresultPromise, theasynckeyword ongetUploadFormDatais no longer needed and only adds a tiny bit of overhead. Also,Promise.withResolversis a relatively new language feature, so it’s worth ensuring your Node/TS target and polyfills guarantee its availability in all deployment environments.Consider:
- Dropping
asyncfrom the function declaration while still returningresultPromise, and- Verifying your tsconfig/lib and Node runtime version include
Promise.withResolversso this doesn’t break older environments.Also applies to: 170-170
97-108: TightenonEndguard and make validation errors more informative.Two small points here:
uploadedFileis initialized once and never set back toundefined, soif (!uploadedFile)is effectively unreachable right now; you can either remove this branch or setuploadedFile = undefinedexplicitly in error/cleanup paths if you want that guard to be meaningful.options.validate.errors?.join(', ')will stringify Ajv error objects as"[object Object]"; if you care about diagnostics, consider either joining on each error’s.messageor passing the rawerrorsarray asdetailsonMeteorErrorinstead of interpolating it into the string.Please verify how
MeteorErroris typically constructed in this codebase (code vs. reason vs. details) and whether you want structured Ajv errors available to callers rather than a generic interpolated string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/api/server/lib/getUploadFormData.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-16T19:09:43.823Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37496
File: packages/apps-engine/tests/server/runtime/deno/LivenessManager.spec.ts:3-3
Timestamp: 2025-11-16T19:09:43.823Z
Learning: In Node.js, `EventEmitter` can be imported from either the 'events' module or the 'stream' module—both export the same reference. While 'events' is the canonical module, importing from 'stream' is valid and works correctly.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
packages/core-services/src/index.ts (1)
MeteorError(56-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
2-2: Streaming fromrequest.bodyviaReadable.fromWebcorrectly removes the extra buffering; verifyReadableStreamcompatibility.Switching from
request.blob()toReadable.fromWeb(request.body)plus theReadableStreamguard is a good way to avoid double-buffering the upload in memory and satisfy the PR goal. The only thing to double‑check is that every caller runs in an environment whererequest.bodyis a WebReadableStreaminstance compatible withstream/web’sReadableStreamconstructor (no custom fetch/ReadableStream polyfills), otherwise theinstanceof ReadableStreamcheck will cause valid uploads to be rejected with “Invalid request body”.Please confirm that all runtime environments using this code expose
Request.bodyas astream/web‑compatibleReadableStreamand that this function is only called for requests that are expected to have a body (e.g., POST uploads).Also applies to: 69-71, 167-168
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Upload handling function was previously loading the file contents via
request.blob(), getting a ReadableStream (web api) from it, creating a new Readable stream (nodejs stream), and then piping the stream to busboy, which in turn accumulated the chunks again in a different Buffer instance.Issue(s)
ARCH-1881
Steps to test or reproduce
Further comments
Summary by CodeRabbit