From 43470ca63f0a12d51811d70c5b22735384b33af4 Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Thu, 23 Jul 2026 14:54:32 -0700 Subject: [PATCH 1/7] Update listener --- services/hackbot-pulse-listener/README.md | 59 +++-- services/hackbot-pulse-listener/app/client.py | 9 +- services/hackbot-pulse-listener/app/config.py | 7 + .../hackbot-pulse-listener/app/consumer.py | 192 +++++++++++++- .../hackbot-pulse-listener/app/failures.py | 54 ++++ .../hackbot-pulse-listener/app/flakiness.py | 195 ++++++++++++++ services/hackbot-pulse-listener/app/models.py | 6 +- services/hackbot-pulse-listener/app/notify.py | 145 ++++++++++- .../hackbot-pulse-listener/app/regression.py | 166 ++++++------ services/hackbot-pulse-listener/deploy.sh | 3 + .../tests/fixtures/xpcshell-bucket.json | 29 +++ .../tests/test_consumer.py | 239 +++++++++++++++++- .../tests/test_failures.py | 42 +++ .../tests/test_flakiness.py | 85 +++++++ .../tests/test_notify.py | 130 ++++++++++ .../tests/test_regression_test_failure.py | 122 +++++++++ 16 files changed, 1343 insertions(+), 140 deletions(-) create mode 100644 services/hackbot-pulse-listener/app/failures.py create mode 100644 services/hackbot-pulse-listener/app/flakiness.py create mode 100644 services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json create mode 100644 services/hackbot-pulse-listener/tests/test_failures.py create mode 100644 services/hackbot-pulse-listener/tests/test_flakiness.py create mode 100644 services/hackbot-pulse-listener/tests/test_regression_test_failure.py diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index 0935faf895..1bdf0b812b 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -1,27 +1,33 @@ # Hackbot Pulse Listener -Listens to Taskcluster build-failure pulse messages, and for failed **Firefox build -tasks** triggers the `build-repair` hackbot agent through the hackbot-api. When a run -finishes (minutes later) it emails a link to the hackbot UI, a summary of the analysis -and fix, a Treeherder link to the failing job, and the commit the agent blamed for the -failure. The email's primary recipient is the author of that blamed commit. +Its job is to **subscribe** to Taskcluster failure messages, **filter** them down to +failures worth acting on, **dedupe** them, and dispatch a hackbot agent through the +hackbot-api. It deliberately holds no investigation logic: each agent resolves the push +and commits itself, so the listener only decides _what to hand off_. When a run finishes +(minutes later) the listener polls the result and emails a report. + +Failed **build** tasks go to `build-repair`; failed **test** tasks go to +`test-repair` (test-repair). ## How it works -1. Consume `task-failed` messages from `pulse.mozilla.org`. -2. Keep only **build-kind** tasks (`tags.kind == "build"`) on a watched `project` - (`WATCHED_REPOS`, default `try`). Build tasks don't run tests, so a failure is a - compilation/link error. -3. Fetch the task definition to read `GECKO_HEAD_REV` (the revision is not in the message). -4. Dedupe by revision with an in-memory TTL cache, so only one agent run is triggered per - revision even when many build tasks fail for the same push. -5. Trigger the agent with the failing tasks -- it resolves the push commits itself and - returns the commit it blamed for the failure, so the listener does no pushlog lookups. -6. `POST /agents/build-repair/runs`, then poll `GET /runs/{run_id}` until terminal. Look the - blamed commit up in the firefox GitHub mirror to get its author's email and send the - report email to that developer. - -The dedupe cache and pending-run tracking are in-memory (reset on restart). +1. **Subscribe.** Consume `task-failed` messages from `pulse.mozilla.org`. +2. **Filter** to a watched `project` (`WATCHED_REPOS`, default `autoland`), then by task + kind: build tasks (compile/link errors) take the build-repair path; test tasks take the + test-repair path. Fetch the task definition for `GECKO_HEAD_REV` (not in the message). + - _Build:_ keep only failures this push introduced (not inherited from an ancestor). + - _Test:_ resolve the failing test groups (mozci), then keep only groups that are a + genuine regression (not inherited) and not intermittent (tests.firefox.dev flakiness + below `FLAKINESS_THRESHOLD`). +3. **Dedupe** with in-memory TTL caches: build-repair once per revision; test-repair once per + `(push, test group)`, so a manifest failing across chunks is investigated once. One + test-repair run per task carries only the task id. +4. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until + terminal, then email a hackbot UI link, the analysis summary, a Treeherder link, and the + commit the agent blamed. Build-repair mails the blamed commit's author; test-repair mails + the notification address (`TEST_REPAIR_NOTIFICATION_EMAIL`). + +The dedupe caches and pending-run tracking are in-memory (reset on restart). ## Run locally @@ -29,18 +35,19 @@ The dedupe cache and pending-run tracking are in-memory (reset on restart). export PULSE_USER=... PULSE_PASSWORD=... # https://pulseguardian.mozilla.org export HACKBOT_API_URL=https://hackbot-api.../ HACKBOT_API_KEY=... export HACKBOT_UI_URL=https://hackbot-ui.../ -export WATCHED_REPOS=try +export WATCHED_REPOS=autoland export DRY_RUN=true # log intended calls, don't POST uv run --package hackbot-pulse-listener python -m app ``` Email is sent only when `SENDGRID_API_KEY` and `NOTIFICATION_SENDER` are set; otherwise it -is logged and skipped. The blamed commit's author (looked up in the firefox GitHub mirror), -the pushing developer, and, if set, the -`NOTIFICATION_TEAM_EMAIL` team address all get the email, which spells out why each -recipient is on it. Set `NOTIFICATION_OVERRIDE_EMAIL` to route every notification to a -single address (useful for local testing). By default only runs that produced a patch are -emailed; set `NOTIFY_ONLY_WITH_PATCH=false` to also notify on transient / not-to-blame runs. +is logged and skipped. Build-repair mails the blamed commit's author (looked up in the +firefox GitHub mirror), the pushing developer, and the `NOTIFICATION_TEAM_EMAIL` team +address if set; test-repair mails `TEST_REPAIR_NOTIFICATION_EMAIL` (with the culprit author CC'd). Set +`NOTIFICATION_OVERRIDE_EMAIL` to route every notification to a single address (useful for +local testing). By default only build-repair runs that produced a patch are emailed; set +`NOTIFY_ONLY_WITH_PATCH=false` to also notify on transient / not-to-blame runs (test-repair always +notifies). When `NOTIFICATION_TEAM_EMAIL` is set, notifications use it as `Reply-To` so recipients can reply with feedback on the analysis. diff --git a/services/hackbot-pulse-listener/app/client.py b/services/hackbot-pulse-listener/app/client.py index a4f98ba518..e09ca7a5ef 100644 --- a/services/hackbot-pulse-listener/app/client.py +++ b/services/hackbot-pulse-listener/app/client.py @@ -13,13 +13,14 @@ def _headers() -> dict[str, str]: return {"X-API-Key": settings.hackbot_api_key} -def trigger_run(inputs: dict) -> str | None: - """Create a build-repair run. Returns the run id, or None in dry-run mode.""" +def trigger_run(inputs: dict, agent_name: str | None = None) -> str | None: + """Create an agent run. Returns the run id, or None in dry-run mode.""" + agent = agent_name or settings.agent_name if settings.dry_run: - logger.info("[dry-run] would trigger %s run: %s", settings.agent_name, inputs) + logger.info("[dry-run] would trigger %s run: %s", agent, inputs) return None - url = f"{settings.hackbot_api_url}/agents/{settings.agent_name}/runs" + url = f"{settings.hackbot_api_url}/agents/{agent}/runs" resp = httpx.post(url, json=inputs, headers=_headers(), timeout=_TIMEOUT) resp.raise_for_status() return resp.json()["run_id"] diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 60ea95ac65..886862c88c 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -12,6 +12,8 @@ class Settings(BaseSettings): hackbot_api_key: str = "" hackbot_ui_url: str = "" agent_name: str = "build-repair" + # Agent that analyzes test failures (separate Cloud Run Job from build-repair). + test_repair_agent_name: str = "test-repair" # Source links shown in notifications. firefox_git_url: str = "https://github.com/mozilla-firefox/firefox" @@ -25,6 +27,9 @@ class Settings(BaseSettings): run_try_push: bool = False model: str | None = None max_turns: int | None = None + # Skip a failing test whose historical cross-push failure rate is at or above + # this (clearly intermittent) before spending an test-repair run. + flakiness_threshold: float = 0.2 # Dedupe (in-memory, by hg revision) dedupe_ttl_seconds: int = 6 * 60 * 60 @@ -45,6 +50,8 @@ class Settings(BaseSettings): notification_sender: str | None = None # Team address CC'd on every notification alongside the revision author. notification_team_email: str | None = None + # Distribution address; primary recipient of test-repair verdicts. + test_repair_notification_email: str | None = None # Send all notifications to this address instead of the developer (local testing). notification_override_email: str | None = None # Only notify when the run produced a patch (skip transient / not-to-blame runs). diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index a37865b86f..b7aac2b662 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -6,7 +6,15 @@ from kombu import Connection, Exchange, Queue from kombu.mixins import ConsumerMixin -from app import client, lando, regression, taskcluster, worker +from app import ( + client, + failures, + flakiness, + lando, + regression, + taskcluster, + worker, +) from app.config import settings from app.models import RunContext @@ -16,30 +24,52 @@ EXCHANGES = ("exchange/taskcluster-queue/v1/task-failed",) -# In-memory dedupe of hg revisions already handed to the agent. A revision is -# recorded only once we actually trigger a run, so an inherited failure on one -# build label never suppresses a genuine regression on another label of the -# same push, while a revision that breaks many builds still triggers only once. -# Messages are handled on worker threads, so the check-and-record is done under -# a lock. +# Taskcluster ``kind`` tags that denote test tasks (vs build tasks). +TEST_KINDS = {"test", "mochitest", "web-platform-tests", "source-test"} + +# In-memory dedupe of hg revisions already handed to the build-repair agent. A +# revision is recorded only once we actually trigger a run, so an inherited +# failure on one build label never suppresses a genuine regression on another +# label of the same push, while a revision that breaks many builds still triggers +# only once. Messages are handled on worker threads, so the check-and-record is +# done under a lock. _seen: TTLCache = TTLCache( maxsize=settings.dedupe_max_size, ttl=settings.dedupe_ttl_seconds ) _seen_lock = threading.Lock() +# Independent dedupe for test-repair runs, keyed by (taskGroupId, test group): one push +# emits many failing test tasks, and we want one run per failing group. +_seen_tests: TTLCache = TTLCache( + maxsize=settings.dedupe_max_size, ttl=settings.dedupe_ttl_seconds +) +_seen_tests_lock = threading.Lock() + + +def _is_test_task(tags: dict) -> bool: + return tags.get("kind") in TEST_KINDS or bool(tags.get("test-suite")) + def process(body: dict, executor: Executor) -> str | None: """Handle one Taskcluster failure message. Returns the triggered run id.""" tags = (body.get("task") or {}).get("tags") or {} - task_label = tags.get("label") or "" - if "build" not in task_label or "test" in task_label: - return None - project = tags.get("project") if project not in settings.watched_repos_set: return None + task_label = tags.get("label") or "" + if "build" in task_label and "test" not in task_label: + return _process_build(body, tags, executor) + if _is_test_task(tags): + return _process_test(body, tags, executor) + return None + + +def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: + """Build-failure path: trigger the build-repair agent (unchanged behavior).""" + project = tags.get("project") + task_label = tags.get("label") or "" task_id = body["status"]["taskId"] task_name = tags.get("label") or task_id developer_email = tags.get("createdForUser") @@ -118,6 +148,146 @@ def process(body: dict, executor: Executor) -> str | None: return run_id +def _harness(tags: dict, label: str) -> str: + """The tests.firefox.dev harness key for a test task.""" + suite = tags.get("test-suite") or "" + if "xpcshell" in suite or "xpcshell" in label: + return "xpcshell" + if "mochitest" in suite: + return "mochitest" + return tags.get("kind") or suite + + +def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: + """Test-failure path: filter, then trigger the test-repair agent for the task. + + One push emits many failing test tasks; each task may fail several groups. We + resolve the failing groups (via mozci) and keep the ones that are genuine, + non-intermittent regressions, then trigger a single run for the task (the + agent resolves the commit range itself from the task id). Dedupe is per + (push, group) so a manifest failing across chunks is investigated once. + """ + status = body.get("status") or {} + task_id = status.get("taskId") + task_group_id = status.get("taskGroupId") + project = tags.get("project") + label = tags.get("label") or task_id + developer_email = tags.get("createdForUser") + harness = _harness(tags, label) + + hg_revision = taskcluster.get_hg_revision(task_id) + if not hg_revision: + logger.warning("No GECKO_HEAD_REV for test task %s; skipping", task_id) + return None + + groups = failures.failing_groups(task_id) + if not groups: + logger.info("No failing test groups resolved for task %s; skipping", task_id) + return None + + fresh = _fresh_groups(groups, project, hg_revision, harness) + if not fresh: + logger.info("No new, non-intermittent groups for task %s; skipping", task_id) + return None + + return _trigger_test_repair( + fresh, + project, + hg_revision, + task_group_id, + task_id, + label, + developer_email, + executor, + ) + + +def _fresh_groups(groups, project: str, hg_revision: str, harness: str): + """Groups that are genuine, non-intermittent regressions worth investigating.""" + fresh = [] + for fg in groups: + # Cheap intermittent gate first (one HTTP call) before the mozci walk. + flak = flakiness.get_flakiness(fg.test, harness) + if flak.total and flak.failure_rate >= settings.flakiness_threshold: + logger.info( + "Test %s is intermittent (failure rate %.2f); skipping", + fg.test, + flak.failure_rate, + ) + continue + is_new, _ = regression.is_new_test_failure(project, hg_revision, fg.group) + if not is_new: + logger.info( + "Group %s at %s inherited from an ancestor; skipping", + fg.group, + hg_revision, + ) + continue + fresh.append(fg) + return fresh + + +def _trigger_test_repair( + fresh, + project: str, + hg_revision: str, + task_group_id: str, + task_id: str, + label: str, + developer_email: str | None, + executor: Executor, +) -> str | None: + # Dedupe per (push, group): trigger only when this task has a fresh group not + # already handed off (e.g. by a sibling chunk), then claim all of them. + keys = [(task_group_id, fg.group) for fg in fresh] + with _seen_tests_lock: + unseen = [k for k in keys if k not in _seen_tests] + if not unseen: + logger.info( + "All fresh groups in task %s already triggered; skipping", task_id + ) + return None + for k in unseen: + _seen_tests[k] = True + + inputs: dict = {"failure_tasks": {label: task_id}} + if settings.model: + inputs["model"] = settings.model + if settings.max_turns is not None: + inputs["max_turns"] = settings.max_turns + + try: + run_id = client.trigger_run(inputs, agent_name=settings.test_repair_agent_name) + except Exception: + logger.exception("Failed to trigger test-repair run for task %s", task_id) + with _seen_tests_lock: + for k in unseen: + _seen_tests.pop(k, None) + return None + + logger.info( + "Triggered test-repair run %s for %s task %s (%d group(s)) at %s", + run_id, + project, + task_id, + len(fresh), + hg_revision, + ) + if run_id is not None: + ctx = RunContext( + run_id=run_id, + repo=project, + git_commit=lando.hg_to_git(hg_revision) or "", + hg_revision=hg_revision, + task_id=task_id, + developer_email=developer_email, + agent=settings.test_repair_agent_name, + test_name=fresh[0].group, + ) + executor.submit(worker.poll_and_notify, ctx) + return run_id + + def make_handler(executor: Executor): def run(body: dict) -> None: try: diff --git a/services/hackbot-pulse-listener/app/failures.py b/services/hackbot-pulse-listener/app/failures.py new file mode 100644 index 0000000000..0663b1abce --- /dev/null +++ b/services/hackbot-pulse-listener/app/failures.py @@ -0,0 +1,54 @@ +"""Extract the failing test groups from a Taskcluster test task, via mozci. + +A ``task-failed`` pulse event names a failing test *task* but not which test +manifests (groups) actually failed. Rather than fetch and parse artifacts +ourselves, we ask mozci: its errorsummary data source reads the task's structured +results and returns the failing groups with their failing tests and failure +types. This works across harnesses (mochitest / xpcshell / web-platform-tests), +keyed only by task id, and stays in sync with CI format changes. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass + +import mozci.push # noqa: F401 (imported so mozci registers its data sources) +from mozci import data + +logger = logging.getLogger(__name__) + + +@dataclass(frozen=True) +class FailingGroup: + """A failing test manifest and a representative failing test within it.""" + + group: str + test: str + failure_type: str + + +def _failure_types(task_id: str) -> dict: + """mozci: ``{group: [(test_name, FailureType), ...]}`` for a task.""" + return data.handler.get("test_task_failure_types", task_id=task_id) + + +def failing_groups(task_id: str) -> list[FailingGroup]: + """Failing test groups for a task; empty on any error (gate fails open).""" + try: + by_group = _failure_types(task_id) + except Exception: + logger.exception("Could not read failing groups for task %s", task_id) + return [] + + groups: list[FailingGroup] = [] + for group, fails in by_group.items(): + if not group or not fails: + continue + test, ftype = fails[0] + groups.append( + FailingGroup( + group=group, test=test, failure_type=getattr(ftype, "name", str(ftype)) + ) + ) + return groups diff --git a/services/hackbot-pulse-listener/app/flakiness.py b/services/hackbot-pulse-listener/app/flakiness.py new file mode 100644 index 0000000000..7519788d84 --- /dev/null +++ b/services/hackbot-pulse-listener/app/flakiness.py @@ -0,0 +1,195 @@ +"""tests.firefox.dev flakiness lookup for the test-repair gate. + +The tests.firefox.dev dashboard (github.com/mozilla/aretestsfastyet) is static JS +that reads pre-aggregated timings JSON from the Firefox CI Taskcluster index at +runtime. We consume the same public artifacts (no auth) to get a test's cross-push +flakiness, so the listener can skip clearly intermittent tests before spending an +agent run. + +Only ``mozilla-central`` / ``try`` datasets are published (there is no autoland +index), so this is a flakiness signal only; the autoland last-green push and the +candidate commit range come from mozci. The decode logic (bucket hashing and +per-test stats) is a direct port of the dashboard's ``common-test-data.js``. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass + +import httpx + +logger = logging.getLogger(__name__) + +_INDEX_URL = ( + "https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/" + "gecko.v2.{repo}.latest.source.test-info-{harness}-timings/artifacts/public/{filename}" +) +_TIMEOUT = 30 +_TOTAL_CHUNKS = 64 + + +@dataclass(frozen=True) +class Flakiness: + """Cross-push pass/fail stats for one test over the timings window.""" + + total: int = 0 + passes: int = 0 + fails: int = 0 + timeouts: int = 0 + crashes: int = 0 + skips: int = 0 + # Most recent day offset (relative to metadata.startDate) the test was green, + # or None if it never passed in the window. Coarse context only. + last_green_day: int | None = None + + @property + def failure_rate(self) -> float: + """Fraction of non-skipped runs that failed/timed-out/crashed (0 if none).""" + denom = self.passes + self.fails + self.timeouts + self.crashes + if denom == 0: + return 0.0 + return (self.fails + self.timeouts + self.crashes) / denom + + +def _to_int32(n: int) -> int: + """Wrap to a signed 32-bit int, matching JavaScript's ``| 0``.""" + n &= 0xFFFFFFFF + return n - 0x100000000 if n >= 0x80000000 else n + + +def _chunk_index(full_path: str, total_chunks: int = _TOTAL_CHUNKS) -> int: + """Bucket a test path to 0..63; port of the dashboard's ``getChunkIndex``.""" + h = 0 + for ch in full_path: + h = _to_int32((h << 5) - h + ord(ch)) + return ((h % total_chunks) + total_chunks) % total_chunks + + +def _fetch_bucket(harness: str, chunk: int, repo: str) -> dict: + filename = f"{harness}-{chunk:02x}.json" + url = _INDEX_URL.format(repo=repo, harness=harness, filename=filename) + resp = httpx.get(url, timeout=_TIMEOUT, follow_redirects=True) + resp.raise_for_status() + return resp.json() + + +def _decompress_days(days: list[int]) -> list[int]: + """Days are stored as offsets from the previous entry; return absolute days.""" + out: list[int] = [] + acc = 0 + for d in days: + acc += d + out.append(acc) + return out + + +def _group_len(group: dict) -> int: + for key in ("counts", "durations", "taskIdIds"): + if key in group: + return len(group[key]) + return len(group.get("days") or []) + + +def _count_at(group: dict, i: int) -> int: + """Runs recorded at index ``i`` of a status group; port of ``getCountAtIndex``.""" + if "counts" in group: + return group["counts"][i] + if "durations" in group: + return len(group["durations"][i]) + if "taskIdIds" in group: + return len(group["taskIdIds"][i]) + return 1 + + +def _classify(status: str) -> str | None: + """Map a status string to pass/fail/timeout/crash/skip (None = ignored).""" + if status == "SKIP": + return "skip" + if status == "CRASH": + return "crash" + if status.startswith("TIMEOUT"): + return "timeout" + if status == "UNKNOWN": + return None + # PASS-*, OK and EXPECTED-FAIL are all treated as green by the dashboard. + if status.startswith("PASS") or status in ("OK", "EXPECTED-FAIL"): + return "pass" + return "fail" + + +def _find_test_id(data: dict, test_path: str) -> int | None: + tables = data.get("tables") or {} + info = data.get("testInfo") or {} + runs = data.get("testRuns") or [] + paths = tables.get("testPaths") or [] + names = tables.get("testNames") or [] + path_ids = info.get("testPathIds") or [] + name_ids = info.get("testNameIds") or [] + for test_id, group in enumerate(runs): + if not group or test_id >= len(path_ids) or test_id >= len(name_ids): + continue + dir_path = paths[path_ids[test_id]] + test_name = names[name_ids[test_id]] + full = f"{dir_path}/{test_name}" if dir_path else test_name + if full == test_path: + return test_id + return None + + +def _compute_stats(data: dict, test_path: str) -> Flakiness: + """Aggregate a test's status groups into a :class:`Flakiness`.""" + statuses = (data.get("tables") or {}).get("statuses") or [] + test_id = _find_test_id(data, test_path) + if test_id is None: + return Flakiness() + test_group = (data.get("testRuns") or [])[test_id] + + counts = {"pass": 0, "fail": 0, "timeout": 0, "crash": 0, "skip": 0} + pass_days: set[int] = set() + bad_days: set[int] = set() + for status_id, group in enumerate(test_group): + if not group or status_id >= len(statuses): + continue + kind = _classify(statuses[status_id]) + if kind is None: + continue + n = _group_len(group) + days = _decompress_days(group.get("days") or list(range(n))) + for i in range(n): + c = _count_at(group, i) + counts[kind] += c + if c > 0 and i < len(days): + if kind == "pass": + pass_days.add(days[i]) + elif kind in ("fail", "timeout", "crash"): + bad_days.add(days[i]) + + green = pass_days - bad_days + return Flakiness( + total=sum(counts.values()), + passes=counts["pass"], + fails=counts["fail"], + timeouts=counts["timeout"], + crashes=counts["crash"], + skips=counts["skip"], + last_green_day=max(green) if green else None, + ) + + +def get_flakiness( + test_path: str, harness: str, repo: str = "mozilla-central" +) -> Flakiness: + """Cross-push flakiness for a test from the tests.firefox.dev timings bucket. + + Fails soft: any network/parse error returns an empty :class:`Flakiness` so the + listener gate treats the test as non-flaky (and errs toward triggering a run). + """ + try: + data = _fetch_bucket(harness, _chunk_index(test_path), repo) + except Exception: + logger.exception( + "tests.firefox.dev lookup failed for %s (%s)", test_path, harness + ) + return Flakiness() + return _compute_stats(data, test_path) diff --git a/services/hackbot-pulse-listener/app/models.py b/services/hackbot-pulse-listener/app/models.py index 2bcaf33cfa..1a8ace1dc4 100644 --- a/services/hackbot-pulse-listener/app/models.py +++ b/services/hackbot-pulse-listener/app/models.py @@ -3,7 +3,7 @@ @dataclass class RunContext: - """What the notifier needs about a triggered build-repair run.""" + """What the notifier needs about a triggered agent run.""" run_id: str repo: str @@ -11,3 +11,7 @@ class RunContext: hg_revision: str task_id: str developer_email: str | None + # Which agent produced the run, and (for test-repair) the failing test group. These + # drive the notifier's recipient/body routing. + agent: str = "build-repair" + test_name: str | None = None diff --git a/services/hackbot-pulse-listener/app/notify.py b/services/hackbot-pulse-listener/app/notify.py index 2e2f8aab6f..3823faea84 100644 --- a/services/hackbot-pulse-listener/app/notify.py +++ b/services/hackbot-pulse-listener/app/notify.py @@ -13,14 +13,22 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: - """Email the developer and team the build failure analysis. + """Email the failure analysis. Only succeeded runs are notified. - Only succeeded runs are notified. + Routes on the agent that produced the run: test-repair sends a + verdict-led body to the test-repair notification address; build-repair keeps its + existing behavior. """ if run_doc.get("status") != "succeeded": logger.info("Run %s did not succeed; skipping notification", ctx.run_id) return + if ctx.agent == settings.test_repair_agent_name: + _send_test_repair_email(ctx, run_doc) + else: + _send_build_repair_email(ctx, run_doc) + +def _send_build_repair_email(ctx: RunContext, run_doc: dict) -> None: patch = _fetch_patch(ctx.run_id, run_doc) if settings.notify_only_with_patch and not patch: logger.info("Run %s produced no patch; skipping notification", ctx.run_id) @@ -37,6 +45,43 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: logger.info("SendGrid not configured; skipping email for run %s", ctx.run_id) return + subject = ( + f"[build-repair] Build failure analysis for {ctx.repo}@{ctx.git_commit[:12]}" + ) + body_md = _build_body(ctx, run_doc, patch, blamed_author) + _deliver(subject, body_md, recipients, patch) + + +def _send_test_repair_email(ctx: RunContext, run_doc: dict) -> None: + findings = (run_doc.get("summary") or {}).get("findings") or {} + culprit = findings.get("culprit_commit") + culprit_author = ( + github.commit_author_email(culprit) + if culprit and findings.get("classification") == "regression" + else None + ) + # test-repair verdicts are always notified (including do-not-backout verdicts), so + # the build-repair notify_only_with_patch gate does not apply here. + recipients = _test_repair_recipients(culprit_author) + if not recipients: + logger.info( + "No recipients for test-repair run %s; skipping notification", ctx.run_id + ) + return + if not (settings.sendgrid_api_key and settings.notification_sender): + logger.info("SendGrid not configured; skipping email for run %s", ctx.run_id) + return + + patch = _fetch_patch(ctx.run_id, run_doc) + banner = _RECOMMENDATION_BANNER.get( + findings.get("recommendation"), findings.get("recommendation") or "analysis" + ) + subject = f"[test-repair] {banner} - {ctx.test_name} ({ctx.repo})" + body_md = _build_test_repair_body(ctx, findings, patch, culprit_author) + _deliver(subject, body_md, recipients, patch) + + +def _deliver(subject: str, body_md: str, recipients: list[str], patch: str | None): import markdown2 import sendgrid from sendgrid.helpers.mail import ( @@ -55,13 +100,7 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: To, ) - subject = ( - f"[build-repair] Build failure analysis for {ctx.repo}@{ctx.git_commit[:12]}" - ) - - body_md = _build_body(ctx, run_doc, patch, blamed_author) html = markdown2.markdown(body_md, extras=["fenced-code-blocks", "tables"]) - sg = sendgrid.SendGridAPIClient(api_key=settings.sendgrid_api_key) to_emails = [To(recipients[0])] + [Cc(addr) for addr in recipients[1:]] message = Mail( @@ -82,7 +121,7 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: message.reply_to = ReplyTo(settings.notification_team_email) response = sg.send(message=message) logger.info( - "Sent build-repair notification to %s (status %s)", + "Sent notification to %s (status %s)", ", ".join(recipients), response.status_code, ) @@ -107,6 +146,94 @@ def _recipients( return recipients +_RECOMMENDATION_BANNER = { + "backout": "BACK OUT the culprit", + "do_not_backout": "DO NOT back out (intermittent)", + "land_fix": "LAND the proposed fix", +} + + +def _test_repair_recipients(culprit_author: str | None) -> list[str]: + """The test-repair notification address (primary To), the culprit author, then the team. + + ``notification_override_email`` short-circuits to a single address for testing. + """ + if settings.notification_override_email: + return [settings.notification_override_email] + recipients: list[str] = [] + for addr in ( + settings.test_repair_notification_email, + culprit_author, + settings.notification_team_email, + ): + if addr and addr not in recipients: + recipients.append(addr) + return recipients + + +def _build_test_repair_body( + ctx: RunContext, + findings: dict, + patch: str | None, + culprit_author: str | None, +) -> str: + recommendation = findings.get("recommendation") + banner = _RECOMMENDATION_BANNER.get(recommendation, recommendation or "analysis") + lines = [ + "# Test failure analysis", + "", + f"- **Recommendation:** {banner}", + f"- **Failing test:** `{ctx.test_name}`", + f"- **Classification:** {findings.get('classification')}", + f"- **Repository:** {ctx.repo}", + f"- **Revision (git):** [`{ctx.git_commit[:12]}`]({_git_url(ctx.git_commit)})", + f"- **Revision (hg):** [`{ctx.hg_revision[:12]}`]({_hg_url(ctx.hg_revision)})", + f"- **Failed task:** [`{ctx.task_id}`]({_task_url(ctx.task_id)})", + f"- **Treeherder:** " + f"[jobs]({_treeherder_url(ctx.repo, ctx.hg_revision, ctx.task_id)})", + ] + + confidence = findings.get("confidence") + if confidence is not None: + lines.append(f"- **Confidence:** {confidence}") + + culprit = findings.get("culprit_commit") + if culprit: + by = f" by {culprit_author}" if culprit_author else "" + lines.append( + f"- **Culprit commit:** [`{culprit[:12]}`]({_git_url(culprit)}){by}" + ) + + last_green = findings.get("last_green_revision") + if last_green: + lines.append(f"- **Last green revision:** `{last_green}`") + + bug = findings.get("culprit_bug") + if bug: + lines.append(f"- **Bug:** [{bug}]({_bug_url(bug)})") + + if settings.hackbot_ui_url: + run_url = f"{settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" + lines.append(f"- **Run details:** {run_url}") + + if findings.get("summary"): + lines += ["", "## Summary", "", _demote_headings(findings["summary"])] + if findings.get("analysis"): + lines += ["", "## Analysis", "", _demote_headings(findings["analysis"])] + if patch: + lines += ["", "## Proposed patch", "", _patch_block(patch)] + + if settings.notification_team_email: + lines += [ + "", + "---", + "", + "_Reply to this email with any feedback on this analysis; it reaches " + "the hackbot team._", + ] + return "\n".join(lines) + + def _fetch_patch(run_id: str, run_doc: dict) -> str | None: """Download the proposed-fix patch artifact, if the run produced one.""" artifacts = run_doc.get("artifacts") or [] diff --git a/services/hackbot-pulse-listener/app/regression.py b/services/hackbot-pulse-listener/app/regression.py index 6445a912e1..8a99dbadf6 100644 --- a/services/hackbot-pulse-listener/app/regression.py +++ b/services/hackbot-pulse-listener/app/regression.py @@ -3,75 +3,49 @@ from mozci.errors import ParentPushNotFound from mozci.push import MAX_DEPTH, Push +from mozci.task import Status logger = logging.getLogger(__name__) - -# When the nearest ancestor that ran the build has not produced a decisive -# result yet, wait for it in-process instead of racing ahead and misreporting an -# inherited failure as new. A build can take tens of minutes and may hit an -# infra exception and be auto-retried, so we poll until it settles or the -# deadline elapses (well above a normal build + one retry). Fails open after the -# deadline so a real regression is never silently dropped. +# Poll an unsettled ancestor for up to MAX_WAIT_SECONDS before giving up. POLL_INTERVAL_SECONDS = 120 MAX_WAIT_SECONDS = 60 * 60 -# A green build; mozci reports it as "passed" (Taskcluster) or "success" -# (Treeherder). Failures are read from mozci's Task.failed attribute instead of -# reimplementing the vocabulary. +# mozci spells a green result "passed" (Taskcluster) or "success" (Treeherder). _PASSED_RESULTS = ("passed", "success") -# A build in one of these states, or with one of these (infra) results, has not -# settled: it is still running or was retried after an exception, so its outcome -# is not knowable yet and we wait for it. Anything else that is not a decisive -# pass/fail (unscheduled, canceled, superseded, ...) or a build that never ran -# at all (coalesced) is treated as non-decisive and skipped. +# States/results whose outcome isn't knowable yet: still running, or awaiting an +# auto-retry after an infra exception. _UNSETTLED_STATES = ("pending", "running", "exception") _UNSETTLED_RESULTS = ("exception", "retry") -# Sentinel meaning the decision cannot be made yet because an ancestor build has -# not settled; the caller waits and re-checks. +# The deciding ancestor hasn't settled yet; the caller waits and re-checks. _PENDING = object() def _build_status(push: Push, label: str): - """Return 'passed', 'failed', _PENDING, or None for a build label on a push. - - 'passed'/'failed' come from a run with a decisive result. _PENDING means the - result is not knowable yet: a task exists but has not settled (still running, - or exceptioned and awaiting a retry), or no task is visible yet but the build - was scheduled to run on this push (its result has not propagated). None means - the build produced no decisive result and none is coming: it was coalesced / - never scheduled, or it only reached a non-decisive terminal state. None is - deliberately non-decisive so coalescing gaps are skipped and never suppress a - real regression. Retriggers are collapsed: any green run means 'passed'; only - a genuine build failure counts as 'failed'. + """'passed'/'failed'/_PENDING/None for a build label on a push. + + None is non-decisive (coalesced, never scheduled, or a non-pass/fail terminal + state), so such gaps are skipped rather than mistaken for an inherited failure. """ label_tasks = [t for t in push.tasks if t.label == label] if label_tasks: if any(t.result in _PASSED_RESULTS for t in label_tasks): return "passed" - # Checked before failure: a retrigger that is still running or was - # exceptioned and auto-retried may yet turn green, and any green run wins, - # so we wait for it rather than prematurely inheriting a failure. + # Checked before failure: a still-running or auto-retried run may yet turn + # green, and any green run wins, so wait rather than inherit prematurely. if any( t.state in _UNSETTLED_STATES or t.result in _UNSETTLED_RESULTS for t in label_tasks ): return _PENDING - # Not "not t.failed": a run can be neither passed nor failed (canceled, - # superseded, ...). mozci's Task.failed also counts `exception`, but those - # are unsettled and already returned above, so only genuine build failures - # reach here. if any(t.failed for t in label_tasks): return "failed" - # Ran but reached a non-decisive terminal state (canceled, ...): skip. return None - # No task for this label on the push. Distinguish a build that was scheduled - # to run here but whose result is not visible yet (wait) from one that was - # never scheduled / coalesced away (skip), using the decision task's - # scheduled set (available well before the builds themselves finish). + # No task here: wait if the build was scheduled (result not visible yet), + # skip if it was coalesced away. try: scheduled = label in push.scheduled_task_labels except Exception: @@ -80,12 +54,12 @@ def _build_status(push: Push, label: str): return _PENDING if scheduled else None -def _classify(branch: str, rev: str, label: str): - """Walk ancestors once. Returns True (new), False (inherited), or _PENDING. +def _classify(branch: str, rev: str, status_fn, describe: str): + """Walk ancestors; return (state, last_green_rev). - A fresh Push is built each call so re-checks re-fetch live data (recent - pushes are not finalized in mozci, so their tasks are never served from - cache). + state is True (new), False (inherited) or _PENDING. status_fn(push) reports + 'passed'/'failed'/_PENDING/None for the failing unit (build label or test + group). A fresh Push per call re-fetches live data for unfinalized pushes. """ ancestor = Push(rev, branch=branch) for _ in range(MAX_DEPTH): @@ -93,74 +67,94 @@ def _classify(branch: str, rev: str, label: str): ancestor = ancestor.parent except ParentPushNotFound: break - status = _build_status(ancestor, label) + status = status_fn(ancestor) if status is None: continue if status is _PENDING: logger.info( - "Build %s not settled yet at %s; deferring decision for %s", - label, - ancestor.rev, - rev, + "%s not settled at %s; deferring for %s", describe, ancestor.rev, rev ) - return _PENDING + return _PENDING, None if status == "failed": logger.info( - "Build %s already failing at %s; inherited failure at %s", - label, - ancestor.rev, - rev, + "%s already failing at %s; inherited at %s", describe, ancestor.rev, rev ) - return False - logger.info( - "Build %s passed at %s; new failure introduced at %s", - label, - ancestor.rev, - rev, - ) - return True + return False, None + logger.info("%s passed at %s; new failure at %s", describe, ancestor.rev, rev) + return True, ancestor.rev logger.warning( - "No ancestor within %s pushes ran build %s; running agent", MAX_DEPTH, label + "No ancestor within %s pushes ran %s; running agent", MAX_DEPTH, describe ) - return True + return True, None -def is_new_build_failure(branch: str, rev: str, label: str) -> bool: - """Return True if this push introduced the failure, False if it inherited it. - - Walks back over pushes that did not run the build (coalescing) until it - finds the nearest ancestor that did. When that ancestor's build has not - settled yet, waits in-process and re-checks until it produces a decisive - result or MAX_WAIT_SECONDS elapses, rather than racing ahead and - misreporting an inherited failure as new. Fails open (returns True) on any - mozci/network error, if the build stays unsettled past the deadline, or if - no ancestor within MAX_DEPTH ran the build, so we never silently drop a real - regression. +def _await_new_failure(branch: str, rev: str, status_fn, describe: str): + """(is_new, last_green_rev) for `rev`, waiting on an unsettled ancestor. + + Fails open ((True, None)) on any error, an ancestor unsettled past the + deadline, or no deciding ancestor within MAX_DEPTH, so a real regression is + never silently dropped. """ try: deadline = time.monotonic() + MAX_WAIT_SECONDS while True: - result = _classify(branch, rev, label) - if result is not _PENDING: - return result + state, last_green = _classify(branch, rev, status_fn, describe) + if state is not _PENDING: + return state, last_green if time.monotonic() >= deadline: break logger.info( - "Waiting %ss for an unsettled ancestor build of %s (%s)", + "Waiting %ss for an unsettled ancestor of %s (%s)", POLL_INTERVAL_SECONDS, + describe, rev, - label, ) time.sleep(POLL_INTERVAL_SECONDS) except Exception: - logger.exception("Regression check failed for %s@%s; running agent", label, rev) - return True + logger.exception( + "Regression check failed for %s@%s; running agent", describe, rev + ) + return True, None logger.warning( - "Build %s still unsettled after %ss at %s; running agent", - label, + "%s still unsettled after %ss at %s; running agent", + describe, MAX_WAIT_SECONDS, rev, ) - return True + return True, None + + +def is_new_build_failure(branch: str, rev: str, label: str) -> bool: + """True if this push introduced the build failure, False if it inherited it.""" + is_new, _ = _await_new_failure( + branch, rev, lambda push: _build_status(push, label), f"build {label}" + ) + return is_new + + +def _group_status(push: Push, group: str): + """'passed'/'failed'/_PENDING/None for a test group (manifest) on a push. + + GroupSummary.status combines retriggers into PASS/FAIL/INTERMITTENT. + INTERMITTENT and a missing group are non-decisive: the tests.firefox.dev + flakiness rate judges intermittency instead. + """ + summary = push.group_summaries.get(group) + if summary is None: + return None + if push.is_group_running(summary): + return _PENDING + if summary.status == Status.PASS: + return "passed" + if summary.status == Status.FAIL: + return "failed" + return None + + +def is_new_test_failure(branch: str, rev: str, group: str) -> tuple[bool, str | None]: + """(is_new, last_green_rev) for a failing test group; shares the build walk.""" + return _await_new_failure( + branch, rev, lambda push: _group_status(push, group), f"group {group}" + ) diff --git a/services/hackbot-pulse-listener/deploy.sh b/services/hackbot-pulse-listener/deploy.sh index 94913f00b1..8aed83e552 100755 --- a/services/hackbot-pulse-listener/deploy.sh +++ b/services/hackbot-pulse-listener/deploy.sh @@ -41,6 +41,8 @@ PULSE_USER="${PULSE_USER:?set PULSE_USER (https://pulseguardian.mozilla.org)}" WATCHED_REPOS="${WATCHED_REPOS:-autoland}" NOTIFICATION_SENDER="${NOTIFICATION_SENDER:?set NOTIFICATION_SENDER (verified SendGrid sender)}" NOTIFICATION_TEAM_EMAIL="${NOTIFICATION_TEAM_EMAIL:-}" +# Distribution address; primary recipient of test-repair verdicts. +TEST_REPAIR_NOTIFICATION_EMAIL="${TEST_REPAIR_NOTIFICATION_EMAIL:-}" SA_NAME="${SA_NAME:-hackbot-pulse-listener-run}" SA_EMAIL="${SA_EMAIL:-${SA_NAME}@${PROJECT}.iam.gserviceaccount.com}" @@ -104,6 +106,7 @@ ENV_VARS="${ENV_VARS},ENVIRONMENT=production" ENV_VARS="${ENV_VARS},PULSE_USER=${PULSE_USER},WATCHED_REPOS=${WATCHED_REPOS}" ENV_VARS="${ENV_VARS},NOTIFICATION_SENDER=${NOTIFICATION_SENDER}" ENV_VARS="${ENV_VARS},NOTIFICATION_TEAM_EMAIL=${NOTIFICATION_TEAM_EMAIL}" +ENV_VARS="${ENV_VARS},TEST_REPAIR_NOTIFICATION_EMAIL=${TEST_REPAIR_NOTIFICATION_EMAIL}" gcloud beta run worker-pools deploy "${SERVICE}" \ --image "${IMAGE}" \ diff --git a/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json b/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json new file mode 100644 index 0000000000..54917444c1 --- /dev/null +++ b/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json @@ -0,0 +1,29 @@ +{ + "metadata": { "startDate": "2026.07.01", "days": 21 }, + "tables": { + "testPaths": ["dom/base/test"], + "testNames": ["test_foo.js", "test_bar.js"], + "statuses": [ + "PASS-PARALLEL", + "FAIL-PARALLEL", + "TIMEOUT-SEQUENTIAL", + "SKIP", + "CRASH" + ], + "taskIds": ["aaa.0", "bbb.0", "ccc.0"] + }, + "testInfo": { + "testPathIds": [0, 0], + "testNameIds": [0, 1] + }, + "testRuns": [ + [ + { "durations": [[120, 130], [140]], "days": [0, 2] }, + { "taskIdIds": [[0]], "days": [2] }, + { "taskIdIds": [[1]], "days": [1] }, + null, + null + ], + [null, null, null, { "counts": [4], "days": [0] }, null] + ] +} diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index cf3a7c647b..f4e484bd76 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -3,12 +3,15 @@ from unittest.mock import MagicMock, patch from app import consumer +from app.failures import FailingGroup +from app.flakiness import Flakiness FIXTURES = Path(__file__).parent / "fixtures" def setup_function(): consumer._seen.clear() + consumer._seen_tests.clear() def _sample_bodies(): @@ -32,15 +35,19 @@ def _build_msg(task_id="ABC", project="autoland", label="build-linux64/opt"): } -def test_sample_messages_are_all_tests_and_skipped(): +def test_sample_messages_route_to_test_repair_not_build(): + # The captured samples are all test tasks; watched ones now reach the test-repair + # path (failing_groups consulted), and none trigger the build-repair agent. executor = MagicMock() with ( - patch.object(consumer.taskcluster, "get_hg_revision") as get_rev, + patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), + patch.object(consumer.failures, "failing_groups", return_value=[]) as fg, patch.object(consumer.client, "trigger_run") as trigger, ): for body in _sample_bodies(): assert consumer.process(body, executor) is None - get_rev.assert_not_called() + # At least the autoland test samples were routed to the test-repair path. + assert fg.called trigger.assert_not_called() executor.submit.assert_not_called() @@ -210,6 +217,232 @@ def test_trigger_failure_releases_revision_for_retry(): assert trigger.call_count == 2 +def _test_msg( + task_id="TT", + project="autoland", + kind="mochitest", + label="test-linux1804-64/opt-mochitest-browser-chrome-1", + suite="mochitest-browser-chrome", + group_id="G1", +): + return { + "status": {"taskId": task_id, "taskGroupId": group_id}, + "runId": 0, + "task": { + "tags": { + "kind": kind, + "project": project, + "label": label, + "test-suite": suite, + "createdForUser": "dev@mozilla.com", + } + }, + } + + +_GROUP = FailingGroup( + group="dom/base/test/mochitest.ini", + test="dom/base/test/test_a.js", + failure_type="GENERIC", +) + + +def _test_repair_patches(is_new=(True, "greenrev"), rate=0.0): + return ( + patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), + patch.object(consumer.failures, "failing_groups", return_value=[_GROUP]), + patch.object( + consumer.flakiness, + "get_flakiness", + return_value=Flakiness( + total=10, passes=int(10 * (1 - rate)), fails=int(10 * rate) + ), + ), + patch.object(consumer.regression, "is_new_test_failure", return_value=is_new), + patch.object(consumer.lando, "hg_to_git", return_value="gitH"), + ) + + +def test_test_failure_triggers_rca_run(): + executor = MagicMock() + p1, p2, p3, p4, p5 = _test_repair_patches() + with ( + p1, + p2, + p3, + p4, + p5, + patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, + ): + run_id = consumer.process(_test_msg(), executor) + + assert run_id == "tr-1" + trigger.assert_called_once() + inputs = trigger.call_args.args[0] + assert trigger.call_args.kwargs["agent_name"] == "test-repair" + # The agent resolves the test, commit range and clone depth itself; the + # listener only hands it the failing task. + assert inputs["failure_tasks"] == { + "test-linux1804-64/opt-mochitest-browser-chrome-1": "TT" + } + assert "test_id" not in inputs + assert "candidate_commits" not in inputs + fn, ctx = executor.submit.call_args.args + assert fn is consumer.worker.poll_and_notify + assert ctx.agent == "test-repair" + assert ctx.test_name == "dom/base/test/mochitest.ini" + + +def test_intermittent_test_skipped_before_mozci(): + executor = MagicMock() + p1, p2, p3, p4, p5 = _test_repair_patches(rate=0.8) + with ( + p1, + p2, + p3, + p4 as is_new, + p5, + patch.object(consumer.client, "trigger_run") as trigger, + ): + assert consumer.process(_test_msg(), executor) is None + is_new.assert_not_called() + trigger.assert_not_called() + + +def test_inherited_test_group_skipped(): + executor = MagicMock() + p1, p2, p3, p4, p5 = _test_repair_patches(is_new=(False, None)) + with ( + p1, + p2, + p3, + p4, + p5, + patch.object(consumer.client, "trigger_run") as trigger, + ): + assert consumer.process(_test_msg(), executor) is None + trigger.assert_not_called() + + +def test_same_group_same_push_triggers_once(): + executor = MagicMock() + p1, p2, p3, p4, p5 = _test_repair_patches() + with ( + p1, + p2, + p3, + p4, + p5, + patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, + ): + consumer.process(_test_msg(task_id="A"), executor) + consumer.process(_test_msg(task_id="B"), executor) + trigger.assert_called_once() + + +def test_no_failing_groups_skips(): + executor = MagicMock() + with ( + patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), + patch.object(consumer.failures, "failing_groups", return_value=[]), + patch.object(consumer.client, "trigger_run") as trigger, + ): + assert consumer.process(_test_msg(), executor) is None + trigger.assert_not_called() + + +def test_unwatched_project_test_skipped(): + executor = MagicMock() + with ( + patch.object(consumer.taskcluster, "get_hg_revision") as get_rev, + patch.object(consumer.failures, "failing_groups") as fg, + ): + assert consumer.process(_test_msg(project="try"), executor) is None + get_rev.assert_not_called() + fg.assert_not_called() + + +def test_test_repair_trigger_failure_releases_group_for_retry(): + executor = MagicMock() + p1, p2, p3, p4, p5 = _test_repair_patches() + with ( + p1, + p2, + p3, + p4, + p5, + patch.object( + consumer.client, "trigger_run", side_effect=[RuntimeError("boom"), "tr-2"] + ) as trigger, + ): + assert consumer.process(_test_msg(task_id="A"), executor) is None + assert consumer.process(_test_msg(task_id="B"), executor) == "tr-2" + assert trigger.call_count == 2 + + +def test_harness_detection(): + # xpcshell via the test-suite tag, and via the label when the suite is generic. + assert consumer._harness({"test-suite": "xpcshell"}, "irrelevant") == "xpcshell" + assert ( + consumer._harness({"test-suite": "test"}, "test-linux/opt-xpcshell-4") + == "xpcshell" + ) + assert ( + consumer._harness({"test-suite": "mochitest-browser-chrome"}, "l") + == "mochitest" + ) + # Falls back to the kind when neither xpcshell nor mochitest matches. + assert ( + consumer._harness({"kind": "web-platform-tests"}, "l") == "web-platform-tests" + ) + + +def test_multiple_failing_groups_trigger_one_run_per_task(monkeypatch): + executor = MagicMock() + groups = [ + FailingGroup("dom/base/test/mochitest.ini", "dom/base/test/a.js", "GENERIC"), + FailingGroup("layout/test/mochitest.ini", "layout/test/b.js", "TIMEOUT"), + ] + with ( + patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), + patch.object(consumer.failures, "failing_groups", return_value=groups), + patch.object( + consumer.flakiness, + "get_flakiness", + return_value=Flakiness(total=10, passes=10), + ), + patch.object( + consumer.regression, + "is_new_test_failure", + return_value=(True, "greenrev"), + ), + patch.object(consumer.lando, "hg_to_git", return_value="gitH"), + patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, + ): + run_id = consumer.process(_test_msg(), executor) + + # The whole task gets a single run; the agent investigates every failing group. + assert run_id == "tr-1" + trigger.assert_called_once() + assert trigger.call_args.args[0]["failure_tasks"] == { + "test-linux1804-64/opt-mochitest-browser-chrome-1": "TT" + } + assert executor.submit.call_count == 1 + + +def test_missing_hg_revision_skips_test_task(monkeypatch): + executor = MagicMock() + with ( + patch.object(consumer.taskcluster, "get_hg_revision", return_value=None), + patch.object(consumer.failures, "failing_groups") as fg, + patch.object(consumer.client, "trigger_run") as trigger, + ): + assert consumer.process(_test_msg(), executor) is None + # Bail before doing the (network-heavy) group resolution. + fg.assert_not_called() + trigger.assert_not_called() + + def test_queue_name_includes_non_production_environment(): with patch.object(consumer.settings, "environment", "development"): (queue,) = consumer._build_queues("guest") diff --git a/services/hackbot-pulse-listener/tests/test_failures.py b/services/hackbot-pulse-listener/tests/test_failures.py new file mode 100644 index 0000000000..35afdea970 --- /dev/null +++ b/services/hackbot-pulse-listener/tests/test_failures.py @@ -0,0 +1,42 @@ +"""Tests for extracting failing test groups from a task via mozci.""" + +from types import SimpleNamespace + +from app import failures + + +def _ftype(name): + return SimpleNamespace(name=name) + + +def test_failing_groups_maps_mozci_output(monkeypatch): + fake = { + "browser/base/content/test/bf/browser.toml": [ + ("browser/base/content/test/bf/browser_a.js", _ftype("GENERIC")), + ], + "dom/base/test/mochitest.ini": [ + ("dom/base/test/test_c.js", _ftype("TIMEOUT")), + ], + } + monkeypatch.setattr(failures, "_failure_types", lambda t: fake) + groups = failures.failing_groups("TASK") + assert {g.group for g in groups} == set(fake) + bf = next(g for g in groups if g.group.endswith("browser.toml")) + assert bf.test == "browser/base/content/test/bf/browser_a.js" + assert bf.failure_type == "GENERIC" + assert next( + g for g in groups if g.group.endswith("mochitest.ini") + ).failure_type == ("TIMEOUT") + + +def test_failing_groups_skips_empty_group(monkeypatch): + monkeypatch.setattr(failures, "_failure_types", lambda t: {"grp": []}) + assert failures.failing_groups("TASK") == [] + + +def test_failing_groups_empty_on_mozci_error(monkeypatch): + def boom(task_id): + raise RuntimeError("mozci could not read the task") + + monkeypatch.setattr(failures, "_failure_types", boom) + assert failures.failing_groups("TASK") == [] diff --git a/services/hackbot-pulse-listener/tests/test_flakiness.py b/services/hackbot-pulse-listener/tests/test_flakiness.py new file mode 100644 index 0000000000..b31d057ebb --- /dev/null +++ b/services/hackbot-pulse-listener/tests/test_flakiness.py @@ -0,0 +1,85 @@ +import json +from pathlib import Path + +import pytest +from app import flakiness + +FIXTURE = Path(__file__).parent / "fixtures" / "xpcshell-bucket.json" + + +@pytest.fixture +def bucket() -> dict: + return json.loads(FIXTURE.read_text()) + + +def test_chunk_index_matches_reference_hash(): + # Port of the dashboard's getChunkIndex (32-bit signed string hash mod 64). + assert flakiness._chunk_index("") == 0 + assert flakiness._chunk_index("a") == 33 + assert flakiness._chunk_index("ab") == 33 + # Always in range and deterministic. + idx = flakiness._chunk_index("dom/base/test/test_foo.js") + assert 0 <= idx < 64 + assert idx == flakiness._chunk_index("dom/base/test/test_foo.js") + + +def test_compute_stats_counts_and_rate(bucket): + stats = flakiness._compute_stats(bucket, "dom/base/test/test_foo.js") + # foo: 3 passes (durations 2+1), 1 fail, 1 timeout, no skip/crash. + assert stats.passes == 3 + assert stats.fails == 1 + assert stats.timeouts == 1 + assert stats.crashes == 0 + assert stats.skips == 0 + assert stats.total == 5 + # failure_rate counts fail+timeout+crash over pass+fail+timeout+crash = 2/5. + assert stats.failure_rate == pytest.approx(0.4) + + +def test_compute_stats_last_green_day(bucket): + # Passes on days 0 and 2; a fail on day 2 and a timeout on day 1, so only day 0 + # is green. Days are differentially compressed and decoded by running sum. + stats = flakiness._compute_stats(bucket, "dom/base/test/test_foo.js") + assert stats.last_green_day == 0 + + +def test_compute_stats_skip_only_test(bucket): + stats = flakiness._compute_stats(bucket, "dom/base/test/test_bar.js") + assert stats.skips == 4 + assert stats.passes == 0 + assert stats.fails == 0 + # No pass/fail/timeout/crash -> rate is 0, not a division error. + assert stats.failure_rate == 0.0 + + +def test_compute_stats_unknown_test_returns_empty(bucket): + stats = flakiness._compute_stats(bucket, "does/not/exist.js") + assert stats.total == 0 + assert stats.failure_rate == 0.0 + + +def test_get_flakiness_uses_bucket(monkeypatch, bucket): + captured = {} + + def fake_fetch(harness, chunk, repo): + captured["harness"] = harness + captured["chunk"] = chunk + captured["repo"] = repo + return bucket + + monkeypatch.setattr(flakiness, "_fetch_bucket", fake_fetch) + stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") + assert stats.passes == 3 + assert captured["harness"] == "xpcshell" + assert captured["repo"] == "mozilla-central" + assert captured["chunk"] == flakiness._chunk_index("dom/base/test/test_foo.js") + + +def test_get_flakiness_fails_soft_on_network_error(monkeypatch): + def boom(*a, **k): + raise RuntimeError("network down") + + monkeypatch.setattr(flakiness, "_fetch_bucket", boom) + stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") + assert stats.total == 0 + assert stats.failure_rate == 0.0 diff --git a/services/hackbot-pulse-listener/tests/test_notify.py b/services/hackbot-pulse-listener/tests/test_notify.py index db7b1c50ed..86f3ab25d5 100644 --- a/services/hackbot-pulse-listener/tests/test_notify.py +++ b/services/hackbot-pulse-listener/tests/test_notify.py @@ -18,6 +18,14 @@ def _ctx(**over): return RunContext(**base) +def _test_repair_ctx(**over): + return _ctx( + agent="test-repair", + test_name="dom/base/test/mochitest.ini", + **over, + ) + + def test_skips_without_recipient(): # No developer, no team, no override -> nothing to send, must not raise. notify.send_email(_ctx(developer_email=None), {"status": "succeeded"}) @@ -282,6 +290,128 @@ def test_recipients_dedupes_and_skips_empty(monkeypatch): assert notify._recipients(None) == [] +def _test_repair_findings(**over): + base = { + "classification": "regression", + "recommendation": "backout", + "culprit_commit": "abc123def456", + "confidence": 0.8, + "last_green_revision": "green99", + "summary": "A landed commit removed a null check.", + "analysis": "# Root cause\nThe diff dropped validation.", + } + base.update(over) + return base + + +def test_test_repair_body_leads_with_recommendation(): + body = notify._build_test_repair_body( + _test_repair_ctx(), _test_repair_findings(), None, "culprit@mozilla.com" + ) + assert "Test failure analysis" in body + assert "BACK OUT the culprit" in body + assert "dom/base/test/mochitest.ini" in body + assert "abc123def456"[:12] in body + assert "by culprit@mozilla.com" in body + assert "green99" in body + assert "## Analysis" in body + + +def test_test_repair_intermittent_body_says_do_not_backout(): + findings = _test_repair_findings( + classification="intermittent", + recommendation="do_not_backout", + culprit_commit=None, + ) + body = notify._build_test_repair_body(_test_repair_ctx(), findings, None, None) + assert "DO NOT back out" in body + assert "Culprit commit" not in body + + +def test_test_repair_recipients_address_then_culprit(monkeypatch): + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr( + notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" + ) + monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") + assert notify._test_repair_recipients("culprit@mozilla.com") == [ + "test-repair@mozilla.com", + "culprit@mozilla.com", + "team@mozilla.com", + ] + + +def test_test_repair_recipients_override_wins(monkeypatch): + monkeypatch.setattr( + notify.settings, "notification_override_email", "me@mozilla.com" + ) + monkeypatch.setattr( + notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" + ) + assert notify._test_repair_recipients("culprit@mozilla.com") == ["me@mozilla.com"] + + +def test_test_repair_intermittent_sends_without_patch(monkeypatch): + # No patch, notify_only_with_patch True -> test-repair still sends (unlike build-repair). + monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") + monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") + monkeypatch.setattr(notify.settings, "notify_only_with_patch", True) + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr( + notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" + ) + monkeypatch.setattr(notify.settings, "notification_team_email", None) + + run_doc = { + "status": "succeeded", + "summary": { + "findings": _test_repair_findings( + classification="intermittent", + recommendation="do_not_backout", + culprit_commit=None, + ) + }, + } + fake_client = MagicMock() + fake_client.send.return_value = MagicMock(status_code=202) + with patch("sendgrid.SendGridAPIClient", return_value=fake_client): + notify.send_email(_test_repair_ctx(), run_doc) + + fake_client.send.assert_called_once() + personalizations = fake_client.send.call_args.kwargs["message"].get()[ + "personalizations" + ][0] + assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] + + +def test_test_repair_regression_ccs_culprit_author(monkeypatch): + monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") + monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr( + notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" + ) + monkeypatch.setattr(notify.settings, "notification_team_email", None) + + run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_findings()}} + fake_client = MagicMock() + fake_client.send.return_value = MagicMock(status_code=202) + with ( + patch("sendgrid.SendGridAPIClient", return_value=fake_client), + patch.object( + notify.github, "commit_author_email", return_value="culprit@mozilla.com" + ) as author, + ): + notify.send_email(_test_repair_ctx(), run_doc) + + author.assert_called_once_with("abc123def456") + personalizations = fake_client.send.call_args.kwargs["message"].get()[ + "personalizations" + ][0] + assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] + assert personalizations["cc"] == [{"email": "culprit@mozilla.com"}] + + def test_attaches_patch_file(monkeypatch): monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") diff --git a/services/hackbot-pulse-listener/tests/test_regression_test_failure.py b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py new file mode 100644 index 0000000000..d9e0c56dd8 --- /dev/null +++ b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py @@ -0,0 +1,122 @@ +"""Tests for the test-failure regression gate (is_new_test_failure).""" + +from app import regression +from mozci.errors import ParentPushNotFound +from mozci.task import Status + +GROUP = "dom/base/test/mochitest.ini" + + +class FakeSummary: + def __init__(self, status, running=False): + self.status = status + self.running = running + + +class FakePush: + def __init__(self, rev, summaries=None, parent=None): + self.rev = rev + self._summaries = summaries or {} + self._parent = parent + + @property + def group_summaries(self): + return self._summaries + + @property + def parent(self): + if self._parent is None: + raise ParentPushNotFound(f"no parent for {self.rev}") + return self._parent + + def is_group_running(self, summary): + return getattr(summary, "running", False) + + +def _install_head(monkeypatch, head): + monkeypatch.setattr(regression, "Push", lambda rev, branch=None: head) + + +def test_new_failure_when_ancestor_passed(monkeypatch): + parent = FakePush("parentrev", {GROUP: FakeSummary(Status.PASS)}) + head = FakePush("headrev", {}, parent=parent) + _install_head(monkeypatch, head) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green == "parentrev" + + +def test_inherited_when_ancestor_failed(monkeypatch): + parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL)}) + head = FakePush("headrev", {}, parent=parent) + _install_head(monkeypatch, head) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is False + assert last_green is None + + +def test_intermittent_ancestor_is_skipped_until_decisive(monkeypatch): + green = FakePush("greenrev", {GROUP: FakeSummary(Status.PASS)}) + flaky = FakePush( + "flakyrev", {GROUP: FakeSummary(Status.INTERMITTENT)}, parent=green + ) + head = FakePush("headrev", {}, parent=flaky) + _install_head(monkeypatch, head) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green == "greenrev" + + +def test_coalesced_ancestor_without_group_is_skipped(monkeypatch): + green = FakePush("greenrev", {GROUP: FakeSummary(Status.PASS)}) + coalesced = FakePush("coalrev", {}, parent=green) # group never ran here + head = FakePush("headrev", {}, parent=coalesced) + _install_head(monkeypatch, head) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green == "greenrev" + + +def test_running_ancestor_is_pending(monkeypatch): + parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL, running=True)}) + head = FakePush("headrev", {}, parent=parent) + _install_head(monkeypatch, head) + state, _ = regression._classify( + "autoland", + "headrev", + lambda push: regression._group_status(push, GROUP), + f"group {GROUP}", + ) + assert state is regression._PENDING + + +def test_no_ancestor_ran_group_fails_open(monkeypatch): + parent = FakePush("parentrev", {}) # group not present, no further parent + head = FakePush("headrev", {}, parent=parent) + _install_head(monkeypatch, head) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green is None + + +def test_mozci_error_fails_open(monkeypatch): + def boom(rev, branch=None): + raise RuntimeError("mozci exploded") + + monkeypatch.setattr(regression, "Push", boom) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green is None + + +def test_pending_past_deadline_fails_open(monkeypatch): + parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL, running=True)}) + head = FakePush("headrev", {}, parent=parent) + _install_head(monkeypatch, head) + # Force the poll loop to time out immediately without real sleeping. + monkeypatch.setattr(regression.time, "sleep", lambda s: None) + ticks = iter([0.0, regression.MAX_WAIT_SECONDS + 1]) + monkeypatch.setattr(regression.time, "monotonic", lambda: next(ticks)) + is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) + assert is_new is True + assert last_green is None From 8a121cf2c11f7f27da2bdb6f6317a82a67aa86ae Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Fri, 24 Jul 2026 11:48:05 -0700 Subject: [PATCH 2/7] Improve logging and error handling --- .../hackbot-pulse-listener/app/__main__.py | 34 +++++++++++++++++-- services/hackbot-pulse-listener/app/config.py | 1 + .../hackbot-pulse-listener/app/consumer.py | 10 +++--- .../hackbot-pulse-listener/app/flakiness.py | 18 ++++++++++ .../hackbot-pulse-listener/app/regression.py | 4 ++- .../tests/test_flakiness.py | 33 ++++++++++++++++++ 6 files changed, 92 insertions(+), 8 deletions(-) diff --git a/services/hackbot-pulse-listener/app/__main__.py b/services/hackbot-pulse-listener/app/__main__.py index 76d736f138..b0fb1d7695 100644 --- a/services/hackbot-pulse-listener/app/__main__.py +++ b/services/hackbot-pulse-listener/app/__main__.py @@ -1,11 +1,18 @@ import logging +import os import signal from concurrent.futures import ThreadPoolExecutor from app import consumer from app.config import settings -logging.basicConfig(level=logging.INFO) +logging.basicConfig( + level=settings.log_level.upper(), + format="%(asctime)s %(levelname)-7s %(name)s: %(message)s", + datefmt="%Y-%m-%d %H:%M:%S", +) +# httpx logs every request at INFO, which drowns out our own lines. +logging.getLogger("httpx").setLevel(logging.WARNING) logger = logging.getLogger(__name__) @@ -30,14 +37,35 @@ def shutdown(signum, _frame): signal.signal(signal.SIGINT, shutdown) logger.info( - "Listening for build failures on %s; watched repos: %s", + "Listening for build and test failures on %s; watched repos: %s%s", ", ".join(consumer.EXCHANGES), sorted(settings.watched_repos_set), + " (DRY RUN: no agent runs will be triggered)" if settings.dry_run else "", ) + code = 0 try: consumer_obj.run() + except Exception: + logger.exception("Consumer loop failed") + code = 1 finally: - executor.shutdown(wait=False) + _exit_now(executor, code) + + +def _exit_now(executor: ThreadPoolExecutor, code: int) -> None: + """Drop in-flight work and exit immediately. + + Worker threads block for a long time by design (a regression check waits up + to an hour for an ancestor push, run polling up to ``run_max_age_minutes``), + and ThreadPoolExecutor's threads are non-daemon: its atexit hook joins them, + so a normal exit would hang for hours and Ctrl+C would appear to do nothing. + That hook also breaks any thread still submitting to a pool mid-shutdown. + In-flight work is disposable (pending runs are only tracked in memory), so + abandon it rather than wait. + """ + executor.shutdown(wait=False, cancel_futures=True) + logging.shutdown() + os._exit(code) if __name__ == "__main__": diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 886862c88c..8cec247e4b 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -58,6 +58,7 @@ class Settings(BaseSettings): notify_only_with_patch: bool = True dry_run: bool = False + log_level: str = "INFO" environment: str = "development" sentry_dsn: str | None = None diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index b7aac2b662..3677bfc349 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -56,6 +56,7 @@ def process(body: dict, executor: Executor) -> str | None: project = tags.get("project") if project not in settings.watched_repos_set: + logger.debug("Ignoring failure on unwatched project %s", project) return None task_label = tags.get("label") or "" @@ -63,6 +64,7 @@ def process(body: dict, executor: Executor) -> str | None: return _process_build(body, tags, executor) if _is_test_task(tags): return _process_test(body, tags, executor) + logger.debug("Ignoring non-build, non-test task %s", task_label) return None @@ -129,8 +131,8 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: return None logger.info( - "Triggered build-repair run %s for %s@%s (git %s)", - run_id, + "%s build-repair for %s@%s (git %s)", + f"Triggered run {run_id}" if run_id else "Would trigger", project, hg_revision, git_commit, @@ -266,8 +268,8 @@ def _trigger_test_repair( return None logger.info( - "Triggered test-repair run %s for %s task %s (%d group(s)) at %s", - run_id, + "%s test-repair for %s task %s (%d group(s)) at %s", + f"Triggered run {run_id}" if run_id else "Would trigger", project, task_id, len(fresh), diff --git a/services/hackbot-pulse-listener/app/flakiness.py b/services/hackbot-pulse-listener/app/flakiness.py index 7519788d84..079c6a58ca 100644 --- a/services/hackbot-pulse-listener/app/flakiness.py +++ b/services/hackbot-pulse-listener/app/flakiness.py @@ -187,6 +187,24 @@ def get_flakiness( """ try: data = _fetch_bucket(harness, _chunk_index(test_path), repo) + except httpx.HTTPStatusError as exc: + # Only some harnesses publish a timings dataset; a 404 means there is + # nothing to look up, so the gate passes the test through unjudged. + if exc.response.status_code == 404: + logger.info( + "No tests.firefox.dev dataset for harness %s; " + "skipping the intermittent check for %s", + harness, + test_path, + ) + else: + logger.warning( + "tests.firefox.dev lookup failed for %s (%s): %s", + test_path, + harness, + exc, + ) + return Flakiness() except Exception: logger.exception( "tests.firefox.dev lookup failed for %s (%s)", test_path, harness diff --git a/services/hackbot-pulse-listener/app/regression.py b/services/hackbot-pulse-listener/app/regression.py index 8a99dbadf6..ed7761b386 100644 --- a/services/hackbot-pulse-listener/app/regression.py +++ b/services/hackbot-pulse-listener/app/regression.py @@ -104,7 +104,9 @@ def _await_new_failure(branch: str, rev: str, status_fn, describe: str): return state, last_green if time.monotonic() >= deadline: break - logger.info( + # _classify already logged which ancestor is unsettled; keep the + # per-poll heartbeat at debug so a long wait isn't dozens of lines. + logger.debug( "Waiting %ss for an unsettled ancestor of %s (%s)", POLL_INTERVAL_SECONDS, describe, diff --git a/services/hackbot-pulse-listener/tests/test_flakiness.py b/services/hackbot-pulse-listener/tests/test_flakiness.py index b31d057ebb..573393cf2c 100644 --- a/services/hackbot-pulse-listener/tests/test_flakiness.py +++ b/services/hackbot-pulse-listener/tests/test_flakiness.py @@ -83,3 +83,36 @@ def boom(*a, **k): stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") assert stats.total == 0 assert stats.failure_rate == 0.0 + + +def _http_error(status_code): + request = flakiness.httpx.Request("GET", "https://example/timings.json") + response = flakiness.httpx.Response(status_code, request=request) + return flakiness.httpx.HTTPStatusError("boom", request=request, response=response) + + +def test_unpublished_harness_404_is_not_an_error(monkeypatch, caplog): + # Only some harnesses publish a timings dataset (e.g. reftest does not). + # A 404 is an expected "no data", not an error worth a traceback/Sentry event. + def not_found(*a, **k): + raise _http_error(404) + + monkeypatch.setattr(flakiness, "_fetch_bucket", not_found) + with caplog.at_level("DEBUG"): + stats = flakiness.get_flakiness("editor/reftests/a.html", "reftest") + + assert stats.total == 0 + assert not [r for r in caplog.records if r.levelname in ("ERROR", "WARNING")] + assert any(r.exc_info for r in caplog.records) is False + + +def test_unexpected_http_status_warns_without_traceback(monkeypatch, caplog): + def server_error(*a, **k): + raise _http_error(500) + + monkeypatch.setattr(flakiness, "_fetch_bucket", server_error) + with caplog.at_level("DEBUG"): + stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") + + assert stats.total == 0 + assert [r for r in caplog.records if r.levelname == "WARNING"] From 51b07ea60efc52e99916da67e245f39171eae393 Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Fri, 24 Jul 2026 13:33:15 -0700 Subject: [PATCH 3/7] Less noise in logs --- .../hackbot-pulse-listener/app/__main__.py | 9 +-- services/hackbot-pulse-listener/app/config.py | 3 + .../app/logging_setup.py | 57 +++++++++++++++++++ 3 files changed, 62 insertions(+), 7 deletions(-) create mode 100644 services/hackbot-pulse-listener/app/logging_setup.py diff --git a/services/hackbot-pulse-listener/app/__main__.py b/services/hackbot-pulse-listener/app/__main__.py index b0fb1d7695..5ffc2a4123 100644 --- a/services/hackbot-pulse-listener/app/__main__.py +++ b/services/hackbot-pulse-listener/app/__main__.py @@ -3,16 +3,11 @@ import signal from concurrent.futures import ThreadPoolExecutor +# Configures logging on import; must precede the imports that pull in mozci. +import app.logging_setup # noqa: F401 from app import consumer from app.config import settings -logging.basicConfig( - level=settings.log_level.upper(), - format="%(asctime)s %(levelname)-7s %(name)s: %(message)s", - datefmt="%Y-%m-%d %H:%M:%S", -) -# httpx logs every request at INFO, which drowns out our own lines. -logging.getLogger("httpx").setLevel(logging.WARNING) logger = logging.getLogger(__name__) diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 8cec247e4b..41e4cd7a5e 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -59,6 +59,9 @@ class Settings(BaseSettings): dry_run: bool = False log_level: str = "INFO" + # mozci's own (loguru) logging. Its per-task "missing results" warnings are + # normal operation, not something to act on; lower this to debug mozci. + mozci_log_level: str = "ERROR" environment: str = "development" sentry_dsn: str | None = None diff --git a/services/hackbot-pulse-listener/app/logging_setup.py b/services/hackbot-pulse-listener/app/logging_setup.py new file mode 100644 index 0000000000..fbbb6fab71 --- /dev/null +++ b/services/hackbot-pulse-listener/app/logging_setup.py @@ -0,0 +1,57 @@ +"""Logging configuration, applied on import. + +Import this before anything that pulls in mozci. mozci logs through loguru, +which writes straight to stderr in its own format and ignores stdlib logging +levels, and it emits lines while being imported -- so configuring later would +both miss those lines and leave its per-task chatter unfiltered. +""" + +import logging + +from app.config import settings + + +def _forward_to_stdlib(message) -> None: + """Re-emit a loguru record through stdlib logging (levels are numbered alike).""" + record = message.record + logging.getLogger(record["name"] or "mozci").log( + record["level"].no, record["message"] + ) + + +def configure() -> None: + logging.basicConfig( + level=settings.log_level.upper(), + format="%(asctime)s %(levelname)-7s %(name)s: %(message)s", + datefmt="%Y-%m-%d %H:%M:%S", + ) + # httpx logs every request at INFO, which drowns out our own lines. + logging.getLogger("httpx").setLevel(logging.WARNING) + _capture_loguru() + + +def _capture_loguru() -> None: + """Route mozci's loguru output into stdlib logging at ``mozci_log_level``. + + mozci warns per task about groups missing from a task's errorsummary, dozens + of times a minute; that is inherent to how it reads CI data and is not + actionable here. Importing ``mozci`` runs its own ``setup_logging()``, which + removes every loguru sink and installs its own on stderr, so the import is + bracketed: drop the default sink first (silencing what mozci logs while being + imported), then take the sink back from it. + """ + try: + from loguru import logger as loguru_logger + except ImportError: + return + + loguru_logger.remove() + try: + import mozci # noqa: F401 + except ImportError: + pass + loguru_logger.remove() + loguru_logger.add(_forward_to_stdlib, level=settings.mozci_log_level.upper()) + + +configure() From 0820585e4f07f806d2178b51163937db72bfc33c Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Fri, 24 Jul 2026 13:34:07 -0700 Subject: [PATCH 4/7] Fix test harness issue --- .../hackbot-pulse-listener/app/consumer.py | 30 +++++++++++++------ .../tests/test_consumer.py | 26 ++++++++++++++-- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index 3677bfc349..c350411907 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -150,14 +150,20 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: return run_id -def _harness(tags: dict, label: str) -> str: - """The tests.firefox.dev harness key for a test task.""" - suite = tags.get("test-suite") or "" - if "xpcshell" in suite or "xpcshell" in label: +def _harness(tags: dict, label: str) -> str | None: + """The tests.firefox.dev harness for a test task, or None if it publishes none. + + Only the mochitest and xpcshell timings datasets exist, so every other + harness has no intermittent data to check against. The Taskcluster ``kind`` + is not a harness name (it is often just "test"), so both the suite tag and + the label are matched -- the same suite arrives tagged either way. + """ + text = f"{tags.get('test-suite') or ''} {label}" + if "xpcshell" in text: return "xpcshell" - if "mochitest" in suite: + if "mochitest" in text or "browser-chrome" in text: return "mochitest" - return tags.get("kind") or suite + return None def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: @@ -204,13 +210,19 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: ) -def _fresh_groups(groups, project: str, hg_revision: str, harness: str): +def _fresh_groups(groups, project: str, hg_revision: str, harness: str | None): """Groups that are genuine, non-intermittent regressions worth investigating.""" + if harness is None: + logger.info( + "No tests.firefox.dev dataset for this harness; " + "skipping the intermittent check at %s", + hg_revision, + ) fresh = [] for fg in groups: # Cheap intermittent gate first (one HTTP call) before the mozci walk. - flak = flakiness.get_flakiness(fg.test, harness) - if flak.total and flak.failure_rate >= settings.flakiness_threshold: + flak = flakiness.get_flakiness(fg.test, harness) if harness else None + if flak and flak.total and flak.failure_rate >= settings.flakiness_threshold: logger.info( "Test %s is intermittent (failure rate %.2f); skipping", fg.test, diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index f4e484bd76..26404fd58f 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -391,10 +391,17 @@ def test_harness_detection(): consumer._harness({"test-suite": "mochitest-browser-chrome"}, "l") == "mochitest" ) - # Falls back to the kind when neither xpcshell nor mochitest matches. + # Recognized from the label too: the same suite arrives with kind "test" and + # sometimes no test-suite tag, and "test" is not a harness. assert ( - consumer._harness({"kind": "web-platform-tests"}, "l") == "web-platform-tests" + consumer._harness( + {"kind": "test"}, "test-linux2404-64/opt-mochitest-browser-chrome-8" + ) + == "mochitest" ) + # Harnesses that publish no timings dataset resolve to None (no lookup). + assert consumer._harness({"kind": "web-platform-tests"}, "l") is None + assert consumer._harness({"kind": "test"}, "test-linux/opt-reftest-1") is None def test_multiple_failing_groups_trigger_one_run_per_task(monkeypatch): @@ -453,3 +460,18 @@ def test_queue_name_omits_production_environment(): with patch.object(consumer.settings, "environment", "production"): (queue,) = consumer._build_queues("guest") assert queue.name == "queue/guest/build-repair-task-failed" + + +def test_no_dataset_harness_skips_flakiness_lookup(): + # reftest/wpt publish no timings dataset, so the lookup would be a guaranteed + # 404; the group is still evaluated for being a regression. + with ( + patch.object(consumer.flakiness, "get_flakiness") as get_flakiness, + patch.object( + consumer.regression, "is_new_test_failure", return_value=(True, "green") + ), + ): + fresh = consumer._fresh_groups([_GROUP], "autoland", "hgrev", None) + + get_flakiness.assert_not_called() + assert fresh == [_GROUP] From ae051d6c2b9c0161638b99f62fd5e353dbbaaa58 Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Tue, 28 Jul 2026 14:35:16 -0700 Subject: [PATCH 5/7] Correctness fixes and simplification --- services/hackbot-pulse-listener/README.md | 22 +- services/hackbot-pulse-listener/app/client.py | 11 +- services/hackbot-pulse-listener/app/config.py | 9 +- .../hackbot-pulse-listener/app/consumer.py | 208 ++++++------ .../hackbot-pulse-listener/app/failures.py | 45 ++- .../hackbot-pulse-listener/app/flakiness.py | 184 ++++++----- services/hackbot-pulse-listener/app/models.py | 8 +- services/hackbot-pulse-listener/app/notify.py | 140 ++++---- .../hackbot-pulse-listener/app/regression.py | 219 +++++++----- .../tests/test_consumer.py | 311 ++++++++---------- .../tests/test_failures.py | 52 ++- .../tests/test_flakiness.py | 124 ++++++- .../tests/test_notify.py | 47 ++- .../tests/test_regression_test_failure.py | 216 ++++++++---- 14 files changed, 962 insertions(+), 634 deletions(-) diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index 1bdf0b812b..27702501cc 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -6,23 +6,25 @@ hackbot-api. It deliberately holds no investigation logic: each agent resolves t and commits itself, so the listener only decides _what to hand off_. When a run finishes (minutes later) the listener polls the result and emails a report. -Failed **build** tasks go to `build-repair`; failed **test** tasks go to -`test-repair` (test-repair). +Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-repair`. ## How it works 1. **Subscribe.** Consume `task-failed` messages from `pulse.mozilla.org`. 2. **Filter** to a watched `project` (`WATCHED_REPOS`, default `autoland`), then by task kind: build tasks (compile/link errors) take the build-repair path; test tasks take the - test-repair path. Fetch the task definition for `GECKO_HEAD_REV` (not in the message). - - _Build:_ keep only failures this push introduced (not inherited from an ancestor). - - _Test:_ resolve the failing test groups (mozci), then keep only groups that are a - genuine regression (not inherited) and not intermittent (tests.firefox.dev flakiness - below `FLAKINESS_THRESHOLD`). + test-repair path. Fetch the task definition for `GECKO_HEAD_REV` (not in the message), + and for a test task the failing groups (mozci). 3. **Dedupe** with in-memory TTL caches: build-repair once per revision; test-repair once per - `(push, test group)`, so a manifest failing across chunks is investigated once. One - test-repair run per task carries only the task id. -4. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until + `(push, test group)`, so a manifest failing across chunks is investigated once. Groups are + claimed before the checks below, so sibling chunks never repeat them. +4. **Judge** whether the failure is worth a run. Both paths fail open — a mozci or + Taskcluster error runs the agent rather than dropping a possible regression. + - _Build:_ keep only failures this push introduced (not inherited from an ancestor). + - _Test:_ keep only groups that are new for this task's own configuration and not + intermittent (tests.firefox.dev flakiness below `FLAKINESS_THRESHOLD`). One run per + task, carrying only the task id. +5. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until terminal, then email a hackbot UI link, the analysis summary, a Treeherder link, and the commit the agent blamed. Build-repair mails the blamed commit's author; test-repair mails the notification address (`TEST_REPAIR_NOTIFICATION_EMAIL`). diff --git a/services/hackbot-pulse-listener/app/client.py b/services/hackbot-pulse-listener/app/client.py index e09ca7a5ef..ff6b1d1379 100644 --- a/services/hackbot-pulse-listener/app/client.py +++ b/services/hackbot-pulse-listener/app/client.py @@ -14,8 +14,17 @@ def _headers() -> dict[str, str]: def trigger_run(inputs: dict, agent_name: str | None = None) -> str | None: - """Create an agent run. Returns the run id, or None in dry-run mode.""" + """Create an agent run. Returns the run id, or None in dry-run mode. + + The model/turn overrides are settings shared by every agent, so they are added + here rather than by each caller. + """ agent = agent_name or settings.agent_name + inputs = dict(inputs) + if settings.model: + inputs["model"] = settings.model + if settings.max_turns is not None: + inputs["max_turns"] = settings.max_turns if settings.dry_run: logger.info("[dry-run] would trigger %s run: %s", agent, inputs) return None diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 41e4cd7a5e..ea14e178bd 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -27,9 +27,12 @@ class Settings(BaseSettings): run_try_push: bool = False model: str | None = None max_turns: int | None = None - # Skip a failing test whose historical cross-push failure rate is at or above - # this (clearly intermittent) before spending an test-repair run. - flakiness_threshold: float = 0.2 + # Skip a failing test whose historical failure rate is at or above this + # (clearly intermittent) before spending a test-repair run. The rate is per + # *run*, over every platform in the timings window (three weeks, thousands of + # runs per test), where even the flakiest tests sit near a few percent -- so + # this is an order of magnitude lower than a per-push failure rate would be. + flakiness_threshold: float = 0.05 # Dedupe (in-memory, by hg revision) dedupe_ttl_seconds: int = 6 * 60 * 60 diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index c350411907..ba97e8673e 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -68,8 +68,15 @@ def process(body: dict, executor: Executor) -> str | None: return None +def _release(cache: TTLCache, lock: threading.Lock, keys) -> None: + """Give up claimed dedupe keys so a later message can retry them.""" + with lock: + for key in keys: + cache.pop(key, None) + + def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: - """Build-failure path: trigger the build-repair agent (unchanged behavior).""" + """Build-failure path: trigger the build-repair agent.""" project = tags.get("project") task_label = tags.get("label") or "" task_id = body["status"]["taskId"] @@ -109,25 +116,19 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: task_id, project, ) - with _seen_lock: - _seen.pop(hg_revision, None) + _release(_seen, _seen_lock, [hg_revision]) return None - inputs: dict = { - "failure_tasks": {task_name: task_id}, - "run_try_push": settings.run_try_push, - } - if settings.model: - inputs["model"] = settings.model - if settings.max_turns is not None: - inputs["max_turns"] = settings.max_turns - try: - run_id = client.trigger_run(inputs) + run_id = client.trigger_run( + { + "failure_tasks": {task_name: task_id}, + "run_try_push": settings.run_try_push, + } + ) except Exception: logger.exception("Failed to trigger build-repair run for %s", hg_revision) - with _seen_lock: - _seen.pop(hg_revision, None) + _release(_seen, _seen_lock, [hg_revision]) return None logger.info( @@ -150,22 +151,6 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: return run_id -def _harness(tags: dict, label: str) -> str | None: - """The tests.firefox.dev harness for a test task, or None if it publishes none. - - Only the mochitest and xpcshell timings datasets exist, so every other - harness has no intermittent data to check against. The Taskcluster ``kind`` - is not a harness name (it is often just "test"), so both the suite tag and - the label are matched -- the same suite arrives tagged either way. - """ - text = f"{tags.get('test-suite') or ''} {label}" - if "xpcshell" in text: - return "xpcshell" - if "mochitest" in text or "browser-chrome" in text: - return "mochitest" - return None - - def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: """Test-failure path: filter, then trigger the test-repair agent for the task. @@ -181,122 +166,151 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: project = tags.get("project") label = tags.get("label") or task_id developer_email = tags.get("createdForUser") - harness = _harness(tags, label) hg_revision = taskcluster.get_hg_revision(task_id) if not hg_revision: logger.warning("No GECKO_HEAD_REV for test task %s; skipping", task_id) return None - groups = failures.failing_groups(task_id) + try: + groups = failures.failing_groups(task_id, label) + except Exception: + # Fail open, as the build path does. An errorsummary that cannot be read + # (Taskcluster error, or the artifact not published yet when the failure + # message arrives) must not be mistaken for "nothing failed": investigate + # the whole task, keyed on its label since no group name is known. + logger.exception( + "Could not resolve the failing groups of task %s; " + "running the agent on the whole task", + task_id, + ) + if not _claim_groups([(task_group_id, label)]): + return None + return _trigger_test_repair( + [], + [(task_group_id, label)], + project, + hg_revision, + task_id, + label, + developer_email, + executor, + ) + if not groups: - logger.info("No failing test groups resolved for task %s; skipping", task_id) + logger.info("Task %s reported no failing test groups; skipping", task_id) return None - fresh = _fresh_groups(groups, project, hg_revision, harness) - if not fresh: - logger.info("No new, non-intermittent groups for task %s; skipping", task_id) + claimed = _claim_groups([(task_group_id, fg.group) for fg in groups]) + if not claimed: + logger.info( + "Every failing group of task %s is already handled; skipping", task_id + ) return None - return _trigger_test_repair( - fresh, + fresh = _fresh_groups( + [fg for fg in groups if (task_group_id, fg.group) in claimed], project, hg_revision, - task_group_id, - task_id, label, - developer_email, - executor, + tags.get("test-suite") or "", ) + if not fresh: + logger.info("No new, non-intermittent groups for task %s; skipping", task_id) + return None + return _trigger_test_repair( + fresh, claimed, project, hg_revision, task_id, label, developer_email, executor + ) -def _fresh_groups(groups, project: str, hg_revision: str, harness: str | None): - """Groups that are genuine, non-intermittent regressions worth investigating.""" - if harness is None: - logger.info( - "No tests.firefox.dev dataset for this harness; " - "skipping the intermittent check at %s", - hg_revision, - ) - fresh = [] - for fg in groups: - # Cheap intermittent gate first (one HTTP call) before the mozci walk. - flak = flakiness.get_flakiness(fg.test, harness) if harness else None - if flak and flak.total and flak.failure_rate >= settings.flakiness_threshold: - logger.info( - "Test %s is intermittent (failure rate %.2f); skipping", - fg.test, - flak.failure_rate, - ) - continue - is_new, _ = regression.is_new_test_failure(project, hg_revision, fg.group) - if not is_new: + +def _claim_groups(keys: list[tuple[str, str]]) -> set[tuple[str, str]]: + """Claim the keys not yet handed off, returning the ones this call claimed. + + Claimed *before* the flakiness and mozci checks, not after: every failing chunk + of a push resolves the same groups, so without an up-front claim they all run + the expensive walk concurrently and then throw the answer away. A key stays + claimed even when the filter then rejects the group, so the sibling tasks do + not recompute a verdict that would come out the same. + """ + with _seen_tests_lock: + claimed = {k for k in keys if k not in _seen_tests} + for k in claimed: + _seen_tests[k] = True + return claimed + + +def _fresh_groups( + groups, project: str, hg_revision: str, label: str, suite: str +) -> list[str]: + """Names of the groups that are genuine, non-intermittent regressions.""" + # Cheap intermittent gate first (one HTTP call per test) before the mozci walk. + flaky = flakiness.intermittent_tests([fg.test for fg in groups], suite, label) + candidates = [fg.group for fg in groups if fg.test not in flaky] + if not candidates: + return [] + + new = regression.new_test_failures(project, hg_revision, label, candidates) + for group in candidates: + if group not in new: logger.info( "Group %s at %s inherited from an ancestor; skipping", - fg.group, + group, hg_revision, ) - continue - fresh.append(fg) - return fresh + return [group for group in candidates if group in new] def _trigger_test_repair( - fresh, + test_groups: list[str], + claimed, project: str, hg_revision: str, - task_group_id: str, task_id: str, label: str, developer_email: str | None, executor: Executor, ) -> str | None: - # Dedupe per (push, group): trigger only when this task has a fresh group not - # already handed off (e.g. by a sibling chunk), then claim all of them. - keys = [(task_group_id, fg.group) for fg in fresh] - with _seen_tests_lock: - unseen = [k for k in keys if k not in _seen_tests] - if not unseen: - logger.info( - "All fresh groups in task %s already triggered; skipping", task_id - ) - return None - for k in unseen: - _seen_tests[k] = True - - inputs: dict = {"failure_tasks": {label: task_id}} - if settings.model: - inputs["model"] = settings.model - if settings.max_turns is not None: - inputs["max_turns"] = settings.max_turns - try: - run_id = client.trigger_run(inputs, agent_name=settings.test_repair_agent_name) + run_id = client.trigger_run( + {"failure_tasks": {label: task_id}}, + agent_name=settings.test_repair_agent_name, + ) except Exception: logger.exception("Failed to trigger test-repair run for task %s", task_id) - with _seen_tests_lock: - for k in unseen: - _seen_tests.pop(k, None) + _release(_seen_tests, _seen_tests_lock, claimed) return None logger.info( - "%s test-repair for %s task %s (%d group(s)) at %s", + "%s test-repair for %s task %s (%s) at %s", f"Triggered run {run_id}" if run_id else "Would trigger", project, task_id, - len(fresh), + f"{len(test_groups)} group(s)" if test_groups else "groups unresolved", hg_revision, ) if run_id is not None: + git_commit = lando.hg_to_git(hg_revision) + if not git_commit: + # Not fatal, unlike on the build path: the agent works from the task id + # alone, and a revision Lando has not mirrored yet is routine for a + # just-landed push. The notification omits the git revision instead of + # linking to an empty commit. + logger.warning( + "Could not map hg revision %s to git for task %s; " + "the notification will omit the git revision", + hg_revision, + task_id, + ) ctx = RunContext( run_id=run_id, repo=project, - git_commit=lando.hg_to_git(hg_revision) or "", + git_commit=git_commit or "", hg_revision=hg_revision, task_id=task_id, developer_email=developer_email, agent=settings.test_repair_agent_name, - test_name=fresh[0].group, + test_groups=list(test_groups), ) executor.submit(worker.poll_and_notify, ctx) return run_id diff --git a/services/hackbot-pulse-listener/app/failures.py b/services/hackbot-pulse-listener/app/failures.py index 0663b1abce..8807c618cb 100644 --- a/services/hackbot-pulse-listener/app/failures.py +++ b/services/hackbot-pulse-listener/app/failures.py @@ -14,7 +14,7 @@ from dataclasses import dataclass import mozci.push # noqa: F401 (imported so mozci registers its data sources) -from mozci import data +from mozci.task import TestTask, is_bad_group, wpt_workaround logger = logging.getLogger(__name__) @@ -28,27 +28,46 @@ class FailingGroup: failure_type: str -def _failure_types(task_id: str) -> dict: +def _failure_types(task: TestTask) -> dict: """mozci: ``{group: [(test_name, FailureType), ...]}`` for a task.""" - return data.handler.get("test_task_failure_types", task_id=task_id) + return task.failure_types -def failing_groups(task_id: str) -> list[FailingGroup]: - """Failing test groups for a task; empty on any error (gate fails open).""" - try: - by_group = _failure_types(task_id) - except Exception: - logger.exception("Could not read failing groups for task %s", task_id) - return [] +def _canonical_group(task: TestTask, group: str) -> str: + """Rewrite a group name the way mozci does. + The errorsummary names web-platform-tests groups by URL path ("/html/foo.html") + while mozci keys the results we compare against by source path + ("testing/web-platform/tests/html/foo.html"), so the regression check only ever + matches if we apply the same transform. + """ + if task.is_wpt and group.startswith((":/", "/")): + return wpt_workaround(group) + return group + + +def failing_groups(task_id: str, label: str) -> list[FailingGroup]: + """Failing test groups for a task, named the way mozci names them. + + Raises on any mozci/network error rather than returning nothing: an + errorsummary that cannot be read must not be mistaken for "nothing failed", + which would silently drop a real regression. + """ + task = TestTask(id=task_id, label=label) groups: list[FailingGroup] = [] - for group, fails in by_group.items(): - if not group or not fails: + for group, fails in _failure_types(task).items(): + if not group or not fails or group == "/": + continue + canonical = _canonical_group(task, group) + # mozci drops these from its own results, so they could never match. + if is_bad_group(task_id, canonical): continue test, ftype = fails[0] groups.append( FailingGroup( - group=group, test=test, failure_type=getattr(ftype, "name", str(ftype)) + group=canonical, + test=test, + failure_type=getattr(ftype, "name", str(ftype)), ) ) return groups diff --git a/services/hackbot-pulse-listener/app/flakiness.py b/services/hackbot-pulse-listener/app/flakiness.py index 079c6a58ca..f3bdfd3c36 100644 --- a/services/hackbot-pulse-listener/app/flakiness.py +++ b/services/hackbot-pulse-listener/app/flakiness.py @@ -1,4 +1,4 @@ -"""tests.firefox.dev flakiness lookup for the test-repair gate. +"""tests.firefox.dev flakiness gate for test-repair. The tests.firefox.dev dashboard (github.com/mozilla/aretestsfastyet) is static JS that reads pre-aggregated timings JSON from the Firefox CI Taskcluster index at @@ -15,9 +15,14 @@ from __future__ import annotations import logging +import threading +from collections.abc import Iterable from dataclasses import dataclass import httpx +from cachetools import TTLCache + +from app.config import settings logger = logging.getLogger(__name__) @@ -28,6 +33,15 @@ _TIMEOUT = 30 _TOTAL_CHUNKS = 64 +# Decoded buckets, keyed by (harness, chunk, repo). The index publishes a new +# dataset at most a few times a day, and a raw bucket is tens of MB of JSON, so +# caching the (small) decoded stats avoids refetching it for every test. The lock +# covers the fetch and decode together: the worker pool is far larger than the +# number of buckets, and concurrent decodes would dominate the memory limit. +_BUCKET_TTL_SECONDS = 60 * 60 +_buckets: TTLCache = TTLCache(maxsize=32, ttl=_BUCKET_TTL_SECONDS) +_buckets_lock = threading.Lock() + @dataclass(frozen=True) class Flakiness: @@ -39,9 +53,6 @@ class Flakiness: timeouts: int = 0 crashes: int = 0 skips: int = 0 - # Most recent day offset (relative to metadata.startDate) the test was green, - # or None if it never passed in the window. Coarse context only. - last_green_day: int | None = None @property def failure_rate(self) -> float: @@ -52,6 +63,22 @@ def failure_rate(self) -> float: return (self.fails + self.timeouts + self.crashes) / denom +def _harness(suite: str, label: str) -> str | None: + """The timings harness for a test task, or None if it publishes none. + + Only the mochitest and xpcshell datasets exist, so every other harness has no + intermittent data to check against. The Taskcluster ``kind`` is not a harness + name (it is often just "test"), so both the suite tag and the label are matched + -- the same suite arrives tagged either way. + """ + text = f"{suite or ''} {label or ''}" + if "xpcshell" in text: + return "xpcshell" + if "mochitest" in text or "browser-chrome" in text: + return "mochitest" + return None + + def _to_int32(n: int) -> int: """Wrap to a signed 32-bit int, matching JavaScript's ``| 0``.""" n &= 0xFFFFFFFF @@ -74,34 +101,16 @@ def _fetch_bucket(harness: str, chunk: int, repo: str) -> dict: return resp.json() -def _decompress_days(days: list[int]) -> list[int]: - """Days are stored as offsets from the previous entry; return absolute days.""" - out: list[int] = [] - acc = 0 - for d in days: - acc += d - out.append(acc) - return out - - -def _group_len(group: dict) -> int: - for key in ("counts", "durations", "taskIdIds"): +def _run_count(group: dict) -> int: + """Runs recorded in a status group; port of ``getCountAtIndex`` over all indices.""" + if "counts" in group: + return sum(group["counts"]) + for key in ("durations", "taskIdIds"): if key in group: - return len(group[key]) + return sum(len(entry) for entry in group[key]) return len(group.get("days") or []) -def _count_at(group: dict, i: int) -> int: - """Runs recorded at index ``i`` of a status group; port of ``getCountAtIndex``.""" - if "counts" in group: - return group["counts"][i] - if "durations" in group: - return len(group["durations"][i]) - if "taskIdIds" in group: - return len(group["taskIdIds"][i]) - return 1 - - def _classify(status: str) -> str | None: """Map a status string to pass/fail/timeout/crash/skip (None = ignored).""" if status == "SKIP": @@ -118,63 +127,57 @@ def _classify(status: str) -> str | None: return "fail" -def _find_test_id(data: dict, test_path: str) -> int | None: +def _test_paths(data: dict) -> dict[int, str]: + """Full test path per test id, from the bucket's index tables.""" tables = data.get("tables") or {} info = data.get("testInfo") or {} - runs = data.get("testRuns") or [] paths = tables.get("testPaths") or [] names = tables.get("testNames") or [] path_ids = info.get("testPathIds") or [] name_ids = info.get("testNameIds") or [] - for test_id, group in enumerate(runs): - if not group or test_id >= len(path_ids) or test_id >= len(name_ids): - continue + out = {} + for test_id in range(min(len(path_ids), len(name_ids))): dir_path = paths[path_ids[test_id]] - test_name = names[name_ids[test_id]] - full = f"{dir_path}/{test_name}" if dir_path else test_name - if full == test_path: - return test_id - return None + name = names[name_ids[test_id]] + out[test_id] = f"{dir_path}/{name}" if dir_path else name + return out -def _compute_stats(data: dict, test_path: str) -> Flakiness: - """Aggregate a test's status groups into a :class:`Flakiness`.""" +def _decode_bucket(data: dict) -> dict[str, Flakiness]: + """Per-test stats for every test in a bucket.""" statuses = (data.get("tables") or {}).get("statuses") or [] - test_id = _find_test_id(data, test_path) - if test_id is None: - return Flakiness() - test_group = (data.get("testRuns") or [])[test_id] - - counts = {"pass": 0, "fail": 0, "timeout": 0, "crash": 0, "skip": 0} - pass_days: set[int] = set() - bad_days: set[int] = set() - for status_id, group in enumerate(test_group): - if not group or status_id >= len(statuses): - continue - kind = _classify(statuses[status_id]) - if kind is None: + paths = _test_paths(data) + out: dict[str, Flakiness] = {} + for test_id, test_group in enumerate(data.get("testRuns") or []): + path = paths.get(test_id) + if not test_group or path is None: continue - n = _group_len(group) - days = _decompress_days(group.get("days") or list(range(n))) - for i in range(n): - c = _count_at(group, i) - counts[kind] += c - if c > 0 and i < len(days): - if kind == "pass": - pass_days.add(days[i]) - elif kind in ("fail", "timeout", "crash"): - bad_days.add(days[i]) - - green = pass_days - bad_days - return Flakiness( - total=sum(counts.values()), - passes=counts["pass"], - fails=counts["fail"], - timeouts=counts["timeout"], - crashes=counts["crash"], - skips=counts["skip"], - last_green_day=max(green) if green else None, - ) + counts = dict.fromkeys(("pass", "fail", "timeout", "crash", "skip"), 0) + for status_id, group in enumerate(test_group): + if not group or status_id >= len(statuses): + continue + kind = _classify(statuses[status_id]) + if kind is not None: + counts[kind] += _run_count(group) + out[path] = Flakiness( + total=sum(counts.values()), + passes=counts["pass"], + fails=counts["fail"], + timeouts=counts["timeout"], + crashes=counts["crash"], + skips=counts["skip"], + ) + return out + + +def _bucket_stats(harness: str, chunk: int, repo: str) -> dict[str, Flakiness]: + key = (harness, chunk, repo) + with _buckets_lock: + stats = _buckets.get(key) + if stats is None: + stats = _decode_bucket(_fetch_bucket(harness, chunk, repo)) + _buckets[key] = stats + return stats def get_flakiness( @@ -186,7 +189,9 @@ def get_flakiness( listener gate treats the test as non-flaky (and errs toward triggering a run). """ try: - data = _fetch_bucket(harness, _chunk_index(test_path), repo) + return _bucket_stats(harness, _chunk_index(test_path), repo).get( + test_path, Flakiness() + ) except httpx.HTTPStatusError as exc: # Only some harnesses publish a timings dataset; a 404 means there is # nothing to look up, so the gate passes the test through unjudged. @@ -204,10 +209,33 @@ def get_flakiness( harness, exc, ) - return Flakiness() except Exception: logger.exception( "tests.firefox.dev lookup failed for %s (%s)", test_path, harness ) - return Flakiness() - return _compute_stats(data, test_path) + return Flakiness() + + +def intermittent_tests(tests: Iterable[str], suite: str, label: str) -> set[str]: + """The tests whose historical failure rate marks them as clearly intermittent. + + Empty when the task's harness publishes no timings dataset, logged once for the + task rather than once per test. + """ + harness = _harness(suite, label) + if harness is None: + logger.info( + "No tests.firefox.dev dataset for %s; skipping the intermittent check", + label, + ) + return set() + + flaky = set() + for test in tests: + rate = get_flakiness(test, harness).failure_rate + if rate >= settings.flakiness_threshold: + logger.info( + "Test %s is intermittent (failure rate %.2f); skipping", test, rate + ) + flaky.add(test) + return flaky diff --git a/services/hackbot-pulse-listener/app/models.py b/services/hackbot-pulse-listener/app/models.py index 1a8ace1dc4..3863ee2fdb 100644 --- a/services/hackbot-pulse-listener/app/models.py +++ b/services/hackbot-pulse-listener/app/models.py @@ -1,4 +1,4 @@ -from dataclasses import dataclass +from dataclasses import dataclass, field @dataclass @@ -11,7 +11,7 @@ class RunContext: hg_revision: str task_id: str developer_email: str | None - # Which agent produced the run, and (for test-repair) the failing test group. These - # drive the notifier's recipient/body routing. + # Which agent produced the run, and (for test-repair) every failing test group + # the run covers. These drive the notifier's recipient/body routing. agent: str = "build-repair" - test_name: str | None = None + test_groups: list[str] = field(default_factory=list) diff --git a/services/hackbot-pulse-listener/app/notify.py b/services/hackbot-pulse-listener/app/notify.py index 3823faea84..a22a1cbe14 100644 --- a/services/hackbot-pulse-listener/app/notify.py +++ b/services/hackbot-pulse-listener/app/notify.py @@ -62,7 +62,7 @@ def _send_test_repair_email(ctx: RunContext, run_doc: dict) -> None: ) # test-repair verdicts are always notified (including do-not-backout verdicts), so # the build-repair notify_only_with_patch gate does not apply here. - recipients = _test_repair_recipients(culprit_author) + recipients = _recipients(settings.test_repair_notification_email, culprit_author) if not recipients: logger.info( "No recipients for test-repair run %s; skipping notification", ctx.run_id @@ -73,10 +73,9 @@ def _send_test_repair_email(ctx: RunContext, run_doc: dict) -> None: return patch = _fetch_patch(ctx.run_id, run_doc) - banner = _RECOMMENDATION_BANNER.get( - findings.get("recommendation"), findings.get("recommendation") or "analysis" + subject = ( + f"[test-repair] {_banner(findings)} - {_test_groups_label(ctx)} ({ctx.repo})" ) - subject = f"[test-repair] {banner} - {ctx.test_name} ({ctx.repo})" body_md = _build_test_repair_body(ctx, findings, patch, culprit_author) _deliver(subject, body_md, recipients, patch) @@ -127,20 +126,18 @@ def _deliver(subject: str, body_md: str, recipients: list[str], patch: str | Non ) -def _recipients( - author_email: str | None, developer_email: str | None = None -) -> list[str]: - """Recipients for a run, deduped and ordered by priority. +def _recipients(primary: str | None, secondary: str | None = None) -> list[str]: + """Recipients for a run, deduped and ordered by priority, team address last. - The blamed commit's author is the primary To (the person who introduced the - failure), then the pushing developer, then the team address. - ``notification_override_email`` short-circuits to a single address so local - testing never mails real developers or the team. + build-repair puts the blamed commit's author first and the pushing developer + second; test-repair puts its distribution address first and the culprit author + second. ``notification_override_email`` short-circuits to a single address so + local testing never mails real developers or the team. """ if settings.notification_override_email: return [settings.notification_override_email] recipients: list[str] = [] - for addr in (author_email, developer_email, settings.notification_team_email): + for addr in (primary, secondary, settings.notification_team_email): if addr and addr not in recipients: recipients.append(addr) return recipients @@ -153,22 +150,18 @@ def _recipients( } -def _test_repair_recipients(culprit_author: str | None) -> list[str]: - """The test-repair notification address (primary To), the culprit author, then the team. +def _banner(findings: dict) -> str: + """The recommendation as a human-readable headline.""" + recommendation = findings.get("recommendation") + return _RECOMMENDATION_BANNER.get(recommendation, recommendation or "analysis") - ``notification_override_email`` short-circuits to a single address for testing. - """ - if settings.notification_override_email: - return [settings.notification_override_email] - recipients: list[str] = [] - for addr in ( - settings.test_repair_notification_email, - culprit_author, - settings.notification_team_email, - ): - if addr and addr not in recipients: - recipients.append(addr) - return recipients + +def _test_groups_label(ctx: RunContext) -> str: + """A one-line name for the run's failing groups, for the email subject.""" + if not ctx.test_groups: + return f"task {ctx.task_id}" + first, *rest = ctx.test_groups + return f"{first} (+{len(rest)} more)" if rest else first def _build_test_repair_body( @@ -177,16 +170,22 @@ def _build_test_repair_body( patch: str | None, culprit_author: str | None, ) -> str: - recommendation = findings.get("recommendation") - banner = _RECOMMENDATION_BANNER.get(recommendation, recommendation or "analysis") + groups = ", ".join(f"`{g}`" for g in ctx.test_groups) or "not resolved" lines = [ "# Test failure analysis", "", - f"- **Recommendation:** {banner}", - f"- **Failing test:** `{ctx.test_name}`", + f"- **Recommendation:** {_banner(findings)}", + f"- **Failing tests:** {groups}", f"- **Classification:** {findings.get('classification')}", f"- **Repository:** {ctx.repo}", - f"- **Revision (git):** [`{ctx.git_commit[:12]}`]({_git_url(ctx.git_commit)})", + ] + # Omitted rather than linked as an empty commit when Lando has not mirrored the + # revision yet; the hg revision below always identifies the push. + if ctx.git_commit: + lines.append( + f"- **Revision (git):** [`{ctx.git_commit[:12]}`]({_git_url(ctx.git_commit)})" + ) + lines += [ f"- **Revision (hg):** [`{ctx.hg_revision[:12]}`]({_hg_url(ctx.hg_revision)})", f"- **Failed task:** [`{ctx.task_id}`]({_task_url(ctx.task_id)})", f"- **Treeherder:** " @@ -212,26 +211,41 @@ def _build_test_repair_body( if bug: lines.append(f"- **Bug:** [{bug}]({_bug_url(bug)})") - if settings.hackbot_ui_url: - run_url = f"{settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" - lines.append(f"- **Run details:** {run_url}") + lines += _run_details(ctx) + _analysis_sections(findings) + _patch_section(patch) + lines += _team_footer() + return "\n".join(lines) - if findings.get("summary"): - lines += ["", "## Summary", "", _demote_headings(findings["summary"])] - if findings.get("analysis"): - lines += ["", "## Analysis", "", _demote_headings(findings["analysis"])] - if patch: - lines += ["", "## Proposed patch", "", _patch_block(patch)] - if settings.notification_team_email: - lines += [ - "", - "---", - "", - "_Reply to this email with any feedback on this analysis; it reaches " - "the hackbot team._", - ] - return "\n".join(lines) +def _run_details(ctx: RunContext) -> list[str]: + if not settings.hackbot_ui_url: + return [] + return [ + f"- **Run details:** {settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" + ] + + +def _analysis_sections(findings: dict) -> list[str]: + lines: list[str] = [] + for key, title in (("summary", "Summary"), ("analysis", "Analysis")): + if findings.get(key): + lines += ["", f"## {title}", "", _demote_headings(findings[key])] + return lines + + +def _patch_section(patch: str | None) -> list[str]: + return ["", "## Proposed patch", "", _patch_block(patch)] if patch else [] + + +def _team_footer() -> list[str]: + if not settings.notification_team_email: + return [] + return [ + "", + "---", + "", + "_Reply to this email with any feedback on this analysis; it reaches " + "the hackbot team._", + ] def _fetch_patch(run_id: str, run_doc: dict) -> str | None: @@ -301,16 +315,9 @@ def _build_body( if bug_id: lines.append(f"- **Bug:** [{bug_id}]({_bug_url(bug_id)})") - if settings.hackbot_ui_url: - run_url = f"{settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" - lines.append(f"- **Run details:** {run_url}") - + lines += _run_details(ctx) lines += _recipients_note(ctx, blamed_commit, blamed_author) - - if findings.get("summary"): - lines += ["", "## Summary", "", _demote_headings(findings["summary"])] - if findings.get("analysis"): - lines += ["", "## Analysis", "", _demote_headings(findings["analysis"])] + lines += _analysis_sections(findings) if findings.get("local_build_verified") is not None: lines += [ @@ -320,18 +327,7 @@ def _build_body( f"- Local build verified: {findings['local_build_verified']}", ] - if patch: - lines += ["", "## Proposed patch", "", _patch_block(patch)] - - if settings.notification_team_email: - lines += [ - "", - "---", - "", - "_Reply to this email with any feedback on this analysis; it reaches " - "the hackbot team._", - ] - + lines += _patch_section(patch) + _team_footer() return "\n".join(lines) diff --git a/services/hackbot-pulse-listener/app/regression.py b/services/hackbot-pulse-listener/app/regression.py index ed7761b386..6c90b73d4c 100644 --- a/services/hackbot-pulse-listener/app/regression.py +++ b/services/hackbot-pulse-listener/app/regression.py @@ -3,7 +3,6 @@ from mozci.errors import ParentPushNotFound from mozci.push import MAX_DEPTH, Push -from mozci.task import Status logger = logging.getLogger(__name__) @@ -23,45 +22,85 @@ _PENDING = object() +def _label_tasks(push: Push, label: str) -> list: + return [t for t in push.tasks if t.label == label] + + +def _unsettled(tasks: list) -> bool: + """Whether any of these runs may still change outcome (running or auto-retried).""" + return any( + t.state in _UNSETTLED_STATES or t.result in _UNSETTLED_RESULTS for t in tasks + ) + + +def _was_scheduled(push: Push, label: str) -> bool: + """Whether the push originally scheduled the label (results not visible yet).""" + try: + return label in push.scheduled_task_labels + except Exception: + logger.debug("Could not read scheduled task labels for %s", push.rev) + return False + + def _build_status(push: Push, label: str): """'passed'/'failed'/_PENDING/None for a build label on a push. None is non-decisive (coalesced, never scheduled, or a non-pass/fail terminal state), so such gaps are skipped rather than mistaken for an inherited failure. """ - label_tasks = [t for t in push.tasks if t.label == label] - if label_tasks: - if any(t.result in _PASSED_RESULTS for t in label_tasks): + tasks = _label_tasks(push, label) + if tasks: + if any(t.result in _PASSED_RESULTS for t in tasks): return "passed" # Checked before failure: a still-running or auto-retried run may yet turn # green, and any green run wins, so wait rather than inherit prematurely. - if any( - t.state in _UNSETTLED_STATES or t.result in _UNSETTLED_RESULTS - for t in label_tasks - ): + if _unsettled(tasks): return _PENDING - if any(t.failed for t in label_tasks): + if any(t.failed for t in tasks): return "failed" return None # No task here: wait if the build was scheduled (result not visible yet), # skip if it was coalesced away. - try: - scheduled = label in push.scheduled_task_labels - except Exception: - logger.debug("Could not read scheduled task labels for %s", push.rev) - scheduled = False - return _PENDING if scheduled else None + return _PENDING if _was_scheduled(push, label) else None + +def _group_status(push: Push, group: str, label: str): + """'passed'/'failed'/_PENDING/None for a test group on a push, for one label. -def _classify(branch: str, rev: str, status_fn, describe: str): - """Walk ancestors; return (state, last_green_rev). + Only tasks sharing the failing task's label are consulted. push.group_summaries + folds every configuration together and reports FAIL when the manifest is broken + on any platform, which would mask a genuine new failure on this one; a label + pins the platform, suite, variant and chunk. - state is True (new), False (inherited) or _PENDING. status_fn(push) reports - 'passed'/'failed'/_PENDING/None for the failing unit (build label or test - group). A fresh Push per call re-fetches live data for unfinalized pushes. + The precedence matches _build_status: pass, then pending, then fail. """ - ancestor = Push(rev, branch=branch) + tasks = _label_tasks(push, label) + results = [ + r + for t in tasks + for r in (getattr(t, "results", None) or []) + if r.group == group + ] + if any(r.ok for r in results): + return "passed" + if _unsettled(tasks): + return _PENDING + if results: + return "failed" + + # Nothing reported for the group: wait if the label was scheduled here, skip if + # this push never ran it (coalesced, or the manifest was chunked elsewhere). + return _PENDING if not tasks and _was_scheduled(push, label) else None + + +def _classify(head: Push, status_fn, describe: str): + """Walk ancestors of `head`; True (new), False (inherited) or _PENDING. + + status_fn(push) reports 'passed'/'failed'/_PENDING/None for the failing unit + (build label or test group). + """ + ancestor = head for _ in range(MAX_DEPTH): try: ancestor = ancestor.parent @@ -72,91 +111,99 @@ def _classify(branch: str, rev: str, status_fn, describe: str): continue if status is _PENDING: logger.info( - "%s not settled at %s; deferring for %s", describe, ancestor.rev, rev + "%s not settled at %s; deferring for %s", + describe, + ancestor.rev, + head.rev, ) - return _PENDING, None + return _PENDING if status == "failed": logger.info( - "%s already failing at %s; inherited at %s", describe, ancestor.rev, rev + "%s already failing at %s; inherited at %s", + describe, + ancestor.rev, + head.rev, ) - return False, None - logger.info("%s passed at %s; new failure at %s", describe, ancestor.rev, rev) - return True, ancestor.rev + return False + logger.info( + "%s passed at %s; new failure at %s", describe, ancestor.rev, head.rev + ) + return True logger.warning( "No ancestor within %s pushes ran %s; running agent", MAX_DEPTH, describe ) - return True, None + return True -def _await_new_failure(branch: str, rev: str, status_fn, describe: str): - """(is_new, last_green_rev) for `rev`, waiting on an unsettled ancestor. +def _await_new_failures(branch: str, rev: str, status_fn, units, describe: str) -> set: + """The units whose failure `rev` introduced; the rest were inherited. - Fails open ((True, None)) on any error, an ancestor unsettled past the - deadline, or no deciding ancestor within MAX_DEPTH, so a real regression is - never silently dropped. + status_fn(push, unit) reports 'passed'/'failed'/_PENDING/None. One walk serves + every unit: mozci memoizes a push's task list per instance, so sharing the head + push across units fetches each ancestor's (large) task data once instead of once + per unit. + + Fails open -- undecided units counted as new -- on any error, an ancestor still + unsettled past MAX_WAIT_SECONDS, or no deciding ancestor within MAX_DEPTH, so a + real regression is never silently dropped. """ - try: - deadline = time.monotonic() + MAX_WAIT_SECONDS - while True: - state, last_green = _classify(branch, rev, status_fn, describe) - if state is not _PENDING: - return state, last_green - if time.monotonic() >= deadline: - break - # _classify already logged which ancestor is unsettled; keep the - # per-poll heartbeat at debug so a long wait isn't dozens of lines. - logger.debug( - "Waiting %ss for an unsettled ancestor of %s (%s)", - POLL_INTERVAL_SECONDS, + deadline = time.monotonic() + MAX_WAIT_SECONDS + unresolved = list(units) + new: set = set() + while True: + try: + # A fresh head each attempt is what re-reads live data for a push whose + # ancestors have not finished yet. + head = Push(rev, branch=branch) + pending = [] + for unit in unresolved: + state = _classify( + head, lambda push, u=unit: status_fn(push, u), f"{describe} {unit}" + ) + if state is _PENDING: + pending.append(unit) + elif state: + new.add(unit) + except Exception: + logger.exception( + "Regression check failed for %s@%s; running agent", describe, rev + ) + return new | set(unresolved) + + if not pending: + return new + if time.monotonic() >= deadline: + logger.warning( + "%s still unsettled after %ss at %s; running agent", describe, + MAX_WAIT_SECONDS, rev, ) - time.sleep(POLL_INTERVAL_SECONDS) - except Exception: - logger.exception( - "Regression check failed for %s@%s; running agent", describe, rev + return new | set(pending) + # _classify already logged which ancestor is unsettled; keep the per-poll + # heartbeat at debug so a long wait isn't dozens of lines. + logger.debug( + "Waiting %ss for an unsettled ancestor of %s (%s)", + POLL_INTERVAL_SECONDS, + describe, + rev, ) - return True, None - - logger.warning( - "%s still unsettled after %ss at %s; running agent", - describe, - MAX_WAIT_SECONDS, - rev, - ) - return True, None + time.sleep(POLL_INTERVAL_SECONDS) + unresolved = pending def is_new_build_failure(branch: str, rev: str, label: str) -> bool: """True if this push introduced the build failure, False if it inherited it.""" - is_new, _ = _await_new_failure( - branch, rev, lambda push: _build_status(push, label), f"build {label}" - ) - return is_new - + return label in _await_new_failures(branch, rev, _build_status, [label], "build") -def _group_status(push: Push, group: str): - """'passed'/'failed'/_PENDING/None for a test group (manifest) on a push. - GroupSummary.status combines retriggers into PASS/FAIL/INTERMITTENT. - INTERMITTENT and a missing group are non-decisive: the tests.firefox.dev - flakiness rate judges intermittency instead. - """ - summary = push.group_summaries.get(group) - if summary is None: - return None - if push.is_group_running(summary): - return _PENDING - if summary.status == Status.PASS: - return "passed" - if summary.status == Status.FAIL: - return "failed" - return None - - -def is_new_test_failure(branch: str, rev: str, group: str) -> tuple[bool, str | None]: - """(is_new, last_green_rev) for a failing test group; shares the build walk.""" - return _await_new_failure( - branch, rev, lambda push: _group_status(push, group), f"group {group}" +def new_test_failures(branch: str, rev: str, label: str, groups: list[str]) -> set[str]: + """The failing groups this push introduced, for one task's label.""" + return _await_new_failures( + branch, + rev, + lambda push, group: _group_status(push, group, label), + groups, + "group", ) diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index 26404fd58f..abc3df38d6 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -1,10 +1,11 @@ import json from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock, patch +import pytest from app import consumer from app.failures import FailingGroup -from app.flakiness import Flakiness FIXTURES = Path(__file__).parent / "fixtures" @@ -247,39 +248,44 @@ def _test_msg( ) -def _test_repair_patches(is_new=(True, "greenrev"), rate=0.0): - return ( - patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), - patch.object(consumer.failures, "failing_groups", return_value=[_GROUP]), - patch.object( - consumer.flakiness, - "get_flakiness", - return_value=Flakiness( - total=10, passes=int(10 * (1 - rate)), fails=int(10 * rate) - ), +@pytest.fixture +def env(monkeypatch): + """The test-repair path's external seams, as named mocks. + + Defaults describe the happy path: one failing group, not intermittent, a new + regression, revision mappable, trigger succeeds. + """ + mocks = SimpleNamespace( + get_hg_revision=MagicMock(return_value="hgrev"), + failing_groups=MagicMock(return_value=[_GROUP]), + intermittent_tests=MagicMock(return_value=set()), + new_test_failures=MagicMock( + side_effect=lambda p, r, label, groups: set(groups) ), - patch.object(consumer.regression, "is_new_test_failure", return_value=is_new), - patch.object(consumer.lando, "hg_to_git", return_value="gitH"), + hg_to_git=MagicMock(return_value="gitH"), + trigger_run=MagicMock(return_value="tr-1"), + executor=MagicMock(), + ) + monkeypatch.setattr(consumer.taskcluster, "get_hg_revision", mocks.get_hg_revision) + monkeypatch.setattr(consumer.failures, "failing_groups", mocks.failing_groups) + monkeypatch.setattr( + consumer.flakiness, "intermittent_tests", mocks.intermittent_tests + ) + monkeypatch.setattr( + consumer.regression, "new_test_failures", mocks.new_test_failures ) + monkeypatch.setattr(consumer.lando, "hg_to_git", mocks.hg_to_git) + monkeypatch.setattr(consumer.client, "trigger_run", mocks.trigger_run) + return mocks -def test_test_failure_triggers_rca_run(): - executor = MagicMock() - p1, p2, p3, p4, p5 = _test_repair_patches() - with ( - p1, - p2, - p3, - p4, - p5, - patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, - ): - run_id = consumer.process(_test_msg(), executor) +def test_test_failure_triggers_rca_run(env): + run_id = consumer.process(_test_msg(), env.executor) assert run_id == "tr-1" - trigger.assert_called_once() - inputs = trigger.call_args.args[0] - assert trigger.call_args.kwargs["agent_name"] == "test-repair" + env.trigger_run.assert_called_once() + inputs = env.trigger_run.call_args.args[0] + assert env.trigger_run.call_args.kwargs["agent_name"] == "test-repair" # The agent resolves the test, commit range and clone depth itself; the # listener only hands it the failing task. assert inputs["failure_tasks"] == { @@ -287,167 +293,137 @@ def test_test_failure_triggers_rca_run(): } assert "test_id" not in inputs assert "candidate_commits" not in inputs - fn, ctx = executor.submit.call_args.args + fn, ctx = env.executor.submit.call_args.args assert fn is consumer.worker.poll_and_notify assert ctx.agent == "test-repair" - assert ctx.test_name == "dom/base/test/mochitest.ini" + assert ctx.test_groups == [_GROUP.group] -def test_intermittent_test_skipped_before_mozci(): - executor = MagicMock() - p1, p2, p3, p4, p5 = _test_repair_patches(rate=0.8) - with ( - p1, - p2, - p3, - p4 as is_new, - p5, - patch.object(consumer.client, "trigger_run") as trigger, - ): - assert consumer.process(_test_msg(), executor) is None - is_new.assert_not_called() - trigger.assert_not_called() +def test_intermittent_test_skipped_before_mozci(env): + env.intermittent_tests.return_value = {_GROUP.test} + assert consumer.process(_test_msg(), env.executor) is None + env.new_test_failures.assert_not_called() + env.trigger_run.assert_not_called() -def test_inherited_test_group_skipped(): - executor = MagicMock() - p1, p2, p3, p4, p5 = _test_repair_patches(is_new=(False, None)) - with ( - p1, - p2, - p3, - p4, - p5, - patch.object(consumer.client, "trigger_run") as trigger, - ): - assert consumer.process(_test_msg(), executor) is None - trigger.assert_not_called() +def test_inherited_test_group_skipped(env): + env.new_test_failures.side_effect = lambda *_: set() + assert consumer.process(_test_msg(), env.executor) is None + env.trigger_run.assert_not_called() -def test_same_group_same_push_triggers_once(): - executor = MagicMock() - p1, p2, p3, p4, p5 = _test_repair_patches() - with ( - p1, - p2, - p3, - p4, - p5, - patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, - ): - consumer.process(_test_msg(task_id="A"), executor) - consumer.process(_test_msg(task_id="B"), executor) - trigger.assert_called_once() +def test_same_group_same_push_triggers_once(env): + consumer.process(_test_msg(task_id="A"), env.executor) + consumer.process(_test_msg(task_id="B"), env.executor) + env.trigger_run.assert_called_once() -def test_no_failing_groups_skips(): - executor = MagicMock() - with ( - patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), - patch.object(consumer.failures, "failing_groups", return_value=[]), - patch.object(consumer.client, "trigger_run") as trigger, - ): - assert consumer.process(_test_msg(), executor) is None - trigger.assert_not_called() +def test_no_failing_groups_skips(env): + env.failing_groups.return_value = [] + assert consumer.process(_test_msg(), env.executor) is None + env.trigger_run.assert_not_called() -def test_unwatched_project_test_skipped(): - executor = MagicMock() - with ( - patch.object(consumer.taskcluster, "get_hg_revision") as get_rev, - patch.object(consumer.failures, "failing_groups") as fg, - ): - assert consumer.process(_test_msg(project="try"), executor) is None - get_rev.assert_not_called() - fg.assert_not_called() +def test_unreadable_errorsummary_still_triggers_run(env): + # Fail open like the build path: an errorsummary that cannot be read must not + # be mistaken for "nothing failed" and drop a possible regression. + env.failing_groups.side_effect = RuntimeError("no artifact") + assert consumer.process(_test_msg(task_id="ERR"), env.executor) == "tr-1" + env.trigger_run.assert_called_once() + # With no groups resolved there is nothing to filter or to name. + env.intermittent_tests.assert_not_called() + env.new_test_failures.assert_not_called() + _, ctx = env.executor.submit.call_args.args + assert ctx.test_groups == [] -def test_test_repair_trigger_failure_releases_group_for_retry(): - executor = MagicMock() - p1, p2, p3, p4, p5 = _test_repair_patches() - with ( - p1, - p2, - p3, - p4, - p5, - patch.object( - consumer.client, "trigger_run", side_effect=[RuntimeError("boom"), "tr-2"] - ) as trigger, - ): - assert consumer.process(_test_msg(task_id="A"), executor) is None - assert consumer.process(_test_msg(task_id="B"), executor) == "tr-2" - assert trigger.call_count == 2 +def test_unreadable_errorsummary_triggers_once_per_task(env): + env.failing_groups.side_effect = RuntimeError("no artifact") + consumer.process(_test_msg(task_id="A"), env.executor) + consumer.process(_test_msg(task_id="B"), env.executor) + env.trigger_run.assert_called_once() -def test_harness_detection(): - # xpcshell via the test-suite tag, and via the label when the suite is generic. - assert consumer._harness({"test-suite": "xpcshell"}, "irrelevant") == "xpcshell" - assert ( - consumer._harness({"test-suite": "test"}, "test-linux/opt-xpcshell-4") - == "xpcshell" - ) - assert ( - consumer._harness({"test-suite": "mochitest-browser-chrome"}, "l") - == "mochitest" - ) - # Recognized from the label too: the same suite arrives with kind "test" and - # sometimes no test-suite tag, and "test" is not a harness. - assert ( - consumer._harness( - {"kind": "test"}, "test-linux2404-64/opt-mochitest-browser-chrome-8" - ) - == "mochitest" - ) - # Harnesses that publish no timings dataset resolve to None (no lookup). - assert consumer._harness({"kind": "web-platform-tests"}, "l") is None - assert consumer._harness({"kind": "test"}, "test-linux/opt-reftest-1") is None +def test_rejected_group_is_not_re_evaluated_by_sibling_chunks(env): + # The dedupe claim is taken before the expensive checks, so a sibling chunk + # reporting the same group does not repeat the mozci walk to reach the same + # verdict. + env.new_test_failures.side_effect = lambda *_: set() + assert consumer.process(_test_msg(task_id="A"), env.executor) is None + assert consumer.process(_test_msg(task_id="B"), env.executor) is None + env.trigger_run.assert_not_called() + assert env.new_test_failures.call_count == 1 -def test_multiple_failing_groups_trigger_one_run_per_task(monkeypatch): - executor = MagicMock() +def test_missing_git_mapping_still_triggers_run(env): + # Unlike a build failure, the run is still useful (the agent works from the + # task id), so a revision Lando has not mirrored yet must not drop it. + env.hg_to_git.return_value = None + assert consumer.process(_test_msg(), env.executor) == "tr-1" + env.trigger_run.assert_called_once() + _, ctx = env.executor.submit.call_args.args + assert ctx.git_commit == "" + + +def test_unwatched_project_test_skipped(env): + assert consumer.process(_test_msg(project="try"), env.executor) is None + env.get_hg_revision.assert_not_called() + env.failing_groups.assert_not_called() + + +def test_test_repair_trigger_failure_releases_group_for_retry(env): + env.trigger_run.side_effect = [RuntimeError("boom"), "tr-2"] + assert consumer.process(_test_msg(task_id="A"), env.executor) is None + assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-2" + assert env.trigger_run.call_count == 2 + + +def test_multiple_failing_groups_trigger_one_run_per_task(env): groups = [ FailingGroup("dom/base/test/mochitest.ini", "dom/base/test/a.js", "GENERIC"), FailingGroup("layout/test/mochitest.ini", "layout/test/b.js", "TIMEOUT"), ] - with ( - patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), - patch.object(consumer.failures, "failing_groups", return_value=groups), - patch.object( - consumer.flakiness, - "get_flakiness", - return_value=Flakiness(total=10, passes=10), - ), - patch.object( - consumer.regression, - "is_new_test_failure", - return_value=(True, "greenrev"), - ), - patch.object(consumer.lando, "hg_to_git", return_value="gitH"), - patch.object(consumer.client, "trigger_run", return_value="tr-1") as trigger, - ): - run_id = consumer.process(_test_msg(), executor) + env.failing_groups.return_value = groups + assert consumer.process(_test_msg(), env.executor) == "tr-1" # The whole task gets a single run; the agent investigates every failing group. - assert run_id == "tr-1" - trigger.assert_called_once() - assert trigger.call_args.args[0]["failure_tasks"] == { + env.trigger_run.assert_called_once() + assert env.trigger_run.call_args.args[0]["failure_tasks"] == { "test-linux1804-64/opt-mochitest-browser-chrome-1": "TT" } - assert executor.submit.call_count == 1 + assert env.executor.submit.call_count == 1 + # Every failing group is named, not an arbitrary one of them. + _, ctx = env.executor.submit.call_args.args + assert ctx.test_groups == [g.group for g in groups] -def test_missing_hg_revision_skips_test_task(monkeypatch): - executor = MagicMock() - with ( - patch.object(consumer.taskcluster, "get_hg_revision", return_value=None), - patch.object(consumer.failures, "failing_groups") as fg, - patch.object(consumer.client, "trigger_run") as trigger, - ): - assert consumer.process(_test_msg(), executor) is None +def test_missing_hg_revision_skips_test_task(env): + env.get_hg_revision.return_value = None + assert consumer.process(_test_msg(), env.executor) is None # Bail before doing the (network-heavy) group resolution. - fg.assert_not_called() - trigger.assert_not_called() + env.failing_groups.assert_not_called() + env.trigger_run.assert_not_called() + + +def test_intermittent_gate_runs_before_the_mozci_walk(env): + # Both gates are batch calls over the task's groups; the cheap one goes first + # and the expensive one only sees what survived. + groups = [ + FailingGroup("a/mochitest.ini", "a/test_a.js", "GENERIC"), + FailingGroup("b/mochitest.ini", "b/test_b.js", "GENERIC"), + ] + env.failing_groups.return_value = groups + env.intermittent_tests.return_value = {"a/test_a.js"} + + assert consumer.process(_test_msg(), env.executor) == "tr-1" + env.intermittent_tests.assert_called_once() + tests, suite, label = env.intermittent_tests.call_args.args + assert list(tests) == ["a/test_a.js", "b/test_b.js"] + assert suite == "mochitest-browser-chrome" + # Only the surviving group reaches the walk. + assert env.new_test_failures.call_args.args[3] == ["b/mochitest.ini"] + _, ctx = env.executor.submit.call_args.args + assert ctx.test_groups == ["b/mochitest.ini"] def test_queue_name_includes_non_production_environment(): @@ -460,18 +436,3 @@ def test_queue_name_omits_production_environment(): with patch.object(consumer.settings, "environment", "production"): (queue,) = consumer._build_queues("guest") assert queue.name == "queue/guest/build-repair-task-failed" - - -def test_no_dataset_harness_skips_flakiness_lookup(): - # reftest/wpt publish no timings dataset, so the lookup would be a guaranteed - # 404; the group is still evaluated for being a regression. - with ( - patch.object(consumer.flakiness, "get_flakiness") as get_flakiness, - patch.object( - consumer.regression, "is_new_test_failure", return_value=(True, "green") - ), - ): - fresh = consumer._fresh_groups([_GROUP], "autoland", "hgrev", None) - - get_flakiness.assert_not_called() - assert fresh == [_GROUP] diff --git a/services/hackbot-pulse-listener/tests/test_failures.py b/services/hackbot-pulse-listener/tests/test_failures.py index 35afdea970..ca544f1a58 100644 --- a/services/hackbot-pulse-listener/tests/test_failures.py +++ b/services/hackbot-pulse-listener/tests/test_failures.py @@ -2,8 +2,12 @@ from types import SimpleNamespace +import pytest from app import failures +LABEL = "test-linux1804-64/opt-mochitest-browser-chrome-1" +WPT_LABEL = "test-linux1804-64/opt-web-platform-tests-2" + def _ftype(name): return SimpleNamespace(name=name) @@ -19,7 +23,7 @@ def test_failing_groups_maps_mozci_output(monkeypatch): ], } monkeypatch.setattr(failures, "_failure_types", lambda t: fake) - groups = failures.failing_groups("TASK") + groups = failures.failing_groups("TASK", LABEL) assert {g.group for g in groups} == set(fake) bf = next(g for g in groups if g.group.endswith("browser.toml")) assert bf.test == "browser/base/content/test/bf/browser_a.js" @@ -31,12 +35,52 @@ def test_failing_groups_maps_mozci_output(monkeypatch): def test_failing_groups_skips_empty_group(monkeypatch): monkeypatch.setattr(failures, "_failure_types", lambda t: {"grp": []}) - assert failures.failing_groups("TASK") == [] + assert failures.failing_groups("TASK", LABEL) == [] -def test_failing_groups_empty_on_mozci_error(monkeypatch): +def test_failing_groups_raises_on_mozci_error(monkeypatch): + # Must not be reported as "nothing failed": the caller has to tell an + # unreadable errorsummary apart from a task with no failing groups. def boom(task_id): raise RuntimeError("mozci could not read the task") monkeypatch.setattr(failures, "_failure_types", boom) - assert failures.failing_groups("TASK") == [] + with pytest.raises(RuntimeError): + failures.failing_groups("TASK", LABEL) + + +def test_wpt_group_names_are_normalized_to_source_paths(monkeypatch): + # The errorsummary names wpt groups by URL path; mozci keys its results by + # source path, and the regression check compares the two. + fake = { + "/html/browsers/the-window-object/foo.html": [("foo.html", _ftype("GENERIC"))], + "/_mozilla/dom/bar.html": [("bar.html", _ftype("GENERIC"))], + } + monkeypatch.setattr(failures, "_failure_types", lambda t: fake) + groups = {g.group for g in failures.failing_groups("TASK", WPT_LABEL)} + assert groups == { + "testing/web-platform/tests/html/browsers/the-window-object/foo.html", + "testing/web-platform/mozilla/tests/dom/bar.html", + } + + +def test_wpt_root_group_is_dropped(monkeypatch): + monkeypatch.setattr( + failures, "_failure_types", lambda t: {"/": [("x", _ftype("GENERIC"))]} + ) + assert failures.failing_groups("TASK", WPT_LABEL) == [] + + +def test_non_wpt_group_names_are_untouched(monkeypatch): + fake = {"dom/base/test/mochitest.ini": [("dom/base/test/a.js", _ftype("GENERIC"))]} + monkeypatch.setattr(failures, "_failure_types", lambda t: fake) + (group,) = failures.failing_groups("TASK", LABEL) + assert group.group == "dom/base/test/mochitest.ini" + + +def test_unusable_group_names_are_dropped(monkeypatch): + # mozci drops absolute and Windows-style names from its own results, so a + # group named that way could never match anything. + fake = {"Z:\\build\\tests\\mochitest.ini": [("a.js", _ftype("GENERIC"))]} + monkeypatch.setattr(failures, "_failure_types", lambda t: fake) + assert failures.failing_groups("TASK", LABEL) == [] diff --git a/services/hackbot-pulse-listener/tests/test_flakiness.py b/services/hackbot-pulse-listener/tests/test_flakiness.py index 573393cf2c..a77577a2d6 100644 --- a/services/hackbot-pulse-listener/tests/test_flakiness.py +++ b/services/hackbot-pulse-listener/tests/test_flakiness.py @@ -12,6 +12,17 @@ def bucket() -> dict: return json.loads(FIXTURE.read_text()) +@pytest.fixture(autouse=True) +def _clear_bucket_cache(): + flakiness._buckets.clear() + yield + flakiness._buckets.clear() + + +def _stats(bucket: dict, test_path: str) -> flakiness.Flakiness: + return flakiness._decode_bucket(bucket).get(test_path, flakiness.Flakiness()) + + def test_chunk_index_matches_reference_hash(): # Port of the dashboard's getChunkIndex (32-bit signed string hash mod 64). assert flakiness._chunk_index("") == 0 @@ -23,8 +34,8 @@ def test_chunk_index_matches_reference_hash(): assert idx == flakiness._chunk_index("dom/base/test/test_foo.js") -def test_compute_stats_counts_and_rate(bucket): - stats = flakiness._compute_stats(bucket, "dom/base/test/test_foo.js") +def test_decode_bucket_counts_and_rate(bucket): + stats = _stats(bucket, "dom/base/test/test_foo.js") # foo: 3 passes (durations 2+1), 1 fail, 1 timeout, no skip/crash. assert stats.passes == 3 assert stats.fails == 1 @@ -36,15 +47,16 @@ def test_compute_stats_counts_and_rate(bucket): assert stats.failure_rate == pytest.approx(0.4) -def test_compute_stats_last_green_day(bucket): - # Passes on days 0 and 2; a fail on day 2 and a timeout on day 1, so only day 0 - # is green. Days are differentially compressed and decoded by running sum. - stats = flakiness._compute_stats(bucket, "dom/base/test/test_foo.js") - assert stats.last_green_day == 0 +def test_decode_bucket_covers_every_test_in_one_pass(bucket): + # Decoded once per bucket and cached, so every test in it must be present. + assert set(flakiness._decode_bucket(bucket)) == { + "dom/base/test/test_foo.js", + "dom/base/test/test_bar.js", + } -def test_compute_stats_skip_only_test(bucket): - stats = flakiness._compute_stats(bucket, "dom/base/test/test_bar.js") +def test_decode_bucket_skip_only_test(bucket): + stats = _stats(bucket, "dom/base/test/test_bar.js") assert stats.skips == 4 assert stats.passes == 0 assert stats.fails == 0 @@ -52,8 +64,8 @@ def test_compute_stats_skip_only_test(bucket): assert stats.failure_rate == 0.0 -def test_compute_stats_unknown_test_returns_empty(bucket): - stats = flakiness._compute_stats(bucket, "does/not/exist.js") +def test_decode_bucket_unknown_test_returns_empty(bucket): + stats = _stats(bucket, "does/not/exist.js") assert stats.total == 0 assert stats.failure_rate == 0.0 @@ -85,6 +97,16 @@ def boom(*a, **k): assert stats.failure_rate == 0.0 +def test_get_flakiness_fails_soft_on_unexpected_bucket_shape(monkeypatch): + # The bucket layout is an undocumented internal format of the dashboard, so a + # shape change must not escape and abort the whole failing task. + monkeypatch.setattr( + flakiness, "_fetch_bucket", lambda *a, **k: ["not", "a", "dict"] + ) + stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") + assert stats.total == 0 + + def _http_error(status_code): request = flakiness.httpx.Request("GET", "https://example/timings.json") response = flakiness.httpx.Response(status_code, request=request) @@ -103,7 +125,7 @@ def not_found(*a, **k): assert stats.total == 0 assert not [r for r in caplog.records if r.levelname in ("ERROR", "WARNING")] - assert any(r.exc_info for r in caplog.records) is False + assert not any(r.exc_info for r in caplog.records) def test_unexpected_http_status_warns_without_traceback(monkeypatch, caplog): @@ -116,3 +138,81 @@ def server_error(*a, **k): assert stats.total == 0 assert [r for r in caplog.records if r.levelname == "WARNING"] + + +def test_harness_detection(): + # xpcshell via the test-suite tag, and via the label when the suite is generic. + assert flakiness._harness("xpcshell", "irrelevant") == "xpcshell" + assert flakiness._harness("test", "test-linux/opt-xpcshell-4") == "xpcshell" + assert flakiness._harness("mochitest-browser-chrome", "l") == "mochitest" + # Recognized from the label too: the same suite arrives with no test-suite tag, + # and the Taskcluster kind ("test") is not a harness. + assert ( + flakiness._harness("", "test-linux2404-64/opt-mochitest-browser-chrome-8") + == "mochitest" + ) + # Harnesses that publish no timings dataset resolve to None (no lookup). + assert flakiness._harness("web-platform-tests", "l") is None + assert flakiness._harness("", "test-linux/opt-reftest-1") is None + + +def test_bucket_is_fetched_once_for_every_test_in_it(monkeypatch, bucket): + # The dataset changes a few times a day and a real bucket is tens of MB, so a + # second test in the same bucket must not refetch it. + calls = [] + + def fake_fetch(harness, chunk, repo): + calls.append(chunk) + return bucket + + monkeypatch.setattr(flakiness, "_fetch_bucket", fake_fetch) + foo = "dom/base/test/test_foo.js" + assert flakiness._chunk_index(foo) == flakiness._chunk_index(foo) + flakiness.get_flakiness(foo, "xpcshell") + flakiness.get_flakiness(foo, "xpcshell") + assert calls == [flakiness._chunk_index(foo)] + + +def test_intermittent_tests_skips_lookup_without_a_dataset(monkeypatch): + # reftest/wpt publish nothing, so the lookup would be a guaranteed 404. + called = [] + monkeypatch.setattr( + flakiness, "_fetch_bucket", lambda *a, **k: called.append(a) or {} + ) + assert ( + flakiness.intermittent_tests(["a.html"], "reftest", "test-linux/reftest-1") + == set() + ) + assert called == [] + + +def test_intermittent_tests_flags_only_tests_over_the_threshold(monkeypatch): + rates = {"flaky.js": 0.30, "solid.js": 0.001} + monkeypatch.setattr( + flakiness, + "get_flakiness", + lambda test, harness, repo="mozilla-central": flakiness.Flakiness( + total=1000, + passes=int(1000 * (1 - rates[test])), + fails=int(1000 * rates[test]), + ), + ) + flaky = flakiness.intermittent_tests( + ["flaky.js", "solid.js"], "xpcshell", "test-linux/opt-xpcshell-1" + ) + assert flaky == {"flaky.js"} + + +def test_few_percent_failure_rate_counts_as_intermittent(monkeypatch): + # The timings dataset records one entry per task run per day, so even the + # flakiest tests sit at a few percent. A threshold tuned for a per-push rate + # would sit above every real intermittent and never fire. + monkeypatch.setattr( + flakiness, + "get_flakiness", + lambda *a, **k: flakiness.Flakiness(total=2000, passes=1880, fails=120), + ) + flaky = flakiness.intermittent_tests( + ["a.js"], "xpcshell", "test-linux/opt-xpcshell-1" + ) + assert flaky == {"a.js"} diff --git a/services/hackbot-pulse-listener/tests/test_notify.py b/services/hackbot-pulse-listener/tests/test_notify.py index 86f3ab25d5..d3e78ec932 100644 --- a/services/hackbot-pulse-listener/tests/test_notify.py +++ b/services/hackbot-pulse-listener/tests/test_notify.py @@ -18,12 +18,13 @@ def _ctx(**over): return RunContext(**base) +def settings_test_repair_address() -> str | None: + return notify.settings.test_repair_notification_email + + def _test_repair_ctx(**over): - return _ctx( - agent="test-repair", - test_name="dom/base/test/mochitest.ini", - **over, - ) + over.setdefault("test_groups", ["dom/base/test/mochitest.ini"]) + return _ctx(agent="test-repair", **over) def test_skips_without_recipient(): @@ -317,6 +318,34 @@ def test_test_repair_body_leads_with_recommendation(): assert "## Analysis" in body +def test_test_repair_body_names_every_failing_group(): + ctx = _test_repair_ctx( + test_groups=["dom/base/test/mochitest.ini", "layout/test/mochitest.ini"] + ) + body = notify._build_test_repair_body(ctx, _test_repair_findings(), None, None) + assert "dom/base/test/mochitest.ini" in body + assert "layout/test/mochitest.ini" in body + + +def test_test_repair_subject_summarizes_multiple_groups(): + ctx = _test_repair_ctx(test_groups=["a/mochitest.ini", "b/mochitest.ini"]) + assert notify._test_groups_label(ctx) == "a/mochitest.ini (+1 more)" + assert ( + notify._test_groups_label(_test_repair_ctx()) == "dom/base/test/mochitest.ini" + ) + assert notify._test_groups_label(_test_repair_ctx(test_groups=[])) == "task TASK123" + + +def test_test_repair_body_omits_unmapped_git_revision(): + # An unmapped revision must not render an empty commit link. + body = notify._build_test_repair_body( + _test_repair_ctx(git_commit=""), _test_repair_findings(), None, None + ) + assert "Revision (git)" not in body + assert "firefox/commit/)" not in body + assert "Revision (hg)" in body + + def test_test_repair_intermittent_body_says_do_not_backout(): findings = _test_repair_findings( classification="intermittent", @@ -334,7 +363,9 @@ def test_test_repair_recipients_address_then_culprit(monkeypatch): notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" ) monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._test_repair_recipients("culprit@mozilla.com") == [ + assert notify._recipients( + settings_test_repair_address(), "culprit@mozilla.com" + ) == [ "test-repair@mozilla.com", "culprit@mozilla.com", "team@mozilla.com", @@ -348,7 +379,9 @@ def test_test_repair_recipients_override_wins(monkeypatch): monkeypatch.setattr( notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" ) - assert notify._test_repair_recipients("culprit@mozilla.com") == ["me@mozilla.com"] + assert notify._recipients( + settings_test_repair_address(), "culprit@mozilla.com" + ) == ["me@mozilla.com"] def test_test_repair_intermittent_sends_without_patch(monkeypatch): diff --git a/services/hackbot-pulse-listener/tests/test_regression_test_failure.py b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py index d9e0c56dd8..28f145d88b 100644 --- a/services/hackbot-pulse-listener/tests/test_regression_test_failure.py +++ b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py @@ -1,102 +1,165 @@ -"""Tests for the test-failure regression gate (is_new_test_failure).""" +"""Tests for the test-failure regression gate (new_test_failures).""" +from types import SimpleNamespace + +import pytest from app import regression from mozci.errors import ParentPushNotFound -from mozci.task import Status GROUP = "dom/base/test/mochitest.ini" - - -class FakeSummary: - def __init__(self, status, running=False): - self.status = status - self.running = running +OTHER_GROUP = "layout/test/mochitest.ini" +LABEL = "test-linux1804-64/opt-mochitest-browser-chrome-1" +OTHER_LABEL = "test-windows11-64/debug-mochitest-browser-chrome-1" + + +def _task(label=LABEL, groups=(), state="completed", result="failed"): + """A test task reporting (group, ok) pairs.""" + return SimpleNamespace( + label=label, + state=state, + result=result, + failed=result == "failed", + results=[SimpleNamespace(group=g, ok=ok, duration=1) for g, ok in groups], + ) class FakePush: - def __init__(self, rev, summaries=None, parent=None): + def __init__(self, rev, tasks=(), parent=None, scheduled=()): self.rev = rev - self._summaries = summaries or {} + self.tasks = list(tasks) + self.scheduled_task_labels = set(scheduled) self._parent = parent - @property - def group_summaries(self): - return self._summaries - @property def parent(self): if self._parent is None: raise ParentPushNotFound(f"no parent for {self.rev}") return self._parent - def is_group_running(self, summary): - return getattr(summary, "running", False) - def _install_head(monkeypatch, head): monkeypatch.setattr(regression, "Push", lambda rev, branch=None: head) -def test_new_failure_when_ancestor_passed(monkeypatch): - parent = FakePush("parentrev", {GROUP: FakeSummary(Status.PASS)}) - head = FakePush("headrev", {}, parent=parent) +def _check(groups=(GROUP,)): + return regression.new_test_failures("autoland", "headrev", LABEL, list(groups)) + + +def _group_state(monkeypatch, head, group=GROUP): _install_head(monkeypatch, head) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green == "parentrev" + return regression._classify( + head, + lambda push: regression._group_status(push, group, LABEL), + f"group {group}", + ) + + +def test_new_failure_when_ancestor_passed(monkeypatch): + parent = FakePush("parentrev", [_task(groups=[(GROUP, True)], result="passed")]) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + assert _check() == {GROUP} def test_inherited_when_ancestor_failed(monkeypatch): - parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL)}) - head = FakePush("headrev", {}, parent=parent) - _install_head(monkeypatch, head) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is False - assert last_green is None + parent = FakePush("parentrev", [_task(groups=[(GROUP, False)])]) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + assert _check() == set() -def test_intermittent_ancestor_is_skipped_until_decisive(monkeypatch): - green = FakePush("greenrev", {GROUP: FakeSummary(Status.PASS)}) - flaky = FakePush( - "flakyrev", {GROUP: FakeSummary(Status.INTERMITTENT)}, parent=green +def test_retriggered_green_ancestor_counts_as_passed(monkeypatch): + # Any green run wins, so the failure here is treated as new (errs toward + # running the agent rather than dropping a regression). + parent = FakePush( + "parentrev", [_task(groups=[(GROUP, False)]), _task(groups=[(GROUP, True)])] ) - head = FakePush("headrev", {}, parent=flaky) - _install_head(monkeypatch, head) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green == "greenrev" + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + assert _check() == {GROUP} -def test_coalesced_ancestor_without_group_is_skipped(monkeypatch): - green = FakePush("greenrev", {GROUP: FakeSummary(Status.PASS)}) - coalesced = FakePush("coalrev", {}, parent=green) # group never ran here - head = FakePush("headrev", {}, parent=coalesced) - _install_head(monkeypatch, head) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green == "greenrev" +def test_other_configuration_failure_does_not_mask_new_failure(monkeypatch): + # The manifest is already broken on another platform at the parent push. The + # all-configuration group summary would call that an inherited failure; only + # this task's own label may decide. + green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) + parent = FakePush( + "parentrev", [_task(label=OTHER_LABEL, groups=[(GROUP, False)])], parent=green + ) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + assert _check() == {GROUP} -def test_running_ancestor_is_pending(monkeypatch): - parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL, running=True)}) - head = FakePush("headrev", {}, parent=parent) - _install_head(monkeypatch, head) - state, _ = regression._classify( - "autoland", - "headrev", - lambda push: regression._group_status(push, GROUP), - f"group {GROUP}", +def test_unfinished_ancestor_task_is_pending(monkeypatch): + # The parent's equivalent task is still running, so it has published no + # results yet. It must be waited for, not skipped as non-decisive. + parent = FakePush("parentrev", [_task(state="running", result=None)]) + head = FakePush("headrev", parent=parent) + assert _group_state(monkeypatch, head) is regression._PENDING + + +def test_unsettled_sibling_defers_a_failed_group(monkeypatch): + # Same precedence as the build path: a retrigger that may still turn green + # outranks an existing failure, so wait instead of calling it inherited. + parent = FakePush( + "parentrev", + [_task(groups=[(GROUP, False)]), _task(state="running", result=None)], ) - assert state is regression._PENDING + head = FakePush("headrev", parent=parent) + assert _group_state(monkeypatch, head) is regression._PENDING + + +def test_scheduled_but_unreported_ancestor_task_is_pending(monkeypatch): + # The label was scheduled on the parent but no task is visible yet. + parent = FakePush("parentrev", [], scheduled=[LABEL]) + head = FakePush("headrev", parent=parent) + assert _group_state(monkeypatch, head) is regression._PENDING + + +def test_coalesced_ancestor_is_skipped(monkeypatch): + # Nothing ran and nothing was scheduled: non-decisive, keep walking. + green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) + coalesced = FakePush("coalrev", [], parent=green) + _install_head(monkeypatch, FakePush("headrev", parent=coalesced)) + assert _check() == {GROUP} + + +def test_ancestor_ran_label_without_the_group_is_skipped(monkeypatch): + # The manifest was chunked into a different task here: non-decisive. + green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) + other = FakePush("otherrev", [_task(groups=[(OTHER_GROUP, False)])], parent=green) + _install_head(monkeypatch, FakePush("headrev", parent=other)) + assert _check() == {GROUP} + + +def test_groups_are_judged_independently(monkeypatch): + parent = FakePush( + "parentrev", + [_task(groups=[(GROUP, True), (OTHER_GROUP, False)], result="passed")], + ) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + assert _check([GROUP, OTHER_GROUP]) == {GROUP} + + +def test_one_ancestor_walk_serves_every_group(monkeypatch): + # mozci memoizes a push's task list per instance, so all groups of a task must + # share one head push rather than refetching the ancestors for each. + parent = FakePush("parentrev", [_task(groups=[(GROUP, True)], result="passed")]) + head = FakePush("headrev", parent=parent) + builds = [] + + def build(rev, branch=None): + builds.append(rev) + return head + + monkeypatch.setattr(regression, "Push", build) + regression.new_test_failures( + "autoland", "headrev", LABEL, [GROUP, OTHER_GROUP, "c"] + ) + assert builds == ["headrev"] def test_no_ancestor_ran_group_fails_open(monkeypatch): - parent = FakePush("parentrev", {}) # group not present, no further parent - head = FakePush("headrev", {}, parent=parent) - _install_head(monkeypatch, head) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green is None + _install_head(monkeypatch, FakePush("headrev", parent=FakePush("parentrev"))) + assert _check() == {GROUP} def test_mozci_error_fails_open(monkeypatch): @@ -104,19 +167,28 @@ def boom(rev, branch=None): raise RuntimeError("mozci exploded") monkeypatch.setattr(regression, "Push", boom) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green is None + assert _check([GROUP, OTHER_GROUP]) == {GROUP, OTHER_GROUP} def test_pending_past_deadline_fails_open(monkeypatch): - parent = FakePush("parentrev", {GROUP: FakeSummary(Status.FAIL, running=True)}) - head = FakePush("headrev", {}, parent=parent) - _install_head(monkeypatch, head) - # Force the poll loop to time out immediately without real sleeping. + parent = FakePush("parentrev", [_task(state="running", result=None)]) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) monkeypatch.setattr(regression.time, "sleep", lambda s: None) ticks = iter([0.0, regression.MAX_WAIT_SECONDS + 1]) monkeypatch.setattr(regression.time, "monotonic", lambda: next(ticks)) - is_new, last_green = regression.is_new_test_failure("autoland", "headrev", GROUP) - assert is_new is True - assert last_green is None + assert _check() == {GROUP} + + +def test_groups_of_one_task_share_one_wait_budget(monkeypatch): + # The budget is per call, not per group: two pending groups must not each get + # their own MAX_WAIT_SECONDS. + parent = FakePush("parentrev", [_task(state="running", result=None)]) + _install_head(monkeypatch, FakePush("headrev", parent=parent)) + + def no_sleep(_seconds): + pytest.fail("waited past a spent deadline") + + monkeypatch.setattr(regression.time, "sleep", no_sleep) + ticks = iter([0.0, regression.MAX_WAIT_SECONDS + 1]) + monkeypatch.setattr(regression.time, "monotonic", lambda: next(ticks)) + assert _check([GROUP, OTHER_GROUP]) == {GROUP, OTHER_GROUP} From 201c5708195a4df0d51b8244fab818fdb4127e93 Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Tue, 28 Jul 2026 16:18:00 -0700 Subject: [PATCH 6/7] Move to treeherder --- services/hackbot-pulse-listener/README.md | 6 +- services/hackbot-pulse-listener/app/config.py | 14 +- .../hackbot-pulse-listener/app/consumer.py | 127 ++++--- .../hackbot-pulse-listener/app/failures.py | 73 ---- .../hackbot-pulse-listener/app/flakiness.py | 241 ------------ .../hackbot-pulse-listener/app/regression.py | 191 +++++----- .../hackbot-pulse-listener/app/treeherder.py | 284 +++++++++++++++ .../tests/fixtures/xpcshell-bucket.json | 29 -- .../tests/test_consumer.py | 142 ++++++-- .../tests/test_failures.py | 86 ----- .../tests/test_flakiness.py | 218 ----------- .../tests/test_regression.py | 214 ++++++----- .../tests/test_regression_test_failure.py | 322 ++++++++-------- .../tests/test_treeherder.py | 344 ++++++++++++++++++ 14 files changed, 1202 insertions(+), 1089 deletions(-) delete mode 100644 services/hackbot-pulse-listener/app/failures.py delete mode 100644 services/hackbot-pulse-listener/app/flakiness.py create mode 100644 services/hackbot-pulse-listener/app/treeherder.py delete mode 100644 services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json delete mode 100644 services/hackbot-pulse-listener/tests/test_failures.py delete mode 100644 services/hackbot-pulse-listener/tests/test_flakiness.py create mode 100644 services/hackbot-pulse-listener/tests/test_treeherder.py diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index 27702501cc..16a415d35d 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -21,8 +21,10 @@ Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-r 4. **Judge** whether the failure is worth a run. Both paths fail open — a mozci or Taskcluster error runs the agent rather than dropping a possible regression. - _Build:_ keep only failures this push introduced (not inherited from an ancestor). - - _Test:_ keep only groups that are new for this task's own configuration and not - intermittent (tests.firefox.dev flakiness below `FLAKINESS_THRESHOLD`). One run per + - _Test:_ first drop whatever Treeherder has already judged not to be a new + regression (intermittent, infra, expected-fail, fixed-by-commit); Treeherder + ingests a minute or so behind us, so the gate waits for the job to appear. Then + keep only groups that are new for this task's own configuration. One run per task, carrying only the task id. 5. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until terminal, then email a hackbot UI link, the analysis summary, a Treeherder link, and the diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index ea14e178bd..178dcf75d6 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -27,12 +27,14 @@ class Settings(BaseSettings): run_try_push: bool = False model: str | None = None max_turns: int | None = None - # Skip a failing test whose historical failure rate is at or above this - # (clearly intermittent) before spending a test-repair run. The rate is per - # *run*, over every platform in the timings window (three weeks, thousands of - # runs per test), where even the flakiest tests sit near a few percent -- so - # this is an order of magnitude lower than a per-push failure rate would be. - flakiness_threshold: float = 0.05 + # Treeherder classifies a failing job shortly after we see the failure, so the + # gate waits for the job to be ingested before reading that verdict. + treeherder_ingest_poll_seconds: int = 30 + treeherder_ingest_max_wait_seconds: int = 240 + # How long to wait for a verdict on a failure whose manifests are unknown, where + # no ancestor comparison is possible and most failures turn out to be classified + # intermittent or expected-fail shortly after we see them. + treeherder_classification_wait_seconds: int = 300 # Dedupe (in-memory, by hg revision) dedupe_ttl_seconds: int = 6 * 60 * 60 diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index ba97e8673e..b3fbaecb4d 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -6,15 +6,7 @@ from kombu import Connection, Exchange, Queue from kombu.mixins import ConsumerMixin -from app import ( - client, - failures, - flakiness, - lando, - regression, - taskcluster, - worker, -) +from app import client, lando, regression, taskcluster, treeherder, worker from app.config import settings from app.models import RunContext @@ -38,8 +30,11 @@ ) _seen_lock = threading.Lock() -# Independent dedupe for test-repair runs, keyed by (taskGroupId, test group): one push -# emits many failing test tasks, and we want one run per failing group. +# Independent dedupe for test-repair runs, keyed by (hg revision, test group): one +# push emits many failing test tasks, and we want one run per failing group. Keyed on +# the revision rather than the Taskcluster task group because backfills and retriggers +# are dispatched by action tasks, which start task groups of their own -- the same +# push would otherwise be investigated once per task group. _seen_tests: TTLCache = TTLCache( maxsize=settings.dedupe_max_size, ttl=settings.dedupe_ttl_seconds ) @@ -155,14 +150,13 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: """Test-failure path: filter, then trigger the test-repair agent for the task. One push emits many failing test tasks; each task may fail several groups. We - resolve the failing groups (via mozci) and keep the ones that are genuine, - non-intermittent regressions, then trigger a single run for the task (the - agent resolves the commit range itself from the task id). Dedupe is per - (push, group) so a manifest failing across chunks is investigated once. + resolve the failing groups and keep the ones that are genuine, non-intermittent + regressions, then trigger a single run for the task (the agent resolves the + commit range itself from the task id). Dedupe is per (push, group) so a manifest + failing across chunks is investigated once. """ status = body.get("status") or {} task_id = status.get("taskId") - task_group_id = status.get("taskGroupId") project = tags.get("project") label = tags.get("label") or task_id developer_email = tags.get("createdForUser") @@ -172,36 +166,38 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: logger.warning("No GECKO_HEAD_REV for test task %s; skipping", task_id) return None + # Cheapest gate first: one small request, and it rules out intermittents and + # infra failures for every harness before any ancestor walking. The same record + # carries the configuration the regression check compares against. + job = treeherder.job_for_task(project, task_id) + reason = treeherder.skip_reason(job) + if reason: + logger.info("Treeherder classified task %s as %s; skipping", task_id, reason) + return None + config = ((job or {}).get("platform"), (job or {}).get("platform_option")) + + whole_task = (project, hg_revision, task_id, label, developer_email) try: - groups = failures.failing_groups(task_id, label) + groups = treeherder.failing_groups(project, hg_revision, task_id) + except treeherder.GroupResultsUnavailable as exc: + # Routine: a task-level failure (crash, timeout, harness error) produces no + # per-manifest results, and a log may still be being parsed. Either way we + # cannot say what failed, so fail open rather than drop it. + logger.info("%s; running the agent on the whole task", exc) + return _trigger_whole_task(*whole_task, executor) except Exception: - # Fail open, as the build path does. An errorsummary that cannot be read - # (Taskcluster error, or the artifact not published yet when the failure - # message arrives) must not be mistaken for "nothing failed": investigate - # the whole task, keyed on its label since no group name is known. logger.exception( - "Could not resolve the failing groups of task %s; " + "Could not read the failing groups of task %s; " "running the agent on the whole task", task_id, ) - if not _claim_groups([(task_group_id, label)]): - return None - return _trigger_test_repair( - [], - [(task_group_id, label)], - project, - hg_revision, - task_id, - label, - developer_email, - executor, - ) + return _trigger_whole_task(*whole_task, executor) if not groups: logger.info("Task %s reported no failing test groups; skipping", task_id) return None - claimed = _claim_groups([(task_group_id, fg.group) for fg in groups]) + claimed = _claim_groups([(hg_revision, group) for group in groups]) if not claimed: logger.info( "Every failing group of task %s is already handled; skipping", task_id @@ -209,25 +205,66 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: return None fresh = _fresh_groups( - [fg for fg in groups if (task_group_id, fg.group) in claimed], + [group for group in groups if (hg_revision, group) in claimed], project, hg_revision, - label, - tags.get("test-suite") or "", + config, ) if not fresh: logger.info("No new, non-intermittent groups for task %s; skipping", task_id) return None + # Ask again before spending a run: the check above takes minutes, which is about + # how long Treeherder takes to classify an intermittent, so a verdict that was + # not available at the start of it often is by now. + reason = treeherder.recheck_skip_reason(project, task_id) + if reason: + logger.info( + "Treeherder classified task %s as %s while it was being checked; skipping", + task_id, + reason, + ) + return None + return _trigger_test_repair( fresh, claimed, project, hg_revision, task_id, label, developer_email, executor ) +def _trigger_whole_task( + project: str, + hg_revision: str, + task_id: str, + label: str, + developer_email: str | None, + executor: Executor, +) -> str | None: + """Investigate a task whose failing manifests could not be determined. + + There is no manifest to compare against an ancestor here, so nothing else delays + the decision. In practice these are mostly intermittents and expected failures + that Treeherder classifies a few minutes later, so wait for a verdict before + spending a run. + """ + claimed = _claim_groups([(hg_revision, label)]) + if not claimed: + logger.info("Task %s is already handled; skipping", task_id) + return None + + reason = treeherder.await_skip_reason(project, task_id) + if reason: + logger.info("Treeherder classified task %s as %s; skipping", task_id, reason) + return None + + return _trigger_test_repair( + [], claimed, project, hg_revision, task_id, label, developer_email, executor + ) + + def _claim_groups(keys: list[tuple[str, str]]) -> set[tuple[str, str]]: """Claim the keys not yet handed off, returning the ones this call claimed. - Claimed *before* the flakiness and mozci checks, not after: every failing chunk + Claimed *before* the regression check, not after: every failing chunk of a push resolves the same groups, so without an up-front claim they all run the expensive walk concurrently and then throw the answer away. A key stays claimed even when the filter then rejects the group, so the sibling tasks do @@ -241,16 +278,10 @@ def _claim_groups(keys: list[tuple[str, str]]) -> set[tuple[str, str]]: def _fresh_groups( - groups, project: str, hg_revision: str, label: str, suite: str + candidates: list[str], project: str, hg_revision: str, config: tuple[str, str] ) -> list[str]: - """Names of the groups that are genuine, non-intermittent regressions.""" - # Cheap intermittent gate first (one HTTP call per test) before the mozci walk. - flaky = flakiness.intermittent_tests([fg.test for fg in groups], suite, label) - candidates = [fg.group for fg in groups if fg.test not in flaky] - if not candidates: - return [] - - new = regression.new_test_failures(project, hg_revision, label, candidates) + """The groups whose failure this push introduced.""" + new = regression.new_test_failures(project, hg_revision, config, candidates) for group in candidates: if group not in new: logger.info( diff --git a/services/hackbot-pulse-listener/app/failures.py b/services/hackbot-pulse-listener/app/failures.py deleted file mode 100644 index 8807c618cb..0000000000 --- a/services/hackbot-pulse-listener/app/failures.py +++ /dev/null @@ -1,73 +0,0 @@ -"""Extract the failing test groups from a Taskcluster test task, via mozci. - -A ``task-failed`` pulse event names a failing test *task* but not which test -manifests (groups) actually failed. Rather than fetch and parse artifacts -ourselves, we ask mozci: its errorsummary data source reads the task's structured -results and returns the failing groups with their failing tests and failure -types. This works across harnesses (mochitest / xpcshell / web-platform-tests), -keyed only by task id, and stays in sync with CI format changes. -""" - -from __future__ import annotations - -import logging -from dataclasses import dataclass - -import mozci.push # noqa: F401 (imported so mozci registers its data sources) -from mozci.task import TestTask, is_bad_group, wpt_workaround - -logger = logging.getLogger(__name__) - - -@dataclass(frozen=True) -class FailingGroup: - """A failing test manifest and a representative failing test within it.""" - - group: str - test: str - failure_type: str - - -def _failure_types(task: TestTask) -> dict: - """mozci: ``{group: [(test_name, FailureType), ...]}`` for a task.""" - return task.failure_types - - -def _canonical_group(task: TestTask, group: str) -> str: - """Rewrite a group name the way mozci does. - - The errorsummary names web-platform-tests groups by URL path ("/html/foo.html") - while mozci keys the results we compare against by source path - ("testing/web-platform/tests/html/foo.html"), so the regression check only ever - matches if we apply the same transform. - """ - if task.is_wpt and group.startswith((":/", "/")): - return wpt_workaround(group) - return group - - -def failing_groups(task_id: str, label: str) -> list[FailingGroup]: - """Failing test groups for a task, named the way mozci names them. - - Raises on any mozci/network error rather than returning nothing: an - errorsummary that cannot be read must not be mistaken for "nothing failed", - which would silently drop a real regression. - """ - task = TestTask(id=task_id, label=label) - groups: list[FailingGroup] = [] - for group, fails in _failure_types(task).items(): - if not group or not fails or group == "/": - continue - canonical = _canonical_group(task, group) - # mozci drops these from its own results, so they could never match. - if is_bad_group(task_id, canonical): - continue - test, ftype = fails[0] - groups.append( - FailingGroup( - group=canonical, - test=test, - failure_type=getattr(ftype, "name", str(ftype)), - ) - ) - return groups diff --git a/services/hackbot-pulse-listener/app/flakiness.py b/services/hackbot-pulse-listener/app/flakiness.py deleted file mode 100644 index f3bdfd3c36..0000000000 --- a/services/hackbot-pulse-listener/app/flakiness.py +++ /dev/null @@ -1,241 +0,0 @@ -"""tests.firefox.dev flakiness gate for test-repair. - -The tests.firefox.dev dashboard (github.com/mozilla/aretestsfastyet) is static JS -that reads pre-aggregated timings JSON from the Firefox CI Taskcluster index at -runtime. We consume the same public artifacts (no auth) to get a test's cross-push -flakiness, so the listener can skip clearly intermittent tests before spending an -agent run. - -Only ``mozilla-central`` / ``try`` datasets are published (there is no autoland -index), so this is a flakiness signal only; the autoland last-green push and the -candidate commit range come from mozci. The decode logic (bucket hashing and -per-test stats) is a direct port of the dashboard's ``common-test-data.js``. -""" - -from __future__ import annotations - -import logging -import threading -from collections.abc import Iterable -from dataclasses import dataclass - -import httpx -from cachetools import TTLCache - -from app.config import settings - -logger = logging.getLogger(__name__) - -_INDEX_URL = ( - "https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/" - "gecko.v2.{repo}.latest.source.test-info-{harness}-timings/artifacts/public/{filename}" -) -_TIMEOUT = 30 -_TOTAL_CHUNKS = 64 - -# Decoded buckets, keyed by (harness, chunk, repo). The index publishes a new -# dataset at most a few times a day, and a raw bucket is tens of MB of JSON, so -# caching the (small) decoded stats avoids refetching it for every test. The lock -# covers the fetch and decode together: the worker pool is far larger than the -# number of buckets, and concurrent decodes would dominate the memory limit. -_BUCKET_TTL_SECONDS = 60 * 60 -_buckets: TTLCache = TTLCache(maxsize=32, ttl=_BUCKET_TTL_SECONDS) -_buckets_lock = threading.Lock() - - -@dataclass(frozen=True) -class Flakiness: - """Cross-push pass/fail stats for one test over the timings window.""" - - total: int = 0 - passes: int = 0 - fails: int = 0 - timeouts: int = 0 - crashes: int = 0 - skips: int = 0 - - @property - def failure_rate(self) -> float: - """Fraction of non-skipped runs that failed/timed-out/crashed (0 if none).""" - denom = self.passes + self.fails + self.timeouts + self.crashes - if denom == 0: - return 0.0 - return (self.fails + self.timeouts + self.crashes) / denom - - -def _harness(suite: str, label: str) -> str | None: - """The timings harness for a test task, or None if it publishes none. - - Only the mochitest and xpcshell datasets exist, so every other harness has no - intermittent data to check against. The Taskcluster ``kind`` is not a harness - name (it is often just "test"), so both the suite tag and the label are matched - -- the same suite arrives tagged either way. - """ - text = f"{suite or ''} {label or ''}" - if "xpcshell" in text: - return "xpcshell" - if "mochitest" in text or "browser-chrome" in text: - return "mochitest" - return None - - -def _to_int32(n: int) -> int: - """Wrap to a signed 32-bit int, matching JavaScript's ``| 0``.""" - n &= 0xFFFFFFFF - return n - 0x100000000 if n >= 0x80000000 else n - - -def _chunk_index(full_path: str, total_chunks: int = _TOTAL_CHUNKS) -> int: - """Bucket a test path to 0..63; port of the dashboard's ``getChunkIndex``.""" - h = 0 - for ch in full_path: - h = _to_int32((h << 5) - h + ord(ch)) - return ((h % total_chunks) + total_chunks) % total_chunks - - -def _fetch_bucket(harness: str, chunk: int, repo: str) -> dict: - filename = f"{harness}-{chunk:02x}.json" - url = _INDEX_URL.format(repo=repo, harness=harness, filename=filename) - resp = httpx.get(url, timeout=_TIMEOUT, follow_redirects=True) - resp.raise_for_status() - return resp.json() - - -def _run_count(group: dict) -> int: - """Runs recorded in a status group; port of ``getCountAtIndex`` over all indices.""" - if "counts" in group: - return sum(group["counts"]) - for key in ("durations", "taskIdIds"): - if key in group: - return sum(len(entry) for entry in group[key]) - return len(group.get("days") or []) - - -def _classify(status: str) -> str | None: - """Map a status string to pass/fail/timeout/crash/skip (None = ignored).""" - if status == "SKIP": - return "skip" - if status == "CRASH": - return "crash" - if status.startswith("TIMEOUT"): - return "timeout" - if status == "UNKNOWN": - return None - # PASS-*, OK and EXPECTED-FAIL are all treated as green by the dashboard. - if status.startswith("PASS") or status in ("OK", "EXPECTED-FAIL"): - return "pass" - return "fail" - - -def _test_paths(data: dict) -> dict[int, str]: - """Full test path per test id, from the bucket's index tables.""" - tables = data.get("tables") or {} - info = data.get("testInfo") or {} - paths = tables.get("testPaths") or [] - names = tables.get("testNames") or [] - path_ids = info.get("testPathIds") or [] - name_ids = info.get("testNameIds") or [] - out = {} - for test_id in range(min(len(path_ids), len(name_ids))): - dir_path = paths[path_ids[test_id]] - name = names[name_ids[test_id]] - out[test_id] = f"{dir_path}/{name}" if dir_path else name - return out - - -def _decode_bucket(data: dict) -> dict[str, Flakiness]: - """Per-test stats for every test in a bucket.""" - statuses = (data.get("tables") or {}).get("statuses") or [] - paths = _test_paths(data) - out: dict[str, Flakiness] = {} - for test_id, test_group in enumerate(data.get("testRuns") or []): - path = paths.get(test_id) - if not test_group or path is None: - continue - counts = dict.fromkeys(("pass", "fail", "timeout", "crash", "skip"), 0) - for status_id, group in enumerate(test_group): - if not group or status_id >= len(statuses): - continue - kind = _classify(statuses[status_id]) - if kind is not None: - counts[kind] += _run_count(group) - out[path] = Flakiness( - total=sum(counts.values()), - passes=counts["pass"], - fails=counts["fail"], - timeouts=counts["timeout"], - crashes=counts["crash"], - skips=counts["skip"], - ) - return out - - -def _bucket_stats(harness: str, chunk: int, repo: str) -> dict[str, Flakiness]: - key = (harness, chunk, repo) - with _buckets_lock: - stats = _buckets.get(key) - if stats is None: - stats = _decode_bucket(_fetch_bucket(harness, chunk, repo)) - _buckets[key] = stats - return stats - - -def get_flakiness( - test_path: str, harness: str, repo: str = "mozilla-central" -) -> Flakiness: - """Cross-push flakiness for a test from the tests.firefox.dev timings bucket. - - Fails soft: any network/parse error returns an empty :class:`Flakiness` so the - listener gate treats the test as non-flaky (and errs toward triggering a run). - """ - try: - return _bucket_stats(harness, _chunk_index(test_path), repo).get( - test_path, Flakiness() - ) - except httpx.HTTPStatusError as exc: - # Only some harnesses publish a timings dataset; a 404 means there is - # nothing to look up, so the gate passes the test through unjudged. - if exc.response.status_code == 404: - logger.info( - "No tests.firefox.dev dataset for harness %s; " - "skipping the intermittent check for %s", - harness, - test_path, - ) - else: - logger.warning( - "tests.firefox.dev lookup failed for %s (%s): %s", - test_path, - harness, - exc, - ) - except Exception: - logger.exception( - "tests.firefox.dev lookup failed for %s (%s)", test_path, harness - ) - return Flakiness() - - -def intermittent_tests(tests: Iterable[str], suite: str, label: str) -> set[str]: - """The tests whose historical failure rate marks them as clearly intermittent. - - Empty when the task's harness publishes no timings dataset, logged once for the - task rather than once per test. - """ - harness = _harness(suite, label) - if harness is None: - logger.info( - "No tests.firefox.dev dataset for %s; skipping the intermittent check", - label, - ) - return set() - - flaky = set() - for test in tests: - rate = get_flakiness(test, harness).failure_rate - if rate >= settings.flakiness_threshold: - logger.info( - "Test %s is intermittent (failure rate %.2f); skipping", test, rate - ) - flaky.add(test) - return flaky diff --git a/services/hackbot-pulse-listener/app/regression.py b/services/hackbot-pulse-listener/app/regression.py index 6c90b73d4c..9aef26e542 100644 --- a/services/hackbot-pulse-listener/app/regression.py +++ b/services/hackbot-pulse-listener/app/regression.py @@ -1,133 +1,119 @@ +"""Whether a CI failure is new at a push or inherited from an ancestor. + +The ancestor chain comes from mozci, which resolves it from the hg pushlog -- +Treeherder itself uses mozci for exactly this. The per-push results come from +Treeherder, which keeps them small: mozci would instead pull the push's whole task +list from the Taskcluster queue, tens of megabytes per ancestor. + +A build is looked up by its task label, which is stable across pushes. A test group +is looked up by configuration (platform and build option) instead, because a test +label carries its chunk number and chunk assignments drift between pushes. +""" + import logging import time from mozci.errors import ParentPushNotFound from mozci.push import MAX_DEPTH, Push +from app import treeherder + logger = logging.getLogger(__name__) # Poll an unsettled ancestor for up to MAX_WAIT_SECONDS before giving up. POLL_INTERVAL_SECONDS = 120 MAX_WAIT_SECONDS = 60 * 60 -# mozci spells a green result "passed" (Taskcluster) or "success" (Treeherder). -_PASSED_RESULTS = ("passed", "success") - -# States/results whose outcome isn't knowable yet: still running, or awaiting an +# Treeherder job results and states. +_PASSED_RESULTS = ("success",) +_FAILED_RESULTS = ("testfailed", "busted") +# Outcomes that aren't knowable yet: still queued or running, or awaiting an # auto-retry after an infra exception. -_UNSETTLED_STATES = ("pending", "running", "exception") -_UNSETTLED_RESULTS = ("exception", "retry") +_UNSETTLED_STATES = ("pending", "running") +_UNSETTLED_RESULTS = ("retry", "exception", "unknown") # The deciding ancestor hasn't settled yet; the caller waits and re-checks. _PENDING = object() -def _label_tasks(push: Push, label: str) -> list: - return [t for t in push.tasks if t.label == label] - - -def _unsettled(tasks: list) -> bool: - """Whether any of these runs may still change outcome (running or auto-retried).""" +def _unsettled(jobs: list[dict]) -> bool: + """Whether any of these runs may still change outcome.""" return any( - t.state in _UNSETTLED_STATES or t.result in _UNSETTLED_RESULTS for t in tasks + job["state"] in _UNSETTLED_STATES or job["result"] in _UNSETTLED_RESULTS + for job in jobs ) -def _was_scheduled(push: Push, label: str) -> bool: - """Whether the push originally scheduled the label (results not visible yet).""" - try: - return label in push.scheduled_task_labels - except Exception: - logger.debug("Could not read scheduled task labels for %s", push.rev) - return False - - -def _build_status(push: Push, label: str): +def _build_status(project: str, rev: str, label: str): """'passed'/'failed'/_PENDING/None for a build label on a push. - None is non-decisive (coalesced, never scheduled, or a non-pass/fail terminal - state), so such gaps are skipped rather than mistaken for an inherited failure. + None is non-decisive (never ran here, or a non-pass/fail terminal state), so such + gaps are skipped rather than mistaken for an inherited failure. """ - tasks = _label_tasks(push, label) - if tasks: - if any(t.result in _PASSED_RESULTS for t in tasks): - return "passed" - # Checked before failure: a still-running or auto-retried run may yet turn - # green, and any green run wins, so wait rather than inherit prematurely. - if _unsettled(tasks): - return _PENDING - if any(t.failed for t in tasks): - return "failed" - return None - - # No task here: wait if the build was scheduled (result not visible yet), - # skip if it was coalesced away. - return _PENDING if _was_scheduled(push, label) else None - + jobs = treeherder.label_jobs(project, rev, label) + if any(job["result"] in _PASSED_RESULTS for job in jobs): + return "passed" + # Checked before failure: a still-running or auto-retried run may yet turn green, + # and any green run wins, so wait rather than inherit prematurely. + if _unsettled(jobs): + return _PENDING + if any(job["result"] in _FAILED_RESULTS for job in jobs): + return "failed" + return None -def _group_status(push: Push, group: str, label: str): - """'passed'/'failed'/_PENDING/None for a test group on a push, for one label. - Only tasks sharing the failing task's label are consulted. push.group_summaries - folds every configuration together and reports FAIL when the manifest is broken - on any platform, which would mask a genuine new failure on this one; a label - pins the platform, suite, variant and chunk. +def _group_status(project: str, rev: str, config: tuple[str, str], group: str): + """'passed'/'failed'/_PENDING/None for a test group on a push, in one config. - The precedence matches _build_status: pass, then pending, then fail. + Only this configuration's runs are consulted, so a manifest already broken on + another platform cannot mask a genuine new failure on this one. Same precedence + as _build_status: pass, then pending, then fail. """ - tasks = _label_tasks(push, label) - results = [ - r - for t in tasks - for r in (getattr(t, "results", None) or []) - if r.group == group + jobs = treeherder.config_jobs(project, rev, *config) + results = treeherder.group_results(project, rev) + recorded = [ + results[job["task_id"]][group] + for job in jobs + if group in (results.get(job["task_id"]) or {}) ] - if any(r.ok for r in results): + if any(recorded): return "passed" - if _unsettled(tasks): + if _unsettled(jobs): return _PENDING - if results: + if recorded: return "failed" - # Nothing reported for the group: wait if the label was scheduled here, skip if - # this push never ran it (coalesced, or the manifest was chunked elsewhere). - return _PENDING if not tasks and _was_scheduled(push, label) else None + # Nothing recorded for the group: this push never ran it (coalesced, or the + # manifest was chunked into another task). + return None -def _classify(head: Push, status_fn, describe: str): - """Walk ancestors of `head`; True (new), False (inherited) or _PENDING. +def _classify(project: str, rev: str, status_fn, describe: str, first_pass=True): + """Walk ancestors of `rev`; True (new), False (inherited) or _PENDING. - status_fn(push) reports 'passed'/'failed'/_PENDING/None for the failing unit - (build label or test group). + status_fn(ancestor_rev) reports 'passed'/'failed'/_PENDING/None for the failing + unit (build label or test group). `first_pass` keeps the "not settled" notice to + one line per unit rather than repeating it on every poll for up to an hour. """ - ancestor = head + push = Push(rev, branch=project) for _ in range(MAX_DEPTH): try: - ancestor = ancestor.parent + push = push.parent except ParentPushNotFound: break - status = status_fn(ancestor) + status = status_fn(push.rev) if status is None: continue if status is _PENDING: - logger.info( - "%s not settled at %s; deferring for %s", - describe, - ancestor.rev, - head.rev, - ) + notice = logger.info if first_pass else logger.debug + notice("%s not settled at %s; deferring for %s", describe, push.rev, rev) return _PENDING if status == "failed": logger.info( - "%s already failing at %s; inherited at %s", - describe, - ancestor.rev, - head.rev, + "%s already failing at %s; inherited at %s", describe, push.rev, rev ) return False - logger.info( - "%s passed at %s; new failure at %s", describe, ancestor.rev, head.rev - ) + logger.info("%s passed at %s; new failure at %s", describe, push.rev, rev) return True logger.warning( @@ -136,13 +122,10 @@ def _classify(head: Push, status_fn, describe: str): return True -def _await_new_failures(branch: str, rev: str, status_fn, units, describe: str) -> set: +def _await_new_failures(project: str, rev: str, status_fn, units, describe: str) -> set: """The units whose failure `rev` introduced; the rest were inherited. - status_fn(push, unit) reports 'passed'/'failed'/_PENDING/None. One walk serves - every unit: mozci memoizes a push's task list per instance, so sharing the head - push across units fetches each ancestor's (large) task data once instead of once - per unit. + status_fn(ancestor_rev, unit) reports 'passed'/'failed'/_PENDING/None. Fails open -- undecided units counted as new -- on any error, an ancestor still unsettled past MAX_WAIT_SECONDS, or no deciding ancestor within MAX_DEPTH, so a @@ -151,15 +134,17 @@ def _await_new_failures(branch: str, rev: str, status_fn, units, describe: str) deadline = time.monotonic() + MAX_WAIT_SECONDS unresolved = list(units) new: set = set() + first_pass = True while True: try: - # A fresh head each attempt is what re-reads live data for a push whose - # ancestors have not finished yet. - head = Push(rev, branch=branch) pending = [] for unit in unresolved: state = _classify( - head, lambda push, u=unit: status_fn(push, u), f"{describe} {unit}" + project, + rev, + lambda ancestor, u=unit: status_fn(ancestor, u), + f"{describe} {unit}", + first_pass=first_pass, ) if state is _PENDING: pending.append(unit) @@ -191,19 +176,35 @@ def _await_new_failures(branch: str, rev: str, status_fn, units, describe: str) ) time.sleep(POLL_INTERVAL_SECONDS) unresolved = pending + first_pass = False -def is_new_build_failure(branch: str, rev: str, label: str) -> bool: +def is_new_build_failure(project: str, rev: str, label: str) -> bool: """True if this push introduced the build failure, False if it inherited it.""" - return label in _await_new_failures(branch, rev, _build_status, [label], "build") + return label in _await_new_failures( + project, + rev, + lambda ancestor, unit: _build_status(project, ancestor, unit), + [label], + "build", + ) + +def new_test_failures( + project: str, rev: str, config: tuple[str, str], groups: list[str] +) -> set[str]: + """The failing groups this push introduced, for one configuration. -def new_test_failures(branch: str, rev: str, label: str, groups: list[str]) -> set[str]: - """The failing groups this push introduced, for one task's label.""" + `config` is (platform, platform_option). With no configuration to compare + against, every group is reported as new rather than silently dropped. + """ + if not all(config): + logger.info("No configuration for %s; running agent", rev) + return set(groups) return _await_new_failures( - branch, + project, rev, - lambda push, group: _group_status(push, group, label), + lambda ancestor, group: _group_status(project, ancestor, config, group), groups, "group", ) diff --git a/services/hackbot-pulse-listener/app/treeherder.py b/services/hackbot-pulse-listener/app/treeherder.py new file mode 100644 index 0000000000..2546f11d31 --- /dev/null +++ b/services/hackbot-pulse-listener/app/treeherder.py @@ -0,0 +1,284 @@ +"""Treeherder classification gate for test-repair. + +Treeherder ingests the same Taskcluster failures the listener sees and classifies +each failing job while parsing its log -- a cross-push analysis of per-manifest +pass rates, refined afterwards by sheriffs and by mozci's autoclassifier. Reading +that verdict is cheaper and broader than judging intermittency ourselves: it covers +every harness, where the tests.firefox.dev timings datasets it replaces published +only mochitest and xpcshell, leaving reftest and web-platform-tests unjudged. + +Ingestion trails the failure message (measured on autoland: ~50s median, ~3min at +p90), so the lookup waits for the job to appear before reading its classification. +""" + +from __future__ import annotations + +import logging +import threading +import time +from urllib.parse import quote + +import httpx +from cachetools import TTLCache + +from app.config import settings + +logger = logging.getLogger(__name__) + +_TIMEOUT = 15 + +# Read-through caches, so one ancestor walk fetches each push once instead of once +# per failing group, and the many failing tasks of a push share a fetch. The TTL is +# deliberately shorter than the regression check's poll interval, so a walk waiting +# on an unsettled ancestor still sees fresh results on its next attempt. +_TTL_SECONDS = 60 +_group_cache: TTLCache = TTLCache(maxsize=128, ttl=_TTL_SECONDS) +_jobs_cache: TTLCache = TTLCache(maxsize=512, ttl=_TTL_SECONDS) +# A revision never maps to a different push, so this one is held far longer. +_push_id_cache: TTLCache = TTLCache(maxsize=512, ttl=6 * 60 * 60) +_cache_lock = threading.Lock() + +# /api/failureclassification/ +_CLASSIFICATIONS = { + 1: "not classified", + 2: "fixed by commit", + 3: "expected fail", + 4: "intermittent", + 5: "infra", + 6: "new failure not classified", + 7: "autoclassified intermittent", + 8: "intermittent needs bugid", +} + +# Verdicts that mean this failure is not a new regression worth an agent run. +# The two left out -- "not classified" and "new failure not classified" -- are the +# ones that still may be a real regression. +_NOT_A_REGRESSION = {2, 3, 4, 5, 7, 8} + + +def _job(project: str, task_id: str) -> dict | None: + """The Treeherder job for a Taskcluster task, or None if not ingested yet. + + Keyed on the task id, which is the one identifier a failure message carries. + ``jobs/?revision=`` looks like it would work but silently ignores the filter + and returns unrelated jobs, so it must not be used here. + """ + url = ( + f"{settings.treeherder_url.rstrip('/')}/api/project/{project}/jobs/" + f"?task_id={task_id}" + ) + resp = httpx.get(url, timeout=_TIMEOUT, follow_redirects=True) + resp.raise_for_status() + results = resp.json().get("results") or [] + return results[0] if results else None + + +def _get(path: str) -> dict: + url = f"{settings.treeherder_url.rstrip('/')}/api/project/{path}" + resp = httpx.get(url, timeout=_TIMEOUT, follow_redirects=True) + resp.raise_for_status() + return resp.json() + + +def group_results( + project: str, revision: str, refresh: bool = False +) -> dict[str, dict[str, bool]]: + """``{task_id: {group: passed}}`` for a push, as Treeherder recorded it. + + Needs the full 40-character revision; a short one 404s. + + Note a group can be recorded as failing on a task whose job did not fail (wpt + expected-fail metadata), so this is only meaningful for a task already known to + have failed. + """ + key = (project, revision) + with _cache_lock: + if not refresh and key in _group_cache: + return _group_cache[key] + + results = _get(f"{project}/push/group_results/?revision={revision}") + with _cache_lock: + _group_cache[key] = results + return results + + +def push_id(project: str, revision: str) -> int | None: + """Treeherder's push id for a revision, or None if it has no push.""" + key = (project, revision) + with _cache_lock: + if key in _push_id_cache: + return _push_id_cache[key] + + results = _get(f"{project}/push/?revision={revision}").get("results") or [] + value = results[0]["id"] if results else None + with _cache_lock: + _push_id_cache[key] = value + return value + + +def _summarize(job: dict) -> dict: + return { + "result": job.get("result"), + "state": job.get("state"), + "task_id": job.get("task_id"), + "platform_option": job.get("platform_option"), + } + + +def _push_jobs(project: str, revision: str, query: str, key) -> list[dict]: + """Cached job query on a push. + + Pending and running jobs are included, which is how a not-yet-finished ancestor + is told apart from one that never ran the task at all. + """ + with _cache_lock: + if key in _jobs_cache: + return _jobs_cache[key] + + push = push_id(project, revision) + jobs = [] + if push is not None: + results = _get(f"{project}/jobs/?push_id={push}&{query}&count=2000") + jobs = [_summarize(job) for job in results.get("results") or []] + with _cache_lock: + _jobs_cache[key] = jobs + return jobs + + +def label_jobs(project: str, revision: str, label: str) -> list[dict]: + """The runs of one exact task label on a push, for build labels. + + Build labels carry no chunk number, so they are stable across pushes. + """ + return _push_jobs( + project, + revision, + f"job_type_name={quote(label, safe='')}", + (project, revision, "label", label), + ) + + +def config_jobs( + project: str, revision: str, platform: str, platform_option: str +) -> list[dict]: + """The runs of one configuration on a push -- platform plus build option. + + Not keyed on the task label: a test label carries its chunk number, and chunk + assignments drift between pushes, so comparing by label makes ancestors look as + though they never ran the manifest. Platform and option are stable, and a group + only appears on the tasks that actually ran it, so the group name itself selects + the right suite. + + ``platform_option`` is filtered here rather than in the query: Treeherder accepts + the parameter but silently ignores it. + """ + jobs = _push_jobs( + project, + revision, + f"platform={quote(platform, safe='')}", + (project, revision, "platform", platform), + ) + return [job for job in jobs if job["platform_option"] == platform_option] + + +class GroupResultsUnavailable(Exception): + """Treeherder records no per-manifest results for a task.""" + + +def failing_groups(project: str, revision: str, task_id: str) -> list[str]: + """The manifests this task reported as failing. + + An empty list means the task failed nothing at manifest level. Raises when the + task has no results at all -- which is not the same thing, and must not be + mistaken for it: a task-level failure (crash, timeout, harness error) or a log + that is still being parsed would otherwise be silently dropped. Network errors + propagate for the same reason. + """ + results = group_results(project, revision) + if task_id not in results: + # A cached snapshot can predate this task's ingestion; refetch once before + # concluding that Treeherder has nothing for it. + results = group_results(project, revision, refresh=True) + if task_id not in results: + raise GroupResultsUnavailable( + f"Treeherder records no group results for task {task_id}" + ) + return [ + group + for group, passed in results[task_id].items() + if not passed and group.strip() and group != "/" + ] + + +def job_for_task(project: str, task_id: str) -> dict | None: + """The Treeherder job for a failing task, waiting for it to be ingested. + + None when Treeherder never ingests it or cannot be reached, which callers must + treat as "no verdict" rather than as a reason to drop the failure. + """ + deadline = time.monotonic() + settings.treeherder_ingest_max_wait_seconds + while True: + try: + job = _job(project, task_id) + except Exception: + logger.exception( + "Treeherder lookup failed for task %s; investigating", task_id + ) + return None + if job is not None: + return job + if time.monotonic() >= deadline: + logger.info( + "Treeherder has not ingested task %s after %ss; investigating", + task_id, + settings.treeherder_ingest_max_wait_seconds, + ) + return None + time.sleep(settings.treeherder_ingest_poll_seconds) + + +def await_skip_reason(project: str, task_id: str) -> str | None: + """Wait a bounded time for a verdict that this failure is not worth a run. + + For a task with no per-manifest results there is nothing to compare against an + ancestor, so nothing else delays the decision -- and in practice these are mostly + intermittents and expected failures that Treeherder classifies a few minutes + later. Returns None once the wait is spent, so an unclassified failure is still + investigated rather than dropped. + """ + deadline = time.monotonic() + settings.treeherder_classification_wait_seconds + while True: + reason = recheck_skip_reason(project, task_id) + if reason: + return reason + if time.monotonic() >= deadline: + return None + time.sleep(settings.treeherder_ingest_poll_seconds) + + +def recheck_skip_reason(project: str, task_id: str) -> str | None: + """Re-read the classification without waiting for ingestion. + + Classification of an intermittent usually lands after we first look, but a + regression check takes minutes, so it is worth asking again before spending a + run. Returns None on any error: a failed re-check must not drop the failure. + """ + try: + return skip_reason(_job(project, task_id)) + except Exception: + logger.exception( + "Treeherder re-check failed for task %s; investigating", task_id + ) + return None + + +def skip_reason(job: dict | None) -> str | None: + """Treeherder's reason not to investigate this failure, or None to proceed. + + Fails open (None) for a missing or unclassified job: an absent verdict must never + drop a real regression. + """ + classification = (job or {}).get("failure_classification_id") + if classification in _NOT_A_REGRESSION: + return _CLASSIFICATIONS.get(classification, str(classification)) + return None diff --git a/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json b/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json deleted file mode 100644 index 54917444c1..0000000000 --- a/services/hackbot-pulse-listener/tests/fixtures/xpcshell-bucket.json +++ /dev/null @@ -1,29 +0,0 @@ -{ - "metadata": { "startDate": "2026.07.01", "days": 21 }, - "tables": { - "testPaths": ["dom/base/test"], - "testNames": ["test_foo.js", "test_bar.js"], - "statuses": [ - "PASS-PARALLEL", - "FAIL-PARALLEL", - "TIMEOUT-SEQUENTIAL", - "SKIP", - "CRASH" - ], - "taskIds": ["aaa.0", "bbb.0", "ccc.0"] - }, - "testInfo": { - "testPathIds": [0, 0], - "testNameIds": [0, 1] - }, - "testRuns": [ - [ - { "durations": [[120, 130], [140]], "days": [0, 2] }, - { "taskIdIds": [[0]], "days": [2] }, - { "taskIdIds": [[1]], "days": [1] }, - null, - null - ], - [null, null, null, { "counts": [4], "days": [0] }, null] - ] -} diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index abc3df38d6..112ad4a05c 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -5,7 +5,6 @@ import pytest from app import consumer -from app.failures import FailingGroup FIXTURES = Path(__file__).parent / "fixtures" @@ -42,7 +41,9 @@ def test_sample_messages_route_to_test_repair_not_build(): executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), - patch.object(consumer.failures, "failing_groups", return_value=[]) as fg, + patch.object(consumer.treeherder, "failing_groups", return_value=[]) as fg, + patch.object(consumer.treeherder, "job_for_task", return_value=None), + patch.object(consumer.treeherder, "skip_reason", return_value=None), patch.object(consumer.client, "trigger_run") as trigger, ): for body in _sample_bodies(): @@ -241,11 +242,7 @@ def _test_msg( } -_GROUP = FailingGroup( - group="dom/base/test/mochitest.ini", - test="dom/base/test/test_a.js", - failure_type="GENERIC", -) +_GROUP = "dom/base/test/mochitest.ini" @pytest.fixture @@ -258,7 +255,16 @@ def env(monkeypatch): mocks = SimpleNamespace( get_hg_revision=MagicMock(return_value="hgrev"), failing_groups=MagicMock(return_value=[_GROUP]), - intermittent_tests=MagicMock(return_value=set()), + job_for_task=MagicMock( + return_value={ + "failure_classification_id": 6, + "platform": "linux1804-64", + "platform_option": "opt", + } + ), + skip_reason=MagicMock(return_value=None), + recheck_skip_reason=MagicMock(return_value=None), + await_skip_reason=MagicMock(return_value=None), new_test_failures=MagicMock( side_effect=lambda p, r, label, groups: set(groups) ), @@ -267,9 +273,14 @@ def env(monkeypatch): executor=MagicMock(), ) monkeypatch.setattr(consumer.taskcluster, "get_hg_revision", mocks.get_hg_revision) - monkeypatch.setattr(consumer.failures, "failing_groups", mocks.failing_groups) + monkeypatch.setattr(consumer.treeherder, "failing_groups", mocks.failing_groups) + monkeypatch.setattr(consumer.treeherder, "job_for_task", mocks.job_for_task) + monkeypatch.setattr(consumer.treeherder, "skip_reason", mocks.skip_reason) monkeypatch.setattr( - consumer.flakiness, "intermittent_tests", mocks.intermittent_tests + consumer.treeherder, "recheck_skip_reason", mocks.recheck_skip_reason + ) + monkeypatch.setattr( + consumer.treeherder, "await_skip_reason", mocks.await_skip_reason ) monkeypatch.setattr( consumer.regression, "new_test_failures", mocks.new_test_failures @@ -296,16 +307,25 @@ def test_test_failure_triggers_rca_run(env): fn, ctx = env.executor.submit.call_args.args assert fn is consumer.worker.poll_and_notify assert ctx.agent == "test-repair" - assert ctx.test_groups == [_GROUP.group] + assert ctx.test_groups == [_GROUP] -def test_intermittent_test_skipped_before_mozci(env): - env.intermittent_tests.return_value = {_GROUP.test} +def test_treeherder_intermittent_skipped_before_any_walk(env): + # Treeherder's own verdict rules the failure out before any mozci work. + env.skip_reason.return_value = "intermittent" assert consumer.process(_test_msg(), env.executor) is None + env.failing_groups.assert_not_called() env.new_test_failures.assert_not_called() env.trigger_run.assert_not_called() +def test_unclassified_failure_is_investigated(env): + # "not classified" / "new failure" leave the decision to the mozci walk. + env.skip_reason.return_value = None + assert consumer.process(_test_msg(), env.executor) == "tr-1" + env.new_test_failures.assert_called_once() + + def test_inherited_test_group_skipped(env): env.new_test_failures.side_effect = lambda *_: set() assert consumer.process(_test_msg(), env.executor) is None @@ -324,21 +344,22 @@ def test_no_failing_groups_skips(env): env.trigger_run.assert_not_called() -def test_unreadable_errorsummary_still_triggers_run(env): - # Fail open like the build path: an errorsummary that cannot be read must not - # be mistaken for "nothing failed" and drop a possible regression. - env.failing_groups.side_effect = RuntimeError("no artifact") +def test_task_without_group_results_still_triggers_run(env): + # A task-level failure (crash, timeout) records no per-manifest results; that + # must not be mistaken for "nothing failed" and drop a real regression. + env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable( + "no group results for task ERR" + ) assert consumer.process(_test_msg(task_id="ERR"), env.executor) == "tr-1" env.trigger_run.assert_called_once() # With no groups resolved there is nothing to filter or to name. - env.intermittent_tests.assert_not_called() env.new_test_failures.assert_not_called() _, ctx = env.executor.submit.call_args.args assert ctx.test_groups == [] -def test_unreadable_errorsummary_triggers_once_per_task(env): - env.failing_groups.side_effect = RuntimeError("no artifact") +def test_unreadable_group_results_triggers_once_per_task(env): + env.failing_groups.side_effect = RuntimeError("treeherder down") consumer.process(_test_msg(task_id="A"), env.executor) consumer.process(_test_msg(task_id="B"), env.executor) env.trigger_run.assert_called_once() @@ -379,10 +400,7 @@ def test_test_repair_trigger_failure_releases_group_for_retry(env): def test_multiple_failing_groups_trigger_one_run_per_task(env): - groups = [ - FailingGroup("dom/base/test/mochitest.ini", "dom/base/test/a.js", "GENERIC"), - FailingGroup("layout/test/mochitest.ini", "layout/test/b.js", "TIMEOUT"), - ] + groups = ["dom/base/test/mochitest.ini", "layout/test/mochitest.ini"] env.failing_groups.return_value = groups assert consumer.process(_test_msg(), env.executor) == "tr-1" @@ -394,7 +412,7 @@ def test_multiple_failing_groups_trigger_one_run_per_task(env): assert env.executor.submit.call_count == 1 # Every failing group is named, not an arbitrary one of them. _, ctx = env.executor.submit.call_args.args - assert ctx.test_groups == [g.group for g in groups] + assert ctx.test_groups == groups def test_missing_hg_revision_skips_test_task(env): @@ -405,23 +423,13 @@ def test_missing_hg_revision_skips_test_task(env): env.trigger_run.assert_not_called() -def test_intermittent_gate_runs_before_the_mozci_walk(env): - # Both gates are batch calls over the task's groups; the cheap one goes first - # and the expensive one only sees what survived. - groups = [ - FailingGroup("a/mochitest.ini", "a/test_a.js", "GENERIC"), - FailingGroup("b/mochitest.ini", "b/test_b.js", "GENERIC"), - ] +def test_every_failing_group_reaches_the_mozci_walk(env): + groups = ["a/mochitest.ini", "b/mochitest.ini"] env.failing_groups.return_value = groups - env.intermittent_tests.return_value = {"a/test_a.js"} + env.new_test_failures.side_effect = lambda p, r, cfg, gs: {"b/mochitest.ini"} assert consumer.process(_test_msg(), env.executor) == "tr-1" - env.intermittent_tests.assert_called_once() - tests, suite, label = env.intermittent_tests.call_args.args - assert list(tests) == ["a/test_a.js", "b/test_b.js"] - assert suite == "mochitest-browser-chrome" - # Only the surviving group reaches the walk. - assert env.new_test_failures.call_args.args[3] == ["b/mochitest.ini"] + assert env.new_test_failures.call_args.args[3] == groups _, ctx = env.executor.submit.call_args.args assert ctx.test_groups == ["b/mochitest.ini"] @@ -436,3 +444,59 @@ def test_queue_name_omits_production_environment(): with patch.object(consumer.settings, "environment", "production"): (queue,) = consumer._build_queues("guest") assert queue.name == "queue/guest/build-repair-task-failed" + + +def test_classification_landing_during_the_check_cancels_the_run(env): + # The regression check takes minutes, which is about how long Treeherder needs + # to classify an intermittent; a verdict that arrives meanwhile must win. + env.recheck_skip_reason.return_value = "intermittent" + assert consumer.process(_test_msg(), env.executor) is None + env.trigger_run.assert_not_called() + + +def test_recheck_happens_after_the_regression_check(env): + order = [] + env.new_test_failures.side_effect = lambda p, r, cfg, gs: ( + order.append("walk"), + set(gs), + )[1] + env.recheck_skip_reason.side_effect = lambda p, t: order.append("recheck") + + assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert order == ["walk", "recheck"] + + +def test_backfill_in_a_new_task_group_is_deduped(env): + # Backfills and retriggers are dispatched by action tasks, which start their own + # Taskcluster task group. The same push+group must still be investigated once. + assert consumer.process(_test_msg(task_id="A", group_id="G1"), env.executor) + assert ( + consumer.process(_test_msg(task_id="B", group_id="ACTION-GROUP"), env.executor) + is None + ) + env.trigger_run.assert_called_once() + + +def test_different_pushes_are_not_deduped(env): + # Dedupe is per push: the same manifest newly failing on a later push is a + # separate regression and must be investigated again. + revisions = iter(["rev-one", "rev-two"]) + env.get_hg_revision.side_effect = lambda task_id: next(revisions) + consumer.process(_test_msg(task_id="A"), env.executor) + consumer.process(_test_msg(task_id="B"), env.executor) + assert env.trigger_run.call_count == 2 + + +def test_whole_task_waits_for_a_verdict_before_triggering(env): + env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") + env.await_skip_reason.return_value = "intermittent" + assert consumer.process(_test_msg(), env.executor) is None + env.await_skip_reason.assert_called_once() + env.trigger_run.assert_not_called() + + +def test_whole_task_triggers_when_no_verdict_arrives(env): + env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") + env.await_skip_reason.return_value = None + assert consumer.process(_test_msg(), env.executor) == "tr-1" + env.trigger_run.assert_called_once() diff --git a/services/hackbot-pulse-listener/tests/test_failures.py b/services/hackbot-pulse-listener/tests/test_failures.py deleted file mode 100644 index ca544f1a58..0000000000 --- a/services/hackbot-pulse-listener/tests/test_failures.py +++ /dev/null @@ -1,86 +0,0 @@ -"""Tests for extracting failing test groups from a task via mozci.""" - -from types import SimpleNamespace - -import pytest -from app import failures - -LABEL = "test-linux1804-64/opt-mochitest-browser-chrome-1" -WPT_LABEL = "test-linux1804-64/opt-web-platform-tests-2" - - -def _ftype(name): - return SimpleNamespace(name=name) - - -def test_failing_groups_maps_mozci_output(monkeypatch): - fake = { - "browser/base/content/test/bf/browser.toml": [ - ("browser/base/content/test/bf/browser_a.js", _ftype("GENERIC")), - ], - "dom/base/test/mochitest.ini": [ - ("dom/base/test/test_c.js", _ftype("TIMEOUT")), - ], - } - monkeypatch.setattr(failures, "_failure_types", lambda t: fake) - groups = failures.failing_groups("TASK", LABEL) - assert {g.group for g in groups} == set(fake) - bf = next(g for g in groups if g.group.endswith("browser.toml")) - assert bf.test == "browser/base/content/test/bf/browser_a.js" - assert bf.failure_type == "GENERIC" - assert next( - g for g in groups if g.group.endswith("mochitest.ini") - ).failure_type == ("TIMEOUT") - - -def test_failing_groups_skips_empty_group(monkeypatch): - monkeypatch.setattr(failures, "_failure_types", lambda t: {"grp": []}) - assert failures.failing_groups("TASK", LABEL) == [] - - -def test_failing_groups_raises_on_mozci_error(monkeypatch): - # Must not be reported as "nothing failed": the caller has to tell an - # unreadable errorsummary apart from a task with no failing groups. - def boom(task_id): - raise RuntimeError("mozci could not read the task") - - monkeypatch.setattr(failures, "_failure_types", boom) - with pytest.raises(RuntimeError): - failures.failing_groups("TASK", LABEL) - - -def test_wpt_group_names_are_normalized_to_source_paths(monkeypatch): - # The errorsummary names wpt groups by URL path; mozci keys its results by - # source path, and the regression check compares the two. - fake = { - "/html/browsers/the-window-object/foo.html": [("foo.html", _ftype("GENERIC"))], - "/_mozilla/dom/bar.html": [("bar.html", _ftype("GENERIC"))], - } - monkeypatch.setattr(failures, "_failure_types", lambda t: fake) - groups = {g.group for g in failures.failing_groups("TASK", WPT_LABEL)} - assert groups == { - "testing/web-platform/tests/html/browsers/the-window-object/foo.html", - "testing/web-platform/mozilla/tests/dom/bar.html", - } - - -def test_wpt_root_group_is_dropped(monkeypatch): - monkeypatch.setattr( - failures, "_failure_types", lambda t: {"/": [("x", _ftype("GENERIC"))]} - ) - assert failures.failing_groups("TASK", WPT_LABEL) == [] - - -def test_non_wpt_group_names_are_untouched(monkeypatch): - fake = {"dom/base/test/mochitest.ini": [("dom/base/test/a.js", _ftype("GENERIC"))]} - monkeypatch.setattr(failures, "_failure_types", lambda t: fake) - (group,) = failures.failing_groups("TASK", LABEL) - assert group.group == "dom/base/test/mochitest.ini" - - -def test_unusable_group_names_are_dropped(monkeypatch): - # mozci drops absolute and Windows-style names from its own results, so a - # group named that way could never match anything. - fake = {"Z:\\build\\tests\\mochitest.ini": [("a.js", _ftype("GENERIC"))]} - monkeypatch.setattr(failures, "_failure_types", lambda t: fake) - assert failures.failing_groups("TASK", LABEL) == [] diff --git a/services/hackbot-pulse-listener/tests/test_flakiness.py b/services/hackbot-pulse-listener/tests/test_flakiness.py deleted file mode 100644 index a77577a2d6..0000000000 --- a/services/hackbot-pulse-listener/tests/test_flakiness.py +++ /dev/null @@ -1,218 +0,0 @@ -import json -from pathlib import Path - -import pytest -from app import flakiness - -FIXTURE = Path(__file__).parent / "fixtures" / "xpcshell-bucket.json" - - -@pytest.fixture -def bucket() -> dict: - return json.loads(FIXTURE.read_text()) - - -@pytest.fixture(autouse=True) -def _clear_bucket_cache(): - flakiness._buckets.clear() - yield - flakiness._buckets.clear() - - -def _stats(bucket: dict, test_path: str) -> flakiness.Flakiness: - return flakiness._decode_bucket(bucket).get(test_path, flakiness.Flakiness()) - - -def test_chunk_index_matches_reference_hash(): - # Port of the dashboard's getChunkIndex (32-bit signed string hash mod 64). - assert flakiness._chunk_index("") == 0 - assert flakiness._chunk_index("a") == 33 - assert flakiness._chunk_index("ab") == 33 - # Always in range and deterministic. - idx = flakiness._chunk_index("dom/base/test/test_foo.js") - assert 0 <= idx < 64 - assert idx == flakiness._chunk_index("dom/base/test/test_foo.js") - - -def test_decode_bucket_counts_and_rate(bucket): - stats = _stats(bucket, "dom/base/test/test_foo.js") - # foo: 3 passes (durations 2+1), 1 fail, 1 timeout, no skip/crash. - assert stats.passes == 3 - assert stats.fails == 1 - assert stats.timeouts == 1 - assert stats.crashes == 0 - assert stats.skips == 0 - assert stats.total == 5 - # failure_rate counts fail+timeout+crash over pass+fail+timeout+crash = 2/5. - assert stats.failure_rate == pytest.approx(0.4) - - -def test_decode_bucket_covers_every_test_in_one_pass(bucket): - # Decoded once per bucket and cached, so every test in it must be present. - assert set(flakiness._decode_bucket(bucket)) == { - "dom/base/test/test_foo.js", - "dom/base/test/test_bar.js", - } - - -def test_decode_bucket_skip_only_test(bucket): - stats = _stats(bucket, "dom/base/test/test_bar.js") - assert stats.skips == 4 - assert stats.passes == 0 - assert stats.fails == 0 - # No pass/fail/timeout/crash -> rate is 0, not a division error. - assert stats.failure_rate == 0.0 - - -def test_decode_bucket_unknown_test_returns_empty(bucket): - stats = _stats(bucket, "does/not/exist.js") - assert stats.total == 0 - assert stats.failure_rate == 0.0 - - -def test_get_flakiness_uses_bucket(monkeypatch, bucket): - captured = {} - - def fake_fetch(harness, chunk, repo): - captured["harness"] = harness - captured["chunk"] = chunk - captured["repo"] = repo - return bucket - - monkeypatch.setattr(flakiness, "_fetch_bucket", fake_fetch) - stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") - assert stats.passes == 3 - assert captured["harness"] == "xpcshell" - assert captured["repo"] == "mozilla-central" - assert captured["chunk"] == flakiness._chunk_index("dom/base/test/test_foo.js") - - -def test_get_flakiness_fails_soft_on_network_error(monkeypatch): - def boom(*a, **k): - raise RuntimeError("network down") - - monkeypatch.setattr(flakiness, "_fetch_bucket", boom) - stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") - assert stats.total == 0 - assert stats.failure_rate == 0.0 - - -def test_get_flakiness_fails_soft_on_unexpected_bucket_shape(monkeypatch): - # The bucket layout is an undocumented internal format of the dashboard, so a - # shape change must not escape and abort the whole failing task. - monkeypatch.setattr( - flakiness, "_fetch_bucket", lambda *a, **k: ["not", "a", "dict"] - ) - stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") - assert stats.total == 0 - - -def _http_error(status_code): - request = flakiness.httpx.Request("GET", "https://example/timings.json") - response = flakiness.httpx.Response(status_code, request=request) - return flakiness.httpx.HTTPStatusError("boom", request=request, response=response) - - -def test_unpublished_harness_404_is_not_an_error(monkeypatch, caplog): - # Only some harnesses publish a timings dataset (e.g. reftest does not). - # A 404 is an expected "no data", not an error worth a traceback/Sentry event. - def not_found(*a, **k): - raise _http_error(404) - - monkeypatch.setattr(flakiness, "_fetch_bucket", not_found) - with caplog.at_level("DEBUG"): - stats = flakiness.get_flakiness("editor/reftests/a.html", "reftest") - - assert stats.total == 0 - assert not [r for r in caplog.records if r.levelname in ("ERROR", "WARNING")] - assert not any(r.exc_info for r in caplog.records) - - -def test_unexpected_http_status_warns_without_traceback(monkeypatch, caplog): - def server_error(*a, **k): - raise _http_error(500) - - monkeypatch.setattr(flakiness, "_fetch_bucket", server_error) - with caplog.at_level("DEBUG"): - stats = flakiness.get_flakiness("dom/base/test/test_foo.js", "xpcshell") - - assert stats.total == 0 - assert [r for r in caplog.records if r.levelname == "WARNING"] - - -def test_harness_detection(): - # xpcshell via the test-suite tag, and via the label when the suite is generic. - assert flakiness._harness("xpcshell", "irrelevant") == "xpcshell" - assert flakiness._harness("test", "test-linux/opt-xpcshell-4") == "xpcshell" - assert flakiness._harness("mochitest-browser-chrome", "l") == "mochitest" - # Recognized from the label too: the same suite arrives with no test-suite tag, - # and the Taskcluster kind ("test") is not a harness. - assert ( - flakiness._harness("", "test-linux2404-64/opt-mochitest-browser-chrome-8") - == "mochitest" - ) - # Harnesses that publish no timings dataset resolve to None (no lookup). - assert flakiness._harness("web-platform-tests", "l") is None - assert flakiness._harness("", "test-linux/opt-reftest-1") is None - - -def test_bucket_is_fetched_once_for_every_test_in_it(monkeypatch, bucket): - # The dataset changes a few times a day and a real bucket is tens of MB, so a - # second test in the same bucket must not refetch it. - calls = [] - - def fake_fetch(harness, chunk, repo): - calls.append(chunk) - return bucket - - monkeypatch.setattr(flakiness, "_fetch_bucket", fake_fetch) - foo = "dom/base/test/test_foo.js" - assert flakiness._chunk_index(foo) == flakiness._chunk_index(foo) - flakiness.get_flakiness(foo, "xpcshell") - flakiness.get_flakiness(foo, "xpcshell") - assert calls == [flakiness._chunk_index(foo)] - - -def test_intermittent_tests_skips_lookup_without_a_dataset(monkeypatch): - # reftest/wpt publish nothing, so the lookup would be a guaranteed 404. - called = [] - monkeypatch.setattr( - flakiness, "_fetch_bucket", lambda *a, **k: called.append(a) or {} - ) - assert ( - flakiness.intermittent_tests(["a.html"], "reftest", "test-linux/reftest-1") - == set() - ) - assert called == [] - - -def test_intermittent_tests_flags_only_tests_over_the_threshold(monkeypatch): - rates = {"flaky.js": 0.30, "solid.js": 0.001} - monkeypatch.setattr( - flakiness, - "get_flakiness", - lambda test, harness, repo="mozilla-central": flakiness.Flakiness( - total=1000, - passes=int(1000 * (1 - rates[test])), - fails=int(1000 * rates[test]), - ), - ) - flaky = flakiness.intermittent_tests( - ["flaky.js", "solid.js"], "xpcshell", "test-linux/opt-xpcshell-1" - ) - assert flaky == {"flaky.js"} - - -def test_few_percent_failure_rate_counts_as_intermittent(monkeypatch): - # The timings dataset records one entry per task run per day, so even the - # flakiest tests sit at a few percent. A threshold tuned for a per-push rate - # would sit above every real intermittent and never fire. - monkeypatch.setattr( - flakiness, - "get_flakiness", - lambda *a, **k: flakiness.Flakiness(total=2000, passes=1880, fails=120), - ) - flaky = flakiness.intermittent_tests( - ["a.js"], "xpcshell", "test-linux/opt-xpcshell-1" - ) - assert flaky == {"a.js"} diff --git a/services/hackbot-pulse-listener/tests/test_regression.py b/services/hackbot-pulse-listener/tests/test_regression.py index 520fcd37a9..0721f999fa 100644 --- a/services/hackbot-pulse-listener/tests/test_regression.py +++ b/services/hackbot-pulse-listener/tests/test_regression.py @@ -1,125 +1,139 @@ +"""Tests for the build-failure regression gate (is_new_build_failure).""" + from unittest.mock import patch +import pytest from app import regression from mozci.errors import ParentPushNotFound LABEL = "build-linux64/opt" +OTHER_LABEL = "build-macosx64/opt" -class FakeTask: - def __init__(self, label=LABEL, state="completed", result="passed"): - self.label = label - self.state = state - self.result = result +def _job(result="success", state="completed", task_id="T"): + return {"result": result, "state": state, "task_id": task_id} - @property - def failed(self): - return self.result in ("busted", "failed", "exception") +def passed(): + return [_job(result="success")] -class FakePush: - def __init__(self, rev, tasks, parent=None): - self.rev = rev - self.tasks = tasks - self._parent = parent - @property - def parent(self): - if self._parent is None: - raise ParentPushNotFound("no parent", rev=self.rev, branch="autoland") - return self._parent +def failed(): + return [_job(result="testfailed")] + +def busted(): + return [_job(result="busted")] -def _passed(label=LABEL): - return [FakeTask(label=label, result="passed")] +def running(): + return [_job(result=None, state="running")] -def _failed(label=LABEL): - return [FakeTask(label=label, result="failed")] +def retried(): + return [_job(result="retry")] -def _infra(label=LABEL): - return [FakeTask(label=label, state="exception", result="exception")] +def never_ran(): + return [] -def _running(label=LABEL): - return [FakeTask(label=label, state="running", result=None)] +class FakePush: + """A push whose only job is to yield the ancestor chain, as mozci does.""" -def _chain(*task_lists): - """Build a parent chain: task_lists[0] is the observed push, [1] its parent, ...""" + def __init__(self, rev, parent=None): + self.rev = rev + self._parent = parent + + @property + def parent(self): + if self._parent is None: + raise ParentPushNotFound("no parent", rev=self.rev, branch="autoland") + return self._parent + + +def _chain(*ancestors): + """Ancestor revs rev1..revN, and the Treeherder jobs recorded on each.""" push = None - for i, tasks in enumerate(reversed(task_lists)): - push = FakePush(rev=f"rev{len(task_lists) - 1 - i}", tasks=tasks, parent=push) - return push + for i in range(len(ancestors), 0, -1): + push = FakePush(f"rev{i}", parent=push) + jobs = {f"rev{i + 1}": a for i, a in enumerate(ancestors)} + return FakePush("head", parent=push), jobs -def _run(observed): - with ( - patch.object(regression, "Push", return_value=observed), - patch.object(regression.time, "sleep"), - ): - return regression.is_new_build_failure("autoland", observed.rev, LABEL) +def _run(chain, *, poll=None): + """Run the gate. `poll` supplies later job snapshots, one per poll attempt.""" + head, jobs = chain + snapshots = [jobs] + list(poll or []) + state = {"attempt": 0} + + def label_jobs(project, rev, label): + snapshot = snapshots[min(state["attempt"], len(snapshots) - 1)] + return [j for j in snapshot.get(rev, []) if label == LABEL] + def bump(_seconds): + state["attempt"] += 1 -def _run_polling(observed_sequence): - """Feed a fresh observed-push chain on each poll, simulating builds settling.""" with ( - patch.object(regression, "Push", side_effect=observed_sequence), - patch.object(regression.time, "sleep"), + patch.object(regression, "Push", return_value=head), + patch.object(regression.treeherder, "label_jobs", label_jobs), + patch.object(regression.time, "sleep", bump), ): - return regression.is_new_build_failure( - "autoland", observed_sequence[0].rev, LABEL - ) + return regression.is_new_build_failure("autoland", "head", LABEL) def test_parent_passed_is_new_failure(): - assert _run(_chain(_failed(), _passed())) is True + assert _run(_chain(passed())) is True def test_parent_failed_is_inherited(): - assert _run(_chain(_failed(), _failed())) is False + assert _run(_chain(failed())) is False + + +def test_parent_busted_is_inherited(): + assert _run(_chain(busted())) is False -def test_parent_intermittent_is_new_failure(): - parent_tasks = [ - FakeTask(result="failed"), - FakeTask(result="passed"), - ] - assert _run(_chain(_failed(), parent_tasks)) is True +def test_parent_with_one_green_retrigger_is_new_failure(): + # Any green run wins, which errs toward running the agent. + assert _run(_chain([_job(result="testfailed"), _job(result="success")])) is True def test_coalesced_parent_then_green_grandparent_is_new_failure(): - assert _run(_chain(_failed(), [], _passed())) is True + assert _run(_chain(never_ran(), passed())) is True def test_coalesced_parent_then_failed_grandparent_is_inherited(): - assert _run(_chain(_failed(), [], _failed())) is False + assert _run(_chain(never_ran(), failed())) is False -def test_infra_parent_is_waited_then_new_failure(): - # An exceptioned parent build is polled, not skipped; once its retry lands - # green the observed push introduced the failure. - assert ( - _run_polling([_chain(_failed(), _infra()), _chain(_failed(), _passed())]) - is True - ) +def test_running_parent_is_waited_then_inherited(): + # A queued or running ancestor is polled until it settles. + assert _run(_chain(running()), poll=[{"rev1": failed()}]) is False -def test_running_parent_is_waited_then_inherited(): - # A still-running parent is polled until it settles; a failure there means - # the observed push inherited it. - assert ( - _run_polling([_chain(_failed(), _running()), _chain(_failed(), _failed())]) - is False - ) +@pytest.mark.parametrize("result", ["retry", "exception", "unknown"]) +def test_unsettled_result_is_waited_not_skipped(result): + # These may still change outcome, so the ancestor must be waited for. Asserting + # the *inherited* verdict is what distinguishes waiting from treating the + # ancestor as non-decisive and walking past it. + chain = _chain([_job(result=result)]) + assert _run(chain, poll=[{"rev1": failed()}]) is False + + +def test_retried_parent_that_turns_green_is_a_new_failure(): + assert _run(_chain(retried()), poll=[{"rev1": passed()}]) is True def test_unsettled_parent_past_deadline_runs_agent(): - # An ancestor build that never settles fails open once the deadline passes. - observed = _chain(_failed(), _running()) + head, jobs = _chain(running()) with ( - patch.object(regression, "Push", return_value=observed), + patch.object(regression, "Push", return_value=head), + patch.object( + regression.treeherder, + "label_jobs", + lambda p, rev, label: jobs.get(rev, []), + ), patch.object(regression.time, "sleep"), patch.object( regression.time, @@ -127,37 +141,55 @@ def test_unsettled_parent_past_deadline_runs_agent(): side_effect=[0.0, regression.MAX_WAIT_SECONDS + 1], ), ): - assert regression.is_new_build_failure("autoland", observed.rev, LABEL) is True + assert regression.is_new_build_failure("autoland", "head", LABEL) is True def test_no_parent_runs_agent(): - assert _run(_chain(_failed())) is True + assert _run(_chain()) is True def test_no_decisive_ancestor_runs_agent(): - empties = [_failed()] + [[] for _ in range(regression.MAX_DEPTH + 2)] - assert _run(_chain(*empties)) is True + assert _run(_chain(*[never_ran() for _ in range(regression.MAX_DEPTH + 2)])) is True -def test_mozci_error_runs_agent(): - with patch.object(regression, "Push", side_effect=RuntimeError("boom")): - assert regression.is_new_build_failure("autoland", "rev", LABEL) is True +def test_other_label_on_parent_is_ignored(): + # Only our own label may decide: another build failing on the parent is not + # our failure being inherited. + head, _ = _chain(failed()) + with ( + patch.object(regression, "Push", return_value=head), + patch.object( + regression.treeherder, + "label_jobs", + lambda p, rev, label: failed() if label == OTHER_LABEL else [], + ), + patch.object(regression.time, "sleep"), + ): + assert regression.is_new_build_failure("autoland", "head", LABEL) is True -def test_other_label_on_parent_ignored(): - parent_tasks = _failed(label="build-macosx64/opt") # different build, not ours - assert _run(_chain(_failed(), parent_tasks)) is True +@pytest.mark.parametrize("boom", ["push", "jobs"]) +def test_lookup_error_runs_agent(boom): + def explode(*args, **kwargs): + raise RuntimeError("upstream down") + head, jobs = _chain(passed()) + with ( + patch.object( + regression, "Push", explode if boom == "push" else lambda *a, **k: head + ), + patch.object(regression.treeherder, "label_jobs", explode), + patch.object(regression.time, "sleep"), + ): + assert regression.is_new_build_failure("autoland", "head", LABEL) is True -def test_exception_parent_is_waited_not_failed(): - # A completed task with an infra `exception` result is unsettled, not a - # failure: we wait for the retry rather than declaring the parent failed. - exception_parent = [FakeTask(state="completed", result="exception")] - settled = _chain(_failed(), _passed()) - assert _run_polling([_chain(_failed(), exception_parent), settled]) is True +def test_pending_notice_is_logged_once_not_every_poll(caplog): + # A pending ancestor is re-polled for up to an hour; the notice must not repeat + # at INFO on every attempt. + chain = _chain(running()) + with caplog.at_level("DEBUG", logger="app.regression"): + assert _run(chain, poll=[{"rev1": running()}, {"rev1": failed()}]) is False -def test_treeherder_result_vocabulary(): - # success/busted are the Treeherder-source spellings of passed/failed. - assert _run(_chain(_failed(), [FakeTask(result="success")])) is True - assert _run(_chain(_failed(), [FakeTask(result="busted")])) is False + notices = [r for r in caplog.records if "not settled" in r.message] + assert [r.levelname for r in notices] == ["INFO", "DEBUG"] diff --git a/services/hackbot-pulse-listener/tests/test_regression_test_failure.py b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py index 28f145d88b..b41d42d5f2 100644 --- a/services/hackbot-pulse-listener/tests/test_regression_test_failure.py +++ b/services/hackbot-pulse-listener/tests/test_regression_test_failure.py @@ -1,6 +1,6 @@ """Tests for the test-failure regression gate (new_test_failures).""" -from types import SimpleNamespace +from unittest.mock import patch import pytest from app import regression @@ -8,187 +8,187 @@ GROUP = "dom/base/test/mochitest.ini" OTHER_GROUP = "layout/test/mochitest.ini" -LABEL = "test-linux1804-64/opt-mochitest-browser-chrome-1" -OTHER_LABEL = "test-windows11-64/debug-mochitest-browser-chrome-1" - - -def _task(label=LABEL, groups=(), state="completed", result="failed"): - """A test task reporting (group, ok) pairs.""" - return SimpleNamespace( - label=label, - state=state, - result=result, - failed=result == "failed", - results=[SimpleNamespace(group=g, ok=ok, duration=1) for g, ok in groups], - ) +CONFIG = ("linux1804-64", "opt") +OTHER_CONFIG = ("windows11-64", "debug") + + +def _job(task_id="T1", result="testfailed", state="completed"): + return {"result": result, "state": state, "task_id": task_id} class FakePush: - def __init__(self, rev, tasks=(), parent=None, scheduled=()): + """A push whose only job is to yield the ancestor chain, as mozci does.""" + + def __init__(self, rev, parent=None): self.rev = rev - self.tasks = list(tasks) - self.scheduled_task_labels = set(scheduled) self._parent = parent @property def parent(self): if self._parent is None: - raise ParentPushNotFound(f"no parent for {self.rev}") + raise ParentPushNotFound("no parent", rev=self.rev, branch="autoland") return self._parent -def _install_head(monkeypatch, head): - monkeypatch.setattr(regression, "Push", lambda rev, branch=None: head) - - -def _check(groups=(GROUP,)): - return regression.new_test_failures("autoland", "headrev", LABEL, list(groups)) - - -def _group_state(monkeypatch, head, group=GROUP): - _install_head(monkeypatch, head) - return regression._classify( - head, - lambda push: regression._group_status(push, group, LABEL), - f"group {group}", +def _chain(depth: int): + push = None + for i in range(depth, 0, -1): + push = FakePush(f"rev{i}", parent=push) + return FakePush("head", parent=push) + + +def _run(groups, jobs, results, *, depth=1, poll=None): + """jobs: {rev: {config: [job]}}; results: {rev: {task_id: {group: passed}}}.""" + snapshots = [(jobs, results)] + list(poll or []) + state = {"attempt": 0} + + def snapshot(): + return snapshots[min(state["attempt"], len(snapshots) - 1)] + + with ( + patch.object(regression, "Push", return_value=_chain(depth)), + patch.object( + regression.treeherder, + "config_jobs", + lambda p, rev, plat, opt: snapshot()[0].get(rev, {}).get((plat, opt), []), + ), + patch.object( + regression.treeherder, + "group_results", + lambda p, rev: snapshot()[1].get(rev, {}), + ), + patch.object( + regression.time, + "sleep", + lambda s: state.update(attempt=state["attempt"] + 1), + ), + ): + return regression.new_test_failures("autoland", "head", CONFIG, list(groups)) + + +def test_new_failure_when_ancestor_passed(): + assert _run( + [GROUP], + {"rev1": {CONFIG: [_job()]}}, + {"rev1": {"T1": {GROUP: True}}}, + ) == {GROUP} + + +def test_inherited_when_ancestor_failed(): + assert ( + _run([GROUP], {"rev1": {CONFIG: [_job()]}}, {"rev1": {"T1": {GROUP: False}}}) + == set() ) -def test_new_failure_when_ancestor_passed(monkeypatch): - parent = FakePush("parentrev", [_task(groups=[(GROUP, True)], result="passed")]) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - assert _check() == {GROUP} - +def test_retriggered_green_ancestor_counts_as_passed(): + jobs = {"rev1": {CONFIG: [_job(task_id="T1"), _job(task_id="T2")]}} + results = {"rev1": {"T1": {GROUP: False}, "T2": {GROUP: True}}} + assert _run([GROUP], jobs, results) == {GROUP} -def test_inherited_when_ancestor_failed(monkeypatch): - parent = FakePush("parentrev", [_task(groups=[(GROUP, False)])]) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - assert _check() == set() - -def test_retriggered_green_ancestor_counts_as_passed(monkeypatch): - # Any green run wins, so the failure here is treated as new (errs toward - # running the agent rather than dropping a regression). - parent = FakePush( - "parentrev", [_task(groups=[(GROUP, False)]), _task(groups=[(GROUP, True)])] - ) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - assert _check() == {GROUP} - - -def test_other_configuration_failure_does_not_mask_new_failure(monkeypatch): - # The manifest is already broken on another platform at the parent push. The - # all-configuration group summary would call that an inherited failure; only +def test_other_configuration_failure_does_not_mask_new_failure(): + # The manifest is already broken on another platform at the parent push; only # this task's own label may decide. - green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) - parent = FakePush( - "parentrev", [_task(label=OTHER_LABEL, groups=[(GROUP, False)])], parent=green - ) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - assert _check() == {GROUP} - - -def test_unfinished_ancestor_task_is_pending(monkeypatch): - # The parent's equivalent task is still running, so it has published no - # results yet. It must be waited for, not skipped as non-decisive. - parent = FakePush("parentrev", [_task(state="running", result=None)]) - head = FakePush("headrev", parent=parent) - assert _group_state(monkeypatch, head) is regression._PENDING - - -def test_unsettled_sibling_defers_a_failed_group(monkeypatch): - # Same precedence as the build path: a retrigger that may still turn green - # outranks an existing failure, so wait instead of calling it inherited. - parent = FakePush( - "parentrev", - [_task(groups=[(GROUP, False)]), _task(state="running", result=None)], - ) - head = FakePush("headrev", parent=parent) - assert _group_state(monkeypatch, head) is regression._PENDING - - -def test_scheduled_but_unreported_ancestor_task_is_pending(monkeypatch): - # The label was scheduled on the parent but no task is visible yet. - parent = FakePush("parentrev", [], scheduled=[LABEL]) - head = FakePush("headrev", parent=parent) - assert _group_state(monkeypatch, head) is regression._PENDING - - -def test_coalesced_ancestor_is_skipped(monkeypatch): - # Nothing ran and nothing was scheduled: non-decisive, keep walking. - green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) - coalesced = FakePush("coalrev", [], parent=green) - _install_head(monkeypatch, FakePush("headrev", parent=coalesced)) - assert _check() == {GROUP} - - -def test_ancestor_ran_label_without_the_group_is_skipped(monkeypatch): - # The manifest was chunked into a different task here: non-decisive. - green = FakePush("greenrev", [_task(groups=[(GROUP, True)], result="passed")]) - other = FakePush("otherrev", [_task(groups=[(OTHER_GROUP, False)])], parent=green) - _install_head(monkeypatch, FakePush("headrev", parent=other)) - assert _check() == {GROUP} - - -def test_groups_are_judged_independently(monkeypatch): - parent = FakePush( - "parentrev", - [_task(groups=[(GROUP, True), (OTHER_GROUP, False)], result="passed")], - ) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - assert _check([GROUP, OTHER_GROUP]) == {GROUP} - - -def test_one_ancestor_walk_serves_every_group(monkeypatch): - # mozci memoizes a push's task list per instance, so all groups of a task must - # share one head push rather than refetching the ancestors for each. - parent = FakePush("parentrev", [_task(groups=[(GROUP, True)], result="passed")]) - head = FakePush("headrev", parent=parent) - builds = [] - - def build(rev, branch=None): - builds.append(rev) - return head - - monkeypatch.setattr(regression, "Push", build) - regression.new_test_failures( - "autoland", "headrev", LABEL, [GROUP, OTHER_GROUP, "c"] - ) - assert builds == ["headrev"] - - -def test_no_ancestor_ran_group_fails_open(monkeypatch): - _install_head(monkeypatch, FakePush("headrev", parent=FakePush("parentrev"))) - assert _check() == {GROUP} - - -def test_mozci_error_fails_open(monkeypatch): - def boom(rev, branch=None): - raise RuntimeError("mozci exploded") - - monkeypatch.setattr(regression, "Push", boom) - assert _check([GROUP, OTHER_GROUP]) == {GROUP, OTHER_GROUP} - - -def test_pending_past_deadline_fails_open(monkeypatch): - parent = FakePush("parentrev", [_task(state="running", result=None)]) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) - monkeypatch.setattr(regression.time, "sleep", lambda s: None) - ticks = iter([0.0, regression.MAX_WAIT_SECONDS + 1]) - monkeypatch.setattr(regression.time, "monotonic", lambda: next(ticks)) - assert _check() == {GROUP} - - -def test_groups_of_one_task_share_one_wait_budget(monkeypatch): + jobs = { + "rev1": {OTHER_CONFIG: [_job(task_id="OTHER")]}, + "rev2": {CONFIG: [_job(task_id="T2")]}, + } + results = { + "rev1": {"OTHER": {GROUP: False}}, + "rev2": {"T2": {GROUP: True}}, + } + assert _run([GROUP], jobs, results, depth=2) == {GROUP} + + +def test_unfinished_ancestor_is_waited_then_inherited(): + jobs = {"rev1": {CONFIG: [_job(result=None, state="running")]}} + poll = [({"rev1": {CONFIG: [_job()]}}, {"rev1": {"T1": {GROUP: False}}})] + assert _run([GROUP], jobs, {}, poll=poll) == set() + + +def test_unsettled_sibling_defers_a_failed_group(): + # Same precedence as the build path: a run that may still turn green outranks + # an existing failure, so wait instead of calling it inherited. + jobs = {"rev1": {CONFIG: [_job(task_id="T1"), _job(task_id="T2", state="running")]}} + results = {"rev1": {"T1": {GROUP: False}}} + poll = [ + ( + {"rev1": {CONFIG: [_job(task_id="T1"), _job(task_id="T2")]}}, + {"rev1": {"T1": {GROUP: False}, "T2": {GROUP: True}}}, + ) + ] + assert _run([GROUP], jobs, results, poll=poll) == {GROUP} + + +def test_ancestor_that_never_ran_the_group_is_skipped(): + # Coalesced, or the manifest was chunked into another task: non-decisive. + jobs = { + "rev1": {CONFIG: [_job(task_id="T1")]}, + "rev2": {CONFIG: [_job(task_id="T2")]}, + } + results = { + "rev1": {"T1": {OTHER_GROUP: False}}, + "rev2": {"T2": {GROUP: True}}, + } + assert _run([GROUP], jobs, results, depth=2) == {GROUP} + + +def test_groups_are_judged_independently(): + jobs = {"rev1": {CONFIG: [_job()]}} + results = {"rev1": {"T1": {GROUP: True, OTHER_GROUP: False}}} + assert _run([GROUP, OTHER_GROUP], jobs, results) == {GROUP} + + +def test_no_ancestor_ran_the_group_fails_open(): + assert _run([GROUP], {}, {}) == {GROUP} + + +def test_lookup_error_fails_open(): + def explode(*args, **kwargs): + raise RuntimeError("treeherder down") + + with ( + patch.object(regression, "Push", return_value=_chain(1)), + patch.object(regression.treeherder, "config_jobs", explode), + patch.object(regression.time, "sleep"), + ): + assert regression.new_test_failures( + "autoland", "head", CONFIG, [GROUP, OTHER_GROUP] + ) == {GROUP, OTHER_GROUP} + + +def test_groups_of_one_task_share_one_wait_budget(): # The budget is per call, not per group: two pending groups must not each get # their own MAX_WAIT_SECONDS. - parent = FakePush("parentrev", [_task(state="running", result=None)]) - _install_head(monkeypatch, FakePush("headrev", parent=parent)) + jobs = {"rev1": {CONFIG: [_job(result=None, state="running")]}} def no_sleep(_seconds): pytest.fail("waited past a spent deadline") - monkeypatch.setattr(regression.time, "sleep", no_sleep) - ticks = iter([0.0, regression.MAX_WAIT_SECONDS + 1]) - monkeypatch.setattr(regression.time, "monotonic", lambda: next(ticks)) - assert _check([GROUP, OTHER_GROUP]) == {GROUP, OTHER_GROUP} + with ( + patch.object(regression, "Push", return_value=_chain(1)), + patch.object( + regression.treeherder, + "config_jobs", + lambda p, rev, plat, opt: jobs.get(rev, {}).get((plat, opt), []), + ), + patch.object(regression.treeherder, "group_results", lambda p, rev: {}), + patch.object(regression.time, "sleep", no_sleep), + patch.object( + regression.time, + "monotonic", + side_effect=[0.0, regression.MAX_WAIT_SECONDS + 1], + ), + ): + assert regression.new_test_failures( + "autoland", "head", CONFIG, [GROUP, OTHER_GROUP] + ) == {GROUP, OTHER_GROUP} + + +def test_missing_configuration_reports_every_group_as_new(): + # Without a configuration there is nothing to compare against, so nothing may + # be silently dropped. + assert regression.new_test_failures("autoland", "head", (None, None), [GROUP]) == { + GROUP + } diff --git a/services/hackbot-pulse-listener/tests/test_treeherder.py b/services/hackbot-pulse-listener/tests/test_treeherder.py new file mode 100644 index 0000000000..66bb545bff --- /dev/null +++ b/services/hackbot-pulse-listener/tests/test_treeherder.py @@ -0,0 +1,344 @@ +"""Tests for the Treeherder classification gate.""" + +import httpx +import pytest +from app import treeherder + + +def _response(payload): + request = httpx.Request("GET", "https://treeherder.example/api") + return httpx.Response(200, json=payload, request=request) + + +def _job(classification): + return {"failure_classification_id": classification, "task_id": "TT"} + + +@pytest.fixture +def api(monkeypatch): + """Stub the job lookup; call .queue with successive return values.""" + calls = [] + + def set_results(*results): + it = iter(results) + + def fake(project, task_id): + calls.append((project, task_id)) + value = next(it) + if isinstance(value, Exception): + raise value + return value + + monkeypatch.setattr(treeherder, "_job", fake) + + monkeypatch.setattr(treeherder.time, "sleep", lambda s: None) + return type("Api", (), {"queue": staticmethod(set_results), "calls": calls}) + + +@pytest.mark.parametrize( + ("classification", "reason"), + [ + (2, "fixed by commit"), + (3, "expected fail"), + (4, "intermittent"), + (5, "infra"), + (7, "autoclassified intermittent"), + (8, "intermittent needs bugid"), + ], +) +def test_already_judged_failures_are_skipped(classification, reason): + assert treeherder.skip_reason(_job(classification)) == reason + + +@pytest.mark.parametrize("classification", [1, 6]) +def test_undecided_failures_are_investigated(classification): + # "not classified" and "new failure not classified" may still be real. + assert treeherder.skip_reason(_job(classification)) is None + + +def test_unknown_classification_is_investigated(): + # A classification we do not know about must not silently drop a regression. + assert treeherder.skip_reason(_job(99)) is None + + +def test_missing_job_is_investigated(): + assert treeherder.skip_reason(None) is None + + +def test_waits_for_ingestion_then_returns_the_job(api): + # Treeherder ingests a minute or so after the failure message arrives. + api.queue(None, None, _job(4)) + assert treeherder.job_for_task("autoland", "TT") == _job(4) + assert len(api.calls) == 3 + + +def test_never_ingested_fails_open(api, monkeypatch): + ticks = iter([0.0, treeherder.settings.treeherder_ingest_max_wait_seconds + 1]) + monkeypatch.setattr(treeherder.time, "monotonic", lambda: next(ticks)) + api.queue(None, None) + assert treeherder.job_for_task("autoland", "TT") is None + + +def test_lookup_error_fails_open(api): + api.queue(httpx.ConnectError("treeherder down")) + assert treeherder.job_for_task("autoland", "TT") is None + + +def test_lookup_uses_the_task_id_not_the_revision(monkeypatch): + # jobs/?revision= silently ignores the filter and returns unrelated jobs. + seen = {} + + def fake_get(url, **kwargs): + seen["url"] = url + return _response({"results": [_job(4)]}) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + assert treeherder._job("autoland", "TASK123")["failure_classification_id"] == 4 + assert "task_id=TASK123" in seen["url"] + assert "revision=" not in seen["url"] + assert "/project/autoland/jobs/" in seen["url"] + + +def test_not_ingested_returns_none(monkeypatch): + monkeypatch.setattr( + treeherder.httpx, "get", lambda url, **k: _response({"results": []}) + ) + assert treeherder._job("autoland", "TT") is None + + +@pytest.fixture(autouse=True) +def _clear_group_cache(): + for cache in ( + treeherder._group_cache, + treeherder._jobs_cache, + treeherder._push_id_cache, + ): + cache.clear() + yield + for cache in ( + treeherder._group_cache, + treeherder._jobs_cache, + treeherder._push_id_cache, + ): + cache.clear() + + +def test_group_results_are_fetched_once_per_push(monkeypatch): + # A push emits many failing tasks; they must share one fetch. + calls = [] + + def fake_get(url, **kwargs): + calls.append(url) + return _response({"TASK1": {"a.ini": False}, "TASK2": {"b.ini": True}}) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + first = treeherder.group_results("autoland", "abc" * 13 + "d") + second = treeherder.group_results("autoland", "abc" * 13 + "d") + assert first == second + assert len(calls) == 1 + assert "group_results/?revision=" in calls[0] + + +def test_group_results_refresh_bypasses_the_cache(monkeypatch): + calls = [] + + def fake_get(url, **kwargs): + calls.append(url) + return _response({"TASK1": {"a.ini": False}}) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + rev = "abc" * 13 + "d" + treeherder.group_results("autoland", rev) + treeherder.group_results("autoland", rev, refresh=True) + assert len(calls) == 2 + + +def test_group_results_cached_per_project_and_revision(monkeypatch): + calls = [] + monkeypatch.setattr( + treeherder.httpx, + "get", + lambda url, **k: (calls.append(url), _response({}))[1], + ) + treeherder.group_results("autoland", "a" * 40) + treeherder.group_results("autoland", "b" * 40) + treeherder.group_results("mozilla-central", "a" * 40) + assert len(calls) == 3 + + +def test_label_jobs_filters_by_push_and_label(monkeypatch): + urls = [] + + def fake_get(url, **kwargs): + urls.append(url) + if "/push/?revision=" in url: + return _response({"results": [{"id": 4242}]}) + return _response( + {"results": [{"result": "success", "state": "completed", "task_id": "T1"}]} + ) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + jobs = treeherder.label_jobs("autoland", "a" * 40, "test-linux/opt-mochitest-1") + assert [(j["result"], j["state"], j["task_id"]) for j in jobs] == [ + ("success", "completed", "T1") + ] + assert any("push_id=4242" in u for u in urls) + # The label is URL-encoded: it contains a slash. + assert any("job_type_name=test-linux%2Fopt-mochitest-1" in u for u in urls) + + +def test_label_jobs_without_a_push_is_empty(monkeypatch): + monkeypatch.setattr( + treeherder.httpx, "get", lambda url, **k: _response({"results": []}) + ) + assert treeherder.label_jobs("autoland", "a" * 40, "l") == [] + + +def test_label_jobs_cached_per_push_and_label(monkeypatch): + calls = [] + + def fake_get(url, **kwargs): + calls.append(url) + if "/push/?revision=" in url: + return _response({"results": [{"id": 1}]}) + return _response({"results": []}) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + rev = "a" * 40 + treeherder.label_jobs("autoland", rev, "one") + treeherder.label_jobs("autoland", rev, "one") + treeherder.label_jobs("autoland", rev, "two") + # push id resolved once, jobs fetched once per distinct label. + assert sum("/push/?revision=" in u for u in calls) == 1 + assert sum("job_type_name=" in u for u in calls) == 2 + + +def test_absent_task_raises_rather_than_reporting_nothing(monkeypatch): + # A task-level failure (crash, timeout) records no per-manifest results, and so + # does a log still being parsed. Neither means "nothing failed". + monkeypatch.setattr( + treeherder, "group_results", lambda p, rev, refresh=False: {"OTHER": {}} + ) + with pytest.raises(treeherder.GroupResultsUnavailable): + treeherder.failing_groups("autoland", "a" * 40, "T1") + + +def test_task_present_with_no_failures_reports_nothing(monkeypatch): + # Distinct from the case above: Treeherder has results, none of them failing. + monkeypatch.setattr( + treeherder, + "group_results", + lambda p, rev, refresh=False: {"T1": {"a.ini": True, "b.ini": True}}, + ) + assert treeherder.failing_groups("autoland", "a" * 40, "T1") == [] + + +def test_recheck_reads_the_current_classification(monkeypatch): + monkeypatch.setattr(treeherder, "_job", lambda p, t: _job(4)) + assert treeherder.recheck_skip_reason("autoland", "T1") == "intermittent" + + +def test_recheck_does_not_wait_for_ingestion(monkeypatch): + monkeypatch.setattr(treeherder, "_job", lambda p, t: None) + monkeypatch.setattr( + treeherder.time, "sleep", lambda s: pytest.fail("re-check must not wait") + ) + assert treeherder.recheck_skip_reason("autoland", "T1") is None + + +def test_recheck_error_does_not_drop_the_failure(monkeypatch): + def boom(project, task_id): + raise RuntimeError("treeherder down") + + monkeypatch.setattr(treeherder, "_job", boom) + assert treeherder.recheck_skip_reason("autoland", "T1") is None + + +def test_failing_groups_returns_only_failures(monkeypatch): + monkeypatch.setattr( + treeherder, + "group_results", + lambda p, rev, refresh=False: { + "T1": {"a.ini": False, "b.ini": True, "/": False} + }, + ) + assert treeherder.failing_groups("autoland", "a" * 40, "T1") == ["a.ini"] + + +def test_failing_groups_refetches_when_task_absent(monkeypatch): + calls = [] + + def fake(project, rev, refresh=False): + calls.append(refresh) + return {"OTHER": {"a.ini": False}} if refresh else {} + + monkeypatch.setattr(treeherder, "group_results", fake) + with pytest.raises(treeherder.GroupResultsUnavailable): + treeherder.failing_groups("autoland", "a" * 40, "T1") + assert calls == [False, True] + + +def test_config_jobs_filters_platform_option_client_side(monkeypatch): + # Treeherder accepts platform_option but silently ignores it, so the query can + # only narrow by platform and the option must be filtered here. + urls = [] + + def fake_get(url, **kwargs): + urls.append(url) + if "/push/?revision=" in url: + return _response({"results": [{"id": 7}]}) + return _response( + { + "results": [ + { + "result": "success", + "state": "completed", + "task_id": "A", + "platform_option": "debug", + }, + { + "result": "testfailed", + "state": "completed", + "task_id": "B", + "platform_option": "opt", + }, + ] + } + ) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + jobs = treeherder.config_jobs("autoland", "a" * 40, "linux2404-64", "debug") + assert [j["task_id"] for j in jobs] == ["A"] + assert any("platform=linux2404-64" in u for u in urls) + assert not any("platform_option=" in u for u in urls) + + +def test_config_jobs_shares_one_fetch_across_options(monkeypatch): + calls = [] + + def fake_get(url, **kwargs): + calls.append(url) + if "/push/?revision=" in url: + return _response({"results": [{"id": 7}]}) + return _response({"results": []}) + + monkeypatch.setattr(treeherder.httpx, "get", fake_get) + rev = "a" * 40 + treeherder.config_jobs("autoland", rev, "linux2404-64", "debug") + treeherder.config_jobs("autoland", rev, "linux2404-64", "opt") + assert sum("platform=" in u for u in calls) == 1 + + +def test_await_skip_reason_returns_a_late_verdict(monkeypatch): + verdicts = iter([None, None, _job(4)]) + monkeypatch.setattr(treeherder, "_job", lambda p, t: next(verdicts)) + monkeypatch.setattr(treeherder.time, "sleep", lambda s: None) + assert treeherder.await_skip_reason("autoland", "T1") == "intermittent" + + +def test_await_skip_reason_gives_up_and_investigates(monkeypatch): + monkeypatch.setattr(treeherder, "_job", lambda p, t: _job(6)) + monkeypatch.setattr(treeherder.time, "sleep", lambda s: None) + ticks = iter([0.0, treeherder.settings.treeherder_classification_wait_seconds + 1]) + monkeypatch.setattr(treeherder.time, "monotonic", lambda: next(ticks)) + assert treeherder.await_skip_reason("autoland", "T1") is None From e4e47bbfc2ed7bf5b4954ae4f19bd10af50e1c68 Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Wed, 29 Jul 2026 17:18:07 -0700 Subject: [PATCH 7/7] Dedupe per push --- services/hackbot-pulse-listener/README.md | 24 +-- services/hackbot-pulse-listener/app/config.py | 8 +- .../hackbot-pulse-listener/app/consumer.py | 138 ++++++++---------- .../hackbot-pulse-listener/app/regression.py | 7 +- .../hackbot-pulse-listener/app/treeherder.py | 28 ++-- .../tests/test_consumer.py | 105 ++++++++++--- .../tests/test_treeherder.py | 27 +++- 7 files changed, 207 insertions(+), 130 deletions(-) diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index bca8c74a04..1b5b039e75 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -21,20 +21,24 @@ Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-r action-callback task for a backfill or retrigger. - Pushes that landed more than `MAX_PUSH_AGE_HOURS` ago (default 24). A failure can surface long after its push, and by then the push has been superseded. -4. **Dedupe** with in-memory TTL caches, both keyed by revision: build-repair once per - revision; test-repair once per `(revision, test group)`, so a manifest failing across - chunks is investigated once. Test groups are claimed before the checks below, so - sibling chunks never repeat them. +4. **Dedupe** with in-memory TTL caches, both keyed by revision: one run per push per + agent, triggered on the first failing task worth investigating. The test-repair + agent works from a single task but reads the push's other failures itself, so a + second run for the same push would re-tread the same ground. A revision is recorded + only once a run is actually triggered, so a task rejected as intermittent or + inherited leaves the push open for the next one. 5. **Judge** whether the failure is worth a run. Every check fails open — an upstream error runs the agent rather than dropping a possible regression. - _Build:_ keep only failures this push introduced, waiting for an unsettled ancestor build to finish first. - - _Test:_ first drop whatever Treeherder has already judged not to be a new regression - (intermittent, infra, expected-fail, fixed-by-commit); Treeherder ingests a minute or - so behind us, so the gate waits for the job to appear. Then keep only groups that are - new for this task's own configuration (platform and build option), and ask Treeherder - once more before triggering, since a verdict often lands while that check runs. One - run per task, carrying only the task id. + - _Test:_ first drop whatever Treeherder judges not to be a new regression + (intermittent, infra, expected-fail, fixed-by-commit). Treeherder ingests a minute or + so behind us and classifies a few minutes after that, so this gate waits for the job + to appear and then for its verdict — it is the cheap filter, and most test failures + stop here, before any group is fetched or any ancestor walked. What survives is + narrowed to the groups that are new for this task's own configuration (platform and + build option), then Treeherder is asked once more, since a verdict can still land + while that walk runs. The run carries only the task id. 6. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until terminal, then email a hackbot UI link, the analysis summary, a Treeherder link, and the commit the agent blamed. Build-repair looks the blamed commit up in the firefox GitHub diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index e970b3bdaf..6e17660945 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -27,7 +27,7 @@ class Settings(BaseSettings): # Pushes older than this are not repaired. A build failure can arrive long # after its push (a backfill, a long-queued task, a replayed message), and by # then the push has been superseded. Generous next to the minutes a build - # needs plus the hour the regression check may wait for an ancestor. + # needs plus the ten the regression check may wait for an ancestor. max_push_age_hours: float = 24 run_try_push: bool = False model: str | None = None @@ -36,9 +36,9 @@ class Settings(BaseSettings): # gate waits for the job to be ingested before reading that verdict. treeherder_ingest_poll_seconds: int = 30 treeherder_ingest_max_wait_seconds: int = 240 - # How long to wait for a verdict on a failure whose manifests are unknown, where - # no ancestor comparison is possible and most failures turn out to be classified - # intermittent or expected-fail shortly after we see them. + # How long to wait for a verdict once the job is ingested. Classification lands a + # few minutes after ingestion, and most test failures turn out to be intermittent + # or expected-fail, so waiting here rejects them before the ancestor walk. treeherder_classification_wait_seconds: int = 300 # Dedupe (in-memory, by hg revision) diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index 687770e17e..40c83aced0 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -30,11 +30,12 @@ ) _seen_lock = threading.Lock() -# Independent dedupe for test-repair runs, keyed by (hg revision, test group): one -# push emits many failing test tasks, and we want one run per failing group. Keyed on -# the revision rather than the Taskcluster task group because backfills and retriggers -# are dispatched by action tasks, which start task groups of their own -- the same -# push would otherwise be investigated once per task group. +# Independent dedupe for test-repair runs, keyed by hg revision: one run per push, +# on the first of its failing tasks worth investigating. The agent works from a single +# task but reads the push's other failures from Treeherder, so a second run for the +# same push would re-tread the same ground. As on the build path a revision is +# recorded only once a run is actually triggered, so a task rejected as intermittent +# or inherited never suppresses a genuine regression on another task of the push. _seen_tests: TTLCache = TTLCache( maxsize=settings.dedupe_max_size, ttl=settings.dedupe_ttl_seconds ) @@ -165,11 +166,10 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: """Test-failure path: filter, then trigger the test-repair agent for the task. - One push emits many failing test tasks; each task may fail several groups. We - resolve the failing groups and keep the ones that are genuine, non-intermittent - regressions, then trigger a single run for the task (the agent resolves the - commit range itself from the task id). Dedupe is per (push, group) so a manifest - failing across chunks is investigated once. + One push emits many failing test tasks. We wait for Treeherder's verdict on this + one, then keep only the failing groups this push introduced, and trigger a single + run for the task -- the agent resolves the failing tests and the commit range + itself from the task id. Dedupe is per push, on the first task worth investigating. """ status = body.get("status") or {} task_id = status.get("taskId") @@ -198,57 +198,59 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: ): return None - # Cheapest gate first: one small request, and it rules out intermittents and - # infra failures for every harness before any ancestor walking. The same record - # carries the configuration the regression check compares against. + # One run per push: a task whose push has already been handed off can stop before + # any Treeherder work. + if _push_claimed(hg_revision): + logger.info( + "Push %s already handed to test-repair; skipping task %s", + hg_revision, + task_id, + ) + return None + + # Cheapest gate first: it rules out intermittents and infra failures for every + # harness before any group resolution or ancestor walking. Treeherder classifies + # a few minutes after ingesting, so the verdict is waited for rather than read + # once. The same record carries the configuration the regression check compares + # against. job = treeherder.job_for_task(project, task_id) - reason = treeherder.skip_reason(job) + reason = treeherder.await_skip_reason(project, task_id, job) if reason: logger.info("Treeherder classified task %s as %s; skipping", task_id, reason) return None config = ((job or {}).get("platform"), (job or {}).get("platform_option")) - whole_task = (project, hg_revision, task_id, label, developer_email) + trigger = (project, hg_revision, task_id, label, developer_email, executor) try: groups = treeherder.failing_groups(project, hg_revision, task_id) except treeherder.GroupResultsUnavailable as exc: # Routine: a task-level failure (crash, timeout, harness error) produces no # per-manifest results, and a log may still be being parsed. Either way we - # cannot say what failed, so fail open rather than drop it. + # cannot say what failed, so fail open rather than drop it. There is no + # manifest to compare against an ancestor, so the gate above is the only + # filter such a failure gets. logger.info("%s; running the agent on the whole task", exc) - return _trigger_whole_task(*whole_task, executor) + return _trigger_test_repair([], *trigger) except Exception: logger.exception( "Could not read the failing groups of task %s; " "running the agent on the whole task", task_id, ) - return _trigger_whole_task(*whole_task, executor) + return _trigger_test_repair([], *trigger) if not groups: logger.info("Task %s reported no failing test groups; skipping", task_id) return None - claimed = _claim_groups([(hg_revision, group) for group in groups]) - if not claimed: - logger.info( - "Every failing group of task %s is already handled; skipping", task_id - ) - return None - - fresh = _fresh_groups( - [group for group in groups if (hg_revision, group) in claimed], - project, - hg_revision, - config, - ) + fresh = _fresh_groups(groups, project, hg_revision, config) if not fresh: logger.info("No new, non-intermittent groups for task %s; skipping", task_id) return None - # Ask again before spending a run: the check above takes minutes, which is about - # how long Treeherder takes to classify an intermittent, so a verdict that was - # not available at the start of it often is by now. + # One last cheap look before spending a run. The gate above already waited for a + # verdict, so this catches one that landed during the walk: a sheriff's + # classification, or autoclassification once a retrigger came back green. reason = treeherder.recheck_skip_reason(project, task_id) if reason: logger.info( @@ -258,55 +260,28 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: ) return None - return _trigger_test_repair( - fresh, claimed, project, hg_revision, task_id, label, developer_email, executor - ) + return _trigger_test_repair(fresh, *trigger) -def _trigger_whole_task( - project: str, - hg_revision: str, - task_id: str, - label: str, - developer_email: str | None, - executor: Executor, -) -> str | None: - """Investigate a task whose failing manifests could not be determined. - - There is no manifest to compare against an ancestor here, so nothing else delays - the decision. In practice these are mostly intermittents and expected failures - that Treeherder classifies a few minutes later, so wait for a verdict before - spending a run. - """ - claimed = _claim_groups([(hg_revision, label)]) - if not claimed: - logger.info("Task %s is already handled; skipping", task_id) - return None - - reason = treeherder.await_skip_reason(project, task_id) - if reason: - logger.info("Treeherder classified task %s as %s; skipping", task_id, reason) - return None - - return _trigger_test_repair( - [], claimed, project, hg_revision, task_id, label, developer_email, executor - ) +def _push_claimed(hg_revision: str) -> bool: + """Whether a run has already been triggered for this push.""" + with _seen_tests_lock: + return hg_revision in _seen_tests -def _claim_groups(keys: list[tuple[str, str]]) -> set[tuple[str, str]]: - """Claim the keys not yet handed off, returning the ones this call claimed. +def _claim_push(hg_revision: str) -> bool: + """Claim the push, or False if another task claimed it first. - Claimed *before* the regression check, not after: every failing chunk - of a push resolves the same groups, so without an up-front claim they all run - the expensive walk concurrently and then throw the answer away. A key stays - claimed even when the filter then rejects the group, so the sibling tasks do - not recompute a verdict that would come out the same. + Deliberately taken at the moment of triggering rather than before the checks: + sibling tasks may then repeat those checks, which is affordable because the + Treeherder results they read are cached per push, and it keeps a task that turns + out to be intermittent or inherited from claiming a push it will not investigate. """ with _seen_tests_lock: - claimed = {k for k in keys if k not in _seen_tests} - for k in claimed: - _seen_tests[k] = True - return claimed + if hg_revision in _seen_tests: + return False + _seen_tests[hg_revision] = True + return True def _fresh_groups( @@ -326,7 +301,6 @@ def _fresh_groups( def _trigger_test_repair( test_groups: list[str], - claimed, project: str, hg_revision: str, task_id: str, @@ -334,6 +308,14 @@ def _trigger_test_repair( developer_email: str | None, executor: Executor, ) -> str | None: + if not _claim_push(hg_revision): + logger.info( + "Push %s was claimed while task %s was being checked; skipping", + hg_revision, + task_id, + ) + return None + try: run_id = client.trigger_run( {"failure_tasks": {label: task_id}}, @@ -341,7 +323,7 @@ def _trigger_test_repair( ) except Exception: logger.exception("Failed to trigger test-repair run for task %s", task_id) - _release(_seen_tests, _seen_tests_lock, claimed) + _release(_seen_tests, _seen_tests_lock, [hg_revision]) return None logger.info( diff --git a/services/hackbot-pulse-listener/app/regression.py b/services/hackbot-pulse-listener/app/regression.py index 149aade5db..6eadcfa92a 100644 --- a/services/hackbot-pulse-listener/app/regression.py +++ b/services/hackbot-pulse-listener/app/regression.py @@ -20,9 +20,12 @@ logger = logging.getLogger(__name__) -# Poll an unsettled ancestor for up to MAX_WAIT_SECONDS before giving up. +# Poll an unsettled ancestor for up to MAX_WAIT_SECONDS before giving up and running +# the agent. An ancestor that has not settled within ten minutes is usually not about +# to: the walk either resolves in a few polls or the run it waits on is itself stuck, +# and holding the decision longer only delays the repair. POLL_INTERVAL_SECONDS = 120 -MAX_WAIT_SECONDS = 60 * 60 +MAX_WAIT_SECONDS = 60 * 10 # Treeherder job results and states. _PASSED_RESULTS = ("success",) diff --git a/services/hackbot-pulse-listener/app/treeherder.py b/services/hackbot-pulse-listener/app/treeherder.py index 2546f11d31..dd74c83d51 100644 --- a/services/hackbot-pulse-listener/app/treeherder.py +++ b/services/hackbot-pulse-listener/app/treeherder.py @@ -237,23 +237,31 @@ def job_for_task(project: str, task_id: str) -> dict | None: time.sleep(settings.treeherder_ingest_poll_seconds) -def await_skip_reason(project: str, task_id: str) -> str | None: +def await_skip_reason(project: str, task_id: str, job: dict | None) -> str | None: """Wait a bounded time for a verdict that this failure is not worth a run. - For a task with no per-manifest results there is nothing to compare against an - ancestor, so nothing else delays the decision -- and in practice these are mostly - intermittents and expected failures that Treeherder classifies a few minutes - later. Returns None once the wait is spent, so an unclassified failure is still - investigated rather than dropped. + ``job`` carries the verdict as of ingestion, which on an intermittent is usually + still "not classified": Treeherder classifies a few minutes later. Waiting for it + here rejects such a failure before the caller's ancestor walk rather than after, + so the filter no longer depends on how long that walk happens to take. + + Returns None once the wait is spent, so an unclassified failure is investigated + rather than dropped, and at once for a task Treeherder holds no job for, since + then there is no verdict to wait for. """ + if job is None: + return None + reason = skip_reason(job) + if reason: + return reason + deadline = time.monotonic() + settings.treeherder_classification_wait_seconds - while True: + while time.monotonic() < deadline: + time.sleep(settings.treeherder_ingest_poll_seconds) reason = recheck_skip_reason(project, task_id) if reason: return reason - if time.monotonic() >= deadline: - return None - time.sleep(settings.treeherder_ingest_poll_seconds) + return None def recheck_skip_reason(project: str, task_id: str) -> str | None: diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index 611870b1c8..87b4387bbd 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -91,7 +91,7 @@ def test_sample_messages_route_to_test_repair_not_build(): patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.treeherder, "failing_groups", return_value=[]) as groups, patch.object(consumer.treeherder, "job_for_task", return_value=None), - patch.object(consumer.treeherder, "skip_reason", return_value=None), + patch.object(consumer.treeherder, "await_skip_reason", return_value=None), patch.object(consumer.client, "trigger_run") as trigger, ): for body in _sample_bodies(): @@ -337,7 +337,6 @@ def env(monkeypatch): "platform_option": "opt", } ), - skip_reason=MagicMock(return_value=None), recheck_skip_reason=MagicMock(return_value=None), await_skip_reason=MagicMock(return_value=None), new_test_failures=MagicMock( @@ -350,7 +349,6 @@ def env(monkeypatch): monkeypatch.setattr(consumer.taskcluster, "get_task", mocks.get_task) monkeypatch.setattr(consumer.treeherder, "failing_groups", mocks.failing_groups) monkeypatch.setattr(consumer.treeherder, "job_for_task", mocks.job_for_task) - monkeypatch.setattr(consumer.treeherder, "skip_reason", mocks.skip_reason) monkeypatch.setattr( consumer.treeherder, "recheck_skip_reason", mocks.recheck_skip_reason ) @@ -387,7 +385,7 @@ def test_test_failure_triggers_rca_run(env): def test_treeherder_intermittent_skipped_before_any_walk(env): # Treeherder's own verdict rules the failure out before any mozci work. - env.skip_reason.return_value = "intermittent" + env.await_skip_reason.return_value = "intermittent" assert consumer.process(_test_msg(), env.executor) is None env.failing_groups.assert_not_called() env.new_test_failures.assert_not_called() @@ -396,7 +394,7 @@ def test_treeherder_intermittent_skipped_before_any_walk(env): def test_unclassified_failure_is_investigated(env): # "not classified" / "new failure" leave the decision to the mozci walk. - env.skip_reason.return_value = None + env.await_skip_reason.return_value = None assert consumer.process(_test_msg(), env.executor) == "tr-1" env.new_test_failures.assert_called_once() @@ -407,12 +405,23 @@ def test_inherited_test_group_skipped(env): env.trigger_run.assert_not_called() -def test_same_group_same_push_triggers_once(env): - consumer.process(_test_msg(task_id="A"), env.executor) - consumer.process(_test_msg(task_id="B"), env.executor) +def test_one_run_per_push(env): + # The agent reads the push's other failures itself, so the first task worth + # investigating is enough; later failing tasks of the same push are skipped. + assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B"), env.executor) is None env.trigger_run.assert_called_once() +def test_later_task_of_a_claimed_push_stops_before_treeherder(env): + assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + env.job_for_task.reset_mock() + env.failing_groups.reset_mock() + assert consumer.process(_test_msg(task_id="B"), env.executor) is None + env.job_for_task.assert_not_called() + env.failing_groups.assert_not_called() + + def test_no_failing_groups_skips(env): env.failing_groups.return_value = [] assert consumer.process(_test_msg(), env.executor) is None @@ -433,22 +442,30 @@ def test_task_without_group_results_still_triggers_run(env): assert ctx.test_groups == [] -def test_unreadable_group_results_triggers_once_per_task(env): +def test_unreadable_group_results_triggers_once_per_push(env): env.failing_groups.side_effect = RuntimeError("treeherder down") consumer.process(_test_msg(task_id="A"), env.executor) consumer.process(_test_msg(task_id="B"), env.executor) env.trigger_run.assert_called_once() -def test_rejected_group_is_not_re_evaluated_by_sibling_chunks(env): - # The dedupe claim is taken before the expensive checks, so a sibling chunk - # reporting the same group does not repeat the mozci walk to reach the same - # verdict. - env.new_test_failures.side_effect = lambda *_: set() +def test_rejected_task_does_not_suppress_a_real_regression_on_the_push(env): + # The push is claimed only when a run is triggered, so a task rejected as + # inherited leaves the push open for the next failing task -- which may be the + # genuine regression. + env.new_test_failures.side_effect = [set(), {_GROUP}] assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) is None - env.trigger_run.assert_not_called() - assert env.new_test_failures.call_count == 1 + assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + env.trigger_run.assert_called_once() + + +def test_intermittent_task_does_not_suppress_the_next_task(env): + # Same for a task Treeherder has already classified: it must not claim a push + # it will not investigate. + env.await_skip_reason.side_effect = ["intermittent", None] + assert consumer.process(_test_msg(task_id="A"), env.executor) is None + assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + env.trigger_run.assert_called_once() def test_missing_git_mapping_still_triggers_run(env): @@ -562,19 +579,38 @@ def test_different_pushes_are_not_deduped(env): assert env.trigger_run.call_count == 2 -def test_whole_task_waits_for_a_verdict_before_triggering(env): +def test_verdict_is_awaited_before_resolving_groups(env): + # The wait for a verdict is the cheap gate, so it must come first: an intermittent + # should cost neither a group fetch nor an ancestor walk. + order = [] + env.await_skip_reason.side_effect = lambda p, t, j: order.append("await") + env.failing_groups.side_effect = lambda *_: (order.append("groups"), [_GROUP])[1] + + assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert order == ["await", "groups"] + + +def test_the_verdict_is_awaited_once_on_every_path(env): + # A task with no group results used to run its own second wait; the up-front one + # covers it, and waiting twice would double the delay before a real repair. env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") + assert consumer.process(_test_msg(), env.executor) == "tr-1" + env.await_skip_reason.assert_called_once() + + +def test_group_less_intermittent_is_dropped_by_the_up_front_gate(env): + # The only filter such a failure gets, since it has no manifest to compare. env.await_skip_reason.return_value = "intermittent" + env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") assert consumer.process(_test_msg(), env.executor) is None - env.await_skip_reason.assert_called_once() env.trigger_run.assert_not_called() -def test_whole_task_triggers_when_no_verdict_arrives(env): - env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") - env.await_skip_reason.return_value = None +def test_the_job_is_passed_to_the_verdict_wait(env): + # The wait needs the ingested job: it is the verdict as of ingestion, and without + # it the wait cannot tell "not classified yet" from "never ingested". assert consumer.process(_test_msg(), env.executor) == "tr-1" - env.trigger_run.assert_called_once() + assert env.await_skip_reason.call_args.args[2] is env.job_for_task.return_value def test_backfilled_test_task_is_skipped(env): @@ -596,3 +632,26 @@ def test_stale_push_skips_a_test_failure(env, fresh_push): ) env.new_test_failures.assert_not_called() env.trigger_run.assert_not_called() + + +def test_group_less_task_claims_the_push_for_every_path(env): + # One run per push whichever path triggered it: a task-level failure (no group + # results) and a manifest failure on the same push must not both run. + env.failing_groups.side_effect = [ + consumer.treeherder.GroupResultsUnavailable("none"), + [_GROUP], + ] + assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B"), env.executor) is None + env.trigger_run.assert_called_once() + + +def test_manifest_failure_claims_the_push_against_a_group_less_task(env): + # The reverse order must dedupe too. + env.failing_groups.side_effect = [ + [_GROUP], + consumer.treeherder.GroupResultsUnavailable("none"), + ] + assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B"), env.executor) is None + env.trigger_run.assert_called_once() diff --git a/services/hackbot-pulse-listener/tests/test_treeherder.py b/services/hackbot-pulse-listener/tests/test_treeherder.py index 66bb545bff..46d59eec2a 100644 --- a/services/hackbot-pulse-listener/tests/test_treeherder.py +++ b/services/hackbot-pulse-listener/tests/test_treeherder.py @@ -330,10 +330,31 @@ def fake_get(url, **kwargs): def test_await_skip_reason_returns_a_late_verdict(monkeypatch): - verdicts = iter([None, None, _job(4)]) + verdicts = iter([_job(1), _job(4)]) monkeypatch.setattr(treeherder, "_job", lambda p, t: next(verdicts)) monkeypatch.setattr(treeherder.time, "sleep", lambda s: None) - assert treeherder.await_skip_reason("autoland", "T1") == "intermittent" + assert treeherder.await_skip_reason("autoland", "T1", _job(1)) == "intermittent" + + +def test_await_skip_reason_uses_the_verdict_it_was_given(monkeypatch): + # Already classified at ingestion: no further request, and no waiting. + def fail(*_): + raise AssertionError("should not re-fetch an already classified job") + + monkeypatch.setattr(treeherder, "_job", fail) + monkeypatch.setattr(treeherder.time, "sleep", fail) + assert treeherder.await_skip_reason("autoland", "T1", _job(4)) == "intermittent" + + +def test_await_skip_reason_does_not_wait_without_a_job(monkeypatch): + # Treeherder never ingested the task, so there is no verdict coming; waiting the + # full window for one would stall the failure for nothing. + def fail(*_): + raise AssertionError("should not poll for a job Treeherder does not have") + + monkeypatch.setattr(treeherder, "_job", fail) + monkeypatch.setattr(treeherder.time, "sleep", fail) + assert treeherder.await_skip_reason("autoland", "T1", None) is None def test_await_skip_reason_gives_up_and_investigates(monkeypatch): @@ -341,4 +362,4 @@ def test_await_skip_reason_gives_up_and_investigates(monkeypatch): monkeypatch.setattr(treeherder.time, "sleep", lambda s: None) ticks = iter([0.0, treeherder.settings.treeherder_classification_wait_seconds + 1]) monkeypatch.setattr(treeherder.time, "monotonic", lambda: next(ticks)) - assert treeherder.await_skip_reason("autoland", "T1") is None + assert treeherder.await_skip_reason("autoland", "T1", _job(6)) is None