Repository navigation
Conversation
controllingSelector.ContactCandidates resends the USE-CANDIDATE check to the nominated pair on every tick until it succeeds. Nothing clears the nomination if every response comes back asymmetric, so the agent never fails over and the connection times out even though other valid pairs exist. Observed with WebKit on macOS: a peer with two NICs on one subnet, bound to the wildcard address, answers a nominated pair from the other interface. Both pairs pass their own connectivity check, but the nomination's own response is asymmetric and gets discarded. RFC 8445 Section 7.2.5.2.1 requires an asymmetric response to fail the check it answers. Apply that to the nomination: fail the nominated pair over, but only when another nominatable pair exists, so a lone pair is never stranded. A per-agent request sequence number, assigned on the agent's task loop, orders binding requests; a response to a request sent before the pair was abandoned is discarded, one sent after is always accepted. Only the selector's own nomination requests, marked when sent, can trigger the fallback; renomination requests are exempt from the cutoff. Also implement RFC 8445 Section 7.3.1.4: a Binding request for a Failed pair re-arms it and sends a triggered check, instead of leaving it Failed forever. Known limitation: the controlling and controlled agents can end up with different selected pairs after a fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
Hey @fergusean thanks for the PR! Reading the description this should be a much smaller PR. Can you implement just this This code should only need one or two tests to confirm that behavior in an existing test file |
JoTurk
reviewed
Sep 30, 2026
| @@ -0,0 +1,928 @@ | |||
| // SPDX-FileCopyrightText: 2026 The Pion community <https://pion.ly> | |||
Member
There was a problem hiding this comment.
Please don't add new standalone test files, and try to consolidate the tests, thank you
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1020 +/- ##
==========================================
+ Coverage 91.90% 91.99% +0.09%
==========================================
Files 46 46
Lines 5510 5561 +51
==========================================
+ Hits 5064 5116 +52
+ Misses 446 445 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fergusean
changed the title
fergusean
added a commit
to zyno-io/sp2p
that referenced
this pull request
Oct 1, 2026
* Fix relay consent so a peer's decline is never missed as an "agree" relay-retry meant both "I'm asking" and "I agreed," and was sent before the local prompt. A peer that already sent relay-retry looked "agreed" to the other side even if it went on to decline: the accepting side allocated a TURN relay and then failed on a plain 15s/30s timeout or a misleading "peer disconnected" instead of a clear decline message. Split consent into two signaling stages carried as additive payload fields on the existing relay-retry/relay-denied messages: consent pending or granted, reason declined or unavailable. No new message type, no version bump, no server change: every server since v0.1.0 already forwards these verbatim, and an old (0.6.2 or earlier) peer's empty payload is always treated as granted/declined, matching its original behavior. internal/conn/relay.go is a new shared implementation (RelayWatch, RetryWithRelay) replacing the drifted copies in flow/helpers.go and peer/peer.go, used by flow/send.go, flow/receive.go, peer/peer.go, and the CLI's machine/TTY relay prompts (internal/cli/machine.go, send.go, stream_reporter.go). signal.Client gains SubscribeTypes for in-order multi-type delivery. The CLI's interactive relay prompt is now cancellable (context.AfterFunc closes the TTY), and internal/cli/machine.go reports a new peer_relay_denied JSON code when the peer, not this side, declined. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * web: split relay consent into pending/granted, matching the Go fix web/src/relay-consent.ts is a new TypeScript port of internal/conn/relay.go's state machine (RelayConsentWatch, same priority order: peer declined, then peer left, then signaling lost, then go, then pending). main.ts's establishP2PWithRetry is rewritten around it, and webrtc.ts's establishWebRTC now removes only the handlers it registered itself (tracked per call) instead of a blanket sigClient.off(type) at attempt start/cleanup, which used to silently remove a longer-lived listener like RelayConsentWatch's own peer-left handler — and now accepts an AbortSignal so a relay retry's attempt 2 can be cancelled the instant the peer declines or leaves. Tests: web/tests/relay-consent.spec.ts is new (mirrors the Go relay watch cases, plus a regression guard for the webrtc.ts handler-cleanup fix, against a fake signal client and a stubbed RTCPeerConnection). web/tests/relay.spec.ts's two existing decline tests are made deterministic with a new held-dialog helper (they raced the dialog against the peer's decline before), and 8 new consent-split tests (4 pairings x which side declines) prove the accepting side never starts a relay attempt before the peer's decision: zero TURN allocations, and the peer's decline reported within 10s. Two of the eight are tagged @pr. web/tests/relay-summary.mjs gets the new labels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: document two-sided relay consent man/sp2p.1 (relay paragraph, JSON events), README.md (machine-mode bullet, TURN Relay section, trust-model bullet), web/llm.md (relay_required section), SECURITY.md (a note that consent is coordinated over unauthenticated signaling but enforced locally on each side), docs/testing.md (Consent controls section rewritten to describe the fix and the new consent-split tests, replacing the "known asymmetric-consent-messaging gap" finding this fix resolves; the CLI-denial race in Known rough edges is now marked resolved), and a CHANGELOG.md Unreleased/Fixed entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add per-lane failure diagnostics for parallel WebRTC setup No protocol change. Every silent per-lane discard site in negotiateWebRTC (Go) and negotiateParallelWebRTC (TS) now records why: a WebRTCLane gets an address-free trace (conn/ICE/DTLS transitions, DataChannel open/close, self- close reason) via Trace()/PairTypes() (candidate types only). Go returns a ParallelLaneReport{Requested,Accepted,Ours,Theirs,Selected,SetupMS, Failures[]{ID,Stage,Class,Trace,Pair}} with a fixed Class vocabulary (timeout/eof/closed/mismatch/error) -- raw error text never leaves OnVerbose. An optional ParallelLaneReporter handler interface and a new CLI parallel_lanes JSON event (verbose-only in human mode) surface it. The browser logs one console line on any unselected accepted lane, and now closes a lane immediately when its own description gathering fails instead of waiting out wait()'s 8s timer. Adds a manual-dispatch-only lanes-stress.yml (not a release gate) that repeats CLI<->Chromium parallel-lane setup, including under CPU contention, to chase the rare shortfall this diagnostic surface exists to explain. Verified live: a real, reproducible WebKit-specific lane shortfall in this sandbox (CLI sender -> webkit receiver, pre-existing on main, confirmed via a stashed baseline comparison) was captured cleanly by the new trace/stage/ class output with no addresses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Fix relay-consent review findings: TTY cancel, error attribution Three fixes from review of 0942bc3/a2b9552/429cf17: - The interactive relay prompt's cancellation (context.AfterFunc(ctx, tty.Close)) did not reliably unblock a pending read on every platform: on macOS, a /dev/tty character device can't be registered with the kqueue-based netpoller, so Go falls back to a plain blocking read that a concurrent Close does not interrupt (verified blocked 3+ seconds under `script`). internal/cli/send.go's new readRelayAnswer reads on its own goroutine and selects against ctx instead, returning immediately on cancellation without waiting for (or ever acting on) whatever the goroutine eventually reads. Reused by stream_reporter.go's plain-TTY path. - Signaling loss (or a peer decline) noticed while attempt 1 was still running could surface as a bare "context canceled" with no OnError/message at all, whenever relay wasn't subsequently attempted (-transport tcp, or no TURN available) — internal/peer/peer.go had the same regression against its pre-relay-consent behavior, which at least said "peer disconnected". flow/helpers.go's retryWithRelay error-mapping switch is extracted into reportRelayWatchErr (and peer.go keeps using its own mapRelayErr) so send.go/receive.go/ peer.go can call it right after a failed attempt 1 too, using the RelayWatch's own Err(). Also reconsidered whether attempt 1 should be cut short by bare signaling loss at all: it shouldn't. AttemptContext no longer includes it (only a peer's relay-retry/relay-denied/peer-left still cancel attempt 1 early) — attempt 1 (WebRTC via STUN, or symmetric TCP with LAN/UPnP addresses) can need no further signaling once candidates/direct endpoints are already exchanged, which is also why this package closes signaling right after key confirmation. Cutting attempt 1 short the instant signaling merely drops would trade an attempt that might still succeed for a guaranteed failure; Establish already bounds the wait with its own internal timeout, and the new post-Establish check still reports signaling loss accurately if that's what ultimately doomed the attempt. - The credential-wait timeout regressed from a clear OnError ("Server did not provide TURN credentials") to a bare returned error. Restored via a new ErrTURNCredentialsTimeout sentinel mapped the same way as every other relay-consent error. Tests: internal/cli/relay_prompt_test.go (readRelayAnswer's cancel/ late-answer/read-outcome behavior via os.Pipe); internal/conn/ relay_test.go (AttemptContext's new cancel set, including a check that bare signaling loss does NOT cancel it; a direct, deterministic test of run()'s drain-before-marking-lost path — verified to fail if drain()'s call or body is removed, unlike replaying the same scenario over a real connection, which can't reliably force that specific select race since message delivery happens-before close detection on the same readLoop goroutine); internal/flow/helpers_test.go (reportRelayWatchErr's full mapping table); internal/peer/peer_test.go (mapRelayErr's new credential-timeout case). internal/server/server_test.go's relay-consent passthrough test now uses an ephemeral TURN config instead of StaticTURN, so its "the second relay-retry returns the same cached credentials" assertion actually exercises turnOnce's caching (a static credential is identical on every issuance regardless of whether the cache runs at all). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs/tests: old-peer allocation caveat; fix an unawaited dialog resolve - docs/testing.md: "a side never starts (or even requests) TURN credentials for attempt 2" was wrong — relay-retry{pending} does request credentials right away (that's a fetch, not an allocation). Reworded to "never starts attempt 2 — and so never allocates anything on the TURN server". - man/sp2p.1 claimed no relay connection is attempted by either side, which doesn't hold against a peer on v0.6.2 or earlier: that peer's early, unconditional relay-retry{} counts as an immediate grant, so this side's own consent alone can start a relay attempt (and briefly allocate) before the old peer's real decision arrives. Added the same caveat README.md's TURN Relay section now has. - web/tests/relay.spec.ts's "CLI receiver denies, browser sender declines" test called senderHeld.resolve(false) without first awaiting senderHeld.opened; registerHeldDialog's resolve() is a no-op until a dialog has actually appeared, so this could silently drop the intended dismissal if the browser's dialog hadn't shown up yet. (Not run this pass — Playwright is unavailable while another run holds its fixed port — but it now matches every other call site in the file, which already awaits .opened first.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Use patched pion/ice so CLI→WebKit gets all 8 lanes on multi-homed Macs pion/ice's controlling agent retried an asymmetrically answered nomination forever. WebKit on a Mac with two NICs on one subnet answers from the other interface, so CLI-offered lanes stalled (as few as 3/8). Replace pion/ice/v4 with zynoconsulting/ice (v4.4.2 plus the fix sent upstream as pion/ice#1019) until a pion/webrtc v4 release includes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Point pion/ice fork references at pion/ice#1020 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * flow: don't double-count a lane's own gather failure as a peer failure negotiateWebRTC recorded a lane's own Description() failure (stageGather or stageSDPSize) but left lanes[id] set, so an empty offer/answer still went out over the wire as if we'd succeeded; the sender's answer-read loop then saw the peer's resulting empty answer and recorded a second, misattributed peer-answer-empty failure for the same lane id. Close and drop the lane once every gather goroutine has finished (gather.Wait()), not from inside the goroutine itself. A review pass found that writing lanes[id] from the goroutine raced with the deferred cleanup's own read of lanes[id] on an early return (e.g. the receiver bailing out of the per-id loop on a peer protocol error while another lane's gather goroutine was still running). Doing the cleanup in the main goroutine after gather.Wait() removes the concurrent writer entirely instead of just narrowing the race window. Also corrects a related doc/comment claim: raw Go/Pion lane error text was said to only ever reach OnVerbose, but negotiateWebRTC doesn't even take a Handler/logger - the text is discarded outright, not logged anywhere. Checked whether OnVerbose would be a safe destination anyway: machine mode's JSON log event forwards OnVerbose output verbatim whenever -v is set, and internal/conn already sends addresses through OnVerbose as ordinary human diagnostics, so the corrected text no longer implies OnVerbose is address-free in general - it explains why raw lane error text specifically is kept out of it. Added TestNegotiateWebRTCOwnDescriptionFailureRecordsExactlyOneFailure, which spins up a real WebRTC primary pair (a real signaling server plus two conn.EstablishWebRTC calls) so a real, non-nil lane exists, and a new testLaneDescriptionFault hook to force its Description() result deterministically. Verified it fails (two failure entries, the second peer-answer-empty) against the pre-fix code and passes after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * conn/flow/peer: bound RelayWatch's goroutines to the watch's own lifetime; report a clear error when a peer's relay-retry can't be honored here RelayWatch.AttemptContext's watcher goroutine only exited on parent.Done() or a peer signal (retry/declined/left) - never on the watch's own Close(). Every caller does `defer relayWatch.Close()` right after WatchRelay, but send.go and receive.go only reach that defer at the very end of their (large) functions, so the goroutine - and the RelayWatch/signal client it holds - could leak for the rest of the transfer, or effectively forever against a caller whose parent context is very long-lived. Added a `case <-w.closeCh` to AttemptContext's select, and an explicit relayWatch.Close() in send.go/receive.go right after the connection is established (before the transfer itself runs), so the goroutine exits right after the attempt instead of lingering. peer.go's establish() was already tightly scoped (returns right after connecting) so it needed no call-site change. A review pass then found that closing the watch earlier exposed the same leak in abortContext (RetryWithRelay's promptCtx/attempt2Ctx): previously, bare signaling loss reaching client.Done() eventually unblocked it via setLost()/abortCh, but the new early Close() now stops run() before that can happen. Fixed the same way: abortContext's select also gained a `case <-w.closeCh`. Separately: when this side runs -transport tcp (or the server has no TURN) and the peer's own attempt 1 fails first, the peer's relay-retry{pending} cancels our attempt 1 via AttemptContext's retryCh, but since this side was never going to retry via relay anyway, the code fell through to a bare (wrapped) context.Canceled with no OnError call - surfacing as JSON code "canceled" with no useful message. Added RelayWatch.PeerRequestedRelay() (exposes the existing sawRetry flag) and conn.PeerRelayUnusableError (matches sentinel conn.ErrPeerRelayUnusable via errors.Is, carrying whether it's because of -transport tcp or no TURN). send.go, receive.go, and peer.go's establish() now report this clearly instead of the bare cancellation whenever attempt 1 was cut short specifically by a peer relay-retry and this side won't be retrying via relay itself. machine.go's finish() maps the new sentinel to the existing relay_not_allowed JSON code (the same code already used for the local could-not-participate case), rather than inventing a new one. Tests: TestRelayWatch_AttemptContextExitsOnClose and TestRelayWatch_AbortContextExitsOnClose (each verified to fail against the pre-fix select, then pass); TestRelayWatch_PeerRequestedRelay; new cases in flow/helpers_test.go's reportRelayWatchErr table, peer/peer_test.go's mapRelayErr table, and a new machine_test.go case for the JSON code mapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * server: make the TURN credential caching test actually detect caching TestSignal_RelayConsentPayloadsPassThroughUnchanged's "second relay-retry returns the same cached credentials" assertion compared two Generate() outputs, but TURNCredentialGenerator.Generate's HMAC username is a Unix expiry timestamp - one-second resolution - and the whole exchange in this test completes within one wall-clock second. So two genuinely separate Generate calls could still produce byte-identical output, meaning the test could pass even with session.turnOnce removed. Added an unexported onGenerate hook to TURNCredentialGenerator, called synchronously at the start of every real Generate call; nil in production. The test now counts real Generate calls directly (via atomic.Int32) and asserts exactly one, independent of the timestamp coincidence. Verified this actually detects the regression: temporarily replaced session.turnOnce.Do(func(){...}) in handler_signal.go with an immediately-invoked closure (same body, no caching) - the test failed with "Generate called 2 times, want exactly 1"; reverted, and it passes again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * cli: make readRelayAnswer's leaked-goroutine test actually detect the leak TestReadRelayAnswer_LateAnswerIsDiscarded wrote 2 bytes to an os.Pipe after cancellation and asserted the write didn't block - but a kernel pipe write that small never blocks regardless of whether the reading goroutine's resultCh send would block, so this never actually exercised resultCh's buffering. The test could pass even if resultCh were unbuffered, silently leaking readRelayAnswer's background reader goroutine on every cancellation. Replaced it with TestReadRelayAnswer_GoroutineExitsAfterCancelAndRelease, using a new blockingReadCloser whose Read blocks until released (simulating a human who hasn't answered yet) and whose Close signals. Since tty.Close() is deferred and so runs only once the goroutine's resultCh send has actually completed, observing Close after release is direct proof the goroutine returned instead of blocking forever trying to send on an unbuffered, unread channel. Verified this actually detects the regression: temporarily made resultCh unbuffered in send.go - the test failed ("reader goroutine did not exit after release"); reverted, and it passes again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * web/tests: fix a hardening test's timing and a missing diagnostics call parallel.spec.ts's "a lane that fails to gather its description is closed immediately, not left for wait()'s 8s timer" test started its Date.now() timer only after the ready/commit message round trip had already completed. Per web/src/webrtc-parallel.ts, the actual 8-second stall this test guards against (an ungathered lane's own wait() sitting on its timeout instead of being closed immediately) happens at Promise.all(lanes.map(lane => lane?.wait() ...)), which must finish before the "ready" step can even begin - so without the hardening fix, that stall would show up in the read()s preceding the timer, not in `await result` after it. The test therefore passed whether or not the fix was present, just 8 seconds slower without it. Moved the timer start to right after the "accept" read, before the offer/answer/ready/commit round trip, so the assertion now covers the point where the real delay would occur. (Reasoned from source, not run - Playwright's port was in use by another test run per instructions; `cd web && npx tsc --noEmit` passes.) Separately, docs/testing.md claims Playwright's dump-on-failure diagnostics capture both the CLI's JSON event stream and the browser's console line whenever a lane-count shortfall occurs. parallel-interop. spec.ts's "browser parallel WebRTC sender interoperates with CLI receiver" test (browser sender, CLI receiver) called trackCLIForDiagnostics for the CLI side but never trackForDiagnostics for the browser page, so the claim didn't actually hold for that test. Added the missing call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Carry only the pion/ice#1021 fix, rebased on ice v4.4.4 Point the pion/ice replace at zynoconsulting/ice 36c1201: pion/ice v4.4.4 plus the same minimal change proposed upstream in pion/ice#1021 (fail a nominated pair whose reply comes from a different remote address, then nominate another). Bump pion/webrtc to v4.2.22, which requires ice v4.4.4, so the fork no longer blocks dependency updates. Update docs and changelog references. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <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
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.
Description
RFC 8445 §7.2.5.2.1 requires that an asymmetric response fail the check
it answers. The controlling agent currently discards an asymmetric
response to its nomination and resends USE-CANDIDATE to the same pair on
every tick, so if every response is asymmetric it never tries another
pair.
Observed with WebKit on macOS: a peer with two NICs on one subnet, bound
to the wildcard address, answers the nomination from the other
interface, so the controlling agent retries that pair until the
connection times out.
Changes
mark the pair it was sent on as Failed.
it, so a later tick can nominate another valid pair.
Tests
Two tests in
selection_test.go: the pair is failed on an asymmetricnomination response, and the next tick nominates a different pair.