Repository navigation
fix(http): only open session streams for sessions the connection knows - #483
Merged
Merged
Conversation
The HTTP server created a session SSE mailbox on demand for any `Acp-Session-Id`, from both GET and session-scoped POSTs, so a client could make it allocate streams for made-up session IDs without limit. Mailboxes for real sessions were also never removed. A session is now known on a connection once the agent names it: in a `session/new` or `session/fork` result, in a successful response to a request scoped to it (such as `session/load` or `session/resume`), or in a request or notification it sends. The session is recorded before the response naming it is queued, so a GET sent right after that response finds it. POSTs no longer allocate streams, and a response for a session with no stream goes to the connection stream. A client may still open a stream before `session/load` so it sees the replay, as the RFD's resume flow and `HttpClient` do. Up to 16 such unconfirmed streams are allowed per connection, each forgotten when the client closes it before the agent names the session. Beyond that, a GET for an unknown session gets 404. A successful `session/close` or `session/delete` removes the session's stream. Co-Authored-By: Claude <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
The HTTP server created a session SSE mailbox on demand for any
Acp-Session-Id, from a GET or from any session-scoped POST. A client holding a valid connection ID could make it allocate streams for made-up session IDs without limit, and mailboxes for real sessions were never removed. The streamable HTTP RFD says an unknownAcp-Session-Idgets404.Fix
session/new/session/forkresult, in a successful response to a request scoped to it (session/load,session/resume, ...), or in a request or notification it sends. The session is recorded before the response that names it is queued, so a GET sent right after that response finds it.HttpClientall open the session GET beforesession/load, so a strict 404 there would breakHttpClientand RFD-conformant clients. Instead, up to 16 unconfirmed session streams are allowed per connection. Each is forgotten when the client closes it before the agent names the session. Beyond that, the GET gets404 Not Found.session/close/session/deleteremoves the session's stream. A reader that is still attached gets the response, and its stream stays open until it disconnects.No public API changes.
Tests
connection.rsunit tests: the unconfirmed stream cap and release, a session created bysession/newbeing openable right after its response even with every unconfirmed slot taken, a POST for an unknown session allocating nothing and its error reaching the connection stream, a pre-load stream confirmed by its result (with replay/response order kept), a failed load leaving the session unknown, and close removing the stream.http_server.rs: a GET past the cap gets 404, and 100 POSTs with made-up session IDs allocate no streams.tests/session_streams.rs:HttpClientagainstAcpHttpServerrunningsession/new+session/prompt,session/loadwith the stream opened first, and a prompt for an unknown session getting its error.