diff --git a/CHANGELOG.md b/CHANGELOG.md index 98ce7c21..07fb7387 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,9 +72,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Two things had to be fixed for this to work at all, both invisible until something could actually select a distribution. WSL kills what an interop session started the moment its `wsl.exe` exits, and `setsid` does not make the - new daemon safe instantly — so the launch now holds its invocation open for a - beat, instead of reporting "started but nothing was answering on the control - socket" over a correctly installed binary. And a pane's connection now asks + new daemon safe instantly — so the launch now holds its invocation open until + the daemon answers, instead of reporting "started but nothing was answering on + the control socket" over a correctly installed binary. And a pane's connection now asks for the remote's *pane* socket: the flag that says so was added by the SSH path and by the local `--stdio` one, but never by WSL, so the workspace would connect and the window would open with a pane that could not reach the machine. diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index c7c30954..ab19d654 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -143,6 +143,16 @@ pub trait RemoteOps: Send + Sync { /// for it to exit. Used only for the daemon launch, which by design never /// exits. fn spawn_detached(&self, cmd: &str) -> Result<(), String>; + /// A wait to run in the *same* invocation as the daemon launch, right after + /// the launch line, for a transport that cannot let the invocation return + /// while the daemon is still fragile. `binary` is the server just launched. + /// + /// Defaulted to nothing, because SSH needs nothing: closing an exec channel + /// is unhurried and a backgrounded daemon survives it. WSL is the one + /// transport that does — see [`wsl::launch_settle`](super::install::wsl). + fn launch_settle(&self, _binary: &str) -> Option { + None + } /// `stat` following symlinks. `Ok(None)` means "not there", which is a normal /// answer, not an error. fn stat(&self, path: &str) -> Result, String>; @@ -1293,8 +1303,9 @@ impl<'a> Installer<'a> { } fn launch_daemon(&self, paths: &RemotePaths) -> Result<(), InstallError> { + let settle = self.ops.launch_settle(&paths.binary); self.ops - .spawn_detached(&launch_command(&paths.binary)) + .spawn_detached(&launch_script(&paths.binary, settle)) .map_err(|reason| InstallError::Launch { reason }) } @@ -1419,6 +1430,21 @@ fn launch_command(binary: &str) -> String { ) } +/// The launch line, plus whatever wait the transport asked for in +/// [`RemoteOps::launch_settle`]. +/// +/// A free function so the one thing that must never happen — the wait replacing +/// the launch instead of following it — is testable without a machine of any +/// kind. A daemon that was never launched fails exactly like one that was +/// reaped, so this is cheap to get wrong and expensive to notice. +fn launch_script(binary: &str, settle: Option) -> String { + let launch = launch_command(binary); + match settle { + Some(settle) => format!("{launch}\n{settle}"), + None => launch, + } +} + /// POSIX single-quote escaping. Home directories with spaces, apostrophes or /// `$` in them are rare but real, and every command here interpolates a path. pub(crate) fn shell_quote(s: &str) -> String { diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 43f224b3..fe8d7db5 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -986,6 +986,29 @@ fn the_launch_command_detaches_and_closes_every_stream() { ); } +/// A transport's settle **follows** the launch; it never replaces it. Cheap to +/// get wrong in a `format!` and expensive to notice, because a daemon that was +/// never launched fails exactly like one that died right after being launched. +/// +/// And a transport that asks for nothing — every one but WSL — gets the launch +/// line by itself, with no trailing newline to change what the shell reads. +#[test] +fn a_launch_settle_follows_the_launch_and_never_replaces_it() { + let plain = launch_script(BINARY, None); + assert_eq!(plain, launch_command(BINARY), "no settle, no wrapping"); + + let settled = launch_script(BINARY, Some("sleep 1\n".to_string())); + assert!( + settled.starts_with(&plain), + "the launch survives: {settled}" + ); + assert!(settled.contains("--daemon"), "{settled}"); + assert!( + settled.ends_with("sleep 1\n"), + "the settle is last: {settled}" + ); +} + /// Every command interpolates a remote path, and home directories with spaces /// or apostrophes exist. Unquoted, `/home/o'brien/...` would end the string /// mid-path and run whatever followed. diff --git a/crates/tty7-core/src/daemon/install/wsl.rs b/crates/tty7-core/src/daemon/install/wsl.rs index dc3ebd63..da4eb457 100644 --- a/crates/tty7-core/src/daemon/install/wsl.rs +++ b/crates/tty7-core/src/daemon/install/wsl.rs @@ -72,7 +72,8 @@ const MARKER: &str = "__tty7_wsl__"; /// stopped distribution pays for booting its VM and init. const COMMAND_TIMEOUT: Duration = Duration::from_secs(60); /// Budget for the daemon launch. It backgrounds and returns, but not -/// immediately — see [`DETACH_SETTLE`], which holds the invocation open. +/// immediately — see [`launch_settle`], which holds the invocation open until the +/// daemon answers. const LAUNCH_TIMEOUT: Duration = Duration::from_secs(30); /// Budget for writing the server binary — a few megabytes over a pipe, plus /// whatever the distribution's disk is doing. @@ -285,8 +286,17 @@ const HOME_SCRIPT: &str = "printf '__tty7_wsl__ home=%s\\n' \"$HOME\"\n"; /// distribution whose default user has no home directory. const CD_HOME: &str = "cd \"$HOME\" 2>/dev/null || cd /\n"; -/// Appended after the daemon launch line so `wsl.exe` does not exit while the -/// daemon is still detaching. +/// How many times the settle asks the daemon whether it is serving, and how long +/// it waits between asks — a little over five seconds in all, generous for a +/// distribution that is booting its VM, and far inside [`LAUNCH_TIMEOUT`]. +const SETTLE_TRIES: u32 = 25; +const SETTLE_STEP: &str = "0.2"; +/// How long to hold the invocation open *after* the daemon answers. See +/// [`launch_settle`]. +const SETTLE_GRACE: &str = "0.3"; + +/// The wait that runs after the daemon launch, in the same `wsl.exe` +/// invocation, so `wsl.exe` does not exit while the daemon is still detaching. /// /// **WSL reaps what an interop session started when that session's `wsl.exe` /// exits**, and `setsid` does not make the child safe the instant it runs. @@ -306,30 +316,52 @@ const CD_HOME: &str = "cd \"$HOME\" 2>/dev/null || cd /\n"; /// /// The SSH path needs none of this (an exec channel's close is unhurried), which /// is why the shared [`launch_command`](super::launch_command) stays as it is -/// and this lives here. +/// and this is a [`RemoteOps::launch_settle`](super::RemoteOps::launch_settle) +/// instead. /// -/// # Why a flat wait and not a wait *for the socket* +/// # It waits for an answer, not for a fixed number of seconds /// -/// Polling `/daemon.sock` until it appears looks like the obvious -/// improvement, and it is wrong. [`ensure_daemon`](super::Installer::ensure_daemon) -/// only reaches the launch when the socket did **not** answer, and the ordinary -/// reason for that is a socket file a dead daemon left behind — `wsl --shutdown` -/// is a routine thing to run. A `-S` test would pass on that stale file -/// immediately, skip the wait, and restore the bug in exactly the state that -/// produced it. +/// A flat `sleep` is a bet on how long a distribution takes to detach, and the +/// one that loses that bet is the cold or loaded distribution — the same +/// condition the failure needed in the first place. So the wait asks the daemon +/// the only question that settles it, `--stdio --bridge`, exactly as +/// [`ensure_daemon`](super::Installer::ensure_daemon) asks it from this side, and +/// stops as soon as it is answered. The normal case is therefore *shorter* than +/// the flat second it replaces, and the cold case is allowed the seconds it +/// actually needs instead of failing with nothing to go on. /// -/// One second, because 0.3 was enough in the reproduction and this is paid only -/// on a launch — which happens when no daemon is already serving the -/// distribution, not on every connect. -const DETACH_SETTLE: &str = "sleep 1\n"; +/// **Waiting on `/daemon.sock` would be wrong**, which is why this +/// waits on the answer and not on the file. +/// [`ensure_daemon`](super::Installer::ensure_daemon) only reaches the launch +/// when the socket did *not* answer, and the ordinary reason for that is a socket +/// file a dead daemon left behind — `wsl --shutdown` is a routine thing to run. +/// A `-S` test would pass on that stale file immediately, skip the wait, and +/// restore the bug in exactly the state that produced it. A daemon that answers +/// cannot be a leftover file. +/// +/// [`SETTLE_GRACE`] follows the answer because what makes the daemon safe is not +/// observable from here — 0.3s of *any* wait was enough in the reproduction, so +/// the answer is followed by that much regardless. +pub(super) fn launch_settle(binary: &str) -> String { + settle_script(binary, SETTLE_TRIES, SETTLE_STEP, SETTLE_GRACE) +} -/// The daemon launch line plus its [`DETACH_SETTLE`] wait. -/// -/// Split out from [`WslRemoteOps::spawn_detached`] so the composition is -/// testable without a distribution: the one thing that must never happen is the -/// wait replacing the launch rather than following it. -fn launch_script(cmd: &str) -> String { - format!("{cmd}\n{DETACH_SETTLE}") +/// [`launch_settle`] with its budget spelled out, so a test can run the real +/// script against a real `sh` without waiting out the real budget. +fn settle_script(binary: &str, tries: u32, step: &str, grace: &str) -> String { + let bin = shell_quote(binary); + // `< /dev/null` on the probe matters twice over: it is what makes `--bridge` + // answer and exit rather than proxy, and this script itself arrives on the + // shell's stdin, so a probe left reading stdin would eat the rest of it. + format!( + "__tty7_settle=0\n\ + while [ \"$__tty7_settle\" -lt {tries} ]; do\n\ + if {bin} --stdio --bridge < /dev/null > /dev/null 2>&1; then break; fi\n\ + __tty7_settle=$((__tty7_settle + 1))\n\ + sleep {step}\n\ + done\n\ + sleep {grace}\n" + ) } /// `HOME_SCRIPT` has to spell the marker out, because a `const` cannot @@ -638,7 +670,12 @@ impl RemoteOps for WslRemoteOps { // The status is ignored for the same reason it is over SSH: the script // reports on the backgrounding, never on the daemon. Whether the daemon // came up is settled by probing its socket. - self.sh(&launch_script(cmd), LAUNCH_TIMEOUT).map(|_| ()) + self.sh(cmd, LAUNCH_TIMEOUT).map(|_| ()) + } + + /// WSL is the transport that needs one. See [`launch_settle`]. + fn launch_settle(&self, binary: &str) -> Option { + Some(launch_settle(binary)) } fn stat(&self, path: &str) -> Result, String> { @@ -1164,41 +1201,100 @@ mod tests { dir } - /// The wait follows the launch; it never replaces it. Cheap to get wrong in - /// a `format!` and expensive to notice — a daemon that is never launched - /// fails exactly like one that was reaped. - #[test] - fn the_launch_script_keeps_the_launch_and_appends_the_wait() { - let script = launch_script("setsid 'tty7-server' --daemon &"); - let (launch, wait) = script.split_once('\n').expect("two parts"); - assert_eq!(launch, "setsid 'tty7-server' --daemon &"); - assert_eq!(wait.trim(), "sleep 1"); + /// A stand-in for the remote `tty7-server`: a script that answers the + /// settle's probe (`code` 0) or never answers it (anything else). + #[cfg(unix)] + fn fake_server(dir: &std::path::Path, name: &str, code: u8) -> String { + use std::os::unix::fs::PermissionsExt as _; + let path = dir.join(name); + std::fs::write(&path, format!("#!/bin/sh\nexit {code}\n")).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + path.to_str().unwrap().to_string() } - /// **The settle wait, executed**: the launch still runs, the invocation is - /// really held afterwards, and it is held for a beat rather than for the - /// whole budget its caller allows. + /// The settle asks the daemon with *the daemon's own binary*, quoted, in the + /// control dialect — the same question + /// [`Installer::ensure_daemon`] asks from this side. Not `--pane` (that is + /// the other socket and would answer for the wrong thing) and not a test on + /// the socket file, which a stale one passes. + #[test] + fn the_settle_asks_the_control_dialect_with_the_quoted_binary() { + let script = settle_script("/home/a b/tty7-server-26.7.6", 4, "0.2", "0.3"); + assert!( + script.contains("'/home/a b/tty7-server-26.7.6' --stdio --bridge < /dev/null"), + "{script}" + ); + assert!(!script.contains("--pane"), "{script}"); + assert!(!script.contains("-S "), "no file test: {script}"); + assert!(script.contains("-lt 4"), "{script}"); + } + + /// **The settle, executed against a daemon that answers.** It stops on the + /// first answer and still holds the invocation for [`SETTLE_GRACE`] — so the + /// normal launch is *shorter* than the flat second this replaced, and the + /// wait is still really there. /// - /// The middle assertion is the one with teeth. A wait that silently became a - /// no-op — a `sleep` a distro rejects, a line lost in a `format!` — reads as - /// a passing test everywhere except against a real distribution, which is + /// This is the assertion with teeth. A settle that silently became a no-op — + /// a `sleep` a distro rejects, a line lost in a `format!` — reads as a + /// passing test everywhere except against a real distribution, which is /// where it already cost an afternoon. #[cfg(unix)] #[test] - fn the_settle_wait_really_holds_the_invocation_open() { + fn the_settle_stops_as_soon_as_the_daemon_answers() { + let dir = sh_scratch("settle-answers"); + let serving = fake_server(&dir, "tty7-server-answers", 0); + let started = std::time::Instant::now(); - let (out, ok) = real_sh(&launch_script("echo launched")); + let (out, ok) = real_sh(&settle_script(&serving, SETTLE_TRIES, SETTLE_STEP, "0.3")); let elapsed = started.elapsed(); assert!(ok, "{out}"); - assert!(out.contains("launched"), "the launch still ran: {out:?}"); assert!( - elapsed >= Duration::from_millis(900), - "the wait did not happen: {elapsed:?}" + elapsed >= Duration::from_millis(250), + "the grace after the answer did not happen: {elapsed:?}" ); assert!( - elapsed < LAUNCH_TIMEOUT, - "the wait outlived the budget its caller gives the whole launch" + elapsed < Duration::from_millis(1500), + "it kept waiting after the daemon had answered: {elapsed:?}" + ); + } + + /// **The settle, executed against a daemon that never answers.** It spends + /// its whole budget and then returns — a launch that failed has to become + /// `ensure_daemon`'s verdict, not a hung invocation. + #[cfg(unix)] + #[test] + fn the_settle_gives_up_rather_than_hanging() { + let dir = sh_scratch("settle-silent"); + let silent = fake_server(&dir, "tty7-server-silent", 1); + + let started = std::time::Instant::now(); + let (out, ok) = real_sh(&settle_script(&silent, 3, "0.1", "0.1")); + let elapsed = started.elapsed(); + + assert!(ok, "{out}"); + assert!( + elapsed >= Duration::from_millis(350), + "it did not ask three times: {elapsed:?}" + ); + assert!( + elapsed < Duration::from_secs(5), + "it never gave up: {elapsed:?}" + ); + } + + /// The real budget stays well inside the one its caller allows the *whole* + /// launch, or a slow daemon turns into a killed invocation instead of a + /// launched one. Also the guard that both steps are still numbers a `sleep` + /// accepts. + #[test] + fn the_settle_budget_fits_inside_the_launch_timeout() { + let step: f64 = SETTLE_STEP.parse().expect("a number for `sleep`"); + let grace: f64 = SETTLE_GRACE.parse().expect("a number for `sleep`"); + let worst = f64::from(SETTLE_TRIES) * step + grace; + assert!( + worst < LAUNCH_TIMEOUT.as_secs_f64() / 2.0, + "{worst}s of settle inside a {LAUNCH_TIMEOUT:?} launch budget" ); }