Fix multiple scheduled update sync issue (#37266)

* Add cron changing filter * changelog * Add base test * Add unit tests * Remove debug code * Add new unit tests * Fix: typo
Automattic · May 8, 2024 · d928d58 · d928d58
1 parent 0f6522a
commit d928d58
Show file tree

Hide file tree

Showing 4 changed files with 147 additions and 0 deletions.
diff --git a/projects/packages/scheduled-updates/changelog/fix-su-multiple-sync-issue b/projects/packages/scheduled-updates/changelog/fix-su-multiple-sync-issue
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fixed
+
+Fix multiple sync issue
diff --git a/projects/packages/scheduled-updates/src/class-scheduled-updates.php b/projects/packages/scheduled-updates/src/class-scheduled-updates.php
@@ -29,6 +29,13 @@ class Scheduled_Updates {
 	 */
 	const PLUGIN_CRON_HOOK = 'jetpack_scheduled_plugins_update';
 
+	/**
+	 * The cron event filter for the scheduled plugins update.
+	 *
+	 * @var string
+	 */
+	const PLUGIN_CRON_SYNC_HOOK = 'jetpack_scheduled_plugins_update_sync';
+
 	/**
 	 * Initialize the class.
 	 */
@@ -195,6 +202,11 @@ public static function delete_scheduled_update( $timestamp, $plugins ) {
 	 * Save the schedules for sync after cron option saving.
 	 */
 	public static function update_option_cron() {
+		// Do not update the option if the filter returns false.
+		if ( ! apply_filters( self::PLUGIN_CRON_SYNC_HOOK, true ) ) {
+			return;
+		}
+
 		Scheduled_Updates_Logs::add_log_fields();
 		Scheduled_Updates_Active::add_active_field();
 		Scheduled_Updates_Health_Paths::add_health_check_paths_field();

diff --git a/...heduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php b/...heduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php
@@ -278,11 +278,17 @@ public function update_item( $request ) {
 			return $verified_plugins;
 		}
 
+		// Prevent the sync option to be updated during deletion. This will ensure that the sync is performed only once.
+		// Context: https://github.com/Automattic/jetpack/issues/27763
+		add_filter( Scheduled_Updates::PLUGIN_CRON_SYNC_HOOK, '__return_false' );
 		$deleted = $this->delete_item( $request );
+
 		if ( is_wp_error( $deleted ) ) {
 			return $deleted;
 		}
 
+		// Re-enable the sync option before creation.
+		remove_filter( Scheduled_Updates::PLUGIN_CRON_SYNC_HOOK, '__return_false' );
 		$item = $this->create_item( $request );
 
 		/**

diff --git a/...es/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php b/...es/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php
@@ -28,6 +28,13 @@ class WPCOM_REST_API_V2_Endpoint_Update_Schedules_Test extends \WorDBless\BaseTe
 	 */
 	public $editor_id;
 
+	/**
+	 * Sync counter.
+	 *
+	 * @var int
+	 */
+	public static $sync_counter;
+
 	/**
 	 * Set up.
 	 */
@@ -52,6 +59,9 @@ public function set_up() {
 		add_filter( 'jetpack_scheduled_update_verify_plugins', '__return_true', 11 );
 
 		Scheduled_Updates::init();
+
+		self::$sync_counter = 0;
+		add_action( 'update_option', array( __CLASS__, 'sync_callback' ) );
 	}
 
 	/**
@@ -67,6 +77,9 @@ public function tear_down() {
 		delete_option( 'jetpack_scheduled_update_statuses' );
 		delete_option( Scheduled_Updates::PLUGIN_CRON_HOOK );
 		remove_filter( 'jetpack_scheduled_update_verify_plugins', '__return_true', 11 );
+
+		self::$sync_counter = 0;
+		remove_action( 'update_option', array( __CLASS__, 'sync_callback' ) );
 	}
 
 	/**
@@ -136,6 +149,7 @@ public function test_get_items() {
 			),
 			$result->get_data()
 		);
+		$this->assertSame( 2, self::$sync_counter );
 	}
 
 	/**
@@ -196,6 +210,7 @@ public function test_create_item() {
 
 		$this->assertSame( 403, $result->get_status() );
 		$this->assertSame( 'rest_forbidden', $result->get_data()['code'] );
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -249,6 +264,7 @@ public function test_create_multiple_item() {
 		$this->assertIsArray( $sync_option );
 		$this->assertIsObject( $sync_option[ $schedule_id ] );
 		$this->assertIsObject( $sync_option[ $schedule_id_2 ] );
+		$this->assertSame( 2, self::$sync_counter );
 	}
 
 	/**
@@ -282,6 +298,7 @@ public function test_init_backward_compatibility() {
 		Scheduled_Updates::init();
 		$post_sync_option = get_option( Scheduled_Updates::PLUGIN_CRON_HOOK );
 		$this->assertEquals( $pre_sync_option, $post_sync_option );
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -311,6 +328,7 @@ public function test_creating_schedules_for_same_time() {
 
 		$this->assertSame( 403, $result->get_status() );
 		$this->assertSame( 'rest_forbidden', $result->get_data()['code'] );
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -339,6 +357,7 @@ public function test_updating_autoupdate_plugins_on_create() {
 		rest_do_request( $request );
 
 		$this->assertEquals( $unscheduled_plugins, get_option( 'auto_update_plugins' ) );
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -385,6 +404,7 @@ public function test_empty_last_run() {
 			),
 			$result->get_data()
 		);
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -419,6 +439,7 @@ public function test_creating_schedule_with_more_than_ten_plugins() {
 		$result = rest_do_request( $request );
 		$this->assertSame( 400, $result->get_status() );
 		$this->assertSame( 'rest_invalid_param', $result->get_data()['code'] );
+		$this->assertSame( 0, self::$sync_counter );
 	}
 
 	/**
@@ -468,6 +489,7 @@ public function test_get_item() {
 			),
 			$result->get_data()
 		);
+		$this->assertSame( 1, self::$sync_counter );
 	}
 
 	/**
@@ -483,6 +505,7 @@ public function test_get_invalid_item() {
 
 		$this->assertSame( 404, $result->get_status() );
 		$this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] );
+		$this->assertSame( 0, self::$sync_counter );
 	}
 
 	/**
@@ -530,6 +553,7 @@ public function test_update_item() {
 		$this->assertIsArray( $sync_option );
 		$this->assertIsObject( $sync_option[ $schedule_id ] );
 		$this->assertSame( $plugins, $sync_option[ $schedule_id ]->args );
+		$this->assertSame( 2, self::$sync_counter );
 	}
 
 	/**
@@ -548,6 +572,7 @@ public function test_update_item_with_status() {
 		$schedule_id = Scheduled_Updates::generate_schedule_id( $plugins );
 
 		wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins );
+		$this->assertSame( 1, self::$sync_counter );
 
 		// Log a start and success.
 		Scheduled_Updates_Logs::log(
@@ -564,6 +589,7 @@ public function test_update_item_with_status() {
 			null,
 			$timestamp
 		);
+		$this->assertSame( 3, self::$sync_counter );
 
 		$request = new WP_REST_Request( 'PUT', '/wpcom/v2/update-schedules/' . $schedule_id );
 		$request->set_body_params(
@@ -597,6 +623,7 @@ public function test_update_item_with_status() {
 			$this->assertSame( $plugins, $sync_option[ $schedule_id ]->args );
 			$this->assertSame( $timestamp, $sync_option[ $schedule_id ]->last_run_timestamp );
 			$this->assertSame( $status, $sync_option[ $schedule_id ]->last_run_status );
+			$this->assertSame( 4, self::$sync_counter );
 		}
 	}
 
@@ -619,6 +646,7 @@ public function test_update_invalid_item() {
 
 		$this->assertSame( 404, $result->get_status() );
 		$this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] );
+		$this->assertSame( 0, self::$sync_counter );
 	}
 
 	/**
@@ -634,6 +662,7 @@ public function test_delete_item() {
 		$schedule_id = Scheduled_Updates::generate_schedule_id( $plugins );
 
 		wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins );
+		$this->assertSame( 1, self::$sync_counter );
 
 		// Unauthenticated request.
 		wp_set_current_user( 0 );
@@ -649,6 +678,7 @@ public function test_delete_item() {
 
 		$this->assertSame( 403, $result->get_status() );
 		$this->assertSame( 'rest_forbidden', $result->get_data()['code'] );
+		$this->assertSame( 1, self::$sync_counter );
 
 		// Successful request.
 		wp_set_current_user( $this->admin_id );
@@ -661,6 +691,7 @@ public function test_delete_item() {
 
 		$sync_option = get_option( Scheduled_Updates::PLUGIN_CRON_HOOK );
 		$this->assertSame( array(), $sync_option );
+		$this->assertSame( 2, self::$sync_counter );
 	}
 
 	/**
@@ -676,6 +707,7 @@ public function test_delete_invalid_item() {
 
 		$this->assertSame( 404, $result->get_status() );
 		$this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] );
+		$this->assertSame( 0, self::$sync_counter );
 	}
 
 	/**
@@ -703,12 +735,105 @@ public function test_updating_autoupdate_plugins_on_delete() {
 		wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins_2 );
 
 		update_option( 'auto_update_plugins', $auto_update );
+		$this->assertSame( 2, self::$sync_counter );
 
 		$request = new WP_REST_Request( 'DELETE', '/wpcom/v2/update-schedules/' . $schedule_id );
 		wp_set_current_user( $this->admin_id );
 		rest_do_request( $request );
 
 		$this->assertSame( $expected_result, get_option( 'auto_update_plugins' ) );
+		$this->assertSame( 3, self::$sync_counter );
+	}
+
+	/**
+	 * A CRUD cycle should sync only three times.
+	 *
+	 * @covers ::create_item
+	 * @covers ::get_item
+	 * @covers ::update_item
+	 * @covers ::delete_item
+	 */
+	public function test_crud_should_sync_only_three_times() {
+		wp_set_current_user( $this->admin_id );
+		$plugins       = array(
+			'custom-plugin/custom-plugin.php',
+			'gutenberg/gutenberg.php',
+		);
+		$request       = new WP_REST_Request( 'POST', '/wpcom/v2/update-schedules' );
+		$base_schedule = $this->get_schedule();
+		$request->set_body_params(
+			array(
+				'plugins'  => $plugins,
+				'schedule' => $base_schedule,
+			)
+		);
+
+		// Create.
+		$schedule_id = Scheduled_Updates::generate_schedule_id( $plugins );
+		$result      = rest_do_request( $request );
+
+		$this->assertSame( 200, $result->get_status() );
+		$this->assertSame( $schedule_id, $result->get_data() );
+		// First sync.
+		$this->assertSame( 1, self::$sync_counter );
+
+		// Read.
+		$request = new WP_REST_Request( 'GET', '/wpcom/v2/update-schedules/' . $schedule_id );
+		$result  = rest_do_request( $request );
+		$data    = $result->get_data();
+
+		$this->assertSame( 200, $result->get_status() );
+		$this->assertIsArray( $data );
+		$this->assertSame( $base_schedule['timestamp'], $data['timestamp'] );
+		// No sync during read.
+		$this->assertSame( 1, self::$sync_counter );
+
+		// Update.
+		$request       = new WP_REST_Request( 'PUT', '/wpcom/v2/update-schedules/' . $schedule_id );
+		$base_schedule = $this->get_schedule( 'next Monday 9:00' );
+		$request->set_body_params(
+			array(
+				'plugins'  => $plugins,
+				'schedule' => $base_schedule,
+			)
+		);
+
+		$result = rest_do_request( $request );
+		$data   = $result->get_data();
+
+		$this->assertSame( 200, $result->get_status() );
+		$this->assertSame( $schedule_id, $result->get_data() );
+
+		// Count here should be 2 despite the fact that the schedule is deleted and added.
+		$this->assertSame( 2, self::$sync_counter );
+
+		// Read again.
+		$request = new WP_REST_Request( 'GET', '/wpcom/v2/update-schedules/' . $schedule_id );
+		$result  = rest_do_request( $request );
+		$data    = $result->get_data();
+
+		$this->assertSame( $base_schedule['timestamp'], $data['timestamp'] );
+		// No sync during read.
+		$this->assertSame( 2, self::$sync_counter );
+
+		// Delete.
+		$request = new WP_REST_Request( 'DELETE', '/wpcom/v2/update-schedules/' . $schedule_id );
+		$result  = rest_do_request( $request );
+
+		$this->assertTrue( $result->get_data() );
+		// One sync during delete.
+		$this->assertSame( 3, self::$sync_counter );
+	}
+
+	/**
+	 * A callback run when a sync is performed.
+	 *
+	 * @param string $option Option name.
+	 */
+	public static function sync_callback( $option ) {
+		if ( Scheduled_Updates::PLUGIN_CRON_HOOK === $option ) {
+			++self::$sync_counter;
+		}
 	}
 
 	/**