Skip to content

Fix ask_user starving the Rust SDK per-session event loop#2034

Open
pallaviraiturkar0 wants to merge 1 commit into
github:mainfrom
pallaviraiturkar0:pallaviraiturkar0-fix-rust-ask-user-event-loop
Open

Fix ask_user starving the Rust SDK per-session event loop#2034
pallaviraiturkar0 wants to merge 1 commit into
github:mainfrom
pallaviraiturkar0:pallaviraiturkar0-fix-rust-ask-user-event-loop

Conversation

@pallaviraiturkar0

Copy link
Copy Markdown
Contributor

Problem

Each session runs one tokio::select! loop in rust/src/session.rs. Interactive callbacks that arrive as notifications (tool-call, permission, elicitation) are tokio::spawned and run concurrently. But ask_user arrives as a JSON-RPC request (userInput.request) and was handled by handle_request(...).await inline in the select loop.

While the user's answer is pending (the host applies a ~5-minute backstop timeout), the loop is parked and cannot drain the next notification. A sibling tool call co-emitted in the same turn — e.g. set_session_title + ask_user (github/copilot-experiences#12540) — is therefore starved, so it never completes and the UI appears frozen.

The root cause is a dispatch asymmetry: tool/permission/elicitation are spawned, but only userInput.request was awaited inline. The host already yields correctly, so the fix is SDK-only.

Fix

In the "userInput.request" arm of handle_request, parse question/choices/allowFreeform up front (keeping the existing inline INVALID_PARAMS error for a missing question, which is instant), then spawn a child task that runs UserInputHandler::handle(...) and sends the JSON-RPC response — instead of awaiting inline. This mirrors the existing permission-request spawn pattern (clone the Client/session id/handler Arc, wrap in a tracing::error_span! + .instrument). The select loop now keeps draining notifications while ask_user is pending.

The change is intentionally minimal and surgical — no refactor beyond moving the dispatch to a spawned task.

Test

Adds an e2e regression test in rust/tests/e2e/ask_user.rs: ask_user_does_not_block_sibling_tool_call_in_same_turn. The model emits both a custom set_marker tool and ask_user in one turn; set_marker fires a Notify, and the UserInputHandler waits (bounded) on that Notify before answering, then asserts it observed the sibling tool executing while its own userInput.request was still pending. Backed by a new replay snapshot under test/snapshots/ask_user/.

Verified the test fails on the pre-fix inline-await code (assertion trips after ~34s) and passes with the fix (~6s). cargo build, cargo fmt --check, cargo clippy, cargo test --lib (187 passed), and the ask_user + parallel-tools e2e tests all pass.

Follow-up (not in this PR)

The parallel non-Rust bindings (dotnet / go / python / nodejs) and the bundled core may share the same inline-await asymmetry for userInput.request and are worth a follow-up audit. This PR deliberately changes only the Rust SDK.

Copilot AI review requested due to automatic review settings July 20, 2026 14:53
@pallaviraiturkar0
pallaviraiturkar0 requested a review from a team as a code owner July 20, 2026 14:53
Comment thread rust/src/session.rs Fixed
Comment thread rust/src/session.rs Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents pending Rust ask_user requests from starving sibling session events.

Changes:

  • Dispatches user-input handlers concurrently.
  • Adds an E2E regression test and replay snapshot.
Show a summary per file
File Description
rust/src/session.rs Spawns user-input request handling.
rust/tests/e2e/ask_user.rs Tests concurrent sibling tool execution.
test/snapshots/ask_user/ask_user_does_not_block_sibling_tool_call_in_same_turn.yaml Adds replay data for the regression test.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread rust/src/session.rs Outdated
@pallaviraiturkar0

Copy link
Copy Markdown
Contributor Author

Replies to the automated review threads (in-thread on Files changed):

  • Copilot — detached task vs. stop_event_loop drain: Valid at the subsystem level, but pre-existing — permission/tool/elicitation handlers already dispatch via detached tokio::spawn (RFD-400). This PR just moves userInput.request onto that same path, making it consistent rather than newly divergent. JoinSet-based draining is worth doing across all handlers as a follow-up, not just this one.
  • CodeQL — answer / was_freeform unused: False positives. Both are used in the serde_json::json! macro on the next lines; cargo build/clippy report no unused_variables warning.

@pallaviraiturkar0

Copy link
Copy Markdown
Contributor Author

The prior Rust SDK Tests (ubuntu-latest, default) failure was an unrelated flake, not a regression from this PR:

  • Failing test: pending_work_resume::should_report_continuependingwork_true_in_resume_event, which panicked at the CLI startup step (Client::spawn_tcpCliStartupFailed, lib.rs:1760) before any session logic runs.
  • Result was 387 passed; 1 failed, and the new test for this PR (ask_user::ask_user_does_not_block_sibling_tool_call_in_same_turn) passed.
  • This PR touches only session.rs, ask_user.rs, and the snapshot — nothing in lib.rs / TCP startup / pending-work resume.

I re-triggered CI (reopen) since I don't have admin to re-run the job directly.

@stephentoub

Copy link
Copy Markdown
Collaborator

Thanks for the fix. It looks like Rust is actually the outlier here; the other 5 SDKs do it "correctly".

My preference though would be to generalize this further to better bring it in line with the other SDKs. Can we consider moving concurrency to the request-dispatch boundary? The current patch is correct, but it fixes one manifestation rather than the architectural cause. The other SDKs are only “fire-and-forget” from the reader loop: each inbound JSON-RPC request gets its own task/goroutine/future, which awaits the handler and sends that request’s response. Rust could similarly spawn handle_request from the requests.recv() branch rather than spawning only userInput.request internally. That would also prevent starvation from exitPlanMode, autoModeSwitch, hooks, transforms, canvas/session-FS providers, or any future slow handler. JSON-RPC permits concurrent requests and out-of-order responses, and the other SDKs establish that serialization is not part of the SDK contract.

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
@pallaviraiturkar0
pallaviraiturkar0 force-pushed the pallaviraiturkar0-fix-rust-ask-user-event-loop branch from bb78252 to d2a0bb8 Compare July 20, 2026 16:57
@pallaviraiturkar0

pallaviraiturkar0 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review @stephentoub — done. Moved concurrency to the request-dispatch boundary instead of special-casing userInput.request:

  • The requests.recv() branch of the per-session tokio::select! loop now tokio::spawns each handle_request as its own task that awaits the handler and sends that request's response, matching how the other five SDKs already dispatch inbound requests. The Arc-backed dispatch context is cloned into the task so the future is 'static.
  • This fixes starvation for every request handler (exitPlanMode, autoModeSwitch, hooks, transforms, canvas/session-FS providers, …), not just ask_user.
  • The userInput.request arm itself is now unchanged from main — reverted the earlier internal-spawn hack.
  • Notifications stay inline (they only do fast dispatch; their slow permission/tool/elicitation callbacks already spawn).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants