Add a media storage abstraction (#131) #155

Merged

claude-bot merged 1 commit from feat/media-storage-abstraction into main

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

claude-bot commented

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

Contributor

Closes #131. Third capture-phase issue of the v2.0.0 Historical Explorer epic (#22).

Problem

Every cached image the app writes — live radar PNG/GIFs, assembled SPC plots, alert-time radar snapshots and the periodic frames (#130) — went through direct pathlib calls scattered across radar.py, alert_processor.py, radar_frames.py, retention.py and api/media.py. New app/services/storage.py is the single seam they all go through instead.

No object-storage backend is implemented, deliberately. Media volume today is small; paying for B2/S3 before the volume justifies it is premature. LocalDiskBackend is the only implementation.

The interface is async — revising this issue's own guidance

This issue's task list said "keep it synchronous-disk-simple". I wrote that, and it was wrong.

A synchronous interface would have defeated the entire purpose. An object backend does network I/O; behind a sync interface that blocks the shared asyncio event loop that also runs alert polling, the Discord bot and the Matrix client — the exact hazard CLAUDE.md warns about and that #128 was parked over. Avoiding it later would mean making every call site async at that point, i.e. the "edit every call site" cost this abstraction exists to prevent.

The cost of async now is close to zero: every real consumer (get_radar_image, fetch_spc_outlook_png / _gif, run_retention_cleanup) is already a coroutine. Only the private helpers being replaced outright were sync.

Design notes

age_seconds() and iter_keys() are not speculative — without them the TTL checks and both retention sweeps cannot leave Path.
iter_keys() is deliberately NON-recursive. That is what still guarantees prune_spc_radar_cache only matches root-level *.cache and never wanders into alert_snapshots/, which a different sweep owns. The old code got this from a non-recursive glob; the guarantee now lives in the contract.
url_for() returns None for disk. media.py falls back to serving bytes itself; a presigned-URL backend fills this in.
Key validation moved into the backend (absolute paths, .., backslashes, depth), so every consumer inherits traversal defence rather than re-implementing it — api/media.py had done so three times over. Those regexes stay as defence in depth and the 404 behaviour is unchanged.
Everything goes through get_media_storage(), including retention, which passes the Settings it was handed rather than constructing a LocalDiskBackend inline. Inline construction works today and would silently pin retention to disk the day a backend swap lands.

On-disk layout is unchanged — nothing to migrate

Keys are paths relative to radar_cache_dir. Verified empirically rather than assumed:

live radar key : KLSX_png.png | KLSX_gif.gif
spc cache key  : spc220_1C_categorical_2026-07-28_conus_dpi100.cache
snapshot key   : alert_snapshots/NWS_KLSX_TO_W_0042__loc.png
on-disk layout : ['KLSX_png.png', 'alert_snapshots/a.png', 'spckey.cache']
root keys      : ['KLSX_png.png', 'spckey.cache']     <- subdirectory excluded

Production's existing cache files are still found by the same keys.

#130 invariant preserved

The two-way retention protection survives: the orphan sweep still treats any filename referenced by a live AlertRadarFrame.path as referenced, and frame cleanup still leaves files still pointed at by a surviving SentAlert.radar_snapshot_path. Both regression tests pass unchanged.

Verification

Step	Result
`ruff check .`	passed
`python -m compileall app`	OK
SQLite bulk tier	838 passed (up from 815; 23 new storage tests)
`alembic upgrade head` on fresh Postgres 16	clean
Postgres integration tier	4 passed
On-disk layout check	unchanged, verified above

New tests/test_media_storage.py covers the backend directly: round-trips, missing keys, age_seconds, non-recursive iter_keys with suffix filtering, url_for, subdirectory creation, and every invalid-key form (../escape, absolute, backslash, empty, too-deep, dot segments).

Unblocks

The deferred task on #130 — routing radar frame writes through the abstraction — is done here.

🤖 Generated with Claude Code

Closes #131. Third capture-phase issue of the v2.0.0 Historical Explorer epic (#22). ## Problem Every cached image the app writes — live radar PNG/GIFs, assembled SPC plots, alert-time radar snapshots and the periodic frames (#130) — went through direct `pathlib` calls scattered across `radar.py`, `alert_processor.py`, `radar_frames.py`, `retention.py` and `api/media.py`. New `app/services/storage.py` is the single seam they all go through instead. **No object-storage backend is implemented, deliberately.** Media volume today is small; paying for B2/S3 before the volume justifies it is premature. `LocalDiskBackend` is the only implementation. ## The interface is async — revising this issue's own guidance This issue's task list said "keep it synchronous-disk-simple". I wrote that, and it was wrong. A synchronous interface would have defeated the entire purpose. An object backend does network I/O; behind a sync interface that blocks the shared asyncio event loop that also runs alert polling, the Discord bot and the Matrix client — the exact hazard `CLAUDE.md` warns about and that #128 was parked over. Avoiding it later would mean making every call site async at that point, i.e. the "edit every call site" cost this abstraction exists to prevent. The cost of async now is close to zero: every real consumer (`get_radar_image`, `fetch_spc_outlook_png` / `_gif`, `run_retention_cleanup`) is already a coroutine. Only the private helpers being replaced outright were sync. ## Design notes - **`age_seconds()` and `iter_keys()` are not speculative** — without them the TTL checks and both retention sweeps cannot leave `Path`. - **`iter_keys()` is deliberately NON-recursive.** That is what still guarantees `prune_spc_radar_cache` only matches root-level `*.cache` and never wanders into `alert_snapshots/`, which a different sweep owns. The old code got this from a non-recursive `glob`; the guarantee now lives in the contract. - **`url_for()` returns `None` for disk.** `media.py` falls back to serving bytes itself; a presigned-URL backend fills this in. - **Key validation moved into the backend** (absolute paths, `..`, backslashes, depth), so every consumer inherits traversal defence rather than re-implementing it — `api/media.py` had done so three times over. Those regexes stay as defence in depth and the 404 behaviour is unchanged. - **Everything goes through `get_media_storage()`, including retention**, which passes the `Settings` it was handed rather than constructing a `LocalDiskBackend` inline. Inline construction works today and would silently pin retention to disk the day a backend swap lands. ## On-disk layout is unchanged — nothing to migrate Keys are paths relative to `radar_cache_dir`. Verified empirically rather than assumed: ``` live radar key : KLSX_png.png | KLSX_gif.gif spc cache key : spc220_1C_categorical_2026-07-28_conus_dpi100.cache snapshot key : alert_snapshots/NWS_KLSX_TO_W_0042__loc.png on-disk layout : ['KLSX_png.png', 'alert_snapshots/a.png', 'spckey.cache'] root keys : ['KLSX_png.png', 'spckey.cache'] <- subdirectory excluded ``` Production's existing cache files are still found by the same keys. ## #130 invariant preserved The two-way retention protection survives: the orphan sweep still treats any filename referenced by a live `AlertRadarFrame.path` as referenced, and frame cleanup still leaves files still pointed at by a surviving `SentAlert.radar_snapshot_path`. Both regression tests pass unchanged. ## Verification | Step | Result | |---|---| | `ruff check .` | passed | | `python -m compileall app` | OK | | SQLite bulk tier | **838 passed** (up from 815; 23 new storage tests) | | `alembic upgrade head` on fresh Postgres 16 | clean | | Postgres integration tier | 4 passed | | On-disk layout check | unchanged, verified above | New `tests/test_media_storage.py` covers the backend directly: round-trips, missing keys, `age_seconds`, non-recursive `iter_keys` with suffix filtering, `url_for`, subdirectory creation, and every invalid-key form (`../escape`, absolute, backslash, empty, too-deep, dot segments). ## Unblocks The deferred task on #130 — routing radar frame writes through the abstraction — is done here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

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

claude-bot added 1 commit

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

Add a media storage abstraction (#131 )

CI / test (pull_request) Successful in 4m18s

Details

5c8f03a4cf

Every cached image the app writes -- live radar PNG/GIFs, assembled SPC plots,
alert-time radar snapshots and the periodic frames (#130) -- went through
direct pathlib calls scattered across radar.py, alert_processor.py,
radar_frames.py, retention.py and api/media.py. New app/services/storage.py is
the single seam they all go through, so a Backblaze B2 / S3 backend can later
be one new class plus a config change rather than an edit to every call site.

No object-storage backend is implemented, deliberately: media volume today is
small and paying for it before the volume justifies it is premature.

The interface is ASYNC, revising this issue's own "keep it synchronous-disk-
simple" guidance. A sync interface would have defeated the purpose -- an object
backend does network I/O, and behind a sync interface that blocks the shared
event loop running alert polling, the Discord bot and the Matrix client.
Avoiding that later would mean making every call site async, i.e. exactly the
edit-everything cost this abstraction exists to prevent. The cost now is near
zero because every real consumer (get_radar_image, fetch_spc_outlook_png/gif,
run_retention_cleanup) is already a coroutine; only the private helpers being
replaced were sync.

age_seconds() and iter_keys() are not speculative -- without them the TTL
checks and the two retention sweeps cannot leave Path. iter_keys is
deliberately NON-recursive, which is what still guarantees prune_spc_radar_cache
only matches root-level *.cache and never wanders into alert_snapshots/.
url_for() returns None for disk; media.py falls back to serving bytes itself,
and a presigned-URL backend fills it in.

Key validation (absolute paths, .., backslashes, depth) now lives in the
backend, so every consumer inherits traversal defence instead of
re-implementing it -- api/media.py had done so three times over. Those regexes
stay as defence in depth.

The on-disk layout is unchanged and nothing needs migrating: keys are paths
relative to radar_cache_dir, verified to still produce KLSX_png.png,
{key}.cache at the root, and alert_snapshots/{name}.png.

Consumers go through get_media_storage(), including retention -- which passes
the Settings it was handed rather than constructing a LocalDiskBackend inline,
so it follows a backend swap instead of being silently pinned to disk.

The two-way retention protection from #130 is preserved: the orphan sweep still
treats live AlertRadarFrame paths as referenced, and frame cleanup still leaves
files owned by a surviving SentAlert.radar_snapshot_path.

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

claude-bot merged commit 16c4b2c71c into main

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

claude-bot referenced this pull request 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 referenced this pull request

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

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