From 62f8fb746b0cd9515f5761cc9b4c308b80f2ea35 Mon Sep 17 00:00:00 2001 From: vrubelroman Date: Wed, 16 Sep 2026 20:14:43 +0000 Subject: [PATCH] Fix delete not finding the download: wrong id sent to MeTube's /delete MeTube's queue/pending/done stores are keyed by the download's URL (PersistentQueue.put: key = value.info.url), not by the id field a Download reports over Socket.IO (which is what we stored as metube_job_id and were sending). Sending the wrong key made MeTube's clear()/cancel() silently no-op ("requested delete for non-existent download" in its own logs) while still returning {"status": "ok"} regardless -- confirmed live by curling /history on the real instance and finding the "deleted" entry still present, unrelated to the DELETE_FILE_ON_TRASHCAN config fix that came right before this. delete_download() now takes the video's canonical youtube_url instead. Co-Authored-By: Claude Sonnet 5 --- backend/app/services/download_jobs.py | 4 +--- backend/app/services/metube_client.py | 17 ++++++++++++----- tests/test_videos.py | 9 ++++++--- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/backend/app/services/download_jobs.py b/backend/app/services/download_jobs.py index d8ac4b8..422ee31 100644 --- a/backend/app/services/download_jobs.py +++ b/backend/app/services/download_jobs.py @@ -75,11 +75,9 @@ def delete_local_copy(db: Session, video: Video) -> DownloadJob: job = get_latest_job(db, video.id) if job is None or job.status != "completed": raise DeleteNotAllowed("No completed local copy to delete") - if not job.metube_job_id: - raise DeleteNotAllowed("Missing MeTube job id, cannot request deletion") client = MeTubeClient() - client.delete_download(job.metube_job_id) + client.delete_download(video.youtube_url) # MeTube's /delete only unlinks the file if it's configured with # DELETE_FILE_ON_TRASHCAN=true -- otherwise it just drops the entry from diff --git a/backend/app/services/metube_client.py b/backend/app/services/metube_client.py index ffaf38b..029a193 100644 --- a/backend/app/services/metube_client.py +++ b/backend/app/services/metube_client.py @@ -77,16 +77,23 @@ class MeTubeClient: response.raise_for_status() return response.json() - def delete_download(self, metube_job_id: str) -> dict: + def delete_download(self, youtube_url: str) -> dict: """Asks MeTube to remove a finished download from its 'done' list and delete the underlying file (actual file deletion additionally depends on MeTube's own DELETE_FILE_ON_TRASHCAN config, which we don't - control). Only ever called with a metube_job_id our own app tracked - from a download it started -- never touches files MeTube already had - before we existed.""" + control). Only ever called for a URL our own app itself enqueued -- + never touches files MeTube already had before we existed. + + MeTube's queue/pending/done stores are keyed by the download's URL + (PersistentQueue.put: `key = value.info.url`), NOT by the id field a + Download reports over Socket.IO -- passing that id here silently + no-ops (MeTube logs "requested delete for non-existent download" and + still returns {"status": "ok"} regardless), which is what happened + before this was fixed: verified by curling this instance's /history + and finding the "deleted" entry still present.""" response = httpx.post( f"{self.api_base_url}/delete", - json={"ids": [metube_job_id], "where": "done"}, + json={"ids": [youtube_url], "where": "done"}, timeout=self.timeout, ) response.raise_for_status() diff --git a/tests/test_videos.py b/tests/test_videos.py index 3f62f52..437ce66 100644 --- a/tests/test_videos.py +++ b/tests/test_videos.py @@ -182,7 +182,7 @@ def test_delete_download_success(client, db_session, monkeypatch): calls = {} monkeypatch.setattr( "app.services.metube_client.MeTubeClient.delete_download", - lambda self, metube_job_id: calls.setdefault("id", metube_job_id) or {"status": "ok"}, + lambda self, youtube_url: calls.setdefault("youtube_url", youtube_url) or {"status": "ok"}, ) monkeypatch.setattr("app.services.metube_client.MeTubeClient.check_media", lambda self, url: False) @@ -191,7 +191,10 @@ def test_delete_download_success(client, db_session, monkeypatch): assert resp.status_code == 200 assert resp.json()["status"] == "deleted" assert resp.json()["media_url"] is None - assert calls["id"] == "vid1.vid1" + # MeTube's queue/done stores are keyed by URL, not by the id it reports + # over Socket.IO -- passing the wrong one is exactly the bug this + # regression test guards against (it silently no-ops on MeTube's side). + assert calls["youtube_url"] == video.youtube_url db_session.refresh(job) assert job.status == "deleted" @@ -214,7 +217,7 @@ def test_delete_download_file_still_present_returns_409(client, db_session, monk db_session.commit() monkeypatch.setattr( - "app.services.metube_client.MeTubeClient.delete_download", lambda self, metube_job_id: {"status": "ok"} + "app.services.metube_client.MeTubeClient.delete_download", lambda self, youtube_url: {"status": "ok"} ) monkeypatch.setattr("app.services.metube_client.MeTubeClient.check_media", lambda self, url: True)