From 5db53eb75b6491e43443040a942699e60fea684e Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 14 Jul 2026 17:51:10 +0200 Subject: [PATCH 1/4] Fix in-process stop teardown stall root cause Bound in-process per-session abort/disconnect teardown in CopilotClient.stop and skip already-disconnected sessions to avoid unbounded teardown awaits. Adds shutdown coverage for connected-session filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc --- nodejs/src/client.ts | 21 ++++++++++++++++++--- nodejs/src/session.ts | 5 +++++ nodejs/test/client.test.ts | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 65784569cc..a285e86e16 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -92,6 +92,8 @@ import { defaultJoinSessionPermissionHandler } from "./types.js"; */ const MIN_PROTOCOL_VERSION = 3; const RUNTIME_SHUTDOWN_TIMEOUT_MS = 10_000; +const SESSION_ABORT_TIMEOUT_MS = 5_000; +const SESSION_DISCONNECT_TIMEOUT_MS = 5_000; /** * Check if value is a Zod schema (has toJSONSchema method) @@ -946,6 +948,7 @@ export class CopilotClient { // Disconnect all active sessions with retry logic const activeSessions = [...this.sessions.values()]; + const connectedSessions = activeSessions.filter((session) => !session._isDisconnected()); // TEMPORARY: over the in-process (FFI) transport the runtime shares this // process, so a turn still running when the runtime disposes the session // can leave that session's SQLite session.db handle open — it isn't @@ -958,16 +961,28 @@ export class CopilotClient { // own the runtime and aborting would cancel pending work other clients // may still resume. Remove once the runtime cleans up fully on shutdown. if (this.connectionConfig.kind === "inprocess") { - await Promise.allSettled(activeSessions.map((session) => session.abort())); + await Promise.allSettled( + connectedSessions.map((session) => + withTimeout( + session.abort(), + SESSION_ABORT_TIMEOUT_MS, + `session.abort timed out after ${SESSION_ABORT_TIMEOUT_MS}ms for ${session.sessionId}` + ) + ) + ); } - for (const session of activeSessions) { + for (const session of connectedSessions) { const sessionId = session.sessionId; let lastError: Error | null = null; // Try up to 3 times with exponential backoff for (let attempt = 1; attempt <= 3; attempt++) { try { - await session.disconnect(); + await withTimeout( + session.disconnect(), + SESSION_DISCONNECT_TIMEOUT_MS, + `session.disconnect timed out after ${SESSION_DISCONNECT_TIMEOUT_MS}ms` + ); lastError = null; break; // Success } catch (error) { diff --git a/nodejs/src/session.ts b/nodejs/src/session.ts index 1f71209de8..45e1d4956f 100644 --- a/nodejs/src/session.ts +++ b/nodejs/src/session.ts @@ -361,6 +361,11 @@ export class CopilotSession { this.transformCallbacks?.clear(); } + /** @internal */ + _isDisconnected(): boolean { + return this.disconnected; + } + /** * Subscribes to events from this session. * diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 07be2b95e3..b27c281b9d 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -3193,5 +3193,40 @@ describe("CopilotClient", () => { await expect(externalClient.stop()).resolves.toEqual([]); expect(externalSendRequest).not.toHaveBeenCalled(); }); + + it("only aborts and disconnects sessions that are still connected", async () => { + const client = new CopilotClient({ + connection: RuntimeConnection.forInProcess(), + }); + + const connectedSession = { + sessionId: "connected-session", + abort: vi.fn(async () => {}), + disconnect: vi.fn(async () => {}), + _markDisconnected: vi.fn(), + _isDisconnected: vi.fn(() => false), + }; + const disconnectedSession = { + sessionId: "disconnected-session", + abort: vi.fn(async () => {}), + disconnect: vi.fn(async () => {}), + _markDisconnected: vi.fn(), + _isDisconnected: vi.fn(() => true), + }; + + (client as any).sessions = new Map([ + ["connected-session", connectedSession], + ["disconnected-session", disconnectedSession], + ]); + + await expect(client.stop()).resolves.toEqual([]); + + expect(connectedSession.abort).toHaveBeenCalledTimes(1); + expect(connectedSession.disconnect).toHaveBeenCalledTimes(1); + expect(disconnectedSession.abort).not.toHaveBeenCalled(); + expect(disconnectedSession.disconnect).not.toHaveBeenCalled(); + expect(connectedSession._markDisconnected).toHaveBeenCalledTimes(1); + expect(disconnectedSession._markDisconnected).toHaveBeenCalledTimes(1); + }); }); }); From 92e2286abece609853fe64007a8116ff41d3b1f7 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 14 Jul 2026 18:00:52 +0200 Subject: [PATCH 2/4] Scope teardown bounds to in-process sessions Keep existing non-in-process disconnect behavior while adding fake-timer coverage for stalled in-process abort and disconnect cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc --- nodejs/src/client.ts | 16 +++++++++------ nodejs/test/client.test.ts | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index a285e86e16..150b287a03 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -960,7 +960,8 @@ export class CopilotClient { // on shutdown (which frees the handle), and for external servers we don't // own the runtime and aborting would cancel pending work other clients // may still resume. Remove once the runtime cleans up fully on shutdown. - if (this.connectionConfig.kind === "inprocess") { + const isInProcess = this.connectionConfig.kind === "inprocess"; + if (isInProcess) { await Promise.allSettled( connectedSessions.map((session) => withTimeout( @@ -978,11 +979,14 @@ export class CopilotClient { // Try up to 3 times with exponential backoff for (let attempt = 1; attempt <= 3; attempt++) { try { - await withTimeout( - session.disconnect(), - SESSION_DISCONNECT_TIMEOUT_MS, - `session.disconnect timed out after ${SESSION_DISCONNECT_TIMEOUT_MS}ms` - ); + const disconnectPromise = session.disconnect(); + await (isInProcess + ? withTimeout( + disconnectPromise, + SESSION_DISCONNECT_TIMEOUT_MS, + `session.disconnect timed out after ${SESSION_DISCONNECT_TIMEOUT_MS}ms` + ) + : disconnectPromise); lastError = null; break; // Success } catch (error) { diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index b27c281b9d..680c3d9deb 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -3228,5 +3228,47 @@ describe("CopilotClient", () => { expect(connectedSession._markDisconnected).toHaveBeenCalledTimes(1); expect(disconnectedSession._markDisconnected).toHaveBeenCalledTimes(1); }); + + it("bounds stalled in-process session teardown", async () => { + vi.useFakeTimers(); + try { + const client = new CopilotClient({ + connection: RuntimeConnection.forInProcess(), + }); + const never = () => new Promise(() => {}); + const session = { + sessionId: "stalled-session", + abort: vi.fn(never), + disconnect: vi.fn(never), + _markDisconnected: vi.fn(), + _isDisconnected: vi.fn(() => false), + }; + (client as any).sessions = new Map([["stalled-session", session]]); + + const stopPromise = client.stop(); + + await vi.advanceTimersByTimeAsync(5_000); + expect(session.abort).toHaveBeenCalledTimes(1); + expect(session.disconnect).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(5_100); + expect(session.disconnect).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(5_200); + expect(session.disconnect).toHaveBeenCalledTimes(3); + + await vi.advanceTimersByTimeAsync(5_000); + await expect(stopPromise).resolves.toEqual([ + expect.objectContaining({ + message: + "Failed to disconnect session stalled-session after 3 attempts: " + + "session.disconnect timed out after 5000ms", + }), + ]); + expect(session._markDisconnected).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); }); }); From d8ccf558df76577b251d19091ffddca4f2aca7c9 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 14 Jul 2026 18:16:27 +0200 Subject: [PATCH 3/4] Fail E2E on in-process cleanup timeouts Report abort timeout failures from client teardown and make the shared E2E harness fail after completing proxy and directory cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc --- nodejs/src/client.ts | 15 ++++++++++++++- nodejs/test/client.test.ts | 21 ++++++++++++++------- nodejs/test/e2e/harness/sdkTestContext.ts | 15 +++++++++++++-- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 150b287a03..3cb12cd113 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -962,7 +962,7 @@ export class CopilotClient { // may still resume. Remove once the runtime cleans up fully on shutdown. const isInProcess = this.connectionConfig.kind === "inprocess"; if (isInProcess) { - await Promise.allSettled( + const abortResults = await Promise.allSettled( connectedSessions.map((session) => withTimeout( session.abort(), @@ -971,6 +971,19 @@ export class CopilotClient { ) ) ); + for (const [index, result] of abortResults.entries()) { + if (result.status === "rejected") { + const error = + result.reason instanceof Error + ? result.reason + : new Error(String(result.reason)); + errors.push( + new Error( + `Failed to abort session ${connectedSessions[index].sessionId} during in-process teardown: ${error.message}` + ) + ); + } + } } for (const session of connectedSessions) { const sessionId = session.sessionId; diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 680c3d9deb..4921660982 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -3258,13 +3258,20 @@ describe("CopilotClient", () => { expect(session.disconnect).toHaveBeenCalledTimes(3); await vi.advanceTimersByTimeAsync(5_000); - await expect(stopPromise).resolves.toEqual([ - expect.objectContaining({ - message: - "Failed to disconnect session stalled-session after 3 attempts: " + - "session.disconnect timed out after 5000ms", - }), - ]); + await expect(stopPromise).resolves.toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: + "Failed to abort session stalled-session during in-process teardown: " + + "session.abort timed out after 5000ms for stalled-session", + }), + expect.objectContaining({ + message: + "Failed to disconnect session stalled-session after 3 attempts: " + + "session.disconnect timed out after 5000ms", + }), + ]) + ); expect(session._markDisconnected).toHaveBeenCalledTimes(1); } finally { vi.useRealTimers(); diff --git a/nodejs/test/e2e/harness/sdkTestContext.ts b/nodejs/test/e2e/harness/sdkTestContext.ts index 624450e47c..922dbd7e01 100644 --- a/nodejs/test/e2e/harness/sdkTestContext.ts +++ b/nodejs/test/e2e/harness/sdkTestContext.ts @@ -291,11 +291,22 @@ export async function createSdkTestContext({ }); afterAll(async () => { - await copilotClient.stop(); - await openAiEndpoint.stop(anyTestFailed); + const stopErrors = await copilotClient.stop(); + let proxyStopError: unknown; + try { + await openAiEndpoint.stop(anyTestFailed); + } catch (error) { + proxyStopError = error; + } await rmDir("remove e2e test copilotHomeDir", copilotHomeDir); await rmDir("remove e2e test homeDir", homeDir); await rmDir("remove e2e test workDir", workDir); + if (stopErrors.length > 0) { + throw new AggregateError(stopErrors, "Copilot client cleanup failed"); + } + if (proxyStopError) { + throw proxyStopError; + } }); return harness; From a5a07553fbe9410533a819f22702e545a649b2ae Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 14 Jul 2026 18:21:38 +0200 Subject: [PATCH 4/4] Keep in-process teardown fix test-only Revert product cleanup behavior changes and remove stale disconnected sessions only from the Node E2E harness before teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc --- nodejs/src/client.ts | 40 ++--------- nodejs/src/session.ts | 5 -- nodejs/test/client.test.ts | 84 ----------------------- nodejs/test/e2e/harness/sdkTestContext.ts | 17 +++++ 4 files changed, 21 insertions(+), 125 deletions(-) diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 3cb12cd113..65784569cc 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -92,8 +92,6 @@ import { defaultJoinSessionPermissionHandler } from "./types.js"; */ const MIN_PROTOCOL_VERSION = 3; const RUNTIME_SHUTDOWN_TIMEOUT_MS = 10_000; -const SESSION_ABORT_TIMEOUT_MS = 5_000; -const SESSION_DISCONNECT_TIMEOUT_MS = 5_000; /** * Check if value is a Zod schema (has toJSONSchema method) @@ -948,7 +946,6 @@ export class CopilotClient { // Disconnect all active sessions with retry logic const activeSessions = [...this.sessions.values()]; - const connectedSessions = activeSessions.filter((session) => !session._isDisconnected()); // TEMPORARY: over the in-process (FFI) transport the runtime shares this // process, so a turn still running when the runtime disposes the session // can leave that session's SQLite session.db handle open — it isn't @@ -960,46 +957,17 @@ export class CopilotClient { // on shutdown (which frees the handle), and for external servers we don't // own the runtime and aborting would cancel pending work other clients // may still resume. Remove once the runtime cleans up fully on shutdown. - const isInProcess = this.connectionConfig.kind === "inprocess"; - if (isInProcess) { - const abortResults = await Promise.allSettled( - connectedSessions.map((session) => - withTimeout( - session.abort(), - SESSION_ABORT_TIMEOUT_MS, - `session.abort timed out after ${SESSION_ABORT_TIMEOUT_MS}ms for ${session.sessionId}` - ) - ) - ); - for (const [index, result] of abortResults.entries()) { - if (result.status === "rejected") { - const error = - result.reason instanceof Error - ? result.reason - : new Error(String(result.reason)); - errors.push( - new Error( - `Failed to abort session ${connectedSessions[index].sessionId} during in-process teardown: ${error.message}` - ) - ); - } - } + if (this.connectionConfig.kind === "inprocess") { + await Promise.allSettled(activeSessions.map((session) => session.abort())); } - for (const session of connectedSessions) { + for (const session of activeSessions) { const sessionId = session.sessionId; let lastError: Error | null = null; // Try up to 3 times with exponential backoff for (let attempt = 1; attempt <= 3; attempt++) { try { - const disconnectPromise = session.disconnect(); - await (isInProcess - ? withTimeout( - disconnectPromise, - SESSION_DISCONNECT_TIMEOUT_MS, - `session.disconnect timed out after ${SESSION_DISCONNECT_TIMEOUT_MS}ms` - ) - : disconnectPromise); + await session.disconnect(); lastError = null; break; // Success } catch (error) { diff --git a/nodejs/src/session.ts b/nodejs/src/session.ts index 45e1d4956f..1f71209de8 100644 --- a/nodejs/src/session.ts +++ b/nodejs/src/session.ts @@ -361,11 +361,6 @@ export class CopilotSession { this.transformCallbacks?.clear(); } - /** @internal */ - _isDisconnected(): boolean { - return this.disconnected; - } - /** * Subscribes to events from this session. * diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 4921660982..07be2b95e3 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -3193,89 +3193,5 @@ describe("CopilotClient", () => { await expect(externalClient.stop()).resolves.toEqual([]); expect(externalSendRequest).not.toHaveBeenCalled(); }); - - it("only aborts and disconnects sessions that are still connected", async () => { - const client = new CopilotClient({ - connection: RuntimeConnection.forInProcess(), - }); - - const connectedSession = { - sessionId: "connected-session", - abort: vi.fn(async () => {}), - disconnect: vi.fn(async () => {}), - _markDisconnected: vi.fn(), - _isDisconnected: vi.fn(() => false), - }; - const disconnectedSession = { - sessionId: "disconnected-session", - abort: vi.fn(async () => {}), - disconnect: vi.fn(async () => {}), - _markDisconnected: vi.fn(), - _isDisconnected: vi.fn(() => true), - }; - - (client as any).sessions = new Map([ - ["connected-session", connectedSession], - ["disconnected-session", disconnectedSession], - ]); - - await expect(client.stop()).resolves.toEqual([]); - - expect(connectedSession.abort).toHaveBeenCalledTimes(1); - expect(connectedSession.disconnect).toHaveBeenCalledTimes(1); - expect(disconnectedSession.abort).not.toHaveBeenCalled(); - expect(disconnectedSession.disconnect).not.toHaveBeenCalled(); - expect(connectedSession._markDisconnected).toHaveBeenCalledTimes(1); - expect(disconnectedSession._markDisconnected).toHaveBeenCalledTimes(1); - }); - - it("bounds stalled in-process session teardown", async () => { - vi.useFakeTimers(); - try { - const client = new CopilotClient({ - connection: RuntimeConnection.forInProcess(), - }); - const never = () => new Promise(() => {}); - const session = { - sessionId: "stalled-session", - abort: vi.fn(never), - disconnect: vi.fn(never), - _markDisconnected: vi.fn(), - _isDisconnected: vi.fn(() => false), - }; - (client as any).sessions = new Map([["stalled-session", session]]); - - const stopPromise = client.stop(); - - await vi.advanceTimersByTimeAsync(5_000); - expect(session.abort).toHaveBeenCalledTimes(1); - expect(session.disconnect).toHaveBeenCalledTimes(1); - - await vi.advanceTimersByTimeAsync(5_100); - expect(session.disconnect).toHaveBeenCalledTimes(2); - - await vi.advanceTimersByTimeAsync(5_200); - expect(session.disconnect).toHaveBeenCalledTimes(3); - - await vi.advanceTimersByTimeAsync(5_000); - await expect(stopPromise).resolves.toEqual( - expect.arrayContaining([ - expect.objectContaining({ - message: - "Failed to abort session stalled-session during in-process teardown: " + - "session.abort timed out after 5000ms for stalled-session", - }), - expect.objectContaining({ - message: - "Failed to disconnect session stalled-session after 3 attempts: " + - "session.disconnect timed out after 5000ms", - }), - ]) - ); - expect(session._markDisconnected).toHaveBeenCalledTimes(1); - } finally { - vi.useRealTimers(); - } - }); }); }); diff --git a/nodejs/test/e2e/harness/sdkTestContext.ts b/nodejs/test/e2e/harness/sdkTestContext.ts index 922dbd7e01..3e4c3a0560 100644 --- a/nodejs/test/e2e/harness/sdkTestContext.ts +++ b/nodejs/test/e2e/harness/sdkTestContext.ts @@ -291,6 +291,7 @@ export async function createSdkTestContext({ }); afterAll(async () => { + removeDisconnectedSessionEntries(copilotClient); const stopErrors = await copilotClient.stop(); let proxyStopError: unknown; try { @@ -312,6 +313,22 @@ export async function createSdkTestContext({ return harness; } +function removeDisconnectedSessionEntries(client: CopilotClient): void { + // `CopilotSession.disconnect()` leaves its object in the client's private registry. + // The in-process client stop path aborts every registry entry, including these + // already-destroyed sessions. Remove only stale test entries before teardown. + const sessions = ( + client as unknown as { + sessions: Map; + } + ).sessions; + for (const [sessionId, session] of sessions) { + if (session.disconnected) { + sessions.delete(sessionId); + } + } +} + function getTrafficCapturePath(testContext: TestContext): string { const testFilePath = testContext.task.file.filepath; const suffix = ".test.ts";