Skip to content

fix: preserve Unix PTY lifecycle and final output - #1927

Closed
rawkode wants to merge 3 commits into
raphamorim:mainfrom
rawkode:fix/unix-pty-lifecycle
Closed

rawkode wants to merge 3 commits into
raphamorim:mainfrom
rawkode:fix/unix-pty-lifecycle

Conversation

@rawkode

@rawkode rawkode commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #1926.

Unix PTY children now retain their exit status and serialize termination with reaping, preventing subsequent shutdown or Drop from signaling a reaped PID. Shutdown sends SIGHUP, allows a 100 ms grace period, escalates to SIGKILL when necessary, and reaps the child. Frontend destructors delegate shutdown to the PTY owner instead of signaling saved PIDs directly. Setup failures also release descriptors and terminate owned children. Child ownership lives in a dedicated module with explicit running/exited states; a retired child retains no signalable PID. External reaping consistently reports an unavailable status rather than appearing to be running.

The reader parses buffered bytes before returning EOF or errors, drains residual output on HUP, and continues across parsing budgets after child exit. One finalization path handles setup failures, I/O failures, explicit shutdown, and natural exit. Pending synchronized updates are flushed before exit notification. Final draining stops when a read would block and limits continuous descendant output to 100 ms between batches; lock waits, parsing, and final reap can extend total completion time.

Validation

Using Rust 1.98.0 on macOS:

  • cargo +1.98.0 test -p rio-vt -p teletypewriter --lib --quiet: 541 terminal-core tests and 15 PTY tests passed.
  • cargo +1.98.0 test -p librio --no-default-features --features pty --lib --quiet: 53 embedding tests passed, including real PTY shells.
  • cargo +1.98.0 check -p rio-vt --no-default-features --quiet, formatting, and git diff --check passed.
  • Regression coverage includes deterministic bytes-to-EIO under terminal-lock contention, EOF/HUP residual output, multiple parsing budgets, synchronized-update notification ordering, setup/reader failure cleanup, cached status, ignored SIGHUP, and a sentinel process verifying no signal after reap.
  • Restoring the original early error return makes the EIO regression fail with the final marker absent; it passes with the fix restored.

Linux native CI passed on the preceding commit 0c53582ed0, including the HUP-only drain regression and a new epoll injected-event roundtrip test. That test exposed a conversion path that dropped HUP/error readiness; injected epoll events now preserve both flags. EIO under coordinated lock contention remains synthetic coverage. Independent read-only review found no blocking issues.

The EventedPty::shutdown hook is explicitly optional: Unix implements termination/reaping, while the default is a no-op and Windows retains cleanup on drop. Regression tests also verify exactly-once finalization and external-reap event delivery.

Copilot AI lite review requested due to automatic review settings September 8, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

It changes low-level PTY/signal and shutdown semantics across crates and platforms, so it warrants final human review despite strong regression coverage.

Pull request overview

This PR addresses #1926 by making Unix PTY process ownership explicit and idempotent (preventing post-reap signaling / PID reuse hazards) and by ensuring the PTY reader reliably drains and parses final output (including under terminal-lock contention, EOF/HUP, and parse-budget limits) before publishing exit-related events.

Changes:

  • Introduces a Unix child lifecycle state machine (reap/status caching + shutdown escalation + “never signal after reap”) and routes shutdown through the PTY owner.
  • Updates the rio-vt reader loop to preserve buffered bytes on EOF/errors, drain output across parse budgets on exit/HUP, and flush synchronized updates before exit notification.
  • Adds regression tests covering the reported output-loss and lifecycle scenarios; updates docs to describe the bounded final-drain/shutdown behavior.
File summaries
File Description
teletypewriter/src/unix/mod.rs Adds ChildLifecycle with cached status + idempotent terminate/reap; ensures shutdown/reap occurs via PTY owner and adds lifecycle tests.
teletypewriter/src/lib.rs Extends EventedPty with a default shutdown() hook used by consumers to request termination/reap.
rio-vt/src/performer/mod.rs Refactors pty_read to return a read outcome, drains buffered output before EOF/errors, improves HUP handling, and performs final drain + sync flush on exit/shutdown.
rio-vt/src/performer/tests.rs Adds targeted regression tests for buffered-output preservation, parse-budget draining, HUP behavior, and shutdown on setup/read failures.
rio-vt/README.md Documents the new drain/shutdown semantics and the bounded descendant-output handling.
librio/src/lib.rs Removes destructor-time direct PID signaling; relies on PTY shutdown messaging.
frontends/rioterm/src/context/mod.rs Removes destructor-time PID signaling; delegates shutdown to the PTY owner via Msg::Shutdown.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@raphamorim

Copy link
Copy Markdown
Owner

Took the changes, and did few updates here #1930 . merged in main! thank you

@raphamorim raphamorim closed this Sep 17, 2026
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.

Unix PTY Lifecycle Can Signal Reused PIDs and Drop Final Output

3 participants