Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

We're re-sending the subscriptions again to all recipients when 1 recipient fails #47961

Open
paoliniluis opened this issue Sep 16, 2024 · 5 comments
Open

We're re-sending the subscriptions again to all recipients when 1 recipient fails #47961

paoliniluis opened this issue Sep 16, 2024 · 5 comments
Labels
.Backend Priority:P2 Average run of the mill bug Reporting/Pulses Now called Subscriptions .Team/BackendComponents also known as BEC Type:Bug Product defects

Comments

@paoliniluis
Copy link
Contributor

Describe the bug

A customer reported that they got the same email over and over again on the same day. The issue is that we simply retry all recipients when a single one fails, causing lots of emails to be sent.

E.g. imagine that a subscription has 3 recipients: a, b and c. If the recipient b fails, then we retry again a,b,c, causing a to receive the subscription twice

To Reproduce

This is a theoretical reproduction since I'm not able to repro unless I cause a failure to the email gateway:

  1. set up metabase with an smtp server
  2. make a dashboard and a subscription adding 3 recipients
  3. when you send the test subscription, cut the connection to the email server

you should see the issue and A should have 2 emails, while b and c only one

Expected behavior

We should keep track of every recipient to send the emails only once

Logs

NA

Information about your Metabase installation

v50

Severity

P1

Additional context

Why did emails start failing on v50 more?
@ixipixi ixipixi added the Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness label Sep 16, 2024
@ranquild ranquild added Reporting/Pulses Now called Subscriptions .Backend .Team/DashViz Dashboard and Viz team and removed .Needs Triage labels Sep 18, 2024
@qnkhuat qnkhuat added the .Team/BackendComponents also known as BEC label Sep 19, 2024
@qnkhuat qnkhuat self-assigned this Sep 19, 2024
@qnkhuat qnkhuat removed the .Team/DashViz Dashboard and Viz team label Sep 19, 2024
@qnkhuat
Copy link
Contributor

qnkhuat commented Sep 19, 2024

Can we check if all recipient emails are valid?

@ixipixi ixipixi added Priority:P2 Average run of the mill bug and removed Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness labels Sep 19, 2024
@qnkhuat qnkhuat removed their assignment Sep 19, 2024
@qnkhuat
Copy link
Contributor

qnkhuat commented Sep 19, 2024

Might be useful to backport #46218 to 50, it records error of each retry failure

@ixipixi
Copy link
Contributor

ixipixi commented Sep 23, 2024

Maybe related: #47949

@ixipixi
Copy link
Contributor

ixipixi commented Oct 31, 2024 •
edited
Loading

Relevant because we send a lot of unnecessary emails:

@ixipixi
Copy link
Contributor

ixipixi commented Oct 31, 2024

We have cloud customers hitting rate limits:

Error sending to email channel. Retrying...","exception":{"exception_class":"com.sun.mail.smtp.SMTPSendFailedException","exception_message":"454 Throttling failure: Maximum sending rate exceeded...

This behavior is very likely compounding the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
.Backend Priority:P2 Average run of the mill bug Reporting/Pulses Now called Subscriptions .Team/BackendComponents also known as BEC Type:Bug Product defects
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@paoliniluis @ranquild @ixipixi @qnkhuat