Skip to content

fix(coordinator): fail the update instead of reporting zero alerts - #148

Merged
finity69x2 merged 2 commits into
finity69x2:masterfrom
seevee:fix/coordinator-fail-closed
Oct 17, 2026
Merged

finity69x2 merged 2 commits into
finity69x2:masterfrom
seevee:fix/coordinator-fail-closed

Conversation

@seevee

@seevee seevee commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Closes #147, the parts @ahwman and I agreed on there.

Three paths turned a failed fetch into "0 alerts" with a fresh last_updated. An AttributeError out of update_alerts was caught and swapped for an empty result. A 200 whose body isn't a FeatureCollection fell through the "features" in data check, and NWS does send problem+json with a 200 on some failures, so that read as a quiet all-clear. And a tracker that lost its latitude/longitude returned the empty default too. Each now raises UpdateFailed, which is what every other error path in the coordinator already does: the last good data stays put and the sensors go unavailable until the next poll succeeds.

Not changed: an empty features: [] is still a confirmed zero, and a malformed feature among valid ones is still skipped. Whether one bad feature should sink the whole update is @finity69x2's call.

The diff looks bigger than it is because the parsing loop lost an indent level. Hide whitespace and it's 15 lines.

Tests cover the problem+json 200, a non-object body, missing or non-list features, the AttributeError path, the empty list, and a tracker without coordinates. ruff, mypy and pytest are clean locally.

Assisted-by: Claude:claude-fable-5-1

Three paths turned a broken fetch into "0 alerts" with a fresh
last_updated: an AttributeError out of update_alerts, a 200 whose body
is not a FeatureCollection (NWS sends problem+json with a 200 on some
failures), and a tracker that has lost its latitude/longitude. Each now
raises UpdateFailed, so the coordinator keeps its last good data and
the sensors go unavailable, which is what every other error path here
already does.

An empty features list is still a confirmed zero. A malformed feature
among valid ones is still skipped, that call is left to the maintainer.

Closes finity69x2#147

Assisted-by: Claude:claude-fable-5-1
@ahwman

ahwman commented Oct 2, 2026

Copy link
Copy Markdown

I favor failing the entire update when any alert feature is malformed, preserving the last valid state. Publishing a partial alert list could hide an active power-risk alert and lead to an incorrect risk assessment. This matches the fail-closed behavior in my local coordinator.

@finity69x2

Copy link
Copy Markdown
Owner

I think I agree that a malformed features list shouldn't completely cause the other valid data to be completely excluded. I think (I think...) that could cause a real alert to be mistaken for an invalid result and the valid alert to be ignored.

I think the vast majority of users (including myself) don't ever check for a valid attributes list or an unavailable sensor. We simply react to an issued alert.

If an alert is issued but it fails to have a valid features structure I think I would rather it succeed to provide partial valid data than to fail to provide any data at all. I would rather react to a false positive (which TBH, I'm not even sure that's the result of the failure mode we are discussing) than fail to react to a false negative which could happen if an alert was ignored due to malformed/missing features.

All that is said from my understanding of the issue being discussed. If I'm misunderstanding or missing something please feel free to explain further.

If not then I'll wait a few days for a response then if none comes I'll merge this as is.

Thanks for the support on this issue.

The parse loop drops a feature it can't read and publishes the rest,
and finity69x2#148 kept that on purpose: a reader that fires on an issued alert
loses nothing it could have had, where failing the whole update would
hide the valid alerts in the same poll. Nothing covered it, so the
behavior was an accident of the loop rather than a decision. This case
is one unreadable feature between two good ones.

Assisted-by: Claude:claude-fable-5-1
@seevee

seevee commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

@finity69x2 your reading is correct, and the PR does as you say. A feature the parser can't read logs a warning and is skipped, the rest are published. The whole-update failures only fire when the body isn't a FeatureCollection at all and there's nothing valid to keep.

I pushed one more test to pin the skip: three features, one unreadable between two good ones - update succeeds with two. Now that behavior's a decision on record. Merge whenever you're ready.

@finity69x2
finity69x2 merged commit bbb8bd2 into finity69x2:master Oct 17, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve NWS coordinator resilience to network failures and malformed alert responses

3 participants