Skip to content

fix: restart Loom imports whose workflow run never started #3123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
richiemcilroy merged 2 commits into from
Oct 24, 2026
Merged

fix: restart Loom imports whose workflow run never started #3123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2033ddf
fix: cancel Loom import runs stuck pending and restart abandoned unco…
richiemcilroy Oct 24, 2026
82e1a2b
fix: keep unconfirmed Loom imports held while their worker may still run
richiemcilroy Oct 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions apps/web/__tests__/unit/loom-import-start.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,45 @@ describe("Loom workflow startup receipts", () => {
},
);

it("cancels a run that never left pending and treats it as inactive", async () => {
const run = {
status: Promise.resolve("pending"),
createdAt: Promise.resolve(new Date(Date.now() - 2 * 60 * 60 * 1000)),
cancel: vi.fn(async () => {
run.status = Promise.resolve("cancelled");
}),
};
mocks.getRun.mockReturnValue(run);
expect(
await getLoomImportRunState({ runId: "run-1", dispatch: "uncertain" }),
).toBe("inactive");
expect(run.cancel).toHaveBeenCalledTimes(1);
});

it("leaves a recently queued run pending", async () => {
const cancel = vi.fn();
mocks.getRun.mockReturnValue({
status: Promise.resolve("pending"),
createdAt: Promise.resolve(new Date(Date.now() - 10 * 60 * 1000)),
cancel,
});
expect(
await getLoomImportRunState({ runId: "run-1", dispatch: "uncertain" }),
).toBe("pending");
expect(cancel).not.toHaveBeenCalled();
});

it("keeps an old pending run pending when it cannot be cancelled", async () => {
mocks.getRun.mockReturnValue({
status: Promise.resolve("pending"),
createdAt: Promise.resolve(new Date(Date.now() - 2 * 60 * 60 * 1000)),
cancel: vi.fn().mockRejectedValue(new Error("Unavailable")),
});
expect(
await getLoomImportRunState({ runId: "run-1", dispatch: "uncertain" }),
).toBe("pending");
});

it("holds startup while the run cannot be queried", async () => {
mocks.getRun.mockReturnValue({
status: Promise.reject(new Error("Not found yet")),
Expand Down
12 changes: 12 additions & 0 deletions apps/web/__tests__/unit/loom-importer-redrive.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,18 @@ describe("redriveLoomImport", () => {
});
});

it("leaves an unconfirmed import processing because its worker may still be running", async () => {
mocks.runState.mockResolvedValue("unconfirmed");
mocks.rows = [
failedImport({ phase: "processing", uploadUpdatedAt: hoursAgo(3) }),
];
await expect(redriveLoomImport("video", now)).resolves.toMatchObject({
status: "skipped",
reason: "phase processing",
});
expect(mocks.start).not.toHaveBeenCalled();
});

it("does not start a workflow when another restart claimed the row", async () => {
mocks.affectedRows = 0;
await expect(redriveLoomImport("video", now)).resolves.toMatchObject({
Expand Down
18 changes: 17 additions & 1 deletion apps/web/lib/loom-import-start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,29 @@ export class LoomImportStartError extends Error {
export const LOOM_IMPORT_CONFIRMATION_MESSAGE =
"We're confirming whether your Loom import started. Please contact support before retrying.";

const STUCK_PENDING_RUN_MS = 60 * 60 * 1000;

async function cancelStuckPendingRun(run: ReturnType<typeof getRun>) {
try {
const createdAt = await run.createdAt;
if (Date.now() - createdAt.getTime() < STUCK_PENDING_RUN_MS) return false;
await run.cancel();

@superagent-security superagent-security Bot Oct 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

return (await run.status) === "cancelled";
} catch {
return false;
}
}

export async function getLoomImportRunState(
receipt?: LoomImportRun,
): Promise<"inactive" | "pending" | "active" | "unconfirmed"> {
if (!receipt || receipt.dispatch === "rejected") return "inactive";
const run = getRun(receipt.runId);
let status: string;
try {
status = await getRun(receipt.runId).status;
status = await run.status;
if (status === "pending" && (await cancelStuckPendingRun(run)))
status = "cancelled";
} catch (error) {
return WorkflowRunNotFoundError.is(error) &&
receipt.dispatch === "uncertain" &&
Expand Down
Loading