From f4855962fce1e85d6a47f24751a1c53be917626e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Revillard?= Date: Tue, 3 Mar 2026 17:16:02 +0100 Subject: [PATCH] fix: use std::sync::RwLock in MessageTool to avoid runtime panic (#411) * fix: use std::sync::RwLock in MessageTool to avoid runtime panic The `requires_approval` method is synchronous but was using `tokio::sync::RwLock` with `.await` which requires blocking the runtime. This caused a panic: "Cannot block the current thread from within a runtime" Changes: - Replace `tokio::sync::RwLock` with `std::sync::RwLock` for `default_channel` and `default_target` fields - Use `unwrap_or_else(|e| e.into_inner())` to gracefully handle poisoned locks (recovers instead of panicking) - Update all usages from `.read().await` to `.read().unwrap_or_else()` The locks are short-held (just cloning strings), making std::sync::RwLock appropriate for sync methods called from async contexts. Fixes: "Cannot block the current thread from within a runtime" panic when the LLM tries to send a message via the message tool. Co-Authored-By: Claude Opus 4.6 * fix: address code review feedback for MessageTool RwLock fix - Fix formatting (long lines broken up per rustfmt) - Add regression test that demonstrates the panic with tokio::sync::RwLock and passes with std::sync::RwLock when calling requires_approval() (sync method) from async context Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/tools/builtin/message.rs | 89 +++++++++++++++++++++++++++++------- 1 file changed, 72 insertions(+), 17 deletions(-) diff --git a/src/tools/builtin/message.rs b/src/tools/builtin/message.rs index e2690b02..78592ad4 100644 --- a/src/tools/builtin/message.rs +++ b/src/tools/builtin/message.rs @@ -3,10 +3,9 @@ //! Allows the agent to proactively message users on any connected channel. use std::path::PathBuf; -use std::sync::Arc; +use std::sync::{Arc, RwLock}; use async_trait::async_trait; -use tokio::sync::RwLock; use crate::bootstrap::ironclaw_base_dir; use crate::channels::{ChannelManager, OutgoingResponse}; @@ -19,6 +18,7 @@ use crate::tools::tool::{ pub struct MessageTool { channel_manager: Arc, /// Default channel for current conversation (set per-turn). + /// Uses std::sync::RwLock because requires_approval() is sync and called from async context. default_channel: Arc>>, /// Default target (user_id or group_id) for current conversation (set per-turn). default_target: Arc>>, @@ -48,8 +48,14 @@ impl MessageTool { /// Set the default channel and target for the current conversation turn. /// Call this before each agent turn with the incoming message's channel/target. pub async fn set_context(&self, channel: Option, target: Option) { - *self.default_channel.write().await = channel; - *self.default_target.write().await = target; + *self + .default_channel + .write() + .unwrap_or_else(|e| e.into_inner()) = channel; + *self + .default_target + .write() + .unwrap_or_else(|e| e.into_inner()) = target; } } @@ -106,24 +112,32 @@ impl Tool for MessageTool { let channel = if let Some(c) = params.get("channel").and_then(|v| v.as_str()) { c.to_string() } else { - self.default_channel.read().await.clone().ok_or_else(|| { - ToolError::ExecutionFailed( - "No channel specified and no active conversation. Provide channel parameter." - .to_string(), - ) - })? + self.default_channel + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone() + .ok_or_else(|| { + ToolError::ExecutionFailed( + "No channel specified and no active conversation. Provide channel parameter." + .to_string(), + ) + })? }; // Get target: use param or fall back to default let target = if let Some(t) = params.get("target").and_then(|v| v.as_str()) { t.to_string() } else { - self.default_target.read().await.clone().ok_or_else(|| { - ToolError::ExecutionFailed( - "No target specified and no active conversation. Provide target parameter." - .to_string(), - ) - })? + self.default_target + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone() + .ok_or_else(|| { + ToolError::ExecutionFailed( + "No target specified and no active conversation. Provide target parameter." + .to_string(), + ) + })? }; let attachments: Vec = match params.get("attachments") { @@ -199,7 +213,10 @@ impl Tool for MessageTool { let param_channel = params.get("channel").and_then(|v| v.as_str()); if let Some(channel) = param_channel { // Check if it differs from the default channel - let default_channel = self.default_channel.blocking_read(); + let default_channel = self + .default_channel + .read() + .unwrap_or_else(|e| e.into_inner()); if let Some(default) = default_channel.as_ref() && channel != default { @@ -515,4 +532,42 @@ mod tests { err ); } + + /// Regression test: requires_approval() is a sync method called from async context. + /// With tokio::sync::RwLock, this would panic with: + /// "Cannot block the current thread from within a runtime" + /// because blocking_read() cannot be called inside an async runtime. + /// With std::sync::RwLock, it works correctly since std locks are safe + /// for short-held locks in sync methods called from async contexts. + #[tokio::test] + async fn requires_approval_works_from_async_context() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + + // Set context asynchronously (simulating real usage pattern) + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Call requires_approval (sync method) from async context. + // This is the critical test: with tokio::sync::RwLock::blocking_read(), + // this would panic. With std::sync::RwLock::read(), it works. + let approval = tool.requires_approval(&serde_json::json!({ + "content": "hello", + "channel": "telegram" + })); + // Different channel from default -> Always + assert!(matches!(approval, ApprovalRequirement::Always)); + + // No channel specified (uses default) -> UnlessAutoApproved + let approval = tool.requires_approval(&serde_json::json!({ + "content": "hello" + })); + assert!(matches!(approval, ApprovalRequirement::UnlessAutoApproved)); + + // Explicit channel (even if same as default) -> Always + let approval = tool.requires_approval(&serde_json::json!({ + "content": "hello", + "channel": "signal" + })); + assert!(matches!(approval, ApprovalRequirement::Always)); + } }