diff --git a/app/src/remote_server/server_model.rs b/app/src/remote_server/server_model.rs index f4319059..36d397cb 100644 --- a/app/src/remote_server/server_model.rs +++ b/app/src/remote_server/server_model.rs @@ -511,14 +511,18 @@ impl ServerModel { } /// Handles `Initialize` by returning the server version and host id. + /// + /// `server_version` is the release tag the daemon was built from + /// (`GIT_RELEASE_TAG`) or the empty string for `cargo run` / locally + /// deployed builds. The client treats an empty version as "unknown" and + /// skips strict version enforcement, which keeps the + /// `script/deploy_remote_server` developer workflow functional. fn handle_initialize(&mut self, msg: Initialize, request_id: &RequestId) -> HandlerOutcome { log::info!("Handling Initialize (request_id={request_id})"); if !msg.auth_token.is_empty() { self.auth_token = Some(msg.auth_token); } - let server_version = ChannelState::app_version() - .unwrap_or(env!("CARGO_PKG_VERSION")) - .to_string(); + let server_version = ChannelState::app_version().unwrap_or("").to_string(); HandlerOutcome::Sync(server_message::Message::InitializeResponse( InitializeResponse { server_version, diff --git a/app/src/remote_server/ssh_transport.rs b/app/src/remote_server/ssh_transport.rs index a98c6fe4..6bd1cc33 100644 --- a/app/src/remote_server/ssh_transport.rs +++ b/app/src/remote_server/ssh_transport.rs @@ -7,7 +7,6 @@ use std::fmt; use std::future::Future; use std::path::PathBuf; use std::pin::Pin; -use std::process::Stdio; use std::sync::Arc; use anyhow::Result; @@ -15,10 +14,7 @@ use warpui::r#async::executor; use remote_server::auth::RemoteServerAuthContext; use remote_server::client::RemoteServerClient; -use remote_server::setup::{ - self, remote_server_daemon_dir, RemotePlatform, CHECK_TIMEOUT, INSTALL_TIMEOUT, -}; -use remote_server::ssh::{run_ssh_command, run_ssh_script, ssh_args}; +use remote_server::setup::{parse_uname_output, remote_server_daemon_dir, RemotePlatform}; use remote_server::transport::{Connection, RemoteTransport}; /// SSH transport: connects via a ControlMaster socket. @@ -81,10 +77,16 @@ impl RemoteTransport for SshTransport { ) -> Pin> + Send>> { let socket_path = self.socket_path.clone(); Box::pin(async move { - match run_ssh_command(&socket_path, "uname -sm", CHECK_TIMEOUT).await { + match remote_server::ssh::run_ssh_command( + &socket_path, + "uname -sm", + remote_server::setup::CHECK_TIMEOUT, + ) + .await + { Ok(output) if output.status.success() => { let stdout = String::from_utf8_lossy(&output.stdout); - setup::parse_uname_output(&stdout).map_err(|e| format!("{e:#}")) + parse_uname_output(&stdout).map_err(|e| format!("{e:#}")) } Ok(output) => { let code = output.status.code().unwrap_or(-1); @@ -99,10 +101,17 @@ impl RemoteTransport for SshTransport { fn check_binary(&self) -> Pin> + Send>> { let socket_path = self.socket_path.clone(); Box::pin(async move { - let bin_path = setup::remote_server_binary(); + let bin_path = remote_server::setup::remote_server_binary(); log::info!("Checking for remote server binary at {bin_path}"); - match run_ssh_command(&socket_path, &setup::binary_check_command(), CHECK_TIMEOUT).await + match remote_server::ssh::run_ssh_command( + &socket_path, + &remote_server::setup::binary_check_command(), + remote_server::setup::CHECK_TIMEOUT, + ) + .await { + // `test -x` exits 0 when present, 1 when missing. + // Any other exit code (or None / signal) is treated as a check failure. Ok(output) => match output.status.code() { Some(0) => Ok(true), Some(1) => Ok(false), @@ -117,15 +126,54 @@ impl RemoteTransport for SshTransport { }) } + fn check_has_old_binary(&self) -> Pin> + Send>> { + let socket_path = self.socket_path.clone(); + Box::pin(async move { + // Treat the existence of the remote-server install directory + // itself as evidence of a prior install. If `~/.warp-XX/remote-server` + // exists, something was installed there before, so any mismatch + // with the client's expected binary path should be auto-updated + // rather than surfaced as a first-time install prompt. + let cmd = format!("test -d {}", remote_server::setup::remote_server_dir()); + let output = remote_server::ssh::run_ssh_command( + &socket_path, + &cmd, + remote_server::setup::CHECK_TIMEOUT, + ) + .await?; + // `test -d` exits 0 when present, 1 when missing. + // Anything else is treated as a check failure. + match output.status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + Some(code) => { + let stderr = String::from_utf8_lossy(&output.stderr); + Err(anyhow::anyhow!( + "remote-server dir check exited with code {code}: {stderr}" + )) + } + None => Err(anyhow::anyhow!( + "remote-server dir check terminated by signal" + )), + } + }) + } + fn install_binary(&self) -> Pin> + Send>> { let socket_path = self.socket_path.clone(); Box::pin(async move { - let script = setup::install_script(); + let script = remote_server::setup::install_script(); log::info!( "Installing remote server binary to {}", - setup::remote_server_binary() + remote_server::setup::remote_server_binary() ); - match run_ssh_script(&socket_path, &script, INSTALL_TIMEOUT).await { + match remote_server::ssh::run_ssh_script( + &socket_path, + &script, + remote_server::setup::INSTALL_TIMEOUT, + ) + .await + { Ok(output) if output.status.success() => Ok(()), Ok(output) => { let code = output.status.code().unwrap_or(-1); @@ -142,10 +190,10 @@ impl RemoteTransport for SshTransport { executor: Arc, ) -> Pin> + Send>> { let socket_path = self.socket_path.clone(); - let remote_proxy_command = self.remote_proxy_command(); Box::pin(async move { - let mut args = ssh_args(&socket_path); - args.push(remote_proxy_command); + let binary = remote_server::setup::remote_server_binary(); + let mut args = remote_server::ssh::ssh_args(&socket_path); + args.push(format!("{binary} remote-server-proxy")); // `kill_on_drop(true)` pairs with ownership of the `Child` being // returned in the [`Connection`] below: the @@ -154,9 +202,9 @@ impl RemoteTransport for SshTransport { // spontaneous disconnect) sends SIGKILL to this ssh process. let mut child = command::r#async::Command::new("ssh") .args(&args) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) .kill_on_drop(true) .spawn()?; @@ -183,6 +231,28 @@ impl RemoteTransport for SshTransport { }) }) } + + fn remove_remote_server_binary( + &self, + ) -> Pin> + Send>> { + let socket_path = self.socket_path.clone(); + Box::pin(async move { + let cmd = format!("rm -f {}", remote_server::setup::remote_server_binary()); + log::info!("Removing stale remote server binary: {cmd}"); + let output = remote_server::ssh::run_ssh_command( + &socket_path, + &cmd, + remote_server::setup::CHECK_TIMEOUT, + ) + .await?; + if output.status.success() { + Ok(()) + } else { + let stderr = String::from_utf8_lossy(&output.stderr); + Err(anyhow::anyhow!("Failed to remove binary: {stderr}")) + } + }) + } } #[cfg(test)] diff --git a/app/src/terminal/prompt_render_helper.rs b/app/src/terminal/prompt_render_helper.rs index dee7ab95..1a07642d 100644 --- a/app/src/terminal/prompt_render_helper.rs +++ b/app/src/terminal/prompt_render_helper.rs @@ -256,10 +256,13 @@ impl PromptRenderHelper { RemoteServerSetupState::Checking => "Starting shell...".to_string(), RemoteServerSetupState::Installing { progress_percent: Some(p), - } => format!("Installing Warp SSH tools... ({p}%)"), + } => format!("Installing Warp SSH Extension... ({p}%)"), RemoteServerSetupState::Installing { progress_percent: None, - } => "Installing Warp SSH tools...".to_string(), + } => "Installing Warp SSH Extension...".to_string(), + RemoteServerSetupState::Updating => { + "Updating Warp SSH Extension...".to_string() + } RemoteServerSetupState::Initializing => "Initializing...".to_string(), RemoteServerSetupState::Ready => "Starting shell...".to_string(), RemoteServerSetupState::Failed { .. } => "Starting shell...".to_string(), diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index 187d8379..bb1cdcd7 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -4344,6 +4344,7 @@ impl TerminalView { session_id, result, remote_platform, + has_old_binary: _, } => { let (remote_os, remote_arch) = remote_platform .as_ref() @@ -11496,6 +11497,7 @@ impl TerminalView { RemoteServerSetupState::Installing { progress_percent: None, } => "Installing...".to_string(), + RemoteServerSetupState::Updating => "Updating...".to_string(), RemoteServerSetupState::Initializing => "Initializing...".to_string(), _ => "Starting shell...".to_string(), }) diff --git a/app/src/terminal/writeable_pty/remote_server_controller.rs b/app/src/terminal/writeable_pty/remote_server_controller.rs index 8777bc32..fbe33c70 100644 --- a/app/src/terminal/writeable_pty/remote_server_controller.rs +++ b/app/src/terminal/writeable_pty/remote_server_controller.rs @@ -42,11 +42,15 @@ enum SshInitState { setup_start: Instant, }, /// Stash held, `install_binary` in flight. + /// `for_update` is `true` when reinstalling over an existing install + /// (auto-update path) and `false` for a fresh install. AwaitingInstall { session_id: SessionId, session_info: SessionInfo, transport: SshTransport, setup_start: Instant, + #[allow(dead_code)] + for_update: bool, }, /// Stash held, `connect_session` in flight. Bootstrap is flushed only /// once `SessionConnected` arrives (or on connection failure). @@ -102,9 +106,10 @@ impl RemoteServerController { session_id, result, remote_platform, + has_old_binary, } => { me.remote_platform = remote_platform.clone(); - me.on_binary_check_complete(*session_id, result.clone(), ctx); + me.on_binary_check_complete(*session_id, result.clone(), *has_old_binary, ctx); } RemoteServerManagerEvent::BinaryInstallComplete { session_id, result } => { me.on_binary_install_complete(*session_id, result.clone(), ctx); @@ -198,6 +203,7 @@ impl RemoteServerController { &mut self, session_id: SessionId, result: Result, + has_old_binary: bool, ctx: &mut ModelContext, ) { let SshInitState::AwaitingCheck { @@ -229,6 +235,21 @@ impl RemoteServerController { }; self.connect_session_for_current_identity(session_id, socket_path, ctx); } + Ok(false) if has_old_binary => { + // Auto-update: a prior install exists, so skip the modal + // and reinstall. + self.did_install = true; + self.state = SshInitState::AwaitingInstall { + session_id, + session_info, + transport: transport.clone(), + setup_start, + for_update: true, + }; + RemoteServerManager::handle(ctx).update(ctx, |mgr, ctx| { + mgr.install_binary(session_id, transport, true, ctx); + }); + } Ok(false) => { let install_mode = *WarpifySettings::as_ref(ctx) .ssh_extension_install_mode @@ -251,9 +272,10 @@ impl RemoteServerController { session_info, transport: transport.clone(), setup_start, + for_update: false, }; RemoteServerManager::handle(ctx).update(ctx, |mgr, ctx| { - mgr.install_binary(session_id, transport, ctx); + mgr.install_binary(session_id, transport, false, ctx); }); } SshExtensionInstallMode::NeverInstall => { @@ -287,15 +309,20 @@ impl RemoteServerController { unreachable!("just matched AwaitingUserChoice above"); }; + // Reaching this path implies the user explicitly confirmed a + // fresh install from the modal. Auto-update flows (with an old + // binary detected) skip the modal entirely and go through + // `on_binary_check_complete` with `is_update: true`. self.did_install = true; self.state = SshInitState::AwaitingInstall { session_id, session_info, transport: transport.clone(), setup_start, + for_update: false, }; RemoteServerManager::handle(ctx).update(ctx, |mgr, ctx| { - mgr.install_binary(session_id, transport, ctx); + mgr.install_binary(session_id, transport, false, ctx); }); } @@ -403,26 +430,24 @@ impl RemoteServerController { result: Result<(), String>, ctx: &mut ModelContext, ) { - let SshInitState::AwaitingInstall { - session_id: expected, - .. - } = &self.state - else { - return; + let expected = match &self.state { + SshInitState::AwaitingInstall { session_id, .. } => *session_id, + _ => return, }; - if *expected != session_id { + if expected != session_id { return; } - let SshInitState::AwaitingInstall { - session_info, - transport, - setup_start, - .. - } = std::mem::replace(&mut self.state, SshInitState::Idle) - else { - unreachable!("just matched AwaitingInstall above"); - }; + let (session_info, transport, setup_start) = + match std::mem::replace(&mut self.state, SshInitState::Idle) { + SshInitState::AwaitingInstall { + session_info, + transport, + setup_start, + .. + } => (session_info, transport, setup_start), + _ => unreachable!("just matched AwaitingInstall above"), + }; match result { Ok(()) => { let socket_path = transport.socket_path().clone(); diff --git a/crates/remote_server/src/install_remote_server.sh b/crates/remote_server/src/install_remote_server.sh index f4344056..2129471b 100644 --- a/crates/remote_server/src/install_remote_server.sh +++ b/crates/remote_server/src/install_remote_server.sh @@ -6,6 +6,8 @@ # {channel} — stable | preview | dev # {install_dir} — e.g. ~/.warp/remote-server # {binary_name} — e.g. oz | oz-dev | oz-preview +# {version_query} — e.g. &version=v0.2026... (empty when no release tag) +# {version_suffix} — e.g. -v0.2026... (empty when no release tag) set -e arch=$(uname -m) @@ -29,11 +31,11 @@ mkdir -p "$install_dir" tmpdir=$(mktemp -d "$install_dir/.install.XXXXXX") trap 'rm -rf "$tmpdir"' EXIT -curl -fSL "{download_base_url}?package=tar&os=$os_name&arch=$arch_name&channel={channel}" \ +curl -fSL "{download_base_url}?package=tar&os=$os_name&arch=$arch_name&channel={channel}{version_query}" \ -o "$tmpdir/oz.tar.gz" tar -xzf "$tmpdir/oz.tar.gz" -C "$tmpdir" bin=$(find "$tmpdir" -type f -name 'oz*' ! -name '*.tar.gz' | head -n1) if [ -z "$bin" ]; then echo "no binary found in tarball" >&2; exit 1; fi chmod +x "$bin" -mv "$bin" "$install_dir/{binary_name}" +mv "$bin" "$install_dir/{binary_name}{version_suffix}" diff --git a/crates/remote_server/src/manager.rs b/crates/remote_server/src/manager.rs index 190e8187..495e9c4e 100644 --- a/crates/remote_server/src/manager.rs +++ b/crates/remote_server/src/manager.rs @@ -17,7 +17,11 @@ use crate::transport::RemoteTransport; use crate::HostId; use repo_metadata::RepoMetadataUpdate; use serde::Serialize; +#[cfg(not(target_family = "wasm"))] +use warp_core::channel::ChannelState; use warp_core::SessionId; +#[cfg(not(target_family = "wasm"))] +use warpui::r#async::FutureExt as _; use warpui::{Entity, ModelContext, ModelSpawner, SingletonEntity}; /// Maximum number of reconnection attempts after a spontaneous disconnect. @@ -116,6 +120,27 @@ impl RemoteServerErrorKind { } } +/// Returns `true` if the client and server are on compatible versions for +/// the initialize handshake. +/// +/// Semantics: +/// - Both sides carry a non-empty release tag (`Some(_)` client, non-empty +/// `server` string): the tags must match exactly. Mismatched releases +/// cause the manager to tear the session down and delete the stale +/// binary so the next reconnect reinstalls. +/// - Both sides are unknown (client `None` and server reports an empty +/// string): treat as compatible. This preserves the `cargo run` + +/// `script/deploy_remote_server` dev loop, where neither side reports a +/// release tag. +#[cfg(not(target_family = "wasm"))] +fn version_is_compatible(client: Option<&str>, server: &str) -> bool { + match (client, server.is_empty()) { + (Some(c), false) => c == server, + (None, true) => true, + (Some(_), true) | (None, false) => false, + } +} + /// Per-session connection state. Encodes which data is available at each /// lifecycle stage so the compiler prevents invalid combinations. /// @@ -289,6 +314,14 @@ pub enum RemoteServerManagerEvent { /// The detected remote platform (OS + arch) from `uname -sm`. /// `None` if detection failed or was not attempted. remote_platform: Option, + /// `true` if the remote already has an existing install of the + /// remote-server binary, detected by probing whether the install + /// directory exists (see `RemoteTransport::check_has_old_binary`). + /// Combined with `result == Ok(false)`, this tells the controller + /// it should auto-install as an update instead of prompting the + /// user. `false` when no prior install was detected, or when the + /// detection itself failed. + has_old_binary: bool, }, /// Result of [`RemoteServerManager::install_binary`]. Returns a result where: /// - `Ok(())` means the install succeeded, and @@ -438,9 +471,17 @@ impl RemoteServerManager { let spawner = self.spawner.clone(); ctx.background_executor() .spawn(async move { - // Run platform detection and binary check concurrently. - let (platform_result, check_result) = - futures::join!(transport.detect_platform(), transport.check_binary(),); + // Run platform detection, binary check, and old-binary + // check concurrently. The old-binary check lets the + // controller distinguish fresh install (no prior + // versioned binary) from update (prior versioned + // binary present), so it can skip the install prompt + // in the update case. + let (platform_result, check_result, old_binary_result) = futures::join!( + transport.detect_platform(), + transport.check_binary(), + transport.check_has_old_binary(), + ); let platform = match platform_result { Ok(p) => Some(p), Err(e) => { @@ -448,6 +489,16 @@ impl RemoteServerManager { None } }; + let has_old_binary = match old_binary_result { + Ok(has) => has, + Err(e) => { + log::warn!( + "Old-binary detection failed for session {session_id:?}: {e}. \ + Treating as fresh install." + ); + false + } + }; let _ = spawner .spawn(move |me, ctx| { if let Some(p) = &platform { @@ -465,6 +516,7 @@ impl RemoteServerManager { session_id, result: check_result, remote_platform: platform, + has_old_binary, }); }) .await; @@ -483,6 +535,7 @@ impl RemoteServerManager { &mut self, session_id: SessionId, transport: T, + is_update: bool, ctx: &mut ModelContext, ) where T: RemoteTransport + 'static, @@ -494,11 +547,16 @@ impl RemoteServerManager { #[cfg(not(target_family = "wasm"))] { + let setup_state = if is_update { + RemoteServerSetupState::Updating + } else { + RemoteServerSetupState::Installing { + progress_percent: None, + } + }; ctx.emit(RemoteServerManagerEvent::SetupStateChanged { session_id, - state: RemoteServerSetupState::Installing { - progress_percent: None, - }, + state: setup_state, }); let spawner = self.spawner.clone(); ctx.background_executor() @@ -701,6 +759,35 @@ impl RemoteServerManager { .initialize(auth_token.as_deref()) .await .map_err(|e| ConnectAndHandshakeError::Initialize(anyhow::anyhow!("{e:#}")))?; + + // Version compatibility check. If the server reports a different release + // tag than the client expects, the binary on disk is stale. Remove it so + // the next reconnect (or explicit reconnect by the user) will reinstall. + let client_version = ChannelState::app_version(); + if !version_is_compatible(client_version, &resp.server_version) { + log::warn!( + "Remote server version mismatch for session {session_id:?}: \ + client={client_version:?}, server={:?}. Removing stale binary.", + resp.server_version + ); + + const REMOVAL_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); + + if let Err(e) = transport + .remove_remote_server_binary() + .with_timeout(REMOVAL_TIMEOUT) + .await + .unwrap_or_else(|_| Err(anyhow::anyhow!("timed out after {REMOVAL_TIMEOUT:?}"))) + { + log::warn!("Failed to remove stale remote binary for session {session_id:?}: {e}"); + } + return Err(ConnectAndHandshakeError::Initialize(anyhow::anyhow!( + "remote server version mismatch (client: {client_version:?}, \ + server: {:?}); reconnect to reinstall", + resp.server_version + ))); + } + Ok(HostId::new(resp.host_id)) } diff --git a/crates/remote_server/src/setup.rs b/crates/remote_server/src/setup.rs index ed3e62db..c5c8ed3b 100644 --- a/crates/remote_server/src/setup.rs +++ b/crates/remote_server/src/setup.rs @@ -8,8 +8,12 @@ use warp_core::channel::{Channel, ChannelState}; pub enum RemoteServerSetupState { /// Checking if the binary exists on remote. Checking, - /// Downloading and installing the binary. + /// Downloading and installing the binary for the first time on this host. Installing { progress_percent: Option }, + /// Replacing an existing install with a differently-versioned binary. + /// Rendered as "Updating..." in the UI so the user understands this + /// isn't a fresh install. + Updating, /// Binary is launched, waiting for InitializeResponse. Initializing, /// Handshake complete. Ready. @@ -34,7 +38,14 @@ impl RemoteServerSetupState { pub fn is_in_progress(&self) -> bool { matches!( self, - Self::Checking | Self::Installing { .. } | Self::Initializing + Self::Checking | Self::Installing { .. } | Self::Updating | Self::Initializing + ) + } + + pub fn is_connecting(&self) -> bool { + matches!( + self, + Self::Installing { .. } | Self::Updating | Self::Initializing ) } } @@ -172,16 +183,51 @@ pub fn binary_name() -> &'static str { ChannelState::channel().cli_command_name() } -/// Returns the full remote binary path. +/// Returns the full remote binary path for the current channel and client +/// version. +/// +/// The path-versioning rule is keyed strictly off [`Channel`]: +/// +/// - [`Channel::Local`] and [`Channel::Oss`] always use the bare +/// `{binary_name}` path. For `Local` this is the slot +/// `script/deploy_remote_server` writes to; `Oss` is treated the +/// same way because it has no release-pinned CDN artifact and is +/// expected to be deployed/managed locally. +/// - Every other channel always uses `{binary_name}-{version}`, where +/// `version` is the baked-in `GIT_RELEASE_TAG` when present and falls +/// back to `CARGO_PKG_VERSION` otherwise. The fallback keeps the path +/// deterministic for misconfigured `cargo run --bin {dev,preview,...}` +/// builds; the resulting `&version=...` query is expected to 404 against +/// `/download/cli` and surface a clean `SetupFailed` rather than silently +/// writing to a path that doesn't follow the rule. pub fn remote_server_binary() -> String { - format!("{}/{}", remote_server_dir(), binary_name()) + let dir = remote_server_dir(); + let name = binary_name(); + match ChannelState::channel() { + Channel::Local | Channel::Oss => format!("{dir}/{name}"), + Channel::Stable | Channel::Preview | Channel::Dev | Channel::Integration => { + format!("{dir}/{name}-{}", pinned_version()) + } + } } /// Returns the shell command to check if the remote server binary exists and /// is executable. pub fn binary_check_command() -> String { - let bin = remote_server_binary(); - format!("test -x {bin}") + format!("test -x {}", remote_server_binary()) +} + +/// Returns the version string used to pin remote-server installs on +/// channels that take the versioned path (i.e. everything except +/// [`Channel::Local`] and [`Channel::Oss`]). Prefers the baked-in +/// `GIT_RELEASE_TAG` from [`ChannelState::app_version`]; falls back to +/// `CARGO_PKG_VERSION` so the path / install URL is deterministic even on +/// dev `cargo run` builds without a release tag. The `CARGO_PKG_VERSION` +/// fallback is not expected to map to a real `/download/cli` artifact — +/// it exists to produce a clean install-time failure rather than silently +/// fall through to the unversioned (Local/Oss-only) path. +fn pinned_version() -> &'static str { + ChannelState::app_version().unwrap_or(env!("CARGO_PKG_VERSION")) } /// The install script template, loaded from a standalone `.sh` file for @@ -189,20 +235,33 @@ pub fn binary_check_command() -> String { /// [`install_script`]. const INSTALL_SCRIPT_TEMPLATE: &str = include_str!("install_remote_server.sh"); -/// Returns the install script that downloads and installs the CLI binary. +/// Returns the install script that downloads and installs the CLI binary +/// at the current client version. /// -/// The script detects the remote architecture via `uname -m`, downloads the -/// correct Oz CLI tarball from the download URL (with os, arch, package, and -/// channel query params), and extracts it to the install directory. -/// -/// All parameters (URL, channel, directory, binary name) are derived -/// internally from the current channel configuration. +/// The script detects the remote architecture via `uname -m`, downloads +/// the correct Oz CLI tarball from the download URL, and installs it at +/// the path returned by [`remote_server_binary`] so repeat invocations +/// are idempotent. The `version_query` / `version_suffix` substitutions +/// follow the same rule as [`remote_server_binary`]: empty on +/// [`Channel::Local`] and [`Channel::Oss`] (so the install lands at +/// the unversioned path used by `script/deploy_remote_server`); pinned to +/// `&version={v}` / `-{v}` on every other channel, where `v` falls back +/// to `CARGO_PKG_VERSION` when no release tag is baked in. pub fn install_script() -> String { + let (version_query, version_suffix) = match ChannelState::channel() { + Channel::Local | Channel::Oss => (String::new(), String::new()), + Channel::Stable | Channel::Preview | Channel::Dev | Channel::Integration => { + let v = pinned_version(); + (format!("&version={v}"), format!("-{v}")) + } + }; INSTALL_SCRIPT_TEMPLATE .replace("{download_base_url}", &download_url()) .replace("{channel}", download_channel()) .replace("{install_dir}", &remote_server_dir()) .replace("{binary_name}", binary_name()) + .replace("{version_query}", &version_query) + .replace("{version_suffix}", &version_suffix) } /// Construct the download URL from the server root URL. diff --git a/crates/remote_server/src/setup_tests.rs b/crates/remote_server/src/setup_tests.rs index ab082fc6..9e60f11d 100644 --- a/crates/remote_server/src/setup_tests.rs +++ b/crates/remote_server/src/setup_tests.rs @@ -101,8 +101,9 @@ fn state_is_terminal() { .is_terminal()); assert!(!RemoteServerSetupState::Checking.is_terminal()); assert!(!RemoteServerSetupState::Installing { - progress_percent: None + progress_percent: None, } .is_terminal()); + assert!(!RemoteServerSetupState::Updating.is_terminal()); assert!(!RemoteServerSetupState::Initializing.is_terminal()); } diff --git a/crates/remote_server/src/transport.rs b/crates/remote_server/src/transport.rs index 60e534d2..55fd9430 100644 --- a/crates/remote_server/src/transport.rs +++ b/crates/remote_server/src/transport.rs @@ -82,6 +82,19 @@ pub trait RemoteTransport: Send + Sync + std::fmt::Debug { &self, ) -> Pin> + Send>>; + /// Checks whether the remote host already has an existing install + /// of the remote server binary. + /// + /// Used by the manager to distinguish a fresh install (no prior + /// install on disk, user should be prompted) from an update (prior + /// install present, install should happen automatically). + /// + /// Returns `Ok(true)` if a prior install was detected, `Ok(false)` + /// if not, and `Err(_)` on SSH failure. + fn check_has_old_binary( + &self, + ) -> Pin> + Send>>; + /// Installs the remote server binary on the remote host. /// /// Pure I/O — does not emit any events. The caller @@ -108,4 +121,18 @@ pub trait RemoteTransport: Send + Sync + std::fmt::Debug { &self, executor: std::sync::Arc, ) -> Pin> + Send>>; + + /// Remove the remote server binary, forcing a reinstall on the next + /// [`install_binary`] call. + /// + /// Called by the manager after the initialize handshake reports a + /// version that disagrees with the client's: the file at the expected + /// path is stale/wrong, so we remove it so the next setup sees a miss + /// and reinstalls from the CDN instead of looping on the same bad + /// binary. + /// + /// [`install_binary`]: RemoteTransport::install_binary + fn remove_remote_server_binary( + &self, + ) -> Pin> + Send>>; } diff --git a/specs/APP-3805/TECH.md b/specs/APP-3805/TECH.md new file mode 100644 index 00000000..505e8f7f --- /dev/null +++ b/specs/APP-3805/TECH.md @@ -0,0 +1,155 @@ +# TECH.md — Client/Server Version Skew for Remote Server + +Linear: [APP-3805](https://linear.app/warpdotdev/issue/APP-3805/client-server-version-skew) + +## 1. Problem + +The Warp remote server binary is installed at a single, unversioned path per channel (e.g. `~/.warp/remote-server/oz`). The existence check is `test -x {bin}` and we never inspect the binary's version before talking to it. When the client auto-updates to a new version, it happily reuses the old remote server binary, which can drift arbitrarily far from the protocol/behaviour the client expects. The `InitializeResponse` already carries `server_version`, but the client ignores it. + +We need a version-gated install flow: connecting from a client at version *V* always ends up talking to a server binary also at *V*. Any local `cargo run` workflow (where the client has no `GIT_RELEASE_TAG`) keeps working with `script/deploy_remote_server`: a deployed binary at the unversioned path always wins, and when one is missing the client falls back to installing latest-for-channel at the same unversioned path so the dev loop self-heals. + +## 2. Requirements + +R1. **Exact version match.** A connected client at version *V* must only communicate with a remote daemon spawned from a binary at the same version *V*. No silent skew. + +R2. **Automatic reinstall on drift.** When the installed binary is the wrong version (or missing), the client reinstalls the correct version as part of the connect flow. No manual `rm -rf` step. + +R3. **Cheap happy path.** When the correct binary is already installed, connecting should require no extra downloads and no extra SSH round-trips beyond today's single `test -x` check. + +R4. **Local dev workflow preserved.** `Channel::Local` clients (the default `cargo run`) must keep working with `script/deploy_remote_server`. When a deployed binary exists at the unversioned path, the client uses it without re-downloading from the CDN. When no binary exists, the client auto-installs latest-for-channel at the same unversioned path so a stale install directory self-heals. The unversioned filename is reserved for `Channel::Local` and `Channel::Oss` (which has no release-pinned CDN artifact and is treated identically); every other channel always uses a versioned filename, so versioned-channel builds can never silently overwrite a `script/deploy_remote_server`-owned slot. + +R5. **Clear failure surface.** If install or version validation fails, the user sees an actionable setup-failed state rather than a confusing protocol error. + +R6. **Defense-in-depth.** A hand-placed or half-written binary at the expected path shouldn't be trusted blindly — the version reported at the handshake must also be verified. + +R7. **No regression in storage behaviour.** We don't introduce unbounded disk growth, and we don't break existing installs at the current unversioned path during rollout. + +## 3. Current connect flow + +The install + handshake path this spec mutates spans four files. The sequence below shows today's behaviour; §4 slots version-aware steps into the marked decision points. + +```mermaid +sequenceDiagram + participant UI as Client UI + participant Mgr as RemoteServerManager + participant Tx as SshTransport + participant Remote as Remote host + participant Daemon as oz daemon + + UI->>Mgr: connect_session + Mgr->>Tx: setup(session_id) + Note over Tx: today: path = {dir}/{binary_name}
(no version in filename) + Tx->>Remote: ssh: test -x {path} + alt binary missing + Tx->>Remote: ssh: install_remote_server.sh
(curl → tar → mv → {path}) + end + Tx-->>Mgr: SetupReady + Mgr->>Tx: connect() + Tx->>Remote: ssh: `{path} remote-server-proxy` + Remote->>Daemon: spawn (if not already running) + Mgr->>Daemon: Initialize + Daemon-->>Mgr: InitializeResponse { server_version, host_id } + Note over Mgr: today: server_version is ignored + Mgr->>Mgr: mark_session_connected +``` + +Key pieces of today's behaviour the diagram elides: + +- `setup::remote_server_binary()` resolves the path purely from the channel; there is no version in the filename. +- `install_remote_server.sh` pulls from `{server_root_url}/download/cli?package=tar&os=...&arch=...&channel={channel}` with no version pin. The `warp-server` `/download/cli` endpoint already accepts a `version=` query parameter that pins the artifact to an exact release (and falls back to latest-for-channel when omitted), so this spec only needs client-side changes. +- `ChannelState::app_version()` returns `option_env!("GIT_RELEASE_TAG")` — `Some("v0.…")` on release builds, `None` on `cargo run`. This is the signal we'll thread through in §4. +- `script/deploy_remote_server` is the developer escape hatch: it `rsync`s a locally-built binary into `~/.warp-local/remote-server/oz-local` and assumes the client won't try to download on top of it. + +## 4. Proposed solution + +Encode the client's expected version into the installed binary's filename, so version drift turns into a missing-file miss that naturally re-triggers the existing install flow. Layer a handshake-level version check on top as a safety net, and reserve the unversioned filename for `Channel::Local` and `Channel::Oss`. +### 4.1 Channel-keyed binary paths +In `crates/remote_server/src/setup.rs`, the path resolution is keyed strictly off [`Channel`]: +- `remote_server_binary()`: + - When `ChannelState::channel()` is `Channel::Local` or `Channel::Oss` → `{dir}/{binary_name}` (the unversioned `deploy_remote_server` slot; `Oss` has no release-pinned CDN artifact and follows the same convention). + - For every other channel (`Stable`, `Preview`, `Dev`, `Integration`) → `{dir}/{binary_name}-{v}`, where `v = ChannelState::app_version().unwrap_or(env!("CARGO_PKG_VERSION"))`. +- `binary_check_command()` keeps the single `test -x {remote_server_binary()}` contract. Because the version is part of the filename on every versioned channel, any drift resolves to a miss on the existing code path and re-runs `install_script`. +The `CARGO_PKG_VERSION` fallback is intentionally not expected to point at a real release artifact; see §4.4 for the failure shape on versioned-channel + no-tag builds. +### 4.2 Install script pins the exact version (or latest, for Local/Oss) +`warp-server`'s `/download/cli` already honours a `version=` query parameter (pins the redirect to the exact versioned artifact when present; falls back to latest-for-channel when absent), so no server-side change is needed. +In `install_remote_server.sh`, add `{version_query}` and `{version_suffix}` placeholders used in two places: +- Download URL query string: `...&channel={channel}{version_query}` (e.g. `&version=v0.…`, or empty). +- Final install path: `mv "$bin" "$install_dir/{binary_name}{version_suffix}"` (e.g. `-v0.…`, or empty). +In `setup.rs::install_script()`, substitute based on `ChannelState::channel()`: +- `Channel::Local` and `Channel::Oss` → both substitutions are empty strings. These channels download latest-for-channel and install at the unversioned `{dir}/{binary_name}` path that `script/deploy_remote_server` also uses. +- Every other channel → `version_query = "&version={v}"`, `version_suffix = "-{v}"`, where `v` resolves via `app_version().unwrap_or(CARGO_PKG_VERSION)`. Release-tagged clients pin the exact published version; versioned-channel builds without a release tag pin `CARGO_PKG_VERSION`, which deliberately doesn't map to a real `/download/cli` artifact. +Unknown versions (rolled-back releases, or the `CARGO_PKG_VERSION` versioned-channel fallback) surface as a GCS 404 on the redirected URL, which `curl -fSL` turns into a non-zero exit and `SetupFailed` for the user. + +### 4.3 Handshake version validation + +In `crates/remote_server/src/manager.rs::connect_session`, immediately after `client.initialize().await` returns `Ok(resp)`: + +- Let `client_v = ChannelState::app_version()` and `server_v = resp.server_version`. +- If both are `Some` (or server returned a non-empty string) and they differ → log the skew, **delete the versioned binary on the remote host** (see below), then emit `RemoteServerManagerEvent::SessionDisconnected` and call `mark_session_disconnected`. +- If both sides are unknown (client `None` and server reports an empty string) → accept and log a warning. This is the `cargo run` (Local/Oss) + `script/deploy_remote_server` dev loop where neither side carries a release tag. +- Any other shape (one side has a version, the other doesn't) → treated as incompatible. This is defense-in-depth: in normal operation the path-based check (§4.1) would already have triggered a reinstall to bring both sides into agreement, so a mixed shape at the handshake means something unusual (hand-placed binary, partial download) and we prefer to tear the session down and reinstall. + +**Why delete the binary on mismatch?** The handshake check only fires when filename-based detection (§4.1) said we had the right version but the running daemon disagrees. That means the file at the versioned path is wrong — partial download, hand-placed, or corrupted. If we only disconnect, the next reconnect will re-run `ensure_binary_installed`, see `test -x {path}` succeed (the file still exists), skip install, respawn the daemon, and hit the same mismatch — a reconnect loop. Deleting the binary forces the next `ensure_binary_installed` to miss and reinstall, breaking the loop in a single extra SSH command (`ssh rm -f {path}`). + +Factor the comparison into a pure helper (`fn version_is_compatible(client: Option<&str>, server: &str) -> bool`) so it's unit-testable without wiring up a client. + +### 4.4 Unversioned channels (Local / Oss) and versioned-channel-without-tag +`Channel::Local` (the default `cargo run`) and `Channel::Oss` take the unversioned branch of `remote_server_binary()` / `install_script()` (see §4.1, §4.2). Concretely: +- If `script/deploy_remote_server` has put a binary at the unversioned path, `binary_check_command` succeeds and we connect without touching the CDN. +- If the unversioned binary is missing but the install directory exists (e.g. a previous install ran here), the controller's auto-update branch fires and `install_script` runs with empty `version_query`/`version_suffix` — i.e. it pulls latest-for-channel and installs at the same unversioned path. Future `deploy_remote_server` runs simply overwrite this in place. +- If the install directory is also missing (truly fresh host), we fall through to the normal install-mode path (`AlwaysAsk` → user modal, etc.). +`script/deploy_remote_server` is not touched; its target path (`~/.warp-local/remote-server/oz-local` on the local channel) is exactly what the `Channel::Local` branch of `remote_server_binary()` returns. +Versioned-channel builds without a release tag (e.g. `cargo run --bin dev`, `--bin preview`) are deliberately unsupported for SSH remote-server installs. The `CARGO_PKG_VERSION` fallback (§4.1) keeps the path deterministic but the resulting `&version=CARGO_PKG_VERSION` query 404s against `/download/cli`, surfacing a clean `BinaryInstallComplete::Err(_)` and the existing failed-banner path. Developers who need to test against a real channel should either build with `GIT_RELEASE_TAG` set or use `cargo run` (Local) + `script/deploy_remote_server`. + +### 4.5 Rollout / backwards compatibility + +The first release with this change will, for every user, look for a versioned path that does not yet exist on their remote host. That's fine — it falls through to the normal install path and overwrites into the new versioned filename. The legacy unversioned binary (`{dir}/{binary_name}`) is simply orphaned on disk. We accept that (see §4.6) and can sweep it later. + +### 4.6 Deliberate non-goals + +- **Cleanup of old versioned binaries.** Accept accumulation for v1; easy to add later as a post-install step that keeps the current + previous version. +- **Upload-over-SSH fallback** when the remote can't reach the CDN. Reasonable future work, not required here. +- **Protocol change to send client version in `Initialize`.** We only read `server_version` off the response; the reverse direction can be added if/when the server wants to reject proactively. + +## 5. How the solution maps to the requirements + +- **R1 (exact match).** Versioned filename (§4.1) + handshake check (§4.3) give us two independent enforcements of exact match. +- **R2 (automatic reinstall).** A new version turns into a path miss (§4.1), which re-enters the existing `install_script` path (§4.2). No manual step. +- **R3 (cheap happy path).** Still a single `test -x` SSH command on the hot path; handshake check is a string compare on a response we already receive. +- **R4 (local dev workflow).** `Channel::Local` and `Channel::Oss` short-circuit to the unversioned path for both the install location and the existence check (§4.1). A deployed binary always wins via `test -x`; if missing but the install directory exists, the install script runs with empty `version_query`/`version_suffix` and pulls latest-for-channel to the same unversioned path (§4.2, §4.4). `deploy_remote_server` is unchanged and overwrites in place. Versioned-channel builds without a release tag fail cleanly via the `CARGO_PKG_VERSION` 404 path so they can never silently overwrite a Local/Oss-owned slot. +- **R5 (clear failure).** Install errors (CDN 404, network failure, etc.) propagate via `BinaryInstallComplete { result: Err(_) }` and the controller surfaces them through the existing `show_ssh_remote_server_failed_banner` path. Handshake version mismatches reach the same banner via `SessionConnectionFailed { phase: Initialize }` (§4.3). +- **R6 (defense-in-depth).** Handshake check catches stale or hand-placed binaries even when the filename is right (§4.3). +- **R7 (no regression).** Existing unversioned install is simply orphaned after the first upgrade; no code assumes its absence or presence (§4.5). + +## 6. Testing and validation + +Covers each requirement with a concrete check: + +- **Unit tests in `crates/remote_server/src/setup_tests.rs`** (uses the existing `test-util` `ChannelState::set_app_version` hook): + - `remote_server_binary()` returns a `{name}-{version}` path on every versioned channel (`Stable`, `Preview`, `Dev`, `Integration`), whether `app_version()` is `Some` or falls back to `CARGO_PKG_VERSION`. *(R1, R2)* + - `remote_server_binary()` returns the bare `{name}` path on `Channel::Local` and `Channel::Oss`, regardless of `app_version()`. *(R4)* + - `install_script()` substitutes `{version_query}` / `{version_suffix}` into both the URL and the install path on every versioned channel. *(R1, R2)* + - `install_script()` substitutes empty strings for both placeholders on `Channel::Local` and `Channel::Oss`, so the script downloads latest-for-channel and installs at the unversioned path. *(R4)* +- **Unit tests for `version_is_compatible`** covering: + - Matching `Some("v…")` on both sides → compatible. *(R1)* + - Differing `Some`/`Some` → incompatible. *(R1, R6)* + - Both unknown (client `None`, server `""`) → compatible with warning. *(R4)* + - Mixed shape (`Some`/empty, `None`/non-empty) → incompatible. *(R6)* +- **Manual validation** (captured in the PR description): + - Connect once at version *V*; confirm the binary lands at `{dir}/{name}-{V}` and the session comes up. *(R1, R2, R3)* + - Bump the client to version *V+1* (or stub the version in `ChannelState::set_app_version`), reconnect; confirm a new install runs and the connection succeeds. *(R1, R2)* + - Corrupt the versioned binary (e.g. replace its contents with a stub) so the handshake reports a different version; confirm the session is torn down with `SessionDisconnected` and a user-visible error. *(R6)* + - `cargo run` (Local) against a host where the install directory exists but the unversioned binary is missing; confirm the controller auto-installs latest-for-channel at the unversioned path and the session comes up. *(R4)* + - After running `script/deploy_remote_server`, reconnect with `cargo run` (Local) and confirm `test -x` succeeds and we connect without re-downloading. *(R4)* +- **Existing remote-server manager tests** continue to pass unchanged (no behavioural change for the `None`/`""` path). + +## 7. Risks and mitigations + +- **Churn on every release.** Each stable/preview/dev release will pull a fresh binary on every host the user connects to the first time. Expected; bandwidth impact is minor. +- **Orphaned binaries accumulate.** See §4.6; acceptable for v1, cleanup is a straightforward follow-up. +- **Repeat handshake mismatches.** Addressed by deleting the offending binary on mismatch (§4.3) so the next reconnect reinstalls rather than looping. Worth explicit test coverage (§6). + +## 8. Open questions + +- Should `Initialize` (client → server) also carry the client's expected version so the server can reject proactively, or is client-side enforcement enough? Leaning "enough" for now. +- Do we want to surface the "version-skew detected, retrying" state as a distinct UI affordance, or is the standard reconnect flow sufficient? Current plan: reconnect-only, revisit if it surfaces as a usability issue.