From 6f15e0bc7722af7edc1febe165fcd674f09a60ca Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 19:17:45 +0900 Subject: [PATCH 1/7] perf(stats): read local-path file sizes lazily per result (#90) Replace the eager per-index size map with FileSizes: a local source root is read on demand for the files a query actually returns, memoized behind a Mutex so the Arc shared by MCP calls stays Sync. Git sources still capture sizes at clone time because the temp checkout is gone by search time. Cached loads no longer re-read every indexed file. Deliberate divergence from upstream semble (eager in SembleIndex.__init__); noted in the semble reference doc. --- .please/docs/references/semble.md | 4 + crates/csp/src/bin/csp/main.rs | 4 +- crates/csp/src/indexing/file_sizes.rs | 167 +++++++++++++++++++++++++ crates/csp/src/indexing/index.rs | 84 ++++--------- crates/csp/src/indexing/index/tests.rs | 26 ++++ crates/csp/src/indexing/mod.rs | 1 + crates/csp/src/stats.rs | 14 +-- 7 files changed, 232 insertions(+), 68 deletions(-) create mode 100644 crates/csp/src/indexing/file_sizes.rs diff --git a/.please/docs/references/semble.md b/.please/docs/references/semble.md index 632238a..7747296 100644 --- a/.please/docs/references/semble.md +++ b/.please/docs/references/semble.md @@ -288,6 +288,10 @@ Ported faithfully (`LazyLock` for the static patterns, `RefCell` (Total saved, efficiency bar, By Period; By Call Type gated behind `--verbose`). `clear_savings`. - **Divergence**: fixed `~/.csp/savings.jsonl` (not the OS cache dir); no `flock` (sub-4KB appends are atomic on POSIX); header is "Csp". +- **Divergence** (issue #90): `file_chars` sizes come from `indexing::file_sizes::FileSizes` — + a local source root is read lazily per returned result with a memo, where upstream + `_compute_file_sizes` runs eagerly over every indexed file in `SembleIndex.__init__`. Git + sources still capture eagerly at clone time (the temp checkout is gone by search time). ### 4.16 MCP — `csp/src/mcp.rs` (core) + `csp/src/bin/csp/mcp_server.rs` (rmcp transport) diff --git a/crates/csp/src/bin/csp/main.rs b/crates/csp/src/bin/csp/main.rs index 1934708..c0db57d 100644 --- a/crates/csp/src/bin/csp/main.rs +++ b/crates/csp/src/bin/csp/main.rs @@ -616,8 +616,8 @@ mod tests { fn search_output_records_savings_when_stats_file_given() { let dir = build_index_dir(); let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); - // file_sizes is captured at build time from the source tree. - assert!(!idx.file_sizes.is_empty()); + // The source tree is still on disk, so sizes are read lazily per result. + assert!(idx.file_sizes.is_available()); let stats = tempdir().unwrap(); let stats_file = stats.path().join("savings.jsonl"); diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs new file mode 100644 index 0000000..d81793a --- /dev/null +++ b/crates/csp/src/indexing/file_sizes.rs @@ -0,0 +1,167 @@ +//! Per-file character counts feeding the `file_chars` side of token-savings +//! telemetry (`crate::stats`). +//! +//! Deliberate divergence from upstream semble, which recomputes every indexed +//! file's size eagerly in `SembleIndex.__init__`: a local source root is read +//! lazily, per result, with a memo — only the handful of files a query actually +//! returns is touched. Git sources still capture eagerly, at clone time. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::Mutex; + +/// UTF-16 character counts per repo-relative file path, resolved eagerly +/// (captured) or lazily (read from a local root on demand). +#[derive(Debug, Default)] +pub struct FileSizes { + /// Sizes captured while the source tree was on disk (git clones: the temp + /// checkout is gone by search time). + captured: HashMap, + /// Local source root read on demand for paths not in `captured`. + lazy_root: Option, + /// Memo of lazily read sizes; `Mutex` because `CspIndex` is shared as + /// `Arc` across MCP calls. + memo: Mutex>, +} + +impl FileSizes { + /// No sizes available — telemetry records `file_chars` as 0. + pub fn empty() -> Self { + Self::default() + } + + /// Sizes already read off a source tree that is no longer available. + pub fn captured(sizes: HashMap) -> Self { + Self { + captured: sizes, + ..Self::default() + } + } + + /// Sizes read on demand from a still-present local source root. + pub fn lazy(root: PathBuf) -> Self { + Self { + lazy_root: Some(root), + ..Self::default() + } + } + + /// Character count for `file_path`: captured → memo → read from the lazy + /// root (memoized on success). `None` when unavailable or unreadable. + pub fn get(&self, file_path: &str) -> Option { + if let Some(size) = self.captured.get(file_path) { + return Some(*size); + } + let root = self.lazy_root.as_deref()?; + if let Some(size) = self.lock_memo().get(file_path) { + return Some(*size); + } + let size = read_file_chars(root, file_path)?; + self.lock_memo().insert(file_path.to_string(), size); + Some(size) + } + + /// `true` when sizes can be produced at all. Prefer this over inspecting + /// the map: a lazy root reports available before anything has been read. + pub fn is_available(&self) -> bool { + !self.captured.is_empty() || self.lazy_root.is_some() + } + + fn lock_memo(&self) -> std::sync::MutexGuard<'_, HashMap> { + self.memo.lock().unwrap_or_else(|e| e.into_inner()) + } +} + +/// UTF-16 character count of the repo-relative `file_path` under `root`, or +/// `None` when it cannot be read. UTF-16 keeps it consistent with +/// `stats::save_search_stats`'s snippet accounting. +/// +/// Chunk paths are repo-relative by construction; a path that is absolute or +/// escapes `root` via `..` can only come from a tampered on-disk index, so it is +/// skipped rather than resolved (path traversal guard — a deliberate addition +/// over upstream, which joins the path unchecked). Only regular files are read: +/// the file walker never follows symlinks, and a path that has since become a +/// symlink, FIFO, or device must not be able to redirect or stall the read. +pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { + let rel = Path::new(file_path); + if !is_safe_relative_path(rel) { + return None; + } + let full = root.join(rel); + let is_regular_file = std::fs::symlink_metadata(&full) + .map(|m| m.is_file()) + .unwrap_or(false); + if !is_regular_file { + return None; + } + let text = std::fs::read_to_string(&full).ok()?; + Some(text.encode_utf16().count() as u64) +} + +/// `true` when `path` is relative and contains no `..` or root component, so +/// joining it onto an index root cannot resolve outside that root. +fn is_safe_relative_path(path: &Path) -> bool { + use std::path::Component; + !path.is_absolute() + && !path.components().any(|c| { + matches!( + c, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + fn lazy_reads_and_memoizes_regular_files() { + let root = tempdir().unwrap(); + std::fs::write(root.path().join("a.ts"), "abcd").unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert!(sizes.is_available()); + assert_eq!(sizes.get("a.ts"), Some(4)); + // Memoized: the value survives the file going away. + std::fs::remove_file(root.path().join("a.ts")).unwrap(); + assert_eq!(sizes.get("a.ts"), Some(4)); + } + + #[test] + fn lazy_returns_none_for_unreadable_paths() { + let outer = tempdir().unwrap(); + let root = outer.path().join("repo"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(outer.path().join("secret.txt"), "top secret").unwrap(); + std::fs::write(root.join("real.ts"), "abcd").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(outer.path().join("secret.txt"), root.join("link.ts")).unwrap(); + let abs = root.join("real.ts").to_string_lossy().into_owned(); + let sizes = FileSizes::lazy(root.clone()); + + assert_eq!(sizes.get("../secret.txt"), None); + assert_eq!(sizes.get(&abs), None); + assert_eq!(sizes.get("missing.ts"), None); + #[cfg(unix)] + assert_eq!(sizes.get("link.ts"), None); + } + + #[test] + fn captured_serves_known_paths_only() { + let sizes = FileSizes::captured([("a.ts".to_string(), 7u64)].into_iter().collect()); + + assert!(sizes.is_available()); + assert_eq!(sizes.get("a.ts"), Some(7)); + assert_eq!(sizes.get("b.ts"), None); + } + + #[test] + fn empty_is_not_available() { + let sizes = FileSizes::empty(); + + assert!(!sizes.is_available()); + assert_eq!(sizes.get("a.ts"), None); + } +} diff --git a/crates/csp/src/indexing/index.rs b/crates/csp/src/indexing/index.rs index c3fa7e1..faaa8d6 100644 --- a/crates/csp/src/indexing/index.rs +++ b/crates/csp/src/indexing/index.rs @@ -12,6 +12,7 @@ use sha2::{Digest, Sha256}; use crate::chunking::source::DESIRED_CHUNK_LENGTH_CHARS; use crate::indexing::create::{create_index_from_path, CreateIndexOptions}; use crate::indexing::dense::{load_model, make_stub_model, Model, SelectableBasicBackend}; +use crate::indexing::file_sizes::{read_file_chars, FileSizes}; use crate::indexing::sparse::Bm25Index; use crate::search::{search as run_search, SearchOptions as RunSearchOptions, SearchResult}; use crate::types::{chunk_from_dict, chunk_to_dict, Chunk, ChunkDict, ContentType, IndexStats}; @@ -80,11 +81,12 @@ pub struct CspIndex { pub model_path: String, pub root: Option, pub content: Vec, - /// Per-file character counts (repo-relative path → UTF-16 length) captured - /// at build time from the source tree, for token-savings telemetry. Empty - /// when the source files aren't available (e.g. a git index loaded from - /// cache). Derived metadata, not part of [`CspIndexState`]. - pub file_sizes: HashMap, + /// Per-file character counts (repo-relative path → UTF-16 length) for + /// token-savings telemetry: read lazily from a still-present local source + /// root, or captured at build time when the source won't outlive the build + /// (a git clone's temp checkout). Derived metadata, not part of + /// [`CspIndexState`]. + pub file_sizes: FileSizes, } pub(crate) fn normalize_content(content: Option>) -> Vec { @@ -101,7 +103,7 @@ impl CspIndex { model_path: state.model_path, root: state.root, content: state.content, - file_sizes: HashMap::new(), + file_sizes: FileSizes::empty(), } } @@ -126,24 +128,20 @@ impl CspIndex { }, )?; + // Absolute, like upstream's `path.resolve()`, so an index built from + // `.` still finds its source tree when loaded from another cwd. + let root = std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()); let mut index = Self::new(CspIndexState { model, bm25_index: result.bm25_index, semantic_index: result.semantic_index, chunks: result.chunks, model_path, - // Absolute, like upstream's `path.resolve()`, so an index built from - // `.` still finds its source tree when loaded from another cwd. - root: Some( - std::path::absolute(path) - .unwrap_or_else(|_| path.to_path_buf()) - .to_string_lossy() - .into_owned(), - ), + root: Some(root.to_string_lossy().into_owned()), content, }); - // Capture file sizes now, while the source tree is on disk. - index.file_sizes = compute_file_sizes(path, &index.chunks); + // The source tree stays on disk, so sizes are read lazily per result. + index.file_sizes = FileSizes::lazy(root); Ok(index) } @@ -165,9 +163,9 @@ impl CspIndex { clone_shallow(url, dir.path(), git_ref)?; let index = Self::from_path(dir.path(), options)?; - // `from_path` already captured file sizes from the checkout; carry them - // over since the temp dir is removed when `dir` drops. - let file_sizes = index.file_sizes.clone(); + // Capture file sizes from the checkout now: the temp dir is removed when + // `dir` drops, so they can't be read lazily at search time. + let file_sizes = FileSizes::captured(compute_file_sizes(dir.path(), &index.chunks)); // Re-root at the URL so a persisted manifest records a stable sourceId // (the temp checkout is removed when `dir` drops). let mut rerooted = Self::new(CspIndexState { @@ -378,13 +376,14 @@ impl CspIndex { root: manifest.source_id, content: manifest.content, }); - // Recompute file sizes from the source when it's a still-present local - // directory (mirrors semble reading sizes off `root` on load). A git URL - // or a moved source leaves this empty → `file_chars` is simply 0. + // Read file sizes lazily from the source when it's a still-present local + // directory — a deliberate divergence from upstream semble, which + // recomputes them eagerly in `SembleIndex.__init__`. A git URL or a moved + // source leaves this unavailable → `file_chars` is simply 0. if let Some(root) = index.root.as_deref() { let root_path = Path::new(root); if root_path.is_dir() { - index.file_sizes = compute_file_sizes(root_path, &index.chunks); + index.file_sizes = FileSizes::lazy(root_path.to_path_buf()); } } Ok(index) @@ -393,52 +392,21 @@ impl CspIndex { /// Per-file UTF-16 character counts for the unique files referenced by `chunks`, /// read from `root`. Mirrors semble `_compute_file_sizes` (unreadable files are -/// skipped). Feeds the `file_chars` side of token-savings telemetry; UTF-16 keeps -/// it consistent with `stats::save_search_stats`'s snippet accounting. -/// -/// Chunk paths are repo-relative by construction; a path that is absolute or -/// escapes `root` via `..` can only come from a tampered on-disk index, so it is -/// skipped rather than resolved (path traversal guard — a deliberate addition -/// over upstream, which joins the path unchecked). Only regular files are read: -/// the file walker never follows symlinks, and a path that has since become a -/// symlink, FIFO, or device must not be able to redirect or stall the read. +/// skipped). Used for sources that won't outlive the build (a git clone's temp +/// checkout); local paths read lazily via [`FileSizes::lazy`] instead. fn compute_file_sizes(root: &Path, chunks: &[Chunk]) -> HashMap { let mut sizes: HashMap = HashMap::new(); for chunk in chunks { if sizes.contains_key(&chunk.file_path) { continue; } - let rel = Path::new(&chunk.file_path); - if !is_safe_relative_path(rel) { - continue; - } - let full = root.join(rel); - let is_regular_file = std::fs::symlink_metadata(&full) - .map(|m| m.is_file()) - .unwrap_or(false); - if !is_regular_file { - continue; - } - if let Ok(text) = std::fs::read_to_string(&full) { - sizes.insert(chunk.file_path.clone(), text.encode_utf16().count() as u64); + if let Some(chars) = read_file_chars(root, &chunk.file_path) { + sizes.insert(chunk.file_path.clone(), chars); } } sizes } -/// `true` when `path` is relative and contains no `..` or root component, so -/// joining it onto an index root cannot resolve outside that root. -fn is_safe_relative_path(path: &Path) -> bool { - use std::path::Component; - !path.is_absolute() - && !path.components().any(|c| { - matches!( - c, - Component::ParentDir | Component::RootDir | Component::Prefix(_) - ) - }) -} - /// Shallow-clone `url` into `dir`, non-interactively. Rejects a ref starting /// with `-` (git-flag injection, CWE-88). fn clone_shallow(url: &str, dir: &Path, git_ref: Option<&str>) -> Result<(), String> { diff --git a/crates/csp/src/indexing/index/tests.rs b/crates/csp/src/indexing/index/tests.rs index 96f9959..0391bc7 100644 --- a/crates/csp/src/indexing/index/tests.rs +++ b/crates/csp/src/indexing/index/tests.rs @@ -355,3 +355,29 @@ fn compute_file_sizes_skips_symlinks_and_non_regular_files() { assert!(!sizes.contains_key("link.ts")); assert!(!sizes.contains_key("dir.ts")); } + +#[test] +fn from_path_reads_file_sizes_lazily() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("sample.ts"), "export const x = 1\n").unwrap(); + + let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); + + assert!(idx.file_sizes.is_available()); + assert_eq!(idx.file_sizes.get("sample.ts"), Some(19)); +} + +#[test] +fn load_from_disk_has_no_file_sizes_when_source_is_gone() { + let source = tempdir().unwrap(); + std::fs::write(source.path().join("sample.ts"), "export const x = 1\n").unwrap(); + let idx = CspIndex::from_path(source.path(), &LoadOptions::default()).unwrap(); + let cache = tempdir().unwrap(); + idx.save(cache.path(), None).unwrap(); + std::fs::remove_dir_all(source.path()).unwrap(); + + let loaded = CspIndex::load_from_disk(cache.path()).unwrap(); + + assert!(!loaded.file_sizes.is_available()); + assert_eq!(loaded.file_sizes.get("sample.ts"), None); +} diff --git a/crates/csp/src/indexing/mod.rs b/crates/csp/src/indexing/mod.rs index 771e6a4..1fd1664 100644 --- a/crates/csp/src/indexing/mod.rs +++ b/crates/csp/src/indexing/mod.rs @@ -8,6 +8,7 @@ pub mod cache; mod cache_orchestrator; pub mod create; pub mod dense; +pub mod file_sizes; pub mod file_walker; pub mod files; pub mod index; diff --git a/crates/csp/src/stats.rs b/crates/csp/src/stats.rs index 89d56ec..04dd87d 100644 --- a/crates/csp/src/stats.rs +++ b/crates/csp/src/stats.rs @@ -7,13 +7,14 @@ //! Time bucketing uses UTC `YYYY-MM-DD` (compared lexicographically, which is //! chronological); `now_secs` is injected so summaries/reports are testable. -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; use std::io::{IsTerminal, Write as _}; use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; +use crate::indexing::file_sizes::FileSizes; use crate::search::SearchResult; use crate::types::CallType; @@ -110,7 +111,7 @@ pub fn save_search_stats( stats_file: &Path, results: &[SearchResult], call_type: CallType, - file_sizes: &HashMap, + file_sizes: &FileSizes, max_snippet_lines: Option, ) { let snippet_chars: u64 = results @@ -123,10 +124,7 @@ pub fn save_search_stats( unique_paths.push(r.chunk.file_path.as_str()); } } - let file_chars: u64 = unique_paths - .iter() - .filter_map(|p| file_sizes.get(*p).copied()) - .sum(); + let file_chars: u64 = unique_paths.iter().filter_map(|p| file_sizes.get(p)).sum(); let record = StatsRecord { ts: now_secs(), @@ -469,8 +467,8 @@ mod tests { } } - fn sizes(pairs: &[(&str, u64)]) -> HashMap { - pairs.iter().map(|(p, s)| ((*p).to_string(), *s)).collect() + fn sizes(pairs: &[(&str, u64)]) -> FileSizes { + FileSizes::captured(pairs.iter().map(|(p, s)| ((*p).to_string(), *s)).collect()) } #[test] From 428daef6a25d4c7bd2f135d10fed22141ba0deae Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 19:51:18 +0900 Subject: [PATCH 2/7] chore(stats): apply code review fixes to lazy file sizes - bound lazy reads by MAX_FILE_BYTES and decode lossily like the indexer - memoize misses so an unreadable path is attempted once per index - dedupe result paths with a HashSet in save_search_stats - tighten telemetry tests to assert real file_chars values --- .please/docs/references/semble.md | 10 +++- crates/csp/src/bin/csp/main.rs | 13 ++++- crates/csp/src/indexing/file_sizes.rs | 76 +++++++++++++++++++++----- crates/csp/src/indexing/index/tests.rs | 6 +- crates/csp/src/stats.rs | 42 +++++++++++--- 5 files changed, 118 insertions(+), 29 deletions(-) diff --git a/.please/docs/references/semble.md b/.please/docs/references/semble.md index 7747296..11376dd 100644 --- a/.please/docs/references/semble.md +++ b/.please/docs/references/semble.md @@ -289,9 +289,13 @@ Ported faithfully (`LazyLock` for the static patterns, `RefCell` - **Divergence**: fixed `~/.csp/savings.jsonl` (not the OS cache dir); no `flock` (sub-4KB appends are atomic on POSIX); header is "Csp". - **Divergence** (issue #90): `file_chars` sizes come from `indexing::file_sizes::FileSizes` — - a local source root is read lazily per returned result with a memo, where upstream - `_compute_file_sizes` runs eagerly over every indexed file in `SembleIndex.__init__`. Git - sources still capture eagerly at clone time (the temp checkout is gone by search time). + a local source root is read lazily per returned result with a memo (misses memoized too), where + upstream `_compute_file_sizes` runs eagerly over every indexed file in `SembleIndex.__init__`. + Git sources still capture eagerly at clone time (the temp checkout is gone by search time). + Because the read now happens inside a live search, it is bounded by `MAX_FILE_BYTES` (the same + ceiling the indexer applies) — upstream, running at construction time, has no such bound. + Decoding matches upstream `read_file_text` (`errors="replace"`) and the csp indexer + (`String::from_utf8_lossy`), so a non-UTF-8 file that got indexed still gets sized. ### 4.16 MCP — `csp/src/mcp.rs` (core) + `csp/src/bin/csp/mcp_server.rs` (rmcp transport) diff --git a/crates/csp/src/bin/csp/main.rs b/crates/csp/src/bin/csp/main.rs index c0db57d..9e9eed7 100644 --- a/crates/csp/src/bin/csp/main.rs +++ b/crates/csp/src/bin/csp/main.rs @@ -617,7 +617,10 @@ mod tests { let dir = build_index_dir(); let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); // The source tree is still on disk, so sizes are read lazily per result. - assert!(idx.file_sizes.is_available()); + // Look the size up under the path the chunks actually carry — that is + // the key `save_search_stats` will use. + let indexed_path = idx.chunks[0].file_path.clone(); + assert!(idx.file_sizes.get(&indexed_path).is_some()); let stats = tempdir().unwrap(); let stats_file = stats.path().join("savings.jsonl"); @@ -626,8 +629,12 @@ mod tests { let content = std::fs::read_to_string(&stats_file).unwrap(); let lines: Vec<&str> = content.lines().filter(|l| !l.is_empty()).collect(); assert_eq!(lines.len(), 1); - assert!(lines[0].contains("\"call\":\"search\"")); - assert!(lines[0].contains("file_chars")); + let record: serde_json::Value = serde_json::from_str(lines[0]).unwrap(); + assert_eq!(record["call"], "search"); + // A nonzero value, not just the key: a lazy lookup that resolved nothing + // would still serialize `"file_chars":0`. + assert!(record["file_chars"].as_u64().unwrap() > 0); + assert!(record["snippet_chars"].as_u64().unwrap() > 0); } #[test] diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs index d81793a..7861067 100644 --- a/crates/csp/src/indexing/file_sizes.rs +++ b/crates/csp/src/indexing/file_sizes.rs @@ -10,6 +10,8 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Mutex; +use crate::indexing::create::MAX_FILE_BYTES; + /// UTF-16 character counts per repo-relative file path, resolved eagerly /// (captured) or lazily (read from a local root on demand). #[derive(Debug, Default)] @@ -19,9 +21,11 @@ pub struct FileSizes { captured: HashMap, /// Local source root read on demand for paths not in `captured`. lazy_root: Option, - /// Memo of lazily read sizes; `Mutex` because `CspIndex` is shared as - /// `Arc` across MCP calls. - memo: Mutex>, + /// Memo of lazily resolved sizes, negatives included — a path that cannot + /// be read must not re-pay the syscalls (and, for a file the indexer + /// accepted, a full read) on every later query. `Mutex` because `CspIndex` + /// is shared as `Arc` across MCP calls. + memo: Mutex>>, } impl FileSizes { @@ -47,18 +51,21 @@ impl FileSizes { } /// Character count for `file_path`: captured → memo → read from the lazy - /// root (memoized on success). `None` when unavailable or unreadable. + /// root. `None` when unavailable or unreadable; both outcomes are memoized, + /// so an unreadable path costs one read attempt per index, not one per + /// query. The memo lock is released across the read so concurrent lookups + /// of different files don't serialize. pub fn get(&self, file_path: &str) -> Option { if let Some(size) = self.captured.get(file_path) { return Some(*size); } let root = self.lazy_root.as_deref()?; if let Some(size) = self.lock_memo().get(file_path) { - return Some(*size); + return *size; } - let size = read_file_chars(root, file_path)?; + let size = read_file_chars(root, file_path); self.lock_memo().insert(file_path.to_string(), size); - Some(size) + size } /// `true` when sizes can be produced at all. Prefer this over inspecting @@ -67,7 +74,7 @@ impl FileSizes { !self.captured.is_empty() || self.lazy_root.is_some() } - fn lock_memo(&self) -> std::sync::MutexGuard<'_, HashMap> { + fn lock_memo(&self) -> std::sync::MutexGuard<'_, HashMap>> { self.memo.lock().unwrap_or_else(|e| e.into_inner()) } } @@ -82,20 +89,26 @@ impl FileSizes { /// over upstream, which joins the path unchecked). Only regular files are read: /// the file walker never follows symlinks, and a path that has since become a /// symlink, FIFO, or device must not be able to redirect or stall the read. +/// +/// The read is bounded by [`MAX_FILE_BYTES`], the same ceiling +/// `create_index_from_path` applies — lazily, this runs inside a live search, +/// so a file that has grown past the indexing limit since it was chunked must +/// not be slurped whole on the query path. Decoding is lossy, matching the +/// indexer (`String::from_utf8_lossy`) and upstream `read_file_text`'s +/// `errors="replace"`: a file with invalid UTF-8 still gets indexed, so it must +/// still be sized instead of silently contributing 0 `file_chars`. pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { let rel = Path::new(file_path); if !is_safe_relative_path(rel) { return None; } let full = root.join(rel); - let is_regular_file = std::fs::symlink_metadata(&full) - .map(|m| m.is_file()) - .unwrap_or(false); - if !is_regular_file { + let meta = std::fs::symlink_metadata(&full).ok()?; + if !meta.is_file() || meta.len() > MAX_FILE_BYTES { return None; } - let text = std::fs::read_to_string(&full).ok()?; - Some(text.encode_utf16().count() as u64) + let bytes = std::fs::read(&full).ok()?; + Some(String::from_utf8_lossy(&bytes).encode_utf16().count() as u64) } /// `true` when `path` is relative and contains no `..` or root component, so @@ -136,6 +149,7 @@ mod tests { std::fs::create_dir(&root).unwrap(); std::fs::write(outer.path().join("secret.txt"), "top secret").unwrap(); std::fs::write(root.join("real.ts"), "abcd").unwrap(); + std::fs::create_dir(root.join("dir.ts")).unwrap(); #[cfg(unix)] std::os::unix::fs::symlink(outer.path().join("secret.txt"), root.join("link.ts")).unwrap(); let abs = root.join("real.ts").to_string_lossy().into_owned(); @@ -144,10 +158,44 @@ mod tests { assert_eq!(sizes.get("../secret.txt"), None); assert_eq!(sizes.get(&abs), None); assert_eq!(sizes.get("missing.ts"), None); + assert_eq!(sizes.get("dir.ts"), None); #[cfg(unix)] assert_eq!(sizes.get("link.ts"), None); } + #[test] + fn lazy_memoizes_misses_so_they_are_read_once() { + let root = tempdir().unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("later.ts"), None); + // The miss is cached: a file appearing afterwards does not resurrect it, + // which is what proves no second read was attempted. + std::fs::write(root.path().join("later.ts"), "abcd").unwrap(); + assert_eq!(sizes.get("later.ts"), None); + } + + #[test] + fn lazy_sizes_non_utf8_files_lossily_like_the_indexer() { + let root = tempdir().unwrap(); + // Latin-1 byte: `create_index_from_path` decodes it lossily and indexes + // the file, so sizing must not reject it. + std::fs::write(root.path().join("legacy.js"), b"ab\xffcd").unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("legacy.js"), Some(5)); + } + + #[test] + fn lazy_skips_files_larger_than_the_indexing_ceiling() { + let root = tempdir().unwrap(); + let big = vec![b'a'; MAX_FILE_BYTES as usize + 1]; + std::fs::write(root.path().join("grown.ts"), &big).unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("grown.ts"), None); + } + #[test] fn captured_serves_known_paths_only() { let sizes = FileSizes::captured([("a.ts".to_string(), 7u64)].into_iter().collect()); diff --git a/crates/csp/src/indexing/index/tests.rs b/crates/csp/src/indexing/index/tests.rs index 0391bc7..4649b7d 100644 --- a/crates/csp/src/indexing/index/tests.rs +++ b/crates/csp/src/indexing/index/tests.rs @@ -364,7 +364,11 @@ fn from_path_reads_file_sizes_lazily() { let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); assert!(idx.file_sizes.is_available()); - assert_eq!(idx.file_sizes.get("sample.ts"), Some(19)); + // Look it up under the path the chunks carry, so a chunk-path/root drift + // (absolute paths, a stray prefix) fails here instead of silently zeroing + // `file_chars` at telemetry time. + assert_eq!(idx.chunks[0].file_path, "sample.ts"); + assert_eq!(idx.file_sizes.get(&idx.chunks[0].file_path), Some(19)); } #[test] diff --git a/crates/csp/src/stats.rs b/crates/csp/src/stats.rs index 04dd87d..81db3ed 100644 --- a/crates/csp/src/stats.rs +++ b/crates/csp/src/stats.rs @@ -7,7 +7,7 @@ //! Time bucketing uses UTC `YYYY-MM-DD` (compared lexicographically, which is //! chronological); `now_secs` is injected so summaries/reports are testable. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::io::{IsTerminal, Write as _}; use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -118,13 +118,13 @@ pub fn save_search_stats( .iter() .map(|r| delivered_chars(&r.chunk.content, max_snippet_lines)) .sum(); - let mut unique_paths: Vec<&str> = Vec::new(); - for r in results { - if !unique_paths.contains(&r.chunk.file_path.as_str()) { - unique_paths.push(r.chunk.file_path.as_str()); - } - } - let file_chars: u64 = unique_paths.iter().filter_map(|p| file_sizes.get(p)).sum(); + // Each source file is counted once, however many of its chunks ranked. + let mut seen: HashSet<&str> = HashSet::new(); + let file_chars: u64 = results + .iter() + .filter(|r| seen.insert(r.chunk.file_path.as_str())) + .filter_map(|r| file_sizes.get(&r.chunk.file_path)) + .sum(); let record = StatsRecord { ts: now_secs(), @@ -514,6 +514,32 @@ mod tests { assert_eq!(record.file_chars, 300); } + #[test] + fn save_reads_sizes_through_a_lazy_root() { + // The variant every local `from_path` / `load_from_disk` index uses: + // `save_search_stats` must resolve repo-relative result paths against + // the source root, not just a pre-seeded map. + let root = tempdir().unwrap(); + std::fs::write(root.path().join("a.ts"), "0123456789").unwrap(); + let dir = tempdir().unwrap(); + let file = dir.path().join("savings.jsonl"); + let results = vec![result("hello", "a.ts"), result("world", "gone.ts")]; + + save_search_stats( + &file, + &results, + CallType::Search, + &FileSizes::lazy(root.path().to_path_buf()), + None, + ); + + let content = std::fs::read_to_string(&file).unwrap(); + let record: StatsRecord = serde_json::from_str(content.lines().next().unwrap()).unwrap(); + // `a.ts` resolves off the root; the missing path simply contributes 0. + assert_eq!(record.file_chars, 10); + assert_eq!(record.snippet_chars, 10); + } + #[test] fn save_caps_snippet_chars_by_max_snippet_lines() { let dir = tempdir().unwrap(); From 17825abe2c6d7be45062d2f0aac9710cf098fb68 Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 21:19:38 +0900 Subject: [PATCH 3/7] chore(stats): dedup paths before reading sizes in compute_file_sizes Collect unique chunk paths first so an unreadable file is attempted once instead of once per chunk (Gemini review on #92). --- crates/csp/src/indexing/index.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/csp/src/indexing/index.rs b/crates/csp/src/indexing/index.rs index faaa8d6..2486744 100644 --- a/crates/csp/src/indexing/index.rs +++ b/crates/csp/src/indexing/index.rs @@ -395,16 +395,14 @@ impl CspIndex { /// skipped). Used for sources that won't outlive the build (a git clone's temp /// checkout); local paths read lazily via [`FileSizes::lazy`] instead. fn compute_file_sizes(root: &Path, chunks: &[Chunk]) -> HashMap { - let mut sizes: HashMap = HashMap::new(); - for chunk in chunks { - if sizes.contains_key(&chunk.file_path) { - continue; - } - if let Some(chars) = read_file_chars(root, &chunk.file_path) { - sizes.insert(chunk.file_path.clone(), chars); - } - } - sizes + // Dedup paths first so an unreadable file is attempted once, not once per chunk. + chunks + .iter() + .map(|c| &c.file_path) + .collect::>() + .into_iter() + .filter_map(|path| read_file_chars(root, path).map(|chars| (path.clone(), chars))) + .collect() } /// Shallow-clone `url` into `dir`, non-interactively. Rejects a ref starting From e85bd8cbb53fa840a1574b08a3b4eca73cfb5a8f Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 21:21:26 +0900 Subject: [PATCH 4/7] fix(stats): contain lazy size reads to the canonical index root Reject a symlink leaf, require the canonicalized path to stay under the canonicalized root (a symlinked intermediate directory no longer escapes), fstat the opened handle instead of the path, and cap the read at MAX_FILE_BYTES while reading (Greptile review on #92). --- crates/csp/src/indexing/file_sizes.rs | 39 +++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs index 7861067..4ca7d6c 100644 --- a/crates/csp/src/indexing/file_sizes.rs +++ b/crates/csp/src/indexing/file_sizes.rs @@ -7,6 +7,7 @@ //! returns is touched. Git sources still capture eagerly, at clone time. use std::collections::HashMap; +use std::io::Read as _; use std::path::{Path, PathBuf}; use std::sync::Mutex; @@ -103,11 +104,28 @@ pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { return None; } let full = root.join(rel); - let meta = std::fs::symlink_metadata(&full).ok()?; + // Reject a symlink at the leaf (the walker never indexes one) and, via + // canonicalization, a symlinked intermediate directory that would resolve + // the read outside `root`. + if std::fs::symlink_metadata(&full).ok()?.is_symlink() { + return None; + } + let canonical = full.canonicalize().ok()?; + if !canonical.starts_with(root.canonicalize().ok()?) { + return None; + } + // fstat the opened handle rather than the path, so the regular-file and + // size checks apply to what is actually read, and cap the read itself. + let file = std::fs::File::open(&canonical).ok()?; + let meta = file.metadata().ok()?; if !meta.is_file() || meta.len() > MAX_FILE_BYTES { return None; } - let bytes = std::fs::read(&full).ok()?; + let mut bytes = Vec::with_capacity(meta.len() as usize); + file.take(MAX_FILE_BYTES + 1).read_to_end(&mut bytes).ok()?; + if bytes.len() as u64 > MAX_FILE_BYTES { + return None; + } Some(String::from_utf8_lossy(&bytes).encode_utf16().count() as u64) } @@ -163,6 +181,23 @@ mod tests { assert_eq!(sizes.get("link.ts"), None); } + #[cfg(unix)] + #[test] + fn lazy_rejects_symlinked_intermediate_directory() { + let outer = tempdir().unwrap(); + let root = outer.path().join("repo"); + std::fs::create_dir(&root).unwrap(); + let outside = outer.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::fs::write(outside.join("leak.ts"), "top secret").unwrap(); + // `repo/vendor` -> `../outside`: the leaf is a regular file, but the + // path only reaches it through a symlinked directory. + std::os::unix::fs::symlink(&outside, root.join("vendor")).unwrap(); + + let sizes = FileSizes::lazy(root); + assert_eq!(sizes.get("vendor/leak.ts"), None); + } + #[test] fn lazy_memoizes_misses_so_they_are_read_once() { let root = tempdir().unwrap(); From 48dab116cf34d8dc3ad4bb5e97f9f653fbd4f414 Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 21:23:53 +0900 Subject: [PATCH 5/7] docs(stats): document the residual canonicalize-then-open race in read_file_chars --- crates/csp/src/indexing/file_sizes.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs index 4ca7d6c..e52f694 100644 --- a/crates/csp/src/indexing/file_sizes.rs +++ b/crates/csp/src/indexing/file_sizes.rs @@ -116,6 +116,14 @@ pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { } // fstat the opened handle rather than the path, so the regular-file and // size checks apply to what is actually read, and cap the read itself. + // + // Residual race: `canonicalize` and `File::open` are separate path walks, + // so a writer swapping a parent directory for a symlink in between can make + // the open follow it to a regular file outside `root`. Closing that needs a + // descriptor-relative no-follow walk (`openat` + `O_NOFOLLOW` per component), + // which `std` does not expose portably. The exposure is a UTF-16 length of + // that file written to the user's own `savings.jsonl`, never its content, by + // a local writer who already controls the indexed tree. let file = std::fs::File::open(&canonical).ok()?; let meta = file.metadata().ok()?; if !meta.is_file() || meta.len() > MAX_FILE_BYTES { From 646cfc826e2b87f08739c54ecb24a1fe410631cd Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 21:25:57 +0900 Subject: [PATCH 6/7] perf(stats): canonicalize the size root once instead of per read FileSizes::lazy and compute_file_sizes canonicalize the root at construction; read_file_chars now requires a canonical root and does a plain prefix check (Gemini review on #92). --- crates/csp/src/indexing/file_sizes.rs | 14 ++++++++++---- crates/csp/src/indexing/index.rs | 8 ++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs index e52f694..c60d1b7 100644 --- a/crates/csp/src/indexing/file_sizes.rs +++ b/crates/csp/src/indexing/file_sizes.rs @@ -43,8 +43,12 @@ impl FileSizes { } } - /// Sizes read on demand from a still-present local source root. + /// Sizes read on demand from a still-present local source root. The root + /// is canonicalized once here so each lookup's containment check is a plain + /// prefix comparison; a root that cannot be canonicalized is kept as-is and + /// every lookup then fails containment, which is the safe outcome. pub fn lazy(root: PathBuf) -> Self { + let root = root.canonicalize().unwrap_or(root); Self { lazy_root: Some(root), ..Self::default() @@ -81,8 +85,10 @@ impl FileSizes { } /// UTF-16 character count of the repo-relative `file_path` under `root`, or -/// `None` when it cannot be read. UTF-16 keeps it consistent with -/// `stats::save_search_stats`'s snippet accounting. +/// `None` when it cannot be read. `root` must already be canonical (see +/// [`FileSizes::lazy`]); the containment check below is a prefix comparison +/// against it. UTF-16 keeps it consistent with `stats::save_search_stats`'s +/// snippet accounting. /// /// Chunk paths are repo-relative by construction; a path that is absolute or /// escapes `root` via `..` can only come from a tampered on-disk index, so it is @@ -111,7 +117,7 @@ pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { return None; } let canonical = full.canonicalize().ok()?; - if !canonical.starts_with(root.canonicalize().ok()?) { + if !canonical.starts_with(root) { return None; } // fstat the opened handle rather than the path, so the regular-file and diff --git a/crates/csp/src/indexing/index.rs b/crates/csp/src/indexing/index.rs index 2486744..1026d41 100644 --- a/crates/csp/src/indexing/index.rs +++ b/crates/csp/src/indexing/index.rs @@ -395,13 +395,17 @@ impl CspIndex { /// skipped). Used for sources that won't outlive the build (a git clone's temp /// checkout); local paths read lazily via [`FileSizes::lazy`] instead. fn compute_file_sizes(root: &Path, chunks: &[Chunk]) -> HashMap { - // Dedup paths first so an unreadable file is attempted once, not once per chunk. + // Canonicalize once (`read_file_chars` needs a canonical root) and dedup + // paths first so an unreadable file is attempted once, not once per chunk. + let Ok(root) = root.canonicalize() else { + return HashMap::new(); + }; chunks .iter() .map(|c| &c.file_path) .collect::>() .into_iter() - .filter_map(|path| read_file_chars(root, path).map(|chars| (path.clone(), chars))) + .filter_map(|path| read_file_chars(&root, path).map(|chars| (path.clone(), chars))) .collect() } From 437bf86635b1b1a25563cc9c969698b8707c247d Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 4 Sep 2026 21:33:18 +0900 Subject: [PATCH 7/7] fix(stats): reject non-regular files before opening them in lazy size reads A FIFO at an indexed path made File::open block until a writer appeared, stalling the search that triggered the size lookup. Check the canonical path with symlink_metadata first; the fstat on the opened handle stays as the race guard. Raised by cubic on #92. --- crates/csp/src/indexing/file_sizes.rs | 28 ++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs index c60d1b7..9b64433 100644 --- a/crates/csp/src/indexing/file_sizes.rs +++ b/crates/csp/src/indexing/file_sizes.rs @@ -120,9 +120,14 @@ pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { if !canonical.starts_with(root) { return None; } - // fstat the opened handle rather than the path, so the regular-file and - // size checks apply to what is actually read, and cap the read itself. - // + // Reject a non-regular file *before* opening it: `open(2)` on a FIFO + // blocks until a writer shows up, which would stall the search path, and + // opening a device node can have side effects. The fstat below re-checks + // the opened handle so the regular-file and size checks apply to what is + // actually read, and the read itself is capped. + if !std::fs::symlink_metadata(&canonical).ok()?.is_file() { + return None; + } // Residual race: `canonicalize` and `File::open` are separate path walks, // so a writer swapping a parent directory for a symlink in between can make // the open follow it to a regular file outside `root`. Closing that needs a @@ -212,6 +217,23 @@ mod tests { assert_eq!(sizes.get("vendor/leak.ts"), None); } + #[cfg(unix)] + #[test] + fn lazy_rejects_fifo_without_blocking() { + let root = tempdir().unwrap(); + let fifo = root.path().join("pipe.ts"); + let status = std::process::Command::new("mkfifo") + .arg(&fifo) + .status() + .unwrap(); + assert!(status.success()); + + // A FIFO with no writer would block `File::open` forever; the + // pre-open regular-file check must skip it instead. + let sizes = FileSizes::lazy(root.path().to_path_buf()); + assert_eq!(sizes.get("pipe.ts"), None); + } + #[test] fn lazy_memoizes_misses_so_they_are_read_once() { let root = tempdir().unwrap();