From 3fdea00d3c39a896e9816417027e313b5886f290 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Sat, 30 May 2026 05:44:21 +0200 Subject: [PATCH 1/2] fix(channels): handle Discord API 429 rate-limit responses with retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `send_with_retry` helper in the Discord REST client that intercepts HTTP 429 responses, reads `Retry-After` from the response header or JSON body (clamped to 60 s), sleeps, and retries up to 3 times before surfacing the error. All three call sites — `send_message`, `edit_message`, and `trigger_typing` — now use this helper. A 30-second per-request timeout is applied via `RequestBuilder::timeout` to guard every external await including the final exhausted-retry request. Closes #4728 --- crates/zeph-channels/src/discord/rest.rs | 280 ++++++++++++++++++++--- 1 file changed, 253 insertions(+), 27 deletions(-) diff --git a/crates/zeph-channels/src/discord/rest.rs b/crates/zeph-channels/src/discord/rest.rs index c3a2d4424..6fe975dad 100644 --- a/crates/zeph-channels/src/discord/rest.rs +++ b/crates/zeph-channels/src/discord/rest.rs @@ -3,9 +3,15 @@ //! Discord REST API client for message operations. +use std::time::Duration; + use serde::{Deserialize, Serialize}; const BASE_URL: &str = "https://discord.com/api/v10"; +const MAX_RETRY_SECS: f64 = 60.0; +const MAX_RETRIES: u32 = 3; +/// Per-request HTTP timeout applied to every Discord REST call. +const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); #[derive(Deserialize)] struct CurrentApplication { @@ -68,6 +74,72 @@ struct EditMessage<'a> { content: &'a str, } +/// Body returned by Discord on HTTP 429. +#[derive(Deserialize, Default)] +struct RateLimitBody { + retry_after: Option, +} + +/// Executes a request with automatic 429 retry-after backoff. +/// +/// Builds a fresh request each attempt via `make_req`. On HTTP 429 the function +/// reads `Retry-After` header (falling back to the JSON body `retry_after` field), +/// clamps to [`MAX_RETRY_SECS`], sleeps, and retries up to [`MAX_RETRIES`] times. +/// When retries are exhausted a final request is issued to obtain a `reqwest::Error` +/// with the original HTTP status — `reqwest::Error` cannot be constructed directly. +/// +/// # Errors +/// +/// Returns a [`reqwest::Error`] when all retries are exhausted, a non-429 HTTP error +/// is received, or the per-request timeout ([`REQUEST_TIMEOUT`]) is exceeded. +async fn send_with_retry(make_req: F) -> Result +where + F: Fn() -> reqwest::RequestBuilder, +{ + let mut attempts = 0u32; + loop { + let resp = make_req().send().await?; + + if resp.status() != reqwest::StatusCode::TOO_MANY_REQUESTS { + return resp.error_for_status(); + } + + // Parse retry delay: header wins, then body field, then default 1 s. + let header_secs = resp + .headers() + .get(reqwest::header::RETRY_AFTER) + .and_then(|v| v.to_str().ok()) + .and_then(|s| s.parse::().ok()); + + let body_secs = resp + .json::() + .await + .unwrap_or_default() + .retry_after; + + let delay_secs = header_secs.or(body_secs).unwrap_or(1.0).min(MAX_RETRY_SECS); + + attempts += 1; + if attempts > MAX_RETRIES { + tracing::warn!( + delay_secs, + attempts, + "discord: rate-limited and retries exhausted" + ); + // Surface as an error by issuing the request once more without retry. + return make_req().send().await?.error_for_status(); + } + + tracing::warn!( + delay_secs, + attempt = attempts, + max = MAX_RETRIES, + "discord: rate-limited (429), backing off" + ); + tokio::time::sleep(Duration::from_secs_f64(delay_secs)).await; + } +} + impl RestClient { #[must_use] pub fn new(token: String) -> Self { @@ -81,41 +153,44 @@ impl RestClient { /// # Errors /// - /// Returns an error if the HTTP request fails. + /// Returns an error if the HTTP request fails or rate-limit retries are exhausted. pub async fn send_message( &self, channel_id: &str, content: &str, ) -> Result { - self.client - .post(format!("{BASE_URL}/channels/{channel_id}/messages")) - .header("Authorization", self.auth_header()) - .json(&CreateMessage { content }) - .send() - .await? - .error_for_status()? - .json() - .await + let url = format!("{BASE_URL}/channels/{channel_id}/messages"); + let auth = self.auth_header(); + let resp = send_with_retry(|| { + self.client + .post(&url) + .header("Authorization", &auth) + .timeout(REQUEST_TIMEOUT) + .json(&CreateMessage { content }) + }) + .await?; + resp.json().await } /// # Errors /// - /// Returns an error if the HTTP request fails. + /// Returns an error if the HTTP request fails or rate-limit retries are exhausted. pub async fn edit_message( &self, channel_id: &str, message_id: &str, content: &str, ) -> Result<(), reqwest::Error> { - self.client - .patch(format!( - "{BASE_URL}/channels/{channel_id}/messages/{message_id}" - )) - .header("Authorization", self.auth_header()) - .json(&EditMessage { content }) - .send() - .await? - .error_for_status()?; + let url = format!("{BASE_URL}/channels/{channel_id}/messages/{message_id}"); + let auth = self.auth_header(); + send_with_retry(|| { + self.client + .patch(&url) + .header("Authorization", &auth) + .timeout(REQUEST_TIMEOUT) + .json(&EditMessage { content }) + }) + .await?; Ok(()) } @@ -162,14 +237,165 @@ impl RestClient { /// # Errors /// - /// Returns an error if the HTTP request fails. + /// Returns an error if the HTTP request fails or rate-limit retries are exhausted. pub async fn trigger_typing(&self, channel_id: &str) -> Result<(), reqwest::Error> { - self.client - .post(format!("{BASE_URL}/channels/{channel_id}/typing")) - .header("Authorization", self.auth_header()) - .send() - .await? - .error_for_status()?; + let url = format!("{BASE_URL}/channels/{channel_id}/typing"); + let auth = self.auth_header(); + send_with_retry(|| { + self.client + .post(&url) + .header("Authorization", &auth) + .timeout(REQUEST_TIMEOUT) + }) + .await?; Ok(()) } } + +#[cfg(test)] +mod tests { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + use super::*; + + #[tokio::test] + async fn send_with_retry_succeeds_on_200() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": "1"}))) + .mount(&server) + .await; + + let client = reqwest::Client::new(); + let url = format!("{}/channels/ch1/messages", server.uri()); + let resp = send_with_retry(|| client.post(&url)).await.unwrap(); + assert_eq!(resp.status(), 200); + } + + #[tokio::test] + async fn send_with_retry_retries_on_429_then_succeeds() { + let server = MockServer::start().await; + + // First call → 429 with Retry-After header. + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with( + ResponseTemplate::new(429) + .append_header("Retry-After", "0") + .set_body_json(serde_json::json!({"retry_after": 0.0})), + ) + .up_to_n_times(1) + .mount(&server) + .await; + + // Second call → 200. + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": "2"}))) + .mount(&server) + .await; + + let client = reqwest::Client::new(); + let url = format!("{}/channels/ch1/messages", server.uri()); + let resp = send_with_retry(|| client.post(&url)).await.unwrap(); + assert_eq!(resp.status(), 200); + } + + #[tokio::test] + async fn send_with_retry_uses_body_retry_after_when_no_header() { + let server = MockServer::start().await; + + // Three 429 responses without Retry-After header but with body field. + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with( + ResponseTemplate::new(429).set_body_json(serde_json::json!({"retry_after": 0.0})), + ) + .up_to_n_times(3) + .mount(&server) + .await; + + // Fourth → 200. + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": "3"}))) + .mount(&server) + .await; + + let client = reqwest::Client::new(); + let url = format!("{}/channels/ch1/messages", server.uri()); + let resp = send_with_retry(|| client.post(&url)).await.unwrap(); + assert_eq!(resp.status(), 200); + } + + #[tokio::test] + async fn send_with_retry_propagates_non_429_errors() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with(ResponseTemplate::new(403)) + .mount(&server) + .await; + + let client = reqwest::Client::new(); + let url = format!("{}/channels/ch1/messages", server.uri()); + let result = send_with_retry(|| client.post(&url)).await; + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.status(), Some(reqwest::StatusCode::FORBIDDEN)); + } + + #[tokio::test] + async fn send_with_retry_errors_when_retries_exhausted() { + let server = MockServer::start().await; + + // Return 429 for all requests — exhausts MAX_RETRIES (3) then the final attempt. + Mock::given(method("POST")) + .and(path("/channels/ch1/messages")) + .respond_with( + ResponseTemplate::new(429) + .append_header("Retry-After", "0") + .set_body_json(serde_json::json!({"retry_after": 0.0})), + ) + .mount(&server) + .await; + + let client = reqwest::Client::new(); + let url = format!("{}/channels/ch1/messages", server.uri()); + let result = send_with_retry(|| client.post(&url)).await; + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.status(), Some(reqwest::StatusCode::TOO_MANY_REQUESTS)); + } + + #[test] + fn rate_limit_body_defaults_to_none() { + let body: RateLimitBody = serde_json::from_str("{}").unwrap(); + assert!(body.retry_after.is_none()); + } + + #[test] + fn rate_limit_body_parses_float() { + let body: RateLimitBody = serde_json::from_str(r#"{"retry_after": 1.5}"#).unwrap(); + assert!((body.retry_after.unwrap() - 1.5).abs() < f64::EPSILON); + } + + #[test] + fn max_retry_secs_clamps() { + let unclamped: f64 = 120.0; + assert_eq!(unclamped.min(MAX_RETRY_SECS), MAX_RETRY_SECS); + } + + #[test] + fn rest_client_debug_redacts_token() { + let rc = RestClient { + client: reqwest::Client::new(), + token: "secret-token".into(), + }; + let debug = format!("{rc:?}"); + assert!(!debug.contains("secret-token")); + assert!(debug.contains("REDACTED")); + } +} From e18f4c1f1b4d9a6a2faad716ef5088f07da711fe Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Sat, 30 May 2026 16:07:23 +0200 Subject: [PATCH 2/2] fix(channels): replace float_cmp in max_retry_secs_clamps test Use to_bits() comparison instead of assert_eq! on f64 values to satisfy clippy::float_cmp under the full feature set. --- crates/zeph-channels/src/discord/rest.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/zeph-channels/src/discord/rest.rs b/crates/zeph-channels/src/discord/rest.rs index 6fe975dad..4b6ea9e8e 100644 --- a/crates/zeph-channels/src/discord/rest.rs +++ b/crates/zeph-channels/src/discord/rest.rs @@ -385,7 +385,10 @@ mod tests { #[test] fn max_retry_secs_clamps() { let unclamped: f64 = 120.0; - assert_eq!(unclamped.min(MAX_RETRY_SECS), MAX_RETRY_SECS); + assert_eq!( + unclamped.min(MAX_RETRY_SECS).to_bits(), + MAX_RETRY_SECS.to_bits() + ); } #[test]