TEL-886: Fix tests and other misc - #792
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| st := NewServiceTest(t, nil) | ||
| call, oc, _ := st.CreateOutboundCall(t) | ||
| serverLocalSDP := oc.cc.LocalSDP() | ||
| serverLocalSDP := getMediaPortRemoteAddr(t, oc.media) |
There was a problem hiding this comment.
🟡 Re-INVITE test for outbound calls compares a network address against an SDP message body, so it can never pass
The outbound re-INVITE check stores the call's RTP destination address instead of the negotiated local SDP (getMediaPortRemoteAddr at pkg/sip/signaling_test.go:723), then compares that address to the SDP returned in the reply, so the check always fails and never verifies the intended behavior.
Impact: The outbound re-INVITE test fails (or silently stops validating anything meaningful), hiding regressions in re-INVITE handling.
Copy-paste mismatch between the address helper and the local-SDP getter
The sibling subtests use the correct source of truth: serverLocalSDP, err := getMediaPort(t, oc.media).GetLocalSDP() (pkg/sip/signaling_test.go:751-752 and pkg/sip/signaling_test.go:766-767). In TestReinvite/outbound/normal, serverLocalSDP is instead a netip.AddrPort returned by getMediaPortRemoteAddr (pkg/sip/signaling_test.go:647-653), and it is then used in require.Equal(t, serverLocalSDP, resp.Body(), "reinvite 200 OK should return server local SDP") (pkg/sip/signaling_test.go:730 and pkg/sip/signaling_test.go:741), comparing an AddrPort with []byte. The preceding require.NotEqual(t, call.localSDP, serverLocalSDP, ...) also becomes trivially true.
| serverLocalSDP := getMediaPortRemoteAddr(t, oc.media) | |
| serverLocalSDP, err := getMediaPort(t, oc.media).GetLocalSDP() | |
| require.NoError(t, err) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| p.audioIn.Close() // Propagate Close() to onwards to room | ||
| p.dtmfIn.Close() // Propagate Close() to onwards to room | ||
| p.audioOut.Close() // Pipeline insulated, but close switch | ||
| p.dtmfOut.Close() // Pipeline insulated, but close switch |
There was a problem hiding this comment.
🟡 Outbound call audio input to the room is closed twice during teardown
The room-bound audio writer is now closed by the media port itself (p.audioIn.Close() at pkg/sip/media_port.go:687) while outbound call teardown still closes the same writer afterwards, so the same audio publisher gets shut down twice.
Impact: Outbound call hangup logs spurious errors and relies on the audio publisher tolerating a second shutdown, which may fail or misbehave.
Teardown path that closes the same chain twice
mediaPort.Close() now propagates Close() through the audioIn/dtmfIn switches to the room-side writers. For outbound calls, audioIn holds exactly c.lkRoomIn (set at pkg/sip/outbound.go:556, possibly wrapped at pkg/sip/outbound.go:554). Teardown calls c.media.Close() (pkg/sip/outbound.go:384) and then c.lkRoomIn.Close() (pkg/sip/outbound.go:393), so the opus encoder / published track writer created in Room.NewParticipantTrack (pkg/sip/room.go:759-779) is closed a second time; the error, if any, is only logged as a warning. Inbound calls do not have this duplicate close (pkg/sip/inbound.go:1620), so the redundant close should be removed from the outbound path (or the writers made idempotent).
Was this helpful? React with 👍 or 👎 to provide feedback.
| if audioInProcessor := c.s.handler.GetMediaProcessor(features, featureFlags, string(c.cc.ID()), MediaProcessorOpts{InputSampleRate: RoomSampleRate}); audioInProcessor != nil { | ||
| local = c.audioInProcessor(local) | ||
| } |
There was a problem hiding this comment.
🔴 Incoming calls crash the service when audio processing is enabled
The newly obtained audio processor is checked but then a never-initialized one is invoked instead (c.audioInProcessor(local) at pkg/sip/inbound.go:1618), so any inbound call that requires audio processing crashes the service instead of connecting.
Impact: Inbound calls for projects with audio processing features enabled fail and take down the process with a panic.
Stale field usage after moving processor creation into publishTrack
The processor used to be assigned to the struct field in runMediaConn (c.audioInProcessor = c.s.handler.GetMediaProcessor(...)), which this PR removed. publishTrack now creates a shadowed local audioInProcessor in the if statement but the body still calls the struct field c.audioInProcessor, which is never set anywhere anymore (only declaration remains at pkg/sip/inbound.go:729). Whenever GetMediaProcessor returns a non-nil processor, the condition passes and calling the nil func field panics. The outbound path does this correctly using the local variable (pkg/sip/outbound.go:553).
| if audioInProcessor := c.s.handler.GetMediaProcessor(features, featureFlags, string(c.cc.ID()), MediaProcessorOpts{InputSampleRate: RoomSampleRate}); audioInProcessor != nil { | |
| local = c.audioInProcessor(local) | |
| } | |
| if audioInProcessor := c.s.handler.GetMediaProcessor(features, featureFlags, string(c.cc.ID()), MediaProcessorOpts{InputSampleRate: RoomSampleRate}); audioInProcessor != nil { | |
| local = audioInProcessor(local) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
No description provided.