fix: bound supervisor log fetch time - #2281
Conversation
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughSupervisor log retrieval now enforces the configured timeout across client creation and request execution. Timeout handling covers built-in and HTTPX exceptions. A regression test verifies that a permanently stalled request raises ChangesSupervisor log timeout
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Supervisor-backed log retrieval now has an overall deadline to prevent indefinite hangs while preserving existing log selection and response behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Set the draft to “Ready for Review”; Codex will then run another test, after which a supervisor will perform a final review. |
|
Always have your AI review all parts of a Coderabbit comment—not just the threads, but the comment itself, which also contains important information. |
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Patch76
left a comment
Patch76
left a comment
There was a problem hiding this comment.
Thanks for this. Bounding the whole fetch instead of only the per-phase read is the right shape for #2279, and putting it in _supervisor_logs_get covers all three in-addon log paths from one place (src/ha_mcp/client/rest_client.py:624, :896, :923). Catching both TimeoutError and httpx.TimeoutException is necessary rather than redundant, since the httpx exception is not a subclass of the builtin. The install in the report is add-on, so the reported path is genuinely covered.
Two questions and two notes before I would approve.
[Concern 1] What Fixes #2279 leaves behind. The issue lists two suggestions, and this implements the first. Without the second — a bounded window — the reported call does not start working again; it surfaces as a timeout error instead of a hang whenever the fetch exceeds HA_TIMEOUT. Nothing can shrink that window today: _get_error_log calls get_error_log() with no arguments (src/ha_mcp/tools/tools_utility.py:778), the client method takes none (rest_client.py:582), and _ERROR_LOG_LINES is hardcoded at 20 000 (rest_client.py:58). Fail-fast beats hanging, so I am not asking you to widen the PR — but is the window meant as a follow-up? If so, Part of #2279 in the body would keep the issue open for it.
[Concern 2] The deadline lands on more than the error log, and shares a knob with the per-phase timeout. asyncio.timeout(self.timeout) gives the whole fetch the budget one phase used to have (HA_TIMEOUT, 30 s by default, src/ha_mcp/config.py:56), and it sits in the shared helper, so it also bounds ha_get_logs(source="supervisor") and source="system_service"), which request up to SUPERVISOR_SEARCH_WINDOW_LINES = 2000 lines (tools_utility.py:65). A large add-on or host log fetch that used to complete slowly now fails at 30 s, and raising the ceiling also loosens every per-phase timeout on the Core client. Did you consider a separate budget for log fetches? The PR body's risk boundary mentions the error-log path only; those two tools are worth naming there either way.
Note: the two non-add-on branches of get_error_log keep the construct this PR replaces. rest_client.py:632 requests the same 20 000-line window through _raw_request, whose client is built with timeout=httpx.Timeout(self.timeout) at rest_client.py:260-268; rest_client.py:639 has no lines at all but is equally without an overall deadline. Both are outside what the issue reports, so leaving them is defensible — I mention it so the decision is explicit rather than implicit.
Note: asyncio.timeout raises a bare TimeoutError, so {e} at rest_client.py:838 interpolates to nothing and the deadline case reads Timeout fetching /core/logs from Supervisor: . Naming the budget instead (... after {self.timeout}s) would make that line worth reading. The new test is bounded by the 300 s default in tests/pytest.ini:88, so a regression does fail rather than hang, but it costs a worker five minutes to get there — @pytest.mark.timeout(10) would make the guard snap shut.
Elioooon
left a comment
Elioooon
left a comment
There was a problem hiding this comment.
Thanks for the detailed review. I pushed 2e2f308e and updated the PR body.
Changes made:
- changed the PR from
Fixes #2279toPart of #2279and called out that the bounded log window is follow-up work - expanded the risk boundary to name the shared Supervisor-direct helper users: add-on logs, add-on
error_log, and add-onsupervisor/system_service - made builtin
TimeoutErrormessages include the configured budget instead of rendering as an empty suffix - added
@pytest.mark.timeout(10)to the stalled-fetch regression test
Validated locally:
uv run pytest tests/src/unit/test_tools_utility_supervisor_logs.py -quv run ruff check src/ha_mcp/client/rest_client.py tests/src/unit/test_tools_utility_supervisor_logs.pyuv run ruff format --check src/ha_mcp/client/rest_client.py tests/src/unit/test_tools_utility_supervisor_logs.pyuv run mypy src/ha_mcp/client/rest_client.py
I kept the timeout knob unchanged here; adding a separate log-fetch budget is larger surface area than this PR needs, and the PR now states that boundary explicitly.
|
CI note: I checked the two failed beta jobs against their uploaded diagnostics.
This PR only adds an overall timeout around Supervisor log retrieval in |
|
Don't worry about the beta lane failures, we'll handle those on our side |
Patch76
left a comment
Patch76
left a comment
There was a problem hiding this comment.
Three of the four items from the last round landed: the body now reads Part of #2279, the risk boundary names the three callers of the shared helper, and @pytest.mark.timeout(10) makes the regression test snap shut instead of riding the 300 s default in tests/pytest.ini:88. Declining a separate log-fetch budget is a reasonable call — it widens the surface well past the hang this PR fixes. The note about the two non-add-on branches of get_error_log (src/ha_mcp/client/rest_client.py:632, :639) stands as it was, which is fine; they are outside what #2279 reports.
The fourth item, the empty interpolation in the timeout message, went the wrong way.
[Concern 1] The type(e).__name__ fallback is unreachable. src/ha_mcp/client/rest_client.py:839 renders {e or type(e).__name__}. TimeoutError and httpx.TimeoutException define neither __bool__ nor __len__, so e is always truthy and the right-hand side never evaluates. asyncio.timeout raises TimeoutError() with empty args, so the deadline case — the one this PR adds — still renders as Timeout fetching /addons/core_mosquitto/logs from Supervisor after 30s: , with a dangling colon and nothing after it. Worth naming because it is the near miss: {str(e)} on its own renders that same empty tail. The form that works is the full {str(e) or type(e).__name__}, which four other sites in src/ha_mcp/ already use. The closest is src/ha_mcp/tools/tools_yaml_read.py:170 — same shape, inside an f-string right after a colon, with a docstring that names the same symptom: a warning reading "was not searched: ." names no reason at all. src/ha_mcp/tools/tools_search.py:2336 carries a comment naming the trap for asyncio.TimeoutError specifically.
Note: test_supervisor_log_fetch_has_overall_deadline asserts only "Timeout" in str(exc_info.value) (tests/src/unit/test_tools_utility_supervisor_logs.py:457), which matches the static prefix and so holds with or without the message change in 2e2f308e. Asserting the rendered string — the budget value and a non-empty cause included — would pin it.
|
Addressed the latest review in 20c09b8: the timeout detail now uses Validation:
|
Patch76
left a comment
Patch76
left a comment
There was a problem hiding this comment.
20c09b84 closes the last open item. src/ha_mcp/client/rest_client.py:839 now interpolates {str(e) or type(e).__name__}, so the deadline case — asyncio.timeout raising TimeoutError() with empty args — renders a cause instead of stopping at the colon, and tests/src/unit/test_tools_utility_supervisor_logs.py:457 asserts "after 0.01s: TimeoutError", which pins the budget interpolation and a non-empty cause in one string rather than only the static prefix. The old "Timeout" in ... form matched the empty-tail message too, so this is the assertion that would have caught it.
Re-checked the rest of the change at the current head rather than carrying the earlier round forward: asyncio was already imported (rest_client.py:5); async with asyncio.timeout(self.timeout) wraps both the client construction and the get(), so a response that never completes is bounded and not just the individual httpx phases; self.timeout is never None on either constructor path (rest_client.py:241, :248), so the wrap cannot degrade to "no deadline"; and the except (TimeoutError, httpx.TimeoutException) arm sits ahead of except httpx.HTTPError. That order is load-bearing, since httpx.TimeoutException inherits from httpx.HTTPError — reversed, an httpx timeout would render as a transport error.
Scope note, so the boundary is on the record rather than implied: the Core-proxy branches of get_addon_logs, _get_system_service_logs and get_error_log still go through _raw_request, which is bounded per httpx phase only, so a body that trickles indefinitely is not covered there — the same class this PR fixes for the Supervisor-direct helper. The PR body already records that boundary ("It does not change the HA Core proxy log paths"), and #2279 reports an add-on install, so I am not asking you to widen it here.
I did not run the suite locally — this is an external contribution, so I read the test and left execution to CI. Approving on the code; the remaining required lanes on the merge head are still in flight, so the merge itself waits on them.
|
Now part of HA-MCP ... congratz! |
🧪 Your changes are now in the dev channel!Your PR has been merged to master and is available for testing in the dev channel. Test your changes before the next stable release (biweekly Wednesday): Quick start# Run dev version
uvx ha-mcp-dev
# Check version
uvx ha-mcp-dev --versionDocker: docker pull ghcr.io/homeassistant-ai/ha-mcp:dev
docker run --rm -i \
-v ha-mcp-dev-data:/home/mcpuser/.ha-mcp \
-e HOMEASSISTANT_URL=http://your-ha:8123 \
-e HOMEASSISTANT_TOKEN=your_token \
ghcr.io/homeassistant-ai/ha-mcp:devFound an issue? Please open a new bug report and mention this PR for context. |
4 participants
What does this PR do?
Part of #2279.
Supervisor-backed log fetches now have an overall deadline in addition to httpx's per-phase timeout. This prevents
ha_get_logs(source="error_log")and the other Supervisor-direct log paths from hanging indefinitely when the response never completes but also never trips httpx's read timeout.This PR intentionally does not shrink the requested error-log window; that remains follow-up work for making very large logs complete instead of timing out.
Type of change
Testing
uv run pytest)uv run ruff check)Validated with:
uv run pytest tests/src/unit/test_tools_utility_supervisor_logs.py -quv run pytest tests/src/unit/test_rest_client_get_error_log.py tests/src/unit/test_tools_utility_supervisor_logs.py -quv run ruff check src/ha_mcp/client/rest_client.py tests/src/unit/test_tools_utility_supervisor_logs.pyuv run ruff format --check src/ha_mcp/client/rest_client.py tests/src/unit/test_tools_utility_supervisor_logs.pyuv run mypy src/ha_mcp/client/rest_client.pyRisk boundary: this bounds the shared Supervisor-direct log helper used by the add-on container log path,
ha_get_logs(source="error_log")on add-on installs, andha_get_logs(source="supervisor"|"system_service")on add-on installs. It does not change the HA Core proxy log paths, the requested log window, parsing, or tool response shaping.Checklist
Summary by CodeRabbit