Mark Task 1 done for NFOService per-task isolation
This commit is contained in:
170
docs/tasks.md
Normal file
170
docs/tasks.md
Normal file
@@ -0,0 +1,170 @@
|
||||
# Tasks — NFO Plot Missing Bug
|
||||
|
||||
These tasks fix the root causes of `<plot>` 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.
|
||||
Reference in New Issue
Block a user