PR Review - Invoice Reminder Scheduling

Severity-ranked findings

Review findings

Filter by severity to isolate what should block merge first.

High

Failed enqueue marks reminder as processed too early

Illustrative reference: services/reminders/scheduleRetry.ts:84-111

The refactor updates last_attempted_at before the queue publish resolves. If the publish call fails, the scheduler will skip the invoice on the next sweep because it looks already attempted.

Behavioral impact: A transient queue outage can silently suppress all future reminders for affected invoices.

Medium

Timezone normalization changed without migration note

Illustrative reference: lib/billing/reminderWindow.ts:14-39

The new logic schedules in tenant-local midnight instead of UTC midnight, but existing reminder timestamps are still persisted in UTC without a recalculation path.

Behavioral impact: Some reminders may move a day earlier or later immediately after deploy for non-US tenants.

Medium

Retry cap no longer distinguishes hard bounce vs temporary delivery error

Illustrative reference: workers/reminders/processReminderAttempt.ts:52-96

All failures increment the same retry counter. Previously, hard-bounced addresses were disabled immediately while soft failures could retry.

Behavioral impact: The system may spend unnecessary retry budget on known-undeliverable addresses.

Low

New event name is more generic than existing analytics taxonomy

Illustrative reference: analytics/events.ts:122-130

invoice_reminder_attempted became notification_attempted without a companion dimension for type.

Behavioral impact: The metrics dashboard will become harder to segment unless downstream queries are updated.

Affected files

Where the risk sits

File	Change summary	Risk note
`services/reminders/scheduleRetry.ts`	Restructures enqueue flow and attempt bookkeeping	Likely source of skipped reminder regression
`lib/billing/reminderWindow.ts`	Normalizes reminder send time by tenant timezone	Needs migration or transition rule
`workers/reminders/processReminderAttempt.ts`	Collapses error handling into unified retry path	Loses hard-vs-soft failure distinction
`api/invoices/[invoiceId]/reminders.ts`	Adds manual requeue endpoint	Should verify auth and audit trail consistency

Behavioral risk

Risk matrix

Revenue reminder flow Tenant timezone logic Email deliverability behavior

High impact

Reminder suppression is hard to detect quickly

If reminders stop enqueuing after a transient failure, invoice owners may not notice until overdue balances rise. This makes the main regression both user-visible and operationally delayed.

Medium impact

Timezone changes may generate support churn

Tenants with carefully configured reminder cadences may perceive the new schedule as unpredictable unless release notes explain the shift.

Missing tests

Coverage gaps

Queue failure path Timezone migration behavior Hard bounce retry suppression

1. Enqueue failure should not advance scheduler watermark

Add an integration test where queue publish rejects after DB write begins. Assert invoice remains eligible for the next sweep.

2. Existing reminders should preserve send date after timezone rollout

Add a migration-style fixture covering tenants in London, Berlin, and Sydney with existing UTC timestamps.

3. Hard bounce should disable future retries immediately

Unit test error classification so a permanent SMTP bounce does not consume the same retry path as a temporary provider outage.

Open questions

What to clarify before merge

Do we need backfill logic for already-scheduled reminders?

The PR description says "future reminders will respect local time," but does not explain how already persisted timestamps should behave.

Was the enqueue watermark order intentionally changed?

If yes, the PR should explain what prevents missed reminders when the queue is unavailable.

Should analytics renaming happen in the same PR?

Bundling event taxonomy changes into reminder scheduling refactor makes it harder to validate downstream dashboards.

Final recommendation

Merge only after the enqueue ordering issue is fixed

The direction is sound, but the queue publish ordering looks merge-blocking. I would ask for a focused fix plus the missing regression tests before approving. The timezone change can still land in the same PR if the migration behavior is documented and covered by tests.