Verify deletion actually removed the file before reporting success
MeTube's /delete only unlinks the file when it's configured with
DELETE_FILE_ON_TRASHCAN=true; otherwise it just drops the entry from its
own "done" list and returns {"status": "ok"} regardless -- we were trusting
that response and marking the job "deleted" (offering a re-download) while
the file was still sitting on mediaVM's disk the whole time.
Now HEAD-check the media_url right after the delete call. If the file is
still reachable, leave the job's status untouched (still "completed", still
playable) and surface a clear 409 explaining MeTube's own config is why,
rather than lying about local state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
8ddea8762d
commit
441a3ef9df
4 changed files with 65 additions and 1 deletions
|
|
@ -9,6 +9,7 @@ from app.models.channel import Channel
|
|||
from app.models.download_job import DownloadJob
|
||||
from app.models.video import Video
|
||||
from app.services.download_jobs import (
|
||||
DeleteDidNotRemoveFile,
|
||||
DeleteNotAllowed,
|
||||
MeTubeRejected,
|
||||
delete_local_copy,
|
||||
|
|
@ -78,6 +79,14 @@ def delete_download(youtube_video_id: str, db: Session = Depends(get_db)) -> dic
|
|||
job = delete_local_copy(db, video)
|
||||
except DeleteNotAllowed as exc:
|
||||
raise HTTPException(status_code=400, detail=str(exc))
|
||||
except DeleteDidNotRemoveFile:
|
||||
raise HTTPException(
|
||||
status_code=409,
|
||||
detail=(
|
||||
"MeTube убрал запись из своего списка, но файл остался на диске "
|
||||
"(на mediaVM выключена настройка DELETE_FILE_ON_TRASHCAN)"
|
||||
),
|
||||
)
|
||||
except Exception:
|
||||
logger.exception("Failed to delete local copy for %s", youtube_video_id)
|
||||
raise HTTPException(status_code=502, detail="MeTube is unavailable")
|
||||
|
|
|
|||
|
|
@ -19,6 +19,15 @@ class DeleteNotAllowed(Exception):
|
|||
pass
|
||||
|
||||
|
||||
class DeleteDidNotRemoveFile(Exception):
|
||||
"""MeTube accepted the /delete request but the file is still reachable
|
||||
afterwards -- its DELETE_FILE_ON_TRASHCAN setting is very likely off on
|
||||
that instance, which is outside this app's control (see AGENTS.md
|
||||
constraint 11: never touch MeTube's own config/source)."""
|
||||
|
||||
pass
|
||||
|
||||
|
||||
def get_latest_job(db: Session, video_id: int) -> DownloadJob | None:
|
||||
return (
|
||||
db.query(DownloadJob)
|
||||
|
|
@ -69,7 +78,18 @@ def delete_local_copy(db: Session, video: Video) -> DownloadJob:
|
|||
if not job.metube_job_id:
|
||||
raise DeleteNotAllowed("Missing MeTube job id, cannot request deletion")
|
||||
|
||||
MeTubeClient().delete_download(job.metube_job_id)
|
||||
client = MeTubeClient()
|
||||
client.delete_download(job.metube_job_id)
|
||||
|
||||
# MeTube's /delete only unlinks the file if it's configured with
|
||||
# DELETE_FILE_ON_TRASHCAN=true -- otherwise it just drops the entry from
|
||||
# its own "done" list and the file stays put. We don't control that
|
||||
# instance's config, so verify rather than trust the "ok" response.
|
||||
if job.media_url and client.check_media(job.media_url):
|
||||
raise DeleteDidNotRemoveFile(
|
||||
"MeTube removed the entry but the file is still on disk "
|
||||
"(its DELETE_FILE_ON_TRASHCAN setting is likely off)"
|
||||
)
|
||||
|
||||
job.status = "deleted"
|
||||
job.media_url = None
|
||||
|
|
|
|||
|
|
@ -75,6 +75,11 @@ function DownloadButton({ video }: Props) {
|
|||
>
|
||||
Удалить
|
||||
</button>
|
||||
{deleteMutation.isError && (
|
||||
<span className="download-badge error" title={(deleteMutation.error as Error).message}>
|
||||
{(deleteMutation.error as Error).message}
|
||||
</span>
|
||||
)}
|
||||
</span>
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -184,6 +184,7 @@ def test_delete_download_success(client, db_session, monkeypatch):
|
|||
"app.services.metube_client.MeTubeClient.delete_download",
|
||||
lambda self, metube_job_id: calls.setdefault("id", metube_job_id) or {"status": "ok"},
|
||||
)
|
||||
monkeypatch.setattr("app.services.metube_client.MeTubeClient.check_media", lambda self, url: False)
|
||||
|
||||
resp = client.delete(f"/api/videos/{video.youtube_video_id}/download")
|
||||
|
||||
|
|
@ -195,3 +196,32 @@ def test_delete_download_success(client, db_session, monkeypatch):
|
|||
db_session.refresh(job)
|
||||
assert job.status == "deleted"
|
||||
assert job.media_url is None
|
||||
|
||||
|
||||
def test_delete_download_file_still_present_returns_409(client, db_session, monkeypatch):
|
||||
"""MeTube can accept /delete and still leave the file on disk if its own
|
||||
DELETE_FILE_ON_TRASHCAN config is off -- we must not lie about that."""
|
||||
from app.models.download_job import DownloadJob
|
||||
|
||||
video = _seed_single_video(db_session)
|
||||
job = DownloadJob(
|
||||
video_id=video.id,
|
||||
status="completed",
|
||||
metube_job_id="vid1.vid1",
|
||||
media_url="http://metube.local/download/f.mp4",
|
||||
)
|
||||
db_session.add(job)
|
||||
db_session.commit()
|
||||
|
||||
monkeypatch.setattr(
|
||||
"app.services.metube_client.MeTubeClient.delete_download", lambda self, metube_job_id: {"status": "ok"}
|
||||
)
|
||||
monkeypatch.setattr("app.services.metube_client.MeTubeClient.check_media", lambda self, url: True)
|
||||
|
||||
resp = client.delete(f"/api/videos/{video.youtube_video_id}/download")
|
||||
|
||||
assert resp.status_code == 409
|
||||
|
||||
db_session.refresh(job)
|
||||
assert job.status == "completed"
|
||||
assert job.media_url == "http://metube.local/download/f.mp4"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue