Skip to content

Partially revert #13660: remove NuGet RestoreTask transient TaskHost workaround - #14297

Merged
JanProvaznik merged 3 commits into
dotnet:mainfrom
JanProvaznik:revert-restoretask-transient-taskhost-workaround
Jul 9, 2026
Merged

Partially revert #13660: remove NuGet RestoreTask transient TaskHost workaround#14297
JanProvaznik merged 3 commits into
dotnet:mainfrom
JanProvaznik:revert-restoretask-transient-taskhost-workaround

Conversation

@JanProvaznik

Copy link
Copy Markdown
Member

Summary

Partially reverts #13660. That PR added a temporary workaround routing NuGet's RestoreTask to a transient (non-sidecar) TaskHost when running in /mt or MSBuild Server mode, to prevent NuGet's static singleton state (PluginManager, EnvironmentWrapper) from leaking across build invocations in a long-lived process.

This change removes that workaround so RestoreTask again follows normal TaskHost routing in all modes.

What is removed (workaround)

  • TaskRouter.RequiresTransientTaskHost + the NuGet.Build.Tasks.RestoreTask allow-list entry
  • AssemblyTaskFactory forceTransientTaskHost branch (useSidecarTaskHost reverted to pre-PR form)
  • BuildParameters.IsLongLivedHost / MarkProcessAsLongLivedHost() infrastructure (field, property, copy-ctor, Translate)
  • OutOfProcServerNode.Run() MarkProcessAsLongLivedHost() call
  • Associated TaskRouter_IntegrationTests cases and the mock NuGet.Build.Tasks.RestoreTask

What is kept (generic diagnostics from the same PR)

  • TaskHostTask TaskHostDetails per-invocation diagnostic logging
  • NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost out hostProcessId, out wasNewlyCreated parameters
  • The TaskHostDetails resource string and its localized .xlf entries

Validation

  • Microsoft.Build builds clean (0 warnings/errors)
  • Microsoft.Build.Engine.UnitTests (net10.0) builds clean; remaining 7 TaskRouter_IntegrationTests pass
  • No dangling references to any removed symbol remain

Notes

The underlying NuGet static-state issue tracked in #13315 / #12246 should be considered when deciding whether this workaround is still needed.

…ost workaround

PR dotnet#13660 routed NuGet's RestoreTask to a transient (non-sidecar) TaskHost
in /mt and MSBuild Server modes to avoid leaking NuGet's static singleton
state across invocations. This removes that workaround so RestoreTask again
follows normal TaskHost routing in all modes, while keeping the generic
per-invocation TaskHost diagnostic logging (TaskHostDetails) introduced by
the same PR.

Removed:
- TaskRouter.RequiresTransientTaskHost + RestoreTask allow-list entry
- AssemblyTaskFactory forceTransientTaskHost branch (useSidecarTaskHost reverted)
- BuildParameters.IsLongLivedHost / MarkProcessAsLongLivedHost infrastructure
- OutOfProcServerNode.MarkProcessAsLongLivedHost() call
- Associated TaskRouter integration tests and mock RestoreTask

Kept:
- TaskHostTask TaskHostDetails diagnostic logging
- NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost out-params
  (hostProcessId, wasNewlyCreated) and localized resource strings

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik requested a review from OvesN July 9, 2026 10:52
@JanProvaznik
JanProvaznik marked this pull request as ready for review July 9, 2026 10:52
Copilot AI review requested due to automatic review settings July 9, 2026 10:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR partially reverts #13660 by removing the special-case routing that forced NuGet’s RestoreTask to run in a transient (non-sidecar) TaskHost under /mt and MSBuild Server scenarios, returning restore task execution to the normal TaskHost routing behavior. It keeps the generic TaskHost diagnostics additions from #13660.

Changes:

  • Removed TaskRouter.RequiresTransientTaskHost and all RestoreTask-specific allow-list logic.
  • Removed BuildParameters.IsLongLivedHost / MarkProcessAsLongLivedHost() plumbing and the server-node initialization hook.
  • Deleted the associated integration tests and mock RestoreTask used to validate the workaround behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs Removes transient-TaskHost forcing for RestoreTask; restores pre-workaround sidecar selection logic.
src/Build/BackEnd/Node/OutOfProcServerNode.cs Removes server-process “long-lived host” marking used to drive the workaround.
src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs Deletes the RestoreTask allow-list helper (RequiresTransientTaskHost).
src/Build/BackEnd/BuildManager/BuildParameters.cs Removes the long-lived-host flag/property and its translation.
src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs Removes tests and mock task validating transient TaskHost behavior for RestoreTask.

Comment thread src/Build/BackEnd/BuildManager/BuildParameters.cs
@JanProvaznik

Copy link
Copy Markdown
Member Author

autoreviewer locally passed, setting auto-merge

@JanProvaznik

Copy link
Copy Markdown
Member Author

coordinator test hang 👎

@JanProvaznik
JanProvaznik merged commit ebe081a into dotnet:main Jul 9, 2026
14 checks passed
This was referenced Sep 11, 2026
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.

3 participants