Skip to content

Mark items failed only once no retry is left - #706

Merged
fredrivett merged 5 commits into
mainfrom
fredrivett/mark-failed-after-last-attempt
Nov 15, 2026
Merged

fredrivett merged 5 commits into
mainfrom
fredrivett/mark-failed-after-last-attempt

Conversation

@fredrivett

@fredrivett fredrivett commented Nov 14, 2026 •

Copy link
Copy Markdown
Owner

Stacked on #705 (fredrivett/single-cover-enrichment); review that first. This PR's own diff is the second commit.

What & why

Six item-processing tasks (classify-url, analyze-image, enrich-item, analyze-media-cover, analyze-document, import-pdf) set processingStatus: "failed" in their catch and rethrew so Trigger.dev would retry, which was the documented convention. With retry: { maxAttempts: 2 } that meant:

  • A first-attempt failure showed a red "Failed" card while Trigger's retry was still queued, and since the grid only polls processing items, a successful retry stayed hidden until reload.
  • Clicking Retry in that window passed claimFailedRetry and started a second paid run beside Trigger's own retry, both writing the same item.
  • Failures no retry can fix (a 403/404 from the source, unusable content) were fetched and reported twice.

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 stays processing, 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 to processing before 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-image keeps Enrich books and products once, from their text plus the cover #705's fallback: when the run carried a book or product's text, onFailure reports and enriches from that text instead of marking the item failed.
  • catchError returns skipRetrying for failures isPermanentFailure recognises: any SafeFetchError (SSRF gate, invalid or too many redirects, oversized body), plus source_blocked (401/403/429), source_not_found, unsupported_content, file_unreadable, document_too_long. Trigger still passes the original error to onFailure, so the stored reason is unchanged. This replaces import-pdf's AbortTaskRunError.
  • analyze-document still drops a run whose PDF was replaced mid-analysis; import-pdf fires pdf_import_failed from onFailure, so once per import rather than once per attempt.
  • The convention in app/trigger/AGENTS.md and the add-trigger-task skill describe the new pattern.

Runs that never reach onFailure

Per the Trigger.dev v4 docs, onFailure isn't called for a run stopped at maxDuration, 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 them stalled, and still is. Items now stay processing through the retry gap (seconds), each attempt advances processingStartedAt, and the 4h threshold stays well above the 2h TTL plus the longest maxDuration.

Checklist

  • User-facing change → added/updated a PostHog event (and a funnel/insight for a new flow), or N/A — no new flow; pdf_import_failed now fires once per failed import (after the last attempt) instead of once per attempt
  • User-facing or service change → docs updated (README features/roadmap/services, .env.example, in-app help, CONTRIBUTING.md, onboarding checklist), or N/A — app/trigger/AGENTS.md convention and the add-trigger-task skill updated; nothing in the user docs changes
  • New error paths report via captureServerException / the error boundary, or N/A — failures report via captureServerException from onFailure, with { task, itemId } plus the payload's url / fileKey when present
  • New behavior is covered by tests, or N/A — unit tests for isPermanentFailure, classify-url, analyze-document and import-pdf; real-Postgres integration tests for analyze-image and the PDF import (an attempt leaves the item processing; the final failure marks it failed with its reason; a completed item is never regressed; permanent failures skip the retry)
  • User-facing change → demo video attached (record-pr-demo skill); 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 there
  • If this fixes a recurring defect, considered a guardrail via the learn skill, or N/A — the convention itself was the cause; it's replaced in trigger/AGENTS.md and the skill template, with one shared hook so new item tasks get it by spreading it

Notes

  • Verified: the new tests fail on the previous code and pass now; bun run check:fix clean; the full unit suite and the trigger/ integration tests pass. I read SDK 4.5.16's task executor to confirm onFailure runs only when the attempt isn't retried, and receives the original error when catchError skips retrying.
  • Not verified: nothing ran through a real Trigger.dev deployment (not available locally), so retries, catchError and onFailure were exercised by calling the hooks directly, not end to end.
  • A 429 is in source_blocked and 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 follows processing items), and let Retry start a second paid run beside Trigger's own.

  • Tasks spread the shared itemFailureHooks (app/trigger/item-failure.ts): onFailure reports (with the payload's url/fileKey) and marks the item failed once no attempt is left, keeping a completed item as it is; catchError skips retrying failures that can't succeed, treating every SafeFetchError (SSRF gate, redirect cap, oversized body) as permanent alongside a 403/404 source or unusable content.
  • analyze-image still 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's pdf_import_failed event fires once per failed import from onFailure, and its AbortTaskRunError shortcut was replaced by catchError.
  • Hooks don't run for runs stopped at maxDuration, crashed, cancelled or expired in the queue; the stuck-items reaper still marks those items stalled.
  • The error-handling convention is documented in app/trigger/AGENTS.md and the add-trigger-task skill.

Written for commit 13242d4. Summary will update on new commits.

View guided diff Turn on auto-fix

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
@vercel

vercel Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
abode Ignored Ignored Preview Nov 15, 2026 12:01am UTC

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread app/trigger/classify-url.ts Resolved
Comment thread app/src/lib/items/processing-error.ts Resolved
Comment thread app/trigger/import-pdf.test.ts Resolved
Comment thread app/trigger/analyze-media-cover.ts Resolved
Comment thread app/trigger/enrich-item.ts Resolved
Comment thread app/src/lib/items/processing-error.ts Resolved
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
Base automatically changed from fredrivett/single-cover-enrichment to main November 14, 2026 23:59
@fredrivett
fredrivett enabled auto-merge November 15, 2026 00:02
@fredrivett
fredrivett merged commit 432689b into main Nov 15, 2026
13 checks passed
@fredrivett
fredrivett deleted the fredrivett/mark-failed-after-last-attempt branch November 15, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants