From 2be1ca083a6efb92d8cbf87fc15a3138bef45b80 Mon Sep 17 00:00:00 2001 From: Stuart McAlpine Date: Sat, 11 Jul 2026 20:53:50 +0100 Subject: [PATCH] fix(sync): defer audio deletion to VIP cron and shorten DELETE timeout on_trash_post() and on_delete_post() each made a blocking 30s remote DELETE with no async path. Bulk-trashing N posts fired N sequential 30s-worst-case DELETEs in one admin request, a WordPress VIP violation that could hang or fatal the request mid-loop. Mirror the generation design: when is_async_generation_enabled() (VIP), capture the project and content IDs and schedule one background DELETE_AUDIO_CRON_HOOK event. The IDs travel in the event args because the meta is wiped on trash (and the row is gone on delete) before the job runs. Off-VIP, fall back to a synchronous DELETE bounded by a new short DELETE_TIMEOUT (3s) instead of 30s. Parametrize Client::build_args() and call_api() with a timeout and add Client::delete_audio_by_ids(); delete_audio() delegates to it. The timeout is now a variable, so the VIP RemoteRequestTimeout sniff no longer fires and the existing phpcs:ignore is removed (none added). Verified: full phpcs ruleset clean; SyncTest and ClientTest pass (121 tests, 303 assertions, 6 new). Co-Authored-By: Claude Opus 4.8 --- src/api/class-client.php | 62 +++++++++- src/post/class-sync.php | 96 ++++++++++++++- tests/phpunit/api/test-client.php | 63 ++++++++++ tests/phpunit/post/test-sync.php | 191 ++++++++++++++++++++++++++++++ 4 files changed, 404 insertions(+), 8 deletions(-) diff --git a/src/api/class-client.php b/src/api/class-client.php index d2741ae1..18f81657 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; + /** + * Default timeout, in seconds, for a BeyondWords API request. + * + * The create/update content calls can be slow, so this is deliberately + * generous. On WordPress VIP those calls are deferred to background cron + * (see `\BeyondWords\Post\Sync::is_async_generation_enabled()`), so the long + * timeout never blocks a web request there. + * + * @since 7.0.0 + */ + const REQUEST_TIMEOUT = 30; + + /** + * Timeout, in seconds, for a DELETE request. + * + * Deletion runs on the synchronous trash/delete admin path — once per post + * on a bulk trash off-VIP — so a long blocking timeout would hang the request + * and can breach VIP request limits. Deletes are best-effort (a timeout just + * leaves the audio in the BeyondWords dashboard), so we cap the wait short. + * + * @since 7.0.0 + */ + const DELETE_TIMEOUT = 3; + /** * Register WordPress hooks. * @@ -180,6 +204,9 @@ public static function update_audio( int $post_id ): array|null|false { /** * DELETE /projects/:project/content/:content_id * + * Resolves the project + content IDs from the post's meta and delegates to + * `delete_audio_by_ids()`. + * * @param int $post_id WordPress post ID. * * @return array|null|false `false` when the request didn't return 204. @@ -188,12 +215,33 @@ public static function delete_audio( int $post_id ): array|null|false { $project_id = \BeyondWords\Post\Meta::get_project_id( $post_id ); $content_id = \BeyondWords\Post\Meta::get_content_id( $post_id, true ); + return self::delete_audio_by_ids( $project_id, $content_id, $post_id ); + } + + /** + * DELETE /projects/:project/content/:content_id using explicit IDs. + * + * Split out from `delete_audio()` so the deferred trash/delete cron job can + * delete audio from the project + content IDs captured before the post meta + * was wiped (see `\BeyondWords\Post\Sync::delete_audio_by_ids()`). Uses the + * short `DELETE_TIMEOUT` because deletion runs on the synchronous + * trash/delete admin path. + * + * @since 7.0.0 + * + * @param int|string|false $project_id BeyondWords project ID. + * @param int|string|false $content_id BeyondWords content ID. + * @param int|false $post_id Optional post ID for error attribution. + * + * @return array|null|false `false` when an ID is missing or the request didn't return 204. + */ + public static function delete_audio_by_ids( int|string|false $project_id, int|string|false $content_id, int|false $post_id = false ): array|null|false { if ( ! $project_id || ! $content_id ) { return false; } $url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, $content_id ); - $response = self::call_api( 'DELETE', $url, '', $post_id ); + $response = self::call_api( 'DELETE', $url, '', $post_id, [], self::DELETE_TIMEOUT ); if ( 204 !== wp_remote_retrieve_response_code( $response ) ) { return false; @@ -443,13 +491,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 REQUEST_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::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 +528,18 @@ 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. Defaults to REQUEST_TIMEOUT; + * DELETEs pass the shorter DELETE_TIMEOUT. * * @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::REQUEST_TIMEOUT ): array { return [ 'blocking' => true, 'body' => $body, 'headers' => $headers, 'method' => strtoupper( $method ), - // phpcs:ignore WordPressVIPMinimum.Performance.RemoteRequestTimeout.timeout_timeout - 'timeout' => 30, + 'timeout' => $timeout, ]; } diff --git a/src/post/class-sync.php b/src/post/class-sync.php index 6b09431b..e992e2a6 100644 --- a/src/post/class-sync.php +++ b/src/post/class-sync.php @@ -35,6 +35,18 @@ class Sync { */ const GENERATE_AUDIO_CRON_HOOK = 'beyondwords_generate_audio'; + /** + * Cron hook that runs a deferred audio deletion. + * + * Like GENERATE_AUDIO_CRON_HOOK, only scheduled when async is enabled (VIP). + * The event args carry the BeyondWords project + content IDs — not a post ID — + * because the post meta is wiped (on trash) or the post row is gone (on + * permanent delete) by the time the job runs. + * + * @since 7.0.0 + */ + const DELETE_AUDIO_CRON_HOOK = 'beyondwords_delete_audio'; + /** * Register WordPress hooks. */ @@ -50,6 +62,12 @@ public static function init(): void { // config change. add_action( self::GENERATE_AUDIO_CRON_HOOK, [ self::class, 'generate_audio_for_post' ] ); + // Background audio deletion — the deferred counterpart to the trash/delete + // handlers. Registered unconditionally (like the generation hook) so a + // queued event still runs after a config change. Takes the project + + // content IDs as args because the post meta is gone by the time it fires. + add_action( self::DELETE_AUDIO_CRON_HOOK, [ self::class, 'delete_audio_by_ids' ], 10, 2 ); + add_filter( 'is_protected_meta', [ self::class, 'is_protected_meta' ], 10, 2 ); // Older posts may be missing `beyondwords_language_code`; back-fill from @@ -261,6 +279,24 @@ public static function batch_delete_audio_for_posts( array $post_ids ): array|fa return \BeyondWords\Api\Client::batch_delete_audio( $post_ids ); } + /** + * Run a deferred audio deletion queued by the trash/delete handlers. + * + * The `DELETE_AUDIO_CRON_HOOK` cron event carries the BeyondWords project + + * content IDs directly (not a post ID) because the post meta has already been + * wiped — or the post row deleted — by the time this fires. Only reached on + * the async path, which is VIP-only by default (see + * `is_async_generation_enabled()`). + * + * @since 7.0.0 + * + * @param int|string $project_id BeyondWords project ID. + * @param int|string $content_id BeyondWords content ID. + */ + public static function delete_audio_by_ids( int|string $project_id, int|string $content_id ): array|false|null { + return \BeyondWords\Api\Client::delete_audio_by_ids( $project_id, $content_id ); + } + /** * Persist relevant fields from a BeyondWords API response into post meta. * @@ -380,6 +416,32 @@ private static function schedule_audio_generation( int $post_id ): void { wp_schedule_single_event( time(), self::GENERATE_AUDIO_CRON_HOOK, [ $post_id ] ); } + /** + * Schedule a deferred audio-deletion cron event. + * + * Mirrors `schedule_audio_generation()`: VIP-only in practice, prefers the + * VIP wrapper when present, and no-ops when an identical event is already + * queued so repeated trashing can't stack duplicate jobs. The project + + * content IDs are the event args (rather than a post ID) because the meta is + * gone by the time the job runs. + * + * @since 7.0.0 + */ + private static function schedule_audio_deletion( int|string $project_id, int|string $content_id ): void { + $args = [ $project_id, $content_id ]; + + if ( wp_next_scheduled( self::DELETE_AUDIO_CRON_HOOK, $args ) ) { + return; + } + + if ( function_exists( 'wpcom_vip_schedule_single_event' ) ) { + wpcom_vip_schedule_single_event( time(), self::DELETE_AUDIO_CRON_HOOK, $args ); + return; + } + + wp_schedule_single_event( time(), self::DELETE_AUDIO_CRON_HOOK, $args ); + } + /** * Clear any pending deferred audio-generation cron event for a post. * @@ -395,6 +457,36 @@ private static function unschedule_audio_generation( int $post_id ): void { wp_clear_scheduled_hook( self::GENERATE_AUDIO_CRON_HOOK, [ $post_id ] ); } + /** + * Delete a post's BeyondWords audio, deferring to background cron on VIP. + * + * On VIP (async enabled) we capture the project + content IDs now and + * schedule a single background event, so the trash/delete request — which may + * be removing many posts at once — returns immediately instead of blocking on + * one remote DELETE per post. We read the IDs before the caller wipes the meta + * (on trash) or WordPress deletes the row (on permanent delete). Off-VIP, + * where WP-Cron is unreliable, we fall back to a synchronous DELETE, bounded + * by the client's short `DELETE_TIMEOUT`. + * + * The caller must have confirmed `Meta::has_content()` first. + * + * @since 7.0.0 + */ + private static function delete_audio_for_post_or_defer( int $post_id ): void { + if ( self::is_async_generation_enabled() ) { + $project_id = \BeyondWords\Post\Meta::get_project_id( $post_id ); + $content_id = \BeyondWords\Post\Meta::get_content_id( $post_id, true ); + + if ( $project_id && $content_id ) { + self::schedule_audio_deletion( $project_id, $content_id ); + } + + return; + } + + self::delete_audio_for_post( $post_id ); + } + /** * Trash hook — DELETE the audio so it disappears from the dashboard, then * remove our local metadata. @@ -408,7 +500,7 @@ public static function on_trash_post( $post_id ): void { return; } - \BeyondWords\Api\Client::delete_audio( $post_id ); + self::delete_audio_for_post_or_defer( $post_id ); \BeyondWords\Post\Meta::remove_all_beyondwords_metadata( $post_id ); } @@ -425,7 +517,7 @@ public static function on_delete_post( $post_id ): void { return; } - \BeyondWords\Api\Client::delete_audio( $post_id ); + self::delete_audio_for_post_or_defer( $post_id ); } /** diff --git a/tests/phpunit/api/test-client.php b/tests/phpunit/api/test-client.php index 8237a260..be622b0a 100644 --- a/tests/phpunit/api/test-client.php +++ b/tests/phpunit/api/test-client.php @@ -220,6 +220,69 @@ public function delete_audio() delete_option('beyondwords_project_id'); } + /** + * @test + */ + public function delete_audio_by_ids_returns_false_when_ids_missing() + { + // A missing project or content ID short-circuits before any HTTP request. + $apiCalled = false; + $filter = function ($preempt, $args, $url) use (&$apiCalled) { + $apiCalled = true; + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + $this->assertFalse(Client::delete_audio_by_ids(false, 'abc-123')); + $this->assertFalse(Client::delete_audio_by_ids(1234, false)); + $this->assertFalse(Client::delete_audio_by_ids(0, 0)); + + remove_filter('pre_http_request', $filter, 1); + + $this->assertFalse($apiCalled, 'No HTTP request should be made when an ID is missing'); + } + + /** + * @test + * + * The deferred trash/delete cron job deletes audio by explicit IDs (the post + * meta is already gone), using the short DELETE_TIMEOUT so it never blocks a + * request for long. + */ + public function delete_audio_by_ids_issues_delete_with_short_timeout() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + $captured = ['method' => null, 'url' => null, 'timeout' => null]; + $filter = function ($preempt, $args, $url) use (&$captured) { + if (str_contains((string) $url, '/content/')) { + $captured['method'] = $args['method'] ?? null; + $captured['url'] = $url; + $captured['timeout'] = $args['timeout'] ?? null; + return ['response' => ['code' => 204, 'message' => 'No Content'], 'body' => '', 'headers' => [], 'cookies' => []]; + } + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + $response = Client::delete_audio_by_ids(BEYONDWORDS_TESTS_PROJECT_ID, BEYONDWORDS_TESTS_CONTENT_ID); + + remove_filter('pre_http_request', $filter, 1); + + // 204 with an empty body decodes to null. + $this->assertNull($response); + $this->assertSame('DELETE', $captured['method']); + $this->assertStringContainsString( + '/projects/' . BEYONDWORDS_TESTS_PROJECT_ID . '/content/' . BEYONDWORDS_TESTS_CONTENT_ID, + (string) $captured['url'] + ); + $this->assertSame(Client::DELETE_TIMEOUT, $captured['timeout']); + + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); + } + /** * @test */ diff --git a/tests/phpunit/post/test-sync.php b/tests/phpunit/post/test-sync.php index af87fb2c..c103c0de 100644 --- a/tests/phpunit/post/test-sync.php +++ b/tests/phpunit/post/test-sync.php @@ -35,6 +35,7 @@ public function init() $this->assertEquals(10, has_action('wp_trash_post', array(Sync::class, 'on_trash_post'))); $this->assertEquals(10, has_action('before_delete_post', array(Sync::class, 'on_delete_post'))); $this->assertEquals(10, has_action(Sync::GENERATE_AUDIO_CRON_HOOK, array(Sync::class, 'generate_audio_for_post'))); + $this->assertEquals(10, has_action(Sync::DELETE_AUDIO_CRON_HOOK, array(Sync::class, 'delete_audio_by_ids'))); $this->assertEquals(10, has_action('is_protected_meta', array(Sync::class, 'is_protected_meta'))); $this->assertEquals(10, has_action('get_post_metadata', array(Sync::class, 'get_lang_code_from_json_if_empty'))); } @@ -1415,4 +1416,194 @@ public function on_delete_post_unschedules_pending_generate_audio_event() wp_delete_post($postId, true); } + + /** + * @test + * @group trash + */ + public function on_trash_post_defers_delete_to_cron_when_async_enabled() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + add_filter('beyondwords_async_generate_audio', '__return_true'); + + $postId = self::factory()->post->create([ + 'post_title' => 'CoreTest::onTrashPostDefersDeleteToCron', + 'meta_input' => [ + 'beyondwords_project_id' => BEYONDWORDS_TESTS_PROJECT_ID, + 'beyondwords_content_id' => BEYONDWORDS_TESTS_CONTENT_ID, + ], + ]); + + // Capture the exact IDs the handler will schedule, before the meta is wiped. + $projectId = \BeyondWords\Post\Meta::get_project_id($postId); + $contentId = \BeyondWords\Post\Meta::get_content_id($postId, true); + + // No synchronous DELETE may be made on the deferred (schedule-only) path. + $apiCalled = false; + $filter = function ($preempt, $args, $url) use (&$apiCalled) { + if (str_contains((string) $url, '/content/')) { + $apiCalled = true; + return ['response' => ['code' => 204, 'message' => 'No Content'], 'body' => '', 'headers' => [], 'cookies' => []]; + } + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + Sync::on_trash_post($postId); + + remove_filter('pre_http_request', $filter, 1); + + // The blocking DELETE is deferred to a single background cron event carrying + // the project + content IDs (not the post ID, since the meta is now gone). + $this->assertFalse($apiCalled, 'Trash must not make a synchronous DELETE when async is enabled'); + $this->assertNotFalse(wp_next_scheduled(Sync::DELETE_AUDIO_CRON_HOOK, [$projectId, $contentId])); + + // Local metadata is still cleared immediately. + $this->assertSame('', get_post_meta($postId, 'beyondwords_content_id', true)); + + wp_unschedule_event( + wp_next_scheduled(Sync::DELETE_AUDIO_CRON_HOOK, [$projectId, $contentId]), + Sync::DELETE_AUDIO_CRON_HOOK, + [$projectId, $contentId] + ); + remove_filter('beyondwords_async_generate_audio', '__return_true'); + wp_delete_post($postId, true); + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); + } + + /** + * @test + * @group delete + */ + public function on_delete_post_defers_delete_to_cron_when_async_enabled() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + add_filter('beyondwords_async_generate_audio', '__return_true'); + + $postId = self::factory()->post->create([ + 'post_title' => 'CoreTest::onDeletePostDefersDeleteToCron', + 'meta_input' => [ + 'beyondwords_project_id' => BEYONDWORDS_TESTS_PROJECT_ID, + 'beyondwords_content_id' => BEYONDWORDS_TESTS_CONTENT_ID, + ], + ]); + + $projectId = \BeyondWords\Post\Meta::get_project_id($postId); + $contentId = \BeyondWords\Post\Meta::get_content_id($postId, true); + + $apiCalled = false; + $filter = function ($preempt, $args, $url) use (&$apiCalled) { + if (str_contains((string) $url, '/content/')) { + $apiCalled = true; + return ['response' => ['code' => 204, 'message' => 'No Content'], 'body' => '', 'headers' => [], 'cookies' => []]; + } + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + Sync::on_delete_post($postId); + + remove_filter('pre_http_request', $filter, 1); + + $this->assertFalse($apiCalled, 'Permanent delete must not make a synchronous DELETE when async is enabled'); + $this->assertNotFalse(wp_next_scheduled(Sync::DELETE_AUDIO_CRON_HOOK, [$projectId, $contentId])); + + wp_unschedule_event( + wp_next_scheduled(Sync::DELETE_AUDIO_CRON_HOOK, [$projectId, $contentId]), + Sync::DELETE_AUDIO_CRON_HOOK, + [$projectId, $contentId] + ); + remove_filter('beyondwords_async_generate_audio', '__return_true'); + wp_delete_post($postId, true); + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); + } + + /** + * @test + * @group trash + */ + public function on_trash_post_deletes_synchronously_off_vip() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + $postId = self::factory()->post->create([ + 'post_title' => 'CoreTest::onTrashPostSyncOffVip', + 'meta_input' => [ + 'beyondwords_project_id' => BEYONDWORDS_TESTS_PROJECT_ID, + 'beyondwords_content_id' => BEYONDWORDS_TESTS_CONTENT_ID, + ], + ]); + + $projectId = \BeyondWords\Post\Meta::get_project_id($postId); + $contentId = \BeyondWords\Post\Meta::get_content_id($postId, true); + + $captured = ['method' => null, 'timeout' => null]; + $filter = function ($preempt, $args, $url) use (&$captured) { + if (str_contains((string) $url, '/content/')) { + $captured['method'] = $args['method'] ?? null; + $captured['timeout'] = $args['timeout'] ?? null; + return ['response' => ['code' => 204, 'message' => 'No Content'], 'body' => '', 'headers' => [], 'cookies' => []]; + } + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + Sync::on_trash_post($postId); + + remove_filter('pre_http_request', $filter, 1); + + // Off-VIP the DELETE runs inline, bounded by the short delete timeout, and + // nothing is queued. + $this->assertSame('DELETE', $captured['method']); + $this->assertSame(\BeyondWords\Api\Client::DELETE_TIMEOUT, $captured['timeout']); + $this->assertFalse(wp_next_scheduled(Sync::DELETE_AUDIO_CRON_HOOK, [$projectId, $contentId])); + + wp_delete_post($postId, true); + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); + } + + /** + * @test + * @group delete + */ + public function delete_audio_by_ids_delegates_to_api_client() + { + update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY); + update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID); + + $captured = ['method' => null, 'url' => null, 'timeout' => null]; + $filter = function ($preempt, $args, $url) use (&$captured) { + if (str_contains((string) $url, '/content/')) { + $captured['method'] = $args['method'] ?? null; + $captured['url'] = $url; + $captured['timeout'] = $args['timeout'] ?? null; + return ['response' => ['code' => 204, 'message' => 'No Content'], 'body' => '', 'headers' => [], 'cookies' => []]; + } + return $preempt; + }; + add_filter('pre_http_request', $filter, 1, 3); + + // The cron callback deletes by explicit IDs (the meta is already gone). + Sync::delete_audio_by_ids(BEYONDWORDS_TESTS_PROJECT_ID, BEYONDWORDS_TESTS_CONTENT_ID); + + remove_filter('pre_http_request', $filter, 1); + + $this->assertSame('DELETE', $captured['method']); + $this->assertStringContainsString( + '/projects/' . BEYONDWORDS_TESTS_PROJECT_ID . '/content/' . BEYONDWORDS_TESTS_CONTENT_ID, + (string) $captured['url'] + ); + $this->assertSame(\BeyondWords\Api\Client::DELETE_TIMEOUT, $captured['timeout']); + + delete_option('beyondwords_api_key'); + delete_option('beyondwords_project_id'); + } }