From 41b274efe9ac10a89941fcc5e10e50498e981540 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 22 Jul 2026 10:09:02 +0300 Subject: [PATCH] fix(runtime): guard Truncator swap/read and un-capture stale truncator pointer (#861) Two live defects surfaced by Codex review of PR #857: 1. Data race: Runtime.Truncator() read r.truncator without holding r.mu while ApplyConfig swapped it under the lock (-race detectable). The field is now an atomic.Pointer[truncate.Truncator]; the accessor and the swap use Load/Store, so reads stay lock-free on the serving hot path and are independent of r.mu (composes with #857's configCommitMu commit-path serialization). 2. Stale captured pointer: the MCP request handler captured the truncator pointer once at construction, so a hot-reloaded tool_response_limit never reached live tool-call responses. NewMCPProxyServer now takes a func() *truncate.Truncator getter; the serving path resolves the truncator at use time via currentTruncator(). Production passes rt.Truncator (method value), so a swap takes effect on the next call. Tests: - TestTruncatorSwapRace: concurrent Truncator() reads + ApplyConfig swaps under -race. - TestTruncatorHotReloadOnServingPath: a limit change is observed by the serving path after reload. Closes #861 (partial): the two live defects only. The broader ApplyConfig/ReloadConfiguration side-effect unification remains tracked in --- cmd/mcpproxy/call_cmd.go | 2 +- cmd/mcpproxy/code_cmd.go | 2 +- internal/runtime/config_watcher_test.go | 8 +- internal/runtime/runtime.go | 29 +++++-- internal/runtime/truncator_race_test.go | 72 ++++++++++++++++ internal/server/mcp.go | 37 ++++++--- internal/server/mcp_activation_test.go | 2 +- internal/server/mcp_auth_scope_test.go | 2 +- internal/server/mcp_block_tools_test.go | 2 +- internal/server/mcp_code_execution_test.go | 10 +-- internal/server/mcp_sigcache_wiring_test.go | 2 +- internal/server/server.go | 2 +- internal/server/toon_detection_parity_test.go | 6 +- internal/server/toon_encode.go | 11 ++- internal/server/toon_encode_test.go | 2 +- .../server/toon_structured_content_test.go | 8 +- .../server/toon_surface_isolation_test.go | 2 +- internal/server/toon_truncation_order_test.go | 14 ++-- internal/server/truncator_reload_test.go | 82 +++++++++++++++++++ internal/server/upstream_test.go | 4 +- 20 files changed, 240 insertions(+), 59 deletions(-) create mode 100644 internal/runtime/truncator_race_test.go create mode 100644 internal/server/truncator_reload_test.go diff --git a/cmd/mcpproxy/call_cmd.go b/cmd/mcpproxy/call_cmd.go index 4bc3923f..09afbf80 100644 --- a/cmd/mcpproxy/call_cmd.go +++ b/cmd/mcpproxy/call_cmd.go @@ -486,7 +486,7 @@ func runCallToolVariantStandalone(ctx context.Context, toolVariant string, args indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, // mainServer not needed for CLI calls false, diff --git a/cmd/mcpproxy/code_cmd.go b/cmd/mcpproxy/code_cmd.go index bb66cf64..6b8f58c1 100644 --- a/cmd/mcpproxy/code_cmd.go +++ b/cmd/mcpproxy/code_cmd.go @@ -253,7 +253,7 @@ func runCodeExecStandalone(globalConfig *config.Config, code string, input map[s indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, false, diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 2261cf33..8dc1cdf1 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -409,12 +409,10 @@ func TestConfigWatcher_ReloadRebuildsTruncator(t *testing.T) { return rt.ConfigSnapshot().Config.ToolResponseLimit == 50 }, 5*time.Second, 25*time.Millisecond, "snapshot must pick up the external edit") - // The LIVE truncator must now enforce the new limit. Read it under r.mu: - // the reload path swaps the field under the same lock. + // The LIVE truncator must now enforce the new limit. Read it via the + // lock-free accessor: the field is an atomic.Pointer swapped on reload (#861). require.Eventually(t, func() bool { - rt.mu.RLock() - tr := rt.truncator - rt.mu.RUnlock() + tr := rt.Truncator() return len(tr.Truncate(content, "srv:tool", nil).TruncatedContent) < len(content) }, 5*time.Second, 25*time.Millisecond, "watcher reload must rebuild the truncator with the new tool_response_limit") diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index cd4d06aa..58b89aa5 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -14,6 +14,7 @@ import ( "sort" "strings" "sync" + "sync/atomic" "time" "go.uber.org/zap" @@ -99,11 +100,15 @@ type Runtime struct { eventMu sync.RWMutex eventSubs map[chan Event]struct{} - storageManager *storage.Manager - indexManager *index.Manager - upstreamManager *upstream.Manager - cacheManager *cache.Manager - truncator *truncate.Truncator + storageManager *storage.Manager + indexManager *index.Manager + upstreamManager *upstream.Manager + cacheManager *cache.Manager + // truncator is swapped on config hot-reload (tool_response_limit) while the + // MCP serving path reads it via Truncator(). An atomic.Pointer makes the + // swap/read race-free (#861) and independent of r.mu, so the accessor stays + // lock-free on the hot path and composes with the config commit serialization. + truncator atomic.Pointer[truncate.Truncator] sigCache *toolsig.Cache // Spec 085 FR-008: single process-wide signature cache (indexing warms, MCP reads) secretResolver *secret.Resolver tokenizer tokens.Tokenizer @@ -319,7 +324,6 @@ func New(cfg *config.Config, cfgPath string, logger *zap.Logger) (*Runtime, erro indexManager: indexManager, upstreamManager: upstreamManager, cacheManager: cacheManager, - truncator: truncator, sigCache: toolsig.NewCache(), secretResolver: secretResolver, tokenizer: tokenizer, @@ -341,6 +345,7 @@ func New(cfg *config.Config, cfgPath string, logger *zap.Logger) (*Runtime, erro lastGoodTools: make(map[string][]*config.ToolMetadata), profileMembership: make(map[string][]string), } + rt.truncator.Store(truncator) // Spec 047: drainer goroutine that publishes coalesced servers.changed // events. Lifetime is tied to appCtx so it shuts down with the runtime. @@ -635,9 +640,13 @@ func (r *Runtime) CacheManager() *cache.Manager { return r.cacheManager } -// Truncator exposes the truncator utility. +// Truncator exposes the current tool-response truncator. It is safe to call +// concurrently with a config hot-reload that swaps the truncator (#861): the +// field is an atomic.Pointer, so the read never tears against the swap. Callers +// on the serving path MUST call this at use time rather than caching the +// returned pointer, so a hot-reloaded tool_response_limit takes effect. func (r *Runtime) Truncator() *truncate.Truncator { - return r.truncator + return r.truncator.Load() } // SignatureCache exposes the process-wide compact-signature cache (Spec 085 @@ -2586,7 +2595,9 @@ func (r *Runtime) applyComponentConfigLocked(oldCfg, newCfg *config.Config) { r.logger.Info("Tool response limit changed, updating truncator", zap.Int("old_limit", oldCfg.ToolResponseLimit), zap.Int("new_limit", newCfg.ToolResponseLimit)) - r.truncator = truncate.NewTruncator(newCfg.ToolResponseLimit) + // Atomic swap: the serving path reads via Truncator() without r.mu, so + // the new limit takes effect on the next tool call (#861). + r.truncator.Store(truncate.NewTruncator(newCfg.ToolResponseLimit)) } // Apply observability usage cadence (Spec 069 A2 — hot-reloadable). The diff --git a/internal/runtime/truncator_race_test.go b/internal/runtime/truncator_race_test.go new file mode 100644 index 00000000..96d12495 --- /dev/null +++ b/internal/runtime/truncator_race_test.go @@ -0,0 +1,72 @@ +package runtime + +import ( + "path/filepath" + "sync" + "testing" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + + "github.com/stretchr/testify/require" +) + +// TestTruncatorSwapRace (#861 defect 1): Runtime.Truncator() must be safe to +// call concurrently while a config apply swaps the truncator under the lock. +// Before the fix, Truncator() read r.truncator without synchronization while +// ApplyConfig wrote it, a data race the -race detector flags. Run with -race. +func TestTruncatorSwapRace(t *testing.T) { + tmpDir := t.TempDir() + cfgPath := filepath.Join(tmpDir, "config.json") + + initialCfg := config.DefaultConfig() + initialCfg.Listen = "127.0.0.1:8080" + initialCfg.DataDir = tmpDir + initialCfg.ToolResponseLimit = 1000 + require.NoError(t, config.SaveConfig(initialCfg, cfgPath)) + + rt, err := New(initialCfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + defer func() { _ = rt.Close() }() + + const readers = 8 + const iterations = 40 + + var wg sync.WaitGroup + + // Readers: hammer the accessor (and use the returned value) concurrently. + for i := 0; i < readers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < iterations*4; j++ { + tr := rt.Truncator() + if tr != nil { + _ = tr.ShouldTruncate("some content that may exceed a limit") + } + } + }() + } + + // Writer: repeatedly apply configs that flip the tool-response limit, which + // swaps the truncator each time. + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + newCfg := config.DefaultConfig() + newCfg.Listen = "127.0.0.1:8080" + newCfg.DataDir = tmpDir + if j%2 == 0 { + newCfg.ToolResponseLimit = 2000 + } else { + newCfg.ToolResponseLimit = 1000 + } + _, applyErr := rt.ApplyConfig(newCfg, cfgPath) + require.NoError(t, applyErr) + } + }() + + wg.Wait() +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 5a3b7ead..4d665899 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -110,12 +110,16 @@ func mcpServerVersion() string { // MCPProxyServer implements an MCP server that acts as a proxy type MCPProxyServer struct { - server *mcpserver.MCPServer - storage *storage.Manager - index *index.Manager - upstreamManager *upstream.Manager - cacheManager *cache.Manager - truncator *truncate.Truncator + server *mcpserver.MCPServer + storage *storage.Manager + index *index.Manager + upstreamManager *upstream.Manager + cacheManager *cache.Manager + // truncatorFn resolves the current tool-response truncator at use time. It + // must NOT be replaced with a captured *truncate.Truncator: the runtime + // swaps its truncator on a tool_response_limit hot-reload, and the serving + // path has to observe the new limit (#861). Access via currentTruncator(). + truncatorFn func() *truncate.Truncator outputValidator *outputvalidation.Validator // Spec 056: output-schema validation (nil when disabled) inputValidator *inputValidator // Spec 085 FR-013: pre-dispatch argument validation (never nil) sanitisationDetector *security.Detector // Spec 054 Track B: secret detector for redact/block (nil when neither used) @@ -217,13 +221,24 @@ func (p *MCPProxyServer) finishToolCall(span oteltrace.Span, serverName, toolNam } } +// currentTruncator resolves the live tool-response truncator. The serving path +// must call this at use time (never cache the result) so a hot-reloaded +// tool_response_limit takes effect on the next tool call (#861). Returns nil +// when no truncator is wired (standalone/test constructions may pass nil). +func (p *MCPProxyServer) currentTruncator() *truncate.Truncator { + if p.truncatorFn == nil { + return nil + } + return p.truncatorFn() +} + // NewMCPProxyServer creates a new MCP proxy server func NewMCPProxyServer( storage *storage.Manager, index *index.Manager, upstreamManager *upstream.Manager, cacheManager *cache.Manager, - truncator *truncate.Truncator, + truncatorFn func() *truncate.Truncator, logger *zap.Logger, mainServer *Server, debugSearch bool, @@ -459,7 +474,7 @@ func NewMCPProxyServer( index: index, upstreamManager: upstreamManager, cacheManager: cacheManager, - truncator: truncator, + truncatorFn: truncatorFn, outputValidator: outputValidator, inputValidator: newInputValidator(logger), sanitisationDetector: sanitisationDetector, @@ -2191,7 +2206,7 @@ func (p *MCPProxyServer) handleCallToolVariant(ctx context.Context, request mcp. toonDetectionText, toonDecisions = p.encodeToonBlocks(serverName, actualToolName, contentTrust, args, ctr) } - forwarded, response, wasTruncated := forwardContentResult(result, p.truncator, p.cacheManager, p.logger, toolName, args) + forwarded, response, wasTruncated := forwardContentResult(result, p.currentTruncator(), p.cacheManager, p.logger, toolName, args) // Spec 056: output-schema validation. Strict mode blocks a violating result // (returns an error); warn mode forwards unchanged after recording a @@ -2597,7 +2612,7 @@ func (p *MCPProxyServer) handleCallTool(ctx context.Context, request mcp.CallToo legacyResponseBytes := rawByteSize(result) legacyRequestBytes := rawByteSize(activityArgs) - forwarded, response, wasTruncated := forwardContentResult(result, p.truncator, p.cacheManager, p.logger, toolName, args) + forwarded, response, wasTruncated := forwardContentResult(result, p.currentTruncator(), p.cacheManager, p.logger, toolName, args) // Spec 056: output-schema validation. Strict mode blocks a violating result // (returns an error); warn mode forwards unchanged after recording a @@ -4812,7 +4827,7 @@ func (p *MCPProxyServer) handleReadCache(ctx context.Context, request mcp.CallTo "read_cache", args, len(response.Records), - p.truncator, + p.currentTruncator(), p.cacheManager, p.logger, ) diff --git a/internal/server/mcp_activation_test.go b/internal/server/mcp_activation_test.go index 944a2600..2f74fc84 100644 --- a/internal/server/mcp_activation_test.go +++ b/internal/server/mcp_activation_test.go @@ -79,7 +79,7 @@ func buildMCPProxyWithActivation(t *testing.T) (*MCPProxyServer, *runtime.Runtim tr := truncate.NewTruncator(0) mainSrv := &Server{runtime: rt, logger: logger} - proxy := NewMCPProxyServer(rt.StorageManager(), idx, um, cm, tr, logger, mainSrv, false, cfg, rt.SignatureCache()) + proxy := NewMCPProxyServer(rt.StorageManager(), idx, um, cm, func() *truncate.Truncator { return tr }, logger, mainSrv, false, cfg, rt.SignatureCache()) return proxy, rt, svc.ActivationDB() } diff --git a/internal/server/mcp_auth_scope_test.go b/internal/server/mcp_auth_scope_test.go index 312e5b23..3d807765 100644 --- a/internal/server/mcp_auth_scope_test.go +++ b/internal/server/mcp_auth_scope_test.go @@ -51,7 +51,7 @@ func createTestMCPProxyServer(t *testing.T) *MCPProxyServer { tr := truncate.NewTruncator(0) - proxy := NewMCPProxyServer(sm, idx, um, cm, tr, logger, nil, false, cfg, nil) + proxy := NewMCPProxyServer(sm, idx, um, cm, func() *truncate.Truncator { return tr }, logger, nil, false, cfg, nil) return proxy } diff --git a/internal/server/mcp_block_tools_test.go b/internal/server/mcp_block_tools_test.go index 80125afd..3cf16ea8 100644 --- a/internal/server/mcp_block_tools_test.go +++ b/internal/server/mcp_block_tools_test.go @@ -56,7 +56,7 @@ func createTestProxyWithRuntime(t *testing.T, servers []*config.ServerConfig) (* tr := truncate.NewTruncator(0) mainSrv := &Server{runtime: rt} - proxy := NewMCPProxyServer(sm, idx, um, cm, tr, logger, mainSrv, false, cfg, rt.SignatureCache()) + proxy := NewMCPProxyServer(sm, idx, um, cm, func() *truncate.Truncator { return tr }, logger, mainSrv, false, cfg, rt.SignatureCache()) return proxy, rt } diff --git a/internal/server/mcp_code_execution_test.go b/internal/server/mcp_code_execution_test.go index e7df7e3a..eb790366 100644 --- a/internal/server/mcp_code_execution_test.go +++ b/internal/server/mcp_code_execution_test.go @@ -59,7 +59,7 @@ func TestCodeExecution_WithNilMainServer(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, // mainServer = nil (CLI mode) false, @@ -121,7 +121,7 @@ func newCodeExecProxy(t *testing.T) *server.MCPProxyServer { mcpProxy := server.NewMCPProxyServer( storageManager, indexManager, upstreamManager, cacheManager, - truncator, logger, nil, false, cfg, nil, + func() *truncate.Truncator { return truncator }, logger, nil, false, cfg, nil, ) t.Cleanup(func() { mcpProxy.Close() }) return mcpProxy @@ -257,7 +257,7 @@ func TestCodeExecution_TypeScript(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, false, @@ -319,7 +319,7 @@ func TestCodeExecution_JavaScriptBackwardCompat(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, false, @@ -381,7 +381,7 @@ func TestCodeExecution_InvalidLanguage(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, logger, nil, false, diff --git a/internal/server/mcp_sigcache_wiring_test.go b/internal/server/mcp_sigcache_wiring_test.go index a91c5b8d..c0eb86a5 100644 --- a/internal/server/mcp_sigcache_wiring_test.go +++ b/internal/server/mcp_sigcache_wiring_test.go @@ -35,7 +35,7 @@ func newRuntimeBackedProxy(t *testing.T) (*MCPProxyServer, *runtime.Runtime) { rt.IndexManager(), rt.UpstreamManager(), rt.CacheManager(), - rt.Truncator(), + rt.Truncator, logger, srv, false, diff --git a/internal/server/server.go b/internal/server/server.go index 8fbe6cb5..5be9daad 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -183,7 +183,7 @@ func NewServerWithConfigPath(cfg *config.Config, configPath string, logger *zap. rt.IndexManager(), rt.UpstreamManager(), rt.CacheManager(), - rt.Truncator(), + rt.Truncator, // getter, re-read at use time so hot-reloaded limits apply (#861) logger, server, cfg.DebugSearch, diff --git a/internal/server/toon_detection_parity_test.go b/internal/server/toon_detection_parity_test.go index db9fc61b..659daa03 100644 --- a/internal/server/toon_detection_parity_test.go +++ b/internal/server/toon_detection_parity_test.go @@ -55,7 +55,7 @@ func findingSet(det *security.Detector, text string) []security.Detection { // handleCallToolVariant runs at mcp.go ~2129-2141 when the seam returns // ("", nil). The result is mutated the same way the real pipeline mutates it. func offPathScanText(p *MCPProxyServer, serverName, toolName, contentTrust string, args map[string]interface{}, result *mcp.CallToolResult) string { - forwarded, response, _ := forwardContentResult(result, p.truncator, nil, nil, serverName+":"+toolName, args) + forwarded, response, _ := forwardContentResult(result, p.currentTruncator(), nil, nil, serverName+":"+toolName, args) p.spotlightForwarded(serverName, toolName, contentTrust, forwarded) return forwardedText(forwarded, response) } @@ -197,7 +197,7 @@ func TestToonDetectionParity_OverLimit(t *testing.T) { t.Run("secret survives truncation", func(t *testing.T) { p := newToonProxy("off") - p.truncator = truncate.NewTruncator(1500) + p.setStaticTruncator(truncate.NewTruncator(1500)) payload := secretTable(40, 0, secret) // ~5KB, secret in the first row require.Greater(t, len(payload), 1500, "fixture must exceed the limit") mkResult := func() *mcp.CallToolResult { return toonTextResult(payload) } @@ -209,7 +209,7 @@ func TestToonDetectionParity_OverLimit(t *testing.T) { t.Run("secret truncated away on both paths", func(t *testing.T) { p := newToonProxy("off") - p.truncator = truncate.NewTruncator(1500) + p.setStaticTruncator(truncate.NewTruncator(1500)) payload := secretTable(40, 39, secret) // secret in the last row, past the budget require.Greater(t, len(payload), 1500, "fixture must exceed the limit") mkResult := func() *mcp.CallToolResult { return toonTextResult(payload) } diff --git a/internal/server/toon_encode.go b/internal/server/toon_encode.go index 10b5b709..6aecc323 100644 --- a/internal/server/toon_encode.go +++ b/internal/server/toon_encode.go @@ -69,9 +69,12 @@ func (p *MCPProxyServer) encodeToonBlocks(serverName, toolName, contentTrust str if pct < 1 || pct > 90 { pct = 15 // validated range is 1-90; 0/unset resolves to the default } + // Snapshot the live truncator once for this encode so budget derivation and + // detection-rendering use one consistent limit (#861). + truncator := p.currentTruncator() retainedBudget := 0 - if p.truncator != nil { - retainedBudget = p.truncator.SimpleTruncateBudget() + if truncator != nil { + retainedBudget = truncator.SimpleTruncateBudget() } // Spotlight framing must match spotlightForwarded's decision EXACTLY so @@ -104,8 +107,8 @@ func (p *MCPProxyServer) encodeToonBlocks(serverName, toolName, contentTrust str // real forwardContentResult), then spotlight, matching the order // of the off path (forward → spotlight). det := original - if p.truncator != nil && p.truncator.ShouldTruncate(det) { - det = p.truncator.Truncate(det, qualifiedTool, args).TruncatedContent + if truncator != nil && truncator.ShouldTruncate(det) { + det = truncator.Truncate(det, qualifiedTool, args).TruncatedContent } if spotlight { det = security.SpotlightUntrusted(det, serverName, toolName) diff --git a/internal/server/toon_encode_test.go b/internal/server/toon_encode_test.go index 9abec790..ab35e10c 100644 --- a/internal/server/toon_encode_test.go +++ b/internal/server/toon_encode_test.go @@ -251,7 +251,7 @@ func TestEncodeToonBlocks_DetectionTextSpotlight(t *testing.T) { func TestEncodeToonBlocks_DetectionTextTruncated(t *testing.T) { p := newToonProxy("adaptive") tr := truncate.NewTruncator(500) - p.truncator = tr + p.setStaticTruncator(tr) // Non-JSON so the truncator's deterministic simpleTruncate path runs both // here and in the seam (JSON-analyzable content mints a timestamped cache diff --git a/internal/server/toon_structured_content_test.go b/internal/server/toon_structured_content_test.go index 0131498b..340f869f 100644 --- a/internal/server/toon_structured_content_test.go +++ b/internal/server/toon_structured_content_test.go @@ -46,7 +46,7 @@ func TestToonStructuredContent_UnaffectedByEncoding(t *testing.T) { for _, mode := range []string{"adaptive", "always"} { t.Run(mode, func(t *testing.T) { p := newToonProxy(mode) - p.truncator = truncate.NewTruncator(0) + p.setStaticTruncator(truncate.NewTruncator(0)) result, structured := structuredTabularResult(t) _, decisions := p.encodeToonBlocks("srv", "tool", contracts.ContentTrustTrusted, nil, result) @@ -54,7 +54,7 @@ func TestToonStructuredContent_UnaffectedByEncoding(t *testing.T) { require.Equal(t, toonenc.OutcomeEncoded, decisions[0].Outcome, "fixture text must encode (envelope over a uniform array)") - forwarded, _, _ := forwardContentResult(result, p.truncator, nil, nil, "srv:tool", nil) + forwarded, _, _ := forwardContentResult(result, p.currentTruncator(), nil, nil, "srv:tool", nil) require.NotNil(t, forwarded) // The text block is TOON… @@ -82,11 +82,11 @@ func TestToonStructuredContent_ViolationStillDetected(t *testing.T) { verdicts := map[string]ovDecision{} for _, mode := range []string{"off", "always"} { p := newToonProxy(mode) - p.truncator = truncate.NewTruncator(0) + p.setStaticTruncator(truncate.NewTruncator(0)) result, _ := structuredTabularResult(t) p.encodeToonBlocks("srv", "tool", contracts.ContentTrustTrusted, nil, result) - forwarded, _, _ := forwardContentResult(result, p.truncator, nil, nil, "srv:tool", nil) + forwarded, _, _ := forwardContentResult(result, p.currentTruncator(), nil, nil, "srv:tool", nil) verdicts[mode] = evaluateOutputValidation(newTestValidator(), "srv:tool", violatedSchema, true, true, forwarded) } diff --git a/internal/server/toon_surface_isolation_test.go b/internal/server/toon_surface_isolation_test.go index 0849f551..666a1375 100644 --- a/internal/server/toon_surface_isolation_test.go +++ b/internal/server/toon_surface_isolation_test.go @@ -102,7 +102,7 @@ func TestSurfaceIsolation_CodeExecution(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { cm.Close() }) - proxy := NewMCPProxyServer(sm, idx, um, cm, truncate.NewTruncator(0), logger, nil, false, cfg, nil) + proxy := NewMCPProxyServer(sm, idx, um, cm, func() *truncate.Truncator { return truncate.NewTruncator(0) }, logger, nil, false, cfg, nil) t.Cleanup(func() { proxy.Close() }) calls := installToonEncodeRecorder(t) diff --git a/internal/server/toon_truncation_order_test.go b/internal/server/toon_truncation_order_test.go index 54115b78..b80d4c92 100644 --- a/internal/server/toon_truncation_order_test.go +++ b/internal/server/toon_truncation_order_test.go @@ -26,7 +26,7 @@ import ( // encodeToonBlocks → forwardContentResult (mcp.go ~2124-2129). func runSeamThenForward(p *MCPProxyServer, result *mcp.CallToolResult) (decisions []toonenc.Decision, finalText string, wasTruncated bool) { _, decisions = p.encodeToonBlocks("srv", "tool", contracts.ContentTrustTrusted, nil, result) - forwarded, response, wasTruncated := forwardContentResult(result, p.truncator, nil, nil, "srv:tool", nil) + forwarded, response, wasTruncated := forwardContentResult(result, p.currentTruncator(), nil, nil, "srv:tool", nil) _ = forwarded return decisions, response, wasTruncated } @@ -43,7 +43,7 @@ func TestToonEncodeThenTruncate_MarkerAndNoticeSurvive(t *testing.T) { for _, mode := range []string{"adaptive", "always"} { t.Run(mode, func(t *testing.T) { p := newToonProxy(mode) - p.truncator = truncate.NewTruncator(limit) + p.setStaticTruncator(truncate.NewTruncator(limit)) result := toonTextResult(original) decisions, finalText, wasTruncated := runSeamThenForward(p, result) @@ -90,7 +90,7 @@ func TestToonEncodeThenTruncate_BudgetBoundary(t *testing.T) { for _, mode := range []string{"adaptive", "always"} { t.Run(mode+"/just below guard passes through", func(t *testing.T) { p := newToonProxy(mode) - p.truncator = truncate.NewTruncator(limitBelow) + p.setStaticTruncator(truncate.NewTruncator(limitBelow)) result := toonTextResult(original) decisions, finalText, _ := runSeamThenForward(p, result) @@ -102,7 +102,7 @@ func TestToonEncodeThenTruncate_BudgetBoundary(t *testing.T) { "passthrough must carry no marker") // Truncation then behaves exactly as today, on the original JSON. offP := newToonProxy("off") - offP.truncator = truncate.NewTruncator(limitBelow) + offP.setStaticTruncator(truncate.NewTruncator(limitBelow)) offResult := toonTextResult(original) _, offText, offTruncated := runSeamThenForward(offP, offResult) assert.True(t, offTruncated) @@ -114,7 +114,7 @@ func TestToonEncodeThenTruncate_BudgetBoundary(t *testing.T) { t.Run(mode+"/at guard encodes", func(t *testing.T) { p := newToonProxy(mode) - p.truncator = truncate.NewTruncator(limitAt) + p.setStaticTruncator(truncate.NewTruncator(limitAt)) result := toonTextResult(original) decisions, finalText, _ := runSeamThenForward(p, result) @@ -132,7 +132,7 @@ func TestToonEncodeThenTruncate_BudgetBoundary(t *testing.T) { // untouched. func TestToonEncodeThenTruncate_UnlimitedNoTruncation(t *testing.T) { p := newToonProxy("adaptive") - p.truncator = truncate.NewTruncator(0) + p.setStaticTruncator(truncate.NewTruncator(0)) result := toonTextResult(tabularJSON(100)) decisions, finalText, wasTruncated := runSeamThenForward(p, result) @@ -148,7 +148,7 @@ func TestToonEncodeThenTruncate_UnlimitedNoTruncation(t *testing.T) { // means unlimited — the seam passes retainedBudget 0 and encoding proceeds. func TestToonEncodeThenTruncate_NilTruncator(t *testing.T) { p := newToonProxy("adaptive") - p.truncator = nil + p.setStaticTruncator(nil) result := toonTextResult(tabularJSON(100)) _, decisions := p.encodeToonBlocks("srv", "tool", contracts.ContentTrustTrusted, nil, result) diff --git a/internal/server/truncator_reload_test.go b/internal/server/truncator_reload_test.go new file mode 100644 index 00000000..777bf58a --- /dev/null +++ b/internal/server/truncator_reload_test.go @@ -0,0 +1,82 @@ +package server + +import ( + "sync/atomic" + "testing" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cache" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/index" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/truncate" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// setStaticTruncator wires a fixed truncator for tests that exercise the +// serving path with a specific limit. The value is captured eagerly so later +// reassignment of the source expression does not affect it. +func (p *MCPProxyServer) setStaticTruncator(tr *truncate.Truncator) { + p.truncatorFn = func() *truncate.Truncator { return tr } +} + +// TestTruncatorHotReloadOnServingPath (#861 defect 2): the MCP request handler +// must resolve the truncator through the injected getter at use time, not +// capture a pointer once at construction. When a config reload swaps the +// runtime truncator, the handler's next tool call must observe the new limit. +// +// With the old captured-pointer wiring, currentTruncator() would keep returning +// the truncator handed to the constructor even after the runtime swapped it, so +// the pre-swap assertion below would still hold after the swap and this test +// would fail. +func TestTruncatorHotReloadOnServingPath(t *testing.T) { + tmpDir := t.TempDir() + logger := zap.NewNop() + + cfg := config.DefaultConfig() + cfg.DataDir = tmpDir + cfg.Servers = []*config.ServerConfig{} + + sm, err := storage.NewManager(tmpDir, logger.Sugar()) + require.NoError(t, err) + t.Cleanup(func() { sm.Close() }) + + idx, err := index.NewManager(tmpDir, logger) + require.NoError(t, err) + t.Cleanup(func() { idx.Close() }) + + um := upstream.NewManager(logger, cfg, sm.GetBoltDB(), secret.NewResolver(), sm) + + cm, err := cache.NewManager(sm.GetDB(), logger) + require.NoError(t, err) + t.Cleanup(func() { cm.Close() }) + + // Live truncator holder, swapped to simulate a config reload. The handler + // must read through the getter so it observes the swap. + var live atomic.Pointer[truncate.Truncator] + live.Store(truncate.NewTruncator(100)) // limit 100 + + proxy := NewMCPProxyServer(sm, idx, um, cm, live.Load, logger, nil, false, cfg, nil) + t.Cleanup(func() { proxy.Close() }) + + // An 11-char payload: under limit 100 it should NOT truncate. + payload := "12345678901" + require.Len(t, payload, 11) + + tr := proxy.currentTruncator() + require.NotNil(t, tr) + assert.False(t, tr.ShouldTruncate(payload), "limit 100 must not truncate an 11-char payload") + + // Simulate a hot reload lowering the limit to 10. + live.Store(truncate.NewTruncator(10)) + + tr = proxy.currentTruncator() + require.NotNil(t, tr) + assert.True(t, tr.ShouldTruncate(payload), + "after reload to limit 10 the serving path must observe the new limit") +} diff --git a/internal/server/upstream_test.go b/internal/server/upstream_test.go index c0f81b5b..a127c5e9 100644 --- a/internal/server/upstream_test.go +++ b/internal/server/upstream_test.go @@ -65,7 +65,7 @@ func TestUpstreamServersHandlerPerformance(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, zap.NewNop(), nil, // mainServer not needed for this test false, @@ -161,7 +161,7 @@ func TestUpstreamServersListOperation(t *testing.T) { indexManager, upstreamManager, cacheManager, - truncator, + func() *truncate.Truncator { return truncator }, zap.NewNop(), nil, // mainServer not needed for this test false,