Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🔵 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-vtreader 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.
Owner
|
Took the changes, and did few updates here #1930 . merged in main! thank you |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, andgit diff --checkpassed.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::shutdownhook 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.