fix(pairing): listener reactivation, dead-trust recovery, and mDNS record lifecycle fixes#87
Open
kirollosnct wants to merge 4 commits into
Open
fix(pairing): listener reactivation, dead-trust recovery, and mDNS record lifecycle fixes#87kirollosnct wants to merge 4 commits into
kirollosnct wants to merge 4 commits into
Conversation
- mdns: handle _shippairing._tcp remove events before TXT validation; removals are keyed by the service instance name and often carry no TXT data (avahi echoes add-time elements only when its lookup hits, zeroconf reports incomplete records), so a withdrawn request stayed cached forever and was replayed to the listener on reactivation - zeroconf: forward pairing removals even when TXT data is missing, and log malformed TXT without suppressing the removal - hub: remove registry entries by exact entry (removeServiceEntry) instead of re-matching caller-supplied identifiers; lookups match loosely (normalized SKI, case-insensitive fingerprint, ShipID) while removal compared raw fields, so user trust removal could leave a stale entry that made every later re-pairing of the same SHIP ID with a rotated certificate fail as a fingerprint conflict - hub: harden removeService to mirror lookup semantics and refuse empty identifiers (previously deleted an arbitrary entry) - hub: replay cached pairing announcements after the listener is reactivated by trust removal (extracted processPendingPairingEntries from the replacement-timeout path) - hub: arm the AddCu replacement timer at trust establishment when no SHIP connection exists, so spec section 4.3 1.a recovery reactivates the listener if the accepted request was withdrawn before a connection could be established (observed when trust removal raced the withdrawal of a still-announced request)
The mDNS browse callback fires only once per service instance, so announcements that arrive while no listener is active are dropped and never re-delivered. Previously the hub compensated by replaying the record cache after each reactivation call site — a ritual that the startup path missed: a devZ announcement live before Start() was cached with no callback registered and never evaluated. Make the replay an intrinsic property of the state transition instead: StartListening registers the browse callback and then evaluates the currently-live records on its own goroutine (subscribe-then-snapshot, so no record can fall between the two deliveries; duplicates are benign as an accepted request stops the listener and a rejected one re-fails identically). A failed snapshot read keeps the subscription alive. Hub-side processPendingPairingEntries and both call sites are removed; ProcessPendingEntries stays on the interface but is deprecated. The RequestPairingEntries docs no longer claim a discovery scan is triggered — it returns a snapshot of the local browse cache and performs no network operation.
A re-report of a known service instance name was unconditionally ignored, so a corrected shippairing request reusing the same instance name (pairing spec §5.5 option 2) — or a re-announcement whose preceding goodbye was lost — was never evaluated. Per §4.2 devA rule 2 the evaluation must begin again in both cases. Compare the re-reported TXT content against the cached entry instead: identical content (TTL refresh) stays suppressed, changed content updates the cache and is re-delivered to the discovery callback. The listener's digest ring buffer still gates true duplicates. Also close the remaining coverage gaps: goodbye eviction removes only the matching cache entry, the §4.3 1.a replacement timer expiry reactivates the listener end to end, and acceptance while a SHIP connection already exists does not arm the recovery timer.
|
coverage: 93.039% (+0.07%) from 92.972% — fix/pairing-reactivate-listener-on-addCu-removal into dev |
kirollosnct
marked this pull request as ready for review
July 13, 2026 10:37
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
2 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 a set of SHIP Pairing Service (SPS) issues around addCu trust removal, listener reactivation, and the mDNS record lifecycle, found while running the SPS test spec (TC_SPS_PROC/TRUST series) against the DUT.
Per spec §4.3 item 2.b / §10.4 *3, user removal of the trusted addCu device must reactivate addCu-request processing immediately. UnregisterRemoteService now derives the pairing type from the stored entry (the caller-supplied identity may carry none), removes the exact registry entry by pointer instead of re-matching identifiers (fixes removals missing loosely-spelled identities), stops any pending replacement timer, and reactivates the pairing listener. ErrListenerAlreadyActive is treated as success.
The browse callback fires once per service instance; announcements received while no listener is active were dropped and never re-delivered. The hub previously replayed the record cache after each reactivation call site — and the startup path missed it (a devZ announcing before Start() was never evaluated). StartListening now registers the callback and then evaluates the currently-live records itself (subscribe-then-snapshot), making the replay intrinsic to every activation path. Hub-side replay plumbing is removed; ProcessPendingEntries is kept but deprecated; RequestPairingEntries docs corrected (it returns the local browse-cache snapshot, no network scan). Net −370 lines.
A re-report of a known instance name was unconditionally ignored, so a corrected request reusing the same instance name (§5.5 option 2) — or a re-announcement whose goodbye was lost — was never evaluated, violating §4.2 devA rule 2 + Note. Re-reports are now compared against the cached content: identical (TTL refresh) stays suppressed, changed content is re-delivered. The digest ring buffer still gates true duplicates.