From 6414d46074e7270abb290a73d23f14e6e995d4db Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 18:15:27 +0000 Subject: [PATCH 1/5] Initial plan From be2759a55d4bed57bd5b8d816a69adb615890693 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 18:23:29 +0000 Subject: [PATCH 2/5] fix: paginate ValidatorClient list methods, add logger, implement LRU cache eviction - ValidatorClient.ListTools and ListResources now paginate through all cursor pages instead of returning only the first page, fixing silent data loss when testing against backends with many tools/resources - Add logger to ValidatorClient (sdk.ClientOptions{Logger: ...}) for test visibility, matching production client behavior - filteredServerCache.getOrCreate now evicts the least-recently-used entry when at max capacity, bounding memory growth in high-traffic deployments instead of logging a warning and growing without bound - Update TestFilteredServerCache_MaxSize to verify LRU eviction behavior Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/a7704c86-f1f2-4129-888b-e7dded16dfd4 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/server/routed.go | 20 +++++++++--- internal/server/routed_test.go | 23 +++++++++---- internal/testutil/mcptest/validator.go | 45 +++++++++++++++++++------- 3 files changed, 65 insertions(+), 23 deletions(-) diff --git a/internal/server/routed.go b/internal/server/routed.go index fd24a144..5bb7b3ee 100644 --- a/internal/server/routed.go +++ b/internal/server/routed.go @@ -84,12 +84,22 @@ func (c *filteredServerCache) getOrCreate(backendID, sessionID string, creator f return entry.server } - // Safety bound: if at capacity after TTL eviction, log a warning but do not - // evict non-expired entries. Routed mode relies on reusing the same filtered - // server instance for a given (backend, session), and evicting an active entry - // would recreate that server mid-session, breaking StreamableHTTP semantics. + // When at capacity after TTL eviction, evict the least-recently-used entry + // to bound memory growth reliably. This may interrupt an active session for + // the evicted (backend, session) pair, but is preferable to unbounded growth. if len(c.servers) >= c.maxSize { - logRouted.Printf("[CACHE] Max size reached (%d), retaining active entries until TTL eviction", c.maxSize) + lruKey := "" + lruTime := now // all real entries have lastUsed <= now + for k, entry := range c.servers { + if entry.lastUsed.Before(lruTime) { + lruKey = k + lruTime = entry.lastUsed + } + } + if lruKey != "" { + logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s (idle %s)", c.maxSize, auth.TruncateSessionID(lruKey), now.Sub(lruTime).Round(time.Second)) + delete(c.servers, lruKey) + } } logRouted.Printf("[CACHE] Creating new filtered server: backend=%s, session=%s", backendID, auth.TruncateSessionID(sessionID)) diff --git a/internal/server/routed_test.go b/internal/server/routed_test.go index 131cd979..736070a5 100644 --- a/internal/server/routed_test.go +++ b/internal/server/routed_test.go @@ -557,8 +557,8 @@ func TestCreateFilteredServer_EdgeCases(t *testing.T) { }) } -// TestFilteredServerCache_MaxSize verifies that the cache allows growth beyond maxSize -// when all entries are still active (non-expired), to avoid disrupting sessions. +// TestFilteredServerCache_MaxSize verifies that when the cache is at capacity, the +// least-recently-used entry is evicted to make room for a new entry. func TestFilteredServerCache_MaxSize(t *testing.T) { assert := assert.New(t) @@ -582,21 +582,30 @@ func TestFilteredServerCache_MaxSize(t *testing.T) { assert.NotNil(s3) assert.Equal(3, len(cache.servers), "Cache should have 3 entries") - // Adding a fourth entry should be allowed (no LRU eviction of active sessions) + // Manually set lastUsed to ensure deterministic LRU ordering: + // session1 is least recently used, session3 is most recently used. + now := time.Now() + cache.servers["backend/session1"].lastUsed = now.Add(-3 * time.Millisecond) + cache.servers["backend/session2"].lastUsed = now.Add(-2 * time.Millisecond) + cache.servers["backend/session3"].lastUsed = now.Add(-1 * time.Millisecond) + + // Adding a fourth entry should evict the LRU entry (session1) to stay within maxSize s4 := cache.getOrCreate("backend", "session4", creator) assert.Equal(4, callCount, "Should have created a 4th server") assert.NotNil(s4) - assert.Equal(4, len(cache.servers), "Cache should grow beyond maxSize for active sessions") + assert.Equal(3, len(cache.servers), "Cache should maintain maxSize by evicting the LRU entry") - // All sessions should still be present + // session1 (LRU) should have been evicted _, session1Exists := cache.servers["backend/session1"] - assert.True(session1Exists, "session1 should still be cached") + assert.False(session1Exists, "session1 (LRU) should have been evicted to make room") + + // session2, session3, session4 should still be present _, session2Exists := cache.servers["backend/session2"] assert.True(session2Exists, "session2 should still be cached") _, session3Exists := cache.servers["backend/session3"] assert.True(session3Exists, "session3 should still be cached") _, session4Exists := cache.servers["backend/session4"] - assert.True(session4Exists, "session4 should still be cached") + assert.True(session4Exists, "session4 should be cached") } // TestFilteredServerCache_TTLEviction verifies that expired entries are evicted. diff --git a/internal/testutil/mcptest/validator.go b/internal/testutil/mcptest/validator.go index b37f30f9..f6f421b4 100644 --- a/internal/testutil/mcptest/validator.go +++ b/internal/testutil/mcptest/validator.go @@ -4,9 +4,12 @@ import ( "context" "fmt" + "github.com/github/gh-aw-mcpg/internal/logger" sdk "github.com/modelcontextprotocol/go-sdk/mcp" ) +var logValidator = logger.New("testutil:validator") + // ValidatorClient is a client for validating MCP servers type ValidatorClient struct { client *sdk.Client @@ -19,7 +22,9 @@ func NewValidatorClient(ctx context.Context, transport sdk.Transport) (*Validato client := sdk.NewClient(&sdk.Implementation{ Name: "mcp-validator", Version: "1.0.0", - }, &sdk.ClientOptions{}) + }, &sdk.ClientOptions{ + Logger: logger.NewSlogLoggerWithHandler(logValidator), + }) session, err := client.Connect(ctx, transport, nil) if err != nil { @@ -33,22 +38,40 @@ func NewValidatorClient(ctx context.Context, transport sdk.Transport) (*Validato }, nil } -// ListTools retrieves the list of tools from the connected MCP server +// ListTools retrieves the list of tools from the connected MCP server, including all paginated results. func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { - result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{}) - if err != nil { - return nil, fmt.Errorf("list tools: %w", err) + var all []*sdk.Tool + var cursor string + for { + result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{Cursor: cursor}) + if err != nil { + return nil, fmt.Errorf("list tools: %w", err) + } + all = append(all, result.Tools...) + if result.NextCursor == "" { + break + } + cursor = result.NextCursor } - return result.Tools, nil + return all, nil } -// ListResources retrieves the list of resources from the connected MCP server +// ListResources retrieves the list of resources from the connected MCP server, including all paginated results. func (v *ValidatorClient) ListResources() ([]*sdk.Resource, error) { - result, err := v.session.ListResources(v.ctx, &sdk.ListResourcesParams{}) - if err != nil { - return nil, fmt.Errorf("list resources: %w", err) + var all []*sdk.Resource + var cursor string + for { + result, err := v.session.ListResources(v.ctx, &sdk.ListResourcesParams{Cursor: cursor}) + if err != nil { + return nil, fmt.Errorf("list resources: %w", err) + } + all = append(all, result.Resources...) + if result.NextCursor == "" { + break + } + cursor = result.NextCursor } - return result.Resources, nil + return all, nil } // CallTool calls a tool on the MCP server From 2ba2f836da822af4e5684557d175e09abf9ae6a3 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 14 Apr 2026 11:38:16 -0700 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/server/routed.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/server/routed.go b/internal/server/routed.go index 5bb7b3ee..60823830 100644 --- a/internal/server/routed.go +++ b/internal/server/routed.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "net/http" + "strings" "sync" "time" @@ -18,6 +19,15 @@ import ( var logRouted = logger.New("server:routed") +func truncateCacheKeyForLog(key string) string { + backendID, sessionID, found := strings.Cut(key, "/") + if !found { + return key + } + + return fmt.Sprintf("%s/%s", backendID, auth.TruncateSessionID(sessionID)) +} + // rejectIfShutdown is a middleware that rejects requests with HTTP 503 when gateway is shutting down // Per spec 5.1.3: "Immediately reject any new RPC requests to /mcp/{server-name} endpoints with HTTP 503" // The logNamespace parameter is used to create a logger for debug output specific to the call site. @@ -74,7 +84,7 @@ func (c *filteredServerCache) getOrCreate(backendID, sessionID string, creator f // Lazy eviction of expired entries for k, entry := range c.servers { if now.Sub(entry.lastUsed) > c.ttl { - logRouted.Printf("[CACHE] Evicting expired server: key=%s (idle %s)", auth.TruncateSessionID(k), now.Sub(entry.lastUsed).Round(time.Second)) + logRouted.Printf("[CACHE] Evicting expired server: key=%s (idle %s)", truncateCacheKeyForLog(k), now.Sub(entry.lastUsed).Round(time.Second)) delete(c.servers, k) } } @@ -97,7 +107,7 @@ func (c *filteredServerCache) getOrCreate(backendID, sessionID string, creator f } } if lruKey != "" { - logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s (idle %s)", c.maxSize, auth.TruncateSessionID(lruKey), now.Sub(lruTime).Round(time.Second)) + logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s (idle %s)", c.maxSize, truncateCacheKeyForLog(lruKey), now.Sub(lruTime).Round(time.Second)) delete(c.servers, lruKey) } } From 44e6134a04ddce89a9aac0bc707f8471569418fa Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 14 Apr 2026 11:38:29 -0700 Subject: [PATCH 4/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/testutil/mcptest/validator.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/testutil/mcptest/validator.go b/internal/testutil/mcptest/validator.go index f6f421b4..e00fbadc 100644 --- a/internal/testutil/mcptest/validator.go +++ b/internal/testutil/mcptest/validator.go @@ -10,6 +10,8 @@ import ( var logValidator = logger.New("testutil:validator") +const validatorPaginationMaxPages = 1000 + // ValidatorClient is a client for validating MCP servers type ValidatorClient struct { client *sdk.Client @@ -42,7 +44,14 @@ func NewValidatorClient(ctx context.Context, transport sdk.Transport) (*Validato func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { var all []*sdk.Tool var cursor string + seenCursors := make(map[string]struct{}) + pages := 0 for { + pages++ + if pages > validatorPaginationMaxPages { + return nil, fmt.Errorf("list tools: exceeded maximum pagination limit of %d pages", validatorPaginationMaxPages) + } + result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{Cursor: cursor}) if err != nil { return nil, fmt.Errorf("list tools: %w", err) @@ -51,6 +60,10 @@ func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { if result.NextCursor == "" { break } + if _, ok := seenCursors[result.NextCursor]; ok { + return nil, fmt.Errorf("list tools: detected repeated pagination cursor %q", result.NextCursor) + } + seenCursors[result.NextCursor] = struct{}{} cursor = result.NextCursor } return all, nil @@ -60,7 +73,14 @@ func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { func (v *ValidatorClient) ListResources() ([]*sdk.Resource, error) { var all []*sdk.Resource var cursor string + seenCursors := make(map[string]struct{}) + pages := 0 for { + pages++ + if pages > validatorPaginationMaxPages { + return nil, fmt.Errorf("list resources: exceeded maximum pagination limit of %d pages", validatorPaginationMaxPages) + } + result, err := v.session.ListResources(v.ctx, &sdk.ListResourcesParams{Cursor: cursor}) if err != nil { return nil, fmt.Errorf("list resources: %w", err) @@ -69,6 +89,10 @@ func (v *ValidatorClient) ListResources() ([]*sdk.Resource, error) { if result.NextCursor == "" { break } + if _, ok := seenCursors[result.NextCursor]; ok { + return nil, fmt.Errorf("list resources: detected repeated pagination cursor %q", result.NextCursor) + } + seenCursors[result.NextCursor] = struct{}{} cursor = result.NextCursor } return all, nil From acc71faeea87db932aab0d776afc5e71d9c8e4b2 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Tue, 14 Apr 2026 11:47:17 -0700 Subject: [PATCH 5/5] review: add truncateCacheKeyForLog test, robust LRU init, DRY pagination - Add table-driven test for truncateCacheKeyForLog covering standard keys, missing delimiter, empty key, short session, and multi-slash keys - Replace lruTime := now initialization with first-candidate flag pattern for more robust LRU selection independent of time invariants - Extract generic paginate[T] helper to eliminate duplicated pagination logic between ListTools and ListResources Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/server/routed.go | 6 ++- internal/server/routed_test.go | 42 +++++++++++++++ internal/testutil/mcptest/validator.go | 71 ++++++++++++++------------ 3 files changed, 83 insertions(+), 36 deletions(-) diff --git a/internal/server/routed.go b/internal/server/routed.go index 60823830..7549b5d2 100644 --- a/internal/server/routed.go +++ b/internal/server/routed.go @@ -99,11 +99,13 @@ func (c *filteredServerCache) getOrCreate(backendID, sessionID string, creator f // the evicted (backend, session) pair, but is preferable to unbounded growth. if len(c.servers) >= c.maxSize { lruKey := "" - lruTime := now // all real entries have lastUsed <= now + var lruTime time.Time + first := true for k, entry := range c.servers { - if entry.lastUsed.Before(lruTime) { + if first || entry.lastUsed.Before(lruTime) { lruKey = k lruTime = entry.lastUsed + first = false } } if lruKey != "" { diff --git a/internal/server/routed_test.go b/internal/server/routed_test.go index 736070a5..c7a7ad4f 100644 --- a/internal/server/routed_test.go +++ b/internal/server/routed_test.go @@ -608,6 +608,48 @@ func TestFilteredServerCache_MaxSize(t *testing.T) { assert.True(session4Exists, "session4 should be cached") } +// TestTruncateCacheKeyForLog verifies that cache keys are properly truncated for logging. +func TestTruncateCacheKeyForLog(t *testing.T) { + tests := []struct { + name string + key string + expected string + }{ + { + name: "standard key with backendID/sessionID", + key: "github/abc123def456ghi789", + expected: "github/abc123de...", + }, + { + name: "key without slash returns as-is", + key: "nodelimiter", + expected: "nodelimiter", + }, + { + name: "empty key", + key: "", + expected: "", + }, + { + name: "key with short session", + key: "backend/ab", + expected: "backend/ab", + }, + { + name: "key with multiple slashes truncates after first", + key: "backend/session/extra", + expected: "backend/session/...", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := truncateCacheKeyForLog(tt.key) + assert.Equal(t, tt.expected, result) + }) + } +} + // TestFilteredServerCache_TTLEviction verifies that expired entries are evicted. func TestFilteredServerCache_TTLEviction(t *testing.T) { assert := assert.New(t) diff --git a/internal/testutil/mcptest/validator.go b/internal/testutil/mcptest/validator.go index e00fbadc..ed32b968 100644 --- a/internal/testutil/mcptest/validator.go +++ b/internal/testutil/mcptest/validator.go @@ -40,62 +40,65 @@ func NewValidatorClient(ctx context.Context, transport sdk.Transport) (*Validato }, nil } -// ListTools retrieves the list of tools from the connected MCP server, including all paginated results. -func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { - var all []*sdk.Tool +// paginate collects all pages from a paginated MCP list call. +// fetch is called with a cursor (empty string for the first page) and returns the items, +// the next cursor (empty when done), and any error. +func paginate[T any](ctx context.Context, fetch func(ctx context.Context, cursor string) ([]T, string, error)) ([]T, error) { + var all []T var cursor string seenCursors := make(map[string]struct{}) pages := 0 for { pages++ if pages > validatorPaginationMaxPages { - return nil, fmt.Errorf("list tools: exceeded maximum pagination limit of %d pages", validatorPaginationMaxPages) + return nil, fmt.Errorf("exceeded maximum pagination limit of %d pages", validatorPaginationMaxPages) } - result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{Cursor: cursor}) + items, nextCursor, err := fetch(ctx, cursor) if err != nil { - return nil, fmt.Errorf("list tools: %w", err) + return nil, err } - all = append(all, result.Tools...) - if result.NextCursor == "" { + all = append(all, items...) + if nextCursor == "" { break } - if _, ok := seenCursors[result.NextCursor]; ok { - return nil, fmt.Errorf("list tools: detected repeated pagination cursor %q", result.NextCursor) + if _, ok := seenCursors[nextCursor]; ok { + return nil, fmt.Errorf("detected repeated pagination cursor %q", nextCursor) } - seenCursors[result.NextCursor] = struct{}{} - cursor = result.NextCursor + seenCursors[nextCursor] = struct{}{} + cursor = nextCursor } return all, nil } -// ListResources retrieves the list of resources from the connected MCP server, including all paginated results. -func (v *ValidatorClient) ListResources() ([]*sdk.Resource, error) { - var all []*sdk.Resource - var cursor string - seenCursors := make(map[string]struct{}) - pages := 0 - for { - pages++ - if pages > validatorPaginationMaxPages { - return nil, fmt.Errorf("list resources: exceeded maximum pagination limit of %d pages", validatorPaginationMaxPages) +// ListTools retrieves the list of tools from the connected MCP server, including all paginated results. +func (v *ValidatorClient) ListTools() ([]*sdk.Tool, error) { + tools, err := paginate(v.ctx, func(ctx context.Context, cursor string) ([]*sdk.Tool, string, error) { + result, err := v.session.ListTools(ctx, &sdk.ListToolsParams{Cursor: cursor}) + if err != nil { + return nil, "", err } + return result.Tools, result.NextCursor, nil + }) + if err != nil { + return nil, fmt.Errorf("list tools: %w", err) + } + return tools, nil +} - result, err := v.session.ListResources(v.ctx, &sdk.ListResourcesParams{Cursor: cursor}) +// ListResources retrieves the list of resources from the connected MCP server, including all paginated results. +func (v *ValidatorClient) ListResources() ([]*sdk.Resource, error) { + resources, err := paginate(v.ctx, func(ctx context.Context, cursor string) ([]*sdk.Resource, string, error) { + result, err := v.session.ListResources(ctx, &sdk.ListResourcesParams{Cursor: cursor}) if err != nil { - return nil, fmt.Errorf("list resources: %w", err) + return nil, "", err } - all = append(all, result.Resources...) - if result.NextCursor == "" { - break - } - if _, ok := seenCursors[result.NextCursor]; ok { - return nil, fmt.Errorf("list resources: detected repeated pagination cursor %q", result.NextCursor) - } - seenCursors[result.NextCursor] = struct{}{} - cursor = result.NextCursor + return result.Resources, result.NextCursor, nil + }) + if err != nil { + return nil, fmt.Errorf("list resources: %w", err) } - return all, nil + return resources, nil } // CallTool calls a tool on the MCP server