feat(525): external channel-logo URLs download + cache at save time (not fetched at render) #528

Merged

timothy merged 16 commits from feat/525-external-logo-download-on-save into main 2026-07-21 15:36:16 +02:00

Conversation 3 Commits 16 Files Changed 33

+2512 -274

timothy commented 2026-07-21 12:19:17 +02:00

Owner

Copy Link

Copy Source

fixes #525

What

An external http(s) channel-logo URL is now downloaded, decode-validated, and cached at save time — becoming byte-identical to an uploaded logo — instead of being fetched over HTTP at render time (the #511 behavior). A bad URL fails the save with a 400 in the editor, not a silent render-time drop at 3am. Follow-up to #511 (PR #518) and #502; supersedes the closed PR #522.

How (10 tasks, TDD, no schema change)

Layer	Change
RemoteImageDecodeBudget (Core, pure)	Extracted #511's decode budget (product w×h×frames ≤ 50 MP, ≤ 600 frames) so it's shared
IRemoteImageValidator / RemoteImageValidator (Infra)	Decode + budget-validate; Core interface throws-on-invalid (no ImageSharp type crosses Core), render path uses a static method returning the Image
IRemoteLogoCacher (Infra)	fetch (IRemoteImageFetcher, #511) → validate → cache (IImageCache) → content-hash name, or a BaseError
UpdateChannelHandler + both create handlers	Resolve a URL logo → cache name before persist; a cacher Left fails the save, nothing persisted
UploadArtworkHandler	Same decode budget applied — closes the pre-existing unchecked-upload gap (one rule: anything entering the logo cache is budget-checked)
WatermarkSelector	A leftover URL logo → warning + no on-screen bug, never a render-time fetch
ExternalLogoMigratorService (startup)	One-time, fail-open conversion of existing URL rows; failures leave the row + warn; idempotent; all-or-nothing on cancel
SPA ChannelEditScreen	Preview works for a saved logo; stale "URL can't drive the bug" copy removed; save 400 shown inline
Docs	decisions.md lifecycle record graphics.channel-logo-caching; channels.md; api-conventions.md

Downstream (M3U/XMLTV/SPA mapper/render) needs no change — a saved logo is a cache name, so all consumers resolve /iptv/logos/… exactly as for an upload.

Why not a formal supersession of #511/#502

graphics.channel-logo-caching is a new active record with supersedes: none: it narrows #511 (whose bounded-fetch primitive stays active for YAML image: graphics elements) and #502 (whose "pass-through" still describes client consumers), rather than reversing them. Content-hash naming (not a GUID) reuses the existing cache convention and makes re-adding an unchanged URL a natural no-op.

Deliberately deferred (in the decision record)

• No refresh button / staleness tracking — re-enter the URL.
• SSRF unchanged — admin-supplied URLs, redirect cap 3, no host denylist (inherits #511's stance). The trigger narrows: render-time (any tuning client) → admin-save + startup, strictly fewer moments.

Review

• 10 tasks, each independently reviewed clean (cold, fresh-context per task).
• Whole-branch final review on Fable: MERGEABLE @ 6d5f6b24. No Blocker/High. Two flagged items fixed in 5530d91a: a migration host-crash guard (a mid-migration DbUpdateConcurrencyException no longer trips StopHost) and a multi-error-400 regression (errors.Join() restored, + regression test). Three carried Minors adjudicated acceptable-defer.

Verification

• Full solution 4,079 tests + web 891 green; Architecture 5/5 (Core stays ImageSharp-free); build 0 warnings.
• Negative-controlled the decode caps and the upload budget (bomb test pins the budget message, not the content-type sniff); the APNG header-lie regression is preserved.
• Format gate + BOM clean across 25 changed .cs; OpenAPI unchanged (no controllers touched); decisions validator OK + catalog regenerated.

⚠️ Not run: local live-E2E of the download write-path

Per the write-path convention I'd normally run scripts/e2e-local.sh and exercise a real channel save with an external logo URL end-to-end (download → preview → M3U emits /iptv/logos/, and a bad URL 400s). I did not run that local live-E2E. The behavior is covered by unit + integration tests, the golden nets, and CI's functional-E2E (curl contracts), but not by an end-to-end run against a live remote host. Flagging it honestly as the one recommended manual check before merge.

Done-when

• URL logo → fetched, decode-validated, cached; Artwork.Path = hash; bad URL rejects the save (400) with a specific reason
• Shared RemoteImageDecodeBudget extracted; applied to the save path and UploadArtworkHandler
• Startup migration converts existing URL rows; failures left intact + warned; idempotent
• WatermarkSelector no longer treats a URL as renderable (→ warning, no fetch, no bug)
• SPA: preview works for a saved logo; stale copy removed; inline error on rejected save
• Docs updated in-PR (decisions lifecycle record, channels, api-conventions)
• Adversarial review passed (final whole-branch review MERGEABLE; fixes folded in)

🤖 Generated with Claude Code

fixes #525 ## What An external `http(s)` channel-logo URL is now **downloaded, decode-validated, and cached at save time** — becoming byte-identical to an uploaded logo — instead of being fetched over HTTP at render time (the #511 behavior). A bad URL fails the save with a **400** in the editor, not a silent render-time drop at 3am. Follow-up to #511 (PR #518) and #502; supersedes the closed PR #522. ## How (10 tasks, TDD, no schema change) | Layer | Change | |---|---| | `RemoteImageDecodeBudget` (Core, pure) | Extracted #511's decode budget (product `w×h×frames ≤ 50 MP`, `≤ 600` frames) so it's shared | | `IRemoteImageValidator` / `RemoteImageValidator` (Infra) | Decode + budget-validate; Core interface throws-on-invalid (no ImageSharp type crosses Core), render path uses a static method returning the `Image` | | `IRemoteLogoCacher` (Infra) | fetch (`IRemoteImageFetcher`, #511) → validate → cache (`IImageCache`) → content-hash name, or a `BaseError` | | `UpdateChannelHandler` + both create handlers | Resolve a URL logo → cache name before persist; a cacher `Left` fails the save, nothing persisted | | `UploadArtworkHandler` | Same decode budget applied — closes the pre-existing unchecked-upload gap (one rule: anything entering the logo cache is budget-checked) | | `WatermarkSelector` | A leftover URL logo → warning + no on-screen bug, never a render-time fetch | | `ExternalLogoMigratorService` (startup) | One-time, fail-open conversion of existing URL rows; failures leave the row + warn; idempotent; all-or-nothing on cancel | | SPA `ChannelEditScreen` | Preview works for a saved logo; stale "URL can't drive the bug" copy removed; save 400 shown inline | | Docs | `decisions.md` lifecycle record `graphics.channel-logo-caching`; `channels.md`; `api-conventions.md` | Downstream (M3U/XMLTV/SPA mapper/render) needs **no change** — a saved logo is a cache name, so all consumers resolve `/iptv/logos/…` exactly as for an upload. ## Why not a formal supersession of #511/#502 `graphics.channel-logo-caching` is a new active record with `supersedes: none`: it **narrows** #511 (whose bounded-fetch primitive stays active for YAML `image:` graphics elements) and #502 (whose "pass-through" still describes client consumers), rather than reversing them. Content-hash naming (not a GUID) reuses the existing cache convention and makes re-adding an unchanged URL a natural no-op. ## Deliberately deferred (in the decision record) - **No refresh button / staleness tracking** — re-enter the URL. - **SSRF unchanged** — admin-supplied URLs, redirect cap 3, no host denylist (inherits #511's stance). The trigger *narrows*: render-time (any tuning client) → admin-save + startup, strictly fewer moments. ## Review - **10 tasks, each independently reviewed clean** (cold, fresh-context per task). - **Whole-branch final review on Fable: `MERGEABLE @ 6d5f6b24`.** No Blocker/High. Two flagged items **fixed in `5530d91a`**: a migration host-crash guard (a mid-migration `DbUpdateConcurrencyException` no longer trips `StopHost`) and a multi-error-400 regression (`errors.Join()` restored, + regression test). Three carried Minors adjudicated acceptable-defer. ## Verification - Full solution **4,079 tests** + web **891** green; Architecture 5/5 (Core stays ImageSharp-free); build 0 warnings. - Negative-controlled the decode caps and the upload budget (bomb test pins the *budget* message, not the content-type sniff); the APNG header-lie regression is preserved. - Format gate + BOM clean across 25 changed `.cs`; OpenAPI unchanged (no controllers touched); decisions validator OK + catalog regenerated. ## ⚠️ Not run: local live-E2E of the download write-path Per the write-path convention I'd normally run `scripts/e2e-local.sh` and exercise a real channel save with an external logo URL end-to-end (download → preview → M3U emits `/iptv/logos/`, and a bad URL 400s). **I did not run that local live-E2E.** The behavior is covered by unit + integration tests, the golden nets, and CI's functional-E2E (curl contracts), but not by an end-to-end run against a live remote host. Flagging it honestly as the one recommended manual check before merge. ## Done-when - [x] URL logo → fetched, decode-validated, cached; `Artwork.Path` = hash; bad URL rejects the save (400) with a specific reason - [x] Shared `RemoteImageDecodeBudget` extracted; applied to the save path **and** `UploadArtworkHandler` - [x] Startup migration converts existing URL rows; failures left intact + warned; idempotent - [x] `WatermarkSelector` no longer treats a URL as renderable (→ warning, no fetch, no bug) - [x] SPA: preview works for a saved logo; stale copy removed; inline error on rejected save - [x] Docs updated in-PR (decisions lifecycle record, channels, api-conventions) - [x] Adversarial review passed (final whole-branch review MERGEABLE; fixes folded in) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 15 commits 2026-07-21 12:19:18 +02:00

docs(spec): external channel-logo URLs become download-on-save ... 2557760476

Design for reversing #511's render-time fetch: an external logo URL is
downloaded once at save time into the artwork cache (content-hash name,
same as an upload), validated against #511's decode budgets, and stored
so nothing downstream knows it came from a URL. Bad URL fails in the
editor, not at 3am. No refresh button — re-add the URL. One-time startup
migration converts existing URL rows; failures leave the row and warn.

refs #511 #502

docs(spec): resolve open questions — fold in upload budget check, keep fetcher namespace ... 9f99156e88

refs #511

docs(plan): external-logo download-on-save implementation plan ... c75db91b63

10 tasks, TDD, no schema change. Extracts RemoteImageDecodeBudget (Core) +
RemoteImageValidator (Infra), adds IRemoteLogoCacher, wires the 3 channel
handlers + UploadArtworkHandler, a startup migration for existing URL rows,
and the SPA preview/copy changes.

refs #525

docs(plan): resolve IRemoteImageValidator layering (Core has no ImageSharp) ... e450d4983c

Core interface returns Task (throws-on-invalid); render path uses a static
Infra method returning Image. Drops the GraphicsEngine threading. refs #525

refactor(525): extract RemoteImageDecodeBudget from ImageElementBase f795db0731

refactor(525): extract RemoteImageValidator; render path delegates to static 1f98e0345d

feat(525): add RemoteLogoCacher (fetch + validate + cache a logo URL) 3bde23808b

feat(525): download external-url logo on channel update 8dbd5a0e6d

feat(525): download external-url logo on channel create + create-from-lineup ... 70f6b32552

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

feat(525): budget-check direct artwork uploads (close the upload gap) 0df6b629ce

feat(525): render path no longer fetches a URL logo; degrades to no bug a55928d69c

feat(525): startup migration converts existing URL logo rows to cache 062d8d2b22

feat(525): SPA previews saved logos, drops stale external-URL copy a96b69b7b9

docs(525): record download-on-save decision; update channels + api-conventions ... 6d5f6b2489

New lifecycle record graphics.channel-logo-caching (supersedes: none — narrows
#511/#502 rather than reversing them; #511's bounded-fetch primitive stays active
for YAML image elements). Regenerated the active catalog. channels.md + api-
conventions.md updated for the new save-time download + 400s.

fix(525): address final-review findings (migration host-crash guard, multi-error 400s) ... 5530d91add

- Medium-1: wrap ExternalLogoMigratorService.ExecuteAsync in try/catch — a DB
  exception (e.g. a channel deleted mid-migration -> DbUpdateConcurrencyException)
  no longer trips BackgroundServiceExceptionBehavior.StopHost and kills the app;
  it logs and self-heals on the next boot. Caller-cancel path handled separately.
- Low-2: CreateChannelHandler/UpdateChannelHandler validation failure now returns
  errors.Join() (all accumulated errors) not errors.Head (first only), restoring
  the repo-wide convention; regression test added.
- Low-4: corrected the Startup registration comment (migrator self-awaits
  WaitForDatabase; order is not load-bearing).

Final whole-branch review: MERGEABLE @ 6d5f6b24 (fable). Carried Minors adjudicated
acceptable-defer.

timothy commented 2026-07-21 12:19:37 +02:00

Author

Owner

Copy Link

Copy Source

Independent review

Review-verdict: MERGEABLE @ 5530d91add9e18c8dd5d56c352fb1fdd5095516c

Whole-branch final review ran on Fable against 6d5f6b24 and returned MERGEABLE — no Blocker/High. It flagged two pre-merge polish items, both folded into 5530d91a (the current head):

• Medium — ExternalLogoMigratorService.ExecuteAsync could crash the whole host: a mid-migration DbUpdateConcurrencyException (e.g. a channel deleted while its logo is downloading) escaped ExecuteAsync, and the default BackgroundServiceExceptionBehavior.StopHost would exit the process. Now wrapped in try/catch — it logs and self-heals on the next boot; caller-cancel handled separately (no partial persist).
• Low — my handler refactor had regressed multi-error validation 400s to first-error-only (errors.Head); restored errors.Join() (the repo-wide convention) + added a regression test.

Since 5530d91a is new code the final reviewer didn't see, I re-verified it directly rather than re-running the full cold review for two review-directed changes: build clean, 138/138 on the affected suites (incl. the new multi-error test), format/BOM clean. The three carried Minors were adjudicated acceptable-defer by the final review (persistence structurally impossible on the fail path; no-op SaveChanges + bounded table load; transitional SPA preview cosmetic — the last is recorded as the accepted residual in the decision).

Per the process: 10 tasks, each independently reviewed clean in fresh context, then this whole-branch capstone. The one gap I'm not papering over is the local live-E2E of the download write-path (see the PR body) — flagged as the recommended manual check before merge.

## Independent review `Review-verdict: MERGEABLE @ 5530d91add9e18c8dd5d56c352fb1fdd5095516c` Whole-branch final review ran on Fable against `6d5f6b24` and returned **MERGEABLE** — no Blocker/High. It flagged two pre-merge polish items, both **folded into `5530d91a`** (the current head): - **Medium** — `ExternalLogoMigratorService.ExecuteAsync` could crash the whole host: a mid-migration `DbUpdateConcurrencyException` (e.g. a channel deleted while its logo is downloading) escaped `ExecuteAsync`, and the default `BackgroundServiceExceptionBehavior.StopHost` would exit the process. Now wrapped in try/catch — it logs and self-heals on the next boot; caller-cancel handled separately (no partial persist). - **Low** — my handler refactor had regressed multi-error validation 400s to first-error-only (`errors.Head`); restored `errors.Join()` (the repo-wide convention) + added a regression test. Since `5530d91a` is new code the final reviewer didn't see, I re-verified it directly rather than re-running the full cold review for two review-directed changes: build clean, 138/138 on the affected suites (incl. the new multi-error test), format/BOM clean. The three carried Minors were adjudicated **acceptable-defer** by the final review (persistence structurally impossible on the fail path; no-op `SaveChanges` + bounded table load; transitional SPA preview cosmetic — the last is recorded as the accepted residual in the decision). Per the process: 10 tasks, each independently reviewed clean in fresh context, then this whole-branch capstone. The one gap I'm **not** papering over is the local live-E2E of the download write-path (see the PR body) — flagged as the recommended manual check before merge.

timothy referenced this pull request 2026-07-21 12:20:51 +02:00

External channel-logo URLs: download + cache at save time instead of fetching at render time #525

timothy commented 2026-07-21 12:46:33 +02:00

Author

Owner

Copy Link

Copy Source

Live-E2E ran — and caught a doc bug

The write-path live-E2E I'd flagged as skipped is now done (scripts/e2e-local.sh, fresh instance, a locally-served PNG as the "good" external URL, verified via curl — never a browser tab):

Check	Result
PUT channel with external logo URL	200; Artwork.Path → iptv/logos/7B8A70C6… (a cache hash), isExternalUrl=false — downloaded + cached
M3U output	emits …/iptv/logos/<hash> — the cached URL, not the raw external one
Serve the cached logo back	200, real PNG bytes
Unreachable URL (127.0.0.1:9)	422 "Could not download logo … Connection refused"
Non-image URL (text/html)	422 "Remote image … returned content type 'text/html'"
Channel logo after a rejected save	unchanged — no partial mutation

The catch: these reject as 422 (BaseError → ValidationProblemDetails via ToErrorResult()), not the 400 I'd written in the docs/PR (400 here still means model-binding failure). Corrected channels.md, api-conventions.md, and the graphics.channel-logo-caching decision record + catalog in c7949c01. This is exactly the class of thing only a live run surfaces — glad the "finish the plan" instruction sent me to run it.

Review-verdict: MERGEABLE @ c7949c01ca99ae32f6dd814b7d593371d3570405

c7949c01 is a docs-only correction (422 wording) on top of the reviewed 5530d91a; no code changed, so the whole-branch verdict carries forward. CI re-running on the new head.

## Live-E2E ran — and caught a doc bug The write-path live-E2E I'd flagged as skipped is now **done** (`scripts/e2e-local.sh`, fresh instance, a locally-served PNG as the "good" external URL, verified via curl — never a browser tab): | Check | Result | |---|---| | `PUT` channel with external logo URL | 200; `Artwork.Path` → `iptv/logos/7B8A70C6…` (a cache hash), `isExternalUrl=false` — **downloaded + cached** | | M3U output | emits `…/iptv/logos/<hash>` — the **cached** URL, not the raw external one | | Serve the cached logo back | 200, real PNG bytes | | Unreachable URL (`127.0.0.1:9`) | **422** `"Could not download logo … Connection refused"` | | Non-image URL (text/html) | **422** `"Remote image … returned content type 'text/html'"` | | Channel logo after a rejected save | **unchanged** — no partial mutation | **The catch:** these reject as **422** (`BaseError` → `ValidationProblemDetails` via `ToErrorResult()`), not the **400** I'd written in the docs/PR (400 here still means model-binding failure). Corrected `channels.md`, `api-conventions.md`, and the `graphics.channel-logo-caching` decision record + catalog in `c7949c01`. This is exactly the class of thing only a live run surfaces — glad the "finish the plan" instruction sent me to run it. `Review-verdict: MERGEABLE @ c7949c01ca99ae32f6dd814b7d593371d3570405` `c7949c01` is a docs-only correction (422 wording) on top of the reviewed `5530d91a`; no code changed, so the whole-branch verdict carries forward. CI re-running on the new head.

timothy force-pushed feat/525-external-logo-download-on-save from c7949c01ca to aa76994825 2026-07-21 13:21:03 +02:00 Compare

timothy commented 2026-07-21 13:21:59 +02:00

Author

Owner

Copy Link

Copy Source

Rebased onto the #520/#521/#527 main

Review-verdict: MERGEABLE @ aa769948 (carries forward the whole-branch review; the rebase is docs-only integration, no code change).

Main advanced under this PR (the full decision-corpus lifecycle migration + startup rewrite landed, #527), so the merge went mergeable:false and I rebased. What the rebase touched:

• decisions.md / catalog conflict — both my graphics.channel-logo-caching record and main's new startup.parallel-orientation record appended at the same spot. Resolved by keeping both; regenerated docs/decisions/README.md from source (never hand-merged). Cross-referenced the now-keyed siblings in the narrowing prose — ffmpeg.remote-image-fetcher-bounded (#511) and ffmpeg.external-logo-graphics-engine (#502) — and confirmed supersedes: none is still the right call (both stay active; #525 narrows, doesn't reverse).
• No code conflicts — the 13 replayed commits applied cleanly; the c7949c01 422 fix re-applied on top.

Re-verified on the rebased head aa769948:

• Full solution 4,080 tests + Architecture 5/5 green; build 0 warnings.
• dotnet format (folder) clean, no BOMs across 25 .cs.
• decisions-validate: OK and kickoff-guard: OK — the two new #527 CI checks pass (my docs don't reintroduce a #237-as-live-state phrasing).

Live-E2E remains done (see the prior comment): good URL → cached /iptv/logos/<hash>, M3U emits the cached URL, unreachable/non-image → 422. CI re-running on aa769948; holding the merge for explicit go-ahead rather than auto-firing.

## Rebased onto the #520/#521/#527 main `Review-verdict: MERGEABLE @ aa769948` (carries forward the whole-branch review; the rebase is docs-only integration, no code change). Main advanced under this PR (the full decision-corpus lifecycle migration + startup rewrite landed, #527), so the merge went `mergeable:false` and I rebased. What the rebase touched: - **`decisions.md` / catalog conflict** — both my `graphics.channel-logo-caching` record and main's new `startup.parallel-orientation` record appended at the same spot. Resolved by keeping **both**; regenerated `docs/decisions/README.md` from source (never hand-merged). Cross-referenced the now-keyed siblings in the narrowing prose — `ffmpeg.remote-image-fetcher-bounded` (#511) and `ffmpeg.external-logo-graphics-engine` (#502) — and confirmed `supersedes: none` is still the right call (both stay `active`; #525 narrows, doesn't reverse). - **No code conflicts** — the 13 replayed commits applied cleanly; the `c7949c01` 422 fix re-applied on top. Re-verified on the rebased head `aa769948`: - Full solution **4,080** tests + Architecture 5/5 green; build 0 warnings. - `dotnet format` (folder) clean, no BOMs across 25 `.cs`. - **`decisions-validate: OK`** and **`kickoff-guard: OK`** — the two new #527 CI checks pass (my docs don't reintroduce a #237-as-live-state phrasing). Live-E2E remains done (see the prior comment): good URL → cached `/iptv/logos/<hash>`, M3U emits the cached URL, unreachable/non-image → **422**. CI re-running on `aa769948`; holding the merge for explicit go-ahead rather than auto-firing.

timothy merged commit 6b1bd9cf4b into main 2026-07-21 15:36:16 +02:00

timothy deleted branch feat/525-external-logo-download-on-save 2026-07-21 15:36:17 +02:00

timothy referenced this issue from a commit 2026-07-21 15:36:18 +02:00

Merge pull request 'feat(525): external channel-logo URLs download + cache at save time' (#528) from feat/525-external-logo-download-on-save into main

timothy referenced this pull request 2026-07-21 15:37:11 +02:00

External channel-logo URLs: download + cache at save time instead of fetching at render time #525

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#528