Repository navigation
Record a suppressed email when a send is refused for all-suppressed recipients - #129
Conversation
…ecipients POST /emails to recipients that are all suppressed still answers 422 all_recipients_suppressed, but now also leaves a metadata-only row (status suppressed, one suppressed event, no body, no quota charge, no send), so the attempt appears in the dashboard and GET /emails as it does at Resend. Single sends only: batch items keep failing at validation, and the platform's own account mail (uncapped) is not recorded. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks @heliocosta-dev for looking at this, and for #130. I really appreciate the time. While re-reading my change I noticed something I missed: If you think that's worth doing, I'd be glad to add it with a test, and to rebase onto |
heliocosta-dev
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @Sienz16, and for such a careful write-up. The diff is clean and your numbers check out. Lint, typecheck and the full test suite (including apps/web) pass on your branch and when merged with the current main. Your first new test also fails on the old code, as you said.
On your comment: yes to both. Making the insert best-effort, with a test, is point 3 below. Please bring main into your branch before the other changes, because #130 rewrote the doc sentences that point 4 touches and the SMTP tests that point 5 adds to. A merge or a rebase both work, since we merge PRs with a merge commit.
The idea fits. Resend keeps these attempts as emails with last_event: "suppressed", and seeing them in the Emails list, searchable by recipient, is useful. One small correction to the premise: refused POST /emails calls already show on the dashboard's Logs page with the 422, and admins can open each one to see its recipients and subject. They just aren't in Emails or GET /emails.
A few changes before we merge:
- Record only where the caller asks for it.
acceptEmailalso serves the SMTP relay, every inbox send and the dashboard's broadcast test send, so the!opts.tx && auth.billing !== "uncapped"guard records refusals from those too. Inbox sends include the worker's automatic inbox forwarding, which would turn inbound mail intoFwd:rows in the outbound list. Could you add an explicit option, for examplerecordSuppressed?: booleandocumented likecompleteInTx, and pass it only fromPOST /emailsfor now? That replaces the current guard. Its comment isn't accurate anyway: accepted account mail does land in the owning team's emails, and the onboarding first-email claim passesopts.txtoo. We can turn the other surfaces on later, one at a time. - Leave topic opt-outs out. Topic opt-outs go into the same
suppressedset. On a topic whose default is off, every recipient who isn't a contact counts as opted out. Recording those would add asuppressedrow for every send to a non-subscriber, for apps that send everything with atopic_idand let us filter, and the dashboard would describe each one as "future sends blocked". Please record only when the suppression list alone emptiesto. Keepsource: "api", which the dashboard keys on since #130, and changereasontorecipient_suppressed, the value the worker writes for the same case. Resend doesn't call a topic opt-outsuppressedeither: it marks itfailed. - Make the write best-effort, as you suggested: wrap the insert in a try/catch and still return
all_suppressed. Log only the error's name and SQLSTATE, as the request-log writer inapps/api/src/app.tsdoes, because a failed insert's message quotes its values, recipients included. - Docs. In
concepts/suppressions.mdxandapi-reference/index.mdx, please say that aPOST /emailsrefused because the suppression list emptiestokeeps a metadata-onlysuppressedemail, and that each retry with the sameIdempotency-Keyadds another one. A topic opt-out, a batch item and the SMTP relay keep nothing. The Resend migration guide (migrate-from-resend.mdx, and the agent prompt inapps/docs/content/prompts/migrate-from-resend.md) should then say we keep the email too, but answer 422 and send noemail.suppressed. Each page has a.pt-BRtwin that needs the same change. We're happy to write any of these if you'd rather not. - Tests. The new batch test relies on your first new test having suppressed
gone@example.com, so it fails when run on its own. Please seed the suppression inside it. Please also add two cases that should leave no row: a topic send whosetorecipients all opted out, and an SMTP refusal (the relay answers550; seeapps/smtp/test/smtp.e2e.test.ts). Thereview-hold.api.test.tschange you flagged is right, and it stays right after these changes: that refusedPOST /emailskeeps its row like any other. - Small things. Reuse the
nowthatacceptEmailalready has, and build the purged columns withpurgedEmailBodyColumns(now)fromemail-retention.tsinstead of settingbodyPurgedAtby hand.
On our side, #130 (released in v0.14.0) prepares the dashboard for these rows: on a row with that source: "api" event, the content panel says no content was stored instead of "Body purged after retention window".
On the questions in your description:
- 200 instead of 422: keep the 422. It's a documented, deliberate difference, and every SDK reports it as
all_recipients_suppressed. We also don't emitemail.suppressed, so a 200 would make the refusal silent. - Usage or send-rate stats: no. Nothing is sent, so these rows stay out of quota, usage and rate math. Your test already checks the usage counter.
- Retention: the same as other email metadata. The existing purge already covers them.
- Where it lives:
acceptEmailis the right place, since it knows which recipients were dropped and why. The opt-in from point 1 leaves the decision to each caller, which fits how the function already splits responsibilities.
Thanks again, and ask away if anything is unclear.
Address review on MillionSend#129: - Add recordSuppressed option to acceptEmail; only POST /emails passes it, replacing the tx/uncapped guard. - Record only when the suppression list alone empties `to`, not topic opt-outs. Event reason is now recipient_suppressed. - Make the insert best-effort: log error name and SQLSTATE only, still return all_suppressed. - Reuse acceptEmail's `now` and purgedEmailBodyColumns. - Docs (EN + pt-BR): suppressions, API reference, Resend migration guide and agent prompt. - Tests: seed suppression in the batch test; add topic opt-out, SMTP refusal and failed-insert cases. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks @heliocosta-dev, a clear and useful review. Pushed 070f152 (after merging the latest
Lint and typecheck are clean. The full Thanks for answering my four questions, and for #130. Ready for another look whenever you have time. |
heliocosta-dev
left a comment
There was a problem hiding this comment.
Thanks, @Sienz16, this is great work, and quick too. All six points are in, and I checked each one against the code. Only POST /emails records, only suppression-list hits count, and the write is best-effort with nothing sensitive in the log. Lint, typecheck and the full test suite (including apps/web) pass on your head, which already includes the latest main. We also mutated the code to check that the new tests catch regressions, and they do for the topic opt-out, the try/catch and the raw error.
We'll merge it as is. A few small follow-ups will come in a PR of our own, so you don't need another round:
- one shared helper for the "error name and SQLSTATE" log line, used here and by the request-log writer you copied it from;
- the caller's
tagson the recorded row, so the dashboard shows which flow sent it; - a stronger failed-insert test: a realistic error with a SQLSTATE, and a check that no recipient shows up in any logged argument. We'll also add a case with one list hit and one topic opt-out, and a same-key retry;
- covering topic sends on the suppressions page, and the Portuguese polish you asked for.
Thanks again for the careful write-up and for taking every point on board!
Hi MillionSend team, and thank you for building this. I'm moving an app from Resend to MillionSend and it has been a great experience so far.
I hit one small difference and tried a fix. I'm new to the codebase, so I'd really value your feedback. Please tell me if there is a better approach, or if I've missed something. I'm happy to change anything.
What I noticed
When every
torecipient of aPOST /emailsis suppressed, the API answers422 all_recipients_suppressedand creates nothing, so the attempt is invisible afterwards: no row in the Emails page, nothing inGET /emails. Resend records these as emails with statussuppressed, so it is easy to see that an app kept mailing a bounced address. With MillionSend, the first bounce is visible but later attempts disappear.What this PR does
In
packages/core/src/accept-email.ts, in the all-suppressed branch ofacceptEmail:emailsrow withlatest_status = "suppressed"(from, to, cc, bcc, reply-to, subject, topic, domain, API key) and oneemail_eventsrow of typesuppressed(data: { source: "api", reason: "all_recipients_suppressed" }), in one transaction.body_purged_atis set so the row reads as already purged.{ ok: false, reason: "all_suppressed" }, so the 422 body and error name are exactly as before and existing clients keep working.The
suppressedstatus, thesuppressedevent type and their dashboard rendering already exist (the worker uses them when a recipient becomes suppressed between accept and send), so no schema change and no web change were needed.What I left out on purpose
opts.tx): a batch fails an all-suppressed item at validation so earlier items are not left accepted. Recording from inside that transaction seemed to need its own design, so batch behavior is unchanged and pinned by a test.auth.billing === "uncapped", used bysystem-mail.ts): not recorded, so it never adds rows to a team's email list.email.suppressed) and quota accounting for these rows.Things worth knowing
Idempotency-Key.Tests
apps/api/test/emails.api.test.ts:suppressedrows with no body, noses_message_id, asuppressedevent, no enqueue and no change tousage_counters.accepted; the row reads back throughGET /emails/{id}withlast_event: "suppressed";suppressedrow;apps/api/test/review-hold.api.test.ts: it asserted that a refused send leaves no rows at all, and now expects the backlog plus exactly onesuppressedrow. This is the one existing assertion whose meaning changes, so I'd appreciate a close look at it.packages/core(1622),apps/api(727),apps/worker(459) andapps/smtp(32) tests pass, andbiome checkandtsc --noEmitare clean on the touched packages. I did not run theapps/webtests, since nothing there changed.Questions for you
I'd be grateful for your thoughts on any of these:
suppressedemail id (Resend-style 200) instead of the 422? I kept the 422 so existing clients don't break.acceptEmailthe right place for this, or would you rather it live in the API layer?Thanks for taking a look. I'm glad to rework it, split it up, or close it if it doesn't fit the project.