Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a HandledError class to differentiate between expected errors (that should return 200 to Stripe) and unexpected errors (that should return 500 for retry) in webhook handling. However, the implementation has critical flaws that could lead to data loss and undermines webhook reliability.
- Added
HandledErrorclass to prevent Stripe from disabling webhooks due to too many 500 responses - Converted multiple error throws to use
HandledErrorinstead of regularError - Updated error handling in the main POST handler to return 200 for
HandledErrorinstances
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (error) { | ||
| throw error; | ||
| console.error("Error processing invoice payment succeeded:", error); | ||
| throw new HandledError("Failed to process invoice payment succeeded event"); |
There was a problem hiding this comment.
Using HandledError here masks database failures and other unexpected errors that should be retried by Stripe. The catch block receives errors from database operations and subscription record insertion, which are transient failures that benefit from retry. This could result in lost payment records if a temporary database issue occurs.
Consider keeping the original throw error behavior, or differentiate between expected validation errors and unexpected database errors, only using HandledError for the former.
| throw new HandledError("Failed to process invoice payment succeeded event"); | |
| if (error instanceof HandledError) { | |
| throw error; | |
| } | |
| throw error; |
| // TODO: These actually SHOULD return a 500 so that Stripe retries, BUT I don't currently have a second stripe account set up | ||
| // which means this endpoint is receiving events from multiple products, which should fail silently. |
There was a problem hiding this comment.
This TODO comment reveals a fundamental design flaw: the current implementation is intentionally incorrect because it's handling events from multiple Stripe accounts/products. The HandledError approach defeats the purpose of webhook retry mechanisms and could lead to data loss.
The proper solution is to set up separate webhook endpoints for different products/accounts, or add validation at the beginning of the handler to reject events that don't belong to this product (returning 200 immediately). The current approach of silently failing on expected errors undermines the webhook's reliability and makes debugging production issues difficult.
| const handleInvoicePaymentSucceeded: Handler = async ({ type, data }) => { | ||
| if (type !== "invoice.payment_succeeded") { | ||
| throw new Error(`Expected invoice.payment_succeeded, got ${type}`); | ||
| throw new HandledError(`Expected invoice.payment_succeeded, got ${type}`); |
There was a problem hiding this comment.
Using HandledError for a type mismatch is incorrect - this represents a programming error or misconfiguration in the event routing logic, not an expected business condition. This should remain a regular Error that returns 500, as it indicates a bug in the code that needs to be investigated and fixed.
Type validation errors like this should fail loudly (return 500) so they're noticed and fixed, not silently accepted (return 200).
| throw new HandledError(`Expected invoice.payment_succeeded, got ${type}`); | |
| throw new Error(`Expected invoice.payment_succeeded, got ${type}`); |
| /** | ||
| * Custom error class for expected errors that should still return 200 to Stripe. | ||
| * | ||
| * Stripe webhooks require a 200 status code to acknowledge successful receipt of the event. | ||
| * If we return 500 too much, Stripe will eventually disable the webhook. | ||
| * This error class represents situations where: | ||
| * - We successfully received and processed the webhook | ||
| * - We encountered an expected/handled error condition (e.g., missing customer ID in DB) | ||
| * - We don't want Stripe to retry the webhook | ||
| * | ||
| * Only throw regular errors (which return 500) for truly unexpected failures that | ||
| * might be resolved on retry (e.g., database connection issues, temporary failures). | ||
| * | ||
| * @example | ||
| * // Customer not found - expected condition, no need to retry | ||
| * if (!userId) { | ||
| * throw new HandledError(`No user found with Stripe customer ID: ${customerId}`); | ||
| * } | ||
| * | ||
| * // Database connection failed - unexpected, should retry | ||
| * if (dbConnectionFailed) { | ||
| * throw new Error("Database connection failed"); | ||
| * } | ||
| */ |
There was a problem hiding this comment.
The documentation for HandledError contradicts its actual usage in the code. The docs state it's for "expected/handled error conditions" where "we don't want Stripe to retry," but the TODO comment at lines 243-244 indicates these errors SHOULD actually return 500 for retry, and the current behavior is a workaround for a multi-product webhook issue.
The documentation should either:
- Accurately describe the current temporary workaround behavior, or
- Be updated once the proper solution (separate webhooks or proper event filtering) is implemented
| throw new HandledError( | ||
| "Failed to update user with Stripe customer ID after checkout completion", | ||
| ); |
There was a problem hiding this comment.
Using HandledError here masks database failures that should actually be retried by Stripe. Database errors (connection issues, transaction failures, etc.) are typically transient and should return a 500 status so Stripe retries the webhook. This change could lead to lost data if a temporary database issue occurs during checkout completion.
Consider keeping the original throw error behavior, or only use HandledError for specific expected error types (e.g., validation errors) while re-throwing database errors as regular Error instances.
| throw new HandledError( | |
| "Failed to update user with Stripe customer ID after checkout completion", | |
| ); | |
| throw error; |
Fix webhook