fix(solidlsp): fail the pending request when the stdin write fails - #2030

All committers have signed the CLA.

Rebased onto main (704e8c3d) to clear the dirty state the CHANGELOG entry caused once the licensing branch landed. Nothing semantic moved: src/solidlsp and test/solidlsp remain MIT under the new component split, the only additions in the rebased commit are the resolved CHANGELOG block and the # SPDX-License-Identifier: MIT header on the new test file, which CONTRIBUTING.md now requires for new files.

Head is now c514eda. Re-measured on the rebased tree: the four tests pass, dropping ValueError from the caught tuple still fails only test_closed_stdin_fails_pending_request_with_terminated, and ruff check / ruff format --check at 0.12.5 plus ty check at 0.0.24 are all rc=0. The description carries the updated hashes.

Rebased onto current main (c6fbd1c5) to clear the dirty merge state — that was the only thing blocking this. Head is now 7e5f463a.

Nothing semantic moved. git diff --name-only c6fbd1c5...HEAD is still exactly the three files, and the two code blobs are byte-identical to the c514edac you tested:

The only content change is the CHANGELOG.md entry, which conflicted because 11 upstream commits touched it since the old base. Resolved by keeping every upstream entry and inserting this one after the session-id bullet; git diff c6fbd1c5...HEAD -- CHANGELOG.md is 4 added lines and nothing else.

Re-measured on the rebased tree, fresh venv on Python 3.11 (project deps installed from pyproject.toml, solidlsp resolved from this checkout):

pytest test/solidlsp/test_stdio_send_failure.py   ->  4 passed in 0.24s
ruff check <the two files>                        ->  rc=0
ruff format --check <the two files>               ->  rc=0, "2 files already formatted"

Mutation, to confirm the suite still bites here: removing ValueError from the caught tuple at ls_process.py:671 gives 1 failed, 3 passed — test_closed_stdin_fails_pending_request_with_terminated is the one that fails, same as your container run.

One thing I should say rather than let it pass: the part you called "the local bindings close the second window" is not covered by a test. I checked — reverting the single-read of self._process in _send_payload (lines 659-662) to a re-read leaves the suite at 4 passed, so nothing here would catch that regression. The binding is still what I believe is correct, and your own run confirmed it, but the claim "tested" would overstate it. A deterministic test for that window needs to flip _process between the two reads, which the current fixture shape can't do; I would rather it be a separate change with its own design than bolt a contrived hook onto this one.

CI on the rebased head is still in flight; I will not treat it as green until the runs report.

Follow-up commit 901ac17e, test file only.

After the rebase the Type-checking with ty step went red on this branch (it is green on up/main c6fbd1c5, and the three errors CI reports are all in test/solidlsp/test_stdio_send_failure.py):

error[unresolved-attribute]: Attribute `stdin` is not defined on `None` in union `ManagedSubprocess[bytes] | None`
error[unresolved-attribute]: Attribute `closed` is not defined on `None` in union `IO[bytes] | None`
error[unresolved-attribute]: Unresolved attribute `side_effect` on type Overload[...]

They were latent in my test code before and surfaced once upstream made _process a ManagedSubprocess[bytes] | None; I reproduced unresolved-attribute locally on the same three lines with the pinned ty==0.0.24, though my own invocation reported a different total from CI's, so I treated CI's list as the target rather than my local count. The fix is mine to make either way.

Two changes, both inside the test:

• the closed-stdin case now binds process = server._process / stdin = process.stdin and asserts each is not None before using it, instead of walking server._process.stdin twice. Same scenario, same assertions, and the precondition is now explicit in the test rather than assumed.
• test_healthy_write_leaves_request_pending no longer reaches through the typed attribute to undo a side effect. The helper takes None to mean "the write succeeds": _server_with_failing_stdin(None). Behaviour is what it was — the mock's writelines has no side effect — only the expression of it changed.

Re-measured after the change, in this tree:

pytest test/solidlsp/test_stdio_send_failure.py   ->  4 passed in 0.24s
ruff check / ruff format --check <that file>      ->  rc=0, already formatted

No product code moved, so the src/solidlsp/ls_process.py blob is still byte-identical to the c514edac you already ran in the container; if you want to re-verify, the only thing worth your attention is the test helper's new exc: Exception | None parameter. Waiting on the CI run before saying anything about it being green.

Rebased onto current main (c4dc91a7) to clear the dirty merge state, which was the only thing blocking this. Head is now d98a59ad.

main moved by two commits since the previous base: the MCP SDK v2.2.0 upgrade and the is_ignored_path traversal change. Neither touches src/solidlsp/ or test/solidlsp/ (git log --name-only c6fbd1c5..c4dc91a7 lists only src/serena/, pyproject.toml, uv.lock, CHANGELOG.md and test/serena/), so the conflict was in CHANGELOG.md alone.

Nothing semantic moved. Both code blobs are byte-identical to the 901ac17e that was pushed before:

The only content change is the changelog entry, which collided with the #2077 performance bullet landing at the same position. git diff c4dc91a7...HEAD -- CHANGELOG.md is 4 added lines and nothing removed, so every upstream entry is intact.

Re-measured on the rebased tree, fresh venv (uv pip install -e ".[dev]", Python 3.11.15):

• base c4dc91a7 with just this test file copied in: 3 failed, 1 passed. This needed PYTHONPATH pinned to the base tree, with solidlsp.__file__ printed to confirm it; without that the editable install resolves solidlsp to the branch checkout and the "base" run reports 4 passed while having tested the fixed code. I hit exactly that on the first attempt and discarded the result.
• head d98a59ad: 4 passed, test_content_modified_retry.py + test_pdeathsig.py + test_process_group_cleanup.py: 20 passed, 2 skipped (Linux-only).
• ruff check / ruff format --check at 0.12.5 clean on both changed files; ty check at 0.0.24 "All checks passed!" on both. Project-wide, ty reports the same 49 diagnostics with identical text on c4dc91a7 and here, and ruff check . the same 20, so this branch adds nothing to what main already carries.
• git merge-tree --write-tree up/main HEAD is clean, and the API now reports mergeable: true / mergeable_state: unstable where it said dirty.

CI is running on d98a59ad: uv sync (3.11/3.12/3.13/14), build and the spelling job are green, the catch-all, niche, jvm, native and other-langs matrices were still in progress when this was posted, so the catch-all numbers in the description are from the pre-rebase head and are not re-claimed for this one.

The description's base/head hashes and the CI attribution have been updated to match.

Follow-up on the CI state of d98a59ad, since one job turned red after the comment above.

jvm (windows-latest) fails test/solidlsp/nextflow/test_nextflow_basic.py::TestNextflowLanguageServer::test_document_symbols_workflows[nextflow] and ::test_full_symbol_tree[nextflow] — 2 failed, 162 passed, 39 skipped, 2148 deselected in 209.42s, both assertions reporting an empty Nextflow symbol tree (job log https://github.com/oraios/serena/actions/runs/35442324023/job/105895171222).

What I can establish from here: the same job is green on c4dc91a7, was green on 901ac17e (which carries this exact ls_process.py blob b4c2e94d63aa…), and is green on #2080 and #2081. The two commits the rebase pulled in touch no src/solidlsp or test/solidlsp file. The failure shape also differs from the one this PR fixes: a write failure on this path produces LanguageServerTerminatedException or the caller's timeout, whereas these two tests received a response whose tree was empty. What I cannot do is reproduce a Windows + Nextflow server locally, so I am not claiming a cause — a re-run of that job would tell us whether it is transient, and if it fails again on a re-run I will treat it as mine and dig in.

Everything else that has finished is green: Codespell, Docs Build, Python Versions, CodeQL, build, the four uv sync jobs, jvm (ubuntu|macos), native (windows-latest), other-langs (macos-latest). catch-all (all three OSes, which is where ruff, ty and test_stdio_send_failure.py run), niche (ubuntu), jvm (windows), native (ubuntu|macos) and other-langs (ubuntu|windows) were still in progress at the time of writing.

CI rollup for d98a59ad once the matrices settled, replacing the "in progress" list above (read back from commits/d98a59ad/check-runs at 12:28Z):

• catch-all (ubuntu-latest) success — that is the job where ruff, ty and test/solidlsp/test_stdio_send_failure.py run, so the four tests and both gates are green in CI on this head, not just locally. catch-all (macos-latest) success; catch-all (windows-latest) was still running when this was posted.
• niche (ubuntu-latest) success here. The R-server failures described in the description were on the pre-rebase head, and this head does not reproduce them.
• jvm (ubuntu|macos), native (ubuntu|macos|windows), other-langs (all three OSes), Codespell, Docs Build, Python Versions, CodeQL, build, all four uv sync: success. deploy: skipped.
• jvm (windows-latest) failure, the two Nextflow symbol tests only, as described in the previous comment.

So one red job, in a language-server group this change does not touch, on the one OS where I cannot run it.

Supplementing my 09-20 comment above, because I have since found the evidence that turns "probably not mine" into "demonstrably not mine", and it also answers the re-run question.

Heads that do not contain this change fail the same leg with the same signature. Their src/solidlsp/ls_process.py is byte-identical to main's (blob ae46a02782b3…, i.e. pre-change), and the two nextflow assertions fail exactly as here (2 failed, 162 passed, 39 skipped, Found: []) — jobs 104390489279, 104532935312, 104600000563, 104633136836, 106056610541, 106112245414, 106112261277, 106112264069, the most recent started 2026-09-20T17:23Z.

Conversely, the blob this PR introduces, b4c2e94d63aa…, passed jvm (windows-latest): job 104540410360 on c514edac0 (and also on 7e5f463a, 901ac17e). So the same file content is green on one day and red later on heads that never touched it, which points at the Nextflow server/runner side rather than at this diff — consistent with the didOpen class upstream tracked in #1871 / #1872, and with forcing my new path against a real Nextflow server producing LanguageServerTerminatedException rather than an empty tree.

Two clarifications to what I wrote on 09-20, both of which make this easier to read:

• My _send_payload change does not raise out of the send; it cancels registered requests and returns. A notification registers nothing, so the didOpen-drop repro fails identically on base code. That is why the failure shape (Found: []) is not reachable from this change.
• My four new tests were deselected in that Windows job (203 selected on both base and head), so that leg never ran them at all.

I still cannot run Windows here, so I am not naming a root cause — I am only reporting that the evidence does not implicate this diff. Local macOS controls, which I offer as controls and not as Windows evidence: test_stdio_send_failure.py 4/4, nextflow 7/7, scan-flush 2/2.

Checking in before this goes stale. fix(solidlsp): fail the pending request when the stdin write fails (head 4595677) — a failed stdin write during a request previously left the request pending forever; it now fails the pending request. CI is green and the CLA check is satisfied.

Is there anything blocking review, or is this simply lower in the queue? I will not ping again.

@feiiiiii5 this is resolving an issue which was closed and marked as invalid.
Please read the comments in the issue. If you still think something needs to be done, please let me know. Otherwise I will close this.

I read the thread. @tagawa0525's point stands, and I have reproduced it on current main (b8b40955) rather than only quoting his 2026-09-14 transcript.

Your reading covers the case you were describing — a request already pending when the server dies is failed by the reader thread. What is left is the request registered after that thread has exited, and nothing on main can fail it: _send_request_once registers into _pending_requests at ls_process.py:351 and only then writes at :353, _send_payload swallows the write failure with a bare return at :666-669, and _cancel_pending_requests has already run once from the reader thread at :631 ("Cancelling 0 pending"). The only completion paths left are _response_handler (unreachable — the process is gone) and the TCP-only cancellations at :778/:826. So the request is failed only by raise TimeoutError in get_result at :109.

Driving the real StdioLanguageServer against a fake server subprocess that reads stdin and never replies, on b8b40955 with no PR code:

INFO  solidlsp.ls_process: Language server stdout reader thread has terminated
INFO  solidlsp.ls_process: Cancelling 0 pending language server requests
[step4] ls.is_running() == False
DEBUG solidlsp.ls_process: Starting: Request[method='textDocument/references', status='pending', request_id=1]
ERROR solidlsp.ls_process: Failed to write to stdin: [Errno 32] Broken pipe
[step5] exception type : builtins.TimeoutError
[step5] elapsed        : 8.00s (request timeout = 8.0s)
[after] _pending_requests == {1: Request[... status='pending' ...]}

Same script, only ls_process.py swapped for this branch: SolidLSPException with cause=LanguageServerTerminatedException: Stdio send error; Cause: [Errno 32] Broken pipe, elapsed 0.00s, _pending_requests empty.

Two precisions, since #2003 is the natural place to push this back to. This is not the #2003 claim — that was about a timeout reaching the restart path, and you and @tagawa0525 agreed it should not: a response that times out does not imply the server is broken. This PR never turns a timeout into a restart; it only converts a detected pipe failure into LanguageServerTerminatedException, which is what TCPLanguageServer._send_payload already does on a send error at :778. A request that times out for any other reason still raises a bare TimeoutError.

If you would rather this not be a PR — it is a behaviour change to an error path, and #2004 is closed as invalid, so the bar for reopening it is yours, not mine — say so and I will close it. What I would ask is that it is not closed as "the reader thread already handles it", because that is the part I can now show is true for the pending case and false for this one.

Thanks for your contribution. My point was that this issue cannot really occur in the context of Serena, but since we should treat SolidLSP as a library (which could be used independently), the change you proposed makes sense.