Propagate FromCache on recursive adapter-id resolution#4768
Conversation
Fixes icerpc/icerpc-csharp-audit#39. When a well-known lookup returns a service address with an adapter-id, the adapter-id is resolved recursively, and the inner resolution's FromCache flag was discarded: a fresh well-known lookup followed by an adapter-id resolution served from the cache returned FromCache=false even though the server addresses came from the cache. LocatorInterceptor then didn't set ICachedResolutionFeature, so a retry didn't request a cache refresh and the refresh was delayed by one extra attempt. The resolution now reports FromCache=true if either lookup was served from the cache.
There was a problem hiding this comment.
Pull request overview
This PR updates LocationResolver so that recursive adapter-id resolutions propagate the FromCache result, and adds a regression test to ensure the ILocationResolver contract is met when the recursive (adapter-id) lookup is served from cache.
Changes:
- Propagates the recursive adapter-id lookup’s
FromCacheflag back to the outer resolution. - Updates the final
FromCachecomputation to consider adapter-id recursion. - Adds a test covering “fresh well-known + cached adapter-id” to prevent regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/IceRpc.Locator/Internal/LocationResolver.cs |
Updates recursive adapter-id resolution to propagate FromCache to the outer resolve call. |
tests/IceRpc.Locator.Tests/LocationResolverTests.cs |
Adds a regression test ensuring FromCache=true when the adapter-id resolution is served from cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The resolution is from the cache if this location's lookup was served from the cache, or if the recursive | ||
| // adapter-id resolution was served from the cache. | ||
| return (serviceAddress, serviceAddress is not null && (!resolved || adapterIdFromCache)); |
There was a problem hiding this comment.
🤖 Claude: Not applied, for three reasons. (1) Reporting only the inner resolution's flag would regress the flag's purpose in LocatorInterceptor: with a cached well-known→adapter-id mapping and freshly-resolved addresses, the staleness risk is the cached mapping itself — if FromCache were false, the retry would never request refreshCache and could not recover from a stale mapping (the addresses would be re-resolved fresh from the same wrong adapter-id every attempt). (2) The outer-cached ⇒ FromCache=true behavior predates this PR (the old expression was !resolved); this PR only adds the inner flag per the audit recommendation ("from-cache if either lookup was served from the cache"). (3) The doc wording you cite was the mismatch — I've clarified the ILocationResolver.ResolveAsync returns doc to say the resolution (either lookup) was served from the cache, in which case a refresh-on-retry can produce a different result.
Co-authored-by: Bernard Normier <bernard@zeroc.com>
Co-authored-by: Bernard Normier <bernard@zeroc.com>
Fixes icerpc/icerpc-csharp-audit#39.
When a well-known lookup returns a service address with an adapter-id,
LocationResolver.PerformResolveAsyncresolves the adapter-id recursively and discarded the inner resolution'sFromCacheflag, computing the returnedFromCachesolely from the outer lookup. A fresh well-known lookup followed by an adapter-id resolution served from the cache (entry younger thanRefreshThreshold) returnedFromCache = falseeven though the server addresses came from the cache, violating theILocationResolvercontract:LocatorInterceptorthen didn't setICachedResolutionFeature, so a retry didn't requestrefreshCacheand the cache refresh was delayed by one extra attempt.The resolution now reports
FromCache = trueif either lookup was served from the cache.Tests
Location_recursive_resolution_with_cached_adapter_id_is_from_cache: fresh well-known lookup + cached adapter-id entry; assertsFromCacheis true, the cached address is returned, and the finder is called exactly once. Fails without the fix (FromCachewas false).All
IceRpc.Locator.Testspass (48/48),dotnet buildanddotnet format --verify-no-changesclean.