Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MCP client (SSE) has an endpoint discovery timeout that is too short #1390

Open
bwalding opened this issue Feb 26, 2025 · 1 comment
Open
Assignees
Labels
bug Something isn't working

Comments

@bwalding
Copy link
Contributor

bwalding commented Feb 26, 2025

Describe the bug

The MCP client (SSE) has to connect in less than 1000ms or it will fail with the error: Error from mcp-server: Transport was not connected or is already closed (in amongst some other trace).

The code leads the reader to believe that it is a 5000ms timeout, but that is not the case.

To Reproduce
Steps to reproduce the behavior:

  1. Have an MCP server that takes more than 1000ms to connect and serve up the POST endpoint as part of the SSE protocol
  2. Launch Goose with configuration to connect to that server.

(I discovered this because my native internet takes 800ms to connect - yay Australia! - vs. 1300ms with the VPN turned on)

Expected behavior

The MCP client should allow this POST endpoint URL to be sent up to 5s (according to code).

Actual behavior

Goose fails after waiting for ~1s.

The cause (IIUC)

See crates/mcp-client/src/transport/sse.rs

The code implies that the timeout for endpoint discovery is 5 seconds.

// Timeout for the endpoint discovery
const ENDPOINT_TIMEOUT_SECS: u64 = 5;
// Wait for the endpoint to be discovered before returning the handle
match timeout(
    Duration::from_secs(ENDPOINT_TIMEOUT_SECS),
    Self::wait_for_endpoint(post_endpoint_clone),
)

However the wait_for_endpoint code actually succeeds / fails in around 1000ms - 10 attempts x 100ms

/// Waits for the endpoint to be set, up to 10 attempts.
async fn wait_for_endpoint(
    post_endpoint: Arc<RwLock<Option<String>>>,
) -> Result<String, Error> {
    // Check every 100ms for the endpoint, for up to 10 attempts
    let check_interval = Duration::from_millis(100);
    let mut attempts = 0;
    let max_attempts = 10;

    while attempts < max_attempts {
        if let Some(url) = post_endpoint.read().await.clone() {
            return Ok(url);
        }
        tokio::time::sleep(check_interval).await;
        attempts += 1;
    }
    Err(Error::SseConnection("No endpoint discovered".to_string()))
}

Fixes
The wait_for_endpoint code should be amended to

  • try "forever" and wait for the caller's timeout to trigger
  • set max_attempts to ENDPOINT_TIMEOUT_SECS * 1000 / 100 (which would be 50 in the current scenario).
bwalding added a commit to bwalding/goose that referenced this issue Feb 26, 2025
@bwalding bwalding changed the title MCP client (SSE) has a endpoint discovery timeout that is too short MCP client (SSE) has an endpoint discovery timeout that is too short Feb 26, 2025
@bwalding
Copy link
Contributor Author

My other comment is that would be good if that error was more consistent with the root cause of the problem.

Currently: Error from mcp-server: Transport was not connected or is already closed
Better (but not great): Error from mcp-server: Transport error: SSE connection error: deadline has elapsed (this is what my change does).

But I think specifically saying "Timed out waiting for SSE endpoint discovery" would have pointed me in the right direction a lot quicker.

@wendytang wendytang added the bug Something isn't working label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants