Skip to content

Fix LSP memory leak on goto definition - #10860

Open
JRI98 wants to merge 1 commit into
roc-lang:mainfrom
JRI98:fix-10857
Open

Fix LSP memory leak on goto definition#10860
JRI98 wants to merge 1 commit into
roc-lang:mainfrom
JRI98:fix-10857

Conversation

@JRI98

@JRI98 JRI98 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Goto-definition queries returning URIs to external or built-in modules dynamically allocated memory that was never released by the handler or internal query fallbacks, resulting in a memory leak detected during server shutdown.

This was addressed by establishing an explicit ownership and cleanup lifecycle for definition query results across the definition handler and hover resolution fallback.

Fixes #10857

@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR establishes explicit ownership and cleanup for goto-definition result URIs, including the hover fallback, and strengthens the built-in definition integration test.

  • Adds DefinitionResult.deinit and invokes it after definition and hover result consumption.
  • Duplicates current-document URIs so every externally returned definition result owns its URI.
  • Updates the test syntax driver to satisfy the generic handler interface.
  • Makes the built-in type definition test require a concrete Builtin.roc location.

Confidence Score: 5/5

The PR appears safe to merge, with definition-result URI ownership consistently established and released across the changed paths.

Every definition result reaching the new cleanup sites contains an allocator-owned URI, allocation failures remain propagated, and each result is released exactly once.

Important Files Changed

Filename Overview
src/lsp/syntax.zig Introduces a uniform owned-URI contract for definition results and releases hover fallback results after use.
src/lsp/handlers/definition.zig Releases definition query results after serializing the location response.
src/lsp/test/handler_integration_tests.zig Reworks the built-in definition fixture and now requires a valid Builtin.roc response.
src/lsp/test/test_syntax_driver.zig Adds the cleanup method required by the generic definition handler interface.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Definition query] --> B{Definition target}
    B -->|Current document| C[Duplicate current URI]
    B -->|External or built-in module| D[Allocate URI with pathToUri]
    C --> E[Owned DefinitionResult]
    D --> E
    E --> F{Consumer}
    F -->|Definition handler| G[Build response]
    F -->|Hover fallback| H[Resolve referenced definition]
    G --> I[deinit frees URI]
    H --> I
Loading

Reviews (1): Last reviewed commit: "Fix LSP memory leak on goto definition" | Re-trigger Greptile

@JRI98
JRI98 enabled auto-merge August 19, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LSP panic: reached unreachable code

1 participant