diff --git a/py/middleware/error_middleware.py b/py/middleware/error_middleware.py index e6f99051..69c29592 100644 --- a/py/middleware/error_middleware.py +++ b/py/middleware/error_middleware.py @@ -46,6 +46,16 @@ async def api_json_error( if request.path.startswith("/api/lm/previews") and exc.status == 404: logger_method = logger.debug + # Download-progress 404 is routine too: in-memory tracking is removed + # once a download finishes/fails, so the extension's final polls 404. + # The extension relies on the 404 status itself (failure detection), + # so only the log level is lowered. + if ( + request.path.startswith("/api/lm/download-progress/") + and exc.status == 404 + ): + logger_method = logger.debug + logger_method( "API %s %s returned HTTP %d: %s", request.method, diff --git a/py/routes/handlers/model_handlers.py b/py/routes/handlers/model_handlers.py index 669dd3c6..6b26c652 100644 --- a/py/routes/handlers/model_handlers.py +++ b/py/routes/handlers/model_handlers.py @@ -1998,9 +1998,11 @@ class ModelDownloadHandler: item_id=item_id, download_id=download_id ) if item is None: + # Missing or non-retryable history entry is a business + # outcome, not a routing error: 200 lets the extension's + # apiFetch 404-fallback and error middleware stay quiet. return web.json_response( - {"success": False, "error": "History item not found or not retryable"}, - status=404, + {"success": False, "error": "History item not found or not retryable"} ) return web.json_response({"success": True, "item": item}) except Exception as exc: @@ -2051,8 +2053,12 @@ class ModelDownloadHandler: completed_at=completed_at, ) if item is None: + # A missing queue item (already completed, or never queued) is + # a normal business outcome, not a routing error. Return 200 + # so the browser extension's apiFetch 404-fallback and the + # error middleware stay quiet. return web.json_response( - {"success": False, "error": "Download not found in queue"}, status=404 + {"success": False, "error": "Download not found in queue"} ) return web.json_response({"success": True, "item": item}) except Exception as exc: @@ -2094,9 +2100,10 @@ class ModelDownloadHandler: service = await DownloadQueueService.get_instance() updated = await service.update_status(download_id, status) if not updated: + # Same rationale as complete_download_in_queue: a missing + # queue item is a business outcome, not a routing error. return web.json_response( - {"success": False, "error": "Download not found in queue"}, - status=404, + {"success": False, "error": "Download not found in queue"} ) return web.json_response({"success": True}) except Exception as exc: diff --git a/tests/routes/test_download_queue_handlers.py b/tests/routes/test_download_queue_handlers.py new file mode 100644 index 00000000..a838426a --- /dev/null +++ b/tests/routes/test_download_queue_handlers.py @@ -0,0 +1,186 @@ +"""Handler-level tests for download queue terminal/status transitions. + +Regression test: a "not found in queue" outcome must be returned as HTTP 200 +with ``success: false``, not 404. The browser extension's apiFetch treats any +404 as a missing endpoint and retries a legacy URL, producing spurious +``/api/downloads/queue/complete ... 404`` warnings on every completion. +""" + +import json +import logging +from pathlib import Path + +import pytest +from aiohttp import web +from aiohttp.test_utils import make_mocked_request + +from py.routes.handlers.model_handlers import ModelDownloadHandler +from py.services.download_queue_service import DownloadQueueService + + +def _make_handler() -> ModelDownloadHandler: + return ModelDownloadHandler( + ws_manager=None, # pyright: ignore[reportArgumentType] - unused by queue endpoints + logger=logging.getLogger("test-download-queue"), + download_use_case=None, # pyright: ignore[reportArgumentType] - unused by queue endpoints + download_coordinator=None, # pyright: ignore[reportArgumentType] - unused by queue endpoints + ) + + +def _queue_request(path: str, query: dict[str, str]) -> web.Request: + query_string = "&".join(f"{key}={value}" for key, value in query.items()) + return make_mocked_request("GET", f"{path}?{query_string}") + + +@pytest.fixture +def queue_service( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> DownloadQueueService: + """Return a tmp-backed DownloadQueueService and stub the singleton.""" + service = DownloadQueueService(db_path=str(tmp_path / "queue.sqlite")) + + async def fake_get_instance(_cls: object = None) -> DownloadQueueService: + return service + + monkeypatch.setattr(DownloadQueueService, "get_instance", fake_get_instance) + return service + + +@pytest.mark.asyncio +async def test_complete_missing_download_returns_200_not_404( + queue_service: DownloadQueueService, +) -> None: + """Completing a download that is not queued is a business outcome.""" + handler = _make_handler() + response = await handler.complete_download_in_queue( + _queue_request( + "/api/lm/downloads/queue/complete", + {"download_id": "dl-nope", "status": "completed"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + assert json.loads(text) == { + "success": False, + "error": "Download not found in queue", + } + # The failed completion must not have side effects on the queue. + assert await queue_service.get_queue() == [] + + +@pytest.mark.asyncio +async def test_complete_queued_download_returns_success( + queue_service: DownloadQueueService, +) -> None: + """The happy path still moves the item to history with HTTP 200.""" + await queue_service.add_to_queue(download_id="dl-1", model_id=1) + handler = _make_handler() + response = await handler.complete_download_in_queue( + _queue_request( + "/api/lm/downloads/queue/complete", + {"download_id": "dl-1", "status": "completed"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + payload = json.loads(text) + assert payload["success"] is True + # The returned item reflects the pre-transition queue record; the + # terminal status lands in history. + history = await queue_service.get_history() + assert len(history["items"]) == 1 + assert history["items"][0]["status"] == "completed" + + +@pytest.mark.asyncio +async def test_status_missing_download_returns_200_not_404( + queue_service: DownloadQueueService, +) -> None: + """Status updates for unknown items also return 200 with success: false.""" + handler = _make_handler() + response = await handler.update_download_queue_status( + _queue_request( + "/api/lm/downloads/queue/status", + {"download_id": "dl-nope", "status": "downloading"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + assert json.loads(text) == { + "success": False, + "error": "Download not found in queue", + } + assert await queue_service.get_queue() == [] + + +@pytest.mark.asyncio +async def test_status_queued_download_returns_success( + queue_service: DownloadQueueService, +) -> None: + """The happy path still updates the queue item with HTTP 200.""" + await queue_service.add_to_queue(download_id="dl-2", model_id=2) + handler = _make_handler() + response = await handler.update_download_queue_status( + _queue_request( + "/api/lm/downloads/queue/status", + {"download_id": "dl-2", "status": "downloading"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + assert json.loads(text) == {"success": True} + + +@pytest.mark.asyncio +async def test_retry_missing_history_returns_200_not_404( + queue_service: DownloadQueueService, +) -> None: + """Retrying a history entry that no longer exists is a business outcome.""" + handler = _make_handler() + response = await handler.retry_download_from_history( + _queue_request( + "/api/lm/downloads/history/retry", + {"download_id": "dl-nope"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + assert json.loads(text) == { + "success": False, + "error": "History item not found or not retryable", + } + # No side effects: history and queue stay empty. + history = await queue_service.get_history() + assert history["items"] == [] + assert await queue_service.get_queue() == [] + + +@pytest.mark.asyncio +async def test_retry_failed_history_returns_success( + queue_service: DownloadQueueService, +) -> None: + """The happy path still re-queues a retryable history entry with HTTP 200.""" + await queue_service.add_to_history( + download_id="dl-fail", model_id=1, status="failed" + ) + handler = _make_handler() + response = await handler.retry_download_from_history( + _queue_request( + "/api/lm/downloads/history/retry", + {"download_id": "dl-fail"}, + ) + ) + assert response.status == 200 + text = response.text + assert text is not None + payload = json.loads(text) + assert payload["success"] is True + # The retried item is re-queued under a fresh download_id. + queue = await queue_service.get_queue() + assert len(queue) == 1 + assert queue[0]["status"] == "queued"