From 8c5bccfcf043b05f61c021ca6223fee925b3bafd Mon Sep 17 00:00:00 2001 From: Pete Walters Date: Thu, 7 Aug 2025 11:57:53 -0500 Subject: [PATCH] Improve cancellation/error handling around some QuickRestore operations --- .../OutgoingDeviceRestorePresenter.swift | 92 +++++++++++++++++-- .../OutgoingDeviceRestoreViewModel.swift | 56 ++++++++--- .../DeviceTransfer/TransferStatusState.swift | 52 +++++++++-- Signal/Provisioning/QuickRestoreManager.swift | 11 ++- .../RegistrationCoodinatorShims.swift | 5 + .../RegistrationCoordinatorImpl.swift | 2 + .../RegistrationNavigationController.swift | 2 +- .../RegistrationTransferStatusState.swift | 5 + ...strationTransferStatusViewController.swift | 31 +++++-- .../RegistrationCoordinatorTestShims.swift | 2 + .../translations/en.lproj/Localizable.strings | 54 +++++++++++ 11 files changed, 274 insertions(+), 38 deletions(-) diff --git a/Signal/DeviceTransfer/OutgoingDeviceRestorePresenter.swift b/Signal/DeviceTransfer/OutgoingDeviceRestorePresenter.swift index a35cd5c64d..0f6b5df111 100644 --- a/Signal/DeviceTransfer/OutgoingDeviceRestorePresenter.swift +++ b/Signal/DeviceTransfer/OutgoingDeviceRestorePresenter.swift @@ -156,7 +156,8 @@ class OutgoingDeviceRestorePresenter: OutgoingDeviceRestoreInitialPresenter { } guard await viewModel.confirmTransfer() else { - // TODO: [Backups] - Display an error + // Silently fail here. The confirmTransfer UI will notify the user of + // success/failure (e.g. FaceID UI) return } @@ -171,8 +172,8 @@ class OutgoingDeviceRestorePresenter: OutgoingDeviceRestoreInitialPresenter { await displayRestoreMessage(isBackup: false, presentingViewController: presentingViewController) case .deviceTransfer: guard let peerConnectionData = restoreMethodData.peerConnectionData else { - // TODO: [Backups] - Display an error - throw OWSAssertionError("Missing transfer connection data") + Logger.error("Missing transfer connection data") + throw DeviceRestoreError.invalidRestoreData } // Push the status sheet if this is a transfer @@ -182,20 +183,91 @@ class OutgoingDeviceRestorePresenter: OutgoingDeviceRestoreInitialPresenter { ) await viewModel.waitForDeviceConnection(peerConnectionData: peerConnectionData) - Task { @MainActor in + Task { @MainActor [weak self] in // TODO: [Backups] - DeviceTransferService does a db.write // internally, and this should be updated to an actor/async aware // in a followup piece of work (and possibly once the old device transfer // flow is removed) - try viewModel.startTransfer(peerConnectionData: peerConnectionData) + do { + try viewModel.startTransfer(peerConnectionData: peerConnectionData) + } catch { + Logger.warn("Device transfer failed: \(error)") + await self?.handleError( + DeviceRestoreError.unknownError, + presentingViewController: presentingViewController + ) + } + } + let success = await viewModel.waitForTransferCompletion() + if !success { + await handleError( + DeviceRestoreError.restoreCancelled, + presentingViewController: presentingViewController + ) + } else { + await displayTransferComplete(presentingViewController: presentingViewController) } - await viewModel.waitForTransferCompletion() - - await displayTransferComplete(presentingViewController: presentingViewController) } } catch { - // TODO: [Backups] - Display an error - Logger.error("error: \(error)") + switch error { + case let restoreError as DeviceRestoreError: + await handleError(restoreError, presentingViewController: presentingViewController) + default: + Logger.error("Unexpected device transfer error: \(error)") + } } } + + @MainActor + func handleError(_ error: DeviceRestoreError, presentingViewController: UIViewController?) async { + guard let presentingViewController else { + Logger.warn("Cannot display transfer error") + return + } + + let (title, body) = switch error { + case .invalidRestoreData: ( + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_FAILED_RESTORE_TITLE", + comment: "Title of prompt notifying restore failed." + ), + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_FAILED_RESTORE_BODY", + comment: "Body of prompt notifying restore failed." + ) + ) + case .restoreCancelled: ( + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_CANCELLED_RESTORE_TITLE", + comment: "Title of prompt notifying restore was cancelled." + ), + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_CANCELLED_RESTORE_BODY", + comment: "Body of prompt notifying restore was cancelled." + ) + ) + case .unknownError: ( + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_UNKNOWN_ERROR_TITLE", + comment: "Title of prompt notifying restore failed for unknown reasons." + ), + OWSLocalizedString( + "OUTGOING_DEVICE_REGISTRATION_UNKNOWN_ERROR_BODY", + comment: "Body of prompt notifying restore failed for unknown reasons." + ) + ) + } + + let sheet = HeroSheetViewController( + hero: .image(UIImage(resource: .checkCircle)), + title: title, + body: body, + primaryButton: .init(title: CommonStrings.okayButton, action: { _ in + presentingViewController.dismiss(animated: true) + }) + ) + sheet.modalPresentationStyle = .formSheet + await presentingViewController.awaitableDismiss(animated: true) + await presentingViewController.awaitablePresent(sheet, animated: true) + } } diff --git a/Signal/DeviceTransfer/OutgoingDeviceRestoreViewModel.swift b/Signal/DeviceTransfer/OutgoingDeviceRestoreViewModel.swift index b270eb3308..6bedf514cf 100644 --- a/Signal/DeviceTransfer/OutgoingDeviceRestoreViewModel.swift +++ b/Signal/DeviceTransfer/OutgoingDeviceRestoreViewModel.swift @@ -7,6 +7,12 @@ import Foundation import SignalServiceKit import MultipeerConnectivity +enum DeviceRestoreError: Error { + case invalidRestoreData + case restoreCancelled + case unknownError +} + class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObserver { struct RestoreMethodData { @@ -35,7 +41,7 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs )?> = AtomicValue(nil, lock: .init()) private var finishTransferContinuation: AtomicValue< - CheckedContinuation? + CheckedContinuation? > = AtomicValue(nil, lock: .init()) init( @@ -46,6 +52,10 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs self.deviceTransferService = deviceTransferService self.quickRestoreManager = quickRestoreManager self.provisioningURL = deviceProvisioningURL + + transferStatusViewModel.onCancel = { [weak self] in + self?.cancelTransfer() + } } func confirmTransfer() async -> Bool { @@ -57,9 +67,16 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs /// 2. Outgoing device will wait for the restore method choice from the other device. /// 3. Confirm the returned choice is 'device transfer' or fail. /// 4. Parse out the MPC connection information returned in the restore method choice, and return this connection data - func waitForRestoreMethodResponse() async throws -> RestoreMethodData { - let restoreMethodToken = try await quickRestoreManager.register(deviceProvisioningUrl: provisioningURL) - let restoreMethod = try await quickRestoreManager.waitForRestoreMethodChoice(restoreMethodToken: restoreMethodToken) + func waitForRestoreMethodResponse() async throws(DeviceRestoreError) -> RestoreMethodData { + let restoreMethod: QuickRestoreManager.RestoreMethodType + do { + let token = try await quickRestoreManager.register(deviceProvisioningUrl: provisioningURL) + restoreMethod = try await quickRestoreManager.waitForRestoreMethodChoice(restoreMethodToken: token) + } catch { + Logger.error("Failed to wait for restore method choice: \(error)") + throw DeviceRestoreError.invalidRestoreData + } + guard case let .deviceTransfer(transferData) = restoreMethod else { return RestoreMethodData(restoreMethod: restoreMethod, peerConnectionData: nil) } @@ -68,7 +85,8 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs let urlString = String(data: stringData, encoding: .utf8), let transferURL = URL(string: urlString) else { - throw OWSAssertionError("Attempting to restore using a method other than device transfer") + Logger.error("Attempting to restore using a method other than device transfer") + throw DeviceRestoreError.invalidRestoreData } do { @@ -81,8 +99,8 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs ) ) } catch { - Logger.error("Failed to register device via URL: \(error)") - throw error + Logger.error("Failed to parse transfer URL: \(error)") + throw DeviceRestoreError.invalidRestoreData } } @@ -120,14 +138,20 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs } } - func waitForTransferCompletion() async { + func waitForTransferCompletion() async -> Bool { return await withCheckedContinuation { continuation in self.finishTransferContinuation.update { switch transferStatusViewModel.state { - case .done, .error: + case .done: // If the transfer is finished, just return - continuation.resume() + continuation.resume(returning: true) return + case .cancelled: + continuation.resume(returning: false) + return + case .error(let error): + Logger.warn("Transfer failed: \(error)") + continuation.resume(returning: false) case .connecting, .idle, .starting, .transferring: break } @@ -136,6 +160,15 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs } } + private func cancelTransfer() { + stopListeningForTransfer() + transferStatusViewModel.state = .cancelled + deviceTransferService.cancelTransferFromOldDevice() + deviceTransferService.stopTransfer() + let continuation = finishTransferContinuation.swap(nil) + continuation?.resume(returning: false) + } + private func stopListeningForTransfer() { deviceTransferService.removeObserver(self) deviceTransferService.stopListeningForNewDevices() @@ -169,10 +202,11 @@ class OutgoingDeviceRestoreViewModel: ObservableObject, DeviceTransferServiceObs finishTransferContinuation.update { continuation in if let error { transferStatusViewModel.state = .error(error) + continuation?.resume(returning: false) } else { transferStatusViewModel.state = .done + continuation?.resume(returning: true) } - continuation?.resume() continuation = nil } } diff --git a/Signal/DeviceTransfer/TransferStatusState.swift b/Signal/DeviceTransfer/TransferStatusState.swift index dd9bfa757d..88aa3f8e3b 100644 --- a/Signal/DeviceTransfer/TransferStatusState.swift +++ b/Signal/DeviceTransfer/TransferStatusState.swift @@ -12,7 +12,8 @@ enum TransferState { case connecting case transferring(Double) case done - case error(Error) + case cancelled + case error(DeviceTransferService.Error) } class TransferStatusViewModel: ObservableObject { @@ -20,34 +21,66 @@ class TransferStatusViewModel: ObservableObject { enum Indefinite { case starting case connecting + case cancelling func title(isNewDevice: Bool) -> String { switch self { case .starting: if isNewDevice { - "Waiting to connect to old iPhone…" // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_NEW_DEVICE_STARTING", + comment: "Status message on new device when transfer is starting." + ) } else { - "Waiting to connect to new iPhone…" // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_OLD_DEVICE_STARTING", + comment: "Status message on old device when transfer is starting." + ) } case .connecting: if isNewDevice { - "Connecting to old iPhone…" // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_NEW_DEVICE_CONNECTING", + comment: "Status message on new device when connecting to old device." + ) } else { - "Connecting to new iPhone…" // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_OLD_DEVICE_CONNECTING", + comment: "Status message on new device when connecting to new device." + ) + } + case .cancelling: + if isNewDevice { + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_NEW_DEVICE_CANCELLING", + comment: "Status message on new device when cancelling transfer." + ) + } else { + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_OLD_DEVICE_CANCELLING", + comment: "Status message on old device when cancelling transfer." + ) } } } func message(isNewDevice: Bool) -> String { if isNewDevice { - "Bring your old device nearby, and make sure Wi-Fi and Bluetooth are enabled." // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_NEW_DEVICE_CONNECTING_MESSAGE", + comment: "Description message on new device displayed during device transfer." + ) } else { - "Bring your new device nearby, and make sure Wi-Fi and Bluetooth are enabled." // TODO: Localize + OWSLocalizedString( + "DEVICE_TRANSFER_STATUS_OLD_DEVICE_CONNECTING_MESSAGE", + comment: "Description message on old device displayed during device transfer." + ) } } } case indefinite(Indefinite) case transferring(Double) + case error(Error) } @Published var viewState: ViewState = .indefinite(.starting) @@ -63,7 +96,10 @@ class TransferStatusViewModel: ObservableObject { self.progressDidUpdate(currentProgress: progress) case .done: viewState = .transferring(1) - case .error(_): + case .cancelled: + viewState = .indefinite(.cancelling) + case .error(let error): + viewState = .error(error) return } } diff --git a/Signal/Provisioning/QuickRestoreManager.swift b/Signal/Provisioning/QuickRestoreManager.swift index 71af12b138..dd5003d0c0 100644 --- a/Signal/Provisioning/QuickRestoreManager.swift +++ b/Signal/Provisioning/QuickRestoreManager.swift @@ -80,8 +80,15 @@ public class QuickRestoreManager { case .disabled, .disabling: nil } - let lastBackupTime = backupSettingsStore.lastBackupDate(tx: tx)?.ows_millisecondsSince1970 - let lastBackupSizeBytes = backupSettingsStore.lastBackupSizeBytes(tx: tx) + let lastBackupTime: UInt64? + let lastBackupSizeBytes: UInt64? + if backupTier != nil { + lastBackupTime = backupSettingsStore.lastBackupDate(tx: tx)?.ows_millisecondsSince1970 + lastBackupSizeBytes = backupSettingsStore.lastBackupSizeBytes(tx: tx) + } else { + lastBackupTime = nil + lastBackupSizeBytes = nil + } return ( localIdentifiers, diff --git a/Signal/Registration/RegistrationCoodinatorShims.swift b/Signal/Registration/RegistrationCoodinatorShims.swift index a8a01b8e7c..8d98d3a5b6 100644 --- a/Signal/Registration/RegistrationCoodinatorShims.swift +++ b/Signal/Registration/RegistrationCoodinatorShims.swift @@ -109,6 +109,7 @@ protocol _RegistrationCoordinator_DeviceTransferServiceShim { func addObserver(_ observer: DeviceTransferServiceObserver) func removeObserver(_ observer: DeviceTransferServiceObserver) func stopAcceptingTransfersFromOldDevices() + func cancelTransferFromOldDevice() } class _RegistrationCoordinator_DeviceTransferServiceWrapper: _RegistrationCoordinator_DeviceTransferServiceShim { @@ -132,6 +133,10 @@ class _RegistrationCoordinator_DeviceTransferServiceWrapper: _RegistrationCoordi func stopAcceptingTransfersFromOldDevices() { deviceTransferService.stopAcceptingTransfersFromOldDevices() } + + func cancelTransferFromOldDevice() { + deviceTransferService.cancelTransferFromOldDevice() + } } // MARK: - ExperienceManager diff --git a/Signal/Registration/RegistrationCoordinatorImpl.swift b/Signal/Registration/RegistrationCoordinatorImpl.swift index bbd6ed2e35..7663407b02 100644 --- a/Signal/Registration/RegistrationCoordinatorImpl.swift +++ b/Signal/Registration/RegistrationCoordinatorImpl.swift @@ -538,6 +538,8 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator { } public func resetRestoreMode() -> Guarantee { + inMemoryState.registrationMessage = nil + inMemoryState.accountEntropyPool = nil db.write { tx in self.updatePersistedState(tx) { $0.shouldSkipRegistrationSplash = false diff --git a/Signal/Registration/UserInterface/RegistrationNavigationController.swift b/Signal/Registration/UserInterface/RegistrationNavigationController.swift index aaba03a9d8..d3b68383b8 100644 --- a/Signal/Registration/UserInterface/RegistrationNavigationController.swift +++ b/Signal/Registration/UserInterface/RegistrationNavigationController.swift @@ -734,7 +734,7 @@ extension RegistrationNavigationController: RegistrationQuickRestoreQRCodePresen extension RegistrationNavigationController: RegistrationTransferStatusPresenter { func cancelTransfer() { - let guarantee = coordinator.resetRestoreMethodChoice() + let guarantee = coordinator.resetRestoreMode() pushNextController(guarantee) } } diff --git a/Signal/Registration/UserInterface/RegistrationTransferStatusState.swift b/Signal/Registration/UserInterface/RegistrationTransferStatusState.swift index 780b56c8c6..439f08aa8f 100644 --- a/Signal/Registration/UserInterface/RegistrationTransferStatusState.swift +++ b/Signal/Registration/UserInterface/RegistrationTransferStatusState.swift @@ -21,6 +21,7 @@ public class RegistrationTransferStatusState: DeviceTransferServiceObserver, Equ set { transferStatusViewModel.onCancel = { [weak self] in self?.stopAcceptingTransfers() + self?.cancelTransfer() newValue() } } @@ -60,6 +61,10 @@ public class RegistrationTransferStatusState: DeviceTransferServiceObserver, Equ ) } + private func cancelTransfer() { + deviceTransferService.cancelTransferFromOldDevice() + } + public func stopAcceptingTransfers() { deviceTransferService.removeObserver(self) deviceTransferService.stopAcceptingTransfersFromOldDevices() diff --git a/Signal/Registration/UserInterface/RegistrationTransferStatusViewController.swift b/Signal/Registration/UserInterface/RegistrationTransferStatusViewController.swift index a9eac5f028..8a64db4c47 100644 --- a/Signal/Registration/UserInterface/RegistrationTransferStatusViewController.swift +++ b/Signal/Registration/UserInterface/RegistrationTransferStatusViewController.swift @@ -66,15 +66,12 @@ class RegistrationTransferStatusViewController: HostingController