Build-wow: stop the status poller mislabelling real failures as timeouts#112986
Draft
borkweb wants to merge 1 commit into
Draft
Build-wow: stop the status poller mislabelling real failures as timeouts#112986borkweb wants to merge 1 commit into
borkweb wants to merge 1 commit into
Conversation
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
## Summary Diagnostics and coverage follow-ups to the build status poller. None of this changes what a user sees; it makes the failure telemetry trustworthy and closes test gaps where the suite would have stayed green with the behaviour removed. ## Why The poller inferred "this request timed out" from `controller.signal.aborted`. That flag latches the instant the deadline fires and stays set for the rest of the iteration, so it only identifies the abort on transports that honour the signal. `wpcom-xhr-request` has no signal support at all, so on oauth and Jetpack-site builds the abort is a no-op, the request runs on to its real conclusion, and a genuine failure minutes later was recorded as `request timed out after 15000ms`. The actual cause was discarded — and because the fabricated reason occupied one of the three deduplication slots, it could prevent the real one from ever being reported. The reason string also dropped the HTTP status. Failures arrive as `WPError`, which carries the status separately and whose message the response body can overwrite, so a 401 and a 403 sharing a body message collapsed to one deduplication slot and one of them was silently discarded. `describeRequestError` was documented as the fix for the abort case rejecting with a DOM `Event`, which it could not do: an `Event` has no `message`, so it fell through to `String( error )` and returned the very `"[object Event]"` the comment said it prevented. The test named for that behaviour drove the hardcoded timeout branch instead and would have passed with the function deleted. ## How A timeout is claimed only when the deadline has passed *and* the rejection is abort-shaped, via a new `isAbortLike()`; otherwise the real error is described. `describeRequestError` now recognises aborts, includes the HTTP status when present, and bounds the string before it reaches the logging pipeline. Response parsing moves below the `try`, so a malformed payload from an injected fetcher is no longer reported as a request failure. Teardown clears both timers before calling `abort()`, which runs transport listeners synchronously at exactly the moment the page is going away. `onFailed` sets state before logging, since the poller dispatches terminal callbacks unguarded and a throw out of the log call would otherwise strand the user on a spinner until the deadline. Coverage: `describeRequestError` gains cases for non-`Error` rejections and for distinct statuses sharing a message; the deadline-passed-but-not-aborted path is tested directly; the cancellation test is split so one case actually asserts the in-flight signal is aborted rather than only counting calls; and the hook gains unmount, `onRequestError` wiring, and step-clamp tests. The clamp and the callback-liveness guards were each verified by removing them and confirming the new tests fail. ## Testing - [ ] `yarn test-client client/landing/stepper/declarative-flow/internals/steps-repository/site-generation/test` — 37 tests pass - [ ] On a build using the xhr transport, stall a status request past the deadline and let it fail for a real reason; confirm the logged reason is the real error, not a timeout
borkweb
force-pushed
the
fix/build-wow-status-poller-followup
branch
from
July 24, 2026 23:12
72605c5 to
767b623
Compare
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
1 participant
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.
Part of BIGR-724.
Proposed Changes
Diagnostics and coverage follow-ups to the build-wow status poller. Nothing here changes what a user sees.
controller.signal.aborted, which latches the instant the deadline fires and stays set. It now requires both the deadline to have passed and the rejection to be abort-shaped.describeRequestErrorrecognises aborts. It was documented as handling a rejection that arrives as a DOMEventand could not — anEventhas nomessage, so it fell through and returned the exact"[object Event]"the comment said it prevented.tryso a malformed payload is not reported as a request failure; teardown clears both timers before callingabort();onFailedsets state before logging.Errorrejections, distinct statuses sharing a message, the deadline-passed-but-not-aborted path, an assertion that cancellation actually aborts the in-flight signal, plus hook-level unmount, request-failure wiring, and step-clamp tests.Why are these changes being made?
wpcom-xhr-requesthas nosignalsupport — onlywpcom-proxy-requesthonours it, andclient/lib/wp/browser.jsselects the xhr transport for oauth and Jetpack-site builds. On those builds the abort is a no-op, so the request runs on to its real conclusion. Becausesignal.abortedwas already latched, a genuine failure minutes later was recorded asrequest timed out after 15000msand the actual cause was discarded. The fabricated reason also occupied one of the three deduplication slots, so the real error could be prevented from ever being reported. That is the opposite of what the failure telemetry was added for, and it degrades exactly when the endpoint is unhealthy.The status was worth keeping for the same reason. Failures arrive as
WPError, which carries the status separately from the message and whose message the response body can overwrite, so a 401 and a 403 sharing a body message collapsed into a single deduplication slot and one was silently dropped.The coverage gaps are included because several of them would have kept the suite green with the behaviour removed — the test named for the
"[object Event]"fix drove a hardcoded branch and never called the function it was named after, and the cancellation test asserted only a call count because the request had already settled by the time it ran.Testing Instructions
yarn test-client client/landing/stepper/declarative-flow/internals/steps-repository/site-generation/test— 37 tests pass.build_wow_site_generation_status_request_failedreason is the real error with its HTTP status, not a timeout.Pre-merge Checklist
Items from the template that do not apply to this change have been removed: there is no UI change (dark mode, accessibility), no new user-facing strings (string freeze, translations), and no selectors or expensive computations (memoizing).
site-generation/test. New coverage for non-Errorrejections, distinct HTTP statuses sharing a message, the deadline-passed-but-not-aborted path, cancellation actually aborting the in-flight signal, and hook-level unmount / request-failure wiring / step-clamp. The clamp and the callback-liveness guards were each verified by removing them and confirming the new tests fail.yarn typecheck-clientreports 19 errors, all pre-existing ontrunkand none insite-generation;yarn eslintis clean on the changed directory. Browser verification is pending the paired backend endpoint, since the failure paths this touches cannot be exercised until status requests can actually fail.build_wow_site_generation_status_request_failednow carries the HTTP status alongside a server-supplied error message, capped at 300 characters. The message originates from our own backend, not user input, andwp-errorcopies response-body fields onto the error object — so if that endpoint ever returns something sensitive inmessage, this would put it in Logstash.