Skip to content

fix(terminal): a write parked on a full PTY input queue holds its slot until the foreground program exits #256

Description

@zackees

Context

test_terminal_240_disconnect_with_blocked_stdin fails on main. Four WebSocket clients open PTYs whose foreground program stops reading stdin, sends 60,000 bytes, and disconnects; the fifth connection must then succeed. It gets HTTP 429 instead — the four semaphore permits are never released.

This is the last known defect in the interactive terminal. It was masked until now: while #255 was open no WebSocket session could survive its handshake at all, so this test could never have reached the assertion. It has therefore not passed since the kernal-api migration (d5afb46, #242).

Measured behaviour

Slots return at ~27 s, and the test's blocking command is sleep 30. The release is timed by the foreground program exiting, not by the client disconnect.

PtySession::write is a blocking write_all. When the foreground program stops reading, the terminal input queue fills and the worker thread parks inside it. That thread:

  • holds the async_engine::SemaphorePermit for the session,
  • owns the PtySession, so the session cannot be dropped either — Drop runs when the worker's closure ends, which is what the write is blocking,
  • cannot be cancelled: it is blocked in the kernel, not in the async runtime.

So teardown waits on the write, which waits on the child, which waits on a disconnect that already happened.

What does not work

Signalling the process tree does not release the write. This was implemented and measured, not assumed:

let result = session.write(&vec![b'x'; 1 << 20]);   // parks on a full queue
signal_pty_tree(pid, true);                          // SIGKILL the whole tree

The write has still not returned after five seconds. The tree is killed and the child is reaped — a defunct entry is left — but the writer stays parked. The kernel keeps a blocked write on the master blocked even once every process holding the slave is gone; it returns only when something drains the queue, and a signal cannot drain a queue.

That dead end is now recorded on PtySession::pid in kernal-api v0.1.10 so the next person does not spend the same afternoon on it.

Killing the child an application-level way has the same problem. The blocked syscall is the whole issue; nothing that only ends the process can unblock it.

Proposed fix

Give the facade a bounded write, so the worker can observe the disconnect between attempts instead of parking inside one call:

/// Write as much as the terminal accepts before `timeout`, returning the count.
pub fn write_available(&mut self, bytes: &[u8], timeout: Duration) -> io::Result<usize>

Implemented over a non-blocking master (O_NONBLOCK plus poll). The platform layer already has precedent: interrupt_target performs a non-blocking byte write for exactly the "do not block on a full PTY input queue" reason, with tests named for it.

The worker then becomes:

let mut written = 0;
while written < data.len() {
    if input_closed.load(Ordering::Relaxed) { break; }   // client gone: stop, drop the session
    written += session.write_available(&data[written..], Duration::from_millis(50))?;
}

Breaking out drops the session, the permit, and the shell within one timeout instead of waiting for the foreground program.

Acceptance criteria

  1. RED -> GREEN. test_terminal_240_disconnect_with_blocked_stdin fails today. It must fail for the stated reason (fifth connection 429 after the four blocked writers disconnect) and pass after the fix, with the fifth connection succeeding while sleep 30 is still running. Chromium and WebKit.
  2. An unblocked terminal is unchanged: test_terminal_240_interactive_browser passes on both browsers, including the 120 KB paste.
  3. A disconnect during an in-flight write tears the session down promptly — asserted by timing, not just by eventual success, so a slow release cannot pass.
  4. No session outlives its client: after the test, no descendant of the CLI holds a PTY master.
  5. soldr cargo test --workspace, bash lint, and bash test pass.
  6. Safari remains a required target: no JSPI flags, WebAssembly.Suspending, or WebAssembly.promising.

Decisions

  • Priority P2. Reachable only with a foreground program that stops reading stdin and a disconnecting client, and the resource is reclaimed when that program exits. The terminal is fully usable without this fix.
  • Filed here rather than upstream, because this repo owns the test and the permit. The primitive belongs in kernal-api; the worker change belongs here.
  • The fix is a bounded write, not a signal. Recorded in "What does not work" so it is not re-litigated.
  • No interim client-side workaround. Releasing the permit on disconnect while the session lingers would trade a bounded leak for unbounded orphaned shells, which is worse.

Open questions

  • How far to take the non-blocking path. O_NONBLOCK on the master changes read semantics too, and the reader thread is a separate concern. Scoping the change to the writer alone (a non-blocking dup used only for write_available) is the smaller change; making the whole master non-blocking is cleaner but touches the reader.
  • Windows. signal_pty_tree is already a no-op there, and ConPTY writes go through a different mechanism. Whether write_available is meaningful on Windows, or the test stays Unix-only (stty is already, and the test is skipped there), needs a call.
  • Queue depth. Whether the input queue should also be bounded so a single client message can never overrun it, independently of this fix.

Related issues

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions