From e4f73a8223d03e025ba14feef54e8173b7fce6e0 Mon Sep 17 00:00:00 2001 From: "blu@blu.cx" <38222767+thebrandonlucas@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:14:52 -0500 Subject: [PATCH] fix(cli): say why a request got no response, and what to check Closes #84. --- CHANGELOG.md | 1 + docs/guides/troubleshooting.md | 2 +- src/api.rs | 37 +++- src/auth.rs | 41 +++- src/error.rs | 337 ++++++++++++++++++++++++++++++++- src/price.rs | 12 +- tests/cli_integration.rs | 125 ++++++++++++ 7 files changed, 540 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a6cae8..aa533d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ User-visible changes to `voltage`, newest first. Each pull request that changes ## Unreleased +- A request that gets no response says why: a timeout (for the auth service, its fixed 30-second limit, which `--timeout` does not change), a connection not made within the 10-second connect limit (which `--timeout` does not change), a failed TLS connection, a connection that could not be made, or one that closed early. A hint names the next step. A write that timed out or closed early may have been applied, so its hint says to look up the resource ID before running the command again, never to retry. This covers API, auth, price, and checkout stream requests, which before said only that the request failed. - `--output-file` on a path that already exists says so and suggests a new path, instead of showing an OS error; any other refusal keeps its cause and gets the same hint. Both say that no change was sent. - `-o` is short for `--output`, as in `voltage wallets list -o json`. diff --git a/docs/guides/troubleshooting.md b/docs/guides/troubleshooting.md index 038342b..cc6a5de 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -18,7 +18,7 @@ Authentication or authorization failed. See [HTTP 401](#http-401) and [HTTP 403] ### Exit code 4 -The connection failed. For a payment or treasury movement, the request may have been sent: the error reports the ID, and the CLI never resubmits it. Read it with `voltage payments get PAYMENT_ID` before you decide to retry. See [payment IDs](../concepts/safety-model.md#payment-ids-and-the-request-journal). +The request got no response. The message says why: no response within `--timeout` (for the auth service, within its fixed 30 seconds, which `--timeout` does not change), no connection within 10 seconds (an unreachable host or a firewall; `--timeout` does not change this limit), a failed TLS connection (often a certificate this system does not trust, or a proxy that inspects TLS), a connection that could not be made (DNS, a refused port, or a proxy), or a connection that closed early. The hint names the next step. A read can simply be retried, but a change that timed out or whose connection closed early may have been applied, so the hint says to look up its resource ID first: a rerun can repeat the change. For a payment or treasury movement, the request may have been sent: the error reports the ID, and the CLI never resubmits it. Read it with `voltage payments get PAYMENT_ID` before you decide to retry. See [payment IDs](../concepts/safety-model.md#payment-ids-and-the-request-journal). ### Exit code 5 diff --git a/src/api.rs b/src/api.rs index 55df986..5a86d53 100644 --- a/src/api.rs +++ b/src/api.rs @@ -15,7 +15,7 @@ use crate::{ read_secret, shell_word, }, dry_run, - error::{CredentialSources, ErrorDetail, ErrorKind}, + error::{CredentialSources, ErrorDetail, ErrorKind, ResponseLimit}, input::{PaginationMode, Request}, output::{Envelope, Outcome, Output}, payment::{ @@ -245,12 +245,26 @@ impl Api { .request(method.into(), path, query, body) .send() .await - .map_err(|_| { - Error::transport(if read_only { - "HTTP read request failed. Check the API URL and whether the API service is running." + .map_err(|error| { + if read_only { + Error::no_response( + &error, + "HTTP read request", + "", + "API", + ResponseLimit::Timeout, + false, + ) } else { - "HTTP request failed; a write may have been submitted. No mutation was retried." - }) + Error::no_response( + &error, + "HTTP request", + "; a write may have been submitted. No mutation was retried.", + "API", + ResponseLimit::Timeout, + true, + ) + } })?; let status = response.status().as_u16(); let retry_after = retry_after(response.headers()); @@ -336,7 +350,16 @@ impl Api { .send(), ) .await - .map_err(|_| Error::transport("Could not open checkout event stream"))?; + .map_err(|error| { + Error::no_response( + &error, + "Opening the checkout event stream", + "", + "API", + ResponseLimit::Timeout, + false, + ) + })?; let status = response.status().as_u16(); if !response.status().is_success() { return Err(Error::new( diff --git a/src/auth.rs b/src/auth.rs index a09eb5e..12875dd 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -13,7 +13,7 @@ use crate::{ InputSource, LOCK_TIMEOUT, Login, Scope, ScopeSources, Settings, Source, name_in_use, read_secret, shell_word, unreadable_name, }, - error::{CredentialRejection, CredentialSources, ErrorDetail}, + error::{CredentialRejection, CredentialSources, ErrorDetail, ResponseLimit}, secret::Secret, terminal::{self, Terminal}, }; @@ -37,7 +37,8 @@ const MAX_LOGIN_WAIT: u64 = 600; const MIN_POLL_INTERVAL: u64 = 5; const MAX_POLL_INTERVAL: u64 = 600; const SLOW_DOWN_STEP: u64 = 5; -const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); +/// `TransportFailure::ConnectTimeout` names this limit in its message. +pub(crate) const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); /// Auth responses are small JSON documents; anything larger is not one. const MAX_AUTH_RESPONSE_BYTES: usize = 1024 * 1024; @@ -188,9 +189,14 @@ async fn auth_request( terminal: Terminal, ) -> Result { let _progress = terminal.progress("Waiting for auth service..."); - let response = request.send().await.map_err(|_| { - Error::transport( - "Authentication request failed; no token request was automatically retried", + let response = request.send().await.map_err(|error| { + Error::no_response( + &error, + "Authentication request", + "; no token request was automatically retried", + "auth", + ResponseLimit::Fixed(AUTH_TIMEOUT), + false, ) })?; let status = response.status().as_u16(); @@ -1297,6 +1303,31 @@ mod tests { assert_eq!(&body[..], b"token"); } + #[tokio::test] + async fn an_auth_timeout_names_its_fixed_limit_not_timeout_flag() { + // The listener takes the connection but never answers. The client's short limit + // stands in for AUTH_TIMEOUT, which the message must still name. + let silent = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let request = client(Duration::from_millis(100)) + .unwrap() + .get(format!("http://{}/", silent.local_addr().unwrap())); + let Err(error) = auth_request(request, Terminal::new(true, true)).await else { + panic!("a silent auth service must fail"); + }; + assert_eq!(error.kind, crate::error::ErrorKind::Transport); + assert_eq!( + error.message, + "Authentication request failed: no response within 30 seconds; no token request was automatically retried" + ); + assert_eq!( + error.hint.as_deref(), + Some( + "Retry. The auth service has a fixed 30-second limit, which --timeout does not change." + ) + ); + drop(silent); + } + #[tokio::test] async fn oversized_auth_responses_are_rejected() { let server = wiremock::MockServer::start().await; diff --git a/src/error.rs b/src/error.rs index aca6d4b..e0e14a6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -6,7 +6,7 @@ use crate::{ }; use serde::Serialize; use serde_json::Value; -use std::borrow::Cow; +use std::{borrow::Cow, error::Error as StdError, io, time::Duration}; use uuid::Uuid; pub type Result = std::result::Result; @@ -164,7 +164,138 @@ pub struct Error { pub(crate) stdout_closed: bool, } +/// How long a request had to get a response. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ResponseLimit { + /// `--timeout`, which a larger value raises. + Timeout, + /// A fixed limit that `--timeout` does not change, such as the auth service's. + Fixed(Duration), +} + +/// Why a request got no HTTP response. Each class has a different fix, so the message +/// names it; the underlying error's text can echo hosts and paths and is never shown. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum TransportFailure { + /// No response within the request's limit. + Timeout(ResponseLimit), + /// The connection was not established within the fixed connect limit, which + /// `--timeout` does not change: an unreachable host or a firewall that drops packets. + ConnectTimeout, + /// The TLS handshake failed, for example because the certificate was not trusted. + Tls, + /// The connection was not established: DNS, a refused port, or a proxy. + Connect, + /// The connection broke before a response arrived. + Other, +} + +impl TransportFailure { + fn of(error: &reqwest::Error, limit: ResponseLimit) -> Self { + let mut tls = false; + let mut cause = error.source(); + while let Some(current) = cause { + tls |= is_tls(current); + cause = current.source(); + } + Self::classify(error.is_timeout(), error.is_connect(), tls, limit) + } + + /// A connect timeout is both a timeout and a connect error. It is checked first, because + /// it ends at the connect limit, not at `--timeout`. + fn classify(timeout: bool, connect: bool, tls: bool, limit: ResponseLimit) -> Self { + match (timeout, connect, tls) { + (true, true, _) => Self::ConnectTimeout, + (true, false, _) => Self::Timeout(limit), + (false, _, true) => Self::Tls, + (false, true, false) => Self::Connect, + (false, false, false) => Self::Other, + } + } + + fn what(self) -> Cow<'static, str> { + match self { + Self::Timeout(ResponseLimit::Timeout) => "no response within --timeout".into(), + Self::Timeout(ResponseLimit::Fixed(limit)) => { + format!("no response within {} seconds", limit.as_secs()).into() + } + Self::ConnectTimeout => "could not connect within 10 seconds".into(), + Self::Tls => { + "the TLS connection failed, for example because the server's certificate was not trusted".into() + } + Self::Connect => "could not connect".into(), + Self::Other => "the connection closed before a response arrived".into(), + } + } + + /// The next step. A `write` that may have reached the server must not be repeated blindly: + /// a rerun can apply the change again, so the resource is looked up first. A failed + /// connection or TLS handshake sent nothing, so its fix is the same for reads and writes. + fn next_step(self, service: &str, write: bool) -> String { + const LOOK_UP_FIRST: &str = "The change may have been applied. Look up the resource ID before you run the command again, because a rerun can repeat the change."; + match self { + Self::Timeout(ResponseLimit::Timeout) if write => { + format!( + "{LOOK_UP_FIRST} A larger --timeout gives the {service} service longer to answer." + ) + } + Self::Timeout(ResponseLimit::Fixed(limit)) if write => format!( + "{LOOK_UP_FIRST} The {service} service has a fixed {}-second limit, which --timeout does not change.", + limit.as_secs() + ), + Self::Other if write => format!( + "{LOOK_UP_FIRST} Then check the {service} URL and whether the {service} service is running." + ), + Self::Timeout(ResponseLimit::Timeout) => "Retry, or pass a larger --timeout.".into(), + Self::Timeout(ResponseLimit::Fixed(limit)) => format!( + "Retry. The {service} service has a fixed {}-second limit, which --timeout does not change.", + limit.as_secs() + ), + Self::ConnectTimeout => format!( + "Check the {service} URL's host, your network or firewall, and whether the {service} service is reachable. --timeout does not change this connect limit." + ), + Self::Tls => format!( + "Check the {service} URL's host, and that this system trusts its certificate, including any proxy that inspects TLS." + ), + Self::Connect => format!( + "Check the {service} URL, your network or proxy, and whether the {service} service is running." + ), + Self::Other => { + format!("Check the {service} URL and whether the {service} service is running.") + } + } + } +} + +/// Whether `error` is a rustls error. rustls errors reach reqwest inside nested `io::Error`s, +/// whose `source()` skips the error they wrap, so each `io::Error` is unwrapped explicitly. +fn is_tls(error: &(dyn StdError + 'static)) -> bool { + if error.is::() { + return true; + } + error + .downcast_ref::() + .and_then(io::Error::get_ref) + .is_some_and(|inner| is_tls(inner)) +} + impl Error { + /// A request to `service` that got no response within `limit`. `request` names it, the + /// message says why it failed, and `consequence` adds what the caller must know, such as + /// a possible write; the hint names the next step, which for a `write` never says to retry. + pub fn no_response( + error: &reqwest::Error, + request: &str, + consequence: &str, + service: &str, + limit: ResponseLimit, + write: bool, + ) -> Self { + let failure = TransportFailure::of(error, limit); + Self::transport(format!("{request} failed: {}{consequence}", failure.what())) + .with_hint(failure.next_step(service, write)) + } + pub fn new(kind: ErrorKind, message: impl Into) -> Self { Self { kind, @@ -269,7 +400,211 @@ impl From for Error { #[cfg(test)] mod tests { use super::*; + use crate::auth::client; use serde_json::json; + use std::{ + io::{Read, Write}, + net::{TcpListener, TcpStream}, + thread, + time::Duration, + }; + + /// Accept one connection on a loopback port and hand it to `serve` on a thread. + fn serve_once(serve: impl FnOnce(TcpStream) + Send + 'static) -> u16 { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + serve(stream); + } + }); + port + } + + fn read_request(stream: &mut TcpStream) { + let mut buffer = [0; 1024]; + let _ = stream.read(&mut buffer); + } + + async fn failure(url: String, timeout: Duration) -> TransportFailure { + let error = client(timeout).unwrap().get(url).send().await.unwrap_err(); + TransportFailure::of(&error, ResponseLimit::Timeout) + } + + #[tokio::test] + async fn transport_failures_are_classified_by_their_fix() { + let closed = TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap() + .port(); + let refused = failure( + format!("http://127.0.0.1:{closed}/"), + Duration::from_secs(5), + ) + .await; + assert_eq!(refused, TransportFailure::Connect); + + // A server that answers TLS with plain HTTP fails the handshake. + let plain = serve_once(|mut stream| { + read_request(&mut stream); + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n"); + }); + let tls = failure( + format!("https://127.0.0.1:{plain}/"), + Duration::from_secs(5), + ) + .await; + assert_eq!(tls, TransportFailure::Tls); + + // The silent server holds the connection until its test ends. + let (release, held) = std::sync::mpsc::channel::<()>(); + let silent = serve_once(move |stream| { + let _ = held.recv(); + drop(stream); + }); + let timeout = failure( + format!("http://127.0.0.1:{silent}/"), + Duration::from_millis(100), + ) + .await; + assert_eq!(timeout, TransportFailure::Timeout(ResponseLimit::Timeout)); + drop(release); + + let closing = serve_once(|mut stream| read_request(&mut stream)); + let closed_early = failure( + format!("http://127.0.0.1:{closing}/"), + Duration::from_secs(5), + ) + .await; + assert_eq!(closed_early, TransportFailure::Other); + } + + #[tokio::test] + async fn no_response_names_the_failure_and_the_next_step_for_its_service() { + let closed = TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap() + .port(); + let error = client(Duration::from_secs(5)) + .unwrap() + .get(format!("http://127.0.0.1:{closed}/")) + .send() + .await + .unwrap_err(); + let reported = Error::no_response( + &error, + "Price request", + "; nothing was retried", + "price", + ResponseLimit::Timeout, + false, + ); + assert_eq!(reported.kind, ErrorKind::Transport); + assert_eq!( + reported.message, + "Price request failed: could not connect; nothing was retried" + ); + assert_eq!( + reported.hint.as_deref(), + Some( + "Check the price URL, your network or proxy, and whether the price service is running." + ) + ); + // Raw transport text, which names the host and port, stays out of the message. + assert!(!reported.message.contains(&closed.to_string())); + } + + #[test] + fn each_failure_has_its_own_next_step() { + assert_eq!( + TransportFailure::Timeout(ResponseLimit::Timeout).next_step("API", false), + "Retry, or pass a larger --timeout." + ); + assert!( + TransportFailure::Tls + .next_step("API", false) + .contains("trusts its certificate") + ); + assert_eq!( + TransportFailure::Other.next_step("auth", false), + "Check the auth URL and whether the auth service is running." + ); + assert_ne!(TransportFailure::Tls.what(), TransportFailure::Other.what()); + let connect = TransportFailure::ConnectTimeout; + assert_eq!(connect.what(), "could not connect within 10 seconds"); + assert_eq!(crate::auth::CONNECT_TIMEOUT, Duration::from_secs(10)); + assert!( + connect + .next_step("API", false) + .contains("--timeout does not change") + ); + } + + #[test] + fn a_fixed_limit_is_named_and_never_blamed_on_timeout_flag() { + let fixed = TransportFailure::Timeout(ResponseLimit::Fixed(Duration::from_secs(30))); + assert_eq!(fixed.what(), "no response within 30 seconds"); + assert_eq!( + fixed.next_step("auth", false), + "Retry. The auth service has a fixed 30-second limit, which --timeout does not change." + ); + assert!(fixed.next_step("auth", true).ends_with( + "The auth service has a fixed 30-second limit, which --timeout does not change." + )); + let flag = TransportFailure::Timeout(ResponseLimit::Timeout); + assert_eq!(flag.what(), "no response within --timeout"); + for write in [false, true] { + assert!(!fixed.next_step("auth", write).contains("larger --timeout")); + assert!(flag.next_step("API", write).contains("larger --timeout")); + } + } + + #[test] + fn a_write_that_may_have_been_applied_is_never_told_to_retry() { + for failure in [ + TransportFailure::Timeout(ResponseLimit::Timeout), + TransportFailure::Timeout(ResponseLimit::Fixed(Duration::from_secs(30))), + TransportFailure::Other, + ] { + let step = failure.next_step("API", true); + assert!(step.starts_with("The change may have been applied. Look up the resource ID before you run the command again"), "{step}"); + assert!(!step.contains("Retry"), "{step}"); + } + // Nothing was sent when the connection or handshake failed, so the fix is the read's. + for failure in [ + TransportFailure::Connect, + TransportFailure::ConnectTimeout, + TransportFailure::Tls, + ] { + assert_eq!( + failure.next_step("API", true), + failure.next_step("API", false) + ); + } + } + + #[test] + fn a_connect_timeout_is_classified_before_a_timeout() { + use TransportFailure as F; + let limit = ResponseLimit::Timeout; + for (timeout, connect, tls, expected) in [ + (true, true, false, F::ConnectTimeout), + (true, true, true, F::ConnectTimeout), + (true, false, false, F::Timeout(limit)), + (false, true, true, F::Tls), + (false, false, true, F::Tls), + (false, true, false, F::Connect), + (false, false, false, F::Other), + ] { + assert_eq!( + F::classify(timeout, connect, tls, limit), + expected, + "{timeout} {connect} {tls}" + ); + } + } #[test] fn exit_codes_match_the_documented_table() { diff --git a/src/price.rs b/src/price.rs index 696c460..4c013d8 100644 --- a/src/price.rs +++ b/src/price.rs @@ -9,6 +9,7 @@ use crate::{ Error, Result, api::{BodyFailure, bounded_body}, auth, + error::ResponseLimit, payment::{Amount, AmountUnit, Currency}, }; use serde::{Deserialize, Serialize}; @@ -206,7 +207,16 @@ impl PriceService { .get(format!("{}/currency/BTCUSD/{time}", self.base)) .send() .await - .map_err(|_| Error::transport("Price request failed; check the price URL"))?; + .map_err(|error| { + Error::no_response( + &error, + "Price request", + "", + "price", + ResponseLimit::Timeout, + false, + ) + })?; let status = response.status().as_u16(); let body = bounded_body(response, MAX_PRICE_RESPONSE_BYTES) .await diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index 7238e31..16ad741 100644 --- a/tests/cli_integration.rs +++ b/tests/cli_integration.rs @@ -2557,6 +2557,131 @@ async fn a_described_change_sends_nothing_records_nothing_and_omits_private_valu assert!(server.received_requests().await.unwrap().is_empty()); } +#[test] +fn requests_without_a_response_say_why_and_name_the_next_step() { + let dir = private_tempdir(); + let closed = std::net::TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap(); + let closed = format!("http://{closed}"); + let api_hint = + "Check the API URL, your network or proxy, and whether the API service is running."; + let cases: [(Vec<&str>, &str, &str); 5] = [ + ( + vec!["wallets", "list", "--org", ORG], + "HTTP read request failed: could not connect", + api_hint, + ), + ( + vec!["wallets", "delete", WALLET, "--org", ORG], + "HTTP request failed: could not connect; a write may have been submitted. No mutation was retried.", + api_hint, + ), + ( + vec![ + "checkout", + "events", + "watch", + "--origin", + "https://shop.example.test", + ], + "Opening the checkout event stream failed: could not connect", + api_hint, + ), + ( + vec![ + "convert", + "10", + "usd", + "--to", + "btc", + "--price-url", + &closed, + ], + "Price request failed: could not connect", + "Check the price URL, your network or proxy, and whether the price service is running.", + ), + ( + vec!["login", "--no-browser", "--auth-url", &closed], + "Authentication request failed: could not connect; no token request was automatically retried", + "Check the auth URL, your network or proxy, and whether the auth service is running.", + ), + ]; + for (args, message, hint) in cases { + let failed = cli_at(dir.path(), &closed) + .env("VOLTAGE_STREAM_TOKEN", "private-stream-token") + .args(&args) + .assert() + .code(4); + let report = json_error(&failed); + assert_eq!(report["error"]["message"], message, "{args:?}"); + assert_eq!(report["error"]["hint"], hint, "{args:?}"); + if args[1] == "delete" { + // A write keeps its unknown-outcome detail. + assert_eq!(report["error"]["detail"]["outcome"], "unknown"); + } + } + + // A server that takes the request and never answers: a write may have been applied, so + // its hint says to look the resource up first, never to retry. + let silent = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let silent_url = format!("http://{}", silent.local_addr().unwrap()); + let writes: [Vec<&str>; 2] = [ + vec!["wallets", "delete", WALLET, "--org", ORG], + vec![ + "payments", + "send", + "--org", + ORG, + "--env", + ENV, + "--wallet", + WALLET, + "--currency", + "btc", + "--address", + "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7", + "--amount", + "1000", + "--unit", + "sats", + ], + ]; + for args in writes { + let failed = cli_at(dir.path(), &silent_url) + .args(["--timeout", "1"]) + .args(&args) + .assert() + .code(4); + let report = json_error(&failed); + let hint = report["error"]["hint"].as_str().unwrap(); + assert!( + hint.starts_with("The change may have been applied. Look up the resource ID before you run the command again"), + "{args:?}: {hint}" + ); + assert!(!hint.contains("Retry"), "{args:?}: {hint}"); + assert!( + report["error"]["message"] + .as_str() + .unwrap() + .contains("no response within --timeout"), + "{args:?}" + ); + assert_eq!(report["error"]["detail"]["outcome"], "unknown", "{args:?}"); + } + // A read that times out may simply be retried. + let read = cli_at(dir.path(), &silent_url) + .args(["--timeout", "1", "wallets", "list", "--org", ORG]) + .assert() + .code(4); + assert_eq!( + json_error(&read)["error"]["hint"], + "Retry, or pass a larger --timeout." + ); + drop(silent); +} + #[tokio::test] async fn output_file_refusals_name_the_path_problem_and_send_nothing() { let (server, dir) = fixture().await;