Skip to content

Improve Post Stats cache handling for invalid or error data #46211

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.

Sign up for GitHub

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

Merged
dognose24 merged 10 commits into from
Dec 9, 2025
Merged

Improve Post Stats cache handling for invalid or error data #46211

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e0ceb39
Remove post stats cache
dognose24 Dec 5, 2025
8877038
changelog
dognose24 Dec 5, 2025
a736465
Improve post stats cache handling for invalid or error data
dognose24 Dec 5, 2025
ffe8c0c
Update changelog
dognose24 Dec 5, 2025
3c3624b
Merge branch 'trunk' into update/blog_stats_debug
dognose24 Dec 9, 2025
4ba2b0e
Respect the cache first and return errors or invalid data as they were
dognose24 Dec 9, 2025
68d3fe3
Keep WP errors from cache along with the valid response
dognose24 Dec 9, 2025
3bbe021
Update annotations
dognose24 Dec 9, 2025
5b9fc39
Update annotations
dognose24 Dec 9, 2025
bd5a7b1
Apply suggestions from code review
kangzj Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/packages/stats/changelog/update-blog_stats_debug
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Improve Post Stats cache handling for invalid or error data
63 changes: 43 additions & 20 deletions projects/packages/stats/src/class-wpcom-stats.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,34 +503,57 @@ protected function fetch_post_stats( $args, $post_id ) {
if ( $stats_cache ) {
$data = reset( $stats_cache );

if (
! is_array( $data )
|| empty( $data )
|| is_wp_error( $data )
) {
return $data;
}
// Check if we have a valid cache structure with a time key.
if ( is_array( $data ) && ! empty( $data ) ) {
$time = key( $data );

// If we have a numeric time, check if cache is still valid.
if ( is_numeric( $time ) ) {
/** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */
$expiration = apply_filters(
'jetpack_fetch_stats_cache_expiration',
self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS
);

$time = key( $data );
$views = $data[ $time ] ?? null;
// If within cache period, return cached data regardless of validity.
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "return cached data regardless of validity" is misleading. The code immediately validates the cached value by checking if it's a WP_Error or array (lines 522-530). Consider updating the comment to: "If within cache period, return cached data after type validation."

Suggested change
// If within cache period, return cached data regardless of validity.
// If within cache period, return cached data after type validation.

Copilot uses AI. Check for mistakes.
if ( ( time() - $time ) < $expiration ) {
$cached_value = $data[ $time ];

// Bail if data is malformed.
if ( ! is_numeric( $time ) || ! is_array( $views ) ) {
return $data;
}
// If it's a WP_Error, return it directly.
if ( is_wp_error( $cached_value ) ) {
return $cached_value;
}

/** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */
$expiration = apply_filters(
'jetpack_fetch_stats_cache_expiration',
self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS
);
// If it's an array, merge with cached_at timestamp.
if ( is_array( $cached_value ) ) {
return array_merge( array( 'cached_at' => $time ), $cached_value );
}
Copy link
Contributor

@kangzj kangzj Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't have to tell them apart - for those two cases we could both return:

return array_merge( array( 'cached_at' => $time ), $cached_value );

Copy link
Contributor

@kangzj kangzj Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If it's a WP_Error, return it directly.
if ( is_wp_error( $cached_value ) ) {
return $cached_value;
}
/** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */
$expiration = apply_filters(
'jetpack_fetch_stats_cache_expiration',
self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS
);
// If it's an array, merge with cached_at timestamp.
if ( is_array( $cached_value ) ) {
return array_merge( array( 'cached_at' => $time ), $cached_value );
}
// If it's an array or WP_Error, add cached time and return to user.
if ( is_array( $cached_value ) || is_wp_error( $cached_value ) ) {
return array_merge( array( 'cached_at' => $time ), (array)$cached_value );
}

Copy link
Contributor Author

@dognose24 dognose24 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 68d3fe3.


if ( ( time() - $time ) < $expiration ) {
return array_merge( array( 'cached_at' => $time ), $views );
// For any other type, return as-is.
return $cached_value;
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 532-533 will return any non-array, non-WP_Error cached value as-is, which could perpetuate invalid cached data (e.g., null, string, boolean) for the entire cache period. This contradicts the PR's goal of "detecting and recovering from invalid cache entries."

Consider either:

  1. Only returning the cached value if it's an array or WP_Error, falling through to refresh otherwise
  2. Adding explicit validation that cached_value is an expected type

Example:

// For any other unexpected type, treat as malformed cache.
// Fall through to refresh.

Then remove the return statement and let execution continue to line 540.

Suggested change
// For any other type, return as-is.
return $cached_value;
// For any other unexpected type, treat as malformed cache.
// Fall through to refresh.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@kangzj kangzj Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Copilot here - other cases should all fall to malformed types?

Copy link
Contributor Author

@dognose24 dognose24 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other cases should all fall to malformed types?

Looks reasonable to me, like only leave expected WP_Error thrown from cache.

Copy link
Contributor Author

@dognose24 dognose24 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 68d3fe3.

}
}
}
}

// Cache doesn't exist, is expired, or is malformed - refresh it.
return $this->refresh_post_stats_cache( $endpoint, $args, $post_id, $meta_name );
}

/**
* Force fetch stats from WPCOM, and update cache if needed.
*
* @param string $endpoint The stats endpoint.
* @param array $args The query arguments.
* @param int $post_id The post ID.
* @param string $meta_name The meta name.
*
* @return array|WP_Error
*/
protected function refresh_post_stats_cache( $endpoint, $args, $post_id, $meta_name ) {
$wpcom_stats = $this->fetch_remote_stats( $endpoint, $args );

// Always cache the result, even if it's an error or empty.
update_post_meta( $post_id, $meta_name, array( time() => $wpcom_stats ) );

return $wpcom_stats;
Comment on lines +559 to 565
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new refresh_post_stats_cache() method and the modified cache validation logic in fetch_post_stats() lack test coverage. Given that this package has comprehensive test coverage (as evidenced by WPCOM_Stats_Test.php with 863 lines of tests), the following scenarios should be tested:

  1. Invalid/malformed cache entries trigger a refresh
  2. WP_Error cached entries are handled correctly within and beyond expiration
  3. The new refresh_post_stats_cache() method properly caches both success and error responses
  4. Cache expiration logic works correctly with the new validation

This is critical since the PR specifically addresses a bug where "post stats cache could get stuck on invalid data or a WP_Error."

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@dognose24 dognose24 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Expand Down
Loading