fix(server): resolve stdio server context cancellation bug #331
Conversation
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
|
""" WalkthroughThis change refactors the Changes
Possibly related PRs
Suggested reviewers
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/stdio.go (1)
192-219: Optional: Clarify cancellation semantics and consider context-aware reads
- Rename
doneto something likecancelChto make its role explicit.- Add a brief comment above the goroutine explaining how closing
doneprevents sends after cancellation.- If you need to abort the blocking
ReadStringcall itself (rather than just drop its result), consider wrappingreaderwith 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
📒 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 theselect-defaultprecheck and the direct call toreader.ReadString('\n')streamlines the flow. Withdefer close(done)and bufferederrChan/readChan, oncectx.Done()is selected in the outerselect,readNextLinereturns 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.
|
Could someone take a look at this PR and get it merged please? :) |
|
I'm hitting this same problem. After looking at the code, I'm a little unsure what the 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 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
force-pushed
the
fix/stdio-read-next-line
branch
from
a1fea83 to
3071550
Compare
|
@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. |
sonirico
force-pushed
the
fix/stdio-read-next-line
branch
from
3071550 to
ec6b89c
Compare
|
👏 |
Fix a critical bug in the stdio server's
readNextLinefunction that was causing"Error reading input: context cancellederrors 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
ReadStringoperations even when the context was cancelled. This prevented proper cleanup and caused connection failures.Changes:
readNextLinegororeader.ReadString('\n')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
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit