fix: drain media before BYE on local hangup to prevent voicemail clipping

[rkfshakti]

Fixes livekit/livekit#4737

Problem

When an agent ends a call right after wait_for_playout() returns — EndCall RPC, end_call tool, or removing the SIP participant — the last word of the utterance is clipped on the callee's end (voicemail scenario). The audio is already past the agent: it is buffered in the room mixer (5-frame ~100ms input buffer), the codec encoder, and in-flight RTP. wait_for_playout() only tracks the local queue, so it cannot see this tail.

The SIP bridge then sends BYE and closes media immediately, so whatever is still buffered never clears the wire. A trailing pause masks the bug because it gives the tail time to flush; an abrupt end-of-speech hangup clips the last word. The reporter's workaround (2s pre-hangup sleep) confirms the mechanism.

Fix

Add a configurable drain window hangup_drain_time (default 500ms): on a locally-initiated clean hangup, keep feeding media to the peer for the window before sending BYE. The window is bounded (default 500ms) and only applies to:

• EndCall RPC (reason rpc)
• ctx cancel / agent end-call (reason hangup)
• participant removed from room (reason removed)
• cancelled invite (inbound, reason cancelled)

Remote BYE (the peer hung up), errors, timeouts, media failures, and pre-connect failures are not delayed. Set hangup_drain_time: -1 to disable.

Test

TestOutboundHangupDrainsMediaBeforeBYE: on an established outbound call, an EndCall RPC must not emit BYE before the drain window has elapsed. Fails on the old code (BYE sent before the media drain window elapsed), passes with the fix. All media-port and outbound tests pass; the pre-existing auth-test failures on clean main are unrelated (they fail identically without this change).

[rkfshakti]

Addressed the Devin review finding (cancelled inbound call delayed): drainOnHangup now gates on the media actually being bridged (c.started broken) and "cancelled" is dropped from the local-hangup set — a remote CANCEL before 200 OK is not a local hangup and gets no drain. Also constrained the media-port test range to 10000-20000 so CI can't land on a privileged port (<1024, seen on TestSetOfferReportsUnknownProvider: 'bind: permission denied').

[codecov]

Codecov Report

❌ Patch coverage is 71.87500% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.25%. Comparing base (0460b40) to head (fd7db18).
⚠️ Report is 391 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/sip/inbound.go	66.66%	4 Missing and 1 partial ⚠️
pkg/config/config.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
+ Coverage   65.25%   69.25%   +3.99%     
==========================================
  Files          51       43       -8     
  Lines        6588     8736    +2148     
==========================================
+ Hits         4299     6050    +1751     
- Misses       1915     2163     +248     
- Partials      374      523     +149     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rkfshakti]

Pushed cab53c5 addressing both review comments:

1. Cancellable drain — the drain window is now select { case <-ctx.Done(): case <-time.After(drain): } in both inbound and outbound paths, so context cancellation cuts the drain short instead of a blind time.Sleep.

2. No sleeping under c.mu — outboundCall.close() releases c.mu before the drain wait and re-acquires it after, since Close/CloseWith/CloseWithTimeout all enter with the lock held. A 500ms drain no longer blocks Participant() readers or concurrent shutdown.

Verified: go build ./pkg/sip/ clean, go test ./pkg/sip/ -run 'TestOutbound|Drain|TestInbound' passes; the -race TestService_* failures reproduce on the unmodified head too (pre-existing, unrelated to this PR).

[rkfshakti]

Thanks @genseric-ghiro for the review. I merged the latest main and addressed each thread:

• preserved the original caller context so cancellation/deadlines can cut the optional drain short
• moved the BYE ordering comment next to the actual teardown call
• serialized close ownership without holding c.mu during the drain or while competing closers wait
• added concurrent-hangup regression coverage

The latest head (6bc104d) is mergeable, all checks are green, and all review threads are resolved. Please let me know if anything else is needed — I’m happy to contribute more.

[rkfshakti]

Hi @davidzhao @Cloudish9 — following up on this one. I've pushed the last open review finding: the outbound drainOnHangup now gates on media actually being bridged (c.started), matching the inbound gate from earlier — so a CANCEL during ringing is no longer delayed by the drain window. Regression test added (TestOutboundPreConnectHangupSkipsDrain); the full TestOutbound|Drain|TestInbound suites pass locally, with the branch rebased onto current main.

Quick context: I'm working from a fork (rkfshakti/sip), so a maintainer CI run is needed to kick off the E2E legs — that's the only gate left on my end. Everything else is green and mergeable. Given the repo's fork PR backlog, I'd love to focus on closing this one out — happy to split it into smaller pieces if that helps review. Thanks!

[devin-ai-integration]

Devin Review found 1 new potential issue.

2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

[devin-ai-integration]

🔴 Shutdown strands draining inbound calls

If shutdown starts during time.Sleep, Shutdown cannot terminate the inbound call because done is already set. Server.Stop then closes SIP transport without waiting for BYE, leaving the peer connected.

Learn more

An inbound call marks itself done before the new drain window. Shutdown calls Shutdown on every registered call, but that method re-enters close, which returns immediately once done is set. Server.Stop then closes SIP transport while the original close is asleep. The original close later attempts CloseWithStatus, too late to reliably send BYE on that transport.

Example: A local EndCall begins a 500 ms drain. At 100 ms, service shutdown finds the call, gets an immediate return from Shutdown, and closes the SIP listener. At 500 ms the call attempts BYE; the peer may remain connected until its own timeout.

Recommended fix: Coordinate the in-progress close with Shutdown and Server.Stop. Interrupt the drain on shutdown and wait for the original close to finish sending BYE before closing signaling transport.

---

Was this helpful? React with 👍 or 👎 to provide feedback.