From 4b7719ba5f6681238274e201639cffc3d472fc76 Mon Sep 17 00:00:00 2001 From: l0ng-ai Date: Wed, 12 Aug 2026 01:00:52 +0800 Subject: [PATCH] fix(watch): stop a filesystem watch feeding itself on Linux (#523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Linux, an idle window with a repository open ran `git status -uall` about 2.6 times a second, forever. `notify`'s inotify backend subscribes with `WatchMask::OPEN`, so every `open(2)` under a watched directory is an event — and `git status` opens `.git/index`, `.git/HEAD` and `refs/heads/*`, all of which the source control watch covers. The read that answers "did this repository change" was itself an event saying it may have changed, so each answer scheduled the next question. Measured on the Linux runner: 78 debounce bursts in 30 seconds, from one real change. `Debounce`'s doc argued this was impossible because `GIT_OPTIONAL_LOCKS=0` stops `git status` writing the index back. That covers writes; `IN_OPEN` fires on reads. macOS FSEvents has no equivalent, which is why it was invisible on the machine it was written on. `is_content_change` drops `Access(Open | Read | Close(Read))` and keeps `Access(Close(Write))`, at both the host watch and the config hot-reload watch. Guarded by a host conformance case rather than a platform test, verified red on the Linux runner with the filter removed. Also here, and how the above was found: the `render_idle` tests counted frames across a window they advance a virtual clock over, while the pane runs its own git pipeline off a 300ms timer on that same clock that nothing advanced during settle — so the measurement set off the pane's first `git` run and raced its landing. `test_window::quiesce` settles both clocks, and the kernel's, before counting. That turned #523 from a 1-in-50 flake into a deterministic failure, which is what made the watch loop findable. Third: `ssh_config::an_alias_added_to_an_included_file_is_seen_without_touching_the_root` (from #528, already on main) failed on every Windows run of this branch. The cache keys on mtime and the test writes twice inside one ~15ms Windows clock tick, so the cache was right to say nothing changed. Test-only; the poll behind it runs at 4Hz and real edits arrive at human speed. Closes #523. --- CHANGELOG.md | 10 +++ crates/tty7-core/src/host/conformance.rs | 42 ++++++++++++ crates/tty7-core/src/host/local.rs | 1 + crates/tty7-core/src/host/mod.rs | 26 ++++++++ src/core/ssh_config.rs | 16 +++++ src/main.rs | 5 ++ src/terminal/git_data.rs | 17 +++++ src/ui/app.rs | 82 ++++++++++++++++++++++++ src/ui/file_tree.rs | 7 ++ src/ui/scm/graph.rs | 7 ++ src/ui/scm/panel.rs | 8 ++- 11 files changed, 220 insertions(+), 1 deletion(-) 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();