From b997a28c533283458a69cd5cd2096c35d203d2ca Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:10:43 +0000 Subject: [PATCH] fix(ci): report budgets the startup guard cannot resolve The timeout contract check skipped a job whenever either budget came from a `with:` value it could not parse, or from a matrix column no `include` row supplied as a number. Both paths produced no pairs and no errors, so the guard printed "invariants hold" for a caller whose budgets were never compared at all. A caller reading `${{ matrix.timeout }}` off a mistyped column while capping the job at 1 minute passed clean. Unresolvable budgets now come back as the reason they could not be read and are reported as violations, which is the whole point of a guard built to catch checks that silently do not run. `Column` tags a matrix reference so it stays distinguishable from that reason string, and the report names only the columns that resolve nowhere, since a column every row supplies is not what left the pair unchecked. --- .../check_workflow_startup_safety.py | 99 ++++++++++++++----- 1 file changed, 75 insertions(+), 24 deletions(-) diff --git a/tests/code_coverage_tests/check_workflow_startup_safety.py b/tests/code_coverage_tests/check_workflow_startup_safety.py index a7192dcad20..cf150daef4c 100644 --- a/tests/code_coverage_tests/check_workflow_startup_safety.py +++ b/tests/code_coverage_tests/check_workflow_startup_safety.py @@ -16,12 +16,15 @@ because CI cannot enforce them on itself. the test budget plus the setup ceilings plus the runner overhead below. Otherwise the job deadline preempts pytest inside its own advertised budget, which is the failure the split timeouts exist to prevent, and it shows up as - a cancelled shard whose tests were passing. + a cancelled shard whose tests were passing. A budget this check cannot resolve + is reported rather than skipped, so a mistyped input or matrix column surfaces + here instead of leaving the pair silently unchecked. """ import re import sys from collections.abc import Iterator, Mapping, Sequence +from dataclasses import dataclass from pathlib import Path from typing import Final @@ -92,8 +95,19 @@ def base_default(base_text: str, name: str) -> int: return base[True]["workflow_call"]["inputs"][name]["default"] -def budget_source(job: ReusableCall, key: str, fallback: int) -> int | str | None: - """A caller passes a literal, or `${{ matrix.x }}` naming a column of its matrix.""" +@dataclass(frozen=True, slots=True) +class Column: + """A budget the caller reads from one column of its own matrix.""" + + name: str + + +def budget_source(job: ReusableCall, key: str, fallback: int) -> int | Column | str: + """A caller passes a literal, or `${{ matrix.x }}` naming a column of its matrix. + + Anything else comes back as the reason it could not be read, since a budget + nothing can resolve has to be reported rather than passed over. + """ value: Final = job.with_.get(key) if value is None: return fallback @@ -101,7 +115,9 @@ def budget_source(job: ReusableCall, key: str, fallback: int) -> int | str | Non return value matrix_ref: Final = MATRIX_REF.match(str(value)) - return matrix_ref.group("key") if matrix_ref else None + if not matrix_ref: + return f"passes `{key}: {value}`, which is neither a number nor a `matrix` reference." + return Column(matrix_ref.group("key")) def matrix_rows(job: ReusableCall) -> Sequence[Mapping[str, object]]: @@ -110,7 +126,7 @@ def matrix_rows(job: ReusableCall) -> Sequence[Mapping[str, object]]: return tuple(e for e in entries if isinstance(e, dict)) -def budget_pairs(job: ReusableCall, test_source: int | str, job_source: int | str) -> Iterator[tuple[int, int]]: +def budget_pairs(job: ReusableCall, test_source: int | Column, job_source: int | Column) -> Iterator[tuple[int, int]]: """Pair each shard's test budget with the job budget of that same shard. Matrix-sourced budgets resolve per `include` row, so two matrix columns are @@ -121,31 +137,66 @@ def budget_pairs(job: ReusableCall, test_source: int | str, job_source: int | st return for row in matrix_rows(job): - test_budget = row.get(test_source) if isinstance(test_source, str) else test_source - job_budget = row.get(job_source) if isinstance(job_source, str) else job_source + test_budget = row.get(test_source.name) if isinstance(test_source, Column) else test_source + job_budget = row.get(job_source.name) if isinstance(job_source, Column) else job_source if isinstance(test_budget, int) and isinstance(job_budget, int): yield test_budget, job_budget +def unresolved_message(where: str, job: ReusableCall, sources: Sequence[int | Column]) -> str: + """Why no shard yielded a pair of budgets to compare. + + Naming only the columns that resolve nowhere keeps the message honest: a + column every row supplies is not what left the pair unchecked. + """ + rows: Final = matrix_rows(job) + missing: Final = tuple( + f"`matrix.{s.name}`" + for s in sources + if isinstance(s, Column) and not any(isinstance(row.get(s.name), int) for row in rows) + ) + if missing: + return ( + f"{where} reads a budget from {', '.join(missing)}, which no `include` row supplies " + "as a number, so the pair would go unchecked." + ) + return ( + f"{where} reads both budgets from its matrix, but no single `include` row supplies both " + "as numbers, so the pair would go unchecked." + ) + + +def job_errors(rel: Path, job_name: str, job: ReusableCall, ceiling: int, base_text: str) -> Iterator[str]: + where: Final = f"{rel}: job `{job_name}`" + test_source: Final = budget_source(job, "timeout-minutes", base_default(base_text, "timeout-minutes")) + job_source: Final = budget_source(job, "job-timeout-minutes", base_default(base_text, "job-timeout-minutes")) + sources: Final = (test_source, job_source) + + unreadable: Final = tuple(f"{where} {reason}" for reason in sources if isinstance(reason, str)) + if unreadable: + yield from unreadable + return + + pairs: Final = tuple(budget_pairs(job, test_source, job_source)) + if not pairs: + yield unresolved_message(where, job, sources) + return + + for test_budget, job_budget in pairs: + required = test_budget + ceiling + JOB_OVERHEAD_MINUTES + if job_budget < required: + yield ( + f"{where} gives pytest {test_budget}m but caps the job at " + f"{job_budget}m. Setup can use up to {ceiling}m plus {JOB_OVERHEAD_MINUTES}m of " + f"runner overhead, so the job deadline would preempt pytest; raise " + f"job-timeout-minutes to at least {required}." + ) + + def timeout_contract_errors(rel: Path, workflow: WorkflowFile, ceiling: int, base_text: str) -> Iterator[str]: for job_name, job in workflow.jobs.items(): - if job.uses != BASE_WORKFLOW: - continue - - test_source: Final = budget_source(job, "timeout-minutes", base_default(base_text, "timeout-minutes")) - job_source: Final = budget_source(job, "job-timeout-minutes", base_default(base_text, "job-timeout-minutes")) - if test_source is None or job_source is None: - continue - - for test_budget, job_budget in budget_pairs(job, test_source, job_source): - required = test_budget + ceiling + JOB_OVERHEAD_MINUTES - if job_budget < required: - yield ( - f"{rel}: job `{job_name}` gives pytest {test_budget}m but caps the job at " - f"{job_budget}m. Setup can use up to {ceiling}m plus {JOB_OVERHEAD_MINUTES}m of " - f"runner overhead, so the job deadline would preempt pytest; raise " - f"job-timeout-minutes to at least {required}." - ) + if job.uses == BASE_WORKFLOW: + yield from job_errors(rel, job_name, job, ceiling, base_text) def workflow_errors(rel: Path, text: str, ceiling: int, base_text: str) -> Iterator[str]: