Skip to content

KM-17461: refetch servers if needed - #372

Closed
kp-mario-nachbaur wants to merge 1 commit into
masterfrom
KM-17461-undissmissable-popup
Closed

KM-17461: refetch servers if needed#372
kp-mario-nachbaur wants to merge 1 commit into
masterfrom
KM-17461-undissmissable-popup

Conversation

@kp-mario-nachbaur

Copy link
Copy Markdown
Contributor

VPN installation fails because there is no target server and no servers in general. Retry after refetching servers.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kp-mario-nachbaur's task in 1m 15s —— View job


PR Review: refetch servers if needed

  • Read the diff and surrounding code
  • Trace serverProvider.download threading
  • Review for bugs, threading, retain cycles
  • Post feedback

Thanks Mario — the fix addresses the right root cause: when install fails because there are no servers, refetch the server list before retrying. The [weak self] additions are correct and avoid retaining the VC in the async callbacks. A few things worth addressing:

🔴 Threading — UIKit called off the main thread

refreshServersAndRetry calls submit() and alertRequiredPermission() from inside the serverProvider.download completion. That completion is not guaranteed to run on the main thread in production:

  • PIAWebServices.downloadServers runs the fetch inside a Task { ... } and invokes the callback from that async context (PIAWebServices.swift:352-370).
  • DefaultServerProvider.download then forwards it on the non-DIP path without hopping to main (DefaultServerProvider.swift:247-250).

So submit()vpn.install/present(...) and alertRequiredPermission()present(...) can execute off the main thread, which is a UIKit violation (crashes/undefined behavior). Recommend dispatching to main:

private func refreshServersAndRetry() {
    Client.providers.serverProvider.download { [weak self] (servers, error) in
        DispatchQueue.main.async {
            guard let self else { return }
            if let servers, !servers.isEmpty {
                self.submit()
            } else {
                if let error {
                    log.error("Failed to refetch servers: \(error)")
                }
                self.alertRequiredPermission()
            }
        }
    }
}

Fix this →

🟡 Discarded error

The download error is currently ignored ({ (servers, _) in). Since this is exactly the failure path users get stuck in, logging it (as above) will make field diagnosis much easier when the refetch itself is what's failing.

🟡 Retry loop has no ceiling / backoff

The flow is:

submit → install fails → alertRequiredPermission → OK → refreshServersAndRetry → no servers → alertRequiredPermission → …

This is user-gated (each iteration requires tapping OK, so it's not a tight infinite loop), and I understand the branch intent is an undismissable popup. Still, if the regions endpoint is persistently down, the only escape is "Contact support" (and that action only exists when canSendMail() is true). Worth confirming with product that a user with no mail configured and a failing regions endpoint isn't hard-stuck. A small backoff or an attempt counter that eventually surfaces a different message would be more resilient. Not blocking.

✅ Looks good

  • [weak self] + guard let self on all three closures is correct and consistent.
  • Refetch-then-retry is a clean, minimal fix for the "no target server / no servers" case.

Testing suggestions

  • VPN permission screen on a fresh install / logged-in account where the server list is empty (e.g., regions cache cleared) — verify tapping OK refetches and then installs.
  • Force the regions endpoint to fail/return empty and confirm no off-main-thread crash when the alert re-presents (this is the scenario the threading fix protects).
  • Verify behavior with and without mail configured (MFMailComposeViewController.canSendMail() true/false).
  • No unit tests are included; VPNPermissionViewController is UIKit/Storyboard-based so it's not easily testable as-is, but the refetch/retry decision logic could be extracted to make it unit-testable in PIA VPNTests.

@kp-mario-nachbaur
kp-mario-nachbaur deleted the KM-17461-undissmissable-popup branch July 27, 2026 08:21
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.

1 participant