Handle concurrent requests to update workspace contents and workspace SG info. #79628

CyrusNajmabadi · 2025-07-28T16:17:27Z

Found by Jason in our integration tests (which often kick off an SG regen due to intellisense-builds completing) and then quickly trying to validate some other feature which can then collide with teh workspace changed caused by that.

The core issue is that tehre are two effective streams of 'changes' mutating the workspace. The first is some actual user-facing feature in question that attempts to call TryApplyChanges on the workspace to update it with whatever work it is doing. The second is that the workspace's host can call into it to tell it whne a file-save/project-build completes so that SG info for that file/project can be appropriately recomputed.

These operations are fundamentally racey in nature, and a request by a feature to update the workspace could fail if the host-request came in between the time that it captured the original solution and then tried to apply the new solution.

In practice this is unlikely to happen as the user would have to somehow do soemthing like kick off a build in between a feature starting/ending. However, it does happen in integration tests, and it does represent an unpleasant combination of factors that is hard to reason about.

To address this, we now separate out the concepts of a solution-content-change (literally something that changes something about the bitwise representation of the contents of the solution), versus the ambient SG execution versions (which control if we think we should rerun generators or not, regardless of the state of files). We still have the rule that if another content change snuck in that we will fail a TryApplyChanges. But we do allow the ambient SG execution versions to change.

… SG info

CyrusNajmabadi · 2025-07-28T16:18:32Z

src/EditorFeatures/Core/EditAndContinue/PdbMatchingSourceTextProvider.cs

@@ -29,7 +29,7 @@ internal sealed class PdbMatchingSourceTextProvider() : IEventListener, IPdbMatc
    private readonly object _guard = new();

    private bool _isActive;
-    private int _baselineSolutionVersion;
+    private int _baselineSolutionContentVersion;


Renamed the generic 'SolutionVersion' concept in many placces to 'SolutionContentVersion' indicating that it revs as the content moves forward.

src/EditorFeatures/Core/EditAndContinue/PdbMatchingSourceTextProvider.cs

CyrusNajmabadi · 2025-07-28T16:20:06Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

@@ -53,7 +52,7 @@ internal sealed class LspWorkspaceManager : IDocumentChangeTracker, ILspService
    /// workspace).
    /// <para/> Access to this is guaranteed to be serial by the <see cref="RequestExecutionQueue{RequestContextType}"/>
    /// </summary>
-    private readonly Dictionary<Workspace, (int? forkedFromVersion, Solution solution)> _cachedLspSolutions = [];
+    private readonly Dictionary<Workspace, (int? forkedFromVersion, SourceGeneratorExecutionVersionMap? versionMap, Solution solution)> _cachedLspSolutions = [];


@dibarbet i wanted to maintain the semantics you had before as they may be important. in this case, we only attempt to remerge if the forked version and the execution version maps are the same.

I think it is correct to only use the cached solution only if the version+execution version are the same.

Because if the workspace execution version changes, we would want to re-fork from the workspace to ensure we have the newly run generators (and not re-use the cached solution where generators did not run).

CyrusNajmabadi · 2025-07-28T16:23:34Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+    private static int DetermineNextSolutionStateContentVersion(Solution newSolution, Solution oldSolution)
+        => oldSolution.SolutionState == newSolution.SolutionState
+            ? oldSolution.SolutionStateContentVersion // If the solution state is the same, we can keep the same version.
+            : oldSolution.SolutionStateContentVersion + 1; // Otherwise, increment the version.


A core part of the change. Content version only moves forward if hte content actually changes.

To my other comment -- as far as I can tell this should be the only part that's necessary, and the other part isn't necessary at all...

you are right :)

CyrusNajmabadi · 2025-07-28T16:24:14Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

            return true;
        }
+
+        void ApplySourceGeneratorExecutionMap()


Major part of the change. Docs below describe the core logic we care about here.

CyrusNajmabadi · 2025-07-28T17:03:37Z

@jasonmalinowski ptal.

src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratorExecutionVersion.cs

…rExecutionVersion.cs Co-authored-by: Joey Robichaud <[email protected]>

dibarbet · 2025-07-29T18:54:25Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

@@ -53,7 +52,7 @@ internal sealed class LspWorkspaceManager : IDocumentChangeTracker, ILspService
    /// workspace).
    /// <para/> Access to this is guaranteed to be serial by the <see cref="RequestExecutionQueue{RequestContextType}"/>
    /// </summary>
-    private readonly Dictionary<Workspace, (int? forkedFromVersion, Solution solution)> _cachedLspSolutions = [];
+    private readonly Dictionary<Workspace, (int? forkedFromVersion, SourceGeneratorExecutionVersionMap? versionMap, Solution solution)> _cachedLspSolutions = [];


I think it is correct to only use the cached solution only if the version+execution version are the same.

Because if the workspace execution version changes, we would want to re-fork from the workspace to ensure we have the newly run generators (and not re-use the cached solution where generators did not run).

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

dibarbet · 2025-07-29T18:59:34Z

src/Tools/ExternalAccess/Razor/Features/SolutionExtensions.cs

@@ -16,5 +16,5 @@ public static ImmutableArray<DocumentId> GetDocumentIds(this Solution solution,
        => LanguageServer.Extensions.GetDocumentIds(solution, new(documentUri));

    public static int GetWorkspaceVersion(this Solution solution)
-        => solution.WorkspaceVersion;
+        => solution.SolutionStateContentVersion;


cc @davidwengier - I am not sure what you use this for, but you may want to check that this is correct (whether or not you care that this changes if the source generators execute)

Thanks for tagging me. This is just a tie-break for two Razor documents, when their content matches, so anything that moves forward over time is good.

dibarbet · 2025-07-29T19:01:23Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

                return (workspaceCurrentSolution, IsForked: false);
            }

            // Step 4: See if we can reuse a previously forked solution.
-            if (cachedSolution != default && cachedSolution.forkedFromVersion == workspaceCurrentSolution.WorkspaceVersion)
+            if (cachedSolution != default &&


can you add a test to LspWorkspaceManagerTests to verify this behavior?

CyrusNajmabadi · 2025-07-29T19:07:02Z

Because if the workspace execution version changes, we would want to re-fork from the workspace to ensure we have the newly run generators (and not re-use the cached solution where generators did not run).

WFM.

CyrusNajmabadi · 2025-07-29T19:58:01Z

@jasonmalinowski ptal.

CyrusNajmabadi · 2025-07-30T15:23:41Z

@jasonmalinowski ptal.

src/Workspaces/Core/Portable/Workspace/Workspace.cs

jasonmalinowski · 2025-07-30T20:53:09Z

src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs

@@ -1568,6 +1568,78 @@ internal async Task TestNonCompilationLanguage(SourceGeneratorExecutionPreferenc
        Assert.NotEqual(initialExecutionMap[noCompilationProject.Id], finalExecutionMap[noCompilationProject.Id]);
    }

+    [Theory, CombinatorialData]


I'm not sure why this test is here versus SolutionWithSourceGeneratorTests.cs.

CyrusNajmabadi

c

CyrusNajmabadi · 2025-07-30T21:02:08Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+    private static int DetermineNextSolutionStateContentVersion(Solution newSolution, Solution oldSolution)
+        => oldSolution.SolutionState == newSolution.SolutionState
+            ? oldSolution.SolutionStateContentVersion // If the solution state is the same, we can keep the same version.
+            : oldSolution.SolutionStateContentVersion + 1; // Otherwise, increment the version.


you are right :)

src/Workspaces/Core/Portable/Workspace/Workspace.cs

jasonmalinowski

Approved; question about why the test is in ServiceHubServicesTests still stands but requires no re-review.

CyrusNajmabadi · 2025-07-30T21:42:22Z

Approved; question about why the test is in ServiceHubServicesTests still stands but requires no re-review.

It was just convenient. But i moved to SolutionWithSourceGenTests as that does make more sense :)

CyrusNajmabadi added 3 commits July 28, 2025 17:29

Handle concurrent requests to update workspace contents and workspace…

620fb3a

… SG info

Fallout

0dbae8b

Add test

15633cd

CyrusNajmabadi requested a review from a team as a code owner July 28, 2025 16:17

github-actions bot added the Area-IDE label Jul 28, 2025

dotnet-policy-service bot added the VSCode label Jul 28, 2025

CyrusNajmabadi commented Jul 28, 2025

View reviewed changes

src/EditorFeatures/Core/EditAndContinue/PdbMatchingSourceTextProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi commented Jul 28, 2025

View reviewed changes

src/EditorFeatures/Core/EditAndContinue/PdbMatchingSourceTextProvider.cs Show resolved Hide resolved

Apply suggestions from code review

1b9cf50

CyrusNajmabadi commented Jul 28, 2025

View reviewed changes

Docs

7d3b2ef

CyrusNajmabadi commented Jul 28, 2025

View reviewed changes

Docs

6446625

CyrusNajmabadi requested a review from jasonmalinowski July 28, 2025 17:03

JoeRobich reviewed Jul 29, 2025

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratorExecutionVersion.cs Outdated Show resolved Hide resolved

Update src/Workspaces/Core/Portable/Workspace/Solution/SourceGenerato…

a852d51

…rExecutionVersion.cs Co-authored-by: Joey Robichaud <[email protected]>

dibarbet reviewed Jul 29, 2025

View reviewed changes

CyrusNajmabadi added 3 commits July 29, 2025 21:08

Merge remote-tracking branch 'upstream/main' into sgMerging

d55947d

Use checksum

bf09940

Add lsp tests

9c8839d

dibarbet approved these changes Jul 29, 2025

View reviewed changes

jasonmalinowski reviewed Jul 30, 2025

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Workspace.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Jul 30, 2025

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Workspace.cs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into sgMerging

0a3eba2

jasonmalinowski reviewed Jul 30, 2025

View reviewed changes

remove unnecessary code

a272b5a

CyrusNajmabadi commented Jul 30, 2025

View reviewed changes

Simplify

1b03f37

CyrusNajmabadi requested a review from jasonmalinowski July 30, 2025 21:07

jasonmalinowski approved these changes Jul 30, 2025

View reviewed changes

CyrusNajmabadi added 2 commits July 30, 2025 23:35

Spelling

78b4641

Move tests

3da0a85

CyrusNajmabadi enabled auto-merge July 30, 2025 21:42

CyrusNajmabadi added 2 commits July 30, 2025 23:43

Remove

cc8dd5f

Add back

cb9aec5

CyrusNajmabadi merged commit f9090f4 into dotnet:main Jul 30, 2025
24 of 25 checks passed

CyrusNajmabadi deleted the sgMerging branch July 30, 2025 22:55

dotnet-policy-service bot added this to the Next milestone Jul 30, 2025

This was referenced Aug 1, 2025

[Automated] PRs inserted in VS build main-10831.79 #79728

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10831.186 #79738

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

dotnet-bot mentioned this pull request Aug 22, 2025

[Automated] PRs inserted in VS build feature.Wix5-10921.95 #80011

Closed

Handle concurrent requests to update workspace contents and workspace SG info. #79628

Handle concurrent requests to update workspace contents and workspace SG info. #79628

Uh oh!

Conversation

CyrusNajmabadi commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 28, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 29, 2025

Uh oh!

CyrusNajmabadi commented Jul 29, 2025

Uh oh!

CyrusNajmabadi commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Jul 28, 2025 •

edited

Loading