Warm the ViCare service cache on refresh instead of bypassing it - #180344
Warm the ViCare service cache on refresh instead of bypassing it#180344lackas wants to merge 4 commits into
Conversation
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: vicare
small-pr
|
Hey there @CFenner, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Warms PyViCare’s cache during coordinator refreshes, preventing blocking API calls during entity state reads.
Changes:
- Refreshes through the cached property path.
- Uses device accessors for coordinator identity.
- Updates mocks and failure tests for the real read path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
homeassistant/components/vicare/coordinator.py |
Warms the service cache safely. |
tests/components/vicare/conftest.py |
Aligns service mocks with PyViCare. |
tests/components/vicare/test_init.py |
Tests refresh failures through getProperty. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
The approach
The approach is right, and a couple of details are righter than they look. Routing through _get_or_update_cache fetches the same full-features URL, so no coverage is lost; the clear_cache() first is load-bearing, because it leaves _cache empty and so disables the stale-cache fallback that would otherwise mask genuine comm/500 errors; and a device whose payload has no device.serial still ends up with a warm cache in one call, since the fetch happens before readFeature raises. Using raw getProperty rather than Device.getSerial() is also the safer choice — getSerial() goes through @handleNotSupported, whose behaviour depends on the process-global Feature.raise_exception_on_not_supported_device_feature.
Measured against real PyViCare 2.61.0: 1 GET per refresh with the entity read free, against 3 on the current head of #180315.
Error paths
Two things here, and both come from the same root — the suppress and the except clauses are not matched to what _get_or_update_cache can actually raise, which is a wider set than fetch_all_features could. Details inline.
A single change would cover both: keep the suppress narrow and assert the cache actually warmed, e.g. check service.is_cache_invalid() after the probe and raise UpdateFailed if it is still invalid — plus add PyViCareInvalidDataError to the UpdateFailed tuple.
Which fix for the beta
openviess/PyViCare#813 is the better one. Hoisting the override into ViCareCachedServiceBase makes both cached services symmetric and removes the need for Home Assistant to work around the asymmetry at all — and it sidesteps both error-path issues above, since the coordinator would keep calling fetch_all_features() rather than routing a probe through _get_or_update_cache. It needs a PyViCare release, though.
Weighed against that: 2026.9.0b0 ships ViCare completely broken. It carries #173776 and none of the fixes, so coordinator.py:49 raises AttributeError during coordinator construction and setup never even reaches the refresh — the integration does not load at all. Given that, landing the Home Assistant-side fix for b1 and treating openviess/PyViCare#813 as the follow-up once released seems the right trade, provided the two error-path points above are addressed first.
| # cached service the latter bypasses the cache, so every entity read | ||
| # would hit the API again, from the event loop. | ||
| with suppress(PyViCareNotSupportedFeatureError): | ||
| self._device.getProperty("device.serial") |
There was a problem hiding this comment.
_get_or_update_cache raises PyViCareInvalidDataError when the response has no data key, and that is in neither except clause, so a malformed payload surfaces as Unexpected error fetching vicare_… data with a traceback on every refresh instead of a clean UpdateFailed. The old raw fetch_all_features() never validated the payload, so this path is new here.
Malformed payload (no 'data'): this PR -> PyViCareInvalidDataError, mapped? False
previous -> no raise
Rate limit / invalid credentials: both -> mapped? True
Still better than the old behaviour, where the same payload detonated at the entity read inside the event loop — it just wants adding to the UpdateFailed tuple.
| # Read one property instead of calling fetch_all_features(): on the | ||
| # cached service the latter bypasses the cache, so every entity read | ||
| # would hit the API again, from the event loop. | ||
| with suppress(PyViCareNotSupportedFeatureError): |
There was a problem hiding this comment.
This suppress is broader than the device.serial-is-optional case it is aimed at. After clear_cache(), _get_or_update_cache converts PyViCareNotPaidForError into PyViCareNotSupportedFeatureError("PACKAGE_NOT_PAID_FOR"), which this swallows — so a wholly unpaid account gets a refresh that reports success with an empty cache:
this PR -> suppressed -> last_update_success True -> entities AVAILABLE
cache populated? False
entity read 1: PyViCareNotSupportedFeatureError | cumulative GETs: 2
entity read 2: PyViCareNotSupportedFeatureError | cumulative GETs: 3
previous -> PyViCareNotPaidForError escapes -> last_update_success False -> entities UNAVAILABLE
Two consequences. The condition becomes undiagnosable — entities stay available reading unknown, with nothing logged, where before they went unavailable. And because the cache never populates, _stringify_state does read self.state, so every entity read re-enters _get_or_update_cache and hits the API from the event loop — the blocking-call problem this PR fixes, reappearing on the unpaid path.
Checking that the cache actually warmed after the suppressed probe distinguishes the benign case from this one.
There was a problem hiding this comment.
Note for other reviewers:
I've added this to the beta milestone for now. As of b0, ViCare is broken and does not set up (because of #173776).
#180315 needs to be merged anyway as a follow-up for the above PR, but this PR here (#180344) is a temporary workaround until a library release with openviess/PyViCare#813 is made. When that happens, that should be pulled into the beta.
However, as it's unclear when that release can be made, we may want to merge this PR temporarily as a hotfix first. And as soon as the library bump is in, it can be reverted.
I'm aware we generally don't do this but as it's essentially a temporary one/two-line workaround, I'd be inclined to not revert #173776, as it does fix an annoying issue (if it worked).
Reading through the cache validates the payload, which the raw fetch_all_features() never did, so PyViCareInvalidDataError is a new error path on the refresh.
|
Finding 1 taken: Finding 2 declined. Failing the refresh on an empty cache is the behaviour we deliberately moved away from: on the first refresh The sub-point is right though. On the unpaid path the cache never warms, so entity reads keep hitting the API from the event loop. That is equally true for the per-gateway coordinator, so it belongs in #176163 rather than in a workaround that gets reverted once openviess/PyViCare#813 is released. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
homeassistant/components/vicare/coordinator.py:70
- Add a cache-aware regression test that verifies one coordinator fetch warms the cache and subsequent entity reads perform no requests. The updated fake still makes
clear_cache()a no-op and reads fixtures directly, so the failure mode this workaround addresses remains invisible and the current tests would pass if this call stopped warming the real PyViCare cache.
with suppress(PyViCareNotSupportedFeatureError):
self._device.getProperty("device.serial")
|
As the library PRs are merged now, I think we should move forward with that, instead of this PR. But we still have to wait for a new library version to be released. |
|
2.62.0 is out and #180395 is up. Verified live against it: refreshes succeed, no Closing, reopen if the bump cannot go into the beta. |
Proposed change
The coordinator refreshes with
clear_cache()+fetch_all_features(), which onViCareCachedServicedoes not warm the cache: that class inheritsViCareService.fetch_all_features(), which fetches and returns without writing_cache. OnlyViCareCachedServiceViaGatewayoverrides it, and Home Assistant never enables that mode.The payload is therefore discarded and every entity read fetches again. Since #173776
native_valueis a property instead of a syncupdate()run in the executor, so that second fetch happens in the event loop:Measured on a live installation with 5 devices: 49 of these per refresh, one per entity, and no state is written. The integration sets up and then never updates again.
Reading one property instead goes through the cached path, so a refresh costs one request and the entity reads are served from the cache.
This is a workaround. The proper fix is upstream in PyViCare, hoisting the override into
ViCareCachedServiceBase(openviess/PyViCare#813), which would let this call go back tofetch_all_features(). That needs a release, so this PR is the version that can make the 2026.9 beta on its own. Happy to drop it for the library fix if a release lands in time.Why this was missed
Both this and the setup crash in #180315 come from the same blind spot:
MockViCareServicedoes not behave like a real service.fetch_all_featureswas a bareMockaccepting any signature,clear_cacheis a no-op, andgetPropertyreads the fixture directly without consulting a cache. Cache behaviour and request counts are therefore invisible to the suite, which is why it stayed green while the real code path did the opposite of what the docstring claims.Stacked on #180315.
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: