-
Notifications
You must be signed in to change notification settings - Fork 819
Reduce shutdown latency by parallelizing session destruction #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## 2025-05-15 - [Sequential session destruction in SDKs] | ||
| **Learning:** All Copilot SDKs (Node.js, Python, Go, .NET) were initially implementing session destruction sequentially during client shutdown. This leads to a linear increase in shutdown time as the number of active sessions grows, especially when individual destructions involve retries and backoff. | ||
| **Action:** Parallelize session cleanup using language-specific concurrency primitives (e.g., `Promise.all` in Node.js, `asyncio.gather` in Python, `Task.WhenAll` in .NET, or WaitGroups/Channels in Go) to ensure shutdown time remains constant and minimal. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,8 +298,8 @@ export class CopilotClient { | |
| async stop(): Promise<Error[]> { | ||
| const errors: Error[] = []; | ||
|
|
||
| // Destroy all active sessions with retry logic | ||
| for (const session of this.sessions.values()) { | ||
| // Destroy all active sessions in parallel with retry logic | ||
| const sessionPromises = Array.from(this.sessions.values()).map(async (session) => { | ||
| const sessionId = session.sessionId; | ||
| let lastError: Error | null = null; | ||
|
|
||
|
Comment on lines
+301
to
305
|
||
|
|
@@ -321,12 +321,18 @@ export class CopilotClient { | |
| } | ||
|
|
||
| if (lastError) { | ||
| errors.push( | ||
| new Error( | ||
| `Failed to destroy session ${sessionId} after 3 attempts: ${lastError.message}` | ||
| ) | ||
| return new Error( | ||
| `Failed to destroy session ${sessionId} after 3 attempts: ${lastError.message}` | ||
| ); | ||
| } | ||
| return null; | ||
| }); | ||
|
|
||
| const results = await Promise.all(sessionPromises); | ||
| for (const result of results) { | ||
| if (result instanceof Error) { | ||
| errors.push(result); | ||
| } | ||
| } | ||
| this.sessions.clear(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
Array.from(this.sessions.values())snapshots the sessions at the start ofstop(). If a session is created/resumed whilestop()is running, it won’t be destroyed but will still be removed bythis.sessions.clear(), which can skipsession.destroy()cleanup (and differs from the previous liveMapiteration behavior). Consider blocking session creation while stopping (e.g., astoppingflag/state checked increateSession/resumeSession) or draining the map as you schedule destructions so sessions added during shutdown are either rejected or deterministically cleaned up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback