Keep LSP server alive after response marshal failures

[John Favret (johnfav03)]

Addresses an issue uncovered in #4601 (comment)

A deeply nested textDocument/selectionRange result can contain a SelectionRange.Parent chain exceeding the JSON encoder's maximum nesting depth. The resulting marshal error previously propagated from writeLoop, which cancelled the server context and terminated the entire connection, causing the client to report server connection closed prematurely.

With the fix, a typed MessageMarshalError was introduced to differentiate serialization failures from other issues that should be fatal. writeLoop now logs the failure, sends an internal error response for the request ID, and continues processing subsequent messages.

[Copilot]

Pull request overview

Keeps the LSP connection alive when an outgoing response cannot be serialized.

Changes:

• Introduces a typed JSON marshal error.
• Converts failed response serialization into an internal-error response.
• Adds regression coverage confirming subsequent responses are delivered.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/lsp/server.go	Handles response marshal failures without terminating the write loop.
internal/lsp/server_marshal_failure_test.go	Tests recovery from an excessively nested response.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Jake Bailey (jakebailey)]

Handling this is good, but, I am worried about the root problem here; we should attempt to handle the nested case too.

[John Favret (johnfav03)]

Handling this is good, but, I am worried about the root problem here; we should attempt to handle the nested case too.

I added a cap of 1000 for selection-range parent chains; it preserves the 999 most specific ranges and the full-file range as the final parent, keeping the response below the encoder's nesting depth limit. If you think there's a better approach let me know and I can implement it

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Gabriela Araujo Britto (gabritto)]

Handling this is good, but, I am worried about the root problem here; we should attempt to handle the nested case too.

I added a cap of 1000 for selection-range parent chains; it preserves the 999 most specific ranges and the full-file range as the final parent, keeping the response below the encoder's nesting depth limit. If you think there's a better approach let me know and I can implement it

I'm curious, what kind of file/AST caused us to have such a deep chain of selection range parents?

[John Favret (johnfav03)]

I'm curious, what kind of file/AST caused us to have such a deep chain of selection range parents?

It was a generated array of ~50K entries (I think these were last names in the repo):

export default [
     'A',
     'B',
     // ...
];

A comma between two entries was removed during editing, and during error recovery the parser treated all the remaining values as comma-separated expressions, wrapping each additional value in another AST node. The cursor ended up beneath about ~18K parent nodes, and that was what produced the deep chain of selection range parents.

[Jake Bailey (jakebailey)]

I'm having trouble understanding the range thing here, are you basically just doing a ring buffer at this point?

Is there any harm in just stopping after its len is too big and just bailing out early?

[John Favret (johnfav03)]

Yeah, it's effectively a ring buffer; I figured this was appropriate here because we discover ranges from broadest to most specific, so stopping early would ignore the selection steps closest to the cursor. If you think it's better to just bail to save traversal cost though I can definitely implement that

[Jake Bailey (jakebailey)]

I think it's fine, I just think the implementation is a little hard to understand, compared to maybe some sort of extracted type?

[Jake Bailey (jakebailey)]

John Favret (@johnfav03) This test turns out to be racy:

--- FAIL: TestWriteLoopRecoversFromUnserializableResponse (2.00s)
    server_test.go:172: timed out waiting for a message (write loop may have died)
FAIL