-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: stream provider media uploads instead of buffering files in memory #1835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9980825
1b121d0
c2944e1
b0bb1c8
4013c1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ import { ApplicationFailure } from '@temporalio/activity'; | |
| import { readOrFetch } from '@gitroom/helpers/utils/read.or.fetch'; | ||
| import { getSsrfSafeDispatcher } from '@gitroom/nestjs-libraries/dtos/webhooks/ssrf.safe.dispatcher'; | ||
| import sharp from 'sharp'; | ||
| import { createReadStream, statSync } from 'fs'; | ||
| import { Readable } from 'stream'; | ||
|
|
||
| export type ValidityMedia = { | ||
| path: string; | ||
|
|
@@ -173,6 +175,162 @@ export abstract class SocialAbstract { | |
| return { width, height }; | ||
| } | ||
|
|
||
| // Resolves the total byte size of the media without loading it into memory: | ||
| // a HEAD request for remote URLs, statSync for local files. | ||
| protected async mediaSize(path: string, identifier = ''): Promise<number> { | ||
| if (path.indexOf('http') === 0) { | ||
| // the media path is user-influenced, keep the SSRF-safe dispatcher that | ||
| // this.fetch applies to every other outbound request. identity encoding | ||
| // so content-length matches the bytes a later GET actually streams | ||
| // (fetch transparently decompresses encoded bodies). | ||
| const head = await fetch(path, { | ||
| method: 'HEAD', | ||
| headers: { 'accept-encoding': 'identity' }, | ||
| dispatcher: getSsrfSafeDispatcher(), | ||
| } as any); | ||
| const length = Number(head.headers.get('content-length')); | ||
| // A failed HEAD can still carry a content-length (of the error body), | ||
| // and a zero/NaN size would poison chunk-count math downstream. | ||
| if (!head.ok || !Number.isFinite(length) || length <= 0) { | ||
| throw new BadBody( | ||
| identifier, | ||
| '{}', | ||
| Buffer.from('{}'), | ||
| 'Could not determine the media size for upload' | ||
| ); | ||
| } | ||
| return length; | ||
| } | ||
|
|
||
| return statSync(path).size; | ||
| } | ||
|
|
||
| // Reads a single [start, end] byte range into memory. Used by providers whose | ||
| // chunked-upload APIs require a Buffer per segment: only one small chunk is | ||
| // resident at a time, never the whole file. | ||
| protected async mediaChunk( | ||
| path: string, | ||
| start: number, | ||
| end: number, | ||
| identifier = '' | ||
| ): Promise<Buffer> { | ||
| if (path.indexOf('http') === 0) { | ||
| const response = await fetch(path, { | ||
| headers: { | ||
| Range: `bytes=${start}-${end}`, | ||
| 'accept-encoding': 'identity', | ||
| }, | ||
| dispatcher: getSsrfSafeDispatcher(), | ||
| } as any); | ||
| // Anything but 206 means the server ignored the Range header: buffering | ||
| // response.body here would silently load the whole file into memory and | ||
| // upload corrupted chunks. | ||
| if (response.status !== 206) { | ||
| throw new BadBody( | ||
| identifier, | ||
| '{}', | ||
| Buffer.from('{}'), | ||
| `Media server did not honor the range request (status ${response.status})` | ||
| ); | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
| return Buffer.from(await response.arrayBuffer()); | ||
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| createReadStream(path, { start, end }) | ||
| .on('data', (chunk) => chunks.push(chunk as Buffer)) | ||
| .on('end', () => resolve(Buffer.concat(chunks))) | ||
| .on('error', reject); | ||
| }); | ||
| } | ||
|
|
||
| // Opens the media as a Node stream. The media path is user-influenced, so | ||
| // remote URLs go through the same SSRF-safe dispatcher as every other | ||
| // outbound request - never fetch these with plain axios/fetch. | ||
| protected async mediaStream( | ||
| path: string, | ||
| identifier = '' | ||
| ): Promise<Readable> { | ||
| if (path.indexOf('http') !== 0) { | ||
| return createReadStream(path); | ||
| } | ||
|
|
||
| // identity encoding so the streamed byte count matches the size mediaSize | ||
| // reported - a decompressed body would overflow any declared length. | ||
| const response = await fetch(path, { | ||
| headers: { 'accept-encoding': 'identity' }, | ||
| dispatcher: getSsrfSafeDispatcher(), | ||
| } as any); | ||
|
|
||
| if (!response.ok || !response.body) { | ||
| throw new BadBody( | ||
| identifier, | ||
| '{}', | ||
| Buffer.from('{}'), | ||
| 'Could not read the media for upload' | ||
| ); | ||
| } | ||
|
|
||
| return Readable.fromWeb(response.body as any); | ||
| } | ||
|
|
||
| // Streamed request bodies can't be replayed by this.fetch's retry, so | ||
| // providers wrap streamed uploads in a factory that rebuilds the request | ||
| // (re-opening its source streams) on every attempt. Errors carrying a | ||
| // `response` (axios errors, or a thrown { response: { status, data } }) | ||
| // go through the same retry and handleErrors classification rules as | ||
| // this.fetch; anything else is rethrown untouched so Temporal keeps its | ||
| // usual retry behavior. | ||
| protected async runStreamedUpload<T>( | ||
| func: () => Promise<T>, | ||
| identifier = '', | ||
| totalRetries = 0 | ||
| ): Promise<T> { | ||
| try { | ||
| return await func(); | ||
| } catch (err: any) { | ||
| if (!err?.response) { | ||
| throw err; | ||
| } | ||
|
Comment on lines
+292
to
+295
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Errors thrown when fetching media for Suggested FixWhen a media fetch fails with a non-ok status, throw an error object that includes the Prompt for AI Agent |
||
|
|
||
| const status = err.response.status || 500; | ||
| const data = err.response.data; | ||
| // '|| {}' / "|| '{}'" match this.fetch, which never hands handleErrors | ||
| // an empty string. | ||
| const json = | ||
| (typeof data === 'string' ? data : safeStringify(data || {})) || '{}'; | ||
| const handleError = this.handleErrors(json, status); | ||
|
|
||
| if ( | ||
| totalRetries <= 2 && | ||
| (status === 429 || | ||
| (status === 500 && !handleError) || | ||
| handleError?.type === 'retry' || | ||
| json.includes('rate_limit_exceeded') || | ||
| json.includes('Rate limit')) | ||
| ) { | ||
| await timer(5000); | ||
| return this.runStreamedUpload(func, identifier, totalRetries + 1); | ||
|
Comment on lines
+306
to
+314
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The retry condition in Suggested FixChange the retry condition in Prompt for AI Agent |
||
| } | ||
|
|
||
| if ( | ||
| (status === 401 && | ||
| (handleError?.type === 'refresh-token' || !handleError)) || | ||
| handleError?.type === 'refresh-token' | ||
| ) { | ||
| throw new RefreshToken(identifier, json, '{}', handleError?.value); | ||
| } | ||
|
|
||
| throw new BadBody( | ||
| identifier, | ||
| json, | ||
| '{}', | ||
| handleError?.value || 'Unknown Error' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| public async mention( | ||
| token: string, | ||
| d: { query: string }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: gitroomhq/postiz-app
Length of output: 19327
🏁 Script executed:
Repository: gitroomhq/postiz-app
Length of output: 50376
🏁 Script executed:
Repository: gitroomhq/postiz-app
Length of output: 34687
🏁 Script executed:
Repository: gitroomhq/postiz-app
Length of output: 16653
🏁 Script executed:
Repository: gitroomhq/postiz-app
Length of output: 23667
Resolve non-HTTP media paths before using them with the filesystem.
mediaSizeandmediaChunkusestatSync/createReadStreamfor any path that does not start withhttp, but providers pass raw media paths such asitem.pathormedia.path. Local uploads are stored with URLs like/uploads/year/month/day/r…, not absolute filesystem paths, so these calls can resolve relative to the worker cwd and fail. Resolve these cases likegetImageDimensions/updateMediauseprocess.env.UPLOAD_DIRECTORYfor non-HTTP paths.🤖 Prompt for AI Agents