Skip to content

Commit 032d43b

Browse files
cosmicBboyclaude
andauthored
fix: run sync sandbox tools off the event loop so traced tools work in agent code mode (#1523)
## Problem With `Agent(code_mode=True)`, live runs showed **every** tool call failing with: ``` Deadlock detected: blocking call used in syncify thread flyte_syncify when calling function <function _fetch_action_outputs ...>, use .aio() if in an async call. ``` Root cause: when the agent is entered through a syncified call (e.g. a sync `agent.run()`), the whole Monty sandbox loop runs on the `flyte_syncify` background loop. `ExternalFunctionBridge.execute_monty` invoked external functions **inline**, so a sync `@flyte.trace` tool's wrapper made its blocking syncify call (`_fetch_action_outputs`) from the syncify thread itself — a guaranteed deadlock that the SDK detects and aborts. The JSON tool-calling path was unaffected because `_make_callable_tool` already dispatches sync tools via `asyncio.to_thread`. Downstream workaround that motivated this fix: unionai/unionai-agents#86 (had to disable code mode entirely). ## Fix `ExternalFunctionBridge` now routes every non-coroutine-function external through `asyncio.to_thread` (new `_call_external` helper, used by both `execute_monty` and `_map_callable`): - `asyncio.to_thread` copies contextvars, so the Flyte task context — and therefore tracing — works exactly as it does on the JSON tool path. - The blocking syncify calls inside sync trace wrappers now happen on a worker thread while the syncify loop stays free to serve them. - Side benefit: slow sync tools no longer stall the event loop driving the sandbox (previously they blocked it outright), including under `flyte_map`. - Coroutine functions (`TaskTemplate.aio`, async tools, call-handler wrappers) keep their inline path, and the existing unwrap loop still drains coroutines returned by non-coroutine callables (local-mode `TaskTemplate.aio()`). ## Also in this PR: remove a flaky Python 3.14 test CI on this PR surfaced a pre-existing flaky failure, unrelated to the sandbox change: `test_task_call_sequence.py::test_interleaved_allocations_prevent_id_reuse` asserted that CPython never hands a freed override object's address to a later allocation when a same-size "noise" object is allocated in between. That's unspecified allocator behavior, and the 3.14 runner reused an address once (`seqs [1,1,1,1,1,1,1,2,1,1]`). The `id()`-keyed sequencing that test demonstrates is also historical — the remote controller has keyed sequences on a call key (task identity + inputs hash + group) via `TaskCallSequencer` since #607, with deterministic coverage in `test_action_name_stability_gaps.py`. The test is removed and the file's docstring now notes that the remaining demonstrations (which hold every object alive, so ids are guaranteed distinct) are deterministic and describe pre-#607 behavior. ## Verification - New regression tests drive `orchestrate_local` on the syncify loop with a sync tool that makes a blocking syncify call (direct call and via `flyte_map`) — they reproduce the exact production error before the fix and pass after. - Repro script confirmed: unfixed bridge → `Deadlock detected: blocking call used in syncify thread flyte_syncify ...`; fixed bridge → returns the tool result. - `tests/flyte/sandbox/` + `tests/flyte/ai/`: 694 passed; sequencing suites (`test_task_call_sequence.py`, `test_action_name_stability_gaps.py`): 22 passed. Lint/mypy/ty clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TqG84zzd1AFjBXPrMnWDxh --------- Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 1fc0a47 commit 032d43b

3 files changed

Lines changed: 145 additions & 33 deletions

File tree

src/flyte/sandbox/_bridge.py

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,30 @@ def _to_monty(value: Any) -> Any:
2929
return value
3030

3131

32+
async def _call_external(fn: Callable[..., Any], *args: Any, **kwargs: Any) -> Any:
33+
"""Call an external ref from the bridge's event loop, resolving to a concrete value.
34+
35+
Coroutine functions are called inline and awaited. Everything else runs in a
36+
worker thread via `asyncio.to_thread` (which propagates contextvars, so the
37+
Flyte task context survives): a sync external may block — in particular a sync
38+
`@flyte.trace` wrapper makes blocking syncify calls, and when this loop *is*
39+
the syncify loop (e.g. agent code mode entered through a syncified call), an
40+
inline call would trip syncify's deadlock detection and kill the tool call.
41+
Offloading also keeps slow sync tools from stalling the loop.
42+
43+
The trailing loop unwraps coroutines returned by non-coroutine-function
44+
callables — e.g. `TaskTemplate.aio()` in local mode may return an unawaited
45+
coroutine from `forward()`.
46+
"""
47+
if inspect.iscoroutinefunction(fn):
48+
result = fn(*args, **kwargs)
49+
else:
50+
result = await asyncio.to_thread(fn, *args, **kwargs)
51+
while inspect.iscoroutine(result):
52+
result = await result
53+
return result
54+
55+
3256
def _from_monty(value: Any) -> Any:
3357
"""Unmarshal a tagged dict back to a flyte.io type."""
3458
if isinstance(value, dict) and _IO_TYPE_KEY in value:
@@ -140,10 +164,7 @@ async def _map_callable(
140164

141165
async def run_row(row: tuple[Any, ...]) -> Any:
142166
try:
143-
result = fn(*row)
144-
while inspect.iscoroutine(result):
145-
result = await result
146-
return result
167+
return await _call_external(fn, *row)
147168
except Exception as exc:
148169
if return_exceptions:
149170
return exc
@@ -202,12 +223,7 @@ async def execute_monty(self, monty_cls: Any, code: str, input_names: list[str],
202223
args = [_from_monty(a) for a in progress.args]
203224
kwargs = {k: _from_monty(v) for k, v in progress.kwargs.items()}
204225

205-
# Call the external function and await if async.
206-
# Loop because TaskTemplate.aio() in local mode may return
207-
# an unawaited coroutine from forward() for async functions.
208-
result = fn(*args, **kwargs)
209-
while inspect.iscoroutine(result):
210-
result = await result
226+
result = await _call_external(fn, *args, **kwargs)
211227

212228
progress = progress.resume({"return_value": _to_monty(result)})
213229
else:

tests/flyte/internal/runtime/test_task_call_sequence.py

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,14 @@
1010
Combined with a stale completion event in ActionCache (remove() does not clean
1111
up _completion_events), duplicate action names cause
1212
"Task X did not return an output path, but the task has outputs defined."
13+
14+
Note: the id()-based logic replicated below is the *historical* behavior these
15+
tests were written against; the remote controller now keys sequences on a call
16+
key (task identity + inputs hash + group) via TaskCallSequencer, covered in
17+
test_action_name_stability_gaps.py. The demonstrations here deliberately hold
18+
every override object alive so their id() values are guaranteed distinct —
19+
assertions about *freed* addresses would depend on unspecified CPython
20+
allocator behavior (and did flake on Python 3.14).
1321
"""
1422

1523
import hashlib
@@ -135,28 +143,6 @@ def test_sequence_increments_for_same_object_identity(self):
135143

136144
assert seqs == list(range(1, 11))
137145

138-
def test_interleaved_allocations_prevent_id_reuse(self):
139-
"""
140-
Simulate what happens in real async code: allocations between loop
141-
iterations (logging, protobuf serialization, other coroutines) prevent
142-
CPython from reusing the same memory address for the next override
143-
object.
144-
"""
145-
sequencer: dict[int, int] = defaultdict(int)
146-
noise = [] # hold references to prevent address reuse
147-
148-
seqs = []
149-
for i in range(10):
150-
override = parse_entity_extraction.override(short_name="same_name")
151-
seq = generate_task_call_sequence(override, sequencer)
152-
seqs.append(seq)
153-
del override
154-
# Allocate an object of the same type/size to grab the freed address
155-
noise.append(parse_entity_extraction.override(short_name=f"noise-{i}"))
156-
157-
# With interleaved allocations, id() is not reused -> seq stays at 1
158-
assert all(s == 1 for s in seqs), f"Expected all sequences to be 1 (id not reused), got {seqs}"
159-
160146
def test_action_name_determinism(self):
161147
"""
162148
Verify that ActionID.new_sub_action_from produces identical names when

tests/flyte/sandbox/test_bridge.py

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
"""Tests for the ExternalFunctionBridge."""
22

3-
from flyte.sandbox._bridge import ExternalFunctionBridge
3+
import asyncio
4+
import threading
5+
6+
import pytest
7+
8+
import flyte.sandbox
9+
from flyte.sandbox._bridge import ExternalFunctionBridge, _call_external
10+
from flyte.syncify import syncify
411

512

613
class TestExternalFunctionBridge:
@@ -28,3 +35,106 @@ def test_init_empty_refs(self):
2835
durable_refs={},
2936
)
3037
assert bridge._all_refs == {}
38+
39+
40+
class TestCallExternal:
41+
"""`_call_external` dispatches sync work off the loop thread and awaits async work inline."""
42+
43+
@pytest.mark.asyncio
44+
async def test_sync_fn_runs_off_loop_thread(self):
45+
seen: dict[str, threading.Thread] = {}
46+
47+
def sync_fn(x: int) -> int:
48+
seen["thread"] = threading.current_thread()
49+
return x + 1
50+
51+
assert await _call_external(sync_fn, 1) == 2
52+
assert seen["thread"] is not threading.current_thread()
53+
54+
@pytest.mark.asyncio
55+
async def test_async_fn_runs_on_loop(self):
56+
seen: dict[str, threading.Thread] = {}
57+
58+
async def async_fn(x: int) -> int:
59+
seen["thread"] = threading.current_thread()
60+
return x + 1
61+
62+
assert await _call_external(async_fn, 1) == 2
63+
assert seen["thread"] is threading.current_thread()
64+
65+
@pytest.mark.asyncio
66+
async def test_returned_coroutine_is_awaited(self):
67+
# TaskTemplate.aio() in local mode may hand back an unawaited coroutine
68+
# from forward(); _call_external must drain it to a concrete value.
69+
async def inner() -> str:
70+
return "done"
71+
72+
def sync_returning_coro():
73+
return inner()
74+
75+
assert await _call_external(sync_returning_coro) == "done"
76+
77+
78+
# --- Regression: agent code mode with sync @flyte.trace tools -----------------
79+
#
80+
# With `code_mode=True` the whole sandbox loop can end up running on the
81+
# `flyte_syncify` background loop (any syncified entry point). Sync
82+
# `@flyte.trace` wrappers make *blocking* syncify calls
83+
# (`_fetch_action_outputs` / `_record_trace_action`); if the bridge invokes
84+
# them inline on that same thread, syncify's deadlock detection aborts every
85+
# tool call. The bridge must run sync externals in a worker thread instead.
86+
87+
88+
@syncify
89+
async def _syncified_helper() -> str:
90+
return "ok"
91+
92+
93+
def blocking_tool(x: int) -> int:
94+
"""A sync tool that blocks on syncify, exactly like a sync @flyte.trace wrapper."""
95+
assert _syncified_helper() == "ok"
96+
return x + 1
97+
98+
99+
@syncify
100+
async def _orchestrate_on_syncify_loop(code: str, tools: list, inputs: dict):
101+
return await flyte.sandbox.orchestrate_local(code, inputs=inputs, tasks=tools)
102+
103+
104+
class TestSyncToolOnSyncifyLoop:
105+
def test_blocking_sync_tool_does_not_deadlock(self):
106+
# Before the fix this raised: "Deadlock detected: blocking call used in
107+
# syncify thread flyte_syncify ... use .aio() if in an async call."
108+
result = _orchestrate_on_syncify_loop("blocking_tool(x)", [blocking_tool], {"x": 1})
109+
assert result == 2
110+
111+
def test_flyte_map_over_blocking_sync_tool_does_not_deadlock(self):
112+
result = _orchestrate_on_syncify_loop(
113+
"flyte_map('blocking_tool', xs)",
114+
[blocking_tool],
115+
{"xs": [1, 2, 3]},
116+
)
117+
assert result == [2, 3, 4]
118+
119+
def test_event_loop_stays_responsive_while_sync_tool_blocks(self):
120+
# While a sync tool sleeps in its worker thread, the loop driving the
121+
# bridge must still be able to run other coroutines.
122+
import time
123+
124+
async def heartbeat() -> int:
125+
ticks = 0
126+
for _ in range(5):
127+
await asyncio.sleep(0.01)
128+
ticks += 1
129+
return ticks
130+
131+
def parked(x: int) -> int:
132+
time.sleep(0.05)
133+
return x
134+
135+
async def drive() -> tuple[int, int]:
136+
return await asyncio.gather(_call_external(parked, 7), heartbeat())
137+
138+
tool_result, ticks = asyncio.run(drive())
139+
assert tool_result == 7
140+
assert ticks == 5

0 commit comments

Comments
 (0)