Merge pull request #1115 from TortugaPower/fix-s3-error

Avoid storing S3 error downloads as files

BookPlayer.xcodeproj/project.pbxproj

@@ -5278,7 +5278,7 @@
 			repositoryURL = "https://github.com/getsentry/sentry-cocoa.git";
 			requirement = {
 				kind = exactVersion;
-				version = 8.7.3;
+				version = 8.21.0;
 			};
 		};
 		41F1A214254B0AA40043FCF3 /* XCRemoteSwiftPackageReference "Kingfisher" */ = {
@@ -5310,7 +5310,7 @@
 			repositoryURL = "https://github.com/RevenueCat/purchases-ios.git";
 			requirement = {
 				kind = upToNextMajorVersion;
-				minimumVersion = 4.0.0;
+				minimumVersion = 4.38.0;
 			};
 		};
 /* End XCRemoteSwiftPackageReference section */

BookPlayer.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

@@ -50,8 +50,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/RevenueCat/purchases-ios.git",
       "state" : {
-        "revision" : "ff4907a2b01fb6e0e891f36e9a14421a5500cc04",
-        "version" : "4.31.9"
+        "revision" : "022748f79384035c0752602300e4e112018cd9c9",
+        "version" : "4.38.1"
       }
     },
     {
@@ -77,8 +77,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/getsentry/sentry-cocoa.git",
       "state" : {
-        "revision" : "9cf7d2e514af1600cc2b3c5592e2848c6c5a76d6",
-        "version" : "8.7.3"
+        "revision" : "38f4f70d07117b9f958a76b1bff278c2f29ffe0e",
+        "version" : "8.21.0"
       }
     },
     {

BookPlayer/AppDelegate.swift

@@ -451,6 +451,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, BPLogger {
       options.debug = false
       options.enableCoreDataTracing = false
       options.enableFileIOTracing = false
+      options.enableAppHangTracking = false
       options.tracesSampleRate = 0.5
     }
   }

BookPlayer/Coordinators/DataInitializerCoordinator.swift

@@ -36,11 +36,13 @@ class DataInitializerCoordinator: BPLogger {
       || error.code == NSFileWriteOutOfSpaceError
     ) {
       // CoreData may fail if device doesn't have space
-      alertPresenter.showAlert(
-        "error_title".localized,
-        message: "coredata_error_diskfull_description".localized,
-        completion: nil
-      )
+      await MainActor.run {
+        alertPresenter.showAlert(
+          "error_title".localized,
+          message: "coredata_error_diskfull_description".localized,
+          completion: nil
+        )
+      }
     } catch let error as NSError where (
       error.code == NSMigrationError ||
       error.code == NSMigrationConstraintViolationError ||
@@ -55,11 +57,13 @@ class DataInitializerCoordinator: BPLogger {
     ) {
       // TODO: We can handle `isRecoveryAttempt` to show a different error message
       Self.logger.warning("Failed to perform migration, attempting recovery with the loading library sequence")
-      alertPresenter.showAlert(
-        "error_title".localized,
-        message: "coredata_error_migration_description".localized
-      ) { [unowned self] in
-        recoverLibraryFromFailedMigration()
+      await MainActor.run {
+        alertPresenter.showAlert(
+          "error_title".localized,
+          message: "coredata_error_migration_description".localized
+        ) { [unowned self] in
+          recoverLibraryFromFailedMigration()
+        }
       }
     } catch {
       let error = error as NSError

BookPlayer/Coordinators/LibraryListCoordinator.swift

@@ -85,6 +85,7 @@ class LibraryListCoordinator: ItemListCoordinator, UINavigationControllerDelegat
     loadLastBookIfNeeded()
     syncList()
     bindImportObserverIfNeeded()
+    bindDownloadErrorObserver()
 
     if let appDelegate = AppDelegate.shared {
       for action in appDelegate.pendingURLActions {
@@ -136,6 +137,13 @@ class LibraryListCoordinator: ItemListCoordinator, UINavigationControllerDelegat
     notifyPendingFiles()
   }
 
+  func bindDownloadErrorObserver() {
+    syncService.downloadErrorPublisher.sink { (relativePath, error) in
+      self.showAlert("network_error_title".localized, message: "\(relativePath)\n\(error.localizedDescription)")
+    }
+    .store(in: &disposeBag)
+  }
+
   @MainActor
   func notifyPendingFiles() {
     // Get reference of all the files located inside the Documents, Shared and Inbox folders

BookPlayer/Generated/AutoMockable.generated.swift

@@ -1414,6 +1414,11 @@ class SyncServiceProtocolMock: SyncServiceProtocol {
         set(value) { underlyingDownloadProgressPublisher = value }
     }
     var underlyingDownloadProgressPublisher: PassthroughSubject<(String, String, String?, Double), Never>!
+    var downloadErrorPublisher: PassthroughSubject<(String, Error), Never> {
+        get { return underlyingDownloadErrorPublisher }
+        set(value) { underlyingDownloadErrorPublisher = value }
+    }
+    var underlyingDownloadErrorPublisher: PassthroughSubject<(String, Error), Never>!
     //MARK: - queuedJobsCount
 
     var queuedJobsCountCallsCount = 0

BookPlayer/Library/ItemList Screen/ItemListViewModel.swift

@@ -211,12 +211,12 @@ class ItemListViewModel: ViewModelProtocol {
       ))
     }
 
-    singleDownloadProgressDelegateInterface.didFinishDownloadingTask = { [weak self] (task, fileURL) in
-      self?.handleSingleDownloadTaskFinished(task, fileURL: fileURL)
-    }
-
-    singleDownloadProgressDelegateInterface.didFinishTaskWithError = { [weak self] (task, error) in
-      self?.handleSingleDownloadTaskFinishedWithError(task, error: error)
+    singleDownloadProgressDelegateInterface.didFinishDownloadingTask = { [weak self] (task, fileURL, error) in
+      if let error {
+        self?.handleSingleDownloadTaskFinishedWithError(task, error: error)
+      } else if let fileURL {
+        self?.handleSingleDownloadTaskFinished(task, fileURL: fileURL)
+      }
     }
   }
 

BookPlayer/Settings/Storage/StorageViewModel.swift

@@ -183,17 +183,19 @@ final class StorageViewModel: StorageViewModelProtocol {
     // Check if existing book already has its file, and this one is a duplicate
     if FileManager.default.fileExists(atPath: fetchedBookURL.path) {
       try FileManager.default.removeItem(at: item.fileURL)
-      onTransition?(.showAlert(
-        BPAlertContent(
-          title: "storage_duplicate_item_title".localized,
-          message: String.localizedStringWithFormat("storage_duplicate_item_description".localized, fetchedBook.relativePath!),
-          style: .alert,
-          actionItems: [
-            BPActionItem.okAction
-          ]
-        )
-      ))
       if shouldReloadItems {
+        let alertMessage = String.localizedStringWithFormat("storage_duplicate_item_description".localized, fetchedBook.relativePath!)
+        /// only show alert when doing individual fix
+        self.onTransition?(.showAlert(
+          BPAlertContent(
+            title: "storage_duplicate_item_title".localized,
+            message: alertMessage,
+            style: .alert,
+            actionItems: [
+              BPActionItem.okAction
+            ]
+          )
+        ))
         self.loadItems()
       }
       return
@@ -232,20 +234,14 @@ final class StorageViewModel: StorageViewModelProtocol {
     guard !brokenItems.isEmpty else { return }
 
     showProgressIndicator = true
-    DispatchQueue.global().async {
-      do {
-        try self.handleFix(for: brokenItems) {
-          DispatchQueue.main.async { [weak self] in
-            self?.showProgressIndicator = false
-          }
-        }
-      } catch {
-        DispatchQueue.main.async { [weak self] in
-          self?.showProgressIndicator = false
-          self?.storageAlert = .error(errorMessage: error.localizedDescription)
-          self?.showAlert = true
-        }
+    do {
+      try handleFix(for: brokenItems) { [weak self] in
+        self?.showProgressIndicator = false
       }
+    } catch {
+      showProgressIndicator = false
+      storageAlert = .error(errorMessage: error.localizedDescription)
+      showAlert = true
     }
   }
 
@@ -256,8 +252,8 @@ final class StorageViewModel: StorageViewModelProtocol {
   // MARK: - Private functions
 
   private func loadItems() {
-    showProgressIndicator = true
     Task { @MainActor in
+      showProgressIndicator = true
       let processedFolder = self.folderURL
 
       let enumerator = FileManager.default.enumerator(
@@ -391,18 +387,20 @@ final class StorageViewModel: StorageViewModelProtocol {
   }
 
   func verifyUploadTask(for item: StorageItem, completionHandler: @escaping () -> Void) {
-    Task { @MainActor in
+    Task {
       if await syncService.hasUploadTask(for: item.path) {
-        onTransition?(.showAlert(
-          BPAlertContent(
-            title: "warning_title".localized,
-            message: String(format: "sync_tasks_item_upload_queued".localized, item.path),
-            style: .alert,
-            actionItems: [
-              BPActionItem.cancelAction,
-              BPActionItem(title: "Continue", handler: completionHandler)
-            ])
-        ))
+        await MainActor.run {
+          onTransition?(.showAlert(
+            BPAlertContent(
+              title: "warning_title".localized,
+              message: String(format: "sync_tasks_item_upload_queued".localized, item.path),
+              style: .alert,
+              actionItems: [
+                BPActionItem.cancelAction,
+                BPActionItem(title: "Continue", handler: completionHandler)
+              ])
+          ))
+        }
       } else {
         completionHandler()
       }

Shared/Network/BPDownloadURLSession.swift

@@ -19,7 +19,7 @@ public class BPDownloadURLSession {
   ///   - didFinishDownloadingTask: Callback triggered when the download task is finished
   public init(
     downloadProgressUpdated: @escaping ((URLSessionDownloadTask, Double) -> Void),
-    didFinishDownloadingTask: @escaping ((URLSessionDownloadTask, URL) -> Void)
+    didFinishDownloadingTask: @escaping ((URLSessionTask, URL?, Error?) -> Void)
   ) {
     let bundleIdentifier: String = Bundle.main.configurationValue(for: .bundleIdentifier)
 

Shared/Network/BPTaskDownloadDelegate.swift

@@ -10,10 +10,7 @@ import Foundation
 
 public class BPTaskDownloadDelegate: NSObject, URLSessionDownloadDelegate {
   /// Callback triggered when the download task is finished
-  public var didFinishDownloadingTask: ((URLSessionDownloadTask, URL) -> Void)?
-  /// Callback triggered when the download task fails
-  /// - Note: the Error parameter represents client side errors
-  public var didFinishTaskWithError: ((URLSessionTask, Error?) -> Void)?
+  public var didFinishDownloadingTask: ((URLSessionTask, URL?, Error?) -> Void)?
   /// Callback triggered when there's an update on the download progress
   public var downloadProgressUpdated: ((URLSessionDownloadTask, Double) -> Void)?
   /// Delegate callback when download finishes
@@ -22,12 +19,21 @@ public class BPTaskDownloadDelegate: NSObject, URLSessionDownloadDelegate {
     downloadTask: URLSessionDownloadTask,
     didFinishDownloadingTo location: URL
   ) {
-    didFinishDownloadingTask?(downloadTask, location)
+    didFinishDownloadingTask?(
+      downloadTask,
+      location,
+      parseErrorFromTask(downloadTask)
+    )
   }
 
   /// Note: this gets called even if there's no error
-  public func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) {
-    didFinishTaskWithError?(task, error)
+  public func urlSession(
+    _ session: URLSession,
+    task: URLSessionTask,
+    didCompleteWithError error: Error?
+  ) {
+    let error = error ?? parseErrorFromTask(task)
+    didFinishDownloadingTask?(task, nil, error)
   }
 
   /// Delegate callback when there's a progress update for the ongoing download
@@ -42,4 +48,16 @@ public class BPTaskDownloadDelegate: NSObject, URLSessionDownloadDelegate {
 
     downloadProgressUpdated?(downloadTask, Double(calculatedProgress))
   }
+
+  private func parseErrorFromTask(_ task: URLSessionTask) -> Error? {
+    guard
+      let response = task.response as? HTTPURLResponse,
+      response.statusCode >= 400
+    else {
+      return nil
+    }
+
+    let errorCode = URLError.Code(rawValue: response.statusCode)
+    return URLError(errorCode)
+  }
 }

Shared/Services/Sync/SyncService.swift

@@ -27,6 +27,8 @@ public protocol SyncServiceProtocol {
   var downloadCompletedPublisher: PassthroughSubject<(String, String, String?), Never> { get }
   /// Progress publisher for ongoing-download tasks
   var downloadProgressPublisher: PassthroughSubject<(String, String, String?, Double), Never> { get }
+  /// Error publisher for ongoing-download tasks
+  var downloadErrorPublisher: PassthroughSubject<(String, Error), Never> { get }
 
   /// Count of the currently queued sync jobs
   func queuedJobsCount() async -> Int
@@ -105,17 +107,20 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
   public var downloadCompletedPublisher = PassthroughSubject<(String, String, String?), Never>()
   /// Progress publisher for ongoing-download tasks
   public var downloadProgressPublisher = PassthroughSubject<(String, String, String?, Double), Never>()
+  /// Error publisher for ongoing-download tasks
+  public var downloadErrorPublisher = PassthroughSubject<(String, Error), Never>()
   /// Background URL session to handle downloading synced items
   private lazy var downloadURLSession: BPDownloadURLSession = {
     BPDownloadURLSession { task, progress in
       self.handleDownloadProgressUpdated(
         task: task,
         individualProgress: progress
       )
-    } didFinishDownloadingTask: { task, location in
+    } didFinishDownloadingTask: { task, location, error in
       self.handleFinishedDownload(
         task: task,
-        location: location
+        location: location,
+        error: error
       )
     }
   }()
@@ -547,26 +552,39 @@ extension SyncService {
 }
 
 extension SyncService {
-  private func handleFinishedDownload(task: URLSessionTask, location: URL) {
+  private func handleFinishedDownload(
+    task: URLSessionTask,
+    location: URL?,
+    error: Error?
+  ) {
     guard let relativePath = task.taskDescription else { return }
 
     do {
-      let fileURL = DataManager.getProcessedFolderURL().appendingPathComponent(relativePath)
+      if error == nil,
+         let location {
+        let fileURL = DataManager.getProcessedFolderURL().appendingPathComponent(relativePath)
 
-      /// If there's already something there, replace with new finished download
-      if FileManager.default.fileExists(atPath: fileURL.path) {
-        try FileManager.default.removeItem(at: fileURL)
-      }
-      try DataManager.createContainingFolderIfNeeded(for: fileURL)
-      try FileManager.default.moveItem(at: location, to: fileURL)
+        /// If there's already something there, replace with new finished download
+        if FileManager.default.fileExists(atPath: fileURL.path) {
+          try FileManager.default.removeItem(at: fileURL)
+        }
+        try DataManager.createContainingFolderIfNeeded(for: fileURL)
+        try FileManager.default.moveItem(at: location, to: fileURL)
 
-      Task {
-        await self.libraryService.loadChaptersIfNeeded(relativePath: relativePath)
+        Task {
+          await self.libraryService.loadChaptersIfNeeded(relativePath: relativePath)
+        }
       }
     } catch {
       Self.logger.trace("Error moving downloaded file to the destination: \(error.localizedDescription)")
     }
 
+    if let error {
+      DispatchQueue.main.async {
+        self.downloadErrorPublisher.send((relativePath, error))
+      }
+    }
+
     guard let startingItemPath = ongoingTasksParentReference[relativePath] else {
       initiatingFolderReference[relativePath] = nil
       return