Recover stale webhook deliveries#367
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
|
|
@ag-linden is attempting to deploy a commit to the Marble Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a stale-sending reclaim mechanism for webhook deliveries in the jobs app: a new constant, a ChangesWebhook Delivery Claiming
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644adb52ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| where: { | ||
| id: deliveryId, | ||
| status: "sending", | ||
| OR: [{ lastAttemptAt: { lt: staleCutoff } }, { lastAttemptAt: null }], |
There was a problem hiding this comment.
When a worker is interrupted after claiming the last allowed attempt, the row is left sending with attemptCount === maxAttempts; this stale predicate still matches it, then the retry increments attemptCount again and processDelivery only checks attemptNumber >= maxAttempts after making the POST. In that recovery case the customer endpoint can receive a fourth request for the default maxAttempts = 3 (possibly duplicating a request that already reached them), so the stale-reclaim path should guard on remaining attempts or make the delivery terminal before sending again.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/jobs/src/consumers/deliveries.ts`:
- Around line 43-45: The delivery claim flow in claimWebhookDeliveryAttempt and
the later webhookDelivery.update* status transitions need a lease check, not
just a boolean claim result. Return claim metadata such as lastAttemptAt or a
dedicated token from claimWebhookDeliveryAttempt, then require every subsequent
state write in deliveries.ts to include that same token in its where clause
before updating the row. Update the later success/failed/retrying writes to
conditional updateMany-style mutations so a reclaimed worker cannot overwrite
the current attempt’s state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cde6bf5b-884d-4fe2-a6ad-3ff5068bd31f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/jobs/package.jsonapps/jobs/src/consumers/deliveries.test.tsapps/jobs/src/consumers/deliveries.tsapps/jobs/src/lib/constants.tsapps/jobs/vitest.config.mjs
| const claimed = await claimWebhookDeliveryAttempt(db, deliveryId); | ||
|
|
||
| if (claim.count === 0) { | ||
| if (!claimed) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fence reclaimed attempts before any later delivery-state write.
After a stale reclaim, two workers can exist for the same delivery. The helper only returns boolean, so the later writes at Line 81, Line 136, Line 146, Line 188, Line 199, and Line 209 are still keyed only by id. If the original worker resumes after the reclaim, it can overwrite the newer attempt's success/failed/retrying state.
Please return claim metadata from claimWebhookDeliveryAttempt (for example the lastAttemptAt written during the claim, or a dedicated lease token) and require every later webhookDelivery.update* in this attempt to match that token in where before mutating the row again.
Possible direction
- const claimed = await claimWebhookDeliveryAttempt(db, deliveryId);
- if (!claimed) {
+ const claim = await claimWebhookDeliveryAttempt(db, deliveryId);
+ if (!claim) {
return;
}-export async function claimWebhookDeliveryAttempt(...)
+export async function claimWebhookDeliveryAttempt(...): Promise<{ claimedAt: Date } | null>
...
- return true;
+ return { claimedAt: now };
...
- return true;
+ return { claimedAt: now };
...
- return false;
+ return null;Then switch the later status transitions to conditional updateMany(...) calls such as where: { id: delivery.id, lastAttemptAt: claim.claimedAt } so a reclaimed worker cannot clobber the current owner.
Also applies to: 217-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/jobs/src/consumers/deliveries.ts` around lines 43 - 45, The delivery
claim flow in claimWebhookDeliveryAttempt and the later webhookDelivery.update*
status transitions need a lease check, not just a boolean claim result. Return
claim metadata such as lastAttemptAt or a dedicated token from
claimWebhookDeliveryAttempt, then require every subsequent state write in
deliveries.ts to include that same token in its where clause before updating the
row. Update the later success/failed/retrying writes to conditional
updateMany-style mutations so a reclaimed worker cannot overwrite the current
attempt’s state.
2 participants
Description
This updates the webhook delivery worker so queue retries can recover delivery rows that were left in
sendingby an interrupted attempt.Changes included:
sendingdeliveries after the outbound delivery timeout window.sendingdeliveries on the queue retry path instead of acknowledging a skipped delivery.sendingrecovery, activesendingretry behavior, and terminal delivery skips.Motivation and Context
The delivery worker marks a
webhookDeliveryrow assendingbefore making the outbound POST. If the worker is interrupted before it records the attempt result and final status, the next queue delivery sees the row insending. Previously that failed thepending/retryingclaim and returned successfully, which acknowledged the queue message while leaving the row permanently in progress.This keeps the current durable claim model, but makes
sendingrecoverable once the prior attempt is stale. Non-stalesendingrows still retry later, which avoids treating an actively owned delivery as complete.How to Test
Passed locally:
Local blockers unrelated to this patch:
The root lint script invokes
pnpx, which is not available in the pinned Corepack pnpm environment. Running the equivalentcorepack pnpm exec ultracite checkreaches the existing rootbiome.jsoncand stops on unknown config keysnoShadowandnoUnnecessaryConditionsbefore checking changed files.The full build stops in
web#buildbecause the local environment does not defineMARBLE_API_KEYfor the Astro content config.Screenshots (if applicable)
N/A
Video Demo (if applicable)
N/A
Types of Changes
Summary by CodeRabbit
Bug Fixes
Tests
Chores