fix(client): GetServers() throws FormatException on DNS hostnames by currantw · Pull Request #497 · valkey-io/valkey-glide-csharp

currantw · 2026-07-30T20:26:33Z

Summary

Fix ConnectionMultiplexer.GetServers() to support DNS hostnames in cluster topology responses. Previously, it used IPEndPoint.Parse() which throws FormatException when node addresses are DNS names (e.g., AWS ElastiCache endpoints like my-cluster.abc123.cache.amazonaws.com:6379).

Issue Link

Closes #419.

Features and Behaviour Changes

GetServers() now correctly returns DnsEndPoint instances for cluster nodes with DNS hostnames, instead of throwing FormatException.
GetEndPoints(false) works for clusters with DNS-based topology.
No behaviour change for clusters with IP-based topology (returns IPEndPoint as before).

Implementation

Replaced IPEndPoint.Parse(addr) with Format.TryParseEndPoint(addr, ...) in the cluster branch of GetServers(). This utility already exists in the codebase and is used throughout (e.g., ConfigurationOptions parsing, EndPointCollection). It returns IPEndPoint for IP addresses and DnsEndPoint for hostnames.

Extracted the LINQ expression into an explicit loop for readability.

Limitations

⚪ None

Testing

Integration tests added: DnsGetServersTests (gated behind VALKEY_GLIDE_DNS_TESTS_ENABLED) starts a cluster with cluster-announce-hostname and verifies:

GetServers() returns DnsEndPoint instances
GetEndPoints(false) returns valid DNS endpoints that can be looked up via GetServer(endpoint)

These tests depend on valkey-io/valkey-glide#6667 which adds cluster-announce-hostname support to cluster_manager.py.

Manual verification: Tested end-to-end against a 3-node Valkey 9.0.3 cluster configured with cluster-announce-hostname valkey.glide.test.tls.com:

GetServers() returned 3 DnsEndPoint instances without throwing.
GetEndPoints(false) returned correct DNS endpoints.
GetServer(endpoint) successfully looked up each discovered endpoint.
All existing unit tests pass (558/558).

Related Issues

valkey-io/valkey-glide#6667 — adds cluster-announce-hostname support to cluster_manager.py (required for DNS integration tests)

Checklist

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated and all checks pass.
~~CHANGELOG.md, README.md, DEVELOPER.md, and other documentation files are updated.~~
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.
~~All TODOs referencing issue(s) closed by this PR have been resolved.~~

…NS hostnames ConnectionMultiplexer.GetServers() used IPEndPoint.Parse() to parse cluster node addresses, which throws FormatException when the cluster topology contains DNS hostnames (e.g., AWS ElastiCache endpoints). Replace with Format.TryParseEndPoint which correctly handles both IP addresses (returning IPEndPoint) and DNS hostnames (returning DnsEndPoint). Closes valkey-io#419 Signed-off-by: currantw <taylor.curran@improving.com>

Add integration tests that start a cluster with cluster-announce-hostname and verify GetServers()/GetEndPoints(false) correctly return DnsEndPoint instances. Also thread a `host` parameter through ServerManager and Server/ClusterServer to support passing --host to cluster_manager.py. These tests are gated behind VALKEY_GLIDE_DNS_TESTS_ENABLED and require the cluster_manager.py change from valkey-io/valkey-glide#6667. Signed-off-by: currantw <taylor.curran@improving.com>

- Move GetServers/GetEndPoints DNS topology tests into existing DnsTests class with DnsTestsFixture (add DnsClusterServer to the fixture). - Use List<ValkeyServer> instead of array+index in GetServers(). - Add host parameter to StandaloneServer for consistency. - Make TLS server startup non-fatal in DnsTestsFixture so non-TLS tests can still run when TLS servers fail to start. - Remove separate DnsGetServersTests.cs file. Signed-off-by: currantw <taylor.curran@improving.com>

Signed-off-by: currantw <taylor.curran@improving.com>

Use idiomatic xUnit assertion across integration tests. Signed-off-by: currantw <taylor.curran@improving.com>

Bump valkey-glide submodule to cf3d56f08 which includes cluster-announce-hostname support in cluster_manager.py (#6667). Add missing `recovery_requests_queue_size` field to ConnectionRequest in ffi.rs to fix compilation against the updated glide-core. Signed-off-by: currantw <taylor.curran@improving.com>

The DNS cluster server (using cluster-announce-hostname) may fail to start in CI environments where wait_for_all_topology_views times out because the CLUSTER SLOTS response reports IPv6 addresses instead of the announced hostname. Wrap in try/catch and skip tests when null. Signed-off-by: currantw <taylor.curran@improving.com>

Copilot

Pull request overview

Fixes ConnectionMultiplexer.GetServers() (and therefore GetEndPoints(false)) for cluster topologies that advertise DNS hostnames by switching endpoint parsing from IPEndPoint.Parse(...) (which throws on hostnames) to Format.TryParseEndPoint(...) (which can return DnsEndPoint).

Changes:

Update cluster GetServers() discovery to parse addresses into EndPoint (supporting DNS hostnames).
Extend test server utilities to start clusters with an announced hostname (--host).
Add/adjust integration tests to validate DNS-based discovery paths and tighten some assertions (Assert.NotEmpty).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/Valkey.Glide.TestUtils/ServerManager.cs	Adds optional `host` arg to pass `--host` to the cluster manager script.
tests/Valkey.Glide.TestUtils/Server.cs	Plumbs optional `host` through server wrappers (cluster/standalone).
tests/Valkey.Glide.IntegrationTests/ScriptingCommandTests.cs	Replaces `Count > 0` assertions with `Assert.NotEmpty`.
tests/Valkey.Glide.IntegrationTests/DnsTests.cs	Adds integration tests for `GetServers()` / `GetEndPoints(false)` with DNS topology; extends fixture to attempt starting a DNS-announced cluster.
tests/Valkey.Glide.IntegrationTests/ClusterClientTests.cs	Replaces `Count > 0` assertions with `Assert.NotEmpty`.
sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs	Uses `Format.TryParseEndPoint` in cluster `GetServers()` to support DNS hostnames.
rust/src/ffi.rs	Adds new struct field initialization (`recovery_requests_queue_size: None`) to keep Rust-side request construction in sync.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

currantw · 2026-07-31T02:55:45Z

+        foreach (EndPoint endpoint in endpoints)
+        {
+            _ = Assert.IsType<DnsEndPoint>(endpoint);
+            Assert.Contains(HostnameTls, endpoint.ToString());
+        }


Addressed in deae300 — added GetServer(endpoint) assertion.

currantw · 2026-07-31T02:55:47Z

+            try
+            {
+                TlsClusterServer = new(useTls: true);
+                TlsStandaloneServer = new(useTls: true);
+            }
+            catch
+            {
+                // TLS servers may fail to start in some environments.
+            }


Addressed in deae300 — removed the try/catch so TLS failures crash the fixture loudly.

- Add GetServer(endpoint) assertion to GetEndPoints test to verify returned DnsEndPoints are usable for server lookup. - Remove try/catch around DnsClusterServer and TLS server startup so failures are loud instead of silently skipped. - Remove unnecessary SkipWhen null guards. Signed-off-by: currantw <taylor.curran@improving.com>

currantw · 2026-07-31T03:17:46Z

        tcp_nodelay: false,
        periodic_checks: None, // TODO #485: Expose periodic_checks in ClusterClientConfiguration.
        inflight_requests_limit: None, // TODO #484: Expose inflight_requests_limit in ConnectionConfiguration.
+        recovery_requests_queue_size: None,


Due to bumping submodule.

currantw · 2026-07-31T03:18:18Z

        if (loadResult.HasMultiData)
        {
-            Assert.True(loadResult.MultiValue.Count > 0);
+            Assert.NotEmpty(loadResult.MultiValue);


Noticed this one of of my tests. Just asking Claude to find and simplify these 🤷

…Async(host) - Add optional `host` parameter to `CreateClientAsync` on Server, ClusterServer, and StandaloneServer to support connecting via a custom hostname. - Replace `BuildClient` helper in DnsTests with `GetServer` + `CreateClientAsync(host)`. - Pass `host: HostnameTls` to all 4 fixture servers so DNS hostname is used consistently. - Use `ClusterAndTlsMode` data member to test all cluster/TLS combinations for connection tests. - Use `TlsMode` data member for GetServers/GetEndPoints tests with `TrustIssuer` for TLS cert validation. - Add static constructor skip for class-level DNS test gating. Signed-off-by: currantw <taylor.curran@improving.com>

currantw added 2 commits July 30, 2026 11:40

currantw self-assigned this Jul 30, 2026

currantw added 6 commits July 30, 2026 14:08

Minor GetServers cleanup

d1c9be6

Signed-off-by: currantw <taylor.curran@improving.com>

Cleanup DnsTests

2bda1c1

Signed-off-by: currantw <taylor.curran@improving.com>

style(test): replace Assert.True(x.Count > 0) with Assert.NotEmpty(x)

04c19e4

Use idiomatic xUnit assertion across integration tests. Signed-off-by: currantw <taylor.curran@improving.com>

currantw requested a review from Copilot July 31, 2026 02:07

Copilot started reviewing on behalf of currantw July 31, 2026 02:08 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

currantw requested review from prateek-kumar-improving and xShinnRyuu July 31, 2026 03:17

currantw commented Jul 31, 2026

View reviewed changes

currantw added this to the 1.2 milestone Jul 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(client): GetServers() throws FormatException on DNS hostnames - #497

fix(client): GetServers() throws FormatException on DNS hostnames#497
currantw wants to merge 10 commits into
valkey-io:mainfrom
currantw:419-get-servers-endpoint-bug

currantw commented Jul 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

currantw Jul 31, 2026

Uh oh!

currantw Jul 31, 2026

Uh oh!

currantw Jul 31, 2026

Uh oh!

currantw Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

currantw commented Jul 30, 2026

Summary

Issue Link

Features and Behaviour Changes

Implementation

Limitations

Testing

Related Issues

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

currantw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

currantw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

currantw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

currantw Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants