Full sync: Ensure actions are processed in wpcom before marking modules as finished

[darssen]

Related to VULCAN-74

The Full Sync modules that don't use chunking and override the send_full_sync_actions method don't wait for a response when sending the full sync actions to be processed by wpcom. Example with updates.

This can create issues since the remote site continues the Full Sync without actually processing the items. The most common issue will be when in wpcom we detect concurrent requests (maybe due to a pull or some other reason).

Proposed changes:

• Check if the action has not been processed (return of sending the action is WP_Error) and not mark the module as finished if it has not.
• When the action is marked as finished, update the sent key too.

Other information:

• Have you written new tests for your changes, if applicable?
• Have you checked the E2E test CI results, and verified that your changes do not break them?
• Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Test that normal Full Sync works as expected

• In Debugger run a Full Sync with this branch and check that actions are still flowing as usual.

Run Full Sync not processing a non chunked module

• Sabotage the processing in WPCOM of specific full sync actions when processing the events

if ( 'jetpack_full_sync_constants' === $action_name ) {
	throw new InvalidArgumentException( 'We do not allow this action for now' );
}

• Check that the Full Sync does not proceed forward that module
• Remove the code in WPCOM and Full Sync shall proceed.
  The above in trunk will not stop the full sync progress, but the jetpack_full_sync_constants action would not have been processed.

Check that now we get total Sync actions sent after in Debugger for non-chunked modules

• Compare the results of the Status column when checking the Full Sync progress for modules like Constants, Functions, Options... in the Debugger.
• With trunk, it will say All sent, while now it will present the number of items synced 42 out of 42 sent.

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the update/full-sync-ensure-actions-are-processed-in-wpcom branch.
• To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/full-sync-ensure-actions-are-processed-in-wpcom

bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/full-sync-ensure-actions-are-processed-in-wpcom

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  
• ✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

Follow this PR Review Process:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
3. You can use GitHub's Reviewers functionality to request a review.
4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

[jp-launch-control]

Code Coverage Summary

Coverage changed in 6 files. Only the first 5 are listed here.

File	Coverage	Δ%	Δ Uncovered
projects/packages/sync/src/modules/class-callables.php	45/182 (24.73%)	-0.70%	5 💔
projects/packages/sync/src/modules/class-constants.php	7/66 (10.61%)	-0.87%	5 💔
projects/packages/sync/src/modules/class-network-options.php	18/42 (42.86%)	-5.79%	5 💔
projects/packages/sync/src/modules/class-options.php	33/110 (30.00%)	-1.43%	5 💔
projects/packages/sync/src/modules/class-themes.php	24/270 (8.89%)	-0.17%	5 💔

Full summary · PHP report · JS report

_{If appropriate, add one of these labels to override the failing coverage check: Covered by non-unit tests Use to ignore the Code coverage requirement check when E2Es or other non-unit tests cover the code Coverage tests to be added later Use to ignore the Code coverage requirement check when tests will be added in a follow-up PR I don't care about code coverage for this PR Use this label to ignore the check for insufficient code coveage.}

[coder-karen]

I'm not sure how reliable my review will be as my testing was inconsistent so I'll add this as a review comment instead.
I added the exception within a few places in WPcom to be sure it will generate the exception (eg. in process_event in jetpack/sync/class.jetpack-sync-event-processor.php, in maybe_perform_sync_pull in jetpack/sync/class.jetpack-sync-shadow-replicastore.php, in handle_remote_action in jetpack/sync/class.jetpack-sync-server-replicator.php) - only once it did not run anything, then continued after removing the code, another time it skipped constants and ran functions and options, then stopped, continuing when the code was removed.

Either way I realize it was an attempt to mimic WPcom slowing things down, and I can see that the full sync was stopping until the 'block' was removed, so in that respect it looks like it tests well.

Total sync actions in status column looks good!

[darssen]

I'm not sure how reliable my review will be as my testing was inconsistent so I'll add this as a review comment instead. I added the exception within a few places in WPcom to be sure it will generate the exception (eg. in process_event in jetpack/sync/class.jetpack-sync-event-processor.php, in maybe_perform_sync_pull in jetpack/sync/class.jetpack-sync-shadow-replicastore.php, in handle_remote_action in jetpack/sync/class.jetpack-sync-server-replicator.php) - only once it did not run anything, then continued after removing the code, another time it skipped constants and ran functions and options, then stopped, continuing when the code was removed.

Either way I realize it was an attempt to mimic WPcom slowing things down, and I can see that the full sync was stopping until the 'block' was removed, so in that respect it looks like it tests well.

Total sync actions in status column looks good!

Thanks for reviewing @coder-karen!

Yeah, sorry I was not very explicit with the sabotaging. I was referring to the first scenario you mentioned, in process_event in jetpack/sync/class.jetpack-sync-event-processor.php. In that scenario, it should run any module that is sent prior to Constants and then stop there and not continue.

[coder-karen]

Thanks for clarifying :) I tested more, tests as expected.

it should run any module that is sent prior to Constants and then stop there and not continue.

Noting that we discussed that in Slack, and the order in which modules show in the debugger doesn't necessarily match the order in which they're sent (hence constants appearing to be skipped and the two following modules showing as synced), so by getting that result in testing that was expected.

[fgiannar]

LGTM