diff --git a/libshpool/src/protocol.rs b/libshpool/src/protocol.rs index 1ab50cbb..e1ec02bb 100644 --- a/libshpool/src/protocol.rs +++ b/libshpool/src/protocol.rs @@ -395,6 +395,29 @@ impl Client { + (sock_to_stdout_h.is_finished() as usize); if nfinished_threads > 0 { + // A finished stdin->sock thread only means our stdin closed; + // the shell's exit status may still be in flight. Stamping + // the fallback now makes sock->stdout bail before reading + // the ExitStatus frame, reporting 1 instead of the real + // status, so wait briefly for the socket side. It returns as + // soon as the frame lands, not after the whole window. + if stdin_to_sock_h.is_finished() && !sock_to_stdout_h.is_finished() { + common::sleep_unless( + MAX_DETACH_WAIT_DUR, + || { + sock_to_stdout_h.is_finished() + || result_slot.lock().unwrap().is_some() + }, + common::PollStrategy::Backoff { + initial_interval: DETACH_BACKOFF_INITIAL_DUR, + factor: 2.0, + max_interval: DETACH_BACKOFF_MAX_STEP_DUR, + }, + ); + nfinished_threads = (stdin_to_sock_h.is_finished() as usize) + + (sock_to_stdout_h.is_finished() as usize); + } + // If one of the threads has exited, but not the other // make sure that the exit result slot has some contents // so the other thread will exit the next time it wakes diff --git a/shpool/tests/regression.rs b/shpool/tests/regression.rs index e8a8e14d..d950b5ee 100644 --- a/shpool/tests/regression.rs +++ b/shpool/tests/regression.rs @@ -1,6 +1,6 @@ use std::{fs, io::Write, process::Command, sync::mpsc, time::Duration}; -use anyhow::Context; +use anyhow::{anyhow, Context}; use ntest::timeout; mod support; @@ -334,3 +334,44 @@ fn pager_exit_transitions_to_shell() -> anyhow::Result<()> { Ok(()) } + +/// Regression test for the attach client discarding the shell's real exit +/// status. The client stamps its fallback status of 1 into the result slot as +/// soon as EITHER pipe thread finishes. A terminal that closes the client's +/// stdin before the shell's ExitStatus frame arrives -- which any terminal +/// does when it hangs up first -- therefore reported 1 no matter how the +/// shell actually exited. +/// +/// We close the client's stdin while the shell is still sleeping toward a +/// distinctive exit code, so the stdin->sock thread always finishes first. +#[test] +#[timeout(30000)] +fn stdin_close_does_not_discard_exit_status() -> anyhow::Result<()> { + let mut daemon_proc = support::daemon::Proc::new("norc.toml", DaemonArgs::default()) + .context("starting daemon proc")?; + + let mut attach_proc = + daemon_proc.attach("sh1", Default::default()).context("starting attach proc")?; + daemon_proc.await_event("daemon-bidi-stream-enter")?; + + let mut line_matcher = attach_proc.line_matcher()?; + attach_proc.run_cmd("echo ready")?; + line_matcher.scan_until_re("ready$")?; + + // MAX_DETACH_WAIT_DUR is 300ms: the frame must arrive inside the bounded + // window the client waits after its stdin side finishes. + attach_proc.run_cmd("sleep 0.15; exit 19")?; + // Close our stdin immediately: the fallback status must not win the race + // against the ExitStatus frame that arrives moments later. + drop(attach_proc.proc.stdin.take()); + + let code = attach_proc + .proc + .wait() + .context("waiting for attach proc to exit")? + .code() + .ok_or(anyhow!("no exit code"))?; + assert_eq!(code, 19, "shell exit status was discarded"); + + Ok(()) +}