Add Ubuntu APT mirror fallback - #6643
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe .NET workflow adds an Ubuntu-only step that writes three prioritized fallback APT mirror URLs to ChangesAPT mirror configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The mirror fallback change is localized and introduces no actionable merge-blocking risk; it is merge-ready after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR adds an Ubuntu-only setup step intended to provide prioritized APT mirrors before disk cleanup and pipeline execution.
Confidence Score: 4/5The PR is not yet safe to merge because the Ubuntu APT fallback remains inactive. The workflow writes a prioritized mirror list but never references it from an active APT source, so subsequent package installations retain the single-mirror failure mode this change is intended to address. Files Needing Attention: .github/workflows/dotnet.yml
|
| Filename | Overview |
|---|---|
| .github/workflows/dotnet.yml | Adds the prioritized mirror-list file, but does not configure an APT source to consume it, leaving fallback behavior inactive. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Write /etc/apt/apt-mirrors.txt"] -. "no mirror+file source reference" .-> B["APT active sources"]
B --> C["Single configured mirror"]
C --> D["Subsequent package installation"]
Reviews (3): Last reviewed commit: "ci: update active apt mirror list" | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b3f730293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'http://archive.ubuntu.com/ubuntu/' '1' \ | ||
| 'http://azure.archive.ubuntu.com/ubuntu/' '2' | |
There was a problem hiding this comment.
Add explicit failover instead of relying on mirror priorities
In the Configure Ubuntu mirror fallback step, priority:1 and priority:2 do not make the second URI a retry target: as documented by apt-transport-mirror(1), the transport redirects requests to mirrors selected from the list, while failures remain with the underlying HTTP transport. With Ubuntu's apt 2.8.3, an unreachable priority-1 endpoint is retried repeatedly without any request reaching a working priority-2 endpoint. Consequently, when archive.ubuntu.com stalls—the scenario this change targets—the job still never tries Azure; use an explicit retry that swaps the source or another mechanism that performs failover.
Useful? React with 👍 / 👎.
Code reviewConfirmed bug — the TUnit/.github/workflows/dotnet.yml Lines 33 to 42 in 8b3f730
A trailing Suggested fix — match optional scheme/trailing slash instead of relying on the exact current format, so this doesn't silently regress if the base image's default URI format changes again: sudo sed -i -E \
's#https?://azure\.archive\.ubuntu\.com/ubuntu/?#mirror+file:/etc/apt/ubuntu-mirrors.txt#' \
/etc/apt/sources.list.d/ubuntu.sourcesDesign note (optional, not blocking): hand-patching the runner image's OS-provided
Everything else in the diff (the |
There was a problem hiding this comment.
Code review
Re-checked against the previous review comment on this PR: the trailing-slash sed bug is fixed correctly (the regex now has /? and matches the deb822 URIs: line without leaving a stray slash). However, that fix is applied to the wrong file, which produces a new, more serious problem.
Confirmed bug — the step is a silent no-op; it does not fix the mirror stall it's meant to fix
TUnit/.github/workflows/dotnet.yml
Lines 33 to 42 in bb9a47d
On the current ubuntu-latest (24.04 "noble") image, /etc/apt/sources.list.d/ubuntu.sources already uses APT's mirror+file transport:
URIs: mirror+file:/etc/apt/apt-mirrors.txt
There is no literal http://azure.archive.ubuntu.com/ubuntu string in that file for the sed to match — I verified this directly against a real ubuntu-latest-equivalent image. Since sed with zero matches exits 0, the step goes green but changes nothing: ubuntu.sources is untouched, and the newly-written /etc/apt/ubuntu-mirrors.txt is never referenced by anything. The mirror actually used for every apt call is still governed by the pre-existing /etc/apt/apt-mirrors.txt, which this PR never touches.
Fix: edit /etc/apt/apt-mirrors.txt directly (it already exists in this exact tab-separated URI<TAB>priority:N format) instead of trying to rewrite ubuntu.sources:
http://azure.archive.ubuntu.com/ubuntu/ priority:1
https://archive.ubuntu.com/ubuntu/ priority:2
https://security.ubuntu.com/ubuntu/ priority:3
If you fix the target file, two more things to fix in the new content
- Priority order looks inverted. GitHub-hosted runners run on Azure, so
azure.archive.ubuntu.comis the in-region mirror — that's exactly why the stock image already lists it atpriority:1. The PR's replacement list putsarchive.ubuntu.com(a smaller, cross-region pool — the classic source of cloud-IP stalls) atpriority:1instead. That makes the steady-state path slower/less reliable and only falls back to the fast mirror after a failure. Keep azure first; add archive as the fallback. - Don't drop
security.ubuntu.com. The replacement list has only 2 entries, dropping the stock image's dedicated security mirror (used by thenoble-securitystanza).archive/azuredo carry-securitytoo so it won't hard-break, but it needlessly shrinks the fallback set for security updates.
Design suggestions (not blocking)
- Fail loudly instead of silently reverting. There's no
set -euo pipefail, and no post-check that the substitution actually took effect. A future Ubuntu image change (e.g. a different default URI/path) would make this silently no-op again exactly as it does today, undermining the reliability goal. Considerset -euo pipefailplus agrep -q ... || exit 1sanity check after the edit. - Consider whether this addresses the actual symptom. APT's mirror method fails over on acquire errors, not stalls-then-trickles. If the reported failure mode is a timeout rather than a hard connection error,
Acquire::http::Timeout/Acquire::Retriestuning (via a drop-in in/etc/apt/apt.conf.d/) on the specific slow step may be the more direct fix, and is additive rather than mutating a package-owned config file that can silently diverge on image updates. - Scoping mutation to a package-owned file (
ubuntu.sourcesorapt-mirrors.txt, both maintained by the base image) for one flaky step means every apt call in the job is affected by any bug here. That's probably fine at this scale (one call site today), but worth an eventual composite action if a second workflow needs the same fallback (dotnet-build-different-locale.ymlalso runs apt-get and could benefit).
CLAUDE.md compliance
No violations — the rules in the repo's CLAUDE.md are scoped to C# source-generator/reflection/testing concerns and don't apply to this CI-only workflow change. The PR was correctly submitted via a feature branch, per the Branch & PR Workflow section.
| 'http://archive.ubuntu.com/ubuntu/' '1' \ | ||
| 'http://azure.archive.ubuntu.com/ubuntu/' '2' \ | ||
| 'https://security.ubuntu.com/ubuntu/' '3' | | ||
| sudo tee /etc/apt/apt-mirrors.txt > /dev/null |
There was a problem hiding this comment.
When a subsequent Ubuntu step installs packages, APT continues using the runner's original source because no active source references /etc/apt/apt-mirrors.txt, causing a degraded mirror to stall dependency installation without using the configured fallback.
Knowledge Base Used: Build and CI
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af7f192db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'http://archive.ubuntu.com/ubuntu/' '1' \ | ||
| 'http://azure.archive.ubuntu.com/ubuntu/' '2' \ | ||
| 'https://security.ubuntu.com/ubuntu/' '3' | | ||
| sudo tee /etc/apt/apt-mirrors.txt > /dev/null |
There was a problem hiding this comment.
Point APT sources at the mirror list
After the ubuntu.sources rewrite was removed, this step only creates /etc/apt/apt-mirrors.txt; APT reads such a list only when a configured source uses the documented mirror+file:/etc/apt/apt-mirrors.txt transport. Because the Ubuntu runner's existing source remains an ordinary HTTP URI and nothing else in the workflow references this file, subsequent Playwright dependency installation continues contacting the original source directly and this fallback configuration has no effect.
Useful? React with 👍 / 👎.
Code review (update)Previous issue resolved differently than suggested, but a new blocking issue was introduced. The trailing-slash TUnit/.github/workflows/dotnet.yml Lines 33 to 42 in 5af7f19 Current step only writes - name: Configure Ubuntu mirror fallback
if: matrix.os == 'ubuntu-latest'
run: |
printf '%s\tpriority:%s\n' \
'http://archive.ubuntu.com/ubuntu/' '1' \
'http://azure.archive.ubuntu.com/ubuntu/' '2' \
'https://security.ubuntu.com/ubuntu/' '3' |
sudo tee /etc/apt/apt-mirrors.txt > /dev/nullNothing in the workflow points any APT source at Suggested fix — the mirror list is inert without wiring it into sudo sed -i -E \
's#^URIs:.*$#URIs: mirror+file:/etc/apt/apt-mirrors.txt#' \
/etc/apt/sources.list.d/ubuntu.sourcesplaced after the Design note (still stands, optional): rather than hand-patching an OS-provided sources file that can shift format across base-image updates, consider pointing at Ubuntu's own dynamic mirror service ( |
Summary
mirror+filetransport on Ubuntu CI runnersarchive.ubuntu.comand fall back toazure.archive.ubuntu.comWhy
Ubuntu package mirrors have intermittently stalled, causing Playwright dependency installation to exceed its timeout. The timed-out install can leave
apt-getholding the package-manager lock, making subsequent retries fail immediately.Using APT's native mirror fallback reduces exposure to a single degraded mirror without changing the test pipeline.
Validation
.github/workflows/dotnet.ymlas YAMLgit diff --checkSummary by CodeRabbit