Skip to content

Commit a8e2bda

Browse files
committed
fix(memleak): release call-lifecycle resources + bump rustrtc to 0.3.86
Fixes several memory leaks that accumulated across calls: - policy: track acquired concurrency slots in PolicyCheckOutcome and release them in SipSession::cleanup()/Drop (release_concurrency was never called, permanently exhausting max_total after N calls). Abort path releases immediately; success path threads holds via DialplanHints/Application. - bridge: N-peer forwarder task now honors a child cancel token (select!) so it stops when the bridge tears down instead of pinning the peers map + extra PeerConnections forever. prune_sub_tasks caps live sub-tasks (renegotiation churn) and replaces racy try_lock drops with awaited lock + push. - mixer: MediaMixer gains a Drop that cancels + aborts its 1Hz task; MixerRegistry auto-removes mixers once participant count hits 0 so orphaned supervisor mixers (hang-up without supervisor_stop) no longer leak. - conference_mixer: prune route_gains referencing a leaving leg; guard participant_count against underflow on duplicate remove. - engine: store SipFlow capture JoinHandle on MediaSession; stop/restart/ DestroySession abort it (previously 'stop' was a no-op logging only). - controller: set_timeout/cancel_timeout now track JoinHandles in a map and abort on cancel/re-register/Drop so pending timers don't outlive the call. - Bump rustrtc to 0.3.86 (SRTP SSRC-context eviction, SCTP flow-control hang, notify_one race fix, MediaRelay task exit, PeerConnection task registry).
1 parent b0b8048 commit a8e2bda

14 files changed

Lines changed: 407 additions & 53 deletions

File tree

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ hex = { version = "0.4.3", optional = true }
147147
audio-codec = "0.3.39"
148148
#udio-codec = { path = "../audio-codec", default-features = false }
149149
#rustrtc = { path = "../rustrtc" }
150-
rustrtc = "0.3.83"
150+
rustrtc = "0.3.86"
151151
reqwest = { version = "0.13.4", default-features = false, features = [
152152
"json",
153153
"stream",

src/call/app/controller.rs

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@ use crate::call::domain::{CallCommand, HangupCommand, LegId, MediaSource};
33
use crate::callrecord::CallRecordHangupReason;
44
use crate::proxy::proxy_call::sip_session::SipSessionHandle;
55
use parking_lot::Mutex;
6-
use std::collections::HashSet;
6+
use std::collections::{HashMap, HashSet};
77
use std::sync::Arc;
88
use std::time::Duration;
99
use thiserror::Error;
1010
use tokio::sync::mpsc;
11+
use tokio::task::JoinHandle;
1112
use tokio::time::Instant;
1213
use tracing::{info, warn};
1314

@@ -57,6 +58,11 @@ pub struct CallController {
5758
pub(crate) fired_timer_tx: mpsc::UnboundedSender<String>,
5859
/// Set of timer IDs that have been cancelled and should be suppressed.
5960
pub(crate) cancelled_timers: Arc<Mutex<HashSet<String>>>,
61+
/// JoinHandles of pending timer tasks, keyed by timer id, so they can be
62+
/// aborted on cancel/re-register instead of sleeping for the full delay
63+
/// (which previously kept Arc clones + the channel sender alive past call
64+
/// end and skewed task metrics).
65+
pub(crate) timer_tasks: Arc<Mutex<HashMap<String, JoinHandle<()>>>>,
6066
}
6167

6268
/// Error returned when the remote party hangs up during `collect_dtmf`.
@@ -122,6 +128,7 @@ impl CallController {
122128
event_rx,
123129
fired_timer_tx,
124130
cancelled_timers: Arc::new(Mutex::new(HashSet::new())),
131+
timer_tasks: Arc::new(Mutex::new(HashMap::new())),
125132
};
126133
(ctrl, fired_timer_rx)
127134
}
@@ -218,33 +225,44 @@ impl CallController {
218225
/// After `delay`, [`CallApp::on_timeout`] will be invoked with `id`.
219226
///
220227
/// Calling `set_timeout` with the same `id` before it fires **re-registers**
221-
/// the timer (the previous one is cancelled). Use [`cancel_timeout`](Self::cancel_timeout)
228+
/// the timer (the previous task is aborted). Use [`cancel_timeout`](Self::cancel_timeout)
222229
/// to suppress a pending fire without re-registering.
223-
///
224-
/// # Panics
225-
/// Will not panic; timer tasks are fire-and-forget on a Tokio runtime.
226230
pub fn set_timeout(&self, id: impl Into<String>, delay: Duration) {
227231
let id = id.into();
228-
// If re-registering, un-cancel any previous suppression.
229-
self.cancelled_timers.lock().remove(&id);
232+
// Re-registering: abort the previous timer task (if any) and clear any
233+
// cancellation flag so the new timer is armed fresh.
234+
if let Some(handle) = self.timer_tasks.lock().remove(&id) {
235+
handle.abort();
236+
}
237+
self.cancelled_timers.lock().remove(&id);
238+
230239
let tx = self.fired_timer_tx.clone();
231240
let cancelled = self.cancelled_timers.clone();
241+
let tasks = self.timer_tasks.clone();
232242
let id_task = id.clone();
233-
crate::utils::spawn(async move {
243+
let handle = crate::utils::spawn(async move {
234244
tokio::time::sleep(delay).await;
245+
// Self-remove so the handle map does not retain finished tasks.
246+
tasks.lock().remove(&id_task);
235247
// Only fire if not cancelled in the meantime.
236248
let was_cancelled = cancelled.lock().remove(&id_task);
237249
if !was_cancelled {
238250
let _ = tx.send(id_task);
239251
}
240252
});
253+
self.timer_tasks.lock().insert(id, handle);
241254
}
242255

243256
/// Cancel a pending timer previously registered with [`set_timeout`](Self::set_timeout).
244257
///
245258
/// If the timer has already fired, this is a no-op.
246259
pub fn cancel_timeout(&self, id: &str) {
247260
self.cancelled_timers.lock().insert(id.to_string());
261+
// Abort the sleeping task immediately rather than letting it run for the
262+
// remainder of its delay.
263+
if let Some(handle) = self.timer_tasks.lock().remove(id) {
264+
handle.abort();
265+
}
248266
}
249267

250268
/// Start recording the call audio.
@@ -424,6 +442,17 @@ impl CallController {
424442
}
425443
}
426444

445+
impl Drop for CallController {
446+
fn drop(&mut self) {
447+
// Abort every still-pending timer task so they don't keep sleeping (and
448+
// holding their captured Arc/channel clones) after the call has ended.
449+
let tasks = std::mem::take(&mut *self.timer_tasks.lock());
450+
for (_, handle) in tasks {
451+
handle.abort();
452+
}
453+
}
454+
}
455+
427456
#[cfg(test)]
428457
mod tests {
429458
use super::*;

src/call/mod.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,6 +913,11 @@ pub struct Dialplan {
913913
/// When present, these take priority over the original SIP request headers
914914
/// when building CallInfo for application flows (IVR, voicemail, etc.).
915915
pub routed_headers: Option<Vec<rsipstack::sip::Header>>,
916+
917+
/// Concurrency slots acquired by routing policy checks. Released by the
918+
/// session on hangup/teardown. Held in an `Arc<Mutex<..>>` so the shared
919+
/// `Arc<Dialplan>` (inside `CallContext`) can still drain them on cleanup.
920+
pub concurrency_holds: Arc<Mutex<Vec<crate::call::policy::ConcurrencyHold>>>,
916921
}
917922

918923
impl std::fmt::Debug for Dialplan {
@@ -973,6 +978,7 @@ impl Dialplan {
973978
passthrough_failure: false,
974979
audio_profile: None,
975980
routed_headers: None,
981+
concurrency_holds: Arc::new(Mutex::new(Vec::new())),
976982
}
977983
}
978984

src/call/policy.rs

Lines changed: 126 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,25 @@ pub enum PolicyCheckStatus {
9797
Rejected(PolicyRejection),
9898
}
9999

100+
/// A concurrency slot acquired by a call via policy enforcement. The slot must
101+
/// be released when the call ends (regardless of how it ends) to avoid
102+
/// permanently exhausting the configured `max_total` concurrency budget.
103+
#[derive(Debug, Clone)]
104+
pub struct ConcurrencyHold {
105+
pub policy_id: String,
106+
pub scope: String,
107+
pub scope_value: String,
108+
}
109+
110+
/// Outcome of [`PolicyGuard::check_policy`]. Carries the decision plus any
111+
/// concurrency slots that were acquired during the check (only present when the
112+
/// call was allowed AND a concurrency limit was enforced).
113+
#[derive(Debug)]
114+
pub struct PolicyCheckOutcome {
115+
pub status: PolicyCheckStatus,
116+
pub concurrency_holds: Vec<ConcurrencyHold>,
117+
}
118+
100119
pub struct PolicyGuard {
101120
limiter: Arc<dyn FrequencyLimiter>,
102121
}
@@ -112,13 +131,61 @@ impl PolicyGuard {
112131
Self { limiter }
113132
}
114133

134+
/// Access the underlying limiter (used to release concurrency slots).
135+
pub fn limiter(&self) -> &Arc<dyn FrequencyLimiter> {
136+
&self.limiter
137+
}
138+
139+
/// Best-effort release of all concurrency slots held by a call. Called from
140+
/// the session cleanup path. Errors are logged and swallowed because a
141+
/// failed release must not abort call teardown.
142+
pub async fn release_concurrency_holds(holds: &[ConcurrencyHold], limiter: &dyn FrequencyLimiter) {
143+
for hold in holds {
144+
if let Err(e) = limiter
145+
.release_concurrency(&hold.policy_id, &hold.scope, &hold.scope_value)
146+
.await
147+
{
148+
error!(
149+
"failed to release concurrency slot for policy={} scope={}: {}",
150+
hold.policy_id, hold.scope, e
151+
);
152+
}
153+
}
154+
}
155+
115156
pub async fn check_policy(
116157
&self,
117158
policy_id: &str,
118159
policy: &PolicySpec,
119160
caller: &str,
120161
callee: &str,
121162
origin_country: Option<&str>,
163+
) -> Result<PolicyCheckOutcome> {
164+
let mut holds: Vec<ConcurrencyHold> = Vec::new();
165+
let status = self
166+
.check_policy_inner(policy_id, policy, caller, callee, origin_country, &mut holds)
167+
.await?;
168+
// Only carry holds forward when the call was allowed. On rejection the
169+
// concurrency check returned false, so nothing was acquired.
170+
let concurrency_holds = if status == PolicyCheckStatus::Allowed {
171+
holds
172+
} else {
173+
Vec::new()
174+
};
175+
Ok(PolicyCheckOutcome {
176+
status,
177+
concurrency_holds,
178+
})
179+
}
180+
181+
async fn check_policy_inner(
182+
&self,
183+
policy_id: &str,
184+
policy: &PolicySpec,
185+
caller: &str,
186+
callee: &str,
187+
origin_country: Option<&str>,
188+
holds: &mut Vec<ConcurrencyHold>,
122189
) -> Result<PolicyCheckStatus> {
123190
// 1. Static Checks
124191
if let Some(prefix) = &policy.called_prefix
@@ -318,17 +385,26 @@ impl PolicyGuard {
318385
));
319386
}
320387

321-
if let Some(limit) = &policy.concurrency
322-
&& !self
388+
if let Some(limit) = &policy.concurrency {
389+
let allowed = self
323390
.limiter
324391
.check_concurrency(policy_id, "caller", caller, limit.max_total)
325-
.await?
326-
{
327-
let reason = format!("Concurrency limit reached for caller {}", caller);
328-
warn!("{}", reason);
329-
return Ok(PolicyCheckStatus::Rejected(
330-
PolicyRejection::ConcurrencyLimitExceeded(reason),
331-
));
392+
.await?;
393+
if !allowed {
394+
let reason = format!("Concurrency limit reached for caller {}", caller);
395+
warn!("{}", reason);
396+
return Ok(PolicyCheckStatus::Rejected(
397+
PolicyRejection::ConcurrencyLimitExceeded(reason),
398+
));
399+
}
400+
// Record the acquired slot so the caller can release it when the
401+
// call ends. This is the only limit type that needs explicit release
402+
// (frequency/daily limits are window-based and self-expire).
403+
holds.push(ConcurrencyHold {
404+
policy_id: policy_id.to_string(),
405+
scope: "caller".to_string(),
406+
scope_value: caller.to_string(),
407+
});
332408
}
333409

334410
Ok(PolicyCheckStatus::Allowed)
@@ -1150,6 +1226,46 @@ mod tests {
11501226
.await;
11511227

11521228
assert!(result.is_ok());
1153-
assert_eq!(result.unwrap(), PolicyCheckStatus::Allowed);
1229+
assert_eq!(result.unwrap().status, PolicyCheckStatus::Allowed);
1230+
}
1231+
1232+
#[tokio::test]
1233+
async fn test_check_policy_records_concurrency_hold() {
1234+
let limiter = InMemoryFrequencyLimiter::new();
1235+
let guard = PolicyGuard::new(limiter.clone());
1236+
1237+
let policy = PolicySpec {
1238+
concurrency: Some(ConcurrencyLimit {
1239+
max_total: 5,
1240+
max_per_account: HashMap::new(),
1241+
}),
1242+
..Default::default()
1243+
};
1244+
1245+
let outcome = guard
1246+
.check_policy("hold-policy", &policy, "caller9", "callee9", None)
1247+
.await
1248+
.unwrap();
1249+
1250+
assert_eq!(outcome.status, PolicyCheckStatus::Allowed);
1251+
assert_eq!(outcome.concurrency_holds.len(), 1);
1252+
let hold = &outcome.concurrency_holds[0];
1253+
assert_eq!(hold.policy_id, "hold-policy");
1254+
assert_eq!(hold.scope, "caller");
1255+
assert_eq!(hold.scope_value, "caller9");
1256+
1257+
// Releasing must decrement the counter so subsequent calls are allowed.
1258+
limiter
1259+
.release_concurrency(&hold.policy_id, &hold.scope, &hold.scope_value)
1260+
.await
1261+
.unwrap();
1262+
1263+
// Now a second check for the same caller should still be allowed (slot
1264+
// was released), proving the release path works.
1265+
let outcome2 = guard
1266+
.check_policy("hold-policy", &policy, "caller9", "callee9", None)
1267+
.await
1268+
.unwrap();
1269+
assert_eq!(outcome2.status, PolicyCheckStatus::Allowed);
11541270
}
11551271
}

src/config.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -973,6 +973,9 @@ pub struct DialplanHints {
973973
pub video_policy: Option<crate::proxy::routing::VideoPolicy>,
974974
/// Per-trunk ringback/early-media audio configuration
975975
pub ringback: Option<crate::proxy::routing::RingbackAudio>,
976+
/// Concurrency slots acquired during routing policy enforcement. The
977+
/// session releases them on hangup to avoid leaking the concurrency budget.
978+
pub concurrency_holds: Vec<crate::call::policy::ConcurrencyHold>,
976979
}
977980

978981
impl std::fmt::Debug for DialplanHints {
@@ -1003,6 +1006,7 @@ pub enum RouteResult {
10031006
app_name: String,
10041007
app_params: Option<serde_json::Value>,
10051008
auto_answer: bool,
1009+
hints: Option<DialplanHints>,
10061010
},
10071011
NotHandled(InviteOption, Option<DialplanHints>),
10081012
Abort(StatusCode, Option<String>),

0 commit comments

Comments
 (0)