From 30640bb780a9fed371deadba8be991a8f6ca2aa5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 22 Aug 2026 23:48:20 +0200 Subject: [PATCH] =?UTF-8?q?fix(772,792):=20close=20the=20fix-round=20revie?= =?UTF-8?q?w=20=E2=80=94=20the=20doc=20still=20described=20the=20semantics?= =?UTF-8?q?=20the=20fix=20reversed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-review's one HIGH was mine and was the obvious one to miss: the previous commit changed the preflight so an unverifiable answer FAILS, and left a `docs/ci-cd.md` paragraph two screens away still saying "anything else is reported as could-not-tell". That paragraph is the one an operator reads when the job goes red, and it would have talked them into reinstating the defect. Replaced with the full arm table, including the two rows the first draft got wrong and why. * "gates nothing" was false in the way this repo has recorded before (#598): the merge-consent hook reads the COMBINED status, so a red preflight blocks the merge like any other red job. It does not SKIP the jobs it diagnoses; that is the accurate claim, in ci-cd.md and in the remote-state row. * The production retry defaults were evaluated by nothing — every test overrode both knobs. A test now drops the overrides and measures three attempts and a real pause, so editing the default to 1/0 (which would falsify the "a blip does not redden a PR" argument) goes red. * `journalctl -u gitea | grep ExecuteCleanupRules` is not a reproduction: that identifier reaches the log only through slow-query warnings, so an empty grep on a healthy host reads as "the rule never ran" — the inverse. Replaced with the admin cron API, which answers deterministically. * The recovery recipe's `docker buildx use default` needs the containerd image store to `--push` (both named hosts have it, checked today) and mutated the operator's builder selection without restoring it. * The stub's comment claimed both halves of real curl's transport failure mattered; only the exit status is observable, because `|| resp=""` discards what curl printed. * The empty-half credential refusal echoed the username; it needs no value at all. The 401/403 arm aborts the remaining pins while 404 continues — deliberate, now stated. * `curl -u "$VAR"` puts a credential in argv, and this job runs container-free on a shared host. NOT fixed here: it is the shape all five `scripts/` callers already use, so fixing one site leaves the class and splits the codebase. Filed as #821 and named at the site. refs #772 refs #792 --- docs/ci-cd.md | 41 ++++++++++++++++--- docs/remote-state-inventory.md | 2 +- scripts/ci-toolchain-image-resolves.sh | 9 +++- .../tests/test_ci_toolchain_image_resolves.py | 29 ++++++++++++- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index cdd6aba43..d994f088c 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1610,10 +1610,17 @@ package — every `:` older than the 15-slot window gone, every `keep_patte `26.3.1`. Reproduce both halves on the Gitea host (LXC 119, `192.168.1.95`): ```bash +# on the Gitea host: the rule itself sqlite3 /var/lib/gitea/data/gitea.db 'select * from package_cleanup_rule;' -journalctl -u gitea --since 2026-08-11 --until 2026-08-14 | grep ExecuteCleanupRules +# from anywhere: that the cleanup task is scheduled and has been running (schedule/prev/exec_times) +curl -s -u user:pass http://192.168.1.95:3000/api/v1/admin/cron?limit=50 \ + | jq '.[] | select(.name == "cleanup_packages")' ``` +(Do **not** reach for `journalctl -u gitea | grep ExecuteCleanupRules` — that identifier reaches the +log only via slow-query warnings, so an empty grep on a healthy host would read as "the rule never +ran", which is the inverse of what it means.) + If a pin disappears again *after* #842 changes that rule, treat this cause as refuted rather than re-applying it — something else is deleting tags. @@ -1621,9 +1628,30 @@ re-applying it — something else is deleting tags. resolves every pin in `docker-build.yml` against the registry on every run and fails with a message that names the tag. It is container-free by necessity — a job consuming the missing image could not run to report it — and deliberately **not** a `needs:` of the five jobs it diagnoses: the container -jobs already fail fast, so gating them would tax every green run to speed up a rare red one. Only -HTTP 404 is treated as "gone"; anything else (a 5xx, an unreachable host) is reported as -could-not-tell, and rejected credentials fail the job rather than passing as unknown. +jobs already fail fast, so gating them would tax every green run to speed up a rare red one. + +**Everything it cannot establish is a FAILURE, not a warning**, and the arms are worth knowing +because they send you to different places: + +| Answer | Job | Message says | +|---|---|---| +| HTTP 200 with a manifest body | green | resolves | +| HTTP 404 | **red** | `IS GONE` — rebuild the tag (recovery above) | +| HTTP 200, body is not a manifest | **red** | something is answering for the registry (proxy, login page) | +| 401 / 403 | **red** | the credentials were rejected — fix the secrets | +| anything else (5xx, unreachable, no `curl`) | **red** after `ETV_CI_ATTEMPTS` tries | `could NOT VERIFY` — check the registry's health, NOT the pin | +| `ETV_REGISTRY_AUTH` unset, or either half empty | **red**, before any query | an absent secret interpolates to `":"`, which is not a credential | + +The first draft warned and exited 0 on the last two rows. Both cold reviews found it independently +and they were right: that makes a missing `curl` or a moved registry indistinguishable from a healthy +pin, and the job would have been green forever having checked nothing. The unknown arm retries first +(`ETV_CI_ATTEMPTS`, default 3, `ETV_CI_RETRY_SECONDS` apart) so an ordinary registry blip does not +redden a PR. + +**It is not a `needs:` of anything, but it is not consequence-free either.** The merge-consent hook +reads the PR's **combined** status and denies on any non-`success` context (ersatztv#598), so a red +preflight blocks the merge exactly like any other red job. "Advisory" would be the wrong word for +it — what it does not do is *skip* the jobs it diagnoses. **Recovery, without needing CI to be healthy.** The tag names a commit, and that commit still builds the same image, so the fastest fix is to republish the *same* tag by hand — no PR, no pin bump, no @@ -1643,12 +1671,15 @@ cd /tmp/etv-toolchain # driver the same inline config ci-image.yml passes it: # [registry."192.168.1.95:3000"] # http = true -docker buildx use default +prev_builder=$(docker buildx inspect --bootstrap 2>/dev/null | awk '/^Name:/{print $2; exit}') +docker buildx use default # needs the containerd image store to --push; + # both named hosts have it (checked 2026-08-22) docker login 192.168.1.95:3000 -u timothy docker buildx build --platform linux/amd64 --provenance=false \ -f docker/ci/Dockerfile -t "192.168.1.95:3000/timothy/ersatztv-ci:$pin" --push . cd "$repo" && git worktree remove /tmp/etv-toolchain +docker buildx use "$prev_builder" # leave the operator's builder as you found it ``` Then confirm the tag resolves before re-running anything — the preflight script does exactly this diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 67eeeec8d..7fda0df97 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -128,7 +128,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | `scripts/ci-peak-anon.sh` | `N/A` | Reads no live remote state — samples the runner's local cgroup `memory.stat`/`memory.peak`. | | `scripts/ci-prove-ban-detects.sh` | `N/A` | Reads no live remote state — mutates a local workflow copy and runs pytest against the local checkout. | | `scripts/ci-step-ran.sh` | `N/A` | Reads no live remote state — reads runner-supplied env vars and local marker files it wrote itself. | -| `scripts/ci-toolchain-image-resolves.sh` — registry manifest read for the pinned toolchain tag | `UNSAFE-KNOWN` | Reads a MUTABLE identifier (a registry tag) with nothing re-checking it before the `container:` jobs pull, so a tag deleted between the preflight and the pull is reported as present. Graded `UNSAFE-KNOWN` rather than `N/A` deliberately: it authorizes nothing — it can only turn its own non-required job red — but a stale PASS is read by a human as "the image is fine", which is an assertion about remote state this file exists to grade. The residual is bounded by what it degrades to: a stale pass leaves exactly the pre-#772 behaviour (five jobs failing at pull), never anything that proceeds on the strength of the read. The opposite error is closed rather than accepted, and closed by the EXIT CODE rather than by wording: an unusable credential, an unverifiable answer (after retries) and an HTTP 200 whose body is not a manifest all FAIL the job. An earlier draft warned and exited 0 on those, which would have let a missing `curl` or a moved registry keep the job green forever — "the check could not run" presenting as "the pin is fine", which is the thing this row would otherwise be asserting falsely. | +| `scripts/ci-toolchain-image-resolves.sh` — registry manifest read for the pinned toolchain tag | `UNSAFE-KNOWN` | Reads a MUTABLE identifier (a registry tag) with nothing re-checking it before the `container:` jobs pull, so a tag deleted between the preflight and the pull is reported as present. Graded `UNSAFE-KNOWN` rather than `N/A` deliberately: nothing proceeds on the strength of the read — it can only turn its own job red, which is not nothing (the merge-consent hook denies on the COMBINED status, ersatztv#598) but is not authorization either — while a stale PASS is read by a human as "the image is fine", which is an assertion about remote state this file exists to grade. The residual is bounded by what it degrades to: a stale pass leaves exactly the pre-#772 behaviour (five jobs failing at pull), never anything that proceeds on the strength of the read. The opposite error is closed rather than accepted, and closed by the EXIT CODE rather than by wording: an unusable credential, an unverifiable answer (after retries) and an HTTP 200 whose body is not a manifest all FAIL the job. An earlier draft warned and exited 0 on those, which would have let a missing `curl` or a moved registry keep the job green forever — "the check could not run" presenting as "the pin is fine", which is the thing this row would otherwise be asserting falsely. | | `scripts/set-provider.sh` | `N/A` | Reads no live remote state — sets local `dotnet user-secrets` values. | | `scripts/__init__.py` | `N/A` | Empty package marker — executes nothing. | | `scripts/scripted-schedules/entrypoint.py` — `ScriptedScheduleApi.get_context(build_id)`, then `define_content` / `reset_playout` / `build_playout` against the same live server | `UNSAFE-KNOWN` | A genuine read-then-act over live ErsatzTV state, and the row cold review found missing when the population was still non-recursive. The context is fetched, handed to user-supplied script functions that mutate the playout, and re-fetched after a reset with nothing pinning either read — a concurrent build or edit between them is invisible. Accepted because it runs inside a single scripted-schedule build the server itself serialises per playout, and because the API exposes no version or ETag on the context to compare against; the honest bound is that the blast radius is one playout's content, reversible by rebuilding. | diff --git a/scripts/ci-toolchain-image-resolves.sh b/scripts/ci-toolchain-image-resolves.sh index fbf0b1bec..84b36be45 100755 --- a/scripts/ci-toolchain-image-resolves.sh +++ b/scripts/ci-toolchain-image-resolves.sh @@ -67,7 +67,7 @@ case "$auth" in *) fail "ETV_REGISTRY_AUTH must be user:pass, got a value with no ':' — the registry cannot be queried and this check refuses to report a pass it did not establish" ;; esac [ -n "${auth%%:*}" ] && [ -n "${auth#*:}" ] \ - || fail "ETV_REGISTRY_AUTH is '${auth%%:*}:<...>' with an empty half — this is what an ABSENT REGISTRY_USER/REGISTRY_PASSWORD secret interpolates to, not a credential. Fix the secrets rather than reading an unauthenticated 401 as could-not-tell." + || fail "ETV_REGISTRY_AUTH has an empty half (user or password) — this is what an ABSENT REGISTRY_USER/REGISTRY_PASSWORD secret interpolates to, not a credential. Fix the secrets rather than reading an unauthenticated 401 as could-not-tell." accept='application/vnd.oci.image.manifest.v1+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.docker.distribution.manifest.list.v2+json' attempts="${ETV_CI_ATTEMPTS:-3}" @@ -82,6 +82,11 @@ rc=0 # the result on a large body. probe() { local url="$1" resp code body + # `-u` puts the credential in argv, visible to `ps` for the length of the call — and this job has + # no `container:`, so that is the shared host. Kept because it is the shape every other curl caller + # in scripts/ already uses (`ci-detect-already-validated.sh`, `pr-changed-files.sh`, + # `select-queue.sh`, `issue-qualification-audit.sh`): fixing one site would leave the class intact + # and the codebase inconsistent. The class is tracked in ersatztv#821. resp=$(curl -s -w '\n%{http_code}' -u "$auth" -H "Accept: $accept" "$url") || resp="" [ -n "$resp" ] || { printf '000 no\n'; return 0; } code=${resp##*$'\n'} @@ -127,6 +132,8 @@ EOF rc=1 ;; 401|403) + # `fail` rather than `rc=1`: unlike a 404, this says nothing about the pin, and it will say + # the same thing about every remaining one. Abandoning the loop keeps the log to one cause. fail "the registry rejected these credentials (HTTP $code) for $registry/$image_repo:$pin, so the pin could not be checked. Fix REGISTRY_USER/REGISTRY_PASSWORD rather than reading this as a pass." ;; *) diff --git a/scripts/tests/test_ci_toolchain_image_resolves.py b/scripts/tests/test_ci_toolchain_image_resolves.py index 1bd404b83..80bcd55a7 100644 --- a/scripts/tests/test_ci_toolchain_image_resolves.py +++ b/scripts/tests/test_ci_toolchain_image_resolves.py @@ -14,6 +14,7 @@ from __future__ import annotations import os import subprocess +import time from pathlib import Path import pytest @@ -46,8 +47,8 @@ if not user or not password: codes = dict(pair.split("=", 1) for pair in (state / "codes").read_text().split() if pair) code = codes.get(tag, codes.get("*", "200")) if code == "TRANSPORT": - # Real curl prints the -w output and exits 7 when the connection fails; both halves matter. - print("000", end="") + # Only the EXIT STATUS is observable: the script's `|| resp=""` discards whatever curl printed, + # so what this reproduces is the non-zero exit, not the `\n000` real curl also emits. sys.exit(7) body = '{"schemaVersion": 2, "mediaType": "application/vnd.oci.image.manifest.v1+json"}' if code == "200-NOT-A-MANIFEST": @@ -274,3 +275,27 @@ def test_the_grep_line_cannot_match_ITSELF(preflight): assert result.returncode != 0 assert "no ersatztv-ci pin found" in result.stderr assert preflight.calls() == [] + + +def test_the_PRODUCTION_retry_defaults_are_the_ones_that_run(preflight): + """Every other test overrides the retry knobs, so nothing evaluated `${VAR:-default}` itself. + + That matters because the defaults are the argument: "unknown fails" is only affordable if an + ordinary registry blip is absorbed first. Edited to 1 attempt / 0 seconds, this file would stay + green while a single transient 503 reddened every PR. So this one drops both overrides and + measures the real thing — three attempts, and a pause long enough to have actually happened. + """ + del preflight.env["ETV_CI_ATTEMPTS"] + del preflight.env["ETV_CI_RETRY_SECONDS"] + preflight.set_codes({"32747a0": "503"}) + + started = time.monotonic() + result = preflight.run() + elapsed = time.monotonic() - started + + assert result.returncode != 0 + assert len(preflight.calls()) == 3, f"the default attempt count is not 3 — got {len(preflight.calls())} call(s)" + assert elapsed >= 8, ( + f"two pauses at the default 5s should take >=10s; took {elapsed:.1f}s, so the default pause " + "has been shortened out from under the 'a blip does not redden a PR' argument" + )