From 511cbd61b9ca251acc3d2a2b4084822436f0cea9 Mon Sep 17 00:00:00 2001 From: Leo Vasanko Date: Thu, 13 Aug 2026 03:45:42 +0000 Subject: [PATCH] Add structured preview exception hierarchy PreviewError subclasses carry a concise short label for UI, a full log message, backend name, and metadata fields (OnlyOffice code/status/url, backend stage, timeout seconds, cancel reason). Exceptions travel across the worker pool wire pickled in the binary response payload (ok=False) and are re-raised with their original type on the caller side. Combined pipelines such as pdf+pyvips tag the failing stage. --- mediapreview/__init__.py | 2 + mediapreview/__main__.py | 17 ++- mediapreview/backends/__init__.py | 14 ++- mediapreview/backends/pdf.py | 35 +++--- mediapreview/exceptions.py | 186 ++++++++++++++++++++++++++++++ mediapreview/office.py | 82 ++++++++----- mediapreview/pool.py | 101 ++++++++-------- mediapreview/protocol.py | 2 +- mediapreview/worker.py | 29 +++-- tests/test_office.py | 106 +++++++++++++++++ 10 files changed, 455 insertions(+), 119 deletions(-) create mode 100644 mediapreview/exceptions.py create mode 100644 tests/test_office.py diff --git a/mediapreview/__init__.py b/mediapreview/__init__.py index 62dabd0..98625b5 100644 --- a/mediapreview/__init__.py +++ b/mediapreview/__init__.py @@ -17,12 +17,14 @@ from mediapreview.backends import ( process_video, ) from mediapreview.cache import CachedPreview, PreviewCache +from mediapreview.exceptions import PreviewError from mediapreview.formats import is_previewable_path from mediapreview.protocol import PreviewRequest, PreviewResponse __all__ = [ "CachedPreview", "PreviewCache", + "PreviewError", "PreviewRequest", "PreviewResponse", "dispatch", diff --git a/mediapreview/__main__.py b/mediapreview/__main__.py index fd17158..12d7f3f 100644 --- a/mediapreview/__main__.py +++ b/mediapreview/__main__.py @@ -31,6 +31,7 @@ from pathlib import Path from docopt import docopt from mediapreview.backends import dispatch +from mediapreview.exceptions import PreviewError from mediapreview.util.logformat import EmojiFormatter @@ -66,12 +67,16 @@ def _preview(args: dict) -> None: sys.stderr.write(f"error: no such file: {path}\n") sys.exit(2) - result, resp = dispatch( - path, - quality=int(args["-q"]), - maxsize=int(args["--maxsize"]), - maxzoom=float(args["--maxzoom"]), - ) + try: + result, resp = dispatch( + path, + quality=int(args["-q"]), + maxsize=int(args["--maxsize"]), + maxzoom=float(args["--maxzoom"]), + ) + except PreviewError as e: + sys.stderr.write(f"error: {e}\n") + sys.exit(1) if not resp.ok or result is None: sys.stderr.write(f"error: {resp.error or 'preview failed'}\n") if resp.stderr: diff --git a/mediapreview/backends/__init__.py b/mediapreview/backends/__init__.py index 076b1b2..9b72834 100644 --- a/mediapreview/backends/__init__.py +++ b/mediapreview/backends/__init__.py @@ -15,8 +15,8 @@ from mediapreview.backends.image import ( ) from mediapreview.backends.pdf import process_pdf from mediapreview.backends.video import process_video +from mediapreview.exceptions import PreviewError, backend_error from mediapreview.formats import DOC_PREVIEW_SUFFIXES -from mediapreview.protocol import PreviewResponse __all__ = [ "dispatch", @@ -49,13 +49,17 @@ def dispatch(path, quality, maxsize, maxzoom, data=None): if mime_type and mime_type.startswith("image/"): backend = "pyvips" return process_image(path, quality=quality, maxsize=maxsize) + except PreviewError: + # Already structured (e.g. a stage of a combined pipeline like + # pdf+pyvips) — keep the original backend/stage identity. + raise except ValueError as e: - return None, PreviewResponse(ok=False, backend=backend, error=str(e)) + raise backend_error(backend, str(e)) from e except ImportError as e: # Missing optional extra — expected, so a plain message, no traceback. logger.error("Preview dispatch failed for %s: %s", path, e) # noqa: TRY400 - return None, PreviewResponse(ok=False, backend=backend, error=str(e)) + raise backend_error(backend, str(e)) from e except Exception as e: logger.exception("Preview dispatch failed for %s", path) - return None, PreviewResponse(ok=False, backend=backend, error=str(e)) - return None, PreviewResponse(ok=False, backend=backend, error="preview unsupported") + raise backend_error(backend, str(e)) from e + raise backend_error(backend, "preview unsupported") diff --git a/mediapreview/backends/pdf.py b/mediapreview/backends/pdf.py index 85300e3..87f037b 100644 --- a/mediapreview/backends/pdf.py +++ b/mediapreview/backends/pdf.py @@ -5,6 +5,7 @@ from time import perf_counter import pyvips from mediapreview.backends.image import AVIF_FAST_EFFORT +from mediapreview.exceptions import backend_error from mediapreview.protocol import PreviewResponse try: @@ -12,6 +13,8 @@ try: except ImportError: # pragma: no cover - optional pdf extra pymupdf = None +BACKEND = "pdf+pyvips" + def process_pdf(path, *, maxsize, maxzoom, quality, page_number=0): if pymupdf is None: @@ -19,26 +22,30 @@ def process_pdf(path, *, maxsize, maxzoom, quality, page_number=0): "PDF previews require the 'pdf' extra: pip install mediapreview[pdf]" ) t_load_start = perf_counter() - with pymupdf.open(path) as pdf: - page = pdf.load_page(page_number) - w, h = page.rect[2:4] - zoom = min(maxsize / w, maxsize / h, maxzoom) - mat = pymupdf.Matrix(zoom, zoom) - pix = page.get_pixmap(matrix=mat) - t_load_end = perf_counter() + try: + with pymupdf.open(path) as pdf: + page = pdf.load_page(page_number) + w, h = page.rect[2:4] + zoom = min(maxsize / w, maxsize / h, maxzoom) + mat = pymupdf.Matrix(zoom, zoom) + pix = page.get_pixmap(matrix=mat) + samples, width, height, n = pix.samples_mv, pix.width, pix.height, pix.n + except Exception as e: + raise backend_error(BACKEND, str(e), stage="pdf") from e + t_load_end = perf_counter() - t_save_start = perf_counter() - img = pyvips.Image.new_from_memory( - pix.samples_mv, pix.width, pix.height, pix.n, "uchar" - ) - ret = img.write_to_buffer(".avif", Q=quality, effort=AVIF_FAST_EFFORT, keep="none") - backend = "pdf+pyvips" + t_save_start = perf_counter() + try: + img = pyvips.Image.new_from_memory(samples, width, height, n, "uchar") + ret = img.write_to_buffer(".avif", Q=quality, effort=AVIF_FAST_EFFORT, keep="none") + except Exception as e: + raise backend_error(BACKEND, str(e), stage="pyvips") from e t_save_end = perf_counter() return ret, PreviewResponse( ok=True, mime="image/avif", - backend=backend, + backend=BACKEND, timings=[ round((t_load_end - t_load_start) * 1000, 1), round((t_save_end - t_save_start) * 1000, 1), diff --git a/mediapreview/exceptions.py b/mediapreview/exceptions.py new file mode 100644 index 0000000..e343733 --- /dev/null +++ b/mediapreview/exceptions.py @@ -0,0 +1,186 @@ +"""Structured preview exceptions. + +Preview failures are represented by ``PreviewError`` and a small number of +subclasses, carrying their fields directly: ``short`` (concise, for UI with +limited space — the backend name is usually printed in front of it, so it is +left out), the exception message itself (for logs), ``backend``, and +optional subclass-specific metadata such as ``code`` or ``timeout_seconds`` +for callers that wish to do their own processing. + +The worker pool ships exceptions between processes with pickle (same +trust domain — the pool unpickles only data from its own workers), so any +exception arrives intact on the caller side, no per-class serialization +machinery needed. Because the initializers are keyword-heavy, pickling is +routed through ``__dict__`` via ``PreviewError.__reduce__``. + +The hierarchy is intentionally small: + +- ``OnlyOfficeError`` covers all OnlyOffice failures; optional fields + (``code``, ``status``, ``url``, ``snippet``) describe the specific failure. +- ``PreviewBackendError`` covers backend conversion failures (ffmpeg, pyvips, + pdf, etc.); ``stage`` identifies the failing step of a combined pipeline + (e.g. "pdf" vs "pyvips" in the "pdf+pyvips" backend). +- ``PreviewTimeoutError`` covers timeouts for any backend. +- ``PreviewCancelledError`` covers cancellations (e.g. pool shutdown). + +BaseExceptions such as ``KeyboardInterrupt``, ``SystemExit`` and +``asyncio.CancelledError`` are never wrapped in these types. +""" + +from __future__ import annotations + + +class PreviewError(Exception): + """Base preview exception. + + ``short`` is concise text for UIs with limited space (the backend name + is usually printed in front of it); ``str(err)`` is the full + human-readable message; ``backend`` identifies the backend + (e.g. "onlyoffice", "ffmpeg"). + """ + + def __init__( + self, + message: str = "preview failed", + short: str = "error", + *, + backend: str | None = None, + ): + super().__init__(message) + self.short = short + self.backend = backend + + def __reduce__(self): + # Keyword-heavy initializers do not unpickle via args; pass the message + # positionally and restore the rest from __dict__. + return (type(self), (str(self),), self.__dict__) + + +class OnlyOfficeError(PreviewError): + """OnlyOffice conversion failed. Specifics are in the extra fields.""" + + def __init__( # noqa: PLR0913 - metadata fields are independent + self, + message: str = "OnlyOffice conversion failed", + short: str = "error", + *, + code: str | None = None, + status: int | None = None, + url: str | None = None, + snippet: str | None = None, + backend: str | None = "onlyoffice", + ): + super().__init__(message, short, backend=backend) + self.code = code + self.status = status + self.url = url + self.snippet = snippet + + +class PreviewBackendError(PreviewError): + """Backend conversion failure (image/video/pdf/etc).""" + + def __init__( + self, + message: str = "preview failed", + short: str = "error", + *, + stage: str | None = None, + backend: str | None = None, + ): + super().__init__(message, short, backend=backend) + self.stage = stage + + +class PreviewTimeoutError(PreviewError): + """Preview conversion exceeded its timeout for a given backend.""" + + def __init__( + self, + message: str = "preview timed out", + short: str = "timeout", + *, + timeout_seconds: float = 0.0, + backend: str | None = None, + ): + super().__init__(message, short, backend=backend) + self.timeout_seconds = timeout_seconds + + +class PreviewCancelledError(PreviewError): + """Preview was cancelled (e.g. pool shut down).""" + + def __init__( + self, + message: str = "Preview cancelled (pool closed)", + short: str = "cancelled", + *, + reason: str = "pool closed", + backend: str | None = None, + ): + super().__init__(message, short, backend=backend) + self.reason = reason + + +# --------------------------------------------------------------------------- +# Factory helpers +# --------------------------------------------------------------------------- + +_OO_CODE_ERRORS = { + "-8": ("jwt error", "OnlyOffice JWT authentication failed"), + "-4": ("input error", "OnlyOffice input error"), + "-2": ("timeout error", "OnlyOffice conversion timed out"), + "-1": ("unknown error", "OnlyOffice conversion failed with unknown error"), +} + + +def onlyoffice_error_from_code(code: str | None = None) -> OnlyOfficeError: + """Build an OnlyOfficeError from a conversion status code (e.g. "-8").""" + if code in _OO_CODE_ERRORS: + short, log = _OO_CODE_ERRORS[code] + elif code: + short, log = f"{code} error", f"OnlyOffice conversion failed: {code}" + else: + short, log = "unknown error", "OnlyOffice conversion failed with unknown error" + return OnlyOfficeError(log, short, code=code) + + +def onlyoffice_unavailable_error(url: str | None = None) -> OnlyOfficeError: + log = "OnlyOffice document server not reachable" + if url: + log = f"{log} at {url}" + return OnlyOfficeError(log, "unavailable", url=url) + + +def onlyoffice_http_error(status: int) -> OnlyOfficeError: + return OnlyOfficeError(f"OnlyOffice HTTP error: {status}", "http error", status=status) + + +def onlyoffice_no_fileurl_error(snippet: str | None = None) -> OnlyOfficeError: + log = "OnlyOffice response did not contain FileUrl" + if snippet: + log = f"{log}: {snippet}" + return OnlyOfficeError(log, "no-fileurl error", snippet=snippet) + + +def backend_error(backend: str, message: str, *, stage: str | None = None) -> PreviewBackendError: + short = message.splitlines()[0][:60] + return PreviewBackendError( + f"[{backend}] preview failed: {message}", + short, + backend=backend, + stage=stage, + ) + + +def preview_timeout_error(backend: str, timeout_seconds: float) -> PreviewTimeoutError: + return PreviewTimeoutError( + f"{backend.capitalize()} preview timed out after {timeout_seconds}s", + "timeout", + backend=backend, + timeout_seconds=timeout_seconds, + ) + + +def preview_cancelled_error(reason: str = "pool closed") -> PreviewCancelledError: + return PreviewCancelledError(f"Preview cancelled ({reason})", "cancelled", reason=reason) diff --git a/mediapreview/office.py b/mediapreview/office.py index 66d8c28..c02ba79 100644 --- a/mediapreview/office.py +++ b/mediapreview/office.py @@ -30,6 +30,14 @@ from pathlib import Path from time import perf_counter from urllib.parse import quote +from mediapreview.exceptions import ( + onlyoffice_error_from_code, + onlyoffice_http_error, + onlyoffice_no_fileurl_error, + onlyoffice_unavailable_error, + preview_timeout_error, +) + try: import httpx import jwt @@ -45,6 +53,7 @@ logger = logging.getLogger(__name__) _httpx_client: httpx.AsyncClient | None = None +_httpx_client_loop: asyncio.AbstractEventLoop | None = None def _get_onlyoffice_url() -> str: @@ -82,35 +91,27 @@ def _get_callback_host() -> str: return "127.0.0.1" -def onlyoffice_error_short_text(detail: str) -> str: - """Short human-readable label for an OnlyOffice failure, for log annotation.""" - if detail.startswith("OnlyOffice conversion error:"): - code = detail.rsplit(":", 1)[-1].strip() - return { - "-8": "onlyoffice jwt error", - "-4": "onlyoffice input error", - "-2": "onlyoffice timeout error", - "-1": "onlyoffice unknown error", - }.get(code, f"onlyoffice {code} error") - if "OnlyOffice response did not contain FileUrl" in detail: - return "onlyoffice no-fileurl error" - return "onlyoffice error" - - +# --------------------------------------------------------------------------- # --------------------------------------------------------------------------- # Async HTTP client # --------------------------------------------------------------------------- def get_httpx_client() -> httpx.AsyncClient: - """Return the shared async HTTP client for OnlyOffice requests.""" + """Return the shared async HTTP client for OnlyOffice requests. + + The client is recreated if the running event loop changes, because an + ``httpx.AsyncClient`` is bound to the loop that created it. + """ if httpx is None: raise ImportError( "OnlyOffice integration requires the 'office' extra: pip install mediapreview[office]" ) - global _httpx_client - if _httpx_client is None: + global _httpx_client, _httpx_client_loop + current_loop = asyncio.get_running_loop() + if _httpx_client is None or _httpx_client_loop is not current_loop: _httpx_client = httpx.AsyncClient() + _httpx_client_loop = current_loop return _httpx_client @@ -330,26 +331,34 @@ async def convert_to_png_async(file_path: Path, request_timeout: float = 5.0) -> headers["Authorization"] = token t_start = perf_counter() - response = await client.post( - convert_url, - content=json.dumps(payload).encode(), - headers=headers, - timeout=request_timeout, - ) - response.raise_for_status() + try: + response = await client.post( + convert_url, + content=json.dumps(payload).encode(), + headers=headers, + timeout=request_timeout, + ) + response.raise_for_status() + except httpx.TimeoutException as e: + raise preview_timeout_error("onlyoffice", request_timeout) from e + except httpx.HTTPStatusError as e: + raise onlyoffice_http_error(e.response.status_code) from e + except httpx.RequestError as e: + raise onlyoffice_unavailable_error(_get_onlyoffice_url()) from e body = response.content t_end = perf_counter() # Parse XML response text = body.decode("utf-8", errors="replace") if "" in text: - code = "unknown" - if "" in text and "" in text: + code = None + if "" in text: code = text.split("")[1].split("")[0] - raise RuntimeError(f"OnlyOffice conversion error: {code}") + raise onlyoffice_error_from_code(code) if "" not in text: - raise RuntimeError("OnlyOffice response did not contain FileUrl") + snippet = text if len(text) <= 200 else text[:200] + "..." + raise onlyoffice_no_fileurl_error(snippet) file_url = text.split("")[1].split("")[0] file_url = file_url.replace("&", "&") @@ -357,8 +366,17 @@ async def convert_to_png_async(file_path: Path, request_timeout: float = 5.0) -> logger.debug("OnlyOffice converted in %.2fs: %s", t_end - t_start, file_url) # Download converted PNG - png_response = await client.get(file_url, timeout=request_timeout) - png_response.raise_for_status() + try: + png_response = await client.get(file_url, timeout=request_timeout) + png_response.raise_for_status() + except httpx.TimeoutException as e: + raise preview_timeout_error("onlyoffice", request_timeout) from e + except httpx.HTTPStatusError as e: + raise onlyoffice_http_error(e.response.status_code) from e + except httpx.RequestError as e: + # The converted file lives on the OO server, so a request failure here + # usually means OO itself could not be reached after conversion. + raise onlyoffice_unavailable_error(_get_onlyoffice_url()) from e return png_response.content finally: await asyncio.to_thread(httpd.shutdown) @@ -385,7 +403,7 @@ class OOConversionManager: async def convert(self, filepath: Path) -> bytes: """Return PNG bytes for *filepath*, deduplicating concurrent requests.""" if not await is_available_cached(): - raise RuntimeError("OnlyOffice server not reachable") + raise onlyoffice_unavailable_error(_get_onlyoffice_url()) stat = await asyncio.to_thread(filepath.stat) key = f"{filepath}:{stat.st_mtime_ns}" diff --git a/mediapreview/pool.py b/mediapreview/pool.py index bbe96f1..0bcfe70 100644 --- a/mediapreview/pool.py +++ b/mediapreview/pool.py @@ -4,6 +4,7 @@ import asyncio import contextlib import logging import os +import pickle import signal import struct import sys @@ -20,6 +21,13 @@ except ImportError as e: # pragma: no cover - optional worker extra "The worker pool requires the 'worker' extra: pip install mediapreview[worker]" ) from e +from mediapreview.exceptions import ( + PreviewError, + PreviewTimeoutError, + backend_error, + preview_cancelled_error, + preview_timeout_error, +) from mediapreview.formats import ( expected_backend as _expected_preview_backend, ) @@ -35,7 +43,6 @@ from mediapreview.protocol import PreviewRequest, PreviewResponse __all__ = [ "PREVIEW_TIMEOUT", "PreviewError", - "PreviewPoolClosedError", "PreviewTimeoutError", "generate_office_preview", "is_previewable_path", @@ -47,33 +54,6 @@ __all__ = [ logger = logging.getLogger(__name__) -class PreviewTimeoutError(Exception): - """Raised when the preview subprocess exceeds PREVIEW_TIMEOUT.""" - - def __init__(self, message: str, *, backend: str | None = None): - super().__init__(message) - self.backend = backend - - -class PreviewError(Exception): - """Raised when the preview subprocess exits with a non-zero status.""" - - def __init__( - self, - message: str, - *, - stderr: str | None = None, - backend: str | None = None, - ): - super().__init__(message) - self.stderr = stderr - self.backend = backend - - -class PreviewPoolClosedError(PreviewError): - """The preview worker pool has been shut down.""" - - class WorkerChecksumError(Exception): """Raised when worker response checksum does not match the packet.""" @@ -82,6 +62,20 @@ class WorkerProtocolError(Exception): """Raised when worker response packet is malformed.""" +def _reraise_worker_error(resp: PreviewResponse, payload: bytes) -> None: + """Re-raise the exception the worker sent back in the payload, if any. + + The payload is pickled by our own worker processes (same trust domain), so + the original exception type arrives intact on the caller side. Falls back + to a plain PreviewBackendError built from the error message. + """ + if payload: + exc = pickle.loads(payload) # noqa: S301 - trusted: our own workers + if isinstance(exc, Exception): + raise exc + raise backend_error(resp.backend or "unknown", resp.error or "preview worker error") + + PREVIEW_TIMEOUT = 10.0 # seconds until preview subprocess is killed PREVIEW_WORKERS = max(2, min(8, cpu_count())) WORKER_KILL_GRACE = 5.0 # max seconds to wait for a killed worker to be reaped @@ -139,11 +133,7 @@ class _PreviewWorker: resp = msgspec.json.decode(meta_raw, type=PreviewResponse) if not resp.ok: - raise PreviewError( - resp.error or "preview worker error", - stderr=resp.stderr, - backend=resp.backend, - ) + _reraise_worker_error(resp, payload) return payload or None, resp async def kill(self) -> None: @@ -279,9 +269,9 @@ class _PreviewWorkerPool: ) if not future.done(): future.set_exception( - PreviewTimeoutError( - args[0].name, - backend=_expected_preview_backend(args[0]), + preview_timeout_error( + _expected_preview_backend(args[0]), + PREVIEW_TIMEOUT, ) ) return @@ -305,9 +295,9 @@ class _PreviewWorkerPool: ) if not future.done(): future.set_exception( - PreviewTimeoutError( - filepath.name, - backend=_expected_preview_backend(filepath), + preview_timeout_error( + _expected_preview_backend(filepath), + PREVIEW_TIMEOUT, ) ) except WorkerChecksumError: @@ -319,7 +309,10 @@ class _PreviewWorkerPool: ) if not future.done(): future.set_exception( - PreviewError(f"worker checksum mismatch for {filepath.name}") + backend_error( + _expected_preview_backend(filepath), + f"worker checksum mismatch for {filepath.name}", + ) ) except PreviewError as e: if not future.done(): @@ -342,14 +335,20 @@ class _PreviewWorkerPool: ) if not future.done(): future.set_exception( - PreviewError(f"worker protocol failure for {filepath.name}: {e}") + backend_error( + _expected_preview_backend(filepath), + f"worker protocol failure for {filepath.name}: {e}", + ) ) except Exception: replace = True logger.exception("Unexpected preview worker error for %s", filepath.name) if not future.done(): future.set_exception( - PreviewError(f"unexpected worker error for {filepath.name}") + backend_error( + _expected_preview_backend(filepath), + f"unexpected worker error for {filepath.name}", + ) ) finally: if replace: @@ -378,7 +377,7 @@ class _PreviewWorkerPool: data: bytes | None = None, ): if self._closed: - raise PreviewPoolClosedError("preview worker pool closed") + raise preview_cancelled_error("preview worker pool closed") loop = asyncio.get_running_loop() future = loop.create_future() self._in_flight.add(future) @@ -410,18 +409,14 @@ class _PreviewWorkerPool: # out their timeouts during server shutdown. for future in list(self._in_flight): if not future.done(): - future.set_exception( - PreviewPoolClosedError("preview worker pool closed") - ) + future.set_exception(preview_cancelled_error("pool closed")) while not self._pending.empty(): try: _priority, _seq, future, _args = self._pending.get_nowait() except asyncio.QueueEmpty: break if not future.done(): - future.set_exception( - PreviewPoolClosedError("preview worker pool closed") - ) + future.set_exception(preview_cancelled_error("pool closed")) while not self._idle.empty(): try: self._idle.get_nowait() @@ -469,7 +464,11 @@ async def shutdown_preview_workers() -> None: async def generate_office_preview( filepath: Path, quality: int, maxsize: int, maxzoom: float ) -> tuple[bytes | None, PreviewResponse | None]: - """Generate a preview for an office file using OnlyOffice + worker AVIF conversion.""" + """Generate a preview for an office file using OnlyOffice + worker AVIF conversion. + + Raises: + OnlyOfficeError: If the OnlyOffice Document Server cannot convert the file. + """ manager = get_oo_manager() t_oo_start = perf_counter() png_bytes = await manager.convert(filepath) @@ -490,5 +489,5 @@ async def run_preview( """Run preview request in a persistent worker process.""" await start_preview_workers() if _preview_pool is None: - raise PreviewPoolClosedError("preview worker pool closed") + raise preview_cancelled_error("preview worker pool closed") return await _preview_pool.run(filepath, quality, maxsize, maxzoom, data) diff --git a/mediapreview/protocol.py b/mediapreview/protocol.py index f4ae49f..dbaa433 100644 --- a/mediapreview/protocol.py +++ b/mediapreview/protocol.py @@ -11,7 +11,7 @@ class PreviewRequest(msgspec.Struct, omit_defaults=True): class PreviewResponse(msgspec.Struct, omit_defaults=True): - ok: bool + ok: bool # Indicates whether binary payload is the file or a pickled exception mime: str | None = None backend: str | None = None timings: list[float] | None = None diff --git a/mediapreview/worker.py b/mediapreview/worker.py index 27945e1..9462e32 100644 --- a/mediapreview/worker.py +++ b/mediapreview/worker.py @@ -19,6 +19,7 @@ import contextlib import io import logging import os +import pickle import signal import struct import sys @@ -43,6 +44,18 @@ from mediapreview.util.logformat import format_level_prefix logger = logging.getLogger(__name__) +def _serialize_exception(e: BaseException) -> bytes: + """Pickle an exception for re-raising across the pool wire. + + Falls back to an empty payload (caller uses the plain error message) in + the unlikely case the exception cannot be pickled. + """ + try: + return pickle.dumps(e) + except Exception: + return b"" + + class _WorkerLogFormatter(logging.Formatter): """Emoji level prefix like the main process, tagged with the worker pid.""" @@ -125,21 +138,17 @@ def _run_loop() -> None: result, resp = dispatch( Path(req.path), req.quality, req.maxsize, req.maxzoom, data ) - if not resp.ok: - captured = stderr_capture.getvalue().strip() - if captured: - resp = PreviewResponse( - ok=False, - backend=resp.backend, - error=resp.error, - stderr=captured, - ) _write_response(resp, result or b"") except Exception as e: logger.exception("Preview worker error for %s", req.path) captured = stderr_capture.getvalue().strip() _write_response( - PreviewResponse(ok=False, error=str(e), stderr=captured or None), b"" + PreviewResponse( + ok=False, + error=str(e), + stderr=captured or None, + ), + _serialize_exception(e), ) finally: root_logger.removeHandler(handler) diff --git a/tests/test_office.py b/tests/test_office.py new file mode 100644 index 0000000..27bde4a --- /dev/null +++ b/tests/test_office.py @@ -0,0 +1,106 @@ +"""Tests for OnlyOffice error handling and structured responses.""" + +from __future__ import annotations + +import pickle +from pathlib import Path + +import pytest + +from mediapreview import office +from mediapreview.exceptions import ( + OnlyOfficeError, + PreviewBackendError, + backend_error, + onlyoffice_error_from_code, + onlyoffice_http_error, + onlyoffice_no_fileurl_error, + onlyoffice_unavailable_error, +) +from mediapreview.pool import generate_office_preview + +FILES = Path(__file__).resolve().parent / "files" + + +@pytest.mark.parametrize( + ("code", "short", "message"), + [ + ("-8", "jwt error", "OnlyOffice JWT authentication failed"), + ("-4", "input error", "OnlyOffice input error"), + ("-2", "timeout error", "OnlyOffice conversion timed out"), + ("-1", "unknown error", "OnlyOffice conversion failed with unknown error"), + ("-99", "-99 error", "OnlyOffice conversion failed: -99"), + (None, "unknown error", "OnlyOffice conversion failed with unknown error"), + ], +) +def test_onlyoffice_error_from_code(code, short, message): + """Conversion status codes map to clean labels and messages.""" + err = onlyoffice_error_from_code(code) + assert str(err) == message + assert err.short == short + assert err.code == code + assert err.backend == "onlyoffice" + assert isinstance(err, OnlyOfficeError) + + +def test_onlyoffice_http_error(): + err = onlyoffice_http_error(502) + assert str(err) == "OnlyOffice HTTP error: 502" + assert err.status == 502 + assert err.short == "http error" + + +def test_onlyoffice_unavailable_error(): + err = onlyoffice_unavailable_error("http://localhost:8988") + assert "http://localhost:8988" in str(err) + assert err.url == "http://localhost:8988" + assert err.short == "unavailable" + + +def test_onlyoffice_no_fileurl_error(): + err = onlyoffice_no_fileurl_error("") + assert "" in str(err) + assert err.snippet == "" + assert err.short == "no-fileurl error" + + +def test_backend_error_stage(): + """Combined pipelines tag the failing stage.""" + err = backend_error("pdf+pyvips", "cannot read document", stage="pdf") + assert err.backend == "pdf+pyvips" + assert err.stage == "pdf" + assert err.short == "cannot read document" + assert isinstance(err, PreviewBackendError) + + +def test_error_pickle_round_trip(): + """Exceptions survive pickling (the worker pool wire) intact.""" + err = onlyoffice_error_from_code("-8") + restored = pickle.loads(pickle.dumps(err)) + assert type(restored) is type(err) + assert str(restored) == str(err) + assert restored.code == err.code + assert restored.short == err.short + assert restored.backend == err.backend + + +@pytest.mark.asyncio +async def test_generate_office_preview_raises_structured_error(monkeypatch): + """On OnlyOffice failure, generate_office_preview raises OnlyOfficeError.""" + async def fake_convert(_filepath: Path, request_timeout: float = 5.0) -> bytes: + raise onlyoffice_error_from_code("-8") + + monkeypatch.setattr(office, "convert_to_png_async", fake_convert) + + with pytest.raises(OnlyOfficeError) as exc_info: + await generate_office_preview( + FILES / "file-sample_100kB.docx", + quality=60, + maxsize=512, + maxzoom=2.0, + ) + + err = exc_info.value + assert err.code == "-8" + assert err.short == "jwt error" + assert str(err) == "OnlyOffice JWT authentication failed"