Skip to content

Commit 77dc30f

Browse files
Fix #1282: deterministically attach ConPTY input before returning so early stdin is not lost
CreatePseudoConsole and CreateProcessW both return synchronously, but the ConPTY pipeline (conhost server thread and the child's console-input reader) reaches steady state asynchronously. On slow/headless CI runners the window between "constructor returned" and "child is actually reading CONIN$" was long enough that the caller's first Input.WriteAsync could be dropped entirely, leaving cmd.exe idle for the full test timeout with zero output. Introduce a purely event-driven readiness barrier: - After CreateProcessW succeeds the constructor issues a single overlapped read on the caller-side output pipe. Its completion is the definitive signal that the ConPTY pipeline is running end-to-end (conhost producing output, and — for interactive shells — the child having attached and started emitting a banner/prompt). - Input.WriteAsync is wrapped in a GatedWriteStream that awaits the readiness task before delegating to the underlying overlapped FileStream, so early keystrokes cannot race the pipeline startup. - The bytes read by the probe are surfaced to the caller as the first bytes of Output via a PrependingReadStream, so no data is lost. - Disposal cancels the barrier so pending writers unblock instead of waiting forever for output that will never arrive. No Task.Delay / Thread.Sleep: the barrier is entirely event-driven and honours the caller's CancellationToken. Regression test Input_WriteAsync_ImmediatelyAfterConstruction_IsDeliveredToChild exercises the "write immediately after construction, expect echo" pattern that failed on the v0.0.1 release-gate CI run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 1dcf1c7 commit 77dc30f

2 files changed

Lines changed: 215 additions & 2 deletions

File tree

Phantom.Workspaces.Llm.Core.Tests/ConPtyPseudoTerminalTests.cs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,45 @@ public async Task ShellProducesOutput_AfterSuccessfulStart()
335335
Assert.Equal(0, await pty.WaitForExitAsync(cts.Token));
336336
}
337337

338+
/// <summary>
339+
/// Regression test for issue #1282: writing to <see cref="ConPtyPseudoTerminal.Input"/>
340+
/// immediately after construction — without any warm-up — must deterministically deliver
341+
/// the bytes to the child. Before the fix, on slow/headless CI runners the ConPTY server
342+
/// and the child's console-input reader had not yet attached when the constructor
343+
/// returned, so the initial keystrokes were silently dropped and cmd.exe stayed idle
344+
/// for the entire 30 s timeout with zero output. The fix gates every input write on the
345+
/// ConPTY pipeline having produced its first output byte (proving the server thread is
346+
/// running end-to-end and the child is emitting a banner/prompt), so first writes cannot
347+
/// race the pipeline startup.
348+
/// </summary>
349+
[Fact]
350+
public async Task Input_WriteAsync_ImmediatelyAfterConstruction_IsDeliveredToChild()
351+
{
352+
using var _ = new ConsoleScope();
353+
var payload = new ShellOpenPayload
354+
{
355+
Command = "cmd.exe",
356+
CommandArguments = [],
357+
Columns = 80,
358+
Rows = 24,
359+
};
360+
361+
await using var pty = new ConPtyPseudoTerminal(payload);
362+
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
363+
364+
// No warm-up, no pre-read: the very first thing we do after construction is issue
365+
// a write. This is the exact pattern that dropped keystrokes on CI before the fix.
366+
var readTask = ReadUntilAsync(pty, "startupraceok", cts.Token);
367+
await pty.Input.WriteAsync(Encoding.ASCII.GetBytes("echo startupraceok\r\n"), cts.Token);
368+
await pty.Input.FlushAsync(cts.Token);
369+
370+
Assert.Contains("startupraceok", await readTask, StringComparison.OrdinalIgnoreCase);
371+
372+
await pty.Input.WriteAsync(Encoding.ASCII.GetBytes("exit\r\n"), cts.Token);
373+
await pty.Input.FlushAsync(cts.Token);
374+
Assert.Equal(0, await pty.WaitForExitAsync(cts.Token));
375+
}
376+
338377
/// <summary>
339378
/// Verifies that the child process does not inheritan excessive number of handles from the
340379
/// parent. Before the fix, all inheritable handles in the parent leaked into the child,

Phantom.Workspaces.Llm.Core/Shell/ConPtyPseudoTerminal.cs

Lines changed: 176 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,10 @@ public SafePseudoConsoleHandle() : base(IntPtr.Zero, ownsHandle: true) { }
171171
private readonly SafePseudoConsoleHandle _hPC;
172172
private readonly SafeProcessHandle _hProcess;
173173
private readonly SafeWaitHandle _hThread;
174+
private readonly FileStream _rawOutput;
175+
private readonly FileStream _rawInput;
176+
private readonly CancellationTokenSource _readyCts = new();
177+
private readonly Task<byte[]> _firstOutputReady;
174178
private bool _disposed;
175179

176180
public Stream Output { get; }
@@ -293,8 +297,55 @@ public ConPtyPseudoTerminal(ShellOpenPayload payload)
293297

294298
// Caller-side pipe handles are created with FILE_FLAG_OVERLAPPED. Use isAsync: true
295299
// so FileStream uses true async I/O with deterministic cancellation and ordering.
296-
Output = new FileStream(outputRead, FileAccess.Read, bufferSize: 4096, isAsync: true);
297-
Input = new FileStream(inputWrite, FileAccess.Write, bufferSize: 4096, isAsync: true);
300+
_rawOutput = new FileStream(outputRead, FileAccess.Read, bufferSize: 4096, isAsync: true);
301+
_rawInput = new FileStream(inputWrite, FileAccess.Write, bufferSize: 4096, isAsync: true);
302+
303+
// Deterministic startup barrier for issue #1282:
304+
//
305+
// CreatePseudoConsole and CreateProcessW both return synchronously, but the ConPTY
306+
// pipeline (conhost/openconsole server thread, and the child's console-input reader)
307+
// reaches steady state asynchronously. On slow/headless CI runners the window between
308+
// "constructor returned" and "child is actually reading CONIN$" is long enough that
309+
// the caller's first writes to Input can be dropped before anything is attached to
310+
// consume them, and the child then sits idle forever.
311+
//
312+
// We issue a single overlapped read on the caller-side output pipe here. Its completion
313+
// is the definitive signal that the ConPTY pipeline is running end-to-end (conhost is
314+
// producing output, and — for interactive shells like cmd.exe — the child has attached
315+
// and started emitting a banner/prompt). Input.WriteAsync waits on this task before
316+
// touching the input pipe. The bytes we prefetch are surfaced to the caller as the
317+
// first bytes of Output so nothing is lost.
318+
//
319+
// No Task.Delay / Thread.Sleep: the barrier is purely event-driven.
320+
_firstOutputReady = ReadFirstOutputAsync(_readyCts.Token);
321+
322+
Output = new PrependingReadStream(this);
323+
Input = new GatedWriteStream(this);
324+
}
325+
326+
private async Task<byte[]> ReadFirstOutputAsync(CancellationToken ct)
327+
{
328+
var buf = new byte[4096];
329+
try
330+
{
331+
int n = await _rawOutput.ReadAsync(buf.AsMemory(), ct).ConfigureAwait(false);
332+
if (n <= 0)
333+
return Array.Empty<byte>();
334+
var trimmed = new byte[n];
335+
Buffer.BlockCopy(buf, 0, trimmed, 0, n);
336+
return trimmed;
337+
}
338+
catch (OperationCanceledException)
339+
{
340+
return Array.Empty<byte>();
341+
}
342+
catch
343+
{
344+
// If the read fails (e.g. pipe already broken because the child died), we still
345+
// release the barrier so callers see the actual downstream error rather than
346+
// deadlocking on the readiness wait.
347+
return Array.Empty<byte>();
348+
}
298349
}
299350

300351
// ── IPseudoTerminal ─────────────────────────────────────────────────────
@@ -356,12 +407,18 @@ public async ValueTask DisposeAsync()
356407

357408
_disposed = true;
358409

410+
// Release the readiness barrier so any pending Input.WriteAsync callers unblock
411+
// instead of waiting forever for output that will never arrive.
412+
_readyCts.Cancel();
413+
try { await _firstOutputReady.ConfigureAwait(false); } catch { }
414+
359415
await Input.DisposeAsync().ConfigureAwait(false);
360416
await Output.DisposeAsync().ConfigureAwait(false);
361417

362418
_hPC.Dispose();
363419
_hThread.Dispose();
364420
_hProcess.Dispose();
421+
_readyCts.Dispose();
365422
}
366423

367424
// ── Helpers ─────────────────────────────────────────────────────────────
@@ -510,4 +567,121 @@ private static void AppendArg(StringBuilder sb, string arg)
510567
sb.Append('"');
511568
}
512569
}
570+
571+
// ── Startup-race gating streams (issue #1282) ───────────────────────────
572+
573+
/// <summary>
574+
/// Wraps the raw ConPTY output <see cref="FileStream"/> and prepends the bytes read by the
575+
/// startup-readiness probe (see <see cref="ReadFirstOutputAsync"/>) so callers observe the
576+
/// output stream as if no bytes had been consumed. Subsequent reads delegate to the
577+
/// underlying overlapped <see cref="FileStream"/>.
578+
/// </summary>
579+
private sealed class PrependingReadStream : Stream
580+
{
581+
private readonly ConPtyPseudoTerminal _owner;
582+
private byte[]? _prepend;
583+
private int _prependOffset;
584+
private bool _prependConsumed;
585+
586+
public PrependingReadStream(ConPtyPseudoTerminal owner) { _owner = owner; }
587+
588+
public override bool CanRead => _owner._rawOutput.CanRead;
589+
public override bool CanSeek => false;
590+
public override bool CanWrite => false;
591+
public override long Length => throw new NotSupportedException();
592+
public override long Position
593+
{
594+
get => throw new NotSupportedException();
595+
set => throw new NotSupportedException();
596+
}
597+
598+
public override void Flush() { }
599+
public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();
600+
public override void SetLength(long value) => throw new NotSupportedException();
601+
public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException();
602+
603+
public override int Read(byte[] buffer, int offset, int count) =>
604+
ReadAsync(buffer.AsMemory(offset, count), CancellationToken.None).AsTask().GetAwaiter().GetResult();
605+
606+
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
607+
ReadAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask();
608+
609+
public override async ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
610+
{
611+
if (!_prependConsumed)
612+
{
613+
_prepend ??= await _owner._firstOutputReady.WaitAsync(cancellationToken).ConfigureAwait(false);
614+
int remaining = _prepend.Length - _prependOffset;
615+
if (remaining > 0)
616+
{
617+
int take = Math.Min(buffer.Length, remaining);
618+
_prepend.AsSpan(_prependOffset, take).CopyTo(buffer.Span);
619+
_prependOffset += take;
620+
return take;
621+
}
622+
_prependConsumed = true;
623+
}
624+
return await _owner._rawOutput.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);
625+
}
626+
627+
protected override void Dispose(bool disposing)
628+
{
629+
if (disposing) _owner._rawOutput.Dispose();
630+
base.Dispose(disposing);
631+
}
632+
633+
public override ValueTask DisposeAsync() => _owner._rawOutput.DisposeAsync();
634+
}
635+
636+
/// <summary>
637+
/// Wraps the raw ConPTY input <see cref="FileStream"/> and gates every write on the
638+
/// startup-readiness signal. Once the ConPTY pipeline has produced its first output byte
639+
/// (proving the server thread and — for interactive shells — the child are attached and
640+
/// ready) writes pass straight through to the underlying overlapped <see cref="FileStream"/>.
641+
/// This eliminates the observable "first keystrokes dropped" race on slow runners.
642+
/// </summary>
643+
private sealed class GatedWriteStream : Stream
644+
{
645+
private readonly ConPtyPseudoTerminal _owner;
646+
647+
public GatedWriteStream(ConPtyPseudoTerminal owner) { _owner = owner; }
648+
649+
public override bool CanRead => false;
650+
public override bool CanSeek => false;
651+
public override bool CanWrite => _owner._rawInput.CanWrite;
652+
public override long Length => throw new NotSupportedException();
653+
public override long Position
654+
{
655+
get => throw new NotSupportedException();
656+
set => throw new NotSupportedException();
657+
}
658+
659+
public override int Read(byte[] buffer, int offset, int count) => throw new NotSupportedException();
660+
public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();
661+
public override void SetLength(long value) => throw new NotSupportedException();
662+
663+
public override void Flush() => _owner._rawInput.Flush();
664+
public override Task FlushAsync(CancellationToken cancellationToken) =>
665+
_owner._rawInput.FlushAsync(cancellationToken);
666+
667+
public override void Write(byte[] buffer, int offset, int count) =>
668+
WriteAsync(buffer.AsMemory(offset, count), CancellationToken.None).AsTask().GetAwaiter().GetResult();
669+
670+
public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
671+
WriteAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask();
672+
673+
public override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
674+
{
675+
await _owner._firstOutputReady.WaitAsync(cancellationToken).ConfigureAwait(false);
676+
await _owner._rawInput.WriteAsync(buffer, cancellationToken).ConfigureAwait(false);
677+
}
678+
679+
protected override void Dispose(bool disposing)
680+
{
681+
if (disposing) _owner._rawInput.Dispose();
682+
base.Dispose(disposing);
683+
}
684+
685+
public override ValueTask DisposeAsync() => _owner._rawInput.DisposeAsync();
686+
}
513687
}

0 commit comments

Comments
 (0)