diff --git a/CHANGELOG.md b/CHANGELOG.md index 21636b5e..ae1d8d0e 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 idle window stops re-reading the repository on Linux** — with a + repository open, tty7 ran `git status -uall` about two and a half times a + second, forever, with nothing on screen changing and nobody touching the + machine. The filesystem watch was feeding itself: Linux reports opening a + file as a watch event, every read of `.git` is an open, and reading `.git` + is exactly how the watch's own events were answered — so each answer + scheduled the next question. Reads are no longer mistaken for changes; + a write finishing still is. macOS and Windows were never affected, which is + why this survived so long. + - **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 diff --git a/crates/tty7-core/src/host/conformance.rs b/crates/tty7-core/src/host/conformance.rs index 0328ae66..f165277b 100644 --- a/crates/tty7-core/src/host/conformance.rs +++ b/crates/tty7-core/src/host/conformance.rs @@ -66,6 +66,7 @@ macro_rules! for_each_host_case { search_respects_max_dirs, shells_are_named_and_have_a_default, watch_reports_create_and_delete, + watch_ignores_reads, watch_is_non_recursive, watch_set_dirs_adds_and_drops, watch_coalesces_within_window, @@ -946,6 +947,47 @@ pub fn watch_reports_create_and_delete(h: &dyn Host, sb: &dyn Sandbox) { ); } +/// Reading a watched file is not a change. +/// +/// The one case where a watch can feed itself: every consumer answers an event +/// by reading the directory it came from, so if a read is an event, the answer +/// asks the question again. `notify`'s inotify backend subscribes to `IN_OPEN`, +/// which makes this reachable on Linux and only there — an idle Source Control +/// panel ran `git status -uall` about 2.6 times a second before this was +/// filtered (issue #523). macOS passes this trivially, which is exactly why it +/// has to be a conformance case rather than a platform test. +pub fn watch_ignores_reads(h: &dyn Host, sb: &dyn Sandbox) { + let sandbox = sb.path(); + let f = h.join(sandbox, "read-me.txt"); + write(h, &f, "hello"); + + let sub = h.watch(&[sandbox.to_path_buf()]).unwrap(); + // The file this asserts about is created *before* the watch, so the drain + // has to come after the tail of that create can still arrive — FSEvents + // will hand one over a beat late, and a stale create is indistinguishable + // here from the reads below reporting. + std::thread::sleep(WATCH_QUIET); + drain(&sub); + + for _ in 0..5 { + h.read_file(&f, 1 << 20) + .expect("the watched file reads back"); + } + std::thread::sleep(WATCH_QUIET); + + let batches = collect_batches(&sub, Duration::from_millis(200)); + let leaked: Vec<&PathBuf> = batches + .iter() + .flatten() + .filter(|p| p.file_name().is_some_and(|n| n == "read-me.txt")) + .collect(); + assert!( + leaked.is_empty(), + "reading a watched file reported as a change, so a watch can feed \ + itself: {leaked:?}" + ); +} + pub fn watch_is_non_recursive(h: &dyn Host, sb: &dyn Sandbox) { let sandbox = sb.path(); let sub_dir = h.join(sandbox, "child"); diff --git a/crates/tty7-core/src/host/local.rs b/crates/tty7-core/src/host/local.rs index be391fa7..6288037e 100644 --- a/crates/tty7-core/src/host/local.rs +++ b/crates/tty7-core/src/host/local.rs @@ -381,6 +381,7 @@ fn local_watch(dirs: &[PathBuf], gitignore: Arc>) -> io::R let watcher = notify::recommended_watcher(move |res: notify::Result| { if let Ok(ev) = res && !ev.paths.is_empty() + && crate::host::is_content_change(&ev.kind) { let _ = raw_tx.send(ev.paths); } diff --git a/crates/tty7-core/src/host/mod.rs b/crates/tty7-core/src/host/mod.rs index 58552739..dcb0a724 100644 --- a/crates/tty7-core/src/host/mod.rs +++ b/crates/tty7-core/src/host/mod.rs @@ -262,6 +262,32 @@ pub trait Host: Send + Sync + 'static { } } +/// Did this watch event mean something *changed*, or only that something was +/// read? +/// +/// The distinction does not exist on macOS, and on Linux it decides whether a +/// watch can feed itself. `notify`'s inotify backend asks for `IN_OPEN`, so +/// every `open(2)` under a watched directory arrives as an event — and the one +/// thing every consumer of a watch does with an event is go and read the +/// directory it came from. Reading `.git/index` to answer "did the repository +/// change" is itself an event saying the repository may have changed, so the +/// answer schedules the next question, at whatever rate the debounce allows, +/// forever. Measured at ~2.6 `git status -uall` per second on an idle Source +/// Control panel before this filter existed (issue #523). +/// +/// `IN_CLOSE_WRITE` also arrives as `Access`, and that one is a real write +/// finishing, so it stays. Everything else under `Access` is a reader. +pub fn is_content_change(kind: ¬ify::EventKind) -> bool { + use notify::event::{AccessKind, AccessMode}; + + !matches!( + kind, + notify::EventKind::Access( + AccessKind::Open(_) | AccessKind::Read | AccessKind::Close(AccessMode::Read), + ) + ) +} + pub fn default_join(dir: &Path, name: &str, sep: char) -> PathBuf { let mut s = dir.to_string_lossy().into_owned(); if !s.is_empty() && !s.ends_with(sep) && !s.ends_with('/') { diff --git a/src/core/ssh_config.rs b/src/core/ssh_config.rs index 36756010..c98b452f 100644 --- a/src/core/ssh_config.rs +++ b/src/core/ssh_config.rs @@ -1423,6 +1423,7 @@ mod tests { ); std::fs::write(ssh.join("conf.d/work"), "Host work\n").unwrap(); + edited_just_now(&ssh.join("conf.d/work")); assert!( cache.resolves(&cfg, &root, "work"), "an alias put back in an included file is found again, \ @@ -1430,6 +1431,21 @@ mod tests { ); } + /// Make a write that has just happened look like one, on a filesystem + /// whose clock is coarser than this test is fast. + /// + /// Windows stamps files from a clock that ticks about every 15ms, so two + /// writes as close together as the ones above can share an mtime — and a + /// cache keyed on mtime is then right to say nothing changed. A real edit + /// arrives at human speed, and the poll behind this cache runs at 4Hz, so + /// the collision is an artefact of the test rather than something a user + /// can reach. Stated by hand instead of slept out. + fn edited_just_now(path: &Path) { + let f = std::fs::File::options().write(true).open(path).unwrap(); + let ahead = std::time::SystemTime::now() + std::time::Duration::from_secs(1); + f.set_modified(ahead).unwrap(); + } + fn temp_root(name: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!( "tty7-ssh-config-test-{name}-{}", diff --git a/src/main.rs b/src/main.rs index aeff7d86..00b3e809 100644 --- a/src/main.rs +++ b/src/main.rs @@ -43,6 +43,11 @@ fn spawn_config_watcher(cx: &mut App) { let watched_file = config_file.clone(); let handler = move |res: notify::Result| { let Ok(event) = res else { return }; + // A reload re-reads the file, and on Linux reading it is itself an + // event — see `tty7_core::host::is_content_change`. + if !tty7_core::host::is_content_change(&event.kind) { + return; + } let hit = event .paths .iter() diff --git a/src/terminal/git_data.rs b/src/terminal/git_data.rs index 3b7e8793..53f241ed 100644 --- a/src/terminal/git_data.rs +++ b/src/terminal/git_data.rs @@ -166,6 +166,12 @@ impl Debounce { self.seq } + /// Is an event still waiting for its probe? + #[cfg(test)] + pub fn is_open(&self) -> bool { + self.opened.is_some() + } + /// What the timer should do now. `Fire` closes the burst, so it is /// returned exactly once however many events went into it. pub fn poll(&mut self, now: Instant) -> DebounceStep { @@ -489,6 +495,17 @@ impl ScmData { self.subs.is_empty() && self.watches.is_empty() } + /// Is any repository still inside a debounce window? + /// + /// The window is measured on the real clock, and closing it costs a + /// frame. A test driving a virtual clock has no other way to tell a panel + /// that has gone quiet from one whose next repaint is merely still owed — + /// see `ui::app::test_window::quiesce`. + #[cfg(test)] + pub fn is_debouncing(&self) -> bool { + self.watches.values().any(|watch| watch.debounce.is_open()) + } + /// Repositories that have a holder but no watch, nothing on the way, and /// no failed attempt still resting. fn unwatched(&self, now: Instant) -> Vec<(HostId, PathBuf)> { diff --git a/src/ui/app.rs b/src/ui/app.rs index 4396a4ff..723a6742 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -7853,6 +7853,88 @@ pub(crate) mod test_window { vcx.background_executor.run_until_parked(); (app, vcx, stream) } + + /// Wait until the window has actually stopped drawing — which is not the + /// same as having reached the state a test was waiting for. + /// + /// Called both after a settle and at the top of `draws_while_idle`: a test + /// that reaches its state, asserts a few things about it and only then + /// measures has given the setup more time to land, but not necessarily + /// enough, and the measurement is the place that cannot afford to be + /// wrong. + /// + /// Both of a `render_idle` test's clocks have to be pumped here, and they + /// are pumped differently. + /// + /// The pane runs its own git pipeline, separate from whatever panel is on + /// screen, and it hangs off a 300ms timer on the *virtual* clock — so it + /// never starts at all unless a test advances that clock. It used to be + /// `draws_while_idle`'s own `advance_clock` that started it, which put the + /// pane's first real `git` run, and the repaint it lands with, inside the + /// window being counted. Whether that repaint arrived before or after the + /// count then came down to how fast git ran, which is why these tests were + /// green here and red on a loaded CI runner (issue #523). + /// + /// What that repaint sets off in turn is timed on the *real* clock: the + /// landing opens a `GIT_WATCH_DEBOUNCE` burst, and closing the burst costs + /// another frame 250ms later. So the sleep below is load-bearing too, and + /// a round that drew nothing is not on its own enough to stop on — a burst + /// still open is a frame already owed. + #[cfg(unix)] + pub(crate) fn quiesce(vcx: &mut VisualTestContext, cwd: Option<&std::path::Path>) { + use crate::terminal::git_data::ScmData; + use crate::terminal::git_status::GitStatusCache; + use crate::ui::app::render_probe; + use crate::ui::host_ops::HostId; + + /// How long quiet has to hold before it counts as quiet. + /// + /// Real time, and the only defence against the third clock in play: + /// the kernel's. The file tree keeps a real `inotify`/`FSEvents` + /// watch, and the writes a test makes while setting up its repository + /// are still being delivered long after every future the test can wait + /// on has resolved. They arrive on the channel, sit in a 200ms debounce + /// on the virtual clock, and are released by the next `advance_clock` + /// — which, without this, was the measurement's own. Any delivery + /// restarts the hold, so the wait is as long as the runner needs and + /// no longer. + const QUIET_HOLD: std::time::Duration = std::time::Duration::from_millis(400); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + let mut quiet_since: Option = None; + loop { + render_probe::arm(u64::MAX); + vcx.executor() + .advance_clock(std::time::Duration::from_millis(300)); + vcx.background_executor.run_until_parked(); + let quiet = render_probe::draws() == 0 + && vcx.update(|_, cx| { + let owed = cx + .try_global::() + .is_some_and(ScmData::is_debouncing); + let answered = cwd.is_none_or(|cwd| { + cx.try_global::() + .and_then(|cache| cache.known_repo_for(HostId::LOCAL, cwd)) + .is_some() + }); + !owed && answered + }); + match quiet { + false => quiet_since = None, + true => { + let since = *quiet_since.get_or_insert_with(std::time::Instant::now); + if since.elapsed() >= QUIET_HOLD { + return; + } + } + } + assert!( + std::time::Instant::now() < deadline, + "the window never stopped drawing" + ); + std::thread::sleep(std::time::Duration::from_millis(20)); + } + } } #[cfg(all(test, unix))] diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index 86b26d23..eaa7beeb 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -2801,6 +2801,7 @@ mod render_idle_gpui_tests { std::thread::sleep(std::time::Duration::from_millis(20)); } vcx.background_executor.run_until_parked(); + test_window::quiesce(&mut vcx, Some(root)); (app, vcx, pane) } @@ -2887,12 +2888,18 @@ mod render_idle_gpui_tests { } fn draws_while_idle(vcx: &mut VisualTestContext) -> u64 { + test_window::quiesce(vcx, None); render_probe::arm(BUDGET); vcx.background_executor.run_until_parked(); vcx.executor() .advance_clock(std::time::Duration::from_secs(3)); vcx.background_executor.run_until_parked(); render_probe::arm(BUDGET); + // No real-time exposure in the counted window, deliberately. The file + // tree holds a real filesystem watch, so real time is a channel input + // arrives on — and a test that spends it here is asking to be handed + // some. What has to be waited out is waited out in `quiesce` above, + // where a frame costs nothing. vcx.executor() .advance_clock(std::time::Duration::from_secs(9)); vcx.background_executor.run_until_parked(); diff --git a/src/ui/scm/graph.rs b/src/ui/scm/graph.rs index 52e3c097..bea86035 100644 --- a/src/ui/scm/graph.rs +++ b/src/ui/scm/graph.rs @@ -2212,12 +2212,18 @@ mod render_idle_gpui_tests { } fn draws_while_idle(vcx: &mut VisualTestContext) -> u64 { + test_window::quiesce(vcx, None); render_probe::arm(BUDGET); vcx.background_executor.run_until_parked(); vcx.executor() .advance_clock(std::time::Duration::from_secs(3)); vcx.background_executor.run_until_parked(); render_probe::arm(BUDGET); + // No real-time exposure in the counted window, deliberately. The file + // tree holds a real filesystem watch, so real time is a channel input + // arrives on — and a test that spends it here is asking to be handed + // some. What has to be waited out is waited out in `quiesce` above, + // where a frame costs nothing. vcx.executor() .advance_clock(std::time::Duration::from_secs(9)); vcx.background_executor.run_until_parked(); @@ -2234,6 +2240,7 @@ mod render_idle_gpui_tests { let page = app.update_in(vcx, |app, _, _| app.scm.graph.page.clone()); if page.is_some() { vcx.background_executor.run_until_parked(); + test_window::quiesce(vcx, None); return page; } if std::time::Instant::now() >= deadline { diff --git a/src/ui/scm/panel.rs b/src/ui/scm/panel.rs index adbcbe1d..309580c7 100644 --- a/src/ui/scm/panel.rs +++ b/src/ui/scm/panel.rs @@ -2712,17 +2712,23 @@ mod render_idle_gpui_tests { ); std::thread::sleep(std::time::Duration::from_millis(20)); } - vcx.background_executor.run_until_parked(); + test_window::quiesce(&mut vcx, Some(root)); (app, vcx, pane) } fn draws_while_idle(vcx: &mut VisualTestContext) -> u64 { + test_window::quiesce(vcx, None); render_probe::arm(BUDGET); vcx.background_executor.run_until_parked(); vcx.executor() .advance_clock(std::time::Duration::from_secs(3)); vcx.background_executor.run_until_parked(); render_probe::arm(BUDGET); + // No real-time exposure in the counted window, deliberately. The file + // tree holds a real filesystem watch, so real time is a channel input + // arrives on — and a test that spends it here is asking to be handed + // some. What has to be waited out is waited out in `quiesce` above, + // where a frame costs nothing. vcx.executor() .advance_clock(std::time::Duration::from_secs(9)); vcx.background_executor.run_until_parked();