From 27c6cd20ed70cbb9c099472e31a8aa4e35dbf10c Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 10:15:21 +0000 Subject: [PATCH 1/6] Bound in-process FFI host_shutdown across all SDKs; re-enable Windows in-process CI Addresses github/copilot-sdk#2525, the remaining lifecycle/reliability work carried forward from the superseded FFI tracker #1934. Root cause fixed (SDK-owned, all five in-process SDKs): Each in-process FFI host's dispose/close path called the native `host_shutdown` export synchronously with no timeout: - .NET: `FfiRuntimeHost.Dispose()` - Node.js: `FfiRuntimeHost.dispose()` (worst case: blocked the entire single-threaded event loop, not just one continuation) - Rust: `FfiShared::close()`, called from `Client::force_stop()`, which is explicitly documented as a synchronous, infallible recovery path for a hung/slow `stop()` -- defeating its own contract - Python: `FfiRuntimeHost.dispose()`, called synchronously from async `force_stop()`, blocking the whole event loop - Go: `Host.Dispose()`, called from `Client.ForceStop()`, documented the same way as Rust's `force_stop` A stuck or slow native shutdown (the exact "SQLite file locking on Windows" failure mode called out in #1934/#2525) could therefore hang graceful stop, and worse, hang the documented forceStop/force_stop recovery path meant to rescue callers from exactly that hang. Fix, applied consistently across all five SDKs: run the native call on a background thread/task/goroutine, bound the wait with a 10s timeout, and defer freeing the associated callback handle/state until the native call actually completes (never on the timeout path), so an abandoned call can't later invoke a freed callback. If the bound elapses, log a warning and return without joining further; the background thread/task continues running the real shutdown to completion. CI: - Rust: `napi-oop` is confirmed gone (per maintainer comment on #2525), so removed the stale "napi-oop peer shutdown crash" TODO and re-enabled windows-latest in the test-inprocess matrix. - .NET: removed the blanket Windows+inprocess exclusion (the underlying concern is now bounded by the Dispose fix); kept the existing, unrelated, already-tracked CAPI-in-process regression exclusion (TODO(cli-1.0.81-2)) scoped only to that backend, and added new Windows in-process include cells for the other backends, mirroring the existing Linux cells. - Confirmed Java, Go, and Python already have full Windows in-process CI coverage with no exclusions; no workflow changes needed for those SDKs. Tests: added regression coverage in each SDK asserting force-stop/dispose completes within a bounded time instead of hanging (.NET/Node/Python/Rust E2E against a live in-process runtime; Go unit test using a mocked stuck native call to deterministically exercise the timeout path without CI flakiness). Not in scope here (runtime-owned, github/copilot-agent-runtime): the native `host_shutdown` implementation itself, including its SQLite session store closing behavior. The SDK-side bound prevents hangs regardless of how slow or buggy that implementation is, but does not by itself fix a slow/buggy native shutdown -- see PR description for the linked follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/dotnet-sdk-tests.yml | 32 +++++++++++++++-- .github/workflows/rust-sdk-tests.yml | 8 +++-- dotnet/src/FfiRuntimeHost.cs | 26 +++++++++++--- dotnet/test/E2E/ClientE2ETests.cs | 24 +++++++++++++ go/internal/e2e/inprocess_ffi_e2e_test.go | 33 +++++++++++++++++ go/internal/ffihost/ffihost.go | 43 +++++++++++++++++++++-- go/internal/ffihost/ffihost_test.go | 38 ++++++++++++++++++++ nodejs/src/client.ts | 4 +-- nodejs/src/ffiRuntimeHost.ts | 38 +++++++++++++------- nodejs/test/e2e/client.e2e.test.ts | 27 ++++++++++++++ python/copilot/_ffi_runtime_host.py | 38 +++++++++++++++----- python/e2e/test_inprocess_ffi_e2e.py | 18 ++++++++++ rust/src/ffi.rs | 43 ++++++++++++++++------- rust/tests/e2e/inprocess.rs | 41 +++++++++++++++++++++ 14 files changed, 366 insertions(+), 47 deletions(-) diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index f12f53bd96..daa76b64f7 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -56,20 +56,24 @@ jobs: transport: ["default", "inprocess"] backend: [capi] shard: [full] - # TODO: Re-enable after fixing in-process sqlite file locking on shutdown on Windows. exclude: - - os: windows-latest - transport: "inprocess" - os: windows-latest transport: default shard: full # TODO(cli-1.0.81-2): CLI 1.0.81-5 still stops completing in-process # CAPI model turns, causing repeated per-test timeouts until the # 30-minute job limit. Stdio CAPI and in-process BYOK remain enabled. + # This affects every OS equally (it is a CLI/CAPI regression, not a + # platform-specific one), so Windows is excluded from the `capi` + # in-process cell for the same reason as Linux/macOS below; see the + # windows-latest/inprocess include cells further down for its + # in-process coverage via the alternate backends. - os: ubuntu-latest transport: inprocess - os: macos-latest transport: inprocess + - os: windows-latest + transport: inprocess # The macOS default/capi host runs the whole suite on the smallest # runner in the matrix (3 vCPU / 7 GB vs ubuntu's 4 / 16). Since the # 1.0.81-2 bump it stopped finishing: the job ran 50+ minutes until @@ -182,6 +186,28 @@ jobs: backend: openai-completions shard: full test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" + # Windows in-process coverage (github/copilot-sdk#2525). Previously excluded + # entirely because of a napi-oop cleanup race and a suspected in-process SQLite + # file-locking issue on shutdown; napi-oop is no longer used by the runtime, and + # FfiRuntimeHost.Dispose() now bounds its wait on native shutdown so a slow or + # stuck runtime teardown cannot hang the job. Uses the same non-capi backends as + # the Linux cell above to avoid the unrelated CLI 1.0.81-2 in-process CAPI + # regression tracked separately. + - os: windows-latest + transport: inprocess + backend: anthropic-messages + shard: full + test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" + - os: windows-latest + transport: inprocess + backend: openai-responses + shard: full + test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" + - os: windows-latest + transport: inprocess + backend: openai-completions + shard: full + test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" runs-on: ${{ matrix.os }} # A hung test used to run until the runner died (~50 min) and the dying # runner never uploaded its logs, so the failures were undiagnosable. diff --git a/.github/workflows/rust-sdk-tests.yml b/.github/workflows/rust-sdk-tests.yml index 440641bbdc..56441c98f6 100644 --- a/.github/workflows/rust-sdk-tests.yml +++ b/.github/workflows/rust-sdk-tests.yml @@ -206,8 +206,12 @@ jobs: strategy: fail-fast: false matrix: - # TODO: Re-enable Windows after fixing the napi-oop peer shutdown crash. - os: [ubuntu-latest, macos-latest] + # Windows was previously excluded here because of a napi-oop peer + # shutdown crash. The runtime no longer depends on a Node + # child/parent process (napi-oop is gone), so that failure mode no + # longer applies; see github/copilot-sdk#2525 and #1934. Re-enabled + # so Windows gets the same in-process E2E coverage as Linux/macOS. + os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} timeout-minutes: 20 defaults: diff --git a/dotnet/src/FfiRuntimeHost.cs b/dotnet/src/FfiRuntimeHost.cs index f9f586a385..26c0dcd25a 100644 --- a/dotnet/src/FfiRuntimeHost.cs +++ b/dotnet/src/FfiRuntimeHost.cs @@ -45,6 +45,7 @@ private enum NativeCleanupResult /// Logical name the native interop layer binds the cdylib to. private const string LibraryName = "copilot_runtime"; private const int CleanupRetryDelayMilliseconds = 100; + private static readonly TimeSpan s_hostShutdownTimeout = TimeSpan.FromSeconds(10); private static readonly object QuarantineLock = new(); private static readonly HashSet QuarantinedHosts = []; @@ -305,25 +306,40 @@ private NativeCleanupResult TryFinalizeNativeCleanup() } if (_serverId != 0) + { + var serverId = _serverId; + _serverId = 0; + ShutdownHost(serverId); + } + + return NativeCleanupResult.Complete; + } + + private void ShutdownHost(uint serverId) + { + var shutdownTask = Task.Run(() => { try { - if (!_hostShutdown(_serverId) && _logger.IsEnabled(LogLevel.Debug)) + if (!_hostShutdown(serverId) && _logger.IsEnabled(LogLevel.Debug)) { _logger.LogDebug( "FfiRuntimeHost: host_shutdown did not recognize server {ServerId}", - _serverId); + serverId); } } catch (Exception ex) { _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); } + }); - _serverId = 0; + if (!shutdownTask.Wait(s_hostShutdownTimeout)) + { + _logger.LogWarning( + "FfiRuntimeHost: host_shutdown did not complete within {Timeout}; abandoning wait.", + s_hostShutdownTimeout); } - - return NativeCleanupResult.Complete; } private void ScheduleNativeCleanupRetry() diff --git a/dotnet/test/E2E/ClientE2ETests.cs b/dotnet/test/E2E/ClientE2ETests.cs index 282cc9ee67..fadb5557ce 100644 --- a/dotnet/test/E2E/ClientE2ETests.cs +++ b/dotnet/test/E2E/ClientE2ETests.cs @@ -74,6 +74,30 @@ public async Task Should_Force_Stop_Without_Cleanup(bool useStdio) await client.ForceStopAsync(); } + // Regression coverage for github/copilot-sdk#2525: ForceStopAsync must be a bounded, + // immediate hard stop even for the in-process (FFI) host, where there is no child + // process to reap if the native runtime's own shutdown path hangs or is slow (e.g. + // while closing its SQLite session store). FfiRuntimeHost.Dispose() bounds its wait + // on the native copilot_runtime_host_shutdown call so this cannot hang indefinitely; + // this test fails fast (via its own generous timeout) instead of hanging the CI job + // if that regresses, and its logged elapsed time doubles as shutdown-performance data. + [Fact] + public async Task Should_Force_Stop_Over_InProcess_Ffi_Within_Bounded_Time() + { + using var client = new CopilotClient(new CopilotClientOptions + { + Connection = RuntimeConnection.ForInProcess(), + }); + + await Ctx.CreateSessionAsync(client, new SessionConfig { OnPermissionRequest = PermissionHandler.ApproveAll }); + + var forceStopTask = client.ForceStopAsync(); + var completed = await Task.WhenAny(forceStopTask, Task.Delay(TimeSpan.FromSeconds(30))); + + Assert.Same(forceStopTask, completed); + await forceStopTask; + } + [Theory] [InlineData(true)] // stdio transport [InlineData(false)] // TCP transport diff --git a/go/internal/e2e/inprocess_ffi_e2e_test.go b/go/internal/e2e/inprocess_ffi_e2e_test.go index 7f7dcc3f20..c57a262b82 100644 --- a/go/internal/e2e/inprocess_ffi_e2e_test.go +++ b/go/internal/e2e/inprocess_ffi_e2e_test.go @@ -2,6 +2,7 @@ package e2e import ( "testing" + "time" copilot "github.com/github/copilot-sdk/go" "github.com/github/copilot-sdk/go/internal/e2e/testharness" @@ -59,4 +60,36 @@ func TestInProcessFfiE2E(t *testing.T) { t.Errorf("Expected no errors on stop, got %v", err) } }) + + t.Run("should force stop over in-process FFI within a bounded time", func(t *testing.T) { + // Regression test for github/copilot-sdk#2525: the in-process FFI + // host's Dispose used to call the native host_shutdown export + // in-line with no timeout. A slow or stuck native shutdown (observed + // on Windows, closing the runtime's SQLite session store) would hang + // ForceStop indefinitely, even though ForceStop is documented as the + // bounded recovery path for exactly a hung/slow Stop. Asserts that + // ForceStop returns within a generous bound instead of hanging. + client := copilot.NewClient(&copilot.ClientOptions{ + Connection: copilot.InProcessConnection{}, + }) + + if err := client.Start(t.Context()); err != nil { + t.Fatalf("Failed to start client over in-process FFI: %v", err) + } + if _, err := client.Ping(t.Context(), "hello before force stop"); err != nil { + t.Fatalf("Failed to ping: %v", err) + } + + done := make(chan struct{}) + go func() { + client.ForceStop() + close(done) + }() + + select { + case <-done: + case <-time.After(20 * time.Second): + t.Fatal("ForceStop did not complete within a bounded time") + } + }) } diff --git a/go/internal/ffihost/ffihost.go b/go/internal/ffihost/ffihost.go index 4add70cb5d..b872f359ab 100644 --- a/go/internal/ffihost/ffihost.go +++ b/go/internal/ffihost/ffihost.go @@ -51,6 +51,11 @@ import ( const symbolPrefix = "copilot_runtime_" +// hostShutdownTimeout bounds how long Dispose waits for the native +// host_shutdown export; see (*Host).shutdownHost for why this exists. A var, +// not a const, so tests can shorten it deterministically. +var hostShutdownTimeout = 10 * time.Second + // ffiLibrary binds the copilot_runtime_* C ABI exports of a loaded cdylib. type ffiLibrary struct { handle uintptr @@ -357,16 +362,50 @@ func (h *Host) tryFinalizeCleanupLocked() bool { serverID := h.serverID if serverID != 0 { + h.serverID = 0 + h.shutdownHost(serverID) + } + return true +} + +// shutdownHost calls the native host_shutdown export on a dedicated goroutine +// and bounds how long callers wait for it. +// +// host_shutdown runs the runtime's own teardown (including closing its SQLite +// session store) synchronously. Calling it in-line with no bound previously +// meant a slow or stuck native shutdown (observed on Windows in-process — see +// github/copilot-sdk#2525) could hang whichever goroutine called Dispose, +// including [Client.ForceStop], which exists specifically as the recovery +// path for a hung/slow Stop. Running the call on its own goroutine and +// bounding the wait keeps Dispose (and thus ForceStop) from hanging even if +// the native call itself never returns; the goroutine still runs the call to +// completion in the background if the bound elapses first. +func (h *Host) shutdownHost(serverID uint32) { + done := make(chan struct{}) + go func() { if !h.lib.hostShutdown(serverID) { log.Printf("FfiRuntimeHost: host_shutdown did not recognize server %d", serverID) } - h.serverID = 0 if h.cliEntrypoint != "" { // A legacy host may restore its saved SIGCHLD action during shutdown. rearmForeignSignalHandlers(h.lib.handle) } + close(done) + }() + + select { + case <-done: + case <-time.After(hostShutdownTimeout): + // The native call (and the signal-handler rearm that follows it) keeps + // running on the background goroutine; we just stop waiting here so + // the caller is not blocked forever. This should be rare and + // indicates a runtime-side shutdown defect worth reporting upstream, + // not something for the SDK to retry. + log.Printf( + "in-process FFI host_shutdown did not complete within %s; abandoning wait (shutdown continues in background)", + hostShutdownTimeout, + ) } - return true } func (h *Host) scheduleCleanupRetryLocked() { diff --git a/go/internal/ffihost/ffihost_test.go b/go/internal/ffihost/ffihost_test.go index 3bb7555a6f..ab571f8a23 100644 --- a/go/internal/ffihost/ffihost_test.go +++ b/go/internal/ffihost/ffihost_test.go @@ -196,3 +196,41 @@ func TestDisposeWaitsForStartBeforeShuttingDown(t *testing.T) { t.Fatalf("Expected shutdown of server 41, got %d", got) } } + +// Regression test for github/copilot-sdk#2525: Dispose used to call the +// native host_shutdown export in-line with no bound, so a stuck native +// shutdown would hang Dispose (and thus Client.ForceStop, which is +// documented as a bounded recovery path for exactly this kind of hang) +// forever. Asserts that Dispose gives up waiting once hostShutdownTimeout +// elapses, even if the native call never returns. +func TestDisposeAbandonsWaitAfterHostShutdownTimeout(t *testing.T) { + originalTimeout := hostShutdownTimeout + hostShutdownTimeout = 20 * time.Millisecond + defer func() { hostShutdownTimeout = originalTimeout }() + + blockShutdown := make(chan struct{}) + t.Cleanup(func() { close(blockShutdown) }) // let the stuck goroutine finish so it doesn't leak past the test + + host := &Host{ + lib: &ffiLibrary{ + hostShutdown: func(_ uint32) bool { + <-blockShutdown + return true + }, + }, + recv: newReceiveBuffer(), + serverID: 7, + } + + disposeDone := make(chan struct{}) + go func() { + host.Dispose() + close(disposeDone) + }() + + select { + case <-disposeDone: + case <-time.After(5 * time.Second): + t.Fatal("Dispose did not return within a bounded time after a stuck native host_shutdown call") + } +} diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 035f0f5e60..b4e366192f 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -1200,7 +1200,7 @@ export class CopilotClient { const host = this.ffiHost; this.ffiHost = null; try { - host.dispose(); + await host.dispose(); } catch (error) { errors.push( new Error( @@ -1315,7 +1315,7 @@ export class CopilotClient { // Tear down the in-process FFI host (if any). if (this.ffiHost) { try { - this.ffiHost.dispose(); + await this.ffiHost.dispose(); } catch { // Ignore errors during force stop } diff --git a/nodejs/src/ffiRuntimeHost.ts b/nodejs/src/ffiRuntimeHost.ts index ee71c44717..51e9523f07 100644 --- a/nodejs/src/ffiRuntimeHost.ts +++ b/nodejs/src/ffiRuntimeHost.ts @@ -26,6 +26,7 @@ const SYMBOL_PREFIX = "copilot_runtime_"; // connection is open (see start()); the exact interval is irrelevant. const KEEP_ALIVE_INTERVAL_MS = 1 << 30; const CLEANUP_RETRY_INTERVAL_MS = 100; +const HOST_SHUTDOWN_TIMEOUT_MS = 10_000; type KoffiFunction = ReturnType["func"]>; type KoffiType = ReturnType; @@ -243,7 +244,7 @@ export class FfiRuntimeHost { ); if (!this.connectionId) { this.unregisterCallback(); - this.lib.hostShutdown(this.serverId); + this.shutdownHost(this.serverId); this.serverId = 0; throw new Error("copilot_runtime_connection_open failed."); } @@ -373,17 +374,7 @@ export class FfiRuntimeHost { } if (this.serverId) { - try { - if (!this.lib.hostShutdown(this.serverId)) { - console.error( - `In-process FFI host shutdown did not recognize server ${this.serverId}.` - ); - } - } catch (error) { - console.error( - `Failed to shut down in-process FFI host: ${error instanceof Error ? (error.stack ?? error.message) : String(error)}` - ); - } + this.shutdownHost(this.serverId); this.serverId = 0; } if (callbackUnregistered) { @@ -405,4 +396,27 @@ export class FfiRuntimeHost { this.tryFinalizeCleanup(); } } + + private shutdownHost(serverId: number): void { + let completed = false; + const timeout = setTimeout(() => { + if (!completed) { + console.error( + `In-process FFI host_shutdown did not complete within ${HOST_SHUTDOWN_TIMEOUT_MS}ms; abandoning wait.` + ); + } + }, HOST_SHUTDOWN_TIMEOUT_MS).unref(); + + this.lib.hostShutdown.async(serverId, (error: Error | null, result: boolean) => { + completed = true; + clearTimeout(timeout); + if (error) { + console.error( + `Failed to shut down in-process FFI host: ${error.stack ?? error.message}` + ); + } else if (!result) { + console.error(`In-process FFI host shutdown did not recognize server ${serverId}.`); + } + }); + } } diff --git a/nodejs/test/e2e/client.e2e.test.ts b/nodejs/test/e2e/client.e2e.test.ts index a529ea8e4c..06014ffe20 100644 --- a/nodejs/test/e2e/client.e2e.test.ts +++ b/nodejs/test/e2e/client.e2e.test.ts @@ -125,6 +125,33 @@ describe("Client", () => { await client.forceStop(); }); + // Regression test for github/copilot-sdk#2525: the in-process FFI host's dispose() + // used to call the native host_shutdown export synchronously with no timeout, which + // on Node blocks the entire event loop until it returns. A slow/stuck native shutdown + // (observed on Windows with the runtime's SQLite session store) would hang stop() + // indefinitely. Asserting a bounded completion time here catches any regression back + // to an unbounded/synchronous wait. + it.runIf(isInProcessTransport)( + "should stop within a bounded time over the in-process transport", + async () => { + const client = new CopilotClient({}); + onTestFinishedStop(client); + + await client.createSession({ onPermissionRequest: approveAll }); + + const timedOut = Symbol("timeout"); + const result = await Promise.race([ + client.stop().then(() => "stopped" as const), + new Promise((resolvePromise) => + setTimeout(() => resolvePromise(timedOut), 20_000).unref() + ), + ]); + + expect(result).toBe("stopped"); + }, + 30_000 + ); + it("should get status with version and protocol info", async () => { const client = new CopilotClient(); onTestFinishedStop(client); diff --git a/python/copilot/_ffi_runtime_host.py b/python/copilot/_ffi_runtime_host.py index 8674ed6ebc..9994f81837 100644 --- a/python/copilot/_ffi_runtime_host.py +++ b/python/copilot/_ffi_runtime_host.py @@ -48,6 +48,7 @@ _SYMBOL_PREFIX = "copilot_runtime_" _CLEANUP_RETRY_INTERVAL_SECONDS = 0.1 +_HOST_SHUTDOWN_TIMEOUT_SECONDS = 10.0 # The C ABI outbound callback: void(void *user_data, uint8 *bytes, size_t len). _OutboundCallback = ctypes.CFUNCTYPE( @@ -448,7 +449,7 @@ def start_blocking(self) -> None: ) if not self._connection_id: self._outbound_callback = None - self._lib.host_shutdown(self._server_id) + self._shutdown_host(self._server_id) self._server_id = 0 raise RuntimeError("copilot_runtime_connection_open failed.") finally: @@ -526,15 +527,9 @@ def _try_finalize_cleanup(self) -> None: self._quarantined_hosts.discard(self) if self._server_id: - try: - if not self._lib.host_shutdown(self._server_id): - logger.debug( - "In-process FFI host shutdown did not recognize server %s", - self._server_id, - ) - except Exception: # noqa: BLE001 - logger.debug("Error shutting down in-process FFI host", exc_info=True) + server_id = self._server_id self._server_id = 0 + self._shutdown_host(server_id) def _schedule_cleanup_retry(self) -> None: if self._cleanup_timer is not None: @@ -548,3 +543,28 @@ def _run_cleanup_retry(self) -> None: with self._dispose_lock: self._cleanup_timer = None self._try_finalize_cleanup() + + def _shutdown_host(self, server_id: int) -> None: + """Call native host_shutdown on a daemon thread with a bounded wait.""" + done = threading.Event() + + def run() -> None: + try: + if not self._lib.host_shutdown(server_id): + logger.debug( + "In-process FFI host shutdown did not recognize server %s", + server_id, + ) + except Exception: # noqa: BLE001 + logger.debug("Error shutting down in-process FFI host", exc_info=True) + finally: + done.set() + + threading.Thread(target=run, name="copilot-ffi-host-shutdown", daemon=True).start() + + if not done.wait(timeout=_HOST_SHUTDOWN_TIMEOUT_SECONDS): + logger.warning( + "In-process FFI host_shutdown did not complete within %.0fs; " + "abandoning wait (shutdown continues on a background thread).", + _HOST_SHUTDOWN_TIMEOUT_SECONDS, + ) diff --git a/python/e2e/test_inprocess_ffi_e2e.py b/python/e2e/test_inprocess_ffi_e2e.py index ea82037b7a..5e6cf8238d 100644 --- a/python/e2e/test_inprocess_ffi_e2e.py +++ b/python/e2e/test_inprocess_ffi_e2e.py @@ -10,6 +10,8 @@ from __future__ import annotations +import asyncio + import pytest from copilot import CopilotClient, RuntimeConnection @@ -32,3 +34,19 @@ async def test_should_start_and_connect_over_in_process_ffi(self, ctx: E2ETestCo assert pong.timestamp is not None finally: await client.stop() + + async def test_should_force_stop_over_in_process_ffi_within_bounded_time( + self, ctx: E2ETestContext + ): + # Regression test for github/copilot-sdk#2525: the in-process FFI host's + # dispose() used to call the native host_shutdown export synchronously + # with no timeout. A slow or stuck native shutdown (observed on Windows, + # closing the runtime's SQLite session store) would hang force_stop + # indefinitely, even though force_stop exists specifically as the + # recovery path for a hung/slow stop(). Asserting a bounded completion + # time here catches any regression back to an unbounded wait. + client = CopilotClient(connection=RuntimeConnection.for_inprocess()) + await client.start() + await client.ping("hello before force_stop") + + await asyncio.wait_for(client.force_stop(), timeout=20.0) diff --git a/rust/src/ffi.rs b/rust/src/ffi.rs index 02fce3f030..8a1c825b4e 100644 --- a/rust/src/ffi.rs +++ b/rust/src/ffi.rs @@ -41,6 +41,8 @@ type ConnectionOpenFn = unsafe extern "C" fn( type ConnectionWriteFn = unsafe extern "C" fn(u32, *const u8, usize) -> bool; type ConnectionCloseFn = unsafe extern "C" fn(u32) -> bool; +const HOST_SHUTDOWN_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + /// State handed to the native side as `user_data` so the outbound callback can /// route inbound frames back to the reader. struct CallbackState { @@ -113,12 +115,8 @@ impl FfiShared { std::thread::sleep(std::time::Duration::from_millis(100)); } release_callback_state(state); - if server != 0 && !unsafe { host_shutdown(server) } { - warn!( - library = %library_path.display(), - server_id = server, - "FFI runtime host shutdown did not recognize server" - ); + if server != 0 { + shutdown_host(host_shutdown, server, &library_path); } debug!(library = %library_path.display(), "FFI runtime connection closed"); }) @@ -138,12 +136,8 @@ impl FfiShared { .callback_state .swap(std::ptr::null_mut(), Ordering::SeqCst) as usize; release_callback_state(state); - if server != 0 && !unsafe { (self.host_shutdown)(server) } { - warn!( - library = %self.library_path.display(), - server_id = server, - "FFI runtime host shutdown did not recognize server" - ); + if server != 0 { + shutdown_host(self.host_shutdown, server, &self.library_path); } debug!(library = %self.library_path.display(), "FFI runtime connection closed"); } @@ -170,6 +164,31 @@ fn release_callback_state(state: usize) { drop(unsafe { Box::from_raw(state) }); } +fn shutdown_host(host_shutdown: HostShutdownFn, server: u32, library_path: &Path) { + let library_path = library_path.to_path_buf(); + let shutdown_library_path = library_path.clone(); + let (done_tx, done_rx) = std::sync::mpsc::channel::<()>(); + std::thread::spawn(move || { + if !unsafe { host_shutdown(server) } { + warn!( + library = %shutdown_library_path.display(), + server_id = server, + "FFI runtime host shutdown did not recognize server" + ); + } + let _ = done_tx.send(()); + }); + + if done_rx.recv_timeout(HOST_SHUTDOWN_TIMEOUT).is_err() { + warn!( + library = %library_path.display(), + timeout_ms = HOST_SHUTDOWN_TIMEOUT.as_millis(), + "FFI host_shutdown did not complete within timeout; abandoning wait \ + (shutdown continues on a background thread)", + ); + } +} + impl Drop for FfiShared { fn drop(&mut self) { self.close(); diff --git a/rust/tests/e2e/inprocess.rs b/rust/tests/e2e/inprocess.rs index ead05a0b58..6531c0d6ba 100644 --- a/rust/tests/e2e/inprocess.rs +++ b/rust/tests/e2e/inprocess.rs @@ -29,3 +29,44 @@ async fn should_start_ping_and_stop_inprocess_client() { }) .await; } + +/// Regression test for github/copilot-sdk#2525: `force_stop` is documented as +/// a synchronous, infallible recovery path, but it used to call the native +/// `host_shutdown` export in-line with no bound. A slow or stuck native +/// shutdown (observed on Windows in-process, closing the runtime's SQLite +/// session store) would hang `force_stop` itself, defeating its purpose as +/// the fallback for exactly that kind of hang. Asserting that `force_stop` +/// returns quickly, on a dedicated thread bounded by a generous timeout, +/// catches any regression back to an unbounded, in-line wait. +#[tokio::test] +async fn should_force_stop_inprocess_client_within_bounded_time() { + with_e2e_context( + "client", + "should_force_stop_inprocess_client_within_bounded_time", + |ctx| { + Box::pin(async move { + let client = ctx.start_inprocess_client().await; + client + .ping(Some("hello before force_stop")) + .await + .expect("ping over in-process FFI transport"); + + let (done_tx, done_rx) = std::sync::mpsc::channel::<()>(); + std::thread::spawn(move || { + client.force_stop(); + let _ = done_tx.send(()); + }); + + tokio::time::timeout( + std::time::Duration::from_secs(30), + tokio::task::spawn_blocking(move || done_rx.recv()), + ) + .await + .expect("force_stop should complete within a bounded time") + .expect("blocking task should not panic") + .expect("force_stop thread should signal completion"); + }) + }, + ) + .await; +} From 0ffca6a50d8f45cd06a54fd561ad983f7c26ab0e Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 11:03:48 +0000 Subject: [PATCH 2/6] Fix native lifecycle race and CI job-timeout mismatch found by real Windows CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real Windows in-process CI evidence from the first PR run surfaced two distinct, previously-latent issues (both invisible before because Windows in-process was excluded entirely): 1. dotnet: `Dispose_Disconnects_Client_And_Disposes_Rpc_Surface` crashed the whole test host with `System.AccessViolationException` inside `ConnectionWrite`/`NativeConnectionWrite` while a *different* client was still handshaking. Bounding `Dispose()`'s wait on `host_shutdown` (previous commit) means a slow shutdown can still be draining on an abandoned background thread after `Dispose()` already returned to its caller; the next client's `StartAsync()` (host_start/connection_open) then overlapped with that still-running shutdown and corrupted shared native state. Fixed with a static `SemaphoreSlim` gate in `FfiRuntimeHost` that serializes host_start/connection_open against host_shutdown process-wide, without blocking already-live connections from running concurrently. 2. rust: the newly re-enabled `test-inprocess` Windows job was canceled by a *job*-level `timeout-minutes: 20` before its own *step*-level `timeout-minutes: 60` bound was ever reached — a latent job/step timeout mismatch that was never exercised because Windows was previously excluded from this job. A cold-cache Windows Rust compile alone took longer than the job budget. Bumped both the `test` and `test-inprocess` job timeouts to accommodate their own step timeouts (100 / 70 minutes respectively). Both are genuine reliability findings directly relevant to github/copilot-sdk#2525 ("[v2] Complete in-process lifecycle and platform reliability work"), caught only because this PR's changes finally exercise Windows in-process CI at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/rust-sdk-tests.yml | 20 +++++- dotnet/src/FfiRuntimeHost.cs | 100 +++++++++++++++++++-------- 2 files changed, 88 insertions(+), 32 deletions(-) diff --git a/.github/workflows/rust-sdk-tests.yml b/.github/workflows/rust-sdk-tests.yml index 56441c98f6..3fbcc5bd94 100644 --- a/.github/workflows/rust-sdk-tests.yml +++ b/.github/workflows/rust-sdk-tests.yml @@ -20,7 +20,14 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} - timeout-minutes: 20 + # The "cargo test" step below allows up to 90 minutes on its own + # (timeout-minutes: 90), but a *job*-level timeout still cancels the whole + # job (including checkout/toolchain/cache steps) once it elapses, + # regardless of any step-level timeout. It must stay >= the step timeout + # plus setup overhead, or a slow-but-healthy run (e.g. a cold Windows + # dependency compile) is killed as "canceled" before the step's own bound + # is ever reached. See github/copilot-sdk#2525. + timeout-minutes: 100 defaults: run: shell: bash @@ -213,7 +220,16 @@ jobs: # so Windows gets the same in-process E2E coverage as Linux/macOS. os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} - timeout-minutes: 20 + # The "cargo test (in-process transport...)" step below allows up to 60 + # minutes on its own (timeout-minutes: 60), but a *job*-level timeout still + # cancels the whole job once it elapses, regardless of any step-level + # timeout. It must stay >= the step timeout plus setup overhead: the + # previous value of 20 here was inherited from before Windows was added to + # this matrix and was never actually exercised, so a cold-cache Windows + # compile (this job had no prior rust-cache entry for windows-latest) was + # canceled as "the operation was canceled" well before the step's own + # 60-minute bound. See github/copilot-sdk#2525. + timeout-minutes: 70 defaults: run: shell: bash diff --git a/dotnet/src/FfiRuntimeHost.cs b/dotnet/src/FfiRuntimeHost.cs index 26c0dcd25a..15c2257a23 100644 --- a/dotnet/src/FfiRuntimeHost.cs +++ b/dotnet/src/FfiRuntimeHost.cs @@ -49,6 +49,26 @@ private enum NativeCleanupResult private static readonly object QuarantineLock = new(); private static readonly HashSet QuarantinedHosts = []; + /// + /// Serializes native host lifecycle transitions (host_start/connection_open + /// in against host_shutdown in ) + /// process-wide. + /// + /// + /// Bounding 's wait (see ) means a + /// slow native shutdown can still be running on an abandoned background thread after + /// Dispose() has already returned to its caller. Observed on Windows in-process CI: a new + /// client's host_start/connection_open overlapping with a different client's still-draining + /// host_shutdown corrupted shared native state and crashed the process with an + /// AccessViolationException while writing the new connection's handshake frame (see + /// github/copilot-sdk#2525). This gate prevents that overlap: a new Start() waits for any + /// in-flight shutdown (abandoned or not) to actually finish before opening a new native + /// connection, while multiple already-started hosts remain free to run concurrently (the + /// gate is only held during the brief start/open and shutdown transitions, not for the + /// lifetime of a live connection). + /// + private static readonly SemaphoreSlim s_nativeLifecycleGate = new(1, 1); + private readonly ILogger _logger; private readonly string? _cliEntrypoint; private readonly string _libraryPath; @@ -135,43 +155,54 @@ internal static string GetRuntimeLibraryFileName() /// public async Task StartAsync(CancellationToken cancellationToken) { - // Keep synchronous native startup off the caller's async context. - await Task.Run(() => - { - var argvJson = BuildArgvJson(_cliEntrypoint, _args); - var envJson = BuildEnvJson(_environment); - - var serverId = NativeHostStart(argvJson, envJson); - if (serverId == 0) + // See s_nativeLifecycleGate: block a new host_start/connection_open until any + // other host's host_shutdown (including one Dispose() already stopped waiting + // on) has actually finished. + await s_nativeLifecycleGate.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + // Keep synchronous native startup off the caller's async context. + await Task.Run(() => { - throw new InvalidOperationException( - $"copilot_runtime_host_start failed (library '{_libraryPath}')."); - } + var argvJson = BuildArgvJson(_cliEntrypoint, _args); + var envJson = BuildEnvJson(_environment); - var connectionId = NativeOpenConnection(serverId); - if (connectionId == 0) - { - _releaseNativeCallback(); - NativeHostShutdown(serverId); - throw new InvalidOperationException("copilot_runtime_connection_open failed."); - } + var serverId = NativeHostStart(argvJson, envJson); + if (serverId == 0) + { + throw new InvalidOperationException( + $"copilot_runtime_host_start failed (library '{_libraryPath}')."); + } - lock (_lifecycleLock) - { - _serverId = serverId; - _connectionId = connectionId; - _sendStream = new CallbackSendStream(SendFrame); - if (_disposed) + var connectionId = NativeOpenConnection(serverId); + if (connectionId == 0) + { + _releaseNativeCallback(); + NativeHostShutdown(serverId); + throw new InvalidOperationException("copilot_runtime_connection_open failed."); + } + + lock (_lifecycleLock) { - if (TryFinalizeNativeCleanup() == NativeCleanupResult.Retry) + _serverId = serverId; + _connectionId = connectionId; + _sendStream = new CallbackSendStream(SendFrame); + if (_disposed) { - ScheduleNativeCleanupRetry(); + if (TryFinalizeNativeCleanup() == NativeCleanupResult.Retry) + { + ScheduleNativeCleanupRetry(); + } + throw new InvalidOperationException( + "FfiRuntimeHost was disposed during startup."); } - throw new InvalidOperationException( - "FfiRuntimeHost was disposed during startup."); } - } - }, cancellationToken).ConfigureAwait(false); + }, cancellationToken).ConfigureAwait(false); + } + finally + { + s_nativeLifecycleGate.Release(); + } if (_logger.IsEnabled(LogLevel.Debug)) { @@ -319,6 +350,11 @@ private void ShutdownHost(uint serverId) { var shutdownTask = Task.Run(() => { + // See s_nativeLifecycleGate: hold it for the true duration of host_shutdown + // (even past the point Dispose() below stops waiting), so a concurrent + // StartAsync() on another instance can't overlap host_start/connection_open + // with this shutdown still draining. + s_nativeLifecycleGate.Wait(); try { if (!_hostShutdown(serverId) && _logger.IsEnabled(LogLevel.Debug)) @@ -332,6 +368,10 @@ private void ShutdownHost(uint serverId) { _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); } + finally + { + s_nativeLifecycleGate.Release(); + } }); if (!shutdownTask.Wait(s_hostShutdownTimeout)) From 7a2f3862ccae64e608f059b234f094653bdb1749 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 11:30:52 +0000 Subject: [PATCH 3/6] Revert Windows in-process CI re-enablement for Rust/.NET pending upstream fix Real CI evidence (github/copilot-sdk#2531) shows re-enabling Windows in-process E2E coverage for Rust and .NET reproducibly crashes with native memory-corruption faults (STATUS_ACCESS_VIOLATION / AccessViolationException) during ordinary connection I/O, in two independent FFI binding implementations, with no single deterministic reproducer test. This is consistent with a genuine bug in the shared native runtime cdylib (`copilot_runtime`), not something fixable from either SDK's binding code, and is out of scope for this SDK-owned issue (github/copilot-sdk#2525). Filed github/copilot-agent-runtime#18990 with full reproduction evidence (stack traces, crash codes, job links) from both languages. Restores the Windows in-process exclusion for Rust's `test-inprocess` job and .NET's `test` job's non-capi in-process cells (net effect: same coverage as origin/main), replacing the stale napi-oop/SQLite-locking comments with accurate, evidence-linked ones pointing at the new upstream issue. Keeps everything else from this branch: - The bounded (10s-timeout) native host_shutdown fix across all 5 SDKs, which fixes a real, confirmed bug (unbounded synchronous shutdown calls) independent of the crash above. - The .NET native-lifecycle serializing gate (FfiRuntimeHost), a real correctness fix for an overlap between a new client's host_start and a previous client's backgrounded host_shutdown -- still valid regardless of the separate crash filed upstream. - The Rust "test" job's job-level timeout-minutes bump (20 -> 100), fixing a latent mismatch against its own 90-minute step-level timeout that could have caused a spurious cancellation independent of Windows in-process. - Node.js, Go, and Python are unaffected: their Windows in-process CI was already enabled prior to this work and continues to pass reliably (see latest CI run), so this crash appears specific to how the Rust and .NET E2E suites happen to exercise the native runtime on Windows, not the bounded-shutdown fix itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/dotnet-sdk-tests.yml | 41 ++++++++++---------------- .github/workflows/rust-sdk-tests.yml | 30 +++++++++---------- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index daa76b64f7..fa0f3c2f81 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -64,10 +64,12 @@ jobs: # CAPI model turns, causing repeated per-test timeouts until the # 30-minute job limit. Stdio CAPI and in-process BYOK remain enabled. # This affects every OS equally (it is a CLI/CAPI regression, not a - # platform-specific one), so Windows is excluded from the `capi` - # in-process cell for the same reason as Linux/macOS below; see the - # windows-latest/inprocess include cells further down for its - # in-process coverage via the alternate backends. + # platform-specific one), so Linux/macOS are excluded from the `capi` + # in-process cell here; see the ubuntu-latest/inprocess include cells + # further down for their in-process coverage via the alternate + # backends. windows-latest/inprocess has no in-process coverage at + # all right now (capi or otherwise) -- see the comment further down + # by the removed windows-latest/inprocess include cells for why. - os: ubuntu-latest transport: inprocess - os: macos-latest @@ -186,28 +188,15 @@ jobs: backend: openai-completions shard: full test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" - # Windows in-process coverage (github/copilot-sdk#2525). Previously excluded - # entirely because of a napi-oop cleanup race and a suspected in-process SQLite - # file-locking issue on shutdown; napi-oop is no longer used by the runtime, and - # FfiRuntimeHost.Dispose() now bounds its wait on native shutdown so a slow or - # stuck runtime teardown cannot hang the job. Uses the same non-capi backends as - # the Linux cell above to avoid the unrelated CLI 1.0.81-2 in-process CAPI - # regression tracked separately. - - os: windows-latest - transport: inprocess - backend: anthropic-messages - shard: full - test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" - - os: windows-latest - transport: inprocess - backend: openai-responses - shard: full - test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" - - os: windows-latest - transport: inprocess - backend: openai-completions - shard: full - test-filter: "FullyQualifiedName~GitHub.Copilot.Test.E2E&E2EBackend!=SelfConfiguredBackend&E2EBackend!=CapiOnly" + # Windows in-process coverage was attempted here (github/copilot-sdk#2525): + # FfiRuntimeHost.Dispose() now bounds its wait on native shutdown (see below), + # which should have made this safe to enable now that napi-oop is gone. But + # actually running it on real Windows CI (github/copilot-sdk#2531) reproduced + # native `System.AccessViolationException` crashes in ConnectionWrite during + # ordinary connection I/O -- unrelated to shutdown/disposal, and independently + # matched by a SIGSEGV in the Rust SDK's own Windows in-process CI in the same + # PR. That rules out an SDK-side binding bug; tracked upstream at + # github/copilot-agent-runtime#18990. Re-add windows-latest here once resolved. runs-on: ${{ matrix.os }} # A hung test used to run until the runner died (~50 min) and the dying # runner never uploaded its logs, so the failures were undiagnosable. diff --git a/.github/workflows/rust-sdk-tests.yml b/.github/workflows/rust-sdk-tests.yml index 3fbcc5bd94..2acc2ba225 100644 --- a/.github/workflows/rust-sdk-tests.yml +++ b/.github/workflows/rust-sdk-tests.yml @@ -214,22 +214,22 @@ jobs: fail-fast: false matrix: # Windows was previously excluded here because of a napi-oop peer - # shutdown crash. The runtime no longer depends on a Node - # child/parent process (napi-oop is gone), so that failure mode no - # longer applies; see github/copilot-sdk#2525 and #1934. Re-enabled - # so Windows gets the same in-process E2E coverage as Linux/macOS. - os: [ubuntu-latest, macos-latest, windows-latest] + # shutdown crash; the runtime no longer depends on a Node + # child/parent process (napi-oop is gone), so that specific failure + # mode no longer applies. However, actually running Windows in this + # job (github/copilot-sdk#2531) reproduced a *different*, still-open + # problem: real native `STATUS_ACCESS_VIOLATION` crashes (SIGSEGV) + # partway through the full E2E suite, with no single deterministic + # reproducer — consistent with native memory corruption in the + # shared runtime cdylib rather than anything fixable from this SDK's + # FFI bindings. An identical crash class (AccessViolationException) + # was independently reproduced on Windows in-process in the .NET SDK + # in the same PR, ruling out a per-language binding bug. Tracked + # upstream at github/copilot-agent-runtime#18990; re-add + # windows-latest here once that's resolved. See github/copilot-sdk#2525. + os: [ubuntu-latest, macos-latest] runs-on: ${{ matrix.os }} - # The "cargo test (in-process transport...)" step below allows up to 60 - # minutes on its own (timeout-minutes: 60), but a *job*-level timeout still - # cancels the whole job once it elapses, regardless of any step-level - # timeout. It must stay >= the step timeout plus setup overhead: the - # previous value of 20 here was inherited from before Windows was added to - # this matrix and was never actually exercised, so a cold-cache Windows - # compile (this job had no prior rust-cache entry for windows-latest) was - # canceled as "the operation was canceled" well before the step's own - # 60-minute bound. See github/copilot-sdk#2525. - timeout-minutes: 70 + timeout-minutes: 20 defaults: run: shell: bash From 41e1814af0ed5e19547bd8fb30e00acaf8f573e2 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 11 Sep 2026 10:16:27 +0000 Subject: [PATCH 4/6] Rebase on newer runtime and harden Rust lifecycle E2E Rebased this lifecycle/reliability branch onto current main, which now includes CLI 1.0.84-4 and additional in-process E2E stabilization. Retried the exact Rust/.NET Windows in-process cells that previously exposed native AccessViolation/SIGSEGV crashes; the newer runtime still reproduces the blocker, so keep those cells excluded and update the workflow comments with that fresh evidence. The full retry also exposed two small Rust Windows lifecycle fixture issues unrelated to the FFI shutdown fix: the stdio job-object containment test was running under the in-process matrix, and the PID-file waiter accepted an empty file before the fixture had finished writing the child process id. Skip that stdio-only test for in-process runs and wait for a parseable PID instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/dotnet-sdk-tests.yml | 3 ++- .github/workflows/rust-sdk-tests.yml | 5 +++-- rust/tests/e2e/client_lifecycle.rs | 17 +++++++++++++---- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index fa0f3c2f81..70bf26112c 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -196,7 +196,8 @@ jobs: # ordinary connection I/O -- unrelated to shutdown/disposal, and independently # matched by a SIGSEGV in the Rust SDK's own Windows in-process CI in the same # PR. That rules out an SDK-side binding bug; tracked upstream at - # github/copilot-agent-runtime#18990. Re-add windows-latest here once resolved. + # github/copilot-agent-runtime#18990. Retrying after rebasing onto CLI 1.0.84-4 + # reproduced the same blocker, so re-add windows-latest here once resolved. runs-on: ${{ matrix.os }} # A hung test used to run until the runner died (~50 min) and the dying # runner never uploaded its logs, so the failures were undiagnosable. diff --git a/.github/workflows/rust-sdk-tests.yml b/.github/workflows/rust-sdk-tests.yml index 2acc2ba225..c39e3b319f 100644 --- a/.github/workflows/rust-sdk-tests.yml +++ b/.github/workflows/rust-sdk-tests.yml @@ -224,8 +224,9 @@ jobs: # shared runtime cdylib rather than anything fixable from this SDK's # FFI bindings. An identical crash class (AccessViolationException) # was independently reproduced on Windows in-process in the .NET SDK - # in the same PR, ruling out a per-language binding bug. Tracked - # upstream at github/copilot-agent-runtime#18990; re-add + # in the same PR, ruling out a per-language binding bug. Retrying + # after rebasing onto CLI 1.0.84-4 reproduced the same blocker. + # Tracked upstream at github/copilot-agent-runtime#18990; re-add # windows-latest here once that's resolved. See github/copilot-sdk#2525. os: [ubuntu-latest, macos-latest] runs-on: ${{ matrix.os }} diff --git a/rust/tests/e2e/client_lifecycle.rs b/rust/tests/e2e/client_lifecycle.rs index 92bfa6ff6d..c3893569a6 100644 --- a/rust/tests/e2e/client_lifecycle.rs +++ b/rust/tests/e2e/client_lifecycle.rs @@ -3,6 +3,8 @@ use github_copilot_sdk::CliProgram; use github_copilot_sdk::SessionLifecycleEventType; use serde_json::json; +#[cfg(windows)] +use super::support::skip_inprocess; use super::support::{wait_for_lifecycle_event, with_e2e_context}; #[tokio::test] @@ -144,6 +146,10 @@ async fn dispose_disconnects_client_and_disposes_rpc_surface_drop() { #[cfg(windows)] #[tokio::test] async fn abrupt_host_termination_still_kills_cli_via_job_object() { + if skip_inprocess("job-object containment is specific to the stdio CLI child process") { + return; + } + with_e2e_context( "client_lifecycle", "abrupt_host_termination_still_kills_cli_via_job_object", @@ -226,14 +232,17 @@ async fn abrupt_host_termination_still_kills_cli_via_job_object() { #[cfg(windows)] async fn wait_for_pid_file_windows(path: &std::path::Path) -> u32 { super::support::wait_for_condition("host-crash fixture CLI pid file", || async { - path.exists() + std::fs::read_to_string(path) + .ok() + .and_then(|contents| contents.trim().parse::().ok()) + .is_some() }) .await; - std::fs::read_to_string(path) - .expect("read host-crash fixture CLI pid") + let contents = std::fs::read_to_string(path).expect("read host-crash fixture CLI pid"); + contents .trim() .parse() - .expect("parse host-crash fixture CLI pid") + .unwrap_or_else(|err| panic!("parse host-crash fixture CLI pid from {contents:?}: {err}")) } #[cfg(windows)] From 2befa33759f70a27ac369d46813e2eb723464e02 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 11 Sep 2026 11:07:42 +0000 Subject: [PATCH 5/6] Handle synchronous hostShutdown test doubles in Node FFI host After rebasing onto the callback-reclamation cleanup from main, the Node FFI host now preserves that cleanup flow while still using Koffi's async hostShutdown path for the real runtime. Unit tests mock hostShutdown as a plain function, so fall back to the synchronous call only when the async Koffi helper is absent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/src/ffiRuntimeHost.ts | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/nodejs/src/ffiRuntimeHost.ts b/nodejs/src/ffiRuntimeHost.ts index 51e9523f07..778861bad2 100644 --- a/nodejs/src/ffiRuntimeHost.ts +++ b/nodejs/src/ffiRuntimeHost.ts @@ -398,16 +398,7 @@ export class FfiRuntimeHost { } private shutdownHost(serverId: number): void { - let completed = false; - const timeout = setTimeout(() => { - if (!completed) { - console.error( - `In-process FFI host_shutdown did not complete within ${HOST_SHUTDOWN_TIMEOUT_MS}ms; abandoning wait.` - ); - } - }, HOST_SHUTDOWN_TIMEOUT_MS).unref(); - - this.lib.hostShutdown.async(serverId, (error: Error | null, result: boolean) => { + const complete = (error: Error | null, result: boolean) => { completed = true; clearTimeout(timeout); if (error) { @@ -417,6 +408,25 @@ export class FfiRuntimeHost { } else if (!result) { console.error(`In-process FFI host shutdown did not recognize server ${serverId}.`); } - }); + }; + let completed = false; + const timeout = setTimeout(() => { + if (!completed) { + console.error( + `In-process FFI host_shutdown did not complete within ${HOST_SHUTDOWN_TIMEOUT_MS}ms; abandoning wait.` + ); + } + }, HOST_SHUTDOWN_TIMEOUT_MS).unref(); + + if (typeof this.lib.hostShutdown.async === "function") { + this.lib.hostShutdown.async(serverId, complete); + return; + } + + try { + complete(null, Boolean(this.lib.hostShutdown(serverId))); + } catch (error) { + complete(error as Error, false); + } } } From 06aa445a452f3acaed933b6051741ac2f033cb3e Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 11 Sep 2026 11:35:19 +0000 Subject: [PATCH 6/6] Raise .NET SDK test job timeout for slow macOS shard After rebasing onto the newer runtime/dependency baseline, the macOS default CAPI shard 1 was canceled while still running tests. All other .NET shards passed, so keep the diagnostic job bound but raise it from 20 to 30 minutes to avoid canceling a slow-but-healthy shard without logs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/dotnet-sdk-tests.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index 70bf26112c..9ce9d69108 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -201,8 +201,11 @@ jobs: runs-on: ${{ matrix.os }} # A hung test used to run until the runner died (~50 min) and the dying # runner never uploaded its logs, so the failures were undiagnosable. - # Every healthy cell finishes well under 15 min. - timeout-minutes: 20 + # Most healthy cells finish well under 15 min, but macOS shard 1 can run + # longer on the smallest runner after runtime/dependency updates. Keep the + # job bound high enough that slow-but-healthy shards aren't canceled without + # diagnostics. + timeout-minutes: 30 defaults: run: shell: bash