Conversation
With a SessionFsProvider bound, the CLI issues sessionFs.* requests while it processes session.create and waits for their responses before it answers. The SDK registered the session on the router before the RPC but only spawned the event loop that serves those requests after the response, so startup deadlocked. Register, spawn, then send the RPC for client-known session IDs, as session.resume already does; the server-assigned-ID cloud path is unchanged. Fixes #2749 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The deadlock fix and cleanup paths are covered by focused regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes a Rust session-creation deadlock by starting the event loop before session.create when the session ID is known.
Changes:
- Starts known-ID session event loops before the create RPC.
- Adds robust cleanup for startup failures and cancellation.
- Adds regression tests for request handling and parse-error cleanup.
| File | Description |
|---|---|
rust/tests/session_test.rs |
Tests filesystem requests during session creation. |
rust/tests/prepared_session_test.rs |
Tests parse-error cleanup behavior. |
rust/src/session.rs |
Implements pre-RPC event-loop startup and lifecycle cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
SDK Consistency Review — PR #2754Scope: This PR changes only What it fixes: In the non-cloud Cross-SDK check: I compared the equivalent
Because those five SDKs don't have a separate "spawn a per-session event loop" step, they were never exposed to this specific ordering bug — Conclusion: No consistency issues found. This is a targeted, language-specific bug fix that brings Rust's behavior in line with the other five SDKs' existing correct behavior. No inline comments or other-SDK changes are being requested.
|
Fixes #2749
Problem
With a
SessionFsProviderbound on the client,Client::create_sessionnever returns. While the CLI processessession.createit issues session-scoped requests (sessionFs.stat/sessionFs.writeFilefor workspace metadata) and waits for their responses before it answers the create call. The SDK registered the session on the router before the RPC, so those requests were routed into the session's request channel, but nothing drained that channel untilspawn_event_loopran, which only happened after thesession.createresponse. Both sides waited on each other.session.resumealready did this in the right order (register, spawn the event loop, then send the RPC) and does not hang.Fix
start_prepared_createnow mirrorsstart_prepared_resumefor sessions whose ID is known client-side (every non-cloud session, and cloud sessions with a caller-supplied ID): the session is registered and its event loop is spawned before thesession.createRPC is sent. The cloud arm with a server-assigned ID is unchanged: the inline response callback still registers the session the instant the response is parsed, and the loop is spawned right after, because the ID is not known earlier.To avoid duplicating the 16-argument
spawn_event_loopcall, its by-value inputs are captured once in a boxedFnOnceand a small private enum (CreateEventLoop::{Running, Deferred}) carries the loop state across the RPC. Every post-registration error path (RPC error, unparsable create result, session-ID mismatch,register_mcp_auth_interestfailure) tears down a running loop the same way resume does: cancel the shutdown token, await the loop, then unregister once. The existing cancellation guard keeps covering caller-side cancellation. No public API changes.Tests
session_fs_request_during_create_is_served_before_create_response(rust/tests/session_test.rs): the fake server issuessessionFs.statbefore answeringsession.createand asserts the SDK serves it within the test timeout, then that create completes. Fails onmain(the timeout elapses; no hang) and passes with this change.create_result_parse_error_preserves_kind_and_cleans_up(rust/tests/prepared_session_test.rs): covers the one post-registration error path that had no cleanup test.Verification
Run locally on Windows with the pinned
1.94.0toolchain:cargo test(lib,session_test,prepared_session_test,jsonrpc_test,integration_test)integration_test)cargo clippywith the CI flagslocal-runtime)cargo fmt --checkThe
e2etest target (needs the pinned CLI runtime and the test harness) was not run locally; CI covers it.Notes for reviewers
create_result.capabilitiesis still written into the shared capabilities cell after the RPC. With the loop now running during the RPC, acapabilities.changedevent processed before the response would be superseded by the response snapshot, which matches whatresumealready does.rust/src/generated/**is untouched.🤖 Generated with Claude Code