Add in-memory cache to DB cache to deal with DB write failures (#9639)

Co-authored-by: Anurag Bhandari <[email protected]>

changelog/update-add-in-memory-cache-to-db-cache-to-deal-with-db-write-failures

@@ -0,0 +1,4 @@
+Significance: patch
+Type: update
+
+Add in-memory cache fallback for our database-cached objects in case of database write failures.

includes/class-database-cache.php

@@ -83,6 +83,16 @@ class Database_Cache implements MultiCurrencyCacheInterface {
 	 */
 	private $refresh_disabled;
 
+	/**
+	 * In-memory cache for the duration of a single request.
+	 *
+	 * This is used to avoid multiple database reads for the same data and as a backstop in case the database write fails,
+	 * thus ensuring the cache generator is not called multiple times (which would mean multiple API calls to our platform).
+	 *
+	 * @var array
+	 */
+	private $in_memory_cache = [];
+
 	/**
 	 * Class constructor.
 	 */
@@ -108,10 +118,10 @@ public function init_hooks() {
 	 * @param boolean  $force_refresh Regenerates the cache regardless of its state if true.
 	 * @param boolean  $refreshed     Is set to true if the cache has been refreshed without errors and with a non-empty value.
 	 *
-	 * @return mixed|null The cache contents. NULL on failure
+	 * @return mixed|null The cached value. NULL on failure to regenerate or validate the data.
 	 */
 	public function get_or_add( string $key, callable $generator, callable $validate_data, bool $force_refresh = false, bool &$refreshed = false ) {
-		$cache_contents = get_option( $key );
+		$cache_contents = $this->get_from_cache( $key );
 		$data           = null;
 		$old_data       = null;
 
@@ -123,11 +133,9 @@ public function get_or_add( string $key, callable $generator, callable $validate
 		}
 
 		if ( $this->should_refresh_cache( $key, $cache_contents, $validate_data, $force_refresh ) ) {
-			$errored = false;
-
 			try {
 				$data    = $generator();
-				$errored = false === $data || null === $data;
+				$errored = ( false === $data || null === $data );
 			} catch ( \Throwable $e ) {
 				$errored = true;
 			}
@@ -139,7 +147,7 @@ public function get_or_add( string $key, callable $generator, callable $validate
 				$data = $old_data;
 			}
 
-			$cache_contents = $this->write_to_cache( $key, $data, $errored );
+			$this->write_to_cache( $key, $data, $errored );
 		}
 
 		return $data;
@@ -151,17 +159,18 @@ public function get_or_add( string $key, callable $generator, callable $validate
 	 * @param string $key The key to look for.
 	 * @param bool   $force If set, return from the cache without checking for expiry.
 	 *
-	 * @return mixed The cache contents.
+	 * @return mixed|null The cache contents. NULL if the cache is expired or missing.
 	 */
 	public function get( string $key, bool $force = false ) {
-		$cache_contents = get_option( $key );
+		$cache_contents = $this->get_from_cache( $key );
 		if ( is_array( $cache_contents ) && array_key_exists( 'data', $cache_contents ) ) {
 			if ( ! $force && $this->is_expired( $key, $cache_contents ) ) {
 				return null;
 			}
 
 			return $cache_contents['data'];
 		}
+
 		return null;
 	}
 
@@ -185,48 +194,53 @@ public function add( string $key, $data ) {
 	 * @return void
 	 */
 	public function delete( string $key ) {
-		delete_option( $key );
+		// Remove from the in-memory cache.
+		unset( $this->in_memory_cache[ $key ] );
 
-		// Clear WP Cache to ensure the new data is fetched by other processes.
-		wp_cache_delete( $key, 'options' );
+		// Remove from the DB cache.
+		if ( delete_option( $key ) ) {
+			// Clear the WP object cache to ensure the new data is fetched by other processes.
+			wp_cache_delete( $key, 'options' );
+		}
 	}
 
 	/**
-	 * Hook function allowing the cache refresh to be selectively disabled in certain situations
-	 * (such as while running an Action Scheduler job). While the refresh is disabled, get_or_add
-	 * will only return the cached value and never regenerate it, even if it's expired.
+	 * Deletes all cache entries that are keyed with a certain prefix.
 	 *
-	 * @return void
-	 */
-	public function disable_refresh() {
-		$this->refresh_disabled = true;
-	}
-
-	/**
-	 * Deletes all saved by looking for cache key prefix. This is useful when you want to cache user related data by
+	 * This is useful when you use dynamic cache keys.
+	 *
+	 * Note: Only key prefixes with known, static prefixes are allowed, for protection purposes.
 	 *
-	 * @param string $key Cache key prefix to delete.
+	 * @param string $key_prefix The cache key prefix.
 	 *
 	 * @return void
 	 */
-	public function delete_by_prefix( string $key ) {
-
-		// Protection against accidentally deleting all options or options that are not related to WcPay caching.
-		// Since only one cache key prefix is supported, we will check only this one by checking does key starts with payment method key prefix.
-		// Feel free to update this statement if more prefix cache keys you are planning to add.
-
-		if ( strncmp( $key, self::PAYMENT_METHODS_KEY_PREFIX, strlen( self::PAYMENT_METHODS_KEY_PREFIX ) ) !== 0 ) {
+	public function delete_by_prefix( string $key_prefix ) {
+		// Protection against accidentally deleting all options or options that are not related to WCPay caching.
+		// Feel free to update this statement as more prefix cache keys are used.
+		if ( strncmp( $key_prefix, self::PAYMENT_METHODS_KEY_PREFIX, strlen( self::PAYMENT_METHODS_KEY_PREFIX ) ) !== 0 ) {
 			return; // Maybe throw exception here...
 		}
-		global $wpdb;
 
-		$plugin_options = $wpdb->get_results( $wpdb->prepare( "SELECT option_name FROM $wpdb->options WHERE option_name LIKE %s", $key . '%' ) );
+		global $wpdb;
 
-		foreach ( $plugin_options as $option ) {
+		$options = $wpdb->get_results( $wpdb->prepare( "SELECT option_name FROM $wpdb->options WHERE option_name LIKE %s", $key_prefix . '%' ) );
+		foreach ( $options as $option ) {
 			$this->delete( $option->option_name );
 		}
 	}
 
+	/**
+	 * Hook function allowing the cache refresh to be selectively disabled in certain situations
+	 * (such as while running an Action Scheduler job). While the refresh is disabled, get_or_add
+	 * will only return the cached value and never regenerate it, even if it's expired.
+	 *
+	 * @return void
+	 */
+	public function disable_refresh() {
+		$this->refresh_disabled = true;
+	}
+
 	/**
 	 * Validates the cache contents and, given the passed params and the current application state, determines whether the cache should be refreshed.
 	 * See get_or_add.
@@ -252,7 +266,7 @@ private function should_refresh_cache( string $key, $cache_contents, callable $v
 			return false;
 		}
 
-		// The value of false means that there was never an option set in the database.
+		// The value of false means that there was never something cached.
 		if ( false === $cache_contents ) {
 			return true;
 		}
@@ -268,11 +282,8 @@ private function should_refresh_cache( string $key, $cache_contents, callable $v
 			return true;
 		}
 
-		// If the data is not errored and invalid, refresh it.
-		if (
-			! $cache_contents['errored'] &&
-			! $validate_data( $cache_contents['data'] )
-		) {
+		// If the data is not errored but invalid, we should refresh it.
+		if ( ! $cache_contents['errored'] && ! $validate_data( $cache_contents['data'] ) ) {
 			return true;
 		}
 
@@ -284,29 +295,60 @@ private function should_refresh_cache( string $key, $cache_contents, callable $v
 		return false;
 	}
 
+	/**
+	 * Get the cache contents for a certain key.
+	 *
+	 * @param string $key The cache key.
+	 *
+	 * @return array|false The cache contents (array with `data`, `fetched`, and `errored` entries).
+	 *                     False if there is no cached data.
+	 */
+	private function get_from_cache( string $key ) {
+		// Check the in-memory cache first.
+		if ( isset( $this->in_memory_cache[ $key ] ) ) {
+			return $this->in_memory_cache[ $key ];
+		}
+
+		// Read from the DB cache.
+		$data = get_option( $key );
+
+		// Store the data in the in-memory cache, including the case when there is no data cached (`false`).
+		$this->in_memory_cache[ $key ] = $data;
+
+		return $data;
+	}
+
 	/**
 	 * Wraps the data in the cache metadata and stores it.
 	 *
 	 * @param string  $key     The key to store the data under.
 	 * @param mixed   $data    The data to store.
 	 * @param boolean $errored Whether the refresh operation resulted in an error before this has been called.
 	 *
-	 * @return array The cache contents (data and metadata).
+	 * @return void
 	 */
-	private function write_to_cache( string $key, $data, bool $errored ): array {
+	private function write_to_cache( string $key, $data, bool $errored ) {
 		// Add the  data and expiry time to the array we're caching.
 		$cache_contents            = [];
 		$cache_contents['data']    = $data;
 		$cache_contents['fetched'] = time();
 		$cache_contents['errored'] = $errored;
 
-		// Create or update the option cache.
-		update_option( $key, $cache_contents, 'no' );
-
-		// Clear WP Cache to ensure the new data is fetched by other processes.
-		wp_cache_delete( $key, 'options' );
-
-		return $cache_contents;
+		// Write the in-memory cache.
+		$this->in_memory_cache[ $key ] = $cache_contents;
+
+		// Create or update the DB option cache.
+		// Note: Since we are adding the current time to the option value, WP will ALWAYS write the option because
+		// the cache contents value is different from the current one, even if the data is the same.
+		// A `false` result ONLY means that the DB write failed.
+		// Yes, there is the possibility that we attempt to write the same data multiple times within the SAME second,
+		// and we will mistakenly think that the DB write failed. We are OK with this false positive,
+		// since the actual data is the same.
+		$result = update_option( $key, $cache_contents, 'no' );
+		if ( false !== $result ) {
+			// If the DB cache write succeeded, clear the WP object cache to ensure the new data is fetched by other processes.
+			wp_cache_delete( $key, 'options' );
+		}
 	}
 
 	/**
@@ -315,12 +357,13 @@ private function write_to_cache( string $key, $data, bool $errored ): array {
 	 * @param string $key            The cache key.
 	 * @param array  $cache_contents The cache contents.
 	 *
-	 * @return boolean True if the contents are expired.
+	 * @return boolean True if the contents are expired. False otherwise.
 	 */
 	private function is_expired( string $key, array $cache_contents ): bool {
 		$ttl     = $this->get_ttl( $key, $cache_contents );
 		$expires = $cache_contents['fetched'] + $ttl;
 		$now     = time();
+
 		return $expires < $now;
 	}
 

includes/class-wc-payments-account.php

@@ -130,7 +130,7 @@ public function init_hooks() {
 	}
 
 	/**
-	 * Wipes the account data option, forcing to re-fetch the account status from WP.com.
+	 * Wipes the account data option, forcing to re-fetch the account data from WP.com.
 	 */
 	public function clear_cache() {
 		$this->database_cache->delete( Database_Cache::ACCOUNT_KEY );
@@ -201,7 +201,7 @@ public function has_working_jetpack_connection(): bool {
 	 * @return boolean Whether there is account data.
 	 */
 	public function has_account_data(): bool {
-		$account_data = $this->database_cache->get( Database_Cache::ACCOUNT_KEY );
+		$account_data = $this->database_cache->get( Database_Cache::ACCOUNT_KEY, true );
 		if ( ! empty( $account_data['account_id'] ) ) {
 			return true;
 		}
@@ -304,7 +304,7 @@ public function is_account_under_review(): bool {
 		}
 
 		$account = $this->get_cached_account_data();
-		return 'under_review' === $account['status'];
+		return 'under_review' === ( $account['status'] ?? false );
 	}
 
 	/**
@@ -2140,6 +2140,10 @@ function () {
 	 */
 	public function update_account_data( $property, $data ) {
 		$account_data = $this->database_cache->get( Database_Cache::ACCOUNT_KEY );
+		if ( ! is_array( $account_data ) ) {
+			// Bail if we don't have any cached account data.
+			return;
+		}
 
 		$account_data[ $property ] = is_array( $data ) ? array_merge( $account_data[ $property ] ?? [], $data ) : $data;
 
@@ -2149,7 +2153,7 @@ public function update_account_data( $property, $data ) {
 	/**
 	 * Refetches account data and returns the fresh data.
 	 *
-	 * @return array|bool|string Either the new account data or false if unavailable.
+	 * @return array|bool Either the new account data or false if unavailable.
 	 */
 	public function refresh_account_data() {
 		return $this->get_cached_account_data( true );

includes/class-wc-payments-customer-service.php

@@ -275,8 +275,8 @@ public function get_payment_methods_for_customer( $customer_id, $type = 'card' )
 			return $payment_methods;
 
 		} catch ( API_Exception $e ) {
-			// If we failed to find the we can simply return empty payment methods as this customer will
-			// be recreated when the user succesfuly adds a payment method.
+			// If we failed to find the payment methods, we can simply return empty payment methods as this customer
+			// will be recreated when the user successfully adds a payment method.
 			if ( $e->get_error_code() === 'resource_missing' ) {
 				return [];
 			}
@@ -383,7 +383,7 @@ public static function map_customer_data( WC_Order $wc_order = null, WC_Customer
 	}
 
 	/**
-	 * Delete all saved payment methods that are stored inside database cache driver.
+	 * Delete all saved payment methods that are stored inside the database cache driver.
 	 *
 	 * @return void
 	 */

includes/class-wc-payments-incentives-service.php

@@ -150,7 +150,7 @@ public function get_cached_connect_incentive( bool $force_refresh = false ): ?ar
 		$store_context_hash = $this->generate_context_hash( $this->get_store_context() );
 
 		// First, get the cache contents, if any.
-		$incentive_data = $this->database_cache->get( Database_Cache::CONNECT_INCENTIVE_KEY );
+		$incentive_data = $this->database_cache->get( Database_Cache::CONNECT_INCENTIVE_KEY, true );
 		// Check if we need to force-refresh the cache contents.
 		if ( empty( $incentive_data['context_hash'] ) || ! is_string( $incentive_data['context_hash'] )
 			|| ! hash_equals( $store_context_hash, $incentive_data['context_hash'] ) ) {

tests/unit/admin/test-class-wc-rest-payments-accounts-controller.php

@@ -48,6 +48,9 @@ public function set_up() {
 		$property_reflection->setAccessible( true );
 		$this->original_api_client = $property_reflection->getValue( $account_service );
 		$property_reflection->setValue( $account_service, $this->mock_api_client );
+
+		// Clear the account cache before each test.
+		$account_service->clear_cache();
 	}
 
 	public function tear_down() {