Notifier radar: make "animated" actually animate, and split the flag that doesn't mean what it says #184
Labels
No labels
area:ai
area:ci-cd
area:notifications
area:observability
area:public-pages
backlog
bug
duplicate
enhancement
help wanted
invalid
question
type:decision
type:feature
type:infra
type:maintenance
type:security
v1.0.1
v1.1.0
v1.2.0
v1.3.0
v2.0.0
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
rbrooks/WeatherBot#184
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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=Trueis a no-op.radar._fetch_radar(radar.py:99-105) accepts theanimatedparameter 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_imagecaches it withcontent_type="image/gif"(radar.py:88);discord_bot.pyattaches it asradar.gif;signal.pybase64-encodes it asdata:image/gif;base64,.... All PNG bytes wearing a GIF label.3.
radar_animateddoesn't gate animation — it gates whether radar is attached at all. Indiscord_bot.py:146,signal.py:113,matrix_bot.py:93,pushover.py:125andwebex.py:156the 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: notimeInfo, capabilities areMap,Query,Dataonly. It serves "now" and has no historical frames — there is no time parameter to sweep.RainViewer can.
weather-maps.jsonreturns ~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:
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
radar_enabled(attach radar at all) +radar_animated(animate where the source allows). Migration must defaultradar_enabledfrom each channel's existingradar_animatedvalue, so no channel changes behaviour on upgrade.Work
assemble_gif(alreadydisposal=2after #182), with each frame basemap-composited usingapp.services.basemapfrom #182 — RainViewer tiles are transparent, so they need the same treatment.get_radar_image's cachecontent_type, Discord'sradar.gif, Signal'sdata:image/gif. The label must follow what was actually produced, since the animation can fall back to a still.radar_enabled, backfilled fromradar_animated; update the 5 notifier gates, the channel schemas,config_ioimport/export, and the settings UI.Watch items
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_channelsthere holds exactly one row, withradar_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'sradar_animateddistribution before v2.0.0 final, since dev cannot exercise that path.Also unresolved and independent of this work:
.forgejo/workflows/cd.ymlis 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.Shipped in v2.0.0-rc3, soaking on dev
Merged via #185 (
6f12520), taggedv2.0.0-rc3, imagegit.rhoving.com/rbrooks/weatherbot:2.0.0-rc3published and deployed todocker-test(iac-repo9c7aa61).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:
Post-migration state on dev:
radar_enabled=t, radar_animated=tfor the single channel — correct, since that row hadradar_animated=true. As noted above, neither dev (1 channel) nor production (4 channels) has a row withradar_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:
docker-testpin now spells this out rather than implying image-swap reversibility./home/ryan/weatherbot-dev-pre-2.0.0rc3-20260820.sql.gzondocker-test(4.9 MB, database confirmed at0036at 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.
rc3 regressed radar capture; fixed in v2.0.0-rc4
rc3 must not be deployed. The
RadarImagereturn-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 witha 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 todocker-test(iac-repob28db33, playok=23 changed=3 failed=0; no migrations, plain image swap).Confirmed working on dev, against a real captured frame
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.pystubs_fetch_radarout 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_radarand mocks only the HTTP layer beneath it — verified to fail with the exact production error when the fix is reverted.Still outstanding for GA
webhook, andwebhook.pynever callsget_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.