From fc045cd74e1e89b114a1379c74eaa8aed9df7db7 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 08:37:47 -0700 Subject: [PATCH] =?UTF-8?q?youtube=20download:=20don't=20clobber=20target?= =?UTF-8?q?=5Fdir=20=E2=86=92=20fixes=20double-nested=20folders?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bug: process_youtube_download wrote the ORGANISED dir (channel/Season YYYY) back to the row's target_dir, but target_dir is supposed to be the youtube ROOT — and plan_destination re-derives channel/season UNDER it. so any re-processing re-nested: Channel/Season 2026/Channel/Season 2026/. that hit the 1-2 videos per channel that got interrupted mid-download and re-queued by the orphan reaper. fix: only record the organised filename for display; never write target_dir. the root stays put so re-runs are idempotent. regression test runs it twice + asserts no target_dir clobber + a stable (non-nested) dest dir. --- core/video/youtube_download.py | 8 ++++++-- tests/test_youtube_download.py | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/core/video/youtube_download.py b/core/video/youtube_download.py index 0a387c11..296a591a 100644 --- a/core/video/youtube_download.py +++ b/core/video/youtube_download.py @@ -162,9 +162,13 @@ def process_youtube_download( dest = plan_destination(dl, settings, container) stem, cont = _stem_and_container(dest, container) - # Record where it's going up front (Downloads page shows the organised name). + # Record the organised filename for the Downloads page. CRITICAL: do NOT write the + # organised DIR back to target_dir — target_dir is the youtube ROOT, and plan_destination + # re-derives the channel/season folders under it. Clobbering it would re-nest on any + # re-run (e.g. the orphan reaper re-queues an interrupted download) → + # Channel/Season/Channel/Season. Leave target_dir = root so re-processing is idempotent. update_row(dl.get("id"), status="downloading", progress=0, - target_dir=dest.get("dir"), filename=dest.get("filename")) + filename=dest.get("filename")) res = download(dl.get("media_id"), dest.get("dir"), stem, profile, cont, progress_hook=progress_hook, cookie_opts=cookie_opts) diff --git a/tests/test_youtube_download.py b/tests/test_youtube_download.py index f15e1b5a..0089e436 100644 --- a/tests/test_youtube_download.py +++ b/tests/test_youtube_download.py @@ -179,3 +179,26 @@ def test_process_passes_the_organised_dir_to_the_downloader(): assert seen["video_id"] == "vid1" assert seen["dest_dir"] == os.path.join("/yt", "Chan", "Season 2024") assert seen["stem"] == "Chan - 2024-03-15 - T" and seen["container"] == "mp4" + + +def test_process_never_clobbers_target_dir_so_reruns_dont_nest(): + """The row's target_dir is the youtube ROOT; plan_destination derives the channel/season + folders under it. The worker must NOT write the organised dir back to target_dir, or a + re-run (e.g. the orphan reaper re-queues an interrupted download) would organise AGAIN → + Channel/Season/Channel/Season. Re-processing the same row must be idempotent.""" + seen = [] + + def fake_download(video_id, dest_dir, stem, profile, container, **kw): + seen.append(dest_dir) + return {"ok": True, "dest_path": "/x"} + + calls, update_row, archive, clear = _recorder() + dl = _dl() # target_dir = "/yt" (the root) + for _ in range(2): # simulate the interrupted-then-requeued re-run + ytd.process_youtube_download(dl, profile=default_profile(), settings={}, + download=fake_download, update_row=update_row, + archive=archive, clear_wishlist=clear, now=lambda: "t") + # no update_row call writes target_dir (that's what caused the nesting) + assert all("target_dir" not in kw for _, kw in calls["rows"]) + # both runs target the SAME organised dir — not a doubly-nested one + assert seen[0] == seen[1] == os.path.join("/yt", "Chan", "Season 2024")