mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 17:57:54 -10:00
* 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]>
306 lines
11 KiB
Go
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
|
|
}
|
|
}
|
|
}
|