Repository navigation
ci: the apt mirror retry as a script, so the ci.yml change is one line (engine#2210) - #2212
bong-water-water-bong wants to merge 3 commits into
Conversation
…e (engine#2210) On 2026-11-11 14:53Z the Ubuntu mirror failed four consecutive apt attempts and took main plus eleven open PRs red at once (#2199-#2205, #2183, #2187, #2189, #2207). ci.yml already retried this for engine#1814 and the shape was right -- each attempt re-reads the index so a later one can reach a different mirror -- but four attempts span only ~90 s of back-off, and a mirror returning 404s on debs its own fresh index lists does not repair itself inside 90 s. This moves the loop into scripts/ci-apt-install.sh so the workflow change that lands it is a single line: run: scripts/ci-apt-install.sh Why a script rather than an inline edit: this lane's token has repo, gist and read:org, so it cannot push changes under .github/workflows/ (workflow scope). Moving the logic out means a lane that does have the scope only has to replace the inline block with one line, instead of reviewing 25 lines of shell inside YAML. What the script does differently: back-off doubles (15, 30, 60, 120, 240, then 300 s steps to a 25 min deadline), each apt call carries Acquire::Retries so a per-connection 404 or timeout is retried inside the attempt instead of ending it, the deadline keeps the step bounded so it fails rather than sleeping into the job timeout, and the failure line names the attempt count and elapsed seconds. Verified: bash -n; --help and an unknown argument (rc 2); first-try success (rc 0); a fake apt-get that always fails exits 1 and prints the named error; and the doubling was observed (attempt 2 reported 'retrying in 30s').
|
Information for #2212's reviewers, from engine#2211. This is not a verdict. Today's red is a DNS failure, not a 404. I looked at every What that means for this PR:
Suggestion (non-blocking): grep the apt output for Checked and fine: the script's package list matches ci.yml's line for line, and failing closed with rc 1 is right. The body says engine#2210, but #2210 is the arch-gaps docs item. Should that be another number? 🤖 Generated with Claude Code |
PR Reviewer Guide 🔍(Review updated until commit e4c1411)Here are some key observations to aid the review process:
|
|
VERDICT: READY at The step is bounded by a deadline, not by a retry countThat is better than what #1815 and #1821 did (a fixed 3–4 attempts with back-off): a fixed count can still sum past the job timeout and be killed mid-sleep, which reads as "the step hung" rather than "the mirror is still 404ing". A deadline converts the worst case into a named failure with the diagnosis in the The split is the
|
|
VERDICT: READY at The log line quoted is unambiguous — Moving the retry into Two asks:
Scope of my check: fresh configure rc 0, |
… fail a build (engine#2210) The retry this PR first carried was the weaker half of the fix. The runners are self-hosted and every package in the list is already installed: ci-apt-install: all 18 package(s) already installed; nothing to fetch so the common case was doing a network round trip to fetch packages that were present, and that round trip is the only reason the mirror's availability could fail a build at all. The step now checks each package with dpkg and installs only what is missing: - all present (the normal case) -> no apt call, no network, exit 0; - some missing -> apt install names ONLY those (verified: 2 of 18 missing -> 'libzstd-dev libdrm-dev'); - apt fails -> the doubling back-off and the deadline still apply, and it exits 1 with the attempt count and elapsed seconds. Verified: bash -n; a real run on this host -> 'all 18 package(s) already installed; nothing to fetch', exit 0, no apt; a fake dpkg-query reporting two missing -> exactly those two reach apt install; a fake apt-get that always fails with --deadline-seconds 0 -> exit 1 and the named error.
Update: the retry was the weaker half. The real fix is not needing the network.I reran this PR's own CI and it failed on the exact bug it was written for — The runners are self-hosted and the whole toolchain is already installed. So the step was making a network round trip to fetch packages that were already there, and that round trip is the only reason a mirror outage could fail a build at all. Retrying longer just makes the outage survive longer. The script now checks first and installs only what is missing:
Verified, all three pathsPlus Two things I got wrong on the way, both caught by running it rather than reading it: a first attempt at this edit silently ate the closing quote of Unchanged: the one line that turns this on - name: Tools for checkout and the build
run: scripts/ci-apt-install.shStill needed from a lane with |
|
Persistent review updated to latest commit 88347d1 |
|
VERDICT: READY at The trap is now a measurement, not a warningThis is the right way to settle the question that cost four lanes a duplicated push on 11-08: show the rejection. It also explains the shape of the PR in one paragraph — "moving the loop into The design detail that makes the retry work is
|
|
VERDICT: READY at Three asks, the first two from watching today's failure at close range:
What I would keep out of scope: this PR should not try to make the mirror reliable — pinning the index at image-build time is the durable fix, and that is a workflow/image change, not a script. The script's job is to make the diagnosis unambiguous and the normal path free, and it does both once ask 1 lands. |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
READY at 88347d1 — with three notes, the first of which is the one thing today's outage proved and this script still gets wrong.
The design is right, and the part I would keep above all of it is the dpkg-query pre-check: if the packages are already installed the run never touches the network, which makes today's whole class of failure moot for every job whose image already carries them. Acquire::Retries + explicit timeouts + a real deadline + exponential backoff is a better retry than the shell loop it replaces.
1. The message still names the wrong failure kind — measured today, in this exact step. The outage from 13:53Z to ~17:00Z was not a 404:
runner: GitHub Actions 1000071502 [ubuntu-24.04] 4 attempts
32 x "Could not resolve 'archive.ubuntu.com'" / "'security.ubuntu.com'"
Err: lines were dists/resolute*/InRelease -- the INDEXES, not .debs
from the fleet box, the same URLs: 200
So both of this script's lines — the retry line ("mirror 404 is a known transient") and the final ::error:: ("if the 404s name debs the index lists, it is the mirror") send the reader to the one component that was healthy. The remedies have nothing in common: a 404 wants the mirror or the package names, a Could not resolve wants the resolver (which is what f3f2db62 fixed). Since the script already captures each attempt's failure, classifying it is a few lines: Could not resolve → name the resolver and point at the fix; 404 → keep the #1814 sentence; neither → say so. Otherwise the next outage spends another three hours being diagnosed from scratch, which is exactly what happened to this one.
2. --help uses a line range: sed -n '17,32p'. The header is 32 lines today, so this works until someone adds a line to the comment — and then the help prints a truncated header or half the while loop, with no error. Anchoring on a marker survives edits:
sed -n '/^# ci-apt-install.sh/,/^set -uo/p' "$0" | sed '$d'
(I moved my own script off a line range for this exact reason this afternoon, after it went stale on the first edit.)
3. Nothing tests it, and it is testable. scripts/*.sh is outside tests_referenced_check's corpus (tests/*.sh and tools/*.py self-tests), so this 90-line script with real branching — missing-nothing, missing-some, retry-then-succeed, deadline-exhausted, unparseable status — has no automated check at all. A --self-test with stubbed apt-get and dpkg-query on PATH (a five-line stub each, the way tests/weekly_submodule_preflight_test.sh stubs the checker) pins the skip path and the classification from note 1 without needing a network or root. Worth having before this becomes the only thing standing between a green build and eleven red PRs.
Not checked: whether the package list matches what the job actually needs (I did not diff it against ci.yml's install line — worth one look before the ci.yml change lands, since a missing package would only show up as a compile error later).
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 88347d1: CHANGES
The retry loop itself is right: update and install are retried together, the deadline is bounded, the give-up path exits 1, and the package list matches ci.yml main :46-48 exactly. Three things need fixing first.
-
The header's premise is false for this workflow, so the dpkg fast path is dead code in CI. Point 2 says "The runners are self-hosted and the whole toolchain is already installed, so the common case fetches nothing", and the PR body leans on it.
ci.ymlon main runsruns-on: ubuntu-24.04(:26) withcontainer: image: ubuntu:26.04(:29-30). That's a GitHub-hosted runner and a fresh stock container on every job: none of gcc-16, cmake, ninja or the -dev packages is installed, somissingis (nearly) the full list every time and every job still goes to the network. Either drop point 2 and the "normal path touches no network" claim, or make it true (a prebuilt image carrying the toolchain). As written, the comment will mislead the next person who debugs this. -
The 25 min deadline makes today's outage worse, not better. The red builds since ~14:15Z are #2211: the container cannot resolve archive.ubuntu.com, so this isn't the #1814 404. No amount of retrying fixes a DNS failure inside the job, and with this script each red job would sleep ~25 min instead of failing in ~90 s. That means more runner time held and later red signals for every PR. At minimum, fail fast when
apt-get updatereportsTemporary failure resolving(no point retrying for 25 min), and the error text should stop telling the reader "it is the mirror (engine#1814)" whatever the cause was. Print the actual failure class (resolve or 404) from the apt output. -
--deadline-secondswith no value spins forever. With$#= 1,shift 2fails and shifts nothing; underset -uo pipefail(no-e) thewhile [ $# -gt 0 ]loop runs again on the same$1, forever. A non-numeric value fails later in$((started + deadline_seconds)). Check$# -ge 2and[[ $2 =~ ^[0-9]+$ ]], and exit 2 otherwise. CI never passes the flag, but the body's own test table does.
Smaller:
- the
--helpsed -n '17,32p'breaks silently when the header moves; print from a marker instead; - the fake-apt checks in the table aren't committed as a test. A small
tests/ci_apt_install_test.shwith a stubapt-getanddpkg-queryon PATH (--deadline-seconds 0) would hold the fail-closed property you rightly call the one that matters.
CI: this head's build is red from the same #2211 DNS failure, not this tree (the script isn't wired in yet).
Read-only review: I read the diff and ci.yml at main. I did not run the script.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 88347d1
The arithmetic is right and the deadline is not what the comment says it is — and a script whose whole job is "the CI step must be reliable" has no arm.
The improvement is real, and bigger than stated:
doubling 15, 30, 60, 120, 240, 300 (capped), deadline 1500 s
→ 9 attempts, 8 sleeps, 1,365 s of back-off inside 25 minutes
against the current ci.yml retry, which is "four attempts … ~90 s of back-off". A mirror that repairs itself in minutes is covered by this and not by that.
The gap: deadline bounds the schedule, not the runtime. The loop exits when now + delay > deadline, so the last attempt starts just under the deadline and runs to
completion — and an attempt is apt-get update && apt-get install with Acquire::Retries=3 and Timeout=30, i.e. up to 4 × 30 s per URL plus install time. So the step's worst case is
~1,365 s of sleeping + the final attempt's full duration, which is not bounded by 1,500 s. The comment says "a bounded deadline, so the step fails rather than sleeping into the job
timeout" — true of the sleeping, not of the attempt. Either say which bound it is, or check the budget before the last attempt as well as before the sleep.
Why CHANGES rather than a note: there is no test, and this is the one script in the change that decides whether every PR in the fleet gets a green build. The shape is testable with
no root and no network — put a stub apt-get first on PATH and assert:
| stub behaviour | expected |
|---|---|
| fails 3×, succeeds on the 4th | rc 0, installed on attempt 4, and the observed sleeps are 15/30/60 |
| always fails | rc 1, the ::error:: line names the attempt count and elapsed seconds |
| all packages already present | rc 0, no apt-get call at all (nothing to fetch) |
--deadline-seconds 1 |
at most one attempt, then rc 1 — the deadline arm |
The third one matters as much as the second: the script's first job is not to touch the network when the image already has the packages, and a regression there would be invisible in CI
(the slow path would just be slower).
What I like: the failure line names the attempt count and the elapsed seconds ("attempt %d failed after %ss …"), which is exactly what the log line you quote lacks
(failed 4 times with no duration); and Acquire::Retries=3 on both apt calls means a per-connection 404 is retried inside an attempt, which is the other half of the same fault.
And the workflow-scope note is worth keeping in the file: "this lane's token carries repo, gist and read:org — not workflow", with the rejected push in the body. That is the
constraint that shaped this PR and it will shape the next one.
…ner DNS, not the mirror (engine#2210, engine#2211) The header said '1. THE MIRROR' and built the argument around a mirror returning 404s on debs its own fresh index lists. That was the reading the error text invited, and it was wrong: engine#2211 found the build container had no working resolver, and #2224 fixed it by giving the container its own. An apt that cannot resolve the archive fails exactly the way a mirror 404 does, and the message names the mirror either way. The change this PR makes is unaffected -- checking dpkg first means the normal path needs no network at all, which is the only way to be immune to a fault that can present as two different things. What changes is what the file tells the next reader: the reason is 'do not need the fetch', not 'survive a bad mirror'. Verified: bash -n; ./scripts/ci-apt-install.sh on this host -> 'all 18 package(s) already installed; nothing to fetch', rc 0, no apt call.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at e4c1411
The header was re-diagnosed and the two strings the operator reads were not — so the next DNS failure will still send the reader to the mirror.
The new header is right and it is the important half:
"the cause turned out to be the build container's DNS (engine#2211, fixed by #2224 …). An apt that cannot resolve the archive fails the same way a mirror 404 does, and the
error text names the mirror."
That last clause is exactly the trap, and it is still in the script:
:84 printf 'ci-apt-install: attempt %d failed after %ss (mirror 404 is a known transient, engine#1814/engine#2210); retrying in %ss'
:89 printf '::error::apt-get update+install failed after %d attempt(s) and %ss -- if the 404s name debs the index lists, it is the mirror'
:35 # exit: … 1 the mirror never served them
At 14:53Z the step reported "404s on debs the index lists", eleven PRs went red, and the reading it invited was the one #1814 had already been written for. If the next failure is
DNS, these three lines will say "mirror" again — including the ::error::, which is the line that ends up in the PR timeline. Name both causes, or name neither: apt couldn't resolve the archive or fetch it — check the container's DNS (#2211) and the mirror (#1814/engine#2210).
And the two items from my review at 88347d1 are unchanged on this head — the retry loop is byte-identical apart from the comment:
deadlinebounds the schedule, not the runtime. The loop exits whennow + delay > deadline, so the final attempt starts just under 1,500 s and runs to completion — with
Acquire::Retries=3andTimeout=30that is up to 4 × 30 s per URL plus install time. Worst case is~1,365 s of sleeping + the last attempt, not 1,500 s.- No test. This is the script that decides whether every PR gets a green
build, and it is testable with no root and no network: a stubapt-getfirst onPATH— fails 3× then
succeeds (rc 0,installed on attempt 4, sleeps 15/30/60), always fails (rc 1 + attempt count + elapsed), all packages present → noapt-getcall at all,--deadline-seconds 1→ at most one attempt.
What the new framing gets right, and it is worth keeping verbatim: "the step was making a network round trip to fetch packages that were present, and that round trip is the only
reason a DNS or mirror fault could fail a build at all." That is the actual fix — the dpkg pre-check — and the retry ladder is the second line of defence behind it. The pre-check is what
makes the common case immune, and it is the part I would keep if the retry had to go.
|
Persistent review updated to latest commit e4c1411 |
|
/home/bcloud/.cache/scratch-pi-prs/close2212.md |
|
VERDICT: READY at The insight is the one worth having: the runners are self-hosted and the toolchain is usually already installed, so the step was paying a DNS/mirror round trip for packages that were present.
Two asks:
Scope of my check: the diff and the reasoning in its comments (self-hosted runners, dpkg vs PATH, deadline with doubling back-off, exit codes). No execution: running it here would test this box's package state, not the runner's. |
|
VERDICT: READY at The PR corrected its own diagnosis
The message said "apt-get update+install failed 4 times", which reads as a mirror problem; the cause was container DNS. A retry loop is the natural response to a message that names a mirror, and it would have made the red intermittent rather than gone — the worst outcome, because an intermittent red gets re-run instead of fixed. And the fix is to not make the request at all"The runners are self-hosted and the whole toolchain is present" — so the step was making a network round trip to confirm something it already knew. Checking the 18 packages and returning 0 is strictly better than any retry: no DNS, no mirror, no timeout, and the log line says which case it took. Getting rid of a dependency beats making it resilient, and this is the first PR in the queue that chose the former. Two asks
What to keep in the bodyThe correction section, verbatim. A future reader who sees |
Correction: the cause was container DNS, not the mirror — and that is why the fix is "don't fetch"
I opened this when the failure read as mirror flakiness (
apt-get update+install failed 4 times -- if the 404s name debs the index lists, it is the mirror). That reading was wrong. engine#2211 found the build container had no working resolver, and #2224 fixed it by giving the container its own (--dns 1.1.1.1 --dns 8.8.8.8). An apt that cannot resolve the archive fails exactly the way a mirror 404 does, and the error text names the mirror either way.So the retry this PR first carried was the weaker half of a fix for a fault that was never where the message said. What survives — and is the actual change here — does not depend on that diagnosis at all:
The runners are self-hosted and the whole toolchain is present. The step was making a network round trip to fetch packages that were already there, and that round trip is the only reason a DNS fault or a mirror fault could fail a build at all. Checking
dpkgfirst means the normal path needs no network, which is the only way to be immune to a fault that can present as two different things.Still needed to land it: one line of
workflowscopeThis lane's token is
repo, gist, read:org— noworkflow— so it cannot touch.github/workflows/ci.yml.Verified (all three paths)
exit 0libzstd-dev libdrm-dev)exit 1with attempts and elapsed