fix(lsp): dispatch notifications off read loop#759
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a deadlock risk in the LSP client by moving server notification handling off the single protocol read-loop goroutine, enabling notification handlers to safely perform re-entrant Client.Call requests without blocking response delivery.
Changes:
- Added a per-client notification worker loop and bounded queue to serialize notification dispatch off the read loop.
- Updated the client read loop to enqueue notifications instead of invoking the handler inline.
- Added regression tests for re-entrant calls from notification handlers and notification ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/lsp/client.go | Introduces a notification queue + worker goroutine so notification handlers don’t run on the protocol read loop. |
| internal/lsp/client_test.go | Adds tests for re-entrant calls inside notification handlers and for preserving notification ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case msg.Method != "": | ||
| c.mu.Lock() | ||
| handler := c.handler | ||
| c.mu.Unlock() | ||
| if handler != nil { | ||
| handler(msg.Method, msg.Params) | ||
| select { | ||
| case c.notify <- notification{method: msg.Method, params: msg.Params}: | ||
| case <-c.closed: | ||
| return | ||
| } |
| func TestClientNotificationHandlersPreserveOrder(t *testing.T) { | ||
| clientReader, serverWriter := io.Pipe() | ||
| client := NewClient(clientReader, io.Discard) | ||
| defer client.Close() | ||
| defer serverWriter.Close() |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughClient notification handling now uses a FIFO queue and dedicated dispatch goroutine, allowing handlers to make client calls without blocking response processing. Shutdown clears queued notifications, and tests cover ordering, bursts, losslessness, and post-close behavior. ChangesLSP notification dispatch
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LSPServer
participant readLoop
participant notificationLoop
participant NotificationHandler
LSPServer->>readLoop: Send notification
readLoop->>notificationLoop: Queue method and params
notificationLoop->>NotificationHandler: Invoke handler
NotificationHandler->>readLoop: Issue Client.Call
readLoop-->>NotificationHandler: Return response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/lsp/client_test.go`:
- Around line 230-282: Run the concurrent test
TestClientNotificationHandlerCanCallClient with Go’s race detector, using go
test -race ./... locally or in a supported CI environment, and verify that
notificationLoop and Client.Call are race-free.
In `@internal/lsp/client.go`:
- Around line 43-44: Replace the fixed-size notification channel controlled by
notificationQueueSize with an unbounded queue pattern, using an intermediate
goroutine and dynamically growing slice to decouple readLoop from slow
notification handlers. Preserve notification execution order while ensuring
protocol reads never block, including when handlers call Client.Call and more
than 64 notifications arrive before the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35a90c72-04a7-499a-9369-97226e4696f5
📒 Files selected for processing (2)
internal/lsp/client.gointernal/lsp/client_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Preserve notifications when the dispatch queue overflows
internal/lsp/client.go:214
When a notification handler is blocked (including in the re-entrantClient.Callcase this PR adds), a server can send more than 64 notifications before replying.enqueueNotificationdiscards the oldest message, so atextDocument/publishDiagnosticsfor a different URI can be lost permanently. The correspondingsession.waitForDiagnosticsthen times out andManager.Checkreturns no diagnostics even though the server published them. This is the resolved CodeRabbit request in a different form: the follow-up commit trades the read-loop deadlock for silent protocol-event loss, andTestClientNotificationOverflowDropsOldestInOrderexplicitly codifies that loss. Use a lossless non-blocking queue (or another explicit, observable policy that preserves required diagnostics) and add an overflow regression test that asserts delivery of all relevant notifications.
Addresses the P1 on Gitlawb#759. The previous commit traded the read-loop deadlock for silent protocol-event loss: at 64 queued items enqueueNotification discarded the OLDEST message. A textDocument/publishDiagnostics is the server's only report for its URI, so a dropped one makes session.waitForDiagnostics time out and Manager.Check return no diagnostics even though the server published them — and the overflow test codified that loss as intended behavior. The queue is now unbounded: the read loop still never blocks (so a handler may make a re-entrant Client.Call), and nothing is discarded. Both alternatives are worse — blocking on a full buffer is the original deadlock, and dropping loses state the protocol never repeats. Growth is bounded in practice by what the server emits while a handler runs; the backing array is released as soon as the queue drains, so a burst does not retain capacity afterwards. Tests: TestClientNotificationQueueIsLossless replaces the drop-oldest test, and the end-to-end burst test now also asserts that all 512 notifications sent while a handler was blocked in a re-entrant call arrive, in order. Both were checked against the old drop-oldest policy and fail there (64 of 512 delivered, FIFO broken), so they have teeth.
|
Pushed [P1] Preserve notifications when the dispatch queue overflows — you read it exactly right: the follow-up commit traded the read-loop deadlock for silent protocol-event loss, and The two alternatives are both worse, and the code now says so where the decision lives:
On the growth question that motivated the bound: it is bounded in practice by how much the server emits while one handler runs, and Tests, and their teeth
I verified both fail under the old policy rather than assuming it — reintroducing the 64-item drop-oldest cap gives: @coderabbitai on the race detector — I cannot run it on this host and want to be straight about that rather than tick the box: Validation (Windows host, Go 1.26.5): @jatmn — ready for another look. I can't use the reviewer-request button on this repo, hence the mention. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff summary ==='
git diff --stat HEAD^ HEAD -- internal/lsp/client.go internal/lsp/client_test.go || true
git diff --unified=80 HEAD^ HEAD -- internal/lsp/client.go internal/lsp/client_test.go || true
echo '=== client implementation outline ==='
ast-grep outline internal/lsp/client.go --items all --view expanded || true
echo '=== relevant notification and shutdown references ==='
rg -n -C 5 --type go \
'(notificationLoop|enqueueNotification|dequeueNotification|notifyMu|notifyWake|notify|close\(|closed|readLoop|NotificationHandler)' \
internal/lsp/client.go internal/lsp/client_test.goLength of output: 42682 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== client construction and close call sites ==='
rg -n -C 6 --type go \
'(NewClient\s*\(|\.Close\s*\(\)|IsClosed\s*\(\)|failPending\s*\()' \
internal
echo '=== session/process transport lifecycle candidates ==='
rg -n -C 6 --type go \
'(exec\.Command|StdoutPipe|StdinPipe|io\.Pipe|Wait\s*\(|Kill\s*\(|Process\.|client\s+\*Client|Client\s+\*Client)' \
internalLength of output: 165231 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Server lifecycle ==='
sed -n '1,115p' internal/lsp/server.go
echo '=== Test LSP server lifecycle ==='
sed -n '1,115p' internal/lsp/manager_test.go
echo '=== all direct Client Close invocations within LSP ==='
rg -n -C 4 --type go '\bclient\.Close\(\)|\.client\.Close\(\)' internal/lspLength of output: 13491
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/lsp/client.go (1)
190-210: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winShutdown drops queued notifications and doesn't wait for an in-flight handler.
Once
c.closedfires the loop returns immediately, so anything still innotifyQueueis discarded, andClose()returns while a handler may still be executing (including one blocked inClient.Call, which will then fail with the read error). The linked issue asks for defined callback lifecycle/shutdown behavior — either drain-then-exit, or document that post-Closenotifications are intentionally abandoned and that handlers may outliveClose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/lsp/client.go` around lines 190 - 210, Define and implement explicit shutdown behavior for Client.notificationLoop: either drain all queued notifications and wait for any in-flight handler before Close returns, or intentionally abandon them and document that handlers may outlive Close. Update the c.closed handling, notification dispatch, and Close synchronization consistently so Client.Call/read-error behavior matches the chosen lifecycle contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/lsp/client.go`:
- Around line 190-210: Define and implement explicit shutdown behavior for
Client.notificationLoop: either drain all queued notifications and wait for any
in-flight handler before Close returns, or intentionally abandon them and
document that handlers may outlive Close. Update the c.closed handling,
notification dispatch, and Close synchronization consistently so
Client.Call/read-error behavior matches the chosen lifecycle contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ae5688e-795a-4216-bf98-b140ebd1ee67
📒 Files selected for processing (2)
internal/lsp/client.gointernal/lsp/client_test.go
Addresses the shutdown defect CodeRabbit found in the lossless queue. Client.Close signals the worker loop to exit, but readLoop keeps consuming frames until the transport ends — and Server.Shutdown closes the client BEFORE closing stdin, so a language server emitting notifications while it handles shutdown/exit fed an unbounded queue that nothing would ever drain. failPending's closeOnce now marks the queue closed and releases whatever is in it, and enqueueNotification drops instead of appending once that flag is set. Both run under notifyMu, so an enqueue racing with shutdown either sees the flag and drops its item or appends just before it and has the item cleared. Queued work is dropped rather than drained: the worker is gone by definition of close, and callers that need diagnostics collect them before shutting the client down. TestClientDropsNotificationsAfterClose keeps the transport writable after Close, as Server.Shutdown does, and requires 64 subsequent notifications to neither reach the handler nor accumulate. Verified against the unguarded enqueue, where it reports 64 retained notifications.
|
Fixed in [P1] Do not retain notifications after
Both run under Queued-but-unhandled notifications are dropped rather than drained, and the code says why: the worker loop is gone by definition of close, and the caller that needs diagnostics ( Regression test — Verified it has teeth by disabling the guard:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== focused commit diff ==='
git diff --unified=90 d8ef0f3^ d8ef0f3 -- internal/lsp/client.go internal/lsp/client_test.go
echo '=== client symbols ==='
ast-grep outline internal/lsp/client.go --items all --view expanded
echo '=== notification lifecycle references ==='
rg -n -C 8 --type go \
'(closeNotifications|enqueueNotification|dequeueNotification|notificationLoop|notifyClosed|notifyQueue|notifyMu|failPending|closeOnce|func \(c \*Client\) Close)' \
internal/lsp/client.go internal/lsp/client_test.goLength of output: 36975 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== notification handler call sites ==='
rg -n -C 10 --type go '\.SetNotificationHandler\s*\(' internal
echo '=== server shutdown and client ownership ==='
ast-grep outline internal/lsp/server.go internal/lsp/manager.go --items all --view expanded 2>/dev/null || true
rg -n -C 12 --type go \
'(func \(.*\) Shutdown|func \(.*\) Close|\.Close\(\)|SetNotificationHandler|publishDiagnostics|waitForDiagnostics)' \
internal/lsp/server.go internal/lsp/manager.go internal/lspLength of output: 66810
🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Summary
Verification
n- go vet ./...n- go test ./...n- go run ./cmd/zero-release buildn- go run ./cmd/zero-release smoken- git diff HEAD --checknEnvironment limitations
Fixes fix(lsp): re-entrant notification handlers can deadlock #758
Summary by CodeRabbit