Get accurate Windows version info from registry instead of os.release()#295842
Get accurate Windows version info from registry instead of os.release()#295842
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows version detection when running VS Code in compatibility mode by reading accurate version information from the Windows registry instead of relying on os.release(), which can return incorrect values due to the deprecated GetVersionEx API.
Changes:
- Created a new
windowsVersion.tsmodule that reads Windows version info from the registry using@vscode/windows-registry - Replaced all usages of the old
getWindowsBuildNumber()function with new async/sync variants - Added initialization of Windows version info during app startup to ensure accurate data is available
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/base/node/windowsVersion.ts | New module providing registry-based Windows version detection with async/sync APIs |
| src/vs/code/electron-main/main.ts | Added initialization call for Windows version info during app startup |
| src/vs/platform/windows/electron-main/windowsMainService.ts | Updated to use async getWindowsRelease() for window configuration |
| src/vs/platform/terminal/node/terminalEnvironment.ts | Removed old getWindowsBuildNumber() function, updated to use async version |
| src/vs/platform/terminal/node/terminalProfiles.ts | Updated to use async getWindowsBuildNumberAsync() |
| src/vs/platform/terminal/node/terminalProcess.ts | Updated to use sync getWindowsBuildNumberSync() in constructor |
| src/vs/platform/terminal/node/ptyService.ts | Updated to use async getWindowsBuildNumberAsync() in async methods |
| src/vs/platform/remote/node/wsl.ts | Removed duplicate getWindowsBuildNumber() function, updated to use async version |
| src/vs/platform/terminal/test/node/terminalEnvironment.test.ts | Updated test conditions to use async getWindowsBuildNumberAsync() |
Comments suppressed due to low confidence (1)
src/vs/platform/terminal/test/node/terminalEnvironment.test.ts:43
- This top-level await in the conditional suite selection won't work correctly. The suite function is called synchronously during test discovery, but the await expression makes this asynchronous. This means the condition will always evaluate using the Promise object itself (which is truthy), not the awaited value.
Consider using getWindowsBuildNumberSync() instead, or restructure the test to evaluate the condition inside a setup function and use suite.skip() conditionally.
(await getWindowsBuildNumberAsync() < 18309 ? suite.skip : suite)('pwsh', async () => {
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
@deepak1556Matched files:
|
bpasero
left a comment
There was a problem hiding this comment.
I am worried about the impact on startup performance such a change has. We now block window opening with this new code that asks the windows registry for something which we do not know when it returns. If we need this info in a reliable way, then it should be moved to the places where we need it and wait there but with sensitive fallback mechanisms if the windows registry is very slow.
Fixed - using sync version. This value is only ever used to check OS release on Mac, and the new functionality is not useful there - so even if by any chance it is not initialized by the time this runs, it should be fine. |
deepak1556
left a comment
There was a problem hiding this comment.
LGTM on the usage in update code, also like that the registry information is cached for the lifetime of the process.
No blockers from my end, what would be nice is we only get the information from registry if we are running in compatibility mode but again checking if we are in compat mode requires probing registry :(
I think there may be env variable to check for compat mode, but I'd rather not rely on that - I actually like the fact that it's consistent always getting OS info from the source. |
|
The trade off I was coming at is, From https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getversion
From https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1
We do ship a manifest today https://github.com/microsoft/vscode/blob/main/resources/win32/appx/AppxManifest.xml without any compatibility section https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1, but again the manifest is not properly assigned to the executable because of a known bug in the OS #254613 (comment), could this be the cause rather than being invoked as compat mode for the bug ? Followup would be,
|
Switched to sync version to avoid delays.
There was a problem hiding this comment.
I think this is better but its fragile: we do not have a deterministic release info on the window anymore now because initWindowsVersionInfo may or may not succeed at the time the window opens. I think a better approach is to leave os.release() in for the window and use the async variant in places were we really need to rely on it and where we can wait on it without causing issues.
Sure, no problem - reverted the change. This property is not used for anything of significance on Windows, so it should be fine. Ideally, we should remove it - since it cannot be properly initialized, it may return incorrect value on Windows. |
|
@deepak1556 We read once and cache it, I don't see it as a perf concern to be honest. |
|
I am also not concerned about performance in this implementation however when the registry read fails and |
I'll see if I can detect and log running in compat mode! |
Fixes #197444
Changes
Used Windows registry to retrieve Windows release and build numbers.
Added passing User-Agent header to update service.
Updated installer launch env to ignore compat mode.
Note
When running in compatibility mode os.release() may return incorrect version info which leads to VS Code downgrade (downloading an old version) and terminal not working.
Validation
Start VS Code with compatibility settings 8.1, wait for update to be installed -> verify the update is next version (not an old one), click Restart -> VS Code should restart into the new version. You should see no errors anywhere along the way. Verify terminal works under these conditions.