mcp: bound streamable Close DELETE so a silent peer cannot hang teardown (#1184)
Close issued a synchronous session-termination DELETE on the connection context and cancelled that context only after `Do` returned. A reachable but silent peer could stall `Close()` until the TCP timeout. Give the DELETE its own short timeout. RED on current main: `TestCloseHangsOnBlackHoledDelete` failed with `Close() blocked on a black-holing DELETE`. GREEN: `go test ./mcp/ -count=1 -timeout 120s -run 'TestStreamableClient|TestCloseHangs|TestCallCancellation_FastReturn'` Fixes #1183 Made with [Cursor](https://cursor.com) Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This commit is contained in:
parent
b074c7f3af
commit
3d6450fccf
2 changed files with 50 additions and 1 deletions
|
|
@ -2016,6 +2016,11 @@ const (
|
|||
reconnectGrowFactor = 1.5
|
||||
// reconnectMaxDelay caps the backoff delay, preventing it from growing indefinitely.
|
||||
reconnectMaxDelay = 30 * time.Second
|
||||
// closeDeleteTimeout bounds the session-termination DELETE sent by Close.
|
||||
// That request is best-effort: a peer that accepts the connection but never
|
||||
// answers must not be able to hold teardown open until the TCP timeout.
|
||||
// See issue #1183.
|
||||
closeDeleteTimeout = 5 * time.Second
|
||||
)
|
||||
|
||||
var (
|
||||
|
|
@ -2767,7 +2772,9 @@ func (c *streamableClientConn) Close() error {
|
|||
// No session was established (e.g. the server is stateless),
|
||||
// so there is nothing to delete.
|
||||
} else {
|
||||
req, err := http.NewRequestWithContext(c.ctx, http.MethodDelete, c.url, nil)
|
||||
reqCtx, stop := context.WithTimeout(c.ctx, closeDeleteTimeout)
|
||||
defer stop()
|
||||
req, err := http.NewRequestWithContext(reqCtx, http.MethodDelete, c.url, nil)
|
||||
if err != nil {
|
||||
c.closeErr = err
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -281,6 +281,48 @@ func TestStreamableClientRedundantDelete(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestStreamableClientCloseUnresponsiveDelete checks that Close stays bounded
|
||||
// when the peer accepts the session-termination DELETE but never answers it.
|
||||
func TestStreamableClientCloseUnresponsiveDelete(t *testing.T) {
|
||||
base := NewStreamableHTTPHandler(func(*http.Request) *Server {
|
||||
return NewServer(testImpl, nil)
|
||||
}, nil)
|
||||
httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method == http.MethodDelete {
|
||||
<-r.Context().Done()
|
||||
return
|
||||
}
|
||||
base.ServeHTTP(w, r)
|
||||
}))
|
||||
t.Cleanup(func() {
|
||||
// The DELETE handler only returns once its request is cancelled, so
|
||||
// drop the connections before waiting for outstanding requests.
|
||||
httpServer.CloseClientConnections()
|
||||
httpServer.Close()
|
||||
})
|
||||
|
||||
client := NewClient(testImpl, nil)
|
||||
session, err := client.Connect(context.Background(),
|
||||
&StreamableClientTransport{Endpoint: httpServer.URL}, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("client.Connect() failed: %v", err)
|
||||
}
|
||||
if session.ID() == "" {
|
||||
t.Fatal("empty session ID: Close would not send DELETE")
|
||||
}
|
||||
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
session.Close()
|
||||
close(done)
|
||||
}()
|
||||
select {
|
||||
case <-done:
|
||||
case <-time.After(closeDeleteTimeout + 5*time.Second):
|
||||
t.Fatal("Close() blocked on an unanswered DELETE")
|
||||
}
|
||||
}
|
||||
|
||||
func TestStreamableClientGETHandling(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue