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.

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

Multiple notifications for certificate failure #650

Closed
humphd opened this issue Apr 19, 2023 · 5 comments · Fixed by #667
Closed

Multiple notifications for certificate failure #650

humphd opened this issue Apr 19, 2023 · 5 comments · Fixed by #667
Assignees
Labels
bug Something isn't working category: notifications A service to notify users about their certificates/domains

Comments

@humphd
Copy link
Contributor

humphd commented Apr 19, 2023

Today on production, I received 2 identical notification messages for a failed certificate. This is likely due to having 2 instances running, and both queue and sent a notification.

It would be nice to only send a single notification.

@Eakam1007
Copy link
Contributor

I believe this is also happening for the expiration on production. Created a certificate today morning and received three emails for the expiration at the same time

@humphd
Copy link
Contributor Author

humphd commented Apr 20, 2023

Fixing this is going to be somewhat tricky. There are a few problems that all need to be addressed:

  1. We need to make sure that we don't create multiple jobs in the queue for the same notification
  2. We need to alter the jobs so that they confirm that the notification is still required before sending it (i.e., has the database changed since the job was enqueued?)
  3. We need to deal with the case that multiple, independent instances of the app (e.g., multiple workers) are both sharing the same queue, and not have them compete with each other. We can't solve this by limiting what a single worker will do, since we scale to multiple
  4. We need to use the database as our ground-truth, and decide based on it whether things are necessary or not.

@SerpentBytes
Copy link
Contributor

I think we are already doing number 1. for Notifications-related code. For number 2. would it help to check when a record was last modified using a new lastModified field?

@humphd
Copy link
Contributor Author

humphd commented Apr 22, 2023

For number 2. would it help to check when a record was last modified using a new lastModified field?

Maybe, but since we also store the date/time when we last notified the user vs. null, that is probably what we should use for this.

@SerpentBytes
Copy link
Contributor

Today on production, I received 2 identical notification messages for a failed certificate. This is likely due to having 2 instances running, and both queue and sent a notification.

It would be nice to only send a single notification.

If this is still an issue, what are the steps to reproduce this bug?

@SerpentBytes SerpentBytes added category: notifications A service to notify users about their certificates/domains bug Something isn't working labels Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working category: notifications A service to notify users about their certificates/domains
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants