mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 00:02:23 +00:00
fix(core): only the machine's own instance inherits the legacy tree
The migration moved `machine.json` into whichever config directory started first. In the very setup this change exists to fix — a default install beside a `--config-dir` one — that is the second instance renaming the machine's tree into its own directory, leaving the primary to come up owning nothing. It also fired in our own test suite, where `routed_pane` and friends launch a real `tty7-server --config-dir <TempDir>` under the developer's own `HOME`. Adoption is now the entitlement of the instance running out of the config directory this machine resolves to on its own: `$TTY7_CONFIG_DIR` where the box names one, `$HOME`'s otherwise. Comparing paths rather than asking whether `--config-dir` was passed is what keeps the ordinary install working — `spawn` hands every daemon it starts an explicit `--config-dir`, its own included — and counting `$TTY7_CONFIG_DIR` is what keeps remote hosts upgrading, since a remote `tty7-server` is launched without the flag and finds its directory that way. Also tightens the cross-filesystem fallback: a rename that failed because another process already carried the file over is the one benign race, not an error to report and not something to copy over. What is left copies through `create_new`, so "never overwrite what is already there" holds against a racing writer and not merely against an `exists` check several syscalls old, and a write that does not finish leaves nothing behind. Tests: the gate both ways, the appearance hint riding along, the same directory under two names, the copy path refusing an occupied destination, and two cross-process cases in `machine_tree` that start a real server under a scratch `HOME` — one carrying the legacy tree in, one leaving it alone.
This commit is contained in:
@@ -668,6 +668,19 @@ fn config_dir() -> Option<PathBuf> {
|
||||
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<PathBuf> {
|
||||
if let Some(dir) = std::env::var_os("TTY7_CONFIG_DIR").filter(|d| !d.is_empty()) {
|
||||
return Some(PathBuf::from(dir));
|
||||
}
|
||||
|
||||
@@ -1349,20 +1349,101 @@ fn legacy_data_dir() -> Option<PathBuf> {
|
||||
/// 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 <TempDir>` 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<PathBuf> {
|
||||
@@ -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();
|
||||
|
||||
@@ -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"))
|
||||
|
||||
Reference in New Issue
Block a user