Avoid stack traces for known advanced logs errors #5713
Conversation
Known advanced errors, like connection reset, should not lead to stack traces in the Supervisor logs. Let those errors bubble up to the API error handling. Fixes: #5606
📝 WalkthroughWalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant Logs as get_supervisor_logs
participant Handler as Exception Handler
Client->>Logs: Request supervisor logs
Logs->>Logs: Retrieve logs
alt APIError occurs
Logs->>Handler: Catch APIError
Handler-->>Logs: Re-raise APIError
Logs-->>Client: Propagate APIError
else Other Exception occurs
Logs->>Logs: Handle general exception (logged internally)
Logs-->>Client: Return handled error response
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/api/__init__.py (1)
406-409: Well-implemented specific exception handling for APIError.This change correctly implements specific handling for
APIErrorexceptions before the generic exception handler. By re-raising these specific errors, they can properly propagate to the API layer where they can be handled appropriately instead of being caught by the broad exception handler and generating stack traces in logs.The comment clearly explains the rationale - these are known errors like connection resets that should be handled at the API level rather than generating verbose stack traces.
A minor style suggestion (not critical): In Python, you can use just
raisewithout arguments to re-raise the current exception:- except APIError as err: - # Advanced logs are generally available, but we did encounter an error - # (e.g. connection reset). Those errors should bubble up to the API. - raise err + except APIError: + # Advanced logs are generally available, but we did encounter an error + # (e.g. connection reset). Those errors should bubble up to the API. + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/api/__init__.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (2)
supervisor/api/__init__.py (2)
12-12: Good import addition for better exception handling.Adding the
APIErrorimport is necessary for the specific exception handling implemented in theget_supervisor_logsfunction.
401-422:✅ Verification successful
Verify this change works with all error handling scenarios.
The implementation looks good, but it's worth verifying that all expected error types are properly categorized. This change assumes that any
APIErrorshould be propagated while other exceptions should be caught and handled with the fallback mechanism.Let's check if there are specific subclasses of
APIErrorthat might need different handling:
🏁 Script executed:
#!/bin/bash # Search for all APIError subclasses to ensure we're handling the right error types # Look for class definitions that inherit from APIError ast-grep --pattern 'class $name($_APIError, $_): $$$' # Alternate search using ripgrep echo "Alternate search:" rg "class.*\(.*APIError" --type py -A 1Length of output: 984
Final Verification: Error Handling is Correct
The error handling implementation in
supervisor/api/__init__.pyhas been verified against the defined APIError subclasses. The shell script confirmed that all expected subclasses (e.g.,APIForbidden,APINotFound,APIAddonNotInstalled,APIDBMigrationInProgress, as well asHomeAssistantAuthErrorandHomeAssistantWSError) derive fromAPIErrorand will therefore be propagated by the first exception clause. Non-APIErrorexceptions are correctly caught by the broad exception handler, logged, and then the fallback to Docker logs is applied—with special handling in place to excludeHostNotSupportedErrorfrom being captured by Sentry.
- APIError and subclasses: All errors are properly propagated.
- Fallback mechanism: Non-API errors are caught, logged, and then fallback logic is executed.
- Logging & Exception Capture: The conditional check for
HostNotSupportedErroris functioning as expected.This implementation meets the intended error handling coverage.
There was a problem hiding this comment.
I think the proper fix for the linked issue should be #5715 (but note that both PRs fix only the first error mentioned there, not #5606 (comment)).
I am unsure if we should silence the APIErrors though, do you have other examples where it makes sense?
Hm, yeah I think you are right, in that particular case the connection error happened on the response stream. I agree, I think this we can rather safely ignore. I wonder though, should we maybe try .. catch them and print a debug log? You might want to be aware during development that these connections are being dropped/resetted. 🤔
To be clear, it won't be entirely silenced: This will trigger I guess this would happen e.g. if |
Yeah, maybe, that wouldn't hurt. I'll update the other PR accordingly.
Right, I was a bit unclear by saying "silenced". It's been a while but IIRC the goal was to catch all those unexpected errors and report them to Sentry, because they should not occur in normal operation. And once we learn about the possible failures, add special handling only to those that we really don't care about (or better said - it's sufficient for them to be just logged). |
Agreed, and that is what Exception handling does here. That said, this actually should no longer be necessary since #5681: With that change, by default all unhandled exception will be reported to Sentry (if optted-in, that is). So if Sentry is the only argument, we could also remove this. That said, this does annotate the error a bit (
Yes, and I am considering Although, i wonder if that exception even can get raised systemd-journal-gatewayd side (so in But then again, in actual testing, by
...and I think we should try to handle this more gracefully. Now my fix is indeed wrong: APIError is not handled in this case, since we dont use the In any case, this PR is the wrong fix. So closing. We probably could try to reconnect "behind the scenes". But not sure if it's worth the effort. Maybe just logging as a warning that a client error while talking to systemd-joournal-gatewayd happened, and move on. |
Proposed change
Known advanced errors, like connection reset, should not lead to stack traces in the Supervisor logs. Let those errors bubble up to the API error handling.
Fixes: #5606
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit