fix: keep re-querying unresolved instances while browsing (#493) - #494
Open
keepsimple1 wants to merge 4 commits into
Open
fix: keep re-querying unresolved instances while browsing (#493)#494keepsimple1 wants to merge 4 commits into
keepsimple1 wants to merge 4 commits into
Conversation
When a service type has many instances, ServiceFound fires for all of them immediately but many were never resolved: the per-instance Command::Resolve retry chain gives up after max_try (3) tries, and because the instance stayed in `pending_resolves`, add_pending_resolve would never re-arm it. The only remaining resolution path was an unsolicited/browse-elicited cache update, which trickles in about one instance per query cycle on a lossy or IPv6-only network. Instances whose SRV/address answers were lost during those first ~1.5s could stay unresolved indefinitely. Fix: drive follow-up SRV/address queries off the browse retransmission cycle. `query_unresolved_instances` sweeps the PTR cache for the type (minus already-resolved instances) and re-queries each, so a pending instance keeps being queried for as long as the browse is active. This inherits the browse's exponential backoff, so it never stalls yet also never becomes a fixed-interval query storm. Also clear `pending_resolves` when the fast-path Resolve chain ends (resolved or gave up), so the set honestly means "a Resolve chain is in flight" and the fast path can re-arm if new partial records arrive. Adds test_unresolved_instance_not_stranded: a hand-rolled responder announces PTR+SRV with a separate SRV target hostname, withholds the address, and only answers after more than max_try address queries have arrived. Before the fix the daemon goes silent after 3 tries and the instance is never resolved; after the fix the browse cycle keeps querying and it resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the quick turnaround — the diagnosis matches what we saw exactly (resolutions arriving right after a What I can and can't give you: I don't have the failing network. The report came from a downstream user of ours (phunapps/matter-rust#113) whose LAN has ~18 instances of one service type; on mine everything resolves in well under a second, so I can only offer no-regression evidence, not confirmation of the fix. He's away until next week and I've asked him to build against the branch — his run is the one that actually tests it. Tested
|
| unpatched | fix/issue-493 |
|
|---|---|---|
| base-type browse, 15 s | 16 resolved | 16 resolved |
| subtype browse alone, 12 s | 1 resolved | 1 resolved |
| subtype + base concurrently | 1 / 16 | 1 / 16 |
Also exercised through our full stack rather than just a probe: a real Matter controller resolving an operational record and reconnecting to a Thread device, then a 204-attribute read over that session — both fine.
So: no regression observed, and nothing got slower.
Two incidental notes that might be useful:
- I had to bump our requirement from
0.20to0.21for the[patch.crates-io]to apply, so this also exercised 0.21 against our adapter. It compiled and behaved identically — no changes needed on our side for the packet-size or re-exported-error-type changes. - One thing we discovered while working around this, in case it's relevant to the fix's shape: browsing a subtype resolves promptly on the affected network (~266 ms for 3 instances) while the base type stalls, but opening both concurrently re-stalls it — consistent with the bottleneck being per-instance resolution work shared across browses rather than anything per-browse. If your fix makes that distinction disappear, that'd be a good confirmation signal.
Happy to run anything else here, and I'll relay the result from the affected network as soon as I have it.
|
This doesn't seem to fix the issue for me; the services still aren't resolved. Output from the example with this patch: |
Owner
Author
|
@qwandor Thanks for running the test! I couldn't pin down what was not working. I've updated the diff to have a better backoff timer for the fast path query. Could you please try it again with the latest diff and also enable debugs? something like: |
|
Sure, here you go, on b255dde: |
|
I dug through @qwandor's debug log against the 0.21.0 source, and I don't think this is a pacing problem — which would explain why the improved backoff didn't move it. What the log showsCounting his run: 6 instances resolved, 17 did not. Every one of the 6 resolved inside the first 350 ms. The run then continued for 63 seconds through browse re-queries at 1 s, 3 s, 7 s, 15 s and 31 s, and produced zero further resolutions. So it isn't "one per cycle" — it's a fast initial batch and then a hard stop. The repeated line for the other 17 is pub fn is_valid(&self) -> bool {
let some_missing = self.ty_domain.is_empty()
|| self.fullname.is_empty()
|| self.host.is_empty()
|| self.addresses.is_empty();
!some_missing
}Those instances have PTR, SRV and TXT — hostname and port are present. What they lack is A/AAAA for the SRV target host. Why they never recover
let max_try = 3;at let instance = dns_ptr.alias();
if !updated_instances.contains(instance) {
continue;
}and Net effect: once an instance is found but its address records don't arrive within the 3-try window, it is stuck for the lifetime of the browse, no matter how many times the PTR is re-queried. That matches the log exactly — 17 stuck, 63 s, 5 re-queries, nothing. It isn't random which ones stallGrouping the instances by fabric prefix, the split is completely stable across the run:
So it's specific hosts whose address records aren't landing, not round-robin starvation across the set. (The SuggestionTrack found-but-address-less instances per active browse and re-issue A/AAAA for their SRV targets on the browse's own re-query schedule, decoupled from Happy to test any diff — and I can reproduce something adjacent here (16 instances, mixed IPv4/IPv6 plus IPv6-only Thread devices), though my link resolves everything promptly, so @qwandor's network remains the real test. For what it's worth downstream: we worked around this by browsing the Matter compressed-fabric subtype rather than the base type, which narrows the query from ~18 instances to 3 and resolves in ~266 ms on his network. That sidesteps the stall rather than fixing it, and it's consistent with the above — fewer instances, so the address queries land inside the retry window. |
Owner
Author
|
Thanks for the logs!
It's not in 0.21.0, but the latest diff in this PR is doing that. I've also added two more log lines and updated the diff. Would you mind try this PR diff out? thanks! |
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
3 participants
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.
Fixes #493
Suspected cause:
Resolution of a found instance is driven by a per-instance
Command::Resolveretry chain that fires every 500 ms but gives up after max_try (3) tries (~1.5 s). The only remaining path to resolution is an unsolicited or browse-elicited cache update.Fix:
Drive follow-up SRV/address queries off the
browseretransmission cycle instead of relying solely on the fast-path chain:Also clear
pending_resolveswhen the fast-path Resolve chain ends (resolved or gave up), so the set honestly means "a Resolve chain is in flight" and the fast path can re-arm if new partial records arrive.