Skip to content

Commit 7310091

Browse files
stephentoubCopilot
stephentoub
and
Copilot
committed
Register sessions before RPC to prevent dropped events
Register sessions in the client's sessions map before issuing the session.create and session.resume RPC calls, so that events emitted by the CLI during the RPC (e.g. session.start, permission requests, tool calls) are not dropped. Previously, sessions were registered only after the RPC completed, creating a window where notifications for the session had no target. The session ID is now generated client-side (via UUID) rather than extracted from the server response. On RPC failure, the session is cleaned up from the map. Changes across all four SDKs (Node.js, Python, Go, .NET): - Generate sessionId client-side before the RPC - Create and register the session in the sessions map before the RPC - Set workspacePath from the RPC response after it completes - Remove the session from the map if the RPC fails Includes unit tests for Node.js, Python, and Go that verify: - Session is in the map when session.create RPC is called - Session is in the map when session.resume RPC is called - Session is cleaned up from map on RPC failure (create and resume) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 427205f commit 7310091

File tree

74 files changed

+873
-196
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

74 files changed

+873
-196
lines changed

‎dotnet/src/Client.cs‎

Lines changed: 80 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -381,33 +381,11 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig config, Cance
381381
config.Hooks.OnSessionEnd != null ||
382382
config.Hooks.OnErrorOccurred != null);
383383

384-
var request = new CreateSessionRequest(
385-
config.Model,
386-
config.SessionId,
387-
config.ClientName,
388-
config.ReasoningEffort,
389-
config.Tools?.Select(ToolDefinition.FromAIFunction).ToList(),
390-
config.SystemMessage,
391-
config.AvailableTools,
392-
config.ExcludedTools,
393-
config.Provider,
394-
(bool?)true,
395-
config.OnUserInputRequest != null ? true : null,
396-
hasHooks ? true : null,
397-
config.WorkingDirectory,
398-
config.Streaming is true ? true : null,
399-
config.McpServers,
400-
"direct",
401-
config.CustomAgents,
402-
config.ConfigDir,
403-
config.SkillDirectories,
404-
config.DisabledSkills,
405-
config.InfiniteSessions);
406-
407-
var response = await InvokeRpcAsync<CreateSessionResponse>(
408-
connection.Rpc, "session.create", [request], cancellationToken);
409-
410-
var session = new CopilotSession(response.SessionId, connection.Rpc, response.WorkspacePath);
384+
var sessionId = config.SessionId ?? Guid.NewGuid().ToString();
385+
386+
// Create and register the session before issuing the RPC so that
387+
// events emitted by the CLI (e.g. session.start) are not dropped.
388+
var session = new CopilotSession(sessionId, connection.Rpc);
411389
session.RegisterTools(config.Tools ?? []);
412390
session.RegisterPermissionHandler(config.OnPermissionRequest);
413391
if (config.OnUserInputRequest != null)
@@ -418,10 +396,42 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig config, Cance
418396
{
419397
session.RegisterHooks(config.Hooks);
420398
}
399+
_sessions[sessionId] = session;
421400

422-
if (!_sessions.TryAdd(response.SessionId, session))
401+
try
423402
{
424-
throw new InvalidOperationException($"Session {response.SessionId} already exists");
403+
var request = new CreateSessionRequest(
404+
config.Model,
405+
sessionId,
406+
config.ClientName,
407+
config.ReasoningEffort,
408+
config.Tools?.Select(ToolDefinition.FromAIFunction).ToList(),
409+
config.SystemMessage,
410+
config.AvailableTools,
411+
config.ExcludedTools,
412+
config.Provider,
413+
(bool?)true,
414+
config.OnUserInputRequest != null ? true : null,
415+
hasHooks ? true : null,
416+
config.WorkingDirectory,
417+
config.Streaming is true ? true : null,
418+
config.McpServers,
419+
"direct",
420+
config.CustomAgents,
421+
config.ConfigDir,
422+
config.SkillDirectories,
423+
config.DisabledSkills,
424+
config.InfiniteSessions);
425+
426+
var response = await InvokeRpcAsync<CreateSessionResponse>(
427+
connection.Rpc, "session.create", [request], cancellationToken);
428+
429+
session.WorkspacePath = response.WorkspacePath;
430+
}
431+
catch
432+
{
433+
_sessions.TryRemove(sessionId, out _);
434+
throw;
425435
}
426436

427437
return session;
@@ -472,34 +482,9 @@ public async Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSes
472482
config.Hooks.OnSessionEnd != null ||
473483
config.Hooks.OnErrorOccurred != null);
474484

475-
var request = new ResumeSessionRequest(
476-
sessionId,
477-
config.ClientName,
478-
config.Model,
479-
config.ReasoningEffort,
480-
config.Tools?.Select(ToolDefinition.FromAIFunction).ToList(),
481-
config.SystemMessage,
482-
config.AvailableTools,
483-
config.ExcludedTools,
484-
config.Provider,
485-
(bool?)true,
486-
config.OnUserInputRequest != null ? true : null,
487-
hasHooks ? true : null,
488-
config.WorkingDirectory,
489-
config.ConfigDir,
490-
config.DisableResume is true ? true : null,
491-
config.Streaming is true ? true : null,
492-
config.McpServers,
493-
"direct",
494-
config.CustomAgents,
495-
config.SkillDirectories,
496-
config.DisabledSkills,
497-
config.InfiniteSessions);
498-
499-
var response = await InvokeRpcAsync<ResumeSessionResponse>(
500-
connection.Rpc, "session.resume", [request], cancellationToken);
501-
502-
var session = new CopilotSession(response.SessionId, connection.Rpc, response.WorkspacePath);
485+
// Create and register the session before issuing the RPC so that
486+
// events emitted by the CLI (e.g. session.start) are not dropped.
487+
var session = new CopilotSession(sessionId, connection.Rpc);
503488
session.RegisterTools(config.Tools ?? []);
504489
session.RegisterPermissionHandler(config.OnPermissionRequest);
505490
if (config.OnUserInputRequest != null)
@@ -510,9 +495,45 @@ public async Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSes
510495
{
511496
session.RegisterHooks(config.Hooks);
512497
}
498+
_sessions[sessionId] = session;
499+
500+
try
501+
{
502+
var request = new ResumeSessionRequest(
503+
sessionId,
504+
config.ClientName,
505+
config.Model,
506+
config.ReasoningEffort,
507+
config.Tools?.Select(ToolDefinition.FromAIFunction).ToList(),
508+
config.SystemMessage,
509+
config.AvailableTools,
510+
config.ExcludedTools,
511+
config.Provider,
512+
(bool?)true,
513+
config.OnUserInputRequest != null ? true : null,
514+
hasHooks ? true : null,
515+
config.WorkingDirectory,
516+
config.ConfigDir,
517+
config.DisableResume is true ? true : null,
518+
config.Streaming is true ? true : null,
519+
config.McpServers,
520+
"direct",
521+
config.CustomAgents,
522+
config.SkillDirectories,
523+
config.DisabledSkills,
524+
config.InfiniteSessions);
525+
526+
var response = await InvokeRpcAsync<ResumeSessionResponse>(
527+
connection.Rpc, "session.resume", [request], cancellationToken);
528+
529+
session.WorkspacePath = response.WorkspacePath;
530+
}
531+
catch
532+
{
533+
_sessions.TryRemove(sessionId, out _);
534+
throw;
535+
}
513536

514-
// Replace any existing session entry to ensure new config (like permission handler) is used
515-
_sessions[response.SessionId] = session;
516537
return session;
517538
}
518539

‎dotnet/src/Session.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public partial class CopilotSession : IAsyncDisposable
7878
/// The path to the workspace containing checkpoints/, plan.md, and files/ subdirectories,
7979
/// or null if infinite sessions are disabled.
8080
/// </value>
81-
public string? WorkspacePath { get; }
81+
public string? WorkspacePath { get; internal set; }
8282

8383
/// <summary>
8484
/// Initializes a new instance of the <see cref="CopilotSession"/> class.

‎go/client.go‎

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import (
4444
"sync/atomic"
4545
"time"
4646

47+
"github.com/google/uuid"
48+
4749
"github.com/github/copilot-sdk/go/internal/embeddedcli"
4850
"github.com/github/copilot-sdk/go/internal/jsonrpc2"
4951
"github.com/github/copilot-sdk/go/rpc"
@@ -484,7 +486,6 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
484486

485487
req := createSessionRequest{}
486488
req.Model = config.Model
487-
req.SessionID = config.SessionID
488489
req.ClientName = config.ClientName
489490
req.ReasoningEffort = config.ReasoningEffort
490491
req.ConfigDir = config.ConfigDir
@@ -517,17 +518,15 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
517518
}
518519
req.RequestPermission = Bool(true)
519520

520-
result, err := c.client.Request("session.create", req)
521-
if err != nil {
522-
return nil, fmt.Errorf("failed to create session: %w", err)
523-
}
524-
525-
var response createSessionResponse
526-
if err := json.Unmarshal(result, &response); err != nil {
527-
return nil, fmt.Errorf("failed to unmarshal response: %w", err)
521+
sessionID := config.SessionID
522+
if sessionID == "" {
523+
sessionID = uuid.New().String()
528524
}
525+
req.SessionID = sessionID
529526

530-
session := newSession(response.SessionID, c.client, response.WorkspacePath)
527+
// Create and register the session before issuing the RPC so that
528+
// events emitted by the CLI (e.g. session.start) are not dropped.
529+
session := newSession(sessionID, c.client, "")
531530

532531
session.registerTools(config.Tools)
533532
session.registerPermissionHandler(config.OnPermissionRequest)
@@ -539,9 +538,27 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
539538
}
540539

541540
c.sessionsMux.Lock()
542-
c.sessions[response.SessionID] = session
541+
c.sessions[sessionID] = session
543542
c.sessionsMux.Unlock()
544543

544+
result, err := c.client.Request("session.create", req)
545+
if err != nil {
546+
c.sessionsMux.Lock()
547+
delete(c.sessions, sessionID)
548+
c.sessionsMux.Unlock()
549+
return nil, fmt.Errorf("failed to create session: %w", err)
550+
}
551+
552+
var response createSessionResponse
553+
if err := json.Unmarshal(result, &response); err != nil {
554+
c.sessionsMux.Lock()
555+
delete(c.sessions, sessionID)
556+
c.sessionsMux.Unlock()
557+
return nil, fmt.Errorf("failed to unmarshal response: %w", err)
558+
}
559+
560+
session.workspacePath = response.WorkspacePath
561+
545562
return session, nil
546563
}
547564

@@ -616,17 +633,10 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
616633
req.InfiniteSessions = config.InfiniteSessions
617634
req.RequestPermission = Bool(true)
618635

619-
result, err := c.client.Request("session.resume", req)
620-
if err != nil {
621-
return nil, fmt.Errorf("failed to resume session: %w", err)
622-
}
623-
624-
var response resumeSessionResponse
625-
if err := json.Unmarshal(result, &response); err != nil {
626-
return nil, fmt.Errorf("failed to unmarshal response: %w", err)
627-
}
636+
// Create and register the session before issuing the RPC so that
637+
// events emitted by the CLI (e.g. session.start) are not dropped.
638+
session := newSession(sessionID, c.client, "")
628639

629-
session := newSession(response.SessionID, c.client, response.WorkspacePath)
630640
session.registerTools(config.Tools)
631641
session.registerPermissionHandler(config.OnPermissionRequest)
632642
if config.OnUserInputRequest != nil {
@@ -637,9 +647,27 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
637647
}
638648

639649
c.sessionsMux.Lock()
640-
c.sessions[response.SessionID] = session
650+
c.sessions[sessionID] = session
641651
c.sessionsMux.Unlock()
642652

653+
result, err := c.client.Request("session.resume", req)
654+
if err != nil {
655+
c.sessionsMux.Lock()
656+
delete(c.sessions, sessionID)
657+
c.sessionsMux.Unlock()
658+
return nil, fmt.Errorf("failed to resume session: %w", err)
659+
}
660+
661+
var response resumeSessionResponse
662+
if err := json.Unmarshal(result, &response); err != nil {
663+
c.sessionsMux.Lock()
664+
delete(c.sessions, sessionID)
665+
c.sessionsMux.Unlock()
666+
return nil, fmt.Errorf("failed to unmarshal response: %w", err)
667+
}
668+
669+
session.workspacePath = response.WorkspacePath
670+
643671
return session, nil
644672
}
645673

0 commit comments

Comments
 (0)