feat: add timeout option to browser monitors#1113
feat: add timeout option to browser monitors#1113miguelmartin-elastic wants to merge 12 commits intoelastic:mainfrom
Conversation
Parse monitor timeout strings, apply them as journey timeouts, and add unit coverage for global, local, and CLI timeout sources. Co-authored-by: Cursor <cursoragent@cursor.com>
Update push warning to trigger only without privateLocations and add a unit coverage for the warning output. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a dedicated test-bundler-zip folder to avoid parallel test collisions between bundler and plugin suites. Include updated Node types from linting hooks. Co-authored-by: Cursor <cursoragent@cursor.com>
Validate CLI/config timeouts for lightweight monitors, pass the timeout into YAML-backed monitor configs, and cover it with tests. Co-authored-by: Cursor <cursoragent@cursor.com>
| import { Bundler } from '../../src/push/bundler'; | ||
|
|
||
| const PROJECT_DIR = join(__dirname, 'test-bundler'); | ||
| const PROJECT_DIR = join(__dirname, 'test-bundler-zip'); |
There was a problem hiding this comment.
I added this because this and the plugin.test.ts were failing when ran locally and in parallel
src/core/runner.ts
Outdated
| if (step.status === 'failed') { | ||
| journey.status = step.status; | ||
| journey.error = step.error; | ||
| await this.withJourneyTimeout(timeoutMs, timeoutValue, async () => { |
There was a problem hiding this comment.
I'm not very familiar with how the runner works behind the scenes, so I would really like to know what you think about this approach @emilioalvap (not sure if this is even needed at all, but I assume it is?)
There was a problem hiding this comment.
Pull request overview
Adds a configurable timeout option for monitors, validates its format, wires it into the local runner as a journey-level timeout, and surfaces a warning when timeout is configured without private locations.
Changes:
- Introduces
timeoutparsing/validation (s/m/h) and adds it toMonitorConfig. - Applies timeout precedence (per-journey monitor > global monitor > CLI/options) and enforces it in the local runner.
- Adds CLI flag support and warning behavior; expands unit test coverage for push + runner behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/push/monitor.ts | Passes timeout from YAML/CLI into monitor config and validates its format. |
| src/push/index.ts | Adds a warning helper for “timeout configured without private locations”. |
| src/dsl/monitor.ts | Adds validateTimeout/parseTimeout, extends MonitorConfig, and validates timeout in Monitor.validate(). |
| src/core/runner.ts | Adds journey-level timeout enforcement with precedence logic and passes CLI timeout into the created runner monitor. |
| src/common_types.ts | Plumbs timeout through CLI/base args typing. |
| src/cli.ts | Adds --timeout flag and invokes the new push warning. |
| package.json | Updates @types/node patch version. |
| tests/push/monitor.test.ts | Adds lightweight monitor tests for CLI timeout and invalid format rejection. |
| tests/push/index.test.ts | Adds tests for --timeout push behavior and the “no privateLocations” warning. |
| tests/push/bundler.test.ts | Renames test directory reference used for zip validation. |
| tests/core/runner.test.ts | Adds journey timeout tests for global/local/CLI precedence and enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/core/runner.ts
Outdated
| const timeoutError = new Error( | ||
| `Journey timeout of ${timeoutValue} exceeded` | ||
| ); | ||
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| const timeoutPromise = new Promise<T>((_, reject) => { | ||
| timeoutId = setTimeout(() => reject(timeoutError), timeoutMs); | ||
| }); | ||
| try { | ||
| return await Promise.race([fn(), timeoutPromise]); | ||
| } finally { | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Promise.racerejects on timeout but does not cancelfn()`, so the journey steps can continue running in the background after the timeout triggers (potentially impacting subsequent journeys and resource cleanup). Consider introducing cancellation (e.g., an AbortController passed down to step execution / Playwright operations) and stopping execution when the timeout fires, rather than only rejecting the wrapper promise.
There was a problem hiding this comment.
I think this is a valid concern. Doing it the right way may require a little bit more of thinking and possibly is out of scope for this issue/PR? If we want to use a different approach I can send a separate PR Lmkwyt @emilioalvap
closes #1101
Summary
timeoutparsing (s/m/h) and apply it as a journey-level timeout in the local runner.Manual Test Plan
Lightweight monitor
Local runner (run)
CLI timeout
npx @elastic/synthetics . --timeout 1sGlobal config timeout
synthetics.config.ts, setmonitor: { timeout: '1s' }.npx @elastic/synthetics .Per‑monitor override
timeout: '1s'in config.monitor.use({ timeout: '2s' })and keep the slow step (~1.1s).npx @elastic/synthetics .Validation
timeout: '1x'in config ormonitor.use.npx @elastic/synthetics .Push (project monitors)
CLI timeout on push
npx @elastic/synthetics push --timeout 30stimeout: "30s".Global config timeout on push
synthetics.config.ts, setmonitor: { timeout: '30s' }.npx @elastic/synthetics pushtimeout: "30s".Per‑monitor override on push
timeout: '30s'.monitor.use({ timeout: '10s' }).npx @elastic/synthetics pushtimeout: "10s"and others keep"30s".No Private locations warning
locations(public) plustimeouton any monitor and DO NOT include any private location.npx @elastic/synthetics push