Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Noting for the record: the automated Codex review has not produced a result on this PR. Three attempts ( That leaves the review question I actually care about, which nobody has answered yet: is the ordering guarantee this PR implements the one you want? The behaviour under test: when a call's response and a preceding notification are both ready, the notification must be dispatched before the call's response is retired, so a client that reacts to the notification has already observed it by the time the call result lands. "Retire" is one-shot — the queue drops a retired response so a late duplicate cannot re-deliver it — which is why ordering has to be settled before retirement rather than after. Two points I would specifically like a second opinion on, since both are judgement calls rather than mechanical fixes:
The diff is +489/−19 across 4 files, all inside |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
f9aac4e to
a61c4a1
Compare
|
Rebased onto current
Separately: |
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
The ordering fix is mergeable with an MCPGODEBUG opt-out for the old response timing. Without one, a client handler that waits on its own CallTool caller deadlocks after upgrade with no error, and the description calls this a "pure internal ordering change".
@Xio-Shark, the PR completes what the Concurrency section of docs/protocol.md already promises for notifications (#26). The cost is compatibility. Since 1afb0eb the read loop retired responses on its own goroutine, and #323 made notification handlers run in order, so a handler that blocked on the caller used to be harmless. Here, with ProgressNotificationHandler sending on an unbuffered channel that the caller reads after CallTool returns, CallTool never returns. The make(chan string) to make(chan string, 4) hunk in mcp/streamable_test.go patches that pattern in a test.
$ go test -count=1 -timeout 15s ./mcp/ -run TestProbeUnbufferedHandler -v # head a61c4a1
=== RUN TestProbeUnbufferedHandler
probe_test.go:40: CallTool did not return in 3s
panic: test timed out after 15s
FAIL github.com/modelcontextprotocol/go-sdk/mcp 15.061s
$ go test -count=1 ./mcp/ -run TestProbeUnbufferedHandler -v # merge-base 6544ab3
--- PASS: TestProbeUnbufferedHandler (0.00s)
# not run: Windows, macOS
probe_test.go (package mcp)
func TestProbeUnbufferedHandler(t *testing.T) {
ctx := context.Background()
s := NewServer(testImpl, nil)
AddTool(s, &Tool{Name: "work"}, func(ctx context.Context, req *CallToolRequest, _ struct{}) (*CallToolResult, any, error) {
req.Session.NotifyProgress(ctx, &ProgressNotificationParams{ProgressToken: req.Params.GetProgressToken(), Progress: 1, Total: 1})
return &CallToolResult{}, nil, nil
})
st, ct := NewInMemoryTransports()
ss, err := s.Connect(ctx, st, nil)
if err != nil {
t.Fatal(err)
}
defer ss.Close()
ch := make(chan float64)
cs, err := NewClient(testImpl, &ClientOptions{
ProgressNotificationHandler: func(_ context.Context, r *ProgressNotificationClientRequest) { ch <- r.Params.Progress },
}).Connect(ctx, ct, nil)
if err != nil {
t.Fatal(err)
}
defer cs.Close()
p := &CallToolParams{Name: "work"}
p.SetProgressToken("x")
done := make(chan struct{})
go func() { cs.CallTool(ctx, p); close(done) }()
select {
case <-done:
t.Log("CallTool returned; draining", <-ch)
case <-time.After(3 * time.Second):
t.Fatal("CallTool did not return in 3s")
}
}With conn.go reverted to 6544ab3 and the PR's tests kept, TestResponseWaitsForPrecedingNotifications, TestRetireQueuedResponse and TestProgressBeforeResult fail, so they do guard the change.
A note in the Concurrency list (internal/docs/protocol.src.md) saying handlers must not wait on the caller of an in-flight call would document the rule, but on its own it leaves existing programs hanging.
@nahapetyan-serob, since #1337 is with you: does this ship with an opt-out like blockingcancelnotify, or do responses stay immediate behind a barrier?
…l responses In readIncoming, incoming *Request messages (including notifications) are enqueued onto handlerQueue and processed sequentially by handleAsync, whereas incoming *Response messages were retired immediately on the reader goroutine. As a result, a call (such as CallTool) could return its result before the client had finished running handlers for notifications (such as notifications/progress or notifications/message) that preceded the response on the wire. Deliver incoming *Response messages through handlerQueue when handleAsync is active so that all synchronous notification handlers queued ahead of a response complete before the awaiting call returns. When a synchronous handler itself issues a nested Call on the same connection, retire its response immediately so the handler does not block waiting on its own handlerQueue slot. Fixes modelcontextprotocol#1337 Signed-off-by: xioshark <151488904+Xio-Shark@users.noreply.github.com>
…ctx nested calls When a response is queued in handlerQueue behind an active handler, readIncoming has already removed its ID from outgoingCalls. Update Connection.Retire to also remove and retire a matching entry in handlerQueue so caller-side retirement (such as context cancellation in mcp/transport.go) still takes effect immediately and cannot race with handleAsync draining the queue. Also compare the monotonic queued-request counter (s.queuedRequests == ac.queuedAtStart) instead of requiring len(s.handlerQueue) == 0 when a synchronous handler issues a nested Call without threading the handler context, avoiding a deadlock when earlier notifications are already waiting in handlerQueue. Signed-off-by: xioshark <151488904+Xio-Shark@users.noreply.github.com>
The preceding-notification ordering guarantee means a response is delivered after the notifications ahead of it on the wire have been handled. A notification handler that waits on the return of the call it notifies about — for example a progress handler sending on an unbuffered channel that the caller drains after Call returns — therefore no longer completes after upgrade, and neither does the call it waits on. Restore the previous timing behind MCPGODEBUG=immediateresponseretire=1, following the existing compatibility-parameter pattern. Document the rule in the Concurrency section of the protocol docs and the parameter in the MCPGODEBUG history. Signed-off-by: xioshark <151488904+Xio-Shark@users.noreply.github.com>
a61c4a1 to
74c4e6f
Compare
|
@chrikrah You're right, and the framing of the PR description was wrong to call this a pure internal change. Both halves of your ask are in
var immediateresponseretire = mcpgodebug.Value("immediateresponseretire")
...
if immediateresponseretire == "1" || !s.handlerRunning || (active != nil && (...)) {Docs. Your reproduction, unmodified, on this branch. I used it as a throwaway file (not committed — a test that hangs by default is not something to ship): $ MCPGODEBUG=immediateresponseretire=1 go test -count=1 -run TestProbeUnbufferedHandler ./mcp
probe_test.go:36: CallTool returned; draining 1
ok github.com/modelcontextprotocol/go-sdk/mcp 0.420s
$ go test -count=1 -run TestProbeUnbufferedHandler ./mcp # default: new ordering
--- FAIL: TestProbeUnbufferedHandler (20.00s) # CallTool never returns, as you reportedCommitted coverage for the same shape lives at the transport layer: On Regarding Rebased onto |
In
readIncoming, incoming*Requestmessages (including notifications) are enqueued ontohandlerQueueand processed sequentially byhandleAsync, whereas incoming*Responsemessages were retired immediately on the reader goroutine. As a result, a call (such asCallTool) could return its result before the client had finished running handlers for notifications (such asnotifications/progressornotifications/message) that preceded the response on the wire.Deliver incoming
*Responsemessages throughhandlerQueuewhenhandleAsyncis active so that all synchronous notification handlers queued ahead of a response complete before the awaiting call returns. When a synchronous handler itself issues a nestedCallon the same connection, retire its response immediately so the handler does not block waiting on its ownhandlerQueueslot.Summary
internal/jsonrpc2/conn.go:inFlightState.handlerQueueelements toqueueItem(holding either an*incomingRequestor a pending(*AsyncCall, *Response)retirement).*Responseretirement behind pending items inhandlerQueuewhenhandleAsyncis running, while preserving immediate retirement when idle or when the response belongs to a nestedCallmade by the currently active synchronous handler (activeSyncReleaser).immediateresponseretirecompatibility parameter (see below), following the existingMCPGODEBUGpattern used byblockingcancelnotifyandfatalnewprotocolerrors.internal/docs/mcpgodebug.src.md,docs/mcpgodebug.md: documentimmediateresponseretire.internal/docs/protocol.src.md,docs/protocol.md: state the ordering rule in the Concurrency section, including the handler must not wait on the return of the call it notifies about.internal/jsonrpc2/conn_test.go:TestResponseWaitsForPrecedingNotificationsandTestSynchronousHandlerNestedCall.mcp/client_test.go:TestProgressBeforeResultcoveringin_memory,streamable_http,proxyprogress forwarding, andnotification_handler_nested_call.mcp/streamable_test.go:notificationschannel intestClientReplay(matchingTestStreamableServerDisconnect).testClientReplaykeeps its handler runnable while the caller is blocked inCallTool, which is what a client with a blocking handler now has to avoid; the buffered channel expresses that. Clients that cannot move work out of the handler can keep the old timing withMCPGODEBUG=immediateresponseretire=1.Evidence
main)CallToolreturned before all 3 progress notifications handledCallToolreturned before all 3 progress notifications handledBehavior change and the compatibility parameter
The ordering guarantee has a corollary: a notification handler must not wait for the call it is notifying about to return. Such a handler now blocks that call instead of being passed by it, and the failure mode is a deadlock with no error. Reported by @chrikrah in
the review thread, with a minimal reproduction: a
ProgressNotificationHandlersending on an unbuffered channel that the caller drains afterCallToolreturns.Two things follow, both in this PR:
docs/protocol.md, so it is visible to anyone writing a handler.MCPGODEBUG=immediateresponseretire=1restores the old response timing for code that cannot move the work out of the handler.Verified with @chrikrah's reproduction, unmodified, as a throwaway test file against this branch:
TestImmediateResponseRetireGODEBUGininternal/jsonrpc2/conn_test.gocovers the same shape at the transport layer: a notification handler that waits on the return of the call it notifies about. It fails in 5 s with the parameter set to0and passes with it set to1, so the parameter is pinned in both directions.Merge Danger
internal/jsonrpc2/conn.gowith no wire format changes. It is observable by applications (see Behavior change above), so it ships withMCPGODEBUG=immediateresponseretire=1as the escape hatch and is cleanly revertible.jsonrpc2.Connectionwhen notifications or synchronous handlers are in flight on the same connection. Re-entrant calls from synchronous notification handlers (e.g.,ToolListChangedHandlercallingListTools) are explicitly guarded and covered by tests.Fixes #1337