Skip to content

Record a suppressed email when a send is refused for all-suppressed recipients - #129

Merged
heliocosta-dev merged 3 commits into
MillionSend:mainfrom
Sienz16:feat/record-suppressed-sends
Oct 16, 2026
Merged

heliocosta-dev merged 3 commits into
MillionSend:mainfrom
Sienz16:feat/record-suppressed-sends

Conversation

@Sienz16

@Sienz16 Sienz16 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

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 to recipient of a POST /emails is suppressed, the API answers 422 all_recipients_suppressed and creates nothing, so the attempt is invisible afterwards: no row in the Emails page, nothing in GET /emails. Resend records these as emails with status suppressed, 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 of acceptEmail:

  • It inserts an emails row with latest_status = "suppressed" (from, to, cc, bcc, reply-to, subject, topic, domain, API key) and one email_events row of type suppressed (data: { source: "api", reason: "all_recipients_suppressed" }), in one transaction.
  • No body is stored; body_purged_at is set so the row reads as already purged.
  • No quota reservation, no enqueue, no SES call.
  • It still returns { ok: false, reason: "all_suppressed" }, so the 422 body and error name are exactly as before and existing clients keep working.

The suppressed status, the suppressed event 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

  • Batch sends (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.
  • Platform account mail (auth.billing === "uncapped", used by system-mail.ts): not recorded, so it never adds rows to a team's email list.
  • Webhooks (email.suppressed) and quota accounting for these rows.

Things worth knowing

  • Each refused attempt creates one row, including retries with the same Idempotency-Key.
  • Recipients are stored as sent, like any other email row.

Tests

  • New in apps/api/test/emails.api.test.ts:
    • two refused sends leave two suppressed rows with no body, no ses_message_id, a suppressed event, no enqueue and no change to usage_counters.accepted; the row reads back through GET /emails/{id} with last_event: "suppressed";
    • a partly suppressed send is accepted normally and records no suppressed row;
    • an all-suppressed batch item records nothing.
  • Updated 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 one suppressed row. This is the one existing assertion whose meaning changes, so I'd appreciate a close look at it.
  • I confirmed the first new test fails on the unchanged code.
  • packages/core (1622), apps/api (727), apps/worker (459) and apps/smtp (32) tests pass, and biome check and tsc --noEmit are clean on the touched packages. I did not run the apps/web tests, since nothing there changed.

Questions for you

I'd be grateful for your thoughts on any of these:

  1. Would you prefer a refused send to return the suppressed email id (Resend-style 200) instead of the 422? I kept the 422 so existing clients don't break.
  2. Should these rows count toward any usage or send-rate statistics? Right now they do not.
  3. Should they follow a different retention window than other email metadata?
  4. Is acceptEmail the 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.

…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>
@Sienz16

Sienz16 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

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: recordSuppressedAttempt is awaited with no error handling. If that insert ever fails, the client would get a 500 instead of the usual 422 all_recipients_suppressed. Since the row is only for visibility, I wonder if it would be better to log the failure and still return the 422. Please tell me if you see it differently.

If you think that's worth doing, I'd be glad to add it with a test, and to rebase onto main now that #130 has landed. Whatever works best for you, and I'm happy to change anything else. Thank you again!

@heliocosta-dev heliocosta-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Record only where the caller asks for it. acceptEmail also 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 into Fwd: rows in the outbound list. Could you add an explicit option, for example recordSuppressed?: boolean documented like completeInTx, and pass it only from POST /emails for 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 passes opts.tx too. We can turn the other surfaces on later, one at a time.
  2. Leave topic opt-outs out. Topic opt-outs go into the same suppressed set. On a topic whose default is off, every recipient who isn't a contact counts as opted out. Recording those would add a suppressed row for every send to a non-subscriber, for apps that send everything with a topic_id and let us filter, and the dashboard would describe each one as "future sends blocked". Please record only when the suppression list alone empties to. Keep source: "api", which the dashboard keys on since #130, and change reason to recipient_suppressed, the value the worker writes for the same case. Resend doesn't call a topic opt-out suppressed either: it marks it failed.
  3. 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 in apps/api/src/app.ts does, because a failed insert's message quotes its values, recipients included.
  4. Docs. In concepts/suppressions.mdx and api-reference/index.mdx, please say that a POST /emails refused because the suppression list empties to keeps a metadata-only suppressed email, and that each retry with the same Idempotency-Key adds 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 in apps/docs/content/prompts/migrate-from-resend.md) should then say we keep the email too, but answer 422 and send no email.suppressed. Each page has a .pt-BR twin that needs the same change. We're happy to write any of these if you'd rather not.
  5. 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 whose to recipients all opted out, and an SMTP refusal (the relay answers 550; see apps/smtp/test/smtp.e2e.test.ts). The review-hold.api.test.ts change you flagged is right, and it stays right after these changes: that refused POST /emails keeps its row like any other.
  6. Small things. Reuse the now that acceptEmail already has, and build the purged columns with purgedEmailBodyColumns(now) from email-retention.ts instead of setting bodyPurgedAt by 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:

  1. 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 emit email.suppressed, so a 200 would make the refusal silent.
  2. 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.
  3. Retention: the same as other email metadata. The existing purge already covers them.
  4. Where it lives: acceptEmail is 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.

Sienz16 and others added 2 commits October 16, 2026 21:29
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>
@Sienz16

Sienz16 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @heliocosta-dev, a clear and useful review. Pushed 070f152 (after merging the latest main, as you suggested):

  1. Opt-in only. acceptEmail takes recordSuppressed?: boolean, documented next to completeInTx. Only POST /emails passes it. The old tx/uncapped guard is gone.
  2. List only. A row is recorded only when the suppression list alone empties to. Topic opt-outs keep nothing. The event keeps source: "api" with reason: "recipient_suppressed".
  3. Best-effort write. The insert is wrapped, logs only error name + SQLSTATE, and still returns all_suppressed. A new test forces the insert to fail and checks the 422, no recipient in the log, and no row left.
  4. Docs. Updated suppressions, API reference, the Resend migration guide and its agent prompt, with the .pt-BR twins. A native check of the Portuguese would be welcome.
  5. Tests. The batch test now seeds its own suppression and passes on its own. Added the topic opt-out and SMTP 550 cases, and both leave no row.
  6. Small things. Reuses now and purgedEmailBodyColumns(now).

Lint and typecheck are clean. The full core, api, smtp and worker suites pass. I didn't run apps/web, since nothing there changed.

Thanks for answering my four questions, and for #130. Ready for another look whenever you have time.

@heliocosta-dev heliocosta-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tags on 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!

@heliocosta-dev
heliocosta-dev merged commit 5435df9 into MillionSend:main Oct 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants