diff --git a/CHANGELOG.md b/CHANGELOG.md index ef3f64d9a6..7b1e6a8afc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. `code_required` (below). A server and a computer must be upgraded together: an older computer has no `/sign-in/code`. +### A new desktop install starts its server again + +A desktop install of v0.2.0 stopped at start with `server stopped straight away (exit status: 1)` and +`Cannot find module '../../../agent-computer/src/egress'`. The server imports its egress rules from +`agent-computer`, and the desktop app did not copy that folder when it laid out the deployment. It +does now, and a test fails if the deployment's code imports anything the desktop app does not copy. +An install laid out by an earlier app is asked to finish installing when it starts; installing +downloads the same release again with the missing folder. Settings and data are kept. + ### A new Bot is made from a sheet that asks where it runs first New Bot opens a sheet instead of the four-screen wizard. It asks where the Bot will run, with Run it diff --git a/desktop/src-tauri/src/deployment.rs b/desktop/src-tauri/src/deployment.rs index e08cc85d6f..563d8474e9 100644 --- a/desktop/src-tauri/src/deployment.rs +++ b/desktop/src-tauri/src/deployment.rs @@ -30,6 +30,10 @@ const IMAGES: &str = "container-images.json"; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct Installed { pub version: String, + /// The top-level entries this app laid out. A stamp written before this existed reads as + /// empty, which never matches, so that deployment is laid out again. + #[serde(default)] + pub layout: Vec, } /// The tarball GitHub publishes for a tag. @@ -141,6 +145,7 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> { std::fs::create_dir_all(root)?; let stamp = Installed { version: version.to_string(), + layout: layout(), }; std::fs::write( stamp_path(root), @@ -157,16 +162,31 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> { /// The image manifest is the one exception. A deployment laid down by an app that predates it has a /// stamp that matches and no manifest, and re-fetching is a better answer than an error about a /// file the person has never heard of. +/// +/// So is a stamp that laid out less than this app copies. An app that left a folder out wrote a +/// stamp that still matches its version, and Repair would otherwise keep the gap forever. pub fn needs_fetch(root: &Path, wanted: &str) -> bool { if !images_path(root).exists() { return true; } match installed(root) { - Some(found) => found.version != wanted, + // A deployment laid out by an app that copied less is missing something this one runs. + Some(found) => found.version != wanted || found.layout != layout(), None => true, } } +/// What this app lays out, in a fixed order, as recorded in the stamp. +fn layout() -> Vec { + let mut entries: Vec = REQUIRED + .iter() + .chain(ALSO_COPIED.iter()) + .map(|entry| entry.to_string()) + .collect(); + entries.sort(); + entries +} + /// Everything the three host processes and Compose need from the tree. /// /// Named rather than "the whole repository" because most of it is not needed to run: the charts, the @@ -175,8 +195,12 @@ pub fn needs_fetch(root: &Path, wanted: &str) -> bool { pub const REQUIRED: [&str; 4] = ["docker-compose.yml", "server", "app", "worker"]; /// The rest of what a deployment needs, which is not what it is checked for. -pub const ALSO_COPIED: [&str; 8] = [ +pub const ALSO_COPIED: [&str; 9] = [ "shared", + // Not a workspace, but the server imports its egress rules from `agent-computer/src` and + // `scripts/start-local-chrome-computer.ts` its browser runtime. Without it the server exits at + // start with `Cannot find module '../../../agent-computer/src/egress'`. + "agent-computer", // A root workspace the server imports as `@openbot/toolkit`. Without it, `bun install // --frozen-lockfile` stops with `Workspace not found "toolkit"`. "toolkit", @@ -531,6 +555,117 @@ mod tests { assert!(!docs, "the rest of the tree is not part of a deployment"); } + /// Relative module specifiers in one source file: `from "..."`, `import("...")` and + /// `require("...")`, with either quote. Package names are not relative and are skipped. + fn relative_imports(source: &str) -> Vec { + let mut found = Vec::new(); + for opener in [ + "from \"", + "from '", + "import(\"", + "import('", + "require(\"", + "require('", + ] { + let quote = opener.chars().last().unwrap(); + for (at, _) in source.match_indices(opener) { + let rest = &source[at + opener.len()..]; + if let Some(end) = rest.find(quote) { + let specifier = &rest[..end]; + if specifier.starts_with("./") || specifier.starts_with("../") { + found.push(specifier.to_owned()); + } + } + } + } + found + } + + fn sources_under(dir: &Path, into: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + let name = entry.file_name().to_string_lossy().into_owned(); + if path.is_dir() { + // Tests and build output run from a checkout, never from a deployment. + if !matches!( + name.as_str(), + "node_modules" | "dist" | "tests" | "test" | "__tests__" + ) { + sources_under(&path, into); + } + } else if [".ts", ".tsx", ".js", ".mjs", ".cjs"] + .iter() + .any(|suffix| name.ends_with(suffix)) + && !name.contains(".test.") + && !name.contains(".spec.") + { + into.push(path); + } + } + } + + /// `#673` made the server import `../../../agent-computer/src/egress`, and the unpacker kept + /// copying only the folders listed here, so every new Desktop install of v0.2.0 had a server + /// that exited at start with `Cannot find module`. A checkout has every folder, which is why + /// nothing caught it. This walks the real tree: whatever the unpacked code imports by a + /// relative path has to be something the unpacker keeps. + #[test] + fn everything_the_unpacked_code_imports_is_unpacked_too() { + let repo = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../..") + .canonicalize() + .unwrap(); + let kept: Vec<&str> = REQUIRED.iter().chain(ALSO_COPIED.iter()).copied().collect(); + let mut files = Vec::new(); + for top in &kept { + sources_under(&repo.join(top), &mut files); + } + assert!( + files + .iter() + .any(|file| file.starts_with(repo.join("server"))), + "the walk found no server sources, so it proves nothing" + ); + + let mut missing = Vec::new(); + for file in &files { + let Ok(source) = std::fs::read_to_string(file) else { + continue; + }; + for specifier in relative_imports(&source) { + let mut resolved = file.parent().unwrap().to_path_buf(); + for part in Path::new(&specifier).components() { + match part { + std::path::Component::ParentDir => { + resolved.pop(); + } + std::path::Component::Normal(name) => resolved.push(name), + _ => {} + } + } + let top = resolved + .strip_prefix(&repo) + .ok() + .and_then(|inside| inside.components().next()) + .map(|first| first.as_os_str().to_string_lossy().into_owned()); + if !top.as_deref().is_some_and(|top| kept.contains(&top)) { + missing.push(format!( + "{} imports {specifier}", + file.strip_prefix(&repo).unwrap_or(file).display() + )); + } + } + } + assert!( + missing.is_empty(), + "a deployment would not contain what these import:\n{}", + missing.join("\n") + ); + } + #[test] fn a_recorded_version_is_not_fetched_again() { let dir = std::env::temp_dir().join(format!("openbot-dep-same-{}", std::process::id())); @@ -544,6 +679,24 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// v0.2.0 installs were laid out by an app that did not copy `agent-computer`, and their stamp + /// matches the version they run, so Repair never fetched again and the server never started. + /// A stamp says what it laid out; one that laid out less than this app copies is fetched once. + #[test] + fn a_deployment_laid_out_by_an_app_that_copied_less_is_fetched_again() { + let dir = std::env::temp_dir().join(format!("openbot-dep-layout-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(images_path(&dir), "{}").unwrap(); + // Exactly what an earlier app wrote. + std::fs::write(stamp_path(&dir), r#"{"version":"v0.2.0"}"#).unwrap(); + assert!(needs_fetch(&dir, "v0.2.0")); + assert_eq!(installed(&dir).unwrap().version, "v0.2.0"); + + record(&dir, "v0.2.0").unwrap(); + assert!(!needs_fetch(&dir, "v0.2.0")); + std::fs::remove_dir_all(&dir).ok(); + } + #[test] fn an_interrupted_fetch_is_replaced_rather_than_trusted() { // Files present, stamp absent: what an interrupted extract leaves behind. The stamp is diff --git a/desktop/src-tauri/src/preparation.rs b/desktop/src-tauri/src/preparation.rs index f29c50b6e7..0747dae969 100644 --- a/desktop/src-tauri/src/preparation.rs +++ b/desktop/src-tauri/src/preparation.rs @@ -322,6 +322,15 @@ fn require_with( if let Some(problem) = stack::deployment_problem(root) { return Err(required(problem)); } + // A deployment laid out by an app that copied less still matches its own fingerprint. Install + // lays it out again; Start cannot. + if deployment::installed(root) + .is_some_and(|found| deployment::needs_fetch(root, &found.version)) + { + return Err(required( + "The deployment was laid out by an earlier OpenBot and is missing files this version runs.", + )); + } let bun = dependencies_ready(root)?; verify(&record.images)?; Ok(bun) @@ -449,6 +458,20 @@ fn main() { ); } + /// A v0.2.0 install prepared by an app that did not copy `agent-computer` still matched its + /// fingerprint, so Start ran a server that could not load. Start now sends it back to Install, + /// which lays the deployment out again. + #[test] + fn a_deployment_laid_out_by_an_earlier_app_requires_installation_before_start() { + let f = Fixture::new(); + // Exactly what an earlier app wrote, in place before it prepared the installation. + std::fs::write(deployment::stamp_path(&f.root), r#"{"version":"v-test"}"#).unwrap(); + f.prepared(None); + let problem = require_with(&f.root, None, |_| Ok(())) + .expect_err("a deployment missing folders this app copies cannot start"); + assert!(problem.said.contains("Finish installing")); + } + #[test] fn cached_dependencies_with_unsupported_runtime_require_installation_before_start() { let f = Fixture::new();