diff --git a/crates/openfang-channels/src/telegram.rs b/crates/openfang-channels/src/telegram.rs index 515233df..cb4a5b01 100644 --- a/crates/openfang-channels/src/telegram.rs +++ b/crates/openfang-channels/src/telegram.rs @@ -57,12 +57,12 @@ pub struct TelegramAdapter { /// can differ per chat and is settable by admins). /// /// Cached errors: `REACTION_INVALID` (emoji not in the free-reaction - /// allowlist, or not a valid reaction at all), `REACTION_NOT_AVAILABLE` - /// (chat admin restricted this emoji), and `REACTION_TOO_MANY` (bot hit - /// per-message reaction cap for this chat). Transient errors (429, 5xx, - /// unrelated 400s) are NOT cached. Grows monotonically over process - /// lifetime; cache resets on restart, which is fine because admins can - /// change allowed reactions at any time. + /// allowlist, or not a valid reaction at all) and `REACTION_NOT_AVAILABLE` + /// (chat admin restricted this emoji). Transient errors (429, 5xx, + /// `REACTION_TOO_MANY` per-message rate-limit, unrelated 400s) are NOT + /// cached. Grows monotonically over process lifetime; cache resets on + /// restart, which is fine because admins can change allowed reactions at + /// any time. rejected_reactions: Arc>>, shutdown_tx: Arc>, shutdown_rx: watch::Receiver, @@ -408,9 +408,10 @@ impl TelegramAdapter { /// Telegram restricts non-premium bots to a free-reaction allowlist, and /// chat admins can further restrict allowed reactions per chat via /// `Chat.available_reactions`. Terminal errors - /// (`REACTION_INVALID`, `REACTION_NOT_AVAILABLE`, `REACTION_TOO_MANY`) - /// are cached per `(chat_id, emoji)` so we don't keep calling the API - /// with reactions that will never succeed in that chat. Because two + /// (`REACTION_INVALID`, `REACTION_NOT_AVAILABLE`) are cached per + /// `(chat_id, emoji)` so we don't keep calling the API with reactions + /// that will never succeed in that chat. `REACTION_TOO_MANY` is a + /// transient per-message rate-limit and is NOT cached. Because two /// concurrent `fire_reaction` calls for the same `(chat_id, emoji)` can /// both pass the cache check before either rejection lands, the first /// rejection may produce up to N duplicate API calls where N is the @@ -466,12 +467,10 @@ impl TelegramAdapter { /// `(chat, emoji)` pair will not succeed without an outside change (chat /// admin updating `Chat.available_reactions`, bot getting Premium, etc.). /// Callers cache these and stop retrying. Transient errors (429, 5xx, -/// `RETRY_AFTER`, unrelated 400s like `MESSAGE_NOT_MODIFIED`) are NOT -/// included here. +/// `RETRY_AFTER`, `REACTION_TOO_MANY` per-message rate-limit, unrelated +/// 400s like `MESSAGE_NOT_MODIFIED`) are NOT included here. fn is_terminal_reaction_error(body_text: &str) -> bool { - body_text.contains("REACTION_INVALID") - || body_text.contains("REACTION_NOT_AVAILABLE") - || body_text.contains("REACTION_TOO_MANY") + body_text.contains("REACTION_INVALID") || body_text.contains("REACTION_NOT_AVAILABLE") } impl TelegramAdapter { @@ -2186,13 +2185,16 @@ mod tests { assert!(is_terminal_reaction_error( r#"{"description":"Bad Request: REACTION_NOT_AVAILABLE"}"# )); - assert!(is_terminal_reaction_error( - r#"{"description":"Bad Request: REACTION_TOO_MANY"}"# - )); } #[test] fn test_is_terminal_reaction_error_rejects_transient() { + // REACTION_TOO_MANY is a per-message rate-limit, not permanent. + // Caching it would suppress valid future reactions on that emoji + // for the lifetime of the process — see issue #1133. + assert!(!is_terminal_reaction_error( + r#"{"description":"Bad Request: REACTION_TOO_MANY"}"# + )); assert!(!is_terminal_reaction_error( r#"{"description":"Too Many Requests: retry after 5"}"# )); @@ -2203,6 +2205,34 @@ mod tests { assert!(!is_terminal_reaction_error("")); } + #[tokio::test] + async fn test_fire_reaction_does_not_cache_reaction_too_many() { + // Regression test for #1133: REACTION_TOO_MANY is a transient + // per-message rate-limit and must NOT be cached as a permanent + // rejection. Caching it would suppress valid future reactions on + // that (chat_id, emoji) pair for the lifetime of the process. + let stub = StubServer::new(vec![( + 400, + r#"{"ok":false,"error_code":400,"description":"Bad Request: REACTION_TOO_MANY"}"#, + )]); + let base = spawn_stub_server(stub.clone()).await; + let adapter = test_adapter(base); + + adapter.fire_reaction(999, 1, "⏳"); + tokio::time::sleep(Duration::from_millis(100)).await; + assert_eq!(stub.hit_count(), 1); + + let cached = adapter + .rejected_reactions + .lock() + .map(|s| s.contains(&(999_i64, "⏳".to_string()))) + .unwrap_or(true); + assert!( + !cached, + "REACTION_TOO_MANY is transient and must NOT populate the cache" + ); + } + #[tokio::test] async fn test_fire_reaction_caches_on_reaction_invalid() { let stub = StubServer::new(vec![(