diff --git a/dotnet/test/E2E/RpcAdditionalEdgeCasesE2ETests.cs b/dotnet/test/E2E/RpcAdditionalEdgeCasesE2ETests.cs index 241a978a99..5157eb6336 100644 --- a/dotnet/test/E2E/RpcAdditionalEdgeCasesE2ETests.cs +++ b/dotnet/test/E2E/RpcAdditionalEdgeCasesE2ETests.cs @@ -26,7 +26,7 @@ public async Task Shell_Exec_With_Zero_Timeout_Does_Not_Kill_Long_Running_Comman var session = await CreateSessionAsync(); var markerPath = Path.Join(Ctx.WorkDir, $"shell-zero-timeout-{Guid.NewGuid():N}.txt"); var command = OperatingSystem.IsWindows() - ? $"powershell -NoLogo -NoProfile -Command \"Start-Sleep -Milliseconds 500; Set-Content -LiteralPath '{markerPath}' -Value 'alive'; Start-Sleep -Seconds 60\"" + ? $"ping 127.0.0.1 -n 2 >nul & echo alive>\"{markerPath}\" & ping 127.0.0.1 -n 61 >nul" : $"sh -c \"sleep 0.5; printf alive > '{markerPath}'; sleep 60\""; var execResult = await session.Rpc.Shell.ExecAsync(command, cwd: Path.GetTempPath(), timeout: TimeSpan.Zero); diff --git a/go/internal/e2e/testharness/context.go b/go/internal/e2e/testharness/context.go index 951adfc291..060997d264 100644 --- a/go/internal/e2e/testharness/context.go +++ b/go/internal/e2e/testharness/context.go @@ -1,6 +1,7 @@ package testharness import ( + "fmt" "os" "os/exec" "path/filepath" @@ -167,7 +168,9 @@ func NewTestContext(t *testing.T) *TestContext { // use a snapshot replace this empty configuration before model traffic begins. dummySnapshotPath := filepath.Join(workDir, "__no_snapshot__.yaml") if err := proxy.Configure(dummySnapshotPath, workDir); err != nil { - proxy.StopWithOptions(true) + if stopErr := proxy.StopWithOptions(true); stopErr != nil { + t.Logf("Failed to stop proxy after initialization error: %v", stopErr) + } os.RemoveAll(homeDir) os.RemoveAll(workDir) t.Fatalf("Failed to initialize proxy: %v", err) @@ -181,7 +184,9 @@ func NewTestContext(t *testing.T) *TestContext { }, "analytics_tracking_id": "e2e-test-tracking-id", }); err != nil { - proxy.StopWithOptions(true) + if stopErr := proxy.StopWithOptions(true); stopErr != nil { + t.Logf("Failed to stop proxy after configuration error: %v", stopErr) + } os.RemoveAll(homeDir) os.RemoveAll(workDir) t.Fatalf("Failed to configure default Copilot user: %v", err) @@ -264,7 +269,9 @@ func (c *TestContext) ConfigureWithoutSnapshot(t *testing.T) { func (c *TestContext) Close(testFailed bool) { c.restoreInProcessEnvironment() if c.proxy != nil { - c.proxy.StopWithOptions(testFailed) + if err := c.proxy.StopWithOptions(testFailed); err != nil { + fmt.Fprintf(os.Stderr, "Failed to stop E2E proxy: %v\n", err) + } } if c.HomeDir != "" { os.RemoveAll(c.HomeDir) diff --git a/go/internal/e2e/testharness/proxy.go b/go/internal/e2e/testharness/proxy.go index 2545882bce..990c066eea 100644 --- a/go/internal/e2e/testharness/proxy.go +++ b/go/internal/e2e/testharness/proxy.go @@ -4,16 +4,22 @@ import ( "bufio" "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" "os" "os/exec" "regexp" + "runtime" + "strconv" "strings" "sync" + "time" ) +const proxyShutdownTimeout = 5 * time.Second + // CapiProxy manages a child process that acts as a replaying proxy to AI endpoints. // It spawns the shared test harness server from test/harness/server.ts. type CapiProxy struct { @@ -118,6 +124,11 @@ func (p *CapiProxy) StopWithOptions(skipWritingCache bool) error { if p.cmd == nil || p.cmd.Process == nil { return nil } + cmd := p.cmd + defer func() { + p.cmd = nil + p.proxyURL = "" + }() // Send stop request to the server if p.proxyURL != "" { @@ -126,20 +137,61 @@ func (p *CapiProxy) StopWithOptions(skipWritingCache bool) error { stopURL += "?skipWritingCache=true" } // Best effort - ignore errors - resp, err := http.Post(stopURL, "application/json", nil) + client := http.Client{Timeout: proxyShutdownTimeout} + resp, err := client.Post(stopURL, "application/json", nil) if err == nil { resp.Body.Close() } } - // Wait for process to exit - p.cmd.Wait() - p.cmd = nil - p.proxyURL = "" + exited := make(chan struct{}, 1) + go func() { + _ = cmd.Wait() + exited <- struct{}{} + }() + if !waitForProcessExit(exited, proxyShutdownTimeout) { + if err := killProcessTree(cmd); err != nil { + return fmt.Errorf("failed to kill proxy process: %w", err) + } + if !waitForProcessExit(exited, proxyShutdownTimeout) { + return fmt.Errorf("proxy process did not exit after being killed") + } + } + return nil +} +func killProcessTree(cmd *exec.Cmd) error { + if runtime.GOOS == "windows" { + taskkill := exec.Command( + "taskkill", + "/PID", + strconv.Itoa(cmd.Process.Pid), + "/T", + "/F", + ) + if err := taskkill.Run(); err == nil { + return nil + } + } + + if err := cmd.Process.Kill(); err != nil && !errors.Is(err, os.ErrProcessDone) { + return err + } return nil } +func waitForProcessExit(exited <-chan struct{}, timeout time.Duration) bool { + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case <-exited: + return true + case <-timer.C: + return false + } +} + // Configure sends configuration to the proxy. func (p *CapiProxy) Configure(filePath, workDir string) error { p.mu.Lock() diff --git a/nodejs/test/e2e/extension_env_access.e2e.test.ts b/nodejs/test/e2e/extension_env_access.e2e.test.ts index 1aa4875dd4..f0f84a35ed 100644 --- a/nodejs/test/e2e/extension_env_access.e2e.test.ts +++ b/nodejs/test/e2e/extension_env_access.e2e.test.ts @@ -17,7 +17,7 @@ import { import { approveAll, RuntimeConnection } from "../../src/index.js"; import { getSdkProtocolVersion } from "../../src/sdkProtocolVersion.js"; import { createSdkTestContext, getLegacyCliPathForTests } from "./harness/sdkTestContext.js"; -import { retry } from "./harness/sdkTestHelper.js"; +import { retry, stopChildProcess } from "./harness/sdkTestHelper.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); const FIXTURE = join(__dirname, "fixtures", "env-access-extension.mjs"); @@ -120,16 +120,12 @@ async function runExtensionAgainstStubHost(options: { }; } finally { connection.dispose(); - child.kill(); - // Windows keeps the directory locked until the child is gone. - await new Promise((resolveExit) => { - if (child.exitCode !== null || child.signalCode !== null) { - resolveExit(); - return; - } - child.once("exit", () => resolveExit()); - }); - await rm(dir, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 }); + try { + await stopChildProcess(child); + } finally { + // Windows keeps the directory locked until the child is gone. + await rm(dir, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 }); + } } } diff --git a/nodejs/test/e2e/harness/sdkTestHelper.ts b/nodejs/test/e2e/harness/sdkTestHelper.ts index de230b1338..c30aa9ea6f 100644 --- a/nodejs/test/e2e/harness/sdkTestHelper.ts +++ b/nodejs/test/e2e/harness/sdkTestHelper.ts @@ -2,8 +2,57 @@ * Copyright (c) Microsoft Corporation. All rights reserved. *--------------------------------------------------------------------------------------------*/ +import type { ChildProcess } from "node:child_process"; import { AssistantMessageEvent, CopilotSession, SessionEvent } from "../../../src"; +const CHILD_SHUTDOWN_TIMEOUT_MS = 1_000; + +export async function stopChildProcess(child: ChildProcess): Promise { + if (hasChildExited(child)) { + return; + } + + child.kill("SIGTERM"); + if (await waitForChildExit(child, CHILD_SHUTDOWN_TIMEOUT_MS)) { + return; + } + + child.kill("SIGKILL"); + if (!(await waitForChildExit(child, CHILD_SHUTDOWN_TIMEOUT_MS))) { + throw new Error("Child process did not exit after SIGKILL"); + } +} + +function hasChildExited(child: ChildProcess): boolean { + return child.exitCode !== null || child.signalCode !== null; +} + +function waitForChildExit(child: ChildProcess, timeoutMs: number): Promise { + if (hasChildExited(child)) { + return Promise.resolve(true); + } + + return new Promise((resolvePromise) => { + let settled = false; + const finish = (exited: boolean) => { + if (settled) { + return; + } + settled = true; + clearTimeout(timeout); + child.off("exit", onExit); + resolvePromise(exited); + }; + const onExit = () => finish(true); + const timeout = setTimeout(() => finish(false), timeoutMs); + + child.once("exit", onExit); + if (hasChildExited(child)) { + onExit(); + } + }); +} + export async function getFinalAssistantMessage( session: CopilotSession, { alreadyIdle = false }: { alreadyIdle?: boolean } = {} diff --git a/nodejs/test/e2e/mcp_oauth.e2e.test.ts b/nodejs/test/e2e/mcp_oauth.e2e.test.ts index 0909afd8ad..c9f1cfb1e7 100644 --- a/nodejs/test/e2e/mcp_oauth.e2e.test.ts +++ b/nodejs/test/e2e/mcp_oauth.e2e.test.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. *--------------------------------------------------------------------------------------------*/ -import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; +import { spawn } from "node:child_process"; import { dirname, resolve } from "node:path"; import { createInterface } from "node:readline"; import { fileURLToPath } from "node:url"; @@ -10,7 +10,7 @@ import { describe, expect, it, onTestFinished } from "vitest"; import type { CopilotSession, MCPServerConfig, McpAuthRequest } from "../../src/index.js"; import { approveAll } from "../../src/index.js"; import { createSdkTestContext } from "./harness/sdkTestContext.js"; -import { waitForCondition } from "./harness/sdkTestHelper.js"; +import { stopChildProcess, waitForCondition } from "./harness/sdkTestHelper.js"; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); @@ -322,7 +322,7 @@ async function startOAuthMcpServer(): Promise<{ env: { ...process.env, EXPECTED_TOKEN }, stdio: ["ignore", "pipe", "pipe"], }); - onTestFinished(() => stopChild(child)); + onTestFinished(() => stopChildProcess(child)); const stderr: string[] = []; child.stderr.on("data", (chunk) => stderr.push(String(chunk))); @@ -375,17 +375,6 @@ async function disconnectSession(session: CopilotSession): Promise { } } -function stopChild(child: ChildProcessWithoutNullStreams): Promise { - if (child.exitCode !== null || child.killed) { - return Promise.resolve(); - } - const exitPromise = new Promise((resolvePromise) => { - child.once("exit", () => resolvePromise()); - }); - child.kill("SIGTERM"); - return exitPromise; -} - function createAsyncQueue(): { push(value: T): void; next(): Promise } { const values: T[] = []; const waiters: Array<(value: T) => void> = []; diff --git a/python/e2e/testharness/proxy.py b/python/e2e/testharness/proxy.py index 58584b831c..9d8fc73e63 100644 --- a/python/e2e/testharness/proxy.py +++ b/python/e2e/testharness/proxy.py @@ -5,15 +5,19 @@ It spawns the shared test harness server from test/harness/server.ts. """ +import asyncio import json import os import platform import re import subprocess +import warnings from typing import Any import httpx +PROCESS_SHUTDOWN_TIMEOUT_SECONDS = 5 + class CapiProxy: """Manages a replaying proxy server for E2E tests.""" @@ -46,15 +50,17 @@ async def start(self) -> str: cwd=os.path.dirname(server_path), shell=use_shell, ) + process = self._process + assert process.stdout is not None # Read until the server prints "Listening: http://..."; npm/npx may emit # wrapper output first on some platforms. line = "" match = None while True: - line = self._process.stdout.readline() + line = process.stdout.readline() if not line: - self._process.kill() + process.kill() raise RuntimeError("Failed to read proxy URL") match = re.search(r"Listening: (http://[^\s]+)", line.strip()) if match: @@ -63,17 +69,17 @@ async def start(self) -> str: self._proxy_url = match.group(1) metadata_match = re.search(r"(\{.*\})\s*$", line.strip()) if not metadata_match: - self._process.kill() + process.kill() raise RuntimeError(f"Proxy startup line missing CONNECT proxy metadata: {line}") try: metadata = json.loads(metadata_match.group(1)) except json.JSONDecodeError as exc: - self._process.kill() + process.kill() raise RuntimeError(f"Failed to parse proxy startup metadata: {line}") from exc self._connect_proxy_url = metadata.get("connectProxyUrl") self._ca_file_path = metadata.get("caFilePath") if not self._connect_proxy_url or not self._ca_file_path: - self._process.kill() + process.kill() raise RuntimeError(f"Proxy startup metadata missing CONNECT proxy details: {line}") return self._proxy_url @@ -97,10 +103,23 @@ async def stop(self, skip_writing_cache: bool = False): except Exception: pass # Best effort - # Wait for process to exit - self._process.wait() - self._process = None - self._proxy_url = None + try: + process = self._process + try: + await asyncio.to_thread(process.wait, timeout=PROCESS_SHUTDOWN_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + _kill_process_tree(process) + try: + await asyncio.to_thread(process.wait, timeout=PROCESS_SHUTDOWN_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + warnings.warn( + f"Proxy process {process.pid} did not exit after being killed", + RuntimeWarning, + stacklevel=2, + ) + finally: + self._process = None + self._proxy_url = None async def configure(self, file_path: str, work_dir: str): """Send configuration to the proxy.""" @@ -164,3 +183,28 @@ def get_proxy_env(self) -> dict[str, str]: "GH_ENTERPRISE_TOKEN": "", "GITHUB_ENTERPRISE_TOKEN": "", } + + +def _kill_process_tree(process: subprocess.Popen) -> None: + if process.poll() is not None: + return + + if platform.system() == "Windows": + try: + subprocess.run( + ["taskkill", "/PID", str(process.pid), "/T", "/F"], + check=False, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=PROCESS_SHUTDOWN_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + # Fall through to the direct-process kill below. + pass + + if process.poll() is None: + try: + process.kill() + except ProcessLookupError: + # The process exited between poll() and kill(). + pass diff --git a/rust/tests/e2e/rpc_additional_edge_cases.rs b/rust/tests/e2e/rpc_additional_edge_cases.rs index 56b9198ab7..26eb591f4f 100644 --- a/rust/tests/e2e/rpc_additional_edge_cases.rs +++ b/rust/tests/e2e/rpc_additional_edge_cases.rs @@ -1,6 +1,6 @@ use github_copilot_sdk::rpc::{ ModeSetRequest, NameSetRequest, PermissionsResetSessionApprovalsRequest, - PermissionsSetApproveAllRequest, PlanUpdateRequest, ShellExecRequest, + PermissionsSetApproveAllRequest, PlanUpdateRequest, ShellExecRequest, ShellKillRequest, WorkspacesCreateFileRequest, WorkspacesReadFileRequest, }; use github_copilot_sdk::session_events::SessionMode; @@ -39,6 +39,16 @@ async fn shell_exec_with_zero_timeout_does_not_kill_long_running_command() { marker_path.exists() }) .await; + let killed = session + .rpc() + .shell() + .kill(ShellKillRequest { + process_id: result.process_id, + signal: None, + }) + .await + .expect("kill zero-timeout shell process"); + assert!(killed.killed); session.disconnect().await.expect("disconnect session"); client.stop().await.expect("stop client"); @@ -547,7 +557,7 @@ async fn workspaces_getworkspace_returns_stable_result_across_calls() { #[cfg(windows)] fn delayed_marker_command(marker_path: &std::path::Path) -> String { format!( - "powershell -NoLogo -NoProfile -Command \"Start-Sleep -Seconds 2; Set-Content -LiteralPath '{}' -Value done\"", + "ping 127.0.0.1 -n 3 >nul & echo done>\"{}\" & ping 127.0.0.1 -n 61 >nul", marker_path.display() ) } @@ -555,7 +565,7 @@ fn delayed_marker_command(marker_path: &std::path::Path) -> String { #[cfg(not(windows))] fn delayed_marker_command(marker_path: &std::path::Path) -> String { format!( - "sh -c \"sleep 2; printf done > '{}'\"", + "sh -c \"sleep 2; printf done > '{}'; sleep 60\"", marker_path.display() ) } diff --git a/test/harness/connectProxy.test.ts b/test/harness/connectProxy.test.ts index 86d205dd39..ea2c8a27f5 100644 --- a/test/harness/connectProxy.test.ts +++ b/test/harness/connectProxy.test.ts @@ -60,6 +60,32 @@ describe("ConnectProxy", () => { await proxy.stop(); }); + test("stops with an active forward-proxy response", async () => { + let requestStarted!: () => void; + const started = new Promise((resolve) => { + requestStarted = resolve; + }); + const proxy = new ConnectProxy((_req, res) => { + res.writeHead(200); + res.write("partial"); + requestStarted(); + return true; + }); + await proxy.start(); + + const proxyUrl = new URL(proxy.proxyUrl); + const request = http.request({ + host: proxyUrl.hostname, + port: Number(proxyUrl.port), + path: "http://example.com/stream", + }); + request.on("error", () => {}); + request.end(); + await started; + + await proxy.stop(); + }); + test("intercepts HTTPS requests to configured domains", async () => { const requests: Array<{ host: string; url: string }> = []; const handler: RequestHandler = (req, res, targetHost) => { diff --git a/test/harness/connectProxy.ts b/test/harness/connectProxy.ts index d5aade0872..1de0ab8816 100644 --- a/test/harness/connectProxy.ts +++ b/test/harness/connectProxy.ts @@ -49,6 +49,8 @@ export class ConnectProxy { private passthroughDomains: Set; private onBlockedConnection?: (host: string, port: string) => void; private openSockets = new Set(); + private stopping = false; + private stopPromise?: Promise; constructor( private handler: RequestHandler, @@ -86,6 +88,8 @@ export class ConnectProxy { } async start(): Promise { + this.stopping = false; + this.stopPromise = undefined; this.ca = generateCA(); const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "e2e-proxy-ca-")); fs.writeFileSync(path.join(tmpDir, "test-ca.pem"), this.ca.certPem); @@ -137,35 +141,53 @@ export class ConnectProxy { } async stop(): Promise { - for (const socket of this.openSockets) { - socket.destroy(); + if (this.stopPromise) { + return this.stopPromise; } - this.openSockets.clear(); - const closeServer = (server?: http.Server) => - new Promise((resolve) => { - if (!server) { - resolve(); - return; - } - server.close(() => resolve()); - }); - - await Promise.all([ - closeServer(this.proxyServer), - closeServer(this.internalServer), - ]); - - if (this._caFilePath) { - try { - fs.rmSync(path.dirname(this._caFilePath), { - recursive: true, - force: true, + this.stopping = true; + const proxyServer = this.proxyServer; + const internalServer = this.internalServer; + const caFilePath = this._caFilePath; + this.proxyServer = undefined; + this.internalServer = undefined; + this._caFilePath = undefined; + this._proxyUrl = undefined; + + this.stopPromise = (async () => { + const closeServer = (server?: http.Server) => + new Promise((resolve) => { + if (!server) { + resolve(); + return; + } + server.close(() => resolve()); + server.closeAllConnections(); }); - } catch { - // Best-effort cleanup. + + for (const socket of this.openSockets) { + socket.destroy(); } - } + this.openSockets.clear(); + + await Promise.all([ + closeServer(proxyServer), + closeServer(internalServer), + ]); + + if (caFilePath) { + try { + fs.rmSync(path.dirname(caFilePath), { + recursive: true, + force: true, + }); + } catch { + // Best-effort cleanup. + } + } + })(); + + return this.stopPromise; } private handleConnect( @@ -173,6 +195,11 @@ export class ConnectProxy { clientSocket: net.Socket, head: Buffer, ) { + if (this.stopping) { + clientSocket.end("HTTP/1.1 503 Proxy Stopping\r\n\r\n"); + return; + } + const { host, port } = parseConnectTarget(req.url ?? ""); debugLog(`CONNECT ${host}:${port}`); if (!host) { @@ -244,6 +271,12 @@ export class ConnectProxy { req: http.IncomingMessage, res: http.ServerResponse, ) { + if (this.stopping) { + res.writeHead(503, { "content-type": "text/plain" }); + res.end("E2E proxy: stopping"); + return; + } + let targetHost: string; try { const url = new URL(req.url ?? ""); diff --git a/test/harness/test-mcp-oauth-server.mjs b/test/harness/test-mcp-oauth-server.mjs index eacd35f304..1ab7260eb0 100644 --- a/test/harness/test-mcp-oauth-server.mjs +++ b/test/harness/test-mcp-oauth-server.mjs @@ -53,10 +53,7 @@ export async function startOAuthMcpServer({ return; } - if ( - req.method === "GET" && - url.pathname === PROTECTED_RESOURCE_PATH - ) { + if (req.method === "GET" && url.pathname === PROTECTED_RESOURCE_PATH) { respondJson(res, 200, { resource: `${baseUrl}/mcp`, authorization_servers: [baseUrl], @@ -165,9 +162,10 @@ export async function startOAuthMcpServer({ url: `http://${host}:${address.port}`, requests, close: () => - new Promise((resolve, reject) => - server.close((err) => (err ? reject(err) : resolve())), - ), + new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + server.closeAllConnections(); + }), }; } @@ -313,7 +311,10 @@ function respondJson(res, statusCode, body) { res.end(data); } -if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { +if ( + process.argv[1] && + path.resolve(process.argv[1]) === fileURLToPath(import.meta.url) +) { const server = await startOAuthMcpServer({ expectedToken: process.env.EXPECTED_TOKEN ?? DEFAULT_EXPECTED_TOKEN, });