fix: skip NSEC records in ServiceBrowser to suppress spurious updates by bluetoothbot · Pull Request #1762 · python-zeroconf/python-zeroconf

bluetoothbot · 2026-05-20T16:18:14Z

Summary

NSEC records assert non-existence of a record type (RFC 6762 §6.1), not a state change. ServiceBrowser.async_update_records was firing ServiceStateChange.Updated for every NSEC arriving on a known service, producing the "Updated non address record" warnings reported in #1328 and triggering unnecessary update_service callbacks.

Fixes #1328

Changes

Skip records with type == _TYPE_NSEC in ServiceBrowser.async_update_records before the generic non-address branch enqueues an update.
Add cdef cython.uint _TYPE_NSEC to browser.pxd so the hot-path comparison stays a C int compare under Cython.
New regression test test_service_browser_nsec_record_does_not_trigger_update injects an NSEC for a known service and asserts only the original add_service callback fires.

Test plan

New test fails on master, passes with the fix applied.
poetry run pytest --timeout=60 -q tests — 386 passed, 2 skipped.
poetry run ruff check / ruff format --check clean on touched files.

Generated by Kōan /fix

Quality Report

Changes: 3 files changed, 69 insertions(+)

Code scan: clean

Tests: passed (4 PASSED)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

codecov · 2026-05-20T16:20:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.77%. Comparing base (4ffba87) to head (8fe58dd).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1762   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          33       33           
  Lines        3509     3511    +2     
  Branches      493      494    +1     
=======================================
+ Hits         3501     3503    +2     
  Misses          5        5           
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codspeed-hq · 2026-05-20T16:21:14Z

Merging this PR will not alter performance

✅ 14 untouched benchmarks

_{Comparing bluetoothbot:koan/fix-issue-1328 (8fe58dd) with master (4ffba87)}

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR prevents ServiceBrowser from emitting spurious ServiceStateChange.Updated events when NSEC records arrive for already-known services (per RFC 6762 §6.1), and adds a regression test to ensure NSEC does not trigger update callbacks.

Changes:

Ignore _TYPE_NSEC records in ServiceBrowser.async_update_records to avoid enqueueing non-address updates.
Add a Cython-typed _TYPE_NSEC declaration for fast comparisons in the browser hot path.
Add a regression test asserting that injecting an NSEC record does not produce an update_service callback.

Reviewed changes

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

File	Description
tests/services/test_browser.py	Adds regression coverage to ensure NSEC does not produce update callbacks
src/zeroconf/_services/browser.py	Skips NSEC records during record processing to prevent spurious updates
src/zeroconf/_services/browser.pxd	Declares `_TYPE_NSEC` as `cython.uint` for efficient comparisons

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

bdraco · 2026-05-20T19:18:44Z

@bluetoothbot rr

bdraco

remove the fixed time.sleep and use proper sync

bdraco · 2026-05-20T19:31:17Z

@bluetoothbot rr

bluetoothbot · 2026-05-20T19:51:11Z

PR Review — fix: skip NSEC records in ServiceBrowser to suppress spurious updates

Tight, well-targeted fix: skip NSEC in ServiceBrowser.async_update_records so non-existence assertions (RFC 6762 §6.1) don't masquerade as service updates. The cdef cython.uint _TYPE_NSEC declaration in browser.pxd correctly preserves the C-int hot-path compare, matching the existing _TYPE_PTR pattern. The switch from is to == resolves the prior Copilot comment cleanly with no perf cost under Cython. The regression test now uses Event.wait() and try/finally, addressing @bdraco's earlier feedback on the previous iteration. Only nits: the IPV6_LOOPBACK_FIND_TIMEOUT PyPy bump in tests/init.py is unrelated scope creep that would be cleaner as its own PR, and the test could note why the bundled insert doesn't race against an Updated callback. None of these block merge.

🟢 Suggestions

1. Unrelated change to PyPy IPv6 timeout (`tests/__init__.py`, L66)

The bump from 0.5 to 1.0 if _IS_PYPY else 0.5 for IPV6_LOOPBACK_FIND_TIMEOUT is unrelated to the NSEC ServiceBrowser fix. Bundling unrelated CI-stability tweaks in a fix: PR makes the changelog less precise and complicates a future bisect: if the PyPy IPv6 path regresses (or unmasks a real bug because the timeout was too short), the commit looks like an NSEC fix in git log. Worth pulling out into a small test: or ci: PR with its own rationale (e.g., a CI log link showing the flake) so the NSEC PR stays focused on one behavioural change.

IPV6_LOOPBACK_FIND_TIMEOUT = 1.0 if _IS_PYPY else 0.5

2. Document or assert the assumption that the bundled insert does not race with an Updated callback (`tests/services/test_browser.py`, L1138-1153)

The test relies on a non-obvious property: a single multicast packet carrying PTR + SRV + TXT + addresses for a brand-new service produces exactly one add_service callback and zero update_service callbacks. That's the current ServiceBrowser behaviour, but the test would silently start failing on master (and so would mask any regression of the NSEC fix) the day someone changes the scheduler to coalesce/emit an update for the SRV+TXT pair of a freshly-added service.

Two low-cost hardening options:

Add a brief comment in the test explaining why the assertion callbacks == [("add", ...)] is valid even though SRV/TXT/A/AAAA arrived alongside the PTR (i.e., the address/SRV/TXT branch suppresses Updated when the service is being added in the same dispatch).
Or, between the bundled inject and the NSEC inject, give the loop one explicit tick (e.g., a short time.sleep(0) equivalent via the existing test helpers) and re-check callbacks to confirm no Updated arrived from the bundle, separating that pre-condition from the NSEC assertion.

Neither blocks merge — just makes the failure mode more diagnostic if the surrounding behaviour ever shifts.

        # NSEC inject runs synchronously through the event loop; once
        # _inject_response returns, async_update_records has already
        # decided not to enqueue a callback for the NSEC record.
        _inject_response(
            zc,
            mock_incoming_msg(
                [
                    r.DNSNsec(
                        registration_name,
                        const._TYPE_NSEC,
                        const._CLASS_IN | const._CLASS_UNIQUE,
                        const._DNS_OTHER_TTL,
                        registration_name,
                        [const._TYPE_AAAA],
                    ),
                ]
            ),
        )

        assert callbacks == [("add", type_, registration_name)]

Checklist

Fix matches issue description (NSEC no longer fires Updated)
Cython .pxd updated in same commit as .py constant addition
Hot-path comparison stays a C int compare under Cython
Test addresses previous reviewer feedback (Event + try/finally)
Docstring and comment style match CLAUDE.md (terse, RFC-cited)
PR scope is single-purpose — suggestion ＃1
Free-threaded (3.14t) safety: no new module-level mutable state
No Co-Authored-By agent trailer in commits

Summary

Tight, well-targeted fix: skip NSEC in ServiceBrowser.async_update_records so non-existence assertions (RFC 6762 §6.1) don't masquerade as service updates. The cdef cython.uint _TYPE_NSEC declaration in browser.pxd correctly preserves the C-int hot-path compare, matching the existing _TYPE_PTR pattern. The switch from is to == resolves the prior Copilot comment cleanly with no perf cost under Cython. The regression test now uses Event.wait() and try/finally, addressing @bdraco's earlier feedback on the previous iteration. Only nits: the IPV6_LOOPBACK_FIND_TIMEOUT PyPy bump in tests/init.py is unrelated scope creep that would be cleaner as its own PR, and the test could note why the bundled insert doesn't race against an Updated callback. None of these block merge.

Automated review by Kōan89bf71b
2319c03
47f97d2
f47827a

bluetoothbot · 2026-05-20T19:54:50Z

Rebase with requested adjustments

Branch koan/fix-issue-1328 was rebased onto master and review feedback was applied.

Changes applied

applied.
Summary
Replaced fixed time.sleep(0.2) delays with deterministic sync in test_service_browser_nsec_record_does_not_trigger_update (per @bdraco's remove the fixed time.sleep and use proper sync and Copilot's flakiness comments at lines 1126/1143):
Added service_added = Event() set inside add_service.
First time.sleep → assert service_added.wait(timeout=5).
Second time.sleep removed: _inject_response synchronously drives async_update_records on the loop, and NSEC is skipped before any callback is enqueued, so no further wait is needed. A short comment notes the invariant.
Wrapped browser/zc lifecycle in try/finally (per Copilot comment at line 1147) so browser.cancel() and zc.close() always run even when an assertion fails.
No change to browser.py: the existing diff already converted record_type is _TYPE_PTR to record_type == _TYPE_PTR, so Copilot's is/== consistency concern is already satisfied in this PR.

Stats

3 files changed, 75 insertions(+), 1 deletion(-)

Actions performed

Already-solved check: negative (confidence=high, reasoning=No recent master commit addresses NSEC records triggering spurious ServiceBrowser updates; closest N)
Rebased koan/fix-issue-1328 onto upstream/master
Applied review feedback
Pre-push CI check: previous run passed
Force-pushed koan/fix-issue-1328 to origin
CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

Automated by Kōan

bdraco · 2026-05-20T20:07:01Z

@bluetoothbot rr

NSEC records assert the non-existence of a record type (RFC 6762 §6.1), not a state change. The browser was firing ServiceStateChange.Updated for every NSEC record arriving for a known service, surfacing as "Updated non address record" warnings and unnecessary update_service callbacks. Skip them in async_update_records.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

bluetoothbot · 2026-05-20T21:27:19Z

Rebase with requested adjustments

Branch koan/fix-issue-1328 was rebased onto master and review feedback was applied.

Stats

4 files changed, 76 insertions(+), 2 deletions(-)

Actions performed

Already-solved check: negative (confidence=high, reasoning=No recent master commit addresses NSEC records triggering spurious ServiceBrowser updates; recent fi)
Rebased koan/fix-issue-1328 onto upstream/master
Pre-push CI check: previous run passed
Force-pushed koan/fix-issue-1328 to origin
CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

Automated by Kōan

bdraco requested a review from Copilot May 20, 2026 16:18

bdraco added the traffic-reduction label May 20, 2026

bluetoothbot temporarily deployed to release May 20, 2026 16:24 — with GitHub Actions Inactive

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread tests/services/test_browser.py Outdated

Comment thread tests/services/test_browser.py Outdated

Comment thread tests/services/test_browser.py Outdated

Comment thread src/zeroconf/_services/browser.py Outdated

Copilot started reviewing on behalf of bdraco May 20, 2026 16:32 View session

bdraco temporarily deployed to release May 20, 2026 19:28 — with GitHub Actions Inactive

bdraco requested changes May 20, 2026

View reviewed changes

bluetoothbot force-pushed the koan/fix-issue-1328 branch from d90444c to 47f97d2 Compare May 20, 2026 19:54

bdraco approved these changes May 20, 2026

View reviewed changes

bdraco marked this pull request as ready for review May 20, 2026 19:56

bluetoothbot added a commit to bluetoothbot/python-zeroconf that referenced this pull request May 20, 2026

fix: resolve CI failures on python-zeroconf#1762 (attempt 1)

f47827a

bluetoothbot temporarily deployed to release May 20, 2026 20:11 — with GitHub Actions Inactive

bluetoothbot and others added 4 commits May 20, 2026 21:26

Potential fix for pull request finding

8b50f14

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

test: replace time.sleep with deterministic sync in NSEC browser test

0f7b6a1

fix: resolve CI failures on python-zeroconf#1762 (attempt 1)

5557174

bluetoothbot force-pushed the koan/fix-issue-1328 branch from f47827a to 5557174 Compare May 20, 2026 21:27

bluetoothbot temporarily deployed to release May 20, 2026 21:30 — with GitHub Actions Inactive

bdraco reviewed May 20, 2026

View reviewed changes

Comment thread tests/__init__.py Outdated

Apply suggestion from @bdraco

8fe58dd

bdraco temporarily deployed to release May 20, 2026 22:39 — with GitHub Actions Inactive

bdraco merged commit 137a5d6 into python-zeroconf:master May 20, 2026
36 checks passed

bluetoothbot deleted the koan/fix-issue-1328 branch May 20, 2026 23:44

Conversation

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

Uh oh!

Summary

Changes

Test plan

Quality Report

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

codspeed-hq Bot commented May 20, 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!

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

bdraco commented May 20, 2026

Uh oh!

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

Uh oh!

PR Review — fix: skip NSEC records in ServiceBrowser to suppress spurious updates

🟢 Suggestions

Checklist

Summary

Uh oh!

bluetoothbot commented May 20, 2026

Rebase with requested adjustments

Changes applied

Summary

Stats

CI status

Uh oh!

bdraco commented May 20, 2026

Uh oh!

bluetoothbot commented May 20, 2026

Rebase with requested adjustments

Stats

CI status

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bluetoothbot commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

bluetoothbot commented May 20, 2026 •

edited

Loading