remove local retries during downloads

Author: AndreyNikiforov

src/icloudpd/base.py

@@ -1238,6 +1238,10 @@ def should_break(counter: Counter) -> bool:
             logger.debug("Retrying...")
             # these errors we can safely retry
             continue
+        except OSError as error:
+            logger.error("IOError during file operations: %s", error)
+            dump_responses(logger.debug, captured_responses)
+            return 1
         except Exception:
             dump_responses(logger.debug, captured_responses)
             raise

src/icloudpd/download.py

@@ -12,10 +12,8 @@
 from tzlocal import get_localzone
 
 # Import the constants object so that we can mock WAIT_SECONDS in tests
-from icloudpd import constants
 from pyicloud_ipd.asset_version import AssetVersion, calculate_version_filename
 from pyicloud_ipd.base import PyiCloudService
-from pyicloud_ipd.exceptions import PyiCloudAPIResponseException
 from pyicloud_ipd.services.photos import PhotoAsset
 from pyicloud_ipd.version_size import VersionSize
 
@@ -128,75 +126,29 @@ def download_media(
         partial(download_response_to_path_dry_run, logger) if dry_run else download_response_to_path
     )
 
-    retries = 0
-    while True:
-        try:
-            append_mode = os.path.exists(temp_download_path)
-            current_size = os.path.getsize(temp_download_path) if append_mode else 0
-            if append_mode:
-                logger.debug(f"Resuming downloading of {download_path} from {current_size}")
-
-            photo_response = photo.download(icloud.photos.session, version.url, current_size)
-            if photo_response.ok:
-                return download_local(
-                    photo_response, temp_download_path, append_mode, download_path, photo.created
-                )
-            else:
-                # Use the standard original filename generator for error logging
-                from icloudpd.base import lp_filename_original as simple_lp_filename_generator
-
-                # Get the proper filename using filename_builder
-                base_filename = filename_builder(photo)
-                version_filename = calculate_version_filename(
-                    base_filename, version, size, simple_lp_filename_generator, photo.item_type
-                )
-                logger.error(
-                    "Could not find URL to download %s for size %s",
-                    version_filename,
-                    size.value,
-                )
-                break
-
-        except PyiCloudAPIResponseException as ex:
-            if "Invalid global session" in str(ex):
-                error_filename = filename_builder(photo)
-                logger.error(
-                    "Could not download %s. Please try again later.",
-                    error_filename,
-                )
-
-                raise
-            else:
-                # short circuiting 0 retries
-                if retries == constants.MAX_RETRIES:
-                    break
-                # you end up here when p.e. throttling by Apple happens
-                wait_time = (retries + 1) * constants.WAIT_SECONDS
-                # Get the proper filename for error messages
-                error_filename = filename_builder(photo)
-                logger.error(
-                    "Error downloading %s, retrying after %s seconds...", error_filename, wait_time
-                )
-                time.sleep(wait_time)
-
-        except OSError:
-            logger.error(
-                "IOError while writing file to %s. "
-                + "You might have run out of disk space, or the file "
-                + "might be too large for your OS. "
-                + "Skipping this file...",
-                download_path,
-            )
-            break
-        retries = retries + 1
-        if retries >= constants.MAX_RETRIES:
-            break
-    if retries >= constants.MAX_RETRIES:
-        # Get the proper filename for error messages
-        error_filename = filename_builder(photo)
+    append_mode = os.path.exists(temp_download_path)
+    current_size = os.path.getsize(temp_download_path) if append_mode else 0
+    if append_mode:
+        logger.debug(f"Resuming downloading of {download_path} from {current_size}")
+
+    photo_response = photo.download(icloud.photos.session, version.url, current_size)
+    if photo_response.ok:
+        return download_local(
+            photo_response, temp_download_path, append_mode, download_path, photo.created
+        )
+    else:
+        # Use the standard original filename generator for error logging
+        from icloudpd.base import lp_filename_original as simple_lp_filename_generator
+
+        # Get the proper filename using filename_builder
+        base_filename = filename_builder(photo)
+        version_filename = calculate_version_filename(
+            base_filename, version, size, simple_lp_filename_generator, photo.item_type
+        )
         logger.error(
-            "Could not download %s. Please try again later.",
-            error_filename,
+            "Could not find URL to download %s for size %s",
+            version_filename,
+            size.value,
         )
 
-    return False
+        return False

tests/test_download_photos.py

@@ -423,14 +423,18 @@ def test_handle_io_error(self) -> None:
                 f"Downloading the first original photo to {data_dir} ...",
                 result.output,
             )
+            # self.assertIn(
+            #     "IOError while writing file to "
+            #     f"{os.path.join(data_dir, os.path.normpath('2018/07/31/IMG_7409.JPG'))}. "
+            #     "You might have run out of disk space, or the file might "
+            #     "be too large for your OS. Skipping this file...",
+            #     result.output,
+            # )
             self.assertIn(
-                "IOError while writing file to "
-                f"{os.path.join(data_dir, os.path.normpath('2018/07/31/IMG_7409.JPG'))}. "
-                "You might have run out of disk space, or the file might "
-                "be too large for your OS. Skipping this file...",
+                "IOError",
                 result.output,
             )
-            assert result.exit_code == 0
+            assert result.exit_code == 1
 
     def test_handle_session_error_during_download(self) -> None:
         base_dir = os.path.join(self.fixtures_path, inspect.stack()[0][3])
@@ -495,10 +499,10 @@ def mocked_authenticate(self: PyiCloudService) -> None:
                         "retry count",
                     )
 
-                    self.assertIn(
-                        "Could not download IMG_7409.JPG. Please try again later.",
-                        result.output,
-                    )
+                    # self.assertIn(
+                    #     "Could not download IMG_7409.JPG. Please try again later.",
+                    #     result.output,
+                    # )
 
                     # Make sure we only call sleep 4 times (skip the first retry)
                     self.assertEqual(
@@ -1702,20 +1706,16 @@ def mock_raise_response_error(_arg: Any, _session: Any, _size: Any) -> NoReturn:
                 )
 
                 # Error msg should be repeated 5 times
-                self.assertEqual(
-                    result.output.count("Error downloading"),
-                    constants.MAX_RETRIES,
-                    "retry count",
-                )
+                self.assertIn("INTERNAL_ERROR", result.output)
 
-                self.assertIn(
-                    "Could not download IMG_7409.JPG. Please try again later.",
-                    result.output,
-                )
+                # self.assertIn(
+                #     "Could not download IMG_7409.JPG. Please try again later.",
+                #     result.output,
+                # )
 
                 # Make sure we only call sleep 4 times (skip the first retry)
                 self.assertEqual(sleep_mock.call_count, constants.MAX_RETRIES, "sleep count")
-                self.assertEqual(result.exit_code, 0, "Exit Code")
+                self.assertEqual(result.exit_code, 1, "Exit Code")
 
     def test_handle_internal_error_during_photo_iteration(self) -> None:
         base_dir = os.path.join(self.fixtures_path, inspect.stack()[0][3])

tests/test_download_photos_id.py

@@ -416,13 +416,10 @@ def test_handle_io_error_name_id7(self) -> None:
                 result.output,
             )
             self.assertIn(
-                "IOError while writing file to "
-                f"{os.path.join(data_dir, os.path.normpath('2018/07/31/IMG_7409_QVk2Yyt.JPG'))}. "
-                "You might have run out of disk space, or the file might "
-                "be too large for your OS. Skipping this file...",
+                "IOError",
                 result.output,
             )
-            assert result.exit_code == 0
+            assert result.exit_code == 1
 
     def test_handle_session_error_during_download_name_id7(self) -> None:
         base_dir = os.path.join(self.fixtures_path, inspect.stack()[0][3])
@@ -487,10 +484,10 @@ def mocked_authenticate(self: PyiCloudService) -> None:
                         "retry count",
                     )
 
-                    self.assertIn(
-                        "Could not download IMG_7409_QVk2Yyt.JPG. Please try again later.",
-                        result.output,
-                    )
+                    # self.assertIn(
+                    #     "Could not download IMG_7409_QVk2Yyt.JPG. Please try again later.",
+                    #     result.output,
+                    # )
 
                     # Make sure we only call sleep 4 times (skip the first retry)
                     self.assertEqual(
@@ -1622,20 +1619,16 @@ def mock_raise_response_error(_arg: Any, _session: Any, _size: Any) -> NoReturn:
                 )
 
                 # Error msg should be repeated 5 times
-                self.assertEqual(
-                    result.output.count("Error downloading"),
-                    constants.MAX_RETRIES,
-                    "retry count",
-                )
+                self.assertIn("INTERNAL_ERROR", result.output)
 
-                self.assertIn(
-                    "Could not download IMG_7409_QVk2Yyt.JPG. Please try again later.",
-                    result.output,
-                )
+                # self.assertIn(
+                #     "Could not download IMG_7409_QVk2Yyt.JPG. Please try again later.",
+                #     result.output,
+                # )
 
                 # Make sure we only call sleep 4 times (skip the first retry)
                 self.assertEqual(sleep_mock.call_count, constants.MAX_RETRIES, "sleep count")
-                self.assertEqual(result.exit_code, 0, "Exit Code")
+                self.assertEqual(result.exit_code, 1, "Exit Code")
 
     def test_handle_internal_error_during_photo_iteration_name_id7(self) -> None:
         base_dir = os.path.join(self.fixtures_path, inspect.stack()[0][3])