fix(notifications): P2 extreme-audit P1 fixes before deploy
Independent audit verdict was NOT SAFE; all findings fixed:
- dispatcher Idempotency-Key is now per-attempt (${id}:${attempts}),
not row id alone, so a transient-failure response cached by Resend
cannot permanently suppress every retry and silently lose an email.
- enqueueDues is fully try/catch wrapped so a notifications-table or
render failure can never throw into / wedge the Stripe money path.
- dues_lapsed dedupe falls back to the subscription id when periodEnd
is null (canceled/deleted subs), so future lapses no longer collapse
to one key and get suppressed. New test; suite 499.
- documented the dispatcher's once/min concurrency assumption.
This commit is contained in:
@@ -4,10 +4,14 @@
|
||||
// (cron has none) so proxy.ts whitelists /api/cron; trust is the CRON_SECRET
|
||||
// shared-secret header, compared in constant time.
|
||||
//
|
||||
// Re-entrancy: a row is updated immediately after its own send, and sendEmail
|
||||
// passes the row id as Resend's Idempotency-Key, so an overlapping run cannot
|
||||
// double-send. Transient failures (retryable) stay pending for the next
|
||||
// minute until the attempt budget is exhausted; permanent ones go to 'failed'.
|
||||
// Re-entrancy: there is no row-level claim/lock, so two overlapping runs can
|
||||
// scan the same pending rows. This is safe because (a) the crontab fires once
|
||||
// a minute and a run drains <=20 rows in ~5s, so overlap is rare, and (b)
|
||||
// sendEmail passes a per-attempt Idempotency-Key (`${id}:${attempts}`):
|
||||
// concurrent runs of the SAME attempt dedupe at Resend, while the next
|
||||
// minute's retry of a transiently-failed row is a deliberately fresh send.
|
||||
// Transient failures stay pending until the attempt budget is exhausted;
|
||||
// permanent ones go to 'failed'.
|
||||
import { NextRequest, NextResponse } from 'next/server'
|
||||
import { timingSafeEqual } from 'node:crypto'
|
||||
import { createAdminClient } from '@/lib/supabase/admin'
|
||||
@@ -63,7 +67,13 @@ export async function POST(req: NextRequest) {
|
||||
subject: row.subject as string,
|
||||
html: row.body_html as string,
|
||||
text: row.body_text as string,
|
||||
idempotencyKey: row.id as string,
|
||||
// Per-attempt, NOT row.id alone: Resend dedupes 24h on key+payload and
|
||||
// its docs do not guarantee a failed response is excluded from that
|
||||
// cache. A stable key would let a cached transient failure permanently
|
||||
// suppress every later retry (email silently lost). Tying the key to
|
||||
// the attempt number means concurrent runs of the SAME attempt still
|
||||
// dedupe, but the next minute's real retry is a fresh send.
|
||||
idempotencyKey: `${row.id}:${(row.attempts as number) ?? 0}`,
|
||||
})
|
||||
|
||||
if (res.ok) {
|
||||
|
||||
@@ -88,6 +88,11 @@ export async function POST(
|
||||
// inline (keeps this money path fast + retry-safe); the dispatcher cron
|
||||
// sends. ignoreDuplicates + the (space_id, dedupe_key) unique index make a
|
||||
// Stripe event replay (new event id, same invoice/period) a no-op.
|
||||
//
|
||||
// BEST-EFFORT: the entire body is wrapped so a notifications-table or
|
||||
// render failure can NEVER throw into the money path. The Stripe ledger /
|
||||
// member_billing / status writes must finalize even if email infra is
|
||||
// down; a missed enqueue is acceptable, a wedged money path is not.
|
||||
async function enqueueDues(
|
||||
type: DuesNotificationType,
|
||||
args: {
|
||||
@@ -97,45 +102,55 @@ export async function POST(
|
||||
currency?: string | null
|
||||
periodEnd?: string | null
|
||||
invoiceId?: string | null
|
||||
subscriptionId?: string | null
|
||||
},
|
||||
): Promise<void> {
|
||||
if (!args.memberId) return
|
||||
const { data: mem } = await admin
|
||||
.from('space_members')
|
||||
.select('email, display_name')
|
||||
.eq('id', args.memberId)
|
||||
.eq('space_id', spaceId)
|
||||
.maybeSingle()
|
||||
const recipient = ((mem?.email as string | null) || args.fallbackEmail || '').trim()
|
||||
if (!recipient) return
|
||||
const { subject, html, text } = renderDuesEmail({
|
||||
type,
|
||||
spaceName: await getSpaceName(),
|
||||
memberName: (mem?.display_name as string | null) ?? null,
|
||||
amount: args.amount ?? null,
|
||||
currency: args.currency ?? null,
|
||||
periodEnd: args.periodEnd ?? null,
|
||||
manageUrl,
|
||||
})
|
||||
await admin.from('notifications').upsert(
|
||||
{
|
||||
space_id: spaceId,
|
||||
member_id: args.memberId,
|
||||
try {
|
||||
if (!args.memberId) return
|
||||
const { data: mem } = await admin
|
||||
.from('space_members')
|
||||
.select('email, display_name')
|
||||
.eq('id', args.memberId)
|
||||
.eq('space_id', spaceId)
|
||||
.maybeSingle()
|
||||
const recipient = ((mem?.email as string | null) || args.fallbackEmail || '').trim()
|
||||
if (!recipient) return
|
||||
const { subject, html, text } = renderDuesEmail({
|
||||
type,
|
||||
channel: 'email',
|
||||
recipient,
|
||||
subject,
|
||||
body_html: html,
|
||||
body_text: text,
|
||||
status: 'pending',
|
||||
dedupe_key: duesDedupeKey(type, {
|
||||
invoiceId: args.invoiceId,
|
||||
memberId: args.memberId,
|
||||
periodEnd: args.periodEnd,
|
||||
}),
|
||||
},
|
||||
{ onConflict: 'space_id,dedupe_key', ignoreDuplicates: true },
|
||||
)
|
||||
spaceName: await getSpaceName(),
|
||||
memberName: (mem?.display_name as string | null) ?? null,
|
||||
amount: args.amount ?? null,
|
||||
currency: args.currency ?? null,
|
||||
periodEnd: args.periodEnd ?? null,
|
||||
manageUrl,
|
||||
})
|
||||
const { error: enqErr } = await admin.from('notifications').upsert(
|
||||
{
|
||||
space_id: spaceId,
|
||||
member_id: args.memberId,
|
||||
type,
|
||||
channel: 'email',
|
||||
recipient,
|
||||
subject,
|
||||
body_html: html,
|
||||
body_text: text,
|
||||
status: 'pending',
|
||||
dedupe_key: duesDedupeKey(type, {
|
||||
invoiceId: args.invoiceId,
|
||||
memberId: args.memberId,
|
||||
periodEnd: args.periodEnd,
|
||||
subscriptionId: args.subscriptionId,
|
||||
}),
|
||||
},
|
||||
{ onConflict: 'space_id,dedupe_key', ignoreDuplicates: true },
|
||||
)
|
||||
if (enqErr) console.error(`[stripe webhook] enqueue ${type} failed:`, enqErr.message)
|
||||
} catch (e) {
|
||||
console.error(
|
||||
`[stripe webhook] enqueue ${type} threw:`,
|
||||
e instanceof Error ? e.message : e,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// Resolve our member: prefer metadata (set on the subscription + session),
|
||||
@@ -211,7 +226,7 @@ export async function POST(
|
||||
// Dues lapsed past grace -> one "marked late" email per lapsed period
|
||||
// (dedupe is member + periodEnd, so next cycle can lapse again).
|
||||
if (desired === 'late') {
|
||||
await enqueueDues('dues_lapsed', { memberId, periodEnd })
|
||||
await enqueueDues('dues_lapsed', { memberId, periodEnd, subscriptionId: sub.id })
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user