fix(cli): retry transient relay failures and raise timeouts (#2196)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@sprout-oss.stage.blox.sqprod.co>

Cargo.lock

@@ -883,6 +883,7 @@ dependencies = [
 name = "buzz-cli"
 version = "0.1.0"
 dependencies = [
+ "axum",
  "base64",
  "buzz-core",
  "buzz-persona",
@@ -896,6 +897,7 @@ dependencies = [
  "hex",
  "infer",
  "nostr",
+ "rand 0.10.1",
  "reqwest 0.13.4",
  "serde",
  "serde_json",

crates/buzz-cli/Cargo.toml

@@ -76,6 +76,11 @@ dirs = "6"
 # WebSocket client — ephemeral event publish (kind:20001 is WS-only on the relay)
 buzz-ws-client = { path = "../buzz-ws-client" }
 
+# Random number generation — full jitter for exponential backoff in with_retry
+rand = { workspace = true }
+
 [dev-dependencies]
 # Scratch files for channel-templates.json fixtures in tests
 tempfile = "3"
+# Minimal HTTP test server for retry/policy integration tests
+axum = { workspace = true }

crates/buzz-cli/src/error.rs

@@ -11,7 +11,7 @@ pub enum CliError {
     Relay { status: u16, body: String },
 
     /// Network-level failure (connect, timeout, DNS)
-    #[error("network error: {0}")]
+    #[error("network error: {}", fmt_reqwest_error(.0))]
     Network(#[from] reqwest::Error),
 
     /// Auth missing or rejected (401/403)
@@ -32,11 +32,58 @@ pub enum CliError {
     #[error("{0}")]
     NotFound(String),
 
+    /// A non-idempotent command's outcome is unknown: the request may have
+    /// reached the relay, but the response was lost. Never auto-retried and
+    /// never labeled retryable — the relay executes these commands before any
+    /// dedup, so a blind re-run can duplicate the mutation.
+    #[error("delivery unknown: {0}")]
+    DeliveryUnknown(String),
+
     /// Catch-all for unexpected failures
     #[error("{0}")]
     Other(String),
 }
 
+/// Walk the full `std::error::Error::source()` chain on a `reqwest::Error`
+/// and render it as a colon-separated string, e.g.
+/// `error sending request: dns error: failed to lookup address information: ...`
+fn fmt_reqwest_error(e: &reqwest::Error) -> String {
+    let mut msg = e.to_string();
+    let mut source: &dyn std::error::Error = e;
+    while let Some(cause) = source.source() {
+        let cause_str = cause.to_string();
+        if !msg.contains(&cause_str) {
+            msg.push_str(": ");
+            msg.push_str(&cause_str);
+        }
+        source = cause;
+    }
+    msg
+}
+
+/// Returns `true` when the error is transient and a retry may succeed.
+///
+/// Transport-level network errors (connect failure, timeout, mid-request,
+/// mid-body transfer, or body decode failure) and relay overload responses
+/// (429 / 502 / 503 / 504) are retryable.  `DeliveryUnknown` is never
+/// retryable: the operation may already have executed.  All other errors
+/// indicate a permanent failure: auth, bad input, builder errors, or logic
+/// errors.
+pub fn is_retryable_error(e: &CliError) -> bool {
+    match e {
+        CliError::Network(ref net_err) => {
+            net_err.is_connect()
+                || net_err.is_timeout()
+                || net_err.is_request()
+                || net_err.is_body()
+                || net_err.is_decode()
+        }
+        CliError::Relay { status, .. } => matches!(status, 429 | 502 | 503 | 504),
+        CliError::DeliveryUnknown(_) => false,
+        _ => false,
+    }
+}
+
 /// Map CliError to process exit code.
 /// 0=success (not an error), 1=user/not-found, 2=network/relay, 3=auth,
 /// 4=other, 5=write conflict (NIP-33 dominated head).
@@ -55,12 +102,13 @@ pub fn exit_code(e: &CliError) -> i32 {
         CliError::Key(_) => 3,
         CliError::Conflict(_) => 5,
         CliError::NotFound(_) => 1,
+        CliError::DeliveryUnknown(_) => 2,
         CliError::Other(_) => 4,
     }
 }
 
 /// Serialize error to JSON and write to stderr.
-/// Format: {"error": "<category>", "message": "<human-readable detail>"}
+/// Format: {"error": "<category>", "message": "<human-readable detail>", "retryable": <bool>}
 pub fn print_error(e: &CliError) {
     let category = match e {
         CliError::Usage(_) => "user_error",
@@ -76,11 +124,113 @@ pub fn print_error(e: &CliError) {
         CliError::Key(_) => "key_error",
         CliError::Conflict(_) => "conflict",
         CliError::NotFound(_) => "not_found",
+        CliError::DeliveryUnknown(_) => "delivery_unknown",
         CliError::Other(_) => "error",
     };
     let obj = serde_json::json!({
         "error": category,
         "message": e.to_string(),
+        "retryable": is_retryable_error(e),
     });
     eprintln!("{}", obj);
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // ---- is_retryable_error ----
+
+    #[test]
+    fn network_builder_errors_are_not_retryable() {
+        // A bad URL produces a builder-level reqwest::Error (is_builder() == true).
+        // Builder errors are not transport failures — not retryable.
+        // Transport errors (is_connect/timeout/request) require live I/O to construct;
+        // the predicate here mirrors with_retry's condition exactly.
+        let e = reqwest::Client::new().get("not-a-url").build().unwrap_err();
+        assert!(e.is_builder(), "expected a builder error from bad URL");
+        assert!(!is_retryable_error(&CliError::Network(e)));
+    }
+
+    #[test]
+    fn relay_429_502_503_504_are_retryable() {
+        for status in [429u16, 502, 503, 504] {
+            assert!(
+                is_retryable_error(&CliError::Relay {
+                    status,
+                    body: String::new()
+                }),
+                "status {status} should be retryable"
+            );
+        }
+    }
+
+    #[test]
+    fn relay_400_401_403_404_422_are_not_retryable() {
+        for status in [400u16, 401, 403, 404, 422] {
+            assert!(
+                !is_retryable_error(&CliError::Relay {
+                    status,
+                    body: String::new()
+                }),
+                "status {status} should not be retryable"
+            );
+        }
+    }
+
+    #[test]
+    fn other_errors_are_not_retryable() {
+        assert!(!is_retryable_error(&CliError::Usage("bad flag".into())));
+        assert!(!is_retryable_error(&CliError::Auth("missing key".into())));
+        assert!(!is_retryable_error(&CliError::Key("bad key".into())));
+        assert!(!is_retryable_error(&CliError::Conflict(
+            "superseded".into()
+        )));
+        assert!(!is_retryable_error(&CliError::NotFound("gone".into())));
+        assert!(!is_retryable_error(&CliError::Other("unexpected".into())));
+    }
+
+    // ---- print_error "retryable" field ----
+
+    #[test]
+    fn json_error_includes_retryable_field_for_network() {
+        // Builder errors (bad URL) are not transport-level — retryable: false.
+        // This test verifies the JSON shape and that the field is present.
+        let e = reqwest::Client::new().get("not-a-url").build().unwrap_err();
+        let err = CliError::Network(e);
+        let v = serde_json::json!({
+            "error": "network_error",
+            "message": err.to_string(),
+            "retryable": is_retryable_error(&err),
+        });
+        assert_eq!(v["retryable"].as_bool(), Some(false));
+        assert_eq!(v["error"].as_str(), Some("network_error"));
+    }
+
+    #[test]
+    fn json_error_retryable_false_for_usage() {
+        let err = CliError::Usage("bad flag".into());
+        let v = serde_json::json!({
+            "error": "user_error",
+            "message": err.to_string(),
+            "retryable": is_retryable_error(&err),
+        });
+        assert_eq!(v["retryable"].as_bool(), Some(false));
+    }
+
+    // ---- Display source-chain ----
+
+    #[test]
+    fn network_display_includes_detail_beyond_prefix() {
+        let e = reqwest::Client::new().get("not-a-url").build().unwrap_err();
+        let display = CliError::Network(e).to_string();
+        assert!(
+            display.starts_with("network error:"),
+            "display should start with 'network error:': {display}"
+        );
+        assert!(
+            display.len() > "network error: ".len(),
+            "display should contain error detail: {display}"
+        );
+    }
+}

crates/buzz-ws-client/src/connection.rs

@@ -13,6 +13,15 @@ use crate::message::{build_auth_event, parse_relay_message, OkResponse, RelayMes
 
 type WsStream = WebSocketStream<MaybeTlsStream<tokio::net::TcpStream>>;
 
+/// Seconds to wait for the relay to send the NIP-42 AUTH challenge after connecting.
+pub const AUTH_CHALLENGE_TIMEOUT_SECS: u64 = 20;
+
+/// Seconds to wait for the relay's OK response to the AUTH event.
+pub const AUTH_OK_TIMEOUT_SECS: u64 = 20;
+
+/// Seconds to wait for the relay's OK response to a published event.
+pub const PUBLISH_OK_TIMEOUT_SECS: u64 = 30;
+
 /// A NIP-42-capable WebSocket connection to a Nostr relay.
 pub struct NostrWsConnection {
     ws: WsStream,
@@ -63,14 +72,18 @@ impl NostrWsConnection {
         keys: &Keys,
         auth_tag: Option<&Tag>,
     ) -> Result<(), WsClientError> {
-        let challenge = self.wait_for_auth_challenge(Duration::from_secs(5)).await?;
+        let challenge = self
+            .wait_for_auth_challenge(Duration::from_secs(AUTH_CHALLENGE_TIMEOUT_SECS))
+            .await?;
 
         let auth_event = build_auth_event(&challenge, &self.relay_url, keys, auth_tag)?;
         let event_id = auth_event.id.to_hex();
 
         self.send_raw(&json!(["AUTH", auth_event])).await?;
 
-        let ok = self.wait_for_ok(&event_id, Duration::from_secs(5)).await?;
+        let ok = self
+            .wait_for_ok(&event_id, Duration::from_secs(AUTH_OK_TIMEOUT_SECS))
+            .await?;
         if !ok.accepted {
             return Err(WsClientError::AuthFailed(ok.message));
         }
@@ -83,7 +96,8 @@ impl NostrWsConnection {
     pub async fn send_event(&mut self, event: Event) -> Result<OkResponse, WsClientError> {
         let event_id = event.id.to_hex();
         self.send_raw(&json!(["EVENT", event])).await?;
-        self.wait_for_ok(&event_id, Duration::from_secs(10)).await
+        self.wait_for_ok(&event_id, Duration::from_secs(PUBLISH_OK_TIMEOUT_SECS))
+            .await
     }
 
     /// Receives the next relay message, waiting up to `timeout_dur`.
@@ -278,3 +292,23 @@ pub async fn publish_event(
     .map_err(|_| WsClientError::Timeout)?;
     result
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn auth_challenge_timeout_meets_floor() {
+        const { assert!(AUTH_CHALLENGE_TIMEOUT_SECS >= 20) };
+    }
+
+    #[test]
+    fn auth_ok_timeout_meets_floor() {
+        const { assert!(AUTH_OK_TIMEOUT_SECS >= 20) };
+    }
+
+    #[test]
+    fn publish_ok_timeout_meets_floor() {
+        const { assert!(PUBLISH_OK_TIMEOUT_SECS >= 30) };
+    }
+}