diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index b0a23dd..cc7f9fa 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -1,7 +1,8 @@ use std::{ collections::{HashMap, HashSet}, fs, - path::Path, + path::{Path, PathBuf}, + sync::{Arc, LazyLock, Mutex, MutexGuard, PoisonError}, }; use fast_glob::glob_match; @@ -19,8 +20,9 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) }; let relative_file_path = crate::path_utils::relative_to_buf(project_root, &absolute_file_path); - let teams = load_teams(project_root, &config.team_file_glob)?; - let teams_by_name = build_teams_by_name_map(&teams); + let loaded = loaded_teams(teams_cache_root(project_root), config.team_file_glob.clone())?; + let teams = &loaded.teams; + let teams_by_name = &loaded.teams_by_name; let mut sources_by_team: HashMap> = HashMap::new(); @@ -38,20 +40,20 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) } } - if let Some((owner_team_name, dir_source)) = most_specific_directory_owner(project_root, &relative_file_path, &teams_by_name) { + if let Some((owner_team_name, dir_source)) = most_specific_directory_owner(project_root, &relative_file_path, teams_by_name) { sources_by_team.entry(owner_team_name).or_default().push(dir_source); } - if let Some((owner_team_name, package_source)) = nearest_package_owner(project_root, &relative_file_path, config, &teams_by_name) { + if let Some((owner_team_name, package_source)) = nearest_package_owner(project_root, &relative_file_path, config, teams_by_name) { sources_by_team.entry(owner_team_name).or_default().push(package_source); } - if let Some((owner_team_name, gem_source)) = vendored_gem_owner(&relative_file_path, config, &teams) { + if let Some((owner_team_name, gem_source)) = vendored_gem_owner(&relative_file_path, config, teams) { sources_by_team.entry(owner_team_name).or_default().push(gem_source); } if let Some(rel_str) = relative_file_path.to_str() { - for team in &teams { + for team in teams { let subtracts: HashSet<&str> = team.subtracted_globs.iter().map(|s| s.as_str()).collect(); for owned_glob in &team.owned_globs { if glob_match(owned_glob, rel_str) && !subtracts.iter().any(|sub| glob_match(sub, rel_str)) { @@ -64,7 +66,7 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) } } - for team in &teams { + for team in teams { let team_rel = crate::path_utils::relative_to_buf(project_root, &team.path); if team_rel == relative_file_path { sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamYml); @@ -98,6 +100,70 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) Ok(file_owners) } +struct LoadedTeams { + teams: Vec, + teams_by_name: HashMap, +} + +type TeamCacheKey = (PathBuf, Vec); + +// Parsing every team file dominates a lookup, so load them once per project root and glob list and share +// the result across threads for the life of the process. +static TEAM_CACHE: LazyLock> = LazyLock::new(Default::default); + +#[derive(Default)] +struct TeamCache { + // Bumped by `clear_team_cache` so a load already running during a clear can't reinstate its stale result. + generation: u64, + loaded: HashMap>, +} + +fn team_cache() -> MutexGuard<'static, TeamCache> { + TEAM_CACHE.lock().unwrap_or_else(PoisonError::into_inner) +} + +fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { + let key = (project_root, team_file_globs); + let generation = { + let cache = team_cache(); + if let Some(loaded) = cache.loaded.get(&key) { + return Ok(Arc::clone(loaded)); + } + cache.generation + }; + + let load = load_teams(&key.0, &key.1)?; + let teams_by_name = build_teams_by_name_map(&load.teams); + let loaded = Arc::new(LoadedTeams { + teams: load.teams, + teams_by_name, + }); + // A load that skipped a team file isn't cached, so fixing the file takes effect on the next lookup. + if !load.skipped_team_file { + cache_teams(key, Arc::clone(&loaded), generation); + } + Ok(loaded) +} + +fn cache_teams(key: TeamCacheKey, loaded: Arc, generation: u64) { + let mut cache = team_cache(); + if cache.generation == generation { + cache.loaded.insert(key, loaded); + } +} + +// Keyed on the absolute root so a relative root isn't reused after the working directory changes. +fn teams_cache_root(project_root: &Path) -> PathBuf { + std::path::absolute(project_root).unwrap_or_else(|_| project_root.to_path_buf()) +} + +/// Drops the teams memoized by `find_file_owners`, for callers whose team files change within one process. +pub fn clear_team_cache() { + let mut cache = team_cache(); + cache.generation += 1; + cache.loaded.clear(); +} + fn build_teams_by_name_map(teams: &[Team]) -> HashMap { let mut map = HashMap::with_capacity(teams.len() * 2); for team in teams { @@ -107,22 +173,36 @@ fn build_teams_by_name_map(teams: &[Team]) -> HashMap { map } -fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::Result, String> { +struct TeamLoad { + teams: Vec, + skipped_team_file: bool, +} + +fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::Result { let mut teams: Vec = Vec::new(); + let mut skipped_team_file = false; for glob_str in team_file_globs { let absolute_glob = project_root.join(glob_str).to_string_lossy().into_owned(); let paths = glob(&absolute_glob).map_err(|e| e.to_string())?; - for path in paths.flatten() { + for entry in paths { + let path = match entry { + Ok(path) => path, + Err(e) => { + eprintln!("Error reading team file path: {e}"); + skipped_team_file = true; + continue; + } + }; match Team::from_team_file_path(path.clone()) { Ok(team) => teams.push(team), Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); - continue; + skipped_team_file = true; } } } } - Ok(teams) + Ok(TeamLoad { teams, skipped_team_file }) } fn read_top_of_file_team(path: &Path) -> Option { @@ -504,4 +584,246 @@ mod tests { assert_eq!(result.0, "Payroll"); matches!(result.1, Source::TeamGem); } + + #[test] + fn test_teams_cache_root_is_absolute_for_relative_roots() { + let cwd = std::env::current_dir().unwrap(); + assert_eq!(teams_cache_root(Path::new("some/project")), cwd.join("some/project")); + assert_eq!(teams_cache_root(Path::new(".")), cwd.join(".")); + assert_eq!(teams_cache_root(&cwd), cwd); + } + + // The team cache is process-wide, so tests that clear it must not interleave. + static TEAM_CACHE_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn test_find_file_owners_reuses_loaded_teams_until_cleared() { + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + let td = tempdir().unwrap(); + let root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + let write_team = |glob: &str| { + fs::create_dir_all(root.join("config/teams")).unwrap(); + fs::write( + root.join("config/teams/payroll.yml"), + format!("name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - {glob}\n"), + ) + .unwrap(); + }; + let owner_of = |file: &str| { + find_file_owners(root, &config, Path::new(file)) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + write_team("app/payroll/**/*"); + assert_eq!(owner_of("app/payroll/a.rb"), Some("Payroll".to_string())); + + write_team("app/other/**/*"); + assert_eq!( + owner_of("app/payroll/a.rb"), + Some("Payroll".to_string()), + "team files are loaded once per process" + ); + + clear_team_cache(); + assert_eq!(owner_of("app/payroll/a.rb"), None); + assert_eq!(owner_of("app/other/a.rb"), Some("Payroll".to_string())); + } + + #[test] + fn test_find_file_owners_shares_loaded_teams_across_threads() { + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + let td = tempdir().unwrap(); + let root = td.path().to_path_buf(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + let write_team = |glob: &str| { + fs::create_dir_all(root.join("config/teams")).unwrap(); + fs::write( + root.join("config/teams/payroll.yml"), + format!("name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - {glob}\n"), + ) + .unwrap(); + }; + let owner_of = |root: &Path, config: &crate::config::Config, file: &str| { + find_file_owners(root, config, Path::new(file)) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + write_team("app/payroll/**/*"); + assert_eq!(owner_of(&root, &config, "app/payroll/a.rb"), Some("Payroll".to_string())); + write_team("app/other/**/*"); + + std::thread::scope(|scope| { + scope.spawn(|| { + assert_eq!( + owner_of(&root, &config, "app/payroll/a.rb"), + Some("Payroll".to_string()), + "another thread reuses the teams loaded by the first" + ); + clear_team_cache(); + }); + }); + + assert_eq!( + owner_of(&root, &config, "app/payroll/a.rb"), + None, + "a clear on another thread applies here too" + ); + } + + #[test] + fn test_clear_team_cache_during_a_load_is_not_undone_by_its_result() { + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + let key: TeamCacheKey = (PathBuf::from("/clear-during-load"), vec!["config/teams/**/*.yml".to_string()]); + let loaded = || { + Arc::new(LoadedTeams { + teams: Vec::new(), + teams_by_name: HashMap::new(), + }) + }; + + let generation = team_cache().generation; + clear_team_cache(); + cache_teams(key.clone(), loaded(), generation); + assert!( + !team_cache().loaded.contains_key(&key), + "a load that started before the clear must not be cached" + ); + + let generation = team_cache().generation; + cache_teams(key.clone(), loaded(), generation); + assert!(team_cache().loaded.contains_key(&key)); + clear_team_cache(); + } + + #[cfg(unix)] + #[test] + fn test_find_file_owners_does_not_cache_a_load_with_an_unreadable_team_directory() { + use std::os::unix::fs::PermissionsExt; + + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + let td = tempdir().unwrap(); + let root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + let owner_of = |file: &str| { + find_file_owners(root, &config, Path::new(file)) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + let teams_dir = root.join("config/teams"); + let locked_dir = teams_dir.join("billing"); + fs::create_dir_all(&locked_dir).unwrap(); + fs::write( + teams_dir.join("payroll.yml"), + "name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - app/payroll/**/*\n", + ) + .unwrap(); + fs::write( + locked_dir.join("billing.yml"), + "name: Billing\ngithub:\n team: '@BillingTeam'\nowned_globs:\n - app/billing/**/*\n", + ) + .unwrap(); + + fs::set_permissions(&locked_dir, fs::Permissions::from_mode(0o000)).unwrap(); + // Permissions aren't enforced for root, so the directory can't be made unreadable there. + if fs::read_dir(&locked_dir).is_ok() { + fs::set_permissions(&locked_dir, fs::Permissions::from_mode(0o755)).unwrap(); + return; + } + let payroll = owner_of("app/payroll/a.rb"); + let billing_while_unreadable = owner_of("app/billing/a.rb"); + fs::set_permissions(&locked_dir, fs::Permissions::from_mode(0o755)).unwrap(); + let billing_once_readable = owner_of("app/billing/a.rb"); + + assert_eq!(payroll, Some("Payroll".to_string())); + assert_eq!(billing_while_unreadable, None); + assert_eq!( + billing_once_readable, + Some("Billing".to_string()), + "a team directory that becomes readable takes effect without clearing the cache" + ); + clear_team_cache(); + } + + #[test] + fn test_find_file_owners_does_not_cache_a_load_that_skipped_a_team_file() { + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + let td = tempdir().unwrap(); + let root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + let write_team_file = |file: &str, contents: &str| { + fs::create_dir_all(root.join("config/teams")).unwrap(); + fs::write(root.join("config/teams").join(file), contents).unwrap(); + }; + let owner_of = |file: &str| { + find_file_owners(root, &config, Path::new(file)) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + write_team_file( + "payroll.yml", + "name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - app/payroll/**/*\n", + ); + write_team_file("billing.yml", "name: [unclosed\n"); + assert_eq!(owner_of("app/payroll/a.rb"), Some("Payroll".to_string())); + assert_eq!(owner_of("app/billing/a.rb"), None); + + write_team_file( + "billing.yml", + "name: Billing\ngithub:\n team: '@BillingTeam'\nowned_globs:\n - app/billing/**/*\n", + ); + assert_eq!( + owner_of("app/billing/a.rb"), + Some("Billing".to_string()), + "fixing the team file takes effect without clearing the cache" + ); + + write_team_file( + "billing.yml", + "name: Billing\ngithub:\n team: '@BillingTeam'\nowned_globs:\n - app/other/**/*\n", + ); + assert_eq!( + owner_of("app/billing/a.rb"), + Some("Billing".to_string()), + "once every team file loads, the teams are cached" + ); + } + + #[test] + fn test_team_cache_is_keyed_on_team_file_glob() { + let td = tempdir().unwrap(); + let root = td.path(); + for (dir, name) in [("a", "Payroll"), ("b", "Billing")] { + fs::create_dir_all(root.join("config/teams").join(dir)).unwrap(); + fs::write( + root.join("config/teams").join(dir).join("team.yml"), + format!("name: {name}\ngithub:\n team: '@{name}Team'\nowned_globs:\n - app/**/*\n"), + ) + .unwrap(); + } + let config_with_team_glob = |team_glob: &str| crate::config::Config { + team_file_glob: vec![team_glob.to_string()], + ..build_config_for_temp("frontend/**/*", "packs/**/*", "vendored") + }; + let config_a = config_with_team_glob("config/teams/a/*.yml"); + let config_b = config_with_team_glob("config/teams/b/*.yml"); + let owner_of = |config: &crate::config::Config| { + find_file_owners(root, config, Path::new("app/a.rb")) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + assert_eq!(owner_of(&config_a), Some("Payroll".to_string())); + assert_eq!(owner_of(&config_b), Some("Billing".to_string())); + assert_eq!(owner_of(&config_a), Some("Payroll".to_string())); + } } diff --git a/src/runner/api.rs b/src/runner/api.rs index 0c09b4a..bf4e9c9 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -6,6 +6,8 @@ use error_stack::Report; use super::{Error, ForFileResult, RunConfig, RunResult, run}; +pub use crate::ownership::file_owner_resolver::clear_team_cache; + pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool, json: bool) -> RunResult { if from_codeowners { return for_file_codeowners_only_fast(run_config, file_path, json);