feat: implement DNS resolver and query handling by niteshpurohit · Pull Request #930 · Code-Vedas/laghu

niteshpurohit · 2026-09-19T13:27:10Z

Added DnsResolver class to manage DNS queries and responses.
Introduced DnsQueryToken for tracking individual queries.
Implemented asynchronous handling of DNS queries using c-ares library.
Created structures for DNS addresses and nameservers to encapsulate relevant data.
Added error handling for various DNS operations and responses.
Implemented tests for DNS resolver functionality, including success, failure, and timeout scenarios.
Ensured that all new structures are trivially copyable where applicable.

closes: #62
closes: #59

- Added DnsResolver class to manage DNS queries and responses. - Introduced DnsQueryToken for tracking individual queries. - Implemented asynchronous handling of DNS queries using c-ares library. - Created structures for DNS addresses and nameservers to encapsulate relevant data. - Added error handling for various DNS operations and responses. - Implemented tests for DNS resolver functionality, including success, failure, and timeout scenarios. - Ensured that all new structures are trivially copyable where applicable. closes: #62 closes: #59

Copilot

Copilot review overview

🟡 Changes recommended

Cancellation leaves native query slots occupied until c-ares completes, allowing canceled work to block or exhaust resolver capacity.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Document callback-only lifetime for DnsQueryResult data · New
Cancel requests without leaking outstanding query slots · New

What changed in this PR

Adds an asynchronous c-ares DNS resolver adapter with bounded queries, socket/timer integration, TTL results, normalized errors, and test/build support.

Changes:

Introduces DNS adapter contracts and c-ares implementation.
Adds resolver lifecycle, nameserver configuration, cancellation, and query tests.
Updates dependency configuration and CI coverage.

File	Description
`src/adapters/dns.cpp`	c-ares resolver implementation
`src/adapters/contract/laghu/adapters/dns.hpp`	Public DNS contracts
`tests/adapters/dns.cpp`	DNS behavior tests
`tests/adapters/dns_contract.cpp`	Type contract checks
`src/core/contract/laghu/core/contract.hpp`	DNS dependency operations
`src/adapters/dependency.cpp`	Dependency operation mapping
`CMakeLists.txt`	DNS targets and tests
`cmake/LaghuDependencies.cmake`	Dependency build-state handling
`.github/workflows/toolchain.yml`	Toolchain DNS coverage
`.github/workflows/codeql.yml`	CodeQL DNS build coverage

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- Added new fields to QuerySlot for better channel management. - Improved cancellation logic to handle native query states. - Updated address_complete and socket_state functions to manage query lifecycle more effectively. - Enhanced tests to cover new cancellation scenarios and ensure proper query exhaustion handling.

Copilot

Copilot review overview

🟡 Changes recommended

The review found a guaranteed invalid expected-value access in tests plus resolver completion and socket-capacity lifecycle bugs.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3

Open (3)

Include async DNS files in build identity inputs · New
Prevent DNS socket capacity overflow and recover safely · New
Prevent cancellation of queries already completing · New

Resolved since last review (2)

Cancel requests without leaking outstanding query slots
Document callback-only lifetime for DnsQueryResult data

Previously missed (1)

In code that hasn't changed since last review

Check expected error before accessing Result::value()

tests/adapters/dns.cpp:403

This expression calls value() on the Result returned for the 254-byte hostname. That result is intentionally an error, so the test invokes std::expected::value() without checking it and can throw/terminate instead of validating the input error. Call has_value() directly (and, ideally, assert invalid_range).

+list(FIND LAGHU_EFFECTIVE_FEATURES async_dns laghu_async_dns_feature_index)
+if(NOT laghu_async_dns_feature_index EQUAL -1)
+  add_library(laghu_dns STATIC
+    src/adapters/dns.cpp


- Added logic to handle socket capacity failures in the DNS resolver. - Introduced a new flag in QuerySlot to track socket capacity issues. - Updated the completion callback to check for inactive tokens during resolution. - Added tests to validate behavior when socket capacity is exceeded. - Improved overall robustness of DNS query handling under load.

Copilot

Copilot review overview

🔵 Needs a closer look

Completion callbacks cannot submit replacement queries when all query slots are occupied, and timeout coverage is environment-dependent.

Review effort: Lite
Findings: 1

Open (1)

Include async DNS files in build identity inputs

Resolved since last review (2)

Prevent cancellation of queries already completing
Prevent DNS socket capacity overflow and recover safely

Previously missed (2)

In code that hasn't changed since last review

Reclaim completed slots before allowing reentrant submissions

src/adapters/dns.cpp:210

When all 64 query slots are occupied, a completion decrements outstanding but leaves its slot active/native_done until the surrounding process_events() or process_timeout() call returns. A completion sink that immediately submits a replacement query therefore sees available outstanding capacity but gets exhaustion because the slot scan below skips the completed slot. Finalize/reclaim the completed slot before allowing reentrant submissions, while ensuring the outer event-loop path cannot retire a newly reused slot.

Make timeout test deterministic and assert native timeout status

tests/adapters/dns.cpp:368

This purported timeout test targets 127.0.0.1:9, which commonly fails immediately with ARES_ECONNREFUSED, and only checks the broad io error. On a host where the port is closed it never exercises ARES_ETIMEOUT, process_timeout(), or timeout normalization; on a filtered host the result changes. Use a fixture that receives and drops the DNS packet (or otherwise assert the native timeout status) so this coverage is deterministic.

- Added support for asynchronous DNS resolution by introducing new properties in QuerySlot. - Implemented completion handling for queries to allow reentrant submissions. - Updated tests to validate the new asynchronous behavior and ensure proper handling of query results.

Copilot

Copilot review overview

🟡 Changes recommended

The resolver currently uses a c-ares typedef/API compatibility issue and a warning-as-error TTL comparison that prevent clean supported builds.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

Use legacy-compatible ares_channel handle type · New
Remove impossible negative check on unsigned ai_ttl · New
Include async DNS files in build identity inputs

- Changed the type of the channel member in QuerySlot from ares_channel_t* to ares_channel. - This change simplifies the management of the DNS channel within the QuerySlot structure.

Copilot

Copilot review overview

🟡 Changes recommended

Batched event and timeout processing can dereference freed resolver state after a reentrant completion callback destroys or replaces the resolver.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 1

Open (4)

Guard resolver lifetime after synchronous completion callbacks · New
Prevent use-after-free when callbacks replace the resolver · New
Remove impossible negative check on unsigned ai_ttl
Include async DNS files in build identity inputs

Resolved since last review (1)

Use legacy-compatible ares_channel handle type

Previously missed (1)

In code that hasn't changed since last review

Define callback behavior for queries canceled during teardown

src/adapters/contract/laghu/adapters/dns.hpp:98

This says completion callbacks are synchronous with destruction, but release() sets shutting_down and drops outstanding c-ares queries without calling DnsQuerySink::complete. Consumers following this contract may wait for callbacks that never arrive; document the destruction behavior explicitly or dispatch cancellation results during teardown.

- Updated comments to clarify that destruction of the resolver abandons outstanding queries without invoking their completion callbacks. - Modified `process_events` and `process_timeout` methods to return early if the query is done or if socket capacity fails, improving clarity and flow. - Added tests to verify that the resolver can be destroyed during event and timeout callbacks, ensuring proper resource management.

Copilot

Copilot review overview

🔵 Needs a closer look

The system-resolver configuration path is constructed but never exercised by a real lookup test.

Review effort: Lite
Findings: 1

Open (1)

Include async DNS files in build identity inputs

Resolved since last review (3)

Prevent use-after-free when callbacks replace the resolver
Guard resolver lifetime after synchronous completion callbacks
Remove impossible negative check on unsigned ai_ttl

Previously missed (1)

In code that hasn't changed since last review

Test system resolver configuration path

tests/adapters/dns.cpp:522

This only constructs a resolver with an empty nameserver list; every actual lookup in query_fixture supplies an explicit server. Consequently the nameservers.empty() path is never exercised through ares_getaddrinfo, so system resolver configuration can regress without detection despite being an adapter acceptance criterion. Add a lookup that drives the system configuration path (or a controllable system-resolver fixture).

- Introduced a new test case for system resolver lookup. - Validates the resolver's ability to resolve "localhost" to the correct IPv4 address. - Ensures that the resolver functions correctly under expected conditions.

Copilot

Copilot review overview

🟡 Changes recommended

c-ares library initialization is missing for the declared supported 1.20.0 system floor, so DNS creation can fail on supported installations.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Missing c-ares library initialization breaks resolver startup · New
Include async DNS files in build identity inputs

+  const int initialized = ares_init_options(&query.channel, &options, option_mask);
+  if (initialized != ARES_SUCCESS) {
+    return std::unexpected{native_error(core::DependencyOperation::dns_session,
+                                         initialized, state.log)};


niteshpurohit self-assigned this Sep 19, 2026

niteshpurohit added this pull request to stack #915 September 19, 2026 13:27

Copilot AI lite review requested due to automatic review settings September 19, 2026 13:27

github-actions Bot added the feature label Sep 19, 2026

Copilot started reviewing on behalf of niteshpurohit September 19, 2026 13:27 View session