Stopping SensorUpdate if get_config fail

[TimoPtr]

Summary

It turns out that when the HA instance is not connected when using HA Cloud it throws a 503 error Service Unavailable. Which make sense, unfortunately our codebase does not properly detect that while updating sensors.

Currently the syncSensorsWithServer has a logic to detect server unreachable when it timesout or if we get a ConnectError. This is not enough and doesn't cover all the potential cases where the error is because we cannot reach the server.

To save some calls I've decide to always make the get_config always since in a lot of case we are going to make it anyway. If this call fails for any reason we should not continue.

Checklist

• New or updated tests have been added to cover the changes following the testing guidelines.
• The code follows the project's code style and best_practices.
• The changes have been thoroughly tested, and edge cases have been considered.
• Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Any other notes

I was considering having a better handling within the IntegrationRepository of the error to have a dedicated error for ServerUnreachable using (ConnectException, SocketTimeoutException and HTTPException.code 503), but we might miss some cases unfortunately due to the nature of the potential configuration of our users (proxy, vpn, .... can lead to many different errors)

I would like also to consider adding a debounce logic on this syncSensorsWithServer since it can be invoked multiples times in a short amount of time, which would result in sending multiple times the same data.

@jpelgrom you have an in-depth knowledge on this two pieces and I would like your opinion before going further, especially the debounce.

[jpelgrom]

The current architecture was specifically designed to allow sensors to continue updating locally, even if the server is temporarily unavailable. After that the syncing was added.

Your currently proposed changed order of calls works, but feels to me like stopping sensors quite early even if getting the config is not strictly necessary to handle updates.

It also breaks the 'feature' of no server trust + no enabled sensors = no network calls during the SensorWorker (see: "As an intended side effect").

@jpelgrom you have an in-depth knowledge on this two pieces

Very nice way of saying I'm mostly responsible for the current sensor sync spaghetti 😊

Currently the syncSensorsWithServer has a logic to detect server unreachable when it timesout or if we get a ConnectError. This is not enough and doesn't cover all the potential cases where the error is because we cannot reach the server.

I was considering having a better handling within the IntegrationRepository of the error to have a dedicated error for ServerUnreachable using (ConnectException, SocketTimeoutException and HTTPException.code 503), but we might miss some cases unfortunately due to the nature of the potential configuration of our users (proxy, vpn, .... can lead to many different errors)

My first preference would be expanding on this logic, which as you noted already somewhat exists. Even if imperfect, catching most/known use cases (like 503 with HA Cloud) will already help.

Network edge cases will always exist because HA doesn't do a lot and users add stuff inbetween, and as you've commented on in the past, we should try to avoid general exception catching.

I would like also to consider adding a debounce logic on this syncSensorsWithServer since it can be invoked multiples times in a short amount of time, which would result in sending multiple times the same data.

This is about the behavior with multiple broadcast receivers triggering at the same time, yes? Users rely on sensor doinga timely update so a simple debounce is quite dangerous IMO. There is already some logic to avoid doing calls if the value hasn't changed. After some consideration I think we may want to move toward something like:

• collect all incoming intents for up to ~1s
• update all sensors (with all intents, preferably in one call instead of multiple calls for each intent)
• sync sensor status (once)
• push sensor values (once)

... so that we guarantee sensors still do very quick updates, can get all intents they need, but avoid multiple network calls which is your main concern I believe. That doesn't seem like something that is achieved with a couple of lines of code though so I wouldn't do that in this PR as well.

[jpelgrom]

Discussing this on Discord, @TimoPtr did rightly point out that

• sensors are still updated, syncing (and returning if get_config fails) happens after the database is updated so no data is lost
• even if we were to continue, there's a very high chance any network call after it would still fail