diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index eb92cf0bed..a25a74121a 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -448,6 +448,8 @@ export class CopilotClient { private runtimePort: number | null = null; private actualHost: string = "localhost"; private state: "disconnected" | "connecting" | "connected" | "error" = "disconnected"; + /** Shared in-flight start; concurrent callers await it instead of spawning another CLI. */ + private startPromise: Promise | null = null; private sessions: Map = new Map(); private stderrBuffer: string = ""; // Captures CLI stderr for error messages /** Resolved connection mode chosen in the constructor. */ @@ -935,6 +937,20 @@ export class CopilotClient { return; } + // Concurrent callers share one in-progress start instead of each spawning a CLI. + if (this.startPromise) { + return this.startPromise; + } + + this.startPromise = this.doStart(); + try { + await this.startPromise; + } finally { + this.startPromise = null; + } + } + + private async doStart(): Promise { this.forceStopping = false; this.connectionClosed = false; this.processTransportError = null; diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 3db96ea47b..119ed40a10 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -62,6 +62,64 @@ describe("approveAll", () => { }); describe("CopilotClient", () => { + it("start() is single-flight: concurrent callers share one startup", async () => { + const client = new CopilotClient({ autoStart: false }); + onTestFinished(() => client.forceStop()); + + // Stub the underlying startup (doStart) that the single-flight guard + // dedupes. Transport-independent: this is the same regardless of the + // stdio vs in-process connection path. The delay makes all three + // start() calls overlap; on success it marks the client connected like + // the real doStart does. + const doStart = vi.fn().mockImplementation( + () => + new Promise((resolve) => + setTimeout(() => { + (client as any).state = "connected"; + resolve(); + }, 50) + ) + ); + (client as any).doStart = doStart; + + // Before the fix, each concurrent caller ran startup (and spawned its own + // CLI, orphaning all but the last). With single-flight they share one. + await Promise.all([client.start(), client.start(), client.start()]); + + expect(doStart).toHaveBeenCalledTimes(1); + expect((client as any).state).toBe("connected"); + + // Once connected, a further start() is a no-op (no extra startup). + await client.start(); + expect(doStart).toHaveBeenCalledTimes(1); + }); + + it("start() retries after a failed attempt (single-flight guard is cleared)", async () => { + const client = new CopilotClient({ autoStart: false }); + onTestFinished(() => client.forceStop()); + + // Stub the underlying startup: fail once, then succeed. Transport- + // independent (does not depend on the stdio vs in-process path). + const doStart = vi + .fn() + .mockImplementationOnce(async () => { + (client as any).state = "error"; + throw new Error("boom"); + }) + .mockImplementationOnce(async () => { + (client as any).state = "connected"; + }); + (client as any).doStart = doStart; + + await expect(client.start()).rejects.toThrow(/boom/); + expect((client as any).state).toBe("error"); + + // The guard must have cleared so a later start() can retry. + await client.start(); + expect(doStart).toHaveBeenCalledTimes(2); + expect((client as any).state).toBe("connected"); + }); + it.each([ { source: "connection path", diff --git a/nodejs/test/e2e/session.e2e.test.ts b/nodejs/test/e2e/session.e2e.test.ts index 4c20acb345..77a1dec408 100644 --- a/nodejs/test/e2e/session.e2e.test.ts +++ b/nodejs/test/e2e/session.e2e.test.ts @@ -103,6 +103,10 @@ describe("Sessions", () => { workingDirectory: workDir, env, connection: RuntimeConnection.forStdio({ path: process.env.COPILOT_CLI_PATH }), + // Explicit token (matches createClient()/other passing resume tests): without it, + // useLoggedInUser defaults to true and the runtime falls back to ambient env-var + // auto-detection for the model call, which flakes on some hosts (e.g. Alpine ARM64). + gitHubToken: isCI ? DEFAULT_GITHUB_TOKEN : undefined, }); onTestFinished(async () => { try { @@ -127,6 +131,7 @@ describe("Sessions", () => { workingDirectory: workDir, env, connection: RuntimeConnection.forStdio({ path: process.env.COPILOT_CLI_PATH }), + gitHubToken: isCI ? DEFAULT_GITHUB_TOKEN : undefined, }); onTestFinished(async () => { try { diff --git a/python/copilot/client.py b/python/copilot/client.py index dd531a2be9..30f754b8e3 100644 --- a/python/copilot/client.py +++ b/python/copilot/client.py @@ -1753,6 +1753,7 @@ def __init__( self._cli_process: subprocess.Popen | None = None self._client: JsonRpcClient | None = None self._state: _ConnectionState = "disconnected" + self._start_lock = asyncio.Lock() self._sessions: dict[str, CopilotSession] = {} self._sessions_lock = threading.Lock() self._github_token_providers: dict[str, _GitHubTokenProviderRegistration] = {} @@ -1938,6 +1939,13 @@ async def start(self) -> None: >>> await client.start() >>> # Now ready to create sessions """ + # Concurrent session creation can auto-start the same client. Keep the + # state check and all transport initialization under one lock so only + # one caller can spawn a runtime and install its connection at a time. + async with self._start_lock: + await self._start() + + async def _start(self) -> None: if self._state == "connected": return diff --git a/python/test_client_start.py b/python/test_client_start.py new file mode 100644 index 0000000000..626f0f6d96 --- /dev/null +++ b/python/test_client_start.py @@ -0,0 +1,85 @@ +"""Startup concurrency regressions without a live CLI runtime.""" + +import asyncio +from unittest.mock import AsyncMock + +import pytest + +from copilot import CopilotClient, RuntimeConnection + + +@pytest.fixture +def client(): + client = CopilotClient(connection=RuntimeConnection.for_stdio(path="unused-cli")) + client._start_cli_server = AsyncMock() + client._connect_to_server = AsyncMock() + client._verify_protocol_version = AsyncMock() + return client + + +@pytest.mark.parametrize( + "phase", ["_start_cli_server", "_connect_to_server", "_verify_protocol_version"] +) +async def test_concurrent_start_initializes_transport_once(client, phase): + entered = asyncio.Event() + release = asyncio.Event() + + async def block(): + entered.set() + await release.wait() + + getattr(client, phase).side_effect = block + first = asyncio.create_task(client.start()) + await asyncio.wait_for(entered.wait(), timeout=1) + second = asyncio.create_task(client.start()) + try: + await asyncio.sleep(0) + assert not second.done() + finally: + release.set() + await asyncio.wait_for(asyncio.gather(first, second), timeout=1) + + client._start_cli_server.assert_awaited_once() + client._connect_to_server.assert_awaited_once() + client._verify_protocol_version.assert_awaited_once() + await client.start() + client._start_cli_server.assert_awaited_once() + + +async def test_start_can_retry_after_failure(client): + client._start_cli_server.side_effect = [RuntimeError("startup failed"), None] + + with pytest.raises(RuntimeError, match="startup failed"): + await client.start() + await asyncio.wait_for(client.start(), timeout=1) + + assert client._start_cli_server.await_count == 2 + client._connect_to_server.assert_awaited_once() + client._verify_protocol_version.assert_awaited_once() + + +async def test_cancelling_waiting_start_does_not_cancel_active_start(client): + entered = asyncio.Event() + release = asyncio.Event() + + async def block(): + entered.set() + await release.wait() + + client._start_cli_server.side_effect = block + first = asyncio.create_task(client.start()) + await asyncio.wait_for(entered.wait(), timeout=1) + second = asyncio.create_task(client.start()) + try: + await asyncio.sleep(0) + second.cancel() + with pytest.raises(asyncio.CancelledError): + _ = await second + assert not first.done() + finally: + release.set() + await asyncio.wait_for(first, timeout=1) + + await client.start() + client._start_cli_server.assert_awaited_once() + client._verify_protocol_version.assert_awaited_once()