Skip to content

fix: handle flaky and dead mirrors gracefully during speed ranking - #4869

Open
wellparth wants to merge 1 commit into
archlinux:masterfrom
wellparth:fix/mirror-region-freeze-concurrency
Open

wellparth wants to merge 1 commit into
archlinux:masterfrom
wellparth:fix/mirror-region-freeze-concurrency

Conversation

@wellparth

@wellparth wellparth commented Nov 1, 2026 •

Copy link
Copy Markdown

PR Description:

During mirror ranking, selecting regions with large numbers of mirrors (such as France or Germany) previously caused archinstall to hang or take several minutes when encountering unresponsive, slow, or flaky mirrors.

Root Cause:

  1. Unguarded connection attempts: DownloadTimer was entered after urllib.request.urlopen(). Consequently, SIGALRM was not active during DNS resolution, TCP connection establishment, or TLS handshakes, relying solely on socket timeouts which could block for 5–10+ seconds per address.
  2. Compound retries on failing mirrors: Mirrors encountering ConnectionResetError or IncompleteRead retried up to 3 times (_speedtest_retries = 3). For dead or flaky mirrors, this multiplied the delay significantly.
  3. Overly generous timeout: Probing a ~131 KB core.db file had a 5-second socket timeout and 5-second timer. Healthy mirrors download this in <1.5s, while failing mirrors blocked for 5+ seconds each.

Changes:

  • Wrap urlopen inside DownloadTimer: DownloadTimer is entered before urllib.request.urlopen, ensuring that DNS resolution, TCP connection, TLS handshake, headers, and body read are all strictly bounded by SIGALRM.
  • Fast-fail without retries: Eliminated the retry loop on mirror speed probing. Unreachable, timing out, or flaky mirrors fail fast and are immediately marked with speed = 0 so the installer moves on to the next mirror without delay.
  • Lower timeout to 3s: Reduced the socket and download timer timeout to 3 seconds, which is ample headroom for viable mirrors while minimizing wait time on unresponsive endpoints.
  • Sequential sorting maintained: Maintained sequential speed checks as recommended by maintainers to avoid bandwidth contention skewing speed measurements.
  • Guarded calculations: Guarded against ZeroDivisionError when timer.time == 0.
  • Robust DownloadTimer: Handled AttributeError and ValueError safely when signal handling is unsupported.
  • Unit Tests: Added test coverage in tests/test_mirrorlist.py for mirror speed ranking order, error/timeout handling, and timer behavior.

Tests and Checks

  • I have tested the code!
    • Unit tests in tests/test_mirrorlist.py passing (6/6 passed).
    • Code formatting and linting verified with ruff check and ruff format (0 errors).

@wellparth
wellparth requested a review from Torxed as a code owner November 1, 2026 11:58
if self._fetched_remote and speed_sort and region_list:
info('Sorting your selected mirror list based on the speed between you and the individual mirrors (this might take a while)')
max_workers = min(16, len(region_list))
with ThreadPoolExecutor(max_workers=max_workers) as executor:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not cause an unfair speed measurement considering max_workers compete for the available speed?

@h8d13 h8d13 Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want concurrency at all for this. Sequentially even with 100s of mirrors in a region, it should breeze through them (10-20 seconds), if it's getting stuck somewhere it's likely due to how we currently handle flaky/dead mirrors, not a need for workers.

If anything its additional packages that should be threaded (#4083)

The title of this PR also doesn't make sense, sorting happens after the TUI. I sense this might be automated with the escapes leaking in the PR xd

…rchlinux#4858)

Mirror ranking previously hung or took several minutes when encountering unresponsive, slow, or flaky mirrors:

- DownloadTimer was entered after urlopen, leaving DNS resolution, connection establishment, and TLS handshakes unguarded by SIGALRM.

- Failed or resetting connections retried up to 3 times per mirror.

- Socket timeouts and download timers were unnecessarily generous (5s) for downloading a ~131 KB core.db file.

Changes:

- Enter DownloadTimer before urlopen so the entire connection and download operation is bounded by SIGALRM.

- Lower probe timeout to 3 seconds, which is ample for viable mirrors transferring core.db while failing fast on slow/dead mirrors.

- Fail fast and skip unreachable or flaky mirrors immediately with speed 0 without retrying during speed ranking.

- Guard against ZeroDivisionError when timer.time == 0.

- Handle AttributeError/ValueError in DownloadTimer when signal/alarm is unavailable.

- Add unit tests for speed ranking order, error/timeout handling, and timer behavior.

Fixes archlinux#4858
@wellparth
wellparth force-pushed the fix/mirror-region-freeze-concurrency branch from 76ee773 to 350c6d1 Compare November 6, 2026 13:00
@wellparth wellparth changed the title fix: test mirror speeds concurrently to prevent TUI freezing fix: handle flaky and dead mirrors gracefully during speed ranking Nov 6, 2026
@wellparth

Copy link
Copy Markdown
Author

Thanks for the feedback @Torxed and @h8d13!

I've updated the PR to pivot away from concurrency and address the root cause of the hang:

  1. Sequential probing maintained: Removed ThreadPoolExecutor to keep bandwidth measurements fair and sequential.
  2. Proper timeout bounding: DownloadTimer was previously entered after urlopen(), leaving DNS resolution and connection handshakes completely unguarded by SIGALRM. It now wraps urlopen directly with a 3s timeout so stalled endpoints fail fast.
  3. No retries on failed probes: Dropped the retry loop on speed checks—unresponsive, dropping, or resetting mirrors are immediately assigned speed 0 and skipped rather than retrying up to 3 times.
  4. Cleaned up PR: Updated the title and description to accurately reflect the fix.

@h8d13

h8d13 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Scope looks much better IMO

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants