Skip to content

Commit adc54b2

Browse files
committed
Fix buyer-1g4: offload sync CrewAI flow.kickoff() to worker thread
CrewAI's sync Flow.kickoff() internally does ThreadPoolExecutor.submit(asyncio.run, _run_flow).result() — the .result() call blocks the calling thread regardless. When invoked from a FastAPI async handler (BackgroundTasks-driven _run_booking_flow), that calling thread is the event loop, so every other request hangs for the duration of the booking. Wrap the four async-context call sites with asyncio.to_thread() so the blocking .result() runs on a worker thread and the event loop stays responsive: - api/main.py:574 _run_booking_flow.kickoff - api/main.py:588 _run_booking_flow.approve_all (auto_approve) - api/main.py:450 approve_all_recommendations endpoint - buyer_deal_flow.py:684 run_buyer_deal_flow.kickoff We deliberately do NOT use ``await flow.kickoff_async()`` here, even though it looks cleaner. The buyer's Flow @start/@listen step methods are sync (they call crew.kickoff() directly). Awaiting kickoff_async from the event loop runs those sync steps in the loop thread anyway, re-introducing the original block. The to_thread(kickoff) shape puts the whole flow execution behind a worker thread. Add regression tests under tests/integration/test_buyer_1g4_async_kickoff.py: - Runtime tests assert flow.kickoff / flow.approve_all execute on a worker thread (not MainThread). - AST source-level tests walk each async def and reject sync .kickoff() / .approve_all() calls outside await/to_thread. Smoke verified: /health stayed responsive (mean 34ms, p95 41ms, max 43ms) across 82 probes during a 17-min concurrent booking flow. Before the fix the same probe hung indefinitely (~58 min before kill). The booking still doesn't reach terminal state end-to-end, but that's gated on a separate cross-repo contract drift (POST /products/search returns 405 because the seller exposes only GET /products) — that bug is already tracked separately and is independent of buyer-1g4.
1 parent cc0f0b0 commit adc54b2

3 files changed

Lines changed: 252 additions & 5 deletions

File tree

src/ad_buyer/flows/buyer_deal_flow.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
match each ref against package capabilities (proposal §5.1).
1212
"""
1313

14+
import asyncio
1415
import logging
1516
import sqlite3
1617
from datetime import datetime
@@ -680,8 +681,12 @@ async def run_buyer_deal_flow(
680681
if audience_plan is not None:
681682
flow.state.audience_plan = audience_plan
682683

683-
# Run flow
684-
result = flow.kickoff()
684+
# Run flow.
685+
# buyer-1g4: offload sync flow.kickoff() to a worker thread so
686+
# the caller's event loop stays responsive. See api/main.py
687+
# _run_booking_flow for the full rationale (sync Flow steps
688+
# block the loop if we use ``await flow.kickoff_async()``).
689+
result = await asyncio.to_thread(flow.kickoff)
685690

686691
return {
687692
"result": result,

src/ad_buyer/interfaces/api/main.py

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
"""FastAPI server for the Ad Buyer System."""
55

6+
import asyncio
67
import json
78
import logging
89
import sqlite3
@@ -447,7 +448,10 @@ async def approve_all_recommendations(job_id: str) -> dict[str, Any]:
447448
detail="Flow state not available. Job may have expired.",
448449
)
449450

450-
result = flow.approve_all()
451+
# buyer-1g4: approve_all() runs sync CrewAI work that holds the
452+
# calling thread; offload to a worker thread so the event loop
453+
# stays responsive.
454+
result = await asyncio.to_thread(flow.approve_all)
451455

452456
job["status"] = "completed" if result.get("status") == "success" else "failed"
453457
job["booked_lines"] = [b.model_dump() for b in flow.state.booked_lines]
@@ -571,7 +575,22 @@ async def _run_booking_flow(job_id: str, request: BookingRequest) -> None:
571575
job["_flow"] = flow
572576

573577
job["progress"] = 0.2
574-
_result = flow.kickoff()
578+
# buyer-1g4: offload the whole sync flow.kickoff() to a worker
579+
# thread so the FastAPI event loop stays responsive while the
580+
# crew agents (which block on real LLM HTTP calls) run.
581+
#
582+
# We do NOT use ``await flow.kickoff_async()`` here even though
583+
# it looks cleaner — the buyer's Flow ``@start`` / ``@listen``
584+
# step methods are sync and themselves call ``crew.kickoff()``
585+
# directly. Awaited from the event loop, those sync steps would
586+
# run IN the event loop thread, re-introducing the block.
587+
#
588+
# CrewAI's sync ``Flow.kickoff()`` already does
589+
# ``ThreadPoolExecutor.submit(asyncio.run, _run_flow).result()``
590+
# internally — the ``.result()`` is what blocks the calling
591+
# thread. ``asyncio.to_thread`` puts that ``.result()`` on a
592+
# worker thread (two threads deep, but the event loop is free).
593+
_result = await asyncio.to_thread(flow.kickoff)
575594

576595
# Propagate any errors captured by flow steps into the job response so
577596
# the client can see what went wrong instead of silent-success (ar-jbod).
@@ -585,7 +604,8 @@ async def _run_booking_flow(job_id: str, request: BookingRequest) -> None:
585604
job["recommendations"] = [r.model_dump() for r in flow.state.pending_approvals]
586605

587606
if request.auto_approve:
588-
flow.approve_all()
607+
# buyer-1g4: same reason as above — offload sync work.
608+
await asyncio.to_thread(flow.approve_all)
589609
job["booked_lines"] = [b.model_dump() for b in flow.state.booked_lines]
590610
job["status"] = "completed"
591611
else:
Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
# Author: Green Mountain Systems AI Inc.
2+
# Donated to IAB Tech Lab
3+
4+
"""Regression tests for buyer-1g4: sync CrewAI Flow.kickoff() must not
5+
block the FastAPI event loop.
6+
7+
CrewAI's sync ``Flow.kickoff()`` internally does::
8+
9+
with ThreadPoolExecutor(max_workers=1) as pool:
10+
return pool.submit(ctx.run, asyncio.run, _run_flow()).result()
11+
12+
The final ``.result()`` blocks the calling thread until the inner thread
13+
finishes — so calling ``flow.kickoff()`` directly from a FastAPI
14+
``async def`` handler blocks the entire event loop for the duration of
15+
the run.
16+
17+
The fix is to wrap the call with ``asyncio.to_thread`` so the
18+
``.result()`` runs on a worker thread instead of the event loop thread.
19+
We deliberately do NOT use ``await flow.kickoff_async()`` — the buyer's
20+
Flow ``@start`` / ``@listen`` step methods are themselves sync (they
21+
call ``crew.kickoff()`` directly), so awaiting kickoff_async from the
22+
event loop just runs those sync steps in the loop thread anyway.
23+
24+
These tests guard against anyone reverting to an un-offloaded sync
25+
``kickoff()`` / ``approve_all()`` in async contexts by asserting the
26+
calls execute on a worker thread, not on ``MainThread``.
27+
"""
28+
29+
import pytest
30+
31+
from ad_buyer.flows.deal_booking_flow import DealBookingFlow
32+
from ad_buyer.interfaces.api.main import (
33+
BookingRequest,
34+
CampaignBrief,
35+
_run_booking_flow,
36+
jobs,
37+
)
38+
from ad_buyer.time_utils import utc_now
39+
40+
41+
def _seed_job(job_id: str) -> None:
42+
jobs[job_id] = {
43+
"job_id": job_id,
44+
"status": "pending",
45+
"progress": 0.0,
46+
"errors": [],
47+
"budget_allocations": {},
48+
"recommendations": [],
49+
"booked_lines": [],
50+
"updated_at": utc_now().isoformat(),
51+
"created_at": utc_now().isoformat(),
52+
}
53+
54+
55+
def _sample_brief() -> CampaignBrief:
56+
return CampaignBrief(
57+
name="buyer-1g4 regression",
58+
objectives=["awareness"],
59+
budget=50000,
60+
start_date="2026-07-01",
61+
end_date="2026-07-31",
62+
target_audience={"geo": ["US"]},
63+
)
64+
65+
66+
class TestRunBookingFlowOffloadsKickoff:
67+
"""``api.main._run_booking_flow`` must offload sync CrewAI work so
68+
the FastAPI event loop stays responsive."""
69+
70+
@pytest.mark.asyncio
71+
async def test_kickoff_runs_on_worker_thread(self, monkeypatch):
72+
"""``flow.kickoff()`` (sync) must execute on a worker thread,
73+
never on ``MainThread``. ``asyncio.to_thread`` provides this
74+
offload; calling ``flow.kickoff()`` directly from the async
75+
background task would run it on the event-loop thread and
76+
block every other request (buyer-1g4)."""
77+
78+
import threading
79+
80+
kickoff_threads: list[str] = []
81+
82+
def fake_kickoff(self, *args, **kwargs):
83+
kickoff_threads.append(threading.current_thread().name)
84+
return {}
85+
86+
monkeypatch.setattr(DealBookingFlow, "kickoff", fake_kickoff)
87+
88+
job_id = "test-buyer-1g4-kickoff-thread"
89+
_seed_job(job_id)
90+
try:
91+
await _run_booking_flow(
92+
job_id,
93+
BookingRequest(brief=_sample_brief(), auto_approve=False),
94+
)
95+
96+
assert kickoff_threads, "flow.kickoff should have been called"
97+
assert "MainThread" not in kickoff_threads[0], (
98+
f"flow.kickoff ran on {kickoff_threads[0]} — buyer-1g4 "
99+
"regression: should be offloaded via asyncio.to_thread"
100+
)
101+
assert jobs[job_id]["status"] != "failed", (
102+
f"Job failed unexpectedly: errors={jobs[job_id]['errors']}"
103+
)
104+
finally:
105+
jobs.pop(job_id, None)
106+
107+
@pytest.mark.asyncio
108+
async def test_approve_all_runs_on_worker_thread(self, monkeypatch):
109+
"""When ``auto_approve=True``, ``approve_all`` is sync CrewAI
110+
work and must be offloaded via ``asyncio.to_thread`` for the
111+
same reason as ``kickoff``."""
112+
113+
import threading
114+
115+
approve_threads: list[str] = []
116+
117+
def fake_kickoff(self, *args, **kwargs):
118+
return {}
119+
120+
def fake_approve_all(self):
121+
approve_threads.append(threading.current_thread().name)
122+
return {"status": "success", "booked_lines": []}
123+
124+
monkeypatch.setattr(DealBookingFlow, "kickoff", fake_kickoff)
125+
monkeypatch.setattr(DealBookingFlow, "approve_all", fake_approve_all)
126+
127+
job_id = "test-buyer-1g4-approve-all-thread"
128+
_seed_job(job_id)
129+
try:
130+
await _run_booking_flow(
131+
job_id,
132+
BookingRequest(brief=_sample_brief(), auto_approve=True),
133+
)
134+
135+
assert approve_threads, "approve_all should have been called"
136+
assert "MainThread" not in approve_threads[0], (
137+
f"approve_all ran on {approve_threads[0]} — buyer-1g4 "
138+
"regression: should be offloaded via asyncio.to_thread"
139+
)
140+
finally:
141+
jobs.pop(job_id, None)
142+
143+
144+
class TestSourceLevelAsyncContract:
145+
"""Source-level assertions for async-context call sites.
146+
147+
These guard against silent reverts to sync ``kickoff()`` /
148+
unwrapped ``approve_all()`` inside ``async def`` functions. They are
149+
cheaper than full mocking for sites whose runtime invocation needs
150+
extensive client/flow stubbing (e.g. ``run_buyer_deal_flow``).
151+
"""
152+
153+
@staticmethod
154+
def _calls_in_async_functions(filename: str) -> dict[str, list[str]]:
155+
"""Walk a source file and return, for each ``async def`` function,
156+
the list of attribute-call names invoked synchronously (i.e.
157+
not via ``await ...`` and not via ``asyncio.to_thread(...)``).
158+
"""
159+
import ast
160+
import pathlib
161+
162+
path = pathlib.Path(filename)
163+
tree = ast.parse(path.read_text())
164+
165+
results: dict[str, list[str]] = {}
166+
167+
class _Visitor(ast.NodeVisitor):
168+
def __init__(self):
169+
self._async_stack: list[str] = []
170+
171+
def visit_AsyncFunctionDef(self, node):
172+
self._async_stack.append(node.name)
173+
results.setdefault(node.name, [])
174+
self.generic_visit(node)
175+
self._async_stack.pop()
176+
177+
def visit_FunctionDef(self, node):
178+
# Skip nested sync defs — they have their own loop semantics.
179+
pass
180+
181+
def visit_Await(self, node):
182+
# Anything under an Await is fine — don't descend.
183+
pass
184+
185+
def visit_Call(self, node):
186+
if self._async_stack and isinstance(node.func, ast.Attribute):
187+
results[self._async_stack[-1]].append(node.func.attr)
188+
self.generic_visit(node)
189+
190+
_Visitor().visit(tree)
191+
return results
192+
193+
def test_run_booking_flow_no_sync_kickoff(self):
194+
from ad_buyer.interfaces.api import main as api_main
195+
196+
calls = self._calls_in_async_functions(api_main.__file__)
197+
booking_calls = calls.get("_run_booking_flow", [])
198+
assert "kickoff" not in booking_calls, (
199+
"_run_booking_flow contains a sync .kickoff() call — buyer-1g4 "
200+
"regression. Wrap with ``await asyncio.to_thread(flow.kickoff)``."
201+
)
202+
203+
def test_approve_all_recommendations_endpoint_no_sync_approve(self):
204+
from ad_buyer.interfaces.api import main as api_main
205+
206+
calls = self._calls_in_async_functions(api_main.__file__)
207+
endpoint_calls = calls.get("approve_all_recommendations", [])
208+
assert "approve_all" not in endpoint_calls, (
209+
"approve_all_recommendations endpoint contains a sync "
210+
".approve_all() call — buyer-1g4 regression. Wrap with "
211+
"``await asyncio.to_thread(flow.approve_all)``."
212+
)
213+
214+
def test_run_buyer_deal_flow_no_sync_kickoff(self):
215+
from ad_buyer.flows import buyer_deal_flow
216+
217+
calls = self._calls_in_async_functions(buyer_deal_flow.__file__)
218+
run_calls = calls.get("run_buyer_deal_flow", [])
219+
assert "kickoff" not in run_calls, (
220+
"run_buyer_deal_flow contains a sync .kickoff() call — "
221+
"buyer-1g4 regression. Use ``await flow.kickoff_async()``."
222+
)

0 commit comments

Comments
 (0)