fix(streamable_http): close active sessions before Shutdown by syf2211 · Pull Request #926 · mark3labs/mcp-go

syf2211 · 2026-07-12T06:09:19Z

Summary

Close active streamable HTTP listening sessions before calling http.Server.Shutdown, so graceful shutdown does not hang while clients remain connected.

Motivation

Fixes #922. StreamableHTTPServer.Shutdown() could block indefinitely when long-lived GET handlers were still open, because http.Server.Shutdown waits for active handlers to finish.

Changes

Add a per-session done signal to streamableHttpSession with closeDone()
Have handleGet return when the session is closed
Add CloseSessions() to terminate active listeners and clean up session state
Call CloseSessions() from Shutdown() before stopping the HTTP server
Add regression tests for shutdown with an active GET connection and for CloseSessions()

Tests

go test ./server -run 'TestStreamableHTTP_ShutdownWithActiveConnection|TestStreamableHTTP_CloseSessions' -count=1 — PASS
go test ./server -count=1 — PASS

Notes

Mirrors the existing SSE server shutdown pattern (CloseSessions + Shutdown).

Summary by CodeRabbit

New Features
- Added a new method to explicitly close all active streaming HTTP GET sessions.
Bug Fixes
- Improved server shutdown reliability by ensuring active sessions are terminated before completing shutdown.
- Ensured long-lived streaming requests stop promptly during shutdown.
Tests
- Added regression tests to prevent shutdown timeouts when active connections exist.
- Added coverage verifying session cleanup works and the server can continue to handle requests afterward.

StreamableHTTPServer.Shutdown() previously called http.Server.Shutdown while long-lived GET listeners were still blocked, so graceful shutdown could hang until clients disconnected. Mirror the SSE server behavior by tracking a per-session done signal, exposing CloseSessions(), and invoking it from Shutdown. Fixes mark3labs#922

mark-iii-labs-huly · 2026-07-12T06:09:27Z

Connected to Huly®: MCP_G-490

coderabbitai · 2026-07-12T06:09:39Z

Walkthrough

Streamable HTTP shutdown now signals active sessions, terminates their IDs, cleans transport state, and allows long-lived GET handlers to exit. Regression tests cover active-connection shutdown and explicit session cleanup.

Changes

Streamable HTTP session shutdown

Layer / File(s)	Summary
Session shutdown signaling `server/streamable_http.go`	Sessions now use an idempotent `done` channel, and GET handling exits when the session is signaled.
Active session shutdown and cleanup `server/streamable_http.go`	`Shutdown` invokes `CloseSessions`, which closes active sessions, terminates their IDs, cleans transport state, and repeats cleanup during draining.
Shutdown regression coverage `server/streamable_http_test.go`	Tests cover shutdown with active connections and verify `CloseSessions` clears sessions while the server remains reachable.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

mark3labs/mcp-go#318: Related handleGet shutdown coordination through session/request done signaling.
mark3labs/mcp-go#724: Shares the cleanupSessionState session-lifecycle path.
mark3labs/mcp-go#852: Related CloseSessions and idempotent done signaling for server shutdown.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR covers the change and tests, but it omits required template sections like Type of Change and Checklist.	Add the missing template sections: Type of Change, Checklist, and any applicable MCP Spec Compliance or Additional Information entries.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code addresses `#922` by terminating active GET sessions during shutdown so graceful shutdown can complete.
Out of Scope Changes check	✅ Passed	The added drain loop, session cleanup, and regression tests all support the shutdown-hang fix and appear in scope.
Title check	✅ Passed	The title clearly states the main change: closing active streamable HTTP sessions before Shutdown.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/streamable_http.go (1)
456-462: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Drain late sessions during shutdown

CloseSessions() runs before srv.Shutdown(ctx), so a GET accepted in that gap can register after the snapshot and never receive closeDone(). That long-lived handler will keep Shutdown waiting until ctx expires. Re-run CloseSessions() while shutdown is in progress so newly-registered sessions are drained too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/streamable_http.go` around lines 456 - 462, The Shutdown method closes
sessions only once before the underlying server shutdown, allowing
late-registered GET sessions to remain open. Update
StreamableHTTPServer.Shutdown to invoke CloseSessions again while
srv.Shutdown(ctx) is in progress, ensuring sessions accepted during the shutdown
window are drained before completion or context expiry.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/streamable_http_test.go`:
- Around line 2988-3025: Update TestStreamableHTTP_ShutdownWithActiveConnection
to start the streamable HTTP server through its Start() path, or inject a real
*http.Server, instead of relying on httptest.NewServer(httpServer) with a nil
internal server. Keep the active GET connection and shutdown assertions,
ensuring Shutdown() reaches the underlying srv.Shutdown(ctx) code path.

In `@server/streamable_http.go`:
- Around line 473-501: Update CloseSessions to accept the caller’s context and
pass it to cleanupSessionState instead of creating context.Background(),
preserving shutdown deadlines and cancellation for unregister hooks. Update the
Shutdown call site and any related callers to propagate the same ctx; optionally
log failures returned by mgr.Terminate while retaining termination and cleanup
behavior.

---

Outside diff comments:
In `@server/streamable_http.go`:
- Around line 456-462: The Shutdown method closes sessions only once before the
underlying server shutdown, allowing late-registered GET sessions to remain
open. Update StreamableHTTPServer.Shutdown to invoke CloseSessions again while
srv.Shutdown(ctx) is in progress, ensuring sessions accepted during the shutdown
window are drained before completion or context expiry.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 57f1c5cc-e520-4954-a117-8034143ae327

📥 Commits

Reviewing files that changed from the base of the PR and between f6f3485 and 4ee7101.

📒 Files selected for processing (2)

server/streamable_http.go
server/streamable_http_test.go

ezynda3 · 2026-07-13T11:00:11Z

@syf2211 can you please address the coderabbit comments?

…utdown test - Propagate caller context through CloseSessions into cleanupSessionState - Drain late-registered sessions while srv.Shutdown is in progress - Log Terminate failures during session close - Exercise Shutdown via injected *http.Server in active-connection test

syf2211 · 2026-07-13T18:05:23Z

Addressed the CodeRabbit feedback in 660e25f:

CloseSessions now accepts the caller's context.Context and passes it through to cleanupSessionState (no more context.Background() during shutdown)
Shutdown drains late-registered sessions while srv.Shutdown is in progress
Terminate failures are logged at warn level
TestStreamableHTTP_ShutdownWithActiveConnection now injects a real *http.Server so Shutdown exercises the srv.Shutdown(ctx) code path

go test ./server/... — all pass.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

server/streamable_http_test.go (1)
3085-3089: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use context-aware request for consistency.

http.Get at line 3085 doesn't use a context, while the earlier request at line 3062 uses http.NewRequestWithContext(t.Context(), ...). Prefer the context-aware variant for consistency and to ensure the request is cancelled if the test times out.
♻️ Proposed refactor
-	resp2, err := http.Get(ts.URL)
+	req2, err := http.NewRequestWithContext(t.Context(), http.MethodGet, ts.URL, nil)
 	require.NoError(t, err)
+	resp2, err := http.DefaultClient.Do(req2)
+	require.NoError(t, err)
 	defer resp2.Body.Close()
 	require.Equal(t, http.StatusOK, resp2.StatusCode)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/streamable_http_test.go` around lines 3085 - 3089, Replace the
contextless http.Get call in the test with an http.NewRequestWithContext request
using t.Context(), then execute it through the appropriate HTTP client while
preserving the existing response status assertion and body cleanup.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/streamable_http_test.go`:
- Around line 3008-3019: Update the shutdown context creation in the t.Cleanup
closure to use t.Context() instead of context.Background(), and verify this
remains valid during cleanup under the project’s Go version. If t.Context() is
already cancelled when cleanup executes and prevents httpServer.Shutdown from
waiting, retain context.Background() and add the minimal usetesting suppression
for that line.

---

Nitpick comments:
In `@server/streamable_http_test.go`:
- Around line 3085-3089: Replace the contextless http.Get call in the test with
an http.NewRequestWithContext request using t.Context(), then execute it through
the appropriate HTTP client while preserving the existing response status
assertion and body cleanup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0ccb4812-5f2e-48b8-9489-c5d29e0b3ac2

📥 Commits

Reviewing files that changed from the base of the PR and between 4ee7101 and 660e25f.

📒 Files selected for processing (2)

server/streamable_http.go
server/streamable_http_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

server/streamable_http.go

syf2211 · 2026-07-14T06:06:25Z

Addressed the remaining CodeRabbit nit in 3fa1e0e: CloseSessions test now uses http.NewRequestWithContext(t.Context(), ...) instead of http.Get. All review feedback from 660e25f and 2bb80b9 is covered; go test ./server/... passes locally.

syf2211 · 2026-07-14T18:04:12Z

Addressed the CodeRabbit usetesting feedback — the shutdown test cleanup now uses t.Context() instead of context.Background(). All CI checks are green on the latest push.

coderabbitai Bot reviewed Jul 12, 2026

View reviewed changes

Comment thread server/streamable_http_test.go

Comment thread server/streamable_http.go

test(streamable_http): use t.Context() in shutdown test cleanup

2bb80b9

coderabbitai Bot reviewed Jul 13, 2026

View reviewed changes

Comment thread server/streamable_http_test.go

test(streamable_http): use context-aware request in CloseSessions test

3fa1e0e

ezynda3 merged commit 87e3e86 into mark3labs:main Jul 22, 2026
6 checks passed

github-actions Bot mentioned this pull request Jul 24, 2026

Update module github.com/mark3labs/mcp-go to v0.57.0 claytono/go-unifi-mcp#169

Merged

1 task

sd2k mentioned this pull request Aug 21, 2026

chore(deps): bump mcp-go from v0.55.0 to v0.58.0 grafana/mcp-grafana#1097

Merged

Navigation Menu

Repository navigation

fix(streamable_http): close active sessions before Shutdown - #926

fix(streamable_http): close active sessions before Shutdown#926
ezynda3 merged 4 commits into
mark3labs:mainfrom
syf2211:fix/streamable-http-shutdown-close-sessions

syf2211 commented Jul 12, 2026 •

edited by mark-iii-labs-huly Bot

Loading

Uh oh!

mark-iii-labs-huly Bot commented Jul 12, 2026

Uh oh!

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

ezynda3 commented Jul 13, 2026

Uh oh!

syf2211 commented Jul 13, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

syf2211 commented Jul 14, 2026

Uh oh!

syf2211 commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

syf2211 commented Jul 12, 2026 • edited by mark-iii-labs-huly Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Changes

Tests

Notes

Summary by CodeRabbit

Uh oh!

mark-iii-labs-huly Bot commented Jul 12, 2026

Uh oh!

coderabbitai Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ezynda3 commented Jul 13, 2026

Uh oh!

syf2211 commented Jul 13, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

syf2211 commented Jul 14, 2026

Uh oh!

syf2211 commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

syf2211 commented Jul 12, 2026 •

edited by mark-iii-labs-huly Bot

Loading

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading