fix(web): stop duplicate auto top-up confirmation emails on ambiguous provider errors - #4971
Open
kilo-code-bot[bot] wants to merge 1 commit into
Open
fix(web): stop duplicate auto top-up confirmation emails on ambiguous provider errors#4971kilo-code-bot[bot] wants to merge 1 commit into
kilo-code-bot[bot] wants to merge 1 commit into
Conversation
…rovider errors The transactional_email_log 'insert marker before send' dedupe for top-up confirmation emails (used by both manual and auto top-up) rolled the marker back whenever sendCreditsTopUpEmail threw, treating any exception as safe to retry. A network timeout or 5xx from Mailgun after it had already accepted the message is ambiguous, not safe: the marker gets deleted, and a Stripe webhook retry (or the duplicate-charge recovery path in processTopUp) re-inserts it and resends an email that may have already been delivered, producing the duplicate auto top-up emails reported by Emilie Schario. Fix: only roll back the marker for failures that are known-safe to retry (the provider was never called, e.g. provider_not_configured). Leave the marker in place for any exception thrown by the send call itself, in both the individual-user path (credits.ts) and the organization path (organization-billing.ts, where the per-recipient loop now tracks ambiguous vs. known-safe failures separately and only rolls back when every failure was known-safe). Adds a regression test asserting the marker survives an ambiguous send exception and that a subsequent retry does not resend the email.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Reported by Emilie Schario: auto top-up transactional emails were being sent twice.
Top-up confirmation emails (both manual and auto top-up, for individual users and organizations) are deduplicated via an "insert marker before send" pattern against the
transactional_email_logtable, keyed by(email_type, stripe_charge_or_invoice_id):onConflictDoNothing().The bug is in step 3: the rollback treated any exception from the send call (
apps/web/src/lib/credits.tsandapps/web/src/lib/organizations/organization-billing.ts) as safe to retry, deleting the marker unconditionally. But an exception fromsendCreditsTopUpEmail→sendViaMailgun→ Mailgun'sclient.messages.create()(apps/web/src/lib/email-mailgun.ts) is ambiguous: a network timeout or a 5xx can occur after Mailgun already accepted/delivered the message. Deleting the marker in that case doesn't mean "safe to retry" — it means "we don't actually know."Once the marker was deleted, any of the following could trigger a resend of an email that may have already gone out:
invoice.paid/charge event (Stripe retries webhooks on non-2xx or timeout).recoverTopUpConfirmationEmailIfMissingrecovery path inprocessTopUp, which runs specifically when a duplicate webhook hits thecredit_transactionsunique-constraintonConflictDoNothing()branch — exactly the scenario most likely to coincide with an in-flight ambiguous send from the first delivery attempt.Multiple independent triggers feed into the same auto top-up flow (per-request usage-path trigger, a coding-plan billing-lifecycle cron, and a separate KiloClaw billing sweep), all of which are already correctly serialized by the
auto_top_up_configs.attempt_started_atlock and thecredit_transactions.stripe_payment_idunique index — so the credit itself is never double-applied. The email-marker rollback bug was the one place where "ambiguous == safe to retry" duplicated a side effect (the email) even though the underlying payment/credit dedup was correct.Fix
apps/web/src/lib/credits.ts: the catch block around the top-up confirmation email send no longer deletes thetransactional_email_logmarker. The only remaining rollback is the pre-existing, explicitprovider_not_configuredcase, where we know for certain the provider was never called. Any other exception now leaves the marker in place, matching the codebase's own stated preference ("we prefer missing one email over duplicate sends").apps/web/src/lib/organizations/organization-billing.ts: the per-recipient send loop now tracks "known-safe" failures (e.g.provider_not_configured) separately from "ambiguous" failures (exceptions thrown by the send call). The marker is only rolled back when every failure across all recipients was known-safe; a single ambiguous failure now blocks the rollback for the whole batch.apps/web/src/lib/purchase-emails.test.tsasserting that after an ambiguous provider exception, the email-log marker survives and a subsequent retry (simulating a webhook redelivery hitting the duplicate-credit-transaction recovery path) does not resend the email.Verification
pnpm exec oxlint --config .oxlintrc.jsonon the changed files: 0 warnings/errors.tsc --noEmitonapps/web: no errors in the changed files.oxfmtformatting applied;git diff --checkclean.purchase-emails.test.tsend-to-end in this sandbox (no Docker/Postgres available to runpnpm test:db), so the new regression test has not been executed against a live database in this environment. It follows the same structure and helpers as the existing marker-dedupe tests in the same file (e.g. "writes a transactional_email_log marker on first-attempt send", "recovers confirmation email on webhook retry when first attempt did not send") and should be run as part of normal CI/PR checks.Built for Alex Gold by Kilo for Slack