Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions libshpool/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 42 additions & 1 deletion shpool/tests/regression.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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(())
}
Loading