fix(lsp): add poll retry for push-only diagnostics servers #1731
+224
−1
Conversation
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
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 4/5
- Safe to merge with low risk; the only issue is a low‑severity polling behavior and no other concerns were flagged.
- In
src/tools/lsp/lsp-client.ts, the polling loop treats empty diagnostics as missing, which can force a full timeout even when the server reports a clean file, potentially slowing feedback. - Pay close attention to
src/tools/lsp/lsp-client.ts- polling loop ignores valid empty diagnostics and may wait unnecessarily.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/lsp/lsp-client.ts">
<violation number="1" location="src/tools/lsp/lsp-client.ts:114">
P2: Polling loop ignores valid empty diagnostics, forcing full timeout even when server reports a clean file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…gyu#1522) Closes code-yeongyu#1522 ## Summary - After pull diagnostics fails, poll diagnosticsStore every 200ms for up to 3s - Fixes timing race where push diagnostics arrive after pull fails but before store read - Returns immediately if push arrives during poll window - Falls back to empty after timeout ceiling (same behavior as before, just with retry) - Servers like Godot that only support push diagnostics now work correctly Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
potb
force-pushed
the
fix/1522-lsp-diagnostics-race
branch
from
729db1c to
3a717b3
Compare
Contributor
Author
|
@cubic-dev-ai recheck |
@potb I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 4/5
- Behavioral impact is limited: the polling loop in
src/tools/lsp/lsp-client.tstreats empty diagnostics as no update, so error-free files may wait an extra 3s before being considered updated. - Test suite slowdown in
src/tools/lsp/client.test.tsis minor and non-blocking, but it does add avoidable wait time each run. - Pay close attention to
src/tools/lsp/lsp-client.tsandsrc/tools/lsp/client.test.ts- polling logic and timer usage can introduce delays.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tools/lsp/lsp-client.ts">
<violation number="1" location="src/tools/lsp/lsp-client.ts:114">
P2: Polling loop ignores valid empty diagnostics updates, forcing a 3s timeout delay for error-free files.</violation>
</file>
<file name="src/tools/lsp/client.test.ts">
<violation number="1" location="src/tools/lsp/client.test.ts:302">
P3: This test will wait for the full diagnostics poll timeout (~3.5s) every run because timers aren’t mocked; it slows the test suite unnecessarily. Consider mocking setTimeout/using fake timers or overriding the timeout to keep the test fast.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
… tests Poll loop now checks `items !== undefined` instead of `items && items.length > 0`, so clean files with zero diagnostics are recognized immediately rather than forcing a full 3s timeout. Tests mock setTimeout/Date.now to avoid real timer waits. Addresses review feedback identified by cubic on code-yeongyu#1731.
potb
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #1522
Push-only LSP servers (e.g. Godot) return nothing on pull requests. When pull fails, we just return empty — but push diagnostics often arrive moments later.
Added a poll retry: on pull failure, check the diagnostics store every 200ms for up to 3s. Returns immediately when data arrives, falls back to empty on timeout (same as before).