Skip to content

Commit 7978c8a

Browse files
Apollon77claude
Apollon77
and
claude
authored
fix(mdns): DnssdNames discovery, staging, and TXT-parameter fixes (#3665)
* fix(mdns): run dependency pass even when filter accepts nothing DnssdNames.#processMessage iterates a "dependency pass" after the filter pass, attaching records whose names became known during message processing (e.g. AAAA for an SRV target the SRV record just registered). The loop guard `while (filteredBeforePass > filtered.size)` was checked before the first iteration with `filteredBeforePass = records.length` and `filtered.size = records.length`, so the body never ran for a packet whose filter pass deleted nothing. That's the common shape of a solicited A/AAAA-only response (e.g. the follow-up `IpService` issues for a Matter SRV target hostname). The records were dropped, `IpService.addresses` stayed empty, and `CommissionableMdnsScanner` never delivered the device — manifesting as intermittent commissioning failures (#524 on matterjs-server) where `avahi-browse` showed the AAAA but matter.js logged hasAddresses: false. Convert the loop to do/while so the dependency pass runs at least once. Add a regression test that sends PTR+SRV in one packet, then A+AAAA alone in a second, and asserts the records attach to the hostname. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(mdns): honour staged-record goodbyes outside the relevance gate Goodbye (ttl=0) handling for staged A/AAAA records lived inside the `if (packetRelevant)` block alongside staging, so a goodbye for a hostname whose earlier A/AAAA was staged was silently dropped when its packet carried nothing else we accept. The stale staged record then got replayed on the next SRV that creates the hostname dependency. Move goodbye eviction outside the gate and split staging-add from goodbye-evict. Eviction is safe to run unconditionally because it only operates on hostnames already present in `#stagedIpRecords` — entries we ourselves admitted under the gate — so a noisy goodbye cannot poison anything new. Also drop an inline comment that just restated `Timer.utility`'s own docstring, per the project's comment policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(mdns): clear stale TXT parameters when a record is removed or replaced DnssdName.parameters was populated eagerly during installRecord and never cleared when the underlying TXT record went away. A goodbye for the TXT (or a replacement TXT that omits a key) left zombie entries in the Map; downstream consumers reading `name.parameters` continued to see values from earlier broadcasts even after the source record had been removed. Make `parameters` a derived view of the current TXT records: recompute lazily from `#records` whenever the cache is invalidated, and invalidate on every TXT install/delete. Tighten the getter return type to `ReadonlyMap<string, string>` (and `DiscoveryData`'s parameter likewise) so callers can't poison the cache. Also fix `keyOf`'s in-place `record.value.sort()` for TXT records. The mutation was previously hidden because the eager extract in installRecord ran before keyOf and read wire order. Now that parameters recomputes on demand from `record.value`, the sort would silently reorder entries the next time the value array was iterated. Add regression tests: - "recomputes parameters when a TXT record is removed" - "drops keys absent from a replacement TXT record" Both attach an SRV alongside the TXT so the DnssdName itself is not collected on TXT removal — that would mask the parameters-cleanup behaviour. Update the "discovers" test fixture to reflect the wire-preserved TXT value order (was relying on the keyOf side effect). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ad7c28a commit 7978c8a

5 files changed

Lines changed: 339 additions & 39 deletions

File tree

‎packages/general/src/net/dns-sd/DnssdName.ts‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,22 @@ export class DnssdName extends BasicObservable<[changes: DnssdName.Changes], May
6666
return this.#records.values();
6767
}
6868

69-
get parameters() {
69+
get parameters(): ReadonlyMap<string, string> {
7070
if (this.#parameters === undefined) {
71-
this.#parameters = new Map();
71+
const parameters = (this.#parameters = new Map<string, string>());
72+
for (const record of this.#records.values()) {
73+
if (record.recordType !== DnsRecordType.TXT) {
74+
continue;
75+
}
76+
for (const entry of record.value as string[]) {
77+
const pos = entry.indexOf("=");
78+
if (pos === -1) {
79+
parameters.set(entry, "");
80+
} else {
81+
parameters.set(entry.slice(0, pos), entry.slice(pos + 1));
82+
}
83+
}
84+
}
7285
}
7386
return this.#parameters;
7487
}
@@ -78,19 +91,6 @@ export class DnssdName extends BasicObservable<[changes: DnssdName.Changes], May
7891
}
7992

8093
installRecord(record: DnsRecord<any>, options?: DnssdName.InstallOptions) {
81-
// For TXT records, extract the standard DNS-SD k/v's
82-
if (record.recordType === DnsRecordType.TXT) {
83-
const entries = record.value;
84-
for (const entry of entries) {
85-
const pos = entry.indexOf("=");
86-
if (pos === -1) {
87-
this.parameters.set(entry, "");
88-
} else {
89-
this.parameters.set(entry.slice(0, pos), entry.slice(pos + 1));
90-
}
91-
}
92-
}
93-
9494
const key = keyOf(record);
9595
if (key === undefined) {
9696
this.#deleteIfUnused();
@@ -117,6 +117,10 @@ export class DnssdName extends BasicObservable<[changes: DnssdName.Changes], May
117117

118118
this.#records.set(key, recordWithExpire);
119119

120+
if (record.recordType === DnsRecordType.TXT) {
121+
this.#parameters = undefined;
122+
}
123+
120124
this.#context.registerForExpiration(recordWithExpire);
121125

122126
// Keep hostname alive as long as any SRV references it
@@ -151,6 +155,10 @@ export class DnssdName extends BasicObservable<[changes: DnssdName.Changes], May
151155
this.#records.delete(key);
152156
this.#recordCount--;
153157

158+
if (record.recordType === DnsRecordType.TXT) {
159+
this.#parameters = undefined;
160+
}
161+
154162
const dependency = this.#dependencies?.get(key);
155163
if (dependency) {
156164
this.#dependencies!.delete(key);
@@ -251,7 +259,8 @@ function keyOf(record: DnsRecord): string | undefined {
251259

252260
case DnsRecordType.TXT:
253261
if (Array.isArray(record.value)) {
254-
return `${record.recordType} ${record.value.sort().join(" ")}`;
262+
// Spread before sort: keyOf must not mutate record.value (parameters recompute relies on wire order)
263+
return `${record.recordType} ${[...record.value].sort().join(" ")}`;
255264
}
256265
break;
257266
}

‎packages/general/src/net/dns-sd/DnssdNames.ts‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,6 @@ export class DnssdNames {
106106
Minutes(1),
107107
this.#pruneStagedIpRecords.bind(this),
108108
);
109-
// Mark as utility so the prune timer doesn't keep the Node event loop alive
110109
this.#stagedIpExpirationTimer.utility = true;
111110
this.#stagedIpExpirationTimer.start();
112111
}
@@ -213,9 +212,9 @@ export class DnssdNames {
213212
}
214213

215214
// Filtered records may be relevant to us if they are referenced by services, e.g. SRV targets become relevant.
216-
// So iteratively process until the set of filtered records does not change
217-
let filteredBeforePass = records.length;
218-
while (filteredBeforePass > filtered.size) {
215+
// Iteratively process until the set of filtered records does not change.
216+
let filteredBeforePass: number;
217+
do {
219218
filteredBeforePass = filtered.size;
220219
for (const record of filtered) {
221220
if (!this.has(record.name)) {
@@ -224,6 +223,31 @@ export class DnssdNames {
224223

225224
handleRecord(record);
226225
}
226+
} while (filteredBeforePass > filtered.size);
227+
228+
// Honour goodbye (ttl=0) for already-staged hostnames regardless of packetRelevant: eviction only
229+
// touches entries we previously admitted under the gate, so a stray goodbye cannot poison anything new.
230+
for (const record of filtered) {
231+
if (
232+
(record.recordType !== DnsRecordType.A && record.recordType !== DnsRecordType.AAAA) ||
233+
record.ttl !== 0 ||
234+
this.has(record.name)
235+
) {
236+
continue;
237+
}
238+
const key = record.name.toLowerCase();
239+
const staged = this.#stagedIpRecords.get(key);
240+
if (staged === undefined) {
241+
continue;
242+
}
243+
const remaining = staged.filter(
244+
s => !(s.record.recordType === record.recordType && s.record.value === record.value),
245+
);
246+
if (remaining.length === 0) {
247+
this.#stagedIpRecords.delete(key);
248+
} else {
249+
this.#stagedIpRecords.set(key, remaining);
250+
}
227251
}
228252

229253
// Stage A/AAAA for unknown hostnames — replayed when a later SRV creates the name.
@@ -232,28 +256,13 @@ export class DnssdNames {
232256
for (let record of filtered) {
233257
if (
234258
(record.recordType !== DnsRecordType.A && record.recordType !== DnsRecordType.AAAA) ||
259+
record.ttl === 0 ||
235260
this.has(record.name)
236261
) {
237262
continue;
238263
}
239264
const key = record.name.toLowerCase();
240265

241-
if (record.ttl === 0) {
242-
const staged = this.#stagedIpRecords.get(key);
243-
if (staged === undefined) {
244-
continue;
245-
}
246-
const remaining = staged.filter(
247-
s => !(s.record.recordType === record.recordType && s.record.value === record.value),
248-
);
249-
if (remaining.length === 0) {
250-
this.#stagedIpRecords.delete(key);
251-
} else {
252-
this.#stagedIpRecords.set(key, remaining);
253-
}
254-
continue;
255-
}
256-
257266
if (record.ttl < this.#minTtl) {
258267
record = { ...record, ttl: this.#minTtl };
259268
}

‎packages/general/src/net/dns-sd/IpService.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ export class IpService {
9595
/**
9696
* Values from TXT records.
9797
*/
98-
get parameters() {
98+
get parameters(): ReadonlyMap<string, string> {
9999
return this.#name.parameters;
100100
}
101101

0 commit comments

Comments
 (0)