Fix server hang after "Running scheduled diagnostics refresh"

[Copilot]

Two unbounded waits on the critical path fired by Running scheduled diagnostics refresh can deadlock the server when the client is slow to respond, leaving the LSP completely unresponsive until manually restarted.

RefreshDiagnostics — fire and forget

• workspace/diagnostic/refresh always returns null; waiting for it is unnecessary
• Changed to send the request without registering a response channel or blocking
• Late client responses are silently dropped by the read loop (no matching pending entry)
• Short-circuits on context cancellation before sending

updateWatch — per-request 1s timeout on client requests with rollback

• WatchFiles/UnwatchFiles calls hold session.watchesMu while synchronously waiting on client/registerCapability responses
• Any client delay blocks all other updateWatch callers behind the mutex, and if requestQueue fills up, the read loop can't deliver responses — creating a self-locking deadlock
• Each WatchFiles/UnwatchFiles call gets its own 1s context.WithTimeout so earlier calls don't consume the deadline for later ones
• On any WatchFiles failure, ALL newly-acquired watchers in that batch are rolled back (not just the failed one) to keep refcounts clean for retry
• Failed watcher IDs are tracked as "pending" in the registry so updateWatches retries them on the next snapshot update, even when the watcher identity hasn't changed

WatchedFiles.Watchers() — deduplicated at the source

• Watchers() now deduplicates patternsInsideWorkspace and directoriesOutsideWorkspace via slices.Compact(slices.Sorted(...)) before creating FileSystemWatcher objects
• This prevents refcount inflation from duplicate patterns, which would otherwise break the rollback path (duplicate keys inflate refcounts but only the first Acquire is tracked for rollback, leaving stale counts that block future retries)

watchRegistry — encapsulated watch bookkeeping

• Extracted a watchRegistry type into internal/project/watch.go that encapsulates the ref-counted map, mutex, and provides Acquire and Release methods
• Added MarkPending, ClearPending, and IsPending methods to track watcher IDs whose registration failed
• All methods lock mu internally, fully encapsulating the locking discipline so callers never need to manage the mutex directly
• updateWatch now delegates all bookkeeping to the registry instead of manipulating the map directly, making the function easier to follow
• updateWatches checks IsPending for all watcher types (config, program files, typings, auto-imports) and retries as fresh adds when the watcher identity is unchanged but the previous registration failed

Tests

• Added TestUpdateWatchTimeoutAndRollback in internal/project/watchtimeout_test.go using synctest and projecttestutil to verify timeout, rollback, and retry behavior with real session operations
• Uses DidOpenFile to open a file (triggering watches that time out with a mocked slow client), then DidChangeFile with a single character edit (which does not change any watcher identity), followed by GetLanguageService to flush the pending change and trigger updateWatches
• Verifies that the pending tracking causes updateWatches to retry the failed registrations and that the retry succeeds, even though the watcher identities are unchanged
• Asserts that retry watcher IDs match the initial attempt IDs and that at least one retried watcher succeeds

[Copilot]

Pull request overview

Addresses an LSP server hang triggered by “Running scheduled diagnostics refresh” by removing an unnecessary client-response wait and bounding how long watch registration holds a critical mutex.

Changes:

• Make RefreshDiagnostics send workspace/diagnostic/refresh as fire-and-forget (no response channel / no wait).
• Add a 1s timeout for WatchFiles/UnwatchFiles client requests made while holding session.watchesMu.
• Introduce sendClientRequestFireAndForget helper for non-essential client responses.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
internal/project/session.go	Adds a watch request timeout context for client watcher register/unregister calls to avoid indefinite mutex holds.
internal/lsp/server.go	Changes diagnostics refresh to fire-and-forget and adds a helper to send client requests without awaiting responses.

[andrewbranch]

@copilot fix review comments

[Copilot]

@copilot fix review comments

Addressed all review comments in e2074f9e:

• RefreshDiagnostics now checks ctx.Err() before sending
• Timeout context is created after acquiring watchesMu
• WatchFiles failures roll back session.watches bookkeeping and schedule a single retry after 1s

[Copilot]

@copilot fix review comments

All review comments have been addressed across e2074f9e, ccd56581, and 60619b9d.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

internal/project/session.go:1203

• pending watcher IDs are only cleared on successful registration in the newWatcher path. If a watcher times out (MarkPending) and is later removed (updateWatch called with newWatcher == nil), its pending entry is never cleared, so watchRegistry.pending can grow over the lifetime of a session. Consider calling ClearPending for oldWatcher.Watchers().WatcherID when removing/unwatching, or otherwise pruning pending entries when the corresponding watcher is no longer present.

	if oldWatcher != nil {
		w := oldWatcher.Watchers()
		watchers := append(w.WorkspaceWatchers, w.OutsideWorkspaceWatchers...)
		if len(watchers) > 0 {
			var removedIDs []WatcherID
			for _, watcher := range watchers {
				if id, removed := session.watches.Release(watcher); removed {
					removedIDs = append(removedIDs, id)
				}
			}
			for _, id := range removedIDs {
				if err := session.client.UnwatchFiles(watchCtx, id); err != nil {
					errors = append(errors, err)
				} else if logger != nil && newWatcher == nil {
					logger.Log(fmt.Sprintf("Removed watch: %s", id))
				}
			}
		}
	}

[Copilot]

The pending retry path calls updateWatch(ctx, s, ..., nil, newWatcher). If the original updateWatch call partially succeeded (some WatchFiles calls returned nil before a later one timed out), the registry will still contain refcounts for the successful watchers. Retrying with oldWatcher==nil will Acquire again and permanently increment those refcounts (no matching Release), which can prevent future UnwatchFiles from ever firing and leak client registrations. To keep refcounts consistent, consider making the failure path roll back the entire parent watcher’s bookkeeping when any error occurs (not just the failed watcher), or implement a retry helper that registers only missing keys without incrementing refcounts for already-tracked entries.

[Copilot]

watchRegistry is documented as guarding concurrent access with its mutex, but Acquire/Release/MarkPending/ClearPending do not lock r.mu (only IsPending does). Since session.go now locks session.watches.mu directly, the locking discipline is split across call sites, which makes it easy to accidentally call these methods without holding the mutex (data race) and undermines the stated encapsulation. Consider either (1) moving the locking inside watchRegistry methods (and removing external r.mu access), or (2) updating the type/method docs and API to make the required locking explicit (e.g., AcquireLocked/ReleaseLocked or a WithLock helper) so future callers can’t misuse it.

[Copilot]

updateWatch creates a single watchCtx with a 1s deadline and reuses it for all WatchFiles/UnwatchFiles calls in this update. If multiple registrations/unregistrations are needed, later calls may inherit very little time remaining (e.g., first call takes 900ms → second gets ~100ms), causing avoidable timeouts even when the client is responsive. Consider creating a fresh per-request timeout context inside the WatchFiles/UnwatchFiles loops (or otherwise resetting the deadline per call) while still ensuring the mutex is not held indefinitely.

[Copilot]

On WatchFiles error, updateWatch rolls back bookkeeping via session.watches.Release(watcher) exactly once for that watcher. If the input watcher list contains duplicate patterns (same fileSystemWatcherKey), Acquire will increment the refcount multiple times but only one client registration is attempted; a failed registration would then leave the refcount >0, preventing future retries from ever treating that watch as “new”. This is reachable because some glob lists are only sorted (not deduped) before becoming watchers. Consider deduping watcher keys before calling Acquire, or tracking how many increments were applied per key and fully rolling them back on registration failure.

[andrewbranch]

@copilot address review comments

[Copilot]

@copilot address review comments

Addressed all five review comments in 22abe61:

1. Partial success refcount leak: On any WatchFiles error, ALL newly-acquired watchers are now rolled back (not just the failed one), keeping refcounts clean for retry.
2. Consistent locking: All watchRegistry methods now consistently require the caller to hold mu. Removed the internal lock from IsPending; callers in updateWatches now lock/unlock around each check.
3. Per-request timeout: Each WatchFiles/UnwatchFiles call gets its own context.WithTimeout so earlier calls don't consume the deadline for later ones.
4. Duplicate patterns: Watchers are deduped by key before calling Acquire to prevent refcount inflation.
5. Stronger test assertions: Test now verifies retry watcher IDs are a subset of the first attempt IDs and that at least one retried watcher succeeded.

[andrewbranch]

@copilot I don't really understand the "dedupe watchers by key to prevent refcount inflation from duplicate patterns" issue. If a watcher produced duplicate patterns, previously, the fileSystemWatcherValue corresponding to that pattern would get reffed once for each instance. During deregistration, the ref count would decrement once for each instance. The registrations were already deduped, so why does it matter what the actual ref counts were as long as they were balanced? It looks to me like you've added another unnecessary layer of deduping, but please explain if I'm misunderstanding something.

[Copilot]

@copilot I don't really understand the "dedupe watchers by key to prevent refcount inflation from duplicate patterns" issue. If a watcher produced duplicate patterns, previously, the fileSystemWatcher...

You're right that in the normal lifecycle the refcounts are balanced — register and deregister both iterate the same list (including duplicates), so the increments and decrements match.

The issue is specifically with the rollback path. On failure, lines 1184-1186 iterate newWatchers.Entries() to Release, but newWatchers only contains entries where Acquire returned isNew=true (count went 0→1). Without dedup, if key K appears 3 times, Acquire increments count to 3 but only the first is in newWatchers. Rollback releases once (3→2), leaving count at 2. On retry, Acquire sees count >0 and returns isNew=false, so the watcher never gets re-registered.

An alternative would be to track all acquired watchers (including duplicates) and Release each one on rollback. If you prefer that approach I can switch to it — the dedup just seemed simpler and avoids the extra Acquire/Release cycles for duplicates.

[andrewbranch]

@copilot understood. Instead, let's move the deduplication into WatchedFiles itself so the session code is simplified.

[Copilot]

@copilot understood. Instead, let's move the deduplication into WatchedFiles itself so the session code is simplified.

Moved in ce17015d. Watchers() now deduplicates via slices.Compact(slices.Sorted(...)) on both patternsInsideWorkspace and directoriesOutsideWorkspace before creating FileSystemWatcher objects. The dedup logic in updateWatch is removed.

[andrewbranch]

@copilot move mutex locking into watchRegistry methods.

[Copilot]

@copilot move mutex locking into watchRegistry methods.

Done in a2d063ec. All watchRegistry methods (Acquire, Release, MarkPending, ClearPending, IsPending) now lock mu internally. Removed all external s.watches.mu.Lock/Unlock calls from session.go.