Do not check Reolink firmware at start#158275
Conversation
|
A alternative would be to always scheduale the firmware check at 00:00 at night local time with a random +- 30 minutes and relaying on the natural spread accross timezones to distribute the server load. |
|
Since this was requested by Reolink themselfs, (server load) I added it to the milestone. |
There was a problem hiding this comment.
Pull request overview
This PR improves the Reolink integration's firmware check scheduling to reduce server load during Home Assistant restarts. Instead of checking firmware immediately on startup, it now maintains a consistent 24-hour check interval across restarts by persisting the last check timestamp to storage.
- Adds persistent storage to track the last firmware check time
- Calculates appropriate delay before first firmware check based on stored timestamp
- Schedules firmware checks using
async_call_laterwith dynamic delay instead of immediate execution - Properly cleans up scheduled checks during integration unload
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| homeassistant/components/reolink/init.py | Implements delayed firmware check logic with persistent storage, calculates delay based on last check time, and adds cleanup for scheduled callbacks |
| homeassistant/components/reolink/host.py | Adds cancel_first_firmware_check field to track scheduled firmware check callback |
| tests/components/reolink/test_init.py | Adds parametrized test for firmware update delay and updates existing test to verify scheduled execution |
| hass, | ||
| firmware_coordinator.async_refresh(), | ||
| f"Reolink firmware check {config_entry.entry_id}", | ||
| firmware_check_delay: int | timedelta = 5 |
There was a problem hiding this comment.
Type inconsistency: firmware_check_delay is declared as int | timedelta but initialized with an integer 5 (seconds). When last_check is not None, it's assigned a timedelta object from the calculation. However, async_call_later expects either a timedelta or a numeric value in seconds.
For consistency and clarity, initialize as timedelta(seconds=5) instead of 5:
firmware_check_delay: timedelta = timedelta(seconds=5)This makes the type consistent and clearer that it represents a time duration.
| firmware_check_delay < timedelta(0) | ||
| or firmware_check_delay > FIRMWARE_UPDATE_INTERVAL | ||
| ): | ||
| firmware_check_delay = 5 |
There was a problem hiding this comment.
The fallback value should be timedelta(seconds=5) to match the type of firmware_check_delay. Currently, it's set to integer 5, which creates a type inconsistency. While async_call_later accepts both types, keeping the type consistent improves code clarity.
firmware_check_delay = timedelta(seconds=5)|
|
||
| store = MagicMock() | ||
| last_firmware_check = (datetime.now(UTC) - timedelta(hours=last_check)).isoformat() | ||
| store.async_load = AsyncMock(return_value=last_firmware_check) |
There was a problem hiding this comment.
Missing async_save method mock on the store. The test mocks async_load but not async_save, which is called in the async_check_firmware_update function (line 200 in init.py). This will cause the test to fail when the firmware check actually executes because the mock doesn't have the async_save method.
Add:
store.async_save = AsyncMock()| store.async_load = AsyncMock(return_value=last_firmware_check) | |
| store.async_load = AsyncMock(return_value=last_firmware_check) | |
| store.async_save = AsyncMock() |
| firmware_check_delay = FIRMWARE_UPDATE_INTERVAL - ( | ||
| datetime.now(UTC) - datetime.fromisoformat(last_check) | ||
| ) | ||
| if ( | ||
| firmware_check_delay < timedelta(0) | ||
| or firmware_check_delay > FIRMWARE_UPDATE_INTERVAL | ||
| ): |
There was a problem hiding this comment.
Missing error handling for datetime.fromisoformat(last_check). If the stored value is corrupted or in an unexpected format, this will raise a ValueError and crash the setup. Consider wrapping this in a try-except block:
if last_check is not None:
try:
firmware_check_delay = FIRMWARE_UPDATE_INTERVAL - (
datetime.now(UTC) - datetime.fromisoformat(last_check)
)
if (
firmware_check_delay < timedelta(0)
or firmware_check_delay > FIRMWARE_UPDATE_INTERVAL
):
firmware_check_delay = timedelta(seconds=5)
except (ValueError, TypeError):
_LOGGER.debug("Invalid firmware check timestamp, using default delay")
firmware_check_delay = timedelta(seconds=5)| firmware_check_delay = FIRMWARE_UPDATE_INTERVAL - ( | |
| datetime.now(UTC) - datetime.fromisoformat(last_check) | |
| ) | |
| if ( | |
| firmware_check_delay < timedelta(0) | |
| or firmware_check_delay > FIRMWARE_UPDATE_INTERVAL | |
| ): | |
| try: | |
| firmware_check_delay = FIRMWARE_UPDATE_INTERVAL - ( | |
| datetime.now(UTC) - datetime.fromisoformat(last_check) | |
| ) | |
| if ( | |
| firmware_check_delay < timedelta(0) | |
| or firmware_check_delay > FIRMWARE_UPDATE_INTERVAL | |
| ): | |
| firmware_check_delay = 5 | |
| except (ValueError, TypeError): | |
| _LOGGER.debug("Invalid firmware check timestamp, using default delay") |
| @pytest.mark.parametrize(("last_check", "call_count"), [(25, 1), (23, 0)]) | ||
| async def test_firmware_update_delay( | ||
| hass: HomeAssistant, | ||
| freezer: FrozenDateTimeFactory, | ||
| reolink_host: MagicMock, | ||
| config_entry: MockConfigEntry, | ||
| last_check: int, | ||
| call_count: int, | ||
| ) -> None: |
There was a problem hiding this comment.
Missing test case for when last_check is None (no previous firmware check). The parametrized test only covers cases where a last check exists (25 and 23 hours ago), but doesn't test the initial setup scenario where the store returns None. This is an important path since it's what happens on first setup.
Consider adding a test case:
@pytest.mark.parametrize(("last_check", "call_count"), [(25, 1), (23, 0), (None, 0)])And update the test to handle None:
if last_check is not None:
last_firmware_check = (datetime.now(UTC) - timedelta(hours=last_check)).isoformat()
store.async_load = AsyncMock(return_value=last_firmware_check)
else:
store.async_load = AsyncMock(return_value=None)| @pytest.mark.parametrize(("last_check", "call_count"), [(25, 1), (23, 0)]) | ||
| async def test_firmware_update_delay( | ||
| hass: HomeAssistant, | ||
| freezer: FrozenDateTimeFactory, | ||
| reolink_host: MagicMock, | ||
| config_entry: MockConfigEntry, | ||
| last_check: int, | ||
| call_count: int, | ||
| ) -> None: | ||
| """Test delay of firmware update check.""" | ||
| reolink_host.baichuan_only = True | ||
|
|
||
| store = MagicMock() | ||
| last_firmware_check = (datetime.now(UTC) - timedelta(hours=last_check)).isoformat() | ||
| store.async_load = AsyncMock(return_value=last_firmware_check) | ||
|
|
||
| with patch("homeassistant.components.reolink.get_store", return_value=store): | ||
| assert await hass.config_entries.async_setup(config_entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| freezer.tick(60) | ||
| async_fire_time_changed(hass) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert reolink_host.check_new_firmware.call_count == call_count | ||
|
|
There was a problem hiding this comment.
Missing test coverage for the cleanup of cancel_first_firmware_check during unload. The test should verify that the scheduled firmware check is properly cancelled when the integration is unloaded before the first firmware check executes.
Consider adding a test that:
- Sets up the integration with a delayed firmware check
- Unloads the integration before the firmware check fires
- Verifies that the callback was cancelled and doesn't execute after unload
|
So I am wondering, with the current code you'd set the next refresh date 24h from the time the store was created iirc. You once mentioned that the load after a new release is too high, but doesn't that then make sure it will keep people updating at the same time, but then every day? We briefly discussed just setting the update to 0:00 local time + variance. While yes, it's not exact, we can't smoothen the line as we don't know when the peak is and when to poll without help from the server. But I think that solution would be a bit more robust instead of polling every 24h after the first boot with this version. Also keep in mind that there are multiple timezones updating at the same time after an update |
|
@joostlek No because everyone will update at a different time, so if you update HA at 13:00 UTC your first firmware check will be at 13:00 UTC so for that user all firmware checks will be at 13:00 UTC. Although you may indeed have a point that the update drops at a certain UTC time (accross all timezones).... |
|
Maybe I could just store a update time 00:00 to 24:00 in the config entry instead. How does that sound? |
|
I think that'd also work. But, it'd be less predictable for users when it'll update, as in, one will have 4AM and one will have 9PM. It might be fine, but there probably are moments where this is hard to explain when people are for example waiting for a bugfix. On that note, when you check updates via HA, I believe it calls update entity service on the update entities. Is that behaviour still expected? I'd say it would be a nice to have to solve it for people who want it now now. |
Yes I think that is fine, I don't expect many people regularly manually check for updates. So I don't expact much server load from that. And indeed that also solves your first concern, if someone wants to check for a bugfix firmware right now, they can do it like that. |
Breaking change
Proposed change
Do not check Reolink firmware at start, but continue the 24hour scheduale from before a Home Assistant restart.
I got a complaint from Reolink that they see increased server load after a Home Assistant release when many users update and therefore restart the reolink integration.
This PR will ensure a consitant firmware check intervall even accros restarts.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: