Aggregate autorevert restart runs like any other run in the HUD (partial revert of #6954) (#8494)

✴️ iz2: Partial revert of #6954 — the UI half of #8300.

**Problem.** Autorevert's restart runs (`workflow_dispatch` on
`trunk/<sha>`) were filtered out of the HUD, so their results —
including real failures — never reached the grid. And a multi-run cell
was merged pairwise, newest `job.id` wins, with the flaky flag taken
from the *loser*: a pass followed by a restart failure rendered plain
red, the reverse order rendered flaky `F`.

**Proposed solution.** Invariant: **who issued a run does not change how
the HUD aggregates it.** A push, a re-run attempt and a restart are all
just runs; a cell's verdict follows from the set of conclusions, not
from the issuer and not from arrival order. Origin is reported on the
tooltip, never aggregated on. Rules, in the new
`torchci/lib/mergeCellRuns.ts`:

- one run → show that run
- success + a real failure, in any order → show the success, marked
flaky `F`
- `cancelled` loses to any non-cancelled run, and is deliberately not
failure evidence (unlike `isFailure()`); `skipped` loses to a real
result, as before
- otherwise rank success > failure > pending > neutral > skipped, newest
run breaking a tie within a class

**What changes?** Restart results reach the grid on the same terms as
any other run, and a mixed cell renders `F` in either order. This
governs every multi-run cell, not just restarts — a twice-scheduled
periodic job or a re-run now follows the ranking above instead of
newest-id-wins, so an older success outranks a newer pending run.

Measured over 14 days on the 266 `pytorch/pytorch` trunk shas that had
restart runs (cells without a restart were not measured): **14,405 cells
differ from `main`** — 13,687 restart-only cells now show their real
conclusion, 208 turn `F` (restart failed, natural run passed), 195 the
reverse, 223 stop being `F` (a cancelled co-run no longer counts as
failure), 92 in smaller classes.

**Changed, at a high level**

- `hud_query` — admit restart runs; project the run's origin and
dispatching identity
- `mergeCellRuns.ts` + `types.ts` — the rules above and which run
represents the cell; unit-tested on both permutation axes (order,
issuer)
- `fetchHud.ts` — collect every run per cell and delegate; the old
reducer is gone
- `JobTooltip.tsx` — one radio per run behind the cell, with the grid's
own status glyph and a `gh` link to that run; picking one rebinds the
log viewer, links and failure classification to it
- `commit_jobs_query` — admits restarts that *passed* (all were hidden
before); its comment records the divergence below

**Known gaps.** A restart that *failed* still doesn't show on the commit
page: `fetchCommit` dedups by newest id with no flaky marker, so
admitting one would replace the natural conclusion instead of combining
with it. Grid and commit page disagree there until that page can render
`F` — tracked as a follow-up. `commit_jobs_batch_query` and its
`viable_strict_sole_blocker` mirror are untouched: `is_green()` has no
per-job-name dedup and vetoes on the first non-success row, so restarts
only add vetoes (17 currently-green commits would go red, 0 red would go
green) — #8300's second action item.

Refs #8300

---------

Co-authored-by: Ivan Zaitsev <izaitsevfb@meta.com>
Co-authored-by: Ivan Zaitsev <izaitsevfb@users.noreply.github.com>

Author: izaitsevfb

torchci/clickhouse_queries/commit_jobs_query/query.sql

@@ -37,7 +37,21 @@ WITH job AS (
         job.torchci_classification_kg.'context' as context,
         job.runner_name AS runner_name,
         workflow.head_commit. 'author'.'email' AS authorEmail,
-        job.run_attempt AS run_attempt
+        job.run_attempt AS run_attempt,
+        -- Origin of the run, so the workflow picker can say what it is offering instead of a bare
+        -- numeric id. Same multiIf as hud_query's run_origin on purpose: one run must not read two
+        -- different ways on the grid and on this page. A plain push stays NULL -- it is the
+        -- overwhelming majority and the default reading.
+        multiIf(
+            workflow.event = 'workflow_dispatch'
+            AND workflow.head_branch LIKE 'trunk/%',
+            'autorevert',
+            job.run_attempt > 1,
+            'retry',
+            workflow.event = 'push',
+            NULL,
+            workflow.event
+        ) AS run_origin
     FROM
         workflow_job job final
         INNER JOIN workflow_run workflow final ON workflow.id = job.run_id
@@ -46,7 +60,15 @@ WITH job AS (
         AND job.name != 'generate-test-matrix'
         AND workflow.event != 'workflow_run' -- Filter out workflow_run-triggered jobs, which have nothing to do with the SHA
         AND workflow.event != 'repository_dispatch' -- Filter out repository_dispatch-triggered jobs, which have nothing to do with the SHA
-        AND NOT (workflow.event = 'workflow_dispatch' AND workflow.head_branch LIKE 'trunk/%') -- Filter out restart jobs
+        -- KNOWN DIVERGENCE FROM hud_query, deliberate. hud_query no longer filters restarts by
+        -- conclusion at all: it admits every run and lets mergeCellRuns decide the cell
+        -- issuer-agnostically, so a restart that failed makes a passing cell render flaky instead of
+        -- vanishing. This surface cannot do that yet -- fetchCommit dedups by newest id and has no
+        -- flaky marker to render, so admitting a non-success restart here would silently REPLACE the
+        -- natural conclusion rather than combine with it. Until the commit page can express a flaky
+        -- result, the success-only filter stays, and the grid and the commit page will disagree about
+        -- a commit whose restart failed.
+        AND NOT (workflow.event = 'workflow_dispatch' AND workflow.head_branch LIKE 'trunk/%' AND job.conclusion_kg != 'success')
         AND workflow.id in (select id from materialized_views.workflow_run_by_head_sha where head_sha = {sha: String})
         AND (
             {workflowId: Int64} = 0
@@ -88,13 +110,27 @@ WITH job AS (
         [ ] as context,
         '' AS runner_name,
         workflow.head_commit.author.email AS authorEmail,
-        workflow.run_attempt as run_attempt
+        workflow.run_attempt as run_attempt,
+        -- Same column as the branch above, because a UNION ALL needs matching shapes. These rows
+        -- carry workflow_id = 0, which fetchCommit normalizes to null and getWorkflowIdsByName then
+        -- filters out, so a startup-failure pseudo-row never reaches the picker -- this is
+        -- projected for the union, not for the dropdown.
+        multiIf(
+            workflow.event = 'workflow_dispatch'
+            AND workflow.head_branch LIKE 'trunk/%',
+            'autorevert',
+            workflow.run_attempt > 1,
+            'retry',
+            workflow.event = 'push',
+            NULL,
+            workflow.event
+        ) AS run_origin
     FROM
         workflow_run workflow final
     WHERE
         workflow.event != 'workflow_run' -- Filter out workflow_run-triggered jobs, which have nothing to do with the SHA
         AND workflow.event != 'repository_dispatch' -- Filter out repository_dispatch-triggered jobs, which have nothing to do with the SHA
-        AND NOT (workflow.event = 'workflow_dispatch' AND workflow.head_branch LIKE 'trunk/%') -- Filter out restart jobs
+        AND NOT (workflow.event = 'workflow_dispatch' AND workflow.head_branch LIKE 'trunk/%' AND workflow.conclusion != 'success') -- Autorevert restart runs count only when they PASSED. This branch maps a still-queued run to 'failure', so admitting a restart here would put a red "Workflow Startup Failure / trunk" box on the commit page for the whole queue window
         AND workflow.id in (select id from materialized_views.workflow_run_by_head_sha where head_sha = {sha: String})
         AND (
             {workflowId: Int64} = 0
@@ -128,7 +164,8 @@ SELECT
     runner_name AS runnerName,
     authorEmail,
     time,
-    run_attempt AS runAttempt
+    run_attempt AS runAttempt,
+    run_origin AS runOrigin
 FROM
     job
 ORDER BY

torchci/clickhouse_queries/hud_query/query.sql

@@ -18,10 +18,63 @@ WITH job AS (
         job.torchci_classification_kg.'line' as line,
         job.torchci_classification_kg.'captures' as captures,
         job.torchci_classification_kg.'line_num' as line_num,
-        annotation.annotation as annotation
+        annotation.annotation as annotation,
+        -- Origin of the run, REPORTED but never aggregated on -- aggregation is issuer-agnostic by
+        -- design (mergeCellRuns). A plain push is left NULL rather than spelled out: it is the
+        -- overwhelming majority and the default reading, and fetchHud strips nulls before shipping
+        -- the grid, so ordinary rows cost nothing.
+        -- NULL means specifically a `push` run, so the UI can name the origin without guessing. Any
+        -- other event (schedule, a non-trunk workflow_dispatch, ...) carries its own event name --
+        -- calling those "push" would be wrong, and periodic schedules are one of the reasons a cell
+        -- has several runs in the first place.
+        multiIf(
+            job.workflow_event = 'workflow_dispatch'
+            AND job.head_branch LIKE 'trunk/%',
+            'autorevert',
+            job.run_attempt > 1,
+            'retry',
+            job.workflow_event = 'push',
+            NULL,
+            job.workflow_event
+        ) AS run_origin,
+        tupleElement(restart_run.latest, 1) as restart_actor_login,
+        tupleElement(restart_run.latest, 2) as restart_triggering_actor_login,
+        tupleElement(restart_run.latest, 3) as restart_run_attempt
     FROM
         workflow_job job final
         LEFT JOIN job_annotation annotation final ON job.id = annotation.jobID
+        -- workflow_job carries no actor column, so the dispatching identity has to come from
+        -- workflow_run. Scoped to restart runs for the requested shas only, and keyed through the
+        -- same materialized view commit_jobs_query uses so this is a primary-key lookup instead of
+        -- a scan on head_sha (which is not in workflow_run's sorting key).
+        -- GROUP BY + argMax rather than FINAL: workflow_run is a ReplacingMergeTree with no version
+        -- column, and FINAL on it measured ~5.6s against ~0.7s for this shape on the same input.
+        -- run_attempt is monotonic per run, so argMax also expresses what we actually want here --
+        -- the latest attempt -- which FINAL alone does not guarantee.
+        LEFT JOIN (
+            SELECT
+                id,
+                -- ONE argMax over a tuple rather than three separate ones: independent argMax calls
+                -- may resolve their tie differently and report an actor from one physical row with a
+                -- triggering actor from another. Ordering by run_attempt takes the latest attempt.
+                argMax(
+                    (actor.'login', triggering_actor.'login', run_attempt),
+                    run_attempt
+                ) AS latest
+            FROM workflow_run
+            WHERE
+                id in (
+                    select id from materialized_views.workflow_run_by_head_sha
+                    where head_sha in {shas: Array(String)}
+                )
+                -- That materialized view is keyed on head_sha ALONE (its only columns are id and
+                -- head_sha), so it can return a run id belonging to a different repo that shares the
+                -- sha -- a fork, most obviously. Constrain the repo here, since the MV cannot.
+                AND repository.'full_name' = {repo: String}
+                AND event = 'workflow_dispatch'
+                AND head_branch LIKE 'trunk/%'
+            GROUP BY id
+        ) restart_run ON restart_run.id = job.run_id
     WHERE
         job.name != 'ciflow_should_run'
         AND job.name != 'generate-test-matrix'
@@ -33,7 +86,12 @@ WITH job AS (
         )  -- Should be filtered out by the workflow_event filters, but workflow_event takes some time to populate
         AND job.workflow_event != 'workflow_run' -- Filter out workflow_run-triggered jobs, which have nothing to do with the SHA
         AND job.workflow_event != 'repository_dispatch' -- Filter out repository_dispatch-triggered jobs, which have nothing to do with the SHA
-        AND NOT (job.workflow_event = 'workflow_dispatch' AND job.head_branch LIKE 'trunk/%') -- Filter out restart jobs
+        -- Autorevert restart runs are no longer filtered out here, and are deliberately NOT filtered
+        -- by conclusion either. Who issued a run must not change how the HUD aggregates it, so a
+        -- restart is admitted on the same terms as a push or a re-run attempt and the cell verdict is
+        -- decided by mergeCellRuns over the whole set of runs. Filtering by conclusion here was the
+        -- issuer-dependent shortcut this replaces: it dropped a restart that failed, was pending or
+        -- was skipped, which silently hid real results rather than aggregating them.
         AND job.id in (select id from materialized_views.workflow_job_by_head_sha where head_sha in {shas: Array(String)})
         AND job.repository_full_name = {repo: String}
         AND job.workflow_name != 'Upload test stats while running' -- Continuously running cron job that cancels itself to avoid running concurrently
@@ -61,6 +119,35 @@ SELECT
     if(line = '', [ ], [ line ]) AS failureLines,
     if(line_num = 0, [ ], [ line_num ]) AS failureLineNumbers,
     captures as failureCaptures,
-    annotation as failureAnnotation
+    annotation as failureAnnotation,
+    run_origin as runOrigin,
+    -- The identity fields below are gated on an autorevert origin, so none can appear on a run the
+    -- origin does not also mark. The two sides are derived independently -- the origin from
+    -- workflow_job, the identity from workflow_run -- and nothing guarantees they agree under
+    -- ingestion lag. The reverse case (origin present, identity missing) stays possible and renders
+    -- fine, since each field is conditional in the tooltip.
+    -- An unmatched LEFT JOIN also yields the column default ('' / 0) rather than NULL, so normalize:
+    -- fetchHud strips only nulls, and an empty string would ship on every ordinary run and make
+    -- "field is present" a false test for "this run was dispatched by autorevert".
+    -- coalesce, not a bare comparison: run_origin is Nullable, and `NULL != 'autorevert'` is NULL,
+    -- which would make the whole if() condition NULL rather than false.
+    if(coalesce(run_origin, '') = 'autorevert', nullIf(restart_actor_login, ''), NULL) as restartDispatchedBy,
+    -- Only meaningful when it differs: triggering_actor equals the actor on a first attempt, and
+    -- names whoever re-ran the run on later ones. Collapsing the two would hide a human re-running
+    -- a bot's restart.
+    if(
+        coalesce(run_origin, '') = 'autorevert'
+        AND restart_triggering_actor_login != ''
+        AND restart_triggering_actor_login != restart_actor_login,
+        restart_triggering_actor_login,
+        NULL
+    ) as restartRerunBy,
+    -- Deliberately NOT called runAttempt: commit_jobs_query already ships that name with different
+    -- semantics (the JOB's run_attempt), and the HUD page reads it at
+    -- pages/hud/[repoOwner]/[repoName]/[branch]/[[...page]].tsx as `cr.run_attempt >
+    -- (existing.runAttempt ?? 0)` when merging crcr rows. That comparison relies on the field being
+    -- undefined in the HUD path today, so populating it here would silently change which job data
+    -- wins those cells.
+    if(coalesce(run_origin, '') = 'autorevert', nullIf(restart_run_attempt, 0), NULL) as restartRunAttempt
 FROM
     job

torchci/components/commit/WorkflowBox.tsx

@@ -16,6 +16,7 @@ import {
 import { fetcher } from "lib/GeneralUtils";
 import { getConclusionSeverityForSorting } from "lib/JobClassifierUtil";
 import { getDurationDisplay, isFailedJob } from "lib/jobUtils";
+import { describeWorkflowRun } from "lib/runOrigin";
 import { getSearchRes, LogSearchResult } from "lib/searchLogs";
 import { Artifact, IssueData, JobData } from "lib/types";
 import {
@@ -264,11 +265,17 @@ export default function WorkflowBox({
             >
               <option value={""}>Select Workflow ID</option>
               {allWorkflowIds.sort().map((id) => (
+                // A bare id says nothing about which run it is -- "which one of these was the
+                // autorevert restart" was the actual question asked in review. Every entry is
+                // named, not just the interesting ones, so the reader compares like with like
+                // rather than reading meaning into an absent label.
                 <option
                   key={`${id.id} ${id.attempt}`}
                   value={`${id.id} ${id.attempt}`}
                 >
-                  {id.id} (Attempt {id.attempt})
+                  {`${id.id} (Attempt ${id.attempt}) — ${describeWorkflowRun(
+                    id
+                  )}`}
                 </option>
               ))}
             </select>