Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ mod tests {
message: "bad".to_string(),
reason: "r".to_string(),
enable_url: None,
retry_after_seconds: None,
};
let label = error_label(&api_err);
assert!(label.contains("error[api]:"));
Expand Down
87 changes: 86 additions & 1 deletion crates/google-workspace-cli/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@

use std::collections::{HashMap, HashSet};
use std::path::PathBuf;
use std::time::SystemTime;

use anyhow::Context;
use futures_util::stream::TryStreamExt;
use futures_util::StreamExt;
use reqwest::header::RETRY_AFTER;
use serde_json::{json, Map, Value};
use tokio::io::AsyncWriteExt;

Expand Down Expand Up @@ -464,6 +466,11 @@ pub async fn execute_method(
.to_string();

if !status.is_success() {
let retry_after_seconds = response
.headers()
.get(RETRY_AFTER)
.and_then(|value| value.to_str().ok())
.and_then(|value| parse_retry_after_seconds(value, SystemTime::now()));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

let error_body = response.text().await.unwrap_or_default();
tracing::warn!(
api_method = method_id,
Expand All @@ -472,7 +479,7 @@ pub async fn execute_method(
latency_ms = latency_ms,
"API error"
);
return handle_error_response(status, &error_body, &auth_method);
return handle_error_response(status, &error_body, &auth_method, retry_after_seconds);
}

tracing::debug!(
Expand Down Expand Up @@ -750,10 +757,28 @@ pub fn extract_enable_url(message: &str) -> Option<String> {
Some(url.to_string())
}

fn parse_retry_after_seconds(value: &str, now: SystemTime) -> Option<u64> {
let value = value.trim();
if value.is_empty() {
return None;
}

if let Ok(seconds) = value.parse::<u64>() {
return Some(seconds);
}

let retry_at = chrono::DateTime::parse_from_rfc2822(value)
.ok()?
.with_timezone(&chrono::Utc);
let now: chrono::DateTime<chrono::Utc> = now.into();
Some((retry_at - now).num_seconds().max(0) as u64)
}

fn handle_error_response<T>(
status: reqwest::StatusCode,
error_body: &str,
auth_method: &AuthMethod,
retry_after_seconds: Option<u64>,
) -> Result<T, GwsError> {
// If 401/403 and no auth was provided, give a helpful message
if (status.as_u16() == 401 || status.as_u16() == 403) && *auth_method == AuthMethod::None {
Expand Down Expand Up @@ -800,6 +825,7 @@ fn handle_error_response<T>(
message,
reason,
enable_url,
retry_after_seconds,
});
}
}
Expand All @@ -809,6 +835,7 @@ fn handle_error_response<T>(
message: error_body.to_string(),
reason: "httpError".to_string(),
enable_url: None,
retry_after_seconds,
})
}

Expand Down Expand Up @@ -1947,6 +1974,7 @@ mod tests {
reqwest::StatusCode::UNAUTHORIZED,
"Unauthorized",
&AuthMethod::None,
None,
)
.unwrap_err();
match err {
Expand All @@ -1973,6 +2001,7 @@ mod tests {
reqwest::StatusCode::UNAUTHORIZED,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand Down Expand Up @@ -2008,6 +2037,7 @@ mod tests {
reqwest::StatusCode::BAD_REQUEST,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand All @@ -2024,6 +2054,39 @@ mod tests {
_ => panic!("Expected Api error"),
}
}

#[test]
fn test_handle_error_response_includes_retry_after() {
let json_err = json!({
"error": {
"code": 429,
"message": "Rate limit exceeded",
"errors": [{ "reason": "rateLimitExceeded" }]
}
})
.to_string();

let err = handle_error_response::<()>(
reqwest::StatusCode::TOO_MANY_REQUESTS,
&json_err,
&AuthMethod::OAuth,
Some(17),
)
.unwrap_err();
match err {
GwsError::Api {
code,
reason,
retry_after_seconds,
..
} => {
assert_eq!(code, 429);
assert_eq!(reason, "rateLimitExceeded");
assert_eq!(retry_after_seconds, Some(17));
}
_ => panic!("Expected Api error"),
}
}
}

#[tokio::test]
Expand Down Expand Up @@ -2156,6 +2219,7 @@ fn test_handle_error_response_non_json() {
reqwest::StatusCode::INTERNAL_SERVER_ERROR,
"Internal Server Error Text",
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand Down Expand Up @@ -2206,6 +2270,25 @@ fn test_extract_enable_url_trims_trailing_punctuation() {
);
}

#[test]
fn test_parse_retry_after_seconds_delta() {
assert_eq!(
parse_retry_after_seconds("17", std::time::UNIX_EPOCH),
Some(17)
);
}

#[test]
fn test_parse_retry_after_seconds_http_date() {
assert_eq!(
parse_retry_after_seconds(
"Wed, 21 Oct 2015 07:28:00 GMT",
std::time::UNIX_EPOCH + std::time::Duration::from_secs(1_445_412_400),
),
Some(80)
);
}

#[test]
fn test_handle_error_response_access_not_configured_with_url() {
// Matches the top-level "reason" field format Google actually returns for this error
Expand All @@ -2223,6 +2306,7 @@ fn test_handle_error_response_access_not_configured_with_url() {
reqwest::StatusCode::FORBIDDEN,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();

Expand Down Expand Up @@ -2260,6 +2344,7 @@ fn test_handle_error_response_access_not_configured_errors_array() {
reqwest::StatusCode::FORBIDDEN,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();

Expand Down
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/helpers/calendar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ async fn handle_agenda(matches: &ArgMatches) -> Result<(), GwsError> {
message: err,
reason: "calendarList_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
3 changes: 3 additions & 0 deletions crates/google-workspace-cli/src/helpers/events/subscribe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ pub(super) async fn handle_subscribe(
message: format!("Failed to create Pub/Sub topic: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand All @@ -246,6 +247,7 @@ pub(super) async fn handle_subscribe(
message: format!("Failed to create Pub/Sub subscription: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -421,6 +423,7 @@ async fn pull_loop(
message: format!("Pub/Sub pull failed: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
2 changes: 2 additions & 0 deletions crates/google-workspace-cli/src/helpers/gmail/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ pub(super) fn build_api_error(status: u16, body: &str, context: &str) -> GwsErro
message: format!("{context}: {message}"),
reason,
enable_url,
retry_after_seconds: None,
}
}

Expand Down Expand Up @@ -3614,6 +3615,7 @@ mod tests {
message,
reason,
enable_url,
..
} => {
assert_eq!(code, 403);
assert!(message.contains("Test context"));
Expand Down
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/helpers/gmail/triage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> {
message: err,
reason: "list_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
4 changes: 4 additions & 0 deletions crates/google-workspace-cli/src/helpers/gmail/watch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ pub(super) async fn handle_watch(
message: format!("Failed to create Pub/Sub topic: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -132,6 +133,7 @@ pub(super) async fn handle_watch(
message: format!("Failed to create Pub/Sub subscription: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -168,6 +170,7 @@ pub(super) async fn handle_watch(
),
reason: "gmailError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -301,6 +304,7 @@ async fn watch_pull_loop(
message: format!("Pub/Sub pull failed: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
3 changes: 3 additions & 0 deletions crates/google-workspace-cli/src/helpers/workflows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ async fn get_json(
message: body,
reason: "workflow_request_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -517,6 +518,7 @@ async fn handle_email_to_task(matches: &ArgMatches) -> Result<(), GwsError> {
message: body,
reason: "task_create_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down Expand Up @@ -676,6 +678,7 @@ async fn handle_file_announce(matches: &ArgMatches) -> Result<(), GwsError> {
message: body,
reason: "chat_send_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/timezone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ async fn fetch_account_timezone(client: &reqwest::Client, token: &str) -> Result
message: body,
reason: "timezone_fetch_failed".to_string(),
enable_url: None,
retry_after_seconds: None,
});
}

Expand Down
24 changes: 24 additions & 0 deletions crates/google-workspace/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ pub enum GwsError {
reason: String,
/// 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>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

},

#[error("{0}")]
Expand Down Expand Up @@ -71,6 +73,7 @@ impl GwsError {
message,
reason,
enable_url,
retry_after_seconds,
} => {
let mut error_obj = json!({
"code": code,
Expand All @@ -80,6 +83,9 @@ impl GwsError {
if let Some(url) = enable_url {
error_obj["enable_url"] = json!(url);
}
if let Some(seconds) = retry_after_seconds {
error_obj["retry_after_seconds"] = json!(seconds);
}
json!({ "error": error_obj })
}
GwsError::Validation(msg) => json!({
Expand Down Expand Up @@ -125,6 +131,7 @@ mod tests {
message: "Not Found".to_string(),
reason: "notFound".to_string(),
enable_url: None,
retry_after_seconds: None,
};
assert_eq!(err.exit_code(), GwsError::EXIT_CODE_API);
}
Expand Down Expand Up @@ -185,6 +192,7 @@ mod tests {
message: "Not Found".to_string(),
reason: "notFound".to_string(),
enable_url: None,
retry_after_seconds: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 404);
Expand Down Expand Up @@ -236,6 +244,7 @@ mod tests {
message: "Gmail API has not been used in project 549352339482 before or it is disabled.".to_string(),
reason: "accessNotConfigured".to_string(),
enable_url: Some("https://console.developers.google.com/apis/api/gmail.googleapis.com/overview?project=549352339482".to_string()),
retry_after_seconds: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 403);
Expand All @@ -253,10 +262,25 @@ mod tests {
message: "API not enabled.".to_string(),
reason: "accessNotConfigured".to_string(),
enable_url: None,
retry_after_seconds: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 403);
assert_eq!(json["error"]["reason"], "accessNotConfigured");
assert!(json["error"]["enable_url"].is_null());
}

#[test]
fn test_error_to_json_retry_after_seconds() {
let err = GwsError::Api {
code: 429,
message: "Rate limit exceeded.".to_string(),
reason: "rateLimitExceeded".to_string(),
enable_url: None,
retry_after_seconds: Some(17),
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 429);
assert_eq!(json["error"]["retry_after_seconds"], 17);
}
}
Loading