fix: start processing after buffered MP4 recordings finish uploading#1911
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!video) throw new Error("Video not found"); | ||
| if (video.ownerId !== user.id) throw new Error("Unauthorized"); | ||
|
|
||
| const rawFileKey = `${video.ownerId}/${videoId}/result.mp4`; |
There was a problem hiding this comment.
rawFileKey from video.ownerId but video.ownerId is redundant here — user.id is already in scope and is guaranteed to equal video.ownerId after the ownership check. Using video.ownerId is consistent with the rest of the codebase (e.g. upload.ts line 277), but mixing both variables makes the source of truth less clear. Using user.id directly removes the dependency on the DB field and makes the equivalence self-evident.
| const rawFileKey = `${video.ownerId}/${videoId}/result.mp4`; | |
| const rawFileKey = `${user.id}/${videoId}/result.mp4`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/video/trigger-instant-recording-processing.ts
Line: 26
Comment:
The action re-derives `rawFileKey` from `video.ownerId` but `video.ownerId` is redundant here — `user.id` is already in scope and is guaranteed to equal `video.ownerId` after the ownership check. Using `video.ownerId` is consistent with the rest of the codebase (e.g. `upload.ts` line 277), but mixing both variables makes the source of truth less clear. Using `user.id` directly removes the dependency on the DB field and makes the equivalence self-evident.
```suggestion
const rawFileKey = `${user.id}/${videoId}/result.mp4`;
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@greptileai please re-review |
| await startVideoProcessingWorkflow({ | ||
| videoId, | ||
| userId: user.id, | ||
| rawFileKey, | ||
| bucketId: video.bucket ?? null, | ||
| processingMessage: "Starting video processing...", | ||
| startFailureMessage: "Video uploaded, but processing could not start.", | ||
| mode: "singlepart", | ||
| }); |
There was a problem hiding this comment.
processVideoWorkflow deletes result.mp4 after processing
startVideoProcessingWorkflow calls processVideoWorkflow, which finishes with cleanupRawUpload(videoId, rawFileKey) — an unconditional bucket.deleteObject(rawFileKey). For the streaming-webm path rawFileKey is raw-upload.webm, a distinct intermediary that should be cleaned up. Here rawFileKey = "${user.id}/${videoId}/result.mp4", which is also the hardcoded outputKey written by the media server (${userId}/${videoId}/result.mp4 in process-video.ts line 255). After the media server successfully writes the transcoded output to result.mp4, cleanupRawUpload immediately deletes that same key, leaving the video unplayable.
The existing admin reprocess path (adminReprocessVideoWorkflow) handles exactly this case — it calls the media server in the same way but has no cleanup step. Either route through that workflow, or guard the cleanup in processVideoWorkflow so it skips deletion when rawFileKey equals the output key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/video/trigger-instant-recording-processing.ts
Line: 28-36
Comment:
**`processVideoWorkflow` deletes `result.mp4` after processing**
`startVideoProcessingWorkflow` calls `processVideoWorkflow`, which finishes with `cleanupRawUpload(videoId, rawFileKey)` — an unconditional `bucket.deleteObject(rawFileKey)`. For the streaming-webm path `rawFileKey` is `raw-upload.webm`, a distinct intermediary that should be cleaned up. Here `rawFileKey = "${user.id}/${videoId}/result.mp4"`, which is also the hardcoded `outputKey` written by the media server (`${userId}/${videoId}/result.mp4` in `process-video.ts` line 255). After the media server successfully writes the transcoded output to `result.mp4`, `cleanupRawUpload` immediately deletes that same key, leaving the video unplayable.
The existing admin reprocess path (`adminReprocessVideoWorkflow`) handles exactly this case — it calls the media server in the same way but has no cleanup step. Either route through that workflow, or guard the cleanup in `processVideoWorkflow` so it skips deletion when `rawFileKey` equals the output key.
How can I resolve this? If you propose a fix, please make it concise.
|
@greptileai please re-review |
|
Hey @ManthanNimodiya, have you tested this locally fully? |
|
@richiemcilroy, Video processed fully both times and result.mp4 survived afterward whereas before the fix, it got deleted right after processing. |
|
/tip $40 |
|
Please visit Algora to complete your tip via Stripe. |
|
🎉🎈 @ManthanNimodiya has been awarded $40 by Cap! 🎈🎊 |
2 participants
Fixes #1902
Problem
The in-browser recorder has two upload pipelines: streaming-webm and buffered-raw.
The buffered-raw path never called anything to start server-side processing after the upload finished. As a result:
This reproduces on any browser using the MP4 fallback recorder (Safari/Firefox), which is what the reporter hit on their self-hosted instance.
Fix
Greptile Summary
Fixes the buffered-raw (MP4) recording path on Safari/Firefox, where server-side processing was never triggered after upload, leaving
videoUploads.phasestuck at "uploading" forever.triggerInstantRecordingProcessingserver action that transitions the upload phase to "processing" and startsprocessVideoWorkflowwithrawFileKey = result.mp4.useWebRecorder.tsimmediately after the buffered-raw upload completes, with a non-fatal toast on failure so thumbnail upload is unaffected.cleanupRawUploadinprocess-video.tsso it is skipped whenrawFileKeyequals the output key (result.mp4), preventing the final video file from being deleted after transcoding.Confidence Score: 5/5
Safe to merge. The three-part fix is coherent: the cleanup guard, the new server action, and the call site all work together correctly without introducing new failure modes.
The previously flagged deletion-of-result.mp4 bug is resolved by the rawFileKey !== outputKey guard. The new server action correctly threads through transitionVideoToProcessing (which updates rawFileKey in the DB before the workflow starts), so validateProcessingRequest's rawFileKey match check passes. The input/output sharing the same S3 key is safe because the media server fully downloads the source before writing output. Error handling in useWebRecorder.ts is non-fatal and consistent with the streaming path.
No files require special attention.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(process-video): skip raw upload clea..." | Re-trigger Greptile