diff --git a/CHANGELOG.md b/CHANGELOG.md index f02c85c8..73d7db8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The legacy provider/model string fields on podcast profiles (`outline_provider`, `outline_model`, `transcript_provider`, `transcript_model` on episode profiles; `tts_provider`, `tts_model` on speaker profiles) are gone from the database, the API and the UI — the app has ignored them since the Model registry references landed in v1.11. Migration 22 first best-effort maps any profile whose `outline_llm`/`transcript_llm`/`voice_model` reference is still empty to an existing model record (matching provider + name + type; no auto-creation, since a migration must not touch credentials), then drops the six columns; the startup data migration that used to retry this mapping on every boot (`open_notebook/podcasts/migration.py`) was deleted. Accepted trade-off: profiles whose mapping never converged (e.g. the provider credential was never configured) lose the legacy strings and stay unresolved — they were already non-functional and the UI already flags them as needing model selection, so you just re-pick the models in the profile form once (#1107) ### Fixed +- Uploading a file content-core can't extract now fails immediately at ingestion with a clear `415 Unsupported Media Type` error that names the detected MIME type, instead of enqueueing a background job that retried up to 15 times over ~1 hour before surfacing a generic "Failed" with no actionable detail. The pre-flight uses content-core 2.x's header-only `check_file_support()` — the same routing real extraction uses, so the verdict can't disagree with what would happen downstream — and the source-retry endpoint is guarded the same way; unexpected check errors (e.g. a file removed before a retry) fall through to normal extraction rather than becoming a hard rejection (#975) - Podcast episode cards no longer show "— / —" for the outline, transcript and speaker model rows on new episodes: the API now resolves the snapshot's model references (`outline_llm`/`transcript_llm`/`voice_model`) to provider/name display fields at serialization time — batched into a single query per request, so listing episodes never does a per-row model lookup — and the card falls back to the legacy snapshot strings for old episodes and degrades to "—" when a referenced model was deleted (#1114) - Renaming a speaker profile no longer breaks the episode profiles that use it: `episode_profile.speaker_config` now stores a `record` reference instead of the profile name (migration 20 converts existing rows; references whose speaker profile no longer exists at migration time become null, and any reference that later stops resolving is treated as "needs setup" — the UI asks you to pick a speaker again). The `POST /api/podcasts/generate` contract is unchanged — it still accepts the speaker profile by name and resolves it at the API boundary (#630) - Clicking a chat/Ask citation that points at a deleted source, insight, or note now shows a shared, friendly "this content no longer exists" state in all three dialogs (instead of a raw error, a blank dialog, or an empty editable note editor), 404 lookups are no longer retried, and non-404 failures show a distinct "unable to load" message (#455) diff --git a/api/main.py b/api/main.py index ad6120a7..4d4ed468 100644 --- a/api/main.py +++ b/api/main.py @@ -48,6 +48,7 @@ from open_notebook.exceptions import ( NotFoundError, OpenNotebookError, RateLimitError, + UnsupportedTypeException, ) from open_notebook.utils.encryption import get_secret_from_env @@ -350,6 +351,17 @@ async def external_service_error_handler(request: Request, exc: ExternalServiceE ) +@app.exception_handler(UnsupportedTypeException) +async def unsupported_type_error_handler( + request: Request, exc: UnsupportedTypeException +): + return JSONResponse( + status_code=415, + content={"detail": str(exc)}, + headers=_cors_headers(request), + ) + + @app.exception_handler(OpenNotebookError) async def open_notebook_error_handler(request: Request, exc: OpenNotebookError): return JSONResponse( diff --git a/api/routers/sources.py b/api/routers/sources.py index f2b64673..c7ff97cd 100644 --- a/api/routers/sources.py +++ b/api/routers/sources.py @@ -3,6 +3,7 @@ import os from pathlib import Path from typing import Any, List, Optional +from content_core import check_file_support from fastapi import ( APIRouter, Depends, @@ -39,10 +40,37 @@ from open_notebook.exceptions import ( InvalidInputError, NotFoundError, OpenNotebookError, + UnsupportedTypeException, ) router = APIRouter() + +async def _assert_file_supported(file_path: str) -> None: + """Pre-flight check that content-core can actually extract this file. + + Rejects unsupported uploads at ingestion time (HTTP 415) instead of letting + them enqueue a background job that fails and then burns the full retry + budget before surfacing a generic error (#975). Uses content-core's + header-only routing, which is the same logic real extraction uses, so the + verdict can't disagree with what would happen downstream. + + Unexpected errors (e.g. the file no longer exists on a retry) are swallowed + so this never turns a transient problem into a hard rejection — real + extraction will surface those. + """ + try: + support = await check_file_support(file_path) + except Exception as e: # pragma: no cover - defensive + logger.debug(f"Pre-flight file-support check skipped for {file_path}: {e}") + return + + if not support.supported: + detail = support.reason or "Unsupported file type" + if support.identified_type: + detail = f"{detail} (detected type: {support.identified_type})" + raise UnsupportedTypeException(detail) + SOURCE_SORT_FIELDS = { "created": "created", "updated": "updated", @@ -416,6 +444,8 @@ async def _build_content_state( status_code=400, detail="Invalid file path: must be within the uploads directory", ) + # Reject unsupported files before enqueueing a doomed background job. + await _assert_file_supported(final_file_path) content_state["file_path"] = final_file_path content_state["delete_source"] = source_data.delete_source elif source_data.type == "text": @@ -931,6 +961,8 @@ async def retry_source_processing(source_id: str): content_state = {} if source.asset: if source.asset.file_path: + # Don't re-queue a retry for a file content-core can't extract. + await _assert_file_supported(source.asset.file_path) content_state = { "file_path": source.asset.file_path, "delete_source": False, # Don't delete on retry diff --git a/tests/test_typed_exceptions_reach_handlers.py b/tests/test_typed_exceptions_reach_handlers.py index 7eaf7e73..ac425f1b 100644 --- a/tests/test_typed_exceptions_reach_handlers.py +++ b/tests/test_typed_exceptions_reach_handlers.py @@ -91,6 +91,28 @@ class TestNotFoundErrorPropagation: assert response.json()["detail"] == "Credential not found" +class TestUnsupportedTypeErrorPropagation: + """UnsupportedTypeException maps to 415 (Unsupported Media Type) via its + dedicated global handler, instead of falling through to the base + OpenNotebookError handler's 500 (#975).""" + + def test_unsupported_type_maps_to_415(self, client): + from open_notebook.exceptions import UnsupportedTypeException + + with patch( + "api.routers.sources.repo_query", + new=AsyncMock( + side_effect=UnsupportedTypeException( + "Unsupported file type: application/zip" + ) + ), + ): + response = client.get("/api/sources") + + assert response.status_code == 415 + assert "application/zip" in response.json()["detail"] + + class TestUntypedExceptionsStillSanitized: """The final `except Exception` arm must keep catching untyped errors and returning a fixed, sanitized 500 (never the raw exception text).""" diff --git a/tests/test_upload_type_mitigations.py b/tests/test_upload_type_mitigations.py index c4981ced..b92b072a 100644 --- a/tests/test_upload_type_mitigations.py +++ b/tests/test_upload_type_mitigations.py @@ -92,3 +92,63 @@ class TestContentCoreRejectsUnrecognizedContent: detector = FileDetector() with pytest.raises(UnsupportedTypeException): await detector.detect(str(garbage)) + + +class TestPreflightFileSupport: + """The upload flow rejects unsupported files at ingestion via content-core's + check_file_support(), so they never enqueue a doomed background job (#975).""" + + @pytest.mark.asyncio + async def test_unsupported_verdict_raises(self): + from content_core import FileSupport + + from api.routers.sources import _assert_file_supported + from open_notebook.exceptions import UnsupportedTypeException + + verdict = FileSupport( + supported=False, + file_path="/uploads/archive.zip", + identified_type="application/zip", + reason="Unsupported file type: application/zip", + ) + with patch( + "api.routers.sources.check_file_support", + new=AsyncMock(return_value=verdict), + ): + with pytest.raises(UnsupportedTypeException) as exc: + await _assert_file_supported("/uploads/archive.zip") + + # message carries both the reason and the detected type for the UI + assert "application/zip" in str(exc.value) + + @pytest.mark.asyncio + async def test_supported_verdict_passes(self): + from content_core import FileSupport + + from api.routers.sources import _assert_file_supported + + verdict = FileSupport( + supported=True, + file_path="/uploads/doc.pdf", + identified_type="application/pdf", + processor="pdf", + ) + with patch( + "api.routers.sources.check_file_support", + new=AsyncMock(return_value=verdict), + ): + # must not raise + await _assert_file_supported("/uploads/doc.pdf") + + @pytest.mark.asyncio + async def test_check_errors_are_swallowed(self): + """A missing file on retry (or any check error) must not become a hard + 415 — extraction downstream will surface the real problem.""" + from api.routers.sources import _assert_file_supported + + with patch( + "api.routers.sources.check_file_support", + new=AsyncMock(side_effect=FileNotFoundError("gone")), + ): + # must not raise + await _assert_file_supported("/uploads/missing.pdf")