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 <noreply@anthropic.com>
This commit is contained in:
parent
441a3ef9df
commit
62f8fb746b
3 changed files with 19 additions and 11 deletions
|
|
@ -75,11 +75,9 @@ def delete_local_copy(db: Session, video: Video) -> DownloadJob:
|
||||||
job = get_latest_job(db, video.id)
|
job = get_latest_job(db, video.id)
|
||||||
if job is None or job.status != "completed":
|
if job is None or job.status != "completed":
|
||||||
raise DeleteNotAllowed("No completed local copy to delete")
|
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 = 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
|
# MeTube's /delete only unlinks the file if it's configured with
|
||||||
# DELETE_FILE_ON_TRASHCAN=true -- otherwise it just drops the entry from
|
# DELETE_FILE_ON_TRASHCAN=true -- otherwise it just drops the entry from
|
||||||
|
|
|
||||||
|
|
@ -77,16 +77,23 @@ class MeTubeClient:
|
||||||
response.raise_for_status()
|
response.raise_for_status()
|
||||||
return response.json()
|
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
|
"""Asks MeTube to remove a finished download from its 'done' list and
|
||||||
delete the underlying file (actual file deletion additionally depends
|
delete the underlying file (actual file deletion additionally depends
|
||||||
on MeTube's own DELETE_FILE_ON_TRASHCAN config, which we don't
|
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
|
control). Only ever called for a URL our own app itself enqueued --
|
||||||
from a download it started -- never touches files MeTube already had
|
never touches files MeTube already had before we existed.
|
||||||
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(
|
response = httpx.post(
|
||||||
f"{self.api_base_url}/delete",
|
f"{self.api_base_url}/delete",
|
||||||
json={"ids": [metube_job_id], "where": "done"},
|
json={"ids": [youtube_url], "where": "done"},
|
||||||
timeout=self.timeout,
|
timeout=self.timeout,
|
||||||
)
|
)
|
||||||
response.raise_for_status()
|
response.raise_for_status()
|
||||||
|
|
|
||||||
|
|
@ -182,7 +182,7 @@ def test_delete_download_success(client, db_session, monkeypatch):
|
||||||
calls = {}
|
calls = {}
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
"app.services.metube_client.MeTubeClient.delete_download",
|
"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)
|
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.status_code == 200
|
||||||
assert resp.json()["status"] == "deleted"
|
assert resp.json()["status"] == "deleted"
|
||||||
assert resp.json()["media_url"] is None
|
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)
|
db_session.refresh(job)
|
||||||
assert job.status == "deleted"
|
assert job.status == "deleted"
|
||||||
|
|
@ -214,7 +217,7 @@ def test_delete_download_file_still_present_returns_409(client, db_session, monk
|
||||||
db_session.commit()
|
db_session.commit()
|
||||||
|
|
||||||
monkeypatch.setattr(
|
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)
|
monkeypatch.setattr("app.services.metube_client.MeTubeClient.check_media", lambda self, url: True)
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue