fix: automatically retry failed scheduled sends #1660

dkoo · 2024-09-16T18:37:20Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Implements an auto-retry system for scheduled newsletter sends, and adds logging for scheduled send failures.

#831 fixed handling for failed scheduled sends by resetting the post to draft status if a scheduled send failed to happen, but by resetting to draft status the newsletter must be manually sent or rescheduled. This PR will attempt to automatically retry the send in exponentially increasing intervals up to a maximum of 10 times.

How to test the changes in this Pull Request:

Check out this branch.
Add return new WP_Error( 'newspack_newsletters_error', 'Temporary error' ); to the first line of the Newspack_Newsletters_Service_Provider::send_newsletter() method to force a failure upon scheduled send.
Schedule a newsletter to be sent at some point in the near future.
Refresh the newsletter in the editor after the scheduled send time has passed and confirm you see an error message about the failed send at the top of the editor that says "Error sending newsletter: Temporary error". Also confirm that the post remains in "Future" status with a scheduled send time of some minutes in the future.
Wait a while or bump the MAX_SCHEDULED_RETRIES number down to something like 3 and wait a bit less, then refresh the newsletter in the editor again. Confirm that the post has reverted to "Draft" status with the error message "Failed to send 10 times. Please check the provider connection and try sending again."
Schedule the newsletter to be sent again at some point in the future, then wait for it to fail once.
Remove the temporary error message you added in step 2 and wait for the next attempt—this time confirm the scheduled send succeeds and that the newsletter post successfully transitions to "Private" or "Published" status.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

codecov · 2024-09-16T18:50:18Z

Codecov Report

Attention: Patch coverage is 28.98551% with 49 lines in your changes missing coverage. Please review.

Project coverage is 19.57%. Comparing base (b245716) to head (ad2246c).
Report is 82 commits behind head on release.

Files with missing lines	Patch %	Lines
...rs/class-newspack-newsletters-service-provider.php	28.98%	49 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             release    #1660      +/-   ##
=============================================
+ Coverage      17.74%   19.57%   +1.82%     
- Complexity      2316     2389      +73     
=============================================
  Files             43       45       +2     
  Lines           8758     9029     +271     
=============================================
+ Hits            1554     1767     +213     
- Misses          7204     7262      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

miguelpeixe

When a newsletter send fails, it'll always send an email to the admin. Given that we're now adding a new retry layer for scheduled newsletters, it's probably best to suppress these emails until the attempts exhaust, otherwise they might get 5 emails regarding the same newsletter. More confusingly, they might get an email when the first attempt fails, only to get resolved by the second attempt.

includes/service-providers/class-newspack-newsletters-service-provider.php

miguelpeixe · 2024-09-16T19:24:30Z

includes/service-providers/class-newspack-newsletters-service-provider.php

 		if ( in_array( $new_status, self::$controlled_statuses, true ) && 'future' === $old_status ) {
 			update_post_meta( $post->ID, 'sending_scheduled', true );
 			$result = $this->send_newsletter( $post );
 			if ( is_wp_error( $result ) ) {
+				$prior_attempts = intval( get_post_meta( $post->ID, 'scheduled_send_attempts', true ) );


Why not leverage the existing newsletter_send_errors meta for this? It's persisted and each error entry is already an attempt.

That's a good idea!

f430275 changes the approach to use the newsletter_send_errors meta.

miguelpeixe · 2024-09-16T19:47:20Z

It might be a good idea to add some exponential backoff. If the first attempt failed because of a server hiccup, it would be fine to try again sooner, like 1 minute, then exponentially try on 2, 4, and 8 minutes. We use something similar for webhooks:

https://github.com/Automattic/newspack-plugin/blob/6e8d7205852f82367fe49c748aebb8a9d09491c9/includes/data-events/class-webhooks.php#L763-L765

miguelpeixe · 2024-09-16T20:08:24Z

I left the attempts running while I reviewed, after it exhausted all 5 attempts the newsletter ended up being marked as sent 🤔

Co-authored-by: Miguel Peixe <miguel.peixe@automattic.com>

dkoo · 2024-09-17T22:48:34Z

@miguelpeixe thanks for the suggestions!

I left the attempts running while I reviewed, after it exhausted all 5 attempts the newsletter ended up being marked as sent 🤔

f430275 should fix this. It was happening because we were deleting the sending_scheduled meta when we hit max retries.

When a newsletter send fails, it'll always send an email to the admin. Given that we're now adding a new retry layer for scheduled newsletters, it's probably best to suppress these emails until the attempts exhaust, otherwise they might get 5 emails regarding the same newsletter. More confusingly, they might get an email when the first attempt fails, only to get resolved by the second attempt.

f430275 also addresses this. If the post has a sending_scheduled meta, it will only send an email after the last attempt, and it will include all error messages in the email body. If the post doesn't have the meta, it means it failed on a regular send attempt, so the email will get sent. We could consider implementing the retry system for regular sends too, but I think that's out of scope for this PR.

It might be a good idea to add some exponential backoff. If the first attempt failed because of a server hiccup, it would be fine to try again sooner, like 1 minute, then exponentially try on 2, 4, and 8 minutes.

bbb656b implements the exponential backoff and also bumps the max retries up to 10, because why not?

miguelpeixe

Awesome! It's working like a charm now. Thank you for the revisions.

miguelpeixe · 2024-09-18T13:42:14Z

Whoops, seems like something is up with the email notification.

When sending regularly, the error message includes the link to edit the newsletter. When scheduled, the link is missing:

That's because get_edit_post_link() returns null if current_user_can( 'edit_post', $post->ID ) is false, which will be the case in a cron/scheduled action. Since there's also a small bug, unrelated to this PR, in which the link is currently broken because it's formatted with & instead of & due to the default $context second argument, I think it's ok to switch to admin_url( 'post.php?post=' . $post_id . '&action=edit' );. This will fix the existing bug and allow the scheduled action to retrieve the URL.

dkoo · 2024-09-18T16:02:09Z

@miguelpeixe ad2246c makes the suggested change and seems to work well for me.

## [3.1.6](v3.1.5...v3.1.6) (2024-09-18) ### Bug Fixes * automatically retry failed scheduled sends ([#1660](#1660)) ([e0cca64](e0cca64))

matticbot · 2024-09-18T16:51:08Z

🎉 This PR is included in version 3.1.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [3.2.0-alpha.4](v3.2.0-alpha.3...v3.2.0-alpha.4) (2024-09-20) ### Bug Fixes * a typo that came after a refactor ([#1656](#1656)) ([0ae5b61](0ae5b61)) * automatically retry failed scheduled sends ([#1660](#1660)) ([e0cca64](e0cca64)) * **cc:** urlencode query args before passing to CC API ([#1659](#1659)) ([4f645d2](4f645d2)) * dont send metadata on subscribe ([#1648](#1648)) ([5b4a059](5b4a059)) * handle encoded URLs and block patterns in redirect check ([#1635](#1635)) ([2fa5d16](2fa5d16)) * **mailchimp:** allow contacts to resubscribe after unsubscribing ([#1654](#1654)) ([b76dbc2](b76dbc2)) ### Features * **mailchimp:** parse contact name into merge fields ([#1628](#1628)) ([aeba65a](aeba65a))

matticbot · 2024-09-20T18:22:19Z

🎉 This PR is included in version 3.2.0-alpha.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

fix: automatically retry failed scheduled sends

84f24e8

dkoo added the [Status] Needs Review label Sep 16, 2024

dkoo self-assigned this Sep 16, 2024

dkoo requested a review from a team as a code owner September 16, 2024 18:37

fix: undefined variable

82c032c

miguelpeixe requested changes Sep 16, 2024

View reviewed changes

dkoo and others added 3 commits September 17, 2024 15:38

chore: improve error message

48df1ea

Co-authored-by: Miguel Peixe <miguel.peixe@automattic.com>

fix: set to draft and send admin email after max attempts

f430275

fix: exponential backoff and bump max attempts to 10

bbb656b

dkoo requested a review from miguelpeixe September 17, 2024 22:48

miguelpeixe approved these changes Sep 18, 2024

View reviewed changes

github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Sep 18, 2024

fix: broken post edit link in error email

ad2246c

dkoo requested a review from miguelpeixe September 18, 2024 16:02

miguelpeixe approved these changes Sep 18, 2024

View reviewed changes

dkoo merged commit e0cca64 into release Sep 18, 2024
9 checks passed

dkoo deleted the fix/retry-failed-scheduled-sends branch September 18, 2024 16:47

matticbot pushed a commit that referenced this pull request Sep 18, 2024

chore(release): 3.1.6 [skip ci]

8fed09d

## [3.1.6](v3.1.5...v3.1.6) (2024-09-18) ### Bug Fixes * automatically retry failed scheduled sends ([#1660](#1660)) ([e0cca64](e0cca64))

matticbot added the released label Sep 18, 2024

matticbot added the released on @alpha label Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: automatically retry failed scheduled sends #1660

fix: automatically retry failed scheduled sends #1660

dkoo commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

miguelpeixe left a comment

miguelpeixe Sep 16, 2024

dkoo Sep 16, 2024

dkoo Sep 17, 2024

miguelpeixe commented Sep 16, 2024

miguelpeixe commented Sep 16, 2024

dkoo commented Sep 17, 2024 •

edited

Loading

miguelpeixe left a comment

miguelpeixe commented Sep 18, 2024 •

edited

Loading

dkoo commented Sep 18, 2024

matticbot commented Sep 18, 2024

matticbot commented Sep 20, 2024

fix: automatically retry failed scheduled sends #1660

fix: automatically retry failed scheduled sends #1660

Conversation

dkoo commented Sep 16, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

miguelpeixe left a comment

Choose a reason for hiding this comment

miguelpeixe Sep 16, 2024

Choose a reason for hiding this comment

dkoo Sep 16, 2024

Choose a reason for hiding this comment

dkoo Sep 17, 2024

Choose a reason for hiding this comment

miguelpeixe commented Sep 16, 2024

miguelpeixe commented Sep 16, 2024

dkoo commented Sep 17, 2024 • edited Loading

miguelpeixe left a comment

Choose a reason for hiding this comment

miguelpeixe commented Sep 18, 2024 • edited Loading

dkoo commented Sep 18, 2024

matticbot commented Sep 18, 2024

matticbot commented Sep 20, 2024

dkoo commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

dkoo commented Sep 17, 2024 •

edited

Loading

miguelpeixe commented Sep 18, 2024 •

edited

Loading