Conversation
conniey
reviewed
Oct 29, 2025
conniey
reviewed
Oct 29, 2025
conniey
reviewed
Oct 29, 2025
conniey
previously approved these changes
Oct 29, 2025
anuchandy
previously approved these changes
Oct 29, 2025
anuchandy
reviewed
Oct 29, 2025
JasonYeMSFT
reviewed
Oct 29, 2025
JasonYeMSFT
reviewed
Oct 29, 2025
srnagar
previously approved these changes
Oct 31, 2025
The base branch was changed.
…vs multi-user web API. Update various Azure Data tools to avoid OAuth access token caching directly and instead rely on IAzureTokenCredentialProvider infra to do so. Remove unsafe instance-level caching in BaseAzureService and Monitor services Removed stateful fields that cached user-specific credentials and tokens in: - BaseAzureService: Removed ArmClient caching and GetCachedTokenAsync helper - MonitorHealthModelService: Removed cached dataplane/control plane tokens - MonitorService: Removed cached management token These instance-level caches were not safe for multi-user scenarios where different users access the same service instance with different identities. Now relies on Azure SDK's built-in thread-safe caching in TokenCredential.
53be163 to
a00e815
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors token caching logic across multiple Azure service implementations by removing manual token caching and relying on the underlying Azure SDK's built-in token caching mechanism. The changes also introduce distinct cache service implementations for single-user CLI and multi-user web API scenarios.
- Removes manual token caching (fields and logic) from PostgresService, MySqlService, MonitorService, MonitorHealthModelService, and MarketplaceService
- Standardizes token acquisition to always use
GetCredential()with properCancellationTokenpropagation - Introduces
SingleUserCliCacheServiceandHttpServiceCacheServiceas distinct implementations ofICacheServicewith appropriate DI registration methods - Updates parameter naming from
tenanttotenantIdortenantIdOrNamefor clarity in several methods
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| PostgresService.cs | Removed cached token fields and logic, simplified GetEntraIdAccessTokenAsync to use SDK token caching |
| MySqlService.cs | Removed cached token fields, lock object, and caching logic from GetEntraIdAccessTokenAsync |
| MonitorService.cs | Removed GetCachedManagementTokenAsync method and associated caching fields, updated CallActivityLogApiAsync to use direct token acquisition |
| MonitorHealthModelService.cs | Removed token caching fields and GetCachedTokenAsync calls, simplified control plane and dataplane token methods |
| MonitorCommandTests.cs | Updated to use SingleUserCliCacheService instead of CacheService |
| MarketplaceService.cs | Removed GetAccessTokenAsync caching method, renamed method to GetArmAccessTokenAsync, updated documentation from "tenant" to "tenantId" |
| ProductListCommandTests.cs | Updated to use SingleUserCliCacheService |
| ProductGetCommandTests.cs | Updated to use SingleUserCliCacheService |
| DatadogService.cs | Updated parameter name from tenant to tenantIdOrName in CreateArmClientAsync call |
| AppConfigCommandTests.cs | Updated to use SingleUserCliCacheService |
| Program.cs (Template/Fabric/Azure servers) | Updated DI registration to use new AddSingleUserCliCacheService() extension method |
| CacheServiceTests.cs | Updated to use SingleUserCliCacheService |
| SingleUserCliCacheService.cs | Renamed from CacheService, added comprehensive documentation |
| HttpServiceCacheService.cs | New stub implementation for multi-user web API scenarios |
| CachingServiceCollectionExtensions.cs | New file with extension methods for registering cache services |
| BaseAzureService.cs | Removed ArmClient caching, removed GetCachedTokenAsync method, renamed parameter tenant to tenantIdOrName in CreateArmClientAsync |
| TransportTypes.cs | Added Http constant for Streamable HTTP transport |
| ServiceStartCommand.cs | Added AddHttpServiceCacheService() calls for HTTP hosts |
| launchSettings.json | Removed trailing whitespace |
vukelich
commented
Nov 3, 2025
anuchandy
approved these changes
Nov 3, 2025
srnagar
reviewed
Nov 3, 2025
srnagar
approved these changes
Nov 3, 2025
Member
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
Bypassing merge rule for microsoft/fabric-mcp owner review required. We're making lots of big changes in |
colbytimm
pushed a commit
to colbytimm/microsoft-mcp
that referenced
this pull request
Dec 8, 2025
…vs multi-user web API. (microsoft#996) * Introduce separate ICacheService implementations for single user CLI vs multi-user web API. Update various Azure Data tools to avoid OAuth access token caching directly and instead rely on IAzureTokenCredentialProvider infra to do so. The cache implementation selected at runtime depends on the `--transport` option, which itself is re-introducing `http` as a choice for remote MCP servers. Remove unsafe instance-level caching in BaseAzureService and Monitor services For example, removed stateful fields that cached user-specific credentials and tokens in: - BaseAzureService: Removed ArmClient caching and GetCachedTokenAsync helper - MonitorHealthModelService: Removed cached dataplane/control plane tokens - MonitorService: Removed cached management token These instance-level caches were not safe for multi-user scenarios where different users access the same service instance with different identities. Now relies on Azure SDK's built-in thread-safe caching in TokenCredential.
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.
What does this PR do?
[Provide a clear, concise description of the changes]Update various Azure Data tools to avoid OAuth access token caching directly and instead rely on
IAzureTokenCredentialProviderinfra to do so.Remove unsafe instance-level caching in
BaseAzureServiceand Monitor servicesRemoved stateful fields that cached user-specific credentials and tokens in:
BaseAzureService: RemovedArmClientcaching andGetCachedTokenAsynchelperMonitorHealthModelService: Removed cached dataplane/control plane tokensMonitorService: Removed cached management tokenThese instance-level caches were not safe for multi-user scenarios where different users access the same service instance with different identities. Now relies on Azure SDK's built-in thread-safe caching in
TokenCredential.GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test prompts/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline