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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
WalkthroughClient notification handling now uses a bounded FIFO queue and a dedicated dispatch goroutine. Handlers can call ChangesLSP notification dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LSPServer
participant readLoop
participant notificationLoop
participant DiagnosticSession
participant Manager
LSPServer->>readLoop: Send publishDiagnostics
readLoop->>notificationLoop: Enqueue notification with receipt sequence
Manager->>DiagnosticSession: Capture global notification baseline
notificationLoop->>DiagnosticSession: Handle diagnostics with sequence
DiagnosticSession-->>Manager: Accept only sequence newer than baseline
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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! |
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 the diagnostics receipt boundary when dispatching asynchronously
internal/lsp/client.go:181
Manager.CheckcapturespublishBaselinebefore it sends the next document version, butpublishCountis incremented only when the queued callback eventually reachessession.handleNotification. A publish for the previous version can therefore already be innotifyQueuewhen a laterChecktakes its baseline; when that old callback is eventually run, it looks newer than the baseline and satisfieswaitForDiagnostics. Since version-lesspublishDiagnosticsmessages are intentionally accepted, the stale result is then returned for the new text (and the 300ms debounce can finish before the real response arrives). The synchronous reader never had this receipt-vs-dispatch gap. Carry a receive sequence/timestamp through the queue and baseline against receipt, or otherwise ensure pre-sync queued diagnostics cannot satisfy the new check; add a regression for queued version-less diagnostics. -
[P2] Put a failure policy on the lossless notification backlog
internal/lsp/client.go:229
The lossless, non-blocking design correctly avoids the old deadlock, but it replaces pipe backpressure with an unbounded slice of fulljson.RawMessagepayloads. A language server can keep emitting notifications faster than the single handler drains them (or while a re-entrant handler is waiting for a response), andreadLoopwill parse and retain every frame until Zero exhausts its heap. This does not require a permanently stuck handler; any sustained producer faster than the consumer is enough. The claimed bounded queue and the 512-message test do not cover this case. Define an observable overload policy—such as failing/evicting the LSP client at a high-water mark—rather than retaining unbounded protocol input.
…n backlog Addresses jatmn's two Gitlawb#759 review findings: - Manager.Check captured its diagnostics baseline from publishCount, which only advances once session.handleNotification actually runs for a URI. Dispatch happens off a queue now, so a publish for a version Check is about to supersede could already be sitting received-but-undispatched at baseline-capture time, then run afterward and look newer than the baseline purely because handling lagged receipt. Since many servers omit the version field, the existing staleness check in handleNotification (positive-version only) couldn't catch it either. Stamp every notification with a receipt sequence at enqueue time (Client.NotificationSeq), carry it through the queue to the handler, and baseline against receipt instead of against how many publishes have been processed. - The lossless notification queue had no failure policy: a language server sustaining a higher notification rate than the single handler can drain (no stuck handler required) would retain every full json.RawMessage payload without bound. Add notifyQueueLimit; exceeding it fails and closes the client observably, letting the manager's existing dead-session eviction take over on next use, instead of growing the queue until the heap gives out. Both regression tests are verified to fail against the pre-fix code: the receipt-vs-baseline race was reproduced directly against the pre-fix API (confirmed real, not hypothetical) before porting the test to the fixed API, and the overload test was confirmed to fail with the limit check removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed [P1] Preserve the diagnostics receipt boundary when dispatching asynchronously (
Fixed by stamping every notification with a receipt sequence at enqueue time ( Added [P2] Put a failure policy on the lossless notification backlog ( The lossless design correctly avoided the old deadlock but replaced pipe backpressure with an unbounded slice of full Added Added Both regression tests verified to have teeth: the receipt/baseline race was proven against the actual pre-fix code before being ported to the fixed API, and the overload test was confirmed to fail with the limit check temporarily removed. Validation:
@jatmn ready for another look. |
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
-
[P2] Bound retained notification bytes, not just notification count
internal/lsp/client.go:263
notifyQueueLimitpermits 4,096 complete peer-controlledjson.RawMessagepayloads to accumulate while a handler is blocked. A language server sending 1 MiB diagnostics or custom notifications can therefore retain several GiB before this count-based limit closes the client, which defeats the new overload policy and can OOM Zero. Apply a byte budget or reject oversized frames before retaining the backlog. -
[P2] Wake a diagnostic wait when overload closes the client
internal/lsp/documents.go:169
The 4,097th queued notification callsfailPending, butwaitForDiagnosticsonly waits on its publish channel or the caller context. The overflow clears the queue without waking that waiter, so aManager.Check(context.Background(), ...)that is already waiting blocks forever (and the normal diagnostics path waits its full timeout) even though the client has been deliberately failed. Observeclient.closedor otherwise release the waiters when closing the client. -
[P2] Restore the delayed PID-file regression coverage dropped by the stale merge
internal/config/command_test.go:151
Relative to the current base, this PR deletesTestAssertProcessTerminatedWaitsForDelayedPIDFileand its polling helper, which were added by main's#802to cover atomic PID-file publication after the firstENOENT. The remaining helper reads immediately, so the Windows startup-race coverage is lost even though this LSP change has no config-test scope. Rebase onto the current base or restore that test/helper.
…sue-758-lsp-notification-deadlock
Track retained notification payload bytes in addition to queue length and fail the client before the backlog exceeds 16 MiB. Wake diagnostic waiters when that failure closes the client, preserving already-received fresh diagnostics during debounce. Add race-enabled regression coverage for byte overload cleanup and client-failure wakeup. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b80c-717b-b041-5d5135a61d96 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manager_test.go`:
- Around line 457-475: Use separate timeout deadlines for the two sequential
polling loops in the test: create one deadline for waiting on NotificationSeq()
to reach 2, then reset the deadline immediately before waiting for
client.IsClosed(). Keep both existing timeout failures and polling behavior
unchanged.
🪄 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 Plus
Run ID: e0cfd15f-668b-49f2-9bdc-3aaaded5ea10
📒 Files selected for processing (4)
internal/lsp/client.gointernal/lsp/client_test.gointernal/lsp/documents.gointernal/lsp/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/lsp/client_test.go
- internal/lsp/documents.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] Stamp receipt sequence when the frame is read, not when it is enqueued
internal/lsp/client.go:208
Async dispatch introduced a window synchronousreadLoopdid not have:readMessageconsumes the frame, thenjson.UnmarshalandenqueueNotificationrun later on the same goroutine whileManager.Checkcan callpublishBaseline()on another. A delayedtextDocument/publishDiagnosticsfor superseded text can therefore be assignedseq > baselineeven though it was already on the wire before the new sync. Version-less publishes then passwaitForDiagnosticsand reach the caller with stale findings — the same receipt-boundary classTestPublishBaselineRejectsAlreadyQueuedPublishcovers for queue ordering, but via read/enqueue reordering. Stamp sequence (or otherwise pin the receipt boundary) at read time, or holdnotifyMuacross read-to-enqueue for notifications. -
[P2] Finish the notification-drain logic consistently
internal/lsp/documents.go:171
ce95206addednotificationsDone()draining forclient.closedin theseq <= baselinewait, but the same concern is only partly applied elsewhere. (1) Thectx.Done()arm at line 171 never waits on drain and only re-checks handledpublishSeq; if cancellation wins while apublishDiagnosticsframe is already accepted but still queued,Manager.Checkcan return(nil, nil)despite wire receipt, and the inner drainselectat line 186 can still letctx.Done()abort before the worker finishes. (2) The debounce-phaseclient.closedarm at line 214 returnstrueimmediately without drain or re-check, soCheckcan read an earlier handled publish fromdiagnosticsForwhile a newer accepted publish is still queued. Please apply the same drain-then-recheck pattern on both paths, with regressions for bounded-context cancellation and for debounce-plus-closure with a follow-up publish still in the queue. -
[P3] Use separate polling deadlines in EOF/drain tests
internal/lsp/manager_test.go:457
TestWaitForDiagnosticsDrainsAcceptedPublishAfterTransportEOFandTestClientDrainsAcceptedNotificationsAfterTransportEOF(internal/lsp/client_test.go:482) reuse one one-second deadline for waiting onNotificationSeq()and then forclient.IsClosed(). Under slow CI scheduling the first loop can consume most of the budget and make the second loop fail intermittently. CodeRabbit's inline request on this is still valid — reset the deadline before each wait loop. -
[P3] Cap
Content-Lengthbefore allocating the frame body
internal/lsp/client.go:457
notifyQueueByteLimitis enforced only inenqueueNotification, butreadMessagealready allocates the full body first. A single oversized notification can therefore bypass the new byte policy beforefailPendingruns. This is a gap in the overload feature added here, not a regression in the read path itself. Reject or capContent-Lengthbeforemake([]byte, contentLength)if you want the byte budget to be authoritative.
Follow-up (not blocking this PR)
These are real hardening opportunities, but they predate or sit outside the core deadlock/receipt fixes and I would not block merge on them alone.
- Baseline before
sync(internal/lsp/manager.go:76):publishBaseline()has run beforesync()onmainas well. A version-less publish received in that gap can still satisfy a later wait. Moving the baseline snapshot to aftersync(or tying freshness to committed document version) would narrow that window; it is not a regression introduced by async dispatch. - Read-then-close before enqueue (
internal/lsp/client.go:208): a frame decoded just beforenotifyClosedis set can be dropped at enqueue. The window is very narrow and mostly overlaps intentional post-Closerejection duringServer.Shutdown; worth hardening only if you want wire-consumed frames to be lossless up to the close boundary.
Stamping a notification's receipt seq at enqueue left a window async dispatch had opened: readMessage consumes the frame, then json.Unmarshal and the enqueue run later on the same goroutine while another can capture a publishBaseline. A publishDiagnostics for superseded text could land above a baseline taken after it was already off the wire and — most servers omit the version field the staleness check needs — reach the caller as the answer for the new text. The read loop now stamps every frame the moment it leaves the wire and hands that seq to the enqueue, so decoding cost sits outside the boundary. ReceiptSeq replaces NotificationSeq: the clock counts frames, and gaps in it are fine. Notification drain handling is now uniform. Every exit from waitForDiagnostics catches the worker up on notifications it already accepted before reading what was recorded, instead of only the seq<=baseline closure arm doing so. That covers cancellation racing an accepted-but-queued publish (which used to make Check return (nil, nil) for text the server had answered), and the debounce closure arm returning while a newer publish was still queued (which handed the caller the older one). After closure the drain is bounded — admission is shut and a handler's re-entrant Call fails immediately — so it is no longer cut short by ctx; on a live client the catch-up is bounded by a short grace instead. Also cap Content-Length before allocating the body, so the notification byte budget is authoritative rather than measured after a huge frame is materialized. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all four findings in [P1] Receipt is stamped at read time. Regression: [P2] The drain logic is uniform now. Every exit from The two policies differ deliberately:
Regressions: [P3] [P3] Separate polling deadlines. Both EOF/drain tests ( I left the two follow-ups alone as non-blocking, per your note. Validation
|
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/documents.go (1)
32-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale API name in the comment:
Client.NotificationSeqno longer exists.The clock this map is stamped from is
Client.ReceiptSeq(client.go:391). Pointing readers at a removed symbol defeats the purpose of the note, especially since the whole point of this PR is which clock wins.📝 Proposed fix
- publishSeq map[string]int64 // uri -> receipt seq of latest publish (see Client.NotificationSeq) + publishSeq map[string]int64 // uri -> receipt seq of latest publish (see Client.ReceiptSeq)🤖 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/documents.go` at line 32, Update the comment for the publishSeq map to reference Client.ReceiptSeq instead of the removed Client.NotificationSeq symbol, preserving the existing explanation that the map stores the latest publish receipt sequence.
🧹 Nitpick comments (2)
internal/lsp/documents.go (1)
260-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe closed-client drain is unbounded — consider a generous cap.
<-drainedhas no ceiling and no ctx escape. The reasoning holds for the session's own handler (parse + record) and for re-entrantCalls failing fast post-closure, butSetNotificationHandleris public: any handler that blocks on something other than a client call wedgesManager.Checkforever. A generous absolute cap (seconds, not the 50ms grace) would preserve the "last chance for these frames" intent while keeping teardown provably finite — matching client.go's own stance that "a user handler may be blocked".♻️ Sketch
if s.client.IsClosed() { - <-drained - return + // Bounded generously: the backlog cannot grow, but a wedged third-party + // handler must not pin this caller forever. + timer := time.NewTimer(closedDrainCap) + defer timer.Stop() + select { + case <-drained: + case <-timer.C: + } + return }🤖 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/documents.go` around lines 260 - 267, Bound the closed-client drain in the surrounding Manager.Check flow instead of waiting on drained indefinitely. Add a generous absolute timeout of several seconds, distinct from the existing 50ms grace period, and return once either drained completes or the cap expires; preserve the current immediate-drain behavior when it finishes within the cap.internal/lsp/manager_test.go (1)
645-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid test — one coupling to keep in mind.
This test pins the post-closure drain as unbounded by setting
catchUpGrace: 0. If the bounded-cap suggestion ondocuments.goLines 260-267 is adopted, that cap must be a separate constant (notcatchUpGrace), or this test starts asserting the opposite of what it intends.🤖 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/manager_test.go` around lines 645 - 753, Keep the post-closure notification drain in waitForDiagnostics independent from session.catchUpGrace, which this test sets to zero to require an unbounded drain even after context cancellation. If adding a cap in the documents.go closure path, introduce and use a separate constant so catchUpGrace continues to govern only the live-client grace period.
🤖 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/documents.go`:
- Line 32: Update the comment for the publishSeq map to reference
Client.ReceiptSeq instead of the removed Client.NotificationSeq symbol,
preserving the existing explanation that the map stores the latest publish
receipt sequence.
---
Nitpick comments:
In `@internal/lsp/documents.go`:
- Around line 260-267: Bound the closed-client drain in the surrounding
Manager.Check flow instead of waiting on drained indefinitely. Add a generous
absolute timeout of several seconds, distinct from the existing 50ms grace
period, and return once either drained completes or the cap expires; preserve
the current immediate-drain behavior when it finishes within the cap.
In `@internal/lsp/manager_test.go`:
- Around line 645-753: Keep the post-closure notification drain in
waitForDiagnostics independent from session.catchUpGrace, which this test sets
to zero to require an unbounded drain even after context cancellation. If adding
a cap in the documents.go closure path, introduce and use a separate constant so
catchUpGrace continues to govern only the live-client grace period.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d443d42f-7352-4a6e-8e9a-beddfe9d36be
📒 Files selected for processing (4)
internal/lsp/client.gointernal/lsp/client_test.gointernal/lsp/documents.gointernal/lsp/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/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
- [P2]
catchUpNotificationscan return before all accepted notifications are handled
internal/lsp/documents.go:268
b0722ffmade everywaitForDiagnosticsexit callcatchUpNotifications()soManager.Checkreads diagnostics only after the worker has applied everything already accepted. On a live client the helper snapshotsacceptedNotificationSeq()once, then returns as soon ashandledThrough() >= target. While it is waiting,readLoopcan still accept anotherpublishDiagnostics(or any notification) with a higher receipt seq; when the worker finishes the stale target,handled >= targetbecomes true and the function returns even though the newer item is still queued. Debounce/timer/ctx.Done()success arms then returntrueanddiagnosticsForcan answer with an older publish while a newer accepted publish for the same URI is still pending — the same “newest received, not the older one that woke the wait” failure the closed-client drain regression was written for, but on the live path. This is PR-owned:mainhad no catch-up helper because the handler ran inline; async dispatch created the gap and this snapshot leaves it open even when grace has not expired. Please loop untilhandledSeqcatches the currentacceptedNotificationSeq()(or equivalent), not a one-shot snapshot, and add a live-client regression where a follow-up publish is accepted during catch-up behind a busy worker.
Follow-up (not blocking this PR)
-
Stamp→enqueue seam in catch-up (
internal/lsp/documents.go:268,internal/lsp/client.go:228):catchUpNotificationswaits onacceptedNotificationSeq()while baselines useReceiptSeq()stamped before decode/enqueue. ApublishDiagnosticsframe can sit in the narrow unmarshal window while catch-up thinks the worker is caught up. The window is much smaller than the enqueue/baseline raceb0722fffixed and overlaps the read-then-close hardening you already listed as non-blocking; worth tightening only if you want wire-to-handler losslessness end to end. -
Production grace vs cancellation test (
internal/lsp/manager_test.go:572,internal/lsp/documents.go:168):TestWaitForDiagnosticsCatchesUpOnContextCancellationuses a five-secondcatchUpGrace; production sessions use50ms. The test therefore does not exercise the live-client timeout path. That timeout is intentional — unbounded catch-up on a live client would hangCheckpast an expired caller deadline — so(nil, nil)after grace under sustained worker backlog is a documented tradeoff, not an oversight. A production-grace regression would still be useful if you want the contract spelled out in tests.
Catch-up snapshotted the accepted sequence once, but the reader keeps running while it waits. The worker finishing the item that was last at the start satisfied the check, so catch-up returned with a newer publish for the same URI still queued — and the caller read diagnosticsFor and got the older one. That is the failure the closed-client drain exists to prevent, reproduced on the live path by the async dispatch this PR introduces. The target is now re-read every pass. This cannot spin forever: the live-client grace already caps the wait no matter how fast the server publishes, and a closed client still drains unconditionally because its backlog cannot grow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed in [P2] Catch-up follows the target instead of snapshotting it. You're right, and it is the same bug I wrote the closed-client drain regression for — I just left the live path holding a stale number. The target is now re-read every pass. It cannot spin: the live-client grace already caps the wait however fast the server publishes, and a closed client still drains unconditionally because its backlog cannot grow. The regression took two attempts, and the first one was wrong in a way worth describing. My initial version asserted The version that landed pins the state instead of racing it: the handler for "second" blocks, so at the moment the stale target is satisfied the worker is provably inside that handler with the newer publish unrecorded. The assertion is then that catch-up has not returned there. Against the snapshot implementation it fails with Optional: production-grace regression. Taken. Not taken: the stamp→enqueue seam in catch-up. Agreed it is real, but it is a strictly smaller window than the one Validation: |
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
- [P2] Required macOS smoke check is failing on head
dd0a7fa
.github/workflows/ci.yml(Smokemacos-latest)
GitHub Actions run 30453340514 reportsSmoke (macos-latest)failure on commitdd0a7fa; Ubuntu, Windows, Performance Smoke, Security & code health, and Zero Review on the same head all passed. The job runsgo test ./..., and this PR’s only production changes are ininternal/lsp, so the failure is PR-owned even though the exact failing test could not be confirmed here — job logs require repository authentication and were not available in this review environment. Please reproduce onmacos-latest, identify the failing test from the job log, fix the underlying defect or stabilize the test, and re-run the full matrix before merge.
Follow-up (not blocking this PR)
-
dd0a7facatch-up re-read looks responsive to the prior P2 snapshot finding.catchUpNotificationsnow re-readsacceptedNotificationSeq()each pass (internal/lsp/documents.go:271-281), which addresses the enqueued-but-still-behind case fromb0722ff. I do not have a remaining actionable defect against that specific request on current head. -
Stamp→enqueue seam in catch-up (
internal/lsp/documents.go:279,internal/lsp/client.go:228-240):ReceiptSeq()advances at read whileacceptedNotificationSeq()advances at enqueue, so catch-up can theoretically return while a frame is still in the read loop’s unmarshal window. That is a much narrower window than the queue/backlog racedd0a7fafixed, and it was already listed as non-blocking follow-up on the prior round. Worth hardening only if you want wire-to-handler losslessness end to end. -
Live-client grace tradeoff (
internal/lsp/documents.go:168,:287-288): productioncatchUpGraceis 50ms and catch-up deliberately gives up afterward; debounce success arms then returntruewithout a post-catch-uppublishSeqre-check likefreshAfterCatchUpuses on the pre-baseline cancel/close paths. That asymmetry is intentional — unbounded live catch-up would hangCheckpast caller deadlines — and is spelled out inTestCatchUpNotificationsGivesUpAfterTheLiveGrace. Not a merge blocker unless you want to change that contract. -
Baseline before
sync(internal/lsp/manager.go:76):publishBaseline()still runs beforesync(); a version-less publish arriving in that gap can satisfy a later wait. That window predates async dispatch. -
Closed-client drain with wedged third-party handler (
internal/lsp/documents.go:265): post-close catch-up waits unbounded onnotificationDrained. Production uses the session’s fast handler; exportedSetNotificationHandlercould still wedgeManager.Checkif a handler blocks offClient.Call. -
Stale API name in comment (
internal/lsp/documents.go:32):publishSeqstill referencesClient.NotificationSeq; the clock isClient.ReceiptSeq().
Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <amp@ampcode.com>
|
PierrunoYT addressed the macOS smoke failure in The failing regression waited for Also updated the stale Validation:
Ready for CI and maintainer re-review. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. I re-verified on current head (7f09894) rather than standing on the earlier rounds, and the concurrency story holds up under everything I threw at it.
I checked each guarantee by mutating the production code and confirming the suite goes red:
- Async dispatch is load-bearing. Putting the handler back inline in
readLoop(internal/lsp/client.go:240) reproduces #758 exactly — the run hangs and the goroutine dump showsreadLoopparked in the handler atclient.go:244with the test goroutine blocked inwriteMessage.TestClientDrainsAcceptedNotificationsAfterTransportEOFcatches it. - FIFO ordering. Flipping
dequeueNotificationto pop the tail fails three tests, including order across the 512-message burst. - Both backlog limits are real. Dropping either the count check or the byte check in
enqueueNotificationfails its own test; neither is redundant. - Oversized frames. Removing the
maxFrameBytescheck failsTestReadMessageRejectsOversizedFrame. - Post-close admission. Removing the
notifyClosedguard failsTestClientDropsNotificationsAfterClose. - Closed-client drain. Dropping
<-drainedincatchUpNotificationsfails two manager tests. - The
dd0a7facatch-up re-read. Restoring the one-shot snapshot failsTestCatchUpNotificationsFollowsPublishesAcceptedWhileWaiting. markHandledafter the handler. Moving it before the handler call fails two tests.- The
<-s.client.closedwakeups inwaitForDiagnostics. Removing them fails two tests.
Worth calling out separately: readMessage on main did make([]byte, contentLength) straight off a peer-supplied header with no ceiling. The 16 MiB cap closes an unbounded-allocation path that predates this PR.
On the macOS failure from the dd0a7fa round — I couldn't reproduce anything like it. go test ./internal/lsp -count=8 is clean, and -count=6 under 3× CPU oversubscription is also clean (285s vs 27s unloaded), which is the shape of load that usually shakes out the timing-sensitive tests here. CI is green on head across all five jobs.
One gap, which is why I'm writing this up rather than just approving silently:
TestPublishBaselineRejectsFrameReadBeforeBaseline doesn't pin the fix it's named for — internal/lsp/manager_test.go:336. The test calls client.stampReceipt() itself and never drives readLoop, so what it actually asserts is the seq comparison, not where the stamp is taken. I moved the stamp from before the json.Unmarshal (client.go:228) down into the enqueue call site — i.e. reverted b0722ff — and the entire package stays green. The P1 from round three can be silently reverted by anyone refactoring readLoop, with no test to stop them.
There's a deterministic way to pin it with no timing dependence: under read-time stamping, a frame that is not a notification still consumes a receipt number ("gaps in it are fine", per your own comment); under enqueue-time stamping it doesn't. So:
client := NewClient(clientReader, io.Discard)
// A response frame: read off the wire, but it never reaches enqueueNotification.
writeMessage(serverWriter, map[string]any{"jsonrpc": "2.0", "id": 1, "result": nil})
// Poll briefly — ReceiptSeq() must become > 0.I ran that against both variants: passes on head, fails with receiptSeq = 0 as soon as the stamp moves to enqueue.
Not blocking the merge — the production code is correct, this is only about keeping it that way. Fine by me either in this PR or as a follow-up.
Two things I'm deliberately not re-raising: the unbounded <-drained for a closed client with a third-party SetNotificationHandler (documents.go:265) — the only in-repo handler is session.handleNotification, which parses, records and closes waiter channels, none of which can block, and both jatmn and CodeRabbit have already logged it; and the Client.NotificationSeq comment at documents.go:32, which is already fixed on head.
Regression test for the gap flagged in review pullrequestreview-4834747507 on Gitlawb#759: TestPublishBaselineRejectsFrameReadBeforeBaseline calls stampReceipt directly and never drives readLoop, so it doesn't pin where the stamp is taken. A response frame (read but never enqueued as a notification) distinguishes the two call sites directly. Co-authored-by: Cursor <cursoragent@cursor.com>
9fc9ff1
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