anthemav: add connect timeout so setup fails fast instead of blocking bootstrap - #181401
anthemav: add connect timeout so setup fails fast instead of blocking bootstrap#181401luck-y13 wants to merge 3 commits into
Conversation
anthemav.Connection.create() retries the initial TCP connection internally (with its own exponential backoff, capped at 300s between attempts) and only returns once it succeeds, so it never raises OSError on its own when the receiver is unreachable/powered off. Nothing bounds that wait in async_setup_entry today, so setup blocks indefinitely. At startup this is worse than just a slow boot for this one entry: the only thing that ever stops it is Home Assistant's own global bootstrap stage-2 timeout (~5 minutes), and when that fires it force-cancels whatever other config entries are still mid-setup alongside it too — so one anthemav receiver being off at boot can take down unrelated integrations that would otherwise have set up fine, with no error logged against them individually (only anthemav's own cancellation is logged, from bootstrap's perspective it's the one entry it was still explicitly waiting on). Wrap the connection attempt in asyncio.timeout(CONNECT_TIMEOUT_SECONDS) and treat TimeoutError the same as OSError/DeviceError: raise ConfigEntryNotReady so Home Assistant's normal per-entry retry/backoff takes over instead. This is the same pattern already used by several other integrations with a similar "connect once during setup" shape (matter, cambridge_audio, music_assistant, zwave_js, hue, lutron_caseta, ...). 10s was chosen as generous for a LAN TCP connect while still well inside a boot that won't be noticeably slower when the receiver is reachable. Added a test (test_config_entry_not_ready_when_connect_hangs) covering the new timeout path, alongside the existing test_config_entry_not_ready_when_oserror.
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @Hyralex, 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.
🟡 Changes recommended
The new AsyncMock side effect does not actually block, so the timeout test fails to exercise its intended path, and the PR template is incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a bounded connection timeout so unreachable Anthem receivers defer setup without blocking bootstrap.
Changes:
- Adds a 10-second connection timeout.
- Converts timeout failures into
ConfigEntryNotReady. - Adds timeout-path test coverage.
File summaries
| File | Description |
|---|---|
homeassistant/components/anthemav/__init__.py |
Applies and handles the connection timeout. |
homeassistant/components/anthemav/const.py |
Defines the timeout duration. |
tests/components/anthemav/test_init.py |
Tests setup retry behavior. |
Review details
Suppressed comments (1)
tests/components/anthemav/test_init.py:95
- Use an async side effect that is actually awaited so this test reaches the timeout path. Because
Connection.createis patched as anAsyncMock, this synchronous lambda returns thesleep()coroutine as its result; the mock await therefore completes immediately and setup fails atavr.protocolinstead of timing out.
side_effect=lambda *args, **kwargs: asyncio.sleep(3600),
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # anthemav.Connection.create() retries the initial connection | ||
| # internally with its own backoff and only returns once it | ||
| # succeeds, so it will not raise OSError on its own when the | ||
| # receiver is unreachable. Bound the attempt so an unreachable | ||
| # receiver fails fast into the normal ConfigEntryNotReady retry | ||
| # path, rather than blocking setup (and, at startup, Home | ||
| # Assistant's bootstrap) for as long as the receiver stays off. |
| # anthemav.Connection.create() retries its initial connection attempt | ||
| # internally (with exponential backoff) and only returns once it succeeds, | ||
| # so it does not fail on its own when the receiver is unreachable. Bound it | ||
| # here instead of relying on Home Assistant's global bootstrap timeout, | ||
| # which would otherwise let one unreachable-at-boot receiver block startup | ||
| # for minutes and can collaterally cancel other integrations still setting | ||
| # up alongside it. |
| """Test setup fails fast (instead of hanging) when the AVR never connects. | ||
|
|
||
| anthemav.Connection.create() retries its initial connection internally | ||
| and only returns once it succeeds, so it never raises OSError on its | ||
| own when the receiver is unreachable — nothing bounds that wait except | ||
| our own timeout. Simulate that by having the mocked create() hang | ||
| indefinitely, and confirm setup still resolves to SETUP_RETRY rather | ||
| than blocking forever. | ||
| """ |
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
The side_effect was a plain lambda returning asyncio.sleep(3600) — since AsyncMock calls that synchronously and awaits its return value, the sleep() coroutine was returned but never awaited, so the mock resolved immediately without actually exercising the timeout path. Use an async function instead so the sleep is genuinely awaited. Verified in isolation (see PR comment) that the original version returns instantly while the fixed version now correctly triggers asyncio.timeout()'s TimeoutError.
The same explanation of why CONNECT_TIMEOUT_SECONDS exists was repeated across the implementation, the constant, and the test docstring. Keep it in one place (the constant) and just point there from the call site; trim the test docstring to its behavior contract.
|
Addressed Copilot's review:
Pushed both as separate commits so the history shows what changed and why. |
There was a problem hiding this comment.
🔵 Needs a closer look
The required pull-request template is incomplete, and several added comments should be condensed.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
homeassistant/components/anthemav/init.py:65
- Restore the complete pull-request template before merge. The description omits multiple unchecked type, additional-information, checklist, and device-integration items, which must remain present even when they do not apply.
homeassistant/components/anthemav/init.py:49
- Condense this explanation to the single non-obvious constraint. The seven-line block repeats the constant comment and PR description for one timeout operation, making the implementation harder to scan.
# See CONNECT_TIMEOUT_SECONDS for why this needs a timeout.
homeassistant/components/anthemav/const.py:10
- Condense this constant comment to one line. The detailed bootstrap narrative is duplicated at the call site and will be harder to keep synchronized.
# anthemav.Connection.create() retries internally and only returns once
tests/components/anthemav/test_init.py:79
- Shorten this test documentation to a one-line behavior description. The multi-paragraph docstring and helper docstring narrate the mock implementation rather than adding a non-obvious testing constraint.
"""Test setup fails fast (instead of hanging) when the AVR never connects."""
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description omits required template fields and checklist items.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
homeassistant/components/anthemav/init.py:50
- Restore the complete pull request template before merging. The description removes the unchecked “Type of change” options, required “Additional information” fields, and multiple checklist items; repository instructions require all template sections and checkboxes to remain present even when unchecked.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
blues-sechseck
left a comment
blues-sechseck
left a comment
There was a problem hiding this comment.
I read through this while looking at how other integrations bound their setup connect, and checked the premise against anthemav 1.4.2 itself. It holds: Connection.create() calls conn.reconnect(), which loops until it succeeds and backs off to 300 s between attempts, and only re-raises OSError when auto_reconnect is false — which it isn't here, since the integration takes the default. So nothing bounds that wait today and a timeout is the right shape.
Two things I noticed, one worth changing before merge.
The new message is attached to DeviceError too, where it isn't true. wait_for_device_initialised() raises DeviceError, converting its own timeout internally — so adding TimeoutError to the tuple doesn't change that path, but the message does. That case means the TCP connection succeeded and the receiver simply didn't report model and MAC within DEVICE_TIMEOUT_SECONDS, and it now logs "Unable to connect to Anthem AVR at host:port", pointing the user at the network for a problem that isn't there. Either split it:
except TimeoutError as err:
raise ConfigEntryNotReady(
f"Timed out connecting to Anthem AVR at "
f"{entry.data[CONF_HOST]}:{entry.data[CONF_PORT]}"
) from err
except (OSError, DeviceError) as err:
raise ConfigEntryNotReady from error word the shared message so it is true for both, e.g. "Anthem AVR at host:port is not ready".
The timed-out connection is never closed. Optional, and not introduced here. When the timeout fires, avr was never assigned, so there is no handle to close(). In the case you are targeting this costs nothing — the receiver is off, create_connection keeps failing, nothing is open. But if it connects just after the deadline, the Connection is left with a live transport and auto_reconnect=True; its connection_lost callback reconnects forever with nobody holding a reference, and its update_callback still fires async_dispatcher_send for an entry that failed setup. Each ConfigEntryNotReady retry can add one. The DeviceError path has had the same shape all along, so I would not hold the PR for it — but Connection.create() does take auto_reconnect, if you want to build the connection yourself and keep the handle.
For what it is worth, I ran the tests: on this branch pytest tests/components/anthemav gives 13 passed, on unmodified dev 12. Both runs show the same 9 errors, which are a translation-lookup artefact of my checkout and unrelated to the change. The new test does exercise the hang rather than an early raise, and it follows the patching style already used in that file.
|
Follow-up: I found a way to actually execute this repo's test suite (a sandboxed environment, which — despite my intent — turned out to still be Windows rather than Linux, so this isn't equivalent to a clean CI run, but it is a real, unmocked execution of the actual test code rather than manual tracing). All 13 tests in
I'd still expect (and recommend) a real Linux/CI confirmation before merge, but wanted to close the loop on the "couldn't verify tests pass" caveat from my original submission — this is as close as I could get to that on the machine available to me. |
Proposed change
anthemav.Connection.create()retries the initial TCP connectioninternally (its own exponential backoff, capped at 300s between attempts)
and only returns once it succeeds — it never raises
OSErroron its ownwhen the receiver is unreachable or powered off. Nothing in
async_setup_entrybounds that wait today, so setup blocks indefinitelywhenever the receiver isn't reachable.
At startup this is worse than just a slow boot for this one entry: the only
thing that currently stops it is Home Assistant's own global bootstrap
stage-2 timeout (~5 minutes), and when that fires it force-cancels whatever
other config entries are still mid-setup alongside it too. I ran into this
directly — an Anthem receiver that's normally powered off caused this
integration to block boot for the full 5 minutes, and when the global
timeout finally fired it collaterally killed a separate, unrelated
integration (a water-leak monitor) that was still finishing its own setup
at that moment, with no error logged against that integration individually
— only anthemav's own cancellation is logged, since from bootstrap's
perspective it's the one entry it was still explicitly waiting on.
This wraps the connection attempt in
asyncio.timeout(CONNECT_TIMEOUT_SECONDS)and treats
TimeoutErrorthe same as the existingOSError/DeviceErrorhandling: raise
ConfigEntryNotReadyso Home Assistant's normal per-entryretry/backoff takes over, instead of blocking bootstrap. This is the same
shape already used by several other integrations with a similar
"connect once during setup" pattern —
matter,cambridge_audio,music_assistant,zwave_js,hue,lutron_caseta, and others all wraptheir initial connect in
asyncio.timeout(...)and raiseConfigEntryNotReadyonTimeoutError.10s was chosen as generous for a LAN TCP connect while not adding
noticeable delay to a normal boot where the receiver is reachable.
Type of change
Additional information
when it isn't, setup now resolves to
SETUP_RETRYwithin ~10s instead ofhanging (and, at boot, instead of consuming up to 5 minutes of the global
bootstrap timeout).
Checklist
environment after all (see comments below) —
pytest tests/components/anthemav/ -vpasses all 13 tests, including the newtest_config_entry_not_ready_when_connect_hangs. That run was still onWindows rather than Linux, with a documented set of OS-specific
workarounds (none touching test/implementation logic) — see the
comment thread for exact commands and caveats. Recommend a Linux/CI
confirmation too before merge.
above.
(
test_config_entry_not_ready_when_connect_hangs, alongside theexisting
test_config_entry_not_ready_when_oserror).done — will do before/while this is in review if that's expected.
🤖 Generated with Claude Code