Conversation
A client that goes away without sending DELETE left its session registered until the process exited. Each such session keeps its own go-sdk server with a copy of every registered tool, resource and prompt, so memory grew with every abandoned session. WithSessionIdleTimeout closes sessions that have had no POST for the configured duration, through go-sdk's SessionTimeout for local sessions and the same rule for rehydrated ones. The shim's session entry now follows its go-sdk session and is dropped as soon as that session ends, whatever ended it. A local session that the SessionIdManager reports terminated now has its go-sdk session closed as well, instead of only losing its entry. The timeout is opt-in, so without it an abandoned session is still kept until DELETE.
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
1 participant
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.
Summary
WithSessionIdleTimeout(time.Duration). The shim closes a session that has had no POST for that long: local sessions through go-sdk'sStreamableHTTPOptions.SessionTimeout, rehydrated sessions (which the go-sdk handler never sees) through an idle timer with the same accounting.bindSession, which starts one goroutine per go-sdk session that waits onServerSession.Waitand then drops the entry, its local marker and its notification channel.SessionTimeoutalone would close the session and leave the entry behind.sessionsMukeeps the watcher of an ended session from dropping an entry already re-bound to a newer session with the same ID.Validatereports a local session terminated, close its go-sdk session too, as the rehydrated path already does. Before, the session and its per-session server stayed open until the process exited.KNOWN GAP (issue #156, finding 5)comments.The timeout is off by default. Without it, two things change: the shim drops an entry as soon as its go-sdk session ends, at the cost of one parked goroutine per live session, and it closes a local session that
Validatereports terminated. Reaping releases only this instance's copy and does not callSessionIdManager.Terminate, because in a multi-replica deployment the session may still be in use through another replica. An embedder whose manager expires sessions after a TTL would pass that TTL.Closes #305
Why
In the issue's reproduction (5,000 resources, 50 abandoned sessions) the abandoned sessions retain 59 MiB. With
WithSessionIdleTimeout(time.Second)the same run is back to 0 registered sessions and 1.8 MiB of heap two seconds after the clients stop, and the abandoned session ID gets 404.Testing
reaping_internal_test.gocovers reaping of abandoned local and rehydrated sessions, active sessions outliving several timeouts, DELETE, the zero default, a manager-reported termination closing the go-sdk session, a superseded go-sdk session keeping the re-bound entry, and the idle timer's pause and restart.go build ./...,go vet ./...,golangci-lint run ./...(0 issues),addlicense -check, andgo test -race ./...on linux/amd64.