Track per-project load completion in LanguageServerProjectLoader

[JoeRobich]

Summary

Tracks initial Language Server project loads on each LoadedProject, allowing callers to await only the projects they requested and report progress independently. Project paths are normalized and compared using platform semantics to prevent duplicate loads, while failures, unloads, and shutdown reliably release waiting callers.

Adds deterministic tests covering concurrent loads, reloads, failures, path normalization, progress reporting, and disposal behavior.

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
2 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot review overview

🟡 Changes recommended

Path canonicalization in GetOrLoadProjectAsync can desynchronize the normalized loader key from primordial ProjectInfo/file-path string comparisons, risking incorrect document/project lookups for equivalent path strings.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2

New issues introduced by this change (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — GetOrLoadProjectAsync now canonicalizes the map key via NormalizeProjectPath(), but the primordial…

Pre-existing issues (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Issues resolved since last review (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — Structured logging placeholder name should be consistent (and usually PascalCase). This message… View resolved comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer.UnitTests/​LanguageServerProjectLoaderTests.cs — In the test harness, RemoteProjectLoadResult.ProjectRestorePath being null can hide… View resolved comment

[JoeRobich]

The nuget.org failures in build 1587731 came from Microsoft.CodeAnalysis.Testing.ReferenceAssemblies in the Public API analyzer tests, not the Language Server loader tests. The Helix work-item payload omitted the repository NuGet.config, so runtime NuGet resolution inherited the machine nuget.org source.

I opened #85210 to include NuGet.config in every Helix work-item payload. The config clears inherited sources and uses the approved feeds. The exact failing test passes with a fresh package cache, and the full Public API analyzer suite passes (317 tests).

[Copilot]

Copilot review overview

🟢 Approval recommended

The behavior change is well-contained, preserves prior semantics where expected, and is backed by comprehensive new unit tests covering concurrency, failure, and unload/shutdown scenarios.

Review tier: Lite
Findings: 2

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — GetOrLoadProjectAsync now canonicalizes the map key via NormalizeProjectPath(), but the primordial… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes core async/concurrency behavior in Language Server project loading/unloading paths and needs human validation for subtle race/regression risk despite strong unit test coverage.

Review tier: Lite
Findings: 2

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — GetOrLoadProjectAsync now canonicalizes the map key via NormalizeProjectPath(), but the primordial… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

[Copilot]

Copilot review overview

🔵 Needs a closer look

The IDE instructions update unintentionally removed an existing MSBuild project-extension convention note and should include both guidance items.

Review tier: Lite
Findings: 1

Pre-existing issues (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Issues resolved since last review (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — GetOrLoadProjectAsync now canonicalizes the map key via NormalizeProjectPath(), but the primordial… View resolved comment

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/instructions/IDE.instructions.md:107

• The IDE instructions file replaced the existing “MSBuild project extensions” gotcha rather than adding the new LanguageServer loading guidance alongside it. The MSBuild extension canonicalization behavior is still current (see ProjectFileExtensionRegistry.AddLeadingDot / GetRegisteredProjectFileExtensions) and is useful for contributors working in this layer, so it shouldn’t be dropped when adding the new LanguageServer note.

[jasonmalinowski]

Should we make these names match?

[JoeRobich]

By having them shadow the internal or protected methods?

[jasonmalinowski]

Yeah just shadow the protected methods. That way there's not two names bouncing around.

[jasonmalinowski]

Not sure if this test makes sense....

[jasonmalinowski]

Not sure if this is actually asserting anything interesting, since if we disposed the trackers, did we already do this?

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved correctness issues remain in cancellation completion, path containment, telemetry ordering, request-context handling, and loose-file fallback.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 · 1

New issues introduced by this change (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — The cancellation check at the start of the lookup runs before this try/finally. If the reload…

Pre-existing issues (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Suppressed comments (1)

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:169

• ❌ Moving the GUID assignment after BeginLoadingProjectAsync introduces a race: that method queues the reload before returning, and its cache lookup can keep the caller here suspended while the reload queue starts. The reload can therefore call ReportTelemetryIfNotPreviouslyReportedAsync before _projectGuidForTelemetry is set, causing solution projects to emit telemetry with a hashed/missing ID instead of the solution GUID. Pass the GUID into the begin operation and assign it before releasing the loader gate/queueing the reload (while preserving the existing-project case).

            var loadedProject = await BeginLoadingProjectAsync(path);
            if (guid is not null)
                await loadedProject.SetProjectGuidForTelemetryAsync(Guid.Parse(guid));

[jasonmalinowski]

If the project is unloaded we'd still dispose it. But we only cancel during shutdown which isn't a critical case.

[jasonmalinowski]

Suggested change

	- Language Server orchestration tests can pass additional MEF parts to
	`LanguageServerTestComposition.GetSharedExportProvider`. A controllable
	`PartNotDiscoverable` project loader can provide deterministic design-time
	build timing and results without invoking MSBuild.
	- Language Server orchestration tests can pass additional MEF parts to
	`LanguageServerTestComposition.GetSharedExportProvider` Adding parts
	with `PartNotDiscoverable` can provide the ability to insert test code or mocks.

[jasonmalinowski]

Do we need a TryParse here if the GUID isn't parsable?

[jasonmalinowski]

I'm OK here not having the HangMitigatingTimeouts, since if we deadlock the dump would be easy to diagnose. If you make the WaitForLoadAsync cancellation token optional, it'd also tighten this up a lot.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve the restore-completion, project-GUID telemetry race, target-success signaling, and unused-helper findings.

Review tier: Lite
Findings: 1 · 1

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — The cancellation check at the start of the lookup runs before this try/finally. If the reload… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Suppressed comments (4)

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs:312

• This completion is signaled before ReloadProjectsAsync runs automatic restore: the restore block at lines 188-193 executes only after all ReloadProjectAsync calls have returned. Consequently OpenSolutionAsync/OpenProjectsAsync can report 100% progress and send project-initialization-complete while dotnet restore is still updating assets, whereas the previous batch wait included that restore barrier. Please include restore completion in the per-request load task (or otherwise preserve the barrier) before releasing these waiters.

        finally
        {
            loadedProject.CompleteInitialLoad();
        }

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:202

• This new internal helper has no callers anywhere in the repository—the only reference is its declaration—so it adds dead API surface without affecting project loading. Please remove it (and any import that becomes unused), or add its intended call site in this change.

    internal ImmutableArray<string> GetSupportedProjectFileExtensions()
    {
        var supportedLanguages = _hostProjectFactory.Workspace.Services.SolutionServices.GetSupportedLanguages<ICommandLineParserService>();
        return _projectFileExtensionRegistry.GetRegisteredProjectFileExtensions().WhereAsArray(
            extension => _projectFileExtensionRegistry.TryGetLanguageNameFromExtension(extension, out var languageName) && supportedLanguages.Contains(languageName));

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:169

• The solution GUID is now assigned only after BeginLoadingProjectAsync returns, but that method queues the reload before doing its cache I/O. A queued design-time build can therefore commit and call ReportTelemetryIfNotPreviouslyReportedAsync before this assignment, causing the one-shot telemetry record to permanently capture a null project GUID. Pass the GUID into the begin method and set it before releasing the loader gate/queueing the work, as the previous implementation did.

            var loadedProject = await BeginLoadingProjectAsync(path);
            if (guid is not null)
                await loadedProject.SetProjectGuidForTelemetryAsync(Guid.Parse(guid));

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs:96

• _targets.Count > 0 does not necessarily mean that an evaluated target exists. TryApplyLoadedProjectInfosAsync adds a new Target before UpdateWithNewProjectInfoAsync finishes; if that update throws or is canceled, the finally still completes this TCS and this predicate reports a partially initialized target as a successful load. Please make target creation/update transactional, or only count targets after a successful update, before using this as the load-success signal.

        using (await _gate.DisposableWaitAsync(cancellationToken))
        {
            return !_disposed && (_primordialProjectInfo.HasValue || _targets.Count > 0);
        }

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical test-project compile issue and a telemetry initialization race remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2 · 1

New issues introduced by this change (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer.UnitTests/​Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj — Removing this linked source file leaves the existing NamedPipeUtil.CreateClient(...) calls in…

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — The cancellation check at the start of the lookup runs before this try/finally. If the reload… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:169

• Setting the telemetry GUID only after BeginLoadingProjectAsync returns introduces a race that the previous API avoided. BeginLoadingProjectAsync queues the reload before awaiting cache work; if that work takes longer than the queue delay, ReloadProjectAsync can report telemetry before this assignment, and ReportTelemetryIfNotPreviouslyReportedAsync latches _reportedTelemetry, so the solution project is permanently reported without its GUID. Pass the GUID into the begin operation and assign it before the reload can run (under the loader synchronization), or otherwise synchronize the assignment with the initial load.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:203

• GetSupportedProjectFileExtensions is added here, but the only repository call-site is its declaration, so this introduces an unused project-system surface and an otherwise unobservable language-service lookup. Please either remove this unrelated API from the change or add the consumer/test that requires it; keeping dead code here makes the loader harder to reason about.

    internal ImmutableArray<string> GetSupportedProjectFileExtensions()
    {
        var supportedLanguages = _hostProjectFactory.Workspace.Services.SolutionServices.GetSupportedLanguages<ICommandLineParserService>();
        return _projectFileExtensionRegistry.GetRegisteredProjectFileExtensions().WhereAsArray(
            extension => _projectFileExtensionRegistry.TryGetLanguageNameFromExtension(extension, out var languageName) && supportedLanguages.Contains(languageName));
    }

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues remain in telemetry initialization, restore-inclusive completion, and progress-test synchronization.

Review tier: Lite
Findings: 1 · 1

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — The cancellation check at the start of the lookup runs before this try/finally. If the reload… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Issues resolved since last review (1)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer.UnitTests/​Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj — Removing this linked source file leaves the existing NamedPipeUtil.CreateClient(...) calls in… View resolved comment

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:169

• Moving the GUID assignment after BeginLoadingProjectAsync introduces a race with the reload that BeginLoadingProjectAsync queues before it awaits cache loading. If cache loading takes longer than the queue delay, ReloadProjectAsync can commit and call ReportTelemetryIfNotPreviouslyReportedAsync before _projectGuidForTelemetry is set; that method marks telemetry as reported, so this project can permanently emit telemetry without its solution GUID. Set the GUID inside BeginLoadingProjectAsync before _projectsToReload.AddWork (or otherwise guarantee assignment before queuing the reload).
  src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:202
• This new internal method has no callers anywhere in the repository, so it currently adds dead API surface and does not participate in project loading or extension discovery. Please remove it until a consumer is added, or add the intended consumer in this change.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LanguageServerProjectLoaderTests.cs:243

• These assertions run as soon as the project completion sources finish, but OnItemProcessed only enqueues a separate zero-delay AsyncBatchingWorkQueue operation; Task.WhenAll(firstLoad, secondLoad) does not wait for those progress reports. The test can therefore observe no 99% report and fail nondeterministically. Await/dispose the progress trackers before asserting their intermediate reports.

            Assert.Contains(firstReporter.Reports, report => report is LSP.WorkDoneProgressReport { Percentage: 99 });
            Assert.Contains(secondReporter.Reports, report => report is LSP.WorkDoneProgressReport { Percentage: 99 });

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:194

• The same change makes OpenProjectsAsync return and send workspace/projectInitializationComplete before the batch-level automatic restore finishes: each LoadedProject signals completion in ReloadProjectAsync before ReloadProjectsAsync awaits RestoreProjectsAsync. This regresses the old restore-inclusive wait for callers opening projects; preserve a restore-inclusive completion for this explicit-open path.

        await WaitForProjectLoadsAsync(loadedProjects.MoveToImmutable(), progressTracker, CancellationToken.None);

src/Workspaces/MSBuild/Test/NetCoreTests.cs:770

• The previous exact-length assertion was removed, and Assert.All plus the three Assert.Contains calls still permits unexpected extensions to be registered. This test constructs a fresh registry with three built-ins, so retain the length check to ensure accidental extra registrations are detected.

        Assert.All(registeredExtensions, extension => Assert.StartsWith(".", extension));

[Copilot]

Copilot review overview

🔵 Needs a closer look

The solution telemetry ordering issue must be fixed before approval, with the remaining review comments also outstanding.

Review tier: Lite
Findings: 1 · 1

Pre-existing issues (2)

Severity	Finding
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — The cancellation check at the start of the lookup runs before this try/finally. If the reload… View comment
	src/​LanguageServer/​Microsoft.CodeAnalysis.LanguageServer/​HostWorkspace/​LanguageServerProjectLoader.cs — _loadedProjects and the reload queue are now keyed with PathUtilities.Comparer. Project identifiers… View comment

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:169

• The solution GUID is assigned only after BeginLoadingProjectAsync returns, but that method queues the design-time build before it awaits the cache read. If the cache read takes longer than the queue's 100 ms delay, the reload can commit and call ReportTelemetryIfNotPreviouslyReportedAsync before _projectGuidForTelemetry is set, so solution telemetry falls back to a hashed path instead of the solution GUID. Thread the parsed GUID into BeginLoadingProjectAsync and set it before enqueuing the load.

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LanguageServerProjectLoaderTests.cs:54

• This test is named for concurrent callers, but the first BeginLoadAsync is fully awaited before the second call starts, so it never exercises concurrent entry into the loader or its cache-load interleaving. Start both tasks before awaiting them (or rename the test); otherwise a regression in the concurrent-call path can pass without being covered.

        var firstLoadedProject = await loader.BeginLoadAsync(projectPath);
        var secondLoadedProject = await loader.BeginLoadAsync(equivalentPath);

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs:203

• This new internal helper has no callers in the repository, so it currently adds dead surface without changing project discovery or loading behavior; AutoLoadProjectsInitializer still enumerates only *.csproj. Please either wire this into the intended discovery path or remove the helper from this change so the PR stays scoped and the unused API does not become maintenance debt.

    internal ImmutableArray<string> GetSupportedProjectFileExtensions()
    {
        var supportedLanguages = _hostProjectFactory.Workspace.Services.SolutionServices.GetSupportedLanguages<ICommandLineParserService>();
        return _projectFileExtensionRegistry.GetRegisteredProjectFileExtensions().WhereAsArray(
            extension => _projectFileExtensionRegistry.TryGetLanguageNameFromExtension(extension, out var languageName) && supportedLanguages.Contains(languageName));
    }