Review: idempotent webhook retries

Fictional sample. The repository, change, and findings are invented to demonstrate a review page; replace them with the real change and its evidence.

The change makes payment webhooks safe to retry: a delivery that arrives twice is recorded once. Verdict: request changes — one blocking finding, two smaller ones.

Verdict

The approach is right; the key is not. Two deliveries of the same event with different retry counters get different keys and are both processed.

Findings

  • Blocking 1
  • Major 1
  • Minor 1
Blocking

The idempotency key includes the retry counter

src/webhooks/handler.ts:42

attempt changes on every retry, so the key never repeats for the case the change exists for. Build the key from event.id and event.type only.

Major

The unique constraint is not in the migration

migrations/0042_webhook_events.sql

The code relies on a conflict that only a unique index on idempotency_key produces. Add it in the same migration.

Minor

The log line repeats the payload

src/webhooks/handler.ts:57

Log the event id instead; the payload can carry personal data.

The change

src/webhooks/handler.ts7 lines added, 2 removed
@@ -38,7 +38,12 @@ export async function handleWebhook(event: WebhookEvent, attempt: number) {
   const verified = verifySignature(event);
   if (!verified) return reject(event, 'signature');

-  await payments.apply(event);
-  log.info('webhook applied', event.payload);
+  const key = `${event.id}:${event.type}:${attempt}`;
+  const inserted = await db.webhookEvents.insertIgnoringConflict({ key, event });
+  if (!inserted) {
+    log.info('duplicate webhook skipped', event.payload);
+    return accept(event);
+  }
+  await payments.apply(event);
   return accept(event);
 }

The handler now records each delivery before applying it and skips a delivery whose key already exists. The findings above point at the lines of this diff.

What would make it mergeable

  1. Drop attempt from the key and add a test that delivers the same event twice with different attempts.
  2. Add the unique index to migration 0042.
  3. Log event.id instead of the payload.

Review workspace

Review this report

No discussion threads yet