diff --git a/docs/reviews/001-code-review.md b/docs/reviews/001-code-review.md index bc3aa38..bd28bc3 100644 --- a/docs/reviews/001-code-review.md +++ b/docs/reviews/001-code-review.md @@ -534,7 +534,7 @@ Highlights: | M1 | negotiation-rejection frame unhandled | implement disambiguation read | medium | medium (wire-facing) | ✅ resolved | | L1 | channels `input` ignored | decide drop-vs-pass-through | small | low | ✅ resolved (2026-09-05) | | L3 | `open_via_channels` 0% covered | end-to-end channels consumer test | medium | low | ✅ resolved (2026-09-05) | -| L6 | pty bridge error paths untested | targeted error-path tests | medium | low | open | +| L6 | pty bridge error paths untested | targeted error-path tests | medium | low | ✅ resolved (2026-09-05) | | N4 | sleep-based timing | readiness signals | small | low | open | | N6 | MSRV unverified | CI MSRV job or bump | small | none | open | @@ -627,16 +627,39 @@ data flow). The channels harness (`wire_client_and_server`) moved to producer path (`register_openable` + `drive_session_pre_negotiated` through alkcall's channels stack). +### Resolution (2026-09-05, L6 — pty bridge error paths) + +Per the review's own disposition ("the `try_clone_reader`/`take_writer` +failure paths can be left as documented-unreachable if the publisher +agrees"), the bridge's error arms are now **documented-unreachable** +(module doc in `local/pty.rs` §"Bridge error paths"), with the +reasoning per arm: `try_clone_reader` needs a dup failure (exhausted +fd table — not deterministically forceable); a reader read error has +no trigger (EIO → EOF is mapped); `take_writer` only fails on a second +take (the bridge takes it once); a writer write/flush error requires +an externally-closed fd; the waiter `wait()` failure needs an +already-reaped child (the bridge never calls `try_wait`). The +ADR-055 §4 `-1` sentinel the review wanted tested is covered at the +adapter level (`exit_error_sends_minus_one` — the sentinel also arises +when the oneshot drops on kill-on-cancel, exercised by the +cancel-cleanup tests). + +The reachable fallback chain *is* now tested: +`signal_after_child_exit_takes_both_kill_fallbacks` — a late signal +(after the child exited) takes `kill(-pgid)` fail → `kill(pid)` fail → +warn + return, with no panic. `local/pty.rs` line coverage +80.85% → 86.01%; the remaining uncovered lines are exactly the +documented-unreachable arms plus `StdinSink`'s in-flight-parking path +(a single write cannot fill the 64-slot channel). + ### Remaining (open) -- **L6** — pty bridge error paths untested. - **N4** — sleep-based timing in signal/cancel tests. - **N6** — MSRV unverified. ### Recommended Order (remaining) -1. **L6** — pty bridge error paths; medium effort. -2. **N4 + N6** — test hardening and MSRV; defer until CI exists. +1. **N4 + N6** — test hardening and MSRV; defer until CI exists. --- diff --git a/src/local/pty.rs b/src/local/pty.rs index 365820f..5d2f9a0 100644 --- a/src/local/pty.rs +++ b/src/local/pty.rs @@ -34,6 +34,33 @@ //! guard (`Option::take()`), so the subsequent `Drop` is a no-op. The //! waiter thread reaps the killed child via its blocking `wait()`, so //! there is no zombie. See `tty-local.md` §"Cancel-Cleanup (ADR-056)". +//! +//! # Bridge error paths (review #001 L6) +//! +//! The three-thread bridge's error arms are documented-unreachable +//! through the public path, per the review's disposition: +//! +//! - **reader `try_clone_reader` failure** — the master fd is open and +//! owned by this allocation; `dup` failure requires an exhausted +//! fd table, which no test can force deterministically. +//! - **reader read error** — the `PtyFd` `Read` impl maps `EIO` +//! (slave closed) to EOF, the only realistic master read error; any +//! other error kind has no trigger from the public path. +//! - **writer `take_writer` failure** — portable-pty 0.9 fails this +//! only on the *second* take (`took_writer` flag); the bridge takes +//! it exactly once. +//! - **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 +//! `-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 +//! cancel-cleanup tests). The late-signal fallback chain +//! (`kill(-pgid)` fail → `kill(pid)` fail → warn) is tested by +//! `signal_after_child_exit_takes_both_kill_fallbacks`. use std::collections::HashMap; use std::future::Future; @@ -702,4 +729,27 @@ mod tests { .expect("exit_code"); assert_ne!(code, 0, "fallback kill should terminate the child: {code}"); } + + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn signal_after_child_exit_takes_both_kill_fallbacks() { + // The signal path's fallback chain (REQ-TTY-02): `kill(-pgid, sig)` + // fails once the child (and its group) is gone, then the + // `kill(pid, sig)` fallback fails too, and the call degrades to a + // warn + return — no panic, no cascade. A late signal (the client + // signals after the process already exited) must be safe. + let handle = allocate_pty( + term(), + vec!["echo".to_string(), "done".to_string()], + None, + env_default(), + ) + .expect("allocate"); + let control = handle.control.clone().expect("control"); + let code = handle.exit_code.await.expect("exit_code"); + assert_eq!(code, 0); + + control.signal("INT"); + control.signal("NOSUCH"); + } }