Skip to content

Commit 15439b3

Browse files
stephentoubCopilot
andauthored
Clean up more argument validation (#1328)
* Fix .NET session lifetime rooting Keep CopilotSession instances rooted by the owning client until explicit cleanup, route generated session RPC APIs through CopilotSession, and add generated argument validation plus lifetime regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix .NET same-client session resume Allow a resumed session to replace the client's current session entry so same-client resumes continue to work while stale session disposal cannot remove the replacement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address .NET session cleanup review Restore StopAsync cleanup error propagation, clear any sessions missed by concurrent registration during stop, and remove an unused Session using. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Keep sessions rooted during destroy Delay removing sessions from the client until session.destroy completes so destroy-time callbacks such as session_fs can still route to the session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Relax .NET mode handler E2E timeout Give mode handler callbacks the same 30 second allowance as their corresponding events to avoid Windows timing failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Restore active session resume guard Reject same-client resumes for session ids already tracked by the client, and start session event processing only after successful registration so failed duplicate registration does not leave an untracked event loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix .NET resume E2E coverage Update resume-focused E2E tests to use an active session from a second TCP client, while keeping same-client active resume covered as an intentional rejection. Clean up permission resume tests so the resumed handler is the only active handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Normalize read_agent timings in replay snapshots Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3b5823b commit 15439b3

15 files changed

Lines changed: 1399 additions & 485 deletions

dotnet/src/Client.cs

Lines changed: 111 additions & 70 deletions
Large diffs are not rendered by default.

dotnet/src/Generated/Rpc.cs

Lines changed: 501 additions & 304 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dotnet/src/Polyfills/DownlevelExtensions.cs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,20 @@ public static void ThrowIfNull(object? argument, [CallerArgumentExpression(nameo
2525
}
2626
}
2727

28+
internal static class DownlevelObjectDisposedExceptionExtensions
29+
{
30+
extension(ObjectDisposedException)
31+
{
32+
public static void ThrowIf(bool condition, object instance)
33+
{
34+
if (condition)
35+
{
36+
throw new ObjectDisposedException(instance?.GetType().FullName);
37+
}
38+
}
39+
}
40+
}
41+
2842
internal static class DownlevelArgumentExceptionExtensions
2943
{
3044
extension(ArgumentException)
@@ -531,6 +545,20 @@ private sealed record SocketConnectState(Socket Socket, TaskCompletionSource<obj
531545
}
532546
}
533547

548+
namespace System.Runtime.ExceptionServices
549+
{
550+
internal static class DownlevelExceptionDispatchInfoExtensions
551+
{
552+
extension(ExceptionDispatchInfo)
553+
{
554+
public static void Throw(Exception exception)
555+
{
556+
ExceptionDispatchInfo.Capture(exception).Throw();
557+
}
558+
}
559+
}
560+
}
561+
534562
namespace System.Runtime.InteropServices
535563
{
536564
internal static class DownlevelRuntimeInformationExtensions

dotnet/src/Session.cs

Lines changed: 125 additions & 52 deletions
Large diffs are not rendered by default.

dotnet/test/E2E/ClientE2ETests.cs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,14 +189,27 @@ public async Task Should_Allow_CreateSession_Called_Without_PermissionHandler(bo
189189
Assert.NotNull(session.SessionId);
190190
}
191191

192-
[Theory]
193-
[InlineData(true)] // stdio transport
194-
[InlineData(false)] // TCP transport
195-
public async Task Should_Allow_ResumeSession_Called_Without_PermissionHandler(bool useStdio)
192+
[Fact]
193+
public async Task Should_Allow_ResumeSession_Called_Without_PermissionHandler()
196194
{
197-
await using var client = new CopilotClient(new CopilotClientOptions { UseStdio = useStdio });
195+
const string connectionToken = "client-e2e-resume-token";
196+
197+
await using var client = new CopilotClient(new CopilotClientOptions
198+
{
199+
UseStdio = false,
200+
TcpConnectionToken = connectionToken,
201+
});
198202
await using var originalSession = await client.CreateSessionAsync(new SessionConfig());
199-
await using var resumedSession = await client.ResumeSessionAsync(originalSession.SessionId, new());
203+
204+
var port = client.ActualPort
205+
?? throw new InvalidOperationException("Client must be using TCP transport to support multi-client resume.");
206+
207+
await using var resumeClient = new CopilotClient(new CopilotClientOptions
208+
{
209+
CliUrl = $"localhost:{port}",
210+
TcpConnectionToken = connectionToken,
211+
});
212+
await using var resumedSession = await resumeClient.ResumeSessionAsync(originalSession.SessionId, new());
200213

201214
Assert.Equal(originalSession.SessionId, resumedSession.SessionId);
202215
}

dotnet/test/E2E/ModeHandlersE2ETests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public async Task Should_Invoke_Exit_Plan_Mode_Handler_When_Model_Uses_Tool()
5757
Prompt = "Create a brief implementation plan for adding a greeting.txt file, then request approval with exit_plan_mode.",
5858
}, timeout: TimeSpan.FromSeconds(120));
5959

60-
var (request, invocation) = await handlerTask.Task.WaitAsync(TimeSpan.FromSeconds(10));
60+
var (request, invocation) = await handlerTask.Task.WaitAsync(TimeSpan.FromSeconds(30));
6161
Assert.Equal(session.SessionId, invocation.SessionId);
6262
Assert.Equal(summary, request.Summary);
6363
Assert.Equal(["interactive", "autopilot", "exit_only"], request.Actions);
@@ -124,7 +124,7 @@ public async Task Should_Invoke_Auto_Mode_Switch_Handler_When_Rate_Limited()
124124
});
125125
Assert.NotEmpty(messageId);
126126

127-
var (request, invocation) = await handlerTask.Task.WaitAsync(TimeSpan.FromSeconds(10));
127+
var (request, invocation) = await handlerTask.Task.WaitAsync(TimeSpan.FromSeconds(30));
128128
Assert.Equal(session.SessionId, invocation.SessionId);
129129
Assert.Equal("user_weekly_rate_limited", request.ErrorCode);
130130
Assert.Equal(1, request.RetryAfterSeconds);

dotnet/test/E2E/PermissionE2ETests.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,10 @@ public async Task Should_Resume_Session_With_Permission_Handler()
204204
var session1 = await CreateSessionAsync();
205205
var sessionId = session1.SessionId;
206206
await session1.SendAndWaitAsync(new MessageOptions { Prompt = "What is 1+1?" });
207+
await session1.DisposeAsync();
207208

208209
// Resume with permission handler
209-
var session2 = await ResumeSessionAsync(sessionId, new ResumeSessionConfig
210+
var session2 = await Client.ResumeSessionAsync(sessionId, new ResumeSessionConfig
210211
{
211212
OnPermissionRequest = (request, invocation) =>
212213
{
@@ -221,6 +222,7 @@ await session2.SendAndWaitAsync(new MessageOptions
221222
});
222223

223224
Assert.True(permissionRequestReceived, "Permission request should have been received");
225+
await session2.DisposeAsync();
224226
}
225227

226228
[Fact]
@@ -255,8 +257,9 @@ public async Task Should_Deny_Tool_Operations_When_Handler_Explicitly_Denies_Aft
255257
});
256258
var sessionId = session1.SessionId;
257259
await session1.SendAndWaitAsync(new MessageOptions { Prompt = "What is 1+1?" });
260+
await session1.DisposeAsync();
258261

259-
var session2 = await ResumeSessionAsync(sessionId, new ResumeSessionConfig
262+
var session2 = await Client.ResumeSessionAsync(sessionId, new ResumeSessionConfig
260263
{
261264
OnPermissionRequest = (_, _) =>
262265
Task.FromResult(new PermissionRequestResult { Kind = PermissionRequestResultKind.UserNotAvailable })
@@ -279,6 +282,7 @@ await session2.SendAndWaitAsync(new MessageOptions
279282
});
280283

281284
Assert.True(permissionDenied, "Expected a tool.execution_complete event with Permission denied result");
285+
await session2.DisposeAsync();
282286
}
283287

284288
[Fact]

dotnet/test/E2E/SessionE2ETests.cs

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,7 @@ public async Task ShouldCreateAndDisconnectSessions()
2828

2929
await session.DisposeAsync();
3030

31-
var ex = await Assert.ThrowsAsync<IOException>(() => session.GetMessagesAsync());
32-
Assert.Contains("not found", ex.Message, StringComparison.OrdinalIgnoreCase);
31+
await Assert.ThrowsAsync<ObjectDisposedException>(() => session.GetMessagesAsync());
3332
}
3433

3534
[Fact]
@@ -212,27 +211,17 @@ public async Task Should_Create_Session_With_Custom_Tool()
212211
}
213212

214213
[Fact]
215-
public async Task Should_Resume_A_Session_Using_The_Same_Client()
214+
public async Task Should_Reject_Resuming_Active_Session_Using_The_Same_Client()
216215
{
217216
var session1 = await CreateSessionAsync();
218217
var sessionId = session1.SessionId;
219218

220-
await session1.SendAsync(new MessageOptions { Prompt = "What is 1+1?" });
221-
var answer = await TestHelper.GetFinalAssistantMessageAsync(session1);
222-
Assert.NotNull(answer);
223-
Assert.Contains("2", answer!.Data.Content ?? string.Empty);
224-
225-
var session2 = await ResumeSessionAsync(sessionId);
226-
Assert.Equal(sessionId, session2.SessionId);
227-
228-
var answer2 = await TestHelper.GetFinalAssistantMessageAsync(session2, alreadyIdle: true);
229-
Assert.NotNull(answer2);
230-
Assert.Contains("2", answer2!.Data.Content ?? string.Empty);
231-
232-
// Can continue the conversation statefully
233-
var answer3 = await session2.SendAndWaitAsync(new MessageOptions { Prompt = "Now if you double that, what do you get?" });
234-
Assert.NotNull(answer3);
235-
Assert.Contains("4", answer3!.Data.Content ?? string.Empty);
219+
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() =>
220+
Client.ResumeSessionAsync(sessionId, new ResumeSessionConfig
221+
{
222+
OnPermissionRequest = PermissionHandler.ApproveAll,
223+
}));
224+
Assert.Contains(sessionId, exception.Message);
236225
}
237226

238227
[Fact]

dotnet/test/E2E/SessionMcpAndAgentConfigE2ETests.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ public async Task Should_Accept_MCP_Server_Configuration_On_Session_Resume()
4848
var session1 = await CreateSessionAsync();
4949
var sessionId = session1.SessionId;
5050
await session1.SendAndWaitAsync(new MessageOptions { Prompt = "What is 1+1?" });
51+
await session1.DisposeAsync();
5152

5253
// Resume with MCP servers
5354
var mcpServers = new Dictionary<string, McpServerConfig>
@@ -141,6 +142,7 @@ public async Task Should_Accept_Custom_Agent_Configuration_On_Session_Resume()
141142
var session1 = await CreateSessionAsync();
142143
var sessionId = session1.SessionId;
143144
await session1.SendAndWaitAsync(new MessageOptions { Prompt = "What is 1+1?" });
145+
await session1.DisposeAsync();
144146

145147
// Resume with custom agents
146148
var customAgents = new List<CustomAgentConfig>

dotnet/test/Harness/E2ETestBase.cs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,21 @@ protected Task<CopilotSession> CreateSessionAsync(SessionConfig? config = null)
8282
/// Resumes a session with a default config that approves all permissions.
8383
/// Convenience wrapper for E2E tests.
8484
/// </summary>
85-
protected Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSessionConfig? config = null)
85+
protected async Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSessionConfig? config = null)
8686
{
8787
config ??= new ResumeSessionConfig();
8888
config.OnPermissionRequest ??= PermissionHandler.ApproveAll;
89-
return Client.ResumeSessionAsync(sessionId, config);
89+
90+
await Client.StartAsync();
91+
var port = Client.ActualPort
92+
?? throw new InvalidOperationException("The shared E2E client must use TCP transport to support multi-client resume.");
93+
94+
var client = Ctx.CreateClient(options: new CopilotClientOptions
95+
{
96+
CliUrl = $"localhost:{port}",
97+
TcpConnectionToken = E2ETestFixture.SharedTcpConnectionToken,
98+
});
99+
return await client.ResumeSessionAsync(sessionId, config);
90100
}
91101

92102
protected static string GetSystemMessage(ParsedHttpExchange exchange)

0 commit comments

Comments
 (0)