fix: use stored event_type for webhook retry routing#144
Conversation
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
|
@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
left a comment
There was a problem hiding this comment.
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_countisn't incremented on retry there's still no bound on how many times
the same event can be re-fired.- the
failed_webhook_eventsrow 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
|
Thanks for the review @Siddhartha-singh01! Great catch on the remaining criteria. I've pushed a follow-up commit ( 1. Retry-count cap (
|
| 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
left a comment
There was a problem hiding this comment.
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!
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.tscorrectly dispatches events dynamically:But the retry route ignores the stored
event_typecolumn and always sendsgithub/pull_request:This works today only by coincidence —
process-pr-event.tsis currently the only handler writing tofailed_webhook_events. The moment any other handler (installation, issues, reviews, comments) dead-letters an event, retrying it will:process-pr-eventinstead of the correct handler{ ok: true }— the maintainer believes the retry succeededChanges
src/app/api/webhooks/github/retry/route.ts(core fix):'github/pull_request'withfailedEvent.event_typeevent_typedoesn't start with'github/'(returns 422)event_typein the success response for observabilitysrc/app/api/webhooks/github/retry/route.test.ts(new test file):github/pull_requestevents (existing behavior preserved)Test Results
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_typeinstead ofname: 'github/pull_request'), plus a defensive guard and observability improvement.