From d2a0bb8d56c45e355cc0eb531720002572b3b4f1 Mon Sep 17 00:00:00 2001 From: Pallavi Raiturkar Date: Mon, 20 Jul 2026 10:49:43 -0400 Subject: [PATCH] Fix ask_user starving the Rust SDK per-session event loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each session runs one `tokio::select!` loop in `rust/src/session.rs`. Inbound JSON-RPC requests were dispatched by `handle_request(...).await` inline in the select loop, so a slow handler parked the reader and could not drain the next message. `ask_user` (`userInput.request`) awaits the user's answer (host backstop timeout: 5 min), so while it was pending the loop was frozen — starving a sibling tool call co-emitted in the same turn (e.g. `set_session_title` + `ask_user` — github/copilot-experiences#12540) and hanging the UI. Notifications didn't hit this because their slow interactive callbacks (permission/tool/elicitation) are already spawned. Move concurrency to the request-dispatch boundary, matching the other five SDKs: spawn each `handle_request` from the `requests.recv()` branch as its own task that awaits the handler and sends that request's response, instead of awaiting inline. This fixes starvation for every request handler — not just `userInput.request` but also `exitPlanMode`, `autoModeSwitch`, hooks, transforms, and canvas/session-FS providers. The Arc-backed dispatch context is cloned into the task so the future is `'static`; JSON-RPC permits concurrent requests and out-of-order responses, so nothing is serialized. Notifications remain inline (they only do fast dispatch work). The `userInput.request` arm itself is unchanged from `main`. Add an e2e regression test (`ask_user` category) where the model emits `set_marker` and `ask_user` in one turn; the user-input handler waits for the sibling tool to fire before answering and asserts it observed the tool while its own request was still pending. The test fails on the inline dispatch (~31s, starvation) and passes with the boundary spawn (~10s). The parallel dotnet/go/python/nodejs bindings and the bundled core already spawn per-request, but their `userInput.request` handlers are worth a follow-up audit for the same inline-await asymmetry (not changed here). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86146d70-7b81-497c-9d80-cd8dd8cf8a41 --- rust/src/session.rs | 66 +++++--- rust/tests/e2e/ask_user.rs | 148 +++++++++++++++++- ..._block_sibling_tool_call_in_same_turn.yaml | 30 ++++ 3 files changed, 223 insertions(+), 21 deletions(-) create mode 100644 test/snapshots/ask_user/ask_user_does_not_block_sibling_tool_call_in_same_turn.yaml diff --git a/rust/src/session.rs b/rust/src/session.rs index 35656c657f..89162346e4 100644 --- a/rust/src/session.rs +++ b/rust/src/session.rs @@ -1440,14 +1440,27 @@ fn spawn_event_loop( loop { // `mpsc::UnboundedReceiver::recv` and // `CancellationToken::cancelled` are both cancel-safe per - // RFD 400. The selected branch's `await`'d handler is - // *not* mid-cancelled by the select — once a branch fires - // it runs to completion within the loop's iteration. - // Spawned child tasks inside `handle_notification` - // (permission/tool/elicitation callbacks) intentionally - // outlive the parent loop and own their own cleanup; - // this is RFD 400's "spawn background tasks to perform - // cancel-unsafe operations" pattern and is correct as-is. + // RFD 400. + // + // Inbound JSON-RPC *requests* are dispatched fire-and-forget: + // each `handle_request` runs in its own spawned task that + // awaits the handler and sends that request's response. This + // mirrors the other Copilot SDKs and moves concurrency to the + // request-dispatch boundary, so any slow handler — not just + // `userInput.request` (which can stay pending for the full + // input backstop of several minutes), but also `exitPlanMode`, + // `autoModeSwitch`, hooks, transforms, or canvas/session-FS + // providers — cannot park the reader loop and starve sibling + // requests or co-emitted notifications. JSON-RPC permits + // concurrent requests and out-of-order responses, so the SDK + // does not serialize them. + // + // `handle_notification` is awaited inline because it only + // performs fast dispatch work; its slow interactive callbacks + // (permission/tool/elicitation) are themselves spawned as child + // tasks. All of these spawned tasks intentionally outlive the + // parent loop and own their own cleanup — RFD 400's "spawn + // background tasks to perform cancel-unsafe operations" pattern. tokio::select! { _ = shutdown.cancelled() => break, Some(notification) = notifications.recv() => { @@ -1456,16 +1469,33 @@ fn spawn_event_loop( ).await; } Some(request) = requests.recv() => { - let ctx = RequestDispatchContext { - client: &client, - handlers: &handlers, - hooks: hooks.as_deref(), - transforms: transforms.as_deref(), - canvas_handler: canvas_handler.as_ref(), - session_fs_provider: session_fs_provider.as_ref(), - bearer_token_providers: &bearer_token_providers, - }; - handle_request(&session_id, ctx, request).await; + // Clone the Arc-backed dispatch context into the task so + // the spawned `handle_request` future is `'static`. All + // clones are cheap (Arc refcount bumps / small maps). + let span = tracing::error_span!("session_request_handler", session_id = %session_id); + let session_id = session_id.clone(); + let client = client.clone(); + let handlers = handlers.clone(); + let hooks = hooks.clone(); + let transforms = transforms.clone(); + let canvas_handler = canvas_handler.clone(); + let session_fs_provider = session_fs_provider.clone(); + let bearer_token_providers = bearer_token_providers.clone(); + tokio::spawn( + async move { + let ctx = RequestDispatchContext { + client: &client, + handlers: &handlers, + hooks: hooks.as_deref(), + transforms: transforms.as_deref(), + canvas_handler: canvas_handler.as_ref(), + session_fs_provider: session_fs_provider.as_ref(), + bearer_token_providers: &bearer_token_providers, + }; + handle_request(&session_id, ctx, request).await; + } + .instrument(span), + ); } else => break, } diff --git a/rust/tests/e2e/ask_user.rs b/rust/tests/e2e/ask_user.rs index 282af7d30d..c134ad3c90 100644 --- a/rust/tests/e2e/ask_user.rs +++ b/rust/tests/e2e/ask_user.rs @@ -1,11 +1,16 @@ use std::sync::Arc; +use std::time::Duration; use async_trait::async_trait; use github_copilot_sdk::handler::{ - PermissionHandler, PermissionResult, UserInputHandler, UserInputResponse, + ApproveAllHandler, PermissionHandler, PermissionResult, UserInputHandler, UserInputResponse, }; -use github_copilot_sdk::{RequestId, SessionConfig, SessionId}; -use tokio::sync::mpsc; +use github_copilot_sdk::tool::ToolHandler; +use github_copilot_sdk::{ + Error, RequestId, SessionConfig, SessionId, Tool, ToolInvocation, ToolResult, +}; +use serde_json::json; +use tokio::sync::{Notify, mpsc}; use super::support::{ DEFAULT_TEST_TOKEN, assistant_message_content, recv_with_timeout, with_e2e_context, @@ -147,6 +152,77 @@ async fn should_handle_freeform_user_input_response() { .await; } +/// Regression test for the per-session event-loop starvation bug where a pending +/// `ask_user` (`userInput.request`) blocked the `tokio::select!` loop and starved +/// a sibling tool call co-emitted in the same turn (github/copilot-experiences#12540). +/// +/// The model emits both `set_marker` and `ask_user` in one assistant turn. The +/// `set_marker` tool fires a `Notify`; the user-input handler waits on that +/// `Notify` before answering. If `ask_user` were awaited inline, the loop could +/// never dispatch the `set_marker` notification, so the handler would never +/// observe the tool firing. With the handler spawned, both run concurrently and +/// the handler observes the sibling tool while its own request is still pending. +#[tokio::test] +async fn ask_user_does_not_block_sibling_tool_call_in_same_turn() { + with_e2e_context( + "ask_user", + "ask_user_does_not_block_sibling_tool_call_in_same_turn", + |ctx| { + Box::pin(async move { + ctx.set_default_copilot_user(); + let client = ctx.start_client().await; + + // Fired by `set_marker` when the sibling tool executes. + let tool_fired = Arc::new(Notify::new()); + // Reports whether the user-input handler observed the sibling tool + // firing while its own `ask_user` request was still pending. + let (observed_tx, mut observed_rx) = mpsc::unbounded_channel(); + + let user_input_handler = Arc::new(SiblingAwareUserInputHandler { + tool_fired: tool_fired.clone(), + observed_tx, + }); + let tools = vec![set_marker_tool(tool_fired.clone())]; + + let session = client + .create_session( + SessionConfig::default() + .with_github_token(DEFAULT_TEST_TOKEN) + .with_permission_handler(Arc::new(ApproveAllHandler)) + .with_user_input_handler( + user_input_handler as Arc, + ) + .with_tools(tools), + ) + .await + .expect("create session"); + + session + .send_and_wait( + "Call set_marker with value 'go' and, at the same time, use the ask_user \ + tool to ask me to choose between 'Option A' and 'Option B'. Wait for my \ + answer before continuing.", + ) + .await + .expect("send") + .expect("assistant message"); + + let observed = + recv_with_timeout(&mut observed_rx, "user input handler observation").await; + assert!( + observed, + "ask_user handler must observe the sibling set_marker tool executing while \ + its own userInput.request is still pending (event loop must not be starved)" + ); + + session.disconnect().await.expect("disconnect session"); + client.stop().await.expect("stop client"); + }) + }, + ) + .await; +} + #[derive(Debug)] struct RecordedUserInputRequest { session_id: SessionId, @@ -204,3 +280,69 @@ impl PermissionHandler for RecordingUserInputHandler { PermissionResult::approve_once() } } + +/// A user-input handler that waits for a sibling tool to fire before answering, +/// then reports whether it observed that tool while its own request was pending. +struct SiblingAwareUserInputHandler { + tool_fired: Arc, + observed_tx: mpsc::UnboundedSender, +} + +#[async_trait] +impl UserInputHandler for SiblingAwareUserInputHandler { + async fn handle( + &self, + _session_id: SessionId, + _question: String, + choices: Option>, + _allow_freeform: Option, + ) -> Option { + // Wait (bounded) for the sibling `set_marker` tool to execute. On the + // buggy inline-await path the event loop is parked here, the tool + // notification is never dispatched, and this times out. + let observed = tokio::time::timeout(Duration::from_secs(30), self.tool_fired.notified()) + .await + .is_ok(); + let _ = self.observed_tx.send(observed); + + let answer = choices + .as_ref() + .and_then(|c| c.first()) + .cloned() + .unwrap_or_else(|| "Option A".to_string()); + Some(UserInputResponse { + answer, + was_freeform: false, + }) + } +} + +struct SetMarkerTool { + tool_fired: Arc, +} + +fn set_marker_tool(tool_fired: Arc) -> Tool { + Tool::new("set_marker") + .with_description("Records a marker value") + .with_parameters(json!({ + "type": "object", + "properties": { + "value": { "type": "string", "description": "Marker value" } + }, + "required": ["value"] + })) + .with_handler(Arc::new(SetMarkerTool { tool_fired })) +} + +#[async_trait] +impl ToolHandler for SetMarkerTool { + async fn call(&self, invocation: ToolInvocation) -> Result { + let value = invocation + .arguments + .get("value") + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + self.tool_fired.notify_one(); + Ok(ToolResult::Text(format!("MARKER_{}", value.to_uppercase()))) + } +} diff --git a/test/snapshots/ask_user/ask_user_does_not_block_sibling_tool_call_in_same_turn.yaml b/test/snapshots/ask_user/ask_user_does_not_block_sibling_tool_call_in_same_turn.yaml new file mode 100644 index 0000000000..4ba16d4d81 --- /dev/null +++ b/test/snapshots/ask_user/ask_user_does_not_block_sibling_tool_call_in_same_turn.yaml @@ -0,0 +1,30 @@ +models: + - claude-sonnet-4.5 +conversations: + - messages: + - role: system + content: ${system} + - role: user + content: Call set_marker with value 'go' and, at the same time, use the ask_user tool to ask me to choose between + 'Option A' and 'Option B'. Wait for my answer before continuing. + - role: assistant + tool_calls: + - id: toolcall_0 + type: function + function: + name: set_marker + arguments: '{"value":"go"}' + - id: toolcall_1 + type: function + function: + name: ask_user + arguments: '{"question":"Please choose between the following options:","choices":["Option A","Option B"]}' + - role: tool + tool_call_id: toolcall_0 + content: MARKER_GO + - role: tool + tool_call_id: toolcall_1 + content: "User selected: Option A" + - role: assistant + content: |- + The marker is set (MARKER_GO) and you selected **Option A**.