diff --git a/docs/architecture/decisions/004-exit-code-on-control-chunk.md b/docs/architecture/decisions/004-exit-code-on-control-chunk.md index 8b0df96..b06ed18 100644 --- a/docs/architecture/decisions/004-exit-code-on-control-chunk.md +++ b/docs/architecture/decisions/004-exit-code-on-control-chunk.md @@ -68,9 +68,14 @@ The `code` is an `i32` in this crate's exit-status convention: number on signal-terminated exit (e.g., `-9` for SIGKILL) — the same convention as Python's `subprocess.Popen.returncode`. It is **not** `ExitStatus::code()`'s behavior, which returns `None` for signal -termination (the encoding comes from `ExitStatusExt::signal()`, see -`src/local/pipe.rs::exit_code_from`). The chunk is the last control -chunk before stream close. +termination. Both local backends encode it: the pipe path via +`ExitStatusExt::signal()` (`src/local/pipe.rs::exit_code_from`), the +PTY path via a direct `libc::waitpid` reap decoded to `WEXITSTATUS` / +`-WTERMSIG` (`src/local/pty.rs::wait_exit_code`) — portable-pty 0.9.0's +`ExitStatus` would flatten a signal death to `code = 1` with the signal +only as a strsignal name string, so the waiter reaps by pid and skips +the conversion (review #004). The chunk is the last control chunk +before stream close. ### 2. The "exit chunk is last" invariant diff --git a/docs/architecture/decisions/005-backend-cleanup-on-session-cancel.md b/docs/architecture/decisions/005-backend-cleanup-on-session-cancel.md index 6e7281c..5805978 100644 --- a/docs/architecture/decisions/005-backend-cleanup-on-session-cancel.md +++ b/docs/architecture/decisions/005-backend-cleanup-on-session-cancel.md @@ -40,7 +40,8 @@ backend's three std threads illustrate the gap: `Receiver::drop`, the writer's `recv()` returns `None`; the thread drops the master writer (EOF to the slave's stdin) and exits. ✓ `Drop` works. -- **Waiter thread** — blocking `Child::wait()`. This is a syscall that +- **Waiter thread** — blocking wait (`Child::wait()`; on Unix now the + `libc::waitpid` reap — ADR-004). This is a syscall that returns **only when the child is reaped**. It does not observe channel close. If the child ignores stdin EOF (a daemon, a long-lived process with no stdin reader, a process in an @@ -141,10 +142,10 @@ alongside `Child::wait()`. The pattern: - The `exit_code` future's `Drop` (runs on cancel only — on resolve, the `Drop` is a no-op via the guard) calls `ChildKiller::kill(SIGHUP)` (or the backend's configured cancel signal). The kill causes the - child to exit; the waiter thread's `wait()` returns; the waiter + child to exit; the waiter thread's blocking wait returns; the waiter thread's `oneshot::send` fails silently (the receiver was dropped with the future). The waiter thread then exits. The child is reaped - by the waiter thread's `wait()`; no zombie. + by the waiter thread; no zombie. For pipe mode (`terminal: None`), the same pattern applies with `tokio::process::Child::start_kill()` (or `Child::kill()`) instead of @@ -260,7 +261,7 @@ seam. that needs a different cancel signal configures it internally. 3. **The waiter thread (local backend) reaps the killed child.** After the `Drop`-on-cancel calls `ChildKiller::kill(SIGHUP)`, the child - exits; the waiter thread's `wait()` returns and reaps it (no + exits; the waiter thread's blocking wait returns and reaps it (no zombie). The waiter thread then exits. The `oneshot::send` from the waiter thread fails silently (the receiver was dropped with the future) — this is expected and not an error. diff --git a/docs/architecture/tty-adapter.md b/docs/architecture/tty-adapter.md index 3c9a3ab..4f5a07b 100644 --- a/docs/architecture/tty-adapter.md +++ b/docs/architecture/tty-adapter.md @@ -221,13 +221,14 @@ adapter's session driver, not in the backend. The ordering: 1. The stdout pump (task A) drains the backend's stdout to EOF. The backend's stdout ends when the process exits and the PTY/pipe buffer - drains (Unix `Child::wait()` blocks until the child is reaped, which + drains (the waiter thread's blocking wait returns when the child is + reaped, which happens after the child exits and its stdout drains — ADR-004 assumption 1). 2. The exit task (task C) awaits `TtyHandle.exit_code`. The exit resolves - after the child is reaped (the local backend's waiter thread calls - `Child::wait()`; docker's `inspect_exec` after the output stream ends; - SSH's channel close after the process exits). + after the child is reaped (the local backend's waiter thread blocks on + the wait (`libc::waitpid`); docker's `inspect_exec` after the output + stream ends; SSH's channel close after the process exits). 3. **The adapter waits for *both* the stdout pump to complete (EOF) *and* `exit_code` to resolve** before enqueueing the exit chunk. If a backend's stdout outlives the exit resolve (a hypothetical backend @@ -311,7 +312,8 @@ architectural commitment. which triggers the backend's cancel-cleanup — the session target is killed (ADR-005). For the local backend, the `exit_code` future's `Drop` calls `ChildKiller::kill(SIGHUP)`, the child exits, the - waiter thread's `wait()` reaps it and exits, and the reader/writer + waiter thread's blocking wait reaps it and exits, and the + reader/writer threads exit on channel close. For docker/SSH backends (future), the `Drop` issues the backend's kill (container kill / channel close). See ADR-005 for the contract and the mechanism. diff --git a/docs/architecture/tty-local.md b/docs/architecture/tty-local.md index 33b45ff..355ce5e 100644 --- a/docs/architecture/tty-local.md +++ b/docs/architecture/tty-local.md @@ -105,13 +105,22 @@ pub enum StdinCmd { Unpin>` (an `AsyncWrite` impl that wraps each `write` as a `StdinCmd::Bytes` and `flush` as a no-op; the `mpsc::Sender` is the sink). -3. **Waiter thread** — blocking `Child::wait()` → `oneshot::Sender` - with the exit code. The async-facing `TtyHandle.exit_code` is a - `Future` wrapping this `oneshot::Receiver` PLUS a kill guard - holding the `portable_pty::ChildKiller` (see "Cancel-Cleanup - (ADR-005)" below). This is the `Future` the adapter awaits (ADR-002 - REQ-TTY-01; ADR-004); its `Drop`-on-cancel kills the child - (ADR-005). + 3. **Waiter thread** — blocking wait → `oneshot::Sender` with the + exit code in ADR-004's encoding. On Unix the reap is + `libc::waitpid(pid, ..)` on the direct child, decoded to + `WEXITSTATUS` (normal exit) or `-WTERMSIG` (signal-terminated exit — + the ADR-004 negative encoding): portable-pty 0.9.0's `ExitStatus` + would flatten a signal-terminated child to `code = 1` with the signal + only as a strsignal name string (review #004); reaping by pid avoids + the conversion entirely, and `std::process::Child`'s `Drop` neither + kills nor reaps, so the two paths never double-reap. `Child::wait()` + remains as the fallback (waitpid failure) and the non-Unix primary. + The async-facing `TtyHandle.exit_code` is a + `Future` wrapping this `oneshot::Receiver` PLUS a kill guard + holding the `portable_pty::ChildKiller` (see "Cancel-Cleanup + (ADR-005)" below). This is the `Future` the adapter awaits (ADR-002 + REQ-TTY-01; ADR-004); its `Drop`-on-cancel kills the child + (ADR-005). `TtyHandle.stderr` is `None` (PTY backends merge stdout/stderr — kernel PTY property, one output stream from the slave). @@ -201,10 +210,12 @@ impl Drop for LocalExitFuture { ``` On cancel: the `Drop` kills the child (SIGHUP); the child exits; the -waiter thread's `wait()` reaps it and exits (its `oneshot::send` fails -silently — the receiver was dropped with the future, which is expected); -the reader/writer threads exit on channel close. The child is reaped -(no zombie) by the waiter thread's `wait()` returning after the kill. +waiter thread reaps it (Unix: `libc::waitpid` — see "Waiter thread" +above) and exits (its `oneshot::send` fails silently — the receiver was +dropped with the future, which is expected); the reader/writer threads +exit on channel close. The child is reaped (no zombie) after the kill; +its SIGHUP death resolves to `-1` (the ADR-004 sentinel collision) on +the no-longer-listened-to channel. **Pipe mode.** The same pattern with `tokio::process::Child` instead of `portable_pty::Child`. The `exit_code` future's `Drop` guard holds the diff --git a/docs/reviews/004-pty-exit-signal-fidelity-alkssh-input.md b/docs/reviews/004-pty-exit-signal-fidelity-alkssh-input.md index a0a126c..1636fce 100644 --- a/docs/reviews/004-pty-exit-signal-fidelity-alkssh-input.md +++ b/docs/reviews/004-pty-exit-signal-fidelity-alkssh-input.md @@ -1,6 +1,6 @@ --- -status: open -last_updated: 2026-10-31 +status: resolved +last_updated: 2026-11-01 --- # Code Review #004 — PTY-path exit-signal fidelity (consumer input from alkssh) @@ -124,4 +124,39 @@ cannot say a signal happened. exit reporting" (the derivation contract this gap constrains) - alkssh POC-3 §D.2 (pipe-path fidelity proven live; PTY-path limit verified against portable-pty source) -- alknet ADR-055 (the `-1` sentinel lineage ADR-004 inherits) \ No newline at end of file +- alknet ADR-055 (the `-1` sentinel lineage ADR-004 inherits) + +## Resolution (2026-11-01) + +The finding was verified against the published portable-pty 0.9.0 +source and confirmed in full: the waiter's `status.exit_code() as i32` +discarded signal identity; the `From` impl +keeps a signal death only as a strsignal name string with `code = 1` +(`unwrap_or(1)`), while the pipe path conformed. + +Resolved **in-crate, without a fork and without an upstream ask**, via +option (1) of the framing above — but through a mechanism the framing +did not list: on Unix the waiter now skips portable-pty's status +conversion entirely and reaps the direct child with +`libc::waitpid(pid, ..)` (`src/local/pty.rs::wait_exit_code`), decoding +`WIFEXITED`/`WEXITSTATUS` (normal exit) and `WIFSIGNALED`/`WTERMSIG` +(signal death → `-N`). This is fully numeric, locale-independent, and +exhaustive (no name→signum table, no strsignal parsing). It relies on +portable-pty's unix backend returning a `std::process::Child` whose +`Drop` neither kills nor reaps — so the two paths never double-reap +(`Child::wait()` remains as the waitpid-failure fallback and the +non-Unix primary). + +Behavioral notes recorded alongside: + +- Signal deaths from the crate's own features now encode honestly: + `signal("TERM")` → `-15`; the ADR-056 cancel kill (SIGHUP) → `-1`, + which is the documented ADR-004 sentinel collision (now common on the + cancel path, not just the wait-failure path). +- Tests strengthened: PTY-path `SIGINT` → `-2`, `SIGTERM` → `-15`, and + the SIGHUP fallback → `-1` collision is pinned. alkssh needs no + change; its mapping table consumes the negative code verbatim. + +Docs updated: ADR-004 decision 1 (both backends' encoding reference), +`tty-local.md` (waiter thread + cancel-cleanup), ADR-005 and +`tty-adapter.md` (mechanism wording). \ No newline at end of file diff --git a/src/local/pty.rs b/src/local/pty.rs index 58e70a3..5817926 100644 --- a/src/local/pty.rs +++ b/src/local/pty.rs @@ -32,9 +32,22 @@ //! `ChildKiller::kill()` (SIGHUP) — best-effort, the child may already be //! exiting. On the happy path (the future resolves), `poll` disarms the //! guard (`Option::take()`), so the subsequent `Drop` is a no-op. The -//! waiter thread reaps the killed child via its blocking `wait()`, so +//! waiter thread reaps the killed child via its blocking wait (Unix: +//! `libc::waitpid`), so //! there is no zombie. See `tty-local.md` §"Cancel-Cleanup (ADR-056)". //! +//! # Exit-status encoding (ADR-004) +//! +//! On Unix the waiter reaps the child via `libc::waitpid` (not portable-pty's +//! `Child::wait()`) so a signal-terminated child reports the negative signal +//! number (ADR-004 decision 1): `signal("TERM")` resolves to `-15`, the +//! ADR-056 cancel kill (SIGHUP) to `-1`. portable-pty 0.9.0's `ExitStatus` +//! would flatten the same death to code `1` with the signal only as a +//! strsignal name string — indistinguishable from a real `exit 1`. The pipe +//! path encodes via `std::os::unix::process::ExitStatusExt::signal()` +//! (`pipe.rs::exit_code_from`); both backends now agree on the convention. +//! See `wait_exit_code` and review #004. +//! //! # Bridge error paths (review #001 L6) //! //! The three-thread bridge's error arms are documented-unreachable @@ -52,9 +65,10 @@ //! - **writer write/flush failure** — the master fd is open and the //! writer thread is the sole writer; a write error requires an //! externally-closed fd the bridge never observes. -//! - **waiter `wait()` failure** — `std::process::Child::wait` fails -//! only if the child was already reaped (the bridge never calls -//! `try_wait`) — unreachable through the public path. The ADR-055 §4 +//! - **waiter `wait()` failure** — reached only on the waitpid-failure +//! fallback (see `wait_exit_code`); `std::process::Child::wait` fails +//! only if the child was already reaped (the bridge normally reaps via +//! `libc::waitpid` and never calls `try_wait`). The ADR-055 §4 //! `-1` sentinel it would produce is covered at the adapter level //! (`exit_error_sends_minus_one`): the sentinel also arises when the //! oneshot is dropped (kill-on-cancel path, exercised by the @@ -78,7 +92,7 @@ use crate::backend::{ use crate::control::signal_from_name; use bytes::Bytes; use futures_core::Stream; -use portable_pty::{native_pty_system, ChildKiller, CommandBuilder, MasterPty, PtySize}; +use portable_pty::{native_pty_system, Child, ChildKiller, CommandBuilder, MasterPty, PtySize}; use tokio::sync::{mpsc, oneshot}; use tokio_stream::wrappers::ReceiverStream; use tracing::{debug, warn}; @@ -196,9 +210,9 @@ impl TtyControl for PtyControl { /// `Ready`, the killer is taken (`Option::take()`) — disarmed — so the /// subsequent `Drop` is a no-op. On cancel (the future is dropped before /// resolving), `Drop` calls `ChildKiller::kill()` (SIGHUP) — best-effort; -/// the child may already be exiting. The waiter thread's blocking `wait()` -/// reaps the killed child, so there is no zombie. The contract is "kill on -/// cancel; no-op on resolve." +/// the child may already be exiting. The waiter thread's blocking wait +/// (Unix: `libc::waitpid`) reaps the killed child, so there is no zombie. +/// The contract is "kill on cancel; no-op on resolve." pub struct LocalExitFuture { rx: oneshot::Receiver, killer: Option>, @@ -394,6 +408,72 @@ impl tokio::io::AsyncWrite for StdinSink { } } +/// Wait for the child to terminate and resolve to the ADR-004 exit-status +/// encoding: `ExitStatus::code()` on normal exit; on Unix, the negative +/// signal number on signal-terminated exit (e.g. `-15` for SIGTERM — +/// Python's `Popen.returncode` parity). +/// +/// On Unix the reap goes through `libc::waitpid(pid, ..)` on the direct +/// child pid, not portable-pty's `Child::wait()`: portable-pty 0.9.0 +/// converts a signal-terminated child to `ExitStatus { code: 1, signal: +/// Some(name) }` — the signal identity survives only as a strsignal name +/// string that cannot be mapped back numerically (it is locale-dependent), +/// and `code = 1` collides with a real `exit 1`. The reaped child is a +/// direct child of this process (portable-pty's unix backend spawns a +/// `std::process::Child`, whose `Drop` neither kills nor reaps), and +/// `Child::wait()` is reachable only on the waitpid-failure path — where +/// the child was not reaped — so exactly one of the two paths reaps. +/// +/// Non-Unix has no Unix signal semantics; `Child::wait()` → +/// `exit_code() as i32` matches the convention for normal exits. `-1` is +/// the adapter's "could not determine" sentinel (ADR-004 §4) for wait +/// failures and undecodable statuses. Note the documented sentinel +/// collision: the ADR-056 cancel path and the request-signal fallback kill +/// with SIGHUP, so a cancelled session's child death resolves to `-1`. +fn wait_exit_code(pid: Option, child: &mut Box) -> i32 { + #[cfg(unix)] + if let Some(pid) = pid { + let mut status: libc::c_int = 0; + let reaped = loop { + let r = unsafe { libc::waitpid(pid, &mut status, 0) }; + if r == pid { + break true; + } + if r >= 0 { + break false; + } + if std::io::Error::last_os_error().kind() == std::io::ErrorKind::Interrupted { + continue; + } + break false; + }; + if reaped { + if libc::WIFEXITED(status) { + return libc::WEXITSTATUS(status); + } + if libc::WIFSIGNALED(status) { + return -libc::WTERMSIG(status); + } + return -1; + } + warn!( + pid, + "pty-waiter: waitpid failed; falling back to Child::wait()" + ); + } + + #[cfg(not(unix))] + let _ = pid; + + match child.wait() { + Ok(status) => status.exit_code() as i32, + Err(e) => { + warn!("pty-waiter: wait failed: {e}"); + -1 + } + } +} + /// Allocate a local PTY, spawn `cmd` into it, and return the async-facing /// `TtyHandle`. /// @@ -457,6 +537,7 @@ pub fn allocate_pty( // reference the same underlying pid/handle via clone_killer. let killer = child.clone_killer(); let killer_for_control = killer.clone_killer(); + let waiter_pid = pid.map(|p| p as i32); let master: Arc>> = Arc::new(Mutex::new(pair.master)); @@ -544,20 +625,12 @@ pub fn allocate_pty( message: format!("spawn pty-writer thread: {e}"), })?; - // --- Waiter thread: blocking Child::wait() → oneshot --- + // --- Waiter thread: blocking wait → oneshot (ADR-004 encoding) --- let (exit_tx, exit_rx) = oneshot::channel::(); thread::Builder::new() .name("pty-waiter".into()) .spawn(move || { - let status = match child.wait() { - Ok(s) => s, - Err(e) => { - warn!("pty-waiter: wait failed: {e}"); - let _ = exit_tx.send(-1); - return; - } - }; - let code = status.exit_code() as i32; + let code = wait_exit_code(waiter_pid, &mut child); debug!(exit_code = code, "pty-waiter: child reaped"); let _ = exit_tx.send(code); }) @@ -723,10 +796,7 @@ mod tests { .await .expect("exit timed out") .expect("exit_code"); - assert_ne!( - code, 0, - "signal-terminated child should report non-zero exit: {code}" - ); + assert_eq!(code, -2, "SIGINT death must report -2 (ADR-004 encoding)"); } #[cfg(unix)] @@ -754,6 +824,31 @@ mod tests { assert_ne!(code, 0, "process group should have been killed: {code}"); } + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn signal_term_kills_child() { + let marker = marker_path("sigterm_ready"); + let cmd = format!("echo ready > '{}'; exec sleep 60", marker.display()); + let handle = allocate_pty( + term(), + vec!["bash".to_string(), "-c".to_string(), cmd], + None, + env_default(), + ) + .expect("allocate"); + let control = handle.control.as_ref().expect("control"); + wait_marker(&marker).await; + control.signal("TERM"); + let code = tokio::time::timeout(std::time::Duration::from_secs(5), handle.exit_code) + .await + .expect("exit timed out") + .expect("exit_code"); + assert_eq!( + code, -15, + "SIGTERM death must report -15 (ADR-004 encoding)" + ); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn cancel_cleanup_kills_child_on_drop() { // ADR-056: dropping the TtyHandle (and thus the exit_code future) @@ -804,13 +899,18 @@ mod tests { let control = handle.control.as_ref().expect("control"); wait_marker(&marker).await; // "NOSUCH" is not a known signal name → falls back to - // ChildKiller::kill (SIGHUP). The child should die. + // ChildKiller::kill (SIGHUP). The child should die by SIGHUP, + // which resolves to `-1` — the documented ADR-004 sentinel + // collision (SIGHUP = signal 1). control.signal("NOSUCH"); let code = tokio::time::timeout(std::time::Duration::from_secs(5), handle.exit_code) .await .expect("exit timed out") .expect("exit_code"); - assert_ne!(code, 0, "fallback kill should terminate the child: {code}"); + assert_eq!( + code, -1, + "SIGHUP death resolves to the -1 sentinel value: {code}" + ); } #[cfg(unix)]