-
Notifications
You must be signed in to change notification settings - Fork 1.3k
HTTP request callback support #1689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e2f57e0
9f854f8
8e305b5
e18edeb
6bcfd60
6457cdc
d3eb4f5
b95e6fa
a5366e7
510ce96
c241268
cb814e1
8d5343b
5325bd4
f99182f
c70adeb
7484511
69a7f05
c9d5b40
f74d396
d44886c
f00fb0e
a4ca676
82e7c9a
d653322
2c100ff
692eb2c
328e715
da91eaf
a607520
979d27a
bd9379c
cc0ce37
8d35395
4b9c8e6
ed7b121
29de529
072d7af
b0ac997
c74e538
32f4164
7d425ff
eeea347
171bb96
a5e7373
6180a8a
04970d5
dd79d3a
130d119
eb0dafe
e120edb
f086ca0
2cbec1f
d05e86c
549b83b
6551152
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,13 @@ public sealed partial class CopilotClient : IDisposable, IAsyncDisposable | |
| private List<ModelInfo>? _modelsCache; | ||
| private ServerRpc? _serverRpc; | ||
|
|
||
| /// <summary> | ||
| /// Client-global RPC handlers (e.g. the LLM inference provider adapter), | ||
| /// built once at construction when the corresponding option is configured and | ||
| /// registered on every connection. Null when no client-global API is enabled. | ||
| /// </summary> | ||
| private readonly ClientGlobalApiHandlers? _clientGlobalApis; | ||
|
|
||
| private sealed record LifecycleSubscription(Type EventType, Action<SessionLifecycleEvent> Handler); | ||
|
|
||
| /// <summary> | ||
|
|
@@ -165,6 +172,8 @@ public CopilotClient(CopilotClientOptions? options = null) | |
| _logger = _options.Logger ?? NullLogger.Instance; | ||
| _onListModels = _options.OnListModels; | ||
|
|
||
| _clientGlobalApis = BuildClientGlobalApis(); | ||
|
|
||
| // Empty mode: validate at construction time that the app supplied a | ||
| // per-session persistence location. The runtime is mode-agnostic, so | ||
| // without this check it would silently fall back to ~/.copilot, which | ||
|
|
@@ -276,6 +285,8 @@ async Task<Connection> StartCoreAsync(CancellationToken ct) | |
| sessionFsTimestamp); | ||
| } | ||
|
|
||
| await ConfigureLlmInferenceAsync(ct); | ||
|
|
||
| LoggingHelpers.LogTiming(_logger, LogLevel.Debug, null, | ||
| "CopilotClient.StartAsync complete. Elapsed={Elapsed}", | ||
| startTimestamp); | ||
|
|
@@ -426,15 +437,13 @@ private async Task CleanupConnectionAsync(List<Exception>? errors, bool graceful | |
|
|
||
| private async Task CleanupConnectionAsync(Connection ctx, List<Exception>? errors, bool gracefulRuntimeShutdown) | ||
| { | ||
| var runtimeShutdownCompleted = false; | ||
| if (gracefulRuntimeShutdown && ctx.CliProcess is not null) | ||
| { | ||
| var runtimeShutdownTimestamp = Stopwatch.GetTimestamp(); | ||
| try | ||
| { | ||
| using var cancellation = new CancellationTokenSource(s_runtimeShutdownTimeout); | ||
| await ctx.Server.Runtime.ShutdownAsync(cancellation.Token); | ||
| runtimeShutdownCompleted = true; | ||
| LoggingHelpers.LogTiming(_logger, LogLevel.Debug, null, | ||
| "CopilotClient.StopAsync runtime shutdown complete. Elapsed={Elapsed}", | ||
| runtimeShutdownTimestamp); | ||
|
|
@@ -466,42 +475,24 @@ or IOException | |
|
|
||
| if (ctx.CliProcess is { } childProcess) | ||
| { | ||
| await CleanupCliProcessAsync(childProcess, ctx.StderrPump, errors, _logger, runtimeShutdownCompleted); | ||
| await CleanupCliProcessAsync(childProcess, ctx.StderrPump, errors, _logger); | ||
| } | ||
| } | ||
|
|
||
| private static async Task CleanupCliProcessAsync(Process childProcess, ProcessStderrPump? stderrPump, List<Exception>? errors, ILogger? logger, bool waitForGracefulExit = false) | ||
| private static async Task CleanupCliProcessAsync(Process childProcess, ProcessStderrPump? stderrPump, List<Exception>? errors, ILogger? logger) | ||
| { | ||
| stderrPump?.Cancel(); | ||
|
|
||
| try | ||
| { | ||
| if (!childProcess.HasExited) | ||
| { | ||
| if (waitForGracefulExit) | ||
| { | ||
| var shutdownWaitTimestamp = Stopwatch.GetTimestamp(); | ||
| try | ||
| { | ||
| await childProcess.WaitForExitAsync().WaitAsync(s_runtimeShutdownTimeout); | ||
| } | ||
| catch (TimeoutException ex) | ||
| { | ||
| if (logger is not null) | ||
| { | ||
| LoggingHelpers.LogTiming(logger, LogLevel.Debug, ex, | ||
| "Timed out waiting for runtime process to exit after graceful shutdown. Elapsed={Elapsed}, Timeout={Timeout}", | ||
| shutdownWaitTimestamp, | ||
| s_runtimeShutdownTimeout); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (childProcess.HasExited) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // The runtime completes all cleanup before responding to | ||
| // runtime.shutdown and then leaves termination to us; it | ||
| // deliberately keeps its JSON-RPC server alive to send the | ||
| // response and never self-exits. Waiting for a self-exit that | ||
| // will never come just wastes time, so terminate the child | ||
| // immediately and only wait to reap it. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the argument is that in the graceful case it will have already cleaned up and in the non-graceful case we shouldn't wait anyway?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline - we confirmed the new force shutdown logic is needed. |
||
| childProcess.Kill(entireProcessTree: true); | ||
| // Kill is asynchronous; wait for the root CLI process to exit so cleanup callers | ||
| // do not observe StopAsync/DisposeAsync completion while it is still tearing down. | ||
|
|
@@ -1678,6 +1669,39 @@ await Rpc.SessionFs.SetProviderAsync( | |
| cancellationToken: cancellationToken); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Builds the client-global RPC handler bag at construction time. Currently | ||
| /// only the LLM inference provider adapter is registered; returns null when no | ||
| /// client-global API is configured so the registration is skipped entirely. | ||
| /// </summary> | ||
| private ClientGlobalApiHandlers? BuildClientGlobalApis() | ||
| { | ||
| var handler = _options.RequestHandler; | ||
| if (handler is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new ClientGlobalApiHandlers | ||
| { | ||
| LlmInference = new LlmInferenceAdapter(handler, () => _serverRpc), | ||
| }; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tells the runtime to route its outbound model-layer requests through this | ||
| /// client's LLM inference provider. No-op when interception is not configured. | ||
| /// </summary> | ||
| private async Task ConfigureLlmInferenceAsync(CancellationToken cancellationToken) | ||
| { | ||
| if (_clientGlobalApis?.LlmInference is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await Rpc.LlmInference.SetProviderAsync(cancellationToken); | ||
| } | ||
|
|
||
| private void ConfigureSessionFsHandlers(CopilotSession session, Func<CopilotSession, SessionFsProvider>? createSessionFsHandler) | ||
| { | ||
| if (_options.SessionFs is null) | ||
|
|
@@ -2072,6 +2096,10 @@ private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string? | |
| var session = GetSession(sessionId) ?? throw new ArgumentException($"Unknown session {sessionId}"); | ||
| return session.ClientSessionApis; | ||
| }); | ||
| if (_clientGlobalApis is not null) | ||
| { | ||
| ClientGlobalApiRegistration.RegisterClientGlobalApiHandlers(rpc, _clientGlobalApis); | ||
| } | ||
| rpc.StartListening(); | ||
| LoggingHelpers.LogTiming(_logger, LogLevel.Debug, null, | ||
| "CopilotClient.ConnectToServerAsync transport setup complete. Elapsed={Elapsed}", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not understanding why all this runtimeShutdownCompleted related code was removed. ShutdownAsync can throw an exception, which is then logged and eaten by the corresponding catch block. So we can be in CleanupCliProcessAsync even if shutdown async didn't complete successfully. What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline - we confirmed the new force shutdown logic is needed.