LSP #43: event-driven pull diagnostics (refresh + previousResultId + unchanged)#46
Merged
Merged
Conversation
…unchanged) Replace the Phase 2 blind-poll diagnostics path with the event-driven pull model the lsp-kotlin maintainer specified. - Detect pull-mode authoritatively from the subprocess InitializeResult's ServerCapabilities.diagnosticProvider (non-null ⇒ pull mode; there is no push capability flag). Pull machinery runs ONLY when pull-mode. - Override the BACKEND forwarder's workspaceDiagnosticRefresh() (the DefaultLanguageClient base THROWS NotImplementedError for it — the flagged trap; the frontend client's is a harmless no-op, a different object) to re-pull every open doc on workspace/diagnostic/refresh. - Trigger pulls on didOpen + debounced didChange, and on refresh. Bounded cold-index backoff covers only the FIRST pull; steady state is event-driven. - Thread DocumentDiagnosticParams.previousResultId per doc; skip republish on an unchanged report (RelatedUnchangedDocumentDiagnosticReport), only emit publishDiagnostics on a full report — kills redundant upward pushes. - Factor the pull/refresh/previousResultId/debounce/backoff logic into a cohesive, transport-agnostic PullDiagnosticsPublisher (internal) shaped for later extraction into lsp-ksrpc as a reusable DiagnosticBridge. Phase 3 position translation stays intact (ranges translate to csgs space before publishing). Flag-gated throughout (flag-off ⇒ no change). Tests: unit guards for pull-mode detection gating, unchanged-report skip, previousResultId threading, refresh→re-pull, close. Real-engine integration (local only, not CI): the deliberate error still flows via the event-driven path (didOpen pull + refresh) and a settled doc does not double-publish (publishes after settle=1, after 20s idle=1). Part of #35; Fixes #43. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
monkopedia-reviewer
approved these changes
Jun 4, 2026
Collaborator
monkopedia-reviewer
left a comment
monkopedia-reviewer
left a comment
There was a problem hiding this comment.
Approved — full tier-1 review.
Event-driven logic (correct):
- Pull-mode gating on
ServerCapabilities.diagnosticProvider != nullis authoritative (verified against the lsp lib: it's nullable and the only pull-mode signal); the publisher is created only in pull-mode. - The refresh override is on the backend forwarder (subprocess-facing
DefaultLanguageClientsubclass, whose baseworkspaceDiagnosticRefresh(): Nothing?throws) →onRefresh()re-pulls every open doc. Correct object, correct signature. previousResultIdthreaded per-doc under a per-docMutex(race-free); full report's nullableresultIdhandled with?.let, unchanged report's non-nullresultIdassigned directly — matches the lib's exact nullability.when(report)over the sealedDocumentDiagnosticReportis exhaustive: full → publish, unchanged → advance id, no republish.- Cold-index backoff is bounded (
maxAttempts, first-pull-only, loops only whileuri in openDocs) — the old unconditional retry/steady-poll loop is fully removed. No leak: publisher runs on the bridge scope, torn down onexit();onCloseclears per-doc state. - Phase 3 csgs range translation preserved on the publish seam.
Cohesion: PullDiagnosticsPublisher is internal, transport-agnostic, clean seams (pull/publish/awaitReady/debounce/backoff). No new public API.
Tests: Unit test genuinely exercises pull-mode gating, unchanged-skip, previousResultId threading, refresh→re-pull, close — not tautological. Real-engine no-double-publish guard correctly CI-skipped (-Dintegration).
Flag-off: unchanged; e2e flag-off green, LSP inert.
CI: build-and-test green (verified remotely) — spotless incl. :backend:spotlessKotlinCheck, ./gradlew test (compiles+runs the new unit test), and :e2e:test -Pe2e under Xvfb all SUCCESSFUL.
Idiomatic, well-documented, scope contained to the diagnostics path.
monkopedia-reviewer
pushed a commit
that referenced
this pull request
Jun 4, 2026
…e-time fixes (#47) Part of #35; Fixes #40. Flag-gated throughout (lspEnabled default OFF + Config.isKotlinLspAvailable): flag-off ⇒ no behavior change. Teardown (reverse-channel / sub-service release): - Frontend EditorPane tears down the LSP wiring on dispose (konstruction switch, flag toggle off, editor leaves composition) via produceState awaitDispose: shutdown→exit handshake then close() the lsp() server stub, run on LSPClient.scope so it survives the produceState coroutine cancel. - BridgeLanguageServer.close() releases the stashed reverse frontendClient channel and does per-konstruction cleanup. - Keep-warm preserved: per-konstruction teardown no longer forwards shutdown/exit/close to the SHARED subprocess stub (that killed the engine for other open konstructions and broke reopen); it sends a best-effort didClose so the engine forgets this doc and leaves the warm engine intact. Leak test (CI-runnable, no engine binary): LspSubserviceLeakTest opens+closes the nested lsp() sub-service 25x over an in-memory duplex ksrpc Connection and asserts the reverse client channel is released every cycle (count returns to baseline) — mirrors ksrpc's RpcSubserviceLeakTest. 3 PullDiagnosticsPublisher close-time fixes (maintainer #46 review): 1. uri-space key consistency — didClose keys onClose by params.textDocument.uri (same space as onOpen/onChange), not the stashed frontendUri. 2. clear-on-close — onClose emits publish(uri, emptyList()) (clearOnClose knob). 3. publish-after-close race — pullOnce re-checks uri in openDocs after the pull before publish. Unit tests added (9 publisher tests, was 6). Real-engine reopen integration test (local-only): open→close→reopen still flows diagnostics, clears on close, no stale diagnostics. Module-extraction decision documented (KDoc): NOT extracted — bake in konstructor first; extraction deferred per the lsp-kotlin maintainer. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #35; Fixes #43.
Replaces the Phase 2 blind-poll diagnostics path with the event-driven pull model the lsp-kotlin maintainer specified (two maintainer reviews captured in #43). Flag-gated throughout (
lspEnableddefault OFF +Config.isKotlinLspAvailable): flag-off ⇒ no behavioural change.The event-driven model
InitializeResult'sServerCapabilities.diagnosticProvider. Non-null ⇒ pull mode (there is no push capability flag). The pull machinery is created only when pull-mode; otherwise nothing runs. Sub-flagsinterFileDependencies/workspaceDiagnosticsare surfaced (informational; a refresh already re-pulls every open doc, honouring inter-file breadth).workspaceDiagnosticRefresh()override.DefaultLanguageClientsubclass (the one talking to the subprocess) THROWSNotImplementedErrorfor this unless overridden — the maintainer's flagged trap (the kodemirror frontend client's version is a harmless no-op, a different object). Overridden to re-pull every open doc on the engine'sworkspace/diagnostic/refresh("results may have changed, re-pull") — exactly the cold-index-warmed case, and the steady-state driver that makes this event-driven instead of a poll loop.previousResultId+unchangeddedup. Per-docresultIdis threaded into eachDocumentDiagnosticParams.previousResultId; anunchangedreport (RelatedUnchangedDocumentDiagnosticReport,kind=="unchanged") does not republish — only afullreport emitspublishDiagnostics. Kills redundant upward pushes.Shaped for extraction
The pull/refresh/
previousResultId/debounce/cold-index-backoff logic is factored into a cohesive, transport-agnosticPullDiagnosticsPublisher(keptinternalto konstructor — no new public API). Given(pull seam, publish seam, readiness gate, debounce, backoff)it tracks per-doc resultId, pulls on trigger/refresh, emits onfull/skipsunchanged.BridgeLanguageServersupplies only the three seams (pull = subprocesstextDocument/diagnostic; publish = translate-to-csgs + URI-rewrite + push; gate =initialized). Clean boundaries for the maintainer's later lift into lsp-ksrpc as a reusableDiagnosticBridge.Verification
:e2e:testgreen flag-off (incl.LspPipeTest, 0 failures/0 errors across suites); all-module./gradlew spotlessCheckgreen;./gradlew testgreen.PullDiagnosticsPublisherTest, CI-runnable): pull-mode detection gating,unchanged-report skip,previousResultIdthreading, refresh→re-pull, close.KONSTRUCTOR_KOTLIN_LSP→intellij-server. The deliberate unresolved-reference error still arrives via the event-driven path (didOpen pull + refresh), translated to csgs line 1; and a settled doc does not double-publish (publishes after settle=1, after 20s idle=1— the old 15s blind-poll would have grown the count). Engine path is not CI-tested.🤖 Generated with Claude Code