TD-2600: separate browser liveness from test progress in the server watchdog - #174
Open
wmech-tiugo wants to merge 1 commit into
Open
wmech-tiugo wants to merge 1 commit into
wmech-tiugo wants to merge 1 commit into
Conversation
…atchdog The server used a single 55 second budget for both "the browser has stopped responding" and "the browser has not finished a test", so a remote browser that was merely slow was killed as if its driver had died. - Controller now tracks liveness (session.alive) separately from progress (session.updated). Silence from the browser is still caught after 55 seconds; a browser that is responding but slow gets the new --progressTimeout, which defaults to 5 minutes. - The runner sends keep-alive pings in auto mode as well as manual. Results are batched and a page reload posts nothing at all, so without them the server has no evidence of liveness other than test results. - /tests/alive now uses the throttled maybeSendKeepAlive, so the extra pings do not issue a webdriver command every few seconds.
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.
TL;DR
The server judged a remote browser by a single 55 second budget that answered two
different questions at once: "has the browser stopped responding?" and "has it
finished a test recently?". Because results are batched client-side every 30
seconds and a chunk reload posts nothing at all, a browser that was merely slow
looked exactly like a dead driver, and the whole lane was killed.
This splits the two. Silence from the browser is still caught after 55 seconds.
A browser that is responding but slow now gets
--progressTimeout, default 5minutes. The runner sends keep-alive pings in auto mode as well as manual, so
the server has evidence of liveness other than test results.
One budget answering two questions
session.updatedis bumped by exactly three routes:/tests/start,/tests/resultsand/tests/done(Apis.ts)./tests/aliveonly setssession.alive, and in auto mode the runner never sent it at all, because thekeep-alive timer was guarded by
if (harness.mode === 'manual').So the watchdog was fed only by posted test results, and
Reporterflushes thoseat most every 30 seconds. The real budget was therefore closer to 25 seconds than
55, and a chunk boundary spends it on a full page reload that re-fetches the whole
test bundle while posting nothing.
This is the second half of TINYMCE-14879, which fixed the start-ping side of the
same problem: "if there is a test-fail-reload or a chunk boundary reload, updates
are not sent to the server".
Two clocks instead of one
Controller.awaitDonenow checks them separately:now - session.alive > 55s. Every route bumpssession.alive,so this fires only when nothing at all is getting through. The message is
unchanged,
No updates from the browser in Xs, assuming driver has failed.now - session.updated > progressTimeout. New message, and a new--progressTimeoutCLI option defaulting to 5 minutes.The default sits below the 360 second idle timeout we already advertise to the
remote grid (
REMOTE_IDLE_TIMEOUT_SECONDS), so bedrock still decides when to giveup rather than the grid.
Why auto mode needs the keep-alive back
Splitting the clocks is only useful if something feeds the liveness one. In auto
mode nothing did, so the runner now sends the ping in both modes.
That exposed a second problem:
/tests/alivecalledkeepAliveAction()with nothrottle, while the runner pings every 5 seconds. Enabling it in auto mode would
have issued a webdriver command every 5 seconds on every lane. The route now uses
maybeSendKeepAlive(), the throttled variant already used by/tests/results.If an older client never pings, liveness and progress collapse back to the same
clock and behaviour is exactly what it is today.
Verified
yarn testgreen: lint, build of all four projects, and 176 tests passing(26 + 3 + 92 + 55).
ClisTestasserts the whole resolved settings object, so the new option is addedto its nine expectation blocks. No other test needed changing.
Not yet exercised against a real remote grid. The behaviour worth confirming there
is that a slow lane now finishes slowly instead of being killed, which is the whole
point of the change.
Why now
On 24-25 September a LambdaTest slowdown cost 38 builds a browser session, 59.4%
of every failure in that window, and
tinymce-premiumfinished a day at 1 greenagainst 31 red with zero failing tests, because nothing ran far enough for a test
to fail.
Nothing on our side changed: build 40 passed and build 41 failed on the identical
commit, config, bedrock version and tunnel client. Across 14 of those kills the
server reported 55.133s to 55.973s against its own 55 second threshold, so it was
fully responsive at the moment it gave up. These were healthy sessions killed on a
budget that had run out, not hung ones.
See TD-2600.
What the reviewer should check
stuck run, higher is kinder to a slow grid. It only applies while the browser is
still responding.
seconds against a 55 second budget, so ten consecutive pings have to be lost
before a healthy lane dies.
The liveness message is deliberately unchanged; the progress one is new and will
need a companion rule in jenkins-build-analytics.