diff --git a/CHANGELOG.md b/CHANGELOG.md index 0df54713..d79628cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **An alias put back in an `Include`d ssh config file is found again** — the + check behind a parked remote workspace watched `~/.ssh/config` for changes + and nothing else, but `Include` is common and editing an included file + leaves the including file's timestamp exactly where it was. So re-adding a + `Host` block where it used to live changed nothing: the workspaces on that + alias stayed parked, with no retry and no error, saying a new profile would + find them again — which is what the user had just done. Every file the parse + reads is watched now, and the root config even when it could not be read, so + one appearing later is noticed too. + - **A rejected stored credential asks again instead of failing forever** — a key passphrase saved with "remember" was written to the keychain before the daemon had tried it, and a wrong one then ended every later connection at "could not diff --git a/crates/tty7-core/src/daemon/singleton.rs b/crates/tty7-core/src/daemon/singleton.rs index b3d82b9c..a8018498 100644 --- a/crates/tty7-core/src/daemon/singleton.rs +++ b/crates/tty7-core/src/daemon/singleton.rs @@ -201,14 +201,44 @@ mod tests { let dir = std::env::temp_dir().join(format!("tty7-singleton-{}-{name}", std::process::id())); std::fs::create_dir_all(&dir).ok(); - crate::core::config::set_config_dir(dir.clone()); - (dir, guard) + crate::core::config::set_config_dir(dir); + // `set_config_dir` is first-wins, so the directory in force may be one + // an earlier test in this process pinned. Hand back the one the seat + // will actually be taken in — a case that writes to the directory it + // asked for instead would be testing a file nothing ever reads. + let path = lock_path().expect("a config directory is pinned"); + let dir = path.parent().expect("the lock lives in a directory"); + (dir.to_path_buf(), guard) } + /// [`claim`], allowing for a seat some neighbour is still holding a + /// reference to. + /// + /// A `Command::spawn` anywhere in this process forks every descriptor this + /// one has open, and BSD `flock` counts an inherited descriptor as another + /// reference to the same lock rather than a second lock — so between a + /// neighbour's `fork` and its `exec`, a seat this thread has already + /// dropped still reads as taken. The suite forks constantly. What these + /// cases are about is that the seat comes back, not the microsecond it + /// comes back in. + fn claim_within(patience: std::time::Duration) -> Claim { + let deadline = std::time::Instant::now() + patience; + loop { + match claim() { + Claim::Taken if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + other => return other, + } + } + } + + const PATIENCE: std::time::Duration = std::time::Duration::from_secs(10); + #[test] fn the_second_claim_is_refused_while_the_first_is_held() { let _pinned = pin_dir("basic"); - let first = match claim() { + let first = match claim_within(PATIENCE) { Claim::Held(s) => s, other => panic!("the first claim must be granted, got {other:?}"), }; @@ -218,11 +248,43 @@ mod tests { ); drop(first); assert!( - matches!(claim(), Claim::Held(_)), + matches!(claim_within(PATIENCE), Claim::Held(_)), "releasing it hands the seat to the next server — no stale file to interpret" ); } + /// A neighbour that forks while the seat is held keeps the seat alive past + /// the holder's `drop`, and this suite forks constantly. + #[cfg(unix)] + #[test] + fn a_reference_a_forking_neighbour_left_behind_does_not_lose_the_seat() { + let _pinned = pin_dir("inherited"); + let seat = match claim_within(PATIENCE) { + Claim::Held(s) => s, + other => panic!("the first claim must be granted, got {other:?}"), + }; + // What a `Command::spawn` on another thread does to this descriptor + // between `fork` and `exec`, done deterministically: BSD `flock` counts + // an inherited descriptor as a second reference to one lock, not a + // second lock, so the seat is not free the instant this process drops + // it. + let inherited = unsafe { libc::dup(held_fd().expect("the seat records its descriptor")) }; + assert!(inherited >= 0, "dup the seat descriptor"); + drop(seat); + let released = std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(50)); + unsafe { libc::close(inherited) }; + }); + let again = claim_within(PATIENCE); + // Joined before the assertion so a failure leaves nothing referencing + // the seat for the next case to trip over. + released.join().expect("the reference is let go"); + assert!( + matches!(again, Claim::Held(_)), + "the seat comes back once the last reference to it is gone, got {again:?}" + ); + } + #[test] fn a_lock_left_behind_by_a_dead_holder_is_claimable() { let (dir, _guard) = pin_dir("stale"); @@ -232,7 +294,7 @@ mod tests { // in reverse. std::fs::write(&path, b"").unwrap(); assert!( - matches!(claim(), Claim::Held(_)), + matches!(claim_within(PATIENCE), Claim::Held(_)), "an unlocked file is not a holder, however it came to exist" ); } diff --git a/crates/tty7-server/tests/pane_history.rs b/crates/tty7-server/tests/pane_history.rs index 5f767998..d00f1a89 100644 --- a/crates/tty7-server/tests/pane_history.rs +++ b/crates/tty7-server/tests/pane_history.rs @@ -168,10 +168,18 @@ fn a_pane_writes_its_own_history_and_hands_it_back_when_it_closes() { .expect("the shell takes input"); collect_until(&mut session, b"tty7_ran_here"); - // Seeded from the user's file, so the pane is not starting blank. + // Seeded from the user's file, so the pane is not starting blank. Waited + // for by its contents rather than its name: the snippet seeds with a + // redirection, which creates the file before `tail` has written a byte + // into it, and a pane caught in that window reads as one that lost the + // user's history. wait_until( - || daemon.pane_history(pane_id).is_some(), - "the pane never got a history file of its own", + || { + daemon + .pane_history(pane_id) + .is_some_and(|h| h.contains("echo from_before")) + }, + "the pane never got a history file of its own, seeded from the user's", ); let seeded = daemon.pane_history(pane_id).unwrap(); assert!( @@ -214,6 +222,45 @@ fn a_pane_writes_its_own_history_and_hands_it_back_when_it_closes() { ); } +/// The snippet seeds with `tail … > "$TTY7_HISTFILE"`, and a redirection +/// creates the file before the command that fills it writes a byte. So the +/// pane's file existing does not mean the pane's history exists, and anything +/// that reads it on the strength of the name being there can read an empty one +/// — the pane looking, for that moment, exactly like a pane that lost the +/// user's history. +/// +/// The window is a `tail` wide, which is why this drives it with a `HISTFILE` +/// that takes a known second to produce its bytes rather than hoping to land +/// inside it. +#[test] +fn a_pane_whose_seed_is_still_copying_is_not_an_empty_history() { + let daemon = Daemon::with_home( + "mkfifo \"$HOME/slow_history\" 2>/dev/null\n\ + HISTFILE=\"$HOME/slow_history\"\n\ + ( sleep 1; printf 'echo from_before\\n' > \"$HOME/slow_history\" ) &\n", + "echo from_before\n", + ); + let panes = daemon.panes(); + let session = panes + .spawn(None, size(), Some(bash()), None, None) + .expect("spawn a bash pane"); + let pane_id = session.pane_id(); + + wait_until( + || { + daemon + .pane_history(pane_id) + .is_some_and(|h| h.contains("echo from_before")) + }, + "the pane never got a history file of its own, seeded from the user's", + ); + let seeded = daemon.pane_history(pane_id).unwrap(); + assert!( + seeded.contains("echo from_before"), + "a pane whose history starts empty has lost the user their history; got {seeded:?}" + ); +} + /// A user with more history than their caps allow — which is most users, since /// bash defaults both to 500. The exit rewrite keeps only the last /// HISTSIZE/HISTFILESIZE lines, so without the integration snippet raising the diff --git a/src/core/ssh_config.rs b/src/core/ssh_config.rs index 6f0bf8da..36756010 100644 --- a/src/core/ssh_config.rs +++ b/src/core/ssh_config.rs @@ -237,7 +237,7 @@ pub fn import_report_from(root: PathBuf, home: &Path) -> ImportReport { ignored: ignored_options(&parsed.blocks), source: root, source_read: parsed.root_read, - files_read: parsed.files_read, + files_read: parsed.files.len(), } } @@ -307,32 +307,75 @@ fn alias_resolves_in(blocks: &[HostBlock], alias: &str) -> bool { /// be the only file IO on that hot path, so the parse is cached by mtime: /// an edit made outside tty7 is seen on the first tick after the file /// changes, and an untouched file costs one stat. -pub fn alias_still_resolves(alias: &str) -> bool { - struct Cache { - mtime: Option, - blocks: Vec, +/// The parse behind [`alias_still_resolves`], kept only as long as none of +/// the files it came from has moved. +/// +/// Watching `~/.ssh/config` alone was not enough (#525): `Include` is common, +/// and editing an included file leaves the including file's mtime exactly +/// where it was — so an alias put back where it used to live went on reading +/// as gone, and its workspaces stayed parked with nothing to say why. The +/// stamps cover every file the parse read, and the root even when it could +/// not be read, so a config that appears later is noticed too. +/// +/// Not covered: a brand-new *filename* arriving in a glob include that +/// matched it before. Catching that means watching the directories a glob +/// resolves through, which is a wider net than this needs. +#[derive(Default)] +struct AliasCache { + stamps: Vec<(PathBuf, Option)>, + blocks: Vec, + parsed: bool, +} + +impl AliasCache { + fn resolves(&mut self, root: &Path, home: &Path, alias: &str) -> bool { + if !self.parsed || self.stamps != stamps_of(self.sources(root)) { + let parsed = parse_config(root, home); + let mut sources = parsed.files; + if !sources.iter().any(|p| p == root) { + sources.push(root.to_path_buf()); + } + self.stamps = stamps_of(sources); + self.blocks = parsed.blocks; + self.parsed = true; + } + alias_resolves_in(&self.blocks, alias) } - static CACHE: std::sync::Mutex> = std::sync::Mutex::new(None); + + fn sources(&self, root: &Path) -> Vec { + match self.stamps.is_empty() { + true => vec![root.to_path_buf()], + false => self.stamps.iter().map(|(p, _)| p.clone()).collect(), + } + } +} + +/// A file that cannot be stat'ed stamps as `None`, which is a value like any +/// other: a config that appears, or disappears, moves the stamp either way. +fn stamps_of(paths: Vec) -> Vec<(PathBuf, Option)> { + paths + .into_iter() + .map(|path| { + let mtime = std::fs::metadata(&path).and_then(|m| m.modified()).ok(); + (path, mtime) + }) + .collect() +} + +pub fn alias_still_resolves(alias: &str) -> bool { + static CACHE: std::sync::Mutex> = std::sync::Mutex::new(None); let Some(home) = home_dir() else { return false; }; let root = home.join(".ssh/config"); - let mtime = std::fs::metadata(&root).and_then(|m| m.modified()).ok(); let mut cache = match CACHE.lock() { Ok(c) => c, Err(_) => return false, }; - if cache.as_ref().is_none_or(|c| c.mtime != mtime) { - *cache = Some(Cache { - mtime, - blocks: parse_config(&root, &home).blocks, - }); - } - match cache.as_ref() { - Some(c) => alias_resolves_in(&c.blocks, alias), - None => false, - } + cache + .get_or_insert_with(AliasCache::default) + .resolves(&root, &home, alias) } fn apply_resolved(profile: &mut ManagedProfile, alias: &str, r: ResolvedHost) -> Option { @@ -491,18 +534,22 @@ struct ResolvedHost { struct ParsedConfig { blocks: Vec, root_read: bool, - files_read: usize, + /// Every file the parse actually read, root and includes alike. A count + /// is enough to report an import; watching for a change needs the paths + /// (#525), because an edit to an included file leaves the file that + /// included it untouched. + files: Vec, } fn parse_config(root: &Path, home: &Path) -> ParsedConfig { let mut blocks = Vec::new(); let mut seen = HashSet::new(); - let mut files_read = 0; - let root_read = parse_config_file(root, home, 0, &mut blocks, &mut seen, &mut files_read); + let mut files = Vec::new(); + let root_read = parse_config_file(root, home, 0, &mut blocks, &mut seen, &mut files); ParsedConfig { blocks, root_read, - files_read, + files, } } @@ -515,7 +562,7 @@ fn parse_config_file( depth: usize, blocks: &mut Vec, seen: &mut HashSet, - files_read: &mut usize, + files: &mut Vec, ) -> bool { if depth > MAX_INCLUDE_DEPTH || seen.len() >= MAX_CONFIG_FILES { return false; @@ -527,7 +574,7 @@ fn parse_config_file( let Ok(text) = std::fs::read_to_string(&path) else { return false; }; - *files_read += 1; + files.push(path.clone()); let base = path.parent().unwrap_or(home).to_path_buf(); let mut current: Option = None; @@ -569,7 +616,7 @@ fn parse_config_file( } for token in split_words(rest) { for include in expand_include(&token, &base, home) { - parse_config_file(&include, home, depth + 1, blocks, seen, files_read); + parse_config_file(&include, home, depth + 1, blocks, seen, files); } } } else if !in_match { @@ -1334,6 +1381,55 @@ mod tests { assert!(report.ignored.is_empty()); } + #[test] + fn a_parse_reports_every_file_it_read_not_just_how_many() { + // What the alias cache has to watch (#525): an edit to an included + // file changes nothing about the file that included it, so a cache + // holding one mtime cannot see it. It can only stat what the parse + // says it read. + let root = temp_root("include-stamps"); + let ssh = root.join(".ssh"); + std::fs::create_dir_all(ssh.join("conf.d")).unwrap(); + std::fs::write(ssh.join("config"), "Include conf.d/*\nHost root\n").unwrap(); + std::fs::write(ssh.join("conf.d/work"), "Host work\n").unwrap(); + + let parsed = parse_config(&ssh.join("config"), &root); + let mut files = parsed.files.clone(); + files.sort(); + assert_eq!( + files, + vec![ssh.join("conf.d/work"), ssh.join("config")], + "the included file is as much a source as the root" + ); + } + + #[test] + fn an_alias_added_to_an_included_file_is_seen_without_touching_the_root() { + // The recovery direction of #525, which is the one that strands a + // user: the alias is gone, its workspaces park, the user puts it back + // in the file where it lived — and nothing about `~/.ssh/config` + // changed, so a root-only cache keeps answering "still gone". + let root = temp_root("include-recovery"); + let ssh = root.join(".ssh"); + std::fs::create_dir_all(ssh.join("conf.d")).unwrap(); + std::fs::write(ssh.join("config"), "Include conf.d/*\nHost root\n").unwrap(); + std::fs::write(ssh.join("conf.d/work"), "Host elsewhere\n").unwrap(); + + let cfg = ssh.join("config"); + let mut cache = AliasCache::default(); + assert!( + !cache.resolves(&cfg, &root, "work"), + "the alias is not in the config yet" + ); + + std::fs::write(ssh.join("conf.d/work"), "Host work\n").unwrap(); + assert!( + cache.resolves(&cfg, &root, "work"), + "an alias put back in an included file is found again, \ + though the root config never changed" + ); + } + fn temp_root(name: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!( "tty7-ssh-config-test-{name}-{}",