fix(python): release the GIL while Shell runs a command

[schinchli]

Description

The Python Shell methods drive the tokio runtime with self.runtime.block_on(...) while holding the GIL. So while a command runs, no other Python thread runs and an asyncio event loop stalls. The repro from #119 on 0.3.3: a background thread gets 1 tick during run("sleep 2"), against 1,588 during a plain time.sleep(2).

cli_main in the same file already releases the GIL with py.detach(...). The methods on Shell can't do the same directly, because py.detach requires an Ungil (Send) closure, and the closure captures two things that aren't Send:

• shell::Shell, which holds Rc<Vec<NamedMcpClient>>. This is the same constraint the comment on Worker in js.rs describes.
• the tokio::task::LocalSet.

This PR adds one private helper, block_on_detached, and routes run, read_file, write_file, remove_file and list_files through it:

• The LocalSet is created inside the closure, so it never has to cross detach.
• The future, which borrows &mut shell::Shell, crosses detach in a small SameThread wrapper with unsafe impl Send.

Why the unsafe impl is sound. Python::detach runs the closure on the calling thread, so the Rc never actually changes thread. Shell is #[pyclass(unsendable)], so PyO3 rejects a call from any other thread, and the &mut self borrow held across detach rules out re-entry on the same thread. I checked the cross-thread case on both builds: a second thread calling run on the same Shell while a command is running gets PyO3's unsendable PanicException, the same as on 0.3.3.

The alternative, if you'd rather have no unsafe: mirror the Node binding and run the shell on a dedicated worker thread. That's a larger change to Shell's structure, so I went with the smaller one first. Happy to switch.

Related Issues

Fixes #119

Documentation PR

None needed. No public API changes.

Type of Change

Bug fix

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.

• I ran the relevant test suites for the bindings I touched (cargo test --workspace --all-targets, pytest tests/python, npm test)
• If I touched Rust, I ran cargo fmt and cargo clippy

Details:

• New test_run_releases_the_gil counts a background thread's ticks during run("sleep 0.5"). It fails on 0.3.3 (assert 1 > 50) and passes with this change; I ran it 5 times in a row with no failures.
• pytest tests/python: 45 passed (maturin develop --release, Python 3.14, macOS arm64).
• cargo test --workspace --all-targets: 1,749 passed, 0 failed.
• cargo fmt --check is clean, and RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --features python builds. cargo clippy --features python --lib reports nothing in src/python.rs; the lints it does report are in other files and already exist on main.
• Manual checks with the patched build: the [BUG] Shell.run() holds the GIL while a command runs #119 repro now gives 1,337 ticks, against 1,343 for time.sleep(2). An asyncio task keeps ticking during asyncio.to_thread(lambda: Shell().run("sleep 1")). Shell state (X=42, then echo $X) persists across calls. write_file, read_file, list_files and remove_file behave as before.
• npm test not run: the Node binding is untouched.

Checklist

• I have read the CONTRIBUTING document
• I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
• My change is focused and reasonably small; I have split unrelated work into separate PRs
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[JackYPCOnline]

@strandly-the-agent can you review it? I dont know if it is designed?

[strandly-the-agent]

Approve — the design is right and the unsafe impl Send is sound. Two should-fix comment/doc nits inline; nothing blocking, no public API change.

To JackYPCOnline's question — is this the intended design? Yes, in my view:

• Sound, and the pattern PyO3 itself describes. Python::detach runs its closure inline on the calling thread (pyo3 0.29 marker.rs:562-573), so nothing in SameThread ever changes thread. PyO3's own docs say "there is no reason to prevent !Send types like Rc from crossing the closure… detach does not itself launch a new thread". The real invariant is no Python references inside the closure, and that holds: nothing outside src/python.rs uses pyo3 (grep -rn pyo3 src --include=*.rs | grep -v python.rs → 0 hits).
• Smallest fix. It is the first unsafe in src/ (only test env-var code had one), confined to one private type. The SAFETY comment argues the wrong invariant, though — see inline.
• Worker-thread alternative (mirroring js.rs) would be more code but would also lift the unsendable restriction: today a Shell can only be used from the thread that created it, so asyncio.to_thread(shell.run, …) or a thread-pool tool raises PanicException. That is pre-existing and orthogonal to #119 — I'd keep this PR as-is and track the cross-thread story as a follow-up issue if you want it.

✅ Verified (head 8a5637c, Python 3.13.15, aarch64)

• maturin develop --release + pytest tests/python -v → 45 passed; test_run_releases_the_gil passed 5/5 reruns.
• Issue #119 repro: 1890 ticks during Shell().run("sleep 2") vs 1884 during time.sleep(2).
• Cross-thread call on a shared Shell → PanicException (same as main; the check is in PyO3's trampoline, untouched). 4 Shells on 4 threads × sleep 1 → wall 1.00s. Shell(timeout=0.3).run("sleep 5") returns at 0.30s through detach. Panic inside a command leaves the Shell, GIL and other threads usable. Drop on another thread → PyO3's unraisable + leak, pre-existing.
• Test margin: during > 50 measured 471–476 ticks idle and with 2 external busy processes; drops to 9–44 only with CPU-bound Python threads in the same process, which the suite doesn't have.
• cargo fmt --check clean; cargo clippy --features python --lib → 0 warnings in src/python.rs (6 pre-existing elsewhere).
• All five block_on sites converted; no block_on left outside the helper. No .pyi stubs or CHANGELOG to update.
• Repro scripts/outputs and the pytest log are uploaded as review artifacts.

Questions (non-blocking)

1. ShellBuilder.build (src/python.rs:414-423) still holds the GIL, and that's where copy-mode binds do their host I/O (vfs::copy_from_host, src/vfs_config.rs:273-275); js.rs:389 treats build as async for that reason. Measured 140 ms / 1 tick for a 93 MB copy bind. Deliberately out of scope for #119?
2. Want a follow-up issue for the unsendable / cross-thread limitation (worker-thread design)? I can file it.

Reading order

src/python.rs:426-450 (wrapper + helper) → one call site, e.g. :478-492 → tests/python/test_bindings.py:273-294.

Appendix — non-blocking (3)

• ⚪ src/python.rs module doc has no threading-model note while src/js.rs:12-20 does; a 2–3 line "GIL released per I/O method, runtime runs inline, cross-thread use rejected by unsendable" would keep the mirror symmetric (AGENTS.md:26).
• ⚪ python/strands_shell/__init__.py:276-286 class docstring never says a Shell must be used on its creating thread; the asyncio users this fixes will hit PanicException (a BaseException) from asyncio.to_thread. Pre-existing.
• Pre-existing, not filed (contrived input): sleep 1e300 panics in src/commands/sleep.rs:21 and the command reports status=0, empty stderr. Say so if you'd like an issue.

[strandly-the-agent]

🟡 The SAFETY comment argues the wrong invariant. unsendable and the &mut self borrow are about re-entry/aliasing of Shell; neither is why asserting Send is OK. The actual reasons: Python::detach runs the closure synchronously on the calling thread (nothing crosses a thread), and the wrapped value holds no Python references — which is what PyO3's Send-as-Ungil bound is really guarding (its docs call smuggling a Bound through such a wrapper unsound). The unconditional impl<T> is also visible to the whole module, so a later thread::spawn(move || job.into_inner()) compiles with no comment saying it's UB.

Suggestion (optionally also move the struct + impls inside block_on_detached so misuse isn't expressible — that variant compiles and cargo fmt --check is clean):

Suggested change

	// SAFETY: `Shell` is `unsendable`, so PyO3 rejects calls from any other
	// thread, and the `&mut self` borrow held across `detach` rules out re-entry
	// on this thread while the GIL is released.
	// SAFETY: `Python::detach` runs its closure synchronously on the calling
	// thread, so the wrapped value never crosses a thread boundary. `T` must hold
	// no Python references (`Py`, `Bound`, `Python`); the `Send` bound on `detach`
	// is PyO3's stand-in for that check, and this impl bypasses it.

[schinchli]

Fixed in 97b3df5. I used your SAFETY wording and also took the optional suggestion: SameThread and its impls now live inside block_on_detached, so the unsafe Send can't be reused elsewhere.

I checked the reasoning against pyo3 0.29.2 (the version in Cargo.lock). Python::detach calls f() directly under a SuspendAttach guard on the calling thread, and without the nightly feature Ungil is unsafe impl<T: Send> Ungil for T, so the Send bound is exactly the stand-in for "holds no Python references".

[strandly-the-agent]

🟡 "the asyncio event loop keep[s] running" over-claims. It only holds when run is called off the loop thread. A coroutine calling shell.run() directly still stalls the loop, and the obvious escape hatch — await asyncio.to_thread(shell.run, …) with the Shell built on the loop thread — raises PanicException because the class is unsendable (verified on this build). The only working async pattern is a single-worker executor that both creates and uses the Shell, and nothing on the user-facing surface says so (python/strands_shell/__init__.py:359-361 only says "Run a command and capture its output").

Suggested change

	/// Drives `fut` to completion with the GIL released, so other Python threads
	/// and the asyncio event loop keep running while a command executes.
	/// Drives `fut` to completion with the GIL released, so other Python threads
	/// (and an asyncio loop on another thread) keep running while a command executes.

Suggestion for the Python docstring at __init__.py:359: "Blocking; releases the GIL while the command runs. A Shell must be created and used on one thread, so from async code run it in a single-worker executor rather than asyncio.to_thread."

[schinchli]

Fixed in 97b3df5. The Rust doc now says "(and an asyncio loop on another thread)", and Shell.run has the docstring you suggested, with one addition from testing: the Shell must also be dropped on its worker thread. Otherwise pyo3 raises RuntimeError: ... unsendable, but is being dropped on another thread when it's garbage-collected from the loop thread.

I ran each case against a release build of this branch:

• asyncio.to_thread(shell.run, ...) with the Shell built on the loop thread: PanicException, as you said.
• Single-worker executor that creates, uses and drops the Shell: the command completes and the loop keeps ticking (~4,100 ticks during a ~4.5s command).
• Plain thread: a second Python thread made ~3,800 ticks while run() was executing, so the GIL is released.

[schinchli]

@JackYPCOnline both review points from @strandly-the-agent are addressed in 97b3df5 (one commit on top of the original fix, docs/comments plus a scoping move, no behavior change). Details are in the two threads above.

Checked locally against what ci.yml runs:

• cargo test --workspace --all-targets: 1749 passed, 0 failed
• cargo doc --workspace --no-deps: clean
• maturin develop --release + pytest tests/python: 45 passed, including the GIL regression test
• cargo fmt --check: clean

The CI and PR-title workflows show action_required because this comes from a fork. Could you approve the workflow run? It should be ready to merge after that.