Repository navigation
fix: faster, cheaper Loom imports: fewer progress webhooks and real restart checks - #3118
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@greptileai review |
| const now = Date.now(); | ||
| const last = lastProgressWebhook.get(jobId); | ||
| if ( | ||
| last && | ||
| last.phase === phase && | ||
| now - last.at < PROGRESS_WEBHOOK_INTERVAL_MS | ||
| ) | ||
| return; | ||
| lastProgressWebhook.set(jobId, { at: now, phase }); | ||
| void sendWebhook(job); |
There was a problem hiding this comment.
The new 15-second limit has no test coverage. Existing importer tests check completion, and the changed API test checks queued jobs, but neither checks these timing rules. Add fake-clock tests for same-phase suppression, sending again after 15 seconds, separate jobs, and immediate phase and terminal updates. This would protect the cost savings without risking missing status updates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/loom-importer/job.ts
Line: 154-163
Comment:
**Webhook timing lacks tests**
The new 15-second limit has no test coverage. Existing importer tests check completion, and the changed API test checks queued jobs, but neither checks these timing rules. Add fake-clock tests for same-phase suppression, sending again after 15 seconds, separate jobs, and immediate phase and terminal updates. This would protect the cost savings without risking missing status updates.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
|
|
||
| const FINISHED_WORKER_AGE_MS = 6 * 60 * 60 * 1000; | ||
| const ABANDONED_PROCESSING_AGE_MS = 10 * 60 * 1000; | ||
| const FINISHED_WORKER_AGE_MS = 60 * 60 * 1000; |
There was a problem hiding this comment.
One-hour fallback can restart a still-running import
Medium Severity
If a workflow fails after dispatch and worker checks are inconclusive, an error row just over one hour old now permits another import. That does not establish that the original worker has stopped: importer jobs explicitly receive a four-hour lifetime, and media jobs also extend their deadlines up to four hours. Lost dispatch acknowledgements or unavailable progress/status endpoints can therefore leave a live worker behind this branch. Redrive does not cancel it before replacing the run and multipart-upload receipts, so the original attempt loses upload ownership while duplicate work starts. Retain a fallback covering the actual worker lifetime, or require confirmed termination/cancellation before restarting an unconfirmed dispatch.
Two inefficiencies in the Loom import pipeline:
flowchart LR R[restart tool] --> G{workflow run} G -->|not found and start uncertain| I[inactive] G -->|running| A[active] G -->|finished| J{importer job} J -->|running| A J -->|finished, or missing for importer dispatches| I J -->|unknown| M[legacy media server check, 1h fallback]loom-importer/job.ts: progress webhooks are sent at most once every 15 seconds per job and phase. Phase changes and terminal webhooks still go out immediately./video/statusnow lists queued jobs as well as running ones.recordLoomMediaDispatchstores which worker accepted the job (importerormedia).getLoomImportRunState: a workflow run that doesn't exist (WorkflowRunNotFoundError) after an uncertain start is inactive. When the run is finished, the importer is asked directly about its job. A running job means active, and a finished job means inactive. A missing job counts as inactive only when the importer took the dispatch, since the media server fleet is load balanced and a 404 there proves nothing. Anything else falls back to the existing media server check.redriveLoomImport: an import stuck inprocessingwith no live run for over 10 minutes can be restarted. The fallback hold for truly unconfirmed workers drops from 6 hours to 1 hour.

Confidence Score: 5/5
The PR appears safe to merge; no new actionable issues were found.Summary
This PR reduces Loom progress webhooks and improves checks before restarting imports.
Reviews (3) · Last reviewed commit: "test: cover Loom importer progress webho..." · Reviewed by Greptile