fix: add timeout for SSE response waiting to prevent indefinite blocking #668
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughStart and SendRequest in the SSE transport were made context-aware: timeouts are computed from defaults then clamped to the request/context deadline; both return context errors if the deadline already passed, use timers for timeout branches, and ensure timers and response-channel entries are cleaned up on timeout or cancellation. New tests validate timeout behavior and cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a timeout mechanism to prevent indefinite blocking when waiting for SSE responses in the SendRequest method. The implementation addresses issue #649 by introducing a 60-second default timeout that respects context deadlines when present.
Key Changes:
- Added response timeout logic that defaults to 60 seconds or uses remaining context deadline time (whichever is shorter)
- Created a timer to enforce the timeout and prevent indefinite waiting
- Added proper cleanup with
defer timer.Stop()to prevent timer leaks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse.go
🔇 Additional comments (1)
client/transport/sse.go (1)
458-461: LGTM! Timeout case properly implemented.The timeout handling correctly performs cleanup and returns an informative error message. This successfully addresses the PR objective of preventing indefinite blocking when the MCP server fails to respond.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/sse_test.go (1)
1126-1127: Consider more lenient lower bound for CI stability.The duration assertion requires
>= 1.5sfor a 2s timeout, providing only a 500ms buffer. On slow CI systems or under load, the timeout might fire slightly earlier due to scheduling delays or timer precision, potentially causing flaky test failures.Consider adjusting to a more lenient lower bound:
- require.GreaterOrEqual(t, duration, 1500*time.Millisecond) + require.GreaterOrEqual(t, duration, 1400*time.Millisecond)Or use a percentage-based bound for more flexibility:
- require.GreaterOrEqual(t, duration, 1500*time.Millisecond) - require.LessOrEqual(t, duration, 2500*time.Millisecond) + expectedTimeout := 2 * time.Second + require.GreaterOrEqual(t, duration, expectedTimeout*7/10) // 70% of expected + require.LessOrEqual(t, duration, expectedTimeout*13/10) // 130% of expected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/sse_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse_test.go
🧬 Code graph analysis (1)
client/transport/sse_test.go (6)
mcptest/mcptest.go (1)
NewServer(44-54)mcp/types.go (3)
Request(179-182)JSONRPCRequest(335-340)NewRequestId(270-272)client/transport/sse.go (1)
NewSSE(85-112)client/transport/interface.go (1)
JSONRPCRequest(58-64)testdata/mockstdio_server.go (1)
JSONRPCRequest(14-19)client/transport/error.go (1)
Error(6-8)
🔇 Additional comments (3)
client/transport/sse_test.go (3)
1130-1178: LGTM! Well-designed deadline precedence test.The test correctly verifies that the request-level context deadline takes precedence. The flexible error message assertion (accepting either "timeout" or "deadline exceeded") is good design that accommodates different error paths.
1209-1232: Internal state access is acceptable but couples test to implementation.The test directly accesses
transport.muandtransport.responsesto verify cleanup, which is acceptable for same-package tests in Go. The use ofRLock()for thread-safe access is correct, and the test serves an important purpose: preventing resource leaks on timeout.However, note that this couples the test to implementation details—if the
responsesfield is renamed or the cleanup mechanism changes, this test will need updates.
1081-1234: Comprehensive timeout test coverage.The test function provides thorough coverage of the timeout feature:
- Verifies timeout fires when server doesn't respond
- Confirms context deadline takes precedence over default timeout
- Ensures proper cleanup of internal response channels
The tests follow Go testing best practices: use of subtests, testify assertions, proper context handling, and appropriate mock server setup. The test structure aligns well with the PR objective to prevent indefinite blocking in SSE SendRequest.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/transport/sse.go (1)
425-488: Missing cleanup on io.ReadAll error creates resource leakIf
io.ReadAll(resp.Body)fails at lines 436-438,deleteResponseChan()is not called, leaving an entry inc.responsesthat may never be cleaned up if the server never sends an SSE response. AdddeleteResponseChan()before returning the error:body, err := io.ReadAll(resp.Body) resp.Body.Close() if err != nil { + deleteResponseChan() return nil, fmt.Errorf("failed to read response body: %w", err) }Note: The timeout calculation does not need to incorporate
c.httpClient.Timeout. ThehttpClient.Timeoutapplies to the HTTP request/response phase (theDo()call), whileresponseTimeoutapplies to the subsequent channel wait for the SSE response—these are sequential phases with separate timeout concerns.
🧹 Nitpick comments (1)
client/transport/sse_test.go (1)
1081-1278: Good coverage; reduce test flakiness from tight timing windows / sleeps
- The new timeout coverage is solid (timeout, ctx precedence, cleanup, expired deadline).
- To avoid CI flakes, consider:
- Removing Line 1226
time.Sleep(50 * time.Millisecond)(cleanup happens beforeSendRequestreturns).- Widening/softening wall-clock bounds (e.g., assert
duration >= (timeout - jitter)and< (timeout + bigger jitter)), sinceresponseTimeoutis computed after some work and schedulers can add noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/transport/sse.go(2 hunks)client/transport/sse_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse.goclient/transport/sse_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/sse_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse.goclient/transport/sse_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Functions that are handlers or long-running must accept context.Context as the first parameter
Applied to files:
client/transport/sse.go
|
We're experiencing the same issue at Ona. When an MCP tool call times out, the agent gets stuck because the SSE response handling blocks indefinitely even after context cancellation. Our investigation found that the We've implemented a workaround by wrapping the Would be great to get this fix merged. @ezynda3 - would you be able to review this PR? cc: #649 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/transport/sse_test.go (3)
1087-1087: Check the type assertion result for http.Flusher.The type assertion
flusher, _ := w.(http.Flusher)ignores theokreturn value in multiple test handlers. While httptest servers typically implement Flusher, explicitly checking prevents potential panics and makes the test more robust.Apply this pattern at each location:
-flusher, _ := w.(http.Flusher) +flusher, ok := w.(http.Flusher) +if !ok { + http.Error(w, "Streaming unsupported", http.StatusInternalServerError) + return +}Also applies to: 1136-1136, 1186-1186, 1239-1239
1127-1128: Consider tightening timeout tolerance or documenting the rationale.The timeout assertions allow for 70%-130% of the expected duration, which is a fairly broad window. While some tolerance is necessary for test stability across different CI environments, consider whether tighter bounds (e.g., 80%-120%) would be sufficient, or add a comment explaining why the broader tolerance is needed.
1234-1277: Move nested subtest to the top level.The
AlreadyExpiredDeadlinesubtest is nested insideTimeoutCleansUpResponseChannel, but it tests a distinct scenario (sending a request with an already-expired context) that doesn't logically belong as a child test. Move it to be a sibling of the other three subtests withinTestSSE_SendRequest_Timeout.Adjust the structure:
require.Equal(t, 0, finalCount) - t.Run("AlreadyExpiredDeadline", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // ... server setup ... - })) - defer server.Close() - // ... rest of test ... - }) }) + + t.Run("AlreadyExpiredDeadline", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // ... server setup ... + })) + defer server.Close() + // ... rest of test ... + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/transport/sse.go(2 hunks)client/transport/sse_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/sse.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/sse_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse_test.go
🧬 Code graph analysis (1)
client/transport/sse_test.go (5)
mcptest/mcptest.go (1)
NewServer(44-54)mcp/types.go (2)
Request(179-182)JSONRPCRequest(335-340)client/transport/sse.go (1)
NewSSE(85-112)client/transport/interface.go (1)
JSONRPCRequest(58-64)client/transport/error.go (1)
Error(6-8)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/sse_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse_test.go
🧬 Code graph analysis (1)
client/transport/sse_test.go (6)
mcptest/mcptest.go (1)
NewServer(44-54)mcp/types.go (3)
Request(179-182)JSONRPCRequest(335-340)NewRequestId(270-272)client/transport/sse.go (1)
NewSSE(85-112)client/transport/interface.go (1)
JSONRPCRequest(58-64)testdata/mockstdio_server.go (1)
JSONRPCRequest(14-19)client/transport/error.go (1)
Error(6-8)
🔇 Additional comments (3)
client/transport/sse_test.go (3)
1081-1129: LGTM: Timeout behavior properly tested.The test correctly validates that SendRequest times out when the server never responds, with appropriate timing bounds and error checking.
1131-1179: LGTM: Context deadline precedence verified.The test correctly validates that the request context deadline takes precedence and enforces appropriate timing bounds.
1181-1233: LGTM: Response channel cleanup properly verified.The test correctly validates that timeout scenarios clean up internal response channels, preventing memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/sse_test.go (1)
1081-1280: Excellent timeout test coverage!The timeout tests comprehensively cover the key scenarios:
- Server never responds
- Context deadline precedence
- Response channel cleanup after timeout
- Already-expired deadline handling
The past review comments have been addressed correctly:
- Line 1278: Error message typo fixed (no space in "context.DeadlineExceeded")
- Line 1236:
AlreadyExpiredDeadlineis now correctly positioned as a sibling test rather than nestedThe tests follow Go best practices with proper isolation, cleanup, and context usage. The duration checks use reasonable tolerances (70-130%) to avoid flakiness while still validating timeout behavior.
Note: The
TimeoutCleansUpResponseChanneltest accesses internal state (transport.muandtransport.responses) for white-box verification of cleanup. This is acceptable for testing but creates coupling to implementation details, so these assertions may need updates if the internal structure changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/sse_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse_test.go
🧬 Code graph analysis (1)
client/transport/sse_test.go (4)
client/transport/sse.go (1)
NewSSE(85-112)client/transport/interface.go (1)
JSONRPCRequest(58-64)testdata/mockstdio_server.go (1)
JSONRPCRequest(14-19)client/transport/error.go (1)
Error(6-8)
There was a problem hiding this comment.
@everfid-ever Nice fix for the indefinite blocking issue! I noticed one thing while reviewing:
There's a missing deleteResponseChan() call on line 437 when io.ReadAll fails. The response channel gets registered at line 417, and if reading the body fails, it'll leak that channel entry in c.responses since it never gets cleaned up (the SSE response with that request ID will never arrive to trigger cleanup).
if err != nil {
deleteResponseChan() // <- missing
return nil, fmt.Errorf("failed to read response body: %w", err)
}@coderabbitai what do you think - am I reading this correctly?
You're right, thank you for your remind, I've already fixed it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1282 @everfid-ever Thank you for addressing this! I can confirm the fix is now in place at line 437 - the 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/transport/sse.go (2)
180-205: Context-aware timeout implementation looks solid.The timeout logic correctly handles:
- Computing the effective timeout from context deadline and default (30s)
- Early return when deadline already passed
- Timer cleanup via defer on all paths
- Appropriate context cancellation on timeout
Consider introducing sentinel errors to allow callers to programmatically distinguish timeout errors from other failures:
🔎 Suggested refactor for typed timeout errors
Define package-level sentinel errors at the top of the file:
// Sentinel errors for timeout scenarios var ( ErrEndpointTimeout = errors.New("endpoint timeout") ErrResponseTimeout = errors.New("response timeout") )Then wrap the timeout error in Start:
case <-timer.C: cancel() - return fmt.Errorf("timeout waiting for endpoint after %v", endpointTimeout) + return fmt.Errorf("%w after %v", ErrEndpointTimeout, endpointTimeout)This enables callers to use
errors.Is(err, transport.ErrEndpointTimeout)for specific handling rather than string matching.Based on learnings, the 30-second default timeout matches the original SSE implementation behavior.
458-483: Timeout implementation correctly prevents indefinite blocking.This implementation successfully addresses issue #649 by:
- Respecting context deadlines when computing timeout
- Handling already-expired deadlines defensively
- Cleaning up response channels on timeout via
deleteResponseChan- Returning informative timeout errors
The 60-second default (vs. 30s in Start) is appropriate since response waiting may need more time than endpoint discovery.
For consistency with the Start method suggestion, consider wrapping the timeout error with the sentinel
ErrResponseTimeout:🔎 Suggested refactor for typed timeout error
case <-timer.C: // Timeout handling deleteResponseChan() - return nil, fmt.Errorf("timeout waiting for SSE response after %v", responseTimeout) + return nil, fmt.Errorf("%w after %v", ErrResponseTimeout, responseTimeout)This allows callers to distinguish timeout errors from other request failures using
errors.Is(err, transport.ErrResponseTimeout).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/sse.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
📚 Learning: 2025-04-06T10:07:06.685Z
Learnt from: leavez
Repo: mark3labs/mcp-go PR: 114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
Applied to files:
client/transport/sse.go
🔇 Additional comments (1)
client/transport/sse.go (1)
180-205: Test coverage for timeout scenarios has been added.Per the AI summary, new timeout-focused tests (TimeoutWhenServerNeverResponds, ContextDeadlineTakesPrecedence, TimeoutCleansUpResponseChannel, AlreadyExpiredDeadline) were added in sse_test.go, addressing the test coverage request from previous reviews.
Also applies to: 458-483
|
@ezynda3 The CI tests are failing in TestStdio/ResponseError. Do we need to fix this? |
3 participants
Description
Fixes #649
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.