From beb4dfbbd40169d4274d42eb0750ce70581422af Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 26 Aug 2026 01:56:21 +0500 Subject: [PATCH 1/2] fix: an update that is not an install, and three provenance fields that were null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **`software_install` and `software_update` produced byte-identical plans.** Same effects, same artifact, same wording — the only difference was the string in `--operation`. The plan phase may read the local disk and may not reach the network, so what is already under `--prefix` now belongs in it: four situations read as four different plans, and an update of a prefix holding nothing is refused rather than quietly installing, because installing when asked to update is doing something else and calling it done. `software_remove` names the versions it leaves. **Three provenance fields were written empty for every bundle install**, and the bundle carried all three. `component_refs` was an empty list while the conversion report named every component. `setup_stable_id` and `setup_version` were null because `setup-passport.json` — a required member of the format — was required and then discarded, so a target configured over the wire reported no applied setup while the same target configured from the local catalog reported one. `setup_version_passport_digest` stays null on purpose: the passport does not carry its own digest and the contract does not define how one is taken, so a value computed here would be this program's opinion rather than the passport's. **Test fixtures no longer collide between processes.** Nine `scratch()` helpers named a temporary directory after the test alone and each begins by removing it, so two `cargo test` runs on one machine deleted each other's fixtures — measured at six and two failures. They name their process now. --- crates/harness-runtime/src/catalog.rs | 3 +- crates/harness-runtime/src/human.rs | 3 +- crates/harness-runtime/src/software.rs | 96 ++++++++++-- crates/harness-runtime/src/wire.rs | 207 ++++++++++++++++++++++++- crates/provider-v3/src/bundle.rs | 41 ++++- crates/setup-core/src/backup.rs | 3 +- crates/setup-core/src/digest.rs | 3 +- crates/setup-core/src/journal.rs | 3 +- crates/setup-core/src/lock.rs | 3 +- crates/setup-core/src/software.rs | 141 +++++++++++++++++ crates/setup-core/src/stamp.rs | 3 +- crates/setup-core/src/target.rs | 3 +- 12 files changed, 483 insertions(+), 26 deletions(-) diff --git a/crates/harness-runtime/src/catalog.rs b/crates/harness-runtime/src/catalog.rs index e2a7cd3..753b938 100644 --- a/crates/harness-runtime/src/catalog.rs +++ b/crates/harness-runtime/src/catalog.rs @@ -374,7 +374,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("harness-catalog-{name}")); + let base = + std::env::temp_dir().join(format!("harness-catalog-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/harness-runtime/src/human.rs b/crates/harness-runtime/src/human.rs index 90e9517..7c86473 100644 --- a/crates/harness-runtime/src/human.rs +++ b/crates/harness-runtime/src/human.rs @@ -943,7 +943,8 @@ mod tests { use crate::catalog::{SETUP_MANIFEST, SETUP_PAYLOAD, SETUP_SCHEMA, SetupManifest}; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("harness-human-{name}")); + let base = + std::env::temp_dir().join(format!("harness-human-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(base.join("target")).unwrap(); fs::canonicalize(&base).unwrap() diff --git a/crates/harness-runtime/src/software.rs b/crates/harness-runtime/src/software.rs index e84fee2..74fa166 100644 --- a/crates/harness-runtime/src/software.rs +++ b/crates/harness-runtime/src/software.rs @@ -119,23 +119,76 @@ pub(crate) fn plan( let entry_point = format!("bin/{}", declared.command); let exposed = root.join(&entry_point); + // Planning may read the local disk and may not reach the network, so what + // is already under the prefix belongs in the plan. Without it an install + // and an update produced byte-identical effects -- two names for one act, + // and neither said what was about to be replaced. + let present = software::Present::under(&root, declared.command); + if operation == Operation::SoftwareRemove { - return Ok(( - Vec::new(), - vec![ - format!( - "remove {}, the {} tree this provider installed", - root.join(declared.version).display(), - declared.version - ), - format!("remove {}", exposed.display()), - ], + let mut effects = vec![ + format!( + "remove {}, the {} tree this provider installed", + root.join(declared.version).display(), + declared.version + ), + format!("remove {}", exposed.display()), + ]; + // Other versions are left, and saying which is the difference between + // leaving them and losing track of them. This build pins one version + // and cannot know whether an older tree is still wanted. + let kept: Vec<&str> = present + .versions + .iter() + .map(String::as_str) + .filter(|version| *version != declared.version) + .collect(); + if !kept.is_empty() { + effects.push(format!( + "leave {} in place: this build pins {} and does not decide about versions it \ + does not pin", + kept.join(", "), + declared.version + )); + } + return Ok((Vec::new(), effects)); + } + + // An update of nothing is a request that cannot be honoured as asked. + // Installing instead would be doing something else and calling it done. + if operation == Operation::SoftwareUpdate && present.versions.is_empty() { + return Err(Error::refuse( + WireReason::ProviderUnavailable, + format!( + "there is no {} under {} to update; software_install is the operation that \ + puts one there", + declared.command, + root.display() + ), )); } let (os, arch) = platform_of_this_host(); let artifact = declared.artifact_for(os, arch)?; - let effects = vec![ + let mut effects = Vec::new(); + if present.holds(declared.version) { + effects.push(format!( + "replace {}, which is already installed", + declared.version + )); + } else if let Some(running) = present.exposed.as_deref() { + effects.push(format!( + "move {} from {running} to {}, keeping {running} where it is", + declared.command, declared.version + )); + } else if !present.versions.is_empty() { + effects.push(format!( + "install {} beside {}, which no entry point names", + declared.version, + present.versions.join(", ") + )); + } + effects.extend([ format!( "download {} ({} bytes) in the operation's own download phase", artifact.url, artifact.bytes @@ -153,7 +206,7 @@ pub(crate) fn plan( ), }, format!("point {} at it", exposed.display()), - ]; + ]); Ok(( vec![SoftwareArtifact { @@ -228,6 +281,25 @@ pub(crate) fn apply( )); }; + // Re-checked here, not trusted from the plan: applying happens later, and + // the prefix could have been emptied in between. The plan's digest binds + // what was decided, not what the disk still holds. + if operation == Operation::SoftwareUpdate + && software::Present::under(&root, declared.command) + .versions + .is_empty() + { + return Err(Error::refuse( + WireReason::ProviderUnavailable, + format!( + "there is no {} under {} to update any more; software_install is the operation \ + that puts one there", + declared.command, + root.display() + ), + )); + } + let (os, arch) = platform_of_this_host(); let artifact = declared.artifact_for(os, arch)?; let installed = software::install(&declared, artifact, path, &root)?; diff --git a/crates/harness-runtime/src/wire.rs b/crates/harness-runtime/src/wire.rs index 070f10d..726d04b 100644 --- a/crates/harness-runtime/src/wire.rs +++ b/crates/harness-runtime/src/wire.rs @@ -564,12 +564,31 @@ pub(crate) struct Applied { pub setup_id: Option, /// The digest of that setup's definition, which is its real identity. pub setup_definition_digest: Option, + /// The setup version, when the thing that arrived stated one. + /// + /// A bundle's setup passport does; a setup in the local catalog carries an + /// id and a description and no version, and inventing one there would be + /// worse than the null. + pub setup_version: Option, /// The bundle format, when a bundle put it there. pub bundle_format: Option, /// The bundle's own digest. pub bundle_digest: Option, /// The digest of the artifact the bundle arrived in. pub artifact_digest: Option, + /// The components the projection carried, in the order it declared them. + /// + /// The contract asks a provider to record which components a target holds, + /// and the bundle's conversion report is the only place their stable + /// identities appear — the manifest carries paths and the setup passport + /// carries references without kinds. This was written as an empty list + /// whatever arrived, so a target configured from a bundle reported that it + /// held no components while holding every one the bundle named. + /// + /// Empty is still correct for the local catalog: a setup there is a tree of + /// files with an id and a description, and it has no component identities + /// to record. Inventing some would be worse than the empty list. + pub component_refs: Vec, } /// Apply one exact plan under the target lock. @@ -658,6 +677,23 @@ fn apply( applied.bundle_format = Some(named.binding.bundle_format.clone()); applied.bundle_digest = Some(named.binding.bundle_digest.clone()); applied.artifact_digest = Some(named.binding.artifact_digest.clone()); + // Two provenance fields the contract names and the passport + // states. They were null for every bundle install, because the + // passport was a required member that nothing read. + if !ready.passport.stable_id.is_empty() { + applied.setup_id = Some(ready.passport.stable_id.clone()); + } + if !ready.passport.version.is_empty() { + applied.setup_version = Some(ready.passport.version.clone()); + } + applied.component_refs = ready + .manifest + .conversion_report + .entries + .iter() + .map(|entry| entry.stable_id.clone()) + .filter(|stable_id| !stable_id.is_empty()) + .collect(); Effect::MaterializeBundle { files: &ready.files, } @@ -979,11 +1015,16 @@ fn write_state( setup_stable_id: applied.setup_id.clone(), // A local setup carries an id and a description and no version, so // there is no version to record and inventing one would be worse than - // the null. These stay for setups that arrive with a version passport. - setup_version: None, + // the null. A bundle states one in its passport, and that is where this + // comes from. + setup_version: applied.setup_version.clone(), + // Still null, deliberately. The passport does not carry its own digest + // and the contract does not define how one is taken, so a value + // computed here would be this program's opinion of the passport's + // identity rather than the passport's. setup_version_passport_digest: None, setup_definition_digest: applied.setup_definition_digest.clone(), - component_refs: Vec::new(), + component_refs: applied.component_refs.clone(), bundle_format: applied.bundle_format.clone(), bundle_digest: applied.bundle_digest.clone(), artifact_digest: applied.artifact_digest.clone(), @@ -1154,7 +1195,8 @@ mod tests { /// correctly refuse its own plan as stale. It must also be unique per test, /// because these run in parallel. fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("harness-runtime-{name}")); + let base = + std::env::temp_dir().join(format!("harness-runtime-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(base.join("target")).unwrap(); fs::canonicalize(&base).unwrap() @@ -1851,9 +1893,22 @@ mod tests { mode: 0o644, }]; for name in REQUIRED_MEMBERS.iter().skip(1) { + // The passport carries the setup identity a provider must record. + // An empty object here would have made every bundle test agree with + // a provider that read nothing, which is how the field stayed null. + let data = if *name == "setup-passport.json" { + serde_json::to_vec(&serde_json::json!({ + "stable_id": "setup_00000000000000000000000000", + "version": "3.1.0", + "harness_id": TEST.harness_id, + })) + .unwrap() + } else { + b"{}".to_vec() + }; entries.push(Entry { name: (*name).to_owned(), - data: b"{}".to_vec(), + data, mode: 0o644, }); } @@ -1884,6 +1939,84 @@ mod tests { ] } + #[test] + fn what_a_bundle_states_about_itself_is_recorded_in_provider_state() { + // `component_refs` is a provenance field the contract asks for, and the + // conversion report is the only place a component's stable identity + // appears. It was written as an empty list whatever arrived, so a + // target configured from a bundle reported holding no components while + // holding every one the bundle named. + let target = seeded("bundle-components"); + let (bytes, bundle_digest, artifact) = + bundle_bytes_declaring(&[("AGENTS.md", "# named\n", 0o644)], Some("instruction")); + let artifact_path = target.join("..").join("components.zip"); + fs::write(&artifact_path, &bytes).unwrap(); + let flags = bundle_flags(&artifact_path, &bundle_digest, &artifact, bytes.len()); + + let mut plan_args = vec![ + "--operation".to_owned(), + "install".to_owned(), + "--provider-release-digest".to_owned(), + RELEASE.to_owned(), + "--operation-id".to_owned(), + "operation_01COMPONENT".to_owned(), + "--expires-at".to_owned(), + far_future().to_owned(), + ]; + plan_args.extend(flags.clone()); + let borrowed: Vec<&str> = plan_args.iter().map(String::as_str).collect(); + let planned = run(args("plan-operation", &target, &borrowed)); + let plan_path = target.join("..").join("components-plan.json"); + fs::write( + &plan_path, + setup_core::canonical::to_canonical_bytes(&planned["plan"]).unwrap(), + ) + .unwrap(); + let mut apply_args = vec![ + "--plan".to_owned(), + plan_path.to_string_lossy().into_owned(), + "--plan-digest".to_owned(), + planned["plan_digest"].as_str().unwrap().to_owned(), + "--provider-release-digest".to_owned(), + RELEASE.to_owned(), + ]; + apply_args.extend(flags); + let borrowed: Vec<&str> = apply_args.iter().map(String::as_str).collect(); + assert_eq!( + run(args("apply-operation", &target, &borrowed))["state"], + "verified" + ); + + let state: serde_json::Value = + serde_json::from_str(&fs::read_to_string(target.join(TEST.state_file)).unwrap()) + .unwrap(); + assert_eq!( + state["component_refs"], + serde_json::json!(["component_00000000000000000000000000"]) + ); + // The passport is a required member and was required and discarded, so + // a target configured from a bundle recorded no setup identity and no + // setup version at all. + assert_eq!(state["setup_stable_id"], "setup_00000000000000000000000000"); + assert_eq!(state["setup_version"], "3.1.0"); + // Still null, and deliberately: the passport does not state its own + // digest and the contract does not define how one is taken. + assert!(state["setup_version_passport_digest"].is_null()); + } + + #[test] + fn a_setup_from_the_local_catalog_records_no_components_because_it_has_none() { + // A catalog setup is a tree of files with an id and a description. It + // carries no component identities, and inventing some would be worse + // than the empty list the contract allows. + let target = seeded("catalog-components"); + plan_then_apply(&target, "backup", &[]); + let state: serde_json::Value = + serde_json::from_str(&fs::read_to_string(target.join(TEST.state_file)).unwrap()) + .unwrap(); + assert_eq!(state["component_refs"], serde_json::json!([])); + } + #[test] fn a_bundle_installs_over_the_wire_and_leaves_unowned_files_alone() { let target = seeded("bundle-install"); @@ -2397,6 +2530,70 @@ mod tests { ); } + #[test] + fn an_update_of_nothing_is_refused_rather_than_quietly_installing() { + // Two names for one act is what this had been: install and update + // produced byte-identical plans. An update of nothing is a request that + // cannot be honoured as asked, and installing instead would be doing + // something else and calling it done. + let target = seeded("software-update-empty"); + let prefix = ready_prefix(&target); + let error = refuse(args( + "plan-operation", + &target, + &software_plan_args("software_update", &prefix), + )); + assert!( + error.detail().contains("software_install is the operation"), + "{}", + error.detail() + ); + } + + #[test] + fn a_plan_says_what_is_already_under_the_prefix() { + let target = seeded("software-plan-present"); + let file = downloaded(&target, TEST_PAYLOAD); + plan_then_install(&target, "software_install", Some(&file)); + + // Installing the pinned version again says so, rather than reading + // exactly like the first install did. + let planned = software_plan(&target, "software_install"); + let effects = planned["effects"].as_array().unwrap(); + assert!( + effects[0].as_str().unwrap().contains("already installed"), + "{effects:?}" + ); + + // And an update is now a different plan from an install, because there + // is something to update. + let updating = software_plan(&target, "software_update"); + assert_eq!(updating["state"], "planned"); + } + + #[test] + fn a_remove_names_the_versions_it_leaves_behind() { + // This build pins one version and cannot know whether an older tree is + // still wanted. Leaving it is right; leaving it silently is not. + let target = seeded("software-remove-others"); + let file = downloaded(&target, TEST_PAYLOAD); + plan_then_install(&target, "software_install", Some(&file)); + let prefix = ready_prefix(&target); + fs::create_dir_all(Path::new(&prefix).join("0.9.0")).unwrap(); + + let planned = software_plan(&target, "software_remove"); + let effects: Vec<&str> = planned["effects"] + .as_array() + .unwrap() + .iter() + .map(|effect| effect.as_str().unwrap()) + .collect(); + assert!( + effects.iter().any(|effect| effect.contains("leave 0.9.0")), + "{effects:?}" + ); + } + #[test] fn installing_software_spends_no_backup_slot() { // Ten slots exist and they hold configuration. If a software install diff --git a/crates/provider-v3/src/bundle.rs b/crates/provider-v3/src/bundle.rs index 51c4ce6..97f2609 100644 --- a/crates/provider-v3/src/bundle.rs +++ b/crates/provider-v3/src/bundle.rs @@ -251,10 +251,35 @@ impl Manifest { pub struct Bundle { /// The manifest as read. pub manifest: Manifest, + /// What the setup passport says the setup is. + pub passport: SetupPassport, /// Each declared file's bytes and mode, by target-relative path. pub files: BTreeMap, u32)>, } +/// The two things the setup passport states that a provider must record. +/// +/// `setup-passport.json` is a required member and was required and then +/// discarded, so a target configured from a bundle recorded no setup identity +/// and no setup version at all -- both are provenance fields the contract names, +/// and both were null for every bundle install. +/// +/// Only what the passport *states* is copied. Its own digest is not computed +/// here: the passport does not carry one and the contract does not define how +/// it is taken, so a value produced here would be this program's opinion rather +/// than the passport's identity, and that is worse than the null the field is +/// allowed to hold. Everything else the passport carries -- tags, permissions, +/// component version references, evidence links -- belongs to the consumer. +#[derive(Debug, Clone, Default, Deserialize, PartialEq, Eq)] +pub struct SetupPassport { + /// The setup's stable identity. + #[serde(default)] + pub stable_id: String, + /// The setup version this bundle was built from. + #[serde(default)] + pub version: String, +} + /// What the caller claimed about the bytes it sent. #[derive(Debug, Clone, Copy)] pub struct Claim<'a> { @@ -346,7 +371,21 @@ impl Bundle { )); } - Ok(Self { manifest, files }) + // The passport is required to be the second member and was, until now, + // required and then discarded. What it states about the setup is the + // only source for two provenance fields, and a malformed one is not a + // reason to refuse a bundle whose files and digests all hold — so it is + // read leniently and its absence of content reads as no content. + let passport = members + .get(1) + .and_then(|member| serde_json::from_slice::(&member.data).ok()) + .unwrap_or_default(); + + Ok(Self { + manifest, + passport, + files, + }) } } diff --git a/crates/setup-core/src/backup.rs b/crates/setup-core/src/backup.rs index d35547a..f0df676 100644 --- a/crates/setup-core/src/backup.rs +++ b/crates/setup-core/src/backup.rs @@ -540,7 +540,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-backup-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-backup-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/setup-core/src/digest.rs b/crates/setup-core/src/digest.rs index a665ce3..844a75a 100644 --- a/crates/setup-core/src/digest.rs +++ b/crates/setup-core/src/digest.rs @@ -269,7 +269,8 @@ mod tests { use super::*; fn scratch(name: &str) -> std::path::PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-digest-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-digest-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/setup-core/src/journal.rs b/crates/setup-core/src/journal.rs index c20401a..c105f69 100644 --- a/crates/setup-core/src/journal.rs +++ b/crates/setup-core/src/journal.rs @@ -234,7 +234,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-journal-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-journal-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/setup-core/src/lock.rs b/crates/setup-core/src/lock.rs index f1d6a6e..90cc499 100644 --- a/crates/setup-core/src/lock.rs +++ b/crates/setup-core/src/lock.rs @@ -250,7 +250,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-lock-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-lock-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/setup-core/src/software.rs b/crates/setup-core/src/software.rs index c004ca6..e63cdc7 100644 --- a/crates/setup-core/src/software.rs +++ b/crates/setup-core/src/software.rs @@ -93,6 +93,72 @@ pub struct Software { pub unsupported: &'static [&'static str], } +/// What is already under a program directory. +/// +/// Read at plan time, which is allowed to look at the local disk and is not +/// allowed to reach the network. Without it `software_install` and +/// `software_update` produced byte-identical plans — two names for one act, and +/// neither of them said what was about to be replaced. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct Present { + /// Every version directory found, sorted. + pub versions: Vec, + /// The version the exposed command currently resolves into, when it does. + pub exposed: Option, +} + +impl Present { + /// Whether this build's pinned version is one of the ones already there. + #[must_use] + pub fn holds(&self, version: &str) -> bool { + self.versions.iter().any(|found| found == version) + } + + /// Read what a program directory holds. + /// + /// A missing or unreadable directory reads as empty rather than failing: a + /// plan for a prefix that does not exist yet is exactly the ordinary case. + #[must_use] + pub fn under(root: &Path, command: &str) -> Self { + let mut versions: Vec = fs::read_dir(root) + .into_iter() + .flatten() + .flatten() + .filter(|entry| entry.path().is_dir()) + .filter_map(|entry| entry.file_name().into_string().ok()) + // `bin` holds the exposed command and a dotted directory holds this + // provider's own bookkeeping. Neither is a version. + .filter(|name| name != "bin" && !name.starts_with('.')) + .collect(); + versions.sort(); + + // The link points into the version directory it was installed from, so + // the version is the component right under the root. + // + // Resolved rather than read. `expose` writes an absolute target, which + // a plain `strip_prefix` handles — but a link someone rewrote by hand + // as `..//` does not: its first component is `bin`, + // and the answer becomes "no entry point names this version" while the + // entry point names it. Resolving costs one syscall and is right for + // both. A dangling link resolves to nothing, which is also correct: + // nothing usable is exposed. + let link = root.join("bin").join(command); + let exposed = fs::canonicalize(&link) + .ok() + .zip(fs::canonicalize(root).ok()) + .and_then(|(to, base)| { + to.strip_prefix(&base).ok().and_then(|rest| { + rest.components() + .next() + .map(|first| first.as_os_str().to_string_lossy().into_owned()) + }) + }) + .filter(|name| versions.contains(name)); + + Self { versions, exposed } + } +} + /// What an install produced. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Installed { @@ -578,6 +644,81 @@ mod tests { fs::remove_dir_all(&at).unwrap(); } + #[test] + fn an_empty_or_absent_prefix_reads_as_holding_nothing() { + // A plan for a prefix that does not exist yet is the ordinary first + // case, not a failure. + let nowhere = scratch("present-absent"); + assert_eq!(Present::under(&nowhere, "codex"), Present::default()); + } + + #[test] + fn what_is_under_a_prefix_is_read_including_which_version_is_exposed() { + let (at, artifact) = staged("present-read", b"payload", CODEX_MEMBER); + let root = at.join("software"); + install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + + // A second version, installed but not exposed. + fs::create_dir_all(root.join("9.9.9")).unwrap(); + + let found = Present::under(&root, "codex"); + assert_eq!(found.versions, vec!["1.2.3".to_owned(), "9.9.9".to_owned()]); + assert!(found.holds("1.2.3")); + assert!(!found.holds("0.0.1")); + fs::remove_dir_all(&at).unwrap(); + } + + #[test] + #[cfg(unix)] + fn the_exposed_version_is_read_through_the_link_however_it_was_written() { + let (at, artifact) = staged("present-link", b"payload", CODEX_MEMBER); + let root = at.join("software"); + install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + let link = root.join("bin").join("codex"); + + // What `expose` writes: an absolute target. + assert!(fs::read_link(&link).unwrap().is_absolute()); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + + // What a person might write instead. Taking the first component of + // `../1.2.3/...` without resolving it gives `bin`, and the answer + // becomes "no entry point names this version" while one does. + fs::remove_file(&link).unwrap(); + std::os::unix::fs::symlink( + std::path::Path::new("..").join("1.2.3").join(CODEX_MEMBER), + &link, + ) + .unwrap(); + assert!(!fs::read_link(&link).unwrap().is_absolute()); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + + // A dangling link exposes nothing, which is what it means. + fs::remove_file(&link).unwrap(); + std::os::unix::fs::symlink(root.join("9.9.9").join("codex"), &link).unwrap(); + assert_eq!(Present::under(&root, "codex").exposed, None); + + fs::remove_dir_all(&at).unwrap(); + } + + #[test] + fn bin_and_the_control_directory_are_not_versions() { + let at = scratch("present-notversions"); + fs::create_dir_all(at.join("bin")).unwrap(); + fs::create_dir_all(at.join(".codex-setup-system")).unwrap(); + fs::create_dir_all(at.join("1.2.3")).unwrap(); + assert_eq!( + Present::under(&at, "codex").versions, + vec!["1.2.3".to_owned()] + ); + fs::remove_dir_all(&at).unwrap(); + } + #[test] fn the_inflation_limit_scales_with_the_artifact_but_never_below_a_floor() { let small = Artifact { diff --git a/crates/setup-core/src/stamp.rs b/crates/setup-core/src/stamp.rs index 7bf5234..a719970 100644 --- a/crates/setup-core/src/stamp.rs +++ b/crates/setup-core/src/stamp.rs @@ -230,7 +230,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-stamp-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-stamp-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); base diff --git a/crates/setup-core/src/target.rs b/crates/setup-core/src/target.rs index 9c0335e..b07ce86 100644 --- a/crates/setup-core/src/target.rs +++ b/crates/setup-core/src/target.rs @@ -180,7 +180,8 @@ mod tests { use super::*; fn scratch(name: &str) -> PathBuf { - let base = std::env::temp_dir().join(format!("setup-core-target-{name}")); + let base = + std::env::temp_dir().join(format!("setup-core-target-{name}-{}", std::process::id())); let _ = fs::remove_dir_all(&base); fs::create_dir_all(&base).unwrap(); fs::canonicalize(&base).unwrap() From 13d442a040f56e10d51c36f89d5f29505f0aeb5c Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 26 Aug 2026 02:11:28 +0500 Subject: [PATCH 2/2] test: a file that was busy because this test forked while writing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linux refuses to exec a file any process holds open for writing. The harness runs these in threads and `Command::output` forks: between the fork and the child's exec the child holds a copy of a write handle another thread is about to close, and a thread exec'ing that file in that window is told it is busy. The code under test does not have this shape — `launch` is its own invocation reading a file some earlier one wrote — so the retry is in the two tests that write and exec inside one multi-threaded process. --- crates/harness-runtime/src/wire.rs | 42 +++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/crates/harness-runtime/src/wire.rs b/crates/harness-runtime/src/wire.rs index 726d04b..b0546c2 100644 --- a/crates/harness-runtime/src/wire.rs +++ b/crates/harness-runtime/src/wire.rs @@ -2229,10 +2229,9 @@ mod tests { #[cfg(unix)] { - let output = std::process::Command::new(&exposed) - .env(TEST.config_home_env, &target) - .output() - .unwrap(); + let output = run_once_it_is_not_busy( + std::process::Command::new(&exposed).env(TEST.config_home_env, &target), + ); assert_eq!( String::from_utf8_lossy(&output.stdout).trim(), "test-harness 1.2.3" @@ -2514,6 +2513,35 @@ mod tests { ); } + /// Run something that was just written, tolerating a fork that has not + /// finished exec'ing yet. + /// + /// Linux refuses to exec a file any process holds open for writing, with + /// `ETXTBSY`. The test harness runs these in threads, and + /// `Command::output` forks: between the fork and the child's exec, the + /// child holds a copy of every descriptor its parent had, including a write + /// handle another thread is about to close. A thread exec'ing that file in + /// exactly that window is told it is busy. + /// + /// The window is microseconds and closes on its own, which is why this + /// failed on one CI runner out of seven and passes locally every time. It + /// is a property of writing and exec'ing in one multi-threaded process, and + /// this program does neither: `launch` is its own invocation, reading a + /// file some earlier invocation wrote. So the retry belongs here, in the + /// test that creates the condition, and not in the code under test. + #[cfg(unix)] + fn run_once_it_is_not_busy(command: &mut std::process::Command) -> std::process::Output { + for _ in 0..50 { + match command.output() { + Err(error) if error.kind() == std::io::ErrorKind::ExecutableFileBusy => { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + other => return other.unwrap(), + } + } + panic!("it stayed busy for a second, which is longer than the fork race lasts"); + } + #[test] #[cfg(unix)] fn what_was_installed_actually_runs() { @@ -2521,9 +2549,9 @@ mod tests { let file = downloaded(&target, TEST_PAYLOAD); let applied = plan_then_install(&target, "software_install", Some(&file)); - let output = std::process::Command::new(applied["executable"].as_str().unwrap()) - .output() - .unwrap(); + let output = run_once_it_is_not_busy(&mut std::process::Command::new( + applied["executable"].as_str().unwrap(), + )); assert_eq!( String::from_utf8_lossy(&output.stdout).trim(), "test-harness 1.2.3"