Repository navigation
perf(runtime): make the capture lock per server - #1813
Merged
Dumbris merged 3 commits intoNov 2, 2026
Merged
Conversation
captureSnapMu makes a server's "approval records persisted + pruned + capture stamp published" one unit for GetServerReview. It was a single runtime-wide mutex, held for the whole of a discovery pass's apply, including the TPA scans, approval record writes and index writes. A server with a large or changing tool set therefore blocked discovery commits of every other server and the review API of every server for as long as its pass ran. Every holder and reader works on one server, so the mutex is replaced by one RWMutex per server (captureSnapLock), stored like the per-server approval locks. No path held captureSnapMu for one server while taking it for another (that would have self-deadlocked), so splitting it adds no lock-order edges.
poolski
marked this pull request as ready for review
November 2, 2026 09:36
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…fe under per-server capture locks The runtime-wide capture lock serialized every server's discovery pass, so the TPA shadowing/clone check of a later pass always saw the earlier pass's tools in the StateView. With per-server locks two passes overlap and each publishes only at its end, so a same-name near-duplicate added on two servers at once scanned clean on both. A pass now registers its inventory as peer context before its approval scan and unregisters it when the apply returns (after its publish); collectPeerToolMetadata overlays other servers' in-flight inventories on the StateView snapshot, so the later-scanning pass holds the clone again. reconcileSignatureCache enumerates the index and then evicts every hash it did not see. A concurrent pass of another server could index and warm a hash in between and lose it until its next pass. sigReconcileMu now covers reconcile from enumeration through RetainHashes and every warm, so a warm lands either before the enumeration (its tool already indexed) or after the eviction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…p published contracts in the overlay collectPeerToolMetadata copied the in-flight registrations after taking the StateView snapshot, so a peer that published and unregistered between the two reads was seen by neither. The in-flight copy is taken first: a pass unregisters only after it published, so what the copy misses is in the snapshot taken next. The overlay replaced a published tool with an in-flight tool of the same raw name. An in-flight replacement is not yet published and its pass may fail, so the published contract is still what a clone would shadow. The overlay is now a union; only an identical published contract is dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Dumbris
enabled auto-merge (squash)
November 2, 2026 17:50
Member
|
Thanks for this — the per-server split is the right shape and the lock-order argument holds. Review (own read + cross-model round with gpt-6.1-sol, each finding verified against the code) found two guarantees the old global lock gave incidentally, so I pushed two commits on top of yours rather than send it back:
|
Member
|
Merged. Thank you, @poolski! One server's long discovery pass no longer stalls every other server's commit and the review API, and the per-server lock keeps the records-plus-stamp consistency that Contributions like this are what keep MCPProxy moving, and they matter a lot to the project. One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it. |
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
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.
captureSnapMumakes a server's "approval records persisted + pruned + capture stamp published" one unit forGetServerReview. It was one runtime-wide mutex held for a discovery pass's whole apply (TPA scans, approval record writes, index writes). A server with a large or changing tool set blocked every other server's discovery commits and every server's review API until its pass finished.Every holder and reader works on one server, so it becomes one
sync.RWMutexper server (captureSnapLock), stored like the per-server approval locks. No path held the lock for one server while taking it for another (that would have self-deadlocked), so the split adds no lock-order edges. Comments that namedcaptureSnapMunow namecaptureSnapLock.TestCaptureOfOneServerDoesNotBlockAnotherpauses a capture of one server partway through and requires a capture and review of another server to complete. The existing review/capture consistency tests (review_snapshot_race_test.go) cover the per-server guarantee.Testing
go test -race ./internal/runtime/...scripts/test-api-e2e.sh(70/70)golangci-lintwith.github/.golangci.yml, bare and--build-tags server: no new issues