Stop leaking orphaned aspire-managed NuGet search helpers - #18958
Stop leaking orphaned aspire-managed NuGet search helpers#18958James Newton-King (JamesNK) merged 6 commits into
Conversation
NuGetPackagePrefetcher fired both prefetches as discarded Task.Run work, so ExecuteAsync returned immediately and BackgroundService.StopAsync had nothing to await. The CLI process exited while a `aspire-managed nuget search` child was still running, leaving it orphaned and holding NuGet lock files. Track both prefetch tasks and await them, so shutdown cancels the in-flight search and the child is torn down instead of orphaned. Also opt `ls` and `ps` out of package metadata prefetching: neither uses template metadata, and neither displays update notifications, so both searches were pure waste. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18958Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18958" |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Prevents NuGet prefetch helpers from outliving Aspire CLI invocations.
Changes:
- Tracks and awaits package prefetch tasks during shutdown.
- Disables metadata prefetching for
lsandps. - Adds lifecycle and command-specific regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Aspire.Cli/NuGet/NuGetPackagePrefetcher.cs |
Tracks prefetch task completion. |
src/Aspire.Cli/Commands/LsCommand.cs |
Disables metadata prefetching. |
src/Aspire.Cli/Commands/PsCommand.cs |
Disables metadata prefetching. |
tests/Aspire.Cli.Tests/NuGet/NuGetPackagePrefetcherTests.cs |
Adds regression coverage. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Cli/NuGet/NuGetPackagePrefetcher.cs:91
- Awaiting these tasks does not guarantee that the CLI waits for cleanup. Release builds give the entire host shutdown only 200 ms (
Program.cs:317-323), andBackgroundService.StopAsyncstops waiting forExecuteTaskwhen that token expires. If that budget is exhausted before this service runs, or before its cancellation continuation kills the helper,Program.cs:1249can still return with the prefetch running—the orphaning path this method claims to prevent. The new test usesCancellationToken.None, so it cannot catch this. Please make helper termination an owned shutdown step that cannot be abandoned by the host timeout, while bounding any post-kill drain separately.
private static async Task PreventOrphanedPrefetchingAsync(List<Task> prefetchTasks, CancellationToken stoppingToken)
{
try
{
await Task.WhenAll(prefetchTasks);
src/Aspire.Cli/Commands/LsCommand.cs:23
aspire lsdoes not list resources from a running AppHost; it discovers candidate AppHost project files in the workspace (asSharedCommandStrings.LsCommandDescriptionandExecuteAsyncdescribe). Correct this rationale so it does not imply thatlsdepends on a running application.
/// <summary>
/// LsCommand lists resources of a running AppHost and never uses template package metadata.
/// </summary>
WaitForCommandSelectionAsync gave up after one second and fell back to the null default, which enables both prefetches. The first-run banner spends 1660ms in fixed delays between host start and the command's action running, so `aspire ls` and `aspire ps` still spawned NuGet searches on a first run or with --banner. Wait for selection until shutdown instead, and skip prefetching entirely when the CLI stops before a command is selected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both suppressed comments were worth answering.
If you want the hard guarantee, I will add it as an explicit shutdown step with its own budget. Your call on the tradeoff. |
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Updated the delayed command-selection regression test to use FakeTimeProvider, so it no longer requires a real Task.Delay.
Further improvements to make NuGet metadata prefetching opt-in per command are tracked in #18965.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/Aspire.Cli.Tests/NuGet/NuGetPackagePrefetcherTests.cs:307
- This does not reproduce the timeout being regressed. The removed implementation used a real
CancellationTokenSource(TimeSpan.FromSeconds(1)), so advancing this fake clock consumes no wall time; against that implementation the command is selected immediately and the test still passes. Wait longer than the old real timeout and assertExecuteTaskis still pending before selecting the command, so the test also proves the service did not silently stop waiting.
timeProvider.Advance(TimeSpan.FromMilliseconds(1500));
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
7921e86
into
microsoft:main
|
🔍 CI Failure Analysis: Transient Infrastructure Failure The CI build failed due to transient infrastructure issues. Failed jobs:
If a rerun was not already requested automatically, visit the workflow run page to rerun the failed jobs manually. |
Description
Fixes #18948. Same root cause as #18779.
aspire lsandaspire psspawn anaspire-managed nuget searchhelper that never exits. They accumulate — 19 over six days in the original report, 47 overnight on my machine — each holding lock files under$TMPDIR/NuGetScratch/lock, until an unrelateddotnet restoredeadlocks at "Determining projects to restore...". Repro steps are in #18948.Three things combine.
The prefetch is never awaited.
NuGetPackagePrefetcher.ExecuteAsyncfired both prefetches as discardedTask.Run(L26, L55), so it returned immediately,BackgroundService.StopAsyncawaited an already-completedExecuteTask, and the CLI exited with the search still running. The log in #18948 shows it: the helper is launched and, unlike every other subprocess in that log, never gets an exit entry. Fixed by collecting both tasks and awaiting them.The teardown path was already correct — nothing ever ran it. The token flows unbroken to
LayoutProcessRunner.cs#L58, where cancellingWaitForExitAsyncunwinds into theawait usingandProcessExecution.cs#L533kills the tree. Cancellation already reaps the helper. Nothing was cancelling it.Read-only commands opted in by default.
lsandpsdon't implementIPackageMetaPrefetchingCommand, so they hit the default at L108 — prefetch for everything except run/publish/deploy/do. Neither uses template metadata, and neither overridesUpdateNotificationsEnabled(defaultfalse), so neither reaches the notification site atBaseCommand.cs:202and the CLI-package search is wasted too. Both now implement the interface with both flagsfalse, matchingNewCommand,McpInitCommandandAgentInitCommand.One second wasn't long enough to learn the command.
WaitForCommandSelectionAsyncgave up after 1000 ms and fell back to the null default, which enables both prefetches. Selection happens when the command's action runs (BaseCommand.cs:61), and the first-run banner plays before that, spending 1660 ms in fixed delays — so the opt-out above was bypassable on a first run or with--banner. It now waits until shutdown. Caught by Copilot review; arithmetic here.Why the existing protection misses all this: on non-Windows the cooperative parent-liveness watchdog is the sole mechanism (
LayoutProcessRunner.cs#L38-L49), and the leaked helpers sit in stateT. A stopped process is never scheduled, so it cannot notice its parent died, andSIGTERMis not delivered to it — onlySIGKILLclears them. #18566 tightened several leak paths, but not this one.What this does not guarantee
Release builds cap host shutdown at 200 ms (
Program.cs#L317-L323). Cancel → kill → drain normally fits inside that, but if it overruns, the helper is still abandoned. Strictly better than today, where nothing cancels it at all — not a hard guarantee. I can add an explicit shutdown step with its own budget if you want one.One behaviour change to rule on
Waiting for selection instead of guessing means invocations where no command action ever runs —
--help,--version, a parse error — no longer prefetch CLI update metadata. Nothing displays an update notification on those paths, so I read it as dead work removed rather than a regression. It does change what those invocations do to the update cache, which is your call rather than mine. Say so and I will restore a bounded fallback.Tests
Four tests in
NuGetPackagePrefetcherTests, each written againstmainfirst:InFlightPrefetchingCompletesBeforeTheServiceStops—ExecuteTaskis still running while a prefetch is in flight, and both callbacks have unwound by the timeStopAsyncreturns. The regression test for the leak; fails deterministically onmain.ReadOnlyCommandsDisablePackageMetadataPrefetching(ls,ps) — resolves the real commands from the real DI graph. Fails deterministically onmain.ReadOnlyCommandsStartNoPrefetching(ls,ps) — drives the real prefetcher with those instances. Only deterministic once the prefetch tasks are awaited, since that is what makesExecuteTaska barrier; onmainit fails forpsbut can pass forlson scheduling luck. Its value is as a guard after the fix.CommandSelectedAfterABannerLengthDelayStillDisablesPrefetching— waits 1500 ms before selecting the command. The delay is the point: it has to outlast the timeout that used to be there.macOS arm64, .NET SDK 10.0.302. Prefetcher plus
LsCommandTestsandPsCommandTests: 69/69, no warnings. FullAspire.Cli.Tests(--filter-not-trait quarantined=true --filter-not-trait outerloop=true): 4695 tests, 4661 pass, 32 skipped, 2 failures —CliPathHelperTests.ResolveSymlinkToFullPath_NonLink_ReturnsNormalizedFullPathandInstallationDiscoveryDiscoverAllTests.DiscoverAllAsync_RunningCliIsAlwaysFirst, both of which fail identically on an unmodified checkout here (macOS/private/tmpsymlink normalization, and a running-CLI assumption).I did not touch stdin or the TTY. #16791 / #17562 is the complementary fix for why an orphaned helper ends up stopped rather than exiting; this stops it being orphaned.
Checklist