Do not refresh OAuth token during Tibber unload - #176595
Conversation
async_unload_entry fetched a client via async_get_client, which performs a network OAuth token refresh and, when the token rotated, a full websocket reset, reconnect and resubscribe - all while tearing the entry down. Any exception escaping the unload path wedges the entry into the non-recoverable FAILED_UNLOAD state, and the EVENT_HOMEASSISTANT_STOP listener awaited rt_disconnect without a timeout. Disconnect the cached client instead, exposed via a read-only property on TibberRuntimeData, bound the disconnect with a 10 second timeout and log instead of raising so unload cannot fail because a dead websocket would not close politely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DERobfZZh3npFRbwpdxzz2
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
home-assistant
Bot
added
bugfix
cla-needed
has-tests
integration: tibber
small-pr
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @Danielhiversen, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
There was a problem hiding this comment.
Pull request overview
Prevents Tibber unloads from refreshing OAuth tokens or hanging on realtime disconnection.
Changes:
- Exposes the cached Tibber client.
- Adds bounded, non-raising realtime disconnect handling.
- Adds unload regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
homeassistant/components/tibber/__init__.py |
Uses cached clients and bounded disconnects during unload and shutdown. |
tests/components/tibber/test_init.py |
Tests token-refresh avoidance and disconnect failures. |
| @pytest.mark.parametrize( | ||
| "side_effect", | ||
| [ | ||
| pytest.param(TimeoutError(), id="timeout"), |
| return self._client | ||
|
|
||
|
|
||
| async def _async_disconnect_client(client: tibber.Tibber | None) -> None: |
Address the Copilot review comment on home-assistant#176595: raising a preconstructed TimeoutError only tested exception suppression and would still pass without the timeout bound. Make rt_disconnect hang on an event that is never set and patch the disconnect timeout, now extracted into the DISCONNECT_TIMEOUT constant, to 0.05 seconds so the asyncio.timeout deadline genuinely expires and cancels it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DERobfZZh3npFRbwpdxzz2
| async def _async_disconnect_client(client: tibber.Tibber | None) -> None: | ||
| """Disconnect the realtime connection without raising.""" | ||
| if client is None: | ||
| return | ||
| try: | ||
| async with asyncio.timeout(DISCONNECT_TIMEOUT): | ||
| await client.rt_disconnect() | ||
| except Exception: # noqa: BLE001 | ||
| _LOGGER.warning( | ||
| "Error disconnecting the Tibber realtime connection", exc_info=True | ||
| ) |
There was a problem hiding this comment.
Okay so I am wondering, why do we need to rt_disconnect and why does that require a token? I believe the token is a JWT token, and in theory you don't have to logout with such token as it will just expire. Is there a way we can modify the library to not need a token to disconnect?
There was a problem hiding this comment.
Good instinct - disconnecting doesn't need a token at all, and that's essentially what this PR fixes.
rt_disconnect() in pyTibber takes no credentials: it just closes the websocket and stops the watchdog task. The token involvement in the current code is incidental - async_unload_entry calls
runtime_data.async_get_client(hass) merely to obtain the client object, and that helper unconditionally refreshes the OAuth token over the network and, if the token rotated, calls set_access_token(), which tears down and
rebuilds the websocket in the middle of the unload. So the unload was paying for a token it never needed, and any failure or hang in that path wedges the entry into FAILED_UNLOAD.
This PR sidesteps that by disconnecting the cached client directly (via the new client property), so unload is now token-free and network-free apart from the socket close itself - no library change needed.
As for why we disconnect at all: the realtime websocket and its watchdog task would otherwise keep running after unload, and a reload builds a fresh client, so the old connection would leak. The asyncio.timeout bound is there because a half-dead websocket can hang in wait_closed() — that's the deadlock behind the FAILED_UNLOAD reports in #168480/#166228. Bounding the close on the library side too is proposed separately in Danielhiversen/pyTibber#435.
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Right. Hmm, I think it's now weird to have 1 method to get the client, and one property to get the client. Given that the tibber client already got a reference to the refresh access token method, I would assume it would keep it up to date. However, I think we should simplify the way we fetch the client, so in the future we don't have places where we get an unauthenticated client where we want an authenticated one, or that we fetch a token when we don't want to
|
Good point on the two accessors. I've collapsed it: there's now a single async_get_client() (always authenticated), the client property is gone, and disconnecting on unload/shutdown is an async_disconnect() action on the runtime data rather than Worth flagging the constraint: in pyTibber 0.37.6 the refresh_access_token callback is only wired to the realtime websocket reconnect (realtime.py), not to the GraphQL/Data-API REST calls – those send the stored _access_token with no refresh-on-401. So the per-fetch re-auth in async_get_client() is doing real work: it's what keeps the polling coordinators authenticated after the OAuth token rotates. I've kept it and added a test that rotates the token to lock that in. A true create-once-never-reauth client would need pyTibber itself to refresh its REST token via the callback – that's a separate library change (tracked in Danielhiversen/pyTibber#435) and out of scope here. This PR stands on its own with the |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
I like the direction this is moving in, small comment on the tests
| @pytest.mark.usefixtures("recorder_mock") | ||
| async def test_async_get_client_propagates_rotated_token(hass: HomeAssistant) -> None: | ||
| """Ensure a rotated OAuth token is pushed to the cached client on next fetch.""" | ||
| session = MagicMock() | ||
| session.async_ensure_token_valid = AsyncMock() | ||
| session.token = {CONF_ACCESS_TOKEN: "token-1"} | ||
|
|
||
| runtime = TibberRuntimeData( | ||
| session=session, | ||
| ) | ||
|
|
||
| with patch("homeassistant.components.tibber.tibber.Tibber") as mock_client_cls: | ||
| mock_client = MagicMock() | ||
| mock_client.set_access_token = AsyncMock() | ||
| mock_client_cls.return_value = mock_client | ||
|
|
||
| await runtime.async_get_client(hass) | ||
| mock_client_cls.assert_called_once_with( | ||
| access_token="token-1", | ||
| websession=ANY, | ||
| time_zone=ANY, | ||
| ssl=ANY, | ||
| refresh_access_token=ANY, | ||
| ) | ||
|
|
||
| session.token = {CONF_ACCESS_TOKEN: "token-2"} | ||
| await runtime.async_get_client(hass) | ||
|
|
||
| mock_client_cls.assert_called_once() | ||
| mock_client.set_access_token.assert_awaited_once_with("token-2") |
There was a problem hiding this comment.
We should not touch the runtime data directly, instead we should do that via the integration
There was a problem hiding this comment.
Good point - those tests asserted on TibberRuntimeData internals rather than anything reachable from outside the integration. All three now go through the config entry.
The rotation test updates the stored OAuth token with async_update_entry, then triggers a client fetch through the notify entity. That last bit looks arbitrary, so the obvious trigger would be a coordinator tick, but the mocked client reports no homes, so nothing subscribes to the Data API coordinator and it never schedules a periodic refresh – the tick fires and nothing happens. notify.send_message is the shortest public path that reaches async_get_client, and test_notify.py already drives it that way.
The missing-token case now asserts the entry ends in SETUP_ERROR with a reauth flow started, rather than catching ConfigEntryAuthFailed from the helper.
I've converted the two neighbouring tests you didn't flag as well - same pattern, and they'd only have come back next round.
One fixture change in conftest.py: the patched tibber.Tibber class is now exposed as tibber_client_cls so a test can assert on construction, while tibber_mock still yields the client instance. No other test file needed changing.
Address joostlek's review on home-assistant#176595: the runtime data tests reached past the integration and asserted on TibberRuntimeData internals. Drive them through config entry setup instead, so they exercise the same surface a user does. The token rotation test now updates the stored OAuth token on the config entry and triggers a client fetch through the notify entity, rather than constructing a runtime object and calling async_get_client directly. The missing token test asserts the entry ends in SETUP_ERROR with a reauth flow, which is the observable outcome, instead of catching ConfigEntryAuthFailed from the helper. A coordinator tick cannot drive these tests: the mocked client reports no homes, so nothing subscribes to the Data API coordinator and it never schedules a periodic refresh. Split the patched Tibber class out of the tibber_mock fixture as tibber_client_cls so tests can assert on construction. tibber_mock still yields the client instance, so other tests are unaffected.
| async with asyncio.timeout(DISCONNECT_TIMEOUT): | ||
| await self._client.rt_disconnect() |
There was a problem hiding this comment.
That's right, and worth spelling out. rt_disconnect() reaches _reset_connection(), which takes LOCK_CONNECT before _reset_connection_locked() cancels the watchdog. reconnect() holds that same lock, and gets to it through an unbounded token refresh. So if the watchdog is mid-reconnect when the entry unloads, the timeout cancels the lock acquisition rather than the disconnect, and the watchdog survives.
It's a deliberate trade rather than a fault this PR introduces. Before it, the unload awaited rt_disconnect() unbounded: the watchdog did always stop eventually, but a half-dead socket could block the unload indefinitely and leave the entry in FAILED_UNLOAD, which needs a Home Assistant restart to clear. This swaps a state the user can't recover from for a task that can leak in a narrow window, and I'd rather leak the task.
The force-stop path you describe belongs in pyTibber rather than here - the integration can't cancel the library's watchdog without reaching into private attributes. Part of it is already open as Danielhiversen/pyTibber#435, which makes the connect lock per-instance and bounds both wait_closed() and close_async(). That narrows the window without closing it: there's still no cancellation path that runs before the lock is taken, and the token refresh inside reconnect() is unbounded.
Breaking change
Proposed change
async_unload_entrycurrently obtains the client viaruntime_data.async_get_client(hass)— solely to disconnect it. That call performs a network OAuth token refresh (OAuth2Session.async_ensure_token_valid()), and when the token has rotated it callsTibber.set_access_token(), which does a full realtime websocket reset, reconnect and resubscribe — all in the middle of tearing the entry down. Any exception escaping here (flaky Tibber API, auth failure), or a hang in the unboundedrt_disconnect()close path on a half-dead websocket, wedges the entry into the non-recoverableFAILED_UNLOADstate, from which only a Home Assistant restart recovers.This change:
async_get_client(), which is always authenticated, and adds anasync_disconnect()action onTibberRuntimeDatafor teardown,async_unload_entrydisconnect the cached client directly (no network round-trip, no token refresh, no websocket rebuild during teardown),asyncio.timeoutand logs instead of raising, so a dead websocket that won't close politely cannot fail the unload,EVENT_HOMEASSISTANT_STOPhandler.Real-world reports of reloads wedging into
FAILED_UNLOADwith this integration: #168480 (several comments), #166228 (March comment with the full traceback chain), #176268 (the realtime-drop scenario in which users attempt the reload).Test evidence:
pytest tests/components/tibber/→ 93 passed. The new and changed tests fail against unpatched code, verified by reverting the component and re-running:test_entry_unloadassertsset_access_tokenis not called during unload, a parametrisedtest_entry_unload_rt_disconnect_errorasserts the entry reachesNOT_LOADEDeven whenrt_disconnecthangs past the deadline or raises, andtest_rotated_token_pushed_to_cached_clientasserts a rotated OAuth token reaches the cached client without rebuilding it. The runtime-data tests now drive the integration through its config entry rather than constructingTibberRuntimeDatadirectly.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: