Defer sessionId to server for cloud sessions#1479
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors session creation across all six SDKs (Node.js, Python, Go, .NET, Rust, Java) to defer sessionId assignment to the CLI/server when the caller doesn't supply one. Each SDK's JSON-RPC layer gains an "inline response callback" hook that runs synchronously in the read loop the instant a successful response is parsed, so the new session can be registered before any subsequent session.event notification is dispatched. This replaces per-SDK pre-generation of UUIDs and pre-registration of sessions, and consolidates the .NET session-init logic into a shared InitializeSession helper.
Changes:
- Add
inline response callbackplumbing in each SDK's JSON-RPC client (jsonrpc.rs/JsonRpc.cs/_jsonrpc.py/jsonrpc2.go/vscode-jsonrpc microtask-based /CompletableFuture.thenApply). - Always lazy-initialize the session from the
session.createresponse, removing client-sideUUID/randomUUIDgeneration and the cloud-only branching. - Validate that the returned
sessionIdmatches a caller-supplied id (mismatch → error), and ensure failure paths only unregister sessions actually registered.
Show a summary per file
| File | Description |
|---|---|
| rust/src/wire.rs | Make SessionCreateWire.session_id optional with skip-if-none. |
| rust/src/types.rs | Update SessionConfig::into_wire and tests to take Option<SessionId>. |
| rust/src/session.rs | Defer session registration to inline callback after session.create response; remove pre-registration. |
| rust/src/lib.rs | Add call_with_inline_callback; delete obsolete pending-registration cancel test. |
| rust/src/jsonrpc.rs | Add InlineResponseCallback + PendingRequest; run callback synchronously in read loop. |
| python/copilot/_jsonrpc.py | Add on_response_inline kwarg, store + invoke synchronously in reader thread. |
| python/copilot/client.py | Replace pre-generated UUID and pre-registration with lazy _initialize_session via inline callback. |
| python/test_client.py | Update mocks to accept **kwargs and invoke the inline callback. |
| nodejs/src/client.ts | Lazy-init session after response; throw on mismatch; remove randomUUID() path. |
| nodejs/test/{client,toolSet}.test.ts | Mocks fall back to "session-id" when caller doesn't supply one. |
| go/internal/jsonrpc2/jsonrpc2.go | Add RequestWithInlineResponse; run callback in read loop with panic recovery. |
| go/client.go | Defer session registration to inline callback; clean up only registered sessions on error. |
| java/src/main/java/com/github/copilot/CopilotClient.java | Initialize session inside thenApply; remove pre-registration and rekey-on-mismatch branch; throw on mismatch. |
| dotnet/src/JsonRpc.cs | Add onResponseInline parameter; invoke synchronously after parsing result. |
| dotnet/src/Client.cs | Extract InitializeSession helper shared by Create/Resume; defer create to inline callback; misc unrelated refactors (field ordering, lazy _modelsCacheLock, lock-on-collection). |
| dotnet/test/Unit/{JsonRpcTests,ClientSessionLifetimeTests}.cs | Pass null for new param; fall back to a generated id when caller doesn't supply one. |
Copilot's findings
- Files reviewed: 18/18 changed files
- Comments generated: 0
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
stephentoub
force-pushed
the
stephentoub/cleanup1
branch
from
d81d4d4 to
6dbb5e8
Compare
stephentoub
force-pushed
the
stephentoub/cleanup1
branch
2 times, most recently
from
333d5b9 to
f3e65ee
Compare
This comment has been minimized.
This comment has been minimized.
stephentoub
force-pushed
the
stephentoub/cleanup1
branch
from
f3e65ee to
42b26b1
Compare
This comment has been minimized.
This comment has been minimized.
stephentoub
force-pushed
the
stephentoub/cleanup1
branch
from
42b26b1 to
544ef71
Compare
Contributor
Cross-SDK Consistency Review ✅All six SDKs (Node.js, Python, Go, .NET, Java, Rust) implement the same logic consistently in this PR:
The extracted Note on PR description accuracyThe PR title ("Always defer sessionId to CLI") and description ("removes
This is a reasonable design (non-cloud CLIs may issue
|
stephentoub
changed the title
When a caller creates a session with `cloud` set and does not supply a `sessionId`, the SDK now omits `sessionId` from the `session.create` request and lets the CLI/server assign one. The returned id is captured synchronously when the response is parsed (via an inline response callback) so that any subsequent `session.event` notification routes to the registered session without race. For all other cases (no `cloud`, or caller supplied `sessionId`), the SDK continues to generate a UUID client-side (when none is supplied) and pre-register the session BEFORE issuing the RPC. Pre-registration is required because the CLI may issue session-scoped requests (e.g. `sessionFs.writeFile` for workspace metadata) during `session.create` processing, before it has sent the response — without pre-registration those requests would have no handler. A caller-supplied `sessionId` is always passed to the server so the server can validate it; mismatched returned ids fail the call with a clear error. In addition, the .NET implementation extracts the session-init local function from CreateSessionAsync into a private InitializeSession helper that is now shared by both CreateSessionAsync and ResumeSessionAsync, eliminating duplicated handler-wiring code. Per-SDK summary: .NET (dotnet/src/Client.cs): New private InitializeSession helper shared by CreateSessionAsync and ResumeSessionAsync. CreateSessionAsync branches on (Cloud != null && SessionId == null): the cloud/no-id path registers the session lazily from an `onResponseInline` callback; all other paths pre-register before the RPC. Validates that the server- returned id matches a caller-supplied id. Python (python/copilot/client.py): create_session uses the same conditional. Cloud+no-id installs an `on_response_inline` callback that registers the session as soon as the response is parsed. Otherwise the session is pre-registered via uuid.uuid4() (or the caller-supplied id). Go (go/client.go): createSession uses the same conditional. Cloud+no-id installs an inline callback that registers from the read loop the instant the response arrives. Otherwise the session is pre-registered via uuid.NewString() (or the caller-supplied id). Rust (rust/src/session.rs): create_session uses the same conditional. Cloud+no-id installs an InlineResponseCallback that validates the returned id and registers in the router. Otherwise the session is pre-registered via uuid::Uuid::new_v4() (or the caller-supplied id). SessionConfig::into_wire now takes the local session id (or None when the server is generating one). Java (java/src/main/java/com/github/copilot/CopilotClient.java): createSession uses the same conditional. Cloud+no-id defers registration to the .thenCompose continuation (race-free because CompletableFuture sync continuations run on the JSON-RPC reader thread). Otherwise the session is pre-registered via UUID.randomUUID() (or the caller-supplied id) before the RPC. Node.js (nodejs/src/client.ts): createSession uses the same conditional. Cloud+no-id lazy-initializes after the response (race-free because vscode-jsonrpc dispatches each message via setImmediate, so microtasks flush before the next message is read). Otherwise the session is pre-registered via randomUUID() (or the caller-supplied id). Co-authored-by: Copilot <[email protected]>
stephentoub
changed the title
stephentoub
force-pushed
the
stephentoub/cleanup1
branch
from
544ef71 to
9e7841f
Compare
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
When a caller creates a session with
cloudset and does not supply asessionId, the SDK now omitssessionIdfrom thesession.createrequest and lets the CLI/server assign one. The returned id is captured synchronously when the response is parsed (via an inline response callback) so any subsequentsession.eventnotification routes to the registered session without race.For all other cases (no
cloud, or caller-suppliedsessionId), the SDK continues to generate a UUID client-side (when none is supplied) and pre-register the session BEFORE issuing the RPC. Pre-registration is required because the CLI may issue session-scoped requests (e.g.sessionFs.writeFilefor workspace metadata) DURINGsession.createprocessing, before it has sent the response — without pre-registration those requests would have no handler.A caller-supplied
sessionIdis always forwarded to the server (even withcloud), so the server can validate it. A mismatched returned id fails the call with a clear error.In addition, the .NET implementation extracts the session-init local function from
CreateSessionAsyncinto a privateInitializeSessionhelper now shared by bothCreateSessionAsyncandResumeSessionAsync, eliminating duplicated handler-wiring code.Decision matrix
cloudsessionIdsessionIdfrom RPC; register lazily from inline callback.What changed per SDK
The same conditional (
useServerGeneratedId = cloud != null && callerSessionId == null) is applied uniformly across all 6 SDKs:dotnet/src/Client.cs): New privateInitializeSessionhelper shared byCreateSessionAsyncandResumeSessionAsync.CreateSessionAsynceither pre-registers (default) or installs anonResponseInlinecallback (cloud + no id).python/copilot/client.py):create_sessioneither pre-registers viauuid.uuid4()or installs anon_response_inlinecallback that registers when the response is parsed.go/client.go):CreateSessioneither pre-registers viauuid.NewString()or installs an inline callback that registers from the read loop the instant the response arrives.rust/src/session.rs):create_sessioneither pre-registers viauuid::Uuid::new_v4()or installs anInlineResponseCallbackthat validates the returned id and registers in the router.SessionConfig::into_wirenow takes the resolved local session id (orNonewhen the server is generating one).java/src/main/java/com/github/copilot/CopilotClient.java):createSessioneither pre-registers viaUUID.randomUUID()or registers in the.thenComposecontinuation (race-free — CompletableFuture sync continuations run on the JSON-RPC reader thread).nodejs/src/client.ts):createSessioneither pre-registers viarandomUUID()or lazy-initializes after the response (race-free — vscode-jsonrpc dispatches each message viasetImmediate, so microtasks flush before the next message is read).Why hybrid instead of always-lazy?
An earlier iteration of this PR took the "always lazy-init" path. CI surfaced that the CLI emits
sessionFs.writeFilefor workspace metadata DURINGsession.createprocessing (before the response). For non-cloud sessions the SDK must have a registered handler at that moment, which is only possible if it pre-registers before sending the RPC. Cloud sessions don't have a workspace and so don't hit that path, which is why deferring to the server is safe there.Testing
All SDK unit tests and lints pass locally. The .NET
CreateSessionReKeyEntryTest(Java) was rewritten to assert that mismatched ids are rejected (the previous "re-key on mismatch" behavior is intentionally gone).E2E suites are validated in CI on the PR.