From 8ac98c399dc9dff8df4e4402b0a1c2465ac80e7e Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sat, 17 Oct 2026 18:34:26 +1100 Subject: [PATCH 01/11] fix: preserve upstream media and validate deletion paths\n --- tubesync/sync/models/source.py | 7 +- tubesync/sync/tasks.py | 16 +-- tubesync/sync/tests/test_index_coverage.py | 29 +++++ .../tests/test_source_deletion_boundary.py | 113 ++++++++++++++++++ 4 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 tubesync/sync/tests/test_index_coverage.py create mode 100644 tubesync/sync/tests/test_source_deletion_boundary.py diff --git a/tubesync/sync/models/source.py b/tubesync/sync/models/source.py index 3dd7f1bbb..f035fba75 100644 --- a/tubesync/sync/models/source.py +++ b/tubesync/sync/models/source.py @@ -652,11 +652,13 @@ def get_index(self, url_type, /): def index_media(self): ''' - Index the media source returning a queue of media metadata as dicts. + Return the capped processing queue and every upstream media key. ''' entries = queue(list(), getattr(settings, 'MAX_ENTRIES_PROCESSING', 0) or None) + upstream_keys = set() if self.index_videos: videos = self.get_index('videos') + upstream_keys.update(item.get(self.key_field) for item in videos if item.get(self.key_field)) entries.extend(reversed(videos)) # Playlists do something different that I have yet to figure out @@ -664,6 +666,7 @@ def index_media(self): if not self.is_playlist: if self.index_streams: streams = self.get_index('streams') + upstream_keys.update(item.get(self.key_field) for item in streams if item.get(self.key_field)) if entries.maxlen is None or 0 == len(entries): entries.extend(reversed(streams)) else: @@ -674,4 +677,4 @@ def index_media(self): ) entries.extend(reversed(streams[: allowed_streams])) - return entries + return entries, upstream_keys diff --git a/tubesync/sync/tasks.py b/tubesync/sync/tasks.py index 6f171d203..6690c42cf 100644 --- a/tubesync/sync/tasks.py +++ b/tubesync/sync/tasks.py @@ -553,7 +553,7 @@ def index_source(source_id): # Reset any errors source.has_failed = False # Index the source - videos = source.index_media() + videos, video_keys = source.index_media() if not videos: source.has_failed = True update_model(source, has_failed=source.has_failed) @@ -593,7 +593,6 @@ def index_source(source_id): end=task.verbose_name.find('Index'), ) vn = 0 - video_keys = set() while len(videos) > 0: vn += 1 video = videos.popleft() @@ -602,7 +601,6 @@ def index_source(source_id): if not key: # Video has no unique key (ID), it can't be indexed continue - video_keys.add(key) if len(db_batch_data) == db_batch_data.maxlen: save_db_batch(Metadata.objects, db_batch_data, db_fields_data) if len(db_batch_media) == db_batch_media.maxlen: @@ -1405,10 +1403,15 @@ def save_all_media_for_source(source_id): @db_task(delay=90, priority=99, queue=Val(TaskQueue.FS)) def delete_all_media_for_source(source_id, source_name, source_directory): + if not source_id or not source_name or not source_directory: + raise ValueError('Source ID, name and directory are required') + directory_path = Path(source_directory) + download_root = Path(settings.DOWNLOAD_ROOT).resolve() + resolved_directory = directory_path.resolve() + if (not directory_path.is_absolute() or resolved_directory == download_root or + not resolved_directory.is_relative_to(download_root)): + raise ValueError('Source directory must be inside the download root') source = None - assert source_id - assert source_name - assert source_directory try: source = Source.objects.get(pk=source_id) except Source.DoesNotExist: @@ -1429,7 +1432,6 @@ def delete_all_media_for_source(source_id, source_name, source_directory): key = source.key + '/deleted', name = f'[Deleting] {source.name}', ) - directory_path = Path(source_directory) remove = ( (source and source.delete_removed_media) or (directory_path / '.to_be_removed').is_file() diff --git a/tubesync/sync/tests/test_index_coverage.py b/tubesync/sync/tests/test_index_coverage.py new file mode 100644 index 000000000..46a7c10a4 --- /dev/null +++ b/tubesync/sync/tests/test_index_coverage.py @@ -0,0 +1,29 @@ +"""The processing cap must not make upstream media look removed.""" +import ast +from collections import deque +from pathlib import Path +from types import SimpleNamespace +from unittest import TestCase + + +class IndexCoverageTests(TestCase): + def test_capped_queue_retains_complete_removal_keys(self): + # Exercise the method without importing installation settings or scheduling downloads. + source = Path(__file__).resolve().parents[1] / "models" / "source.py" + tree = ast.parse(source.read_text(encoding="utf-8")) + model = next(node for node in tree.body if isinstance(node, ast.ClassDef) and node.name == "Source") + method = next(node for node in model.body if isinstance(node, ast.FunctionDef) and node.name == "index_media") + namespace = {"queue": deque, "settings": SimpleNamespace(MAX_ENTRIES_PROCESSING=2)} + exec(compile(ast.Module(body=[method], type_ignores=[]), str(source), "exec"), namespace) + videos = [{"id": key} for key in ("video-a", "video-b", "video-c")] + streams = [{"id": key} for key in ("stream-a", "stream-b", "stream-c")] + for mixed in (False, True): + with self.subTest(mixed=mixed): + model = SimpleNamespace(index_videos=True, index_streams=mixed, + is_playlist=False, key_field="id", + get_index=lambda kind: videos if kind == "videos" else streams) + processing, upstream = namespace["index_media"](model) + self.assertEqual(len(processing), 2) + expected = {row["id"] for row in videos + (streams if mixed else [])} + self.assertEqual(upstream, expected) + self.assertEqual((expected | {"genuinely-removed"}) - upstream, {"genuinely-removed"}) diff --git a/tubesync/sync/tests/test_source_deletion_boundary.py b/tubesync/sync/tests/test_source_deletion_boundary.py new file mode 100644 index 000000000..f10ea98ab --- /dev/null +++ b/tubesync/sync/tests/test_source_deletion_boundary.py @@ -0,0 +1,113 @@ +"""Exercise deletion admission with fabricated paths and mocked database/queue writes.""" +import ast +import contextlib +import tempfile +import unittest +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import Mock + + +class SourceDeletionBoundaryTests(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory() + self.addCleanup(self.directory.cleanup) + self.base = Path(self.directory.name) + self.root = self.base / "downloads" + self.root.mkdir() + source = Path(__file__).resolve().parents[1] / "tasks.py" + tree = ast.parse(source.read_text(encoding="utf-8")) + self.function = next(node for node in tree.body if isinstance(node, ast.FunctionDef) + and node.name == "delete_all_media_for_source") + self.function.decorator_list = [] + + def task(self, present=True, remove=False): + class MissingSource(Exception): + pass + source = SimpleNamespace(delete_removed_media=remove, delete=Mock(), + uuid="synthetic-source", key="synthetic", name="Synthetic") + objects = Mock() + if present: + objects.get.return_value = source + else: + objects.get.side_effect = MissingSource + queryset = Mock() + queryset.defer.return_value = queryset + queryset.filter.return_value = queryset + namespace = {"Source": SimpleNamespace(objects=objects, DoesNotExist=MissingSource), + "Media": SimpleNamespace(objects=SimpleNamespace(all=Mock(return_value=queryset))), + "settings": SimpleNamespace(DOWNLOAD_ROOT=self.root), "Path": Path, + "delete_media": Mock(), "qs_gen": lambda query: [], + "huey_lock_task": Mock(return_value=SimpleNamespace(acquired=False)), + "TaskQueue": SimpleNamespace(FS="synthetic-fs"), "Val": lambda value: value, + "update_model": Mock(), + "atomic": lambda **kwargs: contextlib.nullcontext(), "log": Mock(), "rmtree": Mock()} + exec(compile(ast.Module(body=[self.function], type_ignores=[]), "tasks.py", "exec"), namespace) + return namespace, source, queryset + + def test_invalid_inputs_are_refused_before_queue_or_database_access(self): + outside = self.base / "outside" + outside.mkdir() + (outside / ".to_be_removed").touch() + (self.root / ".to_be_removed").touch() + valid = self.root / "valid" + cases = [("", "Synthetic", str(valid)), ("id", "", str(valid)), + ("id", "Synthetic", ""), ("id", "Synthetic", None), + ("id", "Synthetic", str(self.root)), ("id", "Synthetic", str(outside)), + ("id", "Synthetic", str(self.base / "downloads-other")), + ("id", "Synthetic", str(self.root / ".." / "outside")), + ("id", "Synthetic", "relative-path")] + for args in cases: + with self.subTest(args=args): + namespace, _, query = self.task(remove=True) + with self.assertRaises(ValueError): + namespace["delete_all_media_for_source"](*args) + namespace["Source"].objects.get.assert_not_called() + namespace["Media"].objects.all.assert_not_called() + namespace["delete_media"].map.assert_not_called() + query.update.assert_not_called() + query.delete.assert_not_called() + namespace["rmtree"].assert_not_called() + + def test_valid_captured_directory_preserves_existing_and_missing_source_workflows(self): + for present, remove, marker in [(True, False, False), (True, True, False), + (False, False, True), (False, False, False)]: + with self.subTest(present=present, remove=remove, marker=marker): + path = self.root / f"source-{present}-{remove}-{marker}" + path.mkdir() + if marker: + (path / ".to_be_removed").touch() + namespace, source, query = self.task(present, remove) + namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(path)) + query.filter.assert_called_once_with(source=source if present else "synthetic-id") + query.update.assert_called_once_with(manual_skip=True, skip=True) + query.delete.assert_called_once() + self.assertEqual(source.delete.call_count, int(present)) + if remove or marker: + namespace["rmtree"].assert_called_once_with(path, True) + else: + namespace["rmtree"].assert_not_called() + + def test_already_missing_directory_still_allows_database_cleanup(self): + namespace, _, query = self.task(present=False) + namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(self.root / "missing")) + query.delete.assert_called_once() + namespace["rmtree"].assert_not_called() + + def test_symlink_escape_is_refused_before_side_effects(self): + outside = self.base / "outside" + outside.mkdir() + link = self.root / "redirected" + try: + link.symlink_to(outside, target_is_directory=True) + except OSError as error: + self.skipTest(f"directory symlinks unavailable: {error}") + namespace, _, _ = self.task(remove=True) + with self.assertRaises(ValueError): + namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(link)) + namespace["Media"].objects.all.assert_not_called() + namespace["rmtree"].assert_not_called() + + +if __name__ == "__main__": + unittest.main() From cb867a4438f63acbe916d7bf1ca45bc70a711cdd Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sat, 17 Oct 2026 21:23:54 +1100 Subject: [PATCH 02/11] test: document isolated source execution in boundary regressions --- tubesync/sync/tests/test_index_coverage.py | 2 +- tubesync/sync/tests/test_source_deletion_boundary.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tubesync/sync/tests/test_index_coverage.py b/tubesync/sync/tests/test_index_coverage.py index 46a7c10a4..055168584 100644 --- a/tubesync/sync/tests/test_index_coverage.py +++ b/tubesync/sync/tests/test_index_coverage.py @@ -14,7 +14,7 @@ def test_capped_queue_retains_complete_removal_keys(self): model = next(node for node in tree.body if isinstance(node, ast.ClassDef) and node.name == "Source") method = next(node for node in model.body if isinstance(node, ast.FunctionDef) and node.name == "index_media") namespace = {"queue": deque, "settings": SimpleNamespace(MAX_ENTRIES_PROCESSING=2)} - exec(compile(ast.Module(body=[method], type_ignores=[]), str(source), "exec"), namespace) + exec(compile(ast.Module(body=[method], type_ignores=[]), str(source), "exec"), namespace) # noqa: S102 - selected local method, synthetic globals only videos = [{"id": key} for key in ("video-a", "video-b", "video-c")] streams = [{"id": key} for key in ("stream-a", "stream-b", "stream-c")] for mixed in (False, True): diff --git a/tubesync/sync/tests/test_source_deletion_boundary.py b/tubesync/sync/tests/test_source_deletion_boundary.py index f10ea98ab..af3e45eb2 100644 --- a/tubesync/sync/tests/test_source_deletion_boundary.py +++ b/tubesync/sync/tests/test_source_deletion_boundary.py @@ -42,7 +42,7 @@ class MissingSource(Exception): "TaskQueue": SimpleNamespace(FS="synthetic-fs"), "Val": lambda value: value, "update_model": Mock(), "atomic": lambda **kwargs: contextlib.nullcontext(), "log": Mock(), "rmtree": Mock()} - exec(compile(ast.Module(body=[self.function], type_ignores=[]), "tasks.py", "exec"), namespace) + exec(compile(ast.Module(body=[self.function], type_ignores=[]), "tasks.py", "exec"), namespace) # noqa: S102 - selected local method, synthetic globals only return namespace, source, queryset def test_invalid_inputs_are_refused_before_queue_or_database_access(self): From b15af1ec3cd1c8303fceb5d8cbdbcf22b5f3c80f Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sat, 17 Oct 2026 21:30:11 +1100 Subject: [PATCH 03/11] test: exercise source indexing through its public method --- tubesync/sync/tests/test_index_coverage.py | 27 ++++++++-------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/tubesync/sync/tests/test_index_coverage.py b/tubesync/sync/tests/test_index_coverage.py index 055168584..b95818e8f 100644 --- a/tubesync/sync/tests/test_index_coverage.py +++ b/tubesync/sync/tests/test_index_coverage.py @@ -1,28 +1,21 @@ """The processing cap must not make upstream media look removed.""" -import ast -from collections import deque -from pathlib import Path -from types import SimpleNamespace -from unittest import TestCase +from unittest.mock import patch +from django.test import SimpleTestCase, override_settings -class IndexCoverageTests(TestCase): +from sync.models import Source + + +@override_settings(MAX_ENTRIES_PROCESSING=2) +class IndexCoverageTests(SimpleTestCase): def test_capped_queue_retains_complete_removal_keys(self): - # Exercise the method without importing installation settings or scheduling downloads. - source = Path(__file__).resolve().parents[1] / "models" / "source.py" - tree = ast.parse(source.read_text(encoding="utf-8")) - model = next(node for node in tree.body if isinstance(node, ast.ClassDef) and node.name == "Source") - method = next(node for node in model.body if isinstance(node, ast.FunctionDef) and node.name == "index_media") - namespace = {"queue": deque, "settings": SimpleNamespace(MAX_ENTRIES_PROCESSING=2)} - exec(compile(ast.Module(body=[method], type_ignores=[]), str(source), "exec"), namespace) # noqa: S102 - selected local method, synthetic globals only videos = [{"id": key} for key in ("video-a", "video-b", "video-c")] streams = [{"id": key} for key in ("stream-a", "stream-b", "stream-c")] for mixed in (False, True): with self.subTest(mixed=mixed): - model = SimpleNamespace(index_videos=True, index_streams=mixed, - is_playlist=False, key_field="id", - get_index=lambda kind: videos if kind == "videos" else streams) - processing, upstream = namespace["index_media"](model) + source = Source(index_videos=True, index_streams=mixed) + with patch.object(source, "get_index", side_effect=lambda kind: videos if kind == "videos" else streams): + processing, upstream = source.index_media() self.assertEqual(len(processing), 2) expected = {row["id"] for row in videos + (streams if mixed else [])} self.assertEqual(upstream, expected) From 8928a9657ebd3ed8bb1108f3f979435bd54beee9 Mon Sep 17 00:00:00 2001 From: tcely Date: Sat, 17 Oct 2026 22:46:28 -0400 Subject: [PATCH 04/11] feat: track published and processed media keys * feat: track published and processed media keys * chore: modify indexing and cleanup to include published keys --- tubesync/sync/models/source.py | 70 +++++++++++++++++++++------------- tubesync/sync/tasks.py | 11 ++++-- 2 files changed, 51 insertions(+), 30 deletions(-) diff --git a/tubesync/sync/models/source.py b/tubesync/sync/models/source.py index 3dd7f1bbb..8e945a3fe 100644 --- a/tubesync/sync/models/source.py +++ b/tubesync/sync/models/source.py @@ -1,16 +1,21 @@ import os import re import uuid + from collections import deque as queue +from itertools import islice from pathlib import Path from typing import ClassVar + from django import db from django.conf import settings from django.core.validators import RegexValidator from django.utils import timezone from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ + from yt_dlp import DownloadError + from ..choices import (Val, AudioTrack, SponsorBlock_Category, YouTube_SourceType, IndexSchedule, CapChoices, Fallback, FileExtension, FilterSeconds, @@ -24,14 +29,11 @@ get_image_urls as get_youtube_image_urls, merge_image_thumbnails, ) + from ._migrations import media_file_storage from ._private import _srctype_dict -def _has_index_metadata_value(value): - return bool(value) or value in (0, False,) - - class Source(db.models.Model): ''' A Source is a source of media. Currently, this is either a YouTube channel @@ -619,6 +621,10 @@ def get_index(self, url_type, /): if not isinstance(response, dict): return entries entries = response.pop('entries', list()) + + def _has_index_metadata_value(value): + return bool(value) or value in (0, False,) + default_site = self.videos.model._meta.get_field('site').get_default() site = response.get('extractor_key') or default_site metadata, _ = self.videos.filter( @@ -633,13 +639,15 @@ def get_index(self, url_type, /): previous.get('thumbnails'), response.get('thumbnails'), ) + for key, value in previous.items(): if 'thumbnails' == key: continue - if ( + write_to_key = ( _has_index_metadata_value(value) and not _has_index_metadata_value(response.get(key)) - ): + ) + if write_to_key: response[key] = value update_fields = {'retrieved', 'value'} if site and site not in (metadata.site, default_site): @@ -651,27 +659,37 @@ def get_index(self, url_type, /): return entries def index_media(self): - ''' - Index the media source returning a queue of media metadata as dicts. - ''' - entries = queue(list(), getattr(settings, 'MAX_ENTRIES_PROCESSING', 0) or None) + """ + Index the media source returning a queue of media metadata as dicts and a set of media keys. + """ + entries = queue(maxlen=getattr(settings, 'MAX_ENTRIES_PROCESSING', 0) or None) + seen_keys = set() + def register_key(item): + key = item.get(self.key_field, None) + if key is not None: + seen_keys.add(key) + return item + if self.index_videos: videos = self.get_index('videos') - entries.extend(reversed(videos)) + entries.extend(map(register_key, reversed(videos))) # Playlists do something different that I have yet to figure out - # ruff: ignore[SIM102] - if not self.is_playlist: - if self.index_streams: - streams = self.get_index('streams') - if entries.maxlen is None or 0 == len(entries): - entries.extend(reversed(streams)) - else: - # share the queue between streams and videos - allowed_streams = max( - entries.maxlen // 2, - entries.maxlen - len(entries), - ) - entries.extend(reversed(streams[: allowed_streams])) - - return entries + if not self.is_playlist and self.index_streams: + streams = self.get_index('streams') + if entries.maxlen is None or 0 == len(entries): + entries.extend(map(register_key, reversed(streams))) + else: + # share the queue between streams and videos + allowed_streams = max( + entries.maxlen // 2, + entries.maxlen - len(entries), + ) + entries.extend(reversed(tuple( + islice( + map(register_key, streams), + allowed_streams, + ), + ))) + + return entries, seen_keys diff --git a/tubesync/sync/tasks.py b/tubesync/sync/tasks.py index 6f171d203..bf1b159ca 100644 --- a/tubesync/sync/tasks.py +++ b/tubesync/sync/tasks.py @@ -553,7 +553,7 @@ def index_source(source_id): # Reset any errors source.has_failed = False # Index the source - videos = source.index_media() + videos, published_keys = source.index_media() if not videos: source.has_failed = True update_model(source, has_failed=source.has_failed) @@ -593,7 +593,7 @@ def index_source(source_id): end=task.verbose_name.find('Index'), ) vn = 0 - video_keys = set() + processed_keys = set() while len(videos) > 0: vn += 1 video = videos.popleft() @@ -602,7 +602,7 @@ def index_source(source_id): if not key: # Video has no unique key (ID), it can't be indexed continue - video_keys.add(key) + processed_keys.add(key) if len(db_batch_data) == db_batch_data.maxlen: save_db_batch(Metadata.objects, db_batch_data, db_fields_data) if len(db_batch_media) == db_batch_media.maxlen: @@ -685,7 +685,10 @@ def index_source(source_id): save_db_batch(Metadata.objects, db_batch_data, db_fields_data) save_db_batch(Media.objects, db_batch_media, db_fields_media) # Cleanup of media no longer available from the source - cleanup_removed_media(str(source.pk), video_keys) + use_published = 'published' == str( + getattr(settings, 'CLEANUP_WHICH', 'processed') + ).strip().lower() + cleanup_removed_media(str(source.pk), published_keys if use_published else processed_keys) # Clear references to indexed data videos = video = None db_batch_data.clear() From 1666af9909b0103542749c0464f5240a00b22779 Mon Sep 17 00:00:00 2001 From: tcely Date: Sat, 17 Oct 2026 22:57:35 -0400 Subject: [PATCH 05/11] chore(tests): add CLEANUP_WHICH setting --- tubesync/sync/tests/test_index_coverage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tubesync/sync/tests/test_index_coverage.py b/tubesync/sync/tests/test_index_coverage.py index b95818e8f..c5a63adba 100644 --- a/tubesync/sync/tests/test_index_coverage.py +++ b/tubesync/sync/tests/test_index_coverage.py @@ -6,7 +6,7 @@ from sync.models import Source -@override_settings(MAX_ENTRIES_PROCESSING=2) +@override_settings(MAX_ENTRIES_PROCESSING=2, CLEANUP_WHICH='Published ') class IndexCoverageTests(SimpleTestCase): def test_capped_queue_retains_complete_removal_keys(self): videos = [{"id": key} for key in ("video-a", "video-b", "video-c")] From c5ab92286e2d9f15d9ebfc0585551d566bcfab5c Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sun, 18 Oct 2026 14:39:05 +1100 Subject: [PATCH 06/11] fix: register every stream key before capping the processing queue islice() stopped register_key() at the stream allowance, so streams beyond the cap were missing from the published keys. --- tubesync/sync/models/source.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tubesync/sync/models/source.py b/tubesync/sync/models/source.py index 8e945a3fe..a69c9af69 100644 --- a/tubesync/sync/models/source.py +++ b/tubesync/sync/models/source.py @@ -3,7 +3,6 @@ import uuid from collections import deque as queue -from itertools import islice from pathlib import Path from typing import ClassVar @@ -685,11 +684,6 @@ def register_key(item): entries.maxlen // 2, entries.maxlen - len(entries), ) - entries.extend(reversed(tuple( - islice( - map(register_key, streams), - allowed_streams, - ), - ))) + entries.extend(reversed(tuple(map(register_key, streams))[: allowed_streams])) return entries, seen_keys From 6b1f0687ab9613ecfe0cfab1ed83fad84ae9a70f Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sun, 18 Oct 2026 14:39:06 +1100 Subject: [PATCH 07/11] Only guard the source directory removal, using the form's lexical rule Restore the original argument checks and leave database cleanup unchanged. Before rmtree(), require the directory to be below DOWNLOAD_ROOT without resolving symbolic links, matching safe_join() in the source form. Replace the AST-based test with one that calls the task's func directly. --- tubesync/sync/tasks.py | 18 ++- .../tests/test_source_deletion_boundary.py | 144 +++++------------- 2 files changed, 49 insertions(+), 113 deletions(-) diff --git a/tubesync/sync/tasks.py b/tubesync/sync/tasks.py index e067ef53a..43f187f9e 100644 --- a/tubesync/sync/tasks.py +++ b/tubesync/sync/tasks.py @@ -1408,15 +1408,10 @@ def save_all_media_for_source(source_id): @db_task(delay=90, priority=99, queue=Val(TaskQueue.FS)) def delete_all_media_for_source(source_id, source_name, source_directory): - if not source_id or not source_name or not source_directory: - raise ValueError('Source ID, name and directory are required') - directory_path = Path(source_directory) - download_root = Path(settings.DOWNLOAD_ROOT).resolve() - resolved_directory = directory_path.resolve() - if (not directory_path.is_absolute() or resolved_directory == download_root or - not resolved_directory.is_relative_to(download_root)): - raise ValueError('Source directory must be inside the download root') source = None + assert source_id + assert source_name + assert source_directory try: source = Source.objects.get(pk=source_id) except Source.DoesNotExist: @@ -1437,6 +1432,7 @@ def delete_all_media_for_source(source_id, source_name, source_directory): key = source.key + '/deleted', name = f'[Deleting] {source.name}', ) + directory_path = Path(source_directory) remove = ( (source and source.delete_removed_media) or (directory_path / '.to_be_removed').is_file() @@ -1479,5 +1475,11 @@ def delete_all_media_for_source(source_id, source_name, source_directory): source_lock.acquired = False # Remove the directory, if the user requested that if remove: + # The same lexical check as safe_join() in the source form, + # so symbolic links below DOWNLOAD_ROOT are not followed. + download_root = Path(os.path.abspath(settings.DOWNLOAD_ROOT)) + if download_root not in Path(os.path.abspath(directory_path)).parents: + log.error(f'Not deleting directory outside {download_root} for: {source_name}: {directory_path}') + return log.info(f'Deleting directory for: {source_name}: {directory_path}') rmtree(directory_path, True) diff --git a/tubesync/sync/tests/test_source_deletion_boundary.py b/tubesync/sync/tests/test_source_deletion_boundary.py index af3e45eb2..82ad503a4 100644 --- a/tubesync/sync/tests/test_source_deletion_boundary.py +++ b/tubesync/sync/tests/test_source_deletion_boundary.py @@ -1,113 +1,47 @@ -"""Exercise deletion admission with fabricated paths and mocked database/queue writes.""" -import ast -import contextlib +import logging import tempfile -import unittest +import uuid from pathlib import Path -from types import SimpleNamespace -from unittest.mock import Mock +from unittest.mock import patch +from django.test import TransactionTestCase, override_settings -class SourceDeletionBoundaryTests(unittest.TestCase): - def setUp(self): - self.directory = tempfile.TemporaryDirectory() - self.addCleanup(self.directory.cleanup) - self.base = Path(self.directory.name) - self.root = self.base / "downloads" - self.root.mkdir() - source = Path(__file__).resolve().parents[1] / "tasks.py" - tree = ast.parse(source.read_text(encoding="utf-8")) - self.function = next(node for node in tree.body if isinstance(node, ast.FunctionDef) - and node.name == "delete_all_media_for_source") - self.function.decorator_list = [] - - def task(self, present=True, remove=False): - class MissingSource(Exception): - pass - source = SimpleNamespace(delete_removed_media=remove, delete=Mock(), - uuid="synthetic-source", key="synthetic", name="Synthetic") - objects = Mock() - if present: - objects.get.return_value = source - else: - objects.get.side_effect = MissingSource - queryset = Mock() - queryset.defer.return_value = queryset - queryset.filter.return_value = queryset - namespace = {"Source": SimpleNamespace(objects=objects, DoesNotExist=MissingSource), - "Media": SimpleNamespace(objects=SimpleNamespace(all=Mock(return_value=queryset))), - "settings": SimpleNamespace(DOWNLOAD_ROOT=self.root), "Path": Path, - "delete_media": Mock(), "qs_gen": lambda query: [], - "huey_lock_task": Mock(return_value=SimpleNamespace(acquired=False)), - "TaskQueue": SimpleNamespace(FS="synthetic-fs"), "Val": lambda value: value, - "update_model": Mock(), - "atomic": lambda **kwargs: contextlib.nullcontext(), "log": Mock(), "rmtree": Mock()} - exec(compile(ast.Module(body=[self.function], type_ignores=[]), "tasks.py", "exec"), namespace) # noqa: S102 - selected local method, synthetic globals only - return namespace, source, queryset - - def test_invalid_inputs_are_refused_before_queue_or_database_access(self): - outside = self.base / "outside" - outside.mkdir() - (outside / ".to_be_removed").touch() - (self.root / ".to_be_removed").touch() - valid = self.root / "valid" - cases = [("", "Synthetic", str(valid)), ("id", "", str(valid)), - ("id", "Synthetic", ""), ("id", "Synthetic", None), - ("id", "Synthetic", str(self.root)), ("id", "Synthetic", str(outside)), - ("id", "Synthetic", str(self.base / "downloads-other")), - ("id", "Synthetic", str(self.root / ".." / "outside")), - ("id", "Synthetic", "relative-path")] - for args in cases: - with self.subTest(args=args): - namespace, _, query = self.task(remove=True) - with self.assertRaises(ValueError): - namespace["delete_all_media_for_source"](*args) - namespace["Source"].objects.get.assert_not_called() - namespace["Media"].objects.all.assert_not_called() - namespace["delete_media"].map.assert_not_called() - query.update.assert_not_called() - query.delete.assert_not_called() - namespace["rmtree"].assert_not_called() +from sync import tasks - def test_valid_captured_directory_preserves_existing_and_missing_source_workflows(self): - for present, remove, marker in [(True, False, False), (True, True, False), - (False, False, True), (False, False, False)]: - with self.subTest(present=present, remove=remove, marker=marker): - path = self.root / f"source-{present}-{remove}-{marker}" - path.mkdir() - if marker: - (path / ".to_be_removed").touch() - namespace, source, query = self.task(present, remove) - namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(path)) - query.filter.assert_called_once_with(source=source if present else "synthetic-id") - query.update.assert_called_once_with(manual_skip=True, skip=True) - query.delete.assert_called_once() - self.assertEqual(source.delete.call_count, int(present)) - if remove or marker: - namespace["rmtree"].assert_called_once_with(path, True) - else: - namespace["rmtree"].assert_not_called() - def test_already_missing_directory_still_allows_database_cleanup(self): - namespace, _, query = self.task(present=False) - namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(self.root / "missing")) - query.delete.assert_called_once() - namespace["rmtree"].assert_not_called() +class SourceDirectoryDeletionTestCase(TransactionTestCase): - def test_symlink_escape_is_refused_before_side_effects(self): - outside = self.base / "outside" - outside.mkdir() - link = self.root / "redirected" + def setUp(self): + logging.disable(logging.CRITICAL) + self.addCleanup(logging.disable, logging.NOTSET) + temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(temp_dir.cleanup) + self.base = Path(temp_dir.name) + self.root = self.base / 'downloads' + + def delete(self, directory): + directory.mkdir(parents=True, exist_ok=True) + (directory / '.to_be_removed').touch() + with override_settings(DOWNLOAD_ROOT=self.root), patch.object(tasks, 'rmtree') as rmtree: + tasks.delete_all_media_for_source.func(str(uuid.uuid4()), 'Removed', str(directory)) + return rmtree + + def test_directory_inside_download_root_is_removed(self): + directory = self.root / 'video' / 'removed' + self.delete(directory).assert_called_once_with(directory, True) + + def test_symbolic_link_inside_download_root_is_not_followed(self): + target = self.base / 'other-disk' / 'removed' + target.mkdir(parents=True) + link = self.root / 'removed' + self.root.mkdir() try: - link.symlink_to(outside, target_is_directory=True) - except OSError as error: - self.skipTest(f"directory symlinks unavailable: {error}") - namespace, _, _ = self.task(remove=True) - with self.assertRaises(ValueError): - namespace["delete_all_media_for_source"]("synthetic-id", "Synthetic", str(link)) - namespace["Media"].objects.all.assert_not_called() - namespace["rmtree"].assert_not_called() - - -if __name__ == "__main__": - unittest.main() + link.symlink_to(target, target_is_directory=True) + except OSError as e: + self.skipTest(f'symbolic links unavailable: {e}') + self.delete(link).assert_called_once_with(link, True) + + def test_directory_outside_download_root_is_kept(self): + for directory in (self.root, self.base / 'elsewhere', self.root / '..' / 'elsewhere'): + with self.subTest(directory=directory): + self.delete(directory).assert_not_called() From 285f85ed89f1287302cfeeb049f27a6f5f01439e Mon Sep 17 00:00:00 2001 From: tcely Date: Sat, 17 Oct 2026 23:44:14 -0400 Subject: [PATCH 08/11] fix: register the key for all streams --- tubesync/sync/models/source.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tubesync/sync/models/source.py b/tubesync/sync/models/source.py index 8e945a3fe..6301377e0 100644 --- a/tubesync/sync/models/source.py +++ b/tubesync/sync/models/source.py @@ -3,7 +3,6 @@ import uuid from collections import deque as queue -from itertools import islice from pathlib import Path from typing import ClassVar @@ -673,6 +672,7 @@ def register_key(item): if self.index_videos: videos = self.get_index('videos') entries.extend(map(register_key, reversed(videos))) + del videos # Playlists do something different that I have yet to figure out if not self.is_playlist and self.index_streams: @@ -685,11 +685,8 @@ def register_key(item): entries.maxlen // 2, entries.maxlen - len(entries), ) - entries.extend(reversed(tuple( - islice( - map(register_key, streams), - allowed_streams, - ), - ))) + streams = tuple(map(register_key, streams))[: allowed_streams] + entries.extend(reversed(streams)) + del streams return entries, seen_keys From efa0d004a8db8efc4a36fe85ed85c97e95fc52f5 Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sun, 18 Oct 2026 19:20:15 +1100 Subject: [PATCH 09/11] Move the deletion guard out of this PR and test the processing order tcely asked for the source-directory removal guard to be split into its own pull request. The index test now checks the exact processing order for uncapped, half-full and full queues, matching index_media() before it also returned the published keys, and that the published keys cover media beyond the cap. --- tubesync/sync/tasks.py | 6 --- tubesync/sync/tests/test_index_coverage.py | 45 +++++++++++++----- .../tests/test_source_deletion_boundary.py | 47 ------------------- 3 files changed, 32 insertions(+), 66 deletions(-) delete mode 100644 tubesync/sync/tests/test_source_deletion_boundary.py diff --git a/tubesync/sync/tasks.py b/tubesync/sync/tasks.py index 43f187f9e..bf1b159ca 100644 --- a/tubesync/sync/tasks.py +++ b/tubesync/sync/tasks.py @@ -1475,11 +1475,5 @@ def delete_all_media_for_source(source_id, source_name, source_directory): source_lock.acquired = False # Remove the directory, if the user requested that if remove: - # The same lexical check as safe_join() in the source form, - # so symbolic links below DOWNLOAD_ROOT are not followed. - download_root = Path(os.path.abspath(settings.DOWNLOAD_ROOT)) - if download_root not in Path(os.path.abspath(directory_path)).parents: - log.error(f'Not deleting directory outside {download_root} for: {source_name}: {directory_path}') - return log.info(f'Deleting directory for: {source_name}: {directory_path}') rmtree(directory_path, True) diff --git a/tubesync/sync/tests/test_index_coverage.py b/tubesync/sync/tests/test_index_coverage.py index c5a63adba..900963cb3 100644 --- a/tubesync/sync/tests/test_index_coverage.py +++ b/tubesync/sync/tests/test_index_coverage.py @@ -6,17 +6,36 @@ from sync.models import Source -@override_settings(MAX_ENTRIES_PROCESSING=2, CLEANUP_WHICH='Published ') +VIDEOS = [{"id": key} for key in ("video-a", "video-b", "video-c")] +STREAMS = [{"id": key} for key in ("stream-a", "stream-b", "stream-c")] + + class IndexCoverageTests(SimpleTestCase): - def test_capped_queue_retains_complete_removal_keys(self): - videos = [{"id": key} for key in ("video-a", "video-b", "video-c")] - streams = [{"id": key} for key in ("stream-a", "stream-b", "stream-c")] - for mixed in (False, True): - with self.subTest(mixed=mixed): - source = Source(index_videos=True, index_streams=mixed) - with patch.object(source, "get_index", side_effect=lambda kind: videos if kind == "videos" else streams): - processing, upstream = source.index_media() - self.assertEqual(len(processing), 2) - expected = {row["id"] for row in videos + (streams if mixed else [])} - self.assertEqual(upstream, expected) - self.assertEqual((expected | {"genuinely-removed"}) - upstream, {"genuinely-removed"}) + def index(self, cap, mixed): + source = Source(index_videos=True, index_streams=mixed) + with override_settings(MAX_ENTRIES_PROCESSING=cap), patch.object( + source, "get_index", side_effect=lambda kind: VIDEOS if kind == "videos" else STREAMS, + ): + processing, published = source.index_media() + return [row["id"] for row in processing], published + + def test_processing_order_is_unchanged(self): + # The order index_media() produced before it also returned the published keys. + expected = { + (0, False): ["video-c", "video-b", "video-a"], + (0, True): ["video-c", "video-b", "video-a", "stream-c", "stream-b", "stream-a"], + (2, False): ["video-b", "video-a"], + (2, True): ["video-a", "stream-a"], + (4, False): ["video-c", "video-b", "video-a"], + (4, True): ["video-b", "video-a", "stream-b", "stream-a"], + } + for (cap, mixed), order in expected.items(): + with self.subTest(cap=cap, mixed=mixed): + self.assertEqual(self.index(cap, mixed)[0], order) + + def test_published_keys_include_media_beyond_the_cap(self): + for cap in (0, 2, 4): + for mixed in (False, True): + with self.subTest(cap=cap, mixed=mixed): + expected = {row["id"] for row in VIDEOS + (STREAMS if mixed else [])} + self.assertEqual(self.index(cap, mixed)[1], expected) diff --git a/tubesync/sync/tests/test_source_deletion_boundary.py b/tubesync/sync/tests/test_source_deletion_boundary.py deleted file mode 100644 index 82ad503a4..000000000 --- a/tubesync/sync/tests/test_source_deletion_boundary.py +++ /dev/null @@ -1,47 +0,0 @@ -import logging -import tempfile -import uuid -from pathlib import Path -from unittest.mock import patch - -from django.test import TransactionTestCase, override_settings - -from sync import tasks - - -class SourceDirectoryDeletionTestCase(TransactionTestCase): - - def setUp(self): - logging.disable(logging.CRITICAL) - self.addCleanup(logging.disable, logging.NOTSET) - temp_dir = tempfile.TemporaryDirectory() - self.addCleanup(temp_dir.cleanup) - self.base = Path(temp_dir.name) - self.root = self.base / 'downloads' - - def delete(self, directory): - directory.mkdir(parents=True, exist_ok=True) - (directory / '.to_be_removed').touch() - with override_settings(DOWNLOAD_ROOT=self.root), patch.object(tasks, 'rmtree') as rmtree: - tasks.delete_all_media_for_source.func(str(uuid.uuid4()), 'Removed', str(directory)) - return rmtree - - def test_directory_inside_download_root_is_removed(self): - directory = self.root / 'video' / 'removed' - self.delete(directory).assert_called_once_with(directory, True) - - def test_symbolic_link_inside_download_root_is_not_followed(self): - target = self.base / 'other-disk' / 'removed' - target.mkdir(parents=True) - link = self.root / 'removed' - self.root.mkdir() - try: - link.symlink_to(target, target_is_directory=True) - except OSError as e: - self.skipTest(f'symbolic links unavailable: {e}') - self.delete(link).assert_called_once_with(link, True) - - def test_directory_outside_download_root_is_kept(self): - for directory in (self.root, self.base / 'elsewhere', self.root / '..' / 'elsewhere'): - with self.subTest(directory=directory): - self.delete(directory).assert_not_called() From 50502e1afa85e2d52156e8ba8dd0e6fa48cca4b5 Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sun, 18 Oct 2026 22:20:50 +1100 Subject: [PATCH 10/11] docs: describe INDEXING_CLEANUP_UNAVAILABLE_POLICY and its interaction with MAX_ENTRIES_PROCESSING An updated copy of the wiki's Settings page for tcely to commit to the wiki. --- docs/Settings.md | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 docs/Settings.md diff --git a/docs/Settings.md b/docs/Settings.md new file mode 100644 index 000000000..fd43f8984 --- /dev/null +++ b/docs/Settings.md @@ -0,0 +1,71 @@ +Table of contents: + +- [`INDEXING_CLEANUP_UNAVAILABLE_POLICY`](#indexing_cleanup_unavailable_policy) +- [`MAX_ENTRIES_PROCESSING`](#max_entries_processing) +- [`MEDIA_FORMATSTR`](#media_formatstr) +- [`SHRINK_NEW_MEDIA_METADATA`](#shrink_new_media_metadata) +- [`SHRINK_OLD_MEDIA_METADATA`](#shrink_old_media_metadata) + +-------- + +### `INDEXING_CLEANUP_UNAVAILABLE_POLICY` + +* _Character_: `'processed'` - or `'published'`. Case and surrounding spaces are ignored. +* Decides which media counts as still available when indexing finishes. It only matters for a [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33) with _delete removed media_ enabled, because only then is media that is no longer available deleted. + * `'processed'`: only the media processed during this indexing counts. With [`MAX_ENTRIES_PROCESSING`](#max_entries_processing) set, media beyond the limit is treated as removed, even though it is still published. This is the earlier behaviour. + * `'published'`: every item the source's index returns counts, including those beyond the limit. Only media that is no longer published is deleted. +* With no processing limit, both values give the same result. + +|Added in | +|---------| +| [#1618](../pull/1618) | + +-------- + +### `MAX_ENTRIES_PROCESSING` + +* _Integer_: `0` - (zero) Disables the limit entirely. Higher numbers set the limit. +* The maximum number of videos to process while indexing a [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33). +* The limit keeps the newest videos. When a source also indexes streams, they share it: streams get half the limit or whatever the videos left unused, whichever is more. +* To stop the limit from deleting media that is still published from a source with _delete removed media_ enabled, set [`INDEXING_CLEANUP_UNAVAILABLE_POLICY`](#indexing_cleanup_unavailable_policy) to `'published'`. + +|Added in | Fixed in | +|---------|----------| +| [#305](../pull/305) | [#1044](../pull/1044) | + +-------- + +### `MEDIA_FORMATSTR` + +* _Character_: `'{title} - [{key}].{ext}'` - 1-200 quoted characters. +* A customizable default for the [`media_format`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L123-L128) field of new [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33) instances. + +|Added in | For issue | +|---------|-----------| +| [#1392](../pull/1392) | [#1391](../issues/1391) | + +-------- + +### `SHRINK_NEW_MEDIA_METADATA` + +* _Boolean_: `True` - will enable using the filter function. +* Filter out useless data included when metadata is retrieved. This is things such as expired URLs, etc. +* This setting affects newly indexed media only. + +|Added in | For issue | Env vars | +|---------|-----------|----------| +| [#612](../pull/612) | [#470](../issues/470) | [#622](../pull/622) | + +-------- + +### `SHRINK_OLD_MEDIA_METADATA` + +* _Boolean_: `True` - will enable using the filter function. +* Filter out useless data included when metadata is loaded. This is things such as expired URLs, etc. +* This setting affects metadata on every media instance, the next time it is loaded. + +|Added in | For issue | Env vars | +|---------|-----------|----------| +| [#612](../pull/612) | [#470](../issues/470) | [#622](../pull/622) | + +-------- From a294f738531a56ac1f735f59a5bf2c92277ccc3f Mon Sep 17 00:00:00 2001 From: tcely Date: Sun, 18 Oct 2026 07:59:45 -0400 Subject: [PATCH 11/11] Delete docs/Settings.md Copied to the wiki already. --- docs/Settings.md | 71 ------------------------------------------------ 1 file changed, 71 deletions(-) delete mode 100644 docs/Settings.md diff --git a/docs/Settings.md b/docs/Settings.md deleted file mode 100644 index fd43f8984..000000000 --- a/docs/Settings.md +++ /dev/null @@ -1,71 +0,0 @@ -Table of contents: - -- [`INDEXING_CLEANUP_UNAVAILABLE_POLICY`](#indexing_cleanup_unavailable_policy) -- [`MAX_ENTRIES_PROCESSING`](#max_entries_processing) -- [`MEDIA_FORMATSTR`](#media_formatstr) -- [`SHRINK_NEW_MEDIA_METADATA`](#shrink_new_media_metadata) -- [`SHRINK_OLD_MEDIA_METADATA`](#shrink_old_media_metadata) - --------- - -### `INDEXING_CLEANUP_UNAVAILABLE_POLICY` - -* _Character_: `'processed'` - or `'published'`. Case and surrounding spaces are ignored. -* Decides which media counts as still available when indexing finishes. It only matters for a [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33) with _delete removed media_ enabled, because only then is media that is no longer available deleted. - * `'processed'`: only the media processed during this indexing counts. With [`MAX_ENTRIES_PROCESSING`](#max_entries_processing) set, media beyond the limit is treated as removed, even though it is still published. This is the earlier behaviour. - * `'published'`: every item the source's index returns counts, including those beyond the limit. Only media that is no longer published is deleted. -* With no processing limit, both values give the same result. - -|Added in | -|---------| -| [#1618](../pull/1618) | - --------- - -### `MAX_ENTRIES_PROCESSING` - -* _Integer_: `0` - (zero) Disables the limit entirely. Higher numbers set the limit. -* The maximum number of videos to process while indexing a [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33). -* The limit keeps the newest videos. When a source also indexes streams, they share it: streams get half the limit or whatever the videos left unused, whichever is more. -* To stop the limit from deleting media that is still published from a source with _delete removed media_ enabled, set [`INDEXING_CLEANUP_UNAVAILABLE_POLICY`](#indexing_cleanup_unavailable_policy) to `'published'`. - -|Added in | Fixed in | -|---------|----------| -| [#305](../pull/305) | [#1044](../pull/1044) | - --------- - -### `MEDIA_FORMATSTR` - -* _Character_: `'{title} - [{key}].{ext}'` - 1-200 quoted characters. -* A customizable default for the [`media_format`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L123-L128) field of new [`Source`](https://github.com/meeb/tubesync/blob/680913211370154b5de2ce52a8e34f81640ad4d2/tubesync/sync/models/source.py#L28-L33) instances. - -|Added in | For issue | -|---------|-----------| -| [#1392](../pull/1392) | [#1391](../issues/1391) | - --------- - -### `SHRINK_NEW_MEDIA_METADATA` - -* _Boolean_: `True` - will enable using the filter function. -* Filter out useless data included when metadata is retrieved. This is things such as expired URLs, etc. -* This setting affects newly indexed media only. - -|Added in | For issue | Env vars | -|---------|-----------|----------| -| [#612](../pull/612) | [#470](../issues/470) | [#622](../pull/622) | - --------- - -### `SHRINK_OLD_MEDIA_METADATA` - -* _Boolean_: `True` - will enable using the filter function. -* Filter out useless data included when metadata is loaded. This is things such as expired URLs, etc. -* This setting affects metadata on every media instance, the next time it is loaded. - -|Added in | For issue | Env vars | -|---------|-----------|----------| -| [#612](../pull/612) | [#470](../issues/470) | [#622](../pull/622) | - ---------