Skip to content

fix: ValidatorClient pagination, logger, and filteredServerCache LRU eviction#3789

Merged
lpcox merged 5 commits intomainfrom
copilot/go-fan-review-go-sdk
Apr 14, 2026
Merged

fix: ValidatorClient pagination, logger, and filteredServerCache LRU eviction#3789
lpcox merged 5 commits intomainfrom
copilot/go-fan-review-go-sdk

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

Three improvements to go-sdk usage identified in the go-fan module review: a silent data loss bug in test infrastructure, a missing logger on the test client, and unbounded memory growth in the routed mode server cache.

Changes

ValidatorClient pagination (internal/testutil/mcptest/validator.go)

ListTools() and ListResources() only fetched the first page — backends with many tools silently returned incomplete results. Both methods now loop on NextCursor:

// Before: only returns first page
result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{})
return result.Tools, nil

// After: collects all pages
for {
    result, err := v.session.ListTools(v.ctx, &sdk.ListToolsParams{Cursor: cursor})
    all = append(all, result.Tools...)
    if result.NextCursor == "" { break }
    cursor = result.NextCursor
}

ValidatorClient logger (internal/testutil/mcptest/validator.go)

sdk.ClientOptions{} had no Logger set, unlike all production clients. Now passes logger.NewSlogLoggerWithHandler(logValidator) for consistent test visibility.

filteredServerCache LRU eviction (internal/server/routed.go)

At maxSize (1000 entries), the cache previously logged a warning and grew without bound. Now evicts the least-recently-used entry to reliably cap memory. LRU scan initializes lruTime := now so that any real entry (always lastUsed ≤ now) is a candidate:

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
    }
}

TestFilteredServerCache_MaxSize is updated to assert eviction rather than unbounded growth.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /tmp/go-build3619024610/b514/launcher.test /tmp/go-build3619024610/b514/launcher.test -test.testlogfile=/tmp/go-build3619024610/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s 5542�� cfg olang.org/grpc@v-ifaceassert x_amd64/link --gdwarf-5 --64 -o x_amd64/link cfg 554253/b365/_pkg_.a -I x_amd64/vet --gdwarf-5 g/protobuf/encod/usr/bin/runc -o x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3619024610/b496/config.test /tmp/go-build3619024610/b496/config.test -test.testlogfile=/tmp/go-build3619024610/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3619024610/b394/vet.cfg @v1.1.3/cpu/x86/go1.25.8 64/src/crypto/tl-c=4 x_amd64/vet -p ernal/oidc -lang=go1.25 x_amd64/vet -qui�� _.a xrGu/Ij3Uman6G7f-ifaceassert x_amd64/vet . telabs/wazero/in-atomic x86_64-linux-gnu-bool x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build3619024610/b514/launcher.test /tmp/go-build3619024610/b514/launcher.test -test.testlogfile=/tmp/go-build3619024610/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s 5542�� cfg olang.org/grpc@v-ifaceassert x_amd64/link --gdwarf-5 --64 -o x_amd64/link cfg 554253/b365/_pkg_.a -I x_amd64/vet --gdwarf-5 g/protobuf/encod/usr/bin/runc -o x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build3619024610/b514/launcher.test /tmp/go-build3619024610/b514/launcher.test -test.testlogfile=/tmp/go-build3619024610/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s 5542�� cfg olang.org/grpc@v-ifaceassert x_amd64/link --gdwarf-5 --64 -o x_amd64/link cfg 554253/b365/_pkg_.a -I x_amd64/vet --gdwarf-5 g/protobuf/encod/usr/bin/runc -o x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3619024610/b523/mcp.test /tmp/go-build3619024610/b523/mcp.test -test.testlogfile=/tmp/go-build3619024610/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I cfg -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet cfg wwMF/PIk94C_Qa7ObsBMvwwMF /tmp/go-build527554253/b288/ x_amd64/vet . 554253/b468/ --64 x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI linked an issue Apr 14, 2026 that may be closed by this pull request
5 tasks
… 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>
Copilot AI changed the title [WIP] Review Go Module for modelcontextprotocol/go-sdk fix: ValidatorClient pagination, logger, and filteredServerCache LRU eviction Apr 14, 2026
Copilot AI requested a review from lpcox April 14, 2026 18:24
@lpcox lpcox marked this pull request as ready for review April 14, 2026 18:34
Copilot AI review requested due to automatic review settings April 14, 2026 18:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves reliability and operability around MCP server interactions by fixing incomplete pagination in the test ValidatorClient, ensuring SDK client logging is wired up in tests, and bounding routed-mode filtered server cache growth via LRU eviction.

Changes:

  • Update ValidatorClient.ListTools() / ListResources() to fetch all paginated results via NextCursor.
  • Configure an SDK logger for the test ValidatorClient for improved visibility.
  • Implement LRU eviction in filteredServerCache at capacity and update the max-size cache test accordingly.
Show a summary per file
File Description
internal/testutil/mcptest/validator.go Adds SDK logger to test client and iterates through pagination cursors to avoid silent partial listings.
internal/server/routed.go Changes cache-at-capacity behavior from “warn and grow” to LRU eviction to cap memory usage.
internal/server/routed_test.go Updates max-size cache test to assert deterministic LRU eviction behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

internal/testutil/mcptest/validator.go:73

  • Same as ListTools(): this pagination loop has no max-page or cursor-repeat guard, so a backend that never terminates pagination could make tests hang and grow memory without bound. Align with the existing guarded pagination pattern used in internal/mcp/connection.go (paginateAllMaxPages) or add a local cap.
	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
	}
  • Files reviewed: 3/3 changed files
  • Comments generated: 3

Comment thread internal/testutil/mcptest/validator.go Outdated
Comment thread internal/server/routed.go
Comment on lines +91 to +99
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 != "" {
Comment thread internal/server/routed.go Outdated
lpcox and others added 2 commits April 14, 2026 11:38
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented Apr 14, 2026

Review Feedback

Three items — I'll push commits for each.

1. Missing test for truncateCacheKeyForLog

This helper handles security-sensitive session ID truncation in cache keys but has no test coverage. I'll add a table-driven test.

2. Fragile LRU selection initialization

lruTime := now relies on the subtle invariant that all entries have lastUsed <= now. While practically true, using a first-candidate flag is more robust and self-documenting:

var lruTime time.Time
first := true
for k, entry := range c.servers {
    if first || entry.lastUsed.Before(lruTime) {
        lruKey = k
        lruTime = entry.lastUsed
        first = false
    }
}

3. Duplicated pagination logic in ListTools / ListResources

Both methods have identical pagination loops (cursor tracking, max-pages guard, cycle detection). Since the project uses Go 1.25, I'll extract a generic paginate[T] helper to DRY this up.

- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go-fan] Go Module Review: go-sdk (modelcontextprotocol/go-sdk)

3 participants