Skip to content

A failed Client::connect cannot be retried: the transport is consumed and the second attempt blames the caller #131

Description

@RomanEmreis

Client::connect takes &mut self and returns Result<(), Error>, and since #125 a stdio server that cannot be spawned is one of the errors it returns rather than a panic. The natural thing for an embedder to do with that -- retry, or fall back to another command -- does not work, and the second attempt misreports why:

let mut client = Client::new()
    .with_options(|opt| opt.with_stdio("neva-nonexistent-server-repro", []));

for attempt in 1..=2 {
    match client.connect().await {
        Ok(()) => println!("attempt {attempt}: Ok"),
        Err(e) => println!("attempt {attempt}: Err({e})"),
    }
}
attempt 1: Err(failed to start MCP server `neva-nonexistent-server-repro`: No such file or directory (os error 2))
attempt 2: Err(Transport protocol must be specified)

The second message is untrue. The transport was specified, by with_stdio on the line above.

Where

McpOptions::transport moves the configured transport out and leaves nothing behind (client/options.rs:461):

pub(crate) fn transport(&mut self) -> TransportProto {
    let transport = self.proto.take().unwrap_or_default();

connect calls it once per attempt (client/setup.rs:148), so the second call gets TransportProto::None. Nothing ever writes back to proto -- only with_stdio and with_http assign it.

The failure then surfaces two steps away from its cause:

Step What happens
client/setup.rs:148 transport() yields TransportProto::None
transport.rs:212 None::start() returns Ok(TransportHandle::detached(..)), so connect continues
transport.rs:158 init() sends over TransportProtoSender::None, which reports "Transport protocol must be specified"

Scope

This is only about retrying a failed connect. There is no reconnect case to worry about: disconnect takes self by value, so a client that has connected successfully is consumed on the way out and a second connection means a new Client.

Nor is it new behaviour. It was simply unreachable before #125, because the one failure a caller was most likely to want to retry -- the server process not starting -- ended the process instead of returning. Making that path recoverable is what put a caller in a position to hit this.

Shape of the fix

Two candidates, depending on how far connect should be retryable.

Narrow: put the transport back when start fails. Transport::start takes &mut self, so the local in connect still owns the transport after an error, and the stdio client is untouched by a failed handshake -- nothing has been moved into a task yet. Restoring options.proto on that path and returning the error makes exactly the #125 case retryable, and leaves everything else as it is. It needs a setter next to transport().

This does not generalize: once start succeeds the transport moves into RequestHandler, so a later failure in init cannot be undone this way -- and should not be, since a retry there needs a fresh child process anyway.

Structural: store the configuration, not a live transport. proto currently holds a constructed StdIoClient / HttpClient. If it held what with_stdio and with_http were given, connect could build a transport per attempt and be retryable uniformly, including after init fails. Bigger change, and it touches the dual-mode header plumbing that transport() currently does on the way out.

Whichever way, TransportProto::None::start returning Ok is worth a second look: it lets a client with no transport get as far as init before anything objects, which is why the message a caller sees names the wrong problem.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions