Repository navigation
Mark items failed only once no retry is left - #706
Merged
Merged
Conversation
classify-url, analyze-image, enrich-item, analyze-media-cover, analyze-document and import-pdf set an item failed in their catch and rethrew for Trigger's retry. A first-attempt failure showed a red card while the retry was queued, the grid poll (processing items only) missed that retry's success, and Retry could start a second paid run beside it. Failures no retry can fix (a 403/404 source) were fetched and reported twice. The six tasks now spread itemFailureHooks: onFailure reports and marks the item failed after the last attempt, and catchError skips retrying a permanent failure (isPermanentFailure). import-pdf fires its pdf_import_failed funnel event from onFailure too, and its unreadable/too-long PDFs stop retrying through catchError instead of AbortTaskRunError. analyze-document still drops a run whose PDF was replaced mid-analysis. The trigger AGENTS.md convention and the add-trigger-task skill describe the new pattern. Amp-Thread-ID: https://ampcode.com/threads/T-01a1daa2-36bf-7085-8fc3-5fd53d723953
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
6 tasks done
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Merges fredrivett/single-cover-enrichment (analyze-image's text fallback once its cover has failed for good), keeping that fallback alongside the shared failure hooks: analyze-image reports the error and enriches from the item's text instead of marking it failed when the run carried item text. From cubic's review of #706: - itemFailureHooks always leaves a completed item as it is. User-initiated runs set the item to processing first, so a completed item means a later stage or a background re-run (admin reprocess, cover swap) failed; the keepCompleted option is gone. - Failure reports carry the payload's url/fileKey again. - isPermanentFailure treats every SafeFetchError (redirect cap, invalid redirect, oversized body) as permanent. - import-pdf's upload-failure test checks catchError on the error run threw. Amp-Thread-ID: https://ampcode.com/threads/T-01a1daa2-36bf-7085-8fc3-5fd53d723953
…-last-attempt # Conflicts: # app/trigger/analyze-image.integration.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Six item-processing tasks (
classify-url,analyze-image,enrich-item,analyze-media-cover,analyze-document,import-pdf) setprocessingStatus: "failed"in their catch and rethrew so Trigger.dev would retry, which was the documented convention. Withretry: { maxAttempts: 2 }that meant:processingitems, a successful retry stayed hidden until reload.claimFailedRetryand started a second paid run beside Trigger's own retry, both writing the same item.Now each of those tasks spreads
itemFailureHooks(app/trigger/item-failure.ts):onFailure, which Trigger calls only once no attempt is left, reports the error and marks the item failed. Until then the item staysprocessing, so the grid keeps polling and Retry stays closed. A completed item is always kept as it is: user-initiated runs (Retry, reassign) set the item toprocessingbefore they enqueue, so a completed item means a later stage or a background re-run (admin reprocess, cover swap) failed, and its earlier result stands.analyze-imagekeeps Enrich books and products once, from their text plus the cover #705's fallback: when the run carried a book or product's text,onFailurereports and enriches from that text instead of marking the item failed.catchErrorreturnsskipRetryingfor failuresisPermanentFailurerecognises: anySafeFetchError(SSRF gate, invalid or too many redirects, oversized body), plussource_blocked(401/403/429),source_not_found,unsupported_content,file_unreadable,document_too_long. Trigger still passes the original error toonFailure, so the stored reason is unchanged. This replacesimport-pdf'sAbortTaskRunError.analyze-documentstill drops a run whose PDF was replaced mid-analysis;import-pdffirespdf_import_failedfromonFailure, so once per import rather than once per attempt.app/trigger/AGENTS.mdand theadd-trigger-taskskill describe the new pattern.Runs that never reach
onFailurePer the Trigger.dev v4 docs,
onFailureisn't called for a run stopped atmaxDuration, crashed, cancelled, or expired in the queue (TTL); an out-of-memory kill ends the process, so no hook runs in that attempt. The previous catch didn't run in those cases either, so the stuck-items reaper (reap-stuck-items) was already what marked themstalled, and still is. Items now stayprocessingthrough the retry gap (seconds), each attempt advancesprocessingStartedAt, and the 4h threshold stays well above the 2h TTL plus the longestmaxDuration.Checklist
pdf_import_failednow fires once per failed import (after the last attempt) instead of once per attempt.env.example, in-app help,CONTRIBUTING.md, onboarding checklist), or N/A —app/trigger/AGENTS.mdconvention and theadd-trigger-taskskill updated; nothing in the user docs changescaptureServerException/ the error boundary, or N/A — failures report viacaptureServerExceptionfromonFailure, with{ task, itemId }plus the payload'surl/fileKeywhen presentisPermanentFailure,classify-url,analyze-documentandimport-pdf; real-Postgres integration tests foranalyze-imageand the PDF import (an attempt leaves the itemprocessing; the final failure marks it failed with its reason; a completed item is never regressed; permanent failures skip the retry)record-pr-demoskill); refactor through a user flow → parity run, or N/A — N/A: retries happen only in Trigger.dev, which isn't configured in the demo stack, so the "Failed during a pending retry" state can't be reproduced therelearnskill, or N/A — the convention itself was the cause; it's replaced intrigger/AGENTS.mdand the skill template, with one shared hook so new item tasks get it by spreading itNotes
bun run check:fixclean; the full unit suite and thetrigger/integration tests pass. I read SDK 4.5.16's task executor to confirmonFailureruns only when the attempt isn't retried, and receives the original error whencatchErrorskips retrying.catchErrorandonFailurewere exercised by calling the hooks directly, not end to end.source_blockedand so is no longer retried; Trigger's retry comes seconds later, well inside any rate-limit window.Summary by cubic
Six item-processing tasks (
classify-url,analyze-image,enrich-item,analyze-media-cover,analyze-document,import-pdf) now mark an item failed only after Trigger.dev's last retry attempt, instead of on the first catch. Marking early showed a red "Failed" card while a retry was queued, hid that retry's success from the grid poll (which only followsprocessingitems), and let Retry start a second paid run beside Trigger's own.itemFailureHooks(app/trigger/item-failure.ts):onFailurereports (with the payload'surl/fileKey) and marks the item failed once no attempt is left, keeping a completed item as it is;catchErrorskips retrying failures that can't succeed, treating everySafeFetchError(SSRF gate, redirect cap, oversized body) as permanent alongside a 403/404 source or unusable content.analyze-imagestill enriches from the item's text instead of marking it failed when the run carried book/product text, so the item completes via its text.import-pdf'spdf_import_failedevent fires once per failed import fromonFailure, and itsAbortTaskRunErrorshortcut was replaced bycatchError.maxDuration, crashed, cancelled or expired in the queue; the stuck-items reaper still marks those itemsstalled.app/trigger/AGENTS.mdand theadd-trigger-taskskill.Written for commit 13242d4. Summary will update on new commits.