Skip to content

fix(lsp): bound the client write path against a server that stops reading stdin#146

Merged
jrollin merged 1 commit into
mainfrom
fix/lsp-write-path-wedge-fast-fail
Jul 6, 2026
Merged

fix(lsp): bound the client write path against a server that stops reading stdin#146
jrollin merged 1 commit into
mainfrom
fix/lsp-write-path-wedge-fast-fail

Conversation

@jrollin

@jrollin jrollin commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Context

PR #145 shipped a fast-abort for one LSP failure mode: a server that reads its stdin but never replies (all-timeout windows → abandon the language). It did not cover the sibling mode: a server that stops reading its stdin.

LspClient::write_message did a blocking write_all on ChildStdin. Once the ~64KB kernel pipe buffer fills, that write_all blocks forever — there were read deadlines but no write deadline. A wedged didOpen write (the whole file text) hung cartog index uncancellably; the process stays alive, so neither the death path nor the mute-abort fired. This was finding #5 of the #145 review, deferred as too structural to ride the bug-fix.

Change

Dedicated writer thread (mirrors the existing reader thread) that owns ChildStdin and performs every framed write, acking each on a persistent per-client channel. write_message waits on the ack with a bounded write_timeout (10s default) — a stuck stdin surfaces as an ack timeout the owner can bail on, not a wedged write_all.

On timeout the client is latched wedged (later writes fast-fail, never re-block behind the parked job) and dropped so a warm manager can't reuse it. Drop kills the child, breaking the pipe so the parked writer exits on its own (no join, no leak).

  • Both drain write-timeout arms set stopped_early (drained answers stay trusted) and drop the client via LspManager::drop_client.
  • The cancel-path close_file now fast-fails on a wedged client, so Ctrl-C stays prompt.
  • Chosen over non-blocking-fd I/O: zero unsafe, identical on Windows, trivial framed-write atomicity, reuses a proven pattern.

Review-driven follow-ups (same session, pre-commit)

An xhigh workflow review found gaps in the initial writer-thread version, all fixed here:

  • No second full stall: the write_wedged latch makes any write after a wedge fast-fail (was: close_file/auto_respond re-blocking another full write_timeout).
  • Warm-manager reuse: the wedged client is removed + reaped so start()'s contains_key short-circuit can't reuse it and re-stall every warm cartog_index.
  • Prompt Ctrl-C: cancel-path write fast-fails when wedged.

Performance

The per-write ack round-trip costs ~6.7µs vs ~0.44µs for a direct write_all — two thread wakeups, not allocation (removing the per-call channel alloc left it unchanged). Immaterial in context: writes pipeline 64-wide and each hides behind a 1–50ms server round-trip, so ~100k writes add ~0.6s to a ~3.5-min resolution pass (~0.3%). No gated criterion bench touches this path. Recorded in concurrency.md §7. The ack channel is reused per client to keep it off the per-edge allocation path.

Tests

  • write_message_times_out_when_child_never_reads_stdin (regression-first; proven to hang on main)
  • second_write_fast_fails_after_a_wedge_instead_of_blocking_again
  • write_timeout_does_not_hang_drop, is_write_timeout_matches_only_the_write_prefix
  • drop_client_removes_and_reaps_so_start_wont_reuse_a_wedged_server
  • drain_language_stops_early_on_write_timeout (asserts stopped_early + client dropped)

Docs

concurrency.md §7 (write side now bounded, measured cost) and troubleshooting.md. No CLI/flag/config/MCP/language/count change, so the site and reference docs are untouched per the doc-sync rule.

Gates

cargo fmt --check, clippy (default + --no-default-features, workspace), cargo test --workspace + --no-default-features (both green), cargo test -p cartog-lsp (79 passed), doctests, cargo doc --no-deps warning-clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved LSP reliability when a server stops reading input, preventing hangs during requests and file opens.
    • Added faster failure handling after a write timeout so repeated attempts no longer wait unnecessarily.
    • Ensured stuck language servers are dropped and not reused after timeout-related failures.
  • Documentation

    • Updated concurrency and troubleshooting guides with the new write-timeout behavior and expected recovery flow.

…ding stdin

A live LSP server that stops draining its stdin fills the ~64KB OS pipe buffer and wedges the
blocking write_all forever. There were read deadlines but no write deadline, so a stuck didOpen
(the largest write) hung `cartog index` uncancellably.

Add a dedicated writer thread that owns ChildStdin and acks each framed write on a persistent
per-client channel; write_message waits on the ack with a bounded write_timeout (10s default).
A stuck stdin surfaces as an ack timeout the owner can bail on instead of a wedged write. On
timeout the client is latched wedged (later writes fast-fail, never re-block) and dropped so a
warm manager cannot reuse it; Drop kills the child, breaking the pipe so the parked writer exits.

Both drain write-timeout arms set stopped_early (drained answers stay trusted) and drop the
client. The cancel-path close_file now fast-fails on a wedged client, so Ctrl-C stays prompt.

The ack round-trip costs ~6us/write vs a direct write_all (two thread wakeups, not allocation).
Immaterial: writes pipeline 64-wide and hide behind 1-50ms server round-trips, so ~100k writes
add ~0.6s to a ~3.5min resolution pass. The ack channel is reused per client to keep it off the
per-edge allocation path.

Tests: regression-first write-timeout + fast-fail-once-wedged + drop-and-reap + Drop-does-not-hang.
Docs: concurrency.md (write side now bounded, measured cost) and troubleshooting.md.
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3647646a-d83c-4f30-a9c0-4ea6f0f2681f

📥 Commits

Reviewing files that changed from the base of the PR and between ce3ff3f and 63f471d.

📒 Files selected for processing (5)
  • crates/cartog-lsp/src/client.rs
  • crates/cartog-lsp/src/manager.rs
  • crates/cartog-lsp/src/resolve.rs
  • docs/explanation/concurrency.md
  • docs/troubleshooting.md

📝 Walkthrough

Walkthrough

Introduces a background writer thread in LspClient for stdin writes with per-write acknowledgement timeouts and a wedged-state latch, replacing direct synchronous writes. LspManager adds drop_client and test helpers. resolve.rs's drain_language treats write timeouts as a mute-server condition. Documentation updated accordingly.

Changes

LSP write timeout and wedge handling

Layer / File(s) Summary
Writer thread, ack channel, and write_message rewrite
crates/cartog-lsp/src/client.rs
LspClient now writes via a background writer thread that acks each write over a persistent channel; write_message waits on the ack with a per-write deadline, latching write_wedged and fast-failing on timeout; adds WRITE_TIMEOUT_PREFIX/is_write_timeout classification and new unix tests.
LspManager drop_client and test helpers
crates/cartog-lsp/src/manager.rs
Adds drop_client to remove and drop a per-language client (killing/reaping its process), plus test-only helpers for write timeout configuration, client presence checks, and child PID retrieval, with a supporting integration test.
drain_language stopped-early handling
crates/cartog-lsp/src/resolve.rs
open_file and definitions_batch write-timeout errors now mark stopped_early, drop the client, and break early instead of falling into server-death handling; adds a test verifying this behavior.
Documentation updates
docs/explanation/concurrency.md, docs/troubleshooting.md
Concurrency and troubleshooting docs describe the reader/writer thread split, per-write deadlines, wedging, and fallback to heuristics when a server stops reading stdin.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant LspClient
  participant WriterThread
  participant ChildStdin
  participant LspManager

  Caller->>LspClient: write_message(bytes)
  LspClient->>WriterThread: send WriteJob
  WriterThread->>ChildStdin: write_all + flush
  alt ack within timeout
    WriterThread-->>LspClient: ack Ok(())
    LspClient-->>Caller: Ok(())
  else ack timeout
    LspClient->>LspClient: set write_wedged = true
    LspClient-->>Caller: WRITE_TIMEOUT_PREFIX error
    Caller->>LspManager: drop_client(language)
    LspManager->>LspClient: drop (kill child process)
  end
Loading

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: bounding LSP client writes when the server stops reading stdin.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/lsp-write-path-wedge-fast-fail

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.28571% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.35%. Comparing base (ce3ff3f) to head (63f471d).

Files with missing lines Patch % Lines
crates/cartog-lsp/src/resolve.rs 81.57% 7 Missing ⚠️
crates/cartog-lsp/src/client.rs 98.05% 2 Missing ⚠️
crates/cartog-lsp/src/manager.rs 97.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   88.32%   88.35%   +0.03%     
==========================================
  Files          96       96              
  Lines       32307    32474     +167     
==========================================
+ Hits        28534    28692     +158     
- Misses       3773     3782       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jrollin jrollin merged commit 7119159 into main Jul 6, 2026
14 checks passed
@jrollin jrollin deleted the fix/lsp-write-path-wedge-fast-fail branch July 6, 2026 10:40
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.

1 participant