diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index 50a40989..7dc3805c 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -668,6 +668,19 @@ fn config_dir() -> Option { if let Some(dir) = CONFIG_DIR_OVERRIDE.get() { return Some(dir.clone()); } + machine_config_dir() +} + +/// The config directory this machine resolves to when no single invocation +/// redirects it — `$TTY7_CONFIG_DIR` where the box names one, the default under +/// `$HOME` otherwise. +/// +/// [`config_dir`] with the `--config-dir` override left off, which is the +/// question "is this the machine's tty7 or a second one somebody pointed +/// elsewhere" (see `machine::adopt_legacy_data_dir`). It cannot be answered by +/// whether the override is set: `daemon::spawn` passes `--config-dir` to every +/// daemon it starts, the ordinary install's included. +pub fn machine_config_dir() -> Option { if let Some(dir) = std::env::var_os("TTY7_CONFIG_DIR").filter(|d| !d.is_empty()) { return Some(PathBuf::from(dir)); } diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index d2984cb7..92dc2f9b 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -1349,20 +1349,101 @@ fn legacy_data_dir() -> Option { /// stop. /// /// Called by the daemon at startup, before it opens the store, and by nothing -/// else. The daemon is the tree's only writer, so it is the only process -/// entitled to move it; and a path getter that touches the disk as a side -/// effect is one no test can call without putting the developer's own tree at -/// risk — which is the accident this whole change is about. +/// else. The daemon is the tree's writer, so it is the process entitled to move +/// it; and a path getter that touches the disk as a side effect is one no test +/// can call without putting the developer's own tree at risk — which is the +/// accident this whole change is about. +/// +/// The appearance hint rides along, and it does have a second writer: the GUI +/// records it whenever it applies a theme ([`note_appearance`]). Both writers +/// land their file with a rename, so the worst a collision costs is one of two +/// booleans, and the next theme the GUI applies overwrites it either way. pub fn adopt_legacy_data_dir() { - let Ok(current) = data_dir() else { return }; - let Some(legacy) = legacy_data_dir().filter(|legacy| *legacy != current) else { + adopt_into( + data_dir().ok().as_deref(), + legacy_data_dir().as_deref(), + this_is_the_machines_instance(), + ); +} + +/// [`adopt_legacy_data_dir`] with every process-wide answer already looked up, +/// so a test can state the situation instead of arranging one. +fn adopt_into(current: Option<&Path>, legacy: Option<&Path>, machines_instance: bool) { + let (Some(current), Some(legacy)) = (current, legacy) else { return; }; + // The same directory under two names is not a migration. This is also what + // makes `TTY7_DATA_DIR` a no-op: both getters answer with it. + if legacy == current { + return; + } + if !machines_instance { + decline_legacy(legacy); + return; + } for file in [MACHINE_FILE, APPEARANCE_FILE] { adopt_legacy_file(&legacy.join(file), ¤t.join(file)); } } +/// Whether this process is the machine's tty7 rather than an instance somebody +/// pointed somewhere else. +/// +/// There is one legacy tree and there can be any number of instances, so "who +/// inherits it" has to have exactly one answer, and it cannot be "whoever +/// starts first". Every build before this change read the tree that +/// [`legacy_data_dir`] names, so the instance entitled to it is the one still +/// running out of the config directory this machine resolves to on its own +/// ([`config::machine_config_dir`](crate::core::config::machine_config_dir)) — +/// `$TTY7_CONFIG_DIR` when the box sets one, `$HOME`'s otherwise. Anything +/// aimed elsewhere is a second tty7 by definition, and a second tty7 starts on +/// an empty tree. +/// +/// Comparing paths rather than asking whether `--config-dir` was passed is what +/// makes the ordinary install work: [`daemon::spawn`](crate::daemon::spawn) +/// hands the daemon an explicit `--config-dir` every time, its own resolved +/// directory included, so "was the flag given" is true for everybody and would +/// decline for everybody. +/// +/// Deciding by start order instead would let a second instance rename the tree +/// out from under the first — this bug wearing a different hat, the primary +/// coming up owning nothing and every workspace with no window on it gone. It +/// would also fire in our own test suite, where `routed_pane` and friends +/// launch a real `tty7-server --config-dir ` under the developer's own +/// `HOME`: a start-order rule moves the developer's real `machine.json` into a +/// scratch directory and deletes it with the `TempDir`. +/// +/// Counting `$TTY7_CONFIG_DIR` as the machine's is what keeps remote hosts +/// upgrading: a remote `tty7-server` is launched with no `--config-dir` and +/// finds its config directory exactly this way (see +/// `daemon::remote_link::remote_control_socket`), so a rule written against +/// `$HOME` alone would strand the tree on every box that names one. +fn this_is_the_machines_instance() -> bool { + is_the_machines_instance( + crate::core::config::config_dir_path().as_deref(), + crate::core::config::machine_config_dir().as_deref(), + ) +} + +fn is_the_machines_instance(current: Option<&Path>, machines: Option<&Path>) -> bool { + matches!((current, machines), (Some(c), Some(m)) if c == m) +} + +/// Say where the tree was left when this instance is not the one entitled to +/// take it, so "my workspaces are gone" has an answer in the log rather than +/// only in this comment. +fn decline_legacy(legacy: &Path) { + let file = legacy.join(MACHINE_FILE); + if !file.exists() { + return; + } + log::info!( + "leaving {} where it is: this instance runs on a config directory of its own, and \ + {LEGACY_NOTE}. Move it in by hand if this is the instance that should have it.", + file.display() + ); +} + /// The destination existing at all is the whole guard. It means some newer run /// already owns this file, and the legacy copy beside it is stale — a build /// from before this change, started once since the move, writing where it still @@ -1370,7 +1451,8 @@ pub fn adopt_legacy_data_dir() { /// /// Two processes migrating at once is safe for the same reason they cannot both /// win: they rename the same source, so the loser's rename fails with the -/// source already gone and leaves the winner's result alone. +/// source already gone — see [`adopt_by_copy`], which tells that apart from a +/// rename that failed with something still to carry over. fn adopt_legacy_file(legacy: &Path, current: &Path) { if current.exists() || !legacy.exists() { return; @@ -1391,24 +1473,81 @@ fn adopt_legacy_file(legacy: &Path, current: &Path) { legacy.display(), current.display() ), - // Across filesystems `rename` refuses and a copy is the only way over. - // The original stays: it costs a stale file, and losing the tree to a - // half-finished move is the one outcome worth ruling out. - Err(_) => match std::fs::copy(legacy, current) { - Ok(_) => log::info!( - "copied {} to {} ({LEGACY_NOTE}); the original was left in place", - legacy.display(), - current.display() - ), - Err(e) => log::warn!( - "could not move {} to {}: {e}", - legacy.display(), - current.display() - ), - }, + Err(e) => adopt_by_copy(legacy, current, &e), } } +/// The rename did not go through. Two reasons to tell apart, because only one +/// of them is a problem. +/// +/// The source being gone is the concurrent-migration race: another process +/// renamed it away between the check above and here, and its result is the one +/// this call wanted anyway. Reporting that as a failure — or copying over it — +/// would turn the one benign race into noise in the log. +/// +/// Otherwise the usual reason is that the two directories are on different +/// filesystems, where `rename` refuses and a copy is the only way over. Writing +/// it with `create_new` keeps "never overwrite what is already there" true +/// against a racing writer and not merely against the `exists` check, which by +/// now is several syscalls stale. The original stays: it costs a file nobody +/// reads, and losing the tree to a half-finished move is the one outcome worth +/// ruling out. +fn adopt_by_copy(legacy: &Path, current: &Path, why: &io::Error) { + if !legacy.exists() { + log::debug!( + "{} was carried over by another process ({why})", + legacy.display() + ); + return; + } + match copy_new(legacy, current) { + Ok(()) => log::info!( + "copied {} to {} ({LEGACY_NOTE}); the original was left in place", + legacy.display(), + current.display() + ), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => log::debug!( + "{} already has one; leaving {} where it is", + current.display(), + legacy.display() + ), + Err(e) => log::warn!( + "could not move {} to {} ({why}), nor copy it: {e}", + legacy.display(), + current.display() + ), + } +} + +/// Copy `from` onto a `to` that must not already exist, leaving nothing behind +/// if the write does not finish: a truncated tree is a tree, and the next +/// startup would adopt nothing over it. +/// +/// Read whole rather than streamed so a failure lands before the destination is +/// created, and private because that is how both files are written +/// ([`crate::core::config::write_atomic_private`]) — `fs::copy` would carry the +/// legacy mode over, which is the same thing only as long as the legacy file +/// was ours. +fn copy_new(from: &Path, to: &Path) -> io::Result<()> { + use std::io::Write as _; + + let bytes = std::fs::read(from)?; + let mut open = std::fs::OpenOptions::new(); + open.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + open.mode(0o600); + } + let mut file = open.open(to)?; + if let Err(e) = file.write_all(&bytes).and_then(|()| file.sync_all()) { + drop(file); + let _ = std::fs::remove_file(to); + return Err(e); + } + Ok(()) +} + const LEGACY_NOTE: &str = "the machine tree now lives beside the rest of the config directory"; fn env_dir(key: &str) -> Option { @@ -2096,14 +2235,119 @@ mod tests { if std::env::var_os(DATA_DIR_ENV).is_some() { return; } - let pinned = crate::core::session::test_support::pin_config_dir(); + // Against whatever the config directory resolves to, not against the + // path this call passed in: `set_config_dir` is first-wins and + // process-wide, so the pin only guarantees *a* scratch directory is in + // force — which test put it there depends on the order they ran in. + let _ = crate::core::session::test_support::pin_config_dir(); + let config_dir = crate::core::config::config_dir_path(); + assert!(config_dir.is_some(), "the pin puts one in force"); assert_eq!( default_machine_path().unwrap().parent(), - Some(pinned.as_path()), + config_dir.as_deref(), "a config directory nobody else names must not share anybody's tree" ); } + /// The other half of "one config directory is one instance": there is one + /// legacy tree, so exactly one instance may inherit it, and start order + /// must not be what picks. A second tty7 renaming the tree into its own + /// directory leaves the primary owning nothing — the same bug, one upgrade + /// later. + #[test] + fn only_the_machines_own_instance_inherits_the_legacy_tree() { + let machines = PathBuf::from("/home/u/.config/tty7"); + assert!(is_the_machines_instance( + Some(&machines), + Some(&machines.clone()) + )); + assert!( + !is_the_machines_instance(Some(Path::new("/tmp/scratch")), Some(&machines)), + "an instance pointed somewhere of its own starts on an empty tree" + ); + assert!( + !is_the_machines_instance(None, Some(&machines)), + "nowhere to put it is not a licence to move it" + ); + assert!(!is_the_machines_instance(Some(&machines), None)); + } + + #[test] + fn an_instance_of_its_own_leaves_the_machines_tree_where_it_is() { + let old = tempfile::TempDir::new().unwrap(); + let new = tempfile::TempDir::new().unwrap(); + let legacy = old.path().join(MACHINE_FILE); + std::fs::write(&legacy, br#"{"workspaces":[],"panes":[]}"#).unwrap(); + + adopt_into(Some(new.path()), Some(old.path()), false); + + assert!( + legacy.exists(), + "a --config-dir instance that takes the tree hands the primary an \ + empty one, which is the failure this change exists to stop" + ); + assert!(!new.path().join(MACHINE_FILE).exists()); + } + + #[test] + fn an_upgrade_carries_the_appearance_hint_along_with_the_tree() { + let old = tempfile::TempDir::new().unwrap(); + let new = tempfile::TempDir::new().unwrap(); + for file in [MACHINE_FILE, APPEARANCE_FILE] { + std::fs::write(old.path().join(file), b"{}").unwrap(); + } + + adopt_into(Some(new.path()), Some(old.path()), true); + + for file in [MACHINE_FILE, APPEARANCE_FILE] { + assert!(new.path().join(file).exists(), "{file} was left behind"); + assert!(!old.path().join(file).exists(), "{file} was not moved"); + } + } + + /// `TTY7_DATA_DIR` answers both getters, so the sandboxes every test + /// harness pins with it must come out the far side untouched. + #[test] + fn one_directory_under_two_names_is_not_a_migration() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join(MACHINE_FILE); + std::fs::write(&file, b"live").unwrap(); + + adopt_into(Some(dir.path()), Some(dir.path()), true); + + assert_eq!(std::fs::read(&file).unwrap(), b"live"); + } + + /// The cross-filesystem path, where `rename` refuses and copying is the + /// only way over. It may add a file and it may not replace one. + #[test] + fn a_copied_tree_never_lands_on_one_already_there() { + let dir = tempfile::TempDir::new().unwrap(); + let (from, to) = (dir.path().join("from"), dir.path().join("to")); + std::fs::write(&from, b"carried").unwrap(); + + copy_new(&from, &to).expect("nothing is there yet"); + assert_eq!(std::fs::read(&to).unwrap(), b"carried"); + + let second = copy_new(&from, &to).expect_err("the second must not replace the first"); + assert_eq!(second.kind(), std::io::ErrorKind::AlreadyExists); + assert_eq!(std::fs::read(&to).unwrap(), b"carried"); + } + + /// A rename that failed because somebody else already carried the file + /// over is the one benign race, and it must not turn into a copy — there + /// is nothing left to copy, and the winner's file is what was wanted. + #[test] + fn a_source_already_carried_over_is_not_an_error() { + let dir = tempfile::TempDir::new().unwrap(); + let (legacy, current) = (dir.path().join("legacy"), dir.path().join("current")); + std::fs::write(¤t, b"the winner's").unwrap(); + + adopt_by_copy(&legacy, ¤t, &io::Error::from(io::ErrorKind::NotFound)); + + assert_eq!(std::fs::read(¤t).unwrap(), b"the winner's"); + } + #[test] fn an_upgrade_carries_the_legacy_tree_forward() { let old = tempfile::TempDir::new().unwrap(); diff --git a/crates/tty7-server/tests/machine_tree.rs b/crates/tty7-server/tests/machine_tree.rs index 9e1d5a72..57f16b45 100644 --- a/crates/tty7-server/tests/machine_tree.rs +++ b/crates/tty7-server/tests/machine_tree.rs @@ -452,6 +452,95 @@ fn attachment_rides_the_tree_when_no_record_store_is_served() { ); } +/// Start a server the way an upgraded install starts, and let it exit. +/// +/// `HOME` is a scratch directory of the test's own, which is the whole reason +/// this case can exist: the legacy path is derived from `HOME`, so a test that +/// borrowed the developer's would be reaching for their real `machine.json`. +/// stdin is closed, so the link is at EOF before it is read — the startup work +/// this test is about has already run by then, and the process leaves rather +/// than serving nothing. +fn started_once(home: &Path, extra: &[&str]) { + let mut child = Command::new(env!("CARGO_BIN_EXE_tty7-server")) + .args(["--stdio", "--serve"]) + .args(extra) + .env("HOME", home) + .env_remove("TTY7_DATA_DIR") + .env_remove("TTY7_CONFIG_DIR") + .env_remove("XDG_DATA_HOME") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("could not start tty7-server --stdio --serve"); + + let deadline = Instant::now() + Duration::from_secs(10); + loop { + match child.try_wait().expect("waiting on tty7-server") { + Some(_) => return, + None if Instant::now() >= deadline => { + let _ = child.kill(); + let _ = child.wait(); + panic!("tty7-server kept running with its link at EOF"); + } + None => std::thread::sleep(Duration::from_millis(20)), + } + } +} + +fn seed_legacy_tree(home: &Path) -> PathBuf { + let legacy = home.join(".local").join("share").join("tty7"); + std::fs::create_dir_all(&legacy).unwrap(); + let file = legacy.join(MACHINE_FILE); + std::fs::write(&file, br#"{"workspaces":[],"panes":[]}"#).unwrap(); + file +} + +/// The upgrade, end to end: the daemon moves the tree the old build left in the +/// data directory into the config directory before it opens the store, so a +/// machine's workspaces survive the release that changed where they live. +#[test] +fn a_server_started_after_the_upgrade_carries_the_legacy_tree_in() { + let home = tempfile::TempDir::new().unwrap(); + let legacy = seed_legacy_tree(home.path()); + + started_once(home.path(), &[]); + + assert!( + home.path().join(".config/tty7").join(MACHINE_FILE).exists(), + "the tree must arrive beside the rest of the config directory" + ); + assert!( + !legacy.exists(), + "a move leaves nothing to be adopted twice" + ); +} + +/// And the half that keeps the upgrade from becoming the bug: a second tty7 on +/// a config directory of its own must not rename the machine's tree into it. +/// Whichever instance happened to start first would otherwise decide, and the +/// primary would come up owning nothing. +#[test] +fn a_config_dir_of_its_own_leaves_the_machines_tree_alone() { + let home = tempfile::TempDir::new().unwrap(); + let other = tempfile::TempDir::new().unwrap(); + let legacy = seed_legacy_tree(home.path()); + + started_once( + home.path(), + &["--config-dir", &other.path().to_string_lossy()], + ); + + assert!( + legacy.exists(), + "the machine's tree belongs to the instance running out of its config directory" + ); + assert!( + !other.path().join(MACHINE_FILE).exists(), + "a second instance starts on an empty tree, not on somebody else's" + ); +} + fn bridged(sock: &Path, token: &str) -> Client { let hello = ControlHello::host_rpc(token, token); let mut child = Command::new(env!("CARGO_BIN_EXE_tty7-server"))