From 9e9da98cfd5274c1ba95b938cbca91b7e9b847b6 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 10:15:21 +0000 Subject: [PATCH 1/4] 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 | 74 +++++++++++++++++++--- dotnet/test/E2E/ClientE2ETests.cs | 24 +++++++ go/internal/e2e/inprocess_ffi_e2e_test.go | 33 ++++++++++ go/internal/ffihost/ffihost.go | 49 +++++++++++++-- go/internal/ffihost/ffihost_test.go | 38 +++++++++++ nodejs/src/client.ts | 4 +- nodejs/src/ffiRuntimeHost.ts | 68 ++++++++++++++++---- nodejs/test/e2e/client.e2e.test.ts | 27 ++++++++ python/copilot/_ffi_runtime_host.py | 63 ++++++++++++++++--- python/e2e/test_inprocess_ffi_e2e.py | 18 ++++++ rust/src/ffi.rs | 77 ++++++++++++++++++++--- rust/tests/e2e/inprocess.rs | 42 +++++++++++++ 14 files changed, 506 insertions(+), 51 deletions(-) diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index 9b7216ad20..ce79f984a3 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 828459126b..26cc3d92da 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 cc3bccae92..4ec3569833 100644 --- a/dotnet/src/FfiRuntimeHost.cs +++ b/dotnet/src/FfiRuntimeHost.cs @@ -3,6 +3,7 @@ *--------------------------------------------------------------------------------------------*/ using Microsoft.Extensions.Logging; +using System.Diagnostics; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -38,6 +39,24 @@ internal sealed partial class FfiRuntimeHost : IDisposable /// Logical name the native interop layer binds the cdylib to. private const string LibraryName = "copilot_runtime"; + /// + /// Upper bound on how long waits for the native + /// copilot_runtime_host_shutdown call to return. + /// + /// + /// This call runs the loaded runtime's own teardown (including closing its SQLite + /// session store) synchronously in this process, with no cancellation hook exposed + /// across the FFI boundary. A caller may already have asked the runtime to shut down + /// gracefully over JSON-RPC (Runtime.ShutdownAsync) before reaching here, so + /// this call is expected to be fast; it exists mainly to release the loaded + /// library's resources. But because in-process hosting shares this process (there is + /// no child process to kill if it does not return), a stuck or slow native shutdown + /// would otherwise hang forever, defeating ForceStopAsync's + /// contract of an immediate hard stop. Bounding the wait keeps teardown deterministic + /// even if the runtime's shutdown path never returns; see github/copilot-sdk#2525. + /// + private static readonly TimeSpan s_hostShutdownTimeout = TimeSpan.FromSeconds(10); + private readonly ILogger _logger; private readonly string? _cliEntrypoint; private readonly string _libraryPath; @@ -225,21 +244,58 @@ public void Dispose() _logger.LogDebug(ex, "FfiRuntimeHost: connection_close failed"); } - try + _receiveStream.Complete(); + + var serverId = _serverId; + _serverId = 0; + if (serverId == 0) { - if (_serverId != 0) + DisposeNativeCallback(); + return; + } + + var shutdownTimestamp = Stopwatch.GetTimestamp(); + + // Run the blocking native call on a pooled thread so this Dispose() call can + // enforce a bound on it instead of hanging indefinitely if the runtime's own + // shutdown never returns. The callback GCHandle is freed only once the native + // call actually returns (inside the task, not here), so a slow-but-eventually- + // completing shutdown cannot race a native callback against a freed handle even + // when this method stops waiting early. + var shutdownTask = Task.Run(() => + { + try { - NativeHostShutdown(_serverId); - _serverId = 0; + NativeHostShutdown(serverId); + } + catch (Exception ex) + { + _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); + } + finally + { + DisposeNativeCallback(); } + }); + + if (shutdownTask.Wait(s_hostShutdownTimeout)) + { + LoggingHelpers.LogTiming(_logger, LogLevel.Debug, null, + "FfiRuntimeHost: host_shutdown complete. Elapsed={Elapsed}", + shutdownTimestamp); } - catch (Exception ex) + else { - _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); + // The native call (and the callback cleanup that follows it) keeps running + // on the abandoned background thread; we just stop waiting on it here so the + // caller (e.g. ForceStopAsync) 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. + LoggingHelpers.LogTiming(_logger, LogLevel.Warning, null, + "FfiRuntimeHost: host_shutdown did not complete within Elapsed={Elapsed}, Timeout={Timeout}; abandoning wait.", + shutdownTimestamp, + s_hostShutdownTimeout); } - - _receiveStream.Complete(); - DisposeNativeCallback(); } /// Length as the native pointer-sized unsigned integer the ABI expects. 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 9a824881de..237f03f2a2 100644 --- a/go/internal/ffihost/ffihost.go +++ b/go/internal/ffihost/ffihost.go @@ -38,10 +38,12 @@ import ( "encoding/json" "fmt" "io" + "log" "runtime" "strings" "sync" "sync/atomic" + "time" "unsafe" "github.com/ebitengine/purego" @@ -49,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 shrink it temporarily. +var hostShutdownTimeout = 10 * time.Second + // ffiLibrary binds the copilot_runtime_* C ABI exports of a loaded cdylib. type ffiLibrary struct { handle uintptr @@ -223,10 +230,7 @@ func (h *Host) Start() error { if h.connectionID == 0 { outboundTargets.Delete(callbackToken) h.callbackToken = 0 - h.lib.hostShutdown(h.serverID) - if h.cliEntrypoint != "" { - rearmForeignSignalHandlers(h.lib.handle) - } + h.shutdownHost(h.serverID) h.serverID = 0 return fmt.Errorf("copilot_runtime_connection_open failed") } @@ -358,14 +362,49 @@ func (h *Host) Dispose() { if connID != 0 { h.lib.connectionClose(connID) } + h.recv.Close() + if serverID != 0 { + h.shutdownHost(serverID) + } +} + +// 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() { h.lib.hostShutdown(serverID) 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, + ) } - h.recv.Close() } // hostWriter adapts Host into the io.WriteCloser jsonrpc2 writes request frames to. diff --git a/go/internal/ffihost/ffihost_test.go b/go/internal/ffihost/ffihost_test.go index ccb48af419..29f9b0fbff 100644 --- a/go/internal/ffihost/ffihost_test.go +++ b/go/internal/ffihost/ffihost_test.go @@ -133,3 +133,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 306e1ee05b..01ddb53493 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -1177,7 +1177,7 @@ export class CopilotClient { const host = this.ffiHost; this.ffiHost = null; try { - host.dispose(); + await host.dispose(); } catch (error) { errors.push( new Error( @@ -1292,7 +1292,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 4795e325ce..d37e210a5b 100644 --- a/nodejs/src/ffiRuntimeHost.ts +++ b/nodejs/src/ffiRuntimeHost.ts @@ -26,6 +26,10 @@ const SYMBOL_PREFIX = "copilot_runtime_"; // connection is open (see start()); the exact interval is irrelevant. const KEEP_ALIVE_INTERVAL_MS = 1 << 30; +// Upper bound on how long dispose() waits for the native host_shutdown call; see +// shutdownHost() for why this exists. +const HOST_SHUTDOWN_TIMEOUT_MS = 10_000; + type KoffiFunction = ReturnType["func"]>; type KoffiType = ReturnType; type KoffiRegisteredCallback = ReturnType; @@ -226,9 +230,9 @@ export class FfiRuntimeHost { 0 ); if (!this.connectionId) { - this.unregisterCallback(); - this.lib.hostShutdown(this.serverId); + const serverId = this.serverId; this.serverId = 0; + await this.shutdownHost(serverId); throw new Error("copilot_runtime_connection_open failed."); } @@ -297,7 +301,7 @@ export class FfiRuntimeHost { } /** Closes the FFI connection, shuts down the native host, and releases resources. */ - dispose(): void { + async dispose(): Promise { if (this.disposed) { return; } @@ -317,16 +321,56 @@ export class FfiRuntimeHost { // Ignore teardown failures. } - try { - if (this.serverId) { - this.lib.hostShutdown(this.serverId); - this.serverId = 0; - } - } catch { - // Ignore teardown failures. + this.receiveStream.end(); + + const serverId = this.serverId; + this.serverId = 0; + if (serverId) { + await this.shutdownHost(serverId); + } else { + this.unregisterCallback(); } + } - this.receiveStream.end(); - this.unregisterCallback(); + /** + * Calls the native `host_shutdown` export and bounds how long {@link dispose} waits + * for it. + * + * This runs the runtime's own teardown (including closing its SQLite session store) + * in this process. Calling it synchronously previously blocked the entire Node event + * loop until it returned, with no way to time out — on Windows in-process, a slow or + * stuck shutdown could hang the whole process, which is exactly the failure mode this + * bounds against (see github/copilot-sdk#2525). Using koffi's `.async` variant runs + * the call on koffi's native thread pool instead of the event loop, so a stuck call + * cannot freeze the process, and racing it against a timeout keeps `dispose()` (and + * thus `forceStop()`) from hanging indefinitely even if the native call itself never + * returns. The callback still unregisters the outbound callback once the native call + * completes, whether or not this method already timed out waiting for it. + */ + private async shutdownHost(serverId: number): Promise { + const shutdownCompleted = new Promise((resolvePromise) => { + this.lib.hostShutdown.async(serverId, () => { + this.unregisterCallback(); + resolvePromise(); + }); + }); + + const timedOut = Symbol("host_shutdown timeout"); + const result = await Promise.race([ + shutdownCompleted.then(() => "completed" as const), + new Promise((resolvePromise) => + setTimeout(() => resolvePromise(timedOut), HOST_SHUTDOWN_TIMEOUT_MS).unref() + ), + ]); + + if (result === timedOut) { + // The native call (and the callback unregistration that follows it) keeps + // running; 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. + console.error( + `In-process FFI host_shutdown did not complete within ${HOST_SHUTDOWN_TIMEOUT_MS}ms; abandoning wait.` + ); + } } } diff --git a/nodejs/test/e2e/client.e2e.test.ts b/nodejs/test/e2e/client.e2e.test.ts index bc3421bfa1..84c14b0887 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 5665f8fba7..50adc62848 100644 --- a/python/copilot/_ffi_runtime_host.py +++ b/python/copilot/_ffi_runtime_host.py @@ -49,6 +49,10 @@ _SYMBOL_PREFIX = "copilot_runtime_" +# Upper bound on how long dispose() waits for the native host_shutdown call to +# complete; see FfiRuntimeHost._shutdown_host for why this exists. +_HOST_SHUTDOWN_TIMEOUT_SECONDS = 10.0 + # The C ABI outbound callback: void(void *user_data, uint8 *bytes, size_t len). _OutboundCallback = ctypes.CFUNCTYPE( None, ctypes.c_void_p, ctypes.POINTER(ctypes.c_uint8), ctypes.c_size_t @@ -438,7 +442,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.") @@ -502,14 +506,53 @@ def dispose(self) -> None: except Exception: # noqa: BLE001 logger.debug("Error closing in-process FFI connection", exc_info=True) - try: - if self._server_id: - self._lib.host_shutdown(self._server_id) - self._server_id = 0 - 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 + if server_id: + self._shutdown_host(server_id) + else: + self._outbound_callback = None self._receive_buffer.close() - # Safe to drop now: no native code can invoke the callback after - # connection_close, and all in-flight callbacks have drained. - self._outbound_callback = None + + def _shutdown_host(self, server_id: int) -> None: + """Calls the native ``host_shutdown`` export 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 thread + called ``dispose()``, including the synchronous ``terminate``/``kill``/ + ``wait`` methods of the process-like adapter used by + :meth:`CopilotClient.force_stop`. Running the call on a dedicated + thread and bounding the wait keeps callers from hanging even if the + native call itself never returns; the callback reference is only + dropped once the native call actually completes, whether or not this + bound elapsed first (freeing it earlier risks a use-after-free if + native code were to invoke it from an abandoned call). + """ + done = threading.Event() + + def run() -> None: + try: + self._lib.host_shutdown(server_id) + except Exception: # noqa: BLE001 + logger.debug("Error shutting down in-process FFI host", exc_info=True) + finally: + self._outbound_callback = None + done.set() + + threading.Thread(target=run, name="copilot-ffi-host-shutdown", daemon=True).start() + + if not done.wait(timeout=_HOST_SHUTDOWN_TIMEOUT_SECONDS): + # The native call (and the callback drop that follows it) keeps + # running on the background thread; 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. + 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 a25990062c..474fb2e1a8 100644 --- a/rust/src/ffi.rs +++ b/rust/src/ffi.rs @@ -86,9 +86,27 @@ pub(crate) struct FfiShared { unsafe impl Send for FfiShared {} unsafe impl Sync for FfiShared {} +/// Upper bound on how long [`FfiShared::close`] waits for the native +/// `host_shutdown` export before giving up. +/// +/// `host_shutdown` runs the runtime's own teardown (including closing its +/// SQLite session store) synchronously. Previously `close()` called it +/// in-line with no bound, so a slow or stuck native shutdown (observed on +/// Windows in-process — see github/copilot-sdk#2525) would hang whichever +/// thread called `close()`, including [`Client::force_stop`], which is +/// documented as a synchronous, infallible recovery path for exactly this +/// kind of hang. Running the native call on a dedicated thread and bounding +/// the wait keeps `close()` (and thus `force_stop`) from hanging even if the +/// native call itself never returns; the spawned thread still runs the call +/// to completion and frees the callback state once it does, whether or not +/// this bound elapsed first. +const HOST_SHUTDOWN_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + impl FfiShared { /// Close the connection, shut the host down, and free the callback state. - /// Idempotent; called from [`Client::stop`], drop, and on startup failure. + /// Idempotent; called from [`Client::stop`], [`Client::force_stop`], drop, + /// and on startup failure. Synchronous but bounded: see + /// [`HOST_SHUTDOWN_TIMEOUT`]. pub(crate) fn close(&self) { let _operation = self.operation_lock.lock(); if self.closed.swap(true, Ordering::SeqCst) { @@ -102,22 +120,65 @@ impl FfiShared { if conn != 0 { unsafe { (self.connection_close)(conn) }; } + let server = self.server_id.swap(0, Ordering::SeqCst); - if server != 0 { - unsafe { (self.host_shutdown)(server) }; - } - // Free the callback state only after the connection is closed and the - // host is shut down, so native can no longer invoke the callback. - let state = self + let callback_state = self .callback_state .swap(std::ptr::null_mut(), Ordering::SeqCst); + let library_path = self.library_path.clone(); + + if server == 0 { + // Nothing native to shut down; free the callback state (if any) inline. + Self::finish_close(callback_state, &library_path); + return; + } + + let host_shutdown = self.host_shutdown; + // Raw pointers aren't `Send`, but this one is only ever dereferenced by + // native code (which doesn't care which thread calls it) or freed once, + // after `host_shutdown` returns, so moving it into the spawned thread is + // sound. + let callback_state_addr = callback_state as usize; + let shutdown_library_path = library_path.clone(); + let (done_tx, done_rx) = std::sync::mpsc::channel::<()>(); + std::thread::spawn(move || { + unsafe { (host_shutdown)(server) }; + // Free the callback state only after the host is shut down, so + // native can no longer invoke the callback. + Self::finish_close( + callback_state_addr as *mut CallbackState, + &shutdown_library_path, + ); + let _ = done_tx.send(()); + }); + + if done_rx.recv_timeout(HOST_SHUTDOWN_TIMEOUT).is_err() { + // The native call (and the callback-state cleanup that follows it) + // keeps running on the spawned thread; we just stop waiting here so + // the caller isn't blocked forever. This should be rare and + // indicates a runtime-side shutdown defect worth reporting + // upstream, not something for the SDK to retry. + tracing::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)", + ); + } + } + + /// Waits out any in-flight outbound callbacks and frees the callback + /// state. Must only be called after the native host has been (or is + /// guaranteed never to be) shut down, so native can no longer invoke the + /// callback. + fn finish_close(state: *mut CallbackState, library_path: &Path) { if !state.is_null() { while unsafe { &*state }.active_callbacks.load(Ordering::SeqCst) != 0 { std::thread::yield_now(); } drop(unsafe { Box::from_raw(state) }); } - debug!(library = %self.library_path.display(), "FFI runtime connection closed"); + debug!(library = %library_path.display(), "FFI runtime connection closed"); } fn write_frame(&self, frame: &[u8]) -> bool { diff --git a/rust/tests/e2e/inprocess.rs b/rust/tests/e2e/inprocess.rs index ead05a0b58..3c6ec061a9 100644 --- a/rust/tests/e2e/inprocess.rs +++ b/rust/tests/e2e/inprocess.rs @@ -29,3 +29,45 @@ 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 806a3c3a9eb945fce09afe110c98dcd808163f6b Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 10:23:04 +0000 Subject: [PATCH 2/4] Fix nightly rustfmt trailing blank line in inprocess.rs e2e test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- rust/tests/e2e/inprocess.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rust/tests/e2e/inprocess.rs b/rust/tests/e2e/inprocess.rs index 3c6ec061a9..6531c0d6ba 100644 --- a/rust/tests/e2e/inprocess.rs +++ b/rust/tests/e2e/inprocess.rs @@ -70,4 +70,3 @@ async fn should_force_stop_inprocess_client_within_bounded_time() { ) .await; } - From f3b7896ab90692355c01f83d1b8062d206ad1f4d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 11:03:48 +0000 Subject: [PATCH 3/4] 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 | 77 ++++++++++++++++++++-------- 2 files changed, 75 insertions(+), 22 deletions(-) diff --git a/.github/workflows/rust-sdk-tests.yml b/.github/workflows/rust-sdk-tests.yml index 26cc3d92da..66a345d88f 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 4ec3569833..c2ac1ddc91 100644 --- a/dotnet/src/FfiRuntimeHost.cs +++ b/dotnet/src/FfiRuntimeHost.cs @@ -57,6 +57,26 @@ internal sealed partial class FfiRuntimeHost : IDisposable /// private static readonly TimeSpan s_hostShutdownTimeout = TimeSpan.FromSeconds(10); + /// + /// 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; @@ -122,30 +142,41 @@ internal static string GetRuntimeLibraryFileName() /// public async Task StartAsync(CancellationToken cancellationToken) { - // Keep synchronous native startup off the caller's async context. - await Task.Run(() => + // 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 { - var argvJson = BuildArgvJson(_cliEntrypoint, _args); - var envJson = BuildEnvJson(_environment); - - _serverId = NativeHostStart(argvJson, envJson); - if (_serverId == 0) + // 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); - _connectionId = NativeOpenConnection(_serverId); - if (_connectionId == 0) - { - DisposeNativeCallback(); - NativeHostShutdown(_serverId); - _serverId = 0; - throw new InvalidOperationException("copilot_runtime_connection_open failed."); - } + _serverId = NativeHostStart(argvJson, envJson); + if (_serverId == 0) + { + throw new InvalidOperationException( + $"copilot_runtime_host_start failed (library '{_libraryPath}')."); + } + + _connectionId = NativeOpenConnection(_serverId); + if (_connectionId == 0) + { + DisposeNativeCallback(); + NativeHostShutdown(_serverId); + _serverId = 0; + throw new InvalidOperationException("copilot_runtime_connection_open failed."); + } - _sendStream = new CallbackSendStream(SendFrame); - }, cancellationToken).ConfigureAwait(false); + _sendStream = new CallbackSendStream(SendFrame); + }, cancellationToken).ConfigureAwait(false); + } + finally + { + s_nativeLifecycleGate.Release(); + } if (_logger.IsEnabled(LogLevel.Debug)) { @@ -264,6 +295,11 @@ public void Dispose() // when this method stops waiting early. 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 { NativeHostShutdown(serverId); @@ -275,6 +311,7 @@ public void Dispose() finally { DisposeNativeCallback(); + s_nativeLifecycleGate.Release(); } }); From cc6e7d1a9236f2998be92a0143fa533cf0cc1a8f Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 4 Sep 2026 11:30:52 +0000 Subject: [PATCH 4/4] 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 ce79f984a3..b027c83aa5 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 66a345d88f..453ac9b086 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