diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ff7a2e6..dcfd360c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.39.0] - 2026-09-09 + +### Fixed + +- **A launch cut short between `devpod up` and the end of dl's setup pass left a + workspace that never got provisioned, and no later launch ever noticed.** The + `up` finishes, devpod writes its create result, and what runs next is dl's own + `devpod ssh --command`: the hostname, the terminal title, the onboarding memo, + `gh` and `claude`. Ctrl-C there and the container is left running with none of + it. Nothing about that container says so. `devpod status` answers `Running` and + devpod's create record is complete, which is exactly what the fast attach asks, + so `dl ` attaches in one round trip and provisions nothing, and does + the same on every launch after that. The workspace is permanently half built and + the recovery is knowing to type `dl up`, which nothing says either. + That is the "sometimes it works" of it: whether you get a whole workspace + depends on which second the Ctrl-C landed in. + + A pass now writes down that it is running, before its first trip, in + `${XDG_CACHE_HOME:-~/.cache}/devlaunch/tool-verdicts/.pass`, and + removes it after its last. dl's signal handler `_exit`s without unwinding, so a + record left standing is the one thing on the host that says a pass was cut + short, and a launch that finds one runs the pass before it hands over a shell. + The record carries the same `workspace_result.json` mtime the verdict marker + does, so one left by a container that has since been rebuilt describes nothing + standing and is ignored. + + **After the last trip and not the first**, which is most of the fix rather than + a detail of it. A pass is up to three trips and the probe is the ~1.7s one; the + lend streams the host's binaries and the install fetches a `claude` of a few + hundred megabytes. A record closed when the probe answered would be open for the + window nobody interrupts and shut for the window everybody does. The one outcome + that leaves it standing is a trip the OS would not make at all, because nothing + was learned about the container and nothing was done to it. + + It is positive evidence, never the absence of it. A workspace brought up by VS + Code, by a hand-typed `devpod up`, or by a build older than the record has no + record either, and those go on attaching in one round trip as before. + ## [0.38.0] - 2026-09-09 ### Changed diff --git a/README.md b/README.md index 44dd4bb9..807e1c01 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ one argument instead of a clone, a config file and a build command. [![GitHub pull-requests merged](https://badgen.net/github/merged-prs/blooop/devlaunch)](https://github.com/blooop/devlaunch/pulls?q=is%3Amerged) [![GitHub release](https://img.shields.io/github/release/blooop/devlaunch.svg)](https://GitHub.com/blooop/devlaunch/releases/) [![PyPI](https://img.shields.io/pypi/v/devlaunch)](https://pypi.org/project/devlaunch/) -[![Conda](https://img.shields.io/badge/conda-v0.38.0-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) +[![Conda](https://img.shields.io/badge/conda-v0.39.0-brightgreen?logo=anaconda)](https://prefix.dev/channels/blooop/packages/devlaunch) [![License](https://img.shields.io/github/license/blooop/devlaunch)](https://opensource.org/license/mit/) [![Platform](https://img.shields.io/badge/platform-linux--64-blue)](https://github.com/blooop/devlaunch/releases) [![Pixi Badge](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/prefix-dev/pixi/main/assets/badge/v0.json)](https://pixi.sh) @@ -276,7 +276,7 @@ clone, and [docs/cleanup.md](docs/cleanup.md) says what it carries one past and ```bash $ dl --version -dl 0.38.0 +dl 0.39.0 ``` `--devcontainer ` picks a non-default `devcontainer.json`. A bare name means diff --git a/docs/workspace-tools.md b/docs/workspace-tools.md index 3f0b4122..9868d9a9 100644 --- a/docs/workspace-tools.md +++ b/docs/workspace-tools.md @@ -491,6 +491,35 @@ Nothing has to be cleaned up, and there is nothing to invalidate by hand: the markers are compared, never trusted on age, and deleting the whole directory costs one round trip on each workspace's next launch. +### A pass that was interrupted + +A pass is up to three trips and the slow ones are the last two: the lend streams +the host's binaries, and the install fetches a `claude` of a few hundred megabytes. +A Ctrl-C lands in the middle of that often. What it leaves is the awkward case: +`devpod up` has already finished, so devpod wrote its create result and the +container is running, and everything the Ctrl-C stopped was dl's. The container +looks finished to everything that can be asked about it, so a later launch takes +the fast path and provisions nothing, forever. + +So a pass writes down that it is running before its first trip, in +`.pass` beside the marker, and removes it after its last. +`dl`'s signal handler exits without unwinding, which is why the evidence has to be +a file left standing rather than one written on the way out: nothing runs on the +way out. A launch that finds one still standing for the container standing now runs +the pass before it hands over a shell, and says so. + +The record is anchored to the same `workspace_result.json` mtime the marker is, so +one left by a container that has since been rebuilt describes nothing that exists +and is ignored. It is read as positive evidence and never as the absence of it: a +workspace brought up by VS Code, by a hand-typed `devpod up`, or by a build older +than the record has no record either, and those keep attaching in one round trip. + +A pass whose install ran and *failed* closes the record like any other: it happened +and it said so, and re-running it on every attach afterwards would be a container +re-attempting one failing thing forever. The one outcome that leaves the record +standing is a trip the OS would not make at all, because nothing was learned about +the container and nothing was done to it, which is the state the record is for. + ### What to bake so a launch does no work at all To make every `dl` launch of an image stop at trip 1. The probe asks a **login** diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 0384eadc..226ff68c 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -13,7 +13,7 @@ dependencies = [ [[package]] name = "aid" -version = "0.38.0" +version = "0.39.0" dependencies = [ "devlaunch-test-support", "dl", @@ -437,7 +437,7 @@ dependencies = [ [[package]] name = "devlaunch-core" -version = "0.38.0" +version = "0.39.0" dependencies = [ "devlaunch-runner", "devlaunch-test-support", @@ -455,7 +455,7 @@ dependencies = [ [[package]] name = "devlaunch-runner" -version = "0.38.0" +version = "0.39.0" dependencies = [ "libc", "portable-pty", @@ -464,7 +464,7 @@ dependencies = [ [[package]] name = "devlaunch-test-support" -version = "0.38.0" +version = "0.39.0" dependencies = [ "devlaunch-runner", "serde", @@ -506,7 +506,7 @@ dependencies = [ [[package]] name = "dl" -version = "0.38.0" +version = "0.39.0" dependencies = [ "clap", "devlaunch-core", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index ff7a0106..1c78176d 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,7 +11,7 @@ members = [ # The single source of the version (docs/rust-rewrite-plan.md: cutover ships # 0.1.0, version read from Cargo.toml). [workspace.package] -version = "0.38.0" +version = "0.39.0" edition = "2024" license = "MIT" repository = "https://github.com/blooop/devlaunch" diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 97bc07c7..ea953a39 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -81,6 +81,8 @@ pub devlaunch_core::api::LaunchNotice::SessionManagerReady::pane_id: alloc::stri pub devlaunch_core::api::LaunchNotice::SessionManagerReady::socket: alloc::string::String pub devlaunch_core::api::LaunchNotice::SessionManagerUnavailable pub devlaunch_core::api::LaunchNotice::SessionManagerUnavailable::reason: alloc::string::String +pub devlaunch_core::api::LaunchNotice::SetupPassNeverFinished +pub devlaunch_core::api::LaunchNotice::SetupPassNeverFinished::workspace_id: alloc::string::String pub devlaunch_core::api::LaunchNotice::SshCommand pub devlaunch_core::api::LaunchNotice::SshCommand::argv: alloc::vec::Vec pub devlaunch_core::api::LaunchNotice::SshConfigUnlocatable @@ -397,6 +399,7 @@ pub struct devlaunch_core::api::ToolProvisioning<'e> impl<'e> devlaunch_core::flows::launch::ToolProvisioning<'e> pub fn devlaunch_core::flows::launch::ToolProvisioning<'e>::from_env(&std::path::Path, &'e mut dyn devlaunch_core::notices::Notices) -> Self impl devlaunch_core::flows::launch::Provision for devlaunch_core::flows::launch::ToolProvisioning<'_> +pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&self, &str) -> core::option::Option pub struct devlaunch_core::api::WorkspaceCloneManager<'r> @@ -418,9 +421,11 @@ pub trait devlaunch_core::api::Notices pub fn devlaunch_core::api::Notices::say(&mut self, T) impl devlaunch_core::notices::Notices for alloc::vec::Vec pub trait devlaunch_core::api::Provision +pub fn devlaunch_core::api::Provision::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::api::Provision::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::api::Provision::remembered_claude(&self, &str) -> core::option::Option impl devlaunch_core::flows::launch::Provision for devlaunch_core::flows::launch::ToolProvisioning<'_> +pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&self, &str) -> core::option::Option pub fn devlaunch_core::api::enriched_listing(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::listing::DlView<'_>, devlaunch_core::flows::listing::Sizes) -> core::result::Result, devlaunch_core::clients::devpod::ListingUnreadable> @@ -573,6 +578,8 @@ pub devlaunch_core::flows::launch::LaunchNotice::SessionManagerReady::pane_id: a pub devlaunch_core::flows::launch::LaunchNotice::SessionManagerReady::socket: alloc::string::String pub devlaunch_core::flows::launch::LaunchNotice::SessionManagerUnavailable pub devlaunch_core::flows::launch::LaunchNotice::SessionManagerUnavailable::reason: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::SetupPassNeverFinished +pub devlaunch_core::flows::launch::LaunchNotice::SetupPassNeverFinished::workspace_id: alloc::string::String pub devlaunch_core::flows::launch::LaunchNotice::SshCommand pub devlaunch_core::flows::launch::LaunchNotice::SshCommand::argv: alloc::vec::Vec pub devlaunch_core::flows::launch::LaunchNotice::SshConfigUnlocatable @@ -641,6 +648,7 @@ pub struct devlaunch_core::flows::launch::ToolProvisioning<'e> impl<'e> devlaunch_core::flows::launch::ToolProvisioning<'e> pub fn devlaunch_core::flows::launch::ToolProvisioning<'e>::from_env(&std::path::Path, &'e mut dyn devlaunch_core::notices::Notices) -> Self impl devlaunch_core::flows::launch::Provision for devlaunch_core::flows::launch::ToolProvisioning<'_> +pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&self, &str) -> core::option::Option pub trait devlaunch_core::flows::launch::ColdMachinery<'r> @@ -650,9 +658,11 @@ impl<'r> devlaunch_core::flows::launch::ColdMachinery<'r> for devlaunch_core::fl pub fn devlaunch_core::flows::launch::ColdPath<'r, '_>::open(&mut self) -> core::result::Result, devlaunch_core::flows::launch::ColdRefused> pub fn devlaunch_core::flows::launch::ColdPath<'r, '_>::recorded(&mut self) -> core::option::Option<&devlaunch_core::domain::metadata::MetadataStorage> pub trait devlaunch_core::flows::launch::Provision +pub fn devlaunch_core::flows::launch::Provision::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::flows::launch::Provision::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::flows::launch::Provision::remembered_claude(&self, &str) -> core::option::Option impl devlaunch_core::flows::launch::Provision for devlaunch_core::flows::launch::ToolProvisioning<'_> +pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::pass_never_finished(&self, &str) -> bool pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::provision_tools(&self, &dyn devlaunch_runner::Runner, &str, devlaunch_core::flows::provision::PassOccasion, core::option::Option<&str>) -> core::result::Result, devlaunch_core::flows::provision::DevpodMissing> pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&self, &str) -> core::option::Option pub enum devlaunch_core::flows::lifecycle::DeleteStalled diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index ae8aeb1f..a97669ce 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -559,6 +559,10 @@ pub enum LaunchNotice { /// devpod holds a record for this workspace and no create result, so an `up` /// started and never finished. Its container may well be running. CreateNeverFinished { workspace_id: String }, + /// dl's own setup pass over this container began and never finished -- the + /// launch carrying it was killed. The container is sound; the pass is being run + /// again before this launch hands over a session. + SetupPassNeverFinished { workspace_id: String }, /// `dl up` found it already running: nothing to build and nothing to /// wait for. AlreadyRunning { workspace_id: String }, @@ -1321,6 +1325,23 @@ pub trait Provision { fn remembered_claude(&self, _workspace_id: &str) -> Option { None } + + /// Whether a setup pass over the container standing now began and never + /// finished, from the host's own records and without a round trip. + /// + /// Asked on the fast-attach arm, which is the one path that opens a session + /// having provisioned nothing. A `dl` killed between `devpod up` and the end of + /// its pass leaves a container devpod calls Running *and* finished creating, so + /// [`CreateRecord`] cannot tell it from a workspace that is ready -- and every + /// later launch of it takes that same fast path and provisions nothing again. + /// + /// **Positive evidence, never the absence of it.** `false` by default, and + /// `false` for every doubt in the real implementation: a workspace brought up by + /// VS Code, by a hand-typed `devpod up`, or by a build older than the record has + /// no record either, and those must go on attaching in one round trip. + fn pass_never_finished(&self, _workspace_id: &str) -> bool { + false + } } /// A launch that lends nothing — `DEVLAUNCH_NO_TOOLS`, and every test that is not @@ -1434,6 +1455,10 @@ impl Provision for ToolProvisioning<'_> { fn remembered_claude(&self, workspace_id: &str) -> Option { self.verdicts.remembered_claude(workspace_id) } + + fn pass_never_finished(&self, workspace_id: &str) -> bool { + self.verdicts.pass_never_finished(workspace_id) + } } // =========================================================================== @@ -4211,6 +4236,11 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { spec: raw_spec.to_owned(), }); } + // Before the session, because the session is what the missing + // tools are missing *from*: a shell handed over first would be + // the one that goes looking for a `claude` the interrupted pass + // never installed. + self.finish_the_interrupted_pass(placement)?; let session = self.attach(placement, verb.command()); self.forced_refresh(); return session; @@ -4411,6 +4441,39 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { Ok(refused) } + /// Run the setup pass again when the last one over this container was cut + /// short, and do nothing at all otherwise. + /// + /// The fast-attach arm's whole worth is that it opens a session in one round + /// trip, so this asks the host's records and not the container: a workspace with + /// no interrupted pass behind it -- which is every workspace but the one this is + /// for -- pays a file read and nothing else. + /// + /// [`PassOccasion::TopUp`] because the container never stopped, which is that + /// arm's definition. It does not mean fewer stages run: the occasion decides + /// only whether a remembered verdict may excuse the trip, and there is no + /// verdict here -- the pass that would have written one is the pass that was + /// interrupted. + fn finish_the_interrupted_pass(&mut self, placement: &Placement) -> Result<(), LaunchAborted> { + if !self.provision.pass_never_finished(placement.workspace_id()) { + return Ok(()); + } + self.notices.say(LaunchNotice::SetupPassNeverFinished { + workspace_id: placement.workspace_id().to_owned(), + }); + let seen = self + .provision + .provision_tools( + self.context.runner(), + placement.workspace_id(), + PassOccasion::TopUp, + self.container_title(placement.title()).as_deref(), + ) + .map_err(|DevpodMissing| LaunchAborted::DevpodNotRun(NotRun::NotInstalled))?; + self.claude_seen.set(seen); + Ok(()) + } + fn attach( &mut self, placement: &Placement, @@ -4918,6 +4981,9 @@ mod tests { /// What the host's records say about a workspace no pass ran for, which is /// what `dl`'s real implementation reads out of its verdict cache. claude_remembered: Option, + /// Whether the host's records say the last pass over this container was cut + /// short. Read from the same cache the field above is. + pass_never_finished: bool, } impl RecordingProvision { @@ -4972,6 +5038,10 @@ mod tests { fn remembered_claude(&self, _workspace_id: &str) -> Option { self.claude_remembered } + + fn pass_never_finished(&self, _workspace_id: &str) -> bool { + self.pass_never_finished + } } fn no_notices() -> Vec { @@ -9726,6 +9796,102 @@ mod tests { ); } + /// The launch that is cut short between `devpod up` and the end of dl's own + /// setup pass, which is the moment this whole record exists for. + /// + /// A Ctrl-C there leaves a container devpod calls `Running` *and* finished + /// creating -- devpod wrote its result on the way out of the `up`, and + /// everything after it was dl's. So [`CreateRecord`] answers `Completed`, the + /// fast-attach arm fires, and the session opens in a container with no `gh`, no + /// `claude`, no hostname and no zellij. Nothing about the container says + /// otherwise, so every later `dl ` does the same thing: the workspace is + /// permanently half-built, and the only recovery is knowing to type + /// `dl up`. + /// + /// The container itself is sound, so this does not re-run the `up` the way an + /// unfinished *create* does. It runs the pass that did not finish, and then + /// attaches. + #[test] + fn a_running_workspace_whose_setup_pass_never_finished_is_provisioned_before_attaching() { + let scene = Scene::new() + .with_running("myws") + .with_create_completed("myws"); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + parts.provision.pass_never_finished = true; + let mut cold = NeverCold; + let mut launch = Launch::new( + &mut parts.context, + &mut parts.refresh, + &mut cold, + &parts.provision, + &scene.host, + &mut parts.chatter, + &mut parts.said, + ); + + let launched = launch.run("myws", &LaunchVerb::Attach { command: None }, None); + + drop(launch); + assert_eq!( + launched, + Ok(Launched::Session(Session::RemoteExit { status: 0 })), + "the session still opens: the container is sound, only dl's work was not" + ); + assert_eq!( + parts.provision.occasions(), + [PassOccasion::TopUp], + "the container never stopped, so the pass is a top-up and not an after-up" + ); + assert!( + !scene + .devpod_commands() + .iter() + .any(|argv| argv.first().map(String::as_str) == Some("up")), + "a sound container is not rebuilt to finish dl's own pass: {:?}", + scene.devpod_commands() + ); + assert!( + parts.said.contains(&LaunchNotice::SetupPassNeverFinished { + workspace_id: "myws".to_owned() + }), + "the extra round trip is explained: {:?}", + parts.said + ); + } + + /// The other side of it, and the one that keeps the fast path fast: a workspace + /// with no interrupted pass behind it still attaches in one round trip. + /// + /// Without this, "did the last pass finish" could be answered by provisioning on + /// every warm attach -- which would pass the test above and put a `devpod ssh` + /// on the hottest path dl has, for every workspace dl never provisioned. + #[test] + fn a_warm_attach_with_no_interrupted_pass_behind_it_still_runs_none() { + let scene = Scene::new() + .with_running("myws") + .with_create_completed("myws"); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + let mut cold = NeverCold; + let mut launch = Launch::new( + &mut parts.context, + &mut parts.refresh, + &mut cold, + &parts.provision, + &scene.host, + &mut parts.chatter, + &mut parts.said, + ); + + let _ = launch.run("myws", &LaunchVerb::Attach { command: None }, None); + + drop(launch); + assert_eq!(parts.provision.occasions(), [], "no pass at all"); + } + /// The other side of the same check: a create devpod *did* finish still takes /// the fast path. Without this, "is it set up" could be answered by refusing /// every fast attach, which would pass the test above and cost every warm diff --git a/rust/devlaunch-core/src/flows/provision.rs b/rust/devlaunch-core/src/flows/provision.rs index a2e83e60..8de31ea9 100644 --- a/rust/devlaunch-core/src/flows/provision.rs +++ b/rust/devlaunch-core/src/flows/provision.rs @@ -2234,6 +2234,31 @@ pub enum Provisioning { } impl Provisioning { + /// Whether a trip got through to the container, whatever it then found. + /// + /// What decides whether the in-flight record [`provision`] opens is closed + /// again. Every arm but one is a pass that reached the container and did + /// something there, or established that there was nothing to do; only + /// [`Self::TripRefused`] is the OS declining to make the trip, which leaves the + /// container exactly as it was and this host knowing nothing new about it. + /// + /// Exhaustive rather than `matches!` on the one arm, for + /// [`Self::tools_present`]'s reason: a later arm has to be classified by + /// somebody who is looking at it, and an `_` would classify it as *reached*, + /// which is the direction that loses a workspace. + pub(crate) fn reached_the_container(&self) -> bool { + match self { + Self::AlreadyProvisioned + | Self::CachedProvisioned + | Self::Lent + | Self::ShimKept + | Self::Installed + | Self::InstallRefused { .. } + | Self::Disabled => true, + Self::TripRefused { .. } => false, + } + } + /// Whether the tools are now there — the bool Python returns. Only this /// module's tests read it; the binary matches the arms directly. #[cfg_attr(not(test), allow(dead_code))] @@ -2354,6 +2379,12 @@ fn provision( // launch after upgrading, which is the trade `switches` made too. && verdicts.has_claude_memo(workspace) { + // Including a record some killed launch left open, which is the one route + // out of here that reaches neither place a pass is normally closed. Closing + // it is the right answer and not only the terminating one: the marker being + // trusted means a pass probed *this* container provisioned, so whatever the + // interrupted pass had left to do, there is nothing left to do now. + verdicts.end_pass(workspace); // No probe ran here, and the session that follows still has to decide // whether to forward the host's login, so the answer comes from what the // last pass remembered. @@ -2365,6 +2396,58 @@ fn provision( // Before the pass, not after it: see [`VerdictCache::observe`]. let observed = verdicts.and_then(|verdicts| verdicts.observe(workspace)); + // And the record that this pass is running, opened here for the same "before" + // and a second reason of its own. `dl`'s signal handler `_exit`s without + // unwinding, and a container whose pass was cut short is one devpod calls + // Running and finished creating -- so every later launch fast-attaches to it + // and the pass never runs again. Nothing about the container says otherwise, + // which is why the host writes it down first. + if let (Some(verdicts), Some(observed)) = (verdicts, observed) { + verdicts.begin_pass(workspace, observed); + } + + let outcome = run_the_pass( + runner, workspace, switches, title, host, verdicts, observed, events, + ); + + // Closed out here, from what the pass answered, and that placement is the whole + // of what makes the record worth keeping. It used to close beside the memo, the + // moment the probe trip returned -- which covers ~1.7s of a pass that runs for + // minutes, since the lend streams the host's binaries and the install fetches a + // ~300MB `claude`. A Ctrl-C in the part that was left uncovered is the ordinary + // one, so the record was open for the window nobody interrupts and shut for the + // window everybody does. + // + // A trip that never got through is the one outcome that leaves it standing. + // Nothing was learned about the container and nothing was done to it, which is + // exactly the state the record describes; closing there would erase the only + // evidence a later launch could act on, and it is reachable from the recovery + // pass itself. + if let (Some(verdicts), Ok(pass)) = (verdicts, &outcome) + && pass.provisioning.reached_the_container() + { + verdicts.end_pass(workspace); + } + outcome +} + +/// The pass: the probe trip, then the lend, then the install. +/// +/// Split from [`provision`] only so the in-flight record can be opened before all +/// three and closed after all three. Folding it back means closing the record at +/// each of the six places this returns, and the two that must *not* close it are +/// the two easiest to miss. +#[allow(clippy::too_many_arguments)] +fn run_the_pass( + runner: &dyn Runner, + workspace: &str, + switches: Switches, + title: Option<&str>, + host: Option<&HostLayout>, + verdicts: Option<&VerdictCache>, + observed: Option, + events: &mut dyn Notices, +) -> Result { let host_home = host.and_then(|layout| layout.home.to_str()); let found = match setup_pass(runner, workspace, switches, title, host_home, events) { @@ -2656,6 +2739,12 @@ mod zellij_contract; #[cfg(test)] mod herdr_tab_contract; +/// The file name `docs/workspace-tools.md` publishes for a pass that was killed, +/// held against the path the verdict cache writes. Beside the other three for +/// `zellij_contract`'s reason, and reading the same section splitter. +#[cfg(test)] +mod interrupted_pass_contract; + #[cfg(test)] mod tests { //! # What this pins, and how @@ -3049,21 +3138,39 @@ fi /// registry, so a test driving it without the guard writes into whatever document /// a concurrent measured test installed. In the fixture rather than per test, so /// no test has to remember. - #[derive(Debug)] struct Trips { answers: Vec, stdout: String, seen: Mutex>, + /// See [`Trips::watching`]. `None` for the tests that only count trips. + #[allow(clippy::type_complexity)] + during: Option bool + Send + Sync>>, + saw: Mutex>, /// See [`timing::exclusive`]. Last field, so it is dropped last. _serialized: timing::Exclusive, } + impl std::fmt::Debug for Trips { + /// Hand-written because the watcher is a closure. What a failure message + /// wants of this value is what it recorded, which is the rest of it. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Trips") + .field("answers", &self.answers) + .field("stdout", &self.stdout) + .field("seen", &self.seen) + .field("saw", &self.saw) + .finish_non_exhaustive() + } + } + impl Trips { fn new(exits: &[i32]) -> Self { Self { answers: exits.iter().copied().map(Answer::Exited).collect(), stdout: String::new(), seen: Mutex::new(Vec::new()), + during: None, + saw: Mutex::new(Vec::new()), _serialized: timing::exclusive(), } } @@ -3073,6 +3180,8 @@ fi answers: answers.to_vec(), stdout: String::new(), seen: Mutex::new(Vec::new()), + during: None, + saw: Mutex::new(Vec::new()), _serialized: timing::exclusive(), } } @@ -3083,6 +3192,24 @@ fi self } + /// Run `during` at the moment each trip is in flight, keeping what it + /// answered. + /// + /// The one thing a fake runner can see that a recording of the argv cannot: + /// what was true on the host *while* devpod was running. A `dl` killed + /// mid-pass is killed exactly here, and nothing about it is observable after + /// the call returns. + #[must_use] + fn watching(mut self, during: impl Fn() -> bool + Send + Sync + 'static) -> Self { + self.during = Some(Box::new(during)); + self + } + + /// What [`Self::watching`] saw, one answer per trip, in order. + fn watched(&self) -> Vec { + self.saw.lock().expect("the recording").clone() + } + fn trips(&self) -> Vec { self.seen.lock().expect("the recording").clone() } @@ -3121,7 +3248,12 @@ fi StdinPlan::Inherit | StdinPlan::Null => None, }, }); - self.answers[(seen.len() - 1).min(self.answers.len() - 1)] + let answer = self.answers[(seen.len() - 1).min(self.answers.len() - 1)]; + drop(seen); + if let Some(during) = &self.during { + self.saw.lock().expect("the recording").push(during()); + } + answer } fn outcome(&self, answer: Answer) -> Outcome { @@ -6525,6 +6657,140 @@ fi ) } + #[test] + fn the_pass_is_open_while_the_trip_is_and_closed_once_it_answers() { + // The record a killed `dl` leaves. `dl`'s signal handler `_exit`s without + // unwinding, so the only moment at which the host can be told "a pass is + // running" is before the trip, and the only proof it was told is what is + // standing *during* the trip -- which is the seconds-long window a Ctrl-C + // actually lands in. + let remembering = Remembering::new(); + let verdicts = remembering.verdicts(); + let watching = remembering.verdicts(); + let runner = Trips::new(&[0]) + .reporting(REPORT_PROVISIONED) + .watching(move || watching.pass_never_finished("myws")); + + pass_of(&runner, PassOccasion::AfterUp, &verdicts).expect("devpod answered"); + + assert_eq!( + runner.watched(), + [true], + "the trip ran with no record of the pass carrying it" + ); + assert!( + !verdicts.pass_never_finished("myws"), + "the pass answered, so there is nothing left to finish" + ); + } + + #[test] + fn the_pass_is_open_for_the_install_trip_and_not_only_the_probe() { + // The trip the record is *for*. A probe that answers absent is followed by + // the install, which fetches `gh` and `claude`: minutes against the probe's + // ~1.7s, and so the window nearly every Ctrl-C lands in. Closing the record + // when the probe answered left that window exactly as it was before any of + // this existed -- the container comes up, the install is killed, and every + // launch afterwards fast-attaches into a container with no tools. + let remembering = Remembering::new(); + let verdicts = remembering.verdicts(); + let watching = remembering.verdicts(); + let runner = Trips::new(&[0]) + .reporting(REPORT_ABSENT) + .watching(move || watching.pass_never_finished("myws")); + + let outcome = pass_of(&runner, PassOccasion::AfterUp, &verdicts); + + assert_eq!(outcome, Ok(Provisioning::Installed)); + assert_eq!( + runner.watched(), + [true, true], + "the install ran with no record of the pass carrying it" + ); + assert!( + !verdicts.pass_never_finished("myws"), + "and the record closes once the whole pass has answered" + ); + } + + #[test] + fn a_trip_that_never_got_through_leaves_the_pass_open() { + // Replaces a test that asserted the opposite, and the opposite was wrong. + // `TripRefused` is the OS declining to make the trip at all, so nothing was + // learned about the container and nothing was done to it -- which is the + // one state the record exists to describe. Closing it there erases the only + // evidence a later launch could act on, and it is reachable from the + // recovery pass itself: one refused trip and the workspace can never be + // recovered by anything. + // + // Not the "re-attempts forever" shape `verdict_cache` refuses for + // `ShimKept`. That is a trip that *ran* and would go on failing; this is a + // trip that did not happen, on a host that is broken in a way the pass + // reports every time. + let remembering = Remembering::new(); + let verdicts = remembering.verdicts(); + let runner = Trips::answering(&[Answer::Blocked]); + + let outcome = pass_of(&runner, PassOccasion::AfterUp, &verdicts); + + assert!(matches!(outcome, Ok(Provisioning::TripRefused { .. }))); + assert!(verdicts.pass_never_finished("myws")); + } + + #[test] + fn a_top_up_that_makes_no_trip_opens_no_pass() { + // The cached arm returns before the trip, so there is no pass for a Ctrl-C + // to interrupt -- and a record opened for it would never be closed. + let remembering = Remembering::new(); + let verdicts = remembering.verdicts(); + let first = Trips::new(&[0]).reporting(REPORT_PROVISIONED); + pass_of(&first, PassOccasion::TopUp, &verdicts).expect("devpod answered"); + + let again = Trips::new(&[0]).reporting(REPORT_PROVISIONED); + assert_eq!( + pass_of(&again, PassOccasion::TopUp, &verdicts), + Ok(Provisioning::CachedProvisioned) + ); + + assert!(!verdicts.pass_never_finished("myws")); + } + + #[test] + fn a_cached_answer_closes_a_pass_that_was_left_open() { + // The arm that returns before the trip has to close the record too, or the + // one launch that could act on it would be the one that cannot: a top-up + // whose cache fully answers makes no trip, so it would reach neither of the + // two places the record is closed, and the launch after it would ask again, + // and so would every launch after that. + // + // Closing it is also the right answer rather than only the terminating one. + // The marker being trusted means a pass probed this same container + // provisioned, so whatever the interrupted pass had left to do, there is + // nothing left to do now. + let remembering = Remembering::new(); + let verdicts = remembering.verdicts(); + let first = Trips::new(&[0]).reporting(REPORT_PROVISIONED); + pass_of(&first, PassOccasion::TopUp, &verdicts).expect("devpod answered"); + // A pass opened over the same container and never closed, which is the state + // a killed `dl` leaves. + let observed = verdicts.observe("myws").expect("an anchor"); + verdicts.begin_pass("myws", observed); + assert!(verdicts.pass_never_finished("myws")); + + let again = Trips::new(&[0]).reporting(REPORT_PROVISIONED); + assert_eq!( + pass_of(&again, PassOccasion::TopUp, &verdicts), + Ok(Provisioning::CachedProvisioned) + ); + + assert_eq!(again.count(), 0, "still no round trip"); + assert!( + !verdicts.pass_never_finished("myws"), + "the cached answer left the record open, so every launch after this \ + one asks again and none of them can ever answer" + ); + } + #[test] fn a_top_up_with_a_trusted_verdict_makes_no_trip() { // The saving, stated as the only thing that can be measured about it: zero diff --git a/rust/devlaunch-core/src/flows/provision/interrupted_pass_contract.rs b/rust/devlaunch-core/src/flows/provision/interrupted_pass_contract.rs new file mode 100644 index 00000000..c12ea7bd --- /dev/null +++ b/rust/devlaunch-core/src/flows/provision/interrupted_pass_contract.rs @@ -0,0 +1,81 @@ +//! The file name `docs/workspace-tools.md` publishes for an interrupted pass, held +//! against the path the cache actually writes. +//! +//! Beside `lending_contract` and `zellij_contract` for their reason, and reading the +//! same section splitter. What it guards is narrower than either: the page tells a +//! reader that a killed launch leaves `.pass` beside the marker, and a +//! reader who believes that goes looking for the file -- to see whether a workspace +//! is stuck, or to clear one by hand. A page naming a file that is not there is +//! worse than a page that names none. +//! +//! Two facts, because the sentence makes two claims: what the file is called, and +//! that it sits beside the marker rather than somewhere of its own. Both are read +//! out of [`VerdictCache`] rather than out of a constant, so a rename that moves the +//! path moves the assertion with it. +//! +//! Everything else in the section is explanation and gets no assertions, for the +//! reason `lending_contract` gives about the trip-by-trip narrative. + +use super::lending_contract::{CONTRACT_DOC, contract_doc, section}; +use super::verdict_cache::VerdictCache; + +/// The section carrying what this file guards, matched on its heading for +/// `lending_contract`'s reason. +const HEADING: &str = "### A pass that was interrupted"; + +/// The two paths this workspace's records live at, as the cache spells them. +fn recorded(workspace_id: &str) -> (String, String) { + let dir = tempfile::tempdir().expect("a scratch cache directory"); + let verdicts = VerdictCache::under(dir.path(), None); + let named = |path: std::path::PathBuf| { + path.strip_prefix(dir.path()) + .expect("a path under the scratch cache") + .display() + .to_string() + }; + ( + named(verdicts.in_flight(workspace_id)), + named(verdicts.marker(workspace_id)), + ) +} + +#[test] +fn the_page_names_the_file_a_killed_launch_leaves() { + let (in_flight, _) = recorded("myws"); + let extension = in_flight + .rsplit_once('.') + .expect("the record carries an extension") + .1 + .to_owned(); + + assert!( + section(&contract_doc(), HEADING).contains(&format!(".{extension}")), + "{CONTRACT_DOC} no longer names the record an interrupted pass leaves, which is \ + `.{extension}`; a reader told to look for it would find nothing" + ); +} + +#[test] +fn the_record_really_is_beside_the_marker() { + // "beside the marker" is the page's own word for where to look, and it is the + // half a reader uses to find the directory at all: the marker's own path is + // published one subsection up, and this sentence hangs off it rather than + // repeating it. + let (in_flight, marker) = recorded("myws"); + let directory = |path: &str| { + path.rsplit_once('/') + .map(|(dir, _)| dir.to_owned()) + .unwrap_or_default() + }; + + assert_eq!( + directory(&in_flight), + directory(&marker), + "{CONTRACT_DOC} says the record sits beside the marker and it no longer does" + ); + assert!( + section(&contract_doc(), HEADING).contains("beside the marker"), + "{CONTRACT_DOC} no longer says where the record is, so nothing tells a reader \ + which directory to look in" + ); +} diff --git a/rust/devlaunch-core/src/flows/provision/verdict_cache.rs b/rust/devlaunch-core/src/flows/provision/verdict_cache.rs index f45eedc1..0d253935 100644 --- a/rust/devlaunch-core/src/flows/provision/verdict_cache.rs +++ b/rust/devlaunch-core/src/flows/provision/verdict_cache.rs @@ -103,7 +103,7 @@ impl VerdictCache { /// file's name: devpod itself uses the id as a directory name under its own /// contexts, so an id that could not be a path component is one no workspace /// this could be asked about has. - fn marker(&self, workspace_id: &str) -> PathBuf { + pub(super) fn marker(&self, workspace_id: &str) -> PathBuf { self.markers_dir.join(format!("{workspace_id}.json")) } @@ -258,6 +258,85 @@ impl VerdictCache { }; write_atomically(&self.marker(workspace_id), &text); } + + /// Where the in-flight record for this workspace lives. + pub(super) fn in_flight(&self, workspace_id: &str) -> PathBuf { + self.marker(workspace_id).with_extension("pass") + } + + /// Note that a setup pass over the container [`Self::observe`] identified is + /// about to run, and has not finished. + /// + /// Written before the pass's *first* trip and removed by [`Self::end_pass`] + /// after its last, so the file surviving a launch is the one thing on this host + /// that says the pass was cut short. A pass is up to three trips and the probe + /// is the short one, so a record that closed when the probe answered would be + /// open for ~1.7s of a pass that runs for minutes. `dl`'s signal handler `_exit`s without unwinding + /// (`devlaunch_runner::interrupt`), which is exactly why the evidence has to be + /// a file left standing rather than one written on the way out: nothing runs on + /// the way out. + /// + /// Anchored like everything else here, and against the same failure: a record + /// left by an interrupted pass over a container that has since been rebuilt + /// describes a container that is not standing, and reading it would send every + /// later launch on a round trip it does not owe. + /// + /// Silent about every way of not working, as [`Self::record`] is. A record that + /// could not be written costs one interrupted launch its recovery and nothing + /// else -- which is today's behaviour, exactly. + pub(crate) fn begin_pass(&self, workspace_id: &str, observed: Observed) { + let Observed(result_mtime) = observed; + let Ok(text) = serde_json::to_string(&InFlight { result_mtime }) else { + return; + }; + write_atomically(&self.in_flight(workspace_id), &text); + } + + /// The pass [`Self::begin_pass`] opened has finished. + /// + /// Called for a pass that got through to the container, however it turned out: + /// an install that ran and failed is a pass that happened, and re-running it on + /// every attach afterwards is the "re-attempts forever" behaviour the module + /// note refuses for [`Provisioning::ShimKept`](super::Provisioning::ShimKept). + /// A trip the OS would not make is the other thing and is not called for here -- + /// see [`Provisioning::reached_the_container`](super::Provisioning::reached_the_container), + /// which is where the two are told apart. + pub(crate) fn end_pass(&self, workspace_id: &str) { + let _ = std::fs::remove_file(self.in_flight(workspace_id)); + } + + /// Whether a setup pass over the container standing now began and never + /// finished. + /// + /// **Positive evidence, and it has to be.** The opposite reading -- "no record + /// of a finished pass, so run one" -- is true of every workspace `dl` never + /// provisioned: one created by VS Code, by a hand-typed `devpod up`, or by a + /// build older than this file. Those must keep attaching in one round trip, so + /// the question asked here is the narrow one, and every doubt answers *no*: no + /// record, a record that will not parse, or one whose container is not the one + /// standing. + pub(crate) fn pass_never_finished(&self, workspace_id: &str) -> bool { + let Some(text) = std::fs::read_to_string(self.in_flight(workspace_id)).ok() else { + return false; + }; + let Ok(in_flight) = serde_json::from_str::(&text) else { + return false; + }; + let Some(result) = sole_workspace_result(self.devpod_home.as_ref(), workspace_id) else { + return false; + }; + Stamp::of(&result) == Some(in_flight.result_mtime) + } +} + +/// What one in-flight record says. +/// +/// One field, and a struct rather than the bare [`Stamp`] it wraps, for +/// [`Marker`]'s reason: the check is the parse, so a file a later build writes with +/// a second field it needs is not silently read as this one. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] +struct InFlight { + result_mtime: Stamp, } /// The container a pass is about, as [`VerdictCache::observe`] read it. @@ -477,6 +556,13 @@ mod tests { (cache, home, verdicts) } + /// Open a pass over the container standing now, as [`provision`] does before + /// its trip -- and, unlike [`provision`], never close it. + fn began(verdicts: &VerdictCache, workspace_id: &str) { + let observed = verdicts.observe(workspace_id).expect("an anchor"); + verdicts.begin_pass(workspace_id, observed); + } + /// Remember `seen` against the container standing now, as [`provision`] does. fn remembered_under(verdicts: &VerdictCache, workspace_id: &str, seen: Option) { let observed = verdicts.observe(workspace_id).expect("an anchor"); @@ -873,6 +959,93 @@ mod tests { assert!(!verdicts.trusted("other", Switches::INSTALLING)); } + #[test] + fn a_pass_that_began_and_never_ended_is_still_unfinished() { + // The whole of what this file adds. A `dl` killed between `devpod up` and + // the end of its setup pass leaves a container devpod calls Running and + // finished creating, so every later launch fast-attaches to it and the pass + // never runs again. Nothing else on the host can tell that apart from a + // container whose pass ran to completion, because the cache records only + // success and an absent record is what a workspace dl never touched has too. + let (_cache, _home, verdicts) = anchored(); + + began(&verdicts, "ws"); + + assert!(verdicts.pass_never_finished("ws")); + } + + #[test] + fn an_interrupted_pass_over_a_container_since_rebuilt_asks_for_nothing() { + // The anchor earns its keep here as much as anywhere. A `--recreate` after + // the interruption is a whole new container with a pass of its own, and a + // record left by the old one would put a round trip on every attach from + // then on -- for a pass that already ran, over a container that is gone. + let (_cache, home, verdicts) = anchored(); + began(&verdicts, "ws"); + assert!(verdicts.pass_never_finished("ws")); + + rewritten(&result_in(&home, "ws"), Duration::from_secs(30)); + + assert!(!verdicts.pass_never_finished("ws")); + } + + #[test] + fn nothing_unreadable_asks_for_a_pass_either() { + // Every doubt reads as "no interrupted pass", which is the direction that + // costs nothing: a workspace dl never provisioned has no record at all, and + // it must go on attaching in one round trip. + let (cache, _home, verdicts) = anchored(); + assert!(!verdicts.pass_never_finished("ws"), "no record"); + + let path = cache.path().join(MARKERS_DIR).join("ws.pass"); + std::fs::create_dir_all(path.parent().expect("a parent")).expect("the directory"); + for text in ["", "{", "{}", "null", r#"{"result_mtime": "yesterday"}"#] { + std::fs::write(&path, text).expect("the record"); + assert!(!verdicts.pass_never_finished("ws"), "{text:?}"); + } + } + + #[test] + fn a_host_that_cannot_find_the_container_asks_for_nothing() { + // No `workspace_result.json` to compare against is a doubt like any other. + // It is also what a scratch `XDG_CACHE_HOME` and a machine with no devpod + // home both look like, and neither is a launch that owes a round trip. + let unfinished = devpod_home_with(&[("default", "ws", None)]); + let cache = cache(); + for home in [Some(DevpodHome::at(unfinished.path())), None] { + let verdicts = VerdictCache::under(cache.path(), home.clone()); + std::fs::create_dir_all(cache.path().join(MARKERS_DIR)).expect("the directory"); + std::fs::write( + cache.path().join(MARKERS_DIR).join("ws.pass"), + r#"{"result_mtime":{"secs":1,"nanos":0}}"#, + ) + .expect("the record"); + assert!(!verdicts.pass_never_finished("ws"), "{home:?}"); + } + } + + #[test] + fn one_workspaces_interrupted_pass_says_nothing_about_another() { + let home = devpod_home_with(&[("default", "ws", Some(())), ("default", "other", Some(()))]); + let cache = cache(); + let verdicts = VerdictCache::under(cache.path(), Some(DevpodHome::at(home.path()))); + + began(&verdicts, "ws"); + + assert!(verdicts.pass_never_finished("ws")); + assert!(!verdicts.pass_never_finished("other")); + } + + #[test] + fn a_pass_that_ended_left_nothing_to_finish() { + let (_cache, _home, verdicts) = anchored(); + + began(&verdicts, "ws"); + verdicts.end_pass("ws"); + + assert!(!verdicts.pass_never_finished("ws")); + } + #[test] fn a_stamp_is_the_mtime_it_was_read_from() { // The round trip the equality check depends on: what `Stamp::of` reads back diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index a28e6a91..23994d74 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -2809,6 +2809,11 @@ pub(crate) fn launch_notice(notice: &LaunchNotice) -> Option { for its create, so attaching to it would land as root in a container whose setup \ did not run. Bringing it up instead." ), + LaunchNotice::SetupPassNeverFinished { workspace_id } => format!( + "Workspace {workspace_id}: the last launch's setup pass did not finish, so its \ + tools, hostname and shell config may be missing. Running it again before \ + attaching." + ), LaunchNotice::AlreadyRunning { workspace_id } => { format!("Workspace {workspace_id} is already running.") } diff --git a/rust/dl/tests/resumed_setup.rs b/rust/dl/tests/resumed_setup.rs new file mode 100644 index 00000000..04c6551e --- /dev/null +++ b/rust/dl/tests/resumed_setup.rs @@ -0,0 +1,316 @@ +//! A launch cut short *after* `devpod up` and *before* dl's setup pass finished, +//! and what the next launch does about it. +//! +//! The sibling of `dl/tests/interrupt.rs`, and the moment just after the one that +//! file judges. There the `devpod up` is still running, so the next launch runs it +//! again and everything lands. Here the `up` has already finished: devpod has +//! written its `workspace_result.json`, the container is up, and what the Ctrl-C +//! interrupts is the `devpod ssh --command` that carries dl's own stages -- the +//! hostname, the terminal title, the onboarding memo, `gh` and `claude`. +//! +//! Nothing about that container says it is unfinished. `devpod status` answers +//! `Running` and devpod's create record is complete, which is what the fast-attach +//! arm asks, so every later `dl ` attaches straight into a container whose +//! setup never ran -- forever, and identically, so the workspace is not broken in a +//! way anyone can see. It is the "sometimes it works" the report is about: whether +//! you get a whole workspace depends on which second the Ctrl-C landed in. +//! +//! So a run that opens a pass writes down that it did, and a run that finds that +//! record still standing over the container standing now finishes the pass before +//! it hands over a shell. +//! +//! Linux-only, like the sibling suite, and it spells devpod's own paths on purpose: +//! `devlaunch-core/tests/devpod_layout.rs` scopes its one-spelling rule to that +//! crate's `src` precisely so an end-to-end test can check that what dl wrote +//! landed where devpod will look for it, without routing the check through the code +//! under test. + +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::time::{Duration, Instant}; + +use devlaunch_test_support::KeepingCoverage; + +fn repo_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .canonicalize() + .expect("the repository root") +} + +/// The cold world `dl/tests/launch.rs` builds, with a `devpod` that keeps devpod's +/// *records* as well as its state. +/// +/// The scenario's fake devpod answers `status`, `list` and `up` out of one JSON +/// file and writes nothing under `DEVPOD_HOME`. Real devpod writes +/// `workspace.json` on the way in and `workspace_result.json` on the way out of a +/// completed `up`, and both halves of this launch read them: the fast-attach arm +/// asks whether the create finished, and the host's memory of a pass is anchored to +/// the result file's mtime so that a rebuilt container is never described by an +/// older container's record. A fake that skipped them would make every question +/// here unanswerable and the test vacuous. +struct World { + root: PathBuf, + _scratch: tempfile::TempDir, +} + +impl World { + fn cold() -> Self { + let scratch = tempfile::Builder::new() + .prefix("dlresume") + .tempdir_in("/tmp") + .expect("a scratch directory under /tmp"); + let root = scratch.path().to_path_buf(); + let built = Command::new("python3") + .arg(Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/launch_scenario.py")) + .arg(&root) + .arg(repo_root().join("test/fixtures/devpod_shim.py")) + .output() + .expect("python3 is installed"); + assert!( + built.status.success(), + "launch_scenario.py failed: {}", + String::from_utf8_lossy(&built.stderr) + ); + + let devpod = root.join("bin/devpod"); + let original = std::fs::read_to_string(&devpod).expect("the scenario's devpod"); + let delegate = original + .lines() + .find(|line| line.starts_with("exec ")) + .expect("the delegate exec line"); + // Without the `exec`, so the `up` arm gets to keep going afterwards. + let run = delegate.strip_prefix("exec ").expect("an exec line"); + // `$DL_BLOCK_PASS` is what makes one run the interrupted one: the setup pass + // is the `ssh --command` trip, and blocking it holds `dl` at exactly the + // moment a Ctrl-C during provisioning arrives. The session `ssh` carries no + // `--command`, so it is never the one that blocks. + let script = format!( + "#!/bin/sh\n\ + if [ \"$1\" = \"up\" ]; then\n\ + \x20 {run} \"$@\" || exit $?\n\ + \x20 ws=$2; prev=\n\ + \x20 for a in \"$@\"; do\n\ + \x20 if [ \"$prev\" = \"--id\" ]; then ws=$a; fi\n\ + \x20 prev=$a\n\ + \x20 done\n\ + \x20 d=\"$DEVPOD_HOME/contexts/default/workspaces/$ws\"\n\ + \x20 mkdir -p \"$d\"\n\ + \x20 printf '{{}}' > \"$d/workspace.json\"\n\ + \x20 printf '{{}}' > \"$d/workspace_result.json\"\n\ + \x20 exit 0\n\ + fi\n\ + if [ \"$1\" = \"ssh\" ] && [ -n \"$DL_BLOCK_PASS\" ]; then\n\ + \x20 case \" $* \" in\n\ + \x20 *\" --command \"*)\n\ + \x20 echo \"$$\" > \"$DL_PASS_PID\"\n\ + \x20 : > \"$DL_PASS_STARTED\"\n\ + \x20 exec sleep 300 ;;\n\ + \x20 esac\n\ + fi\n\ + {delegate}\n" + ); + std::fs::write(&devpod, script).expect("rewrite devpod"); + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&devpod, std::fs::Permissions::from_mode(0o755)) + .expect("keep devpod executable"); + + World { + root, + _scratch: scratch, + } + } + + fn path(&self, relative: &str) -> PathBuf { + self.root.join(relative) + } + + /// A `dl blooop/devlaunch@cold` over this world. + /// + /// `DEVLAUNCH_NO_TOOLS` so the pass carries its stages and stops there: the + /// stages are not tools work, so the trip this test counts still happens, and + /// the lend and the network install that would follow it do not. What is being + /// counted is whether a pass ran at all. + fn dl(&self) -> Command { + let root = self.root.display().to_string(); + let mut command = Command::new(env!("CARGO_BIN_EXE_dl")); + command + .arg("blooop/devlaunch@cold") + .env_clear() + .keeping_coverage() + .env("PATH", format!("{root}/bin:/usr/bin:/bin")) + .env("HOME", format!("{root}/home")) + .env("XDG_CACHE_HOME", format!("{root}/cache")) + .env("XDG_CONFIG_HOME", format!("{root}/config")) + .env("DEVPOD_HOME", format!("{root}/devpod")) + .env("DEVPOD_SHIM_STATE", format!("{root}/shim-state.json")) + .env("DEVPOD_SHIM_LOG", format!("{root}/shim-log.jsonl")) + .env("DEVPOD_SHIM_CONFIG", format!("{root}/shim-config.json")) + .env("DEVLAUNCH_NO_GH_TOKEN", "1") + .env("DEVLAUNCH_NO_TOOLS", "1") + .env( + "DL_PASS_STARTED", + self.path("pass.started").display().to_string(), + ) + .env("DL_PASS_PID", self.path("pass.pid").display().to_string()) + .env("GIT_SSH_COMMAND", "false") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null"); + command + } + + /// Run `dl` to completion and answer how many setup passes it made. + fn launched(&self) -> Passes { + self.truncate_log(); + let done = self.dl().output().expect("the dl binary runs"); + assert!( + done.status.success(), + "dl failed ({:?}): {}", + done.status.code(), + String::from_utf8_lossy(&done.stderr) + ); + Passes { + setup_trips: self.setup_trips(), + said: String::from_utf8_lossy(&done.stderr).into_owned(), + } + } + + /// Run `dl` up to the setup pass and SIGTERM it there. + /// + /// SIGTERM rather than SIGINT for one reason that is the harness's and not the + /// behaviour's: a job this test backgrounds inherits an ignored SIGINT under + /// POSIX job control, which `dl` honours on purpose (`dl/tests/interrupt.rs` + /// pins that asymmetry). Both signals reach the same drain and the same + /// `_exit`, which is the part that matters here: nothing unwinds, so nothing + /// tidies up after the pass that was running. + fn interrupted_mid_pass(&self) { + let started = self.path("pass.started"); + let mut child = self + .dl() + .env("DL_BLOCK_PASS", "1") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .expect("the dl binary runs"); + assert!( + wait_for(|| started.exists()), + "the setup pass never started, so the interrupt would prove nothing" + ); + assert!( + Command::new("kill") + .args(["-TERM", &child.id().to_string()]) + .status() + .expect("kill is installed") + .success(), + "sending SIGTERM to dl" + ); + let status = child.wait().expect("dl exits"); + assert_eq!( + status.code(), + Some(143), + "dl's own interrupted ending, 128 + SIGTERM" + ); + self.reap_the_blocked_trip(); + } + + /// Kill the trip `dl` left blocking, because `dl` does not. + /// + /// Not the behaviour under test, and not a defect this test is asserting the + /// absence of. The pass's `devpod ssh --command` stays in `dl`'s own process + /// group on purpose -- `devlaunch_runner`'s `passthrough` explains why only + /// `devpod up` leads a group of its own -- so a terminal Ctrl-C reaches it from + /// the kernel and fells it. What does not reach it is the `kill
` this + /// harness sends, which is aimed at one pid. Left alone the fake trip would + /// outlive the suite, and a test that leaves processes behind is one that makes + /// the next one flaky. + fn reap_the_blocked_trip(&self) { + let Ok(pid) = std::fs::read_to_string(self.path("pass.pid")) else { + return; + }; + let _ = Command::new("kill").args(["-KILL", pid.trim()]).status(); + } + + fn truncate_log(&self) { + std::fs::write(self.path("shim-log.jsonl"), "").expect("an empty log"); + } + + /// How many `devpod ssh --command` trips the log holds -- the setup pass, and + /// the only devpod call a launch makes that is dl provisioning the container. + fn setup_trips(&self) -> usize { + std::fs::read_to_string(self.path("shim-log.jsonl")) + .unwrap_or_default() + .lines() + .filter(|line| line.contains("\"ssh\"") && line.contains("\"--command\"")) + .count() + } + + /// The in-flight records dl's cache holds, by file name. + fn open_passes(&self) -> Vec { + let Ok(entries) = std::fs::read_dir(self.path("cache/devlaunch/tool-verdicts")) else { + return Vec::new(); + }; + let mut found: Vec = entries + .flatten() + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.ends_with(".pass")) + .collect(); + found.sort(); + found + } +} + +/// What one completed `dl` did. +#[derive(Debug)] +struct Passes { + setup_trips: usize, + said: String, +} + +fn wait_for(mut ready: impl FnMut() -> bool) -> bool { + let deadline = Instant::now() + Duration::from_secs(30); + while Instant::now() < deadline { + if ready() { + return true; + } + std::thread::sleep(Duration::from_millis(25)); + } + false +} + +#[test] +fn a_launch_killed_mid_pass_is_finished_by_the_next_one_and_only_the_next_one() { + let world = World::cold(); + + // One launch, killed while its setup pass is on the wire. The container it + // leaves is running and devpod calls its create complete, which is the whole + // difficulty: nothing about it is distinguishable from a workspace that is ready. + world.interrupted_mid_pass(); + assert_eq!( + world.open_passes().len(), + 1, + "the killed run left no record that its pass was running: {:?}", + world.open_passes() + ); + + // The next launch. Before this, it attached in one round trip and provisioned + // nothing -- the fast-attach arm asks devpod's create record, and devpod's + // create really did finish. + let second = world.launched(); + assert_eq!( + second.setup_trips, 1, + "the interrupted pass was not finished: {second:?}" + ); + assert!( + second.said.contains("setup pass did not finish"), + "the extra round trip went unexplained: {}", + second.said + ); + + // And the launch after that pays nothing, which is the half that keeps the fast + // path fast: the record is closed by the pass that finished, not left standing + // for every attach from then on. + let third = world.launched(); + assert_eq!(third.setup_trips, 0, "the fast attach stopped being fast"); + assert_eq!(world.open_passes(), Vec::::new()); +}