fix(net): parse and store DNS response records in cache by AprilNEA · Pull Request #154 · arcboxlabs/arcbox

AprilNEA · 2026-03-31T13:14:49Z

Summary

Parse answer RRs from raw DNS response bytes instead of storing empty Vec::new()
Extract record type, class, TTL, and rdata (A/AAAA as IpAddr, others as raw bytes)
Use minimum TTL from answer records for cache expiry
Skip caching responses with no answer records (NXDOMAIN, SERVFAIL)
DNS cache is now functional — cache hits return real data

Test plan

test_cache_response_stores_records — verifies parsed records
test_cache_hit_returns_valid_response — full round-trip cache store → lookup
test_cache_skips_empty_response — NXDOMAIN not cached
All 43 DNS tests pass

Closes ABX-229

Parse answer resource records from raw DNS response bytes in cache_response() instead of storing Vec::new(). Extracts record type, TTL, and rdata (A/AAAA/raw) so cache hits return actual data. Uses minimum TTL from answer records for cache expiry.

linear · 2026-03-31T13:14:53Z

ABX-229 DNS cache stores empty response records — caching is effectively broken

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56db2b110b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-31T13:19:22Z

virt/arcbox-net/src/dns.rs

+            .iter()
+            .map(|r| r.ttl)
+            .min()
+            .unwrap_or(self.config.cache_ttl.as_secs() as u32);
+        let ttl = Duration::from_secs(u64::from(min_ttl));


Decrement TTL before serving cached records

This computes entry expiration from the minimum upstream TTL, but the cached DnsRecord.ttl values are kept at their original values and later emitted unchanged by cached responses. In practice, a record cached with TTL 60 and served at 59 seconds old will still be returned with TTL 60, allowing downstream resolvers to keep stale data beyond the authoritative lifetime.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-31T13:19:22Z

virt/arcbox-net/src/dns.rs

+            let Some(record) = Self::parse_one_rr(response, &mut offset) else {
+                break;


Return empty cache parse result on RR parse failure

The answer parser currently breaks on the first RR parse failure and returns whatever records were parsed before that point, which causes partial responses to be cached. If an answer section contains a malformed/unsupported RR after valid ones, subsequent cache hits can return an incomplete record set (for example, only part of a multi-record answer), changing resolver behavior compared with the upstream response.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

Implements functional DNS response caching in virt/arcbox-net by parsing answer RRs from upstream response bytes and storing them as structured DnsRecords, so cache hits can return meaningful DNS answers.

Changes:

Parse DNS answer resource records (type/class/TTL/RDATA) from raw response bytes and store them in the cache.
Use the minimum TTL across cached answer records for cache expiry, and skip caching responses with no answers.
Add unit tests covering record parsing, cache hit round-trip behavior, and skipping empty-answer responses.

Copilot · 2026-03-31T13:19:59Z

virt/arcbox-net/src/dns.rs

+        }
+
+        // Parse answer records.
+        let mut records = Vec::with_capacity(ancount);


ancount is taken directly from the response header and then used in Vec::with_capacity(ancount). Since the response bytes are untrusted, a bogus ANCOUNT (e.g., 65535) could trigger a very large allocation attempt even if the packet is tiny. Consider capping the capacity to a sane upper bound derived from response.len() (or just push into a default Vec without preallocating).

Suggested change

let mut records = Vec::with_capacity(ancount);

let mut records = Vec::new();

Copilot · 2026-03-31T13:19:59Z

virt/arcbox-net/src/dns.rs

+        // Parse answer records.
+        let mut records = Vec::with_capacity(ancount);
+        for _ in 0..ancount {
+            let Some(record) = Self::parse_one_rr(response, &mut offset) else {
+                break;
+            };
+            records.push(record);
+        }
+        records


The docstring says this returns an empty vec on any parse failure, but the answer loop breaks on None and returns whatever records were parsed so far. That can lead to caching partial/incomplete answer sets. Either return Vec::new() when any RR fails to parse (to match the doc) or continue parsing by skipping just the problematic RR (after advancing offset).

Copilot · 2026-03-31T13:19:59Z

virt/arcbox-net/src/dns.rs

+    /// Caches a DNS response by parsing answer records from the raw bytes.
+    fn cache_response(&mut self, name: &str, qtype: DnsRecordType, response: &[u8]) {
+        let records = Self::parse_answer_records(response);
+        if records.is_empty() {
+            return;
+        }
+
+        // Use the minimum TTL from the answer records, falling back to config.
+        let min_ttl = records
+            .iter()
+            .map(|r| r.ttl)
+            .min()
+            .unwrap_or(self.config.cache_ttl.as_secs() as u32);
+        let ttl = Duration::from_secs(u64::from(min_ttl));
+
        let key = (name.to_lowercase(), qtype);
        let entry = CacheEntry {
-            records: Vec::new(), // Would parse from response in full implementation
+            records,
            cached_at: Instant::now(),
-            ttl: self.config.cache_ttl,
+            ttl,
        };
        self.cache.insert(key, entry);
    }


cache_response stores all parsed answer RRs, including ones whose owner name may differ from the original query name (e.g., a CNAME chain where the A/AAAA RR is for the canonical name). build_cached_response currently hardcodes the NAME field as a pointer to the question name for every record, so caching such responses will replay incorrect owner names. Consider filtering cached records to only those with record.name matching name (query name), or update cached-response building to emit each record’s actual owner name.

Copilot · 2026-03-31T13:19:59Z

virt/arcbox-net/src/dns.rs

+                let octets: [u8; 16] = rdata_bytes.try_into().ok()?;
+                DnsRdata::Aaaa(Ipv6Addr::from(octets))
+            }
+            _ => DnsRdata::Raw(rdata_bytes.to_vec()),


For non-A/AAAA record types you store DnsRdata::Raw(rdata_bytes.to_vec()) and later re-emit those bytes in cached responses. This is incorrect for RR types whose RDATA may contain compressed domain names (e.g., CNAME/PTR/MX/SRV), because compression pointers in the original upstream packet will refer to offsets that don’t exist in the reconstructed cached response. Either fully parse + re-encode those RDATA formats without compression pointers, or restrict caching to RR types with self-contained RDATA (A/AAAA/TXT, etc.).

Suggested change

_ => DnsRdata::Raw(rdata_bytes.to_vec()),

// For all other RR types, do not cache the RDATA. Their RDATA can contain

// compressed domain names, and re-emitting the raw bytes from the upstream

// packet would produce invalid compression pointers in reconstructed replies.

_ => {

return None;

}

Copilot AI review requested due to automatic review settings March 31, 2026 13:14

chatgpt-codex-connector bot reviewed Mar 31, 2026

View reviewed changes

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(net): parse and store DNS response records in cache#154

fix(net): parse and store DNS response records in cache#154
AprilNEA wants to merge 1 commit intomasterfrom
fix/dns-cache-records

AprilNEA commented Mar 31, 2026

Uh oh!

linear bot commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let Some(record) = Self::parse_one_rr(response, &mut offset) else {
		break;

	let mut records = Vec::with_capacity(ancount);
	let mut records = Vec::new();

-            _ => DnsRdata::Raw(rdata_bytes.to_vec()),
+            // For all other RR types, do not cache the RDATA. Their RDATA can contain
+            // compressed domain names, and re-emitting the raw bytes from the upstream
+            // packet would produce invalid compression pointers in reconstructed replies.
+            _ => {
+                return None;
+            }

Conversation

AprilNEA commented Mar 31, 2026

Summary

Test plan

Uh oh!

linear bot commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants