Fix Windows detached PTY startup handling#402
Conversation
BunsDev
left a comment
There was a problem hiding this comment.
Thanks @romgenie — the core Windows fix here is solid: retaining the ConPTY master, answering CPR/DA/DSR, and filtering query bytes is the right shape for #329, the VtQueryFilter state machine handles split-across-read queries correctly with bounded held prefixes, and the new unit tests all pass locally. Two blocking items before this can land:
1. VT-reply write can freeze (and hard-deadlock) a session's output path — High
On main, the detached reader thread only ever blocks on read(). This PR makes it call writer.write_all(reply) on the shared SharedPtyWriter mutex (pty_runner.rs ~1974) — the same mutex send_input holds across its blocking PTY write (daemon.rs ~377-383). daemon.rs ~354-359 already documents that a child that stops reading stdin can stall send_input indefinitely; that was previously tolerated because output kept draining. Now:
- The reader thread parks on that mutex the moment the child emits any VT query (crossterm apps re-emit CPR/DA on redraw/resize) → output draining and ledger events stop for the whole session.
- Realistic three-way deadlock: reader parked on mutex → PTY output buffer fills → child blocks writing output → child never drains stdin →
send_input's write never completes → mutex never released. Trigger: asend_inputpayload larger than the kernel PTY input buffer (~4 KB on Linux) to a busy TUI that queries cursor position. Only exit is/kill.
Fix: don't block the drain loop on the shared writer — try_lock and drop/retry the reply on contention (a stale CPR reply is harmless), or route replies through a small bounded channel serviced by a dedicated writer thread.
2. Unconditional 30s startup kill regresses quiet non-interactive harnesses on Unix — Medium
spawn_detached_with_observer's no-meaningful-output-in-30s kill has no platform/launch-mode gating, while the piped NonInteractive escape hatch (daemon.rs ~318) is #[cfg(windows)]-only. Chat launches every non-stream harness as NonInteractive (app.rs ~1063-1066), and manifest adapters support print-style modes that are legitimately silent until generation completes (e.g. non_interactive_prompt_prefix_args: ["-s","-p"], harness.rs ~2370). On Unix those sessions now get killed mid-generation and marked failed where main correctly waits — on the platform #329 never affected. COVEN_PTY_STARTUP_TIMEOUT_MS is #[cfg(debug_assertions)]-only, so there's no release escape hatch.
Fix: gate the startup kill to interactive/TUI launches (the class that actually hangs on VT queries) or to Windows, and/or make the timeout release-configurable.
Minor (non-blocking)
assign_process_to_job+SharedPtyKillerre-implement theCreateJobObjectW/OpenProcess/AssignProcessToJobObjectsequence frompiped_killer(daemon.rs ~415-445) with slightly different failure semantics — worth one shared helper.wait_for_windows_process_exitis duplicated verbatim between the daemon and pty_runner test modules.
With items 1 and 2 addressed this is mergeable — happy to re-review quickly.
# Conflicts: # crates/coven-cli/Cargo.toml
BunsDev
left a comment
There was a problem hiding this comment.
Re-reviewed at 48a0e41 — both blocking items are resolved architecturally, not patched around:
-
VT-reply deadlock: fixed. The shared
Arc<Mutex<Box<dyn Write>>>is gone; a dedicated writer thread owns the PTY writer behind a bounded 16-slot channel (spawn_shared_pty_writer/run_pty_writer), and the drain loop's reply path istry_sendwith the result discarded — the reader thread has no handle that can block on the input side at all.send_inputblocking on backpressure now degrades exactly as main did (self-recovering, kill available), and writer-thread lifetime is sound (BrokenPipeon error, exit when senders drop). Regression-locked bydetached_pty_keeps_draining_when_terminal_reply_queue_is_fullandterminal_replies_share_one_fifo_writer_path— verified passing locally. -
Startup kill: fixed.
detached_startup_timeout()isNoneon non-Windows (no kill timer exists on Unix), the Windows piped NonInteractive shortcut keeps the timeout scoped to exactly the interactive-PTY population #329 covered, andCOVEN_PTY_STARTUP_TIMEOUT_MSis now release-configurable on Windows. Locked bydetached_startup_timeout_is_disabled.
Also verified: the main merge (b09853c) brought #409/#412 in unmodified; a trial merge against current main (f6ff81a, #411 model forwarding) compiles clean and passes all 13 detached/startup/VT tests locally. The PowerShell cold-start test change is a pure de-flake (I cherry-picked it into #413 with your authorship to kill #407 independently — it dedupes on merge).
The job-object duplication with piped_killer remains an optional follow-up, non-blocking.
Excellent work on a hard concurrency surface, @romgenie — merging.
… (#423) The fake-codex batch shims copied stdin via PowerShell, whose cold start on a loaded windows-latest runner outlived even the 10s activity deadline (#402's headroom fix) and flaked codex_json_batch_shim_uses_stdin_and_emits_assistant_text again on PR #422 with the same os error 2. findstr is a native always-present binary with no cold start; "^" matches every line, so it copies stdin verbatim until EOF. Applied to both the pty_runner unit shim and the stream_json_integration fixture; the 10s deadline stays as generic startup headroom. Fixes #407 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Root cause
The generic detached runner cloned the PTY reader/writer and then dropped
MasterPtyas soon as spawn returned. On Windows, those cloned pipe handles do not own the HPCON, which caused intermittent early exits with2147483647and no output. When ConPTY stayed alive, the output path was still read-only and could not answer terminal queries such asESC[6n, leaving crossterm-style harnesses blocked during first paint.The reader now retains the master until child exit, shares a serialized writer with user input, recognizes queries split across reads, writes the appropriate replies, and filters only the recognized query bytes. Control-only initialization does not satisfy the startup guard.
Verification
coven-clisuite: 1,112 unit tests passed (1 ignored), 4 executor tests passed, and 2 stream integration tests passed (2 ignored); zero failurescoven-clisuite: 1,166 unit tests passed (1 ignored), plus 4 executor, 4 parallel, 23 smoke, and 4 stream tests (2 ignored); zero failuresdead_codewarnings)cargo fmt --check,git diff --check, and current-tree secret guard passConPTY may consume a primary DA query and inject its own valid DA response before Coven sees it. The native stub therefore accepts a valid DA response, while deterministic parser/writer tests prove Coven emits
ESC[?62;cand filters bothESC[candESC[0c, including split reads.Fixes #329