Eng 2502 add ignore errors path to async polling#7401
Eng 2502 add ignore errors path to async polling#7401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile SummaryThis PR fixes the async polling lifecycle to properly respect the
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 5df181e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @staticmethod | ||
| def _should_ignore_error( | ||
| status_code: int, | ||
| ignore_errors: Optional[Union[bool, List[int]]], | ||
| ) -> bool: | ||
| """ | ||
| Determine if an error response should be ignored based on the ignore_errors config. | ||
|
|
||
| Mirrors AuthenticatedClient._should_ignore_error: | ||
| - False: do not ignore any errors | ||
| - True: ignore all errors | ||
| - List[int]: ignore only if status_code is in the list | ||
| """ | ||
| if ignore_errors is True: | ||
| return True | ||
| if isinstance(ignore_errors, list): | ||
| return status_code in ignore_errors | ||
| return False |
There was a problem hiding this comment.
Duplicated _should_ignore_error logic
This static method duplicates the logic from AuthenticatedClient._should_ignore_error (line 175 in authenticated_client.py). While the duplication is acknowledged in the docstring, having two copies of this branching logic creates a maintenance risk — if the original is updated (e.g., to support new ignore patterns), this copy could drift silently.
Consider extracting this into a shared utility function that both AuthenticatedClient and PollingRequestHandler can import, to keep the logic in one place.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if not response.ok and PollingRequestHandler._should_ignore_error( | ||
| response.status_code, read_request.ignore_errors | ||
| ): | ||
| logger.info( | ||
| "Ignoring errored response with status code {} for initial async polling request as configured.", | ||
| response.status_code, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Redundant _should_ignore_error check after client.send
client.send(next_request, read_request.ignore_errors) (line 317) already uses ignore_errors internally — when the status code matches, AuthenticatedClient.send returns the response without raising RequestFailureResponseException. The _should_ignore_error check on line 319 then re-evaluates the same condition.
While this double-check is functionally safe (and necessary given that send returns the non-ok response instead of filtering it out), it would be clearer to extract this into a shared helper like _send_and_handle_errors in PollingRequestHandler — which you've already built. Consider reusing PollingRequestHandler._send_and_handle_errors here as well to keep the error handling pattern consistent and avoid repeating the ignore + raise logic in three separate places (here, the erasure path at line 381, and _send_and_handle_errors).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Ticket ENG 2502
Description Of Changes
Extended the ignore errors config key handle to properly work on the async polling lifecycle
Code Changes
polling_request_handler.py_should_ignore_error(status_code, ignore_errors)static method that mirrorsAuthenticatedClient._should_ignore_error(True = all, list = specific codes, False = none)._send_and_handle_errorsstatic method that sends, checks ignore, and raises -- reused by bothget_status_responseandget_result_response.get_status_responseandget_result_responsenow passignore_errorstoclient.sendand use the shared error handling.async_dsr_strategy_polling.py_handle_polling_initial_request: on ignored error, logs andcontinues (skips sub-request creation) using status-code-aware check.. same change for the erasure path._initial_request_access: now only raisesAwaitingAsyncProcessingwhen sub-requests were created. Returns[]when all were ignored (matching the existing erasure path behavior).Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works