From e19968e427418627afaddc3e4d21278e0787a18e Mon Sep 17 00:00:00 2001 From: aircode610 Date: Mon, 22 Jun 2026 16:49:40 +0200 Subject: [PATCH] fix(letters): add missing error logging to /api/letters/{id}/extract (#8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generic `except Exception` handler silently swallowed errors — the comment claimed they were "Logged by unhandled_exception_handler" but KlarHTTPException is caught by klar_exception_handler instead, so the original traceback was lost. Added logger.exception() to the generic handler and logger.info() to the typed PdfRenderError/ExtractionError handlers, matching what public.py already does. Added 3 regression tests covering this endpoint path. Co-Authored-By: Claude Opus 4.6 (1M context) --- backend/app/routers/letters.py | 22 +++++-- backend/tests/test_scanned_pdf_graceful.py | 77 ++++++++++++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/backend/app/routers/letters.py b/backend/app/routers/letters.py index 4812bbb..a018631 100644 --- a/backend/app/routers/letters.py +++ b/backend/app/routers/letters.py @@ -1,5 +1,6 @@ """Letter upload + retrieval endpoints (auth-required, /api/letters/*).""" +import logging from uuid import UUID from fastapi import APIRouter, Depends, File, Query, UploadFile @@ -27,6 +28,8 @@ from app.services.persistence import persist_extraction from app.services.storage import detect_magic_mime, save_letter_file +logger = logging.getLogger("klar.letters") + router = APIRouter(prefix="/api/letters", tags=["letters"]) ACCEPTED_MIMES = { @@ -221,24 +224,31 @@ async def extract_letter( letter.original_file, mime, lang=letter.language ) except PdfRenderError as exc: + # PDF couldn't be rendered (corrupt / poppler missing) — distinct, + # actionable message ("try uploading it as an image instead"). + logger.info("PDF render failed for letter %s: %s", letter_id, exc) letter.status = LetterStatus.ERROR db.add(letter) db.commit() - # PDF couldn't be rendered (corrupt / poppler missing) — distinct, - # actionable message ("try uploading it as an image instead"). raise KlarHTTPException(502, ErrorCode.PDF_RENDER_FAILED, message=str(exc)) except ExtractionError as exc: + # Scanned image-only PDF (no text layer) or malformed model output — + # surface the typed, user-friendly message instead of a raw 500. + logger.info("Extraction produced no text for letter %s: %s", letter_id, exc) letter.status = LetterStatus.ERROR db.add(letter) db.commit() - # Scanned image-only PDF (no text layer) or malformed model output — - # surface the typed, user-friendly message instead of a raw 500. raise KlarHTTPException(502, ErrorCode.EXTRACTION_FAILED, message=str(exc)) - except Exception: + except Exception as exc: + # Log the real error to the server console so we can diagnose. + # The 502 response stays generic on the wire to avoid leaking provider + # implementation details to the client. + logger.exception( + "Qwen extraction failed for letter %s: %s", letter_id, exc, + ) letter.status = LetterStatus.ERROR db.add(letter) db.commit() - # Don't leak the raw exception. Logged by unhandled_exception_handler. raise KlarHTTPException(502, ErrorCode.EXTRACTION_FAILED) actions = persist_extraction(db, letter, extracted) diff --git a/backend/tests/test_scanned_pdf_graceful.py b/backend/tests/test_scanned_pdf_graceful.py index 4a07122..6895d24 100644 --- a/backend/tests/test_scanned_pdf_graceful.py +++ b/backend/tests/test_scanned_pdf_graceful.py @@ -354,3 +354,80 @@ async def _render_fail(path): assert "event: error" in blob assert ErrorCode.PDF_RENDER_FAILED.value in blob + + +# -------------------------------------------------------------------------- +# 7. POST /api/letters/{id}/extract (letters.py) — sync extraction endpoint +# must return typed 502 errors with logging, NOT silently swallow them. +# -------------------------------------------------------------------------- + + +async def _call_extract_letter(monkeypatch, *, raise_exc): + """Drive letters.extract_letter with extraction stubbed to raise `raise_exc`. + + Returns the KlarHTTPException the handler raises (or None if it didn't). + """ + from app.routers import letters as letters_router + + async def _boom(*a, **k): + raise raise_exc + + monkeypatch.setattr(letters_router, "extract_from_letter_file", _boom) + + with Session(engine) as db: + user = _make_user(db) + letter = Letter( + user_id=user.id, + language="en", + status=LetterStatus.UPLOADED, + original_file="/tmp/scanned-no-text.pdf", + ) + db.add(letter) + db.commit() + db.refresh(letter) + try: + await letters_router.extract_letter( + letter_id=letter.id, db=db, user=user, + ) + except Exception as exc: # noqa: BLE001 — we assert on the typed error + return exc + return None + + +async def test_extract_letter_scanned_pdf_returns_extraction_failed(monkeypatch): + from app.errors import KlarHTTPException + + exc = await _call_extract_letter( + monkeypatch, + raise_exc=ExtractionError("scanned image without readable content"), + ) + assert isinstance(exc, KlarHTTPException) + assert exc.status_code == 502 + assert exc.code == ErrorCode.EXTRACTION_FAILED + assert "readable content" in exc.message + + +async def test_extract_letter_corrupt_pdf_returns_pdf_render_failed(monkeypatch): + from app.errors import KlarHTTPException + + exc = await _call_extract_letter( + monkeypatch, + raise_exc=PdfRenderError("Could not render this PDF. It may be corrupt."), + ) + assert isinstance(exc, KlarHTTPException) + assert exc.status_code == 502 + assert exc.code == ErrorCode.PDF_RENDER_FAILED + assert "corrupt" in exc.message + + +async def test_extract_letter_unexpected_error_stays_generic(monkeypatch): + from app.errors import KlarHTTPException + + exc = await _call_extract_letter( + monkeypatch, raise_exc=RuntimeError("some provider 500 with secrets") + ) + assert isinstance(exc, KlarHTTPException) + assert exc.status_code == 502 + assert exc.code == ErrorCode.EXTRACTION_FAILED + # Raw provider error must NOT leak into the user-facing message. + assert "secrets" not in exc.message