-
Notifications
You must be signed in to change notification settings - Fork 176
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
d78292b
ac0a574
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 @@ | ||
| 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 as actxmgr | ||||||||||||||||||||||||||||||||||||||||
| 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: | ||||||||||||||||||||||||||||||||||||||||
| error_body: Any = json.loads(text) if text else None | ||||||||||||||||||||||||||||||||||||||||
| except json.JSONDecodeError: | ||||||||||||||||||||||||||||||||||||||||
| error_body = text | ||||||||||||||||||||||||||||||||||||||||
| log.error( | ||||||||||||||||||||||||||||||||||||||||
| "AppProxy at {} returned {} during {}: {!r}", | ||||||||||||||||||||||||||||||||||||||||
| self._address, | ||||||||||||||||||||||||||||||||||||||||
| resp.status, | ||||||||||||||||||||||||||||||||||||||||
| operation, | ||||||||||||||||||||||||||||||||||||||||
| error_body, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+118
to
+139
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't a structure where we use |
||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+135
to
+140
|
||||||||||||||||||||||||||||||||||||||||
| "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.
await resp.text()can raise (e.g., decoding errors or payload read errors). If that happens, the code will skip raisingAppProxyResponseErrorand instead leak the low-level exception. To keep error translation reliable, handle failures when reading the response body (e.g., useresp.text(errors="replace")and/or catch read/decode exceptions and fall back to a placeholder body).