You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
That PR moved concurrency to the request-dispatch boundary: each inbound JSON-RPC request is now tokio::spawned from the requests.recv() branch of the per-session tokio::select! loop (matching the other five SDKs), rather than being awaited inline. This fixed the starvation but surfaced two lower-priority issues, called out by @stephentoub in this review comment. Both were accepted as out of scope for that PR (LGTM/approved) and are tracked here.
Follow-up items
1. Spawned request handlers can outlive stop_event_loop — docs are stale
Session::stop_event_loop (in rust/src/session.rs) cancels the shutdown token and awaits the event-loop task handle. Its docs currently claim:
Any in-flight handler (permission callback, tool call, elicitation response) completes before the loop exits, so the CLI never sees a half-handled request. See RFD-400 review finding #3.
Since request handlers are now detached tokio::spawned tasks, stop_event_loop only awaits the loop task itself — not the in-flight spawned request handlers. Those can outlive teardown, which may drop a response during shutdown. This matches sibling SDK behavior and does not affect active sessions, but the doc comment is now inaccurate.
Action: update the stop_event_loop (and any related Drop/destroy) documentation to reflect that spawned request handlers may not complete before the loop exits, and note the intentional parity with the other SDKs.
2. A panicking request handler leaves the request unanswered
Most sibling SDKs translate a panicking/throwing handler into a JSON-RPC error response. In the Rust SDK, a panic inside a spawned handle_request task simply aborts that task, leaving the single request unanswered (the caller hangs until its own timeout).
This is still an improvement over the pre-#2034 behavior, where a panic in an inline handler could take down the entire per-session event loop — now it's isolated to the one request. But for parity we should catch the panic and reply with a JSON-RPC error.
Action: wrap the spawned handle_request so a panic (or handler error) is converted into a JSON-RPC error response for that request id, matching the sibling SDKs.
Notes
Both are lower priority; neither affects active/healthy sessions.
Reference implementation for the panic→JSON-RPC-error mapping exists in the dotnet/go/python/nodejs bindings.
Context
Follow-up items from the review of #2034 (Fix
ask_userstarving the Rust SDK per-session event loop).That PR moved concurrency to the request-dispatch boundary: each inbound JSON-RPC request is now
tokio::spawned from therequests.recv()branch of the per-sessiontokio::select!loop (matching the other five SDKs), rather than being awaited inline. This fixed the starvation but surfaced two lower-priority issues, called out by @stephentoub in this review comment. Both were accepted as out of scope for that PR (LGTM/approved) and are tracked here.Follow-up items
1. Spawned request handlers can outlive
stop_event_loop— docs are staleSession::stop_event_loop(inrust/src/session.rs) cancels the shutdown token and awaits the event-loop task handle. Its docs currently claim:Since request handlers are now detached
tokio::spawned tasks,stop_event_looponly awaits the loop task itself — not the in-flight spawned request handlers. Those can outlive teardown, which may drop a response during shutdown. This matches sibling SDK behavior and does not affect active sessions, but the doc comment is now inaccurate.Action: update the
stop_event_loop(and any relatedDrop/destroy) documentation to reflect that spawned request handlers may not complete before the loop exits, and note the intentional parity with the other SDKs.2. A panicking request handler leaves the request unanswered
Most sibling SDKs translate a panicking/throwing handler into a JSON-RPC error response. In the Rust SDK, a panic inside a spawned
handle_requesttask simply aborts that task, leaving the single request unanswered (the caller hangs until its own timeout).This is still an improvement over the pre-#2034 behavior, where a panic in an inline handler could take down the entire per-session event loop — now it's isolated to the one request. But for parity we should catch the panic and reply with a JSON-RPC error.
Action: wrap the spawned
handle_requestso a panic (or handler error) is converted into a JSON-RPC error response for that request id, matching the sibling SDKs.Notes
ask_userstarving the Rust SDK per-session event loop #2034: the other bindings'userInput.requesthandlers are also worth an audit for the original inline-await asymmetry.