[Kotlin LSP] Do not download extra Java. by cx-alberto-simoes · Pull Request #1079 · oraios/serena

cx-alberto-simoes · 2026-02-24T14:58:33Z

Changes

Do not download an extra jre, as kotlin lsp already ships one

src/solidlsp/language_servers/kotlin_language_server.py

MischaPanch · 2026-02-24T15:03:38Z

src/solidlsp/language_servers/kotlin_language_server.py

            """Provides JAVA_HOME and JVM options for the Kotlin Language Server process."""
            env: dict[str, str] = {}

+            # Only set JAVA_HOME if using bundled Java (not from PATH)


Shouldn't we verify that the java found on path is already set in JAVA_HOME?

Thanks for the quick feedback. Fixed the other issues (were easy).
For this one I need a little more help. With a couple of questions to LLMs, I got to this solution, but not sure if that is what you meant.
Thank you

I don't know how JAVA_HOME is used by the KotlinLS and whether it's important that the java executable that we find/download coincides with the value of JAVA_HOME. My question was about finding out whether it's necessary and ensuring for the values to coincide for KotlinLS if it is

One of the first errors I found was kotlin-lsp trying to chmod its own shipped Java. Thus, I suppose we can delete all java management here, and rely solely on kotlin-lsp. I did not change that behavior as I am afraid I am breaking something I am not aware of. If we have currently tests for kotlin, I can try to simplify and try to understand if things still work.

We have tests for kotlin, they are disabled in CI due to resource reasons. Even locally I can get test failures when running tests simultaneously because of some race conditions and failures to clean up resources. You can run tests with pytest -m kotlin.

I have noticed today that the kotlin LS starts openjdk processes that are never torn down, a real problem. I suspect this might be a reason for the failing tests. If you'd like to look into it, I'd be grateful.

Pinging @mareurs who knows more about Kotlin LS issues than I do

Investigated the zombie JVM issue. Here's what I found:

TL;DR: The "openjdk processes that are never torn down" are Gradle daemons, not KLS itself.

Evidence

Ran controlled before/after tests (snapshot all Java PIDs, run pytest -m kotlin, snapshot again):

KLS cleanup works correctly. Even under SIGKILL (simulated crash), KLS exits naturally because the --stdio pipe breaks on parent death (EOF on stdin). The psutil-based process tree cleanup in ls_process.py also works for the happy path. Zero leaked KLS processes across multiple test runs.

Each KLS startup spawns a Gradle daemon (~500MB RSS) that persists independently:

PID 2430590 (GradleDaemon 9.0.0) PWD = .../serena/test/resources/repos/kotlin/test_repo JAVA_HOME = .../KotlinLanguageServer/.../jre/21.0.7-linux-x86_64 Parent = systemd --user (already daemonized) RSS = 555MB, idle timeout = 3 hours (Gradle default)

Reproduced deterministically: killed all test Gradle daemons, ran test, new one appeared with PWD pointing at the Kotlin test repo and JAVA_HOME pointing at KLS's bundled JRE.

Why it accumulates

Gradle daemons are keyed by Java home + Gradle version + JVM args

If anything differs between runs (e.g. different JAVA_TOOL_OPTIONS from Serena's JVM options setting), Gradle spawns a new daemon instead of reusing

Default idle timeout is 3 hours, so they pile up during development/testing sessions

Mitigation options

Run gradle --stop after KLS shutdown (or pkill -f GradleDaemon in test teardown)

Set org.gradle.daemon=false in the test repo's gradle.properties

Set shorter daemon idle timeout: org.gradle.daemon.idletimeout=60000 (1 min)

The test repo is at test/resources/repos/kotlin/test_repo — adding gradle.properties with org.gradle.daemon=false there would prevent daemon accumulation during testing.

mareurs · 2026-02-24T16:45:16Z

Code Review

Reviewed the diff. The goal (reuse system Java/KLS) is good, but there are some issues.

Bug: JAVA_HOME / PATH version mismatch

In _setup_java_dependency() lines 187-190:

if java_home := os.environ.get("JAVA_HOME"):
    self._java_home_path = java_home
else:
    self._java_home_path = os.path.dirname(os.path.dirname(java_path))

The version check runs java -version from PATH, but then blindly trusts the existing JAVA_HOME env var. kotlin-lsp.sh uses $JAVA_HOME/bin/java when JAVA_HOME is set (lines 17-24 of the script). So if JAVA_HOME points to Java 17 but PATH has Java 21, the version check passes but KLS runs with Java 17.

Fix: If JAVA_HOME is already set, validate the version at $JAVA_HOME/bin/java instead of (or in addition to) java from PATH. Or always derive JAVA_HOME from the validated PATH binary.

`ls_path` already provides KLS override

The base class LanguageServerDependencyProviderSinglePath already checks ls_path from ls_specific_settings before calling _get_or_install_core_dependency(). Users can already set ls_path: "kotlin-lsp.sh" to use a system KLS. The shutil.which() check partially duplicates this.

Not a blocker — auto-detection is more user-friendly than manual config — but worth being aware of.

Inconsistency with JDTLS approach

PR #835 (merged recently) added an explicit use_system_java_home opt-in setting for Eclipse JDTLS. This PR auto-detects system Java without a setting, which changes behavior for users who previously relied on Serena's managed JRE. Making it opt-in (like JDTLS) or at minimum well-documented would be more consistent.

Minor issues

_java_path unused (line 231): _java_path = self._setup_java_dependency(...) — return value is never used. The real side-effect is setting self._java_home_path.
Duplicate logging: Both _check_java_version_in_path() (line 154) and _setup_java_dependency() (line 184) log that system Java was found.

Note on `kotlin-lsp.sh` and Java

For reference, kotlin-lsp.sh (lines 17-24) does:

JAVA_BIN="java"
if [ -n "$JAVA_HOME" ]; then
    JAVA_BIN="$JAVA_HOME/bin/java"
fi

So JAVA_HOME takes precedence over PATH — this is why the version check and actual JVM used can diverge if they don't match.

MischaPanch · 2026-02-24T16:49:54Z

@mareurs thanks for pitching in, I agree with the analysis and the recommendations. Let's not address the gradle daemon related issues here, it's independent. But I suspect they are related to test failures due to failing initialization in CI (and on my laptop)

mareurs · 2026-02-24T16:52:00Z

@mareurs thanks for pitching in, I agree with the analysis and the recommendations. Let's not address the gradle daemon related issues here, it's independent. But I suspect they are related to test failures due to failing initialization in CI (and on my laptop)

I am lucky, I have 128G RAM, 64 cores. It never fails :)

MischaPanch · 2026-02-24T16:53:12Z

I am lucky, I have 128G RAM, 64 cores. It never fails :)

The upside of a smaller system is that zombies are more apparent ;)

opcode81 · 2026-02-24T17:32:34Z

@cx-alberto-simoes please explain why this change is necessary. What is the point of reusing the system JRE?

The Kotlin LSP spawns detached daemon processes, and the only way to reliably terminate them is to search for processes that are associated with the JRE. See PR #1082.
If we use a different JRE, we lose that ability, which would be a huge problem.

So unless you have very good reasons for not using the bundled JRE, this is not a good idea.

cx-alberto-simoes · 2026-02-25T10:51:51Z

@opcode81, ok, let me clarify:

Original behavior, a jre was downloaded, then kotlin lsp was downloaded (that bundles a Java version).
My original proposal, do not download a jre is one is available (but we still downloaded kotlin lsp what ships a java)
Current behavior, just kotlin lsp is downloaded (the bundled java version is used).

opcode81 · 2026-02-25T11:54:13Z

src/solidlsp/language_servers/kotlin_language_server.py

+            if shutil.which(kotlin_script_name):
+                log.info(f"Found Kotlin {kotlin_script_name} in PATH. Using it instead of bundled version.")
+                return kotlin_script_name


We should not do this implicitly. As @mareurs already mentioned, we have an explicit mechanism for the user to overrride the language server installation (ls_path).

But that is not coherent with other languages. For instance, we do not try to install clangd.

but ok, will try to use ls_path, and see if I still require changes.
Nevertheless, I suggest removing the download of Java.

It is consistent with all language servers that we install.
If we can install it, then the version we install is the one that is used unless it is explicitly overridden.

The problem with clangd is that we cannot install it. Therefore, we have to rely on the user installing it. If we could install it, we would - and apply the same mechanisms.

The problem with just using any version the user may have installed is that we cannot ensure that our code is actually compatible. For instance, some language servers change their behaviour in different versions and start returning different types of content (e.g. a different way of returning symbol names) - and this may necessitate workarounds, etc.
So it is actually discouraged to use any other version than the one we have installed and tested.
If a user wants to use a different version, it should be an explicit decision.

Nevertheless, I suggest removing the download of Java.

Definitely. If that was redundant, then this is an improvement we should keep!

cx-alberto-simoes · 2026-02-25T15:06:55Z

@opcode81 I think this is now acceptable by your design. Thanks

cx-alberto-simoes · 2026-02-27T10:48:59Z

Just a poke, as this PR should be quite simple to approve.

opcode81 · 2026-02-27T11:39:13Z

Indeed, this looks good now. Merging...

Reuse java and kotlin if possible

cc0e2af

MischaPanch reviewed Feb 24, 2026

View reviewed changes

cx-alberto-simoes added 3 commits February 24, 2026 15:10

Minor tweaks

1a17a33

Compute JAVA_HOME

64b4086

Linting

927b088

MischaPanch mentioned this pull request Feb 24, 2026

Kotlin LS spawns heavy zombie processes #1081

Open

mareurs mentioned this pull request Feb 24, 2026

fix(kotlin): Kill Gradle daemons on KLS shutdown #1082

Open

Simplify Kotlin

57048cc

cx-alberto-simoes added 2 commits February 25, 2026 10:52

poe

8eff695

Remove unuised comment

8c5b82d

opcode81 reviewed Feb 25, 2026

View reviewed changes

cx-alberto-simoes added 2 commits February 25, 2026 14:32

Fix docs

ca547c0

Remove looking up in the path

31d6d7d

cx-alberto-simoes changed the title ~~[Kotlin LSP] Reuse system java and kotlin LSP if possible~~ [Kotlin LSP] Do not download extra Java. Feb 25, 2026

Revert

10f08eb

opcode81 merged commit e4c4bf4 into oraios:main Feb 27, 2026
3 of 4 checks passed

Uh oh!

Conversation

cx-alberto-simoes commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Evidence

Why it accumulates

Mitigation options

Uh oh!

mareurs commented Feb 24, 2026

Code Review

Bug: JAVA_HOME / PATH version mismatch

ls_path already provides KLS override

Inconsistency with JDTLS approach

Minor issues

Note on kotlin-lsp.sh and Java

Uh oh!

MischaPanch commented Feb 24, 2026

Uh oh!

mareurs commented Feb 24, 2026

Uh oh!

MischaPanch commented Feb 24, 2026

Uh oh!

opcode81 commented Feb 24, 2026

Uh oh!

cx-alberto-simoes commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opcode81 Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cx-alberto-simoes commented Feb 25, 2026

Uh oh!

cx-alberto-simoes commented Feb 27, 2026

Uh oh!

opcode81 commented Feb 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cx-alberto-simoes commented Feb 24, 2026 •

edited

Loading

`ls_path` already provides KLS override

Note on `kotlin-lsp.sh` and Java

cx-alberto-simoes commented Feb 25, 2026 •

edited

Loading

opcode81 Feb 25, 2026 •

edited

Loading