Notifier radar: make "animated" actually animate, and split the flag that doesn't mean what it says #184

Closed
opened 2026-08-19 20:54:07 +00:00 by claude-bot · 3 comments
Contributor

Split out of #182 item 3, which documented the bug but deliberately left it out of scope. Target: v2.0.0-rc3.

Three problems, not one

1. animated=True is a no-op. radar._fetch_radar (radar.py:99-105) accepts the animated parameter and never reads it — it unconditionally returns a static PNG from _fetch_noaa_static, or a RainViewer PNG tile on fallback. The "animated radar" feature has never produced an animation.

2. The result is mislabelled as GIF in three places. get_radar_image caches it with content_type="image/gif" (radar.py:88); discord_bot.py attaches it as radar.gif; signal.py base64-encodes it as data:image/gif;base64,.... All PNG bytes wearing a GIF label.

3. radar_animated doesn't gate animation — it gates whether radar is attached at all. In discord_bot.py:146, signal.py:113, matrix_bot.py:93, pushover.py:125 and webex.py:156 the flag wraps the entire radar-attachment block. A user who turns "animated radar" off silently loses their radar image entirely. The setting's name actively misleads.

Why this isn't just "fix the content type"

The primary radar source cannot animate. Probed mapservices.weather.noaa.gov/.../radar_base_reflectivity/MapServer?f=json: no timeInfo, capabilities are Map,Query,Data only. It serves "now" and has no historical frames — there is no time parameter to sweep.

RainViewer can. weather-maps.json returns ~13 past frames at 10-minute cadence spanning ~2 hours, each a tileable path. It is already a dependency (the static fallback, plus the dashboard's satellite/nowcast layers).

But RainViewer is currently used as a single fixed tile at zoom 6/256px, which compares badly with the NOAA export:

Source Resolution Framing
NOAA export (current still) 0.72 km/px centred on the location
RainViewer 1 tile @ z6/256 (current fallback) 1.91 km/px wherever it lands
RainViewer 2x2 stitched @ z7/512 0.48 km/px centred

Worth calling out on its own: the existing single-tile fallback is already badly framed. A location sits wherever it happens to fall inside its tile — St. Louis lands at fraction 0.96 across its zoom-6 tile, i.e. hard against the eastern edge, with most of the image showing area the user doesn't care about. That is a live defect in the fallback path today, independent of animation.

Decisions taken

Question Decision
What should "animated" do Real animation from RainViewer, stitched. Fetch the past-frame series, stitch 2x2 tiles at z7/512, crop to a window centred on the location. Ends up sharper than today's still (0.48 vs 0.72 km/px) and correctly framed. Falls back to the NOAA still — correctly labelled PNG — when RainViewer has no frames.
The misnamed flag Split into two. radar_enabled (attach radar at all) + radar_animated (animate where the source allows). Migration must default radar_enabled from each channel's existing radar_animated value, so no channel changes behaviour on upgrade.
Release Fold into rc3, so v2.0.0 ships with the notifier radar path fully correct.

Work

  • RainViewer frame-series fetch (list past frames, not just the latest).
  • Tile stitching + centred crop, replacing the single-tile fetch. Fixes the framing defect for the static fallback too, not only the animation.
  • Real GIF assembly via the existing assemble_gif (already disposal=2 after #182), with each frame basemap-composited using app.services.basemap from #182 — RainViewer tiles are transparent, so they need the same treatment.
  • Correct content type and filename everywhere the result is attached: get_radar_image's cache content_type, Discord's radar.gif, Signal's data:image/gif. The label must follow what was actually produced, since the animation can fall back to a still.
  • Migration adding radar_enabled, backfilled from radar_animated; update the 5 notifier gates, the channel schemas, config_io import/export, and the settings UI.
  • Tests: animation produces >1 GIF frame; fallback produces a correctly-labelled still; stitched output is centred on the location; the migration preserves per-channel behaviour; frames are opaque after compositing.

Watch items

  • Outbound load. A stitched animation is (tiles x frames) requests — 2x2 by ~13 frames is ~52 tile GETs per location per refresh. The 5-minute radar cache absorbs most of this, but confirm the cache key covers the animated variant (#182 added the basemap dimension to it) and consider capping frame count.
  • RainViewer terms for this request volume, given it is a free public API already used elsewhere in the app.
Split out of #182 item 3, which documented the bug but deliberately left it out of scope. Target: **v2.0.0-rc3**. ## Three problems, not one **1. `animated=True` is a no-op.** `radar._fetch_radar` (`radar.py:99-105`) accepts the `animated` parameter and never reads it — it unconditionally returns a static PNG from `_fetch_noaa_static`, or a RainViewer PNG tile on fallback. The "animated radar" feature has never produced an animation. **2. The result is mislabelled as GIF in three places.** `get_radar_image` caches it with `content_type="image/gif"` (`radar.py:88`); `discord_bot.py` attaches it as `radar.gif`; `signal.py` base64-encodes it as `data:image/gif;base64,...`. All PNG bytes wearing a GIF label. **3. `radar_animated` doesn't gate animation — it gates whether radar is attached at all.** In `discord_bot.py:146`, `signal.py:113`, `matrix_bot.py:93`, `pushover.py:125` and `webex.py:156` the flag wraps the entire radar-attachment block. A user who turns "animated radar" off silently loses their radar image entirely. The setting's name actively misleads. ## Why this isn't just "fix the content type" **The primary radar source cannot animate.** Probed `mapservices.weather.noaa.gov/.../radar_base_reflectivity/MapServer?f=json`: no `timeInfo`, capabilities are `Map,Query,Data` only. It serves "now" and has no historical frames — there is no time parameter to sweep. **RainViewer can.** `weather-maps.json` returns ~13 past frames at 10-minute cadence spanning ~2 hours, each a tileable path. It is already a dependency (the static fallback, plus the dashboard's satellite/nowcast layers). But RainViewer is currently used as a **single fixed tile at zoom 6/256px**, which compares badly with the NOAA export: | Source | Resolution | Framing | |---|---:|---| | NOAA export (current still) | 0.72 km/px | centred on the location | | RainViewer 1 tile @ z6/256 (current fallback) | 1.91 km/px | wherever it lands | | RainViewer 2x2 stitched @ z7/512 | **0.48 km/px** | centred | Worth calling out on its own: **the existing single-tile fallback is already badly framed.** A location sits wherever it happens to fall inside its tile — St. Louis lands at fraction **0.96** across its zoom-6 tile, i.e. hard against the eastern edge, with most of the image showing area the user doesn't care about. That is a live defect in the fallback path today, independent of animation. ## Decisions taken | Question | Decision | |---|---| | What should "animated" do | **Real animation from RainViewer, stitched.** Fetch the past-frame series, stitch 2x2 tiles at z7/512, crop to a window centred on the location. Ends up sharper than today's still (0.48 vs 0.72 km/px) *and* correctly framed. Falls back to the NOAA still — correctly labelled PNG — when RainViewer has no frames. | | The misnamed flag | **Split into two.** `radar_enabled` (attach radar at all) + `radar_animated` (animate where the source allows). Migration must default `radar_enabled` from each channel's existing `radar_animated` value, so no channel changes behaviour on upgrade. | | Release | **Fold into rc3**, so v2.0.0 ships with the notifier radar path fully correct. | ## Work - [ ] RainViewer frame-series fetch (list past frames, not just the latest). - [ ] Tile stitching + centred crop, replacing the single-tile fetch. Fixes the framing defect for the static fallback too, not only the animation. - [ ] Real GIF assembly via the existing `assemble_gif` (already `disposal=2` after #182), with each frame basemap-composited using `app.services.basemap` from #182 — RainViewer tiles are transparent, so they need the same treatment. - [ ] Correct content type and filename everywhere the result is attached: `get_radar_image`'s cache `content_type`, Discord's `radar.gif`, Signal's `data:image/gif`. The label must follow what was actually produced, since the animation can fall back to a still. - [ ] Migration adding `radar_enabled`, backfilled from `radar_animated`; update the 5 notifier gates, the channel schemas, `config_io` import/export, and the settings UI. - [ ] Tests: animation produces >1 GIF frame; fallback produces a correctly-labelled still; stitched output is centred on the location; the migration preserves per-channel behaviour; frames are opaque after compositing. ## Watch items - **Outbound load.** A stitched animation is (tiles x frames) requests — 2x2 by ~13 frames is ~52 tile GETs per location per refresh. The 5-minute radar cache absorbs most of this, but confirm the cache key covers the animated variant (#182 added the basemap dimension to it) and consider capping frame count. - **RainViewer terms** for this request volume, given it is a free public API already used elsewhere in the app.
Author
Contributor

Implementation is up as #185 (6f12520), CI green. Not merged and no rc3 tag — both are deliberately held pending a call on soak timing.

Verified independently rather than taken on trust: ruff clean, bulk tier 1001 passed, Postgres tier 14 passed including the 0037 backfill case. Confirmed the dashboard/public radar endpoints did not pick up basemap compositing (they stay bare per #182 — only their hardcoded media_type="image/png" was corrected, which was genuinely wrong since they can serve GIF bytes).

Two findings worth recording against this issue:

Pillow collapses runs of byte-identical consecutive frames, even with optimize=False — and more broadly than first thought: 6 frames with a single differing pixel encode as 3, not 6. Consequence for this feature: on a genuinely clear day every frame composites identically and the "animation" is a valid single-frame GIF. Benign — there is nothing to animate — and labelling stays correct either way. The multi-frame test varies tiles per frame so it exercises this issue's regression rather than Pillow's dedup.

The backfill is a no-op on dev. notification_channels there holds exactly one row, with radar_animated=true. So 0037's value is entirely at production GA, where any channel that had deliberately opted out of radar would otherwise have had it silently switched back on. Worth checking production's radar_animated distribution before v2.0.0 final, since dev cannot exercise that path.

Also unresolved and independent of this work: .forgejo/workflows/cd.yml is now titled "CI (main)" and deploys nothing — the filename is a leftover from the retired merge-time dev deploy (iac-repo#219). Cosmetic; noting it so it is not lost.

Implementation is up as #185 (`6f12520`), **CI green**. Not merged and no rc3 tag — both are deliberately held pending a call on soak timing. Verified independently rather than taken on trust: ruff clean, bulk tier **1001 passed**, Postgres tier **14 passed** including the 0037 backfill case. Confirmed the dashboard/public radar endpoints did *not* pick up basemap compositing (they stay bare per #182 — only their hardcoded `media_type="image/png"` was corrected, which was genuinely wrong since they can serve GIF bytes). Two findings worth recording against this issue: **Pillow collapses runs of byte-identical consecutive frames**, even with `optimize=False` — and more broadly than first thought: 6 frames with a single differing pixel encode as 3, not 6. Consequence for this feature: on a genuinely clear day every frame composites identically and the "animation" is a valid single-frame GIF. Benign — there is nothing to animate — and labelling stays correct either way. The multi-frame test varies tiles per frame so it exercises this issue's regression rather than Pillow's dedup. **The backfill is a no-op on dev.** `notification_channels` there holds exactly one row, with `radar_animated=true`. So 0037's value is entirely at production GA, where any channel that had deliberately opted out of radar would otherwise have had it silently switched back on. Worth checking production's `radar_animated` distribution before v2.0.0 final, since dev cannot exercise that path. Also unresolved and independent of this work: `.forgejo/workflows/cd.yml` is now titled "CI (main)" and deploys nothing — the filename is a leftover from the retired merge-time dev deploy (iac-repo#219). Cosmetic; noting it so it is not lost.
Author
Contributor

Shipped in v2.0.0-rc3, soaking on dev

Merged via #185 (6f12520), tagged v2.0.0-rc3, image git.rhoving.com/rbrooks/weatherbot:2.0.0-rc3 published and deployed to docker-test (iac-repo 9c7aa61).

Play recap: ok=23 changed=3 failed=0 unreachable=0. Container healthy, startup clean, no errors.

Migration 0037 applied and verified on a real database:

INFO [alembic.runtime.migration] Running upgrade 0036 -> 0037, Split
notification_channels.radar_animated into radar_enabled + radar_animated (#184)

Post-migration state on dev: radar_enabled=t, radar_animated=t for the single channel — correct, since that row had radar_animated=true. As noted above, neither dev (1 channel) nor production (4 channels) has a row with radar_animated=false, so no real database anywhere exercises the branch of the backfill that matters. It is verified only by the Postgres-tier test. Worth remembering if a channel is ever set to still-only before v2.0.0 GA.

Rollback is different for this RC

rc1 → rc2 added no migrations and was reversible by image swap. rc2 → rc3 is not. rc3 adds 0037 and rc2's image tops out at 0036, so swapping the image back without restoring a dump first leaves the database ahead of the code and the app crash-loops on Can't locate revision identified by '0037' — precisely the failure that took production down for nine days (#186, iac-repo#335).

Mitigations in place:

  • The rollback note at the docker-test pin now spells this out rather than implying image-swap reversibility.
  • A pre-rc3 dump was taken before deploying: /home/ryan/weatherbot-dev-pre-2.0.0rc3-20260820.sql.gz on docker-test (4.9 MB, database confirmed at 0036 at dump time).

The same caution applies at GA, and more sharply: production goes 1.8.1 → 2.0.0 in one step, running 0034 through 0037 against real data. Dev has only ever migrated incrementally across the RCs, so that combined path has never been exercised anywhere.

## Shipped in v2.0.0-rc3, soaking on dev Merged via #185 (`6f12520`), tagged `v2.0.0-rc3`, image `git.rhoving.com/rbrooks/weatherbot:2.0.0-rc3` published and deployed to `docker-test` (iac-repo `9c7aa61`). Play recap: `ok=23 changed=3 failed=0 unreachable=0`. Container healthy, startup clean, no errors. **Migration 0037 applied and verified on a real database:** ``` INFO [alembic.runtime.migration] Running upgrade 0036 -> 0037, Split notification_channels.radar_animated into radar_enabled + radar_animated (#184) ``` Post-migration state on dev: `radar_enabled=t, radar_animated=t` for the single channel — correct, since that row had `radar_animated=true`. As noted above, neither dev (1 channel) nor production (4 channels) has a row with `radar_animated=false`, so **no real database anywhere exercises the branch of the backfill that matters**. It is verified only by the Postgres-tier test. Worth remembering if a channel is ever set to still-only before v2.0.0 GA. ## Rollback is different for this RC rc1 → rc2 added no migrations and was reversible by image swap. **rc2 → rc3 is not.** rc3 adds 0037 and rc2's image tops out at 0036, so swapping the image back without restoring a dump first leaves the database ahead of the code and the app crash-loops on `Can't locate revision identified by '0037'` — precisely the failure that took production down for nine days (#186, iac-repo#335). Mitigations in place: - The rollback note at the `docker-test` pin now spells this out rather than implying image-swap reversibility. - A pre-rc3 dump was taken before deploying: `/home/ryan/weatherbot-dev-pre-2.0.0rc3-20260820.sql.gz` on `docker-test` (4.9 MB, database confirmed at `0036` at dump time). The same caution applies at GA, and more sharply: **production goes 1.8.1 → 2.0.0 in one step**, running 0034 through 0037 against real data. Dev has only ever migrated incrementally across the RCs, so that combined path has never been exercised anywhere.
Author
Contributor

rc3 regressed radar capture; fixed in v2.0.0-rc4

rc3 must not be deployed. The RadarImage return-type change in #185 missed the two callers that write radar to disk — periodic frame capture (#130) and the alert-time snapshot (#85) — so both failed on every alert with a bytes-like object is required, not 'RadarImage'. Both are fail-soft, so the only symptom was one warning line per alert per cycle: rc3 silently disabled the radar history capture that rc2 had just fixed.

Fixed in #187, released as v2.0.0-rc4, deployed to docker-test (iac-repo b28db33, play ok=23 changed=3 failed=0; no migrations, plain image swap).

Confirmed working on dev, against a real captured frame

  • First frame captured 6 minutes after deploy — capture is working again.
  • Frame on disk is a valid 600×600 PNG, 11.3% opaque (bare radar). That is correct: #182 composites at assembly/serve time, not capture time, precisely so all retained history benefits retroactively. Stored frames are supposed to be bare.
  • Running the real serve path against that frame: 11.3% opaque (palette) → 100% opaque (RGB), with the Esri basemap layers genuinely fetched from the deployed container. Opaque frames are what make the cumulative-smear bug structurally impossible, so #182's fix is confirmed live rather than only in fixtures.

How this was caught, and why it matters for process

Three release candidates and a green 1001-test suite did not catch it. A temporary dev location pointed at live Mississippi convection did, within one soak window — zero frames against four active alerts over 41 minutes.

The root cause of the blind spot, as noted in #187: tests/conftest.py stubs _fetch_radar out for every test via an autouse fixture, and each test that asserts on capture substitutes its own mock. No test ran the real function, so its contract could change under every caller silently. That is now closed by a contract test that restores the real _fetch_radar and mocks only the HTTP layer beneath it — verified to fail with the exact production error when the fix is reverted.

Still outstanding for GA

  • A convective loop has still never been verified. The Mississippi alerts expired at 05:00 UTC before rc4 was deployed. The location retains an Extreme Heat Warning (valid ~19h) which keeps frames accruing and proves capture, but radar over a heat warning is near-empty — so it will not demonstrate storm motion, and identical frames collapse to a single-frame GIF by Pillow's dedup.
  • #184's animated notifier radar remains unexercised. Dev's only channel is type webhook, and webhook.py never calls get_radar_image. Verifying it needs a Discord bot, Signal, Matrix, or Pushover channel on dev.

Cleanup owed

Jackson MS (rc3 soak) (48f10aef-562c-4efb-86f3-1614483f0f7e) was inserted directly into dev's database and subscribed to the test webhook. It should be removed once the soak concludes.

## rc3 regressed radar capture; fixed in v2.0.0-rc4 **rc3 must not be deployed.** The `RadarImage` return-type change in #185 missed the two callers that write radar to disk — periodic frame capture (#130) and the alert-time snapshot (#85) — so both failed on every alert with `a bytes-like object is required, not 'RadarImage'`. Both are fail-soft, so the only symptom was one warning line per alert per cycle: rc3 silently disabled the radar history capture that rc2 had just fixed. Fixed in #187, released as `v2.0.0-rc4`, deployed to `docker-test` (iac-repo `b28db33`, play `ok=23 changed=3 failed=0`; no migrations, plain image swap). ### Confirmed working on dev, against a real captured frame - **First frame captured 6 minutes after deploy** — capture is working again. - Frame on disk is a valid 600×600 PNG, **11.3% opaque** (bare radar). That is *correct*: #182 composites at assembly/serve time, not capture time, precisely so all retained history benefits retroactively. Stored frames are supposed to be bare. - Running the real serve path against that frame: **11.3% opaque (palette) → 100% opaque (RGB)**, with the Esri basemap layers genuinely fetched from the deployed container. Opaque frames are what make the cumulative-smear bug structurally impossible, so #182's fix is confirmed live rather than only in fixtures. ### How this was caught, and why it matters for process Three release candidates and a green 1001-test suite did not catch it. A temporary dev location pointed at live Mississippi convection did, within one soak window — zero frames against four active alerts over 41 minutes. The root cause of the *blind spot*, as noted in #187: `tests/conftest.py` stubs `_fetch_radar` out for every test via an autouse fixture, and each test that asserts on capture substitutes its own mock. No test ran the real function, so its contract could change under every caller silently. That is now closed by a contract test that restores the real `_fetch_radar` and mocks only the HTTP layer beneath it — verified to fail with the exact production error when the fix is reverted. ### Still outstanding for GA - **A convective loop has still never been verified.** The Mississippi alerts expired at 05:00 UTC before rc4 was deployed. The location retains an Extreme Heat Warning (valid ~19h) which keeps frames accruing and proves capture, but radar over a heat warning is near-empty — so it will not demonstrate storm motion, and identical frames collapse to a single-frame GIF by Pillow's dedup. - **#184's animated notifier radar remains unexercised.** Dev's only channel is type `webhook`, and `webhook.py` never calls `get_radar_image`. Verifying it needs a Discord bot, Signal, Matrix, or Pushover channel on dev. ### Cleanup owed `Jackson MS (rc3 soak)` (`48f10aef-562c-4efb-86f3-1614483f0f7e`) was inserted directly into dev's database and subscribed to the test webhook. It should be removed once the soak concludes.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rbrooks/WeatherBot#184
No description provided.