From 0b5614652fe9493c3856c2c5a07c77c5fc72c674 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:57:50 +0200 Subject: [PATCH 01/17] mason: bound descriptors retained by bash task history Store verified directory identity snapshots instead of open handles in both artifact-owner and bundle-read caches. Reopen with existing no-follow checks per access; keep running task pins unchanged. Release adopted-process exit monitors at terminal cleanup. Add an isolated Linux /proc/self/fd regression for 520 finished/replayed tasks, passing at soft limit 1024. Preserve cache eviction/cleanup assertions while naming descriptor-free cached records accurately. Directory identity, foreign-session, directory-retention and monitor-release mutation controls each reddened their named tests. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../release-notes/next/task-descriptors.md | 7 + crates/aft/src/bash_background/persistence.rs | 60 +++++++ crates/aft/src/bash_background/registry.rs | 44 +++-- crates/aft/src/bash_background/task_access.rs | 158 +++++++++++++++--- 4 files changed, 226 insertions(+), 43 deletions(-) create mode 100644 .alfonso/release-notes/next/task-descriptors.md diff --git a/.alfonso/release-notes/next/task-descriptors.md b/.alfonso/release-notes/next/task-descriptors.md new file mode 100644 index 0000000000..25666e263e --- /dev/null +++ b/.alfonso/release-notes/next/task-descriptors.md @@ -0,0 +1,7 @@ +### Fixed + +- Finished and recovered bash tasks no longer keep directory descriptors open + just to cache read permissions. AFT reopens and checks each directory's identity + when it is accessed, preserving session isolation and symlink protection. + Adopted tasks also release their process-exit monitor when they finish, so + retained task history does not exhaust the daemon's file-descriptor limit. diff --git a/crates/aft/src/bash_background/persistence.rs b/crates/aft/src/bash_background/persistence.rs index e37e4a2dab..04d91ce2bc 100644 --- a/crates/aft/src/bash_background/persistence.rs +++ b/crates/aft/src/bash_background/persistence.rs @@ -223,7 +223,66 @@ pub struct PinnedDir { path: PathBuf, } +/// A directory's identity without an open handle. Historical read permissions +/// reopen the directory and compare identities instead of pinning it forever. +#[derive(Debug, Clone)] +pub(crate) struct DirectorySnapshot { + path: PathBuf, + identity: DirectoryIdentity, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct DirectoryIdentity { + #[cfg(unix)] + device: u64, + #[cfg(unix)] + inode: u64, + #[cfg(windows)] + volume: u32, + #[cfg(windows)] + index: (u32, u32), +} + +impl DirectorySnapshot { + pub(crate) fn reopen(&self) -> io::Result { + let current = PinnedDir::open(&self.path)?; + if current.identity()? != self.identity { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "registered directory path identity changed", + )); + } + Ok(current) + } +} + impl PinnedDir { + fn identity(&self) -> io::Result { + #[cfg(unix)] + { + let metadata = self.file.metadata()?; + Ok(DirectoryIdentity { + device: metadata.dev(), + inode: metadata.ino(), + }) + } + #[cfg(windows)] + { + let info = windows_file_information(&self.file)?; + Ok(DirectoryIdentity { + volume: info.volume_serial_number, + index: (info.file_index_high, info.file_index_low), + }) + } + } + + pub(crate) fn snapshot(&self) -> io::Result { + Ok(DirectorySnapshot { + path: self.path.clone(), + identity: self.identity()?, + }) + } + pub fn open(path: &Path) -> io::Result { #[cfg(test)] work_counts::record_open(); @@ -467,6 +526,7 @@ impl PinnedDir { } } + #[cfg(windows)] pub(crate) fn ensure_current_identity(&self) -> io::Result<()> { let current = Self::open(&self.path)?; if !self.same_identity(¤t)? { diff --git a/crates/aft/src/bash_background/registry.rs b/crates/aft/src/bash_background/registry.rs index a8f6066d9e..cd8ef324c1 100644 --- a/crates/aft/src/bash_background/registry.rs +++ b/crates/aft/src/bash_background/registry.rs @@ -947,9 +947,10 @@ pub(crate) struct RegistryInner { Mutex, std::sync::mpsc::Receiver<()>)>>, pub(crate) tasks: Mutex>>, /// Session owners of output files recovered from task records on disk. - /// Directory handles prevent cached permissions from following a replaced I/O directory. + /// Identity snapshots prevent cached permissions from following a replaced + /// I/O directory without keeping a descriptor open for each historical task. persisted_artifact_owners: - Mutex)>>, + Mutex>, task_read_records: Mutex)>>, #[cfg(test)] task_access_work: task_access::WorkCounters, @@ -1584,11 +1585,7 @@ impl BgTaskRegistry { }) } - fn persisted_artifact_owner( - &self, - session_id: &str, - requested: &Path, - ) -> Option> { + fn persisted_artifact_owner(&self, session_id: &str, requested: &Path) -> Option<()> { let cached = self .inner .persisted_artifact_owners @@ -1596,9 +1593,15 @@ impl BgTaskRegistry { .ok()? .get(requested) .cloned(); - if let Some((owner, io)) = cached { - return (owner == session_id && io.open_file(requested.file_name()?, false).is_ok()) - .then_some(io); + if let Some((owner, snapshot)) = cached { + if owner != session_id { + return None; + } + let io = snapshot.reopen().ok()?; + return io + .open_file(requested.file_name()?, false) + .is_ok() + .then_some(()); } // A task record may not have been loaded since the daemon restarted. // Check the record named by the output path, not neighboring tasks or files. @@ -1637,12 +1640,13 @@ impl BgTaskRegistry { { return None; } - let io = Arc::clone(&resolved.dirs.io); - self.inner.persisted_artifact_owners.lock().ok()?.insert( - requested.to_path_buf(), - (session_id.to_string(), Arc::clone(&io)), - ); - Some(io) + let snapshot = resolved.dirs.io.snapshot().ok()?; + self.inner + .persisted_artifact_owners + .lock() + .ok()? + .insert(requested.to_path_buf(), (session_id.to_string(), snapshot)); + Some(()) } pub fn read_artifact_path( @@ -12521,6 +12525,14 @@ fn release_terminal_io_handles(task: &BgTask) -> bool { _ => (None, false), }; drop(handles); + // An adopted process's pidfd or kqueue is useful only until it becomes + // terminal. History must not keep the exit observer's descriptor alive. + #[cfg(unix)] + if runtime_released { + if let Ok(mut observer) = task.adopted_exit.lock() { + *observer = Default::default(); + } + } runtime_released && task.terminal_reader_done.load(Ordering::SeqCst) } diff --git a/crates/aft/src/bash_background/task_access.rs b/crates/aft/src/bash_background/task_access.rs index a6d16d4220..ee6f570b6d 100644 --- a/crates/aft/src/bash_background/task_access.rs +++ b/crates/aft/src/bash_background/task_access.rs @@ -1,12 +1,13 @@ use super::*; -use crate::bash_background::persistence::{PinnedDir, ResolvedTask, TaskLayout}; +use crate::bash_background::persistence::{DirectorySnapshot, PinnedDir, TaskLayout}; #[cfg(test)] use std::sync::atomic::AtomicUsize; const TASK_READ_RECORD_CAPACITY: usize = 64; pub(super) struct TaskReadRecord { - resolved: ResolvedTask, + paths: TaskPaths, + directories: [DirectorySnapshot; 3], metadata: PersistedTask, } @@ -38,8 +39,108 @@ mod tests { paths } + #[cfg(target_os = "linux")] #[test] - fn task_read_cache_is_bounded_and_eviction_releases_handles() { + fn finished_and_recovered_tasks_keep_descriptor_count_bounded() { + // Isolate /proc/self/fd from other tests opening files concurrently. + const CHILD: &str = "AFT_TASK_DESCRIPTOR_TEST_CHILD"; + if std::env::var_os(CHILD).is_none() { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "bash_background::registry::task_access::tests::finished_and_recovered_tasks_keep_descriptor_count_bounded", + "--nocapture", + ]) + .env(CHILD, "1") + .output() + .unwrap(); + assert!( + output.status.success(), + "descriptor test failed: {}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); + print!("{}", String::from_utf8_lossy(&output.stdout)); + return; + } + let count = || fs::read_dir("/proc/self/fd").unwrap().count(); + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let baseline = count(); + let mut peak = baseline; + for index in 0..520 { + let paths = record(®istry, storage.path(), index); + assert!(registry.is_session_owned_artifact_path("session", &paths.stdout)); + peak = peak.max(count()); + assert!( + count() <= baseline + 16, + "finished task {index}: descriptor count {} exceeds baseline {baseline} + 16", + count() + ); + } + registry.replay_session(storage.path(), "session").unwrap(); + assert_eq!(registry.inner.tasks.lock().unwrap().len(), 520); + for task in registry.task_arcs() { + // Model the process monitor acquired while an adopted task was + // running, then exercise the same terminal release as the watchdog. + assert!(!task + .adopted_exit + .lock() + .unwrap() + .marker_due(Some(std::process::id()), Instant::now())); + assert!(release_terminal_io_handles(&task)); + } + let after_replay = count(); + assert!( + after_replay <= baseline + 16, + "replay retained task descriptors: {after_replay}, baseline {baseline}" + ); + println!( + "520 tasks: baseline={baseline}, finished_peak={peak}, after_replay={after_replay}" + ); + } + + #[test] + fn cached_artifact_ownership_rejects_foreign_sessions() { + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let paths = record(®istry, storage.path(), 0); + assert!(registry.is_session_owned_artifact_path("session", &paths.stdout)); + assert!(!registry.is_session_owned_artifact_path("foreign", &paths.stdout)); + } + + #[test] + fn cached_artifact_ownership_rejects_replaced_io_directory() { + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let paths = record(®istry, storage.path(), 0); + assert!(registry.is_session_owned_artifact_path("session", &paths.stdout)); + fs::rename(&paths.io_dir, paths.dir.join("old-io")).unwrap(); + fs::create_dir(&paths.io_dir).unwrap(); + fs::write(&paths.stdout, "replacement output").unwrap(); + assert!(!registry.is_session_owned_artifact_path("session", &paths.stdout)); + } + + #[test] + fn cached_task_reads_reject_replaced_directories() { + for name in ["task", "control", "io"] { + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let paths = record(®istry, storage.path(), 0); + let path = match name { + "task" => &paths.dir, + "control" => &paths.control_dir, + _ => &paths.io_dir, + }; + fs::rename(path, path.with_extension("old")).unwrap(); + fs::create_dir(path).unwrap(); + assert!(registry.task_read_record(&paths.dir).is_none(), "{name}"); + } + } + + #[test] + fn task_read_cache_is_bounded_and_eviction_releases_records() { let registry = BgTaskRegistry::default(); let storage = tempfile::tempdir().unwrap(); let first = record(®istry, storage.path(), 0); @@ -48,19 +149,19 @@ mod tests { record(®istry, storage.path(), index); assert!( registry.inner.task_read_records.lock().unwrap().len() <= 64, - "the pinned task-read cache must hold at most 64 records" + "the task-read cache must hold at most 64 records" ); } assert!( first_record.upgrade().is_none(), - "eviction must close handles" + "eviction must release the cached record" ); assert!(registry.is_readable_task_path("session", &first.stdout)); } #[cfg(unix)] #[test] - fn task_read_cache_identity_failure_releases_handles() { + fn task_read_cache_identity_failure_releases_records() { let registry = BgTaskRegistry::default(); let storage = tempfile::tempdir().unwrap(); let paths = record(®istry, storage.path(), 0); @@ -69,13 +170,13 @@ mod tests { assert!(registry.task_read_record(&paths.dir).is_none()); assert!( cached.upgrade().is_none(), - "a stale record must not retain handles" + "a stale record must leave the cache" ); assert!(registry.inner.task_read_records.lock().unwrap().is_empty()); } #[test] - fn task_read_cache_cleanup_releases_deleted_task_handles() { + fn task_read_cache_cleanup_releases_deleted_task_records() { let registry = BgTaskRegistry::default(); let storage = tempfile::tempdir().unwrap(); let paths = record(®istry, storage.path(), 0); @@ -88,13 +189,13 @@ mod tests { ); assert!( cached.upgrade().is_none(), - "cleanup must close cached handles" + "cleanup must release the cached record" ); assert!(registry.inner.task_read_records.lock().unwrap().is_empty()); } #[test] - fn task_read_cache_retention_releases_deleted_task_handles() { + fn task_read_cache_retention_releases_deleted_task_records() { let registry = BgTaskRegistry::default(); let storage = tempfile::tempdir().unwrap(); let paths = record(®istry, storage.path(), 0); @@ -108,7 +209,7 @@ mod tests { ); assert!( cached.upgrade().is_none(), - "retention must close cached handles" + "retention must release the cached record" ); assert!(registry.inner.task_read_records.lock().unwrap().is_empty()); } @@ -223,7 +324,7 @@ impl BgTaskRegistry { }); } let record = self.task_read_record(task_directory(path)?)?; - let paths = &record.resolved.paths; + let paths = &record.paths; if ![&paths.dir, &paths.control_dir, &paths.io_dir] .into_iter() .any(|directory| path == directory) @@ -280,16 +381,11 @@ impl BgTaskRegistry { let mut records = self.inner.task_read_records.lock().ok()?; if let Some(index) = records.iter().position(|(path, _)| path == root) { // Take the record out first so a failed identity check also - // releases the cache's directory handles. + // discards the stale permission record. let (path, record) = records.remove(index)?; - record.resolved.dirs.task.ensure_current_identity().ok()?; - record - .resolved - .dirs - .control - .ensure_current_identity() - .ok()?; - record.resolved.dirs.io.ensure_current_identity().ok()?; + for directory in &record.directories { + directory.reopen().ok()?; + } records.push_back((path, Arc::clone(&record))); return Some(record); } @@ -309,13 +405,21 @@ impl BgTaskRegistry { { return None; } - let record = Arc::new(TaskReadRecord { resolved, metadata }); + let record = Arc::new(TaskReadRecord { + paths: resolved.paths, + directories: [ + resolved.dirs.task.snapshot().ok()?, + resolved.dirs.control.snapshot().ok()?, + resolved.dirs.io.snapshot().ok()?, + ], + metadata, + }); let mut records = self.inner.task_read_records.lock().ok()?; if let Some((_, existing)) = records.iter().find(|(path, _)| path == root) { return Some(Arc::clone(existing)); } - // Keep only a small working set: each record owns three pinned - // directories, so retaining historical records would exhaust handles. + // Keep only a small working set of permission metadata. Directory + // snapshots carry no handles and are revalidated on each cache hit. if records.len() >= TASK_READ_RECORD_CAPACITY { records.pop_front(); } @@ -448,7 +552,7 @@ impl BgTaskRegistry { .unwrap_or(false) } - /// The read-only exception is pinned and cached per bundle, never per file. + /// The read-only exception is identity-checked per bundle, never per file. /// Bash's separate output-file exception remains exact-file-only: permission /// to inspect a task folder must not let a shell command enter that folder. pub(crate) fn is_readable_task_path(&self, session: &str, path: &Path) -> bool { @@ -461,8 +565,8 @@ impl BgTaskRegistry { let Some(record) = self.task_read_record(root) else { return false; }; - let own_io = record.metadata.session_id == session - && path.starts_with(&record.resolved.paths.io_dir); + let own_io = + record.metadata.session_id == session && path.starts_with(&record.paths.io_dir); if !own_io && !self.head_reads_record(&record) { return false; } From 40f7e14f4c48872dc637d3ddc3c52aeddf04960d Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:07:10 +0200 Subject: [PATCH 02/17] Word the task descriptor release note for users Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .alfonso/release-notes/next/task-descriptors.md | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/.alfonso/release-notes/next/task-descriptors.md b/.alfonso/release-notes/next/task-descriptors.md index 25666e263e..347c0b02ac 100644 --- a/.alfonso/release-notes/next/task-descriptors.md +++ b/.alfonso/release-notes/next/task-descriptors.md @@ -1,7 +1 @@ -### Fixed - -- Finished and recovered bash tasks no longer keep directory descriptors open - just to cache read permissions. AFT reopens and checks each directory's identity - when it is accessed, preserving session isolation and symlink protection. - Adopted tasks also release their process-exit monitor when they finish, so - retained task history does not exhaust the daemon's file-descriptor limit. +- Finished and recovered background commands no longer keep a folder handle open each, so a long command history can no longer exhaust the daemon's open-file limit. From 7a8d1a317d9e39296501a10f0345921520059672 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:51:30 +0200 Subject: [PATCH 03/17] mason: reduce disk-limit walks and health census churn Reuse immutable v1/v2 generation sizes across publication using readdir inode identity and cache stable-directory totals; keep mutable files and reconciliation fresh. Mixed publication fixture drops from 5,514-5,640 to 312 metadata calls per enforcement with eviction parity. Time rollup phases and debug-log once per minute. Keep detailed timing in the uncapped census, not the budgeted health reply: the existing stale-backend root test exposed root-detail displacement in an initial implementation. Cache only rendered census inputs and move prebuilt root JSON; counted mutation controls defend each saving. At 110 roots census rendering measured 7.493 -> 3.334 CPU ms/tick. Existing tests retain their contracts. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../next/disk-limit-health-cpu.md | 4 + crates/aft/src/commands/memory_census.rs | 174 ++++++++++++++- crates/aft/src/subc/health.rs | 147 ++++++++++++- crates/aft/src/subc/health_rollup_bench.rs | 17 +- crates/aft/src/views/eviction.rs | 208 ++++++++++++++++-- 5 files changed, 514 insertions(+), 36 deletions(-) create mode 100644 .alfonso/release-notes/next/disk-limit-health-cpu.md diff --git a/.alfonso/release-notes/next/disk-limit-health-cpu.md b/.alfonso/release-notes/next/disk-limit-health-cpu.md new file mode 100644 index 0000000000..5bfe6b1581 --- /dev/null +++ b/.alfonso/release-notes/next/disk-limit-health-cpu.md @@ -0,0 +1,4 @@ +## Lower background disk-limit and health-rollup CPU + +- Disk-limit accounting now recognizes immutable artifacts from both checkout publication formats. Republishing a view reuses the sizes of retained generations instead of invalidating every cached size; unchanged directories reuse their immutable total. Mutable databases, reader markers, and family stores are still measured freshly, and eviction limits and periodic reconciliation are unchanged. +- Health rollup now logs per-step elapsed time and call counts once per minute at debug level; the uncapped memory census also includes them under `health_rollup_tick`. Census rendering reuses rows when only unreported diagnostics change and moves prebuilt root JSON into the reply without serializing another copy. The census still updates changed byte counts, process measurements, and index-measurement ages. diff --git a/crates/aft/src/commands/memory_census.rs b/crates/aft/src/commands/memory_census.rs index 3c48ae0079..260d8acb8f 100644 --- a/crates/aft/src/commands/memory_census.rs +++ b/crates/aft/src/commands/memory_census.rs @@ -67,8 +67,7 @@ pub(crate) fn render_memory_census_with_roots( (None, None) => None, }; let process_io = crate::process_io::ProcessIoSnapshot::capture().to_value(); - json!({ - "roots": roots, + let mut result = json!({ "process": { "phys_footprint_bytes": process.phys_footprint_bytes, "rss_bytes": process.rss_bytes, @@ -103,7 +102,11 @@ pub(crate) fn render_memory_census_with_roots( "process_io": process_io.clone(), }, "process_io": process_io, - }) + }); + // The rows are already JSON. Move them into the reply rather than + // serializing and allocating a second copy of every root's object. + result["roots"] = Value::Object(roots); + result } fn render_root_row(root: &str, detail: &crate::memory::RootMemorySnapshot) -> Value { @@ -153,7 +156,45 @@ fn render_root_row(root: &str, detail: &crate::memory::RootMemorySnapshot) -> Va /// remain live, and the health publisher adds the current index-walk ages. #[derive(Default)] pub(crate) struct CensusRootRows { - rows: std::collections::BTreeMap, + rows: std::collections::BTreeMap, +} + +#[derive(PartialEq, Eq)] +struct RootRowInputs { + planes: [u64; 7], + semantic_worker: u64, + semantic_overlay: u64, + evictable: u64, +} + +impl From<&crate::memory::RootMemorySnapshot> for RootRowInputs { + fn from(detail: &crate::memory::RootMemorySnapshot) -> Self { + Self { + planes: [ + &detail.trigram, + &detail.semantic, + &detail.symbols, + &detail.callgraph, + &detail.callgraph_projection, + &detail.inspect, + &detail.pending_install, + ] + .map(|plane| plane.estimated_bytes.unwrap_or(0)), + semantic_worker: detail + .semantic + .counts + .get("worker_index_bytes") + .copied() + .unwrap_or(0), + semantic_overlay: detail + .semantic + .counts + .get("checkout_overlay_bytes") + .copied() + .unwrap_or(0), + evictable: detail.evictable_bytes(), + } + } } impl CensusRootRows { @@ -167,14 +208,14 @@ impl CensusRootRows { .roots .iter() .map(|(root, detail)| { - let entry = self - .rows - .entry(root.clone()) - .or_insert_with(|| (detail.clone(), render_root_row(root, detail))); - // Equality validates every input to the pure row renderer; no - // timestamp is reset and no live health state is cached here. - if entry.0 != *detail { - *entry = (detail.clone(), render_root_row(root, detail)); + let inputs = RootRowInputs::from(detail); + let entry = self.rows.entry(root.clone()).or_insert_with(|| { + (RootRowInputs::from(detail), render_root_row(root, detail)) + }); + // Only rendered inputs invalidate a row. Index diagnostics and + // counts not shown in the census can change on every read. + if entry.0 != inputs { + *entry = (inputs, render_root_row(root, detail)); } (root.clone(), entry.1.clone()) }) @@ -219,6 +260,115 @@ mod tests { MemorySnapshot::new("ready", BTreeMap::from([("/repo".to_string(), root)])) } + #[test] + fn census_render_moves_110_prebuilt_rows_without_copying() { + let snapshot = row_cache_fixture(); + let roots = (0..110) + .map(|index| { + let root = format!("/repo-{index}"); + ( + root.clone(), + render_root_row(&root, &snapshot.roots["/repo"]), + ) + }) + .collect::>(); + // Moving JSON preserves its string buffers; serializing it allocates + // new buffers while the input is still alive. + let buffers = roots + .iter() + .map(|(root, row)| { + ( + root.clone(), + row["root"].as_str().unwrap().as_ptr() as usize, + ) + }) + .collect::>(); + let rendered = render_memory_census_with_roots(&snapshot, roots); + let copies = buffers + .iter() + .filter(|(root, buffer)| { + rendered["roots"][root.as_str()]["root"] + .as_str() + .unwrap() + .as_ptr() as usize + != **buffer + }) + .count(); + println!("prebuilt census rows copied: {copies}/110"); + assert_eq!( + copies, 0, + "prebuilt JSON rows were serialized into another copy" + ); + } + + #[test] + fn census_110_roots_ignore_unrendered_churn_but_keep_current_numbers() { + let mut snapshot = row_cache_fixture(); + let detail = snapshot.roots.remove("/repo").unwrap(); + for root in 0..110 { + snapshot + .roots + .insert(format!("/repo-{root}"), detail.clone()); + } + let mut cache = CensusRootRows::default(); + let first = cache.render(&snapshot); + for detail in snapshot.roots.values_mut() { + detail.trigram.counts.insert("files".into(), 1001); + detail.semantic.counts.insert("chunks".into(), 2001); + } + let before = ROOT_ROW_RENDERS.with(|renders| renders.get()); + let second = cache.render(&snapshot); + let rendered = ROOT_ROW_RENDERS.with(|renders| renders.get()) - before; + println!("110-root metadata-only churn: {rendered} root row renders/tick"); + assert_eq!( + rendered, 0, + "unrendered diagnostics invalidated census rows" + ); + assert_eq!(first, second); + + let detail = snapshot.roots.get_mut("/repo-0").unwrap(); + detail.trigram.estimated_bytes = Some(42); + detail + .semantic + .counts + .insert("worker_index_bytes".into(), 7); + detail + .semantic + .counts + .insert("checkout_overlay_bytes".into(), 9); + let updated = cache.render(&snapshot); + assert_eq!(ROOT_ROW_RENDERS.with(|renders| renders.get()) - before, 1); + assert_eq!(updated["/repo-0"]["planes"]["search"], 42); + assert_eq!(updated["/repo-0"]["semantic_worker_bytes"], 7); + assert_eq!(updated["/repo-0"]["semantic_overlay_bytes"], 9); + assert_eq!(updated["/repo-0"]["attributed_bytes"], 42); + } + + #[test] + fn census_row_key_covers_every_rendered_input() { + let mut snapshot = row_cache_fixture(); + let mut cache = CensusRootRows::default(); + cache.render(&snapshot); + for plane in 0..7 { + let detail = snapshot.roots.get_mut("/repo").unwrap(); + let changed = match plane { + 0 => &mut detail.trigram, + 1 => &mut detail.semantic, + 2 => &mut detail.symbols, + 3 => &mut detail.callgraph, + 4 => &mut detail.callgraph_projection, + 5 => &mut detail.inspect, + _ => &mut detail.pending_install, + }; + changed.estimated_bytes = Some(100 + plane); + let actual = cache.render(&snapshot); + assert_eq!( + actual["/repo"], + render_root_row("/repo", &snapshot.roots["/repo"]) + ); + } + } + #[test] fn census_row_cache_reuses_unchanged_rows() { let snapshot = row_cache_fixture(); diff --git a/crates/aft/src/subc/health.rs b/crates/aft/src/subc/health.rs index 939dfeef0b..9d7ada8a2c 100644 --- a/crates/aft/src/subc/health.rs +++ b/crates/aft/src/subc/health.rs @@ -1831,6 +1831,7 @@ pub(super) struct HealthRollupCache { clock_ms: AtomicU64, /// Count of completed refresh attempts, including scheduler contention. refreshes: AtomicU64, + step_log_at_ms: AtomicU64, /// Generation of the newest published census. published_generation: AtomicU64, /// One more than the highest census generation that a contended refresh @@ -1859,6 +1860,7 @@ impl HealthRollupCache { #[cfg(test)] clock_ms: AtomicU64::new(u64::MAX), refreshes: AtomicU64::new(0), + step_log_at_ms: AtomicU64::new(u64::MAX), published_generation: AtomicU64::new(0), stale_through_generation: AtomicU64::new(0), #[cfg(test)] @@ -1896,6 +1898,16 @@ impl HealthRollupCache { observation } + fn step_log_due(&self) -> bool { + let now = self.now_ms(); + let last = self.step_log_at_ms.load(Ordering::Relaxed); + (last == u64::MAX || now.saturating_sub(last) >= 60_000) + && self + .step_log_at_ms + .compare_exchange(last, now, Ordering::Relaxed, Ordering::Relaxed) + .is_ok() + } + fn retention_count( &self, database: Option<&Arc>>, @@ -2136,7 +2148,15 @@ impl HealthRollupCache { } pub(super) fn memory_census(&self) -> Value { - self.snapshot().0.memory_census.clone() + let (snapshot, age_ms) = self.snapshot(); + let mut census = snapshot.memory_census.clone(); + if let Some(tick) = census + .get_mut("health_rollup_tick") + .and_then(Value::as_object_mut) + { + tick.insert("age_ms".into(), json!(age_ms)); + } + census } } @@ -2496,19 +2516,64 @@ mod rollup_bench; macro_rules! rollup_mark { ($timer:ident, $step:literal) => { - #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] $timer.mark($step); }; } +struct RollupStepTimer { + started: Instant, + previous: Instant, + steps: std::collections::BTreeMap<&'static str, (Duration, u64)>, + #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] + cpu: rollup_bench::Timer, +} + +impl RollupStepTimer { + fn new() -> Self { + let started = Instant::now(); + Self { + started, + previous: started, + steps: Default::default(), + #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] + cpu: rollup_bench::Timer::new(), + } + } + + fn mark(&mut self, step: &'static str) { + let now = Instant::now(); + let entry = self.steps.entry(step).or_default(); + entry.0 += now.duration_since(self.previous); + entry.1 += 1; + self.previous = now; + #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] + self.cpu.mark(step); + } + + fn metrics(&self) -> Value { + // Elapsed time includes scheduler waits; the offline benchmark also + // measures thread CPU so host contention is distinguishable from work. + let steps = self + .steps + .iter() + .map(|(step, (elapsed, calls))| { + ( + (*step).to_owned(), + json!({"elapsed_ms": elapsed.as_secs_f64() * 1000.0, "calls": calls}), + ) + }) + .collect::>(); + json!({"elapsed_ms": self.started.elapsed().as_secs_f64() * 1000.0, "steps": steps}) + } +} + fn build_health_diagnostic_rollup_from_actors( cache: &HealthRollupCache, executor: &Executor, shared_app: &App, actor_entries: Vec<(ProjectRootId, Arc)>, ) -> HealthDiagnosticRollup { - #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] - let mut timer = rollup_bench::Timer::new(); + let mut timer = RollupStepTimer::new(); struct RootCandidate { root_label: String, health: RootHealthSnapshot, @@ -2916,6 +2981,13 @@ fn build_health_diagnostic_rollup_from_actors( }); insert_lifecycle_metrics(&mut metrics, lifecycle); rollup_mark!(timer, "metrics JSON assembly"); + let tick = timer.metrics(); + if log::log_enabled!(log::Level::Debug) && cache.step_log_due() { + log::debug!("health rollup tick: {tick}"); + } + // Keep detailed timing outside the size-budgeted health reply: adding it + // there would displace root diagnostics. The uncapped census retains it. + memory_census["health_rollup_tick"] = tick; HealthDiagnosticRollup { status: if busy_roots > 0 || standing_refusals > 0 { HealthStatus::Degraded @@ -5287,6 +5359,73 @@ mod tests { ); } + #[test] + fn rollup_110_root_step_counters_keep_current_numbers_and_age() { + let executor = Executor::new(); + let app = crate::context::App::default_shared(); + let cache = HealthRollupCache::new(); + cache.clock_ms.store(0, Ordering::Relaxed); + let mut dirs = Vec::new(); + let mut first = None; + for index in 0..110 { + let (dir, root) = test_root(&format!("rollup-counters-{index}")); + let ctx = test_ctx(); + if index == 0 { + first = Some((root.clone(), Arc::clone(&ctx))); + } + assert!(executor.register_actor(root, ctx)); + dirs.push(dir); + } + cache.refresh_for_test(&executor, &app); + let (root, ctx) = first.unwrap(); + let label = root.as_path().display().to_string(); + let before = cache.memory_census()["roots"][&label]["attributed_bytes"] + .as_u64() + .unwrap(); + cache.clock_ms.store(3000, Ordering::Relaxed); + assert_eq!(cache.snapshot().1, Some(3000)); + assert_eq!(cache.memory_census()["health_rollup_tick"]["age_ms"], 3000); + ctx.symbol_cache().write().unwrap().insert( + root.as_path().join("current.rs"), + SystemTime::UNIX_EPOCH, + 12, + blake3::hash(b"current"), + Vec::new(), + ); + cache.refresh_for_test(&executor, &app); + let (snapshot, age) = cache.snapshot(); + assert_eq!(age, Some(0)); + let tick = &snapshot.memory_census["health_rollup_tick"]; + assert!(snapshot.metrics.get("health_rollup_tick").is_none()); + assert_eq!(tick["steps"]["per-root memory census"]["calls"], 110); + assert_eq!(tick["steps"]["lifecycle OS probes"]["calls"], 1); + assert_eq!(tick["steps"].as_object().unwrap().len(), 14); + assert!(tick["elapsed_ms"].as_f64().unwrap() >= 0.0); + let census = cache.memory_census(); + assert_eq!(census["health_rollup_tick"]["age_ms"], 0); + assert_eq!(census["roots"].as_object().unwrap().len(), 110); + assert!( + census["roots"][&label]["attributed_bytes"] + .as_u64() + .unwrap() + > before + ); + assert!(census["roots"][&label]["memory_measured_age_ms"].is_u64()); + } + + #[test] + fn rollup_step_debug_log_is_due_once_per_minute() { + let cache = HealthRollupCache::new(); + cache.clock_ms.store(0, Ordering::Relaxed); + assert!(cache.step_log_due()); + assert!(!cache.step_log_due()); + cache.clock_ms.store(59_999, Ordering::Relaxed); + assert!(!cache.step_log_due()); + cache.clock_ms.store(60_000, Ordering::Relaxed); + assert!(cache.step_log_due()); + assert!(!cache.step_log_due()); + } + /// The rollup asks every root for its memory every few seconds. While a /// root's indexes keep their shape it must reuse the last index walk, a /// shape change must walk again at once, and the reported age must say how diff --git a/crates/aft/src/subc/health_rollup_bench.rs b/crates/aft/src/subc/health_rollup_bench.rs index 5490c1d113..f8de6ea70e 100644 --- a/crates/aft/src/subc/health_rollup_bench.rs +++ b/crates/aft/src/subc/health_rollup_bench.rs @@ -43,7 +43,7 @@ impl Timer { } #[test] -#[ignore = "offline per-step CPU measurement with 40 roots and a large aft.db"] +#[ignore = "offline per-step measurement with configurable roots and a large aft.db"] fn bench_health_rollup_steps() { use super::super::test_support::test_root; // The defaults reproduce the original 40-root run. The variables below @@ -177,6 +177,8 @@ fn bench_health_rollup_steps() { })); STEPS.with(|steps| *steps.borrow_mut() = Some(BTreeMap::new())); let mut total = 0u64; + let mut elapsed_steps = BTreeMap::::new(); + let mut elapsed_total = 0.0; for tick in 1..=TICKS { if churn { // What publishing views, reads and an occasional edit do between @@ -213,6 +215,12 @@ fn bench_health_rollup_steps() { &app, executor.actor_entries(), ); + let tick = &built.memory_census["health_rollup_tick"]; + elapsed_total += tick["elapsed_ms"].as_f64().unwrap(); + for (step, value) in tick["steps"].as_object().unwrap() { + *elapsed_steps.entry(step.clone()).or_default() += + value["elapsed_ms"].as_f64().unwrap(); + } let mut publish_timer = Timer::new(); cache.publish(Some(built)); publish_timer.mark("publish holder / old JSON drop"); @@ -230,6 +238,13 @@ fn bench_health_rollup_steps() { ns as f64 / TICKS as f64 / 3000. ); } + for (step, ms) in elapsed_steps { + println!("{step}: elapsed_ms/tick={:.3}", ms / TICKS as f64); + } + println!( + "BUILD TOTAL: elapsed_ms/tick={:.3}", + elapsed_total / TICKS as f64 + ); println!( "TOTAL: us/tick={:.2} us/second={:.2}", total as f64 / TICKS as f64 / 1000., diff --git a/crates/aft/src/views/eviction.rs b/crates/aft/src/views/eviction.rs index 5d27a8a913..6f6aca5806 100644 --- a/crates/aft/src/views/eviction.rs +++ b/crates/aft/src/views/eviction.rs @@ -304,6 +304,7 @@ thread_local! { /// sweep it started most recently. The sweep's report does not record /// its budget, so tests read it here. static LAST_SWEEP_BUDGET: std::cell::Cell> = const { std::cell::Cell::new(None) }; + static TREE_LISTS: std::cell::Cell = const { std::cell::Cell::new(0) }; } fn measured_metadata(path: &Path) -> std::io::Result { @@ -312,14 +313,18 @@ fn measured_metadata(path: &Path) -> std::io::Result { fs::symlink_metadata(path) } -/// Immutable artifacts are reconciled even if an out-of-band writer changes -/// them in place. SQLite files, pins, markers and temporary files are never cached. +/// Published v1 and v2 generation artifacts are reconciled even if an +/// out-of-band writer changes them in place. SQLite files, pins, markers and +/// temporary files are never cached. const SIZE_RECONCILE_INTERVAL: Duration = Duration::from_secs(6 * 60 * 60); pub(super) struct ViewSizeCache { directory: Option>, reconciled: Option, - files: BTreeMap, + files: BTreeMap, + // Stable namespaces reuse the immutable total and list only mutable paths. + snapshot: Option<(u64, Vec)>, + top_paths: BTreeSet, } /// The size caches of one family's views, keyed by view directory. The @@ -343,6 +348,8 @@ impl Default for ViewSizeCache { directory: None, reconciled: None, files: BTreeMap::new(), + snapshot: None, + top_paths: BTreeSet::new(), } } } @@ -365,7 +372,9 @@ fn immutable_generation(path: &Path) -> Option<&str> { .find_map(|(prefix, suffix)| { name.strip_prefix(prefix) .and_then(|name| name.strip_suffix(suffix)) - .filter(|generation| super::manifest_v2::GenerationName::parse(generation).is_some()) + // Both formats write manifests once. The v1 publisher uses names + // without a g2 prefix, so validate those alongside v2 names. + .filter(|generation| super::validate_generation(generation).is_ok()) }) } @@ -406,11 +415,22 @@ fn view_tree_bytes( let reconcile = cache .reconciled .is_none_or(|last| last.elapsed() >= SIZE_RECONCILE_INTERVAL); - if identity.is_none() || identity != cache.directory || reconcile { + let same_directory = identity + .as_ref() + .zip(cache.directory.as_ref()) + .is_some_and(|(current, previous)| current[..2] == previous[..2]); + if !same_directory || reconcile { cache.files.clear(); cache.reconciled = Some(Instant::now()); } + // Publication changes the namespace, not old generations. Re-list names + // on that change, but validate old artifacts using readdir's inode instead + // of throwing away every retained generation's measured length. + if identity.is_none() || identity != cache.directory || reconcile || past(deadline) { + cache.snapshot = None; + } cache.directory = identity; + let reused_snapshot = cache.snapshot.is_some(); let result = tree_bytes_inner(dir, deadline, Some(cache), None); let after = measured_metadata(dir) .ok() @@ -421,16 +441,32 @@ fn view_tree_bytes( // with a manifest published after that file was measured. if !matches!(result, Ok((_, true))) || after != cache.directory { cache.files.clear(); - } else { + cache.snapshot = None; + } else if !reused_snapshot { let manifests = cache .files .keys() + .filter(|path| cache.top_paths.contains(*path)) .filter(|path| path.extension().is_some_and(|ext| ext == "json")) .filter_map(|path| immutable_generation(path).map(str::to_owned)) .collect::>(); cache.files.retain(|path, _| { - immutable_generation(path).is_some_and(|generation| manifests.contains(generation)) + cache.top_paths.contains(path) + && immutable_generation(path) + .is_some_and(|generation| manifests.contains(generation)) }); + cache.snapshot = Some(( + cache + .files + .values() + .fold(0u64, |total, (_, bytes)| total.saturating_add(*bytes)), + cache + .top_paths + .iter() + .filter(|path| !cache.files.contains_key(*path)) + .cloned() + .collect(), + )); } result } @@ -460,16 +496,53 @@ fn tree_bytes_inner( ) -> std::io::Result<(u64, bool)> { #[cfg(test)] TREE_WALKS.with(|walks| walks.set(walks.get() + 1)); - let mut bytes = 0u64; - let mut read = 0usize; + let mut bytes = cache + .as_ref() + .and_then(|cache| cache.snapshot.as_ref()) + .map_or(0, |(bytes, _)| *bytes); + let mut read = cache + .as_ref() + .filter(|cache| cache.snapshot.is_some()) + .map_or(0, |cache| cache.files.len()); let mut complete = true; let mut pending = vec![(dir.to_path_buf(), 0usize)]; while let Some((current, depth)) = pending.pop() { - let entries = match fs::read_dir(¤t) { - Ok(entries) => entries, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, - Err(error) => return Err(error), - }; + let cached_paths = (depth == 0) + .then(|| { + cache + .as_ref() + .and_then(|cache| cache.snapshot.as_ref()) + .map(|(_, paths)| paths.clone()) + }) + .flatten(); + let entries: Box)>>> = + if let Some(paths) = cached_paths { + Box::new(paths.into_iter().map(|path| Ok((path, None)))) + } else { + if depth == 0 { + if let Some(cache) = cache.as_mut() { + cache.top_paths.clear(); + } + } + #[cfg(test)] + TREE_LISTS.with(|lists| lists.set(lists.get() + 1)); + match fs::read_dir(¤t) { + Ok(entries) => Box::new(entries.map(|entry| { + entry.map(|entry| { + #[cfg(unix)] + let inode = { + use std::os::unix::fs::DirEntryExt; + Some(entry.ino()) + }; + #[cfg(not(unix))] + let inode = None; + (entry.path(), inode) + }) + })), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => return Err(error), + } + }; let remaining = MAX_WALK_ENTRIES.saturating_sub(read); for entry in entries.take(remaining.saturating_add(1)) { if read >= MAX_WALK_ENTRIES @@ -480,23 +553,31 @@ fn tree_bytes_inner( break; } read += 1; - let entry = match entry { + let (path, inode) = match entry { Ok(entry) => entry, // Removed while listed: nothing to count. Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, Err(error) => return Err(error), }; - let path = entry.path(); + if depth == 0 { + if let Some(cache) = cache.as_mut() { + cache.top_paths.insert(path.clone()); + } + } let immutable = depth == 0 && immutable_generation(&path).is_some(); if immutable { - if let Some(size) = cache + if let Some((_, size)) = cache .as_ref() .filter(|cache| cache.directory.is_some()) .and_then(|cache| cache.files.get(&path)) + .filter(|(previous_inode, _)| inode == Some(*previous_inode)) { bytes = bytes.saturating_add(*size); continue; } + if let Some(cache) = cache.as_mut() { + cache.files.remove(&path); + } } let metadata = match measured_metadata(&path) { Ok(metadata) => metadata, @@ -505,7 +586,7 @@ fn tree_bytes_inner( }; if metadata.is_dir() { if depth + 1 < MAX_WALK_DEPTH { - pending.push((entry.path(), depth + 1)); + pending.push((path.clone(), depth + 1)); } else { complete = false; } @@ -513,7 +594,9 @@ fn tree_bytes_inner( bytes = bytes.saturating_add(metadata.len()); if immutable { if let Some(cache) = cache.as_mut() { - cache.files.insert(path, metadata.len()); + if let Some(inode) = inode { + cache.files.insert(path, (inode, metadata.len())); + } } } } @@ -1510,6 +1593,73 @@ mod tests { assert_eq!(TREE_STATS.with(|stats| stats.get()), 200); } + #[cfg(unix)] + #[test] + fn republishing_views_measure_only_changed_artifacts() { + let storage = tempfile::tempdir().unwrap(); + let registry = FamilyRegistry::open(storage.path(), FAMILY).unwrap(); + let publish = |dir: &Path, generation: usize| { + let generation = if generation % 2 == 0 { + format!("{generation}-42-1234567890-{generation}-deadbeef") + } else { + format!("g2-{generation:064x}-1-1-{generation}") + }; + for (prefix, suffix) in [("manifest", "json"), ("trigram", "bin"), ("derived", "ref")] { + fs::write( + dir.join(format!("{prefix}-{generation}.{suffix}")), + b"immutable", + ) + .unwrap(); + } + }; + let mut dirs = Vec::new(); + for index in 0..24 { + let root = storage.path().join(format!("checkout-{index}")); + fs::create_dir(&root).unwrap(); + let view = registry + .register_view(&format!("view-{index}"), &root) + .unwrap(); + fs::write(view.view_dir().join("derived-owner.sqlite"), vec![0; 8192]).unwrap(); + fs::create_dir_all(view.view_dir().join("readers/current")).unwrap(); + fs::write(view.view_dir().join("progress.marker"), b"reader").unwrap(); + for generation in 0..64 { + publish(view.view_dir(), generation); + } + dirs.push(view.view_dir().to_path_buf()); + } + measure(®istry, None).unwrap(); + let mut calls = Vec::new(); + for pass in 0..6 { + // Six checkouts publish between ten-minute enforcement passes. + for dir in dirs.iter().take(6) { + publish(dir, 64 + pass); + fs::write(dir.join("derived-owner.sqlite"), vec![0; 8192 + pass]).unwrap(); + } + TREE_STATS.with(|stats| stats.set(0)); + let report = enforce(®istry, EnforceOptions::default()).unwrap(); + let stats = TREE_STATS.with(|stats| stats.replace(0)); + calls.push(stats); + assert!(report.evicted.is_empty()); + for view in &report.after.views { + assert_eq!( + (view.bytes, view.complete), + tree_bytes(&view.view_dir, None).unwrap() + ); + } + println!("republish enforcement pass {pass}: {stats} lstat/pass, {} lstat/hour at six passes/hour", stats * 6); + } + assert!( + calls.iter().all(|calls| *calls < 500), + "retained generations re-statted after publication: {calls:?}" + ); + let usage = measure(®istry, None).unwrap(); + registry.set_disk_budget(budget(1, u64::MAX)); + let report = enforce(®istry, EnforceOptions::default()).unwrap(); + assert_eq!(report.evicted.len(), dirs.len()); + assert_eq!(report.after.view_store_bytes, 0); + assert!(usage.view_store_bytes > 0); + } + #[cfg(unix)] #[test] fn immutable_view_totals_avoid_repeated_stats() { @@ -1545,13 +1695,22 @@ mod tests { } TREE_STATS.with(|stats| stats.set(0)); TREE_WALKS.with(|walks| walks.set(0)); + TREE_LISTS.with(|lists| lists.set(0)); let cold = measure(®istry, None).unwrap(); let cold_stats = TREE_STATS.with(|stats| stats.replace(0)); + let cold_lists = TREE_LISTS.with(|lists| lists.replace(0)); let warm = measure(®istry, None).unwrap(); let warm_stats = TREE_STATS.with(|stats| stats.replace(0)); + let warm_lists = TREE_LISTS.with(|lists| lists.replace(0)); let walks = TREE_WALKS.with(|walks| walks.get()); assert_eq!(walks, 2 * (24 + 1)); assert_eq!(cold.views, warm.views); + println!("directory listings: cold={cold_lists}, warm={warm_lists}"); + assert_eq!( + warm_lists + 24, + cold_lists, + "unchanged view namespaces were listed again" + ); println!("disk measurement / 24 views x 64 generations: cold={cold_stats} lstat, warm={warm_stats} lstat, {walks} tree walks; two measurements per enforcement, interval={}s", ENFORCEMENT_INTERVAL.as_secs()); assert!(cold_stats > 24 * 64 * 3); assert!( @@ -1617,6 +1776,17 @@ mod tests { view_tree_bytes(dir, None, &mut cache).unwrap(), tree_bytes(dir, None).unwrap() ); + let replacement = dir.join("replacement.tmp"); + fs::write( + &replacement, + b"atomic replacement has a different inode and length", + ) + .unwrap(); + fs::rename(&replacement, &manifest).unwrap(); + assert_eq!( + view_tree_bytes(dir, None, &mut cache).unwrap(), + tree_bytes(dir, None).unwrap() + ); fs::write(&manifest, b"out-of-band in-place drift is reconciled").unwrap(); cache.reconciled = Some(Instant::now() - SIZE_RECONCILE_INTERVAL); assert_eq!( From 95346cddb9da4508ab5e423d40eb2072c5e7f824 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:54:47 +0200 Subject: [PATCH 04/17] Word the disk-limit release note for users Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .alfonso/release-notes/next/disk-limit-health-cpu.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.alfonso/release-notes/next/disk-limit-health-cpu.md b/.alfonso/release-notes/next/disk-limit-health-cpu.md index 5bfe6b1581..f0cd7d2e59 100644 --- a/.alfonso/release-notes/next/disk-limit-health-cpu.md +++ b/.alfonso/release-notes/next/disk-limit-health-cpu.md @@ -1,4 +1 @@ -## Lower background disk-limit and health-rollup CPU - -- Disk-limit accounting now recognizes immutable artifacts from both checkout publication formats. Republishing a view reuses the sizes of retained generations instead of invalidating every cached size; unchanged directories reuse their immutable total. Mutable databases, reader markers, and family stores are still measured freshly, and eviction limits and periodic reconciliation are unchanged. -- Health rollup now logs per-step elapsed time and call counts once per minute at debug level; the uncapped memory census also includes them under `health_rollup_tick`. Census rendering reuses rows when only unreported diagnostics change and moves prebuilt root JSON into the reply without serializing another copy. The census still updates changed byte counts, process measurements, and index-measurement ages. +- The background check that keeps index views under their disk limit no longer re-measures unchanged folders on every pass (about 94% fewer file checks), and the health summary does less work per tick. From 935da83fb4bbb7b108f5e85cdb40a43439783748 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:00:24 +0200 Subject: [PATCH 05/17] mason: reap language servers when project roots are deleted Handle issue #631 before ordinary root-presence admission while retaining bound/current-generation checks. Keep generation-scoped retry demand after dropping the watcher, invalidate initializing clients even when none are published, and reap detached clients outside the manager lock. Log the root and server count once per cleanup. Cover standalone running and initializing children, busy-manager and watcher-lock retries, unbind, recreation, and idempotence. Subc already shuts down servers on route-scoped actor retirement; add a fake-server regression proving that without the orphan sweep. The existing binary deletion-control test sent RootDeleted while its directory still existed. Delete the fixture directory before sending the event so its unchanged assertions test actual deletion; stale events for existing/recreated roots must not trigger teardown. Add the release-note credit to @chrisolszewski. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../release-notes/next/deleted-root-lsp.md | 1 + crates/aft/src/context.rs | 94 ++++++- crates/aft/src/main.rs | 1 + crates/aft/src/runtime_drain.rs | 239 +++++++++++++++++- crates/aft/src/subc/mod.rs | 40 +++ 5 files changed, 366 insertions(+), 9 deletions(-) create mode 100644 .alfonso/release-notes/next/deleted-root-lsp.md diff --git a/.alfonso/release-notes/next/deleted-root-lsp.md b/.alfonso/release-notes/next/deleted-root-lsp.md new file mode 100644 index 0000000000..566814a560 --- /dev/null +++ b/.alfonso/release-notes/next/deleted-root-lsp.md @@ -0,0 +1 @@ +Language servers started by standalone AFT now shut down promptly when their project root is deleted, including servers still initializing and managers temporarily busy with another request. Cleanup preserves lifecycle generation checks so it cannot stop servers belonging to a replacement project at the same path. Thanks to @chrisolszewski for reporting and diagnosing issue #631. diff --git a/crates/aft/src/context.rs b/crates/aft/src/context.rs index 231842fcea..8b0b38045b 100644 --- a/crates/aft/src/context.rs +++ b/crates/aft/src/context.rs @@ -1739,6 +1739,12 @@ pub(crate) fn watcher_counters_for_root(root: &Path) -> Arc { counters } +#[derive(Debug)] +enum DeletedRootCleanup { + Pending(u64), + Complete(u64), +} + #[derive(Debug)] pub(crate) struct WatcherDrainSliceState { pub(crate) configure_generation: u64, @@ -3792,6 +3798,7 @@ pub struct AppContext { watcher: parking_lot::Mutex>, watcher_rx: parking_lot::Mutex>>, watcher_drain_slice: parking_lot::Mutex>, + deleted_root_cleanup: parking_lot::Mutex>, watcher_drains_in_flight: AtomicUsize, /// Advances when a watcher drain starts and when it ends. watcher_drain_activity: AtomicU64, @@ -4550,6 +4557,7 @@ impl AppContext { watcher: parking_lot::Mutex::new(None), watcher_rx: parking_lot::Mutex::new(None), watcher_drain_slice: parking_lot::Mutex::new(None), + deleted_root_cleanup: parking_lot::Mutex::new(None), watcher_drains_in_flight: AtomicUsize::new(0), watcher_drain_activity: AtomicU64::new(0), watcher_thread: parking_lot::Mutex::new(None), @@ -5643,6 +5651,58 @@ impl AppContext { ) } + /// Clean up a missing root without admitting ordinary work on that root. + /// Returns true when the drain should stop, including when the LSP + /// manager is busy and cleanup must be retried on a later drain call. + pub(crate) fn try_cleanup_deleted_root(&self, expected_generation: u64) -> bool { + self.subc_lifecycle + .run_if_current( + self.configure_generation.as_ref(), + expected_generation, + || { + let Some(root) = self + .canonical_cache_root_opt() + .or_else(|| self.config().project_root.clone()) + else { + return false; + }; + let mut cleanup = self.deleted_root_cleanup.lock(); + if root.is_dir() { + *cleanup = None; + return false; + } + if matches!(*cleanup, Some(DeletedRootCleanup::Complete(generation)) if generation == expected_generation) { + return true; + } + *cleanup = Some(DeletedRootCleanup::Pending(expected_generation)); + // A concurrent drain may hold the receiver while awaiting + // lifecycle admission. Never wait for it under this gate. + if !self.try_stop_watcher_runtime_in_background() { + return true; + } + let Some(mut lsp) = self.try_lsp() else { + // Directory absence survives watcher shutdown. The next + // drain retries even though no receiver remains to wake it. + return true; + }; + // Also invalidate starts that have not published a client yet. + let clients = lsp.take_all_clients(); + drop(lsp); + *cleanup = Some(DeletedRootCleanup::Complete(expected_generation)); + let _ = self.add_degraded_reason("project_root_deleted".to_string()); + crate::slog_warn!( + "project root deleted {}: shut down {} language server(s); dropping watcher", + root.display(), + clients.len() + ); + LspManager::spawn_idle_lsp_reap(clients); + self.signal_status_changed(); + true + }, + ) + .unwrap_or(false) + } + /// Commit the warm-maintenance identity for a successful configure. /// /// The semantic epoch decision shares the warm-state lock with equivalence @@ -5732,11 +5792,15 @@ impl AppContext { /// whole module for as long as any other thread holds the source, so /// never add one: use `try_lock` and treat contention as pending work. pub fn watcher_drain_has_work(&self) -> bool { + let cleanup_pending = self.deleted_root_cleanup.try_lock().is_none_or(|cleanup| { + matches!(*cleanup, Some(DeletedRootCleanup::Pending(generation)) if generation == self.configure_generation()) + }); let receiver_pending = self .watcher_rx .try_lock() .is_none_or(|slot| slot.as_ref().is_some_and(|rx| !rx.is_empty())); - receiver_pending + cleanup_pending + || receiver_pending || self.watcher_drain_slice.try_lock().is_none_or(|slot| { slot.as_ref().is_some_and(|state| { state.has_pending_work() @@ -11835,6 +11899,34 @@ impl AppContext { self.stop_watcher_runtime(); } + fn try_stop_watcher_runtime_in_background(&self) -> bool { + let taken = (|| { + let _runtime = self.watcher_runtime_lock.try_lock()?; + let mut thread = self.watcher_thread.try_lock()?; + let mut rx = self.watcher_rx.try_lock()?; + let mut slice = self.watcher_drain_slice.try_lock()?; + let mut watcher = self.watcher.try_lock()?; + let mut identity = self.watcher_runtime_identity.try_lock()?; + *rx = None; + *slice = None; + *watcher = None; + Some((thread.take(), identity.take())) + })(); + let Some((runtime, identity)) = taken else { + return false; + }; + if let Some(identity) = identity { + crate::lsp::manager::invalidate_applicability_walk( + &identity.root, + crate::lsp::manager::InvalidationCause::WatcherRemoved, + ); + } + if let Some(runtime) = runtime { + Self::spawn_watcher_shutdown(Arc::clone(&self.app), self.watcher_root_path(), runtime); + } + true + } + /// Remove a watcher runtime whose OS thread already exited (backend /// failure while the root was unbound and drains were suppressed). /// Returns true when a finished corpse was actually removed so the caller diff --git a/crates/aft/src/main.rs b/crates/aft/src/main.rs index 81c4dff3ad..398ccc6762 100644 --- a/crates/aft/src/main.rs +++ b/crates/aft/src/main.rs @@ -5200,6 +5200,7 @@ mod watcher_filter_tests { ctx.set_canonical_cache_root(root); let watcher_tx = install_watcher_rx(&ctx); + tmp.close().expect("delete project root"); watcher_tx.send(WatcherDispatchEvent::RootDeleted).unwrap(); drain_watcher_events(&ctx); diff --git a/crates/aft/src/runtime_drain.rs b/crates/aft/src/runtime_drain.rs index 9e0906c659..093f22124b 100644 --- a/crates/aft/src/runtime_drain.rs +++ b/crates/aft/src/runtime_drain.rs @@ -3496,6 +3496,11 @@ pub fn drain_watcher_events_bounded(ctx: &AppContext, max_paths: usize) -> Drain let configure_generation = ctx.configure_generation(); let content_generation = ctx.configure_content_generation(); let mut outcome = DrainBatchOutcome::default(); + // Ordinary admission rejects missing directories, but their cleanup must + // still run. It retains the lifecycle gate and retries without a watcher. + if ctx.try_cleanup_deleted_root(configure_generation) { + return outcome; + } // Admission before touching the continuation: an unbound invocation must // leave the retained state in place for the rebind to rebase, not take // and drop it. @@ -3657,13 +3662,7 @@ pub fn drain_watcher_events_bounded(ctx: &AppContext, max_paths: usize) -> Drain .is_some_and(|rx| !rx.is_empty()); if root_deleted { - ctx.stop_watcher_runtime_in_background(); - let _ = ctx.add_degraded_reason("project_root_deleted".to_string()); - aft::slog_warn!( - "project root deleted; dropping watcher to avoid delete-storm: {:?}", - ctx.canonical_cache_root_opt() - ); - ctx.signal_status_changed(); + ctx.try_cleanup_deleted_root(configure_generation); return outcome; } if let Some(error) = watcher_failed { @@ -8799,7 +8798,7 @@ mod watcher_slice_tests { // Every test here spawns a real child process, so the module is Unix-only. #[cfg(all(test, unix))] mod idle_lsp_tests { - use super::shutdown_idle_lsp_at; + use super::{drain_watcher_events_bounded, shutdown_idle_lsp_at}; use crate::config::Config; use crate::context::AppContext; use crate::lsp::child_registry::LspChildRegistry; @@ -8840,6 +8839,230 @@ mod idle_lsp_tests { false } + fn deleted_root_context( + root: &Path, + ) -> ( + AppContext, + crossbeam_channel::Sender, + ) { + std::fs::create_dir(root).unwrap(); + let mut config = Config::default(); + config.project_root = Some(root.to_path_buf()); + let ctx = AppContext::new(crate::context::default_language_provider_factory(), config); + ctx.set_canonical_cache_root(root.to_path_buf()); + let (tx, rx) = crossbeam_channel::unbounded(); + *ctx.watcher_rx().lock() = Some(rx); + (ctx, tx) + } + + #[test] + fn deleted_root_drain_shuts_down_running_lsp() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, tx) = deleted_root_context(&root); + let pid = spawn_sleep_client(&ctx, root.clone()); + assert!(crate::bash_background::process::is_process_alive(pid)); + + std::fs::remove_dir_all(&root).unwrap(); + tx.send(crate::watcher_filter::WatcherDispatchEvent::RootDeleted) + .unwrap(); + let (_, logs) = crate::logging::capture_log_lines(|| { + drain_watcher_events_bounded(&ctx, 1); + drain_watcher_events_bounded(&ctx, 1); + }); + + assert_eq!( + ctx.lsp().server_count(), + 0, + "deleted root must release LSP clients" + ); + assert!( + wait_until_dead(pid, Duration::from_secs(6)), + "deleted-root LSP child must exit" + ); + assert!(ctx.watcher_rx().lock().is_none()); + let cleanup_logs = logs + .iter() + .filter(|line| line.contains("project root deleted")) + .collect::>(); + assert_eq!( + cleanup_logs.len(), + 1, + "cleanup must log only once: {logs:?}" + ); + assert!(cleanup_logs[0].contains(&root.display().to_string())); + assert!(cleanup_logs[0].contains("shut down 1 language server(s)")); + } + + #[test] + fn deleted_root_drain_retries_busy_lsp_without_watcher() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let pid = spawn_sleep_client(&ctx, root.clone()); + std::fs::remove_dir_all(&root).unwrap(); + + let held = ctx.lsp(); + let started = Instant::now(); + drain_watcher_events_bounded(&ctx, 1); + assert!(started.elapsed() < Duration::from_millis(250)); + assert_eq!(held.server_count(), 1); + assert!( + ctx.watcher_rx().lock().is_none(), + "the retry must not depend on a watcher" + ); + assert!( + ctx.watcher_drain_has_work(), + "subc maintenance must schedule the retry" + ); + drop(held); + + drain_watcher_events_bounded(&ctx, 1); + assert_eq!( + ctx.lsp().server_count(), + 0, + "later drain must retry the busy manager" + ); + assert!(!ctx.watcher_drain_has_work()); + assert!(wait_until_dead(pid, Duration::from_secs(6))); + } + + #[test] + fn deleted_root_drain_preserves_unbind_admission() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let pid = spawn_sleep_client(&ctx, root.clone()); + ctx.mark_subc_unbound(); + std::fs::remove_dir_all(&root).unwrap(); + + drain_watcher_events_bounded(&ctx, 1); + assert_eq!(ctx.lsp().server_count(), 1); + assert!(ctx.watcher_rx().lock().is_some()); + assert!(crate::bash_background::process::is_process_alive(pid)); + + ctx.mark_subc_bound(); + drain_watcher_events_bounded(&ctx, 1); + assert_eq!(ctx.lsp().server_count(), 0); + assert!(wait_until_dead(pid, Duration::from_secs(6))); + } + + #[test] + fn deleted_root_cleanup_defers_contended_watcher_receiver() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let pid = spawn_sleep_client(&ctx, root.clone()); + std::fs::remove_dir_all(&root).unwrap(); + let held = ctx.watcher_rx().lock(); + let started = Instant::now(); + assert!(ctx.try_cleanup_deleted_root(ctx.configure_generation())); + assert!(started.elapsed() < Duration::from_millis(250)); + assert!(held.is_some()); + assert!(ctx.watcher_drain_has_work()); + assert_eq!(ctx.lsp().server_count(), 1); + drop(held); + + drain_watcher_events_bounded(&ctx, 1); + assert_eq!(ctx.lsp().server_count(), 0); + assert!(wait_until_dead(pid, Duration::from_secs(6))); + } + + #[test] + fn deleted_root_cleanup_rejects_stale_generation_after_recreation() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let old_generation = ctx.configure_generation(); + let old_pid = spawn_sleep_client(&ctx, root.clone()); + std::fs::remove_dir_all(&root).unwrap(); + drain_watcher_events_bounded(&ctx, 1); + assert!(wait_until_dead(old_pid, Duration::from_secs(6))); + + std::fs::create_dir(&root).unwrap(); + ctx.advance_configure_generation(); + let pid = spawn_sleep_client(&ctx, root.clone()); + assert!(!ctx.try_cleanup_deleted_root(ctx.configure_generation())); + let (tx, rx) = crossbeam_channel::unbounded(); + *ctx.watcher_rx().lock() = Some(rx); + tx.send(crate::watcher_filter::WatcherDispatchEvent::RootDeleted) + .unwrap(); + drain_watcher_events_bounded(&ctx, 1); + assert_eq!( + ctx.lsp().server_count(), + 1, + "an old deletion event must not reap a recreated root" + ); + std::fs::remove_dir_all(&root).unwrap(); + assert!(!ctx.try_cleanup_deleted_root(old_generation)); + assert_eq!( + ctx.lsp().server_count(), + 1, + "stale cleanup must not touch the replacement" + ); + assert!(crate::bash_background::process::is_process_alive(pid)); + + assert!(ctx.try_cleanup_deleted_root(ctx.configure_generation())); + assert!(wait_until_dead(pid, Duration::from_secs(6))); + } + + #[test] + fn deleted_root_drain_rejects_initializing_lsp_with_no_published_clients() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let ctx = std::sync::Arc::new(ctx); + std::fs::write( + root.join("Cargo.toml"), + "[package]\nname='deleted'\nversion='0.1.0'\n", + ) + .unwrap(); + let file = root.join("lib.rs"); + std::fs::write(&file, "pub fn f() {}\n").unwrap(); + let signal = tmp.path().join("initializing"); + ctx.lsp().override_binary( + ServerKind::Rust, + crate::fake_lsp_test_helper::fake_server_binary(), + ); + ctx.lsp() + .set_extra_env("AFT_FAKE_LSP_INIT_DELAY_MS", "1500"); + ctx.lsp() + .set_extra_env("AFT_FAKE_LSP_INIT_DELAY_SIGNAL", signal.to_str().unwrap()); + let worker_ctx = std::sync::Arc::clone(&ctx); + let worker = std::thread::spawn(move || { + crate::lsp::manager::start_servers_for_file_unlocked( + || worker_ctx.lsp(), + &file, + &worker_ctx.config(), + ); + }); + let deadline = Instant::now() + Duration::from_secs(30); + while !signal.exists() { + assert!( + Instant::now() < deadline, + "fake LSP never received initialize" + ); + std::thread::sleep(Duration::from_millis(10)); + } + assert!( + !worker.is_finished(), + "initialize delay must still hold the server" + ); + assert_eq!(ctx.lsp().server_count(), 0); + let pids = ctx.lsp_child_registry().pids(); + assert_eq!(pids.len(), 1); + + std::fs::remove_dir_all(&root).unwrap(); + drain_watcher_events_bounded(&ctx, 1); + worker.join().unwrap(); + assert_eq!( + ctx.lsp().server_count(), + 0, + "initializing server must not publish after deletion" + ); + assert!(wait_until_dead(pids[0], Duration::from_secs(6))); + } + #[cfg(unix)] #[test] fn idle_lsp_ttl_shuts_down_stale_client_and_keeps_recent() { diff --git a/crates/aft/src/subc/mod.rs b/crates/aft/src/subc/mod.rs index 97fd5ec0c2..2bbebb0936 100644 --- a/crates/aft/src/subc/mod.rs +++ b/crates/aft/src/subc/mod.rs @@ -15359,6 +15359,46 @@ mod tests { (root_dir, root, ctx, executor, live_roots, pids[0]) } + #[test] + fn deleted_root_reclaim_shuts_down_running_lsp_without_registry_sweep() { + let (root_dir, root, ctx, executor, mut live_roots, pid) = + root_with_running_fake_server("deleted-root-lsp-reclaim"); + let bound = HashMap::from([(root.clone(), HashSet::from([route_key(31, 1)]))]); + root_dir.close().expect("delete project root"); + let metrics = DispatchPathMetrics::new(); + let deadline = Instant::now() + Duration::from_secs(10); + loop { + let outcome = reap_idle_roots( + Instant::now(), + &mut live_roots, + &HashMap::new(), + &bound, + &executor, + &metrics, + ); + if !outcome.forgotten_deleted_roots.is_empty() { + assert_eq!(outcome.forgotten_deleted_roots, vec![root.clone()]); + break; + } + assert!(Instant::now() < deadline, "deleted root was not retired"); + std::thread::sleep(Duration::from_millis(10)); + } + assert!(!executor.actor_registered(&root)); + assert!(live_roots.is_empty()); + + // Retiring the actor must stop its server without either the watcher + // drain or the process-wide orphan sweep assisting the teardown. + let deadline = Instant::now() + Duration::from_secs(10); + while crate::bash_background::process::is_process_alive(pid) { + assert!( + Instant::now() < deadline, + "retired root left LSP child {pid} running" + ); + std::thread::sleep(Duration::from_millis(20)); + } + assert_eq!(ctx.lsp().server_count(), 0); + } + fn quiesce_whole_connection( live_roots: &mut HashMap, executor: &Arc, From ac036b53023a20e116015e702d9b9c01487da7a0 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:03:07 +0200 Subject: [PATCH 06/17] Word the deleted-root language server release note for users Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .alfonso/release-notes/next/deleted-root-lsp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.alfonso/release-notes/next/deleted-root-lsp.md b/.alfonso/release-notes/next/deleted-root-lsp.md index 566814a560..14688d0d09 100644 --- a/.alfonso/release-notes/next/deleted-root-lsp.md +++ b/.alfonso/release-notes/next/deleted-root-lsp.md @@ -1 +1 @@ -Language servers started by standalone AFT now shut down promptly when their project root is deleted, including servers still initializing and managers temporarily busy with another request. Cleanup preserves lifecycle generation checks so it cannot stop servers belonging to a replacement project at the same path. Thanks to @chrisolszewski for reporting and diagnosing issue #631. +- Language servers that standalone AFT started for a project now shut down as soon as that project folder is deleted, instead of running until the hour-long idle timeout. Thanks to @chrisolszewski for reporting and diagnosing this (#631). From 7f4faaf500d04fb88ec7c78d09475b4446d1eb6f Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:11:53 +0200 Subject: [PATCH 07/17] mason: allow an initial inspect lease attempt after slow preparation An absolute retry deadline could expire before the first filesystem lock attempt, falsely reporting contention on an unowned cold inspect cache. Admit that first attempt without restarting or relaxing subsequent contention retries. Add a deterministic slow-preparation regression and preserve the held-lease deadline/epoch assertions. The existing expired-preparation expectation changes from zero to one attempt intentionally; no extra sleep or writer takeover is permitted. Verified Linux and native Windows lock/cache/fallback, binary inspect, and tsconfig integration tests; Linux compiler and warning-denied Windows target checks. Mutation controls independently reject the original false-timeout behavior and a restarted retry budget. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../release-notes/next/inspect-lease-flake.md | 1 + crates/aft/src/fs_lock.rs | 5 ++-- crates/aft/src/inspect/cache.rs | 23 ++++++++++++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 .alfonso/release-notes/next/inspect-lease-flake.md diff --git a/.alfonso/release-notes/next/inspect-lease-flake.md b/.alfonso/release-notes/next/inspect-lease-flake.md new file mode 100644 index 0000000000..7a28b2a631 --- /dev/null +++ b/.alfonso/release-notes/next/inspect-lease-flake.md @@ -0,0 +1 @@ +- `aft_inspect` no longer reports a writer-lease timeout when no other process holds its cache but preparing the cache was slow. diff --git a/crates/aft/src/fs_lock.rs b/crates/aft/src/fs_lock.rs index dbc9c0e056..d73cd1de32 100644 --- a/crates/aft/src/fs_lock.rs +++ b/crates/aft/src/fs_lock.rs @@ -185,7 +185,8 @@ pub fn try_acquire(path: &Path, timeout: Duration) -> Result Result { crate::production_storage::refuse_write(path)?; #[cfg(test)] @@ -410,7 +411,7 @@ fn acquire_with_deadline( let mut malformed_grace_used = false; loop { - if attempted_once || strict_deadline { + if attempted_once { if immediate_retry_budget > 0 && !strict_deadline { immediate_retry_budget -= 1; } else if let Some(deadline) = deadline { diff --git a/crates/aft/src/inspect/cache.rs b/crates/aft/src/inspect/cache.rs index 65e5bf68d4..09d2026d79 100644 --- a/crates/aft/src/inspect/cache.rs +++ b/crates/aft/src/inspect/cache.rs @@ -2863,6 +2863,23 @@ pub(crate) mod tests { ); } + #[test] + fn uncontended_writer_lease_opens_after_slow_preparation() { + let temp = tempfile::tempdir().expect("create temporary cache root"); + let project_root = temp.path().join("project"); + let inspect_dir = temp.path().join("inspect"); + fs::create_dir_all(&project_root).expect("create project root"); + crate::root_cache::configure_artifact_access(&project_root, "shared", false); + + // Cold filesystem preparation can exhaust the retry budget before + // the first lock attempt, even when no other process owns the cache. + let clock = crate::fs_lock::retry_clock::Guard::install(Duration::from_secs(6)); + let cache = InspectCache::open(inspect_dir, project_root) + .expect("an uncontended lease must not require preparation to fit the retry budget"); + assert!(cache.writer_epoch_for_test().is_some()); + assert_eq!(clock.observations(), (Duration::from_secs(6), vec![], 1)); + } + #[test] fn held_writer_lease_fails_within_the_inspect_deadline() { let temp = tempfile::tempdir().expect("create temporary cache root"); @@ -2884,7 +2901,7 @@ pub(crate) mod tests { for (preparation_ms, expected_elapsed_ms, expected_sleeps, expected_attempts) in [ (0, 2_000, vec![Duration::from_millis(100); 20], 20), (1_975, 2_000, vec![Duration::from_millis(25)], 1), - (6_000, 6_000, vec![], 0), + (6_000, 6_000, vec![], 1), ] { let inspect_dir = inspect_dir.clone(); let project_root = project_root.clone(); @@ -2915,6 +2932,10 @@ pub(crate) mod tests { assert_eq!(attempts, expected_attempts, "bounded lease poll iterations"); } + assert!( + held.verify_writer_epoch().unwrap(), + "the holder keeps its lease" + ); drop(held); InspectCache::open(inspect_dir, project_root) .expect("cache opens after the competing writer releases"); From 9a0a37f9858b820c2ab5ffcfa928a30068fcc716 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:44:01 +0200 Subject: [PATCH 08/17] mason: retry merge-queue PR association lookup Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- benchmarks/aft-search/run_search_quality.py | 2 +- .../aft-search/test_merge_queue_search.py | 158 ++++++++++++++++++ scripts/lib/test_merge_queue_workflow.py | 13 +- scripts/merge-queue-search.py | 76 +++++++-- 4 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 benchmarks/aft-search/test_merge_queue_search.py diff --git a/benchmarks/aft-search/run_search_quality.py b/benchmarks/aft-search/run_search_quality.py index 940090ac5c..0bb86d2fc4 100644 --- a/benchmarks/aft-search/run_search_quality.py +++ b/benchmarks/aft-search/run_search_quality.py @@ -123,7 +123,7 @@ def run(args: argparse.Namespace) -> int: if args.self_test: command([sys.executable, str(HERE / "search_quality.py"), "--self-test"]) command( - [sys.executable, "-m", "unittest", "-v", "test_run_real_query.py", "test_run_concept_recall.py", "test_search_quality.py", "test_harness_integrity.py"], + [sys.executable, "-m", "unittest", "-v", "test_run_real_query.py", "test_run_concept_recall.py", "test_search_quality.py", "test_harness_integrity.py", "test_merge_queue_search.py"], cwd=HERE, ) return 0 diff --git a/benchmarks/aft-search/test_merge_queue_search.py b/benchmarks/aft-search/test_merge_queue_search.py new file mode 100644 index 0000000000..5ccc107926 --- /dev/null +++ b/benchmarks/aft-search/test_merge_queue_search.py @@ -0,0 +1,158 @@ +from __future__ import annotations + +import importlib.util +from pathlib import Path +import unittest + + +ROOT = Path(__file__).resolve().parents[2] +SPEC = importlib.util.spec_from_file_location( + "merge_queue_search", ROOT / "scripts/merge-queue-search.py" +) +assert SPEC is not None and SPEC.loader is not None +merge_queue_search = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(merge_queue_search) + + +class MemberPrRetryTests(unittest.TestCase): + def setUp(self) -> None: + self.pr = { + "number": 23, + "head": {"sha": "a" * 40, "ref": "feature"}, + "base": {"ref": "main", "repo": {"full_name": "cortexkit/aft"}}, + } + + def test_empty_lookup_retries_until_association_appears(self) -> None: + responses = iter(([], [self.pr])) + calls: list[tuple[str, str]] = [] + delays: list[float] = [] + + def lookup(repository: str, head: str) -> list[dict]: + calls.append((repository, head)) + return next(responses) + + result = merge_queue_search.member_pr( + "cortexkit/aft", self.pr["head"]["sha"], lookup=lookup, sleep=delays.append + ) + + self.assertEqual(result, self.pr) + self.assertEqual(calls, [("cortexkit/aft", self.pr["head"]["sha"])] * 2) + self.assertEqual(delays, [5]) + + def test_persistent_empty_lookup_fails_after_bounded_retries(self) -> None: + calls: list[tuple[str, str]] = [] + delays: list[float] = [] + + def lookup(repository: str, head: str) -> list[dict]: + calls.append((repository, head)) + return [] + + with self.assertRaisesRegex( + merge_queue_search.InputFault, + r"ambiguous_queue_member:.*:matches=0:attempts=5$", + ): + merge_queue_search.member_pr( + "cortexkit/aft", + self.pr["head"]["sha"], + lookup=lookup, + sleep=delays.append, + ) + + self.assertEqual(len(calls), 5) + self.assertEqual(delays, [5, 10, 15, 30]) + + def test_multiple_matches_fail_without_retry(self) -> None: + calls: list[tuple[str, str]] = [] + delays: list[float] = [] + fallback_calls: list[tuple[str, int]] = [] + + def lookup(repository: str, head: str) -> list[dict]: + calls.append((repository, head)) + return [self.pr, {**self.pr, "number": 24}] + + with self.assertRaisesRegex( + merge_queue_search.InputFault, + r"ambiguous_queue_member:.*:matches=2:attempts=1$", + ): + merge_queue_search.member_pr( + "cortexkit/aft", + self.pr["head"]["sha"], + queue_ref_pr_number=23, + lookup=lookup, + queue_ref_lookup=lambda repository, number: fallback_calls.append((repository, number)), + sleep=delays.append, + ) + + self.assertEqual(len(calls), 1) + self.assertEqual(delays, []) + self.assertEqual(fallback_calls, []) + + def test_lookup_http_error_is_not_retried(self) -> None: + calls = 0 + delays: list[float] = [] + + def lookup(_repository: str, _head: str) -> list[dict]: + nonlocal calls + calls += 1 + raise merge_queue_search.InputFault("queue_pr_lookup_failed:head:unauthorized") + + with self.assertRaisesRegex( + merge_queue_search.InputFault, r"queue_pr_lookup_failed:head:unauthorized$" + ): + merge_queue_search.member_pr( + "cortexkit/aft", + self.pr["head"]["sha"], + lookup=lookup, + sleep=delays.append, + ) + + self.assertEqual(calls, 1) + self.assertEqual(delays, []) + + def test_queue_ref_pr_is_used_only_when_head_matches(self) -> None: + calls = 0 + + def lookup(_repository: str, _head: str) -> list[dict]: + nonlocal calls + calls += 1 + return [] + + result = merge_queue_search.member_pr( + "cortexkit/aft", + self.pr["head"]["sha"], + queue_ref_pr_number=23, + lookup=lookup, + queue_ref_lookup=lambda _repository, _number: self.pr, + sleep=lambda _seconds: None, + ) + + self.assertEqual(result, self.pr) + self.assertEqual(calls, 5) + + def test_queue_ref_pr_with_different_head_does_not_override_fault(self) -> None: + calls = 0 + + def lookup(_repository: str, _head: str) -> list[dict]: + nonlocal calls + calls += 1 + return [] + + different_head = {**self.pr, "head": {**self.pr["head"], "sha": "b" * 40}} + with self.assertRaisesRegex( + merge_queue_search.InputFault, + r"ambiguous_queue_member:.*:matches=0:attempts=5$", + ): + merge_queue_search.member_pr( + "cortexkit/aft", + self.pr["head"]["sha"], + queue_ref_pr_number=23, + lookup=lookup, + queue_ref_lookup=lambda _repository, _number: different_head, + sleep=lambda _seconds: None, + ) + + self.assertEqual(calls, 5) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/lib/test_merge_queue_workflow.py b/scripts/lib/test_merge_queue_workflow.py index 0a6ec088c9..0df2d0b676 100644 --- a/scripts/lib/test_merge_queue_workflow.py +++ b/scripts/lib/test_merge_queue_workflow.py @@ -155,9 +155,16 @@ def setUp(self) -> None: fake_gh.write_text( f"#!{sys.executable}\n" "import json, pathlib, sys\n" - "assert sys.argv[1:4] == ['api', '--paginate', '--slurp']\n" - "head = sys.argv[4].split('/commits/')[1].split('/')[0]\n" - "print(json.dumps([json.loads(pathlib.Path('.fixture-prs.json').read_text()).get(head, [])]))\n" + "fixture = json.loads(pathlib.Path('.fixture-prs.json').read_text())\n" + "if sys.argv[1:4] == ['api', '--paginate', '--slurp']:\n" + " head = sys.argv[4].split('/commits/')[1].split('/')[0]\n" + " print(json.dumps([fixture.get(head, [])]))\n" + "elif sys.argv[1:2] == ['api'] and '/pulls/' in sys.argv[2]:\n" + " number = int(sys.argv[2].rsplit('/', 1)[1])\n" + " pr = next((pr for prs in fixture.values() for pr in prs if pr['number'] == number), None)\n" + " print(json.dumps(pr))\n" + "else:\n" + " raise AssertionError(sys.argv)\n" ) fake_gh.chmod(0o755) self.env = {**os.environ, "PATH": str(bin_dir) + os.pathsep + os.environ["PATH"]} diff --git a/scripts/merge-queue-search.py b/scripts/merge-queue-search.py index c77a5945b3..78cca2af4e 100644 --- a/scripts/merge-queue-search.py +++ b/scripts/merge-queue-search.py @@ -14,6 +14,7 @@ import re import subprocess import sys +import time ROOT = Path(__file__).resolve().parents[1] @@ -45,7 +46,10 @@ def resolve_git_descriptor(descriptor: dict, changed: list[str], base: str, head print(note, file=sys.stderr) -def member_pr(repository: str, head: str) -> dict: +PR_LOOKUP_BACKOFF_SECONDS = (5, 10, 15, 30) + + +def lookup_member_prs(repository: str, head: str) -> list[dict]: result = subprocess.run( ["gh", "api", "--paginate", "--slurp", f"repos/{repository}/commits/{head}/pulls?per_page=100"], @@ -54,14 +58,64 @@ def member_pr(repository: str, head: str) -> dict: if result.returncode: raise InputFault(f"queue_pr_lookup_failed:{head}:{result.stderr.strip()}") pages = json.loads(result.stdout) - matches = [ - pr for page in pages for pr in page - if pr["head"]["sha"] == head and pr["base"]["ref"] == "main" + return [pr for page in pages for pr in page] + + +def lookup_queue_ref_pr(repository: str, number: int) -> dict: + result = subprocess.run( + ["gh", "api", f"repos/{repository}/pulls/{number}"], + text=True, capture_output=True, check=False, + ) + if result.returncode: + raise InputFault(f"queue_pr_lookup_failed:{number}:{result.stderr.strip()}") + return json.loads(result.stdout) + + +def is_member_pr(pr: dict, repository: str, head: str) -> bool: + return ( + pr["head"]["sha"] == head + and pr["base"]["ref"] == "main" and pr["base"]["repo"]["full_name"] == repository - ] - if len(matches) != 1: - raise InputFault(f"ambiguous_queue_member:{head}:matches={len(matches)}") - return matches[0] + ) + + +def member_pr( + repository: str, + head: str, + *, + queue_ref_pr_number: int | None = None, + lookup=lookup_member_prs, + queue_ref_lookup=lookup_queue_ref_pr, + sleep=time.sleep, +) -> dict: + for attempt in range(1, len(PR_LOOKUP_BACKOFF_SECONDS) + 2): + matches = [ + pr for pr in lookup(repository, head) + if is_member_pr(pr, repository, head) + ] + if len(matches) == 1: + return matches[0] + if len(matches) > 1: + raise InputFault( + f"ambiguous_queue_member:{head}:matches={len(matches)}:attempts={attempt}" + ) + if attempt <= len(PR_LOOKUP_BACKOFF_SECONDS): + sleep(PR_LOOKUP_BACKOFF_SECONDS[attempt - 1]) + + if queue_ref_pr_number is not None: + pr = queue_ref_lookup(repository, queue_ref_pr_number) + # The queue ref names only its final PR; its API record is safe to use + # only when the PR still points at this merge's exact second-parent SHA. + if ( + pr is not None + and pr.get("number") == queue_ref_pr_number + and is_member_pr(pr, repository, head) + ): + return pr + + raise InputFault( + f"ambiguous_queue_member:{head}:matches=0:attempts={len(PR_LOOKUP_BACKOFF_SECONDS) + 1}" + ) def member_descriptor(head: str, branch: str, group_head: str) -> tuple[str, dict]: @@ -102,12 +156,14 @@ def queue_members(base: str, head: str, head_ref: str | None, repository: str) - raise InputFault(f"unknown_queue_ref:{head_ref}") members = [] previous = base - for commit in git("rev-list", "--first-parent", "--reverse", f"{base}..{head}").splitlines(): + commits = git("rev-list", "--first-parent", "--reverse", f"{base}..{head}").splitlines() + for index, commit in enumerate(commits): parents = git("show", "-s", "--format=%P", commit).split() # Only two-parent merges bind the synthetic commit to an exact PR head. if len(parents) != 2 or parents[0] != previous: raise InputFault(f"unsupported_queue_topology:{commit}") - pr = member_pr(repository, parents[1]) + queue_ref_pr_number = int(tail[1]) if tail is not None and index == len(commits) - 1 else None + pr = member_pr(repository, parents[1], queue_ref_pr_number=queue_ref_pr_number) if any(member["number"] == pr["number"] for member in members): raise InputFault(f"duplicate_queue_member:{pr['number']}") members.append({"number": pr["number"], "branch": pr["head"]["ref"], "head": parents[1]}) From fbc621ad96cfb9421fbf60b540b18de88647ff4b Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:11:54 +0200 Subject: [PATCH 09/17] mason: report unconfirmed created Swift syntax as unknown A Swift tree-sitter rejection needs compiler confirmation for a new file. Timeout, discovery failure, signal termination and abnormal compiler failure now leave the verdict unknown with syntax_skipped_reason and no syntax_error_line. Preserve conservative rollback for existing files and propagate the reason through write, append, patch metadata and agent-facing text. The native integration test previously required compiler success inside the production five-second budget. It now accepts an explicitly explained unknown outcome; deterministic injected tests still require true for compiler acceptance and false for a confirmed syntax rejection, and a separate macOS native test requires acceptance under the existing generous test-only guard. The old missing-compiler created-file expectation intentionally changes from invalid to unknown. Verified Linux compiler diagnostics, Swift/created-file regressions and binary tests. Mutation controls restored timeout-to-invalid fallback and removed the rendered unknown notice: each named guard failed while the definite-answer control passed, then all six created-Swift tests passed after restore. Native swiftc checks remain for macOS. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../next/swift-syntax-unknown.md | 1 + crates/aft/src/commands/apply_patch.rs | 57 +++-- crates/aft/src/commands/edit_match.rs | 3 + crates/aft/src/edit.rs | 39 ++- crates/aft/src/edit/swift.rs | 225 ++++++++++++++++-- crates/aft/src/subc_format.rs | 8 + .../integration/created_file_syntax_test.rs | 20 +- 7 files changed, 294 insertions(+), 59 deletions(-) create mode 100644 .alfonso/release-notes/next/swift-syntax-unknown.md diff --git a/.alfonso/release-notes/next/swift-syntax-unknown.md b/.alfonso/release-notes/next/swift-syntax-unknown.md new file mode 100644 index 0000000000..73d4fc7f10 --- /dev/null +++ b/.alfonso/release-notes/next/swift-syntax-unknown.md @@ -0,0 +1 @@ +- New Swift files now report syntax as unknown with a reason when compiler confirmation times out or is unavailable, instead of incorrectly reporting a definite syntax error; confirmed valid and invalid compiler verdicts are unchanged. diff --git a/crates/aft/src/commands/apply_patch.rs b/crates/aft/src/commands/apply_patch.rs index 873a0903ee..8f41c8c5cf 100644 --- a/crates/aft/src/commands/apply_patch.rs +++ b/crates/aft/src/commands/apply_patch.rs @@ -47,6 +47,7 @@ struct DiffEntry { delete_lines: Option, hunk_count: usize, syntax_error_line: Option, + syntax_skipped_reason: Option<&'static str>, } fn path_string(path: &Path) -> String { @@ -364,7 +365,7 @@ fn write_patched_file( let params = command_params(req); // A file this patch created has nothing from before the call to protect: // it is never rolled back, and its final content is syntax-checked once - // after every hunk has run (see `created_file_syntax_errors`). + // after every hunk has run (see `created_file_syntax_checks`). let written = if created_in_call { edit::write_format_deferred_created(path, content, &ctx.config(), params) } else { @@ -598,41 +599,47 @@ fn add_failure(failures: &mut Vec, path: &str, error: String) { } /// Check the final content of every file this patch created and still exists, -/// and return the first parse-error line of each one that does not parse. +/// including why a compiler confirmation could not establish its validity. /// /// The check runs once per file after all hunks, because a later hunk in the /// same patch can fix, break, move or delete a file an earlier hunk created. -fn created_file_syntax_errors(created: &HashMap) -> HashMap { +fn created_file_syntax_checks( + created: &HashMap, +) -> HashMap { created .keys() .filter_map(|path| { let content = fs::read_to_string(path).ok()?; - let line = edit::check_created_file_syntax(path, &content).first_error_line?; - Some((path.clone(), line)) + let check = edit::check_created_file_syntax(path, &content); + Some((path.clone(), check)) }) .collect() } /// Put the created-file syntax notice directly under the reply line that -/// created each broken file ("Created" or "Updated and moved"), because one -/// patch can create several files and the notice itself does not name the +/// created each broken or unconfirmed file ("Created" or "Updated and moved"). +/// A patch can create several files and the notice itself does not name the /// file. `created` maps each created file to the index of that reply line. fn insert_created_syntax_notices( output_lines: &mut Vec, created: &HashMap, - errors: &HashMap, + checks: &HashMap, ) { - let mut notices: Vec<(usize, usize)> = errors + let mut notices: Vec<(usize, String)> = checks .iter() - .filter_map(|(path, error_line)| Some((*created.get(path)?, *error_line))) + .filter_map(|(path, check)| { + let notice = if let Some(line) = check.first_error_line { + edit::created_file_syntax_error_notice(line as u64) + } else { + edit::syntax_unknown_notice(check.skipped_reason?) + }; + Some((*created.get(path)?, notice)) + }) .collect(); // Insert from the bottom up so earlier reply-line indices stay valid. notices.sort_unstable_by(|a, b| b.0.cmp(&a.0)); - for (reply_index, error_line) in notices { - output_lines.insert( - reply_index + 1, - edit::created_file_syntax_error_notice(error_line as u64), - ); + for (reply_index, notice) in notices { + output_lines.insert(reply_index + 1, notice); } } @@ -985,12 +992,11 @@ fn report_key(applied: &AppliedHunkResult) -> String { } } -/// `created_errors` holds the final first parse-error line of each file this -/// patch created that does not parse (see `created_file_syntax_errors`). +/// Carry each created file's final syntax verdict into its metadata entry. fn metadata_files( applied: &[AppliedHunkResult], root: Option<&Path>, - created_errors: &HashMap, + created_checks: &HashMap, ) -> (String, Vec) { let mut entries: Vec<(String, DiffEntry)> = Vec::new(); @@ -1017,19 +1023,21 @@ fn metadata_files( delete_lines: applied_hunk.delete_lines, hunk_count: 1, syntax_error_line: None, + syntax_skipped_reason: None, }, )); } } - // Carry each broken created file's error line on the last entry that + // Carry each created file's syntax result on the last entry that // ends at that path, so a file reported by several entries (for example // moved onto, then updated) carries it once. let mut marked = HashSet::new(); for (_, entry) in entries.iter_mut().rev() { - if let Some(line) = created_errors.get(&entry.display_path) { + if let Some(check) = created_checks.get(&entry.display_path) { if marked.insert(entry.display_path.clone()) { - entry.syntax_error_line = Some(*line); + entry.syntax_error_line = check.first_error_line; + entry.syntax_skipped_reason = check.skipped_reason; } } } @@ -1079,6 +1087,7 @@ fn metadata_files( if let Some(line) = entry.syntax_error_line { value[edit::CREATED_SYNTAX_ERROR_LINE_FIELD] = json!(line); } + edit::append_syntax_skipped_reason(&mut value, entry.syntax_skipped_reason); value }) .collect::>(); @@ -1188,8 +1197,8 @@ fn apply_patch(req: &RawRequest, ctx: &AppContext, resolved: &[ResolvedHunk]) -> } } - let created_errors = created_file_syntax_errors(&created); - insert_created_syntax_notices(&mut output_lines, &created, &created_errors); + let created_checks = created_file_syntax_checks(&created); + insert_created_syntax_notices(&mut output_lines, &created, &created_checks); if !failures.is_empty() { let partial = failures.len() < resolved.len(); @@ -1210,7 +1219,7 @@ fn apply_patch(req: &RawRequest, ctx: &AppContext, resolved: &[ResolvedHunk]) -> } let root = project_root_for_relative_paths(ctx); - let (diff, mut files) = metadata_files(&applied, root.as_deref(), &created_errors); + let (diff, mut files) = metadata_files(&applied, root.as_deref(), &created_checks); // A partial patch keeps successful writes, but is not a completed project // mutation. Do not run checkers when any hunk failed or rolled back. if failures.is_empty() { diff --git a/crates/aft/src/commands/edit_match.rs b/crates/aft/src/commands/edit_match.rs index 6fdc7b833f..cbd6f51f0f 100644 --- a/crates/aft/src/commands/edit_match.rs +++ b/crates/aft/src/commands/edit_match.rs @@ -409,6 +409,8 @@ fn handle_append(req: &RawRequest, ctx: &AppContext, op_id: &str) -> Response { if let Some(line) = syntax_error_line { result[edit::CREATED_SYNTAX_ERROR_LINE_FIELD] = serde_json::json!(line); } + let syntax_skipped_reason = created_syntax.and_then(|check| check.skipped_reason); + edit::append_syntax_skipped_reason(&mut result, syntax_skipped_reason); if let Some(reason) = &format_skipped_reason { result["format_skipped_reason"] = serde_json::json!(reason); @@ -440,6 +442,7 @@ fn handle_append(req: &RawRequest, ctx: &AppContext, op_id: &str) -> Response { if lsp_outcome.is_some() { let write_result = edit::WriteResult { syntax_valid, + syntax_skipped_reason, syntax_error_line, formatted, format_skipped_reason: format_skipped_reason.clone(), diff --git a/crates/aft/src/edit.rs b/crates/aft/src/edit.rs index 8690cf570c..8cdfe41cac 100644 --- a/crates/aft/src/edit.rs +++ b/crates/aft/src/edit.rs @@ -1722,8 +1722,10 @@ pub fn compute_reformatted_excerpt( /// Returned by `write_format_validate` to give callers a single struct /// with all post-write signals for the response JSON. pub struct WriteResult { - /// Whether tree-sitter syntax validation passed. `None` if unsupported language. + /// Whether syntax validation passed. `None` if unsupported or unconfirmed. pub syntax_valid: Option, + /// Why a created file's compiler confirmation could not establish validity. + pub syntax_skipped_reason: Option<&'static str>, /// 1-based line of the first parse error, set only when this call created /// the file and its confirmed syntax check failed. Such a file is kept on /// disk as written (there is nothing to roll back to), and the reply tells @@ -1892,6 +1894,24 @@ impl WriteResult { if let Some(line) = self.syntax_error_line { result[CREATED_SYNTAX_ERROR_LINE_FIELD] = serde_json::json!(line); } + append_syntax_skipped_reason(result, self.syntax_skipped_reason); + } +} + +pub(crate) fn syntax_unknown_notice(reason: &str) -> String { + format!("Syntax check: unknown ({reason}); the new file was written, but its syntax could not be confirmed.") +} + +pub(crate) fn append_syntax_skipped_reason(result: &mut serde_json::Value, reason: Option<&str>) { + if let Some(reason) = reason { + result["syntax_skipped_reason"] = serde_json::json!(reason); + if !result["warnings"].is_array() { + result["warnings"] = serde_json::json!([]); + } + result["warnings"] + .as_array_mut() + .unwrap() + .push(serde_json::json!(syntax_unknown_notice(reason))); } } @@ -2084,6 +2104,7 @@ fn write_format_validate_impl( WRITE_PIPELINE_READS.with(|count| count.set(count.get() + 1)); let mut final_text = ObservedFinalText::read(path); let mut syntax_error_line = None; + let mut syntax_skipped_reason = None; let syntax_valid = final_text .as_ref() .map(|observed| observed.text.as_str()) @@ -2092,6 +2113,7 @@ fn write_format_validate_impl( if !existed { let check = check_created_file_syntax(path, source); syntax_error_line = check.first_error_line; + syntax_skipped_reason = check.skipped_reason; return check.valid; } let syntax_valid = validate_captured_syntax(path, source); @@ -2152,6 +2174,7 @@ fn write_format_validate_impl( Ok(WriteResult { syntax_valid, + syntax_skipped_reason, syntax_error_line, formatted, format_skipped_reason, @@ -2180,8 +2203,9 @@ fn parse_captured_syntax(path: &Path, source: &str) -> Option /// Syntax verdict for a file the current call created. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct CreatedFileSyntax { - /// `None` when the language has no parser. + /// `None` when the language has no parser or confirmation could not finish. pub valid: Option, + pub skipped_reason: Option<&'static str>, /// 1-based line of the first parse error. Set only when `valid` is /// `Some(false)` after any compiler confirmation. pub first_error_line: Option, @@ -2193,16 +2217,22 @@ pub(crate) struct CreatedFileSyntax { /// is kept on disk and reported to the agent instead. The error line comes /// from the same tree-sitter parse that produced the verdict. Swift files that /// tree-sitter rejects are confirmed with `swiftc -parse` first, so a file the -/// compiler accepts counts as valid and carries no error line. +/// compiler accepts counts as valid and carries no error line. An unavailable +/// compiler verdict is unknown and carries a reason instead of an error line. pub(crate) fn check_created_file_syntax(path: &Path, source: &str) -> CreatedFileSyntax { let Some(tree) = parse_captured_syntax(path, source) else { return CreatedFileSyntax { valid: None, + skipped_reason: None, first_error_line: None, }; }; let root = tree.root_node(); - let valid = swift::confirm_created(path, source, Some(!root.has_error())); + let (valid, skipped_reason) = + match swift::confirm_created(path, source, Some(!root.has_error())) { + Ok(valid) => (valid, None), + Err(reason) => (None, Some(reason)), + }; let first_error_line = if valid == Some(false) { first_syntax_error_line(root) } else { @@ -2210,6 +2240,7 @@ pub(crate) fn check_created_file_syntax(path: &Path, source: &str) -> CreatedFil }; CreatedFileSyntax { valid, + skipped_reason, first_error_line, } } diff --git a/crates/aft/src/edit/swift.rs b/crates/aft/src/edit/swift.rs index 7a2076af83..b156734d44 100644 --- a/crates/aft/src/edit/swift.rs +++ b/crates/aft/src/edit/swift.rs @@ -76,39 +76,42 @@ pub(super) fn confirm_if_needed( /// Confirm a tree-sitter error in a Swift file that the current call created. /// /// There is no original to compare against, so the compiler's verdict on the -/// new content decides directly. When `swiftc` is unavailable or fails, the -/// tree-sitter verdict stands. -pub(super) fn confirm_created(path: &Path, source: &str, is_valid: Option) -> Option { +/// new content decides directly. An unfinished confirmation is unknown, not +/// evidence that the new file contains a syntax error. +pub(super) fn confirm_created( + path: &Path, + source: &str, + is_valid: Option, +) -> Result, &'static str> { if path.extension().and_then(|ext| ext.to_str()) != Some("swift") || is_valid != Some(false) { - return is_valid; + return Ok(is_valid); } - confirm_with_compiler(path, source, is_valid) + compiler_verdict(path, source).map(Some) } fn confirm_with_compiler(path: &Path, source: &str, is_valid: Option) -> Option { + // Existing files keep the conservative rollback policy when confirmation + // cannot finish; newly created files have no original content to protect. + compiler_verdict(path, source).map(Some).unwrap_or(is_valid) +} + +fn compiler_verdict(path: &Path, source: &str) -> Result { let outcome = parse(source); + log::debug!( + "Swift syntax confirmation for {}: {outcome:?}", + path.display() + ); match outcome { - ParseOutcome::Exited(Some(0)) => { - log::debug!( - "swiftc confirmed syntax after tree-sitter errors: {}", - path.display() - ); - Some(true) - } + ParseOutcome::Exited(Some(0)) => Ok(true), + ParseOutcome::Exited(Some(1)) => Ok(false), + ParseOutcome::TimedOut => Err("swiftc_timeout"), + ParseOutcome::Missing => Err("swiftc_not_installed"), ParseOutcome::Unavailable(reason) => { - log::debug!( - "Swift syntax confirmation unavailable for {}: {reason}", - path.display() - ); - is_valid - } - other => { - log::debug!( - "Swift syntax confirmation failed for {}: {other:?}", - path.display() - ); - is_valid + log::debug!("Swift syntax confirmation unavailable: {reason}"); + Err("swiftc_unavailable") } + ParseOutcome::Exited(None) => Err("swiftc_terminated"), + ParseOutcome::Exited(Some(_)) => Err("swiftc_failed"), } } @@ -177,7 +180,7 @@ fn parse(source: &str) -> ParseOutcome { Ok(path) => path, Err(reason) => { log::debug!("Swift syntax confirmation lookup failed: {reason}"); - return ParseOutcome::Missing; + return ParseOutcome::Unavailable(reason.clone()); } }; let mut command = Command::new(path); @@ -457,7 +460,7 @@ mod tests { for (outcome, valid, line) in [ (ParseOutcome::Exited(Some(0)), Some(true), None), (ParseOutcome::Exited(Some(1)), Some(false), Some(1)), - (ParseOutcome::Missing, Some(false), Some(1)), + (ParseOutcome::Missing, None, None), ] { let calls = std::rc::Rc::new(std::cell::Cell::new(0)); let count = calls.clone(); @@ -492,6 +495,162 @@ mod tests { } } + fn created_write_response(source: &str, runner: Runner) -> serde_json::Value { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("new.swift"); + assert_eq!(validate_syntax_str(source, &path), Some(false)); + let ctx = crate::context::AppContext::new( + Box::new(crate::parser::TreeSitterProvider::new()), + Config { + format_on_edit: false, + ..Config::default() + }, + ); + let request = serde_json::from_value(serde_json::json!({ + "id": "created-swift", "command": "write", "file": path, "content": source + })) + .unwrap(); + let response = with_runner(runner, || { + crate::commands::write::handle_write(&request, &ctx) + }); + assert!(response.success, "{response:?}"); + assert_eq!(response.data["created"], true); + assert_eq!(response.data["rolled_back"], false); + assert_eq!(std::fs::read_to_string(path).unwrap(), source); + if response.data.get("syntax_skipped_reason").is_some() { + let text = crate::subc_format::format_response("write", &response, false); + assert!(text.contains("Syntax check: unknown"), "{text}"); + assert!(!text.contains("new file does not parse"), "{text}"); + } + response.data + } + + #[test] + fn swift_parser_created_timeout_is_unknown() { + let response = created_write_response(TUPLE, Box::new(|_| ParseOutcome::TimedOut)); + assert!(response.get("syntax_valid").is_none(), "{response:?}"); + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + assert_eq!(response["syntax_skipped_reason"], "swiftc_timeout"); + assert!( + response["warnings"].to_string().contains("unknown"), + "{response:?}" + ); + } + + #[test] + fn swift_parser_created_unavailable_is_unknown() { + for (outcome, reason) in [ + (ParseOutcome::Missing, "swiftc_not_installed"), + ( + ParseOutcome::Unavailable("permission denied".into()), + "swiftc_unavailable", + ), + (ParseOutcome::Exited(None), "swiftc_terminated"), + (ParseOutcome::Exited(Some(2)), "swiftc_failed"), + ] { + let mut outcome = Some(outcome); + let response = created_write_response( + TUPLE, + Box::new(move |_| outcome.take().expect("one compiler confirmation")), + ); + assert!(response.get("syntax_valid").is_none(), "{response:?}"); + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + assert_eq!(response["syntax_skipped_reason"], reason); + } + } + + #[test] + fn swift_parser_created_answer_is_definite() { + for (source, status, valid) in [(TUPLE, 0, true), (BROKEN, 1, false)] { + let response = created_write_response( + source, + Box::new(move |actual| { + assert_eq!(actual, source); + ParseOutcome::Exited(Some(status)) + }), + ); + assert_eq!(response["syntax_valid"], valid, "{response:?}"); + assert!( + response.get("syntax_skipped_reason").is_none(), + "{response:?}" + ); + if valid { + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + } else { + assert_eq!(response["syntax_error_line"], 1); + } + } + } + + #[test] + fn swift_parser_created_append_and_patch_report_unknown() { + for command in ["edit_match", "apply_patch"] { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("new.swift"); + let ctx = crate::context::AppContext::new( + Box::new(crate::parser::TreeSitterProvider::new()), + Config { + format_on_edit: false, + ..Config::default() + }, + ); + let params = if command == "edit_match" { + serde_json::json!({ "id": "append", "command": command, "file": path, "op": "append", "appendContent": TUPLE }) + } else { + serde_json::json!({ "id": "patch", "command": command, "patch_text": format!("*** Begin Patch\n*** Add File: {}\n+{}*** End Patch\n", path.display(), TUPLE) }) + }; + let request = serde_json::from_value(params).unwrap(); + let response = with_runner(Box::new(|_| ParseOutcome::TimedOut), || { + if command == "edit_match" { + crate::commands::edit_match::handle_edit_match(&request, &ctx) + } else { + crate::commands::apply_patch::handle_apply_patch(&request, &ctx) + } + }); + assert!(response.success, "{response:?}"); + assert_eq!(std::fs::read_to_string(path).unwrap(), TUPLE); + let data = if command == "edit_match" { + &response.data + } else { + assert!(response.data["output"] + .as_str() + .unwrap() + .contains("Syntax check: unknown")); + &response.data["metadata"]["files"][0] + }; + assert!( + data.get("syntax_valid") + .is_none_or(serde_json::Value::is_null), + "{data:?}" + ); + assert!(data.get("syntax_error_line").is_none(), "{data:?}"); + assert_eq!(data["syntax_skipped_reason"], "swiftc_timeout"); + } + } + + #[cfg(unix)] + #[test] + fn swift_parser_created_slow_checker_is_unknown() { + let response = created_write_response( + TUPLE, + Box::new(|source| { + assert_eq!(source, TUPLE); + let mut command = Command::new("/bin/sh"); + command.args(["-c", "exec sleep 30"]); + let outcome = parse_outcome(run_command( + command, + source, + Instant::now() + Duration::from_millis(50), + )); + assert!(matches!(outcome, ParseOutcome::TimedOut), "{outcome:?}"); + outcome + }), + ); + assert!(response.get("syntax_valid").is_none(), "{response:?}"); + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + assert_eq!(response["syntax_skipped_reason"], "swiftc_timeout"); + } + #[test] fn swift_parser_valid_to_valid_skips_compiler() { let calls = std::rc::Rc::new(std::cell::Cell::new(0)); @@ -653,6 +812,20 @@ mod tests { }; } + #[cfg(target_os = "macos")] + #[test] + fn swift_parser_native_created_file_is_valid() { + let Some(()) = with_native_swiftc(|| { + let check = crate::edit::check_created_file_syntax(Path::new("new.swift"), TUPLE); + assert_eq!(check.valid, Some(true), "{check:?}"); + assert_eq!(check.first_error_line, None); + assert_eq!(check.skipped_reason, None); + }) else { + eprintln!("skipping native Swift parser test: swiftc is unavailable"); + return; + }; + } + #[cfg(not(target_os = "macos"))] #[test] fn swift_parser_real_missing_binary_fallback() { diff --git a/crates/aft/src/subc_format.rs b/crates/aft/src/subc_format.rs index d8b733ef29..22bdd4df30 100644 --- a/crates/aft/src/subc_format.rs +++ b/crates/aft/src/subc_format.rs @@ -1271,6 +1271,14 @@ fn format_write_response(data: &Value) -> String { } append_lsp_error_lines(&mut output, data, true); append_lsp_server_notes(&mut output, data); + if data.get("created").and_then(Value::as_bool) == Some(true) + && data.get("syntax_valid").and_then(Value::as_bool).is_none() + { + if let Some(reason) = data.get("syntax_skipped_reason").and_then(Value::as_str) { + output.push_str("\n\n"); + output.push_str(&crate::edit::syntax_unknown_notice(reason)); + } + } output } diff --git a/crates/aft/tests/integration/created_file_syntax_test.rs b/crates/aft/tests/integration/created_file_syntax_test.rs index f9e3b36f50..7839814bb9 100644 --- a/crates/aft/tests/integration/created_file_syntax_test.rs +++ b/crates/aft/tests/integration/created_file_syntax_test.rs @@ -223,10 +223,10 @@ fn apply_patch_places_notice_under_the_broken_created_file_only() { } /// tree-sitter-swift rejects a parenthesized tuple metatype that `swiftc` -/// accepts. A new Swift file is confirmed with the compiler before it counts -/// as broken, so this file gets no syntax line. +/// accepts. Compiler confirmation may time out on a loaded runner; an +/// unfinished confirmation must be unknown rather than a definite syntax error. #[test] -fn swift_new_file_swiftc_accepts_has_no_syntax_line() { +fn swift_new_file_does_not_report_unconfirmed_syntax_error() { if !cfg!(target_os = "macos") { eprintln!("skipping: the swiftc confirmation test runs only on macOS, where swiftc ships with the Xcode toolchain"); return; @@ -253,9 +253,19 @@ fn swift_new_file_swiftc_accepts_has_no_syntax_line() { ); assert_eq!(resp["created"], true, "{resp:?}"); - assert_eq!(resp["syntax_valid"], true, "{resp:?}"); + if resp.get("syntax_valid").is_some() { + assert_eq!(resp["syntax_valid"], true, "{resp:?}"); + assert!(resp.get("syntax_skipped_reason").is_none(), "{resp:?}"); + } else { + assert!(resp["syntax_skipped_reason"].is_string(), "{resp:?}"); + assert!(text(&resp).contains("Syntax check: unknown"), "{resp:?}"); + } assert!(resp.get("syntax_error_line").is_none(), "{resp:?}"); - assert!(!text(&resp).contains("Syntax check"), "{}", text(&resp)); + assert!( + !text(&resp).contains("new file does not parse"), + "{}", + text(&resp) + ); assert!(aft.shutdown().success()); } From 7c77b555ef7bce75521344676dbfe6d77349aefd Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 18:01:26 +0200 Subject: [PATCH 10/17] mason: retry transient Swift compiler discovery Cache only a found absolute compiler path or definitive absence. xcrun timeout, spawn failure, signal termination, truncated/invalid output and other transient lookup errors leave discovery retryable. Preserve distinct lookup outcomes through parsing so a timed-out discovery reports unknown without poisoning later confirmations. Restore the established tree-sitter verdict and error line when swiftc is definitively missing, including the original created-file test row. Missing installation is not an unfinished compiler check. Shorten the release fragment to one plain sentence. Injected discovery tests cover timeout then successful lookup/compiler confirmation, successful-path reuse, definitive-negative caching, and transient retry. Portable xcrun outcome tests cover nonzero/empty output versus timeout/spawn/termination failures. Before-fix tests failed for both poisoned-cache recovery and missing-compiler compatibility. Mutation controls cached transient lookup failures and changed Missing back to unknown: each targeted guard reddened while its positive control stayed green; all 24 Swift tests passed after restore. Native macOS compiler/discovery follow-up remains required. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../next/swift-syntax-unknown.md | 2 +- crates/aft/src/edit/swift.rs | 325 +++++++++++++++--- 2 files changed, 284 insertions(+), 43 deletions(-) diff --git a/.alfonso/release-notes/next/swift-syntax-unknown.md b/.alfonso/release-notes/next/swift-syntax-unknown.md index 73d4fc7f10..5696111d3c 100644 --- a/.alfonso/release-notes/next/swift-syntax-unknown.md +++ b/.alfonso/release-notes/next/swift-syntax-unknown.md @@ -1 +1 @@ -- New Swift files now report syntax as unknown with a reason when compiler confirmation times out or is unavailable, instead of incorrectly reporting a definite syntax error; confirmed valid and invalid compiler verdicts are unchanged. +- A new Swift file is no longer reported as having a syntax error when the Swift compiler could not finish checking it; AFT reports the check as unknown instead. diff --git a/crates/aft/src/edit/swift.rs b/crates/aft/src/edit/swift.rs index b156734d44..9c4f4dc60d 100644 --- a/crates/aft/src/edit/swift.rs +++ b/crates/aft/src/edit/swift.rs @@ -11,7 +11,7 @@ use crate::format::{FormatError, SubprocessOutcome}; const PARSE_TIMEOUT: Duration = Duration::from_secs(5); #[cfg(all(test, target_os = "macos"))] const NATIVE_SWIFTC_TEST_TIMEOUT: Duration = Duration::from_secs(120); -static SWIFTC: OnceLock> = OnceLock::new(); +static SWIFTC: OnceLock = OnceLock::new(); #[cfg(all(test, target_os = "macos"))] thread_local! { @@ -48,6 +48,15 @@ enum ParseOutcome { Unavailable(String), } +#[derive(Clone, Debug, PartialEq, Eq)] +enum LookupOutcome { + Found(PathBuf), + Missing, + #[cfg(any(test, target_os = "macos"))] + TimedOut, + Unavailable(String), +} + pub(super) fn confirm_if_needed( path: &Path, source: &str, @@ -76,8 +85,9 @@ pub(super) fn confirm_if_needed( /// Confirm a tree-sitter error in a Swift file that the current call created. /// /// There is no original to compare against, so the compiler's verdict on the -/// new content decides directly. An unfinished confirmation is unknown, not -/// evidence that the new file contains a syntax error. +/// new content decides directly. Without an installed compiler, tree-sitter +/// remains the authority. An unfinished confirmation is unknown, not evidence +/// that the new file contains a syntax error. pub(super) fn confirm_created( path: &Path, source: &str, @@ -86,26 +96,26 @@ pub(super) fn confirm_created( if path.extension().and_then(|ext| ext.to_str()) != Some("swift") || is_valid != Some(false) { return Ok(is_valid); } - compiler_verdict(path, source).map(Some) + compiler_verdict(path, source).map(|verdict| verdict.or(is_valid)) } fn confirm_with_compiler(path: &Path, source: &str, is_valid: Option) -> Option { // Existing files keep the conservative rollback policy when confirmation // cannot finish; newly created files have no original content to protect. - compiler_verdict(path, source).map(Some).unwrap_or(is_valid) + compiler_verdict(path, source).unwrap_or(None).or(is_valid) } -fn compiler_verdict(path: &Path, source: &str) -> Result { +fn compiler_verdict(path: &Path, source: &str) -> Result, &'static str> { let outcome = parse(source); log::debug!( "Swift syntax confirmation for {}: {outcome:?}", path.display() ); match outcome { - ParseOutcome::Exited(Some(0)) => Ok(true), - ParseOutcome::Exited(Some(1)) => Ok(false), + ParseOutcome::Exited(Some(0)) => Ok(Some(true)), + ParseOutcome::Exited(Some(1)) => Ok(Some(false)), ParseOutcome::TimedOut => Err("swiftc_timeout"), - ParseOutcome::Missing => Err("swiftc_not_installed"), + ParseOutcome::Missing => Ok(None), ParseOutcome::Unavailable(reason) => { log::debug!("Swift syntax confirmation unavailable: {reason}"); Err("swiftc_unavailable") @@ -115,37 +125,80 @@ fn compiler_verdict(path: &Path, source: &str) -> Result { } } -fn find_swiftc() -> &'static Result { - SWIFTC.get_or_init(|| { - if let Ok(path) = which::which("swiftc") { - return Ok(path); +fn cached_lookup( + cache: &OnceLock, + lookup: impl FnOnce() -> LookupOutcome, +) -> LookupOutcome { + if let Some(outcome) = cache.get() { + return outcome.clone(); + } + let outcome = lookup(); + // A slow or failed lookup does not establish whether Swift is installed. + // Leave discovery retryable instead of poisoning the daemon's cache. + if matches!(outcome, LookupOutcome::Found(_) | LookupOutcome::Missing) { + cache.get_or_init(|| outcome).clone() + } else { + outcome + } +} + +fn find_swiftc() -> LookupOutcome { + cached_lookup(&SWIFTC, discover_swiftc) +} + +fn discover_swiftc() -> LookupOutcome { + match which::which("swiftc") { + Ok(path) if path.is_absolute() => return LookupOutcome::Found(path), + Ok(_) => { + return LookupOutcome::Unavailable("swiftc lookup returned a relative path".into()) } - #[cfg(target_os = "macos")] - { - let mut command = Command::new("xcrun"); - command.args(["--find", "swiftc"]); - match run_command(command, "", Instant::now() + parse_timeout()) { - Ok(result) if result.status.success() => { - let path = PathBuf::from(result.stdout.trim()); - if path.is_absolute() && path.is_file() { - return Ok(path); + Err(which::Error::CannotFindBinaryPath) => {} + Err(error) => return LookupOutcome::Unavailable(error.to_string()), + } + #[cfg(target_os = "macos")] + { + let mut command = Command::new("xcrun"); + command.args(["--find", "swiftc"]); + xcrun_lookup_outcome(run_command(command, "", Instant::now() + parse_timeout())) + } + #[cfg(not(target_os = "macos"))] + LookupOutcome::Missing +} + +#[cfg(any(test, target_os = "macos"))] +fn xcrun_lookup_outcome(result: Result) -> LookupOutcome { + match result { + Ok(result) => match result.status.code() { + Some(0) if result.truncated => { + LookupOutcome::Unavailable("xcrun output was truncated".into()) + } + Some(0) => { + let output = result.stdout.trim(); + if output.is_empty() { + LookupOutcome::Missing + } else { + let path = PathBuf::from(output); + if path.is_absolute() { + LookupOutcome::Found(path) + } else { + LookupOutcome::Unavailable("xcrun returned a relative swiftc path".into()) } - return Err("xcrun returned no swiftc path".into()); } - Ok(result) => return Err(format!("xcrun exited with {}", result.status)), - Err(error) => return Err(format!("xcrun lookup failed: {error}")), } - } - #[cfg(not(target_os = "macos"))] - Err("swiftc is not on PATH".into()) - }) + Some(_) => LookupOutcome::Missing, + None => LookupOutcome::Unavailable("xcrun was terminated".into()), + }, + Err(FormatError::Timeout { .. }) => LookupOutcome::TimedOut, + Err(error) => LookupOutcome::Unavailable(format!("xcrun lookup failed: {error}")), + } } #[cfg(all(test, target_os = "macos"))] pub(super) fn available() -> bool { with_parse_timeout(NATIVE_SWIFTC_TEST_TIMEOUT, || { - if let Err(reason) = find_swiftc() { - eprintln!("swiftc discovery failed: {reason}"); + let lookup = find_swiftc(); + if !matches!(lookup, LookupOutcome::Found(_)) { + eprintln!("swiftc discovery failed: {lookup:?}"); return false; } // Warm up the compiler under the same generous guard as the native tests. @@ -176,13 +229,24 @@ fn parse(source: &str) -> ParseOutcome { { return outcome; } - let path = match find_swiftc() { - Ok(path) => path, - Err(reason) => { - log::debug!("Swift syntax confirmation lookup failed: {reason}"); - return ParseOutcome::Unavailable(reason.clone()); - } - }; + parse_with_lookup(source, find_swiftc, run_swiftc) +} + +fn parse_with_lookup( + source: &str, + lookup: impl FnOnce() -> LookupOutcome, + checker: impl FnOnce(&Path, &str) -> ParseOutcome, +) -> ParseOutcome { + match lookup() { + LookupOutcome::Found(path) => checker(&path, source), + LookupOutcome::Missing => ParseOutcome::Missing, + #[cfg(any(test, target_os = "macos"))] + LookupOutcome::TimedOut => ParseOutcome::TimedOut, + LookupOutcome::Unavailable(reason) => ParseOutcome::Unavailable(reason), + } +} + +fn run_swiftc(path: &Path, source: &str) -> ParseOutcome { let mut command = Command::new(path); // Parsing stdin avoids temporary files. -parse performs no type checking or imports. command.args(["-parse", "-"]); @@ -460,7 +524,7 @@ mod tests { for (outcome, valid, line) in [ (ParseOutcome::Exited(Some(0)), Some(true), None), (ParseOutcome::Exited(Some(1)), Some(false), Some(1)), - (ParseOutcome::Missing, None, None), + (ParseOutcome::Missing, Some(false), Some(1)), ] { let calls = std::rc::Rc::new(std::cell::Cell::new(0)); let count = calls.clone(); @@ -540,7 +604,6 @@ mod tests { #[test] fn swift_parser_created_unavailable_is_unknown() { for (outcome, reason) in [ - (ParseOutcome::Missing, "swiftc_not_installed"), ( ParseOutcome::Unavailable("permission denied".into()), "swiftc_unavailable", @@ -559,6 +622,184 @@ mod tests { } } + #[test] + fn swift_parser_created_missing_keeps_tree_sitter_verdict() { + let response = created_write_response(TUPLE, Box::new(|_| ParseOutcome::Missing)); + assert_eq!(response["syntax_valid"], false, "{response:?}"); + assert_eq!(response["syntax_error_line"], 1); + assert!( + response.get("syntax_skipped_reason").is_none(), + "{response:?}" + ); + } + + #[test] + fn swift_parser_lookup_timeout_retries_then_confirms_created_file() { + let temp = tempfile::tempdir().unwrap(); + let compiler = temp.path().join("swiftc"); + let cache = std::rc::Rc::new(OnceLock::new()); + let lookups = std::rc::Rc::new(std::cell::Cell::new(0)); + let checks = std::rc::Rc::new(std::cell::Cell::new(0)); + for attempt in 0..3 { + let cache = cache.clone(); + let lookups = lookups.clone(); + let checks = checks.clone(); + let compiler = compiler.clone(); + let response = created_write_response( + TUPLE, + Box::new(move |source| { + parse_with_lookup( + source, + || { + cached_lookup(&cache, || { + lookups.set(lookups.get() + 1); + if lookups.get() == 1 { + LookupOutcome::TimedOut + } else { + LookupOutcome::Found(compiler.clone()) + } + }) + }, + |path, source| { + assert_eq!(path, compiler); + assert_eq!(source, TUPLE); + checks.set(checks.get() + 1); + ParseOutcome::Exited(Some(0)) + }, + ) + }), + ); + if attempt == 0 { + assert!(response.get("syntax_valid").is_none(), "{response:?}"); + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + assert_eq!(response["syntax_skipped_reason"], "swiftc_timeout"); + } else { + assert_eq!(response["syntax_valid"], true, "{response:?}"); + assert!(response.get("syntax_error_line").is_none(), "{response:?}"); + assert!( + response.get("syntax_skipped_reason").is_none(), + "{response:?}" + ); + } + } + assert_eq!(lookups.get(), 2, "retry once, then use the discovered path"); + assert_eq!( + checks.get(), + 2, + "a timed-out lookup must not run the compiler" + ); + } + + #[test] + fn swift_parser_lookup_caches_definitive_answers() { + let temp = tempfile::tempdir().unwrap(); + for outcome in [ + LookupOutcome::Found(temp.path().join("swiftc")), + LookupOutcome::Missing, + ] { + let cache = OnceLock::new(); + assert_eq!(cached_lookup(&cache, || outcome.clone()), outcome); + assert_eq!( + cached_lookup(&cache, || panic!("definitive answers are cached")), + outcome + ); + } + } + + #[test] + fn swift_parser_lookup_unavailable_is_retryable() { + let cache = OnceLock::new(); + let outcome = cached_lookup(&cache, || LookupOutcome::Unavailable("spawn denied".into())); + assert!(matches!(outcome, LookupOutcome::Unavailable(_))); + assert!( + cache.get().is_none(), + "a transient error must not poison discovery" + ); + assert_eq!( + cached_lookup(&cache, || LookupOutcome::Missing), + LookupOutcome::Missing + ); + } + + #[cfg(any(unix, windows))] + #[test] + fn swift_parser_xcrun_lookup_distinguishes_definitive_answers() { + let output = |code, stdout: String, truncated| { + #[cfg(unix)] + let status = { + use std::os::unix::process::ExitStatusExt; + std::process::ExitStatus::from_raw(code << 8) + }; + #[cfg(windows)] + let status = { + use std::os::windows::process::ExitStatusExt; + std::process::ExitStatus::from_raw(code as u32) + }; + Ok(SubprocessOutcome { + stdout, + stderr: String::new(), + status, + truncated, + }) + }; + let temp = tempfile::tempdir().unwrap(); + let compiler = temp.path().join("swiftc"); + assert_eq!( + xcrun_lookup_outcome(output(0, compiler.display().to_string(), false)), + LookupOutcome::Found(compiler) + ); + assert_eq!( + xcrun_lookup_outcome(output(0, " \n".into(), false)), + LookupOutcome::Missing + ); + assert_eq!( + xcrun_lookup_outcome(output(1, String::new(), false)), + LookupOutcome::Missing + ); + assert!(matches!( + xcrun_lookup_outcome(output(0, "relative/swiftc".into(), false)), + LookupOutcome::Unavailable(_) + )); + assert!(matches!( + xcrun_lookup_outcome(output(0, String::new(), true)), + LookupOutcome::Unavailable(_) + )); + assert_eq!( + xcrun_lookup_outcome(Err(FormatError::Timeout { + tool: "xcrun".into(), + timeout_secs: 5 + })), + LookupOutcome::TimedOut + ); + for error in [ + FormatError::NotFound { + tool: "xcrun".into(), + }, + FormatError::Failed { + tool: "xcrun".into(), + stderr: "spawn denied".into(), + }, + ] { + assert!(matches!( + xcrun_lookup_outcome(Err(error)), + LookupOutcome::Unavailable(_) + )); + } + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt; + assert!(matches!( + xcrun_lookup_outcome(Ok(SubprocessOutcome { + stdout: String::new(), + stderr: String::new(), + status: std::process::ExitStatus::from_raw(libc::SIGKILL), + truncated: false, + })), + LookupOutcome::Unavailable(_) + )); + } + } + #[test] fn swift_parser_created_answer_is_definite() { for (source, status, valid) in [(TUPLE, 0, true), (BROKEN, 1, false)] { @@ -829,7 +1070,7 @@ mod tests { #[cfg(not(target_os = "macos"))] #[test] fn swift_parser_real_missing_binary_fallback() { - if find_swiftc().is_ok() { + if matches!(find_swiftc(), LookupOutcome::Found(_)) { eprintln!("skipping missing-swiftc test: swiftc is available on PATH"); return; } @@ -894,7 +1135,7 @@ mod tests { #[test] fn swift_parser_available_fails_for_discovered_broken_compiler() { with_parse_timeout(NATIVE_SWIFTC_TEST_TIMEOUT, || { - if find_swiftc().is_err() { + if !matches!(find_swiftc(), LookupOutcome::Found(_)) { eprintln!("skipping native Swift parser test: swiftc is unavailable"); return; } From 0bfaf9f0831456d55aea4069395c94a7a7f2c1fd Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:21:07 +0200 Subject: [PATCH 11/17] mason: tolerate in-flight Windows replacement in frozen LSP test Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/subc/mod.rs | 63 +++++++++++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/crates/aft/src/subc/mod.rs b/crates/aft/src/subc/mod.rs index 2bbebb0936..c331387d63 100644 --- a/crates/aft/src/subc/mod.rs +++ b/crates/aft/src/subc/mod.rs @@ -17624,6 +17624,58 @@ mod tests { assert_eq!(bind_response.data["code"], "request_cancelled"); } + fn frozen_lsp_edit_write_is_visible( + file_len: std::io::Result, + edit_in_flight: bool, + ) -> std::io::Result { + match file_len { + Ok(len) => Ok(len >= 1024 * 1024), + // On Windows, replacing a file can briefly make its path return + // NotFound. Retry only while the edit is in flight; absence is not + // evidence that the replacement bytes have been written. + Err(error) if edit_in_flight && error.kind() == std::io::ErrorKind::NotFound => { + Ok(false) + } + Err(error) => Err(error), + } + } + + #[test] + fn frozen_lsp_write_poll_retries_not_found_only_in_flight() { + let missing = || std::io::Error::from(std::io::ErrorKind::NotFound); + assert!(!frozen_lsp_edit_write_is_visible(Err(missing()), true).unwrap()); + assert_eq!( + frozen_lsp_edit_write_is_visible(Err(missing()), false) + .unwrap_err() + .kind(), + std::io::ErrorKind::NotFound + ); + } + + #[test] + fn frozen_lsp_write_poll_requires_large_file() { + for in_flight in [false, true] { + assert!(!frozen_lsp_edit_write_is_visible(Ok(0), in_flight).unwrap()); + assert!(!frozen_lsp_edit_write_is_visible(Ok(1024 * 1024 - 1), in_flight).unwrap()); + assert!(frozen_lsp_edit_write_is_visible(Ok(1024 * 1024), in_flight).unwrap()); + } + } + + #[test] + fn frozen_lsp_write_poll_rejects_other_errors() { + for in_flight in [false, true] { + assert_eq!( + frozen_lsp_edit_write_is_visible( + Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)), + in_flight, + ) + .unwrap_err() + .kind(), + std::io::ErrorKind::PermissionDenied + ); + } + } + #[tokio::test] async fn frozen_lsp_edit_releases_writer_before_route_bind_deadline() { use crate::lsp::client::LspEvent; @@ -17685,7 +17737,16 @@ mod tests { Box::new(move |ctx| crate::commands::edit_match::handle_edit_match(&request, ctx)), ); let write_deadline = Instant::now() + Duration::from_secs(5); - while std::fs::metadata(&file).unwrap().len() < 1024 * 1024 { + loop { + let edit_in_flight = edit.is_empty(); + if frozen_lsp_edit_write_is_visible( + std::fs::metadata(&file).map(|metadata| metadata.len()), + edit_in_flight, + ) + .unwrap() + { + break; + } assert!(Instant::now() < write_deadline, "edit never wrote the file"); std::thread::sleep(Duration::from_millis(5)); } From 23eca82b43f5f984c624b81a9fdf23a532585523 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 19:47:58 +0200 Subject: [PATCH 12/17] search-quality descriptor for batch-1020k Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../aft-search/slice-descriptors/land-batch-1020k.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 benchmarks/aft-search/slice-descriptors/land-batch-1020k.json diff --git a/benchmarks/aft-search/slice-descriptors/land-batch-1020k.json b/benchmarks/aft-search/slice-descriptors/land-batch-1020k.json new file mode 100644 index 0000000000..ebd574711e --- /dev/null +++ b/benchmarks/aft-search/slice-descriptors/land-batch-1020k.json @@ -0,0 +1,8 @@ +{ + "targeted_mechanism": "none", + "kind": "harness", + "slice_class": "non_ranking", + "fixtures": [ + "harness-goldens" + ] +} From 23f823a32505b74b6d629bbd68c5cbd69af12afc Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:42:01 +0200 Subject: [PATCH 13/17] mason: preserve watcher on inconclusive root checks Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/context.rs | 63 +++++++++++++++++++++++++++++---- crates/aft/src/runtime_drain.rs | 36 +++++++++++++++++++ 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/crates/aft/src/context.rs b/crates/aft/src/context.rs index 8b0b38045b..8fd065a386 100644 --- a/crates/aft/src/context.rs +++ b/crates/aft/src/context.rs @@ -1742,6 +1742,7 @@ pub(crate) fn watcher_counters_for_root(root: &Path) -> Arc { #[derive(Debug)] enum DeletedRootCleanup { Pending(u64), + Inconclusive(u64), Complete(u64), } @@ -5651,10 +5652,30 @@ impl AppContext { ) } - /// Clean up a missing root without admitting ordinary work on that root. - /// Returns true when the drain should stop, including when the LSP - /// manager is busy and cleanup must be retried on a later drain call. + /// After confirming the project root is gone, this method drops its watcher + /// and shuts down its language-server clients. + /// Returns true when the drain should stop, including while a root check is + /// inconclusive or cleanup must be retried on a later drain call. pub(crate) fn try_cleanup_deleted_root(&self, expected_generation: u64) -> bool { + self.try_cleanup_deleted_root_with_probe_impl(expected_generation, |path| { + std::fs::metadata(path) + }) + } + + #[cfg(test)] + pub(crate) fn try_cleanup_deleted_root_with_probe( + &self, + expected_generation: u64, + root_metadata: impl FnOnce(&Path) -> std::io::Result, + ) -> bool { + self.try_cleanup_deleted_root_with_probe_impl(expected_generation, root_metadata) + } + + fn try_cleanup_deleted_root_with_probe_impl( + &self, + expected_generation: u64, + root_metadata: impl FnOnce(&Path) -> std::io::Result, + ) -> bool { self.subc_lifecycle .run_if_current( self.configure_generation.as_ref(), @@ -5667,9 +5688,33 @@ impl AppContext { return false; }; let mut cleanup = self.deleted_root_cleanup.lock(); - if root.is_dir() { - *cleanup = None; - return false; + match root_metadata(&root) { + Ok(metadata) if metadata.is_dir() => { + *cleanup = None; + return false; + } + Ok(_) => {} + Err(error) + if matches!( + error.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::NotADirectory + ) => {} + Err(error) => { + let already_logged = matches!( + *cleanup, + Some(DeletedRootCleanup::Inconclusive(generation)) + if generation == expected_generation + ); + if !already_logged { + crate::slog_warn!( + "project root check inconclusive {}: {}; preserving watcher and language servers", + root.display(), + error + ); + } + *cleanup = Some(DeletedRootCleanup::Inconclusive(expected_generation)); + return true; + } } if matches!(*cleanup, Some(DeletedRootCleanup::Complete(generation)) if generation == expected_generation) { return true; @@ -5793,7 +5838,11 @@ impl AppContext { /// never add one: use `try_lock` and treat contention as pending work. pub fn watcher_drain_has_work(&self) -> bool { let cleanup_pending = self.deleted_root_cleanup.try_lock().is_none_or(|cleanup| { - matches!(*cleanup, Some(DeletedRootCleanup::Pending(generation)) if generation == self.configure_generation()) + matches!( + *cleanup, + Some(DeletedRootCleanup::Pending(generation) | DeletedRootCleanup::Inconclusive(generation)) + if generation == self.configure_generation() + ) }); let receiver_pending = self .watcher_rx diff --git a/crates/aft/src/runtime_drain.rs b/crates/aft/src/runtime_drain.rs index 093f22124b..d2caeb59fb 100644 --- a/crates/aft/src/runtime_drain.rs +++ b/crates/aft/src/runtime_drain.rs @@ -8947,6 +8947,42 @@ mod idle_lsp_tests { assert!(wait_until_dead(pid, Duration::from_secs(6))); } + #[test] + fn deleted_root_inconclusive_check_preserves_runtime_until_root_is_present() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("project"); + let (ctx, _tx) = deleted_root_context(&root); + let pid = spawn_sleep_client(&ctx, root.clone()); + let generation = ctx.configure_generation(); + + let (_, logs) = crate::logging::capture_log_lines(|| { + for _ in 0..2 { + assert!(ctx.try_cleanup_deleted_root_with_probe(generation, |_| { + Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "injected root metadata failure", + )) + })); + } + }); + + assert_eq!(ctx.lsp().server_count(), 1); + assert!(ctx.watcher_rx().lock().is_some()); + assert!(crate::bash_background::process::is_process_alive(pid)); + assert!(ctx.watcher_drain_has_work()); + let inconclusive_logs = logs + .iter() + .filter(|line| line.contains("project root check inconclusive")) + .collect::>(); + assert_eq!(inconclusive_logs.len(), 1, "{logs:?}"); + + let outcome = drain_watcher_events_bounded(&ctx, 1); + assert!(!outcome.has_more); + assert!(ctx.watcher_rx().lock().is_some()); + assert_eq!(ctx.lsp().server_count(), 1); + assert!(crate::bash_background::process::is_process_alive(pid)); + } + #[test] fn deleted_root_cleanup_defers_contended_watcher_receiver() { let tmp = tempfile::tempdir().unwrap(); From 4246de1d56fa6f13affb1644a7893190d650588e Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:36:29 +0200 Subject: [PATCH 14/17] mason: reuse verified resident semantic chunks before SQLite reads Index only validated chunk-row vectors with weak, bounded handles; legacy file payloads can contain independently chosen floats and must not seed this index. Share verified allocations with file runs so unchanged chunks survive one-file edits without payload reads. Keep existing unchanged-file generation reuse and count semantic I/O at production sites. Same-epoch touch already avoided row updates; now skip the write transaction too. Update large_semantic_materialization_batches_pins_and_touches from one transaction to zero because freshly stored keys need no writer, retaining the pin-byte and blob-identity assertions. Measured 300-chunk one-line edit: 366 gets and 127960 payload bytes without shortcut, 4 misses and zero payload bytes with it; canonical file payloads match cold rebuild. Repository-linked fixture: one-file edit 26 gets/79940 bytes -> 4/0; ten-file edit 312/1014310 -> 32/0; no-change zero reads/writes. Three restored mutations reddened resident-key, incremental-content, and same-epoch writer guards. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../release-notes/next/semantic-fill-io.md | 1 + crates/aft/src/blob_store/v2.rs | 78 +++-- crates/aft/src/blob_store/v2/read_tests.rs | 47 +++ crates/aft/src/views/io.rs | 18 ++ crates/aft/src/views/semantic.rs | 50 +++- crates/aft/src/views/semantic_arena.rs | 103 +++++-- crates/aft/src/views/semantic_read_tests.rs | 271 +++++++++++++++++- crates/aft/src/views/semantic_runtime.rs | 6 +- .../aft/src/views/semantic_runtime_tests.rs | 3 +- 9 files changed, 519 insertions(+), 58 deletions(-) create mode 100644 .alfonso/release-notes/next/semantic-fill-io.md diff --git a/.alfonso/release-notes/next/semantic-fill-io.md b/.alfonso/release-notes/next/semantic-fill-io.md new file mode 100644 index 0000000000..7a9991607c --- /dev/null +++ b/.alfonso/release-notes/next/semantic-fill-io.md @@ -0,0 +1 @@ +Semantic view fills reuse resident chunk vectors and unchanged files while avoiding same-epoch touch write transactions, reducing disk I/O across linked checkouts. diff --git a/crates/aft/src/blob_store/v2.rs b/crates/aft/src/blob_store/v2.rs index 623b2f865f..f7ef1e38a9 100644 --- a/crates/aft/src/blob_store/v2.rs +++ b/crates/aft/src/blob_store/v2.rs @@ -515,32 +515,25 @@ impl FamilyStore { Ok(outcome) } - /// Stamps the current epoch on every stored key in one transaction and - /// reports the ones that are gone. + /// Stamps the current epoch on stored keys and reports missing keys. A + /// covering-index read avoids opening a writer when every epoch is current. pub fn touch(&self, keys: &[FamilyKey]) -> StoreResult { for key in keys { self.ensure_plane(key)?; } - let mut connection = lock(&self.inner.connection); - let tx = connection.transaction_with_behavior(TransactionBehavior::Immediate)?; - #[cfg(test)] - TOUCH_TRANSACTIONS.with(|count| count.set(count.get() + 1)); - let epoch = read_epoch(&tx)?; - let mut report = TouchReport { - ref_epoch: epoch, - ..TouchReport::default() - }; - { - // WITHOUT ROWID stores the payload in the primary-key tree. Even - // an unchanged UPDATE can read and rebuild its overflow pages. - // Most touches are in the same GC epoch; use the narrow covering - // index to check both presence and epoch before visiting that tree. - let mut lookup = tx.prepare( + fn inspect( + connection: &rusqlite::Connection, + keys: &[FamilyKey], + ) -> StoreResult<(TouchReport, Vec)> { + let epoch = read_epoch(connection)?; + let mut report = TouchReport { + ref_epoch: epoch, + ..TouchReport::default() + }; + let mut outdated = Vec::new(); + let mut lookup = connection.prepare( "SELECT ref_epoch FROM blob_payloads INDEXED BY blob_membership WHERE full_key = ?1", )?; - let mut update = tx.prepare( - "UPDATE blob_payloads SET ref_epoch = MAX(ref_epoch, ?2) WHERE full_key = ?1", - )?; for key in keys { let stored: Option = lookup .query_row(params![key.as_bytes().as_slice()], |row| row.get(0)) @@ -548,15 +541,48 @@ impl FamilyStore { match stored { Some(stored) => { if stored < epoch as i64 { - update.execute(params![key.as_bytes().as_slice(), epoch as i64])?; + outdated.push(*key); } report.touched += 1; } None => report.missing.push(*key), } } + Ok((report, outdated)) + } + let mut connection = lock(&self.inner.connection); + let read = connection.transaction()?; + let (report, outdated) = inspect(&read, keys)?; + read.commit()?; + if outdated.is_empty() { + return Ok(report); + } + // Another process may advance GC or remove a row between transactions. + // Recheck presence and the epoch after taking the write lock. + let tx = connection.transaction_with_behavior(TransactionBehavior::Immediate)?; + #[cfg(test)] + TOUCH_TRANSACTIONS.with(|count| count.set(count.get() + 1)); + let (report, outdated) = inspect(&tx, keys)?; + let mut written = 0; + { + let mut update = tx.prepare( + "UPDATE blob_payloads SET ref_epoch = ?2 WHERE full_key = ?1 AND ref_epoch < ?2", + )?; + for key in outdated { + written += + update.execute(params![key.as_bytes().as_slice(), report.ref_epoch as i64])?; + } } tx.commit()?; + if self.plane() == FamilyPlane::Semantic { + use std::sync::atomic::Ordering; + crate::views::io::WORK + .semantic_touch_transactions + .fetch_add(1, Ordering::Relaxed); + crate::views::io::WORK + .semantic_touch_rows_written + .fetch_add(written as u64, Ordering::Relaxed); + } Ok(report) } @@ -1075,6 +1101,11 @@ fn get(inner: &StoreInner, key: &FamilyKey) -> StoreResult>> { key: key.plane(), }); } + if inner.plane == FamilyPlane::Semantic { + crate::views::io::WORK + .semantic_store_gets + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } let row: Option<(Vec, Vec, i64)> = lock(&inner.connection) .query_row( "SELECT payload, payload_digest, payload_schema FROM blob_payloads WHERE full_key = ?1", @@ -1085,6 +1116,11 @@ fn get(inner: &StoreInner, key: &FamilyKey) -> StoreResult>> { let Some((payload, digest, schema)) = row else { return Ok(None); }; + if inner.plane == FamilyPlane::Semantic { + crate::views::io::WORK + .semantic_store_payload_bytes + .fetch_add(payload.len() as u64, std::sync::atomic::Ordering::Relaxed); + } if digest.as_slice() != blake3::hash(&payload).as_bytes() || schema != i64::from(inner.plane.payload_schema()) { diff --git a/crates/aft/src/blob_store/v2/read_tests.rs b/crates/aft/src/blob_store/v2/read_tests.rs index 83fa9689f8..4daed4f496 100644 --- a/crates/aft/src/blob_store/v2/read_tests.rs +++ b/crates/aft/src/blob_store/v2/read_tests.rs @@ -193,6 +193,53 @@ fn total_changes(store: &FamilyStore) -> u64 { .unwrap() } +#[test] +fn touch_same_epoch_opens_no_write_transaction() { + let dir = tempfile::tempdir().unwrap(); + let store = FamilyStore::open( + &StoreWriteAccess::for_registered_view(dir.path(), "touch-no-write"), + FamilyPlane::Semantic, + ) + .unwrap(); + let key = FamilyKey::new(FamilyPlane::Semantic, [1; 32]); + store.put_or_touch(&key, b"payload").unwrap(); + take_touch_transactions(); + let before = total_changes(&store); + assert_eq!(store.touch(&[key, key]).unwrap().touched, 2); + assert_eq!(store.touch(&[]).unwrap().touched, 0); + assert_eq!(total_changes(&store), before); + assert_eq!( + take_touch_transactions(), + 0, + "same-epoch touch opened a writer" + ); +} + +#[test] +fn touch_old_epoch_updates_each_row_once() { + let dir = tempfile::tempdir().unwrap(); + let store = FamilyStore::open( + &StoreWriteAccess::for_registered_view(dir.path(), "touch-advance"), + FamilyPlane::Semantic, + ) + .unwrap(); + let key = FamilyKey::new(FamilyPlane::Semantic, [1; 32]); + let missing = FamilyKey::new(FamilyPlane::Semantic, [2; 32]); + store.put_or_touch(&key, b"payload").unwrap(); + let epoch = store.raise_epoch(1).unwrap(); + take_touch_transactions(); + let before = total_changes(&store); + let report = store.touch(&[key, missing, key]).unwrap(); + assert_eq!(report.ref_epoch, epoch); + assert_eq!(report.touched, 2); + assert_eq!(report.missing, vec![missing]); + assert_eq!(total_changes(&store) - before, 1); + assert_eq!(take_touch_transactions(), 1); + assert!(!store.delete_if_unreferenced_since(&key, epoch).unwrap()); + store.touch(&[key, missing]).unwrap(); + assert_eq!(take_touch_transactions(), 0); +} + #[test] fn put_or_touch_same_epoch_reuse_writes_no_rows() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/aft/src/views/io.rs b/crates/aft/src/views/io.rs index 9cdbafb1c2..12803daaf5 100644 --- a/crates/aft/src/views/io.rs +++ b/crates/aft/src/views/io.rs @@ -21,6 +21,12 @@ pub(crate) struct WorkCounters { pub dispatch_sites_resolved: AtomicU64, pub dispatch_full_passes: AtomicU64, pub dispatch_type_target_files: AtomicU64, + pub semantic_store_gets: AtomicU64, + pub semantic_store_payload_bytes: AtomicU64, + pub semantic_touch_rows_written: AtomicU64, + pub semantic_touch_transactions: AtomicU64, + pub semantic_fills: AtomicU64, + pub semantic_files_filled: AtomicU64, } pub(crate) static WORK: WorkCounters = WorkCounters { @@ -37,6 +43,12 @@ pub(crate) static WORK: WorkCounters = WorkCounters { dispatch_sites_resolved: AtomicU64::new(0), dispatch_full_passes: AtomicU64::new(0), dispatch_type_target_files: AtomicU64::new(0), + semantic_store_gets: AtomicU64::new(0), + semantic_store_payload_bytes: AtomicU64::new(0), + semantic_touch_rows_written: AtomicU64::new(0), + semantic_touch_transactions: AtomicU64::new(0), + semantic_fills: AtomicU64::new(0), + semantic_files_filled: AtomicU64::new(0), }; /// Hash source bytes, counting repeated hashes as repeated work. Payload and @@ -66,6 +78,12 @@ pub(crate) fn work_snapshot() -> serde_json::Value { "dispatch_sites_resolved": WORK.dispatch_sites_resolved.load(Ordering::Relaxed), "dispatch_full_passes": WORK.dispatch_full_passes.load(Ordering::Relaxed), "dispatch_type_target_files": WORK.dispatch_type_target_files.load(Ordering::Relaxed), + "semantic_store_gets": WORK.semantic_store_gets.load(Ordering::Relaxed), + "semantic_store_payload_bytes": WORK.semantic_store_payload_bytes.load(Ordering::Relaxed), + "semantic_touch_rows_written": WORK.semantic_touch_rows_written.load(Ordering::Relaxed), + "semantic_touch_transactions": WORK.semantic_touch_transactions.load(Ordering::Relaxed), + "semantic_fills": WORK.semantic_fills.load(Ordering::Relaxed), + "semantic_files_filled": WORK.semantic_files_filled.load(Ordering::Relaxed), }) } diff --git a/crates/aft/src/views/semantic.rs b/crates/aft/src/views/semantic.rs index 09d63da7ca..a334cc1f24 100644 --- a/crates/aft/src/views/semantic.rs +++ b/crates/aft/src/views/semantic.rs @@ -291,6 +291,8 @@ pub struct FillReport { /// Chunks in claimed file versions, including reused chunks. pub chunks: usize, pub chunk_reused: usize, + /// Successful chunk lookups in memory, including admission rechecks. + pub chunk_resident_hits: usize, pub source_reads: usize, pub source_bytes: u64, /// Completions installed into the fill map. @@ -704,6 +706,9 @@ impl SemanticPlane { self.work_items(&state, &resident, snapshot) }; report.queued = items.len(); + super::io::WORK + .semantic_fills + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); crate::cold_build_limiter::progress::remaining(items.len()); report.deferred = items.len().saturating_sub(budget.max_files); items.truncate(budget.max_files); @@ -745,6 +750,9 @@ impl SemanticPlane { crate::project_content::read(&source) }; report.source_reads += 1; + super::io::WORK + .semantic_files_filled + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); report.source_bytes += bytes.as_ref().map_or(0, |bytes| bytes.len() as u64); match bytes { Ok(bytes) if ContentHash::of(&bytes) == item.content => { @@ -880,9 +888,18 @@ impl SemanticPlane { to_embed.push(item); chunks.push(file_chunks); } + let mut canonical_chunks = Vec::new(); let embedded = { let mut rows = |batch: Vec| { - self.embed_chunks(&arena, &store, batch, embed, &mut report, before_embed) + self.embed_chunks( + &arena, + &store, + batch, + embed, + &mut report, + before_embed, + &mut canonical_chunks, + ) }; crate::semantic_index::embed_view_files_with_rows( chunks, @@ -1041,6 +1058,7 @@ impl SemanticPlane { embed: &mut F, report: &mut FillReport, before_embed: &mut dyn FnMut() -> Result<(), String>, + canonical_chunks: &mut Vec>, ) -> Result, String> where F: FnMut(Vec) -> Result>, String>, @@ -1066,13 +1084,22 @@ impl SemanticPlane { let mut rows = Vec::with_capacity(texts.len()); let mut misses = Vec::new(); for (text, key) in texts.iter().zip(&keys) { - if let Some(vector) = load_chunk(store, key, &identity, text) - .map_err(|error| chunk_store_error(error, report))? - { + let resident = arena.chunk_vector(key); + if resident.is_some() { + report.chunk_resident_hits += 1; + } + let vector = match resident { + Some(vector) => Some(vector), + None => load_chunk(store, key, &identity, text) + .map_err(|error| chunk_store_error(error, report))? + .map(|vector| arena.remember_chunk(*key, vector)), + }; + if let Some(vector) = vector { rows.push(Some(BuildEmbeddingRow::Embedded { embedded_text: text.clone(), - vector, + vector: vector.to_vec(), })); + canonical_chunks.push(vector); } else { misses.push(text.clone()); rows.push(None); @@ -1111,8 +1138,9 @@ impl SemanticPlane { if embedded_text == text { let store_started = Instant::now(); let payload = encode_chunk_vector(&identity, text, vector)?; - match store.put_or_touch(key, &payload) { - Ok(_) => {} + let cacheable = match store.put_or_touch(key, &payload) { + Ok(PutOrTouch::Inserted { .. } | PutOrTouch::Reused { .. }) => true, + Ok(PutOrTouch::Quarantined) => false, Err(crate::blob_store::v2::StoreError::ConflictingPayload(_)) => { // A different process may have won. Only adopt // its floats after the same exact-text check. @@ -1120,9 +1148,15 @@ impl SemanticPlane { .map_err(|error| chunk_store_error(error, report))? { *vector = stored; + true + } else { + false } } Err(error) => return Err(chunk_store_error(error, report)), + }; + if cacheable { + canonical_chunks.push(arena.remember_chunk(*key, vector.clone())); } report.timing.store_write += store_started.elapsed(); } @@ -2385,6 +2419,7 @@ mod tests { &mut |_| Ok(vec![vector.clone()]), &mut FillReport::default(), &mut || Ok(()), + &mut Vec::new(), ) .unwrap(); let key = producer.chunk_key(text); @@ -2479,6 +2514,7 @@ mod tests { &mut |texts| Model::default().embed("model-a", texts), &mut report, &mut || Ok(()), + &mut Vec::new(), ) .unwrap(); match &result[0] { diff --git a/crates/aft/src/views/semantic_arena.rs b/crates/aft/src/views/semantic_arena.rs index d8f635932d..f2ca52ceea 100644 --- a/crates/aft/src/views/semantic_arena.rs +++ b/crates/aft/src/views/semantic_arena.rs @@ -45,6 +45,8 @@ pub struct SemanticArena { decodes: AtomicU64, store_reads: AtomicU64, chunk_batch: Mutex<()>, + #[cfg(test)] + chunk_shortcut: std::sync::atomic::AtomicBool, } /// Resident arena memory. Every key is counted once, whatever the number of @@ -69,39 +71,47 @@ pub struct ArenaMemory { #[derive(Debug, Default)] struct VectorPool { by_digest: HashMap>, + // Populate only from chunk-row vectors that were read or stored; a file + // payload can contain different legacy floats. + by_chunk: HashMap>, live_after_prune: usize, scratch: Vec, } impl VectorPool { - /// Points every vector of `run` at an equal resident vector when there is - /// one, and records the rest. Equality is bitwise, so scores computed - /// from a shared vector are exactly those of the vector it replaced. + /// Shares only bit-identical vectors, preserving cached norms and scores. fn share(&mut self, run: &mut SemanticVectors) { for vector in run.vectors_mut() { - self.scratch.clear(); - for value in vector.iter() { - self.scratch - .extend_from_slice(&value.to_bits().to_le_bytes()); - } - let digest = blake3::hash(&self.scratch); - match self.by_digest.get(&digest).and_then(Weak::upgrade) { - Some(resident) if same_bits(&resident, vector) => *vector = resident, - // A digest collision between different floats: keep both. - Some(_) => {} - None => { - self.by_digest.insert(digest, Arc::downgrade(vector)); - } - } + self.share_vector(vector); } - if self.by_digest.len() > self.live_after_prune.max(1024).saturating_mul(2) { + if self.by_digest.len() + self.by_chunk.len() + > self.live_after_prune.max(1024).saturating_mul(2) + { self.prune(); } } + fn share_vector(&mut self, vector: &mut Arc<[f32]>) { + self.scratch.clear(); + for value in vector.iter() { + self.scratch + .extend_from_slice(&value.to_bits().to_le_bytes()); + } + let digest = blake3::hash(&self.scratch); + match self.by_digest.get(&digest).and_then(Weak::upgrade) { + Some(resident) if same_bits(&resident, vector) => *vector = resident, + // Keep unequal allocations on a digest collision. + Some(_) => {} + None => { + self.by_digest.insert(digest, Arc::downgrade(vector)); + } + } + } + fn prune(&mut self) { self.by_digest.retain(|_, vector| vector.strong_count() > 0); - self.live_after_prune = self.by_digest.len(); + self.by_chunk.retain(|_, vector| vector.strong_count() > 0); + self.live_after_prune = self.by_digest.len() + self.by_chunk.len(); } } @@ -145,6 +155,8 @@ pub fn arena_for(storage: &Path, family: &str) -> Arc { decodes: AtomicU64::new(0), store_reads: AtomicU64::new(0), chunk_batch: Mutex::new(()), + #[cfg(test)] + chunk_shortcut: std::sync::atomic::AtomicBool::new(true), }); arenas.insert(key, Arc::downgrade(&arena)); arena @@ -172,6 +184,22 @@ impl Drop for ArenaClaim { } impl SemanticArena { + #[cfg(test)] + pub(crate) fn set_chunk_shortcut(&self, enabled: bool) { + self.chunk_shortcut.store(enabled, Ordering::Relaxed); + } + + /// Looks up the key derived from the producer identity and exact text. + /// Weak handles do not keep floats alive. Cold file loads must first validate + /// chunk rows before those vectors become eligible for this shortcut. + pub(crate) fn chunk_vector(&self, key: &FamilyKey) -> Option> { + #[cfg(test)] + if !self.chunk_shortcut.load(Ordering::Relaxed) { + return None; + } + lock(&self.vectors).by_chunk.get(key)?.upgrade() + } + /// Serializes bounded chunk-vector batches across all views of the family. pub(crate) fn chunk_batch(&self) -> MutexGuard<'_, ()> { lock(&self.chunk_batch) @@ -217,21 +245,40 @@ impl SemanticArena { Ok(Some(self.install(*key, run))) } - /// Makes a run that was just stored resident without decoding it again. - /// When another view installed the key first, its run is kept and - /// returned, so the key never has two resident copies. A new run's - /// vectors are shared with equal ones already resident. + /// Makes a stored run resident, sharing equal allocations. File payloads + /// alone cannot populate the chunk index: legacy floats may not match the + /// independently immutable chunk rows. pub fn install(&self, key: FamilyKey, mut run: SemanticVectors) -> Arc { if let Some(resident) = self.get(&key) { return resident; } - // Digesting a run takes a moment; do it outside the run table lock. - // A run that then loses the race is dropped, and the handles it - // recorded are pruned with the rest. lock(&self.vectors).share(&mut run); Arc::clone(lock(&self.runs).entry(key).or_insert_with(|| Arc::new(run))) } + /// Records a vector only after its exact chunk row was read or stored. + /// The caller keeps this allocation alive until the file run shares it. + pub(crate) fn remember_chunk(&self, key: FamilyKey, vector: Vec) -> Arc<[f32]> { + // Cap index entries separately: distinct chunk keys can share the same + // vector allocation. Weak references never keep the floats alive. + const MAX_CHUNK_KEYS: usize = 65_536; + let mut vector: Arc<[f32]> = vector.into(); + let mut pool = lock(&self.vectors); + pool.share_vector(&mut vector); + if pool.by_chunk.len() >= MAX_CHUNK_KEYS { + if let Some(old) = pool.by_chunk.keys().next().copied() { + pool.by_chunk.remove(&old); + } + } + pool.by_chunk.insert(key, Arc::downgrade(&vector)); + if pool.by_digest.len() + pool.by_chunk.len() + > pool.live_after_prune.max(1024).saturating_mul(2) + { + pool.prune(); + } + vector + } + /// Claims `key` for embedding, or returns `None` while another fill holds /// it. A claim is only worth taking after `get` and the store missed. pub fn claim(self: &Arc, key: FamilyKey) -> Option { @@ -297,6 +344,10 @@ impl SemanticArena { } } memory.bytes = memory.bytes.saturating_add(memory.vector_bytes); + memory.bytes = memory.bytes.saturating_add( + (lock(&self.vectors).by_chunk.capacity() + * std::mem::size_of::<(FamilyKey, Weak<[f32]>)>()) as u64, + ); memory } diff --git a/crates/aft/src/views/semantic_read_tests.rs b/crates/aft/src/views/semantic_read_tests.rs index 909f1094da..e4370a1340 100644 --- a/crates/aft/src/views/semantic_read_tests.rs +++ b/crates/aft/src/views/semantic_read_tests.rs @@ -1,3 +1,186 @@ +#[test] +fn resident_chunk_shortcut_matches_store_and_full_rebuild() { + let source = large_source(); + let edited = source.replace("input + 149\n", "input * 149\n"); + let mut payloads = Vec::new(); + for resident_shortcut in [true, false] { + let storage = tempfile::tempdir().unwrap(); + let root = tempfile::tempdir().unwrap(); + write_tree(root.path(), &[("src/large.rs", &source)]); + let plane = new_plane(storage.path(), "model-a"); + let checkout = ready_checkout(storage.path(), root.path(), &plane); + let arena = plane.arena("family"); + arena.set_chunk_shortcut(resident_shortcut); + let snapshot = edit_snapshot(&checkout, &edited); + let store = checkout.owner.open_store(FamilyPlane::Semantic).unwrap(); + let probe = crate::blob_store::v2::read_tests::ReadProbe::start(&store); + let before = crate::views::io::work_snapshot(); + let report = checkout.fill(&snapshot, &Model::default(), "model-a"); + let after = crate::views::io::work_snapshot(); + let reads = probe.finish(); + let gets: usize = reads + .statements + .iter() + .filter(|(sql, _)| sql.starts_with("SELECT payload, payload_digest, payload_schema")) + .map(|(_, (calls, _))| calls) + .sum(); + eprintln!("LARGE_EDIT shortcut={resident_shortcut} chunks={} resident_hits={} gets={gets} payload_bytes={} page_misses={}", report.chunks, report.chunk_resident_hits, after["semantic_store_payload_bytes"].as_u64().unwrap() - before["semantic_store_payload_bytes"].as_u64().unwrap(), reads.misses); + assert_canonical_blob(&checkout, &edited); + assert_eq!(gets, if resident_shortcut { 4 } else { 366 }); + assert_eq!(report.source_reads, 1); + assert_eq!(report.embedded_texts, 1); + assert_eq!( + report.chunk_resident_hits > 0, + resident_shortcut, + "{report:?}" + ); + arena.set_chunk_shortcut(true); + let key = plane.producer.key(edited.as_bytes(), &large_rel_path()); + payloads.push( + checkout + .owner + .open_store(FamilyPlane::Semantic) + .unwrap() + .get(&key) + .unwrap() + .unwrap(), + ); + let run = arena.get(&key).unwrap(); + let chunk = crate::semantic_index::chunk_view_file( + root.path(), + Path::new("src/large.rs"), + edited.as_bytes(), + plane.producer.caps, + ) + .unwrap() + .unwrap() + .remove(0); + let chunk_key = plane.producer.chunk_key(&chunk.embed_text); + assert!(arena.chunk_vector(&chunk_key).is_some()); + plane.unbind(&checkout.access); + drop(run); + arena.trim(); + assert!( + arena.chunk_vector(&chunk_key).is_none(), + "the chunk index retained an evicted run" + ); + } + assert_eq!( + payloads[0], payloads[1], + "resident and store shortcuts differ" + ); +} + +#[test] +fn legacy_file_vectors_never_seed_chunk_shortcuts() { + let storage = tempfile::tempdir().unwrap(); + let root = tempfile::tempdir().unwrap(); + let source = large_source(); + write_tree(root.path(), &[("src/large.rs", &source)]); + let plane = new_plane(storage.path(), "model-a"); + let checkout = Checkout::open(storage.path(), "scope-a", root.path(), &plane); + let store = checkout.owner.open_store(FamilyPlane::Semantic).unwrap(); + let chunks = crate::semantic_index::chunk_view_file( + root.path(), + Path::new("src/large.rs"), + source.as_bytes(), + plane.producer.caps, + ) + .unwrap() + .unwrap(); + for chunk in &chunks { + let payload = encode_chunk_vector( + &plane.producer.chunk_identity(), + &chunk.embed_text, + &vector("model-a", &chunk.embed_text), + ) + .unwrap(); + store + .put_or_touch(&plane.producer.chunk_key(&chunk.embed_text), &payload) + .unwrap(); + } + let mut legacy = crate::semantic_index::embed_view_files( + vec![chunks], + &mut |texts| Model::default().embed("legacy-floats", texts), + 64, + ) + .unwrap(); + let key = plane.producer.key(source.as_bytes(), &large_rel_path()); + store + .put_or_touch( + &key, + &legacy + .remove(0) + .encode_view_payload(&plane.producer.payload()), + ) + .unwrap(); + checkout.load(); + let edited = source.replace("input + 149\n", "input * 149\n"); + let snapshot = edit_snapshot(&checkout, &edited); + let report = checkout.fill(&snapshot, &Model::default(), "model-a"); + assert_eq!(report.embedded_texts, 1); + assert_canonical_blob(&checkout, &edited); + let shifted = format!("\n{edited}"); + let snapshot = edit_snapshot(&checkout, &shifted); + let report = checkout.fill(&snapshot, &Model::default(), "model-a"); + assert_eq!(report.embedded_texts, 0); + assert!( + report.chunk_resident_hits >= 300, + "one-time validation did not warm canonical chunks: {report:?}" + ); + assert_canonical_blob(&checkout, &shifted); +} + +#[test] +fn incremental_reuse_requires_matching_path_and_content() { + let storage = tempfile::tempdir().unwrap(); + let root = tempfile::tempdir().unwrap(); + write_tree(root.path(), FILES); + let plane = new_plane(storage.path(), "model-a"); + let checkout = ready_checkout(storage.path(), root.path(), &plane); + let old = checkout.load(); + let resident = plane + .resident(&checkout.access, old.generation().name()) + .unwrap(); + let state = ViewState::default(); + let unchanged = RelPath::new(b"src/beta.rs".to_vec()).unwrap(); + let DiskState::Present { content, .. } = old.disk_state(&unchanged) else { + panic!("missing member") + }; + assert!(matches!( + SemanticPlane::vector_source(&state, &resident, &old, &unchanged, content), + VectorSource::Base + )); + write_tree( + root.path(), + &[("src/alpha.rs", "pub fn alpha_total() -> u32 { 999 }\n")], + ); + checkout.driver.register_write_intent(); + let changed = checkout.load(); + let path = RelPath::new(b"src/alpha.rs".to_vec()).unwrap(); + let DiskState::Present { content, .. } = changed.disk_state(&path) else { + panic!("missing changed member") + }; + assert!( + matches!( + SemanticPlane::vector_source(&state, &resident, &changed, &path, content), + VectorSource::Pending + ), + "changed content reused an old generation's vector" + ); + let report = checkout.fill(&changed, &Model::default(), "model-a"); + assert_eq!( + report.source_reads, 1, + "unchanged paths were filled again: {report:?}" + ); + let next = checkout.load(); + let report = checkout.fill(&next, &Model::default(), "model-a"); + assert_eq!( + (report.queued, report.source_reads, report.chunks), + (0, 0, 0) + ); +} + // Kept out of the normal suite because this probe embeds every Rust source in // the checkout with a deterministic 768-dimensional model, without a service. #[test] @@ -20,10 +203,15 @@ fn repository_semantic_fill_read_probe() { ]) .output() .unwrap(); - assert!(listing.status.success()); + assert!( + listing.status.success(), + "{}", + String::from_utf8_lossy(&listing.stderr) + ); let root = tempfile::tempdir().unwrap(); let storage = tempfile::tempdir().unwrap(); let mut files = 0; + let mut edit_paths = Vec::new(); for path in listing .stdout .split(|byte| *byte == 0) @@ -41,6 +229,9 @@ fn repository_semantic_fill_read_probe() { .output() .unwrap(); assert!(contents.status.success()); + if contents.stdout.len() > 4_000 && contents.stdout.len() < 20_000 { + edit_paths.push(path.to_owned()); + } std::fs::write(destination, contents.stdout).unwrap(); files += 1; } @@ -111,12 +302,52 @@ fn repository_semantic_fill_read_probe() { ); assert!(first_report.errors.is_empty() && first_report.store_errors.is_empty()); assert_eq!(first_report.installed, first_report.queued); + first.load(); + if std::env::var_os("AFT_SEMANTIC_READ_PROBE_DISABLE_SHORTCUT").is_some() { + plane.arena("family").set_chunk_shortcut(false); + } describe(&store); + let sample_path = Path::new(&edit_paths[0]); + let sample_bytes = std::fs::read(root.path().join(sample_path)).unwrap(); + let sample_chunks = crate::semantic_index::chunk_view_file( + root.path(), + sample_path, + &sample_bytes, + plane.producer.caps, + ) + .unwrap() + .unwrap(); + for (index, chunk) in sample_chunks.iter().take(10).enumerate() { + let key = plane.producer.chunk_key(&chunk.embed_text); + let probe = ReadProbe::start(&store); + let payload = store.get(&key).unwrap().unwrap(); + let counts = probe.finish(); + eprintln!( + "COLD_GET sample={index} payload_bytes={} page_cache_misses={}", + payload.len(), + counts.misses + ); + } let second = Checkout::open(storage.path(), "second", sibling.path(), &plane); + let before_sibling = crate::views::io::work_snapshot(); let probe = ReadProbe::start(&store); let second_snapshot = second.load(); let second_report = fill(&second, &second_snapshot); eprintln!("SIBLING {second_report:?}\nSQL {:?}", probe.finish()); + let after_sibling = crate::views::io::work_snapshot(); + eprintln!( + "SIBLING_COUNTERS gets={} payload_bytes={} fills={}", + after_sibling["semantic_store_gets"].as_u64().unwrap() + - before_sibling["semantic_store_gets"].as_u64().unwrap(), + after_sibling["semantic_store_payload_bytes"] + .as_u64() + .unwrap() + - before_sibling["semantic_store_payload_bytes"] + .as_u64() + .unwrap(), + after_sibling["semantic_fills"].as_u64().unwrap() + - before_sibling["semantic_fills"].as_u64().unwrap() + ); assert_eq!(second_report.embedded_texts, 0); assert_eq!(second_report.stored_hits, 0); assert_eq!(second_report.queued, 0); @@ -125,6 +356,44 @@ fn repository_semantic_fill_read_probe() { plane.arena("family").decode_count(), plane.arena("family").store_reads() ); + for (label, edits) in [("one-file", 1), ("ten-file", 10), ("no-change", 0)] { + for index in 0..edits { + let path = sibling.path().join(&edit_paths[index]); + let mut text = std::fs::read_to_string(&path).unwrap(); + text.push_str(&format!( + "\npub fn probe_{index}() -> u32 {{ {} }}\n", + edits + index + )); + std::fs::write(path, text).unwrap(); + } + let before = crate::views::io::work_snapshot(); + second.driver.register_write_intent(); + let snapshot = second.load(); + let report = fill(&second, &snapshot); + let after = crate::views::io::work_snapshot(); + let delta = [ + "semantic_store_gets", + "semantic_store_payload_bytes", + "semantic_touch_rows_written", + "semantic_touch_transactions", + "semantic_files_filled", + ] + .map(|key| { + ( + key, + after[key].as_u64().unwrap() - before[key].as_u64().unwrap(), + ) + }); + eprintln!( + "REPUBLISH {label} queued={} filled={} chunks={} delta={delta:?}", + report.queued, report.source_reads, report.chunks + ); + assert!(report.errors.is_empty() && report.store_errors.is_empty()); + assert_eq!(report.source_reads, edits); + if edits == 0 { + assert_eq!(delta.map(|(_, value)| value), [0; 5]); + } + } plane.unbind(&first.access); plane.unbind(&second.access); drop(first); diff --git a/crates/aft/src/views/semantic_runtime.rs b/crates/aft/src/views/semantic_runtime.rs index ce901a15ad..cea7546af5 100644 --- a/crates/aft/src/views/semantic_runtime.rs +++ b/crates/aft/src/views/semantic_runtime.rs @@ -1528,7 +1528,7 @@ where || !report.store_errors.is_empty() { crate::slog_info!( - "semantic view fill root={} queued={} deferred={} embedded_keys={} chunks={} chunk_reused={} texts={} calls={} stored_hits={} resident_hits={} installed={} failed={} errors={} store_errors={} elapsed_ms={} embed_ms={} embed_max_ms={} admission_wait_ms={} cold_admitted={} mutex_wait_ms={} reconcile_ms={} queue_setup_ms={} read_hash_ms={} file_lookup_ms={} chunk_ms={} chunk_wait_ms={} vector_lookup_ms={} store_write_ms={} shared_wait_ms={} fold_ms={} source_reads={} source_bytes={}", + "semantic view fill root={} queued={} deferred={} embedded_keys={} chunks={} chunk_reused={} texts={} calls={} stored_hits={} resident_hits={} installed={} failed={} errors={} store_errors={} elapsed_ms={} embed_ms={} embed_max_ms={} admission_wait_ms={} cold_admitted={} mutex_wait_ms={} reconcile_ms={} queue_setup_ms={} read_hash_ms={} file_lookup_ms={} chunk_ms={} chunk_wait_ms={} vector_lookup_ms={} store_write_ms={} shared_wait_ms={} fold_ms={} source_reads={} source_bytes={} files_filled={} chunk_resident_hits={}", schedule.root.display(), report.queued, report.deferred, @@ -1560,7 +1560,9 @@ where report.timing.shared_wait.as_millis(), report.timing.fold.as_millis(), report.source_reads, - report.source_bytes + report.source_bytes, + report.source_reads, + report.chunk_resident_hits ); } let advanced = report.installed > 0 || report.failed > 0; diff --git a/crates/aft/src/views/semantic_runtime_tests.rs b/crates/aft/src/views/semantic_runtime_tests.rs index a16101819e..d957aebafc 100644 --- a/crates/aft/src/views/semantic_runtime_tests.rs +++ b/crates/aft/src/views/semantic_runtime_tests.rs @@ -1360,7 +1360,8 @@ fn large_semantic_materialization_batches_pins_and_touches() { } assert_eq!((after.0 - writes, after.1 - syncs), (2, 0)); assert_eq!(written_bytes, 2048 * 65 * 2); - assert_eq!(transactions, 1); + // Every key was just stored in this epoch, so folding needs no writer. + assert_eq!(transactions, 0); } /// A large, already-filled checkout must not read or embed unchanged sources From 7cf07bde2628d231c2c96cab89075da937e0fa7f Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:50:07 +0200 Subject: [PATCH 15/17] test: give the untrusted shell refusal test a 15 s bound On a loaded Windows runner, actor startup alone exceeded the 1 s bound and the test failed in #624's merge-queue run. In the opening case the database never finishes opening, so a refusal that waited for it would still hang past 15 s. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/subc/tool_provider.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/aft/src/subc/tool_provider.rs b/crates/aft/src/subc/tool_provider.rs index 3e43029efc..9dd1d0c017 100644 --- a/crates/aft/src/subc/tool_provider.rs +++ b/crates/aft/src/subc/tool_provider.rs @@ -4301,8 +4301,12 @@ mod route_tests { if keyed { body["call_key"] = json!("no-elicitation"); } + // With `opening`, the database never finishes opening, so a + // refusal that waited for it would hang past any bound. The + // generous bound only absorbs actor startup on a loaded + // Windows runner, where one second was not enough. let reply = tokio::time::timeout( - Duration::from_secs(1), + Duration::from_secs(15), exchange_with_actor( body, identity, From d659f0932ba483b618c397242374e7921a5766b6 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:26:30 +0200 Subject: [PATCH 16/17] fix: release the semantic arena's lookup tables after pruning; Unix-gate a test-only probe memory() reports the chunk table's allocated capacity, and HashMap::retain keeps that allocation, so an arena whose views were all released still reported 784 bytes. The deleted-root probe seam is only called from Unix tests, and Windows CI denies dead code. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/context.rs | 3 ++- crates/aft/src/views/semantic_arena.rs | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/aft/src/context.rs b/crates/aft/src/context.rs index 8fd065a386..0d341c25d9 100644 --- a/crates/aft/src/context.rs +++ b/crates/aft/src/context.rs @@ -5662,7 +5662,8 @@ impl AppContext { }) } - #[cfg(test)] + // Only the Unix-only idle language-server tests inject a probe. + #[cfg(all(test, unix))] pub(crate) fn try_cleanup_deleted_root_with_probe( &self, expected_generation: u64, diff --git a/crates/aft/src/views/semantic_arena.rs b/crates/aft/src/views/semantic_arena.rs index f2ca52ceea..939065eb88 100644 --- a/crates/aft/src/views/semantic_arena.rs +++ b/crates/aft/src/views/semantic_arena.rs @@ -111,6 +111,10 @@ impl VectorPool { fn prune(&mut self) { self.by_digest.retain(|_, vector| vector.strong_count() > 0); self.by_chunk.retain(|_, vector| vector.strong_count() > 0); + // `retain` keeps the table's allocation, which `memory()` reports; + // return it once the views that filled it are gone. + self.by_digest.shrink_to_fit(); + self.by_chunk.shrink_to_fit(); self.live_after_prune = self.by_digest.len() + self.by_chunk.len(); } } From 1cf4900a9e91888150790ef3f1319255822daa23 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:49:40 +0200 Subject: [PATCH 17/17] mason: correct Batch K cross-platform regression fixtures Replace the obsolete Windows task-folder pinning assertion with the DirectorySnapshot identity contract: swaps must be refused even though renames are allowed. Recreate the full replacement layout so a missing directory cannot mask a bypassed identity check. Let the fake LSP leave its project cwd before initialization, preserving native Windows deleted-root teardown coverage. Use platform-native relative paths for semantic chunk seeding and lookups; the resident shortcut already works on Windows (362 hits and 4 gets versus 366 gets without it). Assert stale backend census mappings before response budgeting and independently prove that a bounded response may omit both details. The former test incorrectly treated root_count as a guarantee that detail rows survived process-wide payload pressure. Add deterministic budget pressure instead of timing-dependent load. Verified Linux and native Windows scoped suites, 30 Linux health repetitions, Windows cross-compile with warnings denied, and five isolated native Windows mutation reds followed by clean restoration. (#634) Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/access_policy_task_tests.rs | 17 +++----- crates/aft/src/subc/health.rs | 44 ++++++++++++++++----- crates/aft/src/subc/mod.rs | 6 +++ crates/aft/src/views/semantic_read_tests.rs | 7 +++- crates/aft/tests/lsp/fake_server.rs | 5 +++ 5 files changed, 55 insertions(+), 24 deletions(-) diff --git a/crates/aft/src/access_policy_task_tests.rs b/crates/aft/src/access_policy_task_tests.rs index 3aa6ce05ad..814a2cf0e0 100644 --- a/crates/aft/src/access_policy_task_tests.rs +++ b/crates/aft/src/access_policy_task_tests.rs @@ -547,24 +547,17 @@ fn access_policy_foreign_worker_status_and_controls_are_denied() { } #[test] -fn access_policy_task_folder_cache_is_pinned_and_shared_between_files() { +fn access_policy_task_folder_cache_is_identity_checked_and_shared_between_files() { let f = fixture(); crate::access_policy::with_scope(Some(f.head.clone()), || { assert!(read_as(&f.ctx, "head-session", &f.worker.command).success); std::fs::write(&f.worker.json, "invalid metadata").unwrap(); assert!(read_as(&f.ctx, "head-session", &f.worker.stdout).success); - // Windows refuses to rename a folder while the pinned record holds a - // handle to it, which already rules out the swap this test guards. - let renamed = std::fs::rename(&f.worker.dir, f.worker.dir.with_extension("old")); - if cfg!(windows) { - assert!( - renamed.is_err(), - "a pinned task folder must not be renamable" - ); - return; - } - renamed.unwrap(); + // Cached permissions retain identities, not handles. Renaming is allowed, + // but a new directory at the same path must not inherit those permissions. + std::fs::rename(&f.worker.dir, f.worker.dir.with_extension("old")).unwrap(); std::fs::create_dir_all(&f.worker.control_dir).unwrap(); + std::fs::create_dir_all(&f.worker.io_dir).unwrap(); std::fs::write(&f.worker.command, "replacement").unwrap(); assert_floor( read_as(&f.ctx, "head-session", &f.worker.command), diff --git a/crates/aft/src/subc/health.rs b/crates/aft/src/subc/health.rs index 9d7ada8a2c..c77b6e9e0b 100644 --- a/crates/aft/src/subc/health.rs +++ b/crates/aft/src/subc/health.rs @@ -4386,25 +4386,37 @@ mod tests { let (_absent_dir, absent_root) = test_root("health-stale-absent"); let absent_ctx = test_ctx(); install_resident_stale_callgraph(&absent_ctx, &absent_root, "absent.ts", true); - assert!(executor.register_actor(absent_root.clone(), absent_ctx)); + assert!(executor.register_actor(absent_root.clone(), absent_ctx.clone())); let (_existing_dir, existing_root) = test_root("health-stale-existing"); let existing_ctx = test_ctx(); install_resident_stale_callgraph(&existing_ctx, &existing_root, "existing.ts", false); - assert!(executor.register_actor(existing_root.clone(), existing_ctx)); + assert!(executor.register_actor(existing_root.clone(), existing_ctx.clone())); + + // Make response-budget pressure deterministic instead of depending on + // process-wide telemetry contributed by concurrently running tests. + for ctx in [&absent_ctx, &existing_ctx] { + *ctx.semantic_index_status().write().unwrap() = + crate::context::SemanticIndexStatus::Building { + stage: "x".repeat(HEALTH_METRICS_BUDGET_BYTES), + files: None, + entries_done: None, + entries_total: None, + }; + } - let report = test_health_report( - &executor, - &HashMap::new(), - &DispatchPathMetrics::new(), - &App::default_shared(), - ); - let metrics = report.metrics.expect("health metrics"); + // Assert the rollup's per-root census before building the byte-budgeted + // report; the oversized stages above intentionally make it omit details. + let app = App::default_shared(); + let cache = HealthRollupCache::new(); + cache.refresh_for_test(&executor, &app); + let (rollup, _) = cache.snapshot(); + let metrics = &rollup.metrics; let roots = metrics["roots"].as_array().expect("per-root health rows"); let row_for = |root: &ProjectRootId| { roots .iter() .find(|row| row["project_root"] == root.as_path().display().to_string()) - .expect("root health row") + .unwrap_or_else(|| panic!("root health row: {metrics}")) }; let absent = row_for(&absent_root); assert_eq!(absent["resident_callgraph_stale_backend_rows"], 1); @@ -4420,6 +4432,18 @@ mod tests { existing["resident_callgraph_unreadable_stale_backend_rows"], 0 ); + + let report = build_health_report( + &cache, + &executor, + &HashMap::new(), + &DispatchPathMetrics::new(), + &app, + ); + let bounded = report.metrics.expect("bounded health metrics"); + assert_eq!(bounded["root_count"], 2); + assert_eq!(bounded["root_details_omitted"], 2); + assert!(bounded["roots"].as_array().unwrap().is_empty()); } #[cfg(unix)] diff --git a/crates/aft/src/subc/mod.rs b/crates/aft/src/subc/mod.rs index c331387d63..a2f0bdad67 100644 --- a/crates/aft/src/subc/mod.rs +++ b/crates/aft/src/subc/mod.rs @@ -15349,6 +15349,12 @@ mod tests { ); ctx.lsp() .set_extra_env("AFT_FAKE_LSP_IGNORE_SHUTDOWN".into(), "1".into()); + // Windows cannot delete the project while it is a child's cwd. The fake + // moves outside it before initialization, without changing its LSP root. + ctx.lsp().set_extra_env( + "AFT_FAKE_LSP_CWD", + &root.as_path().parent().unwrap().to_string_lossy(), + ); assert_eq!(ctx.lsp().ensure_server_for_file(&file, &config).len(), 1); assert_eq!(ctx.lsp().active_client_count(), 1); let pids = ctx.lsp_child_registry().pids(); diff --git a/crates/aft/src/views/semantic_read_tests.rs b/crates/aft/src/views/semantic_read_tests.rs index e4370a1340..db380a0c3e 100644 --- a/crates/aft/src/views/semantic_read_tests.rs +++ b/crates/aft/src/views/semantic_read_tests.rs @@ -46,9 +46,11 @@ fn resident_chunk_shortcut_matches_store_and_full_rebuild() { .unwrap(), ); let run = arena.get(&key).unwrap(); + // Chunk text includes the platform-native relative path, so use the same + // path representation as a normal index build. let chunk = crate::semantic_index::chunk_view_file( root.path(), - Path::new("src/large.rs"), + &Path::new("src").join("large.rs"), edited.as_bytes(), plane.producer.caps, ) @@ -80,9 +82,10 @@ fn legacy_file_vectors_never_seed_chunk_shortcuts() { let plane = new_plane(storage.path(), "model-a"); let checkout = Checkout::open(storage.path(), "scope-a", root.path(), &plane); let store = checkout.owner.open_store(FamilyPlane::Semantic).unwrap(); + // Seed the same native-path chunk identities that a full build would persist. let chunks = crate::semantic_index::chunk_view_file( root.path(), - Path::new("src/large.rs"), + &Path::new("src").join("large.rs"), source.as_bytes(), plane.producer.caps, ) diff --git a/crates/aft/tests/lsp/fake_server.rs b/crates/aft/tests/lsp/fake_server.rs index 6d6b5cb381..6538d69aa9 100644 --- a/crates/aft/tests/lsp/fake_server.rs +++ b/crates/aft/tests/lsp/fake_server.rs @@ -482,6 +482,11 @@ pub(crate) fn main() -> io::Result<()> { println!("aft fake rust-analyzer 1"); return Ok(()); } + // Change cwd before handling initialize, so its response confirms that + // deletion tests can remove the workspace even on Windows. + if let Some(cwd) = std::env::var_os("AFT_FAKE_LSP_CWD") { + std::env::set_current_dir(cwd)?; + } // AFT_FAKE_LSP_IGNORE_SIGTERM=1: keep running through SIGTERM, like a // server with its own handler that is busy or wedged, so only a kill that // cannot be ignored stops it.