Files
zero/internal/lsp/documents.go
936aa4e77c fix(lsp): dispatch notifications off read loop (#759)
* fix(lsp): dispatch notifications off read loop

* fix(lsp): keep notification reads non-blocking

* fix(lsp): make the notification queue lossless

Addresses the P1 on #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.

* fix(lsp): stop retaining notifications after Close

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.

* fix(lsp): baseline diagnostics against receipt, bound the notification backlog

Addresses jatmn's two #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 <[email protected]>

* fix(lsp): bound notification backlog bytes

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 <[email protected]>

* fix(lsp): preserve diagnostics across client closure

When a fresh publish and client closure wake a diagnostic waiter together, reconcile against the recorded receipt sequence instead of allowing select order to discard the publish.

Add a regression that repeatedly makes both wakeups ready together and verifies diagnostics handled before closure are preserved.

Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-5dd3-7118-a7a6-fba8f9d5af29

Co-authored-by: Pierre Bruno <[email protected]>

* fix(lsp): drain accepted notifications on close

Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8

Co-authored-by: Pierre Bruno <[email protected]>

* fix(lsp): pin the receipt boundary at read time and finish the drain

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) <[email protected]>

* fix(lsp): follow publishes accepted while catch-up is waiting

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) <[email protected]>

* test(lsp): wait for follow-up notification enqueue

Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef
Co-authored-by: Amp <[email protected]>

* test(lsp): pin receipt stamping to read time, not enqueue

Regression test for the gap flagged in review pullrequestreview-4834747507
on #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 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Amp <[email protected]>
Co-authored-by: Cursor <[email protected]>
2026-08-10 09:50:03 +08:00

306 lines
11 KiB
Go

package lsp
import (
"context"
"encoding/json"
"sync"
"time"
)
// lspServer is the subset of *Server the session needs, so a session can be
// driven by a stub over an in-memory pipe in tests.
type lspServer interface {
Client() *Client
Shutdown(ctx context.Context) error
}
// session wraps one language-server connection with per-document version
// tracking and a diagnostics store fed by textDocument/publishDiagnostics
// notifications. Safe for concurrent use.
type session struct {
server lspServer
client *Client
// catchUpGrace bounds catchUpNotifications on a live client. Zero means "do
// not wait", which is what the hand-built sessions in tests get by default.
catchUpGrace time.Duration
mu sync.Mutex
open map[string]bool // uri -> didOpen sent
versions map[string]int // uri -> current (committed) version
diagnostics map[string][]Diagnostic // uri -> latest published diagnostics
lastPublish map[string]time.Time // uri -> time of last publish
publishSeq map[string]int64 // uri -> receipt seq of latest publish (see Client.ReceiptSeq)
waiters map[string][]chan struct{} // uri -> goroutines awaiting the next publish
fileLocks map[string]*sync.Mutex // uri -> per-document sync serializer
}
func newSession(server lspServer) *session {
s := &session{
server: server,
client: server.Client(),
catchUpGrace: notificationCatchUpGrace,
open: map[string]bool{},
versions: map[string]int{},
diagnostics: map[string][]Diagnostic{},
lastPublish: map[string]time.Time{},
publishSeq: map[string]int64{},
waiters: map[string][]chan struct{}{},
fileLocks: map[string]*sync.Mutex{},
}
s.client.SetNotificationHandler(s.handleNotification)
return s
}
func (s *session) handleNotification(method string, params json.RawMessage, seq int64) {
if method != "textDocument/publishDiagnostics" {
return
}
var payload PublishDiagnosticsParams
if err := json.Unmarshal(params, &payload); err != nil {
return
}
s.mu.Lock()
// Drop a delayed publish for a superseded version: if we've already synced a
// newer document version, an older version's diagnostics are stale and must
// not move the cache backward. Many servers omit the version (0) — only skip
// when the payload carries a strictly-older positive version.
if payload.Version > 0 && s.versions[payload.URI] > payload.Version {
s.mu.Unlock()
return
}
s.diagnostics[payload.URI] = payload.Diagnostics
s.lastPublish[payload.URI] = time.Now()
s.publishSeq[payload.URI] = seq
waiters := s.waiters[payload.URI]
delete(s.waiters, payload.URI)
s.mu.Unlock()
for _, ch := range waiters {
close(ch)
}
}
// sync opens the document on first sight, otherwise sends a full-text change.
// It holds a per-URI lock across the whole compute+notify so two concurrent
// syncs of the same document can't race their didOpen/didChange writes onto the
// wire out of order, and only commits the new version/open state after the
// notification succeeds (a failed Notify leaves tracking unchanged).
func (s *session) sync(ctx context.Context, absPath, languageID, text string) error {
uri := PathToURI(absPath)
lock := s.fileLock(uri)
lock.Lock()
defer lock.Unlock()
s.mu.Lock()
first := !s.open[uri]
nextVersion := 1
if !first {
nextVersion = s.versions[uri] + 1
}
s.mu.Unlock()
var err error
if first {
err = s.client.Notify(ctx, "textDocument/didOpen", map[string]any{
"textDocument": TextDocumentItem{URI: uri, LanguageID: languageID, Version: nextVersion, Text: text},
})
} else {
err = s.client.Notify(ctx, "textDocument/didChange", map[string]any{
"textDocument": map[string]any{"uri": uri, "version": nextVersion},
"contentChanges": []any{map[string]any{"text": text}}, // full-document sync
})
}
if err != nil {
return err
}
s.mu.Lock()
s.open[uri] = true
s.versions[uri] = nextVersion
s.mu.Unlock()
return nil
}
func (s *session) fileLock(uri string) *sync.Mutex {
s.mu.Lock()
defer s.mu.Unlock()
lock, ok := s.fileLocks[uri]
if !ok {
lock = &sync.Mutex{}
s.fileLocks[uri] = lock
}
return lock
}
func (s *session) diagnosticsFor(uri string) []Diagnostic {
s.mu.Lock()
defer s.mu.Unlock()
return append([]Diagnostic(nil), s.diagnostics[uri]...)
}
// publishBaseline snapshots the client's current notification receipt
// sequence, captured before a sync so waitForDiagnostics can require a publish
// that was RECEIVED after this point. This must baseline against receipt, not
// against session.publishSeq (which only advances once handleNotification
// actually runs): dispatch happens off a queue, so a publish for a since-
// superseded version can already be sitting in that queue, still unprocessed,
// at the moment a later Check captures its baseline. If baseline were instead
// "how many publishes has this session recorded so far", that stale publish
// would land afterward, still satisfy "more than baseline", and hand the new
// check diagnostics for the wrong text — silently, since many servers omit the
// version field the staleness check in handleNotification relies on.
// Baselining against receipt sequence closes that: a notification whose frame
// had already come off the wire before this call has seq <= baseline no matter
// when it is decoded, queued, or handled.
func (s *session) publishBaseline() int64 {
return s.client.ReceiptSeq()
}
// notificationCatchUpGrace is the live-client catch-up budget a real session
// runs with. It exists for the case where the caller's own deadline has already
// passed: dropping a publish that is sitting accepted-but-unhandled in the queue
// would report "no diagnostics" for text the server had in fact already answered
// for, so a brief overrun is preferable to discarding it. It stays short because
// the caller is by then out of budget, and it costs nothing in the ordinary case
// — the session's own handler only parses and records, so the worker is
// virtually always caught up already and no wait happens at all. A closed client
// is deliberately not bounded by this; see catchUpNotifications.
const notificationCatchUpGrace = 50 * time.Millisecond
// waitForDiagnostics blocks until a publish newer than baseline arrives for the
// URI and the server then goes quiet for the debounce window, or until ctx is
// done or the client closes. It returns true when a fresh publish was observed
// and false when the wait ended before any did — so a caller can avoid returning
// a stale prior result for newer text. Servers don't signal "analysis complete",
// so the debounce approximates it: once a fresh publish lands, wait debounce for
// a follow-up, resetting on each new publish.
func (s *session) waitForDiagnostics(ctx context.Context, uri string, debounce time.Duration, baseline int64) bool {
for {
s.mu.Lock()
ch := make(chan struct{})
s.waiters[uri] = append(s.waiters[uri], ch)
seq := s.publishSeq[uri]
last := s.lastPublish[uri]
s.mu.Unlock()
if seq <= baseline {
select {
case <-ctx.Done():
s.cancelWaiter(uri, ch)
// Cancellation, like closure below, ends the wait on a signal that
// says nothing about what the server sent, and either can win a race
// against a publish this session has already received. Deciding
// "nothing arrived" straight out of the select would discard it, so
// both settle identically: let the worker finish what it accepted,
// then re-read what got recorded.
return s.freshAfterCatchUp(uri, baseline)
case <-s.client.closed:
s.cancelWaiter(uri, ch)
return s.freshAfterCatchUp(uri, baseline)
case <-ch:
continue // a fresh publish arrived; loop into the debounce check
}
}
remaining := debounce - time.Since(last)
if remaining <= 0 {
s.cancelWaiter(uri, ch)
s.catchUpNotifications()
return true
}
timer := time.NewTimer(remaining)
select {
case <-ctx.Done():
timer.Stop()
s.cancelWaiter(uri, ch)
// A fresh publish did arrive; ctx merely cut the debounce short. Catch
// the worker up regardless: the caller reads diagnosticsFor the moment
// this returns, and a newer publish for this URI may be sitting accepted
// but unhandled — it should get the newest received, not the older one
// that happened to wake this wait.
s.catchUpNotifications()
return true
case <-s.client.closed:
timer.Stop()
s.cancelWaiter(uri, ch)
// Same, for whatever the server managed to publish before it died.
s.catchUpNotifications()
return true // preserve the fresh publish already received before failure
case <-ch:
timer.Stop()
continue // a newer publish arrived; re-arm the debounce
case <-timer.C:
s.cancelWaiter(uri, ch)
s.catchUpNotifications()
return true // quiet for the full window
}
}
}
// freshAfterCatchUp reports whether a publish received after baseline has been
// recorded for the URI, having first let the notification worker finish frames
// it already accepted. Receipt is what the answer turns on: a publish that was
// read off the wire but is still queued would otherwise read as "never arrived".
func (s *session) freshAfterCatchUp(uri string, baseline int64) bool {
s.catchUpNotifications()
s.mu.Lock()
defer s.mu.Unlock()
return s.publishSeq[uri] > baseline
}
// catchUpNotifications waits for the client's notification worker to handle
// every notification it had already accepted, so a decision made right after
// cannot miss a publish that was on the wire before it. It returns immediately
// in the ordinary case, where the worker is already caught up.
func (s *session) catchUpNotifications() {
drained := s.client.notificationsDone()
if drained == nil {
return // hand-built client (some unit tests): no worker to catch up on
}
if s.client.IsClosed() {
// Wait the worker out instead of time-boxing it. Admission is already
// closed so the backlog cannot grow, and a handler's re-entrant Call now
// fails immediately rather than blocking on a response that will never
// come — the drain is bounded, and it is the last chance these frames get.
<-drained
return
}
timer := time.NewTimer(s.catchUpGrace)
defer timer.Stop()
for {
// Re-read the target every pass rather than snapshotting it once. The
// reader keeps running while this waits, so a snapshot goes stale: the
// worker finishing the item that was last when we started satisfies
// `handled >= target` while a newer publish for the same URI sits queued,
// and the caller then answers from diagnosticsFor with the older one —
// the exact failure the closed-client drain exists to prevent, on the
// live path. Growth is not unbounded: the grace below caps the wait no
// matter how fast the server publishes.
handled, advanced := s.client.handledThrough()
if handled >= s.client.acceptedNotificationSeq() {
return
}
select {
case <-advanced:
case <-drained:
return
case <-timer.C:
return
}
}
}
// cancelWaiter removes a still-open waiter (one a publish has not already closed
// and cleared) so it can't leak or be closed twice.
func (s *session) cancelWaiter(uri string, target chan struct{}) {
s.mu.Lock()
defer s.mu.Unlock()
list := s.waiters[uri]
for i, ch := range list {
if ch == target {
s.waiters[uri] = append(list[:i], list[i+1:]...)
return
}
}
}