Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 108 additions & 3 deletions crates/tinymcp/src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,33 @@
//! 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
//! from a transport error by matching on message text is how it used to be
//! 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
//! [`crate::redact_endpoint`], never the raw URL. Errors reach logs, telemetry,
//! 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)]
Expand Down Expand Up @@ -55,6 +64,34 @@ pub enum Error {
resource_metadata: Option<String>,
},

/// 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
Expand Down Expand Up @@ -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<String>) -> 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
Expand All @@ -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
Expand Down Expand Up @@ -284,6 +356,39 @@ impl From<serde_json::Error> for Error {
/// `std::result::Result<T, Error>`.
pub type Result<T> = std::result::Result<T, Error>;

/// 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
Expand Down
58 changes: 58 additions & 0 deletions crates/tinymcp/src/registry/supervisor/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
52 changes: 50 additions & 2 deletions crates/tinymcp/src/registry/supervisor/types.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<String, u32>,
/// 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<String>,
}

impl Supervisor {
Expand All @@ -98,6 +112,7 @@ impl Supervisor {
proxy,
backoff: HashMap::new(),
timeouts: HashMap::new(),
terminal: HashSet::new(),
}
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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())
Expand All @@ -182,13 +208,26 @@ 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,
tools = tools.len(),
"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);
Expand Down Expand Up @@ -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()
}
}
4 changes: 1 addition & 3 deletions crates/tinymcp/src/transport/stdio/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading