Repository navigation
Report every browsed instance on macOS - #100
Merged
Merged
Conversation
The dns_sd backend reported an instance from its address callback only when kDNSServiceFlagsMoreComing was clear. All operations share one connection, and on a shared connection that flag covers the replies of every operation. So when an instance's last address came with the flag set, and another operation's reply ended the batch, the instance was never reported and the server never dialed it. On a network with five `_sendspin._tcp` speakers, the server reported three on every start. The address callback now marks the instance as updated. The run loop reports the updated instances after it handles a reply and no other reply is left to read on the connection. The host tests never use the system's mDNS, so they cannot reach this backend. Checked against the five speakers: all five are reported on each of three starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Existing tests bypass macOS dns_sd shared-socket draining and batching, so a macOS smoke test is needed to verify that all five speakers are reported and dialed despite interleaved replies.
No open issues 🎉
What changed in this PR
Fixes macOS mDNS discovery so browsed Sendspin instances are not missed when replies from multiple operations share a batch.
Changes:
- Tracks instances with address updates and reports them once the shared connection has no pending replies.
- Removes pending updates when instances disappear and clears them during disconnect and cleanup.
| File | Description |
|---|---|
src/discovery/dns_sd_discovery.cpp |
Batches instance reports across shared-connection replies and manages pending-update state. |
Review effort: Balanced
kahrendt
added a commit
that referenced
this pull request
Oct 28, 2026
…102) ## Summary `TransportTest.HeldBackConnectionsResumeInTheOrderTheyWereHeld` was flaky. It failed on the Linux TSan job of #100 with `order` holding only the five "first" messages. The test raced the network thread in two places. This PR changes only the test. ## Change - **The first connection's close.** The first connection's close is raised outside the inbound ring's room, so it can reach core while later connections are still parked. The test then closed them and their messages never arrived. It now also waits until every expected message has been delivered before closing the later connections. - **The last later message.** `wait_taken()` shows the kernel bytes were read, not that the push was refused. The last later message could therefore be pushed once core freed room, ahead of the parked ones (`order` showed "later 8" before the first's held message). A probe connection that core closes after that message is taken now ends only after the network thread has pushed it. The test waits for that before core runs. ## Tests - No new tests. - Swapping two entries of the expected order still fails the assertion. - Known gap: the stress runs were on macOS; the original failure was on Linux.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On macOS, the dns_sd backend dropped browsed instances. On the shared connection,
kDNSServiceFlagsMoreComingcovers every operation's replies, so an instance whose last address arrived with it set was never reported or dialed. Updated instances are now reported once no reply is left to read.dns-sd -B _sendspin._tcplists five speakers:ServerListener::on_client_seen()reports three of them on every start:Tests
None: the host tests never use the system's mDNS, so they cannot reach this backend.
🤖 Generated with Claude Code