Repository navigation
fix: restart Loom imports whose workflow run never started - #3123
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| runState === "inactive" && | ||
| age > ABANDONED_PROCESSING_AGE_MS; | ||
| ((runState === "inactive" && age > ABANDONED_PROCESSING_AGE_MS) || | ||
| (runState === "unconfirmed" && age >= FINISHED_WORKER_AGE_MS)); |
There was a problem hiding this comment.
This branch allows a restart after one hour, but the Loom importer allows four-hour jobs. Its heartbeat only updates the worker's local job, not videoUploads.updatedAt. If progress webhooks fail for an hour and the workflow status lookup fails, getLoomImportRunState returns unconfirmed and this branch starts another import. The importer accepts both jobs, which consume capacity and write to the same video files.
Check that the old worker has stopped, or stop it before restarting. One hour without a saved update is not enough.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/loom-import-redrive.ts
Line: 66
Comment:
**A live import starts twice**
This branch allows a restart after one hour, but the Loom importer allows four-hour jobs. Its heartbeat only updates the worker's local job, not `videoUploads.updatedAt`. If progress webhooks fail for an hour and the workflow status lookup fails, `getLoomImportRunState` returns `unconfirmed` and this branch starts another import. The importer accepts both jobs, which consume capacity and write to the same video files.
Check that the old worker has stopped, or stop it before restarting. One hour without a saved update is not enough.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| try { | ||
| const createdAt = await run.createdAt; | ||
| if (Date.now() - createdAt.getTime() < STUCK_PENDING_RUN_MS) return false; | ||
| await run.cancel(); |
There was a problem hiding this comment.
P2: New stuck-run cancellation trusts a workflow runId taken from mutable video metadata, allowing cross-video workflow cancellation
Added run.cancel() uses receipt.runId from client-writable video metadata, so any known pending run ID can be cancelled.
Bind the run ID to the video/owner server-side (protect loomImportRun from metadata writes) and verify ownership before cancelling.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/web/lib/loom-import-start.ts">
<violation number="1" location="apps/web/lib/loom-import-start.ts:66">
<priority>P2</priority>
<title>New stuck-run cancellation trusts a workflow runId taken from mutable video metadata, allowing cross-video workflow cancellation</title>
<evidence>The PR adds cancelStuckPendingRun, which calls `await run.cancel()` on `getRun(receipt.runId)` whenever the run's status is `pending` and it is older than one hour. `receipt` is supplied as `video.metadata?.loomImportRun` by callers such as `apps/web/actions/video/retry-processing.ts:85`, `apps/web/lib/loom-import-redrive.ts:62`, and the recovery jobs. The same checkout lets the owner of a video replace `videos.metadata` with an arbitrary object at `apps/web/app/api/video/metadata/route.ts:10` and `:38-55`; only the `agentUpload` field is protected. Therefore an authenticated user can put an arbitrary `loomImportRun.runId` into the metadata of a Loom-imported video they own and have `getLoomImportRunState` cancel that run. The cancellation path does not verify that the run belongs to the video, the caller, or even the Loom importer. Workflow run IDs are not treated as secrets here: `apps/web/app/s/[videoId]/page.tsx:344` selects `videos.metadata` and `toShareVideo` forwards it unchanged to the public share client, so a run ID from a pending public Loom import is exposed in the share page payload. Before this PR the same metadata value was only used to read `getRun(receipt.runId).status`; there was no mutation of the referenced run. This is a newly introduced cross-tenant cancellation capability against pending workflow runs (including runs that are legitimately queued for more than an hour), not merely a missing hardening.</evidence>
<recommendation>Do not trust a run ID read back from client-writable metadata for cancellation. Store the run ID server-side in a field that the metadata endpoint cannot overwrite (or preserve/reject `loomImportRun` there like `agentUpload`), and before cancelling verify that the run was created for this video/owner and is a Loom-import run. Prefer moving the stuck-run cancellation into a trusted recovery routine that selects the run ID from the server-written receipt and confirms ownership. Add a regression test where metadata contains another video's pending run ID and assert that run is not cancelled.</recommendation>
</violation>
</file>
Found while restarting one of Plans' imports after #3118. Its workflow run was created at 11:31 and was still
pendingmore than 5 hours later. It never started, but the restart check treatedpendingas running, so the import could never be restarted.getLoomImportRunState: a run stillpendingan hour after it was created is cancelled and then counted as inactive. If the cancel fails, or doesn't take, the run stayspending. Runs queued less than an hour ago are left alone.processingimports whose run is unconfirmed, because the importer allows 4-hour jobs and its heartbeat doesn't update the saved row (from review).Tests: loom-import-start 33 (3 new), loom-importer-redrive 28 (1 new hold test), video-pipeline-recovery 12, loom-import 54, operator-tools 31 pass. The share-stage-identity failures also fail on main.


Confidence Score: 5/5
The PR appears safe to merge; the previous duplicate-start issue is fixed.Summary
Allows Loom imports to restart when their workflow never started.
Reviews (3) · Last reviewed commit: "fix: keep unconfirmed Loom imports held ..." · Reviewed by Greptile