Repository: home-assistant/core
Pull request: Fix Life360 recovery from server errors
<!--
  You are amazing! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template! (unless instructed).
-->
## Proposed change
<!--
  Describe the big picture of your changes here to communicate to the
  maintainers why we should accept this pull request. If it fixes a bug
  or resolves a feature request, be sure to link to that issue in the
  additional information section.
-->
If communication with the Life360 is interrupted for any reason, the state of
all the entities become `unavailable`. However, after communication is restored,
the state of the entities does not return to normal right away like it should.
Eventually, when the server has new data for an entity it will update and return
to normal.

This change makes the entities return to normal as soon as communication with
the server is restored.

## Type of change
<!--
  What type of change does your PR introduce to Home Assistant?
  NOTE: Please, check only 1! box!
  If your PR requires multiple boxes to be checked, you'll most likely need to
  split it into multiple PRs. This makes things easier and faster to code review.
-->

- [ ] Dependency upgrade
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes #76105
- This PR is related to issue: #75956 and PR #76141
- Link to documentation pull request: 

## Checklist
<!--
  Put an `x` in the boxes that apply. You can also fill these out after
  creating the PR. If you're unsure about any of them, don't hesitate to ask.
  We're here to help! This is simply a reminder of what we are going to look
  for before merging your code.
-->

- [x] The code change is tested and works locally.
- [ ] Local tests pass. **Your PR cannot be merged unless tests pass**
- [x] There is no commented out code in this PR.
- [ ] I have followed the [development checklist][dev-checklist]
- [x] The code has been formatted using Black (`black --fast homeassistant tests`)
- [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

- [ ] Documentation added/updated for [www.home-assistant.io][docs-repository]

If the code communicates with devices, web services, or third-party tools:

- [x] The [manifest file][manifest-docs] has all fields filled out correctly.  
      Updated and included derived files by running: `python3 -m script.hassfest`.
- [ ] New or updated dependencies have been added to `requirements_all.txt`.  
      Updated by running `python3 -m script.gen_requirements_all`.
- [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to `.coveragerc`.

The integration reached or maintains the following [Integration Quality Scale][quality-scale]:
<!--
  The Integration Quality Scale scores an integration on the code quality
  and user experience. Each level of the quality scale consists of a list
  of requirements. We highly recommend getting your integration scored!
-->

- [ ] No score or internal
- [ ] 🥈 Silver
- [ ] 🥇 Gold
- [ ] 🏆 Platinum

<!--
  This project is very active and we have a high turnover of pull requests.

  Unfortunately, the number of incoming pull requests is higher than what our
  reviewers can review and merge so there is a long backlog of pull requests
  waiting for review. You can help here!
  
  By reviewing another pull request, you will help raise the code quality of
  that pull request and the final review will be faster. This way the general
  pace of pull request reviews will go up and your wait time will go down.
  
  When picking a pull request to review, try to choose one that hasn't yet
  been reviewed.

  Thanks for helping out!
-->

To help with the load of incoming pull requests:

- [ ] I have reviewed two other [open pull requests][prs] in this repository.

[prs]: https://github.com/home-assistant/core/pulls?q=is%3Aopen+is%3Apr+-author%3A%40me+-draft%3Atrue+-label%3Awaiting-for-upstream+sort%3Acreated-desc+review%3Anone+-status%3Afailure

<!--
  Thank you for contributing <3

  Below, some useful links you could explore:
-->
[dev-checklist]: https://developers.home-assistant.io/docs/en/development_checklist.html
[manifest-docs]: https://developers.home-assistant.io/docs/en/creating_integration_manifest.html
[quality-scale]: https://developers.home-assistant.io/docs/en/next/integration_quality_scale_index.html
[docs-repository]: https://github.com/home-assistant/home-assistant.io


Problem statement:
Life360 Entities go unavailable until reload of integration
### The problem

I’m running 2022.7.7, but I’ve had this issue with all 2022.7 builds.

Periodically if the internet flakes out, or perhaps an issue with life360 cloud, the entities for my people goes unavailable and they show as away in HA. If I ‘reload’ the integration they immediately recover and are ‘home’ again. Did not have this issue with the older-style integration on 2022.6 and prior.

I can reproduce the issue pretty consistently by killing internet access to HA and then restoring it. It requires a restart of the integration for the entities to update correctly again.

### What version of Home Assistant Core has the issue?

2022.7.x

### What was the last working version of Home Assistant Core?

2022.6.x

### What type of installation are you running?

Home Assistant OS

### Integration causing the issue

life360

### Link to integration documentation on our website

_No response_

### Diagnostics information

_No response_

### Example YAML snippet

_No response_

### Anything in the logs that might be useful for us?

```txt
2022-08-02 20:53:39 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.286 seconds (success: True)
2022-08-02 20:53:49 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.256 seconds (success: True)
2022-08-02 20:53:59 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.269 seconds (success: True)
2022-08-02 20:54:22 DEBUG (MainThread) [homeassistant.components.life360] CommError: HTTPSConnectionPool(host='api.life360.com', port=443): Max retries exceeded with url: /v3/circles.json (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f907d856170>: Failed to establish a new connection: [Errno -3] Try again'))
2022-08-02 20:54:22 ERROR (MainThread) [homeassistant.components.life360] Error fetching life360 (david@davidcoulson.net) data:
2022-08-02 20:54:22 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 13.065 seconds (success: False)
2022-08-02 20:54:32 INFO (MainThread) [homeassistant.components.life360] Fetching life360 (david@davidcoulson.net) data recovered
2022-08-02 20:54:32 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.651 seconds (success: True)
2022-08-02 20:54:42 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.369 seconds (success: True)
2022-08-02 20:54:52 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.591 seconds (success: True)
2022-08-02 20:55:02 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.418 seconds (success: True)
2022-08-02 20:55:12 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.960 seconds (success: True)
2022-08-02 20:55:22 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.364 seconds (success: True)
2022-08-02 20:55:32 DEBUG (MainThread) [homeassistant.components.life360] Finished fetching life360 (david@davidcoulson.net) data in 0.395 seconds (success: True)
```


### Additional information

_No response_

Hints from issue discussion:
I still need to reproduce this behavior on a test system, but I believe what is happening is, after it is able to get good data from the server it still has the previous data, and since it is the same (in this particular case where the device probably isn't moving), it is not bothering to update the state. What it needs to do is to update the state, even if the data hasn't changed, IF the current state is unavailable.

I suspect if the device was moved, or the Life360 app was opened, the Life360 server would get new data (with a new `last_seen` timestamp), so the next time the HA integration updated (every 10 seconds), the entities would update properly.
See also #75956
I had the same issue when I upgraded to version 2022.7.7, but nothing I tried fixed the issue. I tried reloading the integration, updating the entity and even removing/re-adding the integration. Once I restored back to my previous version 2022.6.3, the integration started to work again.
> See also #75956

One thing to note - My person entities go 'away' when the life360 integration entities go unavailable. Sounds like based on this other thread that this isn't expected/intended behavior?
> My person entities go 'away' when the life360 integration entities go unavailable. Sounds like based on this other thread that this isn't expected/intended behavior?

Oh, interesting. No, that's not the expected behavior, at least not the way I read the person integration code. I'll have to give that a try.

I'm pretty convinced now that I should change the life360 integration so that it does _not_ use the `unavailable` state. Rather, when it can't get data from the server it should just keep the state it has. This is effectively the way it used to work. I'm going to look at other similar device tracker integrations to see what they do.

BTW, the bug reported in this incident becomes moot if I make that change, because it only happens _after_ the state goes to `unavailable`. If it never does, that can't happen.
> I had the same issue when I upgraded to version 2022.7.7, but nothing I tried fixed the issue. I tried reloading the integration, updating the entity and even removing/re-adding the integration. Once I restored back to my previous version 2022.6.3, the integration started to work again.

Are you sure you're not looking at the old, legacy, inoperable entities instead of the new ones? Have you read the breaking changes section of the 2022.7 release notes?

If you're sure it's the new entities that are stuck as unavailable, do you see any log messages that might shed some light? Have you tried enabling debug output for life360?
I was able to reproduce the behavior.

When I disconnected the network, the life360 `device_tracker` entities changed to `unavailable`, and a `person` entity based on one of them changed to `unknown`.

After reconnecting the network, the life360 entities eventually changed back to "normal" on their own (when the server had new different data for them.) And the `person` entity also changed back to normal.

FWIW, opening the Life360 app on my phone effectively forced new data and the corresponding HA entity changed back to normal almost immediately.

I'm still going to try to change the integration so that the entities never change to `unavailable` in the first place.
@pnbruckner Appreciate your work on this integration over the years; thanks for the work on the PR to correct this. Do you think a config option to disable the unavailable state would be acceptable? 
@jerobins thank you. No, I'm positive that would not be accepted either. I'm going to close that PR and open a different one to fix the bug and not try to fight the issue of the unavailable state.

Commit message: Fix Life360 recovery from server errors

Patch to review:
diff --git a/homeassistant/components/life360/device_tracker.py b/homeassistant/components/life360/device_tracker.py
index ebb179beba20fa..8228c2e7b108a0 100644
--- a/homeassistant/components/life360/device_tracker.py
+++ b/homeassistant/components/life360/device_tracker.py
@@ -120,16 +120,16 @@ def _options(self) -> Mapping[str, Any]:
     @callback
     def _handle_coordinator_update(self) -> None:
         """Handle updated data from the coordinator."""
-        # Get a shortcut to this member's data. Can't guarantee it's the same dict every
-        # update, or that there is even data for this member every update, so need to
-        # update shortcut each time.
-        self._data = self.coordinator.data.members.get(self.unique_id)
-
+        # Get a shortcut to this Member's data. This needs to be updated each time since
+        # coordinator provides a new Life360Member object each time, and it's possible
+        # that there is no data for this Member on some updates.
         if self.available:
-            # If nothing important has changed, then skip the update altogether.
-            if self._data == self._prev_data:
-                return
+            self._data = self.coordinator.data.members.get(self.unique_id)
+            assert self._data is None or self._data is not self._prev_data
+        else:
+            self._data = None
 
+        if self._data:
             # Check if we should effectively throw out new location data.
             last_seen = self._data.last_seen
             prev_seen = self._prev_data.last_seen
@@ -168,12 +168,6 @@ def force_update(self) -> bool:
         """Return True if state updates should be forced."""
         return False
 
-    @property
-    def available(self) -> bool:
-        """Return if entity is available."""
-        # Guard against member not being in last update for some reason.
-        return super().available and self._data is not None
-
     @property
     def entity_picture(self) -> str | None:
         """Return the entity picture to use in the frontend, if any."""


Code review comments:
diff_hunk: @@ -120,16 +120,16 @@ def _options(self) -> Mapping[str, Any]:
     @callback
     def _handle_coordinator_update(self) -> None:
         """Handle updated data from the coordinator."""
-        # Get a shortcut to this member's data. Can't guarantee it's the same dict every
-        # update, or that there is even data for this member every update, so need to
-        # update shortcut each time.
-        self._data = self.coordinator.data.members.get(self.unique_id)
-
+        # Get a shortcut to this Member's data. This needs to be updated each time since
+        # coordinator provides a new Life360Member object each time, and it's possible
+        # that there is no data for this Member on some updates.
         if self.available:
-            # If nothing important has changed, then skip the update altogether.
-            if self._data == self._prev_data:
-                return
+            self._data = self.coordinator.data.members.get(self.unique_id)
+            assert self._data is None or self._data is not self._prev_data
path: homeassistant/components/life360/device_tracker.py
text: @user1:
Is this the right type of error checking here? If this fails and throws an `AssertionError`, what happens?

@author:
Asserts are generally used to establish expectations of the software design. In this particular case, it's enforcing a design constraint that the new data is either None or it's not referencing the same object as the "previous data" variable. Other parts of the design/implementation depend on this to work correctly.

This assert should never fail. If it does, the software is broken. Really, this is meant to make sure if someone else tries to maintain this integration in the future, if they make a change that causes this assert to fail, they have broken something.

But, to answer your question, if this assert fails then the update will fail. Again, though, if that happens, then something is broken in the code, and there's no telling what else might be wrong.

@user1:
> This assert should never fail.

Then why include it? Asserts are useful when needing to, say, instruct `mypy` about the presence of something, but in this case, I don't see similar value.

> Really, this is meant to make sure if someone else tries to maintain this integration in the future, if they make a change that causes this assert to fail, they have broken something.

I would argue that this is better served via an actual test case (so that future devs can know about breaks during the dev process).

@author:
> Then why include it?

For the reason I stated.

> Asserts are useful when needing to, say, instruct mypy about the presence of something, but in this case, I don't see similar value.

Yes, that is another advantage to them. But it's not the only use for asserts, nor the traditional use case.

> I would argue that this is better served via an actual test case (so that future devs can know about breaks during the dev process).

That is another way. Either way the dev would know. In fact, the assert would let them no sooner, during interactive development, rather than waiting until some point that they get around to running the tests.

Is there some HA standard about asserts I should know about? I certainly have seen many asserts in the code all over the place. I've been trained to use asserts, and in this instance, I find it much preferable to a test case.

I can add a comment if you think it's necessary, but I believe the assert statement itself is sufficient.

@user2:
We should avoid the use of asserts, instead we should be handling the errors.

Asserts suppress linters and defeat their purposes. In most cases, the use of asserts are either working around a typing problem or supressing something that should have been handled. I rather see the typing fixed or possible error case handled.

@author:
> We should avoid the use of asserts

Then you're recommending non-traditional programming practices relative to assert statements in general. A quick Google search of "why Python assert" will show how asserts have traditionally been used in Python, as well as many other programming languages.

> instead we should be handling the errors

There is no "error" to be handled here. That is, there is no normal, run-time error to be handled. The purpose is to handle a potential future software coding error, and to state and enforce design/implementation assumptions.

> In most cases, the use of asserts are either working around a typing problem or supressing something that should have been handled.

I disagree. At least, if that _is_ why most people use asserts, then I would agree they are using them incorrectly.

> I rather see the typing fixed or possible error case handled.

Again, there is no typing issue or run-time error case to be handled.

But, if you insist, I'll remove it and replace it with a comment.

@user2:
> Then you're recommending non-traditional programming practices relative to assert statements in general.

Sorry to hear you feel that way. We have documented this in our developer guidelines as well. We should avoid the use of asserts. Personally, I wish we could enable the linter to forbid them fully.

> There is no "error" to be handled here. That is, there is no normal, run-time error to be handled. 

So there is a typing issue? Because type hinting says, it can return something you say it can't return?

@author:
No, as I have already said, there is no typing issue that I'm aware of. Ok, I'll get rid of the statement. If that causes a typing issue (because the linter is no longer aware of a design/coding constraint), then I'll fix it with a cast.

@user2:
I pulled the branch local, there are issues actually. Bot in terms of possible bugs and typing.

Will post a diff in a sec that improved the situation.

@user2:
See: <https://github.com/home-assistant/core/pull/76231#issuecomment-1205614649>

(Sorry VSCode GitHub Pull request review posts suggested edits at the bottom :( )

diff_hunk: @@ -168,12 +168,6 @@ def force_update(self) -> bool:
         """Return True if state updates should be forced."""
         return False
 
-    @property
-    def available(self) -> bool:
-        """Return if entity is available."""
-        # Guard against member not being in last update for some reason.
-        return super().available and self._data is not None
path: homeassistant/components/life360/device_tracker.py
text: @user1:
This ain't right?

@author:
The override is not needed anymore. Given the other change, the implementation in the superclass is sufficient and required.