dotnet: in-process FFI runtime hosting (InProcess transport)#1901
Open
SteveSandersonMS wants to merge 12 commits into
Open
dotnet: in-process FFI runtime hosting (InProcess transport)#1901SteveSandersonMS wants to merge 12 commits into
SteveSandersonMS wants to merge 12 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (ctx.FfiHost is { } ffiHost) | ||
| { | ||
| try { ffiHost.Dispose(); } | ||
| catch (Exception ex) { AddCleanupError(errors, ex, _logger); } |
| ?? throw new InvalidOperationException($"Could not determine directory for '{cliEntrypoint}'."); | ||
|
|
||
| // Bundled .NET layout: flat, natural shared-library name next to the CLI. | ||
| var flatLibraryPath = Path.Combine(distDir, GetRuntimeLibraryFileName()); |
| // Bundled .NET layout: flat, natural shared-library name next to the CLI. | ||
| var flatLibraryPath = Path.Combine(distDir, GetRuntimeLibraryFileName()); | ||
| // Dev/tarball layout: dist-cli/prebuilds/<node-platform>-<arch>/runtime.node. | ||
| var prebuildsLibraryPath = Path.Combine(distDir, "prebuilds", prebuildsFolder, "runtime.node"); |
Comment on lines
+227
to
+230
| catch (Exception ex) | ||
| { | ||
| _logger.LogDebug(ex, "FfiRuntimeHost: connection_close failed"); | ||
| } |
Comment on lines
+240
to
+243
| catch (Exception ex) | ||
| { | ||
| _logger.LogDebug(ex, "FfiRuntimeHost: host_shutdown failed"); | ||
| } |
| public static IntPtr Sym(IntPtr handle, string name) | ||
| { | ||
| try { return Libdl2.dlsym(handle, name); } | ||
| catch (DllNotFoundException) { return Libdl1.dlsym(handle, name); } |
| private static class Libdl2 | ||
| { | ||
| [DllImport("libdl.so.2", EntryPoint = "dlopen", CharSet = CharSet.Ansi, BestFitMapping = false, ThrowOnUnmappableChar = true)] | ||
| public static extern IntPtr dlopen([MarshalAs(UnmanagedType.LPStr)] string fileName, int flags); |
| public static extern IntPtr dlopen([MarshalAs(UnmanagedType.LPStr)] string fileName, int flags); | ||
|
|
||
| [DllImport("libdl.so.2", EntryPoint = "dlsym", CharSet = CharSet.Ansi, BestFitMapping = false, ThrowOnUnmappableChar = true)] | ||
| public static extern IntPtr dlsym(IntPtr handle, [MarshalAs(UnmanagedType.LPStr)] string symbol); |
| private static class Libdl1 | ||
| { | ||
| [DllImport("libdl", EntryPoint = "dlopen", CharSet = CharSet.Ansi, BestFitMapping = false, ThrowOnUnmappableChar = true)] | ||
| public static extern IntPtr dlopen([MarshalAs(UnmanagedType.LPStr)] string fileName, int flags); |
| public static extern IntPtr dlopen([MarshalAs(UnmanagedType.LPStr)] string fileName, int flags); | ||
|
|
||
| [DllImport("libdl", EntryPoint = "dlsym", CharSet = CharSet.Ansi, BestFitMapping = false, ThrowOnUnmappableChar = true)] | ||
| public static extern IntPtr dlsym(IntPtr handle, [MarshalAs(UnmanagedType.LPStr)] string symbol); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in InProcess transport to the .NET SDK that hosts the Copilot runtime in-process via a native shared library and drives JSON-RPC across an FFI (C ABI) boundary, instead of spawning the CLI and using stdio/TCP.
Changes:
- Introduces
InProcessRuntimeConnectionand default-connection selection viaCOPILOT_SDK_DEFAULT_CONNECTION. - Adds
FfiRuntimeHostto load the native library, start the embedded host, and bridge JSON-RPC I/O via callback-backed streams. - Updates JSON-RPC framing to write header+body in a single write (important for FFI boundary crossings) and adds an E2E test.
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/E2E/ClientE2ETests.cs | Adds an E2E test for starting/connecting over the new in-process FFI transport. |
| dotnet/src/Types.cs | Adds InProcessRuntimeConnection and RuntimeConnection.ForInProcess() API surface and docs. |
| dotnet/src/JsonRpc.cs | Changes outgoing message framing to build a single buffer for header+JSON body. |
| dotnet/src/GitHub.Copilot.SDK.csproj | Enables unsafe blocks (needed for function pointers / fixed buffers in FFI host). |
| dotnet/src/FfiRuntimeHost.cs | New in-process FFI host implementation (library resolution, start/open/close, callback streams). |
| dotnet/src/Client.cs | Wires InProcessRuntimeConnection into client startup/cleanup; adds default-connection env var logic and FFI CLI path resolution. |
| dotnet/src/build/GitHub.Copilot.SDK.targets | Copies/renames the runtime cdylib next to the bundled CLI in runtimes/<rid>/native/ when present. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 11
- Review effort level: Low
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add a new InProcess transport that loads the runtime cdylib (runtime.node)
in-process and speaks JSON-RPC over its C ABI instead of spawning a CLI
subprocess and talking over stdio/TCP. The Rust host_start call spawns the
residual Node worker (napi-oop) itself; the C# side only drives opaque
JSON-RPC blobs over the FFI boundary.
- FfiRuntimeHost.cs: shared host body (library resolution, host_start/
connection_open handshake, duplex Channel-backed streams bridging the native
outbound callback to the SDK's JsonRpc framing) with two interop backends:
* net8.0/net10.0: source-generated [LibraryImport] P/Invoke with a
DllImportResolver mapping the logical library name to the runtime.node
absolute path and an [UnmanagedCallersOnly] cdecl function-pointer callback
routed via a GCHandle. Trim/NativeAOT-compatible (IsAotCompatible).
* netstandard2.0: classic delegate-based P/Invoke over a hand-rolled
dlopen/LoadLibrary loader (netstandard2.0 has neither LibraryImport,
NativeLibrary, nor UnmanagedCallersOnly), with the outbound callback held as
an instance delegate for the connection's lifetime.
- Types.cs: InProcessRuntimeConnection + RuntimeConnection.ForInProcess(path, args).
- Client.cs: select the FFI transport for InProcessRuntimeConnection (or the
COPILOT_SDK_FFI_HOST override), send a null connection token in FFI mode.
- csproj: AllowUnsafeBlocks for the function-pointer callback.
- ClientE2ETests: Should_Start_And_Connect_Over_InProcess_Ffi.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e-bin The in-process FFI transport no longer resolves or passes a `copilot-runtime-bin` provider path. The child manifest is delivered over the napi-oop handshake, so host_start only needs the CLI entrypoint and env. - FfiRuntimeHost: drop the provider field/param and the provider argument from the C ABI bindings (modern LibraryImport + netstandard2.0 delegate); accept a binary-or-.js entrypoint. A .js entrypoint is launched via `node` (dev/dist-cli); the packaged single-file CLI binary is invoked directly as `copilot --embedded-host` (it embeds its own Node). - Client: FFI path resolution falls back to the bundled CLI (runtimes/<rid>/native/copilot) the same way stdio discovers it, still overridable via COPILOT_CLI_PATH. Renamed ResolveCliJsPathForFfi -> ResolveCliPathForFfi. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The in-process FFI host loads the Rust cdylib in the .NET process, so the build must emit it next to the CLI binary. The MSBuild targets copy the tarball's napi-style prebuilds/<npm-platform>/runtime.node to a flat, natural shared-library name in runtimes/<rid>/native/: win -> copilot_runtime.dll linux -> libcopilot_runtime.so osx -> libcopilot_runtime.dylib (these are what the Rust cdylib would be called if napi didn't rename it to .node; the .NET host loads it by absolute path, so the name is ours). This avoids shipping a doubly-non-obvious prebuilds/<node-platform>/runtime.node tree in .NET output. Emission is conditioned on the source existing, so stdio-only consumers on tarballs without the cdylib are unaffected. FfiRuntimeHost resolves the cdylib relative to the entrypoint, preferring the flat renamed sibling and falling back to the dev dist-cli/prebuilds/<node-platform>-<arch>/runtime.node layout (for COPILOT_CLI_PATH overrides). The prebuilds folder uses the napi-rs <node-platform>-<arch> convention (win32-x64/darwin-x64/linux-x64), not the .NET RID (win-x64/osx-x64) which only matched on Linux; Client now computes this via GetNapiPrebuildsFolder. Validated: FFI E2E passes against a locally-built SEA copilot binary using both the flat renamed libcopilot_runtime.so and the dev prebuilds fallback; the targets emit copilot + libcopilot_runtime.so in runtimes/<rid>/native/ and skip the cdylib gracefully when absent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The native connection_write copies its bytes synchronously, so the C# side need not own the buffer past the call. Thread frames through as ReadOnlySpan instead of allocating+copying a byte[] per write (LibraryImport marshals the span pointer; the netstandard2.0 delegate pins it). Also coalesce the LSP Content-Length header and JSON body into one pooled buffer so each message is a single write — one native boundary crossing instead of two. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BuildFrame stackalloc'd a header scratch then copied it into the rented frame. Over-rent by the fixed <=30B header bound instead and write the header directly into the frame, dropping the scratch buffer and header copy. The single JSON copy remains (its length must precede it as Content-Length). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a transport matrix axis (default + inprocess) to the .NET SDK test workflow so win/macos/linux each run the suite with COPILOT_SDK_DEFAULT_CONNECTION=inprocess as well as the existing default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Set the env var via a guarded step writing to GITHUB_ENV so the default transport cells leave it entirely unset rather than set to empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix XML docs claiming InProcess is .NET 8+ only; it works on netstandard2.0 too via the fallback native loader. - Correct the stale bundled-cdylib layout comment in Client to match FfiRuntimeHost.Create's flat-name-first resolution. - Run the inproc FFI E2E test unconditionally (fail hard) instead of silently returning when COPILOT_CLI_PATH is unset. - Throw when the native connection_write reports failure instead of silently succeeding. - Retry in CallbackReceiveStream rather than reporting a spurious EOF when a signalled read loses the race. - Fail fast when a second, different FFI library path is requested in the same process (both interop paths). - Handle Utf8.TryWrite failure in JsonRpc.BuildFrame explicitly and return the rented buffer instead of a Debug-only assert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The published 1.0.69-2 runtime.node exports copilot_runtime_host_start and copilot_runtime_host_shutdown, so the in-process FFI E2E test now passes against the auto-bundled cdylib without needing COPILOT_CLI_PATH. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/ffi-inproc-host
branch
from
365bea0 to
d2eb6d9
Compare
Runtime 1.0.69-2 drops the internal CLI-only session.mcp.oauth.respond API; regenerated Rpc.cs no longer emits McpOauthRespondRequest/Result or McpOauthApi.RespondAsync. All internal, no public surface change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Consistent across TS, Python, Go, Rust (C# was already committed): - Drop the internal CLI-only session.mcp.oauth.respond API/types. - Widen session-event timeToFirstTokenMs from integer to fractional milliseconds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The job name referenced only matrix.transport, which suppresses GitHub's automatic append of the remaining matrix values, so the three OS cells per transport were indistinguishable in the checks list. Name them by both os and transport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency ReviewThe .NET-specific InProcess FFI transport is correctly .NET-only — no cross-SDK concerns there. However, this PR bumps 1.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In-process FFI runtime hosting for the .NET SDK
Adds a new InProcess transport to the .NET SDK. Instead of spawning the CLI as a subprocess and talking JSON-RPC over stdio/TCP, the SDK loads the runtime's native shared library in-process and drives JSON-RPC over its C ABI. The native host spawns the residual Node worker itself; the C# side only pumps opaque JSON-RPC blobs across the FFI boundary.
What's here
FfiRuntimeHost.cs— shared host body (native library resolution,host_start/connection_openhandshake, duplexChannel-backed streams bridging the native outbound callback to the SDK's JSON-RPC framing) with two interop backends:[LibraryImport]P/Invoke with aDllImportResolvermapping the logical library name to the absolute path of the native lib, and an[UnmanagedCallersOnly]cdecl function-pointer callback routed via aGCHandle. Trim/NativeAOT-compatible (IsAotCompatible).dlopen/LoadLibraryloader (netstandard2.0 has neitherLibraryImport,NativeLibrary, norUnmanagedCallersOnly).Types.cs—InProcessRuntimeConnection+RuntimeConnection.ForInProcess(path, args).Client.cs— selects the FFI transport forInProcessRuntimeConnection(selected explicitly, or via theCOPILOT_SDK_DEFAULT_CONNECTION=inprocessdefault-transport override); resolves the entrypoint fromCOPILOT_CLI_PATHand falls back to the bundled CLI the same way stdio discovers it.MSBuild targets — emit the native shared library next to the bundled CLI as a natural, platform-obvious name in
runtimes/<rid>/native/:copilot_runtime.dlllibcopilot_runtime.solibcopilot_runtime.dylibThe .NET host loads it by absolute path, so the filename is ours. Emission is conditioned on the source existing, so stdio-only consumers are unaffected.
ClientE2ETests—Should_Start_And_Connect_Over_InProcess_Ffi.Status — draft
This depends on native in-process-host support that ships in a future
@github/copilotCLI release. It should not be merged until the pinned CLI version is bumped to a build that ships the runtime shared library and the in-process host capability. Kept as a draft until then.The default stdio/TCP transports are unchanged; InProcess is strictly opt-in.