Fix TryGetResourceToolMap cache miss causing perpetual tools/list refresh loop#14539
Merged
mitchdenny merged 1 commit intorelease/13.2from Feb 19, 2026
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14539Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14539" |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an MCP server performance bug where TryGetResourceToolMap never hit the cache due to comparing against an explicit-only selection value, triggering repeated full refreshes during tools/list.
Changes:
- Update
TryGetResourceToolMapto compare the cached AppHost path against the effective selected connection’s AppHost path. - Add a unit test ensuring consecutive
ListToolsAsynccalls reuse the cached resource tool map (no redundant snapshot fetch). - Extend the test backchannel to allow intercepting
GetResourceSnapshotsAsynccalls for call-count assertions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs | Fixes cache validation to use the effective selected connection’s AppHost path. |
| tests/Aspire.Cli.Tests/TestServices/TestAppHostAuxiliaryBackchannel.cs | Adds a handler hook to count/override GetResourceSnapshotsAsync calls in tests. |
| tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs | Adds a regression test verifying cache hits across repeated tools/list. |
Contributor
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22125121406 |
JamesNK
approved these changes
Feb 18, 2026
JamesNK
reviewed
Feb 18, 2026
| lock (_lock) | ||
| { | ||
| if (_invalidated || _selectedAppHostPath != _auxiliaryBackchannelMonitor.SelectedAppHostPath) | ||
| if (_invalidated || _selectedAppHostPath != _auxiliaryBackchannelMonitor.SelectedConnection?.AppHostInfo?.AppHostPath) |
Member
There was a problem hiding this comment.
I think the API here is confusing. SelectedConnection to an app host and SelectedAppHostPath have different meanings but similar names
Member
There was a problem hiding this comment.
Now there is also ResolvedAppHostPath which gets value from SelectedConnection. But SelectedAppHostPath doesn’t…
Either SelectedConnection or SelectedAppHostPath need a new name. And ResolvedAppHostPath should match name with SelectedConnection
Member
There was a problem hiding this comment.
SelectedAppHostPath Is what is specified via MCP. Why not call it McpSpecifiedAppHostPath?
…resh loop TryGetResourceToolMap always returned false because it compared _selectedAppHostPath against _auxiliaryBackchannelMonitor.SelectedAppHostPath, which is only set by explicit select_apphost calls (usually null). After RefreshResourceToolMapAsync sets _selectedAppHostPath to the connection's actual path, the comparison null != "/path/to/AppHost" always failed, so every tools/list call triggered a full refresh instead of using the cached map. Fix: Add ResolvedAppHostPath property to IAuxiliaryBackchannelMonitor that returns SelectedConnection?.AppHostInfo?.AppHostPath, and compare against that. Rename field to _lastRefreshedAppHostPath for clarity. Fixes #14538 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
fix/mcp-tool-refresh-cache-miss
branch
from
d8249d2 to
25b99a6
Compare
radical
pushed a commit
to radical/aspire
that referenced
this pull request
Feb 19, 2026
…resh loop (dotnet#14539) TryGetResourceToolMap always returned false because it compared _selectedAppHostPath against _auxiliaryBackchannelMonitor.SelectedAppHostPath, which is only set by explicit select_apphost calls (usually null). After RefreshResourceToolMapAsync sets _selectedAppHostPath to the connection's actual path, the comparison null != "/path/to/AppHost" always failed, so every tools/list call triggered a full refresh instead of using the cached map. Fix: Add ResolvedAppHostPath property to IAuxiliaryBackchannelMonitor that returns SelectedConnection?.AppHostInfo?.AppHostPath, and compare against that. Rename field to _lastRefreshedAppHostPath for clarity. Fixes dotnet#14538 Co-authored-by: Mitch Denny <mitch@mitchdeny.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #14538
TryGetResourceToolMapalways returned false because it compared_selectedAppHostPathagainst_auxiliaryBackchannelMonitor.SelectedAppHostPath, which is only set by explicitselect_apphosttool calls (usuallynull). AfterRefreshResourceToolMapAsyncsets_selectedAppHostPathto the actual AppHost path, the comparisonnull != "/path/to/AppHost"always failed — so everytools/listcall triggered a full refresh instead of using the cached map.This caused ~3,800 refresh cycles per 3.5 seconds and generated ~130GB of log data in a 4-hour session.
Root Cause
Fix
Uses
SelectedConnectionwhich applies the same selection logic thatRefreshResourceToolMapAsyncuses viaGetSelectedConnectionAsync, ensuring the comparison matches.Testing
McpServer_ListTools_CachesResourceToolMap_WhenConnectionUnchangedtest verifying thatGetResourceSnapshotsAsyncis only called once across two consecutiveListToolsAsynccalls (proving cache hit on second call)Related
tools/list_changedfrom the list handler). This PR fixes the remaining cache-miss bug.