feat(rust): route cloud sessions through create_session#1445
Open
chuwik wants to merge 10 commits into
Open
Conversation
Cloud (Mission Control) sessions don't fit the create_session contract: the runtime owns the session id, forbids caller-provided sessionId and provider, and may emit session.event notifications or inbound JSON-RPC requests before the session.create response. Add a dedicated Client::create_cloud_session entry point that: - Omits sessionId from the session.create wire (new SessionConfig::into_cloud_wire via shared into_create_wire(Option)). - Rejects caller-provided session_id and provider with InvalidConfig. - Buffers early session-keyed notifications and inbound requests through a refcounted PendingSessionRouting guard with a bounded (128) drop-oldest queue, replayed when register_session installs channels for the runtime-assigned id. - Holds the guard across a best-effort session.destroy on decode failure, then drops it. Also: - Client::create_session now rejects config.cloud with InvalidConfig, pointing callers at create_cloud_session. - Factor the shared handler/sessionFs extraction into prepare_session_runtime, reused by create / create_cloud / resume. - Five router unit tests cover off-mode drop, on-mode buffer + ordered flush, drop-oldest at limit, last-guard-drop clears, and unregister-clears-pending. - Six integration tests cover the cloud-create wire, both rejection paths, handler-driven request flags, and early-notification + early-request buffering, against the existing fake JSON-RPC server. Ports github/github-app#5592 into the canonical SDK so the app can drop its vendored CloudBootstrapWire hack on the next refresh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tokio tasks are cheap; the lazy ensure_started + idempotency guard existed only because cloud session creation needed the router running before any session was registered. Starting both routing tasks once during Client::from_streams collapses the lazy/eager split and lets register_session and begin_pending_session_routing become plain delegations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the pending router buffered notifications and inbound JSON-RPC requests in two separate VecDeques and on register flushed all notifications, then all requests. That meant a request arriving between two notifications would be delivered to the consumer after both notifications instead of in arrival order, batching cross-type messages in a way the docs claimed not to. Some downstream behaviors care: an 'approval requested' session event observed before its matching 'tool.call' request keeps the consumer's permission/elicitation flow coherent. Collapse the two queues into a single FIFO of PendingItem (a notif/req enum), apply the drop-oldest limit globally, and flush in arrival order. Adds a regression test that interleaves a request between two notifications and verifies cross-type ordering survives the flush. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ests When the pending session buffer overflows during cloud session creation, we still have to evict the oldest entry to make room. For notifications that's fine; for inbound JSON-RPC requests it left the runtime hanging on a reply that would never come (until the runtime's own timeout). Add a sync WriterHandle on JsonRpcClient that lets the router enqueue fire-and-forget frames from inside its parking_lot::Mutex. On request eviction, send a JSON-RPC error response (-32603 INTERNAL_ERROR, message 'request dropped: pending session buffer overflow before session.create response') for the evicted request's id before discarding it. Notifications continue to drop with a warn-level log. Test reads bytes back off a duplex stream and asserts the error response is well-formed and carries the expected id and code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI failed before tests because nightly rustfmt reordered imports and wrapped a router test call differently than the committed source. Running the pinned formatter produced the expected import grouping and line wrapping without changing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…out register GPT-5.5 self-review surfaced two issues in the pending-session router: 1. When the last PendingSessionRouting guard dropped without anyone calling register_session (e.g. session.create failed mid-RPC), any buffered inbound JSON-RPC requests were silently discarded. The runtime is waiting on those ids and would hang until its own timeout. This was a symmetric gap to the overflow path fixed in 491b442. Fix: Drop impl now drains pending entries and emits an INTERNAL_ERROR response ("pending session routing ended before session was registered") for every PendingItem::Request before clearing. Notifications still just warn-log on the way out. WriterHandle is now Clone so the snapshot can be taken under lock and the lock released before per-item work. 2. The 'cross-type arrival order' claim from c802943 overstated what the unified FIFO actually guarantees. After register, items still flow through two separate per-session mpsc channels consumed via select! in session.rs, so consumer-observed cross-type order is implementation-defined. Updated push_pending docs to describe the actual guarantee (FIFO eviction + interleaved flush) and renamed the test from pending_buffer_preserves_cross_type_arrival_order to pending_buffer_flush_interleaves_types_in_arrival_order. Unifying per-session channels into one stream is tracked as a follow-up. New router test: last_guard_drop_without_register_responds_to_buffered_requests. 8 router tests + 101 session tests all pass; clippy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # rust/src/session.rs # rust/tests/session_test.rs
Start from the Rust cloud session support in PR #1394 and keep the public session creation API on Client::create_session. Cloud configs now delegate to a private cloud creation path that preserves pending routing for runtime-assigned session IDs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Rust SDK so cloud-backed sessions are created through the existing Client::create_session API while preserving runtime-assigned session IDs and buffering early runtime traffic.
Changes:
- Adds cloud-session routing in
create_session, including validation, cloud wire payloads withoutsessionId, and runtime-assigned ID registration. - Refactors session runtime preparation shared by create/resume paths.
- Extends the router to buffer early cloud notifications/requests and respond to dropped pending requests.
Show a summary per file
| File | Description |
|---|---|
rust/src/session.rs |
Routes cloud configs through internal cloud creation and shares runtime preparation logic. |
rust/src/router.rs |
Adds pending-session buffering, overflow handling, and router tests. |
rust/src/lib.rs |
Starts the session router during client transport setup and exposes pending routing internally. |
rust/src/jsonrpc.rs |
Adds a fire-and-forget writer handle for router-generated JSON-RPC error responses. |
rust/src/types.rs |
Adds cloud-session documentation and shared local/cloud wire conversion. |
rust/src/wire.rs |
Makes sessionId optional on session.create wire payloads. |
rust/tests/session_test.rs |
Adds integration coverage for cloud create wire shape, validation, handler flags, and early buffering. |
rust/README.md |
Documents cloud sessions through Client::create_session. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 1
Comment on lines
+990
to
+1005
| let recovered_session_id = result | ||
| .get("sessionId") | ||
| .and_then(|value| value.as_str()) | ||
| .map(SessionId::from); | ||
| let create_result: CreateSessionResult = match serde_json::from_value(result) { | ||
| Ok(result) => result, | ||
| Err(error) => { | ||
| // Keep the pending guard alive across the destroy so any | ||
| // straggler events for the runtime-assigned id are still | ||
| // routed (and then dropped on guard release). | ||
| if let Some(recovered_id) = recovered_session_id { | ||
| if let Err(destroy_err) = self | ||
| .call( | ||
| "session.destroy", | ||
| Some(serde_json::json!({ "sessionId": recovered_id })), | ||
| ) |
Remove the pending-routing buffer for cloud session creation. Cloud sessions now register after the runtime returns the session id, and any earlier unknown-session traffic is dropped by the normal router path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapse local and cloud session creation onto shared request preparation, event-loop startup, and Session construction helpers so cloud support does not duplicate the normal create path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SessionConfig::with_cloud.Client::create_sessionwhile keeping cloud creation as an internal helper.session.createfor registration and session state.Context
Builds on #1394 while preserving the existing public
Client::create_sessionAPI.