From db2f0fb62a36d463ef9fcd3c24847e46e55811d5 Mon Sep 17 00:00:00 2001 From: Loakesh Indiran <66478092+Loaki07@users.noreply.github.com> Date: Fri, 31 Jul 2026 06:30:19 +0530 Subject: [PATCH] feat(synthetics): attempts view, evidence panel, alerting, concurrency (#13543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Note: this branch carries ~20 earlier synthetics commits. The alerting piece is ~280 lines and is cleanly separable if a smaller review is wanted. ## Backend — alert state - **Four columns on `synthetics`** (`consecutive_failures`, `last_alert_at`, `alerting`, `degraded_notified_at`), all defaulted so no backfill is needed - **`DB_SCHEMA_VERSION` 55 → 56** — without the bump the migrator short-circuits and the columns never appear - Guarded with `has_column` on **both** `up` and `down`: SQLite ignores `IF NOT EXISTS` on `ADD COLUMN`, and sea-query has no `drop_column_if_exists` - **Compare-and-swap** accessor (`update_alert_state_if`), because the caller does a read-modify-write with no transaction - `alerting` is a separate column because without it "recovered" cannot be told from "was never alerting" - **`synthetics_jobs::failing_locations`** — the notification previously had no location field, so a six-location check with one broken region could only say "the check is failing" - Distinct subject, headline, icon and colour for FLAKY vs DEGRADED, from one shared `locations_line` helper so Slack/plain-text/email cannot drift > The table is `synthetics`, not `synthetics_monitors` — the entity module is named after the concept but declares `table_name = "synthetics"`. Recorded at the `DeriveIden` so the next column addition doesn't repeat it. ## Performance — the step-stats panel fetched ~35 MB per load - **`recorded_steps` out of the 5000-row tally** — ~60% of payload; ~4 KB/row and near-identical within a config version, so an *aggregation* re-read the same definitions 5000 times. Still written to every record, because steps are editable and history must not be rewritten - **`retry_history` out too**, replaced by `retry_step_ids` scanned only over rows that actually retried - **Key step stats by `step_id`, not name** — a rename *split* one step into two rows; two steps sharing a name *merged* into one - **Single-pass sparklines** — was O(steps²) per row: ~2M iterations for a 20-step journey, ~50M for a 100-step one - **Flaky Rate could exceed 100%** — the fallback incremented the numerator without the denominator - **Label the real window** — a 1-min check × 2 locations × 4 device combos is 11,520 executions/day, so a 5000-row cap is ~10 hours of a "last 7 days" selection ## UI - **Attempts selector** in the run detail — costs no request, `retry_history` is already on the row. Labels are 1-based while `attempt` is 0-based - Each attempt resolves **its own** screenshots, trace and evidence bundle; a superseded attempt's live under an `attempt-N-` key - **Evidence panel** — parses `evidence.ndjson` per line, grouped by kind in severity order, chips with counts visible at zero, third-party dimmed not dropped, truncation stated, four distinct empty states - **Flaky / Degraded / Unstable tiles.** Unstable is partitioned by `(location, device, engine)` — aggregated, a check solidly broken in one region and healthy in five is indistinguishable from one intermittently broken everywhere. It measures *oscillation*, not badness - All flakiness tiles read **"—" at `retries = 0`**, never `0.0%`: a zero is a measurement, that is the absence of one - **`init_ms` and queue delay shown separately** — init was observed at 113,131 ms on a cold Lambda against a 243 ms check; unsubtracted, Lambda locations look permanently slower at every percentile - Steps and Evidence are sibling `OTabPanel`s — stacking them broke the drawer's scroll ## Bugs fixed during review - **`mapRetryHistory` guarded on `Array.isArray`** but the search API returns blob columns as JSON *strings*, so it returned `[]` for every real record — the attempts strip could never render, whatever the query selected. Same bug in `mapEvidence` and the `attempts` fallback - **`mapRunDetail` read `rawHit.attempt`**; the field is `attempts`. The count was 0 on every run - **`mapRetryHistory` hard-coded `status: "failed"`** — reported a flaky run's *passing* final attempt as a failure - **A superseded attempt's failing step rendered as a pass** — the compact timeline's `passed`/`failed` was never normalised to the `ok`/`fail` vocabulary every consumer tests for - **`failure_detail` is not a column** — OpenObserve flattens nested objects; naming the object is rejected by the search API - **The retry-attribution query named `status_reason` unconditionally.** That field only exists once something has been a `warning`, and the three step queries share a `Promise.all` — so one missing column emptied the *entire* Steps tab - **`fetchSchemaFields` returned an empty Set on failure**, which the gate reads as "the schema has no fields", silently degrading every optional column to a literal ## Lease correctness (added after review of the concurrency docs) Two corruption-class bugs, both of which double-count the alert failure streak this PR introduces. Included here rather than in the concurrency branch because they undermine *this* PR's own feature. - **The browser lease validation was per-device while the work is per-job.** `validate_browser_config` computed `(retries+1) x journey_budget + retries x wait` against the 900s lease, but the probe runs `browser_devices` **sequentially inside** the leased job and nothing multiplied by the combo count (`MAX_BROWSER_DEVICE_COMBOS = 12`). At defaults a plain **desktop + mobile** config is 1210s of work; validation computed 605s and allowed it. It then blew its 900s lease on **every run, forever**, with nothing failing at save. At the cap: 7260s vs 900s. The consequence is verbatim what the `LEASE_SECS` comment in `dispatcher/mod.rs` was written to prevent — *"the reaper would requeue mid-run and the journey would EXECUTE AGAIN — duplicate result records, multiplied browser cost, and false alerts caused by the reliability fix itself."* All three happen at two devices today, on the **managed** path, since browser is Lambda-only. > ⚠️ **Breaking validation change.** An existing 2+ device check will fail to save until its budget, retries or combo count comes down. The error now names the combo count as a lever. **Query dev and prod for `json_array_length(config->'browser_devices') > 1` before rolling out.** - **`ack_complete` had no status guard** — `WHERE id = $4` alone. A duplicate ack succeeded and the caller called `increment_jobs_done` again for one job: `jobs_done` overshot `job_count`, the run was declared complete on a **partial set**, and since this PR it also advanced `consecutive_failures` twice, so `alert_if_fails: 3` fires after two failures. Reachable today with no concurrency work: `aws-sdk-lambda` retries a timed-out `RequestResponse` invoke, re-executing a check already in flight. `AND status = 1` makes it idempotent, and `ack_complete` now returns `None` when it did not apply so the caller skips run accounting. Reaper reassignment is covered too, as of the concurrency work merged below: `AND claimed_by = $5`. ## Verification `vue-tsc` + `type-check:app` clean · Prettier + design ratchet clean · **711 synthetics tests** · `cargo check` clean · infra unit tests pass. Migration applied against a real SQLite metastore: `55 → 56`, all columns correct. Every drawer field verified against live ingested data. > `MonitorResults.spec.ts` fails on a pre-existing `storage.ts` import error — identical on a stashed tree. ## Requires o2-enterprise#2305 — the `AlertDecision` this consumes. Merge together; each is inert alone. --- # Also merged: probe concurrency (#13552) The concurrency branch merged into this one, so this PR now carries both. Everything below is a correctness fix — none of it is throughput work. ### The reaper never completed a run, on any path A run completes when `jobs_done` reaches `job_count`, and only three callers move that counter: the probe ack, the dispatcher's failed invoke, and the reaper. The reaper never did — including in `dead_letter_expired`, whose whole purpose is to report "we gave up after N attempts". It wrote a customer-visible error record into a run that then stayed open forever: no `completed_at`, and since this PR's alerting work, no `resolve_alert` either, so the failure streak never moved. The mechanism was `requeue_expired` ignoring `valid_until`. It reset a job to Pending, and `prune_stale` — later in the **same tick** — deleted it again, because `valid_until` is one interval (60s for a 1-minute check) while a lease is 300s+. `dispatch_attempts` never got past 1, so `MAX_DISPATCH_ATTEMPTS` was unreachable and the retry budget was dead code on the agent path. - requeue only while the job is still inside its window - dead-letter everything that can no longer report, tagged with why (attempts exhausted / window closed / never dispatched), so a dead agent does not read as an unresponsive probe - per-row compare-and-swap instead of SELECT-then-bulk-UPDATE; the reaper runs on every alert_manager node - increment **before** the ingest-token lookup, which returns early when an org has no token ### Every check's retry sequence is bounded by the lease Retries run *inside* the leased job. The browser path has checked this since `journey_budget_ms`; the protocol path never did, and its inputs were unbounded — the `5_000..=300_000` bound on `timeout_ms` is **browser-only**, so a TCP check with `timeout_ms: 3_600_000` saved fine. With the reaper now completing runs, the symptom is a wrong answer rather than a stuck one: the lease expires mid-flight, the reaper completes the run as Error, and the probe's real ack is rejected as stale. A passing check reports Error permanently. `timeout_ms=100s, retries=2, wait=30` needs 360s and hits it. `lease_batch` treats the client's `lease_secs` as a floor request, not the decision — both probes guess 300s, and a probe cannot know how long its own job may take. ### Guards on the ack, and on dispatch failure `AND claimed_by = $5` closes reaper reassignment. `claimed_by` is `Option` during rollout: an older probe acks without it and falls back to the status guard, so nothing is discarded. `fail_dispatch` had no status guard, so a dispatch judged failed *after* the probe acked reset a **completed** job to Pending — leased again, run again, duplicate result, `increment_jobs_done` twice. Third outcome `AlreadySettled` added; the dispatcher matches all three with no wildcard, so handling it is compile-time enforced. `limit` is now clamped before the query. It is cast to `u64`, where a negative wraps to ~1.8e19 and the LIMIT goes unbounded — one agent leasing the entire pending queue for its pool. Reachable from one mistyped env var. ### Queue backlog is observable `synthetics_pending_jobs` and `synthetics_oldest_pending_age_seconds`, per location and pool. `started_ts - scheduled_ts` already covered work that *ran*; a check nobody leased produces no record, so its backlog was invisible by construction. ## Before merging this to main The `timeout_ms` validation is a **breaking change for existing protocol configs**. It runs at create/update only, so nothing stops on deploy — but an over-budget check is rejected the next time someone edits it. Expected result is zero rows: `retries` of 0 or 1 cannot trip the budget rule at all, and the defaults sit at 10,000ms, 1.1% of the lease. ```sql WITH c AS ( SELECT id, org_id, name, synthetics_type, COALESCE((config->>'timeout_ms')::bigint, 10000) AS t, COALESCE((settings->>'retries')::bigint, 0) AS r, COALESCE((settings->>'wait_before_retry_secs')::bigint, 5) AS w FROM synthetics WHERE synthetics_type <> 'browser' ) SELECT id, org_id, name, t AS timeout_ms, r AS retries, w AS wait_s, (r + 1) * t + r * w * 1000 AS worst_case_ms FROM c WHERE t NOT BETWEEN 1000 AND 300000 OR (r + 1) * t + r * w * 1000 > 900000 ORDER BY worst_case_ms DESC; ``` Anything returned is a check that **cannot work today either** — fix the config rather than loosening the rule. ## Not done Wiring the reaper's now-completing runs to a **notification**. The send lives in a crate that depends on `o2_enterprise`, so the reaper cannot call up into it; it needs the send moved into `openobserve_core::synthetics` or a queue. No regression — before this, the run never completed, so there was no notification either. --------- Signed-off-by: Yashodhan Joshi Co-authored-by: omkark06 Co-authored-by: Harsh Mahajan <115500013+007harshmahajan@users.noreply.github.com> Co-authored-by: Yashodhan Joshi Co-authored-by: Yashodhan Joshi Co-authored-by: sai nikhil kethe Co-authored-by: Shrinath Rao Co-authored-by: Claude Opus 4.8 --- .../management/src/request/synthetics/mod.rs | 20 +- src/config/src/config.rs | 2 +- src/config/src/meta/synthetics.rs | 1444 ++++++++++++++++- src/config/src/metrics.rs | 48 + src/core/src/synthetics.rs | 119 +- .../src/table/entity/synthetics_monitors.rs | 19 + ..._add_alert_state_to_synthetics_monitors.rs | 168 ++ src/infra/src/table/migration/mod.rs | 2 + src/infra/src/table/synthetics_jobs.rs | 507 +++++- src/infra/src/table/synthetics_monitors.rs | 92 ++ .../CreateBrowserTest.schema.spec.ts | 260 +++ .../synthetics/CreateBrowserTest.schema.ts | 72 +- .../synthetics/StepEvidence.spec.ts | 199 +++ .../components/synthetics/StepEvidence.vue | 248 +++ .../synthetics/journey/BrowserJourney.spec.ts | 328 +++- .../synthetics/journey/BrowserJourney.vue | 348 ++-- .../journey/BrowserJourneyAssertion.spec.ts | 92 ++ .../journey/BrowserJourneyAssertion.vue | 104 ++ .../journey/BrowserJourneyLocator.spec.ts | 289 ++++ .../journey/BrowserJourneyLocator.vue | 307 ++++ .../journey/BrowserJourneyStep.spec.ts | 270 ++- .../synthetics/journey/BrowserJourneyStep.vue | 146 +- .../journey/BrowserJourneyStepEditor.spec.ts | 392 +++++ .../journey/BrowserJourneyStepEditor.vue | 579 +++++++ .../journey/BrowserJourneyStepError.spec.ts | 111 ++ .../journey/BrowserJourneyStepError.vue | 151 ++ .../synthetics/journey/RecordJourney.spec.ts | 287 ---- .../synthetics/journey/RecordJourney.vue | 193 --- .../journey/TestIdMisconfiguredNotice.spec.ts | 72 + .../journey/TestIdMisconfiguredNotice.vue | 82 + .../journey/UpgradeJourneyBanner.spec.ts | 83 + .../journey/UpgradeJourneyBanner.vue | 94 ++ .../journey/ZeroAssertionNotice.spec.ts | 65 + .../journey/ZeroAssertionNotice.vue | 88 + .../synthetics/results/EvidencePanel.spec.ts | 255 +++ .../synthetics/results/EvidencePanel.vue | 352 ++++ .../synthetics/attemptViews.spec.ts | 231 +++ .../synthetics/syntheticResultsSchema.spec.ts | 428 ++++- .../synthetics/syntheticResultsSchema.ts | 1070 +++++++++++- .../composables/useSyntheticResults.spec.ts | 67 +- web/src/composables/useSyntheticResults.ts | 158 +- .../composables/useSyntheticsRecorder.spec.ts | 74 +- web/src/composables/useSyntheticsRecorder.ts | 76 +- web/src/constants/synthetics.spec.ts | 72 + web/src/constants/synthetics.ts | 144 +- web/src/locales/languages/de-DE.json | 34 + web/src/locales/languages/en-US.json | 136 +- web/src/locales/languages/es-ES.json | 34 + web/src/locales/languages/fr-FR.json | 34 + web/src/locales/languages/it-IT.json | 34 + web/src/locales/languages/ja-JP.json | 34 + web/src/locales/languages/ko-KR.json | 34 + web/src/locales/languages/nl-NL.json | 34 + web/src/locales/languages/pl-PL.json | 34 + web/src/locales/languages/pt-PT.json | 34 + web/src/locales/languages/ru-RU.json | 34 + web/src/locales/languages/tr-TR.json | 34 + web/src/locales/languages/vi-VN.json | 34 + web/src/locales/languages/zh-CN.json | 34 + web/src/locales/languages/zh-TW.json | 34 + web/src/types/synthetics.ts | 132 +- web/src/utils/synthetics/buildPayload.ts | 12 +- web/src/utils/synthetics/buildV2Steps.spec.ts | 176 ++ web/src/utils/synthetics/buildV2Steps.ts | 168 ++ .../synthetics/deriveLocatorKind.spec.ts | 53 + web/src/utils/synthetics/deriveLocatorKind.ts | 43 + web/src/utils/synthetics/liftJourney.spec.ts | 251 +++ web/src/utils/synthetics/liftJourney.ts | 239 +++ .../utils/synthetics/locatorStability.spec.ts | 114 ++ web/src/utils/synthetics/locatorStability.ts | 87 + .../utils/synthetics/mapRecordedStep.spec.ts | 192 ++- web/src/utils/synthetics/mapRecordedStep.ts | 149 +- web/src/utils/synthetics/stepTarget.spec.ts | 86 + web/src/utils/synthetics/stepTarget.ts | 64 + .../views/synthetics/CreateBrowserTest.vue | 36 +- web/src/views/synthetics/MonitorResults.vue | 6 + web/src/views/synthetics/MonitorRuns.spec.ts | 148 +- web/src/views/synthetics/MonitorRuns.vue | 107 +- web/src/views/synthetics/RunDetail.spec.ts | 91 +- web/src/views/synthetics/RunDetail.vue | 782 +++++---- 80 files changed, 12426 insertions(+), 1330 deletions(-) create mode 100644 src/infra/src/table/migration/m20260730_000001_add_alert_state_to_synthetics_monitors.rs create mode 100644 web/src/components/synthetics/CreateBrowserTest.schema.spec.ts create mode 100644 web/src/components/synthetics/StepEvidence.spec.ts create mode 100644 web/src/components/synthetics/StepEvidence.vue create mode 100644 web/src/components/synthetics/journey/BrowserJourneyAssertion.spec.ts create mode 100644 web/src/components/synthetics/journey/BrowserJourneyAssertion.vue create mode 100644 web/src/components/synthetics/journey/BrowserJourneyLocator.spec.ts create mode 100644 web/src/components/synthetics/journey/BrowserJourneyLocator.vue create mode 100644 web/src/components/synthetics/journey/BrowserJourneyStepEditor.spec.ts create mode 100644 web/src/components/synthetics/journey/BrowserJourneyStepEditor.vue create mode 100644 web/src/components/synthetics/journey/BrowserJourneyStepError.spec.ts create mode 100644 web/src/components/synthetics/journey/BrowserJourneyStepError.vue delete mode 100644 web/src/components/synthetics/journey/RecordJourney.spec.ts delete mode 100644 web/src/components/synthetics/journey/RecordJourney.vue create mode 100644 web/src/components/synthetics/journey/TestIdMisconfiguredNotice.spec.ts create mode 100644 web/src/components/synthetics/journey/TestIdMisconfiguredNotice.vue create mode 100644 web/src/components/synthetics/journey/UpgradeJourneyBanner.spec.ts create mode 100644 web/src/components/synthetics/journey/UpgradeJourneyBanner.vue create mode 100644 web/src/components/synthetics/journey/ZeroAssertionNotice.spec.ts create mode 100644 web/src/components/synthetics/journey/ZeroAssertionNotice.vue create mode 100644 web/src/components/synthetics/results/EvidencePanel.spec.ts create mode 100644 web/src/components/synthetics/results/EvidencePanel.vue create mode 100644 web/src/composables/synthetics/attemptViews.spec.ts create mode 100644 web/src/constants/synthetics.spec.ts create mode 100644 web/src/utils/synthetics/buildV2Steps.spec.ts create mode 100644 web/src/utils/synthetics/buildV2Steps.ts create mode 100644 web/src/utils/synthetics/deriveLocatorKind.spec.ts create mode 100644 web/src/utils/synthetics/deriveLocatorKind.ts create mode 100644 web/src/utils/synthetics/liftJourney.spec.ts create mode 100644 web/src/utils/synthetics/liftJourney.ts create mode 100644 web/src/utils/synthetics/locatorStability.spec.ts create mode 100644 web/src/utils/synthetics/locatorStability.ts create mode 100644 web/src/utils/synthetics/stepTarget.spec.ts create mode 100644 web/src/utils/synthetics/stepTarget.ts diff --git a/src/api/management/src/request/synthetics/mod.rs b/src/api/management/src/request/synthetics/mod.rs index 9f02fb6c41..7013e6834d 100644 --- a/src/api/management/src/request/synthetics/mod.rs +++ b/src/api/management/src/request/synthetics/mod.rs @@ -1113,8 +1113,19 @@ async fn process_ack( ..Default::default() }); - // Notify once per run, not once per job ack. - if resp.run_complete && !resp.destinations.is_empty() { + // Notify once per run, not once per job ack — and only when the check's own + // `alert_if_fails` / `cooldown_mins` settings say so. This used to fire on + // every completed run that had a destination, which is why `alert_if_fails: + // 3` alerted on the first failure and a 30-minute cooldown sent thirty + // notifications. + use o2_enterprise::enterprise::synthetics::job_api::AlertDecision; + let recovery = matches!(resp.alert, AlertDecision::Recovered); + let flaky = matches!(resp.alert, AlertDecision::Flaky); + // A degrading target is `warning` on every run for as long as the condition + // lasts, so this one is throttled by transition upstream, not by cooldown. + let degraded = matches!(resp.alert, AlertDecision::Degraded); + let should_notify = !matches!(resp.alert, AlertDecision::Silent); + if should_notify && !resp.destinations.is_empty() { let notification = openobserve_core::synthetics::CheckNotification { org_id: resp.org_id.clone(), monitor_name: resp.synthetics_name.clone(), @@ -1127,6 +1138,11 @@ async fn process_ack( job_count: resp.job_count as i64, error: error.clone(), checked_at, + recovery, + consecutive_failures: resp.consecutive_failures, + flaky, + degraded, + failing_locations: resp.failing_locations.clone(), }; tokio::spawn(async move { openobserve_core::synthetics::notify_check_result(notification).await; diff --git a/src/config/src/config.rs b/src/config/src/config.rs index 7b2d7835e8..7d357ed529 100644 --- a/src/config/src/config.rs +++ b/src/config/src/config.rs @@ -52,7 +52,7 @@ pub type RwAHashSet = tokio::sync::RwLock>; pub type RwBTreeMap = tokio::sync::RwLock>; // for DDL commands and migrations -pub const DB_SCHEMA_VERSION: u64 = 55; +pub const DB_SCHEMA_VERSION: u64 = 56; pub const DB_SCHEMA_KEY: &str = "/db_schema_version/"; // global version variables diff --git a/src/config/src/meta/synthetics.rs b/src/config/src/meta/synthetics.rs index 8dfb8a2e9d..d0eeced603 100644 --- a/src/config/src/meta/synthetics.rs +++ b/src/config/src/meta/synthetics.rs @@ -619,6 +619,128 @@ pub struct SshAuth { pub secret: String, } +// ── Version-2 step record ───────────────────────────────────────────────────── +// +// v1 steps are untyped JSON with a single `selector` and a recorder-stamped +// `timeout_ms`. v2 replaces that with this typed, server-validated structure. +// +// The envelope is defined ONCE, complete, even though later phases populate +// parts of it: `settle.navigation` (Phase 3), `settle.responses` (Phase 4), +// `assertion` / `optional` / `always_run` (Phase 5). Bumping `steps_version` per +// phase would break deployment skew — `deny_unknown_fields` means an additive +// field from a newer recorder would be refused by an older server. Every block +// except `locator` is optional, with a defined absent-behaviour, which is what +// lets the phases ship independently. + +/// One way to find an element. Ordered most-stable-first inside a bundle. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(deny_unknown_fields)] +pub struct LocatorCandidate { + /// "test_attribute" | "role" | "text" | "css" | "xpath" + pub kind: String, + pub value: String, +} + +/// Every way the recorder found to identify one element, plus an optional +/// author pin. Candidates are machine-derived evidence and read-only in the UI; +/// `user_override` is the only channel for author intent, which is what makes +/// "never heal a pinned step" fall out for free. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] +pub struct StepLocator { + #[serde(default)] + pub candidates: Vec, + /// When set, used exclusively — never falls back to `candidates`. + #[serde(default)] + pub user_override: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SettleNavigation { + pub url_pattern: String, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SettleResponse { + pub url_pattern: String, + #[serde(default)] + pub method: Option, + /// Advisory by default: a signal that never fires annotates the step rather + /// than failing it. Only an author may set this — the recorder always emits + /// `false`. + #[serde(default)] + pub required: bool, +} + +/// What the page demonstrably did after this step, observed during recording and +/// replayed as explicit wait conditions. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] +pub struct StepSettle { + #[serde(default)] + pub navigation: Option, + #[serde(default)] + pub responses: Vec, + /// How long settling actually took while recording. Used for reporting + /// ("normally ~2s, today 40s") — never as a timeout. + #[serde(default)] + pub observed_duration_ms: Option, + /// How long this step may spend settling. `None` means the runner's default + /// (30s). + /// + /// This is where a retired hard sleep goes when a journey is lifted + /// (P3.4.3): `wait 30000` becomes a 30s settle budget on the step BEFORE it, + /// so the author's intent — "this step needs longer than usual" — survives + /// while the unconditional sleep does not. + #[serde(default)] + pub budget_ms: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct StepAssertion { + pub kind: String, + #[serde(default)] + pub expected: Option, + #[serde(default)] + pub attribute: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct BrowserStepV2 { + pub id: String, + pub action: String, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub url: Option, + #[serde(default)] + pub locator: Option, + #[serde(default)] + pub value: Option, + #[serde(default)] + pub key: Option, + #[serde(default)] + pub files: Option>, + #[serde(default)] + pub settle: Option, + #[serde(default)] + pub assertion: Option, + /// Failure skips the step and the run continues (cookie banners, popups). + #[serde(default)] + pub optional: bool, + /// Runs even after an earlier step failed (logout, cleanup). + #[serde(default)] + pub always_run: bool, + /// `None` means "use the runner's per-category default". An explicit value + /// is a deliberate author choice, validated into 100..=60000. + #[serde(default)] + pub timeout_ms: Option, +} + /// A (browser, device) pair for browser monitor fan-out. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct BrowserDevice { @@ -633,15 +755,41 @@ pub struct BrowserConfig { #[serde(default = "default_browser_devices")] pub browser_devices: Vec, pub runtime: Option, + /// Which step schema `steps` uses. Explicit, never inferred from shape. + /// Absent means 1, so every stored monitor reads back as v1. + #[serde(default = "default_steps_version")] + pub steps_version: u8, #[serde(default)] pub steps: Vec, #[serde(default)] pub env: Vec, #[serde(default)] pub secrets: Vec, + /// Playwright's page default timeout — NOT a check timeout, despite the + /// name. Every step passes an explicit timeout, so this does not cap the + /// runner's per-category defaults. #[serde(default = "default_browser_timeout_ms")] pub timeout_ms: u32, + /// Wall-clock ceiling for one attempt. Absent means + /// [`DEFAULT_JOURNEY_BUDGET_MS`]. Validated against the job lease so a full + /// retry sequence cannot outlive it — see `validate_browser_config`. + pub journey_budget_ms: Option, pub capture: Option, + /// The DOM attribute the recorder selects on for this monitor. + /// + /// Absent means [`DEFAULT_TEST_ID_ATTR`]. It exists because the attribute is + /// a property of the application under test, not of OpenObserve: Playwright + /// defaults to `data-testid`, O2's own frontend uses `data-test`, and a + /// customer may use `data-qa`, `data-cy` or `data-automation-id`. + /// + /// Getting it wrong is silent. Upstream's generator carries a hardcoded + /// fallback list (`data-testid`, `data-test-id`, `data-test`), so an + /// application outside that list produces NO `test_attribute` candidates at + /// all and every step degrades to role/text/css without any error. + /// + /// Recorded per monitor rather than per org so a journey that was recorded + /// against one application keeps working when another is added. + pub test_id_attr: Option, } /// A recorder-captured secret form value (e.g. a password typed during a login @@ -683,6 +831,10 @@ fn default_browser_timeout_ms() -> u32 { 30_000 } +fn default_steps_version() -> u8 { + 1 +} + fn default_tls_port() -> u16 { 443 } @@ -773,8 +925,177 @@ const SELECTOR_ACTIONS: &[&str] = &[ "setInputFiles", ]; +/// Wall-clock ceiling for ONE browser attempt, in milliseconds. +/// +/// The recorder used to stamp a 10s timeout into every step, so runs were short +/// and nobody needed a budget. With runner-owned defaults (60s navigation and +/// assertions, 30s interactions) and `retries` defaulting to 1, an 18-step +/// journey's worst case goes from ~180s to ~1085s — past the job lease, after +/// which the reaper requeues the job and it runs again. +const DEFAULT_JOURNEY_BUDGET_MS: u32 = 300_000; +const MIN_JOURNEY_BUDGET_MS: u32 = 5_000; +const MAX_JOURNEY_BUDGET_MS: u32 = 900_000; + +/// Lease held on a job while it runs — must stay in sync with `LEASE_SECS` in +/// o2-enterprise `synthetics/dispatcher/mod.rs`, and is the floor the job API +/// applies to every agent lease (`synthetics_jobs::lease_batch`). +/// +/// Applies to every check type, not just browser. Both agents ask for a 300s +/// lease, so any check whose retry sequence runs longer than that has its lease +/// expire while the probe is still working — the reaper then terminates the job +/// and completes the run as an error, and the probe's real result is rejected +/// when it finally acks. The validation below is what keeps every check's worst +/// case inside this number, which is in turn what lets the server lease for it +/// unconditionally. +/// +/// NOTE: the AWS Lambda function timeout must be >= this value, or runs are +/// killed mid-journey. That setting lives outside this repository and cannot be +/// asserted here. 900s is also AWS's maximum, so raising `retries` or +/// `journey_budget_ms` further requires re-deriving all three together. +pub const JOB_LEASE_SECS: i64 = 900; + +/// Ceiling for ONE attempt of a non-browser check, in milliseconds. +/// +/// Net `timeout_ms` was previously unbounded: every protocol config defaults it +/// to 10s, but nothing rejected `timeout_ms: 3_600_000`, so the worst case of a +/// retry sequence had no upper limit and could not be checked against the lease. +const MAX_NET_TIMEOUT_MS: u32 = 300_000; +const MIN_NET_TIMEOUT_MS: u32 = 1_000; + +/// Worst-case wall clock for one leased job, in milliseconds. +/// +/// Retries happen INSIDE the leased job, so the lease has to cover the whole +/// sequence rather than one attempt: `attempts x per_attempt + gaps`. The +/// `multiplier` is for work the probe repeats sequentially within the same job — +/// browser device combos — which the lease also covers. +/// +/// Shared by the browser and protocol paths so the two cannot drift; they differ +/// only in what one attempt costs and whether anything multiplies it. +fn worst_case_run_ms( + per_attempt_ms: u32, + multiplier: i64, + retries: i32, + wait_before_retry_secs: i32, +) -> i64 { + let attempts = i64::from(retries) + 1; + multiplier + * (attempts * i64::from(per_attempt_ms) + + i64::from(retries) * i64::from(wait_before_retry_secs) * 1_000) +} + +/// Bounds a protocol check's `timeout_ms` and its full retry sequence. +/// +/// The browser path has had this since `journey_budget_ms` was introduced; the +/// protocol path never did. Both agents ask for a 300s lease while +/// `timeout_ms` was unbounded and `retries` goes to 3, so a check needing more +/// than the lease was accepted, and then on every single run the reaper +/// terminated the job mid-flight and the probe's real result was thrown away as +/// a stale ack — a passing check reporting an error forever. +/// +/// `timeout_ms` is read from the raw config rather than a typed struct because +/// every protocol config declares the same field with the same serde default, so +/// an absent field legitimately means the default. +fn validate_net_retry_budget( + config: &serde_json::Value, + retries: i32, + wait_before_retry_secs: i32, +) -> Result<(), String> { + let timeout_ms = config + .get("timeout_ms") + .and_then(|v| v.as_u64()) + .and_then(|v| u32::try_from(v).ok()) + .unwrap_or_else(default_timeout_ms); + + if !(MIN_NET_TIMEOUT_MS..=MAX_NET_TIMEOUT_MS).contains(&timeout_ms) { + return Err(format!( + "config.timeout_ms: must be {MIN_NET_TIMEOUT_MS}..={MAX_NET_TIMEOUT_MS}, got {timeout_ms}" + )); + } + + let worst_case_ms = worst_case_run_ms(timeout_ms, 1, retries, wait_before_retry_secs); + if worst_case_ms > JOB_LEASE_SECS * 1_000 { + return Err(format!( + "config: a full retry sequence must fit inside the {JOB_LEASE_SECS}s job lease, but \ + (retries={retries} + 1) x timeout_ms={timeout_ms} + retries x \ + wait_before_retry_secs={wait_before_retry_secs} needs {worst_case_ms}ms. Lower \ + timeout_ms, retries, or wait_before_retry_secs — a check that outlives its lease has \ + its job terminated mid-run and its real result rejected as a stale ack." + )); + } + Ok(()) +} + +/// The complete v2 action vocabulary — exactly Playwright's recorder action +/// model, minus what a monitor cannot use. +/// +/// Deliberately excludes `hover`, `scroll`, `wait`/`waitFor` and `screenshot`: +/// upstream `ActionName` has no counterpart for any of them, so the recorder +/// never emitted one and the extension player could never replay one. They +/// entered journeys only through the manual step editor, and using one aborted +/// replay entirely. `type` and `keydown` are dropped as redundant aliases of +/// `fill` and `press`. +/// +/// Because this set is drawn from Playwright's own model, every stored v2 step +/// is executable by both the probe and the extension player by construction. +const V2_STEP_ACTIONS: &[&str] = &[ + "navigate", "click", "fill", "press", "select", "check", "uncheck", "upload", "assert", +]; + +/// v2 actions that operate on an element and therefore need a locator. +const V2_ELEMENT_ACTIONS: &[&str] = &[ + "click", "fill", "press", "select", "check", "uncheck", "upload", "assert", +]; + +/// Actions retired from the vocabulary (spec X-9, Q-10 / D-6). +/// +/// A stored v1 monitor containing one keeps EXECUTING — validation runs on +/// create and update, never on read — but it cannot be saved again until it is +/// migrated. Rejecting on write while accepting on read is what lets the +/// retirement land without a flag day, and what stops an author unknowingly +/// re-saving a journey whose sleeps are the reason it is flaky. +const RETIRED_STEP_ACTIONS: &[&str] = &["hover", "scroll", "wait", "waitFor", "screenshot"]; + +/// The closed set of assertion kinds (spec P5.1). +/// +/// Closed on purpose: the probe fails an unknown kind rather than passing it, so +/// a typo caught here is an error at save time instead of every run failing — +/// the same reasoning as the HTTP assertion field/operator sets above. +const V2_ASSERTION_KINDS: &[&str] = &[ + "element_visible", + "element_not_visible", + "element_text", + "url_matches", + "page_title", + "element_attribute", +]; + +/// Kinds that ask "is it there?" and so have nothing to compare against. +const V2_VISIBILITY_ASSERTION_KINDS: &[&str] = &["element_visible", "element_not_visible"]; + +/// Kinds that describe the page rather than an element, and so need no locator. +const V2_PAGE_LEVEL_ASSERTION_KINDS: &[&str] = &["url_matches", "page_title"]; + const MAX_STEPS: usize = 50; const MAX_STEPS_JSON_BYTES: usize = 100_000; +/// v2 steps carry up to 5 locator candidates and 5 settle patterns each, roughly +/// 3-4x a v1 step. A maximal 50-step journey lands near 60KB; the cap is set well +/// clear of that. The `config` column is already JSON (jsonb on PostgreSQL), and +/// steps travel over the HTTP resolve/ack bodies rather than the Lambda invoke +/// payload, so neither storage nor transport needs changing. +const MAX_STEPS_JSON_BYTES_V2: usize = 262_144; +const MAX_LOCATOR_CANDIDATES: usize = 5; + +/// The recorder's test-id attribute when a monitor does not set one. +/// +/// `data-test` rather than Playwright's `data-testid`: OpenObserve's own +/// frontend marks interactive elements with it, and self-monitoring is this +/// feature's acceptance test (X-1's o2.introspect monitors). +pub const DEFAULT_TEST_ID_ATTR: &str = "data-test"; + +/// Longest attribute name accepted. A DOM attribute name this long is not a +/// configuration, it is a paste accident. +const MAX_TEST_ID_ATTR_LEN: usize = 64; +const MAX_SETTLE_RESPONSES: usize = 5; const MAX_TAGS: usize = 20; const MAX_VARIABLES: usize = 50; const MAX_BROWSER_DEVICE_COMBOS: usize = 12; @@ -828,7 +1149,61 @@ fn location_allowed(loc: &str, allowed: &[String]) -> bool { && allowed.iter().any(|a| a == &format!("aws-{loc}")) } +/// A save-time warning: the monitor is accepted, but something about it is worth +/// telling the author. +/// +/// Separate from the `Err(String)` channel on purpose. A zero-assertion journey +/// is legitimate — a monitor that only navigates still proves the site answers — +/// so refusing it would be wrong; but it can also click its way through a broken +/// application and pass, which is worth saying out loud (P5.2.4). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum SyntheticWarningCode { + NoAssertions, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct SyntheticWarning { + pub code: SyntheticWarningCode, + /// Machine-readable code plus prose, so a UI can key off one and show the + /// other without re-deriving the wording. + pub message: String, +} + impl Synthetic { + /// Non-blocking problems worth surfacing when a monitor is saved. + /// + /// Deliberately not part of `validate`: everything here is accepted. A caller + /// that ignores this returns exactly the behaviour it had before. + pub fn warnings(&self) -> Vec { + let mut warnings = Vec::new(); + + if self.monitor_type == SyntheticType::Browser { + let has_assertion = self + .config + .get("steps") + .and_then(|s| s.as_array()) + .is_some_and(|steps| { + steps + .iter() + .any(|step| step.get("action").and_then(|a| a.as_str()) == Some("assert")) + }); + if !has_assertion { + warnings.push(SyntheticWarning { + code: SyntheticWarningCode::NoAssertions, + message: "This journey contains no assertions, so it verifies that the steps \ + can be performed but not that the application is working. A journey \ + with no assertion can click its way through a broken page and still \ + pass. Add at least one assertion — for example that a \ + post-login element is visible." + .to_string(), + }); + } + } + + warnings + } + /// Validates a create/update payload. `allowed_*` come from the deployment's /// synthetics capabilities. Empty `allowed_browsers`/`allowed_devices` skip /// the corresponding membership check; an empty `allowed_locations` REJECTS @@ -1049,11 +1424,18 @@ impl Synthetic { allowed_browsers: &[String], allowed_devices: &[String], ) -> Result<(), String> { - match self.monitor_type { + let type_check = match self.monitor_type { SyntheticType::Browser => { let cfg: BrowserConfig = serde_json::from_value(self.config.clone()) .map_err(|e| format!("config: not a valid browser config: {e}"))?; - validate_browser_config(&cfg, &self.frequency, allowed_browsers, allowed_devices) + validate_browser_config( + &cfg, + &self.frequency, + allowed_browsers, + allowed_devices, + self.retries, + self.wait_before_retry_secs, + ) } SyntheticType::Http | SyntheticType::Api => { let cfg: HttpConfig = serde_json::from_value(self.config.clone()) @@ -1118,8 +1500,228 @@ impl Synthetic { // banner check (a rejected auth still proves SSH is up). Ok(()) } + }; + // Type errors first: "not a valid tcp config" is more fundamental than + // anything derived from its fields, and a config that fails to parse has + // no meaningful timeout to report on. + type_check?; + + // Every protocol check gets the same retry-budget-vs-lease bound the + // browser path applies inside `validate_browser_config`. Done here, once, + // rather than in each arm: the arms are per-type and this rule is not, and + // adding a check type should not be able to opt out of it silently. + if self.monitor_type != SyntheticType::Browser { + validate_net_retry_budget(&self.config, self.retries, self.wait_before_retry_secs)?; + } + Ok(()) + } +} + +/// Validates version-2 steps: typed deserialization plus the structural rules +/// that deserialization cannot express. +/// +/// Deserialization does most of the work. Every v2 struct is +/// `deny_unknown_fields`, so a step carrying `code` — or anything else the +/// schema does not know — is refused here. That is what closes the +/// arbitrary-JavaScript path at the schema level rather than relying on the +/// runner to ignore it. +/// Validate one typed assertion (spec P5.1.1–P5.1.3). +/// +/// Follows the shape the HTTP assertions already use: a closed kind set, the +/// values each kind actually needs, and errors naming both the step index and +/// the known set — so a typo cannot silently pass forever. +fn validate_v2_assertion(i: usize, assertion: &StepAssertion) -> Result<(), String> { + if !V2_ASSERTION_KINDS.contains(&assertion.kind.as_str()) { + return Err(format!( + "config.steps[{i}].assertion.kind: unknown kind '{}' (known: {})", + assertion.kind, + V2_ASSERTION_KINDS.join(", ") + )); + } + + // The visibility kinds ask "is it there?" — there is nothing to compare. + if !V2_VISIBILITY_ASSERTION_KINDS.contains(&assertion.kind.as_str()) { + let expected = assertion.expected.as_deref().unwrap_or(""); + if expected.is_empty() { + return Err(format!( + "config.steps[{i}].assertion: kind '{}' requires a non-empty 'expected'", + assertion.kind + )); } } + + if assertion.kind == "element_attribute" + && assertion + .attribute + .as_deref() + .unwrap_or("") + .trim() + .is_empty() + { + return Err(format!( + "config.steps[{i}].assertion: kind 'element_attribute' requires an 'attribute' name" + )); + } + + Ok(()) +} + +/// Reject the retired actions on a v1 create or update (Q-10.a). +/// +/// The error names every offending index and action rather than the first, +/// because an author fixing them one round-trip at a time is exactly the +/// friction that makes people give up and leave the sleeps in. +fn reject_retired_v1_actions(steps: &[serde_json::Value]) -> Result<(), String> { + let offenders: Vec = steps + .iter() + .enumerate() + .filter_map(|(i, step)| { + let action = step.get("action").and_then(|v| v.as_str())?; + RETIRED_STEP_ACTIONS + .contains(&action) + .then(|| format!("{} ({action})", i + 1)) + }) + .collect(); + + if offenders.is_empty() { + return Ok(()); + } + + Err(format!( + "config.steps: step {} use actions that have been retired ({}). None of them can be \ + replayed — they have no counterpart in the recorder's action model — and a hard sleep is \ + the single largest source of the flakiness this schema exists to remove. Upgrade this \ + journey to steps_version 2, which converts each sleep into a settle budget on the \ + preceding step and drops the unreplayable steps. Existing runs are unaffected until you \ + save.", + offenders.join(", "), + RETIRED_STEP_ACTIONS.join(", ") + )) +} + +fn validate_v2_steps(steps: &[serde_json::Value]) -> Result<(), String> { + let mut seen_ids = std::collections::HashSet::new(); + + for (i, raw) in steps.iter().enumerate() { + let step: BrowserStepV2 = + serde_json::from_value(raw.clone()).map_err(|e| format!("config.steps[{i}]: {e}"))?; + + if step.id.is_empty() { + return Err(format!("config.steps[{i}]: 'id' must not be empty")); + } + if !seen_ids.insert(step.id.clone()) { + return Err(format!( + "config.steps[{i}]: duplicate step id '{}'", + step.id + )); + } + + if !V2_STEP_ACTIONS.contains(&step.action.as_str()) { + return Err(format!( + "config.steps[{i}]: action '{}' is not valid in steps_version 2 (valid: {}). \ + hover, scroll, wait and screenshot have no equivalent in the recorder's action \ + model and cannot be replayed; type and keydown are aliases of fill and press.", + step.action, + V2_STEP_ACTIONS.join(", ") + )); + } + + // The probe opens about:blank and never auto-navigates. + if i == 0 && step.action != "navigate" { + return Err(format!( + "config.steps[0]: first step must be 'navigate', got '{}'", + step.action + )); + } + + if step.action == "navigate" { + let url = step + .url + .as_deref() + .ok_or_else(|| format!("config.steps[{i}]: navigate step missing 'url'"))?; + validate_http_url(&format!("config.steps[{i}].url"), url)?; + } + + // P5.1.4 — an assertion is what an `assert` step IS, and is meaningless + // on any other action. Allowing it elsewhere would create a second, + // invisible place for a journey to state an expectation. + if step.action == "assert" { + let assertion = step.assertion.as_ref().ok_or_else(|| { + format!( + "config.steps[{i}]: 'assert' step requires an 'assertion' (kinds: {})", + V2_ASSERTION_KINDS.join(", ") + ) + })?; + validate_v2_assertion(i, assertion)?; + } else if step.assertion.is_some() { + return Err(format!( + "config.steps[{i}]: 'assertion' is only valid on an 'assert' step, not on '{}'", + step.action + )); + } + + // A page-level assertion is about the address bar or the document title, + // so requiring an element would make it depend on something unrelated + // still being on screen. + let needs_locator = V2_ELEMENT_ACTIONS.contains(&step.action.as_str()) + && !step + .assertion + .as_ref() + .is_some_and(|a| V2_PAGE_LEVEL_ASSERTION_KINDS.contains(&a.kind.as_str())); + + if needs_locator { + let locator = step.locator.as_ref().ok_or_else(|| { + format!( + "config.steps[{i}]: '{}' step requires a 'locator'", + step.action + ) + })?; + if locator.candidates.is_empty() && locator.user_override.is_none() { + return Err(format!( + "config.steps[{i}].locator: needs at least one candidate or a user_override" + )); + } + if locator.candidates.len() > MAX_LOCATOR_CANDIDATES { + return Err(format!( + "config.steps[{i}].locator.candidates: too many ({} > {MAX_LOCATOR_CANDIDATES})", + locator.candidates.len() + )); + } + } + + if (step.action == "fill" || step.action == "select") && step.value.is_none() { + return Err(format!( + "config.steps[{i}]: '{}' step requires a 'value'", + step.action + )); + } + + if let Some(settle) = &step.settle { + if settle.responses.len() > MAX_SETTLE_RESPONSES { + return Err(format!( + "config.steps[{i}].settle.responses: too many ({} > {MAX_SETTLE_RESPONSES})", + settle.responses.len() + )); + } + if let Some(budget) = settle.budget_ms + && !(100..=60_000).contains(&budget) + { + return Err(format!( + "config.steps[{i}].settle.budget_ms: must be 100..=60000, got {budget}" + )); + } + } + + if let Some(timeout) = step.timeout_ms + && !(100..=60_000).contains(&timeout) + { + return Err(format!( + "config.steps[{i}].timeout_ms: must be 100..=60000, got {timeout}" + )); + } + } + + Ok(()) } fn validate_browser_config( @@ -1127,7 +1729,73 @@ fn validate_browser_config( frequency: &SyntheticFrequency, allowed_browsers: &[String], allowed_devices: &[String], + retries: i32, + wait_before_retry_secs: i32, ) -> Result<(), String> { + // ── journey budget vs. job lease ─────────────────────────────────────── + // A browser job holds a lease while it runs (o2-enterprise dispatcher, + // LEASE_SECS). If a run outlives its lease the reaper requeues it and the + // journey EXECUTES AGAIN — duplicate result records, multiplied browser + // cost, and false alerts. Per-step timeouts alone cannot bound this, so the + // whole retry sequence must be checked against the lease up front. + let budget_ms = cfg.journey_budget_ms.unwrap_or(DEFAULT_JOURNEY_BUDGET_MS); + if !(MIN_JOURNEY_BUDGET_MS..=MAX_JOURNEY_BUDGET_MS).contains(&budget_ms) { + return Err(format!( + "config.journey_budget_ms: must be {MIN_JOURNEY_BUDGET_MS}..={MAX_JOURNEY_BUDGET_MS}, got {budget_ms}" + )); + } + // Multiplied by the device count, because the probe runs `browser_devices` + // SEQUENTIALLY INSIDE the leased job (`browser-probe/src/index.ts:124`) — the + // lease covers the whole job, not one device. + // + // Without this the check was per-device while the work was per-job, so a + // perfectly ordinary "desktop + mobile" config computed 605ms of budget, + // passed validation, and then blew its 900s lease on EVERY run: the reaper + // requeued mid-journey, the journey executed again, and that produced + // duplicate result records, doubled browser cost and alerts caused by the + // duplicate. Which is verbatim what the LEASE_SECS comment in + // `dispatcher/mod.rs` was written to prevent. + let devices = i64::try_from(cfg.browser_devices.len().max(1)).unwrap_or(1); + let worst_case_ms = worst_case_run_ms(budget_ms, devices, retries, wait_before_retry_secs); + if worst_case_ms > JOB_LEASE_SECS * 1_000 { + return Err(format!( + "config: a full retry sequence across all browser/device combos must fit inside the \ + {JOB_LEASE_SECS}s job lease, but browser_devices={devices} x (retries={retries} x \ + journey_budget_ms={budget_ms} + wait_before_retry_secs={wait_before_retry_secs}) needs \ + {worst_case_ms}ms. Lower journey_budget_ms, retries, or the number of browser/device \ + combos — a run that outlives its lease is requeued and executed a second time." + )); + } + + // ── recorder test-id attribute ───────────────────────────────────────── + // Validated rather than trusted because it is interpolated into a selector + // (`[="value"]`). A name with a quote or bracket in it would produce a + // selector that silently matches nothing, which is the failure mode this + // whole area exists to remove. + if let Some(attr) = &cfg.test_id_attr { + let trimmed = attr.trim(); + if trimmed.is_empty() { + return Err( + "config.test_id_attr: must not be blank — omit it to use the default".to_string(), + ); + } + if trimmed.len() > MAX_TEST_ID_ATTR_LEN { + return Err(format!( + "config.test_id_attr: too long ({} > {MAX_TEST_ID_ATTR_LEN})", + trimmed.len() + )); + } + if !trimmed + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + { + return Err(format!( + "config.test_id_attr: '{trimmed}' is not a valid attribute name \ + (letters, digits, '-' and '_' only)" + )); + } + } + // ── steps ────────────────────────────────────────────────────────────── if cfg.steps.is_empty() { return Err("config.steps: at least one step is required".to_string()); @@ -1141,12 +1809,33 @@ fn validate_browser_config( let steps_bytes = serde_json::to_string(&cfg.steps) .map(|s| s.len()) .unwrap_or(0); - if steps_bytes > MAX_STEPS_JSON_BYTES { + let max_bytes = if cfg.steps_version >= 2 { + MAX_STEPS_JSON_BYTES_V2 + } else { + MAX_STEPS_JSON_BYTES + }; + if steps_bytes > max_bytes { return Err(format!( - "config.steps: serialized steps too large ({steps_bytes} > {MAX_STEPS_JSON_BYTES} bytes)" + "config.steps: serialized steps too large ({steps_bytes} > {max_bytes} bytes)" )); } + // v2 steps are typed and validated separately; v1 keeps its historical + // untyped path byte-for-byte so no stored monitor changes behaviour. + if cfg.steps_version >= 2 { + validate_v2_steps(&cfg.steps)?; + return validate_browser_devices_and_schedule( + cfg, + frequency, + allowed_browsers, + allowed_devices, + ); + } + + // Q-10 / D-6: retired actions are refused on write while stored monitors + // carrying them keep executing on read. + reject_retired_v1_actions(&cfg.steps)?; + let mut seen_ids = std::collections::HashSet::new(); for (i, step) in cfg.steps.iter().enumerate() { let action = step @@ -1210,6 +1899,17 @@ fn validate_browser_config( } } + validate_browser_devices_and_schedule(cfg, frequency, allowed_browsers, allowed_devices) +} + +/// Shared by the v1 and v2 step paths — everything about a browser config that +/// is not the steps themselves. +fn validate_browser_devices_and_schedule( + cfg: &BrowserConfig, + frequency: &SyntheticFrequency, + allowed_browsers: &[String], + allowed_devices: &[String], +) -> Result<(), String> { // ── browser × device combos ──────────────────────────────────────────── if cfg.browser_devices.is_empty() { return Err( @@ -1553,6 +2253,95 @@ mod tests { ) } + fn valid_tcp_synthetic() -> Synthetic { + Synthetic { + name: "db port".to_string(), + monitor_type: SyntheticType::Tcp, + target: "db.example.com".to_string(), + frequency: SyntheticFrequency { + frequency_type: SyntheticFrequencyType::Minutes, + interval: 5, + cron: String::new(), + timezone: None, + }, + locations: vec!["aws-us-east-1".to_string()], + enabled: true, + alert_if_fails: 1, + wait_before_retry_secs: 5, + config: serde_json::json!({ "port": 5432, "timeout_ms": 10000 }), + ..Default::default() + } + } + + #[test] + fn net_retry_sequence_must_fit_the_job_lease() { + let (locs, brs, devs) = allowed(); + // The lease covers the whole retry sequence because retries run inside + // the leased job. 4 x 300s alone is 1200s, past the 900s lease. + let mut s = valid_tcp_synthetic(); + s.config = serde_json::json!({ "port": 5432, "timeout_ms": 300_000 }); + s.retries = 3; + s.wait_before_retry_secs = 0; + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("job lease"), "{err}"); + + // The gaps count too: 3 x 250s = 750s of attempts is fine on its own, + // but not with 300s of waiting between them. + let mut s = valid_tcp_synthetic(); + s.config = serde_json::json!({ "port": 5432, "timeout_ms": 250_000 }); + s.retries = 2; + s.wait_before_retry_secs = 300; + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("job lease"), "{err}"); + } + + #[test] + fn net_timeout_is_bounded_at_all() { + let (locs, brs, devs) = allowed(); + // Was previously unbounded: every protocol config defaults timeout_ms to + // 10s, but nothing rejected an hour, so the worst case had no ceiling to + // check the lease against. + let mut s = valid_tcp_synthetic(); + s.config = serde_json::json!({ "port": 5432, "timeout_ms": 3_600_000 }); + s.retries = 0; + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.starts_with("config.timeout_ms:"), "{err}"); + } + + #[test] + fn net_defaults_and_ordinary_configs_still_validate() { + let (locs, brs, devs) = allowed(); + // A config with no timeout_ms at all must use the serde default rather + // than 0, which would otherwise trip the new minimum. + let mut s = valid_tcp_synthetic(); + s.config = serde_json::json!({ "port": 5432 }); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + + // The worst realistic config: max retries and a 60s timeout is 240s of + // attempts plus 90s of gaps — comfortably inside the lease. This must + // keep passing, or the bound is too tight to be shipped. + let mut s = valid_tcp_synthetic(); + s.config = serde_json::json!({ "port": 5432, "timeout_ms": 60_000 }); + s.retries = 3; + s.wait_before_retry_secs = 30; + assert!( + s.validate(&locs, &brs, &devs, true).is_ok(), + "{:?}", + s.validate(&locs, &brs, &devs, true) + ); + } + + #[test] + fn worst_case_counts_attempts_gaps_and_the_multiplier() { + // retries=0 is one attempt and no gaps. + assert_eq!(worst_case_run_ms(10_000, 1, 0, 30), 10_000); + // retries=2 is three attempts and two gaps. + assert_eq!(worst_case_run_ms(10_000, 1, 2, 30), 30_000 + 60_000); + // The multiplier applies to the whole sequence, not one attempt — the + // probe repeats the entire retry sequence per device inside one lease. + assert_eq!(worst_case_run_ms(10_000, 2, 2, 30), 2 * (30_000 + 60_000)); + } + #[test] fn test_validate_ok() { let (locs, brs, devs) = allowed(); @@ -1563,6 +2352,653 @@ mod tests { ); } + // ── Version-2 steps (spec Phase 2, P2.1/P2.2) ──────────────────────────── + // v2 replaces the untyped step blob with a typed, server-validated one. The + // envelope is defined ONCE with its complete field set, and later phases + // populate blocks that ship optional from day one (settle, assertion, + // optional/always_run). Bumping the version per phase would break + // deployment skew: v2 rejects unknown fields, so an additive field from a + // newer recorder would be refused by an older server. + + fn v2_synthetic(steps: serde_json::Value) -> Synthetic { + let mut s = valid_browser_synthetic(); + s.config["steps_version"] = serde_json::json!(2); + s.config["steps"] = steps; + s + } + + fn v2_click_step() -> serde_json::Value { + serde_json::json!({ + "id": "s2", + "action": "click", + "name": "Sign In", + "locator": { + "candidates": [ + { "kind": "test_attribute", "value": "[data-test=\"login-sign-in\"]" }, + { "kind": "role", "value": "role=button[name=\"Sign In\"]" } + ] + } + }) + } + + fn v2_nav_step() -> serde_json::Value { + serde_json::json!({ + "id": "s1", + "action": "navigate", + "name": "Open", + "url": "https://example.com" + }) + } + + #[test] + fn test_v2_minimal_journey_accepted() { + let (locs, brs, devs) = allowed(); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), v2_click_step()])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + // ── Recorder test-id attribute ─────────────────────────────────────────── + // The attribute is a property of the application under test, not of O2, and + // getting it wrong is SILENT: upstream's generator falls back to a hardcoded + // list, so an app outside it produces no test_attribute candidates at all. + + #[test] + fn test_test_id_attr_absent_is_valid() { + let (locs, brs, devs) = allowed(); + let s = valid_browser_synthetic(); + assert!(s.config.get("test_id_attr").is_none()); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_test_id_attr_accepts_real_attribute_names() { + let (locs, brs, devs) = allowed(); + for attr in [ + "data-test", + "data-testid", + "data-qa", + "data-cy", + "data_e2e", + "id", + ] { + let mut s = valid_browser_synthetic(); + s.config["test_id_attr"] = serde_json::json!(attr); + assert!( + s.validate(&locs, &brs, &devs, true).is_ok(), + "{attr} should be accepted: {:?}", + s.validate(&locs, &brs, &devs, true) + ); + } + } + + #[test] + fn test_test_id_attr_rejects_blank() { + // Blank would silently mean "everything" once interpolated into + // `[="value"]`. Omitting the field is how you ask for the default. + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.config["test_id_attr"] = serde_json::json!(" "); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("test_id_attr"), "{err}"); + } + + #[test] + fn test_test_id_attr_rejects_selector_injection() { + // The value is interpolated into a selector, so a quote or bracket would + // produce one that silently matches nothing. + let (locs, brs, devs) = allowed(); + for bad in ["data-test=\"x\"", "data test", "data]test", "a*b"] { + let mut s = valid_browser_synthetic(); + s.config["test_id_attr"] = serde_json::json!(bad); + assert!( + s.validate(&locs, &brs, &devs, true).is_err(), + "{bad} should be rejected" + ); + } + } + + #[test] + fn test_test_id_attr_rejects_absurd_length() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.config["test_id_attr"] = serde_json::json!("d".repeat(MAX_TEST_ID_ATTR_LEN + 1)); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("too long"), "{err}"); + } + + #[test] + fn test_v1_unchanged_when_steps_version_absent() { + // Regression guard: every stored monitor must keep validating exactly as + // before. v1 steps carry a bare `selector` and no locator bundle. + let (locs, brs, devs) = allowed(); + let s = valid_browser_synthetic(); + assert!(s.config.get("steps_version").is_none()); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_v2_rejects_unknown_fields() { + // This is the mechanism that closes the arbitrary-code hole: anything + // the schema does not know about is refused, so `code` cannot ride along + // under a different name either. + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step["surprise"] = serde_json::json!("hello"); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("surprise"), "{err}"); + } + + #[test] + fn test_v2_rejects_code_field() { + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step["code"] = serde_json::json!("await page.evaluate(() => fetch('http://evil'))"); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("code"), "{err}"); + } + + #[test] + fn test_v2_accepts_exactly_the_nine_action_vocabulary() { + let (locs, brs, devs) = allowed(); + for action in [ + "click", "fill", "press", "select", "check", "uncheck", "upload", + ] { + let mut step = v2_click_step(); + step["action"] = serde_json::json!(action); + if action == "fill" || action == "select" { + step["value"] = serde_json::json!("x"); + } + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + assert!( + s.validate(&locs, &brs, &devs, true).is_ok(), + "{action} should be accepted: {:?}", + s.validate(&locs, &brs, &devs, true) + ); + } + } + + #[test] + fn test_v2_rejects_retired_actions() { + // Upstream Playwright's recorder action model has no counterpart for any + // of these, so the recorder never emitted one and the player could never + // replay one. `type`/`keydown` are redundant aliases of fill/press. + let (locs, brs, devs) = allowed(); + for action in [ + "hover", + "scroll", + "wait", + "waitFor", + "screenshot", + "type", + "keydown", + ] { + let mut step = v2_click_step(); + step["action"] = serde_json::json!(action); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!( + err.contains(action) || err.contains("action"), + "{action} should be rejected in v2, got: {err}" + ); + } + } + + #[test] + fn test_v1_rejects_retired_actions_on_write_naming_every_offender() { + // Q-10.a: the error names EVERY offending step index and action, not the + // first — fixing them one round-trip at a time is the friction that makes + // authors give up and leave the sleeps in. + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.config["steps"] = serde_json::json!([ + { "id": "s1", "action": "navigate", "url": "https://example.com" }, + { "id": "s2", "action": "wait", "timeout_ms": 30000 }, + { "id": "s3", "action": "click", "selector": "#go" }, + { "id": "s4", "action": "hover", "selector": "#menu" } + ]); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("2 (wait)"), "{err}"); + assert!(err.contains("4 (hover)"), "{err}"); + assert!( + !err.contains("3 ("), + "the healthy click must not be named: {err}" + ); + // Q-10.b: the remedy is offered in the message, not left to be guessed. + assert!(err.contains("steps_version 2"), "{err}"); + } + + #[test] + fn test_v1_retired_actions_still_deserialize_so_stored_monitors_keep_running() { + // D-6: rejection is a WRITE-path rule. Nothing calls `validate` when a + // monitor is loaded to be executed, and a stored journey carrying a + // legacy `wait` must keep running until its author migrates it. + let cfg: BrowserConfig = serde_json::from_value(serde_json::json!({ + "steps": [ + { "id": "s1", "action": "navigate", "url": "https://example.com" }, + { "id": "s2", "action": "wait", "timeout_ms": 30000 } + ] + })) + .expect("a stored v1 config with a retired action must still load"); + assert_eq!(cfg.steps.len(), 2); + assert_eq!(cfg.steps[1]["action"], "wait"); + assert_eq!(cfg.steps_version, 1); + } + + fn v2_assert_step(assertion: serde_json::Value) -> serde_json::Value { + serde_json::json!({ + "id": "s3", + "action": "assert", + "name": "Profile visible", + "locator": { + "candidates": [ + { "kind": "test_attribute", "value": "[data-test=\"header-my-account-profile-icon\"]" } + ] + }, + "assertion": assertion + }) + } + + // ── Phase 5 — typed assertions (T5-1…T5-4) ────────────────────────────── + + #[test] + fn test_v2_every_assertion_kind_is_accepted() { + let (locs, brs, devs) = allowed(); + for kind in V2_ASSERTION_KINDS { + let mut assertion = serde_json::json!({ "kind": kind }); + if !V2_VISIBILITY_ASSERTION_KINDS.contains(kind) { + assertion["expected"] = serde_json::json!("something"); + } + if *kind == "element_attribute" { + assertion["attribute"] = serde_json::json!("href"); + } + let s = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_click_step(), + v2_assert_step(assertion) + ])); + assert!( + s.validate(&locs, &brs, &devs, true).is_ok(), + "kind '{kind}' must be accepted: {:?}", + s.validate(&locs, &brs, &devs, true) + ); + } + } + + #[test] + fn test_v2_unknown_assertion_kind_is_rejected_naming_the_known_set() { + let (locs, brs, devs) = allowed(); + let s = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_assert_step(serde_json::json!({ "kind": "element_vissible" })) + ])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("element_vissible"), "{err}"); + assert!( + err.contains("element_visible"), + "the known set must be listed: {err}" + ); + assert!(err.contains("steps[1]"), "the index must be named: {err}"); + } + + #[test] + fn test_v2_expected_is_required_except_for_the_visibility_kinds() { + let (locs, brs, devs) = allowed(); + for kind in ["element_text", "url_matches", "page_title"] { + let s = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_assert_step(serde_json::json!({ "kind": kind })) + ])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("expected"), "kind '{kind}': {err}"); + } + // …and an empty string is not a value. + let s = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_assert_step(serde_json::json!({ "kind": "element_text", "expected": "" })) + ])); + assert!(s.validate(&locs, &brs, &devs, true).is_err()); + } + + #[test] + fn test_v2_element_attribute_requires_an_attribute_name() { + let (locs, brs, devs) = allowed(); + let s = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_assert_step(serde_json::json!({ "kind": "element_attribute", "expected": "/web/" })) + ])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("attribute"), "{err}"); + } + + #[test] + fn test_v2_assertion_is_required_on_assert_and_forbidden_elsewhere() { + let (locs, brs, devs) = allowed(); + + let mut bare = v2_assert_step(serde_json::json!({ "kind": "element_visible" })); + bare.as_object_mut().unwrap().remove("assertion"); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), bare])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("requires an 'assertion'"), "{err}"); + + let mut click = v2_click_step(); + click["assertion"] = serde_json::json!({ "kind": "element_visible" }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), click])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("only valid on an 'assert' step"), "{err}"); + } + + #[test] + fn test_v2_page_level_assertions_need_no_locator() { + // A statement about the address bar must not depend on some unrelated + // element still being on screen. + let (locs, brs, devs) = allowed(); + let step = serde_json::json!({ + "id": "s3", + "action": "assert", + "assertion": { "kind": "url_matches", "expected": "**/web/**" } + }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), v2_click_step(), step])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_a_zero_assertion_journey_is_accepted_with_a_machine_readable_warning() { + // P5.2.4 — accepted, not refused: a monitor that only navigates still + // proves the site answers. The warning is what stops it being mistaken + // for a monitor that checks something. + let (locs, brs, devs) = allowed(); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), v2_click_step()])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + let warnings = s.warnings(); + assert_eq!(warnings.len(), 1); + assert_eq!(warnings[0].code, SyntheticWarningCode::NoAssertions); + + let with_assert = v2_synthetic(serde_json::json!([ + v2_nav_step(), + v2_click_step(), + v2_assert_step(serde_json::json!({ "kind": "element_visible" })) + ])); + assert!(with_assert.warnings().is_empty()); + } + + // ── Phase 3 — settle budget (T3-6's storage side) ─────────────────────── + + #[test] + fn test_v2_settle_budget_is_range_checked() { + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step["settle"] = serde_json::json!({ "budget_ms": 30000 }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step.clone()])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + + step["settle"] = serde_json::json!({ "budget_ms": 90000 }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("settle.budget_ms"), "{err}"); + } + + #[test] + fn test_v2_settle_navigation_and_responses_round_trip() { + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step["settle"] = serde_json::json!({ + "navigation": { "url_pattern": "**/web/**" }, + "responses": [ + { "url_pattern": "**/auth/login", "method": "POST", "required": false } + ], + "observed_duration_ms": 1800, + "budget_ms": 30000 + }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_v2_element_action_requires_a_locator() { + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step.as_object_mut().unwrap().remove("locator"); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("locator"), "{err}"); + } + + #[test] + fn test_v2_user_override_satisfies_the_locator_requirement() { + let (locs, brs, devs) = allowed(); + let step = serde_json::json!({ + "id": "s2", + "action": "click", + "name": "Sign In", + "locator": { + "candidates": [], + "user_override": { "kind": "css", "value": "#pinned" } + } + }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_v2_caps_locator_candidates_at_five() { + let (locs, brs, devs) = allowed(); + let candidates: Vec<_> = (0..6) + .map(|i| serde_json::json!({ "kind": "css", "value": format!("#c{i}") })) + .collect(); + let mut step = v2_click_step(); + step["locator"]["candidates"] = serde_json::json!(candidates); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("candidates"), "{err}"); + } + + #[test] + fn test_v2_caps_settle_response_patterns_at_five() { + let (locs, brs, devs) = allowed(); + let responses: Vec<_> = (0..6) + .map(|i| serde_json::json!({ "url_pattern": format!("**/api/{i}") })) + .collect(); + let mut step = v2_click_step(); + step["settle"] = serde_json::json!({ "responses": responses }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("responses"), "{err}"); + } + + #[test] + fn test_v2_accepts_a_full_settle_block() { + // Defined in Phase 2, populated by Phases 3 and 4. It must validate now + // so a newer recorder is never refused by an older server (spec V-2). + let (locs, brs, devs) = allowed(); + let mut step = v2_click_step(); + step["settle"] = serde_json::json!({ + "navigation": { "url_pattern": "**/web/**" }, + "responses": [ { "url_pattern": "**/auth/login", "method": "POST", "required": false } ], + "observed_duration_ms": 2300 + }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_v2_accepts_assertion_and_flow_control_blocks() { + let (locs, brs, devs) = allowed(); + let step = serde_json::json!({ + "id": "s3", + "action": "assert", + "name": "Profile visible", + "locator": { "candidates": [ { "kind": "test_attribute", "value": "[data-test=\"p\"]" } ] }, + "assertion": { "kind": "element_visible" }, + "optional": true, + "always_run": false + }); + let s = v2_synthetic(serde_json::json!([v2_nav_step(), step])); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_v2_first_step_must_be_navigate() { + let (locs, brs, devs) = allowed(); + let s = v2_synthetic(serde_json::json!([v2_click_step()])); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("navigate"), "{err}"); + } + + #[test] + fn test_v2_raises_the_steps_size_cap() { + // v2 steps are heavier (up to 5 candidates + settle patterns each), so + // the cap moves 100_000 -> 262_144. A payload between the two must be + // rejected as v1 and accepted as v2. + let (locs, brs, devs) = allowed(); + let filler = "x".repeat(150_000); + let big_steps = serde_json::json!([ + v2_nav_step(), + { + "id": "s2", + "action": "click", + "name": filler, + "locator": { "candidates": [ { "kind": "css", "value": "#a" } ] } + } + ]); + let v2 = v2_synthetic(big_steps.clone()); + assert!( + v2.validate(&locs, &brs, &devs, true).is_ok(), + "v2 should allow ~150KB" + ); + + let mut v1 = valid_browser_synthetic(); + v1.config["steps"] = serde_json::json!([ + { "id": "s1", "action": "navigate", "url": "https://example.com" }, + { "id": "s2", "action": "click", "selector": "#a", "name": "x".repeat(150_000) } + ]); + let err = v1.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("too large"), "{err}"); + } + + // ── Journey budget vs. job lease (spec P1.5.4 / T1-5) ──────────────────── + // Raising per-step timeouts and defaulting retries to 1 takes the worst-case + // run to ~1085s against a 300s job lease. An overrunning job has its lease + // expire, is requeued by the reaper, and RUNS AGAIN — duplicate results, + // multiplied browser cost, and a new class of false alert caused by the fix. + // The invariant that keeps a full retry sequence inside its lease: + // (retries + 1) * journey_budget_ms + retries * wait_before_retry_secs * 1000 + // <= LEASE_SECS * 1000 + + #[test] + fn test_browser_journey_budget_within_lease_ok() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 1; + s.wait_before_retry_secs = 5; + s.config["journey_budget_ms"] = serde_json::json!(300_000); + // 2 * 300s + 5s = 605s, inside the 900s lease. + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_browser_journey_budget_exceeding_lease_rejected() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 3; + s.wait_before_retry_secs = 5; + s.config["journey_budget_ms"] = serde_json::json!(300_000); + // 4 * 300s + 15s = 1215s — well past the 900s lease. + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + // The error must name all three inputs, or an operator cannot tell which + // one to change. + assert!(err.contains("journey_budget_ms"), "{err}"); + assert!(err.contains("retries"), "{err}"); + assert!(err.contains("lease"), "{err}"); + } + + // The invariant above was per-DEVICE while the work is per-JOB: the probe runs + // `browser_devices` sequentially inside the leased job, so the real worst case + // is multiplied by the combo count. Missing that, a perfectly ordinary + // desktop+mobile config passed validation and then blew its lease on EVERY + // run — which is exactly the duplicate-execution failure the lease was raised + // to 900s to prevent. + #[test] + fn test_browser_lease_budget_is_multiplied_by_device_count() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 1; + s.wait_before_retry_secs = 5; + s.config["journey_budget_ms"] = serde_json::json!(300_000); + + // One device: 1 * (2 * 300s + 5s) = 605s, inside the 900s lease. + s.config["browser_devices"] = + serde_json::json!([{"browser": "chromium", "device": "desktop"}]); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + + // Two devices: 2 * 605s = 1210s. Same per-device budget, twice the work. + s.config["browser_devices"] = serde_json::json!([ + {"browser": "chromium", "device": "desktop"}, + {"browser": "chromium", "device": "mobile"}, + ]); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + // Every lever the operator can pull must be named, device count included — + // it is the one they are most likely to be able to give up. + assert!(err.contains("browser_devices"), "{err}"); + assert!(err.contains("journey_budget_ms"), "{err}"); + assert!(err.contains("retries"), "{err}"); + assert!(err.contains("lease"), "{err}"); + } + + #[test] + fn test_browser_two_devices_fit_with_a_smaller_budget() { + // The rejection above must be escapable by lowering the budget, not only + // by dropping a device. + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 1; + s.wait_before_retry_secs = 5; + // 2 devices * (2 * 200s + 5s) = 810s, inside 900s. + s.config["journey_budget_ms"] = serde_json::json!(200_000); + s.config["browser_devices"] = serde_json::json!([ + {"browser": "chromium", "device": "desktop"}, + {"browser": "chromium", "device": "mobile"}, + ]); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_browser_journey_budget_boundary_is_inclusive() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 1; + s.wait_before_retry_secs = 0; + // Exactly 2 * 450s = 900s — equal to the lease, which is permitted. + s.config["journey_budget_ms"] = serde_json::json!(450_000); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + + // One millisecond more per attempt tips it over. + s.config["journey_budget_ms"] = serde_json::json!(450_001); + assert!(s.validate(&locs, &brs, &devs, true).is_err()); + } + + #[test] + fn test_browser_journey_budget_defaults_when_absent() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 1; + s.wait_before_retry_secs = 5; + // No journey_budget_ms in config — the 300s default applies and fits. + assert!(s.config.get("journey_budget_ms").is_none()); + assert!(s.validate(&locs, &brs, &devs, true).is_ok()); + } + + #[test] + fn test_browser_journey_budget_bounds() { + let (locs, brs, devs) = allowed(); + let mut s = valid_browser_synthetic(); + s.retries = 0; + s.config["journey_budget_ms"] = serde_json::json!(500); + let err = s.validate(&locs, &brs, &devs, true).unwrap_err(); + assert!(err.contains("journey_budget_ms"), "{err}"); + } + #[test] fn test_validate_empty_location_registry_rejected() { // An empty registry must reject — jobs for such a check would land diff --git a/src/config/src/metrics.rs b/src/config/src/metrics.rs index ee95dc562f..da8a7b3522 100644 --- a/src/config/src/metrics.rs +++ b/src/config/src/metrics.rs @@ -163,6 +163,46 @@ pub static INGEST_PARQUET_FILES: Lazy = Lazy::new(|| { ) .expect("Metric created") }); +/// Checks waiting to be leased, per location and pool. +/// +/// The other half of queue-lag visibility. A result record carries `scheduled_ts` +/// and `started_ts`, so the delay of work that RAN is already derivable — but a +/// check nobody leased produces no record at all, so the backlog it sits in is +/// invisible from the results side by construction. Without this, "the queue is +/// backed up" and "concurrency fixed it" are both unfalsifiable. +pub static SYNTHETICS_PENDING_JOBS: Lazy = Lazy::new(|| { + IntGaugeVec::new( + Opts::new( + "synthetics_pending_jobs", + "Number of synthetics checks pending lease.".to_owned() + HELP_SUFFIX, + ) + .namespace(NAMESPACE) + .const_labels(create_const_labels()), + &["location", "pool"], + ) + .expect("Metric created") +}); + +/// Age of the oldest check still waiting, in seconds, per location and pool. +/// +/// Reported alongside the count because they fail differently: a large count that +/// drains every tick is throughput, while a small count whose oldest entry keeps +/// ageing is a location that has stopped being served at all — one agent down, or +/// no agent ever polling that pool. The count alone cannot tell those apart. +pub static SYNTHETICS_OLDEST_PENDING_AGE_SECONDS: Lazy = Lazy::new(|| { + IntGaugeVec::new( + Opts::new( + "synthetics_oldest_pending_age_seconds", + "Age of the oldest synthetics check pending lease, in seconds.".to_owned() + + HELP_SUFFIX, + ) + .namespace(NAMESPACE) + .const_labels(create_const_labels()), + &["location", "pool"], + ) + .expect("Metric created") +}); + pub static INGEST_PACK_FILES: Lazy = Lazy::new(|| { IntGaugeVec::new( Opts::new( @@ -2050,6 +2090,12 @@ fn register_metrics(registry: &Registry) { registry .register(Box::new(INGEST_PARQUET_FILES.clone())) .expect("Metric registered"); + registry + .register(Box::new(SYNTHETICS_PENDING_JOBS.clone())) + .expect("Metric registered"); + registry + .register(Box::new(SYNTHETICS_OLDEST_PENDING_AGE_SECONDS.clone())) + .expect("Metric registered"); registry .register(Box::new(INGEST_PACK_FILES.clone())) .expect("Metric registered"); @@ -2650,6 +2696,8 @@ mod tests { let _ = INGEST_ERRORS.clone(); let _ = INGEST_WAL_USED_BYTES.clone(); let _ = INGEST_PARQUET_FILES.clone(); + let _ = SYNTHETICS_PENDING_JOBS.clone(); + let _ = SYNTHETICS_OLDEST_PENDING_AGE_SECONDS.clone(); let _ = INGEST_PACK_FILES.clone(); let _ = INGEST_PACK_SEGMENTS.clone(); let _ = INGEST_WAL_WRITE_BYTES.clone(); diff --git a/src/core/src/synthetics.rs b/src/core/src/synthetics.rs index ed92144c8b..d3c1ff8f75 100644 --- a/src/core/src/synthetics.rs +++ b/src/core/src/synthetics.rs @@ -35,6 +35,29 @@ pub struct CheckNotification { pub job_count: i64, pub error: Option, pub checked_at: i64, + /// This message closes an incident rather than opening one. + /// + /// Mandatory once `cooldown_mins` exists: with a cooldown, silence no + /// longer means "recovered", it means "possibly still broken and inside + /// the window". A recovery message is the only thing that closes it. + pub recovery: bool, + /// How many runs in a row had failed when this fired. 0 on a recovery. + pub consecutive_failures: i32, + /// The run recovered by retrying. Informational, not an incident. + pub flaky: bool, + /// The target is reachable but degrading — a certificate inside its warning + /// window, or a failing SFTP probe on a host that authenticated. + /// + /// Kept distinct from `flaky` because they arrive as the same `warning` + /// status: a flaky run fixed itself and needs no action, a degrading one + /// will not fix itself and needs action before it becomes an outage. + pub degraded: bool, + /// Locations that did not pass, worst first. + /// + /// Previously absent entirely, so a six-location check with one broken + /// region could only say "the check is failing" and the reader had to open + /// the UI to find out where. + pub failing_locations: Vec, } /// Fires once per run (when all jobs have completed) for non-passing runs. @@ -44,7 +67,13 @@ pub struct CheckNotification { /// HTTP webhooks, an HTML card for email, plain text for SNS. #[cfg(feature = "enterprise")] pub async fn notify_check_result(n: CheckNotification) { - if n.status == "passed" || n.status == "up" { + // A passing run is worth sending exactly once: when it ends an incident + // somebody was already told about. Otherwise it is a confirmation nobody + // asked for. + // + // A flaky run reports as `warning`, not `passed`, so it is not caught here — + // the decision to send it was already made upstream against `cooldown_mins`. + if !n.recovery && (n.status == "passed" || n.status == "up") { return; } @@ -66,12 +95,25 @@ pub async fn notify_check_result(n: CheckNotification) { DestinationType::Http(_) => build_slack_json(&n), }; - let subject = format!( - "[OpenObserve Synthetics] {} {} is {}", - status_emoji(&n.status), - n.monitor_name, - n.status.to_uppercase() - ); + let subject = if n.recovery { + format!( + "[OpenObserve Synthetics] ✅ {} has RECOVERED", + n.monitor_name + ) + } else if n.degraded { + // Not "is WARNING": the point of the message is that this + // needs action before it becomes an outage. + format!("[OpenObserve Synthetics] 🟡 {} is DEGRADED", n.monitor_name) + } else if n.flaky { + format!("[OpenObserve Synthetics] 🔁 {} is FLAKY", n.monitor_name) + } else { + format!( + "[OpenObserve Synthetics] {} {} is {}", + status_emoji(&n.status), + n.monitor_name, + n.status.to_uppercase() + ) + }; if let Err(e) = crate::alerts::alert::dispatch_notification(destination_type, &subject, msg) .await @@ -92,6 +134,7 @@ pub async fn notify_check_result(n: CheckNotification) { #[cfg(feature = "enterprise")] fn status_emoji(status: &str) -> &'static str { match status { + "recovered" => "✅", "failed" | "down" => "🔴", "warning" => "🟡", "error" => "⚠️", @@ -102,6 +145,23 @@ fn status_emoji(status: &str) -> &'static str { /// What the status means, in operator language — differs per status. #[cfg(feature = "enterprise")] fn status_headline(n: &CheckNotification) -> String { + if n.recovery { + return format!("{} has recovered", n.monitor_name); + } + // `warning` covers two unrelated things, and they need opposite responses: + // a flaky run already fixed itself, a degrading target will not. + if n.degraded { + return format!( + "{} is reachable but degrading — this needs attention before it fails", + n.monitor_name + ); + } + if n.flaky { + return format!( + "{} passed only after retries (flaky) — it recovered on its own", + n.monitor_name + ); + } match n.status.as_str() { "warning" => format!("{} passed only after retries (flaky)", n.monitor_name), "error" => format!( @@ -140,6 +200,25 @@ fn run_url(n: &CheckNotification) -> String { ) } +#[cfg(feature = "enterprise")] +/// "2 of 6: mumbai, frankfurt" — or just the count when we could not attribute. +/// +/// Answers "which region is broken", which the message previously could not: it +/// carried only how many locations were checked, so a one-of-six failure and a +/// six-of-six outage read identically. +fn locations_line(n: &CheckNotification) -> String { + let total = if n.job_count > 0 { n.job_count } else { 1 }; + if n.failing_locations.is_empty() { + return total.to_string(); + } + format!( + "{} of {}: {}", + n.failing_locations.len(), + total, + n.failing_locations.join(", ") + ) +} + /// Slack-compatible webhook payload (also renders fine in Teams/Discord-style /// webhooks that accept a `text` field). #[cfg(feature = "enterprise")] @@ -150,10 +229,7 @@ fn build_slack_json(n: &CheckNotification) -> String { String::new(), format!("*Monitor:* {} ({})", n.monitor_name, n.monitor_type), format!("*Target:* {}", n.target), - format!( - "*Locations checked:* {}", - if n.job_count > 0 { n.job_count } else { 1 } - ), + format!("*Locations:* {}", locations_line(n)), ]; if let Some(e) = n.error.as_deref().filter(|e| !e.is_empty()) { lines.push(format!("*Error:* ```{e}```")); @@ -174,10 +250,7 @@ fn build_plain_text(n: &CheckNotification) -> String { format!("Monitor: {} ({})", n.monitor_name, n.monitor_type), format!("Target: {}", n.target), format!("Status: {}", n.status), - format!( - "Locations checked: {}", - if n.job_count > 0 { n.job_count } else { 1 } - ), + format!("Locations: {}", locations_line(n)), ]; if let Some(e) = n.error.as_deref().filter(|e| !e.is_empty()) { lines.push(format!("Error: {e}")); @@ -191,10 +264,14 @@ fn build_plain_text(n: &CheckNotification) -> String { #[cfg(feature = "enterprise")] fn build_email_html(n: &CheckNotification) -> String { // TODO: update with a better template for all the checks - let color = match n.status.as_str() { - "warning" => "#b58105", - "error" => "#b45309", - _ => "#c62828", + let color = if n.recovery { + "#2e7d32" + } else { + match n.status.as_str() { + "warning" => "#b58105", + "error" => "#b45309", + _ => "#c62828", + } }; let error_row = match n.error.as_deref().filter(|e| !e.is_empty()) { Some(e) => format!( @@ -214,7 +291,7 @@ fn build_email_html(n: &CheckNotification) -> String { {target} Status {status} - Locations checked + Locations {jobs} Time {checked_at} @@ -230,7 +307,7 @@ fn build_email_html(n: &CheckNotification) -> String { mtype = html_escape(&n.monitor_type), target = html_escape(&n.target), status = n.status.to_uppercase(), - jobs = if n.job_count > 0 { n.job_count } else { 1 }, + jobs = html_escape(&locations_line(n)), checked_at = checked_at_utc(n.checked_at), url = run_url(n), ) diff --git a/src/infra/src/table/entity/synthetics_monitors.rs b/src/infra/src/table/entity/synthetics_monitors.rs index 30c6bc7436..a1d01d77fa 100644 --- a/src/infra/src/table/entity/synthetics_monitors.rs +++ b/src/infra/src/table/entity/synthetics_monitors.rs @@ -33,6 +33,21 @@ pub struct Model { /// Denormalised status from the most recent completed check. /// 0=Unknown, 1=Up, 2=Warning, 3=Down pub last_check_status: i32, + /// Runs that have failed back to back; reset to 0 by a pass. Compared + /// against `settings.alert_if_fails` to decide whether to notify. + pub consecutive_failures: i32, + /// When a notification was last sent, in microseconds. 0 = never. + /// Compared against `settings.cooldown_mins`. + pub last_alert_at: i64, + /// Whether the check is currently in the alerting state. Without it, + /// "recovered" cannot be told from "was never alerting" — and once a + /// cooldown exists, silence stops meaning recovery. + pub alerting: bool, + /// When a degradation was last reported, in microseconds. 0 = not currently + /// degraded. Separate from `last_alert_at` because a degradation persists — + /// a certificate is `warning` on every run for weeks — so it needs + /// transition-based suppression rather than a time window. + pub degraded_notified_at: i64, pub owner: Option, pub created_at: i64, pub updated_at: i64, @@ -69,6 +84,10 @@ mod tests { next_run_at: 0, last_triggered_at: 0, last_check_status: 0, + consecutive_failures: 0, + last_alert_at: 0, + alerting: false, + degraded_notified_at: 0, owner: None, created_at: 1750000000000000, updated_at: 1750000000000000, diff --git a/src/infra/src/table/migration/m20260730_000001_add_alert_state_to_synthetics_monitors.rs b/src/infra/src/table/migration/m20260730_000001_add_alert_state_to_synthetics_monitors.rs new file mode 100644 index 0000000000..ef4fd264ad --- /dev/null +++ b/src/infra/src/table/migration/m20260730_000001_add_alert_state_to_synthetics_monitors.rs @@ -0,0 +1,168 @@ +// Copyright 2026 OpenObserve Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +//! Add the alert state that `alert_if_fails` and `cooldown_mins` need in order +//! to mean anything. +//! +//! Both settings are validated on save, stored, and delivered to the probe — +//! and were read by nothing. Every completed run with a destination notified, +//! so `alert_if_fails: 3` alerted on the first failure and a `cooldown_mins` of +//! 30 sent thirty notifications in thirty minutes. +//! +//! Answering "is this the third consecutive failure?" and "have thirty minutes +//! passed since I last spoke?" needs memory BETWEEN runs; a single ack knows +//! only its own outcome. Three columns, all defaulted so existing rows need no +//! backfill: +//! - `consecutive_failures` — runs that failed back to back, reset by a pass. Compared against +//! `alert_if_fails`. +//! - `last_alert_at` — when a notification was last sent, in microseconds. Compared against +//! `cooldown_mins`. 0 = never. +//! - `alerting` — whether the check is currently in the alerting state. This is what makes a +//! recovery notification possible: without it "recovered" cannot be told from "was never +//! alerting", and once a cooldown exists, silence stops meaning recovery. +//! - `degraded_notified_at` — when a degradation was last reported, in microseconds. 0 = not +//! currently degraded. Degradation needs TRANSITION-based suppression, not time-based: a +//! certificate inside a 30-day warning window is `warning` on every single run, so notifying +//! per run is unusable while treating it as healthy means it never notifies at all until the +//! certificate actually expires — after the outage it existed to prevent. + +use sea_orm_migration::prelude::*; + +#[derive(DeriveMigrationName)] +pub struct Migration; + +#[async_trait::async_trait] +impl MigrationTrait for Migration { + async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { + // `synthetics` is created from the entity definition, so on a fresh + // database these columns already exist by the time this runs. SQLite + // ignores the `IF NOT EXISTS` guard on `ADD COLUMN`, so guard + // explicitly with `has_column` to stay idempotent across backends. + if !manager.has_column(TABLE, "consecutive_failures").await? { + manager + .alter_table( + Table::alter() + .table(Synthetics::Table) + .add_column_if_not_exists( + ColumnDef::new(Synthetics::ConsecutiveFailures) + .integer() + .not_null() + .default(0), + ) + .to_owned(), + ) + .await?; + } + if !manager.has_column(TABLE, "last_alert_at").await? { + manager + .alter_table( + Table::alter() + .table(Synthetics::Table) + .add_column_if_not_exists( + ColumnDef::new(Synthetics::LastAlertAt) + .big_integer() + .not_null() + .default(0), + ) + .to_owned(), + ) + .await?; + } + if !manager.has_column(TABLE, "alerting").await? { + manager + .alter_table( + Table::alter() + .table(Synthetics::Table) + .add_column_if_not_exists( + ColumnDef::new(Synthetics::Alerting) + .boolean() + .not_null() + .default(false), + ) + .to_owned(), + ) + .await?; + } + if !manager.has_column(TABLE, "degraded_notified_at").await? { + manager + .alter_table( + Table::alter() + .table(Synthetics::Table) + .add_column_if_not_exists( + ColumnDef::new(Synthetics::DegradedNotifiedAt) + .big_integer() + .not_null() + .default(0), + ) + .to_owned(), + ) + .await?; + } + Ok(()) + } + + async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { + // sea-query has no `drop_column_if_exists`, so the same guard applies in + // reverse — a partially-applied `up` must still roll back cleanly. + for (name, column) in [ + ("consecutive_failures", Synthetics::ConsecutiveFailures), + ("last_alert_at", Synthetics::LastAlertAt), + ("alerting", Synthetics::Alerting), + ("degraded_notified_at", Synthetics::DegradedNotifiedAt), + ] { + if manager.has_column(TABLE, name).await? { + manager + .alter_table( + Table::alter() + .table(Synthetics::Table) + .drop_column(column) + .to_owned(), + ) + .await?; + } + } + Ok(()) + } +} + +const TABLE: &str = "synthetics"; + +// The table is `synthetics`, NOT `synthetics_monitors` — the entity module is +// named after the concept but declares `table_name = "synthetics"`. DeriveIden +// renders the variant name, so this enum must match the create migration's +// (m20260707_000001), or the ALTER hits a table that does not exist. +#[derive(DeriveIden)] +enum Synthetics { + Table, + ConsecutiveFailures, + LastAlertAt, + Alerting, + DegradedNotifiedAt, +} + +#[cfg(test)] +mod tests { + use sea_orm_migration::MigrationName; + + use super::*; + + #[test] + fn test_migration_name() { + assert_eq!( + Migration.name(), + "m20260730_000001_add_alert_state_to_synthetics_monitors" + ); + } +} diff --git a/src/infra/src/table/migration/mod.rs b/src/infra/src/table/migration/mod.rs index 0895738945..0395dc6570 100644 --- a/src/infra/src/table/migration/mod.rs +++ b/src/infra/src/table/migration/mod.rs @@ -142,6 +142,7 @@ mod m20260723_000001_add_env_version_to_gen_ai_agents; mod m20260724_000001_add_name_is_default_to_synthetics_probe_tokens; mod m20260724_000002_add_token_id_to_synthetics_agents; mod m20260728_000001_create_workflows_associations_table; +mod m20260730_000001_add_alert_state_to_synthetics_monitors; pub struct Migrator; @@ -273,6 +274,7 @@ impl MigratorTrait for Migrator { Box::new(m20260724_000001_add_name_is_default_to_synthetics_probe_tokens::Migration), Box::new(m20260724_000002_add_token_id_to_synthetics_agents::Migration), Box::new(m20260728_000001_create_workflows_associations_table::Migration), + Box::new(m20260730_000001_add_alert_state_to_synthetics_monitors::Migration), ] } } diff --git a/src/infra/src/table/synthetics_jobs.rs b/src/infra/src/table/synthetics_jobs.rs index f6e89dd0e9..716e049c24 100644 --- a/src/infra/src/table/synthetics_jobs.rs +++ b/src/infra/src/table/synthetics_jobs.rs @@ -79,7 +79,8 @@ pub struct LeasedRow { pub metadata: String, } -/// Returned by `dead_letter_expired` for each job that exhausted all retries. +/// Returned by `dead_letter_expired` for each job it terminated. The caller owns +/// completing the job's run — see `reason` for which of the three ways it ended. #[derive(Debug)] pub struct DeadLetteredRow { pub id: String, @@ -90,6 +91,47 @@ pub struct DeadLetteredRow { pub dispatch_attempts: i32, pub run_id: String, pub metadata: String, + pub reason: DeadLetterReason, +} + +/// Why a job was terminated without a probe result. +/// +/// All three end the same way — the run is completed with Error — but they are +/// different operational problems, and collapsing them into one message costs +/// the reader the only clue about which one they have. `NeverDispatched` means +/// no probe polled that location at all (a dead private agent); the other two +/// mean a probe took the job and went away. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DeadLetterReason { + /// A probe held the lease, never acked, and has no dispatch attempts left. + AttemptsExhausted, + /// The lease expired and the job is now past `valid_until`. Requeueing it + /// would produce a result stamped for a window that has already closed, so + /// the next scheduled run is the right retry, not this one. + Expired, + /// Never claimed by anything before `valid_until` passed. + NeverDispatched, +} + +/// Classifies a row selected by `dead_letter_expired`. Split out from the query +/// so the mapping is testable without a database. +/// +/// `status` is on the `synthetics_jobs` scale (0=Pending, 1=Leased). +pub fn dead_letter_reason( + status: i32, + dispatch_attempts: i32, + max_attempts: i32, +) -> DeadLetterReason { + if status == 0 { + // Pending and past its window: nothing ever leased it, so the attempt + // count says nothing about why. A leased row with 0 attempts would be a + // different story, which is why status is checked first. + DeadLetterReason::NeverDispatched + } else if dispatch_attempts >= max_attempts { + DeadLetterReason::AttemptsExhausted + } else { + DeadLetterReason::Expired + } } // ── Scheduler: enqueue ──────────────────────────────────────────────────────── @@ -190,12 +232,29 @@ pub async fn drain_monitor( Ok(res.rows_affected()) } +/// Hard ceiling on one lease call, whatever the caller asks for. +/// +/// Well above every real caller — the dispatcher takes 10, a Go agent +/// `min(4 x NumCPU, 16)`, a browser agent 1 — so it never binds in normal +/// operation. It exists so that no single probe can drain the queue, by +/// misconfiguration or otherwise. +const MAX_LEASE_BATCH: i64 = 100; + // ── Dispatcher: lease ───────────────────────────────────────────────────────── /// Leases up to `limit` pending checks from a pool. /// /// Three steps: SELECT candidate IDs → UPDATE status/lease fields → SELECT /// full rows. No raw SQL, works on SQLite and Postgres. +/// +/// `lease_secs` is a floor request, not the decision. Both probes hardcode 300s +/// client-side while a check's retry sequence — which runs *inside* the leased +/// job — is bounded only by `JOB_LEASE_SECS`, so an under-lease means the lease +/// expires while the probe is still working: the reaper terminates the job, +/// completes the run as an error, and the probe's real result is then rejected as +/// a stale ack. A client cannot be trusted to know how long its job may take, so +/// the server raises any request up to the budget the config validation already +/// guarantees every check fits inside. pub async fn lease_batch( conn: &C, pool: &str, @@ -205,8 +264,16 @@ pub async fn lease_batch( lease_secs: i64, browser: Option, ) -> Result, errors::Error> { + let lease_secs = lease_secs.max(config::meta::synthetics::JOB_LEASE_SECS); let lease_expires_at = now_us + lease_secs * 1_000_000; + // `limit` arrives from a client and is cast to u64 below, where a negative + // wraps to ~1.8e19 and the query becomes unbounded — one agent would lease the + // entire pending queue for its pool and then hold a lease on every row of it. + // Reachable from a single mistyped env var, so it is clamped here rather than + // trusted: a probe does not get to decide how much of the queue it may take. + let limit = limit.clamp(1, MAX_LEASE_BATCH); + // Step 1: pick candidate IDs. // // `browser` narrows the candidate set to jobs the caller can actually run, @@ -298,31 +365,90 @@ pub async fn lease_batch( /// Returns the `run_id` of the updated row (for callers to increment run counter). /// /// Status values: 3=Passed, 4=Failed, 5=Warning, 6=Error (dispatch error). +/// +/// Returns `Some(run_id)` when the ack applied, and **`None` when it did not** — +/// the job does not exist, it was no longer Claimed, or it is now held by someone +/// else. The caller must treat `None` as "do not touch run accounting". +/// +/// `claimed_by` is the acking probe's agent id. `None` means the probe did not +/// send one, which is the **compatibility window**: a probe built before this +/// change acks without it, and rejecting those would drop every result until both +/// probes are redeployed. Such an ack falls back to the status guard alone, i.e. +/// exactly the behaviour before this change — so an old probe is no worse off, +/// and a new one gets the ownership guard immediately. The window closes by +/// making this argument non-optional once both probes have shipped. pub async fn ack_complete( conn: &C, job_id: &str, status: i32, result_json: Option<&str>, now_us: i64, + claimed_by: Option<&str>, ) -> Result, errors::Error> { - let sql = r#" - UPDATE synthetics_jobs - SET status = $1, result = $2, completed_at = $3 - WHERE id = $4 - "#; - conn.execute(Statement::from_sql_and_values( - conn.get_database_backend(), - sql, - [ - Value::from(status), - result_json - .map(Value::from) - .unwrap_or(Value::from(None::)), - Value::from(now_us), - Value::from(job_id.to_owned()), - ], - )) - .await?; + // `AND status = 1` makes the ack idempotent: only a job still in the Claimed + // state can be completed. Without it a duplicate ack succeeded, the caller + // called `increment_jobs_done` a second time for one job, `jobs_done` + // overshot `job_count`, and the run was declared complete on a PARTIAL set of + // results — the roll-up status computed from whichever jobs happened to have + // finished. + // + // Duplicate acks are reachable today without any concurrency work: the + // aws-sdk-lambda client retries a timed-out `RequestResponse` invoke, which + // re-executes a check that is already running. + // + // The status guard alone does NOT cover reaper reassignment: the reaper puts + // the row back to Pending, another agent leases it, and the row is at status 1 + // again — so the EVICTED holder's late ack still matches and overwrites the + // new holder's result, or lands before it and gets overwritten. Either way one + // job produced two results and `increment_jobs_done` ran twice for it. + // + // `claimed_by` closes that: only the agent the row is currently leased to can + // complete it. The lease already stamps `claimed_by`, so this compares against + // what the server itself recorded rather than anything the probe asserts. + let (owner_clause, values) = match claimed_by { + Some(agent) => ( + " AND claimed_by = $5", + vec![ + Value::from(status), + result_json + .map(Value::from) + .unwrap_or(Value::from(None::)), + Value::from(now_us), + Value::from(job_id.to_owned()), + Value::from(agent.to_owned()), + ], + ), + None => ( + "", + vec![ + Value::from(status), + result_json + .map(Value::from) + .unwrap_or(Value::from(None::)), + Value::from(now_us), + Value::from(job_id.to_owned()), + ], + ), + }; + let sql = format!( + "UPDATE synthetics_jobs SET status = $1, result = $2, completed_at = $3 \ + WHERE id = $4 AND status = 1{owner_clause}" + ); + let applied = conn + .execute(Statement::from_sql_and_values( + conn.get_database_backend(), + &sql, + values, + )) + .await?; + + // `None` means "this ack did not apply" — the caller must NOT increment the + // run counter. Returning the run_id anyway is what made the guard above + // pointless in an earlier draft: the SELECT below succeeds regardless of + // whether the UPDATE matched. + if applied.rows_affected() == 0 { + return Ok(None); + } // Fetch run_id so caller can call synthetics_runs::increment_jobs_done. let select_sql = "SELECT run_id FROM synthetics_jobs WHERE id = $1"; @@ -342,7 +468,16 @@ pub async fn ack_complete( // ── Reaper ──────────────────────────────────────────────────────────────────── -/// Resets expired leases back to Pending (status=0) when dispatch_attempts < max_attempts. +/// Resets expired leases back to Pending (status=0) when the job has dispatch +/// attempts left *and* is still inside its validity window. +/// +/// The `valid_until` guard is what makes the retry budget real. Without it this +/// requeues a job whose window has already closed, and `prune_stale` — which +/// runs later in the same reaper tick — deletes it again immediately, because +/// `valid_until` is one interval (60s for a 1-minute check) while a lease is +/// 300s or more. So the row went Pending → deleted within one tick, +/// `dispatch_attempts` never got past 1, `max_attempts` was unreachable, and +/// the run it belonged to was never completed by anyone. pub async fn requeue_expired( conn: &C, now_us: i64, @@ -353,6 +488,7 @@ pub async fn requeue_expired( SET status = 0, claimed_by = NULL, claimed_at = NULL, lease_expires_at = NULL WHERE status = 1 AND lease_expires_at < $1 + AND valid_until >= $1 AND dispatch_attempts < $2 "#; let res = conn @@ -365,20 +501,38 @@ pub async fn requeue_expired( Ok(res.rows_affected()) } -/// Marks permanently failed rows as Dead (status=2) when dispatch_attempts >= max_attempts. -/// Returns the affected rows so the reaper can update monitor status and write to streams. +/// Marks every job that can no longer produce a probe result as Dead (status=2) +/// and returns the rows the caller must now account for. +/// +/// Covers all three terminal shapes, because each one leaves a run that nobody +/// else will ever finish: +/// +/// - leased, lease expired, no attempts left → `AttemptsExhausted` +/// - leased, lease expired, past `valid_until` → `Expired` (a retry would report against a closed +/// window; the next scheduled run is the right retry) +/// - pending, past `valid_until` → `NeverDispatched`, previously the business of `prune_stale`, +/// which DELETEd the row and so destroyed the only record that the run was still owed a job +/// +/// The caller **must** complete the run for every row returned. The per-row +/// compare-and-swap below is what makes that safe to do exactly once: the reaper +/// runs on every alert_manager node, and a bulk `UPDATE ... WHERE id IN (...)` +/// after a separate SELECT lets two nodes both believe they terminated the same +/// job. That was harmless while the caller only wrote to a stream; it is not +/// harmless now that the caller increments a run counter, because a double +/// increment pushes `jobs_done` past `job_count` and completes the run early, +/// with a result that is missing a location. pub async fn dead_letter_expired( conn: &C, now_us: i64, max_attempts: i32, ) -> Result, errors::Error> { - // Step 1: find candidates before marking them dead. + // Step 1: find candidates before marking them dead. `status` comes back so + // the CAS can require the row not to have moved under us. let select_sql = r#" - SELECT id, synthetics_id, synthetics_name, org_id, location, dispatch_attempts, run_id, metadata + SELECT id, synthetics_id, synthetics_name, org_id, location, dispatch_attempts, run_id, metadata, status FROM synthetics_jobs - WHERE status = 1 - AND lease_expires_at < $1 - AND dispatch_attempts >= $2 + WHERE (status = 1 AND lease_expires_at < $1 AND (dispatch_attempts >= $2 OR valid_until < $1)) + OR (status = 0 AND valid_until < $1) "#; let rows = conn .query_all(Statement::from_sql_and_values( @@ -392,40 +546,45 @@ pub async fn dead_letter_expired( return Ok(vec![]); } - let dead: Vec = rows + let candidates: Vec<(DeadLetteredRow, i32)> = rows .into_iter() .filter_map(|row| { - Some(DeadLetteredRow { - id: row.try_get::("", "id").ok()?, - synthetics_id: row.try_get("", "synthetics_id").ok()?, - synthetics_name: row.try_get("", "synthetics_name").ok()?, - org_id: row.try_get("", "org_id").ok()?, - location: row.try_get("", "location").ok()?, - dispatch_attempts: row.try_get("", "dispatch_attempts").ok()?, - run_id: row.try_get("", "run_id").ok()?, - metadata: row.try_get("", "metadata").unwrap_or_default(), - }) + let status: i32 = row.try_get("", "status").ok()?; + let dispatch_attempts: i32 = row.try_get("", "dispatch_attempts").ok()?; + let reason = dead_letter_reason(status, dispatch_attempts, max_attempts); + Some(( + DeadLetteredRow { + id: row.try_get::("", "id").ok()?, + synthetics_id: row.try_get("", "synthetics_id").ok()?, + synthetics_name: row.try_get("", "synthetics_name").ok()?, + org_id: row.try_get("", "org_id").ok()?, + location: row.try_get("", "location").ok()?, + dispatch_attempts, + run_id: row.try_get("", "run_id").ok()?, + metadata: row.try_get("", "metadata").unwrap_or_default(), + reason, + }, + status, + )) }) .collect(); - // Step 2: mark them all dead. - let ids: Vec = dead.iter().map(|r| Value::from(r.id.clone())).collect(); - let placeholders: String = ids - .iter() - .enumerate() - .map(|(i, _)| format!("${}", i + 1)) - .collect::>() - .join(", "); - let update_sql = format!( - "UPDATE synthetics_jobs SET status = 2 WHERE id IN ({})", - placeholders - ); - conn.execute(Statement::from_sql_and_values( - conn.get_database_backend(), - &update_sql, - ids, - )) - .await?; + // Step 2: claim each row individually. Only rows this call actually + // transitioned are returned, so only one node ever accounts for a job. + let update_sql = "UPDATE synthetics_jobs SET status = 2 WHERE id = $1 AND status = $2"; + let mut dead = Vec::with_capacity(candidates.len()); + for (row, prev_status) in candidates { + let res = conn + .execute(Statement::from_sql_and_values( + conn.get_database_backend(), + update_sql, + [Value::from(row.id.clone()), Value::from(prev_status)], + )) + .await?; + if res.rows_affected() == 1 { + dead.push(row); + } + } Ok(dead) } @@ -441,6 +600,20 @@ pub enum DispatchFailureOutcome { /// via the dispatcher's existing `mark_failure`), so writing an /// intermediate status here would just be overwritten and wastes a query. DeadLettered, + /// The job is no longer Claimed — it has already been completed by a probe + /// ack, or reassigned to another holder. **The caller must do nothing:** no + /// requeue, no failure record, no run accounting. + /// + /// Reachable whenever a dispatch is judged failed *after* the probe already + /// reported: a Lambda that acks and then panics returns `FunctionError`, and + /// the SDK can also fail reading a response for an invocation that ran fine. + /// Without this the requeue reset a finished job to Pending, it was leased + /// again, and the check ran a second time — producing a duplicate result and a + /// second `increment_jobs_done` for one job. + /// + /// Enforced by the compiler rather than a test: the dispatcher matches all three + /// variants with no wildcard arm, so adding this one made "handle it" mandatory. + AlreadySettled, } /// Called when dispatching a leased job failed *before* the probe could run @@ -458,24 +631,161 @@ pub async fn fail_dispatch( max_attempts: i32, ) -> Result { if current_attempts < max_attempts { + // `AND status = 1` is what keeps this from resurrecting finished work. A + // dispatch can be judged failed after the probe has already acked — a + // Lambda that reports and then panics is exactly that — and without the + // guard the requeue reset a COMPLETED job to Pending, so it was leased and + // run again: duplicate result, and `increment_jobs_done` twice for one job. let sql = r#" UPDATE synthetics_jobs SET status = 0, claimed_by = NULL, claimed_at = NULL, lease_expires_at = NULL - WHERE id = $1 + WHERE id = $1 AND status = 1 "#; - conn.execute(Statement::from_sql_and_values( - conn.get_database_backend(), - sql, - [Value::from(job_id.to_owned())], - )) - .await?; + let applied = conn + .execute(Statement::from_sql_and_values( + conn.get_database_backend(), + sql, + [Value::from(job_id.to_owned())], + )) + .await?; + if applied.rows_affected() == 0 { + return Ok(DispatchFailureOutcome::AlreadySettled); + } Ok(DispatchFailureOutcome::Requeued) } else { + // Same check on the terminal branch, for the same reason: the caller's + // `mark_failure` would otherwise write a dispatch error over a real result. + // `ack_complete` would refuse it, but the caller also writes to the results + // stream and increments the run, and neither of those can be taken back. + let still_claimed = Entity::find() + .select_only() + .column(Column::Id) + .filter(Column::Id.eq(job_id)) + .filter(Column::Status.eq(1i32)) + .into_tuple::() + .one(conn) + .await?; + if still_claimed.is_none() { + return Ok(DispatchFailureOutcome::AlreadySettled); + } Ok(DispatchFailureOutcome::DeadLettered) } } /// Deletes stale Pending rows whose valid_until has passed (missed entirely). +/// Locations of this run's jobs that did not pass, worst first. +/// +/// A notification for a completed run previously said only "the check is +/// failing" — `CheckNotification` had no location field at all, and +/// `AckResponse.location` is the location of whichever job happened to ack LAST, +/// which would name an arbitrary one. With six locations and one broken, the +/// message could not say which. +/// +/// Read from `synthetics_jobs` rather than aggregated on the run row because the +/// run keeps only `MAX(status)` — one integer, no per-location detail. Costs one +/// query, on run completion only, not per ack. +/// +/// Job status ints are the `synthetics_jobs` scale: 3=Passed, 4=Failed, +/// 5=Warning, 6=Error. Anything other than Passed is returned. +pub async fn failing_locations( + conn: &C, + run_id: &str, +) -> Result, errors::Error> { + let rows = Entity::find() + .select_only() + .column(Column::Location) + .column(Column::Status) + .filter(Column::RunId.eq(run_id)) + .filter(Column::Status.ne(3)) + .into_tuple::<(String, i32)>() + .all(conn) + .await?; + let mut rows = rows; + // Worst first: Error(6) > Warning(5) > Failed(4). A reader scanning the first + // line of a message should see the most severe location, not the + // alphabetically first. + rows.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0))); + let mut out: Vec = Vec::with_capacity(rows.len()); + for (loc, _) in rows { + if !out.contains(&loc) { + out.push(loc); + } + } + Ok(out) +} + +/// One location+pool's share of the pending queue. +#[derive(Debug, PartialEq, Eq)] +pub struct PendingBacklogRow { + pub location: String, + pub pool: String, + /// Checks still waiting to be leased. + pub pending: i64, + /// `scheduled_ts` of the oldest one, in microseconds. The caller turns this + /// into an age; doing it here would bake in a clock reading the caller may + /// already have. + pub oldest_scheduled_ts: i64, +} + +/// Per-location, per-pool count of checks waiting to be leased, with the oldest +/// one's scheduled time. +/// +/// Counts only rows that are still leasable — Pending and inside `valid_until`. +/// Rows past their window are excluded on purpose: they are the reaper's to +/// terminate and counting them would report a backlog that no amount of probe +/// capacity can drain, which is a different problem wearing the same number. +pub async fn pending_backlog( + conn: &C, + now_us: i64, +) -> Result, errors::Error> { + let sql = r#" + SELECT location, pool, COUNT(*) AS pending, MIN(scheduled_ts) AS oldest_scheduled_ts + FROM synthetics_jobs + WHERE status = 0 AND valid_until > $1 + GROUP BY location, pool + "#; + let rows = conn + .query_all(Statement::from_sql_and_values( + conn.get_database_backend(), + sql, + [Value::from(now_us)], + )) + .await?; + + Ok(rows + .into_iter() + .filter_map(|row| { + Some(PendingBacklogRow { + location: row.try_get("", "location").ok()?, + pool: row.try_get("", "pool").ok()?, + // SQLite hands COUNT back as i32 on some builds and i64 on others, + // so try the wider type first and fall back rather than dropping + // the row and reporting an empty backlog. + pending: row + .try_get::("", "pending") + .or_else(|_| row.try_get::("", "pending").map(i64::from)) + .unwrap_or(0), + // Falls back to `now_us`, i.e. age 0 — NOT to 0, which is the + // epoch and would publish a ~55-year-old backlog and page whoever + // is on call for a column we simply failed to decode. + oldest_scheduled_ts: row + .try_get::("", "oldest_scheduled_ts") + .or_else(|_| row.try_get::("", "oldest_scheduled_ts").map(i64::from)) + .unwrap_or(now_us), + }) + }) + .collect()) +} + +/// Backstop for pending rows past `valid_until`. +/// +/// `dead_letter_expired` runs earlier in the same reaper tick and moves these to +/// Dead after completing their runs, so in normal operation this matches nothing. +/// It stays as a floor: if the dead-letter path ever fails to claim a row, the +/// row is still removed rather than left to be re-examined every 30 seconds +/// forever. A row this deletes is a run that will not complete, so a non-zero +/// count here is a bug signal, not routine housekeeping — the reaper logs it at +/// warn for that reason. pub async fn prune_stale(conn: &C, now_us: i64) -> Result { let sql = r#" DELETE FROM synthetics_jobs @@ -538,4 +848,75 @@ mod tests { assert_eq!(row.run_id, "3Fzn001XXXXXXXXXXXXXXXX"); assert_eq!(row.dispatch_attempts, 1); } + + const MAX: i32 = 3; + + #[test] + fn a_lease_limit_is_clamped_before_it_reaches_the_query() { + // `limit` is cast to u64 in the query. A negative wraps to ~1.8e19, making + // the LIMIT unbounded — one agent leases the entire pending queue for its + // pool and holds a 900s lease on every row. Reachable from one mistyped + // env var, so the server clamps rather than trusting the client. + assert_eq!((-4i64).clamp(1, MAX_LEASE_BATCH), 1); + assert_eq!(0i64.clamp(1, MAX_LEASE_BATCH), 1); + // Real callers are far below the ceiling and pass through untouched: + // dispatcher 10, Go agent min(4 x NumCPU, 16), browser agent 1. + for asked in [1i64, 10, 16, 25] { + assert_eq!(asked.clamp(1, MAX_LEASE_BATCH), asked); + } + // And nobody drains the queue by asking louder. + assert_eq!(i64::MAX.clamp(1, MAX_LEASE_BATCH), MAX_LEASE_BATCH); + } + + #[test] + fn pending_backlog_row_carries_location_pool_and_the_oldest_entry() { + // The oldest entry is the half that distinguishes a deep queue that drains + // every tick from a location nothing is serving at all. + let row = PendingBacklogRow { + location: "private-dc1".to_string(), + pool: "private-dc1-pool".to_string(), + pending: 42, + oldest_scheduled_ts: 1_750_000_000_000_000, + }; + assert_eq!(row.pending, 42); + assert_eq!(row.oldest_scheduled_ts, 1_750_000_000_000_000); + } + + #[test] + fn pending_row_is_never_dispatched_whatever_its_attempt_count() { + // Status decides this, not the counter. A Pending row past `valid_until` + // was never leased for the window that just closed, so reporting + // "did not respond after N attempts" would blame a probe that was never + // asked. Both counts must classify the same way. + assert_eq!( + dead_letter_reason(0, 0, MAX), + DeadLetterReason::NeverDispatched + ); + assert_eq!( + dead_letter_reason(0, MAX, MAX), + DeadLetterReason::NeverDispatched + ); + } + + #[test] + fn leased_row_separates_a_spent_budget_from_a_closed_window() { + // At or over the budget: retries are genuinely exhausted. + assert_eq!( + dead_letter_reason(1, MAX, MAX), + DeadLetterReason::AttemptsExhausted + ); + assert_eq!( + dead_letter_reason(1, MAX + 1, MAX), + DeadLetterReason::AttemptsExhausted + ); + // Under the budget: attempts were left, but the window closed first, so + // this is the case `requeue_expired` deliberately declines to retry. + // Before the `valid_until` guard, this row was requeued and then deleted + // by `prune_stale` in the same tick, which is why it needs its own name. + assert_eq!(dead_letter_reason(1, 0, MAX), DeadLetterReason::Expired); + assert_eq!( + dead_letter_reason(1, MAX - 1, MAX), + DeadLetterReason::Expired + ); + } } diff --git a/src/infra/src/table/synthetics_monitors.rs b/src/infra/src/table/synthetics_monitors.rs index ed882559bd..7930a3a1a8 100644 --- a/src/infra/src/table/synthetics_monitors.rs +++ b/src/infra/src/table/synthetics_monitors.rs @@ -438,6 +438,94 @@ pub async fn update_last_check_status( Ok(()) } +/// The alert bookkeeping a completed run needs in order to decide whether to +/// notify. Read and written by the ack path only. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct AlertState { + /// Runs that failed back to back. Reset to 0 by a pass. + pub consecutive_failures: i32, + /// When a notification was last sent, in microseconds. 0 = never. + pub last_alert_at: i64, + /// Whether the check is currently in the alerting state. + pub alerting: bool, + /// When a degradation was last reported, in microseconds. 0 = not currently + /// degraded. Degradation persists for as long as the certificate takes to + /// expire, so it is suppressed by transition rather than by a time window. + pub degraded_notified_at: i64, +} + +/// Reads the alert state without pulling the whole monitor (config, secrets and +/// step definitions are several KB, and this runs on every ack). +pub async fn get_alert_state( + conn: &C, + id: &str, +) -> Result, errors::Error> { + let row = Entity::find_by_id(id) + .select_only() + .column(Column::ConsecutiveFailures) + .column(Column::LastAlertAt) + .column(Column::Alerting) + .column(Column::DegradedNotifiedAt) + .into_tuple::<(i32, i64, bool, i64)>() + .one(conn) + .await?; + Ok(row.map( + |(consecutive_failures, last_alert_at, alerting, degraded_notified_at)| AlertState { + consecutive_failures, + last_alert_at, + alerting, + degraded_notified_at, + }, + )) +} + +/// Writes the alert state back after a run completes. +/// Writes the alert state back, but only if it still holds `expected`. +/// +/// Returns `true` when the write applied, `false` when another writer got there +/// first and the caller's decision was made against a stale read. +/// +/// This is a compare-and-swap because the caller does a read-modify-write with no +/// transaction around it: `get_alert_state` then decide then write. Two runs of +/// the SAME check can complete close together — a run takes longer than the +/// interval whenever the target is slow, which is exactly when it is failing — and +/// both would read `consecutive_failures = 2`, both write 3, and both conclude +/// they were outside the cooldown. The streak would undercount and the +/// notification would double. +/// +/// The three columns are guarded, not just the counter: a lost `alerting` +/// transition is what produces a recovery for an incident nobody was told about. +/// +/// Compare `synthetics_runs::increment_jobs_done`, which is a single atomic +/// `jobs_done = jobs_done + 1`. That shape is not available here because the new +/// value depends on a policy decision, not on arithmetic over the old one. +pub async fn update_alert_state_if( + conn: &C, + id: &str, + expected: AlertState, + state: AlertState, +) -> Result { + let res = Entity::update_many() + .col_expr( + Column::ConsecutiveFailures, + Expr::value(state.consecutive_failures), + ) + .col_expr(Column::LastAlertAt, Expr::value(state.last_alert_at)) + .col_expr(Column::Alerting, Expr::value(state.alerting)) + .col_expr( + Column::DegradedNotifiedAt, + Expr::value(state.degraded_notified_at), + ) + .filter(Column::Id.eq(id)) + .filter(Column::ConsecutiveFailures.eq(expected.consecutive_failures)) + .filter(Column::LastAlertAt.eq(expected.last_alert_at)) + .filter(Column::Alerting.eq(expected.alerting)) + .filter(Column::DegradedNotifiedAt.eq(expected.degraded_notified_at)) + .exec(conn) + .await?; + Ok(res.rows_affected > 0) +} + // ── Private helpers ─────────────────────────────────────────────────────────── async fn get_model( @@ -616,6 +704,10 @@ mod tests { next_run_at: 0, last_triggered_at: 0, last_check_status: 0, + consecutive_failures: 0, + last_alert_at: 0, + alerting: false, + degraded_notified_at: 0, owner: None, created_at: 1750000000000000, updated_at: 1750000000000000, diff --git a/web/src/components/synthetics/CreateBrowserTest.schema.spec.ts b/web/src/components/synthetics/CreateBrowserTest.schema.spec.ts new file mode 100644 index 0000000000..ac76e52351 --- /dev/null +++ b/web/src/components/synthetics/CreateBrowserTest.schema.spec.ts @@ -0,0 +1,260 @@ +// Copyright 2026 OpenObserve Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +import { describe, expect, it } from "vitest"; +import { makeBrowserCheckSaveSchema } from "./CreateBrowserTest.schema"; + +const t = (key: string) => key; + +// Most journey fixtures below exercise the target and first-step rules, not names, +// so a name is supplied by default — step name is required (D10) and every fixture +// would otherwise trip that rule for reasons unrelated to what it is testing. A +// case that IS about names passes an explicit `name`, which wins over the default. +function form(journey: unknown[]) { + return { + name: "check", + url: "https://app.test", + locations: ["us-east"], + journey: journey.map((step) => ({ name: "Step", ...(step as Record) })), + }; +} + +function issuePaths(result: { success: boolean; error?: { issues: { path: PropertyKey[] }[] } }) { + return result.success ? [] : (result.error?.issues ?? []).map((i) => i.path.join(".")); +} + +describe("makeBrowserCheckSaveSchema journey validation", () => { + const schema = makeBrowserCheckSaveSchema(t); + + it("should accept a v1 journey whose steps carry selectors", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click", selector: "#login" }, + ]), + ); + + expect(result.success).toBe(true); + }); + + // Regression: a check saved as steps_version 2 comes back with no `selector` + // at all — the element lives in `locator`. Validating on `selector` alone made + // every v2 check unsaveable the moment it was reopened for editing. + it("should accept a v2 journey whose steps identify the element by locator", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { + id: "2", + action: "click", + name: 'Click on internal:testid=[data-test="login-as-internal-user"]', + locator: { + candidates: [ + { + kind: "test_attribute", + value: 'internal:testid=[data-test="login-as-internal-user"]', + }, + ], + user_override: null, + }, + }, + ]), + ); + + expect(issuePaths(result)).toEqual([]); + expect(result.success).toBe(true); + }); + + it("should accept a v2 step whose only target is a pinned override", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { + id: "2", + action: "click", + locator: { candidates: [], user_override: { kind: "css", value: "#login" } }, + }, + ]), + ); + + expect(result.success).toBe(true); + }); + + it("should accept a page-level assertion, which targets no element", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "assert", assertion: { kind: "url_matches", expected: "/home" } }, + ]), + ); + + expect(result.success).toBe(true); + }); + + it("should still reject a step that identifies no element at all", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click" }, + ]), + ); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.1.selector"); + }); + + it("should still require the first step to navigate", () => { + const result = schema.safeParse(form([{ id: "1", action: "click", selector: "#login" }])); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.0.action"); + }); +}); + +// The step name is the string a failed run displays, and it was optional. Enforced +// by min(1).trim() in this schema rather than a second validation path, matching +// the monitor-level name rule. Recorded steps arrive already named from the +// recorder, so the friction lands on hand-added steps (D10). +describe("makeBrowserCheckSaveSchema step name", () => { + const schema = makeBrowserCheckSaveSchema(t); + const pin = { candidates: [], user_override: { kind: "css", value: "#go" } }; + + it("should reject a step with a blank name", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click", name: "", locator: pin }, + ]), + ); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.1.name"); + }); + + it("should reject a whitespace-only name", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click", name: " ", locator: pin }, + ]), + ); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.1.name"); + }); + + it("should report the message the editor binds to the field", () => { + const result = schema.safeParse( + form([{ id: "1", action: "navigate", value: "https://app.test", name: "" }]), + ); + + const issue = (result as any).error.issues.find( + (i: { path: PropertyKey[] }) => i.path.join(".") === "journey.0.name", + ); + expect(issue.message).toBe("synthetics.validation.stepNameRequired"); + }); + + it("should accept a recorded journey, which arrives already named", () => { + const result = schema.safeParse( + form([ + { id: "1", action: "navigate", value: "https://app.test", name: "Open app" }, + { + id: "2", + action: "click", + name: 'Click on [data-test="sign-in"]', + locator: { candidates: [{ kind: "test_attribute", value: '[data-test="sign-in"]' }] }, + }, + ]), + ); + + expect(result.success).toBe(true); + }); +}); + +// Field-level step rules live in the schema, not in validateJourneySteps, so +// there is one enforcement path and every failure carries a field path the editor +// can bind an inline error to. Before this, validation was save-time and +// toast-only and covered two rules (SE-3). +describe("makeBrowserCheckSaveSchema field-level step rules", () => { + const schema = makeBrowserCheckSaveSchema(t); + const pin = { candidates: [], user_override: { kind: "css", value: "#go" } }; + const opened = { id: "1", action: "navigate", value: "https://app.test" }; + + it("should reject a navigate step whose URL is not http(s)", () => { + const result = schema.safeParse(form([{ id: "1", action: "navigate", value: "app.test" }])); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.0.value"); + }); + + it("should reject a navigate step with no URL at all", () => { + const result = schema.safeParse(form([{ id: "1", action: "navigate" }])); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.0.value"); + }); + + it("should accept a navigate step with a valid URL", () => { + const result = schema.safeParse(form([opened])); + + expect(result.success).toBe(true); + }); + + // A `type` step with no text types nothing and the run still passes. + it("should reject a type step with no text", () => { + const result = schema.safeParse( + form([opened, { id: "2", action: "type", value: "", locator: pin }]), + ); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.1.value"); + }); + + it("should accept a type step that has text", () => { + const result = schema.safeParse( + form([opened, { id: "2", action: "type", value: "hunter2", locator: pin }]), + ); + + expect(result.success).toBe(true); + }); + + it("should reject an assertion kind that needs an expected value but has none", () => { + const result = schema.safeParse( + form([ + opened, + { + id: "2", + action: "assert", + locator: pin, + assertion: { kind: "element_text", expected: "" }, + }, + ]), + ); + + expect(result.success).toBe(false); + expect(issuePaths(result)).toContain("journey.1.assertion.expected"); + }); + + it("should accept a visibility assertion, which needs no expected value", () => { + const result = schema.safeParse( + form([ + opened, + { id: "2", action: "assert", locator: pin, assertion: { kind: "element_visible" } }, + ]), + ); + + expect(result.success).toBe(true); + }); +}); diff --git a/web/src/components/synthetics/CreateBrowserTest.schema.ts b/web/src/components/synthetics/CreateBrowserTest.schema.ts index 574f012197..fa2158f433 100644 --- a/web/src/components/synthetics/CreateBrowserTest.schema.ts +++ b/web/src/components/synthetics/CreateBrowserTest.schema.ts @@ -8,6 +8,16 @@ // • url — required + valid HTTP(S) URL. import { z } from "zod"; +import { stepIsMissingTarget } from "@/utils/synthetics/stepTarget"; +import { assertionNeedsExpected } from "@/constants/synthetics"; +import type { AssertionKind } from "@/types/synthetics"; + +/** The version-2 locator bundle, as it sits on an editor step. */ +const locatorCandidateSchema = z.object({ kind: z.string(), value: z.string() }); +const locatorSchema = z.object({ + candidates: z.array(locatorCandidateSchema).nullish(), + user_override: locatorCandidateSchema.nullish(), +}); export const makeBrowserCheckGateSchema = (t: (_key: string) => string) => z.object({ @@ -59,12 +69,26 @@ export const makeBrowserCheckSaveSchema = (t: (_key: string) => string) => z.object({ id: z.string(), action: z.string(), - name: z.string().optional(), + // The string a failed run displays, so it cannot be blank. Recorded + // steps arrive named from the recorder, which is why requiring it + // lands on hand-added steps rather than on every recording. + name: z.string().trim().min(1, t("synthetics.validation.stepNameRequired")), selector: z.string().optional(), selectorType: z.string().optional(), value: z.string().optional(), timeout: z.number().optional(), code: z.string().optional(), + // A version-2 step names its element here and carries no `selector`. + // Declared explicitly because z.object strips what it does not + // declare — leaving it out made every v2 step look target-less to + // the refinement below. + locator: locatorSchema.optional(), + // `expected` is declared, not just tolerated by `.loose()`, so the + // refinement below can read it in a typed way. + assertion: z + .object({ kind: z.string().optional(), expected: z.string().optional() }) + .loose() + .optional(), }), ) .optional() @@ -81,14 +105,9 @@ export const makeBrowserCheckSaveSchema = (t: (_key: string) => string) => }); } - // Validate selectors on steps that require them - const SELECTOR_ACTIONS = ["click", "type", "select", "hover", "assert"] as const; + // Every element-acting step must name its element, by either channel. for (let i = 0; i < val.journey.length; i++) { - const step = val.journey[i]; - if ( - SELECTOR_ACTIONS.includes(step.action as any) && - (!step.selector || step.selector.trim() === "") - ) { + if (stepIsMissingTarget(val.journey[i])) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["journey", i, "selector"], @@ -96,6 +115,43 @@ export const makeBrowserCheckSaveSchema = (t: (_key: string) => string) => }); } } + + // Field-level step rules. These live here rather than in + // validateJourneySteps so there is one enforcement path, and so every + // failure carries a field path the editor can bind an inline error to. + for (let i = 0; i < val.journey.length; i++) { + const step = val.journey[i]; + + if (step.action === "navigate" && !/^https?:\/\/\S+$/i.test(step.value ?? "")) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["journey", i, "value"], + message: t("synthetics.validation.urlInvalid"), + }); + } + + // A `type` step with no text types nothing and the run still passes. + if (step.action === "type" && !(step.value ?? "").trim()) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["journey", i, "value"], + message: t("synthetics.validation.typeTextRequired"), + }); + } + + if ( + step.action === "assert" && + step.assertion?.kind && + assertionNeedsExpected(step.assertion.kind as AssertionKind) && + !(step.assertion.expected ?? "").trim() + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["journey", i, "assertion", "expected"], + message: t("synthetics.validation.expectedRequired"), + }); + } + } }); export type BrowserCheckSaveForm = z.infer>; diff --git a/web/src/components/synthetics/StepEvidence.spec.ts b/web/src/components/synthetics/StepEvidence.spec.ts new file mode 100644 index 0000000000..0e8a399916 --- /dev/null +++ b/web/src/components/synthetics/StepEvidence.spec.ts @@ -0,0 +1,199 @@ +// Copyright 2026 OpenObserve Inc. + +import { describe, expect, it } from "vitest"; +import { mount } from "@vue/test-utils"; +import { createI18n } from "vue-i18n"; +import StepEvidence from "./StepEvidence.vue"; +import type { FailureDetail } from "@/composables/synthetics/syntheticResultsSchema"; +import en from "@/locales/languages/en-US.json"; + +const i18n = createI18n({ legacy: false, locale: "en-US", messages: { "en-US": en } }); + +function detail(over: Partial = {}): FailureDetail { + return { + stepId: "s2", + stepName: "Profile visible", + stepIndex: 2, + error: "locator.waitFor: Timeout 60000ms exceeded", + candidatesTried: [], + settleSignals: [], + settleMs: null, + observedDurationMs: null, + screenshotKey: null, + traceKey: null, + ...over, + }; +} + +function render(d: FailureDetail) { + return mount(StepEvidence, { props: { detail: d }, global: { plugins: [i18n] } }); +} + +describe("StepEvidence — locator resolution (P5.4 item 3)", () => { + it("says the element was absent when no candidate matched", () => { + const w = render( + detail({ + candidatesTried: [ + { kind: "test_attribute", value: "[data-test=x]", outcome: "not_found" }, + { kind: "role", value: "internal:role=button", outcome: "not_found" }, + ], + }), + ); + expect(w.find('[data-test="synthetics-run-detail-locator-none-matched"]').exists()).toBe(true); + expect(w.find('[data-test="synthetics-run-detail-locator-healed"]').exists()).toBe(false); + }); + + it("says the markup moved when a fallback matched instead of the primary", () => { + // This is the mechanical answer to "locator rot?" — and a different + // diagnosis from "not found", which is why they are separate messages. + const w = render( + detail({ + candidatesTried: [ + { kind: "test_attribute", value: "[data-test=x]", outcome: "not_found" }, + { kind: "role", value: "internal:role=button", outcome: "matched" }, + ], + }), + ); + expect(w.find('[data-test="synthetics-run-detail-locator-healed"]').exists()).toBe(true); + }); + + it("does not claim healing when the primary itself matched", () => { + const w = render( + detail({ + candidatesTried: [{ kind: "test_attribute", value: "[data-test=x]", outcome: "matched" }], + }), + ); + expect(w.find('[data-test="synthetics-run-detail-locator-healed"]').exists()).toBe(false); + }); +}); + +describe("StepEvidence — settle signals (P5.4 item 4)", () => { + it("flags a stale signal as the likely real cause", () => { + const w = render( + detail({ + settleSignals: [ + { + kind: "response", + signal: "response matching **/auth/login", + status: "stale", + required: false, + waitedMs: 30000, + }, + ], + }), + ); + expect(w.find('[data-test="synthetics-run-detail-settle-stale-note"]').exists()).toBe(true); + expect(w.text()).toContain("**/auth/login"); + }); + + it("stays quiet when every signal fired", () => { + const w = render( + detail({ + settleSignals: [ + { + kind: "navigation", + signal: "navigation to **/web/**", + status: "fired", + required: false, + waitedMs: 800, + }, + ], + }), + ); + expect(w.find('[data-test="synthetics-run-detail-settle-stale-note"]').exists()).toBe(false); + }); +}); + +describe("StepEvidence — settle timing (P5.4 item 5)", () => { + it("shows today against recording, and calls out a large regression", () => { + // Slow-but-healthy separates from broken on this line alone. + const w = render(detail({ settleMs: 41000, observedDurationMs: 2300 })); + expect(w.text()).toContain("41.0s"); + expect(w.text()).toContain("2.3s"); + expect(w.find('[data-test="synthetics-run-detail-settle-slower"]').exists()).toBe(true); + }); + + it("does not cry regression when timing is comparable", () => { + const w = render(detail({ settleMs: 2400, observedDurationMs: 2300 })); + expect(w.find('[data-test="synthetics-run-detail-settle-slower"]').exists()).toBe(false); + }); + + it("renders nothing for a record that carries no evidence", () => { + // Backwards compatibility: runs predating the probe change must degrade, + // not break the view. + const w = render(detail()); + expect(w.find('[data-test="synthetics-run-detail-locator-resolution"]').exists()).toBe(false); + expect(w.find('[data-test="synthetics-run-detail-settle-signals"]').exists()).toBe(false); + expect(w.find('[data-test="synthetics-run-detail-settle-timing"]').exists()).toBe(false); + }); +}); + +describe("StepEvidence — what the application did (Phase 6)", () => { + const appEvidence = { + stepId: "s2", + consoleErrors: 1, + pageErrors: 1, + requestsFailed: 0, + responsesNon2xx: 3, + worstResponses: [{ method: "POST", url: "https://x/auth/login", status: 503, count: 3 }], + firstConsoleErrors: ["[auth] sign-in failed: 503 Service Unavailable"], + }; + + function renderWith(over: Record = {}) { + return mount(StepEvidence, { + props: { detail: detail(), evidence: appEvidence, ...over }, + global: { plugins: [i18n] }, + }); + } + + it("shows the failing response — the thing no field distinguishes today", () => { + const w = renderWith(); + expect(w.find('[data-test="synthetics-run-detail-app-evidence"]').exists()).toBe(true); + expect(w.text()).toContain("503"); + expect(w.text()).toContain("/auth/login"); + }); + + it("collapses repeats into a count rather than listing them", () => { + expect(renderWith().text()).toContain("x3"); + }); + + it("shows the application's own console error", () => { + expect(renderWith().text()).toContain("sign-in failed"); + }); + + it("stays hidden when the step had nothing to report", () => { + // A healthy step contributes no rows — sparse by construction, not padded. + const w = renderWith({ + evidence: { + ...appEvidence, + consoleErrors: 0, + pageErrors: 0, + requestsFailed: 0, + responsesNon2xx: 0, + worstResponses: [], + firstConsoleErrors: [], + }, + }); + expect(w.find('[data-test="synthetics-run-detail-app-evidence"]').exists()).toBe(false); + }); + + it("stays hidden on a record that predates evidence capture", () => { + const w = renderWith({ evidence: null }); + expect(w.find('[data-test="synthetics-run-detail-app-evidence"]').exists()).toBe(false); + }); + + it("reports truncation rather than letting it pass silently", () => { + // X-8.2 — a capped buffer that drops events without saying so reads as + // "nothing else happened". + const w = renderWith({ truncated: true }); + expect(w.find('[data-test="synthetics-run-detail-evidence-truncated"]').exists()).toBe(true); + }); + + it("says nothing about truncation when nothing was dropped", () => { + expect( + renderWith({ truncated: false }) + .find('[data-test="synthetics-run-detail-evidence-truncated"]') + .exists(), + ).toBe(false); + }); +}); diff --git a/web/src/components/synthetics/StepEvidence.vue b/web/src/components/synthetics/StepEvidence.vue new file mode 100644 index 0000000000..4519b41125 --- /dev/null +++ b/web/src/components/synthetics/StepEvidence.vue @@ -0,0 +1,248 @@ + + + diff --git a/web/src/components/synthetics/journey/BrowserJourney.spec.ts b/web/src/components/synthetics/journey/BrowserJourney.spec.ts index 5c63a8ecb5..3df1af6194 100644 --- a/web/src/components/synthetics/journey/BrowserJourney.spec.ts +++ b/web/src/components/synthetics/journey/BrowserJourney.spec.ts @@ -211,41 +211,82 @@ describe("BrowserJourney recording", () => { expect(wrapper.find('[data-test="synthetics-journey-stop-btn"]').exists()).toBe(true); }); - it("should sync selector edit into step.wire when handleStepUpdate fires", async () => { - const wire = { - id: "w1", - action: "click", - selector: "#old", - name: "Old Click", - selector_type: "css", - }; + // The v1 counterpart of this test — editing the bare `selector` input and + // asserting it reached `wire.selector` — was deleted with the v1 authoring path. + // The mechanism it guarded (an edit must land in `wire`, or journeyToWireSteps + // discards it at replay time) is covered by the navigate-URL test below. + + // Regression: this view used to inline its own, thinner copy of the step + // editor. It rendered no locator bundle, no settle block, no assertion editor + // and no optional/always-run checkboxes, so which fields an author could see + // depended on whether they were recording or editing a saved check. + it("should render the full step editor, not a reduced copy of it", async () => { const step = { id: "s1", action: "click", - name: "Old Click", - selector: "#old", - timeout: 30000, + name: "Sign in", code: "", - wire, + locator: { candidates: [{ kind: "test_attribute", value: '[data-test="sign-in"]' }] }, + settle: { + navigation: { url_pattern: "**/web/**" }, + responses: [{ url_pattern: "**/auth/login", method: "POST", required: false }], + budget_ms: 5000, + }, + wire: { id: "w1", action: "click" }, }; wrapper = mount(BrowserJourney, { props: { modelValue: [step] }, - global: { - stubs: { ...STUBS, JourneySteps: JourneyStepsStubWithExpansion }, - }, + global: { stubs: { ...STUBS, JourneySteps: JourneyStepsStubWithExpansion } }, }); - // The expansion slot renders an OInput for the selector with - // data-test="synthetics-journey-step-selector-input". Update its value. - const selectorInput = wrapper.find('[data-test="synthetics-journey-step-selector-input"]'); - await selectorInput.setValue("#new"); + // The editor keeps its tuning fields behind one `Advanced` collapsible (SE-5), + // and OCollapsible unmounts collapsed content — so open it before asserting. The + // point of this test is that no field is MISSING, not that all of them are + // visible at once. + const advanced = wrapper.find('[data-test="synthetics-journey-step-group-advanced"] button'); + expect(advanced.exists()).toBe(true); + if (advanced.attributes("data-state") !== "open") await advanced.trigger("click"); - const emitted = wrapper.emitted("update:modelValue"); - expect(emitted).toBeTruthy(); - const updatedSteps = emitted![emitted!.length - 1][0] as any[]; - expect(updatedSteps[0].selector).toBe("#new"); - expect(updatedSteps[0].wire.selector).toBe("#new"); + for (const dt of [ + "synthetics-journey-step-editor", + "synthetics-journey-step-group-does", + "synthetics-journey-step-group-advanced", + "synthetics-journey-step-locator", + "synthetics-journey-step-settle", + "synthetics-journey-step-settle-required-0", + "synthetics-journey-step-settle-budget-input", + "synthetics-journey-step-optional-checkbox", + "synthetics-journey-step-always-run-checkbox", + "synthetics-journey-step-timeout-input", + ]) { + expect(wrapper.find(`[data-test="${dt}"]`).exists(), dt).toBe(true); + } + }); + + it("should route an edited navigate URL to wire.url so replay uses it", async () => { + const step = { + id: "s1", + action: "navigate", + name: "Open page", + value: "https://old.test", + code: "", + wire: { id: "w1", action: "navigate", url: "https://old.test" }, + }; + + wrapper = mount(BrowserJourney, { + props: { modelValue: [step] }, + global: { stubs: { ...STUBS, JourneySteps: JourneyStepsStubWithExpansion } }, + }); + + await wrapper + .find('[data-test="synthetics-journey-step-value-input"]') + .setValue("https://new.test"); + + const emitted = wrapper.emitted("update:modelValue")!; + const next = emitted[emitted.length - 1][0] as any[]; + expect(next[0].value).toBe("https://new.test"); + expect(next[0].wire.url).toBe("https://new.test"); }); it("should emit clear-results when modelValue becomes empty", async () => { @@ -260,3 +301,242 @@ describe("BrowserJourney recording", () => { expect(wrapper.emitted("clear-results")).toBeTruthy(); }); }); + +describe("BrowserJourney step validation", () => { + let wrapper: VueWrapper; + + afterEach(() => { + wrapper?.unmount(); + vi.restoreAllMocks(); + }); + + function validate(modelValue: unknown[]): boolean { + wrapper = mountJourney({ modelValue }); + return (wrapper.vm as any).validateStepSelectors(); + } + + it("should pass a v1 journey whose steps carry selectors", () => { + expect( + validate([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click", selector: "#login" }, + ]), + ).toBe(true); + }); + + // Regression: a v2 step identifies its element with a locator bundle and has + // no `selector` at all. Requiring `selector` blocked Save & Continue on every + // recorded journey the moment it was reopened for editing. + it("should pass a v2 step that identifies its element by locator", () => { + expect( + validate([ + { id: "1", action: "navigate", value: "https://app.test" }, + { + id: "2", + action: "click", + locator: { + candidates: [{ kind: "test_attribute", value: 'internal:testid=[data-test="login"]' }], + user_override: null, + }, + }, + ]), + ).toBe(true); + }); + + it("should pass a v2 step whose only target is a pinned override", () => { + expect( + validate([ + { id: "1", action: "navigate", value: "https://app.test" }, + { + id: "2", + action: "click", + locator: { candidates: [], user_override: { kind: "css", value: "#login" } }, + }, + ]), + ).toBe(true); + }); + + it("should pass a page-level assertion, which targets no element", () => { + expect( + validate([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "assert", assertion: { kind: "url_matches", expected: "/home" } }, + ]), + ).toBe(true); + }); + + it("should fail a step that identifies no element at all", () => { + expect( + validate([ + { id: "1", action: "navigate", value: "https://app.test" }, + { id: "2", action: "click" }, + ]), + ).toBe(false); + }); + + it("should fail when the first step does not navigate", () => { + expect(validate([{ id: "1", action: "click", selector: "#login" }])).toBe(false); + }); +}); + +// ── Step creation ───────────────────────────────────────────────────────── +// A created step must carry no timeout. Absence means "use the runner's +// per-category default" (spec P1.1.1-P1.1.3); a stamped value freezes at 30000 +// while recorded steps follow the default, and fires the below-default warning +// as soon as the author picks navigate or assert (default 60000) without ever +// having touched the field. +describe("BrowserJourney step creation", () => { + let wrapper: VueWrapper; + + afterEach(() => { + wrapper?.unmount(); + vi.restoreAllMocks(); + }); + + function lastEmittedSteps(w: VueWrapper): any[] { + const emitted = w.emitted("update:modelValue"); + expect(emitted).toBeTruthy(); + return emitted![emitted!.length - 1][0] as any[]; + } + + it("should create a step with no timeout via Add Step", async () => { + wrapper = mountJourney({ modelValue: [] }); + await wrapper.find('[data-test="synthetics-journey-add-step-btn"]').trigger("click"); + + const steps = lastEmittedSteps(wrapper); + expect(steps).toHaveLength(1); + expect(steps[0].timeout).toBeUndefined(); + }); + + it("should create a step with no timeout via insert below", async () => { + const existing = { + id: "s1", + action: "navigate", + name: "Open app", + value: "https://app.test", + code: "", + }; + wrapper = mountJourney({ modelValue: [existing] }); + + // Drive the row action through the event contract JourneySteps emits, + // rather than reaching into the component's internals. + wrapper.findComponent(JourneyStepsStub).vm.$emit("insert-below", existing); + await flushPromises(); + + const steps = lastEmittedSteps(wrapper); + expect(steps).toHaveLength(2); + expect(steps[1].timeout).toBeUndefined(); + }); +}); + +// A created step is a version-2 step: its identity is the locator bundle, never a +// bare `selector`. Seeding the bundle empty is what makes the editor render the +// Locator block from the start, and what keeps isV2Journey true once the author +// supplies a locator (SE-18). +describe("BrowserJourney step creation is version 2", () => { + let wrapper: VueWrapper; + + afterEach(() => { + wrapper?.unmount(); + vi.restoreAllMocks(); + }); + + it("should seed an empty locator bundle and write no v1 selector fields", async () => { + wrapper = mountJourney({ modelValue: [] }); + await wrapper.find('[data-test="synthetics-journey-add-step-btn"]').trigger("click"); + + const emitted = wrapper.emitted("update:modelValue")!; + const steps = emitted[emitted.length - 1][0] as any[]; + expect(steps[0].locator).toEqual({ candidates: [], user_override: null }); + expect(steps[0].selector).toBeUndefined(); + expect(steps[0].selectorType).toBeUndefined(); + }); +}); + +// Phase 5 / SE-4. The evidence existed and was discarded: JourneySteps declares a +// getReplayResult prop "for error cards" and never rendered one, and BrowserJourney +// never passed it. A failed replay showed a red dot and a one-line banner only. +describe("BrowserJourney per-step failure evidence", () => { + let wrapper: VueWrapper; + + afterEach(() => { + wrapper?.unmount(); + vi.restoreAllMocks(); + }); + + const journey = [ + { id: "s1", action: "navigate", name: "Open app", value: "https://app.test", code: "" }, + { + id: "s2", + action: "click", + name: "Sign in", + code: "", + locator: { candidates: [{ kind: "css", value: "#go" }] }, + }, + ]; + + function mountFailed() { + const stepResults = new Map([ + [ + "s2", + { + stepId: "s2", + stepName: "Sign in", + passed: false, + durationMs: 30000, + error: "Timeout 30000ms exceeded.", + fidelity: { level: "reduced", notes: ["primary locator only"] }, + }, + ], + ]); + return mount(BrowserJourney, { + props: { modelValue: journey, replayPhase: "failed", stepResults }, + global: { stubs: { ...STUBS, JourneySteps: JourneyStepsStubWithExpansion } }, + }) as VueWrapper; + } + + it("should render the error card against the step that failed", () => { + wrapper = mountFailed(); + const cards = wrapper.findAll('[data-test="synthetics-journey-step-error-card"]'); + expect(cards.length).toBe(1); + expect(cards[0].text()).toContain("Timeout 30000ms exceeded"); + }); + + it("should surface the player's fidelity notes (X-8.2)", () => { + wrapper = mountFailed(); + expect(wrapper.find('[data-test="synthetics-journey-step-fidelity"]').text()).toContain( + "primary locator only", + ); + }); + + it("should not render a card for a step that passed", () => { + const stepResults = new Map([ + ["s1", { stepId: "s1", stepName: "Open app", passed: true, durationMs: 900 }], + ]); + wrapper = mount(BrowserJourney, { + props: { modelValue: journey, replayPhase: "passed", stepResults }, + global: { stubs: { ...STUBS, JourneySteps: JourneyStepsStubWithExpansion } }, + }) as VueWrapper; + expect(wrapper.find('[data-test="synthetics-journey-step-error-card"]').exists()).toBe(false); + }); + + it("should not render a card when no replay has run", () => { + wrapper = mount(BrowserJourney, { + props: { modelValue: journey }, + global: { stubs: { ...STUBS, JourneyStepsStubWithExpansion } }, + }) as VueWrapper; + expect(wrapper.find('[data-test="synthetics-journey-step-error-card"]').exists()).toBe(false); + }); + + // The old button emitted a full journey replay from inside a per-step card. A + // single step is not independently runnable, so the honest unit is the prefix. + it("should emit replay-up-to with the failed step's position", async () => { + wrapper = mountFailed(); + await wrapper.find('[data-test="synthetics-journey-error-retry-btn"]').trigger("click"); + // OButtonStub both $emits "click" and lets the native event through (it declares + // no `emits`), so one press registers twice. The payload is the contract here. + const emitted = wrapper.emitted("replay-up-to")!; + expect(emitted.length).toBeGreaterThan(0); + for (const call of emitted) expect(call).toEqual([2]); + }); +}); diff --git a/web/src/components/synthetics/journey/BrowserJourney.vue b/web/src/components/synthetics/journey/BrowserJourney.vue index dc690f9cb3..228a8eb5ca 100644 --- a/web/src/components/synthetics/journey/BrowserJourney.vue +++ b/web/src/components/synthetics/journey/BrowserJourney.vue @@ -2,35 +2,35 @@ // Copyright 2026 OpenObserve Inc. import { computed, onBeforeUnmount, onMounted, ref, watch } from "vue"; import { useI18n } from "vue-i18n"; -import type { BrowserStep, ReplayPhase, StepReplayResult, WireStep } from "@/types/synthetics"; +import type { BrowserStep, ReplayPhase, StepReplayResult } from "@/types/synthetics"; import type { StepDotState } from "./JourneySteps.vue"; import useSyntheticsRecorder from "@/composables/useSyntheticsRecorder"; import { getUUIDv7 } from "@/utils/zincutils"; import OButton from "@/lib/core/Button/OButton.vue"; import OIcon from "@/lib/core/Icon/OIcon.vue"; import OInput from "@/lib/forms/Input/OInput.vue"; -import OSelect from "@/lib/forms/Select/OSelect.vue"; import OBadge from "@/lib/core/Badge/OBadge.vue"; import OCheckbox from "@/lib/forms/Checkbox/OCheckbox.vue"; -import OTooltip from "@/lib/overlay/Tooltip/OTooltip.vue"; import ConfirmDialog from "@/components/ConfirmDialog.vue"; import { toast } from "@/lib/feedback/Toast/useToast"; import JourneySteps from "./JourneySteps.vue"; -import { - ACTION_LABELS, - SELECTOR_ACTIONS as SELECTOR_ACTIONS_CONST, - VALUE_ACTIONS as VALUE_ACTIONS_CONST, - VALUE_LABELS, - SELECTOR_TYPE_OPTIONS, - actionOptions, - VALUE_WIDTH_MAP, - VALUE_TOOLTIP_MAP, -} from "@/constants/synthetics"; +import UpgradeJourneyBanner from "./UpgradeJourneyBanner.vue"; +import ZeroAssertionNotice from "./ZeroAssertionNotice.vue"; +import TestIdMisconfiguredNotice from "./TestIdMisconfiguredNotice.vue"; +import { DEFAULT_TEST_ID_ATTR } from "@/constants/synthetics"; +import BrowserJourneyStepEditor from "./BrowserJourneyStepEditor.vue"; +import BrowserJourneyStepError from "./BrowserJourneyStepError.vue"; +import { stepIsMissingTarget } from "@/utils/synthetics/stepTarget"; const props = defineProps<{ modelValue: BrowserStep[]; readonly?: boolean; startUrl?: string; // URL shown in the recording banner + /** + * DOM attribute the recorder selects on, from the monitor's config. + * Absent falls back to DEFAULT_TEST_ID_ATTR — see useSyntheticsRecorder. + */ + testIdAttr?: string; extensionReady?: boolean; // when false, Record button triggers need-extension-setup autoRecord?: boolean; // if true, start recording immediately on mount /** Owned by the parent (CreateBrowserTest). */ @@ -48,6 +48,17 @@ const emit = defineEmits<{ "need-extension-setup": []; "clear-results": []; replay: []; + /** + * Replay only the first `upTo` steps (1-based, inclusive). + * + * A single step is not independently runnable — journey state is cumulative and + * the extension starts each replay from the target URL, so step 5 alone would run + * against a fresh page with none of the preceding state. A PREFIX is runnable, and + * `replay()` already accepts an arbitrary WireStep[], so this needs no extension + * change. The old error-card button emitted a full `replay` while sitting inside a + * per-step card, promising something it did not do (SE-4). + */ + "replay-up-to": [upTo: number]; "stop-replay": []; "auto-record-consumed": []; "selection-changed": [{ count: number; isRecording: boolean }]; @@ -106,6 +117,39 @@ const failedStepResult = computed(() => { return props.stepResults?.get(step.id); }); +/** + * The failed replay result for a given row, if any. + * + * Only while a replay is in a terminal/active state — a stale result from a previous + * run must not keep a card on screen after the journey is edited. + */ +function failedResultFor(row: BrowserStep): StepReplayResult | undefined { + if (!isReplayActive.value) return undefined; + const r = props.stepResults?.get(row.id); + return r && !r.passed ? r : undefined; +} + +function stepNumberOf(row: BrowserStep): number { + return props.modelValue.findIndex((s) => s.id === row.id) + 1; +} + +/** + * A failed step's evidence lives in the row's expansion, so open it automatically — + * the same thing validateJourneySteps does for validation errors. Without this a + * tester has to guess which row to expand to find out what happened. + */ +watch( + () => (props.replayPhase === "failed" ? firstFailedIndex.value : -1), + (idx) => { + if (idx < 0) return; + const step = props.modelValue[idx]; + if (step && !expandedStepIds.value.includes(step.id)) { + expandedStepIds.value = [...expandedStepIds.value, step.id]; + } + }, + { immediate: true }, +); + /** Derive the status dot state for a step based on replay results. */ function stepDotState(stepId: string): StepDotState | undefined { if (!isReplayActive.value || !props.replayPhase) return undefined; @@ -204,20 +248,53 @@ watch([selectedCount, isRecording], ([count, recording]) => { const selectorErrors = ref>(new Set()); const firstStepError = ref(false); +/** + * Field errors for the expanded editor, keyed by step id then field name. + * + * Populated from the zod issue paths so one enforcement path produces both the + * save block and the inline messages. Keying by step **id** rather than index + * means a reorder or a delete cannot leave an error pointing at the wrong row. + */ +const stepFieldErrors = ref>>(new Map()); + +/** Record zod issues whose path points at a journey step field. */ +function setStepFieldErrors(issues: { path: PropertyKey[]; message: string }[]) { + const next = new Map>(); + for (const issue of issues) { + if (issue.path[0] !== "journey" || typeof issue.path[1] !== "number") continue; + const step = props.modelValue[issue.path[1]]; + if (!step) continue; + // `journey.3.assertion.expected` → "assertion.expected"; a bare + // `journey.3` (a whole-step issue) is attributed to the action field. + const field = issue.path.slice(2).join(".") || "action"; + next.set(step.id, { ...(next.get(step.id) ?? {}), [field]: issue.message }); + } + stepFieldErrors.value = next; +} + +function fieldError(stepId: string, field: string): string { + return stepFieldErrors.value.get(stepId)?.[field] ?? ""; +} + +function clearFieldError(stepId: string, field: string) { + const current = stepFieldErrors.value.get(stepId); + if (!current?.[field]) return; + const { [field]: _dropped, ...rest } = current; + const next = new Map(stepFieldErrors.value); + next.set(stepId, rest); + stepFieldErrors.value = next; +} + function validateJourneySteps(): boolean { // 1. First step must be "navigate" const first = props.modelValue[0]; firstStepError.value = first ? first.action !== "navigate" : false; - // 2. Selector-requiring steps must have a selector + // 2. Element-acting steps must name their element — by a v1 `selector` or a + // v2 locator bundle. See stepIsMissingTarget. const selErrs = new Set(); for (const step of props.modelValue) { - if ( - SELECTOR_ACTIONS_CONST.includes(step.action as any) && - (!step.selector || step.selector.trim() === "") - ) { - selErrs.add(step.id); - } + if (stepIsMissingTarget(step)) selErrs.add(step.id); } selectorErrors.value = selErrs; @@ -268,10 +345,14 @@ defineExpose({ stopActiveRecording, stopActiveReplay, validateStepSelectors: validateJourneySteps, + // The parent view owns the zod parse, so it pushes the resulting issues back + // down here to be rendered against the fields they name. `fieldError` stays + // internal — the template is its only caller. + setStepFieldErrors, }); function startRecording() { - recorder.startRecording(props.startUrl ?? "").catch((err) => { + recorder.startRecording(props.startUrl ?? "", props.testIdAttr).catch((err) => { console.log("error ---", err); recorder.error.value = err instanceof Error ? err.message : String(err); }); @@ -409,8 +490,12 @@ function handleInsertBelow(row: BrowserStep) { id: getUUIDv7(true), action: "click", name: "", - timeout: 30000, code: "", + // A new step is a version-2 step: its identity is the locator bundle, never a + // bare `selector`. Seeding it empty is what makes the editor render the + // Locator block from the start, and what lets isV2Journey stay true once the + // author supplies a locator instead of flipping the journey to v1 (SE-18). + locator: { candidates: [], user_override: null }, }); emit("update:modelValue", next); } @@ -426,7 +511,14 @@ function handleUpdateExpanded(ids: string[]) { function addStep() { emit("update:modelValue", [ ...props.modelValue, - { id: getUUIDv7(true), action: "click", name: "", timeout: 30000, code: "" }, + { + id: getUUIDv7(true), + action: "click", + name: "", + code: "", + // See handleInsertBelow — a new step is version 2. + locator: { candidates: [], user_override: null }, + }, ]); } function duplicateCapturedStep(index: number, step: BrowserStep) { @@ -447,44 +539,16 @@ function getRowStatusColor(row: BrowserStep): string | undefined { return undefined; } -// ── Inline editor helpers ────────────────────────────────────────────────── -const selectorActions = SELECTOR_ACTIONS_CONST; -const valueActions = VALUE_ACTIONS_CONST; -const selectorTypeOptions = SELECTOR_TYPE_OPTIONS; - -function valueActionLabel(action: string): string { - return VALUE_LABELS[action] || t("synthetics.journey.valueFallback"); -} - -function valueWidthClass(action: string): string { - return VALUE_WIDTH_MAP[action] || "w-152!"; -} - -function valueTooltip(action: string): string | undefined { - return VALUE_TOOLTIP_MAP[action]; -} - -function handleStepUpdate(row: BrowserStep, patch: Partial) { +// ── Inline editor ────────────────────────────────────────────────────────── +// BrowserJourneyStepEditor owns the field rendering AND the wire sync, and +// emits a complete replacement step. Keeping a second copy of that logic here +// is what let the two editors drift apart in the first place. +function handleStepReplace(row: BrowserStep, next: BrowserStep) { const idx = findIndex(row); if (idx < 0) return; - const next = [...props.modelValue]; - - // Sync edits into the recorded wire step so the API receives the updated - // values. journeyToWireSteps prefers wire over UI fields, so without this - // sync, edits to recorded steps are silently discarded on save. - let wire = next[idx].wire ? { ...next[idx].wire } : undefined; - if (wire) { - if (patch.name !== undefined) wire.name = patch.name; - if (patch.selector !== undefined) wire.selector = patch.selector; - if (patch.selectorType !== undefined) - wire.selector_type = patch.selectorType.toLowerCase() as WireStep["selector_type"]; - if (patch.value !== undefined) wire.value = patch.value; - if (patch.timeout !== undefined) wire.timeout_ms = patch.timeout; - if (patch.action !== undefined) wire = undefined; // action changed → wire metadata is no longer accurate - } - - next[idx] = { ...next[idx], wire, ...patch }; - emit("update:modelValue", next); + const steps = [...props.modelValue]; + steps[idx] = next; + emit("update:modelValue", steps); } function openChromeExtensions() { @@ -608,6 +672,31 @@ function openChromeExtensions() { + + + + + + + + +
- + diff --git a/web/src/components/synthetics/journey/BrowserJourneyAssertion.spec.ts b/web/src/components/synthetics/journey/BrowserJourneyAssertion.spec.ts new file mode 100644 index 0000000000..d8e059c948 --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyAssertion.spec.ts @@ -0,0 +1,92 @@ +// Copyright 2026 OpenObserve Inc. + +import { describe, expect, it } from "vitest"; +import { mount } from "@vue/test-utils"; +import { createI18n } from "vue-i18n"; +import type { StepAssertion } from "@/types/synthetics"; +import BrowserJourneyAssertion from "./BrowserJourneyAssertion.vue"; +import en from "@/locales/languages/en-US.json"; + +const i18n = createI18n({ + legacy: false, + locale: "en-US", + fallbackLocale: "en-US", + messages: { "en-US": en as Record }, +}); + +function render(assertion?: StepAssertion) { + return mount(BrowserJourneyAssertion, { + props: { assertion }, + global: { plugins: [i18n] }, + }); +} + +const test = (name: string) => `[data-test="${name}"]`; +const EXPECTED = test("synthetics-journey-step-assertion-expected-input"); +const ATTRIBUTE = test("synthetics-journey-step-assertion-attribute-input"); + +describe("BrowserJourneyAssertion", () => { + // An assert step that predates Phase 5 keeps its original meaning rather than + // rendering as an empty, invalid form. + it("defaults to element_visible when the step has no typed assertion", () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-assertion")).exists()).toBe(true); + // Visibility asks "is it there?" — there is nothing to compare. + expect(wrapper.find(EXPECTED).exists()).toBe(false); + expect(wrapper.find(ATTRIBUTE).exists()).toBe(false); + }); + + it("asks for an expected value on every kind except the visibility ones", () => { + expect(render({ kind: "element_text", expected: "Welcome" }).find(EXPECTED).exists()).toBe( + true, + ); + expect(render({ kind: "url_matches", expected: "**/web/**" }).find(EXPECTED).exists()).toBe( + true, + ); + expect(render({ kind: "page_title", expected: "Dashboard" }).find(EXPECTED).exists()).toBe( + true, + ); + expect(render({ kind: "element_not_visible" }).find(EXPECTED).exists()).toBe(false); + }); + + it("asks for an attribute name only for element_attribute", () => { + expect( + render({ kind: "element_attribute", attribute: "href", expected: "/web/" }) + .find(ATTRIBUTE) + .exists(), + ).toBe(true); + expect(render({ kind: "element_text", expected: "x" }).find(ATTRIBUTE).exists()).toBe(false); + }); + + it("emits the edited expected value", async () => { + const wrapper = render({ kind: "element_text", expected: "" }); + await wrapper.find(`${EXPECTED} input`).setValue("Welcome back"); + const emitted = wrapper.emitted("update:assertion")?.at(-1)?.[0] as StepAssertion; + expect(emitted).toEqual({ kind: "element_text", expected: "Welcome back" }); + }); + + // A stale `attribute` left on a page_title assertion would be refused by + // server validation with no visible cause, so switching kind drops what no + // longer applies. + it("drops values that no longer apply when the kind changes", async () => { + const wrapper = render({ kind: "element_attribute", attribute: "href", expected: "/web/" }); + (wrapper.vm as unknown as { kindComputed: string }).kindComputed = "element_visible"; + await wrapper.vm.$nextTick(); + + const emitted = wrapper.emitted("update:assertion")?.at(-1)?.[0] as StepAssertion; + expect(emitted).toEqual({ kind: "element_visible" }); + }); + + it("offers exactly the closed kind set the server accepts", () => { + const wrapper = render(); + const options = (wrapper.vm as unknown as { kindOptions: { value: string }[] }).kindOptions; + expect(options.map((o) => o.value)).toEqual([ + "element_visible", + "element_not_visible", + "element_text", + "url_matches", + "page_title", + "element_attribute", + ]); + }); +}); diff --git a/web/src/components/synthetics/journey/BrowserJourneyAssertion.vue b/web/src/components/synthetics/journey/BrowserJourneyAssertion.vue new file mode 100644 index 0000000000..1c2bf1445a --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyAssertion.vue @@ -0,0 +1,104 @@ + + + diff --git a/web/src/components/synthetics/journey/BrowserJourneyLocator.spec.ts b/web/src/components/synthetics/journey/BrowserJourneyLocator.spec.ts new file mode 100644 index 0000000000..43f0bbf6b6 --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyLocator.spec.ts @@ -0,0 +1,289 @@ +// Copyright 2026 OpenObserve Inc. + +import { describe, expect, it } from "vitest"; +import { mount } from "@vue/test-utils"; +import { createI18n } from "vue-i18n"; +import type { StepLocator } from "@/types/synthetics"; +import BrowserJourneyLocator from "./BrowserJourneyLocator.vue"; +import en from "@/locales/languages/en-US.json"; + +const i18n = createI18n({ + legacy: false, + locale: "en-US", + fallbackLocale: "en-US", + messages: { "en-US": en as Record }, +}); + +const BUNDLE: StepLocator = { + candidates: [ + { kind: "test_attribute", value: '[data-test="login-sign-in"]' }, + { kind: "role", value: 'role=button[name="Sign In"]' }, + { kind: "css", value: ".btn-primary" }, + ], +}; + +function render(locator: StepLocator = BUNDLE) { + return mount(BrowserJourneyLocator, { + props: { locator }, + global: { plugins: [i18n] }, + }); +} + +const test = (name: string) => `[data-test="${name}"]`; + +describe("BrowserJourneyLocator", () => { + it("shows the primary candidate as the effective locator", () => { + const wrapper = render(); + const primary = wrapper.find(test("synthetics-journey-step-locator-primary")); + expect(primary.text()).toContain('[data-test="login-sign-in"]'); + expect(primary.text()).toContain("Test attribute"); + }); + + // The fallbacks used to sit behind an `OCollapsible "N fallbacks"` (P2.5/T5). + // A count advertised nothing, and the list is what the runner will actually try + // if the primary stops matching, so the click bought the author no information. + it("shows every remaining candidate without a disclosure", () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-locator-fallbacks")).exists()).toBe(true); + const list = wrapper.find(test("synthetics-journey-step-locator-fallbacks")).text(); + expect(list).toContain('role=button[name="Sign In"]'); + expect(list).toContain(".btn-primary"); + expect(wrapper.text()).not.toContain("2 fallbacks"); + }); + + it("says what the fallback list is for rather than how long it is", () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-locator-fallbacks-lead")).text()).toMatch( + /tried in order/i, + ); + }); + + // A pinned step never falls back, so the lead-in would be a lie; the pinned note + // stands in for it and the rows render inert. + it("drops the fallback lead-in when the step is pinned", () => { + const wrapper = render({ ...BUNDLE, user_override: { kind: "css", value: "#pinned" } }); + expect(wrapper.find(test("synthetics-journey-step-locator-fallbacks-lead")).exists()).toBe( + false, + ); + expect(wrapper.find(test("synthetics-journey-step-locator-pinned-note")).exists()).toBe(true); + }); + + // P2.5.0 — the list is machine-derived evidence. Pinning is the only way for + // an author to express intent, which is what keeps the stored list + // byte-comparable for the self-healing precondition. + it("emits a user_override when a candidate is pinned, leaving the list untouched", async () => { + const wrapper = render(); + await wrapper.find(test("synthetics-journey-step-locator-pin-primary-btn")).trigger("click"); + + const emitted = wrapper.emitted("update:locator")?.[0]?.[0] as StepLocator; + expect(emitted.user_override).toEqual({ + kind: "test_attribute", + value: '[data-test="login-sign-in"]', + }); + expect(emitted.candidates).toEqual(BUNDLE.candidates); + }); + + it("pins a fallback candidate without reordering the list", async () => { + const wrapper = render(); + const pinButtons = wrapper.findAll(test("synthetics-journey-step-locator-pin-btn")); + expect(pinButtons.length).toBe(2); + await pinButtons[0].trigger("click"); + + const emitted = wrapper.emitted("update:locator")?.[0]?.[0] as StepLocator; + expect(emitted.user_override?.value).toBe('role=button[name="Sign In"]'); + expect(emitted.candidates.map((c) => c.value)).toEqual(BUNDLE.candidates.map((c) => c.value)); + }); + + it("shows the pinned locator as the effective one, and says it will not fall back", () => { + const wrapper = render({ + ...BUNDLE, + user_override: { kind: "css", value: "#pinned" }, + }); + expect(wrapper.find(test("synthetics-journey-step-locator-primary")).text()).toContain( + "#pinned", + ); + expect(wrapper.find(test("synthetics-journey-step-locator-pinned-note")).exists()).toBe(true); + }); + + it("clears the pin, restoring fallback", async () => { + const wrapper = render({ ...BUNDLE, user_override: { kind: "css", value: "#pinned" } }); + await wrapper.find(test("synthetics-journey-step-locator-unpin-btn")).trigger("click"); + const emitted = wrapper.emitted("update:locator")?.[0]?.[0] as StepLocator; + expect(emitted.user_override).toBeNull(); + }); + + // Free text is intent, not evidence. Editing a candidate in place would + // corrupt the list the healing precondition compares against. + it("turns free text into a user_override rather than editing a candidate", async () => { + const wrapper = render(); + await wrapper + .find(`${test("synthetics-journey-step-locator-override-input")} input`) + .setValue("#hand-written"); + await wrapper.find(test("synthetics-journey-step-locator-override-btn")).trigger("click"); + + const emitted = wrapper.emitted("update:locator")?.[0]?.[0] as StepLocator; + expect(emitted.user_override).toEqual({ kind: "css", value: "#hand-written" }); + expect(emitted.candidates).toEqual(BUNDLE.candidates); + }); + + it("renders a single-candidate bundle with no fallback section", () => { + const wrapper = render({ candidates: [{ kind: "css", value: "#only" }] }); + expect(wrapper.find(test("synthetics-journey-step-locator-primary")).text()).toContain("#only"); + expect(wrapper.find(test("synthetics-journey-step-locator-fallbacks")).exists()).toBe(false); + }); +}); + +describe("all-positional notice (Phase 2a)", () => { + const positional = { + candidates: [ + { kind: "test_attribute" as const, value: '[data-test="org-item"] >> nth=1' }, + { kind: "css" as const, value: "div >> internal:has-text=/^Acme$/ >> nth=0" }, + ], + user_override: null, + }; + + it("warns when every candidate identifies the element by position", () => { + const wrapper = mount(BrowserJourneyLocator, { + props: { locator: positional }, + global: { plugins: [i18n] }, + }); + expect( + wrapper.find('[data-test="synthetics-journey-step-locator-positional-warning"]').exists(), + ).toBe(true); + }); + + it("stays silent when any candidate is unambiguous", () => { + const wrapper = mount(BrowserJourneyLocator, { + props: { + locator: { + candidates: [ + { kind: "role" as const, value: 'internal:role=button[name="Save"i]' }, + { kind: "test_attribute" as const, value: '[data-test="org-item"] >> nth=1' }, + ], + user_override: null, + }, + }, + global: { plugins: [i18n] }, + }); + expect( + wrapper.find('[data-test="synthetics-journey-step-locator-positional-warning"]').exists(), + ).toBe(false); + }); + + it("stays silent once the author has pinned — the question is answered", () => { + const wrapper = mount(BrowserJourneyLocator, { + props: { + locator: { ...positional, user_override: { kind: "css" as const, value: "#chosen" } }, + }, + global: { plugins: [i18n] }, + }); + expect( + wrapper.find('[data-test="synthetics-journey-step-locator-positional-warning"]').exists(), + ).toBe(false); + }); +}); + +// ── Empty bundle ────────────────────────────────────────────────────────── +// A hand-added step carries `{ candidates: [], user_override: null }`. Every +// other block is v-if'd away, so the free-text input is the PRIMARY way to name +// the element — not an override of something — and it must say so, and be marked +// required, because the block only renders when a target is needed (D7). +describe("BrowserJourneyLocator empty bundle", () => { + const EMPTY: StepLocator = { candidates: [], user_override: null }; + + // `data-test` sits on the OInput root, so reach the inner to set a + // value — the pattern the pinning tests above already use. + function overrideInput(wrapper: ReturnType) { + return wrapper.find(`${test("synthetics-journey-step-locator-override-input")} input`); + } + + it("labels the input as the primary control, not an override", () => { + const wrapper = render(EMPTY); + expect(overrideInput(wrapper).exists()).toBe(true); + expect(wrapper.text()).toContain("How to find this element"); + expect(wrapper.text()).not.toContain("Use a different locator"); + }); + + it("shows no primary card, no fallbacks and no positional warning", () => { + const wrapper = render(EMPTY); + expect(wrapper.find(test("synthetics-journey-step-locator-primary")).exists()).toBe(false); + expect(wrapper.find(test("synthetics-journey-step-locator-fallbacks")).exists()).toBe(false); + expect(wrapper.find(test("synthetics-journey-step-locator-positional-warning")).exists()).toBe( + false, + ); + }); + + it("marks the input required — the block only renders when a target is needed", () => { + const wrapper = render(EMPTY); + expect(wrapper.findComponent({ name: "OInput" }).props("required")).toBe(true); + }); + + it("still emits update:locator when a value is applied", async () => { + const wrapper = render(EMPTY); + await overrideInput(wrapper).setValue('[data-test="sign-in"]'); + await wrapper.find(test("synthetics-journey-step-locator-override-btn")).trigger("click"); + + const emitted = wrapper.emitted("update:locator"); + expect(emitted).toBeTruthy(); + expect(emitted![0][0]).toEqual({ + candidates: [], + user_override: { kind: "css", value: '[data-test="sign-in"]' }, + }); + }); + + it("keeps the override label, unmarked, when candidates exist", () => { + const wrapper = render(); + expect(wrapper.text()).toContain("Use a different locator"); + expect(wrapper.findComponent({ name: "OInput" }).props("required")).toBe(false); + }); +}); + +// Phase 4 / SE-8, D3. The kind is derived from the value, never picked — `kind` +// labels a locator, it does not parse it, so a picker that only set `kind` would +// store `{ kind: "role", value: "button" }` where `button` resolves as CSS. +describe("BrowserJourneyLocator derived kind", () => { + const typeOverride = async (wrapper: ReturnType, value: string) => { + await wrapper + .find(`${test("synthetics-journey-step-locator-override-input")} input`) + .setValue(value); + }; + + it("stores the kind read from the value, not css", async () => { + const wrapper = render(); + await typeOverride(wrapper, 'internal:role=button[name="Sign In"i]'); + await wrapper.find(test("synthetics-journey-step-locator-override-btn")).trigger("click"); + + const emitted = wrapper.emitted("update:locator")!; + expect((emitted[0][0] as StepLocator).user_override).toEqual({ + kind: "role", + value: 'internal:role=button[name="Sign In"i]', + }); + }); + + it("still stores css for a bare attribute selector", async () => { + const wrapper = render(); + await typeOverride(wrapper, '[data-qa="submit"]'); + await wrapper.find(test("synthetics-journey-step-locator-override-btn")).trigger("click"); + const emitted = wrapper.emitted("update:locator")!; + expect((emitted[0][0] as StepLocator).user_override?.kind).toBe("css"); + }); + + it("shows the derived kind as a read-only badge while typing", async () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-locator-derived-kind")).exists()).toBe(false); + await typeOverride(wrapper, "text=Sign in"); + expect(wrapper.find(test("synthetics-journey-step-locator-derived-kind")).text()).toBe("Text"); + }); + + it("prefills the override from a candidate without carrying its stored kind", async () => { + const wrapper = render(); + await wrapper + .find(test("synthetics-journey-step-locator-start-from-primary-btn")) + .trigger("click"); + const input = wrapper.find(`${test("synthetics-journey-step-locator-override-input")} input`); + // Primary candidate is a bare [data-test=…] stored as test_attribute; the badge + // describes what is in the box, which is CSS. Documented and intended (D3). + expect((input.element as HTMLInputElement).value).toBe('[data-test="login-sign-in"]'); + expect(wrapper.find(test("synthetics-journey-step-locator-derived-kind")).text()).toBe("CSS"); + }); +}); diff --git a/web/src/components/synthetics/journey/BrowserJourneyLocator.vue b/web/src/components/synthetics/journey/BrowserJourneyLocator.vue new file mode 100644 index 0000000000..1599bf7644 --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyLocator.vue @@ -0,0 +1,307 @@ + + + diff --git a/web/src/components/synthetics/journey/BrowserJourneyStep.spec.ts b/web/src/components/synthetics/journey/BrowserJourneyStep.spec.ts index 006dedebb5..1711745156 100644 --- a/web/src/components/synthetics/journey/BrowserJourneyStep.spec.ts +++ b/web/src/components/synthetics/journey/BrowserJourneyStep.spec.ts @@ -17,7 +17,7 @@ const OInputStub = { props: ["modelValue", "label", "placeholder", "type"], emits: ["update:modelValue"], template: - '', + '', }; const OSelectStub = { props: ["modelValue", "label", "options"], @@ -156,28 +156,25 @@ describe("BrowserJourneyStep", () => { expect(actionSelect.exists()).toBe(false); }); - it("should show selector fields for click action", () => { + // The v1 Selector-type + Selector pair is gone (SE-7 / SE-18): a version-2 + // step names its element through the locator bundle, and that is the only + // targeting UI. `stepNeedsTarget` decides when it renders. + it("should show the target block for click action", () => { wrapper = mountStep({ step: makeStep({ action: "click", selector: "#btn" }), expanded: true, }); - const selectorInput = wrapper.find('[data-test="synthetics-journey-step-selector-input"]'); - const selectorTypeSelect = wrapper.find( - '[data-test="synthetics-journey-step-selector-type-select"]', - ); - expect(selectorInput.exists()).toBe(true); - expect(selectorTypeSelect.exists()).toBe(true); + expect(wrapper.find('[data-test="synthetics-journey-step-locator"]').exists()).toBe(true); }); - it("should hide selector fields for navigate action", () => { + it("should hide the target block for navigate action", () => { wrapper = mountStep({ step: makeStep({ action: "navigate", value: "https://example.com" }), expanded: true, }); - const selectorInput = wrapper.find('[data-test="synthetics-journey-step-selector-input"]'); - expect(selectorInput.exists()).toBe(false); + expect(wrapper.find('[data-test="synthetics-journey-step-locator"]').exists()).toBe(false); }); it("should show value input for type action", () => { @@ -200,6 +197,90 @@ describe("BrowserJourneyStep", () => { expect(valueInput.exists()).toBe(true); }); + // ── Timeout guard rails (spec P1.1.4, P1.1.5 / T1-13, T1-14) ─────────── + // The recorder no longer stamps a timeout, so this field renders empty. An + // empty box reads as "no timeout", which is wrong and invites needless + // overrides — show the default the runner will actually apply. + describe("timeout guard rails", () => { + const timeoutInput = (w: VueWrapper) => + w.find('[data-test="synthetics-journey-step-timeout-input"]'); + const warning = (w: VueWrapper) => + w.find('[data-test="synthetics-journey-step-timeout-warning"]'); + + // SE-5 moved the timeout behind the one `Advanced` collapsible, which + // opens itself only when the step carries a non-default. A step with no + // explicit timeout is exactly the case that stays closed, and + // OCollapsible unmounts collapsed content — so these cases have to open + // it before the field is in the DOM. + const openAdvanced = (w: VueWrapper) => + w.find('[data-test="synthetics-journey-step-group-advanced"] button').trigger("click"); + + it("shows the 60s navigate/assert default as placeholder when unset", async () => { + wrapper = mountStep({ + step: makeStep({ action: "navigate", timeout: undefined }), + expanded: true, + }); + await openAdvanced(wrapper); + expect(timeoutInput(wrapper).attributes("placeholder")).toBe("60000"); + }); + + it("shows the 30s interaction default as placeholder when unset", async () => { + wrapper = mountStep({ + step: makeStep({ action: "click", timeout: undefined }), + expanded: true, + }); + await openAdvanced(wrapper); + expect(timeoutInput(wrapper).attributes("placeholder")).toBe("30000"); + }); + + it("warns when the author lowers the timeout below the category default", () => { + wrapper = mountStep({ + step: makeStep({ action: "click", timeout: 5000 }), + expanded: true, + }); + expect(warning(wrapper).exists()).toBe(true); + }); + + it("does not warn at or above the category default", () => { + wrapper = mountStep({ + step: makeStep({ action: "click", timeout: 30000 }), + expanded: true, + }); + expect(warning(wrapper).exists()).toBe(false); + }); + + // Opens `Advanced` first, or the absence proves nothing: an unset timeout + // is the one case that leaves the section collapsed, so the warning would + // be missing whether or not the component wanted to render it. + it("does not warn when no explicit timeout is set", async () => { + wrapper = mountStep({ + step: makeStep({ action: "click", timeout: undefined }), + expanded: true, + }); + await openAdvanced(wrapper); + expect(timeoutInput(wrapper).exists()).toBe(true); + expect(warning(wrapper).exists()).toBe(false); + }); + + // The warning is advisory. Lowering is the author's call — it must never + // block, only inform. + it("keeps the timeout editable while warning", () => { + wrapper = mountStep({ + step: makeStep({ action: "click", timeout: 5000 }), + expanded: true, + }); + expect(timeoutInput(wrapper).attributes("disabled")).toBeUndefined(); + }); + }); + + // ── Retired actions (spec X-9 / T1-9) ────────────────────────────────── + // T1-9 is "the editor no longer OFFERS the four retired actions" — the + // picker filter, covered by constants/synthetics.spec.ts. The per-step + // notice this file used to assert was deleted deliberately: `actionOptions` + // filters RETIRED_ACTIONS out, the recorder never emits one, and no v1 + // journeys exist, so nothing could reach it — and it named no replacement. + // Its absence is pinned by BrowserJourneyStepEditor.spec.ts. + it("should show timeout input when expanded", () => { wrapper = mountStep({ step: makeStep(), expanded: true }); @@ -403,4 +484,171 @@ describe("BrowserJourneyStep", () => { expect(dot.exists()).toBe(true); }); }); + + // ── Value inputs per action ─────────────────────────────────────────────── + describe("value input", () => { + const valueInput = () => wrapper.find('[data-test="synthetics-journey-step-value-input"]'); + + it("should render the file path input for an upload step", () => { + wrapper = mountStep({ + step: makeStep({ action: "upload", value: "/tmp/a.pdf" }), + expanded: true, + }); + + expect(valueInput().exists()).toBe(true); + expect(valueInput().attributes("value")).toBe("/tmp/a.pdf"); + }); + + it("should render the option input for a select step", () => { + wrapper = mountStep({ + step: makeStep({ action: "select", value: "India" }), + expanded: true, + }); + + expect(valueInput().attributes("value")).toBe("India"); + }); + + it("should not render a generic value input for an assert step", () => { + // BrowserJourneyAssertion owns the expected value; a second input took + // typing and had it dropped at save (buildV2Steps drops `value` on assert). + wrapper = mountStep({ + step: makeStep({ action: "assert", assertion: { kind: "element_text", expected: "Hi" } }), + expanded: true, + }); + + expect(valueInput().exists()).toBe(false); + }); + }); + + // ── Wire round-trip on edit ─────────────────────────────────────────────── + describe("value edits reach the replayed wire step", () => { + function editedWire(step: BrowserStep, newValue: string) { + wrapper = mountStep({ step, expanded: true }); + const input = wrapper.find('[data-test="synthetics-journey-step-value-input"]'); + input.setValue(newValue); + const emitted = wrapper.emitted("update:step") as BrowserStep[][]; + return emitted[0][0].wire!; + } + + it("should write an edited navigate URL to wire.url, not wire.value", async () => { + const step = makeStep({ + action: "navigate", + value: "https://old.test", + wire: { id: "step-1", action: "navigate", url: "https://old.test", pageAlias: "page" }, + }); + + const wire = editedWire(step, "https://new.test"); + + expect(wire.url).toBe("https://new.test"); + expect(wire.pageAlias).toBe("page"); // extension metadata survives the edit + }); + + it("should write an edited press key to wire.key", async () => { + const step = makeStep({ + action: "press", + value: "Enter", + wire: { id: "step-1", action: "press", key: "Enter" }, + }); + + expect(editedWire(step, "Tab").key).toBe("Tab"); + }); + + it("should write an edited select option to wire.options", async () => { + const step = makeStep({ + action: "select", + value: "India", + wire: { id: "step-1", action: "select", options: ["India"] }, + }); + + expect(editedWire(step, "Japan").options).toEqual(["Japan"]); + }); + }); + + // ── Settle block (spec P4.1.5, P3.4.3) ──────────────────────────────────── + describe("settle", () => { + const settleStep = () => + makeStep({ + action: "click", + settle: { + navigation: { url_pattern: "**/web/**" }, + responses: [{ url_pattern: "**/api/login", method: "POST", required: false }], + observed_duration_ms: 1200, + budget_ms: 5000, + }, + wire: { id: "step-1", action: "click" }, + }); + + it("should let the author mark a recorded response as required", async () => { + wrapper = mountStep({ step: settleStep(), expanded: true }); + + const checkbox = wrapper.find('[data-test="synthetics-journey-step-settle-required-0"]'); + expect(checkbox.exists()).toBe(true); + + await checkbox.setValue(true); + + const emitted = wrapper.emitted("update:step") as BrowserStep[][]; + const next = emitted[0][0]; + expect(next.settle?.responses?.[0].required).toBe(true); + // ...and it travels with the replayed step, not just the saved one. + expect(next.wire?.settle?.responses?.[0].required).toBe(true); + }); + + it("should show the settle budget and let the author change it", async () => { + wrapper = mountStep({ step: settleStep(), expanded: true }); + + const budget = wrapper.find('[data-test="synthetics-journey-step-settle-budget-input"]'); + expect(budget.attributes("value")).toBe("5000"); + + await budget.setValue("12000"); + + const emitted = wrapper.emitted("update:step") as BrowserStep[][]; + expect(emitted[0][0].settle?.budget_ms).toBe(12000); + }); + + it("should drop the budget when the field is cleared", async () => { + wrapper = mountStep({ step: settleStep(), expanded: true }); + + await wrapper.find('[data-test="synthetics-journey-step-settle-budget-input"]').setValue(""); + + const emitted = wrapper.emitted("update:step") as BrowserStep[][]; + const next = emitted[0][0]; + expect(next.settle?.budget_ms).toBeUndefined(); + expect(next.settle?.navigation?.url_pattern).toBe("**/web/**"); // evidence kept + }); + + it("should warn when the budget falls outside the server-accepted range", () => { + const step = settleStep(); + step.settle!.budget_ms = 90000; + wrapper = mountStep({ step, expanded: true }); + + expect( + wrapper.find('[data-test="synthetics-journey-step-settle-budget-warning"]').exists(), + ).toBe(true); + }); + + it("should not warn for a budget inside the range", () => { + wrapper = mountStep({ step: settleStep(), expanded: true }); + + expect( + wrapper.find('[data-test="synthetics-journey-step-settle-budget-warning"]').exists(), + ).toBe(false); + }); + + // Only the recorded lines are conditional. The block itself holds the budget + // input, which is the ONLY way to create a budget — gating it on "has settle + // data" made it unreachable on a hand-added step (SE-16). This asserted the + // wrapper's absence, which happened to hold while the settle fields sat in + // their own collapsed group; merging that group into `Advanced` means a step + // carrying any non-default (this fixture sets `timeout`) opens it. + it("should render no recorded evidence when the step has none", () => { + wrapper = mountStep({ step: makeStep({ action: "click" }), expanded: true }); + + expect(wrapper.find('[data-test="synthetics-journey-step-settle"]').text()).not.toContain( + "Waits for (recorded)", + ); + expect( + wrapper.find('[data-test="synthetics-journey-step-settle-budget-input"]').exists(), + ).toBe(true); + }); + }); }); diff --git a/web/src/components/synthetics/journey/BrowserJourneyStep.vue b/web/src/components/synthetics/journey/BrowserJourneyStep.vue index e55021dae6..8b051af592 100644 --- a/web/src/components/synthetics/journey/BrowserJourneyStep.vue +++ b/web/src/components/synthetics/journey/BrowserJourneyStep.vue @@ -3,16 +3,8 @@ import { computed, ref } from "vue"; import { useI18n } from "vue-i18n"; import { copyToClipboard } from "@/utils/clipboard"; -import type { BrowserStep, SelectorType, StepReplayResult, WireStep } from "@/types/synthetics"; -import { - ACTION_ICONS, - ACTION_LABELS, - SELECTOR_ACTIONS, - VALUE_ACTIONS, - VALUE_LABELS, - SELECTOR_TYPE_OPTIONS, - actionOptions, -} from "@/constants/synthetics"; +import type { BrowserStep, StepReplayResult } from "@/types/synthetics"; +import { ACTION_ICONS, ACTION_LABELS } from "@/constants/synthetics"; /** * Replay status dot states. @@ -21,12 +13,11 @@ import { */ export type StepDotState = "pending" | "active" | "pass" | "fail" | "skip"; import OButton from "@/lib/core/Button/OButton.vue"; -import OInput from "@/lib/forms/Input/OInput.vue"; -import OSelect from "@/lib/forms/Select/OSelect.vue"; import OIcon from "@/lib/core/Icon/OIcon.vue"; import OBadge from "@/lib/core/Badge/OBadge.vue"; import OCheckbox from "@/lib/forms/Checkbox/OCheckbox.vue"; import OSpinner from "@/lib/feedback/Spinner/OSpinner.vue"; +import BrowserJourneyStepEditor from "./BrowserJourneyStepEditor.vue"; const { t } = useI18n(); @@ -55,64 +46,20 @@ const emit = defineEmits<{ }>(); // ── Computed from shared constants ────────────────────────────────── -const selectorTypeOptions = SELECTOR_TYPE_OPTIONS; const actionIcon = computed(() => ACTION_ICONS[props.step.action]); const actionLabel = computed(() => ACTION_LABELS[props.step.action]); const displayName = computed(() => props.step.name || actionLabel.value); -const selectorPreview = computed(() => props.step.selector || props.step.value || ""); -const showSelector = computed(() => SELECTOR_ACTIONS.includes(props.step.action)); -const showValue = computed(() => VALUE_ACTIONS.includes(props.step.action)); -const valueLabel = computed( - () => VALUE_LABELS[props.step.action] || t("synthetics.journey.valueFallback"), -); - -function update(patch: Partial) { - // Patch edited fields into wire instead of clearing it, so replay still has - // the original extension metadata (framePath, pageAlias, position, snapshot). - // Action changes clear wire since the step type fundamentally changed. - let wire = props.step.wire ? { ...props.step.wire } : undefined; - if (wire) { - if (patch.name !== undefined) wire.name = patch.name; - if (patch.selector !== undefined) wire.selector = patch.selector; - if (patch.selectorType !== undefined) - wire.selector_type = patch.selectorType.toLowerCase() as WireStep["selector_type"]; - if (patch.value !== undefined) wire.value = patch.value; - if (patch.timeout !== undefined) wire.timeout_ms = patch.timeout; - if (patch.action !== undefined) wire = undefined; // action changed — wire metadata is no longer accurate - } - emit("update:step", { ...props.step, wire, ...patch }); -} - -// Computed getters/setters for inline editor fields -const actionComputed = computed({ - get: () => props.step.action, - set: (v: BrowserStep["action"]) => update({ action: v }), -}); - -const nameComputed = computed({ - get: () => props.step.name ?? "", - set: (v: string) => update({ name: v }), -}); - -const selectorTypeComputed = computed({ - get: () => props.step.selectorType ?? "CSS", - set: (v: string | number | boolean | null | undefined) => - update({ selectorType: (v as SelectorType) ?? undefined }), -}); - -const selectorComputed = computed({ - get: () => props.step.selector ?? "", - set: (v: string) => update({ selector: v }), -}); - -const valueComputed = computed({ - get: () => props.step.value ?? "", - set: (v: string) => update({ value: v }), -}); - -const timeoutComputed = computed({ - get: () => String(props.step.timeout ?? ""), - set: (v: string) => update({ timeout: v ? Number(v) : undefined }), +/** + * What the collapsed row shows on the right. + * + * A version-2 step has no bare `selector` — its identity is the locator bundle — + * so reading `selector` alone would leave every recorded step's row blank. The + * pin wins when there is one, because that is what the run will actually use. + */ +const selectorPreview = computed(() => { + const locator = props.step.locator; + const effective = locator?.user_override ?? locator?.candidates?.[0]; + return effective?.value || props.step.selector || props.step.value || ""; }); // ── Status dot visual mapping (combines with step number during replay) ───── @@ -406,62 +353,13 @@ function toggleExpanded() {
- -
- - - - - - - - - - - - - - -
+ + diff --git a/web/src/components/synthetics/journey/BrowserJourneyStepEditor.spec.ts b/web/src/components/synthetics/journey/BrowserJourneyStepEditor.spec.ts new file mode 100644 index 0000000000..3374ffc5dc --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyStepEditor.spec.ts @@ -0,0 +1,392 @@ +// Copyright 2026 OpenObserve Inc. + +import { describe, expect, it } from "vitest"; +import { mount } from "@vue/test-utils"; +import { createI18n } from "vue-i18n"; +import type { BrowserStep } from "@/types/synthetics"; +import BrowserJourneyStepEditor from "./BrowserJourneyStepEditor.vue"; +import en from "@/locales/languages/en-US.json"; + +const i18n = createI18n({ + legacy: false, + locale: "en-US", + fallbackLocale: "en-US", + messages: { "en-US": en as Record }, +}); + +function render(step: Partial = {}) { + const full: BrowserStep = { + id: "s1", + action: "click", + name: "Sign in", + code: "", + locator: { candidates: [], user_override: null }, + ...step, + }; + return mount(BrowserJourneyStepEditor, { + props: { step: full }, + global: { plugins: [i18n] }, + }); +} + +const test = (name: string) => `[data-test="${name}"]`; + +// The locator bundle is the only way a v2 step names its element. The v1 +// Selector-type + Selector pair is gone: no v1 journeys exist, so the fork it +// served could not occur, and keeping it produced two unrelated targeting +// editors in one journey (SE-7) plus a silent steps_version downgrade (SE-18). +describe("BrowserJourneyStepEditor targeting", () => { + it("renders the locator block, never the v1 selector pair", () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-locator")).exists()).toBe(true); + expect(wrapper.find(test("synthetics-journey-step-selector-type-select")).exists()).toBe(false); + expect(wrapper.find(test("synthetics-journey-step-selector-input")).exists()).toBe(false); + }); + + it("renders the locator block for a recorded step too", () => { + const wrapper = render({ + locator: { candidates: [{ kind: "test_attribute", value: '[data-test="x"]' }] }, + }); + expect(wrapper.find(test("synthetics-journey-step-locator")).exists()).toBe(true); + }); + + // SE-1: the form and the validator now share one predicate, so the form can + // neither demand a target the validator ignores nor omit one it requires. + it("renders no target block for a page-level assertion", () => { + const wrapper = render({ + action: "assert", + assertion: { kind: "url_matches", expected: "**/web/**" }, + }); + expect(wrapper.find(test("synthetics-journey-step-locator")).exists()).toBe(false); + }); + + it("renders the target block for an element-level assertion", () => { + const wrapper = render({ action: "assert", assertion: { kind: "element_visible" } }); + expect(wrapper.find(test("synthetics-journey-step-locator")).exists()).toBe(true); + }); + + it("renders no target block for navigate", () => { + const wrapper = render({ action: "navigate", value: "https://example.com" }); + expect(wrapper.find(test("synthetics-journey-step-locator")).exists()).toBe(false); + }); +}); + +// `hover` is retired. It can no longer reach a step — actionOptions filters +// RETIRED_ACTIONS out of the picker, the recorder has never emitted one +// (upstream's ActionName omits them entirely), and no v1 journeys exist, which +// was the only other way one could have entered a journey. The notice also named +// no replacement, so it told an author to fix something without saying how. +describe("BrowserJourneyStepEditor retired actions", () => { + it("renders no retired-action notice, even for a legacy action value", () => { + const wrapper = render({ action: "hover" }); + expect(wrapper.find(test("synthetics-journey-step-retired-action")).exists()).toBe(false); + }); +}); + +// The host parses with zod and pushes the resulting issues back down as per-field +// messages. Before this, validation was save-time and toast-only, so a failure +// named no field (SE-3). +describe("BrowserJourneyStepEditor inline field errors", () => { + function renderWithErrors(step: Partial, errors: Record) { + const full: BrowserStep = { + id: "s1", + action: "click", + name: "Sign in", + code: "", + locator: { candidates: [], user_override: null }, + ...step, + }; + return mount(BrowserJourneyStepEditor, { + props: { step: full, ...errors }, + global: { plugins: [i18n] }, + }); + } + + it("renders a name error on the name field", () => { + const wrapper = renderWithErrors({}, { nameErrorMessage: "Give this step a name" }); + expect(wrapper.find(test("synthetics-journey-step-name-input")).text()).toContain( + "Give this step a name", + ); + }); + + it("renders a value error on the value field", () => { + const wrapper = renderWithErrors( + { action: "type", value: "" }, + { valueErrorMessage: "Enter the text this step should type" }, + ); + expect(wrapper.find(test("synthetics-journey-step-value-input")).text()).toContain( + "Enter the text this step should type", + ); + }); + + it("renders an expected error on the assertion's expected field", () => { + const wrapper = renderWithErrors( + { action: "assert", assertion: { kind: "element_text", expected: "" } }, + { expectedErrorMessage: "Enter the value this assertion should expect" }, + ); + expect(wrapper.find(test("synthetics-journey-step-assertion-expected-input")).text()).toContain( + "Enter the value this assertion should expect", + ); + }); + + it("renders no error text when the host passes none", () => { + const wrapper = renderWithErrors({}, {}); + expect(wrapper.find(test("synthetics-journey-step-name-input")).text()).not.toContain( + "Give this step a name", + ); + }); +}); + +// Phase 2 / SE-5, revised. Two tiers, not three peer collapsibles: what the step +// DOES is plain always-visible markup, and everything a recording or a runner +// default already answers sits behind one `Advanced` collapsible. +// +// The three-group shape charged the same price for the step's identity as for its +// tuning — and wrapped always-visible content in a `default-open` collapsible, +// which is a control whose only effect is to let an author close what they need. +describe("BrowserJourneyStepEditor field layout", () => { + const groups = (wrapper: ReturnType) => + wrapper + .findAll('[data-test^="synthetics-journey-step-group-"]') + .map((g) => g.attributes("data-test")); + + it("renders the always-visible block and exactly one collapsible", () => { + const wrapper = render(); + expect(groups(wrapper)).toEqual([ + "synthetics-journey-step-group-does", + "synthetics-journey-step-group-advanced", + ]); + }); + + // OCollapsible unmounts collapsed content, so `Advanced` must be opened before + // its fields are in the DOM — it being closed by default is the point. + async function openAdvanced(wrapper: ReturnType) { + await wrapper.find(`${test("synthetics-journey-step-group-advanced")} button`).trigger("click"); + } + + it("needs no click to reach action, name, target and value", () => { + const wrapper = render({ action: "type", value: "hunter2" }); + const visible = wrapper.find(test("synthetics-journey-step-group-does")); + expect(visible.find(test("synthetics-journey-step-action-select")).exists()).toBe(true); + expect(visible.find(test("synthetics-journey-step-name-input")).exists()).toBe(true); + expect(visible.find(test("synthetics-journey-step-locator")).exists()).toBe(true); + expect(visible.find(test("synthetics-journey-step-value-input")).exists()).toBe(true); + }); + + // One disclosure in the whole editor. The locator block used to add a second + // (its "N fallbacks" collapse), which nested two levels of hiding over one short + // read-only list. + it("renders one disclosure region in the whole editor", () => { + const wrapper = render({ + locator: { + candidates: [ + { kind: "test_attribute", value: '[data-test="a"]' }, + { kind: "role", value: "role=button" }, + ], + }, + }); + expect(wrapper.findAll('[data-test="o-collapsible-content"]').length).toBe(1); + }); + + // SE-16: the settle fields used to be gated on hasSettle, so a hand-added step + // could never be given a budget — the field that creates one was hidden until + // one existed. + it("reaches the settle budget on a hand-added step with no settle data", async () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-settle-budget-input")).exists()).toBe(false); + await openAdvanced(wrapper); + expect(wrapper.find(test("synthetics-journey-step-settle-budget-input")).exists()).toBe(true); + }); + + it("puts settling, timeout and flow control in Advanced", async () => { + const wrapper = render(); + await openAdvanced(wrapper); + const advanced = wrapper.find(test("synthetics-journey-step-group-advanced")); + expect(advanced.find(test("synthetics-journey-step-settle-budget-input")).exists()).toBe(true); + expect(advanced.find(test("synthetics-journey-step-timeout-input")).exists()).toBe(true); + expect(advanced.find(test("synthetics-journey-step-optional-checkbox")).exists()).toBe(true); + expect(advanced.find(test("synthetics-journey-step-always-run-checkbox")).exists()).toBe(true); + }); + + it("opens Advanced already when the step holds a non-default value", () => { + const wrapper = render({ optional: true }); + expect(wrapper.find(test("synthetics-journey-step-timeout-input")).exists()).toBe(true); + }); + + it("captions Advanced with what it is for when the step holds only defaults", () => { + const wrapper = render(); + const caption = wrapper.find(test("synthetics-journey-step-group-advanced")).text(); + expect(caption).toContain("Page settling"); + expect(caption).not.toMatch(/Optional|Always run|Timeout \d/); + }); + + it("captions Advanced with each non-default value instead", () => { + const wrapper = render({ optional: true, alwaysRun: true, timeout: 10000 }); + const caption = wrapper.find(test("synthetics-journey-step-group-advanced")).text(); + expect(caption).toContain("Optional"); + expect(caption).toContain("Always run"); + expect(caption).toContain("10"); + expect(caption).not.toContain("Page settling"); + }); + + it("captions Advanced when settle evidence was recorded", () => { + const wrapper = render({ + settle: { navigation: { url_pattern: "**/home" }, observed_duration_ms: 1200 }, + }); + expect(wrapper.find(test("synthetics-journey-step-group-advanced")).text()).toContain( + "recorded", + ); + }); +}); + +// Phase 2 / SE-15. The configure forms size fields with flex, not fixed widths; +// the step editor was the outlier with `!important` overrides that defeated reflow. +describe("BrowserJourneyStepEditor layout", () => { + it("uses no !important width overrides", () => { + const wrapper = render({ action: "type", value: "x" }); + const offenders = wrapper + .findAll("*") + .map((n) => n.attributes("class") ?? "") + .filter((c) => /\bw-\d+!/.test(c)); + expect(offenders).toEqual([]); + }); +}); + +// Phase 3 / SE-6, SE-9, SE-20. +describe("BrowserJourneyStepEditor plain language", () => { + it("leads with a sentence describing the step", () => { + const wrapper = render({ + action: "click", + locator: { candidates: [{ kind: "test_attribute", value: '[data-test="sign-in"]' }] }, + }); + const summary = wrapper.find(test("synthetics-journey-step-summary")).text(); + expect(summary).toContain("Click"); + expect(summary).toContain('[data-test="sign-in"]'); + // the effective timeout, not a raw ms number + expect(summary).toContain("30"); + }); + + it("uses the pinned locator in the summary, since that is what runs", () => { + const wrapper = render({ + locator: { + candidates: [{ kind: "css", value: ".ignored" }], + user_override: { kind: "css", value: "#pinned" }, + }, + }); + expect(wrapper.find(test("synthetics-journey-step-summary")).text()).toContain("#pinned"); + }); + + it("reflects an explicit timeout in the summary", () => { + const wrapper = render({ timeout: 5000 }); + expect(wrapper.find(test("synthetics-journey-step-summary")).text()).toContain("5"); + }); + + it("names the runner default in the timeout helper for an interaction", async () => { + const wrapper = render(); + await wrapper.find(`${test("synthetics-journey-step-group-advanced")} button`).trigger("click"); + const help = wrapper.find(test("synthetics-journey-step-timeout-help")).text(); + expect(help).toContain("30"); + expect(help).toContain("Maximum 60"); + }); + + // SE-20: on navigate/assert the category default IS the server maximum, so the + // field can only shorten — saying so stops the below-default warning reading as + // a malfunction. + it("says the field can only shorten on navigate, where default equals the maximum", async () => { + const wrapper = render({ action: "navigate", value: "https://example.com" }); + await wrapper.find(`${test("synthetics-journey-step-group-advanced")} button`).trigger("click"); + const help = wrapper.find(test("synthetics-journey-step-timeout-help")).text(); + expect(help).toContain("60"); + expect(help).toMatch(/only shorten/i); + }); + + it("renames the Playwright terms out of the visible copy", () => { + const wrapper = render({ + locator: { + candidates: [ + { kind: "css", value: "#a" }, + { kind: "css", value: "#b" }, + ], + }, + }); + const txt = wrapper.text(); + expect(txt).toContain("How to find this element"); + expect(txt).toContain("Always use this one"); + expect(txt).not.toMatch(/\bLocator\b/); + }); +}); + +// Phase 4 / SE-13, D11. Both flow-control flags are fully implemented in the probe +// with semantics the labels omit. Both-set is legitimate — run during cleanup, and +// if it fails do not fail the run — so this explains rather than prevents. +describe("BrowserJourneyStepEditor flow-control help", () => { + async function openAdvanced(wrapper: ReturnType) { + await wrapper.find(`${test("synthetics-journey-step-group-advanced")} button`).trigger("click"); + } + + it("attaches an info tooltip to each flag", async () => { + const wrapper = render(); + await openAdvanced(wrapper); + expect(wrapper.find(test("synthetics-journey-step-optional-help")).exists()).toBe(true); + expect(wrapper.find(test("synthetics-journey-step-always-run-help")).exists()).toBe(true); + }); + + it("explains the probe behaviour the labels omit", async () => { + const wrapper = render(); + await openAdvanced(wrapper); + const tips = wrapper.findAllComponents({ name: "OTooltip" }).map((c) => c.props("content")); + const optional = tips.find((c) => /Skipped/i.test(String(c))); + const always = tips.find((c) => /cleanup/i.test(String(c))); + expect(optional).toMatch(/never fails the run/i); + expect(always).toMatch(/after the failed one/i); + }); + + it("leaves both flags independently settable — the combination is legal", async () => { + const wrapper = render({ optional: true, alwaysRun: true }); + const optional = wrapper.find(test("synthetics-journey-step-optional-checkbox")); + const always = wrapper.find(test("synthetics-journey-step-always-run-checkbox")); + expect(optional.attributes("disabled")).toBeUndefined(); + expect(always.attributes("disabled")).toBeUndefined(); + }); +}); + +// Phase 5c / SE-11, D9. Discarding the wire on an action change is correct — its +// payload belongs to the old action — but it used to happen in silence. +// +// Scope note: after the storage path stopped preserving `wire` (SE-24), a step +// loaded from a saved monitor carries none, so there is nothing to discard and the +// notice correctly stays silent. It applies to a live recording session, which is +// the only place a wire is still present. +describe("BrowserJourneyStepEditor action-change notice", () => { + const selectAction = async (wrapper: ReturnType, action: string) => { + await wrapper.findComponent({ name: "OSelect" }).vm.$emit("update:modelValue", action); + }; + + it("says the step is rebuilt when a recorded wire is discarded", async () => { + const wrapper = render({ wire: { id: "w1", action: "click" } as never }); + expect(wrapper.find(test("synthetics-journey-step-action-changed-notice")).exists()).toBe( + false, + ); + + await selectAction(wrapper, "navigate"); + expect(wrapper.find(test("synthetics-journey-step-action-changed-notice")).text()).toMatch( + /rebuilds this step/i, + ); + }); + + it("stays silent for a stored step, which carries no wire to lose", async () => { + const wrapper = render(); // no `wire` — the shape mapWireSteps now produces + await selectAction(wrapper, "navigate"); + expect(wrapper.find(test("synthetics-journey-step-action-changed-notice")).exists()).toBe( + false, + ); + }); + + it("stays silent when the action is re-selected unchanged", async () => { + const wrapper = render({ wire: { id: "w1", action: "click" } as never }); + await selectAction(wrapper, "click"); + expect(wrapper.find(test("synthetics-journey-step-action-changed-notice")).exists()).toBe( + false, + ); + }); +}); diff --git a/web/src/components/synthetics/journey/BrowserJourneyStepEditor.vue b/web/src/components/synthetics/journey/BrowserJourneyStepEditor.vue new file mode 100644 index 0000000000..a9bcc5617c --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyStepEditor.vue @@ -0,0 +1,579 @@ + + + diff --git a/web/src/components/synthetics/journey/BrowserJourneyStepError.spec.ts b/web/src/components/synthetics/journey/BrowserJourneyStepError.spec.ts new file mode 100644 index 0000000000..cb72c0ba37 --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyStepError.spec.ts @@ -0,0 +1,111 @@ +// Copyright 2026 OpenObserve Inc. + +import { describe, expect, it } from "vitest"; +import { mount } from "@vue/test-utils"; +import { createI18n } from "vue-i18n"; +import type { StepReplayResult } from "@/types/synthetics"; +import BrowserJourneyStepError from "./BrowserJourneyStepError.vue"; +import en from "@/locales/languages/en-US.json"; + +const i18n = createI18n({ + legacy: false, + locale: "en-US", + fallbackLocale: "en-US", + messages: { "en-US": en as Record }, +}); + +const test = (name: string) => `[data-test="${name}"]`; + +function render(result: Partial = {}, stepNumber?: number) { + const full: StepReplayResult = { + stepId: "s3", + stepName: "Click sign in", + passed: false, + durationMs: 30_000, + error: "locator.click: Timeout 30000ms exceeded.", + ...result, + }; + return mount(BrowserJourneyStepError, { + props: { result: full, stepNumber }, + global: { plugins: [i18n] }, + }); +} + +// SE-4. This evidence was computed and thrown away: when JourneySteps replaced the +// old row component the error card was not carried across, so a failed replay showed +// only a red dot and a one-line journey banner. +describe("BrowserJourneyStepError", () => { + it("shows the error message", () => { + const wrapper = render(); + expect(wrapper.find(test("synthetics-journey-step-error-message")).text()).toContain( + "Timeout 30000ms exceeded", + ); + }); + + it("prefers the structured error's message over the raw string", () => { + const wrapper = render({ + structuredError: { name: "TimeoutError", message: "waiting for locator" } as never, + }); + expect(wrapper.find(test("synthetics-journey-step-error-message")).text()).toContain( + "waiting for locator", + ); + }); + + it("names the exit reason and the duration", () => { + const wrapper = render({ + structuredError: { name: "TimeoutError", message: "x" } as never, + }); + const card = wrapper.find(test("synthetics-journey-step-error-card")).text(); + expect(card).toMatch(/timeout/i); + expect(card).toContain("30.0 s"); + }); + + it("shows the selector the runner could not act on", () => { + const wrapper = render({ + structuredError: { + name: "TimeoutError", + message: "x", + selector: '[data-test="sign-in"]', + } as never, + }); + expect(wrapper.find(test("synthetics-journey-step-error-selector")).text()).toBe( + '[data-test="sign-in"]', + ); + }); + + // X-8.2 — "A step the player skipped MUST NOT render as a pass. Silent divergence + // is the failure mode this whole section exists to prevent." + it("renders the player's fidelity notes", () => { + const wrapper = render({ + fidelity: { + level: "reduced", + notes: [ + "primary locator only", + 'Flow control not simulated: the preview stops at the first failure regardless of "optional" or "always run".', + ], + }, + }); + const fidelity = wrapper.find(test("synthetics-journey-step-fidelity")); + expect(fidelity.exists()).toBe(true); + expect(fidelity.text()).toContain("primary locator only"); + expect(fidelity.text()).toContain("Flow control not simulated"); + }); + + it("omits the fidelity block when the player reported nothing", () => { + expect(render().find(test("synthetics-journey-step-fidelity")).exists()).toBe(false); + }); + + // SE-4: the old button said "Re-run" inside a per-step card but replayed the whole + // journey. A single step is not independently runnable, so the honest affordance + // names the prefix it will actually run. + it("names the prefix the re-run will execute", () => { + const wrapper = render({}, 3); + expect(wrapper.find(test("synthetics-journey-error-retry-btn")).text()).toContain("1–3"); + }); + + it("emits retry-replay when the re-run is clicked", async () => { + const wrapper = render({}, 3); + await wrapper.find(test("synthetics-journey-error-retry-btn")).trigger("click"); + expect(wrapper.emitted("retry-replay")).toBeTruthy(); + }); +}); diff --git a/web/src/components/synthetics/journey/BrowserJourneyStepError.vue b/web/src/components/synthetics/journey/BrowserJourneyStepError.vue new file mode 100644 index 0000000000..b44e5ad5f0 --- /dev/null +++ b/web/src/components/synthetics/journey/BrowserJourneyStepError.vue @@ -0,0 +1,151 @@ +// Copyright 2026 OpenObserve Inc. + + + + diff --git a/web/src/components/synthetics/journey/RecordJourney.spec.ts b/web/src/components/synthetics/journey/RecordJourney.spec.ts deleted file mode 100644 index 3e0b335c94..0000000000 --- a/web/src/components/synthetics/journey/RecordJourney.spec.ts +++ /dev/null @@ -1,287 +0,0 @@ -// Copyright 2026 OpenObserve Inc. - -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { mount, VueWrapper, flushPromises } from "@vue/test-utils"; - -// ── Stubs ────────────────────────────────────────────────────────────────── -const OButtonStub = { - template: '', -}; -const OIconStub = { - template: '', -}; -const OSpinnerStub = { - template: '