From a456b8249ff1c9923002efb5cd1fe7c074943580 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 17 Dec 2025 22:55:09 +0100 Subject: [PATCH 1/2] perf: add inventory cache for stateless server patterns Add CachedInventory to build tool/resource/prompt definitions once at startup rather than per-request. This is particularly useful for the remote server pattern where a new server instance is created per request. Key features: - InitInventoryCache(t) initializes the cache once at startup - InitInventoryCacheWithExtras(t, tools, resources, prompts) allows injecting additional items (e.g., remote-only Copilot tools) - CachedInventoryBuilder() returns a builder with pre-cached definitions - Per-request configuration (read-only, toolsets, feature flags) still works - Thread-safe via sync.Once - Backward compatible: NewInventory(t) still works without caching This addresses the performance concern raised in go-sdk PR #685 at a higher level by caching the entire []ServerTool slice rather than individual schemas. Related: https://github.com/modelcontextprotocol/go-sdk/pull/685 --- pkg/github/inventory_cache.go | 160 +++++++++++++++++ pkg/github/inventory_cache_test.go | 268 +++++++++++++++++++++++++++++ 2 files changed, 428 insertions(+) create mode 100644 pkg/github/inventory_cache.go create mode 100644 pkg/github/inventory_cache_test.go diff --git a/pkg/github/inventory_cache.go b/pkg/github/inventory_cache.go new file mode 100644 index 000000000..1fb33f154 --- /dev/null +++ b/pkg/github/inventory_cache.go @@ -0,0 +1,160 @@ +package github + +import ( + "sync" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" +) + +// CachedInventory provides a cached inventory builder that builds tool definitions +// only once, regardless of how many times NewInventoryBuilder is called. +// +// This is particularly useful for stateless server patterns (like the remote server) +// where a new server instance is created per request. Without caching, every request +// would rebuild all ~130 tool definitions including JSON schema generation, causing +// significant performance overhead. +// +// Usage: +// +// // Option 1: Initialize once at startup with your translator +// github.InitInventoryCache(myTranslator) +// +// // Then get pre-built inventory on each request +// inv := github.CachedInventoryBuilder(). +// WithReadOnly(cfg.ReadOnly). +// WithToolsets(cfg.Toolsets). +// Build() +// +// // Option 2: Use NewInventory which doesn't use the cache (legacy behavior) +// inv := github.NewInventory(myTranslator).Build() +// +// The cache stores the built []ServerTool, []ServerResourceTemplate, and []ServerPrompt. +// Per-request configuration (read-only, toolsets, feature flags, filters) is still +// applied when building the Inventory from the cached data. +type CachedInventory struct { + once sync.Once + tools []inventory.ServerTool + resources []inventory.ServerResourceTemplate + prompts []inventory.ServerPrompt +} + +// global singleton for caching +var globalInventoryCache = &CachedInventory{} + +// InitInventoryCache initializes the global inventory cache with the given translator. +// This should be called once at startup before any requests are processed. +// It's safe to call multiple times - only the first call has any effect. +// +// For the local server, this is typically called with the configured translator. +// For the remote server, use translations.NullTranslationHelper since translations +// aren't needed per-request. +// +// Example: +// +// func main() { +// t, _ := translations.TranslationHelper() +// github.InitInventoryCache(t) +// // ... start server +// } +func InitInventoryCache(t translations.TranslationHelperFunc) { + globalInventoryCache.init(t, nil, nil, nil) +} + +// InitInventoryCacheWithExtras initializes the global inventory cache with the given +// translator plus additional tools, resources, and prompts. +// +// This is useful for the remote server which has additional tools (e.g., Copilot tools) +// that aren't part of the base github-mcp-server package. +// +// The extra items are appended to the base items from AllTools/AllResources/AllPrompts. +// It's safe to call multiple times - only the first call has any effect. +// +// Example: +// +// func init() { +// github.InitInventoryCacheWithExtras( +// translations.NullTranslationHelper, +// remoteOnlyTools, // []inventory.ServerTool +// remoteOnlyResources, // []inventory.ServerResourceTemplate +// remoteOnlyPrompts, // []inventory.ServerPrompt +// ) +// } +func InitInventoryCacheWithExtras( + t translations.TranslationHelperFunc, + extraTools []inventory.ServerTool, + extraResources []inventory.ServerResourceTemplate, + extraPrompts []inventory.ServerPrompt, +) { + globalInventoryCache.init(t, extraTools, extraResources, extraPrompts) +} + +// init initializes the cache with the given translator and optional extras (sync.Once protected). +func (c *CachedInventory) init( + t translations.TranslationHelperFunc, + extraTools []inventory.ServerTool, + extraResources []inventory.ServerResourceTemplate, + extraPrompts []inventory.ServerPrompt, +) { + c.once.Do(func() { + c.tools = AllTools(t) + c.resources = AllResources(t) + c.prompts = AllPrompts(t) + + // Append extra items if provided + if len(extraTools) > 0 { + c.tools = append(c.tools, extraTools...) + } + if len(extraResources) > 0 { + c.resources = append(c.resources, extraResources...) + } + if len(extraPrompts) > 0 { + c.prompts = append(c.prompts, extraPrompts...) + } + }) +} + +// CachedInventoryBuilder returns an inventory.Builder pre-populated with cached +// tool/resource/prompt definitions. +// +// The cache must be initialized via InitInventoryCache before calling this function. +// If the cache is not initialized, this will initialize it with NullTranslationHelper. +// +// Per-request configuration can still be applied via the builder methods: +// - WithReadOnly(bool) - filter to read-only tools +// - WithToolsets([]string) - enable specific toolsets +// - WithTools([]string) - enable specific tools +// - WithFeatureChecker(func) - per-request feature flag evaluation +// - WithFilter(func) - custom filtering +// +// Example: +// +// inv := github.CachedInventoryBuilder(). +// WithReadOnly(cfg.ReadOnly). +// WithToolsets(cfg.EnabledToolsets). +// WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)). +// Build() +func CachedInventoryBuilder() *inventory.Builder { + // Ensure cache is initialized (with NullTranslationHelper as fallback) + globalInventoryCache.init(translations.NullTranslationHelper, nil, nil, nil) + + return inventory.NewBuilder(). + SetTools(globalInventoryCache.tools). + SetResources(globalInventoryCache.resources). + SetPrompts(globalInventoryCache.prompts) +} + +// IsCacheInitialized returns true if the inventory cache has been initialized. +// This is primarily useful for testing. +func IsCacheInitialized() bool { + // We can't directly check sync.Once state, but we can check if tools are populated + return len(globalInventoryCache.tools) > 0 +} + +// ResetInventoryCache resets the global inventory cache, allowing it to be +// reinitialized with a different translator. This should only be used in tests. +// +// WARNING: This is not thread-safe and should never be called in production code. +func ResetInventoryCache() { + globalInventoryCache = &CachedInventory{} +} diff --git a/pkg/github/inventory_cache_test.go b/pkg/github/inventory_cache_test.go new file mode 100644 index 000000000..4c85a9f14 --- /dev/null +++ b/pkg/github/inventory_cache_test.go @@ -0,0 +1,268 @@ +package github + +import ( + "context" + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCachedInventory(t *testing.T) { + // Reset cache before test + ResetInventoryCache() + + t.Run("InitInventoryCache initializes tools", func(t *testing.T) { + ResetInventoryCache() + + // Cache should be empty before init + assert.False(t, IsCacheInitialized()) + + // Initialize with null translator + InitInventoryCache(translations.NullTranslationHelper) + + // Cache should now be initialized + assert.True(t, IsCacheInitialized()) + }) + + t.Run("CachedInventoryBuilder returns builder with cached tools", func(t *testing.T) { + ResetInventoryCache() + InitInventoryCache(translations.NullTranslationHelper) + + builder := CachedInventoryBuilder() + require.NotNil(t, builder) + + inv := builder.Build() + require.NotNil(t, inv) + + // Should have tools available + tools := inv.AllTools() + assert.Greater(t, len(tools), 0) + }) + + t.Run("CachedInventoryBuilder auto-initializes if not initialized", func(t *testing.T) { + ResetInventoryCache() + + // Don't call InitInventoryCache + + // CachedInventoryBuilder should still work (auto-init with NullTranslationHelper) + builder := CachedInventoryBuilder() + require.NotNil(t, builder) + + inv := builder.Build() + require.NotNil(t, inv) + + tools := inv.AllTools() + assert.Greater(t, len(tools), 0) + }) + + t.Run("InitInventoryCache is idempotent", func(t *testing.T) { + ResetInventoryCache() + + // First init with a custom translator that tracks calls + callCount := 0 + customTranslator := func(_, defaultValue string) string { + callCount++ + return defaultValue + } + + InitInventoryCache(customTranslator) + firstCallCount := callCount + + // Second init should be no-op + callCount = 0 + InitInventoryCache(translations.NullTranslationHelper) + + assert.Equal(t, 0, callCount, "second InitInventoryCache should not call translator") + + // Verify first translator was used (tools are still populated from first call) + assert.True(t, IsCacheInitialized()) + assert.Greater(t, firstCallCount, 0, "first init should have called translator") + }) + + t.Run("cached inventory preserves per-request configuration", func(t *testing.T) { + ResetInventoryCache() + InitInventoryCache(translations.NullTranslationHelper) + + // Build with read-only filter + invReadOnly := CachedInventoryBuilder(). + WithReadOnly(true). + WithToolsets([]string{"all"}). + Build() + + // Build without read-only filter + invAll := CachedInventoryBuilder(). + WithReadOnly(false). + WithToolsets([]string{"all"}). + Build() + + // AvailableTools applies filters - all tools should have more than read-only + ctx := context.Background() + allTools := invAll.AvailableTools(ctx) + readOnlyTools := invReadOnly.AvailableTools(ctx) + + assert.Greater(t, len(allTools), len(readOnlyTools), + "read-only inventory should have fewer tools") + }) + + t.Run("cached inventory supports toolset filtering", func(t *testing.T) { + ResetInventoryCache() + InitInventoryCache(translations.NullTranslationHelper) + + // Build with only one toolset + invOneToolset := CachedInventoryBuilder(). + WithToolsets([]string{"context"}). + Build() + + // Build with all toolsets + invAll := CachedInventoryBuilder(). + WithToolsets([]string{"all"}). + Build() + + ctx := context.Background() + oneToolsetTools := invOneToolset.AvailableTools(ctx) + allTools := invAll.AvailableTools(ctx) + + assert.Greater(t, len(allTools), len(oneToolsetTools), + "all toolsets should have more tools than single toolset") + }) +} + +func TestNewInventoryVsCachedInventoryBuilder(t *testing.T) { + ResetInventoryCache() + + // NewInventory creates fresh tools each time + inv1 := NewInventory(translations.NullTranslationHelper).Build() + inv2 := NewInventory(translations.NullTranslationHelper).Build() + + // Both should have the same number of tools + assert.Equal(t, len(inv1.AllTools()), len(inv2.AllTools())) + + // CachedInventoryBuilder uses cached tools + InitInventoryCache(translations.NullTranslationHelper) + cachedInv := CachedInventoryBuilder().Build() + + // Should also have the same number of tools + assert.Equal(t, len(inv1.AllTools()), len(cachedInv.AllTools())) +} + +func TestInitInventoryCacheWithExtras(t *testing.T) { + ResetInventoryCache() + + // Create some extra tools for testing + extraTools := []inventory.ServerTool{ + { + Tool: mcp.Tool{ + Name: "extra_tool_1", + Description: "An extra tool for testing", + }, + Toolset: ToolsetMetadataContext, + }, + { + Tool: mcp.Tool{ + Name: "extra_tool_2", + Description: "Another extra tool for testing", + }, + Toolset: ToolsetMetadataRepos, + }, + } + + extraResources := []inventory.ServerResourceTemplate{ + { + Template: mcp.ResourceTemplate{ + Name: "extra_resource", + URITemplate: "extra://resource/{id}", + }, + Toolset: ToolsetMetadataContext, + }, + } + + extraPrompts := []inventory.ServerPrompt{ + { + Prompt: mcp.Prompt{ + Name: "extra_prompt", + Description: "An extra prompt for testing", + }, + Toolset: ToolsetMetadataContext, + }, + } + + // Get baseline count without extras + baselineInv := NewInventory(translations.NullTranslationHelper).Build() + baseToolCount := len(baselineInv.AllTools()) + + // Initialize cache with extras + InitInventoryCacheWithExtras( + translations.NullTranslationHelper, + extraTools, + extraResources, + extraPrompts, + ) + + // Build inventory from cache + inv := CachedInventoryBuilder(). + WithToolsets([]string{"all"}). + Build() + + ctx := context.Background() + cachedTools := inv.AvailableTools(ctx) + + // Should have base tools + extra tools + assert.Equal(t, baseToolCount+len(extraTools), len(cachedTools), + "cached inventory should include extra tools") + + // Verify extra tools are present + toolNames := make(map[string]bool) + for _, tool := range cachedTools { + toolNames[tool.Tool.Name] = true + } + assert.True(t, toolNames["extra_tool_1"], "extra_tool_1 should be in cached tools") + assert.True(t, toolNames["extra_tool_2"], "extra_tool_2 should be in cached tools") +} + +func TestInitInventoryCacheWithExtrasIsIdempotent(t *testing.T) { + ResetInventoryCache() + + extraTools1 := []inventory.ServerTool{ + { + Tool: mcp.Tool{ + Name: "first_extra", + Description: "First extra tool", + }, + Toolset: ToolsetMetadataContext, + }, + } + + extraTools2 := []inventory.ServerTool{ + { + Tool: mcp.Tool{ + Name: "second_extra", + Description: "Second extra tool", + }, + Toolset: ToolsetMetadataContext, + }, + } + + // First init with first set of extras + InitInventoryCacheWithExtras(translations.NullTranslationHelper, extraTools1, nil, nil) + + // Second init should be ignored (sync.Once) + InitInventoryCacheWithExtras(translations.NullTranslationHelper, extraTools2, nil, nil) + + inv := CachedInventoryBuilder().WithToolsets([]string{"all"}).Build() + ctx := context.Background() + tools := inv.AvailableTools(ctx) + + toolNames := make(map[string]bool) + for _, tool := range tools { + toolNames[tool.Tool.Name] = true + } + + // First extra should be present + assert.True(t, toolNames["first_extra"], "first_extra should be in cached tools") + // Second extra should NOT be present (second init was ignored) + assert.False(t, toolNames["second_extra"], "second_extra should NOT be in cached tools") +} From 1d05b3285ee660c621475f223a68584a730e661f Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 17 Dec 2025 23:06:41 +0100 Subject: [PATCH 2/2] address review feedback - Fix inconsistent terminology (NewInventoryBuilder -> CachedInventoryBuilder) - Add explicit initialized flag for reliable state checking - Clarify contradictory documentation about initialization requirement - Improve ResetInventoryCache warning about thread safety - Add comment explaining why tests don't use t.Parallel() --- pkg/github/inventory_cache.go | 27 ++++++++++++++++----------- pkg/github/inventory_cache_test.go | 6 ++++++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/pkg/github/inventory_cache.go b/pkg/github/inventory_cache.go index 1fb33f154..424871e5d 100644 --- a/pkg/github/inventory_cache.go +++ b/pkg/github/inventory_cache.go @@ -8,7 +8,7 @@ import ( ) // CachedInventory provides a cached inventory builder that builds tool definitions -// only once, regardless of how many times NewInventoryBuilder is called. +// only once, regardless of how many times CachedInventoryBuilder is called. // // This is particularly useful for stateless server patterns (like the remote server) // where a new server instance is created per request. Without caching, every request @@ -33,10 +33,11 @@ import ( // Per-request configuration (read-only, toolsets, feature flags, filters) is still // applied when building the Inventory from the cached data. type CachedInventory struct { - once sync.Once - tools []inventory.ServerTool - resources []inventory.ServerResourceTemplate - prompts []inventory.ServerPrompt + once sync.Once + initialized bool // set to true after init completes + tools []inventory.ServerTool + resources []inventory.ServerResourceTemplate + prompts []inventory.ServerPrompt } // global singleton for caching @@ -111,14 +112,17 @@ func (c *CachedInventory) init( if len(extraPrompts) > 0 { c.prompts = append(c.prompts, extraPrompts...) } + + c.initialized = true }) } // CachedInventoryBuilder returns an inventory.Builder pre-populated with cached // tool/resource/prompt definitions. // -// The cache must be initialized via InitInventoryCache before calling this function. -// If the cache is not initialized, this will initialize it with NullTranslationHelper. +// The cache should typically be initialized via InitInventoryCache at startup. +// If not already initialized when this function is called, it will automatically +// initialize with NullTranslationHelper as a fallback. // // Per-request configuration can still be applied via the builder methods: // - WithReadOnly(bool) - filter to read-only tools @@ -147,14 +151,15 @@ func CachedInventoryBuilder() *inventory.Builder { // IsCacheInitialized returns true if the inventory cache has been initialized. // This is primarily useful for testing. func IsCacheInitialized() bool { - // We can't directly check sync.Once state, but we can check if tools are populated - return len(globalInventoryCache.tools) > 0 + return globalInventoryCache.initialized } // ResetInventoryCache resets the global inventory cache, allowing it to be -// reinitialized with a different translator. This should only be used in tests. +// reinitialized with a different translator. // -// WARNING: This is not thread-safe and should never be called in production code. +// WARNING: This function is for testing only. It is NOT thread-safe and must only +// be called when no other goroutines are accessing the cache. Tests using this +// function must not use t.Parallel(). func ResetInventoryCache() { globalInventoryCache = &CachedInventory{} } diff --git a/pkg/github/inventory_cache_test.go b/pkg/github/inventory_cache_test.go index 4c85a9f14..19690544b 100644 --- a/pkg/github/inventory_cache_test.go +++ b/pkg/github/inventory_cache_test.go @@ -1,5 +1,11 @@ package github +// NOTE: Tests in this file intentionally do NOT use t.Parallel(). +// They mutate the global inventory cache state via ResetInventoryCache, +// InitInventoryCache, and CachedInventoryBuilder. Running them in parallel +// would cause test flakiness and data races. Keep these tests serial even +// though most other tests in this package use t.Parallel(). + import ( "context" "testing"