fix(site-health): live bounded API probe; collapse timeout consts to two#582
Merged
Conversation
Site Health > Info issued a synchronous wp_remote_request() to the BeyondWords API on every render, with no timeout override (WP's 5s default), no caching, and even on fresh installs with no credentials. A slow or unreachable API blocked the admin page for up to 5s per view. - Skip the probe entirely when Utils::has_api_creds() is false; show a "credentials not configured" status instead. - Cache the result in a 5-minute transient so repeat views and "Copy site info" do not re-hit the network. - Bound the request: prefer vip_safe_wp_remote_get() where available, else wp_remote_request() with an explicit 2s timeout (under VIP's 3s ceiling). - Resolve the host before gethostbyname() so the unreachable message reports a real IP (it was passing the full URL, a no-op). Adds PHPUnit coverage for the configured, unconfigured-skip, cached, and transport-error paths. No phpcs:ignore added; phpcs clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
Reverses the caching approach taken earlier in this branch. WordPress core does this same probe uncached on every Info-screen render (WP_Debug_Data::get_wp_core() hits wordpress.org with a 10s timeout), because Site Health is a diagnostic: a cached result keeps reporting "unreachable" to an admin who just fixed creds/DNS and hit refresh. - Drop the 5-minute transient so every render re-probes. No lint rule required it: RemoteRequestTimeout only errors above 3s, and LowExpiryCacheTime only applies to wp_cache_*, so phpcs stays green. - Use the 3s bound already established on main rather than 2s, which risked a false "unreachable" on a slow-but-working link. - Drop the vip_safe_wp_remote_get branch: its circuit breaker reports a failure without retrying, which would make the diagnostic lie. Centralise the 3s value as Client::BLOCKING_TIMEOUT, the single place to retune the VIP blocking-path ceiling. DELETE_TIMEOUT, RENDER_TIMEOUT and Utils::CONNECTION_CHECK_TIMEOUT now alias it while keeping their own rationale. Every call site and test already used the named constants, so this is non-breaking. Credential-gating and the gethostbyname() host fix are unchanged; core passes a bare hostname there too, which the plugin previously got wrong. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There were six timeout constants for three values (3, 8, 30), several of
them pass-through aliases. Per current guidance the only genuinely slow
BeyondWords endpoint is the voices GET — content create/update returns
the content ID immediately and generates audio asynchronously
server-side — so the generous 30-second default no longer has a
justification. Collapse to:
- Client::DEFAULT_REQUEST_TIMEOUT = 3, the default for call_api(),
build_args() and cached_get() (VIP's blocking-request ceiling)
- Client::VOICES_REQUEST_TIMEOUT = 8, the one override
Removed: REQUEST_TIMEOUT (30), BLOCKING_TIMEOUT, DELETE_TIMEOUT,
RENDER_TIMEOUT, VOICES_TIMEOUT, Utils::CONNECTION_CHECK_TIMEOUT.
delete_audio_by_ids() and the settings connection check now just use
the default; the Site Health probe references the shared constant.
Behaviour change: content create/read/update, the Magic Embed player
bootstrap, and batch delete drop from an implicit 30s timeout to 3s.
Tests updated to the new names; the render-vs-write-path test premise
("write paths keep the longer timeout") is replaced by
all_requests_use_the_short_default_timeout.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
galbus
marked this pull request as ready for review
July 19, 2026 09:14
Contributor
Author
|
@gouravkhunger there are multiple open and recently closed PRs without request timeout values. This one includes the update to only use two values:
If approved, I'll merge the changes here into other open PRs before we review those. |
gouravkhunger
approved these changes
Jul 19, 2026
galbus
added a commit
that referenced
this pull request
Jul 19, 2026
Main's #582 collapsed the timeout constants: REQUEST_TIMEOUT (30s), DELETE_TIMEOUT and RENDER_TIMEOUT are gone, replaced by Client::DEFAULT_REQUEST_TIMEOUT (3s), with VOICES_REQUEST_TIMEOUT (8s) the only exception. Git auto-merged this textually but the result was semantically broken: create_audio(), update_audio(), generate_audio_for_post() and update_or_recreate_audio() all still defaulted their $timeout parameter to the deleted Client::REQUEST_TIMEOUT, which fatals at call time, and a SyncTest assertion referenced it too. BULK_GENERATE_TIMEOUT (15s) existed only to pull the bulk loop below the old generous 30s default. With the default now 3s it would instead make the bulk path five times more permissive than every other call — the opposite of this branch's intent — so it is removed entirely, along with the $timeout parameter it was threaded through. Those four signatures return to matching main, and the bulk loop inherits the 3s default. BULK_GENERATE_SYNC_LIMIT stays: it bounds the number of blocking calls, which is the actual N+1 fix and is orthogonal to per-call duration. The shorter default makes it strictly better — worst case drops from 10 x 15s = 150s to 10 x 3s = 30s, inside a typical execution limit. The SyncTest assertion is rewritten to guard exactly that invariant (cap x per-call timeout <= 60s) instead of the removed constant. Verified: full phpcs ruleset clean; full PHPUnit suite passes (703 tests, 2240 assertions, 1 multisite-only skip). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two related changes to how the plugin bounds blocking requests:
1. Site Health probe: bounded and gated, but live
Site Health > Info (
SiteHealth::add_rest_api_connection()) issued a blockingwp_remote_request()to the BeyondWords API on every render — with notimeoutoverride (WordPress' 5s default), even on fresh installs with no credentials.Utils::has_api_creds()gate; fresh installs make zero HTTP requests and show "Not checked — BeyondWords API credentials are not configured."gethostbyname()(a no-op).resolve_api_host()parses the host first so the "Unable to reach … at {IP}" diagnostic reports a real IP. Core passes a bare hostname here too.Deliberately NOT cached. An earlier revision cached the result in a transient; that was reverted after checking prior art: WordPress core runs this same probe uncached on every Info render (
WP_Debug_Data::get_wp_core()→wp_remote_get('https://wordpress.org', ['timeout' => 10])). Site Health is a diagnostic — a cached result keeps reporting "unreachable" to an admin who just fixed creds/DNS and hit refresh. No phpcs rule requires caching (RemoteRequestTimeoutonly errors above 3s;LowExpiryCacheTimeonly inspectswp_cache_*).vip_safe_wp_remote_get()is also avoided: its circuit breaker reports failure without retrying, which would make the diagnostic lie. Same forClient::call_api(), which clearsbeyondwords_valid_api_connectionon 401 as a side effect.2. Timeout constants: six names for three values → two constants
Per current guidance, the only genuinely slow BeyondWords endpoint is the voices GET — content create/update returns the content ID immediately and generates audio asynchronously server-side. So the generous 30s default had no remaining justification, and the pass-through aliases added churn:
Client::REQUEST_TIMEOUT = 30(default)Client::DELETE_TIMEOUT = 3Client::RENDER_TIMEOUT = 3Client::VOICES_TIMEOUT = 8Client::VOICES_REQUEST_TIMEOUT = 8Utils::CONNECTION_CHECK_TIMEOUT = 3Client::DEFAULT_REQUEST_TIMEOUT = 3(default ofcall_api()/build_args()/cached_get())Behaviour change (intentional): content create/read/update, the Magic Embed player bootstrap, and batch delete drop from an implicit 30s timeout to 3s.
Testing
npm run phpcs(WordPress-VIP-Go): clean, nophpcs:ignoreadded.ClientTest+SyncTest+UtilsTest+SiteHealthTest: 167 tests / 456 assertions passing. New/updated cases assert: the probe is not cached (two renders ⇒ two requests), zero requests without credentials, the probe and all requests useDEFAULT_REQUEST_TIMEOUT(≤ 3s per VIP), and voices keeps its longer bound. The old "write paths keep the longer timeout" test premise is replaced byall_requests_use_the_short_default_timeout.🤖 Generated with Claude Code