From 11f4661ea99f559da5203570b0f47b49a4b26072 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Tue, 7 Jul 2026 06:10:03 +0300 Subject: [PATCH] fix(lsp): coalesce diagnostic bursts (#1861) * fix(lsp): coalesce diagnostic bursts * fix(lsp): tighten diagnostic debounce coverage * test(lsp): share zero-diagnostic log assertion * test(lsp): isolate diagnostic attachment debounce coverage --- .../lsp/LSPDiagnosticRegistry.test.ts | 285 +++++++++++++++++- src/services/lsp/LSPDiagnosticRegistry.ts | 214 +++++++++++-- src/services/lsp/passiveFeedback.ts | 12 +- src/tools/FileEditTool/FileEditTool.ts | 4 +- src/tools/FileWriteTool/FileWriteTool.ts | 4 +- src/utils/attachments.lspDiagnostics.test.ts | 98 +++++- src/utils/attachments.ts | 72 ++++- 7 files changed, 623 insertions(+), 66 deletions(-) diff --git a/src/services/lsp/LSPDiagnosticRegistry.test.ts b/src/services/lsp/LSPDiagnosticRegistry.test.ts index 4b35a69da..97fe882ad 100644 --- a/src/services/lsp/LSPDiagnosticRegistry.test.ts +++ b/src/services/lsp/LSPDiagnosticRegistry.test.ts @@ -47,12 +47,22 @@ function diagnosticCount(files: DiagnosticFile[]): number { return files.reduce((sum, file) => sum + file.diagnostics.length, 0) } +function checkWithDebounce(now: number) { + return registry.checkForLSPDiagnostics({ now, respectDebounce: true }) +} + function deliveryLogs(): string[] { return debugMessages.filter(message => message.startsWith('LSP Diagnostics: Delivering '), ) } +function expectNoZeroDiagnosticDeliveryLog(): void { + expect( + deliveryLogs().some(message => message.includes(' with 0 diagnostic(s) ')), + ).toBe(false) +} + describe('LSPDiagnosticRegistry storm control', () => { beforeEach(() => { registry.resetAllLSPDiagnosticState() @@ -74,6 +84,256 @@ describe('LSPDiagnosticRegistry storm control', () => { ]) }) + test('coalesces repeated same-file burst snapshots into one stable delivery', () => { + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['stale diagnostic'])], + timestamp: 1_000, + }) + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['final diagnostic'])], + timestamp: 1_100, + }) + + expect(checkWithDebounce(1_150)).toEqual([]) + + const diagnosticSets = checkWithDebounce(1_400) + + expect(diagnosticSets).toHaveLength(1) + expect(diagnosticSets[0]?.files).toEqual([ + diagnosticFile('/repo/a.ts', ['final diagnostic']), + ]) + expect(registry.getPendingLSPDiagnosticCount()).toBe(0) + }) + + test('coalesces several files into a bounded deterministic stable delivery', () => { + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [ + ...Array.from({ length: 32 }, (_, index) => + diagnosticFile(`/repo/file-${index}.ts`, [`initial ${index}`]), + ), + ], + timestamp: 2_000, + }) + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/file-1.ts', ['latest file 1'])], + timestamp: 2_100, + }) + + const files = checkWithDebounce(2_400)[0]?.files ?? [] + + expect(diagnosticCount(files)).toBe(30) + expect(files.map(file => file.uri)).toEqual([ + '/repo/file-0.ts', + '/repo/file-1.ts', + '/repo/file-2.ts', + '/repo/file-3.ts', + '/repo/file-4.ts', + '/repo/file-5.ts', + '/repo/file-6.ts', + '/repo/file-7.ts', + '/repo/file-8.ts', + '/repo/file-9.ts', + '/repo/file-10.ts', + '/repo/file-11.ts', + '/repo/file-12.ts', + '/repo/file-13.ts', + '/repo/file-14.ts', + '/repo/file-15.ts', + '/repo/file-16.ts', + '/repo/file-17.ts', + '/repo/file-18.ts', + '/repo/file-19.ts', + '/repo/file-20.ts', + '/repo/file-21.ts', + '/repo/file-22.ts', + '/repo/file-23.ts', + '/repo/file-24.ts', + '/repo/file-25.ts', + '/repo/file-26.ts', + '/repo/file-27.ts', + '/repo/file-28.ts', + '/repo/file-29.ts', + ]) + expect(files[1]?.diagnostics[0]?.message).toBe('latest file 1') + }) + + test('delivers new diagnostics after the debounce window', () => { + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['first diagnostic'])], + timestamp: 3_000, + }) + + expect(checkWithDebounce(3_100)).toEqual([]) + + const firstDelivery = checkWithDebounce(3_300) + expect(firstDelivery[0]?.files).toEqual([ + diagnosticFile('/repo/a.ts', ['first diagnostic']), + ]) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['second diagnostic'])], + timestamp: 3_650, + }) + + expect(checkWithDebounce(3_700)).toEqual([]) + const secondDelivery = checkWithDebounce(3_950) + + expect(secondDelivery[0]?.files).toEqual([ + diagnosticFile('/repo/a.ts', ['second diagnostic']), + ]) + }) + + test('flushes active bursts after the max coalescing delay', () => { + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['first diagnostic'])], + timestamp: 4_000, + }) + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['latest diagnostic'])], + timestamp: 5_900, + }) + + const diagnosticSets = checkWithDebounce(6_100) + + expect(diagnosticSets[0]?.files).toEqual([ + diagnosticFile('/repo/a.ts', ['latest diagnostic']), + ]) + }) + + test('reports the next stable delivery delay for pending diagnostics', () => { + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [diagnosticFile('/repo/a.ts', ['first diagnostic'])], + timestamp: 15_000, + }) + + expect(registry.getNextLSPDiagnosticDeliveryDelay(15_100)).toBe(150) + + const diagnosticSets = checkWithDebounce(15_250) + expect(diagnosticSets[0]?.files).toEqual([ + diagnosticFile('/repo/a.ts', ['first diagnostic']), + ]) + expect(registry.getNextLSPDiagnosticDeliveryDelay(15_260)).toBeNull() + }) + + test('clearing diagnostics updates state without producing attachments', () => { + const file = diagnosticFile('/repo/a.ts', ['transient diagnostic']) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [file], + timestamp: 7_000, + }) + expect(checkWithDebounce(7_300)).toHaveLength(1) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [{ uri: '/repo/a.ts', diagnostics: [] }], + timestamp: 7_600, + }) + + expect(registry.getPendingLSPDiagnosticCount()).toBe(0) + expect(checkWithDebounce(7_900)).toEqual([]) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [file], + timestamp: 8_200, + }) + + expect(checkWithDebounce(8_500)[0]?.files).toEqual([ + file, + ]) + }) + + test('does not dedupe identical diagnostics across different servers', () => { + const sharedFile = diagnosticFile('/repo/a.ts', ['same text and range']) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [sharedFile], + timestamp: 9_000, + }) + expect(checkWithDebounce(9_300)[0]?.files).toEqual([ + sharedFile, + ]) + + registry.registerPendingLSPDiagnostic({ + serverName: 'eslint', + files: [sharedFile], + timestamp: 9_600, + }) + + expect(checkWithDebounce(9_900)[0]?.files).toEqual([ + sharedFile, + ]) + }) + + test('does not let one active server burst block stable diagnostics from another server', () => { + const stableFile = diagnosticFile('/repo/stable.ts', ['stable diagnostic']) + const activeFile = diagnosticFile('/repo/active.ts', ['active diagnostic']) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [stableFile], + timestamp: 10_000, + }) + registry.registerPendingLSPDiagnostic({ + serverName: 'eslint', + files: [activeFile], + timestamp: 10_390, + }) + + const firstDelivery = checkWithDebounce(10_400) + + expect(firstDelivery[0]?.serverName).toBe('typescript') + expect(firstDelivery[0]?.files).toEqual([stableFile]) + expect(registry.getPendingLSPDiagnosticCount()).toBe(1) + + const secondDelivery = checkWithDebounce(10_700) + + expect(secondDelivery[0]?.serverName).toBe('eslint') + expect(secondDelivery[0]?.files).toEqual([activeFile]) + expect(registry.getPendingLSPDiagnosticCount()).toBe(0) + }) + + test('does not let one same-server file hitting max delay flush a fresh file', () => { + const oldFile = diagnosticFile('/repo/old.ts', ['old diagnostic']) + const freshFile = diagnosticFile('/repo/fresh.ts', ['fresh diagnostic']) + + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [oldFile], + timestamp: 11_000, + }) + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [freshFile], + timestamp: 12_990, + }) + + const firstDelivery = checkWithDebounce(13_001) + + expect(firstDelivery[0]?.serverName).toBe('typescript') + expect(firstDelivery[0]?.files).toEqual([oldFile]) + expect(registry.getPendingLSPDiagnosticCount()).toBe(1) + + expect(checkWithDebounce(13_100)).toEqual([]) + + const secondDelivery = checkWithDebounce(13_250) + + expect(secondDelivery[0]?.files).toEqual([freshFile]) + expect(registry.getPendingLSPDiagnosticCount()).toBe(0) + }) + test('does not reattach unchanged diagnostics across turns', () => { const file = diagnosticFile('/repo/a.ts', ['same missing import']) @@ -91,9 +351,7 @@ describe('LSPDiagnosticRegistry storm control', () => { }) expect(registry.checkForLSPDiagnostics()).toEqual([]) - expect(deliveryLogs()).not.toContain( - 'LSP Diagnostics: Delivering 1 file(s) with 0 diagnostic(s) from 1 server(s)', - ) + expectNoZeroDiagnosticDeliveryLog() }) test('returns no diagnostic set for raw empty diagnostic files', () => { @@ -107,6 +365,19 @@ describe('LSPDiagnosticRegistry storm control', () => { expect(deliveryLogs()).toEqual([]) }) + test('clock injection does not enable debounce unless requested', () => { + const file = diagnosticFile('/repo/a.ts', ['clock-only diagnostic']) + registry.registerPendingLSPDiagnostic({ + serverName: 'typescript', + files: [file], + timestamp: 14_000, + }) + + expect(registry.checkForLSPDiagnostics({ now: 14_001 })[0]?.files).toEqual([ + file, + ]) + }) + test('snapshots pending diagnostics without consuming delivery', () => { const file = diagnosticFile('/repo/a.ts', ['same missing import']) @@ -336,9 +607,7 @@ describe('LSPDiagnosticRegistry storm control', () => { 'lsp://diagnostic-storm/typescript', ]) expect(diagnosticCount(secondFiles)).toBe(1) - expect(deliveryLogs()).not.toContain( - 'LSP Diagnostics: Delivering 1 file(s) with 0 diagnostic(s) from 1 server(s)', - ) + expectNoZeroDiagnosticDeliveryLog() }) test('returns compact storm summaries when volume limiting leaves only reserved summaries', () => { @@ -365,9 +634,7 @@ describe('LSPDiagnosticRegistry storm control', () => { .toBe(true) expect(diagnosticCount(files)).toBe(30) expect(registry.getPendingLSPDiagnosticCount()).toBe(0) - expect(deliveryLogs()).not.toContain( - 'LSP Diagnostics: Delivering 30 file(s) with 0 diagnostic(s) from 30 server(s)', - ) + expectNoZeroDiagnosticDeliveryLog() }) test('reserves compact summaries for multiple storming servers before full diagnostics', () => { diff --git a/src/services/lsp/LSPDiagnosticRegistry.ts b/src/services/lsp/LSPDiagnosticRegistry.ts index 052a5d2bd..8d7bcf9fc 100644 --- a/src/services/lsp/LSPDiagnosticRegistry.ts +++ b/src/services/lsp/LSPDiagnosticRegistry.ts @@ -1,4 +1,3 @@ -import { randomUUID } from 'crypto' import { LRUCache } from 'lru-cache' import * as path from 'path' import { logForDebugging } from '../../utils/debug.js' @@ -17,6 +16,8 @@ export type PendingLSPDiagnostic = { files: DiagnosticFile[] /** When diagnostic was received */ timestamp: number + /** When the current coalesced burst for this server/file began */ + firstTimestamp: number /** Whether attachment was already sent to conversation */ attachmentSent: boolean } @@ -56,6 +57,9 @@ const DIAGNOSTIC_STORM_WINDOW_MS = 60_000 const DIAGNOSTIC_STORM_RAW_THRESHOLD = 200 const DIAGNOSTIC_STORM_LOG_THROTTLE_MS = 60_000 const RECENT_FILE_PRIORITY_WINDOW_MS = 5 * 60_000 +export const DIAGNOSTIC_DELIVERY_DEBOUNCE_MS = 250 +const DIAGNOSTIC_DELIVERY_MAX_DELAY_MS = 2_000 +const DIAGNOSTIC_COALESCING_LOG_THROTTLE_MS = 1_000 const MAX_STORM_TOP_FILES = 5 const STORM_SUMMARY_URI_PREFIX = 'lsp://diagnostic-storm' @@ -99,11 +103,16 @@ type ServerDeliveryPlan = { shouldSummarizeStorm: boolean } +type CheckLSPDiagnosticsOptions = { + now?: number + respectDebounce?: boolean +} + // Global registry state const pendingDiagnostics = new Map() -// Cross-turn deduplication: tracks diagnostics that have been delivered -// Maps file URI to a set of diagnostic keys (hash of message+severity+range) +// Cross-turn deduplication: tracks diagnostics that have been delivered. +// Maps server/file keys to diagnostic keys (hash of message+severity+range). // Using LRUCache to prevent unbounded growth in long sessions const deliveredDiagnostics = new LRUCache>({ max: MAX_DELIVERED_FILES, @@ -114,6 +123,7 @@ const recentDiagnosticFileActivity = new LRUCache({ }) const diagnosticWindows = new Map() +const coalescingLogTimestamps = new Map() function normalizeDiagnosticUri(uri: string): string { for (const prefix of ['file://', '_claude_fs_right:', '_claude_fs_left:']) { @@ -124,6 +134,10 @@ function normalizeDiagnosticUri(uri: string): string { return uri } +function createServerFileKey(serverName: string, uri: string): string { + return `${serverName}\0${normalizeDiagnosticUri(uri)}` +} + function displayFileForStormSummary(uri: string): string { const normalized = normalizeDiagnosticUri(uri).replace(/\\/g, '/') return path.basename(normalized) || normalized || '' @@ -331,6 +345,21 @@ function maybeLogStormSummary( logForDebugging(formatStormSummary(serverName, stats)) } +function maybeLogCoalescedDelivery(serverName: string, now: number): void { + const lastLoggedAt = coalescingLogTimestamps.get(serverName) + if ( + lastLoggedAt !== undefined && + now - lastLoggedAt < DIAGNOSTIC_COALESCING_LOG_THROTTLE_MS + ) { + return + } + + coalescingLogTimestamps.set(serverName, now) + logForDebugging( + `LSP Diagnostics: Coalescing pending diagnostics from ${serverName} until burst is stable`, + ) +} + /** * Record an LSP file interaction so diagnostics for recently opened or edited * files are preserved first when a diagnostic burst exceeds the per-turn cap. @@ -358,17 +387,26 @@ export function registerPendingLSPDiagnostic({ files: DiagnosticFile[] timestamp?: number }): void { - // Use UUID for guaranteed uniqueness (handles rapid registrations) - const diagnosticId = randomUUID() - recordDiagnosticsReceived(serverName, files, timestamp) - pendingDiagnostics.set(diagnosticId, { - serverName, - files, - timestamp, - attachmentSent: false, - }) + for (const file of files) { + const diagnosticId = createServerFileKey(serverName, file.uri) + const existingDiagnostic = pendingDiagnostics.get(diagnosticId) + + if (file.diagnostics.length === 0) { + pendingDiagnostics.delete(diagnosticId) + deliveredDiagnostics.delete(diagnosticId) + continue + } + + pendingDiagnostics.set(diagnosticId, { + serverName, + files: [file], + timestamp, + firstTimestamp: existingDiagnostic?.firstTimestamp ?? timestamp, + attachmentSent: false, + }) + } } /** @@ -422,6 +460,7 @@ function createDiagnosticKey(diag: { * - Source and code (if present) */ function deduplicateDiagnosticFiles( + serverName: string, allFiles: DiagnosticFile[], options: { filterPreviouslyDelivered?: boolean } = { filterPreviouslyDelivered: true, @@ -449,7 +488,8 @@ function deduplicateDiagnosticFiles( const previouslyDelivered = options.filterPreviouslyDelivered === false ? new Set() - : deliveredDiagnostics.get(normalizedUri) || new Set() + : deliveredDiagnostics.get(createServerFileKey(serverName, file.uri)) || + new Set() for (const diag of file.diagnostics) { try { @@ -558,13 +598,16 @@ function countDiagnostics(files: DiagnosticFile[]): number { return files.reduce((total, file) => total + file.diagnostics.length, 0) } -function trackDeliveredDiagnostics(files: DiagnosticFile[]): void { +function trackDeliveredDiagnostics( + serverName: string, + files: DiagnosticFile[], +): void { for (const file of files) { - const normalizedUri = normalizeDiagnosticUri(file.uri) - if (!deliveredDiagnostics.has(normalizedUri)) { - deliveredDiagnostics.set(normalizedUri, new Set()) + const deliveredKey = createServerFileKey(serverName, file.uri) + if (!deliveredDiagnostics.has(deliveredKey)) { + deliveredDiagnostics.set(deliveredKey, new Set()) } - const delivered = deliveredDiagnostics.get(normalizedUri)! + const delivered = deliveredDiagnostics.get(deliveredKey)! for (const diag of file.diagnostics) { try { delivered.add(createDiagnosticKey(diag)) @@ -584,24 +627,78 @@ function trackDeliveredDiagnostics(files: DiagnosticFile[]): void { } } +function shouldDeferDiagnosticDelivery( + serverName: string, + diagnostic: PendingLSPDiagnostic, + now: number, + respectDebounce: boolean, +): boolean { + if (!respectDebounce) { + return false + } + + const burstAge = now - diagnostic.firstTimestamp + const stableAge = now - diagnostic.timestamp + const shouldDefer = + stableAge < DIAGNOSTIC_DELIVERY_DEBOUNCE_MS && + burstAge < DIAGNOSTIC_DELIVERY_MAX_DELAY_MS + + if (shouldDefer) { + maybeLogCoalescedDelivery(serverName, now) + } + + return shouldDefer +} + +export function getNextLSPDiagnosticDeliveryDelay( + now = Date.now(), +): number | null { + let nextDelay: number | null = null + + for (const diagnostic of pendingDiagnostics.values()) { + if (diagnostic.attachmentSent) { + continue + } + + const burstAge = now - diagnostic.firstTimestamp + const stableAge = now - diagnostic.timestamp + if ( + stableAge >= DIAGNOSTIC_DELIVERY_DEBOUNCE_MS || + burstAge >= DIAGNOSTIC_DELIVERY_MAX_DELAY_MS + ) { + return 0 + } + + const stableDelay = DIAGNOSTIC_DELIVERY_DEBOUNCE_MS - stableAge + const maxDelay = DIAGNOSTIC_DELIVERY_MAX_DELAY_MS - burstAge + const delay = Math.max(0, Math.min(stableDelay, maxDelay)) + nextDelay = nextDelay === null ? delay : Math.min(nextDelay, delay) + } + + return nextDelay +} + function collectUndeliveredDiagnosticFiles(): { filesByServer: Map - diagnosticsToMark: PendingLSPDiagnostic[] + pendingByServer: Map } { const filesByServer = new Map() - const diagnosticsToMark: PendingLSPDiagnostic[] = [] + const pendingByServer = new Map() for (const diagnostic of pendingDiagnostics.values()) { if (!diagnostic.attachmentSent) { if (!filesByServer.has(diagnostic.serverName)) { filesByServer.set(diagnostic.serverName, []) } + if (!pendingByServer.has(diagnostic.serverName)) { + pendingByServer.set(diagnostic.serverName, []) + } filesByServer.get(diagnostic.serverName)!.push(...diagnostic.files) - diagnosticsToMark.push(diagnostic) + pendingByServer.get(diagnostic.serverName)!.push(diagnostic) } } - return { filesByServer, diagnosticsToMark } + return { filesByServer, pendingByServer } } /** @@ -626,7 +723,7 @@ export function getPendingLSPDiagnosticsSnapshot(): LSPDiagnosticSet[] { for (const [serverName, files] of filesByServer) { let deduplicationResult: DeduplicationResult try { - deduplicationResult = deduplicateDiagnosticFiles(files, { + deduplicationResult = deduplicateDiagnosticFiles(serverName, files, { filterPreviouslyDelivered: false, }) } catch (error: unknown) { @@ -674,19 +771,54 @@ function cloneDiagnosticFile(file: DiagnosticFile): DiagnosticFile { * * @returns Array of pending diagnostics ready for delivery (deduplicated) */ -export function checkForLSPDiagnostics(): LSPDiagnosticSet[] { - const now = Date.now() +export function checkForLSPDiagnostics( + options: CheckLSPDiagnosticsOptions = {}, +): LSPDiagnosticSet[] { + const now = options.now ?? Date.now() + const respectDebounce = options.respectDebounce ?? false logForDebugging( `LSP Diagnostics: Checking registry - ${pendingDiagnostics.size} pending`, ) - const { filesByServer, diagnosticsToMark } = + const { filesByServer, pendingByServer } = collectUndeliveredDiagnosticFiles() if (filesByServer.size === 0) { return [] } + const deliverableFilesByServer = new Map() + const diagnosticsToMark: PendingLSPDiagnostic[] = [] + + for (const [serverName] of filesByServer) { + const pendingForServer = pendingByServer.get(serverName) ?? [] + + for (const pendingDiagnostic of pendingForServer) { + if ( + shouldDeferDiagnosticDelivery( + serverName, + pendingDiagnostic, + now, + respectDebounce, + ) + ) { + continue + } + + if (!deliverableFilesByServer.has(serverName)) { + deliverableFilesByServer.set(serverName, []) + } + deliverableFilesByServer + .get(serverName)! + .push(...pendingDiagnostic.files) + diagnosticsToMark.push(pendingDiagnostic) + } + } + + if (deliverableFilesByServer.size === 0) { + return [] + } + // Only mark as sent AFTER successful deduplication, then delete from map. // Entries are tracked in deliveredDiagnostics LRU for dedup, so we don't // need to keep them in pendingDiagnostics after delivery. @@ -706,10 +838,10 @@ export function checkForLSPDiagnostics(): LSPDiagnosticSet[] { let deliveredCount = 0 const deliveryPlans: ServerDeliveryPlan[] = [] - for (const [serverName, files] of filesByServer) { + for (const [serverName, files] of deliverableFilesByServer) { let deduplicationResult: DeduplicationResult try { - deduplicationResult = deduplicateDiagnosticFiles(files) + deduplicationResult = deduplicateDiagnosticFiles(serverName, files) } catch (error: unknown) { const err = toError(error) logError( @@ -780,14 +912,17 @@ export function checkForLSPDiagnostics(): LSPDiagnosticSet[] { // Volume caps intentionally drop diagnostics for the turn; account for the // full deduplicated batch so unchanged storms cannot trickle old diagnostics // into later turns one capped slice at a time. - trackDeliveredDiagnostics(deduplicationResult.files) + trackDeliveredDiagnostics(serverName, deduplicationResult.files) deliveredFiles.push(...limitResult.files) remainingCapacity -= limitResult.deliveredCount duplicateCount += deduplicationResult.duplicateCount droppedCount += limitResult.droppedCount deliveredCount += limitResult.deliveredCount - if (remainingCapacity <= 0 && serverNames.length < filesByServer.size) { + if ( + remainingCapacity <= 0 && + serverNames.length < deliverableFilesByServer.size + ) { logForDebugging( `LSP Diagnostics: Global turn capacity exhausted after ${serverName}; later server diagnostics will be summarized or dropped`, ) @@ -854,6 +989,7 @@ export function resetAllLSPDiagnosticState(): void { deliveredDiagnostics.clear() recentDiagnosticFileActivity.clear() diagnosticWindows.clear() + coalescingLogTimestamps.clear() } /** @@ -865,11 +1001,25 @@ export function resetAllLSPDiagnosticState(): void { */ export function clearDeliveredDiagnosticsForFile(fileUri: string): void { const normalizedUri = normalizeDiagnosticUri(fileUri) - if (deliveredDiagnostics.has(normalizedUri)) { + let clearedDelivered = false + + for (const key of deliveredDiagnostics.keys()) { + if (key.endsWith(`\0${normalizedUri}`)) { + deliveredDiagnostics.delete(key) + clearedDelivered = true + } + } + + for (const key of pendingDiagnostics.keys()) { + if (key.endsWith(`\0${normalizedUri}`)) { + pendingDiagnostics.delete(key) + } + } + + if (clearedDelivered) { logForDebugging( `LSP Diagnostics: Clearing delivered diagnostics for ${fileUri}`, ) - deliveredDiagnostics.delete(normalizedUri) } } diff --git a/src/services/lsp/passiveFeedback.ts b/src/services/lsp/passiveFeedback.ts index f09707f05..53de258f6 100644 --- a/src/services/lsp/passiveFeedback.ts +++ b/src/services/lsp/passiveFeedback.ts @@ -185,15 +185,13 @@ export function registerLSPNotificationHandlers( const diagnosticFiles = formatDiagnosticsForAttachment(diagnosticParams) - // Only send notification if there are diagnostics + // Register empty diagnostic snapshots too: in LSP they mean the + // file was cleared, and the registry uses them to update state + // without producing an empty model attachment. const firstFile = diagnosticFiles[0] - if ( - !firstFile || - diagnosticFiles.length === 0 || - firstFile.diagnostics.length === 0 - ) { + if (!firstFile) { logForDebugging( - `Skipping empty diagnostics from ${serverName} for ${diagnosticParams.uri}`, + `Skipping invalid diagnostics from ${serverName} for ${diagnosticParams.uri}`, ) return } diff --git a/src/tools/FileEditTool/FileEditTool.ts b/src/tools/FileEditTool/FileEditTool.ts index e8a13fcba..1f5707881 100644 --- a/src/tools/FileEditTool/FileEditTool.ts +++ b/src/tools/FileEditTool/FileEditTool.ts @@ -490,10 +490,10 @@ export const FileEditTool = buildTool({ // 5. Write to disk writeTextContent(absoluteFilePath, updatedFile, encoding, endings) - // Notify LSP servers about file modification (didChange) and save (didSave) const lspManager = getLspServerManager() if (lspManager) { - // Clear previously delivered diagnostics so new ones will be shown + // Clear previously delivered diagnostics after a successful write so + // new diagnostics will be shown. clearDeliveredDiagnosticsForFile(`file://${absoluteFilePath}`) // didChange: Content has been modified lspManager diff --git a/src/tools/FileWriteTool/FileWriteTool.ts b/src/tools/FileWriteTool/FileWriteTool.ts index 1aa2d9dc3..b7013a820 100644 --- a/src/tools/FileWriteTool/FileWriteTool.ts +++ b/src/tools/FileWriteTool/FileWriteTool.ts @@ -304,10 +304,10 @@ export const FileWriteTool = buildTool({ // overwriting a CRLF file or when binaries in cwd poisoned the repo sample. writeTextContent(fullFilePath, content, enc, 'LF') - // Notify LSP servers about file modification (didChange) and save (didSave) const lspManager = getLspServerManager() if (lspManager) { - // Clear previously delivered diagnostics so new ones will be shown + // Clear previously delivered diagnostics after a successful write so + // new diagnostics will be shown. clearDeliveredDiagnosticsForFile(`file://${fullFilePath}`) // didChange: Content has been modified lspManager.changeFile(fullFilePath, content).catch((err: Error) => { diff --git a/src/utils/attachments.lspDiagnostics.test.ts b/src/utils/attachments.lspDiagnostics.test.ts index de908ee6b..e7985e1e1 100644 --- a/src/utils/attachments.lspDiagnostics.test.ts +++ b/src/utils/attachments.lspDiagnostics.test.ts @@ -13,11 +13,22 @@ const realLSPRegistry = await import( ) let diagnosticSets: Array<{ serverName: string; files: DiagnosticFile[] }> = [] +let nextDeliveryDelay: number | null = null +const checkForLSPDiagnosticsOptions: unknown[] = [] +const getNextLSPDiagnosticDeliveryDelayCalls: Array = [] +const { DIAGNOSTIC_DELIVERY_DEBOUNCE_MS } = realLSPRegistry -const checkForLSPDiagnosticsMock = mock(() => diagnosticSets) +const checkForLSPDiagnosticsMock = mock((options?: unknown) => { + checkForLSPDiagnosticsOptions.push(options) + return diagnosticSets +}) const clearAllLSPDiagnosticsMock = mock(() => { diagnosticSets = [] }) +const getNextLSPDiagnosticDeliveryDelayMock = mock((now?: number) => { + getNextLSPDiagnosticDeliveryDelayCalls.push(now) + return nextDeliveryDelay +}) mock.module('./debug.js', () => ({ ...realDebugModule, @@ -30,9 +41,10 @@ mock.module('../services/lsp/LSPDiagnosticRegistry.js', () => ({ ...realLSPRegistry, checkForLSPDiagnostics: checkForLSPDiagnosticsMock, clearAllLSPDiagnostics: clearAllLSPDiagnosticsMock, + getNextLSPDiagnosticDeliveryDelay: getNextLSPDiagnosticDeliveryDelayMock, })) -const { getAttachmentMessages } = await import( +const { getAttachmentMessages, __test } = await import( `./attachments.ts?test=${Date.now()}-${Math.random()}` ) @@ -41,6 +53,24 @@ const SAVED_DISABLE_ATTACHMENTS = process.env.CLAUDE_CODE_DISABLE_ATTACHMENTS type DiagnosticsAttachment = Extract +function lspDiagnosticFile(message = 'stable diagnostic'): DiagnosticFile { + return { + uri: '/repo/a.ts', + diagnostics: [ + { + message, + severity: 'Error', + range: { + start: { line: 0, character: 0 }, + end: { line: 0, character: 1 }, + }, + source: 'typescript', + code: 'TS1000', + }, + ], + } +} + function makeToolUseContext(): ToolUseContext { let inProgressToolUseIDs = new Set() @@ -110,9 +140,13 @@ describe('LSP diagnostic attachment filtering', () => { delete process.env.CLAUDE_CODE_SIMPLE delete process.env.CLAUDE_CODE_DISABLE_ATTACHMENTS diagnosticSets = [] + nextDeliveryDelay = null + checkForLSPDiagnosticsOptions.length = 0 + getNextLSPDiagnosticDeliveryDelayCalls.length = 0 debugMessages.length = 0 checkForLSPDiagnosticsMock.mockClear() clearAllLSPDiagnosticsMock.mockClear() + getNextLSPDiagnosticDeliveryDelayMock.mockClear() }) afterEach(() => { @@ -169,9 +203,67 @@ describe('LSP diagnostic attachment filtering', () => { expect(attachments).toEqual([ { type: 'diagnostics', files: [summaryFile], isNew: true }, ]) - expect(clearAllLSPDiagnosticsMock).toHaveBeenCalledTimes(1) + expect(clearAllLSPDiagnosticsMock).not.toHaveBeenCalled() expect(debugMessages).toContain( 'LSP Diagnostics: Returning 1 diagnostic attachment(s)', ) }) + + test('waits once for debounced diagnostics at the query boundary', async () => { + const file = lspDiagnosticFile() + let now = 100 + const waits: number[] = [] + nextDeliveryDelay = 150 + + const attachments = await __test.getLSPDiagnosticAttachments( + makeToolUseContext(), + { + now: () => now, + wait: async ms => { + waits.push(ms) + now += ms + diagnosticSets = [{ serverName: 'typescript', files: [file] }] + }, + }, + ) + + expect(checkForLSPDiagnosticsOptions).toEqual([ + { respectDebounce: true, now: 100 }, + { respectDebounce: true, now: 250 }, + ]) + expect(getNextLSPDiagnosticDeliveryDelayCalls).toEqual([100]) + expect(waits).toEqual([150]) + expect(attachments).toEqual([ + { + type: 'diagnostics', + files: [file], + isNew: true, + }, + ]) + }) + + test('caps the query-boundary wait when the next ready delay is longer', async () => { + let now = 0 + const waits: number[] = [] + nextDeliveryDelay = DIAGNOSTIC_DELIVERY_DEBOUNCE_MS + 250 + + const attachments = await __test.getLSPDiagnosticAttachments( + makeToolUseContext(), + { + now: () => now, + wait: async ms => { + waits.push(ms) + now += ms + }, + }, + ) + + expect(checkForLSPDiagnosticsOptions).toEqual([ + { respectDebounce: true, now: 0 }, + { respectDebounce: true, now: DIAGNOSTIC_DELIVERY_DEBOUNCE_MS }, + ]) + expect(getNextLSPDiagnosticDeliveryDelayCalls).toEqual([0]) + expect(waits).toEqual([DIAGNOSTIC_DELIVERY_DEBOUNCE_MS]) + expect(attachments).toEqual([]) + }) }) diff --git a/src/utils/attachments.ts b/src/utils/attachments.ts index f0d77c4a8..72fbca6a9 100644 --- a/src/utils/attachments.ts +++ b/src/utils/attachments.ts @@ -187,9 +187,12 @@ import { } from './hooks/AsyncHookRegistry.js' import { checkForLSPDiagnostics, - clearAllLSPDiagnostics, + DIAGNOSTIC_DELIVERY_DEBOUNCE_MS, + getNextLSPDiagnosticDeliveryDelay, + type LSPDiagnosticSet, } from '../services/lsp/LSPDiagnosticRegistry.js' import { logForDebugging } from './debug.js' +import { sleep } from './sleep.js' import { extractTextContent, getUserMessageText, @@ -2932,8 +2935,57 @@ async function getDiagnosticAttachments( * Get LSP diagnostic attachments from passive LSP servers. * Follows the AsyncHookRegistry pattern for consistent async attachment delivery. */ +type LSPDiagnosticAttachmentDeps = { + now?: () => number + wait?: (ms: number, signal?: AbortSignal) => Promise +} + +const LSP_DIAGNOSTIC_ATTACHMENT_MAX_WAIT_MS = DIAGNOSTIC_DELIVERY_DEBOUNCE_MS + +function getLSPDiagnosticCheckOptions(now?: () => number): { + respectDebounce: true + now?: number +} { + const currentTime = now?.() + return currentTime === undefined + ? { respectDebounce: true } + : { respectDebounce: true, now: currentTime } +} + +async function checkForStableLSPDiagnosticsAtQueryBoundary( + toolUseContext: ToolUseContext, + deps: LSPDiagnosticAttachmentDeps, +): Promise { + const diagnosticSets = checkForLSPDiagnostics( + getLSPDiagnosticCheckOptions(deps.now), + ) + if (diagnosticSets.length > 0) { + return diagnosticSets + } + + const nextDelay = getNextLSPDiagnosticDeliveryDelay(deps.now?.()) + if (nextDelay === null) { + return [] + } + + const waitMs = Math.min(nextDelay, LSP_DIAGNOSTIC_ATTACHMENT_MAX_WAIT_MS) + if (waitMs > 0) { + logForDebugging( + `LSP Diagnostics: Waiting ${waitMs}ms for pending diagnostics to stabilize`, + ) + const wait = + deps.wait ?? + ((ms: number, signal?: AbortSignal) => + sleep(ms, signal, { unref: true })) + await wait(waitMs, toolUseContext.abortController.signal) + } + + return checkForLSPDiagnostics(getLSPDiagnosticCheckOptions(deps.now)) +} + async function getLSPDiagnosticAttachments( toolUseContext: ToolUseContext, + deps: LSPDiagnosticAttachmentDeps = {}, ): Promise { // LSP diagnostics are only useful if the agent has the Bash tool to act on them if ( @@ -2945,7 +2997,10 @@ async function getLSPDiagnosticAttachments( logForDebugging('LSP Diagnostics: getLSPDiagnosticAttachments called') try { - const diagnosticSets = checkForLSPDiagnostics() + const diagnosticSets = await checkForStableLSPDiagnosticsAtQueryBoundary( + toolUseContext, + deps, + ) if (diagnosticSets.length === 0) { return [] @@ -2981,15 +3036,6 @@ async function getLSPDiagnosticAttachments( isNew: true, })) - // Clear delivered diagnostics from registry to prevent memory leak - // Follows same pattern as removeDeliveredAsyncHooks - if (diagnosticSets.length > 0) { - clearAllLSPDiagnostics() - logForDebugging( - `LSP Diagnostics: Cleared ${diagnosticSets.length} delivered diagnostic(s) from registry`, - ) - } - logForDebugging( `LSP Diagnostics: Returning ${attachments.length} diagnostic attachment(s)`, ) @@ -3005,6 +3051,10 @@ async function getLSPDiagnosticAttachments( } } +export const __test = { + getLSPDiagnosticAttachments, +} + export async function* getAttachmentMessages( input: string | null, toolUseContext: ToolUseContext,