streamable client: Connect blocks far past its context deadline against an unresponsive server (detached-context cleanup)

[ibetitsmike]

What

Client.Connect with a context deadline can block far past that deadline against a server that accepts TCP connections but never responds (black-holed peer). Observed: a 2s deadline returns after ~12s with the default HTTP client in a local test; in production, with connections crossing a NAT that silently dropped packets, a 10s deadline stretched to ~125s (kernel TCP timeout).

The caller's deadline bounds the initialize request itself, but not the error-path cleanup that follows. StreamableClientTransport.Connect detaches the incoming context for the connection's lifecycle (connCtx, cancel := context.WithCancel(xcontext.Detach(ctx)), mcp/streamable.go ~L2045 in v1.7.0). That detachment is intentional and documented for keeping the standalone SSE stream alive past a short connect deadline. However, when initialize fails at the deadline, the cleanup work that runs before Connect returns (the notifications/cancelled write for the abandoned call and session.Close()) issues HTTP requests bound to the detached connection context, not to the caller's expired one. Against a black-holed server those requests block until the HTTP transport or the kernel gives up, so Connect overshoots its deadline by minutes with a default http.Client (no ResponseHeaderTimeout).

Related: #1183 covers the same detached-context ordering problem for Close() specifically. This report is about Connect not honoring its own deadline on the error path.

Repro

func TestConnectOvershootsDeadline(t *testing.T) {
	// Accepts TCP connections, never responds to any request.
	ln, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		t.Fatal(err)
	}
	defer ln.Close()
	go func() {
		for {
			conn, err := ln.Accept()
			if err != nil {
				return
			}
			defer conn.Close()
		}
	}()

	client := mcp.NewClient(&mcp.Implementation{Name: "c", Version: "0"}, nil)
	tr := &mcp.StreamableClientTransport{Endpoint: "http://" + ln.Addr().String()}
	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
	defer cancel()

	start := time.Now()
	_, err = client.Connect(ctx, tr, nil)
	elapsed := time.Since(start)
	t.Logf("Connect returned after %s, err=%v", elapsed, err)
	if elapsed > 4*time.Second {
		t.Fatalf("Connect blocked %s past a 2s deadline", elapsed)
	}
}

With go-sdk v1.7.0 and Go 1.24 this fails: Connect returned after 12.03s, err=context deadline exceeded; sending "notifications/cancelled": rejected by transport: Post "http://127.0.0.1:...": context deadline exceeded.

Expected

Connect(ctx, ...) returns within (approximately) ctx's deadline regardless of server behavior. Error-path cleanup for a session that never initialized could be best-effort/asynchronous, or bounded by the caller's context rather than the detached connection context.

Workaround

We now run Connect+ListTools in a goroutine and select on the deadline, abandoning the goroutine on timeout, plus a cloned http.Transport with ResponseHeaderTimeout and a short dial timeout (coder/coder#28400).

🤖 Filed by Mux (AI agent) on behalf of @ibetitsmike.

[guglielmo-san]

@ibetitsmike thank you for the report, I think this issue has been recently addressed.
PR #1151 (mcp: allow asynchronous cancel notification, already on main) reworked that: conn.Retire runs synchronously and the cancellation notify is spawned in a goroutine. Because Retire removes the call from outgoingCalls before cs.Close() runs, Close()'s updateInFlight typically sets connClosing = true while the state is already idle, closes the underlying conn, and returns.
Could you re-run the repro against main (or a build that includes #1151) and check the elapsed time?

[Haaaarry]

Confirmation data on whether this is fixed on main, as requested: not fully.

Ran the reporter's black-hole repro test on current main (2732839, includes #1151 and #1190), Go 1.25.5, -count=3: Connect with a 2s deadline returns after 7.00s, 7.00s, 7.00s — very stable, still overshooting the deadline ~3.5x (v1.7.0 overshot to ~12s).

Two experiments to localize what's left:

1. MCPGODEBUG=blockingcancelnotify=1 restores exactly the v1.7.0 behavior (12.00s = deadline + 2×5s). This confirms mcp: allow asynchronous cancel notification #1151 moved one 5s block (the call-path notifications/cancelled in call()) off the return path, but a second ~5s synchronous step remains in Connect's error path.
2. Ruled out the session-termination DELETE: patched closeDeleteTimeout down to 1s locally and the total stayed 7.00s — consistent with Close() skipping the DELETE when no session ID was ever established.

So the remaining overshoot is a ~5s synchronous step in the handshake error path (after the caller's ctx expires, before Connect returns), not the Close() cleanup. Happy to keep digging toward a specific line if that would help.

[omkar619-dev]

Ran the repro against main — go-sdk v1.7.1-0.20260828140610-21c18c6229e1, Go 1.25.

default: Connect returned after 7.0007s (2s deadline) MCPGODEBUG=blockingcancelnotify=1: Connect returned after 12.0016s

So it's improved but not fixed i believe . The 5.000s delta between those two runs is exactly notifyCancellationTimeout, which confirms #1151 is doing its job — the cancel-notify no longer blocks the return. The blocking run also surfaces sending "notifications/cancelled": rejected by transport: Post ... which is absent from the default run. That leaves the remaining 5s as closeDeleteTimeout, so the breakdown on
main is 2s deadline + 5s session-teardown DELETE.

Btw I think the thread was missing half the picture. #1189 describes two blocking cleanup steps and #1151 only covers one: it made the notifications/cancelled write asynchronous in transport.go (conn.Retire, then a goroutine with context.WithoutCancel +
notifyCancellationTimeout). The session.Close() DELETE stayed on the connection context until #1184 gave it closeDeleteTimeout = 5s, and that only merged on the 21st. Both halves are on main now but arrived through separate PRs under separate issues. The original report
was against v1.7.0, released 28 Jul, which predates #1151 by ten days — and its 12.03s matches today's blocking-notify figure, which fits.

The bound itself looks right. What's left is that Connect's error path still waits on session teardown before returning, rather than that teardown being bounded by the caller's context or moved off the return path. So Connect still can't honour its own deadline, it just overshoots by 5s now instead of by minutes.

Also worth flagging: nothing on main asserts the end-to-end invariant. #1184 added TestStreamableClientCloseUnresponsiveDelete for the Close path and #1151 added TestCallCancellation_FastReturn for the call path, but there's no test that Connect returns within its own deadline against an unresponsive peer. Since the detach at streamable.go ~L2045 is unchanged, any cleanup step added to that error path later would reintroduce this silently.

Happy to send a PR — the repro above as a regression test, plus whichever disposition you prefer for the teardown wait.

[arpankernel]

Dug into the remaining overshoot on current main. Still returns ~7s for a 2s deadline against a TCP black hole (accept, never read or respond), which matches the 5s = notifyCancellationTimeout delta noted above.

The remaining 5s isn't on the call path. #1151 made that async and the call returns at the deadline. It's the teardown path:

• On the deadline, call (transport.go) retires the call and fires the best-effort notifications/cancelled in a goroutine bounded by notifyCancellationTimeout (5s), with WithoutCancel(ctx) so it survives the caller's deadline.
• Client.Connect then hits err != nil and calls cs.Close() -> jsonrpc2.Connection.Close() -> wait(), which blocks until in-flight writes drain.
• That async notify is still an in-flight write (outgoingNotifications), stuck in client.Do against the black hole for the full 5s. So Close(), and therefore Connect, waits it out.

Goroutine dump at ~4s confirms it: one goroutine parked in Connection.Close -> wait -> <-c.done, waiting on the notify goroutine blocked in client.Do (the async notify in call).

So the notify's 5s timeout is decoupled from both the caller's deadline and connection teardown. Surviving the deadline is intentional, but teardown shouldn't have to wait for it, since closing the connection signals the server just as well.

Proposed direction: let teardown abort the in-flight best-effort notify instead of waiting up to notifyCancellationTimeout. For example, a "closing" signal on the jsonrpc2 Connection that the async notify selects on to cancel its context. That keeps the current semantics (notify still survives the caller's deadline, still gets its full timeout while the connection is up) but stops a stalled peer from delaying Close.

Happy to put up a PR along those lines if that's the direction you'd want, or bound it differently if you'd prefer.

[Haaaarry]

Independent empirical confirmation of @arpankernel's mechanism: I patched notifyCancellationTimeout from 5s to 1s locally (same black-hole repro, main @ 2732839) and Connect's return dropped from 7.00s to 3.00s (2s deadline + 1s). So the entire remaining overshoot is bounded by that one constant — consistent with teardown (Connection.Close → wait()) draining the in-flight best-effort notifications/cancelled write that the async path from #1151 fires off. Nothing else in the error path contributes measurable delay.

The proposed direction (teardown aborts the in-flight best-effort notify rather than waiting out its timeout) sounds right to me: the notify surviving the caller's deadline is intentional, but once the connection is closing, the close itself signals the peer, so waiting up to 5s for a best-effort message to an unresponsive one buys nothing.

[arpankernel]

Thanks @Haaaarry, the 5s to 1s swap dropping Connect to 3s nails it: the whole overshoot is that one timeout getting drained by teardown, nothing else in the error path.

With the mechanism independently confirmed and agreement on the direction, I'll get a PR up: a "closing" signal on the jsonrpc2 Connection that the async cancel-notify selects on, so Close aborts the in-flight best-effort write instead of waiting out notifyCancellationTimeout. The notify still survives the caller's deadline, it just can't stall teardown against a dead peer.

@guglielmo-san if that direction works for you I'll open it, or happy to bound it differently if you'd prefer.

[omkar619-dev]

Correcting my own comment above btw — I attributed the remaining 5s to closeDeleteTimeout and that was wrong tbh. @Haaaarry had already ruled the DELETE out by patching closeDeleteTimeout to 1s and seeing 7.00s unchanged, and I should have read that before inferring the constant from the 5s delta. @arpankernel's goroutine dump plus the 5s→1s notifyCancellationTimeout experiment settle it: the wait is Connection.Close -> wait() draining the in-flight best-effort notify, not session teardown.

The proposed direction reads right to me too, and it's clearly @arpankernel's to write given they localised it.

One thing from my comment though ,I think still stands and isn't covered by that fix: nothing on main asserts the end-to-end invariant, that Connect returns within its own deadline against an unresponsive peer. #1184 covers the Close path and #1151 the call path, but the detach at streamable.go ~L2045 is unchanged, so any cleanup step added to that error path later reintroduces this silently with nothing to catch it.

I'm happy to send that regression test if it's useful — the black-hole repro from the issue with a bound on elapsed. It fails on main today and would pass once the teardown change lands, so it may be easier to review inside @arpankernel's PR than as a separate one. Whichever the
maintainers prefer.

[arpankernel]

Thanks @omkar619-dev, and good call on the end-to-end invariant. That's the right thing to lock down: #1184 bounded the Close DELETE and #1151 the call path, but nothing today asserts that Connect as a whole stays within its deadline against a dead peer, so a future cleanup step on that detached error path could bring this back with nothing to catch it.

I'll fold that into the PR: the teardown fix plus a black-hole regression test that bounds Connect's elapsed time against its own deadline (RED on main today, GREEN once the notify abort lands). Keeping the test next to the fix makes the invariant easy to review together. Happy to take any refinements you have on it once it's up.

[arpankernel]

@guglielmo-san light ping on direction. I've got the fix ready locally along the lines above (a Closing signal on the jsonrpc2 Connection that the async cancel-notify aborts on) plus the black-hole regression test. Repro goes 7s -> 2s on a 2s deadline, full suite + race green. Happy to open the PR whenever the approach looks good to you, or bound it differently if you'd prefer.

[omkar619-dev]

I agree on keeping it next to the fix — a RED/GREEN pair in one PR reviews far better than a test landing separately that has to describe the bug it guards.

Three refinements on the repro as written, since I wrote it as a bug report rather than something meant to merge:

The defer conn.Close() is inside the accept loop, so it doesn't run until the goroutine exits and connections pile up for the test's duration. It is harmless at this scale but still worth checking — either drop the defer and let the listener close cover it, or accept into a per-connection goroutine.

The 2s deadline costs 2s of suite time for no signal. The failure mode is a ~5s overshoot, so a much shorter deadline catches it just as well — something like 200ms keeps the signal and makes the test nearly free.

Most importantly I'd assert elapsed stays close to the deadline rather than merely under deadline + notifyCancellationTimeout. With a fixed bound like 4s, if someone later lowers notifyCancellationTimeout to 1s the overshoot becomes 3s, the bound passes, and the invariant
is silently unguarded again — the same shape of silent regression the test exists to prevent. Something like elapsed < deadline + 500ms tests "Connect honours its deadline" rather than "Connect isn't catastrophically slow".

Otherwise it's yours — happy to review once it's up.

[arpankernel]

Opened #1241 with the fix and the black-hole regression test.