Web video trimming and cuts#1820
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| }); | ||
| const currentJob = getJob(jobId); | ||
| if (currentJob) { | ||
| sendWebhook(currentJob); |
There was a problem hiding this comment.
sendWebhook(currentJob) is called inside onProgress without being awaited or error-handled; if it rejects this can surface as an unhandled rejection.
| sendWebhook(currentJob); | |
| void sendWebhook(currentJob).catch((error) => | |
| console.warn(`[video/edit] Failed to send webhook update for job ${jobId}:`, error), | |
| ); |
| const body = await c.req.json(); | ||
| const result = editSchema.safeParse(body); |
There was a problem hiding this comment.
c.req.json() can throw on invalid JSON and currently becomes a 500. Returning INVALID_REQUEST here would make retries / debugging nicer.
| const body = await c.req.json(); | |
| const result = editSchema.safeParse(body); | |
| let body: unknown; | |
| try { | |
| body = await c.req.json(); | |
| } catch { | |
| return c.json( | |
| { error: "Invalid request", code: "INVALID_REQUEST" }, | |
| 400, | |
| ); | |
| } | |
| const result = editSchema.safeParse(body); |
| .set({ | ||
| phase: "complete", | ||
| processingProgress: 100, | ||
| processingMessage: payload.message, | ||
| updatedAt: new Date(), | ||
| }) |
There was a problem hiding this comment.
On successful completion for edit uploads, it might be worth explicitly clearing processingError so a previous failure doesn’t linger in the UI.
| .set({ | |
| phase: "complete", | |
| processingProgress: 100, | |
| processingMessage: payload.message, | |
| updatedAt: new Date(), | |
| }) | |
| .set({ | |
| phase: "complete", | |
| processingProgress: 100, | |
| processingMessage: payload.message, | |
| processingError: null, | |
| updatedAt: new Date(), | |
| }) |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const SECOND = 1000; | ||
| const MINUTE = 60 * SECOND; | ||
| const STALE_EDIT_PROCESSING_START_MS = 2 * MINUTE; | ||
| const STALE_EDIT_PROCESSING_PROGRESS_MS = 10 * MINUTE; |
There was a problem hiding this comment.
STALE_EDIT_PROCESSING_START_MS (2 min) governs when a processingProgress === 0 record is treated as stuck. During the media server's download phase, no granular progress webhook is fired — only a single initial webhook with the stale initial job state (progress 0). For a large video (e.g. 1 GB+ MP4), download from S3 to the media server can easily take 3–5+ minutes. If the owner (or anyone else) loads /s/[videoId] or the edit page in a second tab during that window, reconcileStaleEditUpload runs, deletes the videoUploads row, and the workflow step waitForEditCompletion throws "Edit processing state disappeared" — aborting the edit silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/video-edit-processing.ts
Line: 9
Comment:
**2-minute stale threshold too short for the download phase**
`STALE_EDIT_PROCESSING_START_MS` (2 min) governs when a `processingProgress === 0` record is treated as stuck. During the media server's download phase, no granular progress webhook is fired — only a single initial webhook with the stale initial job state (progress 0). For a large video (e.g. 1 GB+ MP4), download from S3 to the media server can easily take 3–5+ minutes. If the owner (or anyone else) loads `/s/[videoId]` or the edit page in a second tab during that window, `reconcileStaleEditUpload` runs, deletes the `videoUploads` row, and the workflow step `waitForEditCompletion` throws "Edit processing state disappeared" — aborting the edit silently.
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| updateJob(jobId, { | ||
| phase: "downloading", | ||
| progress: 0, | ||
| message: "Downloading source video...", | ||
| }); | ||
| await sendWebhook(job); | ||
|
|
||
| const inputTempFile = await downloadVideoToTemp( | ||
| sourceUrl, | ||
| ".mp4", | ||
| abortController.signal, | ||
| ); | ||
| updateJob(jobId, { inputTempFile }); | ||
|
|
||
| updateJob(jobId, { | ||
| phase: "probing", | ||
| progress: 5, | ||
| message: "Analyzing source video...", | ||
| }); | ||
| await sendWebhook(job); | ||
|
|
||
| const sourceMetadata = await probeVideoFile(inputTempFile.path); | ||
|
|
||
| updateJob(jobId, { | ||
| phase: "processing", | ||
| progress: 10, | ||
| message: "Applying edit...", | ||
| }); | ||
| await sendWebhook(job); |
There was a problem hiding this comment.
job reference passed to the first three sendWebhook calls
job is captured once via getJob(jobId) at the top of editVideoAsync. Every subsequent updateJob(jobId, ...) call modifies the store entry, but job still points to the pre-update snapshot. The "downloading", "probing", and "processing" webhooks therefore forward the original (queued) state to the web app instead of the updated phases. The success path already uses getJob(jobId) to get fresh state before calling sendWebhook — the early calls should follow the same pattern.
| try { | |
| updateJob(jobId, { | |
| phase: "downloading", | |
| progress: 0, | |
| message: "Downloading source video...", | |
| }); | |
| await sendWebhook(job); | |
| const inputTempFile = await downloadVideoToTemp( | |
| sourceUrl, | |
| ".mp4", | |
| abortController.signal, | |
| ); | |
| updateJob(jobId, { inputTempFile }); | |
| updateJob(jobId, { | |
| phase: "probing", | |
| progress: 5, | |
| message: "Analyzing source video...", | |
| }); | |
| await sendWebhook(job); | |
| const sourceMetadata = await probeVideoFile(inputTempFile.path); | |
| updateJob(jobId, { | |
| phase: "processing", | |
| progress: 10, | |
| message: "Applying edit...", | |
| }); | |
| await sendWebhook(job); | |
| try { | |
| updateJob(jobId, { | |
| phase: "downloading", | |
| progress: 0, | |
| message: "Downloading source video...", | |
| }); | |
| const downloadingJob = getJob(jobId); | |
| if (downloadingJob) await sendWebhook(downloadingJob); | |
| const inputTempFile = await downloadVideoToTemp( | |
| sourceUrl, | |
| ".mp4", | |
| abortController.signal, | |
| ); | |
| updateJob(jobId, { inputTempFile }); | |
| updateJob(jobId, { | |
| phase: "probing", | |
| progress: 5, | |
| message: "Analyzing source video...", | |
| }); | |
| const probingJob = getJob(jobId); | |
| if (probingJob) await sendWebhook(probingJob); | |
| const sourceMetadata = await probeVideoFile(inputTempFile.path); | |
| updateJob(jobId, { | |
| phase: "processing", | |
| progress: 10, | |
| message: "Applying edit...", | |
| }); | |
| const processingJob = getJob(jobId); | |
| if (processingJob) await sendWebhook(processingJob); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/routes/video.ts
Line: 867-896
Comment:
**Stale `job` reference passed to the first three `sendWebhook` calls**
`job` is captured once via `getJob(jobId)` at the top of `editVideoAsync`. Every subsequent `updateJob(jobId, ...)` call modifies the store entry, but `job` still points to the pre-update snapshot. The "downloading", "probing", and "processing" webhooks therefore forward the original (queued) state to the web app instead of the updated phases. The success path already uses `getJob(jobId)` to get fresh state before calling `sendWebhook` — the early calls should follow the same pattern.
```suggestion
try {
updateJob(jobId, {
phase: "downloading",
progress: 0,
message: "Downloading source video...",
});
const downloadingJob = getJob(jobId);
if (downloadingJob) await sendWebhook(downloadingJob);
const inputTempFile = await downloadVideoToTemp(
sourceUrl,
".mp4",
abortController.signal,
);
updateJob(jobId, { inputTempFile });
updateJob(jobId, {
phase: "probing",
progress: 5,
message: "Analyzing source video...",
});
const probingJob = getJob(jobId);
if (probingJob) await sendWebhook(probingJob);
const sourceMetadata = await probeVideoFile(inputTempFile.path);
updateJob(jobId, {
phase: "processing",
progress: 10,
message: "Applying edit...",
});
const processingJob = getJob(jobId);
if (processingJob) await sendWebhook(processingJob);
```
How can I resolve this? If you propose a fix, please make it concise.| const currentJob = getJob(jobId); | ||
| if (currentJob) { | ||
| await currentJob.inputTempFile?.cleanup(); | ||
| await currentJob.outputTempFile?.cleanup(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function processVideoAsync( |
There was a problem hiding this comment.
editVideoAsync never removes the job from the registry
The success path schedules setTimeout(() => deleteJob(jobId), 5 * 60 * 1000), but the error path exits without scheduling any cleanup. Failed edit jobs remain in the in-memory job registry indefinitely, accumulating until the process restarts. On a long-running server with many transient failures this becomes an unbounded memory leak.
| const currentJob = getJob(jobId); | |
| if (currentJob) { | |
| await currentJob.inputTempFile?.cleanup(); | |
| await currentJob.outputTempFile?.cleanup(); | |
| } | |
| } | |
| } | |
| async function processVideoAsync( | |
| const currentJob = getJob(jobId); | |
| if (currentJob) { | |
| await currentJob.inputTempFile?.cleanup(); | |
| await currentJob.outputTempFile?.cleanup(); | |
| } | |
| setTimeout(() => deleteJob(jobId), 5 * 60 * 1000); | |
| } | |
| } | |
| async function processVideoAsync( |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/routes/video.ts
Line: 982-990
Comment:
**Error path of `editVideoAsync` never removes the job from the registry**
The success path schedules `setTimeout(() => deleteJob(jobId), 5 * 60 * 1000)`, but the error path exits without scheduling any cleanup. Failed edit jobs remain in the in-memory job registry indefinitely, accumulating until the process restarts. On a long-running server with many transient failures this becomes an unbounded memory leak.
```suggestion
const currentJob = getJob(jobId);
if (currentJob) {
await currentJob.inputTempFile?.cleanup();
await currentJob.outputTempFile?.cleanup();
}
setTimeout(() => deleteJob(jobId), 5 * 60 * 1000);
}
}
async function processVideoAsync(
```
How can I resolve this? If you propose a fix, please make it concise.| const isMp4Source = | ||
| data.source.type === "desktopMP4" || data.source.type === "webMP4"; | ||
| const isSegmentsSource = data.source.type === "desktopSegments"; |
There was a problem hiding this comment.
The string "${data.owner.id}/${data.id}/source/original.mp4" duplicates the logic from getEditSourceKey in video-edit-processing.ts. Since that module is server-only, a clean alternative is to compute the boolean on the server (in page.tsx) and forward it as a typed prop. If the S3 key format ever changes, this comparison will silently stop working.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/s/[videoId]/_components/ShareVideo.tsx
Line: 194-196
Comment:
**Source-key format duplicated as a string literal**
The string `"${data.owner.id}/${data.id}/source/original.mp4"` duplicates the logic from `getEditSourceKey` in `video-edit-processing.ts`. Since that module is server-only, a clean alternative is to compute the boolean on the server (in `page.tsx`) and forward it as a typed prop. If the S3 key format ever changes, this comparison will silently stop working.
How can I resolve this? If you propose a fix, please make it concise.
|
please re-review the pr @greptileai |
| .delete(videoUploads) | ||
| .where(eq(videoUploads.videoId, videoId as Video.VideoId)); |
There was a problem hiding this comment.
tx.delete(videoUploads).where(eq(videoUploads.videoId, videoId)) uses only videoId. A second saveVideoEdits call can arrive while videoUploads.phase === "complete" (after the media-server webhook marks it done but before this transaction runs). ACTIVE_UPLOAD_PHASES does not include "complete", so the guard in save-edits.ts passes, markEditProcessing upserts the row with phase = "processing", and then this transaction deletes that freshly-written row. The second workflow then fails immediately in validateEditRequest with "Edit render does not exist".
Adding a phase guard — e.g. and(eq(videoUploads.videoId, videoId), eq(videoUploads.phase, "complete")) — would make the delete a no-op when a new edit has already taken ownership of the row.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/workflows/edit-video.ts
Line: 483-484
Comment:
**Unconditional delete can clobber a concurrently-submitted edit's processing state**
`tx.delete(videoUploads).where(eq(videoUploads.videoId, videoId))` uses only `videoId`. A second `saveVideoEdits` call can arrive while `videoUploads.phase === "complete"` (after the media-server webhook marks it done but before this transaction runs). `ACTIVE_UPLOAD_PHASES` does not include `"complete"`, so the guard in `save-edits.ts` passes, `markEditProcessing` upserts the row with `phase = "processing"`, and then this transaction deletes that freshly-written row. The second workflow then fails immediately in `validateEditRequest` with "Edit render does not exist".
Adding a phase guard — e.g. `and(eq(videoUploads.videoId, videoId), eq(videoUploads.phase, "complete"))` — would make the delete a no-op when a new edit has already taken ownership of the row.
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai please re-review the PR. addressed your latest claim |
| .where(eq(videoUploads.videoId, videoId)); | ||
|
|
||
| if (activeUpload && ACTIVE_UPLOAD_PHASES.has(activeUpload.phase)) { | ||
| throw new Error("Video is already uploading or processing"); |
There was a problem hiding this comment.
This now blocks on phase === "complete" too, but the thrown message reads like it’s still uploading/processing. Consider tailoring the error for the complete case so it’s clearer to the user what’s happening.
| throw new Error("Video is already uploading or processing"); | |
| if (activeUpload && ACTIVE_UPLOAD_PHASES.has(activeUpload.phase)) { | |
| const message = | |
| activeUpload.phase === "complete" | |
| ? "Previous edit is finishing up. Try again in a moment." | |
| : "Video is already uploading or processing"; | |
| throw new Error(message); | |
| } |
| async function clearEditProcessingState(videoId: string): Promise<void> { | ||
| "use step"; | ||
|
|
||
| await db() | ||
| .delete(videoUploads) | ||
| .where(eq(videoUploads.videoId, videoId as Video.VideoId)); | ||
| } |
There was a problem hiding this comment.
clearEditProcessingState deletes by videoId alone. The success path in saveEditResultAndComplete was hardened to also match phase = "complete" and rawFileKey against the workflow's sourceKey, but the error path was not updated.
Race: Edit A's media server job fails, the webhook sets phase = "error". Since "error" is absent from ACTIVE_UPLOAD_PHASES, Edit B can be submitted immediately — markEditProcessing upserts the row with phase = "processing" before Edit A's five-second poller fires. If the poller reads the error state first and throws, the catch block then calls clearEditProcessingState, which deletes Edit B's freshly-written row. Edit B's validateEditRequest then throws "Edit render does not exist".
Scoping the DELETE to also match rawFileKey against the sourceKey already in scope would make it a no-op when a newer edit has taken ownership of the row — consistent with the guard already added to the success path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/workflows/edit-video.ts
Line: 503-509
Comment:
**Unconditional delete in error-path cleanup not guarded by source key**
`clearEditProcessingState` deletes by `videoId` alone. The success path in `saveEditResultAndComplete` was hardened to also match `phase = "complete"` and `rawFileKey` against the workflow's `sourceKey`, but the error path was not updated.
Race: Edit A's media server job fails, the webhook sets `phase = "error"`. Since `"error"` is absent from `ACTIVE_UPLOAD_PHASES`, Edit B can be submitted immediately — `markEditProcessing` upserts the row with `phase = "processing"` before Edit A's five-second poller fires. If the poller reads the error state first and throws, the catch block then calls `clearEditProcessingState`, which deletes Edit B's freshly-written row. Edit B's `validateEditRequest` then throws `"Edit render does not exist"`.
Scoping the DELETE to also match `rawFileKey` against the `sourceKey` already in scope would make it a no-op when a newer edit has taken ownership of the row — consistent with the guard already added to the success path.
How can I resolve this? If you propose a fix, please make it concise.
| .where( | ||
| and( | ||
| eq(videoUploads.videoId, videoId as Video.VideoId), | ||
| eq(videoUploads.rawFileKey, sourceKey), |
There was a problem hiding this comment.
Scoping by rawFileKey helps, but since sourceKey looks stable per-video (getEditSourceKey(...)), this can still delete a newer edit row (phase processing) during error cleanup. Mirroring the success-path phase guard here would make the delete a no-op once a new edit starts.
| eq(videoUploads.rawFileKey, sourceKey), | |
| .where( | |
| and( | |
| eq(videoUploads.videoId, videoId as Video.VideoId), | |
| eq(videoUploads.rawFileKey, sourceKey), | |
| eq(videoUploads.phase, "error"), | |
| ), | |
| ); |
1 participant
adds a web video editor for MP4-backed caps: trim, split, delete segments, and save. Edits are stored as a normalized keep-ranges spec in a new video_edits table, with optional local draft recovery in the browser.
Greptile Summary
This PR adds a web-based video editor for MP4-backed caps, enabling trim, split, segment-delete, and save operations. Edits are stored as a normalized keep-ranges spec in a new
video_editstable and rendered server-side by a new/video/editmedia-server endpoint via aneditVideoWorkflowworkflow that polls for completion.editVideoAsyncreads a fresh job snapshot viagetJob, the error path now schedules adeleteJobcleanup in afinallyblock, and the success-pathsaveEditResultAndCompletedelete is guarded by bothphase = \"complete\"andrawFileKey.clearEditProcessingState(the error-path cleanup) still deletes byvideoIdalone, without the samerawFileKeyguard added to the success path, leaving a narrow race window where a concurrently-submitted second edit's row can be deleted before its workflow begins.Confidence Score: 4/5
Safe to merge with one small fix: the error-path cleanup in the workflow can delete a concurrent edit's database row in a narrow timing window.
The four issues flagged in the prior round are cleanly resolved. One gap in the error-path cleanup — clearEditProcessingState deletes without a source-key guard, unlike the hardened success path — can cause a concurrently-submitted second edit to lose its processing state and fail silently. The fix is a one-line WHERE-clause addition.
apps/web/workflows/edit-video.ts — specifically the clearEditProcessingState function's DELETE statement
Important Files Changed
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(web): guard edit upload cleanup" | Re-trigger Greptile