Skip to content

Commit 699a9a1

Browse files
authored
Reduce scan startup time in large repositories (#301)
* perf(core): discover manifests in a single filesystem walk find_files() started a separate recursive rglob traversal for every expanded manifest pattern, so a scan re-walked each root once per pattern and only filtered excluded directories after descending into them. Replace that with one os.walk() per scan root: - Expand and case-fold all active patterns once, then match in memory. - Prune excluded directories, including .git, before descending. - Reject non-manifests on the basename alone (one set lookup plus one compiled glob alternation) before building a relative path or running a path match. - Cache supported manifest patterns per Core instance, but only when the API lookup succeeds, so a transient failure does not pin the run to the smaller local fallback pattern set. - Emit INFO durations for organization setup, pattern retrieval and discovery, with files/directories visited, directories pruned and manifests found. Matching behaviour is unchanged apart from intentionally excluding .git metadata. Adds parity tests against the previous rglob implementation for every built-in ecosystem and pattern, covering case-insensitivity, brace expansion, nested patterns, dot-directories, exclusions, inclusions, symlinks, excluded ecosystems, multiple roots, sorting and deduplication, plus an opt-in benchmark that asserts old/new result equality on a synthetic large-monorepo fixture. Ref: CE-379 * perf(git): fetch only the refs a comparison needs Git.__init__() ran `git fetch --all` on every invocation, pulling every remote branch and tag before changed-file detection even began. Resolve commit and branch metadata locally instead, and for pull-request comparisons prefer refs already present in the checkout, fetching a single base or head ref only when it is missing. Also recognises Buildkite's native BUILDKITE_COMMIT, BUILDKITE_BRANCH, BUILDKITE_PULL_REQUEST and BUILDKITE_PULL_REQUEST_BASE_BRANCH so Buildkite jobs can calculate a complete base-to-head changed-file range without mapping their environment onto GitHub Actions variable names. Buildkite is checked before GitHub because some pipelines deliberately export GitHub-compatible variables. Adds INFO durations for Git initialisation, changed-file detection and each fetch, including the ref requested and why. Existing GitHub Actions, GitLab CI, Bitbucket Pipelines and local behaviour is preserved; tests cover local-ref preference, absence of an unconditional fetch, the targeted-fetch fallback, all four CI providers, and non-PR and detached-HEAD execution. Ref: CE-379 * feat(buildkite): derive GitHub comment context natively `--scm github` read its configuration solely from GITHUB_* variables, so Buildkite users had to shim every one of them to get PR comments. Fall back to Buildkite's own variables when the GITHUB_* equivalents are absent: PR number, commit, branch, checkout path, commit message, build creator, and owner/repository parsed from BUILDKITE_REPO (preferring the pipeline repository over a contributor's fork). Explicit GITHUB_* and PR_NUMBER values still take priority, and GitHub Enterprise remains configurable via GITHUB_API_URL. A running Buildkite PR build maps to the supported `synchronize` comment path, and a non-PR build maps to `push`, so event routing is unchanged. Default-branch detection requires an actual branch name rather than treating two unset variables as a match, which would otherwise mark any build as the default branch and overwrite the repository baseline. Ref: CE-379 * refactor(cli): reuse sub-path discovery results and clarify scan routing The --sub-path routing pre-check walked every selected path to decide whether any manifests existed, then discarded the result so scan creation walked the same paths again. Retain and reuse it. Apply --excluded-ecosystems before the pre-check rather than after, so every find_files() call in a run sees the same ecosystem filter. Add an INFO duration for CLI run registration, and replace the "No Manifest files changed" line with wording that describes the decision being made: no supported manifest was detected in the changed-file set, so a full report is created. Scan-routing semantics are unchanged. Ref: CE-379 * docs(changelog): note faster local scan setup for large repositories Ref: CE-379 * chore(release): bump version to 2.6.5 Bumped via .hooks/sync_version.py so __init__.py, pyproject.toml and uv.lock stay in sync, and moved the changelog entry under a 2.6.5 heading. Ref: CE-379 * fix(ci): build the Docker preview from the checked-out workspace The publish-docker job downloads the built wheel to ./dist, but the build step omitted `context`, so docker/build-push-action used its default Git context. Buildx then cloned the repository as the build context, where ./dist does not exist, and `COPY dist/socketsecurity-*.whl` failed with "lstat /dist: no such file or directory". Set `context: .` so the build uses the workspace the artifact was downloaded into. This also makes the job's existing trust boundary hold as documented: the context is now the default-branch checkout rather than the pull-request ref, so Dockerfile.preview is read from trusted code and the pull request still enters the image only through the built wheel. Pre-existing; the TestPyPI half of the workflow is unaffected. * ci(preview): build Docker previews for arm64 as well as amd64 The preview image was amd64-only while the release and stable images are built for linux/amd64,linux/arm64, so a preview tag could not stand in for socketdev/cli:latest on arm64 hosts without emulation. Match the release arch matrix and enable QEMU so the arm64 layer can be built on an amd64 runner. Previews are opt-in via label, so the extra build time is an acceptable tradeoff for making the tag a drop-in replacement. * perf(diff): tighten diff-scan poll ceiling and make its timing attributable A finished comparison could sit unobserved for up to 30s between polls, which is dead time on every PR job. Lower the ceiling to 10s: a multi-minute comparison costs roughly 2x the polls while cutting worst-case dead time to 10s. Diff scans now log their ID, poll count, and the wait before the final poll at INFO. Previously the ID was debug-only, so a slow comparison in a customer CI log could not be tied back to a server-side diff scan, and there was no way to tell backend comparison time apart from time the result spent ready-but-unpolled. Also document the diff-scans token scopes. A token missing them still completes the scan, silently falling back to the streaming comparison, which differs in both transport and payload (cached diff-scan responses always embed per-package license details; the streaming path requests a lean payload). Ref: CE-379 * feat(diff): log the diff report URL and cover discovery memory PR/MR runs logged the head and new scan IDs but no link to the result, so a CI log gave no way to reach the report. Log the diff report URL where it is computed, so every diff flow gets it rather than only the full-scan-only branches. Also add a regression test asserting manifest discovery's peak allocation stays bounded by the widest single directory and the result set rather than by repository size. Measured against the per-pattern rglob approach this replaced, on a tree of 59,300 files including one 50,000-entry directory: 3.25 MB peak vs 10.72 MB. os.walk keeps a list of names per directory where rglob materialised DirEntry objects and a Path per candidate, so the single-pass walk allocates strictly less. Ref: CE-379 * docs(diff): record verified cached diff-scan param behaviour Probed the live API against an existing diff scan to confirm what the polling path can and cannot ask for: - omit_license_details is ignored when cached=true, as the existing comment said. License fields remain in the response. - omit_unchanged IS honored and removes unchanged artifacts entirely, measured at ~1.1 KB per artifact (225,542 B -> 78,003 B when dropping 135 of 192 artifacts). Record why the CLI still does not send omit_unchanged: unchanged artifacts feed diff.unchanged_alerts, which create_security_comment_gitlab and the FOSSA compat issue list read unconditionally, not only under --strict-blocking. Omitting them would silently shrink those outputs, so this needs proper gating in its own change rather than a param tweak here. Ref: CE-379 * perf(diff): skip unchanged artifacts when no output reads them Cached diff-scan responses embed every unchanged artifact at roughly 1 KB each. On a large dependency tree that is nearly the whole response — measured at ~11 MB for a tree with ~10k unchanged packages — downloaded, deserialised into Package objects and then discarded on every pull request. omit_unchanged is honored by the API (unlike omit_license_details, which cached responses ignore), so request it whenever no enabled output reads that half of the comparison. Verified against the live API through the SDK: 192 artifacts -> 57. Every consumer is behind an opt-in flag, so the gate is centralised in Core._requires_unchanged_artifacts with the reasoning recorded there: - --strict-blocking blocks on pre-existing issues via diff.unchanged_alerts - --enable-gitlab-security includes them in the dependency scanning report - --generate-license enumerates diff.packages, which must list every dependency - --legal-format fossa reports all currently-present issues Diff.to_dict serialises them too but has no callers. When cli_config is absent the caller is unknown, so the full payload is kept. Tests parametrise over every flag in that list so a new reader of diff.unchanged_alerts or diff.packages cannot be added without also updating the gate. The completion log reports omit_unchanged so it is visible whether the optimisation engaged on a given run. Ref: CE-379 * chore(release): bump version to 2.6.6 * fix: always filter diff scan artifacts * fix: preserve directory-only manifest patterns Address peer review feedback by retaining pathlib.rglob trailing-slash semantics, trimming the release notes, and removing redundant implementation commentary.
1 parent 13651d7 commit 699a9a1

18 files changed

Lines changed: 1768 additions & 173 deletions

.github/workflows/pr-preview.yml

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,17 +254,25 @@ jobs:
254254
- name: Set up Docker publishing
255255
uses: ./.github/actions/setup-docker
256256
with:
257-
enable-qemu: "false"
257+
# QEMU is required to build the arm64 layer on an amd64 runner.
258+
enable-qemu: "true"
258259
dockerhub-username: ${{ secrets.DOCKERHUB_USERNAME }}
259260
dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }}
260261

261262
- name: Build and push Docker preview
262263
uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0
263264
with:
265+
# Build from the checked-out workspace, not the default Git context. The
266+
# wheel is only present here as a downloaded artifact, and this also keeps
267+
# Dockerfile.preview on the default-branch checkout rather than the PR ref.
268+
context: .
264269
file: Dockerfile.preview
265270
push: true
266271
pull: true
267-
platforms: linux/amd64
272+
# Match the arch matrix of the release and stable images so a preview is a
273+
# drop-in replacement for socketdev/cli:latest on arm64 runners too. The
274+
# arm64 layer builds under emulation, so expect roughly double the runtime.
275+
platforms: linux/amd64,linux/arm64
268276
tags: socketdev/cli:pr-${{ needs.context.outputs.pr_number }}
269277
build-args: |
270278
SDK_PREVIEW_VERSION=${{ inputs.sdk_preview_version }}

CHANGELOG.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,26 @@
11
# Changelog
22

3+
## 2.6.6
4+
5+
### Changed: faster local scan setup for large repositories
6+
7+
- Manifest discovery now uses one filesystem walk per scan root and prunes
8+
excluded directories before descent.
9+
- Pull request scans use local Git refs first and fetch only missing history.
10+
Buildkite pull request metadata is now supported directly.
11+
- Supported manifest patterns are cached per invocation, and discovered
12+
manifests are reused during scan creation.
13+
- Added timings for initialization, Git operations, changed-file detection,
14+
pattern lookup, and manifest discovery.
15+
16+
### Changed: scan comparisons no longer fetch unused artifacts
17+
18+
- Scan comparisons omit unchanged artifacts unless an enabled output needs them.
19+
- Diff scans poll more frequently and log identifiers and timing details for
20+
easier troubleshooting.
21+
- Documented the `diff-scans:create`, `diff-scans:list` and `full-scans:list`
22+
token scopes required by the optimized comparison path.
23+
324
## 2.6.5
425

526
### Changed: bump pinned @coana-tech/cli to 15.10.16

benchmarks/manifest_discovery.py

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
#!/usr/bin/env python3
2+
"""Compare legacy per-pattern rglob discovery with the single-pass walker.
3+
4+
This is an opt-in developer benchmark, not a timing assertion in the test
5+
suite. It creates a synthetic monorepo so filesystem or CI-agent changes do not
6+
make regular tests flaky.
7+
"""
8+
9+
import argparse
10+
import tempfile
11+
import time
12+
from pathlib import Path
13+
from types import SimpleNamespace
14+
from unittest.mock import MagicMock
15+
16+
from socketsecurity.core import Core
17+
from socketsecurity.core.socket_config import SocketConfig
18+
from socketsecurity.core.utils import socket_globs
19+
20+
21+
def seed_tree(root: Path, directories: int, files_per_directory: int) -> None:
22+
for directory_index in range(directories):
23+
directory = root / "packages" / f"package-{directory_index:05d}"
24+
directory.mkdir(parents=True)
25+
(directory / "package.json").write_text("{}\n", encoding="utf-8")
26+
for file_index in range(files_per_directory):
27+
(directory / f"source-{file_index:03d}.txt").write_text(
28+
"not a manifest\n",
29+
encoding="utf-8",
30+
)
31+
32+
# These trees model the expensive directories that the new walker prunes
33+
# before descent rather than visiting once for every manifest pattern.
34+
for excluded in (".git/objects", "node_modules/example", ".venv/site-packages"):
35+
directory = root / excluded
36+
directory.mkdir(parents=True)
37+
for index in range(files_per_directory * 10):
38+
(directory / f"object-{index:05d}").write_text("x", encoding="utf-8")
39+
40+
41+
def legacy_discover(root: Path) -> set[str]:
42+
results = set()
43+
excluded_dirs = SocketConfig(api_key="benchmark").excluded_dirs
44+
for ecosystem_patterns in socket_globs.values():
45+
for details in ecosystem_patterns.values():
46+
for pattern in Core.expand_brace_pattern(details["pattern"]):
47+
insensitive = Core.to_case_insensitive_regex(pattern)
48+
for candidate in root.rglob(insensitive):
49+
if candidate.is_file() and not Core.is_excluded(
50+
str(candidate),
51+
excluded_dirs,
52+
):
53+
results.add(candidate.as_posix())
54+
return results
55+
56+
57+
def new_core() -> Core:
58+
core = Core.__new__(Core)
59+
core.config = SocketConfig(api_key="benchmark")
60+
core.cli_config = SimpleNamespace(exclude_paths=None)
61+
core.sdk = MagicMock()
62+
core._supported_patterns = socket_globs
63+
return core
64+
65+
66+
def timed(function, root: Path) -> tuple[set[str], float]:
67+
start = time.perf_counter()
68+
results = set(function(root))
69+
return results, time.perf_counter() - start
70+
71+
72+
def main() -> None:
73+
parser = argparse.ArgumentParser()
74+
parser.add_argument("--directories", type=int, default=500)
75+
parser.add_argument("--files-per-directory", type=int, default=20)
76+
args = parser.parse_args()
77+
78+
with tempfile.TemporaryDirectory(prefix="socket-manifest-benchmark-") as temp:
79+
root = Path(temp)
80+
seed_tree(root, args.directories, args.files_per_directory)
81+
legacy_results, legacy_seconds = timed(legacy_discover, root)
82+
new_results, new_seconds = timed(
83+
lambda path: new_core().find_files(str(path)),
84+
root,
85+
)
86+
87+
if legacy_results != new_results:
88+
raise SystemExit(
89+
"Manifest result mismatch: "
90+
f"legacy={len(legacy_results)}, single_pass={len(new_results)}"
91+
)
92+
93+
speedup = legacy_seconds / new_seconds if new_seconds else float("inf")
94+
print(f"Manifests: {len(new_results)}")
95+
print(f"Legacy per-pattern rglob: {legacy_seconds:.3f}s")
96+
print(f"Single-pass walk: {new_seconds:.3f}s")
97+
print(f"Speedup: {speedup:.1f}x")
98+
99+
100+
if __name__ == "__main__":
101+
main()

docs/ci-cd.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,19 @@ steps:
8181
SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}"
8282
```
8383
84+
The CLI reads Buildkite's native `BUILDKITE_COMMIT`, `BUILDKITE_BRANCH`,
85+
`BUILDKITE_PULL_REQUEST`, and `BUILDKITE_PULL_REQUEST_BASE_BRANCH` variables.
86+
For pull-request builds, ensure the checkout contains the base branch and the
87+
checked-out head commit. The CLI uses those local refs first and performs a
88+
targeted fetch only when a required ref or its comparison history is missing;
89+
it does not fetch every remote ref and tag during startup.
90+
91+
When `--scm github` is used from Buildkite, the CLI also derives GitHub comment
92+
context from `BUILDKITE_REPO`, `BUILDKITE_BUILD_CHECKOUT_PATH`, and the variables
93+
above. Set `GH_API_TOKEN` to a GitHub token with the required repository access.
94+
GitHub Enterprise users should also set `GITHUB_API_URL`; GitHub.com defaults to
95+
`https://api.github.com`.
96+
8497
#### Merge-base baselines in Buildkite (dynamic pipelines)
8598

8699
Notes for using `--base-commit-sha` (see the

docs/troubleshooting.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,34 @@
11
# Troubleshooting
22

3+
## API token scopes for scan comparisons
4+
5+
PR/MR runs compare the new scan against the repository's head scan. That comparison
6+
first uses the diff-scans endpoints, which require an organization token with these
7+
scopes in addition to whatever the scan itself needs:
8+
9+
- `diff-scans:create`
10+
- `diff-scans:list`
11+
- `full-scans:list`
12+
13+
If the token is missing them the scan still succeeds, so this is easy to miss. The only
14+
signal is a warning, after which the CLI falls back to the older streaming comparison:
15+
16+
```
17+
Diff scan comparison failed with APIAccessDenied(Insufficient permissions), falling back to the streaming scan comparison
18+
```
19+
20+
Grant the scopes to use the diff-scans path. It polls with short, bounded requests
21+
rather than holding one connection open while the backend computes, which is what lets
22+
large comparisons survive network idle timeouts — notably Azure NAT gateways, which
23+
reap idle connections after four minutes and surface as an intermittent
24+
`ConnectionResetError`.
25+
26+
The two paths can take noticeably different amounts of time on the same repository,
27+
because cached diff-scan responses always embed per-package license details while the
28+
streaming comparison requests a lean payload. On a large dependency tree, compare the
29+
`Diff scan comparison ready in ...` timing against the `Diff Report Gathered in ...`
30+
total before assuming either path is at fault.
31+
332
## Common gotchas
433

534
- In diff scope, `--strict-blocking` uses a stricter alert set (`new + unchanged`) for blocking checks and diff-based output selection.

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ build-backend = "hatchling.build"
66

77
[project]
88
name = "socketsecurity"
9-
version = "2.6.5"
9+
version = "2.6.6"
1010
requires-python = ">= 3.11"
1111
license = {"file" = "LICENSE"}
1212
dependencies = [

socketsecurity/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
__author__ = 'socket.dev'
2-
__version__ = '2.6.5'
2+
__version__ = '2.6.6'
33
USER_AGENT = f'SocketPythonCLI/{__version__}'

0 commit comments

Comments
 (0)