Skip to content

Commit e07ace1

Browse files
KSemenenkoKSemenenko
committed
Fix lifecycle and image cleanup leaks
1 parent 981d395 commit e07ace1

File tree

10 files changed

+488
-60
lines changed

10 files changed

+488
-60
lines changed

‎CodexSharpSDK.Tests/Unit/CodexClientTests.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using ManagedCode.CodexSharpSDK.Client;
22
using ManagedCode.CodexSharpSDK.Configuration;
3+
using ManagedCode.CodexSharpSDK.Execution;
34
using ManagedCode.CodexSharpSDK.Models;
45
using ManagedCode.CodexSharpSDK.Tests.Shared;
56

@@ -187,6 +188,24 @@ public async Task StopAsync_SetsDisconnectedState()
187188
await Assert.That(client.State).IsEqualTo(CodexClientState.Disconnected);
188189
}
189190

191+
[Test]
192+
public async Task StopAsync_CancelsActiveRun()
193+
{
194+
var runner = new BlockingProcessRunner();
195+
using var client = CreateClientWithRunner(runner);
196+
var thread = client.StartThread();
197+
198+
var runTask = thread.RunAsync("long-running");
199+
await runner.Started.Task.WaitAsync(TimeSpan.FromSeconds(2));
200+
201+
await client.StopAsync();
202+
203+
var exception = await Assert.That(async () => await runTask).ThrowsException();
204+
await Assert.That(exception).IsTypeOf<OperationCanceledException>();
205+
await runner.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(2));
206+
await Assert.That(client.State).IsEqualTo(CodexClientState.Disconnected);
207+
}
208+
190209
[Test]
191210
public async Task Dispose_SetsDisposedStateAndBlocksOperations()
192211
{
@@ -228,6 +247,24 @@ public async Task Dispose_CanBeCalledConcurrently()
228247
await Assert.That(client.State).IsEqualTo(CodexClientState.Disposed);
229248
}
230249

250+
[Test]
251+
public async Task Dispose_CancelsActiveRun()
252+
{
253+
var runner = new BlockingProcessRunner();
254+
var client = CreateClientWithRunner(runner);
255+
var thread = client.StartThread();
256+
257+
var runTask = thread.RunAsync("long-running");
258+
await runner.Started.Task.WaitAsync(TimeSpan.FromSeconds(2));
259+
260+
client.Dispose();
261+
262+
var exception = await Assert.That(async () => await runTask).ThrowsException();
263+
await Assert.That(exception).IsTypeOf<OperationCanceledException>();
264+
await runner.CancellationObserved.Task.WaitAsync(TimeSpan.FromSeconds(2));
265+
await Assert.That(client.State).IsEqualTo(CodexClientState.Disposed);
266+
}
267+
231268
[Test]
232269
public async Task CodexCli_Smoke_GetCliMetadata_ReturnsInstalledVersion()
233270
{
@@ -292,4 +329,44 @@ public async Task ResumeThread_WithThreadOptions_RunsWithRealCodexCli()
292329
await Assert.That(secondResult.Usage).IsNotNull();
293330
await Assert.That(resumedThread.Id).IsEqualTo(threadId);
294331
}
332+
333+
private static CodexClient CreateClientWithRunner(ICodexProcessRunner runner)
334+
{
335+
return new CodexClient(
336+
new CodexClientOptions
337+
{
338+
AutoStart = false,
339+
CodexOptions = new CodexOptions
340+
{
341+
CodexExecutablePath = "codex",
342+
},
343+
},
344+
new CodexExec("codex", null, null, runner));
345+
}
346+
347+
private sealed class BlockingProcessRunner : ICodexProcessRunner
348+
{
349+
public TaskCompletionSource<bool> Started { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously);
350+
351+
public TaskCompletionSource<bool> CancellationObserved { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously);
352+
353+
public async IAsyncEnumerable<string> RunAsync(
354+
CodexProcessInvocation invocation,
355+
Microsoft.Extensions.Logging.ILogger logger,
356+
[System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken)
357+
{
358+
Started.TrySetResult(true);
359+
yield return "{\"type\":\"thread.started\",\"thread_id\":\"thread_1\"}";
360+
361+
try
362+
{
363+
await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken).ConfigureAwait(false);
364+
}
365+
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
366+
{
367+
CancellationObserved.TrySetResult(true);
368+
throw;
369+
}
370+
}
371+
}
295372
}

‎CodexSharpSDK.Tests/Unit/CodexExecTests.cs‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Collections;
12
using System.Text.Json.Nodes;
23
using ManagedCode.CodexSharpSDK.Client;
34
using ManagedCode.CodexSharpSDK.Execution;
@@ -298,6 +299,107 @@ public async Task RunAsync_WithNullLogger_CompletesSuccessfully_WithRealCodexCli
298299
await Assert.That(lines.Any(line => line.Contains("\"type\":\"turn.completed\"", StringComparison.Ordinal))).IsTrue();
299300
}
300301

302+
[Test]
303+
public async Task DefaultProcessRunner_CancellationAfterProcessExit_StillReturnsProcessFailure()
304+
{
305+
var sandboxDirectory = CreateSandboxDirectory();
306+
307+
try
308+
{
309+
var shellScript = CreateShellScript(
310+
sandboxDirectory,
311+
"stderr-race",
312+
OperatingSystem.IsWindows()
313+
? """
314+
for /L %%i in (1,1,20000) do @echo error-line-%%i 1>&2
315+
echo done
316+
exit /b 23
317+
"""
318+
: """
319+
i=1
320+
while [ "$i" -le 20000 ]
321+
do
322+
echo "error-line-$i" 1>&2
323+
i=$((i + 1))
324+
done
325+
echo done
326+
exit 23
327+
""");
328+
var runner = new DefaultCodexProcessRunner();
329+
using var cancellation = new CancellationTokenSource();
330+
331+
await using var enumerator = runner.RunAsync(
332+
shellScript.Invocation,
333+
NullLogger.Instance,
334+
cancellation.Token)
335+
.GetAsyncEnumerator(cancellation.Token);
336+
337+
await Assert.That(await enumerator.MoveNextAsync()).IsTrue();
338+
await Assert.That(enumerator.Current).IsEqualTo("done");
339+
340+
cancellation.Cancel();
341+
342+
var action = async () => await enumerator.MoveNextAsync();
343+
var exception = await Assert.That(action).ThrowsException();
344+
345+
await Assert.That(exception).IsTypeOf<InvalidOperationException>();
346+
await Assert.That(exception!.Message).Contains("Codex Exec exited with code 23");
347+
await Assert.That(exception.Message).Contains("error-line-1");
348+
}
349+
finally
350+
{
351+
Directory.Delete(sandboxDirectory, recursive: true);
352+
}
353+
}
354+
355+
[Test]
356+
public async Task DefaultProcessRunner_CancellationWhileProcessStillRunning_ThrowsOperationCanceledException()
357+
{
358+
var sandboxDirectory = CreateSandboxDirectory();
359+
360+
try
361+
{
362+
var shellScript = CreateShellScript(
363+
sandboxDirectory,
364+
"stderr-cancel",
365+
OperatingSystem.IsWindows()
366+
? """
367+
echo started
368+
:loop
369+
echo error-line 1>&2
370+
goto loop
371+
"""
372+
: """
373+
echo started
374+
while :
375+
do
376+
echo error-line 1>&2
377+
done
378+
""");
379+
var runner = new DefaultCodexProcessRunner();
380+
using var cancellation = new CancellationTokenSource();
381+
382+
await using var enumerator = runner.RunAsync(
383+
shellScript.Invocation,
384+
NullLogger.Instance,
385+
cancellation.Token)
386+
.GetAsyncEnumerator(cancellation.Token);
387+
388+
await Assert.That(await enumerator.MoveNextAsync()).IsTrue();
389+
await Assert.That(enumerator.Current).IsEqualTo("started");
390+
391+
cancellation.Cancel();
392+
393+
var action = async () => await enumerator.MoveNextAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(5));
394+
var exception = await Assert.That(action).ThrowsException();
395+
await Assert.That(exception).IsTypeOf<OperationCanceledException>();
396+
}
397+
finally
398+
{
399+
Directory.Delete(sandboxDirectory, recursive: true);
400+
}
401+
}
402+
301403
private static async Task DrainAsync(IAsyncEnumerable<string> lines)
302404
{
303405
await foreach (var _ in lines)
@@ -318,6 +420,53 @@ private static async Task<List<string>> DrainToListAsync(IAsyncEnumerable<string
318420
return result;
319421
}
320422

423+
private static string CreateSandboxDirectory()
424+
{
425+
var sandboxDirectory = Path.Combine(
426+
Environment.CurrentDirectory,
427+
"tests",
428+
".sandbox",
429+
$"CodexExecTests-{Guid.NewGuid():N}");
430+
Directory.CreateDirectory(sandboxDirectory);
431+
return sandboxDirectory;
432+
}
433+
434+
private static ShellScriptHandle CreateShellScript(string sandboxDirectory, string name, string body)
435+
{
436+
if (OperatingSystem.IsWindows())
437+
{
438+
var scriptPath = Path.Combine(sandboxDirectory, $"{name}.cmd");
439+
File.WriteAllText(scriptPath, $"@echo off{Environment.NewLine}{body}{Environment.NewLine}");
440+
return new ShellScriptHandle(
441+
scriptPath,
442+
new CodexProcessInvocation(
443+
"cmd.exe",
444+
["/d", "/s", "/c", scriptPath],
445+
CreateProcessEnvironment(),
446+
string.Empty));
447+
}
448+
449+
var scriptFilePath = Path.Combine(sandboxDirectory, $"{name}.sh");
450+
File.WriteAllText(scriptFilePath, $"#!/bin/sh{Environment.NewLine}{body}{Environment.NewLine}");
451+
return new ShellScriptHandle(
452+
scriptFilePath,
453+
new CodexProcessInvocation(
454+
"/bin/sh",
455+
[scriptFilePath],
456+
CreateProcessEnvironment(),
457+
string.Empty));
458+
}
459+
460+
private static Dictionary<string, string> CreateProcessEnvironment()
461+
{
462+
return Environment.GetEnvironmentVariables()
463+
.Cast<DictionaryEntry>()
464+
.ToDictionary(
465+
entry => entry.Key?.ToString() ?? string.Empty,
466+
entry => entry.Value?.ToString() ?? string.Empty,
467+
StringComparer.Ordinal);
468+
}
469+
321470
private static bool ContainsPair(IReadOnlyList<string> args, string key, string value)
322471
{
323472
for (var index = 0; index < args.Count - 1; index += 1)
@@ -358,4 +507,6 @@ private static List<string> CollectFlagValues(IReadOnlyList<string> args, string
358507

359508
return result;
360509
}
510+
511+
private sealed record ShellScriptHandle(string ScriptPath, CodexProcessInvocation Invocation);
361512
}

‎CodexSharpSDK.Tests/Unit/CodexThreadTests.cs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,31 @@ public async Task RunAsync_ThrowsObjectDisposedExceptionAfterDispose()
254254
await Assert.That(exception).IsTypeOf<ObjectDisposedException>();
255255
}
256256

257+
[Test]
258+
public async Task RunAsync_StreamImages_CleansUpOwnedStreamsAndTempFilesWhenResolutionFails()
259+
{
260+
var firstExtension = $".cleanup-{Guid.NewGuid():N}-1";
261+
var secondExtension = $".cleanup-{Guid.NewGuid():N}-2";
262+
var firstStream = new TrackingMemoryStream([1, 2, 3, 4]);
263+
var secondStream = new ThrowingReadStream([5, 6, 7, 8]);
264+
var thread = new CodexThread(new CodexExec("codex"), new CodexOptions(), new ThreadOptions());
265+
266+
async Task<RunResult> Action() => await thread.RunAsync(
267+
[
268+
new TextInput("resolve local image inputs"),
269+
LocalImageInput.FromStream(firstStream, $"first{firstExtension}"),
270+
LocalImageInput.FromStream(secondStream, $"second{secondExtension}"),
271+
]);
272+
273+
var exception = await Assert.That(Action!).ThrowsException();
274+
275+
await Assert.That(exception).IsTypeOf<IOException>();
276+
await Assert.That(firstStream.IsDisposed).IsTrue();
277+
await Assert.That(secondStream.IsDisposed).IsTrue();
278+
await Assert.That(FindTemporaryImages(firstExtension)).IsEmpty();
279+
await Assert.That(FindTemporaryImages(secondExtension)).IsEmpty();
280+
}
281+
257282
[Test]
258283
public Task Dispose_CanBeCalledMultipleTimes()
259284
{
@@ -328,4 +353,37 @@ private static bool HasAttribute<TAttribute>(MemberInfo memberInfo)
328353
{
329354
return memberInfo.GetCustomAttribute<TAttribute>() is not null;
330355
}
356+
357+
private static string[] FindTemporaryImages(string extension)
358+
{
359+
return Directory.GetFiles(Path.GetTempPath(), $"codexsharp-image-*{extension}", SearchOption.TopDirectoryOnly);
360+
}
361+
362+
private class TrackingMemoryStream(byte[] buffer) : MemoryStream(buffer)
363+
{
364+
public bool IsDisposed { get; private set; }
365+
366+
protected override void Dispose(bool disposing)
367+
{
368+
IsDisposed = true;
369+
base.Dispose(disposing);
370+
}
371+
372+
public override async ValueTask DisposeAsync()
373+
{
374+
IsDisposed = true;
375+
await base.DisposeAsync().ConfigureAwait(false);
376+
}
377+
}
378+
379+
private sealed class ThrowingReadStream(byte[] buffer) : TrackingMemoryStream(buffer)
380+
{
381+
private static readonly IOException CopyFailureException = new("Simulated image stream failure.");
382+
383+
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
384+
=> ValueTask.FromException<int>(CopyFailureException);
385+
386+
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
387+
=> Task.FromException<int>(CopyFailureException);
388+
}
331389
}

‎CodexSharpSDK/Client/CodexClient.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,15 @@ internal void Start(Func<CodexExec> execFactory)
131131

132132
internal void Stop()
133133
{
134+
CodexExec? execToDispose;
134135
lock (_gate)
135136
{
136137
ThrowIfDisposed();
138+
execToDispose = _exec;
137139
_exec = null;
138140
}
141+
142+
execToDispose?.Dispose();
139143
}
140144

141145
internal CodexExec GetOrCreate(bool autoStart, Func<CodexExec> execFactory)
@@ -161,16 +165,20 @@ internal CodexExec GetOrCreate(bool autoStart, Func<CodexExec> execFactory)
161165

162166
internal void Dispose()
163167
{
168+
CodexExec? execToDispose;
164169
lock (_gate)
165170
{
166171
if (_disposed)
167172
{
168173
return;
169174
}
170175

176+
execToDispose = _exec;
171177
_exec = null;
172178
_disposed = true;
173179
}
180+
181+
execToDispose?.Dispose();
174182
}
175183

176184
private void ThrowIfDisposed() => ObjectDisposedException.ThrowIf(_disposed, nameof(CodexClient));

0 commit comments

Comments
 (0)