diff --git a/src/openhuman/approval/gate.rs b/src/openhuman/approval/gate.rs index cf09480f2c..e625e872a4 100644 --- a/src/openhuman/approval/gate.rs +++ b/src/openhuman/approval/gate.rs @@ -203,6 +203,72 @@ pub struct ApprovalGate { meeting_to_request: Mutex>, } +/// Cleans up a parked approval if its future is dropped before the normal +/// decision/timeout path can run (for example, by an outer turn deadline). +struct ParkedApprovalCleanupGuard<'a> { + gate: &'a ApprovalGate, + request_id: String, + thread_id: Option, + in_call_ctx: Option, + armed: bool, +} + +impl<'a> ParkedApprovalCleanupGuard<'a> { + fn new( + gate: &'a ApprovalGate, + request_id: String, + thread_id: Option, + in_call_ctx: Option, + ) -> Self { + Self { + gate, + request_id, + thread_id, + in_call_ctx, + armed: true, + } + } + + fn disarm(&mut self) { + self.armed = false; + } +} + +impl Drop for ParkedApprovalCleanupGuard<'_> { + fn drop(&mut self) { + if !self.armed { + return; + } + + tracing::warn!( + request_id = %self.request_id, + "[approval::gate] parked approval future dropped externally; cleaning up" + ); + + // Remove in-memory routing first so a storage failure cannot leave a + // later yes/no reply pointed at a waiter that no longer exists. + self.gate.evict_waiter(&self.request_id); + self.gate.clear_thread(&self.thread_id, &self.request_id); + self.gate.clear_meeting(&self.in_call_ctx, &self.request_id); + + match store::decide(&self.gate.config, &self.request_id, ApprovalDecision::Deny) { + Ok(Some(_)) => tracing::debug!( + request_id = %self.request_id, + "[approval::gate] externally dropped approval marked denied" + ), + Ok(None) => tracing::debug!( + request_id = %self.request_id, + "[approval::gate] externally dropped approval was already resolved" + ), + Err(err) => tracing::error!( + request_id = %self.request_id, + error = %err, + "[approval::gate] failed to persist denial for externally dropped approval" + ), + } + } +} + impl ApprovalGate { /// Install the process-global gate. Returns the existing gate if /// one was already installed (re-install is a no-op so repeated @@ -643,7 +709,8 @@ impl ApprovalGate { if let Err(err) = store::insert_pending(&self.config, &pending, &self.session_id) { self.evict_waiter(&request_id); - self.clear_thread(&chat_thread_id); + self.clear_thread(&chat_thread_id, &request_id); + self.clear_meeting(&in_call_ctx, &request_id); tracing::error!( error = %err, tool = tool_name, @@ -660,6 +727,13 @@ impl ApprovalGate { ); } + let mut cleanup_guard = ParkedApprovalCleanupGuard::new( + self, + request_id.clone(), + chat_thread_id.clone(), + in_call_ctx.clone(), + ); + tracing::info!( request_id = %request_id, tool = tool_name, @@ -743,7 +817,7 @@ impl ApprovalGate { "[approval::gate] decision received" ); if decision.is_approve() { - (GateOutcome::Allow, Some(request_id)) + (GateOutcome::Allow, Some(request_id.clone())) } else { ( GateOutcome::Deny { @@ -804,7 +878,7 @@ impl ApprovalGate { // on this path too — otherwise the stale thread→request // mapping survives and the next yes/no on the thread could be // routed to this already-finished request. - (GateOutcome::Allow, Some(request_id)) + (GateOutcome::Allow, Some(request_id.clone())) } else { tracing::warn!( request_id = %request_id, @@ -828,8 +902,9 @@ impl ApprovalGate { }; // The routing mappings are only needed while parked; clear them on // every exit (decision, channel drop, or timeout). - self.clear_thread(&chat_thread_id); - self.clear_meeting(&in_call_ctx); + self.clear_thread(&chat_thread_id, &request_id); + self.clear_meeting(&in_call_ctx, &request_id); + cleanup_guard.disarm(); outcome } @@ -998,17 +1073,26 @@ impl ApprovalGate { self.meeting_to_request.lock().get(meeting_key).cloned() } - /// Drop the thread → request mapping (best-effort; no-op when absent). - fn clear_thread(&self, thread_id: &Option) { + /// Drop the thread → request mapping when it still belongs to this request. + fn clear_thread(&self, thread_id: &Option, request_id: &str) { if let Some(t) = thread_id { - self.thread_to_request.lock().remove(t); + let mut routes = self.thread_to_request.lock(); + if routes.get(t).is_some_and(|current| current == request_id) { + routes.remove(t); + } } } - /// Drop the meeting → request mapping (best-effort; no-op when absent). - fn clear_meeting(&self, ctx: &Option) { + /// Drop the meeting → request mapping when it still belongs to this request. + fn clear_meeting(&self, ctx: &Option, request_id: &str) { if let Some(ic) = ctx { - self.meeting_to_request.lock().remove(&ic.meeting_key); + let mut routes = self.meeting_to_request.lock(); + if routes + .get(&ic.meeting_key) + .is_some_and(|current| current == request_id) + { + routes.remove(&ic.meeting_key); + } } } } @@ -1322,6 +1406,228 @@ mod tests { } } + #[tokio::test] + async fn externally_aborted_chat_waiter_cleans_pending_state() { + let (gate, _dir) = test_gate(); + let gate = Arc::new(gate); + + let g = gate.clone(); + let handle = tokio::spawn(async move { + turn_origin::with_origin( + web_origin(), + APPROVAL_CHAT_CONTEXT.scope( + chat_ctx(), + g.intercept("composio", "send slack", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let request_id = loop { + if let Some(request_id) = gate.pending_for_thread("t-test") { + break request_id; + } + tries += 1; + assert!(tries < 1_000, "chat approval route never appeared"); + tokio::task::yield_now().await; + }; + assert!(gate.waiters.lock().contains_key(&request_id)); + + handle.abort(); + assert!(handle.await.unwrap_err().is_cancelled()); + + assert!(gate.pending_for_thread("t-test").is_none()); + assert!(!gate.waiters.lock().contains_key(&request_id)); + assert!(gate.list_pending().unwrap().is_empty()); + assert_eq!( + store::get_decision(&gate.config, &request_id).unwrap(), + Some(ApprovalDecision::Deny) + ); + } + + #[tokio::test] + async fn aborting_older_chat_waiter_preserves_newer_thread_route() { + let (gate, _dir) = test_gate(); + let gate = Arc::new(gate); + + let old_gate = gate.clone(); + let old_handle = tokio::spawn(async move { + turn_origin::with_origin( + web_origin(), + APPROVAL_CHAT_CONTEXT.scope( + chat_ctx(), + old_gate.intercept("composio", "old action", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let old_request_id = loop { + if let Some(request_id) = gate.pending_for_thread("t-test") { + break request_id; + } + tries += 1; + assert!(tries < 1_000, "old chat approval route never appeared"); + tokio::task::yield_now().await; + }; + + let new_gate = gate.clone(); + let new_handle = tokio::spawn(async move { + turn_origin::with_origin( + web_origin(), + APPROVAL_CHAT_CONTEXT.scope( + chat_ctx(), + new_gate.intercept("composio", "new action", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let new_request_id = loop { + if let Some(request_id) = gate.pending_for_thread("t-test") { + if request_id != old_request_id { + break request_id; + } + } + tries += 1; + assert!(tries < 1_000, "new chat approval route never appeared"); + tokio::task::yield_now().await; + }; + + old_handle.abort(); + assert!(old_handle.await.unwrap_err().is_cancelled()); + + assert_eq!( + gate.pending_for_thread("t-test").as_deref(), + Some(new_request_id.as_str()) + ); + assert!(!gate.waiters.lock().contains_key(&old_request_id)); + assert!(gate.waiters.lock().contains_key(&new_request_id)); + assert_eq!( + store::get_decision(&gate.config, &old_request_id).unwrap(), + Some(ApprovalDecision::Deny) + ); + + gate.decide(&new_request_id, ApprovalDecision::ApproveOnce) + .unwrap(); + assert!(matches!(new_handle.await.unwrap(), GateOutcome::Allow)); + assert!(gate.pending_for_thread("t-test").is_none()); + } + + #[tokio::test] + async fn externally_aborted_in_call_waiter_cleans_meeting_route() { + let (gate, _dir) = test_gate(); + let gate = Arc::new(gate); + + let g = gate.clone(); + let handle = tokio::spawn(async move { + turn_origin::with_origin( + meet_origin(), + APPROVAL_IN_CALL_CONTEXT.scope( + in_call_ctx(), + g.intercept("composio", "send email", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let request_id = loop { + if let Some(request_id) = gate.pending_for_meeting("meet-1") { + break request_id; + } + tries += 1; + assert!(tries < 1_000, "meeting approval route never appeared"); + tokio::task::yield_now().await; + }; + assert!(gate.waiters.lock().contains_key(&request_id)); + + handle.abort(); + assert!(handle.await.unwrap_err().is_cancelled()); + + assert!(gate.pending_for_meeting("meet-1").is_none()); + assert!(!gate.waiters.lock().contains_key(&request_id)); + assert!(gate.list_pending().unwrap().is_empty()); + assert_eq!( + store::get_decision(&gate.config, &request_id).unwrap(), + Some(ApprovalDecision::Deny) + ); + } + + #[tokio::test] + async fn aborting_older_in_call_waiter_preserves_newer_meeting_route() { + let (gate, _dir) = test_gate(); + let gate = Arc::new(gate); + + let old_gate = gate.clone(); + let old_handle = tokio::spawn(async move { + turn_origin::with_origin( + meet_origin(), + APPROVAL_IN_CALL_CONTEXT.scope( + in_call_ctx(), + old_gate.intercept("composio", "old action", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let old_request_id = loop { + if let Some(request_id) = gate.pending_for_meeting("meet-1") { + break request_id; + } + tries += 1; + assert!(tries < 1_000, "old meeting approval route never appeared"); + tokio::task::yield_now().await; + }; + + let new_gate = gate.clone(); + let new_handle = tokio::spawn(async move { + turn_origin::with_origin( + meet_origin(), + APPROVAL_IN_CALL_CONTEXT.scope( + in_call_ctx(), + new_gate.intercept("composio", "new action", serde_json::json!({})), + ), + ) + .await + }); + + let mut tries = 0; + let new_request_id = loop { + if let Some(request_id) = gate.pending_for_meeting("meet-1") { + if request_id != old_request_id { + break request_id; + } + } + tries += 1; + assert!(tries < 1_000, "new meeting approval route never appeared"); + tokio::task::yield_now().await; + }; + + old_handle.abort(); + assert!(old_handle.await.unwrap_err().is_cancelled()); + + assert_eq!( + gate.pending_for_meeting("meet-1").as_deref(), + Some(new_request_id.as_str()) + ); + assert!(!gate.waiters.lock().contains_key(&old_request_id)); + assert!(gate.waiters.lock().contains_key(&new_request_id)); + assert_eq!( + store::get_decision(&gate.config, &old_request_id).unwrap(), + Some(ApprovalDecision::Deny) + ); + + gate.decide(&new_request_id, ApprovalDecision::ApproveOnce) + .unwrap(); + assert!(matches!(new_handle.await.unwrap(), GateOutcome::Allow)); + assert!(gate.pending_for_meeting("meet-1").is_none()); + } + #[tokio::test] async fn auto_approve_tool_skips_prompt() { // The gate reads the "Always allow" allowlist from the process-global