Skip to content

fix: use stored event_type for webhook retry routing#144

Merged
Siddhartha-singh01 merged 2 commits into
Coder-s-OG-s:mainfrom
Dharya-dev:fix/webhook-retry-event-type-routing
May 17, 2026
Merged

fix: use stored event_type for webhook retry routing#144
Siddhartha-singh01 merged 2 commits into
Coder-s-OG-s:mainfrom
Dharya-dev:fix/webhook-retry-event-type-routing

Conversation

@Dharya-dev

Copy link
Copy Markdown
Contributor

Summary

Fixes #143 — The webhook retry route hardcodes 'github/pull_request' as the Inngest event name, silently mis-routing every non-PR dead-lettered event to the wrong handler.

Problem

The main webhook handler in route.ts correctly dispatches events dynamically:

// route.ts — line 70 (CORRECT)
await inngest.send({
  name: `github/${eventType}`,     // dynamic, from x-github-event header
  data: { deliveryId, eventType, payload: JSON.parse(raw) },
});

But the retry route ignores the stored event_type column and always sends github/pull_request:

// retry/route.ts — line 47 (BUG)
await inngest.send({
  name: 'github/pull_request',     // hardcoded — ignores failedEvent.event_type
  data: failedEvent.payload,
});

This works today only by coincidence — process-pr-event.ts is currently the only handler writing to failed_webhook_events. The moment any other handler (installation, issues, reviews, comments) dead-letters an event, retrying it will:

  1. Route it to process-pr-event instead of the correct handler
  2. Cause a silent failure (wrong payload shape, no crash)
  3. Return { ok: true } — the maintainer believes the retry succeeded

Changes

src/app/api/webhooks/github/retry/route.ts (core fix):

  • Replace hardcoded 'github/pull_request' with failedEvent.event_type
  • Add a format guard: reject events whose event_type doesn't start with 'github/' (returns 422)
  • Include event_type in the success response for observability

src/app/api/webhooks/github/retry/route.test.ts (new test file):

  • Verifies correct event_type dispatch (non-PR event → correct handler)
  • Regression guard for github/pull_request events (existing behavior preserved)
  • Validates the format guard rejects invalid event types (422)
  • Tests the 404 path for missing events

Test Results

 Test Files  37 passed (37)
      Tests  304 passed (304)
   Duration  1.20s

All 304 existing tests pass. Zero regressions. The 4 new tests all pass.

Diff

The core fix is a single-line change (name: failedEvent.event_type instead of name: 'github/pull_request'), plus a defensive guard and observability improvement.

The retry route hardcoded 'github/pull_request' as the Inngest event
name, silently mis-routing every non-PR dead-lettered event. This
replaces the hardcoded string with failedEvent.event_type from the
failed_webhook_events table, adds a format guard for corrupted values,
and returns the dispatched event_type in the response for observability.

Closes Coder-s-OG-s#143
@vercel

vercel Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

@Dharya-dev is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Siddhartha-singh01 Siddhartha-singh01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work @Dharya-dev the core fix is exactly right. Using the stored event_type
instead of the hardcoded value fixes the mis-routing, the 422 format guard is a good
addition, and the test file is solid (the stored-type dispatch, the pull_request
regression guard, and the invalid-type case all cover the right things).

One thing before this can close #143: the issue's acceptance criteria had four items,
and this PR covers two of them. Still open:

  • retry_count isn't incremented on retry there's still no bound on how many times
    the same event can be re-fired.
  • the failed_webhook_events row isn't deleted or marked resolved after a successful
    retry, so the dead-letter table keeps growing.

Could you add those two here so the PR fully closes #143? If you'd rather keep this
PR focused on just the routing fix, that's fine too just let us know and we'll open
a separate follow-up issue for the retry-count + cleanup, and adjust the "Fixes #143"
link accordingly.

Also heads up, CI / check is still "awaiting approval" so the tests haven't run
yet; a maintainer will approve the workflow so we can confirm green.

Thanks for the quick, clean contribution!

…oder-s-OG-s#143)

- Enforce MAX_RETRIES=5 ceiling; return 409 when exceeded
- Increment retry_count before dispatch for crash-safety
- Delete failed_webhook_events row after successful Inngest dispatch
- Add 3 new tests covering retry cap, increment, and row cleanup
@Dharya-dev

Copy link
Copy Markdown
Contributor Author

Thanks for the review @Siddhartha-singh01! Great catch on the remaining criteria.

I've pushed a follow-up commit (bb617f4) that adds both:

1. Retry-count cap (MAX_RETRIES = 5)

  • retry_count is incremented before dispatching (crash-safe — the count is durable even if the process dies after Inngest accepts the event but before cleanup).
  • Once retry_count >= 5, the endpoint returns 409 Conflict with { error: 'max retries exceeded', retry_count, max: 5 }.
  • Events that hit the ceiling are left in the table for manual investigation rather than silently looping.

2. Dead-letter row cleanup

  • After a successful inngest.send(), the failed_webhook_events row is deleted so the table doesn't grow unboundedly.
  • Order of operations: increment → dispatch → delete (so if dispatch fails, the row survives with the updated count).

New tests (3 added, 7 total)

Test Status
increments retry_count before dispatching
rejects retries that exceed MAX_RETRIES (409)
deletes the dead-letter row after successful dispatch

Full suite: 37 files, 307 tests, all passing.

Ready for another look whenever you get a chance!

@Siddhartha-singh01 Siddhartha-singh01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-checked both remaining acceptance criteria are in: retry_count is incremented
(before dispatch, nicely crash-safe), the dead-letter row is deleted after a
successful retry, and the MAX_RETRIES=5 / 409 ceiling is a good addition. Traced all
7 tests and they line up. This fully closes #143 now.

LGTM ✅ thanks for the thorough follow-up!

@Siddhartha-singh01
Siddhartha-singh01 merged commit a3dc388 into Coder-s-OG-s:main May 17, 2026
2 of 3 checks passed
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.

Webhook retry route hardcodes the event type mis-routes every non-PR dead-lettered event

2 participants