From 1a04fc5db37fe7269f0b58ecf1b383157b8647b5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 6 Nov 2025 22:28:26 +0000 Subject: [PATCH] Address review feedback: use Message field for SDP, remove table id, store ended_timestamp Co-authored-by: link2xt <18373967+link2xt@users.noreply.github.com> --- src/calls.rs | 33 ++++++++++++++++++++++----------- src/message.rs | 8 ++++++++ src/mimefactory.rs | 29 ++++++++++++++++++----------- src/sql.rs | 9 +++------ src/sql/migrations.rs | 9 ++++----- 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/src/calls.rs b/src/calls.rs index 30f9e7b1c..02409a047 100644 --- a/src/calls.rs +++ b/src/calls.rs @@ -135,8 +135,18 @@ impl CallInfo { } async fn mark_as_ended(&mut self, context: &Context) -> Result<()> { - self.msg.param.set_i64(CALL_ENDED_TIMESTAMP, time()); + let now = time(); + self.msg.param.set_i64(CALL_ENDED_TIMESTAMP, now); self.msg.update_param(context).await?; + + // Also store ended timestamp in calls table + context + .sql + .execute( + "UPDATE calls SET ended_timestamp=? WHERE msg_id=?", + (now, self.msg.id), + ) + .await?; Ok(()) } @@ -152,6 +162,15 @@ impl CallInfo { self.msg.param.set_i64(CALL_ENDED_TIMESTAMP, now); self.msg.param.set_i64(CALL_CANCELED_TIMESTAMP, now); self.msg.update_param(context).await?; + + // Also store ended timestamp in calls table + context + .sql + .execute( + "UPDATE calls SET ended_timestamp=? WHERE msg_id=?", + (now, self.msg.id), + ) + .await?; Ok(()) } @@ -193,27 +212,19 @@ impl Context { let mut call = Message { viewtype: Viewtype::Call, text: "Outgoing call".into(), + call_sdp_offer: Some(place_call_info.clone()), ..Default::default() }; - // Set a placeholder parameter so the message is recognized as a call during sending. - // This ensures mimefactory can read the SDP during the brief window between - // send_msg() starting and the calls table entry being available. - // The param will be removed after send_msg() completes. - call.param.set(Param::WebrtcRoom, &place_call_info); call.id = send_msg(self, chat_id, &mut call).await?; - // Store SDP offer in calls table and remove from params + // Store SDP offer in calls table self.sql .execute( "INSERT OR REPLACE INTO calls (msg_id, offer_sdp) VALUES (?, ?)", (call.id, &place_call_info), ) .await?; - - // Remove SDP from message params for privacy - call.param.remove(Param::WebrtcRoom); - call.update_param(self).await?; let wait = RINGING_SECONDS; task::spawn(Context::emit_end_call_if_unaccepted( diff --git a/src/message.rs b/src/message.rs index 702b730dc..516604e66 100644 --- a/src/message.rs +++ b/src/message.rs @@ -443,6 +443,13 @@ pub struct Message { pub(crate) location_id: u32, pub(crate) error: Option, pub(crate) param: Params, + + /// SDP offer for outgoing calls. + /// This field is used to pass the SDP offer to the database + /// without storing it in message parameters. + /// It is not persisted in the msgs table, only in the calls table. + #[serde(skip)] + pub(crate) call_sdp_offer: Option, } impl Message { @@ -572,6 +579,7 @@ impl Message { chat_blocked: row .get::<_, Option>("blocked")? .unwrap_or_default(), + call_sdp_offer: None, }; Ok(msg) }, diff --git a/src/mimefactory.rs b/src/mimefactory.rs index ea4e5f72d..61a0518dd 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1750,17 +1750,24 @@ impl MimeFactory { } if msg.viewtype == Viewtype::Call { - // Get SDP offer from calls table, or fall back to params if not yet migrated - let offer_sdp = context - .sql - .query_row_optional( - "SELECT offer_sdp FROM calls WHERE msg_id=?", - (msg.id,), - |row| row.get::<_, Option>(0), - ) - .await? - .flatten() - .or_else(|| msg.param.get(Param::WebrtcRoom).map(|s| s.to_string())); + // Get SDP offer from the message field (if being sent), calls table, or params (for old messages) + let offer_sdp = if let Some(ref offer) = msg.call_sdp_offer { + Some(offer.clone()) + } else if !msg.id.is_unset() { + // Try to get from calls table if message is already stored + context + .sql + .query_row_optional( + "SELECT offer_sdp FROM calls WHERE msg_id=?", + (msg.id,), + |row| row.get::<_, Option>(0), + ) + .await? + .flatten() + } else { + None + } + .or_else(|| msg.param.get(Param::WebrtcRoom).map(|s| s.to_string())); if let Some(offer_sdp) = offer_sdp { headers.push(( diff --git a/src/sql.rs b/src/sql.rs index f5d2a8cb4..162469178 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -923,10 +923,7 @@ pub async fn housekeeping(context: &Context) -> Result<()> { // Delete call SDPs for ended calls (older than 24 hours) or trashed calls. // We clean up calls that ended more than 24 hours ago to protect privacy - // as SDPs contain IP addresses. Ended calls are identified by having - // the CALL_ENDED_TIMESTAMP parameter (Param::Arg4='H') set. - // The pattern '%H=%' matches this parameter since params are stored as - // newline-separated key=value pairs (e.g., "E=123\nH=456\n"). + // as SDPs contain IP addresses. // The ON DELETE CASCADE foreign key handles orphaned entries automatically. context .sql @@ -935,8 +932,8 @@ pub async fn housekeeping(context: &Context) -> Result<()> { SELECT calls.msg_id FROM calls INNER JOIN msgs ON calls.msg_id = msgs.id WHERE msgs.chat_id = ? - OR (msgs.param LIKE '%H=%' - AND msgs.timestamp_sent < ?) + OR (calls.ended_timestamp IS NOT NULL + AND calls.ended_timestamp < ?) )", (DC_CHAT_ID_TRASH, time() - 86400), ) diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 6faa838c8..ff1151447 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1343,12 +1343,11 @@ CREATE INDEX gossip_timestamp_index ON gossip_timestamp (chat_id, fingerprint); if dbversion < migration_version { sql.execute_migration( "CREATE TABLE calls( - id INTEGER PRIMARY KEY AUTOINCREMENT, - msg_id INTEGER NOT NULL UNIQUE REFERENCES msgs(id) ON DELETE CASCADE, + msg_id INTEGER PRIMARY KEY REFERENCES msgs(id) ON DELETE CASCADE, offer_sdp TEXT, - answer_sdp TEXT - ) STRICT; - CREATE INDEX calls_msg_id_index ON calls (msg_id);", + answer_sdp TEXT, + ended_timestamp INTEGER + ) STRICT;", migration_version, ) .await?;