diff --git a/generator/tests/test_list_recent_runs.py b/generator/tests/test_list_recent_runs.py new file mode 100644 index 00000000..3b53ce34 --- /dev/null +++ b/generator/tests/test_list_recent_runs.py @@ -0,0 +1,161 @@ +"""Tests for `plugins/tend-ci-runner/scripts/list-recent-runs.sh`. + +The script decides which CI runs a `review-reviewers`/`review-runs` session +analyzes, so a wrong window is invisible in the output — it reads as a normal +run list. It isn't part of the generator package; the test lives here because +this is the repo's only Python suite, and shellcheck can't catch window +arithmetic. +""" + +from __future__ import annotations + +import json +import subprocess +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +LIST_RECENT_RUNS = ( + REPO_ROOT / "plugins" / "tend-ci-runner" / "scripts" / "list-recent-runs.sh" +) + +# `gh` stand-in covering the three calls the script makes: the workflow +# discovery list, the previous-run window anchor (the only `run list` carrying +# `--status`), and the per-workflow run list. +FAKE_GH = """#!/usr/bin/env bash +case "$1 $2" in + "workflow list") echo '[{"name":"tend-review"}]' ;; + "run list") + if [[ "$*" == *"--status"* ]]; then + echo "$FAKE_PREV_RUN_STARTED_AT" + else + cat "$FAKE_RUNS_JSON" + fi + ;; + *) echo "unexpected gh invocation: $*" >&2; exit 1 ;; +esac +""" + + +def _iso(moment: datetime) -> str: + return moment.strftime("%Y-%m-%dT%H:%M:%SZ") + + +def _intended_tick(now: datetime, cron_minute: int) -> datetime: + """The tick the script anchors on, by the same rule the script uses.""" + this_hour_tick = now.replace(minute=cron_minute, second=0, microsecond=0) + if now < this_hour_tick: + return this_hour_tick - timedelta(hours=1) + return this_hour_tick + + +@pytest.fixture +def harness(tmp_path: Path): + """Fake `gh` on PATH plus a builder for the Actions env the script reads.""" + bindir = tmp_path / "fakebin" + bindir.mkdir() + gh = bindir / "gh" + gh.write_text(FAKE_GH) + gh.chmod(0o755) + + def run(schedule: str | None, runs: list[dict], prev_started_at: str = "") -> list: + runs_json = tmp_path / "runs.json" + runs_json.write_text(json.dumps(runs)) + + env = { + "PATH": f"{bindir}:/usr/bin:/bin", + "FAKE_RUNS_JSON": str(runs_json), + "FAKE_PREV_RUN_STARTED_AT": prev_started_at, + "GITHUB_WORKFLOW": "review-reviewers", + "GITHUB_RUN_ID": "999", + } + if schedule is not None: + event = tmp_path / "event.json" + event.write_text(json.dumps({"schedule": schedule})) + env["GITHUB_EVENT_PATH"] = str(event) + + result = subprocess.run( + ["bash", str(LIST_RECENT_RUNS), "tend-"], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + return sorted(entry["databaseId"] for entry in json.loads(result.stdout)) + + return run + + +def _run_record(run_id: int, finished: datetime, conclusion: str | None = "success"): + return { + "databaseId": run_id, + "conclusion": conclusion, + "createdAt": _iso(finished - timedelta(minutes=5)), + "updatedAt": _iso(finished), + } + + +def test_window_is_half_open_at_the_cron_tick(harness): + """A run that finished after the tick belongs to the next cycle, not this one. + + The floor advances by exactly one period per cycle, so a window with no + ceiling is as much wider than a period as the scheduler is late, and the + next cycle lists that tail again — a second agent survey of runs already + analyzed. + """ + now = datetime.now(UTC) + # Put the tick ~30 min from now in either direction, so the test can't race + # a tick rollover between computing fixtures and the script reading its own + # clock. + cron_minute = (now.minute + 30) % 60 + tick = _intended_tick(now, cron_minute) + + listed = harness( + f"{cron_minute} * * * *", + [ + _run_record(1, tick - timedelta(hours=1, minutes=5)), # before floor + _run_record(2, tick - timedelta(minutes=59)), # in window + _run_record(3, tick - timedelta(minutes=2)), # in window + _run_record(4, tick + timedelta(minutes=2)), # after tick + _run_record(5, tick - timedelta(minutes=1), conclusion=None), # running + ], + prev_started_at=_iso(tick - timedelta(minutes=59, seconds=30)), + ) + + assert listed == [2, 3] + + +def test_next_cycle_picks_up_what_the_ceiling_deferred(harness): + """The deferred run is the next cycle's first item — deferred, not dropped.""" + now = datetime.now(UTC) + cron_minute = (now.minute + 30) % 60 + tick = _intended_tick(now, cron_minute) + # A run that finished just after the *previous* tick: the ceiling kept it + # out of that cycle, and this cycle's floor is exactly that tick. + deferred = tick - timedelta(hours=1) + timedelta(minutes=2) + + listed = harness( + f"{cron_minute} * * * *", + [_run_record(6, deferred)], + prev_started_at=_iso(tick - timedelta(minutes=59, seconds=30)), + ) + + assert listed == [6] + + +def test_non_periodic_cron_keeps_the_now_anchored_window_uncapped(harness): + """Off the cron path there is no next cycle to hand a tail to.""" + now = datetime.now(UTC) + + listed = harness( + "*/15 * * * *", # no constant period — falls back to now-anchored 1h + [ + _run_record(7, now - timedelta(minutes=10)), + _run_record(8, now - timedelta(hours=2)), + ], + ) + + assert listed == [7] diff --git a/plugins/tend-ci-runner/scripts/list-recent-runs.sh b/plugins/tend-ci-runner/scripts/list-recent-runs.sh index c16ffa30..0e34ba2f 100755 --- a/plugins/tend-ci-runner/scripts/list-recent-runs.sh +++ b/plugins/tend-ci-runner/scripts/list-recent-runs.sh @@ -9,15 +9,16 @@ # # Window anchor: when invoked under a scheduled workflow with a simple # hourly cron (`MM * * * *`), the completion window is anchored to the most -# recent intended cron tick instead of `now`. Consecutive cycles then tile -# exactly: [intended-1h, intended], then [intended, intended+1h]. Without -# this, GHA scheduler delay (20-40 min during peak hours) shifts each -# cycle's window relative to actual start time and drops runs that finished -# in the slack between consecutive actual starts. When GHA *drops* a tick -# entirely (not just delays it), the window's floor is instead pulled back to -# the previous actual run's intended tick so the orphaned hour still gets -# analyzed. For non-schedule events or non-hourly crons, falls back to a -# now-anchored 1h window. +# recent intended cron tick instead of `now`: the half-open interval +# [intended-1h, intended). Consecutive cycles then tile exactly, the next +# one covering [intended, intended+1h). Without this, GHA scheduler delay +# (20-40 min during peak hours) shifts each cycle's window relative to +# actual start time and drops runs that finished in the slack between +# consecutive actual starts. When GHA *drops* a tick entirely (not just +# delays it), the window's floor is instead pulled back to the previous +# actual run's intended tick so the orphaned hour still gets analyzed. For +# non-schedule events or non-hourly crons, falls back to a now-anchored 1h +# window with no ceiling. # # Environment variables: # TARGET_REPO - Query a different repo (default: current repo) @@ -97,6 +98,16 @@ if [ -n "$cron_minute" ]; then # Default floor: one cron period back. Consecutive ticks tile exactly. COMPLETED_AFTER=$((intended - 3600)) + # Ceiling: the tick itself, exclusive. The floor advances by exactly one + # period per cycle, so a window with no ceiling runs floor..now and is + # wider than a period by however late the scheduler was — and the next + # cycle, whose floor is this tick, lists that tail again. The caller pays a + # second full agent survey for runs it already analyzed, and scheduler delay + # against an hourly cron is routinely tens of minutes, so that share is + # large. Runs finishing after the tick aren't dropped: the next cycle's + # floor is exactly this tick, so they are the first thing it sees. + COMPLETED_BEFORE=$intended + # Dropped-tick recovery. GHA doesn't only *delay* scheduled ticks, it also # *drops* them: a tick that fires zero times leaves that hour's completions # in the gap between the previous and next cycle's windows (the skipped-tick @@ -134,6 +145,9 @@ if [ -n "$cron_minute" ]; then else CREATED_SINCE=$(date -d '3 hours ago' +%Y-%m-%dT%H:%M:%S) COMPLETED_AFTER=$(date -d '1 hour ago' +%s) + # No ceiling off the cron path: a now-anchored window has no next cycle to + # hand the tail to, and a hand-dispatched run is asking about right now. + COMPLETED_BEFORE="" fi all_runs="[]" @@ -151,10 +165,14 @@ for wf in "${WORKFLOWS[@]}"; do all_runs=$(echo "$all_runs" "$runs" | jq -s 'add') done -# Filter: drop in-progress (empty conclusion), keep only recently finished -echo "$all_runs" | jq --argjson cutoff "$COMPLETED_AFTER" ' +# Filter: drop in-progress (empty conclusion), keep only runs that finished +# inside the window — [cutoff, ceiling), half-open so consecutive cycles tile +# without re-listing the boundary run. A null ceiling means unbounded. +echo "$all_runs" | jq --argjson cutoff "$COMPLETED_AFTER" \ + --argjson ceiling "${COMPLETED_BEFORE:-null}" ' [ .[] | select(.conclusion != null and .conclusion != "") | select((.updatedAt | fromdateiso8601) >= $cutoff) + | select($ceiling == null or (.updatedAt | fromdateiso8601) < $ceiling) ] '