-
Notifications
You must be signed in to change notification settings - Fork 174
fix(BA-1929): Surface AppProxy client endpoint errors as domain exceptions #11333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2529906
67be543
9e72550
caafcc1
295882c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Send `Accept: application/json` from the manager's AppProxy client so endpoint create/delete failures return parseable JSON instead of HTML error pages. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Surface AppProxy coordinator failures from `AppProxyClient` as `AppProxyConnectionError` / `AppProxyResponseError` instead of silently dropping deletion errors or leaking raw `aiohttp` exceptions, and preserve the upstream error body as `extra_data` for diagnostics. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||
| from collections.abc import AsyncIterator | ||||||||||||||||||||||||||||||||||||||||
| from contextlib import asynccontextmanager | ||||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||||
| from uuid import UUID | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -97,21 +99,93 @@ async def fetch_status(self) -> AppProxyStatusResponse: | |||||||||||||||||||||||||||||||||||||||
| extra_msg=f"Invalid response from AppProxy at {self._address}" | ||||||||||||||||||||||||||||||||||||||||
| ) from e | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @asynccontextmanager | ||||||||||||||||||||||||||||||||||||||||
| async def _request( | ||||||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||||||
| method: str, | ||||||||||||||||||||||||||||||||||||||||
| path: str, | ||||||||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||||||||
| operation: str, | ||||||||||||||||||||||||||||||||||||||||
| json_body: Any = None, | ||||||||||||||||||||||||||||||||||||||||
| ) -> AsyncIterator[aiohttp.ClientResponse]: | ||||||||||||||||||||||||||||||||||||||||
| """Issue an authenticated request and translate transport errors. | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Connection failures become ``AppProxyConnectionError``. Non-2xx | ||||||||||||||||||||||||||||||||||||||||
| responses become ``AppProxyResponseError`` with the upstream body | ||||||||||||||||||||||||||||||||||||||||
| attached as ``extra_data`` so a structured ``BackendAIError`` | ||||||||||||||||||||||||||||||||||||||||
| payload returned by the coordinator survives the translation. | ||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||
| async with self._client_session.request( | ||||||||||||||||||||||||||||||||||||||||
| method, | ||||||||||||||||||||||||||||||||||||||||
| path, | ||||||||||||||||||||||||||||||||||||||||
| headers={ | ||||||||||||||||||||||||||||||||||||||||
| "Accept": "application/json", | ||||||||||||||||||||||||||||||||||||||||
| "X-BackendAI-Token": self._token, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| json=json_body, | ||||||||||||||||||||||||||||||||||||||||
| ) as resp: | ||||||||||||||||||||||||||||||||||||||||
| if resp.status >= 400: | ||||||||||||||||||||||||||||||||||||||||
| text = await resp.text() | ||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+129
to
+130
|
||||||||||||||||||||||||||||||||||||||||
| text = await resp.text() | |
| try: | |
| try: | |
| text = await resp.text(errors="replace") | |
| except Exception as e: | |
| text = f"<failed to read response body: {e}>" | |
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a structure where we use yield when resp.status // 100 == 2 (or up to 300) and handle errors for the rest be more readable? Also, rather than parsing the JSON again after resp.text, wouldn’t it be better to try resp.json first and fall back to resp.text only if that fails? You might find some useful code patterns by looking at the StorageProxyHTTPClient code.
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging the full upstream error_body at error level can leak sensitive details into logs (e.g., user identifiers, request details, internal stack traces, or other coordinator-provided context). Consider logging a truncated/sanitized body, logging only metadata (status/operation/request id), and/or moving the full body to debug-level logging while still attaching it to extra_data for programmatic diagnostics.
| "AppProxy at {} returned {} during {}: {!r}", | |
| self._address, | |
| resp.status, | |
| operation, | |
| error_body, | |
| ) | |
| "AppProxy at {} returned {} during {}", | |
| self._address, | |
| resp.status, | |
| operation, | |
| ) | |
| if text: | |
| preview = text[:512] + ("..." if len(text) > 512 else "") | |
| log.debug( | |
| "AppProxy error body preview at {} during {}: {!r}", | |
| self._address, | |
| operation, | |
| preview, | |
| ) |
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra_data currently omits request metadata such as HTTP method and request path, which can make diagnosing bulk operations harder. Consider including method and path (and possibly operation) in extra_data so downstream logs/telemetry can attribute failures without relying on parsing the error message string.
| extra_data={"status": resp.status, "body": error_body}, | |
| extra_data={ | |
| "status": resp.status, | |
| "body": error_body, | |
| "method": method, | |
| "path": path, | |
| "operation": operation, | |
| }, |
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring says “translate transport errors”, but the implementation only maps aiohttp.ClientConnectorError. Timeouts (e.g., asyncio.TimeoutError) and other aiohttp.ClientError subclasses can still leak out as non-domain exceptions, which undermines the goal of preserving AppProxy domain context. Consider either (a) broadening the exception mapping to include timeouts and relevant aiohttp.ClientError connection/payload exceptions, or (b) narrowing the docstring to match the actual behavior.
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aiohttp.ClientResponse.json() raises ContentTypeError when the response is valid JSON but the Content-Type header is not application/json. If you want to be tolerant of upstream mislabeling (which is common with proxies and some error handlers), consider parsing JSON regardless of content type (while still treating non-JSON bodies as invalid via JSONDecodeError). This reduces false-negative “Invalid response” errors when the payload is actually JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: