Merging this PR will not alter performance

✅ 6 untouched benchmarks

Comparing bluetoothbot:koan/fix-fd-leak-on-close (7da73d8) with master (277f80d)

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.76%. Comparing base (277f80d) to head (7da73d8).
⚠️ Report is 101 commits behind head on master.

@@           Coverage Diff           @@
##           master    #1685   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files          33       33           
  Lines        3401     3407    +6     
  Branches      461      463    +2     
=======================================
+ Hits         3393     3399    +6     
  Misses          5        5           
  Partials        3        3           

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

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

@bluetoothbot rr

PR Review — fix(core): close owned event loop on Zeroconf.close() to stop FD leak

The fix itself is correct and well-scoped: closing the owned loop after the thread joins plugs the FD leak, the idempotency guard preserves the documented Zeroconf.close() contract, and the browser-cancel guard handles the close-after-cancel race. The new tests fail on master and pass on this branch, which is exactly the right shape for a regression test. Before merging, please address @bdraco's contextlib.suppress request in browser.py:803-806 and reword the misleading "nothing left to do" comment that the Copilot bot flagged. The other items (docstring trim, test threshold) are stylistic suggestions, not blockers.

🟢 Suggestions

@bdraco requested with contextlib.suppress(RuntimeError): here. It's the project's preferred idiom for narrow exception-swallow blocks and reads more clearly than try/except: pass — the intent ("close-loop race is expected, ignore it") is right there in the with line. Suggested change:

with contextlib.suppress(RuntimeError):
    if not self.zc.loop.is_closed():
        self.zc.loop.call_soon_threadsafe(self._async_cancel)

Add import contextlib at the top of the module if it's not already imported.

try:
    if not self.zc.loop.is_closed():
        self.zc.loop.call_soon_threadsafe(self._async_cancel)
except RuntimeError:
    pass

The comment claims _async_cancel had "nothing left to do anyway" once Zeroconf.close() detaches the browser. That's misleading — _async_cancel also stops the query scheduler and cancels the query-sender task, which is the normal cleanup path while the loop is running. The skip only happens when the loop is already closed, where scheduling is impossible regardless.

Suggested rewording, dropping the inaccurate "nothing to do" claim:

# Skip scheduling _async_cancel when the loop is already closed:
# call_soon_threadsafe would raise RuntimeError. The normal path
# (loop running) still uses _async_cancel to stop the query
# scheduler and cancel the query-sender task. is_closed() is racy
# against a concurrent Zeroconf.close(), so suppress RuntimeError
# from call_soon_threadsafe as well.

Per the project's docstring/comment guidelines, this is one of the cases where a comment is justified (subtle race, RFC-style invariant), so a precise wording matters.

Per CLAUDE.md's commenting bar, this 3-line comment restates what the early-return + the Zeroconf.close() docstring (one frame up the call stack) already say. A future reader looking at if self.loop.is_closed(): return will infer "second call is a no-op" from the code itself. Consider dropping the comment (or trimming to a single line, e.g. # Second close() — loop already torn down.). The self.loop.close() block at the bottom already has a useful comment that explains the why (selector + self-pipe FDs); that one is worth keeping.

if self.loop.is_closed():
    # close() is documented as idempotent — a second call after the
    # loop has been torn down must be a no-op rather than raising.
    return

CLAUDE.md explicitly calls this out: "A test docstring should name what the test pins, in one sentence — not re-explain the bug, the fix, or the surrounding flow." The current docstrings describe the FD-leak mechanism (selector, self-pipe sockets, FD limit exhaustion) — that's PR-description / commit-message territory.

Suggested trims:

def test_close_releases_owned_event_loop(self):
    """Zeroconf.close() closes the event loop it started itself."""

def test_close_does_not_leak_file_descriptors(self):
    """Repeated Zeroconf()/close() does not grow the process FD table."""

The issue-number cross-reference also belongs in the PR body, not the test docstring (per the same section of CLAUDE.md).

The threshold _fd_count() - baseline < 10 over 10 cycles allows ~1 FD growth per cycle, which is a lot of slack relative to the bug (~3 FDs per cycle = 30 over 10). If coverage/test harness FDs are the concern, the warm-up cycle already absorbs most one-shots before baseline is captured. A tighter bound like < 5 would still tolerate harness noise but would catch a partial regression (e.g. someone reintroduces just the epoll FD leak but not the self-pipe pair). Optional — current threshold does catch the original bug.

assert _fd_count() - baseline < 10

Checklist

• Resource cleanup in error paths (FDs, sockets, loop)
• Idempotency of public close()
• Race-condition handling (cancel vs. close)
• No bare except / no silent error swallowing
• Test isolation (per-pid /proc, warm-up cycle)
• Test docstrings describe behaviour, not implementation — suggestion ＃4
• Comments justified per project bar — suggestion ＃2, ＃3
• Skip guards for non-Linux / restricted-container CI
• Cython .pxd updates not required (no cdef-class layout change)
• Conventional Commits subject (fix(core): …)
• No Co-Authored-By trailers from automated agents

Summary

The fix itself is correct and well-scoped: closing the owned loop after the thread joins plugs the FD leak, the idempotency guard preserves the documented Zeroconf.close() contract, and the browser-cancel guard handles the close-after-cancel race. The new tests fail on master and pass on this branch, which is exactly the right shape for a regression test. Before merging, please address @bdraco's contextlib.suppress request in browser.py:803-806 and reword the misleading "nothing left to do" comment that the Copilot bot flagged. The other items (docstring trim, test threshold) are stylistic suggestions, not blockers.

Automated review by Kōan8390d25
4351166
4aa37f7

Rebase with requested adjustments

Branch koan/fix-fd-leak-on-close was rebased onto master and review feedback was applied.

Changes applied

• Reworded comment in src/zeroconf/_services/browser.py per Copilot reviewer.
• Comment now states we skip call_soon_threadsafe because the loop is closed (not "nothing to do"), and notes that _async_cancel normally stops the query scheduler and cancels the query-sender task while the loop runs.

Stats

3 files changed, 51 insertions(+), 2 deletions(-)

• Already-solved check: negative (confidence=high, reasoning=No recent commit on master addresses FD leak from unclosed event loop in Zeroconf.close().)
• Rebased koan/fix-fd-leak-on-close onto upstream/master
• Applied review feedback
• Pre-push CI check: previous run passed
• Force-pushed koan/fix-fd-leak-on-close to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

Automated by Kōan

@bluetoothbot rr

Rebase with requested adjustments

Branch koan/fix-fd-leak-on-close was rebased onto master and review feedback was applied.

Changes applied

• Done.

• Summary

• src/zeroconf/_services/browser.py — Reworded cancel() comment per Copilot review: emphasizes skip happens because loop closed, and that _async_cancel (stops query scheduler, cancels query-sender task) is normal cleanup path while loop runs. Wrapped call_soon_threadsafe in try/except RuntimeError to close TOCTOU race between is_closed() check and the call when cancel() runs concurrently with Zeroconf.close().
• tests/test_core.py — Added second skipUnless guarding /proc/<pid>/fd directory existence per Copilot review (restricted containers may lack /proc mount). Reduced FD-leak loop from 20 to 10 cycles per Copilot review to cut CI runtime; threshold still rejects ~3-FDs-per-cycle regression (>=30 over 10 cycles).

Stats

3 files changed, 56 insertions(+), 2 deletions(-)

• Already-solved check: negative (confidence=high, reasoning=No commit on master addresses FD leak from event loop not being closed in Zeroconf._shutdown_threads)
• Rebased koan/fix-fd-leak-on-close onto upstream/master
• Applied review feedback
• Pre-push CI check: previous run passed
• Force-pushed koan/fix-fd-leak-on-close to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

Automated by Kōan

@bluetoothbot rr

Rebase with requested adjustments

Branch koan/fix-fd-leak-on-close was rebased onto master and review feedback was applied.

Changes applied

• Changes:
• src/zeroconf/_services/browser.py: Replaced try/except RuntimeError: pass with contextlib.suppress(RuntimeError) per @bdraco and @bluetoothbot. Added import contextlib. Reworded comment per @Copilot to lead with what _async_cancel normally does and frame the skip as "solely because loop is closed" rather than "nothing left to do".
• Other Copilot review points (FD-dir skip guard, iteration-count reduction) already present in prior commit (@unittest.skipUnless(Path(f"/proc/{os.getpid()}/fd").is_dir(), ...), range(10)).

Stats

3 files changed, 56 insertions(+), 2 deletions(-)

• Already-solved check: negative (confidence=high, reasoning=No recent master commits address FD leak on Zeroconf.close() or call loop.close() on owned event loo)
• Rebased koan/fix-fd-leak-on-close onto upstream/master
• Applied review feedback
• Pre-push CI check: previous run passed
• Force-pushed koan/fix-fd-leak-on-close to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

Automated by Kōan