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
20 changes: 12 additions & 8 deletions src/agent/tmux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,11 +496,14 @@
// down. Measured, tmux 3.5a: with `set-option -w -g window-size
// manual`, *every* `new-window` on a server with no attached client
// answered `server exited unexpectedly`; with the same option said per
// window it answers with a pane id. Unguarded in every release that has
// the option (3.3 … 3.6; guarded only on tmux master), and 3.2a — the
// supported floor — predates it. Stating it after the window exists is
// what `main` did by accident, where a session-scoped write landed on
// the session's current window and on no other.
// window it answers with a pane id. Unguarded in 3.3 … 3.6 (guarded
// only on tmux master). The option itself is older than the supported
// floor — tmux 2.9 added it, `manual` and per-window `setw` included
// (`CHANGES`, 2.8 → 2.9) — so it needs no version gate: measured, tmux
// 3.2 and 3.2a accept it chained after `new-window`, and survive it
// server-wide as well. Stating it after the window exists is what
// `main` did by accident, where a session-scoped write landed on the
// session's current window and on no other.
("window-size", "manual"),
]
}
Expand Down Expand Up @@ -903,14 +906,14 @@
bail!("Cannot parse tmux version from: {version_str}");
}

let major: u32 = parts[0].parse().context(format!(
"Cannot parse tmux major version from: {version_str}"
))?;

Check warning on line 911 in src/agent/tmux.rs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use "with_context" so this context value is only produced when the operation fails.

See more on https://sonarcloud.io/project/issues?id=Thurbeen_thurbox&issues=AaCXB5rSWi3cD0H5vRWT&open=AaCXB5rSWi3cD0H5vRWT&pullRequest=1113
// Minor might have a trailing letter (e.g., "3a"), strip non-digits.
let minor_str: String = parts[1].chars().take_while(char::is_ascii_digit).collect();
let minor: u32 = minor_str.parse().context(format!(
"Cannot parse tmux minor version from: {version_str}"
))?;

Check warning on line 916 in src/agent/tmux.rs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use "with_context" so this context value is only produced when the operation fails.

See more on https://sonarcloud.io/project/issues?id=Thurbeen_thurbox&issues=AaCXB5rSWi3cD0H5vRWU&open=AaCXB5rSWi3cD0H5vRWU&pullRequest=1113

Ok((major, minor))
}
Expand Down Expand Up @@ -4342,9 +4345,10 @@
/// (`spawn_window` → `default_window_size(…, w = NULL)`) and the manual
/// branch of `clients_calculate_size` reads `w->manual_sx` with no NULL
/// check, so a server whose default is `manual` dies on the next
/// `new-window` from an unattached client — every release that has the
/// option (3.3 … 3.6). Measured on 3.5a: `server exited unexpectedly` every
/// time with the server-wide write, a pane id every time without it.
/// `new-window` from an unattached client — 3.3 … 3.6. Measured on 3.5a:
/// `server exited unexpectedly` every time with the server-wide write, a
/// pane id every time without it. 3.2 and 3.2a have the option too and
/// survive the server-wide write (measured).
#[test]
fn the_server_wide_window_options_do_not_size_windows_by_hand() {
for (key, value) in WINDOW_OPTS {
Expand Down
70 changes: 44 additions & 26 deletions tests/program_pane_exit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const SOCKET: &str = "thurbox-program-exit-e2e";

/// Generous next to the notification, which arrives with the exit: the budget is
/// for a loaded machine starting a tmux server, not for the signal itself.
const DEADLINE: Duration = Duration::from_secs(10);
const DEADLINE: Duration = Duration::from_secs(20);

fn have_tmux() -> bool {
Command::new("tmux")
Expand All @@ -58,25 +58,10 @@ fn cleanup() {
.output();
}

/// A tokio runtime is required, not decorative: wiring a pane spawns its writer
/// task, and without one the spawn panics before anything can be observed.
#[tokio::test(flavor = "multi_thread")]
async fn a_program_that_ends_reports_that_it_ended() {
if !have_tmux() {
eprintln!("skipping: tmux is not installed");
return;
}

let dir = tempfile::tempdir().expect("tempdir");
// nextest runs one process per test, so process-wide env is safe here.
std::env::set_var("TMUX_TMPDIR", dir.path());
std::env::set_var(SOCKET_OVERRIDE_ENV, SOCKET);
// The override is dropped when it was injected for someone else's data dir
// (see `socket_for`); this test *is* somebody typing it.
std::env::remove_var(SOCKET_OWNER_ENV);
thurbox::paths::set_test_dir(dir.path());

cleanup();
/// Starts the session with **`remain-on-exit on`** (see the note at the top),
/// kept out of the async test body: a blocking `Command::output` call written
/// directly in an `async fn` blocks the executor thread it runs on.
Comment thread
LeTuR marked this conversation as resolved.
fn start_session(dir: &std::path::Path) -> std::process::Output {
let started = Command::new("tmux")
.args([
"-L",
Expand All @@ -90,10 +75,9 @@ async fn a_program_that_ends_reports_that_it_ended() {
"-y",
"24",
])
.env("TMUX_TMPDIR", dir.path())
.env("TMUX_TMPDIR", dir)
.output()
.expect("run tmux");
// The session thurbox actually runs: see the note at the top.
let _ = Command::new("tmux")
.args([
"-L",
Expand All @@ -104,9 +88,32 @@ async fn a_program_that_ends_reports_that_it_ended() {
"remain-on-exit",
"on",
])
.env("TMUX_TMPDIR", dir.path())
.env("TMUX_TMPDIR", dir)
.output()
.expect("run tmux");
started
}

/// A tokio runtime is required, not decorative: wiring a pane spawns its writer
/// task, and without one the spawn panics before anything can be observed.
#[tokio::test(flavor = "multi_thread")]
async fn a_program_that_ends_reports_that_it_ended() {
if !have_tmux() {
eprintln!("skipping: tmux is not installed");
return;
}

let dir = tempfile::tempdir().expect("tempdir");
// nextest runs one process per test, so process-wide env is safe here.
std::env::set_var("TMUX_TMPDIR", dir.path());
std::env::set_var(SOCKET_OVERRIDE_ENV, SOCKET);
// The override is dropped when it was injected for someone else's data dir
// (see `socket_for`); this test *is* somebody typing it.
std::env::remove_var(SOCKET_OWNER_ENV);
thurbox::paths::set_test_dir(dir.path());

cleanup();
let started = start_session(dir.path());
if !started.status.success() {
cleanup();
eprintln!(
Expand All @@ -127,12 +134,23 @@ async fn a_program_that_ends_reports_that_it_ended() {
// is not padding: a program that exits instantly takes its pane with it
// before `spawn` can size the window, and the spawn fails with "can't find
// pane" — which this test used to report as a skipped environment and pass
// on, proving nothing at all.
// on, proving nothing at all. It also has to outlive `TmuxBackend::register_pane`'s
// own `display-message` round trip (registering which window the pane's
// death will be announced on, so the notification has somewhere to land):
// that round trip's own doc comment spells out that a program which exits
// before it returns costs this pane its exit notification *permanently* —
// the one-shot `%window-close` for that window has already come and gone
// with nothing yet in `pane_windows` to match it against, and no later
// wait, however long, makes it arrive a second time. On a loaded machine
// (this test's own failure mode: a full parallel `nextest` run stacking
// dozens of other tmux/pty-driving tests against a shared CPU budget) that
// round trip can stretch well past a few seconds. 1s cut it close, 3s
// still lost the race once in a large run; 8s gives it real headroom.
let pane = ProgramPane::spawn(
std::sync::Arc::clone(&backend) as std::sync::Arc<dyn SessionBackend>,
"tbp-test-exiting",
"sh",
&["-c".to_string(), "printf started; sleep 1".to_string()],
&["-c".to_string(), "printf started; sleep 8".to_string()],
Some(dir.path()),
&HashMap::new(),
24,
Expand All @@ -151,7 +169,7 @@ async fn a_program_that_ends_reports_that_it_ended() {

let deadline = Instant::now() + DEADLINE;
while !pane.has_exited() && Instant::now() < deadline {
std::thread::sleep(Duration::from_millis(50));
tokio::time::sleep(Duration::from_millis(50)).await;
}
let exited = pane.has_exited();
cleanup();
Expand Down
9 changes: 6 additions & 3 deletions tests/program_restart_exit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,11 @@ async fn restarting_a_finished_program_still_reports_the_ending() {
// Lives for a moment, then ends on its own — an editor being quit. Not
// instant: a program that exits before tmux has sized the window takes the
// pane with it and the spawn fails outright, which would turn this into a
// skip that proves nothing.
let short = ["-c".to_string(), "printf started; sleep 1".to_string()];
// skip that proves nothing. It also has to outlive the pane registration's
// own `display-message` round trip, which can stretch well past a second
// on a loaded machine — 1s cut that close under a full parallel `nextest`
// run; 3s gives it real headroom.
let short = ["-c".to_string(), "printf started; sleep 3".to_string()];
if let Err(e) = terminals.start_program(&key, "sh", &short, Some(dir.path()), 24, 80) {
cleanup();
// Not a skip: tmux is installed, so a pane that would not start is the
Expand All @@ -85,7 +88,7 @@ async fn restarting_a_finished_program_still_reports_the_ending() {
};
let deadline = Instant::now() + DEADLINE;
while !exited(&terminals) && Instant::now() < deadline {
std::thread::sleep(Duration::from_millis(50));
tokio::time::sleep(Duration::from_millis(50)).await;
}
if !exited(&terminals) {
cleanup();
Expand Down
12 changes: 7 additions & 5 deletions tests/window_remain_on_exit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ async fn an_agent_that_dies_at_once_still_leaves_its_window() {
};

// Long enough for the corpse to be reaped if it was ever going to be.
std::thread::sleep(std::time::Duration::from_millis(500));
tokio::time::sleep(std::time::Duration::from_millis(500)).await;
let listed = pane_of("tb-");
let retention = remain_on_exit(&pane);
cleanup();
Expand Down Expand Up @@ -340,7 +340,7 @@ async fn an_older_namesake_does_not_take_the_new_windows_retention() {
}
};

std::thread::sleep(std::time::Duration::from_millis(500));
tokio::time::sleep(std::time::Duration::from_millis(500)).await;
let retention = remain_on_exit(&pane);
let listed = tmux(&["list-windows", "-a", "-F", "#{pane_id}"]);
let alive = String::from_utf8_lossy(&listed.stdout)
Expand Down Expand Up @@ -370,9 +370,11 @@ async fn an_older_namesake_does_not_take_the_new_windows_retention() {
/// every headless spawn. Measured, tmux 3.5a: with `set-option -w -g
/// window-size manual` every `new-window` answered `server exited
/// unexpectedly`; with the same option said per window, a pane id every time.
/// Unguarded in 3.3 through 3.6 and guarded only on tmux master; 3.2a — the
/// supported floor — predates the option, which is why a machine with 3.2a
/// cannot see the failure at all.
/// Unguarded in 3.3 through 3.6 and guarded only on tmux master. The supported
/// floor has the option — tmux 2.9 added it (`CHANGES`, 2.8 → 2.9) — but not
/// the crash: measured, tmux 3.2 and 3.2a accept the per-window write and
/// survive the server-wide one, so a machine with 3.2 cannot see the failure at
/// all.
///
/// The assertion is therefore about the *configuration*, not the crash: it is
/// the one form that fails the same way on every tmux, including the one this
Expand Down