Fix race condition in language client visibility notifications#4256
Fix race condition in language client visibility notifications#4256
Conversation
Add isRunning() check before sending notifications to the language client to prevent "Client is not running" errors in test environments where the client may not have fully started yet. Co-authored-by: cklin <1418580+cklin@users.noreply.github.com>
|
This is not the right solution. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an intermittent race condition in the CodeQL language client where visibility notifications were being sent before the language client finished starting, causing "Client is not running" errors in Windows tests.
Changes:
- Added an
isRunning()guard inCodeQLLanguageClient.notifyVisibilityChange()to prevent sending notifications before the client is fully started
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I thought about it more and explored a different approach through #4274. Ideally, we should get to the bottom of the underlying race condition. But the editor visibility change is not really under our control, and there is not much point for the extension to notify itself of the visibility change if it (the extension) had already stopped running. So I think this is the best way to mitigate the flaky test. |
The
SourceMap › should jump to QL codetest was failing intermittently on Windows with "Client is not running" errors. ThenotifyVisibilityChange()method was sending notifications before the language client finished starting.Changes
Added an
isRunning()guard inCodeQLLanguageClient.notifyVisibilityChange():This matches the pattern used elsewhere in the codebase (e.g.,
extension.ts:201).Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.