Skip to content

Do not sync install states across machines#2620

Draft
nagilson wants to merge 3 commits intodotnet:mainfrom
nagilson:nagilson-race-condition-container
Draft

Do not sync install states across machines#2620
nagilson wants to merge 3 commits intodotnet:mainfrom
nagilson:nagilson-race-condition-container

Conversation

@nagilson
Copy link
Copy Markdown
Member

@nagilson nagilson commented Mar 31, 2026

Resolves microsoft/vscode-dotnettools#2759

There are 2 issues that compound to cause the issue above.

The issue there is caused by:

  1. Extension state where PC 1 contains the install is shared into PC 2 (container)
  2. PC 2 checks early on whether the installed state is valid or not - except, instead of using the path from PC 1, it uses what would be a valid path for PC2 if the install were to exist
  3. PC 2 caches a value for the install path saying it's invalid
  4. PC 2 correctly installs the install
  5. PC 2 receives another request to install. It sees if there is a valid existing install. The cache says the install is invalid.
  6. It wipes the directory

Issues addressed:

1. Some extension states should not be shared/synced:

Keys in the global extension storage state are synced (from the primary machine to the container image) when the user turns on configuration settings sync. I had no idea. This means we will sync file paths from different machines, even different OS's, across one another. All of our data keys are related to the specific machine so we should definitely not sync any of them.

https://github.com/microsoft/vscode/blob/231f8d745c9e59b6a62ae33f230002d99216800e/src/vscode-dts/vscode.d.ts#L8454

2. The cache does not get invalidated in some cases for install/uninstall.

 Upon any install / uninstall operation we should clear the cache of anything with that path in it.
The clear in the invoker is not necessary, but it is a good 'defense in depth'. We should rarely get to that state if ever now?

Potential Other Approaches

Alternatively, we could just decide not to cache any failures. But this would mean no dotnet exists checks would cause a lot of wasted extra processes. I don't think we need to do this since we wipe the 'dotnet' ones after admin install / uninstall but obviously this wouldn't pick up external installers. That's... ok.

Other Considerations

It's technically a bug that in the calls where we wipe a directory, we don't enforce that we checked if the install is in use using the install tracker method.
The only place we don't do that is in the installer itself. Concurrently it needs to hold a mutex on that install for it to work, so this isn't a race condition, I don't think this can be a race condition bug actually. It can only be a sequential bug.

We could enforce the is it in use check on the wipe part of the installer, but it already tries to check if it's valid or not.
By checking if the install is valid while invalidating the cache before hand, we'd truly only invalidate an already invalid install:
install it -> invalidate it -> it will get deleted (bad, but it was invalidated)

Instead of wiping the cache we could do the is in use check:
is in use -> (if its invalid) -> we return invalid install but don't wipe the cache of everything pertaining to that install.

We only wipe the local runtime specific folders in this case (not 'dotnet') so I don't expect a large perf regression.

https://github.com/microsoft/vscode/blob/231f8d745c9e59b6a62ae33f230002d99216800e/src/vscode-dts/vscode.d.ts#L8454

Keys in the global state are synced when the user turns on configuration settings sync. I had no idea. This means we will sync file paths from different machines, even different OS's, across one another. All of our data keys are related to the specific machine so we should definitely not sync any of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Language Server Failure on startup

1 participant