From daccce8961e2b217a8492cfc803d8464055125d8 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 16:19:08 +0200 Subject: [PATCH 1/2] Filter removed server addresses with OptionalTransport equality The locator filter at LocatorInterceptor.ComputeServerAddresses used default ServerAddress equality on RemovedServerAddresses, which requires an exact transport-string match. The connection layer (ClientConnection / ConnectionCache) treats two server addresses as the same physical endpoint when their Protocol/Host/Port/Params match and at least one Transport is null (ServerAddressComparer.OptionalTransport). This asymmetry let a replica that the retry interceptor previously removed (e.g. ice://host:10000?transport=tcp) survive the filter when the locator re-returned it without a transport (ice://host:10000). The retry loop then re-targeted the same broken endpoint. Build a HashSet keyed on OptionalTransport once and use it for both the main and alt-server filters. Add a regression test covering the explicit-transport / no-transport asymmetry. --- src/IceRpc.Locator/LocatorInterceptor.cs | 9 ++++-- .../LocatorInterceptorTests.cs | 29 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/IceRpc.Locator/LocatorInterceptor.cs b/src/IceRpc.Locator/LocatorInterceptor.cs index ff8336d97a..a2c58dd0ac 100644 --- a/src/IceRpc.Locator/LocatorInterceptor.cs +++ b/src/IceRpc.Locator/LocatorInterceptor.cs @@ -108,13 +108,18 @@ public async Task InvokeAsync(OutgoingRequest request, Cancell ServiceAddress serviceAddress, IEnumerable excludedAddresses) { + // Use OptionalTransport so the filter matches the connection layer's equality. Default ServerAddress + // equality requires an exact transport-string match and would let a removed replica back in when the + // locator re-resolves it without an explicit transport. (ServerAddress? ServerAddress, ImmutableList AltServerAddresses) result = (serviceAddress.ServerAddress, serviceAddress.AltServerAddresses); - if (result.ServerAddress is ServerAddress serverAddress && excludedAddresses.Contains(serverAddress)) + if (result.ServerAddress is ServerAddress serverAddress && + excludedAddresses.Contains(serverAddress, ServerAddressComparer.OptionalTransport)) { result.ServerAddress = null; } - result.AltServerAddresses = result.AltServerAddresses.RemoveAll(e => excludedAddresses.Contains(e)); + result.AltServerAddresses = result.AltServerAddresses.RemoveAll( + e => excludedAddresses.Contains(e, ServerAddressComparer.OptionalTransport)); if (result.ServerAddress is null && result.AltServerAddresses.Count > 0) { diff --git a/tests/IceRpc.Locator.Tests/LocatorInterceptorTests.cs b/tests/IceRpc.Locator.Tests/LocatorInterceptorTests.cs index 49060c3704..70ac31231b 100644 --- a/tests/IceRpc.Locator.Tests/LocatorInterceptorTests.cs +++ b/tests/IceRpc.Locator.Tests/LocatorInterceptorTests.cs @@ -92,6 +92,35 @@ public async Task Resolve_refresh_cache_on_the_second_lookup() Assert.That(locationResolver.RefreshCache, Is.True); } + /// Verifies that an excluded server address that differs only in transport from a newly-resolved one + /// is filtered out: the connection layer treats them as the same physical endpoint, so the locator must use the + /// same OptionalTransport equality when applying RemovedServerAddresses. + [Test] + public async Task Removed_address_with_explicit_transport_filters_resolved_address_without_transport() + { + // Arrange + var invoker = new InlineInvoker((request, cancellationToken) => + Task.FromResult(new IncomingResponse(request, FakeConnectionContext.Instance))); + var resolved = new ServiceAddress(new Uri("ice://localhost:10000/foo")); + var locationResolver = new MockLocationResolver(resolved, adapterId: false); + var sut = new LocatorInterceptor(invoker, locationResolver); + var serviceAddress = new ServiceAddress(Protocol.Ice) { Path = "/foo" }; + using var request = new OutgoingRequest(serviceAddress); + var serverAddressFeature = new ServerAddressFeature(serviceAddress) + { + RemovedServerAddresses = ImmutableList.Create( + new ServerAddress(new Uri("ice://localhost:10000?transport=tcp"))) + }; + request.Features = request.Features.With(serverAddressFeature); + + // Act + await sut.InvokeAsync(request, default); + + // Assert + Assert.That(serverAddressFeature.ServerAddress, Is.Null); + Assert.That(serverAddressFeature.AltServerAddresses, Is.Empty); + } + /// Verifies that the locator interceptor does not set the refresh cache parameter on the second attempt /// to resolve a location if the first attempt returned a non cached result. [Test] From f852c7dca3f389db2f56cfe0c7e43074d9799898 Mon Sep 17 00:00:00 2001 From: Jose Date: Tue, 28 Apr 2026 10:30:06 +0200 Subject: [PATCH 2/2] Review fix --- src/IceRpc.Locator/LocatorInterceptor.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/IceRpc.Locator/LocatorInterceptor.cs b/src/IceRpc.Locator/LocatorInterceptor.cs index a2c58dd0ac..b230647998 100644 --- a/src/IceRpc.Locator/LocatorInterceptor.cs +++ b/src/IceRpc.Locator/LocatorInterceptor.cs @@ -108,9 +108,8 @@ public async Task InvokeAsync(OutgoingRequest request, Cancell ServiceAddress serviceAddress, IEnumerable excludedAddresses) { - // Use OptionalTransport so the filter matches the connection layer's equality. Default ServerAddress - // equality requires an exact transport-string match and would let a removed replica back in when the - // locator re-resolves it without an explicit transport. + // Use the ServerAddressComparer.OptionalTransport comparer so the filter matches the connection layer's + // equality. (ServerAddress? ServerAddress, ImmutableList AltServerAddresses) result = (serviceAddress.ServerAddress, serviceAddress.AltServerAddresses); if (result.ServerAddress is ServerAddress serverAddress &&