diff --git a/generator/tests/test_shared_steps.py b/generator/tests/test_shared_steps.py index 88e18971..dc12a603 100644 --- a/generator/tests/test_shared_steps.py +++ b/generator/tests/test_shared_steps.py @@ -1699,6 +1699,11 @@ def test_notifications_check_counts_from_the_snapshot_when_the_recount_fails( OUTAGE_TITLE = "Bot temporarily unavailable" OUTAGE_LABEL = "tend-outage" +# The created_at the fake stamps on a row as it posts it. Later than every +# seeded comment below, so the reconcile's earliest-wins keeper is the seeded +# one and the row just posted is the duplicate. +POSTED_AT = "2026-01-02T12:00:00Z" + # `gh` stand-in for the outage reporter. Same shape as the rate-limit fake — # fixtures in, the script's own `--jq` doing the filtering — plus it captures # comment bodies, which arrive on stdin (`-F -`) rather than in the args. @@ -1706,12 +1711,22 @@ def test_notifications_check_counts_from_the_snapshot_when_the_recount_fails( printf '%s\n' "$*" >> "$GH_CALLS" jq_expr="" +slurp="" prev="" for arg in "$@"; do [ "$prev" = "--jq" ] && jq_expr="$arg" + [ "$arg" = "--slurp" ] && slurp=1 prev="$arg" done +# Real `gh` refuses the combination outright and exits 1, and the comment +# reconcile's shape rests on that: fold the filter back into `--jq` and the +# script dies under pipefail just after posting its row, never reconciling. +if [ -n "$slurp" ] && [ -n "$jq_expr" ]; then + echo "the --slurp option is not supported with --jq or --template" >&2 + exit 1 +fi + emit() { if [ -n "$jq_expr" ]; then printf '%s' "$1" | jq -r "$jq_expr" @@ -1733,16 +1748,43 @@ def test_notifications_check_counts_from_the_snapshot_when_the_recount_fails( "issue create") echo "https://github.com/owner/repo/issues/${FAKE_NEW_ISSUE}" ;; "issue view") emit "$(cat "$KEEPER_JSON")" ;; "issue comment") - cat >> "$COMMENT_BODIES" + body=$(cat) + printf '%s\n' "$body" >> "$COMMENT_BODIES" # Bare `[ -n ... ] && exit 1` would leave the failing test as the case - # body's status and so fail every call; keep it an `if`. + # body's status and so fail every call; keep it an `if`. Fails before the + # row lands in the comment list: a post that 5xx'd left no comment behind. if [ -n "${FAIL_ISSUE_COMMENT:-}" ]; then exit 1; fi + # Land it in the comment list too, so the reconcile that runs straight + # after sees the row this call just posted. + jq -c --arg b "$body" --arg t "$POSTED_AT" \ + '. + [{id: ((map(.id) | max // 0) + 1), created_at: $t, body: $b}]' \ + "$ISSUE_COMMENTS_JSON" > "$ISSUE_COMMENTS_JSON.tmp" + mv "$ISSUE_COMMENTS_JSON.tmp" "$ISSUE_COMMENTS_JSON" ;; "issue close" | "label create") ;; *) - case "$2" in - # The reconciler's primary-key probe. - repos/*/issues/*) + # Matched against the whole arg list, not "$2": these calls carry flags + # (`--paginate`, `-X DELETE`) where the path would otherwise sit. + case "$*" in + *"/comments?per_page=100"*) + if [ -n "${FAIL_COMMENT_LIST:-}" ]; then + echo "gh: 502 server error" >&2 + exit 1 + fi + # Paged the way the endpoint pages, whether or not the caller asked + # for every page: `--slurp` gets the array of pages, a plain read gets + # the oldest 100 alone. Both go through `emit`, so a caller passing + # `--jq` has its own filter applied to what it actually received. + if [ -n "$slurp" ]; then + emit "$(jq -c '[_nwise(100)]' "$ISSUE_COMMENTS_JSON")" + else + emit "$(jq -c '.[0:100]' "$ISSUE_COMMENTS_JSON")" + fi + ;; + *"-X DELETE"*) ;; + # The reconciler's primary-key probe. Last, so the two paths above — + # whose URLs also contain `/issues/` — are matched first. + *repos/*/issues/*) emit "$(jq -c --argjson n "${2##*/}" \ 'map(select(.number == $n)) | .[0] // {"number":0}' "$PROBE_ISSUES_JSON")" ;; @@ -1768,7 +1810,7 @@ def report_failure_env(tmp_path: Path) -> dict[str, str]: event = tmp_path / "event.json" event.write_text(json.dumps({"pull_request": {"number": 851}})) - for name in ("open-issues.json", "probe-issues.json"): + for name in ("open-issues.json", "probe-issues.json", "issue-comments.json"): (tmp_path / name).write_text("[]") (tmp_path / "keeper.json").write_text('{"body": "", "comments": []}') (tmp_path / "comment-bodies.txt").write_text("") @@ -1779,8 +1821,10 @@ def report_failure_env(tmp_path: Path) -> dict[str, str]: "LIST_CALLS": str(tmp_path / "list-calls"), "OPEN_ISSUES_JSON": str(tmp_path / "open-issues.json"), "PROBE_ISSUES_JSON": str(tmp_path / "probe-issues.json"), + "ISSUE_COMMENTS_JSON": str(tmp_path / "issue-comments.json"), "KEEPER_JSON": str(tmp_path / "keeper.json"), "COMMENT_BODIES": str(tmp_path / "comment-bodies.txt"), + "POSTED_AT": POSTED_AT, "FAKE_NEW_ISSUE": "42", "GITHUB_REPOSITORY": "owner/repo", "GITHUB_SERVER_URL": "https://github.com", @@ -2023,3 +2067,201 @@ def test_run_issue_reconcile_refuses_a_call_with_no_row( assert "the row for this run is required" in result.stderr, result.stderr calls = Path(report_failure_env["GH_CALLS"]) assert not calls.exists(), f"reached gh before refusing: {calls.read_text()}" + + +# --------------------------------------------------------------------------- +# report-failure.sh — the append path, and the per-run comment dedup +# --------------------------------------------------------------------------- + + +def _deleted(env: dict[str, str]) -> list[str]: + """Comment ids the reconcile deleted.""" + return [c.rsplit("/", 1)[-1] for c in _calls(env) if "-X DELETE" in c] + + +def _issue_comments(env: dict[str, str], *comments: dict) -> None: + """Seed the comment list the reconcile reads, oldest-first as the API serves it.""" + Path(env["ISSUE_COMMENTS_JSON"]).write_text(json.dumps(list(comments))) + + +def _comment(number: int, body: str, at: str) -> dict: + return {"id": number, "created_at": at, "body": body} + + +def _filler(count: int, *, first_id: int = 1) -> list[dict]: + """Unrelated comments, none carrying this run's anchor.""" + return [ + _comment(first_id + i, f"nightly enrichment {i}", f"2026-01-01T00:{i:02d}:00Z") + for i in range(count) + ] + + +def _seen_by_the_guard(env: dict[str, str], *bodies: str, body: str = "") -> None: + """What `gh issue view --json body,comments` returns for the tracker.""" + Path(env["KEEPER_JSON"]).write_text( + json.dumps({"body": body, "comments": [{"body": b} for b in bodies]}) + ) + + +def _open_tracker(env: dict[str, str], number: int = 42) -> None: + """An outage tracker already open, so the reporter takes the append path. + + Set per-test rather than in the fixture: the create-path tests above start + from an empty list, and these five need the opposite. + """ + Path(env["OPEN_ISSUES_JSON"]).write_text( + json.dumps([{"number": number, "title": OUTAGE_TITLE}]) + ) + + +@pytest.mark.parametrize( + ("body", "comments"), + [ + pytest.param("", (f"| when | {RUN_LINK} | #851 |",), id="in-a-comment"), + pytest.param(f"| when | {RUN_LINK} | #851 |", (), id="in-the-issue-body"), + ], +) +def test_report_failure_skips_a_run_already_recorded( + report_failure_env: dict[str, str], body: str, comments: tuple[str, ...] +) -> None: + """A leg whose sibling already recorded this run posts nothing. + + This is the guard that collapses the flood: a matrix workflow calls the + script once per leg, every leg sharing one GITHUB_RUN_ID, so without it a + 5-leg matrix leaves 5 comments all citing the same run. The body case is + the first run of an outage: one leg seeds the issue with its row, and the + siblings that follow have no comment to match — only the body. + """ + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, *comments, body=body) + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, result.stderr + assert not _comments(report_failure_env), ( + f"appended a second row for a run already recorded: " + f"{_comments(report_failure_env)!r}" + ) + + +def test_report_failure_appends_a_row_for_an_unrecorded_run( + report_failure_env: dict[str, str], +) -> None: + """The happy path: a run the tracker has not seen still gets its row.""" + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, "some other run's row") + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, result.stderr + assert RUN_LINK in _comments(report_failure_env) + + +def test_report_failure_reconciles_a_racing_leg( + report_failure_env: dict[str, str], +) -> None: + """Two legs that both read the tracker before either posted converge to one row. + + The guard is a check-then-act, so jittered legs can both miss. Every leg + sorts the same list the same way, so each computes the same keeper — the + earliest — and deletes the rest. + """ + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, "nothing recorded yet") + _issue_comments( + report_failure_env, + _comment(1, f"| when | {RUN_LINK} | #851 |", "2026-01-02T11:59:00Z"), + ) + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, result.stderr + assert _deleted(report_failure_env) == ["2"], ( + f"expected the later of the two rows deleted, got " + f"{_deleted(report_failure_env)}" + ) + + +def test_report_failure_reconciles_past_the_first_page( + report_failure_env: dict[str, str], +) -> None: + """The flood the reconcile exists for is exactly where it must paginate. + + Issue comments come back oldest-first, so on a tracker past 100 comments an + unpaginated read returns only the oldest page — the rows this run and its + racing sibling just posted are not in it, and the reconcile no-ops on the + one issue that needed it. + """ + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, "nothing recorded yet") + _issue_comments( + report_failure_env, + *_filler(138), + _comment(139, f"| when | {RUN_LINK} | #851 |", "2026-01-02T11:59:00Z"), + ) + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, result.stderr + assert _deleted(report_failure_env) == ["140"], ( + f"the reconcile did not reach past the first page of comments; deleted " + f"{_deleted(report_failure_env)}" + ) + + +def test_report_failure_survives_a_failed_reconcile_read( + report_failure_env: dict[str, str], +) -> None: + """A 5xx on the reconcile's read must not redden a step whose row landed. + + The reconcile is best-effort cleanup and the last statement in the append + branch, so left bare under `set -eo pipefail` a failed read takes the whole + script's status with it — *after* the write succeeded. That reddens the + `Report failure` step with no annotation naming why, on precisely the job + someone is about to diagnose. Duplicate rows on the tracker are the better + failure: the append immediately above warns and continues for the same + reason. + """ + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, "nothing recorded yet") + report_failure_env["FAIL_COMMENT_LIST"] = "1" + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, ( + f"a failed reconcile read reddened the step; stdout:\n{result.stdout}" + ) + assert "::warning::" in result.stdout, result.stdout + assert RUN_LINK in _comments(report_failure_env), ( + f"lost the row the reconcile was cleaning up after: " + f"{_comments(report_failure_env)!r}" + ) + + +def test_report_failure_leaves_a_human_comment_naming_the_run( + report_failure_env: dict[str, str], +) -> None: + """Only the bot's own generated rows are eligible for deletion. + + The reconcile deletes, so its predicate is the whole protection. Selecting + on the bare run URL would make a person linking the run in discussion — the + normal way an outage gets diagnosed — a duplicate to be removed. + """ + _open_tracker(report_failure_env) + _seen_by_the_guard(report_failure_env, "nothing recorded yet") + _issue_comments( + report_failure_env, + _comment( + 1, + "https://github.com/owner/repo/actions/runs/12345 is the one that failed", + "2026-01-02T11:00:00Z", + ), + ) + + result = _run_report_failure(report_failure_env) + + assert result.returncode == 0, result.stderr + assert not _deleted(report_failure_env), ( + f"deleted a human comment that merely named the run: " + f"{_deleted(report_failure_env)}" + ) diff --git a/shared/steps/lib/run-issue.sh b/shared/steps/lib/run-issue.sh index 4a692402..97563a3b 100644 --- a/shared/steps/lib/run-issue.sh +++ b/shared/steps/lib/run-issue.sh @@ -51,6 +51,17 @@ run_issue_ref() { printf '%s' "${num:+#${num}}" } +# The Run cell's link to this run, as it appears in a row. One definition +# because two things have to agree on it exactly: the row `run_issue_row` +# writes, and the dedup that recognises a row already recorded for this run. +# Whole anchor rather than the bare URL, so a human comment merely mentioning +# the run can't be mistaken for a generated row, and a longer run id carrying +# this one as a prefix can't match it. +run_issue_anchor() { + printf '[workflow run](%s/%s/actions/runs/%s)' \ + "$GITHUB_SERVER_URL" "$GITHUB_REPOSITORY" "$GITHUB_RUN_ID" +} + # One row per incident, in the same table format whether it seeds an issue # body (first one) or is appended as a comment (every later one), so both # render identically. Stamps the time when called — capture it once per run. @@ -60,7 +71,7 @@ run_issue_row() { printf '%s\n%s\n%s' \ "| When | Run | Trigger |" \ "|------|-----|---------|" \ - "| $(date -u +%Y-%m-%dT%H:%M:%SZ) | [workflow run](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}) | ${ref:-N/A} |" + "| $(date -u +%Y-%m-%dT%H:%M:%SZ) | $(run_issue_anchor) | ${ref:-N/A} |" } # Every issue this bot filed under one title carrying one label, lowest number @@ -198,14 +209,15 @@ run_issue_create_and_reconcile() { # row would just duplicate it, differing only in its timestamp. The # cross-workflow race has distinct run ids, so it still carries over. # Anchor on the generated row's run link rather than the bare id, so a human - # comment mentioning the run cannot suppress the row. Read into a variable - # rather than piped to grep, which would close the pipe under `pipefail` and - # could report a match as a read failure. - local seen run_url - run_url="${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" + # comment mentioning the run cannot suppress the row — through + # `run_issue_anchor`, the one definition `run_issue_row` writes, so the row + # and the matcher cannot drift apart. Read into a variable rather than piped + # to grep, which would close the pipe under `pipefail` and could report a + # match as a read failure. + local seen seen=$(gh issue view "$keep" --json body,comments \ --jq '.body + "\n" + ([.comments[].body] | join("\n"))' 2>/dev/null || true) - if ! grep -qF "[workflow run](${run_url})" <<< "$seen"; then + if ! grep -qF "$(run_issue_anchor)" <<< "$seen"; then printf '%s\n' "$row" | gh issue comment "$keep" -F - >&2 || true fi diff --git a/shared/steps/report-failure.sh b/shared/steps/report-failure.sh index eba0f8b1..dfb87512 100755 --- a/shared/steps/report-failure.sh +++ b/shared/steps/report-failure.sh @@ -25,6 +25,7 @@ LABEL="tend-outage" TITLE="Bot temporarily unavailable" ROW=$(run_issue_row) +ANCHOR=$(run_issue_anchor) run_issue_ensure_label "$LABEL" "Tracks bot outage incidents" "d93f0b" @@ -46,6 +47,28 @@ if ! EXISTING=$(run_issue_canonical "$LABEL" open "$TITLE"); then fi if [ -n "$EXISTING" ]; then + # Per-run comment dedup. A matrix workflow invokes this script once per leg, + # every leg sharing one GITHUB_RUN_ID. Without a guard each leg appends its + # own near-identical row and floods the issue — a 5-leg matrix failing during + # an outage leaves 5 comments all citing the same run. Skip if this run is + # already recorded, whether in the issue body (a leg of this same run seeded + # the issue) or in an existing comment. + # + # Read into a variable and match in the shell rather than piping to `grep + # -q`: under `pipefail` grep's exit-on-first-match closes the pipe, and the + # SIGPIPE that `gh` then takes becomes the pipeline's status. That reads as + # "not recorded" on any issue whose text outruns the pipe buffer — i.e. the + # flooded ones this guard exists for. A read that outright fails falls + # through to posting, which risks a duplicate row rather than losing one. + RECORDED=$(gh issue view "$EXISTING" --json body,comments \ + --jq '.body + "\n" + ([.comments[].body] | join("\n"))' || true) + case "$RECORDED" in + *"$ANCHOR"*) + echo "Run ${GITHUB_RUN_ID} already recorded on #${EXISTING} — skipping duplicate comment" + exit 0 + ;; + esac + # The common path once a tracker is open — every failure after the first in # one incident appends through here — and it can 5xx like any other write. # Left bare it aborts under `set -e`, which drops the row without saying so: @@ -57,9 +80,43 @@ if [ -n "$EXISTING" ]; then # The rate-limit caller guards both of its writes instead: a failed create # there must still reach the annotation that names what to close, where here # the create is the last statement and the red step is all that is left. + # A leg that failed to post has nothing of its own to reconcile, so it stops + # here; any racing leg that did post reconciles on its own pass. if ! printf '%s\n' "$ROW" | gh issue comment "$EXISTING" -F -; then echo "::warning::Could not append this run's row to #${EXISTING}." + exit 0 fi + + # The check-then-act above still races across concurrently-jittered legs: two + # can both read no matching row before either posts. Reconcile to one row per + # run — keep the earliest comment carrying this run's anchor, delete the rest. + # Convergent, like the create reconcile in run_issue_create_and_reconcile: + # every racing leg sorts the same way and computes the same keeper, so + # deleting an already-deleted comment is a harmless 404. Selecting on the + # anchor keeps only the bot's own generated rows eligible for deletion. + # Paginated, because this endpoint returns comments oldest-first and the + # issues that need reconciling are exactly the flooded ones: past 100 + # comments the rows just posted fall off the first page, and an unpaginated + # read would find nothing to reconcile on the only issues where it matters. + # `--paginate` alone won't do — `gh` applies `--jq` per page, so each page + # would keep its own earliest comment and `sort_by | .[1:]` would delete the + # keeper. `--slurp` refuses `--jq`, hence the filter moved downstream, with + # `add` flattening the array of pages. On an issue with no comments `--slurp` + # yields `[[]]`, so `add` gives `[]` and the filter emits nothing. + # + # Guarded like the append above, and for the same reason: this is the last + # statement in the branch, so a 5xx on the read would take the script's exit + # status with it *after* the row landed — a red step carrying no annotation + # to say why, on the job someone is about to diagnose. Best-effort cleanup + # failing leaves duplicate rows on the tracker, which is the cheaper loss. + sleep 5 + gh api --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${EXISTING}/comments?per_page=100" \ + | jq -r "add | [.[] | select(.body | contains(\"${ANCHOR}\"))] | sort_by(.created_at) | .[1:] | .[].id" \ + | while read -r DUP_ID; do + [ -z "$DUP_ID" ] && continue + gh api -X DELETE "repos/${GITHUB_REPOSITORY}/issues/comments/${DUP_ID}" 2>/dev/null || true + done || echo "::warning::Could not reconcile duplicate rows on #${EXISTING}; this run's row is recorded." else printf '%s\n\n%s\n\n%s\n' \ "The bot failed to process a request. This issue tracks failures until the underlying cause is resolved." \