diff --git a/crates/tinymcp/src/error/mod.rs b/crates/tinymcp/src/error/mod.rs index 50b5eea..5b9272b 100644 --- a/crates/tinymcp/src/error/mod.rs +++ b/crates/tinymcp/src/error/mod.rs @@ -6,9 +6,9 @@ //! existing message: callers match on variants, and message text is not a //! stable API. //! -//! # Why one variant carries structure +//! # Why some variants carry structure //! -//! [`Error::Unauthorized`] is the one a caller acts on rather than reports. A +//! [`Error::Unauthorized`] is one a caller acts on rather than reports. A //! server that answers 401 is *working* — it is reachable, it understood the //! request, and it wants credentials — so the right response is to offer the //! user a way to authenticate, not to show them a failure. Distinguishing that @@ -16,6 +16,15 @@ //! done, and text drifts. It carries its own fields so the decision is made on //! data. //! +//! [`Error::MissingRuntime`] is the other, and it is the same argument pointed +//! at the opposite conclusion. A 401 says *try again with credentials*; a +//! missing `uvx` says *stop*. No amount of retrying installs a binary, so a +//! caller that cannot tell this apart from a transport failure will schedule +//! reconnects forever against a host where the answer cannot change. It was a +//! [`Error::MalformedResponse`] carrying a formatted sentence, which was wrong +//! twice over — nothing was malformed and no response arrived — and which left +//! every caller that wanted to act on it substring-matching English. +//! //! # Endpoints in messages are always redacted //! //! Any variant carrying an endpoint holds the output of @@ -23,7 +32,7 @@ //! and user interfaces, and a URL with credentials in its userinfo would reach //! all three. -use tinymcp_bus::McpAuthChallenge; +use tinymcp_bus::{CommandKind, McpAuthChallenge}; /// Errors returned by this crate. #[derive(Debug, thiserror::Error)] @@ -55,6 +64,34 @@ pub enum Error { resource_metadata: Option, }, + /// A stdio server's launcher is not installed on this host. + /// + /// Terminal, and that is the point: the command was not found on the + /// resolved `PATH`, so the process was never started and no reconnect can + /// change the outcome. A caller should stop attempting and surface an + /// install path — see the module note on why this is a variant rather than + /// a message. + /// + /// `runtime` is what to install, not what was typed. `command` is the + /// launcher as configured, kept verbatim so a user can see the exact string + /// that was looked up — an absolute path that is wrong for this machine + /// reads very differently from a bare `uvx`. + /// + /// The guidance is in the message for the reason given on + /// [`Self::Unauthorized`]: an error that has crossed an RPC boundary and + /// been re-reported as a string still has to be useful to whoever reads it. + #[error("{}", missing_runtime_guidance(command, *runtime))] + MissingRuntime { + /// The launcher that was not found, exactly as it was configured. + command: String, + /// The runtime that launcher belongs to, and therefore what to install. + /// + /// [`CommandKind::Binary`] means the command was not recognised as + /// belonging to a known ecosystem, so there is nothing to name beyond + /// the command itself. + runtime: CommandKind, + }, + /// A remote server answered with a status other than success. /// /// The body is rendered, bounded, because it is where the server says @@ -233,6 +270,17 @@ impl Error { } } + /// Builds a [`Self::MissingRuntime`] for a launcher that was not found. + /// + /// The runtime is classified from the command name rather than passed in, + /// so every producer of this variant agrees about which ecosystem a + /// launcher belongs to. + pub(crate) fn missing_runtime(command: impl Into) -> Self { + let command = command.into(); + let runtime = crate::transport::stdio::spawn_env::required_runtime(&command); + Self::MissingRuntime { command, runtime } + } + /// Whether this error means "the server wants credentials". /// /// Callers use this instead of inspecting a message, which is the whole @@ -253,6 +301,30 @@ impl Error { matches!(self, Self::Unauthorized { .. }) } + /// Whether this error means "the runtime this server needs is not here". + /// + /// Terminal. A caller uses this to stop retrying and offer an install path, + /// which is the whole reason [`Self::MissingRuntime`] is a variant: the + /// condition used to be reachable only by substring-matching a sentence, + /// and a supervisor that could not see it scheduled reconnects on a + /// five-minute ceiling against a binary that was never going to appear. + /// + /// # Examples + /// + /// ``` + /// # use tinymcp::{CommandKind, Error}; + /// let error = Error::MissingRuntime { + /// command: "uvx".into(), + /// runtime: CommandKind::Python, + /// }; + /// assert!(error.is_missing_runtime()); + /// assert!(!error.is_unauthorized()); + /// ``` + #[must_use] + pub const fn is_missing_runtime(&self) -> bool { + matches!(self, Self::MissingRuntime { .. }) + } + /// Whether the 401 advertised OAuth. /// /// `false` for every error that is not a 401. A server that advertises @@ -284,6 +356,39 @@ impl From for Error { /// `std::result::Result`. pub type Result = std::result::Result; +/// The sentence a user reads when a stdio launcher is not installed. +/// +/// "Not found" is almost never what they need to hear; "this server needs +/// Node.js" is. The recognised runtimes get a name and an address, and anything +/// else gets a path hint, because naming the wrong ecosystem is worse than +/// naming none. +/// +/// Lives here, beside the variant, so [`Error::MissingRuntime`]'s `Display` and +/// [`crate::transport::stdio::spawn_env::missing_command_error`] — which +/// delegates to it — cannot drift into two different sentences. +pub(crate) fn missing_runtime_guidance(command: &str, runtime: CommandKind) -> String { + match runtime { + CommandKind::Node => format!( + "`{command}` was not found. This MCP server needs Node.js, which does not appear \ + to be installed, or is not on this application's PATH. Install Node.js from \ + https://nodejs.org and restart the application." + ), + CommandKind::Python => format!( + "`{command}` was not found. This MCP server needs uv (Python), which does not \ + appear to be installed. Install it from https://docs.astral.sh/uv/ and restart \ + the application." + ), + // `CommandKind::Binary`, and whatever is added to that non-exhaustive + // enum next. A runtime this crate cannot name is exactly the case the + // generic sentence exists for, so a new variant degrades to correct + // guidance rather than to a compile error in every downstream crate. + _ => format!( + "`{command}` was not found on this application's PATH. Install it, or its runtime, \ + make sure it is available in your shell, then restart the application." + ), + } +} + /// How much of a failure body to put in a message. /// /// These reach logs, telemetry, and user-facing errors. An upstream answering a diff --git a/crates/tinymcp/src/registry/supervisor/test.rs b/crates/tinymcp/src/registry/supervisor/test.rs index f01d053..97d5595 100644 --- a/crates/tinymcp/src/registry/supervisor/test.rs +++ b/crates/tinymcp/src/registry/supervisor/test.rs @@ -7,6 +7,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +use std::collections::BTreeMap; use std::time::{Duration, Instant}; use axum::routing::post; @@ -270,6 +271,63 @@ async fn a_failed_reconnect_earns_a_backoff_penalty() { assert!(connections.last_error("srv-1").await.is_some()); } +#[tokio::test] +async fn a_missing_runtime_is_terminal_and_earns_no_backoff_penalty() { + // A binary that is not installed will not appear because we waited five + // minutes, so the supervisor must park the server instead of scheduling + // another attempt against it. + let store = Store::open_in_memory().unwrap(); + store + .insert_server(&install("srv-1", Transport::Stdio, true)) + .unwrap(); + // `install` launches through `npx`; an empty PATH forces the + // missing-command branch whether or not this machine has Node. + store + .set_env_values( + "srv-1", + &BTreeMap::from([( + "PATH".to_string(), + "/tinymcp/deliberately/does/not/exist".to_string(), + )]), + ) + .unwrap(); + + let connections = Connections::new(); + let oauth = OAuthFlow::new(None).unwrap(); + let mut supervisor = supervisor(); + let base = Instant::now(); + + supervisor.tick(&store, &connections, &oauth, base).await; + + assert!(!connections.is_connected("srv-1").await); + assert_eq!( + supervisor.backed_off_count(), + 0, + "a backoff promises that waiting helps, and here it cannot" + ); + assert_eq!(supervisor.terminally_failed_count(), 1); + + // Far past any backoff window, so a penalised server would certainly be + // retried by now. A parked one must not be. + let first_error = connections.last_error("srv-1").await; + assert!(first_error.is_some(), "the first attempt is still reported"); + supervisor + .tick(&store, &connections, &oauth, base + BACKOFF_MAX * 2) + .await; + + assert_eq!(supervisor.backed_off_count(), 0); + assert_eq!(supervisor.terminally_failed_count(), 1); + + // Disabling clears the verdict, so installing the runtime and toggling the + // server is a way back. + store.update_enabled("srv-1", false).unwrap(); + supervisor + .tick(&store, &connections, &oauth, base + BACKOFF_MAX * 3) + .await; + + assert_eq!(supervisor.terminally_failed_count(), 0); +} + #[tokio::test] async fn a_server_inside_its_backoff_window_is_not_retried() { let store = Store::open_in_memory().unwrap(); diff --git a/crates/tinymcp/src/registry/supervisor/types.rs b/crates/tinymcp/src/registry/supervisor/types.rs index 4eafbf7..c095830 100644 --- a/crates/tinymcp/src/registry/supervisor/types.rs +++ b/crates/tinymcp/src/registry/supervisor/types.rs @@ -1,6 +1,6 @@ //! The supervisor and its cycle. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::time::{Duration, Instant}; use super::backoff::BackoffState; @@ -82,6 +82,20 @@ pub struct Supervisor { /// that map, so a counter kept there would erase the very history it exists /// to accumulate. timeouts: HashMap, + /// Servers whose last attempt failed in a way retrying cannot fix. + /// + /// Today that is exactly [`Error::MissingRuntime`](crate::Error::MissingRuntime): + /// the launcher is not installed, so the process never started and the next + /// attempt will not start it either. These are skipped entirely rather than + /// given a backoff, because a backoff is a promise that waiting helps. + /// + /// Distinct from [`Self::timeouts`], which counts a *live* session going + /// quiet: that is a reason to wait longer, this is a reason to stop. + /// + /// Cleared when the user disables the server, so toggling it off and on is + /// the recovery path after installing the runtime — the same gesture that + /// already clears a backoff penalty. + terminal: HashSet, } impl Supervisor { @@ -98,6 +112,7 @@ impl Supervisor { proxy, backoff: HashMap::new(), timeouts: HashMap::new(), + terminal: HashSet::new(), } } @@ -154,9 +169,13 @@ impl Supervisor { // The disable path owns tearing the connection down. All that // is left here is to forget any backoff, so re-enabling gets an // immediate attempt rather than inheriting an old penalty. The - // timeout streak goes with it for the same reason. + // timeout streak goes with it for the same reason, and so does + // a terminal verdict — which is the only way back from one: a + // user who installs the missing runtime toggles the server off + // and on to have it tried again. self.backoff.remove(&server_id); self.timeouts.remove(&server_id); + self.terminal.remove(&server_id); continue; } @@ -166,6 +185,13 @@ impl Supervisor { continue; } + // Checked after the liveness block, not before it: a live + // connection is still worth probing and tearing down, and only the + // attempt that follows is pointless. + if self.terminal.contains(&server_id) { + continue; + } + if !self .backoff .entry(server_id.clone()) @@ -182,6 +208,7 @@ impl Supervisor { Ok(tools) => { self.backoff.remove(&server_id); self.timeouts.remove(&server_id); + self.terminal.remove(&server_id); tracing::info!( server_id = %server_id, qualified_name = %server.qualified_name, @@ -189,6 +216,18 @@ impl Supervisor { "reconnected" ); } + Err(error) if error.is_missing_runtime() => { + // No backoff entry: a penalty says "wait, then try again", + // and there is nothing to wait for. The server is parked + // until the user disables and re-enables it. + self.backoff.remove(&server_id); + self.terminal.insert(server_id.clone()); + tracing::warn!( + server_id = %server_id, + qualified_name = %server.qualified_name, + "connecting failed and will not be retried: {error}" + ); + } Err(error) => { let state = self.backoff.entry(server_id.clone()).or_default(); state.record_failure(now); @@ -307,4 +346,13 @@ impl Supervisor { pub fn consecutive_timeouts(&self, server_id: &str) -> u32 { self.timeouts.get(server_id).copied().unwrap_or(0) } + + /// How many servers the supervisor has parked as unretryable. + /// + /// Disjoint from [`Self::backed_off_count`] by construction: a terminal + /// verdict removes any penalty rather than adding to one. + #[must_use] + pub fn terminally_failed_count(&self) -> usize { + self.terminal.len() + } } diff --git a/crates/tinymcp/src/transport/stdio/mod.rs b/crates/tinymcp/src/transport/stdio/mod.rs index d81ff06..e04d646 100644 --- a/crates/tinymcp/src/transport/stdio/mod.rs +++ b/crates/tinymcp/src/transport/stdio/mod.rs @@ -132,9 +132,7 @@ impl McpStdioClient { command = %self.command, "the stdio command was not found on the resolved path" ); - return Err(Error::malformed(spawn_env::missing_command_error( - &self.command, - ))); + return Err(Error::missing_runtime(self.command.clone())); } let mut command = Command::new(&self.command); diff --git a/crates/tinymcp/src/transport/stdio/spawn_env/mod.rs b/crates/tinymcp/src/transport/stdio/spawn_env/mod.rs index f1020b8..00764d0 100644 --- a/crates/tinymcp/src/transport/stdio/spawn_env/mod.rs +++ b/crates/tinymcp/src/transport/stdio/spawn_env/mod.rs @@ -42,6 +42,7 @@ use std::path::{Path, PathBuf}; use std::process::Stdio; use std::time::Duration; +use tinymcp_bus::CommandKind; use tokio::sync::OnceCell; /// The separator this platform puts between path entries. @@ -388,24 +389,44 @@ fn executable_candidates(base: PathBuf) -> Vec { /// ``` #[must_use] pub fn missing_command_error(command: &str) -> String { + crate::error::missing_runtime_guidance(command, required_runtime(command)) +} + +/// Which runtime a stdio command needs, from the command name alone. +/// +/// The launcher is the only evidence available at this point — the install's +/// own [`CommandKind`] is not threaded into the transport — so the name is what +/// classifies it. A path is reduced to its final component and lowercased +/// first, because `/opt/homebrew/bin/NPX` needs Node.js just as much as `npx` +/// does. +/// +/// [`CommandKind::Binary`] is the honest answer for anything unrecognised: the +/// command needs *something*, and guessing at which ecosystem would send a user +/// to install the wrong thing. +/// +/// This is what [`missing_command_error`] switches on, and what +/// [`crate::Error::MissingRuntime`] carries, so the sentence a user reads and +/// the value a caller branches on can never disagree about which runtime is +/// missing. +/// +/// # Examples +/// +/// ``` +/// # use tinymcp::transport::stdio::spawn_env::required_runtime; +/// # use tinymcp::CommandKind; +/// assert_eq!(required_runtime("npx"), CommandKind::Node); +/// assert_eq!(required_runtime("/opt/homebrew/bin/uvx"), CommandKind::Python); +/// assert_eq!(required_runtime("some-bespoke-server"), CommandKind::Binary); +/// ``` +#[must_use] +pub fn required_runtime(command: &str) -> CommandKind { let lowered = command.to_ascii_lowercase(); let base = lowered.rsplit(['/', '\\']).next().unwrap_or(&lowered); match base { - "npx" | "npm" | "node" => format!( - "`{command}` was not found. This MCP server needs Node.js, which does not appear \ - to be installed, or is not on this application's PATH. Install Node.js from \ - https://nodejs.org and restart the application." - ), - "uvx" | "uv" => format!( - "`{command}` was not found. This MCP server needs uv (Python), which does not \ - appear to be installed. Install it from https://docs.astral.sh/uv/ and restart \ - the application." - ), - _ => format!( - "`{command}` was not found on this application's PATH. Install it, or its runtime, \ - make sure it is available in your shell, then restart the application." - ), + "npx" | "npm" | "node" => CommandKind::Node, + "uvx" | "uv" => CommandKind::Python, + _ => CommandKind::Binary, } } diff --git a/crates/tinymcp/src/transport/stdio/test.rs b/crates/tinymcp/src/transport/stdio/test.rs index e489933..da08d07 100644 --- a/crates/tinymcp/src/transport/stdio/test.rs +++ b/crates/tinymcp/src/transport/stdio/test.rs @@ -10,7 +10,7 @@ use super::McpStdioClient; use crate::Error; -use tinymcp_bus::{LATEST_PROTOCOL_VERSION, McpClientIdentityConfig}; +use tinymcp_bus::{CommandKind, LATEST_PROTOCOL_VERSION, McpClientIdentityConfig}; /// A client for `command` with no arguments and no environment. fn client_for(command: &str, env: Vec<(String, String)>) -> McpStdioClient { @@ -65,6 +65,52 @@ async fn a_missing_uv_runtime_says_so_by_name() { assert!(error.to_string().contains("uv"), "{error}"); } +#[tokio::test] +async fn a_missing_runtime_is_a_variant_a_caller_can_branch_on() { + // The contract is the variant, not the sentence. A caller deciding whether + // to stop retrying must not have to substring-match English, which is what + // it had to do while this was a `MalformedResponse` carrying a message. + for (command, expected) in [ + ("uvx", CommandKind::Python), + ("uv", CommandKind::Python), + ("npx", CommandKind::Node), + ("npm", CommandKind::Node), + ("node", CommandKind::Node), + // Unrecognised launchers are still terminal; there is just no ecosystem + // to name. + ("some-bespoke-server", CommandKind::Binary), + ] { + // Bare names only. An absolute path would have to not exist on the + // machine running the test, and `required_runtime`'s own tests already + // cover path stripping and case without touching the filesystem. + let client = client_for(command, empty_path()); + + let error = client + .initialize() + .await + .expect_err("a launcher that is not on the path"); + + assert!( + error.is_missing_runtime(), + "`{command}` produced {error:?}, which no caller can act on" + ); + assert!( + !error.is_unauthorized(), + "`{command}` must not look like a credential problem" + ); + match &error { + Error::MissingRuntime { + command: got, + runtime, + } => { + assert_eq!(got, command, "the command is kept verbatim"); + assert_eq!(*runtime, expected, "`{command}` needs {expected:?}"); + } + other => panic!("`{command}` produced {other:?}, not MissingRuntime"), + } + } +} + #[tokio::test] async fn a_missing_command_names_the_command_it_looked_for() { let client = client_for("some-bespoke-server", empty_path());