From 04370d9e49e822f5bd81791532d180f91acd2bfb Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Sat, 31 Oct 2026 09:36:28 +0000 Subject: [PATCH] fix(python): drain abandoned callback dispatch before interpreter exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A timed-out `execute_sync` async callback leaves its private-loop dispatch running on Tokio's blocking pool. Dropping a `JoinHandle` cannot cancel a *blocking* task, so that pool thread stays parked in `result_rx.recv()` with the GIL released. The atexit hook joined the private-loop worker — which hands the cancelled result to that thread at the very end of the hook — but never waited for the thread itself, because the dispatch was never enrolled in the result-delivery barrier. Its re-attach then raced `Py_FinalizeEx`, and losing that race aborts the process on CPython < 3.13: `take_gil` sees the thread must exit and calls `PyThread_exit_thread()`, whose `pthread_exit` forced unwind crosses Rust frames that never rethrow it, so glibc kills the process with "FATAL: exception not rethrown" (SIGABRT). Enroll the dispatch through a shared `enroll_python_work()` guard and hold it across its Python work and its own `Py`/session drops (a last session clone can run `PyPrivateAsyncLoop::drop`, which attaches), so the exit hook drains it with the GIL released before finalization. Past the boundary the task skips Python entirely, as result deliveries already did. Joining the private-loop worker proves the worker finished, not that the thread waiting on it has — record that in the knowledge bundle as TM-PY-030 variant 4. Observed as a flaky SIGABRT in `test_interpreter_exit_does_not_crash[abandoned]` on the Python 3.9 CI leg. --- crates/bashkit-python/src/async_bridge.rs | 31 ++++++++++----- crates/bashkit-python/src/lib.rs | 25 +++++++++++- .../tests/test_teardown_determinism.py | 38 ++++++++++++++++++- knowledge/runtimes/python-package.md | 11 ++++++ knowledge/security/threat-model.md | 20 ++++++++-- 5 files changed, 110 insertions(+), 15 deletions(-) diff --git a/crates/bashkit-python/src/async_bridge.rs b/crates/bashkit-python/src/async_bridge.rs index 554a97fc8..738f4f22d 100644 --- a/crates/bashkit-python/src/async_bridge.rs +++ b/crates/bashkit-python/src/async_bridge.rs @@ -27,7 +27,7 @@ pub(super) fn register_private_loop(private_loop: &Arc Option { + let mut active = DELIVERIES.0.lock().expect("async deliveries lock"); + if super::interpreter_at_exit() { + return None; + } + *active += 1; + Some(DeliveryGuard) +} + tokio::task_local! { static LOCALS: TaskLocals; } diff --git a/crates/bashkit-python/src/lib.rs b/crates/bashkit-python/src/lib.rs index 815931872..bd747636b 100644 --- a/crates/bashkit-python/src/lib.rs +++ b/crates/bashkit-python/src/lib.rs @@ -3982,8 +3982,31 @@ async fn run_private_loop_awaitable_yielding( // THREAT[TM-DOS-057]: execute_sync() has no caller asyncio loop. Run the // private Python event loop on Tokio's blocking pool so the interpreter // future yields and Bashkit's execution timeout can preempt slow callbacks. + // + // THREAT[TM-PY-030]: a timeout abandons this task, and dropping a + // JoinHandle cannot cancel a *blocking* task — the pool thread stays + // parked in `result_rx.recv()` with the GIL released and re-attaches once + // the private loop delivers the (cancelled) result. Enroll it in the + // atexit barrier so interpreter finalization cannot begin while that + // re-attach is still owed. Joining the private-loop worker is not enough: + // that join only proves the *worker* is done, and it hands the result to + // this thread at the very end of the exit hook, leaving its re-attach to + // race `Py_FinalizeEx` (abort on CPython < 3.13 — see `enroll_python_work`). + let enrolled = async_bridge::enroll_python_work(); tokio::task::spawn_blocking(move || { - Python::attach(|py| session.run_awaitable_on_private_loop(py, &awaitable, call)) + let Some(_enrolled) = enrolled else { + // Boundary already closed: never attach. The `Py` values this task + // owns drop unattached (PyO3 defers the decrefs). + return Err(PyRuntimeError::new_err("interpreter is shutting down")); + }; + let result = + Python::attach(|py| session.run_awaitable_on_private_loop(py, &awaitable, call)); + // Release this task's Python references while still enrolled. Dropping + // the last `session` clone can run `PyPrivateAsyncLoop::drop`, which + // attaches, so it must happen inside the barrier too. + drop(awaitable); + drop(session); + result }) .await .map_err(|e| PyRuntimeError::new_err(format!("Python async callback worker failed: {e}")))? diff --git a/crates/bashkit-python/tests/test_teardown_determinism.py b/crates/bashkit-python/tests/test_teardown_determinism.py index 80a5b3f6c..775f5c0ed 100644 --- a/crates/bashkit-python/tests/test_teardown_determinism.py +++ b/crates/bashkit-python/tests/test_teardown_determinism.py @@ -197,7 +197,43 @@ async def slow(params, stdin=None): """ -@pytest.mark.parametrize("script", [_EXIT_SCRIPT_CLEAN, _EXIT_SCRIPT_ABANDONED], ids=["clean", "abandoned"]) +_EXIT_SCRIPT_ABANDONED_SLOW_CANCEL = """ +import asyncio +import threading +import time + +from bashkit.scripted import ScriptedTool + +cancelling = threading.Event() + +async def slow(params, stdin=None): + try: + await asyncio.sleep(30) + finally: + # Hold the private-loop worker inside the cancelled callback past + # interpreter exit. The timeout already abandoned the blocking-pool + # thread that dispatched this callback, and a blocking task cannot be + # cancelled, so that thread is still parked in its result receive when + # the exit hook runs: it only wakes when the hook joins this worker, + # and must then re-attach from inside the hook. Unenrolled, that + # re-attach raced Py_FinalizeEx and aborted the process on CPython + # < 3.13 (SIGABRT, "FATAL: exception not rethrown"). + cancelling.set() + time.sleep(0.3) + return "late\\n" + +t = ScriptedTool("api", timeout_seconds=0.05) +t.add_tool("slow", "Slow", callback=slow) +assert t.execute_sync("slow").exit_code == 1 +assert cancelling.wait(2), "callback never cancelled" +""" + + +@pytest.mark.parametrize( + "script", + [_EXIT_SCRIPT_CLEAN, _EXIT_SCRIPT_ABANDONED, _EXIT_SCRIPT_ABANDONED_SLOW_CANCEL], + ids=["clean", "abandoned", "abandoned-slow-cancel"], +) def test_interpreter_exit_does_not_crash(script): """Module-level tools torn down at Py_Finalize must not abort (SIGABRT).""" # Replicate this (parent) interpreter's exact import surface in the child. diff --git a/knowledge/runtimes/python-package.md b/knowledge/runtimes/python-package.md index 0d5a7acaa..4e0b31e0b 100644 --- a/knowledge/runtimes/python-package.md +++ b/knowledge/runtimes/python-package.md @@ -238,6 +238,17 @@ while Python is still attachable. This also drains timeout-cancellation cleanup; setting only an exit flag leaves a worker already inside Python racing finalization. The atexit hook then closes result-delivery enrollment and releases the GIL until enrolled deliveries finish releasing Python references. +Enrollment is not only for result deliveries: every native task that may attach +to Python must take an `enroll_python_work()` guard and hold it across its +Python work *and* the drops of the `Py` values and sessions it owns (a last +session clone can run `PyPrivateAsyncLoop::drop`, which attaches). The +`execute_sync` blocking-pool dispatch is enrolled for exactly this reason: a +timeout abandons it, a blocking task cannot be cancelled, and the private-loop +join hands it the cancelled result at the very end of the hook — unenrolled, its +re-attach raced `Py_FinalizeEx` and aborted the process on CPython < 3.13 +(TM-PY-030 variant 4). Joining the private-loop worker is therefore necessary +but not sufficient: it proves the worker finished, not that the thread waiting +on it has. Without that drain, `call_soon_threadsafe` can wake Python, which exits while the native worker still owns attached references; CPython can destroy its thread state before tuple cleanup, causing a native crash. Future deliveries skip Python diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 644527271..2986140c7 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -2030,7 +2030,7 @@ re-entering Python no longer race the caller's GIL hold. | TM-PY-026 | reset() discards security config | `BashTool.reset()` creates new `Bash` with bare builder, dropping all configured limits | `PyBash::reset` and `BashTool::reset` rebuild via `replace_live_bash_with_builder` + `build_live_builder`, which preserves the original limits, env, and registered builtins | **MITIGATED** | | TM-PY-027 | Unbounded recursion in JSON conversion | `py_to_json`/`json_to_py` recurse without depth limit on nested dicts/lists | `json_to_py_inner`, `py_to_json_inner`, and the MontyObject converters all carry a `depth` arg; depth > `MAX_NESTING_DEPTH = 64` raises `ValueError("… nesting depth exceeds maximum of 64")` | **MITIGATED** | -| TM-PY-030 | GIL deadlock / exit crash via async-callback private loop | Private-loop dispatch blocked on a rendezvous channel while attached (GIL held); pyclass dealloc joined in-flight blocking tasks that must re-attach to finish (froze the whole process, observed as a 6 h CI hang); worker thread attached during interpreter finalization to close its loop (SIGABRT at process exit) | Deterministic teardown protocol: dispatch detaches around send/receive; in-flight callbacks are cancelled and workers/runtime joined with the GIL released while the interpreter is alive; once the atexit-set exit flag flips, threads skip Python and the OS reclaims resources | **MITIGATED** | +| TM-PY-030 | GIL deadlock / exit crash via async-callback private loop | Private-loop dispatch blocked on a rendezvous channel while attached (GIL held); pyclass dealloc joined in-flight blocking tasks that must re-attach to finish (froze the whole process, observed as a 6 h CI hang); worker thread attached during interpreter finalization to close its loop (SIGABRT at process exit); a timed-out callback's blocking-pool dispatch thread stayed parked in its result receive and re-attached after the exit hook returned, racing `Py_FinalizeEx` (flaky SIGABRT, "FATAL: exception not rethrown") | Deterministic teardown protocol: dispatch detaches around send/receive; in-flight callbacks are cancelled and workers/runtime joined with the GIL released while the interpreter is alive; every native task that may attach enrolls in the atexit barrier, abandoned blocking dispatch included, so finalization cannot begin while a re-attach is owed; once the atexit-set exit flag flips, threads skip Python and the OS reclaims resources | **MITIGATED** | **TM-PY-026** (mitigated): see table. Regression tests: `tests/test_python_security.py::test_bash_reset_preserves_limits`, @@ -2062,7 +2062,7 @@ forces this window for Bash, BashTool and ScriptedTool (TM-PY-030). `test_exit_drains_private_callback_cancellation` forces cancellation cleanup to overlap process exit for custom builtins and both embedded runtimes. -**TM-PY-030** (mitigated): three crash/deadlock variants in the async-callback +**TM-PY-030** (mitigated): four crash/deadlock variants in the async-callback private-loop machinery (`crates/bashkit-python/src/lib.rs`), resolved by a deterministic teardown protocol and root-cause channel fix. (1) `PyPrivateAsyncLoop::run_awaitable` originally sent work to the dedicated worker thread over a `sync_channel(0)` rendezvous @@ -2078,7 +2078,21 @@ and an abandoned (timed-out) callback task must re-attach to finish, freezing th entire interpreter. (3) The private-loop worker attached on its exit path while the interpreter was finalizing, `Py_Finalize` gc is what usually wakes it, which fatals CPython (`PyGILState_Release`, SIGABRT; `Python::try_attach` cannot detect -finalization before 3.13). +finalization before 3.13). (4) `execute_sync` dispatches a private-loop +callback from Tokio's blocking pool, and a timeout abandons that task: dropping a +`JoinHandle` cannot cancel a *blocking* task, so the pool thread stayed parked in +`result_rx.recv()` with the GIL released. The exit hook joined the private-loop +worker, which hands the cancelled result to that thread at the very end of the +hook, leaving its re-attach to race `Py_FinalizeEx`. The task was never enrolled +in the delivery barrier, so nothing waited for it. Losing that race aborts the +process on CPython < 3.13: `take_gil` sees the thread must exit and calls +`PyThread_exit_thread()`, whose `pthread_exit` forced unwind crosses Rust frames +that never rethrow it, so glibc kills the process with +"FATAL: exception not rethrown" (SIGABRT). Observed as a rare flake in +`test_interpreter_exit_does_not_crash[abandoned]`. **Fix**: the dispatch enrolls +via `async_bridge::enroll_python_work()` and holds the guard across its Python +work and its own `Py`/session drops, so the barrier drains it before +finalization; past the boundary it skips Python entirely. Teardown protocol: an `atexit` handler registered at module import sets `INTERPRETER_AT_EXIT`; atexit runs at the very start of `Py_FinalizeEx`, strictly