fix(lsp): abort drain when a live server never replies#145
Conversation
A server that starts but never answers definition requests (e.g. Volar 3 on a Vue 2 project) burned the full 10s batch deadline per file — hours on a large repo — because only process death aborted the drain. Track consecutive all-timeout request windows (each = one 10s deadline) across files and abandon the language after three (~30s): warn, fall back to heuristics, and keep the edges already resolved. Introduce `stopped_early` distinct from `server_died`: a mute stop keeps the answers drained from responsive files (external/unresolvable get committed), whereas a death distrusts them all. Classify a request timeout precisely (`is_request_timeout`) so a disconnect or LSP-error reply is not miscounted as responsive, and sort files so the abort decision is deterministic run-to-run. Measured on a 2,119-file monorepo with a mute vue server: the vue drain aborts after ~30s instead of ~2.2h, with identical js/ts/tsx yields.
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a shared timeout-error prefix and ChangesLSP unresponsive server handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Resolver as drain_language
participant LspClient
participant Outcomes as LangOutcomes
Resolver->>LspClient: send definition batch window
LspClient-->>Resolver: is_request_timeout = true (repeated)
Resolver->>Resolver: increment consecutive_timeout_windows
alt threshold reached
Resolver->>Outcomes: set stopped_early = true
Resolver->>Outcomes: commit drained outcomes so far
else replies received
Resolver->>Outcomes: reset consecutive_timeout_windows
end
Related issues: None specified. Related PRs: None specified. Suggested labels: lsp, documentation, tests Suggested reviewers: jrollin 🐰 A server gone mute, no reply in sight, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RUSTSEC-2026-0194/0195 landed against quick-xml, a transitive dep via self_update (0.37) and rust-s3/aws-creds (0.38). The fix is quick-xml >=0.41, but both dependents pin <0.41, so no upgrade or [patch] applies — CI's audit + deny gates go red on every PR and on main until then. Ignore the two IDs in deny.toml and .cargo/audit.toml (kept in sync) with a dated rationale. Exposure is narrow: XML parsed is GitHub release metadata and S3 responses, not attacker-controlled input. Remove once self_update/rust-s3 pull quick-xml >=0.41.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 88.03% 88.31% +0.28%
==========================================
Files 96 96
Lines 32200 32307 +107
==========================================
+ Hits 28347 28533 +186
+ Misses 3853 3774 -79 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cartog-lsp/src/client.rs (1)
257-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[must_use]and a co-located unit test foris_request_timeout.
is_request_timeoutis a pure predicate, so its return value shouldn’t be dropped. Add a direct#[cfg(test)]case incrates/cartog-lsp/src/client.rs; the module’s current tests cover other helpers, but not this one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cartog-lsp/src/client.rs` around lines 257 - 268, Add #[must_use] to the is_request_timeout predicate so callers don’t accidentally ignore its result, and add a co-located #[cfg(test)] unit test in client.rs that exercises is_request_timeout directly. Use the existing REQUEST_TIMEOUT_PREFIX and is_request_timeout symbols to create one case that matches the timeout prefix and one that does not, keeping the test in the same module as the helper.Source: Coding guidelines
crates/cartog-lsp/src/manager.rs (1)
640-645: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
insert_client_for_testhardcodeslanguage_idto"python"regardless of thelanguageargument.Currently every caller happens to use
"python", so it's latent, but a future test callinginsert_client_for_test("ruby", ...)(or any non-python language) would silently mislabel the storedlanguage_id, potentially causing confusing failures ifdidOpen/languageId content ever matters for that test.♻️ Proposed fix
- pub(crate) fn insert_client_for_test(&mut self, language: &str, client: LspClient) { - self.clients - .insert(language.to_string(), (client, "python")); + pub(crate) fn insert_client_for_test( + &mut self, + language: &str, + language_id: &'static str, + client: LspClient, + ) { + self.clients + .insert(language.to_string(), (client, language_id)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cartog-lsp/src/manager.rs` around lines 640 - 645, `insert_client_for_test` in `LspManager` hardcodes the stored `language_id` to `"python"` instead of using the `language` argument, so update that test helper to persist the provided language value alongside the client. Keep the change localized to `insert_client_for_test` and ensure the inserted tuple uses the same language string passed by the caller so future tests with non-Python languages are stored correctly.crates/cartog-lsp/src/resolve.rs (1)
296-362: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCore windowing/mute-detection logic is correct.
Traced through all-timeout windows, mixed windows, mid-window
server_died, and cross-file counter persistence — theconsecutive_timeout_windowsbookkeeping, break ordering (server_diedchecked before the window-limit check), andclose_filecleanup on every exit path all look sound. Nice touch keeping drainedin_root/pending_external/pending_unresolvableintact on early stop.One process point: this is squarely the hot path referenced by the existing
~33% faster edge resolutionbenchmark note inconcurrency.md. Per coding guidelines, changes to an indexing/query hot path should add or re-run the matching benchmark — worth capturing updated numbers (or at least a mute-server-specific benchmark) now that the worst case is bounded toUNRESPONSIVE_WINDOW_LIMITwindows instead of the full edge set.As per coding guidelines, "Performance-critical code must avoid per-row or per-edge queries inside loops, avoid quadratic index/resolve behavior, prefer borrowed data over cloning in hot paths, and add or re-run the matching benchmark when changing an indexing or query hot path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cartog-lsp/src/resolve.rs` around lines 296 - 362, This change touches a hot indexing/query path in resolve.rs, so you should add or re-run the relevant performance benchmark and record the updated numbers. Use the definition resolution loop around the windowed timeout handling in resolve.rs as the target, and verify the existing benchmark tied to the edge-resolution performance note (or add a mute-server-specific benchmark if needed) after the UNRESPONSIVE_WINDOW_LIMIT behavior change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cartog-lsp/src/client.rs`:
- Around line 257-268: Add #[must_use] to the is_request_timeout predicate so
callers don’t accidentally ignore its result, and add a co-located #[cfg(test)]
unit test in client.rs that exercises is_request_timeout directly. Use the
existing REQUEST_TIMEOUT_PREFIX and is_request_timeout symbols to create one
case that matches the timeout prefix and one that does not, keeping the test in
the same module as the helper.
In `@crates/cartog-lsp/src/manager.rs`:
- Around line 640-645: `insert_client_for_test` in `LspManager` hardcodes the
stored `language_id` to `"python"` instead of using the `language` argument, so
update that test helper to persist the provided language value alongside the
client. Keep the change localized to `insert_client_for_test` and ensure the
inserted tuple uses the same language string passed by the caller so future
tests with non-Python languages are stored correctly.
In `@crates/cartog-lsp/src/resolve.rs`:
- Around line 296-362: This change touches a hot indexing/query path in
resolve.rs, so you should add or re-run the relevant performance benchmark and
record the updated numbers. Use the definition resolution loop around the
windowed timeout handling in resolve.rs as the target, and verify the existing
benchmark tied to the edge-resolution performance note (or add a
mute-server-specific benchmark if needed) after the UNRESPONSIVE_WINDOW_LIMIT
behavior change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0376ee2-9baf-491a-99ce-2e52dad8b933
📒 Files selected for processing (7)
.gitignorecrates/cartog-lsp/src/client.rscrates/cartog-lsp/src/manager.rscrates/cartog-lsp/src/resolve.rsdocs/explanation/architecture.mddocs/explanation/concurrency.mddocs/troubleshooting.md
1 participant
Problem
During
cartog index's LSP edge-resolution phase, a language server that starts but never answers definition requests (e.g. Volar 3 pointed at a Vue 2 project it can't parse) burned the full 10s batch deadline on every file. On a large repo that is hours of pure timeout — only genuine process death aborted the drain, and a mute-but-alive server staysis_alive() == true.Observed on a 2,119-file React+Vue monorepo: the Vue drain (805
.vuefiles) would hang for ~2.2h and had to be killed by hand.Fix
UNRESPONSIVE_WINDOW_LIMIT = 3(~30s). Counting windows, not files, keeps the bound at ~30s regardless of how unresolved edges distribute (one huge file is many windows; three tiny files are three windows). Logs a WARN and falls back to heuristics for that language, keeping edges already resolved.stopped_earlyvsserver_died. A mute stop is not a death: the answers drained from responsive files are trustworthy, soapply_lang_outcomesstill commits theirexternal/unresolvablemarkings. A process death continues to distrust everything. Reusingserver_diedfor the mute case would have silently discarded a responsive prefix's markings and re-queried them on every future index.client::is_request_timeout, prefix-matched likeis_cancelled) so a disconnect or an LSP-error reply is not miscounted as "responsive."by_fileis sorted, so the decision is identical run-to-run (it was aHashMap).Verification
mute_server_stops_early_after_consecutive_timeout_windows,replying_server_is_never_flagged_unresponsive,stopped_early_still_commits_drained_answers. Test fakes moved to a sharedmanager::test_supportmodule;scripted_fake_serverhardened against shell-quoting.any_server_startedconsumer), clippy-D warningsclean on default and--no-default-features, fmt clean.Docs
Behavior documented in the three surfaces that own LSP failure modes:
docs/troubleshooting.md("LSP server looks stuck"),docs/explanation/concurrency.md, anddocs/explanation/architecture.md. No new command/flag/config-key/MCP-tool/language, so site/cli.md/config.md/README/SKILL unchanged.Deferred (separate PR)
A fully-wedged server that stops reading stdin can block
didOpen'swrite_allforever (no write deadline). Pre-existing, in the same feature area, but the correct fix is a client write-path redesign (writer thread + bounded channel) that shouldn't ride on this diff.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation