Lazily initialize sessions on session.created lifecycle events#1452
Draft
stephentoub wants to merge 1 commit into
Draft
Lazily initialize sessions on session.created lifecycle events#1452stephentoub wants to merge 1 commit into
stephentoub wants to merge 1 commit into
Conversation
For cloud session creation, defer CopilotSession construction until the session.created lifecycle event arrives with the server-assigned session ID. This avoids registering a session under a provisional client-generated ID that will be replaced by the server, and ensures the session object is immediately keyed by its real ID. Changes across all SDKs (Node, .NET, Go, Python, Rust): - Extract a configureSession/CreateConfiguredSession helper for session setup - Add pendingCloudSessionCreates map to track deferred session factories - On session.created lifecycle event with a clientSessionId, invoke the pending factory to materialize the session under the real ID - Surface clientSessionId on SessionLifecycleEvent types - Mark cloud session APIs as experimental where applicable Co-authored-by: Copilot <[email protected]>
Contributor
Cross-SDK Consistency ReviewThe PR description says "All SDKs (Node, .NET, Go, Python, Rust)" — and that's correct — but the Java SDK is not included and is now out of sync. Missing Java changes1. All five updated SDKs add a // Should be added (marked `@Experimental` analogous to other SDKs):
`@JsonProperty`("clientSessionId")
private String clientSessionId;
public String getClientSessionId() { return clientSessionId; }
public void setClientSessionId(String clientSessionId) { this.clientSessionId = clientSessionId; }2.
3. Lazy initialization for cloud sessions
Java has Summary
Suggest following up with a Java PR that mirrors the same changes (or including Java in this PR if it's still feasible).
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates all SDKs to support deferring (“lazy”) session construction for cloud session creation until a session.created lifecycle event provides the server-assigned session ID, avoiding provisional-ID registration and adding clientSessionId correlation on lifecycle events.
Changes:
- Add
clientSessionIdto session lifecycle event types across SDKs and mark cloud session APIs as experimental. - Introduce per-client “pending cloud create” registries to materialize sessions once the created lifecycle event arrives.
- Refactor session setup/registration into shared helpers (where applicable) to reduce duplication between create/resume paths.
Show a summary per file
| File | Description |
|---|---|
| rust/src/wire.rs | Makes sessionId optional in the session.create wire payload. |
| rust/src/types.rs | Adds clientSessionId to lifecycle events; marks cloud options/config as experimental; updates wire creation call sites. |
| rust/src/session.rs | Implements pending cloud-create materialization and runtime-part extraction/cleanup helpers for lazy initialization. |
| rust/src/router.rs | Hooks lifecycle notifications to trigger pending cloud session materialization on session.created. |
| rust/src/lib.rs | Adds pending cloud-create storage to ClientInner and wires it into router startup. |
| python/copilot/client.py | Adds client_session_id to lifecycle events; tracks pending cloud creates and attempts to re-key sessions. |
| nodejs/src/types.ts | Adds experimental annotations for cloud options/config and clientSessionId on lifecycle events. |
| nodejs/src/client.ts | Adds pending cloud-create map and a shared session configuration helper; materializes sessions on lifecycle created. |
| go/types.go | Adds experimental docs for cloud options/config; adds clientSessionId to lifecycle events. |
| go/client.go | Adds pending cloud create materialization and lifecycle-triggered initialization for cloud sessions. |
| dotnet/src/Types.cs | Marks cloud session APIs experimental and adds ClientSessionId to lifecycle event type. |
| dotnet/src/Client.cs | Adds pending cloud-create initialization and shared session configuration helper; lifecycle-triggered materialization. |
Copilot's findings
- Files reviewed: 12/12 changed files
- Comments generated: 10
Comment on lines
+812
to
+814
| if is_cloud_create { | ||
| wire.session_id = None; | ||
| } |
Comment on lines
+964
to
+973
| if is_cloud_create && materialized_cloud_session.lock().is_none() { | ||
| self.inner.pending_cloud_creates.lock().remove(&session_id); | ||
| } | ||
|
|
||
| let Some(parts) = materialized_cloud_session.lock().take() else { | ||
| self.inner.pending_cloud_creates.lock().remove(&session_id); | ||
| return Err(Error::InvalidConfig( | ||
| "cloud session was not registered".to_string(), | ||
| )); | ||
| }; |
Comment on lines
+838
to
841
| if (isCloudCreate && config.sessionId) { | ||
| // In cloud mode this is a provisional client-side ID, not the final server ID. | ||
| } | ||
|
|
Comment on lines
+935
to
+943
| if (isCloudCreate) { | ||
| session = this.sessions.get(createResponse.sessionId); | ||
| if (!session) { | ||
| throw new Error( | ||
| `Cloud session was not registered: ${createResponse.sessionId}` | ||
| ); | ||
| } | ||
| this.pendingCloudSessionCreates.delete(sessionId); | ||
| } |
Comment on lines
+811
to
+819
| if isCloudCreate { | ||
| c.sessionsMux.Lock() | ||
| session = c.sessions[response.SessionID] | ||
| delete(c.pendingCloudCreates, sessionID) | ||
| c.sessionsMux.Unlock() | ||
| if session == nil { | ||
| return nil, fmt.Errorf("cloud session was not registered: %s", response.SessionID) | ||
| } | ||
| } |
Comment on lines
+1318
to
+1320
| if _, err := materialize(event.SessionID); err != nil { | ||
| fmt.Printf("Error materializing cloud session %s: %v\n", event.SessionID, err) | ||
| } |
Comment on lines
+624
to
+635
| if (session is null) | ||
| { | ||
| _sessions.TryGetValue(response.SessionId, out session); | ||
| if (session is null) | ||
| { | ||
| throw new InvalidOperationException($"Session was not registered: {response.SessionId}"); | ||
| } | ||
| } | ||
| if (isCloudCreate) | ||
| { | ||
| _pendingCloudCreates?.TryRemove(sessionId, out _); | ||
| } |
Comment on lines
+1899
to
+1903
| self._pending_cloud_creates.pop(actual_session_id, None) | ||
| raise RuntimeError( | ||
| f"Cloud session was not registered: {response_session_id}" | ||
| ) | ||
| self._pending_cloud_creates.pop(actual_session_id, None) |
Comment on lines
775
to
789
| result, err := c.client.Request("session.create", req) | ||
| if err != nil { | ||
| c.sessionsMux.Lock() | ||
| delete(c.sessions, sessionID) | ||
| delete(c.pendingCloudCreates, sessionID) | ||
| materializedMux.Lock() | ||
| if materializedSessionID != "" { | ||
| delete(c.sessions, materializedSessionID) | ||
| } | ||
| materializedMux.Unlock() | ||
| if session != nil { | ||
| delete(c.sessions, session.SessionID) | ||
| } else if sessionID != "" { | ||
| delete(c.sessions, sessionID) | ||
| } | ||
| c.sessionsMux.Unlock() |
Comment on lines
641
to
645
| catch (Exception ex) | ||
| { | ||
| session.RemoveFromClient(); | ||
| _pendingCloudCreates?.TryRemove(sessionId, out _); | ||
| session?.RemoveFromClient(); | ||
| if (ex is not OperationCanceledException) |
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
For cloud session creation, defer
CopilotSessionconstruction until thesession.createdlifecycle event arrives with the server-assigned session ID. This avoids registering a session under a provisional client-generated ID that will immediately be replaced by the server, and ensures the session object is keyed by its real ID from the start.Changes
All SDKs (Node, .NET, Go, Python, Rust):
configureSession/CreateConfiguredSession/ equivalent consolidates the repeated tool/hook/handler registration logic used by both create and resume paths.session.createdlifecycle event arrives with aclientSessionId, the pending factory is invoked to materialize the session under the real server-assigned ID.clientSessionIdonSessionLifecycleEventtypes across all languages.[Experimental]attribute, Python docstring annotations).Motivation
Cloud-created sessions receive their final session ID from the server, which may differ from the client-generated provisional ID. Previously, the session was eagerly constructed and registered under the provisional ID, then had to be re-keyed. This lazy approach is cleaner: the session is only constructed once it's known what ID it should have.