Skip to content

fix(lsp): read1() on the session pipe so short frames are not held in the buffer - #118

Merged
josephsenior merged 1 commit into
josephsenior:mainfrom
piakdev:fix/lsp-session-short-frame-read
Aug 2, 2026
Merged

fix(lsp): read1() on the session pipe so short frames are not held in the buffer#118
josephsenior merged 1 commit into
josephsenior:mainfrom
piakdev:fix/lsp-session-short-frame-read

Conversation

@piakdev

@piakdev piakdev commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

LspSession._read_loop reads the language server's stdout with proc.stdout.read(4096).

Popen returns a BufferedReader, and BufferedReader.read(size) blocks until it can fill the whole requested buffer or the stream hits EOF. A language server is a long-lived process, so it never closes stdout.

When the server's reply is shorter than 4096 bytes, the reader blocks with a complete JSON-RPC frame already sitting in the pipe, while the server waits for the initialized notification that the client only sends after parsing the initialize response. Both sides wait on each other until the init timeout fires.

Repro

Configure any stdio language server and run an lsp tool call:

lsp_query        LSP find_references on /path/to/file.js:88:10
lsp_query_result LSP unavailable — falling back to grep/search.
tool_result      {available: false, has_error: true, latency_ms: 20024, retryable: false}

Measured against typescript-language-server on a small JS workspace:

frame bytes
window/logMessage 234
initialize response (id=1) 1,814
total before initialized 2,048

2,048 < 4096, so read() never returns. latency_ms lands exactly on the 20s _DEFAULT_INIT_TIMEOUT_SEC.

Meanwhile the server itself is healthy — driving the same binary directly over JSON-RPC from the same cwd answers initialize in ~0.1s.

Fix

read_available = getattr(proc.stdout, 'read1', proc.stdout.read)
...
chunk = read_available(4096)

read1() returns after a single underlying pipe read, so a complete frame is delivered as soon as it arrives. The getattr keeps a read() fallback for file-like test doubles and unusual Popen configurations.

Test plan

  • Adds backend/tests/unit/utils/test_lsp_session_reader.py — spawns a helper process that writes one short initialize response then sleeps, reproducing the deadlock.
  • The new test fails on the unpatched reader (LSP initialize timed out) and passes with read1() — verified by reverting the one-line change and re-running.
  • pytest backend/tests/unit/utils passes (54 LSP-related tests).
  • After the fix, real lsp tool calls return LSP query completed. and find_references yields correct line numbers instead of falling back to grep.

The unrelated failures under validation/, orchestration/, and cli/tui/ reproduce identically on a clean main in this environment (missing pytest plugins such as pytest-trio/pytest-twisted), so they are not regressions from this change.

Summary by Sourcery

Prevent LSP session stdout reader from blocking indefinitely on short JSON-RPC frames by using a non-buffer-filling read method.

Enhancements:

  • Improve LSP session stdout handling to work correctly with long-lived language server processes that emit small responses.

Tests:

  • Add regression test ensuring the LSP session delivers short initialize responses promptly without waiting for EOF.

… the buffer

`LspSession._read_loop` calls `proc.stdout.read(4096)`. `Popen` hands back a
`BufferedReader`, and `BufferedReader.read(size)` keeps blocking until it can
fill the whole requested buffer or the stream reaches EOF. A language server is
a long-lived process, so it never closes stdout.

When the server's reply is shorter than 4096 bytes the reader blocks with a
complete JSON-RPC frame already sitting in the pipe, while the server waits for
the `initialized` notification that only arrives after the client parses the
initialize response. Both sides wait for each other until the init timeout
fires.

Measured against typescript-language-server: 234 bytes of `window/logMessage`
plus a 1,814-byte initialize response = 2,048 bytes total, well short of 4096.
Every `lsp_query` then reported `available=false` with `latency_ms` pinned to
the 20s init timeout and fell back to grep, even though the server had answered
in ~0.1s.

`read1()` returns after a single underlying pipe read, so a complete frame is
delivered as soon as it arrives. `getattr(..., 'read1', ...read)` keeps a
fallback for file-like test doubles and unusual Popen configurations.

Adds a regression test that spawns a helper process which writes one short
initialize response and then sleeps, reproducing the deadlock. It fails on the
unpatched reader (init times out) and passes with `read1()`.

Verified: backend/tests/unit/utils passes (54 LSP-related tests). The unrelated
failures in validation/orchestration/tui reproduce identically on a clean `main`
in this environment and are missing pytest plugins, not regressions from this
change.
@piakdev
piakdev requested a review from josephsenior as a code owner August 2, 2026 12:50
@sourcery-ai

sourcery-ai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Reviewer's Guide

Adjusts the LSP session stdout reader to use a non-blocking, pipe-friendly read method so short JSON-RPC frames are delivered promptly, and adds a regression test to ensure short initialize responses don’t deadlock initialization.

Sequence diagram for updated LspSession stdout read loop

sequenceDiagram
    actor Client
    participant LspSession
    participant LanguageServer
    participant BufferedReader

    Client->>LspSession: start_session()
    LspSession->>LanguageServer: send_initialize()
    LanguageServer->>BufferedReader: write initialize_response (<= 2048 bytes)

    LspSession->>LspSession: _read_stdout()
    LspSession->>BufferedReader: getattr(stdout, read1, read)

    alt before_fix
        LspSession->>BufferedReader: read(4096)
        BufferedReader-->>LspSession: [blocks until buffer full or EOF]
        Client-->>Client: init timeout fires
    else after_fix
        LspSession->>BufferedReader: read1(4096)
        BufferedReader-->>LspSession: chunk (complete JSON-RPC frame)
        LspSession->>LspSession: parse_frame()
        LspSession->>LanguageServer: send_initialized()
        Client-->>Client: LSP query completed
    end
Loading

File-Level Changes

Change Details Files
Modify LSP session stdout reader to avoid blocking indefinitely on short frames from long-lived language servers.
  • Introduce a local read_available callable that prefers read1() when available and falls back to read().
  • Replace direct proc.stdout.read(4096) usage with read_available(4096) inside the read loop.
  • Document BufferedReader.read() semantics and the deadlock scenario in comments near the read loop.
backend/utils/lsp/lsp_session.py
Add a unit test that reproduces and guards against deadlocks when the language server emits a short initialize response frame.
  • Create a helper stdio language server process that writes a single short initialize response and then sleeps, mimicking the problematic behavior.
  • Assert the encoded initialize response frame is smaller than the 4096-byte read buffer to ensure the test hits the original deadlock condition.
  • Use LspSession.ensure_initialized(timeout=1.0) to verify that initialization completes successfully under the new reader behavior and cleanly close the session afterward.
backend/tests/unit/utils/test_lsp_session_reader.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@josephsenior
josephsenior merged commit c9b670f into josephsenior:main Aug 2, 2026
1 check passed
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.

2 participants