Skip to content

Make RestoreTask resettable across builds (NuGet/Home#14958 item 1) - #7507

Merged
zivkan merged 19 commits into
NuGet:devfrom
JanProvaznik:dev-JanProvaznik-restore-state-restoretask-only
Jul 9, 2026
Merged

zivkan merged 19 commits into
NuGet:devfrom
JanProvaznik:dev-JanProvaznik-restore-state-restoretask-only

Conversation

@JanProvaznik

@JanProvaznik JanProvaznik commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Bug

Progress: NuGet/Home#14958 (Plan item 1 — Make RestoreTask resettable)

Description

MSBuild is moving to an execution model where the entry-point process persists across builds (MSBuild Server, and later the multithreaded server). RestoreTask has always relied on the process dying after each build to drop its cached environment-variable-derived state and to reclaim the credential/plugin child processes. In a reused process that state would leak between restores — stale env-derived caches, a credential service pinned to the first build's credentials, and lingering plugin processes.

This PR implements item 1 of NuGet/Home#14958: make RestoreTask explicitly reset that state — refresh env-derived caches at the start of restore and tear down live resources (plugins) at the end — so a reused process behaves as if it had started fresh.

MSBuild server on second connection sets it's environment to match caller so we can use BCL APIs.

Scope: this is only item 1. It deliberately does not include item 4 (moving the env-cache reset to the first satellite task that hits cached state), which the issue calls out as lower-priority and a pre-existing concern in the current model. Static-graph restore is out of scope (it already spawns a fresh, short-lived process per the issue). No impact on VS or nuget.exe.

Approach — small public registry, internal self-registration

The only new public API is a tiny keyed registry in NuGet.Common:

public static class NuGetProcessState
{
    public enum ResetKey { StartRestore, EndRestore }
    public static void RegisterResetAction(ResetKey key, Action resetAction);
    public static void Reset(ResetKey key);   // runs all actions for the key, best-effort & isolated
}

Each cache/resource self-registers its own internal reset from its static constructor, so the individual resets stay internal/private and nothing else becomes public. RestoreTask.Execute drives the lifecycle: Reset(StartRestore) before restore, Reset(EndRestore) in a finally. RestoreTask runs once per restore on the entry node, so these are direct calls with no guard.

Reviewable commit-by-commit

  1. Add NuGetProcessState registry (NuGet.Common) — the infrastructure + unit tests + the single PublicAPI.Unshipped entry.
  2. Invoke it from RestoreTaskReset(StartRestore) at start, Reset(EndRestore) in finally. (No-op until caches register, below.)
  3. NuGet.Common env caches — ExceptionLogger (NUGET_SHOW_STACK), ConcurrencyUtilities (lock base path), NuGetEnvironment (home/temp/folder-path caches).
  4. NuGet.ConfigurationProxyCache (http_proxy family + nuget.config; drops cached proxy creds).
  5. NuGet.CredentialsPreviewFeatureSettings; DefaultCredentialServiceUtility (discards the pinned credential service so the next restore rebuilds it instead of reusing the first build's interactivity/providers/cached credentials).
  6. NuGet.ProjectModelDependencyGraphSpec (env-derived hash-function flag).
  7. NuGet.Protocol env caches — NuGetFeatureFlags, NuGetTestMode, PackageIdValidator, HttpSourceResourceProvider.Throttle (per-restore concurrency semaphore).
  8. NuGet.CommandsSourceRepositoryDependencyProvider throttle (NUGET_CONCURRENCY_LIMIT).
  9. End-of-restore teardownPluginManager disposes the shared instance (kills credential/plugin child processes and their timers) under EndRestore.

Completeness audit

Cross-checked against a whole-program, cross-assembly reachability analysis from the restore task entry points (13 assemblies, ~7,267 reachable methods). Every environment-variable-derived cached static reachable from RestoreTask is reset at StartRestore; the live plugin/credential processes are torn down at EndRestore. Items intentionally not reset, with rationale:

  • Self-refreshing each restore (so reset is unnecessary): UserAgent.UserAgentString and X509TrustStore are both re-initialized unconditionally in BuildTasksUtility.RestoreAsync every restore.
  • Not environment-derived / machine-invariant for the process lifetime: RuntimeEnvironmentHelper.* (OS / process-name flags), PathUtility._isFileSystemCaseInsensitive, framework-mapping tables, resource managers, comparers, JSON serializers, assembly-version singletons, NuGetExtractionFileIO._unixPermissions (umask).
  • Deterministic, env-independent computation caches (resetting would be pure perf loss): GraphOperations.Cache, RuntimeGraph.Cache.
  • Experimental knob with a deliberate caching contract: X509ChainBuildPolicyFactory.Policy (NUGET_EXPERIMENTAL_CHAIN_BUILD_RETRY_POLICY).
How the audit was performed (cross-assembly reachability analyzer)

The "every reachable static" claim above is not eyeballed — it comes from a purpose-built Roslyn analyzer: RestoreStateAnalyzer (README · full report.md).

A normal DiagnosticAnalyzer only sees one compilation and only metadata for referenced assemblies, so it cannot follow a call from RestoreTask (in NuGet.Build.Tasks) into the body of NuGet.Protocol, NuGet.Credentials, etc. To be accurate across assemblies, the tool instead:

  1. Builds NuGet.Build.Tasks for net10.0 (the CoreCLR dotnet restore runtime) with /bl, and reads the exact csc command lines from the binlog — so source files, references, #defines (IS_CORECLR, …), nullable and langversion match the shipping build exactly.
  2. Reconstructs one CSharpCompilation per NuGet project, wiring inter-project references as Roslyn CompilationReferences (symbols resolve into the source of every assembly) and running the same source generators. Result: 13 assemblies, 0 binding errors.
  3. Seeds the restore task entry points and performs an interprocedural worklist walk over IOperation trees — following invocations / object creations / property & event accesses / static initializers, expanding virtual & interface dispatch to all source overrides transitively.
  4. Records every reachable static field/property (classified: mutable / readonly-mutable-ref / Lazy / [ThreadStatic] / …) and every lingering process-state sink (child processes, env edits/reads, timers, static event subscriptions, cwd/console/culture/registry/AppDomain mutations), each annotated with the set of restore tasks that reach it and a representative call path.

The audit table above is the result of diffing that enumeration (filtered to RestoreTask-reachable, environment-derived statics) against the set of RegisterResetAction call sites in this PR. The same report drove the e2e validation target list below.

End-to-end validation

The combined behavior (this NuGet change + the matching MSBuild change that runs RestoreTask in the persistent process) was validated end-to-end on a coordinated VMR build using the two-PAT credential-leak scenario from dotnet/msbuild#13660: restore #1 with PAT-A against a private Azure Artifacts feed, revoke PAT-A, restore #2 with PAT-B in the same MSBuild Server process. On main restore #2 fails with NU1301: 401 (the revoked PAT leaks from the cached plugin); with these resets restore #2 succeeds with PAT-B. RestoreTask was confirmed (via binlog) to run in-process in the persistent server, so the leak-prone path is genuinely exercised.

PR Checklist

  • Meaningful title and description — implements item 1 of dotnet restore/build should work correctly when its MSBuild tasks run repeatedly and in the same process Home#14958; reviewable commit-by-commit.
  • TestsNuGetProcessStateTests (registry runs-all / failure-isolation, and re-read after Reset(StartRestore)); ProxyCacheTests.ResetCache, HttpSourceResourceProviderTests.ResetThrottle, DefaultCredentialServiceUtilityTests.ResetCredentialService. Full NuGet.Build.Tasks.Test suite green; build clean on net472 + net10.0.
  • No behavior change for the existing (process-dies-per-build) model — the resets re-read the same environment a fresh process would, and teardown matches the prior process-exit reclamation.
  • PublicAPI.Unshipped.txt updated — only NuGet.Common.NuGetProcessState (the registry); every per-type reset is internal/private.

JanProvaznik and others added 9 commits June 24, 2026 13:54
A small public keyed registry: RegisterResetAction(ResetKey, Action) accumulates resets under a key and Reset(ResetKey) runs them (best-effort, isolated). Keys: StartRestore (refresh env-derived caches) and EndRestore (tear down live resources). This is the only new public API; each cache/resource registers its own internal reset, so nothing else becomes public.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RestoreTask.Execute calls Reset(StartRestore) before restore and Reset(EndRestore) in a finally. RestoreTask runs once per restore on the entry node, so these are direct calls with no guard. Makes a process reused across builds (MSBuild Server) behave as if started fresh. No-op until caches register (following commits).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ExceptionLogger (NUGET_SHOW_STACK), ConcurrencyUtilities (lock base path) and NuGetEnvironment (home/temp/folder path caches) self-register a reset from their static constructors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ProxyCache.Instance is rebuilt so a reused process re-reads the http_proxy family of env vars / nuget.config and drops cached proxy credentials.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PreviewFeatureSettings re-reads its env flag; DefaultCredentialServiceUtility discards the pinned credential service so a reused process rebuilds it for the next restore instead of reusing the first build's interactivity/providers/cached credentials.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re-reads its env-derived hash-function flag.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NuGetFeatureFlags, NuGetTestMode and PackageIdValidator re-read their env flags; HttpSourceResourceProvider.Throttle (the per-restore concurrency semaphore) is cleared so it does not leak into the next restore.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ore)

Recreates the NUGET_CONCURRENCY_LIMIT-sized semaphore from the current environment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PluginManager registers a private ResetSharedInstance under EndRestore that disposes the shared instance, killing credential/plugin child processes and their timers that the per-build process exit used to reclaim.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik requested a review from a team as a code owner June 24, 2026 13:20
@JanProvaznik
JanProvaznik requested review from nkolev92 and zivkan June 24, 2026 13:20
@dotnet-policy-service dotnet-policy-service Bot added the Community PRs created by someone not in the NuGet team label Jun 24, 2026
@JanProvaznik

Copy link
Copy Markdown
Contributor Author

@OvesN @AR-May — requesting your review on this one (item 1 of NuGet/Home#14958: make RestoreTask resettable across builds). Thanks!

Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Common.Test/NuGetProcessStateTests.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Common.Test/NuGetProcessStateTests.cs Outdated
…essState contract tests

- Move the NUGET_SHOW_STACK re-read test out of NuGetProcessStateTests into ExceptionLoggerTests, where the behavior lives (ExceptionLogger_ResetInstance_ReReadsShowStackFromEnvironment).
- Give NuGetProcessState direct contract + edge-case coverage: null action throws, a registered action runs, every action for a key runs, and actions for a different key do not run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik

Copy link
Copy Markdown
Contributor Author

@zivkan @nkolev92 ptal

…generic catch

Per review, NuGetProcessState.Reset no longer swallows exceptions - reset actions are
expected not to throw, so a genuine bug in a reset surfaces instead of being hidden.

Restore's only reset that interacts with an external resource is the end-of-restore
plugin teardown. The one unguarded throw on its disposal path is fixed at the source:
PluginProcess.CancelRead now guards Process.CancelOutputRead the same way Kill already
does. Once a plugin process exits, the async stdout read completes and CancelOutputRead
throws InvalidOperationException; guarding it lets Plugin/PluginFactory.Dispose run to
completion for every plugin (live ones still get killed) rather than aborting midway.

PluginManager.ResetSharedInstance installs the fresh instance before disposing the old
one, so a reused process always observes a clean PluginManager.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik force-pushed the dev-JanProvaznik-restore-state-restoretask-only branch from 29289e8 to 2aa39d9 Compare June 25, 2026 15:00
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs Outdated
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs Outdated
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs
Comment thread src/NuGet.Core/NuGet.Common/NuGetProcessState.cs Outdated
JanProvaznik and others added 2 commits June 29, 2026 14:48
…osing it

Veronika noted that disposing the shared PluginLogger.DefaultInstance during end-of-restore
plugin teardown leaves a disposed singleton that throws ObjectDisposedException on the next
restore when plugin logging is enabled.

- PluginFactory no longer disposes _logger: it does not own it (it is the shared, process-wide
  DefaultInstance), matching Connection/MessageDispatcher which already document the same.
- PluginLogger.DefaultInstance is now resettable and self-registers a StartRestore reset that
  rebuilds it from the current environment and disposes the previous instance to close its log
  file. A process reused across builds therefore (a) no longer throws on the next restore, and
  (b) honors a toggled NUGET_PLUGIN_ENABLE_LOG / NUGET_PLUGIN_LOG_DIRECTORY_PATH per restore.
  Disposing the previous instance is safe because the prior restore's plugins (and their loggers)
  are torn down by the end-of-restore reset before the next restore's start reset runs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per review feedback (nkolev92): the keyed reset registry was hand-rolled
publish/subscribe over a ConcurrentDictionary<ResetKey, ConcurrentBag<Action>>,
duplicating what the language already provides. Replace it with two ordinary
static events on a renamed NuGet.Common.StaticState:

  - StartMSBuildRestoreTasks  (was Reset(ResetKey.StartRestore))
  - EndMSBuildRestoreTasks    (was Reset(ResetKey.EndRestore))

Contributors subscribe with += from their static constructors; RestoreTask
raises them via RaiseStartMSBuildRestoreTasks()/RaiseEndMSBuildRestoreTasks().
Event invocation preserves the previous "honest" behavior - a throwing handler
propagates rather than being swallowed, so each risky reset still self-guards.

Shrinks the public surface from a class + enum + two methods to a class with
two events + two raise methods, and the name now reflects that this is keyed to
MSBuild's restore-task lifecycle, not NuGet's own process.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik requested review from nkolev92 and zivkan June 30, 2026 16:32
Comment thread src/NuGet.Core/NuGet.Common/Logging/ExceptionLogger.cs

@nkolev92 nkolev92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments, but I think the try/catch + the event name are probably the 2 relevant things

Comment thread src/NuGet.Core/NuGet.Common/StaticState.cs
Comment thread src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs Outdated
Comment thread src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs Outdated
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/Logging/PluginLogger.cs Outdated
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/Logging/PluginLogger.cs
Comment thread src/NuGet.Core/NuGet.Protocol/Plugins/PluginManager.cs
Comment thread src/NuGet.Core/NuGet.Common/StaticState.cs
JanProvaznik and others added 5 commits July 1, 2026 11:43
Per review feedback ("No nullable disable in new files"): new files must be
nullable-enabled. Dropped the directive and annotated the captured
HttpHandlerResourceV3.CredentialService (a Lazy<ICredentialService>?) local as
nullable accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per review feedback (nkolev92): the fully-qualified NuGet.Common.StaticState is
unnecessary where NuGet.Common is imported. Shortened all references to
StaticState; added `using NuGet.Common;` to NuGetTestMode.cs, which lacked it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per review feedback (nkolev92): spell out that any type caching process-global state, or a value derived from it (e.g. an env-derived path from NuGetEnvironment.GetFolderPath), must subscribe a StartMSBuildRestoreTasks reset; EndMSBuildRestoreTasks is for tearing down live OS resources.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik requested a review from nkolev92 July 2, 2026 14:58

@zivkan zivkan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little concerned about the risk of these changes. Since NuGet 7.9 (which inserts into 10.0.400) branches in a few days, I'll merge this next week once NuGet becomes 7.10 (which insertions into 11.0.100), so that just in case this causes problems, we won't have to server 10.0.400.

@zivkan
zivkan enabled auto-merge (squash) July 9, 2026 05:16
@zivkan
zivkan merged commit 6728fd4 into NuGet:dev Jul 9, 2026
20 checks passed
JanProvaznik added a commit to JanProvaznik/NuGet.Client that referenced this pull request Jul 15, 2026
Follow-up to NuGet#7507 (NuGet/Home#14958 item 1). That PR drove the start- and
end-of-restore reset of NuGet's process-global state from RestoreTask, but as
discussed there RestoreTask runs last in a restore - after the Get* collection
tasks (e.g. GetRestoreSettingsTask, which reads NuGetEnvironment) have already
read environment-derived state - and it never runs at all for
dotnet package add, which stops at GenerateRestoreGraphFile /
WriteRestoreGraphTask. So the start-of-restore refresh ran too late and missed
entry points.

Add RefreshNuGetStaticStateTask, which raises StaticState.StartMSBuildRestoreTasks,
wired as the first dependency of _GenerateRestoreGraph. Both in-proc restore
entry targets (Restore and GenerateRestoreGraphFile) funnel through
_GenerateRestoreGraph, which runs exactly once per restore on the entry node, so
the refresh now runs once, ahead of every Get* task and
RestoreTask / WriteRestoreGraphTask. Remove the now-earlier raise from
RestoreTask.Execute; the end-of-restore teardown stays in its finally.

Mark RestoreTask [MSBuildMultiThreadableTask] so that under multithreaded MSBuild
it runs in the same process as RefreshNuGetStaticStateTask and observes the
refreshed state. If a reset handler unexpectedly throws, the task logs a
localized message and continues rather than failing the build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JanProvaznik added a commit to JanProvaznik/NuGet.Client that referenced this pull request Aug 14, 2026
SourceRepositoryDependencyProvider._throttle is a process-wide, bounded
SemaphoreSlim that NuGet#7507 made swappable and disposed at the start of every
restore. Every call site reads the static field twice - once to wait, once to
release in a finally - so a reset landing between the two reads releases a
different, brand-new, already-full semaphore:

  error : Adding the specified count to the semaphore would cause it to exceed
          its maximum count.

Disposing the previous instance additionally faults anyone still waiting on it.
With default settings the throttle is only non-null on macOS (16, to stay under
the 256 open-file limit) or when NUGET_CONCURRENCY_LIMIT is set, so this is
macOS-only in practice.

Fixing the double read is not sufficient. A concurrency gate is only meaningful
if it is the same instance for the lifetime of the operations it gates:
requests already holding a permit on the old semaphore run alongside requests
acquiring on the new one, so the limit is not actually enforced across a swap.
Restore the readonly field and drop ResetCache.

The consequence is that NUGET_CONCURRENCY_LIMIT is fixed for the life of the
process, which is what it was before NuGet#7507 - process exit refreshed it. Making
it genuinely per-build means scoping the throttle to the restore rather than
sharing one static, which is a larger change and worth doing separately.

The regression test drives a request that holds the throttle, raises
StartMSBuildRestoreTasks while it is in flight, then lets it complete. Against
the previous code with NUGET_CONCURRENCY_LIMIT set it fails with
SemaphoreFullException out of SemaphoreSlim.Release, matching the reported stack.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 197f186a-e367-42cc-9d89-7c301bfa0843
JanProvaznik added a commit to JanProvaznik/NuGet.Client that referenced this pull request Aug 14, 2026
The resets added in NuGet#7507 and rewired in NuGet#7551 were scoped to a restore. Every
requirement they serve is scoped to a build:

- Environment variables do not change during a build. MSBuild Server applies the
  client's environment snapshot at the start of each build, so per-restore firing
  refreshes nothing that per-build firing would not.
- The stale-credential repro that motivated this work is two separate MSBuild
  invocations against one server process, which needs the plugin discarded once
  per build.

Per-restore scope bought nothing and cost NuGet/Home#15044, #15045 and #15046.

Collapse StartMSBuildRestoreTasks and EndMSBuildRestoreTasks into a single
BuildEnded event, raised once per build per node from RestoreTask's registered
build-lifetime object. Delete RefreshNuGetStaticStateTask, the
_RefreshNuGetStaticState target and its UsingTask, which existed only to raise
the start event.

Two rules now bind every handler, both documented on the event:

- Invalidate; do not recompute. At the end of build N the process still holds
  build N's environment, so a handler that reads it here caches the value that is
  on its way out. NuGetTestMode, PreviewFeatureSettings and ExceptionLogger
  recomputed eagerly and are converted to compute on first use; the other
  subscribers already invalidated lazily.
- Do not swap a resource that has work in flight. SourceRepositoryDependencyProvider's
  throttle was removed from the scheme in the previous commit for this reason;
  PluginLogger moved to this event because its lifetime is the plugins' lifetime.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 197f186a-e367-42cc-9d89-7c301bfa0843
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PRs created by someone not in the NuGet team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants