fix(lsp): crash-safety — panic recovery, layout canary, race+lint CI - #8
Merged
Merged
Conversation
Range directly over the string in hasCompletedTypeExpr. The rune index was discarded, so the []rune materialization was unnecessary (staticcheck SA6003). Needed for the golangci-lint CI gate added later in this stack.
The parser reads unexported webrpc/webrpc schema/ridl types through unsafe.Pointer struct mirrors. Nothing verified that the mirrored field layouts still match upstream, so a field reorder/resize upstream would silently corrupt hover/goto/diagnostics with no compile error or test failure (audit C2). - Convert the never-read leading mirror fields to blank padding, documenting that they exist only for offset fidelity (also clears the unused linter ahead of the CI lint gate). - Add TestUpstreamLayoutCanary: parse a fixture with known positions and assert every mirrored read (token line/col, error code/message/status, inline-struct arg, parser root) returns the expected value. Verified it fails when a field is inserted ahead of line/col.
go.lsp.dev runs each request in its own goroutine via jsonrpc2.AsyncHandler, and neither jsonrpc2 nor go.lsp.dev/protocol recovers panics. An unrecovered panic in any handler therefore terminates the whole language server, taking down every editor feature until the client respawns it. The unsafe.Pointer parser mirrors make panics a realistic failure mode (audit C1). - Add RecoverHandler middleware: recovers panics, logs method + stack, and replies with a JSON-RPC error when no reply was sent yet. A recovered panic never propagates as a returned error (jsonrpc2 treats that as connection-fatal) and never double-replies. - Wire it in main by replicating protocol.NewServer's setup and wrapping ServerHandler, so it sits inside ReplyHandler and satisfies the reply-exactly-once contract. - Tests cover panic-before-reply, panic-after-reply (no double reply), and pass-through.
CI ran only go build and go test, with no race detector and no lint gate, despite the repo carrying a .golangci.yml and the server being concurrency-bearing and unsafe-heavy. Race and lint regressions could land unblocked (audit I5). - Run tests with -race. - Add a lint step using the repo's pinned golangci-lint and .golangci.yml (no --fix, so CI reports rather than mutates). The codebase is lint-clean as of the earlier commits in this stack.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A malformed document can currently take down the whole language server: go.lsp.dev runs every request in its own goroutine and recovers nothing, so a single panic in a handler — very reachable given the
unsafe.Pointerparser mirrors — kills the process and every editor feature with it. This adds a panic-recovery middleware so a panic degrades to one failed request, plus a canary test that turns silentunsafelayout drift into a red build, and tightens CI to actually catch races and lint regressions.First of three stacked PRs hardening ridl-lsp for production (from a Claude↔Codex audit). This one is the base — safe to merge first, no dependency on the others.
Changes (audit findings)
internal/lsp/recover.go):RecoverHandlerwraps the protocolServerHandler(insideReplyHandler), recovers panics, logs method + stack, and replies a JSON-RPC error when no reply was sent yet. A recovered panic never propagates as a returned error (jsonrpc2 treats that as connection-fatal) and never double-replies. Wired inmain.goby replicatingprotocol.NewServer's setup.internal/ridl/layout_canary_test.go,parser.go): the parser reads unexportedwebrpc/webrpctypes through hand-mirrored structs viaunsafe.Pointer, previously unguarded. Added a canary that parses a fixture with known positions and asserts every mirrored read (token line/col, error code/message/status, inline-struct arg, parser root); converted the never-read leading mirror fields to blank_padding (same layout, documents intent, clears theunusedlinter)..github/workflows/ci.yml): run tests with-raceand add agolangci-lintstep (samego run+.golangci.ymlthe Makefile uses, minus--fix).internal/lsp/semantic_document.go): drop a redundant[]runeconversion (staticcheck SA6003) so the lint gate goes green.Test plan
Canary teeth verified: inserting one bogus field ahead of
line/colin the mirror madeTestUpstreamLayoutCanaryfail (TokenLinereturned 18 instead of 6); reverted.Review
Self-review (2 angles), security-review, and a Codex adversarial pass all returned zero Critical/Important findings. Codex's lone Suggestion —
go run golangci-lintis toolchain-sensitive — is intentionally kept: it matches the repo's existingmake lint, and CI pins Go 1.25.0 viago-version-file.Stack:
ridl-lsp-hardeningThis stack is managed with sdf.