fix(ui): serialise concurrent permission modals with a lock
When the LLM returns multiple tool calls in one response, query.py
executes them concurrently via asyncio.gather. If more than one tool
requires user confirmation, each concurrent _ask_permission call emits
its own modal_request event. The React frontend overwrites its modal
state on every modal_request (setModal), so only the last dialog is
ever shown to the user. The earlier futures never receive a response
and time out after 300 s, causing silent "Permission denied" errors.
This edge case was not addressed by the recent deadlock fix (69c85e4).
Fix: add _permission_lock (asyncio.Lock) to ReactBackendHost and wrap
_ask_permission with 'async with self._permission_lock'. Only one
permission dialog is live at a time; the next concurrent caller waits
until the current modal is resolved or timed out.
Add test_concurrent_ask_permission_are_serialised to verify that two
concurrent _ask_permission calls are serialised and both resolve correctly.
Co-authored-by: Copilot
This commit is contained in:
@@ -25,6 +25,7 @@ The format is based on Keep a Changelog, and this project currently tracks chang
|
||||
- Memory search matches against body content in addition to metadata, with metadata weighted higher for relevance.
|
||||
- Memory search tokenizer handles Han characters for multilingual queries.
|
||||
- Fixed duplicate response in React TUI caused by double Enter key submission in the input handler.
|
||||
- Fixed concurrent permission modals overwriting each other in TUI default mode when the LLM returns multiple tool calls in one response; `_ask_permission` now serialises callers via an `asyncio.Lock` so each modal is shown and resolved before the next one is emitted.
|
||||
|
||||
### Changed
|
||||
|
||||
|
||||
@@ -65,6 +65,7 @@ class ReactBackendHost:
|
||||
self._request_queue: asyncio.Queue[FrontendRequest] = asyncio.Queue()
|
||||
self._permission_requests: dict[str, asyncio.Future[bool]] = {}
|
||||
self._question_requests: dict[str, asyncio.Future[str]] = {}
|
||||
self._permission_lock = asyncio.Lock()
|
||||
self._busy = False
|
||||
self._running = True
|
||||
# Track last tool input per name for rich event emission
|
||||
@@ -647,27 +648,28 @@ class ReactBackendHost:
|
||||
return options
|
||||
|
||||
async def _ask_permission(self, tool_name: str, reason: str) -> bool:
|
||||
request_id = uuid4().hex
|
||||
future: asyncio.Future[bool] = asyncio.get_running_loop().create_future()
|
||||
self._permission_requests[request_id] = future
|
||||
await self._emit(
|
||||
BackendEvent(
|
||||
type="modal_request",
|
||||
modal={
|
||||
"kind": "permission",
|
||||
"request_id": request_id,
|
||||
"tool_name": tool_name,
|
||||
"reason": reason,
|
||||
},
|
||||
async with self._permission_lock:
|
||||
request_id = uuid4().hex
|
||||
future: asyncio.Future[bool] = asyncio.get_running_loop().create_future()
|
||||
self._permission_requests[request_id] = future
|
||||
await self._emit(
|
||||
BackendEvent(
|
||||
type="modal_request",
|
||||
modal={
|
||||
"kind": "permission",
|
||||
"request_id": request_id,
|
||||
"tool_name": tool_name,
|
||||
"reason": reason,
|
||||
},
|
||||
)
|
||||
)
|
||||
)
|
||||
try:
|
||||
return await asyncio.wait_for(future, timeout=300)
|
||||
except asyncio.TimeoutError:
|
||||
log.warning("Permission request %s timed out after 300s, denying", request_id)
|
||||
return False
|
||||
finally:
|
||||
self._permission_requests.pop(request_id, None)
|
||||
try:
|
||||
return await asyncio.wait_for(future, timeout=300)
|
||||
except asyncio.TimeoutError:
|
||||
log.warning("Permission request %s timed out after 300s, denying", request_id)
|
||||
return False
|
||||
finally:
|
||||
self._permission_requests.pop(request_id, None)
|
||||
|
||||
async def _ask_question(self, question: str) -> str:
|
||||
request_id = uuid4().hex
|
||||
|
||||
@@ -406,3 +406,51 @@ async def test_backend_host_apply_provider_select_command_shows_single_segment_t
|
||||
assert should_continue is True
|
||||
user_event = next(item for item in events if item.type == "transcript_item" and item.item and item.item.role == "user")
|
||||
assert user_event.item.text == "/provider"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_ask_permission_are_serialised():
|
||||
"""Concurrent _ask_permission calls must be serialised so the frontend
|
||||
never receives two overlapping modal_request events.
|
||||
|
||||
Without _permission_lock the second call emits a modal_request before the
|
||||
first future is resolved, overwriting the frontend's modal state. The first
|
||||
tool then silently waits 300 s and gets Permission denied.
|
||||
"""
|
||||
host = ReactBackendHost(BackendHostConfig(api_client=StaticApiClient("unused")))
|
||||
|
||||
emitted_order: list[str] = []
|
||||
|
||||
async def _fake_emit(event: BackendEvent) -> None:
|
||||
if event.type == "modal_request" and event.modal:
|
||||
emitted_order.append(str(event.modal.get("request_id", "")))
|
||||
|
||||
host._emit = _fake_emit # type: ignore[method-assign]
|
||||
|
||||
async def _ask_and_approve(tool: str) -> bool:
|
||||
# Start the ask; a background task resolves the future once it appears.
|
||||
async def _resolver():
|
||||
# Busy-wait until this tool's future is registered.
|
||||
while True:
|
||||
await asyncio.sleep(0)
|
||||
for rid, fut in list(host._permission_requests.items()):
|
||||
if not fut.done():
|
||||
fut.set_result(True)
|
||||
return
|
||||
|
||||
asyncio.create_task(_resolver())
|
||||
return await host._ask_permission(tool, "reason")
|
||||
|
||||
# Fire two permission requests concurrently.
|
||||
result_a, result_b = await asyncio.gather(
|
||||
_ask_and_approve("write_file"),
|
||||
_ask_and_approve("bash"),
|
||||
)
|
||||
|
||||
assert result_a is True
|
||||
assert result_b is True
|
||||
# With the lock in place the two modal_request events must be emitted
|
||||
# sequentially (one completes before the other starts), so exactly two
|
||||
# distinct request IDs must have been emitted.
|
||||
assert len(emitted_order) == 2
|
||||
assert emitted_order[0] != emitted_order[1]
|
||||
|
||||
Reference in New Issue
Block a user