Skip to content

fix: PTY I/O never blocks the event loop — terminal_run fails cleanly on a stalled tab (v0.9.4) - #21

Merged
mabry1985 merged 3 commits into
mainfrom
fix/nonblocking-pty-writes
Oct 20, 2026
Merged

mabry1985 merged 3 commits into
mainfrom
fix/nonblocking-pty-writes

Conversation

@mabry1985

@mabry1985 mabry1985 commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Fixes #20

Root cause

PtySession.write() did a blocking os.write() on the master fd on the server's event loop (from terminal_run and from the WebSocket bridge). A PTY write blocks whenever the child stops reading its input. Worse, the child can only consume input as fast as its echo/redraw output is drained, and the drain is the session pump, which needs the same loop. So the write waits on the child, the child waits on the loop: the whole server deadlocks until the shell is killed.

brandLaunch on 10-19 (agent.log + audit.jsonl):

  • 20:19:20 terminal_run (a ~460-byte for … curl loop) → no log line at all until the forced restart at 21:29; the a2a stall watchdog only got to fire during shutdown ("no progress for 900s while running terminal_run").
  • 21:31:18 terminal_run typing a multi-KB heredoc (cat > myclaude <<'EOF' …) → loop silent until the 21:33:27 restart; the audit shows that call took 129 s, ending exactly at the restart (killing the shell made the write fail with EIO, which released the loop).

Reproduced locally on v0.9.3 with real PTYs: a raw-mode child that never reads, and a raw echoing cat, both freeze the loop indefinitely.

Fix

  • InputWriter (pty_session.py): one dedicated input thread per session writes in order, in 1 KiB chunks, waiting on select() with a 0.2 s poll so a close/cancel is noticed between waits. write() only queues (backlog capped at 4 MiB, then keystrokes drop with a warning). async send(data, timeout) waits off-loop and raises PtyWriteTimeout(written, total), abandoning the unwritten rest so a half-typed command can't finish typing itself later (at most the one 1 KiB chunk already in the kernel can still land).
  • Reads move off the loop's shared default executor onto a per-session thread. A pump parks in os.read for the session's lifetime, so each open tab used to hold one of the host's few default workers.
  • aclose kills the child first, then joins the input thread (≤1 s) before closing the fd, so no stray write can land on a reused fd number. Same treatment for WinPtySession.
  • terminal_run types with send(timeout=10s) and returns a clear error ("stopped accepting input — only N of M bytes …; use session='new'; keep long scripts in a file").
  • The WS bridge's sess.write() keeps its call site and is now non-blocking. No socket shutdown/close patterns were involved.
  • PROTO.md hard rule + README guardrail note; version 0.9.4 in both manifests.

Tests

tests/test_input_backpressure.py: real processes only. Each scenario runs its loop in a worker thread with a heartbeat, watched from the test thread, so a wedge fails (and kills the child) instead of hanging:

  1. raw-mode child that never reads → send raises PtyWriteTimeout in ~1 s, write returns instantly, heartbeat gap < 0.75 s, aclose < 5 s.
  2. raw echoing cat + 256 KiB through SessionManager (pump live) → completes, output pumped, loop never stalls.
  3. terminal_run on a tab that stops reading → clear error well inside its budget, loop live.

On v0.9.3, tests 1 and 3 fail with "the event loop wedged — no heartbeat for ~27–30s". Locally: 10/10 green loops of the PTY/tool suites; full gate (ruff, pytest 156 passed, node 25 passed).

🤖 Generated with Claude Code

Follow-up commits: CI caught a pre-existing race in test_connected_labels_the_tab_with_the_live_cwd (the bridge read /proc/<pid>/cwd before the freshly forked shell had chdir'd home); a new tab is now labelled by its start dir, and only a resumed tab reads the live cwd. Also one drop warning per backlog-full episode.

mabry1985 and others added 3 commits October 20, 2026 03:51
… on a stalled tab (v0.9.4)

A PTY write ran synchronously on the server's event loop. It blocks whenever the
child stops reading its input, and the child can only read as fast as its echo is
drained by the session pump, which itself needs the loop. So a long command line
(a heredoc) or a heavily-redrawing zle prompt deadlocked the whole server until the
shell was killed: brandLaunch froze twice on 10-19 (restarts 21:29, 21:33).

- pty_session: InputWriter feeds input from one dedicated thread per session, in
  order, in 1 KiB chunks with a select() wait so close/cancel is noticed; write()
  only queues (bounded backlog); async send() waits with a timeout and raises
  PtyWriteTimeout, abandoning the rest so a half-typed command can't finish later.
  Reads move off the shared default executor onto a per-session thread. aclose
  releases the input thread before the fd can be closed and reused. Same for the
  Windows backend.
- terminal_run types with send(timeout=10s) and returns a clear error.
- tests/test_input_backpressure.py: real PTYs (a raw-mode child that never reads; a
  raw echoing cat) on a watched loop — both wedge v0.9.3 and pass now.

Fixes #20

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d's chdir

CI caught test_connected_labels_the_tab_with_the_live_cwd reading /proc/<pid>/cwd
before the freshly forked shell had chdir'd home (it still reported the server's cwd).
Only a resumed tab needs the live cwd.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t chunk

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@protoreview protoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA panel review — PASS

code-review-structural · head 60bce68ef877 · formal

All three findings are tool-sourced (protopatch) and confirmed by the verifier. No prior requests, no counter-evidence, no drops. Severity grades are appropriate: Finding 1 is a guaranteed TypeError on a common path (minor, not major, because it's a single-line fix with no data-corruption or security impact); Finding 2 is a bounded DoS vector behind auth (minor); Finding 3 is a theoretical GC race (nit).

Findings

Severity Location Finding Verified
🟡 minor tools.py:66 asyncio.sleep(0, "") passes a positional argument to a keyword-only parameter (result), raising TypeError on the common no-foreground-process path. confirmed · nearby, not gating
🟡 minor api.py:295 POST /ticket mints entries into a module-level dict with no per-client or global rate cap; an authenticated caller can hold unbounded entries within the 30 s T… confirmed · nearby, not gating
⚪ nit sessions.py:297 Fire-and-forget task created in _forget has no stored reference; if the event loop is shutting down the task may be cancelled before pty.aclose() completes. confirmed · nearby, not gating
findings JSON (machine-readable)
[
  {
    "file": "tools.py",
    "line": 66,
    "severity": "minor",
    "category": "correctness",
    "claim": "asyncio.sleep(0, \"\") passes a positional argument to a keyword-only parameter (result), raising TypeError on the common no-foreground-process path.",
    "evidence": "asyncio.to_thread(command_name, fg) if fg else asyncio.sleep(0, \"\")",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "tools.py:66 has `asyncio.sleep(0, \"\")`. asyncio.sleep signature is `async def sleep(delay, *, result=None)` \u2014 `result` is keyword-only, so a second positional arg raises TypeError. This fires whenever `fg` is falsy (the common case: no foreground program). \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
    "nearby": true
  },
  {
    "file": "api.py",
    "line": 295,
    "severity": "minor",
    "category": "security",
    "claim": "POST /ticket mints entries into a module-level dict with no per-client or global rate cap; an authenticated caller can hold unbounded entries within the 30 s TTL window.",
    "evidence": "The /ticket endpoint mints a new ticket into the module-level _tickets dict on every call with no per-client or global rate limit. _prune_tickets only removes expired tickets (30 s TTL), so a burst of N requests within that window holds N entries simultaneously.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py: the /ticket route calls mint_ticket() which does `_tickets[t] = _Ticket(...)` with no cap. _prune_tickets only evicts expired entries. No rate-limiting logic exists in the endpoint or mint_ticket. An authenticated caller can grow the dict without bound within the 30 s TTL. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
    "nearby": true
  },
  {
    "file": "sessions.py",
    "line": 297,
    "severity": "nit",
    "category": "correctness",
    "claim": "Fire-and-forget task created in _forget has no stored reference; if the event loop is shutting down the task may be cancelled before pty.aclose() completes.",
    "evidence": "asyncio.get_running_loop().create_task(sess.pty.aclose())",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "sessions.py:297 has `asyncio.get_running_loop().create_task(sess.pty.aclose())` with no reference stored. Python docs warn that an unreferenced task can be GC'd mid-execution. Low practical risk (loop-shutdown timing), but the pattern is as described. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
    "nearby": true
  }
]

3 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.

  • tools.py:67 (minor) — asyncio.sleep(0, "") passes a positional argument to a keyword-only parameter (result), raising TypeError on the common no-foreground-process path.
  • api.py:295 (minor) — POST /ticket mints entries into a module-level dict with no per-client or global rate cap; an authenticated caller can hold unbounded entries within the 30 s TT
  • sessions.py:297 (nit) — Fire-and-forget task created in _forget has no stored reference; if the event loop is shutting down the task may be cancelled before pty.aclose() completes.

@protoreview protoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promoting the PASS verdict for head 60bce68ef877: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

  • minor tools.py:66 — asyncio.sleep(0, "") passes a positional argument to a keyword-only parameter (result), raising TypeError on the common no-foreground-process path.
  • minor api.py:295 — POST /ticket mints entries into a module-level dict with no per-client or global rate cap; an authenticated caller can hold unbounded entries within the 30 s TTL window.
  • nit sessions.py:297 — Fire-and-forget task created in _forget has no stored reference; if the event loop is shutting down the task may be cancelled before pty.aclose() completes.

Approving a WARN does not resolve its findings (issue #22).

@mabry1985
mabry1985 merged commit bdc7f17 into main Oct 20, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

terminal_run can block the host event loop for 900s on a stalled write

1 participant