From 18edab0663b33665e2bc7ff2a4b7ccc48d420870 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 20:38:30 +0100 Subject: [PATCH 1/2] feat: one reusable OpenSSH connection per workspace `dl -- ` at a terminal has gone into the container over OpenSSH since M3. It opened a fresh connection every time, and connection setup is nearly the whole cost of that trip: 2590ms to 3140ms fresh against 16ms to 28ms reused, on a rig at load average 21 where only the ratio travels (#390). Three `-o` options on an argv `clients/ssh.rs` already built are the whole mechanism. `ControlMaster=auto` means no pre-warm, so the spawn counts do not move; the argv does, and its pins say so. The socket is derived rather than configured, and its digest is the load-bearing part. A master filters `SendEnv` against its own permit list, in silence, at exit 0 (#389: `GOT=[]`, rc=0), so a master opened by a run with no token hands the next run an empty `GH_TOKEN` and an unauthenticated `gh` with nothing to say so. The digest covers the host alias, the permit list and `$SSH_AUTH_SOCK`, which makes that state unrepresentable instead of documented: a client whose list differs from the master's cannot find that master. Fields go in length-prefixed, so no two inputs encode alike. `Reuse::Multiplexed | Reuse::Direct` is a sum and not an `Option`, because `Direct` has real causes: a path too long for `sun_path`, or a directory dl cannot make. Both arms run the same command and differ in latency only. Everything that can go wrong ends at `Direct`, so a session that cannot be multiplexed is one that runs unmultiplexed, never one that fails. Closes #422. --- CHANGELOG.md | 32 ++ docs/performance.md | 38 ++ rust/devlaunch-core/src/clients/ssh.rs | 548 +++++++++++++++++++++++- rust/devlaunch-core/src/flows/launch.rs | 113 +++++ 4 files changed, 721 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 783efb13..8df9a662 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **A workspace gets one reusable OpenSSH connection instead of a new one per + run.** `dl -- ` typed at a terminal has gone into the container over + OpenSSH since M3, through the host alias `devpod up` publishes. It just opened a + fresh connection every time, and connection setup is nearly the whole cost of + that trip. It now carries `ControlMaster=auto`, a derived `ControlPath` and + `ControlPersist=60`: the first trip into a live workspace opens a master, every + trip for the next minute joins it, and nothing pre-warms anything, so `dl` starts + no process it did not start before. + + Two savings, and one caveat that belongs on both numbers. Measured on a loaded + host (load average 21 throughout, so the absolute seconds sit two to three times + above a quiet machine and only the ratio travels): a fresh `ssh -t` cost 2590ms + to 3140ms against a reused 16ms to 28ms, about 100x. That is most of two seconds + off every repeat command into a workspace that is already up, and nothing at all + off a first launch into a cold one. The second saving is the one that is not a + stopwatch reading: trips that are not multiplexed serialize on a per workspace + lock, so eight commands fired at one workspace at once used to finish over a 9.9s + to 23.9s staircase, where eight over one master all finished at 8.02s. A fleet of + agents attaching to one workspace is this repository's own daily shape. + + The socket path is derived, and its digest covers the host alias, the `SendEnv` + permit list and `$SSH_AUTH_SOCK`. That is load bearing rather than tidy: a master + filters `SendEnv` against **its own** permit list, in silence, at exit 0, so + without it a master opened by a run with no GitHub token would hand the next run + an empty `GH_TOKEN` and an unauthenticated `gh` with nothing anywhere to say so. + A run whose permit list differs from the master's cannot find that master, so the + mismatch has nowhere to happen. The sockets live under `ssh-control` in `dl`'s + own cache directory, in a directory this user alone can read, and `dl --purge` + takes them with everything else. A socket path too long for a unix socket, or a + directory `dl` cannot create, means the session runs unmultiplexed rather than + failing. [docs/performance.md](docs/performance.md) has the rest. + - **`dl --purge` names where each surviving workspace came from.** The list of workspaces a purge is leaving standing printed ids and nothing else, and an id is the one thing you cannot decide on: `pythontemplate` reads exactly the same diff --git a/docs/performance.md b/docs/performance.md index 6dacd342..7731369a 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -20,6 +20,44 @@ naming it and then the tools probe, rides a single setup pass. So an interactive `dl ` and a one-shot `dl -- ` cost the same trips. +## One connection per workspace + +The trip that carries `dl -- ` at a terminal is OpenSSH, over the host +alias `devpod up` publishes, and that connection is now reused. Three options go +on an argv `dl` already built: `ControlMaster=auto`, a derived `ControlPath`, and +`ControlPersist=60`. The first trip into a live workspace opens a master, every +trip for the next minute joins it, and nothing pre-warms anything, so `dl` starts +no process it did not start before. + +What it saves, measured on one loaded host (load average 21 for the whole run, so +the absolute seconds sit two to three times above a quiet machine and only the +ratio travels): a fresh `ssh -t` cost 2590ms to 3140ms and a reused one 16ms to +28ms. About 100x, and it is worth being plain about where it lands. A first launch +into a cold workspace moves by nothing. A repeat command into a workspace that is +already up saves most of two seconds. And the case it helps most is not a stopwatch +reading at all: trips that are not multiplexed serialize on a per workspace lock, +so eight commands fired at one workspace at once used to finish over a 9.9s to +23.9s staircase, where eight over one master all finished at 8.02s. A fleet of +agents attaching to one workspace is this repository's own daily shape. + +The socket path is derived rather than configured, and what goes into the digest +is the point of the whole thing. A master filters `SendEnv` against **its own** +permit list, in silence, at exit 0, so a master opened by a run with no GitHub +token would hand the next run an empty `GH_TOKEN` and an unauthenticated `gh` with +nothing anywhere to say so. The digest therefore covers the host alias, the +`SendEnv` permit list and `$SSH_AUTH_SOCK`: a run whose permit list differs from +the master's cannot find that master, so the mismatch has nowhere to happen. The +sockets live under `ssh-control` in `dl`'s own cache directory, in a directory +this user alone can read, and `dl --purge` takes them with everything else. Losing +one costs the next trip its couple of seconds and nothing more. + +It fails closed in both directions. A socket path too long for a unix socket, or a +directory `dl` cannot create, means the session runs unmultiplexed rather than +failing. A master that has gone away leaves a socket the next client unlinks, and +`ControlPersist` is a minute rather than an hour because a live master holds a +resident `devpod ssh --stdio` process and a `docker exec` per key, and `dl` must +not be the reason a container never goes idle. + ## Measuring launch time Set `DEVLAUNCH_TIMING=1` and a `dl` command ends with one summary on stderr, diff --git a/rust/devlaunch-core/src/clients/ssh.rs b/rust/devlaunch-core/src/clients/ssh.rs index 9b4ef14f..6c4ece78 100644 --- a/rust/devlaunch-core/src/clients/ssh.rs +++ b/rust/devlaunch-core/src/clients/ssh.rs @@ -101,6 +101,205 @@ pub(crate) fn host_alias(workspace_id: &str) -> String { format!("{workspace_id}{HOST_SUFFIX}") } +// =========================================================================== +// one connection per workspace +// =========================================================================== + +/// How long OpenSSH keeps a master alive with nothing running on it, in seconds. +/// +/// A constant and not a knob, on the `DOTFILES_ATTACH_TIMEOUT` grounds: getting it +/// wrong costs latency, never correctness — a master that has gone away is an +/// ordinary 2s trip, not a failure. 60s and not 600s because a live master holds a +/// resident `devpod ssh --stdio` and a `docker exec` per key, and devpod's docker +/// provider ships an `INACTIVITY_TIMEOUT` option whose own example is `10m`: dl +/// must not be the reason a user's container never goes idle. Measured at this +/// value in devlaunch#390 — reuse after 40s idle is 22ms, and past the window +/// OpenSSH has already unlinked the socket and the next trip is an ordinary +/// 1972ms. +pub(crate) const CONTROL_PERSIST: u32 = 60; + +/// The leaf under devlaunch's cache directory that the control sockets live in. +/// +/// Its own directory rather than a corner of the repo cache, for +/// `LAUNCH_LOCK_DIR`'s reasons exactly: it is keyed by workspace, it is wanted for +/// workspaces that have no clone under the cache at all, and it must not look like +/// a repo to the cache's walkers. Under the cache dir rather than +/// `$XDG_RUNTIME_DIR`, which this project's own containers do not have, so it +/// follows `XDG_CACHE_HOME` and a scratch run gets scratch sockets. +pub(crate) const CONTROL_DIR: &str = "ssh-control"; + +/// The `sockaddr_un::sun_path` a bound socket has to fit in, NUL included. +/// +/// 108 bytes on Linux and 104 on macOS; the smaller of the two, because the cost +/// of being wrong is not a warning. This is why [`Reuse`] has a second arm. +const SUN_PATH: usize = 104; + +/// What OpenSSH appends to `ControlPath` before it binds anything. +/// +/// **`muxserver_listen` does not bind the path it was given.** It binds +/// `.<16 random characters>` and `rename(2)`s that into place once +/// the socket is listening, so the path that has to fit is 17 bytes longer than +/// the one dl composes, and a socket directory that leaves under 17 bytes of head +/// room fails at `unix_listener: path ... too long for Unix domain socket` — +/// **exit 255, the session gone**, not a warning and not a fallback. +/// +/// Measured rather than reasoned: this ticket's first CI run took the e2e suite +/// down with it. `pytest`'s own scratch directory made a 96-byte path, which fits +/// in 104 and does not fit in 104 once OpenSSH has added `.N3IKqcZJ1KbkenKb` to +/// it, and thirteen tests failed on a length check that was 17 bytes too +/// generous. +const LISTEN_SUFFIX: usize = 17; + +/// How long a `ControlPath` dl composes may be. +/// +/// The buffer, less its NUL, less the room OpenSSH takes for itself. +const CONTROL_PATH_LIMIT: usize = SUN_PATH - 1 - LISTEN_SUFFIX; + +/// Whether this invocation may share a connection, and over which socket. +/// +/// A two-arm sum and not `Option`, because [`Reuse::Direct`] is a +/// real answer with a real cause rather than an absence: a socket path that will +/// not fit in `sun_path`, or a directory dl cannot make one in. Both arms produce +/// a valid argv and the same answer from the same session; they differ in latency +/// only, so no consumer needs to know which it got. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum Reuse { + /// Share a master over this socket, opening one if there is none. + Multiplexed(ControlSocket), + /// Open a connection of this session's own, as dl always did. + Direct, +} + +/// The path a master is keyed by, derived rather than configured. +/// +/// Derived is what makes there be no registry, no liveness bookkeeping and no +/// cleanup code: OpenSSH unlinks the socket when the master exits, the master +/// exits when the container goes away (devlaunch#389 measured four ways), and a +/// socket left behind by a killed master is unlinked by the next client. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ControlSocket(PathBuf); + +impl ControlSocket { + /// The path, as OpenSSH is told it. + pub(crate) fn as_path(&self) -> &Path { + &self.0 + } +} + +impl Reuse { + /// Derive the socket this session may share, or answer [`Reuse::Direct`]. + /// + /// `send_env` is the `SendEnv` permit list this invocation would carry and + /// `agent` is `$SSH_AUTH_SOCK`. Both are in the socket's identity, and that is + /// the load-bearing part of the whole mechanism. A master **filters `SendEnv` + /// against its own permit list, silently, at exit 0** (devlaunch#389 + /// reproduced it: `GOT=[]`, rc=0), so a master opened by a run with no token + /// hands the next run an *empty* `GH_TOKEN` and an unauthenticated `gh`, with + /// nothing anywhere in the output to say so. Keying the socket on the permit + /// list makes that state unrepresentable rather than documented: a client + /// whose list differs from the master's cannot find that master. `agent` is + /// the same move for #389's other finding, that a reused master pins agent + /// forwarding to whoever opened it. + /// + /// The alternative — declaring `SendEnv=GH_TOKEN` unconditionally so that the + /// list is a constant — is refused: it would forward a token that + /// `DEVLAUNCH_NO_GH_TOKEN` exists to withhold. + /// + /// The config file is deliberately *not* in the digest. The alias carries the + /// workspace id, and a workspace id is itself a digest of the repo, ref and + /// worktree, so two configs that both publish one alias are publishing one + /// workspace. + /// + /// Every way this can go wrong ends at [`Reuse::Direct`], which is the + /// fail-closed requirement: a session that cannot be multiplexed is a session + /// that runs unmultiplexed, never one that fails. + pub(crate) fn derive( + dir: &Path, + workspace_id: &str, + send_env: &[String], + agent: Option<&str>, + ) -> Self { + let path = dir.join(control_key(&host_alias(workspace_id), send_env, agent)); + // Bytes rather than characters: `sun_path` is a byte buffer. + if path.as_os_str().as_encoded_bytes().len() > CONTROL_PATH_LIMIT { + return Self::Direct; + } + // OpenSSH runs `ControlPath` through `percent_expand` before it binds + // anything, and an unknown key there — or a `%` with nothing after it — + // is `fatal()`, which takes the session with it. The derived name is hex, + // but the directory above it is the user's cache directory and dl does not + // get to say what is in that. A `%` anywhere in the path therefore means + // do not multiplex, on the same footing as a path that will not fit. + if path.as_os_str().as_encoded_bytes().contains(&b'%') { + return Self::Direct; + } + match prepare(dir) { + Ok(()) => Self::Multiplexed(ControlSocket(path)), + Err(_) => Self::Direct, + } + } +} + +/// Make the socket directory, and make it this user's alone. +/// +/// `0700` is not tidiness. Anyone who can connect to a master's socket gets a +/// session **inside the container**, with no key and no prompt, and OpenSSH does +/// not ask who is on the other end of a socket it connects to. The cache directory +/// above this one is an ordinary `0755`, so the leaf has to say so itself — and a +/// leaf whose mode cannot be set is a leaf dl declines to multiplex through. +fn prepare(dir: &Path) -> std::io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + std::fs::create_dir_all(dir)?; + // Set on every run rather than only at creation: the directory outlives any + // one of them, and a mode loosened by something else must not be inherited in + // silence. + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)) +} + +/// The socket's file name: 16 hex characters of a digest over everything a master +/// decides on behalf of a later caller. +/// +/// Hashed and not concatenated, because a `ControlPath` has ~104 bytes to live in +/// (see [`CONTROL_PATH_LIMIT`]) and an alias alone can spend 30 of them. +/// +/// Each field goes in length-prefixed, so that no two different inputs can encode +/// to the same bytes: `["AB", "C"]` and `["A", "BC"]` are one string once +/// concatenated, and a collision here is exactly the silent cross-permit-list +/// reuse this key exists to prevent. The prefixes are what make that impossible +/// *by construction* rather than by an argument about what an alias or an +/// environment variable name is allowed to contain — which is the kind of +/// argument that stops holding the day somebody widens one of them. +/// +/// 64 bits of the digest: what it has to do is tell a handful of live sockets +/// apart, and the birthday bound on that is many orders of magnitude away. +fn control_key(alias: &str, send_env: &[String], agent: Option<&str>) -> String { + let mut message = String::new(); + let mut field = |value: &str| { + message.push_str(&value.len().to_string()); + message.push(':'); + message.push_str(value); + }; + field(alias); + field(&send_env.len().to_string()); + for name in send_env { + field(name); + } + // The marker keeps "no agent" apart from "an agent at the empty path", which + // are different sessions and so must be different sockets. + field(&match agent { + Some(socket) => format!("agent:{socket}"), + None => "none".to_owned(), + }); + + use sha2::Digest as _; + let digest = sha2::Sha256::digest(message.as_bytes()); + digest[..8] + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + /// Build the OpenSSH invocation that runs `command` under a pty. /// /// `config` is required, and it is the whole reason this function takes a path at @@ -132,12 +331,20 @@ pub(crate) fn host_alias(workspace_id: &str) -> String { /// empty one names no directory: landing in the `workspaceFolder` from /// devcontainer.json is the right default, and `cd '' &&` would fail for no /// reason. +/// +/// `reuse` decides whether this session shares a connection, and it arrives as a +/// value rather than being derived here for the reason `config` does: the socket's +/// identity covers `send_env`, so a second derivation is a second chance to +/// disagree with the list actually being sent. A [`Reuse::Multiplexed`] adds three +/// options and nothing else; [`Reuse::Direct`] adds none, and the two argvs run the +/// same command with the same result. pub(crate) fn command_args( config: &Path, workspace_id: &str, command: &str, send_env: &[String], workdir: Option<&str>, + reuse: &Reuse, ) -> Result, UnsafeRequest> { if workspace_id.starts_with('-') { return Err(UnsafeRequest::OptionLikeWorkspaceId { @@ -150,6 +357,18 @@ pub(crate) fn command_args( config.display().to_string(), "-t".to_owned(), ]; + if let Reuse::Multiplexed(socket) = reuse { + // `auto` and not `yes`: the first trip opens the master and every trip + // after it joins one, so dl spawns no extra process and there is no + // pre-warm to get wrong. `yes` would make a second concurrent trip fail + // rather than share. + args.push("-o".to_owned()); + args.push("ControlMaster=auto".to_owned()); + args.push("-o".to_owned()); + args.push(format!("ControlPath={}", socket.as_path().display())); + args.push("-o".to_owned()); + args.push(format!("ControlPersist={CONTROL_PERSIST}")); + } for name in send_env { args.push("-o".to_owned()); args.push(format!("SendEnv={name}")); @@ -397,8 +616,15 @@ mod tests { const A_CONFIG: &str = "/scratch/ssh_config"; fn args_for(workspace_id: &str, command: &str) -> Vec { - command_args(Path::new(A_CONFIG), workspace_id, command, &[], None) - .expect("a well-formed request") + command_args( + Path::new(A_CONFIG), + workspace_id, + command, + &[], + None, + &Reuse::Direct, + ) + .expect("a well-formed request") } // ------------------------------------------------- the OpenSSH invocation @@ -406,14 +632,53 @@ mod tests { #[test] fn the_whole_argv_is_what_dl_hands_to_openssh() { // The argv *is* the contract: `-F` naming the config the alias was found - // in, `-t` before the alias, the alias positionally, one payload argument - // last. + // in, `-t` before the alias, the three multiplexing options, the permit + // list by name, the alias positionally, one payload argument last. + let socket = ControlSocket(PathBuf::from("/scratch/ssh-control/0123456789abcdef")); let args = command_args( Path::new("/scratch/ssh_config"), "devlaunch-main-abcdefgh", "bash -lc claude", &["GH_TOKEN".to_owned()], Some("/workspaces/devlaunch"), + &Reuse::Multiplexed(socket), + ) + .expect("a well-formed request"); + + assert_eq!( + args, + vec![ + "ssh".to_owned(), + "-F".to_owned(), + "/scratch/ssh_config".to_owned(), + "-t".to_owned(), + "-o".to_owned(), + "ControlMaster=auto".to_owned(), + "-o".to_owned(), + "ControlPath=/scratch/ssh-control/0123456789abcdef".to_owned(), + "-o".to_owned(), + "ControlPersist=60".to_owned(), + "-o".to_owned(), + "SendEnv=GH_TOKEN".to_owned(), + "devlaunch-main-abcdefgh.devpod".to_owned(), + "cd /workspaces/devlaunch && bash -lc claude".to_owned(), + ] + ); + } + + #[test] + fn a_session_that_cannot_multiplex_carries_no_control_options_at_all() { + // The other arm of the same pin. `Direct` is not "multiplexing that + // failed": it is an argv with nothing about a control socket in it, which + // is what dl sent before this existed and what it must still be able to + // send. + let args = command_args( + Path::new("/scratch/ssh_config"), + "devlaunch-main-abcdefgh", + "bash -lc claude", + &["GH_TOKEN".to_owned()], + Some("/workspaces/devlaunch"), + &Reuse::Direct, ) .expect("a well-formed request"); @@ -445,7 +710,7 @@ mod tests { // this function looks up, because a second lookup is a second chance to // disagree with the first. for config in ["/scratch/ssh_config", "/home/dev/.ssh/config"] { - let args = command_args(Path::new(config), "myws", "true", &[], None) + let args = command_args(Path::new(config), "myws", "true", &[], None, &Reuse::Direct) .expect("a well-formed request"); let flag = args @@ -511,6 +776,7 @@ mod tests { "bash -lc claude", &["GH_TOKEN".to_owned()], None, + &Reuse::Direct, ) .expect("a well-formed request"); @@ -537,6 +803,7 @@ mod tests { "bash -lc make", &[], Some("/workspaces/myws"), + &Reuse::Direct, ) .expect("a well-formed request"); @@ -556,6 +823,7 @@ mod tests { "bash -lc make", &[], Some("/a dir/with space"), + &Reuse::Direct, ) .expect("a well-formed request"); @@ -585,6 +853,7 @@ mod tests { "bash -lc make", &[], Some(workdir), + &Reuse::Direct, ) .expect("a well-formed request"); @@ -597,8 +866,15 @@ mod tests { // Python's `if workdir:` reads an empty flag value as no workdir; landing // in the workspaceFolder from devcontainer.json is the right default, and // `cd '' &&` would be a payload that fails for no reason. - let args = command_args(Path::new(A_CONFIG), "myws", "bash -lc make", &[], Some("")) - .expect("well-formed"); + let args = command_args( + Path::new(A_CONFIG), + "myws", + "bash -lc make", + &[], + Some(""), + &Reuse::Direct, + ) + .expect("well-formed"); assert_eq!(args.last().map(String::as_str), Some("bash -lc make")); } @@ -617,7 +893,8 @@ mod tests { workspace_id, "bash -lc claude", &[], - None + None, + &Reuse::Direct ), Err(UnsafeRequest::OptionLikeWorkspaceId { workspace_id: workspace_id.to_owned() @@ -631,7 +908,15 @@ mod tests { fn an_ordinary_workspace_id_is_not_refused() { for workspace_id in ["devlaunch-main-abcdefgh", "my_ws.2", "ws-1"] { assert!( - command_args(Path::new(A_CONFIG), workspace_id, "true", &[], None).is_ok(), + command_args( + Path::new(A_CONFIG), + workspace_id, + "true", + &[], + None, + &Reuse::Direct + ) + .is_ok(), "{workspace_id:?}" ); } @@ -644,13 +929,255 @@ mod tests { // shell mangles it — and an argument that cannot mean what it says is // better refused than sent. assert_eq!( - command_args(Path::new(A_CONFIG), "myws", "true", &[], Some("/a\0dir")), + command_args( + Path::new(A_CONFIG), + "myws", + "true", + &[], + Some("/a\0dir"), + &Reuse::Direct + ), Err(UnsafeRequest::UnquotableWorkdir { workdir: "/a\0dir".to_owned() }) ); } + // -------------------------------------------- which master this may share + + /// Every permit list dl can build, plus the pair that collides under a join + /// that does not length-prefix its fields: same number of names, same bytes + /// once run together. + const PERMIT_LISTS: [&[&str]; 6] = [ + &[], + &["GH_TOKEN"], + &["CLAUDE_CODE_OAUTH_TOKEN"], + &["GH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"], + &["AB", "C"], + &["A", "BC"], + ]; + + fn owned(names: &[&str]) -> Vec { + names.iter().map(|name| (*name).to_owned()).collect() + } + + fn sockets_dir() -> (tempfile::TempDir, PathBuf) { + let cache = tempfile::tempdir().expect("a scratch cache"); + let dir = cache.path().join(CONTROL_DIR); + (cache, dir) + } + + fn socket_of(reuse: &Reuse) -> &Path { + match reuse { + Reuse::Multiplexed(socket) => socket.as_path(), + Reuse::Direct => panic!("expected a multiplexed session, got Direct"), + } + } + + #[test] + fn two_send_env_permit_lists_never_derive_one_socket() { + // The mandatory property, and the whole reason the path is derived rather + // than written: a master filters `SendEnv` against *its own* list at exit + // 0, so a client that found a master with a different list would get an + // empty `GH_TOKEN` and an unauthenticated `gh` with no error anywhere + // (devlaunch#389). A different list has to be a different socket, for + // every pair of lists, not merely for the pair someone thought of. + let (_cache, dir) = sockets_dir(); + + let mut seen: Vec<(&[&str], PathBuf)> = Vec::new(); + for list in PERMIT_LISTS { + let reuse = Reuse::derive(&dir, "myws", &owned(list), Some("/run/agent")); + let path = socket_of(&reuse).to_path_buf(); + for (earlier, taken) in &seen { + assert_ne!( + &path, taken, + "{list:?} and {earlier:?} would share a master, and so would \ + share {earlier:?}'s permit list" + ); + } + seen.push((list, path)); + } + } + + #[test] + fn the_same_request_derives_the_same_socket_every_time() { + // The other half of the property: keying on the permit list is only a + // saving if two runs that agree about it *do* find each other's master. + let (_cache, dir) = sockets_dir(); + let permit = owned(&["GH_TOKEN"]); + + let first = Reuse::derive(&dir, "myws", &permit, Some("/run/agent")); + let again = Reuse::derive(&dir, "myws", &permit, Some("/run/agent")); + + assert_eq!(first, again); + } + + #[test] + fn the_agent_socket_a_master_pins_is_part_of_its_key() { + // #389's other finding: a reused master forwards whichever agent opened + // it, whatever the later client's `SSH_AUTH_SOCK` says. Same move, same + // reason — a difference the master would silently override is a + // difference in the key. + let (_cache, dir) = sockets_dir(); + let permit = owned(&["GH_TOKEN"]); + + let paths: Vec = [ + None, + Some(""), + Some("/run/user/1000/keyring/ssh"), + Some("/tmp/agent"), + ] + .into_iter() + .map(|agent| socket_of(&Reuse::derive(&dir, "myws", &permit, agent)).to_path_buf()) + .collect(); + + for (at, path) in paths.iter().enumerate() { + assert!( + !paths[..at].contains(path), + "two agents share a master: {paths:?}" + ); + } + } + + #[test] + fn two_workspaces_never_share_a_master() { + let (_cache, dir) = sockets_dir(); + let permit = owned(&["GH_TOKEN"]); + + let one = Reuse::derive(&dir, "devlaunch-main-abcdefgh", &permit, None); + let other = Reuse::derive(&dir, "devlaunch-main-ijklmnop", &permit, None); + + assert_ne!(one, other); + } + + #[test] + fn the_socket_is_a_short_hex_name_under_the_directory_it_was_given() { + // Hashed rather than concatenated, because the path has to fit in + // `sun_path`: an alias alone is longer than the name derived from it. + let (_cache, dir) = sockets_dir(); + + let reuse = Reuse::derive(&dir, "devlaunch-main-abcdefgh", &[], None); + + let socket = socket_of(&reuse); + assert_eq!(socket.parent(), Some(dir.as_path())); + let name = socket + .file_name() + .and_then(|name| name.to_str()) + .expect("a file name"); + assert_eq!(name.len(), 16, "{name:?}"); + assert!( + name.bytes().all(|byte| byte.is_ascii_hexdigit()), + "{name:?}" + ); + } + + #[test] + fn a_socket_path_too_long_for_sun_path_leaves_the_session_direct() { + // A unix socket path has ~104 bytes to live in, and OpenSSH's own reaction + // to a `bind()` that fails for any other reason is `fatal()` — it would + // take the session with it. A path that will not fit means *do not + // multiplex*, never *build a master ssh will refuse*. + let too_deep = PathBuf::from("/tmp").join("x".repeat(CONTROL_PATH_LIMIT)); + + let reuse = Reuse::derive(&too_deep, "myws", &[], None); + + assert_eq!(reuse, Reuse::Direct); + // And nothing was created on the way to saying so. + assert!(!too_deep.exists()); + } + + #[test] + fn the_room_openssh_takes_for_its_own_temporary_socket_is_counted_too() { + // `muxserver_listen` binds `.<16 characters>` and renames it + // into place, so what has to fit in `sun_path` is 17 bytes longer than what + // dl composes. This band -- a path that fits on its own and does not fit + // once OpenSSH has added its suffix -- is where the first CI run of this + // change took the whole e2e suite down: `unix_listener: path ... too long + // for Unix domain socket`, exit 255, thirteen tests. A directory dl can + // really create, so that the only thing under test is the length. + let cache = tempfile::tempdir().expect("a scratch cache"); + let base = cache.path().as_os_str().as_encoded_bytes().len(); + // A socket a few bytes past the budget and still inside `sun_path`. + let want_socket = CONTROL_PATH_LIMIT + 5; + let want_dir = want_socket - 1 - 16; + assert!( + want_dir > base + 1, + "the scratch path is too long to build this fixture in" + ); + let dir = cache.path().join("d".repeat(want_dir - base - 1)); + assert_eq!(dir.as_os_str().as_encoded_bytes().len(), want_dir); + assert!( + want_socket < SUN_PATH, + "the fixture has to fit in sun_path on its own, or it is the other \ + test and proves nothing about the suffix" + ); + + let reuse = Reuse::derive(&dir, "myws", &[], None); + + assert_eq!( + reuse, + Reuse::Direct, + "a path OpenSSH cannot bind its temporary socket beside is a path dl \ + must not multiplex through" + ); + assert!(!dir.exists(), "nothing was created on the way to saying so"); + } + + #[test] + fn a_cache_directory_with_a_percent_in_it_leaves_the_session_direct() { + // `ControlPath` is percent-expanded by OpenSSH before it is bound, and an + // unknown key is `fatal()` rather than a warning — so a user whose + // `XDG_CACHE_HOME` holds a `%` would have had every terminal session die, + // which is the one outcome this mechanism is not allowed to cause. Nothing + // in the derived name can hold one; everything above it is the user's. + let cache = tempfile::tempdir().expect("a scratch cache"); + let dir = cache.path().join("100%-cache").join(CONTROL_DIR); + + let reuse = Reuse::derive(&dir, "myws", &[], None); + + assert_eq!(reuse, Reuse::Direct); + } + + #[test] + fn a_socket_directory_dl_cannot_make_leaves_the_session_direct() { + // Fail closed: the `LAUNCH_LOCK_DIR` hazard in a new place — something + // else owns that name, and the session still has to run. + let cache = tempfile::tempdir().expect("a scratch cache"); + let dir = cache.path().join(CONTROL_DIR); + std::fs::write(&dir, "not a directory").expect("something in the way"); + + let reuse = Reuse::derive(&dir, "myws", &[], None); + + assert_eq!(reuse, Reuse::Direct); + } + + #[test] + fn the_socket_directory_is_this_users_alone() { + // Anyone who can connect to a master's socket gets a session inside the + // container, with no key and no prompt, and OpenSSH does not ask who is on + // the other end. The cache directory above this one is an ordinary 0755. + use std::os::unix::fs::PermissionsExt as _; + + let (_cache, dir) = sockets_dir(); + + let reuse = Reuse::derive(&dir, "myws", &[], None); + + assert!(matches!(reuse, Reuse::Multiplexed(_))); + let mode = std::fs::metadata(&dir) + .expect("the socket directory") + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o700, "{mode:o}"); + } + + #[test] + fn a_master_lingers_for_a_minute_and_no_longer() { + // Named once, and named here so that moving it is a decision rather than a + // typo: a master holds a resident `devpod ssh --stdio` and a `docker exec` + // per key, and devpod's own container inactivity example is 10m. + assert_eq!(CONTROL_PERSIST, 60); + } + // ------------------------------------------------------- is there a tty #[test] @@ -919,6 +1446,7 @@ mod tests { "bash -lc claude", &["GH_TOKEN".to_owned()], None, + &Reuse::Direct, ) .expect("well-formed"); let env = EnvSpec::inherited().and("GH_TOKEN", "gho_secret"); diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 6c9abac7..cbfa5a07 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -154,6 +154,13 @@ pub(crate) const CONTEXT_OPTIONS_TTL: Duration = Duration::from_secs(3600); /// `--workspace-env` on every `up` while a mount only lands at creation. pub(crate) const PIXI_CACHE_TARGET: &str = "/var/tmp/devlaunch-pixi"; +/// `$SSH_AUTH_SOCK`, OpenSSH's own name for the agent it would forward. +/// +/// Named here rather than read as a literal for [`ssh::CONFIG_VAR`]'s reason: it +/// is somebody else's spelling, and one place to look for it is the whole of what +/// keeps a rename from becoming a silently shared master. +pub(crate) const SSH_AUTH_SOCK_VAR: &str = "SSH_AUTH_SOCK"; + /// The leaf under devlaunch's cache directory that the launch locks live in. /// /// Its own directory rather than the repo cache: this lock is keyed by workspace, @@ -192,6 +199,13 @@ pub struct Host { /// from — so it names the file devpod publishes its host aliases into, and /// the only one it publishes them into. pub(crate) devpod_ssh_config: Option, + /// `$SSH_AUTH_SOCK`: the agent this run would forward. + /// + /// Read for one purpose, and it is not a flag — it goes into the identity of + /// the ssh control socket. A reused master forwards whichever agent opened it + /// and ignores the later client's, so two runs with different agents must not + /// find one another's master (devlaunch#389). + pub(crate) ssh_auth_sock: Option, /// The home directory, which holds the `~/.ssh/config` devpod falls back to /// and expands a `~/` in any of the paths above. `None` on a machine with no /// home directory; `dl` still runs there when `XDG_CACHE_HOME` is set, and @@ -233,6 +247,7 @@ impl Host { stdout_tty: is_a_terminal(libc::STDOUT_FILENO), stderr_tty: is_a_terminal(libc::STDERR_FILENO), devpod_ssh_config: crate::osext::env_str(ssh::CONFIG_VAR), + ssh_auth_sock: crate::osext::env_str(SSH_AUTH_SOCK_VAR), home: crate::osext::home_dir(), cache_dir: cache_dir.into(), devpod_home: DevpodHome::locate(), @@ -246,6 +261,16 @@ impl Host { .join(format!("{workspace_id}.lock")) } + /// Where this host's ssh control sockets live. + /// + /// Under the cache dir, so it follows `XDG_CACHE_HOME` like the rest of dl's + /// storage: a scratch run gets scratch sockets, and `dl --purge` takes them + /// away with everything else. Losing them costs the next trip its ~2s and + /// nothing more, which is what makes that correct by construction. + pub(crate) fn ssh_control_dir(&self) -> PathBuf { + self.cache_dir.join(ssh::CONTROL_DIR) + } + /// The host directory containers share their downloaded pixi packages through. /// /// Under devlaunch's own cache dir, so it follows `XDG_CACHE_HOME` like the @@ -1982,12 +2007,22 @@ fn ssh_with_terminal( gh::openssh_forwarding(session.forwarded_token(notices)), session.forwarded_claude().as_ref(), ); + // Derived from the permit list that is about to be sent, not from one read + // again somewhere else: a master filters `SendEnv` against its own list in + // silence, so the list and the socket it is carried over have to be one fact. + let reuse = ssh::Reuse::derive( + &session.host.ssh_control_dir(), + workspace_id, + &forwarding.args, + session.host.ssh_auth_sock.as_deref(), + ); let args = ssh::command_args( config, workspace_id, payload.as_str(), &forwarding.args, workdir, + &reuse, ) .map_err(SessionRefused::UnsafeRequest)?; notices.say(LaunchNotice::SshCommand { argv: args.clone() }); @@ -5875,6 +5910,10 @@ mod tests { // `~/.ssh/config`, which is exactly that host. let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); let published = published_config(&scene.host); + let socket = ssh::Reuse::derive(&scene.host.ssh_control_dir(), "myws", &[], None); + let ssh::Reuse::Multiplexed(socket) = socket else { + panic!("a scratch cache is short enough to multiplex through"); + }; let (session, _, _) = a_session(&scene, Some("claude")); @@ -5891,6 +5930,12 @@ mod tests { "-F".to_owned(), published.display().to_string(), "-t".to_owned(), + "-o".to_owned(), + "ControlMaster=auto".to_owned(), + "-o".to_owned(), + format!("ControlPath={}", socket.as_path().display()), + "-o".to_owned(), + "ControlPersist=60".to_owned(), "myws.devpod".to_owned(), "bash -lc claude".to_owned(), ]] @@ -6136,6 +6181,74 @@ mod tests { assert!(!argv.iter().any(|arg| arg.contains("gho_secretvalue"))); } + #[test] + fn a_run_with_a_token_and_a_run_without_never_share_a_master() { + // devlaunch#389's silent failure, closed at the flow rather than at the + // digest: a master opened by the run with no token filters `SendEnv` + // against its own empty permit list, so the run *with* a token would get + // an empty `GH_TOKEN` and an unauthenticated `gh` at exit 0. The two runs + // cannot find each other's socket, so the state has nowhere to happen. + // + // The file names are compared rather than the whole paths, because each + // scene has a scratch cache of its own; the name is the key. + let with_token = logged_in(Scene::new().on_a_terminal(&["myws"]).with_running("myws")); + let without = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); + + let _ = a_session(&with_token, Some("claude")); + let _ = a_session(&without, Some("claude")); + + let keyed = |scene: &Scene| -> String { + let argv = scene + .runner + .argvs() + .into_iter() + .next() + .expect("an ssh call"); + let path = argv + .iter() + .find_map(|arg| arg.strip_prefix("ControlPath=")) + .unwrap_or_else(|| panic!("no ControlPath in {argv:?}")) + .to_owned(); + Path::new(&path) + .file_name() + .and_then(|name| name.to_str()) + .expect("a socket name") + .to_owned() + }; + + assert!( + keyed(&with_token) != keyed(&without), + "a permit list of [GH_TOKEN] and one of [] share a master, so the \ + forwarded token would be filtered away in silence" + ); + } + + #[test] + fn the_control_socket_lives_under_devlaunchs_own_cache_directory() { + // Under `XDG_CACHE_HOME` like the rest of dl's storage, in a leaf of its + // own so the cache's walkers do not read it as a repo, and `dl --purge` + // takes it away with everything else. + let scene = Scene::new().on_a_terminal(&["myws"]).with_running("myws"); + + let _ = a_session(&scene, Some("claude")); + + let argv = scene + .runner + .argvs() + .into_iter() + .next() + .expect("an ssh call"); + let path = argv + .iter() + .find_map(|arg| arg.strip_prefix("ControlPath=")) + .unwrap_or_else(|| panic!("no ControlPath in {argv:?}")); + assert_eq!( + Path::new(path).parent(), + Some(scene.host.ssh_control_dir().as_path()) + ); + assert!(scene.host.ssh_control_dir().is_dir()); + } + #[test] fn no_token_means_no_forwarding_flags_at_all() { // The default scene has forwarding opted out. From ae71b57ea9fefc5bbce28dda9b988855f7e88a00 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 21:02:46 +0100 Subject: [PATCH 2/2] fix: DRAIN_GRACE bounds the drain, not each pipe `capture` computed the 500ms grace inside each `collect` and drained the two pipes serially, so one descendant holding both write ends was charged twice: a measured 1.008s where the bound says 500ms. One deadline is now taken once and handed to both calls. Nothing is lost by sharing it. Both drain threads start before the wait for the child does, so the second pipe has had the same grace to reach EOF in by the time it is asked. Decided on latency rather than correctness: a timed-out drain keeps the bytes it read either way. The held-pipe case the bound exists for is named in the code as git's ssh ControlMaster. #422 puts a ControlMaster on dl's hottest path, so the multi-pipe shape stops being occasional, which is why this lands here rather than being rediscovered later as a latency mystery. Closes #501. --- CHANGELOG.md | 12 +++++++ rust/devlaunch-runner/src/lib.rs | 53 ++++++++++++++++++++++-------- rust/devlaunch-runner/src/tests.rs | 51 +++++++++++++++++++++++++--- 3 files changed, 98 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8df9a662..d41bd667 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,6 +149,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A captured command no longer pays the drain grace twice.** When a command + exits while a descendant it forked still holds the pipes open, `dl` waits a + short grace for the last of the output rather than waiting for an end of file + that is never coming. That grace was charged per pipe and the two pipes were + drained one after the other, so a single stuck descendant, which holds stdout + and stderr together, cost 1s of dead waiting on a 500ms grace. It is now one + deadline shared by both drains, so the grace bounds the drain. Nothing is given + up by sharing it: both drains start before the wait for the command does, so + both have had the same grace to finish in. The shape used to be an occasional + `git fetch` over ssh; with a connection master now open on `dl`'s own hottest + path it is the common one. + - **Sixty-six citations that pointed at nothing now point at something, and a guard keeps it that way.** Comments across `rust/` name the test that pins the behaviour they describe, which is most of what makes them worth reading. diff --git a/rust/devlaunch-runner/src/lib.rs b/rust/devlaunch-runner/src/lib.rs index 268288be..c892bb81 100644 --- a/rust/devlaunch-runner/src/lib.rs +++ b/rust/devlaunch-runner/src/lib.rs @@ -488,16 +488,28 @@ impl Runner for ProcessRunner { match ending { // Bounded, not joined (see [`collect`] and [`DRAIN_GRACE`]): the // child has exited, but a descendant it forked into a session of its - // own — git's ssh ControlMaster is the production case — can hold - // the pipe open with no EOF ever coming, and this is the path that - // reaches that far the more often of the two. - Ending::Ended(exit) => Outcome::Ran { - exit, - io: CapturedText { - stdout: collect(stdout), - stderr: collect(stderr), - }, - }, + // own — ssh's ControlMaster is the production case, and dl's own + // pty transport now opens one per workspace — can hold the pipe open + // with no EOF ever coming, and this is the path that reaches that far + // the more often of the two. + // + // **One deadline, computed here and shared by both drains**, so that + // [`DRAIN_GRACE`] bounds the drain rather than each pipe. A descendant + // that inherited the write ends holds *both*, so a per-pipe bound is + // paid twice for one stuck child — 1s of dead wait on a 500ms grace, + // on the hot path (devlaunch#501). Nothing is lost by sharing it: the + // stderr drain starts when the stdout one does, so its grace has been + // running all along. + Ending::Ended(exit) => { + let deadline = Instant::now() + DRAIN_GRACE; + Outcome::Ran { + exit, + io: CapturedText { + stdout: collect(stdout, deadline), + stderr: collect(stderr, deadline), + }, + } + } // The same bound, at zero: a timed-out outcome drops whatever was // written (see [`Outcome::TimedOut`]), so there is nothing here to // wait even a moment for. The drains are dropped mid-read and the @@ -921,13 +933,18 @@ fn drain(pipe: Option) -> Option { }) } -/// What a drain has read, waiting up to [`DRAIN_GRACE`] for it to reach EOF. +/// What a drain has read, waiting until `deadline` for it to reach EOF. /// /// Called once the child is gone, so the wait is for the pipe to close rather /// than for anything more to be written; when it expires the reading thread is /// abandoned with the pipe it will never see the end of, and the bytes it did /// read are the answer. /// +/// The deadline arrives from the caller rather than being computed here, and that +/// is what makes [`DRAIN_GRACE`] a bound on the *drain* instead of on each pipe: +/// one stuck descendant holds both write ends, so a grace computed per call is +/// spent once per pipe for a single stuck child. +/// /// A condvar rather than a poll of `JoinHandle::is_finished`, because this is on /// the hot path: every capture ends here, and the drain thread usually reaches /// EOF a few microseconds *after* the wait for the child returned. Polling at @@ -935,12 +952,11 @@ fn drain(pipe: Option) -> Option { /// twice, against a `capture` that otherwise costs well under a millisecond — /// where waiting to be woken costs the microseconds it actually takes. The bound /// is the same; only the waiting is exact. -fn collect(drain: Option) -> String { +fn collect(drain: Option, deadline: Instant) -> String { let Some(drain) = drain else { return String::new(); }; let (reading, ended) = &*drain.read; - let deadline = Instant::now() + DRAIN_GRACE; let mut read = held(reading); while !read.ended { let left = deadline.saturating_duration_since(Instant::now()); @@ -1012,9 +1028,18 @@ fn kill(child: &mut Child) -> Ending { const POLL_INTERVAL: Duration = Duration::from_millis(5); -/// How long [`collect`] gives a drained pipe to reach EOF once the child has +/// How long a capture gives its pipes, together, to reach EOF once the child has /// exited. /// +/// The whole drain and not each pipe of it. [`ProcessRunner::capture`] takes this +/// deadline once and hands the same instant to both [`collect`] calls, because the +/// case the bound exists for holds *both* write ends: a descendant in a session of +/// its own inherited the child's stdout and stderr together, so a grace charged +/// per pipe is charged twice for one stuck child and the drain costs 1s where it +/// was meant to cost 500ms (devlaunch#501). The second drain loses nothing by it — +/// both threads started before the wait for the child did, so both have had the +/// same grace to reach EOF in. +/// /// A bound rather than an unbounded join, and this is the whole of #302's fix. /// The child is gone by the time it is waited on, so everything it wrote is /// already read or sitting in the pipe buffer and a moment covers the rest; what diff --git a/rust/devlaunch-runner/src/tests.rs b/rust/devlaunch-runner/src/tests.rs index cc653f70..4ef38788 100644 --- a/rust/devlaunch-runner/src/tests.rs +++ b/rust/devlaunch-runner/src/tests.rs @@ -412,6 +412,46 @@ fn a_capture_returns_when_a_grandchild_holds_the_pipe_past_a_clean_exit() { ); } +/// The same shape again, timed: one stuck descendant holds *both* write ends, so +/// the drain must cost one [`DRAIN_GRACE`] and not one per pipe. +/// +/// `setsid sleep 30` inherits the child's stdout and stderr together, which is +/// exactly what an ssh ControlMaster does — and dl now opens one of those on its +/// hottest path, so this went from an occasional git shape to the common one +/// (devlaunch#501). With the grace computed inside each `collect` the two waits +/// are serial and the capture costs 1s; with one deadline shared by both it costs +/// 500ms, and nothing is given up, because both drain threads started before the +/// wait for the child did. +#[test] +fn two_pipes_one_descendant_holds_cost_one_grace_between_them() { + require_setsid(); + let spec = SpawnSpec::from(sh("setsid sleep 30 & printf done")); + + let started = Instant::now(); + let outcome = within( + Duration::from_secs(5), + "capture never returned: the success path is waiting on a pipe a \ + grandchild still holds", + move || ProcessRunner.capture(&spec), + ); + let took = started.elapsed(); + + let (_, io) = ran(outcome); + assert_eq!(io.stdout, "done"); + // Proof the pipes really were held: an EOF that arrived would have made this + // return in microseconds and the bound below would pass having pinned nothing. + assert!( + took >= DRAIN_GRACE * 4 / 5, + "the drain did not wait at all ({took:?}), so the grandchild cannot have \ + been holding the pipes and this test proves nothing" + ); + assert!( + took < DRAIN_GRACE * 8 / 5, + "the drain cost {took:?}, which is more than one grace of {DRAIN_GRACE:?}: \ + the two pipes are each being given their own" + ); +} + #[test] fn a_timeout_that_is_not_reached_answers_normally() { let spec = SpawnSpec::from(sh("printf quick")).with_timeout(Duration::from_secs(30)); @@ -708,10 +748,13 @@ impl Read for Interrupting { #[test] fn a_drain_treats_an_interrupted_read_as_a_retry_not_an_ending() { let pieces = vec!["ab", "cd", "ef"]; - let drained = collect(drain(Some(Interrupting { - pieces: pieces.into_iter(), - interrupt_next: false, - }))); + let drained = collect( + drain(Some(Interrupting { + pieces: pieces.into_iter(), + interrupt_next: false, + })), + Instant::now() + DRAIN_GRACE, + ); assert_eq!( drained, "abcdef", "an interrupted read ended the drain and truncated the output"