fix(python): release the GIL across engine evaluation

[liamcrumm]

Fixes #44.

Problem

intercept() held the CPython GIL for the whole of Runtime::evaluate.

For a pure Rego manifest that is harmless, since evaluation happens in process and finishes in microseconds. It stops being harmless as soon as a manifest declares an llm, endpoint or classifier annotator, because evaluation then makes a blocking HTTP request to the classifier and waits for the reply with the lock still held. Every thread in the process stops for that round trip, not only the one being governed.

interceptor_new had the same omission. It reads the manifest from disk and follows its extends chain, which may fetch over the network.

Measurement

A background thread does nothing but increment a counter. The control is time.sleep(0.3), which releases the GIL. The classifier takes 300 ms. Repro is in #44.

	other threads got
before, three runs	1.77%, 1.90%, 1.87% of normal progress
after, three runs	106.98%, 97.22%, 97.49%

Under agent-framework-core, an asyncio task asking to wake every 5 ms did not run for 313 ms with one 300 ms annotator bound to input. Annotators on a point dispatch in sequence, so two of them cost 604 ms. Timing intercept() directly gave 302, 302 and 303 ms for one annotator, and 604, 605 and 604 ms for two.

For a Python service the old behaviour presented as tail latency on endpoints unrelated to the governed request, which is a slow thing to attribute to a policy engine.

Change

py.detach around the engine call in intercept, and around manifest load plus runtime construction in interceptor_new.

This is not a new pattern. policy_activate, policy_activate_from_memory and policy_evaluate already detach, and policy_activate carries a comment giving this reason. policy_evaluate also detaches while Python host dispatchers may be called back into, so the callback path is proven in tree: an annotator dispatcher, policy dispatcher or telemetry sink written in Python re-acquires through Python::attach, unchanged by this PR.

Anything that reads a Python object still happens before the lock drops. In interceptor_new that is resolve_limits, now explicitly ordered ahead of the detach with a comment saying why.

Test

sdk/python/tests/test_gil_release.py serves the classifier from a thread of the same interpreter.

That design carries the assertion. Holding the GIL starves the server thread, so the annotator times out and the point fails closed with runtime_error:annotation_failed. Releasing it lets the label reach the policy, which denies with annotator_ran. There is no ratio and no threshold, given this repo's history with timing assertions on loaded runners.

Confirmed to fail on the defect, by stashing the lib.rs change and rebuilding:

E         - annotator_ran
E         + runtime_error:annotation_failed
FAILED sdk/python/tests/test_gil_release.py::test_annotator_reaches_a_classifier_served_by_this_interpreter
1 failed in 4.77s

The 4.77 s is the annotator's own 4000 ms timeout expiring, which is what starvation looks like from outside.

Review round

The error precedence note is correct, and no code changed for it. Moving the manifest read into the detached closure also moves it after perf_telemetry and limits validation, so a call carrying both an unreadable manifest and an invalid perf_telemetry now reports the perf error first. Single-fault exception classes and messages are unchanged, and validating the cheap Python-reading arguments before dropping the lock is the intended order, so the precedence change is recorded rather than reverted.

The branch is updated to main.

Verification

Check	Result
pytest sdk/python/tests	88 passed
ruff format --check, ruff check	clean
cargo fmt --check	clean
cargo clippy --all-targets -D warnings	clean
cargo check --locked --workspace	clean on the merge with main

Other bindings

Node and .NET are untouched. Whether either has an equivalent problem, a synchronous binding blocking the Node event loop in particular, is unmeasured and therefore unclaimed here.

Ordering

Independent of #55, the release bump. Either can land first. Both belong in 0.4.0-alpha.3 if the tag has not been cut, since this changes runtime behaviour for every Python host that uses annotators.

[MohammadHaroonAbuomar]

Reviewed at a4056ce, with local builds of both this head and its parent to compare behaviour.

Reproduced the defect and the fix. Rebuilt the binding from the parent commit: with a 400 ms classifier served in-process, a background counter thread made 0.06% of its control progress during intercept() — the call blocked 8.2 s until the annotator timeout because the server thread was starved, then failed closed with runtime_error:annotation_failed. At this head the same probe gives ~95% progress and the verdict settles from the classifier's answer. test_gil_release.py fails on the parent exactly as the description says, and its starvation-based design (deterministic, no timing threshold) is a good fit for this repo's loaded-runner history.

Error paths unchanged. Diffed exception class and message before/after for: missing manifest, malformed manifest YAML, unknown limits key, non-mapping limits, non-object context, unparseable context. Identical. PyErr construction inside the detached closures is lazy in pyo3, so building the error without the GIL is sound, and pyo3's detach guard reattaches on unwind.

Callback paths under the detached region. Probed a Python annotator dispatcher driven by 8 threads × 20 intercept() calls each: 160 dispatches reattached via Python::attach across 8 distinct threads, no deadlock, all verdicts correct. A raising dispatcher still fails closed (runtime_error:annotation_failed). A reentrant dispatcher that calls intercept() on another interceptor from inside the callback (attach nested in detach) also completes correctly. The detach closures capture only owned/Arc data; resolve_limits is the one Python-reading step and is correctly ordered before the detach.

Streaming untouched, no lock-ordering hazard. The Mutex<StreamSession> entry points never detach and the detached regions here never take that lock, so no GIL-vs-mutex inversion is introduced.

Checks run here: maturin editable build; pytest sdk/python/tests 88 passed; ruff format --check and ruff check clean; cargo fmt --check and cargo clippy --all-targets -D warnings clean. The branch is one commit behind main (the dependabot rust-group bump); the merge is textually clean and cargo check --locked passes on the merge result, so a branch update should be routine.

One non-blocking inline note on error precedence in interceptor_new.

[MohammadHaroonAbuomar]

Approved. Adversarial review posted earlier came back clean: detachment ordering sound, dispatcher reentrancy and error shapes verified, and the concurrency payoff empirically demonstrated. The non-blocking precedence note stands as an observation only.

[liamcrumm]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Done in c28dda2.

The only conflict was CHANGELOG.md: this branch's Unreleased entry against main's now-released 0.4.0-alpha.3 section. Both are kept, with the GIL entry left under Unreleased since the alpha.3 tag is cut and this lands after it. sdk/python/src/lib.rs and test_gil_release.py merged without conflict and are byte-identical to the reviewed head, so the approval above still applies to the code.

Net diff against main is still the three intended files. Verified on the merge result:

Check	Result
pytest sdk/python/tests	90 passed (88 + the 2 test_version.py cases from #55)
ruff format --check, ruff check	clean
cargo fmt --check	clean
cargo check --locked --workspace	clean
scripts/check-version-consistency.py	agrees at 0.4.0-alpha.3

One note: cargo clippy --all-targets -D warnings fails here on manual_noop_waker in engine/tests/activation.rs. I checked out origin/main in a separate worktree and it fails there identically, so it is a newer clippy in my sandbox than CI pins, not merge fallout, and I left it alone as out of scope.

The ci and CodeQL runs on the merge commit are sitting at action_required and need a maintainer to approve the workflow run.

[MohammadHaroonAbuomar]

Re-approving after the merge-from-main update; the resolution is identical to the union merge verified locally (unreleased GIL entry above the released 0.4.0-alpha.3 section). CI green on the merged head.