S3 multipart uploads up to 30 GiB#33
Open
bmdavis419 wants to merge 5 commits into
Open
Conversation
Large files use chunked S3 uploads while smaller files keep the single-PUT path, with IndexedDB-backed resume for interrupted transfers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds resumable S3 multipart video uploads: server Convex actions and S3 helpers, client upload orchestrator with resume and progress tracking, IndexedDB resume persistence, and UI wiring to show a "Resuming" state. ChangesResumable Multipart Video Uploads
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard as Dashboard UI
participant Hook as useVideoUploadManager
participant ResumeDB as IndexedDB
participant Upload as uploadVideoFile
participant Server as Convex Server
participant S3 as S3
User->>Dashboard: Select file to upload
Dashboard->>Hook: addFilesToProject(files)
Hook->>Hook: Compute fingerprint
Hook->>ResumeDB: findUploadResumeSessionByFingerprint
alt Resume session found
ResumeDB-->>Hook: Resume session {uploadId, completedParts, ...}
Hook->>Hook: Set resuming: true
else No resume session
Hook->>Hook: Set resuming: false
end
Hook->>Upload: uploadVideoFile({file, resumeSession})
Upload->>Server: initiateVideoUpload
Server->>S3: Decide: single | multipart | already_uploaded
alt strategy = multipart
Server-->>Upload: {uploadId, parts: [], plan}
Upload->>Upload: Merge completed parts from resume
Upload->>Upload: Compute pending parts
Upload->>ResumeDB: saveUploadResumeSession
loop For each batch of pending parts
Upload->>Server: signUploadParts({partNumbers})
Server->>S3: PresignUploadPartCommand
S3-->>Upload: Signed URLs
Upload->>S3: PUT part blobs (concurrent)
S3-->>Upload: ETags
Upload->>Upload: Update completedParts
Upload->>ResumeDB: saveUploadResumeSession (progress)
Upload->>Hook: onProgress(update)
Hook->>Dashboard: Display progress %, throughput, ETA
end
Upload->>Server: completeMultipartUpload({parts})
Server->>S3: CompleteMultipartUploadCommand
Upload->>ResumeDB: deleteUploadResumeSession
else strategy = single
Server-->>Upload: {url, expiration}
Upload->>S3: PUT entire file
S3-->>Upload: 200 OK
else strategy = already_uploaded
Upload->>ResumeDB: deleteUploadResumeSession
end
Upload->>Server: markUploadComplete
Server-->>Upload: {success: true}
Upload-->>Hook: Complete
Hook->>Dashboard: Update upload to complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/routes/dashboard/-useVideoUploadManager.ts (1)
152-171: 💤 Low valueFragile cancellation detection relies on exact error message string.
Checking
errorMessage === "Upload cancelled"is brittle. IfuploadVideoFilechanges its error message, cancellation handling breaks silently. Consider checkingabortController.signal.abortedinstead.♻️ Suggested improvement
} catch (error) { const errorMessage = error instanceof Error ? error.message : "Upload failed"; - const cancelled = errorMessage === "Upload cancelled"; + const cancelled = abortController.signal.aborted; setUploads((prev) =>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/routes/dashboard/-useVideoUploadManager.ts` around lines 152 - 171, The catch block in the upload handler should not treat cancellation by comparing error.message to "Upload cancelled"; instead detect cancellation via the AbortSignal or standard AbortError: replace the fragile check `errorMessage === "Upload cancelled"` with a robust check like `abortController.signal.aborted` (or `error.name === "AbortError"`) inside the same catch in -useVideoUploadManager.ts, then keep the existing setUploads behavior (set status to "pending" and remove the upload when cancelled) using the uploadId and the existing setUploads calls; ensure you reference the same abortController used when calling uploadVideoFile so the signal is available in this catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convex/s3Multipart.ts`:
- Around line 139-149: The current empty catch around s3.send(new
AbortMultipartUploadCommand(...)) swallows real failures and can leave orphaned
uploads; change the catch to capture the error (e.g. catch (err)), log the error
with context including BUCKET_NAME, args.key and args.uploadId (use the module's
logger or console.error), and rethrow or return a failure so callers know the
abort did not succeed (i.e. don't silently ignore errors from
AbortMultipartUploadCommand / s3.send).
In `@convex/videoActions.ts`:
- Around line 544-578: The code currently calls
ctx.runMutation(internal.videos.clearMultipartUploadId, { videoId: args.videoId
}) before validating the uploaded S3 object; move that call so it only runs
after successful metadata validation and reconciliation (i.e., after the
HeadObjectCommand checks, content length/type validation, and after
ctx.runMutation(internal.videos.reconcileUploadedObjectMetadata, ...)). Keep the
S3 metadata fetching (getS3Client + HeadObjectCommand), content length/type
checks (normalizeContentType, isAllowedUploadContentType) and the
reconcileUploadedObjectMetadata call as-is, and ensure clearMultipartUploadId is
invoked only on the successful path to preserve the multipart upload ID for
retries on transient failures.
In `@src/lib/uploadResumeDb.ts`:
- Around line 55-59: The saveUploadResumeSession currently lets IndexedDB write
failures propagate and block uploads; change it to be best-effort by wrapping
the runTransaction call inside a try/catch inside saveUploadResumeSession
(function name) so any error from runTransaction("readwrite", ...) is caught,
not rethrown, and the function returns normally; in the catch block log the
failure (e.g., console.warn or a logger) including the session id and the error
so we preserve observability while ensuring resume persistence failures do not
abort the multipart upload flow.
In `@src/lib/videoUpload.ts`:
- Around line 262-265: The code blindly merges resumeSession.completedParts into
the active upload (via mergeUploadedParts and initiate.uploadedParts), risking
stale ETags/part numbers corrupting the multipart state; add a compatibility
check (e.g., isResumeSessionCompatible or inline checks) that verifies the
resumeSession matches the current initiate context (compare uploadId/sessionId,
target bucket/key, file size or checksum, and any other identifying metadata)
and only pass resumeSession.completedParts into mergeUploadedParts when that
check passes; if it fails, ignore the resumeSession.completedParts (and
optionally log/warn) to avoid poisoning completedParts.
---
Nitpick comments:
In `@app/routes/dashboard/-useVideoUploadManager.ts`:
- Around line 152-171: The catch block in the upload handler should not treat
cancellation by comparing error.message to "Upload cancelled"; instead detect
cancellation via the AbortSignal or standard AbortError: replace the fragile
check `errorMessage === "Upload cancelled"` with a robust check like
`abortController.signal.aborted` (or `error.name === "AbortError"`) inside the
same catch in -useVideoUploadManager.ts, then keep the existing setUploads
behavior (set status to "pending" and remove the upload when cancelled) using
the uploadId and the existing setUploads calls; ensure you reference the same
abortController used when calling uploadVideoFile so the signal is available in
this catch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a3939c0-1688-4cca-9bad-d395f5f06472
📒 Files selected for processing (11)
app/routes/dashboard/-layout.tsxapp/routes/dashboard/-useVideoUploadManager.tsconvex/s3Multipart.tsconvex/schema.tsconvex/uploadLimits.tsconvex/videoActions.tsconvex/videos.tssrc/components/upload/UploadProgress.tsxsrc/lib/uploadLimits.tssrc/lib/uploadResumeDb.tssrc/lib/videoUpload.ts
Presigned UploadPart URLs do not include Content-Type in the signature, so sending that header caused S3 to reject part PUTs. Co-authored-by: Cursor <cursoragent@cursor.com>
Propagate abort failures, clear multipart state after reconcile, make resume persistence best-effort, and validate resume session compatibility before merging parts. Co-authored-by: Cursor <cursoragent@cursor.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Only apply the 20-part batch cap when signing URLs, not when completing uploads with hundreds of parts, and close IndexedDB on transaction errors. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/videoActions.ts (2)
400-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle stale S3 multipart uploads gracefully.
If S3 has auto-aborted the multipart upload (e.g., after lifecycle expiry),
listMultipartUploadedPartsthrowsNoSuchUpload. SincecanResumeMultipartUploadpassed based on DB state, users get stuck with no recovery path—initiateVideoUploadkeeps failing.Consider catching
NoSuchUploadhere and falling through to create a fresh multipart session, clearing the stales3MultipartUploadIdin the process.Proposed fix
if (canResumeMultipartUpload(video, args.fileSize)) { - const uploadedParts = await listMultipartUploadedParts({ - key: video.s3Key!, - uploadId: video.s3MultipartUploadId!, - }); - const { partSizeBytes, partCount } = getMultipartPlan(args.fileSize); - return { - strategy: "multipart" as const, - key: video.s3Key!, - uploadId: video.s3MultipartUploadId!, - partSizeBytes, - partCount, - uploadedParts, - }; + try { + const uploadedParts = await listMultipartUploadedParts({ + key: video.s3Key!, + uploadId: video.s3MultipartUploadId!, + }); + const { partSizeBytes, partCount } = getMultipartPlan(args.fileSize); + return { + strategy: "multipart" as const, + key: video.s3Key!, + uploadId: video.s3MultipartUploadId!, + partSizeBytes, + partCount, + uploadedParts, + }; + } catch (error) { + // S3 multipart upload may have expired; clear stale state and create fresh session below. + await ctx.runMutation(internal.videos.clearMultipartUploadId, { + videoId: args.videoId, + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/videoActions.ts` around lines 400 - 414, When resuming multipart uploads in the block that uses canResumeMultipartUpload(video, args.fileSize), wrap the listMultipartUploadedParts call in a try/catch that specifically handles the S3 NoSuchUpload error: on that error clear the stale video.s3MultipartUploadId (persist the change back to the DB so the record no longer points to a dead upload) and allow execution to fall through to the normal multipart-initiation path (so initiateVideoUpload can create a fresh multipart session); keep existing behavior for other errors and still return uploadedParts when listing succeeds.
605-617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear multipart state even when S3 abort fails with
NoSuchUpload.If the S3 multipart upload was already aborted or expired,
abortMultipartUploadSessionthrows andclearMultipartUploadIdnever runs. This leaves the video record in a broken state that can't be recovered via the client.Consider wrapping the S3 abort call and proceeding with DB cleanup regardless.
Proposed fix
handler: async (ctx, args) => { await requireVideoMemberAccess(ctx, args.videoId); const video = await getVideoForUpload(ctx, args.videoId); if (video.s3Key && video.s3MultipartUploadId) { - await abortMultipartUploadSession({ - key: video.s3Key, - uploadId: video.s3MultipartUploadId, - }); + try { + await abortMultipartUploadSession({ + key: video.s3Key, + uploadId: video.s3MultipartUploadId, + }); + } catch { + // S3 upload may already be aborted/expired; proceed to clear DB state. + } } await ctx.runMutation(internal.videos.clearMultipartUploadId, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/videoActions.ts` around lines 605 - 617, Currently abortMultipartUploadSession may throw (e.g., NoSuchUpload) and prevent running internal.videos.clearMultipartUploadId; wrap the call to abortMultipartUploadSession in a try/catch around the if (video.s3Key && video.s3MultipartUploadId) block, log or swallow the error when appropriate (ignore/continue on NoSuchUpload or other non-fatal S3 errors) and ensure ctx.runMutation(internal.videos.clearMultipartUploadId, { videoId: args.videoId }) always runs in the finally path so the DB multipart state is cleared even if the S3 abort fails.src/lib/uploadResumeDb.ts (1)
104-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose DB on request error to prevent connection leak.
The
request.onerrorhandler rejects but doesn't close the database connection. While the outertry-catchhandles the rejection gracefully, the DB connection remains open.Proposed fix
- request.onerror = () => reject(request.error ?? new Error("Failed to query resume sessions")); + request.onerror = () => { + db.close(); + reject(request.error ?? new Error("Failed to query resume sessions")); + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/uploadResumeDb.ts` at line 104, The request.onerror handler currently rejects but leaves the IndexedDB connection open; update the request.onerror to close the database before rejecting (e.g., call db?.close() or the db variable used to open the connection) so the connection is released on errors; modify the handler attached to request (request.onerror) to call db.close() (guarded if necessary) and then reject(request.error ?? new Error("Failed to query resume sessions")).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@convex/videoActions.ts`:
- Around line 400-414: When resuming multipart uploads in the block that uses
canResumeMultipartUpload(video, args.fileSize), wrap the
listMultipartUploadedParts call in a try/catch that specifically handles the S3
NoSuchUpload error: on that error clear the stale video.s3MultipartUploadId
(persist the change back to the DB so the record no longer points to a dead
upload) and allow execution to fall through to the normal multipart-initiation
path (so initiateVideoUpload can create a fresh multipart session); keep
existing behavior for other errors and still return uploadedParts when listing
succeeds.
- Around line 605-617: Currently abortMultipartUploadSession may throw (e.g.,
NoSuchUpload) and prevent running internal.videos.clearMultipartUploadId; wrap
the call to abortMultipartUploadSession in a try/catch around the if
(video.s3Key && video.s3MultipartUploadId) block, log or swallow the error when
appropriate (ignore/continue on NoSuchUpload or other non-fatal S3 errors) and
ensure ctx.runMutation(internal.videos.clearMultipartUploadId, { videoId:
args.videoId }) always runs in the finally path so the DB multipart state is
cleared even if the S3 abort fails.
In `@src/lib/uploadResumeDb.ts`:
- Line 104: The request.onerror handler currently rejects but leaves the
IndexedDB connection open; update the request.onerror to close the database
before rejecting (e.g., call db?.close() or the db variable used to open the
connection) so the connection is released on errors; modify the handler attached
to request (request.onerror) to call db.close() (guarded if necessary) and then
reject(request.error ?? new Error("Failed to query resume sessions")).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb8cf763-296a-43a2-8bc8-32802ea490af
📒 Files selected for processing (2)
convex/videoActions.tssrc/lib/uploadResumeDb.ts
Comment on lines
+600
to
+605
| } catch (error) { | ||
| try { | ||
| await deleteUploadedObject(video.s3Key); | ||
| } catch { | ||
| // Preserve the original validation or reconciliation failure. | ||
| } |
There was a problem hiding this comment.
🟠 High convex/videoActions.ts:600
In completeMultipartUpload, the catch block unconditionally deletes the uploaded S3 object on any error from lines 591-599. When reconcileUploadedObjectMetadata or clearMultipartUploadId fail due to transient database issues, the valid uploaded file is incorrectly deleted. Use shouldDeleteUploadedObjectOnFailure(error) to gate deletion, matching the pattern at line 756 in markUploadComplete.
- try {
- await deleteUploadedObject(video.s3Key);
- } catch {
- // Preserve the original validation or reconciliation failure.
- }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file convex/videoActions.ts around lines 600-605:
In `completeMultipartUpload`, the catch block unconditionally deletes the uploaded S3 object on any error from lines 591-599. When `reconcileUploadedObjectMetadata` or `clearMultipartUploadId` fail due to transient database issues, the valid uploaded file is incorrectly deleted. Use `shouldDeleteUploadedObjectOnFailure(error)` to gate deletion, matching the pattern at line 756 in `markUploadComplete`.
Evidence trail:
convex/videoActions.ts lines 564-614 (completeMultipartUpload try/catch block at REVIEWED_COMMIT): catch at line 600 unconditionally calls deleteUploadedObject at line 602. Lines 591-599 include database mutations that can fail transiently. convex/videoActions.ts lines 263-275 (shouldDeleteUploadedObjectOnFailure): gates deletion only for validation errors. convex/videoActions.ts lines 755-763 (markUploadComplete): uses shouldDeleteUploadedObjectOnFailure to conditionally delete, demonstrating the intended pattern.
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
1 participant
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
ListParts.Test plan
Made with Cursor
Note
Add S3 multipart upload support for video files up to 30 GiB
videoUpload.tsand coordinated via new server actions invideoActions.ts.uploadResumeDb.ts) is resumed by listing already-uploaded parts from S3.getUploadUrlaction now rejects files requiring multipart upload, which is a breaking change for any client still using that path for large files.Macroscope summarized 6352c41.
Summary by CodeRabbit
New Features
Improvements