Repository navigation
TD-2668: judge test progress by ping count, not elapsed time - #174
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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Invalid progress-timeout values can silently disable the new watchdog.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Separates browser liveness from test-progress monitoring to avoid terminating responsive but slow test sessions.
Changes:
- Adds configurable
--progressTimeout. - Tracks liveness and test progress independently.
- Enables throttled keep-alive pings in auto mode.
| File | Description |
|---|---|
CHANGELOG.md |
Documents watchdog and keep-alive changes. |
modules/runner/src/main/ts/runner/Runner.ts |
Enables keep-alive pings in all modes. |
modules/server/src/main/ts/bedrock/cli/ClOptions.ts |
Defines the progress-timeout option. |
modules/server/src/main/ts/bedrock/cli/Clis.ts |
Registers the new CLI option. |
modules/server/src/main/ts/bedrock/core/Settings.ts |
Adds progress timeout to settings. |
modules/server/src/main/ts/bedrock/server/Apis.ts |
Propagates the timeout and throttles keep-alives. |
modules/server/src/main/ts/bedrock/server/Controller.ts |
Implements separate watchdog clocks. |
modules/server/src/main/ts/bedrock/server/Serve.ts |
Passes the setting into server APIs. |
modules/server/src/test/ts/ClisTest.ts |
Updates expected CLI settings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@TheSpyder three things you were right about, one you were not, and I am taking your design. Right. The "25 second budget" in the description was too strong. The flush is only evaluated when a test completes, so the real gap between posts is 30s plus whatever test is running, a 25 to 55s range. 25s is the worst-case floor for one test, not the budget. You pushed back on that on 28 Sep and you were correct. Right. There is a second wall. Not the one you named. The per-lane Jenkins timeout is My own error, worse than yours. This description blames chunk-boundary reloads. Build 41's kills landed at tests 836, 1719, 1374, 1128 and 1147 against Measured, 89 lanes of So I am taking your design. Progress judged in ping count, not wall time. One thing your version needs that is not obvious from the outside. Separately, two things nobody has flagged. New commits shortly. The release is still the binding constraint: nothing here or in #173 reaches CI until someone cuts one. |
The progress half of the watchdog used `--progressTimeout`, a configurable wall-clock budget. That number cannot be set correctly, because the failure it exists to catch is a connection slowing down, and a constant does not slow down with it. Five minutes was too long for a stuck run and too short for a bad day on the grid, and two reviewers said so independently. Progress is now judged by counting the posts the runner makes. Every post is a ping, only a post carrying results is progress, so a link that halves in speed halves the ping rate and widens the tolerance by the same factor. `session.updated` and `session.done` move inside the `results.length > 0` guard. Without that the heartbeat would feed the very clock it exists to let us read, and the progress watchdog would be silently dead. `--progressTimeout` is removed along with its settings and CLI plumbing, so this change adds no user-facing option.
The runner only posted when a test finished, and results were batched for up to 30 seconds, so a browser working through one slow test looked exactly like a browser that had died. The server had nothing else to judge it by. It now posts on a fixed interval whether or not anything has finished, and an empty batch still goes out: that empty post is what tells a slow browser from a gone one. The timer reschedules only once the previous post settles, so a slow connection stretches the gap rather than stacking requests on top of one that has not come back. The previous `setInterval` had no such protection, and because the request carries no timeout it could never abort either. This replaces the auto-mode keep-alive rather than adding to it. That channel goes back to manual mode only, so there is one channel on the wire instead of two, at roughly three requests a minute.
`/tests/init` called no controller method, so the window where a chunk or retry reload re-fetches the test bundle counted as silence against the liveness clock. No JS runs during it, which is exactly when the browser can least afford to look idle. The runner already posts the session id to that route, so recording activity there costs one line.
wmech-tiugo
changed the title
TL;DR
The server judged a remote browser by a single 55 second budget answering two questions at once: has the browser stopped responding, and has it finished a test recently. Results are batched and a reload posts nothing, so a browser that was merely slow looked exactly like a dead driver and the lane was killed.
This splits the two. Silence from the browser is still caught after 55 seconds. Lack of progress is now measured by counting the posts the runner makes rather than by a configurable timeout, because the failure this exists to catch is a connection slowing down, and a constant does not slow down with it.
One budget answering two questions
session.updatedwas bumped by exactly three routes:/tests/start,/tests/resultsand/tests/done./tests/aliveonly setsession.alive, and in auto mode the runner never sent it, because the keep-alive timer was guarded byif (harness.mode === 'manual').The watchdog only ever ran in auto mode:
awaitDonerequires sticky sessions and manual setsstickyFirstSession: false. So the one mode with a watchdog had no evidence of life other than posted test results, andReporterflushed those at most every 30 seconds, and only when a test completed.Why counting pings rather than seconds
A wall-clock budget cannot be set correctly here. The failure it catches is a remote grid slowing down, and five minutes is at once too long for a genuinely stuck run and too short for a bad day on the grid. Two reviewers said the numbers were wrong, independently, three weeks apart.
Progress is now judged by counting posts. The runner posts on a fixed interval whether or not a test has finished. The server treats every post as a ping and only a post carrying results as progress, so a link that halves in speed halves the ping rate and widens the tolerance by the same factor. It also cannot misfire on
this.timeout(0)orthis.retries(n), because those keep pinging.session.updatedandsession.donehad to move inside theresults.length > 0guard. Without that the heartbeat feeds the very clock it exists to let us read.The reload window nobody was watching
/tests/initcalled no controller method at all, so the window where a chunk or retry reload re-fetches the test bundle counted as silence against the liveness clock. No JS runs during it, which is exactly when the browser can least afford to look idle.The runner already posts the session id to that route, so recording activity there costs one line.
Correcting the original diagnosis
The first version of this description blamed chunk-boundary reloads. That was wrong for the builds it cited. Build 41's kills landed at tests 836, 1719, 1374, 1128 and 1147 against
--chunk=1000, so none was at a boundary. The mechanism was slow execution inside a chunk, which is what a progress budget covers and what the reload story does not.The earlier claim that the effective budget was "closer to 25 seconds than 55" was also too strong. The flush is evaluated only when a test completes, so the real range is 25 to 55 seconds, and 25 is the worst case for a single test rather than the budget.
What this does not fix
Merging this does not make builds like 24-25 September green on its own.
tinymce-premiumsets bedrock'soverallTimeoutto 30 minutes inGruntfile.js. Measured over 89 lanes the p50 bedrock run is 13.9 minutes and p90 is 19.1, so 39% of lanes already sit above the point where a 2x slowdown exceeds that ceiling.Under a uniform 2x slowdown the share of green builds goes from 0% today to roughly 5% with this change, and to roughly 57% if that timeout moves to 40 minutes. That clock starts after the tunnel and the remote session, so 40 still fits under the 50 minute Tests stage. It is one line in another repository and should be raised separately.
Nothing reaches CI until someone cuts a bedrock release either. npm still serves 17.0.1 from 28 August, and #173 has been merged and unreleased since 9 September.
Verified
yarn testgreen: lint, build of all four projects, and 177 tests passing (26 + 3 + 93 + 55).The new test covers the client half of the contract, that the heartbeat posts even when the batch is empty. The server half, that an empty post counts as a ping but not as progress, has no test:
Controller.createarms a session cleanupsetIntervalthat is never cleared, so any test of that module hangs mocha. That is pre-existing and worth its own ticket.Not exercised against a real remote grid. The behaviour worth confirming there is that a slow lane finishes slowly instead of being killed.
What the reviewer should check
The ping threshold. Six pings at a 20 second heartbeat is about two minutes of proven liveness without a finished test before a run is called stuck. It only applies while the browser is still responding.
The heartbeat against the liveness budget. Server-visible gaps are now the interval plus one round trip, against the unchanged 55 second liveness timeout, so a round trip above roughly 35 seconds would trip it. This coupling did not exist before and is the part I am least sure of.
The failure message. Our CI classifier keys on these strings. The liveness message is deliberately unchanged. The new progress message needs a companion rule in jenkins-build-analytics, and without one it is caught silently by that rule's
Unexpected Bedrock Erroralternative and counted as a dropped session.