nest: close PeerConnection/RTSP client on failed dial (fd + ICE mDNS socket leak) - #2378
Open
ajplotkin wants to merge 1 commit into
Open
nest: close PeerConnection/RTSP client on failed dial (fd + ICE mDNS socket leak)#2378ajplotkin wants to merge 1 commit into
ajplotkin wants to merge 1 commit into
Conversation
rtcConn creates a PeerConnection per dial attempt but never closes it on any error path. A failed dial leaves the pc unclosed, and because it never reaches ICE connectivity (no answer is set), pion's connection-state auto-close never fires either. Each abandoned pc keeps its ICE agent's UDP sockets open, including two mDNS sockets on :5353 (udp4+udp6, from pion's default MulticastDNSModeQueryOnly), plus its gathered host-candidate sockets and agent goroutines. On a host that repeatedly fails to dial a camera (e.g. a powered-off Nest camera returning HTTP 400 on every reconnect), these accumulate without bound. The retry loop makes it worse: it continues after a failed ExchangeSDP without closing the previous pc, leaking up to 3 per cycle. Observed 124 open :5353 sockets in one go2rtc process after ~40 min on a --network host deployment, which then degraded host mDNS discovery. rtspConn has the analogous bug: if Describe() fails after Dial() succeeds, the RTSP client's TCP connection is never closed. Close the connection on every failure path. Success paths are unchanged; there the returned client owns the connection and closes it on teardown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ajplotkin
added a commit
to ajplotkin/go2rtc
that referenced
this pull request
Jul 26, 2026
rtcConn/rtspConn never closed the connection on error paths. A failed dial (e.g. a powered-off Nest camera returning HTTP 400) leaked the PeerConnection, which never reaches ICE connectivity so pion's auto-close never fires — each abandoned pc kept its ICE agent's mDNS socket (:5353) open. Observed 142 leaked :5353 sockets in one go2rtc process on the Pi; they starve host mDNS and break HomeKit discovery. Deployed to the Pi as go2rtc-nestfix:v5-20260726 (this fix on 43fc3b3): post-fix, 4 failed off-camera dials left the :5353 count flat at 2. Upstream PR: AlexxIT#2378. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ajplotkin
added a commit
to ajplotkin/nest-homekit-cameras
that referenced
this pull request
Jul 27, 2026
New fork tag nestfix-1.9.14-2 adds the two fixes also filed upstream: - AlexxIT/go2rtc#2378 - rtcConn never closed the PeerConnection on any error path, so a failed dial leaked it along with its ICE agent's mDNS :5353 sockets. A powered-off camera retrying every ~2 min accumulated 142 leaked sockets, which starved host mDNS and left every HomeKit accessory at "No Response". Verified 142 -> 2, flat across 16 failed dials. - AlexxIT/go2rtc#2380 - RTPDepay fed pion whatever packet arrived first, so a consumer attaching mid-fragmented-NAL got orphan tail fragments assembled under a synthesized NAL header: a head-truncated keyframe that passes IsKeyframe but cannot decode. /api/frame.jpeg builds a fresh depayloader per request, so this produced intermittent 500s and grey snapshots. Also surfaces ffmpeg's stderr on a failed transcode instead of a bare "exit status N", which is what made the second bug diagnosable at all. patches/go2rtc-nest.patch regenerated against the new tag and verified: it applies cleanly to a pristine v1.9.14 checkout and the result builds. The adopted community PRs (#2351 tillo, #2194/#2193 MechanicalCoderX, #2327 zephleggett) are unchanged and still credited in the header and README. The tag deliberately excludes the drought-watchdog hardening on the dev branch, which is not running in production and is unvalidated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
pkg/nest/client.goleaks connections on error paths:rtcConn()creates awebrtc.PeerConnectionper dial attempt but never callspc.Close()on any failure. Each abandoned pc keeps its ICE agent's UDP sockets open — including two mDNS sockets on:5353(one udp4, one udp6, opened by pion's defaultMulticastDNSModeQueryOnly), plus its gathered host-candidate sockets and agent goroutines. These are never reclaimed on their own: the pc never reaches ICE connectivity (no answer is set on the failure path), so pion's connection-state auto-close never fires. Accumulation is unbounded.rtspConn()has the analogous bug: ifDescribe()fails afterDial()succeeds, the RTSP client's TCP connection is never closed.Impact (observed)
--network host, Nest WEB_RTC cameras via Device Access / SDM.wrong status: 400 Bad Requestfrom SDM.:5353owned by the single go2rtc process, climbing steadily. That matches the leak arithmetic — a persistently-failing dial leaks 3 PeerConnections per cycle × 2 mDNS sockets = 6/cycle, and ~20 dial cycles across the off cameras in ~40 min ≈ 120 sockets.--network hostalongside other mDNS responders, local mDNS discovery then degraded. (The socket accumulation and its cause are confirmed from source; the downstream "degrades host mDNS" step is an observed correlation on that host.)Root cause
In
rtcConn's retry loop,pcis created and then abandoned on every error path — none closes it. TheExchangeSDPfailure path is the worst: itcontinues the loop, which creates a new pc next iteration while the previous one leaks.Fix
Close the connection on each failure path. Success paths are unchanged — there the returned
WebRTCClient/RTSPClientowns the connection and closes it on teardown (Stop()→Connection.Stop()closes the pc).pc.Close()is idempotent and non-blocking here, and reclaims the whole per-pc fd/goroutine footprint, not just the mDNS sockets.Notes
400up to 3× with backoff; for a persistently-off camera that's wasted work and triples the per-cycle leak. Reusing one pc/offer across theExchangeSDPattempts (the 400 doesn't invalidate the local offer), or not retrying a hard 400 at all, would be a reasonable follow-up — kept out of scope here to keep the fix minimal..localcandidates), so a nest-scopedSetICEMulticastDNSMode(Disabled)would avoid opening the:5353sockets entirely — but that should be scoped to the nest path only (the shared WebRTC server API also serves browser viewers that legitimately send.localhost candidates). Not needed to fix this leak; closing the pc is the correct minimal fix.