Keep NOAA image cards on screen through network blips - #34
Merged
Conversation
Every so often the Home Assistant host briefly loses DNS or routing and the log fills with lines like ERROR [custom_components.noaa_it_all.image] Error fetching aurora forecast image: Cannot connect to host services.swpc.noaa.gov:443 ssl:default [Timeout while contacting DNS servers] The upstream failure is external, but the picture disappearing was ours. Four separate defects combined to cause it: 1. All seven async_image() methods returned b"" on any failure and cached nothing. Core's _async_get_image() does `if image_bytes := await entity.async_image()`, so empty bytes are falsy, it raises, and ImageView turns that into an HTTP 500. One blip destroyed a good image. 2. entity_picture was overridden to the raw NOAA URL, so the browser fetched services.swpc.noaa.gov directly rather than /api/image_proxy/. The card therefore broke whenever the *browser* could not reach NOAA, and any cache inside async_image() would have been unreachable for normal cards. 3. A ClientTimeout expiry raises asyncio.TimeoutError, which is not an aiohttp.ClientError, so it fell through to the catch-all arm and logged "Unexpected error". REQUEST_TIMEOUT of 30s also exceeded HA's ~10s image-proxy budget, so an in-request fetch could never use it. 4. Every transient blip logged at ERROR, which is wrong for a cloud_polling integration. Fixes 1 and 2 are not independent: removing the entity_picture override without a background refresher would leave image_last_updated permanently None, so entity_picture stays None, so the proxy is never called, so async_image() never runs. The picture would be blank always, not just during outages. Collapse the seven near-identical classes into a NoaaImageEntity base that fetches on a 10-minute timer, keeps the last successful bytes, and serves them from memory. A failed fetch touches nothing -- not the bytes, not image_last_updated, not the ETag -- which is the whole fix. Subclasses keep only name, unique ID, device info, URL, content type and a log label. entity_picture falls back to the upstream URL until the first successful fetch: in that window there is nothing cached to serve, and a browser on a working network can still render when Home Assistant's resolver cannot -- which is exactly the reported failure. Also: classify timeouts, DNS failures, resets and disconnects as transient and stage the log level by outage duration (debug while a cached frame is still being served, one warning at ~30 min, error at ~1 hour and then periodically, info on recovery); send the integration's User-Agent; revalidate with ETag/Last-Modified so background polling stays cheap; set the real content type per source (five of seven were not JPEG); cap the response size; and replace the deprecated datetime.utcnow(). The state now advances from "unknown" to the timestamp of the frame being served, so a stale image is templatable. Tests: the module-wide ban on the async_write_ha_state string (c7ed6e6) is replaced by an AST check that the call may only appear in the guarded _write_state_if_added helper, keeping the original intent -- no state write on a path reachable before entity_id is assigned -- now that the background refresher legitimately publishes. Adds coverage for the cache surviving each failure mode, the log escalation ladder, conditional requests, scheduling, and the per-source content types; async_image() had none before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
USER_AGENT was "HomeAssistant/NOAA-Integration" on all 17 outbound request sites (16 in coordinator.py, 1 in image.py). Four things wrong with it: - The "HomeAssistant/" prefix implies Home Assistant core, but this is a third-party HACS integration. Core's own nws integration sends whatever the user put in its API Key field instead. Misbehaviour here would be attributed to the wrong project. - No contact information. api.weather.gov asks for a website or email so they can get in touch rather than simply blocking traffic they cannot place. - "NOAA-Integration" is not unique to this application, so it risks being caught by a block aimed at somebody else's traffic. - No version, so an old release cannot be told apart from a fixed one. That last point matters now in particular: the image entities have just moved to background polling, which is outbound traffic that did not previously exist. Being identifiable and reachable is worth more at that moment. Use noaa_it_all/<version> (+<documentation url>) -- the domain rather than the display name, since it has no spaces and is the token that already appears in logs and issue reports, and the manifest's documentation URL, which points at the public repo rather than this dev fork. A contact email can be added later. Keeping the version in a literal risks it going stale, so TestUserAgent in tests/test_manifest.py asserts it matches manifest.json, along with the domain prefix, the contact URL, that it does not point at the dev repo, and that it no longer claims to be Home Assistant core. const.py is imported by path there rather than as noaa_it_all.const, which would pull in the package __init__ and with it the Home Assistant runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
The version was a literal in const.py, so a release bump meant remembering two places. The previous guard test only caught the drift after the fact; it did not prevent it. const.py now reads manifest.json (which sits next to it) at import and exposes VERSION and DOCUMENTATION_URL, with USER_AGENT interpolating both. Bumping the manifest is now the whole job. Home Assistant imports custom integration modules in an executor thread and this is one small local file, so the read does not block the event loop; a broken manifest falls back to sentinels rather than raising while merely importing constants. test_version_matches_the_manifest cannot catch a regression on its own any more -- both sides now read the same file, so pasting the current version back in as a literal would still pass, right up until the next bump. Added tests that assert the USER_AGENT assignment contains no version literal and no URL literal, and that it interpolates VERSION and DOCUMENTATION_URL. Verified both directions: bumping manifest.json moves the User-Agent with no other edit, and hardcoding either value fails the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
A cold boot produced a burst of template tracebacks:
TypeError: 'NoneType' object is not iterable
UndefinedError: None has no element 0
UndefinedError: 'None' has no attribute 'last_changed'
all within 26 ms of each other, and not reproducible on the next reboot.
Every failing template was an example copied out of README.md.
The cause is a startup race, not bad data. async_setup_entry awaits an
initial refresh of ten coordinators -- all making live NWS calls -- before
forwarding any platform, so on a slow boot the frontend can subscribe to a
dashboard template before the entities exist. state_attr() on a missing
entity returns None, and states.sensor.X returns None outright, so an
unguarded [0] or {% for %} raises. The cards recover on the next render, but
each attempt logs a full traceback and whether it happens at all depends on
who wins the race that boot.
The sensors were never at fault: sensors/forecasts.py publishes 'periods' as
[] in the constructor, on the no-data path and on the error path, and
sensors/meteor_showers.py uses `or []` for 'upcoming'. A None means the
entity is not there yet.
Guard the examples with `or []` plus a count check, and bind states.* objects
before testing them. Covers the three Extended Forecast cards, the
upcoming-shower loop, the mobile header and the two alert automations, which
had the same unguarded [0] on the 'alerts' attribute. Added a note to the
Dashboard Card Examples section explaining the race, so cards written from
these docs inherit the habit, and removed a duplicated heading there.
The forecast cards also drop from a folded scalar calling state_attr() six
times to a literal block binding it once, with the HTML flush against the
block indent -- four spaces of relative indent inside a markdown card would
be parsed as a code fence.
Verified by extracting all 24 yaml blocks from README.md, parsing them, and
rendering the reworked templates through jinja2 both with data and with
state_attr returning None. All render; the originals raise the exact errors
from the report under the same conditions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
Three coordinators reported total failure: Error fetching NOAA Forecasts data: All forecast API requests failed Error fetching NOAA Hurricanes data: All hurricane API requests failed Error fetching NOAA Space Weather data: All space weather API requests failed The forecast one is a real bug, not a blip. ForecastCoordinator._resolve_forecast_urls() set self._urls_fetched = True in its except branch as well as on success. One transient failure of the NWS Points API therefore left _forecast_url and _hourly_forecast_url None with the lookup permanently retired. _async_update_data guards each fetch with `if self._forecast_url:`, so from then on no request was even attempted: both values stayed None, all(v is None) held, and every refresh raised UpdateFailed until Home Assistant restarted. Latch only on success; at a 10-minute update interval, retrying next cycle is already the right backoff. The identical latch was in ObservationsCoordinator._resolve_station() and CloudCoverCoordinator._resolve_gridpoint_url(), silently retiring the station and gridpoint lookups the same way. Both fixed. Those handlers also logged at ERROR; now that they retry, WARNING is the honest level, and the coordinator's own UpdateFailed reporting handles escalation. Second, the space weather and hurricane coordinators were the only ones not sending a User-Agent -- 5 of 19 outbound requests -- and _HURRICANE_ALERTS_URL is api.weather.gov, which requires one. The two fetch bodies were also copy-pasted per endpoint; both are now a loop over an endpoint table. Third, "All X API requests failed" discarded every underlying exception, so the ERROR naming the problem carried no cause and the real reason sat in separate WARNING lines above it -- when a request had been attempted at all, which in the forecast case it had not. Reasons are now collected and appended. _describe() exists because several aiohttp errors stringify to "". Adds tests/test_coordinator.py. coordinator.py had no behavioural coverage across 773 lines and 10 coordinators, which is how the latch survived. Every new test was confirmed to fail against the pre-fix file; the four that pass both ways cover behaviour that was already correct. Not verified: whether api.weather.gov actually 403s the default aiohttp User-Agent. This sandbox's egress proxy blocks NOAA, so the header fix rests on NWS's documented requirement rather than a reproduction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
Left out of f1ccbdf: the scripted edit asserted on a heading that appears in every release section, so it aborted while the commit went ahead anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUzm76iyL3ce3vbAdpfUQp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the reported symptom where NOAA image tiles go blank whenever the Home Assistant host has a momentary network problem:
The upstream failure is genuinely external, but the picture disappearing was a bug in this integration — four of them, in fact:
b""is a hard error. All sevenasync_image()methods returnedb""on any failure and cached nothing. Core's_async_get_image()doesif image_bytes := await entity.async_image(), so empty bytes are falsy, it raisesHomeAssistantError, andImageViewturns that into an HTTP 500. One blip destroyed a perfectly good image because nothing was kept.entity_picturewas overridden to the raw NOAA URL, so the browser fetchedservices.swpc.noaa.govdirectly rather than/api/image_proxy/. The card broke whenever the browser couldn't reach NOAA, and it made any cache insideasync_image()unreachable for normal dashboard cards.ClientTimeoutexpiry raisesasyncio.TimeoutError, which is not anaiohttp.ClientError, so it fell through to the catch-all arm and loggedUnexpected error fetching ....REQUEST_TIMEOUT = 30also exceeded HA's ~10s image-proxy budget, so an in-request fetch could never use its budget.cloud_pollingintegration.Fixes 1 and 2 are not independent. Removing the
entity_pictureoverride without a background refresher would leaveimage_last_updatedpermanentlyNone→entity_picturestaysNone→ the proxy is never called →async_image()never runs. The picture would be blank always, not just during outages.What changed
NoaaImageEntitybase that fetches on a 10-minute timer, keeps the last successful bytes in memory, and serves them from there. A failed fetch touches nothing — not the bytes, notimage_last_updated, not the ETag. That invariant is the fix. Subclasses keep only name, unique ID, device info, URL, content type and a log label.entity_picturefalls back to the upstream URL until the first successful fetch — in that window there's nothing cached to serve, and a browser on a working network can still render when HA's resolver can't, which is exactly the reported failure.User-Agent; revalidates withETag/Last-Modifiedso background polling stays cheap; sets the real content type per source (five of seven were not JPEG — two PNG, two GIF); caps response size; replaces deprecateddatetime.utcnow().Behaviour changes worth knowing
unknownto an ISO-8601 timestamp that advances when the bytes change — so "this image has gone stale" is now templatable.entity_picturebecomes/api/image_proxy/...after the first fetch. Normalpicture-entity/imagecards are unaffected and get better; anything reading the raw NOAA URL out of the attributes will see a change.Type
Checklist
custom_components/noaa_it_all/andtests/0.5.2→0.5.3Tests
744 passed, 21 subtests passed.async_image()had zero coverage before this change, which is why theb""bug survived in seven places. Added: the cache surviving each failure mode (connection error,asyncio.TimeoutError, server disconnect, HTTP 5xx, non-image content type, empty body, oversized body), first-ever failure returningNonerather thanb"", unchanged bytes not advancing the timestamp,304handling, the log-escalation ladder, conditional request headers, timer scheduling and cancellation, and the per-source content types.The startup regression guard from
c7ed6e6(module-wide ban on theasync_write_ha_statestring) is retargeted, not dropped. Its real intent was "no state write on a path Home Assistant can reach beforeentity_idis assigned"; the background refresher now legitimately publishes state, so the string check is replaced by an AST check that the call may only appear inside the guarded_write_state_if_addedhelper, plus behavioural tests that a refresh landing before the entity is added is a no-op.The test harness stubs Home Assistant with
MagicMockmodules, so this also adds stubs forhomeassistant.helpers.eventandhomeassistant.util.dt, realaiohttpexception classes (mocked ones can't be used in anexceptclause), and anImageEntityfake whoseentity_picturemirrors upstream's None-until-image_last_updatedsemantic — the semantic the fallback exists to work around.Manual verification still to do on a live instance
image.*entities show an ISO timestamp state and an/api/image_proxy/...picture.services.swpc.noaa.govand force a refresh — the tiles must keep showing the last image, with at most a WARNING and no ERROR for the first couple of cycles.image_last_updatedadvances.Generated by Claude Code