Skip to content

fix(rust): start the session event loop before the session.create RPC - #2754

Open
Lukaze wants to merge 1 commit into
mainfrom
fix/rust-session-create-event-loop-order
Open

Lukaze wants to merge 1 commit into
mainfrom
fix/rust-session-create-event-loop-order

Conversation

@Lukaze

@Lukaze Lukaze commented Sep 23, 2026

Copy link
Copy Markdown

Fixes #2749

Problem

With a SessionFsProvider bound on the client, Client::create_session never returns. While the CLI processes session.create it issues session-scoped requests (sessionFs.stat / sessionFs.writeFile for 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 until spawn_event_loop ran, which only happened after the session.create response. Both sides waited on each other.

session.resume already did this in the right order (register, spawn the event loop, then send the RPC) and does not hang.

Fix

start_prepared_create now mirrors start_prepared_resume for 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 the session.create RPC 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_loop call, its by-value inputs are captured once in a boxed FnOnce and 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_interest failure) 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 issues sessionFs.stat before answering session.create and asserts the SDK serves it within the test timeout, then that create completes. Fails on main (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

cargo test --no-default-features --features test-support,derive --lib --test session_test --test prepared_session_test --test jsonrpc_test --test integration_test
cargo clippy --all-targets --no-default-features --features test-support,local-runtime,derive -- --no-deps -D warnings -D clippy::unwrap_used -D clippy::disallowed_macros -D clippy::await_holding_invalid_type
cargo +nightly-2026-04-14 fmt --all -- --config-path .rustfmt.nightly.toml --check

Run locally on Windows with the pinned 1.94.0 toolchain:

check result
cargo test (lib, session_test, prepared_session_test, jsonrpc_test, integration_test) 247 + 152 + 27 + 11 passed, 0 failed (3 ignored in integration_test)
cargo clippy with the CI flags exit 0 (only the pre-existing build-script warning about local-runtime)
nightly cargo fmt --check exit 0

The e2e test target (needs the pinned CLI runtime and the test harness) was not run locally; CI covers it.

Notes for reviewers

  • create_result.capabilities is still written into the shared capabilities cell after the RPC. With the loop now running during the RPC, a capabilities.changed event processed before the response would be superseded by the response snapshot, which matches what resume already does.
  • rust/src/generated/** is untouched.

🤖 Generated with Claude Code

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>
@Lukaze
Lukaze requested a review from a team as a code owner September 23, 2026 02:42
Copilot AI balanced review requested due to automatic review settings September 23, 2026 02:42

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.

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.

@github-actions

Copy link
Copy Markdown
Contributor

SDK Consistency Review — PR #2754

Scope: This PR changes only rust/src/session.rs (plus two Rust test files). No other SDK files are touched.

What it fixes: In the non-cloud create_session path, Rust was sending the session.create RPC before spawning the session's event loop. If the CLI issues session-scoped requests (e.g. sessionFs.stat) while still processing session.create, nothing was dispatching them yet, causing a deadlock (#2749). The fix spawns the event loop synchronously alongside router registration, before the RPC is sent — mirroring what session.resume already does in Rust.

Cross-SDK check: I compared the equivalent createSession/CreateSession paths in Node.js (nodejs/src/client.ts), Python (python/copilot/client.py), Go (go/client.go), .NET (dotnet/src/Client.cs), and Java (java/sdk/src/main/java/com/github/copilot/CopilotClient.java). All five already:

  • Generate/accept the session ID client-side for non-cloud sessions.
  • Register the session (and its sessionFs handler via newSessionFSAdapter/createSessionFsAdapter/ConfigureSessionFsHandlers/etc.) before issuing the session.create RPC.
  • Dispatch inbound sessionFs.* requests through a single, already-active connection-level request handler (connection.onRequest, SetRequestHandler, the reader thread, RpcHandler, etc.) rather than a per-session spawned task/thread.

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 — sessionFs requests are already routed correctly the moment the session is registered in the shared session map. This looks like a genuine Rust-only architectural quirk (Rust dispatches session events via a per-session spawned Tokio task, unlike the shared-dispatcher model used elsewhere), not a case of a feature/fix being applied to only one SDK while others need the same change.

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.

Generated by SDK Consistency Review Agent for #2754 · copilot · sonnet50 · 72.2 AIC · ⌖ 12.5 AIC · ⊞ 8.1K ·

This branch has not been deployed

No deployments
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.

session.create deadlocks when a SessionFsProvider is bound

2 participants