diff --git a/CHANGELOG.md b/CHANGELOG.md index dfbfe357408..0276314cff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,10 +17,10 @@ and this project adheres to ### Changed -- Runs on Erlang/OTP 28 and Elixir 1.18.4. OTP 27 only finishes normalising - the first character of a string, which breaks names in many languages. - Lightning does not normalise anything today, but #4577 adds it on every name, - so the runtime moves first. +- Runs on Erlang/OTP 28 and Elixir 1.18.4. OTP 27 only finishes normalising the + first character of a string, which breaks names in many languages. Lightning + does not normalise anything today, but #4577 adds it on every name, so the + runtime moves first. ### Added @@ -43,7 +43,9 @@ and this project adheres to - A workflow health page at `/projects/:project_id/w/:workflow_id/health`, summarising one workflow over a selectable window (last 24 hours, 7 days, or 30 days): a donut of work order outcomes, a breakdown of the failing ones, and - a triage table grouping failures by error signature, heaviest first. The page + a triage table grouping failures by error signature, heaviest first. Each row + has a View button linking to the history page filtered to just the work orders + behind it, where the existing "retry all" can retry the group. The page refreshes itself as that workflow's work orders settle, at most once every 30 seconds. Reachable from the workflows list via a "Health" link in each row's Actions column. @@ -73,17 +75,17 @@ and this project adheres to - AI assistant code blocks share the surface the workflow diffs use, so a reply and the diff below it no longer read as two different products. [#5118](https://github.com/OpenFn/lightning/issues/5118) -- A global assistant reply whose changes could not be applied now says so on - the reply itself, beside the diffs that did not land, and offers to try - again. It used to fall back to a raw YAML panel. +- A global assistant reply whose changes could not be applied now says so on the + reply itself, beside the diffs that did not land, and offers to try again. It + used to fall back to a raw YAML panel. [#5118](https://github.com/OpenFn/lightning/issues/5118) -- A failed apply is now remembered, so reloading no longer turns it back into - a success. The reply kept its diff blocks and offered to undo changes that - had never landed. A retry that works clears the record. +- A failed apply is now remembered, so reloading no longer turns it back into a + success. The reply kept its diff blocks and offered to undo changes that had + never landed. A retry that works clears the record. [#5118](https://github.com/OpenFn/lightning/issues/5118) - Editing an open step with the global assistant no longer puts a diff in the - code editor. The change is already applied, so the diff read as a proposal - to accept or reject when the only control was a close button, and reloading + code editor. The change is already applied, so the diff read as a proposal to + accept or reject when the only control was a close button, and reloading revealed the change had been written all along. [#5118](https://github.com/OpenFn/lightning/issues/5118) diff --git a/assets/js/health/WorkflowHealth.tsx b/assets/js/health/WorkflowHealth.tsx index da0db09df56..82760c86b97 100644 --- a/assets/js/health/WorkflowHealth.tsx +++ b/assets/js/health/WorkflowHealth.tsx @@ -6,7 +6,7 @@ import { FailureBreakdownDonut } from './charts/FailureBreakdownDonut'; import { OutcomesDonut } from './charts/OutcomesDonut'; import { TriageTable } from './charts/TriageTable'; import { DEFAULT_DAYS, RangePicker } from './RangePicker'; -import type { FailureSignatures, Outcomes } from './types'; +import type { ErrorSignatures, Outcomes } from './types'; import { FAILURE_STATES } from './types'; import type { Query } from './useHealthQuery'; import { healthBase, useHealthQuery } from './useHealthQuery'; @@ -58,7 +58,7 @@ export const HealthContent = ({ const base = healthBase(projectId, workflowId); const outcomes = useHealthQuery(`${base}/outcomes?days=${days}`); - const signatures = useHealthQuery( + const signatures = useHealthQuery( `${base}/failures?days=${days}` ); @@ -125,6 +125,9 @@ export const HealthContent = ({ )} diff --git a/assets/js/health/charts/TriageTable.tsx b/assets/js/health/charts/TriageTable.tsx index 7357dd3c39c..9cc0d4e6bc7 100644 --- a/assets/js/health/charts/TriageTable.tsx +++ b/assets/js/health/charts/TriageTable.tsx @@ -1,11 +1,12 @@ -import type { FailureSignature } from '../types'; +import type { ErrorSignature } from '../types'; /** - * Failed work orders grouped by error signature, heaviest first. - * - * Purely informational — there is nothing to act on here, so no row is a link - * or a control. The signature grammar is: - * `exitReason:errorType [@ stepName [adaptor@version]]`. + * Failed work orders grouped by error signature, heaviest first. Each row + * links to the history page filtered to the work orders it counts, where the + * existing "retry all" can act on the group. The signature grammar is: + * `exitReason:errorType [@ stepName [adaptor]]` — the adaptor renders without + * its version, since a merged row can span more than one (see `job_id` on + * `ErrorSignature`). */ // One sentence per error type the worker can report, written to hold @@ -59,24 +60,36 @@ const TIPS: Record = { }; interface TriageTableProps { - signatures: FailureSignature[]; + signatures: ErrorSignature[]; emptyMessage: string; + projectId: string; + workflowId: string; + /** `window.from` off the same response — the picked range's start. */ + from: string; } -export const TriageTable = ({ signatures, emptyMessage }: TriageTableProps) => { +export const TriageTable = ({ + signatures, + emptyMessage, + projectId, + workflowId, + from, +}: TriageTableProps) => { if (signatures.length === 0) { return

{emptyMessage}

; } return ( - // A rename forks a job's history into a signature per name, so a long-lived - // workflow can list far more rows than it has ways of breaking. Capped in - // height rather than in rows: the tail is still worth reading, just not - // worth pushing the rest of the page down for. `max-h` over a row count so - // a short list keeps the card short. -
+ // Capped in height rather than in rows: the tail is still worth reading, + // just not worth pushing the rest of the page down for. `max-h` over a + // row count so a short list keeps the card short. + // + // `-mr-6 pr-4` bleeds the scroll region out to the card's own edge (the + // card is `p-6`), so the scrollbar sits flush against it instead of + // floating in the middle of the card's padding. +
- + + {signatures.map(signature => ( + // job_id joins the key: a job deleted and recreated with the same + // name reads as two identical-looking signatures otherwise. - + {/* Nothing to link on a row whose `exit_reason` never resolved: + that leaves neither a step nor a mappable run state to filter + history on. */} + ))} @@ -115,25 +144,91 @@ export const TriageTable = ({ signatures, emptyMessage }: TriageTableProps) => { ); }; +/** + * Lands on history filtered to exactly the work orders this row counts, where + * the existing "retry all" can act on the group. Not labelled with the row's + * count — the filter re-derives the count on every load, so the number moves. + */ +const ViewButton = ({ href }: { href: string }) => ( + + View + + +); + +// A rejected work order never got a run, so the signature filter would fail +// closed on it server-side — history's existing `rejected` status filter is +// what actually matches these. `to_signature/2` gives every rejected row the +// same literal `exit_reason: "rejected"`, so that is the signal to switch. +// +// No status is ticked for the other rows: the signature filter carries +// `wo.state in failure_states()` itself, so the group is already exactly the +// row's, and a status the reason names would only subtract from it — a `fail:` +// row counts every work order whose latest run holds a step that failed, +// whatever state the run itself ended in. +const historyUrl = ( + projectId: string, + workflowId: string, + from: string, + signature: ErrorSignature +) => { + const params = new URLSearchParams({ + 'filters[workflow_id]': workflowId, + 'filters[date_after]': from, + }); + + if (signature.exit_reason === 'rejected') { + params.set('filters[rejected]', 'true'); + } else { + params.set('filters[error_signature_exit_reason]', signature.exit_reason); + if (signature.error_type) { + params.set('filters[error_signature_error_type]', signature.error_type); + } + if (signature.job_id) { + params.set('filters[error_signature_job_id]', signature.job_id); + } + } + + return `/projects/${projectId}/history?${params.toString()}`; +}; + // The parts are styled apart rather than concatenated server-side: the error // type is the bit worth scanning down the column for. -const Signature = ({ signature }: { signature: FailureSignature }) => ( +const Signature = ({ signature }: { signature: ErrorSignature }) => (

{signature.exit_reason}: {errorTypeOf(signature)} {signature.step_name && @ {signature.step_name}} {signature.adaptor && ( - [{signature.adaptor}] + + {' '} + [{packageNameOf(signature.adaptor)}] + )}

); +// A row is keyed and labelled by `job_id`, not by (job_id, adaptor), so a row +// spanning an adaptor bump mid-window is labelled from its newest failing +// snapshot. Rendering that snapshot's version would head older failures with a +// version that isn't theirs, so only the package name renders. Strips +// everything from the last '@' that isn't the scope's leading one, so a scoped +// package's own '@' survives. +const packageNameOf = (adaptor: string) => { + const lastAt = adaptor.lastIndexOf('@'); + return lastAt > 0 ? adaptor.slice(0, lastAt) : adaptor; +}; + // A step can finish without reporting a type, and a worker can report one as an // empty string. The signature still has to say something, and `default` is the // tip written for exactly that case — hence `||`, which catches '' as well as // null, where `??` would render a bare `fail:` and a tip with no sentence. -const errorTypeOf = ({ error_type }: FailureSignature) => - error_type || 'unknown'; +const errorTypeOf = ({ error_type }: ErrorSignature) => error_type || 'unknown'; -const tipFor = ({ error_type }: FailureSignature) => +const tipFor = ({ error_type }: ErrorSignature) => (error_type && TIPS[error_type]) || TIPS['default']; diff --git a/assets/js/health/types.ts b/assets/js/health/types.ts index dac76b71daa..23f7acbe4bb 100644 --- a/assets/js/health/types.ts +++ b/assets/js/health/types.ts @@ -44,18 +44,23 @@ export interface Outcomes { * One row of the triage table: the parts of an error signature and * the number of work orders that carry it. `step_name` and `adaptor` are null * for a work order whose run failed before reaching a step, or never ran at - * all; `error_type` is null when nothing reported one. + * all; `error_type` is null when nothing reported one. `job_id` is the same + * story as `step_name`/`adaptor` — null for a run-level row and for a + * rejected one — but it is the key the history filter matches on, since + * matching on the resolved name would need a snapshot lookup the filter + * doesn't do. */ -export interface FailureSignature { +export interface ErrorSignature { count: number; exit_reason: string; error_type: string | null; + job_id: string | null; step_name: string | null; adaptor: string | null; } /** The `failures` response, heaviest signature first. */ -export interface FailureSignatures { +export interface ErrorSignatures { window: { from: string; to: string }; - signatures: FailureSignature[]; + signatures: ErrorSignature[]; } diff --git a/assets/test/health/WorkflowHealth.test.tsx b/assets/test/health/WorkflowHealth.test.tsx index f3efdb717b9..5202bd4592f 100644 --- a/assets/test/health/WorkflowHealth.test.tsx +++ b/assets/test/health/WorkflowHealth.test.tsx @@ -18,20 +18,21 @@ const outcomes = { }, }; -const failureSignatures = { +const errorSignatures = { window: outcomes.window, signatures: [ { count: 98, exit_reason: 'fail', error_type: 'RuntimeError', + job_id: 'a1b2c3d4-0000-0000-0000-000000000000', step_name: 'Map-beneficiary', adaptor: '@openfn/language-common@2.0.0', }, ], }; -const both = { outcomes, failures: failureSignatures }; +const both = { outcomes, failures: errorSignatures }; const ERROR = 'Could not load workflow stats. Refresh to try again.'; @@ -231,7 +232,7 @@ describe('WorkflowHealth', () => { window: { from: '2026-08-30T10:00:00Z', to: '2026-08-31T10:00:00Z' }, }; - mount({ outcomes: dayWide, failures: failureSignatures }); + mount({ outcomes: dayWide, failures: errorSignatures }); expect( await screen.findByText('Last 24 hours · 1,287 work orders') @@ -259,7 +260,7 @@ describe('WorkflowHealth', () => { expect(screen.queryByText('cancelled')).not.toBeInTheDocument(); }); - test('lists the failure signatures in the triage table', async () => { + test('lists the error signatures in the triage table', async () => { mount(both); expect( @@ -282,7 +283,7 @@ describe('WorkflowHealth', () => { }); test('degrades both donuts when the outcomes request fails', async () => { - mount({ outcomes: 500, failures: failureSignatures }); + mount({ outcomes: 500, failures: errorSignatures }); // Both donuts read the same response, so both degrade. expect(await screen.findAllByText(ERROR)).toHaveLength(2); diff --git a/assets/test/health/charts/TriageTable.test.tsx b/assets/test/health/charts/TriageTable.test.tsx index db3d500bb75..db5a23bddf3 100644 --- a/assets/test/health/charts/TriageTable.test.tsx +++ b/assets/test/health/charts/TriageTable.test.tsx @@ -2,14 +2,15 @@ import { render, screen, within } from '@testing-library/react'; import { describe, expect, test } from 'vitest'; import { TriageTable } from '#/health/charts/TriageTable'; -import type { FailureSignature } from '#/health/types'; +import type { ErrorSignature } from '#/health/types'; const signature = ( - overrides: Partial = {} -): FailureSignature => ({ + overrides: Partial = {} +): ErrorSignature => ({ count: 62, exit_reason: 'fail', error_type: 'RuntimeError', + job_id: 'a1b2c3d4-0000-0000-0000-000000000000', step_name: 'Map-beneficiary', adaptor: '@openfn/language-common@2.0.0', ...overrides, @@ -19,14 +20,25 @@ const rowText = (name: string | RegExp) => within(screen.getByRole('row', { name })).getByRole('cell', { name }) .textContent; +// Every test renders the same workflow at the same window, since only the +// signature varies between them. +const table = (signatures: ErrorSignature[], emptyMessage = 'No failures') => + render( + + ); + describe('TriageTable', () => { - test('renders the full signature grammar for a step-level failure', () => { - render( - - ); + test('renders the full signature grammar for a step-level failure, adaptor version dropped', () => { + table([signature()]); expect(rowText(/RuntimeError/)).toContain( - 'fail:RuntimeError @ Map-beneficiary [@openfn/language-common@2.0.0]' + 'fail:RuntimeError @ Map-beneficiary [@openfn/language-common]' ); expect(screen.getByRole('cell', { name: '62' })).toBeVisible(); expect( @@ -37,19 +49,15 @@ describe('TriageTable', () => { // A run that crashed before any step has no job to name, so the grammar's // optional clause drops rather than rendering an empty ` @ []`. test('drops the step clause when nothing reached a step', () => { - render( - - ); + table([ + signature({ + exit_reason: 'crash', + error_type: 'CompileError', + job_id: null, + step_name: null, + adaptor: null, + }), + ]); const text = rowText(/CompileError/); expect(text).toContain('crash:CompileError'); @@ -61,19 +69,15 @@ describe('TriageTable', () => { // signature off, so the server labels it outright — and the tip has to be // there, or the row reads as a bug in the page. test('names a rejected work order and tips it', () => { - render( - - ); + table([ + signature({ + exit_reason: 'rejected', + error_type: 'RunLimitExceeded', + job_id: null, + step_name: null, + adaptor: null, + }), + ]); expect(rowText(/RunLimitExceeded/)).toContain('rejected:RunLimitExceeded'); expect( @@ -84,12 +88,7 @@ describe('TriageTable', () => { }); test('shows the tip written for the error type', () => { - render( - - ); + table([signature({ error_type: 'OOMError' })]); expect( screen.getByText( @@ -101,15 +100,10 @@ describe('TriageTable', () => { // A crashing step reports these, never `RuntimeCrash` — see TIPS. Untipped, // the most common crash there is falls through to "no recognised error type". test('tips the raw JS names that reach Lightning in place of RuntimeCrash', () => { - render( - - ); + table([ + signature({ exit_reason: 'crash', error_type: 'ReferenceError' }), + signature({ exit_reason: 'crash', error_type: 'SyntaxError' }), + ]); expect(screen.getByText(/often a typo or a missing import/)).toBeVisible(); expect(screen.getByText(/wasn't in the format it expected/)).toBeVisible(); @@ -119,15 +113,10 @@ describe('TriageTable', () => { // set — and a step can finish without reporting one at all. Neither may // render a blank tip. test('falls back to the default tip for an unmapped or missing type', () => { - render( - - ); + table([ + signature({ error_type: 'SomeUnmappedError' }), + signature({ error_type: null, step_name: 'Verify-cedula' }), + ]); const fallback = 'The step failed without a recognised error type; check its logs.'; @@ -140,12 +129,7 @@ describe('TriageTable', () => { // `??` let that through, rendering the signature as a bare `fail:` and a // "Tip: " with no sentence after it. test('treats an empty error type as a missing one', () => { - render( - - ); + table([signature({ error_type: '' })]); expect(rowText(/unknown/)).toContain('fail:unknown @ Map-beneficiary'); expect( @@ -156,14 +140,86 @@ describe('TriageTable', () => { }); test('shows the empty message when nothing failed', () => { - render( - - ); + table([], 'No failures in the last 30 days'); expect(screen.getByText('No failures in the last 30 days')).toBeVisible(); expect(screen.queryByRole('table')).not.toBeInTheDocument(); }); + + describe('View button', () => { + // The normal case: a step-level row, keyed and filtered on job_id. + test('links a normal row to history filtered on the signature', () => { + table([signature()]); + + const link = screen.getByRole('link', { name: 'View' }); + expect(link).toHaveAttribute( + 'href', + '/projects/proj-1/history' + + '?filters%5Bworkflow_id%5D=wf-1' + + '&filters%5Bdate_after%5D=2026-08-01T10%3A00%3A00Z' + + '&filters%5Berror_signature_exit_reason%5D=fail' + + '&filters%5Berror_signature_error_type%5D=RuntimeError' + + '&filters%5Berror_signature_job_id%5D=a1b2c3d4-0000-0000-0000-000000000000' + ); + // The health page is a dashboard people read row by row — the row they + // came from has to still be there when they come back. + expect(link).toHaveAttribute('target', '_blank'); + }); + + // A run that crashed before reaching a step has no job to key on, so the + // filter must omit job_id — matching the filter's own reading of an + // absent job_id as "the run-level row" rather than matching nothing. + test('omits job_id and error_type for a row with no step', () => { + table([ + signature({ + exit_reason: 'crash', + error_type: null, + job_id: null, + step_name: null, + adaptor: null, + }), + ]); + + const link = screen.getByRole('link', { name: 'View' }); + expect(link).toHaveAttribute( + 'href', + '/projects/proj-1/history' + + '?filters%5Bworkflow_id%5D=wf-1' + + '&filters%5Bdate_after%5D=2026-08-01T10%3A00%3A00Z' + + '&filters%5Berror_signature_exit_reason%5D=crash' + ); + }); + + // A rejected row has no run to key a signature filter on — that filter + // fails closed server-side — so it links to history's own `rejected` + // status filter instead, not to the signature's own fields. + test('links a rejected row to the rejected status filter, not the signature', () => { + table([ + signature({ + exit_reason: 'rejected', + error_type: 'RunLimitExceeded', + job_id: null, + step_name: null, + adaptor: null, + }), + ]); + + const link = screen.getByRole('link', { name: 'View' }); + expect(link).toHaveAttribute( + 'href', + '/projects/proj-1/history' + + '?filters%5Bworkflow_id%5D=wf-1' + + '&filters%5Bdate_after%5D=2026-08-01T10%3A00%3A00Z' + + '&filters%5Brejected%5D=true' + ); + expect(link.getAttribute('href')).not.toContain('error_signature_'); + }); + + // Nothing to filter history on without a resolved exit_reason. + test('renders no button when exit_reason never resolved', () => { + table([signature({ exit_reason: '' })]); + + expect(screen.queryByRole('link', { name: 'View' })).toBeNull(); + }); + }); }); diff --git a/lib/lightning/invocation.ex b/lib/lightning/invocation.ex index d550a80b10c..f0fb929aeb1 100644 --- a/lib/lightning/invocation.ex +++ b/lib/lightning/invocation.ex @@ -15,6 +15,7 @@ defmodule Lightning.Invocation do alias Lightning.Projects.Project alias Lightning.Repo alias Lightning.Run + alias Lightning.RunStep alias Lightning.Workflows.Edge alias Lightning.Workflows.Job alias Lightning.Workflows.Trigger @@ -622,6 +623,7 @@ defmodule Lightning.Invocation do |> filter_by_wo_date_before(search_params.wo_date_before) |> filter_by_date_after(search_params.date_after) |> filter_by_date_before(search_params.date_before) + |> filter_by_error_signature(search_params) |> filter_by_body_or_log_or_id( search_params.search_fields, search_params.search_term @@ -717,6 +719,116 @@ defmodule Lightning.Invocation do ) end + # The inverse of `Run.state_reasons/0`, for reading a run-level signature's + # `exit_reason` back into the state it came from. `"rejected"` is not a + # value in that map — it is `to_signature/2`'s own literal for a work order + # that never got a run — so it naturally misses here and falls through to + # `filter_by_error_signature/2`'s fail-closed branch, exactly like any + # other exit_reason no run can actually be in. + @reason_states Map.new(Run.state_reasons(), fn {state, reason} -> + {reason, state} + end) + + # A triage row's "View" button, scoped to exactly the work orders it + # counted. `error_signature_exit_reason` switches the filter on; a + # present `error_signature_job_id` reads as the step-level row, an + # absent one as the run-level row (no failing step) — safe because + # `steps.job_id` is `NOT NULL`. + # + # Carries `wo.state in failure_states()` itself: a *successful* work order + # can still hold a `fail` step in its latest run (an `on_job_failure` + # handler that ran fine), so without this a signature filter would match + # work orders the triage row never counted, and bulk retry would follow. + defp filter_by_error_signature(query, %SearchParams{ + error_signature_exit_reason: nil + }), + do: query + + defp filter_by_error_signature(query, %SearchParams{ + error_signature_exit_reason: exit_reason, + error_signature_error_type: error_type, + error_signature_job_id: job_id + }) + when is_binary(job_id) do + step_match = + from(s in failing_steps_of_latest_run(), + where: s.job_id == ^job_id and s.exit_reason == ^exit_reason, + where: + fragment( + "coalesce(nullif(?, ''), nullif(?, '')) IS NOT DISTINCT FROM ?", + s.error_type, + parent_as(:latest_run).error_type, + type(^error_type, :string) + ) + ) + + from([workorder: wo] in query, + where: wo.state in ^WorkOrder.failure_states(), + where: + exists( + from(r in subquery(latest_run_for_workorder()), + as: :latest_run, + where: exists(subquery(step_match)) + ) + ) + ) + end + + defp filter_by_error_signature(query, %SearchParams{ + error_signature_exit_reason: exit_reason, + error_signature_error_type: error_type, + error_signature_job_id: nil + }) do + case Map.fetch(@reason_states, exit_reason) do + {:ok, state} -> + from([workorder: wo] in query, + where: wo.state in ^WorkOrder.failure_states(), + where: + exists( + from(r in subquery(latest_run_for_workorder()), + as: :latest_run, + where: r.state == ^state, + where: + fragment( + "nullif(?, '') IS NOT DISTINCT FROM ?", + r.error_type, + type(^error_type, :string) + ), + where: not exists(subquery(failing_steps_of_latest_run())) + ) + ) + ) + + # An `exit_reason` no run can actually be in — fail closed rather than + # drop the filter, which would widen a bulk retry to every failure. + :error -> + from([workorder: wo] in query, where: false) + end + end + + # The run that speaks for a work order: most recently finished first, ties + # broken by id. Correlated on the outer query's `:workorder` binding. + defp latest_run_for_workorder do + from(r in Run, + as: :run, + where: r.work_order_id == parent_as(:workorder).id + ) + |> Query.order_by_run_recency() + |> limit(1) + end + + # Every step of the `:latest_run` binding's run that did not succeed. + # Correlated on `:latest_run`, so it only makes sense nested inside a query + # that introduces that binding. + defp failing_steps_of_latest_run do + from(s in Step, + join: rs in RunStep, + on: rs.step_id == s.id, + where: rs.run_id == parent_as(:latest_run).id + ) + |> Query.where_step_failed() + end + defp filter_by_body_or_log_or_id(query, _search_fields, nil), do: query defp filter_by_body_or_log_or_id(query, search_fields, search_term) do diff --git a/lib/lightning/invocation/query.ex b/lib/lightning/invocation/query.ex index 710ec3591f0..b4ea6abac08 100644 --- a/lib/lightning/invocation/query.ex +++ b/lib/lightning/invocation/query.ex @@ -40,6 +40,28 @@ defmodule Lightning.Invocation.Query do ) end + @doc """ + Appends `exit_reason != "success"` to a query of `Step`. + + A step that never finished carries a `nil` `exit_reason`, and `NULL != + 'success'` is false in SQL, so it is excluded here too — the same answer + the janitor's own race resolves to. Do not "fix" this to `IS DISTINCT + FROM`; that changes which side of the race a lost step lands on. + """ + @spec where_step_failed(Ecto.Queryable.t()) :: Ecto.Queryable.t() + def where_step_failed(query) do + from(s in query, where: s.exit_reason != "success") + end + + # Shared so the health page's bulk `DISTINCT ON` and the history filter's + # correlated per-work-order lookup can't drift apart on what "latest" means. + @spec order_by_run_recency(Ecto.Queryable.t()) :: Ecto.Queryable.t() + def order_by_run_recency(query) do + from([run: r] in query, + order_by: [desc_nulls_last: r.finished_at, desc: r.id] + ) + end + @doc """ Runs for a specific project, or all runs available to the requesting user """ diff --git a/lib/lightning/jobs.ex b/lib/lightning/jobs.ex index b256157713d..1f2baf00bac 100644 --- a/lib/lightning/jobs.ex +++ b/lib/lightning/jobs.ex @@ -163,6 +163,28 @@ defmodule Lightning.Jobs do |> Repo.preload(:credential) end + @doc """ + Gets the name of a job in a project. + + Returns `nil` when the id names no job in that project — including when it + is not a UUID at all, since it reaches here from a query string. + """ + def get_job_name(project_id, job_id) do + case Ecto.UUID.cast(job_id) do + {:ok, id} -> + Repo.one( + from(j in Job, + join: w in assoc(j, :workflow), + where: j.id == ^id and w.project_id == ^project_id, + select: j.name + ) + ) + + :error -> + nil + end + end + @doc """ Creates a job. diff --git a/lib/lightning/workflows/stats.ex b/lib/lightning/workflows/stats.ex index cfa204078ca..024e2acf58a 100644 --- a/lib/lightning/workflows/stats.ex +++ b/lib/lightning/workflows/stats.ex @@ -17,6 +17,7 @@ defmodule Lightning.Workflows.Stats do """ import Ecto.Query + alias Lightning.Invocation.Query alias Lightning.Invocation.Step alias Lightning.Repo alias Lightning.Run @@ -34,15 +35,6 @@ defmodule Lightning.Workflows.Stats do @final_states WorkOrder.final_states() @zero_counts Map.new(@final_states, &{&1, 0}) - # `:cancelled` is final but not a failure — someone stopped it on purpose. Own - # outcome, not the red wedge, which is why this narrows the schema's list - # rather than changing it. - @failure_states WorkOrder.failure_states() -- [:cancelled] - - # The signature grammar is written in the worker's words, so a - # run-level failure has to be mapped back out of its state. - @state_reasons Run.state_reasons() - @doc """ Work order counts by final state over the last `days_back` days. """ @@ -86,7 +78,7 @@ defmodule Lightning.Workflows.Stats do sum past the failure total the outcomes donut draws, which is the trade: a second broken branch is its own thing to fix, not fallout from the first. """ - def failure_signatures( + def error_signatures( %Workflow{id: workflow_id}, days_back \\ @default_days_back ) @@ -147,9 +139,7 @@ defmodule Lightning.Workflows.Stats do from(s in Step, join: rs in RunStep, on: rs.step_id == s.id, - where: - rs.run_id == parent_as(:latest_run).run_id and - s.exit_reason != "success", + where: rs.run_id == parent_as(:latest_run).run_id, select: %{ exit_reason: s.exit_reason, error_type: s.error_type, @@ -157,6 +147,7 @@ defmodule Lightning.Workflows.Stats do job_id: s.job_id } ) + |> Query.where_step_failed() from(lr in subquery(latest_runs(workflow_id, since)), as: :latest_run, @@ -181,12 +172,13 @@ defmodule Lightning.Workflows.Stats do from(wo in WorkOrder, # left_join, not join: a rejected work order has no run and still counts. left_join: r in Run, + as: :run, on: r.work_order_id == wo.id, where: wo.workflow_id == ^workflow_id and wo.last_activity > ^since and - wo.state in ^@failure_states, + wo.state in ^WorkOrder.failure_states(), distinct: wo.id, - order_by: [asc: wo.id, desc_nulls_last: r.finished_at, desc: r.id], + order_by: [asc: wo.id], select: %{ work_order_id: wo.id, work_order_state: wo.state, @@ -195,6 +187,7 @@ defmodule Lightning.Workflows.Stats do run_error_type: r.error_type } ) + |> Query.order_by_run_recency() end # The job's name and adaptor come off the run's own snapshot, not the live @@ -240,8 +233,12 @@ defmodule Lightning.Workflows.Stats do end # Keyed by snapshot as well as job, because the same job id carries a - # different name in every snapshot that renamed it. Only `name` and `adaptor` - # are read out, so the job bodies alongside them never cross the wire. + # different name in every snapshot that renamed it. `lock_version` rides + # along for `merge_counts/1` to pick a label with, since a snapshot's own id + # carries no timestamp to compare rows by — it is unique and monotonic per + # workflow, so the highest one read is the most recent. Only these three + # fields are read out, so the job bodies alongside them never cross the + # wire. defp snapshot_jobs([]), do: %{} defp snapshot_jobs(snapshot_ids) do @@ -250,7 +247,8 @@ defmodule Lightning.Workflows.Stats do cross_lateral_join: j in fragment("jsonb_array_elements(?)", s.jobs), select: {{s.id, fragment("? ->> ?", j, "id")}, - {fragment("? ->> ?", j, "name"), fragment("? ->> ?", j, "adaptor")}} + {fragment("? ->> ?", j, "name"), fragment("? ->> ?", j, "adaptor"), + s.lock_version}} ) |> Repo.all() |> Map.new() @@ -258,12 +256,14 @@ defmodule Lightning.Workflows.Stats do # A rejected work order never got a run, so there is no signature to read. # `:rejected` has one origin — the run limit refusing a webhook payload — so - # the label names it outright. + # the label names it outright. `job_id: nil` gives it the same shape as + # every other row, for the JSON and the TS type on the other end of it. defp to_signature(%{work_order_state: :rejected} = row, _jobs) do %{ count: row.count, exit_reason: "rejected", error_type: "RunLimitExceeded", + job_id: nil, step_name: nil, adaptor: nil } @@ -273,28 +273,69 @@ defmodule Lightning.Workflows.Stats do # reported — a lost or reaped run — carries only the reason, and the rest # comes off the run. `mark_steps_lost/1` is why: it stamps `exit_reason` and # leaves `error_type` alone. + # + # `job_id` is the key `merge_counts/1` folds rows on; `step_name` and + # `adaptor` are labels, resolved off the snapshot and carried with + # `lock_version` so the fold can pick the newest one and then drop it. defp to_signature(row, jobs) do - {step_name, adaptor} = - Map.get(jobs, {row.snapshot_id, row.job_id}, {nil, nil}) + {step_name, adaptor, lock_version} = + Map.get(jobs, {row.snapshot_id, row.job_id}, {nil, nil, nil}) %{ count: row.count, - exit_reason: row.exit_reason || @state_reasons[row.run_state], - error_type: row.error_type || row.run_error_type, + exit_reason: exit_reason(row.exit_reason, row.run_state), + error_type: error_type(row.error_type, row.run_error_type), + job_id: row.job_id, step_name: step_name, - adaptor: adaptor + adaptor: adaptor, + lock_version: lock_version } end + # `mark_steps_lost/1` stamps a step's `exit_reason` and nothing else, so a + # crashed run with no step at all falls back to `Run.state_reasons/0` — the + # worker's own words for each terminal state. + defp exit_reason(step_exit_reason, run_state) do + step_exit_reason || Map.get(Run.state_reasons(), run_state) + end + + # An empty string is missing on both sides: `"" || x` returns `""` (empty + # string is truthy in Elixir), which would split one failure into two + # identical-looking rows. + defp error_type(step_error_type, run_error_type) do + blank_to_nil(step_error_type) || blank_to_nil(run_error_type) + end + + defp blank_to_nil(""), do: nil + defp blank_to_nil(other), do: other + # Two groups can collapse into one signature — a crashed run and a failed run - # whose steps both reported `fail`, say — so the fold happens after the - # coalesce, not in the `group_by`. + # whose steps both reported `fail`, say, or the same job renamed mid-window — + # so the fold happens after the coalesce, not in the `group_by`. Grouping key + # is the triple that identifies a failure: the label (`step_name`, `adaptor`) + # is expected to differ between rows a rename merges, and `lock_version` + # never repeats. defp merge_counts(signatures) do signatures - |> Enum.group_by(&Map.delete(&1, :count), & &1.count) - |> Enum.map(fn {signature, counts} -> - Map.put(signature, :count, Enum.sum(counts)) - end) + |> Enum.group_by(&{&1.exit_reason, &1.error_type, &1.job_id}) + |> Enum.map(fn {_key, rows} -> merge_group(rows) end) |> Enum.sort_by(&{-&1.count, &1.step_name}, :asc) end + + # The label comes from the group's most recent failing snapshot: the row + # with the highest `lock_version`. A row with no snapshot at all (a rejected + # work order, or a run that never reached a step) carries no `lock_version`, + # which sorts lowest and never wins over a labelled one. + defp merge_group(rows) do + label = Enum.max_by(rows, &(&1[:lock_version] || -1)) + + %{ + count: Enum.sum_by(rows, & &1.count), + exit_reason: label.exit_reason, + error_type: label.error_type, + job_id: label.job_id, + step_name: label.step_name, + adaptor: label.adaptor + } + end end diff --git a/lib/lightning/workorders/search_params.ex b/lib/lightning/workorders/search_params.ex index e68f58f8f14..6b3a082247e 100644 --- a/lib/lightning/workorders/search_params.ex +++ b/lib/lightning/workorders/search_params.ex @@ -17,7 +17,10 @@ defmodule Lightning.WorkOrders.SearchParams do :wo_date_after, :wo_date_before, :sort_by, - :sort_direction + :sort_direction, + :error_signature_exit_reason, + :error_signature_error_type, + :error_signature_job_id ] @derive {Jason.Encoder, only: @fields} @@ -51,7 +54,10 @@ defmodule Lightning.WorkOrders.SearchParams do wo_date_after: DateTime.t(), wo_date_before: DateTime.t(), sort_by: String.t(), - sort_direction: String.t() + sort_direction: String.t(), + error_signature_exit_reason: String.t(), + error_signature_error_type: String.t(), + error_signature_job_id: Ecto.UUID.t() } @primary_key false @@ -72,6 +78,15 @@ defmodule Lightning.WorkOrders.SearchParams do field(:wo_date_before, :utc_datetime_usec) field(:sort_by, :string) field(:sort_direction, :string) + + # The error signature the workflow health page's triage row draws its + # "View" button from. `error_signature_exit_reason` switches the + # filter on; a present `error_signature_job_id` is a step-level row, + # an absent one a run-level row. See + # `Lightning.Invocation.filter_by_error_signature/2`. + field(:error_signature_exit_reason, :string) + field(:error_signature_error_type, :string) + field(:error_signature_job_id, :binary_id) end # Raises on invalid input. A malformed filter is only reachable by hand-editing @@ -99,25 +114,26 @@ defmodule Lightning.WorkOrders.SearchParams do end defp from_uri(params) do - statuses = - Enum.map(params, fn {key, value} -> - if key in @statuses and value in [true, "true"] do - key - end - end) - |> Enum.reject(&is_nil/1) - - search_fields = - Enum.map(params, fn {key, value} -> - if key in @search_fields and value in [true, "true"] do - key - end - end) - |> Enum.reject(&is_nil/1) - params - |> Map.put_new("status", statuses) - |> Map.put_new("search_fields", search_fields) + |> Map.put_new("status", selected(params, @statuses)) + |> put_search_fields(params) + end + + # A URL with none of the four search-field flags in it hasn't turned them + # off — it just didn't mention them. Leaving the key out lets the schema + # default (all four) stand; setting it to `[]` makes every search term match + # nothing. The form ships a hidden `false` beside every checkbox, so an + # actual "all off" still arrives as four keys rather than as none. + defp put_search_fields(uri_params, params) do + if Enum.any?(@search_fields, &Map.has_key?(params, &1)) do + Map.put_new(uri_params, "search_fields", selected(params, @search_fields)) + else + uri_params + end + end + + defp selected(params, keys) do + for {key, value} <- params, key in keys, value in [true, "true"], do: key end def to_uri_params(search_params) do diff --git a/lib/lightning/workorders/workorder.ex b/lib/lightning/workorders/workorder.ex index abea2d6e05b..98464acf183 100644 --- a/lib/lightning/workorders/workorder.ex +++ b/lib/lightning/workorders/workorder.ex @@ -50,9 +50,10 @@ defmodule Lightning.WorkOrder do @doc """ Returns the list of failure states for a work order. - These are all final states except `:success`. + Every final state except `:success` and `:cancelled` — a cancelled work + order stopped because someone stopped it, not because anything failed. """ - def failure_states, do: final_states() -- [:success] + def failure_states, do: final_states() -- [:success, :cancelled] @derive {Jason.Encoder, only: [ diff --git a/lib/lightning_web/controllers/api/workflow_health_controller.ex b/lib/lightning_web/controllers/api/workflow_health_controller.ex index 3176c8978fb..4def997ef48 100644 --- a/lib/lightning_web/controllers/api/workflow_health_controller.ex +++ b/lib/lightning_web/controllers/api/workflow_health_controller.ex @@ -27,10 +27,10 @@ defmodule LightningWeb.API.WorkflowHealthController do ) end - def failure_signatures(conn, _params) do + def error_signatures(conn, _params) do json( conn, - Workflows.Stats.failure_signatures( + Workflows.Stats.error_signatures( conn.assigns.workflow, conn.assigns.days_back ) diff --git a/lib/lightning_web/live/run_live/index.ex b/lib/lightning_web/live/run_live/index.ex index b67e2f9727b..79614fd7068 100644 --- a/lib/lightning_web/live/run_live/index.ex +++ b/lib/lightning_web/live/run_live/index.ex @@ -9,6 +9,7 @@ defmodule LightningWeb.RunLive.Index do alias Lightning.Invocation alias Lightning.Invocation.Step + alias Lightning.Jobs alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.Run @@ -46,7 +47,10 @@ defmodule LightningWeb.RunLive.Index do lost: :boolean, rejected: :boolean, sort_by: :string, - sort_direction: :string + sort_direction: :string, + error_signature_exit_reason: :string, + error_signature_error_type: :string, + error_signature_job_id: :string } @empty_page %{ @@ -182,6 +186,8 @@ defmodule LightningWeb.RunLive.Index do page_title: "History", step: %Step{}, filters_changeset: filters_changeset(filters), + error_signature_job_name: + Jobs.get_job_name(project.id, filters["error_signature_job_id"]), pagination_path: &pagination_path(socket, project, &1, filters), page: @empty_page, async_page: AsyncResult.loading() diff --git a/lib/lightning_web/live/run_live/index.html.heex b/lib/lightning_web/live/run_live/index.html.heex index 7318cd0b055..bc0fe2f6e21 100644 --- a/lib/lightning_web/live/run_live/index.html.heex +++ b/lib/lightning_web/live/run_live/index.html.heex @@ -256,6 +256,30 @@ Work order: {display_short_uuid(workorder_id)} <% end %> + + <%!-- Error signature chip (only when the signature filter is active) --%> + <% exit_reason = + get_change(@filters_changeset, :error_signature_exit_reason) %> + <%= if exit_reason do %> + <% error_type = + get_change(@filters_changeset, :error_signature_error_type) + + job_id = get_change(@filters_changeset, :error_signature_job_id) %> + <.filter_chip + id="error-signature-filter-chip" + active={true} + clear_fields={[ + {:error_signature_exit_reason, nil}, + {:error_signature_error_type, nil}, + {:error_signature_job_id, nil} + ]} + > + {exit_reason}{if error_type, + do: ":#{error_type}"}{if job_id, + do: + " @ #{@error_signature_job_name || display_short_uuid(job_id)}"} + + <% end %> diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index 65c146970bb..cddc51ddfa9 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -115,7 +115,7 @@ defmodule LightningWeb.Router do get "/projects/:project_id/workflows/:workflow_id/health/failures", API.WorkflowHealthController, - :failure_signatures + :error_signatures end ## Collections diff --git a/test/lightning/invocation_test.exs b/test/lightning/invocation_test.exs index 873a637aecb..af40cee29fe 100644 --- a/test/lightning/invocation_test.exs +++ b/test/lightning/invocation_test.exs @@ -1030,6 +1030,189 @@ defmodule Lightning.InvocationTest do end end + # The triage row's "View" button: `filter_by_error_signature/2` inside + # `search_workorders_query/2`. Exercised through `search_workorders_for_export_query/2` + # since it applies no destructive-action side filter of its own. + describe "filter_by_error_signature/2" do + defp signature_params(exit_reason, error_type, job_id) do + SearchParams.new(%{ + "status" => SearchParams.status_list(), + "error_signature_exit_reason" => exit_reason, + "error_signature_error_type" => error_type, + "error_signature_job_id" => job_id + }) + end + + defp signature_matches(project, exit_reason, error_type, job_id \\ nil) do + project + |> Invocation.search_workorders_for_export_query( + signature_params(exit_reason, error_type, job_id) + ) + |> Repo.all() + |> Enum.map(& &1.id) + |> MapSet.new() + end + + defp workorder(workflow, trigger, state) do + insert(:workorder, + workflow: workflow, + trigger: trigger, + dataclip: insert(:dataclip), + state: state + ) + end + + # The run carries the work order's own state: every case here is a work + # order that ran once and stopped there. + defp ran_wo(workflow, trigger, state, steps \\ []) do + wo = workorder(workflow, trigger, state) + + insert(:run, + work_order: wo, + starting_trigger: trigger, + dataclip: insert(:dataclip), + state: state, + steps: steps + ) + + wo + end + + defp failing_step(job, error_type) do + build(:step, job: job, exit_reason: "fail", error_type: error_type) + end + + test "matches exactly the work orders behind a step-level signature, and none of a rejected or lost row" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger, job: job} = + build_workflow(project: project) + + other_job = insert(:job, workflow: workflow) + + crashed_wo = + ran_wo(workflow, trigger, :crashed, [failing_step(job, "RuntimeError")]) + + failed_wo = + ran_wo(workflow, trigger, :failed, [failing_step(job, "RuntimeError")]) + + # Same reason and error type, different job — must not match. + ran_wo(workflow, trigger, :failed, [ + failing_step(other_job, "RuntimeError") + ]) + + workorder(workflow, trigger, :rejected) + + lost_wo = ran_wo(workflow, trigger, :lost) + + assert signature_matches(project, "fail", "RuntimeError", job.id) == + MapSet.new([crashed_wo.id, failed_wo.id]) + + # `"rejected"` is `to_signature/2`'s own literal, not a worker reason — + # not present in `Run.state_reasons/0`, so the run-level branch fails + # closed rather than matching the rejected work order. + assert signature_matches(project, "rejected", "RunLimitExceeded") == + MapSet.new() + + # The run-level branch, for a work order whose latest run never + # reached a step. + assert signature_matches(project, "lost", nil) == MapSet.new([lost_wo.id]) + end + + # Mirrors `stats_test.exs`, "treats an empty error type on the run the + # same as a missing one": the row reports `nil`, so the filter has to + # read `""` as `nil` too or the View button lands on an empty page. + test "reads an empty error type on the run as a missing one" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger} = build_workflow(project: project) + + wo = workorder(workflow, trigger, :crashed) + + insert(:run, + work_order: wo, + starting_trigger: trigger, + dataclip: insert(:dataclip), + state: :crashed, + error_type: "" + ) + + assert signature_matches(project, "crash", nil) == MapSet.new([wo.id]) + end + + # Guards every unfiltered history search, not just the View button: the + # signature filter sits in the query behind search, bulk retry, bulk cancel + # and export, and its run-level branch fails closed. A nil signature that + # stopped being a no-op would empty the history page for everyone. + test "with all three fields nil is a no-op" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger} = build_workflow(project: project) + + wo = workorder(workflow, trigger, :failed) + + params = SearchParams.new(%{"status" => SearchParams.status_list()}) + + assert %{ + error_signature_exit_reason: nil, + error_signature_error_type: nil, + error_signature_job_id: nil + } = params + + found = + project + |> Invocation.search_workorders_for_export_query(params) + |> Repo.all() + |> Enum.map(& &1.id) + + assert found == [wo.id] + end + + test "search_workorders_for_retry/2 scopes a bulk retry to the signature" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger, job: job} = + build_workflow(project: project) + + matching_wo = + ran_wo(workflow, trigger, :failed, [failing_step(job, "RuntimeError")]) + + ran_wo(workflow, trigger, :failed, [failing_step(job, "CompileError")]) + + found = + Invocation.search_workorders_for_retry( + project, + signature_params("fail", "RuntimeError", job.id) + ) + + assert [matching_wo.id] == Enum.map(found, & &1.id) + end + + test "a nil error_type filter matches a step whose own error_type is the empty string" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger, job: job} = + build_workflow(project: project) + + wo = ran_wo(workflow, trigger, :failed, [failing_step(job, "")]) + + assert signature_matches(project, "fail", nil, job.id) == + MapSet.new([wo.id]) + end + + test "a successful work order is not matched, even when its latest run holds a failing step" do + project = insert(:project) + + %{workflow: workflow, trigger: trigger, job: job} = + build_workflow(project: project) + + ran_wo(workflow, trigger, :success, [failing_step(job, "RuntimeError")]) + + assert signature_matches(project, "fail", "RuntimeError", job.id) == + MapSet.new() + end + end + describe "search_workorders/1" do test "returns workorders ordered inserted at desc, with nulls first" do project = insert(:project) diff --git a/test/lightning/workflows/stats_test.exs b/test/lightning/workflows/stats_test.exs index ccfa11711e5..ebc76735ce5 100644 --- a/test/lightning/workflows/stats_test.exs +++ b/test/lightning/workflows/stats_test.exs @@ -125,7 +125,7 @@ defmodule Lightning.Workflows.StatsTest do insert_run(workflow, trigger, :cancelled) assert %{cancelled: 1, failed: 0} = Stats.outcomes(workflow).counts - assert %{signatures: []} = Stats.failure_signatures(workflow) + assert %{signatures: []} = Stats.error_signatures(workflow) end # The review comment this whole unit change is for: retrying a failure until @@ -147,7 +147,7 @@ defmodule Lightning.Workflows.StatsTest do end assert %{success: 1, failed: 0} = Stats.outcomes(workflow).counts - assert %{signatures: []} = Stats.failure_signatures(workflow) + assert %{signatures: []} = Stats.error_signatures(workflow) end test "ignores work orders belonging to another workflow", %{ @@ -183,7 +183,7 @@ defmodule Lightning.Workflows.StatsTest do assert Stats.outcomes(workflow) == first end - describe "failure_signatures/2" do + describe "error_signatures/2" do defp failed_run(workflow, trigger, attrs, steps \\ []) do {wo_attrs, run_attrs} = Keyword.split(attrs, [:last_activity]) state = Keyword.get(run_attrs, :state, :failed) @@ -239,12 +239,13 @@ defmodule Lightning.Workflows.StatsTest do step(job, exit_reason: "fail", error_type: "RuntimeError") ]) - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert signature == %{ count: 1, exit_reason: "fail", error_type: "RuntimeError", + job_id: job.id, step_name: job.name, adaptor: job.adaptor } @@ -270,12 +271,47 @@ defmodule Lightning.Workflows.StatsTest do }) |> Repo.update!() - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert signature.step_name == job.name assert signature.adaptor == job.adaptor end + # The row is keyed by `job_id`, not by the resolved name — that is what + # keeps a job renamed mid-window as one row instead of splitting into a + # before-rename row and an after-rename row. The label comes off the + # newer of the two snapshots, since that is the name the job actually has + # by the time anyone triages it. + test "merges a job renamed mid-window into one row, labelled with the newer name", + %{workflow: workflow, trigger: trigger} do + job = hd(workflow.jobs) + job_id = job.id + + failed_run(workflow, trigger, [], [ + step(job, exit_reason: "fail", error_type: "RuntimeError") + ]) + + job + |> Ecto.Changeset.change(name: "Renamed") + |> Repo.update!() + + # `current_snapshot/1` only creates a new snapshot when none exists for + # the workflow's current `lock_version`, so the rename alone would still + # resolve against the snapshot already made above. Bumping it forces a + # second snapshot — the newer one the label tiebreak has to pick. + workflow + |> Ecto.Changeset.change(lock_version: workflow.lock_version + 1) + |> Repo.update!() + + failed_run(workflow, trigger, [], [ + step(job, exit_reason: "fail", error_type: "RuntimeError") + ]) + + assert %{signatures: [signature]} = Stats.error_signatures(workflow) + + assert %{count: 2, job_id: ^job_id, step_name: "Renamed"} = signature + end + # `mark_steps_lost/1` stamps the step's exit_reason and nothing else, so # the error type has to come off the run or the signature reads `lost:`. test "falls back to the run's error type when the step never reported one", @@ -289,7 +325,7 @@ defmodule Lightning.Workflows.StatsTest do [step(job, exit_reason: "lost", error_type: nil)] ) - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert signature.exit_reason == "lost" assert signature.error_type == "LostAfterStart" assert signature.step_name == job.name @@ -301,12 +337,13 @@ defmodule Lightning.Workflows.StatsTest do } do failed_run(workflow, trigger, state: :crashed, error_type: "CompileError") - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert signature == %{ count: 1, exit_reason: "crash", error_type: "CompileError", + job_id: nil, step_name: nil, adaptor: nil } @@ -321,17 +358,53 @@ defmodule Lightning.Workflows.StatsTest do } do work_order(workflow, trigger, state: :rejected) - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert signature == %{ count: 1, exit_reason: "rejected", error_type: "RunLimitExceeded", + job_id: nil, step_name: nil, adaptor: nil } end + # `"" || x` returns `""` in Elixir, so an empty error type would otherwise + # coalesce to itself instead of falling through — splitting one signature + # into two rows that render identically and each half the count. + test "treats an empty error type the same as a missing one", %{ + workflow: workflow, + trigger: trigger + } do + job = hd(workflow.jobs) + + failed_run(workflow, trigger, [], [ + step(job, exit_reason: "fail", error_type: "") + ]) + + failed_run(workflow, trigger, [], [ + step(job, exit_reason: "fail", error_type: nil) + ]) + + assert %{signatures: [%{count: 2, error_type: nil}]} = + Stats.error_signatures(workflow) + end + + # And on the run's own error type, which the step falls through to: it is + # read straight into the signature, so an empty one splits the rows there + # instead. + test "treats an empty error type on the run the same as a missing one", %{ + workflow: workflow, + trigger: trigger + } do + failed_run(workflow, trigger, state: :crashed, error_type: "") + failed_run(workflow, trigger, state: :crashed, error_type: nil) + + assert %{signatures: [%{count: 2, exit_reason: "crash", error_type: nil}]} = + Stats.error_signatures(workflow) + end + test "groups matching work orders and sorts the heaviest first", %{ workflow: workflow, trigger: trigger @@ -347,7 +420,7 @@ defmodule Lightning.Workflows.StatsTest do failed_run(workflow, trigger, state: :crashed, error_type: "CompileError") assert %{signatures: [first, second]} = - Stats.failure_signatures(workflow) + Stats.error_signatures(workflow) assert %{count: 3, error_type: "RuntimeError"} = first assert %{count: 1, error_type: "CompileError"} = second @@ -380,7 +453,7 @@ defmodule Lightning.Workflows.StatsTest do run.(job_a, "FirstAttempt", DateTime.add(now, -60)) run.(job_b, "Retry", now) - assert %{signatures: [signature]} = Stats.failure_signatures(workflow) + assert %{signatures: [signature]} = Stats.error_signatures(workflow) assert %{count: 1, error_type: "Retry", step_name: name} = signature assert name == job_b.name end @@ -408,7 +481,7 @@ defmodule Lightning.Workflows.StatsTest do ) ]) - assert %{signatures: signatures} = Stats.failure_signatures(workflow) + assert %{signatures: signatures} = Stats.error_signatures(workflow) assert [ %{count: 1, error_type: "Earlier", step_name: job_a.name}, @@ -433,7 +506,7 @@ defmodule Lightning.Workflows.StatsTest do ]) assert %{signatures: [%{count: 1, error_type: "RuntimeError"}]} = - Stats.failure_signatures(workflow) + Stats.error_signatures(workflow) end test "ignores steps that succeeded and work orders that succeeded", %{ @@ -456,7 +529,7 @@ defmodule Lightning.Workflows.StatsTest do insert_run(workflow, trigger, :success) assert %{signatures: [%{count: 1, error_type: "RuntimeError"}]} = - Stats.failure_signatures(workflow) + Stats.error_signatures(workflow) end test "still finds a failing step that never stamped a start time", %{ @@ -471,7 +544,7 @@ defmodule Lightning.Workflows.StatsTest do ]) assert %{signatures: [%{error_type: "RuntimeError", step_name: name}]} = - Stats.failure_signatures(workflow) + Stats.error_signatures(workflow) assert name == job.name end @@ -488,7 +561,7 @@ defmodule Lightning.Workflows.StatsTest do last_activity: days_ago(31) ) - assert %{signatures: []} = Stats.failure_signatures(workflow) + assert %{signatures: []} = Stats.error_signatures(workflow) end defp two_jobs(workflow) do @@ -511,7 +584,7 @@ defmodule Lightning.Workflows.StatsTest do # deleting a hardcoded list of keys. Stats.outcomes(workflow) Stats.outcomes(workflow, 7) - Stats.failure_signatures(workflow) + Stats.error_signatures(workflow) other = insert(:simple_workflow) Stats.outcomes(other) diff --git a/test/lightning/workorders/search_params_test.exs b/test/lightning/workorders/search_params_test.exs index 5e4414545f9..2b0410cdfda 100644 --- a/test/lightning/workorders/search_params_test.exs +++ b/test/lightning/workorders/search_params_test.exs @@ -40,6 +40,34 @@ defmodule Lightning.WorkOrders.SearchParamsTest do workflow_id: "babd29f7-bf15-4a66-af21-51209217ebd4" } == SearchParams.new(params) end + + # A server-built link naming a workflow and a date carries no search-field + # flags. Reading that as "search nothing" makes the search box on the + # landed page match nothing at all. + test "falls back to every search field when the params name none of them" do + params = + SearchParams.new(%{ + "workflow_id" => "babd29f7-bf15-4a66-af21-51209217ebd4", + "search_term" => "hello" + }) + + assert params.search_fields == [:id, :body, :log, :dataclip_name] + end + + # And an unticked box still arrives, marked false, so turning them all off + # is a different thing from a link that never mentioned them. + test "searches nothing when every search field is present and false" do + params = + SearchParams.new(%{ + "id" => "false", + "body" => "false", + "log" => "false", + "dataclip_name" => "false", + "search_term" => "hello" + }) + + assert params.search_fields == [] + end end describe "from_map/1" do diff --git a/test/lightning_web/live/run_live/index_test.exs b/test/lightning_web/live/run_live/index_test.exs index 886ffe9a4e8..6305e5c43f1 100644 --- a/test/lightning_web/live/run_live/index_test.exs +++ b/test/lightning_web/live/run_live/index_test.exs @@ -971,6 +971,84 @@ defmodule LightningWeb.RunLive.IndexTest do chip = element(view, "#workorder-id-filter-chip") assert render(chip) =~ "Work order:" end + + test "error signature filter chip appears when the filter is set", %{ + conn: conn, + project: project, + jobs: [job | _] + } do + {:ok, view, _html} = + live_async( + conn, + Routes.project_run_index_path(conn, :index, project.id, + filters: %{ + error_signature_exit_reason: "fail", + error_signature_error_type: "AdaptorError", + error_signature_job_id: job.id + } + ) + ) + + assert has_element?(view, "#error-signature-filter-chip") + chip = element(view, "#error-signature-filter-chip") + + assert render(chip) =~ "fail:AdaptorError @ #{job.name}" + end + + # The name is read back from the id, so the read is scoped to the project + # the page is on — a hand-edited id from elsewhere names nothing here. + test "error signature filter chip falls back to the id for a job outside the project", + %{conn: conn, project: project} do + other_job = insert(:job, workflow: build(:workflow)) + + {:ok, view, _html} = + live_async( + conn, + Routes.project_run_index_path(conn, :index, project.id, + filters: %{ + error_signature_exit_reason: "fail", + error_signature_job_id: other_job.id + } + ) + ) + + chip = render(element(view, "#error-signature-filter-chip")) + + refute chip =~ other_job.name + + assert chip =~ + LightningWeb.LiveHelpers.display_short_uuid(other_job.id) + end + + test "error signature filter chip omits error type and job id when absent", + %{ + conn: conn, + project: project + } do + {:ok, view, _html} = + live_async( + conn, + Routes.project_run_index_path(conn, :index, project.id, + filters: %{error_signature_exit_reason: "lost"} + ) + ) + + chip = element(view, "#error-signature-filter-chip") + html = render(chip) + assert html =~ "lost" + refute html =~ "@" + end + + test "error signature filter chip is absent when the filter is not set", + %{conn: conn, project: project} do + {:ok, view, _html} = + live_async( + conn, + Routes.project_run_index_path(conn, :index, project.id) + ) + + refute has_element?(view, "#error-signature-filter-chip") + end end describe "cancel work orders" do
Work orders @@ -84,29 +97,45 @@ export const TriageTable = ({ signatures, emptyMessage }: TriageTableProps) => { Signature + Actions +
{signature.count.toLocaleString()} +

Tip: {tipFor(signature)}

+ {signature.exit_reason && ( + + )} +