Handle root query client lifetimes

[PrzemyslawKlys]

Summary

• ensure root server QueryDns array overload keeps ClientX instances alive until resolution tasks complete
• add test hook visibility and regression test verifying multiple root lookups avoid premature disposal

Testing

• dotnet test DnsClientX.sln (fails: numerous DNS tests require external network access; environment reports network unreachable)

---

Codex Task

[claude]

Claude finished @PrzemyslawKlys's task —— View job

---

Pull Request Review

Review Tasks:

• Read repository guidelines (CLAUDE.md)
• Analyze DnsClientX.QueryDns.cs changes
• Review ResolveFromRootTests.cs test implementation
• Check AssemblyInfo.cs change
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check for security concerns
• Assess test coverage

Code Quality and Best Practices ✅

DnsClientX.QueryDns.cs (lines 97-118):

• Excellent implementation of proper resource management for the root server array overload
• Correctly uses try/finally pattern to ensure clients are disposed even if exceptions occur
• Proper use of List<ClientX> to track all created clients for cleanup
• Good separation of concerns with factory pattern for client creation
• Clean async/await pattern with proper ConfigureAwait(false)

ResolveFromRootTests.cs:

• Well-structured regression test that effectively verifies the fix
• Good use of factory override pattern for testing
• Proper cleanup with try/finally restoration of original factories
• Good test naming and documentation

AssemblyInfo.cs:

• Appropriate addition of InternalsVisibleTo to enable test hook visibility

Potential Issues ✅

No significant issues found. The implementation correctly addresses the lifetime management concern:

1. Before fix: Individual clients were created and could be GC'd before async operations completed
2. After fix: All clients are tracked in a list and disposed in finally block, ensuring they live until all tasks complete

Performance Considerations ✅

Positive performance impacts:

• Minimal overhead from List<ClientX> tracking (only for RootServer endpoint)
• Proper resource cleanup prevents resource leaks
• No unnecessary object allocations beyond the tracking list

DnsClientX.QueryDns.cs:98-118 - The implementation is efficient:

• Uses ToArray() only once for task creation
• Disposes clients in finally block, not during normal execution path
• Factory pattern allows for easy testing without performance penalty

Security Concerns ✅

No security issues identified:

• Proper resource disposal prevents resource exhaustion attacks
• Factory pattern is safely implemented with null checks
• Test hooks are properly scoped to test assembly only via InternalsVisibleTo

Test Coverage ✅

Excellent test coverage for the specific issue:

ResolveFromRootTests.cs:23-72 provides comprehensive regression test:

• ✅ Tests multiple concurrent root lookups (the exact scenario that was failing)
• ✅ Verifies clients remain alive during async operations
• ✅ Confirms proper disposal after completion
• ✅ Uses proper async testing patterns with TaskCompletionSource
• ✅ Includes proper cleanup of test hooks

Minor suggestions:

1. Consider adding a test for the exception path to verify clients are disposed even when tasks fail
2. The magic number Task.Delay(10) on line 48 could be documented or made configurable

Overall Assessment ⭐

This is a high-quality fix that:

• ✅ Correctly solves the identified lifetime management issue
• ✅ Includes comprehensive regression testing
• ✅ Follows established code patterns and best practices
• ✅ Maintains backward compatibility
• ✅ Has minimal performance impact

The implementation demonstrates good understanding of async resource management patterns in .NET and includes the necessary infrastructure for testing internal behavior.

Recommendation: ✅ Approve and merge

---

[claude]

Claude finished @PrzemyslawKlys's task —— View job

---

Pull Request Review

Review Tasks:

• Read repository guidelines (CLAUDE.md)
• Analyze DnsClientX.QueryDns.cs changes
• Review ResolveFromRootTests.cs test implementation
• Check AssemblyInfo.cs change
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check for security concerns
• Assess test coverage

Code Quality and Best Practices ✅

DnsClientX.QueryDns.cs (lines 97-118):

• Excellent implementation of proper resource management for the root server array overload
• Correctly uses try/finally pattern to ensure clients are disposed even if exceptions occur
• Proper use of List<ClientX> to track all created clients for cleanup
• Good separation of concerns with factory pattern for client creation
• Clean async/await pattern with proper ConfigureAwait(false)

ResolveFromRootTests.cs:

• Well-structured regression test that effectively verifies the fix
• Good use of factory override pattern for testing
• Proper cleanup with try/finally restoration of original factories
• Good test naming and documentation

AssemblyInfo.cs:

• Appropriate addition of InternalsVisibleTo to enable test hook visibility

Potential Issues ✅

No significant issues found. The implementation correctly addresses the lifetime management concern:

1. Before fix: Individual clients were created and could be GC'd before async operations completed
2. After fix: All clients are tracked in a list and disposed in finally block, ensuring they live until all tasks complete

Performance Considerations ✅

Positive performance impacts:

• Minimal overhead from List<ClientX> tracking (only for RootServer endpoint)
• Proper resource cleanup prevents resource leaks
• No unnecessary object allocations beyond the tracking list

DnsClientX.QueryDns.cs:98-118 - The implementation is efficient:

• Uses ToArray() only once for task creation
• Disposes clients in finally block, not during normal execution path
• Factory pattern allows for easy testing without performance penalty

Security Concerns ✅

No security issues identified:

• Proper resource disposal prevents resource exhaustion attacks
• Factory pattern is safely implemented with null checks
• Test hooks are properly scoped to test assembly only via InternalsVisibleTo

Test Coverage ✅

Excellent test coverage for the specific issue:

ResolveFromRootTests.cs:23-72 provides comprehensive regression test:

• ✅ Tests multiple concurrent root lookups (the exact scenario that was failing)
• ✅ Verifies clients remain alive during async operations
• ✅ Confirms proper disposal after completion
• ✅ Uses proper async testing patterns with TaskCompletionSource
• ✅ Includes proper cleanup of test hooks

Minor suggestions:

1. Consider adding a test for the exception path to verify clients are disposed even when tasks fail
2. The magic number Task.Delay(10) on line 48 could be documented or made configurable

Overall Assessment ⭐

This is a high-quality fix that:

• ✅ Correctly solves the identified lifetime management issue
• ✅ Includes comprehensive regression testing
• ✅ Follows established code patterns and best practices
• ✅ Maintains backward compatibility
• ✅ Has minimal performance impact

The implementation demonstrates good understanding of async resource management patterns in .NET and includes the necessary infrastructure for testing internal behavior.

Recommendation: ✅ Approve and merge

---

[Copilot]

Pull request overview

This PR addresses a critical lifetime management issue in the root DNS server query functionality. Previously, ClientX instances were being disposed prematurely within the LINQ Select operation, potentially causing tasks to fail when accessing disposed resources.

Key Changes:

• Refactored QueryDns array overload to maintain client references until all tasks complete
• Added test hooks (RootClientFactory and RootResolveOverride) to enable isolated testing
• Introduced regression test to verify clients remain alive during parallel root lookups

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
DnsClientX/DnsClientX.QueryDns.cs	Refactored root server query logic to use list-based lifetime management with try/finally disposal pattern; added internal test hooks for mocking client creation and resolution
DnsClientX/AssemblyInfo.cs	Added blank line for formatting consistency
DnsClientX.Tests/ResolveFromRootTests.cs	Added comprehensive regression test using mock clients to verify lifetime management fix; test validates clients remain undisposed during task execution and are properly cleaned up afterward

[Copilot]

If any Dispose() call throws an exception in the finally block, it will suppress exceptions from Task.WhenAll(). Consider wrapping the disposal loop in a try-catch to prevent disposal exceptions from masking task failures:

} finally {
    foreach (var client in clients) {
        try {
            client.Dispose();
        } catch {
            // Log or ignore disposal failures
        }
    }
}

Suggested change

	client.Dispose();
	try {
	client.Dispose();
	} catch {
	// Log or ignore disposal failures
	}

[Copilot]

[nitpick] The method doesn't validate if the name array is null or empty. When an empty array is passed, the method will create an empty clients list and return an empty array of DnsResponse, which may be the intended behavior. However, consider adding explicit null/empty validation or documenting this behavior.

Suggested change

	/// <returns>A task that represents the asynchronous operation. The task result contains the DNS response.</returns>
	public static async Task<DnsResponse[]> QueryDns(string[] name, DnsRecordType recordType, DnsEndpoint dnsEndpoint = DnsEndpoint.System, DnsSelectionStrategy dnsSelectionStrategy = DnsSelectionStrategy.First, int timeOutMilliseconds = Configuration.DefaultTimeout, bool retryOnTransient = true, int maxRetries = 3, int retryDelayMs = 200, bool requestDnsSec = false, bool validateDnsSec = false, bool typedRecords = false, bool parseTypedTxtRecords = false, CancellationToken cancellationToken = default) {
	/// <returns>A task that represents the asynchronous operation. The task result contains the DNS response.</returns>
	/// <exception cref="ArgumentNullException">Thrown if <paramref name="name"/> is null.</exception>
	/// <remarks>
	/// If <paramref name="name"/> is an empty array, the method returns an empty <see cref="DnsResponse"/> array.
	/// </remarks>
	public static async Task<DnsResponse[]> QueryDns(string[] name, DnsRecordType recordType, DnsEndpoint dnsEndpoint = DnsEndpoint.System, DnsSelectionStrategy dnsSelectionStrategy = DnsSelectionStrategy.First, int timeOutMilliseconds = Configuration.DefaultTimeout, bool retryOnTransient = true, int maxRetries = 3, int retryDelayMs = 200, bool requestDnsSec = false, bool validateDnsSec = false, bool typedRecords = false, bool parseTypedTxtRecords = false, CancellationToken cancellationToken = default) {
	if (name == null) {
	throw new ArgumentNullException(nameof(name));
	}
	if (name.Length == 0) {
	return Array.Empty<DnsResponse>();
	}

[Copilot]

The XML documentation comment should be placed before the [Fact] attribute, not after it. The standard convention is to place documentation comments immediately before attributes.

Suggested change

	[Fact]
	/// <summary>
	/// Ensures root lookups created in parallel keep their clients alive until completion.
	/// </summary>
	/// <summary>
	/// Ensures root lookups created in parallel keep their clients alive until completion.
	/// </summary>
	[Fact]

[Copilot]

[nitpick] Consider checking cancellationToken.IsCancellationRequested before the LINQ query (line 100) to avoid creating and disposing clients unnecessarily when the operation is already cancelled.

Suggested change

	if (dnsEndpoint == DnsEndpoint.RootServer) {
	if (dnsEndpoint == DnsEndpoint.RootServer) {
	if (cancellationToken.IsCancellationRequested) {
	return await Task.FromCanceled<DnsResponse[]>(cancellationToken).ConfigureAwait(false);
	}

[Copilot]

This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

Suggested change

	try {
	var tasks = name.Select(n => {
	var client = RootClientFactory();
	clients.Add(client);
	if (cancellationToken.IsCancellationRequested) {
	return Task.FromCanceled<DnsResponse>(cancellationToken);
	}
	var resolver = RootResolveOverride;
	return resolver != null
	? resolver(client, n, recordType, cancellationToken)
	: client.ResolveFromRoot(n, recordType, cancellationToken: cancellationToken);
	}).ToArray();
	var tasks = name.Select(n => {
	var client = RootClientFactory();
	clients.Add(client);
	if (cancellationToken.IsCancellationRequested) {
	return Task.FromCanceled<DnsResponse>(cancellationToken);
	}
	var resolver = RootResolveOverride;
	return resolver != null
	? resolver(client, n, recordType, cancellationToken)
	: client.ResolveFromRoot(n, recordType, cancellationToken: cancellationToken);
	}).ToArray();
	try {