From afc4296768650fc1aff6618cc546af4dc0239857 Mon Sep 17 00:00:00 2001 From: Stuart McAlpine Date: Sun, 12 Jul 2026 06:09:27 +0100 Subject: [PATCH] fix: bound blocking editor-dropdown API calls on the metabox render path The classic-editor metabox calls Client::cached_get() up to 3 times per render (script/video templates, video settings). It only cached 2xx array responses and inherited the 30s write-path timeout, so when the BeyondWords API is slow or unreachable each render issued 3 sequential blocking requests of up to 30s (~90s total), repeated on every load and tying up PHP-FPM workers. A 401 self-heals (it clears beyondwords_valid_api_connection, ungating the metabox) but a timeout/DNS/5xx failure does not. - Negative-cache failures (WP_Error, 5xx, 4xx, non-JSON) for a short CACHE_TTL_ON_ERROR (2 min) using an empty-array sentinel, so an outage costs one probe per interval instead of one per render. - Use a short RENDER_REQUEST_TIMEOUT (3s, per VIP guidance) for render-path GETs, threaded via call_api()/build_args(); write-path calls keep WRITE_REQUEST_TIMEOUT (30s). Routing through call_api() preserves the existing 401 self-heal. No new phpcs:ignore is added; the 3s render-path timeout is VIP-compliant. The pre-existing ignore stays only for the legitimate 30s write-path timeout. Tests: replace failed_responses_are_not_cached with failed_responses_are_negative_cached, and add render_path_gets_use_a_short_timeout. phpcs clean; ClientTest (41) and SettingsFieldsTest (22) pass. Co-Authored-By: Claude Opus 4.8 --- src/api/class-client.php | 62 ++++++++++++++++++++++++++----- tests/phpunit/api/test-client.php | 62 +++++++++++++++++++++++++++---- 2 files changed, 108 insertions(+), 16 deletions(-) diff --git a/src/api/class-client.php b/src/api/class-client.php index d2741ae1..2b5a86f0 100644 --- a/src/api/class-client.php +++ b/src/api/class-client.php @@ -46,6 +46,30 @@ class Client { */ const CACHE_TTL = 15 * MINUTE_IN_SECONDS; + /** + * How long to negative-cache a *failed* editor-dropdown fetch (WP_Error / + * timeout, 5xx, 4xx, non-JSON). Much shorter than CACHE_TTL so a transient + * outage self-heals within minutes, but long enough that a slow or + * unreachable API is probed at most once per interval rather than blocking + * every admin edit-screen render. + */ + const CACHE_TTL_ON_ERROR = 2 * MINUTE_IN_SECONDS; + + /** + * Request timeout (seconds) for write-path calls (create/update/delete + * audio). Long, because these run on user-initiated saves that must not be + * dropped mid-generation. + */ + const WRITE_REQUEST_TIMEOUT = 30; + + /** + * Request timeout (seconds) for cached render-path GETs (editor dropdowns). + * Short, per VIP guidance (see `vip_safe_wp_remote_get`), so a slow API + * can't tie up a PHP-FPM worker for the length of an admin render. Paired + * with negative caching ({@see CACHE_TTL_ON_ERROR}) to bound repeat failures. + */ + const RENDER_REQUEST_TIMEOUT = 3; + /** * Register WordPress hooks. * @@ -443,13 +467,14 @@ public static function get_video_settings_templates(): array|null|false { * @param string $body Request body (already JSON-encoded for write methods). * @param int|false $post_id WordPress post ID for error attribution; false to suppress. * @param array $headers Extra per-request headers. + * @param int $timeout Request timeout in seconds; defaults to the long write-path timeout. */ - public static function call_api( string $method, string $url, string $body = '', int|false $post_id = false, array $headers = [] ): array|\WP_Error { + public static function call_api( string $method, string $url, string $body = '', int|false $post_id = false, array $headers = [], int $timeout = self::WRITE_REQUEST_TIMEOUT ): array|\WP_Error { $post = get_post( $post_id ); self::delete_errors( $post_id ); - $response = wp_remote_request( $url, self::build_args( $method, $body, $headers ) ); + $response = wp_remote_request( $url, self::build_args( $method, $body, $headers, $timeout ) ); $response_code = wp_remote_retrieve_response_code( $response ); @@ -479,17 +504,22 @@ public static function call_api( string $method, string $url, string $body = '', * @param string $method HTTP method. * @param string $body Request body. * @param array $headers Extra per-request headers. + * @param int $timeout Request timeout in seconds. * * @return array */ - private static function build_args( string $method, string $body = '', array $headers = [] ): array { + private static function build_args( string $method, string $body = '', array $headers = [], int $timeout = self::WRITE_REQUEST_TIMEOUT ): array { return [ 'blocking' => true, 'body' => $body, 'headers' => $headers, 'method' => strtoupper( $method ), + // The write-path default (WRITE_REQUEST_TIMEOUT) exceeds VIP's 3s + // guidance and trips this sniff — a deliberate trade-off so saves + // aren't dropped mid-generation. Render-path GETs pass the short + // RENDER_REQUEST_TIMEOUT via cached_get(), which stays within guidance. // phpcs:ignore WordPressVIPMinimum.Performance.RemoteRequestTimeout.timeout_timeout - 'timeout' => 30, + 'timeout' => $timeout, ]; } @@ -515,17 +545,25 @@ private static function cache_key( string $suffix ): string { } /** - * GET an endpoint, caching successful list/object responses. + * GET an editor-render-path endpoint, caching both hits and failures. + * + * Successful 2xx array responses are cached for {@see CACHE_TTL}. Failures + * (WP_Error / timeout, 5xx, 4xx, non-JSON) are negative-cached for the much + * shorter {@see CACHE_TTL_ON_ERROR}, so a slow or unreachable API is probed + * at most once per interval instead of re-issuing a blocking request on + * every admin render. The request itself uses the short + * {@see RENDER_REQUEST_TIMEOUT} for the same reason. * - * Only 2xx array responses are cached, so a transient API error retries on - * the next call rather than caching an empty/error payload for the full TTL. + * A 401 additionally self-heals: call_api() clears + * `beyondwords_valid_api_connection`, which ungates the metabox next load. * * @since 7.0.0 * * @param string $suffix Cache-key suffix (include any project/language id). * @param string $url Absolute endpoint URL. * - * @return array|null|false + * @return array|null|false Decoded body on the fetching call; the cached + * value ([] after a cached failure) thereafter. */ private static function cached_get( string $suffix, string $url ): array|null|false { $key = self::cache_key( $suffix ); @@ -535,7 +573,7 @@ private static function cached_get( string $suffix, string $url ): array|null|fa return $cached; } - $response = self::call_api( 'GET', $url ); + $response = self::call_api( 'GET', $url, '', false, [], self::RENDER_REQUEST_TIMEOUT ); $decoded = json_decode( wp_remote_retrieve_body( $response ), true ); if ( @@ -544,8 +582,14 @@ private static function cached_get( string $suffix, string $url ): array|null|fa && is_array( $decoded ) ) { set_transient( $key, $decoded, self::CACHE_TTL ); + + return $decoded; } + // Negative-cache the failure so the next render is served from the + // transient instead of blocking on another remote round-trip. + set_transient( $key, [], self::CACHE_TTL_ON_ERROR ); + return $decoded; } diff --git a/tests/phpunit/api/test-client.php b/tests/phpunit/api/test-client.php index 8237a260..33900f3b 100644 --- a/tests/phpunit/api/test-client.php +++ b/tests/phpunit/api/test-client.php @@ -478,12 +478,15 @@ public function editor_dropdown_responses_are_cached() * @test * @group settings * - * A non-2xx response is not cached, so the next call retries the API rather - * than serving a cached error for the full TTL. + * A failed editor-dropdown fetch is negative-cached for a short TTL, so a + * slow or unreachable API is probed at most once per interval instead of + * blocking every admin edit-screen render. The second call within the TTL + * is served from the transient (the empty-array sentinel) and makes no HTTP + * request. */ - public function failed_responses_are_not_cached() + public function failed_responses_are_negative_cached() { - // No API key → the mock returns a 401, which must not be cached. + // No API key → the mock returns a 401, which is negative-cached. $calls = 0; $counter = function ($preempt, $args, $url) use (&$calls) { if (str_contains((string) $url, '/summarization_settings_templates')) { @@ -493,12 +496,57 @@ public function failed_responses_are_not_cached() }; add_filter('pre_http_request', $counter, 0, 3); - Client::get_summarization_settings_templates(); - Client::get_summarization_settings_templates(); + $first = Client::get_summarization_settings_templates(); + $second = Client::get_summarization_settings_templates(); remove_filter('pre_http_request', $counter, 0); - $this->assertSame(2, $calls, 'Errors should not be cached'); + $this->assertSame(1, $calls, 'A failed response should be negative-cached, so the second call is served from the transient'); + $this->assertSame([], $second, 'The negative-cache sentinel is an empty array'); + } + + /** + * @test + * @group settings + * + * Cached editor-dropdown GETs render on the admin edit screen, so they use + * a short (<= 3s) timeout to avoid tying up a PHP worker when the API is + * slow. Write-path (and other direct call_api) requests keep the longer + * timeout so user-initiated saves aren't dropped mid-generation. + */ + public function render_path_gets_use_a_short_timeout() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + $timeouts = []; + $capture = function ($preempt, $args, $url) use (&$timeouts) { + $timeouts[(string) $url] = $args['timeout'] ?? null; + return $preempt; + }; + add_filter('pre_http_request', $capture, 0, 3); + + Client::get_summarization_settings_templates(); // cached_get → short timeout + Client::get_content(BEYONDWORDS_TESTS_CONTENT_ID); // call_api → long timeout + + remove_filter('pre_http_request', $capture, 0); + + $render_timeout = null; + $write_timeout = null; + foreach ($timeouts as $url => $timeout) { + if (str_contains($url, '/summarization_settings_templates')) { + $render_timeout = $timeout; + } elseif (str_contains($url, '/content/')) { + $write_timeout = $timeout; + } + } + + $this->assertNotNull($render_timeout, 'Expected a render-path request to be captured'); + $this->assertLessThanOrEqual(3, $render_timeout, 'Render-path GETs should use a short (<= 3s) timeout'); + $this->assertSame(30, $write_timeout, 'Direct/write-path requests keep the longer timeout'); + + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); } /**