Address review comments on sync_with_source

Raise a validation error when sync_with_source is combined with
keep_files_before, keep_files_after or keep_max_files, rather than
silently dropping the max_downloads cap that keep_max_files injects.

Move the source id guard into remove_entries_not_in_source, since the
archive class already holds the ids.

break_on_existing is still forced off for the metadata fetch, because the
prebuilt tv_show, singles and music_video presets set it themselves. It
now logs when it does.

Author: sapphimars

docs/source/config_reference/plugins.rst

@@ -755,17 +755,18 @@ Defines where to output files and thumbnails after all post-processing has compl
 
 :expected type: Optional[OverridesFormatter]
 :description:
-    Requires ``maintain_download_archive`` set to True.
+    Requires ``maintain_download_archive`` set to True. Cannot be used with
+    ``keep_files_before``, ``keep_files_after``, or ``keep_max_files``, since those
+    deliberately stop metadata collection early.
 
     Deletes files whose source entry is no longer present in the subscription's URL(s).
     After the metadata pass, any entry in the download archive whose ID is absent from
     the source is removed, along with all of its files.
 
-    This forces a metadata fetch of every URL on each invocation. ytdl-sub cannot
-    tell the difference between "this video was removed" and "metadata collection stopped
-    early", so ``break_on_existing`` and ``keep_max_files``' download cap are both
-    disabled during the metadata pass. Only enable this on sources you expect to change,
-    and expect slower runs on large playlists.
+    This forces a metadata fetch of every URL on each invocation. ytdl-sub cannot tell
+    the difference between "this video was removed" and "metadata collection stopped
+    early", so ``break_on_existing`` is disabled for the metadata fetch. Only enable
+    this on sources you expect to change, and expect slower runs on large playlists.
 
     If metadata collection is truncated for a reason ytdl-sub cannot override (such as
     ``date_range`` with ``breaks`` enabled, or a user-set ``max_downloads``), or if the

src/ytdl_sub/config/preset_options.py

@@ -212,6 +212,16 @@ def __init__(self, name, value):
                 "keep_files/keep_max/sync_with_source requires maintain_download_archive set to True"
             )
 
+        # sync_with_source needs the entire source. keep_max_files caps metadata collection,
+        # and the keep_files options are a competing retention policy
+        if self._sync_with_source and (
+            self._keep_files_before or self._keep_files_after or self._keep_max_files
+        ):
+            raise self._validation_exception(
+                "sync_with_source cannot be used with keep_files_before, keep_files_after, or "
+                "keep_max_files"
+            )
+
     @property
     def output_directory(self) -> OverridesStringFormatterValidator:
         """
@@ -370,17 +380,18 @@ def sync_with_source(self) -> Optional[OverridesBooleanFormatterValidator]:
         """
         :expected type: Optional[OverridesFormatter]
         :description:
-            Requires ``maintain_download_archive`` set to True.
+            Requires ``maintain_download_archive`` set to True. Cannot be used with
+            ``keep_files_before``, ``keep_files_after``, or ``keep_max_files``, since those
+            deliberately stop metadata collection early.
 
             Deletes files whose source entry is no longer present in the subscription's URL(s).
             After the metadata pass, any entry in the download archive whose ID is absent from
             the source is removed, along with all of its files.
 
-            This forces a metadata fetch of every URL on each invocation. ytdl-sub cannot
-            tell the difference between "this video was removed" and "metadata collection stopped
-            early", so ``break_on_existing`` and ``keep_max_files``' download cap are both
-            disabled during the metadata pass. Only enable this on sources you expect to change,
-            and expect slower runs on large playlists.
+            This forces a metadata fetch of every URL on each invocation. ytdl-sub cannot tell
+            the difference between "this video was removed" and "metadata collection stopped
+            early", so ``break_on_existing`` is disabled for the metadata fetch. Only enable
+            this on sources you expect to change, and expect slower runs on large playlists.
 
             If metadata collection is truncated for a reason ytdl-sub cannot override (such as
             ``date_range`` with ``breaks`` enabled, or a user-set ``max_downloads``), or if the

src/ytdl_sub/subscriptions/subscription_download.py

@@ -147,21 +147,7 @@ def _maintain_archive_file(self):
             if self.output_options.sync_with_source and self.overrides.apply_formatter(
                 self.output_options.sync_with_source, expected_type=bool
             ):
-                source_entry_ids = self.download_archive.source_entry_ids
-
-                # A source that returned nothing is indistinguishable from one that was never
-                # enumerated, so neither is treated as every entry having been removed
-                if source_entry_ids is None:
-                    logger.warning(
-                        "sync_with_source: the source returned no entries or was not fully "
-                        "enumerated, skipping sync to avoid deleting files. This happens when "
-                        "metadata collection stops early, i.e. from `date_range.breaks` or a "
-                        "user set `max_downloads`."
-                    )
-                else:
-                    self.download_archive.remove_entries_not_in_source(
-                        source_entry_ids=source_entry_ids
-                    )
+                self.download_archive.remove_entries_not_in_source()
 
             date_range_to_keep = to_date_range(
                 before=self.output_options.keep_files_before,

src/ytdl_sub/subscriptions/subscription_ytdl_options.py

@@ -97,10 +97,7 @@ def _output_options(self) -> Dict:
                 self._enhanced_download_archive.working_ytdl_file_path
             )
 
-        # sync_with_source needs a full enumeration of the source,
-        # max_downloads would truncate the metadata pass and make
-        # present entries look removed.
-        if self._preset.output_options.keep_max_files and not self._sync_with_source:
+        if self._preset.output_options.keep_max_files:
             keep_max_files = self._overrides.apply_formatter(
                 self._preset.output_options.keep_max_files, expected_type=int
             )
@@ -115,9 +112,11 @@ def _sync_with_source_options(self) -> Dict:
         if not self._sync_with_source:
             return {}
 
-        # stopping at the first alreadt downloaded entry would hide
-        # the rest of the soure, which sync_with_source would then
-        # interpret as deleted entries
+        # stopping at the first already downloaded entry would hide the rest of the source,
+        # which sync_with_source would then interpret as deleted entries
+        logger.info(
+            "sync_with_source is enabled, disabling break_on_existing to fetch the entire source"
+        )
         return {"break_on_existing": False}
 
     def _plugin_ytdl_options(self, plugin: Type[PluginT]) -> Dict:

src/ytdl_sub/ytdl_additions/enhanced_download_archive.py

@@ -684,20 +684,24 @@ def remove_stale_files(
 
         return self
 
-    def remove_entries_not_in_source(self, source_entry_ids: Set[str]) -> "EnhancedDownloadArchive":
+    def remove_entries_not_in_source(self) -> "EnhancedDownloadArchive":
         """
-        Checks all entries within mappings. If any entry is no longer present
-        in the source, delete it.
-
-        Parameters
-        ----------
-        source_entry_ids
-            Every entry ID present in the source
+        Checks all entries within the mappings. If any entry is no longer in the source, delete
+        it. Does nothing unless the source was fully enumerated, since a source that returned
+        nothing looks the same as one that was never enumerated.
 
         Returns
         -------
         self
         """
+        if (source_entry_ids := self.source_entry_ids) is None:
+            logger.warning(
+                "sync_with_source: the source returned no entries or stopped early, skipping "
+                "sync to avoid deleting files. Can be caused by `date_range.breaks` or "
+                "`max_downloads`."
+            )
+            return self
+
         stale_mappings: Dict[str, DownloadMapping] = {
             uid: mapping
             for uid, mapping in self.mapping.entry_mappings.items()

tests/unit/test_sync_with_source.py

@@ -29,6 +29,12 @@ def _make_archive(tmp_path, mappings_dict, dry_run: bool = False):
     return archive
 
 
+def _record_source(archive, source_entry_ids):
+    for entry_id in source_entry_ids:
+        archive.record_source_entry_id(entry_id=entry_id)
+    return archive
+
+
 def _mappings():
     return {
         "id1": DownloadMapping("2024-01-01", "yt", {"a.mp4"}),
@@ -40,29 +46,33 @@ def _mappings():
 class TestRemoveEntriesNotInSource:
     def test_entry_removed_from_source_is_pruned(self, tmp_path):
         archive = _make_archive(tmp_path, _mappings())
-        archive.remove_entries_not_in_source(source_entry_ids={"id1", "id3"})
+        _record_source(archive, {"id1", "id3"})
+        archive.remove_entries_not_in_source()
 
         assert sorted(archive.mapping.entry_mappings.keys()) == ["id1", "id3"]
         assert not (tmp_path / "output" / "b.mp4").exists()
 
     def test_entry_still_in_source_is_untouched(self, tmp_path):
         archive = _make_archive(tmp_path, _mappings())
-        archive.remove_entries_not_in_source(source_entry_ids={"id1", "id3"})
+        _record_source(archive, {"id1", "id3"})
+        archive.remove_entries_not_in_source()
 
         assert (tmp_path / "output" / "a.mp4").exists()
         assert (tmp_path / "output" / "c.mp4").exists()
         assert archive.num_entries_removed == 1
 
     def test_all_entries_in_source_removes_nothing(self, tmp_path):
         archive = _make_archive(tmp_path, _mappings())
-        archive.remove_entries_not_in_source(source_entry_ids={"id1", "id2", "id3"})
+        _record_source(archive, {"id1", "id2", "id3"})
+        archive.remove_entries_not_in_source()
 
         assert sorted(archive.mapping.entry_mappings.keys()) == ["id1", "id2", "id3"]
         assert archive.num_entries_removed == 0
 
     def test_source_with_new_entries_removes_nothing(self, tmp_path):
         archive = _make_archive(tmp_path, _mappings())
-        archive.remove_entries_not_in_source(source_entry_ids={"id1", "id2", "id3", "id4"})
+        _record_source(archive, {"id1", "id2", "id3", "id4"})
+        archive.remove_entries_not_in_source()
 
         assert sorted(archive.mapping.entry_mappings.keys()) == ["id1", "id2", "id3"]
         assert archive.num_entries_removed == 0
@@ -77,16 +87,34 @@ def test_all_files_for_entry_are_deleted(self, tmp_path):
             "id2": DownloadMapping("2024-01-02", "yt", {"b.mp4"}),
         }
         archive = _make_archive(tmp_path, mappings)
-        archive.remove_entries_not_in_source(source_entry_ids={"id2"})
+        _record_source(archive, {"id2"})
+        archive.remove_entries_not_in_source()
 
         for file_name in ("a.mp4", "a.nfo", "a-thumb.jpg", "a.info.json"):
             assert not (tmp_path / "output" / file_name).exists()
         assert (tmp_path / "output" / "b.mp4").exists()
 
     def test_dry_run_does_not_delete_files(self, tmp_path):
         archive = _make_archive(tmp_path, _mappings(), dry_run=True)
-        archive.remove_entries_not_in_source(source_entry_ids={"id1", "id3"})
+        _record_source(archive, {"id1", "id3"})
+        archive.remove_entries_not_in_source()
+
+        assert (tmp_path / "output" / "b.mp4").exists()
+
+    def test_unenumerated_source_deletes_nothing(self, tmp_path):
+        archive = _make_archive(tmp_path, _mappings())
+        archive.remove_entries_not_in_source()
+
+        assert sorted(archive.mapping.entry_mappings.keys()) == ["id1", "id2", "id3"]
+        assert (tmp_path / "output" / "b.mp4").exists()
+
+    def test_truncated_source_deletes_nothing(self, tmp_path):
+        archive = _make_archive(tmp_path, _mappings())
+        _record_source(archive, {"id1"})
+        archive.mark_source_enumeration_truncated(reason="ExistingVideoReached")
+        archive.remove_entries_not_in_source()
 
+        assert sorted(archive.mapping.entry_mappings.keys()) == ["id1", "id2", "id3"]
         assert (tmp_path / "output" / "b.mp4").exists()
 
 
@@ -137,3 +165,23 @@ def test_accepts_override(self):
     def test_requires_maintain_download_archive(self):
         with pytest.raises(ValidationException, match="maintain_download_archive"):
             OutputOptions("t", self._base | {"sync_with_source": True})
+
+    @pytest.mark.parametrize(
+        "keep_option, keep_value",
+        [
+            ("keep_files_before", "now"),
+            ("keep_files_after", "19000101"),
+            ("keep_max_files", 10),
+        ],
+    )
+    def test_cannot_be_used_with_keep_options(self, keep_option, keep_value):
+        with pytest.raises(ValidationException, match="cannot be used with"):
+            OutputOptions(
+                "t",
+                self._base
+                | {
+                    "maintain_download_archive": True,
+                    "sync_with_source": True,
+                    keep_option: keep_value,
+                },
+            )