fix: fail service resolution fast when nsec denies the needed address types by bdraco · Pull Request #1827 · python-zeroconf/python-zeroconf

bdraco · 2026-08-28T18:12:56Z

Summary

Follow up to #1825. When an NSEC record at the host name denies every address type a request still needs, async_request now fails fast instead of waiting out the full timeout; RFC 6762 §6.1 makes such an NSEC an authoritative nonexistence assertion.

Details

NSEC records at the server name now set per family denial flags; the bitmap lists the types that exist, so an absent bit denies that family.
A new _is_denied check derives the wanted families from _query_record_types, so AddressResolverIPv4, AddressResolverIPv6, AddressResolver, and plain ServiceInfo all share one definition with no overrides.
Denials are re derived on every cache load, so a host that later gains the record resolves normally on the next request.
_load_from_cache also consults a cached NSEC at the server name when the request is still incomplete, covering the case where the other address family already resolved.
NSEC records are processed last within each update batch, so a denial for an SRV target learned later in the same response is not lost to wire ordering; when an SRV update changes the target host, denials are reset and re derived from the new host's cached NSEC.
Each NSEC is treated as an authoritative snapshot, so a newer bitmap that includes a previously denied family clears the stale denial.
Foreign NSEC records still bail on the key checks before any processing; the hot dispatch path is unchanged for other record types, confirmed against the Cython annotate output.

Test plan

poetry run pytest tests passes, 480 passed
new tests cover both single family resolvers, a denial arriving mid request, partial denial with the other family present, denial reset on a later request, the all types denied case for plain ServiceInfo, NSEC and SRV arriving in either order in one batch, denial reset when the SRV target moves, a newer bitmap clearing a stale denial, and NSECs for unrelated names being ignored
Cython annotate comparison against master shows untouched hot functions unchanged; the new cost sits only on NSEC record lines and cold paths
verified on a live network; an IPv6 resolve of a real v4 only device now fails in about 120ms instead of the 10s timeout, while IPv4, either family, and dual stack resolves are unaffected

codecov · 2026-08-28T18:16:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (7635d45) to head (0ffc2ed).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1827   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          33       33           
  Lines        3799     3839   +40     
  Branches      543      553   +10     
=======================================
+ Hits         3792     3832   +40     
  Misses          5        5           
  Partials        2        2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codspeed · 2026-08-28T18:27:37Z

Merging this PR will not alter performance

✅ 24 untouched benchmarks

_{Comparing nsec_address_denial (0ffc2ed) with master (ca9d027)}

Copilot

Pull request overview

Adds NSEC-based fast failure when required address families are authoritatively unavailable.

Changes:

Tracks IPv4 and IPv6 denial state.
Checks cached and live NSEC records during resolution.
Adds coverage for resolver variants and denial scenarios.

Reviewed changes

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

File	Description
`src/zeroconf/_services/info.py`	Implements address-family denial handling.
`src/zeroconf/_services/info.pxd`	Adds corresponding Cython declarations.
`tests/services/test_info.py`	Tests cached, live, partial, and reset behavior.

Suppressed comments (1)

src/zeroconf/_services/info.py:675

These flags only transition from False to True. If a newer authoritative NSEC for the same host now includes a previously denied family, the old denial remains and can be combined with the new bitmap to make _is_denied incorrectly report that all requested families are absent. Recompute both flags from every current server NSEC rather than accumulating denials across snapshots.

            if _TYPE_A not in rdtypes and not self._ipv4_denied:
                self._ipv4_denied = True
                updated = True
            if _TYPE_AAAA not in rdtypes and not self._ipv6_denied:
                self._ipv6_denied = True
                updated = True

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

bluetoothbot · 2026-08-28T18:53:21Z

~~Previous review~~ — superseded by a newer review below.

bluetoothbot

Tip

No blocking issues found — ready to merge.

bdraco · 2026-08-28T18:54:53Z

. A cached denial fails the request without ever sending a query

ServiceInfo is a single shot throw away

bluetoothbot · 2026-08-28T19:10:58Z

@bdraco: > . A cached denial fails the request without ever sending a query

Agreed, dropping that one. Flags reset at top of _load_from_cache, so each request re-derives from cache. Stale window bounded by NSEC TTL. Address guard in _is_denied covers cached-address case.

bluetoothbot · 2026-08-28T19:11:00Z

~~Previous review~~ — superseded by a newer review below.

bluetoothbot

Tip

No blocking issues found — ready to merge.

… types

bluetoothbot · 2026-08-28T20:05:35Z

PR Review — fix: fail service resolution fast when nsec denies the needed address types

Both outstanding suggestions from the last round are now covered by tests, and no production code changed since — approving with two cosmetic nits.

Specific things done well:

test_legacy_inverted_nsec_at_own_name_does_not_fail_request pins exactly the invariant that keeps the pre-0.150.4 inverted-bitmap shape safe: server == name + bitmap [AAAA] sets _ipv4_denied backwards, and the test asserts both the wrong flag and that _is_denied stays False. That makes the "needs both families" property explicit, so a future relaxation of _is_denied fails loudly instead of silently breaking legacy v4-only responders.
test_denied_flag_is_ignored_when_address_is_held reaches the not self._ipvX_addresses guard by the one route that actually gets there — a plain ServiceInfo with a cached SRV + A + host NSEC [TXT], incomplete only because TXT is missing. Both flags go True, an address is held, and _is_denied is False. That is the load-bearing safety net and it is now covered.
I re-verified test_own_nsec_response_does_not_complete_service against the new server-NSEC path in _load_from_cache: our own [A] NSEC sets ipv6_denied only, so _is_denied stays False and the request keeps querying for the missing TXT. No regression from the added cache lookup.
_read_bitmap (src/zeroconf/_protocol/incoming.py:411) raises on truncated or oversized windows rather than returning a short list, so a malformed NSEC cannot produce an empty rdtypes that would fast-fail a resolvable host.
NSEC deferral in async_update_records also improves the fix: correct nsec bitmap semantics and resolve txt denials #1825 TXT path: a same-batch real TXT now always wins over the denial, regardless of wire order.
Nit: @cython.locals on info.pxd:85 names update (the local is updated) and cache (no such local) — pre-existing, but the line is edited here and the fix restores the intended bint typing.
Nit: the new legacy-NSEC test docstring has a multi-line body that re-explains the bug, against the repo's documented test-docstring rule.
Previously raised and dismissed by @bdraco ("ServiceInfo is a single shot throw away"), not re-raised: a cached denial fails the request without putting a query on the wire.

🟢 Suggestions

1. [Pre-Existing Issue] `@cython.locals` for `async_update_records` names locals that don't exist
src/zeroconf/_services/info.pxd:85

The cython.locals list on this line declares update=bint and cache=DNSCache, but async_update_records has neither local — the accumulator is named updated (src/zeroconf/_services/info.py:576) and there is no cache binding in that method at all.

Why it matters: the intended effect of update=bint was to give the accumulator a C-level bint so updated |= self._process_record_threadsafe(...) compiles to a C or instead of a Python bitwise-or on bool objects. With the name misspelled, Cython silently declares two unused C locals and updated stays a Python object — the annotation is doing nothing on a path the PR description explicitly calls out as perf-sensitive.

This predates the PR, but the line is being edited here anyway (to add record and nsec_records), so renaming update -> updated and dropping the dead cache entry is a one-token fix that finally delivers the typing that was intended.

    @cython.locals(record_update=RecordUpdate, record=DNSRecord, update=bint, cache=DNSCache, nsec_records=list)
    cpdef void async_update_records(self, object zc, double now, cython.list records)

2. Test docstring retells the production-side story
tests/services/test_info.py:3010-3014

CLAUDE.md is explicit about this shape: "Test docstrings retelling the production-side story. A test docstring should name what the test pins, in one sentence — not re-explain the bug, the fix, or the surrounding flow."

The summary line already names what is pinned; the two-line body re-explains the legacy inverted-bitmap behaviour and the _is_denied invariant, which is PR-description material. Every other new test in this batch is a single line and reads well.

Dropping the body (or folding the essential half-clause into the summary) brings it in line with the rest of the file.

    """A pre-0.150.4 inverted NSEC from a service with server == name never denies the request.

    Legacy responders listed the missing address types, so the snapshot records
    the wrong family; a single wrong flag must never make _is_denied true.
    """

Checklist

Automated review by Kōan (Claude) HEAD=ac3afd2 4 min 55s

bluetoothbot

Tip

No blocking issues found — ready to merge.

bdraco · 2026-08-28T20:14:02Z

Addressed both nits in 0ffc2ed.

For the cython.locals one, the suggested updated=bint does not compile; the body already annotates updated: bool = False, so Cython errors with 'updated' redeclared. That annotation already produces a C bint (int __pyx_v_updated in the generated C), so I dropped the dead update and cache entries instead.

The test docstring is now a single line.

bdraco marked this pull request as ready for review August 28, 2026 18:13

bdraco force-pushed the nsec_address_denial branch from e665332 to 41012d1 Compare August 28, 2026 18:23

bdraco requested a balanced review from Copilot August 28, 2026 18:26

Copilot started reviewing on behalf of bdraco August 28, 2026 18:27 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread src/zeroconf/_services/info.py

Comment thread src/zeroconf/_services/info.py

bdraco force-pushed the nsec_address_denial branch from 41012d1 to 25efc2e Compare August 28, 2026 18:47

bluetoothbot approved these changes Aug 28, 2026

View reviewed changes

bdraco force-pushed the nsec_address_denial branch 2 times, most recently from c9f2cbe to b9ff66e Compare August 28, 2026 18:54

bdraco force-pushed the nsec_address_denial branch from b9ff66e to 781b46c Compare August 28, 2026 18:56

bluetoothbot approved these changes Aug 28, 2026

View reviewed changes

fix: fail service resolution fast when nsec denies the needed address…

ac3afd2

… types

bdraco force-pushed the nsec_address_denial branch from 781b46c to ac3afd2 Compare August 28, 2026 19:12

bluetoothbot approved these changes Aug 28, 2026

View reviewed changes

address review nits: fix stale cython.locals and trim test docstring

0ffc2ed

bdraco merged commit 77f8cf0 into master Aug 28, 2026
34 checks passed

bdraco deleted the nsec_address_denial branch August 28, 2026 20:16

Uh oh!

Conversation

bdraco commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Test plan

Uh oh!

codecov Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

bluetoothbot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented Aug 28, 2026

Uh oh!

bluetoothbot commented Aug 28, 2026

Uh oh!

bluetoothbot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

bluetoothbot commented Aug 28, 2026

PR Review — fix: fail service resolution fast when nsec denies the needed address types

🟢 Suggestions

Checklist

Uh oh!

bluetoothbot left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdraco commented Aug 28, 2026 •

edited

Loading

codecov Bot commented Aug 28, 2026 •

edited

Loading

codspeed Bot commented Aug 28, 2026 •

edited

Loading

bluetoothbot commented Aug 28, 2026 •

edited

Loading

bluetoothbot commented Aug 28, 2026 •

edited

Loading