mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-15 20:15:57 +00:00
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
This commit is contained in:
1 parent
30073d36d8
commit
8fa6759646
5 files changed
+125
-40
No files matched your search
@@ -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
|
||||
Reference in new issue
Block a user