github / github/copilot-sdk

Rust: `create_session` deadlocks forever when `ClientOptions::session_fs` is set — the session's request consumer starts after the `session.create` RPC

Đang mở
#2,624 0 bình luận 1 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Java
Star
10.5k
Fork
1.5k
Merge trung bình
1 ngày 14 giờ
Pull request đã merge (30 ngày)
129

Mô tả

## Summary

In the Rust SDK, `Client::create_session` hangs **indefinitely** for any client configured with `ClientOptions::with_session_fs(..)`. The CLI issues a `sessionFs.readFile` while `session.create` is still in flight, and the SDK never answers it, so both sides wait forever.

The Rust SDK pre-registers the session on the router before the RPC (correctly), but the only consumer of that session's request channel — and the only place the `SessionFsProvider` is installed — is `spawn_event_loop`, which runs **after** the create RPC returns. Registration alone only *queues* the inbound request; nothing answers it.

The Go and Node.js SDKs do not have this bug: both start the session's request handling *eagerly, before* the RPC. So this is a Rust-specific divergence from the behaviour the other SDKs already document and implement.

Version: `github-copilot-sdk` 1.0.13 (latest published). CLI 1.0.83 (the release pinned in the crate's own `cli-version.txt`), run as an external server (`copilot --server --port N`). Reproduced on Linux x86_64 across 8+ fresh server instances. **Still present on `main` as of today.**

## Wire trace

Captured with a logging TCP proxy between the SDK and the CLI server:

```
[7.271] C->S REQ id=3 session.create {… "sessionId": "2677a0ec-0f2f-4290-9a8c-b80b903470a3" …}
[7.691] S->C REQ id=1 sessionFs.readFile {"path": "…/.session-state/workspace.yaml",
"sessionId": "2677a0ec-0f2f-4290-9a8c-b80b903470a3"}
[67.27] -- connection closed (client-side 60s timeout; the SDK never responded) --
```

The session IDs match, so the request *is* routed to a registered session — it is queued and never drained, not misrouted.

## Root cause (`rust/src/session.rs`, `start_prepared_create`)

1. `~1175` — the session is registered before the RPC, with a comment showing the window is known and intentional:
> For non-cloud sessions we generate the id client-side … so the session can be registered BEFORE the RPC — the CLI may issue session-scoped requests (e.g. `sessionFs.writeFile` for workspace metadata) during `session.create` processing, before it has sent the response.
2. `rust/src/router.rs` — `register()` creates an `mpsc::unbounded_channel()`; the routing task does `sender.send(request)` for a registered session. The receiver is handed out in `SessionRegistration::channels`.
3. `~1394` — `call_with_inline_callback("session.create", …).await?`
4. `~1419` — `spawn_event_loop(…, session_fs_provider, channels, …)` — **the first and only consumer of that receiver**, and the only place `session_fs_provider` is installed (`~2968`: `session_fs_dispatch::dispatch`).

Steps 3 and 4 are in the wrong order for the window step 1 deliberately creates.

`start_prepared_resume` has the same ordering (`~1636`), so `resume_session` should be affected identically.

## Why the other SDKs are unaffected

Both start the consumer before the RPC:

- **Go** (`go/client.go` ~1093): *"Pre-register non-cloud sessions BEFORE issuing the RPC so any session-scoped requests the CLI emits during session.create processing (e.g. sessionFs.writeFile for workspace metadata) **can be routed to the correct handlers**."* `initializeSession()` is called there, and `newSession` *"starts processEvents eagerly, before the RPC confirms"* (~1447). (#2320 — the eagerly-started `processEvents` goroutine leaking on create failure — is further confirmation Go starts it early.)
- **Node.js** (`nodejs/src/client.ts` ~1627): same comment; `initializeSession(localSessionId)` → `setupSessionFs(s, config)` runs before `sendRequest("session.create", …)`.

Rust is the outlier: it performs the *registration* half of that pattern but not the *handler-installation* half.

## Things that are not workarounds

- `prepare_session(cfg)?.start()` — same code path (`start_prepared_create`).
- Draining the queue from application code — `Client::register_session` and `SessionChannels` are `pub(crate)`.
- Serving the callback from a second `Client` — `sessionFs.setProvider` is connection-level and exclusive (*"Another client is already the session filesystem provider"*).
- Upgrading — 1.0.13 is the latest published version.
- `with_base_directory` instead of `session_fs` does avoid the hang (no `sessionFs` RPC is ever issued), but that defeats the purpose for anyone using `session_fs` as their isolation boundary.

## Suggested fix (verified)

Start the event loop before the create RPC on the non-cloud path, matching Go/Node. The loop is purely reactive — no startup RPC, no dependency on the create response — and `capabilities` is already a shared `Arc>` written once the response arrives. On an early return the armed `PendingSessionRegistration` cancels `shutdown`, which is the loop's own exit condition, so failure cleanup is unchanged.

Concretely: hoist the `spawn_event_loop(..)` call into a one-shot closure and invoke it immediately after `PendingSessionRegistration` is armed when `local_session_id.is_some()`, taking the channels from the stash there; leave the cloud path (server-assigned ID) spawning after the response as it does today.

I applied exactly this to a local 1.0.13 checkout and re-ran the same scenario. `session.create` now completes in ~460 ms, having answered 12 provider calls mid-flight:

```
[4.375] C->S REQ id=3 session.create
[4.824] S->C REQ id=1 sessionFs.readFile -> [4.824] C->S RESP id=1 ok
[4.825] S->C REQ id=2 sessionFs.mkdir -> [4.825] C->S RESP id=2 ok
… mkdir ×3, writeFile ×2, rename, readFile ×2, readdirWithTypes …
[4.834] S->C RESP id=3 session.create (returns)
```

(The create then returns a normal application-level error about a custom agent missing from my own plugin directory — an unrelated problem on my side, and exactly what the `base_directory` variant reports too.)

Happy to open a PR with this change plus a regression test if that's useful.

## Reproduction

Any Rust client with `ClientMode::Empty` + `with_session_fs(..)` + a `SessionFsProvider`, against an external `copilot --server`. It does not require a real Copilot entitlement to observe: a mock JSON-RPC server that answers `connect` and `sessionFs.setProvider`, then issues a `sessionFs.readFile` on receiving `session.create` and waits, reproduces the hang deterministically — the SDK never sends a response.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu trong rust/src/session.rs tại start_prepared_create và so sánh thứ tự của nó với start_prepared_resume; đọc rust/src/router.rs để hiểu các yêu cầu phiên đã đăng ký được xếp hàng và tiêu thụ như thế nào. Di chuyển việc khởi động event loop lên trước RPC create hoặc resume không thuộc cloud, sau đó thêm một regression test bằng mock JSON-RPC server đã mô tả và xác minh rằng các yêu cầu sessionFs nhận được phản hồi trong khi RPC đang diễn ra.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
rust
Lĩnh vực
api, backend
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
76/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.