Skip to content

Allow stateful HTTP endpoints to recover after DELETE - #307

Open
EhsanAzish80 wants to merge 7 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/stateful-session-recovery
Open

EhsanAzish80 wants to merge 7 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/stateful-session-recovery

Conversation

@EhsanAzish80

@EhsanAzish80 EhsanAzish80 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #286 for stateful HTTP endpoints. The session manager creates a fresh server and transport for each initialize. DELETE stops the old server and lets the client initialize a new session. A rejected DELETE leaves the existing session available; the manager removes it only after DELETE succeeds.

Initialize is validated once before the server factory runs. maxSessions is opt-in, and a configured limit returns 503 when active or in-progress sessions reach it. Factory errors are logged without exposing their details in the HTTP response.

Open GET streams keep their sessions from expiring. Expired sessions stop in a background task so the next request does not wait for cleanup. Server.stop() also cancels in-flight handlers for every transport.

The Swift 6.1 CI job is pinned to Ubuntu 24.04, and existing client tests wait for initialize instead of a fixed 10 ms sleep. These CI fixes are shared with #295 and #298.

Tests: 53 focused HTTP transport tests and all 589 Swift tests passed locally on c73ab13. The new expiry test passed five additional runs. All six CI jobs passed on the same commit, including Ubuntu, both macOS jobs, conformance, documentation, and the Linux static build. AI helped write the tests; I ran them.

varun06
varun06 previously approved these changes Oct 24, 2026

@varun06 varun06 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @EhsanAzish80, the Server.stop() change is a real fix. Reverting just that part makes the slow-session test fail. I ran the new tests many times locally with no flakes, and the suite passes.

Two things before merging, since this becomes the recommended production API:

  1. Validate before creating the session. The validation pipeline (Origin, bearer auth, Accept/Content-Type) only runs inside transport.handleRequest, after serverFactory and server.start. With a BearerTokenValidator, an initialize without a token correctly gets a 401, but a Server has already been built and started (factory called once). Could the manager run validationPipeline.validate(...) on the initialize request before calling the factory?
  2. Limit the number of sessions. Each initialize creates a Server that lives up to sessionTimeout
    (1h by default). A maxSessions option that rejects new sessions with 503 would avoid unbounded growth.
    Smaller, non-blocking:
  • lastAccessedAt only updates on incoming requests, so a client holding a GET stream open without
    POSTs gets stopped mid-stream after the timeout.
  • removeExpiredSessions() awaits server.stop() inside the request path; the 60s cleanup task already
    covers it.
  • The 500 body includes the factory's localizedDescription; a generic message plus a log entry would be safer.
  • Worth a release note that Server.stop() now cancels in-flight handlers for all transports.
  • Tests for expiry, a failed DELETE, and a throwing factory would round this out.

@EhsanAzish80
EhsanAzish80 force-pushed the fix/stateful-session-recovery branch from 9eae4c8 to ec9d423 Compare October 24, 2026 18:04
@EhsanAzish80
EhsanAzish80 requested a review from a team as a code owner October 24, 2026 18:04

Copy link
Copy Markdown
Contributor Author

Updated this for the two review points: initialize now runs validation before the server factory, and maxSessions returns 503 when active or in-progress sessions reach the limit. I added tests for auth, concurrent creation, and factory errors. All 565 tests, DocC, and CI pass on ec9d423. AI helped write the new tests; I ran them.

@akhilcb akhilcb left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both blocking points from the earlier review look addressed: validation now runs before the server factory, and maxSessions returns 503 at the limit. I ran the full suite (565 passing) and the HTTP server tests 10 more times with no flakes.

@varun06

varun06 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Thanks @EhsanAzish80, both points are addressed.

One thing left: initialize is now validated twice. The manager runs validationPipeline.validate(...), then
passes the same pipeline to the transport, and transport.handleRequest validates again with an identical
context. With a BearerTokenValidator an authorized initialize calls the token validator twice (an
introspection round trip, or a double-count for rate-limit/nonce validators). If the second check fails, for
example because the token was revoked in between, the client gets a 401 after the factory and
server.start have already run. Could the manager hand the validated request to an internal transport
entry point that skips the pipeline?

Also worth deciding on the default maxSessions: 100: many clients never send DELETE, so with the 1h
sessionTimeout, 100 abandoned sessions would make every new client get 503 for up to an hour. Making it
opt-in, or evicting the session idle longest when full, might be a safer default.

Smaller, non-blocking: the manager repeats the transport's default validator list (a shared static default
would avoid drift), and expiry still ignores open GET streams.

@EhsanAzish80

Copy link
Copy Markdown
Contributor Author

Thanks Varun, I updated #307 in bf10e4d. Initialize now passes validation once before the server factory, and the transport accepts that prevalidated request without running the pipeline again. I made maxSessions opt-in as you suggested; a configured cap still returns 503 when it is reached.

The 5 focused session manager tests and all 566 Swift tests passed locally. The new session tests also passed in Ubuntu CI, and macOS, conformance, docs, and Linux static build are green. The Ubuntu test job hit 60-second limits in ClientTests.testSendErrorResponseRace and testBatchRequestEmpty, then GitHub cancelled it at 10 minutes. Could you rerun that job? I can't rerun upstream Actions from my fork.

AI helped write the new tests; I ran them.

@akhilcb

akhilcb commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Thanks @EhsanAzish80. I checked the latest changes against Varun's comments and his main points are covered: validation now runs once before the server factory, maxSessions is opt-in and still returns 503 at the limit and factory errors no longer appear in the response. CI is green on 428da76.

A few things I noticed, in case they're useful:

  • During a DELETE, the session is out of the map until the transport responds. If the DELETE fails, any request that arrived in between got a 404, and the spec makes the client start a new session.
  • That put-back path and session expiry don't have tests yet.
  • Server.stop() now cancels in-flight handlers for every transport. A line in the PR description would make that easy to spot.
  • Since there's no session cap by default, the docs could suggest setting maxSessions for exposed endpoints.
  • Still open from Varun's earlier list: expiry ignores open GET streams and expired-session cleanup still waits for server.stop() inside the request path.

Copy link
Copy Markdown
Contributor Author

Thanks, I addressed these in c73ab13. The manager now removes a session only after DELETE succeeds, and the new test checks that a rejected DELETE leaves it usable. I added an expiry test covering an open GET stream and expiry after that stream closes; expired-session stopping no longer holds up the request path. I also added the maxSessions note to the README and called out Server.stop() in the PR description. All 589 local Swift tests and all six CI jobs passed on this commit. AI helped write the tests; I ran them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants