mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-14 08:18:36 +00:00
fix(kotlin): give concurrent Serena instances independent LSP storage
Two Serena instances activating the same project concurrently launched their Kotlin LSP processes against the same on-disk index storage location (both the modern launcher's --system-path arg and the legacy initializationOptions.storagePath, hardcoded to None), so the second instance's requests were repeatedly cancelled by the first instance's server. DependencyProvider now claims that storage directory with a non-blocking file lock held for the process's lifetime. A single instance, including across restarts, still gets the same directory and keeps its cache; a second concurrent instance falls back to a directory of its own instead of contending for the first one's. Fixes #1966
This commit is contained in:
1 parent
7cde558158
commit
be609b6257
4 files changed
+114
-3
No files matched your search
@@ -67,6 +67,12 @@ Status of the `main` branch. Changes prior to the next official version change w
|
||||
silently returned an empty result instead of surfacing the crash. The crash is now detected
|
||||
independently via the `window/logMessage` notification tsserver already sends, and the
|
||||
affected wait now raises instead of reporting success (#1814)
|
||||
- Fix: two Serena instances activating the same project concurrently launched their Kotlin LSP
|
||||
processes against the same on-disk index storage location, so the second instance's requests
|
||||
were repeatedly cancelled by the first instance's server. A Kotlin LSP process now claims that
|
||||
storage directory via a lock; a single instance (including across restarts) still gets the
|
||||
same directory, and a second concurrent instance gets a directory of its own instead of
|
||||
contending for the first one's (#1966)
|
||||
|
||||
CLI:
|
||||
- Fix `project index-file` command not using only the relevant language server to index the given file (#1965)
|
||||
|
||||
@@ -23,6 +23,7 @@ import stat
|
||||
import threading
|
||||
from dataclasses import dataclass
|
||||
|
||||
from filelock import FileLock, Timeout
|
||||
from overrides import override
|
||||
|
||||
from solidlsp.dependency_provider import DownloadedDependency, DownloadedDependencyHashDatabase
|
||||
@@ -117,7 +118,37 @@ class KotlinLanguageServer(SolidLanguageServer):
|
||||
class DependencyProvider(LanguageServerDependencyProviderSinglePath):
|
||||
def __init__(self, custom_settings: SolidLSPSettings.CustomLSSettings, ls_resources_dir: str, project_cache_dir: str):
|
||||
super().__init__(custom_settings, ls_resources_dir)
|
||||
self._project_cache_dir = project_cache_dir
|
||||
self._storage_lock: FileLock | None = None
|
||||
self.storage_dir = self._claim_storage_dir(project_cache_dir)
|
||||
|
||||
def _claim_storage_dir(self, project_cache_dir: str) -> str:
|
||||
"""Claims the on-disk directory this instance's Kotlin LSP process will use for its index storage.
|
||||
|
||||
Tries the shared, deterministic per-project directory first via a non-blocking file lock, so a
|
||||
single Serena instance keeps reusing its index across restarts (the primary use case, which must
|
||||
not regress). If another live Serena instance already holds that directory (concurrent sessions
|
||||
on the same project, see oraios/serena#1966), falls back to a directory unique to this process
|
||||
instead of two Kotlin LSP processes contending for the same index.
|
||||
"""
|
||||
lock = FileLock(f"{project_cache_dir}.lock")
|
||||
try:
|
||||
lock.acquire(timeout=0)
|
||||
except Timeout:
|
||||
instance_dir = f"{project_cache_dir}-instance-{os.getpid()}"
|
||||
os.makedirs(instance_dir, exist_ok=True)
|
||||
log.info(
|
||||
"Kotlin LSP storage directory %s is in use by another Serena instance; using %s for this instance",
|
||||
project_cache_dir,
|
||||
instance_dir,
|
||||
)
|
||||
return instance_dir
|
||||
self._storage_lock = lock
|
||||
return project_cache_dir
|
||||
|
||||
def release_storage_lock(self) -> None:
|
||||
if self._storage_lock is not None:
|
||||
self._storage_lock.release()
|
||||
self._storage_lock = None
|
||||
|
||||
@classmethod
|
||||
def _create_artifact(cls, version: str, platform_id: PlatformId) -> KotlinLSPArtifact:
|
||||
@@ -224,7 +255,7 @@ class KotlinLanguageServer(SolidLanguageServer):
|
||||
platform_id = PlatformUtils.get_platform_id()
|
||||
intellij_launcher_name = "intellij-server.exe" if platform_id.is_windows() else "intellij-server"
|
||||
if os.path.basename(core_path).lower() == intellij_launcher_name:
|
||||
command.extend(["--system-path", os.path.join(self._project_cache_dir, "kotlin-lsp-system")])
|
||||
command.extend(["--system-path", os.path.join(self.storage_dir, "kotlin-lsp-system")])
|
||||
return command
|
||||
|
||||
def create_launch_command_env(self) -> dict[str, str]:
|
||||
@@ -243,10 +274,19 @@ class KotlinLanguageServer(SolidLanguageServer):
|
||||
env["JAVA_TOOL_OPTIONS"] = jvm_options
|
||||
return env
|
||||
|
||||
@override
|
||||
def stop(self, shutdown_timeout: float = 2.0) -> None:
|
||||
super().stop(shutdown_timeout=shutdown_timeout)
|
||||
if isinstance(self._dependency_provider, self.DependencyProvider):
|
||||
self._dependency_provider.release_storage_lock()
|
||||
|
||||
def _create_base_initialize_params(self) -> dict:
|
||||
"""
|
||||
Returns the initialize params for the Kotlin Language Server.
|
||||
"""
|
||||
dependency_provider = self._get_dependency_provider()
|
||||
assert isinstance(dependency_provider, self.DependencyProvider)
|
||||
storage_dir = dependency_provider.storage_dir
|
||||
root_uri = pathlib.Path(self.repository_root_path).as_uri()
|
||||
initialize_params = {
|
||||
"locale": "en",
|
||||
@@ -459,7 +499,7 @@ class KotlinLanguageServer(SolidLanguageServer):
|
||||
},
|
||||
"initializationOptions": {
|
||||
"workspaceFolders": [root_uri],
|
||||
"storagePath": None,
|
||||
"storagePath": storage_dir,
|
||||
"codegen": {"enabled": False},
|
||||
"compiler": {"jvm": {"target": "default"}},
|
||||
"completion": {"snippets": {"enabled": True}},
|
||||
|
||||
@@ -266,6 +266,59 @@ class TestKotlinDependencyProvider:
|
||||
]
|
||||
assert other_os_provider.create_launch_command() == [other_os_launcher, "--stdio"]
|
||||
|
||||
def test_single_instance_keeps_the_deterministic_cache_dir(self, tmp_path: Path) -> None:
|
||||
"""The common case (one Serena instance, possibly restarted) must keep reusing the
|
||||
same on-disk directory, or the Kotlin LSP loses its index cache on every restart.
|
||||
"""
|
||||
provider = _make_provider(tmp_path)
|
||||
|
||||
assert provider.storage_dir == str(tmp_path / "project-cache")
|
||||
|
||||
def test_second_concurrent_instance_gets_its_own_storage_dir(self, tmp_path: Path) -> None:
|
||||
"""Two Serena instances activating the same project concurrently (oraios/serena#1966)
|
||||
must not be handed the same Kotlin LSP storage directory: the second one falls back to
|
||||
a directory of its own instead of contending with the first for the same index.
|
||||
"""
|
||||
first = _make_provider(tmp_path)
|
||||
second = _make_provider(tmp_path)
|
||||
try:
|
||||
assert first.storage_dir == str(tmp_path / "project-cache")
|
||||
assert second.storage_dir != first.storage_dir
|
||||
assert second.storage_dir.startswith(str(tmp_path / "project-cache") + "-instance-")
|
||||
finally:
|
||||
first.release_storage_lock()
|
||||
second.release_storage_lock()
|
||||
|
||||
def test_storage_dir_is_reclaimed_once_the_first_instance_releases_it(self, tmp_path: Path) -> None:
|
||||
first = _make_provider(tmp_path)
|
||||
first.release_storage_lock()
|
||||
|
||||
second = _make_provider(tmp_path)
|
||||
try:
|
||||
assert second.storage_dir == str(tmp_path / "project-cache")
|
||||
finally:
|
||||
second.release_storage_lock()
|
||||
|
||||
def test_concurrent_instances_get_different_system_path_arguments(self, tmp_path: Path) -> None:
|
||||
launcher = "/path/to/intellij-server"
|
||||
first = _make_provider(tmp_path, {"ls_path": launcher})
|
||||
second = _make_provider(tmp_path, {"ls_path": launcher})
|
||||
try:
|
||||
with patch(
|
||||
"solidlsp.language_servers.kotlin_language_server.PlatformUtils.get_platform_id",
|
||||
return_value=PlatformId.LINUX_x64,
|
||||
):
|
||||
first_cmd = first.create_launch_command()
|
||||
second_cmd = second.create_launch_command()
|
||||
|
||||
assert first_cmd == [launcher, "--stdio", "--system-path", str(tmp_path / "project-cache" / "kotlin-lsp-system")]
|
||||
assert second_cmd[:2] == [launcher, "--stdio"]
|
||||
assert second_cmd[2] == "--system-path"
|
||||
assert second_cmd[3] != first_cmd[3]
|
||||
finally:
|
||||
first.release_storage_lock()
|
||||
second.release_storage_lock()
|
||||
|
||||
def test_invalid_version_is_rejected_before_download(self) -> None:
|
||||
with pytest.raises(ValueError, match="dot-separated integers"):
|
||||
KotlinLanguageServer.DependencyProvider._create_artifact("latest", PlatformId.OSX_arm64)
|
||||
|
||||
@@ -107,3 +107,15 @@ def test_modern_ready_timeout_warns_and_continues(tmp_path: Path, caplog: Any) -
|
||||
server._start_server()
|
||||
|
||||
assert "Kotlin LSP did not signal indexing completion within 120s; proceeding anyway" in caplog.text
|
||||
|
||||
|
||||
def test_storage_path_is_no_longer_hardcoded_none(tmp_path: Path) -> None:
|
||||
"""A hardcoded None here means every instance shares the LSP's own default index
|
||||
location; two concurrent instances on the same project then contend for it and the
|
||||
second fails to index (oraios/serena#1966).
|
||||
"""
|
||||
server, _indexing_complete, _intellij_server_ready = _make_server(tmp_path)
|
||||
|
||||
storage_path = server._create_base_initialize_params()["initializationOptions"]["storagePath"]
|
||||
|
||||
assert storage_path == str(tmp_path / "project" / "cache" / "kotlin")
|
||||
Reference in new issue
Block a user