diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index 9b7216ad20..b027c83aa5 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -56,20 +56,26 @@ 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 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 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 +188,15 @@ jobs: 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 828459126b..453ac9b086 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 @@ -206,7 +213,20 @@ jobs: strategy: fail-fast: false matrix: - # TODO: Re-enable Windows after fixing the napi-oop peer shutdown crash. + # 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 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 }} timeout-minutes: 20 diff --git a/dotnet/src/FfiRuntimeHost.cs b/dotnet/src/FfiRuntimeHost.cs index cc3bccae92..c2ac1ddc91 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,44 @@ 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); + + /// + /// 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; @@ -103,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}')."); + } - _sendStream = new CallbackSendStream(SendFrame); - }, cancellationToken).ConfigureAwait(false); + _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); + } + finally + { + s_nativeLifecycleGate.Release(); + } if (_logger.IsEnabled(LogLevel.Debug)) { @@ -225,21 +275,64 @@ public void Dispose() _logger.LogDebug(ex, "FfiRuntimeHost: connection_close failed"); } - try + _receiveStream.Complete(); + + var serverId = _serverId; + _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(() => { - if (_serverId != 0) + // 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); - _serverId = 0; + NativeHostShutdown(serverId); } + catch (Exception ex) + { + _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); + } + finally + { + DisposeNativeCallback(); + s_nativeLifecycleGate.Release(); + } + }); + + 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..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; +}