[WIP] Test repair listener improvements

agents/test-repair/hackbot_agents/test_repair/notify.py

@@ -9,7 +9,7 @@
 then visible in the hackbot UI before it lands, and the apply step delivers it at
 most once (see ``hackbot_runtime.actions.slack``).
 
-Five lines of context, then the verdict in full. Every identifier a sheriff would
+A few lines of context, then the verdict in full. Every identifier a sheriff would
 otherwise have to look up -- revisions, task, bug, run -- is a link, the way the
 pulse listener's email does it
 (``services/hackbot-pulse-listener/app/notify.py``); unlike the email this stays
@@ -120,11 +120,19 @@ def _culprit_line(result: TestRepairResult, culprit_author: str | None) -> str:
     bug = result.culprit_bug or result.intermittent_bug
     if bug:
         line += f" ({_bug_link(bug)})"
-    if result.proposed_patch:
-        line += ", patch attached"
     return line
 
 
+def _patch_line(result: TestRepairResult) -> str | None:
+    """Who the attached patch is for, so it is not read as an alternative action."""
+    if not result.proposed_patch:
+        return None
+    return (
+        "Patch attached for the author: squash it into the existing patches and"
+        " reland, rather than landing it as a follow-up. The backout still stands."
+    )
+
+
 def build_message(
     result: TestRepairResult,
     investigation: Investigation,
@@ -144,6 +152,9 @@ def build_message(
         _culprit_line(result, culprit_author),
         _link(RUN_URL.format(run_id=run_id), "Hackbot run details"),
     ]
+    patch = _patch_line(result)
+    if patch:
+        lines.insert(-1, patch)
     if result.summary.strip():
         lines += ["", result.summary.strip()]
     return "\n".join(lines)

agents/test-repair/tests/test_notify.py

@@ -122,12 +122,16 @@ def test_names_a_known_intermittent():
     )
 
 
-def test_mentions_a_proposed_patch():
-    assert (
-        _message(_result(proposed_patch=True))
-        .splitlines()[4]
-        .endswith(", patch attached")
-    )
+def test_a_patch_is_advice_for_the_author_not_an_alternative_action():
+    message = _message(_result(proposed_patch=True))
+    assert "*test-repair: BACK OUT the culprit*" in message.splitlines()[0]
+    assert "squash it into the existing patches and reland" in message
+    assert "rather than landing it as a follow-up" in message
+    assert "The backout still stands." in message
+
+
+def test_no_patch_line_without_a_patch():
+    assert "Patch attached" not in _message()
 
 
 def test_lists_every_failing_group():

services/hackbot-pulse-listener/app/config.py

@@ -47,6 +47,8 @@ class Settings(BaseSettings):
     # Dedupe (in-memory, by hg revision)
     dedupe_ttl_seconds: int = 6 * 60 * 60
     dedupe_max_size: int = 4096
+    # Dedupe by failing manifest, on top of the per-push one above.
+    group_dedupe_ttl_seconds: int = 12 * 60 * 60
 
     # Cap on test-repair runs started in any rolling 24 hours. Each run clones and
     # builds Firefox in its own container, so a bad day on autoland -- a broken

services/hackbot-pulse-listener/app/consumer.py

@@ -50,6 +50,13 @@
 )
 _seen_tests_lock = threading.Lock()
 
+# A manifest stays broken on the pushes following the one that broke it, so per-push
+# dedupe alone still mails a near-identical analysis for each.
+_seen_groups: TTLCache = TTLCache(
+    maxsize=settings.dedupe_max_size, ttl=settings.group_dedupe_ttl_seconds
+)
+_seen_groups_lock = threading.Lock()
+
 # When each test-repair run of the last day was started, oldest first, capping how
 # many may run in any rolling 24 hours. A slot is taken at the moment of triggering
 # and given back if the trigger fails, so only runs that really started count.
@@ -273,6 +280,19 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None:
     # once. The same record carries the configuration the regression check compares
     # against.
     job = treeherder.job_for_task(project, task_id)
+
+    # Populated at ingestion, so this needs no wait for a sheriff to star the job.
+    intermittent = treeherder.intermittent_match(project, job)
+    if intermittent.known:
+        logger.info(
+            "Task %s failed only lines already known in this revision and tracked by "
+            "intermittent bug(s) %s; skipping -- %s",
+            task_id,
+            ", ".join(str(bug) for bug in intermittent.bug_ids),
+            job_link,
+        )
+        return None
+
     reason = treeherder.await_skip_reason(project, task_id, job)
     if reason:
         logger.info(
@@ -351,6 +371,15 @@ def claimed_elsewhere() -> bool:
         )
         return None
 
+    if _groups_claimed(project, fresh):
+        logger.info(
+            "Every failing group of task %s was already investigated on a recent "
+            "push; skipping -- %s",
+            task_id,
+            job_link,
+        )
+        return None
+
     # 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.
@@ -395,6 +424,24 @@ def _claim_push(hg_revision: str) -> bool:
         return True
 
 
+def _groups_claimed(project: str, groups: list[str]) -> bool:
+    """Whether a recent run already covered every one of these groups.
+
+    All, not any: a task that also broke an unanalysed manifest is still worth a run.
+    """
+    with _seen_groups_lock:
+        return bool(groups) and all((project, g) in _seen_groups for g in groups)
+
+
+def _claim_groups(project: str, groups: list[str]) -> list[tuple[str, str]]:
+    """Record the groups a run covers; returns the keys to release if it fails."""
+    keys = [(project, group) for group in groups]
+    with _seen_groups_lock:
+        for key in keys:
+            _seen_groups[key] = True
+    return keys
+
+
 def _test_runs_today() -> int:
     """Runs started in the window, dropping those that have aged out of it.
 
@@ -488,6 +535,8 @@ def _trigger_test_repair(
         _release_test_run()
         return None
 
+    group_keys = _claim_groups(project, test_groups)
+
     try:
         run_id = client.trigger_run(
             {"failure_tasks": {label: task_id}},
@@ -498,6 +547,7 @@ def _trigger_test_repair(
             "Failed to trigger test-repair run for task %s -- %s", task_id, job_link
         )
         _release(_seen_tests, _seen_tests_lock, [hg_revision])
+        _release(_seen_groups, _seen_groups_lock, group_keys)
         _release_test_run()
         return None
 

services/hackbot-pulse-listener/app/notify.py

@@ -12,18 +12,23 @@
 MAX_PATCH_LINES = 400
 
 
-def send_email(ctx: RunContext, run_doc: dict) -> None:
+def send_email(
+    ctx: RunContext, run_doc: dict, already_actioned: str | None = None
+) -> None:
     """Email the failure analysis. 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.
+
+    ``already_actioned`` is Treeherder's classification when a sheriff has already
+    dealt with the failure.
     """
     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)
+        _send_test_repair_email(ctx, run_doc, already_actioned)
     else:
         _send_build_repair_email(ctx, run_doc)
 
@@ -52,7 +57,9 @@ def _send_build_repair_email(ctx: RunContext, run_doc: dict) -> None:
     _deliver(subject, body_md, recipients, patch)
 
 
-def _send_test_repair_email(ctx: RunContext, run_doc: dict) -> None:
+def _send_test_repair_email(
+    ctx: RunContext, run_doc: dict, already_actioned: str | None = None
+) -> None:
     findings = (run_doc.get("summary") or {}).get("findings") or {}
     culprit = findings.get("culprit_commit")
     culprit_author = (
@@ -77,10 +84,15 @@ def _send_test_repair_email(ctx: RunContext, run_doc: dict) -> None:
         return
 
     patch = _fetch_patch(ctx.run_id, run_doc)
+    # In the subject too, so a sheriff can skip it from the inbox.
+    prefix = "[already actioned] " if already_actioned else ""
     subject = (
-        f"[test-repair] {_banner(findings)} - {_test_groups_label(ctx)} ({ctx.repo})"
+        f"[test-repair] {prefix}{_banner(findings)} - "
+        f"{_test_groups_label(ctx)} ({ctx.repo})"
+    )
+    body_md = _build_test_repair_body(
+        ctx, findings, patch, culprit_author, already_actioned
     )
-    body_md = _build_test_repair_body(ctx, findings, patch, culprit_author)
     _deliver(subject, body_md, recipients, patch)
 
 
@@ -147,15 +159,16 @@ def _recipients(primary: str | None, secondary: str | None = None) -> list[str]:
     return recipients
 
 
+# The headline names the sheriff's action, which is always a backout.
 _RECOMMENDATION_BANNER = {
     "backout": "BACK OUT the culprit",
     "do_not_backout": "DO NOT back out (intermittent)",
-    "land_fix": "LAND the proposed fix",
+    "land_fix": "BACK OUT the culprit, reland with the proposed fix squashed in",
 }
 
 
 def _banner(findings: dict) -> str:
-    """The recommendation as a human-readable headline."""
+    """The recommendation as a human-readable headline, or the raw value."""
     recommendation = findings.get("recommendation")
     return _RECOMMENDATION_BANNER.get(recommendation, recommendation or "analysis")
 
@@ -168,14 +181,28 @@ def _test_groups_label(ctx: RunContext) -> str:
     return f"{first} (+{len(rest)} more)" if rest else first
 
 
+def _already_actioned_banner(reason: str | None) -> list[str]:
+    """Say up front that the tree has been dealt with, when it has."""
+    if not reason:
+        return []
+    return [
+        f"> **Already actioned by a sheriff.** Treeherder now classifies this job as "
+        f"_{reason}_, so the tree has been dealt with and this analysis needs no "
+        f"action from a sheriff. It is sent for the developer's reland.",
+        "",
+    ]
+
+
 def _build_test_repair_body(
     ctx: RunContext,
     findings: dict,
     patch: str | None,
     culprit_author: str | None,
+    already_actioned: str | None = None,
 ) -> str:
     groups = ", ".join(f"`{g}`" for g in ctx.test_groups) or "not resolved"
     lines = [
+        *_already_actioned_banner(already_actioned),
         "# Test failure analysis",
         "",
         f"- **Recommendation:** {_banner(findings)}",
@@ -216,10 +243,21 @@ def _build_test_repair_body(
         lines.append(f"- **Bug:** [{bug}]({_bug_url(bug)})")
 
     lines += _run_details(ctx) + _analysis_sections(findings) + _patch_section(patch)
-    lines += _team_footer()
+    lines += _patch_advice(patch) + _team_footer()
     return "\n".join(lines)
 
 
+def _patch_advice(patch: str | None) -> list[str]:
+    """Say who the patch is for, next to the patch itself."""
+    if not patch:
+        return []
+    return [
+        "",
+        "_For the author: squash this into your existing patches and reland. It is a "
+        "suggestion, not a follow-up to land on its own._",
+    ]
+
+
 def _run_details(ctx: RunContext) -> list[str]:
     if not settings.hackbot_ui_url:
         return []