Repository navigation
Ignore a duplicated server answer - #1171
xianshijing-lk wants to merge 1 commit into
Conversation
set(remoteDescription:offerId:) rejected answers whose offerId did not
match the latest offer, but a second copy of the answer for that same
offer passed the check and was applied again. The code already noticed
the problem and only logged it:
if signalingState != .haveLocalOffer {
log("Received answer with unexpected signaling state: ...")
}
then applied the answer anyway, so libwebrtc rejected it with
"Failed to set remote answer sdp: Called in wrong state: stable" and a
renegotiation that had succeeded surfaced as a throw.
Track the offer whose answer was applied and the one whose answer is in
flight, and drop a repeat of either. The check runs before the deferred
initial offer is consumed, for the same reason the offerId validation
does: a rejected answer must not take the pending offer with it.
Port of livekit/client-sdk-js#2114.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
praise: The port keeps the JS helper shape, checks duplicates before applyPendingInitialOffer(), records success only after a successful apply, and the new test fails without the fix.
Requesting changes for one item: the claim window extends past the SDP apply into the ICE resume and the renegotiation. The rest are test coverage and style.
| // Claimed before the first `await`, so a copy arriving mid-apply is caught too, and | ||
| // released however the apply ends so a genuine retry is still possible. | ||
| _answerInFlight = offerId | ||
| defer { _answerInFlight = nil } |
There was a problem hiding this comment.
issue (blocking): The claim and the success record stay held while the raw set(remoteDescription:) overload awaits the ICE resume and createAndSendOffer().
If answer N+1 enters during that tail, the defer of N clears the claim of N+1 and N overwrites the success record. A mid-apply copy of N+1 then passes both checks. SignalClient.notifyDetached spawns detached tasks, so the interleaving is possible.
| try await set(remoteDescription: sd) | ||
|
|
||
| // only a successfully applied answer makes later copies duplicates | ||
| if offerId > 0 { _answeredOfferId = offerId } |
There was a problem hiding this comment.
suggestion (blocking): Record _answeredOfferId and release _answerInFlight right after the setRemoteDescription continuation, before the ICE resume and the renegotiation.
That matches the JS fix, which records success directly after the SDP apply. We can keep the raw overload for the ICE-restart path, which reapplies on purpose.
| hasPendingInitialOffer: _pendingInitialOffer != nil) | ||
| { | ||
| // a repeat of an answer already applied or being applied (e.g. duplicated by a proxy) | ||
| log("Ignoring duplicate answer, offerId: \(offerId)", .warning) |
There was a problem hiding this comment.
nitpick: Log this at .debug.
The SDK handles the event internally, and AGENTS.md reserves .warning for issues the consumer must act on.
| signalingState: LKRTCSignalingState, | ||
| hasPendingInitialOffer: Bool) -> Bool | ||
| { | ||
| if answerInFlight == offerId { return true } |
There was a problem hiding this comment.
question (non-blocking): Is answerInFlight == offerId deliberate for JS parity?
While any answer is in flight, no other answer can be applied, so answerInFlight != nil would also guard a mix of legacy and numbered answers.
| /// no local offer left to answer, so applying it again is invalid. | ||
| @Suite(.tags(.networking)) struct DuplicateAnswerTests { | ||
| /// Records nothing; `Transport` requires a delegate but none of these tests exercise it. | ||
| private final class StubDelegate: TransportDelegate { |
There was a problem hiding this comment.
suggestion (test,non-blocking): Add one decision-matrix test for isDuplicateAnswer.
The JS PR unit-tests all seven decisions of the helper, and this file covers two. The helper is an internal pure function, so one test covers the mid-apply duplicate, the retry after a failed apply, and the unnumbered cases with no WebRTC setup.
| #expect(await transport.signalingState == .stable) | ||
|
|
||
| let answer = try await answer(to: offer.offer) | ||
| try await transport.set(remoteDescription: answer, offerId: offer.offerId) |
There was a problem hiding this comment.
suggestion (test,non-blocking): Add an offerId: 0 variant of this test.
The numbered branch of the helper ignores hasPendingInitialOffer, so this test still passes if the legacy exception is removed.
|
|
||
| let appliedSdp = await transport.remoteDescription?.sdp | ||
|
|
||
| // The same answer again, exactly as a proxy would repeat it. |
There was a problem hiding this comment.
suggestion (test,non-blocking): Use let appliedSdp = try #require(await transport.remoteDescription?.sdp), and #require for the data channel at line 63.
If the first apply leaves remoteDescription nil, the final #expect compares nil == nil and passes.
| let transport = try await makeTransport(singlePCMode: true, delegate: delegate) | ||
| defer { Task { await transport.close() } } | ||
|
|
||
| _ = await transport.dataChannel(for: LKRTCDataChannel.Labels.reliable, |
There was a problem hiding this comment.
nitpick: defer { Task { await transport.close() } } starts a task that outlives the test.
The pattern comes from OfferWithJoinTests, but an awaited teardown in a scoped helper is cleaner in async tests.
Port of livekit/client-sdk-js#2114.
The bug
set(remoteDescription:offerId:)rejects answers whoseofferIddoes not match the latest offer, but a second copy of the answer for that same offer matches, so it passed the check and was applied again.What makes this one stand out is that the code already noticed:
It detects exactly the situation a duplicate produces, logs it, and then applies the answer anyway. libwebrtc rejects it, so a renegotiation that had in fact succeeded surfaces as a throw.
Repro
Added
DuplicateAnswerTests, reusing the local two-peer-connection pattern fromOfferWithJoinTests(no server needed). Against the unfixed code:The fix
Track
_answeredOfferId(the offer whose answer was applied, set only after a successful apply so a retry after a genuine failure still works) and_answerInFlight(claimed before the firstawait, released in adefer, so a copy arriving mid-apply is caught too). The warning above becomes a drop.The decision is a free
isDuplicateAnswerfunction mirroring the JS SDK's exported helper, including thehasPendingInitialOfferterm — this SDK defers the JOIN-bundled offer, so the connection reads.stablewhile one is outstanding and the unnumbered-answer fallback would otherwise drop its answer.The check runs before
applyPendingInitialOffer(), for the same reason the existingofferIdvalidation does: an answer that is about to be rejected must not consume the deferred offer on its way out.Verification
swift test --filter DuplicateAnswerTests— fails before the change (above), passes after.swift test --filter "OfferWithJoinTests|TransportCloseTests|WARPTests|TransportMungeFallbackTests"— 17 tests, all passing.OfferWithJoinTestsis the important one here, since it exercises the deferred-initial-offer path this guard sits in front of.I could not run the full suite to completion locally: a large share of
LiveKitCoreTestsis.tags(.e2e)and needs a live LiveKit server. The suites above are the ones covering the code this touches.🤖 Generated with Claude Code