feat(error): surface retry-after header#809
Conversation
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the error handling capabilities of the Google Workspace CLI by surfacing 'Retry-After' headers from failed API requests. By parsing these headers into a structured format, the application can now provide clearer guidance on when to retry operations, particularly in rate-limiting scenarios. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
googleworkspace-bot
added
area: http
area: core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the Retry-After header by introducing a retry_after_seconds field to the GwsError::Api error variant and implementing parsing logic in the executor for both numeric and date-based header values. Review feedback points out that using local system time to calculate retry delays from HTTP-dates is prone to clock skew; it is suggested to use the response's Date header as the reference time to comply with RFC 7231 and ensure accurate retry intervals.
| .headers() | ||
| .get(RETRY_AFTER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .and_then(|value| parse_retry_after_seconds(value, SystemTime::now())); |
There was a problem hiding this comment.
Calculating the retry delay using SystemTime::now() is susceptible to clock skew between the client and the server. Per RFC 7231 Section 7.1.1.2, the recipient SHOULD use the value of the Date header field from the response as the reference time for calculating delta-seconds from an HTTP-date. If the client's clock is significantly ahead of the server's, this calculation could result in a 0 delay, leading to immediate retries that continue to fail.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Retry-After HTTP header in API error responses. It adds a retry_after_seconds field to the GwsError::Api error variant and implements logic in the executor to parse this header from both delta-seconds and HTTP-date formats. While the field is correctly integrated into the JSON output and error handling logic, feedback indicates that the Display implementation for GwsError::Api should be updated to include the retry hint to improve the usability of the CLI terminal output.
| /// For `accessNotConfigured` errors: the GCP console URL to enable the API. | ||
| enable_url: Option<String>, | ||
| /// Seconds to wait before retrying, when the API returned a Retry-After header. | ||
| retry_after_seconds: Option<u64>, |
There was a problem hiding this comment.
While the field is correctly added to the JSON output, it is currently missing from the human-readable Display implementation for GwsError::Api (line 22).
For a CLI tool, surfacing the retry hint in the terminal output (e.g., error[api]: Rate limit exceeded (retry after 17s)) is critical for usability. Consider updating the Display implementation to include this information when available.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Retry-After HTTP header in API error responses. The GwsError::Api error variant has been updated to include an optional retry_after_seconds field, which is now populated during request execution by parsing the Retry-After header (supporting both integer seconds and HTTP-date formats). The error's string representation and JSON output have also been updated to include this retry information, and all relevant call sites and tests have been adjusted accordingly. I have no feedback to provide.
2 participants
Summary
Retry-Afterresponse headers from failed Google API responsesretry_after_secondsin structured API errors when Google returns either delta seconds or an HTTP-dateFixes #777
Tests