Conversation
BundleNuGetPackageCache was introduced in PR #14105 without integrating the IDiskCache that was added in PR #11394. This meant every invocation of 'aspire add' (or any command that searches NuGet packages in bundle mode) spawned a new aspire-managed process, bypassing the disk cache entirely. Wire up IDiskCache in BundleNuGetPackageCache.SearchPackagesInternalAsync so that results are cached to disk (3-hour TTL) and subsequent runs return near-instantly from cache, matching the non-bundle behavior. CLI package lookups (for update notifications) bypass the cache to ensure fresh results, consistent with the non-bundle path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16318Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16318" |
There was a problem hiding this comment.
Pull request overview
This PR integrates the existing CLI IDiskCache mechanism into bundle-mode NuGet package search so repeated aspire add (and other NuGet search flows in bundle mode) can return results from disk cache instead of spawning aspire-managed on every invocation, aligning bundle-mode performance with the SDK-based path.
Changes:
- Inject
IDiskCacheintoBundleNuGetPackageCacheand plumb auseCacheparameter through internal search calls. - Add disk-cache read-through + write-back behavior for bundle-mode search results (JSON payload caching).
- Explicitly bypass caching for CLI package lookups (
useCache: false) to keep update checks fresh.
Include workingDirectory.FullName in the disk cache key to prevent cross-project cache pollution when no explicit nuget.config is provided. Add BundleNuGetPackageCacheTests covering: - Cache hit on second call (process not re-invoked) - useCache:false bypasses disk cache (CLI packages) - Different working directories get separate cache entries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The NuGetHelper search command was calling GetVersionsAsync() and GetDeprecationMetadataAsync() for every package in the search results. For a search returning 1000 results, this made 2000 additional API calls and produced a ~3MB JSON payload with all package versions. The CLI never uses AllVersions (it only needs the latest version) and uses a hardcoded DeprecatedPackages set instead of NuGet deprecation metadata. Removing these calls eliminates the N+1 query problem and reduces the response payload from ~3MB to ~100KB. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 72 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24646866397 |
| private static async Task<string> ComputeFileHashAsync(FileInfo file, CancellationToken cancellationToken) | ||
| { | ||
| using var stream = file.OpenRead(); | ||
| var hashBytes = await SHA256.HashDataAsync(stream, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
I've seen other hash algo being used in places where it's not used for security. I don't remember the exact name. Should it be used here instead of SHA256?
| private sealed class InMemoryDiskCache : IDiskCache | ||
| { | ||
| private readonly Dictionary<string, string> _entries = new(); | ||
|
|
||
| public Task<string?> GetAsync(string key, CancellationToken cancellationToken = default) | ||
| { | ||
| _entries.TryGetValue(key, out var value); | ||
| return Task.FromResult(value); | ||
| } | ||
|
|
||
| public Task SetAsync(string key, string content, CancellationToken cancellationToken = default) | ||
| { | ||
| _entries[key] = content; | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| public Task ClearAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| _entries.Clear(); | ||
| return Task.CompletedTask; | ||
| } | ||
| } | ||
|
|
||
| private sealed class FakeBundleService : IBundleService |
There was a problem hiding this comment.
Use shared test services. Avoid many private implementations
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Review: 4 items flagged — 1 bug (cache write failure discards results), 1 convention violation (SHA-256 for non-security hash), 2 minor test issues (temp directory leak, duplicate test helper).
| return []; | ||
| } | ||
|
|
||
| // Persist the raw JSON to disk cache for future lookups | ||
| if (cacheEnabled && cacheKey is not null) | ||
| { |
There was a problem hiding this comment.
Bug: If _diskCache.SetAsync throws (e.g. IOException, UnauthorizedAccessException), the exception propagates and the successfully-fetched search results are never returned to the caller.
The cache read path correctly handles these exceptions (lines 159–163 catch and set cacheEnabled = false), but the write path does not. Consider wrapping this in a try/catch similar to the read path so that a cache-write failure doesn't discard valid results:
try
{
await _diskCache.SetAsync(cacheKey, output, cancellationToken).ConfigureAwait(false);
}
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or System.Security.SecurityException)
{
_logger.LogDebug(ex, "Failed to write package search results to disk cache.");
}| private sealed class TestFeatures : IFeatures | ||
| { | ||
| public bool IsFeatureEnabled(string featureName, bool defaultValue = false) => defaultValue; | ||
|
|
||
| public void LogFeatureState() { } | ||
| } |
There was a problem hiding this comment.
Nit: There's already a shared TestFeatures class in tests/Aspire.Cli.Tests/TestServices/TestFeatures.cs with the same behavior (plus the ability to set feature flags). Consider reusing it instead of duplicating here.
| _tempDir = Directory.CreateTempSubdirectory("aspire-test-bundle").FullName; | ||
| var managedDir = Path.Combine(_tempDir, "managed"); | ||
| Directory.CreateDirectory(managedDir); | ||
| var exeName = OperatingSystem.IsWindows() ? "aspire-managed.exe" : "aspire-managed"; |
There was a problem hiding this comment.
Minor: FakeBundleService creates a temp directory via Directory.CreateTempSubdirectory (system temp, not inside the TemporaryWorkspace) but never cleans it up — each test run leaks a directory. If this is intentional (OS cleans temp eventually), feel free to ignore; otherwise consider making FakeBundleService implement IDisposable and deleting _tempDir on dispose.
| } | ||
| } | ||
|
|
||
| private static async Task<string> ComputeFileHashAsync(FileInfo file, CancellationToken cancellationToken) | ||
| { | ||
| using var stream = file.OpenRead(); |
There was a problem hiding this comment.
Convention: Per AGENTS.md, cryptographic hashes like SHA-256 should not be used when the hash is not security-related. This hash is solely for cache key construction/invalidation. Prefer System.IO.Hashing.XxHash3 instead:
private static async Task<string> ComputeFileHashAsync(FileInfo file, CancellationToken cancellationToken)
{
using var stream = file.OpenRead();
var hashBytes = await System.IO.Hashing.XxHash3.HashAsync(stream, cancellationToken).ConfigureAwait(false);
return Convert.ToHexString(hashBytes);
}(The existing NuGetPackageCache.ComputeNuGetConfigHashSuffixAsync has the same pre-existing issue, but this is new code.)
Description
BundleNuGetPackageCache(used when the CLI runs as a self-extracting bundle — the default installed mode) was introduced in PR #14105 without integrating theIDiskCachethat was added in PR #11394. This meant every invocation ofaspire add(or any command that searches NuGet packages in bundle mode) spawned a newaspire-managedprocess, bypassing the disk cache entirely. Subsequent runs were just as slow as the first.This PR wires up
IDiskCacheinBundleNuGetPackageCache.SearchPackagesInternalAsyncso that results are cached to disk (3-hour TTL) and subsequent runs return near-instantly from cache, matching the non-bundle (NuGetPackageCache+DotNetCliRunner) behavior.CLI package lookups (for update notifications) bypass the cache to ensure fresh results, consistent with the non-bundle path.
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: