-
-
Notifications
You must be signed in to change notification settings - Fork 53
Recover stale webhook deliveries #367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import { describe, expect, it, vi } from "vitest"; | ||
|
|
||
| vi.mock("@marble/events", () => ({ | ||
| buildWebhookPayload: vi.fn(), | ||
| serializeEventType: vi.fn(), | ||
| })); | ||
| vi.mock("@/lib/db", () => ({ createDbClient: vi.fn() })); | ||
| vi.mock("@/lib/formats", () => ({ buildWebhookRequestBody: vi.fn() })); | ||
| vi.mock("@/lib/signing", () => ({ signPayload: vi.fn() })); | ||
| vi.mock("@/lib/usage", () => ({ | ||
| checkWebhookUsage: vi.fn(), | ||
| recordWebhookUsage: vi.fn(), | ||
| sendWebhookUsageAlert: vi.fn(), | ||
| })); | ||
|
|
||
| import { WEBHOOK_DELIVERY_STALE_SENDING_MS } from "@/lib/constants"; | ||
| import { claimWebhookDeliveryAttempt } from "./deliveries"; | ||
|
|
||
| function createDb({ | ||
| activeClaimCount, | ||
| staleClaimCount, | ||
| status, | ||
| }: { | ||
| activeClaimCount: number; | ||
| staleClaimCount: number; | ||
| status?: string; | ||
| }) { | ||
| const updateMany = vi | ||
| .fn() | ||
| .mockResolvedValueOnce({ count: activeClaimCount }) | ||
| .mockResolvedValueOnce({ count: staleClaimCount }); | ||
| const findUnique = vi | ||
| .fn() | ||
| .mockResolvedValue(status === undefined ? null : { status }); | ||
|
|
||
| return { | ||
| webhookDelivery: { | ||
| updateMany, | ||
| findUnique, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe("claimWebhookDeliveryAttempt", () => { | ||
| it("claims pending or retrying deliveries for a new attempt", async () => { | ||
| const now = new Date("2026-06-30T12:00:00.000Z"); | ||
| const db = createDb({ activeClaimCount: 1, staleClaimCount: 0 }); | ||
|
|
||
| await expect( | ||
| claimWebhookDeliveryAttempt(db as never, "delivery_1", now) | ||
| ).resolves.toBe(true); | ||
|
|
||
| expect(db.webhookDelivery.updateMany).toHaveBeenCalledTimes(1); | ||
| expect(db.webhookDelivery.updateMany).toHaveBeenCalledWith({ | ||
| where: { | ||
| id: "delivery_1", | ||
| status: { in: ["pending", "retrying"] }, | ||
| }, | ||
| data: { | ||
| status: "sending", | ||
| attemptCount: { increment: 1 }, | ||
| lastAttemptAt: now, | ||
| }, | ||
| }); | ||
| expect(db.webhookDelivery.findUnique).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("reclaims stale sending deliveries instead of acknowledging them", async () => { | ||
| const now = new Date("2026-06-30T12:00:00.000Z"); | ||
| const db = createDb({ activeClaimCount: 0, staleClaimCount: 1 }); | ||
|
|
||
| await expect( | ||
| claimWebhookDeliveryAttempt(db as never, "delivery_1", now) | ||
| ).resolves.toBe(true); | ||
|
|
||
| expect(db.webhookDelivery.updateMany).toHaveBeenCalledTimes(2); | ||
| expect(db.webhookDelivery.updateMany).toHaveBeenNthCalledWith(2, { | ||
| where: { | ||
| id: "delivery_1", | ||
| status: "sending", | ||
| OR: [ | ||
| { | ||
| lastAttemptAt: { | ||
| lt: new Date( | ||
| now.getTime() - WEBHOOK_DELIVERY_STALE_SENDING_MS | ||
| ), | ||
| }, | ||
| }, | ||
| { lastAttemptAt: null }, | ||
| ], | ||
| }, | ||
| data: { | ||
| status: "sending", | ||
| attemptCount: { increment: 1 }, | ||
| lastAttemptAt: now, | ||
| }, | ||
| }); | ||
| expect(db.webhookDelivery.findUnique).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("keeps active sending deliveries on the queue retry path", async () => { | ||
| const now = new Date("2026-06-30T12:00:00.000Z"); | ||
| const db = createDb({ | ||
| activeClaimCount: 0, | ||
| staleClaimCount: 0, | ||
| status: "sending", | ||
| }); | ||
|
|
||
| await expect( | ||
| claimWebhookDeliveryAttempt(db as never, "delivery_1", now) | ||
| ).rejects.toThrow("Webhook delivery delivery_1 is already sending"); | ||
| }); | ||
|
|
||
| it("skips terminal or missing deliveries", async () => { | ||
| const now = new Date("2026-06-30T12:00:00.000Z"); | ||
| const db = createDb({ | ||
| activeClaimCount: 0, | ||
| staleClaimCount: 0, | ||
| status: "success", | ||
| }); | ||
|
|
||
| await expect( | ||
| claimWebhookDeliveryAttempt(db as never, "delivery_1", now) | ||
| ).resolves.toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import { buildWebhookPayload, serializeEventType } from "@marble/events"; | ||
| import { WEBHOOK_DELIVERY_TIMEOUT_MS } from "@/lib/constants"; | ||
| import { | ||
| WEBHOOK_DELIVERY_STALE_SENDING_MS, | ||
| WEBHOOK_DELIVERY_TIMEOUT_MS, | ||
| } from "@/lib/constants"; | ||
| import { createDbClient } from "@/lib/db"; | ||
| import { buildWebhookRequestBody } from "@/lib/formats"; | ||
| import { signPayload } from "@/lib/signing"; | ||
|
|
@@ -37,19 +40,9 @@ async function processDelivery( | |
| env: Env, | ||
| deliveryId: string | ||
| ) { | ||
| const claim = await db.webhookDelivery.updateMany({ | ||
| where: { | ||
| id: deliveryId, | ||
| status: { in: ["pending", "retrying"] }, | ||
| }, | ||
| data: { | ||
| status: "sending", | ||
| attemptCount: { increment: 1 }, | ||
| lastAttemptAt: new Date(), | ||
| }, | ||
| }); | ||
| const claimed = await claimWebhookDeliveryAttempt(db, deliveryId); | ||
|
|
||
| if (claim.count === 0) { | ||
| if (!claimed) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -220,3 +213,56 @@ async function processDelivery( | |
|
|
||
| throw new Error(`Webhook returned ${response.status}`); | ||
| } | ||
|
|
||
| export async function claimWebhookDeliveryAttempt( | ||
| db: ReturnType<typeof createDbClient>, | ||
| deliveryId: string, | ||
| now = new Date() | ||
| ) { | ||
| const claim = await db.webhookDelivery.updateMany({ | ||
| where: { | ||
| id: deliveryId, | ||
| status: { in: ["pending", "retrying"] }, | ||
| }, | ||
| data: { | ||
| status: "sending", | ||
| attemptCount: { increment: 1 }, | ||
| lastAttemptAt: now, | ||
| }, | ||
| }); | ||
|
|
||
| if (claim.count > 0) { | ||
| return true; | ||
| } | ||
|
|
||
| const staleCutoff = new Date( | ||
| now.getTime() - WEBHOOK_DELIVERY_STALE_SENDING_MS | ||
| ); | ||
| const staleClaim = await db.webhookDelivery.updateMany({ | ||
| where: { | ||
| id: deliveryId, | ||
| status: "sending", | ||
| OR: [{ lastAttemptAt: { lt: staleCutoff } }, { lastAttemptAt: null }], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a worker is interrupted after claiming the last allowed attempt, the row is left Useful? React with 👍 / 👎. |
||
| }, | ||
| data: { | ||
| status: "sending", | ||
| attemptCount: { increment: 1 }, | ||
| lastAttemptAt: now, | ||
| }, | ||
| }); | ||
|
|
||
| if (staleClaim.count > 0) { | ||
| return true; | ||
| } | ||
|
|
||
| const delivery = await db.webhookDelivery.findUnique({ | ||
| where: { id: deliveryId }, | ||
| select: { status: true }, | ||
| }); | ||
|
|
||
| if (delivery?.status === "sending") { | ||
| throw new Error(`Webhook delivery ${deliveryId} is already sending`); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import { dirname, resolve } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { defineConfig } from "vitest/config"; | ||
|
|
||
| const root = dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| export default defineConfig({ | ||
| resolve: { | ||
| alias: { | ||
| "@": resolve(root, "src"), | ||
| }, | ||
| }, | ||
| test: { | ||
| include: ["src/**/*.test.ts"], | ||
| }, | ||
| }); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ 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 byid. If the original worker resumes after the reclaim, it can overwrite the newer attempt'ssuccess/failed/retryingstate.Please return claim metadata from
claimWebhookDeliveryAttempt(for example thelastAttemptAtwritten during the claim, or a dedicated lease token) and require every laterwebhookDelivery.update*in this attempt to match that token inwherebefore mutating the row again.Possible direction
Then switch the later status transitions to conditional
updateMany(...)calls such aswhere: { id: delivery.id, lastAttemptAt: claim.claimedAt }so a reclaimed worker cannot clobber the current owner.Also applies to: 217-268
🤖 Prompt for AI Agents