Scan package archives across cores instead of one at a time - #9024
Conversation
The three pip scan-packages shards cost 21.7 runner-minutes per push
(studio 10.17, extras 6.63, hf-stack 4.88), the bulk of Security audit.
Almost none of that is network. Timing the hf-stack shard locally:
Scanning 49 package(s) (with transitive deps)...
0.0s
Downloaded 110 archive(s).
9.7s
Summary
315.6s
pip download is 9.7s of 315.6s. The rest is the serial loop calling
scan_archive on each archive, which is pure CPU: regex over decompressed
archive members. Every archive is independent.
Pooled at 4 workers the same shard runs in 97.7s, and the report is
byte-identical to the serial one. imap with chunksize=1 yields in
submission order so findings are assembled exactly as before; chunksize=1
is also what makes next(timeout=) available at all, since above 1 CPython
returns a bare generator with no timeout support.
The archive-limit [WARN] lines are captured from the worker and replayed
in task order rather than landing wherever a worker reached them, so two
runs of the same input produce identical logs.
Default is min(4, cpu count), matching the runner, with --jobs to override
and --jobs 1 to force the old path. No workflow change needed.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc282269e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # A worker died (OOM or segfault on a hostile archive). | ||
| # Without this the iterator blocks forever and the job | ||
| # only ends at the workflow timeout, with no reason given. | ||
| raise SystemExit( |
There was a problem hiding this comment.
When a worker dies or an archive takes longer than 900 seconds, raising SystemExit with a string makes Python exit with status 1. This conflicts with the scanner's documented contract, where 1 means malicious CRITICAL/HIGH findings and an incomplete scan uses 2, so callers can misclassify a coverage/infrastructure failure as a detected threat and the normal SCAN INCOMPLETE reporting is skipped. Terminate this path with status 2 after emitting the stall message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in b6736da. Confirmed against the language reference: a SystemExit whose value is not an int or None has "the object's value printed and the exit status is one", and 1 is this scanner's code for non-baselined CRITICAL/HIGH findings, so a dead worker was reporting an infrastructure failure as a detected threat.
Routed the stall through the existing SCAN INCOMPLETE block rather than exiting 2 inline, so it also gets the report explaining that coverage was lost. The header counts download and scan failures together now, so it no longer calls a stall a pip download failure. Regression test drives a pool whose first next() raises TimeoutError and asserts exit 2 plus the report; reverting to SystemExit fails it.
raise SystemExit(<string>) prints the string and exits 1, per the language reference: "if it has another type (such as a string), the object's value is printed and the exit status is one". 1 already means "non-baselined CRITICAL or HIGH findings detected" in this scanner's documented contract, and 2 means an incomplete scan. So a dead worker reported an infrastructure failure as a detected threat, and skipped the SCAN INCOMPLETE block that tells the operator coverage was lost. The stall is now recorded beside the pip-download failures and reported by that same block, so it exits 2 and says why. The header counts both kinds, so it no longer calls a scan stall a "pip download failure". Regression test drives a pool whose first next() raises TimeoutError and asserts exit 2 plus the report. Reverting to SystemExit fails it.
for more information, see https://pre-commit.ci
|
@codex review |
1 participant
The three
pip scan-packagesshards cost 21.7 runner-min per push, the bulk of Security audit:Almost none of that is network
Timing the hf-stack shard locally, same inputs the workflow builds:
Scanning 49 package(s) (with transitive deps)...Downloaded 110 archive(s).Summary: 161 MEDIUMpip downloadis 9.7s of 315.6s. The remaining 306s is the serial loop callingscan_archiveon each archive, which is pure CPU (regex over decompressed archive members), one core at a time. Every archive is independent of every other.I looked at caching the downloads first and decided against it: the download is 3% of the cost, and putting a mutable cache in front of a supply-chain scanner's inputs is the wrong trade for 9 seconds.
Change
Pool the scan at
min(4, cpu count)workers, which is 4 onubuntu-latest.Projected across the three shards: 21.7 -> ~7 runner-min per push. No workflow change is needed; the default matches the runner.
--jobs 1forces the old path.Why this is safe
The output is byte-identical to serial, not merely equivalent. Diffing the full 928-line report from both runs of the hf-stack shard:
Two things make that true rather than lucky:
imap(..., chunksize=1)yields in submission order, so findings are appended in the same order the serial loop appended them.chunksize=1is also what makesnext(timeout=)work at all: above 1, CPython'sPool.imapreturns a bare generator with no timeout support (Lib/multiprocessing/pool.py).[WARN]lines are emitted from deep insideiter_archive_filesto stderr. Left alone they land in the parent's stream whenever a worker happens to reach them, so two runs of identical input produce logs differing in where those lines sit. They are captured in the worker and replayed by the caller in task order.A dead worker would otherwise hang
imapforever and the job would only end at the workflow timeout with no reason given, so the read is bounded at 900s per archive and exits with the count scanned so far.Tests
Two added to
tests/security/test_scan_packages.py(119 pass, was 117).The interesting one drives
main()over the same corpus at--jobs 1and--jobs 4and demands byte-identical stdout and stderr. It took two attempts to make it mean anything. My first version scanned the three committed fixtures and caught only one of three mutations:imap->imap_unorderedchunksize1 -> 3imap_unorderedwas missed because three instant archives come back in submission order anyway, so the ordering claim was unearned. The warning replay was missed because none of the fixtures trip the size cap, so that assertion compared""to"". The corpus is now 24 archives plus one whose declared member size exceeds the per-file cap (70 MB of zeros, which deflates to a few KB, so it is written at test time rather than committed).