fix(runtime): guard Truncator swap/read and un-capture stale truncator pointer (#861)#887
Merged
Merged
Conversation
Deploying mcpproxy-docs with
|
| Latest commit: |
41b274e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6220aa89.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-861-truncator-race.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 29899225325 --repo smart-mcp-proxy/mcpproxy-go
|
…r 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
Dumbris
force-pushed
the
fix/861-truncator-race
branch
from
July 22, 2026 08:54
abf9e7b to
41b274e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved: Codex review LGTM; rebased on #888 preserving both intents; API E2E pass at this SHA. Arming auto-merge.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two live defects from #861 (surfaced by Codex review of PR #857). Scope is deliberately limited to these two — the broader
ApplyConfig/ReloadConfigurationside-effect unification stays tracked in #861.Defects fixed
Data race on the truncator field.
Runtime.Truncator()readr.truncatorwithout holdingr.muwhile the config-apply path swapped it under the lock — a real-race-detectable race. The field is nowatomic.Pointer[truncate.Truncator]; the accessorLoad()s and the swapStore()s. Reads stay lock-free on the serving hot path and are independent ofr.mu.Stale captured truncator pointer. The MCP request handler captured the truncator pointer once at construction, so a hot-reloaded
tool_response_limitnever reached live tool-call responses.NewMCPProxyServernow takes afunc() *truncate.Truncatorgetter and the serving path resolves the truncator at use time viacurrentTruncator(). Production passesrt.Truncator(method value), so a swap takes effect on the next call.Composition with #857
PR #857 adds
configCommitMuto serialize the config commit paths (expected to land as aconfigCommitMufield oninternal/runtime/runtime.go). This change composes with that: the truncator swap uses anatomic.PointerStore, independent of whichever mutex serializes the commit, and the accessor is a lock-freeLoad— no lock ordering to reconcile. Changes are kept minimal and localized to reduce merge-conflict risk.Tests (TDD, failing-first)
TestTruncatorSwapRace(internal/runtime): concurrentTruncator()reads whileApplyConfigswaps the truncator, under-race.TestTruncatorHotReloadOnServingPath(internal/server): atool_response_limitchange is observed by the serving path after reload (would fail with the old captured-pointer wiring).Verification
go build ./cmd/mcpproxyandgo build -tags server ./cmd/mcpproxy— passgo test -race ./internal/runtime/ ./internal/server/ ./internal/truncate/— passgolangci-lint run --config .github/.golangci.ymlon affected dirs — 0 issuesRefs #861 (partial — two live defects only) · follows #857