Keep alerts that leave an optional field out - #130
Open
ptarjan wants to merge 2 commits into
Open
Conversation
A packet missing eventState or activePostCount died on .text of a None
find() result, and the one try/except around the whole block turned that
into "Problem finding attribute: 'NoneType' object has no attribute
'text'" and discarded the event -- including its event type and channel,
which were present. An empty activePostCount was worse: int('') raised
ValueError out of process_stream(), and the caller reads ValueError as a
dropped connection, so one malformed packet cost the whole stream a
reconnect and its backoff.
Read each field on its own instead. Only the event type is required; a
missing state means the device only posts while the event is happening,
so treat the packet as active and let update_stale() clear it, and an
unusable post count is reported as 0.
assertNoLogs landed in 3.10, so the four tests on master that use it, and the five added here, error out on the 3.9 leg of the matrix. Install a context-manager equivalent when the real one is missing.
Contributor
Author
|
CI was red on the 3.9 leg: |
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.
process_stream()readeventStateandactivePostCountastree.find(...).text, with onetrywrapped around the whole block. A device that leaves either element out of a packet therefore lost the entire alert — including the event type and channel id it did send — and logged:An empty
<activePostCount/>is worse.int('')raisesValueErrorfrom outside thattry, and the caller inalert_stream()catchesValueErroras a dropped connection, so a single malformed packet tears the stream down and reconnects with backoff.Reproduced on
master, one motion alert per row:<activePostCount><eventState><activePostCount>ValueErrorout ofprocess_stream()Each field is now read on its own. Only
eventTypeis required. A device that omitseventStateonly posts while the event is happening, so the packet itself is the active signal andupdate_stale()clears it as it already does for events that never post an inactive packet; an unusable post count is reported as0.Also tightened the channel-id loop so
echidcan only ever end up anintorNone— it could previously fall out of the loop still holding anElement, whichfetch_attributes()then silently failed to match.Testing
Five tests added; four fail on
master. Full suite: 69 passed, 1 skipped.Verified live against a DS-7608NI-EV2/8P (V3.4.96). 120 real packets captured off
alertStreamover five minutes — this NVR always sends both fields, so the bad packets come from other models or firmware, but note it sendsdynChannelIDfor VMD andchannelIDfor videoloss, i.e. which optional elements appear already varies per event type on one device. With the patch the live stream connects and delivers events normally (8 callbacks in 25 s across three channels), and replaying one of those real packets witheventStateandactivePostCountstripped now delivers the event instead of discarding it.Reported downstream in home-assistant/core#173869 (84 occurrences of the log line, "events stopped coming to HA but third-party apps still get them") and home-assistant/core#144386.