perf(video): stream uploads into a single on-disk copy - #9396
Open
lstein wants to merge 5 commits into
Open
Conversation
Declaring `file: UploadFile` makes Starlette parse the multipart body into its own spooled temp file before the route runs; the route then copied that into a named temp file it could hand to ffmpeg and `videos.create`. Every in-flight upload therefore held TWO full-size copies in temp storage (up to 2 x MAX_UPLOAD_SIZE x MAX_CONCURRENT_VIDEO_UPLOADS = 4 GB) for the whole probe/thumbnail/create phase. invoke-ai#9163 only shrank the overlap window by closing the spool right after the copy loop; the spool's own path can't be reused, because once rolled over it is an unlinked anonymous file with no path. The route now parses the body itself with python_multipart's streaming parser (already a FastAPI dependency), writing the `file` part directly into the one temp file and buffering only the small `metadata` field. Peak temp usage per upload is halved. Two behavioral improvements fall out of streaming the body: - The filename/MIME gate fires from the part headers, so an unsupported file is rejected before any of its bytes reach the disk instead of after the whole body has been spooled. - MAX_UPLOAD_SIZE is enforced as the bytes arrive rather than after. Parsing runs in the thread pool (it writes to disk), and the request schema is pinned with `openapi_extra` so the documented multipart contract is byte-for- byte what the `file` + `metadata` parameters generated — the only OpenAPI change is that the body schema is now inline rather than a `Body_upload_video` component, which nothing references. Deferred non-blocker from PR invoke-ai#9163. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
dunkeroni
as code owners
July 28, 2026 01:54
Same fields, types and requiredness — the body schema is now inline rather than a Body_upload_video component (nothing references it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JPPhoto
requested changes
Aug 9, 2026
Collaborator
There was a problem hiding this comment.
Merge blockers:
invokeai/app/api/routers/videos.py:406-419: A denied or unknownboard_idraises before_stream_video_upload()readsrequest.stream(). The upload middleware has already incremented its global and per-user counters, but the route returns immediately and itsfinallyreleases those counters while a chunked client can keep sending the body, defeating the 429, idle-timeout, and duration accounting. Test: drive the ASGI stack with the maximum number of forbidden-board uploads whosereceive()keeps yielding slowhttp.requestchunks, then start another upload; it must remain 429 until the first requests disconnect, but this route returns 403 and releases the slots.
Other findings/issues:
invokeai/app/api/routers/videos.py:299-305:_stream_video_upload()callsMultipartParser.finalize()and checks onlysaw_file_part;python-multipartfinalization does not verify that the parser reached its end state. A body ending immediately after file bytes, without the closing multipart boundary, therefore returnssaw_file_part=Trueand proceeds to MP4 validation/probe/create as if complete. This accepts malformed or interrupted uploads and can persist a truncated file when later media checks happen to pass. Test: send a valid file part withContent-Type: multipart/form-data; boundary=bbut omit--b--, then assert a 400/422 response and thatvideos.createis not called; current parser state isPART_DATA, yet the route continues.
Suggestions:
-
Consider explicitly aborting/draining rejected request bodies while retaining the middleware lease; this preserves early authorization rejection without a slow-upload quota hole.
-
Consider a sink-backed Starlette multipart parser or explicit parser-contract tests; this would reduce custom parsing logic and prevent future API/schema drift.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-on to #9163 (deferred non-merge-blocker), and the last of that PR's deferred list.
Declaring
file: UploadFilemakes Starlette parse the multipart body into its own spooled temp file before the route body runs. The route then copied that into aNamedTemporaryFile, because ffmpeg andvideos.createneed a real path — and a rolled-overSpooledTemporaryFileis an unlinked anonymous file with no path to reuse.So every in-flight upload held two full-size copies in temp storage — up to
2 × MAX_UPLOAD_SIZE × MAX_CONCURRENT_VIDEO_UPLOADS= 4 GB — for the whole probe/thumbnail/create phase. #9163 only shrank the overlap by closing the spool right after the copy loop.Approach
The route now parses the body itself with
python_multipart's streaming parser (already a FastAPI dependency — this adds no new packages), writing thefilepart directly into the single temp file and buffering only the smallmetadatafield, under its own 1 MB cap. Peak temp usage per upload is halved.Two behavioral improvements fall out of streaming rather than spooling:
MAX_UPLOAD_SIZEis enforced as bytes arrive, not after they've all landed.Parsing runs in the thread pool, since the callbacks write to disk.
API contract
The request schema is pinned with
openapi_extrareproducing exactly what thefile+metadataparameters generated. The only difference in the regeneratedschema.tsis that the body type is now inline instead of aBody_upload_videocomponent — nothing references that component (the frontend builds itsFormDataby hand viaUploadVideoArg), and the field names, types and requiredness are unchanged.metadataJSON validation now runs after the body has streamed rather than before, since the field can legally arrive after the file part. Rejection is identical from the caller's point of view and still happens beforevideos.create.Testing
New tests in
test_video_upload_limits.py, driving the route with a chunked multipart body: single-copy write-through (byte-exact, in order), rejection from the part headers without consuming the rest of the body, missing-file-part → 422, oversized file part rejected mid-stream after ~the cap rather than the whole body, and the existing disk-full cleanup test ported to the new signature.test_configured_upload_slots_bound_peak_double_spool_usagebecomes..._peak_temp_storage_usageand now asserts the one-copy budget.The end-to-end multipart paths (malformed MP4 → 415, spoofed container → 415, malformed metadata → 422, valid metadata → 201) are already covered in
test_videos_multiuser.pyand pass unchanged.pytest tests/app/api tests/app/routers— 654 passed. Frontendlint:tsc/lint:prettierclean.🤖 Generated with Claude Code