From 33406d33d64df177bc2cff43b2a80c9ea56cbff6 Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 6 Oct 2026 19:11:48 -0700 Subject: [PATCH 1/7] Adapt to comicbox 5.3.0: doctor report, scan every suffix, probe() credentials comicbox 5.3.0 ships a doctor (run_checks) that reports whether the host can read each archive format, has the image codecs cover matching needs, and whether its config and package pins are in order, with a fix per problem. Codex now runs it: problems are logged once at startup after loguru is up, GET /api/v4/admin/doctor serves the rows, and the admin Jobs tab ends with a Doctor section with a Re-check button. The Online section is left out because codex keeps credentials in its own database and checks them on the Tagging tab. The Docker final stage runs `comicbox doctor -q` as its smoke test, which proves unrar extracts, pymupdf loads and every pin is satisfied. The scanner no longer drops .cbr or .pdf when comicbox says the tool is missing. The poller's database snapshot lists every row, so an excluded suffix made every existing CBR or PDF row look deleted the day unrar broke, and 5.3.0's probe test-extracts a member, which fails on more hosts than the path check it replaced. Suffixes derive from FileTypeEnum, shared with the janitor; an unreadable archive fails its import with comicbox's reason. The credential validator uses OnlineSource.probe() instead of hand-rolled mokkari and simyan calls: Metron's goes through comicbox's rate gate and Comic Vine's counts against the real shared bucket. Adopting it exposed that _build_credentials passed an unset Metron key as "", which mokkari turns into a blank Bearer header that replaces the username and password, so legacy logins failed on every real run while the old validator passed them. Unset fields are now None. Also: the chosen-match replay passes codex's online config so it reads the cache the search filled; the reader imports pdffile lazily behind is_pdf_supported() so a broken pymupdf disables PDFs instead of failing server start, as comicbox does; str() of a StrEnum MatchMode is its value. Co-Authored-By: Claude Fable 5.1 --- Dockerfile | 7 +- NEWS.md | 16 +- README.md | 23 +- codex/doctor.py | 72 +++++ codex/librarian/fs/filters.py | 46 +-- .../onlinetag/credential_validator.py | 217 +++++++------- codex/librarian/onlinetag/prompt_apply.py | 5 + codex/librarian/onlinetag/session_manager.py | 14 +- codex/librarian/onlinetag/session_state.py | 2 +- .../scribe/janitor/integrity/foreign_keys.py | 21 +- codex/serializers/admin/doctor.py | 29 ++ codex/startup/__init__.py | 2 + codex/urls/api/v4/admin.py | 6 + codex/views/admin/doctor.py | 29 ++ codex/views/reader/page.py | 14 +- frontend/src/api/v4/admin.js | 4 + .../components/admin/tabs/doctor-panel.vue | 206 +++++++++++++ .../src/components/admin/tabs/job-tab.vue | 3 + frontend/src/stores/admin.js | 10 + frontend/tests/unit/doctor-panel.test.js | 198 +++++++++++++ tests/test_admin_doctor_endpoint.py | 91 ++++++ tests/test_credential_validator.py | 276 +++++++++++------- tests/test_dockerfile.py | 8 + tests/test_doctor.py | 104 +++++++ tests/test_fs_filters_comic_suffixes.py | 45 +++ tests/test_janitor_folder_relations.py | 6 +- tests/test_onlinetag_credentials.py | 4 + tests/test_onlinetag_prompt_resolution.py | 25 ++ 28 files changed, 1202 insertions(+), 281 deletions(-) create mode 100644 codex/doctor.py create mode 100644 codex/serializers/admin/doctor.py create mode 100644 codex/views/admin/doctor.py create mode 100644 frontend/src/components/admin/tabs/doctor-panel.vue create mode 100644 frontend/tests/unit/doctor-panel.test.js create mode 100644 tests/test_admin_doctor_endpoint.py create mode 100644 tests/test_doctor.py create mode 100644 tests/test_fs_filters_comic_suffixes.py diff --git a/Dockerfile b/Dockerfile index 071b1782d..351c07927 100644 --- a/Dockerfile +++ b/Dockerfile @@ -134,8 +134,11 @@ RUN mkdir -p /home/abc/.config/comicbox \ COPY --from=wheel-installer /opt/codex /opt/codex ENV PATH="/opt/codex/bin:${PATH}" -# Fail the build, not the container start, on a broken venv. -RUN python -B -c "import django, comicbox, pymupdf, PIL.Image, cryptography, granian, rapidfuzz" +# Fail the build, not the container start, on a broken venv. comicbox's +# doctor proves unrar extracts a RAR member, pymupdf loads and every pin is +# satisfied; HOME keeps the config dir it creates out of the image. +RUN python -B -c "import django, cryptography, granian, rapidfuzz" \ + && HOME=/tmp comicbox doctor -q VOLUME /comics VOLUME /config diff --git a/NEWS.md b/NEWS.md index 2acb1b6a0..e1460037c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,10 +8,24 @@ border-radius: 128px; ## v2.5.3 -- Fixes +- Features + - Doctor report on the admin Jobs tab: archive tools, PDF support, image + codecs, config and package problems, each with its fix. Problems are also + logged at startup. +- Fixes + - A missing or broken unrar no longer hides CBR comics from scans or deletes + them from the library; they are listed as failed imports with the cause. + - A broken PDF library disables PDF reading instead of stopping the server. + - Metron logins by username and password tag again; a blank API key no + longer overrides them. + - Credential checks no longer compete with a running tagging job for + Metron's rate limit. + - Re-applying a chosen online match reuses the cached search. - Failed imports no longer vanish from the admin list after other imports. - Size searches accept kb, mb, gb and tb units and the >= and <= operators. - Codex shuts down cleanly on Python 3.12 and 3.13. +- Performance + - Docker image is about 165 MB smaller. ## v2.5.2 diff --git a/README.md b/README.md index 4b186ee65..714be5dc4 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,8 @@ apk add bsd-compat-headers build-base jpeg-dev libffi-dev libwebp openssl-dev sq ##### Install unrar Runtime Dependency on non-debian Linux -Codex requires unrar to read CBR formatted comic archives. Unrar is often not +Codex requires [RARLAB's unrar](https://www.rarlab.com) to read CBR comic +archives; the bsdtar and 7-Zip fallbacks cannot extract them. Unrar is often not packaged for Linux, but here are some instructions: [How to install unrar in Linux](https://www.unixtutorial.org/how-to-install-unrar-in-linux/) @@ -1106,6 +1107,26 @@ codex like: LOGLEVEL=DEBUG codex ``` +### 🩺 Doctor + +The Admin panel's Jobs tab ends with a Doctor report: whether this install can +read each comic archive format, has the image codecs cover matching needs, and +whether comicbox's config and package versions are in order, with a fix for each +problem. Codex logs the problems when it starts, too. + +The same report runs from the command line, which helps when Codex will not +start. In Codex's Python environment: + +```sh +comicbox doctor +``` + +In Docker: + +```sh +docker exec codex comicbox doctor +``` + ### Watching Filesystem Events with Docker Codex tries to watch for filesystem events to instantly update your comic diff --git a/codex/doctor.py b/codex/doctor.py new file mode 100644 index 000000000..cbbacf093 --- /dev/null +++ b/codex/doctor.py @@ -0,0 +1,72 @@ +""" +comicbox's doctor, run for this install. + +comicbox 5.3.0's ``run_checks`` reports whether this host can read each +archive format, whether Pillow has the codecs cover matching needs, whether +the user's comicbox config parses, and whether comicbox's package pins are +satisfied, with a one-line fix for anything that is not. Codex shows the +rows to admins and logs the problems once at startup, so a missing unrar or +a pymupdf built against the wrong libmupdf is explained in one place rather +than discovered as failed imports with no cause. + +The Online section is left out. Codex keeps the tagging credentials in its +own database and checks them on the Tagging tab, so the doctor's "no +credentials" rows and its ``--online`` advice would describe the CLI, not +this server. + +comicbox caches its tool probes for the life of the process, so a tool +installed while codex runs shows up after a restart. +""" + +from __future__ import annotations + +from dataclasses import replace +from typing import TYPE_CHECKING + +from comicbox.doctor import Status, run_checks +from comicbox.doctor.online import SECTION as _ONLINE_SECTION + +if TYPE_CHECKING: + from comicbox.doctor import CheckResult, DoctorReport + from loguru import Logger + + +def run_doctor() -> DoctorReport: + """Run comicbox's doctor and keep the rows that describe this server.""" + report = run_checks() + results = tuple(row for row in report.results if row.section != _ONLINE_SECTION) + return replace(report, results=results) + + +def problem_count(report: DoctorReport) -> int: + """Count the rows that say something comicbox needs is broken.""" + return sum(row.status.is_failure for row in report.results) + + +def _describe(row: CheckResult) -> str: + """One log line for a row: what, how bad, why, and the fix if there is one.""" + line = f"comicbox doctor: {row.name} {row.status}: {row.detail}" + if row.fix: + line += f" Fix: {row.fix}" + return line + + +def log_doctor_problems(log: Logger) -> None: + """ + Log what the doctor found, once, after logging is configured. + + Failures warn and warnings inform. The scanner used to warn about an + unreadable archive format at import time, before any log sink existed, + so the message only ever reached stderr. + """ + report = run_doctor() + for row in report.results: + if row.status.is_failure: + log.warning(_describe(row)) + elif row.status is Status.WARN: + log.info(_describe(row)) + if problems := problem_count(report): + noun = "problem" if problems == 1 else "problems" + log.warning(f"comicbox doctor found {problems} {noun}. See the Admin Jobs tab.") + else: + log.debug("comicbox doctor found no problems.") diff --git a/codex/librarian/fs/filters.py b/codex/librarian/fs/filters.py index 4229cb984..bd1e74fbd 100644 --- a/codex/librarian/fs/filters.py +++ b/codex/librarian/fs/filters.py @@ -4,8 +4,7 @@ from contextlib import suppress from pathlib import Path -from comicbox.box import Comicbox -from loguru import logger +from comicbox.enums.comicbox import FileTypeEnum # Component-level ignore registry consulted by the poller's walker and # the watchfiles filter. Either constant can be extended to add new @@ -68,40 +67,25 @@ def is_ignored_path(path: Path | str, root: Path | str | None = None) -> bool: _IMAGE_REGEX = r"\.(jpe?g|webp|png|gif|bmp)" _IMAGE_MATCHER: re.Pattern = re.compile(_IMAGE_REGEX, re.IGNORECASE) - -def _build_comic_matcher() -> re.Pattern: - comic_regex = r"\.(cb[zt7" - unsupported = [] - if Comicbox.is_unrar_supported(): - comic_regex += r"r" - else: - unsupported.append("CBR") - comic_regex += r"]" - - if Comicbox.is_pdf_supported(): - comic_regex += r"|pdf" - else: - unsupported.append("PDF") - comic_regex += ")$" - if unsupported: - un_str = ", ".join(unsupported) - logger.warning(f"Cannot detect or read from {un_str} archives") - return re.compile(comic_regex, re.IGNORECASE) - - -_COMIC_MATCHER: re.Pattern = _build_comic_matcher() - - -def _match_suffix(pattern: re.Pattern, path: Path) -> bool: - """Match suffix with pattern.""" - return bool(path and path.suffix and pattern.match(path.suffix) is not None) +#: Every comic archive suffix, lowercase with its dot, derived from +#: comicbox's file types rather than from what this host can read today. +#: The scan used to leave out CBR and PDF when comicbox said their tool +#: was missing. That made every existing row of that type look deleted +#: to the poller the day unrar went missing, and comicbox 5.3.0's probe, +#: which test-extracts a member, fails on more hosts than the path check +#: it replaced. A comic the host cannot open now fails its import with +#: comicbox's reason instead ("'unrar' not on path"), where the admin +#: can see it, and its row stays. +COMIC_SUFFIXES: frozenset[str] = frozenset( + f".{file_type.value.lower()}" for file_type in FileTypeEnum +) def match_comic(path: Path) -> bool: """Match comic file.""" - return _match_suffix(_COMIC_MATCHER, path) + return bool(path and path.suffix) and path.suffix.lower() in COMIC_SUFFIXES def match_image(path: Path) -> bool: """Match image file.""" - return _match_suffix(_IMAGE_MATCHER, path) + return bool(path and path.suffix and _IMAGE_MATCHER.match(path.suffix) is not None) diff --git a/codex/librarian/onlinetag/credential_validator.py b/codex/librarian/onlinetag/credential_validator.py index 6d4ee5318..588f147c4 100644 --- a/codex/librarian/onlinetag/credential_validator.py +++ b/codex/librarian/onlinetag/credential_validator.py @@ -1,38 +1,51 @@ """ -Validate online-source credentials by making a tiny authenticated call. +Validate online-source credentials with comicbox's own probe. Used by the admin tagging settings page to give operators an immediate yes/no on whether a saved (or in-the-form) credential set actually works against Metron / Comic Vine — so they don't discover problems only when a real tagging run fails halfway through. +Each source's ``probe()`` is its cheapest authenticated request, sent +once on a private client with no response cache and never retried, so +the verdict is the server's rather than a cached or pooled answer. +Metron's goes through comicbox's process-wide rate gate; Comic Vine's +is counted against the real shared rate-limit bucket under codex's +comicbox cache dir — the one tagging runs draw on — and is refused +outright when that pool is already spent. + A successful Metron check also reports the account's live rate limits, -which mokkari 4 reads off the ``X-RateLimit-*`` headers of the -validation response itself. The daily "sustained" limit varies by -Metron OpenCollective donor tier (5,000-25,000/day), so it is only -discoverable this way. +which the probe reads off the ``X-RateLimit-*`` headers of its own +response. The daily "sustained" limit varies by Metron OpenCollective +donor tier (5,000-25,000/day), so it is only discoverable this way. """ from __future__ import annotations from dataclasses import dataclass -from pathlib import Path -from tempfile import TemporaryDirectory -from typing import TYPE_CHECKING, Any +from types import MappingProxyType +from typing import TYPE_CHECKING, Any, Final +from comicbox.config.online.settings import OnlineSourceCredentials from comicbox.formats.base.online import SOURCE_NAMES +from comicbox.formats.base.online.retry import RetryCategory +from comicbox.formats.comicvine_api.online_source import ComicVineOnlineSource +from comicbox.formats.metron_api.online_source import MetronOnlineSource + +from codex.settings import COMICBOX_ONLINE_CONFIG if TYPE_CHECKING: - from collections.abc import Collection + from collections.abc import Callable, Collection, Mapping + from comicbox.formats.base.online.sources.base import OnlineSource from comicbox.online_session import OnlineCredentials - from mokkari.session import RateLimitStatus # comicbox owns the canonical online-tag source names (metron, comicvine); derive # from it so codex tracks a new source instead of hand-syncing a literal. KNOWN_SOURCES: frozenset[str] = frozenset(SOURCE_NAMES) -_COMICVINE_TIMEOUT_SECS: float = 10.0 +#: What a probe returns: ``{window: {"limit", "remaining", "reset_epoch"}}``. +type _Windows = Mapping[str, Mapping[str, Any]] @dataclass(frozen=True, slots=True) @@ -46,11 +59,12 @@ class RateLimitWindowInfo: @dataclass(frozen=True, slots=True) class RateLimitInfo: """ - A source account's live rate limits, mirrored from mokkari's status. + A source account's live rate limits, mirrored from the probe's windows. - ``reset`` datetimes are deliberately dropped: the burst window resets - within a minute (stale before an admin reads it) and the sustained - reset isn't worth datetime plumbing for a one-shot validation chip. + ``reset_epoch`` values are deliberately dropped: the burst window + resets within a minute (stale before an admin reads it) and the + sustained reset isn't worth datetime plumbing for a one-shot + validation chip. """ burst: RateLimitWindowInfo = RateLimitWindowInfo() @@ -66,104 +80,91 @@ class ValidationResult: rate_limits: RateLimitInfo | None = None -def _extract_rate_limits(status: RateLimitStatus) -> RateLimitInfo | None: - """Codex-shaped rate limits, or None when the source reported nothing.""" - burst = status.burst - sustained = status.sustained - if ( - burst.limit is None - and burst.remaining is None - and sustained.limit is None - and sustained.remaining is None - ): - return None - return RateLimitInfo( - burst=RateLimitWindowInfo(limit=burst.limit, remaining=burst.remaining), - sustained=RateLimitWindowInfo( - limit=sustained.limit, remaining=sustained.remaining - ), +def _metron_credentials(creds: OnlineCredentials) -> OnlineSourceCredentials: + # ``or None`` is load bearing: mokkari sends a Bearer header whenever + # api_token is not None, so an empty string would defeat the legacy + # username & password fallback. + return OnlineSourceCredentials( + key=creds.metron_key or None, + user=creds.metron_user or None, + password=creds.metron_password or None, ) -def _validate_metron(creds: OnlineCredentials) -> ValidationResult: - if not (creds.metron_key or (creds.metron_user and creds.metron_password)): - return ValidationResult(ok=False, error="API key required.") - from mokkari.exceptions import ApiError, AuthenticationError - from mokkari.session import Session +def _comicvine_credentials(creds: OnlineCredentials) -> OnlineSourceCredentials: + return OnlineSourceCredentials( + key=creds.comicvine_key or None, url=creds.comicvine_url or None + ) - # ``or None`` is load bearing: mokkari sends a Bearer header whenever - # api_token is not None, so an empty string would defeat the legacy - # username & password fallback. - session = Session( - username=creds.metron_user, - passwd=creds.metron_password, - api_token=creds.metron_key or None, - cache=None, - user_agent="codex-credential-check", + +def _window_info(window: Mapping[str, Any] | None) -> RateLimitWindowInfo: + if not window: + return RateLimitWindowInfo() + return RateLimitWindowInfo( + limit=window.get("limit"), remaining=window.get("remaining") ) - try: - session.publishers_list({"page": 1}) - except AuthenticationError as err: - return ValidationResult(ok=False, error=str(err) or "Authentication failed.") - except ApiError as err: - return ValidationResult(ok=False, error=str(err) or "API error.") - # The successful response carried the account's X-RateLimit-* headers. - else: - return ValidationResult( - ok=True, rate_limits=_extract_rate_limits(session.rate_limit_status) - ) - finally: - # This runs in the web process, once per validation, and the - # session is not shared with anything. Since mokkari 4.8.0 it - # holds a pooled TLS connection until closed. - session.close() - - -def _validate_comicvine(creds: OnlineCredentials) -> ValidationResult: - if not creds.comicvine_key: + + +def _metron_rate_limits(windows: _Windows) -> RateLimitInfo | None: + """Codex-shaped rate limits, or None when the response carried no headers.""" + info = RateLimitInfo( + burst=_window_info(windows.get("burst")), + sustained=_window_info(windows.get("daily")), + ) + return None if info == RateLimitInfo() else info + + +def _no_rate_limits(_windows: _Windows) -> None: + """Comic Vine's pools are hourly; the UI chip speaks per-minute / per-day.""" + + +@dataclass(frozen=True, slots=True) +class _Source: + """How codex drives one comicbox online source for a credential check.""" + + cls: type[OnlineSource] + credentials: Callable[[OnlineCredentials], OnlineSourceCredentials] + rate_limits: Callable[[_Windows], RateLimitInfo | None] + + +_SOURCES: Final[Mapping[str, _Source]] = MappingProxyType( + { + "metron": _Source(MetronOnlineSource, _metron_credentials, _metron_rate_limits), + "comicvine": _Source( + ComicVineOnlineSource, _comicvine_credentials, _no_rate_limits + ), + } +) + +# Shown when the client's exception carries no message of its own. +_FALLBACK_ERRORS: Final[Mapping[RetryCategory | None, str]] = MappingProxyType( + { + RetryCategory.AUTH: "Authentication failed.", + RetryCategory.RATE_LIMIT: "Rate limited.", + } +) + + +def _error_message(source: OnlineSource, exc: Exception) -> str: + """Name what went wrong: the exception's first line, else its category.""" + # simyan raises ``RateLimitError(None)`` for a 429 with no error body, + # and str() of that is the word "None". Some upstream errors carry a + # whole HTML page, of which only the first line is the message. + text = str(exc) if exc.args and exc.args[0] is not None else "" + first_line = text.partition("\n")[0].strip() + category = source.classify_retry_exception(exc) + return first_line or _FALLBACK_ERRORS.get(category, type(exc).__name__) + + +def _validate(spec: _Source, creds: OnlineCredentials) -> ValidationResult: + source = spec.cls(spec.credentials(creds), COMICBOX_ONLINE_CONFIG.online) + if not source.is_configured(): return ValidationResult(ok=False, error="API key required.") - from requests_cache import DO_NOT_CACHE - from simyan.comicvine import Comicvine - from simyan.errors import AuthenticationError, ServiceError - - # A credential check must always hit the network — api_key is - # excluded from simyan's cache key, so a cached response would - # validate any key — so responses are never cached (DO_NOT_CACHE) - # and both sqlite files land in a throwaway dir. - # - # The AuthenticationError below only started firing with simyan 4: - # 3.x returned Comic Vine's error body verbatim, so a rejected key - # came back as an ordinary empty result and validated as good. - with TemporaryDirectory(prefix="codex-credential-check-") as tmp: - tmp_path = Path(tmp) - # dict[str, Any] expansion because DO_NOT_CACHE is an int sentinel - # that simyan's timedelta-typed cache_expiry accepts at runtime. - kwargs: dict[str, Any] = { - "api_key": creds.comicvine_key, - "user_agent": "codex-credential-check", - "timeout": _COMICVINE_TIMEOUT_SECS, - "cache_path": tmp_path / "cache.sqlite", - "cache_expiry": DO_NOT_CACHE, - "ratelimit_path": tmp_path / "ratelimits.sqlite", - } - if creds.comicvine_url: - kwargs["base_url"] = creds.comicvine_url - cv = Comicvine(**kwargs) - try: - cv.list_publishers(params={"limit": "1"}, max_results=1) - except AuthenticationError as err: - return ValidationResult( - ok=False, error=str(err) or "Authentication failed." - ) - except ServiceError as err: - return ValidationResult(ok=False, error=str(err) or "Service error.") - return ValidationResult(ok=True) - - -_VALIDATORS = { - "metron": _validate_metron, - "comicvine": _validate_comicvine, -} + try: + windows = source.probe() + except Exception as exc: + return ValidationResult(ok=False, error=_error_message(source, exc)) + return ValidationResult(ok=True, rate_limits=spec.rate_limits(windows or {})) def validate_credentials( @@ -177,4 +178,4 @@ def validate_credentials( constrained the inputs. """ targets = KNOWN_SOURCES if sources is None else (set(sources) & KNOWN_SOURCES) - return {name: _VALIDATORS[name](creds) for name in sorted(targets)} + return {name: _validate(_SOURCES[name], creds) for name in sorted(targets)} diff --git a/codex/librarian/onlinetag/prompt_apply.py b/codex/librarian/onlinetag/prompt_apply.py index 59cda2f03..6285ad896 100644 --- a/codex/librarian/onlinetag/prompt_apply.py +++ b/codex/librarian/onlinetag/prompt_apply.py @@ -44,6 +44,7 @@ from codex.librarian.scribe.tagwrite_errors import add_tag_write_error from codex.librarian.scribe.tasks import BulkTagWriteTask from codex.models.comic import Comic +from codex.settings import COMICBOX_ONLINE_CONFIG if TYPE_CHECKING: from collections.abc import Callable @@ -319,6 +320,10 @@ def _apply_replayed_search( # PROMPT_VERSION; only the comicbox kwarg moved. match=MatchMode(prompt.get("mode") or "auto"), defer_prompts=True, + # The same settings the search ran with, so the replay reads + # the response cache that search filled under /config rather + # than comicbox's own default cache dir. + config=COMICBOX_ONLINE_CONFIG, client_name=CLIENT_NAME, ) session.preload_resolution( diff --git a/codex/librarian/onlinetag/session_manager.py b/codex/librarian/onlinetag/session_manager.py index b46c40c07..5dcbb952a 100644 --- a/codex/librarian/onlinetag/session_manager.py +++ b/codex/librarian/onlinetag/session_manager.py @@ -217,12 +217,16 @@ def _build_credentials(self) -> OnlineCredentials | None: and not defaults.comicvine_key ): return None + # ``or None``, not ``or ""``: comicbox hands the key to mokkari as its + # api_token, and mokkari sends a Bearer header for any token that is + # not None, an empty one included, in place of the username and + # password. An unset key spelled "" shut off every legacy login. return OnlineCredentials( - metron_key=defaults.metron_key or "", - metron_user=defaults.metron_user or "", - metron_password=defaults.metron_password or "", - comicvine_key=defaults.comicvine_key or "", - comicvine_url=defaults.comicvine_url or "", + metron_key=defaults.metron_key or None, + metron_user=defaults.metron_user or None, + metron_password=defaults.metron_password or None, + comicvine_key=defaults.comicvine_key or None, + comicvine_url=defaults.comicvine_url or None, ) @staticmethod diff --git a/codex/librarian/onlinetag/session_state.py b/codex/librarian/onlinetag/session_state.py index 798ff916d..cd11bd833 100644 --- a/codex/librarian/onlinetag/session_state.py +++ b/codex/librarian/onlinetag/session_state.py @@ -165,7 +165,7 @@ def serialize_prompt( "comics": comics, "source": dp.source, "candidates": [serialize_candidate(c) for c in dp.candidates], - "mode": getattr(dp.match, "value", str(dp.match)), + "mode": str(dp.match), "formats": list(formats), "delete_original": delete_original, "rename": rename, diff --git a/codex/librarian/scribe/janitor/integrity/foreign_keys.py b/codex/librarian/scribe/janitor/integrity/foreign_keys.py index 47f629322..0f23d001d 100644 --- a/codex/librarian/scribe/janitor/integrity/foreign_keys.py +++ b/codex/librarian/scribe/janitor/integrity/foreign_keys.py @@ -6,12 +6,12 @@ from pathlib import Path from typing import TYPE_CHECKING, cast -from comicbox.enums.comicbox import FileTypeEnum from django.apps import apps from django.core.exceptions import FieldDoesNotExist from django.db import DEFAULT_DB_ALIAS, connections, transaction from django.db.models.functions import Now +from codex.librarian.fs.filters import COMIC_SUFFIXES from codex.models.util import get_sort_name if TYPE_CHECKING: @@ -22,25 +22,16 @@ # Comic file extensions we'll consider — phantom directory-as-comic # rows (older bug) are out of scope for parent-folder drift repair. -# -# Derived from comicbox rather than written out, so a new archive format -# cannot leave rows behind here, and compared case-insensitively because -# that is how the scanner matched them on the way in -# (``filters`` compiles its regex with ``re.IGNORECASE`` and the importer -# stores the path verbatim, so ``Foo.CBZ`` is a real, importable comic). -# A case-sensitive test dropped those rows from ``needed`` and pruned the +# The scanner's own list, compared case-insensitively because that is +# how the scanner matched them on the way in (the importer stores the +# path verbatim, so ``Foo.CBZ`` is a real, importable comic). A +# case-sensitive test dropped those rows from ``needed`` and pruned the # folders they live in, cascading the comics and their bookmarks away. -# -# Deliberately *not* ``filters.match_comic``: that consults -# ``Comicbox.is_unrar_supported()`` / ``is_pdf_supported()``, so on a host -# without unrar every ``.cbr`` row would drop out of ``needed`` and be -# pruned — the same bug from the other side. -_COMIC_SUFFIXES = frozenset(f".{file_type.value.lower()}" for file_type in FileTypeEnum) def _is_comic_path(path: str) -> bool: """Whether a stored path is a comic archive, however it is cased.""" - return Path(path).suffix.lower() in _COMIC_SUFFIXES + return Path(path).suffix.lower() in COMIC_SUFFIXES # SQLite's parameter cap is 32766; leave headroom for the rare case diff --git a/codex/serializers/admin/doctor.py b/codex/serializers/admin/doctor.py new file mode 100644 index 000000000..c73e0ba4a --- /dev/null +++ b/codex/serializers/admin/doctor.py @@ -0,0 +1,29 @@ +"""Admin doctor report serializers.""" + +from rest_framework.serializers import ( + CharField, + IntegerField, + ListField, + Serializer, +) + + +class DoctorRowSerializer(Serializer): + """One row of comicbox's doctor report.""" + + section = CharField(read_only=True) + name = CharField(read_only=True) + #: OK, WARN, OFF, MISSING, WRONG VERSION, MISCONFIGURED or ERROR. + status = CharField(read_only=True) + found = CharField(read_only=True) + detail = CharField(read_only=True) + fix = CharField(read_only=True) + + +class DoctorReportSerializer(Serializer): + """comicbox's doctor report for this install.""" + + #: comicbox version, Python, platform, and "Docker" inside a container. + header = ListField(child=CharField(), read_only=True) + results = DoctorRowSerializer(many=True, read_only=True) + problems = IntegerField(read_only=True) diff --git a/codex/startup/__init__.py b/codex/startup/__init__.py index 2ba0dd6bd..2ff3beba7 100644 --- a/codex/startup/__init__.py +++ b/codex/startup/__init__.py @@ -8,6 +8,7 @@ from rest_framework.authtoken.models import Token from codex.choices.admin import AdminFlagChoices +from codex.doctor import log_doctor_problems from codex.librarian.status_controller import STATUS_DEFAULTS from codex.models import ( AdminFlag, @@ -257,4 +258,5 @@ def codex_init() -> bool: if AUTH_REMOTE_USER: logger.info("Remote User authorization enabled.") log_docker_hub_deprecation(logger) + log_doctor_problems(logger) return True diff --git a/codex/urls/api/v4/admin.py b/codex/urls/api/v4/admin.py index 3ff54890e..c902d33be 100644 --- a/codex/urls/api/v4/admin.py +++ b/codex/urls/api/v4/admin.py @@ -14,6 +14,7 @@ AdminCustomCoverRemoveView, AdminCustomCoverUploadView, ) +from codex.views.admin.doctor import AdminDoctorView from codex.views.admin.dump_user_data import AdminDumpUserDataView from codex.views.admin.email import AdminEmailSettingsView, AdminEmailTestSendView from codex.views.admin.failed_imports_seen import AdminFailedImportsSeenView @@ -225,6 +226,11 @@ AdminStatsView.as_view(), name="stats", ), + path( + "doctor", + AdminDoctorView.as_view(), + name="doctor", + ), path( "api-key", AdminAPIKey.as_view(), diff --git a/codex/views/admin/doctor.py b/codex/views/admin/doctor.py new file mode 100644 index 000000000..5b6af9351 --- /dev/null +++ b/codex/views/admin/doctor.py @@ -0,0 +1,29 @@ +"""Admin doctor view.""" + +from adrf.mixins import get_data +from asgiref.sync import sync_to_async +from rest_framework.response import Response + +from codex.doctor import problem_count, run_doctor +from codex.serializers.admin.doctor import DoctorReportSerializer +from codex.views.admin.auth import AsyncAdminGenericAPIView + + +class AdminDoctorView(AsyncAdminGenericAPIView): + """GET: comicbox's doctor report for this install.""" + + serializer_class = DoctorReportSerializer + + async def get(self, *_args, **_kwargs) -> Response: + """Run the doctor and serialize its rows.""" + # About a second: a RAR extraction subprocess, config files and + # package metadata. Off the event loop, and off the one thread + # every sync view shares, since it touches no database. + report = await sync_to_async(run_doctor, thread_sensitive=False)() + obj = { + "header": report.header, + "results": report.results, + "problems": problem_count(report), + } + serializer = self.get_serializer(obj) + return Response(await get_data(serializer)) diff --git a/codex/views/reader/page.py b/codex/views/reader/page.py index 01ef280b1..e13db8ca0 100644 --- a/codex/views/reader/page.py +++ b/codex/views/reader/page.py @@ -5,12 +5,12 @@ import time from typing import TYPE_CHECKING, Final +from comicbox.box import Comicbox from comicbox.exceptions import ComicboxError from django.http import HttpResponse from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema from loguru import logger -from pdffile import PageMode, PDFFile from rest_framework.exceptions import NotFound from codex.librarian.bookmark.tasks import BookmarkUpdateTask @@ -22,7 +22,7 @@ from codex.views.reader._archive_cache import archive_cache, page_acl_cache if TYPE_CHECKING: - from pdffile import PageVerdict + from pdffile import PageVerdict, PDFFile from codex.views.reader._archive_cache import _ArchiveEntry @@ -132,6 +132,16 @@ def _try_pdf_image_serve( ``None`` when the caller should fall back to the legacy single-page-PDF path. """ + if not Comicbox.is_pdf_supported(): + # Since comicbox 5.3.0 a pdffile that is installed but will not + # import (pymupdf against the wrong libmupdf, say) disables PDFs + # instead of breaking comicbox. Importing pdffile at module + # level here undid that: it failed the reader's import and with + # it the server. The legacy path below reports the same comic + # as unreadable, which is the truth. + return None + from pdffile import PageMode, PDFFile + with archive_cache.open_entry(path) as entry: # ``Comicbox._get_archive`` returns the underlying archive # union (zip / rar / 7z / tar / pdf). Caller has gated on diff --git a/frontend/src/api/v4/admin.js b/frontend/src/api/v4/admin.js index 9e51b0647..58e6835e3 100644 --- a/frontend/src/api/v4/admin.js +++ b/frontend/src/api/v4/admin.js @@ -182,6 +182,10 @@ export const getAllLibrarianStatuses = () => export const getStats = () => HTTP.get("/admin/stats", { params: { ts: Date.now() } }); +// comicbox's doctor report: one row per environment check. +export const getDoctorReport = () => + HTTP.get("/admin/doctor", { params: { ts: Date.now() } }); + export const getAPIKey = () => HTTP.get("/admin/api-key", { params: { ts: Date.now() } }); diff --git a/frontend/src/components/admin/tabs/doctor-panel.vue b/frontend/src/components/admin/tabs/doctor-panel.vue new file mode 100644 index 000000000..b6fcc14b6 --- /dev/null +++ b/frontend/src/components/admin/tabs/doctor-panel.vue @@ -0,0 +1,206 @@ + + + + + + diff --git a/frontend/src/components/admin/tabs/job-tab.vue b/frontend/src/components/admin/tabs/job-tab.vue index 77785763a..171732110 100644 --- a/frontend/src/components/admin/tabs/job-tab.vue +++ b/frontend/src/components/admin/tabs/job-tab.vue @@ -234,6 +234,7 @@ + @@ -244,6 +245,7 @@ import { camelCase } from "text-case"; import { ADMIN_JOBS } from "@/choices/admin-jobs.json"; import AdminSection from "@/components/admin/tabs/admin-section.vue"; +import DoctorPanel from "@/components/admin/tabs/doctor-panel.vue"; import AdminExpandToggle from "@/components/admin/tabs/expand-toggle.vue"; import { etaRemaining, @@ -275,6 +277,7 @@ export default { AdminExpandToggle, AdminSection, ConfirmDialog, + DoctorPanel, }, setup() { const { now } = useNowTimer(); diff --git a/frontend/src/stores/admin.js b/frontend/src/stores/admin.js index 28539f1ef..b4b2862b3 100644 --- a/frontend/src/stores/admin.js +++ b/frontend/src/stores/admin.js @@ -87,6 +87,7 @@ export const useAdminStore = defineStore("admin", { }, timestamps: {}, stats: undefined, + doctor: undefined, taggingDefaults: undefined, emailSettings: undefined, oidcSettings: undefined, @@ -298,6 +299,15 @@ export const useAdminStore = defineStore("admin", { console.warn(error); } }, + async loadDoctor() { + if (this._requireAdmin()) return false; + try { + const response = await API.getDoctorReport(); + this.doctor = response.data; + } catch (error) { + console.warn(error); + } + }, async loadAllStatuses() { if (this._requireAdmin()) return false; try { diff --git a/frontend/tests/unit/doctor-panel.test.js b/frontend/tests/unit/doctor-panel.test.js new file mode 100644 index 000000000..bf43b1880 --- /dev/null +++ b/frontend/tests/unit/doctor-panel.test.js @@ -0,0 +1,198 @@ +/* + * Tests for the Jobs tab's Doctor panel. + * + * The panel is what an administrator sees of comicbox's doctor report, so + * behavior locked in here: + * - Every status the doctor can report wears the chip colour the + * component's own map says, so a status the backend adds without a + * colour fails here rather than rendering grey by accident. + * - A section header renders once, ahead of its first row, and the rows + * stay in report order. + * - The hint carries the platform header and a verdict that counts the + * problems in plain words, red when there are any. + * - A fix is a shell command or a setting path, so it renders as code. + * - A report already in the store shows at once without a refetch; an + * empty store fetches on mount; Re-check always fetches. + */ +import { createTestingPinia } from "@pinia/testing"; +import { mount } from "@vue/test-utils"; +import { describe, expect, test } from "vitest"; + +import DoctorPanel, { + STATUS_COLORS, +} from "@/components/admin/tabs/doctor-panel.vue"; +import vuetify from "@/plugins/vuetify"; +import { useAdminStore } from "@/stores/admin"; + +const HEADER = ["comicbox 5.3.0", "Python 3.14.4", "Linux-6.1", "Docker"]; +const PDF_FIX = "pip install --force-reinstall 'comicbox-pdffile~=1.0'"; + +const RESULTS = [ + { + section: "Archives", + name: "CBR", + status: "OK", + found: "rarfile 4.5", + detail: "via unrar · probe read OK · crypto: cryptography", + fix: "", + }, + { + section: "Archives", + name: "PDF", + status: "MISCONFIGURED", + found: "comicbox-pdffile 1.0.0", + detail: "pdffile fails to import: ImportError(...)", + fix: PDF_FIX, + }, + { + section: "Images (online cover matching)", + name: "Pillow", + status: "OK", + found: "Pillow 12.3.0", + detail: "jpeg webp png gif · no jxl codec", + fix: "", + }, + { + section: "Config", + name: "user config", + status: "OK", + found: "~/.config/comicbox/config.yaml", + detail: "parsed", + fix: "", + }, + { + section: "Config", + name: "unknown key", + status: "WARN", + found: "~/.config/comicbox/config.yaml", + detail: "general.loglevl is ignored", + fix: "did you mean general.loglevel?", + }, + { + section: "Python packages", + name: "requirements", + status: "OK", + found: "", + detail: "31 satisfied", + fix: "", + }, +]; + +const REPORT = { header: HEADER, results: RESULTS, problems: 1 }; + +// One row per status the component knows, so the chip test enumerates the +// vocabulary from the component instead of keeping a second copy here. +const EVERY_STATUS_REPORT = { + header: HEADER, + results: Object.keys(STATUS_COLORS).map((status) => ({ + section: "Statuses", + name: status.toLowerCase(), + status, + found: "", + detail: "", + fix: "", + })), + problems: 4, +}; + +const COLOR_CLASSES = new Set( + Object.values(STATUS_COLORS) + .filter(Boolean) + .map((color) => `text-${color}`), +); + +function mountPanel(doctor) { + const pinia = createTestingPinia({ initialState: { admin: { doctor } } }); + const wrapper = mount(DoctorPanel, { + global: { plugins: [pinia, vuetify] }, + }); + return { wrapper, store: useAdminStore() }; +} + +describe("AdminDoctorPanel", () => { + test("every status wears its chip colour", () => { + const chips = mountPanel(EVERY_STATUS_REPORT).wrapper.findAll(".v-chip"); + expect(chips).toHaveLength(Object.keys(STATUS_COLORS).length); + for (const chip of chips) { + const status = chip.text(); + const color = STATUS_COLORS[status]; + const colorClasses = chip + .classes() + .filter((cls) => COLOR_CLASSES.has(cls)); + // A tonal chip carries exactly its colour as text-; an + // uncoloured one carries none, which is the default grey. + expect(colorClasses).toStrictEqual(color ? [`text-${color}`] : []); + } + }); + + test("renders each section header once, ahead of its rows", () => { + const { wrapper } = mountPanel(REPORT); + const headers = wrapper + .findAll(".doctorSectionRow") + .map((row) => row.text()); + expect(headers).toStrictEqual([ + "Archives", + "Images (online cover matching)", + "Config", + "Python packages", + ]); + // Rows stay in report order under their headers. + const names = wrapper.findAll(".doctorName").map((cell) => cell.text()); + expect(names).toStrictEqual(RESULTS.map((result) => result.name)); + // The first row of the table is a header, not a check. + expect(wrapper.find("tbody tr").classes()).toContain("doctorSectionRow"); + }); + + test.each([ + { problems: 0, verdict: "No problems", cls: "doctorVerdictOk" }, + { problems: 1, verdict: "1 problem", cls: "doctorVerdictProblems" }, + { problems: 3, verdict: "3 problems", cls: "doctorVerdictProblems" }, + ])("verdict for $problems problems", ({ problems, verdict, cls }) => { + const { wrapper } = mountPanel({ ...REPORT, problems }); + const el = wrapper.find(".doctorVerdict"); + expect(el.text()).toBe(verdict); + expect(el.classes()).toContain(cls); + }); + + test("renders the header line in the hint", () => { + const hint = mountPanel(REPORT).wrapper.find(".adminHint").text(); + expect(hint).toContain(HEADER.join(" · ")); + expect(hint).toContain("1 problem"); + // The hint does not restate the section title. + expect(hint).not.toContain("Doctor"); + }); + + test("renders a fix as code and an empty fix as nothing", () => { + const { wrapper } = mountPanel(REPORT); + const fixes = wrapper.findAll(".doctorFix"); + expect(fixes).toHaveLength(RESULTS.length); + const codes = wrapper.findAll(".doctorFix code").map((el) => el.text()); + expect(codes).toStrictEqual([PDF_FIX, "did you mean general.loglevel?"]); + expect(fixes[0].find("code").exists()).toBe(false); + expect(fixes[0].text()).toBe(""); + }); + + test("renders found and detail", () => { + const text = mountPanel(REPORT).wrapper.text(); + expect(text).toContain("rarfile 4.5"); + expect(text).toContain("general.loglevl is ignored"); + }); + + test("shows Checking… and no table before the report arrives", () => { + const { wrapper } = mountPanel(); + expect(wrapper.find(".adminHint").text()).toBe("Checking…"); + expect(wrapper.find(".doctorTable").exists()).toBe(false); + expect(wrapper.find(".v-btn").exists()).toBe(true); + }); + + test("fetches on mount only when the store is empty", () => { + expect(mountPanel(REPORT).store.loadDoctor).not.toHaveBeenCalled(); + expect(mountPanel().store.loadDoctor).toHaveBeenCalledTimes(1); + }); + + test("Re-check always fetches", async () => { + const { wrapper, store } = mountPanel(REPORT); + await wrapper.find(".v-btn").trigger("click"); + expect(store.loadDoctor).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/test_admin_doctor_endpoint.py b/tests/test_admin_doctor_endpoint.py new file mode 100644 index 000000000..9b1e5ba6c --- /dev/null +++ b/tests/test_admin_doctor_endpoint.py @@ -0,0 +1,91 @@ +"""Integration tests for the /admin/doctor endpoint.""" + +from pathlib import Path +from typing import Final, override +from unittest.mock import patch + +from comicbox.doctor import CheckResult, DoctorReport, Status +from django.contrib.auth.models import User +from django.test import Client, TestCase + +_TEST_PASSWORD: Final = "test-pw-hush-S106" # noqa: S105 +_URL: Final = "/api/v4/admin/doctor" +_HTTP_OK: Final = 200 +_HTTP_FORBIDDEN: Final = 403 + +_REPORT = DoctorReport( + header=("comicbox 5.3.0", "Python 3.14.4", "Linux-6.1", "Docker"), + results=( + CheckResult( + "Archives", "CBR", Status.OK, found="rarfile 4.5", detail="via unrar" + ), + CheckResult( + "Archives", + "PDF", + Status.WRONG_VERSION, + found="comicbox-pdffile 0.6.3", + detail="comicbox requires comicbox-pdffile~=1.0", + fix="pip install 'comicbox-pdffile~=1.0'", + ), + CheckResult( + "Config", + "user config", + Status.OK, + found=Path("/home/abc/.config/comicbox/config.yaml"), + detail="parsed", + ), + ), +) + + +def _data(response) -> dict: + """Unwrap the v4 ``{data, meta, errors}`` envelope.""" + return response.json()["data"] + + +class DoctorAuthTestCase(TestCase): + """The report names paths and versions, so only admins read it.""" + + def test_anonymous_blocked(self) -> None: + assert Client().get(_URL).status_code == _HTTP_FORBIDDEN + + def test_non_admin_blocked(self) -> None: + User.objects.create_user(username="regular", password=_TEST_PASSWORD) + client = Client() + client.login(username="regular", password=_TEST_PASSWORD) + assert client.get(_URL).status_code == _HTTP_FORBIDDEN + + +class DoctorReportTestCase(TestCase): + """The rows arrive as plain strings with a problem count.""" + + @override + def setUp(self) -> None: + User.objects.create_user( + username="doctor_admin", + password=_TEST_PASSWORD, + is_staff=True, + is_superuser=True, + ) + self.client.login(username="doctor_admin", password=_TEST_PASSWORD) + + def test_report(self) -> None: + with patch("codex.views.admin.doctor.run_doctor", return_value=_REPORT): + response = self.client.get(_URL) + assert response.status_code == _HTTP_OK + data = _data(response) + assert data["header"] == list(_REPORT.header) + assert data["problems"] == 1 + pdf = data["results"][1] + assert pdf == { + "section": "Archives", + "name": "PDF", + # The enum's spelling, space included, not its member name. + "status": "WRONG VERSION", + "found": "comicbox-pdffile 0.6.3", + "detail": "comicbox requires comicbox-pdffile~=1.0", + "fix": "pip install 'comicbox-pdffile~=1.0'", + } + # A Path renders as its string. + assert data["results"][2]["found"] == "/home/abc/.config/comicbox/config.yaml" + assert data["results"][0]["fix"] == "" diff --git a/tests/test_credential_validator.py b/tests/test_credential_validator.py index 79cb0344a..4b8aef846 100644 --- a/tests/test_credential_validator.py +++ b/tests/test_credential_validator.py @@ -1,8 +1,13 @@ """Unit tests for the online-source credential validator.""" -from typing import Final +from dataclasses import replace +from pathlib import Path +from typing import Any, Final from unittest.mock import MagicMock, patch +from comicbox.config.online.settings import OnlineSourceCredentials +from comicbox.formats.comicvine_api.online_source import ComicVineOnlineSource +from comicbox.formats.metron_api.online_source import MetronOnlineSource from comicbox.online_session import OnlineCredentials from codex.librarian.onlinetag.credential_validator import ( @@ -12,6 +17,23 @@ ValidationResult, validate_credentials, ) +from codex.settings import COMICBOX_ONLINE_CONFIG + +# What Metron's probe returns off the validation response's X-RateLimit-* +# headers. The reset epochs are what codex deliberately drops. +_METRON_WINDOWS: Final = { + "burst": {"limit": 20, "remaining": 19, "reset_epoch": None}, + "daily": {"limit": 25_000, "remaining": 24_987, "reset_epoch": 1_784_419_200.0}, +} +# A response that carried no X-RateLimit-* headers at all. +_EMPTY_METRON_WINDOWS: Final = { + "burst": {"limit": None, "remaining": None, "reset_epoch": None}, + "daily": {"limit": None, "remaining": None, "reset_epoch": None}, +} +# Comic Vine's per-endpoint hourly pools; never shown, whatever they hold. +_COMICVINE_WINDOWS: Final = { + "origins": {"limit": 200, "remaining": 199, "reset_epoch": 1_784_419_200.0}, +} def _full_creds() -> OnlineCredentials: @@ -26,6 +48,17 @@ def _legacy_creds() -> OnlineCredentials: ) +def _probe(source_cls: type, **kwargs: Any): + """Patch ``source_cls.probe``; autospec so the mock is called with ``self``.""" + return patch.object(source_cls, "probe", autospec=True, **kwargs) + + +def _probed_source(probe: MagicMock) -> Any: + """Return the source instance the (autospecced) probe ran on.""" + probe.assert_called_once() + return probe.call_args.args[0] + + class TestValidateCredentials: """Cover success, auth-error, transport-error, and missing-creds paths.""" @@ -33,76 +66,21 @@ def test_known_sources_set(self) -> None: assert frozenset({"metron", "comicvine"}) == KNOWN_SOURCES def test_metron_success(self) -> None: - from mokkari.session import RateLimitStatus - - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.return_value = [] - # A session that saw no X-RateLimit-* headers -> rate_limits None. - instance.rate_limit_status = RateLimitStatus() - mocked.return_value = instance + with _probe(MetronOnlineSource, return_value=_EMPTY_METRON_WINDOWS) as probe: results = validate_credentials(_full_creds(), {"metron"}) assert results == {"metron": ValidationResult(ok=True)} - instance.publishers_list.assert_called_once_with({"page": 1}) - assert mocked.call_args.kwargs["api_token"] == "token" # noqa: S105 - # Since mokkari 4.8.0 the session holds a pooled TLS connection - # until closed, and this one is used once in the web process. - instance.close.assert_called_once_with() + source = _probed_source(probe) + assert source._credentials == OnlineSourceCredentials(key="token") # noqa: SLF001 - def test_metron_legacy_login_still_validates(self) -> None: - """A stored username & password authenticates when no API key is set.""" - from mokkari.session import RateLimitStatus - - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.return_value = [] - instance.rate_limit_status = RateLimitStatus() - mocked.return_value = instance - results = validate_credentials(_legacy_creds(), {"metron"}) + def test_metron_success_without_windows(self) -> None: + """A source that keeps no windows returns None; that is still a pass.""" + with _probe(MetronOnlineSource, return_value=None): + results = validate_credentials(_full_creds(), {"metron"}) assert results == {"metron": ValidationResult(ok=True)} - kwargs = mocked.call_args.kwargs - # None, not "" — mokkari sends a Bearer header for any non-None token, - # which would shut off the username & password fallback. - assert kwargs["api_token"] is None - assert kwargs["username"] == "user" - - def test_metron_key_wins_over_legacy_login(self) -> None: - """Both stored: the token is what mokkari gets told to prefer.""" - from mokkari.session import RateLimitStatus - - creds = OnlineCredentials( - metron_key="token", - metron_user="user", - metron_password="pw", # noqa: S106 - ) - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.return_value = [] - instance.rate_limit_status = RateLimitStatus() - mocked.return_value = instance - results = validate_credentials(creds, {"metron"}) - assert results["metron"].ok is True - assert mocked.call_args.kwargs["api_token"] == "token" # noqa: S105 def test_metron_success_reports_rate_limits(self) -> None: """The validation response's live account limits ride along, sans reset.""" - from datetime import UTC, datetime - - from mokkari.session import RateLimitStatus, RateLimitWindow - - status = RateLimitStatus( - burst=RateLimitWindow(limit=20, remaining=19), - sustained=RateLimitWindow( - limit=25_000, - remaining=24_987, - reset=datetime(2026, 7, 19, tzinfo=UTC), - ), - ) - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.return_value = [] - instance.rate_limit_status = status - mocked.return_value = instance + with _probe(MetronOnlineSource, return_value=_METRON_WINDOWS): results = validate_credentials(_full_creds(), {"metron"}) assert results == { "metron": ValidationResult( @@ -114,95 +92,142 @@ def test_metron_success_reports_rate_limits(self) -> None: ) } + def test_metron_partial_windows_keep_what_was_reported(self) -> None: + windows = {"daily": {"limit": 5_000, "remaining": None, "reset_epoch": None}} + with _probe(MetronOnlineSource, return_value=windows): + results = validate_credentials(_full_creds(), {"metron"}) + assert results["metron"].rate_limits == RateLimitInfo( + sustained=RateLimitWindowInfo(limit=5_000) + ) + + def test_metron_legacy_login_still_validates(self) -> None: + """A stored username & password authenticates when no API key is set.""" + with _probe(MetronOnlineSource, return_value=_EMPTY_METRON_WINDOWS) as probe: + results = validate_credentials(_legacy_creds(), {"metron"}) + assert results == {"metron": ValidationResult(ok=True)} + # key is None, not "" — mokkari sends a Bearer header for any non-None + # token, which would shut off the username & password fallback. + assert _probed_source(probe)._credentials == OnlineSourceCredentials( # noqa: SLF001 + user="user", + password="pw", # noqa: S106 + ) + + def test_metron_key_wins_over_legacy_login(self) -> None: + """Both stored: the token reaches mokkari, which prefers it.""" + creds = OnlineCredentials( + metron_key="token", + metron_user="user", + metron_password="pw", # noqa: S106 + ) + with _probe(MetronOnlineSource, return_value=_EMPTY_METRON_WINDOWS) as probe: + results = validate_credentials(creds, {"metron"}) + assert results["metron"].ok is True + assert _probed_source(probe)._credentials == OnlineSourceCredentials( # noqa: SLF001 + key="token", + user="user", + password="pw", # noqa: S106 + ) + def test_metron_auth_failure(self) -> None: from mokkari.exceptions import AuthenticationError - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.side_effect = AuthenticationError() - mocked.return_value = instance + with _probe(MetronOnlineSource, side_effect=AuthenticationError()): results = validate_credentials(_full_creds(), {"metron"}) assert results["metron"].ok is False assert "authoriz" in (results["metron"].error or "").lower() - # A rejected key leaves a connection open just the same. - instance.close.assert_called_once_with() def test_metron_api_error(self) -> None: from mokkari.exceptions import ApiError - with patch("mokkari.session.Session") as mocked: - instance = MagicMock() - instance.publishers_list.side_effect = ApiError("boom") - mocked.return_value = instance + with _probe(MetronOnlineSource, side_effect=ApiError("boom")): results = validate_credentials(_full_creds(), {"metron"}) assert results["metron"] == ValidationResult(ok=False, error="boom") + def test_error_text_is_the_exceptions_first_line(self) -> None: + """Some upstream errors carry a whole HTML page; the admin sees one line.""" + from mokkari.exceptions import ApiError + + err = ApiError("502 Bad Gateway\nnginx") + with _probe(MetronOnlineSource, side_effect=err): + results = validate_credentials(_full_creds(), {"metron"}) + assert results["metron"] == ValidationResult(ok=False, error="502 Bad Gateway") + def test_metron_missing_creds(self) -> None: creds = OnlineCredentials(metron_key="", metron_user="", metron_password="") - results = validate_credentials(creds, {"metron"}) + with _probe(MetronOnlineSource) as probe: + results = validate_credentials(creds, {"metron"}) assert results["metron"].ok is False assert "api key required" in (results["metron"].error or "").lower() + probe.assert_not_called() def test_metron_half_a_legacy_login_is_not_enough(self) -> None: creds = OnlineCredentials(metron_user="user", metron_password="") - results = validate_credentials(creds, {"metron"}) + with _probe(MetronOnlineSource) as probe: + results = validate_credentials(creds, {"metron"}) assert results["metron"].ok is False assert "api key required" in (results["metron"].error or "").lower() + probe.assert_not_called() def test_comicvine_success(self) -> None: - with patch("simyan.comicvine.Comicvine") as mocked: - instance = MagicMock() - instance.list_publishers.return_value = [] - mocked.return_value = instance + with _probe(ComicVineOnlineSource, return_value=_COMICVINE_WINDOWS) as probe: results = validate_credentials(_full_creds(), {"comicvine"}) + # Comic Vine's hourly pools never become the per-minute/per-day chip. assert results == {"comicvine": ValidationResult(ok=True)} - instance.list_publishers.assert_called_once_with( - params={"limit": "1"}, max_results=1 - ) + source = _probed_source(probe) # No override means simyan's own default endpoint. - assert "base_url" not in mocked.call_args.kwargs + assert source._credentials == OnlineSourceCredentials(key="key") # noqa: SLF001 + # Codex's own online settings, so the probe is counted against the + # same shared rate-limit bucket tagging runs draw on. + assert source._settings is COMICBOX_ONLINE_CONFIG.online # noqa: SLF001 def test_comicvine_custom_url_overrides_the_endpoint(self) -> None: - """A saved custom URL becomes simyan's base_url.""" + """A saved custom URL reaches the source as its base url.""" creds = OnlineCredentials( comicvine_key="key", comicvine_url="https://cv.example.com/api" ) - with patch("simyan.comicvine.Comicvine") as mocked: - instance = MagicMock() - instance.list_publishers.return_value = [] - mocked.return_value = instance + with _probe(ComicVineOnlineSource, return_value=_COMICVINE_WINDOWS) as probe: results = validate_credentials(creds, {"comicvine"}) assert results == {"comicvine": ValidationResult(ok=True)} - assert mocked.call_args.kwargs["base_url"] == "https://cv.example.com/api" + assert _probed_source(probe)._credentials == OnlineSourceCredentials( # noqa: SLF001 + key="key", url="https://cv.example.com/api" + ) def test_comicvine_url_alone_is_not_credentials(self) -> None: """A custom URL cannot authenticate; the key is still required.""" creds = OnlineCredentials(comicvine_url="https://cv.example.com/api") - results = validate_credentials(creds, {"comicvine"}) + with _probe(ComicVineOnlineSource) as probe: + results = validate_credentials(creds, {"comicvine"}) assert results["comicvine"].ok is False assert "required" in (results["comicvine"].error or "").lower() + probe.assert_not_called() def test_comicvine_auth_failure(self) -> None: from simyan.errors import AuthenticationError - with patch("simyan.comicvine.Comicvine") as mocked: - instance = MagicMock() - instance.list_publishers.side_effect = AuthenticationError("bad key") - mocked.return_value = instance + with _probe(ComicVineOnlineSource, side_effect=AuthenticationError("bad key")): results = validate_credentials(_full_creds(), {"comicvine"}) assert results["comicvine"] == ValidationResult(ok=False, error="bad key") - def test_comicvine_rejected_key_fails_through_the_real_simyan(self) -> None: + def test_comicvine_rejected_key_fails_through_the_real_simyan( + self, tmp_path: Path + ) -> None: """ A bad key must fail validation, not pass it. Comic Vine answers a rejected key with HTTP 200 and status_code 100 in the body. simyan 3.x returned that body verbatim, so the - publisher list came back empty and the key validated as good. - simyan 4 maps the status code to an exception, so this patches - the HTTP session rather than the client, and lets simyan's own - code decide. + listing came back empty and the key validated as good. simyan 4 + maps the status code to an exception, so this patches the HTTP + session rather than the client, and lets comicbox's real probe + and simyan's own code decide. The cache dir is pointed at a + scratch path so the probe's bucket and cache files stay out of + the developer's config dir. """ + online = COMICBOX_ONLINE_CONFIG.online + config = replace( + COMICBOX_ONLINE_CONFIG, + online=replace(online, cache=replace(online.cache, dir=tmp_path)), + ) response = MagicMock() response.raise_for_status.return_value = None response.json.return_value = { @@ -210,23 +235,55 @@ def test_comicvine_rejected_key_fails_through_the_real_simyan(self) -> None: "error": "Invalid API Key", "results": [], } - with patch("simyan.comicvine.CachedLimiterSession.request") as request: + with ( + patch( + "codex.librarian.onlinetag.credential_validator.COMICBOX_ONLINE_CONFIG", + config, + ), + patch("simyan.comicvine.CachedLimiterSession.request") as request, + ): request.return_value = response results = validate_credentials(_full_creds(), {"comicvine"}) assert results == { "comicvine": ValidationResult(ok=False, error="Invalid API Key") } + # The probe spent one request, on the origins pool tagging never uses. + request.assert_called_once() + assert "/origins" in request.call_args.kwargs["url"] + + def test_comicvine_rate_limit_exhausted(self) -> None: + """The probe refuses to block on a spent pool; the admin learns why.""" + from simyan.errors import RateLimitError + + err = RateLimitError("the hourly origins budget is spent") + with _probe(ComicVineOnlineSource, side_effect=err): + results = validate_credentials(_full_creds(), {"comicvine"}) + assert results["comicvine"] == ValidationResult( + ok=False, error="the hourly origins budget is spent" + ) + + def test_messageless_rate_limit_error_names_its_category(self) -> None: + """A 429 with no error body is ``RateLimitError(None)``, not "None".""" + from simyan.errors import RateLimitError + + with _probe(ComicVineOnlineSource, side_effect=RateLimitError(None)): + results = validate_credentials(_full_creds(), {"comicvine"}) + assert results["comicvine"] == ValidationResult(ok=False, error="Rate limited.") def test_comicvine_service_error(self) -> None: from simyan.errors import ServiceError - with patch("simyan.comicvine.Comicvine") as mocked: - instance = MagicMock() - instance.list_publishers.side_effect = ServiceError("upstream down") - mocked.return_value = instance + with _probe(ComicVineOnlineSource, side_effect=ServiceError("upstream down")): results = validate_credentials(_full_creds(), {"comicvine"}) assert results["comicvine"] == ValidationResult(ok=False, error="upstream down") + def test_unclassified_messageless_error_names_its_type(self) -> None: + with _probe(ComicVineOnlineSource, side_effect=ConnectionResetError()): + results = validate_credentials(_full_creds(), {"comicvine"}) + assert results["comicvine"] == ValidationResult( + ok=False, error="ConnectionResetError" + ) + def test_comicvine_missing_creds(self) -> None: creds = OnlineCredentials(comicvine_key="") results = validate_credentials(creds, {"comicvine"}) @@ -234,15 +291,10 @@ def test_comicvine_missing_creds(self) -> None: assert "required" in (results["comicvine"].error or "").lower() def test_default_targets_all_known_sources(self) -> None: - from mokkari.session import RateLimitStatus - with ( - patch("mokkari.session.Session") as metron_mock, - patch("simyan.comicvine.Comicvine") as cv_mock, + _probe(MetronOnlineSource, return_value=_EMPTY_METRON_WINDOWS), + _probe(ComicVineOnlineSource, return_value=_COMICVINE_WINDOWS), ): - metron_mock.return_value.publishers_list.return_value = [] - metron_mock.return_value.rate_limit_status = RateLimitStatus() - cv_mock.return_value.list_publishers.return_value = [] results = validate_credentials(_full_creds()) assert set(results) == KNOWN_SOURCES assert all(r.ok for r in results.values()) diff --git a/tests/test_dockerfile.py b/tests/test_dockerfile.py index 45b521b28..d751cbefe 100644 --- a/tests/test_dockerfile.py +++ b/tests/test_dockerfile.py @@ -110,3 +110,11 @@ def test_bytecode_is_kept(stages: dict[str, list[tuple[str, str]]]) -> None: ] assert installs assert all("--compile-bytecode" in command for command in installs) + + +def test_final_smoke_test_runs_the_doctor( + stages: dict[str, list[tuple[str, str]]], +) -> None: + """The doctor proves the image reads every archive format before it ships.""" + runs = _runs(stages["final"]) + assert [run for run in runs if "comicbox doctor -q" in run] diff --git a/tests/test_doctor.py b/tests/test_doctor.py new file mode 100644 index 000000000..101f0031e --- /dev/null +++ b/tests/test_doctor.py @@ -0,0 +1,104 @@ +""" +comicbox's doctor as codex runs it. + +The report is comicbox's; what codex adds is dropping the Online section, +counting the problems, and logging them once at startup. +""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from comicbox.doctor import CheckResult, DoctorReport, Status +from comicbox.doctor.online import SECTION as ONLINE_SECTION + +from codex.doctor import log_doctor_problems, problem_count, run_doctor + +_HEADER = ("comicbox 5.3.0", "Python 3.14.4", "Linux-6.1") +_CBR_MISSING = CheckResult( + "Archives", + "CBR", + Status.MISSING, + found="rarfile 4.5", + detail="no RAR tool found: 'unrar' not on path", + fix="apt install unrar (Debian: enable non-free)", +) +_PDF_OK = CheckResult( + "Archives", "PDF", Status.OK, found="comicbox-pdffile 1.0.0", detail="pymupdf" +) +_UNKNOWN_KEY = CheckResult( + "Config", + "unknown key", + Status.WARN, + found=Path("/config/comicbox.yaml"), + detail="general.loglevl is ignored", + fix="did you mean general.loglevel?", +) +_METRON = CheckResult(ONLINE_SECTION, "metron", Status.OFF, detail="no credentials") +_REPORT = DoctorReport( + header=_HEADER, results=(_CBR_MISSING, _PDF_OK, _UNKNOWN_KEY, _METRON) +) + + +def _patched(report: DoctorReport = _REPORT): + return patch("codex.doctor.run_checks", return_value=report) + + +class TestRunDoctor: + """What codex keeps of comicbox's report.""" + + def test_drops_the_online_section(self) -> None: + """Codex checks credentials on the Tagging tab, from its own database.""" + with _patched(): + report = run_doctor() + assert report.header == _HEADER + assert report.results == (_CBR_MISSING, _PDF_OK, _UNKNOWN_KEY) + + def test_problems_are_failures_not_warnings(self) -> None: + with _patched(): + report = run_doctor() + assert problem_count(report) == 1 + + def test_the_real_doctor_runs(self) -> None: + """Unpatched: comicbox's checks run here, whatever they find.""" + report = run_doctor() + assert report.header + names = {row.name for row in report.results} + assert {"CBZ", "CBR", "PDF", "Pillow", "cover hash"} <= names + assert not any(row.section == ONLINE_SECTION for row in report.results) + + +class TestLogDoctorProblems: + """Failures warn with their fix, warnings inform, and a verdict closes.""" + + def test_failures_warn_with_the_fix(self) -> None: + log = MagicMock() + with _patched(): + log_doctor_problems(log) + warnings = [call.args[0] for call in log.warning.call_args_list] + assert warnings == [ + ( + "comicbox doctor: CBR MISSING: no RAR tool found: 'unrar' not on path" + " Fix: apt install unrar (Debian: enable non-free)" + ), + "comicbox doctor found 1 problem. See the Admin Jobs tab.", + ] + + def test_warnings_inform(self) -> None: + log = MagicMock() + with _patched(): + log_doctor_problems(log) + infos = [call.args[0] for call in log.info.call_args_list] + assert infos == [ + ( + "comicbox doctor: unknown key WARN: general.loglevl is ignored" + " Fix: did you mean general.loglevel?" + ) + ] + + def test_a_clean_report_is_quiet(self) -> None: + log = MagicMock() + with _patched(DoctorReport(header=_HEADER, results=(_PDF_OK, _METRON))): + log_doctor_problems(log) + log.warning.assert_not_called() + log.info.assert_not_called() + log.debug.assert_called_once() diff --git a/tests/test_fs_filters_comic_suffixes.py b/tests/test_fs_filters_comic_suffixes.py new file mode 100644 index 000000000..7ae698c7e --- /dev/null +++ b/tests/test_fs_filters_comic_suffixes.py @@ -0,0 +1,45 @@ +""" +The scanner's comic suffixes come from comicbox, not from this host's tools. + +Leaving a suffix out of the scan because its tool is missing made every +existing row of that type look deleted to the poller. comicbox 5.3.0's +RAR probe test-extracts a member, so it fails on more hosts than the path +check it replaced, and this is where that must not cost anyone a library. +""" + +from pathlib import Path +from unittest.mock import patch + +import pytest +from comicbox.box import Comicbox +from comicbox.enums.comicbox import FileTypeEnum + +from codex.librarian.fs.filters import COMIC_SUFFIXES, match_comic + + +def test_suffixes_are_every_comicbox_file_type() -> None: + """A format comicbox adds is scanned without anyone editing a list here.""" + assert {f".{ft.value.lower()}" for ft in FileTypeEnum} == COMIC_SUFFIXES + assert {".cbz", ".cbr", ".cb7", ".cbt", ".pdf"} == COMIC_SUFFIXES + + +@pytest.mark.parametrize("name", ["a.cbz", "a.CBR", "a.cb7", "a.Cbt", "a.PDF"]) +def test_every_file_type_matches_in_any_case(name: str) -> None: + """The importer stores whatever case the filesystem had.""" + assert match_comic(Path(name)) + + +@pytest.mark.parametrize("name", ["a.jpg", "a.zip", "a.cbz.part", "noext", ".cbz"]) +def test_other_files_do_not_match(name: str) -> None: + """Only a whole, known suffix is a comic.""" + assert not match_comic(Path(name)) + + +def test_a_missing_tool_does_not_hide_its_comics() -> None: + """Unreadable archives fail their import with the reason; they are still scanned.""" + with ( + patch.object(Comicbox, "is_unrar_supported", return_value=False), + patch.object(Comicbox, "is_pdf_supported", return_value=False), + ): + assert match_comic(Path("/comics/a.cbr")) + assert match_comic(Path("/comics/a.pdf")) diff --git a/tests/test_janitor_folder_relations.py b/tests/test_janitor_folder_relations.py index c27c4e1f0..be68a7a3a 100644 --- a/tests/test_janitor_folder_relations.py +++ b/tests/test_janitor_folder_relations.py @@ -17,8 +17,8 @@ from django.test import TestCase from loguru import logger +from codex.librarian.fs.filters import COMIC_SUFFIXES from codex.librarian.scribe.janitor.integrity.foreign_keys import ( - _COMIC_SUFFIXES, _is_comic_path, fix_folder_relations, ) @@ -235,8 +235,8 @@ class ComicSuffixTests(TestCase): def test_suffixes_are_derived_from_comicbox(self) -> None: """A new archive format in comicbox must not be left behind here.""" - assert frozenset({".cbz", ".cbr", ".cb7", ".cbt", ".pdf"}) == _COMIC_SUFFIXES, ( - _COMIC_SUFFIXES + assert frozenset({".cbz", ".cbr", ".cb7", ".cbt", ".pdf"}) == COMIC_SUFFIXES, ( + COMIC_SUFFIXES ) def test_comic_paths_are_recognized_in_any_case(self) -> None: diff --git a/tests/test_onlinetag_credentials.py b/tests/test_onlinetag_credentials.py index f7f2b28ff..7fdbe42be 100644 --- a/tests/test_onlinetag_credentials.py +++ b/tests/test_onlinetag_credentials.py @@ -22,6 +22,9 @@ def test_metron_credentials_accept_a_key_or_a_legacy_login(self) -> None: legacy = manager._build_credentials() # noqa: SLF001 assert legacy is not None assert legacy.metron_user == "u" + # None, not "": mokkari sends a Bearer header for any token that is + # not None, which would replace the username and password. + assert legacy.metron_key is None assert manager._source_has_credentials(legacy, "metron") is True # noqa: SLF001 ComicboxTaggingDefaults.objects.update_or_create( @@ -31,6 +34,7 @@ def test_metron_credentials_accept_a_key_or_a_legacy_login(self) -> None: keyed = manager._build_credentials() # noqa: SLF001 assert keyed is not None assert keyed.metron_key == "t" + assert keyed.metron_user is None assert manager._source_has_credentials(keyed, "metron") is True # noqa: SLF001 def test_no_metron_credentials_at_all_is_unconfigured(self) -> None: diff --git a/tests/test_onlinetag_prompt_resolution.py b/tests/test_onlinetag_prompt_resolution.py index 7ceb496ba..7d62b762b 100644 --- a/tests/test_onlinetag_prompt_resolution.py +++ b/tests/test_onlinetag_prompt_resolution.py @@ -24,6 +24,7 @@ from codex.librarian.onlinetag.statuses import USER_MATCHED from codex.librarian.onlinetag.tasks import OnlineTagPromptResponseTask from codex.librarian.scribe.tagwrite_errors import get_tag_write_errors +from codex.settings import COMICBOX_ONLINE_CONFIG from tests.onlinetag_session_fakes import ( APPLY_FETCH_TARGET, APPLY_SESSION_TARGET, @@ -215,6 +216,30 @@ def test_resolve_unmatched_result_does_not_write(self) -> None: assert not self.write_tasks() + def test_replay_runs_with_codex_online_config(self) -> None: + """ + The replay reads the cache the search filled. + + Built without ``config``, the session fell back to comicbox's own + defaults: the user's config file and a cache dir under platformdirs, + not the one under /config the search wrote to. + """ + comic = make_comic() + set_pending_prompts({"fp1": _prompt(comic, candidates=[{"source": "metron"}])}) + FakeSession.tag_results = [ + SimpleNamespace( + path=Path(comic.path), + tags={"series": "Existing"}, + error=None, + matched=False, + ) + ] + + with patch(APPLY_SESSION_TARGET, FakeSession): + self.manager.resolve_prompt("fp1", "choose", 0, None) + + assert FakeSession.last_kwargs["config"] is COMICBOX_ONLINE_CONFIG + def test_resolve_skip_drops_prompt_without_writing(self) -> None: set_pending_prompts( {"fp1": {"fingerprint": "fp1", "pk": 1, "path": "/c/1.cbz", "source": "x"}} From 697e238e2295af8581d68226df4b626b6c6f0f2e Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Tue, 6 Oct 2026 19:47:33 -0700 Subject: [PATCH 2/7] Doctor on the Stats tab, with codex's own checks Move the Doctor section from the Jobs tab to the top of the Stats tab, above the Platform readout it extends: Jobs is for actions, and an admin asking what this install is goes to Stats. codex/doctor is now a package. checks.py adds a "Codex" section of rows, in comicbox's row shape so the panel and the startup log treat them alike: - database: SQLite version, FTS5 (proved with a temp virtual table, since not every build records compile options) and the journal mode in effect. - config dir: writable, with free space; warns under 1 GiB, since the database, backups, logs and cover cache all grow there. - one row per library: missing, the unmounted docker volume sentinel, unreadable, not writable unless read only, or empty. - watcher: Folder rows under event-enabled libraries against the kernel's inotify watch limit on Linux, with a sysctl to run on the host. - credentials: the stored tagging credentials read raw through Cast so the field's converter cannot hand back ciphertext, decrypted with the live key; a changed key file is otherwise invisible until a run fails. The view runs the report through channels' database_sync_to_async now that it queries the database. Co-Authored-By: Claude Fable 5.1 --- NEWS.md | 6 +- README.md | 10 +- codex/doctor.py | 72 ----- codex/doctor/__init__.py | 90 ++++++ codex/doctor/checks.py | 303 ++++++++++++++++++ codex/views/admin/doctor.py | 11 +- .../components/admin/tabs/doctor-panel.vue | 10 +- .../src/components/admin/tabs/job-tab.vue | 3 - .../src/components/admin/tabs/stats-tab.vue | 5 +- frontend/tests/unit/doctor-panel.test.js | 2 +- tests/test_doctor.py | 56 +++- tests/test_doctor_codex_checks.py | 259 +++++++++++++++ 12 files changed, 716 insertions(+), 111 deletions(-) delete mode 100644 codex/doctor.py create mode 100644 codex/doctor/__init__.py create mode 100644 codex/doctor/checks.py create mode 100644 tests/test_doctor_codex_checks.py diff --git a/NEWS.md b/NEWS.md index e1460037c..8b72433fb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,9 +9,9 @@ border-radius: 128px; ## v2.5.3 - Features - - Doctor report on the admin Jobs tab: archive tools, PDF support, image - codecs, config and package problems, each with its fix. Problems are also - logged at startup. + - Doctor report on the admin Stats tab: archive tools, PDF support, image + codecs, database, libraries, watcher limits, config and package problems, + each with its fix. Problems are also logged at startup. - Fixes - A missing or broken unrar no longer hides CBR comics from scans or deletes them from the library; they are listed as failed imports with the cause. diff --git a/README.md b/README.md index 714be5dc4..34188ab5b 100644 --- a/README.md +++ b/README.md @@ -1109,10 +1109,12 @@ LOGLEVEL=DEBUG codex ### 🩺 Doctor -The Admin panel's Jobs tab ends with a Doctor report: whether this install can -read each comic archive format, has the image codecs cover matching needs, and -whether comicbox's config and package versions are in order, with a fix for each -problem. Codex logs the problems when it starts, too. +The Admin panel's Stats tab opens with a Doctor report: whether this install can +read each comic archive format, has the image codecs cover matching needs, +whether comicbox's config and package versions are in order, and whether the +database, config directory, library folders, filesystem watcher and stored +credentials are usable, with a fix for each problem. Codex logs the problems +when it starts, too. The same report runs from the command line, which helps when Codex will not start. In Codex's Python environment: diff --git a/codex/doctor.py b/codex/doctor.py deleted file mode 100644 index cbbacf093..000000000 --- a/codex/doctor.py +++ /dev/null @@ -1,72 +0,0 @@ -""" -comicbox's doctor, run for this install. - -comicbox 5.3.0's ``run_checks`` reports whether this host can read each -archive format, whether Pillow has the codecs cover matching needs, whether -the user's comicbox config parses, and whether comicbox's package pins are -satisfied, with a one-line fix for anything that is not. Codex shows the -rows to admins and logs the problems once at startup, so a missing unrar or -a pymupdf built against the wrong libmupdf is explained in one place rather -than discovered as failed imports with no cause. - -The Online section is left out. Codex keeps the tagging credentials in its -own database and checks them on the Tagging tab, so the doctor's "no -credentials" rows and its ``--online`` advice would describe the CLI, not -this server. - -comicbox caches its tool probes for the life of the process, so a tool -installed while codex runs shows up after a restart. -""" - -from __future__ import annotations - -from dataclasses import replace -from typing import TYPE_CHECKING - -from comicbox.doctor import Status, run_checks -from comicbox.doctor.online import SECTION as _ONLINE_SECTION - -if TYPE_CHECKING: - from comicbox.doctor import CheckResult, DoctorReport - from loguru import Logger - - -def run_doctor() -> DoctorReport: - """Run comicbox's doctor and keep the rows that describe this server.""" - report = run_checks() - results = tuple(row for row in report.results if row.section != _ONLINE_SECTION) - return replace(report, results=results) - - -def problem_count(report: DoctorReport) -> int: - """Count the rows that say something comicbox needs is broken.""" - return sum(row.status.is_failure for row in report.results) - - -def _describe(row: CheckResult) -> str: - """One log line for a row: what, how bad, why, and the fix if there is one.""" - line = f"comicbox doctor: {row.name} {row.status}: {row.detail}" - if row.fix: - line += f" Fix: {row.fix}" - return line - - -def log_doctor_problems(log: Logger) -> None: - """ - Log what the doctor found, once, after logging is configured. - - Failures warn and warnings inform. The scanner used to warn about an - unreadable archive format at import time, before any log sink existed, - so the message only ever reached stderr. - """ - report = run_doctor() - for row in report.results: - if row.status.is_failure: - log.warning(_describe(row)) - elif row.status is Status.WARN: - log.info(_describe(row)) - if problems := problem_count(report): - noun = "problem" if problems == 1 else "problems" - log.warning(f"comicbox doctor found {problems} {noun}. See the Admin Jobs tab.") - else: - log.debug("comicbox doctor found no problems.") diff --git a/codex/doctor/__init__.py b/codex/doctor/__init__.py new file mode 100644 index 000000000..09d1328b9 --- /dev/null +++ b/codex/doctor/__init__.py @@ -0,0 +1,90 @@ +""" +The doctor: comicbox's health checks plus codex's own, for this install. + +comicbox 5.3.0's ``run_checks`` reports whether this host can read each +archive format, whether Pillow has the codecs cover matching needs, whether +the user's comicbox config parses, and whether comicbox's package pins are +satisfied, with a one-line fix for anything that is not. Codex adds the +checks for what it needs on top (``codex.doctor.checks``), shows the rows +to admins, and logs the problems once at startup, so a missing unrar or a +full disk is explained in one place rather than discovered as failed +imports with no cause. + +comicbox's Online section is left out. Codex keeps the tagging credentials +in its own database and checks them on the Tagging tab, so the doctor's "no +credentials" rows and its ``--online`` advice would describe the CLI, not +this server. + +comicbox caches its tool probes for the life of the process, so a tool +installed while codex runs shows up after a restart. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from comicbox.doctor import CheckResult, DoctorReport, Status, run_checks +from comicbox.doctor.online import SECTION as _ONLINE_SECTION + +from codex.doctor.checks import CHECKS, SECTION + +if TYPE_CHECKING: + from loguru import Logger + + from codex.doctor.checks import Check + + +def _run_check(name: str, check: Check) -> list[CheckResult]: + """Run one codex check; a crash becomes one ERROR row and hides nothing else.""" + rows: list[CheckResult] = [] + try: + rows.extend(check()) + except Exception as exc: # reported as the row + rows.append( + CheckResult( + SECTION, name, Status.ERROR, detail=f"{type(exc).__name__}: {exc}" + ) + ) + return rows + + +def run_doctor() -> DoctorReport: + """Run comicbox's doctor and codex's checks; keep the rows that describe this server.""" + report = run_checks() + comicbox_rows = [row for row in report.results if row.section != _ONLINE_SECTION] + codex_rows = [row for name, check in CHECKS for row in _run_check(name, check)] + return DoctorReport(header=report.header, results=(*comicbox_rows, *codex_rows)) + + +def problem_count(report: DoctorReport) -> int: + """Count the rows that say something codex needs is broken.""" + return sum(row.status.is_failure for row in report.results) + + +def _describe(row: CheckResult) -> str: + """One log line for a row: what, how bad, why, and the fix if there is one.""" + line = f"doctor: {row.name} {row.status}: {row.detail}" + if row.fix: + line += f" Fix: {row.fix}" + return line + + +def log_doctor_problems(log: Logger) -> None: + """ + Log what the doctor found, once, after logging is configured. + + Failures warn and warnings inform. The scanner used to warn about an + unreadable archive format at import time, before any log sink existed, + so the message only ever reached stderr. + """ + report = run_doctor() + for row in report.results: + if row.status.is_failure: + log.warning(_describe(row)) + elif row.status is Status.WARN: + log.info(_describe(row)) + if problems := problem_count(report): + noun = "problem" if problems == 1 else "problems" + log.warning(f"The doctor found {problems} {noun}. See the Admin Stats tab.") + else: + log.debug("The doctor found no problems.") diff --git a/codex/doctor/checks.py b/codex/doctor/checks.py new file mode 100644 index 000000000..40603e552 --- /dev/null +++ b/codex/doctor/checks.py @@ -0,0 +1,303 @@ +""" +What codex itself needs, as rows of the doctor report. + +comicbox's doctor covers what comicbox needs. These rows cover the rest: +the database engine, the config directory, each library root, the +filesystem watcher's kernel limits, and the key that decrypts the stored +tagging credentials. Each is a comicbox ``CheckResult`` so the admin page +and the startup log treat them exactly like comicbox's own. + +Paths appear here because this is an admin-only page. Nothing from these +rows is ever sent anywhere; the telemeter has its own, count-only report. +""" + +from __future__ import annotations + +import os +import shutil +import sqlite3 +import sys +from functools import partial +from pathlib import Path +from typing import TYPE_CHECKING + +from comicbox.doctor import CheckResult, Status +from cryptography.fernet import Fernet, InvalidToken +from django.conf import settings +from django.db import connection +from django.db.models import TextField +from django.db.models.functions import Cast +from django.db.utils import OperationalError +from django.template.defaultfilters import filesizeformat + +from codex.librarian.fs.mounted import DOCKER_UNMOUNTED_FN +from codex.models.admin import ComicboxTaggingDefaults +from codex.models.collections import Folder +from codex.models.fields import EncryptedCharField +from codex.models.library import Library +from codex.settings import CONFIG_PATH +from codex.util import is_docker + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Iterator + +SECTION = "Codex" + +#: (check name, check). The name labels the ERROR row the runner reports +#: when the check itself crashes. +type Check = Callable[[], Iterable[CheckResult]] + +_row = partial(CheckResult, SECTION) + +# Below this much free space the config dir row warns: the database, its +# nightly backups, the logs and the cover cache all grow here. +_LOW_DISK_BYTES = 1 << 30 +_PERMISSIONS_FIX = "fix its permissions (Docker: PUID and PGID)" + +# The kernel's inotify limits, which the event watcher needs one watch per +# folder against. Only Linux has them; macOS and the BSDs watch by other +# means. Per real user across every process, so other watchers share them. +_INOTIFY_DIR = Path("/proc/sys/fs/inotify") +_HAS_INOTIFY = sys.platform == "linux" +# Warn before the limit is reached: other processes draw on the same count. +_WATCH_HEADROOM = 0.8 +_WATCHES_SUGGESTED_MIN = 524_288 + + +def check_database() -> Iterator[CheckResult]: + """SQLite's version, the FTS5 engine search indexes with, and the journal mode.""" + found = f"sqlite {sqlite3.sqlite_version}" + with connection.cursor() as cursor: + cursor.execute("PRAGMA journal_mode") + mode_row = cursor.fetchone() + journal_mode = str(mode_row[0] if mode_row else "").lower() + try: + # A temp table proves the engine is there; compile options are + # not recorded in every build. + cursor.execute( + "CREATE VIRTUAL TABLE temp.codex_doctor_fts USING fts5(probe)" + ) + cursor.execute("DROP TABLE temp.codex_doctor_fts") + except OperationalError as exc: + yield _row( + "database", + Status.MISSING, + found=found, + detail=f"no FTS5: comics cannot be search indexed ({exc})", + fix="use a Python whose sqlite3 was built with FTS5", + ) + return + if journal_mode != "wal": + yield _row( + "database", + Status.WARN, + found=found, + detail=f"fts5 · journal mode is {journal_mode}, not wal: readers block writers", + fix="keep the config dir on a local filesystem, not a network share", + ) + return + yield _row("database", Status.OK, found=found, detail=f"fts5 · {journal_mode}") + + +def check_config_dir() -> Iterator[CheckResult]: + """Check the config dir is writable with room: the database, backups, logs and covers live here.""" + if not os.access(CONFIG_PATH, os.W_OK): + yield _row( + "config dir", + Status.MISCONFIGURED, + found=CONFIG_PATH, + detail="not writable: the database, logs, backups and covers live here", + fix=_PERMISSIONS_FIX, + ) + return + free = shutil.disk_usage(CONFIG_PATH).free + detail = f"writable · {filesizeformat(free)} free" + if free < _LOW_DISK_BYTES: + yield _row( + "config dir", + Status.WARN, + found=CONFIG_PATH, + detail=detail, + fix="free some space: backups, logs and the cover cache grow here", + ) + return + yield _row("config dir", Status.OK, found=CONFIG_PATH, detail=detail) + + +def _check_library(root: Path, *, read_only: bool) -> CheckResult: + """One library root: there, readable, and writable unless it is read only.""" + name = "library" + if not root.is_dir(): + return _row( + name, + Status.MISSING, + found=root, + detail="not there: its comics look deleted until it is back", + fix="mount it, or remove the library on the Libraries tab", + ) + if (root / DOCKER_UNMOUNTED_FN).exists(): + return _row( + name, + Status.MISCONFIGURED, + found=root, + detail="is the unmounted docker volume, not your comics", + fix="bind mount the comics directory at this path", + ) + if not os.access(root, os.R_OK | os.X_OK): + return _row( + name, + Status.MISCONFIGURED, + found=root, + detail="not readable", + fix=_PERMISSIONS_FIX, + ) + notes = ["readable"] + if read_only: + notes.append("read only") + elif os.access(root, os.W_OK): + notes.append("writable") + else: + return _row( + name, + Status.WARN, + found=root, + detail="readable · not writable: tag writes and renames fail", + fix=f"{_PERMISSIONS_FIX}, or mark it read only on the Libraries tab", + ) + if not any(root.iterdir()): + return _row( + name, + Status.WARN, + found=root, + detail=" · ".join([*notes, "empty: suspect unmounted"]), + fix="mount it, or add comics", + ) + return _row(name, Status.OK, found=root, detail=" · ".join(notes)) + + +def check_libraries() -> Iterator[CheckResult]: + """Every library root, in path order.""" + libraries = list(Library.objects.order_by("path").values_list("path", "read_only")) + if not libraries: + yield _row( + "library", + Status.OFF, + detail="no libraries", + fix="add one on the Libraries tab", + ) + return + for path, read_only in libraries: + yield _check_library(Path(path), read_only=read_only) + + +def _inotify_limit(name: str) -> int: + return int((_INOTIFY_DIR / name).read_text()) + + +def _suggested_watches(folders: int) -> int: + """Suggest a limit with room to grow: the usual recommendation, or the next power of two.""" + return max(_WATCHES_SUGGESTED_MIN, 1 << (2 * folders).bit_length()) + + +def check_watcher() -> Iterator[CheckResult]: + """Whether the kernel lets the event watcher follow every folder it must.""" + roots = Library.objects.filter(events=True).count() + if not roots: + yield _row("watcher", Status.OFF, detail="no library watches for events") + return + # One inotify watch per directory. Folder rows are the directories + # codex knows, so this is a floor; comic-less directories add to it. + folders = Folder.objects.filter(library__events=True).count() + roots + found = f"{folders} folders" + if not _HAS_INOTIFY: + yield _row( + "watcher", Status.OK, found=found, detail="no kernel watch limit here" + ) + return + try: + max_watches = _inotify_limit("max_user_watches") + except (OSError, ValueError) as exc: + yield _row( + "watcher", + Status.WARN, + found=found, + detail=f"inotify limit unreadable: {exc}", + fix="check fs.inotify.max_user_watches by hand", + ) + return + detail = f"inotify allows {max_watches}" + host = "the Docker host" if is_docker() else "this host" + fix = f"sysctl fs.inotify.max_user_watches={_suggested_watches(folders)} on {host}" + if folders > max_watches: + yield _row( + "watcher", + Status.MISCONFIGURED, + found=found, + detail=f"{detail}: changes in the rest go unseen until the next poll", + fix=fix, + ) + elif folders > max_watches * _WATCH_HEADROOM: + yield _row( + "watcher", + Status.WARN, + found=found, + detail=f"{detail}: nearly all of them, shared with every other watcher", + fix=fix, + ) + else: + yield _row("watcher", Status.OK, found=found, detail=detail) + + +def check_credentials() -> Iterator[CheckResult]: + """ + Whether the stored tagging credentials still decrypt. + + The field decrypts on load and, on failure, hands back the ciphertext + as if it were the value, so the Tagging tab would show gibberish and a + run would authenticate with it. A read through ``Cast`` skips the + field's converter and yields what is really stored. + """ + fields = [ + field + for field in ComicboxTaggingDefaults._meta.get_fields() + if isinstance(field, EncryptedCharField) + ] + raw = ( + ComicboxTaggingDefaults.objects.filter(pk=1) + .annotate(**{f"raw_{f.name}": Cast(f.name, TextField()) for f in fields}) + .values(*(f"raw_{f.name}" for f in fields)) + .first() + ) or {} + stored: dict[str, str] = { + f.name: token for f in fields if (token := raw.get(f"raw_{f.name}")) + } + if not stored: + yield _row("credentials", Status.OFF, detail="no tagging credentials stored") + return + fernet = Fernet(settings.FIELD_ENCRYPTION_KEY) + unreadable = [] + for name, token in stored.items(): + try: + fernet.decrypt(token.encode()) + except InvalidToken: + unreadable.append(name) + found = ", ".join(stored) + if unreadable: + yield _row( + "credentials", + Status.MISCONFIGURED, + found=", ".join(unreadable), + detail="cannot be decrypted: the key file in the config dir is not the one they were saved with", + fix="enter them again on the Tagging tab", + ) + return + yield _row("credentials", Status.OK, found=found, detail="decrypt") + + +CHECKS: tuple[tuple[str, Check], ...] = ( + ("database", check_database), + ("config dir", check_config_dir), + ("library", check_libraries), + ("watcher", check_watcher), + ("credentials", check_credentials), +) diff --git a/codex/views/admin/doctor.py b/codex/views/admin/doctor.py index 5b6af9351..dba856367 100644 --- a/codex/views/admin/doctor.py +++ b/codex/views/admin/doctor.py @@ -1,7 +1,7 @@ """Admin doctor view.""" from adrf.mixins import get_data -from asgiref.sync import sync_to_async +from channels.db import database_sync_to_async from rest_framework.response import Response from codex.doctor import problem_count, run_doctor @@ -16,10 +16,11 @@ class AdminDoctorView(AsyncAdminGenericAPIView): async def get(self, *_args, **_kwargs) -> Response: """Run the doctor and serialize its rows.""" - # About a second: a RAR extraction subprocess, config files and - # package metadata. Off the event loop, and off the one thread - # every sync view shares, since it touches no database. - report = await sync_to_async(run_doctor, thread_sensitive=False)() + # About a second: a RAR extraction subprocess, config files, + # package metadata and a few library queries. Off the event loop + # and off the one thread every sync view shares; channels' + # wrapper closes the connection the executor thread opened. + report = await database_sync_to_async(run_doctor, thread_sensitive=False)() obj = { "header": report.header, "results": report.results, diff --git a/frontend/src/components/admin/tabs/doctor-panel.vue b/frontend/src/components/admin/tabs/doctor-panel.vue index b6fcc14b6..0941abc57 100644 --- a/frontend/src/components/admin/tabs/doctor-panel.vue +++ b/frontend/src/components/admin/tabs/doctor-panel.vue @@ -1,9 +1,9 @@ @@ -245,7 +244,6 @@ import { camelCase } from "text-case"; import { ADMIN_JOBS } from "@/choices/admin-jobs.json"; import AdminSection from "@/components/admin/tabs/admin-section.vue"; -import DoctorPanel from "@/components/admin/tabs/doctor-panel.vue"; import AdminExpandToggle from "@/components/admin/tabs/expand-toggle.vue"; import { etaRemaining, @@ -277,7 +275,6 @@ export default { AdminExpandToggle, AdminSection, ConfirmDialog, - DoctorPanel, }, setup() { const { now } = useNowTimer(); diff --git a/frontend/src/components/admin/tabs/stats-tab.vue b/frontend/src/components/admin/tabs/stats-tab.vue index 665b468f1..291ca6381 100644 --- a/frontend/src/components/admin/tabs/stats-tab.vue +++ b/frontend/src/components/admin/tabs/stats-tab.vue @@ -1,5 +1,6 @@