Skip to content

fix(server): resolve stdio server context cancellation bug #331

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

Merged
merged 2 commits into from
May 30, 2025

Conversation

sonirico
Copy link
Contributor

@sonirico sonirico commented May 25, 2025

Fix a critical bug in the stdio server's readNextLine function that was causing "Error reading input: context cancelled errors when Claude Desktop attempted to connect to MCP servers.

The issue was in the select-default pattern that would immediately bypass context cancellation checks and proceed with blocking ReadString operations even when the context was cancelled. This prevented proper cleanup and caused connection failures.

Changes:

  • Remove problematic select-default pattern in readNextLine goro
  • Simplify the read logic to directly call reader.ReadString('\n')
  • Maintain proper done channel handling for result communication
  • Ensure context cancellation is respected throughout the read operation

This resolves connection issues introduced in version 0.30.0 where the stdio transport became unreliable for client connections.

Description

Fixes #<issue_number> (if applicable)

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • MCP spec compatibility implementation
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring (no functional changes)
  • Performance improvement
  • Tests only (no functional changes)
  • Other (please describe):

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly

MCP Spec Compliance

  • This PR implements a feature defined in the MCP specification
  • Link to relevant spec section: Link text
  • Implementation follows the specification exactly

Additional Information

Summary by CodeRabbit

  • Refactor
    • Improved the internal process for reading lines, resulting in more streamlined and maintainable code. No changes to user-facing functionality.

Fix a critical bug in the stdio server's readNextLine function that was
causing "Error reading input" and "context cancelled" errors when Claude
Desktop attempted to connect to MCP servers.

The issue was in the select-default pattern that would immediately bypass
context cancellation checks and proceed with blocking ReadString operations
even when the context was cancelled. This prevented proper cleanup and
caused connection failures.

Changes:
- Remove problematic select-default pattern in readNextLine goroutine
- Simplify the read logic to directly call reader.ReadString('\n')
- Maintain proper done channel handling for result communication
- Ensure context cancellation is respected throughout the read operation

This resolves connection issues introduced in version 0.30.0 where the
stdio transport became unreliable for client connections.

Fixes: stdio server connection failures with Claude Desktop
Copy link
Contributor

coderabbitai bot commented May 25, 2025

"""

Walkthrough

This change refactors the readNextLine method in server/stdio.go by consolidating separate buffered channels for line and error into a single buffered channel carrying a struct with both values. It removes the done channel and nested select statements, simplifying concurrency and channel communication logic.

Changes

Files Change Summary
server/stdio.go Consolidated multiple channels into one struct-carrying channel in readNextLine; removed done channel and nested selects, simplifying concurrency handling.

Possibly related PRs

Suggested reviewers

  • robert-jackson-glean
    """

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (1.64.8)

Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2
Failed executing command with error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3071550 and ec6b89c.

📒 Files selected for processing (1)
  • server/stdio.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/stdio.go
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
server/stdio.go (1)

192-219: Optional: Clarify cancellation semantics and consider context-aware reads

  • Rename done to something like cancelCh to make its role explicit.
  • Add a brief comment above the goroutine explaining how closing done prevents sends after cancellation.
  • If you need to abort the blocking ReadString call itself (rather than just drop its result), consider wrapping reader with a context-aware I/O helper (e.g., ctxio.Reader) so that the read can be interrupted immediately on cancel.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 243a292 and b690e80.

📒 Files selected for processing (1)
  • server/stdio.go (1 hunks)
🔇 Additional comments (1)
server/stdio.go (1)

197-210: Simplified goroutine for cancellable reads
The removal of the select-default precheck and the direct call to reader.ReadString('\n') streamlines the flow. With defer close(done) and buffered errChan/readChan, once ctx.Done() is selected in the outer select, readNextLine returns immediately and any in-flight send in the goroutine is gracefully dropped. This change directly addresses the previous bug where reads would block indefinitely despite context cancellation.

@ibakshay
Copy link

Could someone take a look at this PR and get it merged please? :)

@corylanou
Copy link

I'm hitting this same problem. After looking at the code, I'm a little unsure what the done channel is accomplishing?

This is what I ended up with locally:

// readNextLine reads a single line from the input reader in a context-aware manner.
// It uses channels to make the read operation cancellable via context.
// Returns the read line and any error encountered. If the context is cancelled,
// returns an empty string and the context's error. EOF is returned when the input
// stream is closed.
func (s *StdioServer) readNextLine(ctx context.Context, reader *bufio.Reader) (string, error) {
	readChan := make(chan string, 1)
	errChan := make(chan error, 1)

	go func() {
		// If the context is cancelled, go ahead and exit the reader.
		select {
		case <-ctx.Done():
			// Nothing to do here, we're just exiting.
			return
		default:
			// Add default so this is a non-blocking read.
		}
		line, err := reader.ReadString('\n')
		if err != nil {
			errChan <- err
			return
		}
		readChan <- line
	}()

	select {
	case <-ctx.Done():
		return "", ctx.Err()
	case err := <-errChan:
		return "", err
	case line := <-readChan:
		return line, nil
	}
}

Effectively, if the context is cancelled, regardless of what the the go routine is doing it's going to exit. We don't care about what we've read so we just throw that away.

The done is perplexing to me, as if we get to the actual reader.ReadString that's going to block the go routine, so if we've passed the done and the context checks, they are now meaningless.

I think we just need to make sure that if we come into the go routine and the context is cancelled, exit out. The select case at the end of the statement is already going to do the right thing for the function as a whole.

Did I miss something else we were trying to accomplish?

I tested my solution with claude and it worked.

@sonirico
Copy link
Contributor Author

I'm hitting this same problem. After looking at the code, I'm a little unsure what the done channel is accomplishing?

This is what I ended up with locally:

// readNextLine reads a single line from the input reader in a context-aware manner.
// It uses channels to make the read operation cancellable via context.
// Returns the read line and any error encountered. If the context is cancelled,
// returns an empty string and the context's error. EOF is returned when the input
// stream is closed.
func (s *StdioServer) readNextLine(ctx context.Context, reader *bufio.Reader) (string, error) {
	readChan := make(chan string, 1)
	errChan := make(chan error, 1)

	go func() {
		// If the context is cancelled, go ahead and exit the reader.
		select {
		case <-ctx.Done():
			// Nothing to do here, we're just exiting.
			return
		default:
			// Add default so this is a non-blocking read.
		}
		line, err := reader.ReadString('\n')
		if err != nil {
			errChan <- err
			return
		}
		readChan <- line
	}()

	select {
	case <-ctx.Done():
		return "", ctx.Err()
	case err := <-errChan:
		return "", err
	case line := <-readChan:
		return line, nil
	}
}

Effectively, if the context is cancelled, regardless of what the the go routine is doing it's going to exit. We don't care about what we've read so we just throw that away.

The done is perplexing to me, as if we get to the actual reader.ReadString that's going to block the go routine, so if we've passed the done and the context checks, they are now meaningless.

I think we just need to make sure that if we come into the go routine and the context is cancelled, exit out. The select case at the end of the statement is already going to do the right thing for the function as a whole.

Did I miss something else we were trying to accomplish?

I tested my solution with claude and it worked.

Totally fair point. The done channel doesn’t really pull its weight here. It might make sense if it came from the sending side as part of a broader coordination strategy, but in this context it feels more like a leftover from a once well-intentioned abstraction.

To be honest, I didn’t write this part originally. In the PR I tried to make the smallest possible change to fix the actual bug without redecorating the whole room. Partly out of politeness, partly because small diffs tend to avoid the classic "why did you touch this unrelated thing?" conversation. That said, yeah, some of the code does have that "possibly AI-generated with a deep love for overengineering" vibe, especially around context and channel handling.

All that to say: I’m definitely not attached to the done channel. Your version is cleaner, and if it's working well with Claude, I’m happy to update the PR and drop it. Always in favor of simplicity when it works.

@sonirico sonirico force-pushed the fix/stdio-read-next-line branch from a1fea83 to 3071550 Compare May 28, 2025 14:44
@corylanou
Copy link

@sonirico I apologize if my question about the done channel came across as accusatory - that wasn't my intention. I know that was part of the original code you were working with. My question was directed more toward understanding the original architectural decision to make sure I wasn't overlooking something important (which I don't believe I was).

It's possible the channel was left over from a previous refactor. In any case, thank you for making the change. I'm hoping this can get merged soon since the current state blocks anyone trying to upgrade and use this with Claude.

Copy link
Collaborator

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagging @ezynda3 and @robert-jackson-glean to add any context as to why it was implemented like this in the first place.

LGTM, we should make a new release once this PR is merged.

@sonirico sonirico force-pushed the fix/stdio-read-next-line branch from 3071550 to ec6b89c Compare May 29, 2025 14:47
@ezynda3 ezynda3 merged commit d250b38 into mark3labs:main May 30, 2025
4 checks passed
@sonirico
Copy link
Contributor Author

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants