Re-add typeerror handling to format message #5704
Conversation
📝 WalkthroughWalkthroughThis pull request converts synchronous error message formatting to an asynchronous approach. In the API utility, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as api_return_error
participant Logger as async_format_message
participant Sentry as async_capture_exception
Client->>API: Trigger error (DockerAPIError)
API->>Logger: Call async_format_message(message)
Logger->>Logger: Try formatting message
alt Formatting error (TypeError)
Logger->>Sentry: Capture exception asynchronously
end
Logger-->>API: Return formatted/error message
API-->>Client: Return error response
✨ 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: 3
🧹 Nitpick comments (1)
tests/utils/test_log_format.py (1)
24-27: Typo in test function name.There is a typo in the function name:
test_exeptionshould betest_exception.-async def test_exeption(): +async def test_exception():Also note that unlike the other test functions, this one is correctly declared as async since it calls an async function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supervisor/api/utils.py(2 hunks)supervisor/utils/log_format.py(1 hunks)tests/utils/test_log_format.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 (3)
tests/utils/test_log_format.py (1)
3-3: Import statement updated correctly.The import has been correctly updated to use the new
async_format_messagefunction instead of the removedformat_message.supervisor/api/utils.py (1)
28-28: Import statement updated correctly.The import has been correctly updated to use the new
async_format_messagefunction.supervisor/utils/log_format.py (1)
3-3: Added necessary imports for async functionality.Correctly added imports for
asyncioandasync_capture_exceptionfrom Sentry to support the new error handling functionality.Also applies to: 7-8
|
|
||
|
|
||
| def test_format_message_port(): | ||
| """Tests for message formater.""" | ||
| message = '500 Server Error: Internal Server Error: Bind for 0.0.0.0:80 failed: port is already allocated")' | ||
| assert ( | ||
| format_message(message) | ||
| async_format_message(message) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Function calls updated but missing await.
The test is calling the new async_format_message function but isn't awaiting it. According to the implementation docstring, this function "Must be called from event loop" but these test functions aren't async.
Either:
- The non-async test functions should be updated to be async and use await
- The implementation should clarify if it's safe to call without await in some contexts
Also applies to: 19-19
🏁 Script executed:
#!/bin/bash
# Check if async_format_message is being awaited in other places in the codebase
rg "await async_format_message" --type pyLength of output: 43
Await the Async Call in Tests
The tests are invoking async_format_message without awaiting its result, which contradicts its implementation requirement ("Must be called from event loop"). This occurs at both line 10 and line 19 of tests/utils/test_log_format.py. You need to either:
- Update the test functions to be async and use
await async_format_message(message), or - Clarify in the function's docstring that it’s safe to call without awaiting in test contexts.
| @@ -139,7 +139,7 @@ def api_return_error( | |||
| if error and not message: | |||
| message = get_message_from_exception_chain(error) | |||
| if check_exception_chain(error, DockerAPIError): | |||
| message = format_message(message) | |||
| message = async_format_message(message) | |||
There was a problem hiding this comment.
Function call updated but missing await.
The code is calling async_format_message but isn't awaiting it. According to the implementation docstring, this function "Must be called from event loop".
Since api_return_error isn't an async function, you should either:
- Make
api_return_errorasync and await the call - Run the function in the event loop using
asyncio.run_coroutine_threadsafeor similar
This is especially important for error handling, as any exceptions from TypeError might be lost if not properly awaited.
| def async_format_message(message: str) -> str: | ||
| """Return a formated message if it's known. | ||
|
|
||
| Must be called from event loop. | ||
| """ | ||
| try: | ||
| match = RE_BIND_FAILED.match(message) | ||
| if match: | ||
| return f"Port '{match.group(1)}' is already in use by something else on the host." | ||
| except TypeError as err: | ||
| _LOGGER.error("The type of message is not a string - %s", err) | ||
| asyncio.get_running_loop().create_task(async_capture_exception(err)) | ||
|
|
||
| return message |
There was a problem hiding this comment.
Function implementation issue with async pattern.
The implementation has several issues:
- The function isn't declared as async even though it creates an async task and requires an event loop
- It doesn't await the
async_capture_exceptiontask which could lead to lost exceptions - The docstring states it "Must be called from event loop" but doesn't explain how to properly call it
These issues could lead to unexpected behavior when called from various contexts. The current implementation is neither truly synchronous nor properly asynchronous.
Consider one of these options:
# Option 1: Make it truly async and await the exception capture
-def async_format_message(message: str) -> str:
+async def async_format_message(message: str) -> str:
"""Return a formated message if it's known.
Must be called with await.
"""
try:
match = RE_BIND_FAILED.match(message)
if match:
return f"Port '{match.group(1)}' is already in use by something else on the host."
except TypeError as err:
_LOGGER.error("The type of message is not a string - %s", err)
- asyncio.get_running_loop().create_task(async_capture_exception(err))
+ await async_capture_exception(err)
return message# Option 2: Keep it synchronous with better naming and handling
-def async_format_message(message: str) -> str:
+def format_message_with_error_capture(message: str) -> str:
"""Return a formated message if it's known.
This function attempts to run in an event loop context to capture exceptions,
but will still work if no event loop is available.
"""
try:
match = RE_BIND_FAILED.match(message)
if match:
return f"Port '{match.group(1)}' is already in use by something else on the host."
except TypeError as err:
_LOGGER.error("The type of message is not a string - %s", err)
try:
asyncio.get_running_loop().create_task(async_capture_exception(err))
except RuntimeError:
# No event loop available, just log the error
pass
return message📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def async_format_message(message: str) -> str: | |
| """Return a formated message if it's known. | |
| Must be called from event loop. | |
| """ | |
| try: | |
| match = RE_BIND_FAILED.match(message) | |
| if match: | |
| return f"Port '{match.group(1)}' is already in use by something else on the host." | |
| except TypeError as err: | |
| _LOGGER.error("The type of message is not a string - %s", err) | |
| asyncio.get_running_loop().create_task(async_capture_exception(err)) | |
| return message | |
| async def async_format_message(message: str) -> str: | |
| """Return a formated message if it's known. | |
| Must be called with await. | |
| """ | |
| try: | |
| match = RE_BIND_FAILED.match(message) | |
| if match: | |
| return f"Port '{match.group(1)}' is already in use by something else on the host." | |
| except TypeError as err: | |
| _LOGGER.error("The type of message is not a string - %s", err) | |
| await async_capture_exception(err) | |
| return message |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Closing, as discussed separately, there is no code path where the exception can happen today. FWIW, we should rework |
Proposed change
TypeErrorhandling in format message should not have been removed, required to keep UI from becoming unresponsive as per #2052 . Adjusted to reduce coroutines as per discussion.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit