Media storage abstraction (disk backend now, object storage later)

claude-bot commented

2026-07-27 20:06:42 +00:00

Contributor

Parent: #22. Resolves the "object storage as first target or later enhancement?" open question — later, but behind an interface added now.

Media currently writes to settings.radar_cache_dir through direct filesystem calls scattered across app/services/radar.py (live radar cache, SPC plot-220 frames, assembled SPC GIFs) and app/services/alert_processor.py (alert_snapshots/). Serving happens in app/api/media.py.

Change

A small storage interface — put(key, bytes, content_type), get(key), exists(key), delete(key), url_for(key) — with a LocalDiskBackend as the only implementation.

No Backblaze/S3 work in v2.0.0. The point is that adding a backend later is a config change plus one new class, not an edit to every call site. Media volume today is radar PNGs and small; paying for object storage before the volume justifies it is premature, but so is scattering more direct filesystem calls.

Tasks

Define the interface and LocalDiskBackend; keep the existing on-disk layout so nothing needs migrating.
Move radar cache, SPC frames/GIFs, and alert snapshots onto it.
Route app/api/media.py reads through get / url_for.
Move prune_spc_radar_cache and _cleanup_alert_radar_snapshots onto delete plus enumeration.
Keep it synchronous-disk-simple; do not introduce an external dependency in the alert dispatch path.

Explicitly out of scope: implementing a B2/S3 backend.

Parent: #22. Resolves the "object storage as first target or later enhancement?" open question — **later**, but behind an interface added now. Media currently writes to `settings.radar_cache_dir` through direct filesystem calls scattered across `app/services/radar.py` (live radar cache, SPC plot-220 frames, assembled SPC GIFs) and `app/services/alert_processor.py` (`alert_snapshots/`). Serving happens in `app/api/media.py`. ## Change A small storage interface — `put(key, bytes, content_type)`, `get(key)`, `exists(key)`, `delete(key)`, `url_for(key)` — with a `LocalDiskBackend` as the only implementation. **No Backblaze/S3 work in v2.0.0.** The point is that adding a backend later is a config change plus one new class, not an edit to every call site. Media volume today is radar PNGs and small; paying for object storage before the volume justifies it is premature, but so is scattering more direct filesystem calls. ## Tasks - [ ] Define the interface and `LocalDiskBackend`; keep the existing on-disk layout so nothing needs migrating. - [ ] Move radar cache, SPC frames/GIFs, and alert snapshots onto it. - [ ] Route `app/api/media.py` reads through `get` / `url_for`. - [ ] Move `prune_spc_radar_cache` and `_cleanup_alert_radar_snapshots` onto `delete` plus enumeration. - [ ] Keep it synchronous-disk-simple; do not introduce an external dependency in the alert dispatch path. **Explicitly out of scope:** implementing a B2/S3 backend.

claude-bot added this to the v2.0.0 - Historical Weather Event Explorer milestone

2026-07-27 20:06:42 +00:00

claude-bot added the

v2.0.0

type:feature

labels

2026-07-27 20:06:42 +00:00

claude-bot referenced this issue

2026-07-27 20:09:21 +00:00

Historical Weather Event Explorer #22

claude-bot referenced this issue

2026-07-27 23:21:36 +00:00

Capture radar snapshots for every alert, with an independent retention window #130

rbrooks referenced this issue from a commit

2026-07-27 23:49:50 +00:00

Capture periodic radar frames for active alerts (#130)

claude-bot referenced this issue

2026-07-27 23:50:15 +00:00

Capture periodic radar frames for active alerts (#130) #144

claude-bot referenced this issue

2026-07-28 00:00:53 +00:00

Capture radar snapshots for every alert, with an independent retention window #130

claude-bot commented

2026-07-28 04:54:17 +00:00

Author

Contributor

Picking this up on branch feat/media-storage-abstraction.

Revising one instruction in this issue's own task list — the "keep it synchronous-disk-simple" line, which I wrote. On reflection it is wrong, and following it would undermine the point of the issue.

The stated goal is that adding a B2/S3 backend later is "a config change plus one new class, not an edit to every call site." With a synchronous interface that is not true. A future object-storage backend does network I/O, and behind a sync interface that blocks the shared asyncio event loop — the exact failure mode CLAUDE.md warns about and that #128 was parked over. Avoiding it would mean making the call sites async at that point, i.e. editing every call site. The abstraction would have bought nothing.

So the interface is async from the start, implemented synchronously by the disk backend. The cost now is close to zero, because the real consumers are already async:

get_radar_image — async
fetch_spc_outlook_png / fetch_spc_outlook_gif — async
run_retention_cleanup — async

The only sync functions involved are the private helpers this issue replaces outright (_cache_path, _is_cache_valid, _cache_outcome, _spc_cache_path/_valid/_outcome/_read/_write) plus prune_spc_radar_cache, whose sole caller is async.

Interface

class MediaStorage(ABC):
    async def put(key, data, *, content_type=None) -> None
    async def get(key) -> bytes | None
    async def exists(key) -> bool
    async def delete(key) -> bool
    async def age_seconds(key) -> float | None      # every TTL check needs this
    async def iter_keys(prefix="", *, suffix="") -> list[str]   # both prune sweeps need this
    def url_for(key) -> str | None                  # None = no direct URL, serve via /media

age_seconds and iter_keys are not speculative — without them the TTL checks and the two retention sweeps cannot move off Path. url_for returns None for disk, which is the hook a presigned-URL backend fills in; media.py falls back to get when it is None.

Keys are paths relative to radar_cache_dir, so the on-disk layout is unchanged and nothing needs migrating: KLSX_png.png, spc220_1C_..._dpi100.cache, alert_snapshots/NWS_..._loc.png.

Bonus: centralising path-traversal defence

Today each consumer re-implements its own guard — a regex plus a resolve()-containment check, duplicated three times in media.py. The backend now owns key validation (reject absolute paths, .., backslashes; resolve-check containment on every operation), so every call site inherits it. The existing per-route checks stay as defence in depth rather than being the only guard.

Invariant that must survive

The mutual protection between the two retention sweeps (#130): _cleanup_alert_radar_snapshots treats any filename referenced by a live AlertRadarFrame.path as referenced, and _cleanup_alert_radar_frames leaves files still pointed at by SentAlert.radar_snapshot_path. The first frame deliberately shares its filename with the snapshot, so dropping either guard resurfaces that bug.

Picking this up on branch `feat/media-storage-abstraction`. **Revising one instruction in this issue's own task list** — the "keep it synchronous-disk-simple" line, which I wrote. On reflection it is wrong, and following it would undermine the point of the issue. The stated goal is that adding a B2/S3 backend later is "a config change plus one new class, not an edit to every call site." With a **synchronous** interface that is not true. A future object-storage backend does network I/O, and behind a sync interface that blocks the shared asyncio event loop — the exact failure mode `CLAUDE.md` warns about and that #128 was parked over. Avoiding it would mean making the call sites async at that point, i.e. editing every call site. The abstraction would have bought nothing. So the interface is **async** from the start, implemented synchronously by the disk backend. The cost now is close to zero, because the real consumers are already async: - `get_radar_image` — async - `fetch_spc_outlook_png` / `fetch_spc_outlook_gif` — async - `run_retention_cleanup` — async The only sync functions involved are the private helpers this issue replaces outright (`_cache_path`, `_is_cache_valid`, `_cache_outcome`, `_spc_cache_path/_valid/_outcome/_read/_write`) plus `prune_spc_radar_cache`, whose sole caller is async. ## Interface ```python class MediaStorage(ABC): async def put(key, data, *, content_type=None) -> None async def get(key) -> bytes | None async def exists(key) -> bool async def delete(key) -> bool async def age_seconds(key) -> float | None # every TTL check needs this async def iter_keys(prefix="", *, suffix="") -> list[str] # both prune sweeps need this def url_for(key) -> str | None # None = no direct URL, serve via /media ``` `age_seconds` and `iter_keys` are not speculative — without them the TTL checks and the two retention sweeps cannot move off `Path`. `url_for` returns `None` for disk, which is the hook a presigned-URL backend fills in; `media.py` falls back to `get` when it is `None`. Keys are paths relative to `radar_cache_dir`, so the **on-disk layout is unchanged and nothing needs migrating**: `KLSX_png.png`, `spc220_1C_..._dpi100.cache`, `alert_snapshots/NWS_..._loc.png`. ## Bonus: centralising path-traversal defence Today each consumer re-implements its own guard — a regex plus a `resolve()`-containment check, duplicated three times in `media.py`. The backend now owns key validation (reject absolute paths, `..`, backslashes; resolve-check containment on every operation), so every call site inherits it. The existing per-route checks stay as defence in depth rather than being the only guard. ## Invariant that must survive The mutual protection between the two retention sweeps (#130): `_cleanup_alert_radar_snapshots` treats any filename referenced by a live `AlertRadarFrame.path` as referenced, and `_cleanup_alert_radar_frames` leaves files still pointed at by `SentAlert.radar_snapshot_path`. The first frame deliberately shares its filename with the snapshot, so dropping either guard resurfaces that bug.

rbrooks referenced this issue from a commit

2026-07-28 05:26:36 +00:00

Add a media storage abstraction (#131)

claude-bot referenced this issue from a pull request that will close it,

2026-07-28 05:27:02 +00:00

Add a media storage abstraction (#131) #155

claude-bot referenced this issue from a commit

2026-07-28 05:36:21 +00:00

Merge pull request 'Add a media storage abstraction (#131)' (#155) from feat/media-storage-abstraction into main

claude-bot closed this issue

2026-07-28 05:36:21 +00:00

claude-bot commented

2026-07-28 05:36:41 +00:00

Author

Contributor

Done in #155 (merged to main). CI green in 4m19s.

Shipped: app/services/storage.py with an async MediaStorage contract and LocalDiskBackend. All five consumers migrated — radar.py (live radar + SPC cache + prune_spc_radar_cache), alert_processor.py, radar_frames.py, retention.py (both sweeps), and api/media.py (all three routes). No B2/S3 backend, as scoped.

On-disk layout verified unchanged, so production's existing cache needs no migration:

on-disk layout : ['KLSX_png.png', 'alert_snapshots/a.png', 'spckey.cache']
root keys      : ['KLSX_png.png', 'spckey.cache']     <- subdirectory excluded

Verified locally in Docker: ruff clean, compileall OK, 838 SQLite-tier tests (up from 815), migrations clean on Postgres 16, 4 Postgres-tier tests. The #130 two-way retention protection passes unchanged.

Three things worth carrying forward:

The async decision is the load-bearing one. Recorded in the PR and the commit: a sync interface would have meant editing every call site the day an object backend lands, which is precisely what this issue existed to avoid. If a future backend is added, it must not do blocking I/O — the contract is async so that it does not have to.
iter_keys() being non-recursive is a correctness guarantee, not a detail. It is what keeps prune_spc_radar_cache from reaching into alert_snapshots/, which a different sweep owns. Previously that came from a non-recursive glob; it is now part of the documented contract and should stay that way.
Go through get_media_storage(), never construct a backend inline. Retention passes its injected Settings through the factory for exactly this reason — an inline LocalDiskBackend works today and would silently keep that consumer on disk after a swap.

Also closes the deferred task on #130 (routing frame writes through the abstraction).

Done in #155 (merged to `main`). CI green in 4m19s. **Shipped:** `app/services/storage.py` with an async `MediaStorage` contract and `LocalDiskBackend`. All five consumers migrated — `radar.py` (live radar + SPC cache + `prune_spc_radar_cache`), `alert_processor.py`, `radar_frames.py`, `retention.py` (both sweeps), and `api/media.py` (all three routes). No B2/S3 backend, as scoped. **On-disk layout verified unchanged**, so production's existing cache needs no migration: ``` on-disk layout : ['KLSX_png.png', 'alert_snapshots/a.png', 'spckey.cache'] root keys : ['KLSX_png.png', 'spckey.cache'] <- subdirectory excluded ``` **Verified locally in Docker:** ruff clean, `compileall` OK, 838 SQLite-tier tests (up from 815), migrations clean on Postgres 16, 4 Postgres-tier tests. The #130 two-way retention protection passes unchanged. **Three things worth carrying forward:** 1. **The async decision is the load-bearing one.** Recorded in the PR and the commit: a sync interface would have meant editing every call site the day an object backend lands, which is precisely what this issue existed to avoid. If a future backend is added, it must not do blocking I/O — the contract is async so that it does not have to. 2. **`iter_keys()` being non-recursive is a correctness guarantee, not a detail.** It is what keeps `prune_spc_radar_cache` from reaching into `alert_snapshots/`, which a different sweep owns. Previously that came from a non-recursive `glob`; it is now part of the documented contract and should stay that way. 3. **Go through `get_media_storage()`, never construct a backend inline.** Retention passes its injected `Settings` through the factory for exactly this reason — an inline `LocalDiskBackend` works today and would silently keep that consumer on disk after a swap. Also closes the deferred task on #130 (routing frame writes through the abstraction).

claude-bot referenced this issue

2026-07-28 14:26:25 +00:00

Lightning cluster archive #132

claude-bot referenced this issue

2026-07-28 15:38:48 +00:00

Unify retention onto one 13-month horizon (#133) #157

claude-bot referenced this issue

2026-07-28 15:39:34 +00:00

Measure capture-table storage after a convective season and revisit the retention windows #158

claude-bot referenced this issue

2026-07-28 16:12:03 +00:00

CD deploys only to the dev host; the live instance is 6 days behind and manually managed #154