fix(ci): the apt bounds were calibrated against a claim, and the step over-installed - #417
Conversation
… over-installed The aarch64 provision failed on three consecutive PRs (#412, #415, #416) in one hour. The log says exactly where, and it is not a mirror outage: attempt 1 update killed at 180s, precisely the timeout attempt 2 update killed at 180s again attempt 3 update succeeded; install killed at 300s MID-DOWNLOAD, the log ending inside `Get:20 gcc-13-aarch64-linux-gnu [21.1 MB]` No apt error appears anywhere in it. The provision was succeeding slowly, and the wrapper converted that into a hard failure three times over -- the shape this release keeps finding, with the sign reversed: a gate that fires on a healthy run rather than one that passes a broken one. It blocked three PRs while reading as infrastructure decay. The bound came from a premise this script asserted and never measured: "a healthy `update` on these runners is a few seconds", which made 180s look like an order of magnitude of headroom. Rewritten to state the arithmetic instead, since that phrasing is precisely what went unchecked. Two independent changes. `apt-get update` now runs ONLY after a direct install has failed. The runner image ships a populated index, so a refresh does not belong on the happy path -- it is the recovery step for the one failure it fixes, an index stale enough that the requested version has moved. Attempt 1 skips it, removing the 180s that killed two of the three attempts before they reached the package at all. Worst case is now 300 + 15 + 600 + 30 + 600 = 1245s, inside the job's 25-minute budget with room for the check. The step also asked for the wrong package, and the comment above it said so without drawing the conclusion: it named `libc6-dev-arm64-cross` as what actually lands the headers, and stated the cross linker is unused because this gate is `cargo check` only. So it installed a whole cross toolchain to obtain a dependency it had already identified -- 20+ packages, one of them 21.1 MB, for a set of headers. bindgen runs the HOST clang against `--sysroot` and never invokes the cross compiler. Both ARM targets now install the header package directly, plus `--no-install-recommends`, which is the same argument as the timeout: every byte downloaded is time spent inside a bound. The sufficiency of the narrower package is VERIFIED BY THIS JOB, not asserted -- if the headers are not where the export step points clang, bindgen fails loudly in `cargo check` on the very run that installs it. The loop's branch structure is proven with a stubbed dry run: attempt 1 issues `install` alone, attempts 2 and 3 issue `update` then `install`. shellcheck and actionlint both clean.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the CI apt provisioning used by the libretro-cross job to be more resilient to slow mirrors and to avoid installing an unnecessary cross toolchain when only glibc headers are needed for bindgen.
Changes:
- Narrow the bindgen provisioning packages from full GCC cross toolchains to header-only
libc6-dev-*-crosspackages. - Rework
.github/scripts/apt-install-retry.shsoapt-get updateruns only on retry attempts, and add--no-install-recommendsto reduce download time. - Update the script’s documentation to reflect the motivating failure mode and intended retry budgeting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/ci.yml | Switches ARM bindgen provisioning to header-only libc6-dev-*-cross packages. |
| .github/scripts/apt-install-retry.sh | Changes retry flow (install-first, update-on-retry), adds --no-install-recommends, and adjusts timeout commentary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This change's own first CI run passed, and the log says why the previous
bounds could not have:
Fetched 4201 kB in 4min 45s (14.7 kB/s)
Fourteen point seven kilobytes per second. The mirror is degraded by
roughly three orders of magnitude, which is why every bound derived from
"healthy" behaviour was wrong -- and why the old 40 MB package set was
hopeless rather than unlucky: at that rate it needed about 45 minutes,
past the whole job budget.
Even at 4.2 MB it only just passed, and not the way the first draft of
this comment assumed. The real sequence, read from the log rather than
inferred from the exit code:
attempt 1 install downloaded all 4201 kB (285s), then was killed at
300s during dpkg unpack -- the download finished, the
install did not
attempt 2 update, then install: NO re-download, because the archives
were already in /var/cache/apt/archives. Succeeded.
So the run was rescued by apt's archive cache persisting across
attempts. That is a genuinely useful property -- each attempt makes
progress instead of starting over -- but it was undesigned and
undocumented, which puts it in the same class as the bound it rescued:
behaviour nobody wrote down, working by accident, and indistinguishable
from a design until it stops.
It is now written down and no longer load-bearing. INSTALL_TIMEOUT is
sized so ONE attempt completes at the worst speed actually observed:
285s of download plus dpkg, so 600s is about 2x that. ATTEMPTS drops to
2 to keep the worst case inside the job's 25 minutes -- 600 + 15 + (180
+ 600) = 1395s plus ~45s of surrounding steps. The third attempt is no
loss: attempt 2 already retries with a refreshed index AND a warm
download cache, covering both the stale index and the slow mirror, so a
third would only repeat it.
The first run passed, and it corrects this PR's own reasoning
The log also settles what was actually wrong, and it is worse than "a slow mirror": 14.7 kB/s — degraded by roughly three orders of magnitude. That is why every bound derived from "healthy" behaviour was wrong, and why the old ~40 MB package set was hopeless rather than unlucky: at that rate it needed about 45 minutes, past the entire job budget. No timeout value could have saved it; only the narrowing could. And the pass was more fragile than I claimedI wrote that skipping The run was rescued by apt's archive cache persisting across attempts. That is a genuinely useful property — each attempt makes progress instead of starting over — but it was undesigned and undocumented, which puts it in exactly the class this release keeps finding: behaviour nobody wrote down, working by accident, indistinguishable from a design until it stops. What changed as a resultIt is now documented and no longer load-bearing. The third attempt is no loss: attempt 2 already retries with a refreshed index and a warm download cache, covering both the stale index and the slow mirror, so a third would only repeat it. The comment now carries a standing instruction: do not tune these numbers again without a |
Both reviewers raised it independently on #417. `APT_PACKAGE` comes from a workflow `env:` block and never from event data, so this is not closing a live injection path -- but it is one token that makes the guarantee structural rather than dependent on every future caller remembering where the value came from. A value beginning with a hyphen is now an operand, not an option.
|
Antigravity review addressed. One blocking finding accepted, one declined as factually wrong, three suggestions declined. Accepted — the
|
Antigravity review (Gemini via Ultra)This PR narrows the installed apt packages to only the needed headers and adjusts the retry logic to skip Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
|
Re-review after "Consider dropping the quotes" so a future If multi-package support is ever needed, the correct shape is an array ( Comment length — answered in the previous comment: the comment is the fix here. The defect was a bound derived from an unmeasured assertion; a shorter unmeasured assertion reproduces it exactly. The "not a size micro-optimisation" phrasing — fair as style, kept for one reason. |
The aarch64 provision failed on three consecutive PRs in one hour — #412, #415, #416. It is not a mirror outage. The log says exactly where:
No apt error appears anywhere in it. The provision was succeeding slowly, and the wrapper converted that into a hard failure three times over — the shape this release keeps finding, with the sign reversed: a gate that fires on a healthy run rather than one that passes a broken one. It blocked three PRs while reading as infrastructure decay.
The bound came from a claim, not a measurement
The script asserted, in its own comment, that "a healthy
updateon these runners is a few seconds", which made 180 s look like an order of magnitude of headroom. That was never checked. It is now stated as arithmetic instead, because that phrasing is precisely what went unexamined.Two independent changes
1.
apt-get updateruns only after a direct install has failed. The runner image ships a populated index, so a refresh does not belong on the happy path — it is the recovery step for the one failure it actually fixes, an index stale enough that the requested version has moved and install 404s. Attempt 1 skips it, removing the 180 s that killed two of three attempts before they reached the package.Worst case is now
300 + 15 + 600 + 30 + 600 = 1245s, inside the job's 25-minute budget with room for thecargo check.2. The step was asking for the wrong package — and the comment above it said so without drawing the conclusion. It named
libc6-dev-arm64-crossas what actually lands the headers, and stated the cross linker is unused because this gate iscargo checkonly. So it installed a whole cross toolchain to obtain a dependency it had already identified: 20+ packages, one of them 21.1 MB, for a set of headers. bindgen runs the host clang against--sysrootand never invokes the cross compiler.Both ARM targets now install the header package directly, plus
--no-install-recommends— the same argument as the timeout, since every byte downloaded is time spent inside a bound.Verification
The narrower package's sufficiency is verified by this job, not asserted. If the headers are not where the export step points clang, bindgen fails loudly in
cargo checkon the very run that installs it. That is the check to watch on this PR.The loop's branch structure is proven with a stubbed dry run, since the real thing cannot be executed here:
install1install1 update2 install2 update3 install3shellcheckandactionlintboth clean.What this does not claim
It does not claim the mirror was healthy. Two 180 s
updatetimeouts in a row is slow by any reading. It claims the response to slow was wrong: three retries against a bound too tight to succeed, downloading a toolchain nothing invokes.