diff --git a/crates/fractal/src/fleet_cli.rs b/crates/fractal/src/fleet_cli.rs index 5f2983233dd..3888d1e749d 100644 --- a/crates/fractal/src/fleet_cli.rs +++ b/crates/fractal/src/fleet_cli.rs @@ -127,18 +127,11 @@ fn read_code_without_echo() -> Result { const FABRIC_PAIRING_UNAVAILABLE: &str = "Fabric invitations need st-fabric-adapter installed and \ Fabric running with --runtime-peers; use --via tailscale for a Tailscale-only invitation"; -/// The pairing helper beside st, named by ST_FABRIC_ADAPTER, or on PATH. A configured -/// path that is not a file counts as no helper, so `auto` invitations route around it. -fn fabric_adapter() -> Option { - if let Some(path) = std::env::var_os("ST_FABRIC_ADAPTER") { - let path = PathBuf::from(path); - return path.is_file().then_some(path); - } - std::env::current_exe() - .ok() - .and_then(|path| path.parent().map(|parent| parent.join("st-fabric-adapter"))) - .filter(|path| path.is_file()) - .or_else(|| fractal_daemon::fleet::transport::resolve_tool(None, "st-fabric-adapter")) +/// A configured path that is not a file counts as no helper, so `auto` invitations +/// route around it. Conflicting environment overrides are errors, not absence. +fn fabric_adapter() -> Result> { + Ok(fractal_daemon::fleet::transport::resolve_fabric_adapter()? + .filter(|path| path.is_file())) } /// The `--via` an invitation is created with. Without the pairing helper, a Fabric endpoint @@ -611,7 +604,7 @@ pub(super) async fn run_fleet(endpoint: &Endpoint, command: FleetCommand, json_o } else { None }; - let adapter = fabric_adapter(); + let adapter = fabric_adapter()?; let mut via = args.via.clone(); if adapter.is_none() && !args.migrate { via = invite_via(&via, &member_endpoints(&client, &config.node).await?, false)?; diff --git a/crates/smallclaims/src/fleet/join.rs b/crates/smallclaims/src/fleet/join.rs index e34aa7e648b..f914a1af940 100644 --- a/crates/smallclaims/src/fleet/join.rs +++ b/crates/smallclaims/src/fleet/join.rs @@ -19,8 +19,8 @@ use super::file::{FleetFile, FleetMode}; use super::handshake::{JoinRequest, JoinResponse, Joiner, SealedJoin, WriterHead, verify_response}; use super::keys::MemberKey; use super::transport::{ - Fabric, Route, bindable_tailnet_addresses, local_addresses, parse_route, resolve_tool, - tailscale_addresses, + Fabric, Route, bindable_tailnet_addresses, local_addresses, parse_route, resolve_fabric_adapter, + resolve_tool, tailscale_addresses, }; use crate::error::Error; use crate::store::{AdmittedStore, Runtime, Store, valid_fleet_node_name}; @@ -528,15 +528,11 @@ impl PairingHelper { fn launch(command: &str, executable: Option<&Path>) -> Result { use std::process::Stdio; use tokio::io::AsyncBufReadExt as _; - let executable = executable.map(Path::to_path_buf) - .or_else(|| std::env::var_os("ST_FABRIC_ADAPTER").map(PathBuf::from)) - .or_else(|| { - std::env::current_exe().ok() - .and_then(|exe| exe.parent().map(|parent| parent.join("st-fabric-adapter"))) - .filter(|path| path.is_file()) - }) - .or_else(|| resolve_tool(None, "st-fabric-adapter")) - .context("st-fabric-adapter is unavailable; install it alongside st or set ST_FABRIC_ADAPTER")?; + let executable = match executable { + Some(path) => Some(path.to_path_buf()), + None => resolve_fabric_adapter()?, + } + .context("st-fabric-adapter is unavailable; install it alongside st or set ST_FABRIC_ADAPTER")?; let mut child = tokio::process::Command::new(executable) .arg(command) .stdin(Stdio::piped()) diff --git a/crates/smallclaims/src/fleet/transport.rs b/crates/smallclaims/src/fleet/transport.rs index dd6fc412b08..08367cc1738 100644 --- a/crates/smallclaims/src/fleet/transport.rs +++ b/crates/smallclaims/src/fleet/transport.rs @@ -104,6 +104,49 @@ pub fn resolve_tool(override_path: Option<&Path>, name: &str) -> Option }) } +/// Find the pairing helper: new then legacy environment, beside this executable, then +/// each PATH directory. Within each directory the new executable name wins. +/// Configured paths are returned even if absent, so callers retain their absence policy. +pub fn resolve_fabric_adapter() -> Result> { + let executable = std::env::current_exe().ok(); + let paths = std::env::var_os("PATH"); + discover_fabric_adapter( + std::env::var_os("FRACTAL_FABRIC_ADAPTER").as_deref(), + std::env::var_os("ST_FABRIC_ADAPTER").as_deref(), + executable.as_deref().and_then(Path::parent), + paths.as_deref(), + ) +} + +fn discover_fabric_adapter( + new_env: Option<&std::ffi::OsStr>, + old_env: Option<&std::ffi::OsStr>, + beside: Option<&Path>, + paths: Option<&std::ffi::OsStr>, +) -> Result> { + if let (Some(new), Some(old)) = (new_env, old_env) { + anyhow::ensure!( + new == old, + "FRACTAL_FABRIC_ADAPTER and ST_FABRIC_ADAPTER name different paths; \ + unset one or set both to the same path" + ); + } + if let Some(path) = new_env.or(old_env) { + return Ok(Some(PathBuf::from(path))); + } + let in_directory = |directory: &Path| { + ["fractal-fabric-adapter", "st-fabric-adapter"] + .into_iter() + .map(|name| directory.join(name)) + .find(|candidate| candidate.is_file()) + }; + Ok(beside.and_then(in_directory).or_else(|| { + paths.and_then(|paths| { + std::env::split_paths(paths).find_map(|directory| in_directory(&directory)) + }) + })) +} + #[derive(Debug)] struct CommandFailure(String); @@ -1268,6 +1311,65 @@ mod tests { assert!(relative.dial_socket("node-b", "p").await.is_err()); } + #[test] + fn fabric_adapter_environment_precedence_and_conflict() { + let new = std::ffi::OsStr::new("new-adapter"); + let old = std::ffi::OsStr::new("old-adapter"); + for (new_env, old_env, expected) in [ + (Some(new), None, new), + (None, Some(old), old), + (Some(new), Some(new), new), + ] { + assert_eq!( + discover_fabric_adapter(new_env, old_env, Some(Path::new(".")), None).unwrap(), + Some(PathBuf::from(expected)) + ); + } + let error = discover_fabric_adapter(Some(new), Some(old), None, None).unwrap_err(); + assert!(error.to_string().contains("FRACTAL_FABRIC_ADAPTER and ST_FABRIC_ADAPTER")); + assert!(error.to_string().contains("different paths")); + } + + #[test] + fn fabric_adapter_location_and_name_precedence() { + let root = tempfile::tempdir().unwrap(); + let directories = ["beside", "first", "second"].map(|name| root.path().join(name)); + for directory in &directories { + std::fs::create_dir(directory).unwrap(); + } + let paths = std::env::join_paths(&directories[1..]).unwrap(); + let candidates: Vec<_> = directories.iter().flat_map(|directory| { + ["fractal-fabric-adapter", "st-fabric-adapter"] + .map(|name| directory.join(name)) + }).collect(); + for candidate in &candidates { + std::fs::write(candidate, "").unwrap(); + } + for (new, old) in [(Some("configured"), None), (None, Some("configured"))] { + assert_eq!( + discover_fabric_adapter( + new.map(std::ffi::OsStr::new), + old.map(std::ffi::OsStr::new), + Some(&directories[0]), + Some(&paths), + ).unwrap(), + Some(PathBuf::from("configured")) + ); + } + for expected in &candidates { + // Removing each winner proves both names at every location and that a + // legacy name in an earlier directory beats a new name later on. + assert_eq!( + discover_fabric_adapter(None, None, Some(&directories[0]), Some(&paths)).unwrap(), + Some(expected.clone()) + ); + std::fs::remove_file(expected).unwrap(); + } + let absent = discover_fabric_adapter(None, None, Some(&directories[0]), Some(&paths)) + .unwrap(); + assert!(absent.is_none()); + } + #[test] fn an_override_is_used_without_searching_path() { let override_path = Path::new("/nonexistent/shim/fabric"); diff --git a/release-notes/fabric-adapter-discovery-readers.json b/release-notes/fabric-adapter-discovery-readers.json new file mode 100644 index 00000000000..ee0df730582 --- /dev/null +++ b/release-notes/fabric-adapter-discovery-readers.json @@ -0,0 +1,11 @@ +{ + "version": 1, + "summary": "Fabric pairing accepts new and legacy adapter executable and environment names, refusing conflicting environment overrides.", + "replay": { "status": "none", "detail": "No claims or projections change." }, + "schema": { "status": "none", "detail": "No stored formats change." }, + "rules": { "status": "none", "detail": "Pairing trust and authorization are unchanged." }, + "compatibility": { "status": "changed", "detail": "Discovery accepts FRACTAL_FABRIC_ADAPTER then ST_FABRIC_ADAPTER. Different paths in both variables are an error. Otherwise it searches beside the executable then each PATH directory, preferring fractal-fabric-adapter over st-fabric-adapter within each directory. The produced binary remains st-fabric-adapter." }, + "downtime": { "status": "none", "detail": "No runtime migration is required." }, + "manual": { "status": "none", "detail": "Existing installations continue working. If both overrides are set, use the same path or unset one." }, + "recovery": { "status": "none", "detail": "Rollback retains support for the legacy executable and environment variable." } +}