diff --git a/CHANGELOG.md b/CHANGELOG.md index d2a1a9a4..1cbda73b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -287,6 +287,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 that read `unverified` are measured, against a workspace provisioned to run them; the eighth says plainly which half of it was measured and which was not. +- **Two branches that derive the same workspace id are no longer quietly given + the same workspace.** A workspace id is `--`, and + only the four-character suffix makes it unique: the owner is not in the readable + half at all, `feature/auth` and `feature-auth` slug to one string, and a long ref + loses its tail to the 47-character cap. Two branches whose ids do land on the + same string share one clone directory and one devpod workspace, because devpod + names workspaces globally rather than per repository. + + What that looked like: the second one attached to the first one's container and + said nothing. You got somebody else's checkout, from a command that had no + reason to look wrong, and a later `dl rm` on either of them deleted a clone + the other still claimed. Rare, roughly one in thirty-seven thousand across ten + near-identical branches in one repository, and the kind of rare that costs a day + when it happens. + + `dl` now checks before it asks devpod anything, and refuses: + + ``` + Workspace id 'devlaunch-release-999999999999999999999911-dq8q' is already held by + 'blooop/devlaunch@release/999999999999999999999911630'. Launching + 'blooop/devlaunch@release/999999999999999999999911783' under it would put both + in one clone directory and one container, so each would open the other's + checkout and 'dl rm' on either would delete work the other still owns. + Rename one of the two branches and launch it again. + ``` + + Two spellings of one repository are not a collision: `NVIDIA/cuda-samples` and + `nvidia/cuda-samples` derive one id on purpose, and the check compares triples by + the same rule the id is derived under, so the second spelling still opens the + workspace you already have. Refs stay case-sensitive, because `Main` and `main` + really are two branches. + + The check reads the records `dl` already keeps, so it needs no round trip and + nothing new on disk, and it fails open in every direction: a record whose branch + is not a legal ref is skipped, a `metadata.json` that cannot be read means "no + collision" rather than an error, and a launch that matches its own record + attaches exactly as it did before. Reading the records on the warm attach path + takes no lock, runs no migration and writes nothing, so the attach costs what it + cost before. + + The same hole in the schema 2 to 3 migration is closed with it: two records that + derived one destination both ended up owning it, because the guard against + adopting another record's clone only knew the paths records pointed at before + the run started. + ## [0.25.0] - 2026-08-28 ### Fixed diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 0aa2b673..9d8ea757 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -112,6 +112,7 @@ pub const devlaunch_core::api::HANDOFF_VAR: &str pub const devlaunch_core::api::PREWARM_VAR: &str pub trait devlaunch_core::api::ColdMachinery<'r> pub fn devlaunch_core::api::ColdMachinery::open(&mut self) -> core::result::Result, devlaunch_core::flows::launch::ColdRefused> +pub fn devlaunch_core::api::ColdMachinery::recorded(&mut self) -> core::option::Option<&devlaunch_core::domain::metadata::MetadataStorage> pub trait devlaunch_core::api::Notices pub fn devlaunch_core::api::Notices::say(&mut self, T) pub trait devlaunch_core::api::Provision diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 93e7324d..c1bd058f 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -75,6 +75,7 @@ pub fn devlaunch_core::flows::launch::ColdPath<'r, 'e>::new(&'r dyn devlaunch_ru pub fn devlaunch_core::flows::launch::ColdPath<'r, 'e>::records(&mut self) -> core::result::Result<&mut devlaunch_core::flows::records::Records<'r>, devlaunch_core::flows::records::StartupError> impl<'r> devlaunch_core::flows::launch::ColdMachinery<'r> for devlaunch_core::flows::launch::ColdPath<'r, '_> 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> impl<'r> devlaunch_core::flows::listing::CommandContext<'r> pub fn devlaunch_core::flows::listing::CommandContext<'r>::git(&self) -> devlaunch_core::clients::git::Git<'r> pub fn devlaunch_core::flows::listing::CommandContext<'r>::new(&'r dyn devlaunch_runner::Runner) -> Self @@ -127,6 +128,7 @@ pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::provision_tools(&sel pub fn devlaunch_core::flows::launch::ToolProvisioning<'_>::remembered_claude(&self, &str) -> core::option::Option impl<'r> devlaunch_core::flows::launch::ColdMachinery<'r> for devlaunch_core::flows::launch::ColdPath<'r, '_> 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> impl devlaunch_core::notices::Notices for alloc::vec::Vec pub fn alloc::vec::Vec::say(&mut self, T) impl devlaunch_core::flows::launch::Provision for devlaunch_core::flows::launch::ToolProvisioning<'_> @@ -589,6 +591,7 @@ impl core::marker::StructuralPartialEq for devlaunch_core::domain::metadata::Sec pub struct devlaunch_core::domain::metadata::MetadataStorage impl devlaunch_core::domain::metadata::MetadataStorage pub fn devlaunch_core::domain::metadata::MetadataStorage::default_path() -> core::result::Result +pub fn devlaunch_core::domain::metadata::MetadataStorage::look(impl core::convert::Into) -> Self pub fn devlaunch_core::domain::metadata::MetadataStorage::on_metadata_lock_wait(&mut self, impl core::ops::function::Fn() + 'static) pub fn devlaunch_core::domain::metadata::MetadataStorage::open(impl core::convert::Into) -> core::result::Result<(Self, alloc::vec::Vec), devlaunch_core::domain::metadata::MetadataError> impl core::fmt::Debug for devlaunch_core::domain::metadata::MetadataStorage @@ -1249,6 +1252,14 @@ pub devlaunch_core::flows::launch::LaunchRefusal::BranchNotNamed pub devlaunch_core::flows::launch::LaunchRefusal::BranchNotNamed::error: devlaunch_core::flows::launch::BranchNotNamed pub devlaunch_core::flows::launch::LaunchRefusal::BranchNotNamed::owner: alloc::string::String pub devlaunch_core::flows::launch::LaunchRefusal::BranchNotNamed::repo: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::branch: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::owner: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::recorded_branch: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::recorded_owner: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::recorded_repo: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::repo: alloc::string::String +pub devlaunch_core::flows::launch::LaunchRefusal::IdCollision::workspace_id: alloc::string::String pub devlaunch_core::flows::launch::LaunchRefusal::NoSession(devlaunch_core::flows::launch::SessionRefused) pub devlaunch_core::flows::launch::LaunchRefusal::NotPrepared pub devlaunch_core::flows::launch::LaunchRefusal::NotPrepared::branch: alloc::string::String @@ -1420,6 +1431,7 @@ pub fn devlaunch_core::flows::launch::ColdPath<'r, 'e>::new(&'r dyn devlaunch_ru pub fn devlaunch_core::flows::launch::ColdPath<'r, 'e>::records(&mut self) -> core::result::Result<&mut devlaunch_core::flows::records::Records<'r>, devlaunch_core::flows::records::StartupError> impl<'r> devlaunch_core::flows::launch::ColdMachinery<'r> for devlaunch_core::flows::launch::ColdPath<'r, '_> 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 struct devlaunch_core::flows::launch::Host impl devlaunch_core::flows::launch::Host pub fn devlaunch_core::flows::launch::Host::from_process(impl core::convert::Into) -> Self @@ -1456,8 +1468,10 @@ pub fn devlaunch_core::flows::launch::UnquotableCommand::fmt(&self, &mut core::f impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::UnquotableCommand pub trait devlaunch_core::flows::launch::ColdMachinery<'r> pub fn devlaunch_core::flows::launch::ColdMachinery::open(&mut self) -> core::result::Result, devlaunch_core::flows::launch::ColdRefused> +pub fn devlaunch_core::flows::launch::ColdMachinery::recorded(&mut self) -> core::option::Option<&devlaunch_core::domain::metadata::MetadataStorage> impl<'r> devlaunch_core::flows::launch::ColdMachinery<'r> for devlaunch_core::flows::launch::ColdPath<'r, '_> 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::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 diff --git a/rust/devlaunch-core/src/domain/metadata.rs b/rust/devlaunch-core/src/domain/metadata.rs index b22fbb8a..01e76bb9 100644 --- a/rust/devlaunch-core/src/domain/metadata.rs +++ b/rust/devlaunch-core/src/domain/metadata.rs @@ -366,6 +366,19 @@ impl std::fmt::Debug for MetadataStorage { } } +/// Whether a load may act on what it finds, or only look at it. +/// +/// The two side effects a load can have -- quarantining an unusable file and +/// backing up one that will not round-trip -- exist to protect bytes from the +/// caller's *next save*. A caller that will never save does not need them and must +/// not take them, because writing to the cache is exactly what the warm launch path +/// is not allowed to do. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum OnDisk { + Preserve, + LeaveAlone, +} + impl MetadataStorage { /// Where the file lives when nothing says otherwise. pub fn default_path() -> Result { @@ -400,10 +413,49 @@ impl MetadataStorage { worktrees: IndexMap::new(), wait_watcher: None, }; - let notices = storage.load(); + let notices = storage.load(OnDisk::Preserve); Ok((storage, notices)) } + /// Load the store at `metadata_path` to *look* at, changing nothing on disk. + /// + /// Three writes separate this from [`open`](Self::open), and every one of them + /// is there for a caller that is about to save: the cache directory is created + /// so the first save has somewhere to land, an unusable file is quarantined to + /// `.corrupt` so that save cannot overwrite bytes nobody has read, and a file + /// that will not round-trip is copied to `.bak` for the same reason. A reader + /// saves nothing, so it takes none of them, and the cache is byte for byte as + /// it was found. + /// + /// **That is what lets a read sit on the warm launch path at all** + /// (devlaunch#145): `a_warm_triple_launch_writes_nothing_to_the_cache` seeds the + /// cache with a corrupt document and asserts the launch leaves it and its two + /// siblings exactly as they were. A file that cannot be read or parsed loads as + /// an empty store here, silently -- there is nobody to tell, and the command + /// that actually needs the records will open them properly and report it then. + /// + /// No lock either. A reader that raced a writer sees the file as it was a + /// moment ago, which is what any lock-free read of a file that is replaced + /// atomically sees, and is the same staleness a caller would get by opening it + /// one instruction earlier. + #[must_use] + pub fn look(metadata_path: impl Into) -> Self { + let metadata_path = metadata_path.into(); + let file_path = resolve_link(&metadata_path); + let lock_path = sibling(&file_path, ".lock"); + let mut storage = Self { + metadata_path, + file_path, + lock_path, + schema_version: SCHEMA_VERSION, + repositories: IndexMap::new(), + worktrees: IndexMap::new(), + wait_watcher: None, + }; + let _ = storage.load(OnDisk::LeaveAlone); + storage + } + /// Be told when a mutation is about to queue behind another dl run. /// /// The one thing a returned notice cannot cover: the point of saying it is @@ -654,7 +706,7 @@ impl MetadataStorage { } }) .map_err(MetadataError::Lock)?; - let notices = self.load(); + let notices = self.load(OnDisk::Preserve); let value = work(self)?; drop(guard); Ok((value, notices)) @@ -780,7 +832,7 @@ impl MetadataStorage { .map_err(MetadataError::Lock)?; // Reload under the lock so the version below reflects any concurrent // migrator's result, not the copy this process loaded at startup. - let _ = self.load(); + let _ = self.load(OnDisk::Preserve); if self.schema_version >= SCHEMA_VERSION { return Ok(MigrationCommit::AlreadyCurrent); } @@ -795,12 +847,12 @@ impl MetadataStorage { // --- loading ---------------------------------------------------------- /// Load from disk, never failing on damaged input. - fn load(&mut self) -> Vec { + fn load(&mut self, on_disk: OnDisk) -> Vec { self.schema_version = SCHEMA_VERSION; self.repositories = IndexMap::new(); self.worktrees = IndexMap::new(); - let (data, mut notices) = self.read_file(); + let (data, mut notices) = self.read_file(on_disk); let Some(mut object) = data else { return notices; }; @@ -838,14 +890,16 @@ impl MetadataStorage { }); } - if notices.iter().any(Notice::implies_lossy_rewrite) { + if let OnDisk::Preserve = on_disk + && notices.iter().any(Notice::implies_lossy_rewrite) + { notices.push(self.backup()); } notices } /// Read and sanity-check the file, quarantining it if it is unusable. - fn read_file(&self) -> (Option>, Vec) { + fn read_file(&self, on_disk: OnDisk) -> (Option>, Vec) { if !self.file_path.exists() { return (None, Vec::new()); } @@ -863,6 +917,12 @@ impl MetadataStorage { }, }, }; + // A look reports nothing and moves nothing: an unusable file is an empty + // store to a reader, and the quarantine belongs to the caller that is about + // to write over it. + if let OnDisk::LeaveAlone = on_disk { + return (None, Vec::new()); + } let notice = Notice::FileUnusable { path: self.file_path.clone(), problem, @@ -1417,6 +1477,69 @@ mod tests { assert_eq!(storage.schema_version(), SCHEMA_VERSION); } + // --- looking, which changes nothing ----------------------------------- + + #[test] + fn a_look_reads_a_corrupt_file_as_empty_and_leaves_it_exactly_as_it_found_it() { + // The difference between `look` and `open`, stated as the thing that made it + // necessary: `open` quarantines an unusable document so its next save cannot + // overwrite bytes nobody has read, and the collision guard on the warm launch + // path (blooop/devlaunch#438) has no next save. A launch that only looks must + // leave the cache byte for byte as it was, garbage included. + let dir = temp_dir(); + let path = dir.path().join("metadata.json"); + write(&path, "not json"); + + let looked = MetadataStorage::look(&path); + + assert!(looked.worktrees().is_empty(), "unusable reads as empty"); + assert_eq!(read(&path), "not json"); + assert_eq!(names_in(dir.path()), vec!["metadata.json".to_owned()]); + } + + #[test] + fn a_look_at_a_cache_that_is_not_there_creates_nothing() { + // `open` makes the directory so its first save has somewhere to land. A + // reader has no first save, and a warm launch on a machine with no cache yet + // must not leave one behind. + let dir = temp_dir(); + let absent = dir.path().join("no-cache-here").join("metadata.json"); + + let looked = MetadataStorage::look(&absent); + + assert!(looked.worktrees().is_empty()); + assert_eq!(names_in(dir.path()), Vec::::new()); + } + + #[test] + fn a_look_reads_the_same_records_an_open_does() { + let dir = temp_dir(); + let path = dir.path().join("metadata.json"); + { + let mut storage = quiet_storage(dir.path()); + storage + .add_worktree(WorktreeInfo::new( + "owner1", + "repo1", + "branch1", + dir.path().join("repos/owner1/repo1/repo1-branch1-abcd"), + "repo1-branch1-abcd", + )) + .expect("the record is saved"); + } + + let looked = MetadataStorage::look(&path); + + assert_eq!( + looked + .worktrees() + .values() + .map(|record| record.workspace_id.clone()) + .collect::>(), + vec!["repo1-branch1-abcd".to_owned()] + ); + } + #[test] fn a_repository_can_be_added_read_listed_and_removed() { let dir = temp_dir(); diff --git a/rust/devlaunch-core/src/domain/workspace_id.rs b/rust/devlaunch-core/src/domain/workspace_id.rs index 8503cfe9..aef260fd 100644 --- a/rust/devlaunch-core/src/domain/workspace_id.rs +++ b/rust/devlaunch-core/src/domain/workspace_id.rs @@ -37,13 +37,16 @@ //! worth relying on is the *independence*: truncation policy is a readability //! choice with no effect on the collision rate. //! -//! **A collision is not caught today**, which is why the width is the whole of -//! the defence. Sharing an id means sharing one clone directory and one devpod -//! workspace, so the loser opens the winner's checkout with nothing said. -//! blooop/devlaunch#438 is the guard for that, and it is cheap because -//! [`WorktreeInfo`](crate::domain::model::WorktreeInfo) already stores the triple -//! beside the id derived from it. See [`SUFFIX_LENGTH`] for what the width is -//! chosen against in the meantime. +//! **A collision is caught, and refused rather than attached to.** Sharing an id +//! means sharing one clone directory and one devpod workspace, so the loser would +//! otherwise open the winner's checkout with nothing said. A launch whose derived +//! id a *different* triple already holds is refused instead +//! (`flows::launch::colliding_record`, blooop/devlaunch#438), and the check costs +//! nothing to hold because [`WorktreeInfo`](crate::domain::model::WorktreeInfo) +//! already stores the triple beside the id derived from it. So the width now +//! decides how often two branches have to be renamed, not whether anybody loses +//! work to a name they never saw. See [`SUFFIX_LENGTH`] for what it is chosen +//! against. //! //! [`source_workspace_id`] covers git sources that name no ref (plain URL //! specs), which cannot form a triple. Note that path specs (`dl ./some/dir`) do @@ -147,10 +150,13 @@ pub(crate) const SUFFIX_SPACE: u64 = 36u64.pow(SUFFIX_LENGTH as u32); /// of that shape wants 5 characters, which is 25.8 bits and still narrower than /// what it replaced. /// -/// **The width is the whole of the defence, which is the part worth fixing.** A -/// collision is not detected, so the loser of one silently opens the winner's -/// checkout. blooop/devlaunch#438 is the guard, and it costs nothing to hold: the -/// triple is already stored beside the id it derived. +/// **The width is no longer the whole of the defence.** A collision is detected +/// and the second launch is refused, naming both branches, rather than silently +/// opening the first one's checkout (`flows::launch::colliding_record`, +/// blooop/devlaunch#438) -- the check costs nothing to hold, because the triple is +/// already stored beside the id it derived. What the width buys now is how rarely +/// anybody has to act on that refusal, which is why the figures above are a +/// judgement about a workload rather than a correctness argument. pub(crate) const SUFFIX_LENGTH: usize = 4; /// Domain tag for ref-less sources, kept out of the triple's hash input so a URL @@ -169,6 +175,38 @@ pub enum NamePart { Ref, } +/// A triple reduced to the parts that decide *which workspace it is*. +/// +/// Owner and repo are folded to lower case; the ref is left exactly as it is. +/// GitHub treats owner and repo case-insensitively, so `NVIDIA/cuda-samples` and +/// `nvidia/cuda-samples` are one repository and must derive one id -- giving each +/// spelling its own would clone one repo twice into two containers. Git refs are +/// case-sensitive, so `Main` and `main` really are two branches and must not fold. +/// +/// **One rule, one place, because two callers now depend on agreeing.** +/// [`WorkspaceId::suffix`] hashes this, and `flows::launch`'s collision guard +/// compares it to decide whether a record holding the derived id belongs to the +/// launch in front of it. A guard that compared raw strings instead would read the +/// second spelling of a repository as a *different* triple holding the id and +/// refuse it -- telling the user to rename a branch when both branches are `main` +/// and the spelling of the owner is the only difference, which is a refusal with no +/// way out of it. The convergence is the feature; the guard has to know that. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct Identity { + pub(crate) owner: String, + pub(crate) repo: String, + pub(crate) git_ref: String, +} + +/// Reduce a triple to its [`Identity`]. See there for the rule and why it is one. +pub(crate) fn identity_of(owner: &str, repo: &str, git_ref: &str) -> Identity { + Identity { + owner: owner.to_lowercase(), + repo: repo.to_lowercase(), + git_ref: git_ref.to_owned(), + } +} + /// A name that is not safe as a git ref or a path component. #[derive(Debug, Clone, PartialEq, Eq, Hash)] // binary surface — not part of the frozen wf API (#251 §7) @@ -302,22 +340,30 @@ impl WorkspaceId { &self.git_ref } + /// This triple as [`identity_of`] reads it. + /// + /// Held here so a caller comparing a stored triple against this one compares + /// it by the rule the id was derived under, rather than by a second rule that + /// looks the same until a repository is spelled `NVIDIA`. + pub(crate) fn identity(&self) -> Identity { + identity_of(&self.owner, &self.repo, &self.git_ref) + } + /// The [`SUFFIX_LENGTH`]-character identity suffix. Never truncated. /// - /// Owner and repo are lowercased before hashing because GitHub treats them - /// case-insensitively: `NVIDIA/cuda-samples` and `nvidia/cuda-samples` are - /// one repository, and giving each spelling its own id means one repo cloned - /// twice into two containers. The ref is hashed verbatim — git refs really - /// are case-sensitive, and `Main` and `main` can both exist in one repo. + /// The hash is taken over [`identity_of`] rather than over the raw triple, so + /// the rule that decides which spellings are one workspace lives in exactly one + /// place. See that function for why owner and repo fold and the ref does not. /// /// Lowercasing does not move ids for input that was already lowercase, which /// is every id this project has published. pub(crate) fn suffix(&self) -> String { - hashed_suffix(&[ - &self.owner.to_lowercase(), - &self.repo.to_lowercase(), - &self.git_ref, - ]) + let Identity { + owner, + repo, + git_ref, + } = self.identity(); + hashed_suffix(&[&owner, &repo, &git_ref]) } /// The derived id, at most [`TARGET_LENGTH`] characters. @@ -1624,6 +1670,53 @@ mod tests { ); } + #[test] + fn identity_folds_the_owner_and_the_repo_and_never_the_ref() { + // The rule two callers depend on agreeing about: `suffix` hashes it, and + // `flows::launch`'s collision guard compares it. It is one function so that + // they cannot drift, and this is the test that pins what it says. + let folded = identity_of("NVIDIA", "CUDA-Samples", "Main"); + + assert_eq!(folded.owner, "nvidia"); + assert_eq!(folded.repo, "cuda-samples"); + assert_eq!( + folded.git_ref, "Main", + "git refs are case-sensitive, so the ref is never folded" + ); + } + + #[test] + fn the_guard_and_the_derivation_agree_about_which_spellings_are_one_workspace() { + // Stated as the property rather than as two separate assertions, because the + // defect this pins was exactly the two disagreeing: the derivation folded the + // owner and the guard did not, so the second spelling of a repository derived + // the first one's id and was then refused for holding it. + let cases = [ + ( + ("NVIDIA", "cuda-samples", "main"), + ("nvidia", "cuda-samples", "main"), + ), + ( + ("blooop", "DevLaunch", "main"), + ("blooop", "devlaunch", "main"), + ), + ]; + for (left, right) in cases { + let a = WorkspaceId::new(left.0, left.1, left.2).expect("a safe triple"); + let b = WorkspaceId::new(right.0, right.1, right.2).expect("a safe triple"); + assert_eq!( + a.value() == b.value(), + a.identity() == b.identity(), + "{left:?} and {right:?}: the id and the identity disagree" + ); + } + // And the ref, which must part them both ways. + let upper = WorkspaceId::new("owner", "repo", "Main").expect("a safe triple"); + let lower = WorkspaceId::new("owner", "repo", "main").expect("a safe triple"); + assert_ne!(upper.value(), lower.value()); + assert_ne!(upper.identity(), lower.identity()); + } + #[test] fn a_source_id_cannot_collide_with_a_triple_id() { let triple = id("anyone", "github.com", "owner/repo"); diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index cbfa5a07..35b918f7 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -69,8 +69,11 @@ use crate::clients::gh::{self, GhEvent, StagedToken, Token, TokenLookup}; use crate::clients::ssh; use crate::domain::locks::{self, Contention, LockError}; use crate::domain::metadata::MetadataStorage; +use crate::domain::model::WorktreeInfo; use crate::domain::spec::{self, DevcontainerPath, SpecIdentity, WorkspaceSpec}; -use crate::domain::workspace_id::{NamePart, UnsafeName, WorkspaceId, validate_ref_name}; +use crate::domain::workspace_id::{ + NamePart, UnsafeName, WorkspaceId, identity_of, validate_ref_name, +}; use crate::flows::kept_copies::KeptCopies; use crate::flows::lifecycle::{ self, KnownWorkspace, LifecycleNotice, Refresh, RefreshReason, StopOutcome, @@ -2538,13 +2541,36 @@ pub enum ColdRefused { /// path, which is the path a user waits on. A launcher holding a /// `&mut MetadataStorage` would have paid for all three before it could ask /// devpod anything, so it holds a *way to get one* instead, and "a warm launch -/// does no metadata I/O" is a fact about which calls happen rather than a +/// brings none of that up" is a fact about which calls happen rather than a /// property to be re-tested. /// +/// [`recorded`](Self::recorded) is the one thing a warm launch may ask for, and +/// its docs say what makes it different in kind: it reads the file and takes none +/// of the three. +/// /// It is the same move [`lifecycle::resolve_known_workspace`] makes with its /// `recorded_id` closure, one level up. pub trait ColdMachinery<'r> { fn open(&mut self) -> Result, ColdRefused>; + + /// What `metadata.json` already records, for a reader that only wants to look. + /// + /// Separate from [`open`](Self::open) because the two cost different things, + /// and the difference is the whole reason the collision guard + /// (blooop/devlaunch#438) can run in front of a warm attach at all. `open` + /// brings the *machinery* up: `config.toml`, the clone manager, and the cache + /// migration under the metadata lock, which is a subprocess every sibling + /// launch of the repository would queue behind. This reads the file and stops. + /// A launch that only has to answer "does some other triple already hold this + /// id" needs the records and none of the rest. + /// + /// **`None` means "nothing recorded", never "something went wrong".** A store + /// that cannot be found, opened or parsed answers `None` and the launch + /// proceeds, on [`recorded_id`]'s reading: a lookup that failed must not be + /// able to stop a command that would otherwise have worked. The guard above it + /// is a check on a rare accident, and a check that can fail closed turns a rare + /// accident into a common one. + fn recorded(&mut self) -> Option<&MetadataStorage>; } /// A launcher that must never reach the cold path, for callers that have already @@ -2559,6 +2585,12 @@ impl<'r> ColdMachinery<'r> for NoColdPath { fn open(&mut self) -> Result, ColdRefused> { Err(ColdRefused::NoColdPath) } + + /// Nothing recorded, which is the truth about a caller that has no records at + /// all -- and the fail-open answer the collision guard is built to accept. + fn recorded(&mut self) -> Option<&MetadataStorage> { + None + } } /// devlaunch's records, opened on the first ask and kept for the rest of the @@ -2580,6 +2612,15 @@ pub struct ColdPath<'r, 'e> { runner: &'r dyn Runner, said: &'e mut dyn Notices, records: Option>, + /// `metadata.json` as read by a caller that only wants to look at it. + /// + /// A second copy of the store, and deliberately so: loaded without the + /// migration, without `config.toml` and without the clone manager, which is the + /// whole point of [`ColdMachinery::recorded`]. Nothing ever writes through it, + /// and once the real records are open they answer instead, so it can become + /// neither a second writer nor a stale copy anybody acts on. A few kilobytes, + /// held for the length of one command. + looked_at: Option, } impl<'r, 'e> ColdPath<'r, 'e> { @@ -2593,6 +2634,7 @@ impl<'r, 'e> ColdPath<'r, 'e> { runner, said, records: None, + looked_at: None, } } @@ -2617,6 +2659,29 @@ impl<'r> ColdMachinery<'r> for ColdPath<'r, '_> { Err(refused) => Err(ColdRefused::Startup(refused)), } } + + /// The records as something to read, without bringing the machinery up. + /// + /// The real records answer if this command has already opened them, so a cold + /// launch reads the file once rather than twice and the guard sees exactly what + /// the rest of the command will write through. Otherwise it is + /// [`MetadataStorage::look`]: the file, and none of `open_records`'s config, + /// clone manager, migration or lock. + /// + /// Every failure on the way is `None`, silently. There is no sentence to say + /// here: a store dl cannot read is reported the moment the command actually + /// needs it, through the notices [`Self::records`] says, and a *look* that came + /// up empty is not news -- it is the answer "nothing is recorded against that + /// id", which is also what a machine with no records at all says. + fn recorded(&mut self) -> Option<&MetadataStorage> { + if self.records.is_some() { + return self.records.as_ref().map(|records| &records.storage); + } + if self.looked_at.is_none() { + self.looked_at = Some(MetadataStorage::look(MetadataStorage::default_path().ok()?)); + } + self.looked_at.as_ref() + } } // =========================================================================== @@ -2804,6 +2869,79 @@ fn recorded_id(cold: &mut dyn ColdMachinery<'_>, triple: (&str, &str, &str)) -> lifecycle::recorded_devpod_workspace_id(opened.storage, owner, repo, branch) } +/// The triple that already holds this launch's derived id, if a different one does. +/// +/// **The failure this closes is silence.** A workspace id is one hashed suffix +/// away from being injective ([`SUFFIX_LENGTH`](crate::domain::workspace_id)), and +/// two triples that do collide share both of the things the id names: the clone +/// directory `///` and the devpod workspace, whose +/// names are global rather than scoped by repository. So the second launch opens +/// the first one's checkout, having said nothing, and a later `dl rm` on +/// either deletes a clone the other still claims. That is the hazard +/// [`migration::migrate_record`](crate::flows::migration) already refuses to walk +/// into at migration time; this is the same refusal at launch time. +/// +/// **The scan is local.** [`WorktreeInfo`] stores the triple beside the id derived +/// from it, so the answer is in the records dl already keeps and costs no round +/// trip, no devpod call and no new stored state. +/// +/// Three ways a record can hold the id, and each names a resource that would +/// actually be shared: +/// +/// - its `workspace_id` is the id, so the clone directory is one directory; +/// - its `devpod_workspace_id` is the id, so the container is one container; +/// - its triple *derives* the id, which is the collision itself. This is the arm +/// that catches a record whose clone has not been migrated onto its derived name +/// yet, and the arm that gives the fail-open rule below something to protect. +/// +/// **A record that cannot be parsed back into a [`WorkspaceId`] is skipped, not +/// failed on.** The old derivation coerced unsafe refs instead of rejecting them, +/// so a stored branch is not necessarily a legal ref, and the migration reports +/// such records as `unusable` rather than stopping. A guard that refused every +/// launch on the machine because one old record will not parse would be worse than +/// the collision it is looking for. +/// +/// **"A different triple" means [`Identity`](crate::domain::workspace_id::Identity), +/// not a different pair of strings.** +/// `NVIDIA/cuda-samples@main` and `nvidia/cuda-samples@main` derive one id +/// deliberately -- GitHub's owners and repos are case-insensitive, and +/// [`identity_of`] is the rule that makes both spellings one workspace instead of +/// one repository cloned twice. Comparing the raw strings here would read the +/// second spelling as an intruder holding the first one's id and refuse it, with a +/// message telling the reader to rename a branch when both branches are `main`. +/// Since the comparison and the derivation have to agree, they read the same rule. +fn colliding_record( + cold: &mut dyn ColdMachinery<'_>, + workspace: &WorkspaceId, +) -> Option<(String, String, String)> { + let derived = workspace.value(); + let mine = workspace.identity(); + let storage = cold.recorded()?; + storage + .worktrees() + .values() + .find(|record| { + identity_of(&record.owner, &record.repo, &record.branch) != mine + && holds_id(record, &derived) + }) + .map(|record| { + ( + record.owner.clone(), + record.repo.clone(), + record.branch.clone(), + ) + }) +} + +/// Whether *record* occupies *derived* -- see [`colliding_record`] for the three +/// arms and why each of them is a resource and not a coincidence. +fn holds_id(record: &WorktreeInfo, derived: &str) -> bool { + record.workspace_id == derived + || record.devpod_workspace_id.as_deref() == Some(derived) + || WorkspaceId::new(&record.owner, &record.repo, &record.branch) + .is_ok_and(|derivable| derivable.value() == derived) +} + /// The default branch a bare `owner/repo` means. /// /// One extra repo-lock cycle, and it is deliberate rather than an oversight: @@ -2997,6 +3135,25 @@ pub enum LaunchRefusal { /// still lists and cannot be described — and that is precisely the workspace /// somebody is about to run `dl rm` on. UnknownWorkspace { name: String }, + /// Two triples derive one workspace id, and the other one got here first. + /// + /// Refused rather than attached to, for [`colliding_record`]'s reason: the id + /// names both the clone directory and the devpod workspace, so going ahead + /// hands this launch the *other* triple's checkout without saying so, and + /// leaves a later `dl rm` able to delete work neither user knows is + /// shared. Both triples and the id they share are carried, because the only + /// way past it is to rename one of the two branches. + IdCollision { + /// The id both triples derive. + workspace_id: String, + owner: String, + repo: String, + branch: String, + /// The triple `metadata.json` already records against `workspace_id`. + recorded_owner: String, + recorded_repo: String, + recorded_branch: String, + }, /// A bare `owner/repo` whose default branch could not be named. BranchNotNamed { owner: String, @@ -3211,6 +3368,25 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { Ok(workspace) => workspace, Err(unsafe_name) => return Ok(Err(LaunchRefusal::UnsafeSpec(unsafe_name))), }; + // Before devpod is asked anything and before the cold path can build a + // directory: an id two triples derive is not an id either of them may be + // launched under, and the damage is done by the *attach*, so a guard behind + // the status call would fire after the wrong container was already picked. + // Reads the records and not the machinery, which is what keeps this off + // devlaunch#145's bill -- see `ColdMachinery::recorded`. + if let Some((recorded_owner, recorded_repo, recorded_branch)) = + colliding_record(self.cold, &workspace) + { + return Ok(Err(LaunchRefusal::IdCollision { + workspace_id: workspace.value(), + owner, + repo, + branch, + recorded_owner, + recorded_repo, + recorded_branch, + })); + } // A devpod that could not be run ends the launch here, before the clone: // it is the probe Python raises `DevpodNotInstalled` out of. let resolved = resolve_triple( @@ -3865,6 +4041,14 @@ mod tests { storage: &mut self.storage, }) } + + /// The same store, read rather than opened -- and not counted as an open, + /// because it is not one: the collision guard reads the file and the + /// machinery stays down. `opens` is what the #145 assertions are written + /// against, so counting a look would make them assert something else. + fn recorded(&mut self) -> Option<&MetadataStorage> { + Some(&self.storage) + } } /// A cold path that fails the test if anything opens it. @@ -3878,6 +4062,34 @@ mod tests { fn open(&mut self) -> Result, ColdRefused> { panic!("a warm launch opened the cold path"); } + + /// Nothing recorded, which is the truth about a scene with no store behind + /// it -- and not a panic, because looking is exactly what a warm launch is + /// now allowed to do. The panic above still says what it said: the clone + /// manager, `config.toml` and the migration stay off this path. + fn recorded(&mut self) -> Option<&MetadataStorage> { + None + } + } + + /// A cold path with real records that cannot be *read*. + /// + /// The fail-open case, and the reason it needs a stub of its own: a store dl + /// cannot open, parse or find is common enough (a fresh machine, a half-written + /// file, a cache on a filesystem that went away) that a guard which stopped the + /// launch over it would break far more launches than the collision it is + /// watching for ever will. So the records here hold a collision and the look + /// answers `None` anyway. + struct UnreadableRecords<'r>(RealCold<'r>); + + impl<'r> ColdMachinery<'r> for UnreadableRecords<'r> { + fn open(&mut self) -> Result, ColdRefused> { + self.0.open() + } + + fn recorded(&mut self) -> Option<&MetadataStorage> { + None + } } /// A cold path whose `metadata.json` will not open, refusing exactly as @@ -3904,6 +4116,14 @@ mod tests { metadata_refusal(), ))) } + + /// A store that will not open has nothing to show a reader either, and + /// answers so rather than refusing: the collision guard treats "could not + /// read" as "no collision", which is what keeps a broken cache from + /// stopping every launch on the machine. + fn recorded(&mut self) -> Option<&MetadataStorage> { + None + } } /// Records which workspaces had tools lent to them, on which occasion, and under @@ -6893,6 +7113,384 @@ mod tests { assert_eq!(cold.opens.get(), 1, "the record was consulted exactly once"); } + // ------------------------------------- stage two: the derived-id collision + // + // blooop/devlaunch#438. `COLLIDING_A` and `COLLIDING_B` are a real pair: two + // branches of one repository whose triples derive one id, found by searching + // the shape the module doc records having seen in the wild -- long release + // refs differing only past the truncation point, so the readable halves are + // identical and only the four-character suffix separates them. They are + // written down rather than searched for at test time because a fixed pair is + // the same test every run, and because finding one is a birthday search over + // 36^4 that has no business running in a unit test. + // + // If a change to the derivation moves them apart, the first test below fails + // and says so, rather than the guard's tests quietly passing against two ids + // that no longer collide. + + const COLLIDING_A: &str = "release/999999999999999999999911630"; + const COLLIDING_B: &str = "release/999999999999999999999911783"; + + /// Add one worktree record to the store at *cache_dir*, and save it. + fn record_worktree( + cache_dir: &Path, + owner: &str, + repo: &str, + branch: &str, + workspace_id: &str, + ) { + let (mut storage, _) = + MetadataStorage::open(cache_dir.join("metadata.json")).expect("a fresh store opens"); + storage + .add_worktree(WorktreeInfo::new( + owner, + repo, + branch, + cache_dir.join(format!("repos/{owner}/{repo}/{workspace_id}")), + workspace_id, + )) + .expect("the record is saved"); + } + + #[test] + fn both_of_the_colliding_refs_really_do_derive_one_id() { + let a = WorkspaceId::new("blooop", "devlaunch", COLLIDING_A).expect("a safe triple"); + let b = WorkspaceId::new("blooop", "devlaunch", COLLIDING_B).expect("a safe triple"); + + assert_ne!(COLLIDING_A, COLLIDING_B, "two different branches"); + assert_eq!( + a.value(), + b.value(), + "the pair the collision tests are written against no longer collides" + ); + } + + #[test] + fn a_launch_whose_derived_id_another_triple_already_holds_is_refused() { + // The failure being closed: devpod workspace names are global rather than + // scoped by repository, so the second of two colliding triples finds the + // first one's container under its own derived id, attaches, and says + // nothing. The user gets somebody else's checkout, and a later `rm` on + // either one deletes a clone the other still claims. + let held = WorkspaceId::new("blooop", "devlaunch", COLLIDING_A).expect("a safe triple"); + let scene = Scene::new().with_running(&held.value()); + record_worktree( + scene.cache_dir(), + "blooop", + "devlaunch", + COLLIDING_A, + &held.value(), + ); + let git = Git::new(&scene.runner); + let mut cold = RealCold::new(scene.cache_dir(), git); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + 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( + &format!("blooop/devlaunch@{COLLIDING_B}"), + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Refused(LaunchRefusal::IdCollision { + workspace_id: held.value(), + owner: "blooop".to_owned(), + repo: "devlaunch".to_owned(), + branch: COLLIDING_B.to_owned(), + recorded_owner: "blooop".to_owned(), + recorded_repo: "devlaunch".to_owned(), + recorded_branch: COLLIDING_A.to_owned(), + })) + ); + assert_eq!( + scene.devpod_heads(), + Vec::>::new(), + "refused before devpod was asked anything, so nothing attached" + ); + } + + #[test] + fn a_launch_that_matches_its_own_record_attaches_and_reads_no_machinery() { + // The warm path, unchanged and unslowed. A record holding this triple's own + // derived id is not a collision -- it is the ordinary case, every workspace + // dl has ever made -- and the guard looking at the records must not turn + // devlaunch#145's warm attach into a cache migration under the metadata + // lock. `opens` is the assertion: the guard read the file, the machinery + // stayed down. + let workspace = + WorkspaceId::new("blooop", "devlaunch", COLLIDING_A).expect("a safe triple"); + let scene = Scene::new().with_running(&workspace.value()); + record_worktree( + scene.cache_dir(), + "blooop", + "devlaunch", + COLLIDING_A, + &workspace.value(), + ); + let git = Git::new(&scene.runner); + let mut cold = RealCold::new(scene.cache_dir(), git); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + { + 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( + &format!("blooop/devlaunch@{COLLIDING_A}"), + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Session(Session::RemoteExit { status: 0 })), + "the triple's own record is not a collision" + ); + } + assert_eq!( + cold.opens.get(), + 0, + "a warm launch still brings no clone manager, config or migration up" + ); + } + + #[test] + fn a_repository_spelled_in_another_case_is_the_same_workspace_and_is_not_refused() { + // The convergence `suffix` exists to guarantee, seen from the guard's side. + // GitHub's owners are case-insensitive, so `NVIDIA/cuda-samples` and + // `nvidia/cuda-samples` are one repository and derive one id on purpose -- + // the alternative is one repo cloned twice into two containers. A guard that + // compared the triples as raw strings read the second spelling as a + // *different* triple holding the first one's id and refused it, and the + // refusal it printed told the reader to rename one of the two branches when + // both branches are `main`: a message naming no way out that exists, in + // front of a workspace the reader already has running. + let recorded = WorkspaceId::new("nvidia", "cuda-samples", "main").expect("a safe triple"); + let typed = WorkspaceId::new("NVIDIA", "cuda-samples", "main").expect("a safe triple"); + assert_eq!( + typed.value(), + recorded.value(), + "the two spellings are one workspace, which is what makes this a trap" + ); + let scene = Scene::new().with_running(&recorded.value()); + record_worktree( + scene.cache_dir(), + "nvidia", + "cuda-samples", + "main", + &recorded.value(), + ); + let git = Git::new(&scene.runner); + let mut cold = RealCold::new(scene.cache_dir(), git); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + 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( + "NVIDIA/cuda-samples@main", + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Session(Session::RemoteExit { status: 0 })), + "the other spelling of a repository is not an intruder holding its id" + ); + } + + #[test] + fn a_ref_that_differs_only_in_case_is_a_different_workspace_and_does_collide() { + // The other half of the rule, and the reason the fix folds two of the three + // parts rather than all of them. Git refs are case-sensitive: `Main` and + // `main` can both exist in one repository, so they are two workspaces. Were + // the ref folded along with the owner, this launch would be waved through to + // attach to the other branch's container -- the exact silent wrong-checkout + // this guard exists to stop. + let typed = WorkspaceId::new("owner", "repo", "Main").expect("a safe triple"); + let other = WorkspaceId::new("owner", "repo", "main").expect("a safe triple"); + assert_ne!( + typed.value(), + other.value(), + "the two refs hash apart, which is why the collision below has to be staged" + ); + let scene = Scene::new().with_running(&typed.value()); + record_worktree( + scene.cache_dir(), + "owner", + "repo", + "main", + // The `main` record is put on the id `Main` derives. Two refs differing + // only in case do not collide on their own -- they hash apart by design + // -- so a real collision between them cannot be produced, only staged. + // What is under test is the comparison, not the hash: the guard has to + // read these two as different triples. + &typed.value(), + ); + let git = Git::new(&scene.runner); + let mut cold = RealCold::new(scene.cache_dir(), git); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + 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( + "owner/repo@Main", + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Refused(LaunchRefusal::IdCollision { + workspace_id: typed.value(), + owner: "owner".to_owned(), + repo: "repo".to_owned(), + branch: "Main".to_owned(), + recorded_owner: "owner".to_owned(), + recorded_repo: "repo".to_owned(), + recorded_branch: "main".to_owned(), + })), + "a ref differing only in case is a different branch, not the same one" + ); + } + + #[test] + fn a_record_whose_branch_is_not_a_legal_ref_does_not_block_a_launch() { + // The old derivation coerced unsafe refs instead of rejecting them, so a + // stored branch is not necessarily a ref `WorkspaceId::new` will accept. + // The migration reports such a record as `unusable` and carries on; the + // guard skips it. A guard that failed on it would refuse every launch on + // the machine, which is far worse than the one-in-thirty-seven-thousand + // accident it is watching for. + let workspace = WorkspaceId::new("owner", "repo", "main").expect("a safe triple"); + let scene = Scene::new().with_running(&workspace.value()); + record_worktree( + scene.cache_dir(), + "owner", + "repo", + "a branch with spaces", + "repo-a-branch-with-spaces-legacy", + ); + let git = Git::new(&scene.runner); + let mut cold = RealCold::new(scene.cache_dir(), git); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + 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( + "owner/repo@main", + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Session(Session::RemoteExit { status: 0 })) + ); + } + + #[test] + fn a_store_that_cannot_be_read_does_not_refuse_a_launch() { + // `recorded_id` states the rule and the guard honours it: a lookup that + // failed means "no collision found", not an error. The records here hold + // the colliding record from the refusal test, so the only thing standing + // between this launch and a refusal is that the look came up empty. + let held = WorkspaceId::new("blooop", "devlaunch", COLLIDING_A).expect("a safe triple"); + let scene = Scene::new().with_running(&held.value()); + record_worktree( + scene.cache_dir(), + "blooop", + "devlaunch", + COLLIDING_A, + &held.value(), + ); + let git = Git::new(&scene.runner); + let mut cold = UnreadableRecords(RealCold::new(scene.cache_dir(), git)); + let updater = SelfInvocation::new("dl"); + let completion = scene.cache_dir().join("completion.json"); + let mut parts = launching(&scene.runner, &updater, &completion); + 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( + &format!("blooop/devlaunch@{COLLIDING_B}"), + &LaunchVerb::Attach { + command: Some("true".to_owned()), + }, + None, + ); + + assert_eq!( + launched, + Ok(Launched::Session(Session::RemoteExit { status: 0 })), + "an unreadable store means no collision, not a refusal" + ); + } + #[test] fn a_placement_answers_the_fast_attach_question_on_its_own() { // Python reads it off two correlated locals; here it is one value. diff --git a/rust/devlaunch-core/src/flows/migration.rs b/rust/devlaunch-core/src/flows/migration.rs index 2d4f332a..8e359f60 100644 --- a/rust/devlaunch-core/src/flows/migration.rs +++ b/rust/devlaunch-core/src/flows/migration.rs @@ -191,14 +191,21 @@ pub fn migrate_cache( // filesystem has done. `commit_migration` reloads under the lock and re-checks // the version, so a concurrent process that already migrated is seen here. let committed = storage.commit_migration(|worktrees| { - // Snapshotted before any record is touched: it has to describe the layout - // the run started from, not one the run is halfway through rewriting. - let claimed: HashSet = worktrees + // Seeded with the layout the run started from, and *grown as destinations + // are taken*. Both halves matter. A pre-run snapshot alone answers "does + // another record own this directory today", which misses the case where two + // schema-2 records of one repository derive the same schema-3 id: their + // shared destination is a new path no record pointed at before the run, so + // the guard below never fired, the first record renamed onto it and the + // second silently repointed itself at the same directory. That is exactly + // the outcome `blocked` exists to prevent, reached by a route the snapshot + // could not see (blooop/devlaunch#438). + let mut claimed: HashSet = worktrees .values() .map(|record| record.local_path.clone()) .collect(); for record in worktrees.values_mut() { - migrate_record(record, repos_dir, &claimed, &mut report); + migrate_record(record, repos_dir, &mut claimed, &mut report); } // Anything still under an old-scheme name that no record claims. Computed @@ -253,11 +260,14 @@ pub fn migrate_cache( /// the truth about where the clone is now, which is the same principle that made /// removal work for old-scheme workspaces (#64). /// -/// `claimed` is every path some record pointed at before this run started. +/// `claimed` is every path a record points at: the ones they pointed at before the +/// run, plus every destination taken since. It grows as this runs, which is what +/// makes the second record to derive a given destination `blocked` rather than a +/// second owner of one directory. fn migrate_record( record: &mut WorktreeInfo, repos_dir: &Path, - claimed: &HashSet, + claimed: &mut HashSet, report: &mut MigrationReport, ) { let Ok(workspace) = WorkspaceId::new(&record.owner, &record.repo, &record.branch) else { @@ -276,9 +286,11 @@ fn migrate_record( let dest = clone_dir(repos_dir, &record.owner, &record.repo, &workspace.value()); if dest != src && claimed.contains(&dest) { - // The derived name is a directory some *other* record owns. Only possible - // when a branch was literally named after another branch's derived id — - // #55's `foo-bexoza` case, now needing an exact hash match. Rename nothing + // The derived name is a directory some *other* record owns, either since + // before this run (a branch literally named after another branch's derived + // id, #55's `foo-bexoza` case, now needing an exact hash match) or since a + // moment ago, because an earlier record in this same pass derived the same + // id and took it. Rename nothing // and, unlike every other outcome, do not repoint the record either: // adopting a clone another record owns is how one workspace's `rm` deletes // another's work, which is the class of bug #9766 was. @@ -311,6 +323,8 @@ fn migrate_record( if record.workspace_id != derived { report.orphaned_ids.push(record.workspace_id.clone()); } + // Taken, so no later record in this pass can land on it too. + claimed.insert(dest.clone()); record.local_path = dest; // The record carries the derived id, because removal by id looks records up by // exactly the id dl derives from the spec. `devpod_workspace_id` is left alone: @@ -980,6 +994,58 @@ mod tests { ); } + #[test] + fn two_records_that_derive_one_destination_do_not_both_end_up_owning_it() { + // The hole the pre-run snapshot left (blooop/devlaunch#438). These two + // branches are a real collision: different refs of one repository whose + // triples derive one id, so they derive one destination. That destination is + // a *new* path, which no record pointed at before the run, so a `claimed` + // built only from the old layout does not contain it: the first record + // renames onto it and the second finds `dest.exists()`, renames nothing and + // repoints itself at the same directory. Two records, one clone, and a later + // `dl rm` on either deletes work the other still claims -- which is the + // outcome the `blocked` arm exists to prevent, reached by a route its guard + // could not see. + let first = "release/999999999999999999999911630"; + let second = "release/999999999999999999999911783"; + let shared = new_leaf("blooop", "devlaunch", first); + assert_eq!( + shared, + new_leaf("blooop", "devlaunch", second), + "the pair this test is written against no longer collides" + ); + let cache = build_legacy_cache(&[("blooop", "devlaunch", &[first, second])]); + let repo_root = cache.repo_root("blooop", "devlaunch"); + + let report = cache.migrate().expect("a migration ran"); + + assert_eq!( + report.blocked, + [Blocked { + from: repo_root.join(old_leaf(second)), + to: repo_root.join(&shared), + }], + "the second record to derive the shared destination is refused" + ); + assert_eq!( + cache.worktree(&format!("blooop/devlaunch/{first}"))["local_path"].as_str(), + Some(repo_root.join(&shared).display().to_string().as_str()), + "the first record took the destination" + ); + assert_eq!( + cache.worktree(&format!("blooop/devlaunch/{second}"))["local_path"].as_str(), + Some( + repo_root + .join(old_leaf(second)) + .display() + .to_string() + .as_str() + ), + "the second record kept its own clone rather than adopting the first's" + ); + assert!(repo_root.join(old_leaf(second)).is_dir()); + } + #[test] fn a_branch_named_after_another_branchs_derived_id_is_refused() { // #55's `foo-bexoza` case, now needing an exact hash match. Adopting a clone diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index 9389cc0b..794f1d26 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -308,6 +308,7 @@ fn ran(outcome: Result, cache: &Path) -> Ran { // `dl --prune` lists. LaunchRefusal::UnsafeSpec(_) | LaunchRefusal::UnknownWorkspace { .. } + | LaunchRefusal::IdCollision { .. } | LaunchRefusal::BranchNotNamed { .. } | LaunchRefusal::NotPrepared { .. } => Ran { ending: Ending::Refused, diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 3fd41ebc..f7e43b2a 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1536,6 +1536,25 @@ pub(crate) fn delete_refused(workspace: &str, swept: Swept) -> String { } /// The one sentence a target no command can address gets. +/// Why two branches cannot both be launched, and which two they are. +/// +/// **Names both specs and the id, because renaming a branch is the only way past +/// this.** A message that named one side would leave the reader hunting for the +/// other through `dl --ls`, where the two rows are the same 47 characters. +/// +/// The second sentence is the stake rather than the mechanism. Nobody needs the +/// birthday bound at the moment they are stopped; they need to know that going +/// ahead would have handed them somebody else's checkout, because that is the +/// thing they would otherwise have gone looking for a devpod bug about. +pub(crate) fn id_collision(workspace_id: &str, spec: &str, held_by: &str) -> String { + format!( + "Workspace id '{workspace_id}' is already held by '{held_by}'. Launching '{spec}' under \ + it would put both in one clone directory and one container, so each would open the \ + other's checkout and 'dl rm' on either would delete work the other still owns. \ + Rename one of the two branches and launch it again." + ) +} + pub(crate) fn unknown_workspace(target: &str) -> String { format!( "Unknown workspace '{target}'. Use 'dl --ls' to list workspaces, or specify owner/repo or \ @@ -2623,6 +2642,19 @@ pub(crate) fn launch_refusal(refused: &LaunchRefusal) -> Option { match refused { LaunchRefusal::UnsafeSpec(name) => Some(unsafe_name(name)), LaunchRefusal::UnknownWorkspace { name } => Some(unknown_workspace(name)), + LaunchRefusal::IdCollision { + workspace_id, + owner, + repo, + branch, + recorded_owner, + recorded_repo, + recorded_branch, + } => Some(id_collision( + workspace_id, + &format!("{owner}/{repo}@{branch}"), + &format!("{recorded_owner}/{recorded_repo}@{recorded_branch}"), + )), LaunchRefusal::BranchNotNamed { owner, repo, error } => Some(format!( "Repository '{owner}/{repo}': {}", branch_not_named(error) @@ -3628,6 +3660,38 @@ mod tests { ); } + #[test] + fn an_id_collision_names_both_branches_the_id_and_the_way_out() { + // The only action available to the reader is to rename one of the two + // branches, so the sentence has to hand them both of them: the two rows are + // the same 47 characters in `dl --ls`, and a message that named one side + // would leave them hunting for the other. + let line = launch_refusal(&LaunchRefusal::IdCollision { + workspace_id: "devlaunch-release-999999999999999999999911-dq8q".to_owned(), + owner: "blooop".to_owned(), + repo: "devlaunch".to_owned(), + branch: "release/999999999999999999999911783".to_owned(), + recorded_owner: "blooop".to_owned(), + recorded_repo: "devlaunch".to_owned(), + recorded_branch: "release/999999999999999999999911630".to_owned(), + }) + .expect("a refusal dl has to say itself"); + + assert!( + line.contains("'devlaunch-release-999999999999999999999911-dq8q'"), + "{line}" + ); + assert!( + line.contains("'blooop/devlaunch@release/999999999999999999999911783'"), + "{line}" + ); + assert!( + line.contains("'blooop/devlaunch@release/999999999999999999999911630'"), + "{line}" + ); + assert!(line.contains("Rename one of the two branches"), "{line}"); + } + #[test] fn the_terminal_title_is_the_one_notice_with_no_line() { // Not a debug line -- a notice that is not a sentence at all. Rendering it diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index 1d2c5eae..9e696143 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -449,12 +449,20 @@ fn a_devcontainer_choice_a_running_workspace_cannot_honour_is_said_not_discarded } #[test] -fn a_warm_triple_launch_does_no_metadata_io_at_all() { +fn a_warm_triple_launch_writes_nothing_to_the_cache() { // devlaunch#145, observed on disk rather than through a mock. The cache is - // seeded with an unparsable `metadata.json`: any path that *reads* it + // seeded with an unparsable `metadata.json`: any path that *opens* it // quarantines it to `metadata.json.corrupt` and says so, and any path that takes - // the metadata lock leaves `metadata.json.lock` behind. A launch that did no - // metadata I/O leaves the garbage byte-identical and creates neither sibling. + // the metadata lock leaves `metadata.json.lock` behind. A launch that wrote + // nothing leaves the garbage byte-identical and creates neither sibling. + // + // **A warm launch does read the file**, and this test is deliberately not about + // that. The collision guard (blooop/devlaunch#438) reads it through + // `MetadataStorage::look`, which takes no lock, runs no migration and writes + // nothing -- so every assertion below still holds, and holds honestly. What #145 + // bought was that a user waiting on an attach does not pay for the *machinery*; + // the name of this test used to say "no I/O at all", which claimed more than the + // assertions do and would send a reader looking to undo that read. let world = World::with(&["--warm"]); std::fs::write(world.path("cache/devlaunch/metadata.json"), "not json") .expect("a corrupt document"); @@ -490,6 +498,54 @@ fn a_warm_triple_launch_does_no_metadata_io_at_all() { ); } +#[test] +fn a_warm_launch_whose_id_another_triple_holds_is_refused_by_the_real_binary() { + // blooop/devlaunch#438, through the shipping code path rather than a stub. + // + // **This is the test that watches `ColdPath::recorded`.** Every other test of + // the guard hands `Launch` a cold path built for the test, so all of them stay + // green if the production implementation answers "nothing recorded" -- which is + // exactly what a rebase did to it once, grafting `NoColdPath`'s body onto + // `ColdPath` and switching the guard off with the suite still passing. Only a + // run of the real binary against a real `metadata.json` can tell the two apart. + // + // The world is the warm one, with its record moved onto a different branch: the + // clone and the devpod workspace stay exactly where they were, so `main` still + // derives `MAIN` and devpod still has it running, but the record holding that id + // now names `blooop/devlaunch@other`. That is what a collision looks like from + // disk, and it is staged rather than derived because a real suffix collision is + // a one-in-1.7-million search. + let world = World::with(&["--warm"]); + let records = world.path("cache/devlaunch/metadata.json"); + let seeded = std::fs::read_to_string(&records).expect("the fixture's records"); + let moved = seeded + .replace("\"blooop/devlaunch/main\"", "\"blooop/devlaunch/other\"") + .replace("\"branch\": \"main\"", "\"branch\": \"other\""); + assert_ne!( + moved, seeded, + "the record was not moved, so this proves nothing" + ); + std::fs::write(&records, &moved).expect("the edited records"); + + let run = world.dl(&["blooop/devlaunch@main", "--", "echo", "hi"]); + + run.exited(1); + let said = run.stderr_lines().join("\n"); + assert!(said.contains(MAIN), "the id is named: {said}"); + assert!( + said.contains("blooop/devlaunch@main"), + "the launched triple is named: {said}" + ); + assert!( + said.contains("blooop/devlaunch@other"), + "the triple already holding it is named: {said}" + ); + // Refused before devpod was asked anything, which is the point of where the + // guard sits: the damage is done by the attach, so a check behind the status + // call would fire after the wrong container had already been chosen. + assert_eq!(world.calls().exact(&world.root), Vec::::new()); +} + // =========================================================================== // the opt-in dotfiles refresh // ===========================================================================