From 8fa67596467940bccf0f237ae51c0eeb27fceecd Mon Sep 17 00:00:00 2001 From: arimu1 <19286898+arimu1@users.noreply.github.com> Date: Tue, 21 Jul 2026 10:08:20 +0700 Subject: [PATCH] Gate ContentModified retry on declared retryOnContentModified methods Review on #1728 found two blocking issues in the previous commit: 1. pyrefly_server.py wraps send_request with its own 5x/0.2s retry for RequestCancelled/ContentModified. The base-class retry nested inside it, so a persistent ContentModified produced 5x5=25 round-trips. 2. The base-class retry applied to every request on every server, but general.staleRequestSupport.retryOnContentModified in InitializeParams is the client's contract with the server about exactly which requests it will reissue. rust-analyzer declares only the three semanticTokens methods and haskell-language-server declares none; blanket-retrying e.g. textDocument/hover (or workspace/executeCommand, which is not idempotent) contradicts that declared capability. Fix: send_request now only retries a method if it was registered via the new LanguageServerInterface.set_content_modified_retry_methods, which SolidLanguageServer._create_initialize_params populates from each server's own general.staleRequestSupport.retryOnContentModified list right after building it. The mechanism is opt-in per method, per server, and mirrors what we actually told the server -- so it's a no-op wherever nothing is declared. rust-analyzer now declares textDocument/hover in that list (the method issue #1724's repro -- test_find_symbol[rust_add_function] with include_info=True -- actually needs), in addition to the pre-existing semanticTokens methods. This also resolves the pyrefly double-retry: pyrefly never declares general.staleRequestSupport, so the base-class retry is a no-op for every pyrefly request and pyrefly's own wrapper remains the sole handler for both RequestCancelled and ContentModified there, exactly as before this PR. pyrefly_server.py is intentionally left untouched; narrowing its retry tuple, as literally suggested, would have removed ContentModified handling for it entirely once the base layer stopped covering it too. Also: reduced _CONTENT_MODIFIED_MAX_ATTEMPTS from 5 to 3 (now the max per allowlisted method only, so worst-case added latency for hover is 2 * request_timeout + 0.4s instead of 4 * request_timeout + 0.8s), and restructured send_request around a _send_request_once helper so the final failed attempt raises directly instead of falling through to a trailing `raise AssertionError("unreachable")`. Retry tests now monkeypatch time.sleep and gained coverage for the allowlist gate (undeclared method, executeCommand, default-empty registration). Fixes #1724 --- CHANGELOG.md | 10 +- .../language_servers/rust_analyzer.py | 4 + src/solidlsp/ls.py | 15 ++- src/solidlsp/ls_process.py | 101 ++++++++++++------ test/solidlsp/test_content_modified_retry.py | 35 +++++- 5 files changed, 125 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63f411ac..fbda3e99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,12 +52,10 @@ Status of the `main` branch. Changes prior to the next official version change w extensions containing PHP sources (e.g. Drupal's `.module` / `.install` / `.inc` / `.theme`) become visible to the symbol tools and are indexed by the language server #1710 - Fix: an LSP `ContentModified` (-32801) response was surfaced as a hard `SolidLSPException` - instead of being retried. Per the LSP spec, `ContentModified` means the server discarded a - stale, in-flight computation because the workspace changed underneath it, not that the request - itself is invalid; well-behaved clients are expected to retry. `LanguageServerInterface.send_request` - now retries such requests a bounded number of times before giving up, for all language servers - (this was flaking `test_find_symbol[rust_add_function]` on windows-latest, where rust-analyzer - returns `ContentModified` for hover requests cancelled by concurrent indexing). #1724 + instead of being retried, as the spec expects for requests a client declared it will reissue. + `send_request` now retries such responses for methods declared via a server's + `retryOnContentModified` capability; rust-analyzer declares `textDocument/hover`, fixing the + flaky `test_find_symbol[rust_add_function]` on windows-latest. #1724 * JetBrains: - Allow external files from dependencies (specified via references like "") to be diff --git a/src/solidlsp/language_servers/rust_analyzer.py b/src/solidlsp/language_servers/rust_analyzer.py index 2c01ed87..d7cd97fc 100644 --- a/src/solidlsp/language_servers/rust_analyzer.py +++ b/src/solidlsp/language_servers/rust_analyzer.py @@ -413,6 +413,10 @@ class RustAnalyzer(SolidLanguageServer): "textDocument/semanticTokens/full", "textDocument/semanticTokens/range", "textDocument/semanticTokens/full/delta", + # rust-analyzer's internal cancellation tracking does not cover hover, + # so hover requests it cancels while indexing come back ContentModified; + # we retry them ourselves (see LanguageServerInterface.send_request). #1724 + "textDocument/hover", ], }, "regularExpressions": {"engine": "ECMAScript", "version": "ES2020"}, diff --git a/src/solidlsp/ls.py b/src/solidlsp/ls.py index 820d241d..4bdb0f00 100644 --- a/src/solidlsp/ls.py +++ b/src/solidlsp/ls.py @@ -3203,4 +3203,17 @@ class SolidLanguageServer(ABC): """ Create the InitializeParams object to send to the language server during initialization. """ - return self._create_initialize_params_builder().with_base_options(self._create_base_initialize_params()).build() + params = self._create_initialize_params_builder().with_base_options(self._create_base_initialize_params()).build() + self._register_content_modified_retry_methods(params) + return params + + def _register_content_modified_retry_methods(self, params: InitializeParams) -> None: + """ + Reads back `general.staleRequestSupport.retryOnContentModified` from the InitializeParams + we are about to send and registers it with the underlying `LanguageServerInterface`, so + that `send_request` only retries `ContentModified` (-32801) responses for methods we have + actually told the server we will retry (see `LanguageServerInterface.send_request`). + """ + general_capabilities = cast(dict, params.get("capabilities", {})).get("general", {}) + retry_methods = general_capabilities.get("staleRequestSupport", {}).get("retryOnContentModified", []) + self.server.set_content_modified_retry_methods(retry_methods) diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index a7b97cde..b7f1a3d8 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -7,7 +7,7 @@ import subprocess import threading import time from abc import ABC, abstractmethod -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass from queue import Empty, Queue from typing import IO, Any, AnyStr, cast @@ -38,9 +38,13 @@ log = logging.getLogger(__name__) # Per the LSP spec, `ContentModified` (-32801) means the server discarded a stale, in-flight # computation because the workspace changed underneath it, not that the request itself is -# invalid. A well-behaved client is expected to retry such requests rather than treat them as -# a hard failure, so we do so here, generically, for all requests to any language server. -_CONTENT_MODIFIED_MAX_ATTEMPTS = 5 +# invalid. A well-behaved client is expected to retry such requests -- but only requests it has +# declared, via `general.staleRequestSupport.retryOnContentModified` in its InitializeParams, that +# it will retry. We honor that contract: `send_request` only retries methods a given language +# server implementation has actually opted into via `set_content_modified_retry_methods` (see +# `SolidLanguageServer._create_initialize_params`), so this mechanism is a no-op for any server +# that hasn't declared such methods. +_CONTENT_MODIFIED_MAX_ATTEMPTS = 3 _CONTENT_MODIFIED_RETRY_DELAY = 0.2 @@ -162,12 +166,34 @@ class LanguageServerInterface(ABC): self._response_handlers_lock = threading.Lock() self._tasks_lock = threading.Lock() + self._content_modified_retry_methods: frozenset[str] = frozenset() + """ + The set of LSP method names for which `send_request` will retry a `ContentModified` + (-32801) response instead of raising immediately. Empty by default, i.e. no retries, + since retrying is only correct for methods this client has told the server (via + `general.staleRequestSupport.retryOnContentModified`) that it will reissue; see + `set_content_modified_retry_methods`. + """ + def set_request_timeout(self, timeout: float | None) -> None: """ :param timeout: the timeout, in seconds, for all requests sent to the language server. """ self._request_timeout = timeout + def set_content_modified_retry_methods(self, methods: Iterable[str]) -> None: + """ + Declares the LSP method names for which `send_request` should retry `ContentModified` + (-32801) responses. This should mirror whatever methods are declared in + `general.staleRequestSupport.retryOnContentModified` of the InitializeParams sent to the + server: that declaration is a promise to the server about which requests the client will + reissue on cancellation, so retrying anything outside of it would contradict what the + server was told. + + :param methods: LSP method names (e.g. ``"textDocument/hover"``) eligible for retry. + """ + self._content_modified_retry_methods = frozenset(methods) + @abstractmethod def is_running(self) -> bool: """ @@ -303,44 +329,55 @@ class LanguageServerInterface(ABC): request.on_error(exception) self._pending_requests.clear() + def _send_request_once(self, method: str, params: dict | None) -> Request.Result: + """ + Sends a single request to the server (no retries) and waits for its response. + """ + with self._request_id_lock: + request_id = self.request_id + self.request_id += 1 + + request = Request(request_id=request_id, method=method) + log.debug("Starting: %s", request) + + with self._response_handlers_lock: + self._pending_requests[request_id] = request + + self._send_payload(make_request(method, request_id, params)) + + log.debug("Waiting for response to request %s with params:\n%s", method, params) + result = request.get_result(timeout=self._request_timeout) + log.debug("Completed: %s", request) + return result + def send_request(self, method: str, params: dict | None = None) -> PayloadLike: """ Send request to the server, register the request id, and wait for the response. - Requests that fail with the LSP `ContentModified` error (-32801) are retried a bounded + If `method` is one of the methods registered via `set_content_modified_retry_methods`, + a response failing with the LSP `ContentModified` error (-32801) is retried a bounded number of times with a short delay instead of being surfaced to the caller immediately; - see `_CONTENT_MODIFIED_MAX_ATTEMPTS` above. + see `_CONTENT_MODIFIED_MAX_ATTEMPTS` above. Methods that were not registered are never + retried, regardless of the error, so as to not retry requests behind the server's back. """ - for attempt in range(1, _CONTENT_MODIFIED_MAX_ATTEMPTS + 1): - with self._request_id_lock: - request_id = self.request_id - self.request_id += 1 + result = self._send_request_once(method, params) + if not result.is_error(): + log.debug("Returning result:\n%s", result.payload) + return result.payload - request = Request(request_id=request_id, method=method) - log.debug("Starting: %s", request) - - with self._response_handlers_lock: - self._pending_requests[request_id] = request - - self._send_payload(make_request(method, request_id, params)) - - log.debug("Waiting for response to request %s with params:\n%s", method, params) - result = request.get_result(timeout=self._request_timeout) - log.debug("Completed: %s", request) - - if not result.is_error(): - log.debug("Returning result:\n%s", result.payload) - return result.payload - - is_content_modified = isinstance(result.error, LSPError) and result.error.code == LSPErrorCodes.ContentModified - if attempt < _CONTENT_MODIFIED_MAX_ATTEMPTS and is_content_modified: + if method in self._content_modified_retry_methods: + for attempt in range(2, _CONTENT_MODIFIED_MAX_ATTEMPTS + 1): + is_content_modified = isinstance(result.error, LSPError) and result.error.code == LSPErrorCodes.ContentModified + if not is_content_modified: + break log.info("Request %s got ContentModified (-32801); retrying (%d/%d)", method, attempt, _CONTENT_MODIFIED_MAX_ATTEMPTS) time.sleep(_CONTENT_MODIFIED_RETRY_DELAY) - continue + result = self._send_request_once(method, params) + if not result.is_error(): + log.debug("Returning result:\n%s", result.payload) + return result.payload - raise SolidLSPException(f"Error processing request {method} with params:\n{params}", cause=result.error) from result.error - - raise AssertionError("unreachable") # loop always returns or raises + raise SolidLSPException(f"Error processing request {method} with params:\n{params}", cause=result.error) from result.error @abstractmethod def _send_payload(self, payload: StringDict) -> None: diff --git a/test/solidlsp/test_content_modified_retry.py b/test/solidlsp/test_content_modified_retry.py index 27e05d10..b36669bb 100644 --- a/test/solidlsp/test_content_modified_retry.py +++ b/test/solidlsp/test_content_modified_retry.py @@ -6,6 +6,11 @@ invalid -- clients are expected to retry. This is what caused issue #1724 (flaky ``test_find_symbol[rust_add_function]`` on windows-latest): rust-analyzer returns ``ContentModified`` for cancelled hover requests, and Serena surfaced it as a hard error. +Retrying is only spec-compliant for methods the client actually declared, via +``general.staleRequestSupport.retryOnContentModified`` in its InitializeParams, that it will +retry -- so ``send_request`` only retries methods registered with it through +``set_content_modified_retry_methods`` (see ``SolidLanguageServer._create_initialize_params``). + No language markers: these use a local test double and run in catch-all. """ @@ -23,15 +28,22 @@ from solidlsp.lsp_protocol_handler.lsp_types import LSPErrorCodes from solidlsp.lsp_protocol_handler.server import LSPError +@pytest.fixture(autouse=True) +def _no_real_sleep(monkeypatch: pytest.MonkeyPatch) -> None: + """The retry delay is only there to be polite to the real server; don't pay for it in tests.""" + monkeypatch.setattr(ls_process.time, "sleep", lambda _seconds: None) + + class _ScriptedServer(LanguageServerInterface): """Test double that answers each request synchronously from a scripted list of results, without a real language server process. """ - def __init__(self, results: list[Request.Result]) -> None: + def __init__(self, results: list[Request.Result], retry_methods: tuple[str, ...] = ("textDocument/hover",)) -> None: super().__init__(Language.PYTHON, lambda _line: logging.INFO) self._results = list(results) self.sent_payload_count = 0 + self.set_content_modified_retry_methods(retry_methods) def is_running(self) -> bool: return True @@ -83,3 +95,24 @@ def test_request_cancelled_is_not_retried() -> None: with pytest.raises(SolidLSPException): server.send_request("textDocument/hover") assert server.sent_payload_count == 1 + + +def test_content_modified_is_not_retried_for_undeclared_method() -> None: + """Retrying is a promise to the server about which methods will be reissued (per + ``general.staleRequestSupport.retryOnContentModified``); a method that was never registered + via ``set_content_modified_retry_methods`` must not be retried, even on ContentModified. + This also covers non-idempotent requests like ``workspace/executeCommand``, which no server + declaration should ever include. + """ + server = _ScriptedServer([_content_modified()], retry_methods=("textDocument/hover",)) + with pytest.raises(SolidLSPException): + server.send_request("workspace/executeCommand") + assert server.sent_payload_count == 1 + + +def test_content_modified_retry_methods_default_to_empty() -> None: + """A server that never calls `set_content_modified_retry_methods` must not retry anything.""" + server = _ScriptedServer([_content_modified()], retry_methods=()) + with pytest.raises(SolidLSPException): + server.send_request("textDocument/hover") + assert server.sent_payload_count == 1