Skip to content

fix: bound blocking editor-dropdown API calls on the metabox render path#580

Merged
galbus merged 2 commits into
mainfrom
claude/trusting-mccarthy-70c63f
Jul 19, 2026
Merged

fix: bound blocking editor-dropdown API calls on the metabox render path#580
galbus merged 2 commits into
mainfrom
claude/trusting-mccarthy-70c63f

Conversation

@galbus

@galbus galbus commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Problem

Client::cached_get() powers the classic-editor metabox's dropdowns. On every post.php / post-new.php render, Metabox::render_meta_box_content() triggers up to 3 sequential blocking API callsget_summarization_settings_templates(), get_video_settings_templates(), get_video_settings().

Two problems compounded:

  • Failures were never cached. cached_get() only stored a transient for a 2xx response with an array body. WP_Error (timeout/DNS), 5xx, and non-JSON responses fell through, so the identical 3-call stall repeated on every edit-screen load for the duration of an incident.
  • Render path inherited the 30s write timeout. build_args() hard-coded timeout => 30, so each call could block up to 30s (~90s per render).

A 401 self-heals (call_api() clears beyondwords_valid_api_connection, which ungates the metabox in Plugin::init), but a network blackhole / DNS failure / 5xx does not clear that flag — so the metabox keeps rendering and keeps re-issuing the requests, tying up PHP-FPM workers and making wp-admin editing unusable while the API is degraded.

Fix

All changes are in src/api/class-client.php:

  1. Negative-cache failures. cached_get() now writes set_transient($key, [], CACHE_TTL_ON_ERROR) (2 min) for any WP_Error / non-2xx / non-JSON response, using an empty-array sentinel (never false, so get_transient's hit/miss check stays unambiguous). An outage now costs one probe per 2 min instead of one per render. Successful responses keep the 15-min CACHE_TTL.
  2. Short render-path timeout. Added a $timeout param threaded cached_get() → call_api() → build_args(). Render-path GETs pass RENDER_REQUEST_TIMEOUT (3s, per VIP / vip_safe_wp_remote_get guidance); write-path calls keep WRITE_REQUEST_TIMEOUT (30s). Routing through call_api() preserves the existing 401 self-heal.

Worst case during an outage drops from ~90s-per-render-forever to a single ~9s render, then cached for 2 min.

Reviewer notes

  • No new phpcs:ignore. The 3s render-path timeout is VIP-compliant on its own. The pre-existing ignore remains only for the legitimate 30s write-path timeout (with a clarified comment explaining the trade-off).
  • Deliberately skipped the optional has_api_creds() short-circuit: it would break the tested contract that a no-creds get_languages()/get_voices() returns the API's 401 message array, and it's largely redundant — the metabox is already gated on has_valid_api_connection(), and the negative-cache + short-timeout already bound the no-creds cost to one 3s probe per 2 min.
  • Out of scope: the 30s write-path timeout still trips the VIP sniff and relies on the existing ignore. That's a deliberate save-reliability trade-off, not part of this defect.

Testing

  • phpcs (full WordPress-VIP-Go scan): clean, exit 0.
  • ClientTest — 41 tests / 136 assertions pass. Replaced failed_responses_are_not_cached with failed_responses_are_negative_cached (2nd call served from cache) and added render_path_gets_use_a_short_timeout (render GET ≤3s, write path =30s).
  • SettingsFieldsTest (exercises the metabox render path) — 22 tests / 90 assertions pass.
  • Did not run the full Cypress suite.

🤖 Generated with Claude Code

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 <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

…thy-70c63f

Resolves a conflict in src/api/class-client.php. main's 2be1ca0 added the
same call_api()/build_args() timeout parametrization for a short DELETE
timeout, so both sides had touched the identical lines.

Resolution keeps both intents:

- Adopt main's REQUEST_TIMEOUT/DELETE_TIMEOUT naming and drop this
  branch's duplicate WRITE_REQUEST_TIMEOUT (same value, same meaning).
- Rename this branch's render-path constant to RENDER_TIMEOUT to match
  that convention.
- Drop this branch's phpcs:ignore. main demonstrated it is unnecessary
  once the timeout is a variable, so no suppression remains in the file.
- Keep the negative-caching in cached_get() (CACHE_TTL_ON_ERROR)
  unchanged; it now passes self::RENDER_TIMEOUT.

Verified: full phpcs ruleset clean; ClientTest, SyncTest and
SettingsFieldsTest pass (144 tests, 399 assertions).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@galbus
galbus marked this pull request as ready for review July 19, 2026 07:36
@galbus
galbus requested a review from gouravkhunger July 19, 2026 07:36
@galbus
galbus merged commit 672a93e into main Jul 19, 2026
9 checks passed
@galbus
galbus deleted the claude/trusting-mccarthy-70c63f branch July 19, 2026 07:40
galbus added a commit that referenced this pull request Jul 19, 2026
…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>
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