Skip to content

Commit 31eeee2

Browse files
committed
Fix resource leak in session storage cleanup
Implements fix by calling Close() on sessions that implement io.Closer before deleting them from storage during cleanup. This prevents connection leaks when vMCP sessions with backend clients expire. Closes: #3871
1 parent f0f55c1 commit 31eeee2

File tree

2 files changed

+366
-6
lines changed

2 files changed

+366
-6
lines changed

pkg/transport/session/storage_local.go

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ package session
66
import (
77
"context"
88
"fmt"
9+
"io"
10+
"log/slog"
911
"sync"
1012
"time"
1113
)
@@ -66,9 +68,12 @@ func (s *LocalStorage) Delete(_ context.Context, id string) error {
6668

6769
// DeleteExpired removes all sessions that haven't been updated since the given time.
6870
func (s *LocalStorage) DeleteExpired(ctx context.Context, before time.Time) error {
69-
var toDelete []string
71+
var toDelete []struct {
72+
id string
73+
session Session
74+
}
7075

71-
// First pass: collect IDs of expired sessions
76+
// First pass: collect expired sessions
7277
s.sessions.Range(func(key, val any) bool {
7378
// Check for context cancellation
7479
select {
@@ -80,16 +85,47 @@ func (s *LocalStorage) DeleteExpired(ctx context.Context, before time.Time) erro
8085
if session, ok := val.(Session); ok {
8186
if session.UpdatedAt().Before(before) {
8287
if id, ok := key.(string); ok {
83-
toDelete = append(toDelete, id)
88+
toDelete = append(toDelete, struct {
89+
id string
90+
session Session
91+
}{id, session})
8492
}
8593
}
8694
}
8795
return true
8896
})
8997

90-
// Second pass: delete expired sessions
91-
for _, id := range toDelete {
92-
s.sessions.Delete(id)
98+
// Second pass: close and delete expired sessions
99+
for _, item := range toDelete {
100+
// Check for context cancellation before processing each session
101+
select {
102+
case <-ctx.Done():
103+
return ctx.Err()
104+
default:
105+
}
106+
107+
// Re-check expiration and use CompareAndDelete to handle race conditions:
108+
// - Session may have been touched via Manager.Get().Touch() and is no longer expired
109+
// - Session may have been replaced via Store/UpsertSession with a new object
110+
// Only proceed if the stored value is still the same session object and still expired
111+
if item.session.UpdatedAt().Before(before) {
112+
// CompareAndDelete ensures we only delete if the value hasn't been replaced
113+
if deleted := s.sessions.CompareAndDelete(item.id, item.session); deleted {
114+
// Successfully deleted - now close in background if implements io.Closer
115+
if closer, ok := item.session.(io.Closer); ok {
116+
// Run Close() in a goroutine to avoid blocking the cleanup loop
117+
go func(id string, c io.Closer) {
118+
if err := c.Close(); err != nil {
119+
slog.Warn("failed to close session during cleanup",
120+
"session_id", id,
121+
"error", err)
122+
}
123+
}(item.id, closer)
124+
}
125+
}
126+
// If CompareAndDelete returned false, the session was already replaced/deleted - skip it
127+
}
128+
// If re-check shows session is no longer expired (was touched), skip it
93129
}
94130

95131
return nil

0 commit comments

Comments
 (0)