fix(bridge): split NotificationCache out of Translator lock#224
Merged
Conversation
diagnostics_pump wrote incoming notifications into the cache through Arc<Mutex<Translator>>, the same lock held by every MCP tool call. While a slow textDocument/diagnostic round-trip held that lock, the pump blocked trying to cache incoming publishDiagnostics/log/message notifications, and the LSP transport's non-blocking try_send silently dropped them once the notification channel filled. Cache-only reads (get_cached_diagnostics, read_resource, subscribe) also locked the translator purely to validate a path against immutable workspace roots, so they queued behind the same in-flight round-trip. Extract NotificationCache into its own Arc<Mutex<NotificationCache>>, independent of Translator, and validate workspace-relative paths against a lock-free Arc<[PathBuf]> snapshot instead of the translator lock. get_diagnostics keeps holding the translator lock across its LSP round-trip; nothing else needs to anymore. Closes #104
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR improves mcpls’s bridge concurrency by decoupling notification caching and workspace-root validation from the global Arc<Mutex<Translator>>, preventing LSP push notifications and cache-only MCP operations from being stalled behind slow in-flight LSP round-trips.
Changes:
- Split
NotificationCacheout ofTranslatorinto an independentArc<Mutex<NotificationCache>>, and updateddiagnostics_pump+ MCP handlers to use it directly. - Introduced lock-free workspace-root validation via an
Arc<[PathBuf]>snapshot andvalidate_path_against_roots, used by cache-only MCP resource flows. - Updated constructors/tests and documented the behavior and API changes in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mcpls-core/src/transport.rs | Updates HTTP transport tests to construct McplsServer with the new cache + workspace-roots snapshot args. |
| crates/mcpls-core/src/mcp/server.rs | Switches cache-only tools/resources to use notification_cache + lock-free root validation instead of locking the translator. |
| crates/mcpls-core/src/mcp/handlers.rs | Extends HandlerContext to carry notification_cache and immutable workspace_roots snapshot. |
| crates/mcpls-core/src/lib.rs | Makes diagnostics_pump lock only NotificationCache; wires cache + roots snapshot through startup and adds a concurrency regression test. |
| crates/mcpls-core/src/bridge/translator.rs | Removes embedded NotificationCache; adds validate_path_against_roots; refactors cached/log/message handlers to operate on cache inputs. |
| crates/mcpls-core/src/bridge/mod.rs | Re-exports validate_path_against_roots for crate-internal use. |
| CHANGELOG.md | Documents the contention fix and breaking signature changes under Unreleased. |
…ck split get_cached_diagnostics still held the notification_cache mutex across canonicalize() and the cached-diagnostics-to-MCP-shape mapping, reintroducing the same lock-held-during-syscall class this branch exists to eliminate. Split Translator::handle_cached_diagnostics into cached_diagnostics_uri (path validation, no lock) and diagnostics_from_cache_entry (pure mapping over an already-cloned entry, no lock); the cache lock is now held only for the map lookup and clone. read_resource discarded the canonicalized path returned by validate_path_against_roots and built its cache-lookup URI from the raw input path, so paths with symlinks or `..` segments missed the cache even when tracked under their canonical form. Use the validated path for the URI, matching get_cached_diagnostics.
bug-ops
enabled auto-merge (squash)
July 24, 2026 22:50
…`-based `path_to_uri` re-parses the file URI through `url::Url::parse` for RFC 3986 character encoding, which normalizes away `..` segments on every platform. A path differing from its canonical form only by literal `..` segments therefore produces the same URI with or without the read_resource fix, regardless of OS. The test only passed on macOS because `/tmp` there is itself a symlink to `/private/tmp`, which `canonicalize()` resolves -- an effect unrelated to what the test claimed to prove. On Linux CI, `/tmp` is a real directory, so the assertion failed. Rewrite the test using a real symlinked directory so canonicalize() resolves something URI string normalization cannot, producing a deterministic raw-vs-canonical difference on any platform where symlinks work. Unix-only, since symlink creation on Windows CI runners typically requires elevated privileges.
bug-ops
added a commit
that referenced
this pull request
Jul 24, 2026
PR #224 (issue #104) extracted NotificationCache out of Translator and introduced a lock-free workspace_roots snapshot after this branch was created. Update subscribe() to read the cache via self.context.notification_cache directly instead of the now-removed Translator::notification_cache(), and build the diagnostics-cache URI from the canonicalized path returned by validate_path_against_roots, matching the pattern already established in read_resource.
bug-ops
added a commit
that referenced
this pull request
Jul 24, 2026
…ibe (#225) * test(rust-analyzer): fix off-by-one column positions in hover, definition, and completion tests test_hover_on_u64_type passed a column past the end of its target line, so rust-analyzer always returned a null hover. test_definition_user_struct and test_completions_basic had the same class of error, surviving only because rust-analyzer's token resolution happened to tolerate the off-by-one. test_completions_basic also asserted nothing on success, so it could never fail regardless of the result. Fixes #138 * fix(bridge): replay cached diagnostics on subscribe A client subscribing to an lsp-diagnostics:// URI after the LSP server already pushed its first publishDiagnostics notification for that file saw nothing until the next push. subscribe() now checks the notification cache and immediately replays a resources/updated notification when diagnostics are already cached for the URI. The subscription is recorded before the cache is checked, closing a race with diagnostics_pump: whichever of the two observes the update first delivers it, so at most a harmless duplicate notification can occur, never a dropped one. The e2e test client read exactly one line per request, so it misread the new out-of-order notification as the RPC response. It now matches responses by id and queues notifications separately, which is required JSON-RPC/MCP behavior regardless of this fix. Fixes #131 * fix(bridge): log failed diagnostics-replay notifications instead of discarding subscribe() silently dropped errors from the replay notification, unlike diagnostics_pump which treats the same failure as peer disconnect. Log at warn level for debuggability; do not fail the subscribe call itself, since the subscription already succeeded and only the initial replay was affected. Addresses review feedback on #131 * fix(bridge): adapt subscribe() to the split notification cache from #224 PR #224 (issue #104) extracted NotificationCache out of Translator and introduced a lock-free workspace_roots snapshot after this branch was created. Update subscribe() to read the cache via self.context.notification_cache directly instead of the now-removed Translator::notification_cache(), and build the diagnostics-cache URI from the canonicalized path returned by validate_path_against_roots, matching the pattern already established in read_resource. * fix(bridge): canonicalize subscribe/unsubscribe resource URIs subscribe() tracked the client's raw request.uri, but diagnostics_pump matches subscriptions against a URI rebuilt from the canonical LSP path. A client subscribing with a non-canonical but valid URI (symlink, macOS /var vs /private/var) would be recorded under a key that never matches subs.contains(), so it would receive the initial replay but silently stop getting further diagnostics pushes. Both subscribe and the replay notification now use the canonical URI derived from validated_path. unsubscribe mirrors the same canonicalization to remove the matching entry, falling back to the raw URI if the file no longer exists (e.g. deleted since subscribing) so unsubscribing a stale entry never errors. Addresses further review feedback on #131
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.
Summary
diagnostics_pumpcached incoming LSP notifications throughArc<Mutex<Translator>>, the same lock held by every MCP tool call. While a slowtextDocument/diagnosticround-trip (get_diagnostics) held that lock, the pump blocked trying to acquire it to cache an incomingpublishDiagnostics/log/message notification. The LSP transport forwards notifications via a non-blockingtry_send, so once the bounded channel filled, those notifications were silently dropped rather than queued. Separately,get_cached_diagnostics,read_resource, andsubscribelocked the translator purely to validate a path against workspace roots (immutable after startup), so those cache-only reads also queued behind the same in-flight round-trip.NotificationCacheinto its ownArc<Mutex<NotificationCache>>, independent ofTranslator.diagnostics_pumpnow only locks the cache.Arc<[PathBuf]>snapshot (validate_path_against_roots), used byget_cached_diagnostics,read_resource, andsubscribeinstead of locking the translator.get_diagnosticsis unchanged — it's the one path that legitimately needs the translator lock across the LSP round-trip; that broader lock-granularity problem is tracked separately in perf(bridge): reduce Mutex<Translator> hold time — release lock before client.request() #108 and research(bridge): upgrade Arc<Mutex<Translator>> to Arc<RwLock<Translator>> after #104 and #108 land #114.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features(432/432 passing, including a new concurrency regression test proving the pump makes progress while the translator lock is held elsewhere)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featuresCloses #104