Skip to content

perf: give the voices GET a longer timeout than the shared render bound#578

Merged
galbus merged 4 commits into
mainfrom
claude/silly-murdock-db3769
Jul 19, 2026
Merged

perf: give the voices GET a longer timeout than the shared render bound#578
galbus merged 4 commits into
mainfrom
claude/silly-murdock-db3769

Conversation

@galbus

@galbus galbus commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Scope has narrowed twice as main moved. This PR originally set a flat 'timeout' => 3 in build_args() and removed the VIP phpcs:ignore; 2be1ca0 then parametrized the timeout, and afc4296 (#580) bounded the render-path GETs with RENDER_TIMEOUT and added negative caching. Both of those superseded parts of this branch and have been dropped. What's left is the one case main doesn't handle: voices.

What

Give the voices GET its own bound, since it's the one editor dropdown that's genuinely slow.

+	const VOICES_TIMEOUT = 8;

-	private static function cached_get( string $suffix, string $url ): array|null|false {
+	private static function cached_get( string $suffix, string $url, int $timeout = self::RENDER_TIMEOUT ): array|null|false {
-		$response = self::call_api( 'GET', $url, '', false, [], self::RENDER_TIMEOUT );
+		$response = self::call_api( 'GET', $url, '', false, [], $timeout );

-		return self::cached_get( 'voices_' . $language_code, $url );
+		return self::cached_get( 'voices_' . $language_code, $url, self::VOICES_TIMEOUT );

Every other dropdown keeps RENDER_TIMEOUT (3s) via the default — nothing else changes.

Why

afc4296 applies a flat RENDER_TIMEOUT of 3s to every cached render GET. Per @gouravkhunger's Sentry figures that's correct for all of them except voices:

Endpoint p95 Bound on main
Languages, templates, project/video settings ~250ms 3s ✅
Voices ~3.7s 3s ❌

So on main today, voices exceeds its own timeout at p95. And because afc4296 also added negative caching, the consequence is worse than a single slow render: the timeout gets cached as an empty-array sentinel, so the Voice dropdown stays empty for a full CACHE_TTL_ON_ERROR (2 min) rather than retrying on the next render.

Reviewer notes

  • VOICES_TIMEOUT = 8 is the one judgment call — ~2x the observed 3.7s p95 to absorb the tail, still well under the 30s REQUEST_TIMEOUT. It's a named const, so it's a one-line change if you'd prefer 5s or 10s.
  • Only ever paid on a cache miss (15-min CACHE_TTL).
  • main's render_path_gets_use_a_short_timeout exercises get_summarization_settings_templates(), not voices, so it's unaffected by this change and still passes.
  • This branch's earlier CACHED_GET_TIMEOUT and its test were dropped as duplicates of RENDER_TIMEOUT / render_path_gets_use_a_short_timeout.
  • No phpcs:ignore added.

Verification

  • phpcs full ruleset (WordPress-VIP-Go) — clean, exit 0.
  • ClientTest — 44 tests / 148 assertions, including voices_get_uses_longer_timeout (asserts the exact bound, plus that it sits above RENDER_TIMEOUT and below REQUEST_TIMEOUT).
  • SelectVoiceTest — 12 / 61. SettingsFieldsTest — 22 / 90. SyncTest — 79 / 164.
  • Cypress suite not run.

🤖 Generated with Claude Code

build_args() set a 30s timeout on every BeyondWords API request and
suppressed the VIP RemoteRequestTimeout sniff to allow it. These are
blocking requests on real page-generating paths: the classic editor
render fires up to five sequential cached GETs (languages, voices,
summarization/video templates, video settings), and the write POST/PUT
runs synchronously inside wp_after_insert_post. A slow or unresponsive
API could pin a PHP worker for up to 30s per call (~150s worst case on
a cold cache), degrading the whole site under concurrency on VIP and
hanging wp-admin for editors.

Drop the timeout to 3s -- the VIP-recommended bound for blocking
requests on render paths -- and remove the now-unnecessary phpcs:ignore.
The 15-minute transients already keep these calls off the network on the
hot path; the shorter timeout only changes behaviour when the API is
slow, where failing fast beats worker exhaustion.

Verified with phpcs (WordPress-VIP-Go ruleset): passes with no
suppression. No PHPUnit/Jest test references the timeout value.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.


🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@galbus
galbus marked this pull request as ready for review July 19, 2026 07:03
@galbus

galbus commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@gouravkhunger 3s enough time for requests to our REST API? Do we have any observable metrics for response times?

WordPress VIP recommends < 3s.

@galbus
galbus requested a review from gouravkhunger July 19, 2026 07:05
@gouravkhunger

Copy link
Copy Markdown
Contributor

@galbus 3s should be enough for almost every query. The only one I will reconfirm is GET voices which usually takes around a second to prepare.

@galbus

galbus commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

No code change — leaving the bound at 3s, since ~1s for GET voices fits with ~3x headroom. Worth noting for the reconfirmation: get_voices() goes through cached_get() under the 15-minute CACHE_TTL, so that ~1s is only paid on a cold cache rather than on every editor render. If your check shows a worse tail on that endpoint, the clean fix is a per-call timeout arg on build_args() so just the voices GET gets a longer bound — happy to push that.

🤖 Addressed by Claude Code

…db3769

Conflict in Client::build_args(). main's 2be1ca0 ("defer audio deletion
to VIP cron and shorten DELETE timeout") parametrized the timeout with
REQUEST_TIMEOUT (30) / DELETE_TIMEOUT (3) and already dropped the
phpcs:ignore, which supersedes this branch's flat `'timeout' => 3`.
Resolved in favour of main: a flat 3s would have clobbered that design
and forced the create/update content calls -- deliberately generous, and
deferred to background cron on VIP -- down to 3s.

That leaves the render path this branch set out to fix still unbounded:
cached_get() called call_api() without a timeout, so the classic-editor
dropdown GETs (languages, voices, templates, project/video settings)
still defaulted to 30s -- up to five sequential blocking GETs during page
generation on a cold cache. main's commit bounded DELETE only.

Add CACHED_GET_TIMEOUT (3) following the existing per-path const idiom
and pass it from cached_get(). This matches the reviewer feedback on
PR #578 that 3s covers these queries (GET voices ~1s), and leaves
REQUEST_TIMEOUT at 30 for the content writes per main's design.

Verified: full phpcs ruleset clean; ClientTest 43 tests / 142 assertions
and SyncTest 79 tests / 164 assertions pass, including a new test
asserting the dropdown GETs are issued with CACHED_GET_TIMEOUT.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@galbus galbus changed the title perf: bound BeyondWords API requests to a 3s VIP-compliant timeout perf: bound cached editor GETs to a 3s timeout Jul 19, 2026
@galbus

galbus commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Heads up that merging main changed this PR's scope, so it's worth a re-look. 2be1ca0 already parametrized build_args() with REQUEST_TIMEOUT/DELETE_TIMEOUT and removed the phpcs:ignore, which superseded the flat 'timeout' => 3 here — I resolved the conflict in favour of main and dropped it, since a flat 3s would have forced the create/update content calls down to 3s against that commit's deliberate design.

What's left is the gap 2be1ca0 didn't cover: cached_get() still passed no timeout, so the classic-editor dropdown GETs inherited the 30s default — up to five sequential blocking GETs during page generation, and unlike the writes they aren't deferred to cron on VIP. So this PR is now just CACHED_GET_TIMEOUT = 3 on those GETs, which is exactly the "almost every query" case @gouravkhunger sized at 3s. REQUEST_TIMEOUT stays 30 for content writes. phpcs clean; ClientTest (43) and SyncTest (79) pass, with a new test covering the GET timeout.

🤖 Addressed by Claude Code

@gouravkhunger

Copy link
Copy Markdown
Contributor

@galbus as discussed on slack, only voices is slow (~3.7s p95 in sentry), every other editor GET is ~250ms p95 max, so I'll keep 3s on the rest and give voices a longer per-call timeout.

Sentry p95 for the editor dropdown GETs: voices ~3.7s, every other
endpoint ~250ms. The shared CACHED_GET_TIMEOUT (3s) would therefore
abandon more than 5% of cold-cache voices fetches, leaving the Voice
dropdown empty in the classic editor.

Add VOICES_TIMEOUT (8s) and a $timeout param on cached_get() so
get_voices() passes its own bound while the other dropdowns keep the
short 3s. 8s is ~2x the observed p95 to absorb the tail, still well
under the 30s REQUEST_TIMEOUT default, and only ever paid on a cache
miss (15-minute CACHE_TTL).

Verified: full phpcs ruleset clean; ClientTest 44/145, SelectVoiceTest
12/61 and SyncTest 79/164 pass, with a new test asserting voices is
issued with VOICES_TIMEOUT and the other dropdowns keep
CACHED_GET_TIMEOUT.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@galbus

galbus commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Done in 72f2b2c — split the bound per endpoint using your Sentry figures: added VOICES_TIMEOUT (8s) and a $timeout param on cached_get() so get_voices() passes its own bound, while languages/templates/project+video settings keep CACHED_GET_TIMEOUT (3s) on their ~250ms p95.

Flagging the one number I picked: 8s for voices, ~2x your 3.7s p95 to absorb the tail while staying ~4x tighter than the old 30s default. It's a named const, so it's a one-line change if you'd rather have 5s or 10s. Worth noting the cost is cold-cache only (15-minute CACHE_TTL), and a timeout isn't fatal — only 2xx responses get cached, so a slow fetch leaves the dropdown empty and refills next request rather than caching the failure.

phpcs clean; ClientTest 44/145 (two new tests asserting voices gets the longer bound and the rest keep the short one), SelectVoiceTest 12/61, SyncTest 79/164.

🤖 Addressed by Claude Code

…db3769

main's afc4296 ("bound blocking editor-dropdown API calls on the metabox
render path", PR #580) landed the same render-path fix as this branch,
plus negative caching. As reconciled on main it gives RENDER_TIMEOUT (3s)
passed by cached_get(), which fully supersedes this branch's
CACHED_GET_TIMEOUT. Dropped that const and the test covering it --
main's render_path_gets_use_a_short_timeout asserts the same thing, via
get_summarization_settings_templates() rather than voices, so it is
unaffected by the change below.

What survives is the voices-specific bound, and main makes the case
stronger rather than weaker: cached_get() now applies a flat 3s to every
render GET including voices, which is ~3.7s p95 in Sentry, and failures
are negative-cached. Voices would therefore not merely time out on >5%
of cold-cache fetches -- it would blank the Voice dropdown for a whole
CACHE_TTL_ON_ERROR instead of retrying on the next render.

Keep VOICES_TIMEOUT (8s) and thread a $timeout param through main's
cached_get(), defaulting to RENDER_TIMEOUT so every other dropdown is
untouched.

Verified: full phpcs ruleset clean; ClientTest 44/148, SelectVoiceTest
12/61, SettingsFieldsTest 22/90 and SyncTest 79/164 pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@galbus galbus changed the title perf: bound cached editor GETs to a 3s timeout perf: give the voices GET a longer timeout than the shared render bound Jul 19, 2026
@galbus

galbus commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Conflicts resolved against main (967d5e4), and the overlap with #580 has narrowed this PR again — worth a re-read.

afc4296 landed the same render-path fix plus negative caching, so its RENDER_TIMEOUT fully supersedes this branch's CACHED_GET_TIMEOUT; I dropped that const and its test (render_path_gets_use_a_short_timeout already covers it, via get_summarization_settings_templates() rather than voices, so it's unaffected by what's left here).

Flagging one thing that afc4296 didn't account for: it applies a flat 3s to every cached render GET including voices, which is ~3.7s p95 per @gouravkhunger's Sentry figures — so on main today voices exceeds its own timeout at p95. Negative caching makes that bite harder than a slow render: the timeout is cached as the empty-array sentinel, so the Voice dropdown stays empty for a full CACHE_TTL_ON_ERROR (2 min) instead of retrying next render.

So this PR is now just VOICES_TIMEOUT (8s) plus a $timeout param on cached_get() defaulting to RENDER_TIMEOUT — voices gets its own bound, everything else is untouched. 8s remains the one number I picked (~2x p95); it's a named const if you'd rather have 5s or 10s.

phpcs clean; ClientTest 44/148, SelectVoiceTest 12/61, SettingsFieldsTest 22/90, SyncTest 79/164.

🤖 Addressed by Claude Code

@galbus
galbus merged commit 57ca0a8 into main Jul 19, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants