fix(local): PTY waiter reaps via libc::waitpid — ADR-004 exit encoding holds on the PTY path

- portable-pty 0.9.0's ExitStatus flattens a signal-terminated child to
  code 1 with the signal only as a strsignal name string, so a SIGTERM
  death reported {"code":1} — indistinguishable from a real exit 1 and
  a violation of ADR-004 decision 1's negative-code convention (review
  #004, consumer input from alkssh)
- on Unix the waiter now skips the portable-pty conversion entirely:
  libc::waitpid(pid) on the direct child, decoded WIFEXITED/WEXITSTATUS
  and WIFSIGNALED/-WTERMSIG (numeric, locale-independent); Child::wait()
  remains as the waitpid-failure fallback and the non-Unix primary
  (std::process::Child's Drop neither kills nor reaps, so the two paths
  never double-reap)
- signal features now round-trip: signal("TERM") -> -15, ADR-056 cancel
  kill (SIGHUP) -> -1 (the documented ADR-004 sentinel collision)
- tests: SIGINT -> -2, SIGTERM -> -15, SIGHUP fallback -> -1 pinned
- docs: ADR-004 decision 1, tty-local.md, ADR-005, tty-adapter.md,
  review #004 closed as resolved with disposition

Verification: cargo test (113), cargo test --all-features (138 + the
tests/pty.rs, tests/pipe.rs, tests/negotiation.rs integrations),
clippy --all-targets --all-features -D warnings, fmt, doc,
fuzz/shared corpus replay (9)
This commit is contained in:
glm-5.3-flash committed 2026-11-01 12:06:23 +00:00
1 parent d7a296d7bd
commit db0b1fce5c
6 files changed
+204 -50

No files matched your search

@@ -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
@@ -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.
+7 -5
View File
@@ -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.
+22 -11
View File
@@ -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<i32>`
with the exit code. The async-facing `TtyHandle.exit_code` is a
`Future` wrapping this `oneshot::Receiver<i32>` 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<i32>` 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<i32>` 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
@@ -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)
- 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<std::process::ExitStatus>` 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).
+124 -24
View File
@@ -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<i32>,
killer: Option<Box<dyn ChildKiller + Send + Sync>>,
@@ -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<i32>, child: &mut Box<dyn Child + Send + Sync>) -> 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<Mutex<Box<dyn MasterPty + Send>>> = 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<i32> ---
// --- Waiter thread: blocking wait → oneshot<i32> (ADR-004 encoding) ---
let (exit_tx, exit_rx) = oneshot::channel::<i32>();
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)]