From 2aa184c87055685756df2605d6bf30367bd30039 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 19 Apr 2026 18:41:04 +0200 Subject: [PATCH] Mark Task 1 done for NFOService per-task isolation --- docs/tasks.md | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 docs/tasks.md diff --git a/docs/tasks.md b/docs/tasks.md new file mode 100644 index 0000000..e1140b2 --- /dev/null +++ b/docs/tasks.md @@ -0,0 +1,170 @@ +# Tasks — NFO Plot Missing Bug + +These tasks fix the root causes of `` being empty in `tvshow.nfo` after adding a series via the web UI. +The bug does **not** appear after a server restart because the repair scan uses a different, correctly isolated code path. + +--- + +## Task 1 — Replace shared NFOService in BackgroundLoaderService with per-task instances + +- [x] Completed + +### Where +`src/server/services/background_loader_service.py` — method `_load_nfo_and_images` (~line 555) + +```python +nfo_path = await self.series_app.nfo_service.create_tvshow_nfo( + serie_name=task.name, + serie_folder=task.folder, + year=task.year, + ... +) +``` + +### Goal +Create a fresh, isolated `NFOService` (with its own `TMDBClient` and `aiohttp` session) for every background loading task, exactly the same way `_repair_one_series` in `initialization_service.py` already does it. +Each task must own its client so that closing the session at the end of one task never kills an in-flight request inside another task. + +### How it should look +```python +from src.core.services.nfo_factory import NFOServiceFactory + +factory = NFOServiceFactory() +nfo_service = factory.create() +nfo_path = await nfo_service.create_tvshow_nfo( + serie_name=task.name, + serie_folder=task.folder, + year=task.year, + ... +) +``` + +### Possible traps and issues +- `NFOServiceFactory.create()` raises `ValueError` if no TMDB API key is available. Wrap in try/except and fall back gracefully (same behaviour as now when `nfo_service` is `None`). +- The factory reads the API key from `settings` first, then from `config.json`. Do not pass the key explicitly so the fallback chain stays intact. +- Each new `NFOService` opens its own `aiohttp` connector. Make sure to call `await nfo_service.close()` in a `finally` block to avoid connector leaks. + +### Docs changes needed +None — this is an internal implementation detail. + +### Why this is needed +Up to 5 background workers share one `NFOService`/`TMDBClient` instance. The `async with self.tmdb_client:` context manager inside `create_tvshow_nfo` calls `close()` on `__aexit__`, setting `session = None`. When Worker B exits its context while Worker A is still inside `_enrich_details_with_fallback` trying the `en-US` fallback request, that request throws "Connector is closed". The exception is silently swallowed, both `en-US` and `ja-JP` fallbacks fail, `details["overview"]` stays empty, and `plot` is written as an empty element. + +--- + +## Task 2 — Guard NFOService init in SeriesApp on factory fallback, not just env var + +### Where +`src/core/SeriesApp.py` — `__init__` method (~line 175) + +```python +self.nfo_service: Optional[NFOService] = None +if settings.tmdb_api_key: # ← checks env var ONLY + factory = get_nfo_factory() + self.nfo_service = factory.create() +``` + +### Goal +The guard condition should be equivalent to what `NFOServiceFactory.create()` itself checks: whether the key is available from *any* source (env var or `config.json`). Replace the guard with a try/create pattern so that `nfo_service` is initialised whenever the factory would succeed. + +### How it should look +```python +self.nfo_service: Optional[NFOService] = None +try: + from src.core.services.nfo_factory import get_nfo_factory + factory = get_nfo_factory() + self.nfo_service = factory.create() + logger.info("NFO service initialized successfully") +except ValueError: + logger.info("NFO service not available — TMDB API key not configured") +except Exception as e: + logger.warning("Failed to initialize NFO service: %s", e) +``` + +### Possible traps and issues +- This changes the condition from "env var set" to "factory can produce a service". The factory already has a safe fallback and raises `ValueError` when no key exists — so the `except ValueError` path is the normal "not configured" case, not an error. +- `SeriesApp` is used in tests with `settings.tmdb_api_key = None`. Those tests must not be affected; the `except ValueError` branch keeps behaviour identical. +- `series_app.nfo_service` is still `None` when not configured — downstream code that checks `if self.series_app.nfo_service:` remains correct. + +### Docs changes needed +`docs/CONFIGURATION.md` — note that `TMDB_API_KEY` env var is not required if `nfo.tmdb_api_key` is set in `config.json`. + +### Why this is needed +If the TMDB API key is configured only via `config.json` (not the `TMDB_API_KEY` env var), `settings.tmdb_api_key` is `None` and the guard prevents `nfo_service` from ever being created. The background loader then skips NFO creation completely (`nfo_service` is `None`). The repair scan at startup uses `NFOServiceFactory` directly (reads config.json) so it does create the NFO — which is exactly why restart works but add does not. + +--- + +## Task 3 — Remove non-reentrant `async with self.tmdb_client:` from NFOService public methods + +### Where +`src/core/services/nfo_service.py` — `create_tvshow_nfo` (~line 151) and `update_tvshow_nfo` (~line 265) + +```python +async with self.tmdb_client: + details = await self.tmdb_client.get_tv_show_details(...) + ... +``` + +### Goal +The `TMDBClient.__aenter__` / `__aexit__` open and **close** the session, making any concurrent call to the same client instance fail. Because Task 1 creates a fresh instance per call, this context manager becomes redundant. Change both methods to use `_ensure_session()` at the start and `close()` in a `finally` block, or simply call `await self.tmdb_client._ensure_session()` once and close after all requests. This makes the lifetime explicit and prevents double-close if the caller already manages it. + +### How it should look +```python +async def create_tvshow_nfo(self, ...) -> Path: + try: + await self.tmdb_client._ensure_session() + search_results = await self.tmdb_client.search_tv_show(search_name) + ... + finally: + await self.tmdb_client.close() +``` + +### Possible traps and issues +- `TMDBClient.close()` is idempotent (checks `session.closed` before closing), so calling it in `finally` is safe even if the try block never opened a session. +- After Task 1 every `NFOService` is short-lived (one call), so `finally: close()` effectively replaces the context manager with no behaviour change. +- Do not remove the `__aenter__`/`__aexit__` from `TMDBClient` itself — other callers (e.g. tests, CLI) may still use it as a context manager. +- `update_tvshow_nfo` has the same pattern; fix both methods. + +### Docs changes needed +None — internal implementation detail. + +### Why this is needed +Even after Task 1 fixes the shared-instance problem, the `async with self.tmdb_client:` pattern is fragile by design: `__aexit__` calls `close()`, which would break any hypothetical future reuse. Removing the implicit close makes the session lifetime explicit and eliminates the root mechanism that caused the original bug. + +--- + +## Task 4 — Add `en-US` search fallback so `search_overview` is never empty + +### Where +`src/core/services/nfo_service.py` — `create_tvshow_nfo` (~line 178) and `_enrich_details_with_fallback` (~line 395) + +```python +search_overview = tv_show.get("overview") or None # always None for anime — de-DE search returns "" +``` + +### Goal +When the German `search_tv_show` result has an empty `overview`, perform a second search in `en-US` to obtain a non-empty overview as the last-resort fallback text. Store this as `search_overview` so `_enrich_details_with_fallback` can use it even if all language-specific detail requests fail. + +### How it should look +```python +search_overview = tv_show.get("overview") or None +if not search_overview: + try: + en_results = await self.tmdb_client.search_tv_show(search_name, language="en-US") + en_match = self._find_best_match(en_results.get("results", []), search_name, year) + search_overview = en_match.get("overview") or None + except Exception: + pass # best-effort only +``` + +### Possible traps and issues +- This adds one extra TMDB request per series when the German overview is empty. It is best-effort and must be wrapped in a broad `except` so it never blocks NFO creation. +- The TMDB search endpoint rate-limit is generous; one extra request per add is negligible. +- `_find_best_match` can raise `TMDBAPIError` if the result list is empty — catch both `TMDBAPIError` and `Exception`. +- `update_tvshow_nfo` calls `_enrich_details_with_fallback` without `search_overview`. This is acceptable because the detail request with `en-US` fallback covers it; the search overview is only a last resort for the create path. + +### Docs changes needed +None — transparent improvement. + +### Why this is needed +Most anime have no German translation on TMDB. The `de-DE` search result returns `overview: ""`. The current code stores this as `search_overview = None` so the last-resort fallback in `_enrich_details_with_fallback` never fires. Combined with session contention (Task 1), the detail-level `en-US` fallback also fails, leaving `plot` empty. This task ensures that at least the search-level `en-US` overview is available as a safety net.