diff --git a/py/routes/handlers/model_handlers.py b/py/routes/handlers/model_handlers.py index 821aab37..ff09209d 100644 --- a/py/routes/handlers/model_handlers.py +++ b/py/routes/handlers/model_handlers.py @@ -1787,14 +1787,20 @@ class ModelDownloadHandler: async def delete_download_history_item(self, request: web.Request) -> web.Response: try: - item_id = int(request.query.get("id", "0")) - if not item_id: + download_id = request.query.get("download_id") + id_str = request.query.get("id") + item_id = int(id_str) if id_str else None + + if not download_id and not item_id: return web.json_response( - {"success": False, "error": "id is required"}, status=400 + {"success": False, "error": "id or download_id is required"}, + status=400, ) service = await DownloadQueueService.get_instance() - deleted = await service.delete_history_item(item_id) + deleted = await service.delete_history_item( + id=item_id, download_id=download_id + ) return web.json_response({"success": deleted}) except Exception as exc: self._logger.error( @@ -1804,14 +1810,20 @@ class ModelDownloadHandler: async def retry_download_from_history(self, request: web.Request) -> web.Response: try: - item_id = int(request.query.get("id", "0")) - if not item_id: + download_id = request.query.get("download_id") + id_str = request.query.get("id") + item_id = int(id_str) if id_str else None + + if not download_id and not item_id: return web.json_response( - {"success": False, "error": "id is required"}, status=400 + {"success": False, "error": "id or download_id is required"}, + status=400, ) service = await DownloadQueueService.get_instance() - item = await service.retry_from_history(item_id) + item = await service.retry_from_history( + item_id=item_id, download_id=download_id + ) if item is None: return web.json_response( {"success": False, "error": "History item not found or not retryable"}, diff --git a/py/services/download_queue_service.py b/py/services/download_queue_service.py index 4eb22956..90a58f85 100644 --- a/py/services/download_queue_service.py +++ b/py/services/download_queue_service.py @@ -74,6 +74,8 @@ class DownloadQueueService: ); CREATE INDEX IF NOT EXISTS idx_dh_completed ON download_history(completed_at DESC); CREATE INDEX IF NOT EXISTS idx_dh_status ON download_history(status); + CREATE UNIQUE INDEX IF NOT EXISTS idx_dh_download_id + ON download_history(download_id) WHERE download_id IS NOT NULL; """ @classmethod @@ -390,7 +392,7 @@ class DownloadQueueService: ) conn.execute( """ - INSERT INTO download_history ( + INSERT OR IGNORE INTO download_history ( download_id, model_id, model_version_id, model_name, version_name, thumbnail_url, status, error, file_path, bytes_downloaded, total_bytes, completed_at @@ -547,17 +549,27 @@ class DownloadQueueService: "offset": offset, } - async def delete_history_item(self, id: int) -> bool: - """Delete a single history entry by its *id*. + async def delete_history_item( + self, id: Optional[int] = None, download_id: Optional[str] = None + ) -> bool: + """Delete a single history entry by *download_id* (preferred) or *id*. Returns ``True`` if a row was deleted. """ async with self._lock: conn = self._get_conn() - cursor = conn.execute( - "DELETE FROM download_history WHERE id = ?", - (id,), - ) + if download_id: + cursor = conn.execute( + "DELETE FROM download_history WHERE download_id = ?", + (download_id,), + ) + elif id is not None: + cursor = conn.execute( + "DELETE FROM download_history WHERE id = ?", + (id,), + ) + else: + return False conn.commit() return cursor.rowcount > 0 @@ -614,21 +626,34 @@ class DownloadQueueService: # Retry # ------------------------------------------------------------------ - async def retry_from_history(self, item_id: int) -> Optional[dict[str, Any]]: + async def retry_from_history( + self, + item_id: Optional[int] = None, + download_id: Optional[str] = None, + ) -> Optional[dict[str, Any]]: """Re-queue a failed or canceled download from history. - Looks up the history record by its primary key. If the status is - ``failed`` or ``canceled`` a new queue entry is created with the - same model metadata and a fresh download id, and the original - history entry is **deleted** to prevent exponential growth when - the retried item is later canceled or fails again and re-retried. + Looks up the history record by *download_id* (preferred) or + *item_id*. If the status is ``failed`` or ``canceled`` a new + queue entry is created with the same model metadata and a fresh + download id, and the original history entry is **deleted** to + prevent exponential growth when the retried item is later + canceled or fails again and re-retried. """ async with self._lock: conn = self._get_conn() - row = conn.execute( - "SELECT * FROM download_history WHERE id = ?", - (item_id,), - ).fetchone() + if download_id: + row = conn.execute( + "SELECT * FROM download_history WHERE download_id = ?", + (download_id,), + ).fetchone() + elif item_id is not None: + row = conn.execute( + "SELECT * FROM download_history WHERE id = ?", + (item_id,), + ).fetchone() + else: + return None if row is None: return None status = str(row["status"]) @@ -660,7 +685,7 @@ class DownloadQueueService: ) conn.execute( "DELETE FROM download_history WHERE id = ?", - (item_id,), + (row["id"],), ) conn.commit() queued = conn.execute( diff --git a/tests/services/test_download_queue_service.py b/tests/services/test_download_queue_service.py new file mode 100644 index 00000000..c46c7794 --- /dev/null +++ b/tests/services/test_download_queue_service.py @@ -0,0 +1,193 @@ +"""Unit tests for DownloadQueueService history operations. + +Covers the new ``download_id``-based code paths in +``delete_history_item`` and ``retry_from_history``, plus backward +compatibility with ``id``. +""" + +from pathlib import Path + +import pytest + +from py.services.download_queue_service import DownloadQueueService + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_service(tmp_path: Path) -> DownloadQueueService: + """Create a DownloadQueueService backed by a temporary database.""" + return DownloadQueueService(db_path=str(tmp_path / "queue.sqlite")) + + +async def _seed( + svc: DownloadQueueService, + download_id: str, + status: str = "failed", +) -> tuple[int, str]: + """Insert a history row and return (autoincrement id, download_id).""" + row_id = await svc.add_to_history( + download_id=download_id, + model_id=1, + model_version_id=100, + model_name="TestModel", + version_name="v1", + status=status, + ) + return row_id, download_id + + +# --------------------------------------------------------------------------- +# delete_history_item +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_delete_by_download_id(tmp_path: Path) -> None: + """delete_history_item(download_id=...) removes the correct row.""" + svc = _make_service(tmp_path) + rid, did = await _seed(svc, "dl-aaa") + + deleted = await svc.delete_history_item(download_id=did) + assert deleted is True + + # Verify gone from history + history = await svc.get_history() + assert len(history["items"]) == 0 + + +@pytest.mark.asyncio +async def test_delete_by_id_legacy(tmp_path: Path) -> None: + """delete_history_item(id=...) still works (backward compat).""" + svc = _make_service(tmp_path) + rid, _did = await _seed(svc, "dl-bbb") + + deleted = await svc.delete_history_item(id=rid) + assert deleted is True + + history = await svc.get_history() + assert len(history["items"]) == 0 + + +@pytest.mark.asyncio +async def test_delete_no_params_returns_false(tmp_path: Path) -> None: + """Calling delete_history_item with no params returns False.""" + svc = _make_service(tmp_path) + await _seed(svc, "dl-ccc") + + deleted = await svc.delete_history_item() + assert deleted is False + + # Row is still there + history = await svc.get_history() + assert len(history["items"]) == 1 + + +@pytest.mark.asyncio +async def test_delete_download_id_precedence(tmp_path: Path) -> None: + """When both id and download_id are given, download_id is used.""" + svc = _make_service(tmp_path) + # Insert two rows + rid_a, did_a = await _seed(svc, "dl-aaa") + rid_b, did_b = await _seed(svc, "dl-bbb") + + # Delete by download_id while also passing the *wrong* id + deleted = await svc.delete_history_item(id=rid_b, download_id=did_a) + assert deleted is True + + history = await svc.get_history() + ids_left = [it["id"] for it in history["items"]] + assert rid_a not in ids_left # dl-aaa was deleted + assert rid_b in ids_left # dl-bbb (wrong id) was ignored + + +# --------------------------------------------------------------------------- +# retry_from_history +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_retry_by_download_id(tmp_path: Path) -> None: + """retry_from_history(download_id=...) re-queues and deletes history.""" + svc = _make_service(tmp_path) + rid, did = await _seed(svc, "dl-fail", status="failed") + + item = await svc.retry_from_history(download_id=did) + assert item is not None + assert item["status"] == "queued" + + # History row must be deleted (the bug fix) + history = await svc.get_history() + ids_in_history = [it["id"] for it in history["items"]] + assert rid not in ids_in_history + + # Queue must contain the new item + queue = await svc.get_queue() + assert len(queue) == 1 + + +@pytest.mark.asyncio +async def test_retry_by_download_id_canceled(tmp_path: Path) -> None: + """retry_from_history works for 'canceled' status too.""" + svc = _make_service(tmp_path) + rid, did = await _seed(svc, "dl-cancel", status="canceled") + + item = await svc.retry_from_history(download_id=did) + assert item is not None + assert item["status"] == "queued" + + history = await svc.get_history() + assert len(history["items"]) == 0 + + +@pytest.mark.asyncio +async def test_retry_by_id_legacy(tmp_path: Path) -> None: + """retry_from_history(item_id=...) still works (backward compat).""" + svc = _make_service(tmp_path) + rid, _did = await _seed(svc, "dl-legacy", status="failed") + + item = await svc.retry_from_history(item_id=rid) + assert item is not None + assert item["status"] == "queued" + + history = await svc.get_history() + assert len(history["items"]) == 0 + + +@pytest.mark.asyncio +async def test_retry_no_params_returns_none(tmp_path: Path) -> None: + """Calling retry_from_history with no params returns None.""" + svc = _make_service(tmp_path) + await _seed(svc, "dl-none", status="failed") + + item = await svc.retry_from_history() + assert item is None + + # History untouched + history = await svc.get_history() + assert len(history["items"]) == 1 + + +@pytest.mark.asyncio +async def test_retry_non_retryable_status(tmp_path: Path) -> None: + """retry_from_history returns None for 'completed' status.""" + svc = _make_service(tmp_path) + _rid, did = await _seed(svc, "dl-ok", status="completed") + + item = await svc.retry_from_history(download_id=did) + assert item is None + + # History untouched + history = await svc.get_history() + assert len(history["items"]) == 1 + + +@pytest.mark.asyncio +async def test_retry_unknown_download_id(tmp_path: Path) -> None: + """retry_from_history returns None for a non-existent download_id.""" + svc = _make_service(tmp_path) + await _seed(svc, "dl-real", status="failed") + + item = await svc.retry_from_history(download_id="dl-nope") + assert item is None