OUT-4105 | Sweep stale grouped-email windows instead of deleting them on failure - #1438
OUT-4105 | Sweep stale grouped-email windows instead of deleting them on failure#1438priosshrsth wants to merge 6 commits into
Conversation
…1397) deleteLabel passed `id: currentLabel?.id` straight into label.delete, so when findFirst matched nothing Prisma got `{ id: undefined }` and threw PrismaClientValidationError, failing the whole delete transaction. Return early instead.
* OUT-4093 | Remove the localStorage→localForage assignee migration
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR preserves grouped-email windows across temporary database outages by adding delayed retry rounds, round-scoped idempotency keys, and a larger in-run retry budget.
Confidence Score: 4/5The retry enqueue failure path should be fixed before merging because it can leave buffered emails permanently unscheduled. The new recovery path improves database-outage handling when re-enqueue succeeds, but its only scheduling handoff can reject inside a non-retried failure hook, leaving no later job to recover the unsent window. Files Needing Attention: src/jobs/notifications/flush-grouped-email.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Flush exhausts task attempts] --> B{retryRound below maximum?}
B -->|Yes| C[Trigger delayed retry round]
C -->|Scheduled| D[Future flush reads unsent rows]
C -->|Trigger request rejects| E[Failure hook exits]
E --> F[Window remains unsent and unscheduled]
B -->|No| G[Report terminal failure]
G --> H[Delete window rows]
Reviews (1): Last reviewed commit: "OUT-4105 | Stop dropping buffered groupe..." | Re-trigger Greptile |
| retryRound, | ||
| error: serializeError(error), | ||
| }) | ||
| await enqueueGroupedEmailFlush({ workspaceId, windowKey, retryRound: retryRound + 1 }) |
There was a problem hiding this comment.
Failed enqueue orphans the window
If the Trigger.dev enqueue request rejects, the failure hook exits before scheduling the retry or reaching terminal cleanup. Trigger.dev does not retry this hook, and no reconciliation job schedules old unsent windows, so the buffered emails remain permanently unsent.
Knowledge Base Used: Notification delivery workflows
… on failure When the Supabase pooler went unreachable on 8/24 every flush window burned its ~3s retry budget on the first $queryRaw, and onFailure then DELETEd the whole window — unsent rows included — so those grouped emails were destroyed, or orphaned when the delete failed too. Nothing ever re-flushed them. onFailure now leaves the rows alone, and an hourly sweeper re-enqueues any window still unsent after 30 minutes. Rows are pruned after 15 days rather than at the first sign of trouble. The flush run is already idempotent, so a re-enqueue only sends what is still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Y4tF84tzJW7Bo1DehsDX22
dda4edc to
1c096b7
Compare
|
Deployment failed for project tasks-app with the following error: Learn More: https://vercel.link/multiple-function-regions |
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Y4tF84tzJW7Bo1DehsDX22
…er send twice The sweeper can enqueue a window whose original flush is still pending, and two runs reading "sentAt" IS NULL would both send. Replace the read with an UPDATE ... RETURNING that claims the unsent rows via the existing batchId column: the losing run matches nothing and no-ops. A failed attempt releases its claim so the next Trigger.dev retry re-sends, and the sweeper releases any claim older than 30 minutes — well past the job's 60s maxDuration, so a release can never race a live send. Covered by an integration test that runs two flushes concurrently against real Postgres; it fails with two sends if the claim is reduced to a plain SELECT. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Y4tF84tzJW7Bo1DehsDX22
1bc04dd to
51d44fd
Compare
The Sentry
PrismaClientInitializationErrorburst (16 events,flushGroupedEmailRun) was the Supabase pooler being unreachable 14:30–15:17 on 8/24 — the same outage produced single events intask-notifications.serviceandnotification.service. The unreachable DB isn't ours to fix; what it exposed is. OUT-4105flushGroupedEmailOnFailurewasDELETEing the whole window including unsent rows, so a transient outage destroyed buffered grouped emails — or orphaned them atsentAt IS NULLwhen the delete failed too. Either way nothing re-flushed them:bufferGroupedEmailEventonly reuses windows younger than 5 minutes.What to look at:
flushGroupedEmailOnFailure— the delete is gone entirely. Sentry capture and logging are unchanged.sweep-grouped-email-windows.ts— hourly. Prunes rows past 15 days, releases stale claims, then re-enqueues distinct windows still unsent after 30 minutes. The 24-hour upper bound on re-enqueue is deliberate: without it a genuinely un-sendable window would retry hourly for a fortnight and page every time.claimUnsentWindowEventsis the important one. Reintroducing a recovery path means two runs can target one window, so the read is now anUPDATE ... RETURNINGthat claims the unsent rows through the existingbatchIdcolumn. The losing run matches zero rows and no-ops. Worth checking the three ways a claim is released: a failed attempt releases its own (so the next Trigger.dev retry re-sends), a successful send flipssentAt, and the sweeper releases anything older than 30 minutes — well past the job'smaxDuration: 60, so a release can never race a live send.flushGroupedEmailRunwas split so the claim wraps the dispatch in atry/catch;dispatchClaimedWindowis the old body unchanged.Verified:
yarn tsc,yarn lint:check(0 errors),yarn prettier:check. Unit 181 passed — same 10 pre-existing failures (withErrorHandler,authenticate) asmain, no new ones. Integration 19/19 against real Postgres, including a newsends once when two runs race the same windowcase; I confirmed it fails with 2 sends when the claim is reduced to a plainSELECT, so it isn't a test that passes by construction. Not verified: no staging run, so cron registration and the sweeper's SQL at production scale are untested outside the container.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y4tF84tzJW7Bo1DehsDX22