Two pre-existing cover-upload issues CodeRabbit flagged on #841 (verbatim move,
so correctly not fixed there):
1. One `except Exception as e` wrapped BOTH the PIL decode and the img.save/
tmp.replace, returned 400 for both, and echoed `e` — so a disk/permission
failure was mislabeled a client error and could leak a filesystem path. Now:
decode/validation -> 400 "Invalid image" (generic); save/replace failure ->
logged 500 "could not save cover" (no detail).
2. A shared `{pid}.png.tmp` let two concurrent uploads clobber each other's temp
file. Now a unique `tempfile.mkstemp` in the cover dir, atomic replace to
publish. Plus an existence re-check just before publishing so an upload that
raced a playlist delete can't leave an orphan cover.
mkstemp itself is INSIDE the try (Codex catch): an unwritable dir / full disk
raises there and is the same persistence failure as save/replace, so it hits the
logged generic-500 path instead of escaping as an unhandled 500. Cleanup guards
`tmp is not None` for the mkstemp-failed case.
Did NOT add a full per-playlist critical section (CodeRabbit's "heavy lift"):
FeedBack is single-user (Principle I), so a cover upload racing a delete on the
same id can't happen — documented in the code rather than building a lock
framework for a precluded race.
tests/test_playlist_cover_errors.py pins all of it; negative-checked three ways:
the old single-except-400 shape fails the 500 + no-leak-400 tests, and moving
mkstemp back outside the try fails the temp-creation-500 test. Fix passes 5/5.
Full suite 2405 passed; boot smoke: valid cover 200, bad image -> generic
"Invalid image" 400, no .tmp litter.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The biggest router yet — 12 playlist routes + custom covers — and the first that
needs the config-path seam. server.py: 9,302 -> 9,085 (-217).
Three causally-linked pieces, all required for playlists:
- `config_dir` joins the appstate seam (the plan always put the path constants
there; deferred in S3, needed now). It's env-derived, so the ~49
pop-and-reimport fixtures reconfigure it for free — ZERO setattr retargeting.
STATIC_DIR/SLOPPAK_CACHE_DIR (patched via setattr) stay in server.py until a
router that reads them is extracted, and get retargeted then.
- `_clean_str` (pure request-field sanitizer, 14 callers) -> lib/reqfields.py;
server.py imports it back. Unblocks wanted/saved/collections/profile/... later.
- routers/playlists.py: bodies verbatim, `@app`->`@router`, `meta_db`->
`appstate.meta_db`, `CONFIG_DIR`->`appstate.config_dir`, `_clean_str` from
reqfields, `_ART_CACHE_HEADERS` as a local const (art keeps server.py's).
The two exclusive cover helpers (_playlist_cover_path/_url) move with it.
include_router at the original site; full 143-route table identical to
origin/main. One test retarget: test_playlists_api called
`server._playlist_cover_path` directly -> now imports it from routers.playlists
(reads appstate.config_dir, which the `server` fixture configures).
Verified: pyflakes clean; route table identical; pytest 2401 passed (28 in
playlists+collections+appstate); packaging guard 51 (auto-picked up reqfields);
eslint 0; boot smoke drives create/rename/add-song/cover-upload/serve/delete —
the cover writes 1.png under CONFIG_DIR THROUGH appstate.config_dir and serves
200 with an mtime cache-bust token; a wrong-typed name field still 400s via
_clean_str; demo untouched.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
