smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: prevent ClosePseudoConsole deadlock hanging PTY on Windows

Browse Source 
ConPTY's ClosePseudoConsole can deadlock when it tries to flush output
to a pipe that nobody is reading (microsoft/terminal#1810). This caused
the PTY Promise to never resolve on Windows, making bash commands with
pty:true hang indefinitely.

Root cause: portable-pty's drop(master) calls ClosePseudoConsole
synchronously. If ConPTY's internal render thread is blocked writing to
a full/undrained output pipe, ClosePseudoConsole waits forever.

Fix (three parts):

1. Rust (pty.rs): Reordered teardown to follow Microsoft's recommended
   shutdown sequence:
   - Drop writer first (close ConPTY input pipe)
   - Drain reader thread with 500ms timeout (consume output pipe)
   - Drop master in a background thread with recv_timeout(2s):
     * Clean case: ClosePseudoConsole completes, thread reclaimed
     * Hung case: timeout expires, main thread returns anyway
   - Replace child.wait() with try_wait() polling on Windows
     (WaitForSingleObject can also hang in ConPTY)

2. TypeScript (bash-pty-selection.ts): Remove the Windows blanket
   disable that prevented PTY from ever being used on Windows.

3. Tests: Updated to verify PTY works on Windows with UI context.
This commit is contained in:
Sanskar Singh
2026-05-16 07:02:24 +05:30
parent 5d1a14e72a
commit cffc94a978
3 changed files with 66 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pi-natives/src/pty.rs
+60 -6
View File
@@ -75,6 +75,7 @@ const CONTROL_MESSAGES_PER_TICK: usize = 64;
const READER_EVENTS_PER_TICK: usize = 256;
const POST_CANCEL_DRAIN_TIMEOUT: Duration = Duration::from_millis(300);
const POST_EXIT_DRAIN_TIMEOUT: Duration = Duration::from_millis(300);
#[cfg(not(windows))]
const FINAL_READER_DRAIN_TIMEOUT: Duration = Duration::from_millis(50);
struct PtySessionCore {
@@ -438,18 +439,49 @@ fn run_pty_sync(
exit_code = Some(i32::try_from(status.exit_code()).unwrap_or(i32::MAX));
}
} else {
let status = child
.wait()
.map_err(|err| Error::from_reason(format!("Failed waiting PTY process: {err}")))?;
exit_code = Some(i32::try_from(status.exit_code()).unwrap_or(i32::MAX));
// On Windows, child.wait() can hang indefinitely in ConPTY.
// Poll try_wait() with a short timeout instead.
#[cfg(windows)]
{
let wait_start = Instant::now();
while exit_code.is_none() && wait_start.elapsed() < Duration::from_secs(5) {
if let Some(status) = child
.try_wait()
.map_err(|err| Error::from_reason(format!("Failed checking PTY status: {err}")))?
{
exit_code = Some(i32::try_from(status.exit_code()).unwrap_or(i32::MAX));
break;
}
std::thread::sleep(Duration::from_millis(50));
}
}
#[cfg(not(windows))]
{
let status = child
.wait()
.map_err(|err| Error::from_reason(format!("Failed waiting PTY process: {err}")))?;
exit_code = Some(i32::try_from(status.exit_code()).unwrap_or(i32::MAX));
}
}
}
// --- Teardown ---
// Step 1: Close the ConPTY input pipe first.
// Per Microsoft docs, close the input handle before calling ClosePseudoConsole.
// This signals to ConPTY that no more input will arrive, allowing its internal
// I/O threads to finish processing and eventually close the output pipe.
drop(writer);
drop(master);
// Step 2: Drain the reader thread.
// After the child exits and input is closed, ConPTY should flush remaining
// output and signal EOF on the output pipe, causing the reader thread to exit.
// On Windows, use a generous timeout to accommodate ConPTY's async teardown.
if !reader_done {
let finalize_deadline = Instant::now() + FINAL_READER_DRAIN_TIMEOUT;
#[cfg(windows)]
let drain_timeout = Duration::from_millis(500);
#[cfg(not(windows))]
let drain_timeout = FINAL_READER_DRAIN_TIMEOUT;
let finalize_deadline = Instant::now() + drain_timeout;
while Instant::now() < finalize_deadline {
let remaining = finalize_deadline.saturating_duration_since(Instant::now());
let wait_duration = remaining.min(Duration::from_millis(5));
@@ -468,6 +500,28 @@ fn run_pty_sync(
}
}
// Step 3: Drop master (calls ClosePseudoConsole on Windows).
// ClosePseudoConsole can deadlock if ConPTY tries to flush output
// while nobody is reading the pipe (microsoft/terminal#1810).
// Always offload to a background thread on Windows, then wait with
// a timeout so the thread is reclaimed when ClosePseudoConsole
// completes cleanly. If it hangs, we walk away — the thread leaks,
// but the main thread never blocks.
#[cfg(windows)]
{
let (drop_tx, drop_rx) = mpsc::channel::<()>();
std::thread::spawn(move || {
drop(master);
let _ = drop_tx.send(());
});
let _ = drop_rx.recv_timeout(Duration::from_secs(2));
}
#[cfg(not(windows))]
{
drop(master);
}
// Step 4: Join reader thread if it finished.
// A detached descendant can keep the PTY slave open forever; do not block
// completion waiting on join when the reader thread did not reach EOF.
if reader_done {
packages/coding-agent/src/tools/bash-pty-selection.ts
-1
View File
@@ -10,6 +10,5 @@ export interface BashPtyContext {
export function canUseInteractiveBashPty(pty: boolean, ctx: BashPtyContext | undefined): boolean {
if (!pty) return false;
if ($env.PI_NO_PTY === "1") return false;
if (process.platform === "win32" && $env.PI_FORCE_PTY !== "1") return false;
return ctx?.hasUI === true && ctx.ui !== undefined;
}
packages/coding-agent/test/tools/bash-pty-selection.test.ts
+6 -34
View File
@@ -4,7 +4,6 @@ import { canUseInteractiveBashPty } from "@oh-my-pi/pi-coding-agent/tools/bash-p
const originalPlatform = process.platform;
const originalNoPty = Bun.env.PI_NO_PTY;
const originalForcePty = Bun.env.PI_FORCE_PTY;
function setPlatform(platform: NodeJS.Platform): void {
Object.defineProperty(process, "platform", {
value: platform,
@@ -29,13 +28,6 @@ function setNoPty(value: string | undefined): void {
Bun.env.PI_NO_PTY = value;
}
function setForcePty(value: string | undefined): void {
if (value === undefined) {
delete Bun.env.PI_FORCE_PTY;
return;
}
Bun.env.PI_FORCE_PTY = value;
}
function interactiveContext() {
return { hasUI: true, ui: {} };
}
@@ -44,17 +36,16 @@ describe("bash PTY selection", () => {
afterEach(() => {
restorePlatform();
setNoPty(originalNoPty);
setForcePty(originalForcePty);
});
it("disables interactive PTY on Windows even when requested with UI", () => {
it("allows interactive PTY on Windows when requested with UI", () => {
setPlatform("win32");
setNoPty(undefined);
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(false);
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(true);
});
it("allows interactive PTY on non-Windows only when requested with UI and not disabled", () => {
it("allows interactive PTY on non-Windows when requested with UI and not disabled", () => {
setPlatform("linux");
setNoPty(undefined);
@@ -65,28 +56,9 @@ describe("bash PTY selection", () => {
setNoPty("1");
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(false);
});
it("allows interactive PTY on Windows when PI_FORCE_PTY=1", () => {
it("disables interactive PTY when pty is false", () => {
setPlatform("win32");
setNoPty(undefined);
setForcePty("1");
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(true);
});
it("ignores PI_FORCE_PTY on non-Windows (follows normal UI check)", () => {
setPlatform("linux");
setNoPty(undefined);
setForcePty("1");
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(true);
expect(canUseInteractiveBashPty(true, undefined)).toBe(false);
});
it("PI_NO_PTY=1 overrides PI_FORCE_PTY=1", () => {
setPlatform("win32");
setNoPty("1");
setForcePty("1");
expect(canUseInteractiveBashPty(true, interactiveContext())).toBe(false);
expect(canUseInteractiveBashPty(false, interactiveContext())).toBe(false);
});
});