Log background cache refresh failures in LocationResolver#4550
Log background cache refresh failures in LocationResolver#4550pepone merged 2 commits intoicerpc:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #4428 by ensuring failures from fire-and-forget background cache refreshes in the locator location resolution flow are observed and logged, instead of silently becoming unobserved task exceptions.
Changes:
- Add a background refresh helper in
LocationResolverthat awaits refresh work and logs failures. - Thread an
ILoggerinto the cachedLocationResolverimplementation and introduce a new log event ID for background refresh failures. - Update locator tests and composition root to pass a logger into
LocationResolver.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/IceRpc.Locator.Tests/LocationResolverTests.cs | Updates LocationResolver construction to pass NullLogger.Instance after constructor signature change. |
| src/IceRpc.Locator/LocatorInterceptor.cs | Passes logger into LocationResolver from the locator composition root. |
| src/IceRpc.Locator/LocationEventId.cs | Adds BackgroundRefreshFailed event id for the new log message. |
| src/IceRpc.Locator/Internal/LocationResolver.cs | Replaces fire-and-forget refresh with RefreshInBackgroundAsync that catches and logs refresh exceptions. |
| private async Task RefreshInBackgroundAsync(Location location) | ||
| { | ||
| try | ||
| { | ||
| _ = await _serverAddressFinder.FindAsync(location, cancellationToken: default).ConfigureAwait(false); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| _logger.LogBackgroundRefreshFailed(location.Kind, location, exception); | ||
| } |
There was a problem hiding this comment.
The new background-refresh error logging path (catch in RefreshInBackgroundAsync) isn’t covered by tests. Please add a unit test that forces IServerAddressFinder.FindAsync to throw during a background refresh (expired cache entry + background enabled) and asserts a log entry is emitted with EventId=BackgroundRefreshFailed and the exception attached (e.g., using TestLoggerFactory from IceRpc.Tests.Common).
| return (serviceAddress, serviceAddress is not null && !resolved); | ||
| } | ||
|
|
||
| private async Task RefreshInBackgroundAsync(Location location) |
There was a problem hiding this comment.
(Minor) I would use a local function. That's the pattern we typically use.
Matches the pattern used elsewhere in the repo (e.g. ClientConnection's PerformDisposeAsync/PerformShutdownAsync) — the helper is only invoked from inside PerformResolveAsync and captures the enclosing 'location' parameter, so a local function expresses that scope more directly than a private instance method.
Fixes #4428
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com