diff --git a/packages/core/src/repowise/core/ingestion/external_systems/cmake.py b/packages/core/src/repowise/core/ingestion/external_systems/cmake.py index bcabdd00f..8099858cd 100644 --- a/packages/core/src/repowise/core/ingestion/external_systems/cmake.py +++ b/packages/core/src/repowise/core/ingestion/external_systems/cmake.py @@ -31,6 +31,8 @@ import structlog +from repowise.core.fs_walk import WalkSnapshot + from ..languages.specs.cpp import INCLUDE_FRAGMENT_EXTENSIONS from .base import ExternalSystemRecord @@ -595,6 +597,7 @@ def discover_cmake_reactor( repo_root: Path, *, max_files: int = 2000, + snapshot: WalkSnapshot | None = None, ) -> list[CMakeFile]: """Walk the repo from ``repo_root`` following ``add_subdirectory`` links. @@ -637,9 +640,9 @@ def visit(rel_dir: str) -> None: skip_dirs = {".git", "build", "_build", "out", "_deps", "cmake-build-debug", "cmake-build-release", "node_modules", ".venv", "venv"} if len(out) < max_files: - from repowise.core.fs_walk import iter_glob + from repowise.core.fs_walk import glob_via - for cml in iter_glob(repo_root, "CMakeLists.txt"): + for cml in glob_via(snapshot, repo_root, "CMakeLists.txt"): try: rel = cml.resolve().relative_to(repo_root).as_posix() except ValueError: diff --git a/packages/core/src/repowise/core/ingestion/resolvers/cpp_workspace.py b/packages/core/src/repowise/core/ingestion/resolvers/cpp_workspace.py index 14e297705..351249d80 100644 --- a/packages/core/src/repowise/core/ingestion/resolvers/cpp_workspace.py +++ b/packages/core/src/repowise/core/ingestion/resolvers/cpp_workspace.py @@ -373,7 +373,11 @@ def build_cpp_workspace_index(ctx: ResolverContext) -> CppWorkspaceIndex: # is what that path did before. source_map = getattr(ctx, "source_map", None) - cmake_files = discover_cmake_reactor(repo_path) + # Same reason as ``source_map`` above: the stand-in context has no + # snapshot, and None means the live walk this always did. + cmake_files = discover_cmake_reactor( + repo_path, snapshot=getattr(ctx, "walk_snapshot", None) + ) file_api_targets = parse_cmake_file_api_reply(repo_path) bazel_files = discover_bazel_packages(repo_path) if is_bazel_repo(repo_path) else [] diff --git a/packages/core/src/repowise/core/ingestion/resolvers/ruby.py b/packages/core/src/repowise/core/ingestion/resolvers/ruby.py index d694aa438..7a3005e9e 100644 --- a/packages/core/src/repowise/core/ingestion/resolvers/ruby.py +++ b/packages/core/src/repowise/core/ingestion/resolvers/ruby.py @@ -67,16 +67,26 @@ def _scan_gem_metadata(ctx: ResolverContext) -> tuple[frozenset[str], tuple[str, names: set[str] = set() lib_roots: set[str] = set() if ctx.repo_path is not None: - from repowise.core.fs_walk import iter_glob + from repowise.core.fs_walk import glob_via repo = ctx.repo_path.resolve() candidates: list = [] try: candidates.extend( - iter_glob(ctx.repo_path, "Gemfile", prune_nested_git=ctx.prune_nested_git) + glob_via( + ctx.walk_snapshot, + ctx.repo_path, + "Gemfile", + prune_nested_git=ctx.prune_nested_git, + ) ) candidates.extend( - iter_glob(ctx.repo_path, "*.gemspec", prune_nested_git=ctx.prune_nested_git) + glob_via( + ctx.walk_snapshot, + ctx.repo_path, + "*.gemspec", + prune_nested_git=ctx.prune_nested_git, + ) ) except OSError: pass diff --git a/packages/core/src/repowise/core/ingestion/resolvers/swift_spm.py b/packages/core/src/repowise/core/ingestion/resolvers/swift_spm.py index a268f591f..8c570392e 100644 --- a/packages/core/src/repowise/core/ingestion/resolvers/swift_spm.py +++ b/packages/core/src/repowise/core/ingestion/resolvers/swift_spm.py @@ -19,7 +19,7 @@ from pathlib import Path from typing import TYPE_CHECKING -from repowise.core.fs_walk import iter_glob +from repowise.core.fs_walk import WalkSnapshot, glob_via if TYPE_CHECKING: from .context import ResolverContext @@ -64,7 +64,10 @@ def parse_package_swift(path: Path) -> dict[str, str]: def build_swift_targets( - repo_path: Path | None, *, prune_nested_git: bool = True + repo_path: Path | None, + *, + prune_nested_git: bool = True, + snapshot: WalkSnapshot | None = None, ) -> dict[str, str]: """Walk the repo for every ``Package.swift``, merge their target maps. @@ -75,7 +78,9 @@ def build_swift_targets( if repo_path is None or not repo_path.is_dir(): return {} merged: dict[str, str] = {} - for pkg_swift in iter_glob(repo_path, "Package.swift", prune_nested_git=prune_nested_git): + for pkg_swift in glob_via( + snapshot, repo_path, "Package.swift", prune_nested_git=prune_nested_git + ): try: pkg_dir = pkg_swift.parent.relative_to(repo_path).as_posix() except ValueError: @@ -90,7 +95,9 @@ def get_or_build_swift_targets(ctx: ResolverContext) -> dict[str, str]: cached = getattr(ctx, "_swift_targets", None) if cached is not None: return cached - mapping = build_swift_targets(ctx.repo_path, prune_nested_git=ctx.prune_nested_git) + mapping = build_swift_targets( + ctx.repo_path, prune_nested_git=ctx.prune_nested_git, snapshot=ctx.walk_snapshot + ) ctx._swift_targets = mapping # type: ignore[attr-defined] return mapping diff --git a/packages/core/src/repowise/core/ingestion/traverser.py b/packages/core/src/repowise/core/ingestion/traverser.py index b10bed964..c343b4cbb 100644 --- a/packages/core/src/repowise/core/ingestion/traverser.py +++ b/packages/core/src/repowise/core/ingestion/traverser.py @@ -18,7 +18,7 @@ import configparser import os import threading -from collections.abc import Iterable, Iterator +from collections.abc import Callable, Iterable, Iterator from dataclasses import dataclass, field from datetime import datetime from pathlib import Path @@ -436,6 +436,8 @@ def __init__( self._console_scripts_prune_nested = not (include_submodules or include_nested_repos) self.stats = TraversalStats() self._count_lock = threading.Lock() + self._console_scripts_lock = threading.Lock() + self._dir_ignore_lock = threading.Lock() log.info( "FileTraverser initialised", repo_root=str(self.repo_root), @@ -456,11 +458,34 @@ def _console_script_tables(self) -> ConsoleScriptTables: "repowise.cli.augment_hook:main"``) has no in-repo importer, so without this it reads as unreachable unless its filename happens to match an entry-stem heuristic. + + Double-checked, because the first caller is normally + :meth:`_build_file_info` under the ingestion thread pool — + :func:`~repowise.core.pipeline.incremental.build_repo_graph` maps it + over every path with ~2x cpu_count workers. Unsynchronised, every + worker reaching the ``is None`` check before the first one assigns + starts its own :func:`_collect_console_scripts`, and each of those is a + full :func:`~repowise.core.fs_walk.iter_glob` walk of the repo. On a + 17k-file repo with 28 workers that was ~264s of file-info phase against + ~8s once the value is computed a single time, for identical FileInfos. + + ``functools.cached_property`` is not a substitute: 3.12 dropped its + internal lock (it serialised across instances), so it permits the + duplicate computation this exists to prevent. + + The fast path stays lock-free — once assigned, readers never acquire — + and the assignment publishes a finished tuple, so a reader racing it + sees either ``None`` or the whole object. """ if self._console_scripts is None: - self._console_scripts = _collect_console_scripts( - self.repo_root, prune_nested_git=self._console_scripts_prune_nested - ) + with self._console_scripts_lock: + # Re-check under the lock: a racer may have filled it while + # this thread waited, and recomputing is the bug itself. + if self._console_scripts is None: + self._console_scripts = _collect_console_scripts( + self.repo_root, + prune_nested_git=self._console_scripts_prune_nested, + ) return self._console_scripts @property @@ -562,9 +587,17 @@ def _get_dir_ignore(self, dirpath: Path) -> pathspec.PathSpec: against the immediate child name (see ``_should_skip_dir`` / ``_build_file_info``), consistent with the existing per-directory ``.repowiseIgnore`` handling. + + Read outside the lock and written under it, for the same reason as + :meth:`_console_script_tables`: the callers are ``_build_file_info`` + workers, one per path. A miss here is two ``exists()`` and a small + compile rather than a repo walk, so concurrent misses on one directory + waste little, but they are pure waste — and the directories holding the + most files are the ones every worker reaches at once. """ key = str(dirpath) - if key not in self._dir_ignore_cache: + spec = self._dir_ignore_cache.get(key) + if spec is None: lines: list[str] = [] for name in (".gitignore", self._extra_ignore_filename): ignore_file = dirpath / name @@ -572,8 +605,13 @@ def _get_dir_ignore(self, dirpath: Path) -> pathspec.PathSpec: lines.extend( ignore_file.read_text(encoding="utf-8", errors="ignore").splitlines() ) - self._dir_ignore_cache[key] = _compile_gitignore(lines) - return self._dir_ignore_cache[key] + spec = _compile_gitignore(lines) + with self._dir_ignore_lock: + # setdefault, not assignment: a racer's spec is equivalent, and + # keeping the first published one means callers holding a + # reference always see the cached object. + spec = self._dir_ignore_cache.setdefault(key, spec) + return spec def _should_skip_dir( self, @@ -810,9 +848,11 @@ def _detect_monorepo(self) -> tuple[list[PackageInfo], bool]: if self.dir_chain_skipped(rel_pkg_path): continue seen_paths.add(rel_pkg) - lang = _primary_language_in(pkg_dir, prune_nested_git=prune_nested) - entry_pts = _find_entry_points_in( - pkg_dir, self.repo_root, prune_nested_git=prune_nested + lang, entry_pts = _scan_package_dir( + pkg_dir, + self.repo_root, + prune_nested_git=prune_nested, + is_pruned=self.dir_chain_skipped, ) packages.append( PackageInfo( @@ -1151,41 +1191,60 @@ def _is_console_script_target(rel_path: str, modules: frozenset[str]) -> bool: return False -def _primary_language_in(directory: Path, *, prune_nested_git: bool = True) -> LanguageTag: +def _scan_package_dir( + directory: Path, + repo_root: Path, + *, + prune_nested_git: bool = True, + is_pruned: Callable[[Path], bool], +) -> tuple[LanguageTag, list[str]]: + """Primary language and entry-point paths for one package, in one walk. + + Both answers come off the same pass because they are derived from the same + listing: language from the file extensions, entry points from the + filenames. Read separately they cost two walks of a tree that can be the + largest thing in the repo. + + ``is_pruned`` is the ignore-file layer :func:`~.package_roots. + scan_package_roots` already applies, required rather than optional because + a scan that skips it answers from files nothing indexes. It matters more + here than it does there. + :func:`~repowise.core.fs_walk.walk_repo` skips vendored trees and nested + checkouts but not gitignored ones, so without it this descends into build + output — and unlike a manifest scan, which only matches filenames, language + detection *opens* every file whose extension it does not recognise + (:func:`_detect_by_shebang`). A gitignored build tree is exactly where those + files are, and none of them can be indexed, so the reads buy nothing. + + Skipping them is also the more correct answer: a package's primary language + and its entry points should describe the sources traversal indexes, not + artifacts a build wrote. + """ from repowise.core.fs_walk import walk_repo counts: dict[str, int] = {} + entry_points: list[str] = [] try: - for dirpath, _dirnames, filenames in walk_repo( + for dirpath, dirnames, filenames in walk_repo( directory, prune_nested_git=prune_nested_git ): + # Prune in place so the walk never descends, matching + # scan_package_roots. Candidates are repo-relative because + # dir_chain_skipped tests each level against the repo root. + rel_dir = dirpath.relative_to(repo_root) + dirnames[:] = [d for d in dirnames if not is_pruned(rel_dir / d)] for fname in filenames: + if fname in _ENTRY_POINT_NAMES: + entry_points.append((dirpath / fname).relative_to(repo_root).as_posix()) lang = _detect_language(dirpath / fname) if lang not in ("unknown", "yaml", "json", "markdown", "toml"): counts[lang] = counts.get(lang, 0) + 1 except OSError: pass - if not counts: - return "unknown" - return max(counts, key=lambda k: counts[k]) # type: ignore[return-value] - - -def _find_entry_points_in( - directory: Path, repo_root: Path, *, prune_nested_git: bool = True -) -> list[str]: - from repowise.core.fs_walk import walk_repo - - result: list[str] = [] - try: - for dirpath, _dirnames, filenames in walk_repo( - directory, prune_nested_git=prune_nested_git - ): - for fname in filenames: - if fname in _ENTRY_POINT_NAMES: - result.append((dirpath / fname).relative_to(repo_root).as_posix()) - except OSError: - pass - return sorted(result) + language: LanguageTag = "unknown" + if counts: + language = max(counts, key=lambda k: counts[k]) # type: ignore[assignment] + return language, sorted(entry_points) def _is_nested_git_repo(path: Path) -> bool: diff --git a/tests/unit/ingestion/external_systems/test_cmake.py b/tests/unit/ingestion/external_systems/test_cmake.py index 4671b7c8c..706286858 100644 --- a/tests/unit/ingestion/external_systems/test_cmake.py +++ b/tests/unit/ingestion/external_systems/test_cmake.py @@ -121,3 +121,47 @@ def test_malformed_does_not_raise(tmp_path): # Trailing-open paren is tolerated; either zero targets or the args are # captured but we don't crash. assert isinstance(cm.targets, list) + + +def test_orphan_glob_reads_the_snapshot_when_given_one(tmp_path): + """A snapshot answers the orphan sweep, and answers it the same way. + + ``discover_cmake_reactor`` falls back to a repo-wide glob for + ``CMakeLists.txt`` files no ``add_subdirectory`` chain reached. That glob + is the one every resolver used to pay for separately; it now reads a + shared :class:`WalkSnapshot` when the caller has one. Same files either + way — a caller must not be able to tell which path served it. + """ + from repowise.core.fs_walk import WalkSnapshot + + _write(tmp_path, "CMakeLists.txt", "project(top)") + _write(tmp_path, "orphan/CMakeLists.txt", "add_library(orphan STATIC o.cc)") + + live = cmake.discover_cmake_reactor(tmp_path) + shared = cmake.discover_cmake_reactor(tmp_path, snapshot=WalkSnapshot(tmp_path)) + + assert {f.path for f in shared} == {f.path for f in live} + assert "orphan/CMakeLists.txt" in {f.path for f in shared} + + +def test_orphan_glob_honours_the_snapshot_nested_repo_setting(tmp_path): + """The snapshot decides whether a nested checkout is in scope. + + Pruning nested git repos is the default, so an orphan under one stays + hidden. A snapshot built with ``prune_nested_git=False`` — what a repo + indexed with ``--include-nested-repos`` gets — surfaces it, which is the + point of sharing the traverser's own boundary rather than restating it. + """ + from repowise.core.fs_walk import WalkSnapshot + + _write(tmp_path, "CMakeLists.txt", "project(top)") + _write(tmp_path, "vendored/.git", "gitdir: elsewhere") + _write(tmp_path, "vendored/CMakeLists.txt", "add_library(vend STATIC v.cc)") + + pruned = cmake.discover_cmake_reactor(tmp_path, snapshot=WalkSnapshot(tmp_path)) + included = cmake.discover_cmake_reactor( + tmp_path, snapshot=WalkSnapshot(tmp_path, prune_nested_git=False) + ) + + assert "vendored/CMakeLists.txt" not in {f.path for f in pruned} + assert "vendored/CMakeLists.txt" in {f.path for f in included} diff --git a/tests/unit/ingestion/test_traverser.py b/tests/unit/ingestion/test_traverser.py index 6eeb0d5a0..923994cba 100644 --- a/tests/unit/ingestion/test_traverser.py +++ b/tests/unit/ingestion/test_traverser.py @@ -2,6 +2,8 @@ from __future__ import annotations +import time +from concurrent.futures import ThreadPoolExecutor from pathlib import Path import pytest @@ -12,6 +14,7 @@ _compile_gitignore, _detect_language, _is_generated, + _scan_package_dir, ) @@ -1109,3 +1112,138 @@ def test_generated_suffix_still_wins(self, tmp_path: Path) -> None: p = tmp_path / "api_pb2.py" p.write_text("x = 1\n") assert _is_generated(p) is True + + +class TestConcurrentLazyInit: + """The lazy scans behind ``_build_file_info`` run once, not once per worker. + + ``build_repo_graph`` maps ``_build_file_info`` over every path with ~2x + cpu_count workers, so an unsynchronised ``is None`` check is not a wasted + branch — it is one full repo walk per worker that wins the race. + """ + + def test_console_script_tables_is_collected_once(self, tmp_path: Path) -> None: + calls = 0 + real = traverser_mod._collect_console_scripts + + def counting(repo_root, **kwargs): + nonlocal calls + calls += 1 + # Widen the window a real walk would occupy, so an unsynchronised + # implementation reliably loses the race instead of flaking. + time.sleep(0.05) + return real(repo_root, **kwargs) + + traverser_mod._collect_console_scripts = counting + try: + tv = FileTraverser(tmp_path) + with ThreadPoolExecutor(max_workers=16) as pool: + results = list(pool.map(lambda _: tv._console_script_tables(), range(16))) + finally: + traverser_mod._collect_console_scripts = real + + assert calls == 1 + # Every caller sees the one published object, not a private copy. + assert all(r is results[0] for r in results) + + def test_dir_ignore_cache_publishes_one_spec_per_directory(self, tmp_path: Path) -> None: + (tmp_path / "pkg").mkdir() + (tmp_path / "pkg" / ".gitignore").write_text("build/\n", encoding="utf-8") + tv = FileTraverser(tmp_path) + + with ThreadPoolExecutor(max_workers=16) as pool: + specs = list(pool.map(lambda _: tv._get_dir_ignore(tmp_path / "pkg"), range(16))) + + assert all(s is specs[0] for s in specs) + + def test_pre_seeded_root_entry_survives_concurrent_readers(self, tmp_path: Path) -> None: + tv = FileTraverser(tmp_path) + seeded = tv._dir_ignore_cache[str(tmp_path)] + + with ThreadPoolExecutor(max_workers=8) as pool: + list(pool.map(lambda _: tv._get_dir_ignore(tmp_path), range(8))) + + assert tv._dir_ignore_cache[str(tmp_path)] is seeded + + +def _never_pruned(_rel_dir: Path) -> bool: + """The old helpers applied no ignore layer; this is that, spelled out.""" + return False + + +class TestPackageScanPruning: + """The package scan answers from the files traversal actually indexes. + + ``_scan_package_dir`` reads a package's primary language and its entry + points off one walk. Without the traverser's own boundary test it reads + them off directories that are never indexed, so a package could be + described entirely by files absent from the index. + """ + + def _pkg(self, tmp_path: Path, gitignore: str) -> Path: + (tmp_path / ".gitignore").write_text(gitignore, encoding="utf-8") + pkg = tmp_path / "pkg" + (pkg / "src").mkdir(parents=True) + (pkg / "package.json").write_text('{"name": "p"}', encoding="utf-8") + (pkg / "src" / "index.ts").write_text("export const a = 1;\n", encoding="utf-8") + return pkg + + def test_gitignored_output_no_longer_decides_the_language(self, tmp_path: Path) -> None: + pkg = self._pkg(tmp_path, "pkg/out/\n") + out = pkg / "out" + out.mkdir() + # Enough generated JS to outvote the single real source. + for i in range(5): + (out / f"chunk{i}.js").write_text("var a=1;\n", encoding="utf-8") + + tv = FileTraverser(tmp_path) + assert not any("/out/" in fi.path for fi in tv.traverse()) + + unpruned, _ = _scan_package_dir(pkg, tmp_path, is_pruned=_never_pruned) + pruned, _ = _scan_package_dir(pkg, tmp_path, is_pruned=tv.dir_chain_skipped) + assert unpruned == "javascript" + assert pruned == "typescript" + + def test_entry_points_come_only_from_indexed_directories(self, tmp_path: Path) -> None: + pkg = self._pkg(tmp_path, "pkg/out/\n") + out = pkg / "out" + out.mkdir() + (out / "index.html").write_text("\n", encoding="utf-8") + (pkg / "index.html").write_text("\n", encoding="utf-8") + + tv = FileTraverser(tmp_path) + _, unpruned = _scan_package_dir(pkg, tmp_path, is_pruned=_never_pruned) + _, pruned = _scan_package_dir(pkg, tmp_path, is_pruned=tv.dir_chain_skipped) + + assert "pkg/out/index.html" in unpruned + assert "pkg/out/index.html" not in pruned + assert "pkg/index.html" in pruned + + def test_a_committed_build_dir_is_excluded_too(self, tmp_path: Path) -> None: + """Not just gitignored trees. ``dist`` is in ``_BLOCKED_DIRS``, so the + traverser never indexes it even when it is committed — and a directory + nothing indexes must not name the package's language.""" + pkg = self._pkg(tmp_path, "node_modules/\n") # dist/ deliberately tracked + dist = pkg / "dist" + dist.mkdir() + # Outnumber the one real source rather than tie with it: `walk_repo` + # does not sort dirnames, so on a tie `max(counts, key=...)` returns + # whichever language the filesystem happened to yield first. That made + # this pass locally and fail on CI. + for i in range(5): + (dist / f"chunk{i}.js").write_text("var a=1;\n", encoding="utf-8") + + tv = FileTraverser(tmp_path) + assert not any("/dist/" in fi.path for fi in tv.traverse()) + + unpruned, _ = _scan_package_dir(pkg, tmp_path, is_pruned=_never_pruned) + pruned, _ = _scan_package_dir(pkg, tmp_path, is_pruned=tv.dir_chain_skipped) + assert unpruned == "javascript" + assert pruned == "typescript" + + def test_a_package_with_nothing_indexed_reports_unknown(self, tmp_path: Path) -> None: + """The one shape where the answer gets smaller rather than sharper.""" + pkg = self._pkg(tmp_path, "pkg/src/\n") + tv = FileTraverser(tmp_path) + language, _ = _scan_package_dir(pkg, tmp_path, is_pruned=tv.dir_chain_skipped) + assert language == "unknown"