From 5d79a7a2756cd606b10ba0bc485749ed4ea297bd Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Thu, 24 Sep 2026 16:49:55 -0600 Subject: [PATCH 1/5] perf: cache loaded teams in find_file_owners find_file_owners re-globbed and re-parsed every team config file on each call. In a large monorepo that load was ~96% of every lookup (~21ms of ~22ms), so callers resolving ownership one file at a time paid it again for every file. Memoize the loaded teams and the by-name map per project root and team file globs, matching how teams_by_github_team_name already caches teams for CODEOWNERS lookups, and add clear_team_cache() for callers whose team files change within a process. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/ownership/file_owner_resolver.rs | 73 +++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index b0a23dd..fedaa48 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -1,11 +1,13 @@ use std::{ collections::{HashMap, HashSet}, fs, - path::Path, + path::{Path, PathBuf}, + sync::Arc, }; use fast_glob::glob_match; use glob::glob; +use memoize::memoize; use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache}; @@ -19,8 +21,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(project_root.to_path_buf(), 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 +41,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 +67,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 +101,25 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) Ok(file_owners) } +struct LoadedTeams { + teams: Vec, + teams_by_name: HashMap, +} + +// Parsing every team file dominates a lookup, so load them once per project root and glob list for the +// life of the process, as teams_by_github_team_name does for CODEOWNERS lookups. +#[memoize] +fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { + let teams = load_teams(&project_root, &team_file_globs)?; + let teams_by_name = build_teams_by_name_map(&teams); + Ok(Arc::new(LoadedTeams { teams, teams_by_name })) +} + +/// Drops the teams memoized by `find_file_owners`, for callers whose team files change within one process. +pub fn clear_team_cache() { + memoized_flush_loaded_teams(); +} + fn build_teams_by_name_map(teams: &[Team]) -> HashMap { let mut map = HashMap::with_capacity(teams.len() * 2); for team in teams { @@ -504,4 +526,39 @@ mod tests { assert_eq!(result.0, "Payroll"); matches!(result.1, Source::TeamGem); } + + #[test] + fn test_find_file_owners_reuses_loaded_teams_until_cleared() { + 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())); + } } From d9c6f076fd35680311bae1581e7d4d5bf6ca959b Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Thu, 24 Sep 2026 17:10:48 -0600 Subject: [PATCH 2/5] Re-export clear_team_cache from runner The Ruby extension only uses codeowners::runner, so expose the cache clear there for CodeOwnership.bust_caches! to call. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/runner/api.rs | 2 ++ 1 file changed, 2 insertions(+) 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); From eae97bc93564f7e8b2884cf3cfcc35fb434e0c6a Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Thu, 24 Sep 2026 17:53:29 -0600 Subject: [PATCH 3/5] Share the team cache across threads and key it on the absolute root #[memoize] caches per thread by default, so each thread paid the team load and clear_team_cache only cleared the calling thread. Use SharedCache so the load happens once per process and a clear applies to every thread. Key the cache on the absolute project root so a relative root is not reused after the working directory changes. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/ownership/file_owner_resolver.rs | 67 ++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index fedaa48..d23dbd2 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -21,7 +21,7 @@ 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 loaded = loaded_teams(project_root.to_path_buf(), config.team_file_glob.clone())?; + 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; @@ -107,14 +107,20 @@ struct LoadedTeams { } // Parsing every team file dominates a lookup, so load them once per project root and glob list for the -// life of the process, as teams_by_github_team_name does for CODEOWNERS lookups. -#[memoize] +// life of the process. SharedCache makes that one load per process rather than per thread, and lets +// clear_team_cache clear it for every thread. +#[memoize(SharedCache)] fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { let teams = load_teams(&project_root, &team_file_globs)?; let teams_by_name = build_teams_by_name_map(&teams); Ok(Arc::new(LoadedTeams { teams, teams_by_name })) } +// 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() { memoized_flush_loaded_teams(); @@ -527,8 +533,20 @@ mod tests { 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"); @@ -561,4 +579,47 @@ mod tests { 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" + ); + } } From 64e29a3aea18036d6e65b2ad08f6bf5847ae26a3 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Fri, 25 Sep 2026 13:12:37 -0600 Subject: [PATCH 4/5] Don't cache a team load that skipped a team file load_teams skips a team file it can't read or parse and still returns Ok, so the partial set was memoized until the process restarted. Track whether anything was skipped and only cache complete loads, so fixing the file takes effect on the next lookup. memoize can't cache conditionally, so this replaces #[memoize(SharedCache)] with a small process-wide map behind a Mutex. Load errors are no longer cached either. Also adds a test that the team file glob is part of the cache key. --- src/ownership/file_owner_resolver.rs | 136 ++++++++++++++++++++++++--- 1 file changed, 122 insertions(+), 14 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index d23dbd2..d9b9957 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -2,12 +2,11 @@ use std::{ collections::{HashMap, HashSet}, fs, path::{Path, PathBuf}, - sync::Arc, + sync::{Arc, LazyLock, Mutex, MutexGuard, PoisonError}, }; use fast_glob::glob_match; use glob::glob; -use memoize::memoize; use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache}; @@ -106,14 +105,33 @@ struct LoadedTeams { teams_by_name: HashMap, } -// Parsing every team file dominates a lookup, so load them once per project root and glob list for the -// life of the process. SharedCache makes that one load per process rather than per thread, and lets -// clear_team_cache clear it for every thread. -#[memoize(SharedCache)] +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); + +fn team_cache() -> MutexGuard<'static, HashMap>> { + TEAM_CACHE.lock().unwrap_or_else(PoisonError::into_inner) +} + fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { - let teams = load_teams(&project_root, &team_file_globs)?; - let teams_by_name = build_teams_by_name_map(&teams); - Ok(Arc::new(LoadedTeams { teams, teams_by_name })) + let key = (project_root, team_file_globs); + if let Some(loaded) = team_cache().get(&key) { + return Ok(Arc::clone(loaded)); + } + + 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 { + team_cache().insert(key, Arc::clone(&loaded)); + } + Ok(loaded) } // Keyed on the absolute root so a relative root isn't reused after the working directory changes. @@ -123,7 +141,7 @@ fn teams_cache_root(project_root: &Path) -> PathBuf { /// Drops the teams memoized by `find_file_owners`, for callers whose team files change within one process. pub fn clear_team_cache() { - memoized_flush_loaded_teams(); + team_cache().clear(); } fn build_teams_by_name_map(teams: &[Team]) -> HashMap { @@ -135,22 +153,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 { @@ -622,4 +654,80 @@ mod tests { "a clear on another thread applies here too" ); } + + #[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())); + } } From c4631fa46a436d37a82b00e6d5da455832ee980d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 25 Sep 2026 14:58:12 -0700 Subject: [PATCH 5/5] Don't let a load that raced a clear reinstate stale teams The lock is released while `load_teams` reads the team files, so a load that was already running when `clear_team_cache()` fired could finish and insert its result after the clear, silently undoing it. Once `bust_caches!` calls `clear_team_cache()`, that would leave a stale answer right after an explicit bust. Keep a generation counter next to the map, under the same lock. A lookup records the generation along with its cache miss, `clear_team_cache()` bumps it, and a finished load only inserts if the generation hasn't moved. Reading and checking it under the lock that guards the map avoids any separate memory-ordering argument. The insert moves into `cache_teams` so the race can be tested deterministically: record the generation, clear, then insert, and check the result isn't cached. Also adds a test for a team directory the glob can't read. `glob` yields that as an `Err` entry rather than an empty match, so it's a separate way to skip a team file from a parse error, and the fix for partial loads relies on it being flagged too. The test skips itself where permissions aren't enforced, as when running as root. Both new tests fail when their guard is removed: without the generation check a result loaded before a clear gets cached, and without flagging the glob error an unreadable directory's partial load is cached (left: None, right: Some("Billing")). --- src/ownership/file_owner_resolver.rs | 110 +++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 7 deletions(-) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index d9b9957..cc7f9fa 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -109,17 +109,28 @@ 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); +static TEAM_CACHE: LazyLock> = LazyLock::new(Default::default); -fn team_cache() -> MutexGuard<'static, HashMap>> { +#[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); - if let Some(loaded) = team_cache().get(&key) { - return Ok(Arc::clone(loaded)); - } + 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); @@ -129,11 +140,18 @@ fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::res }); // 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 { - team_cache().insert(key, Arc::clone(&loaded)); + 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()) @@ -141,7 +159,9 @@ fn teams_cache_root(project_root: &Path) -> PathBuf { /// Drops the teams memoized by `find_file_owners`, for callers whose team files change within one process. pub fn clear_team_cache() { - team_cache().clear(); + let mut cache = team_cache(); + cache.generation += 1; + cache.loaded.clear(); } fn build_teams_by_name_map(teams: &[Team]) -> HashMap { @@ -655,6 +675,82 @@ mod tests { ); } + #[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());