diff --git a/crates/bashkit-python/src/lib.rs b/crates/bashkit-python/src/lib.rs index c49a8405..81abb30c 100644 --- a/crates/bashkit-python/src/lib.rs +++ b/crates/bashkit-python/src/lib.rs @@ -233,41 +233,47 @@ impl PyBash { } /// Execute commands synchronously (blocking). - fn execute_sync(&self, commands: String) -> PyResult { + /// Releases GIL before blocking on tokio to prevent deadlock with callbacks. + fn execute_sync(&self, py: Python<'_>, commands: String) -> PyResult { let inner = self.inner.clone(); let rt = tokio::runtime::Runtime::new() .map_err(|e| PyRuntimeError::new_err(format!("Failed to create runtime: {}", e)))?; - rt.block_on(async move { - let mut bash = inner.lock().await; - match bash.exec(&commands).await { - Ok(result) => Ok(ExecResult { - stdout: result.stdout, - stderr: result.stderr, - exit_code: result.exit_code, - error: None, - }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), - } + py.detach(|| { + rt.block_on(async move { + let mut bash = inner.lock().await; + match bash.exec(&commands).await { + Ok(result) => Ok(ExecResult { + stdout: result.stdout, + stderr: result.stderr, + exit_code: result.exit_code, + error: None, + }), + Err(e) => Ok(ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: 1, + error: Some(e.to_string()), + }), + } + }) }) } /// Reset interpreter to fresh state. - fn reset(&self) -> PyResult<()> { + /// Releases GIL before blocking on tokio to prevent deadlock. + fn reset(&self, py: Python<'_>) -> PyResult<()> { let inner = self.inner.clone(); let rt = tokio::runtime::Runtime::new() .map_err(|e| PyRuntimeError::new_err(format!("Failed to create runtime: {}", e)))?; - rt.block_on(async move { - let mut bash = inner.lock().await; - let builder = Bash::builder(); - *bash = builder.build(); - Ok(()) + py.detach(|| { + rt.block_on(async move { + let mut bash = inner.lock().await; + let builder = Bash::builder(); + *bash = builder.build(); + Ok(()) + }) }) } @@ -378,40 +384,46 @@ impl BashTool { }) } - fn execute_sync(&self, commands: String) -> PyResult { + /// Releases GIL before blocking on tokio to prevent deadlock with callbacks. + fn execute_sync(&self, py: Python<'_>, commands: String) -> PyResult { let inner = self.inner.clone(); let rt = tokio::runtime::Runtime::new() .map_err(|e| PyRuntimeError::new_err(format!("Failed to create runtime: {}", e)))?; - rt.block_on(async move { - let mut bash = inner.lock().await; - match bash.exec(&commands).await { - Ok(result) => Ok(ExecResult { - stdout: result.stdout, - stderr: result.stderr, - exit_code: result.exit_code, - error: None, - }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), - } + py.detach(|| { + rt.block_on(async move { + let mut bash = inner.lock().await; + match bash.exec(&commands).await { + Ok(result) => Ok(ExecResult { + stdout: result.stdout, + stderr: result.stderr, + exit_code: result.exit_code, + error: None, + }), + Err(e) => Ok(ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: 1, + error: Some(e.to_string()), + }), + } + }) }) } - fn reset(&self) -> PyResult<()> { + /// Releases GIL before blocking on tokio to prevent deadlock. + fn reset(&self, py: Python<'_>) -> PyResult<()> { let inner = self.inner.clone(); let rt = tokio::runtime::Runtime::new() .map_err(|e| PyRuntimeError::new_err(format!("Failed to create runtime: {}", e)))?; - rt.block_on(async move { - let mut bash = inner.lock().await; - let builder = Bash::builder(); - *bash = builder.build(); - Ok(()) + py.detach(|| { + rt.block_on(async move { + let mut bash = inner.lock().await; + let builder = Bash::builder(); + *bash = builder.build(); + Ok(()) + }) }) } @@ -652,17 +664,20 @@ impl ScriptedTool { } /// Execute a bash script synchronously (blocking). - fn execute_sync(&self, commands: String) -> PyResult { + /// Releases GIL before blocking on tokio to prevent deadlock with callbacks. + fn execute_sync(&self, py: Python<'_>, commands: String) -> PyResult { let mut tool = self.build_rust_tool(); let rt = tokio::runtime::Runtime::new() .map_err(|e| PyRuntimeError::new_err(format!("Failed to create runtime: {}", e)))?; - let resp = rt.block_on(async move { - tool.execute(ToolRequest { - commands, - timeout_ms: None, + let resp = py.detach(|| { + rt.block_on(async move { + tool.execute(ToolRequest { + commands, + timeout_ms: None, + }) + .await }) - .await }); Ok(ExecResult { stdout: resp.stdout, diff --git a/crates/bashkit-python/tests/test_bashkit.py b/crates/bashkit-python/tests/test_bashkit.py index 76b369fb..8a15eea2 100644 --- a/crates/bashkit-python/tests/test_bashkit.py +++ b/crates/bashkit-python/tests/test_bashkit.py @@ -728,3 +728,123 @@ async def test_async_multiple_tools(): assert r.exit_code == 0 assert "A" in r.stdout assert "B" in r.stdout + + +# =========================================================================== +# GIL deadlock prevention tests +# =========================================================================== + + +def test_execute_sync_releases_gil_for_callback(): + """execute_sync must release GIL before blocking on tokio runtime. + + Without py.allow_threads(), a tool callback calling Python::attach() + inside rt.block_on() can deadlock when another thread holds the GIL. + This test validates the fix by running execute_sync with a callback + from a background thread while the main thread also holds the GIL. + """ + import threading + import time + + tool = ScriptedTool("api") + + def slow_callback(params, stdin=None): + """Callback that briefly sleeps, requiring GIL reacquisition.""" + time.sleep(0.01) + return f"ok-{params.get('id', 0)}\n" + + tool.add_tool( + "slow", + "Slow command", + callback=slow_callback, + schema={"type": "object", "properties": {"id": {"type": "integer"}}}, + ) + + results = [None, None] + errors = [None, None] + + def run_in_thread(idx): + try: + r = tool.execute_sync(f"slow --id {idx}") + results[idx] = r + except Exception as e: + errors[idx] = e + + t0 = threading.Thread(target=run_in_thread, args=(0,)) + t1 = threading.Thread(target=run_in_thread, args=(1,)) + + t0.start() + t1.start() + + # Timeout guards against deadlock — if GIL isn't released, threads block forever + t0.join(timeout=10) + t1.join(timeout=10) + + assert not t0.is_alive(), "Thread 0 deadlocked (GIL not released in execute_sync)" + assert not t1.is_alive(), "Thread 1 deadlocked (GIL not released in execute_sync)" + assert errors[0] is None, f"Thread 0 error: {errors[0]}" + assert errors[1] is None, f"Thread 1 error: {errors[1]}" + assert results[0].exit_code == 0 + assert results[1].exit_code == 0 + assert "ok-0" in results[0].stdout + assert "ok-1" in results[1].stdout + + +def test_bash_execute_sync_releases_gil(): + """Bash.execute_sync must release GIL before blocking on tokio runtime.""" + import threading + + bash = Bash() + results = [None, None] + errors = [None, None] + + def run_in_thread(idx): + try: + r = bash.execute_sync(f"echo thread-{idx}") + results[idx] = r + except Exception as e: + errors[idx] = e + + t0 = threading.Thread(target=run_in_thread, args=(0,)) + t1 = threading.Thread(target=run_in_thread, args=(1,)) + + t0.start() + t1.start() + + t0.join(timeout=10) + t1.join(timeout=10) + + assert not t0.is_alive(), "Thread 0 deadlocked (GIL not released)" + assert not t1.is_alive(), "Thread 1 deadlocked (GIL not released)" + assert errors[0] is None, f"Thread 0 error: {errors[0]}" + assert errors[1] is None, f"Thread 1 error: {errors[1]}" + + +def test_bashtool_execute_sync_releases_gil(): + """BashTool.execute_sync must release GIL before blocking on tokio runtime.""" + import threading + + tool = BashTool() + results = [None, None] + errors = [None, None] + + def run_in_thread(idx): + try: + r = tool.execute_sync(f"echo thread-{idx}") + results[idx] = r + except Exception as e: + errors[idx] = e + + t0 = threading.Thread(target=run_in_thread, args=(0,)) + t1 = threading.Thread(target=run_in_thread, args=(1,)) + + t0.start() + t1.start() + + t0.join(timeout=10) + t1.join(timeout=10) + + assert not t0.is_alive(), "Thread 0 deadlocked (GIL not released)" + assert not t1.is_alive(), "Thread 1 deadlocked (GIL not released)" + assert errors[0] is None, f"Thread 0 error: {errors[0]}" + assert errors[1] is None, f"Thread 1 error: {errors[1]}"